Add pr-context-synthesis skill#4374
Conversation
97e8570 to
59935ce
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new Claude command for PR synthesis and a corresponding skill definition. The changes establish a structured process for generating concise summaries of pull requests by fetching data from GitHub. Feedback highlights a brittle approach to identifying linked issues, suggesting the use of GitHub's native API fields instead. Additionally, a contradiction was identified between the command's behavior and the skill's rules regarding how to handle drift between the PR description and the actual diff.
| Parse the PR body for `Fixes #N` / `Closes #N` / `Resolves #N` (case-insensitive) and fetch each linked issue: | ||
|
|
||
| ```bash | ||
| gh issue view <N> -R <owner>/<repo> --json title,body,labels,state | ||
| ``` |
There was a problem hiding this comment.
Manually parsing the PR body for linked issues using keywords like Fixes #N is brittle. A more robust approach is to leverage GitHub's built-in issue linking by using the closingIssuesReferences field from the gh pr view API. This ensures you are using the same mechanism that GitHub uses to link PRs and issues.
To implement this, you'll need to add closingIssuesReferences to the --json fields in the gh pr view call on line 32. Then, you can iterate over the structured data to get issue details.
| Parse the PR body for `Fixes #N` / `Closes #N` / `Resolves #N` (case-insensitive) and fetch each linked issue: | |
| ```bash | |
| gh issue view <N> -R <owner>/<repo> --json title,body,labels,state | |
| ``` | |
| gh issue view <issue.number> -R <issue.repository.owner>/<issue.repository.name> --json title,body,labels,state |
| ## Rules | ||
|
|
||
| 1. **Synthesize, don't copy-paste.** If `<pr_text>` is five words, say so plainly: *"description is minimal; intent inferred from diff"*. Don't pad to look thorough. | ||
| 2. **Watch for description-vs-diff drift.** `<pr_text>` must describe `<diff_summary>`'s *current* state, not the author's iteration history. If a claim is no longer true of the diff, raise a finding with `Action: update the PR description to match the current diff`. Do **not** flag the absence of a changelog of removed/superseded behaviour — that belongs in commit history, not the description. |
There was a problem hiding this comment.
There is a contradiction between this rule and the implementation described in .claude/commands/pr-synthesis.md.
This file says to raise a finding with Action: update the PR description....
However, .claude/commands/pr-synthesis.md (line 57) states that in case of a drift, the skill should note it in the synthesis as "description claims X; diff shows Y".
The latter seems more appropriate for a synthesis skill, which should report facts rather than take actions. The consumer of the synthesis can then decide whether to raise a finding. I recommend aligning this rule with the implementation.
| 2. **Watch for description-vs-diff drift.** `<pr_text>` must describe `<diff_summary>`'s *current* state, not the author's iteration history. If a claim is no longer true of the diff, raise a finding with `Action: update the PR description to match the current diff`. Do **not** flag the absence of a changelog of removed/superseded behaviour — that belongs in commit history, not the description. | |
| 2. Watch for description-vs-diff drift. <pr_text> must describe <diff_summary>'s current state, not the author's iteration history. If a claim is no longer true of the diff, note it in the synthesis (e.g., "description claims X; diff shows Y"). Do not flag the absence of a changelog of removed/superseded behaviour — that belongs in commit history, not the description. |
87b5610 to
8af2427
Compare
1db236d to
b243671
Compare
What this is
A primitive for producing a tight 1–3 paragraph what / why / how synthesis of a single PR (or PR-shaped change). Stays anchored to the actual diff and refuses vague verbs.
Ships with
/pr-synthesis <N|owner/repo#N|url>, a slash command that fetches PR metadata, linked issue, and diff and prints the synthesis verbatim.Why split out
Extracted from #4351 because the synthesis primitive is reusable beyond a single review report — review and incident-investigation flows both want the same per-PR summary shape. Splitting keeps the contract small.
Files
docs/skills/pr-context-synthesis.md— rules + output shape..claude/commands/pr-synthesis.md— slash command wrapping the procedure.Worked examples
Example 1: PR #4243
Boundary case: just over the gate. Generated bindings dominate the line count; the substantive change is a 275-line estimator + a 236-line e2e test.
Adds a new
Eip4626native price estimator to the price-estimation stack atcrates/price-estimation/src/native/eip4626.rs. The estimator unwraps an EIP-4626 vault token by callingasset()andconvertToAssets(10^decimals)on-chain, then delegates pricing of the underlying token to the next estimator in the configured stage. New contract bindings (IERC4626,MockERC4626Wrapper) ship as pre-generated alloy crates; an e2e forked-mainnet test covers a single vault and a recursive vault chain.Vault tokens like sDAI often lack direct DEX liquidity, which makes the existing native-price estimators fall over. This change lets the orderbook quote vault tokens by reducing them to their underlying. (No linked issue; intent inferred from the PR body and the new test names.)
Mechanically: a
Mutex<HashSet<Address>>negative cache short-circuits subsequent requests for tokens whoseasset()reverts (cleared on restart); each on-chain RPC is bounded bytokio::time::timeout, with the leftover budget forwarded to the inner estimator so total wall-clock stays inside the caller's timeout — important for nested vaults. A configurabledepthparameter (default 1) controls recursion, wired infactory.rs::create_native_estimator. Config deserialisation rejects stages whereEip4626is the last entry, since it must be followed by another estimator to price the underlying.Diff summarised at file-scope only — total +4,411 / −21 across 24 files exceeds the 2k full-diff threshold. Generated bindings (
ierc4626/src/lib.rs+648,mockerc4626wrapper/src/lib.rs+2,768) and theMockERC4626Wrapper.jsonartifact (+240) account for ~83% of additions; the substantive Rust changes are the 275-line estimator, 236-line e2e test, and 14 small wiring edits.Example 2: PR #4217
Stress case: two orders of magnitude over the gate. Demonstrates that the per-file bucket summary is genuinely more useful than the full diff would be — 96% of the lines are mechanical codegen output, and the substantive change hides in a 116-file
+213/−1,531workspace bucket of import-path edits.Moves the
contractscrate fromcrates/contractsto a standalonecontracts/directory at repo root, and replaces itsbuild.rsruntime code-generation with 148 pre-generated per-contract Alloy binding crates undercontracts/generated/contracts-generated/. A newcontracts-facadecrate re-exports the bindings so consumers see a single import surface. All 116 workspace crates that previously depended oncrates/contractsare rewired to import from the new generated crates. Codegen still exists, but is now a separatecargo run -p contractsstep rather than an implicit cost on every workspace build.Driver:
build.rswas the longest critical-path step in the workspace build. The PR description's flame graphs claim ~15 s shaved off cold builds (from ~60 s to ~45 s) and visibly higher CPU utilisation since the build no longer serialises behind a single codegen blocker. Pre-generated bindings are also independently version-controllable and reviewable — historically the only way to inspect a binding diff was to rerun the build.Diff summarised at file-scope only — +375,175 / −1,654 across 384 files is two orders of magnitude over the 2k full-diff threshold. Bucketed: generated bindings 148 files / +292,618 (mechanical, alloy-codegen output); contract artifacts 74 files / +7,278 (JSON ABI moves); workspace crates 116 files / +213 / −1,531 (overwhelmingly import-path edits and removal of the old
crates/contracts/); Cargo.lock 3 files / +7,747; contracts tooling 38 files / +1,192. Almost everything except the 116 workspace-crate imports is mechanically generated or moved.Reviewing this in isolation
The skill body is self-contained. The intro mentions consumers (
/review-pr, ad-hoc summaries, future incident walks) but doesn't link to anything that doesn't exist onmain. Rules, Shape, and Example stand on their own.How to try it