feat(code-review): add guided review fix workflow

[atomsbaza]

Problem / Motivation

Code Review Sage previously exposed review context and findings, but it did not provide a complete governed path from local review feedback to safely applying fixes. Users could inspect findings, but could not select candidate fixes, group dependent changes, validate them, apply them, commit them, and request a re-review through one coherent workflow.

Why it matters

Without a guided workflow, applying review fixes requires manual worktree and Git coordination, making it easier to mix unrelated changes, apply an unsafe candidate, lose dependency ordering, or blur the boundaries between Apply, Commit, Push, and Review again. This feature makes those transitions explicit and gives users deterministic validation and evidence for the changes they accept.

What changed (motivation → approach → change)

The implementation adds a local-review → feedback → Review Fix lifecycle while keeping candidate changes isolated and state transitions governed:

• Added local review context, lifecycle handling, Review Fix task routes, candidate worktree coordination, dependency grouping, CAS state transitions, model resolution/pinning, and validation before Apply.
• Kept Apply, Commit, Push, and Review-again as separate boundaries, with conflict, retry, and re-plan handling.
• Added deterministic fake ACP edit behavior and safety restrictions for repeatable tests.
• Added the Code Review Sage local-review view, Review Fix setup and task panel, model picker, diff/markdown selection support, stable Monaco layout handling, API/types, translations, and focused UI coverage.
• Updated Sage documentation and committed redacted UI evidence under temp-screenshots/interactive-code-review/.
• Ops Mission Control and Redirects E2E gate repair are deliberately excluded from this PR and will be handled in a separate PR.

Tests

• Backend targeted tests: 65 passed, covering local review, fake ACP, Git coordination, Review Fix routes, and task-runner behavior.
• Frontend targeted Vitest: 7 files / 56 tests passed, covering Sage model API/context, Review Fix UI, model picker, PR review detail, PR pick list, and run detail.
• Focused mypy on the changed Python source scope: Success: no issues found in 15 source files with unrelated imported platform modules excluded.
• npm run i18n:check: passed.
• py_compile, isort, and flake8: passed.
• ESLint on the replacement Sage/Review Fix scope: 0 errors and 15 accessibility warnings.
• tsc -b: could not complete locally because all available existing dependency trees lack graphology-layout-forceatlas2 and graphology-communities-louvain; no changed-file TypeScript errors were reached.
• git diff --check: passed for source/test files; committed PNG evidence is valid binary image data.

Manual verification

Performed against an isolated Gateway, temporary repository, and fake ACP backend (isolated evidence only; this is not production integration verification):

1. Created a review finding and grouped dependent fixes.
2. Started execution and observed the awaiting-validation state.
3. Ran validation, then applied the candidate.
4. Observed the awaiting-commit state and committed the accepted group.
5. Confirmed the target file changed as expected, the target repository was clean, and the target HEAD matched the expected commit.

Screenshots / video

Additional redacted evidence

Related Issues

No linked issue: this feature has no tracked issue reference.

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

N/A — repository template placeholder remains unchanged.

Update 2026-08-29 (CI fixes + rebase)

• Rebased onto current main (was behind by 10 commits; the locale catalogs, security_posture.py, and website/src/api/client.ts overlaps are pre-resolved) and squashed to this single commit.
• Backend Tests (3.12, 1) failure fixed: test_cli_logging.py::TestDrainBeforeHardExit::test_no_listener_is_a_silent_no_op failed under CI's shard ordering because an earlier test can leak a live _LOG_QUEUE_LISTENER (the fixture cleared handlers but not the listener, and the short-lived-command setup path never retires one). The _pristine_logging fixture now stops the listener in setup as well. Reproduced deterministically with a leaking-test file before the fix; test/test_cli_logging.py passes with the leaker and alone.
• Bundle Size Gate fixed: the all i18n catalog chunk re-measures at 9815 KB with this PR's ~130 translated keys per catalog (added on top of main's own Dev Fleet increment that set the 9800 KB ceiling). The catalogs are eager by design (see src/i18n/index.ts's lazy-loading seam, reserved for a future catalog feat: unify artifacts library UI + session-origin Source column #13), so the growth is irreducible here — CHUNK_BUDGETS.all moves to 10350 KB (measured + ~5% headroom, per the file's convention).
• Re-validated on the merged tree: 88 backend tests across the PR's Review Fix / git-coordination / fake-ACP / local-review files; test_cli_logging.py (34); focused mypy --platform linux on all 14 changed Python sources (clean); isort + flake8 on src/ and test/ (clean); npm run i18n:check (pass); targeted Vitest (6 files / 53 tests); bundle-size gate (734 chunks within budget).

Update 2026-08-31 (review findings addressed + rebase)

• Rebased onto current main (twice: first 66 commits behind, then again 356 behind on 2026-08-31 — taskrunner's persisted-run restore gained main's worktree_path alongside the review_fix metadata; the all chunk re-measured byte-identical at 10066 KB on the merged tree). First conflict: the CHUNK_BUDGETS.all ceiling — re-measured 10066 KB on the merged tree (main's own 9985 KB plus this PR's Sage Review Fix strings, now that the dead-surface catalog keys are removed), ceiling 10570 KB (~5% headroom per the file's convention).

GPT 5.6 Review — all blocking findings fixed

1. Unsupported model kwarg — removed from the run_review dispatch; the model binds at pool.begin_batch(model=…) as designed. The regression test's stub carries the driver's real signature, so a reintroduced kwarg raises the same TypeError.
2. Windows os.fchmod crash — save_session now delegates to atomic_write(path, content, mode=0o600); the hand-rolled temp-file/fchmod/replace block is gone.
3. Apply/Push not bound to approved snapshots — Apply compares the live candidate patch id against the validated candidate_patch_id and rejects drift (re-capture and re-validate); Push recomputes the preview and rejects a stale signature (remote/branch/upstream/commits/files/diverged) before contacting the remote.
4. Failed starts persisting lifecycle state — execute_review_fix rolls the state back (start_rolled_back) when the post-transition start fails and the state is still RUNNING; review_again restores PUSHED the same way (fresh-read, only if unchanged).
5. new_worktree inert mode — removed (enum variant, setup option, catalog keys). CURRENT_BRANCH is the only target mode.
6. Synchronous filesystem work on the loop — the local-session glob and the validation-artifact/patch writes are offloaded via asyncio.to_thread (byte-exact newline="\n" preserved for patches).
7. Two-button row cap — excess selection actions moved into an overflow menu; Discard and Push additionally sit behind the shared confirm dialog.
8. auto_approve provenance bypass — the resume/retry action routes through the dashboard's _gate_auto_approve (deny = untrusted run, critical SEL audit), matching the core execute path.
9. Group-ID path traversal — group ids must match ^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$ at plan time, and artifact roots must be non-link and contained in the candidate worktree.
10. (advisory finding) Local Fix direct work_dir — kept deliberately for now: it is the quick local-review path, while the governed candidate → validate → apply → commit lifecycle is the Review Fix task flow. Fix-run outcome (status/error/changed files) is now rendered and polled so a failed fix is no longer invisible; happy to convert Local Fix into a Review Fix task in a follow-up if maintainers prefer.

Opus 4.8 Review — both blocking findings fixed

• discard_candidate bricking — the CAS transition to DONE now happens BEFORE git worktree remove --force; RUNNING is rejected as a discard source; a failed destroy logs into the task logs without stranding the task. The finding's repro (discard from awaiting_commit) is now the regression test.
• Windows fchmod — same atomic_write fix as above.

First Principles Review (BLOCK) — inert surface deleted

• Six zero-writer states removed with their transition rows: APPLIED, READY_TO_REREVIEW, BLOCKED_DEPENDENCY, BLOCKED_TARGET_CHANGED, BLOCKED_CONFLICT, BLOCKED_MANUAL_RESOLUTION — plus the recheck_continue action, the reevaluate_grouping button, the replan_preview/confirm_replan log-only actions, and ReviewFixChatLink. (BLOCKED_VALIDATION stays — validation writes it; recovery runs through retry/resume. ReviewFixGroupState.APPLIED is a different, live enum.)
• Subtractions: the four zero-consumer aliases in task_models.py; the include_taskrunner parameter (the dashboard adapter owns the taskrunner-scoped routes); ReviewFixValidationRun.output_path/error (zero writers); the hand-rolled temp write (replaced by atomic_write).
• The two declared riders from the previous round (SelectionToolbar scroll-dismiss, glossary DNT 1→0) remain, unchanged.

Design Review (CONCERNS)

• Spec lands in the same commit: docs/system-specs/modules/taskrunner.md now documents the review-fix lifecycle — final state set, ReviewFixMetadata fields, the CAS contract, the transition table, model pinning, artifact-safety rules, and route ownership.
• resolve_pinned_model allows an empty advertised set (repo-wide convention); auto is still rejected so a fix task keeps one concrete model across retries.
• Route ownership made explicit — register_fix_task_routes registers only the app-scoped endpoints; the taskrunner-scoped routes belong to dashboard/routes/taskrunner.py via its adapter. Extracting the generic adapter out of the app is left as a follow-up discussion.
• Product identity (version bump, "PR review as optional secondary surface", gh dependency drop) — these deserve explicit maintainer sign-off; happy to split them into a separate PR if preferred.

UX Review (CONCERNS)

• Addressed: fix-run outcome rendered and polled; real confirm dialogs for Discard/Push that name the action; Discard hidden while running; overflow menu for excess actions; failed-fix errors surfaced instead of disappearing.
• Deferred as follow-ups: plural _one forms, task-panel vocabulary, model-precedence labeling, store copy.

Re-validation on the merged tree

• Backend: 843 targeted tests (Sage + review-fix suites) green, including new regression tests for patch-drift rejection, stale-push-preview, discard-from-awaiting_commit, destroy-failure, failed-start rollback, group-id rejection, auto-approve denial, and empty-advertised-set allow.
• mypy --platform linux src/kiro_crew clean; black / isort / flake8 / subprocess-encoding / loop-bound locks / brand / harness-parity / docs-lint all clean.
• Frontend: tsc -b clean; 154 targeted vitest tests green; full npm run test (jscpd + vitest + Electron) green; ESLint 0 errors on changed files.
• i18n: dead keys removed from all 13 catalogs, en-XA regenerated and matched; npm run i18n:check delta vs the previous head introduces zero new findings.
• Bundle: 735 chunks within budget with the re-measured ceiling.

Update 2026-09-01 (CI fixes: boundary gate + host-sandbox-dependent tests)

• Merged current main (aa5f5a6) — resolved the conflict in agent_sdk/__init__.py by combining main's turn-usage vocabulary (AgentTurnUsage, TURN_STOP_REASON_*) with this round's lazy model-selection re-exports.
• Backend Lint & Type Check (3.10 + 3.12) fixed: the agent-SDK boundary gate flagged 3 new offenders (fix_tasks.py, routes.py, review_fix.py) importing kiro_crew.acp.client directly, plus review_pool.py growing 2→3 ACP edges. advertised_model_ids / model_is_unusable / AcpModelUnavailable are now re-exported from kiro_crew.agent_sdk (the one layer allowed to import the ACP package) and all four files read them from the facade. The re-exports resolve lazily via module __getattr__ + TYPE_CHECKING: a module-scope ACP import in agent_sdk/__init__ would put the ACP client + runtime on the gateway boot path (see test_the_boot_path_does_not_import_acp_at_module_scope). review_pool.py's acp.client edge is removed outright; the baseline is untouched.
• Backend Tests shard 3 (3.10 / 3.12 / Windows) fixed: the test_review_fix_actions_coverage.py tests at lines 263 and 415 drive real git through git_coord._git, whose async chokepoint (sandboxed_spawn_argv_async) probes the host sandbox and fails closed on CI runners without one (Ubuntu denies unprivileged user namespaces; Windows has none). The unsandboxed_git fixture in review_fix_helpers.py now patches the async half alongside the sync prepare half, and the coverage module imports the fixture so sharded collection registers it. Test-only change; the real git subprocess behavior stays under test.
• Automated Rule Check fixed: the "no hardcoded model id" rule tripped on the "claude-sonnet-4" literal in test_backend_routes.py (TestSelectedModelGoesToThePoolOnly). Replaced with a "model-under-test" sentinel — the stub pool validates nothing and the test pins kwarg passthrough, not a real id.
• PR Hygiene commit count: the fix round initially produced 4 commits (the gate allows at most 2), so the branch was re-squashed to exactly 2 commits with a tree byte-identical to the previously validated head. Backend Tests (3.10, 1) from the previous round was confirmed to be the uv-setup infra flake — it passed on re-run without changes.
• Local validation on this head: boundary / subprocess-encoding / sync-IO / black gates clean at origin/main...HEAD scope; isort + flake8 clean on all touched files; mypy src/kiro_crew clean (1,226 files); 348 targeted tests green (review-fix suites, git coordination, agent-SDK gate self-tests, Sage backend routes, review pool, fix routes).
• CI on this head: CI run 33467217540 ✅ (all shards + Coverage Gate), Code Review 33467217865 ✅, Build 33467217560 ✅, PR Readiness ✅.

Update 2026-09-01 (AI review round 2: GPT 5.6 + Opus 4.8 blockers closed)

Rebase: mergeable:false resolved — branch rebased onto main (6715282) and re-squashed to 1 commit (PR Hygiene cap 2). The security_posture.py conflict was resolved by keeping upstream's dev-fleet module split and this PR's sage_lib/local_review.py non-egress entry (dropping the superseded dev_fleet/server.py line).

GPT 5.6 (round 2)

BLOCKING — review_fix.py:120 fileless groups can apply unowned/unvalidated bytes (fixed, 3 layers, fail-closed):

• Plan: build_review_fix_groups raises ReviewFixPlanError(code="fileless_finding") when a finding has no file_path (auto groups), and code="fileless_group" when a raw group ends up with an empty affected_files set.
• Capture: capture_group_patch refuses a group with no owned files — an unscoped capture would have hashed a whole-worktree diff as the group's patch.
• Apply: fix_tasks._apply_group now requires a captured candidate_patch_id before the id comparison; the old if group.candidate_patch_id and … truthy-check let an uncaptured group ("") through into the apply-vs-commit split state. The "first apply binds the id" fallback is removed — apply verifies the id, never overwrites it.
• Tests: fileless finding → plan error; fileless group → plan/capture error; uncaptured group → apply rejected; captured-then-mutated → mismatch rejected (existing test preserved).

BLOCKING — review_fix.py:169 all fix tasks run concurrently in one worktree (fixed): build_review_fix_tasks now consumes its groups parameter and builds serialize-per-resource depends_on chains — a task depends on the previous task owning the same file or belonging to the same dependency group; unrelated findings still run in parallel. Edges point strictly backwards, so the graph is acyclic by construction and task_planner's forward-only normalization (0 < dep < index, cross-group expansion) accepts it unchanged.

• Tests: different files → no deps; same file → chained; one group across files → chained; 3-same-file chain → 1→3→4.

BLOCKING — local_review.py:348 large synchronous reads on the loop (fixed, both anchors):

• (a) build_context bounds the AGENTS.md/CONTRIBUTING.md read at disk (open("rb").read(8192) → decode → [:4000]), so a huge guidance file is no longer pulled fully into memory; and the routes.py call site builds the prompt via await asyncio.to_thread(_local_prompt, diff) (mirroring the existing working_tree_diff offload above it).
• (b) review_fix_git.apply_patch's post-apply conflict-marker scan moved into a sync helper offloaded with asyncio.to_thread. Public signatures unchanged.
• Tests: 100 KB AGENTS.md still yields an exact 4,000-char GUIDANCE block; marker detection covered by test_git_coord_review_fix.py (which passes unchanged).

FINDING — review_pool.py:365 return rt hands a live runtime to a different model (fixed, fail-closed): the fast path now raises RuntimeError("review pool is busy with model 'X'; retry after the batch drains") when an explicit model differs from the live runtime's model. Same-model and Auto (None) requests keep pooling exactly as before, and a failed mismatch does not consume a batch count.

• Tests: mismatch → raise, runtime untouched, batch count unchanged; same/Auto → reuse + clean drain; new model after drain → fresh runtime.

FINDING (function-local imports) — push-back: the three deferred imports are deliberate, each inside an existing rule exception:

• agent_sdk/__init__.py:128 sits in the module __getattr__ lazy resolver: a module-scope kiro_crew.acp import would drag the ACP client and runtime onto the gateway boot path — exactly what test_the_boot_path_does_not_import_acp_at_module_scope forbids (AST-parsed at source level). This is the boot-path deferral the rule's exceptions are written for, not an accidental function-local import.
• fix_tasks.py:140 builds the hyphenated sage_lib sys.path entry at call time (the directory is not importable top-level); deferring keeps import cost off the route-table path.
• task_executor.py:887 avoids ordering surprises in the suite's import graph (same pattern as the neighboring dashboard.handlers.usage import, which carries an explicit # circular import note). We verified review_fix_git's full top-level import closure does not reach task_executor, so if reviewers prefer, hoisting this one import is safe and we're happy to do it in a follow-up — it just didn't belong in this hardening round. The rule is blocking:false, so this is left as a documented advisory.

Opus 4.8 (round 2)

BLOCKING — local_review.py:332 category/reviewer bypass store.redact_text (fixed): category and reviewer now go through store.redact_text like title/message/suggestion (backend-security-controls: never trust LLM output — scrub before the dashboard). The category is redacted once, before the fingerprint is computed, and the same variable feeds both the fingerprint and the constructor — dedup keys can never diverge from stored fields.

• Tests: AKIA-prefixed credentials in category/reviewer are absent from the stored finding; stored category equals redact_text(raw).

Local gates on this head (post-rebase, becaa06)

• scripts/check_agent_sdk_boundary.py: passed (baseline unchanged — shrink-only respected).
• pytest: test_review_fix.py + test_review_fix_actions_coverage.py 35 passed; test_local_review.py + test_review_pool.py + test_backend_routes.py 259 passed; review-fix routes/git/taskrunner/dashboard suites 244 passed.
• vitest: CodeReviewSageReportView.test.tsx 12 passed (2 new: single-selection keeps the dedicated Fix button; both-selected moves Fix into the overflow and it still fires), Review Fix UI + Local Review + Sage detail suites 82 passed.
• tsc -b: clean. ESLint on the touched TSX: clean. No literal model ids added to tests (model-under-test sentinels only).

---

Update — round-2 residuals fixed (8e63ec9)

All six round-2 findings (5 BLOCKING + 1 FINDING) plus both Opus 4.8 FINDINGS are fixed on this head, disposition per anchor class:

pathspec/path-encoding — review_fix_git.py (BLOCKING #1 + Opus O2): every consume point (candidate_patch, stage_paths, commit_group) wraps owned paths as :(literal) pathspecs — pathspec magic in a crafted filename can no longer widen a diff/stage/commit beyond the task's owned paths. git status is now parsed from --porcelain=v1 -z --no-renames (NUL-split), so non-ASCII filenames reach dirty_overlap byte-exact instead of C-quoted (core.quotePath).

• Tests: test_pathspec_magic_in_a_owned_path_captures_nothing_extra, test_non_ascii_paths_are_tracked_and_overlapped_raw.

validation-evidence — review_fix.py:455 + task_executor.py (BLOCKING #2): the runner's skip output is now the exported sentinel TESTS_SKIPPED_OUTPUT; validate_group refuses to credit it as a pass, so a command that never ran can no longer mint a passing validation record (exit_code=0). The generic runner's skip semantics are untouched.

• Test: test_validate_group_treats_missing_test_command_as_failed_validation.

phase-machine — review_fix.py:483 (BLOCKING #3): the task advances out of AWAITING_VALIDATION only when every group has finished its validation phase. Completion is read per group — a PROPOSED group counts as finished only with recorded validation runs, which distinguishes "never validated" from "validated and failed" — and one failed group blocks the task even when the last group to finish passed. Revalidation from BLOCKED_VALIDATION still works.

• Tests: test_first_group_validation_keeps_task_awaiting_for_sibling, test_last_group_out_of_order_fail_blocks_after_sibling_passed, test_all_groups_passed_then_last_advances_ready_to_apply, test_failed_group_then_sibling_pass_blocks_validation.

local-fix-concurrency — routes.py (BLOCKING #4): direct-fix requests register the repository under _LOCAL_LOCK before create_task; a racing request gets 409 fix_in_progress instead of a second agent editing the same checkout. The slot is released in _local_fix_bg's finally on every exit path (the pre-run session save moved inside the try so a raise there cannot leak the slot).

• Tests: test_a_second_fix_for_the_same_repo_is_rejected_409, test_the_registry_slot_is_released_after_the_run, test_the_registry_slot_is_released_after_a_failure.

pool-model — review_pool.py:371 (FINDING #6 + Opus O1): the live-runtime reuse guard is strict model equality — any disagreement (pinned-vs-pinned or Auto-vs-pinned) fails closed with the busy error instead of silently running a review under the wrong model; only an exact match (including None == None) reuses.

• Tests: test_auto_on_pinned_runtime_fails_closed, test_pinned_on_auto_runtime_fails_closed, test_explicit_model_mismatch_on_live_runtime_fails_closed, test_same_or_auto_model_reuses_live_runtime.

ui-action-cap — ReviewFixTaskPanel.tsx (BLOCKING #5): preview is offered from the moment a group is committed (the diff is final — the action block used to vanish for the whole task-committed window), Push stays push-phase-only, and Refresh diff lives in the overflow menu — every state renders at most two controls per action row (max-two-buttons-per-row).

• Tests: shows preview without push while the task is committed and keeps refresh in overflow, shows exactly preview and push during awaiting_push with refresh in overflow, hides the push affordances until a group is committed.

Local gates on this head (8e63ec9)

• pytest: review-fix + git-coord + sage suites 382 passed (test_review_fix.py incl. the 4 multi-group state-machine cases, test_git_coord_review_fix.py, test_review_fix_routes.py, test_review_fix_actions_coverage.py, test_review_pool.py, test_backend_routes.py, test_local_review.py).
• vitest: ReviewFixUi.test.tsx 40 passed. tsc -b: clean. scripts/check_agent_sdk_boundary.py: passed (shrink-only baseline respected). i18n key gate: passed (more_actions present in all 14 locales).
• No literal model ids in tests (model-under-test sentinels only).

Commit shape: history rewrite is disabled in this environment, so instead of amending into 8ed8f3db4 the residuals land as a plain fast-forward third commit fix(code-review): close residual review-hardening gaps (round 2) on top of 2e14007ff — same content, additive history.

Update — cleanup: squashed to 2 commits, black, census drop (7c86e02)

Mechanical follow-up to the round-2 head 8e63ec9bd; content unchanged except the three items below.

• PR Hygiene (commit cap): branch rebuilt as exactly 2 commits — ab434e8e2 feat(code-review): add guided review fix workflow (round-1 feature + round-2 residuals squashed) and 7c86e023f fix(ci): drop the memory.py log-site census after #7542 (re-measure → drop the stale entry). Tip tree differs from 8e63ec9bd only by the two items below.
• black (baselined gate): the 4 files it flagged as new offenders are reformatted with --target-version py310 — src/kiro_crew/review_fix.py, src/kiro_crew/review_fix_git.py, test/test_git_coord_review_fix.py, test/test_review_fix.py. Whitespace-only.
• census: upstream refactor(channels): delete the two dead outbound-upload surfaces #7542 converted dashboard/handlers/memory.py's two pip-stderr sites to redact_log_via_context, so on the merge ref the module has 0 baseline sites and the recorded 2 was pure slack (test_the_census_holds_no_slack: memory.py: 0 sites, census says 2). The dashboard/handlers/memory.py entry is dropped from _BASELINE_LOG_SITE_CENSUS per the emptied-module rule; no source change.

Gates on this head: black --check --target-version py310 on the 4 files → clean; pytest test_review_fix.py + test_git_coord_review_fix.py 32 passed, wider review-fix suites 114 passed, backend-routes coverage 266 passed; vitest ReviewFixUi + SagePrReviewDetailModel 47 passed; check_agent_sdk_boundary.py passed. test_security_posture.py is 46/47 on this stale pre-#7542 checkout — the one local failure (test_no_new_gate_side_log_line_reads_the_baseline_redactor) counts the two now-gone sites in the local memory.py; both census tests pass against the upstream-merged tree CI tests (scanner run over upstream's converted memory.py: 0 sites).

[dwu96]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[github-actions]

UX Review (Fable 5, fork) — ⏭️ skipped

Revision a6b026c3b432d9456bd813e5264c33a03ffef36f touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[github-actions]

Design Review (Fable 5, fork) — ⚠️ could not complete

The design review did not produce a verdict for cc307677e09d7995fd719d1996243cfa6777309a (the review step never ran, because an earlier step in this job failed — no model call was made). See the Fork Design Review job logs. Advisory — does not block merge.

[github-actions]

First Principles Review (Fable 5, fork) — ⚠️ could not complete

The first-principles review did not produce a verdict for cc307677e09d7995fd719d1996243cfa6777309a (the review step never ran, because an earlier step in this job failed — no model call was made). See the Fork First Principles Review job logs. Advisory — does not block merge.

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 51e9163b4342f1b801c0b65a2d6b786d12bf22b9 via the fork AI-review pipeline; updated in place on each push.

3 of 3 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/apps/builtins/code_review_sage/backend/fix_tasks.py:458 -- Git mutations occur before the CAS transition

await review_fix_git.apply_patch(current_target, patch) / commit_sha = await review_fix_git.commit_group(...)
Concurrent group actions at one revision -> both mutate Git before mutate_review_fix -> the losing request returns stale while its changes or staging remain unrecorded.
Anchor: residual/crash-data-loss-corruption
Fix: Serialize context validation, Git mutation, and state persistence per task.

BLOCKING -- src/kiro_crew/apps/builtins/code_review_sage/sage_lib/local_review.py:297 -- Output limits are enforced after unbounded capture

diff_text = _run_git(repo, *args) / patch_text = await _git_text(candidate_path, *diff_args) / passed, output = await run_tests(...)
A large generated diff or continuously noisy validation command -> local review, patch capture, or validation -> the gateway buffers complete output and is OOM-killed before truncation.
Anchor: residual/crash-data-loss-corruption
Fix: Stream each subprocess through a hard byte limit, rejecting or discarding excess before buffering.

BLOCKING -- src/kiro_crew/review_fix.py:155 -- Soft grouping permits overlapping file ownership (origin: validation)

affected = sorted(set(files) | {str(value) for value in raw.get("affected_files", [])})
Same-file findings split across soft groups -> edit_soft_grouping -> the first group’s full-file patch includes sibling edits -> Apply lands changes the user has not approved for that group.
Anchor: residual/crash-data-loss-corruption
Fix: Reject or coalesce groups when an affected file appears in more than one group.

[BLOCK-MERGE] 51e9163
[GPT-REVIEWED] 51e9163

[github-actions]

Opus 4.8 Review (fork) — ⚠️ review incomplete

Reviewed cc307677e09d7995fd719d1996243cfa6777309a via the fork AI-review pipeline; updated in place on each push.

No completed Opus verdict for this commit; see the Fork Opus 4.8 Review job logs.

[atomsbaza]

Re: FINDING — function-local imports (agent_sdk/__init__.py:128, fix_tasks.py:140, task_executor.py:887)

Thanks for the detailed pass. We're keeping the deferred imports for now, with reasoning per site:

1. agent_sdk/__init__.py:128 — this import sits inside the module __getattr__ lazy resolver, and the deferral is a hard boot-path constraint, not a style miss: kiro_crew.acp.__init__ drags in both the ACP client and the runtime, and this package is imported on the gateway's route-table boot path. A module-scope import would put both on every gateway start — exactly what test_the_boot_path_does_not_import_acp_at_module_scope enforces (it AST-parses the source precisely because the forbidden thing is the import statement's position). This matches the rule's "optional dependency / deliberate deferral" exception family; the file's comment block cites the test and RFC (docs/request-for-change/rfc-crew-agent-sdk-boundary.md §5.5, model-id vocabulary is driver-owned).

2. fix_tasks.py:140 — builds the sys.path entry for the hyphenated sage_lib directory at call time; a top-level insert would run on module import even when the adapter is never exercised, and the directory is not importable as a module at top level anyway. Deferred on purpose.

3. task_executor.py:887 — we verified this one concretely: review_fix_git's entire top-level import closure (git_coord, security, task_models, sel, executors, atomic_write, …) never reaches task_executor, so hoisting would not raise in practice, and it mirrors the neighboring dashboard.handlers.usage deferral in the same function (which carries an explicit # circular import note for the suite's import-order sensitivity). We kept it for consistency in this hardening round, but this is the one site we'd concede is hoistable — happy to do so in a follow-up if you'd like.

The rule itself is blocking: false in AUTOSDE.yaml, so this FINDING is advisory. We'd suggest sites 1–2 are correctly deferred per the documented exceptions; for site 3, say the word and we'll hoist it.

[atomsbaza]

CI status on d1296b5ab — the 4 red checks are pre-existing failures on current main, not from this PR.

All 3 Backend Tests (3.10/3.12/Windows) (3) failures are the same single test: test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor — it finds dashboard/handlers/memory.py: 2 sites, census says 0. That file and its 2 redact_and_truncate warning sites arrived on main with #7279/#7283 (commit 2e627dc10, merged Aug 31), after this PR's branch point but before the rebase; the census in the test was never raised. Coverage Gate fails only because it requires all backend-test shards to pass (fail-closed by design).

Proof it is not ours: the identical test fails identically in a clean worktree checked out at upstream/main (6715282) — the PR's rebase base — and main's own CI run 33485506788 (commit 0df1ffb1) is red with exactly the same 4 jobs: Backend Tests (Windows) (3), Backend Tests (3.10, 3), Backend Tests (3.12, 3), Coverage Gate. We cannot rerun the failed jobs (gh run rerun requires admin on kirodotdev/KiroCrew — maintainer action), and a fix belongs in a separate PR since it re-measures a main-owned census: raise "dashboard/handlers/memory.py" to 2 in _BASELINE_LOG_SITE_CENSUS (or convert those 2 sites to redact_log_via_context), following the pattern of #7492 which did the same for files.py.

Everything attributable to this diff is green: 53/53 other checks pass — Automated Rule Check ✅, PR Hygiene ✅, Backend Lint & Type Check (3.10 + 3.12) ✅ (includes the black baseline gate), all Frontend Tests (4 shards) ✅, Frontend Coverage Merge ✅, Publish readiness ✅. The local AI-review blocker work is complete and verified; the two red-gate blocks above are main's to fix.

[atomsbaza]

Residual register — GPT 5.6 round 2 (5 BLOCKING + 1 FINDING) + Opus 4.8 round 2 (2 FINDING)

All eight findings were verified in code at 2e14007ff and are fixed-with-test on this head (8e63ec9bd). Disposition by anchor class:

Anchor class	Finding	Disposition	Test
pathspec/path-encoding	GPT #1 review_fix_git.py:60 pathspec magic in owned paths	fixed — :(literal) wrapping at all consume points (candidate_patch / stage_paths / commit_group)	test_pathspec_magic_in_a_owned_path_captures_nothing_extra
pathspec/path-encoding	Opus O2 review_fix_git.py:106 porcelain without -z (quotePath breaks non-ASCII)	fixed — --porcelain=v1 -z --no-renames, NUL-split parser	test_non_ascii_paths_are_tracked_and_overlapped_raw
validation-evidence	GPT #2 review_fix.py:455 command-not-found counts as pass	fixed — TESTS_SKIPPED_OUTPUT sentinel; skip never credits validation	test_validate_group_treats_missing_test_command_as_failed_validation
phase-machine	GPT #3 review_fix.py:487 first group's validation advances the whole task	fixed — advance only when every group finished its validation phase; sibling outcome gates READY vs BLOCKED; PROPOSED counts as done only with recorded runs	test_first_group_validation_keeps_task_awaiting_for_sibling, test_last_group_out_of_order_fail_blocks_after_sibling_passed, test_all_groups_passed_then_last_advances_ready_to_apply, test_failed_group_then_sibling_pass_blocks_validation
local-fix-concurrency	GPT #4 routes.py:2693 direct-fix path bypasses _LOCAL_LOCK (TOCTOU)	fixed — _ACTIVE_FIX_REPOS check-and-register under the lock before create_task; loser gets 409 fix_in_progress; slot released in finally (save moved inside try so a raise cannot leak the slot)	test_a_second_fix_for_the_same_repo_is_rejected_409, test_the_registry_slot_is_released_after_the_run, test_the_registry_slot_is_released_after_a_failure
pool-model	GPT #6 + Opus O1 review_pool.py:371 truthy guard lets Auto reuse a pinned runtime	fixed — strict model equality; any mismatch fails closed with the busy error; only exact match (None == None included) reuses	test_auto_on_pinned_runtime_fails_closed, test_pinned_on_auto_runtime_fails_closed, test_explicit_model_mismatch_on_live_runtime_fails_closed, test_same_or_auto_model_reuses_live_runtime
ui-action-cap	GPT #5 ReviewFixTaskPanel.tsx:321 3 buttons in awaiting_push; preview lost when committed	fixed — preview from group-committed, push only in push phase, Refresh diff in the overflow menu; ≤2 controls per row in every state	vitest: shows preview without push while the task is committed and keeps refresh in overflow, shows exactly preview and push during awaiting_push with refresh in overflow, hides the push affordances until a group is committed

Per the loop-stop rule: these are the registered residual anchors from round 1's fixes, all now closed at the root. If a re-review raises a BLOCKING against one of these anchor classes without a new root cause, the register above is the intended answer rather than a third code round; a genuinely new root cause will of course be fixed.

Gates on this head: 382 pytest passed (review-fix, git-coord, sage routes/pool/local-review), 40 vitest passed, tsc -b clean, check_agent_sdk_boundary.py passed (shrink-only), i18n key gate passed. Commit shape: additive third commit (fast-forward; history rewrite unavailable in this environment), fix(code-review): close residual review-hardening gaps (round 2).

[atomsbaza]

Rebased onto current main + round-3 fix — head 4f53bd646 — requesting workflow approval

Rebase: 222 commits, one conflict (agent_sdk/__init__.py __all__, kept both sides).

The re-review on 711596526e raised 5 new anchors; all were verified in code and are fixed-with-test on this head:

Anchor	Finding	Fix	Test
GPT B1 + Opus B1 (routes.py _local_fix_bg)	pre-turn CAS revision-mismatch return sits inside the try; terminal save is after the finally → fix persists "running", findings stuck "fixing" (preserved by reconcile_findings)	save moved INTO the finally (after the slot release) + the mismatch path resets "fixing"→"open" exactly like the except handler	test_a_revision_mismatch_persists_the_terminal_state
GPT B2 (routes.py:501)	explicit-model run pins the batch but dispatches with an unbound wrapper → Auto acquire on a pinned batch → busy error on first task	same shape as _post_comments_bg: base_dispatch + model_pinned, wrapper forwards model if model else model_pinned	TestRunReviewBgModelBinding (explicit run binds "m1"; no-model run passes None)
GPT B4 (review_fix.py:130)	default grouping makes one group per finding; each group's patch is whole-file, so the first applied group lands its sibling's unapproved edits	default branch co-locates findings per file_path (sequential ids by first appearance; explicit raw_groups branch untouched)	test_default_groups_own_a_file_once_so_patches_cannot_overlap
GPT F5 (review_fix.py:159)	bool(raw.get("hard", False)) turns JSON "false" into a hard lock	hard=raw.get("hard") is True	test_hard_flag_locks_only_on_a_json_boolean
GPT B3 (routes.py:2766) local fixes bypass candidate/Apply	push-back — the live-checkout local fix is the feature contract of Local Review, not an oversight: it is a user-initiated action behind require_enabled, the _LOCAL_LOCK + _ACTIVE_FIX_REPOS single-flight, pre- and post-turn CAS revision checks, an untrusted-data-framed prompt, and the standard PreToolUse deny rules — the same trust level as the user editing their own working tree. Routing it through the candidate worktree would remove the feature (instant local fixes) rather than close a gap. Happy to continue this discussion; if a maintainer still wants it converged, that is a product decision we'll implement.

Gates on this head: 381 pytest passed (sage routes, review_fix, routes coverage), flake8 src/kiro_crew clean, mypy src/kiro_crew/ clean (1287 files).

Per the loop-stop rule this closes every raised anchor at the root. Force-pushed to the existing branch — requesting a re-run of the review workflows on 4f53bd646.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #4085 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4085: REBASE. Independently valuable and not duplicative, but ordering matters: whichever lands second must re-run the glyph sweep over the other's new call sites, so the two authors should agree on the order. Files: website/src/apps/code-review-sage/components/ReportView.tsx.
• This PR is OVERLAPPING with PR #6802. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5274: REBASE. Different goals on the same admission surface. If PR #6802 lands, PR #5274's local-review and local-fix entry points need the same _admit treatment, otherwise the app-disable guarantee has a new bypass; if PR #5274 lands first, PR #6802 must widen its gate to cover the new endpoints. Files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py.
• This PR is OVERLAPPING with PR #8143. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5274: REBASE. Complementary goals (per-run model pinning vs cross-run concurrency safety) that meet in one function and in the pool's reuse guard. Whoever lands second must reconcile the hunk and decide what an overlapping mismatched-model review does: queue, or surface the busy error. Files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[atomsbaza]

Rebased onto latest main again — head a26aa7449

main moved another 96 commits; one rebase conflict (website/src/apps/code-review-sage/lib/types.ts, both sides' Run fields kept: upstream reason + this PR's model).

One new incompatibility surfaced after the rebase, found and fixed: upstream's new TestRunPayloadCarriesReasonToken._drive stub declared begin_batch(self) without the model kwarg the real ReviewPool.begin_batch(model=None) contract carries, so _run_review_bg's now-model-aware call raised TypeError inside the stub and the run fell into the generic error branch. The stub's signature was aligned with the real contract — no assertion weakened, no production change.

Gates on this head: 988 pytest passed (sage routes/pool/local-review, review_fix, routes coverage, security posture) · flake8 src/kiro_crew test conftest.py xdist_budget.py clean · mypy src/kiro_crew/ clean (1287 files) · tsc -b errors are all in files this PR does not touch (verified zero overlap with the PR's website diff — upstream main carries them).

Still requesting workflow approval to re-run the review workflows on this head.

[atomsbaza]

Force-pushed head to a73ecea656ed634074fb187729caf6296a898250 (amended: add feature-map row for the new review-fix dashboard handler + black-format test/test_review_fix.py). Requesting workflow approval for this head — CI should now pass both previously failing gates (Feature Map Gate, Backend Lint & Type Check). Local verify: black gate passed (0 new offenders), flake8 clean, mypy clean (1287 files), pytest test/test_review_fix.py 33 passed, feature-map gate with FEATURE_MAP_BASE_REF=origin/main ✓

[atomsbaza]

Rebased onto latest main (64 commits after the previous base) and force-pushed head to e7671e89b — requesting workflow approval for this head.

What the rebase surfaced and how it was resolved:

1. The two failing Backend Tests shards (Linux + Windows) came from upstream's own new test file, not this PR. test/test_slot_close_recreation_race.py (landed via Slot close: concurrent same-key recreation during post-pop teardown can clobber the replacement session #7191 after this PR's base) fails on main too: its _Req stub predates read_bounded_json's stream contract and dies with AttributeError: '_Req' object has no attribute 'can_read_body', cascading into 120s timeouts across the shard. Maintainer-side branch fix/slot-close-race-req-stub already carries the correct fix; this PR re-applies that same 37-line stub fix locally (fed can_read_body/content_length/charset/content per the _shared.py docstring) so the branch's CI gate goes green and doesn't block on a main-side defect.

2. No conflicts on the sage surfaces — the rebase was clean; the local_review/fix_tasks files this PR adds are untouched by main's concurrent sage work.

Local verify on the rebased head: pytest test_slot_close_recreation_race.py 42 passed (was 2 failed + 11 passed), test_review_fix.py 33 passed, black gate passed, flake8 clean, mypy clean (1294 files), feature-map gate ✓. Still 2 commits.

[atomsbaza]

Maintainer action needed: allow-fork-workflow-change label

This head (5fd3f9804) prunes 4 graduated entries from .github/black-baseline.txt (code_review_sage/backend/routes.py, sage_lib/review_pool.py, tests/test_backend_routes.py, tests/test_review_pool.py). These files are now black-clean (the PR formats them), and the black gate requires pruning entries that no longer offend — the baseline only shrinks. No workflow files are added or modified.

The guard blocks the PR until a maintainer applies the label; everything else on this head is green or in progress. Thank you!