Skip to content

Isolate reviewer CLIs from project autoload; add per-model timeout - #1

Merged
selyafi merged 1 commit into
mainfrom
harden/reviewer-cli-isolation-and-timeout
Jun 7, 2026
Merged

Isolate reviewer CLIs from project autoload; add per-model timeout#1
selyafi merged 1 commit into
mainfrom
harden/reviewer-cli-isolation-and-timeout

Conversation

@selyafi

@selyafi selyafi commented Jun 4, 2026

Copy link
Copy Markdown
Owner

Two hardening fixes to the model-invocation path, both surfaced by reviewing the external xdev skill against diffsmith's adapters. No review-quality change — these are safety, reliability, and determinism.

diffsmith-4tz — isolate reviewer CLIs from project autoload

Reviewer CLIs ran in the caller's cwd, where codex autoloads .agents/skills/*/SKILL.md and can activate a project skill whose workflow posts review comments, and gemini/claude onboard from AGENTS.md / CLAUDE.md. Reviewing a repo that ships such a skill could silently break diffsmith's no-auto-post guarantee.

  • New provider.IsolatedRunner() runs each command in a fresh, per-call temp dir (removed after).
  • codex/gemini/claude New(nil) defaults now use it. The diff is piped via stdin, so reviewers need no cwd access; $HOME-based auth/config is untouched.
  • DefaultRunner (now sharing runCmd) keeps inheriting cwd for gh/glab, whose inbox flow's repo detection needs it.
  • Deviation from the issue: dropped codex --ignore-rules — in an empty temp cwd there's no project .rules to ignore, and hard-coding the flag would break reviews on codex versions that don't recognize it.

diffsmith-ptr — per-model wall-clock timeout

Models run in a parallel fan-out that joins on all of them, so one hung reviewer CLI (e.g. an MCP cold-start) blocked the whole review with no recovery.

  • Each goroutine's Review now runs under its own context.WithTimeout, so a slow model is cancelled and dropped while siblings finish.
  • The same cap guards the synthesis call: a hung lead model surfaces as a synthesis skip reason, never a silent fallback.
  • New --model-timeout flag (default 10m; 0 disables). Cancellation reaches the child via exec.CommandContext.

Tests

TDD-first, each RED observed before GREEN:

  • IsolatedRunner runs in / cleans up a temp dir (not the caller's cwd).
  • A slow model times out while a fast one succeeds.
  • Synthesis times out as a skip.

Full suite + go vet + go test -race green.

🤖 Generated with Claude Code

Two hardening fixes to the model-invocation path, both surfaced by
reviewing the external xdev skill against diffsmith's adapters.

diffsmith-4tz — reviewer CLIs ran in the caller's cwd, where codex
autoloads .agents/skills/*/SKILL.md (and can *activate* a project skill
whose workflow posts review comments), and gemini/claude onboard from
AGENTS.md / CLAUDE.md. That silently risks diffsmith's no-auto-post
guarantee. New provider.IsolatedRunner() runs each command in a fresh,
per-call temp dir (removed after), which the codex/gemini/claude New(nil)
defaults now use. The diff is piped via stdin so reviewers need no cwd
access; $HOME-based auth/config is untouched. DefaultRunner (now sharing
runCmd) keeps inheriting cwd for gh/glab, whose inbox flow needs it.

diffsmith-ptr — models run in a parallel fan-out that joins on all of
them, so one hung reviewer CLI (e.g. an MCP cold-start) blocked the whole
review with no recovery. Each goroutine's Review now runs under its own
context.WithTimeout, so a slow model is cancelled and dropped while
siblings finish. The same cap guards the synthesis call: a hung lead
model surfaces as a synthesis skip reason, never a silent fallback. New
--model-timeout flag (default 10m; 0 disables); cancellation reaches the
child via exec.CommandContext.

Tests added TDD-first (each RED observed before GREEN): IsolatedRunner
runs/cleans a temp dir; a slow model times out while a fast one succeeds;
synthesis times out as a skip. Full suite + go vet + go test -race green.
@selyafi
selyafi merged commit 8860768 into main Jun 7, 2026
2 checks passed
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.

1 participant