fix(stop-hook): honour conversational turns on transcript-free harnesses - #687
Conversation
50f3be3 to
90a8eee
Compare
Review @ 90a8eeeVerified against the head commit in a clean worktree: t121 60/60, t68 + t147 + t165 + t171 59/59, t122 + t195 15/15, the coverage-registry guard 33/33, and That said, I found one test-efficacy hole (proved by mutation), one real enforcement gap, two contract problems, and a mechanical blocker. Requesting changes on findings 1-4 plus the rebase. 1. P1 (test-efficacy): the "(f2) THE PROBE MUST NOT SELF-SATISFY" case is vacuous - it passes with the fix deletedMutation check: I removed The reason: t121 runs the hook against the mock engine (t121:173), and the mock never calls Fix options: (a) have the mock engine write a witness file recording 2. P1: the marker predicate is blind to
|
53d5b80 to
9a752e9
Compare
Addressed @ 9a752e9 (rebased onto v2 2.5.30, re-bumped to 2.5.32)Thank you for the mutation check on finding 1 — that one landed hard. It invalidated the exact claim I had been making in the PR body, the commit message, and to my collaborator: that the probe assertion was what kept the fix from being silently dead. It was the opposite. I reproduced your result before fixing anything (removed Findings 1-4 addressed, 5 documented, all three nits done, and the rebase is in. I took your option (a) on finding 1 and your scoping option on finding 2 — reasoning below. 1. P1 test-efficacy — fixed, and re-verified by mutation in both directionsYour diagnosis was exact: t121 drives the mock engine, the mock never calls Took option (a), both halves:
Mutation re-run with the marking deleted: 2. P1 jump/bolt/swarm/state blindness — scoped, not closed, deliberatelyConfirmed your finding first: We chose your second option — narrow the claims — rather than touching the marker from a shared seam. Reasoning: So every parity claim is gone, replaced by an explicit gap statement in four places:
3. P2 persistent fail-open on a failed engine-marker write — fixed, plus a second hole your case exposedTook your fix: Writing the test for it surfaced something adjacent that you did not report: 4. P2 Kiro IDE stop contract — reconciled, and the PR's headline claim was wrongYou were right, and the consequence is bigger than a stale comment: the reproduction in the PR body did not happen on Kiro IDE. I probed the current build — a temporary
That matches Kiro's own docs: So what the fix delivers is host-dependent, and the PR now says so instead of implying uniform benefit:
The Kiro CLI adapter relays stdout and exit status verbatim; its live host behavior is measured in the follow-up below rather than inferred from that relay. I did not test 5. P3 intent-global markers — acknowledged in proseAdded a 6. Blocker rebase — done, and 2.5.31 was indeed contestedRebased onto Nits
Validation
Follow-up: live Kiro CLI Stop verificationThe earlier Kiro CLI non-measurement is now closed for Kiro CLI 2.16.0 in both executable paths. In the legacy/V2 interactive path, one user turn produced the initial response and then the reason-driven continuation, with the hook called twice. In Scope remains host-specific: this verifies Kiro CLI 2.16.0 and Kiro CLI |
v2 shipped 2.5.30 while this branch still declared 2.5.27. Scanned the open PRs by their actual AIDLC_VERSION diff rather than their titles: 2.5.31 (awslabs#535, awslabs#661, awslabs#686), 2.5.32 (awslabs#660, awslabs#687) and 2.5.33 (awslabs#689) are claimed, so this takes 2.5.34. The CHANGELOG entry was rebuilt from v2's file with this branch's block reinserted, so no upstream heading is lost. Its sensor-cache bullet now describes the engine-path match rather than the leaf-name one, and a new bullet covers the clean-filter binding. Coverage registry regenerated with the tool, not hand-edited.
Round 2 @ 9a752e9All six round-1 items are addressed, and the fixes verify. Re-checked in a clean worktree at the new head: t121 61/61, the new t259 13/13, t68 + t147 + t165 + t171 + t195 green, The mutation re-check on finding 1 now bites in both directions: with That said, applying the same mutation discipline to the new code found one more hole, plus a leftover comment sweep. Requesting these before approval; both are small. 1. P2 (test-efficacy): the t259 stale-marker cleanup case passes with the cleanup deletedMutation check: I removed the entire The reason: the case simulates the failed write with a directory at the marker path ( One-line fix: after the failing 2. P3: the passthrough-dispatch comment still claims the block contract "is shared", in both adaptersThe big contract comments are now right, but the one-line comment on the stop passthrough branch survived in both files and contradicts the corrected text a screen above it:
Both lines pre-date this PR (they are on the d0cd10a base), so ordinarily I would not block on them, but finding 4 was "reconcile the contradiction" and a grep for the block contract lands on these lines first. A one-line sweep in each adapter finishes the job; dist regenerates with it. Nit (non-blocking): the Kiro CLI row now undersells your own follow-upThe PR body table and the CHANGELOG bullet still say Kiro CLI is unverified and to treat it like the IDE until measured, but your follow-up comment measures exactly that on 2.16.0, both paths, consumption confirmed. Stale-conservative rather than wrong; your call whether to touch those two lines while you are in there. With the t259 assertion added and the two comment lines swept, this is an approve from me. |
Round 2 addressed @
|
14c666d to
2d384b5
Compare
Re-review @
|
v2 shipped 2.5.30 while this branch still declared 2.5.27. Scanned the open PRs by their actual AIDLC_VERSION diff rather than their titles: 2.5.31 (awslabs#535, awslabs#661, awslabs#686), 2.5.32 (awslabs#660, awslabs#687) and 2.5.33 (awslabs#689) are claimed, so this takes 2.5.34. The CHANGELOG entry was rebuilt from v2's file with this branch's block reinserted, so no upstream heading is lost. Its sensor-cache bullet now describes the engine-path match rather than the leaf-name one, and a new bullet covers the clean-filter binding. Coverage registry regenerated with the tool, not hand-edited.
|
Thanks, Camilo. The marker fallback addresses a real gap on transcript-free harnesses, and the round-two fixes and mutation coverage look sound. Current The branch has drifted again, but please do not spend time rebasing or renumbering it. Maintainers can integrate the authored changes onto current One scope correction needs to be preserved during that integration: the Kiro With those integration points handled, this is work we should carry forward. |
|
Thank you, @apackeer, for the thorough review, the mutation checks, and for following this through across the repeated base churn. I appreciate the confirmation that the marker fallback and the round-two fixes are technically sound. I’ll leave the branch as-is per your guidance and won’t spend more cycles rebasing or renumbering it. During maintainer integration, the Kiro Thanks for carrying this work forward. |
|
@leandrodamascena can you take another pass? LGTM but if it does to you can you approve/merge? |
apackeer
left a comment
apackeer
left a comment
There was a problem hiding this comment.
Approved. The marker fallback and review fixes are sound. Maintainers can carry the authored changes onto current v2 during integration, preserving the documented Kiro v3/KAS distinction and coverage limitations.
The Stop hook's tier-3 conversational carve-out was inert on Kiro IDE, Kiro CLI
and opencode. It answers "was the human's last prompt answered with zero engine
calls?" by reading the harness transcript, and only Claude Code and Codex
deliver `transcript_path`. On the other three the predicate always returned
false, so every purely conversational turn mid-stage fell through to the
cap-bounded block and was counted as no-progress.
Give the same predicate a second evidence source: two mtime markers the
framework writes on seams that already exist.
.aidlc-human-turn touched by the UserPromptSubmit mint, beside the
HUMAN_TURN ledger event it already appends
.aidlc-engine-touch touched by every advancing `next` / `report` / `park`
conversational <=> mtime(.aidlc-human-turn) > mtime(.aidlc-engine-touch)
Markers rather than the audit ledger because `next` is read-only and emits no
audit event: a ledger-only predicate would be blind to a conductor that
consulted the engine and then bailed mid-loop, which is the precise failure the
forwarding loop exists to catch.
The load-bearing subtlety is that the Stop hook consults the engine itself. An
unmarked probe would refresh the engine marker on every stop, leaving the
predicate false forever and the carve-out dead code that looks implemented. The
hook sets AIDLC_STOP_HOOK_PROBE=1 on its spawn and markEngineTouch() no-ops on
it. Both halves are pinned: t121 asserts via an env witness written by the mock
engine what the hook actually handed the child, and t259 asserts in-process that
markEngineTouch honours the mark. An earlier revision asserted only marker
mtimes in t121, which was vacuous — the mock never calls markEngineTouch, so
that assertion passed with the marking deleted.
WHAT THIS CHANGES IS HOST-DEPENDENT, and not uniform. On Claude Code, Codex and
opencode the nudge is suppressed and the turn ends clean (opencode's plugin
parses the block itself and re-prompts with the reason). On Kiro IDE nothing
user-visible changes: that host's Stop trigger is observational — probed live,
neither stdout nor stderr reaches the agent, and the Stop payload carries only
{session_id, hook_event_name, cwd} — so the nudge was never delivered there and
only stop.drops and the counter are corrected. aidlc-stop.json always said as
much; the adapter comment claiming parity with Claude's block contract was
wrong and is corrected. On Kiro CLI the host's behaviour is unmeasured and the
comment now says so rather than assuming parity.
KNOWN GAP, documented rather than closed: the marker path is blind to
aidlc-jump / aidlc-bolt / aidlc-swarm and the mutating aidlc-state verbs, which
the transcript predicate does count as engagement, because none of those tools
touch the engine marker. A conductor that jumps the pointer and then quits is
released here and blocked on Claude. Those turns were nudged before the marker
path existed, so this is a real if narrow relaxation; closing it means touching
the marker from a seam all four tools cross, which widens the blast radius well
past this carve-out.
Fail-closed throughout: a missing or non-file marker reads as "no evidence", the
autonomy guard still suppresses the carve-out under autonomous Construction, a
delivered transcript still wins, and a marker whose write fails is DELETED
rather than left stale — a stale engine marker would be a persistent silent
fail-open, since the human marker keeps advancing past it. markHumanTurn and
markEngineTouch self-gate on a born workflow, keeping `next` a pure read that
scaffolds nothing.
2d384b5 to
30b1ae8
Compare
2 participants
Summary
The Stop hook's tier-3 conversational carve-out was inert on Kiro IDE, Kiro CLI and opencode. It answers "was the human's last prompt answered with zero engine calls?" by reading the harness transcript, and only Claude Code and Codex deliver
transcript_path. On the other three the predicate always returnedfalse, so every purely conversational turn mid-stage — asking why an earlier decision was made, reading code without advancing the workflow — fell through to the cap-bounded block and was counted as no-progress.This gives the same predicate a second evidence source so it reaches a verdict without a transcript. What that is worth to a user depends on the host, and it is not uniform — see User experience. Full diagnosis in #688.
Revised after review (#687 review @
90a8eee): the probe test was vacuous, the parity claim was too broad, a failed marker write could fail open, and the original reproduction claim did not hold on Kiro IDE. Per-finding response in the comments.Round 2 addressed at
2d384b54: the stale-path test now directly asserts physical cleanup, both passthrough comments are contract-accurate, and the Kiro CLI 2.16.0 evidence is reflected throughout this body, the CHANGELOG, and the reference docs.Changes
One predicate, two evidence sources. Where
transcript_pathis delivered the transcript stays authoritative — including when it disagrees with the new signal, which is pinned. Where it is not, the hook compares two mtimes:The two seams already existed.
.aidlc-human-turnis touched by theUserPromptSubmitmint, beside theHUMAN_TURNledger event it already appends — the coreaidlc-mint-presence.tshook (Claude, opencode) and both Kiro adapters' inlinedminttargets..aidlc-engine-touchis touched by every advancingaidlc-orchestrate next/report/park. Read-only routing (--status,--doctor,--help,--version, the workspace verbs) is excluded, so "what stage am I on?" stays a chat turn.Markers rather than the audit ledger. A ledger-only predicate looks cheaper — one file, no engine coordination — but
nextis read-only and emits no audit event, so it would be blind to a conductor that consulted the engine and then bailed mid-loop. That is the failure the forwarding loop exists to catch, so a ledger reading would have weakened enforcement to fix an annoyance.The load-bearing subtlety. The Stop hook consults the engine itself, on every stop. An unmarked probe would refresh the engine marker each time, leaving the predicate false forever and the carve-out dead code that looks implemented. The hook sets
AIDLC_STOP_HOOK_PROBE=1on its spawn andmarkEngineTouch()no-ops on it. Both halves are now pinned — see Test plan, because the first attempt at pinning this was vacuous and review caught it by mutation.Known gap — the marker path is more permissive than the transcript path.
isEngineToolCallcounts as engagement any non-read-onlyaidlc-jump/aidlc-bolt/aidlc-swarmcall and the mutatingaidlc-stateverbs. None of those tools touch the engine marker — its only writers areaidlc-orchestrate's three subcommands. So a conductor that runsaidlc-jump(mutating the stage pointer, emitting audit) and then ends its turn without consulting the engine is released on the transcript-free harnesses where the same turn blocks on Claude Code. Those turns were nudged before this change, so it is a real if narrow relaxation. Closing it means touching the marker from a seam all four tools cross (the audit path, orwriteStateFile), which widens the blast radius well past this carve-out — so it is documented, in the code and indocs/reference/06-hooks-and-tools.md, rather than closed. Happy to do it as a follow-up PR if preferred.Fail-closed throughout. A missing or non-regular-file marker reads as "no evidence" and falls through to the cap-bounded block. The autonomy guard still suppresses the carve-out under
Construction Autonomy Mode: autonomous. A marker whose write fails is deleted rather than left stale: a stale engine marker would be a persistent silent fail-open, since the human marker keeps advancing past it.markHumanTurn/markEngineTouchself-gate on a born workflow, keepingnexta pure read that scaffolds nothing (pinned by t165/t171).Session scope. Both markers are per-intent with no session key, where the transcript predicate was inherently per-session. Two concurrent sessions on one intent can cross-talk. Narrow, and the failure mode is a released stop rather than a wrong transition, so it is accepted and documented;
session_idis available if it is ever worth closing.User experience
The
{decision: block}contract is Claude Code's. Each other host consumes it, or not, on its own terms — so the visible effect differs, and an earlier revision of this PR wrongly implied it was uniform.aidlc-opencode-adapter.ts:502-527)stop.dropsand the no-progress counter are correctedreasonis reinjected, andStopfires again after the induced continuation (two invocations total)--v3/KAS.kiro/hooksregistrationreasonis reinjected;Stopfires once and does not re-fire after the induced continuationOn Kiro IDE the
Stoptrigger is observational. Probed live on the current build with a temporary hook emitting unique sentinels: the command ran (witness written), and neither its stdout nor its stderr reached the agent's context. TheStoppayload is{session_id, hook_event_name, cwd}— three fields, no transcript, no turn id. That matches Kiro's docs (Stopsits outside the blockable set; stdout is forwarded only forSessionStart/UserPromptSubmit; no{"decision":"block"}contract exists for any trigger).So the reproduction this PR originally led with was wrong. The nudge was never delivered on Kiro IDE — the hook produced it and the host discarded it. The block counter on disk, which is what #688 reported, is written before the decision is emitted, so it was evidence the hook ran, not that the human saw anything.
aidlc-stop.jsonand the kiro-ide guide always said the trigger was advisory; the adapter comment claiming parity with Claude's contract was wrong before this PR and I made it worse by stating it explicitly. Both are corrected, with the probe evidence recorded in the comment.Checklist
Test Plan
Coverage
t121(integration, drives the real hook) gained eight(f2)cases plus aseedTurnMarkershelper;t259(new, unit, in-process) pins the lib half.(f2) THE PROBE IS MARKEDAIDLC_STOP_HOOK_PROBE=1to its own consultationprocess.envt259× 13markEngineTouchno-ops under the env var, writes without it, does not refresh an existing marker under the probe, only literal"1"suppresses; the predicate both directions; three fail-closed cases; the birth self-gate; and a failed write physically removing the stale path (plus the fail-closed predicate check)Why two files. Review proved by mutation that the original single assertion was vacuous:
t121runs against a mock engine that never callsmarkEngineTouch, so.aidlc-engine-touchis unrefreshable there and an mtime-equality check passes even with the probe marking deleted from the hook. The env witness pins the hook half;t259pins the lib half in-process. Both are needed — the carve-out is dead code if either the hook forgets to mark its probe ormarkEngineTouchforgets to honour the mark.Mutation re-verified. With
[STOP_HOOK_PROBE_ENV]: "1"removed from the spawn,(f2) THE PROBE IS MARKEDfails:Expected: "1" Received: null. Restored: green.Validation
Run on the rebased branch (
2.5.33on top ofv2at 2.5.32 /57ae3d05).bun run check(dist byte-parity across five harnesses, three tsconfigs, biome over 545 files): cleanbun tests/gen-coverage-registry.ts --check,bun scripts/ci-changelog-guard.ts upstream/v2: cleant121(61),t259(13),t195,t205,t27,t131,t228,t241,t245,t01,t02, plust68,t147,t165,t171.t122is e2e (--releaseonly) and is not claimed hereThe three failures are pre-existing
Verified three independent ways: by stashing this work and re-running in place, by a
git worktreeat bareupstream/v2, and by the sibling branch #689 producing the identical three.t66claim-sourcessensor —Stage "intent-capture" imports unknown sensor id "claim-sources"; the golden export fixture was not updatedt89claim-sourcesroot cause; the sensor-resolution fixtures were not updatedt19Matches what #615 reported for
t66/t89.Live host verification and remaining limits
Kiro CLI 2.16.0 was verified live on both executable paths. Legacy/V2 consumed the adapter-relayed block, reinjected
reason, and invokedStoptwice across the induced continuation.--v3/KAS consumed the same shape through standalone.kiro/hooks, reinjectedreason, and invokedStoponce without re-firing after the induced continuation. This is CLI-only evidence; it does not revise the separate Kiro IDE probe, where Stop-hook stdout and stderr were discarded.Not verified live here: I have not watched this exact conversational carve-out end cleanly in a live opencode session. Its plugin-owned re-prompt path is exercised in code/tests, but the remaining live-host claim is intentionally limited to Kiro CLI 2.16.0.
stop_hook_activeis still hardcodedfalseon Kiro, deliberately. There is no honest way to derive it: reading the block counter is circular (it isprior, and the seeding branch only matters whenprior === null), a block emits no audit event, and a sibling marker would live in the same directory and fail for the same reasons. Guessingtruewould be worse — it would seed at 2 on a legitimately fresh first block, weakening the guard in the common case to fix a rare one. Residual cost: at most one extra counted block in the narrow window where the counter file is missing mid-sequence.The jump/bolt/swarm/state gap is open by choice, not closed. See Changes.
Impact
Bounded, and narrower than the first revision of this PR claimed. On hosts that act on the block it removes one spurious nudge per conversational detour; the cap already limited the damage, and no state was corrupted, no stages skipped, no artifacts lost. On Kiro IDE — the harness where the bug was reported — it corrects the ledger and the counter and nothing else.
The stronger argument is second-order: the nudge is the only mechanism that catches a conductor that genuinely abandoned the loop, and a warning that fires every time a human asks a question mid-stage teaches users to ignore it.
Lowering
CLAUDE_CODE_STOP_HOOK_BLOCK_CAPis not an alternative:nextCountstarts at 1 againstif (nextCount >= cap) return false, socap=1disables forwarding-loop enforcement entirely rather than softening it.Upgrade
mkdir -p your-project/.kiro && cp -R dist/kiro-ide/.kiro/. your-project/.kiro/Version
Bumps to 2.5.33, rebased onto
v2at 2.5.32 (57ae3d05). Upstream now owns 2.5.31 and 2.5.32; this PR takes the next free patch and #689 takes 2.5.34, so the two remain independently mergeable without duplicate headings. The final rebase was force-updated with an explicit lease after local validation.References
Fixestrailer on a PR targetingv2does not auto-close--doctordrop-log accuracy fix, originally the second commit here. Split out for reviewability; no code dependency — disjoint regions of the adaptertoolSuccessfailed-write guard preserved throughout is [Bug]: AI-DLC v2 audit/sensor hooks never fire in Kiro IDE — empty tool_input (payload delivered via USER_PROMPT env, not stdin) #417claim-sourcesfixture breakage int66/t89traces to fix: ground Intent Capture claims (2.5.10) #647Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.