diff --git a/.changeset/eager-tunas-gather.md b/.changeset/eager-tunas-gather.md new file mode 100644 index 000000000..cc842fbf8 --- /dev/null +++ b/.changeset/eager-tunas-gather.md @@ -0,0 +1,7 @@ +--- +type: Changed +pr: 4206 +--- +**Planner wave assignment now sequences automatic external review after internal fixes** — phases with internal review lanes defer PR creation until accepted fixes land and re-check open-time properties immediately before opening. + + diff --git a/agents/gsd-planner.md b/agents/gsd-planner.md index a8989be58..65126dd7b 100644 --- a/agents/gsd-planner.md +++ b/agents/gsd-planner.md @@ -756,6 +756,8 @@ for each plan B in plan_order: **Rule:** Same-wave plans must have zero `files_modified`/`files_deleted` overlap. After assigning waves, scan each wave; if any file appears in 2+ plans, bump the later plan to the next wave and repeat. +**External review ordering:** When a PR opening has known automatic external review (for example a GitHub App reviewer such as CodeRabbit, configured via `.coderabbit.yaml`, which reviews automatically on PR open) and the plan includes internal review lanes, run internal review and apply the accepted internal-review fixes before the final open. If an open-time property exists (for example a not-behind-base check that must legitimately be measured at PR-open instant), re-check it immediately before opening, with nothing intervening; post-open CI, review, and tracking may follow. Examples: @gsd-core/references/planner-antipatterns.md ("External Review Before PR Open (#4107)"). + Non-file coupling: @~/.claude/gsd-core/references/planner-coupling.md diff --git a/gsd-core/references/planner-antipatterns.md b/gsd-core/references/planner-antipatterns.md index 2e08a9039..5b9cb1dbe 100644 --- a/gsd-core/references/planner-antipatterns.md +++ b/gsd-core/references/planner-antipatterns.md @@ -228,3 +228,28 @@ test -f src/i18n/en.json && test -f src/i18n/de.json || { echo "missing input fi ``` **When `|| echo "default"` is acceptable:** only when absence is semantically the default AND the result is NOT used in a comparison that should detect absence. + +## External Review Before PR Open (#4107) + +Apply this ordering only when opening the PR is known to trigger automatic external review and the plan also has internal review lanes. + +**Bad:** + +```text +Wave 1: Open PR; automatic external review starts +Wave 2: Run internal review +Wave 3: Apply accepted fixes +``` + +The external reviewer spends its first pass on a diff the plan already expects to change. + +**Good:** + +```text +Wave 1: Run internal review +Wave 2: Apply accepted fixes +Wave 3: If applicable, re-check the open-time property; then immediately open PR +Wave 4+: Run post-open CI, external review, and tracking work +``` + +Nothing may intervene between an applicable re-check and the open. Post-open work may follow; "immediately" constrains only that gap. Opening-time properties do not justify an early PR. diff --git a/tests/prompt-thinning.test.cjs b/tests/prompt-thinning.test.cjs index b79e44c5c..25fb97e86 100644 --- a/tests/prompt-thinning.test.cjs +++ b/tests/prompt-thinning.test.cjs @@ -17,6 +17,7 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const { collectSection } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs'); const EXECUTE_PHASE = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md'); const EXECUTOR_AGENT = path.join(__dirname, '..', 'agents', 'gsd-executor.md'); @@ -70,6 +71,36 @@ describe('prompt thinning — sub-200K context window support (#1978)', () => { 'gsd-planner.md must reference planner-antipatterns.md for extended checkpoint anti-patterns and specificity examples' ); }); + + test('sequences known external review after internal review fixes (#4107)', () => { + const planner = fs.readFileSync(PLANNER_AGENT, 'utf-8'); + const assignWaves = planner.match(/([\s\S]{0,3000}?)<\/step>/)?.[1] || ''; + assert.match(assignWaves, /known automatic external review/i); + assert.match(assignWaves, /plan includes internal review lanes/i); + assert.match(assignWaves, /accepted internal-review fixes[^.]*before[^.]*open/i); + assert.match(assignWaves, /run internal review and apply the accepted internal-review fixes before the final open/i); + assert.match(assignWaves, /if an open-time property exists[^.]*re-check it immediately before opening[^.]*nothing intervening/i); + assert.match(assignWaves, /post-open CI[^.]*review[^.]*tracking may follow/i); + // Directionality fixture (#4107): an inverted-order sentence — PR opens + // first, fixes land after — must fail the ordering-sensitive assertion + // above. Proves the regex tests sequence, not mere keyword co-occurrence. + const invertedOrdering = 'Open the PR before scheduling the accepted internal-review fixes.'; + assert.doesNotMatch(invertedOrdering, /accepted internal-review fixes[^.]*before[^.]*open/i); + const invertedPlannerText = 'Open the PR, then run internal review and apply the accepted internal-review fixes.'; + assert.doesNotMatch(invertedPlannerText, /run internal review and apply the accepted internal-review fixes before the final open/i); + assert.match(assignWaves, /planner-antipatterns\.md \("External Review Before PR Open \(#4107\)"\)/); + + const reference = fs.readFileSync(PLANNER_ANTIPATTERNS_REF, 'utf-8'); + const example = collectSection(reference, (heading) => heading.text === 'External Review Before PR Open (#4107)')?.body || ''; + assert.match(example, /Bad[\s\S]*open[^\n]*PR[\s\S]*internal review/i); + assert.match(example, /Good[\s\S]*internal review[\s\S]*accepted fixes[\s\S]*if applicable[^\n]*re-check[^\n]*then immediately open PR/i); + assert.match(example, /nothing may intervene[^.]*re-check[^.]*open/i); + assert.match(example, /post-open work may follow/i); + // Directionality fixture: a Good section that opens the PR before + // internal review must fail the "Good" assertion above. + const invertedExample = 'Good:\nWave 1: If applicable, re-check the open-time property; then immediately open PR\nWave 2: Run internal review\nWave 3: Apply accepted fixes\n'; + assert.doesNotMatch(invertedExample, /Good[\s\S]*internal review[\s\S]*accepted fixes[\s\S]*if applicable[^\n]*re-check[^\n]*then immediately open PR/i); + }); }); describe('executor-examples.md — extracted reference file', () => {