From 652796e903c630d7ac367fc57de6a0c11da4b1c2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 16 Sep 2026 19:09:19 -0400 Subject: [PATCH] fix(#4665): route --fix past the empty-scope exit (#4810) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4665): add failing-first contract coverage for the --fix empty-scope recovery * fix(#4665): route --fix past the empty-scope exit check_empty_scope exited the entire workflow whenever REVIEW_FILES was empty — before dispatch-fix — so with #3661's incremental scoping, a phase whose only post-review changes were planning artifacts could never run --fix against its standing REVIEW.md findings, and the skip output did not even mention the flag. The skip is now a self-contained guarded fence (explicit REVIEW_FILES emptiness check): it fires only when --fix is absent OR the phase's REVIEW.md does not exist. Otherwise the workflow proceeds directly to dispatch-fix, which delegates to code-review-fix.md — the canonical fix implementation that already documents handling an existing REVIEW.md — while the fresh-review steps (structural pre-pass, reviewer lanes, spawn_reviewer, commit_review) are skipped: nothing new to review, nothing to commit. dispatch-fix.md's route docstring is synced. Emitted-Drift-Ack-Growth: code-review.md — check_empty_scope gains the --fix recovery branch (#4665) * docs(#4665): backfill changeset PR number --------- Co-authored-by: sim --- .changeset/proud-elks-fly.md | 5 ++ gsd-core/workflows/code-review.md | 18 +++-- .../code-review/steps/dispatch-fix.md | 7 +- .../code-review-pipeline-regression.test.cjs | 72 +++++++++++++++++++ 4 files changed, 95 insertions(+), 7 deletions(-) create mode 100644 .changeset/proud-elks-fly.md diff --git a/.changeset/proud-elks-fly.md b/.changeset/proud-elks-fly.md new file mode 100644 index 000000000..0ef30363a --- /dev/null +++ b/.changeset/proud-elks-fly.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4810 +--- +**`/gsd-code-review --fix` now works on an existing REVIEW.md even when no source files changed** — incremental scoping (#3661) narrowed the review file set to empty for phases whose only post-review changes were planning artifacts, and the empty-scope check exited the whole workflow before the fix step, so standing findings could never be addressed. With `--fix` and an existing REVIEW.md, the empty scope now routes straight to the fix workflow instead of skipping. (#4665) diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index dc79b35ca..db8acacb2 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -533,11 +533,21 @@ This `if`/`else`/`fi` is the entire guard: when `DEPTH_OK` is not the literal st -If REVIEW_FILES is empty: +An empty `REVIEW_FILES` (#3661) means nothing new to re-review — NOT a phase with no standing findings. #4665: with `--fix` + an existing REVIEW.md, route to `dispatch-fix` (the flag covers "if REVIEW.md already exists"): + +```bash +REVIEW_PATH="${PHASE_DIR}/${PADDED_PHASE}-REVIEW.md" +if [ "${#REVIEW_FILES[@]}" -ne 0 ]; then + # non-empty scope: no-op + true +elif [ "$FIX_FLAG" != "true" ] || [ ! -f "${REVIEW_PATH}" ]; then + echo "No source files changed in phase ${PHASE_ARG}. Skipping review." + # Exit workflow. Do NOT spawn agent or create REVIEW.md. + exit 0 +fi ``` -No source files changed in phase ${PHASE_ARG}. Skipping review. -``` -Exit workflow. Do NOT spawn agent or create REVIEW.md. + +**`--fix` recovery:** proceed DIRECTLY to `dispatch-fix`, skipping `structural_pre_pass`, `dispatch_reviewer_lanes`, `spawn_reviewer`, `commit_review` — no fresh review, nothing to commit; `code-review-fix.md` resolves the existing REVIEW.md and owns the fix logic. The reviewer agent is not dispatched here. diff --git a/gsd-core/workflows/code-review/steps/dispatch-fix.md b/gsd-core/workflows/code-review/steps/dispatch-fix.md index 7a2efb628..3dbaa339f 100644 --- a/gsd-core/workflows/code-review/steps/dispatch-fix.md +++ b/gsd-core/workflows/code-review/steps/dispatch-fix.md @@ -2,9 +2,10 @@ If the `--fix` flag was passed (`FIX_FLAG=true`), delegate to the `code-review-fix.md` workflow to auto-apply findings from the REVIEW.md that was just written (or that already existed). -This step runs AFTER `commit_review` so REVIEW.md is guaranteed to be on disk before the fixer -is invoked. If REVIEW.md was not created (agent failed, scope was empty, etc.), the `code-review-fix.md` -workflow handles the missing-review error and exits cleanly. +This step runs AFTER `commit_review` so a freshly-written REVIEW.md is guaranteed to be on disk +before the fixer is invoked. Since #4665 there is a second route: `check_empty_scope` jumps here +directly when the incremental scope is empty but `--fix` was passed and a REVIEW.md already exists +— `code-review-fix.md` resolves it from the phase and owns the missing-review error either way. ```bash if [ "$FIX_FLAG" = "true" ]; then diff --git a/tests/code-review-pipeline-regression.test.cjs b/tests/code-review-pipeline-regression.test.cjs index dff9339c3..f5be4d2b6 100644 --- a/tests/code-review-pipeline-regression.test.cjs +++ b/tests/code-review-pipeline-regression.test.cjs @@ -1577,3 +1577,75 @@ describe('CONS-01..03 — external reviewer evidence consolidation (#4209)', () } }); }); + +// --------------------------------------------------------------------------- +// #4665 — `--fix` must be able to act on an existing REVIEW.md even when the +// incremental scope for a FRESH review is empty. check_empty_scope used to +// exit the entire workflow whenever REVIEW_FILES was empty — before +// dispatch-fix — so the documented contract ("after review completes (or if +// REVIEW.md already exists), auto-apply findings found") was unreachable for +// any phase whose post-review changes are planning artifacts only. +// --------------------------------------------------------------------------- + +describe('#4665 — check_empty_scope --fix recovery onto an existing REVIEW.md', () => { + const readWorkflow = () => fs.readFileSync(WORKFLOW_PATH, 'utf8'); + + test('check_empty_scope recovers --fix onto an existing REVIEW.md instead of exiting (#4665)', () => { + const src = readWorkflow(); + const stepIdx = src.indexOf(''); + const nextStepIdx = src.indexOf(''); + assert.ok(stepIdx !== -1, 'check_empty_scope step must exist'); + assert.ok(nextStepIdx > stepIdx, 'structural_pre_pass must follow check_empty_scope'); + const block = src.slice(stepIdx, nextStepIdx); + + // The recovery computes the phase's REVIEW.md path with the same + // expression spawn_reviewer uses. + assert.ok( + block.includes('REVIEW_PATH="${PHASE_DIR}/${PADDED_PHASE}-REVIEW.md"'), + 'check_empty_scope must compute REVIEW_PATH exactly as spawn_reviewer does' + ); + // The skip is now guarded: it fires only when --fix is absent OR no + // REVIEW.md exists on disk. + assert.ok( + block.includes('FIX_FLAG') && block.includes('-f "${REVIEW_PATH}"'), + 'the empty-scope skip must be guarded on FIX_FLAG and the existing REVIEW.md file check' + ); + // The fence must be self-contained about its own precondition: an explicit + // emptiness check, so a literal-minded execution cannot read the recovery + // paragraph as skipping a needed fresh review on a non-empty scope. + assert.ok( + block.includes("\"${#REVIEW_FILES[@]}\" -ne 0"), + 'check_empty_scope must assert REVIEW_FILES emptiness explicitly, not only in prose' + ); + }); + + test('the fix-recovery path routes to dispatch-fix past the fresh-review steps (#4665)', () => { + const src = readWorkflow(); + const stepIdx = src.indexOf(''); + const nextStepIdx = src.indexOf(''); + const block = src.slice(stepIdx, nextStepIdx); + + assert.match(block, /dispatch-fix/, 'the recovery must route to dispatch-fix'); + assert.match( + block, /structural_pre_pass[\s\S]*dispatch_reviewer_lanes[\s\S]*spawn_reviewer[\s\S]*commit_review/, + 'the recovery must name the fresh-review steps it skips (no reviewer spawn, nothing to commit)' + ); + assert.doesNotMatch( + block, /spawn the (reviewer|agent)|gsd-code-reviewer/, + 'the recovery must not contain an instruction to spawn a fresh reviewer' + ); + }); + + test('the plain empty-scope skip survives for the non-fix path (#4665)', () => { + const src = readWorkflow(); + const stepIdx = src.indexOf(''); + const nextStepIdx = src.indexOf(''); + const block = src.slice(stepIdx, nextStepIdx); + + assert.ok( + block.includes('No source files changed in phase ${PHASE_ARG}. Skipping review.'), + 'the plain skip text must remain for invocations without --fix or without an existing REVIEW.md' + ); + assert.match(block, /Exit workflow/i, 'the non-recovery path must still end the workflow'); + }); +});