From 841a9ed9b4728f544d162a463aba0e61c2b98a08 Mon Sep 17 00:00:00 2001 From: Aryan Godara Date: Fri, 1 May 2026 14:32:07 +0530 Subject: [PATCH 1/4] Add git-blame-historic-context skill --- docs/skills/git-blame-historic-context.md | 42 +++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 docs/skills/git-blame-historic-context.md diff --git a/docs/skills/git-blame-historic-context.md b/docs/skills/git-blame-historic-context.md new file mode 100644 index 0000000000..bcac34cac6 --- /dev/null +++ b/docs/skills/git-blame-historic-context.md @@ -0,0 +1,42 @@ +# Skill — `git blame` for historic context + +Use before flagging code that looks unusual, redundant, accidental, or "easy to clean up". Often that code looks weird because it had to. + +## Procedure + +```bash +git blame -L , -- # who/what/when + +# cowprotocol/services squash-merges. Most blames point at one commit whose +# subject ends with "(#NNNN)" — extract the PR number, then pivot to the +# PR conversation, which is usually richer than the commit body alone. +git log -1 --format='%s%n%b' +gh pr view -R / +``` + +## Decision + +Promote what blame reveals into the finding's Explanation, then weight the finding: + +- **Strengthens** — surrounding code was added recently for a reason the diff now contradicts. Keep / raise severity. +- **Weakens** — the originating PR explains *why* the shape is unusual (deliberate workaround, perf fix, cross-version compat). Soften, or pivot from `Action:` to `Question:`. Example: a two-line guard looked over-defensive; blame showed it was a hot-patch lifted from prod logs — Medium → Question, asking whether the failure mode still applies. +- **Inverts** — flagging this would ask the author to undo a hard-won fix. Drop the finding. Example: a `Some(_) =>` arm looked redundant; blame revealed it was added months ago to swallow a panic on an edge case the diff was about to remove. Dropped, with a note that the panic is back. + +## Edge cases + +- **Merge commit, not squash** — rare here; inspect with `git log -1 --format='%s%n%b' ` and walk parents (`^1`, `^2`) to find the commit that actually authored the line. +- **Same author as the PR under review, recent** — context is fresh; ask the author directly in the review thread instead of synthesising blame. +- **Refactor moved code wholesale** — surface blame points at the move, not the originating fix. Use `git blame -w -C -C -C -L , -- ` (whitespace-insensitive, copy-detection) to recover the real authoring commit. +- **Vendored / generated / contract-binding code** — blame the generator's input (upstream config, source `.sol`, codegen template). Skip if the surface is a JSON ABI or lockfile. + +## When to skip + +- Lines entirely new in the diff under review (no history yet). +- Pure additions of new symbols (nothing to blame). +- Generated code where the input lives elsewhere — blame the input instead. + +## Used by + +- [`COW_PR_REVIEW_SKILL.md`](../COW_PR_REVIEW_SKILL.md) §6 — before flagging unusual-looking code. +- [`COW_ORDER_DEBUG_SKILL.md`](../COW_ORDER_DEBUG_SKILL.md) — when investigating *"why is this check here?"* during order debugging. +- Ad-hoc code investigations where a line of code prompts *"this looks accidental"*. From c58803a05a916d21f6a6a71bbe3756c0b2a1ccf1 Mon Sep 17 00:00:00 2001 From: Aryan Godara Date: Tue, 5 May 2026 18:48:23 +0530 Subject: [PATCH 2/4] feat: add /blame-context slash command and worked example --- .claude/commands/blame-context.md | 52 +++++++++++++++++++++++ docs/skills/git-blame-historic-context.md | 30 +++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 .claude/commands/blame-context.md diff --git a/.claude/commands/blame-context.md b/.claude/commands/blame-context.md new file mode 100644 index 0000000000..bfddef651c --- /dev/null +++ b/.claude/commands/blame-context.md @@ -0,0 +1,52 @@ +--- +description: Recover historic context for a file:line via `git blame` plus the squash-PR pivot. Wraps `docs/skills/git-blame-historic-context.md` end-to-end and prints the strengthens / weakens / inverts decision so the caller can weight a finding before flagging suspicious-looking code. Read-only. +--- + +Look up historic context for: $ARGUMENTS + +## Parse $ARGUMENTS + +Accept any of: + +- `path:line` — single line. e.g. `crates/driver/src/.../settlement.rs:444` +- `path:start-end` — line range. e.g. `crates/driver/src/.../settlement.rs:434-447` +- `path line` or `path start-end` — space-separated form (handy when paths contain `:`). + +If `$ARGUMENTS` is empty or unparseable, print the usage block and abort: + +``` +Usage: /blame-context : + /blame-context :- +``` + +## Procedure + +Follow `docs/skills/git-blame-historic-context.md` end-to-end. Concretely: + +1. `git blame -L , -- ` to find the originating commit. +2. If the surface looks like a wholesale move/refactor (same author across many lines, recent date, identical hashes), retry with `git blame -w -C -C -C -L , -- ` to recover the real authoring commit. +3. `git log -1 --format='%s%n%b' ` for the commit body. +4. If the subject ends with `(#NNNN)`, pivot to the PR conversation: `gh pr view -R /`. The PR body is usually richer than the squash commit alone. + +Then print the report below. + +## Output + +``` +─── Blame for : + + +─── Originating commit / PR + + +─── Decision + the suspicion that this code is unusual. + +Action: +``` + +## Rules + +- **Read-only.** `git blame`, `git log`, `git show`, `gh pr view`, `gh api` GET-only. No `git commit`, no `git checkout`, no mutating `gh api` verbs. +- **Don't comment on the PR.** Don't edit files. The caller owns what to do with the decision — this command just supplies the evidence. +- **Don't invent a decision.** If blame surfaces nothing useful (line is brand new, generator-output, vendored), print `Decision: insufficient signal` and explain why. diff --git a/docs/skills/git-blame-historic-context.md b/docs/skills/git-blame-historic-context.md index bcac34cac6..1a13d8990c 100644 --- a/docs/skills/git-blame-historic-context.md +++ b/docs/skills/git-blame-historic-context.md @@ -2,6 +2,36 @@ Use before flagging code that looks unusual, redundant, accidental, or "easy to clean up". Often that code looks weird because it had to. +## How to invoke + +Two ways: + +- **Procedurally** — follow the steps below when you encounter unusual-looking code in any context (PR review, order-debug session, ad-hoc *"why is this here?"*). +- **Via slash command** — `/blame-context :` wraps the procedure end-to-end and prints the strengthens / weakens / inverts decision. Useful when you want a one-shot answer rather than walking the procedure manually. + +## Example + +A reviewer sees this in `crates/driver/src/domain/competition/solution/settlement.rs:444`: + +```rust +let max_gas = eth::Gas(block_limit.0 / eth::U256::from(2)); +``` + +`/2` looks arbitrary — a reviewer might be tempted to flag it as a magic number. Run the procedure first: + +```bash +git blame -L 444,444 -- crates/driver/src/domain/competition/solution/settlement.rs +# → a4ee76aae3 Felix Leupold 2024-03-18 ... + +git log -1 --format='%s%n%b' a4ee76aae3 +# → subject ends with `(#NNNN)`; pivot to the PR +gh pr view -R cowprotocol/services +# → body: "block builders' default algorithm picks tx whose gas limit +# fits remaining space; leave headroom for inclusion." +``` + +Decision: **weakens** the "magic number" suspicion — the constant has documented inclusion-economics rationale. Drop the finding, or downgrade to a `Question:` confirming the rationale still applies on the chain in question. + ## Procedure ```bash From 8cb5c1d5978878dd3eaa973866aa624ef69111fb Mon Sep 17 00:00:00 2001 From: Aryan Godara Date: Tue, 5 May 2026 19:43:48 +0530 Subject: [PATCH 3/4] fix: drop unused -R flag from gh pr view --- .claude/commands/blame-context.md | 2 +- docs/skills/git-blame-historic-context.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/commands/blame-context.md b/.claude/commands/blame-context.md index bfddef651c..41e0a3a8cb 100644 --- a/.claude/commands/blame-context.md +++ b/.claude/commands/blame-context.md @@ -26,7 +26,7 @@ Follow `docs/skills/git-blame-historic-context.md` end-to-end. Concretely: 1. `git blame -L , -- ` to find the originating commit. 2. If the surface looks like a wholesale move/refactor (same author across many lines, recent date, identical hashes), retry with `git blame -w -C -C -C -L , -- ` to recover the real authoring commit. 3. `git log -1 --format='%s%n%b' ` for the commit body. -4. If the subject ends with `(#NNNN)`, pivot to the PR conversation: `gh pr view -R /`. The PR body is usually richer than the squash commit alone. +4. If the subject ends with `(#NNNN)`, pivot to the PR conversation: `gh pr view ` (the gh CLI infers the repo from the working directory). The PR body is usually richer than the squash commit alone. Then print the report below. diff --git a/docs/skills/git-blame-historic-context.md b/docs/skills/git-blame-historic-context.md index 1a13d8990c..a9da2a9db1 100644 --- a/docs/skills/git-blame-historic-context.md +++ b/docs/skills/git-blame-historic-context.md @@ -25,7 +25,7 @@ git blame -L 444,444 -- crates/driver/src/domain/competition/solution/settlement git log -1 --format='%s%n%b' a4ee76aae3 # → subject ends with `(#NNNN)`; pivot to the PR -gh pr view -R cowprotocol/services +gh pr view # → body: "block builders' default algorithm picks tx whose gas limit # fits remaining space; leave headroom for inclusion." ``` @@ -41,7 +41,7 @@ git blame -L , -- # who/what/when # subject ends with "(#NNNN)" — extract the PR number, then pivot to the # PR conversation, which is usually richer than the commit body alone. git log -1 --format='%s%n%b' -gh pr view -R / +gh pr view ``` ## Decision From 4cc12016e0f9c67f130acf82ed67995b83fee748 Mon Sep 17 00:00:00 2001 From: Aryan Godara Date: Wed, 6 May 2026 21:09:56 +0530 Subject: [PATCH 4/4] address Jose review on git-blame skill: reframe 'when to invoke', add second worked example, typo + cosmetic fixes --- docs/skills/git-blame-historic-context.md | 49 ++++++++++++++++++++--- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/docs/skills/git-blame-historic-context.md b/docs/skills/git-blame-historic-context.md index a9da2a9db1..6982fecfe3 100644 --- a/docs/skills/git-blame-historic-context.md +++ b/docs/skills/git-blame-historic-context.md @@ -1,15 +1,21 @@ # Skill — `git blame` for historic context -Use before flagging code that looks unusual, redundant, accidental, or "easy to clean up". Often that code looks weird because it had to. +Use before flagging code that looks unusual, redundant, accidental, or "easy to clean up". Often, certain decisions led to sub-optimal-looking code, and those decisions are codified in git history rather than the code itself. + +## When to invoke + +Not on first sighting. Build the full picture first — read the change / PR / file end-to-end and *collect* lines that look off during the pass — then run blame on each candidate. Code that looks weird is often being partially fixed, moved, or replaced by the diff you're currently reading; suspicion based on a single line, before you've seen its neighbours and the rest of the diff, is unreliable. ## How to invoke Two ways: -- **Procedurally** — follow the steps below when you encounter unusual-looking code in any context (PR review, order-debug session, ad-hoc *"why is this here?"*). +- **Procedurally** — follow the steps below. - **Via slash command** — `/blame-context :` wraps the procedure end-to-end and prints the strengthens / weakens / inverts decision. Useful when you want a one-shot answer rather than walking the procedure manually. -## Example +## Examples + +### Example 1 — magic constant (weakens) A reviewer sees this in `crates/driver/src/domain/competition/solution/settlement.rs:444`: @@ -32,6 +38,39 @@ gh pr view Decision: **weakens** the "magic number" suspicion — the constant has documented inclusion-economics rationale. Drop the finding, or downgrade to a `Question:` confirming the rationale still applies on the chain in question. +### Example 2 — defensive process exit (inverts) + +A reviewer sees this in `crates/observe/src/panic_hook.rs:14-15`: + +```rust +let new_hook = move |info: &std::panic::PanicHookInfo| { + previous_hook(info); + std::process::exit(1); +}; +``` + +Hard-killing the process from inside a panic hook looks aggressive — surely a panic should be reported and recovered from, not nuke the whole binary? Run the procedure first: + +```bash +git blame -L 14,15 -- crates/observe/src/panic_hook.rs +# → 8b918a02df Valentin 2022-09-12 ... +# (file moved from crates/shared/src/exit_process_on_panic.rs; the +# whitespace-insensitive copy-detection retry is unnecessary here +# because the same SHA blames the moved lines.) + +git log -1 --format='%s%n%b' 8b918a02df +# → "Exit process if thread panics (#530) +# Fixes #514 ." + +gh pr view 530 +# → body links to issue #514, whose body reads: +# "We spawn background tasks through `tokio::task::spawn` … a panic +# in a spawned task/thread does not affect the rest of the program. +# In these cases we want the whole program to exit." +``` + +Decision: **inverts** the suspicion. Removing the `process::exit(1)` would re-introduce exactly the silent-corruption failure mode the original PR fixed — a background panic (e.g. a cache updater) would be swallowed by tokio and the rest of the process would carry on with stale state. Drop the finding entirely; *suggesting* this change would ask the author to undo a deliberate cross-process invariant. + ## Procedure ```bash @@ -46,7 +85,7 @@ gh pr view ## Decision -Promote what blame reveals into the finding's Explanation, then weight the finding: +Promote what blame reveals into the finding's Explanation, then weigh the finding: - **Strengthens** — surrounding code was added recently for a reason the diff now contradicts. Keep / raise severity. - **Weakens** — the originating PR explains *why* the shape is unusual (deliberate workaround, perf fix, cross-version compat). Soften, or pivot from `Action:` to `Question:`. Example: a two-line guard looked over-defensive; blame showed it was a hot-patch lifted from prod logs — Medium → Question, asking whether the failure mode still applies. @@ -54,7 +93,7 @@ Promote what blame reveals into the finding's Explanation, then weight the findi ## Edge cases -- **Merge commit, not squash** — rare here; inspect with `git log -1 --format='%s%n%b' ` and walk parents (`^1`, `^2`) to find the commit that actually authored the line. +- **Merge commit, not squash** — inspect with `git log -1 --format='%s%n%b' ` and walk parents (`^1`, `^2`) to find the commit that actually authored the line. - **Same author as the PR under review, recent** — context is fresh; ask the author directly in the review thread instead of synthesising blame. - **Refactor moved code wholesale** — surface blame points at the move, not the originating fix. Use `git blame -w -C -C -C -L , -- ` (whitespace-insensitive, copy-detection) to recover the real authoring commit. - **Vendored / generated / contract-binding code** — blame the generator's input (upstream config, source `.sol`, codegen template). Skip if the surface is a JSON ABI or lockfile.