From 0afcea07237aecdc20bf8957ab74b3dd0e915bfe Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 10 May 2026 11:19:50 -0400 Subject: [PATCH] fix: block verifier pass on unresolved debt markers (#3343) * fix: block verifier pass on unresolved debt markers * chore: add changeset for verifier debt gate * test: align verifier debt cleanup with standards * fix: address coderabbit verifier debt findings * fix: address follow-up coderabbit guard findings * fix: tighten debt marker matching * fix: ignore deleted files in debt scan * docs: document debt scan path contract * fix: harden debt scan path handling * fix: tighten debt marker reference parsing * fix: clarify debt scan failure logging * fix: preserve verifier debt error contract --- .changeset/verifier-debt-gate.md | 7 + agents/gsd-verifier.md | 10 +- get-shit-done/workflows/verify-phase.md | 3 +- sdk/src/phase-runner.test.ts | 375 +++++++++++++++++++++++- sdk/src/phase-runner.ts | 231 ++++++++++++++- 5 files changed, 609 insertions(+), 17 deletions(-) create mode 100644 .changeset/verifier-debt-gate.md diff --git a/.changeset/verifier-debt-gate.md b/.changeset/verifier-debt-gate.md new file mode 100644 index 000000000..728505fc9 --- /dev/null +++ b/.changeset/verifier-debt-gate.md @@ -0,0 +1,7 @@ +--- +type: Fixed +pr: 3343 +--- +**Phase verification no longer passes with unresolved `TBD`/`FIXME`/`XXX` markers** — the SDK phase runner now blocks advance after a nominal verifier pass when phase-modified source files contain untracked debt markers. Same-line issue/PR references and `DEF-*` IDs remain allowed for formal deferrals. + +The debt scan covers literal source paths declared in phase plan `files_modified` frontmatter and task `files`; globs are not expanded, and undeclared files modified during execution are not scanned. Git-diff-based coverage would be a separate enhancement. diff --git a/agents/gsd-verifier.md b/agents/gsd-verifier.md index d6999639e..2c85292b3 100644 --- a/agents/gsd-verifier.md +++ b/agents/gsd-verifier.md @@ -422,8 +422,10 @@ grep -E "^\- \`" "$PHASE_DIR"/*-SUMMARY.md | sed 's/.*`\([^`]*\)`.*/\1/' | sort Run anti-pattern detection on each file: ```bash -# TODO/FIXME/placeholder comments -grep -n -E "TODO|FIXME|XXX|HACK|PLACEHOLDER" "$file" 2>/dev/null +# Debt-marker comments +grep -n -E "TBD|FIXME|XXX" "$file" 2>/dev/null +# Warning-level cleanup comments +grep -n -E "TODO|HACK|PLACEHOLDER" "$file" 2>/dev/null grep -n -E "placeholder|coming soon|will be here|not yet implemented|not available" "$file" -i 2>/dev/null # Empty implementations grep -n -E "return null|return \{\}|return \[\]|=> \{\}" "$file" 2>/dev/null @@ -437,7 +439,9 @@ grep -n -B 2 -A 2 "console\.log" "$file" 2>/dev/null | grep -E "^\s*(const|funct **Stub classification:** A grep match is a STUB only when the value flows to rendering or user-visible output AND no other code path populates it with real data. A test helper, type default, or initial state that gets overwritten by a fetch/store is NOT a stub. Check for data-fetching (useEffect, fetch, query, useSWR, useQuery, subscribe) that writes to the same variable before flagging. -Categorize: 🛑 Blocker (prevents goal) | ⚠ī¸ Warning (incomplete) | ℹī¸ Info (notable) +**Debt marker gate:** Any `TBD`, `FIXME`, or `XXX` marker in a file modified by this phase is a 🛑 BLOCKER unless the same line references formal follow-up work (`issue #123`, `PR #123`, `#123`, or `DEF-*`). Unreferenced markers mean completion is not auditable; set `status: gaps_found` and list each marker under `gaps`. + +Categorize: 🛑 Blocker (prevents goal or unresolved debt marker) | ⚠ī¸ Warning (incomplete) | ℹī¸ Info (notable) ## Step 7b: Behavioral Spot-Checks diff --git a/get-shit-done/workflows/verify-phase.md b/get-shit-done/workflows/verify-phase.md index 04d051496..fe2ae3a88 100644 --- a/get-shit-done/workflows/verify-phase.md +++ b/get-shit-done/workflows/verify-phase.md @@ -329,7 +329,8 @@ Extract files modified in this phase from SUMMARY.md, scan each: | Pattern | Search | Severity | |---------|--------|----------| -| TODO/FIXME/XXX/HACK | `grep -n -E "TODO\|FIXME\|XXX\|HACK"` | ⚠ī¸ Warning | +| TBD/FIXME/XXX without same-line `issue #123`, `PR #123`, `#123`, or `DEF-*` reference | `grep -n -e TBD -e FIXME -e XXX` | 🛑 Blocker | +| TODO/HACK | `grep -n -e TODO -e HACK` | ⚠ī¸ Warning | | Placeholder content | `grep -n -iE "placeholder\|coming soon\|will be here"` | 🛑 Blocker | | Empty returns | `grep -n -E "return null\|return \{\}\|return \[\]\|=> \{\}"` | ⚠ī¸ Warning | | Log-only functions | Functions containing only console.log | ⚠ī¸ Warning | diff --git a/sdk/src/phase-runner.test.ts b/sdk/src/phase-runner.test.ts index 19fe502f6..398b5b9b0 100644 --- a/sdk/src/phase-runner.test.ts +++ b/sdk/src/phase-runner.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; +import { mkdtemp, mkdir, writeFile, rm, symlink } from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { PhaseRunner, PhaseRunnerError } from './phase-runner.js'; @@ -39,15 +39,20 @@ vi.mock('./plan-parser.js', () => ({ })); import { runPhaseStepSession } from './session-runner.js'; +import { parsePlanFile } from './plan-parser.js'; const mockRunPhaseStepSession = vi.mocked(runPhaseStepSession); +const mockParsePlanFile = vi.mocked(parsePlanFile); // ─── Factory helpers ───────────────────────────────────────────────────────── +let defaultProjectDir = '/tmp/project'; +const defaultPhaseDir = '.planning/phases/01-auth'; + function makePhaseOp(overrides: Partial = {}): PhaseOpInfo { return { phase_found: true, - phase_dir: '/tmp/project/.planning/phases/01-auth', + phase_dir: defaultPhaseDir, phase_number: '1', phase_name: 'Authentication', phase_slug: 'auth', @@ -60,8 +65,8 @@ function makePhaseOp(overrides: Partial = {}): PhaseOpInfo { roadmap_exists: true, planning_exists: true, commit_docs: true, - context_path: '/tmp/project/.planning/phases/01-auth/CONTEXT.md', - research_path: '/tmp/project/.planning/phases/01-auth/RESEARCH.md', + context_path: join(defaultProjectDir, defaultPhaseDir, 'CONTEXT.md'), + research_path: join(defaultProjectDir, defaultPhaseDir, 'RESEARCH.md'), ...overrides, }; } @@ -100,6 +105,27 @@ function makePlanInfo(overrides: Partial = {}): PlanInfo { }; } +function makeParsedPlan(filesModified: string[] = []) { + return { + frontmatter: { + phase: '01-auth', + plan: '01', + type: 'execute', + wave: 1, + depends_on: [], + files_modified: filesModified, + autonomous: true, + requirements: [], + must_haves: { truths: [], artifacts: [], key_links: [] }, + }, + objective: 'Test plan objective', + execution_context: [], + context_refs: [], + tasks: [{ name: 'Test task', type: 'auto', files: [], read_first: [], action: 'do the thing', verify: 'check it', done: 'done', acceptance_criteria: [] }], + raw: '', + }; +} + function makePlanIndex(planCount: number, overrides: Partial = {}): PhasePlanIndex { const plans: PlanInfo[] = []; const waves: Record = {}; @@ -136,7 +162,7 @@ function makeDeps(overrides: Partial = {}): PhaseRunnerDeps { const events: GSDEvent[] = []; return { - projectDir: '/tmp/project', + projectDir: defaultProjectDir, tools: { initPhaseOp: vi.fn().mockResolvedValue(makePhaseOp()), phaseComplete: vi.fn().mockResolvedValue(undefined), @@ -183,9 +209,21 @@ function getEmittedEvents(deps: PhaseRunnerDeps): GSDEvent[] { // ─── Tests ─────────────────────────────────────────────────────────────────── describe('PhaseRunner', () => { - beforeEach(() => { + let tempProjectDirs: string[] = []; + + beforeEach(async () => { + tempProjectDirs = []; + defaultProjectDir = await mkdtemp(join(tmpdir(), 'gsd-phase-runner-default-')); + tempProjectDirs.push(defaultProjectDir); + await mkdir(join(defaultProjectDir, defaultPhaseDir), { recursive: true }); + await writeFile(join(defaultProjectDir, defaultPhaseDir, '01-PLAN.md'), '---\nfiles_modified: []\n---\n', 'utf-8'); vi.clearAllMocks(); mockRunPhaseStepSession.mockResolvedValue(makePlanResult()); + mockParsePlanFile.mockResolvedValue(makeParsedPlan()); + }); + + afterEach(async () => { + await Promise.all(tempProjectDirs.map((dir) => rm(dir, { recursive: true, force: true }))); }); // ─── Happy path ──────────────────────────────────────────────────────── @@ -728,6 +766,331 @@ Use TypeScript.`, 'utf-8'); expect(verifyStep?.error).toBe('verification_gaps_found'); }); + it('keeps phase pending when changed phase files contain unresolved TBD/FIXME/XXX markers', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-architectural-debt-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\n# TBD: wire retry handling before release\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + expect(result.steps.map(s => s.step)).not.toContain(PhaseStepType.Advance); + + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.success).toBe(false); + expect(verifyStep?.error).toBe('verification_gaps_found'); + }); + + it('allows changed-file debt markers when they reference tracked follow-up work', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-tracked-debt-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\n# FIXME(issue #3322): preserve upstream retry behavior\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(true); + expect(deps.tools.phaseComplete).toHaveBeenCalledWith('1'); + expect(result.steps.map(s => s.step)).toContain(PhaseStepType.Advance); + }); + + it('allows changed-file entries for files deleted by the phase', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-deleted-file-debt-scan-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + await mkdir(phaseDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/deleted.sh"]\n---\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/deleted.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(true); + expect(deps.tools.phaseComplete).toHaveBeenCalledWith('1'); + expect(result.steps.map(s => s.step)).toContain(PhaseStepType.Advance); + }); + + it('allows dotted lowercase xxx placeholder text when scanning debt markers', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-lowercase-placeholder-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\napi_host="xxx.example.test"\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(true); + expect(deps.tools.phaseComplete).toHaveBeenCalledWith('1'); + expect(result.steps.map(s => s.step)).toContain(PhaseStepType.Advance); + }); + + it('keeps phase pending when changed files contain lowercase debt markers', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-lowercase-debt-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\n# fixme: wire retry handling before release\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.error).toBe('verification_gaps_found'); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + }); + + it('keeps phase pending when debt markers are followed by punctuation', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-punctuated-debt-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\n# FIXME. remove before release\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.error).toBe('verification_gaps_found'); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + }); + + it('allows bare hash issue references when they read as references', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-bare-hash-ref-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\n# FIXME tracked in #3322\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(true); + expect(deps.tools.phaseComplete).toHaveBeenCalledWith('1'); + expect(result.steps.map(s => s.step)).toContain(PhaseStepType.Advance); + }); + + it('does not treat quoted numeric fragments as debt references', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-hex-fragment-debt-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\ncolor="#123" # FIXME temp styling\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.error).toBe('verification_gaps_found'); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + }); + + it('does not allow unrelated earlier issue text to satisfy a later debt marker', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-unrelated-debt-ref-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\nlabel="issue #123"; # FIXME temp styling\n', 'utf-8'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.error).toBe('verification_gaps_found'); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + }); + + it('reports one unresolved debt finding per line', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-duplicate-debt-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const sourceDir = join(projectDir, 'scripts', 'upstream'); + await mkdir(phaseDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["scripts/upstream/run.sh"]\n---\n', 'utf-8'); + await writeFile(join(sourceDir, 'run.sh'), '#!/usr/bin/env bash\n# TBD TBD before release\n', 'utf-8'); + + const logger = { warn: vi.fn(), info: vi.fn(), debug: vi.fn() } as any; + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config, logger }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['scripts/upstream/run.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + const blockCall = logger.warn.mock.calls.find(([message]: [string]) => message.includes('Verification blocked')); + expect(blockCall?.[1].findings).toHaveLength(1); + expect(blockCall?.[1].findings[0]).toMatchObject({ + file: 'scripts/upstream/run.sh', + line: 2, + marker: 'TBD', + }); + expect(blockCall?.[1].findings[0]).not.toHaveProperty('text'); + }); + + it('keeps phase pending when a declared file resolves through a symlink outside the project', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-symlink-project-')); + const externalDir = await mkdtemp(join(tmpdir(), 'gsd-symlink-external-')); + tempProjectDirs.push(projectDir, externalDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + await mkdir(phaseDir, { recursive: true }); + await writeFile(join(phaseDir, '01-PLAN.md'), '---\nfiles_modified: ["linked-outside/secret.sh"]\n---\n', 'utf-8'); + await writeFile(join(externalDir, 'secret.sh'), '#!/usr/bin/env bash\necho safe\n', 'utf-8'); + await symlink(externalDir, join(projectDir, 'linked-outside'), 'dir'); + + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + mockParsePlanFile.mockResolvedValue(makeParsedPlan(['linked-outside/secret.sh'])); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.error).toBe('verification_gaps_found'); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + }); + + it('does not advance when verification status cannot be checked', async () => { + const phaseOp = makePhaseOp({ has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + (deps.tools.exec as ReturnType).mockImplementation((cmd: string) => { + if (cmd === 'check.verification-status') return Promise.reject(new Error('status parser crashed')); + return Promise.resolve(undefined); + }); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + expect(result.steps.map(s => s.step)).not.toContain(PhaseStepType.Advance); + + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.success).toBe(false); + expect(verifyStep?.error).toBe('verification_gaps_found'); + expect(mockRunPhaseStepSession.mock.calls.filter((call) => call[1] === PhaseStepType.Plan)).toHaveLength(1); + expect(mockRunPhaseStepSession.mock.calls.filter((call) => call[1] === PhaseStepType.Execute)).toHaveLength(1); + }); + + it('keeps phase pending when plan files cannot be listed for the debt scan', async () => { + const projectDir = await mkdtemp(join(tmpdir(), 'gsd-debt-missing-plans-')); + tempProjectDirs.push(projectDir); + const phaseDir = join(projectDir, '.planning', 'phases', '01-auth'); + const logger = { warn: vi.fn(), info: vi.fn(), debug: vi.fn() } as any; + const phaseOp = makePhaseOp({ phase_dir: phaseDir, has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ projectDir, config, logger }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + expect(result.success).toBe(false); + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + expect(result.steps.map(s => s.step)).not.toContain(PhaseStepType.Advance); + + const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); + expect(verifyStep?.success).toBe(false); + expect(verifyStep?.error).toBe('verification_gaps_found'); + expect(logger.warn.mock.calls.some(([message]: [string]) => message.includes('unresolved architectural debt markers'))).toBe(false); + expect(logger.warn.mock.calls.some(([message]: [string]) => message.includes('architectural debt scan could not complete'))).toBe(true); + }); + it('halts when verification review callback rejects', async () => { const onVerificationReview = vi.fn().mockResolvedValue('reject'); const phaseOp = makePhaseOp({ has_context: true, has_plans: true, plan_count: 1 }); diff --git a/sdk/src/phase-runner.ts b/sdk/src/phase-runner.ts index 748925fd5..078b4b567 100644 --- a/sdk/src/phase-runner.ts +++ b/sdk/src/phase-runner.ts @@ -27,8 +27,9 @@ import type { ContextEngine } from './context-engine.js'; import type { GSDLogger } from './logger.js'; import { runPhaseStepSession, runPlanSession } from './session-runner.js'; import { parsePlanFile } from './plan-parser.js'; -import { readFile } from 'node:fs/promises'; -import { join } from 'node:path'; +import { realpathSync } from 'node:fs'; +import { readdir, readFile } from 'node:fs/promises'; +import { basename, dirname, isAbsolute, join, relative, resolve } from 'node:path'; import { checkResearchGate } from './research-gate.js'; // ─── Error type ────────────────────────────────────────────────────────────── @@ -47,7 +48,22 @@ export class PhaseRunnerError extends Error { // ─── Verification result enum ──────────────────────────────────────────────── -export type VerificationOutcome = 'passed' | 'human_needed' | 'gaps_found'; +export type VerificationOutcome = 'passed' | 'human_needed' | 'gaps_found' | 'architectural_debt' | 'status_unreadable'; + +interface ArchitecturalDebtFinding { + file: string; + line: number; + marker: string; + text: string; +} + +type ArchitecturalDebtCheckReason = 'markers_found' | 'scan_error'; + +interface ArchitecturalDebtCheck { + pass: boolean; + findings: ArchitecturalDebtFinding[]; + reason?: ArchitecturalDebtCheckReason; +} // ─── PhaseRunner deps interface ────────────────────────────────────────────── @@ -889,6 +905,21 @@ export class PhaseRunner { outcome = await this.parseVerificationOutcome(lastResult, phaseNumber); if (outcome === 'passed') { + const debtCheck = await this.checkArchitecturalDebt(phaseNumber); + if (!debtCheck.pass) { + const message = + debtCheck.reason === 'scan_error' + ? `Verification blocked because architectural debt scan could not complete for phase ${phaseNumber}` + : `Verification blocked by unresolved architectural debt markers in phase ${phaseNumber}`; + this.logger?.warn(message, { + phase: phaseNumber, + reason: debtCheck.reason, + findingCount: debtCheck.findings.length, + findings: debtCheck.findings.map(({ file, line, marker }) => ({ file, line, marker })), + }); + outcome = 'architectural_debt'; + break; + } break; } @@ -929,6 +960,10 @@ export class PhaseRunner { } } + if (outcome === 'status_unreadable') { + break; + } + if (outcome === 'gaps_found') { if (gapRetryCount < maxGapRetries) { gapRetryCount++; @@ -986,7 +1021,7 @@ export class PhaseRunner { step: PhaseStepType.Verify, success: verifySuccess, durationMs, - ...(!verifySuccess && { error: `verification_${outcome}` }), + ...(!verifySuccess && { error: this.verificationErrorForOutcome(outcome) }), }); return { @@ -994,7 +1029,7 @@ export class PhaseRunner { success: verifySuccess, durationMs, planResults: allPlanResults, - ...(!verifySuccess && { error: `verification_${outcome}` }), + ...(!verifySuccess && { error: this.verificationErrorForOutcome(outcome) }), }; } @@ -1149,9 +1184,191 @@ export class PhaseRunner { this.logger?.warn(`Unknown verification status '${status}' for phase ${phaseNumber}, treating as gaps_found`); return 'gaps_found'; } catch (err) { - // Can't parse VERIFICATION.md — fall back to session result + // Can't parse VERIFICATION.md — fail closed so a missing/broken status check never completes the phase. this.logger?.warn(`Could not check verification status for phase ${phaseNumber}: ${err instanceof Error ? err.message : String(err)}`); - return 'passed'; + return 'status_unreadable'; + } + } + + private verificationErrorForOutcome(outcome: VerificationOutcome): string { + if (outcome === 'status_unreadable' || outcome === 'architectural_debt') return 'verification_gaps_found'; + return `verification_${outcome}`; + } + + /** + * Block phase completion when source files changed by this phase still contain + * unresolved TBD/FIXME/XXX comments. Markers are allowed only when the same + * line references tracked follow-up work (issue/PR number or DEF-* id). + * + * The debt scan is intentionally scoped to literal source paths declared in + * phase plan frontmatter `files_modified` and task `files`. Glob patterns are + * not expanded, and files modified during execution but omitted from the plan + * are not scanned; git-diff-based coverage would be a separate enhancement. + */ + private async checkArchitecturalDebt(phaseNumber: string): Promise { + let phaseOp: PhaseOpInfo; + try { + phaseOp = await this.tools.initPhaseOp(phaseNumber); + } catch (err) { + this.logger?.warn(`Could not initialize phase ${phaseNumber} for architectural debt check: ${err instanceof Error ? err.message : String(err)}`); + return { pass: false, findings: [], reason: 'scan_error' }; + } + + let planPaths: string[]; + try { + planPaths = await this.listPhasePlanPaths(phaseOp.phase_dir); + } catch { + return { pass: false, findings: [], reason: 'scan_error' }; + } + if (phaseOp.has_plans && planPaths.length === 0) { + this.logger?.warn(`No phase plans found for architectural debt check in phase ${phaseNumber}`); + return { pass: false, findings: [], reason: 'scan_error' }; + } + const filesToScan = new Set(); + + for (const planPath of planPaths) { + try { + const parsedPlan = await parsePlanFile(planPath); + for (const file of this.extractPlanFiles(parsedPlan)) { + if (this.shouldScanForArchitecturalDebt(file)) { + filesToScan.add(file); + } + } + } catch (err) { + this.logger?.warn(`Could not parse plan for architectural debt check (${planPath}): ${err instanceof Error ? err.message : String(err)}`); + return { pass: false, findings: [], reason: 'scan_error' }; + } + } + + const findings: ArchitecturalDebtFinding[] = []; + for (const file of filesToScan) { + const absolutePath = this.resolveProjectPath(file); + if (!absolutePath) { + findings.push({ file, line: 0, marker: 'path', text: 'File is outside the project root' }); + continue; + } + + try { + const content = await readFile(absolutePath, 'utf-8'); + findings.push(...this.findUnresolvedDebtMarkers(file, content)); + } catch (err) { + const code = typeof err === 'object' && err !== null && 'code' in err ? (err as { code?: unknown }).code : undefined; + if (code === 'ENOENT') { + continue; + } + + findings.push({ + file, + line: 0, + marker: 'read', + text: err instanceof Error ? err.message : String(err), + }); + } + } + + const hasDebtMarkers = findings.some(({ marker }) => marker !== 'path' && marker !== 'read'); + return { + pass: findings.length === 0, + findings, + reason: findings.length === 0 ? undefined : hasDebtMarkers ? 'markers_found' : 'scan_error', + }; + } + + private async listPhasePlanPaths(phaseDir: string): Promise { + const absolutePhaseDir = this.resolveProjectPath(phaseDir); + if (!absolutePhaseDir) { + const err = new Error(`Phase directory is outside the project root: ${phaseDir}`); + this.logger?.warn(err.message); + throw err; + } + + try { + const entries = await readdir(absolutePhaseDir, { withFileTypes: true }); + return entries + .filter((entry) => entry.isFile() && (entry.name === 'PLAN.md' || entry.name.endsWith('-PLAN.md'))) + .map((entry) => join(absolutePhaseDir, entry.name)); + } catch (err) { + this.logger?.warn(`Could not list phase plans for architectural debt check (${phaseDir}): ${err instanceof Error ? err.message : String(err)}`); + throw err; + } + } + + private extractPlanFiles(parsedPlan: ParsedPlan): string[] { + const files = new Set(); + for (const file of parsedPlan.frontmatter.files_modified ?? []) { + files.add(file); + } + for (const task of parsedPlan.tasks ?? []) { + for (const file of task.files ?? []) { + files.add(file); + } + } + return [...files]; + } + + private shouldScanForArchitecturalDebt(file: string): boolean { + return !/\.(md|markdown)$/i.test(file); + } + + private findUnresolvedDebtMarkers(file: string, content: string): ArchitecturalDebtFinding[] { + const findings: ArchitecturalDebtFinding[] = []; + const markerPattern = /(?:^|[^\w.])(TBD|FIXME|XXX)(?=\b(?:\.(?:\s|$)|[^\w.]|$))/i; + const lines = content.split(/\r?\n/); + + lines.forEach((line, index) => { + const match = markerPattern.exec(line); + if (match) { + const markerSegment = line.slice(match.index); + if (this.hasFormalDebtReference(markerSegment)) return; + + findings.push({ + file, + line: index + 1, + marker: match[1].toUpperCase(), + text: line.trim(), + }); + } + }); + + return findings; + } + + private hasFormalDebtReference(line: string): boolean { + return /\bDEF-[A-Z0-9-]+\b/i.test(line) || /\b(?:issue|issues|pr|pull request)\s+#?\d+\b/i.test(line) || /(?:^|\s)#\d+\b/.test(line); + } + + private resolveProjectPath(pathValue: string): string | undefined { + const root = this.realpathForBoundary(resolve(this.projectDir)); + if (!root) return undefined; + + const absolutePath = isAbsolute(pathValue) ? resolve(pathValue) : resolve(this.projectDir, pathValue); + const canonicalPath = this.realpathForBoundary(absolutePath); + if (!canonicalPath) return undefined; + + const relativePath = relative(root, canonicalPath); + if (relativePath === '' || (!relativePath.startsWith('..') && !isAbsolute(relativePath))) { + return canonicalPath; + } + return undefined; + } + + private realpathForBoundary(pathValue: string): string | undefined { + const missingSegments: string[] = []; + let currentPath = pathValue; + + while (true) { + try { + return join(realpathSync(currentPath), ...missingSegments.reverse()); + } catch (err) { + const code = typeof err === 'object' && err !== null && 'code' in err ? (err as { code?: unknown }).code : undefined; + if (code !== 'ENOENT') return undefined; + + const parent = dirname(currentPath); + if (parent === currentPath) return undefined; + + missingSegments.push(basename(currentPath)); + currentPath = parent; + } } }