From ac1d1cb2fed8a0479a62e904bc13daaa4677bf08 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Mon, 8 Jun 2026 16:00:37 -0700 Subject: [PATCH 1/4] fix(gateway): diff restricted-path push guard against base_branch, not master (#3024) The gateway's restricted-path push check enumerates a new-branch push's modified files via a merge-base fallback hardcoded to origin/main|master. When a pipeline's base_branch carries content not yet on trunk (e.g. an in-flight skills library), those inherited commits fall 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 from pushing files it never touched. Thread the pipeline's configured base_branch (orchestrator-authoritative, never agent-settable) onto the gateway Session and use it as the preferred diff base in get_changed_files_in_push / _enumerate_push_commits. main and master remain trailing fallbacks for legacy/non-pipeline sessions and as a safety net when the base ref can't be resolved. Behavior is unchanged when base_branch is None. Closes #3024 --- gateway/gateway.py | 34 ++- gateway/git_client.py | 92 +++++++- gateway/session_manager.py | 15 ++ gateway/tests/test_git_client_base_branch.py | 222 +++++++++++++++++++ orchestrator/gateway_client.py | 11 + orchestrator/kubernetes_spawner.py | 5 + 6 files changed, 365 insertions(+), 14 deletions(-) create mode 100644 gateway/tests/test_git_client_base_branch.py diff --git a/gateway/gateway.py b/gateway/gateway.py index e0249b123f..d692dde117 100644 --- a/gateway/gateway.py +++ b/gateway/gateway.py @@ -1511,15 +1511,22 @@ def git_push() -> tuple[Response, int] | Response: # The downstream anchor/phase/agent-restriction checks already gate on # `not is_infrastructure_push`; this gate makes the role check symmetric. session_role = None + # Pipeline base branch (#3024): used as the preferred diff base for the + # new-branch fallback so a branch forked from a non-trunk base is not + # blamed for files it inherited unchanged from that base. + session_base_branch = None changed_files = None # populated below; reused by attribution + phase checks if hasattr(g, "session") and g.session: session_role = getattr(g.session, "agent_role", None) + session_base_branch = getattr(g.session, "base_branch", None) if session_role and not is_infrastructure_push: # Get the list of files being pushed for downstream attribution-aware # role enforcement (the canonical agent-role check below) and the # phase-restriction check further down. - changed_files, check_error = get_changed_files_in_push(exec_path, remote, branch) + changed_files, check_error = get_changed_files_in_push( + exec_path, remote, branch, base_branch=session_base_branch + ) # SECURITY: Fail closed - if we can't determine changed files, block the push. # This prevents bypass via git diff manipulation (timeout, corrupt refs, etc.) @@ -1613,7 +1620,11 @@ def git_push() -> tuple[Response, int] | Response: # Resolve attribution for every commit in the push range. try: attributed_push = _get_attributed_fn( - exec_path, remote, branch, session_role=session_role + exec_path, + remote, + branch, + session_role=session_role, + base_branch=session_base_branch, ) except Exception as exc: logger.warning("attribution_lookup_exception", error=str(exc), exc_info=True) @@ -1830,7 +1841,9 @@ def git_push() -> tuple[Response, int] | Response: if session_phase and not is_infrastructure_push: # Get the list of files being pushed (reuse if already fetched for role check) if changed_files is None: - changed_files, check_error = get_changed_files_in_push(exec_path, remote, branch) + changed_files, check_error = get_changed_files_in_push( + exec_path, remote, branch, base_branch=session_base_branch + ) if check_error: audit_log( "push_denied_file_check_failed", @@ -8214,6 +8227,11 @@ def session_create() -> tuple[Response, int] | Response: agent_anchor_id = data.get("agent_anchor_id") # Optional agent anchor ID claude_code_version = data.get("claude_code_version") # Optional Claude Code version branch = data.get("branch") # Optional git branch for non-pushing sessions + # Optional pipeline base branch (PR base). Stored on the session and used + # as the preferred diff base for the new-branch restricted-path push check + # (#3024). Distinct from the locally-derived ``worktree_base_branch`` below, + # which selects the ref a fresh worktree forks from. + session_base_branch = data.get("base_branch") jira_ticket = data.get("jira_ticket") # Optional Atlassian ticket key — advisory only synthetic = data.get("synthetic", False) # Orchestrator-internal temp session # Per-session upstream routing (issue #2769). Default to "anthropic" so @@ -8254,6 +8272,15 @@ def session_create() -> tuple[Response, int] | Response: if len(pipeline_id) > 256: return make_error("Invalid pipeline_id: must be 256 characters or fewer") + # Validate base_branch if provided (#3024) + 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") + # Validate issue_number if provided if issue_number is not None and (not isinstance(issue_number, int) or issue_number < 1): return make_error("Invalid issue_number: must be a positive integer") @@ -8516,6 +8543,7 @@ def session_create() -> tuple[Response, int] | Response: agent_anchor_id=agent_anchor_id, claude_code_version=claude_code_version, branch=branch, + base_branch=session_base_branch, jira_ticket=jira_ticket if isinstance(jira_ticket, str) and jira_ticket else None, synthetic=synthetic, upstream=upstream, diff --git a/gateway/git_client.py b/gateway/git_client.py index d5bdb25509..412669f71f 100644 --- a/gateway/git_client.py +++ b/gateway/git_client.py @@ -1375,8 +1375,31 @@ def cleanup_credential_helper(path: str | None) -> None: # ============================================================================= +def _fallback_base_candidates(base_branch: str | None) -> list[str]: + """Ordered, de-duplicated diff-base candidates for new-branch pushes. + + Used only by the merge-base fallback that fires when ``origin/`` + does not yet exist (a pipeline's first push). The pipeline's configured + ``base_branch`` is tried first: a branch forked from a non-trunk base + carries that base's content, and diffing against trunk (``main``/ + ``master``) instead would attribute every file inherited unchanged from + the base to the pushing role — falsely blocking non-documenter roles on + bases that legitimately carry documenter-owned files (#3024). ``main`` + and ``master`` remain as trailing candidates so sessions with no + ``base_branch`` (legacy / non-pipeline) keep today's behavior, and so the + merge-base still resolves if the configured base ref can't be fetched. + """ + candidates: list[str] = [] + if base_branch and base_branch != "HEAD": + candidates.append(base_branch) + for trunk in ("main", "master"): + if trunk not in candidates: + candidates.append(trunk) + return candidates + + def get_changed_files_in_push( - repo_path: str, remote: str, branch: str + repo_path: str, remote: str, branch: str, base_branch: str | None = None ) -> tuple[list[str], str | None]: """ Get the list of files that would be changed by a push. @@ -1396,6 +1419,11 @@ def get_changed_files_in_push( repo_path: Path to the git repository remote: Remote name (e.g., "origin") branch: Branch name being pushed + base_branch: The pipeline's configured base branch (PR base). Used as + the preferred diff base for the new-branch merge-base fallback so a + branch forked from a non-trunk base is not blamed for files it + inherited unchanged from that base (#3024). ``None`` (legacy / + non-pipeline sessions) falls back to ``main``/``master`` as before. Returns: Tuple of (changed_files, error_message) @@ -1494,7 +1522,28 @@ def get_changed_files_in_push( # If remote branch doesn't exist yet, use per-commit file detection via # merge-base + diff-tree. This avoids false positives from inherited # differences between worktree branches and origin/main (Bug #1239). - for default_branch in ("main", "master"): + # + # Diff against the pipeline's configured base_branch first (#3024): a + # branch forked from a non-trunk base carries that base's content, and + # diffing against trunk would mis-attribute those inherited files to the + # pushing role. main/master remain trailing fallbacks. + fallback_bases = _fallback_base_candidates(base_branch) + if base_branch and base_branch != "HEAD": + # Best-effort fetch so origin/ resolves for the + # merge-base below — the top-of-function fetch only refreshed the + # pushed branch. main/master are assumed already present locally. + try: + subprocess.run( + git_cmd("fetch", remote, base_branch), + cwd=repo_path, + capture_output=True, + text=True, + timeout=30, + check=False, + ) + except Exception: + pass # merge-base will fall through to the next candidate + for default_branch in fallback_bases: merge_base_result = subprocess.run( git_cmd("merge-base", f"{remote}/{default_branch}", "HEAD"), cwd=repo_path, @@ -1632,17 +1681,19 @@ class AttributedPushRange: def _enumerate_push_commits( - repo_path: str, remote: str, branch: str + repo_path: str, remote: str, branch: str, base_branch: str | None = None ) -> tuple[list[str], str | None]: """Return (commits_oldest_first, error). Mirrors ``get_changed_files_in_push`` rev-list logic: prefers - ``/..HEAD``, then falls back to merge-base with - main/master for new-branch pushes. On any error, returns - ``([], "...")`` — the caller then fails closed. Output lines - that don't parse as a git SHA (7–64 lowercase hex) are - rejected so that a misbehaving git wrapper can't smuggle - arbitrary strings into the commit list. + ``/..HEAD``, then falls back to merge-base with the + pipeline's ``base_branch`` (then main/master) for new-branch pushes. + Diffing against ``base_branch`` first keeps commits inherited from a + non-trunk base out of the range so they are not attributed to the + pushing role (#3024). On any error, returns ``([], "...")`` — the + caller then fails closed. Output lines that don't parse as a git SHA + (7–64 lowercase hex) are rejected so that a misbehaving git wrapper + can't smuggle arbitrary strings into the commit list. """ import subprocess @@ -1687,7 +1738,21 @@ def _rev_list(base: str) -> list[str] | None: if primary is not None: return primary, None - for default_branch in ("main", "master"): + # New-branch fallback: prefer the pipeline's configured base_branch so + # commits inherited from a non-trunk base stay out of the range (#3024). + if base_branch and base_branch != "HEAD": + try: + subprocess.run( + git_cmd("fetch", remote, base_branch), + cwd=repo_path, + capture_output=True, + text=True, + timeout=30, + check=False, + ) + except Exception: + pass # merge-base will fall through to the next candidate + for default_branch in _fallback_base_candidates(base_branch): mb = subprocess.run( git_cmd("merge-base", f"{remote}/{default_branch}", "HEAD"), cwd=repo_path, @@ -1822,12 +1887,17 @@ def get_attributed_changed_files_in_push( branch: str, session_role: str | None = None, registry_client: object | None = None, + base_branch: str | None = None, ) -> AttributedPushRange: """Return attributed files + commit range for a planned push. ``session_role`` is advisory (currently used only for audit logging in the caller); the function itself does not filter on it. + ``base_branch`` is forwarded to ``_enumerate_push_commits`` as the + preferred new-branch diff base so commits inherited from a non-trunk + base are excluded from the attributed range (#3024). + The caller supplies a ``registry_client`` with a ``lookup_bulk`` method so the gateway's push handler can mock the client in tests and avoid the network round-trip when it already knows the @@ -1837,7 +1907,7 @@ def get_attributed_changed_files_in_push( On any detection failure, returns an ``AttributedPushRange`` with ``error`` set and ``files`` empty — the caller MUST fail closed. """ - commits, err = _enumerate_push_commits(repo_path, remote, branch) + commits, err = _enumerate_push_commits(repo_path, remote, branch, base_branch=base_branch) if err: logger.error( "get_attributed_changed_files_in_push enumeration failed — failing closed", diff --git a/gateway/session_manager.py b/gateway/session_manager.py index 45b7c5d8c6..0751723866 100644 --- a/gateway/session_manager.py +++ b/gateway/session_manager.py @@ -219,6 +219,12 @@ class Session: last_branch: str | None = None # Last known branch from git push claude_code_version: str | None = None # Claude Code version from container assigned_branch: str | None = None # Worktree branch locked to this session + # Pipeline's configured base branch (PR base). The push handler uses it as + # the preferred diff base for the new-branch restricted-path check so a + # branch forked from a non-trunk base is not blamed for files inherited + # unchanged from that base (#3024). Orchestrator-authoritative (set from the + # launcher-authenticated register_session) — never agent-settable. + base_branch: str | None = None auto_commit_sha: str | None = None # SHA from post-agent auto-commit jira_ticket: str | None = None # Advisory Jira ticket key (issue #1556) synthetic: bool = False # Orchestrator-internal temp session — skip auto-commit @@ -271,6 +277,8 @@ def to_dict_for_persistence(self) -> dict[str, Any]: result["claude_code_version"] = self.claude_code_version if self.assigned_branch is not None: result["assigned_branch"] = self.assigned_branch + if self.base_branch is not None: + result["base_branch"] = self.base_branch if self.auto_commit_sha is not None: result["auto_commit_sha"] = self.auto_commit_sha if self.jira_ticket is not None: @@ -307,6 +315,7 @@ def from_persistence(cls, data: dict[str, Any]) -> Session: last_branch=data.get("last_branch"), claude_code_version=data.get("claude_code_version"), assigned_branch=data.get("assigned_branch"), + base_branch=data.get("base_branch"), auto_commit_sha=data.get("auto_commit_sha"), jira_ticket=data.get("jira_ticket"), synthetic=bool(data.get("synthetic", False)), @@ -468,6 +477,7 @@ def register_session( agent_anchor_id: str | None = None, claude_code_version: str | None = None, branch: str | None = None, + base_branch: str | None = None, jira_ticket: str | None = None, synthetic: bool = False, upstream: str = "anthropic", @@ -488,6 +498,10 @@ def register_session( agent_anchor_id: Optional agent anchor ID for scoped anchor file writes claude_code_version: Optional Claude Code version string branch: Optional git branch for non-pushing pipeline sessions + base_branch: Optional pipeline base branch (PR base). Stored on the + session and used as the preferred diff base for the new-branch + restricted-path push check so a branch forked from a non-trunk + base is not blamed for files inherited unchanged from it (#3024). upstream: Upstream registry name driving ``/v1/messages`` traffic for this session (issue #2769). Defaults to ``"anthropic"`` so all pre-#2769 callers keep byte-identical @@ -549,6 +563,7 @@ def register_session( agent_role=agent_role, agent_anchor_id=agent_anchor_id, claude_code_version=claude_code_version, + base_branch=base_branch, jira_ticket=jira_ticket, synthetic=synthetic, upstream=upstream, diff --git a/gateway/tests/test_git_client_base_branch.py b/gateway/tests/test_git_client_base_branch.py new file mode 100644 index 0000000000..14cdfb264a --- /dev/null +++ b/gateway/tests/test_git_client_base_branch.py @@ -0,0 +1,222 @@ +"""Regression tests for #3024 — the restricted-path push guard must diff a +new-branch push against the pipeline's configured ``base_branch`` instead of +trunk (``main``/``master``). + +When ``base_branch`` carries content not yet on trunk (e.g. an in-flight skills +library), the old merge-base-with-main fallback enumerated those inherited +commits and mis-attributed their files to the pushing role, blocking +non-documenter roles. These tests pin the corrected diff base. +""" + +import subprocess +import sys +from pathlib import Path +from unittest.mock import MagicMock, patch + +# Add parent directory to path for imports +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from git_client import ( # noqa: E402 + _enumerate_push_commits, + _fallback_base_candidates, + get_changed_files_in_push, + git_cmd, +) + +# --------------------------------------------------------------------------- +# _fallback_base_candidates — ordering / dedup +# --------------------------------------------------------------------------- + + +class TestFallbackBaseCandidates: + def test_base_branch_tried_first(self): + assert _fallback_base_candidates("release-x") == ["release-x", "main", "master"] + + def test_none_keeps_trunk_only(self): + assert _fallback_base_candidates(None) == ["main", "master"] + + def test_head_sentinel_ignored(self): + # "HEAD" is the worktree-create default, not a real base — skip it. + assert _fallback_base_candidates("HEAD") == ["main", "master"] + + def test_base_branch_equal_to_trunk_is_deduped(self): + assert _fallback_base_candidates("main") == ["main", "master"] + assert _fallback_base_candidates("master") == ["master", "main"] + + +# --------------------------------------------------------------------------- +# Real-git end-to-end regression (the #3024 scenario) +# --------------------------------------------------------------------------- + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + git_cmd(*args), + cwd=str(repo), + capture_output=True, + text=True, + check=True, + ) + + +def _write(repo: Path, rel: str, content: str) -> None: + p = repo / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(content) + + +def _build_repo(tmp_path: Path) -> Path: + """Create a bare origin + a working clone modelling the #3024 setup. + + Layout produced in the working clone (``work``): + - ``main`` : initial commit only + - ``base`` : ``main`` + a documenter-owned skills file and + ``lint_ignorelist.txt`` (carried, not yet on main) + - ``egg/p/work`` : ``base`` + the refiner's analysis draft only; + NOT pushed to origin (new-branch push scenario) + Only ``main`` and ``base`` are pushed to origin. + """ + origin = tmp_path / "origin.git" + origin.mkdir() + _git(origin, "init", "--bare", "--initial-branch=main", str(origin)) + + work = tmp_path / "work" + work.mkdir() + _git(work, "init", "--initial-branch=main", str(work)) + _git(work, "config", "user.email", "test@egg.local") + _git(work, "config", "user.name", "egg test") + _git(work, "remote", "add", "origin", str(origin)) + + # main: initial commit + _write(work, "README.md", "hello\n") + _git(work, "add", "README.md") + _git(work, "commit", "-m", "init") + _git(work, "push", "origin", "main") + + # base: carries documenter-owned content not yet on main + _git(work, "checkout", "-b", "base") + _write(work, ".agents/skills/add-graphql-field/SKILL.md", "# skill\n") + _write(work, "lint_ignorelist.txt", "ignore-me\n") + _git(work, "add", "-A") + _git(work, "commit", "-m", "carry skills library on base") + _git(work, "push", "origin", "base") + + # pipeline branch off base: refiner writes only its analysis draft + _git(work, "checkout", "-b", "egg/p/work") + _write(work, ".egg-state/drafts/123-analysis.md", "# analysis\n") + _git(work, "add", "-A") + _git(work, "commit", "-m", "refiner analysis") + return work + + +def test_base_branch_excludes_inherited_files(tmp_path): + """With base_branch set, only the refiner's own commit is attributed — + files inherited unchanged from base are not blamed on the push (#3024).""" + work = _build_repo(tmp_path) + + files, error = get_changed_files_in_push(str(work), "origin", "egg/p/work", base_branch="base") + + assert error is None + assert files == [".egg-state/drafts/123-analysis.md"] + # The documenter-owned files inherited from base must NOT appear. + assert ".agents/skills/add-graphql-field/SKILL.md" not in files + assert "lint_ignorelist.txt" not in files + + +def test_without_base_branch_inherited_files_leak_in(tmp_path): + """Pins the pre-fix behavior: diffing against trunk re-introduces the + inherited base files (the false positive #3024 fixes). base_branch=None + falls back to main, so the carried skills files appear in the diff.""" + work = _build_repo(tmp_path) + + files, error = get_changed_files_in_push(str(work), "origin", "egg/p/work", base_branch=None) + + assert error is None + assert ".agents/skills/add-graphql-field/SKILL.md" in files + assert "lint_ignorelist.txt" in files + + +def test_enumerate_push_commits_excludes_base_commits(tmp_path): + """_enumerate_push_commits (attribution path) honours base_branch too — + only the refiner's commit is enumerated, not the inherited base commit.""" + work = _build_repo(tmp_path) + + with_base, err_base = _enumerate_push_commits( + str(work), "origin", "egg/p/work", base_branch="base" + ) + without_base, err_none = _enumerate_push_commits( + str(work), "origin", "egg/p/work", base_branch=None + ) + + assert err_base is None and err_none is None + # base_branch scoping yields exactly one commit (the refiner's); the + # trunk-based fallback also pulls in the base-carry commit. + assert len(with_base) == 1 + assert len(without_base) > len(with_base) + + +# --------------------------------------------------------------------------- +# Fail-safe: an unfetchable base_branch falls through to trunk, not an error +# --------------------------------------------------------------------------- + + +def test_base_branch_fetch_failure_falls_through_to_trunk(): + """If origin/ can't be resolved, the merge-base loop falls + through to main/master rather than failing closed prematurely.""" + with patch("subprocess.run") as mock_run: + + def side_effect(cmd, **kwargs): + result = MagicMock() + cmd_str = " ".join(cmd) + + if "fetch" in cmd: + # Both the pushed-branch and base_branch fetches fail. + result.returncode = 128 + result.stderr = "fatal: couldn't find remote ref" + return result + + # Primary rev-list fails (new branch). + if "rev-list" in cmd and "origin/branch..HEAD" in cmd_str: + result.returncode = 128 + result.stderr = "fatal: bad revision" + return result + + # base_branch merge-base can't resolve (ref never fetched). + if "merge-base" in cmd and "origin/missing-base" in cmd_str: + result.returncode = 128 + result.stderr = "fatal: not a valid object name" + return result + + # main merge-base succeeds — the trailing fallback. + if "merge-base" in cmd and "origin/main" in cmd_str: + result.returncode = 0 + result.stdout = "abc123\n" + return result + + if "rev-list" in cmd: + result.returncode = 0 + result.stdout = "sha1\n" + return result + + if "diff-tree" in cmd: + result.returncode = 0 + result.stdout = "src/app.py\n" + return result + + result.returncode = 128 + return result + + mock_run.side_effect = side_effect + + files, error = get_changed_files_in_push( + "/fake/repo", "origin", "branch", base_branch="missing-base" + ) + + assert error is None + assert files == ["src/app.py"] + # base_branch was attempted before main. + merge_base_refs = [ + " ".join(c[0][0]) for c in mock_run.call_args_list if "merge-base" in c[0][0] + ] + assert any("origin/missing-base" in r for r in merge_base_refs) + assert any("origin/main" in r for r in merge_base_refs) diff --git a/orchestrator/gateway_client.py b/orchestrator/gateway_client.py index d44291397c..8fc73d6072 100644 --- a/orchestrator/gateway_client.py +++ b/orchestrator/gateway_client.py @@ -731,6 +731,7 @@ def register_session( pr_number: int | None = None, claude_code_version: str | None = None, branch: str | None = None, + base_branch: str | None = None, worktree_container_id: str | None = None, jira_ticket: str | None = None, synthetic: bool = False, @@ -757,6 +758,12 @@ def register_session( pr_number: Optional GitHub PR number for checkpoint linkage claude_code_version: Optional Claude Code version string branch: Optional git branch for non-pushing session metadata + base_branch: Optional pipeline base branch (PR base). The gateway + stores it on the session and uses it as the preferred diff base + for the new-branch restricted-path push check, so a branch + forked from a non-trunk base is not blamed for files inherited + unchanged from that base (#3024). Omitted callers keep today's + main/master fallback. worktree_container_id: Optional container_id under which per-agent worktrees were already created by a prior create_worktrees call. When provided, the gateway reuses those worktrees @@ -811,6 +818,10 @@ def register_session( request_data["claude_code_version"] = claude_code_version if branch is not None: request_data["branch"] = branch + if base_branch is not None: + # Only include when set — omitting keeps the wire shape identical + # for callers that don't carry a base_branch (#3024). + request_data["base_branch"] = base_branch if worktree_container_id is not None: request_data["worktree_container_id"] = worktree_container_id if jira_ticket: diff --git a/orchestrator/kubernetes_spawner.py b/orchestrator/kubernetes_spawner.py index 351733c8bf..0ff05c11f0 100644 --- a/orchestrator/kubernetes_spawner.py +++ b/orchestrator/kubernetes_spawner.py @@ -772,6 +772,11 @@ def spawn_agent_job( issue_number=issue_number, claude_code_version=os.environ.get("CLAUDE_CODE_VERSION"), branch=branch, + # Pipeline base branch — the gateway uses it as the preferred + # diff base for the new-branch restricted-path push check so a + # branch forked from a non-trunk base is not blamed for files + # inherited unchanged from that base (#3024). + base_branch=base_branch, jira_ticket=jira_ticket, # Reuse the per-agent worktrees created above under # agent_worktree_id. Without this, the gateway would From fbe1d069b155b557371445a51700c78612483529 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 8 Jun 2026 23:35:28 +0000 Subject: [PATCH 2/4] fix(gateway): address review feedback on #3024 base_branch push diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses four findings from the egg-reviewer review on PR #3025. 1. Defense-in-depth: branch/base_branch validation (item #1) - Route both base_branch and branch through worktree_manager's validate_branch_ref in /api/v1/sessions/create AND in session_manager.register_session (the in-process spawner entry point). Rejects leading-dash values like "--upload-pack=evil" that would otherwise be parsed as a git flag by the downstream ``git fetch`` / ``git merge-base origin/ HEAD`` calls. 2. Harmonized fork-point SHA validation (item #2) - get_changed_files_in_push now validates the merge-base output with _SHA_LINE_RE.match like _enumerate_push_commits, so a multi-line or garbled stdout from a misbehaving git wrapper can't smuggle a non-SHA value through as a fork point. _SHA_LINE_RE moves above _fallback_base_candidates to be visible to both paths. 3. Tightened mock matching in the fallback-fail test (item #3) - 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. Resilient to git_cmd argv changes and to ref names that happen to contain "origin/main". 4. Eliminated redundant base_branch fetch on first push (item #4) - Extracted _fetch_base_branch_best_effort helper used by both fallback paths. The helper rev-parses origin/ locally first and skips the network fetch if 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. Timeout reduced from 30s to 15s (the base ref is a fallback, not the critical path). Side-effect: pre-existing tests in test_git_client.py that used "abc123"/"def456" as fake fork-point SHAs are updated to use 40-char hex shapes since _SHA_LINE_RE now rejects values shorter than 7 chars in both fallback paths. --- gateway/gateway.py | 25 +++ gateway/git_client.py | 97 ++++++++---- gateway/session_manager.py | 21 +++ gateway/tests/test_git_client.py | 10 +- gateway/tests/test_git_client_base_branch.py | 152 +++++++++++++++++-- 5 files changed, 257 insertions(+), 48 deletions(-) diff --git a/gateway/gateway.py b/gateway/gateway.py index d692dde117..448c97f1cf 100644 --- a/gateway/gateway.py +++ b/gateway/gateway.py @@ -211,6 +211,7 @@ WorktreeManager, get_active_docker_containers, startup_cleanup, + validate_branch_ref, validate_identifier, ) except ImportError: @@ -367,6 +368,7 @@ WorktreeManager, get_active_docker_containers, startup_cleanup, + validate_branch_ref, validate_identifier, ) @@ -8273,6 +8275,16 @@ def session_create() -> tuple[Response, int] | Response: return make_error("Invalid pipeline_id: must be 256 characters or fewer") # Validate base_branch if provided (#3024) + # + # Defense-in-depth: ``base_branch`` flows into ``git fetch`` and + # ``git merge-base origin/ HEAD`` as positional argv (see + # ``get_changed_files_in_push`` / ``_enumerate_push_commits``). A value + # starting with ``-`` would otherwise be interpreted as a git flag (the + # historical ``--upload-pack=...`` shape that has produced git RCEs). + # The orchestrator already validates ``base_branch`` at pipeline submission + # and the launcher secret gates this endpoint, but the gateway must not + # rely on caller hygiene here — ``validate_branch_ref`` rejects leading + # dashes, ``..``, null bytes, ``//``, and other unsafe ref shapes. if session_base_branch is not None: if not isinstance(session_base_branch, str): return make_error("Invalid base_branch: must be a string") @@ -8280,6 +8292,10 @@ def session_create() -> tuple[Response, int] | Response: 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") + try: + validate_branch_ref(session_base_branch, "base_branch") + except ValueError as exc: + return make_error(str(exc)) # Validate issue_number if provided if issue_number is not None and (not isinstance(issue_number, int) or issue_number < 1): @@ -8317,11 +8333,20 @@ def session_create() -> tuple[Response, int] | Response: return make_error("Invalid claude_code_version: must be 64 characters or fewer") # Validate branch if provided + # + # Same argv-injection concern as ``base_branch`` above — ``branch`` flows + # into ``git fetch origin `` and ``git rev-list origin/..HEAD`` + # in the push handler, so refuse leading dashes and other unsafe ref shapes + # via ``validate_branch_ref`` regardless of upstream callers' own checks. if branch is not None: if not isinstance(branch, str): return make_error("Invalid branch: must be a string") if len(branch) > 256: return make_error("Invalid branch: must be 256 characters or fewer") + try: + validate_branch_ref(branch, "branch") + except ValueError as exc: + return make_error(str(exc)) # Validate synthetic if provided if not isinstance(synthetic, bool): diff --git a/gateway/git_client.py b/gateway/git_client.py index 412669f71f..7374d2cff4 100644 --- a/gateway/git_client.py +++ b/gateway/git_client.py @@ -1375,6 +1375,62 @@ def cleanup_credential_helper(path: str | None) -> None: # ============================================================================= +# Matches a single git SHA line (7–64 lowercase hex). Used to reject +# multi-line / garbled stdout from a misbehaving git wrapper before treating +# the value as a commit identifier. Shared by both new-branch fallback paths +# (``get_changed_files_in_push`` and ``_enumerate_push_commits``). +_SHA_LINE_RE = re.compile(r"^[0-9a-f]{7,64}$") + + +def _fetch_base_branch_best_effort( + repo_path: str, remote: str, base_branch: str, timeout: int = 15 +) -> None: + """Best-effort fetch of ``origin/`` for the new-branch fallback. + + A single ``git_push`` request calls both ``get_changed_files_in_push`` and + ``_enumerate_push_commits`` and both run this fallback on the first push + (before ``origin/`` exists). Without coordination they each pay + the full fetch timeout — up to 60 s of added latency when the remote is + reachable-but-slow. This helper checks for the ref locally first via a + cheap ``rev-parse --verify --quiet``: on the second call the ref is + already present (the first call just fetched it) and the redundant network + round-trip is skipped. Fetch timeout is 15 s here vs. 30 s for the primary + branch fetch, because the base ref is a fallback, not the critical path. + + Best-effort throughout: both rev-parse and fetch errors are swallowed — + if the fetch fails, the merge-base loop just falls through to the next + candidate (``main`` / ``master``) and ultimately fails closed if nothing + resolves. + """ + import subprocess + + try: + check = subprocess.run( + git_cmd("rev-parse", "--verify", "--quiet", f"{remote}/{base_branch}"), + cwd=repo_path, + capture_output=True, + text=True, + timeout=5, + check=False, + ) + if check.returncode == 0 and (check.stdout or "").strip(): + return + except Exception: + pass # treat as not-yet-fetched and try the network fetch + + try: + subprocess.run( + git_cmd("fetch", remote, base_branch), + cwd=repo_path, + capture_output=True, + text=True, + timeout=timeout, + check=False, + ) + except Exception: + pass # merge-base will fall through to the next candidate + + def _fallback_base_candidates(base_branch: str | None) -> list[str]: """Ordered, de-duplicated diff-base candidates for new-branch pushes. @@ -1532,17 +1588,10 @@ def get_changed_files_in_push( # Best-effort fetch so origin/ resolves for the # merge-base below — the top-of-function fetch only refreshed the # pushed branch. main/master are assumed already present locally. - try: - subprocess.run( - git_cmd("fetch", remote, base_branch), - cwd=repo_path, - capture_output=True, - text=True, - timeout=30, - check=False, - ) - except Exception: - pass # merge-base will fall through to the next candidate + # The helper short-circuits on the second call of the same push + # (when _enumerate_push_commits ran first or vice-versa) via a + # local rev-parse check, avoiding a redundant network fetch. + _fetch_base_branch_best_effort(repo_path, remote, base_branch) for default_branch in fallback_bases: merge_base_result = subprocess.run( git_cmd("merge-base", f"{remote}/{default_branch}", "HEAD"), @@ -1555,8 +1604,12 @@ def get_changed_files_in_push( if merge_base_result.returncode != 0: continue - fork_point = merge_base_result.stdout.strip() - if not fork_point: + # Validate the merge-base output is a single SHA line (mirrors + # _enumerate_push_commits): a misbehaving git wrapper or multi-line + # stdout (e.g. parent-pair output that would surface if --all were + # ever added) must not leak through as a fork point. + fork_point = (merge_base_result.stdout or "").strip() + if not _SHA_LINE_RE.match(fork_point): continue # Get files changed in each commit between fork point and HEAD @@ -1677,9 +1730,6 @@ class AttributedPushRange: error: str | None = None -_SHA_LINE_RE = re.compile(r"^[0-9a-f]{7,64}$") - - def _enumerate_push_commits( repo_path: str, remote: str, branch: str, base_branch: str | None = None ) -> tuple[list[str], str | None]: @@ -1740,18 +1790,11 @@ def _rev_list(base: str) -> list[str] | None: # New-branch fallback: prefer the pipeline's configured base_branch so # commits inherited from a non-trunk base stay out of the range (#3024). + # The helper short-circuits if the ref is already local (the sister + # function in the same push already fetched it), avoiding a redundant + # network round-trip. if base_branch and base_branch != "HEAD": - try: - subprocess.run( - git_cmd("fetch", remote, base_branch), - cwd=repo_path, - capture_output=True, - text=True, - timeout=30, - check=False, - ) - except Exception: - pass # merge-base will fall through to the next candidate + _fetch_base_branch_best_effort(repo_path, remote, base_branch) for default_branch in _fallback_base_candidates(base_branch): mb = subprocess.run( git_cmd("merge-base", f"{remote}/{default_branch}", "HEAD"), diff --git a/gateway/session_manager.py b/gateway/session_manager.py index 0751723866..7909924e46 100644 --- a/gateway/session_manager.py +++ b/gateway/session_manager.py @@ -542,6 +542,27 @@ def register_session( if len(upstream_model) > 256: raise ValueError("upstream_model must be 256 characters or fewer") + # Validate branch / base_branch shape (#3024 defense-in-depth). + # Both values flow into ``git fetch`` / ``git merge-base`` as positional + # argv inside the push handler, so a leading-dash value would be parsed + # as a git flag (e.g. ``--upload-pack=evil``). The HTTP route applies + # the same check; mirror it here so a direct in-process caller (the + # slice-2 spawner, tests) cannot bypass it. + try: + from .worktree_manager import validate_branch_ref + except ImportError: + from worktree_manager import ( # type: ignore[no-redef,import-untyped] + validate_branch_ref, + ) + if branch is not None: + if not isinstance(branch, str) or len(branch) > 256: + raise ValueError("branch must be a string of ≤256 characters") + validate_branch_ref(branch, "branch") + if base_branch is not None: + if not isinstance(base_branch, str) or len(base_branch) > 256: + raise ValueError("base_branch must be a string of ≤256 characters") + validate_branch_ref(base_branch, "base_branch") + # Generate cryptographically secure token token = secrets.token_urlsafe(SESSION_TOKEN_BYTES) token_hash = _hash_token(token) diff --git a/gateway/tests/test_git_client.py b/gateway/tests/test_git_client.py index 291a7287d6..61489b4b40 100644 --- a/gateway/tests/test_git_client.py +++ b/gateway/tests/test_git_client.py @@ -943,7 +943,7 @@ def side_effect(cmd, **kwargs): # merge-base call succeeds for main if "merge-base" in cmd and "origin/main" in cmd_str: result.returncode = 0 - result.stdout = "abc123\n" + result.stdout = "abcdef0123456789abcdef0123456789abcdef01\n" return result # Fallback rev-list returns two commits @@ -1001,7 +1001,7 @@ def side_effect(cmd, **kwargs): if "merge-base" in cmd and "origin/master" in cmd_str: result.returncode = 0 - result.stdout = "def456\n" + result.stdout = "def4567890abcdef1234567890abcdef12345678\n" return result if "rev-list" in cmd: @@ -1046,7 +1046,7 @@ def side_effect(cmd, **kwargs): if "merge-base" in cmd and "origin/main" in cmd_str: result.returncode = 0 - result.stdout = "abc123\n" + result.stdout = "abcdef0123456789abcdef0123456789abcdef01\n" return result if "rev-list" in cmd: @@ -1102,7 +1102,7 @@ def side_effect(cmd, **kwargs): if "merge-base" in cmd and "origin/main" in cmd_str: result.returncode = 0 - result.stdout = "abc123\n" + result.stdout = "abcdef0123456789abcdef0123456789abcdef01\n" return result if "rev-list" in cmd: @@ -1298,7 +1298,7 @@ def side_effect(cmd, **kwargs): # Fallback: merge-base with main if "merge-base" in cmd and "origin/main" in cmd_str: result.returncode = 0 - result.stdout = "abc123\n" + result.stdout = "abcdef0123456789abcdef0123456789abcdef01\n" return result # Fallback rev-list diff --git a/gateway/tests/test_git_client_base_branch.py b/gateway/tests/test_git_client_base_branch.py index 14cdfb264a..7b6e263d93 100644 --- a/gateway/tests/test_git_client_base_branch.py +++ b/gateway/tests/test_git_client_base_branch.py @@ -19,6 +19,7 @@ from git_client import ( # noqa: E402 _enumerate_push_commits, _fallback_base_candidates, + _fetch_base_branch_best_effort, get_changed_files_in_push, git_cmd, ) @@ -162,48 +163,88 @@ def test_enumerate_push_commits_excludes_base_commits(tmp_path): def test_base_branch_fetch_failure_falls_through_to_trunk(): """If origin/ can't be resolved, the merge-base loop falls - through to main/master rather than failing closed prematurely.""" + through to main/master rather than failing closed prematurely. + + Mock routing keys on the **subcommand verb at a fixed argv position** and + on **exact equality of named positional argv slots**, never substring + ``in`` checks against ``" ".join(cmd)`` — a `"origin/main"` substring + would otherwise misroute if a future ref name happened to contain it + (``origin/main-backup``) or if a ``-c`` config slot mentioned the trunk. + """ + # git_cmd argv shapes the SUT calls. Verifying these here keeps the mock + # honest if git_cmd grows new prefix args (e.g. extra ``-c`` flags). + fetch_branch_argv = git_cmd("fetch", "origin", "branch") + fetch_base_argv = git_cmd("fetch", "origin", "missing-base") + primary_rev_list_argv = git_cmd("rev-list", "origin/branch..HEAD") + rev_parse_base_argv = git_cmd("rev-parse", "--verify", "--quiet", "origin/missing-base") + mb_base_argv = git_cmd("merge-base", "origin/missing-base", "HEAD") + mb_main_argv = git_cmd("merge-base", "origin/main", "HEAD") + # 40-char hex SHA — _SHA_LINE_RE requires 7-64 lowercase hex; the + # harmonized validation in get_changed_files_in_push now rejects shorter + # values too, so use a realistic SHA shape rather than the toy "abc123". + fake_fork_point = "abcdef0123456789abcdef0123456789abcdef01" + fake_commit = "1234567890abcdef1234567890abcdef12345678" + rev_list_from_main_argv = git_cmd("rev-list", f"{fake_fork_point}..HEAD") + diff_tree_argv = git_cmd("diff-tree", "--no-commit-id", "--name-only", "-r", fake_commit) + with patch("subprocess.run") as mock_run: def side_effect(cmd, **kwargs): result = MagicMock() - cmd_str = " ".join(cmd) - if "fetch" in cmd: - # Both the pushed-branch and base_branch fetches fail. + # Both fetches fail (pushed-branch + base_branch refs missing). + if cmd == fetch_branch_argv or cmd == fetch_base_argv: result.returncode = 128 result.stderr = "fatal: couldn't find remote ref" + result.stdout = "" + return result + + # rev-parse on origin/missing-base fails (ref never fetched), so + # the helper falls through to the network fetch (which also fails). + if cmd == rev_parse_base_argv: + result.returncode = 128 + result.stdout = "" + result.stderr = "" return result # Primary rev-list fails (new branch). - if "rev-list" in cmd and "origin/branch..HEAD" in cmd_str: + if cmd == primary_rev_list_argv: result.returncode = 128 result.stderr = "fatal: bad revision" + result.stdout = "" return result # base_branch merge-base can't resolve (ref never fetched). - if "merge-base" in cmd and "origin/missing-base" in cmd_str: + if cmd == mb_base_argv: result.returncode = 128 result.stderr = "fatal: not a valid object name" + result.stdout = "" return result # main merge-base succeeds — the trailing fallback. - if "merge-base" in cmd and "origin/main" in cmd_str: + if cmd == mb_main_argv: result.returncode = 0 - result.stdout = "abc123\n" + result.stdout = f"{fake_fork_point}\n" + result.stderr = "" return result - if "rev-list" in cmd: + # rev-list of the post-fork-point range yields one synthetic SHA. + if cmd == rev_list_from_main_argv: result.returncode = 0 - result.stdout = "sha1\n" + result.stdout = f"{fake_commit}\n" + result.stderr = "" return result - if "diff-tree" in cmd: + # diff-tree of that synthetic SHA reports one changed file. + if cmd == diff_tree_argv: result.returncode = 0 result.stdout = "src/app.py\n" + result.stderr = "" return result result.returncode = 128 + result.stdout = "" + result.stderr = "" return result mock_run.side_effect = side_effect @@ -214,9 +255,88 @@ def side_effect(cmd, **kwargs): assert error is None assert files == ["src/app.py"] - # base_branch was attempted before main. - merge_base_refs = [ - " ".join(c[0][0]) for c in mock_run.call_args_list if "merge-base" in c[0][0] + + # base_branch merge-base was tried **before** main: assert ordering + # by indexing on the exact argv, not on substring containment. + merge_base_calls = [ + call.args[0] + for call in mock_run.call_args_list + if len(call.args) > 0 and "merge-base" in call.args[0] ] - assert any("origin/missing-base" in r for r in merge_base_refs) - assert any("origin/main" in r for r in merge_base_refs) + assert mb_base_argv in merge_base_calls + assert mb_main_argv in merge_base_calls + assert merge_base_calls.index(mb_base_argv) < merge_base_calls.index(mb_main_argv) + + +# --------------------------------------------------------------------------- +# _fetch_base_branch_best_effort — rev-parse short-circuit avoids redundant +# fetch when the ref is already local (e.g. on the second call within one push) +# --------------------------------------------------------------------------- + + +class TestFetchBaseBranchBestEffort: + def test_skips_fetch_when_ref_already_local(self): + """If rev-parse resolves origin/, no network fetch runs. + + This is the redundant-fetch elimination: a single push calls both + get_changed_files_in_push and _enumerate_push_commits, each of which + runs this helper. The first call fetches; the second finds the ref + locally and skips, halving the worst-case fallback latency. + """ + with patch("subprocess.run") as mock_run: + + def side_effect(cmd, **kwargs): + result = MagicMock() + # rev-parse succeeds — ref is already local. + if "rev-parse" in cmd: + result.returncode = 0 + result.stdout = "deadbeefdeadbeef\n" + result.stderr = "" + return result + # Any other call (we expect none) returns a failure to make + # an accidental network fetch loud rather than silent. + result.returncode = 128 + result.stdout = "" + result.stderr = "" + return result + + mock_run.side_effect = side_effect + + _fetch_base_branch_best_effort("/fake/repo", "origin", "base") + + argvs = [call.args[0] for call in mock_run.call_args_list if len(call.args) > 0] + # rev-parse ran; fetch did NOT. + assert any("rev-parse" in argv for argv in argvs) + for argv in argvs: + assert "fetch" not in argv, f"Unexpected fetch after successful rev-parse: {argv!r}" + + def test_fetches_when_ref_not_local(self): + """If rev-parse fails, the helper falls through to git fetch.""" + with patch("subprocess.run") as mock_run: + + def side_effect(cmd, **kwargs): + result = MagicMock() + if "rev-parse" in cmd: + result.returncode = 128 # ref unknown locally + result.stdout = "" + result.stderr = "" + return result + if "fetch" in cmd: + result.returncode = 0 + result.stdout = "" + result.stderr = "" + return result + result.returncode = 128 + return result + + mock_run.side_effect = side_effect + + _fetch_base_branch_best_effort("/fake/repo", "origin", "base") + + fetched = [ + call.args[0] + for call in mock_run.call_args_list + if len(call.args) > 0 and "fetch" in call.args[0] + ] + assert fetched, "expected git fetch to run when rev-parse fails" + assert fetched[0] == git_cmd("fetch", "origin", "base") From a0eec59ab0209adecf9977ed8aa35650ae3913ee Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Tue, 9 Jun 2026 00:04:47 +0000 Subject: [PATCH 3/4] fix(gateway): harmonize SHA-line validation across get_changed_files_in_push paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review feedback on #3025: the previous round closed the cross-function fork-point validation gap (item #2), but the primary and fallback rev-list paths of get_changed_files_in_push still piped each line straight to diff-tree's positional argv without validating it as a SHA. _enumerate_push_commits already validated uniformly via a nested _parse_shas — same shape as item #2, remaining piece. Promote _parse_shas to module scope as _parse_sha_lines and use it in: - get_changed_files_in_push primary path (rev-list origin/branch..HEAD) - get_changed_files_in_push fallback path (rev-list fork_point..HEAD) - _enumerate_push_commits (drops the now-duplicate nested helper) On parse failure the primary path falls through to the merge-base fallback (same effect as a non-zero rev-list returncode); the fallback inner rev-list continues to the next trunk candidate (same effect as a non-zero log_result returncode). Both routes still fail closed when every candidate is exhausted. Test mocks in TestGetChangedFilesInPush now use realistic 40-char hex SHA fixtures (_FAKE_SHA_{1,2,3}) instead of toy 'sha1'/'sha2'/'sha3' strings, matching the same fixup the previous round applied to the merge-base mock returns. --- gateway/git_client.py | 76 ++++++++++++++++++++++---------- gateway/tests/test_git_client.py | 35 +++++++++------ 2 files changed, 75 insertions(+), 36 deletions(-) diff --git a/gateway/git_client.py b/gateway/git_client.py index 7374d2cff4..8bd83528f3 100644 --- a/gateway/git_client.py +++ b/gateway/git_client.py @@ -1377,11 +1377,33 @@ def cleanup_credential_helper(path: str | None) -> None: # Matches a single git SHA line (7–64 lowercase hex). Used to reject # multi-line / garbled stdout from a misbehaving git wrapper before treating -# the value as a commit identifier. Shared by both new-branch fallback paths -# (``get_changed_files_in_push`` and ``_enumerate_push_commits``). +# the value as a commit identifier. Shared by both ``get_changed_files_in_push`` +# and ``_enumerate_push_commits`` for fork-point and rev-list validation. _SHA_LINE_RE = re.compile(r"^[0-9a-f]{7,64}$") +def _parse_sha_lines(text: str) -> list[str] | None: + """Parse rev-list stdout into a list of SHAs, or ``None`` if any line is garbled. + + Mirrors the validation in ``_enumerate_push_commits`` and applies it to + both fallback and primary rev-list output in ``get_changed_files_in_push`` + so SHA-from-stdout discipline is uniform across every path that pipes a + line of git stdout into ``diff-tree``'s positional argv. A misbehaving + git wrapper that smuggled ``--ext-diff=…`` or another diff-tree flag as + a "SHA" would otherwise be passed through; failing closed on parse error + keeps the caller's fail-closed semantics for security checks intact. + """ + out: list[str] = [] + for line in (text or "").splitlines(): + s = line.strip() + if not s: + continue + if not _SHA_LINE_RE.match(s): + return None + out.append(s) + return out + + def _fetch_base_branch_best_effort( repo_path: str, remote: str, base_branch: str, timeout: int = 15 ) -> None: @@ -1526,15 +1548,29 @@ def get_changed_files_in_push( check=False, ) - if rev_list_result.returncode == 0: + # Validate every rev-list line is a SHA before feeding it to diff-tree's + # positional argv (parity with _enumerate_push_commits, which goes through + # _parse_sha_lines on both its primary and fallback paths). On parse + # failure (garbled or multi-line stdout from a misbehaving git wrapper), + # treat the primary path as a miss and fall through to the merge-base + # fallback — same effect as a non-zero rev-list returncode. + primary_shas = ( + _parse_sha_lines(rev_list_result.stdout) if rev_list_result.returncode == 0 else None + ) + if primary_shas is None and rev_list_result.returncode == 0: + logger.error( + "rev-list stdout contained a non-SHA line - falling through to fallback", + repo_path=repo_path, + remote=remote, + branch=branch, + ) + + if primary_shas is not None: all_files: set[str] = set() commits_found = 0 commits_inspected = 0 diff_tree_errors: list[str] = [] - for sha in rev_list_result.stdout.strip().split("\n"): - sha = sha.strip() - if not sha: - continue + for sha in primary_shas: commits_found += 1 # NOTE: On merge commits, `diff-tree -r` uses combined diff # format, which only shows files differing from *all* parents @@ -1624,14 +1660,19 @@ def get_changed_files_in_push( if log_result.returncode != 0: continue + # Validate every SHA line before piping it to diff-tree argv. + # On parse failure, continue to the next candidate trunk — the + # caller's fail-closed loop still kicks in if every candidate + # fails (mirrors the per-line validation in _enumerate_push_commits). + fallback_shas = _parse_sha_lines(log_result.stdout) + if fallback_shas is None: + continue + all_files = set() commits_found = 0 commits_inspected = 0 diff_tree_errors = [] - for sha in log_result.stdout.strip().split("\n"): - sha = sha.strip() - if not sha: - continue + for sha in fallback_shas: commits_found += 1 dt_result = subprocess.run( git_cmd("diff-tree", "--no-commit-id", "--name-only", "-r", sha), @@ -1760,17 +1801,6 @@ def _enumerate_push_commits( except Exception: pass - def _parse_shas(text: str) -> list[str] | None: - out: list[str] = [] - for line in (text or "").splitlines(): - s = line.strip() - if not s: - continue - if not _SHA_LINE_RE.match(s): - return None - out.append(s) - return out - def _rev_list(base: str) -> list[str] | None: result = subprocess.run( git_cmd("rev-list", "--reverse", f"{base}..HEAD"), @@ -1782,7 +1812,7 @@ def _rev_list(base: str) -> list[str] | None: ) if result.returncode != 0: return None - return _parse_shas(result.stdout) + return _parse_sha_lines(result.stdout) primary = _rev_list(f"{remote}/{branch}") if primary is not None: diff --git a/gateway/tests/test_git_client.py b/gateway/tests/test_git_client.py index 61489b4b40..c5d1c87f15 100644 --- a/gateway/tests/test_git_client.py +++ b/gateway/tests/test_git_client.py @@ -782,6 +782,15 @@ def test_execute_env_includes_ssh_url_rewrite(self): assert found_ssh_protocol, "Missing URL rewrite for ssh://git@github.com/ format" +# Realistic 40-char hex SHA fixtures. ``get_changed_files_in_push`` and +# ``_enumerate_push_commits`` both validate rev-list / merge-base stdout via +# ``_SHA_LINE_RE`` (7–64 lowercase hex) before passing the value to ``diff-tree`` +# argv, so test mocks must emit shapes that the production validator accepts. +_FAKE_SHA_1 = "1" * 40 +_FAKE_SHA_2 = "2" * 40 +_FAKE_SHA_3 = "3" * 40 + + class TestGetChangedFilesInPush: """Tests for getting changed files in a push.""" @@ -801,14 +810,14 @@ def side_effect(cmd, **kwargs): if "rev-list" in cmd and "origin/branch..HEAD" in cmd_str: result.returncode = 0 - result.stdout = "sha1\nsha2\n" + result.stdout = f"{_FAKE_SHA_1}\n{_FAKE_SHA_2}\n" return result if "diff-tree" in cmd: result.returncode = 0 - if "sha1" in cmd: + if _FAKE_SHA_1 in cmd: result.stdout = "file1.py\nfile2.py\n" - elif "sha2" in cmd: + elif _FAKE_SHA_2 in cmd: result.stdout = "dir/file3.txt\n" return result @@ -898,7 +907,7 @@ def side_effect(cmd, **kwargs): return result if "rev-list" in cmd: result.returncode = 0 - result.stdout = "sha1\n" + result.stdout = f"{_FAKE_SHA_1}\n" return result if "diff-tree" in cmd: result.returncode = 0 @@ -949,15 +958,15 @@ def side_effect(cmd, **kwargs): # Fallback rev-list returns two commits if "rev-list" in cmd: result.returncode = 0 - result.stdout = "sha1\nsha2\n" + result.stdout = f"{_FAKE_SHA_1}\n{_FAKE_SHA_2}\n" return result # diff-tree per commit if "diff-tree" in cmd: - if "sha1" in cmd: + if _FAKE_SHA_1 in cmd: result.returncode = 0 result.stdout = "src/auth.py\n" - elif "sha2" in cmd: + elif _FAKE_SHA_2 in cmd: result.returncode = 0 result.stdout = "src/models.py\nsrc/auth.py\n" return result @@ -1006,7 +1015,7 @@ def side_effect(cmd, **kwargs): if "rev-list" in cmd: result.returncode = 0 - result.stdout = "sha1\n" + result.stdout = f"{_FAKE_SHA_1}\n" return result if "diff-tree" in cmd: @@ -1051,7 +1060,7 @@ def side_effect(cmd, **kwargs): if "rev-list" in cmd: result.returncode = 0 - result.stdout = "sha1\nsha2\n" + result.stdout = f"{_FAKE_SHA_1}\n{_FAKE_SHA_2}\n" return result # All diff-tree calls fail @@ -1107,7 +1116,7 @@ def side_effect(cmd, **kwargs): if "rev-list" in cmd: result.returncode = 0 - result.stdout = "sha1\nsha2\nsha3\n" + result.stdout = f"{_FAKE_SHA_1}\n{_FAKE_SHA_2}\n{_FAKE_SHA_3}\n" return result # First diff-tree succeeds, rest fail @@ -1202,7 +1211,7 @@ def side_effect(cmd, **kwargs): # Primary rev-list succeeds with 3 commits if "rev-list" in cmd and "origin/branch..HEAD" in cmd_str: result.returncode = 0 - result.stdout = "sha1\nsha2\nsha3\n" + result.stdout = f"{_FAKE_SHA_1}\n{_FAKE_SHA_2}\n{_FAKE_SHA_3}\n" return result # First diff-tree succeeds, rest fail — triggers fallback @@ -1248,7 +1257,7 @@ def side_effect(cmd, **kwargs): return result if "rev-list" in cmd: result.returncode = 0 - result.stdout = "sha1\n" + result.stdout = f"{_FAKE_SHA_1}\n" return result if "diff-tree" in cmd: result.returncode = 0 @@ -1304,7 +1313,7 @@ def side_effect(cmd, **kwargs): # Fallback rev-list if "rev-list" in cmd: result.returncode = 0 - result.stdout = "sha1\n" + result.stdout = f"{_FAKE_SHA_1}\n" return result if "diff-tree" in cmd: From ceaa583f85b244ed2448387576e278728912b0a8 Mon Sep 17 00:00:00 2001 From: "james-in-a-box[bot]" <246424927+james-in-a-box[bot]@users.noreply.github.com> Date: Tue, 9 Jun 2026 00:21:37 +0000 Subject: [PATCH 4/4] Fix checks: lengthen mock SHA to satisfy new 7+ char SHA-line validation --- gateway/tests/test_scoped_push_detection.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gateway/tests/test_scoped_push_detection.py b/gateway/tests/test_scoped_push_detection.py index 837d42b4f5..6855c0dbfc 100644 --- a/gateway/tests/test_scoped_push_detection.py +++ b/gateway/tests/test_scoped_push_detection.py @@ -31,8 +31,8 @@ def test_diff_returns_only_agent_files(self, mock_run): # git fetch MagicMock(returncode=0, stdout="", stderr=""), # git rev-list origin/branch..HEAD - MagicMock(returncode=0, stdout="abc123\n", stderr=""), - # git diff-tree for abc123 + MagicMock(returncode=0, stdout="abc1234\n", stderr=""), + # git diff-tree for abc1234 MagicMock( returncode=0, stdout="tests/test_new.py\ntests/conftest.py\n",