Fix #2685: skip PR-phase auto-PR in slice-DAG mode; include contract on context PR

[jwbron]

Summary

Two related fixes to slice-DAG PR topology, as called out in #2685:

• Gate the PR-phase auto-PR on slice-DAG mode. A new _should_skip_pr_phase_auto_pr helper at the PR-phase entry mirrors the implement-phase slice-loop gate (_use_slice_loop = _slice_count > 1). When the contract has more than one slice, per-slice PRs already stack on the context PR (Slice PRs are missing analysis/plan docs and all BRC history; need a 'context' PR for refine+plan phases plus per-slice BRC in each slice PR #2548), so the legacy <pipeline_branch> → main auto-PR would only duplicate the program-level surface. Babysit-pr keeps its existing skip; single-slice / zero-slice contracts keep the legacy auto-PR; contract-load failures fail safe to running the legacy path so a transient read hiccup doesn't drop the PR silently.
• Include the contract JSON on the context PR diff. _gather_context_pr_files now resolves the contract path through the loader (get_contract_path canonical + _legacy_contract_path fallback) rather than the static glob templates — the canonical issue-<N>.json filename can't be expressed by {identifier} substitution. Reviewers approving the context PR now see the structured slice DAG alongside the prose drafts and BRC transcripts that produced it.

Both fixes are unblocked by #2690 (merged), which moved the context-PR / per-slice-BRC temp worktrees under WORKTREE_BASE_DIR so their gateway pushes actually land — before that, the context PR was opening with no files on the branch.

The PR-phase still records phase timing and runs its housekeeping (statefile commit, BRC history rewrite, gateway push) in slice-DAG mode; only the gh pr create step is skipped.

Test plan

• pytest orchestrator/tests/test_auto_pr.py orchestrator/tests/test_context_pr.py — 98 passed
• pytest orchestrator/tests/test_auto_pr.py::TestShouldSkipPrPhaseAutoPr — 6 new tests pass (babysit short-circuit, slice-DAG > 1, single-slice, zero-slice, fail-safe on load raise, babysit-wins-over-slice-DAG)
• pytest orchestrator/tests/test_context_pr.py::TestOpenContextPRAdversarial — 13 passed (3 new contract-glob tests: canonical issue-N.json, legacy <N>.json fallback, qualified issue-N-v2.json)
• pytest orchestrator/tests/test_pipeline_failure_path.py orchestrator/tests/test_slice_run_loop_integration.py — 64 passed (regression-related modules)
• ruff check + ruff format --check clean on changed files
• Operator validation: re-run a multi-slice pipeline end-to-end and confirm (a) only per-slice PRs + context PR are opened, no egg/<id>/work → main PR, and (b) the context PR diff includes .egg-state/contracts/issue-<N>.json

Closes #2685.

[egg-reviewer]

Summary

Reviewed thoroughly. Both fixes are correct, well-tested, and additive — they don't change behavior for monolithic or single-slice pipelines, and they correctly mirror the existing implement-phase slice-loop gate. Tests pass locally (pytest orchestrator/tests/test_auto_pr.py orchestrator/tests/test_context_pr.py → 98 passed; regression suites → 64 passed).

No blocking issues found. A handful of non-blocking observations below.

What I verified

Correctness of the gate (_should_skip_pr_phase_auto_pr)

• Babysit short-circuit returns before the contract loader is touched (mcp_load.assert_not_called() in the test pins this).
• The slice_count > 1 predicate matches the implement-phase gate at line 19921 exactly, so the two gates can never disagree about whether a pipeline is in slice-DAG mode (within a single tick).
• Fail-safe semantics on contract-load errors are correct: implement-phase falls back to monolithic on the same error → no per-slice PRs created → legacy auto-PR is the right path. This matches the documented fail-safe shape.
• Boundary on slice_count == 1 (monolithic) and slice_count == 0 (pre-slice-DAG) is covered by named tests.

Correctness of the gather change (_gather_context_pr_files)

• Identifier regex check at line 9945 runs before the new contract path resolution, so the path-traversal defense remains intact (verified by test_gather_rejects_path_traversal_identifier).
• Symlink check is preserved on the new contract paths (cp.is_symlink(): continue before cp.is_file()).
• All three loader path shapes are covered: canonical issue-<N>.json for int identifiers, legacy <N>.json fallback, and qualified issue-<N>-<qualifier>.json for CUSTOM-mode pipelines.
• The existing test_files_copied_include_all_curated_artifacts uses missing = expected - rel (one-sided), so adding the contract to the result set doesn't break it.

Cross-module wiring

• skip_pr_creation = True (slice-DAG branch) leaves phase_failed = False, so the phase still completes normally and the run loop transitions out of PR. Housekeeping (statefile commit, BRC history rewrite, gateway push) still runs as the PR body claims.
• The static-glob inventory test (test_static_glob_inventory_matches_documented_artifact_set) was updated correctly to reflect that the contract is now dynamically resolved, with a defensive assertion that contract paths stay out of the static set.

Non-blocking observations

1. Unreachable defensive fallback at orchestrator/routes/pipelines.py:19809:

   skip_reason=_skip_reason or "babysit_pr_already_exists",

   skip_pr_creation is True only when the gate already populated _skip_reason (babysit → "babysit_pr_already_exists", slice-DAG → "slice_dag_mode_slice_count=N"). The or fallback is dead. It does no harm, but consider just skip_reason=_skip_reason.

2. Docstring wording at _should_skip_pr_phase_auto_pr:

   (The head-move guard still runs upstream and may force the skip independently.)

   The head-move guard at line 19610 runs downstream of this helper (the call site calls the gate first at 19575, then runs the head-move guard). "Upstream" reads as misleading. Suggest "still runs at the call site after this helper returns" or similar.

3. Private-API import. The gather change imports _legacy_contract_path from egg_contracts.loader with # type: ignore[attr-defined]. There is no public equivalent because the loader normally only exposes load_contract (which hides the fallback internally). This is fine for now, but if the loader is refactored, the gather will silently lose its legacy-path fallback. Optionally, the loader could expose a get_contract_path_candidates(identifier, repo_root) helper that returns [canonical, legacy_or_none_filtered] and have both call sites consume that. Not necessary for this PR.

4. Both-files-present edge case. If both .egg-state/contracts/issue-2685.json and .egg-state/contracts/2685.json exist on disk (e.g., during a partial migration), the new code adds both to the context PR, whereas the loader's load_contract picks canonical only. In practice this is benign — the reviewer would just see the migration in the diff — but it's a slight semantic divergence from the loader. Not worth changing.

5. Log-message specificity. The previous PR-phase skip log was "Skipping PR creation (babysit-pr already has a PR)". The new generic "Skipping PR creation" plus skip_reason= is structurally cleaner but breaks log-grep alerts that may have keyed on the old literal. If any operator dashboards key on the old string, they'll need updating. Minor observability migration concern.

Pre-existing observation (not a regression)

The two contract-loader imports — one in _should_skip_pr_phase_auto_pr at line 8322, one in _gather_context_pr_files at line 9988 — both repeat the import-inside-function pattern from the existing slice-loop gate at line 19913. This is consistent and intentional (lazy import), but the codebase now has three near-identical "try import egg_contracts.loader; on failure, log and degrade" blocks for the same module. If a future refactor consolidates, this PR's gate and gather can be folded in. Not in scope here.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Per-item disposition:

1. Unreachable defensive fallback at pipelines.py:19809 — fixed-in-PR (commit 0443c9b). Dropped the or "babysit_pr_already_exists" tail; _skip_reason is always populated when skip_pr_creation is True (the gate sets it to babysit_pr_already_exists or slice_dag_mode_slice_count=N).

2. Docstring wording — "upstream" is misleading — fixed-in-PR (commit 0443c9b). Changed to "still runs at the call site after this helper returns". The head-move guard at line 19610 runs downstream of the gate at line 19575.

3. Private-API import of _legacy_contract_path — disagree (reviewer explicitly noted "Not necessary for this PR"). The # type: ignore[attr-defined] comment plus the lazy-import-in-try pattern already document the private-API risk; consolidating to a public helper is a loader-side refactor that should land with the future consolidation of the three near-identical lazy-import blocks (your pre-existing observation), not here.

4. Both-files-present edge case — disagree (reviewer explicitly noted "Not worth changing"). Including both files during a partial migration is benign — reviewers see the migration in the diff. Matching the loader's canonical-only semantics would require either calling the loader twice (once for canonical, once for legacy) or duplicating its precedence logic, both of which add complexity for a transient on-disk state.

5. Log-message specificity / observability migration — disagree (the structured "skip_reason" field is the supported surface; literal-string log grep was never a stable contract). Operators keying on free-form log strings instead of structlog fields is fragile by construction; the new emission strictly improves the structured surface (always-present skip_reason with two well-known values) and any dashboards depending on the old literal should migrate to skip_reason=babysit_pr_already_exists.

6. Pre-existing observation: three near-identical lazy-import blocks — disagree (reviewer explicitly noted "Not in scope here"). Consolidating the lazy-import pattern across the implement-phase gate, the new PR-phase gate, and the gather helper is a worthwhile refactor but belongs in its own PR — fits naturally with observation Phase 1.5: Documentation extraction #3 (public helper for contract path candidates).

Tests still pass (98/98 in test_auto_pr.py + test_context_pr.py); ruff clean on the changed file.

— Authored by egg

[james-in-a-box]

Check Failure: Integration Tests / Integration Tests

What's failing: The Docker make build step fails while installing python3.14-venv and python3.14-dev from the deadsnakes PPA.

Root cause: Transient infrastructure failure — https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu jammy InRelease returned HTTP 503 Service Unavailable during apt-get update. With no fresh package index, apt-get install could not locate python3.14-venv / python3.14-dev and exited 100, failing both the repo-deps and base Docker stages.

Excerpt from the failed log:

#7 89.93 Err:5 https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu jammy InRelease
#7 89.93   503  Service Unavailable [IP: 185.125.189.187 443]
#7 90.85 W: Failed to fetch ... 503  Service Unavailable
#7 92.09 E: Unable to locate package python3.14-venv
#7 92.09 E: Unable to locate package python3.14-dev
ERROR: failed to build: ... exit code: 100
make: *** [Makefile:470: build] Error 1

Why this is not a code issue: This PR only modifies orchestrator/routes/pipelines.py, orchestrator/tests/test_auto_pr.py, and orchestrator/tests/test_context_pr.py — none of which touch the Dockerfile, build infrastructure, or apt sources. The previous Test workflow run on this same branch (run 25765323465, ~20 minutes earlier) succeeded with the same Dockerfile.

What needs to be done:

• Re-run the failed Integration Tests job. The autofixer GitHub App lacks actions: write, so it cannot trigger the rerun itself.
• No code change required.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Integration Tests / Integration Tests": 1}

[egg-reviewer]

Re-review (delta since f65d1dd)

Re-reviewed commit 0443c9b against my prior feedback. Both addressable items are fixed correctly; the rest of the disposition is sound.

Items addressed

1. Dead or "babysit_pr_already_exists" fallback at orchestrator/routes/pipelines.py:20505 — fixed. The fallback is now provably unreachable, not just unused:

   • skip_pr_creation = True is reachable only via if is_babysit_mode: (line 20305) or elif _skip_decision: (line 20342).
   • The babysit branch implies the helper was called with is_babysit_mode=True and short-circuited to _skip_reason = "babysit_pr_already_exists" at pipelines.py:8376.
   • The _skip_decision branch (with is_babysit_mode=False) is only entered when the helper went through the slice-count check and returned _skip_reason = "slice_dag_mode_slice_count=N" at pipelines.py:8402.
   • The log line at 20505 therefore always sees a non-None _skip_reason. Good.

2. Docstring direction at _should_skip_pr_phase_auto_pr (pipelines.py:8361-8362) — fixed. "still runs upstream" → "still runs at the call site after this helper returns". The head-move guard at 20296-20341 runs after the helper returns at 20271, so the new wording is accurate.

Items declined

The author appropriately declined the other four non-blocking observations (private-API _legacy_contract_path import, both-files-present edge case, log-string observability migration, three near-identical lazy-import blocks). My prior review explicitly tagged each as out-of-scope or not worth changing, so the disagreements are warranted.

Other delta

The merge of main (fcaf3d6) brings in unrelated refactors (LIVE_POD_STATUSES, integration regression tests, etc.) that do not touch the PR's logic surface. _should_skip_pr_phase_auto_pr, _gather_context_pr_files, and the call site at 20271 are intact. No conflicts.

Tests

The pinning tests in test_auto_pr.py:770-838 continue to cover the helper's contract directly (babysit short-circuit with mock_load.assert_not_called(), slice-count > 1 → skip with slice_dag_mode_slice_count=N reason, slice-count ∈ {0, 1} → don't skip, contract-load raise → don't skip, defensive babysit-wins-over-slice-DAG). The log-line change doesn't require new test coverage — the field-shape contract is unchanged (always-present skip_reason).

No new issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

5 previous review(s) hidden.