Fix #2398: share one synthetic session across slice integration branch creation

[jwbron]

Summary

• orchestrator.gateway_client.create_slice_integration_branch was making three sequential gateway calls — fetch_branch, get_remote_branch_sha, and the push — each of which registered and deleted its own synthetic session. The reviewer on Fix #2393: push slice integration branch by SHA, not parent ref name #2395 flagged this: for a 15-slice pipeline that's 45 session lifecycles where one would do.
• Add an optional keyword-only bearer_token to fetch_branch and get_remote_branch_sha: when supplied, they skip the per-call register/delete and authenticate with the caller's token. Default None preserves every other call site (state_store.py, routes/pipelines.py).
• In create_slice_integration_branch, move register_session ahead of the fetch/ls-remote/push and pass that token to all three. The push's session metadata (branch=integration_branch, agent_role, synthetic=True) is the strictest of the three, and the fetch + ls-remote endpoints accept any synthetic session, so the extra metadata is harmless there.
• 15-slice pipelines now register 15 sessions instead of 45 (3x fewer round-trips). Per the issue, if the TOCTOU follow-up later removes the ls-remote round-trip, this still buys 2x.

Why follow-up, not in PR #2395

Touches fetch_branch / get_remote_branch_sha signatures used by other call sites. Perf optimization, not a correctness fix — out of scope for the #2393 hotfix.

Test plan

• pytest orchestrator/tests/test_create_slice_integration_branch.py — 10 tests pass (8 existing + 2 new)
• pytest orchestrator/tests/test_state_store.py orchestrator/tests/test_pipelines_api.py orchestrator/tests/test_source_branch.py orchestrator/tests/test_slice_run_loop_integration.py — 259 pass; existing call sites of fetch_branch / get_remote_branch_sha unaffected by the new keyword-only param
• make test — 3703 passed
• make lint
• Re-run a multi-slice pipeline end-to-end and confirm the gateway audit log shows 1 synthetic-session register per slice integration-branch creation (down from 3)

Closes #2398.

[egg-reviewer]

Reviewed gateway_client.py and test_create_slice_integration_branch.py. Cross-checked the four other call sites of fetch_branch / get_remote_branch_sha (state_store.py:952, routes/pipelines.py:1473/1478/4148/4337) and the gateway side of fetch/ls-remote/push routes. The refactor is correct and well-bounded.

Correctness

• The owns_session = bearer_token is None flag drives both the conditional register_session and the conditional delete_session in finally — every path (success, parent-missing return, mid-flow exception, fetch-best-effort failure) cleans up exactly when it should.
• fetch_branch and get_remote_branch_sha correctly skip both register and delete when a bearer_token is supplied; create_slice_integration_branch owns the session lifecycle for the whole sequence and tears it down in its own finally.
• The new test test_session_cleaned_up_when_parent_missing is a good catch — the refactor moves register_session ahead of the ls-remote SHA lookup, so the missing-parent return path now has a session to clean up where it previously had none.

Backward compatibility

bearer_token is keyword-only with a None default, and all four existing callers pass everything by keyword (pipeline_id=, repo_path=, args=, mode=), so adding *, bearer_token=None after mode doesn't disturb them. They continue to take the per-call register/delete path — verified.

Security / metadata leakage

The session is registered with branch=integration_branch and agent_role=coder because the push needs both for the _SLICE_INTEGRATION_BRANCH_RE exemption (gateway/gateway.py:1308–1318) and for branch == session.assigned_branch (line 1455–1476). Reusing that session for the fetch + ls-remote is safe: /api/v1/git/fetch (which serves both operation=fetch and operation=ls-remote, line 2854–3057) only enforces @require_session_auth + path/args validation + private-mode policy. It never inspects assigned_branch or agent_role. The PR description's claim that "the extra metadata is harmless for those calls" holds against the route handler.

Session lifetime

DEFAULT_SESSION_TTL_HOURS = 24 (gateway/session_manager.py:243); the worst-case window for fetch (120s timeout) + ls-remote (120s) + push is well under that. Sharing one session across the three calls is not at risk of mid-flow expiry.

Non-blocking notes

1. gateway_client.py:1956 — the fetch_branch docstring still says pipeline_id: Pipeline ID (used as container_id for the temp session). When bearer_token is supplied, that's not literally true — the synthetic container_id from the upstream caller is what's used, and pipeline_id is only consumed for log fields. Worth a one-line clarification so future readers don't misread the contract.
2. gateway_client.py:1957 and :2103 — the mode parameter is silently ignored when bearer_token is supplied (the session's mode is already established at register time). Not a bug, but a one-line note in the docstring would prevent a future caller passing mode="private" and assuming it's been honored on a borrowed session.
3. test_create_slice_integration_branch.py:385 and :419 — assert delete_spy.call_args_list == [((("shared-tok"),), {})] is parseable but unusual; from unittest.mock import call and [call("shared-tok")] would read more directly. Style only.

Test coverage

The two new tests pin (a) the one-session-shared invariant including bearer-token forwarding to all three downstream calls, and (b) cleanup on the missing-parent early-return. Both exercise the production code path through create_slice_integration_branch.

LGTM.

— Authored by egg

[james-in-a-box]

Thanks for the LGTM and the three non-blocking notes — addressed all of them in ebe86c1.

• Note 1 (fetch_branch docstring on pipeline_id no longer literally being the temp-session container_id when bearer_token is supplied) — fixed-in-PR (commit ebe86c1). Docstring now says pipeline_id is "used as container_id for the temp session and for log fields. When bearer_token is supplied no session is registered, so it's only used for log fields in that case."
• Note 2 (mode parameter silently ignored on a borrowed session in both fetch_branch and get_remote_branch_sha) — fixed-in-PR (commit ebe86c1). Both docstrings now call out that mode is ignored when bearer_token is supplied because the supplied session's mode was fixed at its register_session time.
• Note 3 (delete_spy.call_args_list == [(((\"shared-tok\"),), {})] is parseable but unusual; prefer from unittest.mock import call and [call(\"shared-tok\")]) — fixed-in-PR (commit ebe86c1). Both assertions in test_create_slice_integration_branch.py now use call(...).

pytest orchestrator/tests/test_create_slice_integration_branch.py — 10/10 pass. ruff check and ruff format --check clean on both files.

— Authored by egg

[egg-reviewer]

Re-review at ebe86c1. Delta since my prior review is purely the three non-blocking notes from the previous LGTM, addressed verbatim:

• Note 1 — fetch_branch docstring on pipeline_id (gateway_client.py:1956–1960). Now clarifies that pipeline_id is used both as container_id for the temp session and for log fields, and only the latter applies when bearer_token is supplied. Matches the implementation: pipeline_id flows into temp_container_id inside the if owns_session: branch (skipped on borrowed session) and into the unconditional logger.info/logger.warning calls. ✓
• Note 2 — mode ignored on borrowed session (gateway_client.py:1963–1965 and :2107–2109). Both docstrings now state that mode is ignored when bearer_token is supplied because the supplied session's mode was fixed at register time. Verified: mode is only consumed inside if owns_session: (passed to register_session); when a token is supplied that branch is skipped. ✓
• Note 3 — delete_spy.call_args_list uses mock.call() (test_create_slice_integration_branch.py:21, :385, :419). Import added; both assertions rewritten as [call("shared-tok")] / [call("orphan-tok")]. Sanity-checked against MagicMock directly — [call("shared-tok")] == [((("shared-tok"),), {})] is True, so the new form is semantically identical and just more readable. ✓

No new logic, no new code paths. Correctness/security/backward-compat analysis from the prior review still holds:

• Single shared synthetic session across fetch + ls-remote + push, owned by create_slice_integration_branch.
• Backward-compat preserved by the keyword-only bearer_token=None parameter; existing callers (state_store.py, routes/pipelines.py) take the original register/delete path.
• Session metadata (branch=integration_branch, agent_role=coder) is the strictest of the three; the /api/v1/git/fetch route doesn't inspect assigned_branch or agent_role, so reusing the push session for fetch + ls-remote is safe.

LGTM.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.