fix: read-only Codex sandbox with unified diff file fallback for oversized diffs

[wesm]

Summary

• Revert Codex sandbox from --sandbox danger-full-access back to --sandbox read-only. With full access, Codex was scanning /home, /nix/store, and /root when investigating large diffs.
• Unified oversized-diff handling for all agents: when a diff is too large to inline, the worker writes it to a snapshot file in the repo's git dir and the prompt references the file path. No agent-specific branching — same path for Codex, Claude, Gemini, etc.
• Build() returns a plain truncation note for oversized diffs (backward compat for all existing callers). BuildWithDiffFile("") returns ErrDiffTruncatedNoFile so the worker knows to write a snapshot and retry.
• CI prebuilt prompts embed a placeholder path at enqueue time; the worker replaces it with the real snapshot path at execution time. Snapshot failure hard-fails the job so it retries.
• Codex review prompt instructs the agent not to search or read files outside the repository checkout.
• ResolveGitDir exported from internal/git with MSYS path normalization.
• End-to-end integration tests verify the full worker → prompt → agent flow for diff snapshots (file exists, content matches git diff, exclude patterns applied, cleanup after review).
• Kilo and OpenCode agent tests moved behind //go:build integration tag (agent package 17s → 10s).

🤖 Generated with Claude Code

[roborev-ci]

roborev: Combined Review (5b233a6)

Verdict: Changes are not ready to merge; there are 2 medium-severity correctness issues in the new sandboxed Codex diff handling.

Medium

1. Sandboxed prebuilt Codex reviews can fail outright in unsafe/full-access mode
   Location: internal/daemon/worker.go around preparePrebuiltCodexPrompt and prepareDiffFileForCodex
preparePrebuiltCodexPrompt now treats a missing generated diff file as fatal, but prepareDiffFileForCodex intentionally returns no file when agent.AllowUnsafeAgents() is enabled. That makes prebuilt Codex reviews fail instead of falling back to the original git-based prompt in unsafe/full-access mode.

2. Diff snapshot creation failures are silently downgraded into unusable sandboxed reviews
   Location: internal/daemon/worker.go around the new prepareDiffFileForCodex call in processJob
   On the normal review path, errors creating the diff snapshot are converted into diffFile == "" and the code falls back to pb.Build(...). For oversized Codex diffs, that fallback still tells the agent to inspect git state, but the review now runs with --sandbox read-only, so those instructions are not usable. The result can be a broken or no-op review instead of a clean retry/failure path.

---

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

[roborev-ci]

roborev: Combined Review (5b233a6)

Summary verdict: 2 medium-severity issues need attention before merge.

Medium

1. Sandboxed Codex reviews can fail outright in unsafe/full-access mode

   • Location: internal/daemon/worker.go around preparePrebuiltCodexPrompt and prepareDiffFileForCodex
   • Issue: Prebuilt Codex prompts now always include the diff-file placeholder, but preparePrebuiltCodexPrompt treats a missing generated diff file as fatal. prepareDiffFileForCodex intentionally returns no file when agent.AllowUnsafeAgents() is enabled, so prebuilt Codex reviews fail instead of falling back to the original git-based prompt.
   • Suggested fix: Only inject the placeholder when the review is actually sandboxed, or allow preparePrebuiltCodexPrompt to preserve the stored prompt when unsafe/full-access mode is active.

2. Diff snapshot failures are silently downgraded into a broken sandboxed fallback

   • Location: internal/daemon/worker.go around the new prepareDiffFileForCodex call in processJob
   • Issue: On the normal review path, failures creating the diff snapshot are converted into diffFile == "", and execution falls back to pb.Build(...). For oversized Codex diffs, that fallback still tells the agent to inspect git state, but the agent is launched with --sandbox read-only, so those instructions are unusable and the review can degrade into a no-op or incomplete analysis instead of failing cleanly.
   • Suggested fix: Propagate diff snapshot creation failures for sandboxed Codex reviews so the job retries/fails explicitly, or retain a full-access fallback when no readable diff snapshot can be prepared.

---

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

[roborev-ci]

roborev: Combined Review (7b40338)

Verdict: Changes are directionally correct, but there are 3 medium-severity issues that should be fixed before merge.

Medium

• Stale diff snapshot path can be persisted into retried Codex jobs
  Location: internal/daemon/worker.go:427
  For oversized non-prebuilt Codex reviews, processJob rebuilds the prompt with a concrete snapshot path via BuildWithDiffFile(...) and then deletes that file afterward. If the prompt is persisted for retry/failover, later retries can reuse a prompt that points at a deleted file because preparePrebuiltCodexPrompt only rewrites the placeholder sentinel, not an already-materialized path.
  Fix: Persist a placeholder-based prompt and substitute the real snapshot path only in memory right before invoking Codex, or regenerate the prompt on each retry.

• Dirty-review path does not get the new diff-file fallback
  Location: internal/daemon/worker.go:406
  The new oversized-diff fallback only applies to the git-backed Build(...) path. Jobs using job.DiffContent != nil still go through BuildDirty(...) without snapshot support, so large dirty Codex reviews can still fall back to truncated inline instructions even though review mode now forces --sandbox read-only.
  Fix: Add the same snapshot-file flow for dirty reviews by writing job.DiffContent to a temporary diff file and teaching the dirty prompt builder to reference it when inline diff content overflows.

• git rev-parse --git-dir output may include a trailing newline and break os.Stat
  Location: internal/git/git.go:278
cmd.Output() preserves the trailing newline from git rev-parse --git-dir. Unless normalizeMSYSPath already strips whitespace, passing that through filepath.Clean can leave the newline intact and cause later os.Stat(gitDir) calls to fail with ENOENT.
  Fix: Trim the command output before normalization/cleaning, e.g. strings.TrimSpace(...).

---

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

[roborev-ci]

roborev: Combined Review (e3a75ee)

Verdict: Changes are not ready to merge due to one high-severity build break and one medium-severity review regression.

High

• Compilation failure at internal/daemon/worker.go:1167
  prepareDiffFileForCodex calls gitpkg.GetDiff / gitpkg.GetRangeDiff with excludes..., but the current function signatures in internal/git/git.go only accept two string arguments. This will not compile (too many arguments in call).
  Fix: Either update internal/git/git.go so those helpers accept excludes ...string, or remove the extra arguments if they are not needed.

Medium

• Dirty-review fallback is incomplete at internal/daemon/worker.go:406
  The new diff-snapshot flow only covers stored prompts and the normal job.GitRef path. Dirty reviews still use BuildDirty(...) unchanged, so large uncommitted diffs can still be truncated in the prompt without a file-based fallback. With non-agentic Codex now running under --sandbox read-only, Codex can no longer recover omitted diff content itself, which is a review-quality regression.
  Fix: Add the same snapshot/file-reference mechanism for dirty reviews, or keep dirty Codex reviews on the previous sandbox mode until oversized dirty diffs are supplied out-of-band.

---

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

[roborev-ci]

roborev: Combined Review (209273e)

Verdict: One medium-severity regression needs to be addressed; no higher-severity issues were identified.

Medium

• Retry path persists stale oversized-diff instructions for some Codex reviews
  Location: internal/daemon/worker.go, internal/daemon/worker.go
  Large non-agentic Codex reviews built through the normal worker path still save the original pb.Build(...) prompt before the diff-file rewrite happens. On retry, that leaves the stored prompt with the old git-based oversized-diff fallback instead of CodexDiffFilePathPlaceholder, so preparePrebuiltCodexPrompt can only append a cat <diff> note rather than replacing the stale instructions. In read-only sandbox mode, retries can therefore contain contradictory guidance and point the model back toward commands expected to fail.
  Suggested fix: When truncation is detected, persist a placeholder-based prompt for Codex reviews by building with prompt.CodexDiffFilePathPlaceholder, then replace that placeholder at execution time.

---

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

[roborev-ci]

roborev: Combined Review (389f1ef)

Summary verdict: Changes are not ready to merge; there is one high-severity correctness issue and one medium-severity path-handling bug.

High

• internal/daemon/worker.go:422, internal/daemon/worker.go:1078, internal/prompt/prompt.go:382
  Oversized-diff review jobs can continue even when the diff snapshot file fails to be created. In that case prepareDiffFile returns "", BuildWithDiffFile falls back to a prompt that only states the diff was too large, and the agent loses access to the actual changes. This can produce false “clean” reviews for exactly the large diffs this feature is meant to handle.
  Suggested fix: Make snapshot creation mandatory for oversized-diff reviews and fail/retry the job if it cannot be written, or keep a fallback that still exposes the real diff to the agent.

Medium

• internal/git/git.go:278
cmd.Output() includes a trailing newline. If gitDir is used without trimming, the resulting path can be invalid, causing diff snapshot creation to fail.
  Suggested fix: Apply strings.TrimSpace(string(out)) before passing the value to normalizeMSYSPath().

---

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

[roborev-ci]

roborev: Combined Review (0ec9708)

Verdict: Changes are not ready to merge; there are 2 High and 1 Medium findings that should be addressed.

High

• internal/daemon/worker.go:1036
preparePrebuiltPrompt can silently continue after diff snapshot creation fails for an oversized prebuilt prompt, degrading the prompt to a truncation note with no inline diff and no readable diff file. In the read-only Codex flow, that means a large-diff review may run without access to the actual changes. This should fail/retry the job instead of proceeding with the diff removed.

• internal/daemon/worker.go:1060 (stripDiffFileBlock)
  The diff-fallback stripping logic is too broad and brittle. It currently matches the first occurrence of the marker text, which could appear in user-supplied PR discussion or prior review text and cause unrelated prompt content to be truncated. It also does not robustly handle both generated fallback variants, so broken diff instructions may be left behind. The replacement should target the specific generated diff-file fallback block using a precise anchor near the actual diff section.

Medium

• internal/git/git.go:279 (ResolveGitDir)
  The result of git rev-parse --git-dir may include a trailing newline. If that output is passed through without trimming, the returned path can be corrupted and later file operations such as os.WriteFile can fail. Trim whitespace from the command output before normalizing/cleaning the path.

---

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

[roborev-ci]

roborev: Combined Review (099e55f)

Verdict: Changes are not ready to merge due to 1 High and 1 Medium issue in the diff-file fallback path.

High

• internal/daemon/worker.go (~1036-1060, preparePrebuiltPrompt)
  If a stored or prebuilt prompt contains the diff-file placeholder and prepareDiffFile fails, the code logs a warning, removes the file-reference block, and still returns success. That allows the worker to proceed with a prompt that has neither the inline diff nor a valid external diff reference, which can silently produce incorrect reviews in retries, failover, or CI-prebuilt flows.
  Recommended fix: treat diff snapshot preparation failure as a hard failure for placeholder-based prompts, or rebuild the prompt with a real inline fallback before continuing.

Medium

• internal/daemon/worker.go (~1071-1085, stripDiffFileBlock)
  stripDiffFileBlock does not fully remove the compact fallback form "(Diff too large to include inline; read from \`)". It leaves behind the read from ...` instruction, so after snapshot preparation fails the prompt can still direct the agent to a nonexistent placeholder path.
  Recommended fix: strip the entire fallback block, including the compact variant, and replace it with a plain truncation note if needed.

---

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

[roborev-ci]

roborev: Combined Review (bd5593e)

Verdict: No medium, high, or critical issues identified.

All reported findings were below the requested severity threshold, so there are no actionable PR comments to surface at Medium or above.

---

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

[roborev-ci]

roborev: Combined Review (61c20c7)

Verdict: 3 medium-severity issues need attention before merge.

Medium

• internal/daemon/worker.go:1039, internal/prompt/prompt.go:145
  Prebuilt prompt handling uses a fixed, predictable placeholder path and later performs global string replacement across the stored prompt. Because the prompt includes untrusted content such as commit metadata, diffs, and PR discussion, an attacker can include that placeholder string in reviewed content and cause unintended substitution with the local snapshot path. This can distort what the reviewer model sees.
  Suggested fix: replace the fixed sentinel with a per-job unguessable token, or preferably store the diff-file reference as structured metadata and inject it only into the dedicated diff section. At minimum, constrain substitution to an explicit marker-delimited block instead of whole-prompt replacement.

• internal/prompt/prompt.go, internal/daemon/worker.go
  Prebuilt prompts are size-capped using the placeholder diff path, then the worker substitutes the real snapshot path later. Real snapshot paths, especially under .git worktrees, can be longer than the placeholder, so a prompt that passes validation at enqueue time can exceed the configured max prompt size at execution time.
  Suggested fix: size prompts against the actual snapshot path, or reserve enough space for the worst-case path length and re-apply the prompt cap after substitution.

• internal/prompt/prompt.go
Build() and BuildWithAdditionalContext() now fall back to a bare “Diff too large to include inline” note with no actionable way to inspect the omitted diff. For callers still using these public APIs outside the worker diff-file flow, this is a functional regression because large-diff reviews become effectively blind.
  Suggested fix: preserve the previous actionable fallback for non-diff-file APIs (for example, git inspection instructions), and keep the new diff-file-only behavior limited to BuildWithDiffFile / BuildWithAdditionalContextAndDiffFile.

---

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

Note: gemini review skipped (agent quota exhausted)

[roborev-ci]

roborev: Combined Review (2d8d4a1)

Verdict: not ready to merge; one high-severity issue needs to be fixed.

High

• internal/daemon/worker.go:389, internal/daemon/worker.go:422: Large-diff prompts in the normal review path are saved with a concrete snapshot file path, but that snapshot is deleted at the end of processJob. If the job is retried or picked up again, the worker reuses the stored prompt, and preparePrebuiltPrompt only regenerates a snapshot when the prompt still contains prompt.DiffFilePathPlaceholder. That leaves retries pointing at a deleted file, so the agent can lose access to the diff entirely. The prompt should be persisted with a placeholder for normal large-diff reviews as well, or rewritten back to the placeholder form before saving so retries can regenerate a fresh snapshot path.

---

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

[roborev-ci]

roborev: Combined Review (0c21150)

Verdict: changes are directionally good, but there is 1 High and 2 Medium issues that should be addressed before merge.

High

• Dirty Codex reviews can silently analyze only a truncated subset of the change
  • Locations: cmd/roborev/review.go:438, internal/daemon/worker.go:421, internal/agent/codex.go:144
  • Issue: Dirty reviews still use BuildDirty, which only inlines/truncates diffContent. Since non-agentic Codex reviews now run with --sandbox read-only, Codex can no longer recover an oversized dirty diff by inspecting the worktree, so it may review only part of the actual change.
  • Suggested fix: Add a dirty-diff snapshot flow equivalent to BuildWithSnapshot / WriteDiffSnapshot, or keep the previous full-access behavior for dirty Codex reviews until oversized dirty diffs can be materialized to a readable file.

Medium

• Prompt-size cap can be bypassed after placeholder replacement

  • Locations: internal/daemon/worker.go:1025, internal/prompt/prompt.go:145
  • Issue: Prebuilt prompts are capped while they still contain the short placeholder path, then preparePrebuiltPrompt replaces that placeholder with the real absolute snapshot path later without reapplying the cap. On deep repo/worktree paths, this can push the prompt back over DefaultMaxPromptSize.
  • Suggested fix: Build and cap using the real snapshot path, or reserve worst-case path space before storing the prebuilt prompt.

• ResolveGitDir may return a path with a trailing newline

  • Location: internal/git/git.go:278
  • Issue: git rev-parse --git-dir returns output with a trailing newline. If that value is not trimmed before path normalization, gitDir can become invalid (for example .../.git\n), which would break later calls such as os.CreateTemp.
  • Suggested fix: Apply strings.TrimSpace(string(out)) before further path processing.

---

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

[roborev-ci]

roborev: Combined Review (4397055)

Verdict: One high-severity correctness issue needs to be fixed before merge.

High

• internal/git/git.go:278 (ResolveGitDir): git rev-parse --git-dir returns output with a trailing newline, and that value is passed through path normalization/joining without trimming. This can produce a git-dir path ending in \n, which then causes snapshot creation to fail in WriteDiffSnapshot when os.CreateTemp is called.
  Suggested fix: trim before normalization, e.g. normalizeMSYSPath(strings.TrimSpace(string(out))).

---

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

[roborev-ci]

roborev: Combined Review (e11ed33)

Verdict: One medium-severity issue should be addressed before merge.

Medium

• internal/prompt/prompt.go:287: BuildDirtyWithSnapshot silently falls back to the truncated inline prompt if snapshot creation fails (ResolveGitDir, CreateTemp, write, or close errors return SnapshotResult{Prompt: p}, nil). With Codex reviews now running under --sandbox read-only, a large dirty review may then proceed against incomplete input with no visible failure.
  Fix: if the diff was truncated and the snapshot cannot be created, return an error instead of silently degrading, or provide another guaranteed full-diff fallback.

---

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

[roborev-ci]

roborev: Combined Review (4d854db)

Verdict: One medium-severity issue should be addressed before merge; no high or critical findings were identified.

Medium

• internal/prompt/prompt.go (BuildDirtyWithSnapshot): Dirty-review snapshot handling currently depends on detecting "(Diff too large to include in full)" in the already size-capped output of BuildDirty. If that marker is itself trimmed under a tight prompt cap, no snapshot is created and the agent receives a silently truncated diff. If the marker does survive, appending the snapshot path afterward can push the final prompt back over the configured max size.
  Fix: Return structured truncation metadata from BuildDirty and route dirty diff snapshot fallback through the same cap-aware prompt-building path used for commit/range reviews.

---

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

[wesm]

Not actionable. Merging