Skip to content

feat(core): support PR handoff history#1789

Open
ChiragArora31 wants to merge 7 commits into
AgentWrapper:mainfrom
ChiragArora31:feat/pr-handoff-history
Open

feat(core): support PR handoff history#1789
ChiragArora31 wants to merge 7 commits into
AgentWrapper:mainfrom
ChiragArora31:feat/pr-handoff-history

Conversation

@ChiragArora31

Copy link
Copy Markdown
Contributor

Problem

ao session claim-pr could 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

  • Added repoOverride to claimPR(...) so callers can resolve a bare PR number against an explicit owner/repo.
  • Infer the repository from full GitHub/GitLab-style PR URLs before resolving the PR.
  • Preserve the replaced primary PR in prHistory metadata when a session claims a different PR.
  • Return previousPr from claimPR(...) and show it in ao session claim-pr output.
  • Added CLI support:
    • ao session claim-pr 42 app-1 --repo owner/repo
    • ao spawn --claim-pr 42 --claim-pr-repo owner/repo
  • Updated the orchestrator prompt and CLI docs so the new handoff options are discoverable.

Why this approach

The existing single active pr metadata field remains the source of truth for lifecycle polling, status derivation, and dashboard behavior. prHistory is 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-pr
  • pnpm --filter @aoagents/ao-cli test -- commands/session commands/spawn
  • pnpm --filter @aoagents/ao-core typecheck
  • pnpm --filter @aoagents/ao-core build && pnpm --filter @aoagents/ao-cli typecheck
  • pnpm 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.ts
  • pnpm 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-sensitive canonicalCompareKey > expands ~ to HOME assertion because macOS resolved the temp HOME as /private/var/... while the test expected /var/.... The feature-specific suites above passed.

@greptile-apps

greptile-apps Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extends claimPR with cross-repo support (repoOverride / --repo), URL-based repo inference, and a bounded prHistory trail that records replaced primary PRs — making the handoff recoverable and auditable without changing the active PR lifecycle model.

  • Cross-repo claiming: normalizeRepoOverride validates owner/repo format; resolveClaimPRTarget infers the repo from a full GitHub/GitLab URL or falls back to the explicit override; sameRepoForBranchConflicts correctly suppresses false-positive branch-based ownership conflicts when the claimed PR is in a different repository.
  • prHistory trail: buildPreviousPREntry / appendPRHistory record the displaced PR (URL, number, branch, timestamp) into a bounded JSON array (max 20 entries) before updateMetadata atomically writes the new primary PR; readMetadata now surfaces the field (fixing a previously noted silent gap).
  • CLI and prompt: --claim-pr-repo added to ao spawn, --repo to ao session claim-pr, both guarded against misuse; orchestrator prompt and CLI.md updated for discoverability.

Confidence Score: 5/5

Safe 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 claimPR mutation path is unchanged in shape — only new pre-processing (resolveClaimPRTarget) and post-processing (appendPRHistory) are added. The cross-repo conflict guard (sameRepoForBranchConflicts) is correctly derived and covered by a dedicated test. prHistory is a pure metadata append with no effect on lifecycle polling. The two observations logged are theoretical edge cases with no current failure path.

No files require special attention; the logic in session-manager.ts around sameRepoForBranchConflicts and appendPRHistory is the most consequential new code and is well covered by claim-pr.test.ts.

Important Files Changed

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 }"
Loading
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

Comment thread packages/core/src/metadata.ts
Comment thread packages/core/src/prompts/orchestrator.md Outdated
ChiragArora31 and others added 2 commits May 11, 2026 21:50
- 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>
@ChiragArora31

Copy link
Copy Markdown
Contributor Author

Thanks — merged latest main and pushed fixes for the three review points.

readMetadata: prHistory is now read from disk the same way we already persist it in writeMetadata, with a small round-trip test in metadata.test.ts.

Prompt template: Removed the stray > before {{REPO_NOT_CONFIGURED_SECTION_END}} so Markdown does not emit an empty blockquote line after templating.

Cross-repo sameBranch: Branch-based takeover now runs only when the claimed PR’s SCM repo matches the session project’s configured repo (both normalized). Claiming an external PR with e.g. main no longer clears an unrelated local session on main. Regression test covers repoOverride + overlapping branch names.

Let me know if you want Greptile re-run on the new commits.

@ChiragArora31

Copy link
Copy Markdown
Contributor Author

@harshitsinghbhandari please review

ChiragArora31 and others added 2 commits May 17, 2026 04:56
…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
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.

Session is locked to its first PR — can't reattach when that PR is closed or replaced

1 participant