From ec5106b3b8b68224860d94ec5f5c9b50f9c980d2 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 11 Aug 2026 17:07:45 -0700 Subject: [PATCH] Fix AI code review hiding details, stale+misleading driver (#15087) Summary: The Claude/Codex CI reviewer was posting PR comments like "the review has been written to review-findings.md" instead of the review itself, and the comment footer cited claude_md/code_review.md as the review methodology even though CI stopped feeding that file to the model in https://github.com/facebook/rocksdb/issues/14507 (it switched to claude_md/ci_review_prompt.md). code_review.md had been orphaned from CI for months while still advertising itself as the CI methodology, so edits to it (e.g. the "post directly in your response" note) had no effect on CI. Separately, an over-long review exceeded GitHub's per-comment character limit and was rejected outright, losing the whole review. Changes: - ci_review_prompt.md: make the final response the review -- it becomes the PR comment verbatim -- and forbid ending the turn with a meta-summary such as "written to review-findings.md". review-findings.md is demoted to a recovery backstop; the per-phase incremental writes and the turn-limit recovery path are unchanged, so both the normal and recovery comment paths still produce a real review. - Repoint the "Review methodology" footer in parse-claude-review.js and parse-codex-review.js to claude_md/ci_review_prompt.md (the file CI actually uses), so the citation no longer points at a deleted file. - build-ai-review-comment.js: cap the assembled comment at GitHub's 65536-char limit, trimming only the model response (the sole unbounded input) and appending a pointer to the execution-log artifact, so an oversized review is truncated cleanly instead of rejected by GitHub. The header, footer, and
scaffolding are always preserved. - Delete claude_md/code_review.md (CI-dead since https://github.com/facebook/rocksdb/issues/14507; superseded locally). - Add claude_md/manual_code_review.md: a lightweight, single-reviewer local wrapper around ci_review_prompt.md (no multi-agent team/debate, no scratch files, inline output) for running a modest review by hand. It sources its methodology from ci_review_prompt.md so it cannot drift. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15087 Test Plan: - The workflow scripts have no CI unit coverage; verified both parse-*-review.js still parse, and that no reference to code_review.md remains anywhere. - Dry-ran manual_code_review.md by reviewing the current commit locally; it produced a right-sized single-reviewer report without spawning agents or writing files, and helped find+fix the comment limit issue. - Exercised the end-to-end GitHub CI path (per https://github.com/facebook/rocksdb/issues/14696, review-workflow changes not yet on main can only be tested from a branch on facebook/rocksdb): pushed these changes to the fix_hidden_review_details_test branch and ran the Claude Code Review workflow (claude-review.yml) via "Run workflow" against PR https://github.com/facebook/rocksdb/issues/15079, confirming the posted comment carries the full review inline (not a review-findings.md summary) with the ci_review_prompt.md footer. https://github.com/facebook/rocksdb/actions/runs/31537781229 https://github.com/facebook/rocksdb/pull/15079#issuecomment-5259127037 Reviewed By: xingbowang Differential Revision: D115635558 Pulled By: pdillinger fbshipit-source-id: d3ca7188162a086cd42d7b8bd8e60f3fc3df3bad --- .github/scripts/build-ai-review-comment.js | 49 ++++++--- .github/scripts/parse-claude-review.js | 2 +- .github/scripts/parse-codex-review.js | 2 +- claude_md/ci_review_prompt.md | 44 ++++++--- claude_md/code_review.md | 109 --------------------- claude_md/manual_code_review.md | 67 +++++++++++++ 6 files changed, 133 insertions(+), 140 deletions(-) delete mode 100644 claude_md/code_review.md create mode 100644 claude_md/manual_code_review.md diff --git a/.github/scripts/build-ai-review-comment.js b/.github/scripts/build-ai-review-comment.js index 4bb81cb368..89e1d3c92b 100644 --- a/.github/scripts/build-ai-review-comment.js +++ b/.github/scripts/build-ai-review-comment.js @@ -5,23 +5,40 @@ // return build({ icon, headerTitle, triggerLine, responseBody, footerLines // }); +// GitHub rejects issue comments longer than this many characters. The model +// response (responseBody) is the only unbounded input, so we trim it to fit +// while keeping the fixed scaffolding (header, trigger, footer) intact. +const MAX_COMMENT_LENGTH = 65536; +const TRUNCATION_NOTE = + '\n\n*[Truncated — full output in execution log artifact]*'; + module.exports = function buildAiReviewComment( {icon, headerTitle, triggerLine, responseBody, footerLines}) { - return [ - `## ${icon} ${headerTitle}`, - '', - triggerLine, - '', - '---', - '', - responseBody, - '', - '---', - '', - '
', - 'ℹ️ About this response', - '', - ...footerLines, - '
', + const assemble = (body) => [`## ${icon} ${headerTitle}`, + '', + triggerLine, + '', + '---', + '', + body, + '', + '---', + '', + '
', + 'ℹ️ About this response', + '', + ...footerLines, + '
', ].join('\n'); + + const body = responseBody || ''; + const comment = assemble(body); + if (comment.length <= MAX_COMMENT_LENGTH) { + return comment; + } + // Drop only as much of the body as needed to fit, then re-assemble so the + // footer/
block is preserved rather than cut off mid-comment. + const overflow = comment.length - MAX_COMMENT_LENGTH; + const keep = Math.max(0, body.length - overflow - TRUNCATION_NOTE.length); + return assemble(body.substring(0, keep) + TRUNCATION_NOTE); }; diff --git a/.github/scripts/parse-claude-review.js b/.github/scripts/parse-claude-review.js index 5f229c6223..b6973c8d15 100644 --- a/.github/scripts/parse-claude-review.js +++ b/.github/scripts/parse-claude-review.js @@ -103,7 +103,7 @@ module.exports = function parseClaude({executionFile, conclusion, meta}) { responseBody, footerLines: [ 'Generated by [Claude Code](https://github.com/anthropics/claude-code-action).', - 'Review methodology: `claude_md/code_review.md`', + 'Review methodology: `claude_md/ci_review_prompt.md`', '', '**Limitations:**', '- Claude may miss context from files not in the diff', diff --git a/.github/scripts/parse-codex-review.js b/.github/scripts/parse-codex-review.js index 59fc20437e..da2bcbe713 100644 --- a/.github/scripts/parse-codex-review.js +++ b/.github/scripts/parse-codex-review.js @@ -83,7 +83,7 @@ module.exports = function parseCodex( responseBody, footerLines: [ 'Generated by Codex CLI.', - 'Review methodology: `claude_md/code_review.md`', + 'Review methodology: `claude_md/ci_review_prompt.md`', '', '**Limitations:**', '- Codex may miss context from files not in the diff', diff --git a/claude_md/ci_review_prompt.md b/claude_md/ci_review_prompt.md index 96ac326130..a2aaec4263 100644 --- a/claude_md/ci_review_prompt.md +++ b/claude_md/ci_review_prompt.md @@ -4,12 +4,19 @@ Thorough multi-agent code review for RocksDB commits following CLAUDE.md guidelines, with structured codebase exploration, inter-agent debate, and verification. -IMPORTANT — Incremental output: After completing EACH major phase (Setup, -Codebase Context, Initial Review, Debate, Synthesis), append your findings -to review-findings.md using the Write tool. This ensures partial results are -saved even if the review is interrupted by the turn limit. After the final -synthesis, write the complete review to review-findings.md, replacing -previous content. Also output the final review as your response text. +IMPORTANT — Where the review goes: Your FINAL RESPONSE TEXT is the review. +The PR comment is generated verbatim from your final response, so the +complete, polished review MUST appear there in full. Do NOT end your turn with +a meta-summary such as "the review has been written to review-findings.md" — +paste the entire review as your response. + +review-findings.md is a RECOVERY BACKSTOP, not the primary output: after +completing EACH major phase (Setup, Codebase Context, Initial Review, Debate, +Synthesis), append your current findings to it with the Write tool, and after +the final synthesis overwrite it with the complete review. This preserves +partial results if the turn limit interrupts the run (a recovery step reads +this file); it does NOT substitute for outputting the full review as your +final response. ## Workflow Steps @@ -466,7 +473,9 @@ Team lead synthesizes the debate into a consensus document: - Include detailed bug analysis (root cause, vulnerable code paths) - Include suggested fixes - Note which findings were debated and the outcome -- Write the complete review to review-findings.md and output as response +- Output the complete review as your final response text (this becomes the PR + comment verbatim), and also overwrite review-findings.md with the identical + content as a recovery backstop **Final report quality rules:** - The final report must be CLEAN and POLISHED. No stream-of-consciousness, @@ -483,9 +492,10 @@ Team lead synthesizes the debate into a consensus document: **REQUIRED output structure (so the PR page stays scrollable):** -The final response (and contents of `review-findings.md`) MUST follow this -exact structure. The summary appears first so reviewers can see HIGH findings -at a glance; everything else is hidden behind a `
` block. +Your final response — which becomes the PR comment verbatim — (and the +identical contents of `review-findings.md`) MUST follow this exact structure. +The summary appears first so reviewers can see HIGH findings at a glance; +everything else is hidden behind a `
` block. ```markdown ## Summary @@ -533,6 +543,9 @@ Rules for this structure: - Do NOT nest a `
` inside another `
`. - Do NOT add any text after the closing `
` — the comment-builder appends its own footer. +- Your final response MUST be the review itself in this structure — never a + meta-summary like "the review has been written to review-findings.md". The + PR comment is generated verbatim from your final response. - If the review is partial (recovery path), still produce the summary block first, and put whatever was salvaged inside the `
` block. @@ -571,9 +584,14 @@ Rules for this structure: - [ ] All callers enumerated with parameter range table ## Output Structure -Write all review artifacts to the working directory root: -- `review-findings.md` — Incremental findings (appended after each phase), - then replaced with the final synthesized review at the end +Your **final response text is the primary output** — it becomes the PR comment +verbatim. The files below are working artifacts and recovery backstops only; +they are NOT a substitute for outputting the full review in your final response. + +Write these artifacts to the working directory root: +- `review-findings.md` — Recovery backstop: incremental findings appended after + each phase, overwritten with the final synthesized review at the end. Used + only if the turn limit interrupts the run. - `context.md` — Codebase context (call chains, invariants) - `findings-*.md` — Per-agent findings (design, correctness, cross-component, invariant-adversary, caller-audit, performance, api, serialization, tests) diff --git a/claude_md/code_review.md b/claude_md/code_review.md deleted file mode 100644 index 9b9721e735..0000000000 --- a/claude_md/code_review.md +++ /dev/null @@ -1,109 +0,0 @@ -# Code Review Skill - -This document defines the methodology for performing thorough, multi-perspective -code reviews on RocksDB changes. It is used both by CI (GitHub Actions) and -local review workflows. - -## Prerequisites - -Before starting a review: -- Read `CLAUDE.md` in the repository root for project-specific guidelines -- Read any files in `claude_md/` for architecture context -- Identify the commit/diff/PR to review and parse the changed files - -## Review Methodology - -Conduct the review from multiple perspectives, then synthesize findings into a -single report. Each perspective catches different classes of bugs. - -### Perspective 1: Codebase Context & Call-Chain Analysis - -Before reviewing the diff itself, build deep context: -- For each changed function/method, trace the call chain UPWARD 3-5 levels. - Who consumes the changed behavior? What invariants do callers rely on? -- Trace DOWNWARD into callees. What side effects do they have? What shared - state do they mutate (counters, sequence numbers, flags, metadata)? -- Identify the "sibling" or "reference" implementation that handles the same - scenario the standard way. Compare behaviors. -- For data written to shared structures (memtable, SST block, cache entry), - find ALL readers and verify their visibility rules match the writer. - -### Perspective 2: Correctness & Edge Cases - -- Thread safety and concurrency (lock ordering, data races) -- Error handling and Status propagation -- Edge cases: empty inputs, overflow, boundary conditions -- Data corruption scenarios -- Behavioral contract changes: do return value semantics match callers? - -### Perspective 3: Cross-Component & Adversarial Analysis - -Check the change in ALL execution contexts: - -| Context | Key difference | -|---------|---------------| -| WritePreparedTxnDB | read_callback_ controls visibility | -| ReadOnly DB / SecondaryInstance | No mutable memtable | -| CompactionService / Remote compaction | Different process | -| User-defined timestamps | Extra key comparison dimension | -| MemPurge | Memtable-to-memtable path | -| BlobDB | Values in blob files, not inline | -| Old snapshots | Seqno far behind current | -| Concurrent writers | Lock-free vs locked paths | -| FIFO / Universal compaction | Different invariants | -| Prefix seek / total order seek | Different iterator behavior | - -When the change claims a property ("logically redundant", "safe because X"), -systematically try to break it: list preconditions, construct counterexamples. - -### Perspective 4: Performance - -RocksDB is a high-performance storage engine. -- Memory allocation on hot paths -- Unnecessary copies (prefer move semantics, Slice, string_view) -- Cache efficiency and data locality -- Loop optimization, branch prediction (LIKELY/UNLIKELY) - -### Perspective 5: API, Compatibility & Testing - -- Public API backwards compatibility -- Serialization format correctness and versioning -- Test coverage for edge cases, failure modes, and system-level interactions - -## How to Explore the Codebase - -Use available tools to explore deeply — do NOT just review the diff in isolation. -The most critical bugs hide at component boundaries. - -- Read the header files for changed implementations -- Search for callers of changed functions (3-5 levels up) -- Read existing tests to understand guaranteed behaviors -- Check related implementations for conventions and patterns - -## Output Format - -NOTE: Post the review **directly in your response** (the PR review comment). -This is the only durable output: a CI review runs in an **ephemeral** GitHub -Actions runner whose workspace is deleted the moment the job ends, so anything -written to a file is unrecoverable by the author. - -### Summary -Brief overall assessment (1-2 sentences). - -### Issues Found -Categorize by severity: -- :red_circle: **Critical**: Must fix (correctness, data corruption, security) -- :yellow_circle: **Suggestion**: Should consider (performance, edge cases) -- :green_circle: **Nitpick**: Minor style issues - -For each issue include: -- File and line reference -- Which review perspective found it -- Description with root cause analysis -- Suggested fix - -### Cross-Component Analysis -Execution context checks and assumption stress-test results. - -### Positive Observations -Good patterns, clever optimizations, or improvements. diff --git a/claude_md/manual_code_review.md b/claude_md/manual_code_review.md new file mode 100644 index 0000000000..e2f7107c8c --- /dev/null +++ b/claude_md/manual_code_review.md @@ -0,0 +1,67 @@ +# Manual (Local) Code Review — Lightweight Wrapper + +## What this is + +A resource-light, **single-reviewer** variant of `claude_md/ci_review_prompt.md` +for running a review by hand in a local Claude Code / Devmate session. It reuses +the full CI review *methodology* but drops the multi-agent orchestration and the +CI-only file-persistence machinery, to approximate the heavyweight result at a +fraction of the cost. + +**Scope: LOCAL manual review only. This file is NOT used by CI.** CI uses +`claude_md/ci_review_prompt.md`. + +## How to use + +Read `claude_md/ci_review_prompt.md` for the complete methodology — the analysis +techniques (caller-chain, callee side-effects, cross-component data consumers, +alternative execution contexts, assumption stress-testing), the per-perspective +reviewer instructions, the review checklist, and the anti-patterns table. + +Then apply that methodology with the OVERRIDES below. **Where this file +conflicts with `ci_review_prompt.md`, this file wins.** + +## Overrides (this is what makes it lightweight) + +### 1. Single reviewer — no team, no subagents +- Do NOT create a team, spawn subagents, or run parallel/background agents. +- Do NOT run the round-robin Debate phase or the Consensus phase. +- Instead, YOU personally apply each reviewer role from `ci_review_prompt.md` + (Design, Correctness, Cross-Component & Adversarial, Invariant Adversary, + Caller-Context, Performance, API/Compat, Serialization, Test Coverage) as + sequential *lenses* in one pass. Spend a few focused minutes per lens. Skip + lenses that plainly don't apply (e.g., Serialization when there is no on-disk + or wire-format change) and note the skip in one line. + +### 2. No intermediate files +- Do NOT write `context.md`, `findings-*.md`, `consensus.md`, or + `review-findings.md`. Keep working notes in your reasoning, not on disk. +- Rationale: those files exist only so CI can recover partial results when an + ephemeral runner hits the turn limit — a local session has neither problem. + `ci_review_prompt.md` already outputs the review as its final response, so + skipping the file loses nothing here. + +### 3. Right-size the effort +- Do the Codebase Context phase, but proportionally: trace the 2-3 most + load-bearing call chains and the highest-risk cross-component data consumers, + rather than exhaustively enumerating every caller 3-5 levels up. +- Prioritize the highest-signal lenses for RocksDB: **Correctness**, + **Cross-Component & Adversarial**, and **Assumption Stress-Testing**. These + catch the bugs that matter most; give them the most attention. +- Escalation clause: if a thread looks dangerous, go deep on that specific + thread even though the overall pass is modest. Modest is the default, not a cap. + +### 4. Identify the review target locally +- Unlike CI, the diff is NOT pre-pasted into this prompt. Determine what to + review from the user's request against the local worktree — e.g. the current + commit (`git show`), uncommitted changes (`git diff` / `git diff --staged`), + a named commit range, or a PR — and read the changed files plus their + surrounding context directly from the repo. When in doubt, include all changes + not yet upstream, as in + `git diff "$(git merge-base upstream/main HEAD 2> /dev/null || git merge-base origin/main HEAD 2> /dev/null)"` + plus any untracked files (`git status`) that seem relevant. + +## Reminder + +The goal is a thorough *single* reviewer, not a 9-agent debate: depth where it +matters, brevity everywhere else.