Compare commits

..
Author SHA1 Message Date
Philippe Loulidi 8cc5deff1c ref-check: see Terraform citations, resolve wildcards as globs, probe scope first
Re-review findings, each reproduced before fixing:
- .tf/.tfvars/.hcl were missing from the citation allowlist, so `backend.tf` or
  `*/.terraform.lock.hcl` in a prompt was never checked — 18 unchecked citations
  in the infra setup that motivated this feature.
- A wildcard token now resolves as a glob, scope-rebased when that is what
  matches; the suggested `glob:` ref for `pgbouncer/*/patch.yaml` used to be a
  ref that itself failed the check.
- namedPath probes the scope before the root: a scoped prompt citing `alerts/`
  means its own tree even when the root has one too.
- USAGE documents the `target` field --json now emits.
2026-08-03 16:00:33 -06:00
Philippe Loulidi 07762fe6d4 ref-check: strip anchor, line and glob prefix before the ambient-config test
`AGENTS.md#conventions` and `glob:**/.env.local` name scrubbed files too. Found by
the review on this PR.
2026-08-03 15:49:20 -06:00
Philippe Loulidi a0bd28480c ref-check: don't report scrubbed ambient config as a stale ref
The read root is scrubbed of AGENTS.md, CLAUDE.md, .env and friends, so a ref to
one is missing there by design. The template's own shared.md cites AGENTS.md, so
the review note cried wolf on every PR — including this repo's, which is how it
was found. The standalone gate is unchanged: there the tree is a real checkout.
2026-08-03 15:43:18 -06:00
Philippe Loulidi b954180d8b ref-check: cover scope-relative citations that carry an extension
A scoped prompt writes `pgbouncer/base/pdb.yaml` for what is really
`infrastructure/pgbouncer/base/pdb.yaml`. Only extensionless tokens were resolved
before, so the precise ref did not silence the citation and the only thing that
did was `glob:**/pdb.yaml` — which resolves while pointing at every same-named
file in the repo. Six agents pinning a real setup all reached for that glob.

Suggestions now name the resolved path; a token with its own wildcard keeps the
glob form.
2026-08-03 15:36:36 -06:00
Philippe Loulidi 4349be0b24 ref-check: make the suggested ref actually silence its citation (#51)
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:29:59 -06:00
4 changed files with 141 additions and 18 deletions

No files matched your search

+5 -2
View File
@@ -91,8 +91,11 @@ 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 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.
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.
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,6 +69,8 @@ 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);
}
@@ -248,6 +250,67 @@ 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",
@@ -409,6 +472,27 @@ 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}]} on stdout.
--json Emit {ok, problems:[{file, line, kind, problem, target?}]} on stdout.
`;
const KIND_LABEL: Record<RefProblem["kind"], string> = {
+51 -15
View File
@@ -8,6 +8,7 @@ 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
@@ -46,6 +47,7 @@ const CITATION_EXTENSIONS = new Set([
".css",
".go",
".graphql",
".hcl",
".h",
".hpp",
".html",
@@ -69,6 +71,8 @@ const CITATION_EXTENSIONS = new Set([
".sh",
".sql",
".swift",
".tf",
".tfvars",
".toml",
".ts",
".tsx",
@@ -95,6 +99,8 @@ 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 {
@@ -693,10 +699,11 @@ 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 an extensionless token names, or null if it names nothing. */
/** 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. */
const namedPath = async (token: string, scopeRoot: string): Promise<string | null> => {
for (const candidate of pathishCandidates(token)) {
for (const base of [root, scopeRoot]) {
for (const base of [scopeRoot, root]) {
const absolute = path.resolve(base, candidate);
let resolved = resolvable.get(absolute);
if (resolved === undefined) {
@@ -716,6 +723,19 @@ 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)) {
@@ -745,6 +765,7 @@ 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;
}
@@ -762,18 +783,15 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
}
for (const { line, token } of findProseCitations(text)) {
// 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);
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.
if (!isCodeCitation(token) && !named) {
continue;
}
// 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.
// Coverage accepts the resolved path too, else the only ref that silences a
// scope-relative citation is an over-broad `glob:**/<basename>`.
const forms = [...citationForms(token), ...(named ? coveringForms(named) : [])];
if (
ignored.has(token) ||
@@ -785,9 +803,12 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise<
continue;
}
const lineCitation = LINE_CITATION_RE.exec(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);
// 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));
problems.push({
file: relative,
line,
@@ -795,6 +816,7 @@ 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,
});
}
}
@@ -821,6 +843,13 @@ 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;
@@ -848,7 +877,14 @@ export async function reviewSetupRefNotes(options: {
return []; // a check that cannot run must never degrade the review
}
const notes: string[] = [];
const broken = report.problems.filter((problem) => problem.kind !== "unannotated-citation");
// 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))),
);
if (broken.length > 0) {
notes.push(
`The reviewer setup cites code that no longer resolves (${broken.length} ref(s)): ` +