Conversation
The rail row is now Agent Hub and the page is titled Agents, the names the prototype uses; the zh column follows. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The agents page now reads as the prototype's agent hub: three sections (connected, available, not installed), one control per row, and a 520x500 sheet in the shared detail host carrying the row's one-liner, its description, an install block or a key field, and an action bar. - catalogue.ts: what the page knows about an agent beyond the wire -- the one-line summary, who makes it, and the install command and site for one this machine has not got (13 presets plus Raven's own five). - source.ts: sectionOf replaces groupOf and costOf; the wizard's stageOf and its three predicates are unchanged. - store.ts: a write in flight is held on the row, a refusal stays on the row with the write it refused, and Retry sends it again; connectRow, describe, saveKey and recheck are the page's verbs over the shared write path. - styles.css: every rule under the extAgents- prefix, the prototype's geometry on page.css tokens; page.css sizes the detail panel off data-owner and drops the .xaedit, .sutest and .sukey rules only the old page used. - i18n: 46 new gui.agent keys in both columns, 18 old ones removed, and the zh copy of connect changed to the prototype's word. - check-class-namespace: extAgents pinned at 0 / 0; the shared pins the removed rules were holding move to the domains that still use them. - Tests: the page test rewritten (23 cases), new store and catalogue tests, two snapshots re-recorded. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
A third-party acp row can now carry a `model`: the id the session is put on over `session/set_config_option`, validated against the choices the agent's own handshake advertised. The built-in row can carry one too, validated against raven's own providers -- one raven knows, holding a usable credential -- and stored naming the provider it was picked under. `subagents.list` reports the row's model, the menu (`model_choices`), what `subagents.update` accepts for it by kind (`model_source`), and whether the row is raven's own (`own`). - config/schema: `ThirdPartyAcpSubagentConfig.model`. - rpc: `SubagentRow.own / model / model_choices / model_source`; `subagents.update` takes `model`, `provider`, `clear_model` (clear wins), accepts `description: null` as "back to the factory text" (a built-in row goes back to following its seed), materializes a built-in override on its first edit and otherwise edits the row the table merges under that name, whatever spelling or transport config stored it in; a discovered product row can be tested (`source: "vendored"`). - manager: `session_model_for` is the one resolver every lane reads an acp row's model through -- a spawn, a direct chat, and a DAG node via the new `model_for` hook threaded through the runner and its three construction sites. A built-in row's own model is a pin the backend pairs with its own credential through the provider pool on every run (`live_pin_resolver`), so a cached backend never runs one conversation's key against another's model; the manager is handed the pool by the loop and the playbook CLI. - capabilities: `SnapshotStore.has_model_menu`, so a snapshot recorded before the menu was measured is re-verified at startup. - ui-web/ui-tui: generated clients; the offline fixture carries the new fields and an acp row with a menu. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The sheet now says which model an agent answers with and lets the reader change it, by the row's own rule (`model_source`): the built-in row picks from raven's connected providers and sends the provider with the pick; an acp row picks from the choices its handshake advertised and sends the value verbatim; an openai or cli row, or an acp row that advertised none, wears the pill disabled. Unset reads by ownership -- one of Raven's own follows the main Raven, a third party runs on its own default -- and a set model shows the name the agent gave it, or the id with the provider it is stored under, with a clear control on hover. The picker is the shared component, hung off the pill and sized to the sheet. - types/source/store: the four wire fields on the row, a `model` op that maps onto `subagents.update` (`clear_model` alone, or `model` + `provider`), and `setModel` / `clearModel`, whose refusal is toasted rather than held on the row -- the pick was wrong, not the row. - A shipped product can be tested from the sheet now that the server takes `source: "vendored"`. - The field rules stop at the field's own controls, so the picker's search box keeps the component's look inside the sheet. - Seven new `gui.agent.model_*` messages in both columns; page and store tests for every pill state. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
`schemas/subagent.schema.json` is exported from the config models, and the `model` field the acp row gained was not exported with it; the byte-equality guard in tests/test_agent_schemas.py caught the drift. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The source-language gate refuses non-English additions outside the named exemption zones, and a comment that quoted the Chinese button words was one. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the agent picker and test sheet still have user-visible paths that cannot behave as presented.
I found four functional blockers and one repository-rule blocker, marked inline. I covered the full merge-base diff, the extAgents callers and prior implementation, model dispatch through spawn/direct-chat/DAG/playbook, generated wire compatibility, commit history, AGENTS.md, CONTEXT-MAP.md, ui-web/CONTEXT.md and CONTRIBUTING.md, and the rewritten tests for removed coverage.
Verification: uv run pytest over the PR's 16 backend suites: 1471 passed; npm test in ui-web: 188 files / 2501 tests passed; focused extAgents suite: 5 files / 86 tests passed; type-check and generated-client check passed; ESLint completed with 0 errors and 5 warnings in untouched domains; class-namespace and large-file checks passed.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the five previously reported findings still need to be addressed.
The sole change since my previous review translates the stylesheet comment into English. That fixes the source-language gate failure, but it does not modify the implementation or test files behind any of the five open threads. Direct inspection confirms that all five findings still stand; I found no additional defect in this comment-only delta.
Verification:
uv run python scripts/check_source_language.py github/refactor/ui_web_architecture...HEAD— passed.npm test -- --run src/features/extAgents— 5 files and 86 tests passed.npm run type-check— passed.
Coverage: I reviewed the new delta and commit history, rechecked the existing findings against the current implementation and callers, and carried forward the prior full-diff review of project rules, backward compatibility, architecture boundaries, and test integrity. No test was weakened in this revision. The five threads remain open because none is settled.
LivXue
left a comment
There was a problem hiding this comment.
Request changes
Thanks -- the page reads well, and the lane work behind the pill is the right shape: one session_model_for for a spawn, a chat and a DAG node, and the acp-only keyword guarded by signature rather than by a kind check. Two findings block, both on the built-in row's model path; the two after them should be fixed or filed. Everything below was read at the head commit; raven/providers/pool.py is unchanged by this PR, so its behaviour is taken as given.
Blocking
1. A built-in row's stored model and the model the dispatch runs can disagree, silently. subagents.update stores a provider-qualified id, and both halves this PR adds read it back by resolving the provider with find_by_model alone -- which fails for an id whose prefix names a section raven carries no spec for. The write side then refuses the picker's own pick with a message blaming the model or the credential, both of which are fine; the bare-id route stores the id, and every dispatch afterwards runs the conversation's pair (or the gateway's key) while the row keeps reporting the stored id. Nothing on the row or the wire says the pin was dropped, and the one log line fires once per resolver, so a later, different unusable pin logs nothing at all. Fix direction: one resolver on both sides (see the inline comments on _host_pair and _row_pin), a refusal that names its actual cause, and ideally a row field for "the pin resolved".
2. An acp model push the agent refuses is retried every turn, and stays on the row as if it landed. _set_model runs on every route into a session, so a permanent refusal is re-attempted as if transient, one warning per attempt with no bound. The row (subagents.list) and the instances report keep showing the pick, and the refusal is a transient toast -- not the held red text with Retry the description promises. Fix direction: the same held-failure treatment the other verbs get, plus a row field (model_push_error, or model_in_effect); failing that, log once per problem rather than once per turn.
Important
3. A config that cannot be read is reported as a credentials problem (raven/rpc/methods/subagents.py:511). The bare except Exception returns None, which the caller renders as "it names no provider raven knows, or that provider has no usable credentials". A PermissionError, a malformed config, a loader failure: none of it is logged, and the user is told their model or key is at fault. An unreadable config is a server-side error, not a field-validation one.
4. The menu backfill can leave a "ready" row with an unmeasured menu, and a failed Test overwrites a good snapshot. _verify_missing_snapshots (raven/agent/subagent/probe.py:579) records only on ready; when the re-verify does not return ready, the old snapshot -- the one with no modelChoices -- survives and _probe_acp reports its stored ready, so the sheet draws a disabled pill and "managed by itself" for an agent that does offer a menu, with an INFO line as the only trace. Same area: _test_acp records unconditionally (probe.py:509), so a manual Test that fails on a flaky machine replaces a good snapshot and costs the row its menu and its statefulness until a later success. has_model_menu already tells "never measured" from "measured, none"; a wire field carrying that distinction would be the durable fix.
Nits
ui-web/src/features/extAgents/manifest.ts(untouched here) still says the domain's detail-card button is.xaeditand that the class prefix "waits on the CSS step". This PR is that step: fivepage.cssrules gone, prefix renamed,LEGACY_LOCAL.extAgentsdown to 0.ui-web/CONTRIBUTING.md: theLEGACY_BORROWEDrow still reads "one of the eleven,warn, is SetupSheet.tsx..." and "11 in the shared components", but this PR's ledger edit movedwarnoff that list (components 11 -> 10,LEGACY_CHROME_EXPR10 -> 11). That row's totals look stale independently (the two lists hold 23 names; the row says 34) -- worth re-deriving in the same edit.ui-web/src/styles/page.css:4143: the specificity comment still cites.sukey input, whose rules this PR deleted (6 occurrences at base, 1 at head, and it is the comment).raven/rpc/models.py:150(own): added to the wire, read once (source.ts:57), consumed by nothing -- ownership is decided client-side byisOwnRow(catalogue.ts:108). Either render from the field or drop it; the server is the one that knows a shipped product was registered as a plain config row. The description's "Unset reads by ownership (own, a new wire field)" describes a read the code does not make.ui-web/src/features/extAgents/catalogue.ts:13andstyles.css:17: both point at something a reader of the tree cannot open -- "sources are listed in the PR that added them", and amy_docs/temp/.../proto_app.csspath. The prototype reference is useful provenance; the PR pointer is not a source.ui-web/src/features/extAgents/store.ts:304:stopTesthas no caller.
Process
- The base is
refactor/ui_web_architecture(#475, still open). Please say so in the description, or land #475 first -- otherwise the diff a reviewer sees is not the diff that merges. - Second description/code mismatch, alongside
own: "a refusal stays on the row as red text with Retry" does not hold for the model op (finding 2).
What is good here
- The key removal is clean: 17 i18n keys gone (34 leaves with the en/zh pairs), with no dangling reference left in either front end.
- The class-namespace ledger moves are consistent and self-enforcing -- the pin assertion (
check-class-namespace.mjs:859) fails the next PR that leaves a pin behind. - The lane unification is the right shape, and the acp-only keyword travels only to a
runthat declares it or takes**kwargs, so a backend written against the older paper is not handed it. - The contract artifacts (
openrpc.json, bothgenerated.ts, the JSON schema) move in the same change as the models they describe. - The verification section shows the acp path exercised end to end against a live gateway. The built-in path is where it stops short: the pick recorded there is
openrouter/..., whose prefixfind_by_modelresolves, so the spec-less section looks like the case neither the tests nor the live run covered.
The model option's currentValue came from model.options asked without a session id, which answers with the process default. After a switch, the response to session/set_config_option therefore carried the value it had just replaced, so a client rendering currentValue as the selected item snapped its picker back to the old model. Both call sites now ask model.options about the session whose answer it is. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
_probe_acp resolved argv[0] alone, and a shim-launched preset's command is an npx one that resolves on any machine with node. Pi therefore read as connectable wherever pi was not installed, and the connect failed a minute later inside the adapter with "executable not found". SHIM_REQUIRED_EXECUTABLES declares, per shim preset, the local agent the shim drives and its install command. The probe asks after that executable too and reports the row missing with the install beside it, the way a local-executable row already did. Codex is left out on purpose: codex-acp ships the agent as its own binary and wants a login, not an install. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the five previously reported findings remain unaddressed.
Neither new commit modifies the implementation or test files behind my five open threads, and direct inspection confirms that each still stands. I found no additional blocker in the ACP session-model or shim-probe delta under the fourth-round bar.
Named nonblocking follow-up: SHIM_REQUIRED_EXECUTABLES now requires a PATH-level claude for the Claude Code preset, but the pinned @agentclientprotocol/claude-agent-acp@0.66.0 resolves its native Claude CLI from the platform-specific optional dependency of @anthropic-ai/claude-agent-sdk. A Node-only installation can therefore run the adapter while this probe reports it missing. This change introduces the false negative and it is reachable in ordinary use, but the displayed global-install command is a working escape hatch, so it does not meet the fourth-round blocker threshold.
Verification:
uv run pytest tests/test_acp_config_options.py tests/test_acp_methods.py tests/test_subagent_probe.py tests/test_subagent_third_party.py -x: 475 passed on Python 3.12.3.npm test -- --run src/features/extAgents: 5 files and 86 tests passed.npm run type-check: passed.- Source-language, large-file, and
git diff --checkgates: passed. The environment has nomake, so I ran the large-file target command from the Makefile directly.
Coverage: reviewed the full revision diff and the two-commit delta, surrounding callers and history, AGENTS.md / CLAUDE.md / CONTEXT*.md rules, backward compatibility, Runtime/Web UI architecture boundaries, and whether tests were weakened. The new tests add coverage rather than weakening existing assertions. Existing threads remain open because none is settled.
subagents.update stored a built-in row's pick as <provider>/<id>, and the two readers of that id disagreed with the write: _host_pair compared the whole prefixed id against a section's bare model list, so a pick under a section raven has no spec for was refused with a message blaming the model and the key, while _row_pin handed the pool no provider for the same id, so a stored pick that did land ran on a derived credential or not at all. stored_provider_name in providers.wire is the inverse of stored_model_id, and both sides read the provider through it: a spec'd provider by its route names, a spec-less section by the prefix the write put there. The write stores every accepted id prefixed, a spec-less section serves only the ids it lists, and a config that cannot be read raises as the server's failure instead of being reported as the reader's. The unusable-pin warning is keyed on the pin rather than the resolver, so a later, different wrong pin gets its own line. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The push is re-asserted on every route into a session and on every session a spawn opens, so an agent that refuses a value for good produced one warning per turn for as long as the row kept the pick. The first refusal of a value is a warning; the rest are debug lines, and a value the agent later takes is forgotten so a fresh refusal is a fresh warning. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
isOwnRow now reads the wire's own flag before the catalogue, so a renamed acp row whose handshake named Raven sorts and words itself as Raven's. A stored host id is matched to its provider in one spelling: the stored head is the public form (openai-codex), the host list keys the slug (openai_codex), and the pill printed the raw prefix while the picker lost its tick. The picker on the sheet offers listed ids only (a new allowTyped prop on the shared ModelPicker): an acp row's write takes exact menu values and a built-in row's adds to no provider, so the typed row could never land. A running test keeps a Stop control in place of the disabled Test button, which was the only caller subagents.test_cancel had left. The model op goes through act like every other verb, so a refused pick is held on the row with Retry rather than toasted. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The manifest still said the domain's detail button was .xaedit and that its prefix waited on the CSS step this branch is; the CONTRIBUTING ledger rows quoted LEGACY_CHROME, LEGACY_CHROME_EXPR and LEGACY_BORROWED totals that no longer match the tool (125 / 15 / 23 today, and warn is counted on the expression list); a page.css comment cited .sukey input, deleted here; and two comments pointed at a pull request and a local scratch path a reader of the tree cannot open. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
…itself SHIM_REQUIRED_EXECUTABLES required a claude on PATH for the Claude Code preset, but claude-agent-acp@0.66.0 pins @anthropic-ai/claude-agent-sdk, which carries the CLI as a per-platform optional dependency: a Node-only machine runs the adapter while the probe called it missing, and the CLI that answers is the bundled one either way. Pi is the only shim that reaches for a local install, so it is the only entry left; a contract test keeps claude_code and codex out of the table. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: add the required file-level purpose documentation to ui-web/src/features/extAgents/catalogue.test.ts.
Four of my five original findings are fixed; I replied to and resolved those threads. The remaining new test module still begins directly with imports, so it does not satisfy AGENTS.md section 1.1. At this fifth-round bar: the PR introduces the file, the violation is unconditional rather than dependent on a rare runtime combination, and the repository explicitly treats this as a hard rejection rule with no compliant workaround other than adding the file-level documentation.
Named nonblocking follow-ups:
- The previously reported Claude adapter probe false-negative remains unchanged.
- The new
stored_provider_nameresolver can reinterpret a pre-existing built-in override such asdeepseek-ai/DeepSeek-V3, which previously ran through the configured gateway, as an explicitdeepseek_aiprovider and drop the pin. Reselecting the model under the gateway storesopenrouter/deepseek-ai/DeepSeek-V3, so operators have a working recovery and this does not meet the late-round blocker threshold.
Verification:
uv run pytest tests/test_provider_pool.py tests/test_provider_wire_model.py tests/test_rpc_subagents_model.py tests/test_subagent_acp.py tests/test_subagent_manager.py -x: 764 passed.npm test: 188 files and 2505 tests passed.npm run type-checkandnpm run gen:check: passed.npm run lint: passed with 0 errors and 5 pre-existing warnings on untouched files.- Source-language, large-file, class-namespace, and
git diff --checkgates: passed.
Coverage: reviewed the full target diff and this four-commit delta, surrounding callers and history, AGENTS.md / CLAUDE.md / CONTEXT*.md rules, backward compatibility, Runtime/Web UI architecture boundaries, and test integrity. The added tests strengthen coverage; no test was weakened.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: add the required file-level purpose documentation to ui-web/src/features/extAgents/catalogue.test.ts.
The new delta correctly stops treating a globally installed claude executable as a prerequisite for claude-agent-acp, and its regression test matches the adapter packaging contract. The remaining blocker is unchanged: this PR-added test module still begins directly with imports, contrary to the mandatory file-level purpose documentation in AGENTS.md section 1.1. At this sixth round it meets all three blocking criteria: this PR introduced the file, every normal checkout contains the violation, and there is no compliant workaround without changing the file.
The previously named legacy configured-gateway model-id compatibility case remains a nonblocking follow-up.
Verification: uv run pytest tests/test_subagent_third_party.py tests/test_subagent_probe.py -x (293 passed); source-language checker passed; large-file checker passed via its underlying uv command because make is unavailable; git diff --check github/refactor/ui_web_architecture...HEAD passed. I reviewed the revision delta and target diff context, affected callers and history, project rules and domain context, backward compatibility, architecture boundaries, and whether tests were weakened.
…test comment Four follow-ups from the review round, in one commit. A stored id whose prefix names neither a spec nor a provider section -- deepseek-ai/DeepSeek-V3 written by hand before ids carried their provider -- was read as an explicit provider and its pin dropped. stored_provider_name takes the config's provider table, and such a prefix is part of the id: the pool derives the provider as it does for a bare id, which is the gateway branch the id always ran through. A manual acp Test that failed before session/new recorded a snapshot with no menu and no statefulness over a good one, silently costing the row both until the next success. The verdict is recorded; the capabilities are the previous record's, as SnapshotStore.load already trusts a stale row's. A snapshot recorded before the model menu existed reported its stored "ready" while the sheet drew a disabled pill for an agent that may offer a menu. CapabilitySnapshot knows whether its menu was measured, written as the key's absence so a re-recorded verdict cannot mint a measured-empty menu, and the probe reports such a row as attention with "run a test". The catalogue test module opens with its file-level purpose comment. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The roster reads a stale snapshot's capabilities on purpose, so the record a failed manual test writes over one has to keep them as well: the previous record is now read with allow_stale, and the merged record takes this test's fingerprint so the row does not stay stale for good. A comment in the rpc tests still said the acp test recorded for config rows only. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
|
Review round, status by finding. LivXue:
gloryfromca:
Verification for the round: uv run pytest over the touched backend files: 1345 passed before the follow-ups, then 667 / 607 / 365 passed on the files each follow-up touches (test_subagent_acp.py, test_subagent_probe.py, test_rpc_subagents.py, test_subagent_manager.py, test_provider_wire_model.py, test_rpc_subagents_model.py, test_provider_pool.py); npm test in ui-web, 188 files / 2505 tests; type-check, lint (0 errors), gen:check, check-class-namespace and the TUI catalogue check all pass. |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The file-level purpose documentation now satisfies the remaining repository-rule blocker. This revision also closes both follow-ups I had named: the bundled Claude adapter is no longer rejected for lacking a global executable, and legacy gateway-served model ids remain on the pool derivation path while configured spec-less provider sections resolve explicitly. The snapshot updates preserve prior capabilities across a failed test without collapsing an unmeasured model menu into a measured-empty one. I found no new plain error in the delta, and every thread I opened is now resolved.
Verification: uv run pytest tests/test_provider_wire_model.py tests/test_provider_pool.py tests/test_rpc_subagents.py tests/test_rpc_subagents_model.py tests/test_subagent_acp.py tests/test_subagent_manager.py tests/test_subagent_probe.py -x (926 passed); npm test -- --run src/features/extAgents/catalogue.test.ts (6 passed); source-language, large-file, and git diff --check gates passed. This pass covered the revision delta and full target-diff context from the prior rounds, affected callers and history, project rules and domain context, backward compatibility, architecture boundaries, and test integrity.
|
Owner's decision on the one open item: the row field for whether a stored model is in effect (LivXue findings 1 and 2, the raven_loop.py:340 thread) is a feature the prototype does not draw, so it is deferred -- not in this PR and not tracked as an issue for now. Every other finding in the round is fixed or answered on its thread. |
The Claude Code preset could not connect for an account whose Claude settings name a recent model: claude-agent-acp@0.66.0 pins a Claude Agent SDK whose bundled CLI is 2.1.220, and the CLI refuses such a model with "version 2.1.251 or newer is required", so every connect from the row ended in that refusal. 0.79.0 bundles CLI 2.1.274 through the same per-platform SDK package, answers the handshake with resume, fork and load, six model choices and five modes, and still stamps the tool name, the raw output and the plan frames the claude_code dialect reads. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The connect proved the agent answered and recorded nothing, so a row connected from the page read "capabilities not recorded -- run a test", stateless (no instance, so no direct chat) and menuless (no model pill) until someone pressed Test or the gateway restarted into the boot backfill. Measured on the served page: Claude Code connected in 9 s and an instance was then refused as stateless. record_capabilities is the one writer the manual test and the connect share, and capabilities_wanted is the backfill's own three cases -- no record, a launch config that changed, a record from before the menu -- so a complete record is not re-measured on every connect. Best effort: the agent has already proved itself, and a handshake that fails afterwards is logged rather than turned into a refusal. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
|
Two more pre-existing defects found by the real-machine acceptance on this branch, fixed in faefa84 and 3d34c9d: the Claude Code preset's adapter pin moves from 0.66.0 to 0.79.0 (0.66.0 bundles CLI 2.1.220, which refuses any recent Claude model with "version 2.1.251 or newer is required", so the row could not connect for such an account; 0.79.0 measured: handshake with resume/fork/load, six model choices, five modes, connect from the row in 9 s, a tool-using turn rendered with the same _meta.claudeCode.toolName / rawOutput / fence shapes the dialect reads), and a row connected from the page now records its capability snapshot at connect (record_capabilities, shared with the manual Test; only when the record is missing, stale or from before the menu), where before it stayed stateless and menuless until a Test or a restart. Tests: test_rpc_subagents.py test_add_records_the_capabilities_of_an_acp_row_that_answered and siblings, test_subagent_acp.py test_the_connect_records_what_a_test_would. |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The acceptance-test delta is sound. The Claude ACP preset now pins a published adapter whose SDK dependency matches the documented compatibility rationale, while retaining the measured dialect shapes. The add/enable path records capabilities only when the snapshot is missing, stale, or predates model menus; it shares the manual-Test writer, treats recording as best effort after a successful ping, and leaves concurrent config changes detectable through the existing fingerprint. I found no new plain error.
Verification: uv run pytest tests/test_rpc_subagents.py tests/test_subagent_acp.py tests/test_subagent_probe.py tests/test_subagent_third_party.py tests/test_acp_dialects.py -x (665 passed); Ruff passed on all touched Python files; source-language, large-file, and git diff --check gates passed. I also verified the npm package metadata for @agentclientprotocol/claude-agent-acp@0.79.0. This pass covered the new delta plus the previously reviewed target-diff context, affected callers and history, project rules and domain context, backward compatibility, architecture boundaries, and test integrity.
Summary
Takes the web UI's agents page (rail entry "Agent Hub", page "Agents") to the prototype's agent hub, and gives the wire what the prototype needed and did not have: a default model on an agent row.
Base:
refactor/ui_web_architecture, the long-lived architecture branch (#475), notmain. The diff shown here is the diff that merges into that branch.What the page does now:
The model pill, by the row's own rule (
model_source, a new wire field):stored_model_id, the wayconfig.set modelstores the host's), validated against a provider raven knows that holds a usable credential, and paired with that credential on every dispatch throughProviderPool.bind_pin-- the backend resolves the pin per run, so a cached backend never runs one conversation's key against another's model. The write and the dispatch read the provider off the stored id through one function,stored_provider_name(the inverse ofstored_model_id), so a section raven has no spec for resolves to that section on both sides.model_choices) and the value is pushed oversession/set_config_option, through one resolver (session_model_for) for a spawn, a direct chat and a DAG node (model_forthreaded through the runner).own(a new field: built-in, vendored, or an acp row whose handshake named Raven), with the catalogue as the fallback for a server that predates it. The picker offers listed ids only; there is no typed entry, since an acp row's write takes exact menu values and a built-in row's adds to no provider.Deviations from the prototype, deliberately:
Install data (commands and sites) comes from each vendor's own documentation: Claude Code, Codex, OpenCode, Hermes, OpenClaw, MiroThinker, GitHub Copilot, Qwen Code, CodeBuddy, Qoder, Grok, Kimi Code, Pi.
Wire changes:
SubagentRow.own / model / model_choices / model_source;subagents.updatetakesmodel,provider,clear_model(clear wins) anddescription: null(back to the factory text; a built-in row goes back to following its seed);subagents.testtakessource: "vendored";ThirdPartyAcpSubagentConfig.model(andschemas/subagent.schema.jsonre-exported for it);SubagentManager(provider_pool=...)is injected by the loop and the playbook CLI.Reviewed in two passes (three read-only reviewers plus a refuter per finding): ten findings confirmed, all fixed in the backend commit -- among them a dispatch that withheld the model from a cached built-in backend, a
model_sourcethat promised a write the server refused, and a built-in override edit that would have shadowed a legacy-spelled row.Review round (gloryfromca, LivXue): all five of the first review's blockers and the second review's two blockers plus its two "fix or file" items where a fix was in reach are addressed in four commits -- the one-resolver read of a stored built-in model (
stored_provider_name) with the spec-less section case tested on both sides and an unreadable config raised rather than blamed on the pick, the unusable-pin warning keyed per pin, a refused acp push said once per value instead of once per turn, the wire'sownflag read by the page, the stored-prefix spelling matched to the host slug, no typed picks, a Stop for a running test, the model op held on the row, and the stale narrative in the manifest, the CONTRIBUTING ledger rows and two comments. The first reviewer's two later nonblocking follow-ups are in as well: the Claude Code preset no longer requires aclaudeon PATH (the adapter's SDK pin carries the CLI), and a stored id whose prefix names no provider section is read as part of the id, so a hand-written gateway-served id keeps its pin. The second review's finding 4 is fixed on the record side: a failed manual Test writes its verdict over the previous record's capabilities rather than erasing them, and a row recorded before the menu existed reads "attention, model menu not measured" until a re-verify lands instead of a stored "ready" (CapabilitySnapshot.model_menu_measured). Deferred by the owner as a feature the prototype does not draw: a row field saying whether the stored model is in effect (pin resolved / push accepted), which would be the durable answer to the second review's "silently" in findings 1 and 2. Not in this PR and not tracked as an issue for now.Four pre-existing defects the live regression surfaced are fixed here as well, each in its own commit:
raven acpansweredsession/set_config_option(andsession/new) with the process default as the model option'scurrentValue, because it askedmodel.optionswithout a session id. A client that renderscurrentValueas the selected item snapped back to the old model after every switch. Both call sites now ask about the session whose answer it is.argv[0]alone, and a shim-launched preset's command is annpxone that resolves on any machine with node, so Pi read as connectable whereverpiwas not installed and the connect failed a minute later inside the adapter.SHIM_REQUIRED_EXECUTABLES(inpresets.py) names the local agent a shim reaches for with its install command; the probe asks after it too and reports the row missing with the install beside it. Only Pi is listed:codex-acpbundles its agent, andclaude-agent-acpruns the CLI its@anthropic-ai/claude-agent-sdkpin carries as a per-platform optional dependency (verified in the 0.66.0 package: the SDK's platform package holds theclaudebinary, and the live run's "Claude Code 2.1.220" is that bundled build, not the machine's), so neither wants an install.claude-agent-acp@0.66.0bundles CLI 2.1.220 through its SDK pin, and the CLI refuses such a model ("version 2.1.251 or newer is required"). The pin is now 0.79.0 (CLI 2.1.274), measured: handshake with resume, fork and load, six model choices, five modes; connect from the row in 9 s where 0.66.0 failed after 155 s.record_capabilities, shared by both), only when the record is missing, stale or from before the menu.Type
Verification
Backend (worktree venv with the dev extra):
Web and TUI:
Served page (a test gateway from this branch, an isolated RAVEN_HOME copied from a real one): roster of 5 connected / 4 available / 9 not installed with display names; Raven-Research's pill lists the 55 choices its handshake advertised, a pick lands on the row and reads back from
subagents.list; the built-in row's pick is stored asopenrouter/anthropic/claude-opus-5and clears back to null; the three products whose snapshots predated the menu key were re-verified at startup and got their menus.Live regression on the same gateway: a product row switched off and back on through the row control (the readiness ping relaunches it); MiroThinker connected with a key typed in the sheet; an absent row's install block, copy and re-check ("still not found"); a pick and a clear on Raven-Code's own menu. Then, with the session on DeepSeek and Raven-Research's row set to
deepseek/deepseek/deepseek-v4-flashfrom its menu, one direct-chat turn to a Raven-Research instance: the host sentsession/set_config_option {configId: model}with that value (acp frame journal), the child loggedSession model: deepseek/deepseek-v4-flashand ran the turn on it (Iteration 1/60 model=deepseek/deepseek-v4-flash), and answered in 15 s. Connecting Claude Code, Codex and Pi from the row each ended in the server's refusal shown red on the row with Retry -- the machine's Claude Code CLI is too old for the adapter's model, Codex is not logged in, Pi's executable is not installed -- so their Test buttons were not reached.Risk
Behaviour change: the agents page is redrawn; a built-in row's
modelin config (previously masked by the turn binding on every dispatch) now takes effect, paired with its own credential, and falls back to the conversation's model when no credential can serve it (one warning in the log). An acp row'smodelis new and off by default.The Pi row now reads "not installed" on a machine without the
piexecutable, where it read "available" before; a hand-written row is not held to this, only a row carrying the preset's provenance.An acp row whose capability snapshot predates the model menu reads "attention" with a "run a test" detail until the boot-time re-verify or a manual Test records the menu; before, it read the old "ready" with a disabled model pill. A failed manual Test no longer drops a row's recorded menu and statefulness.
A built-in row's bare stored id (written by hand before this branch) is now paired by keyword on read and re-stored prefixed on its next write; a pick under a spec-less section is accepted only for an id that section lists. A hand-written id whose prefix names no provider section (
deepseek-ai/DeepSeek-V3) keeps its provider derived by the pool -- the configured gateway, as such an id always ran.Rollback: revert the PR; the new config fields (
modelon an acp row, a built-in override row) are ignored by the previous code.Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes
Related Issues
N/A