Isolate reviewer CLIs from project autoload; add per-model timeout - #1
Merged
Merged
Conversation
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.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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
codexautoloads.agents/skills/*/SKILL.mdand can activate a project skill whose workflow posts review comments, andgemini/claudeonboard fromAGENTS.md/CLAUDE.md. Reviewing a repo that ships such a skill could silently break diffsmith's no-auto-post guarantee.provider.IsolatedRunner()runs each command in a fresh, per-call temp dir (removed after).codex/gemini/claudeNew(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 sharingrunCmd) keeps inheriting cwd forgh/glab, whose inbox flow's repo detection needs it.codex --ignore-rules— in an empty temp cwd there's no project.rulesto 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.
Reviewnow runs under its owncontext.WithTimeout, so a slow model is cancelled and dropped while siblings finish.--model-timeoutflag (default10m;0disables). Cancellation reaches the child viaexec.CommandContext.Tests
TDD-first, each RED observed before GREEN:
IsolatedRunnerruns in / cleans up a temp dir (not the caller's cwd).Full suite +
go vet+go test -racegreen.🤖 Generated with Claude Code