You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reviews system (V1, #192/#197) is structurally one PR per worker session. But the workspace project model (#165, merged) makes a single session legitimately span multiple repos and therefore multiple PRs. In that case the reviewer cannot represent per-PR review — it silently reviews an arbitrary one. This issue proposes making the review unit (session, PR) so multi-repo/multi-PR workspace sessions are first-class.
Analyzed against:8a41660 | Type: enhancement (design/scope), not a V1 regression
Why this is concrete, not hypothetical: workspace projects (#165)
PR #165 shipped workspace project registration: a kind=workspace project is a root-as-repo parent with multiple direct child repos (workspace_repos), and a session spawned on it materialises one worktree per repo — session_worktrees is PRIMARY KEY (session_id, repo_name) (migrations/0009_workspace_projects.sql:25). So one session → N repos → up to N PRs is a designed outcome of the workspace feature, not an edge case.
The review engine assumes the opposite. Reviewing a workspace session today would review whichever PR happens to come back first, with no way to review the other repos' PRs.
Current behavior (single-PR-by-design)
Three layers pin one PR per session:
Schema — review table is session_id NOT NULL UNIQUE (migrations/0011_add_review_tables.sql:10). Exactly one review row per session; pr_url is a mutable column on it. A repeat trigger reuses the row.
Engine — internal/review/review.goworkerPR():
prs, _:=e.prs.ListPRsBySession(ctx, workerID)
iflen(prs) ==0 { return..., ErrInvalid }
returnprs[0] // arbitrary pick; no ORDER BY in the query
Design — V1 locked decisions describe "one reviewer per worker," "the worker's PR" (singular), idempotency keyed on "the same PR head."
Meanwhile the storage layer already permits multiple PRs per session: pr.session_id has only a non-unique index (idx_pr_session, 0001_init.sql:68), and the schema comment admits the one-PR invariant is "enforced at runtime" (0001_init.sql:52) — i.e. not enforced in SQL. ListPRsBySession returns a slice; prs[0] with no ORDER BY is non-deterministic when len(prs) > 1.
Proposed direction
Make the review unit (session, pr) rather than (session), with PR identity traceable to its workspace repo:
Schema: drop review.session_id UNIQUE; key the review on (session_id, pr_url) (composite unique). Keep review_run per-pass. Optionally carry repo_name so a workspace review batch can be presented per child repo.
Engine:Trigger/List/Submit resolve a PR identity, not just workerID. Replace the prs[0] pick with explicit PR selection. Interim hardening (cheap, do first): make the pick deterministic (newest PR) and signal when len(prs) > 1 instead of silently choosing.
API: reviews surface becomes PR-scoped under a session (e.g. a prUrl/repo selector on the existing /sessions/{id}/reviews/... routes), so a workspace session can list/trigger reviews per repo.
#236 proposes multiple reviewers per one PR with storage direction session_id + target_sha + reviewer_harness + attempt — that key still assumes one PR per session. This issue is the orthogonal axis (multiple PRs per session, driven by #165). If both land, review identity becomes ~session_id + pr_url + target_sha + reviewer_harness + attempt. #236's schema work should reserve room for pr_url so it isn't re-keyed later.
Related
PR #165 — workspace project registration: the multi-repo/multi-PR-per-session driver
Summary
The reviews system (V1, #192/#197) is structurally one PR per worker session. But the workspace project model (#165, merged) makes a single session legitimately span multiple repos and therefore multiple PRs. In that case the reviewer cannot represent per-PR review — it silently reviews an arbitrary one. This issue proposes making the review unit
(session, PR)so multi-repo/multi-PR workspace sessions are first-class.Analyzed against:
8a41660| Type: enhancement (design/scope), not a V1 regressionWhy this is concrete, not hypothetical: workspace projects (#165)
PR #165 shipped workspace project registration: a
kind=workspaceproject is a root-as-repo parent with multiple direct child repos (workspace_repos), and a session spawned on it materialises one worktree per repo —session_worktreesisPRIMARY KEY (session_id, repo_name)(migrations/0009_workspace_projects.sql:25). So one session → N repos → up to N PRs is a designed outcome of the workspace feature, not an edge case.The review engine assumes the opposite. Reviewing a workspace session today would review whichever PR happens to come back first, with no way to review the other repos' PRs.
Current behavior (single-PR-by-design)
Three layers pin one PR per session:
reviewtable issession_id NOT NULL UNIQUE(migrations/0011_add_review_tables.sql:10). Exactly onereviewrow per session;pr_urlis a mutable column on it. A repeat trigger reuses the row.internal/review/review.goworkerPR():Meanwhile the storage layer already permits multiple PRs per session:
pr.session_idhas only a non-unique index (idx_pr_session,0001_init.sql:68), and the schema comment admits the one-PR invariant is "enforced at runtime" (0001_init.sql:52) — i.e. not enforced in SQL.ListPRsBySessionreturns a slice;prs[0]with noORDER BYis non-deterministic whenlen(prs) > 1.Proposed direction
Make the review unit
(session, pr)rather than(session), with PR identity traceable to its workspace repo:review.session_id UNIQUE; key the review on(session_id, pr_url)(composite unique). Keepreview_runper-pass. Optionally carryrepo_nameso a workspace review batch can be presented per child repo.Trigger/List/Submitresolve a PR identity, not justworkerID. Replace theprs[0]pick with explicit PR selection. Interim hardening (cheap, do first): make the pick deterministic (newest PR) and signal whenlen(prs) > 1instead of silently choosing.prUrl/reposelector on the existing/sessions/{id}/reviews/...routes), so a workspace session can list/trigger reviews per repo.Acceptance criteria
Interaction with #236 (Review V2)
#236 proposes multiple reviewers per one PR with storage direction
session_id + target_sha + reviewer_harness + attempt— that key still assumes one PR per session. This issue is the orthogonal axis (multiple PRs per session, driven by #165). If both land, review identity becomes ~session_id + pr_url + target_sha + reviewer_harness + attempt. #236's schema work should reserve room forpr_urlso it isn't re-keyed later.Related
reviewuniqueness area)