fix(agents): add no-re-read critical rules to ui-checker and planner (#2346) (#2355)

* fix(agents): add no-re-read critical rules to ui-checker and planner (#2346)

Closes #2346

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

agents/gsd-planner.md

@@ -36,13 +36,7 @@ If the prompt contains a `<required_reading>` block, you MUST use the `Read` too
 </role>
 
 <documentation_lookup>
-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 <name> "<query>"   # resolve library ID
-npx --yes ctx7@latest docs <libraryId> "<query>" # 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 <name> "<query>"` then `npx --yes ctx7@latest docs <libraryId> "<query>"`). The CLI fallback works via Bash when MCP is unavailable.
 </documentation_lookup>
 
 <project_context>
@@ -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.
 </project_context>
 
 <context_fidelity>
@@ -67,18 +54,11 @@ The orchestrator provides user decisions in `<user_decisions>` 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 `<planner_authority_limits>`)
-- Complexity/difficulty as scope justification (see `<planner_authority_limits>`)
-- 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.
 
 </philosophy>
 
@@ -1224,6 +1199,15 @@ Follow templates in checkpoints and revision_mode sections respectively.
 
 </structured_returns>
 
+<critical_rules>
+
+- **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')`.
+
+</critical_rules>
+
 <success_criteria>
 
 ## Standard Mode

agents/gsd-ui-checker.md

@@ -277,6 +277,15 @@ Fix blocking issues in UI-SPEC.md and re-run `/gsd-ui-phase`.
 
 </structured_returns>
 
+<critical_rules>
+
+- **No re-reads:** Once a file is loaded via `<required_reading>` 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.
+
+</critical_rules>
+
 <success_criteria>
 
 Verification is complete when:

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 <critical_rules> 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 <critical_rules> block', () => {
+      content = content || fs.readFileSync(agentPath, 'utf-8');
+      assert.ok(
+        content.includes('<critical_rules>'),
+        'gsd-ui-checker.md must have a <critical_rules> 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('<critical_rules>');
+      const rulesEnd = content.indexOf('</critical_rules>', rulesStart);
+      assert.ok(rulesStart !== -1 && rulesEnd !== -1, '<critical_rules> 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('<critical_rules>');
+      const successIdx = content.indexOf('<success_criteria>');
+      assert.ok(rulesIdx !== -1 && successIdx !== -1, 'both sections must exist');
+      assert.ok(
+        rulesIdx < successIdx,
+        '<critical_rules> must appear before <success_criteria>'
+      );
+    });
+  });
+
+  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 <critical_rules> block', () => {
+      content = content || fs.readFileSync(agentPath, 'utf-8');
+      assert.ok(
+        content.includes('<critical_rules>'),
+        'gsd-planner.md must have a <critical_rules> 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('<critical_rules>');
+      const rulesEnd = content.indexOf('</critical_rules>', rulesStart);
+      assert.ok(rulesStart !== -1 && rulesEnd !== -1, '<critical_rules> 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('<critical_rules>');
+      const successIdx = content.lastIndexOf('<success_criteria>');
+      assert.ok(rulesIdx !== -1 && successIdx !== -1, 'both sections must exist');
+      assert.ok(
+        rulesIdx < successIdx,
+        '<critical_rules> must appear before <success_criteria>'
+      );
+    });
+  });
+
+});