Complete SF safe ID remediation sweep

src/resources/extensions/sf/auto-dispatch.ts

@@ -278,6 +278,29 @@ function validationAttentionMarkerPath(basePath: string, mid: string): string {
 	);
 }
 
+function validationAttentionRuntimePath(basePath: string, mid: string): string {
+	return join(
+		sfRoot(basePath),
+		"runtime",
+		"units",
+		`rewrite-docs-${mid}-validation-attention.json`,
+	);
+}
+
+function hasActiveValidationAttentionMarker(
+	basePath: string,
+	mid: string,
+): boolean {
+	const markerPath = validationAttentionMarkerPath(basePath, mid);
+	if (!existsSync(markerPath)) return false;
+	if (existsSync(validationAttentionRuntimePath(basePath, mid))) return true;
+	logWarning(
+		"dispatch",
+		`ignoring stale validation attention marker for ${mid}: remediation unit was never recorded`,
+	);
+	return false;
+}
+
 function buildValidationAttentionRemediationPrompt(
 	mid: string,
 	midTitle: string,
@@ -1129,8 +1152,11 @@ export const DISPATCH_RULES: DispatchRule[] = [
 						if (verdict === "needs-attention") {
 							const attentionPlan =
 								extractValidationAttentionPlan(validationContent);
-							const markerPath = validationAttentionMarkerPath(basePath, mid);
-							if (attentionPlan && !existsSync(markerPath)) {
+							if (
+								attentionPlan &&
+								!hasActiveValidationAttentionMarker(basePath, mid)
+							) {
+								const markerPath = validationAttentionMarkerPath(basePath, mid);
 								try {
 									mkdirSync(
 										join(sfRoot(basePath), "runtime", "validation-attention"),

src/resources/extensions/sf/tests/reassess-roadmap-safe-id.test.ts

@@ -0,0 +1,198 @@
+/**
+ * Path-traversal ID validation tests for handleReassessRoadmap.
+ * Part of the validation-safe-id-path-segments self-feedback fix.
+ */
+import assert from "node:assert/strict";
+import { mkdtempSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import test from "node:test";
+
+import { handleReassessRoadmap } from "../tools/reassess-roadmap.ts";
+
+function makeTmpBase(): string {
+	return mkdtempSync(join(tmpdir(), "sf-reassess-safe-id-"));
+}
+
+function cleanup(base: string): void {
+	try {
+		rmSync(base, { recursive: true, force: true });
+	} catch {
+		/* noop */
+	}
+}
+
+/** Minimal valid params — unsafe_id checks fire before any DB access. */
+function baseParams() {
+	return {
+		milestoneId: "M001",
+		completedSliceId: "S01",
+		verdict: "confirmed",
+		assessment: "All good.",
+		sliceChanges: {
+			modified: [],
+			added: [],
+			removed: [],
+		},
+	};
+}
+
+// ─── milestoneId traversal ────────────────────────────────────────────────
+
+test("handleReassessRoadmap rejects path traversal in milestoneId", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{ ...baseParams(), milestoneId: "../outside" },
+			base,
+		);
+		assert.ok("error" in result, "should return error");
+		assert.equal(result.error, "unsafe_id");
+		assert.ok("field" in result);
+		assert.ok("reason" in result);
+	} finally {
+		cleanup(base);
+	}
+});
+
+test("handleReassessRoadmap rejects path separator in milestoneId", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{ ...baseParams(), milestoneId: "M001/../../etc/passwd" },
+			base,
+		);
+		assert.ok("error" in result);
+		assert.equal(result.error, "unsafe_id");
+	} finally {
+		cleanup(base);
+	}
+});
+
+// ─── completedSliceId traversal ───────────────────────────────────────────
+
+test("handleReassessRoadmap rejects path traversal in completedSliceId", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{ ...baseParams(), completedSliceId: "../evil" },
+			base,
+		);
+		assert.ok("error" in result);
+		assert.equal(result.error, "unsafe_id");
+		assert.ok("field" in result);
+		assert.ok("reason" in result);
+	} finally {
+		cleanup(base);
+	}
+});
+
+test("handleReassessRoadmap rejects newline in completedSliceId", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{ ...baseParams(), completedSliceId: "S01\nbad: true" },
+			base,
+		);
+		assert.ok("error" in result);
+		assert.equal(result.error, "unsafe_id");
+	} finally {
+		cleanup(base);
+	}
+});
+
+// ─── sliceChanges.modified[].sliceId traversal ───────────────────────────
+
+test("handleReassessRoadmap rejects path traversal in sliceChanges.modified sliceId", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{
+				...baseParams(),
+				sliceChanges: {
+					modified: [{ sliceId: "../bad", title: "title" }],
+					added: [],
+					removed: [],
+				},
+			},
+			base,
+		);
+		assert.ok("error" in result);
+		assert.equal(result.error, "unsafe_id");
+		assert.ok("field" in result);
+		assert.ok("reason" in result);
+	} finally {
+		cleanup(base);
+	}
+});
+
+// ─── sliceChanges.added[].sliceId traversal ──────────────────────────────
+
+test("handleReassessRoadmap rejects path traversal in sliceChanges.added sliceId", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{
+				...baseParams(),
+				sliceChanges: {
+					modified: [],
+					added: [{ sliceId: "S01/../../x", title: "Injected" }],
+					removed: [],
+				},
+			},
+			base,
+		);
+		assert.ok("error" in result);
+		assert.equal(result.error, "unsafe_id");
+		assert.ok("field" in result);
+		assert.ok("reason" in result);
+	} finally {
+		cleanup(base);
+	}
+});
+
+// ─── sliceChanges.removed[] traversal ────────────────────────────────────
+
+test("handleReassessRoadmap rejects path traversal in sliceChanges.removed entry", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{
+				...baseParams(),
+				sliceChanges: {
+					modified: [],
+					added: [],
+					removed: ["../outside"],
+				},
+			},
+			base,
+		);
+		assert.ok("error" in result);
+		assert.equal(result.error, "unsafe_id");
+		assert.ok("field" in result);
+		assert.ok("reason" in result);
+	} finally {
+		cleanup(base);
+	}
+});
+
+test("handleReassessRoadmap rejects backslash separator in sliceChanges.removed entry", async () => {
+	const base = makeTmpBase();
+	try {
+		const result = await handleReassessRoadmap(
+			{
+				...baseParams(),
+				sliceChanges: {
+					modified: [],
+					added: [],
+					removed: ["S01\\..\\evil"],
+				},
+			},
+			base,
+		);
+		assert.ok("error" in result);
+		assert.equal(result.error, "unsafe_id");
+	} finally {
+		cleanup(base);
+	}
+});

src/resources/extensions/sf/tests/remediation-completion-guard.test.ts

@@ -144,6 +144,23 @@ test("completing-milestone dispatches validation-attention remediation once when
 			"dispatch writes a loop-prevention marker",
 		);
 
+		mkdirSync(join(base, ".sf", "runtime", "units"), { recursive: true });
+		writeFileSync(
+			join(
+				base,
+				".sf",
+				"runtime",
+				"units",
+				"rewrite-docs-M001-validation-attention.json",
+			),
+			JSON.stringify({
+				version: 1,
+				unitType: "rewrite-docs",
+				unitId: "M001/validation-attention",
+				phase: "dispatched",
+			}),
+		);
+
 		const second = await completingRule!.match(ctx);
 		assert.ok(second !== null, "second rule should match");
 		assert.equal(second!.action, "stop", "second pass should not loop");
@@ -152,6 +169,58 @@ test("completing-milestone dispatches validation-attention remediation once when
 	}
 });
 
+test("completing-milestone redispatches validation-attention when marker is stale", async () => {
+	const base = mkdtempSync(join(tmpdir(), "sf-remediation-"));
+	mkdirSync(join(base, ".sf", "milestones", "M001"), { recursive: true });
+	mkdirSync(join(base, ".sf", "runtime", "validation-attention"), {
+		recursive: true,
+	});
+
+	try {
+		writeFileSync(
+			join(base, ".sf", "milestones", "M001", "M001-VALIDATION.md"),
+			[
+				"---",
+				"verdict: needs-attention",
+				"remediation_round: 0",
+				"---",
+				"",
+				"# Validation Report",
+				"",
+				"## Remediation Plan",
+				"- Fix roadmap tracking.",
+			].join("\n"),
+		);
+		writeFileSync(
+			join(base, ".sf", "runtime", "validation-attention", "M001.json"),
+			JSON.stringify({
+				milestoneId: "M001",
+				createdAt: new Date().toISOString(),
+			}),
+		);
+
+		const ctx = {
+			mid: "M001",
+			midTitle: "Test Milestone",
+			basePath: base,
+			state: { phase: "completing-milestone" } as any,
+			prefs: {} as any,
+			session: undefined,
+		};
+
+		const result = await completingRule!.match(ctx);
+
+		assert.ok(result !== null, "rule should match");
+		assert.equal(result!.action, "dispatch");
+		if (result!.action === "dispatch") {
+			assert.equal(result!.unitType, "rewrite-docs");
+			assert.equal(result!.unitId, "M001/validation-attention");
+		}
+	} finally {
+		rmSync(base, { recursive: true, force: true });
+	}
+});
+
 test("completing-milestone proceeds normally when VALIDATION verdict is pass (#2675 guard)", async () => {
 	const base = mkdtempSync(join(tmpdir(), "sf-remediation-"));
 	mkdirSync(join(base, ".sf", "milestones", "M001"), { recursive: true });

src/resources/extensions/sf/tests/validate-milestone-safe-id.test.ts

@@ -0,0 +1,180 @@
+/**
+ * Path-traversal ID validation tests for handleValidateMilestone.
+ *
+ * Verifies that unsafe milestoneId values are rejected at the entry point
+ * before any path construction or DB write occurs.
+ * See: validation-safe-id-path-segments self-feedback entry.
+ */
+
+import assert from "node:assert/strict";
+import { randomUUID } from "node:crypto";
+import { mkdirSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import test from "node:test";
+import { clearParseCache } from "../files.ts";
+import { clearPathCache } from "../paths.ts";
+import { closeDatabase, openDatabase } from "../sf-db.ts";
+import { handleValidateMilestone } from "../tools/validate-milestone.ts";
+import type { ValidateMilestoneParams } from "../tools/validate-milestone.ts";
+
+function makeTmpBase(): string {
+	const base = join(tmpdir(), `sf-val-safeid-${randomUUID()}`);
+	mkdirSync(join(base, ".sf", "milestones"), { recursive: true });
+	return base;
+}
+
+function cleanup(base: string): void {
+	clearPathCache();
+	clearParseCache();
+	closeDatabase();
+	try {
+		rmSync(base, { recursive: true, force: true });
+	} catch {
+		/* swallow */
+	}
+}
+
+function openTestDb(base: string): void {
+	const dbPath = join(base, ".sf", "sf.db");
+	assert.equal(openDatabase(dbPath), true, "test DB should open");
+}
+
+const baseParams: Omit<ValidateMilestoneParams, "milestoneId"> = {
+	verdict: "pass",
+	remediationRound: 1,
+	successCriteriaChecklist: "- [x] done",
+	sliceDeliveryAudit: "all slices delivered",
+	crossSliceIntegration: "integrated",
+	requirementCoverage: "100%",
+	verdictRationale: "all criteria met",
+};
+
+test("handleValidateMilestone rejects path traversal milestoneId: ../outside", async () => {
+	const base = makeTmpBase();
+	openTestDb(base);
+	try {
+		const result = await handleValidateMilestone(
+			{ ...baseParams, milestoneId: "../outside" },
+			base,
+		);
+		assert.ok("error" in result, "should return an error for traversal ID");
+		assert.ok(
+			typeof (result as { error: string }).error === "string",
+			"error should be a string",
+		);
+		if ("details" in result) {
+			const details = (
+				result as { error: string; details: { error: string; field: string; reason: string } }
+			).details;
+			assert.equal(details.error, "unsafe_id");
+			assert.equal(details.field, "milestone_id");
+		}
+	} finally {
+		cleanup(base);
+	}
+});
+
+test("handleValidateMilestone rejects path traversal milestoneId: M001/../etc/passwd", async () => {
+	const base = makeTmpBase();
+	openTestDb(base);
+	try {
+		const result = await handleValidateMilestone(
+			{ ...baseParams, milestoneId: "M001/../etc/passwd" },
+			base,
+		);
+		assert.ok("error" in result, "should return an error for traversal ID");
+		if ("details" in result) {
+			const details = (
+				result as { error: string; details: { error: string; field: string; reason: string } }
+			).details;
+			assert.equal(details.error, "unsafe_id");
+		}
+	} finally {
+		cleanup(base);
+	}
+});
+
+test("handleValidateMilestone rejects milestoneId with path separator: a/b", async () => {
+	const base = makeTmpBase();
+	openTestDb(base);
+	try {
+		const result = await handleValidateMilestone(
+			{ ...baseParams, milestoneId: "a/b" },
+			base,
+		);
+		assert.ok("error" in result, "should return an error for ID with separator");
+		if ("details" in result) {
+			const details = (
+				result as { error: string; details: { error: string; field: string; reason: string } }
+			).details;
+			assert.equal(details.error, "unsafe_id");
+			assert.equal(details.field, "milestone_id");
+		}
+	} finally {
+		cleanup(base);
+	}
+});
+
+test("handleValidateMilestone rejects milestoneId with embedded newline", async () => {
+	const base = makeTmpBase();
+	openTestDb(base);
+	try {
+		const result = await handleValidateMilestone(
+			{ ...baseParams, milestoneId: "M001\nbad: true" },
+			base,
+		);
+		assert.ok(
+			"error" in result,
+			"should return an error for ID with control char",
+		);
+		if ("details" in result) {
+			const details = (
+				result as { error: string; details: { error: string; field: string; reason: string } }
+			).details;
+			assert.equal(details.error, "unsafe_id");
+			assert.equal(details.field, "milestone_id");
+			assert.ok(
+				details.reason.includes("control char") || details.reason.includes("newline"),
+				"reason should mention control char or newline",
+			);
+		}
+	} finally {
+		cleanup(base);
+	}
+});
+
+test("handleValidateMilestone does not reject a safe milestoneId as unsafe_id (smoke)", async () => {
+	const base = makeTmpBase();
+	openTestDb(base);
+	try {
+		// A safe ID should pass the safe-id check; later DB/disk errors are expected
+		// in this minimal test setup and do not indicate a safe-id rejection.
+		let result:
+			| { error: string }
+			| { error: string; details: { error: string; field: string; reason: string } }
+			| { milestoneId: string; verdict: string; validationPath: string }
+			| undefined;
+		try {
+			result = await handleValidateMilestone(
+				{ ...baseParams, milestoneId: "M001" },
+				base,
+			);
+		} catch {
+			// DB/disk errors from the minimal test setup are acceptable here
+			return;
+		}
+		if (result !== undefined && "details" in result) {
+			const details = (
+				result as { error: string; details: { error: string } }
+			).details;
+			assert.notEqual(
+				details.error,
+				"unsafe_id",
+				"safe ID should not produce unsafe_id error",
+			);
+		}
+	} finally {
+		cleanup(base);
+	}
+});

src/resources/extensions/sf/tools/complete-slice.ts

@@ -419,6 +419,18 @@ export async function handleCompleteSlice(
 	paramsInput: CompleteSliceParams,
 	basePath: string,
 ): Promise<CompleteSliceResult | { error: string }> {
+	// ── Path-traversal guard (validation-safe-id-path-segments) ───────────
+	// Checked on raw input before normalizeCompleteSliceParams so the
+	// structured error shape is preserved. Rejects any ID that could escape
+	// .sf/ when interpolated into a path or used in a DB write.
+	const idCheck = checkSafeIds({
+		milestoneId: paramsInput.milestoneId,
+		sliceId: paramsInput.sliceId,
+	});
+	if (idCheck) {
+		return { error: "unsafe_id", field: idCheck.fieldName, reason: idCheck.reason } as unknown as { error: string };
+	}
+
 	let params: CompleteSliceParams;
 	try {
 		params = normalizeCompleteSliceParams(paramsInput);
@@ -442,21 +454,6 @@ export async function handleCompleteSlice(
 		return { error: "milestoneId is required and must be a non-empty string" };
 	}
 
-	// ── Path-traversal guard (validation-safe-id-path-segments) ───────────
-	// Reject any ID that could escape .sf/ when interpolated into a path.
-	// Must run before sliceUnitKey, resolveSlicePath, and all DB writes.
-	const idCheck = checkSafeIds({
-		milestoneId: params.milestoneId,
-		sliceId: params.sliceId,
-	});
-	if (idCheck) {
-		return {
-			error: "unsafe_id",
-			field: idCheck.fieldName,
-			reason: idCheck.reason,
-		};
-	}
-
 	// ── Ownership check (opt-in: only enforced when claim file exists) ──────
 	const ownershipErr = checkOwnership(
 		basePath,

src/resources/extensions/sf/tools/complete-task.ts

@@ -256,6 +256,24 @@ export async function handleCompleteTask(
 	paramsInput: CompleteTaskParams,
 	basePath: string,
 ): Promise<CompleteTaskResult | { error: string; field?: string; reason?: string }> {
+	// ── Path-traversal safety check (runs on raw input before normalization) ─
+	// Reject any ID that could escape .sf/ when interpolated into a path.
+	// Must run before normalizeCompleteTaskParams, which also validates IDs
+	// but returns a generic error string rather than the structured unsafe_id
+	// shape expected downstream.
+	const idCheck = checkSafeIds({
+		milestoneId: paramsInput.milestoneId,
+		sliceId: paramsInput.sliceId,
+		taskId: paramsInput.taskId,
+	});
+	if (idCheck) {
+		return {
+			error: "unsafe_id",
+			field: idCheck.fieldName,
+			reason: idCheck.reason,
+		};
+	}
+
 	let params: CompleteTaskParams;
 	try {
 		params = normalizeCompleteTaskParams(paramsInput);
@@ -286,20 +304,6 @@ export async function handleCompleteTask(
 		return { error: "milestoneId is required and must be a non-empty string" };
 	}
 
-	// ── Path-traversal safety check ────────────────────────────────────────
-	const idCheck = checkSafeIds({
-		milestoneId: params.milestoneId,
-		sliceId: params.sliceId,
-		taskId: params.taskId,
-	});
-	if (idCheck) {
-		return {
-			error: "unsafe_id",
-			field: idCheck.fieldName,
-			reason: idCheck.reason,
-		};
-	}
-
 	// ── Ownership check (opt-in: only enforced when claim file exists) ──────
 	const ownershipErr = checkOwnership(
 		basePath,

src/resources/extensions/sf/tools/product-audit-tool.ts

@@ -265,7 +265,11 @@ function renderAuditMarkdown(
 export async function handleProductAudit(
 	rawParams: ProductAuditParams,
 	basePath: string,
-): Promise<ProductAuditResult | { error: string }> {
+): Promise<
+	| ProductAuditResult
+	| { error: string }
+	| { error: string; field: string; reason: string }
+> {
 	let params: ProductAuditParams;
 	try {
 		params = validateProductAuditParams(rawParams);