Repository navigation
fix(*): refresh an acp agent's model menu from a dispatch reply - #1038
Conversation
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 three commits, then traced the capability snapshot store, atomic-update semantics, ACP session/new / session/load response shapes, backend callers, roster/list consumers, and the existing probe/backfill concurrency rules. I also checked backward compatibility, the repository rules and context map, and confirmed the tests were extended rather than weakened. The partial locked revision preserves unrelated concurrent menu updates and stands down for a genuinely newer measurement.
Verification:
PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_subagent_acp.py tests/test_subagent_probe.py tests/test_rpc_subagents.py -x: 581 passed.- A plain
uv run pytest ...attempt stopped during fixture setup because the default dev environment does not install theraven_everosworkspace package. - An
uv run --all-extras pytest ...attempt could not builddatriebecause this runner has nogcc; no tests ran in that attempt.
|
Two follow-ups from an independent review of the previous head, both pushed as new commits on top (no history rewritten). N1, a backend that stood down stayed refused. After Fix: on a stand-down Evidence:
N2, the mode-tier design spec named the old function. It now names Not covered: native Windows (CI only), and only two or three processes on one machine for the shared-file case. |
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 58b7360d86ff, the full branch diff, the two new commits, and the N1/N2 follow-up note. I traced the revised store return contract through relearn_session_menus, the backend adoption hook, snapshot staleness, the probe/backfill writers, and the menu consumers. I also checked the relevant history, backward compatibility, AGENTS.md and context-map constraints, architecture boundaries, and that the tests were strengthened rather than weakened.
The stand-down now returns the differently stamped record without writing the refused reply over it, and the backend adopts that record so its next menu response can revise against the stored stamp. A changed launch fingerprint remains correctly marked stale for the existing backend. The spec rename also retains the intentionally revision-pinned line references.
Verification: PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_subagent_acp.py tests/test_subagent_probe.py tests/test_rpc_subagents.py -x - 584 passed.
|
Windows compatibility review (head Scope: Inspected all 7 changed files ( Findings
Verification and limitations
Exact verification commands# suite-v2: 23 passed, 0 failed, 0 errors, 0 skipped; exit 0
# Working directory: C:\Users\livxu\AppData\Local\Temp\raven-windows-review\bec6f99f3e954eb2bfaa1c73a4997ceb\pr1038\source\head-v2
uv run pytest tests/test_subagent_acp.py tests/test_rpc_subagents.py tests/test_subagent_probe.py -k 'relearn or model_menu or has_model_menu or verify_missing_snapshots or model_choices or stood_down or newer_record' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\bec6f99f3e954eb2bfaa1c73a4997ceb\checks\1038-suite-v2\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\bec6f99f3e954eb2bfaa1c73a4997ceb\checks\1038-suite-v2\junit.xml'
# model-v2: 26 passed, 0 failed, 0 errors, 0 skipped; exit 0
# Working directory: C:\Users\livxu\AppData\Local\Temp\raven-windows-review\bec6f99f3e954eb2bfaa1c73a4997ceb\pr1038\source\head-v2
uv run pytest tests/test_rpc_subagents_model.py -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\bec6f99f3e954eb2bfaa1c73a4997ceb\checks\1038-model-v2\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\bec6f99f3e954eb2bfaa1c73a4997ceb\checks\1038-model-v2\junit.xml' |
The model menu on an acp agent's card is read from the session/new handshake and cached in the capability snapshot, which never expires by age; only a Test, a Connect or the boot backfill re-measured it. Every dispatch already re-learned the mode menu from the session reply it holds, but not the model menu beside it in the same reply, so a card could keep offering model ids the agent had long stopped serving. The relearn path (now relearn_session_menus) reads both menus off that reply with the probe's own parsers, grouped choices included, and opens no session and starts no process to do it. Each menu is judged on its own, and a reply that carries no model entry, an empty one, or one the adapter flagged as unread keeps the recorded menu rather than clearing it. A menu recorded as unmeasured becomes measured once a reply carries one. The models.availableModels key and the model_source rule are unchanged. The write now goes through SnapshotStore.revise, which stands down when the stored row carries a newer measurement stamp than the record the backend was built from. A Test or boot verify that lands while a dispatch is in flight on the previous backend is no longer overwritten by that backend's older record; this protects the mode relearn too.
Backends of one agent each hold the record they were built from, and a relearn on one does not reach the others' copies. Writing a backend's whole copy back meant a second backend, whose reply reported only modes, restored the model menu the first had just refreshed. SnapshotStore.revise now applies just the fields the reply reported, onto the row as read inside the file lock, and still stands down when that row carries another measurement stamp. The stale flag of the caller's record is kept on what it returns, since re-reading menus is not a handshake. A redundant model_menu_measured clause is dropped: an unmeasured menu is always empty, so any reported menu already differs.
Raven's own ACP server attaches configOptions to session/new only; session/load and the resume route carry the modes alone. The relearn docstring claimed both menus arrive on every route.
…ement When a relearn stood down because the stored row carried a newer measurement stamp, the backend kept the older record and never re-read the store, so every later reply was refused the same way until the backend was rebuilt. Hot-apply after a Test and the boot backfill rebuild it in-process, but a failed hot-apply or another process sharing the snapshot file left it refusing for good. SnapshotStore.revise now returns the record it stood down for, together with whether it wrote, and still writes nothing in that case: the reply may predate that measurement, so its menus are not taken over it. The relearn hands that record back, the backend adopts it through its existing hook, and the next reply is judged against the stamp actually stored. The handed-back record is flagged stale when it was measured under another launch config than the caller's.
The measured table keeps its line numbers, which are those of the revision it is pinned to, and notes the name that revision used.
Use the platform command quoting helper for Python and stub paths. Keep the RPC dispatch fixture at a path with spaces so unquoted script arguments fail on every test host, while the shared ACP fixture supports spaced checkouts.
e6fb8ba to
a224ddc
Compare
|
Fixed the spaced-Windows-path test issue in I reproduced both real-process dispatch tests on Windows 11 build 26300 / Python 3.13.12 from a checkout with spaces: the unquoted interpreter path was truncated before launch. Both command builders now use After the required rebase: the menu/snapshot/dispatch selection passed 23 tests (561 deselected), and The previous tip CI follow-up: all four unit shards and all three Windows jobs passed. The diff-coverage gate remains at 87.50% (35/40 changed executable lines), below the 90% threshold, the same percentage reported before this Windows test correction. It remains a separate cross-platform coverage limitation; no coverage policy was changed. |
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 new test-only commit and the PR diff after its rebase onto the updated github/main. I checked the shared command_quote / command_argv contract on POSIX and Windows, the affected ACP launch callers, the strengthened spaced-path regression, compatibility with the rebased base, repository naming and source rules, architecture boundaries, and test integrity. The quoting change is scoped to fixtures and directly covers the Windows-review follow-up without changing production behavior.
Verification: PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_subagent_acp.py tests/test_subagent_probe.py tests/test_rpc_subagents.py -x - 584 passed.
|
Windows compatibility review (head Scope: Rechecked the complete 7-file diff after the rebase onto Findings
Verification and limitations
Exact verification commands# suite: 23 passed, 0 failed, 0 errors, 0 skipped; exit 0
# Working directory: C:\Users\livxu\AppData\Local\Temp\raven-windows-review\f852247ffea44090ac4a3eb8f196eca7\pr1038\source\head
uv run pytest tests/test_subagent_acp.py tests/test_rpc_subagents.py tests/test_subagent_probe.py -k 'relearn or model_menu or has_model_menu or verify_missing_snapshots or model_choices or stood_down or newer_record' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\f852247ffea44090ac4a3eb8f196eca7\checks\1038-suite\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\f852247ffea44090ac4a3eb8f196eca7\checks\1038-suite\junit.xml'
# model: 111 passed, 0 failed, 0 errors, 0 skipped; exit 0
# Working directory: C:\Users\livxu\AppData\Local\Temp\raven-windows-review\f852247ffea44090ac4a3eb8f196eca7\pr1038\source\head
uv run pytest tests/test_rpc_subagents_model.py tests/test_utils_commands.py -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\f852247ffea44090ac4a3eb8f196eca7\checks\1038-model\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\f852247ffea44090ac4a3eb8f196eca7\checks\1038-model\junit.xml' |
A newer stored row this build cannot read back is neither written over nor handed to the backend, and a snapshot file that cannot be written logs once and reports nothing learned. Both branches were reachable and right but unexercised, which left the PR's changed lines under the diff coverage threshold.
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 new coverage-only commit against the full current PR diff. I checked both added cases against SnapshotStore.revise, relearn_session_menus, and the backend adoption hook: an unreadable differently stamped row remains byte-for-byte untouched and is not adopted, while an unwritable cache logs and leaves backend state unchanged. I also checked compatibility, repository test/source rules, architecture boundaries, history, and that the assertions strengthen rather than weaken coverage. No production code changed in this revision.
Verification: PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_subagent_acp.py tests/test_subagent_probe.py tests/test_rpc_subagents.py -x - 586 passed.
|
Windows compatibility review (head Scope: Rechecked the complete 7-file diff against the previously reviewed head Findings
Verification and limitations
Exact verification commands# suite: 25 passed, 0 failed, 0 errors, 0 skipped; exit 0
# Working directory: C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5d49a031e2574684a1e292f8aec11174\pr1038\source\head
uv run pytest tests/test_subagent_acp.py tests/test_rpc_subagents.py tests/test_subagent_probe.py -k 'relearn or model_menu or has_model_menu or verify_missing_snapshots or model_choices or stood_down or newer_record or newer_row or cannot_be_written' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5d49a031e2574684a1e292f8aec11174\checks\1038-suite\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5d49a031e2574684a1e292f8aec11174\checks\1038-suite\junit.xml'
# model: 111 passed, 0 failed, 0 errors, 0 skipped; exit 0
# Working directory: C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5d49a031e2574684a1e292f8aec11174\pr1038\source\head
uv run pytest tests/test_rpc_subagents_model.py tests/test_utils_commands.py -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5d49a031e2574684a1e292f8aec11174\checks\1038-model\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5d49a031e2574684a1e292f8aec11174\checks\1038-model\junit.xml' |
|
Not a blocker (review of 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. A menu relearn rebuilds every acp backend, and the rebuild loses the record that restores a cleared model pick.
|
|
No blockers; suggestions only, and they are marked inline. The new independent review changes my standing verdict from clean to nonblocking. I confirmed the mechanism: I am not duplicating the finding as another review thread; it is already documented in the reviewer comment above and remains a named follow-up. This revision can still merge. Verification: |
Summary
An acp agent's card offers a model picker built from the
configOptionsentry withcategory: "model"that the agent returned onsession/newwhen it was last measured. That measurement is cached in the capability snapshot, which has no age-based expiry and is invalidated only when the launch fingerprint (command, cwd, env) changes. It was re-taken only by a Test, a Connect, or the boot backfill, and the backfill skips a row that is alreadyreadyand every row on a registered machine. Every dispatch already re-learned the mode menu from the session reply it holds, but not the model menu that arrives in the same reply. So a card could go on offering model ids the agent had stopped serving weeks earlier.What changes:
relearn_session_modes->relearn_session_menus, backend hook_relearn_modes->_relearn_menus) now reads the model menu off the same reply, with the probe's own_read_session_model_choices, so grouped choices are parsed exactly as the handshake parses them. It runs at the same two points as before: aftersession/newand after a successfulsession/loadon the dispatch path.configOptions, nomodelentry, an empty or value-lessmodelentry, or the adapter's catalog-unread flag keeps the recorded model menu instead of clearing it. Raven's own ACP server sendsconfigOptionsonsession/newonly, so a resumed session reports modes alone and must not erase the models.subagents.listreturns it on the next read, and the agent table is rebuilt through the same listener the mode relearn already used. A row recorded before the menu existed becomes measured once a reply carries one.models.availableModelshandling is unchanged, and so is_model_rule: third-party rows stayagent, Raven's own rows stayraven(host provider catalogue),fixedrows still have no menu.SnapshotStore.revise, which applies only the fields the reply reported onto the stored row read inside the file lock, and stands down when that row carries another measurement stamp. This closes two overwrite paths that also applied to the mode relearn before this change: a backend built before a Test or boot verify could write its older record over the newer one (status, detail and fingerprint included), and a second backend of the same agent whose reply carried only modes could restore the model menu the first had just refreshed. The stamp check is the same newest-measurement-wins rule the boot backfill already applies to its own late verdicts.revisewrites nothing and hands back the newer stored record, which the backend adopts through its existing hook (and rebuilds the agent table, as for any moved record). The reply that was refused may predate that measurement, so its menus are never written over it; the backend's next reply is judged against the stamp actually stored and refreshes normally. Without this, a backend no rebuild reached (a failed hot-apply, or another process sharing the snapshot file) refused every later reply until it was replaced. The handed-back record is flagged stale when it was measured under another launch config than the backend's.A row whose stored
modelis no longer in the refreshed menu was checked and needs no change: the card shows the raw id with its clear control, the picker opens on the new menu, andsubagents.updatevalidatesmodelonly when one is sent, so a new pick or a clear both go through. Only re-sending the dropped id is refused, which is correct because the agent no longer takes it. A dispatch with that id still runs; the agent's refusal is logged once per value as before.Limits that remain:
session/load) refreshes nothing for models.Deliberately not done: an empty
modelentry is read as "not reported" rather than "the agent now offers none". Clearing on an empty but present value would erase the picker on every resumed session whose reply carries the current value with no options.Windows test portability: both the RPC dispatch fixture and the shared ACP stub helper now quote the interpreter and script tokens with
command_quote. The existing RPC test copies its stub to a temporary path containing spaces, so this launch requirement is tested on every host. No production behavior changes in this correction.Type
Verification
The historical author runs below used Python 3.12.13 and
--all-extrasserially, with imports checked against that checkout. The native Windows follow-up below records what was rerun for the latest test correction and rebase:uv run --frozen --python 3.12 --all-extras pytest -n 0 tests/test_acp_unprompted.py tests/test_rpc_instances.py tests/test_rpc_subagents.py tests/test_rpc_subagents_model.py tests/test_subagent_acp.py tests/test_subagent_acp_mcp_lifecycle.py tests/test_subagent_acp_remote.py tests/test_subagent_host_env.py tests/test_subagent_manager.py tests/test_subagent_mirasim.py tests/test_subagent_probe.py tests/test_subagent_third_party.py tests/test_acp_methods.py-> 1525 passed, 0 failed.subagents.listafter a real dispatch all failed on their assertions; the keep-on-silence and catalog-unread tests passed before and after, as guards. After review, three more were red before the adoption change: the next reply after a stand-down, the stale flag on an adopted record, and the same sequence through the backend's own hook against the real store. The stand-down test was tightened from "returns None" to "returns exactly the newer record and leaves the snapshot file byte-identical".initializeand onesession/new, with verify, record and ping patched to fail if called.ruff checkandruff format --checkover the lint targets: clean.ty check: all checks passed.lint-imports: 10 contracts kept, 0 broken.scripts/check_commit_messages.py origin/main..HEAD,scripts/check_large_files.py origin/main..HEAD,scripts/check_source_language.py origin/main...HEAD,commitlint --from origin/main: all exit 0.Those historical runs did not include the full Python suite,
tests/integration/, or native Windows. No ui-web code or wire field changed, so no ui-web tests were added.Native Windows follow-up (Windows 11 build 26300, Python 3.13.12, UTF-8 mode):
uv sync --frozen --python 3.13 --all-packages --extra dev, ran these exact commands ($evidenceDiris an external report directory):Results: 23 passed / 561 deselected, then 111 passed; both exit 0, with no failures, errors or skips, and the strict idle gate enabled. Imports resolved to this worktree; profile and OAuth directories were isolated.
Diff coverage follow-up (Linux, Python 3.12.13,
--all-extras, serial, imports checked against this checkout):The five uncovered changed lines were two failure branches of the menu revision, both added by this PR: a newer stored row this build cannot read back (its stamp still forbids writing over it, and there is no record to hand back), and a snapshot file that cannot be written (logged once, nothing learned). Two tests now pin them by observable results: the snapshot file stays byte-identical, the relearn returns nothing, the backend keeps the record it holds and rebuilds no table, and the write failure appears in the log. No production line changed.
Mutation, each alone and restored: letting the unreadable newer row fall through, or handing back the caller's own record for it, turns its test red; letting the write failure raise, or reporting it as written, turns the other red; dropping the relearn's own check for "nothing revised" turns both red. Removing the stamp check now turns 5 tests red, not adopting the newer record 4, writing the whole record back 1.
Coverage measured with CI's settings (
--cov=raven --cov-branch) over the relevant subset, thenpython scripts/coverage_gate.py --coverage-json <that run's json> diff --base-ref origin/main --threshold 90against the same base CI used: 100.00% (40/40 executable changed lines). Control, the same run with only the two new tests deselected: 87.50% (35/40), the same five lines CI reported, so the local measurement reproduces CI's.pytest -n 0over tests/test_acp_unprompted.py tests/test_rpc_instances.py tests/test_rpc_subagents.py tests/test_rpc_subagents_model.py tests/test_subagent_acp.py tests/test_subagent_acp_mcp_lifecycle.py tests/test_subagent_acp_remote.py tests/test_subagent_host_env.py tests/test_subagent_manager.py tests/test_subagent_mirasim.py tests/test_subagent_probe.py tests/test_subagent_third_party.py tests/test_acp_methods.py: 1528 passed, 0 failed, on the rebased head (the earlier 1525 was counted before the rebase, which brought in tests of its own).ruff check and format, ty, import contracts (10 kept), commit-message, commitlint, large-file and source-language checks passed. Only the relevant subset ran under coverage locally; CI's full sharded run decides the gate.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
No user-facing docs describe when the model picker refreshes, so none were changed.
Risk
User-visible: an acp agent's model picker now follows the agent after a dispatch instead of only after a Test. The mode relearn also gains the stamp check and per-field write, so in the two overwrite cases above it now stands down or writes less than before. A backend that stands down adopts the newer stored record wholesale and triggers one table rebuild, which brings the table to the same record a rebuild would read anyway.
relearn_session_menusreplacesrelearn_session_modesin the module's exports; nothing else in the tree imported the old name; the dated mode-tier design spec now names the function by its current name and keeps the line numbers of the revision it is pinned to. One file write happens only when a reply's menu differs from the cached one, as before.Security: no new input surface; the reply is parsed by the existing tolerant readers. Compatibility: the snapshot file format is unchanged. Rollback: revert the squash commit; snapshots written by this change load unchanged on the previous code.
Related Issues
N/A