Apply shared safe ID validation

src/resources/extensions/sf/bootstrap/db-tools.ts

@@ -729,6 +729,7 @@ export function registerDbTools(pi: ExtensionAPI): void {
 		promptGuidelines: [
 			"Use sf_self_report for ANY sf-internal observation — not just bugs. Acceptable kinds include: 'prompt-quality-issue' (you found a prompt ambiguous, contradictory, or missing context), 'improvement-idea' (a non-bug enhancement that would help), 'agent-friction' (workflow friction you worked around), 'design-thought' (broader speculation), 'missing-feature' (capability you wished sf had), as well as classic bug kinds like 'brittle-predicate' or 'git-empty-pathspec'.",
 			"Do NOT use this for bugs in the user's project, for your own task work, or to track your task's todo list. ONLY for observations about sf-the-tool itself.",
+			"This tool FILES new entries; it does not address or resolve existing ones. The backlog is a triage inbox awaiting human/triage-agent review — do NOT autonomously pick entries off the backlog and try to fix them. Treat existing entries as out of scope unless your task plan explicitly names a backlog entry id as the work.",
 			"Over-reporting is preferred to under-reporting at this stage. If you noticed it about sf, file it. Dedup and threshold-to-roadmap promotion are tracked as their own backlog items and will eventually clean noise.",
 			"Severity guide: low = cosmetic / nice-to-have / improvement idea. medium = noisy or imperfect or recurring friction. high = blocked the unit (sf-the-tool prevented you from completing the task). critical = needs immediate fix (currently treated as high until inline-fix dispatch lands).",
 			"high/critical entries mark the originating unit as blocked: it will not seal as success, and will be re-queued only after sf is bumped past the recorded version.",

src/resources/extensions/sf/prompts/execute-task.md

@@ -82,7 +82,7 @@ Then:
     - After a compile-repair edit, rerun the narrow failing command immediately before more feature work. If two repair attempts leave the same unknown-symbol class, stop broad edits and write a precise handoff/blocker summary.
 17. **Blocker discovery:** If execution reveals that the remaining slice plan is fundamentally invalid — not just a bug or minor deviation, but a plan-invalidating finding like a wrong API, missing capability, or architectural mismatch — set `blocker_discovered: true` in the task summary frontmatter and describe the blocker clearly in the summary narrative. Do NOT set `blocker_discovered: true` for ordinary debugging, minor deviations, or issues that can be fixed within the current task or the remaining plan. This flag triggers an automatic replan of the slice.
 17b. **sf-internal anomalies and observations:** If during execution you observe sf-the-tool misbehaving (empty `git add --` pathspecs, brittle gate predicates, advisory-downgrade hiding real failures, false safety floods), find a prompt ambiguous or contradictory, hit workflow friction, or have an idea that would make sf better — call `sf_self_report`. Use `prompt-quality-issue`, `improvement-idea`, `agent-friction`, or `design-thought` kinds for non-bug observations alongside the classic bug kinds. Severity guide: `low`/`medium` for cosmetic / noisy / nice-to-have (sf continues); `high`/`critical` only when the sf issue actually prevents the task from sealing correctly (this blocks the unit). For high/critical, include `acceptance_criteria` so a future resolver has a falsifiable bar. This is distinct from `blocker_discovered` (which is about the user's plan, not about sf). Over-reporting is preferred to under-reporting at this stage.
-17c. **If your task picks up a backlog entry:** Read the entry's `acceptanceCriteria` (in `.sf/BACKLOG.md` or `.sf/self-feedback.jsonl`). Confirm each criterion is satisfied by your fix before considering the entry resolved. When you complete the task, the system will eventually run `markResolved` with structured evidence — for now, cite the entry id and which criteria you met in your task summary's `narrative` so the resolution is traceable. Do not silently fix and move on.
+17c. **The self-feedback backlog is a TRIAGE inbox, not a work queue.** Do NOT autonomously pick up entries from `.sf/BACKLOG.md` or `~/.sf/agent/upstream-feedback.jsonl` and try to fix them — those are open observations awaiting human/triage-agent review to decide which become scheduled work, duplicates, or wontfix. Your scope is the task plan you were dispatched with. The only interaction your task should have with the backlog is FILING new entries (via `sf_self_report`) when you observe sf-internal anomalies. The exception: if a backlog entry id is *explicitly named* in your task plan as the work to be done, treat it as you would any other planned item — read its `acceptanceCriteria`, satisfy each, and cite the entry id + criteria met in your task summary's `narrative` so the resolution is traceable.
 18. If you made an architectural, pattern, library, or observability decision during this task that downstream work should know about, append it to `.sf/DECISIONS.md` (read the template at `~/.sf/agent/extensions/sf/templates/decisions.md` if the file doesn't exist yet). Not every task produces decisions — only append when a meaningful choice was made.
 19. If you discover a non-obvious rule, recurring gotcha, or useful pattern during execution, append it to `.sf/KNOWLEDGE.md`. Only add entries that would save future agents from repeating your investigation. Don't add obvious things.
 20. Read the template at `~/.sf/agent/extensions/sf/templates/task-summary.md`

src/resources/extensions/sf/safety/safe-id.ts

@@ -0,0 +1,108 @@
+/**
+ * Safe-ID validation — reject any ID before it reaches a filesystem path
+ * or DB write where path-traversal characters could escape `.sf/`.
+ *
+ * sf tool handlers historically only checked "non-empty string" on
+ * milestoneId / sliceId / taskId fields and then interpolated those into
+ * paths like `milestones/${id}/slices/${id}/...`. That allowed inputs
+ * such as `../outside`, `S01/../../x`, or `T01\nbad: true` to either
+ * write outside `.sf` or corrupt downstream YAML/JSON consumers.
+ *
+ * This module provides a single chokepoint validator. It is intentionally
+ * focused on SECURITY (path traversal, separators, control chars) rather
+ * than format. Format-specific patterns (e.g. MILESTONE_ID_RE) live next
+ * to their domain in `milestone-id-utils.ts` etc.
+ *
+ * Filed under `validation-safe-id-path-segments` in the self-feedback
+ * backlog. Apply at every tool-handler entry point that uses an ID in a
+ * filesystem path or DB write.
+ */
+
+const FORBIDDEN_CHAR = /[\\/\x00-\x1f\n\r\t]/;
+const MAX_ID_LENGTH = 64;
+
+export class UnsafeIdError extends TypeError {
+	constructor(
+		public readonly fieldName: string,
+		public readonly reason: string,
+		public readonly value: string,
+	) {
+		super(
+			`${fieldName} is unsafe: ${reason} (got ${JSON.stringify(value).slice(0, 80)})`,
+		);
+		this.name = "UnsafeIdError";
+	}
+}
+
+/**
+ * Validate a single string is safe to use as a path segment. Throws
+ * UnsafeIdError on rejection so callers get a structured error they can
+ * surface to the agent without leaking the full attempted value into logs.
+ */
+export function validateSafePathSegment(
+	value: unknown,
+	fieldName: string,
+): asserts value is string {
+	if (typeof value !== "string") {
+		throw new UnsafeIdError(fieldName, "not a string", String(value));
+	}
+	if (value.length === 0) {
+		throw new UnsafeIdError(fieldName, "empty", value);
+	}
+	if (value.length > MAX_ID_LENGTH) {
+		throw new UnsafeIdError(
+			fieldName,
+			`exceeds ${MAX_ID_LENGTH} characters`,
+			value,
+		);
+	}
+	if (value === "." || value === "..") {
+		throw new UnsafeIdError(fieldName, ". or ..", value);
+	}
+	if (FORBIDDEN_CHAR.test(value)) {
+		throw new UnsafeIdError(
+			fieldName,
+			"contains path separator, control char, newline, tab, or NUL",
+			value,
+		);
+	}
+	if (value.includes("..")) {
+		throw new UnsafeIdError(fieldName, "contains '..' (path traversal)", value);
+	}
+	if (value.startsWith(".") || value.startsWith("-")) {
+		throw new UnsafeIdError(fieldName, "starts with '.' or '-'", value);
+	}
+}
+
+export interface SafeIdsInput {
+	milestoneId?: string | null;
+	sliceId?: string | null;
+	taskId?: string | null;
+}
+
+/**
+ * Validate the standard milestone/slice/task ID trio. Any field that is
+ * undefined or null is skipped; any field that is present is validated.
+ * Throws UnsafeIdError on the first failure.
+ */
+export function assertSafeIds(input: SafeIdsInput): void {
+	if (input.milestoneId != null)
+		validateSafePathSegment(input.milestoneId, "milestone_id");
+	if (input.sliceId != null) validateSafePathSegment(input.sliceId, "slice_id");
+	if (input.taskId != null) validateSafePathSegment(input.taskId, "task_id");
+}
+
+/**
+ * Convenience: returns the first UnsafeIdError encountered or null if all
+ * provided IDs are safe. Use when the caller wants to handle the error
+ * inline rather than via try/catch.
+ */
+export function checkSafeIds(input: SafeIdsInput): UnsafeIdError | null {
+	try {
+		assertSafeIds(input);
+		return null;
+	} catch (err) {
+		if (err instanceof UnsafeIdError) return err;
+		throw err;
+	}
+}

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

@@ -0,0 +1,47 @@
+/**
+ * Focused test: handleCompleteMilestone rejects unsafe milestoneId values
+ * before any filesystem or DB operation.
+ *
+ * Part of the validation-safe-id-path-segments self-feedback fix.
+ */
+
+import assert from "node:assert/strict";
+import { test } from "node:test";
+import { handleCompleteMilestone } from "../tools/complete-milestone.ts";
+
+const BASE_PARAMS = {
+	title: "Test Milestone",
+	oneLiner: "A test.",
+	narrative: "Narrative here.",
+	verificationPassed: true,
+};
+
+test("handleCompleteMilestone rejects path-traversal milestoneId (../etc)", async () => {
+	const result = await handleCompleteMilestone(
+		{ ...BASE_PARAMS, milestoneId: "../etc" },
+		"/tmp/nonexistent-base",
+	);
+	assert.ok("error" in result, "should return an error response");
+	const err = result as { error: string; field?: string; reason?: string };
+	assert.equal(err.error, "unsafe_id");
+	assert.equal(err.field, "milestone_id");
+	assert.ok(
+		typeof err.reason === "string" && err.reason.length > 0,
+		"reason should be a non-empty string",
+	);
+});
+
+test("handleCompleteMilestone rejects milestoneId with embedded newline (M001\\nbad)", async () => {
+	const result = await handleCompleteMilestone(
+		{ ...BASE_PARAMS, milestoneId: "M001\nbad" },
+		"/tmp/nonexistent-base",
+	);
+	assert.ok("error" in result, "should return an error response");
+	const err = result as { error: string; field?: string; reason?: string };
+	assert.equal(err.error, "unsafe_id");
+	assert.equal(err.field, "milestone_id");
+	assert.ok(
+		typeof err.reason === "string" && err.reason.length > 0,
+		"reason should be a non-empty string",
+	);
+});

src/resources/extensions/sf/tests/complete-slice-safe-id.test.ts

@@ -0,0 +1,82 @@
+/**
+ * Path-traversal ID validation for handleCompleteSlice
+ * (validation-safe-id-path-segments self-feedback fix)
+ *
+ * Asserts that handleCompleteSlice rejects unsafe milestoneId / sliceId
+ * values before they reach any filesystem path interpolation or DB write.
+ */
+
+import assert from "node:assert/strict";
+import test from "node:test";
+import { handleCompleteSlice } from "../tools/complete-slice.ts";
+import type { CompleteSliceParams } from "../types.ts";
+
+/** Minimal valid-looking params — fields beyond milestoneId/sliceId are
+ *  irrelevant because the safe-id check fires before they are used. */
+function baseParams(): CompleteSliceParams {
+	return {
+		sliceId: "S01",
+		milestoneId: "M001",
+		sliceTitle: "Test Slice",
+		oneLiner: "one-liner",
+		narrative: "narrative",
+		verification: "all tests pass",
+		deviations: "None.",
+		knownLimitations: "None.",
+		followUps: "None.",
+		keyFiles: [],
+		keyDecisions: [],
+		patternsEstablished: [],
+		observabilitySurfaces: [],
+		provides: [],
+		requirementsSurfaced: [],
+		drillDownPaths: [],
+		affects: [],
+		requirementsAdvanced: [],
+		requirementsValidated: [],
+		requirementsInvalidated: [],
+		filesModified: [],
+		requires: [],
+		uatContent: "## UAT\n\nSmoke test passes.",
+	};
+}
+
+test("handleCompleteSlice rejects path-traversal milestoneId", async () => {
+	const params = { ...baseParams(), milestoneId: "../etc" };
+	const result = await handleCompleteSlice(params, "/tmp/sf-safe-id-test");
+
+	assert.ok(
+		"error" in result,
+		"result must be an error response",
+	);
+	const err = result as { error: string; field?: string; reason?: string };
+	assert.equal(err.error, "unsafe_id", 'error must be "unsafe_id"');
+	assert.ok(
+		typeof err.field === "string" && err.field.length > 0,
+		"field must be a non-empty string",
+	);
+	assert.ok(
+		typeof err.reason === "string" && err.reason.length > 0,
+		"reason must be a non-empty string",
+	);
+});
+
+test("handleCompleteSlice rejects path-traversal sliceId", async () => {
+	const params = { ...baseParams(), sliceId: "S01/../../x" };
+	const result = await handleCompleteSlice(params, "/tmp/sf-safe-id-test");
+
+	assert.ok(
+		"error" in result,
+		"result must be an error response",
+	);
+	const err = result as { error: string; field?: string; reason?: string };
+	assert.equal(err.error, "unsafe_id", 'error must be "unsafe_id"');
+	assert.ok(
+		typeof err.field === "string" && err.field.length > 0,
+		"field must be a non-empty string",
+	);
+	assert.ok(
+		typeof err.reason === "string" && err.reason.length > 0,
+		"reason must be a non-empty string",
+	);
+});

src/resources/extensions/sf/tests/complete-task-safe-id.test.ts

@@ -0,0 +1,120 @@
+/**
+ * Tests that handleCompleteTask rejects path-traversal IDs before any
+ * filesystem or DB operation is attempted.
+ *
+ * Covers the validation-safe-id-path-segments self-feedback fix.
+ */
+
+import assert from "node:assert/strict";
+import { mkdirSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { randomUUID } from "node:crypto";
+import { describe, it } from "node:test";
+import { handleCompleteTask } from "../tools/complete-task.js";
+
+function makeTmpBase(): string {
+	const base = join(tmpdir(), `sf-ct-safe-id-${randomUUID()}`);
+	mkdirSync(join(base, ".sf", "milestones", "M001", "slices", "S01", "tasks"), {
+		recursive: true,
+	});
+	return base;
+}
+
+const VALID_PARAMS = {
+	milestoneId: "M001",
+	sliceId: "S01",
+	taskId: "T01",
+	oneLiner: "Test task",
+	narrative: "Did the thing",
+	verification: "Checked it",
+	deviations: "None.",
+	knownIssues: "None.",
+	keyFiles: ["src/foo.ts"],
+	keyDecisions: ["Used approach A"],
+	blockerDiscovered: false,
+	verificationEvidence: [],
+};
+
+describe("handleCompleteTask — path-traversal ID rejection", () => {
+	it("rejects milestoneId containing path traversal (../etc)", async () => {
+		const base = makeTmpBase();
+		const result = await handleCompleteTask(
+			{ ...VALID_PARAMS, milestoneId: "../etc" },
+			base,
+		);
+		assert.ok(
+			"error" in result,
+			"result should be an error response",
+		);
+		assert.equal(
+			(result as { error: string }).error,
+			"unsafe_id",
+			'error should be "unsafe_id"',
+		);
+		assert.equal(
+			(result as { field?: string }).field,
+			"milestone_id",
+			'field should be "milestone_id"',
+		);
+		assert.ok(
+			typeof (result as { reason?: string }).reason === "string" &&
+				(result as { reason?: string }).reason!.length > 0,
+			"reason should be a non-empty string",
+		);
+	});
+
+	it("rejects sliceId containing path traversal (S01/../../x)", async () => {
+		const base = makeTmpBase();
+		const result = await handleCompleteTask(
+			{ ...VALID_PARAMS, sliceId: "S01/../../x" },
+			base,
+		);
+		assert.ok(
+			"error" in result,
+			"result should be an error response",
+		);
+		assert.equal(
+			(result as { error: string }).error,
+			"unsafe_id",
+			'error should be "unsafe_id"',
+		);
+		assert.equal(
+			(result as { field?: string }).field,
+			"slice_id",
+			'field should be "slice_id"',
+		);
+		assert.ok(
+			typeof (result as { reason?: string }).reason === "string" &&
+				(result as { reason?: string }).reason!.length > 0,
+			"reason should be a non-empty string",
+		);
+	});
+
+	it("rejects taskId containing newline (T01\\nbad)", async () => {
+		const base = makeTmpBase();
+		const result = await handleCompleteTask(
+			{ ...VALID_PARAMS, taskId: "T01\nbad" },
+			base,
+		);
+		assert.ok(
+			"error" in result,
+			"result should be an error response",
+		);
+		assert.equal(
+			(result as { error: string }).error,
+			"unsafe_id",
+			'error should be "unsafe_id"',
+		);
+		assert.equal(
+			(result as { field?: string }).field,
+			"task_id",
+			'field should be "task_id"',
+		);
+		assert.ok(
+			typeof (result as { reason?: string }).reason === "string" &&
+				(result as { reason?: string }).reason!.length > 0,
+			"reason should be a non-empty string",
+		);
+	});
+});

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

@@ -17,6 +17,7 @@ import {
 	transaction,
 	updateMilestoneStatus,
 } from "../sf-db.js";
+import { checkSafeIds } from "../safety/safe-id.js";
 import { invalidateStateCache } from "../state.js";
 import { isClosedStatus } from "../status-guards.js";
 import { extractVerdict } from "../verdict-parser.js";
@@ -150,6 +151,16 @@ export async function handleCompleteMilestone(
 		return { error: "title is required and must be a non-empty string" };
 	}
 
+	// ── Reject path-traversal in milestoneId before any path/DB usage ───────
+	const idCheck = checkSafeIds({ milestoneId: params.milestoneId });
+	if (idCheck) {
+		return {
+			error: "unsafe_id",
+			field: idCheck.fieldName,
+			reason: idCheck.reason,
+		} as unknown as { error: string };
+	}
+
 	// ── Verify that verification passed ─────────────────────────────────────
 	if (params.verificationPassed !== true) {
 		return {

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

@@ -25,6 +25,7 @@ import {
 	transaction,
 	updateSliceStatus,
 } from "../sf-db.js";
+import { checkSafeIds } from "../safety/safe-id.js";
 import { invalidateStateCache } from "../state.js";
 import { isClosedStatus } from "../status-guards.js";
 import type { CompleteSliceParams } from "../types.js";
@@ -441,6 +442,21 @@ 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

@@ -13,6 +13,7 @@ import { clearParseCache } from "../files.js";
 import { getGatesForTurn } from "../gate-registry.js";
 import { renderPlanCheckboxes } from "../markdown-renderer.js";
 import { clearPathCache, resolveSliceFile, resolveTasksDir } from "../paths.js";
+import { checkSafeIds } from "../safety/safe-id.js";
 import {
 	getMilestone,
 	getPendingGatesForTurn,
@@ -254,7 +255,7 @@ function paramsToTaskRow(
 export async function handleCompleteTask(
 	paramsInput: CompleteTaskParams,
 	basePath: string,
-): Promise<CompleteTaskResult | { error: string }> {
+): Promise<CompleteTaskResult | { error: string; field?: string; reason?: string }> {
 	let params: CompleteTaskParams;
 	try {
 		params = normalizeCompleteTaskParams(paramsInput);
@@ -285,6 +286,20 @@ 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/validate-milestone.ts

@@ -14,6 +14,7 @@ import { clearParseCache, saveFile } from "../files.js";
 import { insertMilestoneValidationGates } from "../milestone-validation-gates.js";
 import { clearPathCache, resolveMilestonePath } from "../paths.js";
 import { loadEffectiveSFPreferences } from "../preferences.js";
+import { checkSafeIds } from "../safety/safe-id.js";
 import {
 	deleteAssessmentByScope,
 	getMilestoneSlices,
@@ -116,6 +117,21 @@ export async function handleValidateMilestone(
 		};
 	}
 
+	// Reject path-traversal in any ID before they reach string interpolation
+	// below — this is the chokepoint. See safety/safe-id.ts for rationale and
+	// the validation-safe-id-path-segments self-feedback entry.
+	const idCheck = checkSafeIds({ milestoneId: params.milestoneId });
+	if (idCheck) {
+		return {
+			error: idCheck.message,
+			details: {
+				error: "unsafe_id",
+				field: idCheck.fieldName,
+				reason: idCheck.reason,
+			},
+		};
+	}
+
 	// ── Resolve paths and render markdown ────────────────────────────────
 	const validationMd = renderValidationMarkdown(params);
 

src/resources/extensions/sf/tools/workflow-tool-executors.ts

@@ -6,6 +6,7 @@ import {
 } from "../bootstrap/write-gate.js";
 import { saveArtifactToDb } from "../db-writer.js";
 import { GATE_REGISTRY } from "../gate-registry.js";
+import { checkSafeIds } from "../safety/safe-id.js";
 import {
 	getMilestone,
 	getSliceStatusSummary,
@@ -90,6 +91,31 @@ export async function executeSummarySave(
 			isError: true,
 		};
 	}
+	// Reject path-traversal in any ID before they reach string interpolation
+	// below — this is the chokepoint. See safety/safe-id.ts for rationale and
+	// the validation-safe-id-path-segments self-feedback entry.
+	const idCheck = checkSafeIds({
+		milestoneId: params.milestone_id,
+		sliceId: params.slice_id,
+		taskId: params.task_id,
+	});
+	if (idCheck) {
+		return {
+			content: [
+				{
+					type: "text",
+					text: `Error saving artifact: ${idCheck.message}`,
+				},
+			],
+			details: {
+				operation: "save_summary",
+				error: "unsafe_id",
+				field: idCheck.fieldName,
+				reason: idCheck.reason,
+			},
+			isError: true,
+		};
+	}
 	const contextGuard = shouldBlockContextArtifactSaveInSnapshot(
 		loadWriteGateSnapshot(basePath),
 		params.artifact_type,