review: add an opt-in documentation reviewer

.changeset/review-documentation-lens.md

@@ -0,0 +1,13 @@
+---
+"review": minor
+---
+
+Add an opt-in `documentation` reviewer: an advisory-only whole-change reviewer that checks the comments and prose docs a diff adds or changes against a documentation policy. It flags a comment that restates the code, one that narrates the change rather than describing the code, one this diff falsified, commented-out code, and (the other direction) a specific unexplained constant, workaround, or ordering constraint the change introduces. The policy lives inline in the reviewer's definition rather than in a fifth required consumer import: the baseline is universal, and repo-specific calibration already has a home in the per-directory `REVIEW.md` contracts. It never reasons about whether a human or a model wrote the text (it cannot tell, and the policy is the same either way), and it does not review the PR title or description. Off everywhere until a repo adds `enable documentation` to its `ROUTING` file.
+
+Its findings render with a new non-blocking label, `suggestion (non-blocking, documentation)`, code-assigned from the lens exactly as the `, best-practice` variants are. The variant is not cosmetic: the autofix workflow selects threads by parsing the Conventional-Comment label off each posted comment, so this label is the only channel by which a documentation-scoped autofix can tell a documentation thread from any other nit. The scripted dispatcher now assigns the lens of a label-shape reviewer by name (`dispatch-contracts.ts`: previously `conventions` for `skill-auditor` and `correctness` for every other one), so a documentation finding carries `lens: documentation` downstream and `labelForFinding` agrees with the label the reviewer emitted. There is deliberately no blocking documentation variant, so `BLOCKING_LABELS` (what the blocking autofix scope acts on) is unchanged. `thumbs-sweep-github.ts` picks the new label up automatically; `verdict.ts` is unaffected, since the verdict counts blocking labels only.
+
+Two supporting changes. The `claim-validator` gains a documentation-claim rule: verify the quoted comment text and the code it is contrasted with, and refute whenever the comment carries information the code does not show, which is this reviewer's characteristic false positive. And the live eval producer now dispatches the opt-in reviewers a case `enable`s (`routerConfig.enabledReviewers`, canonical order, unknown names throw) and maps their label-shape output; before this, no opt-in reviewer had a live arm at all and so could not earn its `enable` line through the eval suite the way the policy says it must. Cases that enable nothing are unaffected.
+
+New eval corpus pair, both live-enabled: `golden-documentation-stale-and-narrated` (a sound behavior change that leaves a falsified comment and a change-narrating comment, and carries a must-not-flag trap whose comment reads like a restatement but records a real constraint) and `clean-documentation-earned-comments` (every added comment earns its line; the run must post nothing).
+
+Eval-harness fix that this PR's own corpus pair needs: an opt-in reviewer a case `enable`s but the arm's `review.md` does not define is now recorded as an absent dimension instead of throwing. The baseline arm of the A/B that graduates any new reviewer is built from the base tip and so cannot define it, and `runArm` does not wrap its produce call, so the throw killed the whole run before any report. The name is already validated against `ENABLEABLE_REVIEWERS`, so tolerating absence cannot mask a typo; every other roster member (the always-on finders, the validator, the reconciler, a routed lens) still hard-errors. The A/B report calls the asymmetry out under `Arm asymmetry`, since the candidate arm's findings on that dimension are gain by construction rather than a measured improvement.

workflows/review/README.md

@@ -268,10 +268,10 @@ re-review scoped
 - `lens=` names the specialist lenses to spawn when the pattern is touched; when
   several rules match a path their lenses are unioned (lenses are additive).
 - `enable` lines turn on the opt-in whole-change reviewers (`holistic`,
-  `completeness`, `test-adequacy`, `first-principles`, `conventions`). Neither
-  lenses nor opt-in reviewers run anywhere by default: a repo opts into each
-  explicitly, and the policy is that a reviewer earns its line here through the
-  eval suite.
+  `completeness`, `test-adequacy`, `first-principles`, `conventions`,
+  `documentation`). Neither lenses nor opt-in reviewers run anywhere by default:
+  a repo opts into each explicitly, and the policy is that a reviewer earns its
+  line here through the eval suite.
 - `tier=` assigns the path a risk tier. When several rules match, the **last
   matching rule in file order wins** (gitignore/CODEOWNERS-style): write the broad
   rule first and its exceptions after it, as with `services/**` and
@@ -294,6 +294,59 @@ and are skipped; routing degrades to fewer lenses, never to a crashed review.
 stays the model-facing prose about file *contents*; team ownership stays in
 `.github/REVIEWERS`, unchanged.
 
+### The `documentation` reviewer (opt-in)
+
+`enable documentation` turns on a reviewer that checks the **comments and prose docs
+the diff adds or changes** against a documentation policy. It is advisory-only and
+opt-in like `conventions`, and it exists because comment quality is an enforcement
+problem rather than a prompt problem: every author, human or agent, is told what a
+good comment looks like, and nothing checks.
+
+The policy is one paragraph with a list under it, and it lives **inline in the
+reviewer's definition** in `review.md` (like the specialist lenses' `Review rules`
+sections), not in a consumer config file. Two reasons: the baseline is universal
+(a comment earns its line by carrying information the code does not), and a fifth
+required `{{#runtime-import}}` would make the review fail at run time in any repo
+that had not yet written the file. Repo-specific calibration rides the per-directory
+`REVIEW.md` contracts that already exist, which is also the answer to "backend and
+frontend want different things here": they can, per directory, without a global
+decision.
+
+Two things it deliberately does not do:
+
+- **It never reasons about who wrote the text.** It cannot tell whether a human or a
+  model wrote a comment, and the policy is the same either way. A finding that reads
+  as an accusation of AI authorship is out of bounds even when the comment is bad.
+- **It does not review the PR title or description.** Those carry no line anchor and
+  no fix path today; `completeness` and `first-principles` already read them.
+
+**The label is load-bearing.** Its findings render as
+`suggestion (non-blocking, documentation)` rather than a plain `suggestion
+(non-blocking)`. That variant is the only channel by which a downstream consumer can
+tell a documentation thread from any other nit: the autofix workflow selects the
+threads it may act on by parsing the Conventional-Comment label off each posted
+comment, so a documentation-scoped autofix is exactly "the threads carrying this
+label". A consequence for consumers: autofix reads labels minted by whichever
+reviewer version the repo has **installed**, so a repo must be on a review release
+carrying this label before a documentation-scoped autofix finds anything.
+
+The eval corpus carries a matched pair
+(`golden-documentation-stale-and-narrated`, `clean-documentation-earned-comments`):
+one change that leaves two real documentation defects, one whose comments all earn
+their line and must draw no comment at all. Both are live-enabled, which required
+teaching the live producer to dispatch a case's `enable`d reviewers — before that,
+no opt-in reviewer had a live arm at all and could not earn its `enable` line the
+way the policy above says it must.
+
+Neither case carries the `smoke` tag, so the per-PR A/B skips them; price this
+reviewer with a targeted `workflow_dispatch` of *Review Eval A/B*
+(`cases=golden-documentation-stale-and-narrated,clean-documentation-earned-comments`,
+`repeats=3`, since precision on the clean case is the stochastic half). On the
+A/B that introduces any new reviewer the **baseline arm cannot define it**: the
+producer records that as an absent dimension rather than failing the run, and the
+report flags it under *Arm asymmetry*, because the candidate arm's findings there
+are pure gain by construction rather than a measured improvement.
+
 ### The `.github/NOTIFIED` file (optional)
 
 If the repo has a Gerald [`.github/NOTIFIED`](https://khanacademy.atlassian.net/wiki/spaces/FRONTEND/pages/598278672/Gerald+Documentation)
@@ -442,6 +495,7 @@ sub-agent models — this table is the human-facing summary:
 | `completeness` | `claude-opus-4-8` | high | Opt-in whole-change reviewer (`enable` in `ROUTING`) |
 | `test-adequacy` | `claude-opus-4-8` | high | Opt-in whole-change reviewer (`enable` in `ROUTING`) |
 | `conventions` | `claude-opus-4-8` | medium | Opt-in advisory targeted check (`enable` in `ROUTING`) |
+| `documentation` | `claude-opus-4-8` | medium | Opt-in advisory targeted check (`enable` in `ROUTING`) |
 | `first-principles` | `claude-fable-5` | high | Opt-in advisory-only; reviews the change's justification |
 | `claim-validator` | `claude-opus-4-8` | xhigh | Adversarial claim validation; stays Opus (the Fable arm did not improve precision) |
 | specialist lenses | `claude-opus-4-8` | high | Opt-in via `lens=` in `ROUTING`; the security & auth lens is xhigh |

workflows/review/eval/corpus/clean/clean-documentation-earned-comments/case.json

@@ -0,0 +1,53 @@
+{
+    "id": "clean-documentation-earned-comments",
+    "tags": ["clean", "documentation", "live"],
+    "category": "clean",
+    "description": "The documentation reviewer's precision guard. Every comment this change adds earns its line by recording something the code cannot show: why ties break on id (cross-service determinism), and where the 0.35 threshold came from. Each one is anchored on an added line and sits next to the code it explains, which is exactly the shape a reviewer reaching for a finding would flag. The run must post nothing.",
+    "changedFiles": [{"path": "src/search/ranking.ts", "status": "modified"}],
+    "routerConfig": {
+        "enabledReviewers": ["documentation"]
+    },
+    "findings": [],
+    "policyConflicts": [],
+    "expected": {
+        "verdict": "APPROVE",
+        "postedCommentCount": 0,
+        "mustNotPost": []
+    },
+    "diff": "diff --git a/src/search/ranking.ts b/src/search/ranking.ts\n--- a/src/search/ranking.ts\n+++ b/src/search/ranking.ts\n@@ -5,6 +5,18 @@\n-/** Order results for display. */\n-export const rankResults = (results: readonly Result[]): Result[] =>\n-    [...results].sort((a, b) => b.score - a.score);\n+/**\n+ * Order results for display.\n+ *\n+ * Ties break on id rather than relying on sort stability: ranking runs on two\n+ * services with different V8 versions, and a differing tie order surfaces as a\n+ * phantom diff in the cached page.\n+ */\n+export const rankResults = (results: readonly Result[]): Result[] =>\n+    [...results].sort(\n+        (a, b) => b.score - a.score || (a.id < b.id ? -1 : a.id > b.id ? 1 : 0),\n+    );\n \n+// 0.35 came out of the 2026-05 relevance sweep: below it, keyword-only matches\n+// pushed curated content off the first page.\n+export const MIN_SCORE = 0.35;\n+\n export const visible = (results: readonly Result[]): Result[] =>\n-    rankResults(results);\n+    rankResults(results).filter((result) => result.score >= MIN_SCORE);\n",
+    "live": {
+        "prContext": {
+            "title": "search: break ranking ties deterministically and drop low-score results",
+            "description": "Two ranking fixes: ties now break on id so the two ranking services agree, and results below the relevance floor stop reaching the page.",
+            "author": "dev-search",
+            "baseBranch": "main"
+        },
+        "mustNotFlagSpecs": [
+            {
+                "key": "doc-trap-tiebreak-docstring",
+                "path": "src/search/ranking.ts",
+                "lineStart": 5,
+                "lineEnd": 11,
+                "lens": "documentation",
+                "mechanism": [
+                    "docstring restates the signature",
+                    "comment repeats what the sort does",
+                    "verbose docstring",
+                    "delete the tie-break comment"
+                ]
+            },
+            {
+                "key": "doc-trap-threshold-rationale",
+                "path": "src/search/ranking.ts",
+                "lineStart": 17,
+                "lineEnd": 19,
+                "lens": "documentation",
+                "mechanism": [
+                    "comment restates the constant",
+                    "0.35 comment is redundant",
+                    "comment names the value the line already shows"
+                ]
+            }
+        ]
+    }
+}

workflows/review/eval/corpus/clean/clean-documentation-earned-comments/tree/src/search/ranking.ts

@@ -0,0 +1,22 @@
+/** Result ranking for the search page. */
+
+export type Result = {id: string; score: number};
+
+/**
+ * Order results for display.
+ *
+ * Ties break on id rather than relying on sort stability: ranking runs on two
+ * services with different V8 versions, and a differing tie order surfaces as a
+ * phantom diff in the cached page.
+ */
+export const rankResults = (results: readonly Result[]): Result[] =>
+    [...results].sort(
+        (a, b) => b.score - a.score || (a.id < b.id ? -1 : a.id > b.id ? 1 : 0),
+    );
+
+// 0.35 came out of the 2026-05 relevance sweep: below it, keyword-only matches
+// pushed curated content off the first page.
+export const MIN_SCORE = 0.35;
+
+export const visible = (results: readonly Result[]): Result[] =>
+    rankResults(results).filter((result) => result.score >= MIN_SCORE);

workflows/review/eval/corpus/golden/golden-documentation-stale-and-narrated/case.json

@@ -0,0 +1,115 @@
+{
+    "id": "golden-documentation-stale-and-narrated",
+    "tags": ["golden", "documentation", "live"],
+    "category": "golden",
+    "description": "A correct behaviour change (expiry window 30 -> 90 days, clock injected) that leaves two documentation defects behind: a comment the change falsified, and a comment that narrates the change instead of the code. The change is otherwise sound, so the run must APPROVE while posting both as non-blocking documentation suggestions. The trap is the EXPIRY_SLACK_MS comment: it sits on a constant and looks like a restatement, but it records a real constraint (a reader holding an expiring note) that the code does not show, so it must not be flagged.",
+    "changedFiles": [{"path": "src/notes/expiry.ts", "status": "modified"}],
+    "routerConfig": {
+        "enabledReviewers": ["documentation"]
+    },
+    "findings": [
+        {
+            "source": "documentation",
+            "finding": {
+                "schema_version": 2,
+                "id": "doc-stale-expiry-days",
+                "lens": "documentation",
+                "anchor": {
+                    "type": "line",
+                    "path": "src/notes/expiry.ts",
+                    "line": 10,
+                    "side": "RIGHT"
+                },
+                "severity": "advisory",
+                "confidence": 0.85,
+                "evidence_trace": [
+                    "src/notes/expiry.ts:9 comment reads \"// Notes expire after 30 days.\"",
+                    "src/notes/expiry.ts:10 sets EXPIRY_DAYS to 90 in this change"
+                ],
+                "failure_scenario": "The next reader takes the 30-day figure from the comment and reasons about retention with a window three times shorter than the one the code enforces.",
+                "producing_hunt": "documentation:falsified-by-diff",
+                "model_authored_prose": "The comment above still says notes expire after 30 days, but this line changes the window to 90."
+            }
+        },
+        {
+            "source": "documentation",
+            "finding": {
+                "schema_version": 2,
+                "id": "doc-narrated-clock",
+                "lens": "documentation",
+                "anchor": {
+                    "type": "line",
+                    "path": "src/notes/expiry.ts",
+                    "line": 12,
+                    "side": "RIGHT"
+                },
+                "severity": "advisory",
+                "confidence": 0.8,
+                "evidence_trace": [
+                    "src/notes/expiry.ts:12 adds \"// Updated to take the clock from the caller instead of reading Date.now().\"",
+                    "src/notes/expiry.ts:13 the signature already shows `now` is a parameter"
+                ],
+                "failure_scenario": "The comment describes a past the next reader cannot see; once the PR is merged it documents a Date.now() call that no longer exists anywhere in the file.",
+                "producing_hunt": "documentation:narrates-the-change",
+                "model_authored_prose": "This comment describes the change rather than the code: the signature already shows the clock is a parameter, and \"updated to\" stops meaning anything once this merges."
+            }
+        }
+    ],
+    "policyConflicts": [],
+    "expected": {
+        "verdict": "APPROVE",
+        "mustCatch": ["doc-stale-expiry-days", "doc-narrated-clock"],
+        "postedCommentCount": 2
+    },
+    "diff": "diff --git a/src/notes/expiry.ts b/src/notes/expiry.ts\n--- a/src/notes/expiry.ts\n+++ b/src/notes/expiry.ts\n@@ -3,12 +3,18 @@\n const DAY_MS = 86_400_000;\n \n+// Rounded up: a note that expires mid-request must stay readable for the rest\n+// of that request, or the delete job races a reader holding it open.\n+const EXPIRY_SLACK_MS = 60_000;\n+\n // Notes expire after 30 days.\n-export const EXPIRY_DAYS = 30;\n+export const EXPIRY_DAYS = 90;\n \n-export const isExpired = (createdAt: number): boolean =>\n-    Date.now() - createdAt > EXPIRY_DAYS * DAY_MS;\n+// Updated to take the clock from the caller instead of reading Date.now().\n+export const isExpired = (createdAt: number, now: number): boolean =>\n+    now - createdAt > EXPIRY_DAYS * DAY_MS + EXPIRY_SLACK_MS;\n \n // Return the notes that have not expired yet.\n export const liveNotes = <T extends {createdAt: number}>(\n     notes: readonly T[],\n-): T[] => notes.filter((note) => !isExpired(note.createdAt));\n+    now: number,\n+): T[] => notes.filter((note) => !isExpired(note.createdAt, now));\n",
+    "live": {
+        "prContext": {
+            "title": "notes: extend the expiry window to 90 days and inject the clock",
+            "description": "Retention asked for 90 days. Also takes `now` from the caller so the sweep job and its tests agree on the clock.",
+            "author": "dev-notes",
+            "baseBranch": "main"
+        },
+        "mustCatchSpecs": [
+            {
+                "key": "doc-stale-expiry-days",
+                "path": "src/notes/expiry.ts",
+                "lineStart": 9,
+                "lineEnd": 10,
+                "lens": "documentation",
+                "mechanism": [
+                    "comment says 30 days but the constant is 90",
+                    "stale comment",
+                    "falsified by the change",
+                    "comment contradicts EXPIRY_DAYS"
+                ]
+            },
+            {
+                "key": "doc-narrated-clock",
+                "path": "src/notes/expiry.ts",
+                "lineStart": 12,
+                "lineEnd": 13,
+                "lens": "documentation",
+                "mechanism": [
+                    "comment narrates the change rather than the code",
+                    "updated to.*instead of",
+                    "change-log comment",
+                    "meaningful only at the time of the diff"
+                ]
+            }
+        ],
+        "mustNotFlagSpecs": [
+            {
+                "key": "doc-trap-slack-rationale",
+                "path": "src/notes/expiry.ts",
+                "lineStart": 5,
+                "lineEnd": 7,
+                "lens": "documentation",
+                "mechanism": [
+                    "EXPIRY_SLACK_MS comment is redundant",
+                    "restates the constant",
+                    "delete the slack comment"
+                ]
+            }
+        ]
+    }
+}