mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
12 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Xingbo Wang
|
5f1132b884 |
Require DB mutex checks in AI code reviews (#15324)
Summary: CONTEXT: The AI review workflow had generic concurrency and performance checks, but it did not require reviewers to determine whether changed code or transitive callees execute while DBImpl::mutex_ is held. Reviews could therefore miss added contention, blocking work, lifetime errors, or snapshot and lock-ordering violations. PREVIOUS: Review agents could examine DB-mutex changes through general correctness and performance lenses, but there was no explicit call-path audit, before/after mutex-held work comparison, or evidence requirement for regressions. NEW: Every non-trivial review must state whether the DB mutex is affected. When it is affected, reviewers must inspect lock boundaries and correctness invariants, compare CPU and I/O before and after the change, keep unavoidable locked work highly optimized, and require measurements for material regressions. IMPLEMENTATION: Add a DB Mutex Review Perspective covering locking contracts, object lifetime, snapshot boundaries, lock ordering, hidden callee costs, blocking work, validation, and performance evidence. Assign its checks to the correctness and performance reviewers and add required context and review checklist entries. SAFETY: This changes only the AI code-review instructions. It does not change RocksDB runtime behavior, APIs, file formats, or compatibility. Changes that cannot execute under the DB mutex only need to state that the mutex is not affected. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15324 Test Plan: - `git show --check HEAD` - `git diff --check upstream/main...2026_10_05_main` - Documentation-only change; no runtime tests were run. Reviewed By: joshkang97 Differential Revision: D123415306 Pulled By: xingbowang fbshipit-source-id: 0f78e0b1d677edf003cd5dc6ed5ff8123aff4f98 |
||
|
Xingbo Wang
|
14a82121d4 |
Add shared diff and pull request summary guidance (#15261)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15261 Why the change is required: Internal Phabricator diffs and external GitHub pull requests need the same substantive explanation of a RocksDB change. Existing local instructions focus on code generation and review but do not define what a summary must tell reviewers, so summaries can omit prior behavior, safety boundaries, compatibility, or validation evidence and can drift between the two review surfaces. Previous behavior: There was no CLAUDE.md at the internal_repo_rocksdb wrapper root, the RocksDB CLAUDE.md did not point authors to summary guidance, and there was no shared document covering both internal diff and external pull request summaries. New behavior: The wrapper-level CLAUDE.md requires authors to consult a shared summary guideline before writing or updating an internal diff summary. The repository CLAUDE.md applies the same guideline to external pull request summaries. The guideline requires motivation, previous and new behavior, implementation, safety and compatibility boundaries, and test evidence, with RocksDB-specific advice for correctness, persisted formats, concurrency, and performance changes. Examples are separated by access boundary: the internal wrapper contains Phabricator diff examples, while the repository guideline contains publicly accessible GitHub pull request examples. Implementation: Add a concise wrapper-level CLAUDE.md, add claude_md/pull_request_summary_guideline.md as the shared source of truth, and link it from repo/CLAUDE.md. Keep annotated internal diff references in the wrapper-level file and corresponding public pull request references in the repository guideline, including a recent change from Peter Dillinger. Safety boundaries and unsupported cases: This is documentation-only and changes no runtime, API, build, test, or CI behavior. The repository guideline contains no internal links, so external readers do not need Meta access. The guidance is manually followed by authors and agents; it does not mechanically validate summaries, replace reviewer judgment, or require a rigid section layout for small changes. ___ Differential Revision: D121388476 fbshipit-source-id: bb09ab7ba8ef6dc2b2830a42735322890536622f |
||
|
Xingbo Wang
|
5d6f321b70 |
Document behavioral compatibility review requirements (#15218)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15218 Why the change is required: Public and serialized RocksDB inputs can change observable performance or resource behavior without a signature or default change. These changes require deliberate human judgment, clear documentation, and research into downstream production impact. The automated CI review also needs an explicit perspective for identifying gaps in that analysis. Previous behavior: CLAUDE.md described source and ABI compatibility but did not explicitly guide authors or reviewers through behavioral changes to already accepted inputs. The CI review prompt had only a brief API-compatibility checklist item and no shared instructions for evaluating intentionality, release notes, downstream usage, or regression coverage. New behavior: CLAUDE.md states that behavior changes are human judgment calls that must be intentional, documented, and researched. It gives concrete guidance for existing accepted inputs while asking agents and reviewers to call out gaps or ambiguity. The CI review prompt applies the same perspective to every review agent and makes clear that a behavior change is not automatically a defect. Implementation: Revise the Behavioral Compatibility policy in CLAUDE.md, reduce its workflow entry to a concise pointer, add a dedicated Behavioral Compatibility Review Perspective to ci_review_prompt.md, and point the API and compatibility reviewer to that shared section. Safety boundaries and unsupported cases: This is documentation-only and changes no runtime, API, build, or CI execution behavior. The guidance supports human judgment; it does not automatically decide whether a behavior change is acceptable, guarantee exhaustive discovery of downstream consumers, or replace release-note, testing, and production-impact review. Reviewed By: pdillinger Differential Revision: D120344151 fbshipit-source-id: aa5bbb00fbbe167ef21b91b3551868c4d1b4046d |
||
Peter Dillinger
|
ec5106b3b8 |
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 <details> 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 |
||
|
Peter Dillinger
|
fd7399004f |
Add lazy wide-column read API (GetEntityLazy / MultiGetEntityLazy) (#15012)
Summary:
Adds an experimental **lazy wide-column read API** that lets callers enumerate a
wide-column entity and read back only the columns (and, in a later phase, the
byte ranges) they actually need, instead of eagerly materializing every
blob-backed column like `GetEntity()` / `MultiGetEntity()`.
New public entry points (`include/rocksdb/db.h`):
- `DB::GetEntityLazy(read_options, cf, key, LazyWideColumns* result)`
- `DB::MultiGetEntityLazy(read_options, cf, num_keys, keys, LazyWideColumnsBatch* result, statuses, sorted_input)`
Both leave a wide-column entity's **inline** columns materialized up front but
its **blob-backed** columns as *unresolved references*, so blobs for columns
that are never pulled are never read.
### Result types (`include/rocksdb/lazy_wide_columns.h`)
- **`LazyWideColumn`** — the lazy analogue of `WideColumn`: `name()`, `index()`,
`inline_value()` (an `OptSlice`; empty ⇒ unresolved reference),
`is_reference()`, and `logical_size()` (`std::optional<uint64_t>`; known for
inline and uncompressed references, empty for compressed ones). All no-I/O.
- **`LazyWideColumns`** — a move-only result that is **indexable and iterable
like a vector** of `LazyWideColumn` (`size()`, `operator[]`, range `begin/end`).
Unlike `PinnableWideColumns`, it may be used after the producing call returns:
it **pins the SuperVersion** (like an iterator) so referenced blob files / SST
readers stay valid for deferred reads.
- **`LazyWideColumnsBatch`** — the `MultiGetEntityLazy` result; vector-like over
its per-key `LazyWideColumns`.
### Resolution
- **`LazyColumnReadRequest { const LazyWideColumn* column; uint64_t offset;
size_t length; bool force_verify; PinnableSlice* result; Status* status; }`** —
a read names a **typed column pointer**, not an untyped index. Because a
column identifies its owning result (and, for a batch, its owning entity),
`MultiResolve` routes each read correctly and **rejects a null or foreign
column** with `InvalidArgument` on that read's per-read status — cross-result
misuse can't be silently mis-indexed.
- **`LazyWideColumns::MultiResolve` / `LazyWideColumnsBatch::MultiResolve`** take
the same request type (pointer+count and `std::vector` sugar overloads); the
batch form resolves reads spanning any entities of the batch in one call.
- **`ResolveColumn(column, result)` / `ResolveColumnRange(column, offset, length,
result)`** — per-column sugar over a one-entry `MultiResolve`.
- A column's blob is fetched **at most once** over the result's lifetime (bytes
are cached in the result), so repeated reads of the same column reuse that
fetch. Resolution honors the `ReadOptions` captured at `GetEntityLazy()` time
(e.g. `read_tier`, `verify_checksums`); the resolve methods intentionally take
no per-resolve `ReadOptions`.
### Where it works
- Primary, **read-only**, and **secondary** DB instances (each `GetImpl`
override forwards the lazy signal and transfers a SuperVersion pin via the
shared `DBImpl::TransferSuperVersionPin` helper).
- Forwarded through `StackableDB` wrappers (e.g. `TransactionDB`) rather than
falling through to the `NotSupported` base default.
- Requires `max_open_files == -1` (immortal table readers) so same-file
("embedded") references stay resolvable lazily; otherwise the call returns
`InvalidArgument`.
## Implementation notes
- Read path: `GetContext` gains a lazy mode that leaves blob references
unresolved and captures the SST's `SameFileBlobReader`; `Version::Get` forwards
the new (defaulted) `lazy_columns_same_file_reader` out-param.
- `ReadPathBlobResolver` performs on-demand, cached blob resolution bound to the
pinned `Version` + decoded blob references.
- Internal identity is by heap-stable `Rep` pointers, so column/result/batch
pointers stay valid across moves with no move-time fix-ups.
## Scope / current limitations
- Resolves **whole columns**; byte ranges are served by slicing the whole column
for now (so `length`/`offset`/`force_verify` don't yet save I/O or skip
checksums). Single column family. Synchronous. `kBlockCacheTier` on the
originating call yields `Incomplete` on a blob-cache miss.
- User-defined timestamps: entities aren't writable under UDT, so `GetEntityLazy`
is at parity with `GetEntity` (same read-timestamp validation; plain values
return a single default column).
- Planned follow-ups: partial (byte-range) blob reads that skip checksum/cache-
fill, cross-key coalescing, and asynchronous execution.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15012
Test Plan:
- **`db/wide/db_lazy_entity_test.cc` (15 tests, all passing):** enumeration with
no blob I/O; single range and whole-column resolution; `MultiResolve` of many
blobs and mixed ranges/columns (incl. the `std::vector` overload); cross-key
batch resolution; result-outlives-call / pin survives move; unpulled columns
never read; `max_open_files == -1` requirement; `kBlockCacheTier` ⇒
`Incomplete`; foreign/null column ⇒ `InvalidArgument` (single-entity and
batch); UDT parity with `GetEntity`; `StackableDB` forwarding; and read-only
and secondary instances.
- **db_stress:** `GetEntityLazy` / `MultiGetEntityLazy` run as a differential
against eager `GetEntity` / `MultiGetEntity` (gated by `--lazy_entity_read_one_in`,
which only takes effect under `--open_files=-1`), including coverage on
read-only (`--open_read_only_one_in`) and secondary (`--test_secondary`)
instances. Smoke-run (`EXIT=0`, "Verification successful") with integrated blob
files + `--use_put_entity_one_in`.
- `make format-auto` clean; builds `ASSERT_STATUS_CHECKED`-clean.
Reviewed By: xingbowang
Differential Revision: D113792365
Pulled By: pdillinger
fbshipit-source-id: cb73fc845fe0e20ed352d697e552e7b47b2eb9cf
|
||
zaidoon
|
1cec28d82d |
Finish C API code generation (continues #14572) (#14868)
Summary: This continues and finishes **https://github.com/facebook/rocksdb/issues/14572** ("Add semi-automated code generation for RocksDB C API bindings") by xingbowang. The original author is unavailable to finish it, so I've taken it over. **All 13 of the original commits are preserved** (this branch was created from the PR head and builds on top of it — `git log` shows the original `Xingbo Wang` authorship intact); my follow-up work is in the commits prefixed `C API codegen:`. The underlying design is unchanged and is the original author's: hand-written source templates (`tools/c_api_gen/c_base.h` / `c_base.cc`) plus two generators (auto-discovery from the C++ headers + a spec-driven generator) are inlined into a single, self-contained, `generated` `include/rocksdb/c.h` and `db/c.cc`. This grows the public C API by **668 functions** while keeping `c.h` a single includable header with no `-I` requirement (so `bindgen` and other FFI tools keep working unchanged). This branch reconciles the PR with ~4 months of `main` and addresses the outstanding review feedback (clang-tidy bot, the automated code review, the `c.h` self-containedness discussion, and pdillinger's points about `include/rocksdb` hygiene and `generated` marking). ## What changed on top of the original PR ### Reconciled with current `main` - Merged current `main` (conflicts were confined to the generated/test files) and regenerated. Reconciled the 14 C API functions `main` added since the merge-base (e.g. `rocksdb_set_db_options`, the backup-engine rate limiters, `memtable_batch_lookup_optimization`, `optimize_multiget_for_io`, …) and restored 5 enum constants that upstream had added by hand (`rocksdb_txndb_write_policy_*`, `..._index_block_search_type_auto`, `rocksdb_blob_cache_read_byte`). ### Maintainer feedback (pdillinger) - **No non-user-includable files in `include/rocksdb`.** Moved the hand-written templates out of `include/rocksdb/` and `db/` to `tools/c_api_gen/c_base.{h,cc}`. They were `#include`-ing generated fragments, which broke `make check-headers` and was shipped by `make install`. `include/rocksdb/` now contains only the user-facing, self-contained, `generated` `c.h`. - `c.h` / `c.cc` carry the `// generated` marker. ### Backward compatibility (zero ABI break) - The generator derived each wrapper's C type purely from the C++ field, which had silently changed **5 already-shipped signatures** (e.g. `rocksdb_writeoptions_disable_WAL` `int` → `unsigned char`). Added an ABI type-pinning layer (`tools/c_api_gen/abi_type_overrides.json`) so already-shipped functions keep their historical C signature (the body still casts to the real field type). A repo-wide diff against the merge-base now reports **0 ABI drift**. - New `check_api_compatibility.py` gate (wired into CI + `make`) fails on any removed/changed public function **or** removed enum/typedef symbol, vs a reference revision. Intentional changes go in an allowlist with a reason. ### Correctness (from the automated review) - Restored 5 option setters that were declared in `c.h` but **defined nowhere** (link failure for downstream bindings such as `rust-rocksdb`). Added `check_api_completeness.py` (dependency-free; runs in CI + `make`) asserting every declared function has exactly one definition — this is the gate that would have caught it. - `CopyStringVector` now null-checks `malloc`; the WAL filter `std::move`s the `WriteBatch`; the backup exclude-files callback captures by value instead of the wrapper pointer. ### Build / CI robustness - Removed the dead `C_API_CODEGEN_STAMP` Makefile prerequisite (it was a silent no-op). - The `make check` staleness check is now opt-out-able (behind `SKIP_FORMAT_BUCK_CHECKS`) and skips gracefully when `clang++` is unavailable, so `make check` works without the codegen toolchain. CI remains the authoritative gate. - Pinned `clang-format` consistently through `regen_all.py` / `verify_generated_up_to_date.py` (CI uses clang-format-21) so regeneration is byte-reproducible across environments. - Cleared all 20 `clang-tidy` warnings the bot reported on `db/c.cc` changed lines (fixed in the `c_base.cc` template, not the generated output). - Updated the internal Buck `c_test_bin` wrapper to expose generated `c_api_gen/*.inc` fragments as headers, so sandboxed Buck builds can compile `db/c_test.c` after the generated round-trip tests are included. ### Test coverage - Added `gen_roundtrip_tests.py`, which derives **462 set→get→assert round-trip checks across 25 option objects** from the same generated fragments and wires them into `db/c_test.c`. Coverage now tracks the generated surface automatically. ### Docs - Added the `unreleased_history/public_api_changes` note and fixed `claude_md/add_public_api.md`, which still told contributors to hand-edit the now-`generated` `c.h`/`c.cc`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14868 Test Plan: - `make c_test && ./c_test` — **passes**, including the 462 generated round-trip assertions (a successful link also confirms the API is complete). - `python3 tools/c_api_gen/check_api_completeness.py` — all 1737 declared functions defined exactly once. - `python3 tools/c_api_gen/check_api_compatibility.py --ref <release>` — 1070 reference functions + 229 enum/typedef symbols preserved, 0 removed/changed. - `python3 tools/c_api_gen/verify_generated_up_to_date.py` — generated output is stable. - `include/rocksdb/c.h` confirmed self-contained (only `<stdbool.h>`, `<stddef.h>`, `<stdint.h>`). cc xingbowang - `buck2 build --flagfile fbcode//mode/dev fbcode//internal_repo_rocksdb/repo:c_test_bin` — passes. - `buck2 build --flagfile fbcode//mode/dev --config fbcode.arch=aarch64 fbcode//internal_repo_rocksdb/repo:c_test_bin` — passes. Reviewed By: pdillinger Differential Revision: D109149150 Pulled By: xingbowang fbshipit-source-id: 3417375345f360a4c78bdfe27e9850b89d0a226a |
||
|
Xingbo Wang
|
76b8cce2f7 |
Fix spelling of memtable_verify_per_key_checksum_on_seek (#14811)
Summary: - Rename the misspelled `memtable_veirfy_per_key_checksum_on_seek` option and related flags/config keys to `memtable_verify_per_key_checksum_on_seek`. - Update memtable option plumbing, options serialization/logging, db_bench/db_stress/crash-test flags, tests, and the option-addition guide to use the corrected name. - Keep the checksum-on-seek behavior unchanged. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14811 Test Plan: CI Reviewed By: pdillinger Differential Revision: D107382276 Pulled By: xingbowang fbshipit-source-id: 7621dc718d61503b982a7e3f65cc9293a1ad085b |
||
xingbowang
|
cc8d9ea04d |
CI: AI review workflow improvements (#14659)
Summary: - Upgrade the Claude review workflow models and add complexity-based thinking budget selection for the main review run. - Keep AI review comments tied to the reviewed commit and restructure long reviews so high-severity findings stay visible while details live in a collapsible section. - Add early auto-trigger gating plus Codex review/comment workflows, including shared comment-building and parsing helpers. ## Testing - Not run. The branch refresh was a no-op rebase onto current `upstream/main`, and no new code changes were made during this task. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14659 Reviewed By: pdillinger Differential Revision: D102224743 Pulled By: xingbowang fbshipit-source-id: 39c25494062cbcd52d3e85f8a859b5a3907c758e |
||
|
Xingbo Wang
|
0921ec1200 |
Improve Claude Code review: comprehensive prompt, incremental findings, recovery flow (#14507)
Summary: Improves the Claude Code review CI workflow to produce deeper, more reliable reviews. **Motivation:** The previous 30-turn limit caused reviews to silently produce "no output" on complex PRs (e.g. https://github.com/facebook/rocksdb/issues/14477). The review prompt was also too shallow — single-pass with no codebase context phase. **Changes:** **1. Comprehensive multi-agent review prompt** (`claude_md/ci_review_prompt.md`) - 9 specialized review agents: design, correctness, cross-component, invariant-adversary, caller-audit, performance, API, serialization, test coverage - Deep codebase context phase before agents spawn: caller-chain analysis (3-5 levels up), callee side-effect tracing, cross-component data consumer analysis, execution context verification, assumption stress-testing - Inter-agent debate with round-robin critique assignments - Final report quality rules: disproven findings removed, no stream-of-consciousness **2. Incremental findings + recovery flow** - `Write` tool added so Claude saves findings to `review-findings.md` after each phase - If the review hits the turn limit, a recovery step launches a Sonnet session to format partial findings into the standard output - Recovery file existence check prevents crash if recovery step fails - `getLastAssistantText` fallback truncated to 50KB to avoid enormous PR comments **3. Prompts extracted to files** (`claude_md/ci_*.md`) - `ci_review_prompt.md` — full review methodology - `ci_query_prompt.md` — `/claude-query` system prompt - `ci_recovery_prompt.md` — recovery formatting prompt - Secure: checkout is from base branch (main), not PR head **4. `max_turns` increased from 30 to 300** - Orchestrator budget for multi-agent workflow; sub-agents get their own turns - Recovery flow ensures partial results if limit is still hit Pull Request resolved: https://github.com/facebook/rocksdb/pull/14507 Reviewed By: archang19 Differential Revision: D98170111 Pulled By: xingbowang fbshipit-source-id: 390626a53e7a7f91c2d3e91ed4403494532425ed |
||
|
Xingbo Wang
|
77a0e347bb |
Add Claude code review CI workflow (#14480)
Summary: Add GitHub Actions workflows for AI-powered code review on PRs using Claude (Anthropic). Follows the clang-tidy security pattern with two separate workflows for privilege separation. ## Trigger Modes **1. Auto** — Runs after `pr-jobs` workflow completes successfully via `workflow_run`. Safe for fork PRs (runs from default branch, never executes PR code). **2. Manual** — Maintainers comment `/claude-review [focus area]` or `/claude-query <question>` on any PR. Restricted to 15 authorized team members. **3. workflow_dispatch** — For manual testing. ## Security Model (Two-Workflow Separation) Same pattern as clang-tidy: **`claude-review.yml`** (analysis): - Runs Claude with `ANTHROPIC_API_KEY` - Has ONLY `contents: read` — no PR write, no issue write - Saves review markdown + metadata as artifact **`claude-review-comment.yml`** (posting): - Triggers on `workflow_run` completion - Downloads artifact and posts/updates PR comment - Has `pull-requests: write` but never runs AI This separation prevents a crafted PR from tricking Claude into exfiltrating write tokens. ## Review Methodology Review prompt in `claude_md/code_review.md` (shared with local Claude Code reviews). Five perspectives: - Call-chain analysis (3-5 levels up/down) - Correctness & edge cases - Cross-component & adversarial (10 execution contexts) - Performance - API compatibility & test coverage ## Shared Scripts - `.github/scripts/post-pr-comment.js` — Create-or-update PR comment with marker-based dedup. Now used by both clang-tidy and Claude review. - `.github/scripts/parse-claude-review.js` — Parses `claude-code-base-action` execution log into markdown. ## Files Changed | File | Description | |------|-------------| | `.github/workflows/claude-review.yml` | Analysis workflow (476 lines) | | `.github/workflows/claude-review-comment.yml` | Comment posting workflow (146 lines) | | `.github/scripts/post-pr-comment.js` | Shared PR comment utility (57 lines) | | `.github/scripts/parse-claude-review.js` | Execution log parser (78 lines) | | `.github/workflows/clang-tidy-comment.yml` | Updated to use shared script | | `claude_md/code_review.md` | Review methodology (104 lines) | ## Setup Required Add `ANTHROPIC_API_KEY` secret to the repo settings. ## Testing Tested end-to-end on `xingbowang/rocksdb` fork — both auto and manual triggers, artifact upload/download, comment posting, and duplicate detection all verified working. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14480 Reviewed By: omkarhgawde Differential Revision: D97832666 Pulled By: xingbowang fbshipit-source-id: f80c7d8683ac980614dc4ca66c1e545deb3be504 |
||
Hui Xiao
|
300fb8cace |
Add claude md file for deprecated option removal (#14360)
Summary: **Context/Summary:** It's very easy to make mistake in removing deprecated option such as deleting the corresponding entry in type info that breaks backward compatibility or missing tests to test backward compatibility. Example: https://github.com/facebook/rocksdb/pull/14350#discussion_r2834554287. Added a claude md file for that. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14360 Test Plan: In claude code, prompted for deprecated option removal with this claude md file in a repo separated from my previous deprecation efforts as much as possible and received correct change at one try. Reviewed By: pdillinger Differential Revision: D93920127 Pulled By: hx235 fbshipit-source-id: 1977ce7404b0188f84258552b4843e70394a5aea |
||
|
Xingbo Wang
|
92378eb3b8 |
Add CLAUDE.md and optimize tooling for claude code (#14293)
Summary: * Add CLAUDE.md This CLAUDE.md is generated through the analysis of 9,012 commits from the RocksDB repository since 2016. It aggregate the commits into 8 major components based on the component it is changing. It selected top 100 most complex PR based on line of changes to collect the code review feedbacks. For each PR, we collected: • PR title and description to understand the change context • Files changed to identify affected components and measure complexity • Inline code review comments from reviewers • General review summaries and approval/change request feedback The feedback was then categorized by RocksDB component and analyzed for recurring themes, patterns, and best practices. This CLAUDE.md file could be used for guiding code generation and code review. * Optimize tooling for claude code Add make check-progress and format-auto targets for automation Add machine-parseable progress reporting for `make check` to support automated monitoring tools like Claude Code: - Add `build_tools/check_progress.sh` script that outputs JSON progress - Add `make check-progress` target to poll build/test progress - Detects phases: compiling -> linking -> generating -> testing - Reports failed tests with exit codes, signals, and log output - Limits to 10 failures with last 50 lines of output each Also add non-interactive formatting support: - Add `-y` flag to format-diff.sh for auto-apply without prompts - Add `make format-auto` target for CI/automation use Pull Request resolved: https://github.com/facebook/rocksdb/pull/14293 Test Plan: Local build Reviewed By: pdillinger Differential Revision: D92085763 Pulled By: xingbowang fbshipit-source-id: ba122a4ff51087aec5c06bab804edfee34e13880 |