feat(session): support multiple PRs per session (#230)

* feat(session): support multiple PRs per session

A session can now own several pull requests (a root plus stacked
children) instead of being capped at one. The SQLite schema was already
1-session->many-PR (pr.url PK, session_id a plain FK), so this is a
behavioural change across the observe -> persist -> derive -> react
pipeline, not a migration.

- observe: the SCM observer discovers every open PR whose source branch
  matches a session branch or descends from it ("branch/..." stacking),
  attributing each to the owning session; the longest matching branch
  wins so a child session claims its own stacked PRs.
- derive: session status is a worst-wins aggregate over all owned PRs,
  with a stack model (B is a child of A iff B.target == A.source and A is
  open) exposed via prs[] on every session read DTO.
- react: per-PR reactions; a stacked child blocked by an open parent is
  exempt from the rebase/merge-conflict nudge (only the bottom of the
  stack is eligible), and the session completes only when no PR is open
  and at least one merged.
- tests: unit coverage across stack/status/observer/lifecycle, a
  real-SQLite ListPRFactsForSession test for the stacked-PR read path,
  and a functional end-to-end integration test driving the real store +
  lifecycle + observer through attribution, completion, and stacked-child
  nudge suppression.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(scm): ignore fork heads in PR attribution and persist discovered siblings before completion

Branch-prefix attribution now requires a discovered PR's head branch to live
in the project repo. A fork PR can reuse a session's branch name while its
commits live in the fork, so the previous code could auto-claim foreign work.
Carry head repo full_name from the REST list response and skip any PR whose
head repo is not the base repo.

discoverNewPRs also writes each newly discovered PR as an open baseline row
before the refresh/lifecycle pass runs. A session can own several PRs, and a
terminal observation triggers a completion check that reads all of the
session's PRs from the store. Without the early write, an open sibling found
in the same poll was not yet durable and the session could terminate while
that PR was still open.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(session): surface actionable signals from blocked stacked children; clarify worker prompt

Status aggregation previously dropped any open PR blocked by an open parent,
hiding actionable child signals (failing CI, draft, requested changes,
unresolved comments) behind the parent's status. A blocked child still cannot
merge, so its readiness signals (mergeable/approved/review-pending/open) stay
suppressed, but its problem signals now contribute to the worst-wins aggregate.
The all-blocked fallback is preserved so a session never goes dark.

The worker multi-PR prompt said independent PRs could branch off the base
branch as usual, which conflicts with branch-prefix attribution. Clarify that
a PR may target the base branch, but its source branch must stay under the
session branch namespace for AO to track it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Pritom Mazumdar 2026-06-17 17:31:28 +05:30 committed by GitHub
parent eeaddd221a
commit 3986d488c8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
26 changed files with 1347 additions and 214 deletions

View File

@ -57,36 +57,36 @@ func (p *Provider) RepoPRListGuard(ctx context.Context, repo ports.SCMRepo, etag
return ports.SCMGuardResult{ETag: firstNonEmptyHeader(resp.ETag, etag), NotModified: resp.NotModified}, nil return ports.SCMGuardResult{ETag: firstNonEmptyHeader(resp.ETag, etag), NotModified: resp.NotModified}, nil
} }
// DetectPRByBranch finds an open PR whose head branch matches the session branch. // ListOpenPRsByRepo lists every open pull request in the repository so the
func (p *Provider) DetectPRByBranch(ctx context.Context, repo ports.SCMRepo, branch string) (ports.SCMPRObservation, error) { // observer can attribute each to a session by head-branch prefix. It paginates
branch = strings.TrimSpace(branch) // the REST pulls endpoint; AO repos are not expected to carry thousands of
if branch == "" { // concurrent open PRs, and the observer only calls this when the repo PR-list
return ports.SCMPRObservation{}, fmt.Errorf("%w: empty branch", ErrNotFound) // ETag guard reports a change.
func (p *Provider) ListOpenPRsByRepo(ctx context.Context, repo ports.SCMRepo) ([]ports.SCMPRObservation, error) {
const perPage = 100
out := []ports.SCMPRObservation{}
for page := 1; ; page++ {
q := url.Values{}
q.Set("state", "open")
q.Set("sort", "updated")
q.Set("direction", "desc")
q.Set("per_page", strconv.Itoa(perPage))
q.Set("page", strconv.Itoa(page))
resp, err := p.client.doREST(ctx, http.MethodGet, repoPath(repo.Owner, repo.Name, "pulls"), q, nil)
if err != nil {
return nil, err
}
var pulls []restListPull
if err := json.Unmarshal(resp.Body, &pulls); err != nil {
return nil, fmt.Errorf("github scm: decode open PR list: %w", err)
}
for _, pull := range pulls {
out = append(out, restListPullToSCM(pull))
}
if len(pulls) < perPage {
return out, nil
}
} }
pulls, err := p.detectPRByHead(ctx, repo, repo.Owner+":"+branch)
if err != nil {
return ports.SCMPRObservation{}, err
}
if len(pulls) == 0 {
return ports.SCMPRObservation{}, fmt.Errorf("%w: no open PR for branch %s", ErrNotFound, branch)
}
return restListPullToSCM(pulls[0]), nil
}
func (p *Provider) detectPRByHead(ctx context.Context, repo ports.SCMRepo, head string) ([]restListPull, error) {
q := url.Values{}
q.Set("state", "open")
q.Set("head", head)
q.Set("per_page", "10")
resp, err := p.client.doREST(ctx, http.MethodGet, repoPath(repo.Owner, repo.Name, "pulls"), q, nil)
if err != nil {
return nil, err
}
var pulls []restListPull
if err := json.Unmarshal(resp.Body, &pulls); err != nil {
return nil, fmt.Errorf("github scm: decode branch PR list: %w", err)
}
return pulls, nil
} }
// CommitChecksGuard checks GitHub's per-commit check-runs ETag guard. // CommitChecksGuard checks GitHub's per-commit check-runs ETag guard.
@ -201,8 +201,11 @@ type restListPull struct {
Draft bool `json:"draft"` Draft bool `json:"draft"`
Title string `json:"title"` Title string `json:"title"`
Head struct { Head struct {
Ref string `json:"ref"` Ref string `json:"ref"`
SHA string `json:"sha"` SHA string `json:"sha"`
Repo struct {
FullName string `json:"full_name"`
} `json:"repo"`
} `json:"head"` } `json:"head"`
Base struct { Base struct {
Ref string `json:"ref"` Ref string `json:"ref"`
@ -224,6 +227,7 @@ func restListPullToSCM(pull restListPull) ports.SCMPRObservation {
Draft: pull.Draft, Draft: pull.Draft,
Closed: closed, Closed: closed,
SourceBranch: pull.Head.Ref, SourceBranch: pull.Head.Ref,
HeadRepo: pull.Head.Repo.FullName,
TargetBranch: pull.Base.Ref, TargetBranch: pull.Base.Ref,
HeadSHA: pull.Head.SHA, HeadSHA: pull.Head.SHA,
Title: pull.Title, Title: pull.Title,

View File

@ -258,6 +258,24 @@ func TestParsePRURL(t *testing.T) {
} }
} }
func TestRestListPullToSCMCarriesHeadRepo(t *testing.T) {
var pull restListPull
pull.Number = 7
pull.State = "open"
pull.Head.Ref = "feat/x"
pull.Head.SHA = "deadbeef"
pull.Head.Repo.FullName = "forker/hello"
pull.Base.Ref = "main"
obs := restListPullToSCM(pull)
if obs.SourceBranch != "feat/x" {
t.Fatalf("SourceBranch = %q, want feat/x", obs.SourceBranch)
}
if obs.HeadRepo != "forker/hello" {
t.Fatalf("HeadRepo = %q, want forker/hello", obs.HeadRepo)
}
}
func TestObserve_HappyPath(t *testing.T) { func TestObserve_HappyPath(t *testing.T) {
f := newFakeGH(t) f := newFakeGH(t)
fx := basePRFixture() fx := basePRFixture()

View File

@ -16,6 +16,8 @@ type PRFacts struct {
Review ReviewDecision Review ReviewDecision
Mergeability Mergeability Mergeability Mergeability
ReviewComments bool // has unresolved review comments (any author) to address ReviewComments bool // has unresolved review comments (any author) to address
SourceBranch string
TargetBranch string
UpdatedAt time.Time UpdatedAt time.Time
} }

View File

@ -61,4 +61,8 @@ type Session struct {
SessionRecord SessionRecord
Status SessionStatus `json:"status"` Status SessionStatus `json:"status"`
TerminalHandleID string `json:"terminalHandleId,omitempty"` TerminalHandleID string `json:"terminalHandleId,omitempty"`
// PRs are the session's attributed pull requests (one session can own many).
// They feed status derivation and are surfaced on the API read model. Not
// serialized here: the HTTP boundary maps them to the curated wire shape.
PRs []PRFacts `json:"-"`
} }

View File

@ -1257,6 +1257,49 @@ components:
- sessionId - sessionId
- reason - reason
type: object type: object
ControllersSessionView:
properties:
activity:
$ref: '#/components/schemas/DomainActivity'
createdAt:
format: date-time
type: string
displayName:
type: string
harness:
type: string
id:
type: string
isTerminated:
type: boolean
issueId:
type: string
kind:
type: string
projectId:
type: string
prs:
items:
$ref: '#/components/schemas/SessionPRFacts'
type: array
status:
type: string
terminalHandleId:
type: string
updatedAt:
format: date-time
type: string
required:
- id
- projectId
- kind
- activity
- isTerminated
- createdAt
- updatedAt
- status
- prs
type: object
DegradedProject: DegradedProject:
properties: properties:
id: id:
@ -1352,7 +1395,7 @@ components:
properties: properties:
sessions: sessions:
items: items:
$ref: '#/components/schemas/Session' $ref: '#/components/schemas/ControllersSessionView'
type: array type: array
required: required:
- sessions - sessions
@ -1591,7 +1634,7 @@ components:
ok: ok:
type: boolean type: boolean
session: session:
$ref: '#/components/schemas/Session' $ref: '#/components/schemas/ControllersSessionView'
sessionId: sessionId:
type: string type: string
required: required:
@ -1687,44 +1730,6 @@ components:
- sessionId - sessionId
- message - message
type: object type: object
Session:
properties:
activity:
$ref: '#/components/schemas/DomainActivity'
createdAt:
format: date-time
type: string
displayName:
type: string
harness:
type: string
id:
type: string
isTerminated:
type: boolean
issueId:
type: string
kind:
type: string
projectId:
type: string
status:
type: string
terminalHandleId:
type: string
updatedAt:
format: date-time
type: string
required:
- id
- projectId
- kind
- activity
- isTerminated
- createdAt
- updatedAt
- status
type: object
SessionPRFacts: SessionPRFacts:
properties: properties:
ci: ci:
@ -1762,7 +1767,7 @@ components:
SessionResponse: SessionResponse:
properties: properties:
session: session:
$ref: '#/components/schemas/Session' $ref: '#/components/schemas/ControllersSessionView'
required: required:
- session - session
type: object type: object

View File

@ -111,9 +111,18 @@ type CleanupSessionsQuery struct {
Project string `query:"project,omitempty" description:"Project id filter. When omitted, clean terminated sessions across all projects."` Project string `query:"project,omitempty" description:"Project id filter. When omitted, clean terminated sessions across all projects."`
} }
// SessionView is the session wire shape: the domain read model plus the
// session's attributed pull requests in the curated SessionPRFacts shape. One
// session can own many PRs (e.g. a stack), so prs is a list. The embedded
// domain.Session.PRs field is json:"-"; this curated prs is what serializes.
type SessionView struct {
domain.Session
PRs []SessionPRFacts `json:"prs"`
}
// ListSessionsResponse is the body of GET /api/v1/sessions. // ListSessionsResponse is the body of GET /api/v1/sessions.
type ListSessionsResponse struct { type ListSessionsResponse struct {
Sessions []domain.Session `json:"sessions"` Sessions []SessionView `json:"sessions"`
} }
// SpawnSessionRequest is the body of POST /api/v1/sessions. // SpawnSessionRequest is the body of POST /api/v1/sessions.
@ -128,7 +137,7 @@ type SpawnSessionRequest struct {
// SessionResponse is the { session } body shared by session create/get. // SessionResponse is the { session } body shared by session create/get.
type SessionResponse struct { type SessionResponse struct {
Session domain.Session `json:"session"` Session SessionView `json:"session"`
} }
// RenameSessionRequest is the body of PATCH /api/v1/sessions/{sessionId}. // RenameSessionRequest is the body of PATCH /api/v1/sessions/{sessionId}.
@ -147,7 +156,7 @@ type RenameSessionResponse struct {
type RestoreSessionResponse struct { type RestoreSessionResponse struct {
OK bool `json:"ok"` OK bool `json:"ok"`
SessionID domain.SessionID `json:"sessionId"` SessionID domain.SessionID `json:"sessionId"`
Session domain.Session `json:"session"` Session SessionView `json:"session"`
} }
// KillSessionResponse is the body of POST /api/v1/sessions/{sessionId}/kill. // KillSessionResponse is the body of POST /api/v1/sessions/{sessionId}/kill.

View File

@ -88,7 +88,7 @@ func (c *SessionsController) list(w http.ResponseWriter, r *http.Request) {
envelope.WriteError(w, r, err) envelope.WriteError(w, r, err)
return return
} }
envelope.WriteJSON(w, http.StatusOK, ListSessionsResponse{Sessions: sessions}) envelope.WriteJSON(w, http.StatusOK, ListSessionsResponse{Sessions: sessionViews(sessions)})
} }
func (c *SessionsController) spawn(w http.ResponseWriter, r *http.Request) { func (c *SessionsController) spawn(w http.ResponseWriter, r *http.Request) {
@ -117,7 +117,7 @@ func (c *SessionsController) spawn(w http.ResponseWriter, r *http.Request) {
envelope.WriteError(w, r, err) envelope.WriteError(w, r, err)
return return
} }
envelope.WriteJSON(w, http.StatusCreated, SessionResponse{Session: sess}) envelope.WriteJSON(w, http.StatusCreated, SessionResponse{Session: sessionView(sess)})
} }
func (c *SessionsController) get(w http.ResponseWriter, r *http.Request) { func (c *SessionsController) get(w http.ResponseWriter, r *http.Request) {
@ -130,7 +130,7 @@ func (c *SessionsController) get(w http.ResponseWriter, r *http.Request) {
envelope.WriteError(w, r, err) envelope.WriteError(w, r, err)
return return
} }
envelope.WriteJSON(w, http.StatusOK, SessionResponse{Session: sess}) envelope.WriteJSON(w, http.StatusOK, SessionResponse{Session: sessionView(sess)})
} }
func (c *SessionsController) listPRs(w http.ResponseWriter, r *http.Request) { func (c *SessionsController) listPRs(w http.ResponseWriter, r *http.Request) {
@ -204,7 +204,7 @@ func (c *SessionsController) restore(w http.ResponseWriter, r *http.Request) {
envelope.WriteError(w, r, err) envelope.WriteError(w, r, err)
return return
} }
envelope.WriteJSON(w, http.StatusOK, RestoreSessionResponse{OK: true, SessionID: sessionID(r), Session: sess}) envelope.WriteJSON(w, http.StatusOK, RestoreSessionResponse{OK: true, SessionID: sessionID(r), Session: sessionView(sess)})
} }
func (c *SessionsController) kill(w http.ResponseWriter, r *http.Request) { func (c *SessionsController) kill(w http.ResponseWriter, r *http.Request) {
@ -348,7 +348,7 @@ func (c *SessionsController) listOrchestrators(w http.ResponseWriter, r *http.Re
envelope.WriteError(w, r, err) envelope.WriteError(w, r, err)
return return
} }
envelope.WriteJSON(w, http.StatusOK, ListSessionsResponse{Sessions: sessions}) envelope.WriteJSON(w, http.StatusOK, ListSessionsResponse{Sessions: sessionViews(sessions)})
} }
func (c *SessionsController) getOrchestrator(w http.ResponseWriter, r *http.Request) { func (c *SessionsController) getOrchestrator(w http.ResponseWriter, r *http.Request) {
@ -365,7 +365,7 @@ func (c *SessionsController) getOrchestrator(w http.ResponseWriter, r *http.Requ
envelope.WriteAPIError(w, r, http.StatusNotFound, "not_found", "SESSION_NOT_FOUND", "Unknown session", nil) envelope.WriteAPIError(w, r, http.StatusNotFound, "not_found", "SESSION_NOT_FOUND", "Unknown session", nil)
return return
} }
envelope.WriteJSON(w, http.StatusOK, SessionResponse{Session: sess}) envelope.WriteJSON(w, http.StatusOK, SessionResponse{Session: sessionView(sess)})
} }
func sessionID(r *http.Request) domain.SessionID { func sessionID(r *http.Request) domain.SessionID {
@ -436,6 +436,18 @@ func writeSessionPRError(w http.ResponseWriter, r *http.Request, err error) {
} }
} }
func sessionView(s domain.Session) SessionView {
return SessionView{Session: s, PRs: sessionPRFacts(s.PRs)}
}
func sessionViews(sessions []domain.Session) []SessionView {
out := make([]SessionView, 0, len(sessions))
for _, s := range sessions {
out = append(out, sessionView(s))
}
return out
}
func sessionPRFacts(prs []domain.PRFacts) []SessionPRFacts { func sessionPRFacts(prs []domain.PRFacts) []SessionPRFacts {
out := make([]SessionPRFacts, 0, len(prs)) out := make([]SessionPRFacts, 0, len(prs))
for _, pr := range prs { for _, pr := range prs {

View File

@ -67,7 +67,7 @@ func (s *scmMessengerSpy) snapshot() []scmCapturedNudge {
} }
// cannedSCMProvider satisfies observe/scm.Provider with hand-built observations // cannedSCMProvider satisfies observe/scm.Provider with hand-built observations
// keyed by branch (for DetectPRByBranch) and by PR number (for everything else, // keyed by branch (for ListOpenPRsByRepo) and by PR number (for everything else,
// since every test case uses scmTestRepo). It is the integration-package analog // since every test case uses scmTestRepo). It is the integration-package analog
// of observer_test.go's fakeProvider: the SCM adapter has its own httptest-based // of observer_test.go's fakeProvider: the SCM adapter has its own httptest-based
// coverage, so this test holds the provider constant and exercises every other // coverage, so this test holds the provider constant and exercises every other
@ -101,14 +101,14 @@ func (p *cannedSCMProvider) RepoPRListGuard(_ context.Context, _ ports.SCMRepo,
return ports.SCMGuardResult{ETag: "repo-etag"}, nil return ports.SCMGuardResult{ETag: "repo-etag"}, nil
} }
func (p *cannedSCMProvider) DetectPRByBranch(_ context.Context, _ ports.SCMRepo, branch string) (ports.SCMPRObservation, error) { func (p *cannedSCMProvider) ListOpenPRsByRepo(_ context.Context, _ ports.SCMRepo) ([]ports.SCMPRObservation, error) {
p.mu.Lock() p.mu.Lock()
defer p.mu.Unlock() defer p.mu.Unlock()
pr, ok := p.detected[branch] out := make([]ports.SCMPRObservation, 0, len(p.detected))
if !ok { for _, pr := range p.detected {
return ports.SCMPRObservation{}, ports.ErrSCMNotFound out = append(out, pr)
} }
return pr, nil return out, nil
} }
func (p *cannedSCMProvider) CommitChecksGuard(_ context.Context, _ ports.SCMRepo, _, _ string) (ports.SCMGuardResult, error) { func (p *cannedSCMProvider) CommitChecksGuard(_ context.Context, _ ports.SCMRepo, _, _ string) (ports.SCMGuardResult, error) {
@ -274,7 +274,7 @@ func TestSCMObserverEndToEnd(t *testing.T) {
logTail = "setup\nsetup\nFAILED: build broke\n" logTail = "setup\nsetup\nFAILED: build broke\n"
) )
f.provider.detected["feat/x"] = ports.SCMPRObservation{ f.provider.detected["feat/x"] = ports.SCMPRObservation{
URL: prURL, Number: 42, SourceBranch: "feat/x", TargetBranch: "main", HeadSHA: headSHA, URL: prURL, Number: 42, SourceBranch: "feat/x", HeadRepo: scmTestRepo.Repo, TargetBranch: "main", HeadSHA: headSHA,
} }
f.provider.observations[42] = failingSCMObservation(prURL, 42, headSHA, logTail) f.provider.observations[42] = failingSCMObservation(prURL, 42, headSHA, logTail)
@ -350,7 +350,7 @@ func TestSCMObserverEndToEnd(t *testing.T) {
// the production fallback the observer relies on when the upstream // the production fallback the observer relies on when the upstream
// ETag guard misses. The ETag-driven 304 short-circuit on the same // ETag guard misses. The ETag-driven 304 short-circuit on the same
// SHA is covered by the unit tests in observe/scm/observer_test.go // SHA is covered by the unit tests in observe/scm/observer_test.go
// (Poll_RepoETag304SkipsDetectPR, Poll_CIETagChangeRefreshesWhenRepoUnchanged). // (Poll_RepoETag304SkipsListPRs, Poll_CIETagChangeRefreshesWhenRepoUnchanged).
if err := f.observer.Poll(ctx); err != nil { if err := f.observer.Poll(ctx); err != nil {
t.Fatalf("second Poll: %v", err) t.Fatalf("second Poll: %v", err)
} }
@ -374,7 +374,7 @@ func TestSCMObserverEndToEnd(t *testing.T) {
headSHA = "cafef00d" headSHA = "cafef00d"
) )
f.provider.detected["feat/x"] = ports.SCMPRObservation{ f.provider.detected["feat/x"] = ports.SCMPRObservation{
URL: prURL, Number: 77, SourceBranch: "feat/x", TargetBranch: "main", HeadSHA: headSHA, Merged: true, URL: prURL, Number: 77, SourceBranch: "feat/x", HeadRepo: scmTestRepo.Repo, TargetBranch: "main", HeadSHA: headSHA, Merged: true,
} }
f.provider.observations[77] = mergedSCMObservation(prURL, 77, headSHA) f.provider.observations[77] = mergedSCMObservation(prURL, 77, headSHA)
@ -397,7 +397,7 @@ func TestSCMObserverEndToEnd(t *testing.T) {
t.Run("Branch with no open PR writes nothing and sends no nudge", func(t *testing.T) { t.Run("Branch with no open PR writes nothing and sends no nudge", func(t *testing.T) {
ctx := context.Background() ctx := context.Background()
f := newSCMFixture(t, "feat/quiet") f := newSCMFixture(t, "feat/quiet")
// No entry in provider.detected — DetectPRByBranch returns ErrSCMNotFound, // No entry in provider.detected — ListOpenPRsByRepo returns an empty list,
// the production "no PR yet" signal. // the production "no PR yet" signal.
if err := f.observer.Poll(ctx); err != nil { if err := f.observer.Poll(ctx); err != nil {
@ -416,3 +416,224 @@ func TestSCMObserverEndToEnd(t *testing.T) {
} }
}) })
} }
// openSCMObservation builds an open-PR observation with caller-chosen branches
// and mergeability, CI passing and no review. The multi-PR cases drive the stack
// model (target/source branch pairs) and the completion rule, so branches must
// be configurable rather than the fixed feat/x->main the single-PR helpers bake in.
func openSCMObservation(prURL string, num int, headSHA, src, tgt string, merge domain.Mergeability) ports.SCMObservation {
mo := ports.SCMMergeabilityObservation{State: string(merge)}
switch merge {
case domain.MergeMergeable:
mo.Mergeable = true
case domain.MergeConflicting:
mo.Conflict = true
mo.Blockers = []string{"conflicts"}
}
return ports.SCMObservation{
Fetched: true,
Provider: "github", Host: "github.com", Repo: "octocat/hello",
PR: ports.SCMPRObservation{
URL: prURL,
HTMLURL: prURL,
Number: num,
State: string(domain.PRStateOpen),
SourceBranch: src,
TargetBranch: tgt,
HeadSHA: headSHA,
Title: "wip",
},
CI: ports.SCMCIObservation{Summary: string(domain.CIPassing), HeadSHA: headSHA},
Review: ports.SCMReviewObservation{Decision: string(domain.ReviewNone)},
Mergeability: mo,
}
}
// mergedSCMObservationBranches is mergedSCMObservation with caller-chosen
// branches so a stacked child (feat/x/auth -> feat/x) can be merged distinctly
// from the root (feat/x -> main).
func mergedSCMObservationBranches(prURL string, num int, headSHA, src, tgt string) ports.SCMObservation {
o := mergedSCMObservation(prURL, num, headSHA)
o.PR.SourceBranch = src
o.PR.TargetBranch = tgt
return o
}
// detectedPR is the open-PR-list discovery shape: the observer attributes a
// listed PR to a session by source-branch prefix, so only identity + branches
// matter here.
func detectedPR(prURL string, num int, src, tgt, headSHA string) ports.SCMPRObservation {
return ports.SCMPRObservation{URL: prURL, HTMLURL: prURL, Number: num, SourceBranch: src, HeadRepo: scmTestRepo.Repo, TargetBranch: tgt, HeadSHA: headSHA}
}
// TestSCMObserverMultiPREndToEnd is the functional regression guard for the
// multi-PR-per-session feature. It drives the real store + lifecycle + observer
// through the three behaviours the feature adds on top of the single-PR lane:
// branch-prefix attribution of several PRs to one session, the "all PRs
// merged/closed and at least one merged" completion bar, and the stacked-child
// merge-conflict nudge suppression. The SCM provider is canned (its own httptest
// coverage lives in observe/scm), so every other layer runs for real.
func TestSCMObserverMultiPREndToEnd(t *testing.T) {
t.Run("one session owns its root and stacked child PRs from a single repo list", func(t *testing.T) {
ctx := context.Background()
f := newSCMFixture(t, "feat/x")
const (
rootURL = "https://github.com/octocat/hello/pull/101"
childURL = "https://github.com/octocat/hello/pull/102"
)
// Root PR on the session branch, plus a stacked child whose source branch
// descends from it (feat/x/auth). matchSession claims both for the one
// session: the child by the "branch/..." stacking convention.
f.provider.detected["feat/x"] = detectedPR(rootURL, 101, "feat/x", "main", "sha-root")
f.provider.detected["feat/x/auth"] = detectedPR(childURL, 102, "feat/x/auth", "feat/x", "sha-child")
f.provider.observations[101] = openSCMObservation(rootURL, 101, "sha-root", "feat/x", "main", domain.MergeMergeable)
f.provider.observations[102] = openSCMObservation(childURL, 102, "sha-child", "feat/x/auth", "feat/x", domain.MergeBlocked)
if err := f.observer.Poll(ctx); err != nil {
t.Fatalf("Poll: %v", err)
}
prs, err := f.store.ListPRsBySession(ctx, f.session.ID)
if err != nil {
t.Fatalf("ListPRsBySession: %v", err)
}
if len(prs) != 2 {
t.Fatalf("one session should own both discovered PRs, got %d: %+v", len(prs), prs)
}
byURL := map[string]domain.PullRequest{}
for _, pr := range prs {
if pr.SessionID != f.session.ID {
t.Fatalf("PR %q attributed to %q, want %q", pr.URL, pr.SessionID, f.session.ID)
}
byURL[pr.URL] = pr
}
// The branch pair is what the stack model is derived from, so it must be
// persisted by the observer write path (not just discovered).
if byURL[rootURL].SourceBranch != "feat/x" || byURL[rootURL].TargetBranch != "main" {
t.Fatalf("root branch pair lost: %+v", byURL[rootURL])
}
if byURL[childURL].SourceBranch != "feat/x/auth" || byURL[childURL].TargetBranch != "feat/x" {
t.Fatalf("child branch pair lost: %+v", byURL[childURL])
}
if got := f.spy.count(); got != 0 {
t.Fatalf("clean PRs must not nudge, got %d: %+v", got, f.spy.snapshot())
}
})
t.Run("session stays alive while a stacked PR is open and terminates once all are merged", func(t *testing.T) {
ctx := context.Background()
f := newSCMFixture(t, "feat/x")
const (
rootURL = "https://github.com/octocat/hello/pull/201"
childURL = "https://github.com/octocat/hello/pull/202"
)
f.provider.detected["feat/x"] = detectedPR(rootURL, 201, "feat/x", "main", "sha-root")
f.provider.detected["feat/x/auth"] = detectedPR(childURL, 202, "feat/x/auth", "feat/x", "sha-child")
f.provider.observations[201] = openSCMObservation(rootURL, 201, "sha-root", "feat/x", "main", domain.MergeMergeable)
f.provider.observations[202] = openSCMObservation(childURL, 202, "sha-child", "feat/x/auth", "feat/x", domain.MergeBlocked)
// Poll 1: both PRs open and tracked. The session is live.
if err := f.observer.Poll(ctx); err != nil {
t.Fatalf("Poll 1: %v", err)
}
if rec, _, _ := f.store.GetSession(ctx, f.session.ID); rec.IsTerminated {
t.Fatal("session terminated with two open PRs")
}
// Poll 2: the root merges while the child stays open. One merged PR does
// not satisfy the completion bar while another PR is still open.
f.provider.observations[201] = mergedSCMObservationBranches(rootURL, 201, "sha-root", "feat/x", "main")
if err := f.observer.Poll(ctx); err != nil {
t.Fatalf("Poll 2: %v", err)
}
rootPR, ok, err := f.store.GetPR(ctx, rootURL)
if err != nil || !ok {
t.Fatalf("GetPR root: ok=%v err=%v", ok, err)
}
if !rootPR.Merged {
t.Fatalf("root PR should be persisted merged: %+v", rootPR)
}
if rec, _, _ := f.store.GetSession(ctx, f.session.ID); rec.IsTerminated {
t.Fatal("session terminated while the stacked child PR is still open")
}
// Poll 3: the child merges too. Now every PR is merged/closed and at least
// one merged, so the session completes and terminates.
f.provider.observations[202] = mergedSCMObservationBranches(childURL, 202, "sha-child", "feat/x/auth", "feat/x")
if err := f.observer.Poll(ctx); err != nil {
t.Fatalf("Poll 3: %v", err)
}
rec, ok, err := f.store.GetSession(ctx, f.session.ID)
if err != nil || !ok {
t.Fatalf("GetSession: ok=%v err=%v", ok, err)
}
if !rec.IsTerminated {
t.Fatalf("session should terminate once all PRs are merged: %+v", rec)
}
if got := f.spy.count(); got != 0 {
t.Fatalf("merge-driven completion must not nudge, got %d: %+v", got, f.spy.snapshot())
}
})
t.Run("stacked child blocked by an open parent is exempt from the rebase nudge", func(t *testing.T) {
ctx := context.Background()
f := newSCMFixture(t, "feat/x")
const (
rootURL = "https://github.com/octocat/hello/pull/301"
childURL = "https://github.com/octocat/hello/pull/302"
)
f.provider.detected["feat/x"] = detectedPR(rootURL, 301, "feat/x", "main", "sha-root")
f.provider.detected["feat/x/auth"] = detectedPR(childURL, 302, "feat/x/auth", "feat/x", "sha-child")
// Poll 1 establishes both rows (open, mergeable) so the stack relationship
// is durable before conflicts appear, making the poll-2 reaction order
// independent of map iteration.
f.provider.observations[301] = openSCMObservation(rootURL, 301, "sha-root", "feat/x", "main", domain.MergeMergeable)
f.provider.observations[302] = openSCMObservation(childURL, 302, "sha-child", "feat/x/auth", "feat/x", domain.MergeMergeable)
if err := f.observer.Poll(ctx); err != nil {
t.Fatalf("Poll 1: %v", err)
}
if got := f.spy.count(); got != 0 {
t.Fatalf("clean establishing poll must not nudge, got %d: %+v", got, f.spy.snapshot())
}
// Poll 2: both PRs now report merge conflicts. Only the bottom of the
// stack (the root, targeting main) is eligible for the rebase nudge; the
// child targets feat/x, the still-open root's source branch, so it is
// expected to conflict against its parent until the parent merges and is
// suppressed.
f.provider.observations[301] = openSCMObservation(rootURL, 301, "sha-root", "feat/x", "main", domain.MergeConflicting)
f.provider.observations[302] = openSCMObservation(childURL, 302, "sha-child", "feat/x/auth", "feat/x", domain.MergeConflicting)
if err := f.observer.Poll(ctx); err != nil {
t.Fatalf("Poll 2: %v", err)
}
msgs := f.spy.snapshot()
if len(msgs) != 1 {
t.Fatalf("exactly one PR (the stack bottom) should be nudged, got %d: %+v", len(msgs), msgs)
}
if msgs[0].session != f.session.ID {
t.Fatalf("nudge addressed to %q, want %q", msgs[0].session, f.session.ID)
}
if !strings.Contains(msgs[0].body, "merge conflicts") {
t.Fatalf("nudge body missing merge-conflict cue: %q", msgs[0].body)
}
// The persisted dedup signature must be the root's, never the child's —
// proving the child was suppressed at the reaction layer, not merely
// deduped after sending.
rootSig, err := f.store.GetPRLastNudgeSignature(ctx, rootURL)
if err != nil {
t.Fatalf("GetPRLastNudgeSignature root: %v", err)
}
if rootSig == "" {
t.Fatal("root PR should have a persisted nudge signature")
}
childSig, err := f.store.GetPRLastNudgeSignature(ctx, childURL)
if err != nil {
t.Fatalf("GetPRLastNudgeSignature child: %v", err)
}
if childSig != "" {
t.Fatalf("stacked child must not record a nudge signature: %q", childSig)
}
})
}

View File

@ -18,6 +18,11 @@ import (
type sessionStore interface { type sessionStore interface {
GetSession(ctx context.Context, id domain.SessionID) (domain.SessionRecord, bool, error) GetSession(ctx context.Context, id domain.SessionID) (domain.SessionRecord, bool, error)
UpdateSession(ctx context.Context, rec domain.SessionRecord) error UpdateSession(ctx context.Context, rec domain.SessionRecord) error
// ListPRsBySession returns every PR row tracked for the session. The
// reducer reads it to apply the multi-PR completion rule (terminate only
// when no open PR remains and at least one merged) and to suppress
// merge-conflict nudges on PRs stacked behind an open parent.
ListPRsBySession(ctx context.Context, id domain.SessionID) ([]domain.PullRequest, error)
// GetPRLastNudgeSignature / UpdatePRLastNudgeSignature persist the // GetPRLastNudgeSignature / UpdatePRLastNudgeSignature persist the
// reaction-dedup map so nudges survive a daemon restart. // reaction-dedup map so nudges survive a daemon restart.
GetPRLastNudgeSignature(ctx context.Context, prURL string) (string, error) GetPRLastNudgeSignature(ctx context.Context, prURL string) (string, error)

View File

@ -15,6 +15,7 @@ var ctx = context.Background()
type fakeStore struct { type fakeStore struct {
sessions map[domain.SessionID]domain.SessionRecord sessions map[domain.SessionID]domain.SessionRecord
prs map[domain.SessionID][]domain.PullRequest
signatures map[string]string signatures map[string]string
signatureWriteErr error signatureWriteErr error
@ -22,7 +23,7 @@ type fakeStore struct {
} }
func newFakeStore() *fakeStore { func newFakeStore() *fakeStore {
return &fakeStore{sessions: map[domain.SessionID]domain.SessionRecord{}, signatures: map[string]string{}} return &fakeStore{sessions: map[domain.SessionID]domain.SessionRecord{}, prs: map[domain.SessionID][]domain.PullRequest{}, signatures: map[string]string{}}
} }
func (f *fakeStore) GetSession(_ context.Context, id domain.SessionID) (domain.SessionRecord, bool, error) { func (f *fakeStore) GetSession(_ context.Context, id domain.SessionID) (domain.SessionRecord, bool, error) {
@ -30,6 +31,10 @@ func (f *fakeStore) GetSession(_ context.Context, id domain.SessionID) (domain.S
return r, ok, nil return r, ok, nil
} }
func (f *fakeStore) ListPRsBySession(_ context.Context, id domain.SessionID) ([]domain.PullRequest, error) {
return f.prs[id], nil
}
func (f *fakeStore) UpdateSession(_ context.Context, rec domain.SessionRecord) error { func (f *fakeStore) UpdateSession(_ context.Context, rec domain.SessionRecord) error {
f.sessions[rec.ID] = rec f.sessions[rec.ID] = rec
return nil return nil
@ -259,6 +264,7 @@ func TestPRObservation_MergeConflictNudgesAgent(t *testing.T) {
func TestPRObservation_MergedTerminatesWithoutNudge(t *testing.T) { func TestPRObservation_MergedTerminatesWithoutNudge(t *testing.T) {
m, st, msg := newManager() m, st, msg := newManager()
st.sessions["mer-1"] = working("mer-1") st.sessions["mer-1"] = working("mer-1")
st.prs["mer-1"] = []domain.PullRequest{{URL: "pr1", Merged: true}}
if err := m.ApplyPRObservation(ctx, "mer-1", ports.PRObservation{Fetched: true, URL: "pr1", Merged: true}); err != nil { if err := m.ApplyPRObservation(ctx, "mer-1", ports.PRObservation{Fetched: true, URL: "pr1", Merged: true}); err != nil {
t.Fatal(err) t.Fatal(err)
} }
@ -271,6 +277,91 @@ func TestPRObservation_MergedTerminatesWithoutNudge(t *testing.T) {
} }
} }
// A session with one merged PR and one still-open PR must NOT terminate: the
// completion bar is "no open PR remains AND at least one merged".
func TestPRObservation_MergedWithOpenSiblingDoesNotTerminate(t *testing.T) {
m, st, _ := newManager()
st.sessions["mer-1"] = working("mer-1")
st.prs["mer-1"] = []domain.PullRequest{
{URL: "pr1", Merged: true},
{URL: "pr2"},
}
if err := m.ApplyPRObservation(ctx, "mer-1", ports.PRObservation{Fetched: true, URL: "pr1", Merged: true}); err != nil {
t.Fatal(err)
}
if got := st.sessions["mer-1"]; got.IsTerminated {
t.Fatalf("session with an open sibling PR must stay alive, got %+v", got)
}
}
// Once the last open PR merges (all PRs now merged), the session terminates.
func TestPRObservation_LastMergeTerminatesSession(t *testing.T) {
m, st, _ := newManager()
st.sessions["mer-1"] = working("mer-1")
st.prs["mer-1"] = []domain.PullRequest{
{URL: "pr1", Merged: true},
{URL: "pr2", Merged: true},
}
if err := m.ApplyPRObservation(ctx, "mer-1", ports.PRObservation{Fetched: true, URL: "pr2", Merged: true}); err != nil {
t.Fatal(err)
}
if got := st.sessions["mer-1"]; !got.IsTerminated {
t.Fatalf("session should terminate once all PRs are merged, got %+v", got)
}
}
// A closed PR that leaves the session with an open sibling and no merge does not
// terminate; closing the last PR with no merge also does not terminate (nothing
// shipped).
func TestPRObservation_ClosedWithoutMergeDoesNotTerminate(t *testing.T) {
m, st, _ := newManager()
st.sessions["mer-1"] = working("mer-1")
st.prs["mer-1"] = []domain.PullRequest{{URL: "pr1", Closed: true}}
if err := m.ApplyPRObservation(ctx, "mer-1", ports.PRObservation{Fetched: true, URL: "pr1", Closed: true}); err != nil {
t.Fatal(err)
}
if got := st.sessions["mer-1"]; got.IsTerminated {
t.Fatalf("a closed-without-merge PR must not terminate the session, got %+v", got)
}
}
// A PR stacked on an open parent (its target branch is the parent's source
// branch) is exempt from the merge-conflict nudge: conflicts there are expected
// until the parent merges.
func TestPRObservation_StackedChildConflictSuppressed(t *testing.T) {
m, st, msg := newManager()
st.sessions["mer-1"] = working("mer-1")
st.prs["mer-1"] = []domain.PullRequest{
{URL: "parent", SourceBranch: "ao/x", TargetBranch: "main"},
{URL: "child", SourceBranch: "ao/x/auth", TargetBranch: "ao/x"},
}
o := ports.PRObservation{Fetched: true, URL: "child", Mergeability: domain.MergeConflicting}
if err := m.ApplyPRObservation(ctx, "mer-1", o); err != nil {
t.Fatal(err)
}
if len(msg.msgs) != 0 {
t.Fatalf("stacked child conflict should be suppressed, got %v", msg.msgs)
}
}
// The bottom-of-stack PR (not stacked on any open parent) still gets the
// merge-conflict nudge even when it has open stacked children.
func TestPRObservation_BottomOfStackConflictNudges(t *testing.T) {
m, st, msg := newManager()
st.sessions["mer-1"] = working("mer-1")
st.prs["mer-1"] = []domain.PullRequest{
{URL: "parent", SourceBranch: "ao/x", TargetBranch: "main"},
{URL: "child", SourceBranch: "ao/x/auth", TargetBranch: "ao/x"},
}
o := ports.PRObservation{Fetched: true, URL: "parent", Mergeability: domain.MergeConflicting}
if err := m.ApplyPRObservation(ctx, "mer-1", o); err != nil {
t.Fatal(err)
}
if len(msg.msgs) != 1 || !strings.Contains(msg.msgs[0], "merge conflicts") {
t.Fatalf("bottom-of-stack conflict should nudge, got %v", msg.msgs)
}
}
// TestPRObservation_DedupSurvivesManagerRestart simulates a daemon restart by // TestPRObservation_DedupSurvivesManagerRestart simulates a daemon restart by
// constructing a second Manager over the same store and asserts that an // constructing a second Manager over the same store and asserts that an
// identical PR observation does not re-fire the nudge — the dedup signature // identical PR observation does not re-fire the nudge — the dedup signature

View File

@ -43,10 +43,19 @@ func (m *Manager) ApplyPRObservation(ctx context.Context, id domain.SessionID, o
if !o.Fetched { if !o.Fetched {
return nil return nil
} }
if o.Merged { // A PR reaching a terminal state (merged or closed) no longer ends the
return m.MarkTerminated(ctx, id) // session on its own: a session may own several PRs. Terminate only when no
} // open PR remains and at least one of them merged. The observer persists the
if o.Closed { // PR row before calling lifecycle, so the store already reflects this
// transition when sessionComplete reads it.
if o.Merged || o.Closed {
done, err := m.sessionComplete(ctx, id)
if err != nil {
return err
}
if done {
return m.MarkTerminated(ctx, id)
}
return nil return nil
} }
rec, ok, err := m.store.GetSession(ctx, id) rec, ok, err := m.store.GetSession(ctx, id)
@ -79,11 +88,67 @@ func (m *Manager) ApplyPRObservation(ctx context.Context, id domain.SessionID, o
return m.sendOnce(ctx, id, o.URL, "review:"+o.URL, sig, msg, reviewMaxNudge) return m.sendOnce(ctx, id, o.URL, "review:"+o.URL, sig, msg, reviewMaxNudge)
} }
if o.Mergeability == domain.MergeConflicting { if o.Mergeability == domain.MergeConflicting {
// Only the bottom of a stack is eligible for the rebase nudge. A PR
// stacked on an open parent is expected to report conflicts against its
// parent branch until the parent merges and it retargets, so nudging the
// agent to rebase it now would be noise. Mergeability UNKNOWN (the brief
// post-retarget recompute window) never reaches here.
blocked, err := m.prBlockedByOpenParent(ctx, id, o.URL)
if err != nil {
return err
}
if blocked {
return nil
}
return m.sendOnce(ctx, id, o.URL, "merge-conflict:"+o.URL, string(o.Mergeability), "Your PR has merge conflicts. Rebase onto the base branch and resolve them.", 0) return m.sendOnce(ctx, id, o.URL, "merge-conflict:"+o.URL, string(o.Mergeability), "Your PR has merge conflicts. Rebase onto the base branch and resolve them.", 0)
} }
return nil return nil
} }
// sessionComplete reports whether the session has reached the multi-PR
// completion bar: at least one PR merged and no PR still open. A session with no
// PRs, or with any open PR, is not complete.
func (m *Manager) sessionComplete(ctx context.Context, id domain.SessionID) (bool, error) {
prs, err := m.store.ListPRsBySession(ctx, id)
if err != nil {
return false, err
}
merged := false
for _, pr := range prs {
if !pr.Merged && !pr.Closed {
return false, nil
}
if pr.Merged {
merged = true
}
}
return merged, nil
}
// prBlockedByOpenParent reports whether the PR at prURL is stacked on top of
// another still-open PR in the same session — i.e. its target branch is the
// source branch of a sibling open PR. Such a PR is not the bottom of its stack
// and is exempt from merge-conflict nudges. Branch facts are read from the
// store, which the observer has already updated for this observation.
func (m *Manager) prBlockedByOpenParent(ctx context.Context, id domain.SessionID, prURL string) (bool, error) {
prs, err := m.store.ListPRsBySession(ctx, id)
if err != nil {
return false, err
}
openSources := make(map[string]bool, len(prs))
for _, pr := range prs {
if !pr.Merged && !pr.Closed && pr.SourceBranch != "" {
openSources[pr.SourceBranch] = true
}
}
for _, pr := range prs {
if pr.URL == prURL {
return pr.TargetBranch != "" && openSources[pr.TargetBranch], nil
}
}
return false, nil
}
// ApplySCMObservation is the provider-neutral lifecycle entrypoint used by the // ApplySCMObservation is the provider-neutral lifecycle entrypoint used by the
// SCM observer. The existing reaction logic still operates on PRObservation, so // SCM observer. The existing reaction logic still operates on PRObservation, so
// lifecycle performs the compatibility projection internally instead of leaking // lifecycle performs the compatibility projection internally instead of leaking

View File

@ -38,7 +38,7 @@ const (
type Provider interface { type Provider interface {
ParseRepository(remote string) (ports.SCMRepo, bool) ParseRepository(remote string) (ports.SCMRepo, bool)
RepoPRListGuard(ctx context.Context, repo ports.SCMRepo, etag string) (ports.SCMGuardResult, error) RepoPRListGuard(ctx context.Context, repo ports.SCMRepo, etag string) (ports.SCMGuardResult, error)
DetectPRByBranch(ctx context.Context, repo ports.SCMRepo, branch string) (ports.SCMPRObservation, error) ListOpenPRsByRepo(ctx context.Context, repo ports.SCMRepo) ([]ports.SCMPRObservation, error)
CommitChecksGuard(ctx context.Context, repo ports.SCMRepo, headSHA, etag string) (ports.SCMGuardResult, error) CommitChecksGuard(ctx context.Context, repo ports.SCMRepo, headSHA, etag string) (ports.SCMGuardResult, error)
FetchPullRequests(ctx context.Context, refs []ports.SCMPRRef) ([]ports.SCMObservation, error) FetchPullRequests(ctx context.Context, refs []ports.SCMPRRef) ([]ports.SCMObservation, error)
FetchFailedCheckLogTail(ctx context.Context, repo ports.SCMRepo, check ports.SCMCheckObservation) (string, error) FetchFailedCheckLogTail(ctx context.Context, repo ports.SCMRepo, check ports.SCMCheckObservation) (string, error)
@ -191,6 +191,14 @@ type subject struct {
hasPR bool hasPR bool
} }
// sessionRepo pairs a live session with its parsed repo and branch for per-repo
// branch-prefix discovery of new (including stacked) pull requests.
type sessionRepo struct {
session domain.SessionRecord
repo ports.SCMRepo
branch string
}
type repoGuardState struct { type repoGuardState struct {
result ports.SCMGuardResult result ports.SCMGuardResult
hadETag bool hadETag bool
@ -226,11 +234,11 @@ func (o *Observer) Poll(ctx context.Context) error {
if o.disabled { if o.disabled {
return nil return nil
} }
subjects, err := o.discoverSubjects(ctx) subjects, sessionRepos, err := o.discoverSubjects(ctx)
if err != nil { if err != nil {
return err return err
} }
if len(subjects) == 0 { if len(sessionRepos) == 0 {
return nil return nil
} }
proceed, err := o.checkCredentials(ctx) proceed, err := o.checkCredentials(ctx)
@ -241,7 +249,7 @@ func (o *Observer) Poll(ctx context.Context) error {
return nil return nil
} }
repoGuards := o.guardRepos(ctx, subjects) repoGuards := o.guardRepos(ctx, sessionRepos)
repoRefreshOK := pendingRepoRefreshes(repoGuards) repoRefreshOK := pendingRepoRefreshes(repoGuards)
markRepoRefreshFailed := func(repo ports.SCMRepo) { markRepoRefreshFailed := func(repo ports.SCMRepo) {
key := prKey(repo, 0) key := prKey(repo, 0)
@ -252,7 +260,7 @@ func (o *Observer) Poll(ctx context.Context) error {
if err := ctx.Err(); err != nil { if err := ctx.Err(); err != nil {
return err return err
} }
o.detectMissingPRs(ctx, subjects, repoGuards, now, markRepoRefreshFailed) o.discoverNewPRs(ctx, sessionRepos, subjects, repoGuards, now, markRepoRefreshFailed)
if err := ctx.Err(); err != nil { if err := ctx.Err(); err != nil {
return err return err
} }
@ -405,13 +413,19 @@ func (o *Observer) checkCredentials(ctx context.Context) (bool, error) {
return observe.CheckCredentialsOnce(ctx, probe, &o.credentialsChecked, &o.disabled, o.logger, "scm observer") return observe.CheckCredentialsOnce(ctx, probe, &o.credentialsChecked, &o.disabled, o.logger, "scm observer")
} }
func (o *Observer) discoverSubjects(ctx context.Context) (map[string]*subject, error) { // discoverSubjects builds the per-PR refresh subjects (one per open tracked PR)
// and the per-session repo list used for branch-prefix discovery of new PRs. A
// session may own several PRs, so each open tracked PR becomes its own subject;
// merged/closed PRs are not re-fetched since lifecycle already saw the terminal
// transition and the completion rule reads them from the store.
func (o *Observer) discoverSubjects(ctx context.Context) (map[string]*subject, []sessionRepo, error) {
sessions, err := o.store.ListAllSessions(ctx) sessions, err := o.store.ListAllSessions(ctx)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
projects := map[domain.ProjectID]domain.ProjectRecord{} projects := map[domain.ProjectID]domain.ProjectRecord{}
out := map[string]*subject{} out := map[string]*subject{}
var sessionRepos []sessionRepo
for _, sess := range sessions { for _, sess := range sessions {
if sess.IsTerminated { if sess.IsTerminated {
continue continue
@ -424,7 +438,7 @@ func (o *Observer) discoverSubjects(ctx context.Context) (map[string]*subject, e
if !ok { if !ok {
p, found, err := o.store.GetProject(ctx, string(sess.ProjectID)) p, found, err := o.store.GetProject(ctx, string(sess.ProjectID))
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
if !found || !p.ArchivedAt.IsZero() { if !found || !p.ArchivedAt.IsZero() {
continue continue
@ -445,47 +459,37 @@ func (o *Observer) discoverSubjects(ctx context.Context) (map[string]*subject, e
o.logger.Debug("scm observer: project has no supported SCM origin", "project", proj.ID, "origin", proj.RepoOriginURL) o.logger.Debug("scm observer: project has no supported SCM origin", "project", proj.ID, "origin", proj.RepoOriginURL)
continue continue
} }
sessionRepos = append(sessionRepos, sessionRepo{session: sess, repo: repo, branch: branch})
prs, err := o.store.ListPRsBySession(ctx, sess.ID) prs, err := o.store.ListPRsBySession(ctx, sess.ID)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
known, hasPR := chooseKnownPR(prs) for _, pr := range openTrackedPRs(prs) {
s := &subject{session: sess, repo: repo, branch: branch, known: known, hasPR: hasPR} key := prKey(repo, pr.Number)
if hasPR && known.Number > 0 {
key := prKey(repo, known.Number)
if existing, ok := out[key]; ok { if existing, ok := out[key]; ok {
o.logger.Warn("scm observer: duplicate tracked PR ownership skipped", "pr", key, "kept_session", existing.session.ID, "skipped_session", sess.ID) o.logger.Warn("scm observer: duplicate tracked PR ownership skipped", "pr", key, "kept_session", existing.session.ID, "skipped_session", sess.ID)
continue continue
} }
out[key] = s out[key] = &subject{session: sess, repo: repo, branch: branch, known: pr, hasPR: true}
} else {
out["session:"+string(sess.ID)] = s
} }
} }
return out, nil return out, sessionRepos, nil
} }
func chooseKnownPR(prs []domain.PullRequest) (domain.PullRequest, bool) { func openTrackedPRs(prs []domain.PullRequest) []domain.PullRequest {
if len(prs) == 0 { out := make([]domain.PullRequest, 0, len(prs))
return domain.PullRequest{}, false
}
for _, pr := range prs { for _, pr := range prs {
if pr.Number > 0 && !pr.Merged && !pr.Closed { if pr.Number > 0 && !pr.Merged && !pr.Closed {
return pr, true out = append(out, pr)
} }
} }
for _, pr := range prs { return out
if pr.Number > 0 {
return pr, true
}
}
return domain.PullRequest{}, false
} }
func (o *Observer) guardRepos(ctx context.Context, subjects map[string]*subject) map[string]repoGuardState { func (o *Observer) guardRepos(ctx context.Context, sessionRepos []sessionRepo) map[string]repoGuardState {
repos := map[string]ports.SCMRepo{} repos := map[string]ports.SCMRepo{}
for _, s := range subjects { for _, sr := range sessionRepos {
repos[prKey(s.repo, 0)] = s.repo repos[prKey(sr.repo, 0)] = sr.repo
} }
out := map[string]repoGuardState{} out := map[string]repoGuardState{}
for key, repo := range repos { for key, repo := range repos {
@ -511,41 +515,117 @@ func pendingRepoRefreshes(guards map[string]repoGuardState) map[string]bool {
return out return out
} }
func (o *Observer) detectMissingPRs(ctx context.Context, subjects map[string]*subject, guards map[string]repoGuardState, now time.Time, markRepoFailed func(ports.SCMRepo)) { // discoverNewPRs lists each repo's open PRs once and attaches any not-yet-tracked
for oldKey, s := range subjects { // PR to the session that owns its source branch. A session owns a PR when the
if s.hasPR { // PR's source branch equals the session branch or descends from it (the
continue // "branch/..." stacking convention). One session may therefore pick up several
} // PRs (its root plus stacked children). Repos whose PR-list guard reports
g := guards[prKey(s.repo, 0)] // NotModified against a known ETag are skipped, since nothing new can have
// appeared since the last poll.
func (o *Observer) discoverNewPRs(ctx context.Context, sessionRepos []sessionRepo, subjects map[string]*subject, guards map[string]repoGuardState, now time.Time, markRepoFailed func(ports.SCMRepo)) {
byRepo := map[string][]sessionRepo{}
repos := map[string]ports.SCMRepo{}
for _, sr := range sessionRepos {
key := prKey(sr.repo, 0)
byRepo[key] = append(byRepo[key], sr)
repos[key] = sr.repo
}
for repoKey, repo := range repos {
g := guards[repoKey]
if g.err != nil { if g.err != nil {
continue continue
} }
if g.result.NotModified && g.hadETag { if g.result.NotModified && g.hadETag {
continue continue
} }
pr, err := o.provider.DetectPRByBranch(ctx, s.repo, s.branch) pulls, err := o.provider.ListOpenPRsByRepo(ctx, repo)
if err != nil { if err != nil {
o.logger.Debug("scm observer: no PR detected for branch", "session", s.session.ID, "branch", s.branch, "err", err) o.logger.Debug("scm observer: open PR list failed", "repo", repoFullName(repo), "err", err)
if markRepoFailed != nil && !errors.Is(err, ports.ErrSCMNotFound) { if markRepoFailed != nil && !errors.Is(err, ports.ErrSCMNotFound) {
markRepoFailed(s.repo) markRepoFailed(repo)
} }
continue continue
} }
if pr.Number <= 0 { for _, pr := range pulls {
continue if pr.Number <= 0 || pr.SourceBranch == "" {
continue
}
// Branch-prefix attribution must only claim PRs whose head branch
// lives in the project repo. A fork PR can carry a head branch whose
// name matches an AO session branch; its commits live in the fork, so
// auto-claiming it would misattribute work. Same-repo PRs always
// report the base repo's full name as their head repo, so anything
// else (including an empty head repo from a deleted fork) is skipped.
if !strings.EqualFold(pr.HeadRepo, repoFullName(repo)) {
continue
}
key := prKey(repo, pr.Number)
if _, ok := subjects[key]; ok {
continue
}
sr, ok := matchSession(byRepo[repoKey], pr.SourceBranch)
if !ok {
continue
}
known := domain.PullRequest{
URL: firstNonEmpty(pr.URL, pr.HTMLURL),
SessionID: sr.session.ID,
Number: pr.Number,
Draft: pr.Draft,
SourceBranch: pr.SourceBranch,
TargetBranch: pr.TargetBranch,
HeadSHA: pr.HeadSHA,
Provider: repo.Provider,
Host: repo.Host,
Repo: repoFullName(repo),
UpdatedAt: now,
}
// Persist the discovered PR as an open baseline row immediately, before
// the refresh/lifecycle pass runs. A session can own several PRs, and a
// terminal observation for one of them triggers a completion check that
// reads every PR of the session from the store. Without this write, an
// open sibling/child discovered in the same poll would not yet be
// durable, and the session could terminate while that PR is still open.
if err := o.store.WriteSCMObservation(ctx, known, nil, nil, nil, ports.ReviewWritePreserve); err != nil {
o.logger.Error("scm observer: persist discovered PR failed", "session", sr.session.ID, "pr", known.URL, "err", err)
if markRepoFailed != nil {
markRepoFailed(repo)
}
continue
}
subjects[key] = &subject{
session: sr.session,
repo: repo,
branch: sr.branch,
known: known,
hasPR: true,
}
} }
newKey := prKey(s.repo, pr.Number)
if existing, ok := subjects[newKey]; ok && existing != s {
o.logger.Warn("scm observer: detected PR is already tracked by another session", "pr", newKey, "kept_session", existing.session.ID, "skipped_session", s.session.ID)
continue
}
s.known = domain.PullRequest{URL: pr.URL, SessionID: s.session.ID, Number: pr.Number, SourceBranch: pr.SourceBranch, TargetBranch: pr.TargetBranch, HeadSHA: pr.HeadSHA, Provider: s.repo.Provider, Host: s.repo.Host, Repo: repoFullName(s.repo), UpdatedAt: now}
s.hasPR = true
delete(subjects, oldKey)
subjects[newKey] = s
} }
} }
// matchSession picks the session that owns sourceBranch. A session owns the
// branch when it is an exact match or a stacked descendant ("branch/..."). When
// several session branches are prefixes of the same source branch the longest
// (most specific) one wins, so a child session claims its own stacked PRs rather
// than the ancestor session.
func matchSession(candidates []sessionRepo, sourceBranch string) (sessionRepo, bool) {
var best sessionRepo
bestLen := -1
for _, sr := range candidates {
if sr.branch == "" {
continue
}
if sr.branch == sourceBranch || strings.HasPrefix(sourceBranch, sr.branch+"/") {
if len(sr.branch) > bestLen {
best = sr
bestLen = len(sr.branch)
}
}
}
return best, bestLen >= 0
}
func (o *Observer) selectRefreshCandidates(ctx context.Context, subjects map[string]*subject, guards map[string]repoGuardState, markRepoFailed func(ports.SCMRepo)) refreshSelection { func (o *Observer) selectRefreshCandidates(ctx context.Context, subjects map[string]*subject, guards map[string]repoGuardState, markRepoFailed func(ports.SCMRepo)) refreshSelection {
selection := refreshSelection{ selection := refreshSelection{
subjectsByPR: map[string]*subject{}, subjectsByPR: map[string]*subject{},

View File

@ -108,7 +108,8 @@ type fakeProvider struct {
mu sync.Mutex mu sync.Mutex
repoGuards map[string]ports.SCMGuardResult repoGuards map[string]ports.SCMGuardResult
checkGuards map[string]ports.SCMGuardResult checkGuards map[string]ports.SCMGuardResult
detected map[string]ports.SCMPRObservation openPRs map[string][]ports.SCMPRObservation
listErr error
observations map[string]ports.SCMObservation observations map[string]ports.SCMObservation
reviews map[string]ports.SCMReviewObservation reviews map[string]ports.SCMReviewObservation
logTails map[string]string logTails map[string]string
@ -120,7 +121,7 @@ type fakeProvider struct {
credentialErr error credentialErr error
credentialChecks int credentialChecks int
repoGuardCalls int repoGuardCalls int
detectCalls int listCalls int
fetchBatches [][]ports.SCMPRRef fetchBatches [][]ports.SCMPRRef
logCalls int logCalls int
reviewCalls int reviewCalls int
@ -145,15 +146,14 @@ func (p *fakeProvider) RepoPRListGuard(_ context.Context, repo ports.SCMRepo, _
p.repoGuardCalls++ p.repoGuardCalls++
return p.repoGuards[prKey(repo, 0)], nil return p.repoGuards[prKey(repo, 0)], nil
} }
func (p *fakeProvider) DetectPRByBranch(_ context.Context, _ ports.SCMRepo, branch string) (ports.SCMPRObservation, error) { func (p *fakeProvider) ListOpenPRsByRepo(_ context.Context, repo ports.SCMRepo) ([]ports.SCMPRObservation, error) {
p.mu.Lock() p.mu.Lock()
defer p.mu.Unlock() defer p.mu.Unlock()
p.detectCalls++ p.listCalls++
pr, ok := p.detected[branch] if p.listErr != nil {
if !ok { return nil, p.listErr
return ports.SCMPRObservation{}, ports.ErrSCMNotFound
} }
return pr, nil return p.openPRs[prKey(repo, 0)], nil
} }
func (p *fakeProvider) CommitChecksGuard(_ context.Context, repo ports.SCMRepo, sha, _ string) (ports.SCMGuardResult, error) { func (p *fakeProvider) CommitChecksGuard(_ context.Context, repo ports.SCMRepo, sha, _ string) (ports.SCMGuardResult, error) {
return p.checkGuards[commitKey(repo, sha)], nil return p.checkGuards[commitKey(repo, sha)], nil
@ -272,9 +272,9 @@ func TestPoll_DisablesOnceWhenCredentialsUnavailable(t *testing.T) {
if provider.credentialChecks != 1 { if provider.credentialChecks != 1 {
t.Fatalf("credential checks = %d, want one lazy check", provider.credentialChecks) t.Fatalf("credential checks = %d, want one lazy check", provider.credentialChecks)
} }
if provider.repoGuardCalls != 0 || provider.detectCalls != 0 || len(provider.fetchBatches) != 0 { if provider.repoGuardCalls != 0 || provider.listCalls != 0 || len(provider.fetchBatches) != 0 {
t.Fatalf("provider API calls should be skipped without credentials: guards=%d detects=%d batches=%d", t.Fatalf("provider API calls should be skipped without credentials: guards=%d lists=%d batches=%d",
provider.repoGuardCalls, provider.detectCalls, len(provider.fetchBatches)) provider.repoGuardCalls, provider.listCalls, len(provider.fetchBatches))
} }
} }
@ -389,13 +389,13 @@ func TestStart_LogsDisabledWarningWhenNoTokenAndNoSubjects(t *testing.T) {
if provider.credentialChecks != 1 { if provider.credentialChecks != 1 {
t.Fatalf("credential checks = %d, want exactly one pre-poll check", provider.credentialChecks) t.Fatalf("credential checks = %d, want exactly one pre-poll check", provider.credentialChecks)
} }
if provider.repoGuardCalls != 0 || provider.detectCalls != 0 || len(provider.fetchBatches) != 0 { if provider.repoGuardCalls != 0 || provider.listCalls != 0 || len(provider.fetchBatches) != 0 {
t.Fatalf("no provider API calls expected when disabled: guards=%d detects=%d batches=%d", t.Fatalf("no provider API calls expected when disabled: guards=%d lists=%d batches=%d",
provider.repoGuardCalls, provider.detectCalls, len(provider.fetchBatches)) provider.repoGuardCalls, provider.listCalls, len(provider.fetchBatches))
} }
} }
func TestPoll_RepoETag304SkipsDetectPR(t *testing.T) { func TestPoll_RepoETag304SkipsListPRs(t *testing.T) {
store := testStoreWithSession() store := testStoreWithSession()
provider := &fakeProvider{repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v1", NotModified: true}}, observations: map[string]ports.SCMObservation{}} provider := &fakeProvider{repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v1", NotModified: true}}, observations: map[string]ports.SCMObservation{}}
obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(1, 0).UTC()) obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(1, 0).UTC())
@ -403,16 +403,16 @@ func TestPoll_RepoETag304SkipsDetectPR(t *testing.T) {
if err := obs.Poll(context.Background()); err != nil { if err := obs.Poll(context.Background()); err != nil {
t.Fatal(err) t.Fatal(err)
} }
if provider.detectCalls != 0 { if provider.listCalls != 0 {
t.Fatalf("detectPR called on 304: %d", provider.detectCalls) t.Fatalf("ListOpenPRsByRepo called on 304: %d", provider.listCalls)
} }
} }
func TestPoll_DetectPRNotFoundCommitsRepoETag(t *testing.T) { func TestPoll_NoOpenPRsCommitsRepoETag(t *testing.T) {
store := testStoreWithSession() store := testStoreWithSession()
provider := &fakeProvider{ provider := &fakeProvider{
repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v2"}}, repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v2"}},
detected: map[string]ports.SCMPRObservation{}, openPRs: map[string][]ports.SCMPRObservation{},
observations: map[string]ports.SCMObservation{}, observations: map[string]ports.SCMObservation{},
} }
obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(1, 0).UTC()) obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(1, 0).UTC())
@ -420,22 +420,22 @@ func TestPoll_DetectPRNotFoundCommitsRepoETag(t *testing.T) {
if err := obs.Poll(context.Background()); err != nil { if err := obs.Poll(context.Background()); err != nil {
t.Fatal(err) t.Fatal(err)
} }
if provider.detectCalls != 1 { if provider.listCalls != 1 {
t.Fatalf("detectPR calls = %d, want 1", provider.detectCalls) t.Fatalf("ListOpenPRsByRepo calls = %d, want 1", provider.listCalls)
} }
if got := obs.Cache.RepoPRListETag[prKey(testRepo, 0)]; got != "v2" { if got := obs.Cache.RepoPRListETag[prKey(testRepo, 0)]; got != "v2" {
t.Fatalf("repo ETag after not-found detection = %q, want v2", got) t.Fatalf("repo ETag after empty listing = %q, want v2", got)
} }
if len(provider.fetchBatches) != 0 { if len(provider.fetchBatches) != 0 {
t.Fatalf("not-found branch should not fetch PR batch: %#v", provider.fetchBatches) t.Fatalf("empty listing should not fetch PR batch: %#v", provider.fetchBatches)
} }
} }
func TestPoll_RepoETag200DetectsPRAndRefreshesSamePoll(t *testing.T) { func TestPoll_RepoETag200DiscoversPRAndRefreshesSamePoll(t *testing.T) {
store := testStoreWithSession() store := testStoreWithSession()
provider := &fakeProvider{ provider := &fakeProvider{
repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v2"}}, repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v2"}},
detected: map[string]ports.SCMPRObservation{"feat": {URL: "https://github.com/o/r/pull/1", Number: 1, SourceBranch: "feat", TargetBranch: "main", HeadSHA: "sha1"}}, openPRs: map[string][]ports.SCMPRObservation{prKey(testRepo, 0): {{URL: "https://github.com/o/r/pull/1", Number: 1, SourceBranch: "feat", HeadRepo: "o/r", TargetBranch: "main", HeadSHA: "sha1"}}},
observations: map[string]ports.SCMObservation{prKey(testRepo, 1): testObs(1)}, observations: map[string]ports.SCMObservation{prKey(testRepo, 1): testObs(1)},
} }
lc := &fakeLifecycle{} lc := &fakeLifecycle{}
@ -443,8 +443,8 @@ func TestPoll_RepoETag200DetectsPRAndRefreshesSamePoll(t *testing.T) {
if err := obs.Poll(context.Background()); err != nil { if err := obs.Poll(context.Background()); err != nil {
t.Fatal(err) t.Fatal(err)
} }
if provider.detectCalls != 1 { if provider.listCalls != 1 {
t.Fatalf("detectPR calls = %d, want 1", provider.detectCalls) t.Fatalf("ListOpenPRsByRepo calls = %d, want 1", provider.listCalls)
} }
if len(provider.fetchBatches) != 1 || len(provider.fetchBatches[0]) != 1 || provider.fetchBatches[0][0].Number != 1 { if len(provider.fetchBatches) != 1 || len(provider.fetchBatches[0]) != 1 || provider.fetchBatches[0][0].Number != 1 {
t.Fatalf("new PR not refreshed in same poll: %#v", provider.fetchBatches) t.Fatalf("new PR not refreshed in same poll: %#v", provider.fetchBatches)
@ -454,6 +454,90 @@ func TestPoll_RepoETag200DetectsPRAndRefreshesSamePoll(t *testing.T) {
} }
} }
// A session whose branch is the prefix of two open PRs (its root plus a stacked
// child on branch "feat/child") picks up both PRs in a single poll.
func TestPoll_DiscoversStackedChildByBranchPrefix(t *testing.T) {
store := testStoreWithSession()
childObs := testObs(2)
childObs.PR.SourceBranch = "feat/child"
childObs.PR.TargetBranch = "feat"
provider := &fakeProvider{
repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v2"}},
openPRs: map[string][]ports.SCMPRObservation{prKey(testRepo, 0): {
{URL: "https://github.com/o/r/pull/1", Number: 1, SourceBranch: "feat", HeadRepo: "o/r", TargetBranch: "main", HeadSHA: "sha1"},
{URL: "https://github.com/o/r/pull/2", Number: 2, SourceBranch: "feat/child", HeadRepo: "o/r", TargetBranch: "feat", HeadSHA: "sha2"},
}},
observations: map[string]ports.SCMObservation{prKey(testRepo, 1): testObs(1), prKey(testRepo, 2): childObs},
}
lc := &fakeLifecycle{}
obs := newTestObserver(store, provider, lc, time.Unix(1, 0).UTC())
if err := obs.Poll(context.Background()); err != nil {
t.Fatal(err)
}
fetched := map[int]bool{}
for _, batch := range provider.fetchBatches {
for _, ref := range batch {
fetched[ref.Number] = true
}
}
if !fetched[1] || !fetched[2] {
t.Fatalf("expected both root and stacked child fetched, got %#v", fetched)
}
}
// A PR whose head branch matches a session branch but lives in a fork (its head
// repo differs from the project repo) must not be auto-attributed: its commits
// are not the session's work. It is neither fetched nor persisted.
func TestPoll_IgnoresForkPRWithMatchingBranch(t *testing.T) {
store := testStoreWithSession()
provider := &fakeProvider{
repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v2"}},
openPRs: map[string][]ports.SCMPRObservation{prKey(testRepo, 0): {{URL: "https://github.com/forker/r/pull/1", Number: 1, SourceBranch: "feat", HeadRepo: "forker/r", TargetBranch: "main", HeadSHA: "sha1"}}},
observations: map[string]ports.SCMObservation{prKey(testRepo, 1): testObs(1)},
}
obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(1, 0).UTC())
if err := obs.Poll(context.Background()); err != nil {
t.Fatal(err)
}
if len(provider.fetchBatches) != 0 {
t.Fatalf("fork PR must not be fetched, got %#v", provider.fetchBatches)
}
if len(store.writes) != 0 {
t.Fatalf("fork PR must not be persisted, got %d writes", len(store.writes))
}
}
// A newly discovered open PR is persisted as a baseline row during discovery,
// before the refresh/lifecycle pass. This is what lets a same-poll terminal
// observation for a sibling PR see the open PR in the store and avoid completing
// the session prematurely. The persist holds even when the refresh fetch yields
// no observation for the new PR.
func TestPoll_DiscoveredPRPersistedAsBaselineBeforeRefresh(t *testing.T) {
store := testStoreWithSession()
provider := &fakeProvider{
repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "v2"}},
openPRs: map[string][]ports.SCMPRObservation{prKey(testRepo, 0): {{URL: "https://github.com/o/r/pull/1", Number: 1, SourceBranch: "feat", HeadRepo: "o/r", TargetBranch: "main", HeadSHA: "sha1"}}},
observations: map[string]ports.SCMObservation{}, // refresh returns nothing
}
obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(1, 0).UTC())
if err := obs.Poll(context.Background()); err != nil {
t.Fatal(err)
}
var baseline *domain.PullRequest
for i := range store.writes {
if store.writes[i].pr.Number == 1 {
baseline = &store.writes[i].pr
break
}
}
if baseline == nil {
t.Fatalf("discovered PR #1 not persisted as a baseline row; writes=%#v", store.writes)
}
if baseline.Merged || baseline.Closed {
t.Fatalf("baseline row must be open, got merged=%v closed=%v", baseline.Merged, baseline.Closed)
}
}
func TestPoll_CIETagChangeRefreshesWhenRepoUnchanged(t *testing.T) { func TestPoll_CIETagChangeRefreshesWhenRepoUnchanged(t *testing.T) {
store := testStoreWithSession() store := testStoreWithSession()
store.prs["p-1"] = []domain.PullRequest{knownPR(1)} store.prs["p-1"] = []domain.PullRequest{knownPR(1)}
@ -950,7 +1034,7 @@ func TestDiscoverSubjects_BackfillsRepoOriginURL(t *testing.T) {
provider := &fakeProvider{} provider := &fakeProvider{}
obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(0, 0).UTC()) obs := newTestObserver(store, provider, &fakeLifecycle{}, time.Unix(0, 0).UTC())
if _, err := obs.discoverSubjects(context.Background()); err != nil { if _, _, err := obs.discoverSubjects(context.Background()); err != nil {
t.Fatalf("discoverSubjects: %v", err) t.Fatalf("discoverSubjects: %v", err)
} }
if got := store.projects["p"].RepoOriginURL; got != "https://github.com/o/r.git" { if got := store.projects["p"].RepoOriginURL; got != "https://github.com/o/r.git" {
@ -970,12 +1054,12 @@ func TestDiscoverSubjects_NonGitPathDoesNotBackfill(t *testing.T) {
checks: map[string][]domain.PullRequestCheck{}, checks: map[string][]domain.PullRequestCheck{},
} }
obs := newTestObserver(store, &fakeProvider{}, &fakeLifecycle{}, time.Unix(0, 0).UTC()) obs := newTestObserver(store, &fakeProvider{}, &fakeLifecycle{}, time.Unix(0, 0).UTC())
subjects, err := obs.discoverSubjects(context.Background()) subjects, sessionRepos, err := obs.discoverSubjects(context.Background())
if err != nil { if err != nil {
t.Fatalf("discoverSubjects: %v", err) t.Fatalf("discoverSubjects: %v", err)
} }
if len(subjects) != 0 { if len(subjects) != 0 || len(sessionRepos) != 0 {
t.Fatalf("non-git project should be skipped, got %d subjects", len(subjects)) t.Fatalf("non-git project should be skipped, got %d subjects %d sessionRepos", len(subjects), len(sessionRepos))
} }
if got := store.projects["p"].RepoOriginURL; got != "" { if got := store.projects["p"].RepoOriginURL; got != "" {
t.Fatalf("RepoOriginURL = %q, want empty (no persist on failed backfill)", got) t.Fatalf("RepoOriginURL = %q, want empty (no persist on failed backfill)", got)

View File

@ -104,6 +104,10 @@ type SCMPRObservation struct {
Closed bool Closed bool
// SourceBranch is the PR head/source branch name. // SourceBranch is the PR head/source branch name.
SourceBranch string SourceBranch string
// HeadRepo is the full name (owner/name) of the repository the PR head
// branch lives in. It matches the base repo for same-repo PRs and differs
// for PRs opened from a fork, so branch-prefix attribution can ignore forks.
HeadRepo string
// TargetBranch is the PR base/target branch name. // TargetBranch is the PR base/target branch name.
TargetBranch string TargetBranch string
// HeadSHA is the current head commit SHA for the PR. // HeadSHA is the current head commit SHA for the PR.

View File

@ -20,6 +20,7 @@ type Store interface {
ListAllSessions(ctx context.Context) ([]domain.SessionRecord, error) ListAllSessions(ctx context.Context) ([]domain.SessionRecord, error)
RenameSession(ctx context.Context, id domain.SessionID, displayName string, updatedAt time.Time) (bool, error) RenameSession(ctx context.Context, id domain.SessionID, displayName string, updatedAt time.Time) (bool, error)
GetDisplayPRFactsForSession(ctx context.Context, id domain.SessionID) (domain.PRFacts, bool, error) GetDisplayPRFactsForSession(ctx context.Context, id domain.SessionID) (domain.PRFacts, bool, error)
ListPRFactsForSession(ctx context.Context, id domain.SessionID) ([]domain.PRFacts, error)
ListPRsBySession(ctx context.Context, sessionID domain.SessionID) ([]domain.PullRequest, error) ListPRsBySession(ctx context.Context, sessionID domain.SessionID) ([]domain.PullRequest, error)
ListPRComments(ctx context.Context, prURL string) ([]domain.PullRequestComment, error) ListPRComments(ctx context.Context, prURL string) ([]domain.PullRequestComment, error)
GetProject(ctx context.Context, id string) (domain.ProjectRecord, bool, error) GetProject(ctx context.Context, id string) (domain.ProjectRecord, bool, error)
@ -354,14 +355,11 @@ func toAPIError(err error) error {
} }
func (s *Service) toSession(ctx context.Context, rec domain.SessionRecord) (domain.Session, error) { func (s *Service) toSession(ctx context.Context, rec domain.SessionRecord) (domain.Session, error) {
pr, ok, err := s.store.GetDisplayPRFactsForSession(ctx, rec.ID) prs, err := s.store.ListPRFactsForSession(ctx, rec.ID)
if err != nil { if err != nil {
return domain.Session{}, fmt.Errorf("pr facts %s: %w", rec.ID, err) return domain.Session{}, fmt.Errorf("pr facts %s: %w", rec.ID, err)
} }
if !ok { return domain.Session{SessionRecord: rec, Status: deriveStatus(rec, prs, s.now(), s.harnessSignals(rec.Harness)), TerminalHandleID: rec.Metadata.RuntimeHandleID, PRs: prs}, nil
return domain.Session{SessionRecord: rec, Status: deriveStatus(rec, nil, s.now(), s.harnessSignals(rec.Harness)), TerminalHandleID: rec.Metadata.RuntimeHandleID}, nil
}
return domain.Session{SessionRecord: rec, Status: deriveStatus(rec, &pr, s.now(), s.harnessSignals(rec.Harness)), TerminalHandleID: rec.Metadata.RuntimeHandleID}, nil
} }
// now tolerates a zero-value Service (tests construct the struct literally // now tolerates a zero-value Service (tests construct the struct literally

View File

@ -78,6 +78,14 @@ func (f *fakeStore) ListPRsBySession(_ context.Context, id domain.SessionID) ([]
return []domain.PullRequest{{URL: pr.URL, SessionID: id, Number: pr.Number, Draft: pr.Draft, Merged: pr.Merged, Closed: pr.Closed, CI: pr.CI, Review: pr.Review, Mergeability: pr.Mergeability, UpdatedAt: pr.UpdatedAt}}, nil return []domain.PullRequest{{URL: pr.URL, SessionID: id, Number: pr.Number, Draft: pr.Draft, Merged: pr.Merged, Closed: pr.Closed, CI: pr.CI, Review: pr.Review, Mergeability: pr.Mergeability, UpdatedAt: pr.UpdatedAt}}, nil
} }
func (f *fakeStore) ListPRFactsForSession(_ context.Context, id domain.SessionID) ([]domain.PRFacts, error) {
pr, ok := f.pr[id]
if !ok {
return nil, nil
}
return []domain.PRFacts{pr}, nil
}
func (f *fakeStore) ListPRComments(context.Context, string) ([]domain.PullRequestComment, error) { func (f *fakeStore) ListPRComments(context.Context, string) ([]domain.PullRequestComment, error) {
return nil, nil return nil, nil
} }

View File

@ -0,0 +1,34 @@
package session
import "github.com/aoagents/agent-orchestrator/backend/internal/domain"
// stackInfo is the derived position of one PR within its session's set of PRs.
// PRs form a stack when one targets the source branch of another: PR B is a
// child of PR A when B.TargetBranch == A.SourceBranch and A is open.
type stackInfo struct {
// Blocked is true when an open PR in the set owns the branch this PR targets,
// i.e. this PR is a child stacked on a parent that has not merged yet.
Blocked bool
// BottomOfStack is true when no open PR sits below this one. It is the only
// PR in a stack that should receive a merge-conflict rebase nudge; an
// independent PR (targeting the base branch) is its own bottom.
BottomOfStack bool
}
// buildStacks derives the stack position of every PR from the source/target
// branch columns alone. A parent counts only while open, matching the rule that
// a merged or closed parent no longer blocks its children.
func buildStacks(prs []domain.PRFacts) map[string]stackInfo {
openSources := make(map[string]bool, len(prs))
for _, p := range prs {
if !p.Merged && !p.Closed && p.SourceBranch != "" {
openSources[p.SourceBranch] = true
}
}
out := make(map[string]stackInfo, len(prs))
for _, p := range prs {
blocked := p.TargetBranch != "" && openSources[p.TargetBranch]
out[p.URL] = stackInfo{Blocked: blocked, BottomOfStack: !blocked}
}
return out
}

View File

@ -0,0 +1,127 @@
package session
import (
"testing"
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
)
// live builds an idle, non-terminated session that has already signaled, so the
// derived status is governed purely by its PRs.
func live() domain.SessionRecord {
return statusRec(domain.ActivityIdle, false)
}
func TestBuildStacksMarksBlockedChildren(t *testing.T) {
// #142 (root → main), #143 stacked on #142, #144 stacked on #143.
prs := []domain.PRFacts{
{URL: "p142", SourceBranch: "ao/abc", TargetBranch: "main"},
{URL: "p143", SourceBranch: "ao/abc/auth", TargetBranch: "ao/abc"},
{URL: "p144", SourceBranch: "ao/abc/tests", TargetBranch: "ao/abc/auth"},
}
st := buildStacks(prs)
if st["p142"].Blocked || !st["p142"].BottomOfStack {
t.Fatalf("root should be unblocked bottom-of-stack, got %+v", st["p142"])
}
if !st["p143"].Blocked || st["p143"].BottomOfStack {
t.Fatalf("middle should be blocked, got %+v", st["p143"])
}
if !st["p144"].Blocked {
t.Fatalf("top should be blocked, got %+v", st["p144"])
}
}
func TestBuildStacksMergedParentUnblocksChild(t *testing.T) {
prs := []domain.PRFacts{
{URL: "p142", SourceBranch: "ao/abc", TargetBranch: "main", Merged: true},
{URL: "p143", SourceBranch: "ao/abc/auth", TargetBranch: "ao/abc"},
}
st := buildStacks(prs)
if st["p143"].Blocked {
t.Fatal("child should be unblocked once parent is merged")
}
}
func TestDeriveStatusWorstWinsAcrossIndependentPRs(t *testing.T) {
// Two independent open PRs (both target main): mergeable vs ci_failed.
// CI failure is more urgent, so the session reports ci_failed.
prs := []domain.PRFacts{
{URL: "a", SourceBranch: "ao/a", TargetBranch: "main", Mergeability: domain.MergeMergeable},
{URL: "b", SourceBranch: "ao/b", TargetBranch: "main", CI: domain.CIFailing},
}
if got := deriveStatus(live(), prs, statusNow, true); got != domain.StatusCIFailed {
t.Fatalf("got %q want ci_failed", got)
}
}
func TestDeriveStatusAllMergeableReportsMergeable(t *testing.T) {
prs := []domain.PRFacts{
{URL: "a", SourceBranch: "ao/a", TargetBranch: "main", Mergeability: domain.MergeMergeable},
{URL: "b", SourceBranch: "ao/b", TargetBranch: "main", Mergeability: domain.MergeMergeable},
}
if got := deriveStatus(live(), prs, statusNow, true); got != domain.StatusMergeable {
t.Fatalf("got %q want mergeable", got)
}
}
func TestDeriveStatusStackedChildExemptFromAggregation(t *testing.T) {
// Root mergeable; blocked child is pr_open. Child is exempt, so the session
// reports mergeable rather than being dragged down to pr_open.
prs := []domain.PRFacts{
{URL: "root", SourceBranch: "ao/abc", TargetBranch: "main", Mergeability: domain.MergeMergeable},
{URL: "child", SourceBranch: "ao/abc/x", TargetBranch: "ao/abc"},
}
if got := deriveStatus(live(), prs, statusNow, true); got != domain.StatusMergeable {
t.Fatalf("got %q want mergeable (child exempt)", got)
}
}
func TestDeriveStatusMergedParentOpenChildStaysOnChild(t *testing.T) {
// Parent merged, child now unblocked and review_pending: still alive, status
// follows the open child.
prs := []domain.PRFacts{
{URL: "root", SourceBranch: "ao/abc", TargetBranch: "main", Merged: true},
{URL: "child", SourceBranch: "ao/abc/x", TargetBranch: "main", Review: domain.ReviewRequired},
}
if got := deriveStatus(live(), prs, statusNow, true); got != domain.StatusReviewPending {
t.Fatalf("got %q want review_pending", got)
}
}
func TestDeriveStatusAllMergedReportsMerged(t *testing.T) {
prs := []domain.PRFacts{
{URL: "a", Merged: true},
{URL: "b", Merged: true},
}
if got := deriveStatus(live(), prs, statusNow, true); got != domain.StatusMerged {
t.Fatalf("got %q want merged", got)
}
}
func TestDeriveStatusAllClosedNoneMergedFallsToActivity(t *testing.T) {
prs := []domain.PRFacts{
{URL: "a", Closed: true},
{URL: "b", Closed: true},
}
if got := deriveStatus(statusRec(domain.ActivityActive, false), prs, statusNow, true); got != domain.StatusWorking {
t.Fatalf("got %q want working", got)
}
}
func TestDeriveStatusEmptyPRsUsesActivity(t *testing.T) {
if got := deriveStatus(statusRec(domain.ActivityActive, false), nil, statusNow, true); got != domain.StatusWorking {
t.Fatalf("got %q want working", got)
}
}
func TestDeriveStatusDegenerateAllBlockedStillAggregates(t *testing.T) {
// Two PRs each targeting the other's source branch (no visible root). The
// fallback aggregates across all so the session never goes dark.
prs := []domain.PRFacts{
{URL: "a", SourceBranch: "x", TargetBranch: "y", CI: domain.CIFailing},
{URL: "b", SourceBranch: "y", TargetBranch: "x", Mergeability: domain.MergeMergeable},
}
if got := deriveStatus(live(), prs, statusNow, true); got != domain.StatusCIFailed {
t.Fatalf("got %q want ci_failed (degenerate fallback)", got)
}
}

View File

@ -19,9 +19,14 @@ const noSignalGrace = 90 * time.Second
// session's harness has an activity hook pipeline at all; only then can // session's harness has an activity hook pipeline at all; only then can
// prolonged silence mean the pipeline is broken (no_signal) rather than the // prolonged silence mean the pipeline is broken (no_signal) rather than the
// permanent, normal silence of a hook-less harness. // permanent, normal silence of a hook-less harness.
func deriveStatus(rec domain.SessionRecord, pr *domain.PRFacts, now time.Time, signalCapable bool) domain.SessionStatus { //
// A session may own several PRs at once (independent or stacked). The PR-derived
// status is the worst-wins aggregate across its open PRs; stacked children whose
// parent is still open are exempt from the aggregation since they cannot merge
// until the parent does. Merged/closed PRs only matter once no open PR remains.
func deriveStatus(rec domain.SessionRecord, prs []domain.PRFacts, now time.Time, signalCapable bool) domain.SessionStatus {
if rec.IsTerminated { if rec.IsTerminated {
if pr != nil && pr.Merged { if anyMerged(prs) {
return domain.StatusMerged return domain.StatusMerged
} }
return domain.StatusTerminated return domain.StatusTerminated
@ -31,13 +36,12 @@ func deriveStatus(rec domain.SessionRecord, pr *domain.PRFacts, now time.Time, s
return domain.StatusNeedsInput return domain.StatusNeedsInput
} }
if pr != nil { open := openPRs(prs)
if pr.Merged { if len(open) > 0 {
return domain.StatusMerged return aggregatePRStatus(open)
} }
if !pr.Closed { if anyMerged(prs) {
return prPipelineStatus(*pr) return domain.StatusMerged
}
} }
if rec.Activity.State == domain.ActivityActive { if rec.Activity.State == domain.ActivityActive {
@ -53,6 +57,94 @@ func deriveStatus(rec domain.SessionRecord, pr *domain.PRFacts, now time.Time, s
return domain.StatusIdle return domain.StatusIdle
} }
// openPRs returns the PRs that are neither merged nor closed, preserving order.
func openPRs(prs []domain.PRFacts) []domain.PRFacts {
out := make([]domain.PRFacts, 0, len(prs))
for _, p := range prs {
if !p.Merged && !p.Closed {
out = append(out, p)
}
}
return out
}
func anyMerged(prs []domain.PRFacts) bool {
for _, p := range prs {
if p.Merged {
return true
}
}
return false
}
// aggregatePRStatus is the worst-wins reduction over a session's open PRs.
// A stacked child blocked by an open parent cannot merge yet, so its readiness
// signals (mergeable/approved/review-pending/open) are not actionable for the
// session and are suppressed. Its problem signals are still actionable: failing
// CI, draft state, and requested-changes/unresolved-comments must stay visible
// so a broken child is not hidden behind the stack. If no PR contributes any
// signal (a degenerate stack with no visible root), it falls back to aggregating
// the raw status across all open PRs so the session never goes dark.
func aggregatePRStatus(open []domain.PRFacts) domain.SessionStatus {
stacks := buildStacks(open)
candidates := make([]domain.SessionStatus, 0, len(open))
for _, p := range open {
s := prPipelineStatus(p)
if stacks[p.URL].Blocked && !isActionableChildSignal(s) {
continue
}
candidates = append(candidates, s)
}
if len(candidates) == 0 {
for _, p := range open {
candidates = append(candidates, prPipelineStatus(p))
}
}
worst := candidates[0]
for _, s := range candidates[1:] {
if statusSeverity(s) < statusSeverity(worst) {
worst = s
}
}
return worst
}
// isActionableChildSignal reports whether a blocked stacked child's pipeline
// status is a problem the user can act on now, independent of the child's
// inability to merge until its parent does.
func isActionableChildSignal(s domain.SessionStatus) bool {
switch s {
case domain.StatusCIFailed, domain.StatusDraft, domain.StatusChangesRequested:
return true
default:
return false
}
}
// statusSeverity ranks PR pipeline statuses from most to least urgent so the
// aggregate surfaces the PR that most needs attention. mergeable is least urgent
// so a session only reports mergeable when every aggregated PR is mergeable.
func statusSeverity(s domain.SessionStatus) int {
switch s {
case domain.StatusCIFailed:
return 0
case domain.StatusChangesRequested:
return 1
case domain.StatusDraft:
return 2
case domain.StatusReviewPending:
return 3
case domain.StatusPROpen:
return 4
case domain.StatusApproved:
return 5
case domain.StatusMergeable:
return 6
default:
return 7
}
}
func prPipelineStatus(pr domain.PRFacts) domain.SessionStatus { func prPipelineStatus(pr domain.PRFacts) domain.SessionStatus {
switch { switch {
case pr.CI == domain.CIFailing: case pr.CI == domain.CIFailing:

View File

@ -27,13 +27,13 @@ func silentRec(age time.Duration) domain.SessionRecord {
} }
} }
func statusPR(facts domain.PRFacts) *domain.PRFacts { return &facts } func statusPR(facts domain.PRFacts) []domain.PRFacts { return []domain.PRFacts{facts} }
func TestServiceDerivesStatusFromSessionFactsAndPR(t *testing.T) { func TestServiceDerivesStatusFromSessionFactsAndPR(t *testing.T) {
tests := []struct { tests := []struct {
name string name string
rec domain.SessionRecord rec domain.SessionRecord
pr *domain.PRFacts pr []domain.PRFacts
// hookless marks a harness with no activity pipeline (signalCapable // hookless marks a harness with no activity pipeline (signalCapable
// false): silence is its permanent normal state, never no_signal. // false): silence is its permanent normal state, never no_signal.
hookless bool hookless bool
@ -79,6 +79,50 @@ func TestServiceDerivesStatusFromSessionFactsAndPR(t *testing.T) {
} }
} }
// A blocked stacked child cannot merge until its parent does, so its readiness
// signals are suppressed, but its problem signals (failing CI, draft,
// requested-changes/unresolved-comments) must still surface for the session.
func TestAggregateStackedChildSignals(t *testing.T) {
parent := domain.PRFacts{URL: "parent", SourceBranch: "feat", Mergeability: domain.MergeMergeable}
child := func(f domain.PRFacts) domain.PRFacts {
f.URL = "child"
f.SourceBranch = "feat/child"
f.TargetBranch = "feat"
return f
}
tests := []struct {
name string
prs []domain.PRFacts
want domain.SessionStatus
}{
{"blocked-child-ci-failing-surfaces", []domain.PRFacts{parent, child(domain.PRFacts{CI: domain.CIFailing})}, domain.StatusCIFailed},
{"blocked-child-draft-surfaces", []domain.PRFacts{parent, child(domain.PRFacts{Draft: true})}, domain.StatusDraft},
{"blocked-child-changes-requested-surfaces", []domain.PRFacts{parent, child(domain.PRFacts{Review: domain.ReviewChangesRequest})}, domain.StatusChangesRequested},
{"blocked-child-unresolved-comments-surfaces", []domain.PRFacts{parent, child(domain.PRFacts{ReviewComments: true})}, domain.StatusChangesRequested},
// A blocked child's readiness signals stay hidden: only the parent's
// mergeable state drives the session.
{"blocked-child-mergeable-suppressed", []domain.PRFacts{parent, child(domain.PRFacts{Mergeability: domain.MergeMergeable})}, domain.StatusMergeable},
{"blocked-child-approved-suppressed", []domain.PRFacts{parent, child(domain.PRFacts{Review: domain.ReviewApproved})}, domain.StatusMergeable},
// Degenerate set where every open PR is blocked and none is actionable:
// fall back to the raw aggregate so the session never goes dark.
{
"all-blocked-no-actionable-falls-back",
[]domain.PRFacts{
{URL: "a", SourceBranch: "feat/a", TargetBranch: "feat/b", Mergeability: domain.MergeMergeable},
{URL: "b", SourceBranch: "feat/b", TargetBranch: "feat/a", Mergeability: domain.MergeMergeable},
},
domain.StatusMergeable,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := deriveStatus(statusRec(domain.ActivityIdle, false), tt.prs, statusNow, true); got != tt.want {
t.Fatalf("got %q want %q", got, tt.want)
}
})
}
}
// Without an injected capability predicate the service must never claim // Without an injected capability predicate the service must never claim
// no_signal; with one, capability follows the predicate per harness. // no_signal; with one, capability follows the predicate per harness.
func TestHarnessSignalsCapabilityGate(t *testing.T) { func TestHarnessSignalsCapabilityGate(t *testing.T) {

View File

@ -654,8 +654,9 @@ func (m *Manager) buildSystemPrompt(ctx context.Context, kind domain.SessionKind
return "", err return "", err
} }
if ok { if ok {
return workerOrchestratorPrompt(orchestratorID), nil return workerOrchestratorPrompt(orchestratorID) + "\n\n" + workerMultiPRPrompt(), nil
} }
return workerMultiPRPrompt(), nil
} }
return "", nil return "", nil
} }
@ -696,6 +697,23 @@ An active orchestrator session exists for this project. If you hit a true blocke
Only ping the orchestrator for true blockers, cross-session coordination, or decisions that cannot be resolved within your own task.`, orchestratorID) Only ping the orchestrator for true blockers, cross-session coordination, or decisions that cannot be resolved within your own task.`, orchestratorID)
} }
// workerMultiPRPrompt explains the branch convention AO uses to attribute pull
// requests to this session. A worker may open several PRs in one session: AO
// tracks every open PR whose source branch is the session's own branch or a
// descendant of it. Stacking a PR on top of another therefore only requires
// branching off with a `<session-branch>/<topic>` name; PRs on unrelated
// branches are attributed to whichever session owns their branch prefix.
func workerMultiPRPrompt() string {
return `## Pull requests for this session
You can open more than one pull request from this session. AO attributes a PR to you when its source branch is your session's working branch or a branch descended from it (a "/"-separated child like ` + "`your-branch/topic`" + `).
- For independent PRs, create each source branch as a child of your session branch (` + "`your-branch/<topic>`" + `) so it stays in this session's namespace, then open the PR targeting your base branch as usual. The PR can target the base branch; only the source branch needs to stay under your session branch for AO to track it.
- To stack a PR on top of another (so it merges after its parent), create the child branch from the parent branch and name it ` + "`<parent-branch>/<topic>`" + `, then target the parent branch in the PR. AO recognizes the stack from the branch relationship and will only nudge you to resolve conflicts on the bottom-most PR.
Keep branch names within your session's branch namespace so AO can track every PR you open.`
}
// spawnEnv builds the runtime environment: the per-project env vars first, then // spawnEnv builds the runtime environment: the per-project env vars first, then
// the AO-internal vars last so they always win (a project cannot override // the AO-internal vars last so they always win (a project cannot override
// AO_SESSION_ID and friends). // AO_SESSION_ID and friends).

View File

@ -181,6 +181,79 @@ func (q *Queries) GetPRLastNudgeSignature(ctx context.Context, url string) (stri
return last_nudge_signature, err return last_nudge_signature, err
} }
const listPRFactsBySession = `-- name: ListPRFactsBySession :many
SELECT
pr.url,
pr.number,
pr.pr_state,
pr.review_decision,
pr.ci_state,
pr.mergeability,
pr.source_branch,
pr.target_branch,
pr.updated_at,
EXISTS (
SELECT 1
FROM pr_comment
WHERE pr_comment.pr_url = pr.url
AND pr_comment.resolved = 0
AND pr_comment.is_bot = 0
) AS review_comments
FROM pr
WHERE pr.session_id = ?
ORDER BY pr.updated_at DESC
`
type ListPRFactsBySessionRow struct {
URL string
Number int64
PRState domain.PRState
ReviewDecision domain.ReviewDecision
CIState domain.CIState
Mergeability domain.Mergeability
SourceBranch string
TargetBranch string
UpdatedAt time.Time
ReviewComments bool
}
// All PR snapshots for a session (every state), with source/target branch for
// stack derivation and the unresolved-comment flag. The status aggregator
// filters open vs merged/closed in Go and derives stacks from the branches.
func (q *Queries) ListPRFactsBySession(ctx context.Context, sessionID domain.SessionID) ([]ListPRFactsBySessionRow, error) {
rows, err := q.db.QueryContext(ctx, listPRFactsBySession, sessionID)
if err != nil {
return nil, err
}
defer rows.Close()
items := []ListPRFactsBySessionRow{}
for rows.Next() {
var i ListPRFactsBySessionRow
if err := rows.Scan(
&i.URL,
&i.Number,
&i.PRState,
&i.ReviewDecision,
&i.CIState,
&i.Mergeability,
&i.SourceBranch,
&i.TargetBranch,
&i.UpdatedAt,
&i.ReviewComments,
); err != nil {
return nil, err
}
items = append(items, i)
}
if err := rows.Close(); err != nil {
return nil, err
}
if err := rows.Err(); err != nil {
return nil, err
}
return items, nil
}
const listPRsBySession = `-- name: ListPRsBySession :many const listPRsBySession = `-- name: ListPRsBySession :many
SELECT url, session_id, number, pr_state, review_decision, ci_state, mergeability, updated_at, provider, host, repo, source_branch, target_branch, head_sha, title, additions, deletions, changed_files, author, base_sha, merge_commit_sha, is_draft, is_merged, is_closed, provider_state, provider_mergeable, provider_merge_state_status, html_url, created_at_provider, updated_at_provider, merged_at_provider, closed_at_provider, metadata_hash, ci_hash, review_hash, observed_at, ci_observed_at, review_observed_at, last_nudge_signature FROM pr SELECT url, session_id, number, pr_state, review_decision, ci_state, mergeability, updated_at, provider, host, repo, source_branch, target_branch, head_sha, title, additions, deletions, changed_files, author, base_sha, merge_commit_sha, is_draft, is_merged, is_closed, provider_state, provider_mergeable, provider_merge_state_status, html_url, created_at_provider, updated_at_provider, merged_at_provider, closed_at_provider, metadata_hash, ci_hash, review_hash, observed_at, ci_observed_at, review_observed_at, last_nudge_signature FROM pr
WHERE session_id = ? WHERE session_id = ?

View File

@ -101,6 +101,31 @@ ORDER BY
pr.updated_at DESC pr.updated_at DESC
LIMIT 1; LIMIT 1;
-- name: ListPRFactsBySession :many
-- All PR snapshots for a session (every state), with source/target branch for
-- stack derivation and the unresolved-comment flag. The status aggregator
-- filters open vs merged/closed in Go and derives stacks from the branches.
SELECT
pr.url,
pr.number,
pr.pr_state,
pr.review_decision,
pr.ci_state,
pr.mergeability,
pr.source_branch,
pr.target_branch,
pr.updated_at,
EXISTS (
SELECT 1
FROM pr_comment
WHERE pr_comment.pr_url = pr.url
AND pr_comment.resolved = 0
AND pr_comment.is_bot = 0
) AS review_comments
FROM pr
WHERE pr.session_id = ?
ORDER BY pr.updated_at DESC;
-- name: ClaimPRForSession :exec -- name: ClaimPRForSession :exec
INSERT INTO pr (url, session_id, number, pr_state, review_decision, ci_state, mergeability, updated_at) INSERT INTO pr (url, session_id, number, pr_state, review_decision, ci_state, mergeability, updated_at)
VALUES (?, ?, ?, ?, ?, ?, ?, ?) VALUES (?, ?, ?, ?, ?, ?, ?, ?)

View File

@ -24,6 +24,34 @@ func (s *Store) GetDisplayPRFactsForSession(ctx context.Context, id domain.Sessi
return prFactsFromGen(r), true, nil return prFactsFromGen(r), true, nil
} }
// ListPRFactsForSession returns the PR snapshot for every PR a session owns
// (open, merged, and closed), newest first. The status aggregator filters and
// builds stacks from these; an empty slice means the session has no PRs.
func (s *Store) ListPRFactsForSession(ctx context.Context, id domain.SessionID) ([]domain.PRFacts, error) {
rows, err := s.qr.ListPRFactsBySession(ctx, id)
if err != nil {
return nil, fmt.Errorf("list pr facts for %s: %w", id, err)
}
out := make([]domain.PRFacts, 0, len(rows))
for _, r := range rows {
out = append(out, domain.PRFacts{
URL: r.URL,
Number: int(r.Number),
Draft: r.PRState == domain.PRStateDraft,
Merged: r.PRState == domain.PRStateMerged,
Closed: r.PRState == domain.PRStateClosed,
CI: r.CIState,
Review: r.ReviewDecision,
Mergeability: r.Mergeability,
ReviewComments: r.ReviewComments,
SourceBranch: r.SourceBranch,
TargetBranch: r.TargetBranch,
UpdatedAt: r.UpdatedAt,
})
}
return out, nil
}
func prFactsFromGen(r gen.GetDisplayPRFactsBySessionRow) domain.PRFacts { func prFactsFromGen(r gen.GetDisplayPRFactsBySessionRow) domain.PRFacts {
state := r.PRState state := r.PRState
return domain.PRFacts{ return domain.PRFacts{

View File

@ -0,0 +1,81 @@
package store_test
import (
"context"
"testing"
"time"
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
"github.com/aoagents/agent-orchestrator/backend/internal/ports"
)
// ListPRFactsForSession is the real-SQLite batch read the multi-PR status
// aggregator builds stacks from: every owned PR returned newest-first with its
// state flags and branch pair projected (the stack model needs both).
//
// The branch pair is written via WriteSCMObservation (the observer path, the
// source of truth for tracked PRs). The other writer, WritePR, deliberately
// omits source/target branch (UpsertLegacyPR), so the stack model depends on the
// observer having populated the row.
func TestListPRFactsForSessionProjectsAllPRsNewestFirst(t *testing.T) {
s := newTestStore(t)
ctx := context.Background()
seedProject(t, s, "mer")
r, _ := s.CreateSession(ctx, sampleRecord("mer"))
now := time.Now().UTC().Truncate(time.Second)
// A stack: root (open) -> child targets the root branch (open) -> a merged
// historical PR. Distinct updated_at so newest-first ordering is observable.
write := func(pr domain.PullRequest) {
t.Helper()
if err := s.WriteSCMObservation(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve); err != nil {
t.Fatalf("write %s: %v", pr.URL, err)
}
}
write(domain.PullRequest{URL: "root", SessionID: r.ID, Number: 1, CI: domain.CIPassing, SourceBranch: "feat/x", TargetBranch: "main", UpdatedAt: now, ObservedAt: now})
write(domain.PullRequest{URL: "child", SessionID: r.ID, Number: 2, Draft: true, SourceBranch: "feat/x/child", TargetBranch: "feat/x", UpdatedAt: now.Add(time.Second), ObservedAt: now})
write(domain.PullRequest{URL: "old", SessionID: r.ID, Number: 3, Merged: true, SourceBranch: "feat/old", TargetBranch: "main", UpdatedAt: now.Add(2 * time.Second), ObservedAt: now})
facts, err := s.ListPRFactsForSession(ctx, r.ID)
if err != nil {
t.Fatal(err)
}
if len(facts) != 3 {
t.Fatalf("ListPRFactsForSession = %d, want 3", len(facts))
}
// Newest-first by updated_at: old, child, root.
if facts[0].URL != "old" || facts[1].URL != "child" || facts[2].URL != "root" {
t.Fatalf("order = [%s %s %s], want [old child root]", facts[0].URL, facts[1].URL, facts[2].URL)
}
byURL := map[string]domain.PRFacts{}
for _, f := range facts {
byURL[f.URL] = f
}
if !byURL["old"].Merged || byURL["old"].Closed || byURL["old"].Draft {
t.Fatalf("merged PR flags wrong: %+v", byURL["old"])
}
if !byURL["child"].Draft || byURL["child"].Merged {
t.Fatalf("draft child flags wrong: %+v", byURL["child"])
}
// The stack model is derived from the source/target branch pair, so it must
// survive the projection.
if byURL["child"].SourceBranch != "feat/x/child" || byURL["child"].TargetBranch != "feat/x" {
t.Fatalf("child branch pair lost: %+v", byURL["child"])
}
if byURL["root"].SourceBranch != "feat/x" || byURL["root"].TargetBranch != "main" {
t.Fatalf("root branch pair lost: %+v", byURL["root"])
}
if byURL["root"].CI != domain.CIPassing {
t.Fatalf("root CI = %q, want passing", byURL["root"].CI)
}
// A session with no PRs returns an empty (non-nil) slice, never an error.
empty, _ := s.CreateSession(ctx, sampleRecord("mer"))
got, err := s.ListPRFactsForSession(ctx, empty.ID)
if err != nil {
t.Fatal(err)
}
if len(got) != 0 {
t.Fatalf("no-PR session = %d facts, want 0", len(got))
}
}

View File

@ -444,6 +444,23 @@ export interface components {
reason: string; reason: string;
sessionId: string; sessionId: string;
}; };
ControllersSessionView: {
activity: components["schemas"]["DomainActivity"];
/** Format: date-time */
createdAt: string;
displayName?: string;
harness?: string;
id: string;
isTerminated: boolean;
issueId?: string;
kind: string;
projectId: string;
prs: components["schemas"]["SessionPRFacts"][];
status: string;
terminalHandleId?: string;
/** Format: date-time */
updatedAt: string;
};
DegradedProject: { DegradedProject: {
id: string; id: string;
kind: string; kind: string;
@ -479,7 +496,7 @@ export interface components {
sessionId: string; sessionId: string;
}; };
ListSessionsResponse: { ListSessionsResponse: {
sessions: components["schemas"]["Session"][]; sessions: components["schemas"]["ControllersSessionView"][];
}; };
MergePRResponse: { MergePRResponse: {
method: string; method: string;
@ -571,7 +588,7 @@ export interface components {
}; };
RestoreSessionResponse: { RestoreSessionResponse: {
ok: boolean; ok: boolean;
session: components["schemas"]["Session"]; session: components["schemas"]["ControllersSessionView"];
sessionId: string; sessionId: string;
}; };
ReviewRun: { ReviewRun: {
@ -609,22 +626,6 @@ export interface components {
ok: boolean; ok: boolean;
sessionId: string; sessionId: string;
}; };
Session: {
activity: components["schemas"]["DomainActivity"];
/** Format: date-time */
createdAt: string;
displayName?: string;
harness?: string;
id: string;
isTerminated: boolean;
issueId?: string;
kind: string;
projectId: string;
status: string;
terminalHandleId?: string;
/** Format: date-time */
updatedAt: string;
};
SessionPRFacts: { SessionPRFacts: {
ci: string; ci: string;
mergeability: string; mergeability: string;
@ -638,7 +639,7 @@ export interface components {
url: string; url: string;
}; };
SessionResponse: { SessionResponse: {
session: components["schemas"]["Session"]; session: components["schemas"]["ControllersSessionView"];
}; };
SetActivityRequest: { SetActivityRequest: {
/** /**