Skip to content

refactor: move event and stop-reason constants into agent_sdk - #9359

Closed
iamwhatever wants to merge 1 commit into
mainfrom
refactor/agent-sdk-event-constants
Closed

refactor: move event and stop-reason constants into agent_sdk#9359
iamwhatever wants to merge 1 commit into
mainfrom
refactor/agent-sdk-event-constants

Conversation

@iamwhatever

Copy link
Copy Markdown
Collaborator

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's kind, a turn's stop_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.py declares them: the 19 EVENT_* kinds, a frozenset ALL_EVENT_KINDS over them, and the six provider-neutral STOP_REASON_* reasons. acp/types.py imports the same objects back and re-exports every name. from kiro_crew.acp.types import EVENT_TOOL_CALL still works, so no consumer file changed. agent_sdk/__init__.py exports the names too, and its two TURN_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_WIRE is a harness's own spelling of a refusal. The parser normalises it to STOP_REASON_REFUSAL before 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
Loading

🟩 added · 🟨 changed · 🟥 removed · 🟦 unchanged

A consumer can now read a kind from the SDK; acp/types.py reads the same objects so existing importers do not change.

Tests

test/test_agent_sdk_events.py is new and locks in four things.

The shim is the same object. Every EVENT_* and STOP_REASON_* name reachable as kiro_crew.acp.types.X is asserted equal to and identical with kiro_crew.agent_sdk.events.X, name by name.

There is one declaration. An AST pass asserts acp/types.py assigns no EVENT_* or STOP_REASON_* name of its own, with STOP_REASON_CONTENT_FILTERED_WIRE as 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 the is check above.

The direction holds. One test reads the new module's own imports and asserts it names neither kiro_crew.acp nor kiro_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 name agent_sdk exports is defined in a backend package -- the LLMEvent = AcpEvent channel the import gate cannot see, because it exempts agent_sdk/ wholly.

ALL_EVENT_KINDS matches the constants, no two kinds share a string, and every kind in 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.py proves the observable output is unchanged: every snapshot under test/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.md rather than in an 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

@iamwhatever
iamwhatever requested a review from a team as a code owner September 8, 2026 03:11
@iamwhatever
iamwhatever requested a review from patrigao September 8, 2026 03:11
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Intent: Move the event-kind and stop-reason string vocabulary out of the ACP backend layer and into kiro_crew.agent_sdk.events, so application code can read a kind or a stop reason without naming the backend package. acp/types.py re-exports every name, so this is a pure relocation with zero behaviour change and zero consumer edits.

Not a goal: Paying off any boundary-baseline edge. providers/base.py still re-exports EVENT_* from acp/types.py, and that edge is PR 5's wave. Also not a goal: the rest of RFC PR 2 (the AgentEvent object, ApprovalToken, ToolStatus, CompactionResult, SessionCapabilities, the error taxonomy, the driver translation, and the deprecation-shim mechanism they need). No consumer file may change in this PR; if one has to, the re-export set is incomplete and the fix is the re-export, not the consumer.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3c6de2a3a272d3dda9ce19661c66d14926a4ec9b and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3c6de2a

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

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 — kiro_crew.acp.types now executes agent_sdk/__init__.py on import, making the SDK package's whole top-level import chain load-bearing for the ACP layer — is already fenced by the fresh-interpreter test, which fails on any future top-level acp/providers edge anywhere in that chain, so it does not rise to a Watch item.

[DESIGN-REVIEWED] 3c6de2a

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3c6de2a3a272d3dda9ce19661c66d14926a4ec9b — 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 checks complete. The move itself is derived from a repo-recorded RFC (docs/request-for-change/rfc-crew-agent-sdk-boundary.md, PR 2, with the gate scripts/check_agent_sdk_boundary.py already in place) and the vocabulary is consumed by 32 files in src/. The one item that doesn't earn its place today is ALL_EVENT_KINDS: my grep found zero consumers outside the defining module, the two re-export sites, tests, and docs — and its own docstring justifies it in the future tense.

First-Principles-Verdict: CONCERNS

ALL_EVENT_KINDS ships with zero src/ consumers — its only stated user is a dispatcher that "can" exist, which is speculative surface on a move PR.

Not justified as shipped

  • Item 3 — zero consumers: grep ALL_EVENT_KINDS across the repo hits only agent_sdk/events.py (defining), the two re-export sites, test_agent_sdk_events.py, and the RFC. The base RFC's PR 2 never asked for it; the paragraph prescribing it was added by this same PR.

What this change ships

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

  1. Event kinds and stop reasons are now importable from kiro_crew.agent_sdk — justified
  2. Their declaration moved out of acp/types.py; every existing import unchanged via re-export — justified
  3. New public frozenset ALL_EVENT_KINDS — zero consumers, justified only by a hypothetical future dispatcher
  4. TURN_STOP_REASON_* became aliases instead of duplicate literals — justified
  5. STOP_REASON_CONTENT_FILTERED_WIRE deliberately stays behind in the ACP layer — justified
  6. New gate tests pin single-declaration, direction, and corpus agreement — justified
  7. RFC section records PR 2a as landed, same commit — justified

Watch

ALL_EVENT_KINDS is exported from three paths (agent_sdk.events, agent_sdk, acp.types) with no runtime reader; the corpus test is its only user, and test_all_event_kinds_covers_every_event_constant already derives the identical set from the EVENT_* names.
Clears when: a src/ consumer asserts against it, or it is deleted and the corpus test uses the derived set.

Subtractions

  • Delete ALL_EVENT_KINDS (and its three export lines): test_the_replay_corpus_emits_only_sdk_event_kinds can use the set the sibling test already builds from EVENT_* constants (0 consumers; grep pattern ALL_EVENT_KINDS, 12 hits, all defining/re-export/test/doc).

[FIRST-PRINCIPLES-REVIEWED] 3c6de2a

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3c6de2a3a272d3dda9ce19661c66d14926a4ec9b — this comment is updated in place on each push.

Review details

The change is a clean re-export relocation. Verified independently:

  • agent_sdk/events.py imports nothing from acp/providers (only __future__).
  • The import chain acp.types → agent_sdk.events runs agent_sdk/__init__native_commandsdrivers.acp, and every kiro_crew.acp import in drivers/acp.py is function-local (grep confirmed indentation), so no reentry into the partially-initialized acp.types — no cycle.
  • Re-exports are the same objects; all 19 EVENT_*, ALL_EVENT_KINDS, and 6 STOP_REASON_* removed from acp/types.py are re-imported, with no shadowing local ALL_EVENT_KINDS left behind (single occurrence at the import line).

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 3c6de2a3a272d3dda9ce19661c66d14926a4ec9b.

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

CI round 1 — every red is inherited from main, none is in this diff

This diff touches five files: src/kiro_crew/agent_sdk/events.py (new), src/kiro_crew/acp/types.py, src/kiro_crew/agent_sdk/__init__.py, test/test_agent_sdk_events.py, and the RFC. None of the four failures reaches any of them.

1. test_security_conductor_skill_contract.py::TestSkillIsInstallable::test_no_bundled_scripts_are_shipped_here — red on Backend Tests (3.12, 3) and Backend Tests (Windows) (3).

main is red on this test independently of this PR. The CI runs for fd6684b40 (this PR's merge base) and 2f9ed9724 both concluded failure. The test asserts src/kiro_crew/builtin_skills/security-conductor/scripts does not exist; that directory is present on origin/main today, added by #9270. This PR changes no file under src/kiro_crew/builtin_skills/, so the merge ref's content there is exactly main's.

Two open PRs already own the fix: #9369 (unbreak the security-conductor script contract on main) and #9362 (security-conductor scripts guard checks for stubs, not absence). Fixing it here would mean editing src/kiro_crew/builtin_skills/, which is outside this PR's scope. It clears on the next rebase after either lands.

2. test_work_ledger.py::test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding — red on Backend Tests (Windows) (4).

A Windows concurrency flake: the assertion wanted 3 threads reporting already_bound and got 2, with one thread reporting invalid_value. That is a race in the test's own thread coordination, not a code path this diff touches. Re-run requested for that job alone.

3. Coverage Gate — cascades from the two failed shards; clears when they do.

4. Backend Lint & Type Check (3.12)cancelled, not failed: fail-fast killed it when a sibling shard went red. It carries no verdict on this diff, and a cancelled job cannot be re-run on its own.

Local verification on this exact head (ac2ac7e47029): mypy clean on 1333 files; isort, black, flake8 clean on the changed files; all 32 structural gates green including the agent-SDK boundary gate; test_acp_frame_replay.py, test_agent_lifecycle_cycle.py and test_agent_sdk_boundary.py green; both local review lanes (gpt-5.6-sol, claude-opus-4.8) PASS with zero findings.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 8, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

CI round 2 — a second pre-existing failure, still nothing in this diff

Backend Tests (Windows) (4) was re-run on its own and failed again, on the same test. That makes it two for two, so it is not a flake to re-run a third time. It is also not this PR's.

test_work_ledger.py::test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding

Both attempts got invalid_value from one racing thread where the test wants already_bound:

attempt 1: chat-0-c=already_bound; chat-1-c=invalid_value; chat-2-c=bound;          chat-3-c=already_bound
attempt 2: chat-0-c=bound;         chat-1-c=invalid_value; chat-2-c=already_bound;  chat-3-c=already_bound

invalid_value is work_ledger.CODE_INVALID_VALUE — a real WorkLedgerError from apply_conductor_action, raised under Windows contention instead of the clean already-bound refusal. The test's own assertion message names this failure class ("a bare OSError from a Windows sharing violation, or a thread that never finished"), and its git history is a chain of attempts at it: #9257 name the cause when the binding race test shortfalls, and before that #9237 stop the lock-file open from truncating on Windows. So this is a known open defect in the work-ledger Windows locking path, owned by whoever is carrying that thread.

It cannot be reached from this diff. test_work_ledger.py and src/kiro_crew/work_ledger.py import nothing from kiro_crew.acp or kiro_crew.agent_sdk. work_ledger has its own unrelated EVENT_KINDS frozenset, which is a different vocabulary from the EVENT_* constants this PR moved. And test/test_agent_sdk_events.py is not on Windows shard 4 at all — zero mentions in that job's log — so not even a shard-load coupling exists.

Both remaining blockers are pre-existing and belong to other PRs:

Red Cause Owner
test_security_conductor_skill_contract on Backend Tests (3.12, 3) and (Windows) (3) builtin_skills/security-conductor/scripts exists on main and the contract test forbids it; main's own CI is red on it (fd6684b40, 2f9ed9724) #9369, #9362
test_work_ledger binding race on (Windows) (4) invalid_value under Windows contention in apply_conductor_action the #9257 / #9237 thread
Coverage Gate cascades from the two above
Backend Lint & Type Check (3.12) cancelled by fail-fast, never ran to a verdict; not individually re-runnable

This PR's own state on ac2ac7e47029 is unchanged and green everywhere it is measured: mypy clean on 1333 files, isort / black / flake8 clean, all 32 structural gates including the agent-SDK boundary gate, test_acp_frame_replay.py snapshots byte-identical, test_agent_lifecycle_cycle.py passing, and both AI review lanes plus Design / First Principles / UX at PASS with zero findings.

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.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Red attribution on ac2ac7e47029, test by test

Exact failed-test sets, read from each job's own short test summary info. No red on this head is in this PR's diff.

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.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
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.
@iamwhatever
iamwhatever force-pushed the refactor/agent-sdk-event-constants branch from ac2ac7e to 3c6de2a Compare September 8, 2026 05:52
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 8, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

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.

@iamwhatever iamwhatever closed this Sep 8, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 8, 2026
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.

1 participant