From d8e3ba185f29fcf2996b3250039694e3bb0f3b6e Mon Sep 17 00:00:00 2001 From: Nahiyan Khan Date: Wed, 9 Sep 2026 00:25:42 -0400 Subject: [PATCH 1/2] Preserve guidance meaning in CLI output --- .changeset/preserve-packet-meaning.md | 5 + README.md | 2 +- packages/ghost/README.md | 2 +- packages/ghost/src/commands/gather-command.ts | 9 +- packages/ghost/src/embed/gather.ts | 9 +- packages/ghost/src/review/review-packet.ts | 22 +++ packages/ghost/src/skill-bundle/SKILL.md | 9 +- .../src/skill-bundle/references/ground.md | 2 +- .../src/skill-bundle/references/schema.md | 11 +- packages/ghost/test/cli.test.ts | 12 +- packages/ghost/test/semantic-delivery.test.ts | 134 ++++++++++++++++++ 11 files changed, 190 insertions(+), 27 deletions(-) create mode 100644 .changeset/preserve-packet-meaning.md create mode 100644 packages/ghost/test/semantic-delivery.test.ts diff --git a/.changeset/preserve-packet-meaning.md b/.changeset/preserve-packet-meaning.md new file mode 100644 index 00000000..fde5b489 --- /dev/null +++ b/.changeset/preserve-packet-meaning.md @@ -0,0 +1,5 @@ +--- +"@design-intelligence/ghost": patch +--- + +Include referenced baseline prose in Markdown review and preserve full parsed kind descriptions in guidance menus. diff --git a/README.md b/README.md index 41f84294..4a424c5c 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ ghost manifest # emit a machine-readable index of commands and flags ``` For a task-specific gather, your agent reads the complete, unfiltered menu and -pulls every node whose stated situation applies. Bare `ghost gather` inspects +pulls every node whose stated situation applies. `ghost gather --format json` inspects the catalog without grounding a task. Because only selected nodes enter the working context, the agent can see the shape of the brand without loading the whole package. diff --git a/packages/ghost/README.md b/packages/ghost/README.md index 54df63f5..45ecf94d 100644 --- a/packages/ghost/README.md +++ b/packages/ghost/README.md @@ -52,7 +52,7 @@ ghost manifest # emit a machine-readable index of commands and flags ``` For a task-specific gather, your agent reads the complete, unfiltered menu and -pulls every node whose stated situation applies. Bare `ghost gather` inspects +pulls every node whose stated situation applies. `ghost gather --format json` inspects the catalog without grounding a task. Run `ghost --help` for the core workflow and `ghost --help` for diff --git a/packages/ghost/src/commands/gather-command.ts b/packages/ghost/src/commands/gather-command.ts index dee9331f..51f2bc1b 100644 --- a/packages/ghost/src/commands/gather-command.ts +++ b/packages/ghost/src/commands/gather-command.ts @@ -99,13 +99,18 @@ function formatMenuMarkdown(menu: GhostGatherResult): string { "", "## Available guidance", "", - "Pull the applicable IDs together with `ghost pull […]`. If no listed guidance applies, run bare `ghost pull` to receive the cover and uncovered-guidance policy. Skip clear non-matches; topic overlap alone is not enough. Do not limit the number.", + "Pull the applicable IDs together with `ghost pull […]`.", + menu.contract.selection.instruction, + menu.contract.ifNoneApply, "", ]; - const groups = groupMenuByKind(menu.nodes, menu.kinds ?? []); + const kinds = menu.kinds ?? []; + const groups = groupMenuByKind(menu.nodes, kinds); for (const group of groups) { lines.push(group.kind ? `### ${group.kind}` : "### Other guidance", ""); + const purpose = kinds.find((kind) => kind.name === group.kind)?.purpose; + if (purpose) lines.push(purpose, ""); for (const entry of group.entries) { lines.push(`- \`${entry.id}\``); if (entry.for?.trim()) { diff --git a/packages/ghost/src/embed/gather.ts b/packages/ghost/src/embed/gather.ts index e48df23d..138f1ffa 100644 --- a/packages/ghost/src/embed/gather.ts +++ b/packages/ghost/src/embed/gather.ts @@ -101,11 +101,8 @@ export function menuCoverage( function menuKinds(snapshot: GhostEmbedSnapshot): GhostMenuKind[] { return (snapshot.glossary?.kinds ?? []).map((kind) => ({ name: kind.name, - // Legend entries are one line each: keep the section's first paragraph - // and collapse internal wrapping. Empty purpose stays explicit so - // declared kind order survives even when the glossary has no prose yet. - purpose: (kind.purpose.split(/\n\s*\n/, 1)[0] ?? "") - .replace(/\s+/g, " ") - .trim(), + // Later paragraphs may carry selection rules. Preserve the parsed purpose, + // including Markdown structure, rather than silently shortening its meaning. + purpose: kind.purpose.trim(), })); } diff --git a/packages/ghost/src/review/review-packet.ts b/packages/ghost/src/review/review-packet.ts index 89c1c6a1..72888bea 100644 --- a/packages/ghost/src/review/review-packet.ts +++ b/packages/ghost/src/review/review-packet.ts @@ -157,6 +157,8 @@ export function formatReviewPacket(packet: ReviewPacket): string { } } + const shownNodes = new Set(packet.materialNodes.map((node) => node.id)); + const shownSections = new Set(); out.push("## Offered checks — weigh which apply"); if (packet.checks.length === 0) { out.push("_No checks were offered for this diff._", ""); @@ -177,6 +179,26 @@ export function formatReviewPacket(packet: ReviewPacket): string { for (const baseline of check.baseline) { out.push(`- ${baseline.ref}`); if (baseline.warning) out.push(` - ⚠ ${baseline.warning}`); + // A missing heading falls back to the whole node, so later sections + // can point back to it just as they can to a matched material node. + const wholeNode = + baseline.heading === undefined || baseline.warning !== undefined; + const sectionKey = `${baseline.nodeId}\0${baseline.heading?.toLowerCase() ?? ""}`; + if ( + shownNodes.has(baseline.nodeId) || + (!wholeNode && shownSections.has(sectionKey)) + ) { + out.push(" - Baseline prose shown above.", ""); + continue; + } + if (baseline.for) out.push(` - Applies when: ${baseline.for}`); + out.push( + "", + ...baseline.body.split(/\r?\n/).map((line) => `> ${line}`), + "", + ); + if (wholeNode) shownNodes.add(baseline.nodeId); + else shownSections.add(sectionKey); } out.push(""); } diff --git a/packages/ghost/src/skill-bundle/SKILL.md b/packages/ghost/src/skill-bundle/SKILL.md index fb4abefe..2b34a66e 100644 --- a/packages/ghost/src/skill-bundle/SKILL.md +++ b/packages/ghost/src/skill-bundle/SKILL.md @@ -75,8 +75,9 @@ ghost stats # summarize local gather/pull events while tuning `gather` does no selection. Its Markdown is an agent-facing instruction surface: the task, then every selectable id and its applicability. Check the full list and pull every id that applies. If none apply, run bare `ghost pull`. -Declared kind headings render in glossary order, undeclared kinds -alphabetically, and uncategorized guidance last. +Declared kinds render in glossary order with their full parsed purposes, +undeclared kinds alphabetically, and uncategorized guidance last. Read the kind +selection rules as well as each item's condition. Markdown omits package diagnostics that do not change the next action. JSON retains the selection contract, coverage, materials, substantial fenced examples, Skeletons, and missing `for` payloads for integrations and audits. @@ -92,8 +93,8 @@ and diagnostic metadata for integrations. Pulls append structured events to tuning. `review` does no grading. It assembles the review packet: touched files, -matched material-backed nodes, offered checks, coverage gaps, and the diff. The -host agent renders findings. +matched material-backed nodes, offered checks with baseline prose, coverage +gaps, and the diff. The host agent renders findings. 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/ground.md b/packages/ghost/src/skill-bundle/references/ground.md index d847c0e2..05489e7a 100644 --- a/packages/ghost/src/skill-bundle/references/ground.md +++ b/packages/ghost/src/skill-bundle/references/ground.md @@ -13,7 +13,7 @@ should be shaped by a ghost package. Run `ghost gather ` with the real task, not a generic label. Read the supplied guidance, then check every item under `Available guidance`. Pull every id whose `Applies when` condition fits the task. Skip clear non-matches; topic -overlap alone is not enough. +overlap alone is not enough. Honor the kind's selection rules too. The cover is not in this menu because every pull includes it automatically. If nothing in the list applies, run bare `ghost pull` for the cover and uncovered- diff --git a/packages/ghost/src/skill-bundle/references/schema.md b/packages/ghost/src/skill-bundle/references/schema.md index 36748b4f..b287c624 100644 --- a/packages/ghost/src/skill-bundle/references/schema.md +++ b/packages/ghost/src/skill-bundle/references/schema.md @@ -41,10 +41,10 @@ Obligation or a replaceable Default), `foundation` (the brand's load-bearing decisions), and `context` (what bends in a named situation). A package may declare any vocabulary; the glossary is the only kind authority. -`ghost gather --format json` includes each kind's first paragraph as its -purpose for operator tooling. Agent-facing Markdown uses kind headings only; -selection comes from each item's `Applies when` condition. Declared kinds render -in frontmatter order even when their purpose is empty; undeclared kinds render +`ghost gather` preserves each kind's full parsed purpose in JSON and shows it +once above that kind's entries in Markdown. Read the kind's selection rules +alongside each item's `Applies when` condition. Declared kinds render in +frontmatter order even when their purpose is empty; undeclared kinds render alphabetically after declared kinds, and uncategorized guidance renders last. ## Nodes @@ -129,5 +129,6 @@ it does not grade them. JSON retains node kinds and transport diagnostics omitted from agent-facing Markdown. - `ghost review` matches touched files to exact local material paths, offers - relevant checks, and emits a review packet for the host agent. + relevant checks, and emits their referenced baseline prose for the host + agent. Repeated baselines point to prose already included in the packet. - `ghost stats` summarizes local gather and pull events. diff --git a/packages/ghost/test/cli.test.ts b/packages/ghost/test/cli.test.ts index 3561964a..a62d2d2b 100644 --- a/packages/ghost/test/cli.test.ts +++ b/packages/ghost/test/cli.test.ts @@ -620,8 +620,8 @@ describe("ghost CLI", () => { expect(markdown.stdout).not.toContain("already in context"); expect(markdown.stdout).not.toContain("concrete support"); expect(markdown.stdout).not.toContain("Selection contract"); - expect(markdown.stdout).not.toContain("selectable node"); - expect(markdown.stdout).not.toContain("`for` payload"); + expect(markdown.stdout).toContain("If no selectable node applies"); + expect(markdown.stdout).toContain("whose `for` payload matches the task"); expect(markdown.stdout).not.toContain("Numbering"); expect(markdown.stdout).not.toContain("materials:"); expect(markdown.stdout).not.toContain("payloads:"); @@ -828,7 +828,7 @@ describe("ghost CLI", () => { ).rejects.toThrow(); }); - it("keeps glossary kind purposes in JSON and only headings in Markdown", async () => { + it("keeps glossary kind purposes in JSON and Markdown", async () => { await runCli(["init"], dir); // JSON carries the glossary's declared kinds with their prose purposes. @@ -841,13 +841,11 @@ describe("ghost CLI", () => { expect(foundation.purpose).toContain("load-bearing decisions"); expect(foundation.purpose).toContain("Pull every foundation chapter"); - // Markdown uses the kind only as navigation; purpose prose stays in JSON. + // Both formats preserve the kind's selection meaning. const markdown = await runCli(["gather", "build", "a", "page"], dir); expect(markdown.stdout).not.toContain("Kinds:"); expect(markdown.stdout).toContain("### foundation"); - expect(markdown.stdout).not.toContain( - "The brand's load-bearing decisions for color", - ); + expect(markdown.stdout).toContain(foundation.purpose); expect(markdown.stdout).not.toContain(menu.contract.noAsk); // A missing glossary degrades to no legend, not an error. diff --git a/packages/ghost/test/semantic-delivery.test.ts b/packages/ghost/test/semantic-delivery.test.ts new file mode 100644 index 00000000..f5ba62c8 --- /dev/null +++ b/packages/ghost/test/semantic-delivery.test.ts @@ -0,0 +1,134 @@ +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"; +import { runCli } from "./cli-test-utils.js"; + +const PURPOSE = + "Rules for writing copy.\n\nSelect only for legal copy.\n\n- Keep the qualification.\n- Do not apply to informal messages."; +const BODY = + "## Wording\n\nState the unique approved phrase.\n\n## Tone\n\nUse the unique restrained tone."; + +describe("agent-facing semantic delivery", () => { + let dir: string; + let packageDir: string; + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "ghost-semantic-")); + packageDir = join(dir, ".ghost"); + await mkdir(join(packageDir, "checks"), { recursive: true }); + await writeFile( + join(packageDir, "manifest.yml"), + "schema: ghost.package/v1\nid: semantic-test\n", + ); + await writeFile( + join(packageDir, "glossary.md"), + `---\nkinds:\n - name: rule\n - name: unused\n---\n\n# rule\n\n${PURPOSE}\n\n# unused\n\nUnused meaning.\n`, + ); + await writeFile( + join(packageDir, "rule.voice.md"), + `---\nfor: Writing legal copy.\n---\n\n${BODY}\n`, + ); + }); + + afterEach(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + async function check(id: string, reference: string): Promise { + await writeFile( + join(packageDir, "checks", `${id}.md`), + `---\nname: ${id}\ndescription: Review wording.\nseverity: high\nreferences:\n - ${reference}\n---\n\nCompare the change with its referenced guidance.\n`, + ); + } + + async function packet(diff = "") { + const loaded = await loadGhostPackage(resolveGhostPackage(packageDir, dir)); + return buildReviewPacket(loaded, diff, { cwd: dir, packageDir }); + } + + it("delivers every paragraph of kind meaning in JSON and Markdown", async () => { + await writeFile( + join(packageDir, "rule.other.md"), + "---\nfor: Writing legal copy.\n---\n\nOther rule.\n", + ); + const json = await runCli( + ["gather", "legal copy", "--format", "json"], + dir, + ); + expect(json.code).toBe(0); + const menu = JSON.parse(json.stdout); + expect( + menu.kinds.find((kind: { name: string }) => kind.name === "rule").purpose, + ).toBe(PURPOSE); + + const markdown = await runCli(["gather", "legal copy"], dir); + expect(markdown.code).toBe(0); + expect(markdown.stdout.split(PURPOSE)).toHaveLength(2); + expect(markdown.stdout).not.toContain("Unused meaning."); + expect(markdown.stdout.indexOf(PURPOSE)).toBeLessThan( + markdown.stdout.indexOf("`rule.other`"), + ); + expect(markdown.stdout).toContain(menu.contract.selection.instruction); + }); + + it("renders the baseline and applicability for a prose-only review check", async () => { + await check("voice", "rule.voice"); + const result = await packet(); + expect(result.materialNodes).toEqual([]); + expect(result.checks[0]?.baseline[0]?.body).toBe(BODY); + const markdown = formatReviewPacket(result); + expect(markdown).toContain("Applies when: Writing legal copy."); + expect(markdown).toContain("> State the unique approved phrase."); + expect(markdown).toContain("> Use the unique restrained tone."); + }); + + it("renders a repeated baseline section once and points back to it", async () => { + await check("first", "rule.voice > Wording"); + await check("second", "rule.voice > wording"); + const markdown = formatReviewPacket(await packet()); + expect(markdown.split("State the unique approved phrase.")).toHaveLength(2); + expect(markdown).not.toContain("Use the unique restrained tone."); + expect(markdown).toContain("prose shown above"); + }); + + it("preserves distinct baseline sections", async () => { + await check("first", "rule.voice > Wording"); + await check("second", "rule.voice > Tone"); + const markdown = formatReviewPacket(await packet()); + expect(markdown).toContain("State the unique approved phrase."); + expect(markdown).toContain("Use the unique restrained tone."); + }); + + it("keeps missing-heading warnings and the whole-body fallback", async () => { + await check("first", "rule.voice > Missing"); + await check("second", "rule.voice > Tone"); + const markdown = formatReviewPacket(await packet()); + expect(markdown).toContain("heading 'Missing' not found"); + expect(markdown.split("State the unique approved phrase.")).toHaveLength(2); + expect(markdown.split("Use the unique restrained tone.")).toHaveLength(2); + expect(markdown).toContain("prose shown above"); + }); + + it("reuses a matched material node's prose for whole-node and section references", async () => { + await writeFile( + join(packageDir, "rule.voice.md"), + `---\nfor: Writing legal copy.\nmaterials:\n - copy.txt\n---\n\n${BODY}\n`, + ); + await check("first", "rule.voice"); + await check("second", "rule.voice > Wording"); + const result = await packet( + "diff --git a/copy.txt b/copy.txt\n--- a/copy.txt\n+++ b/copy.txt\n@@ -1 +1 @@\n-old\n+new\n", + ); + expect(result.materialNodes).toHaveLength(1); + const markdown = formatReviewPacket(result); + expect(markdown.split("State the unique approved phrase.")).toHaveLength(2); + expect(markdown.split("Use the unique restrained tone.")).toHaveLength(2); + expect(markdown.split("prose shown above")).toHaveLength(3); + }); +}); From ebc6e3da6aa2b885ad620ea76e06b083b236e304 Mon Sep 17 00:00:00 2001 From: Nahiyan Khan Date: Wed, 9 Sep 2026 23:37:13 -0400 Subject: [PATCH 2/2] Add explicit guidance selection to review --- .changeset/explicit-review-guidance.md | 5 + packages/ghost/README.md | 12 + .../ghost/src/commands/command-discovery.ts | 2 +- packages/ghost/src/commands/review-command.ts | 26 +- packages/ghost/src/review/resolve.ts | 54 ++- packages/ghost/src/review/review-packet.ts | 88 ++++- packages/ghost/src/skill-bundle/SKILL.md | 4 + .../src/skill-bundle/references/making.md | 9 +- .../src/skill-bundle/references/schema.md | 12 + .../ghost/test/review-explicit-packet.test.ts | 204 ++++++++++ packages/ghost/test/review-node-cli.test.ts | 221 +++++++++++ packages/ghost/test/review-selection.test.ts | 350 ++++++++++++++++++ 12 files changed, 974 insertions(+), 13 deletions(-) create mode 100644 .changeset/explicit-review-guidance.md create mode 100644 packages/ghost/test/review-explicit-packet.test.ts create mode 100644 packages/ghost/test/review-node-cli.test.ts create mode 100644 packages/ghost/test/review-selection.test.ts 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 45ecf94d..ed48fe8b 100644 --- a/packages/ghost/README.md +++ b/packages/ghost/README.md @@ -58,6 +58,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 6b7fd318..bceab62e 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 72888bea..6bac88b1 100644 --- a/packages/ghost/src/review/review-packet.ts +++ b/packages/ghost/src/review/review-packet.ts @@ -29,11 +29,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[]; } @@ -42,6 +49,10 @@ export interface ReviewPacket { packageId: string; 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; @@ -52,6 +63,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( @@ -69,12 +81,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 { @@ -82,6 +109,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 @@ -94,6 +122,9 @@ export async function buildReviewPacket( packageId: ghostPackage.manifest.id, touchedFiles: resolution.touchedFiles.map((file) => file.path), materialNodes, + ...(resolution.explicitNodeIds.length > 0 + ? { explicitNodeIds: resolution.explicitNodeIds, explicitNodes } + : {}), checks, gaps: resolution.gaps, diff: diffText, @@ -151,13 +182,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) { @@ -171,9 +245,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 2b34a66e..b23610bf 100644 --- a/packages/ghost/src/skill-bundle/SKILL.md +++ b/packages/ghost/src/skill-bundle/SKILL.md @@ -95,6 +95,10 @@ tuning. `review` does no grading. It assembles the review packet: touched files, matched material-backed nodes, offered checks with baseline prose, 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 b287c624..8aa33255 100644 --- a/packages/ghost/src/skill-bundle/references/schema.md +++ b/packages/ghost/src/skill-bundle/references/schema.md @@ -131,4 +131,16 @@ it does not grade them. - `ghost review` matches touched files to exact local material paths, offers relevant checks, and emits their 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. 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..b431a3a9 --- /dev/null +++ b/packages/ghost/test/review-explicit-packet.test.ts @@ -0,0 +1,204 @@ +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("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([]); + }); +});