Compare commits

..
Author SHA1 Message Date
Philippe Loulidi fbcc822c2a ref-check: make the suggested ref actually silence its citation
A scoped prompt names a directory by basename (`cert-manager`); the ref that pins
it is root-relative (`infrastructure/cert-manager/`). Coverage only compared the
token as written, so adding the exact ref the message suggested left the same
problem reported. Coverage now also accepts the path the token resolved to, in
plain and glob form.
2026-08-03 15:18:40 -06:00
4 changed files with 18 additions and 141 deletions

No files matched your search

+2 -5
View File
@@ -91,11 +91,8 @@ settle. A reviewer of this feature named the case: a Terraform prompt citing
"this is very likely going to drift" [observed] — not one of those has an extension, and
`anthropic/claude-opus-5` is shaped identically while naming no file. Syntax cannot tell
them apart, so an extensionless token counts as a citation exactly when it *names
something real*: probed against the scope's own subtree first, then the repo root (a
scoped prompt citing `alerts/` means its own tree even when the root has one too), brace
lists cut back to their fixed prefix. A token carrying `*` resolves as a glob instead,
scope-rebased when that is what matches, so the suggested `glob:` ref itself resolves.
Model ids resolve to nothing and stay prose.
something real*: probed against the repo root and the scope's own subtree, brace lists
and wildcards cut back to their fixed prefix. Model ids resolve to nothing and stay prose.
The consequence is deliberate: an extensionless path that had already drifted before
adoption is invisible (nothing distinguishes it from prose), while every one that is true
-84
View File
@@ -69,8 +69,6 @@ test("isCodeCitation separates paths from prose tokens", () => {
"src/entities/oauth/",
".github/workflows/**",
"*PrivacyPolicy.ts",
"castai-spot.tf",
"*/.terraform.lock.hcl",
]) {
expect(isCodeCitation(token)).toBe(true);
}
@@ -250,67 +248,6 @@ test("the ref the message suggests silences the citation it was suggested for",
}
});
test("a scope-relative citation WITH an extension is covered by its precise ref", async () => {
const files = (pin: string): Record<string, string> => ({
"infrastructure/pgbouncer/base/pdb.yaml": "kind: PodDisruptionBudget\n",
"server/www/k8s/pdb.yaml": "kind: PodDisruptionBudget\n",
"infrastructure/.expo-code-review/config.jsonc": "{}\n",
"infrastructure/.expo-code-review/agents/k8s.md": `${pin}\n\nSee \`pgbouncer/base/pdb.yaml\`.\n`,
});
const unpinned = await checkConfigRefs({ root: await makeRepo(files("")) });
expect(unpinned.problems).toHaveLength(1);
// the suggestion is the precise path, never a basename glob
expect(unpinned.problems[0]!.problem).toContain(
`${REF} infrastructure/pgbouncer/base/pdb.yaml —`,
);
const pinned = await checkConfigRefs({
root: await makeRepo(
files(`<!-- ${REF} infrastructure/pgbouncer/base/pdb.yaml — the PDB this rule is about -->`),
),
});
expect(pinned.problems).toEqual([]);
});
test("a scope-relative wildcard token gets a glob suggestion that itself resolves", async () => {
const files = (pin: string): Record<string, string> => ({
"infra/pgbouncer/production/patch.yaml": "x\n",
"infra/.expo-code-review/config.jsonc": "{}\n",
"infra/.expo-code-review/agents/a.md": `${pin}\nSee \`pgbouncer/*/patch.yaml\`.\n`,
});
const report = await checkConfigRefs({ root: await makeRepo(files("")) });
expect(report.problems).toHaveLength(1);
expect(report.problems[0]!.problem).toContain(`${REF} glob:infra/pgbouncer/*/patch.yaml`);
// …and that suggestion, once written, clears the citation and resolves
const pinned = await checkConfigRefs({
root: await makeRepo(files(`<!-- ${REF} glob:infra/pgbouncer/*/patch.yaml — the patches -->`)),
});
expect(pinned.problems).toEqual([]);
});
test("scope wins over root when both could satisfy an extensionless citation", async () => {
const root = await makeRepo({
"alerts/root.txt": "x\n",
"infra/alerts/scope.tf": "y\n",
"infra/.expo-code-review/config.jsonc": "{}\n",
"infra/.expo-code-review/agents/a.md": "Watch `alerts/` closely.\n",
});
const report = await checkConfigRefs({ root });
expect(report.problems).toHaveLength(1);
expect(report.problems[0]!.problem).toContain(`${REF} infra/alerts/ —`);
});
test("a token carrying its own wildcard keeps the glob suggestion", async () => {
const root = await makeRepo({
".github/workflows/deploy.yml": "on: push\n",
".expo-code-review/config.jsonc": "{}\n",
".expo-code-review/agents/sec.md": "Changes under `.github/workflows/**` are high risk.\n",
});
const report = await checkConfigRefs({ root });
expect(report.problems[0]!.problem).toContain(`${REF} glob:.github/workflows/**`);
});
test("a scope-relative annotated ref is broken, and names the root-relative fix", async () => {
const root = await makeRepo({
"infrastructure/general-central/module/main.tf": "resource {}\n",
@@ -472,27 +409,6 @@ test("reviewSetupRefNotes advises on broken refs and on cited code the PR change
).toEqual([]);
});
test("a ref to scrubbed ambient config is not review advice (the read root removes it)", async () => {
const root = await makeRepo(
setup({
"src/auth.ts": "export const a = 1;\n",
".expo-code-review/shared.md":
`<!-- ${REF} AGENTS.md — the conventions to judge against -->\n` +
`<!-- ${REF} CLAUDE.md#conventions — same, with an anchor -->\n` +
`<!-- ${REF} glob:**/.env.local — same, as a glob -->\n` +
`<!-- ${REF} src/gone.ts — really moved -->\n`,
}),
);
const setupDir = path.join(root, ".expo-code-review");
const notes = await reviewSetupRefNotes({ root, setupDirs: [setupDir], changedFiles: [] });
expect(notes).toHaveLength(1);
expect(notes[0]).toContain("1 ref(s)");
expect(notes[0]).not.toContain("shared.md:1");
// The standalone gate still reports all of them — there the tree is a real checkout.
expect((await checkConfigRefs({ root })).problems).toHaveLength(4);
});
test("setup problems are labelled by setup-dir position when the config lives outside the tree", async () => {
const code = await makeRepo({ "src/auth.ts": "export const a = 1;\n" });
const trustedBase = await makeRepo(
+1 -1
View File
@@ -25,7 +25,7 @@ Exit 0 = every ref holds. Exit 1 = at least one is broken.
Options:
--root <dir> Repository root to check (default: the current git repo).
--json Emit {ok, problems:[{file, line, kind, problem, target?}]} on stdout.
--json Emit {ok, problems:[{file, line, kind, problem}]} on stdout.
`;
const KIND_LABEL: Record<RefProblem["kind"], string> = {
+15 -51
View File
@@ -8,7 +8,6 @@ import { CONFIG_DIRNAME, stripJsonComments, stripTrailingCommas } from "../confi
import { ROUTING_FILENAME } from "../config/routing.js";
import { git, pathInside } from "./exec.js";
import { matchesIgnore } from "./noise.js";
import { isAmbientRuntimeConfig } from "./scrub.js";
/**
* Built by concatenation so this module's own regexes and doc comments are not
@@ -47,7 +46,6 @@ const CITATION_EXTENSIONS = new Set([
".css",
".go",
".graphql",
".hcl",
".h",
".hpp",
".html",
@@ -71,8 +69,6 @@ const CITATION_EXTENSIONS = new Set([
".sh",
".sql",
".swift",
".tf",
".tfvars",
".toml",
".ts",
".tsx",
@@ -99,8 +95,6 @@ export interface RefProblem {
kind: RefProblemKind;
/** One sentence, imperative where a fix is obvious. */
problem: string;
/** The ref target (or prose token) at fault. Absent for structural problems. */
target?: string;
}
export interface ParsedRef {
@@ -699,11 +693,10 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
// Does an extensionless token name something real? Cached: prompts repeat the same
// paths, and each miss would otherwise cost a stat per occurrence.
const resolvable = new Map<string, string | null>();
/** The root-relative path a token names, or null. Scope first: a scoped prompt
* citing `alerts/` means its own tree even when the root has one too. */
/** The root-relative path an extensionless token names, or null if it names nothing. */
const namedPath = async (token: string, scopeRoot: string): Promise<string | null> => {
for (const candidate of pathishCandidates(token)) {
for (const base of [scopeRoot, root]) {
for (const base of [root, scopeRoot]) {
const absolute = path.resolve(base, candidate);
let resolved = resolvable.get(absolute);
if (resolved === undefined) {
@@ -723,19 +716,6 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
return null;
};
/** The glob a wildcard token names, rebased onto the scope when that is what
* matches, so the suggested `glob:` ref actually resolves. */
const namedGlob = (token: string, scopeRoot: string): string | null => {
const scopeRel = path.relative(root, scopeRoot).split(path.sep).join("/");
const candidates = [...(scopeRel && scopeRel !== "." ? [`${scopeRel}/${token}`] : []), token];
for (const candidate of candidates) {
if (index.files.some((file) => matchesIgnore(file, candidate))) {
return candidate;
}
}
return null;
};
for (const dir of dirs) {
const scopeRoot = path.dirname(dir);
for (const file of await setupFiles(dir)) {
@@ -765,7 +745,6 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
line: ref.line,
kind: problem.startsWith("cites a line number") ? "line-number-ref" : "broken-ref",
problem,
target: ref.target,
});
continue;
}
@@ -783,15 +762,18 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
}
for (const { line, token } of findProseCitations(text)) {
const isWild = token.includes("*");
const named = isWild ? namedGlob(token, scopeRoot) : await namedPath(token, scopeRoot);
// Shape alone makes a citation; otherwise it must name something real, or
// `anthropic/claude-opus-5` would read as a path.
// Extension or wildcard tail ⇒ a citation on shape alone. Otherwise it only
// counts if it names something real: `eas-build-worker/terraform` and
// `general-central/{module,production}` are paths, `anthropic/claude-opus-5`
// is shaped identically and is not.
const named = isCodeCitation(token) ? null : await namedPath(token, scopeRoot);
if (!isCodeCitation(token) && !named) {
continue;
}
// Coverage accepts the resolved path too, else the only ref that silences a
// scope-relative citation is an over-broad `glob:**/<basename>`.
// Coverage must accept the path the token RESOLVED to, not just the token as
// written: a prompt says `cert-manager` and the ref that pins it is
// `infrastructure/cert-manager/`. Without this, the fix the message suggests
// does not silence the citation it was suggested for.
const forms = [...citationForms(token), ...(named ? coveringForms(named) : [])];
if (
ignored.has(token) ||
@@ -803,12 +785,9 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
continue;
}
const lineCitation = LINE_CITATION_RE.exec(token);
// A wildcard token means the family; anything else gets the precise resolved path.
const suggestion = isWild
? named
? `${GLOB_PREFIX}${named}`
: suggestedRef(token)
: (named ?? suggestedRef(token));
// For an extensionless token the suggestion is the path that actually resolved,
// which is also how a scope-relative citation learns its root-relative form.
const suggestion = named ?? suggestedRef(token);
problems.push({
file: relative,
line,
@@ -816,7 +795,6 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
problem: lineCitation
? `\`${token}\` pins a line number; cite the file or a \`#symbol\` instead, as \`${REF_MARK} ${suggestedRef(lineCitation[1]!)}\``
: `\`${token}\` cites code without a ref; add \`${REF_MARK} ${suggestion} — why it matters\` (or \`${IGNORE_MARK} ${token}\` if it is not a path)`,
target: token,
});
}
}
@@ -843,13 +821,6 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
};
}
/** The file a target names, with any `glob:` prefix, `#anchor` or `:line` stripped. */
function citedBasename(target: string): string {
const withoutPrefix = target.startsWith(GLOB_PREFIX) ? target.slice(GLOB_PREFIX.length) : target;
const withoutLine = LINE_CITATION_RE.exec(withoutPrefix)?.[1] ?? withoutPrefix;
return path.basename(splitAnchor(withoutLine)[0]);
}
/** How many examples a review-side note names before saying "and N more". */
const NOTE_EXAMPLES = 5;
@@ -877,14 +848,7 @@ export async function reviewSetupRefNotes(options: {
return []; // a check that cannot run must never degrade the review
}
const notes: string[] = [];
// The read root is scrubbed of ambient runtime config (AGENTS.md, CLAUDE.md, .env…),
// so a ref to one is missing by design here, not stale. The template itself cites
// AGENTS.md, which would make this note cry wolf on every PR.
const broken = report.problems.filter(
(problem) =>
problem.kind !== "unannotated-citation" &&
!(problem.target && isAmbientRuntimeConfig(citedBasename(problem.target))),
);
const broken = report.problems.filter((problem) => problem.kind !== "unannotated-citation");
if (broken.length > 0) {
notes.push(
`The reviewer setup cites code that no longer resolves (${broken.length} ref(s)): ` +