Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions orchestrator/routes/pipelines.py
Original file line number Diff line number Diff line change
Expand Up @@ -10211,7 +10211,28 @@ def _open_context_pr_for_pipeline(
error=str(fetch_err),
)

tmp_worktree = Path(tempfile.mkdtemp(prefix=f"egg-context-{pipeline_id}-", dir="/tmp"))
# Root under WORKTREE_BASE_DIR so the path falls inside the gateway's
# repo-path allowlist (gateway/git_client.py ALLOWED_REPO_PATHS) — a
# ``/tmp`` location would be rejected by ``validate_repo_path`` and
# the subsequent ``push_worktree_branch`` call would fail with
# ``repo_path must be within allowed directories`` (#2684). Falls
# back to the system temp dir in environments where the base path
# is absent (e.g. unit tests) — emit a warning on that branch so a
# broken docker volume mount in production is noisy rather than
# silently recreating the #2684 push-rejection.
if WORKTREE_BASE_DIR.exists():
tmp_dir_base = str(WORKTREE_BASE_DIR)
else:
logger.warning(
"Context PR hook: WORKTREE_BASE_DIR missing — falling back to "
"system temp (likely a broken volume mount in production; the "
"push to the context branch will be rejected by the gateway "
"allowlist) (#2684)",
pipeline_id=pipeline_id,
worktree_base_dir=str(WORKTREE_BASE_DIR),
)
tmp_dir_base = None
tmp_worktree = Path(tempfile.mkdtemp(prefix=f"egg-context-{pipeline_id}-", dir=tmp_dir_base))
# Use a unique sub-path so ``git worktree add`` doesn't collide with
# the (already-created-by-mkdtemp) directory. ``git worktree add``
# refuses to add to an existing non-empty directory.
Expand Down Expand Up @@ -10968,8 +10989,34 @@ def _commit_slice_brc_history_to_integration_branch(
error=str(fetch_err),
)

# Root under WORKTREE_BASE_DIR so the temp path falls inside the
# gateway's repo-path allowlist (gateway/git_client.py
# ALLOWED_REPO_PATHS). A ``/tmp`` location is rejected by
# ``validate_repo_path``, which silently failed the BRC-history
# push and left slice PRs without their consensus transcript
# (#2684). Falls back to system temp when the base dir is absent
# (e.g. unit tests) — emit a warning on that branch so a broken
# docker volume mount in production is noisy rather than silently
# recreating the #2684 push-rejection.
if WORKTREE_BASE_DIR.exists():
tmp_dir_base = str(WORKTREE_BASE_DIR)
else:
logger.warning(
"Per-slice BRC commit: WORKTREE_BASE_DIR missing — falling "
"back to system temp (likely a broken volume mount in "
"production; the push to the integration branch will be "
"rejected by the gateway allowlist) (#2684)",
pipeline_id=pipeline_id,
slice_id=slice_id,
integration_branch=integration_branch,
worktree_base_dir=str(WORKTREE_BASE_DIR),
)
tmp_dir_base = None
tmp_worktree = Path(
tempfile.mkdtemp(prefix=f"egg-slice-brc-{pipeline_id}-{slice_id}-", dir="/tmp")
tempfile.mkdtemp(
prefix=f"egg-slice-brc-{pipeline_id}-{slice_id}-",
dir=tmp_dir_base,
)
)
wt_path = tmp_worktree / "wt"

Expand Down
51 changes: 51 additions & 0 deletions orchestrator/tests/test_context_pr.py
Original file line number Diff line number Diff line change
Expand Up @@ -1334,3 +1334,54 @@ def test_call_site_swallows_any_exception(self):
"_open_context_pr_for_pipeline in try/except Exception so a "
"hook failure can never escape into the transition path (D3)"
)


# ----------------------------------------------------------------------
# Gateway allowlist compatibility (#2684)
# ----------------------------------------------------------------------


class TestContextPRGatewayAllowlistCompatibility:
"""Regression coverage for #2684 (context-PR sibling).

Same shape as the slice-BRC hook: the context-PR hook builds a
temp worktree and passes ``repo_path=str(wt_path)`` to
``gateway.push_worktree_branch``. The gateway's
``validate_repo_path`` rejects ``/tmp`` paths, so the push fails
silently and the context PR opens without the curated refine/plan
artifacts. The hook must root the temp worktree inside
``WORKTREE_BASE_DIR``.
"""

def test_temp_worktree_is_rooted_under_worktree_base_dir(
self, tmp_path, pipeline, make_spawner, monkeypatch
):
import routes.pipelines as pipelines_mod

fake_base = tmp_path / "egg-worktrees-root"
fake_base.mkdir()
monkeypatch.setattr(pipelines_mod, "WORKTREE_BASE_DIR", fake_base)

_seed_repo(tmp_path, identifier=2548)
spawner = make_spawner()
load, save, _ = _stub_load_save_contract(contract_pr_factory=lambda: _make_contract())
with (
patch("egg_contracts.loader.load_contract", load),
patch("egg_contracts.loader.save_contract", save),
):
_open_context_pr_for_pipeline(pipeline, spawner, tmp_path)

ctx_pushes = [
c
for c in spawner.gateway.push_worktree_branch.call_args_list
if c.kwargs.get("branch") == "egg/issue-2548/context"
]
assert ctx_pushes, (
"context-branch push must happen at least once so the regression "
"test has a repo_path to inspect"
)
repo_path = ctx_pushes[0].kwargs["repo_path"]
assert repo_path.startswith(str(fake_base) + "/"), (
f"context-PR temp worktree must live under WORKTREE_BASE_DIR; "
f"got {repo_path!r}, expected prefix {str(fake_base)!r}"
)
65 changes: 65 additions & 0 deletions orchestrator/tests/test_per_slice_brc_commit.py
Original file line number Diff line number Diff line change
Expand Up @@ -721,3 +721,68 @@ def _spy_copy(src, dst, *args, **kwargs):
# The seeded files should have been picked up.
assert str(paths["slice-1-md"]) in copy_srcs
assert str(paths["slice-1-json"]) in copy_srcs


# ----------------------------------------------------------------------
# Gateway allowlist compatibility (#2684)
# ----------------------------------------------------------------------


class TestGatewayAllowlistCompatibility:
"""Regression coverage for #2684.

The hook constructs a temp worktree and passes ``repo_path=str(wt_path)``
to ``gateway.push_worktree_branch``. The gateway's
``validate_repo_path`` only accepts paths under
``ALLOWED_REPO_PATHS`` (``/home/egg/.egg-worktrees/`` et al.); a
``/tmp`` location is rejected and the push fails silently — slice
PRs then open without their consensus-history file. The hook must
therefore root its temp worktree inside ``WORKTREE_BASE_DIR``.
"""

def test_temp_worktree_is_rooted_under_worktree_base_dir(
self, tmp_path, pipeline, make_spawner, monkeypatch
):
import routes.pipelines as pipelines_mod

fake_base = tmp_path / "egg-worktrees-root"
fake_base.mkdir()
monkeypatch.setattr(pipelines_mod, "WORKTREE_BASE_DIR", fake_base)

_seed_per_slice_brc_files(tmp_path, identifier=2548, slice_ids=["slice-1"])
spawner = make_spawner()
with patch("routes.pipelines._write_brc_history", _no_op_write_brc_history):
_commit_slice_brc_history_to_integration_branch(
pipeline,
spawner,
tmp_path,
slice_id="slice-1",
integration_branch="egg/issue-2548/slice-1",
)

spawner.gateway.push_worktree_branch.assert_called_once()
repo_path = spawner.gateway.push_worktree_branch.call_args.kwargs["repo_path"]
assert repo_path.startswith(str(fake_base) + "/"), (
f"slice BRC temp worktree must live under WORKTREE_BASE_DIR; "
f"got {repo_path!r}, expected prefix {str(fake_base)!r}"
)

def test_production_worktree_base_dir_lies_within_gateway_allowlist(self):
"""Pin the contract between orchestrator and gateway.

``WORKTREE_BASE_DIR`` is where the slice-BRC temp worktree
lives; the gateway's ``ALLOWED_REPO_PATHS`` must contain a
prefix that covers it. This test reads ``WORKTREE_BASE_DIR``
from the orchestrator module rather than hardcoding the
production path, so a drift on *either* side
(orchestrator-side path move OR gateway-side allowlist tweak)
trips the regression instead of silently passing against a
stale hardcoded prefix.
"""
from routes.pipelines import WORKTREE_BASE_DIR

from gateway.git_client import validate_repo_path

candidate = str(WORKTREE_BASE_DIR / "egg-slice-brc-pipeline-x-slice-y-abc" / "wt")
ok, error = validate_repo_path(candidate)
assert ok, f"WORKTREE_BASE_DIR drifted out of gateway ALLOWED_REPO_PATHS: {error}"
Loading