From cbc18f5e3e60ce82aa4bc1bf5328817fd438f7b8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 9 May 2026 19:27:36 -0400 Subject: [PATCH] fix: reconcile planner action contract (#3326) * fix: reconcile planner action contract The deep_work_rules workflow block was stronger than the planner agent contract: it required verbatim context copies and self-sufficient action text, so planners were incentivized to inline implementation code. Bound action content to directive prose with concrete identifiers, allow behavior/test acceptance criteria, and pin the cross-file contract with a regression test. Closes #3320. * chore: add changeset for planner contract fix --- .changeset/eager-elks-purr.md | 5 + agents/gsd-planner.md | 8 +- get-shit-done/workflows/plan-phase.md | 18 ++-- .../bug-3320-planner-deep-work-rules.test.cjs | 92 +++++++++++++++++++ 4 files changed, 112 insertions(+), 11 deletions(-) create mode 100644 .changeset/eager-elks-purr.md create mode 100644 tests/bug-3320-planner-deep-work-rules.test.cjs diff --git a/.changeset/eager-elks-purr.md b/.changeset/eager-elks-purr.md new file mode 100644 index 000000000..65b40fe4c --- /dev/null +++ b/.changeset/eager-elks-purr.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3326 +--- +Reconciled /gsd-plan-phase deep_work_rules with the gsd-planner action contract so planners keep action blocks as directive prose, avoid fenced implementation dumps, and allow behavior/test acceptance criteria alongside source assertions. (#3320) diff --git a/agents/gsd-planner.md b/agents/gsd-planner.md index b2f4a093f..68074d93e 100644 --- a/agents/gsd-planner.md +++ b/agents/gsd-planner.md @@ -198,8 +198,10 @@ Every task has four required fields: - Bad: "the auth files", "relevant components" **:** Specific implementation instructions, including what to avoid and WHY. -- Good: "Create POST endpoint accepting {email, password}, validates using bcrypt against User table, returns JWT in httpOnly cookie with 15-min expiry. Use jose library (not jsonwebtoken - CommonJS issues with Edge runtime)." +- Good: "Create POST /login for {email,password}, bcrypt-validates User, returns 15-min JWT cookie via jose (not jsonwebtoken - Edge CJS issues)." - Bad: "Add authentication", "Make login work" +- NEVER place fenced code blocks (```) inside ``. Action is directive prose, not implementation code. +- Code excerpts belong in `` source files or referenced context. Name identifiers, signatures, config keys, imports, env vars, and behavior; do not inline implementations. **:** How to prove the task is complete. @@ -213,9 +215,9 @@ Every task has four required fields: - Bad: "It works", "Looks good", manual-only verification - Simple format also accepted: `npm test` passes, `curl -X POST /api/auth/login` returns 200 -**Nyquist Rule:** Every `` must include an `` command. If no test exists yet, set `MISSING — Wave 0 must create {test_file} first` and create a Wave 0 task that generates the test scaffold. +**Nyquist Rule:** Every `` includes ``. If no test exists, set `MISSING — Wave 0 must create {test_file} first` and create that scaffold. -**Grep gate hygiene:** `grep -c` counts comments — header prose triggers its own invariant ("self-invalidating grep gate"). Use `grep -v '^#' | grep -c token`. Bare `== 0` gates on unfiltered files are forbidden. +**Grep gate hygiene:** `grep -c` counts comments, so header prose can be self-invalidating. Use `grep -v '^#' | grep -c token`. Bare `== 0` gates on unfiltered files are forbidden. **:** Acceptance criteria - measurable state of completion. - Good: "Valid credentials return 200 + JWT cookie, invalid credentials return 401" diff --git a/get-shit-done/workflows/plan-phase.md b/get-shit-done/workflows/plan-phase.md index 049c00811..10d268f80 100644 --- a/get-shit-done/workflows/plan-phase.md +++ b/get-shit-done/workflows/plan-phase.md @@ -878,22 +878,24 @@ Every task MUST include these fields — they are NOT optional: - Any file whose patterns, signatures, types, or conventions must be replicated or respected 2. **``** — Verifiable conditions that prove the task was done correctly. Rules: - - Every criterion must be checkable with grep, file read, test command, or CLI output + - Every criterion must be checkable as a source assertion, behavior assertion, test command, or CLI output - NEVER use subjective language ("looks correct", "properly configured", "consistent with") - - ALWAYS include exact strings, patterns, values, or command outputs that must be present + - Include exact strings, patterns, values, command outputs, or observable behavior where that is the right proof - Examples: - Code: `auth.py contains def verify_token(` / `test_auth.py exits 0` + - Behavior: `POST /api/auth/login returns 200 + httpOnly JWT cookie for valid credentials` - Config: `.env.example contains DATABASE_URL=` / `Dockerfile contains HEALTHCHECK` - Docs: `README.md contains '## Installation'` / `API.md lists all endpoints` - Infra: `deploy.yml has rollback step` / `docker-compose.yml has healthcheck for db` 3. **``** — Must include CONCRETE values, not references. Rules: - NEVER say "align X with Y", "match X to Y", "update to be consistent" without specifying the exact target state - - ALWAYS include the actual values: config keys, function signatures, SQL statements, class names, import paths, env vars, etc. - - If CONTEXT.md has a comparison table or expected values, copy them into the action verbatim - - The executor should be able to complete the task from the action text alone, without needing to read CONTEXT.md or reference files (read_first is for verification, not discovery) + - Include concrete identifiers and reference values: config keys, function signatures, SQL table names, class names, import paths, env vars, endpoint paths, etc. + - If CONTEXT.md has a comparison table or expected values, copy only the target identifiers/values needed to remove ambiguity + - Do not include full file contents, fenced code blocks, or complete implementations in `` + - The executor should understand the intended target state from `` and use `` files for current implementation details, patterns, and source-of-truth context -**Why this matters:** Executor agents work from the plan text. Vague instructions like "update the config to match production" produce shallow one-line changes. Concrete instructions like "add DATABASE_URL=postgresql://... , set POOL_SIZE=20, add REDIS_URL=redis://..." produce complete work. The cost of verbose plans is far less than the cost of re-doing shallow execution. +**Why this matters:** Executor agents work from the plan text. Vague instructions like "update the config to match production" produce shallow one-line changes. Concrete instructions like "add DATABASE_URL, set POOL_SIZE=20, add REDIS_URL, and read config/runtime.ts before editing" produce complete work without turning the planner into the executor. @@ -901,8 +903,8 @@ Every task MUST include these fields — they are NOT optional: - [ ] Each plan has valid frontmatter - [ ] Tasks are specific and actionable - [ ] Every task has `` with at least the file being modified -- [ ] Every task has `` with grep-verifiable conditions -- [ ] Every `` contains concrete values (no "align X with Y" without specifying what) +- [ ] Every task has `` with behavior, test-command, CLI, or source assertions +- [ ] Every `` contains concrete identifiers without fenced code blocks or full implementations - [ ] Dependencies correctly identified - [ ] Waves assigned for parallel execution - [ ] must_haves derived from phase goal diff --git a/tests/bug-3320-planner-deep-work-rules.test.cjs b/tests/bug-3320-planner-deep-work-rules.test.cjs new file mode 100644 index 000000000..3cc0027f5 --- /dev/null +++ b/tests/bug-3320-planner-deep-work-rules.test.cjs @@ -0,0 +1,92 @@ +'use strict'; + +// allow-test-rule: source-text-is-product [#3320] +// The bug is a contradiction in prompt/workflow source text. These assertions +// intentionally pin the contract words that planner agents consume. + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const ROOT = path.join(__dirname, '..'); +const PLANNER_AGENT = path.join(ROOT, 'agents', 'gsd-planner.md'); +const PLAN_PHASE_WORKFLOW = path.join(ROOT, 'get-shit-done', 'workflows', 'plan-phase.md'); + +function read(relativePath) { + return fs.readFileSync(path.join(ROOT, relativePath), 'utf8'); +} + +function extractDeepWorkRules() { + const workflow = fs.readFileSync(PLAN_PHASE_WORKFLOW, 'utf8'); + const match = workflow.match(/[\s\S]*?<\/deep_work_rules>/); + assert.ok(match, 'plan-phase.md must contain a deep_work_rules block'); + return match[0]; +} + +describe('bug #3320 planner action contract', () => { + test('planner agent explicitly keeps implementation code out of action blocks', () => { + const planner = fs.readFileSync(PLANNER_AGENT, 'utf8'); + + assert.match( + planner, + /NEVER place fenced code blocks \(```\) inside ``/, + 'gsd-planner.md must explicitly forbid fenced implementation code in ' + ); + assert.match( + planner, + /Code excerpts belong in `` source files or referenced context/, + 'gsd-planner.md must route code excerpts to context/read-first material' + ); + }); + + test('plan-phase deep_work_rules no longer requires self-sufficient code dumps', () => { + const deepWorkRules = extractDeepWorkRules(); + + assert.doesNotMatch( + deepWorkRules, + /copy them into the action verbatim/, + 'deep_work_rules must not tell planners to copy source material verbatim into ' + ); + assert.doesNotMatch( + deepWorkRules, + /complete the task from the action text alone/, + 'deep_work_rules must not make self-sufficient without read_first/context' + ); + assert.match( + deepWorkRules, + /Do not include full file contents, fenced code blocks, or complete implementations in ``/, + 'deep_work_rules must explicitly bound concrete values to avoid code dumping' + ); + }); + + test('plan-phase acceptance criteria allow behavior and test assertions', () => { + const deepWorkRules = extractDeepWorkRules(); + + assert.match( + deepWorkRules, + /behavior assertion/, + 'acceptance criteria must allow behavior assertions, not just grep checks' + ); + assert.match( + deepWorkRules, + /test command/, + 'acceptance criteria must allow test-command assertions' + ); + }); + + test('quality gate matches the reconciled planner contract', () => { + const workflow = read('get-shit-done/workflows/plan-phase.md'); + + assert.match( + workflow, + /Every task has `` with behavior, test-command, CLI, or source assertions/, + 'quality gate must not narrow acceptance criteria back to grep-only checks' + ); + assert.match( + workflow, + /Every `` contains concrete identifiers without fenced code blocks or full implementations/, + 'quality gate must enforce concrete prose without implementation dumps' + ); + }); +});