Skip to content

docs(review): add maintainer review playbook and rule bundle - #3854

Draft
liu-cong wants to merge 19 commits into
llm-d:mainfrom
liu-cong:review-playbook
Draft

liu-cong wants to merge 19 commits into
llm-d:mainfrom
liu-cong:review-playbook

Conversation

@liu-cong

@liu-cong liu-cong commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

What type of PR is this?
/kind documentation

What this PR does / why we need it:

What this PR adds

Adds a two-stage review bundle under docs/review/ and a compiled human-readable maintainer playbook in docs/review_playbook.md:

  • Stage 1 (agent-facing non-code rules, docs/review/rules/*.md): 4 compact, metadata-free files containing 17 When:-triggered rules (7,213 B / 1,803 tokens total, with - Accept: exceptions and - Open: bounds). docs/review/playbooks/review_a_pull_request.md routes 100% (540/540) of non-test .go files under cmd/, pkg/, and internal/ to 0 to 2 rule files per PR (~819 rule tokens loaded on average; 0 rule files loaded for test-only, doc-only, build, CI, and self-contained utility diffs).
  • Stage 2 (human reference guide and provenance sidecar): docs/review/provenance.json, docs/review/decisions/ (28 settled maintainer tradeoffs), docs/review/open_questions/ (12 open design questions), docs/review/playbooks/, and docs/review/references/ store PR thread citations, recurrence counts, consequences, and stale_after dates. hack/render_playbook.py compiles Stage-1 rules and provenance into docs/review_playbook.md for human readers while keeping provenance.json and docs/review_playbook.md out of agent context (0 B loaded).
  • Deterministic compiler and CI sync gate (hack/render_playbook.py, make verify-review-playbook): Standard-library Python script wired into make presubmit that verifies byte-for-byte sync between docs/review/ and docs/review_playbook.md, provenance.json schema and 1-to-1 rule coverage (MAX_REPRESENTATIVE_THREADS = 3), rules/index.md section consistency, When: rule prefix enforcement, ASCII-only output, code-span-safe relative links and heading anchors, first-column routing-table glob existence, and 100% production Go file routing coverage. Expired stale_after timestamps emit a non-blocking WARNING: under --check (make verify-review-playbook) and fail under opt-in --check-staleness.
  • Entrypoint updates (AGENTS.md, CONTRIBUTING.md, DEVELOPMENT.md): Establish AGENTS.md as the single home for repository-wide coding, prose, logging (logger.V(logging.DEFAULT).Info(...) for once-per-request operational signals), constant-reuse, and PR rules; document the two-pass additive review flow; and specify same-PR updates when contracts or routed paths change.

Design principle: non-code contracts and two-pass additive review

Everything discoverable from the repository via rg/code search or AGENTS.md belongs in Pass 1 (code and full-repo search first): reviewers inspect the diff, read modified files in full, trace callers/callees/sibling implementations/tests across the checkout, and record all verified Pass-1 code findings before opening any Stage-1 rule file.

Pass 2 (targeted non-code decisions, strictly additive) loads 0 to 2 Stage-1 rule files covering only facts that cannot be derived from local source code alone:

  1. external_wire_and_engine_contracts.md (4 rules, 457 tokens): external vLLM/SGLang wire fields, OpenAI/Anthropic streaming chunk parity, Envoy ext_proc frame ordering, and ingress header stripping.
  2. unencoded_runtime_hazards.md (4 rules, 386 tokens): async channel abandonment, shared map/slice reference mutation across plugins, background goroutine shutdown leaks, and nil-typed interface traps.
  3. governance_and_compatibility_policy.md (4 rules, 430 tokens): config/CLI deprecation vs startup failure, CRD/API compatibility, and raw Deployment/Helm chart parity.
  4. settled_architectural_boundaries.md (5 rules, 530 tokens): EPP vs sidecar responsibilities, plugin state isolation via CycleState, and metric cardinality bounds.

A Stage-1 rule applies only when its When: condition appears in the diff. Matching 0 Stage-1 When: triggers means only that no non-code rule fired, and never drops, suppresses, or downgrades verified Pass-1 code, concurrency, caller/callee, or test defects.

Evaluation: three-round blinded post-cutoff benchmark (48 PRs, 117 maintainer bugs)

Playbooks v2 (12 files, 53 rules, 6,505 tokens) and v3 (4 files, 17 non-code When: rules, 1,803 tokens) were mined strictly from pre-cutoff history (<= 2026-10-26, PR #1..#3659, Issue #1..#2284) with a verified blank-room firewall (0 reads of evaluation data or post-cutoff PRs) and tested on 48 post-cutoff PRs (#3664..#3884, 117 maintainer-verified review findings) across three rounds:

  • Round 1 (12 PRs, 27 maintainer bugs, isolated diff sandbox, 0 repo searches): Compared AGENTS.md Only against the unpruned 53-Rule Playbook v2.
  • Round 2 (30 PRs, 77 maintainer bugs, full-repo search at base_sha): Showed that loading 6.90 files (3,399 rule tokens/PR) from the unpruned 53-rule v2 bundle diluted attention on complex diffs (34/77 vs 38/77 exact maintainer bugs caught, 24 style/test nits, and a 10 wins vs 13 losses blind preference deficit).
  • Round 3 (48 PRs, 117 maintainer bugs, full-repo search + 17-rule pruned non-code Playbook v3): Evaluated With Playbook v3 (<= 2 files / ~819 rule tokens loaded per PR) against AGENTS.md Only on the same 30 benchmark PRs (77 maintainer bugs) plus 18 unseen holdout PRs (40 maintainer bugs).

Did the pruned playbook work, and how well?

  • Reversed every Round-2 regression on the 30-PR benchmark: Cut Stage-1 rule token load by -75.6% (3,399 -> 830 tok/PR), caught more maintainer bugs than both AGENTS.md Only and Playbook v2 (26/77 vs 25/77 exact; 29/77 vs 26/77 including partial catches), eliminated 100% of false positives (0/38), cut minor nit noise by -87.5% vs v2 (3 vs 24) and -57.1% vs AGENTS.md Only (3 vs 7), lifted the share of comments that are real bugs to 92.1% (35/38 vs 81.6% 40/49 without playbook and 69.4% 59/85 in v2), and flipped blind maintainer preference from 10 wins vs 13 losses (43.5% win rate) to 12 wins vs 7 losses (11 ties, 63.2% win rate).
  • Eliminated 100% of false positives and lifted real-bug share to 93.7% across all 48 PRs: Across all 48 post-cutoff PRs (117 maintainer bugs), With Playbook v3 produced 0 false positives out of 63 comments (0.0% vs 8/89 = 9.0% without playbook), cut minor nit noise by -60.0% (4 vs 10), raised the share of comments that are real bugs from 79.8% (71/89) to 93.7% (59/63), and tied overall blind preference (15 wins, 18 ties, 15 losses; 50.0% win rate).
  • Pinpointed and fixed Pass-1 over-conservatism on the 18 unseen holdout PRs: On the 18 unseen holdout PRs (40 maintainer bugs), With Playbook v3 reached 96.0% real-bug share (24/25, 0 FPs, 1 nit vs 77.5% 31/40, 6 FPs, 3 nits without playbook), but approved 10/18 PRs with 0 comments when 0 Stage-1 When: triggers fired (13/40 vs 16/40 exact bugs caught). review_a_pull_request.md and AGENTS.md now lock verified Pass-1 code findings before Pass 2 opens any Stage-1 rule file.
Metric 30-PR Benchmark: Unpruned 53-Rule v2 (Round 2) 30-PR Benchmark: AGENTS.md Only (Round 3) 30-PR Benchmark: Pruned 17-Rule v3 (Round 3) All 48 PRs: AGENTS.md Only All 48 PRs: Pruned 17-Rule v3
Maintainer bugs caught (exact) 34/77 (44.2%, vs 38/77 baseline) 25/77 (32.5%) 26/77 (33.8%, +1 vs baseline) 41/117 (35.0%) 39/117 (33.3%)
Maintainer bugs caught (incl. partial) 36/77 (46.8%, vs 40/77 baseline) 26/77 (33.8%) 29/77 (37.7%, +3 vs baseline) 44/117 (37.6%) 42/117 (35.9%)
Total review comments posted 85 (2.83/PR) 49 (1.63/PR) 38 (1.27/PR, -22.4%) 89 (1.85/PR) 63 (1.31/PR, -29.2%)
Share of comments that are real bugs 69.4% (59/85) 81.6% (40/49) 92.1% (35/38) 79.8% (71/89) 93.7% (59/63)
False positives (wrong claims) 2 (2.4%) 2 (4.1%) 0 (0.0%) 8 (9.0%) 0 (0.0%)
Minor style / nit noise 24 (28.2%) 7 (14.3%) 3 (7.9%, -87.5% vs v2) 10 (11.2%) 4 (6.3%, -60.0%)
Blind preference (Playbook / Tie / Baseline) 10 / 7 / 13 (43.5% win rate) Baseline 12 / 11 / 7 (63.2% win rate) Baseline 15 / 18 / 15 (50.0% win rate)
Stage-1 rule tokens loaded per PR 3,399 tok (6.90/12 files) 0 tok 830 tok (1.87/4 files, -75.6%) 0 tok 819 tok (1.83/4 files)

Concrete Side-by-Side Review Comment Examples (AGENTS.md Only vs With Playbook v3 vs Maintainer)

  1. Catching external wire-contract & shared-memory bugs that AGENTS.md Only missed (#3751 & #3884):
    • On #3751 (check prompt-token parity between prefill and decode):
      • Maintainer (@ahg-g, @roytman): Flagged (1) Anthropic /v1/messages input_tokens excludes cache_read_input_tokens + cache_creation_input_tokens, causing false parity mismatches whenever KV cache transfer works (comment); (2) injecting include_usage: true when the client set stream_options.continuous_usage_stats mutates every streaming chunk (comment); and (3) fieldChoices duplicates response constants across packages (comment).
      • AGENTS.md Only: Posted 4 comments (1 hit on non-object stream_options, 1 partial hit on fieldChoices, 1 SSE newline finding, 1 test nit), missing both Anthropic cached-token summation and vLLM continuous_usage_stats.
      • With Playbook v3 (guided by external_wire_and_engine_contracts.md & settled_architectural_boundaries.md): Posted 3 comments -- all 3/3 (100%) matching the maintainer's exact bugs with 0 nits and 0 false positives ([high] pkg/common/usage/prompt_tokens.go:55: "Sum input_tokens + cache_read_input_tokens + cache_creation_input_tokens..."; [high] pkg/coordinator/steps/decode.go:220: "Check for continuous_usage_stats in stream_options and avoid injecting include_usage..."; [low] pkg/common/usage/prompt_tokens.go:27: "promote FieldChoices to pkg/common/request/constants.go...").
    • On #3884 (release the request body after it is forwarded):
      • Maintainer (@ahg-g, comment): "Replace SchedulingRequest.Body with a new struct that holds only the kept fields, instead of clearing fields on the shared one... A tokenizer still running when the body is forwarded shares this Body. If ReleaseContent runs in between, that second read is a nil dereference in a goroutine, which crashes the EPP process."
      • AGENTS.md Only: Approved with 0 comments, missing the crash.
      • With Playbook v3 (guided by unencoded_runtime_hazards.md Rule 1): Caught the exact unjoined DataProducer goroutine race ([high] pkg/epp/framework/common/request/types.go:175: "Replace r.SchedulingRequest.Body in releaseRequestBody() with a new InferenceRequestBody struct containing only the retained metadata fields... instead of mutating the shared struct in place").
  2. Catching a wire-protocol fallback bug while eliminating a Round-2 false positive (#3706):
    • Maintainer (@ahg-g, comment): Flagged that forward-prompt-token-ids checks backend == "vllm" but still forwards approximate token IDs when token-producer falls back to Go-side legacy /v1/messages or /v1/responses translation, corrupting the prompt on vLLM workers.
    • Round-2 Unpruned 53-Rule Playbook v2: Loaded 11/12 rule files (6,004.50 rule tokens) and emitted 2 false-positive [high] security blockers claiming that internal EPP-to-sidecar kv_transfer_params.prompt_token_ids forwarding allowed external callers to spoof token IDs.
    • Round-3 AGENTS.md Only: Posted 3 comments (1 constant hit, 1 json.RawMessage("null") check, 1 nit), missing the legacy translation fallback bug.
    • Round-3 With Playbook v3 (guided by settled_architectural_boundaries.md Rule 5 & unencoded_runtime_hazards.md): Emitted 0 false positives (recognizing internal EPP-to-sidecar hops as trusted) and caught the exact legacy-translation prompt-corruption bug ([medium] pkg/epp/framework/plugins/requestcontrol/forwardtokens/plugin.go:161: "forwardChat forwards prompt_token_ids even when token-producer (backend: "vllm") fell back to legacy rendering for /v1/messages or /v1/responses").
  3. Eliminating false positives on a clean PR (#3821) & catching Helm chart drift (#3804):
    • On #3821 (Send the prefill data-parallel rank through the P/D sidecar): Maintainer @sagearc approved the Go code with 0 code bugs. AGENTS.md Only blocked the PR (request_changes) with 4 false positives (4/4 = 100% invalid), including claiming connector_p2p.go:71 bypassed a non-existent helper prefillDPRank(r) (when dispatch.go:133 already validated n >= 0) and claiming connector_sglang.go:68 allowed external body spoofing. With Playbook v3 verified dispatch.go caller validation and internal sidecar trust and approved cleanly with 0 false positives and 0 nits.
    • On #3804 (add startup probes to coordinator and EPP Deployments): Guided by governance_and_compatibility_policy.md Rule 4 (config/charts/ Helm and deploy/ Kustomize parity), With Playbook v3 caught the exact maintainer bug (comment) that config/charts/routerlib/templates/_deployment.yaml was missing startupProbe (3/3 = 100% real bugs).
  4. Where With Playbook v3 was too conservative before the Pass-1 retention fix (Holdout #3828, #3883, & #3672):
    • On holdout PRs where 0 Pass-2 Stage-1 When: triggers matched the diff -- such as #3828 (firstMMContinuation skipping non-string identifier check in pkg/kvevents/pool.go:496, comment), #3883 (buf.Bytes() pinning ~2x bytes.Buffer growth slack in RawBody, comment), and #3672 (dropped "non-JSON body" unit test case, comment) -- AGENTS.md Only caught the localized code/test bug while With Playbook v3 discarded its Pass-1 inspection when 0 Pass-2 triggers fired and approved with 0 comments.
    • Fix applied in docs/review/playbooks/review_a_pull_request.md and AGENTS.md: Pass-1 findings are now locked before Pass 2 opens any Stage-1 rule file ("Record all verified Pass-1 code findings before opening any Stage-1 rule file... matching 0 Stage-1 When: triggers means only that no non-code rule fired, and must never drop, suppress, or downgrade verified Pass-1 code, concurrency, caller/callee, or test defects").

Testing

  • Verified bundle validation and deterministic compilation via make verify-review-playbook (python3 hack/render_playbook.py --check) and opt-in freshness checks via python3 hack/render_playbook.py --check --check-staleness.
  • Verified negative validation cases against temporary bundle copies: out-of-sync docs/review_playbook.md, unrouted production .go files under cmd/, pkg/, or internal/, non-matching first-column routing globs, orphan or missing provenance.json rule entries, missing When: rule prefixes, broken relative links or heading anchors, and expired stale_after dates (WARNING: with exit 0 under --check; exit 1 under --check-staleness).
  • Verified make LINT_NEW_ONLY=true lint (golangci-lint and typos) passes with zero issues.

Which issue(s) this PR fixes:
Fixes #

Release note (write NONE if no user-facing change):

NONE

Distill maintainer review feedback into docs/review_playbook.md so reviewers and authors share a single reference checklist across failure classes and project direction. Link the playbook and top review checks from the Code review section of AGENTS.md.

Signed-off-by: Cong Liu <conliu@google.com>
Address review audit findings across docs/review_playbook.md, AGENTS.md, DEVELOPMENT.md, and CONTRIBUTING.md: align code-level claims with StrictDecoder, std-error-handling, metrics subsystems, and OTel TracerScope; deduplicate AGENTS.md and playbook rules; replace non-matching or multi-topic citations with verified single-topic comments; unbundle double-topic rules; restore KV-event sequence gap and dynamic priority-band clusters; and convert Project direction headings to imperative form.

Signed-off-by: Cong Liu <conliu@google.com>
Signed-off-by: Cong Liu <conliu@google.com>
Rules that took a side on an unresolved maintainer debate state only the
agreed invariant and carry the contested part as an Open question line
with one link per position. Rules that contradicted the code (Screener
scope, sidecar watches, E2E ports, make presubmit coverage, image tag
policy, header prefixes, gosec suppressions, models-responder) are
narrowed to what the code and reviewers enforce. Two inverted citations
are replaced or dropped. AGENTS.md states which make targets use host Go.

Signed-off-by: Cong Liu <conliu@google.com>
Add the Open Knowledge Format (OKF) v0.2 review bundle under docs/review/ (12 section Review Rule Set files, 28 settled Decision files, 12 Open Question files, 3 Reference files, and 3 Playbook files) and hack/render_playbook.py so docs/review_playbook.md renders deterministically from the structured bundle.

Signed-off-by: Cong Liu <conliu@google.com>
…bundle

Add a verify-review-playbook target to Makefile (running inside the builder container via python3 hack/render_playbook.py --check), append it to presubmit, and point the AGENTS.md Code review section at docs/review/ as the canonical source and docs/review_playbook.md as the rendered page.

Signed-off-by: Cong Liu <conliu@google.com>
Signed-off-by: Cong Liu <conliu@google.com>
Signed-off-by: Cong Liu <conliu@google.com>
@github-actions github-actions Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/documentation Categorizes issue or PR as related to documentation. area/dev Related to dev tooling, experience, etc. area/docs labels Nov 1, 2026
…idance

Cap Representative review threads at three maintainer citations per rule, prune unreferenced frontmatter sources and footnote definitions, format sources as single-line YAML flow maps, enforce citation and unused-source checks in hack/render_playbook.py, and document context-bounded loading rules.

Signed-off-by: Cong Liu <conliu@google.com>
Ensure trailing [^id]: footnote definitions in concept bodies cannot drift from inline [^id] references.

Signed-off-by: Cong Liu <conliu@google.com>
@github-actions github-actions Bot added kind/documentation Categorizes issue or PR as related to documentation. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Nov 1, 2026
@liu-cong
liu-cong marked this pull request as ready for review November 1, 2026 19:31
@liu-cong
liu-cong requested a review from a team as a code owner November 1, 2026 19:31
@liu-cong

liu-cong commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

@ahg-g @elevran @vMaroon what do you think

@github-actions github-actions Bot added the needs-rebase PR has merge conflicts and needs a rebase label Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

@liu-cong This PR has merge conflicts with main. Please rebase on main and push to resolve them:

gh pr checkout 3854
git fetch origin main
git rebase origin/main
# resolve conflicts
git push --force-with-lease

Signed-off-by: Cong Liu <conliu@google.com>

# Conflicts:
#	Makefile
Align the review playbook's presubmit check list and observability path routing with docs/telemetry.md and the verify-telemetry and verify-review-playbook targets in Makefile.

Signed-off-by: Cong Liu <conliu@google.com>
@github-actions github-actions Bot added kind/documentation Categorizes issue or PR as related to documentation. and removed needs-rebase PR has merge conflicts and needs a rebase kind/documentation Categorizes issue or PR as related to documentation. labels Nov 2, 2026
@ahg-g

ahg-g commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

looks interesting, we can certainly test it, how do we keep it up-to-date?

…-playbook

Reviewers asked how docs/review/ stays current as the codebase moves.
validate_bundle now fails when a backticked repository path, an Applies
to glob, or a diff-path routing table glob matches nothing in the repo,
and when a rules/*.md stale_after timestamp is missing, malformed, or in
the past. The maintenance playbook documents the same-PR, rule-authoring,
and six-month audit loops, and AGENTS.md points contract changes at it.

Signed-off-by: Cong Liu <conliu@google.com>
Signed-off-by: Cong Liu <conliu@google.com>
@github-actions github-actions Bot added kind/documentation Categorizes issue or PR as related to documentation. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Nov 2, 2026
@ahg-g

ahg-g commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Measured the context cost for review agents at 78a1c234 (about 4 bytes per token):

Loaded by an agent Size
AGENTS.md delta (always loaded) +0.9 KB
review_a_pull_request.md ~5 KB
Routed rule files, typical plugin PR (scope, plugin, config, perf, docs, testing) ~70 KB (~18k tokens)
Routed rule files, typical coordinator/sidecar PR ~79 KB (~20k tokens)
All 12 rule files 152 KB (~38k tokens)
Rendered docs/review_playbook.md 55 KB (~14k tokens)

Concerns:

  1. Routing loads more than the full playbook. 44-50% of each rules/*.md file is frontmatter sources: and [^id]: footnotes, so the 3-6 routed files for a normal PR exceed the 55 KB rendered page. Step 3 of review_a_pull_request.md ("skip frontmatter and footnote definitions") cannot be followed by agents, because file-read tools return the whole file.
  2. The per-rule "Consequence if violated" line repeats verbatim across rules, and the "Observed across N pull requests" evidence block does not help judge a diff. Both cost tokens on every review.
  3. The constant-reuse and prose-style rules duplicate AGENTS.md, while AGENTS.md names docs/review/ as canonical. That leaves two sources for the same rule.
  4. stale_after is enforced in make presubmit. Once a rule set expires, every open PR fails presubmit until someone refreshes it.
  5. Every backticked path and Applies to glob is validated, so package moves (for example refactor(epp): merge framework requestcontrol into requesthandling #3830, requestcontrol -> requesthandling) must also edit docs/review/ and regenerate the playbook.

Suggestions:

  • Move provenance (sources:, footnotes, evidence counts) out of the agent-facing files, into a separate provenance file or the generator input.
  • Have the renderer emit per-section files without metadata, and route agents to those.
  • Run the stale_after check as a scheduled job or warning, not in presubmit. Keep path validation for the routing table globs.
  • Start with the 10-15 highest-signal rules and evaluate: run review agents on a set of past PRs with and without the bundle, and compare their findings against what maintainers flagged.

Separate token-dense agent review rules from human provenance and rationale so coding and review agents load only compact matched sections under docs/review/rules/.

Move citations, recurrence counts, per-rule consequences, and stale_after timestamps to docs/review/provenance.json, compile docs/review_playbook.md for human readers, drop five rules covered by AGENTS.md and CI linters, restrict path checks to routing-table globs, and gate stale_after expiry behind --check-staleness.

Signed-off-by: Cong Liu <conliu@google.com>
…data

Signed-off-by: Cong Liu <conliu@google.com>
Ensure diff-path routing covers every production Go package under cmd/,
pkg/, and internal/ so review agents load subsystem rules on core runtime
and common utility diffs, and enforce routing coverage, rules/index.md
consistency, code-span-safe link rewriting, and provenance.json schema
invariants in hack/render_playbook.py. Align AGENTS.md logging levels
with settled V(logging.DEFAULT) guidance and drop duplicate v.Enabled()
prose from observability.md.

Signed-off-by: Cong Liu <conliu@google.com>
@github-actions github-actions Bot added kind/documentation Categorizes issue or PR as related to documentation. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Nov 3, 2026
Comment thread hack/render_playbook.py Fixed
Comment thread hack/render_playbook.py Fixed
Comment thread hack/render_playbook.py Fixed
Eliminate exponential regex backtracking in Markdown code-span, link, and YAML flow-mapping parsing in hack/render_playbook.py by scanning delimiters in a single linear O(n) pass.

Signed-off-by: Cong Liu <conliu@google.com>
@liu-cong

liu-cong commented Nov 3, 2026

Copy link
Copy Markdown
Member Author

Updated in 56073920, 0c599290, and ca39c568. Context size at ca39c568 vs 78a1c234 (at 4 bytes per token):

Loaded by an agent 78a1c234 ca39c568
AGENTS.md delta (always loaded) +0.9 KB +0.9 KB (+885 B)
review_a_pull_request.md 6.6 KB 4.5 KB (~1.1k tokens)
Routed rule files, typical plugin PR (6 files) 70.3 KB (~17.6k tokens) 10.8 KB (~2.7k tokens, -84.6%)
Routed rule files, typical coordinator or sidecar PR (6 files) 78.9 KB (~19.7k tokens) 13.4 KB (~3.4k tokens, -83.0%)
All 12 docs/review/rules/*.md files 152.6 KB (~38.1k tokens) 26.0 KB (~6.5k tokens, -82.9%)
docs/review/provenance.json + docs/review_playbook.md 54.8 KB rendered 0 B loaded by agents (60.8 KB compiled for humans)
  1. Two-stage split and AGENTS.md deduplication (Concerns 1-3). Moved all YAML frontmatter, sources: metadata, [^id] citations, Consequence if violated lines, and Observed across N pull requests blocks out of docs/review/rules/*.md into docs/review/provenance.json, and dropped duplicate rules and clauses covered by AGENTS.md or CI linters. Total Stage-1 rule size dropped by 82.9% (152,579 B -> 26,018 B across 53 deduplicated subsystem rules). AGENTS.md is canonical for repository-wide coding, prose, logging (logger.V(logging.DEFAULT).Info(...) for once-per-request operational signals), constant-reuse, and PR rules, and unverified generated_by metadata was removed. Agents load only AGENTS.md, docs/review/playbooks/review_a_pull_request.md, and the diff-routed docs/review/rules/*.md slices (0 B from provenance.json or docs/review_playbook.md), while hack/render_playbook.py compiles Stage-1 rules, provenance.json, decisions/, and open_questions/ into human-readable docs/review_playbook.md (60,838 B).

  2. Keeping the bundle up to date and presubmit checks (maintain_review_bundle.md, Concerns 4-5).

    • Same-PR sync (make verify-review-playbook in make presubmit): python3 hack/render_playbook.py --check verifies that docs/review_playbook.md matches docs/review/, links, anchors, and provenance.json schemas resolve, first-column routing globs in review_a_pull_request.md match files, and 100% (540/540) of non-test .go files under cmd/, pkg/, and internal/ match at least one routing row (dropping prose backticked-path and Applies to glob checks so package renames only touch docs/review/ when a routed glob moves). AGENTS.md directs PRs that change a documented contract or settle an open question to update docs/review/ and re-render in the same PR.
    • Incremental updates and 6-month freshness audits: Recurring maintainer corrections (>= 3 threads) update a Stage-1 rule file with <= 3 representative threads in provenance.json and a log.md entry. stale_after enforcement was removed from make presubmit (--check prints a non-blocking WARNING: on stderr and exits 0), while strict expiry checks remain opt-in via python3 hack/render_playbook.py --check-staleness for scheduled or manual 6-month audits that prune stale rules and settle open questions.
  3. Blinded post-cutoff evaluation (12 merged PRs after the 2026-10-26 mining cutoff, 27 maintainer review findings). Evaluated AGENTS.md alone (Control) against AGENTS.md plus diff-routed Stage-1 rules (Treatment, ~4.3K mean net extra guidance tokens per PR on the eval snapshot) on exact pre-review commit diffs with a blinded judge. The Stage-1 bundle cut total emitted findings by 22.9% (35 -> 27) and cut unmatched minor or style nits by 45.5% Strict (11 -> 6) and 50.0% Lenient (10 -> 5), raising actionable precision from 68.6% -> 77.8% Strict (+9.2 pp) and 71.4% -> 81.5% Lenient (+10.1 pp) and improving Lenient F1 (36.8% -> 38.8% Micro, 35.4% -> 37.4% Macro; Strict recall 9/27 -> 8/27, Strict Micro F1 29.0% -> 27.7%, Strict Macro F1 33.3% -> 34.5%). We kept the 53 deduplicated subsystem rules so maintainers can inspect the post-cutoff data and decide whether to prune further toward a smaller ~15-rule set.

@liu-cong

liu-cong commented Nov 3, 2026

Copy link
Copy Markdown
Member Author

looks interesting, we can certainly test it, how do we keep it up-to-date?

I plan to ship another PR with a skill pr-review-playbook-refresher. I will schedule a periodical run, but anyone can use the skill to trigger a refresh.

@github-actions github-actions Bot added kind/documentation Categorizes issue or PR as related to documentation. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Nov 3, 2026
@liu-cong
liu-cong marked this pull request as draft November 3, 2026 18:52
@liu-cong

liu-cong commented Nov 3, 2026

Copy link
Copy Markdown
Member Author

@ahg-g I marked this Draft to do more thorough testing of this approach.

… review

Signed-off-by: Cong Liu <conliu@google.com>
@liu-cong

liu-cong commented Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Updated docs/review/ to Playbook v3 based on a three-round blinded A/B benchmark across 48 post-cutoff PRs (#3664..#3884, 117 maintainer-verified review findings, with 0 overlap against the <= 2026-10-26 mining corpus):

1. What changed in Playbook v3

  • Pruned Stage-1 rules by 72.3% (12 files / 53 rules / 26.0 KB -> 4 non-code files / 17 When:-triggered rules / 7.2 KB, -75.6% fewer rule tokens loaded per PR): Removed 36 rules that restated code-discoverable patterns, Go idioms, or generic unit-testing advice already visible via rg/code search or AGENTS.md. Retained only 4 Stage-1 files encoding contracts invisible in local source code (external_wire_and_engine_contracts.md, unencoded_runtime_hazards.md, governance_and_compatibility_policy.md, and settled_architectural_boundaries.md).
  • Two-Pass Additive Review Protocol (docs/review/playbooks/review_a_pull_request.md, AGENTS.md):
    • Pass 1 (code and full-repo search first): Inspect the diff, read modified files in full, search callers/callees/tests across the repository, and record all verified Pass-1 code findings before opening any Stage-1 rule file.
    • Pass 2 (targeted non-code decisions, strictly additive): Route touched paths to 0 to 2 Stage-1 rule files (~819 rule tokens/PR; 0 rule files for test-only, doc-only, build, CI, and self-contained utility diffs). Matching 0 Stage-1 When: triggers means only that no non-code rule fired and must never drop or suppress verified Pass-1 code defects.

2. Three-Round Benchmark Summary (48 Post-Cutoff PRs, 117 Maintainer Bugs)

Metric 30-PR Benchmark: Unpruned 53-Rule v2 (Round 2) 30-PR Benchmark: AGENTS.md Only (Round 3) 30-PR Benchmark: Pruned 17-Rule v3 (Round 3) All 48 PRs: AGENTS.md Only All 48 PRs: Pruned 17-Rule v3
Maintainer bugs caught (exact) 34/77 (44.2%, vs 38/77 baseline) 25/77 (32.5%) 26/77 (33.8%, +1 vs baseline) 41/117 (35.0%) 39/117 (33.3%)
Maintainer bugs caught (incl. partial) 36/77 (46.8%, vs 40/77 baseline) 26/77 (33.8%) 29/77 (37.7%, +3 vs baseline) 44/117 (37.6%) 42/117 (35.9%)
Total review comments posted 85 (2.83/PR) 49 (1.63/PR) 38 (1.27/PR, -22.4%) 89 (1.85/PR) 63 (1.31/PR, -29.2%)
Share of comments that are real bugs 69.4% (59/85) 81.6% (40/49) 92.1% (35/38) 79.8% (71/89) 93.7% (59/63)
False positives (wrong claims) 2 (2.4%) 2 (4.1%) 0 (0.0%) 8 (9.0%) 0 (0.0%)
Minor style / nit noise 24 (28.2%) 7 (14.3%) 3 (7.9%, -87.5% vs v2) 10 (11.2%) 4 (6.3%, -60.0%)
Blind preference (Playbook / Tie / Baseline) 10 / 7 / 13 (43.5% win rate) Baseline 12 / 11 / 7 (63.2% win rate) Baseline 15 / 18 / 15 (50.0% win rate)
Stage-1 rule tokens loaded per PR 3,399 tok (6.90/12 files) 0 tok 830 tok (1.87/4 files, -75.6%) 0 tok 819 tok (1.83/4 files)

3. Concrete Side-by-Side Review Comment Examples (AGENTS.md Only vs With Playbook v3 vs Maintainer)

  1. Catching external wire-contract & shared-memory bugs that AGENTS.md Only missed (#3751 & #3884):
    • On #3751 (check prompt-token parity between prefill and decode):
      • Maintainer (@ahg-g, @roytman): Flagged (1) Anthropic /v1/messages input_tokens excludes cache_read_input_tokens + cache_creation_input_tokens, causing false parity mismatches whenever KV cache transfer works (comment); (2) injecting include_usage: true when the client set stream_options.continuous_usage_stats mutates every streaming chunk (comment); and (3) fieldChoices duplicates response constants across packages (comment).
      • AGENTS.md Only: Posted 4 comments (1 hit on non-object stream_options, 1 partial hit on fieldChoices, 1 SSE newline finding, 1 test nit), missing both Anthropic cached-token summation and vLLM continuous_usage_stats.
      • With Playbook v3 (guided by external_wire_and_engine_contracts.md & settled_architectural_boundaries.md): Posted 3 comments -- all 3/3 (100%) matching the maintainer's exact bugs with 0 nits and 0 false positives ([high] pkg/common/usage/prompt_tokens.go:55: "Sum input_tokens + cache_read_input_tokens + cache_creation_input_tokens..."; [high] pkg/coordinator/steps/decode.go:220: "Check for continuous_usage_stats in stream_options and avoid injecting include_usage..."; [low] pkg/common/usage/prompt_tokens.go:27: "promote FieldChoices to pkg/common/request/constants.go...").
    • On #3884 (release the request body after it is forwarded):
      • Maintainer (@ahg-g, comment): "Replace SchedulingRequest.Body with a new struct that holds only the kept fields, instead of clearing fields on the shared one... A tokenizer still running when the body is forwarded shares this Body. If ReleaseContent runs in between, that second read is a nil dereference in a goroutine, which crashes the EPP process."
      • AGENTS.md Only: Approved with 0 comments, missing the crash.
      • With Playbook v3 (guided by unencoded_runtime_hazards.md Rule 1): Caught the exact unjoined DataProducer goroutine race ([high] pkg/epp/framework/common/request/types.go:175: "Replace r.SchedulingRequest.Body in releaseRequestBody() with a new InferenceRequestBody struct containing only the retained metadata fields... instead of mutating the shared struct in place").
  2. Catching a wire-protocol fallback bug while eliminating a Round-2 false positive (#3706):
    • Maintainer (@ahg-g, comment): Flagged that forward-prompt-token-ids checks backend == "vllm" but still forwards approximate token IDs when token-producer falls back to Go-side legacy /v1/messages or /v1/responses translation, corrupting the prompt on vLLM workers.
    • Round-2 Unpruned 53-Rule Playbook v2: Loaded 11/12 rule files (6,004.50 rule tokens) and emitted 2 false-positive [high] security blockers claiming that internal EPP-to-sidecar kv_transfer_params.prompt_token_ids forwarding allowed external callers to spoof token IDs.
    • Round-3 AGENTS.md Only: Posted 3 comments (1 constant hit, 1 json.RawMessage("null") check, 1 nit), missing the legacy translation fallback bug.
    • Round-3 With Playbook v3 (guided by settled_architectural_boundaries.md Rule 5 & unencoded_runtime_hazards.md): Emitted 0 false positives (recognizing internal EPP-to-sidecar hops as trusted) and caught the exact legacy-translation prompt-corruption bug ([medium] pkg/epp/framework/plugins/requestcontrol/forwardtokens/plugin.go:161: "forwardChat forwards prompt_token_ids even when token-producer (backend: "vllm") fell back to legacy rendering for /v1/messages or /v1/responses").
  3. Eliminating false positives on a clean PR (#3821) & catching Helm chart drift (#3804):
    • On #3821 (Send the prefill data-parallel rank through the P/D sidecar): Maintainer @sagearc approved the Go code with 0 code bugs. AGENTS.md Only blocked the PR (request_changes) with 4 false positives (4/4 = 100% invalid), including claiming connector_p2p.go:71 bypassed a non-existent helper prefillDPRank(r) (when dispatch.go:133 already validated n >= 0) and claiming connector_sglang.go:68 allowed external body spoofing. With Playbook v3 verified dispatch.go caller validation and internal sidecar trust and approved cleanly with 0 false positives and 0 nits.
    • On #3804 (add startup probes to coordinator and EPP Deployments): Guided by governance_and_compatibility_policy.md Rule 4 (config/charts/ Helm and deploy/ Kustomize parity), With Playbook v3 caught the exact maintainer bug (comment) that config/charts/routerlib/templates/_deployment.yaml was missing startupProbe (3/3 = 100% real bugs).
  4. Where With Playbook v3 was too conservative before the Pass-1 retention fix (Holdout #3828, #3883, & #3672):
    • On holdout PRs where 0 Pass-2 Stage-1 When: triggers matched the diff -- such as #3828 (firstMMContinuation skipping non-string identifier check in pkg/kvevents/pool.go:496, comment), #3883 (buf.Bytes() pinning ~2x bytes.Buffer growth slack in RawBody, comment), and #3672 (dropped "non-JSON body" unit test case, comment) -- AGENTS.md Only caught the localized code/test bug while With Playbook v3 discarded its Pass-1 inspection when 0 Pass-2 triggers fired and approved with 0 comments.
    • Fix applied in docs/review/playbooks/review_a_pull_request.md and AGENTS.md: Pass-1 findings are now locked before Pass 2 opens any Stage-1 rule file ("Record all verified Pass-1 code findings before opening any Stage-1 rule file... matching 0 Stage-1 When: triggers means only that no non-code rule fired, and must never drop, suppress, or downgrade verified Pass-1 code, concurrency, caller/callee, or test defects").

@github-actions github-actions Bot added kind/documentation Categorizes issue or PR as related to documentation. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/dev Related to dev tooling, experience, etc. area/docs kind/documentation Categorizes issue or PR as related to documentation. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants