fix(fabrika): scope build check's unvalidated list to the surface that ran (#5288)

claude-plugins/fabrika/skills/build/contract.md

@@ -988,9 +988,17 @@ fabrika build check --surface code
 `{"verdict": "green", "surface": "code", "tree": "<abs tree root>", "ran": ["pnpm typecheck", "pnpm lint:worktree"], "unvalidated": []}`.
 Red and unknown produce no stdout (`18` / `11`), diagnostics on stderr verbatim from the runners.
 
-`unvalidated` is always present and lists the changed files **this verdict does not cover** — the
-class no surface validates (`.yml`, `.sh`, `.sql`, `.css`, …). A non-empty list beside a green is the
-honest reading of a mixed diff, and the same line is repeated on stderr.
+`unvalidated` is always present and lists the changed files **this verdict does not cover** —
+computed against *this* surface's validators, so it holds both the class no surface validates
+(`.yml`, `.sh`, `.sql`, `.css`, …) and the class another surface would have read. Markdown under
+`--surface code` is the common case, and its mirror is code under `--surface plan`. A non-empty list
+beside a green is the honest reading of a mixed diff, and the same line is repeated on stderr.
+
+The list is a **disclosure, not a second validator run**: `--surface code` names the markdown it
+skipped and does not scan it. Running the markdown validators there would make the surface guess at
+file classes, which the anchor exists to refuse — so the remedy for a mixed diff that needs its
+markdown read is a second run, not a wider surface. `unvalidated: []` therefore means every changed
+file was read by a validator that passed, and nothing weaker (#5288).
 
 Per surface:
 
@@ -1043,17 +1051,19 @@ Preconditions: a linked worktree (`12`), the lane's branch checked out (`14`).
 | `build check: the diff against <base> is empty — nothing to validate (ADR 0092).` | 7 | refusal |
 | `build check: red — <runner> failed; diagnostics above.` | 18 | refusal |
 | `build check: no surface validates any of the <n> changed file(s) (<files>) — there is nothing here to run, so the verdict is a refusal, never green.` | 22 | refusal |
+| `build check: <n> changed file(s) --surface <surface> does not validate — NOT covered by this verdict: <files>.` | 0 | scope note beside a green |
 
 **Scope** — this tree's diff against the branch base. A zero-file diff is `7` — zero scope, never
 a green (ADR 0092). A diff no surface validates is `22` — the same rule one step further in: a file
 the verb cannot classify is a file it cannot check, and an unchecked file never counts toward a
-green. A green's `unvalidated` list is what keeps the partial case honest.
+green. A green's `unvalidated` list is what keeps the partial case honest — and it is scoped to the
+surface that ran, so a file another surface would have read counts as uncovered here too.
 
 **Example**
 
 ```
 $ fabrika build check --surface code
-{"verdict":"green","surface":"code","tree":"/private/var/folders/…/build-4312","ran":["pnpm typecheck","pnpm lint:worktree"],"unvalidated":["scripts/deploy.sh"]}
+{"verdict":"green","surface":"code","tree":"/private/var/folders/…/build-4312","ran":["pnpm typecheck","pnpm lint:worktree"],"unvalidated":["README.md","scripts/deploy.sh"]}
 ```
 
 **Grounding**
@@ -1062,6 +1072,10 @@ $ fabrika build check --surface code
 - #5229 — two extension patterns and no third class: a workflow-only diff greened under `--surface
   prose` having opened no file, and refused under `--surface code` with a message pointing at the
   branch that greened. `22` and `unvalidated` are the two halves of that fix.
+- #5288 — `unvalidated` was computed from the third class alone, so `--surface code` over
+  `["a.ts", "README.md"]` greened with an empty list: the markdown had a validator, just not the one
+  that ran. Scoping the list to the surface closes it, and the mirrored `--surface plan` case, with
+  one rule.
 - v1's discipline was prose-only (`SKILL.md:895-935`, exact-CI-command mandate with no
   enforcement); here the command set is the verb's, not the agent's memory.
 - ADR 0092 — zero diff is a refusal, not a vacuous green.

packages/fabrika-cli/src/build/check-verb.ts

@@ -16,7 +16,8 @@
  * provably contradicts.
  *
  * **Green means "the validators ran and passed", never "I could not tell."** See {@link classifyDiff}
- * for the third file class that keeps that distinction representable (#5229).
+ * for the third file class that keeps that distinction representable (#5229), and
+ * {@link notCoveredBy} for the per-surface coverage the green's `unvalidated` list reports (#5288).
  */
 import {Effect, FileSystem} from "effect";
 import type {ChildProcessSpawner} from "effect/unstable/process";
@@ -59,25 +60,58 @@ export interface CheckOptions {
 }
 
 /**
- * The changed files split three ways, with `unvalidated` the file class **no** surface validates.
+ * The changed files split three ways, with `unvalidatable` the file class **no** surface validates.
  *
  * That third bucket is the point. Filtering with the two regexes and reading nothing off what fell
  * out of both made "matched neither" an absence, and an absence cannot be refused: a `.yml`/`.sh`
  * diff produced an empty markdown list, zero validator iterations and a green that had opened no
  * file (#5229). Named, it is a state the verb can act on.
+ *
+ * `unvalidatable` is a property of the **tree** — no surface covers these files. Whether *this* run
+ * covered a file is a narrower question, and {@link notCoveredBy} is the one that answers it; the two
+ * were the same word once, which is how a markdown file could sit outside a `--surface code` green's
+ * disclosure while the field's own documentation said it listed everything the verdict missed (#5288).
  */
 export interface DiffClasses {
 	readonly code: ReadonlyArray<string>;
 	readonly markdown: ReadonlyArray<string>;
-	readonly unvalidated: ReadonlyArray<string>;
+	readonly unvalidatable: ReadonlyArray<string>;
 }
 
 export const classifyDiff = (files: ReadonlyArray<string>): DiffClasses => ({
 	code: files.filter((f) => CODE_RE.test(f)),
 	markdown: files.filter((f) => MARKDOWN_RE.test(f)),
-	unvalidated: files.filter((f) => !CODE_RE.test(f) && !MARKDOWN_RE.test(f)),
+	unvalidatable: files.filter((f) => !CODE_RE.test(f) && !MARKDOWN_RE.test(f)),
 });
 
+/** The file classes each surface's validators actually open. `unvalidatable` is in no surface's. */
+const COVERS: Record<Surface, ReadonlyArray<keyof DiffClasses>> = {
+	code: ["code"],
+	prose: ["markdown"],
+	plan: ["markdown"],
+};
+
+/**
+ * The changed files this surface's validators do not read — what a green must disclose.
+ *
+ * A superset of the `unvalidatable` bucket, and the extra members are the whole point: `--surface
+ * code` ran typecheck and `lint:worktree` over a `["a.ts", "README.md"]` diff, neither of which reads
+ * markdown (`lint:worktree` filters `.md` out by extension), and greened with an empty disclosure —
+ * which affirmatively reads as "nothing uncovered". `--surface plan` did the same to code files.
+ * Reporting coverage per surface answers both with one rule instead of two (#5288).
+ *
+ * Disclosing is deliberately not validating: running the markdown validators under `--surface code`
+ * would make the surface guess at file classes, which the anchor exists to refuse.
+ */
+export const notCoveredBy = (
+	surface: Surface,
+	files: ReadonlyArray<string>,
+): ReadonlyArray<string> => {
+	const classes = classifyDiff(files);
+	const covered = new Set(COVERS[surface].flatMap((bucket) => classes[bucket]));
+	return files.filter((file) => !covered.has(file));
+};
+
 /**
  * Why no surface can validate this diff at all, or `null`.
  *
@@ -86,11 +120,11 @@ export const classifyDiff = (files: ReadonlyArray<string>): DiffClasses => ({
  * sentence pointing at the wrong remedy (it invites `--surface prose`, the branch that greened).
  */
 export const unvalidatableDiff = (files: ReadonlyArray<string>): string | null => {
-	const {code, markdown, unvalidated} = classifyDiff(files);
+	const {code, markdown, unvalidatable} = classifyDiff(files);
 	if (code.length > 0 || markdown.length > 0) return null;
-	const shown = unvalidated.slice(0, 5).join(", ");
-	const rest = unvalidated.length > 5 ? `, +${unvalidated.length - 5} more` : "";
-	return `no surface validates any of the ${unvalidated.length} changed file(s) (${shown}${rest})`;
+	const shown = unvalidatable.slice(0, 5).join(", ");
+	const rest = unvalidatable.length > 5 ? `, +${unvalidatable.length - 5} more` : "";
+	return `no surface validates any of the ${unvalidatable.length} changed file(s) (${shown}${rest})`;
 };
 
 /** Why `--surface` provably contradicts the diff, or `null`. */
@@ -247,15 +281,17 @@ export const runCheck = (
 		if (mismatch !== null) {
 			return refuse(OFF_VOCABULARY, `${VERB}: ${mismatch} — the surface is provably wrong.`, scope);
 		}
-		const {markdown, unvalidated} = classifyDiff(files);
-		// A green over a partly-unvalidatable diff has to carry what it skipped, on both channels:
-		// #5187 greened over 25 workflow files whose `ran` line was true and misleading at once (#5229).
+		const {markdown} = classifyDiff(files);
+		// A partial green has to carry what it skipped, on both channels: #5187 greened over 25 workflow
+		// files whose `ran` line was true and misleading at once (#5229), and a `--surface code` green
+		// then did the same to markdown while reporting an empty list (#5288).
+		const unvalidated = notCoveredBy(surface as Surface, files);
 		const noted =
 			unvalidated.length === 0
 				? scope
 				: [
 						...scope,
-						`${VERB}: ${unvalidated.length} changed file(s) no surface validates — NOT covered by this verdict: ${unvalidated.join(", ")}.`,
+						`${VERB}: ${unvalidated.length} changed file(s) --surface ${surface} does not validate — NOT covered by this verdict: ${unvalidated.join(", ")}.`,
 					];
 
 		if (surface === "code") {

packages/fabrika-cli/src/build/check-verb.unit.test.ts

@@ -2,7 +2,7 @@ import {Effect, Layer} from "effect";
 import {describe, expect, it} from "vitest";
 import {errOut, fakeFs, fakeShell, okOut} from "../fakes.test-support.ts";
 import type {ExecResult} from "../io/exec.ts";
-import {classifyDiff, runCheck, surfaceMismatch} from "./check-verb.ts";
+import {classifyDiff, notCoveredBy, runCheck, surfaceMismatch} from "./check-verb.ts";
 import {
 	OFF_VOCABULARY,
 	PRECONDITION_UNKNOWN,
@@ -76,14 +76,37 @@ describe("classifyDiff — matched-neither is a bucket, not an absence", () => {
 		expect(classifyDiff([".github/workflows/ci.yml", "scripts/x.sh", "a.ts", "R.md"])).toEqual({
 			code: ["a.ts"],
 			markdown: ["R.md"],
-			unvalidated: [".github/workflows/ci.yml", "scripts/x.sh"],
+			unvalidatable: [".github/workflows/ci.yml", "scripts/x.sh"],
 		});
 	});
 
 	it("puts every file in exactly one bucket", () => {
 		const files = ["a.tsx", "b.mjs", "c.json", "d.md", "e.mdx", "f.sql", "g.css", "LICENSE"];
-		const {code, markdown, unvalidated} = classifyDiff(files);
-		expect([...code, ...markdown, ...unvalidated].sort()).toEqual([...files].sort());
+		const {code, markdown, unvalidatable} = classifyDiff(files);
+		expect([...code, ...markdown, ...unvalidatable].sort()).toEqual([...files].sort());
+	});
+});
+
+describe("notCoveredBy — a green discloses what THIS surface did not read", () => {
+	it("names the markdown a code run skipped", () => {
+		expect(notCoveredBy("code", ["a.ts", "README.md"])).toEqual(["README.md"]);
+	});
+
+	it("names the code a plan run skipped — the symmetric case, same rule", () => {
+		expect(notCoveredBy("plan", ["a.ts", "plans/epic.md"])).toEqual(["a.ts"]);
+	});
+
+	it("is empty only when the surface read every changed file", () => {
+		expect(notCoveredBy("code", ["a.ts", "b.tsx"])).toEqual([]);
+		expect(notCoveredBy("prose", ["docs/a.md"])).toEqual([]);
+	});
+
+	it("still carries the class no surface validates", () => {
+		expect(notCoveredBy("code", ["a.ts", "scripts/deploy.sh"])).toEqual(["scripts/deploy.sh"]);
+	});
+
+	it("reports in diff order, so the list reads against the diff it came from", () => {
+		expect(notCoveredBy("code", ["R.md", "a.ts", "x.sh"])).toEqual(["R.md", "x.sh"]);
 	});
 });
 
@@ -252,6 +275,62 @@ describe("runCheck", () => {
 		expect(JSON.parse(out.stdout).unvalidated).toEqual(["scripts/deploy.sh"]);
 	});
 
+	// The #5288 regression. README.md landed in the markdown bucket, so the green listed nothing — and
+	// an empty `unvalidated` reads as "nothing uncovered" over a file no runner opened (`lint:worktree`
+	// filters `.md` out by extension).
+	it("names the markdown a --surface code green did not read", async () => {
+		const out = await run(
+			[
+				...LANE_OK,
+				[DIFF, okOut("apps/web/src/App.tsx\nREADME.md\n")],
+				[TYPECHECK, okOut("")],
+				[LINT, okOut("")],
+			],
+			{},
+			{"/repo/trees/lane-a/README.md": "nothing to resolve here\n"},
+		);
+		expect(out.code).toBe(0);
+		expect(JSON.parse(out.stdout).unvalidated).toEqual(["README.md"]);
+		expect(out.stderr).toContain(
+			"build check: 1 changed file(s) --surface code does not validate — NOT covered by this verdict: README.md.",
+		);
+	});
+
+	it("names the code a --surface plan green did not read — same rule, mirrored", async () => {
+		const shell = fakeShell([...LANE_OK, [DIFF, okOut("apps/web/src/App.tsx\nplans/epic.md\n")]]);
+		const out = await Effect.runPromise(
+			Effect.provide(
+				runCheck({...options, surface: "plan"}),
+				Layer.merge(
+					shell.layer,
+					fakeFs({
+						files: {"/repo/trees/lane-a/plans/epic.md": "## Dependencies\n\n- phase 1: #12\n"},
+					}).layer,
+				),
+			),
+		);
+		expect(out.code).toBe(0);
+		expect(JSON.parse(out.stdout).unvalidated).toEqual(["apps/web/src/App.tsx"]);
+		expect(shell.calls).not.toContain("pnpm typecheck --force");
+	});
+
+	// Disclosing is not validating: --surface code names the markdown it skipped and stays green over
+	// content the prose validators would red. Widening the surface to scan it is the fix #5288 declined.
+	it("discloses the skipped markdown without scanning it", async () => {
+		const out = await run(
+			[
+				...LANE_OK,
+				[DIFF, okOut("apps/web/src/App.tsx\ndocs/guide.md\n")],
+				[TYPECHECK, okOut("")],
+				[LINT, okOut("")],
+			],
+			{},
+			{"/repo/trees/lane-a/docs/guide.md": "run it from /Users/someone/phoenix\n"},
+		);
+		expect(out.code).toBe(0);
+		expect(JSON.parse(out.stdout).unvalidated).toEqual(["docs/guide.md"]);
+	});
+
 	it("reds a plan diff whose Dependencies block does not parse", async () => {
 		const out = await run(
 			[...LANE_OK, [DIFF, okOut("plans/epic.md\n")]],