Skip to content

Add git-blame-historic-context skill#4373

Open
AryanGodara wants to merge 4 commits into
mainfrom
aryan/git-blame-skill
Open

Add git-blame-historic-context skill#4373
AryanGodara wants to merge 4 commits into
mainfrom
aryan/git-blame-skill

Conversation

@AryanGodara
Copy link
Copy Markdown
Member

@AryanGodara AryanGodara commented May 1, 2026

What this is

A standalone procedure for using git blame to recover historic context before flagging unusual-looking code. Often, certain decisions led to sub-optimal-looking code, and those decisions are codified in git history rather than the code itself. This skill formalises the lookup + decision rubric.

Ships with /blame-context <path>:<line>, a slash command that wraps the procedure end-to-end. Use either the slash command or the procedure manually.

Why split out

Extracted from #4351 because the procedure is reusable beyond PR review — order-debug sessions and ad-hoc code investigations both want the same lookup play. Splitting keeps each consumer's surface focused.

Files

  • docs/skills/git-blame-historic-context.md — the procedure + decision rubric.
  • .claude/commands/blame-context.md — slash command wrapping the procedure.

Reviewing this in isolation

The skill body is self-contained. The "Used by" footer links to ../COW_PR_REVIEW_SKILL.md, which lands with #4351; that link will 404 until that PR merges. Nothing else here depends on #4351.

How to try it

/blame-context crates/driver/src/domain/competition/solution/settlement.rs:444
/blame-context crates/observe/src/panic_hook.rs:14-15

Worked sample output for both is in the skill file's Examples section.

@AryanGodara AryanGodara changed the title Add git-blame-historic-context skill" Add git-blame-historic-context skill May 1, 2026
@AryanGodara AryanGodara force-pushed the aryan/git-blame-skill branch from 07f2227 to ff73770 Compare May 5, 2026 13:45
@AryanGodara AryanGodara marked this pull request as ready for review May 5, 2026 13:48
@AryanGodara AryanGodara requested a review from a team as a code owner May 5, 2026 13:48
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new Claude command and detailed documentation for the 'git blame historic context' skill, which assists developers in understanding the rationale behind unusual code by tracing it back to its originating PR. The feedback focuses on improving the automation of the gh pr view command by removing unnecessary repository flags that would require manual variable substitution, allowing the CLI to infer the context automatically.

Comment thread .claude/commands/blame-context.md Outdated
Comment thread docs/skills/git-blame-historic-context.md Outdated
@AryanGodara AryanGodara force-pushed the aryan/git-blame-skill branch from 9b89fe4 to 8cb5c1d Compare May 5, 2026 14:27
Comment thread docs/skills/git-blame-historic-context.md Outdated
Comment thread docs/skills/git-blame-historic-context.md Outdated
Comment thread docs/skills/git-blame-historic-context.md
Comment thread docs/skills/git-blame-historic-context.md Outdated
Comment thread docs/skills/git-blame-historic-context.md Outdated
@AryanGodara AryanGodara requested a review from jmg-duarte May 11, 2026 07:32
@AryanGodara AryanGodara force-pushed the aryan/git-blame-skill branch from 4cc1201 to 88c64b3 Compare May 11, 2026 07:33
@jmg-duarte
Copy link
Copy Markdown
Contributor

I tried it with:

/blame-context @crates/configs/src/shared.rs:L171-172   

/// The `BalancerV2VaultRelayer`-style allowance manager contract.
pub balances: Option<Address>,

and got:

● ─── Blame for crates/configs/src/shared.rs:171-172
  bfc4d08978  José Duarte  2026-03-19  Extract shared CLI arguments to config file-based SharedConfig (#4266)

  ─── Originating commit / PR
  PR #4266 — "Extract shared CLI arguments to config file-based SharedConfig" (merged 2026-03-19)

  Continues the migration from CLI-based configuration to file-based configuration by moving shared arguments (node URL, contract addresses, gas estimators, logging, tracing, ethrpc, current block) from `clap`-based
  `shared::arguments::Arguments` to a serde-based `SharedConfig` in the `configs` crate.

  This is the final step in eliminating `CliArguments` from the autopilot and orderbook `run()` functions, making both services fully driven by their TOML configuration files.

  Relevant change: introduces `crates/configs/src/shared.rs` with `SharedConfig` and sub-configs including `ContractAddresses` — the struct holding the `balances: Option<Address>` field on lines 171-172. All fields are
  `Option<Address>` overrides, with canonical per-chain addresses used when `None` (per the struct's doc comment on lines 162-163).

  ─── Decision
  Weakens the suspicion that this code is unusual.
  The field is a routine `Option<Address>` override slot in a newly-introduced `ContractAddresses` config struct created as part of a planned, documented CLI→TOML migration; nothing about the line is bespoke or load-bearing
  on its own.
  Action: drop

I was expecting this to dig a bit deeper to be fair, but in this case, that's ok.

Then I tried with:

/blame-context @contracts/src/codegen.rs:L112-158

/// Generate a self-contained Cargo workspace under `output_dir` containing:
/// - `contracts-generated/<name>/` per-contract crates
/// - `contracts-facade/` facade crate that re-exports all contracts
/// - Root workspace `Cargo.toml`
pub fn generate(self, artifacts_dir: &Path, output_dir: &Path) -> Result<()> {
let contracts_generated_dir = output_dir.join("contracts-generated");
let facade_dir = output_dir.join("contracts-facade");
std::fs::create_dir_all(&contracts_generated_dir)?;
std::fs::create_dir_all(&facade_dir)?;
// Collect all contracts with their submodule association, then
// generate crates in parallel.
let entries: Vec<CrateEntry> = self
.contracts
.iter()
.map(|c| (c, None))
.chain(
self.submodules
.iter()
.flat_map(|s| s.contracts.iter().map(move |c| (c, Some(s.name.clone())))),
)
.collect::<Vec<_>>()
.into_par_iter()
.map(|(contract, submodule)| {
contract.write_crate(artifacts_dir, &contracts_generated_dir)?;
Ok(CrateEntry {
contract_name: contract.name.clone(),
crate_name: to_crate_name(&contract.name),
dir_name: to_dir_name(&contract.name),
submodule,
})
})
.collect::<Result<Vec<_>>>()?;
let mut entries = entries;
entries.sort_by_key(|entry| entry.crate_name.clone());
// Generate the facade crate
write_facade_lib(&entries, &facade_dir)?;
write_facade_cargo_toml(&entries, &facade_dir)?;
// Generate the workspace Cargo.toml
write_workspace_cargo_toml(output_dir)?;
Ok(())
}

and got:

● ─── Blame for contracts/src/codegen.rs:112-158
  cfbec985df  José Duarte  2026-04-08  Refactor contracts crate out of the main folder for faster compilation 🚀 (#4217)

  ─── Originating commit / PR
  Title: Refactor contracts crate out of the main folder for faster compilation 🚀 (#4217)

  Replace the `crates/contracts` build.rs-based compile-time code generation with a standalone code-generation tool that produces pre-generated per-contract crates. This eliminates the slow build.rs step from the main
  workspace build and makes contract bindings explicit, version-controllable, and independently compilable.

  Changes:
  * Move `crates/contracts` to top-level `contracts/` directory as a standalone binary
  * Replace build.rs code generation with pre-generated Alloy contract binding crates under `contracts/generated/`
  * Add `contracts-facade` crate that re-exports all generated bindings for easy consumption
  * Update all workspace crates to import from the new generated crates
  * Add missing contract artifacts (Balancer factory V3-V6, weighted pool factory V4, NoProtocolFee LBP factory, DEX router variants)

  The PR's perf charts show workspace build time drop (~60s blocker eliminated, CPU graph reshaped from 60s serial blocker to 45s with higher parallel utilization). Merged 2026-04-08. The 375k-line diff is dominated by
  checked-in generated bindings; this `generate()` is the producer.

  ─── Decision
  Weakens the suspicion that this code is unusual.
  The rayon `into_par_iter` over contracts + the two-tree output layout (`contracts-generated/` per-crate + `contracts-facade/` re-export crate + root workspace `Cargo.toml`) is the entire stated point of the PR — a
  parallelizable, version-controllable replacement for the old build.rs — so the parallelism, deterministic sort, and facade emission are load-bearing perf/ergonomic choices, not accidental complexity.
  Action: drop finding

Again, I'd expect it to go deeper, but since this was yet another refactor, I'll also give the benefit of the doubt.

I also tried:

/blame-context

but with lines selected and it didn't work — IMO, this should work

Finally, I tried it with

/blame-context @crates/autopilot/src/maintenance.rs:L50-56                

pub enum SyncTarget {
/// Essential processing (e.g. event indexing) of the given block is
/// sufficient,
PartiallyProcessed(u64),
/// The given block as to be fully processed.
FullyProcessed(u64),
}

and I got:

● ─── Blame for crates/autopilot/src/maintenance.rs:50-56
  49d8b6e3b2  Martin Magnus  2026-02-13  Make autopilot sync more granular (#4144)
  5690fc4de0  Martin Magnus  2026-02-03  (line 56, closing brace — original `enum SyncTarget {}` introduction)

  ─── Originating commit / PR
  Title: Make autopilot sync more granular (#4144)

  Fix metrics regression introduced by #4141. That PR sped up auction-building by waiting only for "minimum processing" of a block, but it unintentionally also relaxed the wait used by the settlement-monitoring task — that
  task previously required the *full* maintenance pass to finish before looking up settlements from the DB. The relaxed wait made runloop/settle latency metrics look degraded even though end-to-end "time to happy moo" was
  healthy.

  Fix: introduce `SyncTarget` with two variants so callers can pick the level of completion they need:
  - `PartiallyProcessed(block)` — used pre-auction (only essential event indexing required)
  - `FullyProcessed(block)` — used by the settlement-landed monitor (needs the full maintenance pass)

  Two `watch` streams back the two states. Alternatives explicitly rejected in the PR body: one channel per sub-task (too granular) and atomic counters per update type (awaiting changes is harder than two watch streams).

  Reviewed/approved by fafk and jmg-duarte; merged 2026-02-13.

  ─── Decision
  Weakens the suspicion that this code is unusual.
  The two-variant `SyncTarget` enum is the explicit fix the PR is named after — selecting partial vs. full block processing is the load-bearing distinction (pre-auction vs. settlement-landed monitoring), and the alternatives
   section documents why the two-state shape was chosen over finer-grained options.
  Action: drop finding

Which, once again, I'd expect it to go deeper.

So, my point is: this is not "historical context", this is "latest change context"; either rename the skill or make it recurse. For this case, I'd got for the first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants