Follow up to #514: make insights a daemon-owned job type

[mariusvniekerk]

Follow-up to #514.

Because #514 is opened from a cross-repo branch (darrenhaas:darren/insights-command), GitHub cannot stack this PR directly on that branch inside roborev-dev/roborev. This PR carries the follow-up work against main and references #514 for context.

Meaningful changes

• add a first-class insights job type so the daemon, worker, storage layer, and TUI all treat insights runs as stored-prompt task work
• move insights dataset assembly into the daemon enqueue path, including history selection, branch filtering, review/comment loading, and prompt construction
• simplify the CLI to a thin enqueue client and remove the CLI-side N+1 /api/jobs + /api/review + /api/comments fan-out
• keep review comments in the analysis corpus, exclude compact jobs from the insights dataset, and use the requested branch for insights filtering/metadata
• return a clean skipped response when no matching failing reviews exist instead of forcing the CLI to prefetch history
• add daemon, CLI, and storage tests that lock in the new insights job contract

Validation

• go fmt ./...
• go vet ./...
• go test ./...

[roborev-ci]

roborev: Combined Review (909f43a)

Summary Verdict: One Medium severity issue found regarding prompt budget enforcement in insights.go.

Findings

Medium

• Location: internal/prompt/insights.go:90
  • Problem: guidelineCap := max(promptBudget/10, 1024) does the opposite of the comment’s stated “10% cap” and guarantees at least 102
    4 bytes for guidelines. Because BuildInsightsPrompt writes the system prompt and guidelines before any overall size check, repos with a small max_prompt_size can still produce prompts larger than the configured budget.
  • Fix: Cap guideline text against the remaining prompt budget instead of a hard minimum, and
    enforce the final prompt length before returning.

---

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

[roborev-ci]

roborev: Combined Review (66c53e3)

Summary Verdict: The PR successfully introduces the insights command to analyze historical reviews, but requires a few medium-severity fixes related to branch-exclusion logic, configuration loading, and database query optimization.

Medium Severity Findings

Location: internal/daemon/server.go:668

• Problem: insights jobs are still blocked by branch exclusion when --branch is not provided, because the enqueue path falls back to the currently checked-out branch. That makes a repo-wide historical analysis fail simply
  because the operator happened to run it from an excluded branch, even though the job is not reviewing that branch.

• Fix: Skip branch-exclusion checks for insights jobs when no explicit branch filter was requested, or only apply the exclusion check to req.Branch when it is set.

• Location: internal/daemon/insights.go:27

  • Problem: Insights loads guidelines with config.LoadRepoConfig(repoRoot), which is not the default-branch-aware config path called out elsewhere in the codebase. If the working tree has uncommitted or feature-
    branch .roborev.toml changes, the analysis can be grounded on the wrong guidelines and suggest incorrect additions/removals.
  • Fix: Reuse the same default-branch-aware guideline loading path used for normal review prompt construction instead of reading repo config directly from the working tree.

Location: internal/daemon/insights.go:53

• Problem: Calling s.db.ListJobs with limit=0 and without a since filter at the database level fetches all historically completed jobs into memory before applying the since filter in a Go
  loop. On repositories with a long history, this can cause significant memory bloat and performance degradation.
• Fix: Add a WithSince option to filter jobs directly within the SQL query, or introduce pagination/limits.

---

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

[roborev-ci]

roborev: Combined Review (316cb05)

Verdict: Adds the roborev insights command and daemon support to analyze failing review patterns and suggest guideline improvements.

🟡 Medium Severity

• internal/daemon/server.go:712

  • Problem: Repo-wide insights requests still inherit
    the current checkout branch for the excluded-branch check. If the operator runs roborev insights from an excluded branch without --branch, the daemon skips the job even though the query is meant to analyze historical reviews across all branches.
  • Fix: For JobTypeInsights, only apply branch
    exclusion when req.Branch is explicitly set, or skip the exclusion check entirely for unscoped insights jobs.

• internal/prompt/insights.go:92

  • Problem: The guideline truncation logic does not actually cap guidelines to 10% of the prompt
    budget: max(promptBudget/10, 1024) raises the cap above 10% for smaller budgets, and for very small MaxPromptSize values the fixed prompt boilerplate plus the 1024-byte minimum can exceed the configured prompt budget altogether.

Fix: Use an upper bound instead of a floor, e.g. min(promptBudget/10, 1024), and enforce a final budget check so the returned prompt never exceeds MaxPromptSize.

---

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

[roborev-ci]

roborev: Combined Review (376e27b)

The PR adds a new roborev insights flow to analyze historical failing reviews, but contains medium-severity logic issues regarding prompt size enforcement and branch-exclusions.

Medium

• Location: internal/prompt/insights.go:93
  Problem: The guideline budget uses max(promptBudget/10, 1024), which guarantees at least 1 KiB of guideline text. For small max_prompt_size values,
  the prompt can exceed the configured budget before any review data is added, making the prompt size enforcement ineffective.
  Fix: Use a true upper cap for guidelines, such as min(promptBudget/10, 1024), and ensure the final prompt is clipped or rechecked against promptBudget.

• Location: internal/daemon/server.go:713
  Problem: Insights jobs still inherit branch-exclusion logic from normal reviews when --branch is omitted, because branchToCheck falls back to the current checkout branch. This incorrectly skips historical analysis whenever roborev insights is run from an excluded branch, even though the command is read-only and not scoped to the current branch by default.
  Fix: Do not apply branch-exclusion checks to insights jobs unless an explicit branch filter is provided, or exempt JobTypeInsights from this check entirely.

---

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

[roborev-ci]

roborev: Combined Review (1582a4e)

Verdict: The PR successfully introduces the insights command flow, but requires fixes for duration
parsing, prompt budget enforcement, and JSON output wait logic.

Medium Severity

• Location: cmd/roborev/insights.go:211
  Problem: parseSinceDuration accepts negative standard Go durations (e.g., --since=-5h). Because the
  code subtracts this using time.Now().Add(-d), it turns into a future timestamp and silently produces the wrong review window.
  Fix: Reject any parsed duration <= 0 before returning, and add a regression test for negative hour/minute inputs.

• Location: internal/prompt /insights.go:82
  Problem: The prompt-size cap is not enforced for small max_prompt_size values. BuildInsightsPrompt writes the full system prompt and section scaffolding before budget checks, allowing the returned prompt to exceed the limit and cause runtime failures.
  **
  Fix:** Enforce the budget from the start of prompt construction by reserving space for fixed sections, truncating/omitting as necessary, or returning an explicit error when the configured budget is too small.

• Location: cmd/roborev/insights.go:189-197

  Problem: When --json is provided, the function encodes the enqueue metadata and returns immediately, silently bypassing the --wait logic (which defaults to true). This prevents users from retrieving the completed insights analysis.
  Fix: Wait for the job to complete if opts.wait is true before
  emitting the JSON response, and include the final result in the JSON payload.

---

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

[roborev-ci]

roborev: Combined Review (536ce87)

Verdict: The PR adds a new insights command for historical review-pattern analysis, with one medium severity issue identified regarding configuration loading.

Medium

• Location: internal/daemon/insights.go
  :26
• Problem: The insights prompt loads review_guidelines with config.LoadRepoConfig(repoRoot), which reads from the checked-out filesystem instead of using the default-branch
  -first config loading path used elsewhere. That means roborev insights can analyze historical reviews against branch-local or uncommitted guideline changes rather than the canonical project guidelines, producing misleading gap/addition recommendations.
• Fix: Reuse the same guideline-loading helper/logic as normal review execution so insights resolves
  .roborev.toml from the default branch with the existing filesystem fallback behavior.

---

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

[roborev-ci]

roborev: Combined Review (41eebd7)

The PR introduces the insights command and server-side prompt construction for historical failing-review analysis, but contains medium-severity edge cases around dataset filtering and prompt-size handling that need addressing.

🟡 Medium

• Location: internal/daemon/insights.go (around the UsesStoredPrompt() filter) and internal/storage/models.go (in UsesStoredPrompt())

  • Problem: The insights dataset can include old non-review prompt jobs from databases
    that predate job_type. fetchInsightsReviews() excludes only job_type=task rows and skips only explicit stored-prompt job types. Older prompt/analyze jobs with empty job_type are only classified as task jobs by IsTaskJob()'s fallback heuristic, meaning
    they can leak into insights analysis and skew the results.
  • Fix: Filter positively for real review jobs (review, range, dirty) or extend the stored-prompt exclusion path to use the same backward-compatibility heuristic as IsTaskJob().

• Location: internal/ prompt/insights.go (review comment serialization loop)

  • Problem: Review comments are appended without any size cap. If one matching review has a very large comment thread, entry.Len() can exceed the prompt budget and the loop will omit that review entirely, potentially producing an insights prompt with no review
    bodies even though matching failing reviews exist.
  • Fix: Truncate comment text and/or cap total serialized comment bytes per review before the budget check so at least a bounded version of each selected review is included.

---

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

[roborev-ci]

roborev: Combined Review (d887f46)

Verdict: The PR introduces useful functionality for review insights, but contains a few medium-severity logic issues regarding prompt data constraints and legacy job classification that should be addressed.

Medium Severity

• Location: internal/prompt/insights.go:29
  **Problem
  **: The prompt asks the model to identify findings that were "dismissed without corresponding code changes", but the dataset passed into insights only includes review text, comments, and a Closed flag. There is no signal for whether a closure came from an actual code fix versus a manual dismissal, so this part of the analysis
  will be speculative and can recommend suppressing valid findings.
  Fix: Either include explicit dismissal/no-code-change metadata in the prompt input, or reword the insights prompt so it only makes claims that can be supported by the available review/comment data.

• Location: internal/storage /models.go:140
  Problem: IsReviewJob() uses j.GitRef != "" && !j.IsTaskJob() as its legacy fallback. That positively classifies any old job with an empty job_type and a non-empty non-task
  GitRef as a review, which can pull legacy non-review stored-prompt jobs back into insights datasets. This undercuts the new "real review jobs only" filtering for older synced data.
  Fix: Make the fallback a positive review heuristic instead of a negation of task detection, for example:
  only treat legacy jobs as reviews when they have a CommitID, GitRef == "dirty", or a range ref containing ...

---

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

[roborev-ci]

roborev: Combined Review (e7cdc04)

Verdict: The PR successfully adds a new insights command and flow, but introduces a medium-severity issue with prompt size enforcement.

Medium

• Location: internal/prompt/insights.go:82
  • Problem: BuildInsightsPrompt does not actually
    guarantee MaxPromptSize. It writes the full system prompt and framing sections before any budget check, and later appends omission notes without rechecking the remaining space. If a repo config sets a small max_prompt_size, the generated insights prompt can still exceed that limit, which defeats the configured cap and can
    cause provider-side truncation or request rejection.
  • Fix: Enforce the budget across the entire function, not just per-review entries. Check the fixed scaffolding against promptBudget, truncate or elide sections when necessary, and only append omission notes when they still fit.

---

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

[wesm]

"Technically valid but overstated. The fixed scaffolding (system prompt ~1.5KB + capped guidelines
≤1KB + headers) is bounded and small. The per-review budget check on line 183 is where the real
enforcement happens, and it works correctly.

The only scenarios where the prompt exceeds MaxPromptSize:

1. Budget is set smaller than the fixed scaffolding (~2.5KB) — the prompt would be useless anyway
2. The omission/note lines at the end (~100 bytes) — negligible

I'd call this a false positive. Not worth adding complexity to guard against configurations that
would make insights non-functional regardless."