feat: TUI-triggered fix via background worktrees (#279)

[mariusvniekerk]

Summary

• Add fix job type that runs agents in isolated git worktrees, capturing patches stored in DB
• Extract shared worktree helpers to internal/worktree package (reused by refine.go and worker pool)
• Add /api/job/fix and /api/job/patch API endpoints for creating fix jobs and retrieving patches
• Add TUI Tasks view (T), fix prompt modal (F), patch apply (A), and automatic rebase when patches are stale
• Update help text, hint bars, CLAUDE.md, and plan doc to match implementation

Test plan

• Run go test ./... — all 2531 tests pass
• Run go vet ./... — clean
• Manual test: open TUI, select a completed review, press F to trigger fix, monitor in Tasks view (T), apply patch (A)
• Manual test: verify rebase flow triggers when applying a stale patch

Closes #279

🤖 Generated with Claude Code

[roborev-ci]

roborev: Combined Review

Verdict: Not ready to merge; the PR has 3 High and 5 Medium issues requiring fixes.

Critical

None.

High

1. Missing auth/authz on new fix/patch endpoints

   • Refs: internal/daemon/server.go:1494, internal/daemon/server.go:1496, internal/daemon/server.go:1573, internal/daemon/server.go:1576
   • /api/job/fix and /api/job/patch can be called without authentication/authorization, allowing unauthorized job execution and patch retrieval.
   • Add request authentication and per-request authorization; fail closed outside trusted local-only mode.

2. /api/jobs response decoding mismatch breaks Tasks/fix flows

   • Refs: cmd/roborev/tui.go:3599, cmd/roborev/tui.go:3659, cmd/roborev/tui.go:3708, internal/daemon/server.go:812
   • TUI decodes /api/jobs as []ReviewJob, but server returns an object (jobs, has_more, stats), causing decode failures and downstream breakage.
   • Decode into a wrapper struct matching the API payload.

3. Fix-job prompt/patch-capture contract conflict

   • Refs: internal/daemon/server.go:1613, cmd/roborev/tui.go:3738, internal/daemon/worker.go:399, internal/worktree/worktree.go:73
   • Prompt tells agent to create a commit, but patch capture reads staged diff; committed changes can yield empty patch (“no patch available”).
   • Either instruct agents not to commit for this path, or capture committed deltas explicitly.

Medium

1. IDOR / missing scope validation on job IDs

   • Refs: internal/daemon/server.go:1508, internal/daemon/server.go:1588
   • Caller-controlled parent_job_id / job_id are used without ownership/scope checks, enabling cross-job access in shared-daemon scenarios.
   • Validate job ownership/scope and expected job type/status before acting/returning patch data.

2. Prompt-injection risk from unescaped patch text in rebase prompt

   • Refs: cmd/roborev/tui.go:3749, cmd/roborev/tui.go:3512
   • stalePatch is inserted into fenced markdown; embedded backticks can break fencing and alter model instructions.
   • Use variable-length fencing (longer than any backtick run) or escape/encode patch content.

3. Over-broad handling of git apply --check failures

   • Refs: cmd/roborev/tui.go:3674, cmd/roborev/tui.go:2099
   • All apply-check failures are treated as “stale patch” and auto-rebased, masking other real errors.
   • Rebase only on hunk/context mismatch; surface other error causes directly.

4. Terminal escape injection in Tasks rendering

   • Refs: cmd/roborev/tui.go:3545, cmd/roborev/tui.go:3546
   • GitRef/CommitSubject are rendered without sanitization, allowing ANSI/OSC injection via commit metadata.
   • Sanitize display fields before truncation/rendering.

5. Unsafe protocol.file.allow=always enablement from .gitmodules

   • Refs: internal/worktree/worktree.go:34
   • Auto-enabling file protocol based on repo metadata expands trust boundary and can expose local paths on untrusted repos.
   • Default deny; require explicit opt-in/allowlist for file protocol usage.

---

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

[roborev-ci]

roborev: Combined Review

Verdict: Not ready to merge; there are 2 High and 5 Medium issues (security + runtime correctness).

High

1. Unauthenticated/CSRF-exposed sensitive fix APIs (/api/job/fix, /api/job/patch)

   • Refs: internal/daemon/server.go:1490, internal/daemon/server.go:1493, internal/daemon/server.go:1496, internal/daemon/server.go:1570
   • Issue: State-changing fix job creation and patch retrieval are exposed without auth/authz and without CSRF protections. If daemon binding is broadened (or via localhost browser requests), this can allow unauthorized job triggering and patch exfiltration.
   • Suggested fix: Require daemon auth token (or equivalent) for /api/*, and enforce Origin/Referer (or CSRF token) checks for state-changing endpoints; keep loopback-only default binding.

2. TUI runtime regression: wrong JSON decode shape for /api/jobs

   • Refs: cmd/roborev/tui.go:3599, cmd/roborev/tui.go:3659, cmd/roborev/tui.go:3708, internal/daemon/server.go:812
   • Issue: TUI paths decode /api/jobs into []ReviewJob, but server returns an object wrapper ({"jobs": ..., "has_more": ...}), causing fix/task/apply/rebase lookup failures at runtime.
   • Suggested fix: Decode into a response wrapper struct (with jobs) or use a single-job endpoint where appropriate.

Medium

1. Unbounded request body in handleFixJob (DoS risk)

   • Refs: internal/daemon/server.go:1503, internal/daemon/server.go:1506
   • Issue: json.NewDecoder(r.Body) reads without size cap; large payloads can consume memory/CPU.
   • Suggested fix: Wrap with http.MaxBytesReader (same pattern as enqueue handler).

2. Prompt-injection risk from raw stale patch interpolation in rebase flow

   • Refs: cmd/roborev/tui.go:2162, cmd/roborev/tui.go:~3700
   • Issue: Untrusted patch text is embedded directly into prompt fences, enabling instruction injection/breakout.
   • Suggested fix: Escape fence delimiters or pass patch via a non-instructional channel/encoding; reinforce system guardrails.

3. Fix prompt vs patch-capture mismatch can produce empty patch output

   • Refs: internal/daemon/server.go:1616, internal/worktree/worktree.go:79, internal/daemon/worker.go:399
   • Issue: Prompt asks agent to commit, but capture logic uses staged diff vs HEAD; committed changes can result in empty captured patch.
   • Suggested fix: Either remove “create commit” instruction or capture diff relative to pre-run HEAD including committed changes.

4. Stale patch persistence across reruns

   • Refs: internal/storage/jobs.go:1093, internal/daemon/worker.go:404, internal/daemon/server.go:1593
   • Issue: Rerun/reset doesn’t reliably clear review_jobs.patch; when rerun yields no new changes, old patch may still be served.
   • Suggested fix: Clear patch on rerun/start or always overwrite patch state (including empty/null).

5. Missing test coverage on new fix flows contributed to regressions

   • Refs: internal/daemon/server.go:1496, cmd/roborev/tui.go:3595
   • Issue: No focused tests for /api/job/fix, /api/job/patch, and TUI apply/rebase/task paths.
   • Suggested fix: Add handler + TUI flow tests for happy path and failure cases.

---

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

[roborev-ci]

roborev: Combined Review

Verdict: The change is not ready to merge due to 1 High and 4 Medium issues affecting security and workflow safety.

High

1. Prompt-injection path into agentic execution
   • Location: internal/daemon/server.go:1518, internal/daemon/server.go:1556, cmd/roborev/tui.go:3670
   • Finding: fixPrompt is built from untrusted review.Output and queued as Agentic: true; rebase flow also embeds raw patch text into prompts. This allows attacker-controlled text to become executable agent instructions.
   • Suggested fix: Treat review/patch text as untrusted data, enforce non-overridable guardrails (ignore embedded instructions), and require explicit approval before agentic command execution in fix/rebase flows.

Medium

2. No auth/authz on new fix/patch endpoints (IDOR-style access)

   • Location: internal/daemon/server.go:1494, internal/daemon/server.go:1574
   • Finding: POST /api/job/fix and GET /api/job/patch allow direct job references without authentication/authorization checks; local callers can trigger/fetch other jobs’ artifacts.
   • Suggested fix: Add daemon API auth, enforce per-job authorization, and restrict patch retrieval by job type/status/caller.

3. Blind patch apply without required inspection

   • Location: cmd/roborev/tui.go:2098, cmd/roborev/tui.go:3594 (Tasks A apply path)
   • Finding: Users can apply generated patches without a mandatory preview/summary step, increasing risk of silently applying harmful or incorrect changes.
   • Suggested fix: Add a required patch-view/summary confirmation before enabling apply.

4. Unsafe automatic enabling of protocol.file.allow=always

   • Location: internal/worktree/worktree.go:46, internal/worktree/worktree.go:58
   • Finding: Behavior is auto-enabled based on .gitmodules, which is repo-controlled input; this weakens Git’s default protections and can expose sensitive local files via file:// submodules.
   • Suggested fix: Disable by default and gate behind explicit trusted-user config.

5. Incorrectly treating all patch-check errors as “stale” (auto-rebase)

   • Location: cmd/roborev/tui.go:2088, cmd/roborev/tui.go:3600
   • Finding: Non-staleness errors (malformed patch/path/repo issues) can be misclassified as stale and trigger rebase jobs, masking root causes and creating incorrect follow-on actions.
   • Suggested fix: Classify CheckPatch() errors and only auto-rebase on true conflict/staleness conditions.

---

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

[wesm]

@mariusvniekerk if you rebase, there are better review_guidelines which may help with the noisy security reviews

[mariusvniekerk]

Hmmm think i'm already rebased.

[mariusvniekerk]

rebased on main due to conflicts.

[roborev-ci]

roborev: Combined Review

Verdict: Changes introduce multiple security/correctness risks that should be addressed before merge.

High

1. Unauthenticated fix-job creation can trigger agentic command-capable runs

   • Refs: internal/daemon/server.go:1527, internal/agent/claude.go:95, internal/agent/claude.go:97
   • POST /api/job/fix lacks auth/authorization (and CSRF protections), but enqueues Agentic: true jobs. This enables untrusted local callers/pages/processes to trigger fix jobs.
   • Recommended fix: Require authenticated/authorized callers for state-changing endpoints, add CSRF protection, and restrict agentic fix creation to trusted clients.

2. Patch data is exposed without access control

   • Refs: internal/daemon/server.go:1601, internal/daemon/server.go:1630, internal/daemon/server.go:717, internal/storage/jobs.go:1381, internal/storage/models.go:69
   • Patch content is retrievable via GET /api/job/patch and indirectly via job fetch paths that now include Patch, which can leak sensitive code/secrets.
   • Recommended fix: Enforce authz on patch access, return patches only for eligible completed fix jobs, and remove Patch from generic job responses unless explicitly requested by privileged callers.

Medium

1. Fix patch persistence/state transitions are inconsistent and non-atomic

   • Refs: internal/daemon/worker.go:401, internal/daemon/worker.go:403, internal/storage/jobs.go:938, internal/daemon/server.go:1625
   • Current flow can produce invalid states: patch save/capture failures may still end as done, and patch writes are not atomic with final status transition, so canceled jobs can still retain retrievable patches.
   • Recommended fix: Make patch persistence atomic with final status transition, fail fix jobs when expected patch capture/save fails, and gate patch retrieval on strict final-state checks.

2. Unbounded patch handling can cause resource/token exhaustion

   • Refs: internal/daemon/worker.go:403, internal/storage/jobs.go:938, cmd/roborev/tui.go:3547, cmd/roborev/tui.go:3590, cmd/roborev/tui.go:210
   • Patch size is effectively unbounded in storage/fetch/apply paths, and stale patch text is injected into rebase prompts, risking memory/disk pressure and model context overflow.
   • Recommended fix: Add patch size caps at capture/save/API boundaries, use bounded reads/streaming apply, and truncate/summarize patch content before prompt injection.

3. refine may drop committed worktree changes

   • Refs: cmd/roborev/refine.go:549
   • Pre-checking git.IsWorkingTreeClean(worktreePath) can incorrectly report “no changes” when agent-created commits make the tree clean, causing patch capture to be skipped.
   • Recommended fix: Remove the clean-tree precheck and determine change presence from CapturePatch() result.

---

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

[roborev-ci]

roborev: Combined Review

Verdict: Changes introduce one significant security regression and multiple medium-risk prompt-handling/control-flow issues that should be addressed before merge.

High

1. Git file-protocol security bypass via repo-controlled .gitmodules

   • Refs: internal/worktree/worktree.go:46, internal/worktree/worktree.go:47, internal/worktree/worktree.go:48, internal/worktree/worktree.go:58, internal/worktree/worktree.go:59, internal/worktree/worktree.go:149, internal/worktree/worktree.go:151
   • Issue: Code auto-enables -c protocol.file.allow=always when .gitmodules indicates file/relative submodules. This lets untrusted repository content relax Git security defaults.
   • Why it matters: Can permit local-path/file submodule resolution in malicious repos (CVE-class risk).
   • Fix direction: Remove auto-enable behavior; require explicit user opt-in/config for local file submodules.

2. Unauthenticated high-impact daemon endpoints

   • Refs: internal/daemon/server.go:1525, internal/daemon/server.go:1600
   • Issue: /api/job/fix and /api/job/patch expose job execution/patch retrieval without authz/authn checks.
   • Why it matters: Any local caller that can reach the daemon can trigger agentic jobs or read patch data.
   • Fix direction: Add daemon auth (token/socket/mTLS), authorization scoping, and CSRF-style protections for localhost HTTP clients.

Medium

1. Prompt-injection path from review output into agentic fix jobs

   • Refs: internal/daemon/server.go:1554, internal/daemon/server.go:1636, internal/daemon/worker.go:332, internal/daemon/worker.go:361
   • Issue: buildFixPrompt directly embeds review output into instructions for an Agentic: true fixer.
   • Why it matters: Untrusted review/code-derived text can steer command-capable fix execution.
   • Fix direction: Treat findings as untrusted structured data, strongly delimit/escape content, and explicitly instruct model to ignore embedded instructions.

2. Prompt-injection/robustness risk in rebase prompt patch embedding

   • Refs: cmd/roborev/tui.go:3490
   • Issue: triggerRebase injects raw stale patch text into prompt fencing.
   • Why it matters: Crafted patch content can break delimiters/instruction boundaries and distort model behavior.
   • Fix direction: Pass patch as a separate artifact/field (preferred), or encode/escape + enforce size limits.

3. Over-broad auto-rebase trigger on any patch-check error

   • Refs: cmd/roborev/tui.go:3563, cmd/roborev/tui.go:2072
   • Issue: Any CheckPatch error is treated as stale/conflict and triggers rebase flow.
   • Why it matters: Operational failures (not true staleness) can incorrectly enqueue rebase jobs.
   • Fix direction: Use typed errors from patch check and only auto-rebase for conflict/stale conditions.

---

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

[roborev-ci]

roborev: Combined Review

Verdict: Not ready to merge — 1 high and 5 medium issues remain after deduplication.

High

• Missing authz on fix-job endpoint enables unauthorized agentic edits
  • Location: internal/daemon/server.go:1525 (handleFixJob)
  • Issue: POST /api/job/fix is state-changing and accepts caller-controlled prompts for agentic fix jobs, but no authentication/authorization is enforced.
  • Risk: Any local process that can reach the daemon can enqueue autonomous code-modifying jobs.
  • Fix: Require API authentication + per-repo/job authorization; gate or restrict arbitrary prompt overrides.

Medium

• Patch retrieval endpoint allows IDOR-style access to patch contents

  • Location: internal/daemon/server.go:1609 (handleGetPatch)
  • Issue: Patch data is retrievable by job_id without access control checks.
  • Risk: Unauthorized access to potentially sensitive patch/code content.
  • Fix: Enforce authenticated, authorized access; return 403 when unauthorized.

• Prompt injection risk in rebase flow via untrusted patch interpolation

  • Location: cmd/roborev/tui.go:3559 (triggerRebase)
  • Issue: stalePatch is embedded directly into prompt text using fixed markdown fencing.
  • Risk: Crafted patch content can break fencing and inject instructions into agentic execution context.
  • Fix: Escape/dynamically harden delimiters or pass patch as structured attachment/context instead of inline text.

• Rebase flow can fail for large patches due to request size cap mismatch

  • Location: cmd/roborev/tui.go (triggerRebase), internal/daemon/server.go:1530
  • Issue: Rebase embeds full stale patch into /api/job/fix request, while server enforces ~1MB body limit.
  • Risk: Larger rebases fail with request-size errors.
  • Fix: Reference stored patch/job ID instead of inlining full patch, or redesign size handling with explicit limits/error UX.

• Fix action eligibility check does not enforce failing-review requirement

  • Location: cmd/roborev/tui_handlers.go:1340 (handleFixKey)
  • Issue: Code allows fix triggering for any completed job, not strictly failing review jobs.
  • Risk: Misleading/unsafe fix generation from PASS or non-review jobs.
  • Fix: Require failing verdict (and ideally review-capable job type) before enabling fix flow.

• Submodule update weakens Git file-protocol safety globally

  • Location: internal/worktree/worktree.go:46, internal/worktree/worktree.go:59
  • Issue: Detecting any local/file submodule enables protocol.file.allow=always for the whole submodule update command.
  • Risk: Broader-than-needed local file protocol exposure across submodules.
  • Fix: Avoid global override; require explicit opt-in and/or validate allowed local paths before enabling.

---

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

[roborev-ci]

roborev: Combined Review

Verdict: Significant security and correctness risks remain; fix before merge.

High

• Unauthenticated fix-job creation enables localhost abuse
  • Location: internal/daemon/server.go:1526
  • POST /api/job/fix accepts user-controlled prompts and enqueues agentic fix jobs without auth, creating a high-impact local attack surface (including CSRF-style localhost abuse).

Medium

• Patch retrieval endpoint lacks authorization/scope checks

  • Location: internal/daemon/server.go:1612
  • GET /api/job/patch returns raw patch content by job_id without authorization or repo/job ownership checks, risking disclosure of sensitive changes.

• stale_job_id can reference unrelated jobs/repos

  • Location: internal/daemon/server.go:1556
  • stale_job_id is used without validating repo/parent linkage, allowing unrelated patch content to be injected into rebase/fix flow.

• Prompt-injection risk from raw patch embedding

  • Location: internal/daemon/server.go:1665
  • buildRebasePrompt embeds patch text directly in fenced prompt content; delimiter-breaking content can inject instructions into downstream model runs.

• Fix jobs may be missing in TUI due to client-side filtering over paged /api/jobs

  • Location: cmd/roborev/tui.go (fetchFixJobs)
  • If /api/jobs is paginated/limited, non-fix jobs can crowd out fix jobs before client-side filtering.

• Auto-rebase may trigger on dirty-worktree conflicts (not just stale-base conflicts)

  • Location: cmd/roborev/tui.go (applyFixPatch)
  • Treating all PatchConflictError cases as stale-base can enqueue unnecessary rebase jobs and mask local state issues.

---

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

[roborev-ci]

roborev: Combined Review (e4b41302)

Verdict: Changes are not clean yet — 1 High and 2 Medium issues need to be addressed before merge.

High

1. Untrusted .gitmodules can enable local file-protocol submodule fetches
   • Files: internal/worktree/worktree.go:46, internal/worktree/worktree.go:58
   • The code conditionally enables -c protocol.file.allow=always based on repository content (.gitmodules). In untrusted/shared repos, attacker-controlled submodule metadata can trigger local filesystem-backed fetches during fix jobs.
   • Risk: unintended local data access/exfiltration into worktree/agent processing.
   • Suggested fix: do not auto-enable file protocol from repo content; require explicit trusted-repo opt-in and strict path allowlisting (or fail closed with clear error).

Medium

1. Terminal escape injection in Tasks view

   • File: cmd/roborev/tui.go:3333 (renderTasksView)
   • job.GitRef and job.CommitSubject are rendered without display sanitization.
   • Risk: ANSI/control-sequence injection can spoof or hide terminal output.
   • Suggested fix: apply sanitizeForDisplay(...) to these fields before truncation/rendering.

2. Pagination regression from client-side fix-job filtering

   • Files: cmd/roborev/tui_handlers.go:1092, cmd/roborev/tui.go:542
   • Queue rows are filtered client-side, but pagination offset still uses len(m.jobs), which no longer matches server-side row progression.
   • Risk: duplicate rows, skipped rows, incorrect has_more.
   • Suggested fix: filter server-side for queue requests (preferred), or track offset using unfiltered server row count.

3. Tasks view can miss fix jobs due to paginated mixed-job fetch

   • File: cmd/roborev/tui.go (fetchFixJobs)
   • fetchFixJobs fetches paginated /api/jobs then filters for fix jobs locally.
   • Risk: Tasks may appear empty when fix jobs exist beyond a mixed first page.
   • Suggested fix: request job_type=fix server-side or fetch unpaginated (limit=0) before filtering.

---

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

[roborev-ci]

roborev: Combined Review (d2167587)

Verdict: Changes introduce 1 High and 5 Medium issues that should be addressed before merge.

High

1. Queue pagination breaks after client-side fix-job filtering
   • Files: cmd/roborev/tui_handlers.go:1095, cmd/roborev/tui.go:672
   • Issue: Pagination is done server-side across all jobs, but fix jobs are filtered out client-side. Using offset := len(m.jobs) after filtering misaligns subsequent requests, which can duplicate rows and skip older non-fix jobs.
   • Suggested fix: Filter out fix jobs server-side for queue requests (for example job_type!=fix), or track/use unfiltered row count for offsets.

Medium

1. Security: repo-controlled .gitmodules can auto-enable local file protocol

   • Files: internal/worktree/worktree.go:45, internal/worktree/worktree.go:58, internal/worktree/worktree.go:170
   • Issue: Auto-enabling protocol.file.allow=always from .gitmodules allows untrusted repos to force local-path/file submodule resolution, risking unintended local data exposure to downstream processing.
   • Suggested fix: Require explicit user opt-in (config/flag), and restrict to trusted repos/allowlist.

2. Tasks view can miss fix jobs due to client-side filtering after default page limit

   • Files: cmd/roborev/tui.go:3514, internal/daemon/server.go:751
   • Issue: fetchFixJobs() pulls /api/jobs with default limit (50) and then filters to fix jobs. If the first 50 are non-fix, fix jobs may exist but tasks appear empty.
   • Suggested fix: Add/use server-side filter (for example /api/jobs?job_type=fix).

3. Rebase path lacks stale/parent relationship validation

   • Files: internal/daemon/server.go:1558, internal/daemon/server.go:1590
   • Issue: stale_job_id patch can be queued against parent_job_id without verifying lineage/repo consistency/completion state, allowing invalid rebase pairings.
   • Suggested fix: Validate stale job is fix, done, has non-empty patch, matches RepoID, and is correctly linked to parent.

4. Missing endpoint tests for new fix/rebase flows and validation paths

   • File: internal/daemon/server_test.go
   • Issue: No coverage for /api/job/fix and /api/job/patch success/error cases (missing parent, invalid stale linkage, stale without patch, etc.).
   • Suggested fix: Add table-driven tests for these cases.

5. Missing TUI tests for fix filtering + pagination behavior

   • File: cmd/roborev/tui_test.go
   • Issue: No tests for mixed fix/non-fix pagination interactions in queue/tasks flows.
   • Suggested fix: Add tests simulating mixed pages and verify queue/task pagination correctness.

---

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

[roborev-ci]

roborev: Combined Review (37d4c53c)

Verdict: Changes are not clean; there are 4 deduplicated findings to address (2 High, 2 Medium).

High

1. Unsafe submodule file protocol enablement

   • File/lines: internal/worktree/worktree.go:46, internal/worktree/worktree.go:59
   • Issue: Create() conditionally enables git -c protocol.file.allow=always submodule update ... based on .gitmodules. In untrusted/shared repos, repo-controlled submodule config can abuse file:// sources and bypass safer Git defaults.
   • Recommendation: Do not auto-enable protocol.file.allow=always from repo content; require explicit user opt-in or strict URL/path allowlisting.

2. Fix patch commit can include unrelated workspace changes (reported by multiple agents)

   • File/refs: cmd/roborev/tui.go (applyFixPatch, commitPatch)
   • Issue: Flow applies patch then commits via git add -A + git commit, which can silently include unrelated modified/untracked files (including sensitive data) if the tree is not clean.
   • Recommendation: Enforce clean working tree/index before apply, or use indexed patch application and commit only intended staged changes.

Medium

1. Terminal escape injection risk in patch viewer

   • File/line: cmd/roborev/tui.go:~3600 (renderPatchView)
   • Issue: Patch lines are rendered directly (display := line then write), allowing untrusted ANSI/OSC/control sequences from patch content to execute in terminal contexts.
   • Recommendation: Sanitize/strip control/escape sequences before rendering (or escape non-printable bytes).

2. Client-side fix-job filtering breaks pagination correctness

   • File/lines: cmd/roborev/tui.go:542 (fetchMoreJobs), cmd/roborev/tui_handlers.go:1108 (handleJobsMsg)
   • Issue: Offset uses len(m.jobs) after client-side filtering, while server pagination is over unfiltered jobs, which can cause skipped/duplicated entries.
   • Recommendation: Move filtering server-side or track pagination offsets against unfiltered server rows.

---

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

[roborev-ci]

roborev: Combined Review (6e39cd62)

Verdict: Changes introduce multiple High and Medium risks that should be addressed before merge.

High

• Unsafe local file protocol enablement for submodules
  Files: internal/worktree/worktree.go:46, internal/worktree/worktree.go:58
  The code conditionally enables protocol.file.allow=always for git submodule update based on .gitmodules. In shared/untrusted repos, malicious .gitmodules entries can reference local filesystem repos and pull unintended local data into the worktree, with downstream exposure risk in AI review/fix flows.
  Suggested fix: Do not auto-enable file protocol by default; require explicit trusted-repo opt-in and reject absolute/UNC/file:// paths (or allow only tightly constrained relative paths).

• Applying a fix can commit unrelated workspace changes
  File: cmd/roborev/tui.go (reported around modified commitPatch / applyFixPatch; e.g., ~line 384 in one review)
  commitPatch uses git add -A before commit, which stages all changes (including unrelated edits/untracked files), creating accidental commit/data exposure risk.
  Suggested fix: Stage only patch-introduced changes (for example, apply with index via git apply --index) or enforce a clean working tree before apply/commit.

Medium

• Client-side filtering of paginated jobs can hide non-fix jobs
  File: cmd/roborev/tui_handlers.go (in handleJobsMsg, IsFixJob() filter block)
  Filtering fix jobs after paginated /api/jobs fetch can make the queue appear empty when first pages are fix-heavy, even if later pages contain non-fix jobs.
  Suggested fix: Filter server-side for queue requests (for example, exclude fix type), or keep fetching pages client-side until non-fix jobs are found or pagination is exhausted.

---

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

[roborev-ci]

roborev: Combined Review (d7652bfd)

Verdict: Not ready to merge — 2 High and 2 Medium issues were identified after deduplicating reviewer outputs.

High

1. Unsafe submodule file protocol enablement

   • Files: internal/worktree/worktree.go:47, internal/worktree/worktree.go:59
   • protocol.file.allow=always is enabled based on repo-controlled .gitmodules, which can allow unintended file:// submodule resolution in untrusted/shared repos and expose local data during background worktree setup.
   • Recommended fix: keep file protocol disabled by default; require explicit trusted opt-in and strict local-path allowlisting (or skip submodule init/update for untrusted/fix-job flows).

2. Applying fix patches can commit unrelated local changes

   • Files: cmd/roborev/tui.go:3779, cmd/roborev/tui.go:3811
   • commitPatch uses git add -A, which stages all working-tree changes (including unrelated or sensitive files) before commit.
   • Recommended fix: stage only patch-introduced changes (git apply --index flow or explicit touched-path staging), and/or require a clean working tree unless explicitly confirmed.

Medium

1. stale_job_id is not validated against parent/repo linkage

   • Files: internal/daemon/server.go:1561, internal/daemon/server.go:1592
   • /api/job/fix rebase prompt construction accepts stale_job_id without enforcing same repo/parent-chain constraints, allowing incorrect cross-job patch mixing.
   • Recommended fix: validate stale job type/linkage (fix, same repo_id, expected parent_job_id, valid status, non-empty patch) and reject mismatches with 400.

2. Queue pagination offset is inconsistent with client-side fix-job filtering

   • Files: cmd/roborev/tui_handlers.go (handleJobsMsg), cmd/roborev/tui.go (fetchMoreJobs)
   • Jobs are filtered client-side (!j.IsFixJob()), but pagination offset uses filtered length (len(m.jobs)), causing duplicates/skips/empty-looking pages when early pages contain many fix jobs.
   • Recommended fix: apply filtering server-side or track/paginate by unfiltered row count.

---

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

[roborev-ci]

roborev: Combined Review (5d10a9f5)

Verdict: Changes are not ready to merge due to 3 High and 3 Medium issues requiring fixes.

High

1. Pathspec injection risk when staging patch files

   • File: cmd/roborev/tui.go:3865
   • commitPatch() passes patch-derived filenames into git add -- <files...>, which can still honor Git pathspec magic (e.g. :(...)) and stage unintended paths.
   • Fix: use literal pathspec mode (git --literal-pathspecs add -- ... or :(literal) per path) and reject suspicious pathspec patterns.

2. Unsafe auto-enable of local-file submodule protocol

   • Files: internal/worktree/worktree.go:46, internal/worktree/worktree.go:58
   • Worktree creation enables protocol.file.allow=always based on repo .gitmodules, allowing untrusted repos to trigger local file-path submodule fetch behavior.
   • Fix: require explicit trusted opt-in; if enabled, enforce strict path allowlisting.

3. Migration ordering bug can break startup on legacy DBs

   • Files: internal/storage/db.go:433, internal/storage/db.go:625, internal/storage/db.go:757
   • migrateJobStatusConstraint() can recreate idx_review_jobs_uuid before migrateSyncColumns() adds uuid, causing failures (no such column: uuid) on older schemas.
   • Fix: run sync-column migration earlier or gate index creation on uuid existence.

Medium

1. Terminal escape sequence injection in patch viewer

   • File: cmd/roborev/tui.go:3652
   • Raw patch lines are rendered directly; untrusted content can include ANSI/OSC control sequences (spoofing/clipboard abuse).
   • Fix: strip/escape unsafe control sequences before rendering.

2. applied/rebased transitions not restricted to fix jobs

   • Files: internal/storage/jobs.go:1125, internal/storage/jobs.go:1147, internal/daemon/server.go:1666, internal/daemon/server.go:1696
   • MarkJobApplied/MarkJobRebased can update any done job, including non-fix jobs, causing invalid state transitions.
   • Fix: enforce AND job_type='fix' in update conditions and return sql.ErrNoRows otherwise.

3. Applying patch may commit unrelated local edits in touched files

   • Files: cmd/roborev/tui.go:3779, cmd/roborev/tui.go:3823
   • After patch apply, git add -- <files> stages full file content, potentially including pre-existing unrelated edits.
   • Fix: require clean working tree before apply, or use index-driven flow (git apply --index) so only patch-intended changes are committed.

---

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

[mariusvniekerk]

Tasks View Screenshot

TUI showing the tasks view with fix job statuses, help overlay with keybindings/workflow, and review output.

[roborev-ci]

roborev: Combined Review (edb46f1b)

Verdict: Not merge-ready — 1 High and 5 Medium issues remain after deduplication.

High

1. Untrusted .gitmodules can force unsafe local-path submodule fetches
   • File/line: internal/worktree/worktree.go:47, internal/worktree/worktree.go:59, internal/worktree/worktree.go:175
   • Issue: Auto-enabling protocol.file.allow=always based on .gitmodules (attacker-controlled in untrusted/shared repos) bypasses safer git defaults and can enable local-path submodule access/exfiltration risk.
   • Suggested fix: Require explicit opt-in for protocol.file.allow=always; validate/deny unsafe file URLs (absolute paths, .., UNC) unless explicitly allowed.

Medium

1. Terminal escape injection risk when rendering patch lines

   • File/line: cmd/roborev/tui.go (around renderPatchView, b.WriteString(" " + display))
   • Issue: Patch content is written to terminal without control-sequence sanitization, allowing ANSI/OSC escape abuse from untrusted repository content.
   • Suggested fix: Strip/escape control sequences (especially ESC-initiated) before render.

2. Diff prefix stripping can corrupt valid paths (patchFiles)

   • File/line: cmd/roborev/tui.go (patchFiles)
   • Issue: Sequentially stripping both a/ and b/ can mangle legitimate paths (e.g., top-level b/...) and break git add -- ....
   • Suggested fix: Strip only one expected diff prefix; add tests for top-level a//b/ path cases.

3. Fix-only statuses can be applied to non-fix jobs

   • File/line: internal/storage/jobs.go (MarkJobApplied, MarkJobRebased)
   • Issue: Updates gate on status='done' but not job_type='fix', so non-fix jobs can be transitioned to applied/rebased.
   • Suggested fix: Add AND job_type = 'fix' to both updates and test rejection for non-fix jobs.

4. Non-atomic rebase flow can leave stale terminal state

   • File/line: cmd/roborev/tui.go (Update, tuiApplyPatchResultMsg rebase branch)
   • Issue: Old job is marked rebased before replacement rebase/fix job is guaranteed; enqueue failure can leave user without a recovery job.
   • Suggested fix: Make operation atomic server-side (single endpoint/transaction), or create replacement job first and mark rebased only on success.

5. Migration logic is fragile to schema formatting differences

   • File/line: internal/storage/db.go (migrateJobStatusConstraint)
   • Issue: strings.Replace against sqlite_master SQL can fail silently if DDL text differs (whitespace/format), leaving status constraint unchanged.
   • Suggested fix: Assert replacement occurred (e.g., SQL changed / contains new statuses) or rebuild table schema explicitly for deterministic migration.

---

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

[roborev-ci]

roborev: Combined Review (dffbc95a)

Verdict: Not ready to merge; 5 Medium/High issues were identified across reviews.

High

1. Pathspec injection risk when staging patch-derived files
   Files: cmd/roborev/tui.go:3878, cmd/roborev/tui.go:3897
commitPatch() stages file paths parsed from patch headers via git add -- ...; crafted names (e.g. pathspec magic like :(...)) can cause unintended files to be staged.

2. Unsafe auto-enable of local file submodule protocol
   Files: internal/worktree/worktree.go:47, internal/worktree/worktree.go:59, internal/worktree/worktree.go:175
   Worktree setup auto-enables protocol.file.allow=always based on .gitmodules, allowing untrusted repos to force local file-based submodule clones.

Medium

1. Terminal escape injection in TUI rendering
   Files: cmd/roborev/tui.go:3528, cmd/roborev/tui.go:3649
   Untrusted commit/patch text is rendered directly; control sequences can inject terminal actions/spoof output.

2. Non-fix jobs can be moved to fix-only states
   Files: internal/storage/jobs.go (MarkJobApplied, MarkJobRebased), internal/daemon/server.go (handleMarkJobApplied, handleMarkJobRebased)
   State transitions check status='done' but do not enforce job_type='fix', so non-fix jobs can be marked applied/rebased.

3. Patch apply flow may commit unrelated local edits
   Files: cmd/roborev/tui.go (applyFixPatch, commitPatch)
   Committing via full-file staging (git add -- <files>) can include pre-existing unrelated edits in those files.

---

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

[mariusvniekerk]

Security Finding Responses

1. Pathspec injection risk in commitPatch() — False Positive

Finding: commitPatch() stages file paths parsed from patch headers via git add -- ...; crafted names could cause unintended files to be staged.

Response: The -- separator in git add -- files... prevents pathspec magic interpretation — everything after -- is treated as a literal path. Additionally, paths are extracted by sourcegraph/go-diff's parser (not raw string splitting), and the TUI checks for existing staged changes before applying. No code change needed.

2. Unsafe auto-enable of local file submodule protocol — False Positive

Finding: Worktree setup auto-enables protocol.file.allow=always based on .gitmodules, allowing untrusted repos to force local file-based submodule clones.

Response: protocol.file.allow=always is only enabled when the user's own .gitmodules already contains local file-path URLs. The worktree is created from the user's local repo, so .gitmodules content is under their control. The flag is scoped to the individual git submodule update command invocation (passed via -c), not set as a global or repo-level config. This is the correct behavior for repos that use local submodule paths. No code change needed.

[roborev-ci]

roborev: Combined Review (c76d3a0e)

Verdict: 1 High and 2 Medium issues should be addressed before merge.

High

• Unintended/stale file inclusion in patch commits (plus pathspec injection risk)
  File/line: cmd/roborev/tui.go:3820 (applyFixPatch), cmd/roborev/tui.go:3911 (commitPatch), cmd/roborev/tui.go:3932 (dirtyPatchFiles)
  Patch-derived paths are passed to git add -- <files...> without literal pathspec protection, and git commit then commits the entire index. This can:
  • stage unintended files via git pathspec magic in crafted filenames, and
  • commit unrelated pre-staged changes outside the patch.
    Suggested fix: use literal pathspec handling (git --literal-pathspecs add or GIT_LITERAL_PATHSPECS=1 with --pathspec-from-file --pathspec-file-nul), reject pathspec-magic filenames, and prevent or isolate unrelated staged files (e.g., fail fast or use a temporary index).

Medium

• File protocol enabled for recursive submodule update in background jobs
  File/line: internal/worktree/worktree.go:46, internal/worktree/worktree.go:57, internal/worktree/worktree.go:175 (used from internal/daemon/worker.go:365)
  Enabling protocol.file.allow=always based on repo-controlled .gitmodules plus recursive submodule update allows file:///local-path submodules in externally sourced repos, increasing local data exposure risk in daemon fix-job flows.
  Suggested fix: default-deny file protocol for daemon/background jobs; require explicit opt-in and restrict to safe relative paths inside repo root.

• Queue pagination can skip/overlap after client-side fix-job filtering
  File/line: cmd/roborev/tui_handlers.go (handleJobsMsg), cmd/roborev/tui.go (pagination offset based on len(m.jobs))
  Fix jobs are filtered client-side, but pagination offset is derived from filtered length while server offsets are unfiltered. Mixed job types can cause duplicates/skips across pages.
  Suggested fix: move filtering server-side for queue fetches, or track pagination using raw/unfiltered offsets.

---

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

[roborev-ci]

roborev: Combined Review (59c470a0)

Verdict: Not ready to merge — 1 High and 3 Medium issues should be addressed before approval.

High

1. Unsafe submodule file transport enablement from repo-controlled .gitmodules
   Files: internal/worktree/worktree.go:48, internal/worktree/worktree.go:60, internal/worktree/worktree.go:201
   Enabling protocol.file.allow=always based on repository content weakens Git’s default protections and can allow crafted submodule URLs to reference local filesystem paths in untrusted repos.
   Recommended fix: require explicit trusted opt-in, block file:/relative local submodule URLs in automated flows, and/or enforce an allowlist.

Medium

1. Git pathspec injection risk when staging patch-derived filenames
   Files: cmd/roborev/tui.go:3842, cmd/roborev/tui.go:3878
   Filenames parsed from patch text are passed to git add as pathspecs; pathspec-magic forms (for example :(...)) may stage unintended files.
   Recommended fix: use literal pathspec mode (git --literal-pathspecs or GIT_LITERAL_PATHSPECS=1) and validate/reject suspicious names.

2. Fix action can run for non-failing reviews
   File: cmd/roborev/tui_handlers.go (handleFixKey)
   Logic gates on status == done but does not require a failing verdict, so PASS reviews can still trigger background fix jobs.
   Recommended fix: require failing verdict (or presence of findings) before allowing fix trigger.

3. Tasks view can hide fix jobs due to pagination/filter order
   Files: cmd/roborev/tui.go (fetchFixJobs), internal/daemon/server.go:608
   Client fetches /api/jobs with default limit and filters fix jobs client-side; if first page is filled by non-fix jobs, fix jobs are not shown.
   Recommended fix: add server-side job_type=fix filtering or fetch unbounded/explicitly paginated results that include fix jobs.

---

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

[roborev-ci]

roborev: Combined Review (74dee7a0)

Verdict: Changes are not ready to merge due to 1 High and 3 Medium issues.

High

• internal/worktree/worktree.go:46, internal/worktree/worktree.go:59 (invoked from internal/daemon/worker.go:~365)
  git submodule update can run with -c protocol.file.allow=always when .gitmodules uses file URLs. In shared/untrusted repos, this can enable local-path submodule fetch behavior that risks exposing local Git data to background agents.
  Fix: Default daemon/background jobs to protocol.file.allow=never; require explicit trusted opt-in + path allowlist if file protocol is needed.

Medium

• cmd/roborev/tui.go:~3950 (commitPatch), cmd/roborev/tui.go:~4020 (patchFiles)
  Patch-derived filenames are passed to git add -- <files> without forcing literal pathspec handling. Names beginning with pathspec magic (e.g. :(...)) may stage unintended files.
  Fix: Use literal pathspecs (git --literal-pathspecs add -- ... or GIT_LITERAL_PATHSPECS=1) and reject suspicious parsed paths.

• cmd/roborev/tui.go:2110
  Rebase flow marks stale job as rebased before triggerRebase is confirmed successful. If enqueue fails, old job is terminal with no replacement job.
  Fix: Make stale-mark + new enqueue atomic server-side, or mark rebased only after successful re-enqueue.

• cmd/roborev/refine.go:574
runRefine exits early on git.IsWorkingTreeClean(worktreePath), which misses committed-only agent changes even though worktree.CapturePatch() can capture committed deltas.
  Fix: Remove the clean-tree short-circuit and rely on CapturePatch() output ("" = no effective change).

---

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