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
This commit is contained in:
Tom Boucher
@@ -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<ArchitecturalDebtCheck> {
|
||||
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<string>();
|
||||
|
||||
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<string[]> {
|
||||
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<string>();
|
||||
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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user