diff --git a/README.md b/README.md index 0f468e2..dad1c12 100644 --- a/README.md +++ b/README.md @@ -220,7 +220,8 @@ A target is a file, a `dir/`, `glob:`, `file#symbol`, or `doc.md#headin never a line number (line numbers rot without any signal). The check is strict on purpose: any backticked token in `.expo-agents/code-review/` that names something that exists in the repo **must** be a ref, because stale citations are exactly the ones -nobody annotated. Mark the rare false positive once with +nobody annotated. A trailing-slash name like `ios/` that exists only inside packages +reads as "each package's `ios/`" and is left alone. Mark the rare false positive once with ``. Refs are always repo-root-relative, including in a scope's own setup dir, and config declarations (`enforceAgents` ids, scope directories and globs) are checked too. @@ -230,7 +231,8 @@ Two run points: - `ecr ref-check` exits 1 on any problem. Run it in CI or a pre-commit hook. - `ecr review` / `ecr ci` run it too but never fail a PR's checks with it — broken refs (and cited code *this PR* changes) surface as a **Review setup** note in the - comment. + comment. A ref to a top-level directory (`packages/`) is checked but too broad to + drive the "this PR changes cited code" note. Full detail: [LLP 0012](./llp/0012-config-ref-integrity.explainer.md). diff --git a/llp/0012-config-ref-integrity.explainer.md b/llp/0012-config-ref-integrity.explainer.md index dd0ef5e..7cd94ca 100644 --- a/llp/0012-config-ref-integrity.explainer.md +++ b/llp/0012-config-ref-integrity.explainer.md @@ -82,8 +82,11 @@ config keys, severities, commands and model ids. It comes in two tiers. **Shape alone** (`isCodeCitation`): an allowlisted source extension, a trailing slash, or a wildcard tail inside a path — and never whitespace, `:`, `<`, `(` or quotes. So `session.ts`, `src/entities/oauth/` and `.github/workflows/**` are citations, while -`ecr ci`, `label:`, `knex.raw()`, `critical` and a bare `.ts` are prose. Fenced -code blocks are skipped whole: they are examples, not claims about the tree. +`ecr ci`, `label:`, `knex.raw()`, `critical` and a bare `.ts` are prose. The +allowlist carries native build files (`.mm`, `.gradle`, `.xml`, `.plist`, `.podspec`, …), +which mobile prompts cite constantly; `.env` stays off it, because `process.env` is the +commoner token. Fenced code blocks are skipped whole: they are examples, not claims about +the tree. **Shape plus the filesystem** (`pathishCandidates`), for the citations shape cannot settle. A reviewer of this feature named the case: a Terraform prompt citing @@ -97,6 +100,29 @@ lists cut back to their fixed prefix. A token carrying `*` resolves as a glob in scope-rebased when that is what matches, so the suggested `glob:` ref itself resolves. Model ids resolve to nothing and stay prose. +A trailing slash is shape, but not enough on its own. Prompts for a repo of many +packages say `ios/`, `src/` or `android/src/` meaning *each package's* directory of that +name, and no single ref can pin that: `ios/` does not exist at the root, and +`glob:**/ios/**` matches the whole tree, so it would verify nothing. So a trailing-slash +token that resolves at neither the scope root nor the repo root is a citation only when +no directory of that name exists anywhere in the repo; if one does, it is +package-relative prose. Swept against expo/expo this removed 28 of 149 problems, every +one of them a package-relative name whose suggested fix could never resolve [observed]. +The tradeoff is the extensionless one again: a misspelled `iso/` or a deleted `oldpkg/` +still fails, but a stale `foo/bar/` whose name survives somewhere else +in the tree goes quiet. + +Every suggested fix must itself resolve, or following the message trades one failure +for another. A partial path (`start/doctor/apple/XcodePrerequisite.ts`) is taught the +one repo file it is a suffix of, or `glob:**/` when several are, and a suffix glob +covers the partial path it was written for: coverage reads each `**/` as zero or more +directories, as routing does ([LLP 0004][dialect]), while resolution keeps the shared +dialect unchanged, so `./ref-check` needs no change. A wildcard directory +(`packages/*/apple/`) is taught `glob:packages/*/apple/**`, since a glob matches files +only, and a leading `./` is dropped, since repo paths never carry one. A citation that matches nothing at +all gets no suggestion: the message says it does not resolve and may be stale, which is +usually the real finding. + 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 today gets pinned and will be caught the moment it moves. This mechanism prevents future @@ -136,8 +162,19 @@ The review path also runs it, and never fails a PR's checks with it — that inv holds ([LLP 0002][pipeline]). Instead the review *advises*: broken refs in the setup become a note the reviewer comment carries, and `citedPathsTouchedBy` adds the drift signal a static check cannot give — "this PR changes code your reviewer prompts cite, so -the guidance may need updating" — even when the ref still resolves. In CI the config -comes from the trusted base ref while refs resolve against the PR head tree, so a PR that +the guidance may need updating" — even when the ref still resolves. Only refs that fail +to resolve count toward the first note; a line-number citation is refused by the command +but usually names a file that exists, so it is not reported as unresolved. The drift +signal matches by prefix, so a ref to a top-level directory (`packages/`, `docs/`) is +still checked but does not feed it. With every suggested ref followed, plus `packages/`, +`docs/` and `apps/`, the note fired on 282 of expo/expo's last 300 commits; without the +top-level directories, 196 [observed]. A note that fires on nearly every PR is noise. +The cut stops at one segment because two (`packages/expo-ui/`) usually name one unit +someone owns, where a PR touching it is the signal working; the remaining volume comes +mostly from such package directories, and from wide two-segment ones like +`docs/pages/`, which an author should narrow by hand. + +In CI the config comes from the trusted base ref while refs resolve against the PR head tree, so a PR that moves cited code is reported against the config that will actually review it. ## Two Checkers, One Grammar @@ -172,6 +209,7 @@ the glob dialect must land in `./ref-check` and `src/core/config-refs.ts` togeth same annotation will pass one checker and fail the other. [verify]: 0007-cli-commands-and-ci.explainer.md#verify-config-the-config-guard +[dialect]: 0004-diff-noise-and-prompts.explainer.md#the-mini-glob-dialect [verify5]: 0005-verification-fingerprints-rendering.explainer.md#verifier-confinement-and-fail-open [routing]: 0006-config-schema-loading-routing.explainer.md#routing-manifest [pipeline]: 0002-review-engine-pipeline.explainer.md#pipeline-stages diff --git a/src/__tests__/config-refs.test.ts b/src/__tests__/config-refs.test.ts index 2d31793..8af861f 100644 --- a/src/__tests__/config-refs.test.ts +++ b/src/__tests__/config-refs.test.ts @@ -554,3 +554,177 @@ test("routing: a glob counts as live under the same matcher scope assignment use const report = await checkConfigRefs({ root }); expect(report.problems.filter((p) => p.problem.includes("matches no file"))).toEqual([]); }); + +// --------------------------------------------------------------------------- +// precision: native files, suggestions that resolve, package-relative dirs +// --------------------------------------------------------------------------- + +test("native build files are citations by shape; process.env is not", () => { + for (const token of [ + "RCTFontUtils.mm", + "ios/EXFoo.m", + "android/build.gradle", + "build.gradle.kts", + "android/src/main/AndroidManifest.xml", + "Info.plist", + "ExpoModulesCore.podspec", + "ios/App.xcodeproj/project.pbxproj", + "Debug.xcconfig", + "App.entitlements", + "gradle.properties", + "toolchain.cmake", + "proguard-rules.pro", + "docs/pages/index.mdx", + ]) { + expect(isCodeCitation(token)).toBe(true); + } + expect(isCodeCitation("process.env")).toBe(false); +}); + +test("a suffix glob ref covers the partial path it was written for", async () => { + const root = await makeRepo( + setup({ + "packages/cli/src/start/doctor/XcodePrerequisite.ts": "export {}\n", + ".expo-code-review/agents/security.md": + `\n` + + "See `start/doctor/XcodePrerequisite.ts`.\n", + }), + ); + expect((await checkConfigRefs({ root })).problems).toEqual([]); + + // Once the file is gone, the glob ref is the one problem: the prose it covers is not + // reported a second time (no resolved file stands in for the token here). + const gone = await makeRepo( + setup({ + "packages/cli/src/index.ts": "export {}\n", + ".expo-code-review/agents/security.md": + `\n` + + "See `start/doctor/XcodePrerequisite.ts`.\n", + }), + ); + expect((await checkConfigRefs({ root: gone })).problems.map((problem) => problem.kind)).toEqual([ + "broken-ref", + ]); +}); + +test("a partial path is taught the one file it names, or a suffix glob when several", async () => { + const files = (pin: string, extra: Record = {}): Record => + setup({ + "packages/cli/src/export/embed/exportServer.ts": "export {}\n", + ...extra, + ".expo-code-review/agents/security.md": `${pin}\nSee \`export/embed/exportServer.ts\`.\n`, + }); + + const unique = await checkConfigRefs({ root: await makeRepo(files("")) }); + expect(unique.problems).toHaveLength(1); + expect(unique.problems[0]!.problem).toContain( + `add \`${REF} packages/cli/src/export/embed/exportServer.ts —`, + ); + const uniquePinned = await checkConfigRefs({ + root: await makeRepo( + files(``), + ), + }); + expect(uniquePinned.problems).toEqual([]); + + const twin = { "packages/other/export/embed/exportServer.ts": "export {}\n" }; + const several = await checkConfigRefs({ root: await makeRepo(files("", twin)) }); + expect(several.problems[0]!.problem).toContain( + `add \`${REF} glob:**/export/embed/exportServer.ts —`, + ); + const severalPinned = await checkConfigRefs({ + root: await makeRepo( + files(``, twin), + ), + }); + expect(severalPinned.problems).toEqual([]); +}); + +test("a wildcard directory or ./ glob is taught a glob that matches files", async () => { + const files = (pin: string): Record => + setup({ + "packages/a/apple/A.swift": "struct A {}\n", + "internal/x.ts": "export {}\n", + ".expo-code-review/agents/ios.md": `${pin}\nSee \`packages/*/apple/\` and \`./internal/*\`.\n`, + }); + const report = await checkConfigRefs({ root: await makeRepo(files("")) }); + expect(report.problems.map((problem) => problem.problem)).toEqual([ + expect.stringContaining(`add \`${REF} glob:internal/* —`), + expect.stringContaining(`add \`${REF} glob:packages/*/apple/** —`), + ]); + const pinned = await checkConfigRefs({ + root: await makeRepo( + files( + `\n` + + ``, + ), + ), + }); + expect(pinned.problems).toEqual([]); +}); + +test("a citation that matches nothing says so instead of suggesting a broken ref", async () => { + const root = await makeRepo( + setup({ + "src/a.ts": "export {}\n", + ".expo-code-review/agents/api.md": + "See `tools/src/publish-packages/helpers.ts`, `gone.ts` and `tools/gone.ts:12`.\n", + }), + ); + const problems = (await checkConfigRefs({ root })).problems.map((problem) => problem.problem); + expect(problems).toHaveLength(3); + for (const problem of problems) { + expect(problem).toContain("matches nothing in the repo"); + expect(problem).not.toContain(`add \`${REF}`); + expect(problem).not.toContain(`as \`${REF}`); + } +}); + +test("a trailing-slash name that exists somewhere is package-relative prose; one that exists nowhere fails", async () => { + const root = await makeRepo( + setup({ + "packages/a/ios/A.swift": "struct A {}\n", + "packages/b/android/src/B.kt": "class B\n", + ".expo-code-review/agents/native.md": + "Each package's `ios/` and `android/src/` dirs; never `iso/`.\n", + }), + ); + const problems = (await checkConfigRefs({ root })).problems; + expect(problems.map((problem) => problem.target)).toEqual(["iso/"]); + expect(problems[0]!.problem).toContain("matches nothing in the repo"); + expect(problems[0]!.problem).not.toContain("glob:"); +}); + +test("a top-level directory ref must resolve but does not feed the drift note", async () => { + const root = await makeRepo( + setup({ + "package.json": "{}\n", + "packages/a/index.ts": "export {}\n", + ".expo-code-review/agents/security.md": + `\n` + + `\n` + + `\n` + + `\n`, + }), + ); + const report = await checkConfigRefs({ root }); + expect(report.problems.map((problem) => problem.target)).toEqual(["gone/"]); + expect(report.citedPaths).toEqual(["package.json", "packages/a"]); + expect(citedPathsTouchedBy(report, ["packages/b/index.ts"])).toEqual([]); +}); + +test("a line-number citation of a file that exists is not called unresolvable in the review", async () => { + const root = await makeRepo( + setup({ + "src/a.ts": "export {}\n", + ".expo-code-review/agents/security.md": `\nSee \`src/a.ts:42\`.\n`, + }), + ); + const setupDirs = [path.join(root, ".expo-code-review")]; + expect(await reviewSetupRefNotes({ root, setupDirs, changedFiles: [] })).toEqual([]); + // The command still refuses both. + expect((await checkConfigRefs({ root })).problems.map((problem) => problem.kind)).toEqual([ + "line-number-ref", + "line-number-ref", + ]); +}); diff --git a/src/core/config-refs.ts b/src/core/config-refs.ts index 0424560..74d9d95 100644 --- a/src/core/config-refs.ts +++ b/src/core/config-refs.ts @@ -42,16 +42,22 @@ const LINE_CITATION_RE = /^(.+?):(\d+)(?:-\d+)?$/; /** * Extensions that make a backticked token a *code citation* rather than prose. * Deliberately an allowlist: `openai/gpt-5.5` and `label:` must not read as - * paths, while `session.ts` and `.github/workflows/**` must. + * paths, while `session.ts` and `.github/workflows/**` must. Native build files are on + * it (`RCTFontUtils.mm`, `build.gradle`, `AndroidManifest.xml`, `Info.plist`, + * `proguard-rules.pro`) because mobile prompts cite them constantly; `.env` is not, + * since `process.env` is the far more common backticked token. */ const CITATION_EXTENSIONS = new Set([ ".c", ".cc", ".cjs", + ".cmake", ".cpp", ".cs", ".css", + ".entitlements", ".go", + ".gradle", ".graphql", ".hcl", ".h", @@ -63,12 +69,21 @@ const CITATION_EXTENSIONS = new Set([ ".jsonc", ".jsx", ".kt", + ".kts", ".lock", + ".m", ".md", + ".mdx", ".mjs", + ".mm", ".mts", + ".pbxproj", ".php", + ".plist", + ".podspec", ".prisma", + ".pro", + ".properties", ".proto", ".py", ".rb", @@ -84,6 +99,8 @@ const CITATION_EXTENSIONS = new Set([ ".tsx", ".txt", ".vue", + ".xcconfig", + ".xml", ".yaml", ".yml", ".zsh", @@ -454,6 +471,14 @@ function fileLabel(root: string, setupDir: string, file: string): string { return path.join(CONFIG_DIRNAME, path.relative(setupDir, file)); } +/** What prose adds to a path that a repo path never has: `.../`, `./`, a leading `/`. */ +function barePath(token: string): string { + return token + .replace(/^\.\.\.\/?/, "") + .replace(/^(?:\.\/)+/, "") + .replace(/^\/+/, ""); +} + function splitAnchor(target: string): [string, string | undefined] { const hash = target.indexOf("#"); return hash === -1 ? [target, undefined] : [target.slice(0, hash), target.slice(hash + 1)]; @@ -748,6 +773,65 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise< return null; }; + // @ref LLP 0012#unannotated-citations-are-broken-refs — the ref a message suggests must itself resolve + /** + * The ref an unannotated citation should become, chosen so that writing it down + * resolves: the path or glob it names, else the one repo file it is a suffix of, else + * a suffix glob when several are. Null when nothing in the repo matches: the citation + * itself is stale, and any ref to it would be broken too. + */ + const resolvingRef = ( + citation: string, + scopeRoot: string, + named: string | null, + ): string | null => { + const isWild = citation.includes("*"); + if (named) { + return isWild ? `${GLOB_PREFIX}${named}` : named; + } + if (index.files.length === 0) { + return suggestedRef(citation); // no file list (not a git checkout) — uncheckable + } + const bare = barePath(citation); + if (!bare) { + return null; + } + if (isWild) { + // A glob matches files only, so a directory pattern (`packages/*/apple/`) needs a + // `**` tail to match anything; a leading `./` never matches (paths have none). + const pattern = bare.endsWith("/") ? `${bare}**` : bare; + const glob = + namedGlob(pattern, scopeRoot) ?? + (pattern.startsWith("**/") ? null : namedGlob(`**/${pattern}`, scopeRoot)); + return glob ? `${GLOB_PREFIX}${glob}` : null; + } + if (bare.endsWith("/")) { + return null; // a directory that exists nowhere (one that exists somewhere is prose) + } + const matches = index.files.filter((file) => file === bare || file.endsWith(`/${bare}`)); + if (matches.length === 0) { + return null; + } + if (matches.includes(bare)) { + return bare; + } + // A partial path that names exactly one file gets that file. A bare or `...`- + // abbreviated filename keeps its short suffix glob, which is why that form exists. + if (matches.length === 1 && bare.includes("/") && !citation.startsWith("...")) { + return matches[0]!; + } + return `${GLOB_PREFIX}**/${bare}`; + }; + + /** Does a directory with this (partial) path exist anywhere — `**\//**`? */ + const dirExistsAnywhere = (token: string): boolean => { + const bare = barePath(token); + return ( + bare.length > 1 && + index.files.some((file) => file.startsWith(bare) || file.includes(`/${bare}`)) + ); + }; + for (const dir of dirs) { const scopeRoot = path.dirname(dir); for (const file of await setupFiles(dir)) { @@ -788,10 +872,15 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise< continue; } refs.push(ref); - const cited = splitAnchor(ref.target)[0]; - if (cited && !cited.startsWith(GLOB_PREFIX) && !URL_RE.test(cited)) { - citedPaths.add(cited.replace(/\/$/, "")); + const cited = splitAnchor(ref.target)[0].replace(/\/$/, ""); + if (!cited || cited.startsWith(GLOB_PREFIX) || URL_RE.test(cited)) { + continue; + } + // @ref LLP 0012#run-points-command-and-review — a top-level dir is checked, but too broad to signal drift + if (!cited.includes("/") && (await pathKind(path.resolve(root, cited))) === "dir") { + continue; } + citedPaths.add(cited); } for (const { line, token } of findProseCitations(text)) { @@ -802,32 +891,51 @@ export async function checkConfigRefs(options: CheckConfigRefsOptions): Promise< 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:**/`. - const forms = [...citationForms(token), ...(named ? coveringForms(named) : [])]; + const lineCitation = LINE_CITATION_RE.exec(token); + // @ref LLP 0012#unannotated-citations-are-broken-refs — a dir that exists somewhere is a package-relative name, not a citation + if (!named && !isWild && !lineCitation && token.endsWith("/") && dirExistsAnywhere(token)) { + continue; + } + const cited = lineCitation ? lineCitation[1]! : token; + const suggestion = resolvingRef( + cited, + scopeRoot, + lineCitation ? await namedPath(cited, scopeRoot) : named, + ); + // Coverage accepts the resolved path and the suggested ref too, so writing the + // suggestion down silences the citation, and a scope-relative citation is not + // left needing an over-broad `glob:**/`. + const forms = [ + ...citationForms(token), + ...(named ? coveringForms(named) : []), + ...(suggestion ? coveringForms(suggestion) : []), + ]; if ( ignored.has(token) || forms.some((form) => covered.has(form)) || coveringGlobs.some((pattern) => - forms.some((form) => matchesIgnore(form.replace(/^\.\.\.\//, ""), pattern)), + // @ref LLP 0004#the-mini-glob-dialect — coverage reads `**/` as zero or more dirs (the routing variant), so `glob:**/a/B.kt` covers `a/B.kt` + forms.some((form) => matchesPathGlobs([pattern], form.replace(/^\.\.\.\//, ""))), ) ) { 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)); + const ignoreHint = `(or \`${IGNORE_MARK} ${token}\` if it is not a path)`; + let problem: string; + if (lineCitation) { + problem = suggestion + ? `\`${token}\` pins a line number; cite the file or a \`#symbol\` instead, as \`${REF_MARK} ${suggestion}\`` + : `\`${token}\` pins a line number, and \`${cited}\` matches nothing in the repo; update the prompt to cite where that code lives now, by file or \`#symbol\``; + } else { + problem = suggestion + ? `\`${token}\` cites code without a ref; add \`${REF_MARK} ${suggestion} — why it matters\` ${ignoreHint}` + : `\`${token}\` matches nothing in the repo, so no ref to it can resolve; the citation may be stale — update the prompt if that code moved or was deleted ${ignoreHint}`; + } problems.push({ file: relative, line, kind: lineCitation ? "line-number-ref" : "unannotated-citation", - 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)`, + problem, target: token, }); } @@ -892,9 +1000,11 @@ export async function reviewSetupRefNotes(options: { // 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. + // Only refs that fail to resolve: a line-number citation (annotated or prose) points at + // a file that may well exist, and an unannotated citation is a gate failure, not advice. const broken = report.problems.filter( (problem) => - problem.kind !== "unannotated-citation" && + (problem.kind === "broken-ref" || problem.kind === "structural") && !(problem.target && isAmbientRuntimeConfig(citedBasename(problem.target))), ); if (broken.length > 0) {