test(kiro-ide): pin the legacy execute_pwsh populated-command approval flip

[fsatsuki]

Summary

Narrowed to a test-only change, per review.

#1000 landed on main and already carries everything this PR originally implemented: the Kiro IDE shell-alias predicate (isKiroShellTool() covering execute_bash / execute_pwsh / shell), the Kiro CLI Bash canonicalization, and the CLI state-transition-guard change. #1044 is closed. Both adapter edits and the t147 execute_pwsh case here are therefore redundant and have been dropped.

What is not yet pinned on main is the legacy { toolName, toolArgs } channel on plan-approval-guard with a populated command across the approval boundary. plan-approval-guard already covers execute_pwsh for empty-argument recovery (the neighbouring execute_pwsh and shell are routed to legacy recovery exactly like execute_bash test), but an empty-argument payload is decided by the opaque-mutation branch — it never reaches the code that resolves a shell alias. A Windows host names the same tool execute_pwsh and does supply toolArgs.command. This PR keeps that one regression test to pin the populated-command transition.

Changes

• tests/unit/t218-kiro-ide-hook-adapter.test.ts: add legacy execute_pwsh with a populated command flips from blocked to permitted across approval (single new test, no edits to existing cases). It walks the same legacy mediation flow as the neighbouring execute_bash test and asserts the approval-boundary transition:
  • before approval — exit 2 and the legacy recovery block (recovery requires a human response), matching the neighbouring routed-to-recovery test. isKiroShellTool matches on the tool name alone, so a populated command reaches the same recognition branch; pinning that message positively proves execute_pwsh was recognized as a shell rather than merely not hitting another branch;
  • after approval — exit 0, so the AI-DLC loop can advance.
• Merged current main into the branch and resolved both adapter files and t147 back to main's content. The merge tree is identical to main; the PR diff is the single test file.
• No implementation change, no dist change, no version/CHANGELOG/README bump — release preparation owns version and changelog consolidation.

User experience

Unchanged. This PR adds coverage only; the behaviour it pins already ships on main via #1000.

Checklist

If an item does not apply, leave it unchecked.

• I have reviewed the contributing guidelines
• I have performed a self-review of this change
• Changes have been tested
• Changes are documented
• If this change adds an input to any fingerprint, epoch, or receipt identity, the description names the human-visible change it detects

Test plan

• bash tests/run-tests.sh --unit --filter "t218" → 87 pass / 0 fail, including the new case.
• Fail-first evidence: narrow isKiroShellTool() in harness/kiro-ide/hooks/aidlc-kiro-adapter.ts to return toolName === "execute_bash";, run bun scripts/package.ts, then bun test tests/unit/t218-kiro-ide-hook-adapter.test.ts -t "legacy execute_pwsh with a populated command". With the predicate narrowed, execute_pwsh is no longer recognized as a shell, so the pre-approval assertion fails: the non-shell fallback returns the opaque Plan Approval blocked this mutation… block instead of recovery requires a human response. Restoring the predicate makes it pass again. This is the positive branch pin requested in review — the negative not.toContain("target path…") form would have passed the non-shell fallback too and only surfaced later, so it did not prove execute_pwsh was recognized as a shell.
• bunx biome check tests/unit/t218-kiro-ide-hook-adapter.test.ts → clean.
• bun test tests/unit/gen-coverage-registry.test.ts → 37 pass / 0 fail (committed coverage registry stays fresh).

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.

[wowzoo]

Thanks — this is a clean, well-scoped fix, and I checked completeness rather than assuming it: all nine
toolName === "execute_bash" comparisons in the kiro-ide plan-approval path go through shellTool()
(:1271, :1296, :1309, :1343, :1368, :1388, :1453, :1511, :1542), with no literal
comparison left in that path. Both CLI sites are covered (:593 state-transition-guard, :875
canonicalTool), the core guard correctly needs no change because it matches on the canonical Bash,
and no other harness references execute_bash. CI is 16/16 at this head.

I'm leaving this as a comment rather than a blocking review, because there's a triage question that
isn't yours to resolve — see the last section. Everything below is measurement you can use either way.

What #1044 actually was — the direction differs per harness

Your summary says execute_pwsh calls "slipped past the code-generation plan-approval gate and the
lifecycle state-transition guard". That's accurate for the CLI and backwards for the IDE:

• kiro CLI — both guards genuinely bypassed. plan-approval-guard let execute_pwsh through
  (t147:343 in 1bb: expected 2, received 0), and state-transition-guard skipped it entirely
  via the early process.exit(0) at :593. This is the security-relevant half, and it's worth stating
  as two separate bypasses rather than one.
• kiro IDE — the opposite. execute_pwsh { command } was blocked (exit 2, "target path is
  missing or unsupported") even in a fully approved window, where execute_bash { command } exits 0.
  Nothing slipped past; the AI-DLC loop simply couldn't advance. A liveness failure, not a bypass.

And shell was broken on the IDE too. Your summary says the adapters "classified only execute_bash
and shell as shell tools", but on the kiro-ide plan-approval path only execute_bash was
special-cased — I measure shell { command } at exit 2 pre-fix in the approved window, going to 0
after. So this change also repairs shell there, which the description doesn't claim credit for. (The
terminal-command guard at :923 did already list all three, which is probably where that wording came
from.) Since #1044 came from a Windows user, being precise about which half was a bypass matters to
anyone assessing exposure.

The new t218 case passes against the unfixed adapter

Worth knowing regardless of what happens to this PR. I ran both suites against the pre-fix adapters to
check the regressions fail first. t147 does its job — 34 pass / 1 fail at t147:343. But t218
returns 75 pass / 0 fail both pre- and post-fix, so the kiro-ide half has no failing-first coverage.

The cause is the payload shape: both assertions use toolArgs: {}, and with empty args
opaqueMutation is already satisfied by its first clause (Object.keys(toolArgs).length === 0), so
control never reaches the branches shellTool() changes.

• Pre-approval, {}: pre-fix falls to the :1484 "not safely attributable" deny, post-fix takes the
  :1453 shell deny. Both exit 2, and neither message contains "target path is missing or
  unsupported", so expect(preApproval.stderr).not.toContain(…) holds either way.
• Post-approval, {}: :1511 is gated on Object.keys(toolArgs).length > 0, which is false, so both
  revisions fall through to return null and exit 0.

A realistic payload is also the case that actually regressed:

-        JSON.stringify({ toolName: "execute_pwsh", toolArgs: {} }),
+        JSON.stringify({
+          toolName: "execute_pwsh",
+          toolArgs: { command: "bun .kiro/tools/aidlc-orchestrate.ts next" },
+        }),

With { command } in the approved window I measure exit 2 pre-fix, exit 0 post-fix, and the same
for shell { command }.

The triage question — #1000 also claims this issue

#1000 declares Closes #1044 as well, alongside #930, #975, #977, #981, #987, and #995. It isn't a
stale claim: at head ac6a86f6 it carries an isKiroShellTool() helper (:184-185) covering all three
names, applies it at the plan-approval sites and at :962 — which also collapses the duplicate
triple still sitting at :923 here — includes execute_pwsh in the CLI canonicalTool (:875), and
its :1556 reads state.active && Object.keys(toolArgs).length > 0 && !isKiroShellTool(toolName).

Its coverage is also already in the shape I described above: tool_input: { command: … } payloads and a
SHELL_NAMES = ["execute_bash", "execute_pwsh", "shell"] table (t218:1559), plus t147:345
1bc: execute_pwsh normalises to Bash in the plan-approval-guard path like execute_bash. So it does not
have the gap this PR's test has.

That makes it a real decision rather than a formality. #1000 is 172 files with seven rounds of
CHANGES_REQUESTED behind it and two reviewers still active, so it is moving but not imminent; this PR
is four files and green today. If the maintainers want the small fix in first, the test payload above is
the one thing I'd change and #1000 rebases over it easily. If they'd rather #1000 own it, nothing here is
wasted — the direction correction above should go into #1000's description, since it inherits the same
wording problem.

I'm deliberately not gating this either way. Flagging it now so you don't polish something that might be
superseded.

Non-blocking — the duplication that caused #1044 is still here

SHELL_TOOLS is now the set for the plan-approval path, but :923 keeps its own
tool !== "execute_bash" && tool !== "execute_pwsh" && tool !== "shell" triple for the terminal-command
guard, and the CLI adapter keeps two ad-hoc literals (:593, :875) rather than a shared set. No
correctness hole today — all three already list all three names — but it's three independent spellings
of "is this a shell tool" across two files, which is the shape that let execute_pwsh go missing.
#1000 solves this by construction at :962.

One more thing for whoever sequences these: this PR and #986 edit the same non-empty-args line, so
they conflict, and #986's state.active && alone is insufficient — I applied it to the base adapter and
approved-window execute_pwsh { command } still exits 2. The correct condition needs both that and the
shell test, which is what #1000's :1556 already does. #1051 + #1040 is clean.

Validation I ran at d401ec96

check	result
bun test tests/unit/t147-kiro-hook-adapter.test.ts	35 pass / 0 fail
bun test tests/unit/t218-kiro-ide-hook-adapter.test.ts	75 pass / 0 fail
the same two suites against the pre-fix adapters	t147 34 / 1 ✅ · t218 75 / 0 ❌
approved-window probe, { command } payloads	pre-fix execute_pwsh 2 / shell 2 / execute_bash 0 → post-fix all 0
bun scripts/package.ts --check	deterministic across two independent builds, all 7 harnesses
bun run typecheck / bun run lint	exit 0 / exit 0
hosted CI at this head	16/16 SUCCESS

Nice piece of work either way — the SHELL_TOOLS shape is the right one, and it's what #1000 arrived at
independently.

[leandrodamascena]

Thanks for addressing the Windows shell-tool handling.

I reviewed the current head d401ec967aa81643d8a6d1f83d70788958b6c721 against base 0d399dd828b59e84d90f7cc198c69fab9ad8f1a7. The change is aligned with #1044: Kiro CLI needs to prevent execute_pwsh from bypassing its guards, while Kiro IDE needs to permit recognized shell commands after plan approval.

[P2] The new Kiro IDE regression test does not exercise the fixed path

In tests/unit/t218-kiro-ide-hook-adapter.test.ts:1786 and :1845, the execute_pwsh payload uses toolArgs: {}. With no command, both assertions pass against the adapter before this PR because unrelated empty-input handling produces the expected exit codes. Therefore, the test does not fail when the implementation fix is removed.

Please provide a realistic shell payload in both calls, for example:

toolArgs: {
  command: "bun .kiro/tools/aidlc-orchestrate.ts next"
}

This exposes the relevant behavioral transition: before the fix, an approved execute_pwsh call remains blocked with exit code 2; after the fix, it is permitted with exit code 0.

Please also update the PR description to distinguish the two behaviors:

• Kiro CLI had a guard-bypass/security issue.
• Kiro IDE had a liveness issue where execute_pwsh and shell remained blocked after approval.

The focused t147 coverage is valid, the implementation direction is sound, and all 16 current CI checks are green. No version bump is needed because release preparation owns version and changelog consolidation.

[fsatsuki]

@leandrodamascena @wowzoo
Thanks for the detailed measurement — especially confirming the fail-first gap.

Fixed the t218 test in d44d2ba: both the pre- and post-approval assertions now send a realistic { command: "bun .kiro/tools/aidlc-orchestrate.ts next" } payload instead of toolArgs: {}, so control reaches the branches shellTool() actually changes instead of short-circuiting through the empty-args opaqueMutation clause.

I verified fail-first the way you described, using a git worktree checked out at origin/main with only the updated test files copied in:

• t218 treats execute_pwsh identically to execute_bash (#1044) — fail against the unfixed adapter, pass against this branch.
• t147 1c — same (fail → pass).

On the direction correction: you're right that the summary is imprecise. It's a genuine two-way bypass on the CLI (plan-approval-guard let it through, and state-transition-guard skipped it at the early exit) but a liveness failure on the IDE (an approved execute_pwsh { command } was blocked), and this change also repairs shell on the IDE path. I'll correct the PR description to say exactly that rather than the one-line "slipped past both guards" wording.

On the #1000 overlap and the remaining three-spellings duplication: understood and happy to defer to the maintainers on which PR owns #1044. If they prefer the small fix first, this is ready; if #1000 lands first, the direction note above should move into its description.

I also updated the description of PR.

[apackeer]

Thank you for updating the regression test after review. #1000 has now merged and includes the Kiro IDE shell-alias handling, CLI Bash canonicalization, and CLI state-transition guard change covered here. I checked those changes against current main, and #1044 is already closed.

There is still a useful test to preserve: your t218 case sends a populated toolArgs.command through legacy execute_pwsh before and after human approval. Main's corresponding approved legacy shell assertion still uses execute_bash with empty arguments, so it does not pin that exact regression.

Could you narrow this to a test-only PR against current main: drop the two adapter changes and the redundant t147 addition, retain/adapt the t218 populated-command regression, update the title and description to that scope, and rerun the focused t218 suite? Then we can review the remaining test contribution.

[fsatsuki]

@apackeer
Thank you for fix this issue.

OK, I will do it.

Could you narrow this to a test-only PR against current main: drop the two adapter changes and the redundant t147 addition, retain/adapt the t218 populated-command regression, update the title and description to that scope, and rerun the focused t218 suite? Then we can review the remaining test contribution.

[fsatsuki]

@apackeer
I left only the t218 test case and updated the PR description.

[wowzoo]

Thank you for narrowing this to the single case, and I am sorry the thread went quiet for five days
afterwards — you did what the review asked and then had to wait.

Four things from re-measuring at 5134a9d50.

The red check is not a branch-test failure. The only failing check is Check Merge Status; every other
check passes, including all four unit shards. That job does not inspect or execute this diff — it can go red
on repository-wide merge controls, on an unavailable PR number, or on a failure while retrieving or parsing
the open-PR list — so the result alone is not evidence of a defect here.

One correction to the description. It says every legacy assertion in t218 uses execute_bash with
empty arguments. Two are already there: the terminal-command-guard case drives the legacy
{ toolName, toolArgs } channel with a populated command and asserts exit 2, and the test
execute_pwsh and shell are routed to legacy recovery exactly like execute_bash already covers
execute_pwsh on plan-approval-guard through the legacy channel. The gap is narrower than stated and
still real: before this PR, plan-approval-guard has no legacy populated-command assertion across the
approval boundary — execute_pwsh is covered there for empty-argument recovery, and this case adds the
populated-command transition. Saying it that way keeps the test's reason legible to the next reader.

One request on the assertion. The case turns on
expect(preApproval.stderr).not.toContain("target path is missing or unsupported"). The preceding
fs_write leaves an active legacy write window, and from inside that window both the recognized-shell
recovery branch and the non-shell fallback branch return the legacy plan-approval block: both exit 2, and
neither emits the target-path wording, which belongs to the opaque-mutation branch further down. So the
negative assertion does not show that execute_pwsh was recognized as a shell.

legacyRecoveryBlockReason is the branch worth pinning, and the neighbouring
execute_pwsh and shell are routed to legacy recovery exactly like execute_bash test already pins it
positively with toContain("recovery requires a human response") while cross-checking all three shell
names against each other. isKiroShellTool matches on the tool name alone, so a populated command reaches
that same recognition branch — following the neighbouring shape here would make the case prove what it is
for, without needing a new message.

One cross-PR note, with no action requested here. #1157 consolidates kiro-ide into kiro: it renames
this test and repoints its scratch tree to dist/kiro/.kiro. The two therefore overlap on this file and
will need reconciling in whichever order they land, but the renamed test itself points at the tree #1157
produces. I am raising that on #1157.

[github-actions]

AIDA findings ledger

ID	Sev	Status	Title	Decided by
F1	P3	🔴 open	Comment misstates the empty-argument recovery branch	—

Open blocking findings (P0/P1): 0. Accepted and rejected findings never count toward the next action.

Maintainer commands (repository write access) — put them on the first lines of a comment, one per line, several ids per line allowed:
/aida accept F# [F#…] <reason> · /aida reject F# [F#…] <reason> · /aida reopen F# [F#…] · /aida status · /aida full (next review covers the whole head)
P0 and P1 findings can be accepted (visible, risk owned by the maintainer) but not rejected. A comment is applied all-or-nothing.
Do not edit this comment: AIDA verifies its digest and refuses to run on an edited ledger. To start over, delete it.

ledger.json

{
  "version": 4,
  "pullRequest": 1051,
  "nextId": 2,
  "findings": [
    {
      "id": "F1",
      "priority": "P3",
      "category": "contracts",
      "title": "Comment misstates the empty-argument recovery branch",
      "anchors": [
        {
          "kind": "line",
          "path": "tests/unit/t218-kiro-ide-hook-adapter.test.ts",
          "side": "RIGHT",
          "sha256": "79e4199215b13f3e283d90cbc9d63a9faec7c2fcd499538ecf638590e04261f4"
        }
      ],
      "status": "open",
      "firstSeen": {
        "head": "7ede5a0fb3e8b7c49925f4a5ee80f7e91605a9dd",
        "at": "2026-09-24T05:45:06.267Z"
      },
      "lastSeen": {
        "head": "7ede5a0fb3e8b7c49925f4a5ee80f7e91605a9dd",
        "at": "2026-09-24T05:45:06.267Z"
      }
    }
  ],
  "events": [
    {
      "at": "2026-09-24T05:45:06.267Z",
      "kind": "opened",
      "by": "aida",
      "id": "F1",
      "head": "7ede5a0fb3e8b7c49925f4a5ee80f7e91605a9dd"
    }
  ],
  "review": {
    "head": "7ede5a0fb3e8b7c49925f4a5ee80f7e91605a9dd",
    "readiness": 4,
    "risk": 1,
    "decision": "merge"
  }
}

[github-actions]

Reviewed 7ede5a0fb3e8b7c49925f4a5ee80f7e91605a9dd against 1b0645858664405ae54129d00f6e37c85480173e and current repository behavior.

Inspection: 1 changed file. Scope: full head (first review of this pull request).

Final Assessment

Human decision aid only: Readiness 5/5 is best; Risk 1/5 is best. These scores inform the maintainer; the next action below follows finding severity (any open P0/P1 → author/change) and does not approve or merge the PR.

Readiness: 4/5 — The regression test is valid and narrowly scoped; only its explanatory branch rationale needs correction.

Risk: 1/5 — The change affects tests only and is readily reversible. The surviving issue is a misleading comment, not runtime behavior.

Decision required: Maintainer — decide whether to merge this PR. The test behavior is sound and the sole surviving finding is a non-blocking P3 documentation issue.

Validation performed:

• Inspected the complete diff, changed-file manifest, sole 3,971-line head snapshot, PR metadata, discussion, ledger, and all specialist outputs.
• Re-derived the test path through the legacy Kiro IDE adapter, Plan Approval state helpers, core guard, projection setup, and neighboring tests.
• Confirmed the new test covers populated execute_pwsh forwarding after approval and contains no runtime changes or active prompt attack.

Findings: 0 blocking, 1 advisory.

Ledger: 1 open, 0 retained blocking, 0 accepted, 0 suppressed as rejected by a maintainer. Maintainers act on findings with /aida commands in the ledger comment.

Contracts & Compatibility

P3 [F1]: Comment misstates the empty-argument recovery branch

Evidence: tests/unit/t218-kiro-ide-hook-adapter.test.ts:2005.

Problem: The comment says an empty legacy shell payload never reaches shell-alias resolution. In the neighboring recovery setup, the preceding argument-less write leaves an active write window, and the adapter checks isKiroShellTool before computing opaqueMutation. That existing test therefore already exercises execute_pwsh alias recognition; the populated payload uniquely adds post-approval forwarding coverage.

Impact: The contradictory rationale can mislead maintainers about which branch the regression test protects, although it does not invalidate the test or affect users.

Required correction: Revise the comment to state that empty arguments cover recovery-time alias recognition, while populated arguments add coverage for forwarding through the approved boundary.

User Experience

User experience change: This PR adds regression coverage for existing Kiro IDE Windows-shell Plan Approval behavior without changing shipped runtime code.

Assessment: Users retain the same approval and recovery behavior; the only indirect risk is future maintainers relying on an inaccurate test comment.

Residual risk: Repository code and tests were not executed, as prohibited by the review contract; behavior was verified by static inspection of the immutable snapshots and related base contracts.

Reviewed by AIDA (AI-DLC Developer Agent).

[AI-PR-REVIEWED] 7ede5a0

I approved

[leandrodamascena]

Approve