refactor: move event and stop-reason constants into agent_sdk - #9359
refactor: move event and stop-reason constants into agent_sdk#9359iamwhatever wants to merge 1 commit into
Conversation
|
Intent: Move the event-kind and stop-reason string vocabulary out of the ACP backend layer and into Not a goal: Paying off any boundary-baseline edge. |
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: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A real inversion fixed at its root — declaration moves to the leaf, re-exports preserve every consumer, and each drift channel (interning, direction, lazy edges) is pinned. The one structural cost — [DESIGN-REVIEWED] 3c6de2a |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All checks complete. The move itself is derived from a repo-recorded RFC ( First-Principles-Verdict: CONCERNS
Not justified as shipped
What this change shipsIntent: make the event-kind/stop-reason vocabulary importable from the SDK side of the boundary so later RFC slices and consumer migrations don't inherit backend imports — a MOVE, not a fix.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 3c6de2a |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe change is a clean re-export relocation. Verified independently:
The candidate list correctly identified no grounded defect, and I could not derive one to the required bar. No findings. [OPUS-REVIEWED] 3c6de2a Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
CI round 1 — every red is inherited from
|
CI round 2 — a second pre-existing failure, still nothing in this diff
Both attempts got
It cannot be reached from this diff. Both remaining blockers are pre-existing and belong to other PRs:
This PR's own state on Nothing further can be done here without either editing files outside this PR's scope or landing someone else's fix first, so the automated loop is stopping and handing over. |
Red attribution on
|
| Job | Failed | Test |
|---|---|---|
| Backend Tests (3.12, 3) | 1 of 22,885 | test_security_conductor_skill_contract.py::TestSkillIsInstallable::test_no_bundled_scripts_are_shipped_here |
| Backend Tests (Windows) (3) | 1 of 20,214 | same test |
| Backend Tests (Windows) (4) | 1 of 21,174 | test_work_ledger.py::test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding |
Shards 3 on both platforms fail on exactly one test and it is the same one — the stale staging guard tracked as #9363 (test(security-conductor): stale staging guard fails now that the ledger script ships), already labelled bug / auto-fixable / claimed. main's own CI is red on it at fd6684b40 and 2f9ed9724, and src/kiro_crew/builtin_skills/security-conductor/scripts is still present on origin/main. This PR changes no file under builtin_skills/, so the merge ref's content there is exactly main's. Fixes open: #9369, #9362.
Windows (4) is a different test and does not belong to #9363. It is the work-ledger binding race, failing twice on this head with invalid_value from one thread where already_bound is expected — the failure class its own assertion message names, and the subject of #9257 and #9237. test_work_ledger.py and src/kiro_crew/work_ledger.py import nothing from kiro_crew.acp or kiro_crew.agent_sdk, and test/test_agent_sdk_events.py is not on that shard at all.
Correction to my earlier note. I previously called Backend Lint & Type Check (3.12) a fail-fast cancellation. It is the 15-minute job timeout: started 03:12:21Z, killed 03:27:41Z at 15m20s, with Type check (mypy) as the one cut step. Everything before it passed on this diff — black, subprocess-encoding, comment-history, agent-SDK import boundary, blocking-IO, lockdown-before-publish, isort, flake8. Locally mypy src/kiro_crew/ is clean on 1333 source files. A re-run of the failed jobs on this head has been requested.
Coverage Gate cascades from the shards above and clears when they do.
The event-kind and stop-reason strings are what application code branches on, and they lived in `acp/types.py`. That put the vocabulary inside the backend layer the agent-SDK boundary exists to hide, so any consumer reading a kind had to name the ACP package to do it. `kiro_crew.agent_sdk.events` now declares them: the 19 `EVENT_*` kinds, a frozenset `ALL_EVENT_KINDS` over them, and the six provider-neutral `STOP_REASON_*` reasons. `kiro_crew.acp.types` imports the same objects back and re-exports every name, so `from kiro_crew.acp.types import EVENT_TOOL_CALL` still works and no consumer file changed. One reason stayed behind. `STOP_REASON_CONTENT_FILTERED_WIRE` is a harness's own spelling of a refusal, normalised to `STOP_REASON_REFUSAL` before any consumer sees it. It is a wire literal the parser and its tests share, so promoting it would export a name nothing above the boundary reads. Zero behaviour change: every string keeps its value, and every replay snapshot under `test/fixtures/acp_frames/` is byte-identical.
ac2ac7e to
3c6de2a
Compare
|
Closing: this moved EVENT_*/stop-reason constants into agent_sdk/events.py as infrastructure for PR2b/2c, which are cancelled. With zero consumers and no change to per-provider onboarding cost, the PR has no standalone value. #9381 (PR3a) does not depend on it. |
Problem / Motivation
The event-kind and stop-reason strings live in
src/kiro_crew/acp/types.py. Those strings are what application code branches on: an event'skind, a turn'sstop_reason. A dashboard handler that wants to know "was this a tool call?" has to import from the ACP package to find out.That is backwards. The ACP package is the backend layer the agent-SDK boundary exists to hide. Naming it is what the boundary gate counts as a violation, so the vocabulary sat on the wrong side of the line it is supposed to cross.
Why it matters
The boundary baseline records 106 imports of the backend layer from application code, and each one has to be paid off before a second backend can be added without editing consumers. Some of those edges exist only to read a string. Every later slice of the RFC's PR 2 -- the event object, the approval token, the error taxonomy -- lands on top of this vocabulary, so it has to move first or each of them carries the same wrong-side import along.
What changed (motivation → approach → change)
The strings were in the backend layer. The root cause is just where they were declared: nothing about a kind is backend-specific, because no backend puts these strings on the wire. The driver assigns them while translating a harness's own frames.
So a new leaf module
src/kiro_crew/agent_sdk/events.pydeclares them: the 19EVENT_*kinds, a frozensetALL_EVENT_KINDSover them, and the six provider-neutralSTOP_REASON_*reasons.acp/types.pyimports the same objects back and re-exports every name.from kiro_crew.acp.types import EVENT_TOOL_CALLstill works, so no consumer file changed.agent_sdk/__init__.pyexports the names too, and its twoTURN_STOP_REASON_*names became aliases of the moved reasons instead of a second spelling of the same literals.One reason stayed behind.
STOP_REASON_CONTENT_FILTERED_WIREis a harness's own spelling of a refusal. The parser normalises it toSTOP_REASON_REFUSALbefore any consumer sees it, so it is a wire literal the parser and its tests share, not vocabulary anything above the boundary reads. Moving it would export a name nobody up there can use.flowchart LR subgraph Before C1[consumer]:::ctx --> T1[acp/types.py<br/>declares EVENT_*]:::removed end subgraph After C2[consumer]:::ctx --> E2[agent_sdk/events.py<br/>declares EVENT_*]:::added T2[acp/types.py<br/>re-exports]:::changed --> E2 end classDef added fill:#DCFCE7,stroke:#16A34A,color:#14532D,stroke-width:2px classDef changed fill:#FEF3C7,stroke:#D97706,color:#78350F,stroke-width:2px classDef removed fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D,stroke-dasharray:4 3 classDef ctx fill:#E0F2FE,stroke:#0284C7,color:#0C4A6E linkStyle 0 stroke:#DC2626,stroke-dasharray:4 3 linkStyle 1,2 stroke:#16A34A,stroke-width:2px🟩 added · 🟨 changed · 🟥 removed · 🟦 unchanged
A consumer can now read a kind from the SDK;
acp/types.pyreads the same objects so existing importers do not change.Tests
test/test_agent_sdk_events.pyis new and locks in four things.The shim is the same object. Every
EVENT_*andSTOP_REASON_*name reachable askiro_crew.acp.types.Xis asserted equal to and identical withkiro_crew.agent_sdk.events.X, name by name.There is one declaration. An AST pass asserts
acp/types.pyassigns noEVENT_*orSTOP_REASON_*name of its own, withSTOP_REASON_CONTENT_FILTERED_WIREas the single recorded exception. This is the assertion that actually catches drift: CPython interns short identifier-like constants, so a re-declared"cancelled"would satisfy theischeck above.The direction holds. One test reads the new module's own imports and asserts it names neither
kiro_crew.acpnorkiro_crew.providers; another imports it first in a fresh interpreter and asserts no backend module loads, which catches an edge added lazily inside a function. A third asserts no nameagent_sdkexports is defined in a backend package -- theLLMEvent = AcpEventchannel the import gate cannot see, because it exemptsagent_sdk/wholly.ALL_EVENT_KINDSmatches the constants, no two kinds share a string, and everykindin the committed replay snapshots is a kind the SDK now defines.All five gate assertions are mutation-verified: re-declaring a constant in
acp/types.py, adding a backend import to the new module, dropping a kind from the frozenset, dropping a name from the re-export, and renaming the wire reason each turn their test red.Manual verification
N/A -- unit coverage sufficient. The change moves constants and adds no runtime path, and
test/test_acp_frame_replay.pyproves the observable output is unchanged: every snapshot undertest/fixtures/acp_frames/is byte-identical, so the dispatch layer emits exactly the stream it did before.Related Issues
no linked issue: this is the first slice of the RFC's PR 2, tracked in
docs/request-for-change/rfc-crew-agent-sdk-boundary.mdrather than in an issue.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)