Skip to content

refactor: share daemon job polling helpers#576

Open
mariusvniekerk wants to merge 2 commits intomainfrom
bd-pui-1-daemon-polling-dedupe-clean
Open

refactor: share daemon job polling helpers#576
mariusvniekerk wants to merge 2 commits intomainfrom
bd-pui-1-daemon-polling-dedupe-clean

Conversation

@mariusvniekerk
Copy link
Collaborator

Summary

  • add a shared daemon review API helper in cmd/roborev for job and review fetches
  • route CLI wait/show paths through the shared helper to remove duplicated polling and fetch logic
  • reduce duplication inside internal/daemon HTTPClient with shared getReview/getJobByID helpers and reuse WaitForReview from the CLI path

Validation

  • go fmt ./...
  • go vet ./...
  • go test ./cmd/roborev ./internal/daemon -run 'TestWaitForReview|TestHTTPClientWaitForReviewUsesJobID|TestWaitForPromptJob|TestShowPromptResult|TestWaitForAnalysisJob_Timeout|TestReviewCmdWaitQuiet|TestWaitByJobID|TestWaitByJobIDHandlesMixedOutcomes'

Notes

  • PR is opened from a clean branch pointing at commit 38d00ec so it excludes later local .beads backup commits from the working branch.

@roborev-ci
Copy link

roborev-ci bot commented Mar 24, 2026

roborev: Combined Review (38d00ec)

Verdict: The PR successfully refactors job polling and review fetching but introduces a regression that causes indefinite hangs on invalid job IDs.

Findings

Medium Severity

  • Location: cmd/roborev/job_helpers.go:31
    • **
      Problem:** waitForJobCompletion now continues on every api.getJob error, which changes the previous len(jobs)==0 behavior from an immediate "job not found" failure into indefinite polling until the context expires. For invalid/stale job IDs this can make review/fix
      /run commands hang instead of failing fast.
    • Fix: Preserve the old not-found path by checking errors.Is(err, ErrJobNotFound) (or an equivalent sentinel) and returning immediately; only retry on transient fetch/parse failures.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link

roborev-ci bot commented Mar 25, 2026

roborev: Combined Review (49c2c4d)

Verdict: Clean

All agents (codex, gemini) reviewed the
changes and found no issues. The refactor cleanly consolidates daemon job/review polling into shared helpers, preserves existing behavior, and introduces stable test adjustments without introducing any security concerns.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

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