Fix #2684: root slice-BRC + context-PR temp worktrees under WORKTREE_BASE_DIR

[jwbron]

Summary

• _commit_slice_brc_history_to_integration_branch and _open_context_pr_for_pipeline both built their temp worktree at tempfile.mkdtemp(dir="/tmp"), then passed repo_path=str(wt_path) to gateway.push_worktree_branch. The gateway's validate_repo_path rejects anything outside ALLOWED_REPO_PATHS (/home/egg/repos/, /home/egg/.egg-worktrees/, /home/egg/.egg-state/, /repos/), so every push failed with repo_path must be within allowed directories. The failure was swallowed at WARNING level — slice PRs opened without their BRC consensus transcript and context PRs opened without their refine/plan artifacts.
• Reparent both temp worktrees to WORKTREE_BASE_DIR (/home/egg/.egg-worktrees), which is already in the gateway allowlist and already mounted by the spawner. Falls back to the system temp dir when the base path is absent (unit tests).
• Add regression tests pinning (a) the hook's captured repo_path lives under WORKTREE_BASE_DIR, and (b) WORKTREE_BASE_DIR lies inside the gateway's ALLOWED_REPO_PATHS — catches drift on either side of the orchestrator/gateway contract.

Closes #2684.

Test plan

• make test (full suite, changeset-aware): 2760 passed
• Targeted: pytest orchestrator/tests/test_per_slice_brc_commit.py::TestGatewayAllowlistCompatibility orchestrator/tests/test_context_pr.py::TestContextPRGatewayAllowlistCompatibility — 3 passed
• Verified existing 60 tests in test_per_slice_brc_commit.py + test_context_pr.py still pass post-fix

[egg-reviewer]

Review summary

The fix is correct, surgical, and well-tested. Both _open_context_pr_for_pipeline and _commit_slice_brc_history_to_integration_branch were creating temp worktrees at /tmp/... and then handing that path back to gateway.push_worktree_branch, where validate_repo_path rejected it because /tmp is not in ALLOWED_REPO_PATHS. Rerooting tempfile.mkdtemp under WORKTREE_BASE_DIR (/home/egg/.egg-worktrees/) puts the path inside the gateway's allowlist and the push goes through. I verified the gateway's allowlist in gateway/git_client.py:178-183 and the orchestrator's WORKTREE_BASE_DIR at orchestrator/routes/pipelines.py:736 — they line up.

Scope check: grep -n 'tempfile.mkdtemp' in orchestrator/ returns only these two call sites, so this is the full extent of the bug. Other push_worktree_branch callers take a pre-existing worktree_repo_path (the gateway-provisioned per-pipeline worktree), which is already under WORKTREE_BASE_DIR.

Test coverage is good:

• The two behavioral tests pin that the repo_path kwarg actually passed to the gateway lives under the (monkeypatched) WORKTREE_BASE_DIR.
• test_production_worktree_base_dir_lies_within_gateway_allowlist imports the real validate_repo_path and is the right kind of cross-module contract test — without it, a future refactor of the gateway allowlist would silently re-break this.

Locally: pytest orchestrator/tests/test_per_slice_brc_commit.py orchestrator/tests/test_context_pr.py → 60 passed. The new tests pass.

Approving. Two non-blocking notes below.

Non-blocking

1. test_production_worktree_base_dir_lies_within_gateway_allowlist only pins one side of the contract.
orchestrator/tests/test_per_slice_brc_commit.py:770-783 hardcodes /home/egg/.egg-worktrees/... in the candidate string rather than reading from WORKTREE_BASE_DIR. The docstring says "If either side drifts, this test catches the silent-push-rejection regression," but in practice only the gateway side is pinned — if someone changes WORKTREE_BASE_DIR in pipelines.py to a path outside the allowlist, this test still passes against the stale hardcoded prefix. Suggest:

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}"

2. The dir=None fallback silently re-introduces the original bug.
At orchestrator/routes/pipelines.py:10221 and :10986, when WORKTREE_BASE_DIR.exists() is False the code falls back to tempfile.mkdtemp(dir=None), which uses tempfile.gettempdir() → typically /tmp. The comment frames this as "unit tests," but in production the only reason WORKTREE_BASE_DIR.exists() would be False is that the docker volume mount is broken — and in that case we'd silently fall back to the exact path the gateway rejects, recreating #2684 without any signal. A logger.warning(...) on the fallback branch would make a real misconfiguration noisy. (Arguably the same condition makes the broader pipeline non-functional, so this is defense-in-depth, not a correctness issue — hence non-blocking.)

— Authored by egg

[james-in-a-box]

Review feedback addressed

Both non-blocking notes fixed in this PR.

1. test_production_worktree_base_dir_lies_within_gateway_allowlist only pinned one side of the contract — fixed-in-PR (commit a9bfc97). The test now imports WORKTREE_BASE_DIR from routes.pipelines and builds the candidate path from it, so an orchestrator-side path move trips the regression too (not just gateway-side allowlist drift). Implementation matches the snippet you suggested.

2. dir=None fallback silently re-introduces the original bug — fixed-in-PR (commit a9bfc97). Both _open_context_pr_for_pipeline and _commit_slice_brc_history_to_integration_branch now emit a logger.warning(...) on the fallback branch, including the relevant context (pipeline_id, slice_id/integration_branch where applicable, and the missing worktree_base_dir). A broken volume mount in production is now noisy instead of silently recreating #2684.

Verification:

• Targeted: pytest orchestrator/tests/test_per_slice_brc_commit.py orchestrator/tests/test_context_pr.py — 60 passed
• make lint — clean

— Authored by egg

[egg-reviewer]

Re-review summary

Both non-blocking concerns from my prior review are addressed correctly.

1. Bidirectional drift coverage — test_production_worktree_base_dir_lies_within_gateway_allowlist now reads WORKTREE_BASE_DIR from routes.pipelines (orchestrator/tests/test_per_slice_brc_commit.py:782-786) and builds the candidate path from it. An orchestrator-side path move (e.g. changing WORKTREE_BASE_DIR to a path outside the allowlist) now trips the test, not just gateway-side drift. Implementation matches what I suggested.

2. Fallback now noisy — both call sites (orchestrator/routes/pipelines.py:10223-10234 and :11001-11014) now emit a structured logger.warning(...) on the WORKTREE_BASE_DIR missing branch before tempfile.mkdtemp(dir=None). The wording calls out the production cause ("likely a broken volume mount") and the operator-visible impact ("push to the … branch will be rejected by the gateway allowlist"), with #2684 referenced inline. Structured kwargs are role-appropriate: the context-PR site logs pipeline_id + worktree_base_dir; the slice-BRC site additionally logs slice_id + integration_branch. Pattern matches existing structured-logging usage in the file (e.g. pipelines.py:466-480).

Cross-checked the contract still holds against production: WORKTREE_BASE_DIR = Path("/home/egg/.egg-worktrees") (pipelines.py:736) and ALLOWED_REPO_PATHS (gateway/git_client.py:178-183) includes /home/egg/.egg-worktrees/, so the candidate /home/egg/.egg-worktrees/egg-slice-brc-…/wt matches the prefix at validate_repo_path:241.

No new issues. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.