From 67a9550720ea0f5d921ce30e5078747578419d71 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 22 Apr 2026 12:04:10 -0400 Subject: [PATCH] fix(#2549,#2550,#2552): bound discuss-phase context reads, add phase-type map selection, prohibit split reads (#2590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2549: load_prior_context was reading every prior *-CONTEXT.md file, growing linearly with project phase count. Cap to the 3 most recent phases. If .planning/DECISIONS-INDEX.md exists, read that instead. #2550: scout_codebase claimed to select maps "based on phase type" but had no classifier — agents read all 7 maps. Replace with an explicit phase-type-to-maps table (2–3 maps per phase type) with a Mixed fallback. #2552: Add explicit instruction not to split-read the same file at two different offsets. Split reads break prompt cache reuse and cost more than a single full read. Closes #2549 Closes #2550 Closes #2552 Co-authored-by: Claude Sonnet 4.6 --- get-shit-done/workflows/discuss-phase.md | 33 ++++++-- ...9-2550-2552-discuss-phase-context.test.cjs | 78 +++++++++++++++++++ 2 files changed, 106 insertions(+), 5 deletions(-) create mode 100644 tests/bug-2549-2550-2552-discuss-phase-context.test.cjs diff --git a/get-shit-done/workflows/discuss-phase.md b/get-shit-done/workflows/discuss-phase.md index 9964a45db..36ae06e55 100644 --- a/get-shit-done/workflows/discuss-phase.md +++ b/get-shit-done/workflows/discuss-phase.md @@ -318,13 +318,20 @@ Extract from these: - **REQUIREMENTS.md** — Acceptance criteria, constraints, must-haves vs nice-to-haves - **STATE.md** — Current progress, any flags or session notes -**Step 2: Read all prior CONTEXT.md files** +**Step 2: Read bounded prior CONTEXT.md files** + +Reading every prior CONTEXT.md grows linearly with phase count and inflates context cost. Instead, read a bounded set: + ```bash -# Find all CONTEXT.md files from phases before current -(find .planning/phases -name "*-CONTEXT.md" 2>/dev/null || true) | sort +# Find CONTEXT.md files from phases before current, sorted newest-first +(find .planning/phases -name "*-CONTEXT.md" 2>/dev/null || true) | sort -r ``` -For each CONTEXT.md where phase number < current phase: +Read at most **3** prior CONTEXT.md files (the most recent 3 phases before the current one). This is sufficient for decision continuity — earlier decisions are already captured in PROJECT.md and REQUIREMENTS.md. + +If `.planning/DECISIONS-INDEX.md` exists, read that instead of individual CONTEXT files — it is a bounded rolling summary that supersedes per-phase reads. + +For each CONTEXT.md read: - Read the `` section — these are locked preferences - Read `` — particular references or "I want it like X" moments - Note any patterns (e.g., "user consistently prefers minimal UI", "user rejected single-key shortcuts") @@ -436,7 +443,23 @@ Lightweight scan of existing code to inform gray area identification and discuss ls .planning/codebase/*.md 2>/dev/null || true ``` -**If codebase maps exist:** Read the most relevant ones (CONVENTIONS.md, STRUCTURE.md, STACK.md based on phase type). Extract: +**If codebase maps exist:** Select 2–3 maps based on phase type. Do NOT read all seven — that inflates context without improving discussion quality. Use this selection table: + +| Phase type (infer from title + ROADMAP entry) | Read these maps | +|---|---| +| UI / frontend / styling / design | CONVENTIONS.md, STRUCTURE.md, STACK.md | +| Backend / API / service / data model | STACK.md, ARCHITECTURE.md, INTEGRATIONS.md | +| Integration / third-party / provider | STACK.md, INTEGRATIONS.md, ARCHITECTURE.md | +| Infrastructure / DevOps / CI / deploy | STACK.md, ARCHITECTURE.md, INTEGRATIONS.md | +| Testing / QA / coverage | TESTING.md, CONVENTIONS.md, STRUCTURE.md | +| Documentation / content | CONVENTIONS.md, STRUCTURE.md | +| Mixed / unclear | STACK.md, ARCHITECTURE.md, CONVENTIONS.md | + +Read CONCERNS.md only if the phase explicitly addresses known concerns or security issues. + +**Important — no split reads:** Read each map file in a single Read call. Do not read the same file at two different offsets — split reads break prompt cache reuse and cost more than a single full read. + +Extract: - Reusable components/hooks/utilities - Established patterns (state management, styling, data fetching) - Integration points (where new code would connect) diff --git a/tests/bug-2549-2550-2552-discuss-phase-context.test.cjs b/tests/bug-2549-2550-2552-discuss-phase-context.test.cjs new file mode 100644 index 000000000..b10152a7a --- /dev/null +++ b/tests/bug-2549-2550-2552-discuss-phase-context.test.cjs @@ -0,0 +1,78 @@ +'use strict'; + +/** + * Bugs #2549, #2550, #2552: discuss-phase context bloat and cache invalidation. + * + * #2549: load_prior_context must cap prior CONTEXT.md reads (was O(phases)) + * #2550: scout_codebase must select maps by phase type (was always all 7) + * #2552: scout_codebase must not instruct split reads of the same file + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const DISCUSS_PHASE = path.join( + __dirname, '..', 'get-shit-done', 'workflows', 'discuss-phase.md', +); + +describe('discuss-phase context fixes (#2549, #2550, #2552)', () => { + let src; + test('discuss-phase.md source exists', () => { + assert.ok(fs.existsSync(DISCUSS_PHASE), 'discuss-phase.md must exist'); + src = fs.readFileSync(DISCUSS_PHASE, 'utf-8'); + }); + + // ─── #2549: load_prior_context cap ────────────────────────────────────── + test('#2549: load_prior_context must NOT instruct reading ALL prior CONTEXT.md files', () => { + if (!src) src = fs.readFileSync(DISCUSS_PHASE, 'utf-8'); + assert.ok( + !src.includes('For each CONTEXT.md where phase number < current phase'), + 'load_prior_context must not unboundedly read all prior CONTEXT.md files', + ); + }); + + test('#2549: load_prior_context must reference a bounded read (3 phases or DECISIONS-INDEX)', () => { + if (!src) src = fs.readFileSync(DISCUSS_PHASE, 'utf-8'); + const hasBound = src.includes('3') && src.includes('prior CONTEXT.md'); + const hasIndex = src.includes('DECISIONS-INDEX.md'); + assert.ok( + hasBound || hasIndex, + 'load_prior_context must reference a bounded read (e.g., most recent 3 phases) or DECISIONS-INDEX.md', + ); + }); + + // ─── #2550: scout_codebase phase-type selection ────────────────────────── + test('#2550: scout_codebase must not instruct reading all 7 codebase maps', () => { + if (!src) src = fs.readFileSync(DISCUSS_PHASE, 'utf-8'); + assert.ok( + !src.includes('Read the most relevant ones (CONVENTIONS.md, STRUCTURE.md, STACK.md based on phase type)'), + 'scout_codebase must not use the old vague "most relevant" instruction without a selection table', + ); + }); + + test('#2550: scout_codebase must include a phase-type-to-maps selection table', () => { + if (!src) src = fs.readFileSync(DISCUSS_PHASE, 'utf-8'); + // The table maps phase types to specific map selections + assert.ok( + src.includes('Phase type') && src.includes('Read these maps'), + 'scout_codebase must include a phase-type to map-selection table', + ); + // Key phase types must be covered + assert.ok(src.includes('UI') || src.includes('frontend'), 'Table must cover UI/frontend phases'); + assert.ok(src.includes('Backend') || src.includes('API'), 'Table must cover backend phases'); + assert.ok(src.includes('Testing'), 'Table must cover testing phases'); + assert.ok(src.includes('Mixed'), 'Table must have a fallback for mixed/unclear phases'); + }); + + // ─── #2552: no split reads ─────────────────────────────────────────────── + test('#2552: scout_codebase must explicitly prohibit split reads of the same file', () => { + if (!src) src = fs.readFileSync(DISCUSS_PHASE, 'utf-8'); + const prohibitsSplit = src.includes('split reads') || src.includes('split read'); + assert.ok( + prohibitsSplit, + 'scout_codebase must explicitly warn against split reads (same file, two offsets) that break prompt cache', + ); + }); +});