fix(acp): ship Claude Code as selectable and hide agents policy forbids - #7301
Conversation
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Hiding policy-denied agents works, but the page header still statically promises "Kiro CLI, Claude Code, or KAS" a denied deployment can't see. Watch
Suggestions
[UX-REVIEWED] 73ae069 |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of I have what I need. Composing the review. Design-Verdict: CONCERNS Sound widening with the right controls sequenced first, but it ships a live audit/deny bypass on default-allow, mitigated only by a caveat sentence. Watch
Suggestions
[DESIGN-REVIEWED] 73ae069 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All claims verified: First-Principles-Verdict: CONCERNS Every item names its harm; the one soft spot is hiding the policy-denied row — argued from principle, with no reported user failing at the greyed chip. What this change shipsIntent: let an operator who has the Claude binaries actually pick Claude Code, and stop the panel lying about why they can't — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 73ae069 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
8493fed to
116e1e1
Compare
116e1e1 to
5f5aaa0
Compare
5f5aaa0 to
040d44f
Compare
040d44f to
be6c842
Compare
be6c842 to
4880fc3
Compare
|
|
|
/ai-review override gpt 4880fc3: Mechanism confirmed against Anthropic's SDK docs and accepted; the prescribed fix is declined. Removing ACP_BACKEND_CLAUDE from the baseline does not close the bypass, it only hides a harness that acp/client.py already implements and that any edition could already register — the exclusion constrained the switch, never the harness. Honouring .claude/settings.json is documented, intended Claude Code behaviour. Kiro Crew officially supports the Kiro product lines (kiro-cli, KAS), which do run under Crew's approval path; Claude Code is offered as a provider with its own governance model, stated at the point of choice on the Agent Backend panel in all 12 locales, in docs/system-specs/features/claude-code-provider.md, and in AGENTS.md (amended in this PR so the repo rule matches shipped behaviour). Third round on this same span with the same prescription. Prerequisite for closing even the pre-approved case — whether claude-agent-acp forwards PreToolUse or settingSources over ACP — is named in the spec as follow-up work. |
|
AI-review override not recorded: keep the reason to 500 characters or fewer. |
4880fc3 to
dc151b8
Compare
The harness was runnable and unreachable: acp/client.py owns its whole spawn path and the adapter is a public npm package, so the only thing missing was the switch.
dc151b8 to
73ae069
Compare
|
|
…end architecture Upstream shipped a parallel backend architecture while this branch was built (kirodotdev#7166 install-state probing + policy-gated selection, kirodotdev#7301 Claude Code selectable, plus the subagent_manager/ and session_* decompositions). This merge adopts it as the foundation: acp_backends.py is the backend-id vocabulary leaf; Claude Code is serviceable (our wave-1 refusal posture dropped as factually superseded); the install probe and governance selectability layers gate the bundled rows. Our registry generalizes their closed set on top: operator descriptors, GenericAdapter serving, HarnessBinding per-chat/spawn/cron selection, and fail-closed capability gates all preserved, with the subagent/session harness logic re-homed into upstream's decomposed components and AgentBackendTab superseded by HarnessPanel carrying its install-state and policy display.
Problem / Motivation
Developer → Agent Backend renders Claude Code as permanently unavailable on every
public build, and the reason it gives — "Not enabled in this build" — is not true.
acp_backends.BASELINE_SELECTABLE_BACKENDSshipped{kiro, kas}and excludedACP_BACKEND_CLAUDE, describing it as "a dormant seam ... not something a public buildcan serve a session with". That description does not match the code:
acp/client.pyowns the entire Claude spawn path — the_is_claudebranch,_resolve_claude_acp_bin,_resolve_claude_code_executable, the argv cache.CLAUDE_ACP_NPM_PKG(@agentclientprotocol/claude-agent-acp).providers/acp.pyconstructs it, andPROVIDER_LABEL_CLAUDElabels its sessions.Nothing about the harness is edition-private. The exclusion removed the switch, not
the capability, so the one thing standing between a public user and a Claude session was
a set literal.
Second problem, in the same panel: an agent the deployment may not select was rendered
greyed with an explanation. That is the wrong treatment for a restriction.
Why it matters
The first is a dead end with a misleading sign on it. #7166 added an install probe so the
panel could say what is missing and how to install it, but that machinery never ran for
Claude Code, because the build gate was checked first and always lost the option. An
operator who has
claudeand wants the adapter was told the build could not do it.The second is worse than saying nothing: a greyed chip invites the reader to go find out
how to enable it, and under a managed policy there is nothing they can do from their
machine. Advertising a forbidden option is also the opposite of what the restriction is
for.
Fix (symptoms → root cause → change)
1. Claude Code joins the public baseline.
BASELINE_SELECTABLE_BACKENDSgainsACP_BACKEND_CLAUDE, so the baseline is now everyACP_BACKENDS_KNOWNid, and the comment states what makes that correct rather thanasserting dormancy.
Usability on a given host is a separate question, and #7166's probe already answers
it: without the adapter the option is disabled with the component named and the command
that installs it. That is the line the user needed all along; it just had a build gate in
front of it.
The test pinning the old set existed "so a widening is deliberate". This is that
deliberate widening, and the assertion now reads the other way round — a future
narrowing has to state its reason next to the removal.
2. An unselectable agent is hidden, not dimmed.
AgentBackendTabnow filters the rendered set instead of disabling it, which splits thetwo facts onto the two treatments they deserve:
sentence is what explains the absence.
installed === 'missing' || restart_required. Both are things the user can act on, which is why they are shown atall, and the status line carries the action.
The
not_enabled_in_this_buildstring is therefore unreachable and is deleted, fromen.jsonand all 12 other locales.Two properties are load-bearing:
a denied persisted value to the floor on load, so this should not arise; if it ever
does, a control rendering no pressed chip is a worse failure than one extra row.
unavailable()is false when neither the schema nor the probe hasanswered, so an in-flight query, a 403 (non-owner) or a 404 (older gateway) hides
nothing and disables nothing.
3. Everything the widening falsified.
Sixteen sites described the Claude seam as dormant, or the public edition as
Kiro-CLI-only, or referenced the deleted string. Each is corrected in place across
platform/bootstrap.py,platform/interfaces.py,platform/defaults.py,providers/acp.py,config/loader.py,session.py,dashboard/chat_utils.py,dashboard/handlers/agents.py,acp/client.py,acp/types.py,acp_backends.pyand onetest docstring. Where a comment had a real point underneath the stale adjective it keeps
it —
providers/acp.pystill excludes the Claude client from the kiro-family runtime set,and now says why (one process per session, no shared runtime) instead of leaning on
"dormant".
4.
kirocrew doctorreported it wrongly, to the user.Doctor printed
claude-acp: ✅ <path> (dormant seam — not used by the public core), andprinted it only when the adapter happened to be installed. Both halves were wrong: an
operator who could select the harness was told the build did not use it, and an operator
who had not installed it was told nothing at all — which is doctor's entire job.
It now reports Claude Code as a real optional backend either way, and takes its verdict
from
agent_sdk.probe_backend— the same ownerGET /api/acp-backendsuses — so doctorand the panel cannot drift. A half-install names the absent component and the install
command, matching the dashboard line exactly.
5. Four spec docs.
features/claude-code-provider.mdwas titled "dormant ACP seam"; it is now "a selectableACP harness" and separates three questions the old text conflated — registered (build),
permitted (governance scope), usable (probe).
features/agent-host-contract.mdhad thedormancy premise under its conditional-surface reasoning (eleven places, including host
seam bucket 8 moving from "none — no preflight" to "partial", since the probe now exists
but a declared requirement still does not).
modules/harness-parity.mddrops the dormantframing and records that
ACP_BACKENDS_KNOWNand the baseline are now coupled.features/README.mdgets the index line for the retitled doc.6. What Crew gates on this harness, and what a pre-approval skips — stated in the panel.
By default Claude asks and Crew decides: with no matching rule, every tool call reaches
the SDK's
canUseToolcallback, which is whatclaude-agent-acpturns into ACPsession/request_permission, which runshooks.on_tool_calland lands in the SEL record.A Claude session is governed like any other on that path.
What escapes is a call that was already pre-approved, so it never asks. Allow rules sit
at step 5 of the SDK's six-step evaluation, ahead of the callback at step 6, and
Anthropic's docs state the consequence in bold: "Auto-approved tools never reach
canUseTool." No callback means no ACP request, so for that one call there is nothing forCrew to gate or record. It matters here rather than being purely the operator's own choice
because the SDK reads
.claude/settings.jsonfrom the project directory by default —a cloned repository can carry allow rules its author wrote — and our core passes nothing
that would change it (
_permission_modeis stored and never read; nosettingSources, noPreToolUsehook, no settings seed).That is documented, intended Claude behaviour, not something this PR breaks — the harness
was already implemented and reachable by any edition that registered it, so the old
exclusion bought invisibility rather than enforcement. So this PR ships it and states the
narrow difference at the point of choice: a standing line on the Claude row, present
whenever the harness is offered and independent of the install probe, in all 12 locales.
Kiro CLI and KAS have no equivalent settings file that can pre-approve past Crew's gate and
deliberately carry no such line — claiming it for them would be false and would train the
reader to skip it.
Closing even the pre-approved case needs one of the two mechanisms Anthropic documents (a
PreToolUsehook, whose deny holds even inbypassPermissions, or excludingprojectfrom
settingSources). Whetherclaude-agent-acpforwards either over ACP is notanswerable from this repository, and is named in the spec as the prerequisite for gating
every Claude tool call rather than every call Claude asks about.
A pinned model reached the adapter in the wrong namespace. Making the backend
selectable put a second consumer behind one model translation that only ever had one.
KiroCrewConfig.acp_effective_model— the factory's own selection, shared with thespawn-side effort verdict — hardcoded
model_registry.to_acp_id, whose own docstringsays it produces kiro-cli ids. So a concrete
agent.modelhanded the claude adaptera kiro-namespaced id, which its
set_config_optionrejects, and nothing withheld it: thepre-wire availability guard in
acp.clientis deliberately kiro-only, because the twobackends advertise in different namespaces and widening the check would call every
legitimate claude model unusable.
The give-away is that the warm-pool model-switch path in
session_allocationalreadykeyed this translation on the backend, so the same pinned model translated correctly on a
warm claim and incorrectly on a cold start — the outcome depended on whether a pooled
process happened to exist. The fix makes the cold path agree with the warm one rather
than inventing a rule:
to_provider_id(m, "claude_code")on the claude backend,to_acp_idotherwise.autocollapses to""in both, which is why enabling thisbackend by editing config.json works today and why the defect was never on the default
path — the client skips the model send entirely when nothing is pinned.
The gap this does NOT close
AcpClient._claude_session_mcp_servers()returns[]by default, and theclaude-agent-acp adapter does not read
kirocrew.mcp.jsonon its own. So a Claude sessionon this build starts with zero MCP tools — the harness works (prompts, streaming,
model/effort, the permission flow) but
kirocrew-core, cron and every user MCP server areabsent. kiro-cli is unaffected; it gets its servers via
--agent.This is now stated in the code comment, in
claude-code-provider.mdunder its own### Known gapheading, and inagent-host-contract.md§5, rather than being left forsomeone to discover mid-session. Closing it means translating
kirocrew.mcp.jsonintothat array, which is a separate change with its own tests.
Tests
test/test_agent_backend_editable.py— the baseline assertion is rewritten to the newintent and additionally pins
baseline == ACP_BACKENDS_KNOWN, so adding a known idwithout shipping it, or shipping one without making it known, both fail.
The registration-reaches-the-allowlist test can no longer borrow Claude Code as the
"not yet registered" id, so it constructs that state. What it actually tests is
unchanged and now unmistakable: the allowlist resolves the registry per call.
restore_registry(and the equivalent inline snapshot intest_harness_parity.py) nowrestores
_baselineas well as_selectable.register_selectable_backendwritesboth, so restoring one leaked a widened baseline into every later test in the run —
a real defect, latent until a test needed to narrow the baseline.
test/test_harness_parity.py— H3's degradation row keepsclaudein theparametrize for the opposite reason: it must now survive. The assertion is
membership-conditional, which is what makes that row prove the gate reads the registry
instead of hardcoding a verdict.
website/src/test/AgentBackendTab.test.tsx(26) — hiding replaces dimming andoutranks both a
missingverdict and the restart line, so neither leaks into the DOM;the selected backend stays visible even when it reads as unselectable; widening the
schema makes a hidden row appear with no edit to the component; a 403 hides nothing;
and the aria-describedby association is re-pinned on the remaining disabled case (a
missing install), where it now carries an actionable line rather than a dead end.
test/test_doctor_claude_backend.py(3, new) — doctor reports the harness whether ornot it is installed; it never calls it dormant; and a half-install names the absent
component with the command on the following line. Mutation-checked: removing doctor's
missing-component line reddens two of the three and nothing else.
test/test_acp_effective_model_backend.py(5, new) — the cold-start model id istranslated into the namespace of the backend that will run it: a canonical key
resolves to a claude provider id on the claude backend and stays a kiro id on kiro
and kas, an explicit
model_overrideis translated too, andautostill pinsnothing on claude (the default-config path, which is why enabling the backend by
hand works today). Mutation-checked: restoring the hardcoded
to_acp_idreddensexactly the two claude-namespace cases.
test/test_doctor_claude_backend.py(+2, and two existing assertions retargeted)— doctor never labels an install "selectable". It reads the install probe and
never consults
apply_selectable_denials, so claiming selectability would printthe opposite of the truth to the operator of a policy-denied deployment. The two
older tests asserted the previous wording incidentally; their intent (an off-PATH
install is not printed as
None, and the harness is never called dormant) isunchanged.
Manual verification
(
website/scripts/capture-agent-backend-probe.mjs) against the real bundle. The firstattempt reproduced the deleted string, which caught that the harness serves a prebuilt
dist— the frames below are from a rebuilt one.claude-acp: ⏭ claude-agent-acp not found (optional agent backend)followed by the npm command. Thethree new tests assert that exact shape rather than a mock's.
test_acp_client.py::TestResolveKiroBinEnvOverride::test_spawn_passes_installed_path_through_exact_wrappersfails in this worktree. It also fails on a pristine
origin/maincheckout with none ofthese changes applied (verified in a detached worktree), and this diff touches no spawn
kwargs — comments only in
acp/client.py. Pre-existing, not folded in here.Screenshots / video
Before this change, on this same host, the Claude Code chip was greyed and its line read
"Not enabled in this build" — no before frame is included because the capture harness now
builds the fixed bundle and cannot produce it.
After, on this host: Claude Code is offered, disabled only because the adapter is absent,
naming the component and the command that installs it.
A managed deployment whose policy denies the harness — the row is absent entirely, and
the footer sentence explains it:
The two remaining probe states, unchanged by this PR
Installed on disk, but this gateway process cached its absence — disabled, and worded as
a restart rather than as missing:
unknownleaves the option enabled and must not read as missing — the check failed,which is not evidence the binary is absent:
Related Issues
Follow-up to #7166, which added the install probe and the
agent_backendgovernancescope. That PR made the two reasons an option can be unavailable visible; this one
fixes the reason itself being wrong for Claude Code, and changes how the policy reason is
presented.
no linked issue: this closes nothing on merge — #7166 is already merged, and the
follow-up work named above (the MCP seat and the pre-approval gate) is not filed yet.
Pattern harvest
Rule candidate: review-prompt
Pattern: a registry entry excluded by a comment asserting a capability the code contradicts
The defect generalizes.
BASELINE_SELECTABLE_BACKENDSexcluded Claude Code with acomment claiming a public build "cannot serve a session with it", while
acp/client.pycarried the whole spawn path and the adapter was a public npm package.Nothing mechanical could catch that: the set was internally consistent, the tests
asserted the exclusion, and the comment justified it — so the exclusion outlived its
own reason and rendered a dead option for months. A reviewer prompt can ask the one
question that finds this class: when a registry, allowlist or capability set excludes a
member, does the code that would serve that member actually exist?
The same shape produced the two smaller bugs in this PR.
restore_registryrestoredonly
_selectablewhileregister_selectable_backendwrites_baselinetoo, anddoctor collapsed the probe's
unknownverdict into "not found" — both cases of asecond piece of state existing but one site behaving as if it did not.
Checklist