diff --git a/.changeset/explicit-review-guidance.md b/.changeset/explicit-review-guidance.md new file mode 100644 index 00000000..66cc4647 --- /dev/null +++ b/.changeset/explicit-review-guidance.md @@ -0,0 +1,5 @@ +--- +"@design-intelligence/ghost": minor +--- + +Allow `ghost review --node ` to include host-selected guidance and offer referencing checks alongside existing material matches without filtering other checks. diff --git a/packages/ghost/README.md b/packages/ghost/README.md index 43f49113..7864c963 100644 --- a/packages/ghost/README.md +++ b/packages/ghost/README.md @@ -64,6 +64,18 @@ the catalog without grounding a task. Run `ghost --help` for the core workflow and `ghost --help` for current flags and command behavior. +When new work uses unchanged components or prose guidance, name the nodes that +govern the change: + +```bash +ghost review --node component.button --node voice +``` + +Repeat `--node` for each applicable ID. This adds guidance and referencing +checks alongside existing diff matches; it never filters other checks. +Unknown IDs stop review rather than produce a partial packet. Review still +requires `.ghost/checks/`, and the agent decides which offered checks apply. + ## Library ```ts diff --git a/packages/ghost/src/commands/command-discovery.ts b/packages/ghost/src/commands/command-discovery.ts index 737903bd..c1119c36 100644 --- a/packages/ghost/src/commands/command-discovery.ts +++ b/packages/ghost/src/commands/command-discovery.ts @@ -149,7 +149,7 @@ const COMMAND_DISCOVERY = [ defaultHelp: true, compactName: "review", summary: - "Emit an advisory review packet for a diff (needs .ghost/checks/).", + "Review a diff with matched and optional explicit guidance (needs .ghost/checks/).", }, { name: "checks", diff --git a/packages/ghost/src/commands/review-command.ts b/packages/ghost/src/commands/review-command.ts index 7cbbd191..401366d7 100644 --- a/packages/ghost/src/commands/review-command.ts +++ b/packages/ghost/src/commands/review-command.ts @@ -3,7 +3,9 @@ import { readFile } from "node:fs/promises"; import { resolve } from "node:path"; import { promisify } from "node:util"; import type { CAC } from "cac"; +import { UsageError } from "#ghost-core"; import { loadGhostPackage, resolveGhostPackage } from "../package.js"; +import { validateExplicitReviewNodes } from "../review/resolve.js"; import { buildReviewPacket, formatReviewPacket, @@ -17,12 +19,16 @@ export function registerReviewCommand(cli: CAC): void { cli .command( "review", - "Emit an advisory review packet for a diff using material-backed nodes and checks.", + "Emit an advisory review packet for a diff using matched and explicit guidance plus checks.", ) .option( "--package ", "Use this ghost package directory (default: ./.ghost)", ) + .option( + "--node ", + "Add explicit guidance and its checks; repeat for multiple IDs (does not filter matches or always-offered checks)", + ) .option("--base ", "Git ref to diff against (default: HEAD)") .option("--diff ", "Read diff from a file, or '-' for stdin") .option("--format ", "Output format: markdown or json", { @@ -44,6 +50,10 @@ export function registerReviewCommand(cli: CAC): void { await exitCli(2); return; } + const nodeIds = validateExplicitReviewNodes( + ghostPackage.catalog, + normalizeNodeOption(opts.node), + ); const diffText = await resolveDiff({ base: opts.base, diff: opts.diff, @@ -51,6 +61,7 @@ export function registerReviewCommand(cli: CAC): void { const packet = await buildReviewPacket(ghostPackage, diffText, { packageDir: paths.packageDir, cwd: process.cwd(), + nodeIds, }); process.stdout.write( format === "json" @@ -64,6 +75,19 @@ export function registerReviewCommand(cli: CAC): void { }); } +function normalizeNodeOption(value: unknown): string[] { + if (value === undefined) return []; + const values = Array.isArray(value) ? value : [value]; + return values.map((id: unknown) => { + if (typeof id !== "string" || id.trim().length === 0) { + throw new UsageError( + "--node requires a nonempty node ID for each occurrence.", + ); + } + return id; + }); +} + async function resolveDiff(options: { base?: string; diff?: string; diff --git a/packages/ghost/src/review/resolve.ts b/packages/ghost/src/review/resolve.ts index 6a00fa32..09d8fba2 100644 --- a/packages/ghost/src/review/resolve.ts +++ b/packages/ghost/src/review/resolve.ts @@ -4,7 +4,9 @@ import { type MaterialTransportOptions, materialLocator, materialLocatorClaimsPath, + NodeIdSchema, parseCheckReference, + UsageError, } from "#ghost-core"; import type { LoadedCheck } from "../scan/check-files.js"; import { parseTouchedFiles, type TouchedFile } from "./diff.js"; @@ -18,8 +20,9 @@ export interface MatchedMaterialNode { export interface OfferedCheck { id: string; severity: string | undefined; - offered: "matched" | "always"; + offered: "matched" | "always" | "explicit"; via: string[]; + explicitVia?: string[]; } export interface CoverageGap { @@ -30,18 +33,39 @@ export interface CoverageGap { } export interface ReviewResolution { + explicitNodeIds: string[]; touchedFiles: TouchedFile[]; materialNodes: MatchedMaterialNode[]; offeredChecks: OfferedCheck[]; gaps: CoverageGap[]; } +/** Validate the complete selection before any review work; identities are exact. */ +export function validateExplicitReviewNodes( + catalog: GhostCatalog, + ids: readonly string[] = [], +): string[] { + const uniqueIds = [...new Set(ids)]; + const invalid = uniqueIds.filter( + (id) => !NodeIdSchema.safeParse(id).success || !catalog.nodes.has(id), + ); + if (invalid.length > 0) { + throw new UsageError( + `Invalid or unknown review node IDs: ${invalid.map((id) => JSON.stringify(id)).join(", ")}. Run ghost gather --format json with the same --package to find exact node IDs.`, + ); + } + return uniqueIds; +} + export function resolveReview( catalog: GhostCatalog, checks: Map, diffText: string, transport: MaterialTransportOptions, + nodeIds: readonly string[] = [], ): ReviewResolution { + const explicitNodeIds = validateExplicitReviewNodes(catalog, nodeIds); + const explicitNodes = new Set(explicitNodeIds); const touchedFiles = parseTouchedFiles(diffText); const materialNodeIds = new Set(); const matched = new Map< @@ -83,22 +107,39 @@ export function resolveReview( for (const check of checks.values()) { const matchedRefs: string[] = []; + const explicitRefs: string[] = []; let referencesMaterial = false; for (const raw of check.references) { const ref = parseCheckReference(raw); if (ref === null) continue; + if (explicitNodes.has(ref.nodeId)) explicitRefs.push(raw); if (materialNodeIds.has(ref.nodeId)) { referencesMaterial = true; referencedMaterialNodes.add(ref.nodeId); if (touchedMaterialNodes.has(ref.nodeId)) matchedRefs.push(raw); } } - if (matchedRefs.length > 0 || !referencesMaterial) { + if ( + matchedRefs.length > 0 || + !referencesMaterial || + explicitRefs.length > 0 + ) { offeredChecks.push({ id: check.id, severity: check.doc.frontmatter.severity, - offered: matchedRefs.length > 0 ? "matched" : "always", - via: matchedRefs.length > 0 ? matchedRefs : check.references.slice(), + offered: + matchedRefs.length > 0 + ? "matched" + : !referencesMaterial + ? "always" + : "explicit", + via: + matchedRefs.length > 0 + ? matchedRefs + : !referencesMaterial + ? check.references.slice() + : explicitRefs, + ...(explicitRefs.length > 0 ? { explicitVia: explicitRefs } : {}), }); } } @@ -111,7 +152,9 @@ export function resolveReview( gaps.push({ kind: "unmatched-file", detail: - "changed files match no node `materials` locators — no ghost package guidance claims them", + explicitNodeIds.length > 0 + ? "changed files have no local material locator matches" + : "changed files match no node `materials` locators — no ghost package guidance claims them", files: unmatched, }); } @@ -135,6 +178,7 @@ export function resolveReview( })); return { + explicitNodeIds, touchedFiles, materialNodes: matchedNodes, offeredChecks, diff --git a/packages/ghost/src/review/review-packet.ts b/packages/ghost/src/review/review-packet.ts index b209655e..d39e0be3 100644 --- a/packages/ghost/src/review/review-packet.ts +++ b/packages/ghost/src/review/review-packet.ts @@ -30,11 +30,18 @@ export interface PacketMaterialNode { files: string[]; } +/** Explicit guidance without invented touched-file or material-match claims. */ +export type PacketExplicitNode = Omit< + PacketMaterialNode, + "matchedMaterials" | "files" +>; + export interface PacketCheck { id: string; severity: string | undefined; - offered: "matched" | "always"; + offered: "matched" | "always" | "explicit"; via: string[]; + explicitVia?: string[]; prose: string; baseline: BaselineProse[]; } @@ -45,6 +52,10 @@ export interface ReviewPacket { diagnostics: ReadonlyArray>; touchedFiles: string[]; materialNodes: PacketMaterialNode[]; + /** Present only when the host explicitly names guidance for this review. */ + explicitNodeIds?: string[]; + /** Selected nodes not already included in materialNodes. */ + explicitNodes?: PacketExplicitNode[]; checks: PacketCheck[]; gaps: CoverageGap[]; diff: string; @@ -55,6 +66,7 @@ export interface BuildReviewPacketOptions { /** Absolute path of the ghost package directory (default: cwd/.ghost). */ packageDir?: string; cwd?: string; + nodeIds?: readonly string[]; } export async function buildReviewPacket( @@ -72,12 +84,27 @@ export async function buildReviewPacket( packageDir: options.packageDir ?? join(cwd, ".ghost"), materialsDir: GHOST_MATERIALS_DIR, }, + options.nodeIds, ); const materialNodes: PacketMaterialNode[] = resolution.materialNodes.map( (matched) => materialNodeFromMatch(ghostPackage, matched), ); + const matchedIds = new Set(materialNodes.map((node) => node.id)); + const explicitNodes = resolution.explicitNodeIds + .filter((id) => !matchedIds.has(id)) + .map((id): PacketExplicitNode => { + const node = ghostPackage.catalog.nodes.get(id) as GhostCatalogNode; + return { + id: node.id, + ...(node.kind !== undefined ? { kind: node.kind } : {}), + ...(node.for !== undefined ? { for: node.for } : {}), + prose: node.body, + materials: node.materials ?? [], + }; + }); + const checks: PacketCheck[] = resolution.offeredChecks.map((offered) => { const check = ghostPackage.checks.get(offered.id); return { @@ -85,6 +112,7 @@ export async function buildReviewPacket( severity: offered.severity, offered: offered.offered, via: offered.via, + ...(offered.explicitVia ? { explicitVia: offered.explicitVia } : {}), prose: check?.doc.body.trim() ?? "", baseline: check?.references @@ -98,6 +126,9 @@ export async function buildReviewPacket( diagnostics: [...ghostPackage.invalid, ...ghostPackage.invalidChecks], touchedFiles: resolution.touchedFiles.map((file) => file.path), materialNodes, + ...(resolution.explicitNodeIds.length > 0 + ? { explicitNodeIds: resolution.explicitNodeIds, explicitNodes } + : {}), checks, gaps: resolution.gaps, diff: diffText, @@ -158,13 +189,56 @@ export function formatReviewPacket(packet: ReviewPacket): string { : undefined; out.push(`- \`${locator}\`${note ? ` — Note: ${note}` : ""}`); } + if (packet.explicitNodeIds?.includes(node.id)) { + const additional = node.materials.filter( + (material) => + !node.matchedMaterials.includes(materialLocator(material)), + ); + if (additional.length > 0) { + out.push("Other declared materials (not diff matches):"); + for (const material of additional) { + const { locator, note } = normalizeMaterial(material); + out.push(`- \`${locator}\`${note ? `: ${note}` : ""}`); + } + } + } out.push("Files:"); for (const file of node.files) out.push(`- \`${file}\``); out.push(""); } } - const shownNodes = new Set(packet.materialNodes.map((node) => node.id)); + if (packet.explicitNodeIds?.length) { + out.push( + "## Explicit guidance", + "", + "The host supplied these nodes for this review. Weigh their applicability; selection does not establish a file match.", + "", + ); + const matchedIds = new Set(packet.materialNodes.map((node) => node.id)); + for (const id of packet.explicitNodeIds) { + if (matchedIds.has(id)) out.push(`- \`${id}\` (prose shown above)`); + } + for (const node of packet.explicitNodes ?? []) { + out.push(`### \`${node.id}\``, ""); + if (node.for) out.push(`Applies when: ${node.for}`, ""); + out.push(node.prose, ""); + if (node.materials.length > 0) { + out.push("Declared materials (not diff matches):"); + for (const material of node.materials) { + const { locator, note } = normalizeMaterial(material); + out.push(`- \`${locator}\`${note ? `: ${note}` : ""}`); + } + out.push(""); + } + } + out.push(""); + } + + const shownNodes = new Set([ + ...packet.materialNodes.map((node) => node.id), + ...(packet.explicitNodes ?? []).map((node) => node.id), + ]); const shownSections = new Set(); out.push("## Offered checks — weigh which apply"); if (packet.checks.length === 0) { @@ -178,9 +252,17 @@ export function formatReviewPacket(packet: ReviewPacket): string { out.push( check.offered === "matched" ? `Offered via material match: ${refs}` - : `Always offered — no referenced material-backed node gates it: ${refs}`, + : check.offered === "explicit" + ? `Offered via explicit guidance: ${refs}` + : `Always offered — no referenced material-backed node gates it: ${refs}`, "", ); + if (check.explicitVia?.length && check.offered !== "explicit") { + out.push( + `Also selected explicitly: ${check.explicitVia.map((ref) => `\`${ref}\``).join(", ")}`, + "", + ); + } if (check.baseline.length > 0) { out.push("Baseline prose:"); for (const baseline of check.baseline) { diff --git a/packages/ghost/src/skill-bundle/SKILL.md b/packages/ghost/src/skill-bundle/SKILL.md index 9ab99770..7c834d29 100644 --- a/packages/ghost/src/skill-bundle/SKILL.md +++ b/packages/ghost/src/skill-bundle/SKILL.md @@ -96,6 +96,10 @@ tuning. `review` does no grading. It assembles the review packet: touched files, matched material-backed nodes, offered checks with baseline prose, loading failures, coverage gaps, and the diff. The host agent renders findings. +Use `ghost review --node ` for guidance that governs the change but may not +match touched material files. Repeat the flag for multiple IDs. Explicit +selection adds context and checks; it does not filter other checks or grade +applicability. For visual work, do not stop at generation: ground, make, then verify in two tracks, repair within budget, and review. See diff --git a/packages/ghost/src/skill-bundle/references/making.md b/packages/ghost/src/skill-bundle/references/making.md index cfb005fe..2d3b9852 100644 --- a/packages/ghost/src/skill-bundle/references/making.md +++ b/packages/ghost/src/skill-bundle/references/making.md @@ -73,9 +73,12 @@ pass fails, stop patching and re-inspect the pulled guidance and materials, or ask for human review. When the artifact holds, run `ghost review` when `.ghost/checks/` exists and a -diff is available. Judge the packet yourself. Report what was made, which node -ids governed it, what was verified and how, what stayed provisional, and what -was not inspected. +diff is available. Pass `--node ` for guidance that governed this change, +repeating the flag as needed, especially when its material files are unchanged. +Do not forward every previously pulled node without checking whether it applies +to this change. Judge the packet yourself. Report what was made, which node ids +governed it, what was verified and how, what stayed provisional, and what was +not inspected. ## Render honesty diff --git a/packages/ghost/src/skill-bundle/references/schema.md b/packages/ghost/src/skill-bundle/references/schema.md index ded92b27..0580a12b 100644 --- a/packages/ghost/src/skill-bundle/references/schema.md +++ b/packages/ghost/src/skill-bundle/references/schema.md @@ -133,6 +133,18 @@ it does not grade them. relevant checks, includes loading diagnostics, and emits referenced baseline prose for the host agent. Repeated baselines point to prose already included in the packet. + Repeat `--node ` to add exact guidance IDs, including nested IDs or the + cover. The cover is not added automatically. Unknown IDs stop review with + exit 2; duplicates are ignored in first-request order. The flag accepts node + IDs rather than paths to node files, globs, or heading references. + Explicit selection adds referencing checks without suppressing material + matches or always-offered checks. JSON adds `explicitNodeIds` and + `explicitNodes` only when IDs are supplied; matched nodes keep their prose in + `materialNodes`. Check provenance retains `matched` or `always` where it + already applies, uses `explicit` for newly offered checks, and records + explicit reasons in `explicitVia`. Selected guidance without a check remains + visible. File coverage gaps are still material-match gaps, not evidence that + explicitly selected guidance does not apply. - `ghost stats` summarizes local gather and pull events. - `ghost skill check` compares an installed `SKILL.md` and `references/` with this CLI's bundle. It uses install's `--agent` and `--dest` resolution, diff --git a/packages/ghost/test/review-explicit-packet.test.ts b/packages/ghost/test/review-explicit-packet.test.ts new file mode 100644 index 00000000..9c76d1af --- /dev/null +++ b/packages/ghost/test/review-explicit-packet.test.ts @@ -0,0 +1,246 @@ +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { loadGhostPackage, resolveGhostPackage } from "../src/package.js"; +import { + buildReviewPacket, + formatReviewPacket, +} from "../src/review/review-packet.js"; + +const BUTTON = + "## Usage\n\nKeep the action accountable.\n\n## Rules\n\nUse the approved button."; +const VOICE = "State what happened without applause."; +const changed = (path: string) => + `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -1 +1 @@\n-old\n+new\n`; + +describe("explicit review packet", () => { + let dir: string; + let packageDir: string; + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "ghost-review-explicit-")); + packageDir = join(dir, ".ghost"); + await mkdir(join(packageDir, "checks"), { recursive: true }); + await writeFile( + join(packageDir, "manifest.yml"), + "schema: ghost.package/v1\nid: explicit-review\ncover: brand\n", + ); + await writeFile( + join(packageDir, "brand.md"), + "---\nfor: All brand work.\n---\n\nBrand stance.\n", + ); + await writeFile( + join(packageDir, "component.button.md"), + `---\nfor: Composing an action.\nmaterials:\n - locator: components/button.tsx\n note: Canonical control implementation\n - https://example.com/button\n---\n\n${BUTTON}\n`, + ); + await writeFile( + join(packageDir, "voice.md"), + `---\nfor: Writing copy.\n---\n\n${VOICE}\n`, + ); + await writeFile( + join(packageDir, "checks", "button.md"), + "---\nname: Button\ndescription: Check the action.\nseverity: high\nreferences:\n - component.button > Rules\n - voice\n---\n\nAssess both control and copy.\n", + ); + await writeFile( + join(packageDir, "checks", "voice.md"), + "---\nname: Voice\ndescription: Check copy.\nseverity: medium\nreferences:\n - voice\n---\n\nAssess the voice.\n", + ); + }); + afterEach(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + async function packet( + nodeIds?: readonly string[], + diff = changed("checkout.tsx"), + ) { + return buildReviewPacket( + await loadGhostPackage(resolveGhostPackage(packageDir, dir)), + diff, + { cwd: dir, packageDir, nodeIds }, + ); + } + + it("includes unchanged material guidance, full declarations, and every check baseline", async () => { + const result = await packet(["component.button"]); + expect(result.materialNodes).toEqual([]); + expect(result.explicitNodeIds).toEqual(["component.button"]); + expect(result.explicitNodes).toEqual([ + { + id: "component.button", + kind: "component", + for: "Composing an action.", + prose: BUTTON, + materials: [ + { + locator: "components/button.tsx", + note: "Canonical control implementation", + }, + "https://example.com/button", + ], + }, + ]); + const check = result.checks.find((item) => item.id === "button"); + expect(check).toMatchObject({ + offered: "explicit", + via: ["component.button > Rules"], + explicitVia: ["component.button > Rules"], + }); + expect(check?.baseline.map((baseline) => baseline.body)).toEqual([ + "Use the approved button.", + VOICE, + ]); + expect(result.checks.map((item) => item.id)).toEqual(["button", "voice"]); + expect( + result.gaps.find((gap) => gap.kind === "unmatched-file")?.files, + ).toEqual(["checkout.tsx"]); + const markdown = formatReviewPacket(result); + expect(markdown).toContain("Explicit guidance"); + expect(markdown).toContain(BUTTON); + expect(markdown).toContain("Canonical control implementation"); + expect(markdown).toContain("https://example.com/button"); + expect(markdown).toContain(VOICE); + expect(markdown).toContain("Offered via explicit guidance"); + expect(markdown.split("Use the approved button.")).toHaveLength(2); + expect(markdown).toContain(result.diff.trimEnd()); + }); + + it("preserves load diagnostics alongside explicit guidance, checks, and coverage gaps", async () => { + await writeFile( + join(packageDir, "broken.md"), + "---\nfor: [not, text]\n---\n\nInvalid guidance.\n", + ); + await writeFile( + join(packageDir, "checks", "broken.md"), + "No frontmatter.\n", + ); + const result = await packet(["component.button"]); + expect(result.diagnostics.map((entry) => entry.file)).toEqual([ + "broken.md", + "checks/broken.md", + ]); + expect(result.explicitNodeIds).toEqual(["component.button"]); + expect(result.materialNodes).toEqual([]); + expect(result.checks.find((check) => check.id === "button")).toMatchObject({ + offered: "explicit", + explicitVia: ["component.button > Rules"], + }); + expect(result.checks.map((check) => check.id)).toEqual(["button", "voice"]); + expect(result.gaps).toContainEqual( + expect.objectContaining({ + kind: "unmatched-file", + files: ["checkout.tsx"], + }), + ); + const markdown = formatReviewPacket(result); + for (const diagnostic of result.diagnostics) { + expect(markdown).toContain(diagnostic.file); + expect(markdown).toContain(diagnostic.message); + } + expect(markdown).toContain("ghost validate"); + expect(markdown).toContain("## Explicit guidance"); + expect(markdown).toContain(BUTTON); + expect(markdown.split("Use the approved button.")).toHaveLength(2); + expect(markdown).toContain("Baseline prose shown above."); + expect(markdown).toContain(VOICE); + expect(markdown).toContain("## Coverage gaps"); + expect(markdown).toContain(result.diff.trimEnd()); + }); + + it("keeps matched and always-offered provenance when explicit selection overlaps", async () => { + const result = await packet( + ["voice", "component.button", "voice"], + changed("components/button.tsx"), + ); + expect(result.explicitNodeIds).toEqual(["voice", "component.button"]); + expect(result.explicitNodes?.map((node) => node.id)).toEqual(["voice"]); + expect(result.materialNodes.map((node) => node.id)).toEqual([ + "component.button", + ]); + expect(result.checks).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + id: "button", + offered: "matched", + via: ["component.button > Rules"], + explicitVia: ["component.button > Rules", "voice"], + }), + expect.objectContaining({ + id: "voice", + offered: "always", + via: ["voice"], + explicitVia: ["voice"], + }), + ]), + ); + const markdown = formatReviewPacket(result); + expect(markdown.split(BUTTON)).toHaveLength(2); + expect(markdown.split(VOICE)).toHaveLength(2); + expect(markdown).toContain("https://example.com/button"); + expect(markdown).toContain("Also selected explicitly"); + expect(markdown).toContain("`component.button` (prose shown above)"); + }); + + it("includes a selected cover with no checks without manufacturing a check or fallback", async () => { + const result = await packet(["brand"]); + expect(result.explicitNodes?.[0]).toMatchObject({ + id: "brand", + prose: "Brand stance.", + }); + expect(result.checks.map((check) => check.id)).toEqual(["voice"]); + expect(formatReviewPacket(result)).not.toContain("# ghost default"); + }); + + it("does not auto-add the cover and preserves default packet shape without selection", async () => { + const result = await packet(); + expect(result).not.toHaveProperty("explicitNodeIds"); + expect(result).not.toHaveProperty("explicitNodes"); + expect(result.checks.map((check) => check.id)).toEqual(["voice"]); + expect(result.checks[0]).not.toHaveProperty("explicitVia"); + expect(formatReviewPacket(result)).not.toContain("Brand stance."); + expect(formatReviewPacket(result)).not.toContain("Explicit guidance"); + }); + + it("preserves a complete long explicit node with no referencing checks", async () => { + const body = Array.from( + { length: 4000 }, + (_, index) => `Decision ${index}: 日本語 🧭.`, + ).join("\n"); + await writeFile( + join(packageDir, "long.md"), + `---\nfor: Long guidance.\n---\n\n${body}\n`, + ); + const result = await packet(["long"]); + expect(result.explicitNodes?.[0].prose).toBe(body); + expect(formatReviewPacket(result)).toContain(body); + expect(result.checks.map((check) => check.id)).toEqual(["voice"]); + }); + + it("accepts a nested ID with external-only materials without inventing local matches", async () => { + await mkdir(join(packageDir, "email")); + await writeFile( + join(packageDir, "email", "receipt.md"), + "---\nfor: Transactional email.\nmaterials:\n - https://example.com/receipt\n---\n\nKeep the receipt factual.\n", + ); + await writeFile( + join(packageDir, "checks", "receipt.md"), + "---\nname: Receipt\ndescription: Review the receipt.\nseverity: medium\nreferences:\n - email/receipt\n---\n\nCheck the receipt.\n", + ); + const result = await packet(["email/receipt"]); + expect(result.explicitNodeIds).toEqual(["email/receipt"]); + expect(result.materialNodes).toEqual([]); + expect(result.explicitNodes?.[0]).toMatchObject({ + id: "email/receipt", + materials: ["https://example.com/receipt"], + prose: "Keep the receipt factual.", + }); + expect(result.checks.find((check) => check.id === "receipt")).toMatchObject( + { offered: "always", explicitVia: ["email/receipt"] }, + ); + expect(formatReviewPacket(result)).toContain("https://example.com/receipt"); + }); + + it("rejects unknown explicit IDs for programmatic packet callers too", async () => { + await expect(packet(["voice", "unknown"])).rejects.toThrow("unknown"); + }); +}); diff --git a/packages/ghost/test/review-node-cli.test.ts b/packages/ghost/test/review-node-cli.test.ts new file mode 100644 index 00000000..6acbfca6 --- /dev/null +++ b/packages/ghost/test/review-node-cli.test.ts @@ -0,0 +1,221 @@ +import { spawnSync } from "node:child_process"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { runCli } from "./cli-test-utils.js"; + +const bin = fileURLToPath(new URL("../dist/bin.js", import.meta.url)); +const diff = [ + "diff --git a/brand/logo.svg b/brand/logo.svg", + "--- a/brand/logo.svg", + "+++ b/brand/logo.svg", + "@@ -1 +1 @@", + "-old", + "+new", + "diff --git a/new.html b/new.html", + "new file mode 100644", + "--- /dev/null", + "+++ b/new.html", + "@@ -0,0 +1 @@", + "+
New page
", +].join("\n"); +describe("review explicit guidance CLI", () => { + let dir: string; + let packageDir: string; + const review = (...args: string[]) => + runCli(["review", "--diff=-", "--format", "json", ...args], dir, { + stdin: diff, + }); + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "ghost-review-node-")); + packageDir = join(dir, ".ghost"); + await mkdir(join(packageDir, "checks"), { recursive: true }); + await mkdir(join(dir, "brand")); + const files: Record = { + "manifest.yml": + "schema: ghost.package/v1\nid: review-test\ncover: voice\n", + "glossary.md": "---\nkinds:\n - name: asset\n---\n", + "voice.md": "---\nfor: All writing.\n---\n\nUse concrete words.\n", + }; + for (const name of ["logo", "layout"]) { + files[`asset.${name}.md`] = + `---\nfor: ${name} work.\nmaterials:\n - brand/${name}.svg\n---\n\nPreserve ${name} guidance.\n`; + await writeFile(join(dir, "brand", `${name}.svg`), ""); + } + for (const [name, reference] of Object.entries({ + logo: "asset.logo", + layout: "asset.layout", + words: "voice", + })) { + files[`checks/${name}.md`] = + `---\nname: ${name}\ndescription: Review ${name}.\nseverity: medium\nreferences:\n - ${reference}\n---\n\nCheck ${name}.\n`; + } + await Promise.all( + Object.entries(files).map(([path, text]) => + writeFile(join(packageDir, path), text), + ), + ); + }); + afterEach(async () => { + vi.restoreAllMocks(); + await rm(dir, { recursive: true, force: true }); + }); + + it("adds untouched material and prose guidance, deduplicating repeated flags", async () => { + const result = await review( + "--node", + "asset.layout", + "--node=voice", + "--node", + "asset.layout", + ); + expect(result.code).toBe(0); + const packet = JSON.parse(result.stdout); + expect(packet.explicitNodeIds).toEqual(["asset.layout", "voice"]); + expect(packet.explicitNodes.map((node: { id: string }) => node.id)).toEqual( + ["asset.layout", "voice"], + ); + expect(packet.explicitNodes[0].prose).toContain( + "Preserve layout guidance.", + ); + expect(packet.materialNodes.map((node: { id: string }) => node.id)).toEqual( + ["asset.logo"], + ); + expect(packet.checks).toEqual( + expect.arrayContaining([ + expect.objectContaining({ id: "logo", offered: "matched" }), + expect.objectContaining({ + id: "layout", + offered: "explicit", + explicitVia: ["asset.layout"], + }), + expect.objectContaining({ id: "words", offered: "always" }), + ]), + ); + expect(packet.gaps).toContainEqual( + expect.objectContaining({ + kind: "unmatched-file", + files: ["new.html"], + }), + ); + }); + + it("keeps no-flag selection and packet shape unchanged", async () => { + const result = await review(); + expect(result.code).toBe(0); + const packet = JSON.parse(result.stdout); + expect(packet).not.toHaveProperty("explicitNodeIds"); + expect(packet).not.toHaveProperty("explicitNodes"); + expect( + packet.checks.map((check: { id: string }) => check.id).sort(), + ).toEqual(["logo", "words"]); + for (const check of packet.checks) + expect(check).not.toHaveProperty("explicitVia"); + }); + + it.each([ + ["missing"], + ["asset.layout", "missing", "also-missing"], + ["asset.layout,voice"], + ["voice#heading"], + ])("rejects exact unknown IDs before reading a diff file: %j", async (...ids) => { + const result = await runCli( + [ + "review", + "--package", + packageDir, + "--diff", + "absent.diff", + ...ids.flatMap((id) => ["--node", id]), + ], + dir, + ); + expect(result.code).toBe(2); + expect(result.stdout).toBe(""); + for (const id of ids.filter((id) => id !== "asset.layout")) { + expect(result.stderr).toContain(id); + } + expect(result.stderr).toContain("ghost gather"); + expect(result.stderr).toContain("same --package"); + expect(result.stderr).not.toContain("ENOENT"); + }); + + it("rejects invalid IDs before stdin or git access", async () => { + const stdinRead = vi.spyOn(process.stdin, "setEncoding"); + for (const source of [["--diff=-"], ["--base", "missing-ref"]]) { + const result = await runCli( + ["review", "--node", "missing", ...source], + dir, + ); + expect(result.code).toBe(2); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("missing"); + expect(result.stderr).not.toMatch(/fatal:|Command failed/); + } + expect(stdinRead).not.toHaveBeenCalled(); + }); + + it.each([ + ["--node", " "], + ["--node", "voice", "--node"], + ])("rejects empty and bare repeated values before diff access: %j", async (...flags) => { + const result = await runCli( + ["review", "--diff", "absent.diff", ...flags], + dir, + ); + expect(result.code).toBe(2); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("--node requires"); + expect(result.stderr).not.toContain("ENOENT"); + }); + + it.each([ + "--node", + "--node=", + ])("reports missing values through the executable parser: %s", (flag) => { + const result = spawnSync(process.execPath, [bin, "review", flag], { + cwd: dir, + encoding: "utf8", + input: "", + timeout: 3000, + }); + expect(result.status).toBe(2); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("value is missing"); + expect(result.stderr).toContain("--node "); + }); + + it("retains the missing-checks guard but accepts an empty checks directory", async () => { + await rm(join(packageDir, "checks"), { recursive: true }); + const missing = await review("--node", "voice"); + expect(missing.code).toBe(2); + expect(missing.stdout).toBe(""); + expect(missing.stderr).toContain("ghost checks init"); + await mkdir(join(packageDir, "checks")); + const empty = await review("--node", "voice"); + expect(empty.code).toBe(0); + expect(JSON.parse(empty.stdout)).toMatchObject({ + explicitNodeIds: ["voice"], + checks: [], + }); + }); + + it("exposes additive repeatable selection in help and the manifest", async () => { + const help = await runCli(["review", "--help"], dir, { allowNoExit: true }); + expect(help.stdout).toContain("--node "); + expect(help.stdout).toMatch(/repeat.*does not filter/); + const manifest = await runCli(["manifest", "--format", "json"], dir); + const command = JSON.parse(manifest.stdout).data.commands.find( + (entry: { name: string }) => entry.name === "review", + ); + expect(command.options).toContainEqual( + expect.objectContaining({ + rawName: "--node ", + name: "node", + takesValue: true, + }), + ); + }); +}); diff --git a/packages/ghost/test/review-selection.test.ts b/packages/ghost/test/review-selection.test.ts new file mode 100644 index 00000000..72a6aa0c --- /dev/null +++ b/packages/ghost/test/review-selection.test.ts @@ -0,0 +1,350 @@ +import { resolve } from "node:path"; +import { describe, expect, it, vi } from "vitest"; +import { + type GhostCatalog, + type GhostCatalogNode, + UsageError, +} from "#ghost-core"; +import { + resolveReview, + validateExplicitReviewNodes, +} from "../src/review/resolve.js"; +import type { LoadedCheck } from "../src/scan/check-files.js"; + +const transport = { + repoRoot: resolve("/review-fixture"), + packageDir: resolve("/review-fixture/packages/brand/.ghost"), +}; + +function node(id: string, materials?: string[]): GhostCatalogNode { + return { + id, + slug: id, + materials, + concrete: Boolean(materials?.length), + hasFencedExample: false, + hasSkeleton: false, + body: "# Guidance\n\n## Details\n\nKeep the next action clear.", + }; +} + +function catalog(...nodes: GhostCatalogNode[]): GhostCatalog { + return { nodes: new Map(nodes.map((entry) => [entry.id, entry])) }; +} + +function check(id: string, references: string[]): LoadedCheck { + return { + id, + references, + usesDeprecatedSource: false, + doc: { + frontmatter: { + name: id, + description: "Review the referenced guidance.", + severity: "medium", + references, + }, + body: "Confirm the next action is clear.", + }, + }; +} + +function checks(...entries: LoadedCheck[]): Map { + return new Map(entries.map((entry) => [entry.id, entry])); +} + +function diff(...paths: string[]): string { + return paths + .map((path) => + [ + `diff --git a/${path} b/${path}`, + `--- a/${path}`, + `+++ b/${path}`, + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"), + ) + .join("\n"); +} + +const guidance = catalog( + node("brand"), + node("voice"), + node("voice.md"), + node("marketing/email"), + node("asset.button", ["src/button.ts"]), + node("asset.other", ["src/other.ts"]), + node("asset.unchecked", ["src/unchecked.ts"]), +); + +describe("validateExplicitReviewNodes", () => { + it("defaults to no explicit nodes", () => { + expect(validateExplicitReviewNodes(guidance)).toEqual([]); + }); + + it("deduplicates stably without changing the input or normalizing identities", () => { + const ids = Object.freeze([ + "marketing/email", + "brand", + "voice.md", + "voice", + "brand", + "marketing/email", + ]); + expect(validateExplicitReviewNodes(guidance, ids)).toEqual([ + "marketing/email", + "brand", + "voice.md", + "voice", + ]); + expect(ids).toHaveLength(6); + }); + + it("reports all invalid and unknown IDs with a discovery fix", () => { + const invalid = [ + "missing", + "brand.md", + "*.md", + "brand > Details", + "../voice", + ".ghost/voice.md", + " marketing/email", + "marketing//email", + "Brand", + "", + ]; + let caught: unknown; + try { + validateExplicitReviewNodes(guidance, ["brand", ...invalid, "brand"]); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(UsageError); + const error = caught as UsageError; + expect(error.exitCode).toBe(2); + for (const id of invalid) + expect(error.message).toContain(JSON.stringify(id)); + expect(error.message).toContain("ghost gather --format json"); + expect(error.message).toContain("same --package"); + }); + + it("deduplicates repeated invalid IDs in the diagnostic", () => { + expect(() => + validateExplicitReviewNodes(guidance, ["missing", "missing", "other"]), + ).toThrow('Invalid or unknown review node IDs: "missing", "other".'); + }); +}); + +describe("resolveReview explicit selection", () => { + it("preserves default matching, always checks, via values, and gap wording", () => { + const loaded = checks( + check("material", ["asset.button > Details", "voice"]), + check("always", ["brand", "marketing/email > Details"]), + check("untouched", ["asset.other"]), + ); + const patch = diff("src/button.ts", "src/unchecked.ts", "src/new.ts"); + const result = resolveReview(guidance, loaded, patch, transport); + expect(result).toEqual( + resolveReview(guidance, loaded, patch, transport, []), + ); + expect(result.explicitNodeIds).toEqual([]); + expect(result.offeredChecks).toEqual([ + { + id: "material", + severity: "medium", + offered: "matched", + via: ["asset.button > Details"], + }, + { + id: "always", + severity: "medium", + offered: "always", + via: ["brand", "marketing/email > Details"], + }, + ]); + expect(result.materialNodes).toEqual([ + { + id: "asset.button", + files: ["src/button.ts"], + locators: ["src/button.ts"], + }, + { + id: "asset.unchecked", + files: ["src/unchecked.ts"], + locators: ["src/unchecked.ts"], + }, + ]); + expect(result.gaps).toEqual([ + { + kind: "unmatched-file", + detail: + "changed files match no node `materials` locators — no ghost package guidance claims them", + files: ["src/new.ts"], + }, + { + kind: "unchecked-material", + detail: + "touched material-backed nodes have no check referencing them — review coverage is missing", + nodes: ["asset.unchecked"], + }, + ]); + }); + + it("adds mixed-reference checks via exact explicit refs, including anchors", () => { + const loaded = checks( + check("mixed", ["asset.other", "marketing/email > Details", "brand"]), + check("unrelated", ["asset.other"]), + ); + const result = resolveReview( + guidance, + loaded, + diff("src/new.ts"), + transport, + ["brand", "marketing/email", "brand"], + ); + expect(result.explicitNodeIds).toEqual(["brand", "marketing/email"]); + expect(result.offeredChecks).toEqual([ + { + id: "mixed", + severity: "medium", + offered: "explicit", + via: ["marketing/email > Details", "brand"], + explicitVia: ["marketing/email > Details", "brand"], + }, + ]); + expect(result.materialNodes).toEqual([]); + expect(result.gaps).toEqual([ + { + kind: "unmatched-file", + detail: "changed files have no local material locator matches", + files: ["src/new.ts"], + }, + ]); + }); + + it("offers each check once in input order with matched then always precedence", () => { + const loaded = checks( + check("z-explicit", ["asset.other", "brand"]), + check("a-matched", ["brand", "asset.button > Details", "asset.other"]), + check("m-always", ["voice > Details", "brand"]), + ); + const result = resolveReview( + guidance, + loaded, + diff("src/button.ts"), + transport, + ["brand", "asset.button", "brand"], + ); + expect(result.offeredChecks).toEqual([ + { + id: "z-explicit", + severity: "medium", + offered: "explicit", + via: ["brand"], + explicitVia: ["brand"], + }, + { + id: "a-matched", + severity: "medium", + offered: "matched", + via: ["asset.button > Details"], + explicitVia: ["brand", "asset.button > Details"], + }, + { + id: "m-always", + severity: "medium", + offered: "always", + via: ["voice > Details", "brand"], + explicitVia: ["brand"], + }, + ]); + expect(result.gaps).toEqual([]); + }); + + it("selects an untouched material-backed node without inventing a material match", () => { + const result = resolveReview( + guidance, + checks(check("other", ["asset.other > Details"])), + "", + transport, + ["asset.other"], + ); + expect(result.materialNodes).toEqual([]); + expect(result.offeredChecks).toEqual([ + { + id: "other", + severity: "medium", + offered: "explicit", + via: ["asset.other > Details"], + explicitVia: ["asset.other > Details"], + }, + ]); + expect(result.gaps).toEqual([]); + }); + + it("does not synthesize checks or suppress material coverage gaps", () => { + const patch = diff("src/unchecked.ts", "src/new.ts"); + const baseline = resolveReview(guidance, checks(), patch, transport); + const result = resolveReview(guidance, checks(), patch, transport, [ + "brand", + "asset.unchecked", + "asset.other", + ]); + expect(result.offeredChecks).toEqual([]); + expect(result.materialNodes).toEqual(baseline.materialNodes); + expect(result.gaps).toEqual([ + { + ...baseline.gaps[0], + detail: "changed files have no local material locator matches", + }, + baseline.gaps[1], + ]); + }); + + it("does not suppress checks unrelated to the explicit selection", () => { + const loaded = checks( + check("material", ["asset.button"]), + check("always", ["voice"]), + ); + const patch = diff("src/button.ts"); + const baseline = resolveReview(guidance, loaded, patch, transport); + const result = resolveReview(guidance, loaded, patch, transport, ["brand"]); + expect(result.offeredChecks).toEqual(baseline.offeredChecks); + expect(result.materialNodes).toEqual(baseline.materialNodes); + expect(result.gaps).toEqual(baseline.gaps); + }); + + it("rejects a mixed valid/invalid selection before processing any checks", () => { + const loaded = checks(check("always", ["brand"])); + const values = vi.spyOn(loaded, "values"); + expect(() => + resolveReview(guidance, loaded, "", transport, ["brand", "missing", "*"]), + ).toThrow('Invalid or unknown review node IDs: "missing", "*".'); + expect(values).not.toHaveBeenCalled(); + values.mockRestore(); + }); + + it("keeps package-relative material matching when explicit nodes are supplied", () => { + const local = catalog( + node("asset.logo", ["materials/logo.svg"]), + node("brand"), + ); + const result = resolveReview( + local, + checks(check("logo", ["asset.logo"])), + diff("packages/brand/.ghost/materials/logo.svg"), + transport, + ["brand"], + ); + expect(result.materialNodes).toEqual([ + { + id: "asset.logo", + files: ["packages/brand/.ghost/materials/logo.svg"], + locators: ["materials/logo.svg"], + }, + ]); + expect(result.offeredChecks[0].offered).toBe("matched"); + expect(result.gaps).toEqual([]); + }); +});