Skip to content

fix(kiro-ide): stop reporting a failed write as harness decay - #689

Merged
apackeer merged 1 commit into
awslabs:v2from
csotomon:fix/kiro-ide-doctor-drop-log-accuracy
Aug 6, 2026
Merged

fix(kiro-ide): stop reporting a failed write as harness decay#689
apackeer merged 1 commit into
awslabs:v2from
csotomon:fix/kiro-ide-doctor-drop-log-accuracy

Conversation

@csotomon

@csotomon csotomon commented Jul 30, 2026

Copy link
Copy Markdown

Summary

/aidlc --doctor reports hook decay on the Kiro IDE harness when a write simply failed. The adapter's audit-and-sensors target 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:

  • (a) the write failed — no artifact exists, so declining to forward it is correct behaviour, not decay;
  • (b) the write succeeded but its result wording matched no known pattern — the invisible degradation this harness exists to surface.

Only (b) belongs in the drop log. Recording (a) made --doctor report 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 from upstream/v2 to expunge the two accidental 61.5 MB .bun-build blobs, *.bun-build is 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 current upstream/v2 (c73ee984 / 2.5.37), re-bumped to 2.5.40, regenerated all five harness distributions, and clarified both payload-document references so hookDebug is 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 === false failed-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 in tool_response. The real-world case that produced this was a str_replace whose old string matched multiple times:

audit-and-sensors: str_replace yielded no extractable path from toolResult:
Caught an error while replacing string String '[Answer]:' found multiple times in …

Classify before logging. Recognised failure prose is now recorded via hookDebug (available under AIDLC_HOOK_DEBUG, where it belongs) and (b) keeps the visible recordHookDrop. 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 while is grounded in a live capture; ^Error:, ^Failed to and ^An error occurred are marked in the code as defensive guesses, because no capture in this repo or in docs/reference/kiro-ide-hook-payload.md backs 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_replace fails because the old string matched multiple times — a normal authoring mistake, correctly rejected by the tool. /aidlc --doctor then reports hook degradation for the Kiro IDE harness, pointing at kiro-adapter.drops, on a workspace where every hook is functioning.

After. That failure no longer appears in the drop log, so --doctor stays quiet. A genuinely unparseable successful write is still reported, unchanged — the signal keeps working, only the false positive is gone.

Checklist

  • I have reviewed the contributing guidelines
  • I have performed a self-review of this change
  • Changes have been tested
  • Changes are documented

Test Plan

Coverage added

t218 gains a paired F4d/F4e, sitting beside the existing F4c so the contrast lives in one place:

Case Pins
F4d The verbatim captured prose (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 no ARTIFACT_UPDATED row is written
F4e An unknown success wording on the same tool: a visible drop still lands
F4c (existing) Unrecognised wording on fs_write still drops — unchanged

They 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 F4d fail (Expected: false Received: true), 46 pass / 1 fail. Restored: 47/47.

Behaviour check for reviewers

# (a) a FAILED write -> must NOT be recorded as decay
echo '{"tool_name":"str_replace","tool_response":"Caught an error while replacing string ..."}' \
  | bun .kiro/hooks/aidlc-kiro-adapter.ts audit-and-sensors

# (b) a SUCCESSFUL write with unrecognised wording -> must still be recorded
echo '{"tool_name":"fs_write","tool_response":"Wrote bytes somewhere unfamiliar"}' \
  | bun .kiro/hooks/aidlc-kiro-adapter.ts audit-and-sensors
Input kiro-adapter.drops
str_replace with the verbatim failure prose 0 entries (was 1)
fs_write with unrecognised success wording 1 entry (unchanged)

Validation

Run on the Round 4 rebased branch (2.5.40 on top of v2 at 2.5.37 / c73ee984).

  • bun run check (dist byte-parity across five harnesses, three tsconfigs, Biome over 557 files): clean
  • bun tests/gen-coverage-registry.ts --check: clean; bun scripts/ci-changelog-guard.ts upstream/v2: all 149 upstream entries preserved, 1 new
  • t218: 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 cited t147; that suite scaffolds dist/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 aggregate
  • Earlier full default-suite run (before the Round 4 rebase): 284 files, 3 failed files — all three pre-existing

The three failures are pre-existing

Confirmed by a git worktree checked out at bare upstream/v2, independently of this branch, and reproduced identically on the sibling branch #687.

File Cause Touched here?
t66 #647's claim-sources sensor — Stage "intent-capture" imports unknown sensor id "claim-sources"; the golden export fixture was not updated No
t89 Same claim-sources root cause; the sensor-resolution fixtures were not updated No
t19 Live preflight; needs authenticated AWS credentials No

Also reported on #687 and, for t66/t89, on #615.

Scope

One authored harness source file (harness/kiro-ide/hooks/aidlc-kiro-adapter.ts), plus t218 cases, documentation/CHANGELOG corrections, and a root *.bun-build ignore rule. No core change, no other harness affected.

docs/reference/kiro-ide-hook-payload.md described 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 --doctor still 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 v2 at 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

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@apackeer apackeer 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.

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:

  1. feed the verbatim Caught an error while replacing string ... prose through audit-and-sensors on the 1.x stdin channel and assert the drops file does not exist;
  2. 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.
  • hookDebug is 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.

@csotomon
csotomon force-pushed the fix/kiro-ide-doctor-drop-log-accuracy branch 3 times, most recently from 9a88f52 to 53ced04 Compare July 31, 2026 14:17
@csotomon

csotomon commented Jul 31, 2026

Copy link
Copy Markdown
Author

Addressed @ 53ced04

All four addressed. Two notes up front, both on me:

I missed this review for a full day. I had been checking gh pr view --json comments and this arrived as a review, not a comment, so it never showed up in what I was looking at. Nothing subtle about the mistake — I was querying the wrong field. Apologies for the lag.

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: done

Rebased onto v2 at 2.5.30 (d0cd10a6). You called 2.5.31 correctly: #686 holds it, so #687 took 2.5.32 and this PR now takes 2.5.33. GitHub reports MERGEABLE. The stale "2.5.27 to #687" pairing is gone from the body.

P2 — kiro-ide-hook-payload.md: both lines fixed

You were right that the PR body's "no output-format change" claim did not cover this: the doc describes the drop-log behaviour, which is precisely what changed.

  • The consequences bullet for audit-logger / sensor-fire now describes the split — recognised failure prose goes to hookDebug, anything else still records a visible drop — instead of asserting the drop is unconditional.
  • Observation 2 ("1.x carries no success flag") now explains why the classification exists at that point: the channel cannot report failure structurally, so failure arrives only as prose, so the adapter must classify before logging.

P2 — pinning test: added as a paired case, mutation-verified

Added F4d/F4e to t218, adjacent to F4c exactly as you suggested so the contrast lives in one place:

  • F4d — the verbatim captured prose (Caught an error while replacing string String '[Answer]:' found multiple times in the file) through the 1.x stdin channel, asserting the drops file does not exist and no ARTIFACT_UPDATED row was written.
  • F4e — an unknown success wording on the same tool, asserting a visible drop still lands.

They are deliberately a pair, with a comment saying so: the contract is the distinction, and pinning the two halves apart would let a reordered guard or a tightened regex regress one while the other kept passing — which is the regression you described.

Verified they bite: neutering the one grounded pattern makes F4d fail (Expected: false Received: true), 46 pass / 1 fail. Restored, 47/47.

P2 — wrong suite cited: corrected

You are right, and thank you for the file:line. t147 scaffolds dist/kiro/.kiro — the CLI adapter — and touches the IDE adapter only through a static respawn-source check. It never drove the branch this PR changes. The body now cites t218 as the suite that exercises it, with t245 for the registrations. I have removed t147 from this PR's validation list rather than leaving it as decoration.

P3 — pattern evidence: graded, not quietly kept

Kept all four patterns but stopped describing them as observed. The comment now grades them explicitly:

^Caught an error while   OBSERVED live on IDE 1.x (str_replace, multiple matches)
^Error:                  DEFENSIVE GUESS. Not observed; no capture in this
^Failed to               repo or in docs/reference/kiro-ide-hook-payload.md
^An error occurred       backs these three shapes.

I checked before writing that: Caught an error while appears in this repo only in my own regex, and the other three strings appear only in unrelated code of ours, not in any capture. So there was nothing to cite.

The reasoning for keeping them is recorded alongside, in your terms: the risk is one-way, because 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. The comment ends with an instruction to delete a pattern rather than widen the set if a capture ever contradicts one. A CHANGELOG bullet carries the same caveat so it is visible to users, not only to readers of the adapter.

If you would rather ship only the grounded pattern and add the others when captures appear, say so — I have no attachment to the three guesses and the diff is two lines.

Validation

  • bun run check (dist parity across five harnesses, three tsconfigs, biome over 544 files): clean
  • bun tests/gen-coverage-registry.ts --check, bun scripts/ci-changelog-guard.ts: clean
  • t218 47/47 (the suite that drives this branch), t245, t68: 85 tests, 327 assertions green
  • Full default suite: 284 files, 3 failed filest19 (credentials), t66 and t89 (fix: ground Intent Capture claims (2.5.10) #647's claim-sources fixtures), all three failing identically on bare upstream/v2 and none touched here

Confirmed for the record

Your three "verified along the way" points match what I see: the success wordings cannot collide with the failure prefixes, hookDebug is opt-in and never throws, and t218 T1–T4 are unaffected because the 0.12 explicit-false channel returns earlier.

Follow-up: live Kiro CLI Stop evidence

I subsequently measured the host behavior on Kiro CLI 2.16.0 rather than inferring it from adapter code:

  • Legacy/V2 interactive path, embedded agent hooks.stop: the CLI consumed {"decision":"block","reason":"..."}, re-injected the reason, and produced the requested continuation. Stop ran twice—on the original response and on the induced continuation.
  • --v3 / KAS, standalone .kiro/hooks Stop: the CLI also consumed the block and re-injected the reason. A robust probe asked it to multiply Roman numerals XVII and XIX without placing 323 in the reason; KAS then inferred and emitted 323. Here Stop ran once for the original turn and did not run again after the induced continuation.

This closes the Kiro CLI uncertainty only. It does not show that Kiro IDE consumes Stop stdout and does not revise the separate IDE probe recorded on #687. For this PR, whose changed branch is the IDE 1.x failure-prose/drop classification, the CLI result is contextual evidence about host divergence rather than a new claim that the IDE branch is block-capable.

@csotomon
csotomon requested a review from apackeer July 31, 2026 17:39
iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Jul 31, 2026
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.
@apackeer

Copy link
Copy Markdown
Contributor

Round 2 @ 53ced04

All 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

  1. Rebase/re-bump: merge-base is v2 @ d0cd10a (2.5.30), trio consistent at 2.5.33, and the 2.5.31/fix: ground claims whose source tags are written adjacently (2.5.37) #686 + 2.5.32/fix(stop-hook): honour conversational turns on transcript-free harnesses #687 slot accounting in the body checks out. MERGEABLE, CI green.
  2. Payload doc: both spots in docs/reference/kiro-ide-hook-payload.md now describe the split correctly.
  3. t218 coverage: F4d/F4e exist, are paired deliberately, and the body's t147 miscite is retracted with the reason recorded. Good.
  4. Regex grounding: the evidence-graded comment is exactly what I asked for. To answer your direct question: keep all four patterns with the grading as-is. The one-way risk argument holds and the delete-don't-widen instruction is the right failure mode. No need to strip to the grounded one.

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 findings

N1 (P1, blocks): two 61.5 MB binaries committed at the repo root. .18c7639fbaefc075-00000000.bun-build and .18c763ffb87feffe-00000000.bun-build arrived with the force-push (absent from 866ae5c). They are the identical blob (26b9d2ee), Mach-O 64-bit executables, so a macOS bun build/crash artifact committed twice, 123 MB total into a public repo's history. Drop them from the branch history (not a follow-up delete commit, which would leave the blobs in history). Optionally gitignore *.bun-build so this cannot recur, your call.

N2 (P2): F4d/F4e do not drive the channel they claim to cover. Both call runIde(dir, ..., ctx(...)) (tests/unit/t218-kiro-ide-hook-adapter.test.ts:1042, 1057), and ctx() builds the 0.12 camelCase payload with toolSuccess: true delivered via USER_PROMPT (line 174). The test prose and the F4d name say "1.x channel", and the fix's motivating case is 1.x error prose with no success flag, but the 1.x parse path (tool_response, snake_case, no flag) is never exercised by these cases. The tests are not vacuous, toolSuccess: true falls through to the classifier, but the claim is false as written and the channel the fix exists for is untested. The suite already has the right helpers sitting above: runIdeStdin (line 142) + ctx1x (line 181). Either switch both cases to runIdeStdin(ctx1x(...)), or keep one 0.12 case and add the 1.x pair; just make the prose match what runs.

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. recordHookDrop appends (core/tools/aidlc-lib.ts:4857), --doctor reads every historical line (core/tools/aidlc-utility.ts:1661), and the bundle path counts any non-empty .drops as degraded (core/tools/aidlc-doctor-bundle.ts:1441-1443). So a workspace that accumulated false drops under the old adapter keeps reporting them after copying in the fixed shell, which is precisely the "doctor cries wolf" symptom this PR fixes going forward. Ask: extend the CHANGELOG upgrade instruction with one line telling users to delete a verified-stale kiro-adapter.drops under .aidlc-hooks-health/. Related, pre-existing and NOT on this PR: the doctor prose claims a .drops file "self-clears on a clean run" (aidlc-utility.ts:1648 comment and the FAILED-row fix text) but I could find no code path that truncates or removes .drops files; if you spot one I missed, point me at it, otherwise that is a separate issue.

Verdict

Implementation 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.

@csotomon

csotomon commented Aug 1, 2026

Copy link
Copy Markdown
Author

Round 2 addressed @ 94fdbb64

Thanks @apackeer — N1–N4 are addressed.

  1. N1 history cleanup: rebuilt the branch as exactly one commit on upstream/v2; neither .bun-build path appears in upstream/v2..HEAD or the resulting tree. Added *.bun-build to .gitignore. The cleanup rewrite used an explicit lease against 53ced04e; the final upstream rebase used another explicit lease against 64f0b10f. Local backup refs preserve both prior heads.
  2. N2 real 1.x channel: F4d/F4e now use runIdeStdin(..., ctx1x(...)), so they exercise snake_case tool_response with no success flag.
  3. N3 evidence wording: CHANGELOG now says start-anchored failure prefixes, preserving the next bullet’s observed-vs-defensive grading.
  4. N4 upgrade: CHANGELOG and the PR body now tell users to inspect the affected <record>/.aidlc-hooks-health/kiro-adapter.drops and delete only that individual file after verifying its entries are historical/stale. The separate pre-existing self-clears claim was intentionally left untouched here.

Post-rewrite validation: t218 green (47 tests in this branch; the local Bun invocation also discovered the sibling worktree copy), package byte-parity, coverage registry, changelog guard against upstream/v2, all three TypeScript configs, and Biome are green. Worktree and remote head both resolve to 94fdbb64.

Final base refresh: rebased onto current upstream/v2 at 57ae3d05, rebumped 2.5.33 → 2.5.34, regenerated all distributions, and re-ran the full validation listed above plus t68. The branch remains one clean commit above the current base.

Ready for re-review.

@csotomon
csotomon force-pushed the fix/kiro-ide-doctor-drop-log-accuracy branch from 64f0b10 to 94fdbb6 Compare August 1, 2026 03:08

@apackeer apackeer 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.

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-build path exists in the head tree; *.bun-build is ignored at .gitignore:9.
  • N2: F4d/F4e now use runIdeStdin plus ctx1x (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.

@apackeer

apackeer commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Approval status

Not approving yet. The substantive implementation is sound and all round-2 findings are fixed, but the current head (94fdbb64) is still conflicting with v2 at 6b264081.

Approval needs only the final refresh already described in the review:

  • rebase onto current v2, resolve the version/generated-tree conflicts, regenerate, and rerun checks;
  • coordinate the contested 2.5.34 slot and update the stale version/order paragraph;
  • clarify at docs/reference/kiro-ide-hook-payload.md:62,79 that hookDebug writes only when hook debugging is enabled.

Once those land and the refreshed head verifies, this is an approve.

iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Aug 3, 2026
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.
@apackeer

apackeer commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Thanks, Camilo. The implementation and the 1.x coverage are sound. v2 has moved again, but none of the substantive files changed after this branch's base. Repeatedly asking you to rebase around release metadata and generated-tree churn is not practical, so maintainers should take ownership of bringing this onto current v2, regenerating, running the current checks, and merging it.

The only content correction remaining is to clarify both hookDebug statements in docs/reference/kiro-ide-hook-payload.md as “sent to hookDebug (written only when hook debugging is enabled).” We can fold that into the maintainer integration as well; no further action is needed from you.

@leandrodamascena, could you take a final review pass on the substantive behavior and this integration direction?

@csotomon

csotomon commented Aug 5, 2026

Copy link
Copy Markdown
Author

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 v2 churn.

I’ll leave the branch as-is as requested. During maintainer integration, the two hookDebug references should be clarified as “sent to hookDebug (written only when hook debugging is enabled),” followed by the normal version selection, regeneration, and current checks.

Thanks as well for requesting the final review pass and helping move this work toward integration.

@leandrodamascena leandrodamascena 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.

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 passed
  • t68: 7 passed
  • bun 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.

@csotomon
csotomon force-pushed the fix/kiro-ide-doctor-drop-log-accuracy branch from 94fdbb6 to 2afbf17 Compare August 5, 2026 15:34
@csotomon

csotomon commented Aug 5, 2026

Copy link
Copy Markdown
Author

Round 4 is addressed in 2afbf171.

  • Rebased onto current upstream/v2 at c73ee984 (2.5.37); the branch remains exactly one commit above the base.
  • Re-bumped to 2.5.40. Open PR feat: add a tutorial scope for a taught single-sitting run (2.5.38) #712 already claims 2.5.38 and open PR feat: code-generation joins the unit-major construction walk (2.5.56) #705 claims 2.5.39, so 2.5.40 was the first unclaimed slot found.
  • Updated both references in docs/reference/kiro-ide-hook-payload.md to state that hookDebug is written only when hook debugging is enabled.
  • Regenerated all five harness distributions and confirmed package byte parity.
  • Confirmed the two accidental .bun-build artifacts are absent from both upstream/v2..HEAD history and the HEAD tree.
  • Force-updated the branch with an explicit lease against the previously observed remote head 94fdbb64.

Validation on the rebased commit:

  • bun test tests/unit/t218-kiro-ide-hook-adapter.test.ts: 47/47 for this PR file; Bun also discovered the untouched fix(stop-hook): honour conversational turns on transcript-free harnesses #687 sibling worktree, for 92/92 aggregate.
  • bun test tests/unit/t68-version-changelog-sync.test.ts: 7/7 for this PR file; 14/14 aggregate with the sibling worktree.
  • bun scripts/package.ts --check: all five harnesses in sync.
  • bun tests/gen-coverage-registry.ts --check: clean.
  • bun scripts/ci-changelog-guard.ts upstream/v2: all 149 upstream entries preserved, 1 new.
  • bun run check: package parity, all three tsconfigs, and Biome over 557 files clean.
  • git diff --check upstream/v2..HEAD: clean.

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 leandrodamascena 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.

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 passed
  • t68: 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.

@leandrodamascena

Copy link
Copy Markdown
Contributor

@apackeer can you pls check this again and maybe approve/merge? Thanks

@apackeer
apackeer force-pushed the fix/kiro-ide-doctor-drop-log-accuracy branch from 2afbf17 to 910bb6e Compare August 6, 2026 01:40

@apackeer apackeer 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.

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.
@apackeer
apackeer force-pushed the fix/kiro-ide-doctor-drop-log-accuracy branch from 910bb6e to 4d45252 Compare August 6, 2026 02:25
@apackeer
apackeer merged commit a795583 into awslabs:v2 Aug 6, 2026
5 checks passed
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