From 38d00ecb4652a7bc5c34685a16ea2df978a06372 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Tue, 24 Mar 2026 11:23:36 -0400 Subject: [PATCH 1/2] refactor: share daemon job polling helpers --- .beads/backup/backup_state.json | 12 ++-- .beads/backup/dependencies.jsonl | 7 ++ .beads/backup/events.jsonl | 42 ++++++++++++ .beads/backup/issues.jsonl | 8 +++ .beads/backup/labels.jsonl | 24 +++++++ cmd/roborev/analyze.go | 56 +-------------- cmd/roborev/daemon_api.go | 79 +++++++++++++++++++++ cmd/roborev/daemon_client.go | 113 ++++++------------------------- cmd/roborev/job_helpers.go | 80 ++++------------------ cmd/roborev/run.go | 57 +++++----------- internal/daemon/client.go | 78 +++++++++------------ 11 files changed, 251 insertions(+), 305 deletions(-) create mode 100644 cmd/roborev/daemon_api.go diff --git a/.beads/backup/backup_state.json b/.beads/backup/backup_state.json index e0f05b7c1..e9a893f9a 100644 --- a/.beads/backup/backup_state.json +++ b/.beads/backup/backup_state.json @@ -1,13 +1,13 @@ { - "last_dolt_commit": "74hjbh5dsambsmc06pb0v5ag2bv2cllq", + "last_dolt_commit": "bkp9f7illjujnoj0ts8e04otd1tr7od7", "last_event_id": 0, - "timestamp": "2026-03-24T14:59:16.853732Z", + "timestamp": "2026-03-24T15:21:47.129544Z", "counts": { - "issues": 0, - "events": 0, + "issues": 8, + "events": 42, "comments": 0, - "dependencies": 0, - "labels": 0, + "dependencies": 7, + "labels": 24, "config": 11 } } \ No newline at end of file diff --git a/.beads/backup/dependencies.jsonl b/.beads/backup/dependencies.jsonl index e69de29bb..cbdab5407 100644 --- a/.beads/backup/dependencies.jsonl +++ b/.beads/backup/dependencies.jsonl @@ -0,0 +1,7 @@ +{"created_at":"2026-03-24T11:05:47Z","created_by":"Marius van Niekerk","depends_on_id":"bd-pui","issue_id":"bd-pui.1","type":"parent-child"} +{"created_at":"2026-03-24T11:05:47Z","created_by":"Marius van Niekerk","depends_on_id":"bd-pui","issue_id":"bd-pui.2","type":"parent-child"} +{"created_at":"2026-03-24T11:05:47Z","created_by":"Marius van Niekerk","depends_on_id":"bd-pui","issue_id":"bd-pui.3","type":"parent-child"} +{"created_at":"2026-03-24T11:05:47Z","created_by":"Marius van Niekerk","depends_on_id":"bd-pui","issue_id":"bd-pui.4","type":"parent-child"} +{"created_at":"2026-03-24T11:05:47Z","created_by":"Marius van Niekerk","depends_on_id":"bd-pui","issue_id":"bd-pui.5","type":"parent-child"} +{"created_at":"2026-03-24T11:05:47Z","created_by":"Marius van Niekerk","depends_on_id":"bd-pui","issue_id":"bd-pui.6","type":"parent-child"} +{"created_at":"2026-03-24T11:05:47Z","created_by":"Marius van Niekerk","depends_on_id":"bd-pui","issue_id":"bd-pui.7","type":"parent-child"} diff --git a/.beads/backup/events.jsonl b/.beads/backup/events.jsonl index e69de29bb..807d43f08 100644 --- a/.beads/backup/events.jsonl +++ b/.beads/backup/events.jsonl @@ -0,0 +1,42 @@ +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:04:39Z","event_type":"created","id":1,"issue_id":"bd-pui","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:04:39Z","event_type":"label_added","id":2,"issue_id":"bd-pui","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:04:39Z","event_type":"label_added","id":3,"issue_id":"bd-pui","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:04:39Z","event_type":"label_added","id":4,"issue_id":"bd-pui","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:05:47Z","event_type":"created","id":5,"issue_id":"bd-pui.1","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":6,"issue_id":"bd-pui.1","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":7,"issue_id":"bd-pui.1","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":8,"issue_id":"bd-pui.1","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:05:47Z","event_type":"created","id":9,"issue_id":"bd-pui.2","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":10,"issue_id":"bd-pui.2","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":11,"issue_id":"bd-pui.2","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":12,"issue_id":"bd-pui.2","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:05:47Z","event_type":"created","id":13,"issue_id":"bd-pui.3","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":14,"issue_id":"bd-pui.3","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":15,"issue_id":"bd-pui.3","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":16,"issue_id":"bd-pui.3","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:05:47Z","event_type":"created","id":17,"issue_id":"bd-pui.4","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":18,"issue_id":"bd-pui.4","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":19,"issue_id":"bd-pui.4","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":20,"issue_id":"bd-pui.4","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:05:47Z","event_type":"created","id":21,"issue_id":"bd-pui.5","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":22,"issue_id":"bd-pui.5","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":23,"issue_id":"bd-pui.5","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":24,"issue_id":"bd-pui.5","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:05:47Z","event_type":"created","id":25,"issue_id":"bd-pui.6","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":26,"issue_id":"bd-pui.6","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":27,"issue_id":"bd-pui.6","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":28,"issue_id":"bd-pui.6","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:05:47Z","event_type":"created","id":29,"issue_id":"bd-pui.7","new_value":"","old_value":""} +{"actor":"Marius van Niekerk","comment":"Added label: refactor","created_at":"2026-03-24T11:05:47Z","event_type":"label_added","id":30,"issue_id":"bd-pui.7","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: deduplication","created_at":"2026-03-24T11:05:48Z","event_type":"label_added","id":31,"issue_id":"bd-pui.7","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":"Added label: simplification","created_at":"2026-03-24T11:05:48Z","event_type":"label_added","id":32,"issue_id":"bd-pui.7","new_value":null,"old_value":null} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:11Z","event_type":"updated","id":33,"issue_id":"bd-pui","new_value":"{\"design\":\"\\n\\nAcceptance Criteria\\n- Each completed child issue explicitly includes running /roborev:fix after implementation before closure\\n\"}","old_value":"{\"id\":\"bd-pui\",\"title\":\"Simplification pass: code reuse and deduplication\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"epic\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:04:39Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:04:39Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:11Z","event_type":"updated","id":34,"issue_id":"bd-pui.1","new_value":"{\"design\":\"Goal\\nEliminate overlapping job polling and review-fetch logic across the CLI and daemon HTTP client layers while preserving current command-specific output and exit-code behavior.\\n\\nTarget Files\\n- cmd/roborev/daemon_client.go\\n- cmd/roborev/job_helpers.go\\n- cmd/roborev/run.go\\n- cmd/roborev/analyze.go\\n- internal/daemon/client.go\\n\\nChange\\n- Identify the canonical polling/fetch abstraction for job status and final review retrieval.\\n- Collapse duplicate waitFor* implementations where the only differences are output formatting or verdict handling.\\n- Leave intentionally distinct flows separate when they differ materially, but route them through shared status/review helpers.\\n\\nEdge Cases\\n- Context cancellation and timeout handling\\n- Unknown future job statuses\\n- done status arriving before review becomes readable\\n- Prompt jobs that should not use verdict-based exit codes\\n- Quiet mode versus streaming output mode\\n\\nAcceptance Criteria\\n- Independent job polling implementations are reduced to a small deliberate set rather than separate ad hoc loops per command\\n- cmd/roborev/daemon_client.go no longer duplicates review/status fetch logic already represented in internal/daemon/client.go or a new shared helper\\n- Review, run, analyze, and compact flows preserve current output and exit-code semantics under targeted tests\\n- No daemon/background behavior change is introduced while deduplicating the client-side logic\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not collapse materially different UX flows into one unreadable mega-function\\n- Do not change retry/backoff semantics unless tests prove the old behavior was accidental\"}","old_value":"{\"id\":\"bd-pui.1\",\"title\":\"Unify daemon review polling and CLI daemon client reuse\",\"design\":\"Goal\\nEliminate overlapping job polling and review-fetch logic across the CLI and daemon HTTP client layers while preserving current command-specific output and exit-code behavior.\\n\\nTarget Files\\n- cmd/roborev/daemon_client.go\\n- cmd/roborev/job_helpers.go\\n- cmd/roborev/run.go\\n- cmd/roborev/analyze.go\\n- internal/daemon/client.go\\n\\nChange\\n- Identify the canonical polling/fetch abstraction for job status and final review retrieval.\\n- Collapse duplicate waitFor* implementations where the only differences are output formatting or verdict handling.\\n- Leave intentionally distinct flows separate when they differ materially, but route them through shared status/review helpers.\\n\\nEdge Cases\\n- Context cancellation and timeout handling\\n- Unknown future job statuses\\n- done status arriving before review becomes readable\\n- Prompt jobs that should not use verdict-based exit codes\\n- Quiet mode versus streaming output mode\\n\\nAcceptance Criteria\\n- Independent job polling implementations are reduced to a small deliberate set rather than separate ad hoc loops per command\\n- cmd/roborev/daemon_client.go no longer duplicates review/status fetch logic already represented in internal/daemon/client.go or a new shared helper\\n- Review, run, analyze, and compact flows preserve current output and exit-code semantics under targeted tests\\n- No daemon/background behavior change is introduced while deduplicating the client-side logic\\n\\nAnti-patterns\\n- Do not collapse materially different UX flows into one unreadable mega-function\\n- Do not change retry/backoff semantics unless tests prove the old behavior was accidental\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:47Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:05:47Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:12Z","event_type":"updated","id":35,"issue_id":"bd-pui.2","new_value":"{\"design\":\"Goal\\nReduce repeated transport and loader boilerplate in the TUI without obscuring Bubble Tea state transitions.\\n\\nTarget Files\\n- cmd/roborev/tui/api.go\\n- cmd/roborev/tui/fetch.go\\n- cmd/roborev/tui/actions.go\\n- cmd/roborev/tui/handlers_msg.go\\n\\nChange\\n- Standardize the TUI around a small set of synchronous load helpers such as loadReview, loadJob, loadPatch, paged job loaders, and repo/branch loaders plus thin tea.Cmd wrappers.\\n- Reuse getJSON/postJSON consistently rather than open-coding request/response handling.\\n- Keep message construction local to the command wrappers so update flows remain explicit.\\n\\nEdge Cases\\n- 404 versus generic server failures\\n- Clipboard/copy flows that need loaded job metadata\\n- Pagination and branch filter sentinels\\n- Reconnect and stale fetch sequencing\\n\\nAcceptance Criteria\\n- Repeated GET/POST request plus decode boilerplate in fetch.go and actions.go is routed through a shared loader layer\\n- fetchReviewAndCopy, fetchPatchAndJob, fetchJobByID, and similar functions reuse shared synchronous loaders instead of each issuing bespoke transport logic\\n- Bubble Tea message types and optimistic-update behavior remain explicit and current TUI fetch/review/copy tests continue to pass\\n- Transport helpers stay simple enough that a maintainer can still follow each async state transition from dispatch to message handling\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not introduce generic reflection-heavy request helpers\\n- Do not hide message typing behind callbacks that make update flow harder to follow\"}","old_value":"{\"id\":\"bd-pui.2\",\"title\":\"Normalize TUI HTTP loading and Tea command wrappers\",\"design\":\"Goal\\nReduce repeated transport and loader boilerplate in the TUI without obscuring Bubble Tea state transitions.\\n\\nTarget Files\\n- cmd/roborev/tui/api.go\\n- cmd/roborev/tui/fetch.go\\n- cmd/roborev/tui/actions.go\\n- cmd/roborev/tui/handlers_msg.go\\n\\nChange\\n- Standardize the TUI around a small set of synchronous load helpers such as loadReview, loadJob, loadPatch, paged job loaders, and repo/branch loaders plus thin tea.Cmd wrappers.\\n- Reuse getJSON/postJSON consistently rather than open-coding request/response handling.\\n- Keep message construction local to the command wrappers so update flows remain explicit.\\n\\nEdge Cases\\n- 404 versus generic server failures\\n- Clipboard/copy flows that need loaded job metadata\\n- Pagination and branch filter sentinels\\n- Reconnect and stale fetch sequencing\\n\\nAcceptance Criteria\\n- Repeated GET/POST request plus decode boilerplate in fetch.go and actions.go is routed through a shared loader layer\\n- fetchReviewAndCopy, fetchPatchAndJob, fetchJobByID, and similar functions reuse shared synchronous loaders instead of each issuing bespoke transport logic\\n- Bubble Tea message types and optimistic-update behavior remain explicit and current TUI fetch/review/copy tests continue to pass\\n- Transport helpers stay simple enough that a maintainer can still follow each async state transition from dispatch to message handling\\n\\nAnti-patterns\\n- Do not introduce generic reflection-heavy request helpers\\n- Do not hide message typing behind callbacks that make update flow harder to follow\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:47Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:05:47Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:12Z","event_type":"updated","id":36,"issue_id":"bd-pui.3","new_value":"{\"design\":\"Goal\\nRemove repetitive field-copy boilerplate from agent WithReasoning, WithAgentic, WithModel, and WithSessionID methods while preserving immutable-style behavior.\\n\\nTarget Files\\n- internal/agent/codex.go\\n- internal/agent/claude.go\\n- internal/agent/gemini.go\\n- internal/agent/cursor.go\\n- internal/agent/copilot.go\\n- internal/agent/opencode.go\\n- internal/agent/pi.go\\n- internal/agent/kilo.go\\n- internal/agent/kiro.go\\n- internal/agent/droid.go\\n\\nChange\\n- Introduce a small internal helper or embedded config pattern for the common command/model/reasoning/agentic/session state each agent clones.\\n- Keep type-specific fields and behavior local to each agent.\\n- Preserve the current contract that empty model inputs keep the existing/default model where appropriate.\\n\\nEdge Cases\\n- Agents with session state versus agents without it\\n- Agents where WithModel(\\\"\\\") intentionally returns the existing instance\\n- Agents with extra fields such as provider or built-in defaults\\n\\nAcceptance Criteria\\n- Repeated copy constructors across agent adapters are substantially reduced through a shared internal pattern\\n- Public agent behavior, including default-model preservation and session sanitization, remains unchanged\\n- Existing agent tests covering reasoning/model/session persistence continue to pass or are expanded to lock the preserved behavior\\n- The resulting abstraction remains internal and obvious rather than introducing a second parallel agent hierarchy\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not erase agent-specific semantics just to force uniformity\\n- Do not add exported compatibility wrappers for old construction paths\"}","old_value":"{\"id\":\"bd-pui.3\",\"title\":\"Extract shared agent configuration cloning helpers\",\"design\":\"Goal\\nRemove repetitive field-copy boilerplate from agent WithReasoning, WithAgentic, WithModel, and WithSessionID methods while preserving immutable-style behavior.\\n\\nTarget Files\\n- internal/agent/codex.go\\n- internal/agent/claude.go\\n- internal/agent/gemini.go\\n- internal/agent/cursor.go\\n- internal/agent/copilot.go\\n- internal/agent/opencode.go\\n- internal/agent/pi.go\\n- internal/agent/kilo.go\\n- internal/agent/kiro.go\\n- internal/agent/droid.go\\n\\nChange\\n- Introduce a small internal helper or embedded config pattern for the common command/model/reasoning/agentic/session state each agent clones.\\n- Keep type-specific fields and behavior local to each agent.\\n- Preserve the current contract that empty model inputs keep the existing/default model where appropriate.\\n\\nEdge Cases\\n- Agents with session state versus agents without it\\n- Agents where WithModel(\\\"\\\") intentionally returns the existing instance\\n- Agents with extra fields such as provider or built-in defaults\\n\\nAcceptance Criteria\\n- Repeated copy constructors across agent adapters are substantially reduced through a shared internal pattern\\n- Public agent behavior, including default-model preservation and session sanitization, remains unchanged\\n- Existing agent tests covering reasoning/model/session persistence continue to pass or are expanded to lock the preserved behavior\\n- The resulting abstraction remains internal and obvious rather than introducing a second parallel agent hierarchy\\n\\nAnti-patterns\\n- Do not erase agent-specific semantics just to force uniformity\\n- Do not add exported compatibility wrappers for old construction paths\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:47Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:05:47Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:12Z","event_type":"updated","id":37,"issue_id":"bd-pui.4","new_value":"{\"design\":\"Goal\\nMake workflow-based agent selection use one shared precedence path across CLI, daemon, and batch-review callers.\\n\\nTarget Files\\n- cmd/roborev/fix.go\\n- cmd/roborev/refine.go\\n- internal/agent/model_resolution.go\\n- internal/daemon/worker.go\\n- internal/review/batch.go\\n\\nChange\\n- Extract a shared workflow-resolution helper that determines preferred agent, backup selection, resolved model, and reasoning context.\\n- Remove duplicated preferred-versus-backup comparison code from fix/refine/worker paths.\\n- Keep caller-specific behavior separate, for example agentic mode, retry policy, and provider configuration.\\n\\nEdge Cases\\n- Backup agent selected via alias/canonical name mismatch\\n- Empty CLI model meaning preserve agent default unless workflow/backup config overrides it\\n- Fix workflow versus review/security/design workflow mapping\\n- Daemon failover behavior versus foreground CLI selection\\n\\nAcceptance Criteria\\n- Fix, refine, daemon worker, and batch review paths use the same workflow precedence rules for primary agent, backup agent, and model selection\\n- Duplicated preferred-versus-backup comparison logic is removed from at least the currently duplicated CLI/daemon call sites\\n- Existing tests around backup model selection and workflow model resolution continue to pass, with new coverage added where current behavior was only implicit\\n- Caller-specific concerns such as retry policy and WithAgentic(true) remain outside the shared resolver\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not mix config resolution with execution/retry orchestration in one helper\\n- Do not change fallback order without explicit tests proving intent\"}","old_value":"{\"id\":\"bd-pui.4\",\"title\":\"Unify workflow agent, model, and backup resolution\",\"design\":\"Goal\\nMake workflow-based agent selection use one shared precedence path across CLI, daemon, and batch-review callers.\\n\\nTarget Files\\n- cmd/roborev/fix.go\\n- cmd/roborev/refine.go\\n- internal/agent/model_resolution.go\\n- internal/daemon/worker.go\\n- internal/review/batch.go\\n\\nChange\\n- Extract a shared workflow-resolution helper that determines preferred agent, backup selection, resolved model, and reasoning context.\\n- Remove duplicated preferred-versus-backup comparison code from fix/refine/worker paths.\\n- Keep caller-specific behavior separate, for example agentic mode, retry policy, and provider configuration.\\n\\nEdge Cases\\n- Backup agent selected via alias/canonical name mismatch\\n- Empty CLI model meaning preserve agent default unless workflow/backup config overrides it\\n- Fix workflow versus review/security/design workflow mapping\\n- Daemon failover behavior versus foreground CLI selection\\n\\nAcceptance Criteria\\n- Fix, refine, daemon worker, and batch review paths use the same workflow precedence rules for primary agent, backup agent, and model selection\\n- Duplicated preferred-versus-backup comparison logic is removed from at least the currently duplicated CLI/daemon call sites\\n- Existing tests around backup model selection and workflow model resolution continue to pass, with new coverage added where current behavior was only implicit\\n- Caller-specific concerns such as retry policy and WithAgentic(true) remain outside the shared resolver\\n\\nAnti-patterns\\n- Do not mix config resolution with execution/retry orchestration in one helper\\n- Do not change fallback order without explicit tests proving intent\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:48Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:05:48Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:12Z","event_type":"updated","id":38,"issue_id":"bd-pui.5","new_value":"{\"design\":\"Goal\\nEnsure each agent adapter has one source of truth for CLI argument assembly, avoiding drift between CommandLine previews and actual runtime args.\\n\\nTarget Files\\n- internal/agent/cursor.go\\n- internal/agent/pi.go\\n- internal/agent/codex.go\\n- internal/agent/claude.go\\n- internal/agent/gemini.go\\n- internal/agent/copilot.go\\n- internal/agent/opencode.go\\n- internal/agent/kilo.go\\n- internal/agent/kiro.go\\n- internal/agent/droid.go\\n\\nChange\\n- Refactor CommandLine to derive from a single argument-construction path per agent, with an explicit preview/runtime distinction only where necessary.\\n- Remove duplicated inline argument assembly that repeats buildArgs behavior.\\n- Preserve omission of runtime-only values like prompt stdin and repo path when rendering previews.\\n\\nEdge Cases\\n- Session resume arguments that are valid for execution but should be sanitized for preview\\n- Agents whose preview intentionally substitutes a default model like auto\\n- Runtime-only arguments such as repo path, stdin markers, or temp prompt files\\n\\nAcceptance Criteria\\n- Each agent adapter has one obvious argument builder rather than separate partially duplicated preview and runtime code paths\\n- CommandLine still emits a representative preview without leaking runtime-only prompt or path values\\n- Existing build-arg and command-line tests continue to pass and cover any preview/runtime split that remains necessary\\n- No agent changes its actual executed flags as an accidental side effect of the cleanup\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not force all agents through one global builder if their CLIs materially differ\\n- Do not silently change preview output formatting without updating tests intentionally\"}","old_value":"{\"id\":\"bd-pui.5\",\"title\":\"Unify agent command preview and argument builders\",\"design\":\"Goal\\nEnsure each agent adapter has one source of truth for CLI argument assembly, avoiding drift between CommandLine previews and actual runtime args.\\n\\nTarget Files\\n- internal/agent/cursor.go\\n- internal/agent/pi.go\\n- internal/agent/codex.go\\n- internal/agent/claude.go\\n- internal/agent/gemini.go\\n- internal/agent/copilot.go\\n- internal/agent/opencode.go\\n- internal/agent/kilo.go\\n- internal/agent/kiro.go\\n- internal/agent/droid.go\\n\\nChange\\n- Refactor CommandLine to derive from a single argument-construction path per agent, with an explicit preview/runtime distinction only where necessary.\\n- Remove duplicated inline argument assembly that repeats buildArgs behavior.\\n- Preserve omission of runtime-only values like prompt stdin and repo path when rendering previews.\\n\\nEdge Cases\\n- Session resume arguments that are valid for execution but should be sanitized for preview\\n- Agents whose preview intentionally substitutes a default model like auto\\n- Runtime-only arguments such as repo path, stdin markers, or temp prompt files\\n\\nAcceptance Criteria\\n- Each agent adapter has one obvious argument builder rather than separate partially duplicated preview and runtime code paths\\n- CommandLine still emits a representative preview without leaking runtime-only prompt or path values\\n- Existing build-arg and command-line tests continue to pass and cover any preview/runtime split that remains necessary\\n- No agent changes its actual executed flags as an accidental side effect of the cleanup\\n\\nAnti-patterns\\n- Do not force all agents through one global builder if their CLIs materially differ\\n- Do not silently change preview output formatting without updating tests intentionally\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:48Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:05:48Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:12Z","event_type":"updated","id":39,"issue_id":"bd-pui.6","new_value":"{\"design\":\"Goal\\nReduce repeated flag/repo/global/default precedence logic across CI-related commands and workflow-file generation.\\n\\nTarget Files\\n- cmd/roborev/ci.go\\n- cmd/roborev/ghaction.go\\n- internal/config/config.go only if a shared helper belongs there\\n\\nChange\\n- Consolidate the repeated precedence patterns used by resolveAgentList, resolveReviewTypes, resolveCIReasoning, resolveCIMinSeverity, resolveCISynthesisAgent, resolveCIUpsertComments, and resolveWorkflowConfig.\\n- Keep command-specific UX and output separate from shared resolution logic.\\n- Prefer a small shared config-resolver helper over open-coded precedence ladders in multiple command files.\\n\\nEdge Cases\\n- Empty flag meaning auto-detect rather than explicit empty value\\n- Repo config overriding global config only for some fields\\n- Workflow generation needing a list while CLI review execution may accept an empty-string sentinel\\n\\nAcceptance Criteria\\n- Repeated flag \\u003e repo \\u003e global \\u003e default precedence code is consolidated into a shared helper or small helper family\\n- ci.go and ghaction.go derive agent/workflow configuration consistently from the same precedence rules\\n- Existing CI-related tests continue to pass, with added coverage where precedence behavior was previously duplicated but unpinned\\n- The extracted helper remains simple and data-focused rather than embedding command execution concerns\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not push command-specific side effects into shared config helpers\\n- Do not replace explicit precedence with reflection or map-based indirection that hides behavior\"}","old_value":"{\"id\":\"bd-pui.6\",\"title\":\"Consolidate CI and workflow config precedence helpers\",\"design\":\"Goal\\nReduce repeated flag/repo/global/default precedence logic across CI-related commands and workflow-file generation.\\n\\nTarget Files\\n- cmd/roborev/ci.go\\n- cmd/roborev/ghaction.go\\n- internal/config/config.go only if a shared helper belongs there\\n\\nChange\\n- Consolidate the repeated precedence patterns used by resolveAgentList, resolveReviewTypes, resolveCIReasoning, resolveCIMinSeverity, resolveCISynthesisAgent, resolveCIUpsertComments, and resolveWorkflowConfig.\\n- Keep command-specific UX and output separate from shared resolution logic.\\n- Prefer a small shared config-resolver helper over open-coded precedence ladders in multiple command files.\\n\\nEdge Cases\\n- Empty flag meaning auto-detect rather than explicit empty value\\n- Repo config overriding global config only for some fields\\n- Workflow generation needing a list while CLI review execution may accept an empty-string sentinel\\n\\nAcceptance Criteria\\n- Repeated flag \\u003e repo \\u003e global \\u003e default precedence code is consolidated into a shared helper or small helper family\\n- ci.go and ghaction.go derive agent/workflow configuration consistently from the same precedence rules\\n- Existing CI-related tests continue to pass, with added coverage where precedence behavior was previously duplicated but unpinned\\n- The extracted helper remains simple and data-focused rather than embedding command execution concerns\\n\\nAnti-patterns\\n- Do not push command-specific side effects into shared config helpers\\n- Do not replace explicit precedence with reflection or map-based indirection that hides behavior\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:48Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:05:48Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:12Z","event_type":"updated","id":40,"issue_id":"bd-pui.7","new_value":"{\"design\":\"Goal\\nRemove repeated repo-root, main-repo-root, and current-branch discovery snippets in fix-related command paths.\\n\\nTarget Files\\n- cmd/roborev/fix.go\\n- Related callers in cmd/roborev/compact.go or adjacent command files only if they can share the same helper cleanly\\n\\nChange\\n- Extract helpers for the common patterns currently repeated in fix list/open/batch/single-job flows: working directory lookup, worktree root resolution, main repo root resolution for daemon/API queries, and current branch fallback when --branch is omitted.\\n- Use those helpers consistently so fix code talks about intent instead of path plumbing.\\n\\nEdge Cases\\n- Git worktrees versus main repo root\\n- Detached HEAD or missing current branch\\n- Commands that query the daemon using main root but operate on the current worktree root\\n\\nAcceptance Criteria\\n- Repeated os.Getwd plus git.GetRepoRoot plus git.GetMainRepoRoot plus git.GetCurrentBranch snippets in fix flows are replaced by shared helpers\\n- Worktree-root versus main-repo-root behavior remains unchanged for daemon queries and local file operations\\n- Fix list/open/batch/single-job paths read more directly in terms of intent after extraction\\n- Existing fix tests continue to pass, with targeted additions for worktree/main-root behavior if current coverage is thin\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not hide unrelated fix orchestration in the new helper\\n- Do not normalize away meaningful distinctions between worktree root and main repo root\"}","old_value":"{\"id\":\"bd-pui.7\",\"title\":\"Extract shared repo-root and branch resolution helpers for fix flows\",\"design\":\"Goal\\nRemove repeated repo-root, main-repo-root, and current-branch discovery snippets in fix-related command paths.\\n\\nTarget Files\\n- cmd/roborev/fix.go\\n- Related callers in cmd/roborev/compact.go or adjacent command files only if they can share the same helper cleanly\\n\\nChange\\n- Extract helpers for the common patterns currently repeated in fix list/open/batch/single-job flows: working directory lookup, worktree root resolution, main repo root resolution for daemon/API queries, and current branch fallback when --branch is omitted.\\n- Use those helpers consistently so fix code talks about intent instead of path plumbing.\\n\\nEdge Cases\\n- Git worktrees versus main repo root\\n- Detached HEAD or missing current branch\\n- Commands that query the daemon using main root but operate on the current worktree root\\n\\nAcceptance Criteria\\n- Repeated os.Getwd plus git.GetRepoRoot plus git.GetMainRepoRoot plus git.GetCurrentBranch snippets in fix flows are replaced by shared helpers\\n- Worktree-root versus main-repo-root behavior remains unchanged for daemon queries and local file operations\\n- Fix list/open/batch/single-job paths read more directly in terms of intent after extraction\\n- Existing fix tests continue to pass, with targeted additions for worktree/main-root behavior if current coverage is thin\\n\\nAnti-patterns\\n- Do not hide unrelated fix orchestration in the new helper\\n- Do not normalize away meaningful distinctions between worktree root and main repo root\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:48Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:05:48Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:08:47Z","event_type":"updated","id":41,"issue_id":"bd-pui","new_value":"{\"design\":\"Goal\\nTrack a conservative simplification pass over duplicated logic identified in the March 2026 audit. The work should reduce maintenance bloat and repeated control flow without changing user-facing behavior.\\n\\nScope\\nThis epic covers the independently actionable refactor categories discovered in cmd/roborev, cmd/roborev/tui, internal/agent, and internal/daemon.\\n\\nNon-goals\\n- No feature changes\\n- No behavior changes to CLI/TUI output beyond bug-for-bug preservation\\n- No broad rewrites without tests anchoring current behavior\\n\\nSuccess Criteria\\n- Each child issue has a narrow, behavior-preserving refactor scope\\n- The highest-value duplicate code paths are tracked separately so they can be executed incrementally\\n- Each child issue names concrete files, edge cases, and acceptance criteria\\n- Each completed child issue explicitly includes running /roborev:fix after implementation before closure\"}","old_value":"{\"id\":\"bd-pui\",\"title\":\"Simplification pass: code reuse and deduplication\",\"design\":\"\\n\\nAcceptance Criteria\\n- Each completed child issue explicitly includes running /roborev:fix after implementation before closure\\n\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"epic\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:04:39Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:08:12Z\"}"} +{"actor":"Marius van Niekerk","comment":null,"created_at":"2026-03-24T11:21:47Z","event_type":"status_changed","id":42,"issue_id":"bd-pui.1","new_value":"{\"status\":\"in_progress\"}","old_value":"{\"id\":\"bd-pui.1\",\"title\":\"Unify daemon review polling and CLI daemon client reuse\",\"design\":\"Goal\\nEliminate overlapping job polling and review-fetch logic across the CLI and daemon HTTP client layers while preserving current command-specific output and exit-code behavior.\\n\\nTarget Files\\n- cmd/roborev/daemon_client.go\\n- cmd/roborev/job_helpers.go\\n- cmd/roborev/run.go\\n- cmd/roborev/analyze.go\\n- internal/daemon/client.go\\n\\nChange\\n- Identify the canonical polling/fetch abstraction for job status and final review retrieval.\\n- Collapse duplicate waitFor* implementations where the only differences are output formatting or verdict handling.\\n- Leave intentionally distinct flows separate when they differ materially, but route them through shared status/review helpers.\\n\\nEdge Cases\\n- Context cancellation and timeout handling\\n- Unknown future job statuses\\n- done status arriving before review becomes readable\\n- Prompt jobs that should not use verdict-based exit codes\\n- Quiet mode versus streaming output mode\\n\\nAcceptance Criteria\\n- Independent job polling implementations are reduced to a small deliberate set rather than separate ad hoc loops per command\\n- cmd/roborev/daemon_client.go no longer duplicates review/status fetch logic already represented in internal/daemon/client.go or a new shared helper\\n- Review, run, analyze, and compact flows preserve current output and exit-code semantics under targeted tests\\n- No daemon/background behavior change is introduced while deduplicating the client-side logic\\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\\n\\nAnti-patterns\\n- Do not collapse materially different UX flows into one unreadable mega-function\\n- Do not change retry/backoff semantics unless tests prove the old behavior was accidental\",\"status\":\"open\",\"priority\":2,\"issue_type\":\"task\",\"owner\":\"mvanniekerk@inmarket.com\",\"created_at\":\"2026-03-24T15:05:47Z\",\"created_by\":\"Marius van Niekerk\",\"updated_at\":\"2026-03-24T15:08:12Z\"}"} diff --git a/.beads/backup/issues.jsonl b/.beads/backup/issues.jsonl index e69de29bb..072a9687c 100644 --- a/.beads/backup/issues.jsonl +++ b/.beads/backup/issues.jsonl @@ -0,0 +1,8 @@ +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"33af5971e3b81d54ad61fef7e9b61c804346057486a1fb19a3fc4f569cc62376","created_at":"2026-03-24T15:04:39Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nTrack a conservative simplification pass over duplicated logic identified in the March 2026 audit. The work should reduce maintenance bloat and repeated control flow without changing user-facing behavior.\n\nScope\nThis epic covers the independently actionable refactor categories discovered in cmd/roborev, cmd/roborev/tui, internal/agent, and internal/daemon.\n\nNon-goals\n- No feature changes\n- No behavior changes to CLI/TUI output beyond bug-for-bug preservation\n- No broad rewrites without tests anchoring current behavior\n\nSuccess Criteria\n- Each child issue has a narrow, behavior-preserving refactor scope\n- The highest-value duplicate code paths are tracked separately so they can be executed incrementally\n- Each child issue names concrete files, edge cases, and acceptance criteria\n- Each completed child issue explicitly includes running /roborev:fix after implementation before closure","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui","is_template":0,"issue_type":"epic","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"open","target":"","timeout_ns":0,"title":"Simplification pass: code reuse and deduplication","updated_at":"2026-03-24T15:08:48Z","waiters":"","wisp_type":"","work_type":""} +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"f4e391603b1d12ca1f9e8fd08821d271c296a9686e50d987b39fbf412af28dde","created_at":"2026-03-24T15:05:47Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nEliminate overlapping job polling and review-fetch logic across the CLI and daemon HTTP client layers while preserving current command-specific output and exit-code behavior.\n\nTarget Files\n- cmd/roborev/daemon_client.go\n- cmd/roborev/job_helpers.go\n- cmd/roborev/run.go\n- cmd/roborev/analyze.go\n- internal/daemon/client.go\n\nChange\n- Identify the canonical polling/fetch abstraction for job status and final review retrieval.\n- Collapse duplicate waitFor* implementations where the only differences are output formatting or verdict handling.\n- Leave intentionally distinct flows separate when they differ materially, but route them through shared status/review helpers.\n\nEdge Cases\n- Context cancellation and timeout handling\n- Unknown future job statuses\n- done status arriving before review becomes readable\n- Prompt jobs that should not use verdict-based exit codes\n- Quiet mode versus streaming output mode\n\nAcceptance Criteria\n- Independent job polling implementations are reduced to a small deliberate set rather than separate ad hoc loops per command\n- cmd/roborev/daemon_client.go no longer duplicates review/status fetch logic already represented in internal/daemon/client.go or a new shared helper\n- Review, run, analyze, and compact flows preserve current output and exit-code semantics under targeted tests\n- No daemon/background behavior change is introduced while deduplicating the client-side logic\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\n\nAnti-patterns\n- Do not collapse materially different UX flows into one unreadable mega-function\n- Do not change retry/backoff semantics unless tests prove the old behavior was accidental","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui.1","is_template":0,"issue_type":"task","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"in_progress","target":"","timeout_ns":0,"title":"Unify daemon review polling and CLI daemon client reuse","updated_at":"2026-03-24T15:21:47Z","waiters":"","wisp_type":"","work_type":""} +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"3de51b7ce2a03c8ac49a13ba5a808b0e4bfcc0653b907c6ff8f6504d61965504","created_at":"2026-03-24T15:05:47Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nReduce repeated transport and loader boilerplate in the TUI without obscuring Bubble Tea state transitions.\n\nTarget Files\n- cmd/roborev/tui/api.go\n- cmd/roborev/tui/fetch.go\n- cmd/roborev/tui/actions.go\n- cmd/roborev/tui/handlers_msg.go\n\nChange\n- Standardize the TUI around a small set of synchronous load helpers such as loadReview, loadJob, loadPatch, paged job loaders, and repo/branch loaders plus thin tea.Cmd wrappers.\n- Reuse getJSON/postJSON consistently rather than open-coding request/response handling.\n- Keep message construction local to the command wrappers so update flows remain explicit.\n\nEdge Cases\n- 404 versus generic server failures\n- Clipboard/copy flows that need loaded job metadata\n- Pagination and branch filter sentinels\n- Reconnect and stale fetch sequencing\n\nAcceptance Criteria\n- Repeated GET/POST request plus decode boilerplate in fetch.go and actions.go is routed through a shared loader layer\n- fetchReviewAndCopy, fetchPatchAndJob, fetchJobByID, and similar functions reuse shared synchronous loaders instead of each issuing bespoke transport logic\n- Bubble Tea message types and optimistic-update behavior remain explicit and current TUI fetch/review/copy tests continue to pass\n- Transport helpers stay simple enough that a maintainer can still follow each async state transition from dispatch to message handling\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\n\nAnti-patterns\n- Do not introduce generic reflection-heavy request helpers\n- Do not hide message typing behind callbacks that make update flow harder to follow","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui.2","is_template":0,"issue_type":"task","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"open","target":"","timeout_ns":0,"title":"Normalize TUI HTTP loading and Tea command wrappers","updated_at":"2026-03-24T15:08:12Z","waiters":"","wisp_type":"","work_type":""} +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"083c71b8b2d2bc49fb5aa86d09f29eff16d7af4ebc647d045fcb8cbca0a3bb47","created_at":"2026-03-24T15:05:47Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nRemove repetitive field-copy boilerplate from agent WithReasoning, WithAgentic, WithModel, and WithSessionID methods while preserving immutable-style behavior.\n\nTarget Files\n- internal/agent/codex.go\n- internal/agent/claude.go\n- internal/agent/gemini.go\n- internal/agent/cursor.go\n- internal/agent/copilot.go\n- internal/agent/opencode.go\n- internal/agent/pi.go\n- internal/agent/kilo.go\n- internal/agent/kiro.go\n- internal/agent/droid.go\n\nChange\n- Introduce a small internal helper or embedded config pattern for the common command/model/reasoning/agentic/session state each agent clones.\n- Keep type-specific fields and behavior local to each agent.\n- Preserve the current contract that empty model inputs keep the existing/default model where appropriate.\n\nEdge Cases\n- Agents with session state versus agents without it\n- Agents where WithModel(\"\") intentionally returns the existing instance\n- Agents with extra fields such as provider or built-in defaults\n\nAcceptance Criteria\n- Repeated copy constructors across agent adapters are substantially reduced through a shared internal pattern\n- Public agent behavior, including default-model preservation and session sanitization, remains unchanged\n- Existing agent tests covering reasoning/model/session persistence continue to pass or are expanded to lock the preserved behavior\n- The resulting abstraction remains internal and obvious rather than introducing a second parallel agent hierarchy\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\n\nAnti-patterns\n- Do not erase agent-specific semantics just to force uniformity\n- Do not add exported compatibility wrappers for old construction paths","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui.3","is_template":0,"issue_type":"task","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"open","target":"","timeout_ns":0,"title":"Extract shared agent configuration cloning helpers","updated_at":"2026-03-24T15:08:12Z","waiters":"","wisp_type":"","work_type":""} +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"22e9009e006283289aa912c841a88244a73c289a72b055c8556495ab45d28a3f","created_at":"2026-03-24T15:05:48Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nMake workflow-based agent selection use one shared precedence path across CLI, daemon, and batch-review callers.\n\nTarget Files\n- cmd/roborev/fix.go\n- cmd/roborev/refine.go\n- internal/agent/model_resolution.go\n- internal/daemon/worker.go\n- internal/review/batch.go\n\nChange\n- Extract a shared workflow-resolution helper that determines preferred agent, backup selection, resolved model, and reasoning context.\n- Remove duplicated preferred-versus-backup comparison code from fix/refine/worker paths.\n- Keep caller-specific behavior separate, for example agentic mode, retry policy, and provider configuration.\n\nEdge Cases\n- Backup agent selected via alias/canonical name mismatch\n- Empty CLI model meaning preserve agent default unless workflow/backup config overrides it\n- Fix workflow versus review/security/design workflow mapping\n- Daemon failover behavior versus foreground CLI selection\n\nAcceptance Criteria\n- Fix, refine, daemon worker, and batch review paths use the same workflow precedence rules for primary agent, backup agent, and model selection\n- Duplicated preferred-versus-backup comparison logic is removed from at least the currently duplicated CLI/daemon call sites\n- Existing tests around backup model selection and workflow model resolution continue to pass, with new coverage added where current behavior was only implicit\n- Caller-specific concerns such as retry policy and WithAgentic(true) remain outside the shared resolver\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\n\nAnti-patterns\n- Do not mix config resolution with execution/retry orchestration in one helper\n- Do not change fallback order without explicit tests proving intent","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui.4","is_template":0,"issue_type":"task","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"open","target":"","timeout_ns":0,"title":"Unify workflow agent, model, and backup resolution","updated_at":"2026-03-24T15:08:12Z","waiters":"","wisp_type":"","work_type":""} +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"e0a3f457dbbb5468400a301b48f572053b46598e81538d9c1f7322c52e25149b","created_at":"2026-03-24T15:05:48Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nEnsure each agent adapter has one source of truth for CLI argument assembly, avoiding drift between CommandLine previews and actual runtime args.\n\nTarget Files\n- internal/agent/cursor.go\n- internal/agent/pi.go\n- internal/agent/codex.go\n- internal/agent/claude.go\n- internal/agent/gemini.go\n- internal/agent/copilot.go\n- internal/agent/opencode.go\n- internal/agent/kilo.go\n- internal/agent/kiro.go\n- internal/agent/droid.go\n\nChange\n- Refactor CommandLine to derive from a single argument-construction path per agent, with an explicit preview/runtime distinction only where necessary.\n- Remove duplicated inline argument assembly that repeats buildArgs behavior.\n- Preserve omission of runtime-only values like prompt stdin and repo path when rendering previews.\n\nEdge Cases\n- Session resume arguments that are valid for execution but should be sanitized for preview\n- Agents whose preview intentionally substitutes a default model like auto\n- Runtime-only arguments such as repo path, stdin markers, or temp prompt files\n\nAcceptance Criteria\n- Each agent adapter has one obvious argument builder rather than separate partially duplicated preview and runtime code paths\n- CommandLine still emits a representative preview without leaking runtime-only prompt or path values\n- Existing build-arg and command-line tests continue to pass and cover any preview/runtime split that remains necessary\n- No agent changes its actual executed flags as an accidental side effect of the cleanup\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\n\nAnti-patterns\n- Do not force all agents through one global builder if their CLIs materially differ\n- Do not silently change preview output formatting without updating tests intentionally","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui.5","is_template":0,"issue_type":"task","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"open","target":"","timeout_ns":0,"title":"Unify agent command preview and argument builders","updated_at":"2026-03-24T15:08:12Z","waiters":"","wisp_type":"","work_type":""} +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"f50ad75cb6a3d59fc6c6ec64ab29258f48196c29ae5760e0c303415f80c1087d","created_at":"2026-03-24T15:05:48Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nReduce repeated flag/repo/global/default precedence logic across CI-related commands and workflow-file generation.\n\nTarget Files\n- cmd/roborev/ci.go\n- cmd/roborev/ghaction.go\n- internal/config/config.go only if a shared helper belongs there\n\nChange\n- Consolidate the repeated precedence patterns used by resolveAgentList, resolveReviewTypes, resolveCIReasoning, resolveCIMinSeverity, resolveCISynthesisAgent, resolveCIUpsertComments, and resolveWorkflowConfig.\n- Keep command-specific UX and output separate from shared resolution logic.\n- Prefer a small shared config-resolver helper over open-coded precedence ladders in multiple command files.\n\nEdge Cases\n- Empty flag meaning auto-detect rather than explicit empty value\n- Repo config overriding global config only for some fields\n- Workflow generation needing a list while CLI review execution may accept an empty-string sentinel\n\nAcceptance Criteria\n- Repeated flag \u003e repo \u003e global \u003e default precedence code is consolidated into a shared helper or small helper family\n- ci.go and ghaction.go derive agent/workflow configuration consistently from the same precedence rules\n- Existing CI-related tests continue to pass, with added coverage where precedence behavior was previously duplicated but unpinned\n- The extracted helper remains simple and data-focused rather than embedding command execution concerns\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\n\nAnti-patterns\n- Do not push command-specific side effects into shared config helpers\n- Do not replace explicit precedence with reflection or map-based indirection that hides behavior","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui.6","is_template":0,"issue_type":"task","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"open","target":"","timeout_ns":0,"title":"Consolidate CI and workflow config precedence helpers","updated_at":"2026-03-24T15:08:13Z","waiters":"","wisp_type":"","work_type":""} +{"acceptance_criteria":"","actor":"","agent_state":"","assignee":null,"await_id":"","await_type":"","close_reason":"","closed_at":null,"closed_by_session":"","compacted_at":null,"compacted_at_commit":null,"compaction_level":0,"content_hash":"e3bc8f7e3f317d8bf16450a637dcd7b03288a30676a7666a82a2e52afe8972ec","created_at":"2026-03-24T15:05:48Z","created_by":"Marius van Niekerk","crystallizes":0,"defer_until":null,"description":"","design":"Goal\nRemove repeated repo-root, main-repo-root, and current-branch discovery snippets in fix-related command paths.\n\nTarget Files\n- cmd/roborev/fix.go\n- Related callers in cmd/roborev/compact.go or adjacent command files only if they can share the same helper cleanly\n\nChange\n- Extract helpers for the common patterns currently repeated in fix list/open/batch/single-job flows: working directory lookup, worktree root resolution, main repo root resolution for daemon/API queries, and current branch fallback when --branch is omitted.\n- Use those helpers consistently so fix code talks about intent instead of path plumbing.\n\nEdge Cases\n- Git worktrees versus main repo root\n- Detached HEAD or missing current branch\n- Commands that query the daemon using main root but operate on the current worktree root\n\nAcceptance Criteria\n- Repeated os.Getwd plus git.GetRepoRoot plus git.GetMainRepoRoot plus git.GetCurrentBranch snippets in fix flows are replaced by shared helpers\n- Worktree-root versus main-repo-root behavior remains unchanged for daemon queries and local file operations\n- Fix list/open/batch/single-job paths read more directly in terms of intent after extraction\n- Existing fix tests continue to pass, with targeted additions for worktree/main-root behavior if current coverage is thin\n- After implementation is complete, /roborev:fix has been run before the issue is considered done\n\nAnti-patterns\n- Do not hide unrelated fix orchestration in the new helper\n- Do not normalize away meaningful distinctions between worktree root and main repo root","due_at":null,"ephemeral":0,"estimated_minutes":null,"event_kind":"","external_ref":null,"hook_bead":"","id":"bd-pui.7","is_template":0,"issue_type":"task","last_activity":null,"metadata":"{}","mol_type":"","notes":"","original_size":null,"owner":"mvanniekerk@inmarket.com","payload":"","pinned":0,"priority":2,"quality_score":null,"rig":"","role_bead":"","role_type":"","sender":"","source_repo":"","source_system":"","spec_id":"","status":"open","target":"","timeout_ns":0,"title":"Extract shared repo-root and branch resolution helpers for fix flows","updated_at":"2026-03-24T15:08:13Z","waiters":"","wisp_type":"","work_type":""} diff --git a/.beads/backup/labels.jsonl b/.beads/backup/labels.jsonl index e69de29bb..09dd87997 100644 --- a/.beads/backup/labels.jsonl +++ b/.beads/backup/labels.jsonl @@ -0,0 +1,24 @@ +{"issue_id":"bd-pui","label":"deduplication"} +{"issue_id":"bd-pui","label":"refactor"} +{"issue_id":"bd-pui","label":"simplification"} +{"issue_id":"bd-pui.1","label":"deduplication"} +{"issue_id":"bd-pui.1","label":"refactor"} +{"issue_id":"bd-pui.1","label":"simplification"} +{"issue_id":"bd-pui.2","label":"deduplication"} +{"issue_id":"bd-pui.2","label":"refactor"} +{"issue_id":"bd-pui.2","label":"simplification"} +{"issue_id":"bd-pui.3","label":"deduplication"} +{"issue_id":"bd-pui.3","label":"refactor"} +{"issue_id":"bd-pui.3","label":"simplification"} +{"issue_id":"bd-pui.4","label":"deduplication"} +{"issue_id":"bd-pui.4","label":"refactor"} +{"issue_id":"bd-pui.4","label":"simplification"} +{"issue_id":"bd-pui.5","label":"deduplication"} +{"issue_id":"bd-pui.5","label":"refactor"} +{"issue_id":"bd-pui.5","label":"simplification"} +{"issue_id":"bd-pui.6","label":"deduplication"} +{"issue_id":"bd-pui.6","label":"refactor"} +{"issue_id":"bd-pui.6","label":"simplification"} +{"issue_id":"bd-pui.7","label":"deduplication"} +{"issue_id":"bd-pui.7","label":"refactor"} +{"issue_id":"bd-pui.7","label":"simplification"} diff --git a/cmd/roborev/analyze.go b/cmd/roborev/analyze.go index 366a8f9f2..7b3094b64 100644 --- a/cmd/roborev/analyze.go +++ b/cmd/roborev/analyze.go @@ -667,8 +667,7 @@ func runAnalyzeAndFix(cmd *cobra.Command, ep daemon.DaemonEndpoint, repoRoot str // waitForAnalysisJob polls until the job completes and returns the review. // The context controls the maximum wait time. func waitForAnalysisJob(ctx context.Context, ep daemon.DaemonEndpoint, jobID int64) (*storage.Review, error) { - client := ep.HTTPClient(30 * time.Second) - baseURL := ep.BaseURL() + api := newDaemonReviewAPI(ep.BaseURL(), ep.HTTPClient(30*time.Second)) pollInterval := 1 * time.Second maxInterval := 5 * time.Second @@ -680,64 +679,15 @@ func waitForAnalysisJob(ctx context.Context, ep daemon.DaemonEndpoint, jobID int default: } - req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/api/jobs?id=%d", baseURL, jobID), nil) - if err != nil { - return nil, fmt.Errorf("create request: %w", err) - } - - resp, err := client.Do(req) + job, err := api.getJob(ctx, jobID) if err != nil { return nil, fmt.Errorf("check job status: %w", err) } - - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - resp.Body.Close() - return nil, fmt.Errorf("server error (%d): %s", resp.StatusCode, body) - } - - var jobsResp struct { - Jobs []storage.ReviewJob `json:"jobs"` - } - if err := json.NewDecoder(resp.Body).Decode(&jobsResp); err != nil { - resp.Body.Close() - return nil, fmt.Errorf("parse job status: %w", err) - } - resp.Body.Close() - - if len(jobsResp.Jobs) == 0 { - return nil, fmt.Errorf("job %d not found", jobID) - } - - job := jobsResp.Jobs[0] switch job.Status { case storage.JobStatusDone: - // Fetch the review - reviewReq, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/api/review?job_id=%d", baseURL, jobID), nil) - if err != nil { - return nil, fmt.Errorf("create review request: %w", err) - } - - reviewResp, err := client.Do(reviewReq) - if err != nil { - return nil, fmt.Errorf("fetch review: %w", err) - } - defer reviewResp.Body.Close() - - if reviewResp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(reviewResp.Body) - return nil, fmt.Errorf("fetch review (%d): %s", reviewResp.StatusCode, body) - } - - var review storage.Review - if err := json.NewDecoder(reviewResp.Body).Decode(&review); err != nil { - return nil, fmt.Errorf("parse review: %w", err) - } - return &review, nil - + return api.getReview(ctx, jobID, "review") case storage.JobStatusFailed: return nil, fmt.Errorf("job failed: %s", job.Error) - case storage.JobStatusCanceled: return nil, fmt.Errorf("job was canceled") } diff --git a/cmd/roborev/daemon_api.go b/cmd/roborev/daemon_api.go new file mode 100644 index 000000000..57f83de53 --- /dev/null +++ b/cmd/roborev/daemon_api.go @@ -0,0 +1,79 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + + "github.com/roborev-dev/roborev/internal/storage" +) + +var errReviewNotFound = errors.New("review not found") + +type daemonReviewAPI struct { + baseURL string + client *http.Client +} + +func newDaemonReviewAPI(baseURL string, client *http.Client) daemonReviewAPI { + return daemonReviewAPI{baseURL: baseURL, client: client} +} + +func (a daemonReviewAPI) getJob(ctx context.Context, jobID int64) (*storage.ReviewJob, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("%s/api/jobs?id=%d", a.baseURL, jobID), nil) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + + resp, err := a.client.Do(req) + if err != nil { + return nil, fmt.Errorf("fetch job: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return nil, fmt.Errorf("fetch job (%d): %s", resp.StatusCode, body) + } + + var result struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, fmt.Errorf("parse job: %w", err) + } + if len(result.Jobs) == 0 { + return nil, fmt.Errorf("%w: %d", ErrJobNotFound, jobID) + } + return &result.Jobs[0], nil +} + +func (a daemonReviewAPI) getReview(ctx context.Context, jobID int64, label string) (*storage.Review, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("%s/api/review?job_id=%d", a.baseURL, jobID), nil) + if err != nil { + return nil, fmt.Errorf("create %s request: %w", label, err) + } + + resp, err := a.client.Do(req) + if err != nil { + return nil, fmt.Errorf("fetch %s: %w", label, err) + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("%w: job %d", errReviewNotFound, jobID) + } + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return nil, fmt.Errorf("fetch %s (%d): %s", label, resp.StatusCode, body) + } + + var review storage.Review + if err := json.NewDecoder(resp.Body).Decode(&review); err != nil { + return nil, fmt.Errorf("parse %s: %w", label, err) + } + return &review, nil +} diff --git a/cmd/roborev/daemon_client.go b/cmd/roborev/daemon_client.go index 6f5d35004..95fad9127 100644 --- a/cmd/roborev/daemon_client.go +++ b/cmd/roborev/daemon_client.go @@ -2,7 +2,9 @@ package main import ( "bytes" + "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -19,8 +21,11 @@ import ( // waitForJob polls until a job completes and displays the review // Uses the provided serverAddr to ensure we poll the same daemon that received the job. func waitForJob(cmd *cobra.Command, ep daemon.DaemonEndpoint, jobID int64, quiet bool) error { - serverAddr := ep.BaseURL() - client := ep.HTTPClient(5 * time.Second) + ctx := cmd.Context() + if ctx == nil { + ctx = context.Background() + } + api := newDaemonReviewAPI(ep.BaseURL(), ep.HTTPClient(5*time.Second)) if !quiet { cmd.Printf("Waiting for review to complete...") @@ -33,33 +38,11 @@ func waitForJob(cmd *cobra.Command, ep daemon.DaemonEndpoint, jobID int64, quiet const maxUnknownRetries = 10 // Give up after 10 consecutive unknown statuses for { - resp, err := client.Get(fmt.Sprintf("%s/api/jobs?id=%d", serverAddr, jobID)) + job, err := api.getJob(ctx, jobID) if err != nil { return fmt.Errorf("failed to check job status: %w", err) } - // Handle non-200 responses - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - resp.Body.Close() - return fmt.Errorf("server error checking job status (%d): %s", resp.StatusCode, body) - } - - var jobsResp struct { - Jobs []storage.ReviewJob `json:"jobs"` - } - if err := json.NewDecoder(resp.Body).Decode(&jobsResp); err != nil { - resp.Body.Close() - return fmt.Errorf("failed to parse job status: %w", err) - } - resp.Body.Close() - - if len(jobsResp.Jobs) == 0 { - return fmt.Errorf("%w: %d", ErrJobNotFound, jobID) - } - - job := jobsResp.Jobs[0] - switch job.Status { case storage.JobStatusDone: if !quiet { @@ -111,24 +94,17 @@ func waitForJob(cmd *cobra.Command, ep daemon.DaemonEndpoint, jobID int64, quiet // showReview fetches and displays a review by job ID // When quiet is true, suppresses output but still returns exit code based on verdict. func showReview(cmd *cobra.Command, ep daemon.DaemonEndpoint, jobID int64, quiet bool) error { - client := ep.HTTPClient(5 * time.Second) - resp, err := client.Get(fmt.Sprintf("%s/api/review?job_id=%d", ep.BaseURL(), jobID)) - if err != nil { - return fmt.Errorf("failed to fetch review: %w", err) + ctx := cmd.Context() + if ctx == nil { + ctx = context.Background() } - defer resp.Body.Close() - - if resp.StatusCode == http.StatusNotFound { + api := newDaemonReviewAPI(ep.BaseURL(), ep.HTTPClient(5*time.Second)) + review, err := api.getReview(ctx, jobID, "review") + if errors.Is(err, errReviewNotFound) { return fmt.Errorf("no review found for job %d", jobID) } - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("server error fetching review (%d): %s", resp.StatusCode, body) - } - - var review storage.Review - if err := json.NewDecoder(resp.Body).Decode(&review); err != nil { - return fmt.Errorf("failed to parse review: %w", err) + if err != nil { + return err } if !quiet { @@ -234,59 +210,12 @@ func waitForReview(jobID int64) (*storage.Review, error) { } func waitForReviewWithInterval(jobID int64, pollInterval time.Duration) (*storage.Review, error) { - ep := getDaemonEndpoint() - addr := ep.BaseURL() - client := ep.HTTPClient(10 * time.Second) - - for { - resp, err := client.Get(fmt.Sprintf("%s/api/jobs?id=%d", addr, jobID)) - if err != nil { - return nil, fmt.Errorf("polling job %d: %w", jobID, err) - } - - if resp.StatusCode != http.StatusOK { - resp.Body.Close() - return nil, fmt.Errorf("polling job %d: server returned %s", jobID, resp.Status) - } - - var result struct { - Jobs []storage.ReviewJob `json:"jobs"` - } - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - resp.Body.Close() - return nil, fmt.Errorf("polling job %d: decode error: %w", jobID, err) - } - resp.Body.Close() - - if len(result.Jobs) == 0 { - return nil, fmt.Errorf("job %d not found", jobID) - } - - job := result.Jobs[0] - switch job.Status { - case storage.JobStatusDone: - // Get the review - reviewResp, err := client.Get(fmt.Sprintf("%s/api/review?job_id=%d", addr, jobID)) - if err != nil { - return nil, err - } - defer reviewResp.Body.Close() - - var review storage.Review - if err := json.NewDecoder(reviewResp.Body).Decode(&review); err != nil { - return nil, err - } - return &review, nil - - case storage.JobStatusFailed: - return nil, fmt.Errorf("job failed: %s", job.Error) - - case storage.JobStatusCanceled: - return nil, fmt.Errorf("job was canceled") - } - - time.Sleep(pollInterval) + client, err := daemon.NewHTTPClientFromRuntime() + if err != nil { + return nil, err } + client.SetPollInterval(pollInterval) + return client.WaitForReview(jobID) } // enqueueReview enqueues a review job and returns the job ID diff --git a/cmd/roborev/job_helpers.go b/cmd/roborev/job_helpers.go index 6b989a62e..8976c0a27 100644 --- a/cmd/roborev/job_helpers.go +++ b/cmd/roborev/job_helpers.go @@ -4,10 +4,8 @@ package main import ( "context" - "encoding/json" "fmt" "io" - "net/http" "time" "github.com/roborev-dev/roborev/internal/storage" @@ -16,7 +14,7 @@ import ( // waitForJobCompletion polls a job until it completes, streaming output if provided. // This consolidates polling logic used across compact, analyze, fix, and run commands. func waitForJobCompletion(ctx context.Context, serverAddr string, jobID int64, output io.Writer) (*storage.Review, error) { - client := getDaemonHTTPClient(30 * time.Second) + api := newDaemonReviewAPI(serverAddr, getDaemonHTTPClient(30*time.Second)) pollInterval := 1 * time.Second maxInterval := 5 * time.Second lastOutputLen := 0 @@ -30,37 +28,11 @@ func waitForJobCompletion(ctx context.Context, serverAddr string, jobID int64, o case <-time.After(pollInterval): } - // Check job status - req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/api/jobs?id=%d", serverAddr, jobID), nil) + job, err := api.getJob(ctx, jobID) if err != nil { - return nil, fmt.Errorf("create request: %w", err) - } - - resp, err := client.Do(req) - if err != nil { - continue // Keep polling - } - - if resp.StatusCode != http.StatusOK { - resp.Body.Close() continue } - var jobsResp struct { - Jobs []storage.ReviewJob `json:"jobs"` - } - if err := json.NewDecoder(resp.Body).Decode(&jobsResp); err != nil { - resp.Body.Close() - continue - } - resp.Body.Close() - - if len(jobsResp.Jobs) == 0 { - return nil, fmt.Errorf("job %d not found", jobID) - } - - job := jobsResp.Jobs[0] - // Show status progress indicator while waiting if output != nil && lastStatus != string(job.Status) { lastStatus = string(job.Status) @@ -90,50 +62,22 @@ func waitForJobCompletion(ctx context.Context, serverAddr string, jobID int64, o // Stream partial output while running if output != nil && job.Status == storage.JobStatusRunning { - reviewReq, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/api/review?job_id=%d", serverAddr, jobID), nil) - if err == nil { - reviewResp, err := client.Do(reviewReq) - if err == nil { - if reviewResp.StatusCode == http.StatusOK { - var review storage.Review - if json.NewDecoder(reviewResp.Body).Decode(&review) == nil { - if len(review.Output) > lastOutputLen { - // First output - add newline after waiting dots - if lastOutputLen == 0 && waitDots > 0 { - fmt.Fprintln(output) - } - fmt.Fprint(output, review.Output[lastOutputLen:]) - lastOutputLen = len(review.Output) - } - } - } - reviewResp.Body.Close() + review, err := api.getReview(ctx, jobID, "review") + if err == nil && len(review.Output) > lastOutputLen { + // First output - add newline after waiting dots + if lastOutputLen == 0 && waitDots > 0 { + fmt.Fprintln(output) } + fmt.Fprint(output, review.Output[lastOutputLen:]) + lastOutputLen = len(review.Output) } } switch job.Status { case storage.JobStatusDone: - // Fetch final review - reviewReq, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/api/review?job_id=%d", serverAddr, jobID), nil) - if err != nil { - return nil, fmt.Errorf("create review request: %w", err) - } - - reviewResp, err := client.Do(reviewReq) + review, err := api.getReview(ctx, jobID, "review") if err != nil { - return nil, fmt.Errorf("fetch review: %w", err) - } - defer reviewResp.Body.Close() - - if reviewResp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(reviewResp.Body) - return nil, fmt.Errorf("fetch review (%d): %s", reviewResp.StatusCode, body) - } - - var review storage.Review - if err := json.NewDecoder(reviewResp.Body).Decode(&review); err != nil { - return nil, fmt.Errorf("parse review: %w", err) + return nil, err } // Print any remaining output @@ -141,7 +85,7 @@ func waitForJobCompletion(ctx context.Context, serverAddr string, jobID int64, o fmt.Fprint(output, review.Output[lastOutputLen:]) } - return &review, nil + return review, nil case storage.JobStatusFailed: return nil, fmt.Errorf("job failed: %s", job.Error) diff --git a/cmd/roborev/run.go b/cmd/roborev/run.go index 6920cab40..7fe1b859f 100644 --- a/cmd/roborev/run.go +++ b/cmd/roborev/run.go @@ -2,7 +2,9 @@ package main import ( "bytes" + "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -195,8 +197,11 @@ var promptPollInterval = 500 * time.Millisecond // Unlike waitForJob, this doesn't apply verdict-based exit codes since prompt // jobs don't have PASS/FAIL verdicts. func waitForPromptJob(cmd *cobra.Command, ep daemon.DaemonEndpoint, jobID int64, quiet bool, pollInterval time.Duration) error { - serverAddr := ep.BaseURL() - client := ep.HTTPClient(5 * time.Second) + ctx := cmd.Context() + if ctx == nil { + ctx = context.Background() + } + api := newDaemonReviewAPI(ep.BaseURL(), ep.HTTPClient(5*time.Second)) if pollInterval <= 0 { pollInterval = promptPollInterval @@ -212,36 +217,15 @@ func waitForPromptJob(cmd *cobra.Command, ep daemon.DaemonEndpoint, jobID int64, const maxUnknownRetries = 10 // Give up after 10 consecutive unknown statuses for { - resp, err := client.Get(fmt.Sprintf("%s/api/jobs?id=%d", serverAddr, jobID)) + job, err := api.getJob(ctx, jobID) if err != nil { return fmt.Errorf("failed to check job status: %w", err) } - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - resp.Body.Close() - return fmt.Errorf("server error checking job status (%d): %s", resp.StatusCode, body) - } - - var jobsResp struct { - Jobs []storage.ReviewJob `json:"jobs"` - } - if err := json.NewDecoder(resp.Body).Decode(&jobsResp); err != nil { - resp.Body.Close() - return fmt.Errorf("failed to parse job status: %w", err) - } - resp.Body.Close() - - if len(jobsResp.Jobs) == 0 { - return fmt.Errorf("job %d not found", jobID) - } - - job := jobsResp.Jobs[0] - switch job.Status { case storage.JobStatusDone: // Pass done message to showPromptResult - it prints after successful fetch - return showPromptResult(cmd, serverAddr, jobID, quiet, " done!\n\n") + return showPromptResult(cmd, ep.BaseURL(), jobID, quiet, " done!\n\n") case storage.JobStatusFailed: if !quiet { @@ -285,24 +269,17 @@ func waitForPromptJob(cmd *cobra.Command, ep daemon.DaemonEndpoint, jobID int64, // Unlike showReview, this doesn't apply verdict-based exit codes. // The doneMsg parameter is printed before the result on success (used for "done!" message). func showPromptResult(cmd *cobra.Command, addr string, jobID int64, quiet bool, doneMsg string) error { - client := getDaemonHTTPClient(5 * time.Second) - resp, err := client.Get(fmt.Sprintf("%s/api/review?job_id=%d", addr, jobID)) - if err != nil { - return fmt.Errorf("failed to fetch result: %w", err) + ctx := cmd.Context() + if ctx == nil { + ctx = context.Background() } - defer resp.Body.Close() - - if resp.StatusCode == http.StatusNotFound { + api := newDaemonReviewAPI(addr, getDaemonHTTPClient(5*time.Second)) + review, err := api.getReview(ctx, jobID, "result") + if errors.Is(err, errReviewNotFound) { return fmt.Errorf("no result found for job %d", jobID) } - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("server error fetching result (%d): %s", resp.StatusCode, body) - } - - var review storage.Review - if err := json.NewDecoder(resp.Body).Decode(&review); err != nil { - return fmt.Errorf("failed to parse result: %w", err) + if err != nil { + return err } // Only print after successful fetch to avoid "done!" followed by error diff --git a/internal/daemon/client.go b/internal/daemon/client.go index bff070190..4dbc58c3d 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -87,8 +87,8 @@ func (c *HTTPClient) SetPollInterval(interval time.Duration) { c.pollInterval = interval } -func (c *HTTPClient) GetReviewBySHA(sha string) (*storage.Review, error) { - resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/review?sha=%s", c.baseURL, sha)) +func (c *HTTPClient) getReview(url string) (*storage.Review, error) { + resp, err := c.httpClient.Get(url) if err != nil { return nil, err } @@ -97,7 +97,6 @@ func (c *HTTPClient) GetReviewBySHA(sha string) (*storage.Review, error) { if resp.StatusCode == http.StatusNotFound { return nil, nil } - if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("server returned %s", resp.Status) } @@ -106,31 +105,15 @@ func (c *HTTPClient) GetReviewBySHA(sha string) (*storage.Review, error) { if err := json.NewDecoder(resp.Body).Decode(&review); err != nil { return nil, err } - return &review, nil } -func (c *HTTPClient) GetReviewByJobID(jobID int64) (*storage.Review, error) { - resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/review?job_id=%d", c.baseURL, jobID)) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode == http.StatusNotFound { - return nil, nil - } - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("server returned %s", resp.Status) - } - - var review storage.Review - if err := json.NewDecoder(resp.Body).Decode(&review); err != nil { - return nil, err - } +func (c *HTTPClient) GetReviewBySHA(sha string) (*storage.Review, error) { + return c.getReview(fmt.Sprintf("%s/api/review?sha=%s", c.baseURL, sha)) +} - return &review, nil +func (c *HTTPClient) GetReviewByJobID(jobID int64) (*storage.Review, error) { + return c.getReview(fmt.Sprintf("%s/api/review?job_id=%d", c.baseURL, jobID)) } func (c *HTTPClient) MarkReviewClosed(jobID int64) error { @@ -200,33 +183,36 @@ func (c *HTTPClient) EnqueueReview(repoPath, gitRef, agentName string) (int64, e return job.ID, nil } +func (c *HTTPClient) getJobByID(jobID int64) (*storage.ReviewJob, error) { + resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/jobs?id=%d", c.baseURL, jobID)) + if err != nil { + return nil, fmt.Errorf("polling job %d: %w", jobID, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("polling job %d: server returned %s", jobID, resp.Status) + } + + var result struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, fmt.Errorf("polling job %d: decode error: %w", jobID, err) + } + if len(result.Jobs) == 0 { + return nil, fmt.Errorf("job %d not found", jobID) + } + return &result.Jobs[0], nil +} + func (c *HTTPClient) WaitForReview(jobID int64) (*storage.Review, error) { missingReviewAttempts := 0 for { - resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/jobs?id=%d", c.baseURL, jobID)) + job, err := c.getJobByID(jobID) if err != nil { - return nil, fmt.Errorf("polling job %d: %w", jobID, err) - } - - if resp.StatusCode != http.StatusOK { - resp.Body.Close() - return nil, fmt.Errorf("polling job %d: server returned %s", jobID, resp.Status) - } - - var result struct { - Jobs []storage.ReviewJob `json:"jobs"` - } - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - resp.Body.Close() - return nil, fmt.Errorf("polling job %d: decode error: %w", jobID, err) - } - resp.Body.Close() - - if len(result.Jobs) == 0 { - return nil, fmt.Errorf("job %d not found", jobID) + return nil, err } - - job := result.Jobs[0] switch job.Status { case storage.JobStatusDone: review, err := c.GetReviewByJobID(jobID) From 49c2c4d2a329b4305ff39b489cf35fc0b9c493aa Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 25 Mar 2026 09:20:57 -0400 Subject: [PATCH 2/2] fix: address review finding and stabilize postgres test --- cmd/roborev/job_helpers.go | 4 ++++ cmd/roborev/job_helpers_test.go | 36 +++++++++++++++++++++++++++++++ internal/storage/postgres_test.go | 16 ++++++++++---- 3 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 cmd/roborev/job_helpers_test.go diff --git a/cmd/roborev/job_helpers.go b/cmd/roborev/job_helpers.go index 8976c0a27..69bd4ca6d 100644 --- a/cmd/roborev/job_helpers.go +++ b/cmd/roborev/job_helpers.go @@ -4,6 +4,7 @@ package main import ( "context" + "errors" "fmt" "io" "time" @@ -30,6 +31,9 @@ func waitForJobCompletion(ctx context.Context, serverAddr string, jobID int64, o job, err := api.getJob(ctx, jobID) if err != nil { + if errors.Is(err, ErrJobNotFound) { + return nil, err + } continue } diff --git a/cmd/roborev/job_helpers_test.go b/cmd/roborev/job_helpers_test.go new file mode 100644 index 000000000..a30cdf13d --- /dev/null +++ b/cmd/roborev/job_helpers_test.go @@ -0,0 +1,36 @@ +package main + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/roborev-dev/roborev/internal/storage" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWaitForJobCompletionReturnsNotFoundImmediately(t *testing.T) { + var jobCalls int + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/jobs": + jobCalls++ + writeJSON(w, map[string]any{"jobs": []storage.ReviewJob{}}) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 1500*time.Millisecond) + defer cancel() + + review, err := waitForJobCompletion(ctx, server.URL, 123, nil) + require.Error(t, err) + assert.Nil(t, review) + require.ErrorIs(t, err, ErrJobNotFound) + assert.Equal(t, 1, jobCalls, "expected not-found to fail fast instead of polling until timeout") +} diff --git a/internal/storage/postgres_test.go b/internal/storage/postgres_test.go index 577005f1a..1ea22add4 100644 --- a/internal/storage/postgres_test.go +++ b/internal/storage/postgres_test.go @@ -762,11 +762,19 @@ func TestIntegration_BatchUpsertJobs(t *testing.T) { require.NotNil(t, storedWT) assert.Equal(t, "/worktrees/feature-x", *storedWT) - // Also verify PullJobs returns the field: use a different - // valid UUID to exclude, and scope via cursor to avoid - // scanning the entire table. + // Also verify PullJobs returns the field. Scope the pull cursor to just before + // this row so the test doesn't depend on table size or test shuffle order. + var pulledCursor string + var rowID int64 + var updatedAt time.Time + err = pool.pool.QueryRow(ctx, + `SELECT updated_at, id FROM review_jobs WHERE uuid = $1`, wtJobUUID, + ).Scan(&updatedAt, &rowID) + require.NoError(t, err) + pulledCursor = fmt.Sprintf("%s %d", updatedAt.Format(time.RFC3339Nano), rowID-1) + otherMachine := uuid.NewString() - pulled, _, err := pool.PullJobs(ctx, otherMachine, "", 100) + pulled, _, err := pool.PullJobs(ctx, otherMachine, pulledCursor, 100) require.NoError(t, err) var found *PulledJob