Fix #2368: synthetic-session exemption for slice integration-branch pushes

[jwbron]

Summary

Closes #2368.

Multi-slice implement phases were stranded because two correct-in-isolation behaviours collided:

• BRC coder push thrashes through 3 wrong incantations before landing on the correct one #2028 (gateway): every pipeline-session push without consensus_push=true is rejected unless the target is in INFRASTRUCTURE_BRANCHES (only CHECKPOINT_BRANCH and PIPELINE_STATE_BRANCH).
• Slice the implement phase into a DAG of independent units #2220 (orchestrator): create_slice_integration_branch registers a synthetic pipeline session and pushes parent:refs/heads/<integration> via /api/v1/git/push so the slice PR's diff is non-empty before agents spawn.

Every multi-slice pipeline 403'd on the per-slice integration push and 15 slices failed before any agent ran. Latent since #2220 — only unmasked once #2337 stopped silently demoting multi-slice contracts to monolithic implement.

Changes

• gateway/gateway.py — extend the infrastructure-push bypass: a session whose synthetic flag is True (identity check, not truthy — defends against MagicMock-style fakes and any future non-bool storage) targeting egg/<base>/(slice|phase)-N is treated as orchestrator infrastructure. The synthetic flag can only be set by the launcher (/api/v1/sessions/create is gated on require_launcher_auth), so a sandboxed agent's session token cannot reach this exemption. Audit emits a distinct push_slice_integration_exempt event and tags the existing push_infrastructure_exempt with exempt_type=slice_integration_branch.
• orchestrator/gateway_client.py — docstring of create_slice_integration_branch now points at the actual exemption mechanism (it previously referenced "decision-15 / orchestrator role's existing prefix-allowlist," which never gated the push).
• orchestrator/routes/pipelines.py — bonus fix from Multi-slice implement phase fails: create_slice_integration_branch blocked by pipeline-session push enforcement #2368: slice integration-branch derives from pipeline.branch, so a qualifier suffix (-v3, -backend) is preserved. Two qualified pipelines for the same issue would otherwise collide in the egg/issue-N/slice-M namespace.

No new orchestrator-role push surface is introduced; agent BRC enforcement is unchanged.

Tests

• Gateway (test_pipeline_push_block.py::TestSliceIntegrationBranchExemption): 7 new cases — issue / qualified-issue / JIRA / legacy phase-N parent shapes accepted; non-synthetic session on slice-shaped branch still blocked (the synthetic flag is load-bearing); synthetic session on a non-slice branch still blocked (the regex is load-bearing); audit event emits the expected exempt type.
• Orchestrator (test_slice_run_loop_integration.py::TestSliceIntegrationBranchPrecedesAgentSpawn, TestSliceIntegrationBranchQualifierPreserved): assert create_slice_integration_branch precedes _run_concurrent_phase (catches the Slice the implement phase into a DAG of independent units #2220-style ordering regression directly), assert _run_concurrent_phase is NOT invoked when integration-branch creation fails, assert qualified-pipeline branches propagate to parent_branch and integration_branch.

Recovery for issue-2261-v3

Once this lands: restart_phase implement reruns the slice loop with each create_slice_integration_branch call now succeeding. No data migration required.

Test plan

• make test — 4654 passed (3 pre-existing path-traversal failures in test_phase_api.py::TestPathTraversalProtection are unrelated and present on main).
• make lint — clean.

[egg-reviewer]

Review summary

The synthetic-session exemption in gateway.py is sound: Session.synthetic is only ever set through /api/v1/sessions/create, which is gated by @require_launcher_auth, and the regex check provides defense-in-depth. The is True identity comparison correctly defends against MagicMock truthiness shenanigans. The qualifier-preservation fix at routes/pipelines.py:10989 is the right call — pipeline_branch is the canonical source.

But this PR ships with a cross-module silent no-op in stacked_pr_reconciler.py that this PR's own changes expose. Requesting changes.

---

BLOCKING

orchestrator/stacked_pr_reconciler.py:181 — qualifier dropped, orphan detection silently never matches for qualified pipelines

The reconciler computes the issue branch like this:

issue_number = contract.issue.number if contract.issue is not None else None
pipeline_id = contract.contract_key
issue_branch = f"egg/issue-{issue_number}" if issue_number else f"egg/{pipeline_id}"

For a qualified pipeline (pipeline_id="issue-2261-v3", issue.number=2261), this resolves to egg/issue-2261 — the unqualified form. But after this PR, the orchestrator's slice loop creates branches as egg/issue-2261-v3/slice-1 (qualifier preserved — that's literally what routes/pipelines.py:10989 was just fixed to do). So the lookup at line 189:

slice_branch = f"{issue_branch}/{slice_.id}"   # → "egg/issue-2261/slice-1"
pr = pr_by_head.get(slice_branch)              # never matches; real head is egg/issue-2261-v3/slice-1

… returns None for every qualified-pipeline slice, the orphan loop continues, and find_orphaned_child_prs silently returns []. No log line, no alert, no orphan ever healed. For any qualified or -backend-style pipeline (which is exactly the use case pipeline_branch was added for), the stacked-PR auto-rebase you wired in #2137 is dead.

This is the textbook "cross-module silent no-op" pattern: each module's branch derivation is internally consistent — the slice creator uses pipeline.branch, the reconciler uses issue.number — but they disagree on shape. Both compile. Both have passing tests. The wiring dead-ends.

This PR is in the area (pipeline_branch propagation is the bonus fix), so per "pre-existing issues are blocking when the PR modifies the surrounding code," it must be fixed here.

Suggested fix:

issue_branch = f"egg/{contract.contract_key}"

Contract.contract_key already returns the canonical pipeline_id for all three pipeline shapes (issue-driven issue-N, qualified issue-N-vX, JIRA ENG-1234). The two-branch ternary becomes obsolete. Then add a regression test in test_stacked_pr_reconciler* that builds a contract with pipeline_id="issue-N-v3" and asserts the orphan is detected when the head is egg/issue-N-v3/slice-X.

---

NON-BLOCKING

gateway/gateway.py:1084 — regex broader than the documented branch shape

_SLICE_INTEGRATION_BRANCH_RE = re.compile(r"^egg/[A-Za-z0-9][A-Za-z0-9_/-]*/(?:slice|phase)-\d+$")

The character class includes /, so the regex matches egg/foo/bar/baz/slice-1 — multi-segment bases the orchestrator never produces. Not exploitable today (g.session.synthetic is True is the load-bearing check, and synthetic sessions only come from launcher-auth code paths), but the documented shape is egg/<base>/(slice|phase)-N where <base> is one segment. Tighten to:

_SLICE_INTEGRATION_BRANCH_RE = re.compile(r"^egg/[A-Za-z0-9][A-Za-z0-9_-]*/(?:slice|phase)-\d+$")

Drop the / from the second character class. Same defense-in-depth; less surface.

orchestrator/tests/test_slice_run_loop_integration.py:1533 — confusing-looking except syntax

except AttributeError, ValueError:
    pass

This actually parses on Python 3.14 — it's an ExceptHandler(type=Tuple(...)), not the Python-2 except E, name: syntax — but it reads like a parse error. Standard idiom is except (AttributeError, ValueError):. Also worth checking whether the branch is even reachable: setattr on a Pydantic BaseModel raises ValidationError, not AttributeError/ValueError. If this is dead code, drop the try/except entirely. (The same shape exists at _make_pipeline() line 91 — that's the precedent, but precedent doesn't make it less confusing for the next reader.)

orchestrator/tests/test_slice_run_loop_integration.py — test setup duplicated from _make_pipeline()

The new test_qualified_pipeline_branch_propagates_to_slice_branches duplicates the PipelineConfig/Contract boilerplate from _make_pipeline(). Factor a parameterized helper that takes pipeline_id and returns the configured pipeline, so the qualified case is one line.

Dual audit events on one push

A successful slice-integration push emits both push_slice_integration_exempt and push_infrastructure_exempt (with exempt_type="slice_integration_branch"). The PR description frames this as intentional for distinct operator tracing, which is fine — but document it in the audit-log reference (docs/reference/audit-log.md if it exists) or in a comment near the second audit_log call so an operator grepping for push_infrastructure_exempt doesn't conclude this was an infra push.

---

What's right

• synthetic field validation at gateway.py:8113 correctly uses isinstance(..., bool) — defends against truthy ints.
• The exemption is applied before the pipeline-session push block (line 1392) and the push-target enforcement (line 1441), so is_infrastructure_push=True cleanly short-circuits both. File restrictions (1569) and agent restrictions (1774) also correctly skip on this flag.
• The 7 gateway tests cover the matrix that matters: standard issue, qualified, JIRA, legacy phase-N, non-synthetic blocked, synthetic-but-non-slice blocked, and audit event verification.
• The qualifier fix at routes/pipelines.py:10989 is structurally correct — pipeline_branch is the canonical source. The bug it fixes is a real one.

Once the reconciler issue_branch is fixed and there's a regression test for it, this is good to go.

— Authored by egg

[james-in-a-box]

Thanks for the careful review. Pushed 47c9f56 addressing every item.

Disposition

BLOCKING

• orchestrator/stacked_pr_reconciler.py:181 — qualifier dropped, orphan detection silently no-op for qualified pipelines — fixed-in-PR (commit 47c9f56). Replaced the issue_number/pipeline_id ternary with issue_branch = f"egg/{contract.contract_key}" exactly as suggested. Contract.contract_key returns the canonical pipeline id for all three shapes (issue-N, issue-N-v3, ENG-1234), so the reconciler's lookup now matches the producer's branch shape (pipeline.branch propagation at routes/pipelines.py:10989). Added two regression tests in test_stacked_pr_reconciler.py: test_qualified_pipeline_id_preserves_qualifier_in_issue_branch (orphan surfaces under egg/issue-2137-v3/slice-1, intended-new-base falls back to qualified pipeline branch) and test_qualified_pipeline_id_walks_up_to_qualified_ancestor (walk-up resolver also uses the qualified shape).

NON-BLOCKING

• gateway/gateway.py:1084 — regex broader than the documented branch shape — fixed-in-PR (commit 47c9f56). Tightened _SLICE_INTEGRATION_BRANCH_RE to r"^egg/[A-Za-z0-9][A-Za-z0-9_-]*/(?:slice|phase)-\d+$" (dropped / from the second character class). Added test_synthetic_session_multi_segment_base_blocked in test_pipeline_push_block.py to lock the rejection of multi-segment bases the orchestrator never produces.

• orchestrator/tests/test_slice_run_loop_integration.py:1533 — confusing-looking except syntax — fixed-in-PR (commit 47c9f56). The try/except was dead code — PipelineConfig is a Pydantic v2 BaseModel without validate_assignment=True, so plain setattr on these valid values never raises. Replaced both occurrences (the new test and _make_pipeline()'s precedent at line 91) with direct kwarg construction PipelineConfig(concurrent_execution=True, max_concurrent_agents=6, consensus_timeout_minutes=30).

• orchestrator/tests/test_slice_run_loop_integration.py — test setup duplicated from _make_pipeline() — fixed-in-PR (commit 47c9f56). Refactored _make_pipeline to derive branch from pipeline_id (since they're 1:1 — qualified and JIRA shapes alike), so test_qualified_pipeline_branch_propagates_to_slice_branches now collapses to a single _make_pipeline(pipeline_id="issue-2261-v3", issue_number=2261) call.

• Dual audit events on one push — fixed-in-PR (commit 47c9f56). Added an inline comment near the second audit_log call documenting that a successful slice-integration push intentionally emits both push_slice_integration_exempt (orchestrator-specific) and push_infrastructure_exempt with exempt_type="slice_integration_branch" (generic), and that operators grepping for "infra pushes" should filter out the slice variant via exempt_type. No docs/reference/audit-log.md exists today; happy to create one as a follow-up if it would be useful.

— Authored by egg

[egg-reviewer]

Re-review summary

The blocking issue from the prior review — the qualifier-dropping silent no-op in stacked_pr_reconciler.py — is correctly fixed at orchestrator/stacked_pr_reconciler.py:190:

issue_branch = f"egg/{contract.contract_key}"

I verified Contract.contract_key (shared/egg_contracts/models.py:764-775): it returns pipeline_id directly when set (covering qualified issue-N-v3 and JIRA ENG-1234) and synthesises issue-{issue.number} for legacy contracts. The producer side at orchestrator/routes/pipelines.py:10989 (issue_branch = pipeline_branch) and the consumer side now agree on shape for all three documented pipeline kinds. Both regression tests trace correctly:

• test_qualified_pipeline_id_preserves_qualifier_in_issue_branch — slice-2-only contract, walk-up falls back to egg/issue-2137-v3 (qualified pipeline branch), not egg/issue-2137. ✓
• test_qualified_pipeline_id_walks_up_to_qualified_ancestor — 3-level chain, slice-1's qualified branch surfaces from extant_branches. ✓

The other four review items are also resolved:

• Regex tightening (gateway/gateway.py:1086): [A-Za-z0-9_-]* (no /) — the documented single-segment shape, with test_synthetic_session_multi_segment_base_blocked locking the rejection of egg/foo/bar/slice-1.
• except E, F: syntax: removed; replaced with direct PipelineConfig(...) kwargs in both call sites.
• Test setup duplication: _make_pipeline(pipeline_id=..., issue_number=...) collapses the qualified case to one line; branch is now derived from pipeline_id so the helper is correct for qualified and JIRA pipelines too. All 27 existing _make_pipeline() callers retain identical behaviour (default pipeline_id="issue-9999" → branch="egg/issue-9999").
• Dual audit-event documentation: inline comment added at gateway/gateway.py:1348-1356. Wording is slightly tangled — the rationale given (orchestrator-specific path "also visible") really applies to the first event, not the generic one being commented on — but the operator-guidance ("filter via exempt_type") is the practically useful part. Non-blocking.

Non-blocking observations

orchestrator/routes/pipelines.py:10982 — degenerate fallback shape mismatches the reconciler

The slice loop's third fallback computes pipeline_branch = f"egg/{pipeline_id}/work" when both pipeline.branch and pipeline.issue_number are unset. The reconciler would derive issue_branch = f"egg/{contract.contract_key}" = egg/{pipeline_id} (no /work), so producer creates egg/{pipeline_id}/work/slice-N but consumer looks up egg/{pipeline_id}/slice-N — same cross-module silent no-op shape as the bug just fixed.

This path is degenerate (it only fires when neither branch nor issue_number is set on the pipeline, which doesn't happen for issue-driven, qualified, or JIRA pipelines), and the regex would reject the /work/ shape from the synthetic-session exemption path anyway, so the slice-integration push would 403 before it hit the reconciler. Pre-existing and out of scope, but worth flagging as latent technical debt: either the fallback should drop /work, or it should be removed entirely (it's not clear who relies on it).

Comment wording at orchestrator/stacked_pr_reconciler.py:186 — "pre-#2137 split"

The historical reference to "#2137" is a stretch — the bug is the issue-number-based derivation that this PR introduced and removed in the same lifetime, not a pre-#2137 artefact. Re-reading the comment, I'd drop the "pre-#2137" framing and just describe the bug ("the prior f"egg/issue-{issue_number}" ternary hard-coded the unqualified shape"). Cosmetic.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. The PR is approved; I'm following up on the two non-blocking observations.

Disposition

NON-BLOCKING

• orchestrator/routes/pipelines.py:10982 — degenerate fallback shape mismatches the reconciler — deferred-to #2371. Reviewer explicitly framed this as "Pre-existing and out of scope, but worth flagging as latent technical debt," and egg/{x}/work is the broadly-used per-container/per-agent worktree convention (kubernetes_spawner.py, concurrent_executor.py, routes/pipelines.py:14510 & :14867). Disentangling the slice-loop fallback from that convention warrants its own design discussion rather than a drive-by tweak inside this PR — criterion (ii). Tracked in Slice-loop fallback branch shape mismatches reconciler (egg/{pipeline_id}/work vs egg/{pipeline_id}) #2371 with the three options (drop /work, remove the fallback, audit the convention first).

• Comment wording at orchestrator/stacked_pr_reconciler.py:186 — "pre-Independent implement phases #2137 split" — fixed-in-PR (commit cbd60fe). Re-framed the comment to describe the prior f"egg/issue-{issue_number}" ternary directly, dropping the misleading historical reference. Cosmetic; no behaviour change.

— Authored by egg

[egg-reviewer]

Re-review summary

The delta since 47c9f56 is a single commit (cbd60fe) that rewrites the comment block at orchestrator/stacked_pr_reconciler.py:179-189. No behavioural changes; the substantive fixes (synthetic-session exemption in gateway.py, qualifier preservation in routes/pipelines.py, reconciler contract_key-based derivation, regression tests) are unchanged from the approved revision.

The comment fix correctly addresses the cosmetic non-blocking observation I raised:

• Drops the misleading "pre-#2137 split" framing — the issue-number ternary it described was both introduced and removed inside PR #2370, not a pre-#2137 artefact.
• Describes the prior code directly: f"egg/issue-{issue_number}" ternary hard-coded the unqualified shape, silently no-op'ing orphan detection on qualified pipelines.
• Preserves the load-bearing technical claim — that contract_key aligns the consumer's lookup shape with the producer's pipeline.branch propagation — which is what justifies the one-line behavioural fix immediately below.

Re-verified the surrounding logic is still correct: issue_branch = f"egg/{contract.contract_key}" at line 190 returns the canonical pipeline id for all three pipeline shapes (verified against shared/egg_contracts/models.py Contract.contract_key), and the regression tests in test_stacked_pr_reconciler.py exercise the qualified-pipeline path end-to-end.

The non-blocking observation about the egg/{pipeline_id}/work degenerate fallback in routes/pipelines.py:10982 was deferred to #2371 with rationale; that's a fine disposition for a latent issue that doesn't fire in any documented pipeline shape and is gated by the regex anyway.

Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.