fix: make ref-check's suggestions resolve, and cut its false positives

README.md

@@ -220,7 +220,8 @@ A target is a file, a `dir/`, `glob:<pattern>`, `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
 `<!-- @ref-ignore knex.raw() -->`. 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).
 

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:<agent>`, `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:<agent>`, `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:**/<token>` 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

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":
+        `<!-- ${REF} glob:**/start/doctor/XcodePrerequisite.ts — the doctor check -->\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":
+        `<!-- ${REF} glob:**/start/doctor/XcodePrerequisite.ts — the doctor check -->\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<string, string> = {}): Record<string, string> =>
+    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(`<!-- ${REF} packages/cli/src/export/embed/exportServer.ts — the server -->`),
+    ),
+  });
+  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(`<!-- ${REF} glob:**/export/embed/exportServer.ts — both servers -->`, twin),
+    ),
+  });
+  expect(severalPinned.problems).toEqual([]);
+});
+
+test("a wildcard directory or ./ glob is taught a glob that matches files", async () => {
+  const files = (pin: string): Record<string, string> =>
+    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(
+        `<!-- ${REF} glob:packages/*/apple/** — the apple dirs -->\n` +
+          `<!-- ${REF} glob:internal/* — the internals -->`,
+      ),
+    ),
+  });
+  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":
+        `<!-- ${REF} packages/ — every package -->\n` +
+        `<!-- ${REF} packages/a/ — one package -->\n` +
+        `<!-- ${REF} package.json — the root manifest -->\n` +
+        `<!-- ${REF} gone/ — not there -->\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": `<!-- ${REF} src/a.ts:3 — pinned -->\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",
+  ]);
+});