Fix orchestrator reading verdict files from worktree

[james-in-a-box]

Summary

The orchestrator reads verdict, draft, and check-result files from repo_path
(the main repo at /home/egg/repos/{repo}/), but reviewer containers write
these files into per-pipeline worktrees at /home/egg/.egg-worktrees/{pipeline_id}/{repo}/.
This mismatch meant verdict files were never found, causing all reviews to be
silently auto-approved.

Additionally, contract files were created at pipeline-creation time in the main
repo, polluting the host working directory with untracked .egg-state/contracts/
files.

Changes:

• Mount .egg-worktrees into the orchestrator container (docker-compose.yml)
• Compute worktree_repo_path after worktree creation in _run_pipeline
• Use worktree_repo_path for all container-written file reads: verdicts,
  drafts, check results, and contract population
• Defer contract creation from pipeline-creation endpoints into _run_pipeline
  so contracts are written to the worktree, not the main repo
• Add the worktrees volume to the integration test orchestrator service

State store and decision queue calls continue to use repo_path (backed by
a named Docker volume), unchanged.

Issue: none

Test plan:

• All 3043 unit tests pass, ruff lint clean
• Verify a pipeline run finds verdict files in the worktree
• Confirm no .egg-state/contracts/ files appear in the main repo

Authored-by: egg

[egg-reviewer]

Review of PR #617: Fix orchestrator reading verdict files from worktree

The core diagnosis is correct: the orchestrator was reading verdict/draft/check files from repo_path while containers write them into per-pipeline worktrees. Deferring contract creation and using worktree_repo_path for file reads is the right fix. However, there are issues that need to be addressed before merge.

1. _populate_contract_from_plan has an inconsistent path bug (Correctness)

File: orchestrator/routes/pipelines.py, line 1558 and 1603 (unchanged but now broken by the new call pattern)

_populate_contract_from_plan receives worktree_repo_path (correct for reading the plan draft at line 1547), but then passes it to both load_contract(contract_id, repo_path) (line 1558) and save_contract(contract, repo_path) (line 1603).

The contract was created at worktree_repo_path (lines 1748-1761), so load_contract will find it there — that part works. But consider what happens when the contract was created before the worktree path was derived (e.g., pipeline.contract_synced was already True from a previous run attempt, or a race). The function silently reads and saves at the worktree path, which is consistent now but fragile because the parameter is named repo_path and there's no documentation clarifying which path semantics are expected.

This isn't a blocking bug currently, but it's worth noting that the _populate_contract_from_plan function's internal repo_path parameter now serves dual purpose (reading drafts AND reading/writing contracts) and both must be the same path. If they ever need to diverge, this will break silently. Consider adding a comment at the call site (line 2213) clarifying this coupling.

2. Contract creation failure is silently swallowed (Correctness — Blocking)

File: orchestrator/routes/pipelines.py, lines 1770-1775

The old code in create_pipeline() treated contract creation failure as a hard error: it deleted the pipeline and returned a 500 error. The new code in _run_pipeline downgrades this to a logger.warning and continues execution:

except Exception as contract_err:
    logger.warning(
        "Failed to create contract in worktree, continuing without contract",
        ...
    )

The PR description says contracts are "required for pipeline to function." If that's true, silently continuing without one will produce confusing failures downstream (e.g., when _populate_contract_from_plan tries to load_contract and fails, or when other components expect the contract to exist). The pipeline will run partway and fail with an unrelated error, making debugging harder.

Suggested fix: Either:

• (a) Fail the pipeline immediately (set status to FAILED and break), matching the original severity, or
• (b) If contracts are truly optional now, update the PR description and add guards in downstream contract consumers (like _populate_contract_from_plan which already handles missing contracts gracefully — but verify all paths).

3. Worktree path resolution picks the first arbitrary repo (Correctness)

File: orchestrator/routes/pipelines.py, lines 1695-1699

for name in wt_result.worktrees:
    candidate = Path(f"/home/egg/.egg-worktrees/{worktree_id}/{name}")
    if candidate.exists():
        worktree_repo_path = candidate
        break

This iterates over wt_result.worktrees (a dict) and takes the first entry whose path exists. For multi-repo pipelines, this could pick the wrong repo. Dict iteration order in Python 3.7+ is insertion order, but there's no guarantee the first entry corresponds to pipeline.repo.

Currently pipelines appear to use a single repo (pipeline_repos = [pipeline.repo] if pipeline.repo else []), so in practice there's likely one entry. But the fallback behavior is fragile.

Suggested fix: Match against pipeline.repo explicitly:

repo_short = pipeline.repo.split("/")[-1] if pipeline.repo else None
if repo_short and repo_short in wt_result.worktrees:
    candidate = Path(f"/home/egg/.egg-worktrees/{worktree_id}/{repo_short}")
    if candidate.exists():
        worktree_repo_path = candidate

4. Hardcoded worktree path (Maintainability)

File: orchestrator/routes/pipelines.py, line 1696

The path /home/egg/.egg-worktrees/{worktree_id}/{name} is hardcoded. The docker-compose mount also hardcodes /home/egg/.egg-worktrees. If the gateway's worktree base directory ever changes, these will silently break. Consider extracting this as a constant or deriving it from a config/env var. Non-blocking, but worth a quick cleanup.

5. docker-compose.yml uses bind mount vs. named volume inconsistency (Operational)

File: docker-compose.yml, line 133 vs integration_tests/local_pipeline/docker-compose.yml, line 90

Production docker-compose uses a bind mount:

- ${HOST_HOME:-/home/egg}/.egg-worktrees:/home/egg/.egg-worktrees

Integration tests use a named volume:

- worktrees:/home/egg/.egg-worktrees

This is intentional (tests don't have a host filesystem), but worth a brief comment in the integration test compose file explaining the divergence, since the main compose file's comment says "read container-written artifacts" which doesn't explain why the mount strategy differs.

Non-blocking.

Summary

The fix correctly identifies and resolves the root cause (worktree path mismatch). The docker-compose changes are clean. Issue #2 (silent contract failure) is the only blocking concern — the error handling regression changes the contract from "required" to "best-effort" without adequate justification or downstream guards. The rest are non-blocking observations.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Pushed fixes addressing all points:

#1 (Path coupling comment): Added a clarifying comment at the _populate_contract_from_plan call site (line ~2224) documenting that worktree_repo_path is used for both draft reads and contract load/save, and both must be the same path.

#2 (Contract failure handling — blocking): Agreed. Changed from logger.warning + continue to logger.error + set pipeline status to FAILED + return. This restores the original severity: contract creation failure is a hard error that stops the pipeline immediately.

#3 (Worktree path resolution): Agreed. Now matches against pipeline.repo explicitly by extracting the short name (owner/repo → repo) and looking it up in wt_result.worktrees. Falls back to iterating if the repo isn't found, for safety.

#4 (Hardcoded path): Extracted /home/egg/.egg-worktrees as WORKTREE_BASE_DIR constant at module level, with a comment noting it must match the gateway's constant and docker-compose mounts.

#5 (Volume mount comment): Added a comment in the integration test docker-compose explaining the named volume vs bind mount divergence (CI has no host filesystem).

— Authored by egg

[egg-reviewer]

Re-review of PR #617: Fix orchestrator reading verdict files from worktree

All five issues from the previous review have been addressed. Four of the five fixes are correct. One has a subtle gap.

Previous feedback status

1. Path coupling comment — Fixed. Comment at line ~2227 documents the coupling. ✓
2. Contract failure handling — Fixed. logger.error + FAILED + return restores original severity. ✓
3. Worktree path resolution — Partially fixed. See below.
4. Hardcoded path — Fixed. Extracted as WORKTREE_BASE_DIR constant with comment noting it must match gateway. ✓
5. Volume mount comment — Fixed. Integration test compose now explains the named-volume divergence. ✓

Remaining issue: worktree path resolution has a dead branch (Correctness — Non-blocking)

File: orchestrator/routes/pipelines.py, lines 1701-1712

repo_short = pipeline.repo.split("/")[-1] if pipeline.repo else None
if repo_short and repo_short in wt_result.worktrees:
    candidate = WORKTREE_BASE_DIR / worktree_id / repo_short
    if candidate.exists():
        worktree_repo_path = candidate
else:
    # Fallback: take the first existing worktree path
    for name in wt_result.worktrees:
        ...

When repo_short is found in wt_result.worktrees (the if branch is true) but candidate.exists() returns False, worktree_repo_path silently remains repo_path (the default from line 1674). The else fallback doesn't execute because it's attached to the outer if, not the inner if candidate.exists().

In practice this is unlikely — if the gateway reported the worktree was created, the directory should exist. But the whole point of the candidate.exists() guard is to handle the case where it doesn't. If that guard ever triggers, the pipeline silently reverts to the original bug (reading from repo_path instead of the worktree).

Suggested fix: Either remove the inner exists() check entirely (trust the gateway), or fall through to the fallback loop when the check fails:

repo_short = pipeline.repo.split("/")[-1] if pipeline.repo else None
matched = False
if repo_short and repo_short in wt_result.worktrees:
    candidate = WORKTREE_BASE_DIR / worktree_id / repo_short
    if candidate.exists():
        worktree_repo_path = candidate
        matched = True
if not matched:
    for name in wt_result.worktrees:
        candidate = WORKTREE_BASE_DIR / worktree_id / name
        if candidate.exists():
            worktree_repo_path = candidate
            break

This is non-blocking because the scenario requires the gateway to report success while the directory is missing, which shouldn't happen in normal operation. But it's worth fixing for defensive correctness.

Summary

The feedback has been substantively addressed. The blocking issue (#2, contract failure handling) is properly fixed. The remaining observation above is non-blocking. This is ready to merge with or without the minor fix.

— Authored by egg

[james-in-a-box]

Agreed. Fixed the dead branch in the worktree path resolution. Changed else (attached to outer if) to use a matched flag so the fallback loop executes whenever the primary match fails — whether repo_short isn't found in wt_result.worktrees or candidate.exists() returns False.

— Authored by egg

[egg-reviewer]

Re-review of PR #617: Fix orchestrator reading verdict files from worktree

The branch has been squashed into a single commit (2936cc7). Reviewed the full PR diff against main, with focus on the fix for the dead-branch issue identified in the previous review.

Previous feedback status

All six issues from across both prior reviews have been addressed:

1. Path coupling comment — Fixed. Comment at lines 2229-2233 documents that worktree_repo_path is used for both draft reads and contract load/save. ✓
2. Contract failure handling — Fixed. logger.error + pipeline.status = FAILED + return at lines 1785-1794 restores original severity. ✓
3. Worktree path resolution (repo matching) — Fixed. Matches against pipeline.repo explicitly at line 1701-1703. ✓
4. Hardcoded path — Fixed. Extracted as WORKTREE_BASE_DIR constant at line 60 with comment noting it must match the gateway. ✓
5. Volume mount comment — Fixed. Integration test compose at lines 89-92 explains the named-volume divergence. ✓
6. Dead branch in fallback logic — Fixed. The matched flag pattern at lines 1702-1714 correctly falls through to the fallback loop when either (a) repo_short isn't in wt_result.worktrees, or (b) the primary candidate directory doesn't exist. ✓

Full review of current state

Traced all data flow paths through the changed code:

• Contract creation (lines 1758-1794): Correctly uses worktree_repo_path for both local and issue modes. Failure is a hard error that sets FAILED status and returns. The pipeline.contract_synced guard prevents duplicate creation on retry.
• Contract population (line 2235-2236): _populate_contract_from_plan receives worktree_repo_path, and both load_contract and save_contract inside it use the same path — consistent with contract creation location.
• File reads (lines 1998, 2078, 2163, 2242): All four artifact reads (_read_check_results, verdict path, _read_review_verdict, _read_phase_draft) correctly use worktree_repo_path.
• Remaining repo_path usages in _run_pipeline: get_state_store(repo_path) (lines 1638, 2326) and get_decision_queue(pipeline_id, repo_path) (line 2254) correctly use the original repo path — StateStore operates on its own persistent worktree and doesn't read from the pipeline worktree.
• Docker-compose mounts: Both production (bind mount at line 133) and integration test (named volume at line 93) correctly share the worktrees directory between gateway and orchestrator, matching the existing gateway mount.
• WORKTREE_BASE_DIR constant matches the gateway's definition in gateway/worktree_manager.py:45.

No new issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.