Fix auto-PR path resolution and consensus-stall recovery short-circuit

[james-in-a-box]

Fix auto-PR path resolution and consensus-stall recovery short-circuit

Two bugs that compounded to permanently stall pipeline issue-1748 after BRC consensus completed (5 implement agents CONFIRMED, but no PR opened, no phase transition, until the overseer manually opened PR #1756 as a fallback). Issue #1749 has the full timeline.

Proximate — .git-presence path toggle. routes/pipelines.py:1296-1298 and mcp_tools.py:1150-1152 both used base_path / repo_name if not (base_path / ".git").exists() else base_path. A stray .git at EGG_REPO_PATH silently flipped meaning and resolved to the parent containing all repos instead of the named repo subdir, breaking downstream git/PR machinery. Replaced both call sites with a new resolve_worktree_repo_path helper that prefers the named subdir, falls back to base only when base itself is a git repo, and raises RuntimeError with a specific message otherwise.

Latent — tracker-reconstruction short-circuit. container_monitor.py:_attempt_tracker_reconstruction returned True (Track 1 succeeded — skip aggressive recovery) just because get_peer_consensus_tracker(pipeline_id) is not None. But ConsensusStallCheck only fires DEGRADED when an existing tracker is already is_complete=True, so this is precisely the case where Track 1 has nothing to do and Track 2 must drive the transition. Now an existing complete tracker (or a broken tracker.evaluate()) falls through to Track 2; the reconstruction-after-missing path is unchanged. Also bumped the Track 1 success log from INFO to WARNING and named the track so future stalls are spottable from logs.

Closes #1749.

Test Plan

• Automated:
  • orchestrator/tests/test_resolve_worktree_repo_path.py (new, 7 tests): both-git-repos prefers named subdir; only-subdir-is-git returns subdir; only-base-is-git falls back to base; subdir-without-git falls back to base; neither-is-git raises with specific message; empty repo_name still works when base is the repo, and raises when not.
  • orchestrator/tests/test_consensus_stall_check.py (3 new tests): existing complete tracker triggers Track 2 (the headline regression); existing incomplete tracker still skips Track 2; broken tracker.evaluate() falls through to Track 2.
  • Existing test_tracker_reconstruction_success_skips_aggressive still passes (covers the tracker-missing path, unchanged).
  • Full orchestrator suite: 3630 passed.
• Manual: none required — pure orchestrator state-machine fix.

Manual Steps (if any)

None.

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1}

[egg-reviewer]

Thorough, well-scoped fix. Both bugs are clearly identified, the changes are minimal, and the tests cover the critical regression paths. Approving.

Path resolution fix (resolve_worktree_repo_path)

The old pattern base_path / repo_name if not (base_path / ".git").exists() else base_path had inverted priority — a stray .git at base_path would short-circuit to the parent directory. The new helper checks the more-specific path first (base_path / repo_name / .git), falls back to base only when base itself is the repo, and raises RuntimeError when neither works. Both call sites (mcp_tools.py:1150, pipelines.py:1392) are inside try/except Exception blocks so the RuntimeError propagates safely.

No remaining instances of the old pattern in production code — confirmed by search.

Stall recovery fix (_attempt_tracker_reconstruction)

The three-way return logic is correct:

• Tracker exists, is_complete=True → False (Track 2 fires — this is the fix)
• Tracker exists, incomplete → True (polling loop continues)
• Tracker broken (evaluate() raises) → False (Track 2 fires)
• Tracker missing → attempt reconstruction

This matches the ConsensusStallCheck invariant: DEGRADED only fires when the tracker reports complete but the phase hasn't transitioned, so returning "success" for an already-complete tracker was always a no-op that suppressed the actual recovery path.

One non-blocking suggestion

Docstring inaccuracy in _attempt_tracker_reconstruction (container_monitor.py:339-349): The docstring says "Returns True only when Track 1 actually has something useful to do: the tracker was missing and we successfully rebuilt it from messages." But the code also returns True when the tracker exists and reports is_complete=False (line ~367). The docstring should cover both True return paths to avoid misleading future readers. Something like:

Returns True when the polling loop can handle things: either the tracker was missing and we successfully rebuilt it, or it exists but reports incomplete (the polling loop will keep watching).

— Authored by egg

[james-in-a-box]

Agreed. Updated the docstring to cover both True return paths: tracker missing + successfully rebuilt, and tracker exists but reports incomplete. Also explicitly mentions the False return on evaluate() raising. Fixed in 9113bb4.

— Authored by egg

[egg-reviewer]

Re-reviewed delta since d315f8c. The only change is commit 9113bb4: a docstring update to _attempt_tracker_reconstruction that addresses my previous suggestion.

The docstring now correctly documents all return paths:

• True: tracker missing and successfully rebuilt, OR tracker exists but reports incomplete
• False: tracker exists and reports is_complete=True, OR tracker.evaluate() raises

No code logic changes. Previous approval stands — the path resolution fix and stall recovery fix are both correct. Suggestion addressed accurately.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.