Skip to content

feat: subagent review panels with synthesis#762

Open
wesm wants to merge 32 commits into
mainfrom
feat/proper-subagent-reviews
Open

feat: subagent review panels with synthesis#762
wesm wants to merge 32 commits into
mainfrom
feat/proper-subagent-reviews

Conversation

@wesm
Copy link
Copy Markdown
Member

@wesm wesm commented Jun 1, 2026

Summary

Adds subagent review panels: one review request can fan out to multiple
reviewer jobs that run in parallel, then a synthesis parent job produces the
single verdict/result users see. Panels are opt-in for foreground reviews and
configured in TOML. When no panel is selected, the existing single-agent review
path remains the default.

The CI poller now uses the same panel primitive instead of the legacy
ci_pr_batches / ci_pr_batch_jobs tracking tables. CI and foreground reviews
therefore share the same member-job, synthesis, storage, posting, and TUI
machinery.

Runtime Model

A panel run creates N member jobs plus one synthesis parent job. All jobs
in the run share a panel_run_uuid.

  • Each member is a normal review job pinned at enqueue time to a resolved agent,
    model, provider, reasoning level, review type, and optional member-specific
    instructions.
  • The reviewed target is frozen once at enqueue time, so all members review the
    same commit/range/dirty base.
  • The synthesis parent waits behind claim_blocked until members finish, then
    stores the single synthesized review output and verdict.
  • If no member produces usable output, the parent stores an all-failed panel
    result without calling an agent.
  • If exactly one member produces usable output, the parent passes that output
    through and labels the result with that member's agent.
  • If all successful members passed, the parent stores a clean combined review
    without calling an agent.
  • Otherwise, the synthesis agent combines the member outputs, deduplicates
    findings, preserves severity/file references, and applies min_severity
    filtering. The synthesis prompt is input-only: it tells the agent not to call
    tools or run commands, and not to perform a fresh review beyond combining the
    supplied member results.

Everything user-facing keys off the parent job:

  • roborev list, roborev wait, and fix discovery use /api/jobs, which hides
    panel members by default. They show synthesis parents plus ordinary
    single-agent jobs.
  • GET /api/jobs?panel_run=<uuid>&limit=0 expands a run's members.
  • roborev show prints a one-line member summary for a synthesis job; roborev show --json adds a panel block with member job id, name, agent, review
    type, status, and verdict.
  • The TUI queue supports one-level parent/member expansion. Space toggles,
    right expands, and left collapses. The parent row shows panel status and
    aggregated cost when enough token data is available.
  • Fix flows operate on the parent synthesis job, not individual member jobs.

Configuration

Panels live under [review] in both global ~/.roborev/config.toml and per-repo
.roborev.toml. Per-repo scalar keys override global keys. Subagent and panel
maps are unioned, with per-repo entries overriding same-named global entries.

Every subagent field is optional. Empty fields inherit the existing workflow
resolution at run time.

[review]
# Used by foreground/explicit reviews when no --panel flag is given.
# Omit to keep foreground reviews single-agent.
default_panel = "standard"

# Used by automatic post-commit hook reviews. Hook reviews do not fall back to
# default_panel, so leaving this unset keeps post-commit reviews single-agent.
hook_review_panel = "quick"

[review.subagents.bug]
agent = "codex"
review_type = ""                 # default review

[review.subagents.security]
agent = "claude-code"
review_type = "security"
reasoning = "thorough"

[review.subagents.design]
agent = "gemini"
review_type = "design"
instructions = "Focus on API ergonomics and backward compatibility."

[review.panels.standard]
members = ["bug", "security", "design"]
synthesis_agent = "claude-code"
synthesis_model = "claude-opus-4-8"
synthesis_backup_agent = "codex"
synthesis_backup_model = "gpt-5"

[review.panels.quick]
members = ["bug"]

Selection rules:

  • roborev review and roborev review --branch use default_panel unless a
    --panel flag is supplied.
  • roborev review --panel <name> forces a named panel.
  • roborev review --panel none forces a single-agent review.
  • roborev review --local remains single-agent; panels require the daemon.
  • Post-commit hook reviews use only hook_review_panel; they never implicitly
    use default_panel.

Config validation reports all panel reference errors together: empty panels,
unknown subagents, and invalid default_panel / hook_review_panel references.

CI Poller

The CI poller creates one panel run per PR HEAD and posts one PR comment/status
from the synthesis parent.

There are two CI modes:

  1. Named panel: set [ci].panel or per-repo [ci].panel. The poller loads
    config from the PR's default branch and resolves [review.panels.<name>]
    from that config.
  2. Implicit matrix: when panel is unset, the existing agents x review_types matrix, or explicit [ci.reviews] map, becomes the panel
    members.

For implicit matrix synthesis, [ci].synthesis_agent, [ci].synthesis_model,
and [ci].synthesis_backup_agent are honored first. If unset, synthesis falls
back to fix-workflow resolution. Named panels use their own
[review.panels.<name>] synthesis fields.

[ci]
enabled = true
repos = ["myorg/api", "myorg/web-*"]
poll_interval = "5m"
min_severity = "medium"

# Option A: run a named panel for CI.
panel = "standard"

# Option B, used when panel is unset: matrix members.
review_types = ["security", "review"]
agents = ["codex", "gemini"]
# or:
# [ci.reviews]
# codex = ["security", "review"]
# gemini = ["review"]

synthesis_agent = "claude-code"
synthesis_model = "claude-opus-4-8"
synthesis_backup_agent = "codex"

# Off by default. When true, CI PR comment footers include token cost estimates.
include_costs = false

Per-repo .roborev.toml can override panel, agents, review_types,
reviews, reasoning, min_severity, upsert_comments, and include_costs
under its own [ci] table.

CI posting is claimed with compare-and-set semantics so only one daemon instance
posts a given panel. The poller handles superseded PR heads, throttled pushes,
restarts, batch timeouts, closed PRs, and failed/canceled members. If a PR is
closed/merged while a panel finishes, the mapping is removed rather than marked
posted so reopening the same HEAD can enqueue/review again.

CI comments use a combined-review title that includes the reviewed HEAD short
SHA. The footer includes panel name, synthesis agent/runtime, member statuses and
runtimes, total runtime, optional cost, and the synthesis job id. Oversized
comments are truncated to GitHub's comment limit while preserving a compact
footer.

Storage and Migration

This PR adds panel state to review_jobs and replaces the legacy CI batch
tracking tables with ci_pr_panels.

SQLite changes:

  • Adds backup_agent and backup_model to review_jobs for job-level failover.
  • Adds panel_run_uuid, panel_role, panel_name, panel_member_name,
    panel_member_index, panel_member_config_json, and local-only
    claim_blocked to review_jobs.
  • Adds panel indexes after legacy table rebuilds so older databases retain the
    indexes after migration.
  • Adds ci_pr_panels for PR HEAD to panel-run/synthesis-job mapping and posting
    state.
  • Retires and drops legacy ci_pr_batches and ci_pr_batch_jobs after canceling
    any queued/running legacy batch jobs. The drain/drop path is transactional and
    idempotent so interrupted prior migrations can recover cleanly.

Postgres changes:

  • Adds schema version v15 on top of v14.
  • Adds the same panel and backup columns to review_jobs.
  • Adds ci_pr_panels and panel indexes.
  • Sync/hydration paths carry the new panel and backup columns.

Panel columns are additive. The legacy CI batch tables are intentionally removed
as a one-way migration after draining.

UI and CLI

  • roborev review accepts --panel <name>|none and reports panel enqueue
    details while preserving the Enqueued job <id> token skills depend on.
  • roborev show and show --json expose panel metadata for synthesis jobs.
  • TUI queue rendering supports collapsed/expanded panel rows, left/right
    expand/collapse behavior, parent status while members are running, parent cost
    aggregation, and member rows without unsupported close affordances.
  • Skills documentation for Codex and Claude mentions --panel and explains that
    panel review IDs refer to the synthesis parent.

Development and Test Safety Notes

This branch also includes supporting safety/test changes discovered while
validating the feature:

  • Test daemon discovery avoids accidentally targeting the user's live daemon when
    no runtime file or explicit --server is present.
  • Test Git config isolation now includes XDG_CONFIG_HOME so user-level global
    ignores cannot affect tests.
  • Prompt test repositories disable Git auto-maintenance to avoid race-sensitive
    object cleanup during parallel test runs.
  • AGENTS.md now warns agents not to install or overwrite the local roborev
    binary without explicit permission.
  • Panel target freezing uses the existing kit Git resolver. This PR intentionally
    does not change the shared internal/git.ResolveSHA environment behavior.

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 1, 2026

roborev: Combined Review (93ff37c)

Panel review changes are mostly sound, but two Medium issues should be fixed before merge.

Medium

  • internal/storage/ci_panels.go:215 - GetActivePanelsForPR returns panels that already have posting_claimed_at set. Supersede or closed-PR cleanup can delete/cancel a run after postPanelRun has already won the posting claim but before it publishes, allowing a stale comment to be posted.

    • Fix: exclude freshly claimed rows from active cleanup, or add an explicit superseded/abandoned CAS that postPanelRun re-checks immediately before posting.
  • internal/daemon/rerun_panel.go:113 - Panel synthesis reruns do not copy BackupAgent / BackupModel, so a panel with configured synthesis failover loses that failover on rerun.

    • Fix: copy job.BackupAgent and job.BackupModel into panelRerunSynthesisOpts.

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

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 1, 2026

roborev: Combined Review (17b3fc8)

Medium-risk changes need follow-up before merge.

Medium

  • internal/config/panels.go:270 - The implicit CI matrix path now resolves synthesis through the fix workflow and ignores existing [ci] settings (synthesis_agent, synthesis_model, synthesis_backup_agent). Existing CI configs will silently run synthesis with a different agent/model and lose their configured backup.
    Fix: Have ResolveCISynthesis honor the CI synthesis fields for the matrix path, falling back to the fix workflow only when those fields are unset.

  • internal/daemon/ci_poller.go:455 - alreadyReviewedPR still treats legacy ci_pr_reviews rows as already reviewed, but the event handler now only posts panel synthesis jobs. Any in-flight or unposted legacy CI review row can cause the PR HEAD to be skipped forever with no comment/recovery path.
    Fix: Keep the legacy ci_pr_reviews completion handler until old rows drain, or migrate/delete legacy rows and stop using HasCIReview as an already-reviewed signal.


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

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 1, 2026

roborev: Combined Review (115df5e)

Panel enqueue path has one medium issue that can cause clients to miss newly queued panel runs.

Medium

  • internal/daemon/panel_enqueue.go:366
    Panel enqueues return immediately after inserting jobs, bypassing the enqueue tail used by single-agent reviews. As a result, the synthesis parent does not emit a job.enqueued SSE event or activity-log entry, so clients relying on enqueue events can miss a newly queued panel until a later poll or worker event.

    Fix: After EnqueuePanelRun, run the same enqueue side effects for the synthesis job, at least activity logging and the job.enqueued broadcast, or factor the shared enqueue tail so both paths use it.


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

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 1, 2026

roborev: Combined Review (67878ec)

Medium: CI panel throttling can leave stale panel runs active and later post results for a superseded head SHA.

Medium

  • internal/daemon/ci_poller.go:317
    The new CI panel flow returns on throttle before calling supersedePriorPanels at internal/daemon/ci_poller.go:355. If a new push arrives within the throttle window, the older unposted panel run can remain active; when it finishes, postPanelRun only checks that the PR is open, so it may post a review/comment for a superseded head SHA.
    Fix: Supersede active runs for a different head before the throttle return, matching the old behavior, or make posting/reconcile verify the PR head still equals row.HeadSHA and abandon stale runs.

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

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 1, 2026

roborev: Pass

No issues found.


Review type: | Agent: codex | Job: 19309

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 1, 2026

roborev: Fail

Review findings

Panel review fanout needs fixes before merge: three Medium correctness issues can publish misleading or stale results, or ignore CI synthesis config.

Medium

  • internal/daemon/synthesis_worker.go:48
    A panel with exactly one successful member and one or more failed/skipped members stores that successful output verbatim as the canonical synthesis result. If the lone output is a pass, the parent review can be parsed and auto-closed as passing even though other reviewers failed or were skipped.
    Fix: Only passthrough when the panel has exactly one member total; otherwise produce a synthesized/raw panel comment that includes failed/skipped member statuses.

  • internal/config/panels.go:270
    The implicit CI matrix path resolves synthesis from an empty PanelSpec, so existing [ci] settings like synthesis_agent, synthesis_model, and synthesis_backup_agent are ignored.
    Fix: Seed the synthesis spec from globalCfg.CI.SynthesisAgent, SynthesisModel, and SynthesisBackupAgent before falling back to workflow defaults.

  • internal/daemon/ci_poller.go:316
    Throttled pushes return before supersedePriorPanels runs, leaving the previous HEAD’s active panel alive. That stale run can still finish and post a PR comment for an obsolete commit while the new HEAD is only marked deferred.
    Fix: Supersede older active panel runs before the throttle early return, or have the throttled branch cancel/delete older active runs before returning.


Review type: | Agent: codex | Job: 19315

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 1, 2026

roborev: Fail

Medium issue found: throttled PR re-pushes can leave stale panel runs active.

Medium

  • Location: internal/daemon/ci_poller.go:316
  • Problem: The CI poller applies PR throttling before supersedePriorPanels, so a rapid re-push can leave an older-head panel run active until the throttle window expires. That old run can still finish and publish stale review output for a commit that is no longer the PR head, while the new head only has a pending status.
  • Fix: Cancel or mark prior active panel runs as superseded before returning from the throttle path when the head SHA changed, or otherwise ensure old-head panel results cannot be posted after a newer head is observed.

Review type: | Agent: codex | Job: 19321

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Fail

Medium: CI panel migration has two functional regressions that can cause stale or missing PR comments. No medium-or-higher security issues were reported.

Medium

  • internal/daemon/ci_poller.go:1462: handleReviewFailed now routes both review.failed and review.canceled synthesis events into panel posting. A canceled synthesis parent is used when runs are abandoned during manual cancel/supersede, and supersedePriorPanels cancels before retiring the mapping. That means a cancellation event can claim/post a stale raw fallback comment for an old HEAD while the row is still active.

    • Suggested fix: Do not post on review.canceled synthesis events, or retire/delete the CI panel mapping before canceling/killing the synthesis parent and make posting ignore canceled synthesis rows.
  • internal/daemon/ci_poller.go:447: Legacy ci_pr_reviews rows still make alreadyReviewedPR skip a PR HEAD, but the old completion handler that looked up GetCIReviewByJobID and posted the PR comment was removed. A legacy single CI review queued/running during upgrade can complete without posting, and future polls will not create a panel because HasCIReview remains true.

    • Suggested fix: Keep the legacy completion posting path until those rows drain, or migrate/cancel/delete unposted legacy ci_pr_reviews rows so they do not suppress panel creation.

Panel: ci_default_security | Synthesis: codex, 9s, ~$0.04 | Members: codex_default (codex/default, done, 4m53s), codex_security (codex/security, done, 2m11s) | Total: 7m13s, cost partial ~$0.04 | Job: 19423

@wesm wesm force-pushed the feat/proper-subagent-reviews branch from c96019e to 4950bf2 Compare June 2, 2026 02:50
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Pass

No issues found.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, canceled, 15m58s), codex_security (codex/security, done, 6m30s, ~$4.33) | Total: 22m28s, cost partial ~$4.33 | Job: 19486

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Fail

Medium severity issues were found; no High or Critical findings reported.

Medium

  • [internal/daemon/synthesis_worker.go:48] Single successful panel member output is passed through unfiltered, so CI min_severity is ignored for one-member panels or panels where only one reviewer succeeds. The previous synthesis path only bypassed the LLM for a single result when no severity filtering was requested.

    • Fix: Only use the passthrough branch when job.MinSeverity is empty or low; otherwise run the synthesis/filtering path or apply the same severity filter before storing the synthesis review.
  • [internal/storage/db.go:972] drainAndDropOldCIBatchTables checks only for ci_pr_batches but unconditionally references ci_pr_batch_jobs. If an upgrade is interrupted after dropping ci_pr_batch_jobs but before dropping ci_pr_batches, subsequent opens fail permanently with a missing-table error.

    • Fix: Run the drain/drop sequence in a transaction and/or check both legacy tables before referencing ci_pr_batch_jobs, dropping any remaining table independently.

Panel: ci_default_security | Synthesis: codex, 7s, ~$0.04 | Members: codex_default (codex/default, done, 15m35s, ~$11.08), codex_security (codex/security, done, 4m29s, ~$2.82) | Total: 20m11s, ~$13.94 | Job: 19504

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Fail

Medium issue found: panel reruns may reuse stale prompts.

Medium

  • internal/daemon/rerun_panel.go:85: Panel reruns copy Prompt and PromptPrebuilt from the previous member job, so rerun members can reuse stale prebuilt prompts instead of rebuilding from current refs/config. This diverges from single-job reruns, which clear review prompts so the worker regenerates them.
    • Fix: Preserve Prompt/PromptPrebuilt only for prompt-native job types if panels support them; for review/range/dirty panel members, leave Prompt empty and PromptPrebuilt=false so the worker rebuilds the prompt.

Panel: ci_default_security | Synthesis: codex, 8s, ~$0.04 | Members: codex_default (codex/default, done, 9m57s, ~$7.85), codex_security (codex/security, done, 7m7s, ~$5.36) | Total: 17m12s, ~$13.24 | Job: 19510

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Fail

One medium correctness issue remains; no medium-or-higher security issues were found.

Medium

  • internal/daemon/ci_poller.go:729 - CI auto-design appends the preferred design agent/model from config.DesignAgentFromConfig directly, but that helper does not perform availability or backup selection. Unlike the normal CI matrix path, an unavailable configured design_agent with an installed backup can be enqueued as-is, causing the worker to ignore the intended design backup and either run a generic fallback with the wrong model or fail/retry.

    Fix: Resolve the appended design member through the same workflow availability path as matrix members, passing the design workflow backup into agent.GetAvailableWithConfigFromConfig, or reuse resolveMatrixMemberAgent for a synthetic design matrix entry.


Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 11m49s), codex_security (codex/security, done, 9m13s) | Total: 21m9s | Job: 19558

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Fail

Medium issues remain in synthesis agent fallback and headed CI pass comments.

Medium

  • internal/daemon/synthesis_worker.go:237: configureSynthesisAgent resolves the initial synthesis agent without passing the stored BackupAgent, so an unavailable primary can fall through to an arbitrary default fallback agent/model instead of the explicit synthesis backup configured for the panel.

    • Fix: Pass job.BackupAgent into availability resolution and use job.BackupModel when that backup is selected, or immediately fail over to the stored backup on primary availability failure.
  • internal/daemon/ci_poller.go:2308: Headed panel comments suppress output when the parsed verdict is pass. For single-review panel passthroughs whose output is just No issues found., the CI comment becomes only the “Combined Review” title plus footer, with no actual pass/result text.

    • Fix: In the headed path, include a pass line such as No issues found. or preserve pass output instead of suppressing it.

Panel: ci_default_security | Synthesis: codex, 9s | Members: codex_default (codex/default, done, 8m59s), codex_security (codex/security, done, 4m27s) | Total: 13m35s | Job: 19573

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (8464c57)

Medium issues found; no Critical or High findings.

Medium

  • internal/daemon/server.go:240 - The panel sweep goroutine starts before server readiness is confirmed, but startup failure paths stop the config watcher and worker pool without canceling the sweep context. If readiness fails while the parent context remains active, runPanelSweep can keep running against a daemon that failed to start.

    • Suggested fix: Start the panel sweep only after readiness succeeds, or call cancelSweep() on every startup failure/unready return path before returning.
  • internal/storage/reviews.go:153 - FindReusableSessionCandidates now only allows review and range job types, excluding completed dirty reviews from session reuse even though dirty review enqueueing still computes a reusable session SHA. This regresses session continuity for repeated dirty reviews on the same branch and agent.

    • Suggested fix: Include dirty in the allowed job types, or change the filter to exclude only panel roles and known non-review job types.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 12m4s), codex_security (codex/security, done, 5m44s) | Total: 17m56s | Job: 19615

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (25bbee7)

Panel synthesis adds useful fan-out support, but one Medium issue needs fixing before the PR is safe to merge.

Medium

  • internal/daemon/ci_poller.go:1577: Headed panel synthesis output can bypass MaxCommentLen truncation. If the synthesis agent emits output already starting with ## roborev:, appendPanelPRFooter returns it without the UTF-8-safe truncation used by the normal panel formatter. An oversized comment can fail GitHub posting, release the panel posting claim, and cause repeated retries instead of posting a bounded comment.

    Suggested fix: Apply the same UTF-8-safe truncation before appending the panel footer for headed output, or route headed output through the bounded panel formatter while preserving a single header.


Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 9m4s), codex_security (codex/security, done, 4m58s) | Total: 14m9s | Job: 19639

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (bdaca58)

Summary verdict: One Medium issue remains; no Medium-or-higher security findings were reported.

Medium

  • internal/daemon/ci_poller.go:729: The auto-design panel member stores the raw DesignAgentFromConfig preference without the availability/backup resolution used by CI matrix members and the old auto-design path. If the configured design primary is unavailable but design_backup_agent is available, CI can enqueue the design member with the unavailable primary and wrong model, then run a generic fallback or fail instead of honoring the design backup.
    • Fix: Resolve the appended design member through the same config-only workflow availability path as CI members, including the design backup agent, and set the model with ModelForSelectedAgent.

Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 15m2s), codex_security (codex/security, done, 5m9s) | Total: 20m18s | Job: 19690

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (845abcb)

Severity: medium

File: internal/daemon/ci_poller.go:1525, internal/daemon/ci_poller.go:454

A PR author can close their PR while a panel run is finishing, causing postPanelRun to mark the panel as posted even though no comment was posted. If the author reopens the same HEAD, alreadyReviewedPR treats the non-retired row as already reviewed, so no new panel review/comment is enqueued for that commit. This can suppress the CI review output for a reopened PR when the roborev status is not enforced.

Remediation: Do not call MarkPanelPosted for closed/merged PR abandonment. Retire or delete the mapping instead, or track abandoned-closed separately so alreadyReviewedPR does not treat it as reviewed after a reopen.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, canceled, 15m17s), codex_security (codex/security, done, 4m34s) | Total: 19m51s | Job: 19711

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (ff0cd76)

Medium severity issue found.

Medium

  • internal/storage/ci_panels.go:233 - MarkPanelRetired can retire a panel after postPanelRun has already claimed it for posting. Because the poster does not re-read the row before sending the GitHub comment, a new push can supersede an in-flight panel while it still posts a comment for the old head, and MarkPanelPosted then silently no-ops against the retired row.

    Suggested fix: Make retirement and posting mutually exclusive with a CAS/state check: skip active posting claims or re-check retired_at/head state immediately before posting, and treat a zero-row MarkPanelPosted as an abort/error.


Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 10m12s), codex_security (codex/security, done, 5m27s) | Total: 15m45s | Job: 19732

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (246e1ca)

Review needs changes: one medium-severity issue found.

Medium

  • internal/git/git.go:520: ResolveSHA now runs git rev-parse with a cleaned Git environment, but that cleanup also drops GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM. CI or containerized environments may rely on those variables for controlled Git config such as safe.directory, so ref resolution can fail or read unintended config. Preserve global/system config selectors and limit cleanup to repository-scoped variables such as GIT_DIR, GIT_WORK_TREE, index/object paths, namespace, prefix, and quarantine state.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 10m35s), codex_security (codex/security, done, 4m10s) | Total: 14m51s | Job: 19744

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (0f37a5f)

Review needs changes: one Medium issue was found.

Medium

  • internal/git/git.go:523 - ResolveSHA now runs git rev-parse with cleanGitRepoEnv, which strips Git config-related environment variables such as GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM, GIT_CONFIG_COUNT, and GIT_CONFIG_KEY_/VALUE_. These are not repository locator variables, and callers may rely on them for behavior like safe.directory, isolated global config, or per-process config injection. This can make ref resolution fail or use the wrong Git configuration in CI/worktree environments.

    Suggested fix: Limit cleanup to repository/worktree locator variables such as GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, object directories, and diff hooks, while preserving global/system config selectors and injected GIT_CONFIG_COUNT entries.


Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 5m14s), codex_security (codex/security, done, 6m3s) | Total: 11m24s | Job: 19759

@mariusvniekerk
Copy link
Copy Markdown
Collaborator

The git stuff its workgin on feels very out of scope for ths pr.

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (775d596)

Medium issue found.

Medium

  • internal/storage/db.go:971 - drainAndDropOldCIBatchTables only checks for ci_pr_batches before running an UPDATE that selects from ci_pr_batch_jobs. Because the statements run outside a transaction and drop ci_pr_batch_jobs before ci_pr_batches, an interrupted migration can leave only ci_pr_batches; the next daemon startup will fail every Open() with no such table: ci_pr_batch_jobs.

    Fix: Make the migration atomic and idempotent: wrap the update/drop sequence in a transaction and/or check that ci_pr_batch_jobs exists before referencing it, then drop each legacy table independently.


Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 6m10s), codex_security (codex/security, done, 5m13s) | Total: 11m30s | Job: 19774

@wesm
Copy link
Copy Markdown
Member Author

wesm commented Jun 2, 2026

The git stuff its workgin on feels very out of scope for ths pr.

Yeah, I scrubbed it out

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (1a1d059)


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, canceled, 15m57s), codex_security (codex/security, done, 7m48s) | Total: 23m45s | Job: 19792

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (67844b0)

Medium issue found in the panel synthesis path; no high or critical findings.

Medium

  • internal/daemon/panel_enqueue.go:502 - Single-member panels overwrite the persisted synthesis job’s agent/model/reasoning with the member’s display values. If min_severity is above low, the synthesis worker cannot use the passthrough branch and runs synthesis with those overwritten fields, so the configured synthesis_agent/synthesis_model may be ignored or the filtering pass may fail unexpectedly.

    Suggested fix: Keep synthesis job execution fields set to the resolved synthesis spec, and move the single-member display override into response/rendering metadata, or have the worker use explicit synthesis execution fields when filtering is required.


Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 12m26s), codex_security (codex/security, done, 8m47s) | Total: 21m21s | Job: 19816

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Review Failed (bf2d282)

All review jobs in this batch failed.

  • codex (default): failed
  • codex (security): failed

Check CI logs for error details.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, failed, 4s), codex_security (codex/security, failed, 3s) | Total: 7s | Job: 19948

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Review Failed (58b47de)

All review jobs in this batch failed.

  • codex (default): failed
  • codex (security): failed

Check CI logs for error details.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, failed, 3s), codex_security (codex/security, failed, 3s) | Total: 6s | Job: 19963

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Review Failed (caa9a9d)

All review jobs in this batch failed.

  • codex (default): failed
  • codex (security): failed

Check CI logs for error details.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, failed, 4s), codex_security (codex/security, failed, 4s) | Total: 8s | Job: 19969

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Review Failed (f392170)

All review jobs in this batch failed.

  • codex (default): failed
  • codex (security): failed

Check CI logs for error details.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, failed, 4s), codex_security (codex/security, failed, 3s) | Total: 7s | Job: 19993

@wesm wesm force-pushed the feat/proper-subagent-reviews branch from f392170 to 79af1cb Compare June 2, 2026 22:22
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Review Failed (79af1cb)

All review jobs in this batch failed.

  • codex (default): failed
  • codex (security): failed

Check CI logs for error details.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, failed, 3s), codex_security (codex/security, failed, 3s) | Total: 6s | Job: 19999

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Review Failed (29cd49e)

All review jobs in this batch failed.

  • codex (default): failed
  • codex (security): failed

Check CI logs for error details.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, failed, 3s), codex_security (codex/security, failed, 3s) | Total: 6s | Job: 20011

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 2, 2026

roborev: Combined Review (f2fe595)


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 11m54s), codex_security (codex/security, failed, 2s) | Total: 11m56s | Job: 20017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants