From 59da9f016bf40c9dd2f97f6ad5c9af7aa54c6bd8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 9 Sep 2026 10:12:06 -0400 Subject: [PATCH] fix(#4466): bound quick.md's post-execute review scope tip at the task's own last commit (#4571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4466): bound quick.md's post-execute review scope tip at the task's own last commit The review-scoping step computed CHANGED_FILES as `git diff --name-only "${DIFF_BASE}..HEAD"`. DIFF_BASE is correctly bound to the quick task's start (via the oldest QUICK_COMMITS entry's parent), but the tip was bare HEAD -- unbounded. Anything landing on the shared tree between the task's own commits and this review step running (a worktree merge-back, another session sharing the tree) got folded into the quick task's own review scope. QUICK_COMMITS (newest-first) already holds the correct tip as its first line -- read QUICK_TIP from the value already computed, diff against that instead of HEAD. No new derivation, no new git call. Added tests/quick-review-scope-tip-bound.test.cjs: extracts the scoping fence verbatim from quick.md and runs it against a real git fixture matching the issue's own scenario (quick task's own commit, then a later unrelated commit on the shared tree). Manually verified watch-it-fail (bare HEAD includes the unrelated file) / watch-it-pass (bounded tip excludes it) via direct bash execution before wiring the test file, since this repo blocks local node --test. Independent code review caught one drive-by finding: an allow-test-rule marker copied from a sibling test's pattern was unnecessary here (and there) -- local/no-source-grep's looksLikeSourcePath only matches readFileSync targets ending in .cjs/.cts/.js/.mjs/.mts/.ts, never .md, so the rule can never fire regardless of the marker. Confirmed by reading eslint-rules/no-source-grep.cjs directly; removed. Emitted-Drift-Ack-Growth: quick.md — the fix adds a QUICK_TIP line and its explanatory comment; not a regeneration artifact, a hand-authored bug fix. Co-Authored-By: Claude Sonnet 5 * docs(#4466): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: sim Co-authored-by: Claude Sonnet 5 --- .changeset/vivid-quails-roam.md | 5 + gsd-core/workflows/quick.md | 9 +- tests/quick-review-scope-tip-bound.test.cjs | 130 ++++++++++++++++++++ 3 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 .changeset/vivid-quails-roam.md create mode 100644 tests/quick-review-scope-tip-bound.test.cjs diff --git a/.changeset/vivid-quails-roam.md b/.changeset/vivid-quails-roam.md new file mode 100644 index 000000000..fc5f38546 --- /dev/null +++ b/.changeset/vivid-quails-roam.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4571 +--- +**`/gsd-quick`'s post-execute review no longer scopes past its own last commit** — the review-scoping step diffed against bare HEAD instead of the quick task's own newest commit, so any later commit landing on the same tree before the review ran (a worktree merge-back, a shared tree) was silently folded into the quick task's own code-review scope. diff --git a/gsd-core/workflows/quick.md b/gsd-core/workflows/quick.md index 651f94564..1cc6cf864 100644 --- a/gsd-core/workflows/quick.md +++ b/gsd-core/workflows/quick.md @@ -569,7 +569,14 @@ else fi if [ -n "$DIFF_BASE" ]; then - CHANGED_FILES=$(git diff --name-only "${DIFF_BASE}..HEAD" -- . ':!.planning' 2>/dev/null | tr '\n' ' ') + # #4466: bound the tip at the quick task's own last commit, not HEAD -- + # QUICK_COMMITS is already the complete, newest-first list of this task's + # commits, so its first line is the correct tip. An unbounded `..HEAD` + # picks up any later commit landed on the same tree in the window between + # this task's commits and this review step (worktree merge-back, a shared + # tree, another session) and folds it into this task's own review scope. + QUICK_TIP=$(echo "$QUICK_COMMITS" | head -1) + CHANGED_FILES=$(git diff --name-only "${DIFF_BASE}..${QUICK_TIP}" -- . ':!.planning' 2>/dev/null | tr '\n' ' ') else CHANGED_FILES="" fi diff --git a/tests/quick-review-scope-tip-bound.test.cjs b/tests/quick-review-scope-tip-bound.test.cjs new file mode 100644 index 000000000..6474fb6ce --- /dev/null +++ b/tests/quick-review-scope-tip-bound.test.cjs @@ -0,0 +1,130 @@ +'use strict'; + +/** + * Regression coverage for #4466: quick.md's post-execute review scoping step + * computes `CHANGED_FILES` as `git diff --name-only "${DIFF_BASE}..HEAD"`. + * `DIFF_BASE` (the parent of QUICK_COMMITS's oldest entry) is correctly + * bound to the quick task's start, but the tip is bare HEAD — unbounded. + * Anything landing on the shared tree between the task's own commits and + * this review step running (a worktree merge-back, another session sharing + * the tree) gets folded into the quick task's own review scope. + * + * QUICK_COMMITS (newest-first) already holds the correct tip as its first + * line — the fix reads that instead of using HEAD, no new git call needed. + * + * Mirrors the issue's own verified reproduction methodology: extract the + * fence VERBATIM from quick.md (never reimplemented), run it against a real + * constructed git fixture matching the issue's own exact scenario (an + * unrelated commit landing after the quick task's own work, before review). + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); +const { GIT_FIXTURE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'quick.md'); + +function extractFirstBashBlockAfter(content, startAnchor, stopAnchor) { + const start = content.indexOf(startAnchor); + assert.ok(start !== -1, `quick.md must contain the anchor "${startAnchor}"`); + const stop = stopAnchor ? content.indexOf(stopAnchor, start + startAnchor.length) : content.length; + assert.ok(!stopAnchor || stop !== -1, `quick.md must contain the anchor "${stopAnchor}" after "${startAnchor}"`); + const region = content.slice(start, stop); + + const fenceStart = region.indexOf('```bash'); + assert.ok(fenceStart !== -1, `no \`\`\`bash fence found between "${startAnchor}" and its stop anchor`); + const fenceEnd = region.indexOf('```', fenceStart + '```bash'.length); + assert.ok(fenceEnd !== -1, `unterminated \`\`\`bash fence after "${startAnchor}"`); + return region.slice(fenceStart + '```bash'.length, fenceEnd); +} + +function seedFixtureRepo(dir) { + gitOrThrow(['init', '-q'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 't@example.com'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'T'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); +} + +function writeAndCommit(dir, relPath, content, message) { + const abs = path.join(dir, relPath); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + gitOrThrow(['add', '-A'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['commit', '-q', '-m', message], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); +} + +// Matches the issue's own fixture exactly: init, then the quick task's own +// commit (referencing quick_id in its message), then an unrelated later +// commit landing on the same tree before the review step runs. +const QUICK_ID = '260906-abc'; + +function buildFixture(tmpDir) { + seedFixtureRepo(tmpDir); + writeAndCommit(tmpDir, 'README.md', '# init\n', 'chore: init'); + writeAndCommit(tmpDir, 'src/quick-a.js', 'quick-a\n', `feat(quick-${QUICK_ID}): the quick task's own work`); + writeAndCommit(tmpDir, 'src/unrelated.js', 'unrelated\n', 'fix: an unrelated commit from another session on the shared tree'); +} + +function runScopingFence(tmpDir) { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const fence = extractFirstBashBlockAfter(content, "**Scope files from executor's commits:**", '**Invoke review:**'); + + const script = [ + '#!/usr/bin/env bash', + 'set -uo pipefail', + `quick_id="${QUICK_ID}"`, + '{', + fence, + '} 1>&2', + 'printf \'%s\\n\' "$CHANGED_FILES"', + ].join('\n'); + + const scriptPath = path.join(tmpDir, '.scope-script.sh'); + fs.writeFileSync(scriptPath, script); + + const result = spawnSync('bash', [scriptPath], { + cwd: tmpDir, + encoding: 'utf8', + timeout: GIT_FIXTURE_TIMEOUT_MS, + }); + if (result.error) { + throw new Error(`bash spawn failed: ${result.error.message}\ndiagnostics:\n${result.stderr || '(none)'}`); + } + if (result.status !== 0) { + throw new Error(`bash exited ${result.status} (signal ${result.signal})\ndiagnostics:\n${result.stderr || '(none)'}`); + } + const files = result.stdout.trim().split(/\s+/).filter(Boolean).sort(); + return { files, diagnostics: result.stderr }; +} + +describe('#4466: quick.md review scoping bounds the tip at the quick task\'s own last commit', () => { + const workflowContent = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const fence = extractFirstBashBlockAfter(workflowContent, "**Scope files from executor's commits:**", '**Invoke review:**'); + + test('the diff uses a bounded tip, not bare HEAD (the gate exists)', () => { + assert.ok( + /git diff --name-only "\$\{DIFF_BASE\}\.\.\$\{QUICK_TIP\}"/.test(fence), + 'the scoping fence must diff against a bounded QUICK_TIP, not bare HEAD', + ); + }); + + test('real execution: a later unrelated commit on the shared tree is excluded from scope (issue #4466 repro)', () => { + const tmpDir = fs.realpathSync.native(createTempDir('gsd-4466-')); + try { + buildFixture(tmpDir); + const { files, diagnostics } = runScopingFence(tmpDir); + assert.deepEqual( + files, + ['src/quick-a.js'], + `quick task's review scope must not include a later unrelated commit, got: ${JSON.stringify(files)}\ndiagnostics:\n${diagnostics || '(none)'}`, + ); + } finally { + cleanup(tmpDir); + } + }); +});