fix(dashboard): show error and restore focus on CollapsibleToolGroup approval rollback by aniruddhaadak80 · Pull Request #6065 · kirodotdev/KiroCrew

aniruddhaadak80 · 2026-08-26T14:11:04Z

What Problem This Solves

When an approval POST fails in CollapsibleToolGroup.submitDecision, the rollback path was mute and dropped focus: the row flips from "Approved" back to "Approval needed" with zero on-screen explanation, and keyboard focus falls to <body> since the optimistic resolve unmounts the decision buttons.

Why This Change Was Made

Mirrors ApprovalCard's existing error/focus pattern for consistency:

On failure, shows an inline ErrorNotice with the same i18n keys as ApprovalCard (terminal vs retryable failure messages)
Restores keyboard focus to the button matching the failed decision so a keyboard user retrying a failed Reject with Enter does not silently approve the command instead
Resets failure state when permission props change

User Impact

Users will now see an inline error message when an approval POST fails (e.g. network error, server rejection), and keyboard focus is properly restored to the failed decision button for retry. Previously this was a silent rollback with no explanation.

Evidence

TypeScript typecheck passes (tsc --noEmit — zero errors)
Change is limited to website/src/pages/chat/CollapsibleToolGroup.tsx (39 additions, 3 deletions)
Error handling pattern and i18n keys are identical to ApprovalCard.tsx (lines 43-72, 99-104)

Fixes #5554

Upstream landed the canonical spelling, security.redact_and_truncate (redact the FULL text, then cut), and converted most of the sites this branch originally covered; the rebased delta carries what remains: - cron_script._stderr_tail redacts a bounded span (the last _STDERR_TAIL_REDACT_SPAN bytes) BEFORE tail-slicing, so a key straddling the old seek-to-size-minus-limit window start no longer loses its anchoring prefix, while redaction stays O(span) per call: an adversarial subprocess cannot stall the worker by stuffing its stderr with credential-shaped tokens. Captures beyond _STDERR_FULL_REDACT_MAX are withheld behind a fixed marker instead. Tests pin the behaviors: a straddling key is scrubbed within budget, an oversized capture is withheld behind the marker, and the text handed to redact never exceeds the span regardless of capture size.

…lize helper The complete() helper's >/dev/null 2>&1 hides gh's actual HTTP error when a check-run finalize permanently fails. The ::warning:: line names the check-run but cannot carry the underlying error, leaving operators with no diagnostic. Remove the stderr suppression so the real error reaches the job log. Fixes kirodotdev#5822

…approval rollback When an approval POST fails in CollapsibleToolGroup.submitDecision, the rollback path was silent — it only did console.error and setLocalResolved(null), leaving the row flipped back with zero on-screen explanation and keyboard focus dropped to <body>. Mirror ApprovalCard's existing error/focus pattern: - Track failure state with terminal/message/attempted fields - Render ErrorNotice inline on rollback with the same i18n keys as ApprovalCard - Restore keyboard focus to the button matching the failed decision via useEffect - Reset failure state when permission props change Fixes kirodotdev#5554

bolichen97 · 2026-08-29T18:14:49Z

Full-diff/commit-containment audit (current head 1a043597d85102ea83192bab4685e49bca0624ef): the unique CollapsibleToolGroup.tsx change is a more complete alternative to #5562—both add ErrorNotice, rollback state, buttonsRef and attempted-action focus restore, while this version additionally distinguishes terminal 404-style ApiError outcomes from retryable failures. The two implementations should not both merge.

This PR also literally carries #5603 (287dfb792...) and #6031 (a288e5e86...) ahead of its own commit even though its declared base is main; #6072 then carries this entire chain again. Please rebase/retarget so #6065's review diff contains only its own dashboard commit, choose this implementation or #5562 as the single owner (preferably preserving this terminal-vs-retryable behavior), and make #6072 depend on the cleaned PR explicitly or drop the carried commit. The redaction and fork-review changes should remain in their own PRs, not in this scope.

NicholasRBowers · 2026-09-01T18:58:36Z

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been stale with failing CI. I reviewed the blockers but they require your input:

The PR mixes three unrelated change families: the CollapsibleToolGroup approval-rollback UI fix (the title's scope), a cron_script.py _stderr_tail redact-before-tail rework, and an edit to .github/workflows/fork-first-principles-review.yml. The workflow edit trips the Fork workflow-change guard — a hard block for fork PRs that requires human review. Dropping it from this PR is your call.
The cron_script.py change conflicts with main, which already landed the redact-before-the-tail-slice hardening (via fix(cron): report the terminal stderr, not the leading one #4402's fix series). Deciding whether your version is superseded or still adds value (e.g. the 4 MiB full-redact ceiling) is author intent.
The UI change overlaps competing PR fix(chat): surface collapsed approval failures #5562; the maintainer overlap audit (2026-08-29) notes this version is the more complete of the two. Reconciling with that PR is a scope decision.
The Fork PR Description check also fails (required template sections missing) — mechanical, but moot until scope is settled.

When you've addressed these, the pipeline will re-assess on its next cycle.

bolichen97

Description ↔ code mismatch: the change is not limited to `CollapsibleToolGroup.tsx`

Blocking finding from a Description-vs-code consistency audit. Criterion: the description says it only changes A, but the diff also changes B, and B affects risk and review scope — here B is a credential-redaction path.

The claim

Change is limited to website/src/pages/chat/CollapsibleToolGroup.tsx (39 additions, 3 deletions)

The Evidence section lists frontend typecheck only.

What the diff actually contains

website/src/pages/chat/CollapsibleToolGroup.tsx     +39/-3   <- the described change
.github/workflows/fork-first-principles-review.yml  +1/-1
src/kiro_crew/cron_script.py                        +31/-13
test/test_cron_script.py                            +71/-0
test/test_spec_builder_routes_coverage.py           +22/-1

Five files, +164/-18. The +39/-3 figure for the frontend file is accurate — the problem is the other four files, none of which is mentioned.

src/kiro_crew/cron_script.py changes credential-redaction behaviour:

adds _STDERR_FULL_REDACT_MAX = 4 * 1024 * 1024;
rewrites _stderr_tail to redact the whole buffer before tailing, instead of tailing then redacting;
adds a path where stderr larger than 4 MiB becomes [stderr omitted: too large to redact in full];
in run_script_sandboxed, replaces redact(stdout)[:200] with a new redact_and_truncate(stdout, 200).

The two test files add cases for exactly those redaction boundaries.

Why this blocks

The frontend change itself is fine and matches its own description — the failure state, ErrorNotice, focus restoration keyed on failure, and reset on props change are all present in CollapsibleToolGroup.tsx. The problem is that a reviewer given the contract "limited to one frontend file, 39 lines, frontend typecheck as evidence" will review that file and stop. The redaction reordering never gets security review, and the Evidence section offers nothing that would have exercised it.

Note the four non-frontend files are identical to the entire contents of #6031, whose body separately claims to be a one-line workflow edit. The same commit appears to have been picked up by both branches, so this is most likely an accidental carry rather than an intentional bundling.

Required change — pick one

Drop the unrelated commit so this PR is the frontend fix it says it is, and let the cron_script.py redaction change be reviewed on its own.
Or rewrite the description to list all five files and disclose the redaction behaviour change (redact-before-tail reordering, new redact_and_truncate and _STDERR_FULL_REDACT_MAX, >4 MiB withholding), with evidence that covers the backend change and not just frontend typecheck.

Not at issue: the merge conflict and the needs-author-decision label are process state, not a description mismatch.

_{Automated Description↔code consistency audit. Only these five classes are treated as blocking: described behaviour absent from code; undisclosed change to user-visible behaviour/API/permissions/security boundary; test-pass claims contradicted by evidence; undisclosed scope beyond what the description says; conflict with spec/docs/an invariant. Wording and style are never flagged.}

bolichen97 · 2026-09-02T20:09:19Z

Closing — every part of this is already on `main`

Verified relationship: already on main``

the first adjudication is upheld against current main (1a765b8). #6065 authored exactly one commit, 1a04359, touching only website/src/pages/chat/CollapsibleToolGroup.tsx (+39/-3). The decisive test is the blob diff in the main->PR direction: git diff origin/main:website/src/pages/chat/CollapsibleToolGroup.tsx pr/6065:website/src/pages/chat/CollapsibleToolGroup.tsx contains not one added line of behaviour. Every + line either DELETES a capability main has (the permissionMetas / onApproveBatch props, batchPreviews, the batch preview block, approve_all/reject_all i18n, and the !failure?.terminal button-row gate at main:244) or reflows a comment / reorders an import. The PR's file is a strict subset of main's. Everything the PR adds is on main from merged #5562 (commit f69cccb, ancestor of origin/main and the last commit to touch that file): the ErrorNotice/ApiError imports (:6-7), buttonsRef (:87), the identical failure: {terminal, message, attempted} state (:90), setFailure(null) in the props-change effect (:96-101) and at the top of submitDecision (:151), the byte-identical gone predicate and setFailure({terminal: gone, ...}) catch arm (:166-173), the focus-restore ladder (:182-189), ref={buttonsRef} (:245), and the three components.approvalCard.* keys (:252-257). Main additionally ships the tests for that behaviour (CollapsibleToolGroupCov80.test.tsx asserts the reject-focus ladder and the 404 / 400-"no pending approval" terminal cases); #6065 ships none.

Both the adversarial pass objections fail on the merits. (1) "Four of five files are absent from main, so partly on main": the absence is real, the ownership is not #6065's. git rev-parse pr/5603 = 287dfb7 (single-commit branch) and git rev-parse pr/6031 = a288e5e are literally the two commits under 1a04359, and one live REST call each confirms both PRs are state=open, merged=false with those exact heads. That is the stacked-branch artifact the brief names as the initial scan error class 4; both pair verdicts independently ruled a stacked branch / survivor BOTH, and the maintainer's 2026-08-29 audit asked for a rebase of the carry, not for #6065 to own it. (2) "The maintainer called #6065 'a more complete alternative to #5562' because it distinguishes terminal 404-style refusals from retryable ones, so that is unlanded": that comment predates #5562's merge, and the commit that landed carries the terminal/retryable split verbatim — the same gone predicate, the same terminal: gone, the same approval_no_longer_pending/decision_not_recorded_error/decision_failed ternary (none of which appears in the blob diff, i.e. identical) — plus the !failure?.terminal gate #6065 lacks. The maintainer's "preferably preserving this terminal-vs-retryable behavior" condition is satisfied by main.

Judgment written to the cached PR data 2 gh api REST calls used; git was read-only throughout.

Carry this over first

This closure is about redundancy, and these items are the exception: they are not on main and not in the surviving PR, so they need a home before the topic is finished. Please don't let them go with the branch.

Nothing #6065 authored is off main — verified part by part (A1-A7 above), and main is a strict superset (the !failure?.terminal button gate plus two frontend test files).

The four non-frontend files in its review diff DO contain content main lacks, but every hunk is a verbatim inherited commit belonging to another OPEN PR, not this PR's remainder:

Owned by OPEN fix(security): redact before slicing at every bounded-logging site #5603 (its entire and only commit, 287dfb7): _STDERR_FULL_REDACT_MAX = 4 * 1024 * 1024 and the McpToolClient._stderr_tail whole-capture-redact-then-slice rewrite with the [stderr omitted: too large to redact in full] marker and the +1-byte concurrent-writer re-check in src/kiro_crew/cron_script.py (main:510-527 still fh.seek(max(0, size - limit)) then return redact(fh.read().strip())) — this is the only genuinely new behaviour in the whole 5-file diff; the 3 new test_stderr_tail_* tests in test/test_cron_script.py; and class TestRedactTruncated in test/test_spec_builder_routes_coverage.py (extra tests for _redact_and_truncate, which already exists on main at src/kiro_crew/apps/builtins/spec_builder/backend/parsers.py:77).
Owned by OPEN fix(ci): stop suppressing stderr in fork first-principles review finalize helper #6031 (its entire own commit, a288e5e): dropping 2>&1 from the gh api --method PATCH .../check-runs finalize call in .github/workflows/fork-first-principles-review.yml (main:718 still >/dev/null 2>&1;).
Behaviourally already on main, only respelled: the redact_and_truncate import and the redact(stdout)[:200] -> redact_and_truncate(stdout, 200) swap at cron_script.py's Bad-output arm. security.py:13793's redact_and_truncate is redact_credentials(redact_exfiltration_urls(text)[0])[0][:max_chars] and security.py:12370's redact runs the same two passes in the same order, so main:906 already computes the identical value (landed by merged fix: redact cron stderr/stdout before truncating, report stderr tail #5574).

Live owner check: gh api repos/kirodotdev/KiroCrew/pulls/5603 -> open, merged=false, head=287dfb792; pulls/6031 -> open, merged=false, head=a288e5e86. Closing #6065 takes nothing away from either.

_{From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. Findings that implied a closure were re-adjudicated independently, including an adversarial pass whose only job was to refute them; the reasoning above is what survived. If it is wrong, reopening costs nothing — please say so, and treat the reasoning rather than the outcome as the thing to correct.}

aniruddhaadak80 added 3 commits August 26, 2026 13:10

aniruddhaadak80 requested a review from a team August 26, 2026 14:11

aniruddhaadak80 requested a review from a team as a code owner August 26, 2026 14:11

aniruddhaadak80 requested a review from krishdhasmana August 26, 2026 14:11

github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 26, 2026

This was referenced Aug 29, 2026

fix(chat): surface collapsed approval failures #5562

Merged

fix(security): redact before slicing at every bounded-logging site #5603

Open

fix(ci): stop suppressing stderr in fork first-principles review finalize helper #6031

Open

bolichen97 mentioned this pull request Aug 29, 2026

fix(security): add config.json and config.local.json to bash write-protect list #6072

Closed

3 tasks

bolichen97 enabled auto-merge August 30, 2026 00:00

NicholasRBowers added needs-pr-triage PR scanner: awaiting automated triage needs-author-decision PR blocked on author input and removed needs-pr-triage PR scanner: awaiting automated triage labels Sep 1, 2026

bolichen97 requested changes Sep 2, 2026

View reviewed changes

bolichen97 closed this Sep 2, 2026

auto-merge was automatically disabled September 2, 2026 20:09
Pull request was closed

github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 2, 2026

Navigation Menu

Uh oh!

fix(dashboard): show error and restore focus on CollapsibleToolGroup approval rollback - #6065

fix(dashboard): show error and restore focus on CollapsibleToolGroup approval rollback#6065
aniruddhaadak80 wants to merge 3 commits into
kirodotdev:mainfrom
aniruddhaadak80:fix/collapsible-tool-group-approval-rollback-error-focus

aniruddhaadak80 commented Aug 26, 2026

Uh oh!

bolichen97 commented Aug 29, 2026

Uh oh!

NicholasRBowers commented Sep 1, 2026

Uh oh!

bolichen97 left a comment

Uh oh!

bolichen97 commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aniruddhaadak80 commented Aug 26, 2026

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

bolichen97 commented Aug 29, 2026

Uh oh!

NicholasRBowers commented Sep 1, 2026

Uh oh!

bolichen97 left a comment

Choose a reason for hiding this comment

Description ↔ code mismatch: the change is not limited to CollapsibleToolGroup.tsx

The claim

What the diff actually contains

Why this blocks

Required change — pick one

Uh oh!

bolichen97 commented Sep 2, 2026

Closing — every part of this is already on main

Carry this over first

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Description ↔ code mismatch: the change is not limited to `CollapsibleToolGroup.tsx`

Closing — every part of this is already on `main`