fix(kiro-ide): stop reporting a failed write as harness decay - #689
Conversation
apackeer
left a comment
There was a problem hiding this comment.
Reviewed at head 866ae5c. The change itself is correct and well-scoped: the classifier runs only after canonicalWriteTool matched and extractWrittenPath failed, so it can only suppress false-positive drops, never audit a failed write; the 0.12 toolSuccess === false guard still returns earlier; fail-open behaviour is unchanged; dist is byte-identical to the authored file; t218 passes 45/45 on the head and the behaviour verified correctly in an independent runtime probe. I'd approve the logic as-is, but there are two blockers and two smaller asks.
P1: rebase and re-bump - the PR is CONFLICTING and its version slot is stale
The PR bumps to 2.5.28 "leaving 2.5.27 to #687", but v2 has moved: #675 merged as 2.5.30 (d0cd10a), and GitHub now reports this branch as CONFLICTING. It needs a rebase onto current v2 and a re-bump past 2.5.30. Heads-up: 2.5.31 is already claimed by #686, so per the changelog policy's conflict-trap rule, whichever lands second re-bumps again.
P2: docs/reference/kiro-ide-hook-payload.md is now stale in two places
Per the repo Documentation Policy (stale references updated in the same commit):
- Line ~60: "error prose that matches no known pattern records a visible hook-drop" - after this change, recognised failure prose records a debug line, not a drop.
- Line ~74: "A write-class tool whose wording does not match a known pattern records a visible hook-drop (never a silent no-op)" - no longer unconditionally true.
The PR body says "no output-format change", but this doc describes exactly the drop-log behaviour that changed.
P2: no pinning test for the new branch, and the validation cites the wrong suite
Nothing in the test suite pins the new classify-before-drop behaviour. t218 covers the neighbouring paths exhaustively (F4c: unrecognised wording still drops; T1: toolSuccess=false drops), so a regression here - reordering the guard, tightening a regex - would pass the suite silently. Please add t218 cases that:
- feed the verbatim
Caught an error while replacing string ...prose throughaudit-and-sensorson the 1.x stdin channel and assert the drops file does not exist; - keep an unknown success wording asserting a visible drop still lands (F4c covers this today - worth a paired case beside the new one so the contrast is pinned in one place).
Related: the PR body cites t147 as validation, but t147 scaffolds dist/kiro/.kiro - the Kiro CLI adapter (tests/unit/t147-kiro-hook-adapter.test.ts:48); its only touch of the IDE adapter is a static respawn-source check (test 15). The suite that actually drives the changed branch is t218. Worth correcting so future readers reach for the right suite.
P3: three of the four classifier patterns are ungrounded
The comment on isFailedWriteResult says "the patterns Kiro actually emits", but only ^Caught an error while is evidenced (the live capture in the PR body). ^Error:, ^Failed to , and ^An error occurred appear in no capture in the repo and not in docs/reference/kiro-ide-hook-payload.md. Risk direction is mild - a match only suppresses a drop when path extraction already failed, so masking real decay would need a new success wording that starts with error prose - but this repo grounds such claims (2.5.10 did exactly this exercise for Intent Capture). Please either cite captures for the other three or soften the comment to mark them as defensive guesses.
Verified along the way
- Success wordings ("Created the ...", "Replaced text in ...", "Appended the text to ...") cannot collide with the four failure prefixes, so no successful write can be misclassified as failed today.
hookDebugis opt-in and never throws, so the new debug call costs nothing on normal runs.- t218 T1-T4 still pass: the 0.12 explicit-false channel is unaffected.
9a88f52 to
53ced04
Compare
Addressed @ 53ced04All four addressed. Two notes up front, both on me: I missed this review for a full day. I had been checking The P3 finding is the one I want to flag as the real catch. You are right that I wrote "the patterns Kiro actually emits" while only one of the four had ever been observed. That is the same failure mode you caught on #687 — asserting more than the evidence supports — and it slipped past my own review of the same file. P1 — rebase and re-bump: doneRebased onto P2 —
|
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 @ 53ced04All four round-1 findings are addressed. I verified each against the diff rather than the reply, and a second independent pass (different model, minimal prompt, same head) cross-checked the branch; its findings are folded in below. The implementation itself remains correct. Three of the four new items are small; the first is not, so the CHANGES_REQUESTED stands until it is gone. Round-1 findings: all confirmed fixed
The Kiro CLI 2.16.0 Stop measurements are appreciated and correctly scoped as CLI-only evidence; they do not change anything here and I will pick them up on #687. New findingsN1 (P1, blocks): two 61.5 MB binaries committed at the repo root. N2 (P2): F4d/F4e do not drive the channel they claim to cover. Both call N3 (P2): the CHANGELOG overclaim survived in one spot. The 2.5.33 entry's second bullet says the classifier matches "start-anchored patterns Kiro actually emits" while the very next bullet correctly says three of the four are defensive guesses. That is the same asserting-more-than-the-evidence mode you flagged in your own reply; the code comment got the grading treatment, this sentence did not. Suggest "start-anchored failure prefixes" or similar. N4 (P2, mild): stale false drops survive the upgrade. VerdictImplementation approved in substance; N1 must be fixed before merge and N2/N3 should land with it since both misstate what the change is backed by. N4 is a one-line doc ask. With those in, this is an approve from me. |
53ced04 to
64f0b10
Compare
Round 2 addressed @
|
64f0b10 to
94fdbb6
Compare
apackeer
left a comment
There was a problem hiding this comment.
Round 3 @ 94fdbb64
All four round-2 findings are fixed. I verified the rewritten one-commit ancestry/tree, the real 1.x stdin cases, the evidence wording, and the stale-drop upgrade guidance. The implementation remains correct. Two items remain, one blocking.
P1 (blocks): v2 advanced again and the PR is now conflicting
Current v2 is 6b264081 and owns 2.5.33 (core/tools/aidlc-version.ts:4, CHANGELOG.md:4). This head is still parented on 57ae3d05; GitHub reports mergeable: false, mergeable_state: dirty, and rebaseable: false. Rebase onto current v2, resolve the changelog/version/generated-tree conflicts, regenerate distributions, and rerun the checks.
2.5.34 is valid relative to the merged base, but it is no longer uncontested: #692 also currently carries 2.5.34, while #687 still carries the now-merged 2.5.33. Please update the PR body's version/order paragraph. Whichever user-visible PR lands after another claimant must re-bump under the conflict-trap policy.
P3: the reference says an opt-in debug line is always recorded
docs/reference/kiro-ide-hook-payload.md:62 says recognised failure prose "records a hookDebug line", and line 79 similarly says it is "recorded via hookDebug only". But hookDebug returns without writing unless debugging is enabled (core/tools/aidlc-lib.ts:4879-4897), and the Kiro IDE guide explicitly says no log is written by default (docs/guide/harnesses/kiro-ide.md:127-137). Please make both reference lines explicit, e.g. "is sent to hookDebug (written only when hook debugging is enabled)". The runtime behavior is fine; this is documentation accuracy.
Verification
STAMP: tests/logs/2026-08-01T21-10-31Z
TRACES: /tmp/aidlc-pr689-r3/tests/logs/2026-08-01T21-10-31Z/*.log (1 files)
SUMMARY: tests/logs/2026-08-01T21-10-31Z/summary.txt + failures.txt - Result: PASS; Failed files: 0
RESULT: unit . 1 pass/0 fail . reds: none . live vars set: none . invariant grep hits: 0 (path-excluded)
The focused t218 slice passed all 47 tests, including F4d/F4e through the actual snake_case stdin path. Authored/generated Kiro IDE adapter files are byte-identical. CI is green on this head, but it predates the latest base commits.
Round-2 confirmation
- N1: no
.bun-buildpath exists in the head tree;*.bun-buildis ignored at.gitignore:9. - N2: F4d/F4e now use
runIdeStdinplusctx1x(tests/unit/t218-kiro-ide-hook-adapter.test.ts:1042-1069). - N3: the CHANGELOG now distinguishes the one captured prefix from three defensive guesses (
CHANGELOG.md:9-10). - N4: upgrade guidance names the per-record drops file and warns users to preserve current failures (
CHANGELOG.md:6).
Substantive code approved. The rebase remains the merge blocker; the debug wording should be corrected in the same refresh.
Approval statusNot approving yet. The substantive implementation is sound and all round-2 findings are fixed, but the current head ( Approval needs only the final refresh already described in the review:
Once those land and the refreshed head verifies, this is an approve. |
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 implementation and the 1.x coverage are sound. The only content correction remaining is to clarify both @leandrodamascena, could you take a final review pass on the substantive behavior and this integration direction? |
|
Thank you, @apackeer, for the careful review and for confirming that the implementation and the real IDE 1.x coverage are sound. I also appreciate the pragmatic integration handoff given the continuing I’ll leave the branch as-is as requested. During maintainer integration, the two Thanks as well for requesting the final review pass and helping move this work toward integration. |
leandrodamascena
left a comment
There was a problem hiding this comment.
Round 4 @ 94fdbb64
The implementation remains correct and all targeted validation passes. Two items remain, one blocking.
P1 (blocks): rebase and re-bump
v2 is now at 2.5.37, while this branch remains based on 57ae3d05 and declares 2.5.34. GitHub reports the PR as CONFLICTING/DIRTY.
Please rebase onto current v2, bump past 2.5.37, regenerate distributions, and rerun checks. Conflict simulation shows only release bookkeeping files conflict; the adapter, tests, and reference merge cleanly.
P3: qualify the hookDebug wording
docs/reference/kiro-ide-hook-payload.md:62,79 says recognized failure prose records a hookDebug line. hookDebug writes only when hook debugging is enabled.
Please clarify both locations, for example: “sent to hookDebug and written only when hook debugging is enabled.”
Verification
t218: 47 passedt68: 7 passedbun scripts/package.ts --check: passed- Authored/generated Kiro IDE adapter parity: passed
git diff --check: passed
The functional change is approved; the rebase remains the merge blocker.
94fdbb6 to
2afbf17
Compare
|
Round 4 is addressed in
Validation on the rebased commit:
The PR body has also been refreshed with the Round 4 base, version, and validation details. @leandrodamascena, thank you for the precise follow-up review. |
leandrodamascena
left a comment
There was a problem hiding this comment.
Round 5 @ 2afbf171
No findings. The prior blockers are resolved: the branch is one commit above current v2 at c73ee984, GitHub reports it mergeable, 2.5.40 has no open-PR collision, and both hookDebug statements now describe its opt-in write behavior accurately.
The substantive patch is unchanged across the rebase. Verified locally:
t218: 47 passedt68: 7 passed- package parity: passed for all five harnesses
- changelog guard: passed (149 preserved, 1 new)
- coverage registry: passed
- authored/generated Kiro IDE adapter parity: passed
git diff --check: passed
bun run check reached typecheck locally but this worktree has no bun-types installation; the PR's current Contract checks (parity + typecheck + lint), smoke/unit suite, changelog check, and build are all green in CI. Approved.
|
@apackeer can you pls check this again and maybe approve/merge? Thanks |
2afbf17 to
910bb6e
Compare
apackeer
left a comment
There was a problem hiding this comment.
Approved at 910bb6e. The branch is rebased onto current v2, release metadata is synchronized at 2.5.43, explicit toolSuccess=true now outranks failure-prose inference, F4d/F4e/F4f cover the three classification paths, generated distributions are in parity, and the current CI contract, smoke/unit, changelog, and documentation checks are green.
The adapter's audit-and-sensors target recorded a visible hook drop whenever
it could not extract a file path from a write tool's result. Two distinct
cases reach that branch, and conflating them made the drop log useless as a
health signal:
(a) the write FAILED, so there is no artifact to audit and declining to
forward it is CORRECT behaviour, not decay;
(b) the write SUCCEEDED but its result wording matched no known pattern,
which is the invisible degradation this harness exists to surface.
Only (b) belongs in the drop log. The IDE 1.x stdin channel carries no success
flag, so the existing `toolSuccess === false` guard cannot catch (a) -- the
failure arrives only as error prose, e.g. a str_replace whose old string
matched multiple times. Every such failure was therefore logged as decay, and
`/aidlc --doctor` reported hook degradation on workspaces whose hooks were
working perfectly. That trains the reader to ignore the one channel that
matters when something genuinely breaks.
Classify before logging: recognised failure prose is recorded via hookDebug and
(b) keeps the visible drop. Every pattern is start-anchored, because a loose
"contains error" test would swallow a successful write to a file whose NAME
mentions an error. Anything unrecognised is still treated as a success, keeping
the default biased toward reporting rather than silence.
EVIDENCE GRADING, per review: of the four patterns only `^Caught an error while`
is grounded in a live capture. `^Error:`, `^Failed to ` and `^An error occurred`
are now marked in the code as defensive guesses rather than observed shapes. The
risk direction is one-way -- a match only suppresses a drop after path
extraction has already failed, and none of the known success wordings ("Created
the ...", "Replaced text in ...", "Appended the text to ...") can collide with
them -- so they are kept, but no longer described as things Kiro emits.
t218 gains a paired F4d/F4e: the verbatim captured failure prose through the 1.x
stdin channel asserting NO drop file, beside an unknown SUCCESS wording still
asserting a visible drop. They sit together because the CONTRAST is the
contract; pinning them apart would let a reordered guard or a tightened regex
regress one while the other kept passing. Mutation-verified: neutering the
grounded pattern fails F4d.
docs/reference/kiro-ide-hook-payload.md no longer claims that any write wording
without a match records a visible drop -- it described exactly the behaviour
this change alters, in two places.
910bb6e to
4d45252
Compare
Summary
/aidlc --doctorreports hook decay on the Kiro IDE harness when a write simply failed. The adapter'saudit-and-sensorstarget records a visible hook drop whenever it cannot extract a file path from a write tool's result, and two very different situations reach that branch:Only (b) belongs in the drop log. Recording (a) made
--doctorreport degradation on a workspace whose hooks were working perfectly, which trains the reader to ignore the one channel that matters when something genuinely breaks.Found while investigating #688 on a live Kiro IDE workflow; split out of #687 because it is an independent concern with an independent risk profile.
Revised after review (CHANGES_REQUESTED @
866ae5c): rebased and re-bumped, the payload doc corrected, a pinning test added, the wrong-suite citation fixed, and the pattern evidence graded. Per-finding response in the comments.Round 2 addressed at
94fdbb64: the branch was rebuilt as one commit fromupstream/v2to expunge the two accidental 61.5 MB.bun-buildblobs,*.bun-buildis now ignored, F4d/F4e drive the real 1.x stdin channel, and the CHANGELOG evidence and stale-drop upgrade guidance are corrected.Round 4 addressed at
2afbf171: rebased onto currentupstream/v2(c73ee984/ 2.5.37), re-bumped to 2.5.40, regenerated all five harness distributions, and clarified both payload-document references sohookDebugis described as writing only when hook debugging is enabled. Versions 2.5.38 and 2.5.39 are already claimed by open PRs #712 and #705, respectively, so 2.5.40 was the first unclaimed slot found.Changes
Why the existing guard cannot catch it. The
toolSuccess === falsefailed-write guard (#417) still works on the 0.12 channel, but IDE 1.x delivers context on stdin with no success flag at all. A failed write is therefore indistinguishable from a successful one at the payload level — the failure arrives only as error prose intool_response. The real-world case that produced this was astr_replacewhose old string matched multiple times:Classify before logging. Recognised failure prose is now recorded via
hookDebug(available underAIDLC_HOOK_DEBUG, where it belongs) and (b) keeps the visiblerecordHookDrop. The fail-open path is unchanged — the event is still not forwarded either way, so no audit or sensor behaviour changes.Pattern evidence is graded, not assumed. Of the four failure-prose patterns only
^Caught an error whileis grounded in a live capture;^Error:,^Failed toand^An error occurredare marked in the code as defensive guesses, because no capture in this repo or indocs/reference/kiro-ide-hook-payload.mdbacks them. They are kept because the risk direction is one-way — a match can only suppress a drop after path extraction has already failed, and none of the known success wordings (Created the …,Replaced text in …,Appended the text to …) can collide with them — and the comment instructs deleting a pattern rather than widening the set if a capture ever contradicts one.The classifier is deliberately narrow. Every pattern is start-anchored, rather than a loose "contains error" test that would swallow a successful write to a file whose name mentions an error. Anything unrecognised is still treated as a success and still earns a visible drop: the default stays biased toward reporting rather than silence, so this can only ever remove false positives, never mask real decay.
User experience
Before. A
str_replacefails because the old string matched multiple times — a normal authoring mistake, correctly rejected by the tool./aidlc --doctorthen reports hook degradation for the Kiro IDE harness, pointing atkiro-adapter.drops, on a workspace where every hook is functioning.After. That failure no longer appears in the drop log, so
--doctorstays quiet. A genuinely unparseable successful write is still reported, unchanged — the signal keeps working, only the false positive is gone.Checklist
Test Plan
Coverage added
t218gains a pairedF4d/F4e, sitting beside the existingF4cso the contrast lives in one place:Caught an error while replacing string String '[Answer]:' found multiple times in the file) through the 1.x stdin channel: the drops file does not exist, and noARTIFACT_UPDATEDrow is writtenfs_writestill drops — unchangedThey are deliberately a pair. The contract is the distinction, so pinning the halves apart would let a reordered guard or a tightened regex regress one while the other kept passing.
Mutation-verified: neutering the one grounded pattern makes
F4dfail (Expected: false Received: true), 46 pass / 1 fail. Restored: 47/47.Behaviour check for reviewers
kiro-adapter.dropsstr_replacewith the verbatim failure prosefs_writewith unrecognised success wordingValidation
Run on the Round 4 rebased branch (
2.5.40on top ofv2at 2.5.37 /c73ee984).bun run check(dist byte-parity across five harnesses, three tsconfigs, Biome over 557 files): cleanbun tests/gen-coverage-registry.ts --check: clean;bun scripts/ci-changelog-guard.ts upstream/v2: all 149 upstream entries preserved, 1 newt218: 47/47 for this PR file — the suite that actually drives the changed branch. Bun also discovered the untouched fix(stop-hook): honour conversational turns on transcript-free harnesses #687 sibling worktree and reported 92/92 aggregate. (An earlier revision of this body citedt147; that suite scaffoldsdist/kiro/.kiro, the CLI adapter, and touches the IDE adapter only through a static respawn-source check, so it never exercised this change and is no longer cited.)t68: 7/7 for this PR file; Bun also discovered the untouched fix(stop-hook): honour conversational turns on transcript-free harnesses #687 sibling worktree and reported 14/14 aggregateThe three failures are pre-existing
Confirmed by a
git worktreechecked out at bareupstream/v2, independently of this branch, and reproduced identically on the sibling branch #687.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 updatedt19Also reported on #687 and, for
t66/t89, on #615.Scope
One authored harness source file (
harness/kiro-ide/hooks/aidlc-kiro-adapter.ts), plust218cases, documentation/CHANGELOG corrections, and a root*.bun-buildignore rule. No core change, no other harness affected.docs/reference/kiro-ide-hook-payload.mddescribed the old unconditional behaviour in two places — "error prose that matches no known pattern records a visible hook-drop", and "A write-class tool whose wording does not match a known pattern records a visible hook-drop (never a silent no-op)" — and is corrected in the same commit per the repo Documentation Policy. An earlier revision of this body claimed "no output-format change", which did not account for that doc describing exactly the drop-log behaviour this changes.No command, flag, or audit/sensor behaviour changes; no breaking change for CI or scripts.
Not verified here
Three of the four failure patterns are unobserved. See Changes. They are defensive, one-way, and labelled as such in the code rather than presented as captures. If preferred, shipping only the grounded pattern is a two-line diff.
Upgrade
mkdir -p your-project/.kiro && cp -R dist/kiro-ide/.kiro/. your-project/.kiro/If
/aidlc --doctorstill reports a Kiro adapter drop created before this fix, inspect the affected intent record’s<record>/.aidlc-hooks-health/kiro-adapter.drops. Delete only that individual file, and only after verifying its entries are historical/stale; preserve it if it contains current failures.Version
Bumps to 2.5.40, rebased onto
v2at 2.5.37 (c73ee984). Open PR #712 currently claims 2.5.38 and open PR #705 claims 2.5.39, so 2.5.40 was the first unclaimed slot found. The cleaned branch remains one commit above the current base and was force-updated with an explicit lease after local validation.Ordering: there is no code dependency between this PR and #687 — they touch disjoint regions of the adapter and the split is purely about reviewability. Either merge order works.
References
toolSuccessfailed-write guard this complements 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.