Repository navigation
fix: restore bundled ACP agent startup on Windows - #908
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 diff and surrounding ACP launcher/client and rendered-config code, the relevant history, AGENTS.md/CLAUDE.md and ACP context documentation, backward compatibility of the POSIX exec path, Windows process/config lifetime behavior, and whether the test edits weakened existing coverage. I found no concrete regression or project-rule violation.
Verification:
uv run --all-extras pytest tests/test_cli_acp_commands.py tests/test_config_product_render.py tests/test_agents_code_launcher.py tests/test_agents_design_launcher.py tests/test_agents_oncall_launcher.py tests/test_agents_ppt_launcher.py tests/test_agents_research_launcher.py tests/test_cli_agents_commands.py -x- 475 passed, 1 skipped (case-sensitive filesystem condition).uv run --all-extras pytest -m integration tests/integration/test_acp_stdio_smoke.py -x- 11 passed.git diff --check github/main...HEAD- passed.
The initial test invocation without --all-extras stopped during fixture setup because raven_everos was not installed; rerunning with the repository-declared extras resolved the environment and produced the results above.
|
No blockers in the reviewed scope. Scope: I reviewed the full PR diff (19 changed files), including tests and documentation, and traced affected callers and contracts at Checked all five bundled launchers and the scaffold, the Windows child-wait path, POSIX exec behavior, native stdin pipe reads, EOF/error handling, stdout isolation, PID-based render cleanup, fd ownership, fchmod availability, and seed-file newlines. Verification (native Windows, Python 3.12, isolated source snapshots; local JUnit-output options omitted below):
Merge readiness: GitHub and
|
|
Windows compatibility review: no new issue found in the inspected changes. Reviewed the complete 19-file diff at head Native Windows / Python 3.13.12, detached temporary checkout with uv run pytest tests/test_config_product_render.py tests/test_cli_acp_commands.py tests/test_agents_code_launcher.py tests/test_agents_design_launcher.py tests/test_agents_oncall_launcher.py tests/test_agents_ppt_launcher.py tests/test_agents_research_launcher.py -n 0 -x --tb=short --basetemp "$reviewRunDir/pr908/tests"
# 397 passed, 7 skipped (POSIX modes/pipe transport).
uv run pytest tests/integration/test_acp_stdio_smoke.py -m integration -k bundled_launcher -n 0 -x --tb=short --basetemp "$runDir/pr908/integration-tests"
# 5 passed, 6 deselected.
uv run pytest tests/test_cli_agents_commands.py -k default_landing -n 0 -x --tb=short --basetemp "$runDir/pr908/scaffold-tests"
# 1 passed, 71 deselected.The five real-launcher cases each answered two initialize requests with stdin kept open, retained the live rendered config during a sweep, and exited on EOF. Unit coverage also exercised native pipe reads and child-exit propagation. Limits: synthetic credentials, memory service disabled, no LLM tasks or rendering workloads. The finite-input session/new handshake and the broader unit failures described in the existing review were not rerun in this selection; these results do not resolve that separate coverage gap or establish full Windows compatibility. |
|
Merge conflict: @LivXue this PR no longer merges onto the current main.
The reviewer's latest verdict on this head is No blockers. Once the conflict is resolved (rebase onto main, or merge main in) and the new head has a verdict, the board queues it again by itself. |
|
Blocking: resolve the content conflict with current main in tests/test_cli_agents_commands.py before merging. Confirmed against |
bd68335 to
949778e
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.
The rebase resolves the prior merge-conflict blocker. I reviewed the patch-to-patch delta, the current github/main...HEAD diff, the resolved test in context, current callers and compatibility assumptions, and retained the earlier full-scope review of project rules, architecture, history, and test strength. The resolution preserves main's Windows-safe permission-test guard while adding this PR's scaffold assertions; git range-diff shows no other semantic change to the patch.
Verification on the rebased head:
git merge-tree --write-tree HEAD github/main- passed.git diff --check github/main...HEAD- passed.- Focused unit suites - 480 passed, 1 skipped for the documented case-sensitive-filesystem condition.
- ACP stdio integration suite - 11 passed.
|
Windows re-review: the rebased launcher checks pass, but the previously reported finite-input handshake failure remains. Scope: the complete 19-file diff at Code inspection: Native Windows / Python 3.13.12 verification, in an isolated detached checkout after
Exact test selections ( $R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\26804879b5e54446be3d865cc00f408b'
uv run pytest tests/test_config_product_render.py tests/test_utils_pid.py tests/test_cli_acp_commands.py tests/test_agents_code_launcher.py tests/test_agents_design_launcher.py tests/test_agents_oncall_launcher.py tests/test_agents_ppt_launcher.py tests/test_agents_research_launcher.py tests/test_cli_agents_commands.py::test_the_default_landing_is_the_raven_home_agents_tree tests/test_cli_agents_commands.py::test_an_unwritable_target_refuses_cleanly -n 0 -x --tb=short --basetemp "$R/unit-pytest"
# 409 passed, 8 skipped
uv run pytest tests/integration/test_acp_stdio_smoke.py -m integration -k bundled_launcher -n 0 -x --tb=short --basetemp "$R/launchers-pytest"
# 5 passed, 6 deselected
uv run pytest tests/integration/test_acp_stdio_smoke.py -m integration -k 'not bundled_launcher' -n 0 -x --tb=short --basetemp "$R/finite-stdin-pytest"
# 3 passed, 1 failed, 5 deselected; -x stopped before the last two selected tests
uv run pytest tests/integration/test_acp_stdio_smoke.py::test_a_batch_of_requests_is_all_answered tests/integration/test_acp_stdio_smoke.py::test_closing_stdin_exits_cleanly -m integration -n 0 -x --tb=short --basetemp "$R/remaining-pytest"
# 2 passedLimits: synthetic credentials and disabled memory service in launcher profiles; no LLM tasks or rendering workloads. Base comparison here was source inspection; a combined tree with #909 was not tested. This is not a full Windows compatibility pass. |
|
No blockers; suggestions only, and they are marked inline. The native Windows evidence changes my stance from clean to nonblocking: the finite-input handshake failure is real, but it is not introduced by this revision, and this change improves the merge-base behavior from six stdio failures to one. The author has assigned the remaining metadata-fetch delay to open PR #909, whose combined-tree run passes all 11 smoke tests, so I am carrying that as the named follow-up rather than blocking #908. Local confirmation on Linux: |
949778e to
5ed268e
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.
The target now contains the #909 metadata fix, resolving the previously named finite-input follow-up. I reviewed the complete diff against fix/model_catalog_cache_only, compared the production/test patch with the previously reviewed #908 revision, inspected the new Windows workflow in context, and rechecked callers, compatibility, project rules, architecture, and test strength. The prior patch is unchanged; the only new #908 delta is dedicated native Windows coverage for the full ACP stdio module.
Verification:
- GitHub
Windows ACP stdiojob - passed all 11 tests on Windows/Python 3.13. - Focused local unit suites - 480 passed, 1 documented filesystem skip.
- Local ACP stdio integration suite - 11 passed.
git merge-tree --write-tree HEAD github/fix/model_catalog_cache_only- passed.git diff --check github/fix/model_catalog_cache_only...HEAD- passed.
|
Windows compatibility re-review: the previously reported finite-input handshake now passes at this stacked revision. No additional issue was found in the inspected changes. Reviewed the complete 20-file diff at Native Windows / Python 3.13.12, isolated detached checkout after $R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\b89bdffc4a4845fb988b4088f488acef/pr908'
uv run pytest tests/test_config_product_render.py tests/test_utils_pid.py tests/test_cli_acp_commands.py tests/test_agents_code_launcher.py tests/test_agents_design_launcher.py tests/test_agents_oncall_launcher.py tests/test_agents_ppt_launcher.py tests/test_agents_research_launcher.py tests/test_cli_agents_commands.py::test_the_default_landing_is_the_raven_home_agents_tree tests/test_cli_agents_commands.py::test_an_unwritable_target_refuses_cleanly -n 0 -x --tb=short --basetemp "$R/unit-pytest"
# 409 passed, 8 platform-related skips
uv run pytest tests/integration/test_acp_stdio_smoke.py -m integration -n 0 -x --tb=short --basetemp "$R/integration-pytest"
# 11 passedThe complete handshake now receives IDs Limits: synthetic credentials and disabled memory service in launcher profiles; no LLM tasks, rendering workloads or full repository suite. The skipped checks cover POSIX permissions/pipe transport, not passing Windows equivalents. |
5ed268e to
c89312c
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: restore the stacked branch relationship so this PR cleanly contains only #908 over its declared #909 target.
GitHub currently reports this revision as non-mergeable (dirty) with 2,386 changed files because the head rebased #909 and #908 onto newer main history while fix/model_catalog_cache_only still points at 78ed94ee153e. git merge-tree --write-tree HEAD github/fix/model_catalog_cache_only confirms content conflicts in agents/raven-design/run.py, raven/config/product_render.py, tests/test_agents_design_launcher.py, and tests/test_config_product_render.py.
This late-round blocker meets all three required conditions: this revision introduced the history mismatch; every ordinary merge attempt reaches it; and the operator has no working merge path until the branch relationship is repaired. The logical three-patch series itself remains equivalent by git range-diff (apart from CI-commit author metadata), and uv run --all-extras pytest -m integration tests/integration/test_acp_stdio_smoke.py -x still passes all 11 tests.
78ed94e to
38ebfd8
Compare
c89312c to
2ea3464
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.
The prior stacked-branch blocker is resolved: the target now points to the matching rebased fix/model_catalog_cache_only history, the 20-file PR diff is restored, and git merge-tree --write-tree HEAD github/fix/model_catalog_cache_only succeeds.
I rechecked the repository rules, full target diff, affected callers and contracts, history/range-diff, backward compatibility, test changes for weakening, and the launcher/stdin/render-lifetime architecture constraints. The range-diff shows no semantic change to the previously reviewed patches; only commit metadata changed.
Verification on 2ea34646a2fa:
git diff --check github/fix/model_catalog_cache_only...HEADpassed.uv run --all-extras pytest -m integration tests/integration/test_acp_stdio_smoke.py -xpassed: 11 tests.- The exact-head GitHub Actions
Windows ACP stdiojob passed, including ACP handshakes and bundled launchers.
|
Windows compatibility re-review: no new issue found in the inspected scope. One inherited Windows test failure remains. Reviewed the complete 20-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_agents_code_launcher.py' 'tests/test_agents_design_launcher.py' 'tests/test_agents_oncall_launcher.py' 'tests/test_agents_ppt_launcher.py' 'tests/test_agents_research_launcher.py' 'tests/test_cli_acp_commands.py' 'tests/test_cli_agents_commands.py' 'tests/test_config_product_render.py' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr908\unit-pytest'
# 1 failed, 293 passed, 6 skipped in 30.45s; exit 1PR head: uv run pytest 'tests/integration/test_acp_stdio_smoke.py' -m integration -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr908\integration-pytest'
# 11 passed in 52.60s; exit 0Exact base control: uv run pytest 'tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr908\control-pytest'
# 1 failed in 8.58s; exit 1Additional head coverage: uv run pytest 'tests/test_config_product_render.py' 'tests/test_cli_agents_commands.py::test_the_default_landing_is_the_raven_home_agents_tree' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr908\remainder-pytest'
# 127 passed, 1 skipped in 10.78s; exit 0Reproduced on both head and exact base: the registration test assumes the stored command starts with an unquoted Limits: The unit run stopped at an inherited assertion; the later render/scaffold selection passed separately. No live LLM task, installed-agent login, rendering workload, ACL-hardening audit or full repository suite was run. Four non-failing idle-ceiling warnings occurred in integration; strict idle checking was not enabled. This scoped result does not establish full Windows compatibility. |
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 revision fixes the inherited Windows-only assertion by decoding the stored launcher command with the same production helper used for spawning, then checking the complete argv. It also runs that regression test in the existing Windows ACP job. The change strengthens coverage without changing runtime behavior or weakening a test.
I reviewed the revision delta, repository rules, the command decoder contract and history, affected registration caller, cross-platform compatibility, test intent, and the unchanged launcher/stdin/render-lifetime architecture. The target merge-tree and diff whitespace checks pass.
Verification:
uv run --all-extras pytest tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster tests/integration/test_acp_stdio_smoke.py -m "integration or not integration" -n 0 -x --tb=short: 12 passed.- Exact-head GitHub Actions
Windows ACP stdio: passed. - GitHub reports the head mergeable with the recorded target.
|
Windows compatibility re-review: the registration assertion is fixed; no new issue identified in the inspected changes. One intermittent EOF failure is recorded below. Reviewed the complete 20-file diff at Native Windows / Python 3.13.12, exact detached checkout, isolated user/Raven/temp directories, uv run pytest 'tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster' 'tests/integration/test_acp_stdio_smoke.py' -m 'integration or not integration' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\51c9a14534a54a89bcca3d83da323996\pr908\acp-pytest'
# First attempt: 4 passed, 1 failed; exit 1
uv run pytest 'tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster' 'tests/integration/test_acp_stdio_smoke.py' -m 'integration or not integration' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\51c9a14534a54a89bcca3d83da323996\pr908\acp-isolated-pytest'
# Separate rerun: 12 passed; exit 0The formerly failing registration case passed in both runs. In the first run, Limits: no full repository suite, live LLM workload, rendering task or ACL audit. This is a scoped Windows review, not a compatibility certification. |
38ebfd8 to
4ba33b8
Compare
69cb19f to
be55e81
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.
The rebased first three patches are semantically unchanged. The two new commits fix the Windows close path that previously called unavailable os.killpg: Windows now ends the launcher tree with bounded taskkill /T /F before the existing process cleanup, while POSIX retains the established process-group SIGKILL path. The added tests directly cover Windows fallback behavior, PID-reuse safety, and POSIX child cleanup; no test was weakened.
I checked the repository rules, actual target diff and range-diff history, close callers and process ownership contract, Windows/POSIX backward compatibility, the prior EOF evidence, test strength, and the launcher/stdin/render-lifetime architecture. The refreshed target merge-tree and diff whitespace checks pass.
Verification:
uv run --frozen --python 3.13 --no-sync pytest tests/test_subagent_acp.py -n 0 -x --tb=short: 312 passed.- Focused registration and ACP stdio suite: 12 passed.
- Exact-head GitHub Actions
Windows ACP stdio: passed. - The initial
--all-extrasattempt did not run tests because this host lacks the C compiler needed by the optionaldatriedependency in the rebased target; the frozen CI-shaped runs above completed normally.
|
Windows compatibility re-review: one new Windows test defect reproduced. Scope: the complete 22-file PR diff at head P2 - Make the new Windows teardown fixture tolerate an already-absent attribute. tests/test_subagent_acp.py:3193 calls Native Windows, Python 3.13.12, isolated detached checkout and user/Raven/temp directories. Dependencies were synced with uv run pytest tests/test_subagent_acp.py -k close -n 0 -x --tb=short --basetemp "$R/checks/908-close/pytest" --junitxml "$R/checks/908-close/junit.xml"
# 1 passed, 1 failed; 307 deselected. Failure: AttributeError at line 3193.
uv run pytest tests/test_subagent_acp.py::test_close_without_killpg_kills_nothing_by_the_pid_of_an_exited_launcher -n 0 -x --tb=short --basetemp "$R/checks/908-second-close/pytest" --junitxml "$R/checks/908-second-close/junit.xml"
# 1 failed with the same AttributeError.
uv run pytest tests/test_config_product_render.py tests/test_utils_pid.py tests/test_cli_acp_commands.py tests/test_agents_code_launcher.py tests/test_agents_design_launcher.py tests/test_agents_oncall_launcher.py tests/test_agents_ppt_launcher.py tests/test_agents_research_launcher.py tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster tests/test_cli_agents_commands.py::test_the_default_landing_is_the_raven_home_agents_tree -n 0 -x --tb=short --basetemp "$R/checks/908-unit/pytest" --junitxml "$R/checks/908-unit/junit.xml"
# 410 passed, 7 skipped (POSIX modes/pipe-transport checks).
uv run pytest tests/integration/test_acp_stdio_smoke.py -m integration -n 0 -x --tb=short --basetemp "$R/checks/908-stdio/pytest" --junitxml "$R/checks/908-stdio/junit.xml"
# First run: 3 passed, 1 failed; handshake replies were [1, 3], missing session/new.
uv run pytest tests/integration/test_acp_stdio_smoke.py -m integration -n 0 -x --tb=short --basetemp "$R/checks/908-stdio-isolated/pytest" --junitxml "$R/checks/908-stdio-isolated/junit.xml"
# Separate rerun: 11 passed, including all five real bundled launchers.
uv run pytest C:/Users/livxu/AppData/Local/Temp/raven-windows-review/7083af8abaec45c385fd83020f19e718/test_native_acp_cleanup.py -s -n 0 -x --tb=short --basetemp "$R/checks/908-native-close/pytest" --junitxml "$R/checks/908-native-close/junit.xml"
# 1 passed; scratch probe, real taskkill, launcher and its worker both exited.The scratch cleanup probe launched a Python launcher and worker from a path containing spaces and non-ASCII characters, held a handle to the worker, and verified both exited after the production The first stdio failure matches the previously recorded intermittent symptom; the separate green run does not establish that it is resolved. The stdin/server and smoke-test sources are unchanged from that reviewed revision; no new causal attribution is made. No LLM task, rendering workload or full repository suite was run. |
|
Blocking: make the new Windows teardown fixture tolerate an absent os.killpg and run it in Windows CI. The report holds and changes my stance from clean. The production cleanup probe is useful evidence that the runtime fix works. The blocker is the newly added Windows-path coverage itself: use |
|
Fixed the native teardown-test blocker in The helper tolerates an already-absent Native Windows build 10.0.26300, Python 3.13.12,
Complete-PR pre-commit, Ruff, import contracts, focused production type checks, commitlint and repository gates passed. Broader local dependency/type-check limitations and the earlier intermittent EOF symptom are disclosed in the updated description; this is not a full-suite or full-Windows-compatibility claim. The original stacked base is preserved. Git HTTPS was unavailable, so the exact locally tested commit and tree were verified byte-for-byte on GitHub and the existing head branch was advanced through the Git database API with |
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.
The prior Windows-test blocker is resolved. The fixture now tolerates an already-absent os.killpg, keeps launched clients behind finally cleanup, exercises real taskkill on native Windows while retaining a POSIX simulation, and the Windows ACP job now selects both teardown regressions.
I reviewed the repository rules, revision delta, fixture and production teardown contracts, affected CI caller, Windows/POSIX compatibility, test strength, history, and the unchanged launcher/stdin/render-lifetime architecture. No test was weakened; the target merge-tree and diff whitespace checks pass.
Verification:
uv run --frozen --python 3.13 --no-sync pytest tests/test_subagent_acp.py -n 0 -x --tb=short: 312 passed.- Focused registration and ACP stdio suite: 12 passed.
- Exact-head GitHub Actions
Windows ACP stdio: passed, including the newly selected teardown tests.
|
Windows compatibility re-review: the previously reported teardown-test defect is fixed. No additional issue found in the inspected scope. Reviewed the complete 22-file PR at Native Windows / Python 3.13.12, isolated source and user/Raven/temp directories, a fresh run-owned uv environment, $R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2fe2e239804c4819b5a0992536a10bc3'
uv run pytest tests/test_subagent_acp.py -k close_without_killpg -n 0 -x --tb=short --basetemp "$R\checks\908-teardown\pytest" --junitxml "$R\checks\908-teardown\junit.xml"
# 2 passed, 310 deselected, no skips.
uv run pytest tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster tests/integration/test_acp_stdio_smoke.py -m 'integration or not integration' -n 0 -x --tb=short --basetemp "$R\checks\908-stdio\pytest" --junitxml "$R\checks\908-stdio\junit.xml"
# 12 passed, including all five real bundled launchers and finite-input handshake.
uv run pytest tests/test_config_product_render.py tests/test_utils_pid.py tests/test_cli_acp_commands.py tests/test_agents_code_launcher.py tests/test_agents_design_launcher.py tests/test_agents_oncall_launcher.py tests/test_agents_ppt_launcher.py tests/test_agents_research_launcher.py tests/test_cli_agents_commands.py::test_the_default_landing_is_the_raven_home_agents_tree -n 0 -x --tb=short --basetemp "$R\checks\908-unit\pytest" --junitxml "$R\checks\908-unit\junit.xml"
# 409 passed, 7 platform-related skips (POSIX modes/pipe transport).The two teardown cases now run past fixture setup on native Windows. The live case completes an initialize handshake, checks the actual Limits: synthetic credentials, memory service disabled in launcher profiles, no live LLM/rendering workload, ACL audit or full repository suite. Three non-failing idle-ceiling warnings occurred in stdio tests. This passing run does not establish that the previous intermittent finite-input EOF symptom is eliminated, or establish full Windows compatibility. |
) ## Summary Creating an ACP session or opening the model picker could query LiteLLM's provider metadata API even with `allow_fetch=False`. Unknown Ollama models could delay `session/new` beyond the EOF cleanup grace period and drop its reply. Both context-window and output-ceiling queries now preserve the caller's lookup policy. A cache-only lookup skips LiteLLM's `get_model_info` only for the Ollama drivers (`ollama`, `ollama_chat`): on a table miss they ask the serving daemon (`POST /api/show`), which is the request that delayed `session/new`. Every other driver answers offline, by stripping the routing prefix and reading the table, and that lookup stays on cache-only paths: the table files most direct vendors under the bare model name, so skipping it changed 203 of 413 measured Model Refs, for example `openai/gpt-4o-mini` fell from its 128000-token window to the 200000 fallback, and `openai/gpt-4o` read OpenRouter's 4096-token output ceiling instead of the vendor's 16384. huggingface stays skipped under both policies, and fetch-enabled callers retain SDK lookup behavior. Merge this PR before #908. The Windows stdin and launcher changes in #908 are stacked on this metadata fix, and its Windows ACP stdio job validates the complete combination. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification At this head, rebased onto main, Linux, Python 3.12: - `uv run --frozen --python 3.12 --all-extras pytest -q` with the proxy variables unset: 3 failed, 29662 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. - `uv run --frozen --python 3.12 --all-extras pytest -n 0 tests/test_provider_rates.py tests/test_rpc_model.py tests/test_acp_server.py tests/test_acp_methods.py tests/test_acp_config_options.py tests/test_agent_loop_lazy_provider.py tests/test_lazy_provider.py tests/test_token_wise_pricing.py tests/test_rpc_session_init_bundle.py -q`: 609 passed. - Cache-only `resolve_context_window` and `declared_max_output_tokens` over 413 Model Refs (every provider-prefixed spelling of a bare-keyed chat row in the bundled LiteLLM table, plus common ones), each tree in its own empty home: this head and main answer identically for all 413, in under 0.2 s per tree, so no lookup left the process. - Removing the Ollama skip from the window lookup turns 8 tests red, including the ACP EOF handshake regression; removing it from the ceiling lookup turns 6 red. The new direct-vendor tests fail without the fix. - `ruff check`, `ruff format --check`, `lint-imports`, `ty check` and `deptry` over the Makefile targets, `commitlint`, `scripts/check_commit_messages.py`, `scripts/check_large_files.py`, `scripts/check_source_language.py` and `pre-commit run --from-ref origin/main --to-ref HEAD`: passed. - On the stacked #908 tree: `uv run --frozen --python 3.12 --all-extras pytest tests/integration/test_acp_stdio_smoke.py -m integration -q -n 0`: 11 passed on Linux. #908's Windows ACP stdio job runs the same file on windows-latest. The metadata regressions cover uncached Ollama spellings, cached aliases, offline normalization for direct vendors, both lookup policies, real model.options handling, and all three ACP handshake replies after EOF. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed Configuration and protocol contracts are unchanged; the resolver docstrings describe the lookup policy. ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes Cache-only lookups return what main returns for every Model Ref except an uncached Ollama model, whose window and ceiling stay unknown rather than being asked of the daemon; such a model keeps the configured default, and a self-hosted model can set agents.defaults.contextWindowTokens explicitly. Fetch-enabled lookup behavior is preserved. The Ollama driver set mirrors LiteLLM 1.85.0's `_get_model_info_helper`, whose only other branch that leaves the process is huggingface's; a LiteLLM upgrade that adds one would need it added there. Revert this change to roll back. ## Related Issues #908
Use platform-aware PID checks and permission handling for rendered configs. Preserve supplied line endings in seeded workspace files. Keep Windows launchers alive while their ACP child is running. Read stdin without overlapped pipe transports or pending synchronous pipe reads that can block native-module initialization. Preserve the POSIX exec path.
Run the real ACP stdio integration suite on Windows with Python 3.13 and locked development dependencies. Cover all handshake replies after EOF and repeated requests through each bundled launcher.
Windows has no os.killpg, so AcpClient.close raised AttributeError before any of its teardown ran, leaving the connection marked closed with its process, read loops and transport still in place. Now that the launchers wait for their child, killing the launcher alone would also leave raven acp running until its stdin closed. Where killpg is missing, run taskkill /T /F on the launcher's pid while it is still alive, as raven.ops.transport does, then Process.kill. Once the launcher has exited its pid may name another process, so nothing is killed by number. POSIX is unchanged.
Disabling the killpg branch of AcpClient.close left every ACP test green: nothing checked that a child the launcher started dies with the connection, which is why the client gives each server its own session. A launcher that backgrounds a worker now has to lose it on close.
Tolerate an absent killpg attribute and always close launched clients. Record real taskkill results on Windows after an ACP handshake, retain the POSIX simulation, and run both regressions in Windows CI.
Return the created session without optional config options when shutdown cancels a cold model catalogue lookup. Preserve cancellation on live connections and during required session setup. Cover both EOF timings, ordinary cancellation, and a fresh bytecode cache in the real stdio handshake.
a1dcfd0 to
db1e2bb
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.
The retarget to current main is clean: range-diff shows the six previously reviewed patches unchanged, and the branch merges without conflicts. The two new commits address the observed finite-input race by returning an already-created session without optional model metadata only when EOF shutdown has latched; required subscription/setup cancellation and ordinary live-connection cancellation still propagate. The launcher docstrings now describe the Windows wait behavior accurately.
I reviewed the repository rules, full main-target diff, callers and shutdown sequencing, patch history, Windows/POSIX backward compatibility, test strength, and the launcher/stdin/render-lifetime architecture. No test was weakened; merge-tree and diff whitespace checks pass.
Verification:
- ACP methods, server, and client modules: 498 passed.
- Focused registration and real stdio suite: 13 passed.
- Exact-head GitHub Actions
Windows ACP stdio: passed.
|
Windows compatibility re-review: the EOF/session-reply fix passes the focused native checks. No additional Windows compatibility issue found in the inspected changes. Scope: all 26 PR files at Both EOF timings now return the created session even when shutdown cancels optional model discovery; ordinary cancellation and cancellation during required subscription setup still propagate. Real warm/cold-import handshakes, all five bundled launchers, command registration and native taskkill teardown passed at this head. Native Windows / Python 3.13.12, fresh per-head environment from $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/d1d0b660deeb48e3a2e4d079a3316cbc'
uv run pytest tests/test_acp_methods.py tests/test_acp_server.py tests/test_cli_acp_commands.py -n 0 -x --tb=short --basetemp "$R/checks/908-eof/pytest" --junitxml "$R/checks/908-eof/junit.xml"
# 218 passed, 1 skipped (POSIX pipe transport).
uv run pytest tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster tests/integration/test_acp_stdio_smoke.py -m 'integration or not integration' -n 0 -x --tb=short --basetemp "$R/checks/908-stdio/pytest" --junitxml "$R/checks/908-stdio/junit.xml"
# 13 passed; 6 advisory idle-ceiling warnings.
uv run pytest tests/test_subagent_acp.py -k close_without_killpg -n 0 -x --tb=short --basetemp "$R/checks/908-teardown/pytest" --junitxml "$R/checks/908-teardown/junit.xml"
# 2 passed, 310 deselected.Limits: synthetic credentials and disabled memory service in launcher profiles; no live LLM/rendering workload, Windows ACL audit or full repository suite. The author's broader Windows failures remain outside this focused selection. The passing deterministic EOF cases and warm/cold handshakes provide new evidence for the previous intermittent EOF symptom, without claiming exhaustive timing coverage. |
…b key (#927) ## Summary Two gaps in how Raven stops its cli sub-agents and what environment it starts them with. **Stopping a cli sub-agent on Windows.** A cli sub-agent's timeout, cancel and stream-failure paths, and the `kimi` commands Raven runs to explain a failed Kimi Code connect, ended the process group with `os.killpg`, which Windows does not have. The `AttributeError` replaced the timeout error or the cancellation, and the CLI was left running. Where `killpg` is missing, the tree is now ended from the launcher's pid with `taskkill /T /F` while the launcher is still alive, and the launcher is then killed itself, the way `raven/ops/transport.py` already ends a command on Windows. Once the launcher has exited its pid may name another process, so nothing is killed by number. POSIX keeps the group kill. **A web key rotated through `settings.set`.** `settings.set` rewrites `~/.raven/env` when a web credential changes, but a POSIX cli or acp sub-agent reads that file only through the login-shell capture, which a running Raven takes once and keeps. Every later spawn started with the key the write had replaced, until a Connect, a Test, "Check again" or a restart took the capture again. A mirrored key's write now takes the capture again before the reply. Captures also run one at a time: a refresh that read the shell before the write, a Connect's for instance, could otherwise land after the write's own and put the replaced key back, and readers that arrive while the first capture runs now wait for it rather than each running the shell. Windows is unchanged for keys. Nothing sources `~/.raven/env` there, and a Windows sub-agent's environment is Raven's own with the registry's PATH, so a key held only in config does not reach it; #912 states this in the code. On POSIX the keys reach a sub-agent only through a `source` line in the user's shell startup files, which the onboard wizard adds only with the user's agreement, and a Windows delivery path would need a consent step of its own, so this PR does not add one. Not changed here: an acp sub-agent that is already connected keeps the environment it was launched with until its next launch; a key written by `raven onboard`, which runs as its own process, reaches a running Raven's sub-agents at the next Connect, Test, "Check again" or restart; and on Windows the wizard still prints the POSIX `source` line when `$SHELL` is not bash or zsh. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Linux, Python 3.12, rebased onto main 182daca: - `uv run --frozen --python 3.12 --all-extras pytest -q` with the proxy variables unset: 3 failed, 29667 passed, 158 skipped. The same 3 fail on main on this machine (an npm on PATH, a root-only permission check, a LibreOffice conversion); 0 introduced. - `uv run --frozen --python 3.12 --all-extras pytest tests/test_subagent_third_party.py tests/test_subagent_kimi_code.py tests/test_subagent_acp.py tests/test_subagent_host_env.py tests/test_rpc_console.py tests/test_rpc_settings.py tests/test_rpc_subagents.py -q`: 1080 passed. - Windows stop: `tests/_windows_kill.py` leaves a Windows host as it is, where the real `taskkill` runs, and elsewhere removes `os.killpg` and stands in for `taskkill` by ending the process group it is given, the launcher's tree here; either way it records each call. The launchers, their workers and the Kimi stand-in are Python started on `sys._base_executable`, because a Windows venv's `python.exe` is a redirector that runs the base interpreter as its child and returns a pid the script never sees. The tests have not run on a Windows host, and no CI job runs these test files there. - Windows stop, measured here: the six tests that use the helper pass. With `os.killpg` removed for the whole session and a `taskkill` on PATH that ends the group it is given, they pass again through that `taskkill` (3 calls), and the POSIX-only Kimi test beside them is skipped. With `sys.executable` replaced by a wrapper that runs Python as its child, as a venv's `python.exe` does on Windows, both modes stay green; started on that wrapper instead, the two cancel tests fail on the pid they compare. - Windows stop, against the fix: the six fail with `AttributeError` on main. Removing the alive check in the cli backend, the `taskkill` call or the kill after it in either module, or the cli backend's handling of a `taskkill` that times out, turns at least one of them red. Removing the POSIX group kill from the Kimi Code commands turns only the new test whose stand-in `kimi` starts a process of its own red (1 failed, 347 passed over the two test files). - Rotated key: `test_a_rotated_key_reaches_the_capture_the_next_spawn_reads` saves a key twice through `settings.set` and reads the capture after each save, through a real bash that sources the mirror; on main the second read still returns the first key. Removing the refresh turns 12 tests red; refreshing before the mirror is rewritten turns that test red. Two existing tests that write a web key now stub the refresh, so they do not run the host's login shell. - Concurrency: three tests force the interleavings with events: a refresh that read the shell first and lands after a later one, a second reader that arrives while the first capture runs, and readers waiting on a first capture that fails. Each failed in 3 of 3 runs on main and passes with the lock; removing the lock from either capture path, or either re-check inside the first one, turns one of them red. - With `--cov=raven --cov=raven_everos --cov-branch` added to the full run, `python scripts/coverage_gate.py diff --base-ref origin/main --threshold 90`: 93.22% of the changed lines (55 of 59). The four left are except clauses for a process group or a process that is already gone. - `ruff check`, `ruff format --check`, `lint-imports`, `ty check` and `deptry` over the Makefile targets, `commitlint`, the three repository gate scripts and `pre-commit run --from-ref origin/main --to-ref HEAD`: passed. - `git merge-file` of `raven/rpc/methods/console.py` with each open PR that touches it (#883, #900, #911, #912, #923, #925), and of `raven/agent/subagent/backends/env.py` with #912 in both orders: no conflicts. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No user-facing docs describe either behavior. ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes Saving a web key now waits for the user's login shell to start before the reply, as Connect and Test already do; the capture's 15 s timeout bounds it, and it can also wait for a capture already in flight. A capture that fails keeps the previous one and does not fail the save. No credential reaches a sub-agent that did not before; it is the same shell reading the same file, read again. On Windows a stop now waits up to 5 s for `taskkill`, and a worker that a launcher started and left behind after exiting outlives the stop there, since Windows has no group to address and an exited launcher's pid is not trusted. If `taskkill` is missing, fails or does not answer within those 5 s, only the launcher is killed, and a cli stop then waits for any worker still holding the launcher's output pipes to exit. Revert this PR to roll back. ## Related Issues #908, #912
… cache (#942) ## Summary A long-running Raven host held on to three kinds of memory it no longer used. Observed on one dev box on 2026-10-12: - Two ACP agents, started on 2026-09-20 by a host that exited without closing them, were still running 22 days later. They had been reparented to init and held 523 MiB PSS between them. - A gateway's glibc thread arenas had grown from 85 to 214 MiB over 18 days of light use. - `SessionManager` kept every session it had ever loaded. This PR cleans up all three automatically. **1. ACP agents that outlive their host (Linux).** An ACP agent starts in a session of its own, so it gets none of the signals sent to its host's process group or terminal, and while the host lives only `AcpClient.close` ends it. A host that exited without closing its agents, or was killed, crashed or OOM-killed before its teardown, left them running. - The agent now starts through a one-line exec wrapper (`raven/acp_client/leftovers.py`). It runs the host's own interpreter with `-I -S`, arms `PR_SET_PDEATHSIG(SIGKILL)`, checks that its parent is still the host, restores SIGPIPE and SIGXFSZ to their defaults and execs the agent in place, with the same pid and group and the agent's own argv. The agent's process then ends with the host. - The signal reaches only the direct child, and an `npx` launcher's real server is a grandchild. So every host also records the process groups it launched, one file per host under `acp-launches/` in the config file's directory. The first ACP launch in any process ends the groups recorded by hosts that have since died. That sweep runs once per process on a daemon thread. Every launch waits for it for at most 5 s, and once one launch has waited that long the rest stop waiting for it. - A group is ended only when all of the following hold: - the record is of this version and comes from this boot and PID namespace; - its host is gone, judged by pid and start time; - the group is not group 0, 1 or the sweeper's own; - its leader, if still running, has the start time on record; - a current member still carries the dead host's mark, `RAVEN_ACP_HOST=<host pid>-<host start ticks>`, which every launched agent gets last in its environment. A read that fails, of a record or of `/proc`, keeps the record and ends nothing; a record that does not parse is deleted. **2. Native heap trimming (Linux with glibc).** glibc keeps the memory native code frees, such as PyMuPDF rasters and tokenizer tables, in its arenas. `raven/utils/heap.py` checks every ten minutes and, once anonymous memory stands 32 MiB above its lowest reading since the last trim, runs `malloc_trim(0)` in a worker thread. The five long-running hosts start it: `raven gateway`, `raven serve`, the TUI's RPC host, `raven acp` and `raven a2a serve`. **3. A bounded session cache.** `SessionManager._cache` becomes an LRU with a soft limit of 64 sessions. - A session leaves the cache only when nothing about it is unsaved. Its message count must match what is on disk, and a fingerprint of its metadata, consolidation mark and pending clarification must match the one taken when it was last read or written. A metadata patch counts its own keys as saved even when the caller set them on its copy first. - A session that a caller still holds comes back as the same object. - A reloaded session keeps the `updated_at` it was saved with. `CONTEXT.md` gains two terms, **Leftover agent** and **Launch mark**. Measured on this box on 2026-10-13: - **PyMuPDF.** 112 pages rasterised three times in a 3-thread pool. In two runs, anonymous memory was 279.8 and 315.3 MiB after `store_shrink(100)`, and 93.5 and 84.6 MiB after `heap.trim()`. - **Gateway.** An isolated gateway with a key-less config, run for 100 s. With the interval shortened to 45 s, one trim fired at t+47.7 s and took anonymous memory from 293 to 273 MiB; with the default interval, none fired. Final PSS was 321.6 against 328.6 MiB. Its boot check launched 16 ACP agents through the wrapper: the five discovered agents under `agents/`, which that config cannot start, and eleven presets, eight of which completed the handshake (Claude Code, Codex, OpenCode, Hermes Agent, OpenClaw, CodeBuddy, Qoder and Pi). Afterwards no marked process and no launch record remained. - **Killed host.** A stand-in host was SIGKILLed after launching through `AcpClient`. On main, a direct agent, an `npx`-shaped launcher and its grandchild all survive with ppid 1. On this branch, the direct agent and the launcher end with the host, and the next host's first launch ends the grandchild. - **Discovered agents, with and without the wrapper.** Each of the five under `agents/` was verified twice in a home holding only dummy keys (a handshake calls no model): Raven-Design completed the handshake both ways, and the other four stopped on the same missing key both ways. The Windows regression fixtures use explicit executable filenames and native project-path strings. The unreadable-transcript symlink fixture skips only when Windows reports missing symlink privilege (WinError 1314); other creation errors still fail. These test corrections do not extend the Linux-only cleanup mechanisms. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification The original author's Linux measurements below precede the Windows test-fixture follow-up. The native Windows results that follow were run against the final follow-up tree. - **Author's Linux full suite before the Windows test-fixture follow-up:** `uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider` (repo addopts: `-n 4`, integration and e2e deselected). Result: 3 failed, 30261 passed, 158 skipped, 2 xfailed. The failures are the ones this box gives on main too: `test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for`, `test_knowledge_parser_excel.py::test_a_legacy_workbook_is_converted_and_read` (no LibreOffice) and `test_subagent_node_runtime.py::test_what_cannot_be_read_names_nothing` (run as root). - **Directly affected files:** `tests/test_acp_leftovers.py`, `tests/test_utils_heap.py`, `tests/test_session_manager.py`, `tests/test_cli_{a2a,acp,gateway,serve,tui}_commands.py` and `tests/test_subagent_host_env.py`, under `--idle-ceiling-strict` as CI runs them. Result: 620 passed. - **Neighbours:** `tests/test_rpc_session.py`, `tests/test_session_{export,title,project_grouping}.py`, `tests/test_defer_manager.py`, `tests/test_subagent_acp.py` and `tests/test_acp_{ask_user,elicitation,remote}.py`. Result: 683 passed. - **Lint and gates:** `make lint-python lint-imports lint-deps lint-types check-large-files check-source-language check-commits`. Result: all seven exit 0; import contracts 10 kept, 0 broken. - **Mutation:** guards were deleted or inverted one at a time, out of tree, and each such mutant failed the test written for it. They covered the sweep's wait, bound, daemon flag and start failure; the version, boot-time, PID namespace, liveness, leader and mark checks; the `/proc/<pid>/stat` parse; the trimmer's reader and threshold; and the cache's eviction order, fingerprint, `updated_at` restore and patch rule. - **Diff coverage:** 96.27% (387 of 402 executable changed lines) from the directly affected and neighbouring files alone, against CI's 90% gate. What stays uncovered is the arms for a `/proc` read or a `killpg` that fails, and the import inside `raven a2a serve`, which only a running server reaches. **Native Windows test-fixture follow-up (2026-10-14):** Windows 10.0.26300, Python 3.13.12, `PYTHONUTF8=1`, a dedicated worktree and `uv sync --frozen --python 3.13 --all-packages --extra dev`. The imported Raven package was verified to come from that worktree. Pytest's isolated homes and a separate caller `RAVEN_HOME` were used. JUnit output paths are omitted from the command below. ```powershell uv run --frozen --no-sync pytest tests/test_acp_leftovers.py tests/test_utils_heap.py tests/test_session_manager.py tests/test_session_project_grouping.py tests/test_subagent_host_env.py tests/test_cli_a2a_commands.py::test_serve_starts_the_heap_trimmer tests/test_cli_acp_commands.py::TestServe::test_the_heap_trimmer_starts_before_the_channel_is_served tests/test_cli_gateway_commands.py::test_run_starts_the_heap_trimmer_at_boot tests/test_cli_serve_commands.py::test_serve_starts_the_heap_trimmer_at_boot tests/test_cli_tui_commands.py::test_the_terminal_host_starts_the_heap_trimmer -k 'not test_acp_child_inherits_custom_raven_home and not test_acp_child_is_told_it_serves_as_a_subagent' -n 0 --idle-ceiling-strict --tb=short ``` Result: **229 passed, 73 skipped, 2 deselected**, exit 0 with the idle ceiling enforced. Of the skips, 72 are existing Linux/glibc cases and one is the measured lack of Windows symlink privilege. The three corrected discovery/path cases failed before the corrections and pass in this selection. Before the symlink guard, the same broader selection had 229 passes, 72 skips, two deselections and one WinError 1314 failure. The two deselected real ACP child tests still depend on the native close fix in #908. The broader CLI permission/signal limitations reported in the Windows review are covered separately by #939. This follow-up claims no full Windows suite pass, real ACP lifecycle validation, privileged symlink run, Windows ACL audit, or new Linux run. - `uv run --frozen --no-sync ruff check raven evolver agents plugins-dist tests scripts docs-site` and the matching `ruff format --check`: passed (2,272 files formatted); the final symlink edit also passed both file-scoped checks. - `uv run --frozen --no-sync lint-imports`: 10 contracts kept. `uv run --frozen --no-sync ty check raven/acp_client/client.py raven/acp_client/leftovers.py raven/cli/a2a_commands.py raven/cli/acp_commands.py raven/cli/gateway_commands.py raven/cli/serve_commands.py raven/cli/tui_commands.py raven/session/manager.py raven/utils/heap.py`: passed. - `uv run --frozen --no-sync pre-commit run --from-ref origin/main --to-ref HEAD`: all applicable hooks passed. - `node node_modules/@commitlint/cli/cli.js --from origin/main --to HEAD --config commitlint.config.cjs`, `uv run --frozen --no-sync python scripts/check_commit_messages.py origin/main..HEAD`, `uv run --frozen --no-sync python scripts/check_large_files.py origin/main...HEAD`, `uv run --frozen --no-sync python scripts/check_source_language.py origin/main...HEAD`, and `git diff --check origin/main...HEAD`: passed. The relevant-tests checkbox remains unchecked because the broader Windows files still contain the disclosed limitations above. - [ ] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No user-facing docs change, because no command, flag or setting does. The two new terms are glossary entries in `CONTEXT.md`. ## Risk User-visible behaviour changes: - **An ACP agent that exists but cannot be executed now fails after the spawn.** Examples are a bad interpreter line or a binary for the wrong architecture. The wrapper writes `raven: cannot start <path>: <error>` to the agent's stderr and exits 127, so the failure surfaces as a connection that ends with that line in its stderr, where `AcpClient.launch` used to raise `cannot start` at once. A missing program still fails at launch, as before. - **Agent environment.** Every ACP agent's environment carries `RAVEN_ACP_HOST`. It is an identifier, not a secret; its parts are world-readable in `/proc/<pid>/stat`. - **Locale.** An agent started under a C or POSIX locale gets `LC_CTYPE=C.UTF-8`, because the wrapper interpreter's PEP 538 coercion carries through the exec. - **Launch records.** Every ACP launch and close writes a small JSON file under `acp-launches/` beside the config file, synchronously and with an fsync. - **First launches.** While dead hosts' records are swept, a process's ACP launches wait for the sweep, 5 s at most in all: once one launch has waited that long, the rest start without it. Normally the sweep takes milliseconds. - **Session timestamps.** A reloaded session's `updated_at` is the one it was saved with, where it used to be the time of the load, so a save that adds no message no longer moves it. Sentinel's defer timing and the export header read it in memory, and the session list orders by the saved value. Left as today: - Anything but Linux. #928 tracks macOS and Windows. - A group whose surviving processes carry no readable mark: one started with a fresh environment, another user's, or an undumpable one. - An agent binary that is set-id or carries file capabilities: its exec clears the parent-death signal, so only the sweep can end it, and only where its environment is readable. - Spawns other than ACP agents: the CLI sub-agent backend, background exec and the sandbox executor. - Leftovers created during one long-lived host's life. They wait for another process's first ACP launch. - A host started with another `--config` sweeps that directory's records only. Edge cases of the records directory: - **Another PID namespace.** Its records are kept until a reboot, about 300 bytes per container start. - **A directory shared between machines.** Another machine's record older than this machine's boot is removed by this machine's sweep. That host's agents would then not be swept if it died before its next launch or close rewrote the file. Nothing is ever signalled across machines. Edge cases of the session cache: - A change made through a reference to a session that had already left the cache, and then released unsaved, is lost, as it would be in a fresh process. - An in-memory undo whose save failed, followed by new messages back to the same count, reads as clean. `save()` already mishandles that state, so the residue predates this PR. A group is signalled only when it passes all five checks above. A crafted record cannot point a sweep at a live host's agents, because the mark names the identity the liveness check reads. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes Rollback is a revert. Nothing persisted changes shape: session files are written exactly as before, and `acp-launches/` only holds what the next sweep reads, so it can be deleted at any time. ## Related Issues #928 (macOS and Windows; stays open)
3 participants
Summary
Bundled ACP agents could fail to start on Windows when rendering configuration, reading stdin, or replacing the launcher process. Closing an ACP connection could also fail before releasing its process and transport because Windows has no os.killpg.
Type
Verification
Native Windows build 10.0.26300, Python 3.13.12, PYTHONUTF8=1. The final tree is based on the fetched main revision; all eight patches replayed unchanged. The last upstream update adds bug-report upload/receiver code and has no file overlap with this PR. The 19-case selection below was rerun on the final tree; the broader selection and controls predate that last unrelated update. Tests used isolated user/Raven homes, frozen dependencies, and verified imports from the tested checkout. Run-owned --basetemp and --junitxml output paths are omitted from the selections below.
uv run --frozen --no-sync pytest tests/test_acp_server.py::TestFrameLoop::test_session_creation_answers_when_eof_cancels_the_catalogue tests/test_acp_methods.py::TestConfigOptions::test_session_creation_preserves_cancellation_outside_shutdown_metadata tests/integration/test_acp_stdio_smoke.py tests/test_cli_agents_commands.py::test_register_pins_the_resolved_row_into_the_host_roster tests/test_subagent_acp.py::test_close_without_killpg_ends_the_tree_through_taskkill tests/test_subagent_acp.py::test_close_without_killpg_kills_nothing_by_the_pid_of_an_exited_launcher -m 'integration or not integration' -n 0 --tb=short: 19 passed, no skips after the final rebase. This includes both EOF timings, cancellation controls, both bytecode-cache states, all five real launchers, and real taskkill. One non-failing idle-ceiling warning was reported.uv run --frozen --no-sync pytest tests/test_acp_server.py tests/test_acp_methods.py tests/test_acp_updates.py tests/test_acp_config_options.py tests/test_cli_acp_commands.py tests/test_config_product_render.py tests/test_utils_pid.py tests/test_agents_code_launcher.py tests/test_agents_design_launcher.py tests/test_agents_oncall_launcher.py tests/test_agents_ppt_launcher.py tests/test_agents_research_launcher.py tests/test_cli_agents_commands.py tests/test_subagent_acp.py tests/test_rpc_bootstrap.py tests/test_subagent_host_env.py tests/test_subagent_probe.py -n 0 --tb=short: 1284 passed, 58 failed, 10 skipped, 4 errors. The same selection on the rebased PR immediately before the EOF fix returned 1280 passed, the identical 58 failing and 4 error test IDs, and 10 skipped. No new failure/error IDs from the EOF fix.uv run --frozen --no-sync pytest tests/integration/test_acp_stdio_smoke.py::test_a_whole_handshake_runs_in_one_process[cold_imports] -m integration -n 0 --tb=shortfailed with response IDs [1, 3]. A separate stack-capture probe measured 6.76 seconds in the first LiteLLM import; the handler awaited model.options while its worker imported LiteLLM and its dependencies.uv run --frozen --no-sync pytest tests/test_acp_server.py::TestFrameLoop::test_session_creation_answers_when_eof_cancels_the_catalogue tests/test_acp_methods.py::TestConfigOptions::test_session_creation_preserves_cancellation_outside_shutdown_metadata -n 0 --tb=short: before the fix, 2 failed and 2 passed. Both EOF cases failed with the missing session reply. They pass with the fix and are included in the broader post-rebase selection above. Warm/cold real handshakes plus these four cases also passed as a six-case selection.uv run --frozen --no-sync ruff check raven evolver agents plugins-dist tests scripts docs-siteand the correspondingruff format --check: passed; 2266 files already formatted.uv run --frozen --no-sync lint-imports: 10 contracts kept.uv run --frozen --no-sync ty check raven/acp/methods.py raven/acp/updates.py raven/acp_client/client.py raven/cli/acp_commands.py raven/config/product_render.py: passed.uv run --frozen --no-sync pre-commit run --from-ref origin/main --to-ref HEAD: every applicable hook passed. The two final docstring edits also passed their file-scoped pre-commit run.node node_modules/@commitlint/cli/cli.js --from origin/main --to HEAD --config commitlint.config.cjs,uv run --frozen --no-sync python scripts/check_commit_messages.py origin/main..HEAD,uv run --frozen --no-sync python scripts/check_large_files.py origin/main...HEAD,uv run --frozen --no-sync python scripts/check_source_language.py origin/main...HEAD, andgit diff --check origin/main...HEAD: passed.The broader Windows selection remains red, so the relevant-tests checkbox is unchecked. This revision has no new Linux or full repository run, live LLM workload, rendering workload, or ACL audit. POSIX-only skips are not counted as Windows passes.
Risk
At EOF, a completed session/new can omit optional configOptions when shutdown cancels a slow lookup; the sessionId and session modes are retained. Active connections keep their existing model-menu behavior. The handler grace remains five seconds.
Windows keeps the launcher present while its server runs, and connection close waits up to five seconds for taskkill. Idle Windows pipe input polls every 10 ms. Redirected files and text streams retain their existing reader path. POSIX retains exec and 0600 permissions; Windows rendered files inherit directory ACLs.
There is no configuration-format change. Previously generated agent folders retain their existing launchers; the scaffold change applies to new folders. Revert this PR to roll back the launcher, stdin, process-cleanup and EOF-response changes.
Related Issues
#909