-
Notifications
You must be signed in to change notification settings - Fork 165
Add git-blame-historic-context skill #4373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
AryanGodara
wants to merge
4
commits into
main
Choose a base branch
from
aryan/git-blame-skill
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
841a9ed
Add git-blame-historic-context skill
AryanGodara c58803a
feat: add /blame-context slash command and worked example
AryanGodara 8cb5c1d
fix: drop unused -R flag from gh pr view
AryanGodara 4cc1201
address Jose review on git-blame skill: reframe 'when to invoke', add…
AryanGodara File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <path>:<line> | ||
| /blame-context <path>:<start>-<end> | ||
| ``` | ||
|
|
||
| ## Procedure | ||
|
|
||
| Follow `docs/skills/git-blame-historic-context.md` end-to-end. Concretely: | ||
|
|
||
| 1. `git blame -L <start>,<end> -- <path>` 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 <start>,<end> -- <path>` to recover the real authoring commit. | ||
| 3. `git log -1 --format='%s%n%b' <sha>` for the commit body. | ||
| 4. If the subject ends with `(#NNNN)`, pivot to the PR conversation: `gh pr view <NNNN>` (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. | ||
|
|
||
| ## Output | ||
|
|
||
| ``` | ||
| ─── Blame for <path>:<lines> | ||
| <sha> <author> <date> <subject> | ||
|
|
||
| ─── Originating commit / PR | ||
| <commit body, OR PR title + body if a (#NNNN) PR was found> | ||
|
|
||
| ─── Decision | ||
| <Strengthens | Weakens | Inverts> the suspicion that this code is unusual. | ||
| <one-sentence reason naming the concrete signal from the originating PR/commit> | ||
| Action: <keep finding | downgrade to Question | drop> | ||
| ``` | ||
|
|
||
| ## 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| # Skill — `git blame` for historic context | ||
|
|
||
| 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. | ||
| - **Via slash command** — `/blame-context <path>:<line>` 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. | ||
|
|
||
| ## Examples | ||
|
|
||
| ### Example 1 — magic constant (weakens) | ||
|
|
||
| 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 <NNNN> | ||
| # → 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. | ||
|
|
||
| ### 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 | ||
| git blame -L <start>,<end> -- <path> # 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' <sha> | ||
| gh pr view <NNNN> | ||
| ``` | ||
|
|
||
| ## Decision | ||
|
|
||
| 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. | ||
| - **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** — inspect with `git log -1 --format='%s%n%b' <sha>` and walk parents (`<sha>^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 <start>,<end> -- <path>` (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"*. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to find a second example, not to replace, but rather to provide more than one data point
I find that this example is a bit lackluster