feat(core): support PR handoff history#1789
Conversation
Greptile SummaryThis PR extends
Confidence Score: 5/5Safe to merge — changes are additive, the active PR model is unchanged, and the new cross-repo path is well-guarded by validation and isolated from the branch-conflict heuristic. The core No files require special attention; the logic in
|
| Filename | Overview |
|---|---|
| packages/core/src/session-manager.ts | Adds normalizeRepoOverride, resolveClaimPRTarget, readPRHistory, buildPreviousPREntry, and appendPRHistory helpers; modifies claimPR to support cross-repo PR resolution, preserve replaced PRs in prHistory, and return previousPr in the result. The sameRepoForBranchConflicts guard correctly prevents false-positive branch conflict detection for external repos. |
| packages/core/src/metadata.ts | Adds prHistory to readMetadata (line 171) and writeMetadata — addresses the gap flagged in a prior review where readMetadata silently dropped the field even though writeMetadata already persisted it. |
| packages/core/src/types.ts | Adds ClaimPROptions.repoOverride, PRHistoryEntry interface, and ClaimPRResult.previousPr; extends SessionMetadata with the prHistory?: string field. All additions are additive and backward-compatible. |
| packages/cli/src/commands/session.ts | Adds --repo <owner/repo> option to ao session claim-pr and surfaces result.previousPr in the output. Input validation is delegated to normalizeRepoOverride in the core layer. |
| packages/cli/src/commands/spawn.ts | Adds --claim-pr-repo <owner/repo> option to ao spawn, guards it behind --claim-pr, and threads it through SpawnClaimOptions into sm.claimPR. Guard logic correctly rejects the flag when --claim-pr is absent. |
| packages/core/src/tests/session-manager/claim-pr.test.ts | New test suite with thorough coverage: URL-based repo inference, explicit repoOverride, invalid override rejection, cross-repo non-stripping, idempotent re-claim, sequential PR switching with prHistory validation, and takeover scenarios. |
| packages/core/src/tests/metadata.test.ts | Adds a round-trip test for prHistory write/read via writeMetadata/readMetadata, confirming the fix to the previously silent gap. |
| packages/core/src/prompts/orchestrator.md | Updates the orchestrator prompt to document --claim-pr-repo in the spawn command and --repo in ao session claim-pr, making the new handoff options discoverable to the orchestrator agent. |
| docs/CLI.md | Adds examples and explanatory text for --claim-pr-repo on ao spawn and --repo on ao session claim-pr, including a prose description of prHistory tracking. |
Sequence Diagram
sequenceDiagram
participant CLI as CLI (session/spawn)
participant SM as SessionManager.claimPR
participant RCT as resolveClaimPRTarget
participant SCM as SCM plugin
participant Meta as metadata (updateMetadata)
CLI->>SM: "claimPR(sessionId, prRef, { repoOverride })"
SM->>SM: requireSessionRecord(sessionId) → raw
SM->>RCT: resolveClaimPRTarget(reference, project, options)
RCT->>RCT: normalizeRepoOverride(repoOverride)
RCT->>RCT: parsePrFromUrl(reference) → infer repo from URL
RCT-->>SM: "{ reference: prNumber, project: { ...project, repo } }"
SM->>SCM: resolvePR(reference, claimTarget.project)
SCM-->>SM: PRInfo
SM->>SCM: getPRState(pr) — must be open
SM->>SM: compute sameRepoForBranchConflicts
SM->>SM: loadActiveSessionRecords → find conflicting sessions
SM->>SCM: checkoutPR(pr, workspacePath)
SM->>SM: buildPreviousPREntry(raw, pr.url, replacedAt)
SM->>SM: appendPRHistory(raw, previousPr) → nextPRHistory
SM->>Meta: updateMetadata(pr, branch, prHistory, lifecycle)
loop each conflicting session
SM->>Meta: updateMetadata pr empty prAutoDetect false
end
SM-->>CLI: "ClaimPRResult { pr, previousPr, branchChanged, takenOverFrom }"
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/core/src/session-manager.ts:2795-2797
The `prHistory` slice is computed from the `raw` snapshot captured at the very beginning of `claimPR` via `requireSessionRecord`, not from the live on-disk state at write time. `updateMetadata` internally re-reads and locks the file, so any entry written to `prHistory` by a concurrent mutator between the initial read and this update will be silently overwritten by `nextPRHistory`. In practice `claimPR` is human-initiated and unlikely to race, but reading the history inside `mutateMetadata`'s callback (where the lock is held and `existing` is the current state) would make the write fully atomic.
```suggestion
const replacedAt = new Date().toISOString();
const previousPr = buildPreviousPREntry(raw, pr.url, replacedAt);
// NOTE: nextPRHistory is derived from the `raw` snapshot taken at the start
// of this call. A concurrent write to prHistory between here and
// updateMetadata would be overwritten. This is acceptable for a
// human-initiated command but could be made fully atomic by computing
// the history inside a mutateMetadata callback.
const nextPRHistory = appendPRHistory(raw, previousPr);
```
### Issue 2 of 2
packages/core/src/session-manager.ts:121-128
The segment validation regex `[A-Za-z0-9_.-]+` permits values like `"-org"`, `"org."`, or `".repo"` — strings that neither GitHub nor GitLab accept as valid owner/repo names. Adding simple boundary checks (no leading/trailing `.` or `-`) would give users a clearer error rather than letting the SCM plugin reject it later with a potentially cryptic response.
```suggestion
segments.every((segment) => {
return (
segment.length > 0 &&
segment !== "." &&
segment !== ".." &&
/^[A-Za-z0-9][A-Za-z0-9_.-]*[A-Za-z0-9]$|^[A-Za-z0-9]$/.test(segment)
);
});
```
Reviews (4): Last reviewed commit: "fix(core): require owner repo override f..." | Re-trigger Greptile
- Map prHistory in readMetadata to match writeMetadata and SessionMetadata - Remove stray blockquote prefix before REPO_NOT_CONFIGURED template end - Scope same-branch claim conflicts to the same SCM repo as the claimed PR - Add regression tests for prHistory read and cross-repo branch collisions Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks — merged latest
Prompt template: Removed the stray Cross-repo Let me know if you want Greptile re-run on the new commits. |
|
@harshitsinghbhandari please review |
…tory Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # packages/core/src/session-manager.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
…-1789 # Conflicts: # packages/cli/__tests__/commands/spawn.test.ts # packages/cli/src/commands/spawn.ts # packages/core/src/__tests__/session-manager/claim-pr.test.ts # packages/core/src/metadata.ts # packages/core/src/prompts/orchestrator.md # packages/core/src/session-manager.ts
Problem
ao session claim-prcould attach a new PR to an existing session, but the handoff was lossy: replacing the primary PR erased the previous PR from session metadata, and bare PR numbers were always resolved against the configured project repo. That makes it awkward to recover from closed/replaced PRs or to claim follow-up PRs that live in a different repo.Closes #1618.
What changed
repoOverridetoclaimPR(...)so callers can resolve a bare PR number against an explicitowner/repo.prHistorymetadata when a session claims a different PR.previousPrfromclaimPR(...)and show it inao session claim-proutput.ao session claim-pr 42 app-1 --repo owner/repoao spawn --claim-pr 42 --claim-pr-repo owner/repoWhy this approach
The existing single active
prmetadata field remains the source of truth for lifecycle polling, status derivation, and dashboard behavior.prHistoryis only a bounded historical trail of replaced primary PRs, so this avoids changing the active PR model while giving users and maintainers the auditability needed for explicit handoffs.Testing
pnpm --filter @aoagents/ao-core test -- session-manager/claim-prpnpm --filter @aoagents/ao-cli test -- commands/session commands/spawnpnpm --filter @aoagents/ao-core typecheckpnpm --filter @aoagents/ao-core build && pnpm --filter @aoagents/ao-cli typecheckpnpm exec prettier --check docs/CLI.md packages/core/src/prompts/orchestrator.md packages/core/src/session-manager.ts packages/core/src/types.ts packages/core/src/metadata.ts packages/core/src/__tests__/session-manager/claim-pr.test.ts packages/core/src/__tests__/test-utils.ts packages/cli/src/commands/session.ts packages/cli/src/commands/spawn.ts packages/cli/__tests__/commands/session.test.ts packages/cli/__tests__/commands/spawn.test.tspnpm exec eslint ...on touched source/test files: no errors; existing warnings only.I also ran
pnpm test. It reached the CLI suite and failed on the existing environment-sensitivecanonicalCompareKey > expands ~ to HOMEassertion because macOS resolved the temp HOME as/private/var/...while the test expected/var/.... The feature-specific suites above passed.