fix: five verified defects from the Broad-Side self-scan (#128-#132)

CHANGELOG.md

@@ -12,6 +12,14 @@ All notable changes to this project are documented here. The format is based on
 - **Broad-Side: truncation detection with fence-tolerant parsing** (#103, #133). Lens output is now parsed tolerantly — markdown code fences are stripped before JSON parsing, mirroring the tolerance in OpenRouter's headless-agent scaffold. Parseable output is saved as clean JSON; output that still does not parse (the signature of a `max_tokens` cutoff) is saved verbatim but marked `truncated`. The collect summary and `run-meta.json` report truncation counts, and the synthesis prompt is told which modules are unrepresented rather than clean. Also documented the retry-safety invariant (batch requests are pure, resubmission always safe) and the distinction between Broad-Side's pre-flight `max_cost` estimate and OpenRouter's runtime cost accounting.
 - **Broad-Side: triage pass** (#103, #135). Collect now runs a second cross-lens post-pass alongside synthesis (skip with `include_triage: false`): every finding is scored by impact × fix difficulty and turned into a prioritized work order — P0–P3 priority, effort estimate, per-module grouping, deduplicated leads, and explicit `omitted` notes for dropped items — saved as `triage.json`/`triage.md` and surfaced in the collect summary. The triage prompt frames the queue as a starting point for re-verification, never a commitment. Both post-passes submit as separate batches together and poll independently, and the state file tracks each so a resumed collect can finish whichever is still pending.
 
+### Fixed
+
+- **Windows: `writeLibraryConfig` no longer ENOENTs on parentless paths** (#128). The hand-rolled `includes("/")` separator check treated every Windows path as a bare filename, leaving `mkdir` a no-op before the write failed. Now derives the parent directory with `dirname()`.
+- **`isWithinPath` accepts subpaths of filesystem roots** (#130). Appending a separator to an already-terminated root (`/` → `//`, `C:\` → `C:\\`) produced a prefix no real path starts with, rejecting every legitimate subpath. Trailing separators are now respected before prefixing.
+- **`acquireLock` closes the lock descriptor when `writeFile` throws** (#131). A non-EEXIST write failure previously leaked the file handle until GC.
+- **Phase continuation no longer burns the full compaction settle timeout** (#129). When a phase run ends without a compaction event, the pending compaction promise is now settled with `false`, so `waitForCompaction` returns immediately instead of waiting out `COMPACTION_SETTLE_TIMEOUT_MS` on every continuation.
+- **Completion re-validates under the status lock** (#132). `completeValidatedPhase` now re-runs `validatePhaseOutput` inside the atomic status update; a stale PASS whose output changed since the caller's validation refuses to complete (and leaves status untouched) instead of completing a phase on evidence that no longer holds. Validations that never touched a file on disk (no `outputPath`) keep the legacy path, matching the synthetic-validation contract in unit tests.
+
 ## [0.16.0] — 2026-08-17
 
 The field-test round. Immediately after 0.15.0 shipped, the same 7-phase deepseek-harness analysis was re-run on a fresh worktree through the published binary — this time with the driving chat as orchestrator — and the run's own gaps became this release (#111–#114): the very first completion appended decision rows without their promised heading, both full runs ended with no dashboard ever rendered, the analysis→publish→synthesis library loop was unreachable from any served text, and the terminal completion message named nothing actionable while skills, amendments, a publishable spec, and the usage log all sat unused.

core/completion.ts

@@ -1,7 +1,7 @@
 import { appendFile, copyFile, mkdir, readFile, readdir, writeFile } from "node:fs/promises";
 import { join } from "node:path";
 
-import { getNextEligiblePhase, resolvePhase } from "./pipeline.ts";
+import { getNextEligiblePhase, resolvePhase, validatePhaseOutput } from "./pipeline.ts";
 import { applyHandoff, autoAssignIds, buildTerminalNextActions, loadHandoffFile, normalizeStatus } from "./status.ts";
 import type { NormalizedStatus, OpenQuestionEntry, PhaseHandoff, ProposedConventionEntry, ValidationResult, WorkspaceState } from "./types.ts";
 import { dateOnly, pathExists, uniqueStrings } from "./utils.ts";
@@ -335,6 +335,25 @@ export async function completeValidatedPhase(
 		const phase = resolvePhase(lockedState, validation.phaseId);
 		if (!phase?.primary_output) throw new Error(`Phase ${validation.phaseId} is missing primary_output.`);
 
+		// Re-validate the output under the lock (#132). The caller's
+		// validation snapshot can predate a concurrent edit or another
+		// session's completion; a stale PASS must not complete a phase whose
+		// output no longer validates. The locked recheck is the authoritative
+		// one and is what every artifact below is written from.
+		// A validation that never touched a file on disk (no outputPath)
+		// has nothing to race against, so the caller's result stands — the
+		// real surfaces (MCP, Pi) always validate real files.
+		const authoritative = validation.outputPath
+			? await validatePhaseOutput(lockedState, validation.phaseId)
+			: validation;
+		if (authoritative.overall === "FAIL" || authoritative.overall === "MISSING") {
+			throw new Error(
+				`Refusing to complete ${validation.phaseId}: the output no longer validates under the status lock ` +
+				`(now ${authoritative.overall}). It changed since the last validation — re-run validation and fix the output first.`,
+			);
+		}
+		const lockedValidation = authoritative;
+
 		const nextStatus = normalizeStatus(lockedState.status, lockedState.pipeline, lockedState.status.pipeline, lockedState.cwd);
 		const existingPhase = nextStatus.phases[validation.phaseId] ?? {
 			status: "pending",
@@ -343,7 +362,7 @@ export async function completeValidatedPhase(
 			open_questions: [],
 			carry_forward: [],
 		};
-		const gapEntries: OpenQuestionEntry[] = validation.rows
+		const gapEntries: OpenQuestionEntry[] = lockedValidation.rows
 			.filter((row) => row.result.toUpperCase().includes("PARTIAL"))
 			.map((row) => ({
 				kind: "needs-maintainer-decision",
@@ -363,7 +382,7 @@ export async function completeValidatedPhase(
 				...existingPhase.owner_notes,
 				`Completed via ${sourceLabel}.`,
 				`Primary output: .codecarto/${validation.primaryOutput}`,
-				`Validation: ${validation.overall}`,
+				`Validation: ${lockedValidation.overall}`,
 			]),
 			outputs_present: uniqueStrings([...existingPhase.outputs_present, validation.primaryOutput]),
 			open_questions: mergedOpenQuestions,
@@ -378,7 +397,7 @@ export async function completeValidatedPhase(
 			? [`Begin ${nextEligible.id} phase by producing ${nextEligible.primary_output ?? `findings/${nextEligible.id}/`}`]
 			: buildTerminalNextActions(nextStatus);
 
-		const artifacts = await writeCompletionArtifacts(lockedState.workspaceDir, validation.phaseId, validation, completionTimestamp, handoff);
+		const artifacts = await writeCompletionArtifacts(lockedState.workspaceDir, validation.phaseId, lockedValidation, completionTimestamp, handoff);
 		closeoutPath = artifacts.closeoutPath;
 		orchestratorCheckpoint = buildOrchestratorCheckpoint(artifacts.decisionsAppended, artifacts.totalPendingProposals, nextStatus);
 		return { state: { ...nextWorkspace, status: nextStatus } };

core/orchestrator-config.ts

@@ -15,7 +15,7 @@
 // don't have to expand themselves.
 
 import { homedir } from "node:os";
-import { join, resolve } from "node:path";
+import { dirname, join, resolve } from "node:path";
 import type { PathLike } from "node:fs";
 import { mkdir, readFile, writeFile } from "node:fs/promises";
 import { expandTilde, pathExists } from "./utils.ts";
@@ -179,7 +179,10 @@ export async function writeLibraryConfig(
 	if (namespace) library.namespace = namespace;
 
 	const updated: Record<string, unknown> = { ...existing, library };
-	const dir = configPath.includes("/") ? configPath.slice(0, configPath.lastIndexOf("/")) : ".";
+	// dirname() honors the platform separator; the previous hand-rolled
+	// `includes("/")` check treated every Windows path as a bare filename
+	// and left mkdir a no-op before the writeFile ENOENT'd (#128).
+	const dir = dirname(configPath);
 	await mkdir(dir, { recursive: true });
 	await writeFile(configPath, `${stringifySimpleYaml(updated)}\n`, "utf8");
 }

core/status.ts

@@ -358,7 +358,14 @@ export async function acquireLock(lockPath: string): Promise<{ release: () => Pr
 	while (true) {
 		try {
 			const handle = await open(lockPath, "wx");
-			await handle.writeFile(`${process.pid}\n${new Date().toISOString()}\n`, "utf8");
+			try {
+				await handle.writeFile(`${process.pid}\n${new Date().toISOString()}\n`, "utf8");
+			} catch (error) {
+				// A non-EEXIST write failure must not leak the descriptor the
+				// open just created (#131); close best-effort, then rethrow.
+				await handle.close().catch(() => undefined);
+				throw error;
+			}
 			await handle.close();
 			return {
 				release: async () => {

core/utils.ts

@@ -37,7 +37,13 @@ export function isWithinPath(path: string, root: string): boolean {
 	const normalizedPath = normalizeForComparison(resolve(path));
 	const normalizedRoot = normalizeForComparison(resolve(root));
 	if (normalizedPath === normalizedRoot) return true;
-	return normalizedPath.startsWith(`${normalizedRoot}${process.platform === "win32" ? "\\" : "/"}`);
+	// A filesystem root (e.g. "/" or "C:\") already ends in a separator;
+	// appending another one produced a prefix ("//" / "C:\\") that no real
+	// path starts with, falsely rejecting every legitimate subpath (#130).
+	const prefix = normalizedRoot.endsWith("/") || normalizedRoot.endsWith("\\")
+		? normalizedRoot
+		: `${normalizedRoot}${process.platform === "win32" ? "\\" : "/"}`;
+	return normalizedPath.startsWith(prefix);
 }
 
 /**

extensions/codecarto/agent-runner.ts

@@ -256,6 +256,12 @@ export async function runPhase(
 
 	try {
 		await session.prompt(prompt);
+		// If no compaction fired during the run, the promise above would
+		// otherwise strand the continuation path for the full settle timeout
+		// (#129). Settle it with what actually happened — resolve() is
+		// idempotent, so a real compaction_end event earlier in the run
+		// keeps its `true`.
+		resolveCompaction?.(false);
 		let primaryOutputPresent = true;
 		if (options.primaryOutput) {
 			primaryOutputPresent = await primaryOutputExists(cwd, options.primaryOutput);

tests/broadside-scan-fixes.test.mjs

@@ -0,0 +1,158 @@
+// Tests for the five defects the Broad-Side self-scan found and verified
+// (#128–#132).
+//
+// Two of these are true regression tests — they fail against the unfixed
+// code: #130 (filesystem-root subpaths) and the #132 stale-PASS refusal.
+//
+// The other three pin behavior around a fix they cannot themselves provoke,
+// because the failing condition is not reachable from this suite:
+//   #128 is a Windows-only path bug. On POSIX the old `includes("/")` branch
+//        and dirname() agree, so no assertion here can separate them.
+//   #131 guards the path where `writeFile` throws on an open descriptor,
+//        which needs injection this suite has no seam for; the test covers
+//        acquire/release/re-acquire instead.
+//   #129 fixes runPhase settling the compaction promise, but runPhase needs
+//        an AgentSession; the test covers waitForCompaction's contract on an
+//        already-settled promise, which held before the fix too.
+// Treat those three as coverage, not as guards — a refactor could revert
+// their fixes without turning this file red.
+
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { cp, mkdtemp, mkdir, readFile, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { dirname, join, resolve } from "node:path";
+import { fileURLToPath, pathToFileURL } from "node:url";
+
+const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..");
+const core = await import(pathToFileURL(`${REPO_ROOT}/core/index.ts`).href);
+const { completeValidatedPhase } = await import(pathToFileURL(`${REPO_ROOT}/core/completion.ts`).href);
+const { waitForCompaction } = await import(pathToFileURL(`${REPO_ROOT}/extensions/codecarto/agent-runner.ts`).href);
+
+async function initWorkspace(cwd, pipeline = "workflow/pipeline-architecture-only.yaml") {
+	const packaged = join(REPO_ROOT, ".codecarto");
+	await cp(packaged, join(cwd, ".codecarto"), { recursive: true });
+	const statusPath = join(cwd, ".codecarto", "workflow", "status.yaml");
+	const raw = await core.loadYamlFile(statusPath);
+	raw.pipeline = pipeline;
+	await writeFile(statusPath, `${core.stringifySimpleYaml(raw)}\n`, "utf8");
+}
+
+// ---------- #128: writeLibraryConfig must create parent directories ----------
+
+test("#128 writeLibraryConfig creates nested parent directories", async () => {
+	const dir = await mkdtemp(join(tmpdir(), "cc-128-"));
+	try {
+		const configPath = join(dir, "deeply", "nested", "dir", "config.yaml");
+		await core.writeLibraryConfig(configPath, "/some/library");
+		const written = await readFile(configPath, "utf8");
+		assert.match(written, /library:/);
+		assert.match(written, /\/some\/library/);
+	} finally {
+		await rm(dir, { recursive: true, force: true });
+	}
+});
+
+// ---------- #130: isWithinPath at filesystem roots ----------
+
+test("#130 isWithinPath accepts subpaths when the root is a filesystem root", () => {
+	assert.equal(core.isWithinPath("/etc/passwd", "/"), true, "/etc/passwd is within /");
+	assert.equal(core.isWithinPath("/repo/src", "/repo"), true, "normal case still works");
+	assert.equal(core.isWithinPath("/repo2", "/repo"), false, "sibling prefixes still rejected");
+	assert.equal(core.isWithinPath("/repo", "/repo/src"), false, "parent is not within child");
+});
+
+// ---------- #131: acquireLock keeps the descriptor discipline ----------
+
+test("#131 acquireLock releases cleanly and allows re-acquisition", async () => {
+	const dir = await mkdtemp(join(tmpdir(), "cc-131-"));
+	try {
+		const lockPath = join(dir, "status.lock");
+		const first = await core.acquireLock(lockPath);
+		await first.release();
+		// The descriptor must have been closed by release's rm; re-acquiring
+		// with a fresh exclusive open must succeed.
+		const second = await core.acquireLock(lockPath);
+		assert.equal(typeof second.release, "function");
+		await second.release();
+	} finally {
+		await rm(dir, { recursive: true, force: true });
+	}
+});
+
+// ---------- #129: settled compaction promise must not cost 30s ----------
+
+test("#129 waitForCompaction returns immediately when the promise is pre-settled", async () => {
+	const started = Date.now();
+	const compacted = await waitForCompaction(Promise.resolve(false), 30_000);
+	const elapsed = Date.now() - started;
+	assert.equal(compacted, false);
+	assert.ok(elapsed < 1000, `settled promise must not wait for the timeout (took ${elapsed}ms)`);
+});
+
+// ---------- #132: completion re-validates under the status lock ----------
+
+test("#132 completeValidatedPhase refuses a stale PASS when the output changed", async () => {
+	const cwd = await mkdtemp(join(tmpdir(), "cc-132-"));
+	try {
+		await initWorkspace(cwd);
+		const outputPath = join(cwd, ".codecarto", "findings", "architecture", "architecture-map.md");
+		await writeFile(
+			outputPath,
+			"# Architecture Map\n\n## Validation\n\n| Criterion | Result | Evidence |\n|---|---|---|\n| Intent documented | PASS | yes |\n\n**Overall:** PASS\n",
+			"utf8",
+		);
+		const handoffDir = join(cwd, ".codecarto", "scratch", "handoffs");
+		await mkdir(handoffDir, { recursive: true });
+		await writeFile(
+			join(handoffDir, "architecture.yaml"),
+			"phase_id: architecture\nopen_questions: []\ncarry_forward: []\ncarry_forward_closures: []\nopen_question_closures: []\npost_pipeline: []\ndecisions: []\nproposed_conventions: []\ncloseout_summary: done\n",
+			"utf8",
+		);
+
+		const state = await core.getWorkspaceState(cwd);
+		const validation = await core.validatePhaseOutput(state, "architecture");
+		assert.equal(validation.overall, "PASS", "precondition: output validates");
+
+		// A concurrent session (or the executor) rewrites the output before
+		// completion acquires the lock. The stale PASS must not complete it.
+		await writeFile(outputPath, "garbage without a validation block", "utf8");
+
+		await assert.rejects(
+			() => completeValidatedPhase(cwd, validation, "test"),
+			/no longer validates/,
+		);
+
+		const after = await core.getWorkspaceState(cwd);
+		assert.equal(after.status.phases.architecture.status, "pending", "status must stay untouched after refusal");
+	} finally {
+		await rm(cwd, { recursive: true, force: true });
+	}
+});
+
+test("#132 completeValidatedPhase completes normally when the output is unchanged", async () => {
+	const cwd = await mkdtemp(join(tmpdir(), "cc-132b-"));
+	try {
+		await initWorkspace(cwd);
+		const outputPath = join(cwd, ".codecarto", "findings", "architecture", "architecture-map.md");
+		await writeFile(
+			outputPath,
+			"# Architecture Map\n\n## Validation\n\n| Criterion | Result | Evidence |\n|---|---|---|\n| Intent documented | PASS | yes |\n\n**Overall:** PASS\n",
+			"utf8",
+		);
+		const handoffDir = join(cwd, ".codecarto", "scratch", "handoffs");
+		await mkdir(handoffDir, { recursive: true });
+		await writeFile(
+			join(handoffDir, "architecture.yaml"),
+			"phase_id: architecture\nopen_questions: []\ncarry_forward: []\ncarry_forward_closures: []\nopen_question_closures: []\npost_pipeline: []\ndecisions: []\nproposed_conventions: []\ncloseout_summary: done\n",
+			"utf8",
+		);
+
+		const state = await core.getWorkspaceState(cwd);
+		const validation = await core.validatePhaseOutput(state, "architecture");
+		const result = await completeValidatedPhase(cwd, validation, "test");
+		assert.equal(result.updatedState.status.phases.architecture.status, "complete");
+	} finally {
+		await rm(cwd, { recursive: true, force: true });
+	}
+});