refactor(acp): define the capability sets in the acp_backends leaf - #7672
Conversation
The five ACP_BACKENDS_* capability sets lived in kiro_crew.acp.types. That package is a FORBIDDEN_ROOT for the agent-SDK boundary gate, so any consumer outside the ACP layer that asked a capability question had to add a forbidden edge to a shrink-only baseline. Definitions move to the leaf kiro_crew.acp_backends and are re-exported from acp.types, so no call site changes and membership is unchanged.
Design Review (Fable 5) — ✅ PASSDesign-level review of Reviewed the full diff, the PR description, and the destination module's existing precedent (backend id constants already live in the Design-Verdict: PASS Named harm (shrink-only baseline vs. the correct capability-query pattern), fixed at root by the leaf module built for exactly this, with tests pinning both halves. [DESIGN-REVIEWED] 2e76e16 |
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: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified. The boundary gate ( First-Principles-Verdict: PASS Cause-level fix to a real gate-vs-pattern conflict, following the file's own documented precedent; the only excess is redundant guard tests. What this change shipsIntent: let code outside the ACP layer ask "does the selected harness support X?" without a forbidden import edge — a FIX for the conflict between the agent-sdk boundary gate and the capability-set pattern.
WatchThe ratchet test is parametrized over exactly the five existing names, and AGENTS.md/harness-parity docs still direct authors to "a named Subtractions
[FIRST-PRINCIPLES-REVIEWED] 2e76e16 |
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: |
buluoray
left a comment
There was a problem hiding this comment.
Post-merge audit — this landed at 2e76e16e1 while I was reading it, so framing these as follow-ups rather than merge blockers. Verdict: a clean refactor that does exactly what it claims and nothing else. One line of spec drift worth fixing; the rest are nits.
What I verified
It is a pure move, byte-for-byte. All five frozenset values are identical pre- and post-move, the entire relocated comment-plus-definition block is byte-identical between the old acp/types.py and the new acp_backends.py, and no definition is left behind in types.py. That is the whole risk surface here — a capability set is opt-in membership, so a silent membership shift during a relocation would hand a harness something nobody granted it (H6/H7). It didn't happen, and test_membership_is_unchanged_by_the_move pins it going forward.
The leaf property is real, not aspirational. acp_backends.py imports only logging and typing. The motivation checks out too: scripts/check_agent_sdk_boundary.py:101 carries FORBIDDEN_ROOTS = ("kiro_crew.acp", "kiro_crew.providers"), so an outside consumer really would have had to add a forbidden edge against a shrink-only baseline just to ask a capability question.
Re-export hygiene is right. The # noqa: F401 - re-exported for existing importers is in place, and test_the_re_export_is_the_same_object asserts identity rather than equality — which matters, because the one existing reader (acp/session_provider.py:36) still resolves through the re-export.
The relocated doc citation is accurate. agent-host-contract.md now points at acp_backends.py:303-316; the comment block runs 303–315 and the definition is on 316. Worth saying because moved line-range citations are usually the first thing to rot.
Mutation-verified the new guards — five mutations, tree restored clean after each:
| Mutation | Result |
|---|---|
Redefine ACP_BACKENDS_STEER in acp/types.py |
2 failed ✅ |
Same, but annotated (: frozenset[str] = …) |
1 failed |
Grant KAS SESSION_SHARING |
2 failed ✅ |
Leaf imports kiro_crew.acp |
circular ImportError — inconclusive |
| Re-export becomes a copy | 2 failed ✅ |
13/13 pass at baseline, and scripts/check_harness_parity.py passes against origin/main.
Findings
1. Spec drift — harness-parity.md:81 still points at the old home (should fix, one line)
The H6 row's pointer column still lists acp/types.py as where the capability sets live. That is the spec this PR implements, and AGENTS.md requires the spec to move in the same commit. The H8 row two lines below is already correct (acp_backends.py is the home, acp/types.py re-exports), so the file now contradicts itself on the same page.
2. The location guard is evadable, though coverage is not (nit)
test_defined_in_the_leaf_not_in_the_acp_package asserts on the substring "\nNAME = frozenset(", so an annotated redefinition back in acp/types.py slips past it — that is the 1-failure row in the table above. The suite still fails closed, because the identity guard catches the same mutation. But the assertion that goes silent is the one whose message actually explains the problem ("define it in the leaf acp_backends.py so a consumer can read it without importing kiro_crew.acp"), so a future author gets the less useful failure.
The file already imports ast for literal_eval — walking Assign / AnnAssign targets would make the check exact for the same cost.
3. Pre-existing doc drift this PR widens (nit)
docs/ci/harness-parity-gate.md:36 describes the vocabulary-home rule as flagging an identifier or set "defined outside acp/types.py", while scripts/check_harness_parity.py:90 has VOCABULARY_PATH = "src/kiro_crew/acp_backends.py".
To be fair on attribution: I checked the history and that line was written 2026-08-13 in #3400 and went stale on 2026-08-28 when #6593 moved VOCABULARY_PATH — so it is not this PR's doing. But this PR extends what it misdescribes from identifiers to capability sets, which makes it a natural thing to sweep while the context is fresh. Same for the fix-hint string at check_harness_parity.py:144 ("the named constant from acp/types.py") and the two RFC citations at rfc-crew-agent-sdk-boundary.md:368 (acp/types.py:151) and :1252 (acp/types.py:175-181).
Two observations, neither a defect
No consumer benefits yet. The only reader of a capability set outside acp_backends.py is acp/session_provider.py, which lives inside kiro_crew.acp and therefore never paid the forbidden edge. So this changes no import-graph edge today; the payoff arrives when readiness / prerequisite / MCP-wiring consumers start asking these questions. The test docstring frames it honestly as prospective ("onboarding a new harness means teaching outside consumers … so that cost was about to be paid repeatedly"), which is the right justification — just worth stating that it is prospective.
One guard I could not falsify. Making the leaf import kiro_crew.acp at module level produces a circular-import crash rather than a clean red, because kiro_crew.acp transitively reaches config.loader, which reaches acp_backends. So the leaf property is currently enforced structurally, and test_reading_a_capability_set_does_not_load_the_acp_package is belt-and-braces against a future restructure where that cycle no longer exists. Cheap and forward-looking — I would keep it.
Problem / Motivation
The five ACP capability frozensets —
ACP_BACKENDS_SESSION_SHARING,ACP_BACKENDS_STEER,ACP_BACKENDS_INTERNAL_SANDBOX,ACP_BACKENDS_ACP_RUNTIME,ACP_BACKENDS_KIRO_IDENTITY_STORE— were defined insrc/kiro_crew/acp/types.py.kiro_crew.acpis aFORBIDDEN_ROOTfor the agent-SDK boundary gate, and importing anything under it executes that package's__init__(ACP client + runtime). So a consumer outside the ACP layer could not ask "does the selected harness support X?" without adding a forbidden import edge, to a baseline the gate only ever lets shrink.This is not hypothetical. #6777 (OpenCode harness) adds exactly that edge to
dashboard/kiro_readiness.py:That is the correct pattern — ask a capability set, never branch on harness identity — and today it costs a forbidden edge to write.
Why it matters
Onboarding a harness is precisely the act of teaching outside consumers to ask these sets. The two live onboarding PRs each touch ~10 files outside
acp/andproviders/: readiness (kiro_readiness.py,kiro_prerequisite.py), MCP wiring (mcp_gateway/session_servers.py, +112 in #6777), agent config (dashboard/handlers/agents.py, +213), andcli_doctor.py. Every one of those that asks a capability question pays the same edge.So the cost is per-consumer per-harness, and it is charged against a shrink-only ratchet — which means the gate and the correct pattern are in direct conflict until the definitions move.
What changed (motivation → approach → change)
Approach. The precedent already exists in the destination file.
acp_backends.pyis a documented leaf — it imports nothing fromkiro_crew.acp,kiro_crew.configorkiro_crew.platform, which is what lets the config loader and the dashboard read it — and it already hosts the backend id constants for exactly this reason, withacp.typesre-exporting them so no call site changed. Its own comment says so:The capability sets are the same kind of thing with the same consumers, so they get the same treatment.
Change. The definitions move to
src/kiro_crew/acp_backends.pyand are added to the existingfrom kiro_crew.acp_backends import (...)re-export block inacp/types.py. Every priorfrom kiro_crew.acp.types import ACP_BACKENDS_*keeps resolving, to the same object rather than a copy.The 65 lines of membership prose moved verbatim with the definitions rather than being retyped. That prose is the harness-parity evidence record — why KAS is in
ACP_BACKENDS_KIRO_IDENTITY_STOREbut not inACP_BACKENDS_SESSION_SHARING, and whyACP_BACKENDS_INTERNAL_SANDBOXdeliberately fails open — and separating it from what it explains would be the real loss here, not the line move.Membership is unchanged. Opting a harness into a capability is a deliberate edit with evidence (harness-parity H5/H6); a relocation is not the place for it. The five sets are byte-identical and pinned by a test.
Not changed:
docs/system-specs/modules/harness-parity.mdand the H6/H7 rule text still nameacp/types.py. Those references still resolve through the re-export, so repointing them would be churn beyond this PR's purpose.Tests
test/test_acp_capability_sets_leaf.py, 13 tests:test_defined_in_the_leaf_not_in_the_acp_package(×5) — the ratchet. A future edit moving one back would compile and pass everything else while silently re-imposing the forbidden edge on every consumer that reads it.test_the_re_export_is_the_same_object(×5) — identity, not equality: a copy would drift the moment either side is edited.test_reading_a_capability_set_does_not_load_the_acp_package— subprocess probe asserting neitherkiro_crew.acpnorkiro_crew.providersloads. The boundary gate cannot catch this regression: the consumers who would start paying that import are already in its baseline, so it would stay green.test_membership_is_unchanged_by_the_move— pins all five memberships explicitly.test_acp_runtime_is_a_superset_of_session_sharing— asserts the relationship the prose describes, so adding a harness to sharing without adding it to the runtime set fails rather than describing a backend that multiplexes sessions with no multiplexer.Mutation-verified: re-defining
ACP_BACKENDS_STEERinacp/types.pyfails both the ratchet and the identity test, and passes once reverted.Manual verification
N/A — pure relocation with no user-visible surface; equivalence is asserted directly.
Beyond the test suite I verified two things before committing: both import paths yield the same frozenset objects, and the five definitions parsed out of
origin/main'sacp/types.pyare identical to the five now inacp_backends.py.Gates green on this head:
agent-sdk-boundary,harness-parity,docs-lint(including the citation check), black, isort, flake8, mypy,subprocess-encoding,sync-io-in-async, brand, changelog,focus-cue,testpaths-coverage,vendor-manifest,scrub-lint. 1357 tests pass across the ACP-backend, harness-parity, agent-SDK, KAS and governance surfaces.Related Issues
no linked issue: this is the next step of the in-flight
agent_sdkboundary consolidation (RFC landed in #6939, continued in #7303 and #7523) rather than a discrete tracked issue.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)