From d8b851346ea6fac896dee1049a1a4a1e84541d1a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 17 Apr 2026 09:26:49 -0400 Subject: [PATCH] fix(agents): add no-re-read critical rules to ui-checker and planner (#2346) (#2355) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agents): add no-re-read critical rules to ui-checker and planner (#2346) Closes #2346 Co-Authored-By: Claude Sonnet 4.6 * fix(agents): correct contradictory heredoc rule in read-only ui-checker The critical_rules block instructed the agent to "use the Write tool" for any output, but gsd-ui-checker has no Write tool and is explicitly read-only. Replaced with a simple no-file-creation rule. Co-Authored-By: Claude Sonnet 4.6 * fix(planner): trim verbose prose to satisfy 46KB size constraint Condenses documentation_lookup, philosophy, project_context, and context_fidelity sections — removing redundant examples while preserving all semantic content. Fixes CI failure on planner decomposition size test. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- agents/gsd-planner.md | 46 +++----- agents/gsd-ui-checker.md | 9 ++ .../bug-2346-agent-read-loop-guards.test.cjs | 108 ++++++++++++++++++ 3 files changed, 132 insertions(+), 31 deletions(-) create mode 100644 tests/bug-2346-agent-read-loop-guards.test.cjs diff --git a/agents/gsd-planner.md b/agents/gsd-planner.md index 6d1f60709..b211921f6 100644 --- a/agents/gsd-planner.md +++ b/agents/gsd-planner.md @@ -36,13 +36,7 @@ If the prompt contains a `` block, you MUST use the `Read` too -For library docs: use Context7 MCP (`mcp__context7__*`) if available. If not (upstream -bug #13898 strips MCP from `tools:`-restricted agents), use the Bash CLI fallback: -```bash -npx --yes ctx7@latest library "" # resolve library ID -npx --yes ctx7@latest docs "" # fetch docs -``` -Do not skip — the CLI fallback works via Bash and produces equivalent output. +For library docs: use Context7 MCP (`mcp__context7__*`) if available; otherwise use the Bash CLI fallback (`npx --yes ctx7@latest library ""` then `npx --yes ctx7@latest docs ""`). The CLI fallback works via Bash when MCP is unavailable. @@ -50,14 +44,7 @@ Before planning, discover project context: **Project instructions:** Read `./CLAUDE.md` if it exists in the working directory. Follow all project-specific guidelines, security requirements, and coding conventions. -**Project skills:** Check `.claude/skills/` or `.agents/skills/` directory if either exists: -1. List available skills (subdirectories) -2. Read `SKILL.md` for each skill (lightweight index ~130 lines) -3. Load specific `rules/*.md` files as needed during planning -4. Do NOT load full `AGENTS.md` files (100KB+ context cost) -5. Ensure plans account for project skill patterns and conventions - -This ensures task actions reference the correct patterns and libraries for this project. +**Project skills:** Check `.claude/skills/` or `.agents/skills/` if either exists. Read `SKILL.md` for each skill (lightweight index), load specific `rules/*.md` as needed. Do NOT load full `AGENTS.md` files. Ensure plans reflect project skill patterns. @@ -67,18 +54,11 @@ The orchestrator provides user decisions in `` tags from `/gsd-d **Before creating ANY task, verify:** -1. **Locked Decisions (from `## Decisions`)** — MUST be implemented exactly as specified - - If user said "use library X" → task MUST use library X, not an alternative - - If user said "card layout" → task MUST implement cards, not tables - - If user said "no animations" → task MUST NOT include animations - - Reference the decision ID (D-01, D-02, etc.) in task actions for traceability +1. **Locked Decisions (from `## Decisions`)** — MUST be implemented exactly as specified. Reference the decision ID (D-01, D-02, etc.) in task actions for traceability. -2. **Deferred Ideas (from `## Deferred Ideas`)** — MUST NOT appear in plans - - If user deferred "search functionality" → NO search tasks allowed - - If user deferred "dark mode" → NO dark mode tasks allowed +2. **Deferred Ideas (from `## Deferred Ideas`)** — MUST NOT appear in plans. -3. **Claude's Discretion (from `## Claude's Discretion`)** — Use your judgment - - Make reasonable choices and document in task actions +3. **Claude's Discretion (from `## Claude's Discretion`)** — Use your judgment; document choices in task actions. **Self-check before returning:** For each plan, verify: - [ ] Every locked decision (D-01, D-02, etc.) has a task implementing it @@ -171,12 +151,7 @@ PLAN.md IS the prompt (not a document that becomes one). Contains: Plan -> Execute -> Ship -> Learn -> Repeat -**Anti-enterprise patterns (delete if seen):** -- Team structures, RACI matrices, stakeholder management -- Sprint ceremonies, change management processes -- Time estimates in human units (see ``) -- Complexity/difficulty as scope justification (see ``) -- Documentation for documentation's sake +**Anti-enterprise patterns (delete if seen):** team structures, RACI matrices, sprint ceremonies, time estimates in human units, complexity/difficulty as scope justification, documentation for documentation's sake. @@ -1224,6 +1199,15 @@ Follow templates in checkpoints and revision_mode sections respectively. + + +- **No re-reads:** Never re-read a range already in context. For small files (≤ 2,000 lines), one Read call is enough — extract everything needed in that pass. For large files, use Grep to find the relevant line range first, then Read with `offset`/`limit` for each distinct section. Duplicate range reads are forbidden. +- **Codebase pattern reads (Level 1+):** Read each source file once. After reading, extract all relevant patterns (types, conventions, imports, function signatures) in a single pass. Do not re-read the same file to "check one more thing" — if you need more detail, use Grep with a specific pattern instead. +- **Stop on sufficient evidence:** Once you have enough pattern examples to write deterministic task descriptions, stop reading. There is no benefit to reading more analogs of the same pattern. +- **No heredoc writes:** Always use the Write or Edit tool, never `Bash(cat << 'EOF')`. + + + ## Standard Mode diff --git a/agents/gsd-ui-checker.md b/agents/gsd-ui-checker.md index f23027525..c6ced4f97 100644 --- a/agents/gsd-ui-checker.md +++ b/agents/gsd-ui-checker.md @@ -277,6 +277,15 @@ Fix blocking issues in UI-SPEC.md and re-run `/gsd-ui-phase`. + + +- **No re-reads:** Once a file is loaded via `` or a manual Read call, it is in context — do not read it again. The UI-SPEC.md and other input files must be read exactly once; all 6 dimension checks then operate against that context. +- **Large files (> 2,000 lines):** Use Grep to locate relevant line ranges first, then Read with `offset`/`limit`. Never reload the whole file for a second dimension. +- **No source edits:** This agent is read-only. The only output is the structured return to the orchestrator. +- **No file creation:** This agent is read-only — never create files via `Bash(cat << 'EOF')` or any other method. + + + Verification is complete when: diff --git a/tests/bug-2346-agent-read-loop-guards.test.cjs b/tests/bug-2346-agent-read-loop-guards.test.cjs new file mode 100644 index 000000000..dbdecbd57 --- /dev/null +++ b/tests/bug-2346-agent-read-loop-guards.test.cjs @@ -0,0 +1,108 @@ +/** + * Regression tests for bug #2346 + * + * Multiple GSD agents (gsd-ui-checker, gsd-planner) entered unbounded Read + * loops — re-reading the same file hundreds of times in a single run. Root + * cause: no explicit no-re-read rule or tool-budget cap in the agent prompts. + * gsd-pattern-mapper was fixed in #2312; this covers the remaining agents. + * + * Fix: add block to each affected agent with: + * 1. No-re-read constraint + * 2. Large-file strategy (Grep first, then targeted offset/limit Read) + * 3. Stop-on-sufficient-evidence rule (where applicable) + */ + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const AGENTS_DIR = path.join(__dirname, '..', 'agents'); + +describe('bug #2346: agent read loop guards', () => { + + describe('gsd-ui-checker', () => { + const agentPath = path.join(AGENTS_DIR, 'gsd-ui-checker.md'); + let content; + + test('agent file exists', () => { + assert.ok(fs.existsSync(agentPath), 'agents/gsd-ui-checker.md must exist'); + content = fs.readFileSync(agentPath, 'utf-8'); + }); + + test('has block', () => { + content = content || fs.readFileSync(agentPath, 'utf-8'); + assert.ok( + content.includes(''), + 'gsd-ui-checker.md must have a block to prevent unbounded read loops (#2346)' + ); + }); + + test('critical_rules contains no-re-read constraint', () => { + content = content || fs.readFileSync(agentPath, 'utf-8'); + const rulesStart = content.indexOf(''); + const rulesEnd = content.indexOf('', rulesStart); + assert.ok(rulesStart !== -1 && rulesEnd !== -1, ' block must be complete'); + const rulesBlock = content.slice(rulesStart, rulesEnd); + assert.ok( + rulesBlock.includes('re-read') || rulesBlock.includes('re read'), + 'critical_rules must include a no-re-read rule' + ); + }); + + test('critical_rules appears before success_criteria', () => { + content = content || fs.readFileSync(agentPath, 'utf-8'); + const rulesIdx = content.indexOf(''); + const successIdx = content.indexOf(''); + assert.ok(rulesIdx !== -1 && successIdx !== -1, 'both sections must exist'); + assert.ok( + rulesIdx < successIdx, + ' must appear before ' + ); + }); + }); + + describe('gsd-planner', () => { + const agentPath = path.join(AGENTS_DIR, 'gsd-planner.md'); + let content; + + test('agent file exists', () => { + assert.ok(fs.existsSync(agentPath), 'agents/gsd-planner.md must exist'); + content = fs.readFileSync(agentPath, 'utf-8'); + }); + + test('has block', () => { + content = content || fs.readFileSync(agentPath, 'utf-8'); + assert.ok( + content.includes(''), + 'gsd-planner.md must have a block to prevent unbounded read loops (#2346)' + ); + }); + + test('critical_rules contains no-re-read constraint', () => { + content = content || fs.readFileSync(agentPath, 'utf-8'); + const rulesStart = content.indexOf(''); + const rulesEnd = content.indexOf('', rulesStart); + assert.ok(rulesStart !== -1 && rulesEnd !== -1, ' block must be complete'); + const rulesBlock = content.slice(rulesStart, rulesEnd); + assert.ok( + rulesBlock.includes('re-read') || rulesBlock.includes('re read'), + 'critical_rules must include a no-re-read rule' + ); + }); + + test('critical_rules appears before success_criteria', () => { + content = content || fs.readFileSync(agentPath, 'utf-8'); + const rulesIdx = content.indexOf(''); + const successIdx = content.lastIndexOf(''); + assert.ok(rulesIdx !== -1 && successIdx !== -1, 'both sections must exist'); + assert.ok( + rulesIdx < successIdx, + ' must appear before ' + ); + }); + }); + +});