From a203ae9954b5c4cfe6d3cdda1717814ad30cb083 Mon Sep 17 00:00:00 2001 From: Miyoung Choi Date: Tue, 21 Jul 2026 17:03:58 -0700 Subject: [PATCH 1/3] ci(docs): collect documentation writer review data --- .github/PULL_REQUEST_TEMPLATE.md | 9 + .github/workflows/docs-review-receipt.yaml | 54 ++ AGENTS.md | 2 + CONTRIBUTING.md | 41 ++ package.json | 2 + scripts/docs-review-receipt.mts | 618 +++++++++++++++++++++ test/docs-review-receipt.test.ts | 305 ++++++++++ 7 files changed, 1031 insertions(+) create mode 100644 .github/workflows/docs-review-receipt.yaml create mode 100644 scripts/docs-review-receipt.mts create mode 100644 test/docs-review-receipt.test.ts diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index f59719df7e0..673e393a355 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -26,6 +26,15 @@ - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: +## Documentation Writer Review + +- [ ] Documentation writer subagent reviewed the completed implementation +- Result: `docs-updated` | `no-docs-needed` | `blocked` +- Evidence: +- Agent: +- Reviewed PR SHA: +- Reviewed `AGENTS.md` blob SHA: + ## DGX Station Hardware Evidence - [ ] Tested on DGX Station diff --git a/.github/workflows/docs-review-receipt.yaml b/.github/workflows/docs-review-receipt.yaml new file mode 100644 index 00000000000..cf6daeb5c84 --- /dev/null +++ b/.github/workflows/docs-review-receipt.yaml @@ -0,0 +1,54 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +name: CI / Documentation Writer Review + +on: + pull_request: + types: [opened, edited, synchronize, reopened] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + docs-review-receipt: + runs-on: ubuntu-latest + timeout-minutes: 3 + steps: + - name: Checkout + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Setup Node.js + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6 + with: + node-version: "22" + + - name: Collect receipt inputs + id: receipt-inputs + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + shell: bash + run: | + set -euo pipefail + changed_files="$RUNNER_TEMP/docs-review-changed-files.txt" + git diff --name-only "$BASE_SHA" "$HEAD_SHA" > "$changed_files" + agents_blob="$(git rev-parse "${HEAD_SHA}:AGENTS.md")" + echo "changed_files=$changed_files" >> "$GITHUB_OUTPUT" + echo "agents_blob=$agents_blob" >> "$GITHUB_OUTPUT" + + - name: Check documentation writer review receipt + continue-on-error: true + run: >- + node --experimental-strip-types --no-warnings scripts/docs-review-receipt.mts check + --event "$GITHUB_EVENT_PATH" + --changed-files "${{ steps.receipt-inputs.outputs.changed_files }}" + --agents-blob "${{ steps.receipt-inputs.outputs.agents_blob }}" + --mode advisory diff --git a/AGENTS.md b/AGENTS.md index beadf288b05..1935dcfdf2e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,6 +253,8 @@ Follow `.agents/skills/_shared/pr-follow-up.md`: after opening or pushing to a P - Treat `docs/` as the source of truth for user-facing documentation and follow `docs/CONTRIBUTING.md`. - After completing development changes, run a documentation writer subagent before final handoff. Give it the changed files, behavior summary, and test evidence so it can update docs or report that no doc changes are needed. +- After the review, complete the PR template's Documentation Writer Review section. Record the result, evidence, agent surface, reviewed PR SHA, and current `AGENTS.md` blob SHA. +- If implementation changes after the recorded PR SHA, rerun the documentation writer review and refresh the receipt. - For normal docs changes, include source pages under `docs/`. - Update `.agents/skills/nemoclaw-user-guide/SKILL.md` only when the AI-agent docs routing guidance changes. - During pre-tag release prep, run `nemoclaw-contributor-update-docs` and include the canonical release entry in the release-note docs PR. Create or update `docs/changelog/YYYY-MM-DD.mdx` for `vX.Y.Z` following `docs/CONTRIBUTING.md`; a PR that updates ordinary pages without the dated changelog entry is incomplete. Merge that PR, or record an explicit maintainer waiver, before generating the release plan. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2635111ef5a..171a8f1badb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -484,6 +484,47 @@ If your change affects user-facing behavior (new commands, changed defaults, new If you use an AI coding agent (Cursor, Claude Code, Codex, etc.), the repo includes the `nemoclaw-contributor-update-docs` skill that drafts doc updates. Use it before writing from scratch and follow the style guide in [docs/CONTRIBUTING.md](docs/CONTRIBUTING.md). During release prep, run that skill first, make any doc version bumps, then open the docs refresh PR. +### Documentation Writer Review Receipt + +After you complete a code change, a documentation writer subagent must review the implementation. +Complete the Documentation Writer Review section in the PR description after that review. + +Record one result: + +- `docs-updated` when the review changes documentation. List the changed documentation paths as evidence. +- `no-docs-needed` when the evidence explains why documentation does not change. +- `blocked` when a named decision, dependency, access problem, or input prevents the review. + +Record the product and surface that ran the review, such as `Codex Desktop`, `Codex CLI`, `Claude Code`, or `Cursor`. +Use the same name for the same surface across PRs so the report groups its data correctly. + +Commit all changes from the final review. +Then run these commands to record the revisions that the review covers: + +```bash +git rev-parse --short HEAD +git rev-parse --short HEAD:AGENTS.md +``` + +Rerun the review when implementation changes after the recorded PR SHA. +The Documentation Writer Review check reports an advisory finding when the receipt is missing, incomplete, or stale. +The check compares both recorded SHAs with the PR version. + +Maintainers can export receipt data from PR descriptions: + +```bash +npm run docs-review:report -- --since 2026-06-12 --format csv > /tmp/nemoclaw-docs-review.csv +``` + +The report uses the authenticated GitHub CLI session and returns JSON by default. +It measures receipt coverage, PR SHA freshness, review results, and agent-surface counts. +It records the `AGENTS.md` blob SHA, but only the PR check compares that SHA with the PR version. +It does not prove that an agent loaded `AGENTS.md`; it records observable workflow compliance. +The retrospective report classifies code changes from the checked Type of Change field. +It reports a PR as unclassified when that field is incomplete or contradictory. +Use `--format summary` to print only aggregate metrics. +Use `--until YYYY-MM-DD` to set the end of the reporting period. + To build and preview docs locally: ```console diff --git a/package.json b/package.json index a0b9c0068b7..e372bdf8dbf 100644 --- a/package.json +++ b/package.json @@ -72,6 +72,8 @@ "docs:sync-agent-variants": "npm run docs:prepare", "docs:check-agent-variants": "tsx scripts/sync-agent-variant-docs.mts --check", "docs:check-routes": "tsx scripts/check-docs-published-routes.mts", + "docs-review:check": "node --experimental-strip-types --no-warnings scripts/docs-review-receipt.mts check", + "docs-review:report": "node --experimental-strip-types --no-warnings scripts/docs-review-receipt.mts report", "docs:validate": "npm run docs:check-starter-prompt && npm run docs:check-agent-variants && npm run docs:check-routes && FERN_VERSION=$(node -p \"require('./fern/fern.config.json').version\") && cd fern && npx --yes \"fern-api@${FERN_VERSION}\" check", "docs:strict": "npm run docs:prepare && npm run docs:validate", "docs:live": "npm run docs:prepare && FERN_VERSION=$(node -p \"require('./fern/fern.config.json').version\") && cd fern && npx --yes \"fern-api@${FERN_VERSION}\" docs dev", diff --git a/scripts/docs-review-receipt.mts b/scripts/docs-review-receipt.mts new file mode 100644 index 00000000000..9257642d4d7 --- /dev/null +++ b/scripts/docs-review-receipt.mts @@ -0,0 +1,618 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Validate documentation writer review receipts and report their adoption. + * + * The check command reads one pull_request event and its changed paths. + * The report command reads pull requests through the authenticated gh CLI. + */ + +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; + +type ReviewResult = "blocked" | "docs-updated" | "no-docs-needed"; +type ReceiptStatus = "invalid" | "missing" | "not-required" | "unclassified" | "valid"; + +interface ChangeClassification { + codeChanged: boolean | null; + docsChanged: boolean | null; +} + +interface ParsedReceipt { + agent: string | null; + agentsBlobSha: string | null; + completed: boolean; + duplicateSections: boolean; + evidence: string | null; + present: boolean; + result: ReviewResult | null; + reviewedPrSha: string | null; +} + +interface ReceiptRecord { + agent: string | null; + agentsBlobSha: string | null; + agentsShaMatches: boolean | null; + codeChanged: boolean | null; + docsChanged: boolean | null; + evidence: string | null; + issues: string[]; + prShaMatches: boolean | null; + result: ReviewResult | null; + reviewedPrSha: string | null; + status: ReceiptStatus; +} + +interface PullRequestEvent { + pull_request?: { + body?: string | null; + head?: { sha?: string }; + html_url?: string; + number?: number; + }; +} + +interface GhPullRequest { + author: { login: string } | null; + body: string; + createdAt: string; + headRefOid: string; + isDraft: boolean; + mergedAt: string | null; + number: number; + state: string; + url: string; +} + +interface ReportRecord extends ReceiptRecord { + author: string | null; + createdAt: string; + headPrSha: string; + isDraft: boolean; + mergedAt: string | null; + number: number; + state: string; + url: string; +} + +const RESULTS = new Set(["blocked", "docs-updated", "no-docs-needed"]); +const SHA_PATTERN = /^[0-9a-f]{7,40}$/u; + +function main(): void { + const [command, ...args] = process.argv.slice(2); + if (command === "check") { + runCheck(args); + return; + } + if (command === "report") { + runReport(args); + return; + } + throw new Error("Usage: docs-review-receipt.mts [options]"); +} + +function runCheck(args: string[]): void { + const eventPath = requireOption(args, "--event"); + const changedFilesPath = requireOption(args, "--changed-files"); + const expectedAgentsBlob = requireOption(args, "--agents-blob"); + const mode = optionalOption(args, "--mode") ?? "advisory"; + if (mode !== "advisory" && mode !== "required") { + throw new Error(`Invalid --mode value: ${mode}`); + } + + const event = readJson(eventPath); + const pullRequest = event.pull_request; + const headPrSha = pullRequest?.head?.sha; + if (!pullRequest || !headPrSha || !SHA_PATTERN.test(headPrSha)) { + throw new Error("The event file does not contain a valid pull request head SHA"); + } + + const changedFiles = readLines(changedFilesPath); + const record = evaluateReceipt( + pullRequest.body ?? "", + classifyChangedFiles(changedFiles), + headPrSha, + expectedAgentsBlob, + ); + const output = { + type: "documentation-writer-review-receipt", + pr: pullRequest.number ?? null, + url: pullRequest.html_url ?? null, + headPrSha, + ...record, + }; + console.log(JSON.stringify(output)); + writeStepSummary(output); + + if (record.status === "missing" || record.status === "invalid") { + for (const issue of record.issues) { + console.error( + `::warning title=Documentation writer review receipt::${escapeAnnotation(issue)}`, + ); + } + if (mode === "required") process.exitCode = 1; + } +} + +function runReport(args: string[]): void { + const repository = optionalOption(args, "--repo") ?? "NVIDIA/NemoClaw"; + if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/u.test(repository)) { + throw new Error(`Invalid --repo value: ${repository}`); + } + const since = requireOption(args, "--since"); + const sinceDate = parseDate(since, "--since"); + const through = optionalOption(args, "--until") ?? new Date().toISOString().slice(0, 10); + const throughDate = parseDate(through, "--until"); + if (sinceDate > throughDate) throw new Error("--since must not be later than --until"); + const format = optionalOption(args, "--format") ?? "json"; + if (format !== "csv" && format !== "json" && format !== "summary") { + throw new Error(`Invalid --format value: ${format}`); + } + + const pullRequests = listPullRequests(repository, sinceDate, throughDate); + const records = pullRequests.map(toReportRecord); + const report = buildReport(repository, since, through, records); + + if (format === "csv") { + process.stdout.write(renderCsv(records)); + } else if (format === "summary") { + const { records: _records, ...summary } = report; + console.log(JSON.stringify(summary, null, 2)); + } else { + console.log(JSON.stringify(report, null, 2)); + } +} + +function evaluateReceipt( + body: string, + changes: ChangeClassification, + headPrSha: string, + expectedAgentsBlob?: string, +): ReceiptRecord { + const parsed = parseReceipt(body); + const { codeChanged, docsChanged } = changes; + const issues: string[] = []; + + if (codeChanged === null) { + issues.push("The PR description does not select one code or documentation-only change type."); + return { + agent: parsed.agent, + agentsBlobSha: parsed.agentsBlobSha, + agentsShaMatches: null, + codeChanged, + docsChanged, + evidence: parsed.evidence, + issues, + prShaMatches: null, + result: parsed.result, + reviewedPrSha: parsed.reviewedPrSha, + status: "unclassified", + }; + } + + if (!codeChanged) { + return { + agent: parsed.agent, + agentsBlobSha: parsed.agentsBlobSha, + agentsShaMatches: null, + codeChanged, + docsChanged, + evidence: parsed.evidence, + issues, + prShaMatches: null, + result: parsed.result, + reviewedPrSha: parsed.reviewedPrSha, + status: "not-required", + }; + } + + if (!parsed.present) { + issues.push("Code-changing PRs must include the Documentation Writer Review section."); + } else { + if (parsed.duplicateSections) { + issues.push("The PR description contains more than one Documentation Writer Review section."); + } + if (!parsed.completed) { + issues.push("Mark the documentation writer subagent review as completed."); + } + if (!parsed.result) { + issues.push("Keep exactly one result: docs-updated, no-docs-needed, or blocked."); + } + if (!parsed.evidence || looksLikePlaceholder(parsed.evidence)) { + issues.push("Add documentation review evidence or a no-docs-needed rationale."); + } + if (!parsed.agent || looksLikePlaceholder(parsed.agent)) { + issues.push("Record the agent surface that ran the documentation writer review."); + } + if (!parsed.reviewedPrSha) { + issues.push("Record a valid reviewed PR SHA with 7 to 40 hexadecimal characters."); + } + if (!parsed.agentsBlobSha) { + issues.push("Record a valid AGENTS.md blob SHA with 7 to 40 hexadecimal characters."); + } + if (parsed.result === "docs-updated" && docsChanged !== true) { + issues.push("The docs-updated result requires a changed Markdown or docs/ file."); + } + } + + const prShaMatches = parsed.reviewedPrSha + ? headPrSha.toLowerCase().startsWith(parsed.reviewedPrSha) + : null; + if (prShaMatches === false) { + issues.push("The reviewed PR SHA does not match the pull request head SHA."); + } + + const normalizedAgentsBlob = expectedAgentsBlob?.trim().toLowerCase(); + const agentsShaMatches = + parsed.agentsBlobSha && normalizedAgentsBlob + ? normalizedAgentsBlob.startsWith(parsed.agentsBlobSha) + : null; + if (agentsShaMatches === false) { + issues.push("The reviewed AGENTS.md blob SHA does not match the pull request version."); + } + + return { + agent: parsed.agent, + agentsBlobSha: parsed.agentsBlobSha, + agentsShaMatches, + codeChanged, + docsChanged, + evidence: parsed.evidence, + issues, + prShaMatches, + result: parsed.result, + reviewedPrSha: parsed.reviewedPrSha, + status: parsed.present ? (issues.length === 0 ? "valid" : "invalid") : "missing", + }; +} + +function parseReceipt(body: string): ParsedReceipt { + const headingPattern = /^## Documentation Writer Review\s*$/gmu; + const matches = [...body.matchAll(headingPattern)]; + if (matches.length === 0) { + return { + agent: null, + agentsBlobSha: null, + completed: false, + duplicateSections: false, + evidence: null, + present: false, + result: null, + reviewedPrSha: null, + }; + } + + const first = matches[0]; + const start = (first.index ?? 0) + first[0].length; + const remaining = body.slice(start); + const nextHeading = /^##\s+/mu.exec(remaining); + const section = remaining.slice(0, nextHeading?.index ?? remaining.length); + const lines = section.split(/\r?\n/u).map((line) => line.trim()); + const resultValue = fieldValue(lines, "Result"); + const resultMatch = resultValue?.match(/^`(blocked|docs-updated|no-docs-needed)`$/u); + const result = + resultMatch && RESULTS.has(resultMatch[1] as ReviewResult) + ? (resultMatch[1] as ReviewResult) + : null; + const reviewedPrSha = parseSha(fieldValue(lines, "Reviewed PR SHA")); + const agentsBlobSha = parseSha(fieldValue(lines, "Reviewed `AGENTS.md` blob SHA")); + + return { + agent: nonEmpty(fieldValue(lines, "Agent")), + agentsBlobSha, + completed: lines.some((line) => + /^- \[[xX]\] Documentation writer subagent reviewed the completed implementation$/u.test( + line, + ), + ), + duplicateSections: matches.length > 1, + evidence: nonEmpty(fieldValue(lines, "Evidence")), + present: true, + result, + reviewedPrSha, + }; +} + +function fieldValue(lines: string[], name: string): string | null { + const prefix = `- ${name}:`; + const line = lines.find((candidate) => candidate.startsWith(prefix)); + return line ? line.slice(prefix.length).trim() : null; +} + +function parseSha(value: string | null): string | null { + const normalized = value?.replace(/^`|`$/gu, "").trim().toLowerCase(); + return normalized && SHA_PATTERN.test(normalized) ? normalized : null; +} + +function nonEmpty(value: string | null): string | null { + const normalized = value?.trim(); + return normalized ? normalized : null; +} + +function looksLikePlaceholder(value: string): boolean { + return /[<>|]/u.test(value); +} + +function toReportRecord(pullRequest: GhPullRequest): ReportRecord { + return { + author: pullRequest.author?.login ?? null, + createdAt: pullRequest.createdAt, + headPrSha: pullRequest.headRefOid, + isDraft: pullRequest.isDraft, + mergedAt: pullRequest.mergedAt, + number: pullRequest.number, + state: pullRequest.state, + url: pullRequest.url, + ...evaluateReceipt( + pullRequest.body ?? "", + classifyPrType(pullRequest.body ?? ""), + pullRequest.headRefOid, + ), + }; +} + +function classifyChangedFiles(changedFiles: string[]): ChangeClassification { + return { + codeChanged: changedFiles.some(isCodeFile), + docsChanged: changedFiles.some(isDocumentationFile), + }; +} + +function classifyPrType(body: string): ChangeClassification { + const checked = body + .split(/\r?\n/u) + .map((line) => line.trim()) + .filter((line) => /^- \[[xX]\] /u.test(line)); + const codeOnly = checked.some( + (line) => line.startsWith("- [x] Code change (") || line.startsWith("- [X] Code change ("), + ); + const codeWithDocs = checked.some((line) => + /^- \[[xX]\] Code change with doc updates$/u.test(line), + ); + const docsOnly = checked.some((line) => /^- \[[xX]\] Doc only \(/u.test(line)); + + if ((codeOnly || codeWithDocs) && !docsOnly) { + return { codeChanged: true, docsChanged: codeWithDocs }; + } + if (docsOnly && !codeOnly && !codeWithDocs) { + return { codeChanged: false, docsChanged: true }; + } + return { codeChanged: null, docsChanged: null }; +} + +function isCodeFile(file: string): boolean { + return !file.startsWith("docs/") && !file.toLowerCase().endsWith(".md"); +} + +function isDocumentationFile(file: string): boolean { + const lower = file.toLowerCase(); + return file.startsWith("docs/") || lower.endsWith(".md") || lower.endsWith(".mdx"); +} + +function listPullRequests(repository: string, since: Date, through: Date): GhPullRequest[] { + const pullRequests = queryPullRequestRange(repository, since, through); + if (pullRequests.length < 1000) return pullRequests; + if (formatDate(since) === formatDate(through)) { + throw new Error(`The ${formatDate(since)} report reached GitHub's 1000-PR search limit.`); + } + + const rangeDays = Math.floor((through.getTime() - since.getTime()) / 86_400_000); + const midpoint = new Date(since.getTime() + Math.floor(rangeDays / 2) * 86_400_000); + const nextDay = new Date(midpoint.getTime() + 86_400_000); + return [ + ...listPullRequests(repository, since, midpoint), + ...listPullRequests(repository, nextDay, through), + ]; +} + +function queryPullRequestRange(repository: string, since: Date, through: Date): GhPullRequest[] { + return ghJsonArray([ + "pr", + "list", + "--repo", + repository, + "--state", + "all", + "--limit", + "1000", + "--search", + `created:${formatDate(since)}..${formatDate(through)}`, + "--json", + "number,url,state,isDraft,author,createdAt,mergedAt,headRefOid,body", + ]); +} + +function parseDate(value: string, option: string): Date { + if (!/^\d{4}-\d{2}-\d{2}$/u.test(value)) throw new Error(`Invalid ${option} value: ${value}`); + const parsed = new Date(`${value}T00:00:00.000Z`); + if (Number.isNaN(parsed.getTime()) || formatDate(parsed) !== value) { + throw new Error(`Invalid ${option} value: ${value}`); + } + return parsed; +} + +function formatDate(value: Date): string { + return value.toISOString().slice(0, 10); +} + +function buildReport(repository: string, since: string, through: string, records: ReportRecord[]) { + const eligible = records.filter((record) => record.codeChanged); + const unclassified = records.filter((record) => record.codeChanged === null); + const recorded = eligible.filter((record) => record.status !== "missing"); + const valid = eligible.filter((record) => record.status === "valid"); + const fresh = recorded.filter((record) => record.prShaMatches === true); + const resultCounts: Record = { + blocked: 0, + "docs-updated": 0, + "no-docs-needed": 0, + }; + const agentCounts: Record = {}; + for (const record of valid) { + if (record.result) resultCounts[record.result] += 1; + if (record.agent) { + const agent = record.agent.toLowerCase(); + agentCounts[agent] = (agentCounts[agent] ?? 0) + 1; + } + } + + return { + repository, + since, + through, + metrics: { + totalPrs: records.length, + eligibleCodePrs: eligible.length, + unclassifiedPrs: unclassified.length, + recordedReceipts: recorded.length, + receiptCoverage: ratio(recorded.length, eligible.length), + validReceipts: valid.length, + validReceiptRate: ratio(valid.length, eligible.length), + freshReceipts: fresh.length, + freshReceiptRate: ratio(fresh.length, recorded.length), + results: resultCounts, + agents: agentCounts, + }, + records, + }; +} + +function ratio(numerator: number, denominator: number): number | null { + return denominator === 0 ? null : Number((numerator / denominator).toFixed(4)); +} + +function renderCsv(records: ReportRecord[]): string { + const headers = [ + "number", + "url", + "state", + "is_draft", + "author", + "created_at", + "merged_at", + "code_changed", + "docs_changed", + "receipt_status", + "result", + "agent", + "reviewed_pr_sha", + "head_pr_sha", + "pr_sha_matches", + "agents_blob_sha", + "evidence", + "issues", + ]; + const rows = records.map((record) => [ + record.number, + record.url, + record.state, + record.isDraft, + record.author, + record.createdAt, + record.mergedAt, + record.codeChanged, + record.docsChanged, + record.status, + record.result, + record.agent, + record.reviewedPrSha, + record.headPrSha, + record.prShaMatches, + record.agentsBlobSha, + record.evidence, + record.issues.join("; "), + ]); + return `${[headers, ...rows].map((row) => row.map(csvCell).join(",")).join("\n")}\n`; +} + +function csvCell(value: boolean | number | string | null): string { + const text = value === null ? "" : String(value); + const formulaSafe = /^[=+\-@\t\r]/u.test(text) ? `'${text}` : text; + return /[",\n\r]/u.test(formulaSafe) ? `"${formulaSafe.replaceAll('"', '""')}"` : formulaSafe; +} + +function writeStepSummary(output: { + headPrSha: string; + pr: number | null; + status: ReceiptStatus; + result: ReviewResult | null; + agent: string | null; + issues: string[]; +}): void { + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (!summaryPath) return; + const lines = [ + "## Documentation writer review receipt", + "", + `- PR: ${output.pr === null ? "unknown" : `#${output.pr}`}`, + `- Head PR SHA: \`${output.headPrSha.slice(0, 12)}\``, + `- Status: \`${output.status}\``, + `- Result: ${output.result ? `\`${output.result}\`` : "not recorded"}`, + `- Agent: ${output.agent ?? "not recorded"}`, + ]; + if (output.issues.length > 0) { + lines.push("", "### Advisory findings", ""); + for (const issue of output.issues) lines.push(`- ${issue}`); + } + fs.appendFileSync(summaryPath, `${lines.join("\n")}\n`); +} + +function readJson(file: string): T { + return JSON.parse(fs.readFileSync(file, "utf8")) as T; +} + +function readLines(file: string): string[] { + return fs + .readFileSync(file, "utf8") + .split(/\r?\n/u) + .map((line) => line.trim()) + .filter(Boolean); +} + +function requireOption(args: string[], name: string): string { + const value = optionalOption(args, name); + if (!value) throw new Error(`Missing required option ${name}`); + return value; +} + +function optionalOption(args: string[], name: string): string | null { + const index = args.indexOf(name); + if (index === -1) return null; + const value = args[index + 1]; + if (!value || value.startsWith("--")) throw new Error(`Missing value for ${name}`); + return value; +} + +function ghJsonArray(args: string[]): T[] { + let output: string; + try { + output = execFileSync("gh", args, { + encoding: "utf8", + maxBuffer: 100 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + }); + } catch (error) { + const details = readErrorText(error); + throw new Error([`gh ${args.join(" ")} failed`, details].filter(Boolean).join("\n")); + } + const parsed = JSON.parse(output) as unknown; + if (!Array.isArray(parsed)) throw new Error("GitHub CLI did not return a JSON array"); + return parsed as T[]; +} + +function readErrorText(error: unknown): string | null { + if (typeof error !== "object" || error === null) return null; + const stderr = Reflect.get(error, "stderr"); + return typeof stderr === "string" ? stderr.trim() : null; +} + +function escapeAnnotation(value: string): string { + return value.replaceAll("%", "%25").replaceAll("\r", "%0D").replaceAll("\n", "%0A"); +} + +try { + main(); +} catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + process.exitCode = 1; +} diff --git a/test/docs-review-receipt.test.ts b/test/docs-review-receipt.test.ts new file mode 100644 index 00000000000..9f1f69b5562 --- /dev/null +++ b/test/docs-review-receipt.test.ts @@ -0,0 +1,305 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { describe, expect, it } from "vitest"; + +const SCRIPT = "scripts/docs-review-receipt.mts"; +const HEAD_SHA = "a".repeat(40); +const AGENTS_BLOB_SHA = "b".repeat(40); + +function receipt(overrides: Partial> = {}): string { + const values = { + checked: "x", + result: "`docs-updated`", + evidence: "Updated docs/get-started/quickstart.mdx.", + agent: "Codex", + prSha: HEAD_SHA.slice(0, 12), + agentsSha: AGENTS_BLOB_SHA.slice(0, 12), + ...overrides, + }; + return `## Documentation Writer Review + +- [${values.checked}] Documentation writer subagent reviewed the completed implementation +- Result: ${values.result} +- Evidence: ${values.evidence} +- Agent: ${values.agent} +- Reviewed PR SHA: ${values.prSha} +- Reviewed \`AGENTS.md\` blob SHA: ${values.agentsSha} +`; +} + +function runCheck( + body: string, + changedFiles: string[], + options: { agentsBlob?: string; mode?: "advisory" | "required" } = {}, +) { + const directory = fs.mkdtempSync(path.join(os.tmpdir(), "docs-review-receipt-")); + const eventPath = path.join(directory, "event.json"); + const changedFilesPath = path.join(directory, "changed-files.txt"); + const summaryPath = path.join(directory, "summary.md"); + fs.writeFileSync( + eventPath, + JSON.stringify({ + pull_request: { + number: 42, + html_url: "https://github.com/NVIDIA/NemoClaw/pull/42", + body, + head: { sha: HEAD_SHA }, + }, + }), + ); + fs.writeFileSync(changedFilesPath, `${changedFiles.join("\n")}\n`); + + try { + const result = spawnSync( + process.execPath, + [ + "--experimental-strip-types", + "--no-warnings", + SCRIPT, + "check", + "--event", + eventPath, + "--changed-files", + changedFilesPath, + "--agents-blob", + options.agentsBlob ?? AGENTS_BLOB_SHA, + "--mode", + options.mode ?? "advisory", + ], + { + cwd: process.cwd(), + encoding: "utf8", + env: { ...process.env, GITHUB_STEP_SUMMARY: summaryPath }, + }, + ); + return { + ...result, + output: result.stdout ? JSON.parse(result.stdout) : null, + summary: fs.existsSync(summaryPath) ? fs.readFileSync(summaryPath, "utf8") : "", + }; + } finally { + fs.rmSync(directory, { recursive: true, force: true }); + } +} + +describe("documentation writer review receipt", () => { + it("accepts a fresh receipt for a code and documentation change", () => { + const result = runCheck(receipt(), ["src/lib/example.ts", "docs/get-started/quickstart.mdx"]); + + expect(result.status).toBe(0); + expect(result.output).toMatchObject({ + status: "valid", + result: "docs-updated", + agent: "Codex", + prShaMatches: true, + agentsShaMatches: true, + issues: [], + }); + expect(result.summary).toContain("Status: `valid`"); + }); + + it("reports a missing receipt without blocking advisory mode", () => { + const result = runCheck("## Summary\n\nChange the CLI.\n", ["src/lib/example.ts"]); + + expect(result.status).toBe(0); + expect(result.output.status).toBe("missing"); + expect(result.stderr).toContain("Code-changing PRs must include"); + }); + + it("fails required mode when a code change has no receipt", () => { + const result = runCheck("## Summary\n\nChange the CLI.\n", ["src/lib/example.ts"], { + mode: "required", + }); + + expect(result.status).toBe(1); + expect(result.output.status).toBe("missing"); + }); + + it("does not require a receipt for a documentation-only change", () => { + const result = runCheck("## Summary\n\nUpdate prose.\n", ["README.md", "docs/index.mdx"]); + + expect(result.status).toBe(0); + expect(result.output).toMatchObject({ status: "not-required", codeChanged: false }); + expect(result.stderr).toBe(""); + }); + + it("reports stale PR and AGENTS.md revisions", () => { + const result = runCheck( + receipt({ + result: "`no-docs-needed`", + evidence: "The change affects an internal test helper only.", + prSha: "c".repeat(12), + agentsSha: "d".repeat(12), + }), + ["test/example.test.ts"], + ); + + expect(result.output.status).toBe("invalid"); + expect(result.output.issues).toEqual( + expect.arrayContaining([ + "The reviewed PR SHA does not match the pull request head SHA.", + "The reviewed AGENTS.md blob SHA does not match the pull request version.", + ]), + ); + }); + + it("requires a documentation path for docs-updated", () => { + const result = runCheck(receipt(), ["src/lib/example.ts"]); + + expect(result.output.status).toBe("invalid"); + expect(result.output.issues).toContain( + "The docs-updated result requires a changed Markdown or docs/ file.", + ); + }); + + it("rejects the unmodified PR template fields", () => { + const result = runCheck( + receipt({ + checked: " ", + result: "`docs-updated` | `no-docs-needed` | `blocked`", + evidence: "", + agent: "", + prSha: "", + agentsSha: "", + }), + ["src/lib/example.ts"], + ); + + expect(result.output.status).toBe("invalid"); + expect(result.output.issues).toHaveLength(6); + }); +}); + +describe("documentation writer review report", () => { + it("summarizes eligible PRs and emits CSV records", () => { + const directory = fs.mkdtempSync(path.join(os.tmpdir(), "docs-review-report-")); + const bin = path.join(directory, "bin"); + fs.mkdirSync(bin); + const ghPath = path.join(bin, "gh"); + const pullRequests = [ + { + number: 1, + url: "https://github.com/NVIDIA/NemoClaw/pull/1", + state: "MERGED", + isDraft: false, + author: { login: "engineer" }, + createdAt: "2026-06-13T00:00:00Z", + mergedAt: "2026-06-14T00:00:00Z", + headRefOid: HEAD_SHA, + body: `## Type of Change + +- [x] Code change with doc updates + +${receipt({ evidence: "=1+1" })}`, + files: [{ path: "src/lib/example.ts" }, { path: "docs/index.mdx" }], + }, + { + number: 2, + url: "https://github.com/NVIDIA/NemoClaw/pull/2", + state: "OPEN", + isDraft: true, + author: { login: "engineer" }, + createdAt: "2026-06-15T00:00:00Z", + mergedAt: null, + headRefOid: "c".repeat(40), + body: `## Type of Change + +- [x] Code change (feature, bug fix, or refactor) +`, + files: [{ path: "src/lib/other.ts" }], + }, + { + number: 3, + url: "https://github.com/NVIDIA/NemoClaw/pull/3", + state: "MERGED", + isDraft: false, + author: { login: "writer" }, + createdAt: "2026-06-16T00:00:00Z", + mergedAt: "2026-06-17T00:00:00Z", + headRefOid: "d".repeat(40), + body: `## Type of Change + +- [x] Doc only (prose changes, no code sample modifications) +`, + files: [{ path: "docs/index.mdx" }], + }, + ]; + fs.writeFileSync( + ghPath, + `#!/usr/bin/env bash +printf '%s' '${JSON.stringify(pullRequests)}' +`, + ); + fs.chmodSync(ghPath, 0o755); + + try { + const jsonResult = spawnSync( + process.execPath, + [ + "--experimental-strip-types", + "--no-warnings", + SCRIPT, + "report", + "--since", + "2026-06-12", + "--until", + "2026-06-12", + ], + { + cwd: process.cwd(), + encoding: "utf8", + env: { ...process.env, PATH: `${bin}${path.delimiter}${process.env.PATH ?? ""}` }, + }, + ); + const report = JSON.parse(jsonResult.stdout); + expect(jsonResult.status).toBe(0); + expect(report.metrics).toEqual({ + totalPrs: 3, + eligibleCodePrs: 2, + unclassifiedPrs: 0, + recordedReceipts: 1, + receiptCoverage: 0.5, + validReceipts: 1, + validReceiptRate: 0.5, + freshReceipts: 1, + freshReceiptRate: 1, + results: { blocked: 0, "docs-updated": 1, "no-docs-needed": 0 }, + agents: { codex: 1 }, + }); + + const csvResult = spawnSync( + process.execPath, + [ + "--experimental-strip-types", + "--no-warnings", + SCRIPT, + "report", + "--since", + "2026-06-12", + "--until", + "2026-06-12", + "--format", + "csv", + ], + { + cwd: process.cwd(), + encoding: "utf8", + env: { ...process.env, PATH: `${bin}${path.delimiter}${process.env.PATH ?? ""}` }, + }, + ); + expect(csvResult.status).toBe(0); + expect(csvResult.stdout).toContain("receipt_status"); + expect(csvResult.stdout).toContain("1,https://github.com/NVIDIA/NemoClaw/pull/1"); + expect(csvResult.stdout).toContain("2,https://github.com/NVIDIA/NemoClaw/pull/2"); + expect(csvResult.stdout).toContain("'=1+1"); + } finally { + fs.rmSync(directory, { recursive: true, force: true }); + } + }); +}); From 73740914467e11ef449af52da5a84d974f89c7d2 Mon Sep 17 00:00:00 2001 From: Miyoung Choi Date: Tue, 21 Jul 2026 17:30:33 -0700 Subject: [PATCH 2/3] ci(docs): scope review receipts to pull requests --- .github/PULL_REQUEST_TEMPLATE.md | 7 ++- AGENTS.md | 4 +- CONTRIBUTING.md | 13 +++-- scripts/docs-review-receipt.mts | 95 ++++++++++++++++++++++++-------- test/docs-review-receipt.test.ts | 27 +++++---- 5 files changed, 102 insertions(+), 44 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 673e393a355..4f3d95e4d67 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -27,13 +27,14 @@ - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - + - [ ] Documentation writer subagent reviewed the completed implementation - Result: `docs-updated` | `no-docs-needed` | `blocked` - Evidence: - Agent: -- Reviewed PR SHA: -- Reviewed `AGENTS.md` blob SHA: +- PR: # + + ## DGX Station Hardware Evidence diff --git a/AGENTS.md b/AGENTS.md index 1935dcfdf2e..0c0ef1cb800 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,8 +253,8 @@ Follow `.agents/skills/_shared/pr-follow-up.md`: after opening or pushing to a P - Treat `docs/` as the source of truth for user-facing documentation and follow `docs/CONTRIBUTING.md`. - After completing development changes, run a documentation writer subagent before final handoff. Give it the changed files, behavior summary, and test evidence so it can update docs or report that no doc changes are needed. -- After the review, complete the PR template's Documentation Writer Review section. Record the result, evidence, agent surface, reviewed PR SHA, and current `AGENTS.md` blob SHA. -- If implementation changes after the recorded PR SHA, rerun the documentation writer review and refresh the receipt. +- After the review, complete the PR template's Documentation Writer Review section. Record the result, evidence, agent surface, and PR number. Put the reviewed head SHA and current `AGENTS.md` blob SHA in the template's hidden metadata comments. +- If implementation changes after the hidden head SHA, rerun the documentation writer review and refresh the hidden metadata. The receipt check runs again when new commits are pushed. - For normal docs changes, include source pages under `docs/`. - Update `.agents/skills/nemoclaw-user-guide/SKILL.md` only when the AI-agent docs routing guidance changes. - During pre-tag release prep, run `nemoclaw-contributor-update-docs` and include the canonical release entry in the release-note docs PR. Create or update `docs/changelog/YYYY-MM-DD.mdx` for `vX.Y.Z` following `docs/CONTRIBUTING.md`; a PR that updates ordinary pages without the dated changelog entry is incomplete. Merge that PR, or record an explicit maintainer waiver, before generating the release plan. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 171a8f1badb..b9ef200217c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -497,18 +497,21 @@ Record one result: Record the product and surface that ran the review, such as `Codex Desktop`, `Codex CLI`, `Claude Code`, or `Cursor`. Use the same name for the same surface across PRs so the report groups its data correctly. +Record the PR number in the visible receipt so the check can detect a receipt copied from another PR. Commit all changes from the final review. -Then run these commands to record the revisions that the review covers: +Then run these commands and put their values in the receipt's hidden HTML metadata comments: ```bash git rev-parse --short HEAD git rev-parse --short HEAD:AGENTS.md ``` -Rerun the review when implementation changes after the recorded PR SHA. +The visible PR number ties the receipt to this PR, while the hidden head SHA identifies the implementation revision that the review covered. +Rerun the review when implementation changes after the hidden head SHA. +Pushing a new commit runs the receipt check again and reports the review as stale until the hidden metadata is refreshed. The Documentation Writer Review check reports an advisory finding when the receipt is missing, incomplete, or stale. -The check compares both recorded SHAs with the PR version. +The check compares the receipt's PR number with the current PR number, the hidden head SHA with the current PR head, and the hidden `AGENTS.md` blob SHA with the current PR's file. Maintainers can export receipt data from PR descriptions: @@ -517,8 +520,8 @@ npm run docs-review:report -- --since 2026-06-12 --format csv > /tmp/nemoclaw-do ``` The report uses the authenticated GitHub CLI session and returns JSON by default. -It measures receipt coverage, PR SHA freshness, review results, and agent-surface counts. -It records the `AGENTS.md` blob SHA, but only the PR check compares that SHA with the PR version. +It measures receipt coverage, PR-number integrity, head-revision freshness, review results, and agent-surface counts. +It records the `AGENTS.md` blob SHA, but only the PR check compares that SHA with the current PR's file. It does not prove that an agent loaded `AGENTS.md`; it records observable workflow compliance. The retrospective report classifies code changes from the checked Type of Change field. It reports a PR as unclassified when that field is incomplete or contradictory. diff --git a/scripts/docs-review-receipt.mts b/scripts/docs-review-receipt.mts index 9257642d4d7..f69ad491757 100644 --- a/scripts/docs-review-receipt.mts +++ b/scripts/docs-review-receipt.mts @@ -25,9 +25,10 @@ interface ParsedReceipt { completed: boolean; duplicateSections: boolean; evidence: string | null; + prNumber: number | null; present: boolean; result: ReviewResult | null; - reviewedPrSha: string | null; + reviewedHeadSha: string | null; } interface ReceiptRecord { @@ -37,10 +38,12 @@ interface ReceiptRecord { codeChanged: boolean | null; docsChanged: boolean | null; evidence: string | null; + headShaMatches: boolean | null; issues: string[]; - prShaMatches: boolean | null; + prNumber: number | null; + prNumberMatches: boolean | null; result: ReviewResult | null; - reviewedPrSha: string | null; + reviewedHeadSha: string | null; status: ReceiptStatus; } @@ -112,6 +115,7 @@ function runCheck(args: string[]): void { const record = evaluateReceipt( pullRequest.body ?? "", classifyChangedFiles(changedFiles), + pullRequest.number ?? null, headPrSha, expectedAgentsBlob, ); @@ -167,6 +171,7 @@ function runReport(args: string[]): void { function evaluateReceipt( body: string, changes: ChangeClassification, + expectedPrNumber: number | null, headPrSha: string, expectedAgentsBlob?: string, ): ReceiptRecord { @@ -183,10 +188,12 @@ function evaluateReceipt( codeChanged, docsChanged, evidence: parsed.evidence, + headShaMatches: null, issues, - prShaMatches: null, + prNumber: parsed.prNumber, + prNumberMatches: null, result: parsed.result, - reviewedPrSha: parsed.reviewedPrSha, + reviewedHeadSha: parsed.reviewedHeadSha, status: "unclassified", }; } @@ -199,10 +206,12 @@ function evaluateReceipt( codeChanged, docsChanged, evidence: parsed.evidence, + headShaMatches: null, issues, - prShaMatches: null, + prNumber: parsed.prNumber, + prNumberMatches: null, result: parsed.result, - reviewedPrSha: parsed.reviewedPrSha, + reviewedHeadSha: parsed.reviewedHeadSha, status: "not-required", }; } @@ -225,8 +234,11 @@ function evaluateReceipt( if (!parsed.agent || looksLikePlaceholder(parsed.agent)) { issues.push("Record the agent surface that ran the documentation writer review."); } - if (!parsed.reviewedPrSha) { - issues.push("Record a valid reviewed PR SHA with 7 to 40 hexadecimal characters."); + if (parsed.prNumber === null) { + issues.push("Record this pull request number in the documentation review receipt."); + } + if (!parsed.reviewedHeadSha) { + issues.push("Refresh the hidden head SHA after the documentation writer review."); } if (!parsed.agentsBlobSha) { issues.push("Record a valid AGENTS.md blob SHA with 7 to 40 hexadecimal characters."); @@ -236,11 +248,19 @@ function evaluateReceipt( } } - const prShaMatches = parsed.reviewedPrSha - ? headPrSha.toLowerCase().startsWith(parsed.reviewedPrSha) + const prNumberMatches = + parsed.prNumber !== null && expectedPrNumber !== null + ? parsed.prNumber === expectedPrNumber + : null; + if (prNumberMatches === false) { + issues.push("The receipt PR number does not match this pull request."); + } + + const headShaMatches = parsed.reviewedHeadSha + ? headPrSha.toLowerCase().startsWith(parsed.reviewedHeadSha) : null; - if (prShaMatches === false) { - issues.push("The reviewed PR SHA does not match the pull request head SHA."); + if (headShaMatches === false) { + issues.push("The documentation writer review is stale after a new implementation commit."); } const normalizedAgentsBlob = expectedAgentsBlob?.trim().toLowerCase(); @@ -259,10 +279,12 @@ function evaluateReceipt( codeChanged, docsChanged, evidence: parsed.evidence, + headShaMatches, issues, - prShaMatches, + prNumber: parsed.prNumber, + prNumberMatches, result: parsed.result, - reviewedPrSha: parsed.reviewedPrSha, + reviewedHeadSha: parsed.reviewedHeadSha, status: parsed.present ? (issues.length === 0 ? "valid" : "invalid") : "missing", }; } @@ -277,9 +299,10 @@ function parseReceipt(body: string): ParsedReceipt { completed: false, duplicateSections: false, evidence: null, + prNumber: null, present: false, result: null, - reviewedPrSha: null, + reviewedHeadSha: null, }; } @@ -295,8 +318,9 @@ function parseReceipt(body: string): ParsedReceipt { resultMatch && RESULTS.has(resultMatch[1] as ReviewResult) ? (resultMatch[1] as ReviewResult) : null; - const reviewedPrSha = parseSha(fieldValue(lines, "Reviewed PR SHA")); - const agentsBlobSha = parseSha(fieldValue(lines, "Reviewed `AGENTS.md` blob SHA")); + const prNumber = parsePrNumber(fieldValue(lines, "PR")); + const reviewedHeadSha = parseSha(hiddenFieldValue(lines, "docs-review-head-sha")); + const agentsBlobSha = parseSha(hiddenFieldValue(lines, "docs-review-agents-blob-sha")); return { agent: nonEmpty(fieldValue(lines, "Agent")), @@ -308,9 +332,10 @@ function parseReceipt(body: string): ParsedReceipt { ), duplicateSections: matches.length > 1, evidence: nonEmpty(fieldValue(lines, "Evidence")), + prNumber, present: true, result, - reviewedPrSha, + reviewedHeadSha, }; } @@ -320,6 +345,21 @@ function fieldValue(lines: string[], name: string): string | null { return line ? line.slice(prefix.length).trim() : null; } +function hiddenFieldValue(lines: string[], name: string): string | null { + const prefix = `"; + const line = lines.find( + (candidate) => candidate.startsWith(prefix) && candidate.endsWith(suffix), + ); + return line ? line.slice(prefix.length, -suffix.length).trim() : null; +} + +function parsePrNumber(value: string | null): number | null { + const normalized = value?.trim(); + if (!normalized || !/^#[1-9]\d*$/u.test(normalized)) return null; + return Number(normalized.slice(1)); +} + function parseSha(value: string | null): string | null { const normalized = value?.replace(/^`|`$/gu, "").trim().toLowerCase(); return normalized && SHA_PATTERN.test(normalized) ? normalized : null; @@ -347,6 +387,7 @@ function toReportRecord(pullRequest: GhPullRequest): ReportRecord { ...evaluateReceipt( pullRequest.body ?? "", classifyPrType(pullRequest.body ?? ""), + pullRequest.number, pullRequest.headRefOid, ), }; @@ -441,7 +482,9 @@ function buildReport(repository: string, since: string, through: string, records const unclassified = records.filter((record) => record.codeChanged === null); const recorded = eligible.filter((record) => record.status !== "missing"); const valid = eligible.filter((record) => record.status === "valid"); - const fresh = recorded.filter((record) => record.prShaMatches === true); + const fresh = recorded.filter( + (record) => record.prNumberMatches === true && record.headShaMatches === true, + ); const resultCounts: Record = { blocked: 0, "docs-updated": 0, @@ -495,9 +538,11 @@ function renderCsv(records: ReportRecord[]): string { "receipt_status", "result", "agent", - "reviewed_pr_sha", + "receipt_pr_number", + "pr_number_matches", + "reviewed_head_sha", "head_pr_sha", - "pr_sha_matches", + "head_sha_matches", "agents_blob_sha", "evidence", "issues", @@ -515,9 +560,11 @@ function renderCsv(records: ReportRecord[]): string { record.status, record.result, record.agent, - record.reviewedPrSha, + record.prNumber, + record.prNumberMatches, + record.reviewedHeadSha, record.headPrSha, - record.prShaMatches, + record.headShaMatches, record.agentsBlobSha, record.evidence, record.issues.join("; "), diff --git a/test/docs-review-receipt.test.ts b/test/docs-review-receipt.test.ts index 9f1f69b5562..93c1da0e531 100644 --- a/test/docs-review-receipt.test.ts +++ b/test/docs-review-receipt.test.ts @@ -18,7 +18,8 @@ function receipt(overrides: Partial> = {}): string { result: "`docs-updated`", evidence: "Updated docs/get-started/quickstart.mdx.", agent: "Codex", - prSha: HEAD_SHA.slice(0, 12), + prNumber: "#42", + headSha: HEAD_SHA.slice(0, 12), agentsSha: AGENTS_BLOB_SHA.slice(0, 12), ...overrides, }; @@ -28,8 +29,9 @@ function receipt(overrides: Partial> = {}): string { - Result: ${values.result} - Evidence: ${values.evidence} - Agent: ${values.agent} -- Reviewed PR SHA: ${values.prSha} -- Reviewed \`AGENTS.md\` blob SHA: ${values.agentsSha} +- PR: ${values.prNumber} + + `; } @@ -97,7 +99,9 @@ describe("documentation writer review receipt", () => { status: "valid", result: "docs-updated", agent: "Codex", - prShaMatches: true, + prNumber: 42, + prNumberMatches: true, + headShaMatches: true, agentsShaMatches: true, issues: [], }); @@ -129,12 +133,13 @@ describe("documentation writer review receipt", () => { expect(result.stderr).toBe(""); }); - it("reports stale PR and AGENTS.md revisions", () => { + it("reports a copied PR number and stale head and AGENTS.md revisions", () => { const result = runCheck( receipt({ result: "`no-docs-needed`", evidence: "The change affects an internal test helper only.", - prSha: "c".repeat(12), + prNumber: "#41", + headSha: "c".repeat(12), agentsSha: "d".repeat(12), }), ["test/example.test.ts"], @@ -143,7 +148,8 @@ describe("documentation writer review receipt", () => { expect(result.output.status).toBe("invalid"); expect(result.output.issues).toEqual( expect.arrayContaining([ - "The reviewed PR SHA does not match the pull request head SHA.", + "The receipt PR number does not match this pull request.", + "The documentation writer review is stale after a new implementation commit.", "The reviewed AGENTS.md blob SHA does not match the pull request version.", ]), ); @@ -165,14 +171,15 @@ describe("documentation writer review receipt", () => { result: "`docs-updated` | `no-docs-needed` | `blocked`", evidence: "", agent: "", - prSha: "", + prNumber: "#", + headSha: "", agentsSha: "", }), ["src/lib/example.ts"], ); expect(result.output.status).toBe("invalid"); - expect(result.output.issues).toHaveLength(6); + expect(result.output.issues).toHaveLength(7); }); }); @@ -196,7 +203,7 @@ describe("documentation writer review report", () => { - [x] Code change with doc updates -${receipt({ evidence: "=1+1" })}`, +${receipt({ evidence: "=1+1", prNumber: "#1" })}`, files: [{ path: "src/lib/example.ts" }, { path: "docs/index.mdx" }], }, { From b0d457467b7107b9f960509cc27a0b70840700e7 Mon Sep 17 00:00:00 2001 From: Miyoung Choi Date: Wed, 22 Jul 2026 11:55:27 -0700 Subject: [PATCH 3/3] fix(docs): validate review receipt fields --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- CONTRIBUTING.md | 1 + scripts/docs-review-receipt.mts | 47 ++++++++++++++++++++++++-------- test/docs-review-receipt.test.ts | 25 +++++++++++++++++ 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 2275e01df41..a412fcec8ca 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -27,7 +27,7 @@ - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - + - [ ] Documentation writer subagent reviewed the completed implementation - Result: `docs-updated` | `no-docs-needed` | `blocked` - Evidence: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e1a9a870765..8161c8aa4f5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -404,6 +404,7 @@ During release prep, run that skill first, make any doc version bumps, then open After you complete a code change, a documentation writer subagent must review the implementation. Complete the Documentation Writer Review section in the PR description after that review. +Keep one review completion checkbox and one instance of each visible or hidden field. Record one result: diff --git a/scripts/docs-review-receipt.mts b/scripts/docs-review-receipt.mts index f69ad491757..ee633f4b09e 100644 --- a/scripts/docs-review-receipt.mts +++ b/scripts/docs-review-receipt.mts @@ -23,6 +23,7 @@ interface ParsedReceipt { agent: string | null; agentsBlobSha: string | null; completed: boolean; + duplicateFields: string[]; duplicateSections: boolean; evidence: string | null; prNumber: number | null; @@ -222,6 +223,11 @@ function evaluateReceipt( if (parsed.duplicateSections) { issues.push("The PR description contains more than one Documentation Writer Review section."); } + if (parsed.duplicateFields.length > 0) { + issues.push( + `The Documentation Writer Review section repeats singleton fields: ${parsed.duplicateFields.join(", ")}.`, + ); + } if (!parsed.completed) { issues.push("Mark the documentation writer subagent review as completed."); } @@ -297,6 +303,7 @@ function parseReceipt(body: string): ParsedReceipt { agent: null, agentsBlobSha: null, completed: false, + duplicateFields: [], duplicateSections: false, evidence: null, prNumber: null, @@ -312,6 +319,17 @@ function parseReceipt(body: string): ParsedReceipt { const nextHeading = /^##\s+/mu.exec(remaining); const section = remaining.slice(0, nextHeading?.index ?? remaining.length); const lines = section.split(/\r?\n/u).map((line) => line.trim()); + const completionPattern = + /^- \[[xX]\] Documentation writer subagent reviewed the completed implementation$/u; + const duplicateFields = ["Result", "Evidence", "Agent", "PR"].filter( + (name) => fieldValues(lines, name).length > 1, + ); + for (const name of ["docs-review-head-sha", "docs-review-agents-blob-sha"]) { + if (hiddenFieldValues(lines, name).length > 1) duplicateFields.push(name); + } + if (lines.filter((line) => completionPattern.test(line)).length > 1) { + duplicateFields.push("review completion checkbox"); + } const resultValue = fieldValue(lines, "Result"); const resultMatch = resultValue?.match(/^`(blocked|docs-updated|no-docs-needed)`$/u); const result = @@ -325,11 +343,8 @@ function parseReceipt(body: string): ParsedReceipt { return { agent: nonEmpty(fieldValue(lines, "Agent")), agentsBlobSha, - completed: lines.some((line) => - /^- \[[xX]\] Documentation writer subagent reviewed the completed implementation$/u.test( - line, - ), - ), + completed: lines.some((line) => completionPattern.test(line)), + duplicateFields, duplicateSections: matches.length > 1, evidence: nonEmpty(fieldValue(lines, "Evidence")), prNumber, @@ -340,18 +355,26 @@ function parseReceipt(body: string): ParsedReceipt { } function fieldValue(lines: string[], name: string): string | null { + return fieldValues(lines, name)[0] ?? null; +} + +function fieldValues(lines: string[], name: string): string[] { const prefix = `- ${name}:`; - const line = lines.find((candidate) => candidate.startsWith(prefix)); - return line ? line.slice(prefix.length).trim() : null; + return lines + .filter((candidate) => candidate.startsWith(prefix)) + .map((line) => line.slice(prefix.length).trim()); } function hiddenFieldValue(lines: string[], name: string): string | null { + return hiddenFieldValues(lines, name)[0] ?? null; +} + +function hiddenFieldValues(lines: string[], name: string): string[] { const prefix = `"; - const line = lines.find( - (candidate) => candidate.startsWith(prefix) && candidate.endsWith(suffix), - ); - return line ? line.slice(prefix.length, -suffix.length).trim() : null; + return lines + .filter((candidate) => candidate.startsWith(prefix) && candidate.endsWith(suffix)) + .map((line) => line.slice(prefix.length, -suffix.length).trim()); } function parsePrNumber(value: string | null): number | null { @@ -423,7 +446,7 @@ function classifyPrType(body: string): ChangeClassification { } function isCodeFile(file: string): boolean { - return !file.startsWith("docs/") && !file.toLowerCase().endsWith(".md"); + return !isDocumentationFile(file); } function isDocumentationFile(file: string): boolean { diff --git a/test/docs-review-receipt.test.ts b/test/docs-review-receipt.test.ts index 93c1da0e531..f8ccb8793dd 100644 --- a/test/docs-review-receipt.test.ts +++ b/test/docs-review-receipt.test.ts @@ -133,6 +133,31 @@ describe("documentation writer review receipt", () => { expect(result.stderr).toBe(""); }); + it("does not require a receipt for an MDX file outside docs", () => { + const result = runCheck("## Summary\n\nUpdate prose.\n", ["examples/guide.mdx"]); + + expect(result.status).toBe(0); + expect(result.output).toMatchObject({ + status: "not-required", + codeChanged: false, + docsChanged: true, + }); + expect(result.stderr).toBe(""); + }); + + it("rejects repeated singleton receipt fields", () => { + const body = receipt().replace( + "- Result: `docs-updated`", + "- Result: `docs-updated`\n- Result: `no-docs-needed`", + ); + const result = runCheck(body, ["src/lib/example.ts", "docs/index.mdx"]); + + expect(result.output.status).toBe("invalid"); + expect(result.output.issues).toContain( + "The Documentation Writer Review section repeats singleton fields: Result.", + ); + }); + it("reports a copied PR number and stale head and AGENTS.md revisions", () => { const result = runCheck( receipt({