fix(lifecycle): preserve terminal PR association across restore + harden detectPR (closes #1724)#1726
Open
i-trytoohard wants to merge 5 commits into
Open
fix(lifecycle): preserve terminal PR association across restore + harden detectPR (closes #1724)#1726i-trytoohard wants to merge 5 commits into
i-trytoohard wants to merge 5 commits into
Conversation
`gh pr list --head <branch>` matches PRs across all forks, so when a session's branch is the project default (typically "main") we'd match unrelated contributors' fork PRs. Two defenses: 1. Short-circuit when session.branch === project.defaultBranch — a worker session should never legitimately have its head ref on the default branch, and matching there is unsafe by construction. 2. Request `headRepositoryOwner` from gh and discard PRs whose head owner differs from the project repo owner. Fork PRs are someone else's work. Refs #1724.
When `session.lifecycle.pr.state === "merged"`, the lifecycle manager now short-circuits before calling `scm.detectPR`. The merged head branch is typically deleted on merge, and `session.branch` may drift to the default branch (e.g. via agent-side `git checkout main` between poll cycles or by the metadata-update hook). With branch drift, `gh pr list --head main` matches whatever PRs happen to share that head ref name — including unrelated fork PRs from other contributors — and the lifecycle would then overwrite the session's settled merged-PR association with the unrelated PR's number/url. Closed (unmerged) PRs intentionally do not short-circuit: an agent that pivots to a new branch after a closed PR should still rediscover its follow-up PR via detectPR (existing behavior, exercised by the "refreshes branch metadata again after a closed PR" test). Invariants preserved: - Sessions with `pr.state === "open"` still go through detectPR when they have no attached PR yet (orchestrator/auto-detect-off guards unchanged). - Closed-PR rediscovery on branch switch still works. - The new guard runs after the existing role/auto-detect/no-branch guards so opt-outs continue to take precedence. Refs #1724.
When a session's PR has reached a terminal state (merged or closed) and
the session is later restored, two things conspire to attach the session
to an unrelated PR:
1. The original head branch is typically deleted on merge, so on
workspace restore `session.branch` may drift to the project default
branch (or be rewritten by an agent-side `git checkout main`).
2. With branch == default branch, downstream `detectPR` matches
whatever PR happens to share that head ref name (including fork PRs).
Fix 1 (scm-github) and Fix 2 (lifecycle) defend in depth, but the cleanest
guarantee is to refuse the restore up-front: a session whose PR has
already merged/closed has no remaining work to do. If the user wants
follow-up work, they should spawn a new session.
The previous behavior — silently clearing terminal `lifecycle.pr.*`
fields on restore and resetting `session.state` to "working" — is removed
because it's now unreachable: the new check at step 3a throws before
we get there.
Invariants preserved:
- Workers with `pr.state` of "open"/"none" still restore normally.
- `isRestorable` semantics unchanged for terminated/done/runtime-lost.
- Errors surface via `SessionNotRestorableError`, the same channel used
by other restore-blockers (workspace missing, OpenCode mapping, etc).
Refs #1724.
Contributor
Test Coverage Report
Per-file breakdown
Uncovered lines
|
Contributor
Greptile SummaryThree layered defences against the fork-PR mis-association bug (#1724):
Confidence Score: 5/5Safe to merge — all three changes are narrowly scoped to the bug path and are backed by new regression tests that directly reproduce the reported scenario. The three changes work in concert and each is independently correct: the lifecycle guard prevents detectPR from running after branch drift, the session-manager guard prevents silent state-reset on restore, and the scm-github filter prevents fork PR association even if both earlier guards were somehow bypassed. No existing restorable-session paths are affected — only terminal-PR sessions (merged/closed) are newly rejected. Test coverage for the new branches is thorough. No files require special attention. The two observations in scm-github/src/index.ts (defaultBranch fallback when unconfigured, and undocumented gh ordering for sameRepoPrs[0]) are minor hardening notes, not defects.
|
| Filename | Overview |
|---|---|
| packages/plugins/scm-github/src/index.ts | detectPR now short-circuits on defaultBranch match and filters gh results to same-repo PRs via headRepositoryOwner; limit bumped to 10 to allow filtering before selection |
| packages/core/src/lifecycle-manager.ts | Adds an early continue for sessions whose lifecycle.pr.state === "merged", preventing detectPR from running after branch drift post-merge |
| packages/core/src/session-manager.ts | New step 3a throws SessionNotRestorableError for merged/closed PR state before restore; removes the old clear-on-restore block that was the root enabler of the bug |
| packages/plugins/scm-github/test/index.test.ts | Adds tests for: null/missing headRepositoryOwner rejection, defaultBranch short-circuit, fork-only null, fork+same-repo same-repo wins; existing tests updated with headRepositoryOwner fixtures |
| packages/core/src/tests/lifecycle-manager.test.ts | Adds regression test verifying detectPR is never called when lifecycle.pr.state === merged and session.branch has drifted to main |
| packages/core/src/tests/session-manager/restore.test.ts | Updates merged-PR test to assert rejection (was asserting success); adds independent closed-PR test that writes lifecycle JSON directly to exercise step 3a's closed branch |
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/plugins/scm-github/src/index.ts:615
**Default-branch guard silently inactive when `defaultBranch` is unconfigured**
The guard `if (project.defaultBranch && ...)` is a no-op when `project.defaultBranch` is `undefined` or `""`. In that case a session whose branch drifted to `"main"` (or another default branch name) still proceeds to `gh pr list --head main`. The `headRepositoryOwner` filter provides strong backup protection against fork PRs, but a same-repo PR that coincidentally has `HEAD=main` (e.g. a backport from main) could still be incorrectly auto-associated. A fallback to `"main"` (or the conventional default) when `defaultBranch` is unset would make this guard more reliable independent of project configuration completeness.
### Issue 2 of 2
packages/plugins/scm-github/src/index.ts:664
**`sameRepoPrs[0]` ordering is undocumented**
`gh pr list` returns results in most-recently-updated order by default, so `sameRepoPrs[0]` will be whichever same-repo PR was last touched. In the common case of one active PR per branch this is fine, but if a branch has two open same-repo PRs (e.g. targeting different base branches), the association is ordering-dependent without any tiebreaker. A brief comment noting the reliance on gh's default sort order would make the selection intent clear for future readers.
Reviews (2): Last reviewed commit: "fix(scm-github,session-manager): tighten..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…estore test
Address PR review:
1. Strict-by-default fork filter in `detectPR` — require the head owner
to be a non-empty string AND match the project repo owner. A null,
undefined, or empty `headRepositoryOwner.login` (e.g. deleted account
or future gh API change) no longer silently passes the filter.
2. Update existing same-repo cache/draft tests to include
`headRepositoryOwner: { login: "acme" }` since the strict filter
would otherwise reject them as fork PRs.
3. Add a regression test that null/missing `headRepositoryOwner` is
rejected.
4. Add an independent restore test for the `pr.state === "closed"`
branch of session-manager's step 3a (previously only `merged` was
covered).
Refs #1724.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three layered fixes for #1724, where a restored worker session whose PR had merged got silently re-associated with an unrelated fork PR (in the reported repro:
ao-179(release worker for #1723) ended up polling CI/reviews for fork PR #643 after restore).Per-commit:
fix(scm-github): drop fork PRs and default-branch sessions from detectPR—detectPRnow (a) returnsnullwhensession.branch === project.defaultBranch, and (b) requestsheadRepositoryOwnerfromgh pr listand discards PRs whose head owner differs from the repo owner. Thegh pr list --head <branch>filter previously matched any PR with that head ref name across all forks.fix(lifecycle): skip detectPR for sessions whose PR is already merged— lifecycle-manager short-circuitsscm.detectPRwhensession.lifecycle.pr.state === "merged". Branch drift between poll cycles (agent runsgit checkout main, branch metadata hook fires, lifecycle observes branch=main) was the trigger that let detectPR run against the default branch and pick up unrelated PRs. Closed (unmerged) PRs intentionally still go through detectPR so the existing follow-up-PR rediscovery flow is preserved.fix(session-manager): refuse to restore sessions with merged/closed PRs— restore now throwsSessionNotRestorableErrorup-front when the session's PR is in a terminal state. Previously restore silently clearedlifecycle.pr.*and resetsession.statetoworking, which combined with branch drift caused the bug. The dead clear-on-restore block has been removed.Investigation notes
~/.agent-orchestrator/projects/agent-orchestrator/sessions/ao-179.jsonshowed:branch: "main",lifecycle.pr.number: 643,lifecycle.pr.state: "open",agentReportedPrUrl: ".../pull/1723".lifecycle-manager.ts:657-662previously guarded detectPR bysession.prtruthiness; oncerepairSessionMetadataOnRead(session-manager.ts:623-665) clearedprfor duplicate-PR-attached sessions, detectPR ran unguarded againstsession.branch="main"and matched fork PRs (gh pr list --head mainreturns Fix for #640 ao start fails with "Dependencies not installed" due to npm hoisting of @composio/ao-core #643 by MayurVirkar and feat: Add first-class container deployment support #415 by zvictor).scm-github/src/index.ts::detectPRrequested--head <branch>without owner filtering — the entry point for the fork-match.branchfromchore/release-0.6.0→"main"is the agent-side metadata hook (agent-claude-code/src/index.ts:188-200) firing on agit checkout maininvocation post-merge. It is not pinpointed in this PR because the three fixes here neutralize the impact regardless of how the branch drifted.Invariants preserved
detectPRsemantics unchanged for non-default branches with same-repo PRs (existing tests still green).pr.statein{"open", "none"}continue to be auto-detected; the existingclosed-PR + branch-switch follow-up-detection test still passes; orchestrator/auto-detect-off opt-outs continue to take precedence.isRestorablesemantics unchanged for terminated/done/runtime-lost; only the PR-terminal sub-case now refuses with an explicit reason. Workers withpr.stateof"open"/"none"still restore normally.Test plan
pnpm typecheck— cleanpnpm --filter @aoagents/ao-core test— 1102 unit tests pass; 6 unrelated FTS5 failures stem from a missingbetter-sqlite3native binding in the local sandbox (pre-existing, not caused by this PR)pnpm --filter @aoagents/ao-plugin-scm-github test— 172 tests passscm-github: same-repo PR preferred when both fork and same-repo PRs match; fork-only matches returnnull;branch === defaultBranchshort-circuits without callingghlifecycle-manager: merged-PR session does not calldetectPRsession-manager: restoring a session withstatus: "merged"rejects with/is merged/Closes #1724.