From a411e08e8839fa1cf97291229aa70be31bf55b0b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 5 May 2026 16:06:29 -0400 Subject: [PATCH] fix(coderabbit): resolve all 12 findings on PR #3152 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MAJOR (security/correctness): - commands/gsd/debug.md: add Write to allowed-tools (session file creation requires it — workflow explicitly says 'use Write tool, never heredoc') - workflows/debug.md: add SLUG sanitization guard to steps 1b+1c (status/ continue subcommands used raw user input in file paths — path traversal) - workflows/thread.md: sanitize $ARGUMENTS in RESUME mode before file path construction (was bypassing the sanitization guard in CLOSE/STATUS modes) MINOR (consistency/correctness): - docs/INVENTORY-MANIFEST.json: remove stale top-level 'workflows' array (duplicate of families.workflows introduced in earlier update) - commands/gsd/resume-work.md: normalize process to 'Execute end-to-end.' - commands/gsd/settings.md: normalize process to 'Execute end-to-end.' - commands/gsd/update.md: normalize otherwise branch to 'execute end-to-end.' - docs/adr/0002: add Status: Accepted + Date header (ADR convention) - workflows/extract-learnings.md: rename step extract_learnings → extract-learnings - tests/extract-learnings.test.cjs: tighten step-name assertion to exact name ARCHITECTURE: - scripts/command-contract-helpers.cjs: extract CANONICAL_TOOLS, parseFrontmatter, executionContextRefs as shared module — single source of truth consumed by both lint script and test suite (prevents silent lint/test disagreement) - scripts/lint-command-contract.cjs: require() helpers instead of duplicating - tests/command-contract.test.cjs: require() helpers; move readFileSync calls inside test() callbacks (registration-time throws surface as named failures) --- commands/gsd/debug.md | 1 + commands/gsd/resume-work.md | 14 +-- commands/gsd/settings.md | 10 +- commands/gsd/update.md | 11 +-- docs/INVENTORY-MANIFEST.json | 91 +------------------ ...0002-command-contract-validation-module.md | 3 + get-shit-done/workflows/debug.md | 4 + get-shit-done/workflows/extract-learnings.md | 2 +- get-shit-done/workflows/thread.md | 6 +- scripts/command-contract-helpers.cjs | 61 +++++++++++++ scripts/lint-command-contract.cjs | 57 +----------- scripts/strip-prose-atrefs.cjs | 11 ++- tests/command-contract.test.cjs | 68 +++----------- tests/extract-learnings.test.cjs | 4 + 14 files changed, 106 insertions(+), 237 deletions(-) create mode 100644 scripts/command-contract-helpers.cjs diff --git a/commands/gsd/debug.md b/commands/gsd/debug.md index 8a49d2de9..08416d9b9 100644 --- a/commands/gsd/debug.md +++ b/commands/gsd/debug.md @@ -4,6 +4,7 @@ description: Systematic debugging with persistent state across context resets argument-hint: [list | status | continue | --diagnose] [issue description] allowed-tools: - Read + - Write - Bash - Task - AskUserQuestion diff --git a/commands/gsd/resume-work.md b/commands/gsd/resume-work.md index e44f1c331..5ec33d911 100644 --- a/commands/gsd/resume-work.md +++ b/commands/gsd/resume-work.md @@ -26,15 +26,5 @@ Routes to the resume-project workflow which handles: -**Follow the resume-project workflow**. - -The workflow handles all resumption logic including: - -1. Project existence verification -2. STATE.md loading or reconstruction -3. Checkpoint and incomplete work detection -4. Visual status presentation -5. Context-aware option offering (checks CONTEXT.md before suggesting plan vs discuss) -6. Routing to appropriate next command -7. Session continuity updates - +Execute end-to-end. + diff --git a/commands/gsd/settings.md b/commands/gsd/settings.md index 3f5417391..ab5ec17d1 100644 --- a/commands/gsd/settings.md +++ b/commands/gsd/settings.md @@ -24,13 +24,5 @@ Routes to the settings workflow which handles: -**Follow the settings workflow**. - -The workflow handles all logic including: -1. Config file creation with defaults if missing -2. Current config reading -3. Interactive settings presentation with pre-selection -4. Answer parsing and config merging -5. File writing -6. Confirmation display +Execute end-to-end. diff --git a/commands/gsd/update.md b/commands/gsd/update.md index 5606e2607..445fd9e8e 100644 --- a/commands/gsd/update.md +++ b/commands/gsd/update.md @@ -38,17 +38,8 @@ Routes to the update workflow which handles: Parse the first token of $ARGUMENTS: - If it is `--sync`: strip the flag, execute the sync-skills workflow (passing remaining args for --from/--to/--dry-run/--apply). - If it is `--reapply`: strip the flag, execute the reapply-patches workflow. -- Otherwise: **Follow the update workflow**. +- Otherwise: execute the update workflow end-to-end. -The update workflow handles all logic including: -1. Installed version detection (local/global) -2. Latest version checking via npm -3. Version comparison -4. Changelog fetching and extraction -5. Clean install warning display -6. User confirmation -7. Update execution -8. Cache clearing diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index 6dbb9e84e..32490fadb 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -304,94 +304,5 @@ "gsd-validate-commit.sh", "gsd-workflow-guard.js" ] - }, - "workflows": [ - "add-backlog.md", - "add-phase.md", - "add-tests.md", - "add-todo.md", - "ai-integration-phase.md", - "analyze-dependencies.md", - "audit-fix.md", - "audit-milestone.md", - "audit-uat.md", - "autonomous.md", - "check-todos.md", - "cleanup.md", - "code-review-fix.md", - "code-review.md", - "complete-milestone.md", - "debug.md", - "diagnose-issues.md", - "discovery-phase.md", - "discuss-phase-assumptions.md", - "discuss-phase-power.md", - "discuss-phase.md", - "do.md", - "docs-update.md", - "edit-phase.md", - "eval-review.md", - "execute-phase.md", - "execute-plan.md", - "explore.md", - "extract-learnings.md", - "fast.md", - "forensics.md", - "graduation.md", - "health.md", - "help.md", - "import.md", - "inbox.md", - "ingest-docs.md", - "insert-phase.md", - "list-phase-assumptions.md", - "list-workspaces.md", - "manager.md", - "map-codebase.md", - "milestone-summary.md", - "new-milestone.md", - "new-project.md", - "new-workspace.md", - "next.md", - "node-repair.md", - "note.md", - "pause-work.md", - "plan-milestone-gaps.md", - "plan-phase.md", - "plan-review-convergence.md", - "plant-seed.md", - "pr-branch.md", - "profile-user.md", - "progress.md", - "quick.md", - "reapply-patches.md", - "remove-phase.md", - "remove-workspace.md", - "resume-project.md", - "review.md", - "scan.md", - "secure-phase.md", - "session-report.md", - "settings-advanced.md", - "settings-integrations.md", - "settings.md", - "ship.md", - "sketch-wrap-up.md", - "sketch.md", - "spec-phase.md", - "spike-wrap-up.md", - "spike.md", - "stats.md", - "sync-skills.md", - "thread.md", - "transition.md", - "ui-phase.md", - "ui-review.md", - "ultraplan-phase.md", - "undo.md", - "update.md", - "validate-phase.md", - "verify-phase.md", - "verify-work.md" - ] + } } diff --git a/docs/adr/0002-command-contract-validation-module.md b/docs/adr/0002-command-contract-validation-module.md index d6a36622e..093bfd86a 100644 --- a/docs/adr/0002-command-contract-validation-module.md +++ b/docs/adr/0002-command-contract-validation-module.md @@ -1,5 +1,8 @@ # Command Contract Validation Module +- **Status:** Accepted +- **Date:** 2026-05-05 + We decided to centralize the `commands/gsd/*.md` file contract into a single validation seam enforced at two layers: a fast lint script (`scripts/lint-command-contract.cjs`) that runs as a pre-test CI step, and a behavioral regression test (`tests/command-contract.test.cjs`) that validates the full contract against the live filesystem. ## Decision diff --git a/get-shit-done/workflows/debug.md b/get-shit-done/workflows/debug.md index f5f7b039b..4859b7ab3 100644 --- a/get-shit-done/workflows/debug.md +++ b/get-shit-done/workflows/debug.md @@ -64,6 +64,8 @@ STOP after displaying list. Do NOT proceed to further steps. When SUBCMD=status and SLUG is set: +**Sanitize SLUG first:** strip whitespace, reject unless it matches `^[a-z0-9][a-z0-9-]*$`, enforce max 30 chars, reject any `..`, `/`, or `\`. If invalid, print "No debug session found with slug: {SLUG}" and stop. + Check `.planning/debug/{SLUG}.md` exists. If not, check `.planning/debug/resolved/{SLUG}.md`. If neither, print "No debug session found with slug: {SLUG}" and stop. Parse and print full summary: @@ -81,6 +83,8 @@ No agent spawn. Just information display. STOP after printing. When SUBCMD=continue and SLUG is set: +**Sanitize SLUG first:** strip whitespace, reject unless it matches `^[a-z0-9][a-z0-9-]*$`, enforce max 30 chars, reject any `..`, `/`, or `\`. If invalid, print "No active debug session found with slug: {SLUG}. Check `/gsd-debug list` for active sessions." and stop. + Check `.planning/debug/{SLUG}.md` exists. If not, print "No active debug session found with slug: {SLUG}. Check `/gsd-debug list` for active sessions." and stop. Read file and print Current Focus block to console: diff --git a/get-shit-done/workflows/extract-learnings.md b/get-shit-done/workflows/extract-learnings.md index 059fbf0fc..27e1658e2 100644 --- a/get-shit-done/workflows/extract-learnings.md +++ b/get-shit-done/workflows/extract-learnings.md @@ -42,7 +42,7 @@ If PLAN.md or SUMMARY.md files are not found or missing, exit with error: "Requi Track which optional artifacts are missing for the `missing_artifacts` frontmatter field. - + Analyze all collected artifacts and extract learnings into 4 categories: ### 1. Decisions diff --git a/get-shit-done/workflows/thread.md b/get-shit-done/workflows/thread.md index af5b8eaa4..bb131c450 100644 --- a/get-shit-done/workflows/thread.md +++ b/get-shit-done/workflows/thread.md @@ -117,7 +117,11 @@ No agent spawn. STOP after printing. **RESUME mode:** -If $ARGUMENTS matches an existing thread name (file `.planning/threads/{ARGUMENTS}.md` exists): +If $ARGUMENTS matches an existing thread name: + +**Sanitize first:** apply the same slug sanitization used by CLOSE and STATUS — strip any characters not matching `[a-z0-9-]`, reject slugs longer than 60 chars or containing `..` or `/`. If invalid, output "Invalid thread slug." and stop. Use the sanitized value as SLUG for all subsequent file path construction. + +Check `.planning/threads/{SLUG}.md` exists. If not, fall through to CREATE mode. Resume the thread — load its context into the current session. Read the file content and display it as plain text. Ask what the user wants to work on next. diff --git a/scripts/command-contract-helpers.cjs b/scripts/command-contract-helpers.cjs new file mode 100644 index 000000000..f7080cac9 --- /dev/null +++ b/scripts/command-contract-helpers.cjs @@ -0,0 +1,61 @@ +'use strict'; +/** + * command-contract-helpers.cjs (ADR-0002) + * + * Single source of truth for the commands/gsd/*.md contract constants and + * parsers shared by scripts/lint-command-contract.cjs and + * tests/command-contract.test.cjs. + * + * Keeping these in one place ensures the lint script and the test suite + * always agree on what constitutes a valid tool, a valid @-ref, and a valid + * frontmatter structure. A new canonical tool added here is automatically + * enforced by both consumers. + */ + +const CANONICAL_TOOLS = new Set([ + 'Read', 'Write', 'Edit', 'Bash', 'Glob', 'Grep', + 'Task', 'Agent', 'Skill', 'SlashCommand', + 'AskUserQuestion', 'WebFetch', 'WebSearch', 'TodoWrite', + 'mcp__context7__resolve-library-id', + 'mcp__context7__query-docs', + 'mcp__context7__*', +]); + +function parseFrontmatter(content) { + const lines = content.split('\n'); + if (lines[0].trim() !== '---') return {}; + const end = lines.indexOf('---', 1); + if (end === -1) return {}; + const fm = {}; + let key = null; + for (const line of lines.slice(1, end)) { + const kv = line.match(/^([a-zA-Z0-9_-]+):\s*(.*)/); + if (kv) { key = kv[1]; fm[key] = kv[2].trim(); } + else if (key && line.match(/^\s+-\s+/)) { + const val = line.replace(/^\s+-\s+/, '').trim(); + fm[key] = fm[key] ? fm[key] + '\n' + val : val; + } + } + return fm; +} + +function executionContextRefs(content) { + const refs = []; + const re = /([\s\S]*?)<\/execution_context(?:_extended)?>/g; + let m; + while ((m = re.exec(content)) !== null) { + for (const rawLine of m[1].split('\n')) { + const line = rawLine.trim(); + if (!line.startsWith('@')) continue; + const token = line.split(/\s+/)[0]; + const trailingProse = line.length > token.length; + const normalized = token + .replace(/^@(?:~|\$HOME)\//, '') + .replace(/^(?:\.claude\/)?(?:get-shit-done\/)?/, ''); + refs.push({ token, normalized, trailingProse }); + } + } + return refs; +} + +module.exports = { CANONICAL_TOOLS, parseFrontmatter, executionContextRefs }; diff --git a/scripts/lint-command-contract.cjs b/scripts/lint-command-contract.cjs index 0c772350b..f4af95a8b 100644 --- a/scripts/lint-command-contract.cjs +++ b/scripts/lint-command-contract.cjs @@ -22,58 +22,11 @@ const ROOT = path.join(__dirname, '..'); const COMMANDS_DIR = path.join(ROOT, 'commands', 'gsd'); const GSD_ROOT = path.join(ROOT, 'get-shit-done'); -// All tool names the Claude Code / GSD runtime recognises. -// Wildcard entries (mcp__context7__*) match any mcp__context7__ prefixed name. -const CANONICAL_TOOLS = new Set([ - 'Read', 'Write', 'Edit', 'Bash', 'Glob', 'Grep', - 'Task', 'Agent', 'Skill', 'SlashCommand', - 'AskUserQuestion', 'WebFetch', 'WebSearch', 'TodoWrite', - 'mcp__context7__resolve-library-id', - 'mcp__context7__query-docs', - 'mcp__context7__*', -]); - -// ─── parsers ───────────────────────────────────────────────────────────────── - -function parseFrontmatter(content) { - const lines = content.split('\n'); - if (lines[0].trim() !== '---') return {}; - const end = lines.indexOf('---', 1); - if (end === -1) return {}; - const fm = {}; - let key = null; - for (const line of lines.slice(1, end)) { - const kv = line.match(/^([a-zA-Z0-9_-]+):\s*(.*)/); - if (kv) { key = kv[1]; fm[key] = kv[2].trim(); } - else if (key && line.match(/^\s+-\s+/)) { - const val = line.replace(/^\s+-\s+/, '').trim(); - fm[key] = fm[key] ? fm[key] + '\n' + val : val; - } - } - return fm; -} - -function extractExecutionContextRefs(content) { - const results = []; - const blockRe = /([\s\S]*?)<\/execution_context(?:_extended)?>/g; - let m; - while ((m = blockRe.exec(content)) !== null) { - const block = m[1]; - for (const rawLine of block.split('\n')) { - const line = rawLine.trim(); - if (!line.startsWith('@')) continue; - // Capture the @-reference token (stops at first space) - const refToken = line.split(/\s+/)[0]; - const hasTrailingProse = line.length > refToken.length; - // Normalise path: strip @~/.../get-shit-done/ or @$HOME/.../get-shit-done/ prefix - const normalized = refToken - .replace(/^@(?:~|\$HOME)\//, '') - .replace(/^(?:\.claude\/)?(?:get-shit-done\/)?/, ''); - results.push({ ref: refToken, normalized, hasTrailingProse, rawLine }); - } - } - return results; -} +const { + CANONICAL_TOOLS, + parseFrontmatter, + executionContextRefs: extractExecutionContextRefs, +} = require('./command-contract-helpers.cjs'); // ─── check one file ─────────────────────────────────────────────────────────── diff --git a/scripts/strip-prose-atrefs.cjs b/scripts/strip-prose-atrefs.cjs index 4c1194aad..b14032fb3 100644 --- a/scripts/strip-prose-atrefs.cjs +++ b/scripts/strip-prose-atrefs.cjs @@ -30,11 +30,11 @@ const DRY_RUN = process.argv.includes('--dry-run'); const ROOT = path.join(__dirname, '..'); const COMMANDS_DIR = path.join(ROOT, 'commands', 'gsd'); -const AT_PATH_RE = /@(?:~|\$HOME)\/.+?get-shit-done\/[^\s`\)]+/g; +const AT_PATH_PATTERN = /@(?:~|\$HOME)\/.+?get-shit-done\/[^\s`\)]+/; +const mkAtRe = () => new RegExp(AT_PATH_PATTERN.source, 'g'); function transformLine(line) { - if (!AT_PATH_RE.test(line)) return line; - AT_PATH_RE.lastIndex = 0; + if (!AT_PATH_PATTERN.test(line)) return line; const trimmed = line.trim(); @@ -76,8 +76,9 @@ function processFile(filePath) { if (/<(process|context)>/.test(t) && !t.includes('execution_context')) inProse = true; if (/<\/(process|context)>/.test(t) && !t.includes('execution_context')) inProse = false; - if (inProse && AT_PATH_RE.test(line)) { - AT_PATH_RE.lastIndex = 0; + if (inProse && AT_PATH_PATTERN.test(line)) { + const re = mkAtRe(); + re.lastIndex = 0; out.push(transformLine(line)); } else { out.push(line); diff --git a/tests/command-contract.test.cjs b/tests/command-contract.test.cjs index b3773a3c8..a023b91c4 100644 --- a/tests/command-contract.test.cjs +++ b/tests/command-contract.test.cjs @@ -27,53 +27,11 @@ const ROOT = path.join(__dirname, '..'); const COMMANDS_DIR = path.join(ROOT, 'commands', 'gsd'); const GSD_ROOT = path.join(ROOT, 'get-shit-done'); -const CANONICAL_TOOLS = new Set([ - 'Read', 'Write', 'Edit', 'Bash', 'Glob', 'Grep', - 'Task', 'Agent', 'Skill', 'SlashCommand', - 'AskUserQuestion', 'WebFetch', 'WebSearch', 'TodoWrite', - 'mcp__context7__resolve-library-id', - 'mcp__context7__query-docs', - 'mcp__context7__*', -]); - -// ─── helpers ───────────────────────────────────────────────────────────────── - -function parseFrontmatter(content) { - const lines = content.split('\n'); - if (lines[0].trim() !== '---') return {}; - const end = lines.indexOf('---', 1); - if (end === -1) return {}; - const fm = {}; - let key = null; - for (const line of lines.slice(1, end)) { - const kv = line.match(/^([a-zA-Z0-9_-]+):\s*(.*)/); - if (kv) { key = kv[1]; fm[key] = kv[2].trim(); } - else if (key && line.match(/^\s+-\s+/)) { - const val = line.replace(/^\s+-\s+/, '').trim(); - fm[key] = fm[key] ? fm[key] + '\n' + val : val; - } - } - return fm; -} - -function executionContextRefs(content) { - const refs = []; - const re = /([\s\S]*?)<\/execution_context(?:_extended)?>/g; - let m; - while ((m = re.exec(content)) !== null) { - for (const rawLine of m[1].split('\n')) { - const line = rawLine.trim(); - if (!line.startsWith('@')) continue; - const token = line.split(/\s+/)[0]; - const trailingProse = line.length > token.length; - const normalized = token - .replace(/^@(?:~|\$HOME)\//, '') - .replace(/^(?:\.claude\/)?(?:get-shit-done\/)?/, ''); - refs.push({ token, normalized, trailingProse }); - } - } - return refs; -} +const { + CANONICAL_TOOLS, + parseFrontmatter, + executionContextRefs, +} = require('../scripts/command-contract-helpers.cjs'); const commandFiles = fs .readdirSync(COMMANDS_DIR) @@ -128,27 +86,23 @@ describe('command contract: allowed-tools (ADR-0002)', () => { describe('command contract: execution_context @-refs resolve (ADR-0002)', () => { for (const { name, full } of commandFiles) { - const content = fs.readFileSync(full, 'utf-8'); - const refs = executionContextRefs(content); - if (refs.length === 0) continue; - for (const { token, normalized } of refs) { - test(`${name}: @-ref "${normalized}" exists on disk`, () => { + test(`${name}: all execution_context @-refs exist on disk`, () => { + const refs = executionContextRefs(fs.readFileSync(full, 'utf-8')); + for (const { normalized } of refs) { assert.ok( fs.existsSync(path.join(GSD_ROOT, normalized)), `${name}: execution_context @-ref "${normalized}" does not exist — ` + 'create the file or remove the reference', ); - }); - } + } + }); } }); describe('command contract: execution_context @-refs on own line (ADR-0002)', () => { for (const { name, full } of commandFiles) { - const content = fs.readFileSync(full, 'utf-8'); - const refs = executionContextRefs(content); - if (refs.length === 0) continue; test(`${name}: no @-refs with trailing prose in execution_context`, () => { + const refs = executionContextRefs(fs.readFileSync(full, 'utf-8')); const bad = refs.filter(r => r.trailingProse); assert.equal( bad.length, 0, diff --git a/tests/extract-learnings.test.cjs b/tests/extract-learnings.test.cjs index a679b8578..91dcd1ae1 100644 --- a/tests/extract-learnings.test.cjs +++ b/tests/extract-learnings.test.cjs @@ -86,6 +86,10 @@ describe('extract-learnings workflow', () => { const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); assert.ok(content.includes(''), 'Workflow must close step tags'); + assert.ok( + content.includes(''), + 'Workflow step must use hyphen convention: ', + ); }); test('workflow has success_criteria tag', () => {