Skip to content

feat: ao session claim-pr + spawn --claim-pr wiring#101

Merged
harshitsinghbhandari merged 3 commits into
mainfrom
feat/90
Jun 5, 2026
Merged

feat: ao session claim-pr + spawn --claim-pr wiring#101
harshitsinghbhandari merged 3 commits into
mainfrom
feat/90

Conversation

@harshitsinghbhandari
Copy link
Copy Markdown
Collaborator

Closes #90

Implements the ao session claim-pr <session-id> <pr-ref> lifecycle surface plus ao spawn --claim-pr wiring from the design at /tmp/aa-session-claim-pr-design.md.

What changed:

  • Added GET /api/v1/sessions/{sessionId}/pr and POST /api/v1/sessions/{sessionId}/pr/claim with the specified JSON envelopes and error mapping.
  • Added the service/store PR-claim path with live GitHub SCM observation, active-owner takeover guard, and CLI-side gh repo view --json url -q .url shorthand fallback when a project has no stored repo URL.
  • Added ao session claim-pr and ao spawn --claim-pr / --no-takeover CLI flows.
  • Added migration 0004_pr_session_changed_cdc.sql, widening change_log.event_type and introducing the new pr_session_changed CDC event for PR ownership transfers.
  • Added tests across store, service, controller, and CLI layers.

Local gate run from backend/:

go build ./... && go test -race ./... && go vet ./...

Intentional omission: --assign-on-github remains out of scope per the design.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 4, 2026

Greptile Summary

This PR wires up ao session claim-pr and ao spawn --claim-pr end-to-end: new HTTP endpoints, a service layer that fetches live SCM facts and enforces open-state/repo-match guards, an atomic store-layer claim that fires the new pr_session_changed CDC event, and a migration that widens change_log.event_type.

  • Service/store claim path: ClaimPR atomically upserts via ClaimPRForSession + writePRRows, enforces active-owner takeover guard, and emits a pr_session_changed CDC event via the new pr_session_cdc_update trigger.
  • CLI surface: ao spawn --claim-pr spawns then claims in a second call with automatic rollback on failure; ao session claim-pr adds --no-takeover support and gh repo view fallback for numeric refs.
  • Migration 0006: Recreates change_log with the extended event_type CHECK constraint and adds the pr_session_cdc_update trigger; rollback correctly drops pr_session_changed rows before reverting.

Confidence Score: 4/5

Safe to merge with awareness of one CDC correctness issue in the PR takeover path.

During a PR re-claim (takeover), ClaimPRForSession ON CONFLICT DO UPDATE SET only updates session_id, review_decision, and updated_at. If review_decision changed, the pr_cdc_update trigger fires immediately with stale pr_state/ci_state/mergeability, and the correcting event from the subsequent UpsertPR shares the same updated_at. CDC consumers that deduplicate on URL+timestamp could retain the stale snapshot.

backend/internal/storage/sqlite/queries/pr.sql and its generated counterpart backend/internal/storage/sqlite/gen/pr.sql.go — ClaimPRForSession ON CONFLICT clause needs pr_state, ci_state, and mergeability added to its SET list.

Important Files Changed

Filename Overview
backend/internal/storage/sqlite/queries/pr.sql Adds ClaimPRForSession and GetPRClaimAndOwner queries; ON CONFLICT clause omits pr_state/ci_state/mergeability updates, causing stale CDC events during takeover.
backend/internal/service/session/claim_pr.go Core claim-PR service logic with SCM fetch, repo-match and open-state guards; BranchChanged is intentionally false pending branch-checkout implementation.
backend/internal/storage/sqlite/store/pr_store.go Adds ClaimPR store method with two-step upsert; comment incorrectly cites migration 0005 instead of 0006.
backend/internal/storage/sqlite/migrations/0006_pr_session_changed_cdc.sql Widens change_log event_type CHECK constraint and adds pr_session_cdc_update trigger; rollback correctly reverts.
backend/internal/httpd/controllers/sessions.go Adds GET /sessions/{id}/pr and POST /sessions/{id}/pr/claim with correct error mapping and AllowTakeover defaulting to true.
backend/internal/cli/spawn.go Adds --claim-pr / --no-takeover flags; rolls back spawned session if claim fails.
backend/internal/cli/session.go Adds session claim-pr subcommand with DTOs and writeClaimPRResult.
backend/internal/adapters/scm/github/provider.go Adds scmObserveError to wrap ErrNotFound as ports.ErrSCMPRNotFound in the legacy Observe path.
backend/internal/cli/pr_ref.go New file; resolvePRRef normalizes numeric and URL PR refs with gh repo view fallback.
backend/internal/storage/sqlite/gen/pr.sql.go sqlc-generated code reflecting the same partial ON CONFLICT SET columns as the source SQL.

Sequence Diagram

sequenceDiagram
    participant CLI
    participant HTTP as HTTP /sessions/{id}/pr/claim
    participant Svc as session.Service.ClaimPR
    participant SCM as GitHub SCM
    participant Store as SQLite Store (ClaimPR tx)
    participant CDC as change_log (triggers)
    CLI->>HTTP: "POST pr/claim {pr, allowTakeover}"
    HTTP->>Svc: ClaimPR(id, ref, opts)
    Svc->>Svc: normalizePRRef / requireSameGitHubRepo
    Svc->>SCM: FetchPullRequests([ref])
    SCM-->>Svc: SCMObservation
    Svc->>SCM: FetchReviewThreads(ref)
    SCM-->>Svc: review threads
    Svc->>Store: ClaimPR(pr, checks, threads, comments, reviewMode, allowTakeover)
    Store->>Store: GetPRClaimAndOwner
    alt "PR owned by active session and allowTakeover=false"
        Store-->>Svc: PRClaimedByActiveSessionError
        Svc-->>HTTP: 409 PR_CLAIMED_BY_ACTIVE_SESSION
    else
        Store->>Store: ClaimPRForSession UPSERT
        Store->>CDC: pr_session_cdc_update fires pr_session_changed
        Store->>Store: writePRRows UpsertPR + checks + threads
        Store->>CDC: pr_cdc_update fires pr_updated
        Store-->>Svc: ClaimOutcome
        Svc-->>HTTP: ClaimPRResult
        HTTP-->>CLI: 200 ClaimPRResponse
    end
Loading

Reviews (4): Last reviewed commit: "fix: align PR claim branch with latest m..." | Re-trigger Greptile

Comment thread backend/internal/service/session/claim_pr.go
@harshitsinghbhandari
Copy link
Copy Markdown
Collaborator Author

Stale review_decision in the first-claim pr_created CDC event.

ClaimPRForSession (generated at backend/internal/storage/sqlite/gen/pr.sql.go from backend/internal/storage/sqlite/queries/pr.sql) omits review_decision from both the INSERT column list and the ON CONFLICT SET clause. On a first-time claim, the row is inserted with the column's default ('none'), the pr_cdc_insert trigger (migration 0004 lines 164-173) fires and writes a pr_created event payload carrying review: 'none'. The follow-up UpsertPR inside the same transaction populates the real value and fires pr_cdc_update, but the window — although same-transaction — is visible to any eager CDC consumer that processes pr_created before reading the subsequent pr_updated.

Fix: add review_decision to ClaimPRForSession's INSERT columns (taking it from the parameter set, mirroring how UpsertPR plumbs it) and add it to the ON CONFLICT SET. The generated code rebuild is mechanical (sqlc generate). No migration needed.

Once this PR lands pr_session_changed, downstream consumers (SSE controller in #110, dashboard later) will rely on the pr_created payload being final, so locking this down here is cheap and saves a "why is review_decision always 'none' on first observation" debug session later.

@harshitsinghbhandari
Copy link
Copy Markdown
Collaborator Author

Orphan session on ao spawn --claim-pr when the claim step fails after a successful spawn.

backend/internal/cli/spawn.go's --claim-pr flow is a two-step HTTP sequence: POST /sessions succeeds, then POST /sessions/{id}/pr/claim runs. If the claim call returns any error (network blip, takeover refusal under --no-takeover, PR not found, GitHub auth fail), the spawned session is left dangling — runtime allocated, worktree created, no PR attached, no remediation hint surfaced to the user. The error message that bubbles up from the controller does not tell the user the spawn already succeeded, so the natural retry (ao spawn --claim-pr ... again) compounds the leak.

Two options, leaning toward (a):

(a) CLI-side cleanup on claim failure. On a non-fatal claim error, call DELETE /sessions/{id} (or ao session kill equivalent) and surface a single combined error: "spawn succeeded as <id> but claim failed: <err>; cleaned up. Retry with ao session claim-pr <id> after fixing the underlying cause." Keeps the API simple but couples the two operations in the CLI.

(b) Server-side combined route. POST /sessions?claimPR=... that runs spawn + claim under one rollback boundary. Cleaner semantically but a bigger change for this PR; also makes the operation atomic which may not be desirable (a successful spawn is still useful even without the PR).

(a) is the smaller fix and matches how the CLI already composes other multi-step flows. Either way, the error message should mention the session id so a user who picks "leave it" can find it later with ao session ls.

@harshitsinghbhandari harshitsinghbhandari merged commit 3413acc into main Jun 5, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port missing CLI commands from original AO (read + lifecycle surface)

1 participant