Fix #1941: populate contract.pr on advance_phase out of plan

[jwbron]

Summary

• Extracts _populate_contract_from_plan_safe as a shared entry point for the two sites that run the populate step when leaving the plan phase.
• advance_phase (especially force=true) now invokes that helper + commits the result when previous_phase == PLAN, so _sync_worktree_with_remote in the newly-spawned _run_pipeline thread pushes the contract update via the local-ahead path instead of resetting it.
• _run_pipeline's existing post-complete call routes through the same helper; happy-path behavior is unchanged.

Fixes #1941. Before this, a force=true advance out of plan (the recovery hammer used in #1938) replaced the plan-phase _run_pipeline thread before it reached the populate step, leaving contract.pr empty and the PR phase's auto-PR path falling back to orchestrator placeholders (observed on PR #1937).

Test plan

• pytest orchestrator/tests/test_advance_phase_populate_on_plan_exit.py — new regression suite:
  • force=true advance from plan → pr calls the populate helper with the resolved worktree path
  • Populate is followed by a scoped commit with pipeline_id
  • Advances that don't leave plan skip both populate and commit
  • A populate crash does not block the advance (force=true recovery invariant)
  • Integration: with a real yaml-tasks plan draft and empty contract, the helper writes contract.pr.title and contract.pr.description
• pytest orchestrator/tests/ — full orchestrator suite passes (4314 passed, 1 skipped)
• ruff check + ruff format --check clean on all touched files

🤖 Generated with Claude Code

[egg-reviewer]

Clean, well-targeted fix for #1941. The approach is sound: extracting _populate_contract_from_plan_safe as a shared entry point ensures both advance_phase and _run_pipeline's post-complete path run the same populate logic, and the commit-before-thread-spawn guarantees _sync_worktree_with_remote pushes the change rather than resetting it.

No blocking issues. A few non-blocking observations:

1. Commit proceeds after silently failed populate (non-blocking)

phases.py:419-433 — If _populate_contract_from_plan_safe catches an exception internally (which it's designed to do), execution falls through to _commit_statefiles_to_worktree with the message "Populate contract from plan on plan-phase exit." In practice this is a no-op commit (nothing changed on disk), but the message could be misleading in git logs if someone investigates. Consider either:

• Tracking whether populate succeeded and skipping the commit when it didn't, or
• Accepting this as harmless (the commit is idempotent/no-op when nothing was staged)

I lean toward accepting it — it matches how _persist_phase_brc_history works (unconditional commit after best-effort write), and the commit helper already skips when nothing is staged.

2. Test call-order assertion gap (non-blocking)

test_populate_is_followed_by_commit verifies that both populate and commit were called with the right arguments, but doesn't assert that populate was called before commit. Since these are mocks on different objects, ordering verification would require a shared Mock manager. Minor, but the test name implies it checks ordering.

3. Inconsistent worktree resolution in the same code path (pre-existing, non-blocking)

advance_phase now calls both _persist_phase_brc_history (which uses _resolve_pipeline_worktree_path from pipelines.py:5348) and the new populate block (which uses resolve_worktree_path from routes/__init__.py:217). These are functionally equivalent but different implementations. Not introduced by this PR, just worth noting since both run in the same request now.

4. Test coverage is solid

The regression suite covers the key scenarios well:

• Force advance from plan calls populate with correct args
• Populate is followed by a scoped commit
• Non-plan advances skip populate
• Populate failure doesn't block the advance
• Integration test verifies the end-to-end yaml-tasks → contract.pr transformation

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Addressing the observations:

1. Commit after silently failed populate — Agreed this is harmless. The commit helper already skips when nothing is staged, and this matches the existing _persist_phase_brc_history pattern. Leaving as-is.

2. Test call-order assertion gap — Agreed. Added a shared call tracker (call_order list) to test_populate_is_followed_by_commit that records invocations across both mocks and asserts populate precedes the plan-exit commit. Fixed in a80fd94.

3. Inconsistent worktree resolution — Acknowledged as pre-existing. Worth a follow-up to consolidate but out of scope here.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1}

[egg-reviewer]

Re-review of changes since 434aeae (commits a80fd94 and f0beaea).

Previous feedback status

Item 1 (commit after failed populate): Author accepted as harmless. Agreed — no action needed.

Item 2 (call-order assertion gap): Fixed in a80fd94. The call_order list + cross-mock side effects correctly verify that populate is invoked before the plan-exit commit. The approach is sound:

• mock_populate.side_effect appends "populate" to the shared tracker
• _track_commit wraps the commit mock to append "commit" and delegates to the original side_effect
• The assertion finds the populate index and confirms at least one commit index follows it
• The pattern correctly handles the BRC-history commit that precedes the populate block (it appears earlier in call_order and doesn't confuse the assertion)

Item 3 (inconsistent worktree resolution): Acknowledged as pre-existing/out-of-scope. Fine.

New changes review

f0beaea — formatting fix

Trivial ruff-format adjustment to the assert line wrapping. No behavioral change.

a80fd94 — call-order assertion

No issues. The implementation directly addresses the feedback: it records invocation order across two independently-mocked functions and asserts the constraint the test name promises. One minor observation:

orig_commit_side_effect will always be None here (fresh MagicMock), so the if orig_commit_side_effect: guard is dead code — but it's a reasonable defensive pattern if someone later adds a default side_effect to the fixture, so it's fine to keep.

Verdict

No blocking or non-blocking issues in the new changes. Previous feedback has been properly addressed.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

5 previous review(s) hidden.