Skip to content

refactor(acp): define the capability sets in the acp_backends leaf - #7672

Merged
chenmingwei23 merged 1 commit into
mainfrom
refactor/acp-capability-sets-leaf
Sep 1, 2026
Merged

refactor(acp): define the capability sets in the acp_backends leaf#7672
chenmingwei23 merged 1 commit into
mainfrom
refactor/acp-capability-sets-leaf

Conversation

@iamwhatever

Copy link
Copy Markdown
Collaborator

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 in src/kiro_crew/acp/types.py.

kiro_crew.acp is a FORBIDDEN_ROOT for 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:

from kiro_crew.acp.types import ACP_BACKENDS_KIRO_IDENTITY_STORE

async def selected_backend_uses_kiro_identity() -> bool:
    backend = getattr(cfg.agent, "acp_backend", ACP_BACKEND_KIRO)
    return backend in ACP_BACKENDS_KIRO_IDENTITY_STORE

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/ and providers/: readiness (kiro_readiness.py, kiro_prerequisite.py), MCP wiring (mcp_gateway/session_servers.py, +112 in #6777), agent config (dashboard/handlers/agents.py, +213), and cli_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.py is a documented leaf — it imports nothing from kiro_crew.acp, kiro_crew.config or kiro_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, with acp.types re-exporting them so no call site changed. Its own comment says so:

acp.types re-exports these, so every existing call site keeps importing them from there; this module is only where they are DEFINED.

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.py and are added to the existing from kiro_crew.acp_backends import (...) re-export block in acp/types.py. Every prior from 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_STORE but not in ACP_BACKENDS_SESSION_SHARING, and why ACP_BACKENDS_INTERNAL_SANDBOX deliberately 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.md and the H6/H7 rule text still name acp/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 neither kiro_crew.acp nor kiro_crew.providers loads. 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_STEER in acp/types.py fails 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's acp/types.py are identical to the five now in acp_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_sdk boundary consolidation (RFC landed in #6939, continued in #7303 and #7523) rather than a discrete tracked issue.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

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.
@iamwhatever
iamwhatever requested a review from a team as a code owner September 1, 2026 16:26
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 2e76e16e1e18fd0af1fc70e660a1606e71d9eb29 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Reviewed the full diff, the PR description, and the destination module's existing precedent (backend id constants already live in the acp_backends leaf with the same re-export pattern).

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2e76e16e1e18fd0af1fc70e660a1606e71d9eb29 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 2e76e16

Verdict parsed from the review's SHA-scoped output markers for commit 2e76e16e1e18fd0af1fc70e660a1606e71d9eb29.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 2e76e16e1e18fd0af1fc70e660a1606e71d9eb29: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 2e76e16e1e18fd0af1fc70e660a1606e71d9eb29 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. The boundary gate (scripts/check_agent_sdk_boundary.py) is real with a shrink-only baseline; acp_backends.py documents itself as the leaf and already hosts the backend identifiers with the identical re-export pattern; the five sets have ~12 real read sites today (providers/acp.py, acp/client.py, acp/runtime.py, acp/session_provider.py, session.py); membership moved byte-identical. The main redundancies I found are inside the new test file itself and against existing pins in test_harness_parity.py and test_agent_sdk_backend_identity.py.

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 ships

Intent: 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.

  1. Capability sets readable from the leaf acp_backends without loading kiro_crew.acp — justified (gate is real; precedent documented in the destination module)
  2. acp.types re-exports the same objects, all ~12 existing read sites unchanged — justified
  3. Membership prose moved verbatim with the definitions — justified
  4. New 13-test file pinning location, identity, no-load, membership — justified, internally redundant (see Subtractions)
  5. runtime ⊇ session-sharing relationship newly asserted, not just described — rides along, declared, low-cost
  6. agent-host-contract.md citation repointed to acp_backends.py:303-316 — undeclared, but derived (citation must resolve)
  7. harness-parity docs still say acp/types.py — declared non-change

Watch

The ratchet test is parametrized over exactly the five existing names, and AGENTS.md/harness-parity docs still direct authors to "a named ACP_BACKENDS_* set in acp/types.py" — so a sixth capability set defined in types.py lands green and re-imposes the forbidden edge the move just removed. The description acknowledges the doc non-change but not this gap.

Subtractions

  • Drop test_defined_in_the_leaf_not_in_the_acp_package (×5): every behavior-affecting mutation it catches also fails test_the_re_export_is_the_same_object — the author's own mutation run confirms both fail on redefinition in types.py; its unique coverage is a dead shadowed definition.
  • Shrink test_membership_is_unchanged_by_the_move: test_harness_parity.py:297 already pins ACP_BACKENDS_INTERNAL_SANDBOX == {KIRO} as equality (partial pins at 221–232), so future opt-in edits now touch two pinning files.

[FIRST-PRINCIPLES-REVIEWED] 2e76e16

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 2e76e16e1e18fd0af1fc70e660a1606e71d9eb29 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2e76e16

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 2e76e16e1e18fd0af1fc70e660a1606e71d9eb29: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23
chenmingwei23 merged commit ced4df2 into main Sep 1, 2026
148 of 151 checks passed
@chenmingwei23
chenmingwei23 deleted the refactor/acp-capability-sets-leaf branch September 1, 2026 22:19
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 1, 2026

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⚠️ (see below)
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants