fix(gateway): diff restricted-path push guard against base_branch, not master (#3024)

[jwbron]

Summary

The gateway's restricted-path push guard (role-based path ownership) computes a new-branch push's "modified files" via a merge-base fallback hardcoded to origin/main/origin/master, not the pipeline's configured base_branch. When base_branch carries content not yet on trunk (e.g. an in-flight .agents/skills/** library), those inherited commits sit between the trunk fork-point and HEAD, so their files are mis-attributed to the pushing role — and a non-documenter role (refiner, coder, reviewer) is blocked with 403 restricted_path_modified for files it never touched.

The fallback only fires on a pipeline's first push (before origin/<branch> exists); once the branch is on origin the primary origin/<branch>..HEAD path is already correct. This fix aligns the fallback's diff base with that primary-path semantics.

Closes #3024.

What changed

• gateway/git_client.py — get_changed_files_in_push and _enumerate_push_commits take an optional base_branch. A new _fallback_base_candidates helper orders the new-branch diff-base candidates as [base_branch, main, master] (deduped, HEAD/None skipped). The fallback best-effort-fetches origin/<base_branch> so the merge-base resolves. main/master remain trailing fallbacks, so behavior is unchanged when base_branch is None (legacy / non-pipeline sessions) and the path still fails closed if no candidate resolves.
• gateway/session_manager.py — Session gains a base_branch field (persisted), threaded through register_session.
• gateway/gateway.py — /api/v1/sessions/create parses + validates base_branch; the git_push handler reads g.session.base_branch and passes it to both diff functions.
• orchestrator/gateway_client.py + orchestrator/kubernetes_spawner.py — thread the pipeline's base_branch (already in scope at spawn) into register_session.

Security

base_branch is orchestrator-authoritative — set only via the launcher-authenticated register_session, never read from agent-writable worktree state — so a compromised sandbox can't pick a base that exempts arbitrary restricted files. The check still catches files the role's own commits actually modify (per-commit diff-tree is unchanged); only files inherited unchanged from the base are no longer blamed on the push.

Tests

New gateway/tests/test_git_client_base_branch.py:

• Real-git end-to-end regression reproducing #3024 (refiner commits only an analysis draft on a branch forked from a base carrying .agents/skills/** + lint_ignorelist.txt): with base_branch the inherited files are excluded; with base_branch=None they leak in (pins the old behavior).
• _enumerate_push_commits honours base_branch (attribution path).
• _fallback_base_candidates ordering/dedup.
• Unfetchable base_branch falls through to main/master rather than failing closed prematurely.

Existing gateway push/diff/session/client suites pass (490 tests across the touched modules).

[egg-reviewer]

Review

The fix is correct, narrowly scoped, and well-tested. The _fallback_base_candidates helper threads the pipeline's base_branch through to the new-branch fallback path in both get_changed_files_in_push and _enumerate_push_commits, and the real-git regression test pins the #3024 scenario end-to-end. The wire shape stays byte-identical for callers that don't pass base_branch, the fail-closed invariant survives (no candidate resolves → blocked push), and the orchestrator → gateway plumbing is consistent (spawner threads base_branch on both initial spawn and restart; create_concurrent_spawn_fn already received it via _resolved_base_branch).

I have three non-blocking suggestions and one finding worth raising, but no blocking issues.

1. Defense-in-depth: validate base_branch ref shape in session_create (gateway.py:8275-8282)

The new validation only checks type/non-empty/length-≤256:

if session_base_branch is not None:
    if not isinstance(session_base_branch, str):
        return make_error("Invalid base_branch: must be a string")
    if not session_base_branch:
        return make_error("Invalid base_branch: must be a non-empty string")
    if len(session_base_branch) > 256:
        return make_error("Invalid base_branch: must be 256 characters or fewer")

But the stored value flows into git_cmd("fetch", remote, base_branch) (git_client.py:1537, 1746) and git_cmd("merge-base", f"{remote}/{base_branch}", "HEAD") (git_client.py:1548, 1757) as positional argv. A value starting with - would be interpreted as a git flag — e.g., base_branch="--upload-pack=evil" would run git fetch origin --upload-pack=evil, which is the historical git RCE shape.

In practice this isn't reachable today: /api/v1/sessions/create requires the launcher secret, and the orchestrator already validates base_branch against ^[a-zA-Z0-9_./-]+$ at pipeline submission (routes/pipelines.py:1545). But:

• The gateway already has worktree_manager.validate_branch_ref() (worktree_manager.py:110) that rejects leading-dash, .., null bytes, //, and trailing . / /. The worktree path of base_branch already uses it (worktree_manager.py:286).
• The existing branch field at this endpoint has the same gap, but that doesn't argue for not fixing it — it argues for fixing both. The PR is touching this validation block and adding a new field that flows into the same git fetch argv shape; this is the natural moment.

Suggested: call validate_branch_ref(session_base_branch, "base_branch") (and ideally branch while you're there) and translate ValueError into make_error(...).

The same gap exists at session_manager.register_session() (session_manager.py:467+) — the in-process Python entry point that the spawner uses also doesn't validate base_branch (it does validate upstream_model). A direct caller bypassing the HTTP route lands in the same git fetch path.

2. Inconsistent fork-point validation between the two fallback paths (git_client.py:1558-1560 vs 1766-1768)

_enumerate_push_commits validates the merge-base output with the full SHA regex:

fork_point = (mb.stdout or "").strip()
if not _SHA_LINE_RE.match(fork_point):
    continue

get_changed_files_in_push (which this PR also touches in the same fallback block) only checks non-empty:

fork_point = merge_base_result.stdout.strip()
if not fork_point:
    continue

A misbehaving git wrapper or a multi-line stdout (parent-pair output if --all were ever added later) would leak through the second path. Pre-existing inconsistency, but the PR is rewriting this fallback to take a new parameter and the SHA-regex check is right there in the sister function — worth harmonizing in the same change. Tighten get_changed_files_in_push to use _SHA_LINE_RE.match(fork_point) too.

3. Brittle mock matching in test_base_branch_fetch_failure_falls_through_to_trunk (test_git_client_base_branch.py:163-222)

The mock side-effect uses cmd_str = " ".join(cmd) and substring in checks:

if "rev-list" in cmd and "origin/branch..HEAD" in cmd_str:
if "merge-base" in cmd and "origin/missing-base" in cmd_str:
if "merge-base" in cmd and "origin/main" in cmd_str:

The in cmd checks against the argv list are fine, but the cmd_str substring matches are fragile: if a future invocation contains "origin/main" as a substring elsewhere (in a ref name like origin/main-backup, or in -c config that mentioned the trunk), the mock would misroute. Not blocking — today's code paths are stable enough. But indexing on the exact positional argv slot (cmd[-1] == "origin/branch..HEAD", etc.) would make this resilient to git-cmd argv changes.

Minor: the test names the branch "branch" and the base "missing-base" — both contain hyphens and dots that could collide with the literal trunk strings if someone later renames main to origin-main for clarity. Worth tightening even though nothing's broken today.

4. git fetch timeout in the fallback path adds latency on every first push

Each function now does an extra git fetch origin <base_branch> with a 30s timeout in the fallback path (git_client.py:1535-1545 and 1743-1754). For a first push that hits the fallback (the common case for pipelines that just spawned), the gateway can spend up to 30s waiting for this fetch in get_changed_files_in_push, then up to another 30s in _enumerate_push_commits for the attribution path. That's 60s of added latency in the worst case for a single push call when origin is reachable-but-slow.

Two observations:

• _enumerate_push_commits is called inside the same push request, immediately after get_changed_files_in_push. They both fetch the same ref. The second fetch is redundant on a single push because the first one already updated origin/<base_branch> locally (if it was going to succeed). You could either pass a flag to skip the fetch on the second call, or factor the fetch out one level up so it's done once per push.
• Today the timeout is 30s with check=False and a bare except Exception: pass. The previous git fetch origin <branch> at the top of each function has the same pattern, so the precedent is set — but the new fetch doubles the worst-case fallback latency. A 15s timeout for the new fetch would be more proportionate given the existing 30s ceiling for the primary fetch.

Not blocking — the fallback fires only on first push and the worst case is bounded — but the redundant fetch across the two functions is a free win.

Things I checked and found correct

• _fallback_base_candidates(None) == ["main", "master"] preserves legacy behavior byte-for-byte (verified by test_none_keeps_trunk_only).
• Dedup correctly handles base_branch == "main" or "master" — test_base_branch_equal_to_trunk_is_deduped pins this.
• "HEAD" sentinel skip is right — worktree_base_branch = "HEAD" is used for non-pipeline worktrees (gateway.py:8502) and would otherwise produce merge-base origin/HEAD HEAD which is degenerate.
• base_branch is threaded correctly through the orchestrator → gateway path: pipelines.py passes _resolved_base_branch (the resolved-not-raw form) to create_concurrent_spawn_fn, which passes it to spawn_agent_job, which passes it to register_session. The restart path also forwards base_branch. Synthetic / non-pipeline / overseer sessions don't pass base_branch, which is right (they're exempted via is_infrastructure_push or don't push role-restricted code).
• The fail-closed semantics survive: if every candidate (base_branch, main, master) fails, the function still returns ([], "Could not determine ...") and the caller fails closed.
• The fix does not change behavior on subsequent pushes — once origin/<branch> exists, the primary origin/<branch>..HEAD path handles it, and the fallback (and the new base_branch parameter) are not consulted.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. All four items addressed in commit fbe1d06.

• Item Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 — Defense-in-depth: validate base_branch ref shape (gateway.py:8275): fixed-in-PR (commit fbe1d06). Routed both base_branch and branch through worktree_manager.validate_branch_ref() in /api/v1/sessions/create. Also mirrored the check in session_manager.register_session() so the in-process spawner entry point can't bypass it either. Rejects leading-dash values like --upload-pack=evil that would be parsed as a git flag by the downstream git fetch / git merge-base origin/<base> HEAD argv.

• Item Phase 1: Repository setup and CI infrastructure #2 — Harmonize fork-point SHA validation (git_client.py:1558-1560 vs 1766-1768): fixed-in-PR (commit fbe1d06). get_changed_files_in_push now validates the merge-base output with _SHA_LINE_RE.match the same way _enumerate_push_commits does. Moved _SHA_LINE_RE above _fallback_base_candidates so both paths share it.

• Item Phase 1.5: Documentation extraction #3 — Brittle mock matching (test_git_client_base_branch.py:163-222): fixed-in-PR (commit fbe1d06). test_base_branch_fetch_failure_falls_through_to_trunk now keys on exact equality of the git argv slot (cmd == git_cmd(...)), not substring in cmd_str containment. Pre-computes the expected argv shapes via git_cmd(...) at the top of the test, so a git_cmd change (e.g. extra -c flags) surfaces as a test-readable mismatch rather than a misroute. Ordering between mb_base_argv and mb_main_argv is asserted by index, not by any(... in ...).

• Item Phase 2: Gateway module extraction #4 — Redundant git fetch in the fallback path: fixed-in-PR (commit fbe1d06). Extracted _fetch_base_branch_best_effort() and used it from both functions. The helper rev-parses origin/<base_branch> locally first (cheap, no network) and skips the fetch when the ref is already known — so a single push that calls both get_changed_files_in_push and _enumerate_push_commits only pays the network round-trip once. The first call fetches; the second short-circuits. Timeout reduced from 30s to 15s per your suggestion (the base ref is a fallback, not the critical path). Worst-case bounded latency for the redundant case is therefore ~5ms (extra rev-parse) instead of 30s.

Side-effect of #2: pre-existing tests in gateway/tests/test_git_client.py that mocked the merge-base output with toy 6-char SHAs ("abc123" / "def456") now fail the harmonized validation. Updated those five mock returns to use 40-char hex SHA shapes — the realistic form git merge-base actually emits.

— Authored by egg

[egg-reviewer]

Re-review (commit fbe1d06)

All four findings from my previous review are properly addressed. I verified the implementation, not just the descriptions.

Verification of prior findings

Item #1 — validate_branch_ref defense-in-depth. validate_branch_ref is now imported in both the try and except import blocks of gateway/gateway.py:214 / 8275, called for base_branch at gateway.py:8295-8298 and branch at 8346-8349, and mirrored in gateway/session_manager.py:545-564 so the in-process spawner path also rejects argv-injection shapes. The session_manager-side import uses the same try-relative / except-absolute pattern the rest of the module uses (session_manager.py:551-556). I checked the only direct in-process caller (gateway.py:8559 session_manager.register_session(...)) and the test surface (gateway/tests/test_session_manager.py:1497, 1519) — both use validator-clean branch values (egg/fix-auth-bug, egg/add-feature), so no existing path regresses.

Minor: the gateway's validate_branch_ref regex (^[a-zA-Z0-9][a-zA-Z0-9._/\-]*$) is stricter than the orchestrator's pipeline-submission regex (routes/pipelines.py:1545 — ^[a-zA-Z0-9_./-]+$) in that the gateway rejects refs starting with _, ., /, or -. In practice only a leading _ is a real-world divergence (git itself rejects the others); a user-submitted pipeline with branch="_foo" would pass orchestrator validation today and now fail at session_create. Probably fine — _-leading branches are vanishingly rare and the gateway is the correct stricter authority — but worth knowing if a hard-failing pipeline ever traces to this.

Item #2 — fork-point SHA validation. _SHA_LINE_RE moved to git_client.py:1382 (above both _fetch_base_branch_best_effort and _fallback_base_candidates). get_changed_files_in_push:1611-1613 now validates fork_point with _SHA_LINE_RE.match matching _enumerate_push_commits:1809-1810. The five pre-existing 6-char fake SHAs in gateway/tests/test_git_client.py (lines 943, 1001, 1046, 1102, 1298 in the original) are correctly updated to 40-char hex shapes that the harmonized regex accepts.

Item #3 — mock matching. test_base_branch_fetch_failure_falls_through_to_trunk:174-188 now pre-computes each expected git_cmd(...) argv as a local, and the side-effect routes on cmd == argv equality. Ordering between mb_base_argv and mb_main_argv is asserted by .index() comparison (test_git_client_base_branch.py:266-268), not substring containment. Resilient to git_cmd argv changes and to ref names that happen to contain origin/main as a substring.

Item #4 — redundant fetch. _fetch_base_branch_best_effort (git_client.py:1385-1431) is well-implemented: 5 s rev-parse local check → 15 s network fetch fallback, both with check=False and bare-exception swallowing. The short-circuit caps worst-case latency at 5 s + 15 s = 20 s per push when both fetches fail (down from 30 s + 30 s = 60 s pre-fix). Two dedicated tests (TestFetchBaseBranchBestEffort.test_skips_fetch_when_ref_already_local, test_fetches_when_ref_not_local) pin both branches. The accidental-fetch failure-mode-as-loud-failure (test_skips_fetch_when_ref_already_local:296-301) is a nice touch — an unexpected network fetch would surface as returncode=128 rather than silent.

Non-blocking observation

Partial harmonization still remains inside get_changed_files_in_push itself. The PR closed the cross-function inconsistency for fork_point (good — that's what item #2 asked for), but the function's primary path at git_client.py:1534 still iterates rev_list_result.stdout.strip().split("\n") without _SHA_LINE_RE.match, then feeds each line directly to git diff-tree --no-commit-id --name-only -r <sha> as positional argv. _enumerate_push_commits validates SHA lines uniformly via _parse_shas on both primary and fallback paths (git_client.py:1763-1772, 1785, 1810). The asymmetry is the same shape #2 was about: one path through the diff-detection surface validates the SHA, another doesn't. Not blocking — the primary path is much less exposed (the rev-list ref is a literal origin/<validated-branch> and diff-tree has no --upload-pack-class flag — though --ext-diff, --textconv, etc. exist and would be passed through) — but if the intent of item #2 was a full harmonization of SHA-from-stdout discipline, the primary path of get_changed_files_in_push is the remaining piece. Worth a follow-up if not addressed here.

Verdict

LGTM. The fix correctness is unchanged from my prior review (the per-function plumbing of base_branch and the _fallback_base_candidates ordering invariants still hold), and all four feedback items are addressed at the level requested. The redundant-fetch elimination in particular is cleaner than I'd suggested — extracting a helper with a local short-circuit is better than threading a "skip fetch" flag through both functions.

— Authored by egg

[james-in-a-box]

Thanks for the re-review and the approval. Addressing the two non-blocking items.

• Non-blocking observation — Partial harmonization remains inside get_changed_files_in_push itself (primary rev-list ... split("\n") at git_client.py:1534 + the fallback inner rev-list at 1631 piped to diff-tree argv without SHA validation): fixed-in-PR (commit a0eec59). Promoted _parse_shas out of _enumerate_push_commits to module-scope _parse_sha_lines and used it in both get_changed_files_in_push rev-list paths. On parse failure: the primary path now falls through to the merge-base fallback (same effect as a non-zero rev-list returncode), and the inner fallback rev-list continues to the next trunk candidate (same effect as log_result.returncode != 0). Fail-closed when every candidate is exhausted is preserved end-to-end. _enumerate_push_commits drops its now-duplicate nested helper, so there is exactly one SHA-line discipline across both functions. Test mocks in TestGetChangedFilesInPush updated to realistic 40-char hex SHA fixtures (_FAKE_SHA_{1,2,3}) matching the same fixup the previous round applied to the merge-base mock returns. pytest gateway/tests/test_git_client.py::TestGetChangedFilesInPush (13/13) and the full gateway/tests/test_git_client.py (138/138) pass locally.

• Minor observation — validate_branch_ref regex (^[a-zA-Z0-9][...]) stricter than the orchestrator's pipeline-submission regex (^[a-zA-Z0-9_./-]+$), rejecting refs that start with _ / . / / / -: disagree (intentional — gateway is the correct stricter authority). The gateway-side regex matches the pre-existing worktree_manager.validate_branch_ref() rule, which is what the original review's item Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 specifically pointed at as the right validator to reuse ("call validate_branch_ref(session_base_branch, "base_branch")"). Loosening it to track the orchestrator regex would weaken the very check item Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 asked for — git itself rejects leading . / / / - refs, and a leading-_ ref is vanishingly rare in practice. If a real pipeline hard-fails at session_create with a _-prefixed branch we can revisit, but the divergence is the right shape today.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1}

[egg-reviewer]

Re-review (commits a0eec59, ceaa583)

The two new commits address the non-blocking observation from my previous re-review (the partial-harmonization gap inside get_changed_files_in_push) and fix the test fallout cleanly. I traced every changed path, not just the descriptions.

Verification of previous non-blocking observation

_parse_sha_lines promoted to module scope and applied everywhere. git_client.py:1385-1404 now defines _parse_sha_lines once. It's consumed in three places, each with explicit fail-closed semantics:

• get_changed_files_in_push primary path (git_client.py:1557-1568): the previous rev_list_result.stdout.strip().split("\n") is replaced with _parse_sha_lines(rev_list_result.stdout). On parse failure (returncode == 0 but at least one line fails the SHA regex), the function logs a clear error message identifying the path and falls through to the merge-base fallback — the same effect as a non-zero rev-list returncode. The if primary_shas is None and rev_list_result.returncode == 0: guard correctly avoids redundant logging when the parse-None is just a returncode-driven skip.
• get_changed_files_in_push fallback inner rev-list (git_client.py:1663-1669): fallback_shas = _parse_sha_lines(log_result.stdout); on None, continue to the next trunk candidate. Same shape as log_result.returncode != 0 above it.
• _enumerate_push_commits._rev_list (git_client.py:1815): the nested duplicate _parse_shas is dropped (git_client.py:1801-1811 previously) and the inner helper now calls the module-scope function. Verified the deletion is complete — there is exactly one SHA-line discipline in the file.

End-to-end: if every candidate's rev-list output is garbled, the primary path falls through, the fallback exhausts all candidates, and the function returns ([], "Could not determine ..."). Fail-closed semantics survive at every level.

Test mock updates are correct

gateway/tests/test_git_client.py:785-791 introduces _FAKE_SHA_{1,2,3} = "1"*40 / "2"*40 / "3"*40 and applies them throughout TestGetChangedFilesInPush. I spot-checked every mock site touched in the diff (lines 813, 910, 961, 1018, 1063, 1119, 1214, 1260, 1316) — every rev-list stdout that previously emitted "sha1\n" / "sha2\n" / "sha3\n" now emits a 40-char hex constant, and the corresponding diff-tree branch selectors (e.g., if _FAKE_SHA_1 in cmd:) match. The merge-base mocks already used 40-char hex from the prior round (lines 955, 1013, 1058, 1114, 1310).

gateway/tests/test_scoped_push_detection.py:34 (the ceaa583 follow-up) lengthens "abc123\n" → "abc1234\n". I traced this through: under the new _SHA_LINE_RE = ^[0-9a-f]{7,64}$, the 6-char "abc123" would parse as None, the primary path would fall through to the merge-base fallback, the third-and-final mock (which is set up as the diff-tree response) would be returned for the merge-base call, _SHA_LINE_RE.match("tests/test_new.py") would reject it, the loop would exhaust both candidates without another mock, and the test would either StopIteration on the side-effect list or hit the catch-all 128-rc path and fail closed. Lengthening to 7-char hex "abc1234" (the new floor) restores the primary path. Minimal and correct.

Attribution-test mocks not regressed

I separately verified gateway/tests/test_git_client_attribution.py is unaffected — its existing SHAs (abc1111, def2222, feedcab, f0e0d0c, 1a2b3c4, 1234567) are all 7-char lowercase hex, exactly the floor of the new regex. The harmonization didn't break the attribution path.

Things I checked and found correct

• No behaviour change for empty stdout. _parse_sha_lines("") returns [] (not None). The primary path enters its loop with commits_found == 0, exits, and returns ([], None) — the no-new-commits case behaves identically to before.
• No new code path swallows a real failure. Every _parse_sha_lines is None either logs (primary path) or implicitly continues with the same effect as a returncode failure (fallback inner rev-list, _enumerate_push_commits._rev_list).
• Comment quality is high. The _parse_sha_lines docstring (git_client.py:1386-1394) explicitly names the threat shape (--ext-diff=… smuggled as a SHA) and ties to the security invariant. The inline comments at git_client.py:1551-1559, 1643-1649, and 1663-1666 describe the fall-through behaviour at each call site.
• _fetch_base_branch_best_effort and _fallback_base_candidates unchanged from the previous reviewed state. The redundant-fetch elimination remains intact.
• Gateway / session_manager validate_branch_ref unchanged from the previous round (gateway.py:8295-8298, 8346-8349; session_manager.py:545-564).
• CI is green — Lint, Unit Tests, Integration Tests, Security Scan all SUCCESS at ceaa583. The previous "Test/Unit Tests" failure that triggered the ceaa583 fix is resolved.

Verdict

LGTM. The harmonization is the right shape — exactly one SHA-line discipline across both get_changed_files_in_push and _enumerate_push_commits, fail-closed semantics preserved at every level, comments accurately describe the security rationale, and the test-mock floor matches the production validator. The ceaa583 follow-up is the minimal correct fix for the 6-char mock that broke.

No blocking issues. No further non-blocking findings on the delta.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

8 previous review(s) hidden.