Add roborev wait command to block until a review job completes#263
Merged
wesm merged 20 commits intoroborev-dev:mainfrom Feb 15, 2026
Merged
Add roborev wait command to block until a review job completes#263wesm merged 20 commits intoroborev-dev:mainfrom
roborev wait command to block until a review job completes#263wesm merged 20 commits intoroborev-dev:mainfrom
Conversation
roborev: Combined ReviewVerdict: No Medium/High/Critical findings; the reviewed All four review passes (security + default across both agents) reported no issues, and there were no deduplicated findings at Medium severity or above to include. Synthesized from 4 reviews (agents: codex, gemini | types: default, security) |
Implements roborev-dev#226. Adds a new `wait` command that waits for an already-running review job to complete without enqueuing a new one. This provides a token-efficient way for external coding agents to wait for reviews in a review-fix refinement loop. The command supports looking up jobs by git ref (resolved to SHA) or by job ID, with flags for --sha, --job, --quiet, and --timeout. Exit codes: 0 (pass), 1 (fail), 3 (timeout), 4 (job not found). Also extends waitForJob() with an optional timeout parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review findings for the wait command: 1. Add repo filter to lookupJobBySHA to avoid cross-repo job mismatch when the daemon tracks multiple repos. Follows the same normalization pattern as findJobForCommit. 2. Map "job not found" from waitForJob to exit code 4 in waitCmd, matching the documented behavior without changing reviewCmd. 3. Error when --job is set without a positional argument instead of silently falling through to the HEAD default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review findings: 1. Use GetMainRepoRoot instead of GetRepoRoot in lookupJobBySHA so that repo scoping matches the daemon's storage path, which always uses the main repo root (not the worktree path). 2. Replace brittle strings.Contains "not found" matching with a sentinel errJobNotFound error and errors.Is check, preventing unrelated errors (e.g. "no review found") from being misclassified as exit code 4. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review findings: 1. Resolve SHA from GetRepoRoot (worktree-aware, where the user is working) but use GetMainRepoRoot for repo filtering (to match daemon storage). Previously both used GetMainRepoRoot, which resolved HEAD to the wrong commit in worktrees. 2. Add wait_test.go with coverage for: - --job flag requires argument - --sha and positional arg conflict - Exit code 4 when no job found (by SHA and by job ID) - Exit code 4 quiet mode suppresses output - Exit code 3 on timeout - Exit code 0 for passing review - Exit code 1 for failing review - "no review found" error is NOT remapped to exit code 4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addresses review findings from jobs 127/128. Adds a test that creates a main repo + worktree, runs wait --sha HEAD from the worktree, and asserts that: - git_ref is resolved from the worktree HEAD (not main repo) - repo query param uses the main repo root (not worktree path) This directly covers the core worktree fix behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove trailing periods from doc comments (matches rest of codebase) - Rename errJobNotFound to ErrJobNotFound (matches ErrDaemonNotRunning) - Use "daemon returned <status>" error format (matches existing HTTP errors) - Drop error wrapping in connection error (matches existing pattern) - Capitalize subtest descriptions (matches existing test style) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers non-200 daemon response during SHA lookup, verifying the error message format includes the HTTP status code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Assert on full status line and response body content instead of just the status code number. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split lookupJobBySHA into resolveGitContext + lookupJobByRef to eliminate double SHA resolution in the positional-arg code path. lookupJobBySHA is kept as a thin convenience wrapper. Add waitMockHandler and requireExitCode test helpers to reduce boilerplate across wait tests. Add three new test cases: - TestWaitInvalidPositionalArg (bad arg that is neither ref nor number) - TestWaitPositionalArgAsGitRef (HEAD as positional arg, not --sha) - TestWaitNumericFallbackToJobID (numeric arg treated as job ID) Co-authored-by: Cursor <cursoragent@cursor.com>
Use standard 0/1 exit semantics: 0 for PASS verdict, 1 for any failure (FAIL verdict, timeout, no job found, job error). Removes the non-standard exit codes 3 and 4 that added complexity without meaningful benefit -- agents can read stderr for failure details. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the built-in timeout in favor of the shell's timeout command (e.g. `timeout 60 roborev wait`). This keeps the wait command minimal and avoids changes to the shared waitForJob function. Co-authored-by: Cursor <cursoragent@cursor.com>
Update the error handling in lookupJobByRef and handleWaitLookupErr to use ErrJobNotFound instead of exitError for improved clarity. This change standardizes the error reporting when no job is found, enhancing the user experience by providing a more descriptive error message. Co-authored-by: Cursor <cursoragent@cursor.com>
…b IDs lookupJobBySHA now fails with "invalid git ref" when the ref cannot be resolved, instead of silently querying with the raw string. The --job flag rejects IDs <= 0 with "invalid job ID" instead of flowing into the polling loop. Tests added for both paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extends the <= 0 validation to the positional numeric fallback, not just the --job flag, for consistent input validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers the case where `roborev wait 0` falls through both git ref resolution and the positive-numeric check, producing an error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace custom lookupJobBySHA, lookupJobByRef, resolveGitContext, and handleWaitLookupErr with the existing findJobForCommit function. This gets the wait command the same repo-path fallback logic used elsewhere and removes ~80 lines of duplicated code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers waitForJob returning an error when /api/jobs?id=... returns HTTP 500 during polling, ensuring the status code and body are surfaced in the error message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ests - Add newWaitEnv helper to consolidate repo+daemon+chdir setup - Add runWait helper to consolidate command creation and execution - Convert 6 arg-validation tests into a single table-driven test - Net reduction of ~130 lines while preserving all test coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ensureDaemon() ran before input validation and git ref resolution, so invalid inputs like --job 0 or --sha bad-ref could trigger a daemon start/restart before returning a validation error. Move ensureDaemon after all local checks so validation errors are returned immediately without daemon contact. Add TestWaitArgValidationWithoutDaemon to verify validation errors are returned even when no daemon is running. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aef94a2 to
6649373
Compare
roborev: Combined ReviewVerdict: All reviewers agree this PR is clean with no Medium/High/Critical findings. Consolidated Findings (Medium+)No Medium, High, or Critical issues were identified across the four review outputs (security and default reviews). Synthesized from 4 reviews (agents: codex, gemini | types: security, default) |
hughdbrown
pushed a commit
to hughdbrown/roborev
that referenced
this pull request
Feb 16, 2026
…orev-dev#263) ## Summary Fixes roborev-dev#226 When an external coding agent runs a review-fix refinement loop with roborev, the agent needs to know when a review finishes. Today the only option is `roborev review --wait`, which enqueues a new review and blocks. But in the common flow -- where a post-commit hook already triggers the review -- the agent just needs to wait for the result without submitting a duplicate job. This forces agents to poll `roborev list` or parse `roborev show` output, wasting tokens on repeated CLI invocations and output parsing. `roborev wait` provides a token-efficient primitive for this: a single blocking call that exits with a machine-readable exit code. The agent commits, the hook enqueues the review, and the agent calls `roborev wait` to block until the verdict is ready. This is convenient because it allows the user to fully control the editor agent - it can be launched in a special sandboxed environment if needed and it maintains the full context history of user interactions. The review agent provides "fresh eyes" through a new context window managed by `roborev` to complete the review and provide feedback. ## Architecture ``` Agent commits code | v Post-commit hook ──> roborev review ──> daemon enqueues job | v Agent calls: roborev wait | v CLI resolves HEAD ──> GET /api/jobs?git_ref=<sha>&repo=<path>&limit=1 | (existing daemon API, no changes) v CLI polls: GET /api/jobs?id=<job_id> | (reuses existing waitForJob with exponential backoff) v Job completes ──> GET /api/review?job_id=<id> | v Exit code signals verdict to agent: 0 = PASS 1 = FAIL ``` No daemon, API, or storage changes are required. The command is purely a CLI-side consumer of existing endpoints. ## Usage ``` roborev wait # Wait for most recent job for HEAD roborev wait abc123 # Wait for most recent job for commit roborev wait 42 # Job ID (if "42" is not a valid git ref) roborev wait --job 42 # Force as job ID roborev wait --sha HEAD~1 # Wait for job matching HEAD~1 roborev wait --quiet # Suppress output (for hooks/agents) ``` Argument resolution: git ref first (handles ambiguous numeric SHAs like `123456`), then numeric job ID fallback. `--sha` and `--job` flags disambiguate when needed. Worktrees are handled correctly -- refs resolve from the worktree HEAD while job lookups scope to the main repo path. ## Design questions - It seems like there are two different approaches we could take. `roborev review --wait` uses `waitForJob()` which I'm reusing here. But I see that `roborev refine` uses `client.WaitForReview()` which has some slightly different logic and returns the actual review from the job. ## Example Here's a snippet of a Claude Code session where I'm using a dev build of `roborev` which implements the `wait` command ``` ⏺ All clean. Let me commit. ⏺ Bash(git add cmd/roborev/main.go cmd/roborev/wait_test.go && git diff --cached --stat) ⎿ cmd/roborev/main.go | 7 +++++-- cmd/roborev/wait_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) ⏺ Bash(git commit -m "Validate wait command inputs: reject invalid refs and non-positive job IDs…) ⎿ [add-wait-command 5299820] Validate wait command inputs: reject invalid refs and non-positive job IDs 2 files changed, 47 insertions(+), 2 deletions(-) ⏺ Now let me wait for the post-commit review. ⏺ Bash(export PATH=~/go-sdk/go/bin:$PATH && roborev wait --sha HEAD --timeout 300) ⎿ Error: Exit code 1 Waiting for review to complete... done! Review (by codex) ------------------------------------------------------------ Reviewing commit `5299820` now against the surrounding `wait` command flow so I can validate behavior changes, not just the patch hunk. I’ll inspect the touched functions and nearby error-handling paths, then report concrete findings with severities and line references. I’ve finished checking the main code path and found one functional hole tied to this change: non-positive IDs are only rejected for `--job`, not for numeric positional IDs. I’ll grab exact line references and finalize the review in the format you requested. I’m running the affected wait-command tests to confirm the behavior and validate whether the uncovered positional-ID case is currently untested in execution as well as in static inspection. 1. **Summary** This commit tightens `wait` input validation by: … +20 lines (ctrl+o to see all) ⏺ Fair point — the positional numeric fallback also needs the <= 0 check. Let me fix that. ``` --- **Disclaimer**: this is my first time writing in Go, and I used coding assistants to help craft this feature. I have walked through the changes line by line (having the agent both explain the logic and show how it matches existing logic in the codebase) and made an effort to mirror the style and choices already established in the repo. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Wes McKinney <wesmckinn+git@gmail.com>
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.
Summary
Fixes #226
When an external coding agent runs a review-fix refinement loop with roborev, the agent needs to know when a review finishes. Today the only option is
roborev review --wait, which enqueues a new review and blocks. But in the common flow -- where a post-commit hook already triggers the review -- the agent just needs to wait for the result without submitting a duplicate job. This forces agents to pollroborev listor parseroborev showoutput, wasting tokens on repeated CLI invocations and output parsing.roborev waitprovides a token-efficient primitive for this: a single blocking call that exits with a machine-readable exit code. The agent commits, the hook enqueues the review, and the agent callsroborev waitto block until the verdict is ready.This is convenient because it allows the user to fully control the editor agent - it can be launched in a special sandboxed environment if needed and it maintains the full context history of user interactions. The review agent provides "fresh eyes" through a new context window managed by
roborevto complete the review and provide feedback.Architecture
No daemon, API, or storage changes are required. The command is purely a CLI-side consumer of existing endpoints.
Usage
Argument resolution: git ref first (handles ambiguous numeric SHAs like
123456), then numeric job ID fallback.--shaand--jobflags disambiguate when needed. Worktrees are handled correctly -- refs resolve from the worktree HEAD while job lookups scope to the main repo path.Design questions
roborev review --waituseswaitForJob()which I'm reusing here. But I see thatroborev refineusesclient.WaitForReview()which has some slightly different logic and returns the actual review from the job.Example
Here's a snippet of a Claude Code session where I'm using a dev build of
roborevwhich implements thewaitcommandDisclaimer: this is my first time writing in Go, and I used coding assistants to help craft this feature. I have walked through the changes line by line (having the agent both explain the logic and show how it matches existing logic in the codebase) and made an effort to mirror the style and choices already established in the repo.