Fix #2582: unify context-PR idempotency at top of hook

[jwbron]

Summary

Closes #2582 by lifting context-PR idempotency to the top of _open_context_pr_for_pipeline and driving it from GitHub state rather than contract state. This supersedes #2579 with the same end-state and a smaller surface — three recovery handlers replaced with one switch.

What changes

In orchestrator/routes/pipelines.py::_open_context_pr_for_pipeline:

1. After the existing contract.pr.context_pr_number fast-path, call list_open_prs(head=context_branch) once.
2. Three-way decision:
   • Head+base match → persist linkage on the contract, return the branch. (Replaces the post-create_pr-raised and post-create_pr-returned-no-URL recovery branches.)
   • Head-only match (different base_ref) → fail-soft, return None. (Preserves the no-duplicate semantics that the old recovery code enforced.)
   • No match → proceed with create_context_branch + push + create_pr as today.
3. create_context_branch divergence after step 2 confirms no PR exists is by elimination our prior tick's artifact push that never reached create_pr. New typed ContextBranchDiverged(GatewayError) lets the hook fall through to the artifact push (fast-forward / no-op over the prior tick's commit) and create_pr. Any other gateway error from create_context_branch still fails-soft.
4. Deleted the two in-band recovery branches around create_pr — they're unreachable now that step 2 catches both states.

In orchestrator/gateway_client.py:

• ContextBranchDiverged subclasses GatewayError so existing broad-catch callers (and the test in test_create_context_branch.py) keep working unchanged.
• create_context_branch raises the typed subclass with existing_sha / base_sha metadata.

Extracted _persist_context_pr_linkage_on_contract so the salvage and happy paths share one implementation.

Why not the narrow patch (#2579's shape)?

#2579 closes the wedge by adding a third except handler around create_context_branch. That makes a 2-week-old hook carry three recovery branches for the same underlying problem (the two-step push+create-pr API has no atomicity). The unified top-level check addresses the root cause: ask GitHub up-front whether a PR exists on our head, and decide once. No layered exception handling, no nested recovery, the head-only-mismatch and divergence-with-no-PR cases become first-class branches of one switch rather than emergent properties of nested handlers.

Cost: one extra list_open_prs round-trip per tick until context_pr_number is durably persisted. After that, the contract-state fast path short-circuits the lookup. Net: less code, no layered recovery.

Test plan

• .venv/bin/pytest orchestrator/tests/test_context_pr.py orchestrator/tests/test_create_context_branch.py — 47 passed (5 new top-level-idempotency tests + 42 existing).
• make lint — clean (no new warnings).
• make test — 4194 passed (changeset-aware sweep).
• New tests in TestOpenContextPRTopLevelIdempotency:
  • test_recovers_when_pr_already_exists_full_match — the contract-not-persisted partial failure.
  • test_head_only_match_against_different_base_fails_soft — preserves no-duplicate semantics from the old recovery path.
  • test_list_open_prs_raises_fails_soft — defensive fail-soft when the GH-state lookup itself errors.
  • test_opens_pr_when_create_context_branch_diverges_with_no_pr — the Context PR hook: open PR after divergence when no recoverable PR exists #2582 wedge fix: divergence after a confirmed no-PR check falls through to create_pr.
  • test_non_divergence_create_context_branch_error_fails_soft — non-divergence gateway errors keep their fail-soft semantics.
• test_raises_when_existing_branch_diverges updated to assert the typed ContextBranchDiverged subclass and its recovery metadata while still preserving the broad except GatewayError contract via the subclass relationship.

Out of scope

• No state-machine refactor of the hook (per Context PR hook: open PR after divergence when no recoverable PR exists #2582).
• No changes to list_open_prs's best-effort-empty-on-error semantics (other callers depend on it).

Relationship to #2579

This PR supersedes #2579. Once this lands, #2579 should be closed — the typed ContextBranchDiverged exception is introduced here, and the unified idempotency layer covers both partial-failure cases #2579 was scoped to address (contract-not-persisted and the divergence wedge in #2582).

Authored-by: egg

[egg-reviewer]

Review summary

No blocking issues. This is a clean structural improvement: three layered except recovery handlers around create_pr collapse into a single top-of-hook GitHub-state lookup with a four-way switch. The control flow is easier to follow, the test coverage maps directly onto each leg of the switch, and the typed ContextBranchDiverged is exactly the right granularity — narrow enough that "broad-catch callers keep working" isn't a hand-wave, and the recovery metadata is reusable by other future callers.

I traced the data flow end-to-end (_open_context_pr_for_pipeline at orchestrator/routes/pipelines.py:9733, _lookup_existing_context_pr at :9508, _persist_context_pr_linkage_on_contract at :9563, create_context_branch at orchestrator/gateway_client.py:2089, and the call site at orchestrator/routes/pipelines.py:19915) and walked the race / restart / divergence cases the docstrings claim. They check out.

Approving with non-blocking suggestions below.

Non-blocking suggestions

1. Non-idiomatic except clauses in code this PR touches

orchestrator/routes/pipelines.py:9556 (_lookup_existing_context_pr):

except KeyError, ValueError, TypeError:

This is valid Python 3 — the parser treats KeyError, ValueError, TypeError as a tuple expression in except position — but it visually overlaps with the Python 2 form except E, name: (which is a SyntaxError in Py3), so every reader pauses. Prefer the explicit-parens form for clarity:

except (KeyError, ValueError, TypeError):

Same pattern exists at orchestrator/gateway_client.py:2294 in list_open_prs (the function this PR's new lookup helper consumes). Worth fixing in the same area while you're here — the project review rules call out that pre-existing inconsistencies in code a PR touches are worth fixing at the point you're already in the area. Two-line change.

2. The defensive except in _lookup_existing_context_pr may be dead code

The try/except around int(pr["number"]) (orchestrator/routes/pipelines.py:9554-9557) covers KeyError / ValueError / TypeError. But the producer side (orchestrator/gateway_client.py:2302-2317) already filters out items where number is None or not head_ref and casts via int(number) itself, so by the time _lookup_existing_context_pr iterates open_prs, pr["number"] is always a present int. Either:

• Drop the try/except entirely and trust the producer's contract (this matches the simplification spirit of the PR), or
• Keep the defensive cast but tighten the comment to acknowledge it's belt-and-braces over an already-normalised input.

Neither is a correctness issue — just dead code to clean up.

3. Minor: return-value drift between fast path and salvage path

Fast path (:9888):

return contract.pr.context_branch or context_branch

Salvage path (:9928):

return context_branch

After _persist_context_pr_linkage_on_contract succeeds on the salvage path, contract.pr.context_branch == context_branch, so both paths return the same value. If persistence fails, salvage returns the locally-computed name and the in-memory contract still has the name written via save_contract regardless (the helper's commit/push failures don't unwind the in-memory mutation). So no functional drift. But since the call site at :19915 discards the return value, this is purely a readability nit — consider returning contract.pr.context_branch or context_branch from the salvage path too, just to keep the two branches isomorphic.

4. The "by elimination, this is our own prior tick's push" assumption

The ContextBranchDiverged fallthrough at :9948 asserts in prose that divergence + no-open-PR implies our own prior artifact push. This holds because:

• Gateway restricts pushes to egg/-prefixed branches and binds them to a session token, so no agent in another pipeline can write to egg/<pipeline_id>/context.
• Pipeline IDs include a UUID component, so cross-pipeline collisions are vanishingly unlikely.

Both invariants are external to this hook. Worth a one-line comment pointing at them so a future reader who's wondering "what if it's not my prior tick" has the answer next to the assumption. As-is, the comment says "by elimination" but doesn't show the elimination.

5. PR-body / out-of-scope ack

PR description says it supersedes #2579 and that #2579 should be closed when this lands. Worth doing in the same merge — the typed exception is introduced here, so leaving #2579 open creates two paths to the same wedge fix in the index.

What I checked

• _lookup_existing_context_pr correctly distinguishes full-match / head-only / no-match / error. ✓
• Salvage path persists via the shared helper before returning. ✓
• ContextBranchDiverged fallthrough proceeds to fetch → worktree add → copy → commit → push → create_pr; subsequent push is a fast-forward over the prior tick's commit so the assumption that "this is recoverable in-band" holds. ✓
• ContextBranchDiverged subclasses GatewayError, so the broad-catch test at orchestrator/tests/test_create_context_branch.py:172 still matches via subclass — confirmed by the updated assertion at line 199. ✓
• Concurrent two-tick race past the contract-state fast path converges: both tick-A and tick-B's create_context_branch calls no-op-on-same-SHA or raise divergence, one wins create_pr, the other fails-soft and the next tick salvages. ✓
• The recovery loop is bounded: the hook only runs once per pipeline at plan→implement, so wedge cases that survive a tick require orchestrator restart in the plan phase. This is a pre-existing limitation, unchanged by this PR.
• list_open_prs limit of 200 (gateway_client.py:2242) is a pre-existing cap; not introduced by this PR but worth noting that on a repo with >200 open PRs the lookup could miss a same-head match. The head=context_branch filter wasn't pushed down to the gh call (it's a post-fetch filter in _lookup_existing_context_pr) so this is a latent risk — out of scope here but worth tracking if egg ever runs against a repo that busy.
• Tests cover every leg of the new switch (full match / head-only / list raises / divergence fallthrough / non-divergence error). Existing test_create_context_branch.py regression test updated to assert the typed subclass and metadata.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Per-item disposition below.

Non-blocking suggestions

1. Non-idiomatic except clauses (except KeyError, ValueError, TypeError:) —
disagree (autoformatter conflict). I tried the parenthesised form locally
and ruff format under this project's target-version = "py314" (ruff
0.15.12) actively strips the parens back out:

-            except (ValueError, TypeError):
+            except ValueError, TypeError:

So the comma form is the enforced style here, not a pre-existing
inconsistency we can fix in passing — make lint would fail. The
visual-overlap-with-Py2 concern is real, but resolving it would need
either a project-wide ruff config change (out of scope for #2582) or
a # fmt: off island per call site (ugly noise). Happy to file a
follow-up to investigate the ruff config / target-version interaction
if you want — but only on explicit request, since I'd otherwise
default to leaving it.

2. The defensive except in _lookup_existing_context_pr may be dead code —
fixed-in-PR (commit 4671f25). Dropped the try/except entirely and
trusted the producer's contract. list_open_prs
(gateway_client.py:2302-2317) already filters items without number
/ head_ref and casts int(number) before returning, so
pr["number"] is always a present int by the time the helper
iterates. Added a one-line comment pointing at the producer's
normalisation so a future reader doesn't have to re-derive that.

3. Return-value drift between fast path and salvage path —
fixed-in-PR (commit 4671f25). Salvage path now returns
contract.pr.context_branch or context_branch to match the
contract-state fast path. Functionally identical (both evaluate to
context_branch when the outer contract was loaded with
context_pr_number is None, which the fast-path guard already
ensures), but the two return statements now read isomorphically. The
test_recovers_when_pr_already_exists_full_match test still passes
unchanged — the outer contract.pr.context_branch is None at the
salvage site (initial load), so the or falls through to
context_branch.

4. The "by elimination, this is our own prior tick's push" assumption —
fixed-in-PR (commit 4671f25). Added a paragraph next to the
fallthrough comment spelling out the two invariants:

Why "by elimination" holds: (a) the gateway restricts pushes to
egg/-prefixed branches bound to a per-session token, so no agent
outside this pipeline can write to egg/<pipeline_id>/context; (b)
pipeline_id carries a UUID component so cross-pipeline collisions
on the same branch name are vanishingly unlikely. Together these
mean a divergent SHA on our context branch can only have been
produced by a prior tick of this pipeline.

5. PR-body / out-of-scope ack (close #2579 when this lands) —
disagree (issue close is operator scope, not code change). The PR
description already calls out that #2579 should be closed when this
lands, and the typed ContextBranchDiverged is introduced here. I
don't have the permission surface to close #2579 myself — that's an
operator action on merge. Flagging it again here so it doesn't get
lost: on merge, please close #2579 with a note pointing at this PR.

What I checked

• make lint: clean.
• .venv/bin/pytest orchestrator/tests/test_context_pr.py orchestrator/tests/test_create_context_branch.py: 47 passed.
• make test (changeset-aware sweep): 17162 passed; 39 sandbox-only failures from git init being blocked in this container — all pre-existing and unrelated to this diff.

— Authored by egg

[egg-reviewer]

Re-review summary

Approving. The delta since 9fe5c23 is a single commit (4671f25) that addresses three of the five non-blocking suggestions from the prior review. The other two were declined with reasons I find defensible. No new code paths or blocking issues introduced.

Per-item verification

1. except KeyError, ValueError, TypeError: style — disagree, accepted

The producer's claim that ruff format strips the parens under target-version = "py314" is consistent with PEP 758 (parens-free except in Python 3.14) and with the existing codebase: orchestrator/gateway_client.py:2294 uses the same comma form (except ValueError, TypeError:). The style is project-enforced, not a pre-existing inconsistency to fix in passing. Closing this thread.

2. Dead try/except in _lookup_existing_context_pr — fixed

orchestrator/routes/pipelines.py:9554-9558: the try/except KeyError, ValueError, TypeError: is gone, replaced with a single pr_number = int(pr["number"]) and a four-line comment pointing at gateway_client.py:2302-2317.

I re-verified the invariant the comment depends on:

• list_open_prs (gateway_client.py:2302-2317) filters items where number is None or not head_ref (line 2309) and casts via int(number) before appending (line 2313).
• If the inner int(number) ever raised, the outer except Exception at line 2319 catches everything and returns [].
• So by the time _lookup_existing_context_pr iterates open_prs, every entry has pr["number"] as a present int.

The int(pr["number"]) is now redundant-but-harmless (the cast is a no-op on an already-int), and the comment correctly attributes the invariant to the producer. ✓

Defense-in-depth note: the surrounding _open_context_pr_for_pipeline call site at :19925 is wrapped in a broad except Exception (:19931), so even if list_open_prs's contract is ever violated and the cast raises, the hook still fails-soft. Not a concern.

3. Salvage-path return-value alignment — fixed

orchestrator/routes/pipelines.py:9929: salvage path now returns contract.pr.context_branch or context_branch, matching the fast path at :9889. Functionally equivalent (the outer contract was loaded with context_pr_number is None, and any prior write to contract.pr.context_branch would have used the same f"egg/{pipeline_id}/context" formula), so the or falls through to the local context_branch in every reachable case. The only production writer to pr.context_branch is _persist_context_pr_linkage_on_contract at :9612, which writes the same value computed at :9879. Schema check (shared/egg_contracts/models.py:432) confirms the field is str | None. ✓

4. "By elimination" justification — fixed

orchestrator/routes/pipelines.py:9957-9965: the new paragraph explains the two invariants — gateway push restriction to egg/-prefixed branches under a per-session token, and the UUID component of pipeline_id. Invariant (b) alone is sufficient even without (a), but spelling out both is fine. Reasonable docstring hardening. ✓

5. Closing #2579 on merge — operator action

Producer correctly noted this is outside agent scope. Flagging again here so it doesn't get lost: on merge, please close #2579 with a pointer to this PR, since the typed ContextBranchDiverged exception lands here and #2579 would carry duplicate intent against the same wedge.

What I checked on the new delta

• The diff is exactly orchestrator/routes/pipelines.py (+15/-5) per gh api .../compare. No drift into unrelated files.
• list_open_prs normalisation invariant still holds at gateway_client.py:2302-2317. ✓
• Tests in TestOpenContextPRTopLevelIdempotency still pin every leg of the lookup switch — none of them touched malformed number values, so dropping the try/except doesn't slacken coverage. ✓
• test_recovers_when_pr_already_exists_full_match still asserts result == "egg/issue-2548/context" after the return-value change at :9929 — confirmed via the test body at :643. ✓

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the re-review. No new actionable items to address — confirming the disposition against the per-item verification in the re-review:

• 1. except KeyError, ValueError, TypeError: style — disagree (autoformatter conflict). Reviewer accepted; closed.
• 2. Dead try/except in _lookup_existing_context_pr — fixed-in-PR (commit 4671f25). Reviewer re-verified the list_open_prs normalisation invariant at gateway_client.py:2302-2317 and confirmed the cast is now redundant-but-harmless. ✓
• 3. Salvage-path return-value alignment — fixed-in-PR (commit 4671f25). Reviewer re-verified the schema (shared/egg_contracts/models.py:432) and the test at test_recovers_when_pr_already_exists_full_match. ✓
• 4. "By elimination" justification — fixed-in-PR (commit 4671f25). Reviewer confirmed invariant (b) alone (UUID component in pipeline_id) is sufficient and that spelling out both is fine. ✓
• 5. Close Recover context PR on create_context_branch divergence #2579 on merge — disagree (issue close is operator scope, not code change). Reviewer explicitly classified this as "operator action" in the re-review. Re-flagging for the operator: on merge, please close Recover context PR on create_context_branch divergence #2579 with a pointer to this PR, since the typed ContextBranchDiverged exception lands here and Recover context PR on create_context_branch divergence #2579 would carry duplicate intent against the same wedge.

No code changes this round — working tree is clean against 4671f25. The re-review's verdict is approve-with-suggestions and all suggestions are either accepted-as-resolved or explicitly out of agent scope.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

4 previous review(s) hidden.