Repository navigation
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the full github/main...HEAD diff and the surrounding snapshot store, boot verification, connection-time capability refresh, RPC consumers, and relevant history. I also checked the repository rules and Runtime context, backward compatibility with older/stale snapshot rows, and that the tests add coverage without weakening existing assertions. The revised persistence path keeps prior capabilities while replacing transient verdict fields, retries failed records on later boots, and prevents a stale authentication result from disabling an edited row. I found no concrete failure to raise.
Verification: the first uv run pytest tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q attempt failed during shared fixture setup because the fresh environment lacked raven_everos; after uv sync --frozen --extra dev --dev, the same command passed with 525 tests. uv run ruff check and uv run ruff format --check passed for all four changed files, and git diff --check github/main...HEAD passed.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the delta from f80d54e2716c and rechecked the full diff. The only new change makes DNS deterministic in the research-tool test module: resolver-failure cases override the fixture, literal private addresses still take the validator's direct-IP path, and the separate security-network tests continue to cover hostnames resolving to private addresses. It does not change production code or the previously reviewed authentication fix.
For this revision I covered the diff, affected validator and web-tool callers, test history, backward compatibility, test-strength preservation, and the repository's source-language, test-naming, domain-context, and architecture constraints. I found no concrete failure.
Verification: uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q passed with 721 tests. Ruff check and format check passed for all five changed files, and git diff --check github/main...HEAD passed.
|
No blockers in the reviewed scope. Scope: I reviewed the full PR diff (5 changed files), including tests and documentation, and traced affected callers and contracts at Checked capability snapshot persistence, fingerprint/stale handling, manual and boot verification, failure retries, registry refresh, and RPC/Web/TUI authentication consumers. The latest delta only makes DNS deterministic in the research-tool tests: resolver-failure tests override it, literal private-address checks stay active, and the separate network-security suite covers private DNS results. Production authentication behavior is unchanged by that delta. Verification (native Windows, Python 3.12, isolated source snapshots; local JUnit-output options omitted below):
The original three-suite run at the previous reviewed head gave 467 passed, 57 failed, and 1 skipped; the merge-base control had the identical 57 failing IDs (457 passed, 1 skipped). The failures concern existing Windows executable/path and process-group behavior. The latest production diff is unchanged; the new-head run above also exercises the deterministic DNS fixture and the separate network-security tests. No new finding was identified.
|
|
Windows compatibility review: no new issue found in the inspected scope. Reviewed head Native Windows / Python 3.13.12, in a detached temporary checkout with uv run pytest tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_agents_research_tools.py -k 'research or boot_replaces or manual_test_merges or credential_verdict' -n 0 -x --tb=short --basetemp "$reviewRunDir/pr915/tests"Result: 99 passed, 205 deselected. ACP verification and HTTP/DNS are substituted in this selection; no live agent login or external network service was tested. This scoped result does not establish full Windows compatibility. |
74cddde to
00f87b7
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This head rewrites the two branch commits but has the exact same tree (3b22dd265c0e509b142c3aaa5dd021bf3585cf2f) and merge base as the previously reviewed revision. I confirmed there is no file delta from 74cdddeaa311 and rechecked the full PR diff, commit history, project constraints, callers, backward compatibility, architecture boundaries, and test-strength considerations; nothing changes the prior clean assessment.
Verification on this checkout: uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q passed with 721 tests. Ruff check and format check passed for all five changed files, and git diff --check github/main...HEAD passed.
|
Windows compatibility re-review: no new issue found in the inspected scope. Reviewed the complete 5-file PR diff at Git checks: source tree identical to previously reviewed Native Windows / Python 3.13.12, detached checkout, PR head: uv run pytest 'tests/test_subagent_probe.py' 'tests/test_rpc_subagents.py' 'tests/test_agents_research_tools.py' -k 'research or boot_replaces or manual_test_merges or credential_verdict' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr915\unit-pytest'
# 99 passed, 205 deselected in 11.05s; exit 0Limits: ACP verification and HTTP/DNS are substituted in this selection; no live agent login, external network service or full repository suite was tested. Previously reported broader Windows process/path failures were not remeasured here. This scoped result does not establish full Windows compatibility. |
A new non-authentication failure could leave an old needs_auth flag persisted, and a stale snapshot could still disable the edited agent's connection control. Carry current failure flags when preserving prior capabilities, recheck previously failed rows on subsequent boots, and omit stale authentication verdicts from the listing RPC.
Use deterministic DNS beside the mocked HTTP transport while retaining the real URL validation. Explicit resolver-failure tests keep their own overrides, and the strict idle ceiling remains enabled.
The two docstrings still described verifying only missing or stale rows. A row whose last verification failed is measured again at every start, so a timeout is not a permanent verdict.
00f87b7 to
f75ce7c
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the rebase onto 562c8457c790 and the new documentation commit. The newer target adds research-tool cases in the file carrying the DNS fixture; those cases remain correctly isolated and pass. The updated snapshot-verification docstrings now match the implementation's retry-on-next-start behavior. I rechecked the complete scoped diff, affected callers and history, project rules and context, backward compatibility, architecture boundaries, and whether the rebased tests lost coverage; I found no concrete failure.
Verification: uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q passed with 724 tests. Ruff check and format check passed for all five changed files, and git diff --check github/main...HEAD passed.
|
Windows compatibility re-review: no new issue found in the inspected scope. Scope: the complete 5-file PR diff at head Native Windows, Python 3.13.12, isolated detached checkout and user/Raven/temp directories. Dependencies were synced with uv run pytest tests/test_subagent_probe.py::test_boot_replaces_an_auth_verdict_and_retries_until_ready tests/test_subagent_probe.py::test_manual_test_merges_current_failure_flags_with_old_capabilities tests/test_rpc_subagents.py::test_a_row_carries_the_credential_verdict_its_snapshot_measured -n 0 -x --tb=short --basetemp "$R/checks/915-regressions/pytest" --junitxml "$R/checks/915-regressions/junit.xml"
# 11 passed; real temporary snapshot persistence, simulated verifier results.
uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py -n 0 -x --tb=short --basetemp "$R/checks/915-research/pytest" --junitxml "$R/checks/915-research/junit.xml"
# 199 passed; mocked HTTP/DNS and URL-validation cases.No real third-party ACP authentication, complete probe/ACP suite or full repository suite was run. The unchanged process-launch/teardown paths are not claimed fixed by this PR; Windows ACP teardown is addressed separately in #908. |
|
Not a blocker: full acceptance at head f75ce7c merged onto main 182daca found no blocking issue. It did find one narrow regression, and three texts that still describe the rule this PR replaces. Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. 1. A boot verify that fails late overwrites a Test that passed in the meantime.
Nothing serialises this against a manual Test that records through
On main a boot timeout is never written, so the Test's result survives. Here the new 2. Three texts still describe the old rule:
How I measured: the PR's own test fixtures ( |
|
No blockers; suggestions only, and they are marked inline. I independently reproduced @0xKT’s nonblocking finding through the production snapshot functions: a manual Test stored Verification remains green: the affected five-suite command passed with 724 tests. |
This branch has not been deployed
3 participants
Summary
A new non-authentication failure could leave an old needs_auth flag persisted, and a stale snapshot could still disable an edited agent's connection control. Carry current failure flags when retaining prior capabilities, recheck failed rows on subsequent boots, and withhold stale authentication verdicts from the listing RPC. The start-up verification's docstrings now say that a failed row is measured again at every start.
Timeouts and transport failures remain non-ready and offer a retry path. Model-menu and resume capability data survive failed measurements. No UI or wire schema change is needed.
The research-tool unit tests also replace external DNS beside their mocked HTTP transport. Real URL validation still runs, and resolver-failure cases keep their explicit overrides. This removes the uncontrolled dependency from the test that exceeded the CI idle ceiling; the ceiling is not relaxed.
Type
Verification
At this head, rebased onto main, Linux, Python 3.12:
uv run --frozen --python 3.12 --all-extras pytest -qwith the proxy variables unset: 3 failed, 29651 passed, 158 skipped. The same 3 fail on main at the merge base on this machine (an npm on PATH, a root-only permission check, a LibreOffice conversion); 0 introduced.--idle-ceiling-strict: 616 passed.ruff check,ruff format --check,lint-imports,ty checkanddeptryover the Makefile targets,commitlint, the three repository gate scripts andpre-commit run --from-ref origin/main --to-ref HEAD: passed.Linux, Python 3.12, using the synchronized uv container environment:
613 passed, including the strict idle ceiling. Native Windows, with PYTHONUTF8=1 and the existing environment selected by UV_PROJECT_ENVIRONMENT:
88 passed. In an isolated reverse check, an external-DNS guard failed the original largest-read test at socket.getaddrinfo; the same test passed with the resolver fixture. The previous CI shard completed 7198 assertions but failed its idle ceiling. Its log did not measure DNS separately, so the exact cause of that historical wait is not asserted.
The authentication regressions cover real temporary snapshot persistence, retries until ready, previous-ready and previous-failed capability retention, and stale RPC verdicts. Earlier native Windows probe/RPC runs had 19 failures identical to the base; the Linux selection above is green. The unchanged extAgents source/page suites were previously checked with 154 passing tests.
Whole-branch gates on native Windows:
All listed gates passed. The full diff was checked for caller coverage, ownership, admission and failure behavior, compatibility, regression strength and architecture. The full repository suite and native macOS were not run locally. No screenshot change applies; affected code documentation is updated.
Risk
Previously failed rows are verified once per boot until ready, so some rows now incur a new handshake at startup. A timeout does not become a successful authentication verdict. Stale capability fallback data remains available, while stale authentication no longer disables the edited row.
The resolver fixture is confined to the research unit-test module; production network validation is unchanged. Revert the squash commit to roll back. No wire schema changes.
Related Issues
#554