feat(blame): add coco blame --explain command - #2012
feat(blame): add coco blame --explain command#2012gfargo-horizon-agent[bot] wants to merge 3 commits into
Conversation
Registers blame as a top-level command (blameData.ts was TUI-only). Plain mode renders a blame gutter; --explain resolves the distinct introducing commits via getCommitDetail, batches them into a single LLM call (never one-per-sha), and prints why each range was likely written. Caps lines/shas to keep --explain cost-bounded, and adds the blameExplain dynamic-model task across all six provider tables.
![gfargo-horizon-agent[bot]](https://avatars.githubusercontent.com/in/4083186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — LGTM
REVIEW: LGTM
RESOLVES: full
The PR implements coco blame <file> [--lines] [--explain] [--json] per the plan: mirrors the log/recap command layout, batches the LLM call by distinct introducing commit with sha/line/token caps, fills blameExplain across the DynamicModelTask type, all six provider tables, the runtime task list, and both schemas, and registers the command everywhere (index, fish completions, export, smoke test) with render/parse unit tests. Deferred items (TUI E binding, PR lookup) match the plan's explicit risk notes and are correctly out of scope; error/no-key paths use commandExit/`handleMissingA
1 nit — 1 inline on the diff
| handleMissingApiKey(logger, config, { command: 'blame' }) | ||
| } | ||
|
|
||
| if (!range && lines.length > MAX_EXPLAIN_LINES) { |
There was a problem hiding this comment.
🧹 Line-count cap skipped when --lines is set
The 400-line pre-check only fires if (!range && ...), so an explicit --lines 1:100000 bypasses the friendly early rejection. Cost stays bounded downstream by the 25-distinct-commit cap plus enforcePromptBudget, so this is a wording inconsistency with the acceptance criterion ('refuses / truncates when the range exceeds a sane sha/line ceiling') rather than a real cost blowup.
Previously the 400-line pre-check only fired when no --lines range was given, so an explicit large range (e.g. --lines 1:100000) bypassed the friendly early rejection and fell through to the LLM call.
The test spawns a subprocess that transpiles and imports the module via tsx, which exceeds Jest's default 5s timeout under macOS CI's heavier resource contention. Follows the same fix already applied in src/commands/hooks/handler.test.ts for subprocess-spawning tests.
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — CONCERNS
REVIEW: CONCERNS
RESOLVES: partial
The CLI command is fully and cleanly delivered — mechanical blame rendering, a single batched --explain LLM call grouped by introducing commit, unconditional cost caps, a blameExplain DynamicModelTask registered across every provider, and no API-key requirement outside --explain. The sketch's second half — an in-TUI E explain binding on the existing blame surface — is not delivered, so the work item is only partially resolved.
1 concern · 1 nit — 2 inline on the diff
| @@ -0,0 +1,250 @@ | |||
| import { z } from 'zod' | |||
There was a problem hiding this comment.
E explain binding not delivered
The work item explicitly asks to 'Add an E binding to the blame surface for the same in-TUI', alongside the CLI command. This PR touches no workstation files and the existing blame surface (src/workstation/surfaces/blame/index.ts) has no explain binding or explain code path. This should be explicitly called out as deferred/out-of-scope on the PR, or added before merge if it was intended to land with this item.
|
|
||
| const execFileAsync = promisify(execFile) | ||
|
|
||
| // Each test spawns a subprocess that transpiles and imports the validator |
There was a problem hiding this comment.
🧹 Unrelated timeout fix bundled into blame PR
Raising the jest timeout for the commitlintValidator subprocess test is unrelated to coco blame and is scope creep per repo convention ('keep changes scoped to the work item'). It is harmless and a reasonable flakiness fix, but ideally belongs in its own commit/PR.
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — CONCERNS
REVIEW: CONCERNS
RESOLVES: full
Both halves of OSS-1604 now land in the worktree: the CLI --explain and the deferred in-TUI E binding, cleanly factored through a shared explainBlameGroups core with reused cost caps, cache, and abort/ownership guard. Remaining concerns are an ungraceful CLI crash when an introducing commit can't be resolved, no direct tests for the extracted orchestration core, and a 400-line cap that makes E unusable (with a CLI-only remediation message) on large files in the TUI.
⚠️ getCommitDetail failure crashes CLI --explain —src/commands/blame/explain.ts:108— ThePromise.allovergetCommitDetailsits outside the try/catch that wrapsexecuteChain, and the CLI caller (handler.ts:74) does not wrapexplainBlameGroups. If any introducing commit can't be resolved (shallow-clone boundary, rewritten/rebased history, submodule edge), the whole--explaininvocation throws with a stack trace instead of degrading like an LLM failure does. Moving the detail-resolution into the existing try/catch fixes both the CLI and TUI callers at once (the TUI hook currently masks this only via its own outer catch).⚠️ No tests for extracted explain orchestration core —src/commands/blame/explain.ts:61—explainBlameGroupsis now the shared core for both the CLI and TUI, yet it has zero direct test coverage: the 400-line/25-commit caps,UNCOMMITTED_SHAfiltering, truncation accounting, batched-call construction, and the partial-failure fallback text are all unverified. Onlyrender.ts(pure formatting) has a test file, while sibling LLM-backed commands (changelog, review) ship handler/orchestration tests. The extraction into a pure, data-returning function makes this trivially testable, so the gap is now easy to close.⚠️ In-TUI E unusable on files over 400 lines —src/workstation/runtime/hooks/useBlameExplainActions.ts:81— The hook always passes the full hydratedblameLines(entire file) to the explain core, butexplainBlameGroupsrejects any range overMAX_EXPLAIN_LINES(400). For any file longer than 400 lines,Etherefore always fails, and the surfaced error tells the user to "Narrow the range with --lines a:b" — a CLI flag the TUI has no equivalent for. The plan anticipated using the selected line's range; scoping the explain window to the visible/selected range (or a bounded window around the cursor) would make the feature usable on large files instead of permanently erroring.- 🧹 Unrelated CI-stability fix bundled in —
src/lib/utils/commitlintValidator.test.ts:11— Thejest.setTimeout(15000)change fixes a flaky subprocess test unrelated to blame. It's a safe one-liner, but AGENTS.md asks to keep changes scoped to the work item; ideally its own commit/PR.
What
Adds a non-interactive
coco blame <file> [--lines a:b] [--explain] [--json]command. Plain mode renders the existingblameData.tsgutter output;--explainresolves each distinct introducing commit and asks an LLM for a per-range intent summary.Why
Plane: OSS-1604
Blame is currently TUI-only and purely mechanical (who/when, never why).
cocoalready has every input needed (blameData.ts,logData.ts) to answer the most common code-archaeology question: "who wrote this and what were they thinking."How
src/commands/blame/{config,index,handler,prompt,render}.ts, mirroring thelogcommand's structure.getBlame()→ optional--lines a:bfilter → table/JSON render.--explainpath: groups blame lines by introducing commit (dedup — 2 lines from the same sha cost onegetCommitDetailfetch), resolves commit details, and issues a single batched LLM call covering all distinct commits (not one call per sha).--explainruns over 400 lines (points the user at--lines), and caps at 25 distinct commits per call with a truncation notice.--jsonemits blame lines (+explanationsin explain mode) viaemitJson; only the--explainbranch touches config/LLM plumbing, so plaincoco blamenever requires an API key.blameExplainDynamicModelTaskentry across all six provider default tables (OPENAI/ANTHROPIC/GEMINI/MISTRAL/BEDROCK/OLLAMA) — this is compiler-enforced byRecord<DynamicModelTask, LLMModel>, and is what caused the prior CI failure on this item.src/index.ts(command, fish completion list, export block) andbin/smokeCli.ts(blame --help+blame README.mdagainst the smoke repo).Scoped out (per the implementation plan): the in-TUI
Ebinding on the blame surface and PR-lookup-for-explain are deferred to a follow-up — wiring an async LLM action into the TUI runtime is materially riskier than the CLI command and isn't needed for this to land.Testing
npx jest src/commands/blame src/lib/langchain/utils/dynamicModelsgreen; full suite green except 4 pre-existing tree-sitter WASM failures reproduced identically on a cleanmaincheckout — unrelated to this change)blame --help,blame README.md --lines 1:5,blame README.md --lines 1:3 --json, andblame README.md --explain(reaches the LLM call and fails gracefully on the sandbox's placeholder API key — same behavior asreview/recap/changelogin this environment)🤖 Generated by the harbor agent loop. Reviewed by a human before merge.
Closes #1950