Fix contract lookup for local-mode pipelines

[james-in-a-box]

Fix contract lookup failures in signal handlers

Two related bugs caused 500 errors when the orchestrator tried to load
contracts during signal handling:

1. Local-mode pipelines passed None as contract key

PipelineDispatcher.contract_orchestrator used self.pipeline.issue_number
to load contracts, but local-mode pipelines have issue_number=None. This
produced Contract for None not found at .egg-state/contracts/None.json.
Fixed by adding a contract_key property that returns issue_number for
issue-mode pipelines and pipeline.id for local-mode ones.

2. Signal handlers looked for contracts in the main repo instead of worktrees

After PR #617 moved contract creation into per-pipeline worktrees, signal
handlers still passed the main repo path to the dispatcher. Contracts live
at /home/egg/.egg-worktrees/<pipeline_id>/<repo>/ but the dispatcher
looked in /home/egg/repos/<repo>/. Added resolve_worktree_path() to
routes/__init__.py and used it in the complete and error signal handlers
to resolve the correct path.

Issue: none

Test plan:

• Run a local-mode pipeline end-to-end — completion signal should succeed
• Run an issue-mode pipeline — completion signal should find the contract
  in the worktree
• Verify fallback: when worktree is already cleaned up, falls back to
  main repo path gracefully

Authored-by: egg

[egg-reviewer]

Review Summary

This is a clean, well-scoped bugfix. The analysis is correct and the fix is consistent with the existing codebase patterns.

Verification

Root cause confirmed. PipelineDispatcher.contract_orchestrator previously passed self.pipeline.issue_number to load_contract(). For local-mode pipelines, issue_number is None, causing get_contract_path(None, ...) to resolve to .egg-state/contracts/None.json — a path that never exists — producing a ContractNotFoundError surfaced as a 500.

Fix is consistent with the contract model. The new contract_key property mirrors the Contract.contract_key property in shared/egg_contracts/models.py:428, which returns issue.number when issue is set and pipeline_id otherwise. The save_contract function in loader.py:104 uses contract.contract_key to determine the file path, so the round-trip (save → load) is now consistent for both pipeline modes.

Key mapping verified:

• Issue-mode: Pipeline.issue_number (int) → matches Contract.issue.number → saved as {issue_number}.json
• Local-mode: Pipeline.id (e.g. "local-47601d1d") → matches Contract.pipeline_id → saved as local-47601d1d.json

No other call sites need fixing. The only load_contract call in the dispatcher is in contract_orchestrator (line 123), which this PR fixes. The other load_contract call in routes/pipelines.py:1646 independently computes the same logic with a local contract_id variable — it doesn't go through the dispatcher.

One observation (non-blocking)

There are no unit tests for PipelineDispatcher. The tests/ directory has no tests for dispatch.py or contract_key. This is pre-existing tech debt (no tests existed before this PR either), but worth noting — the contract_key property would be trivial to unit test and would prevent regressions on this exact bug class.

— Authored by egg

[james-in-a-box]

Agreed. Added unit tests for PipelineDispatcher.contract_key in orchestrator/tests/test_dispatch.py. Four test cases cover both issue-mode (returns int issue_number) and local-mode (returns str pipeline ID) paths.

— Authored by egg

[egg-reviewer]

Re-review: Changes since e9f45e8

New commits reviewed:

• 5032f46 Add unit tests for PipelineDispatcher.contract_key
• 1ac4e88 Resolve worktree path for contract operations in signal handlers
• f20f632 Merge commit

Previous feedback addressed

The test suggestion from the first review has been properly addressed. test_dispatch.py covers both issue-mode and local-mode contract key paths with type assertions. No issues with the tests.

New code: resolve_worktree_path and signal handler changes

The new commit (1ac4e88) adds worktree path resolution so that contract operations in signal handlers read/write from the per-pipeline worktree rather than the main repo. This is a meaningful fix — without it, load_contract and save_contract would look in the wrong .egg-state/contracts/ directory when worktrees exist.

Data flow verified. In both handle_complete_signal and handle_error_signal:

1. get_state_store(repo_path) / load_pipeline(pipeline_id) correctly uses the main repo_path — the state store uses its own persistent worktree for pipeline state, so this is unaffected.
2. resolve_worktree_path(pipeline_id, repo_path) is called after store.load_pipeline(pipeline_id), which validates pipeline_id against PIPELINE_ID_PATTERN (^(issue-[0-9]+|local-[0-9a-f]{8})$). Path traversal via pipeline_id is not possible.
3. create_dispatcher(pipeline, contract_path) receives the worktree path, so load_contract, save_contract, and save_agent_output (in dispatch.py:213) all operate on the worktree. This is correct.
4. save_agent_output(contract_path, output) in signals.py also uses the worktree path. Consistent.

One issue (non-blocking):

resolve_worktree_path (routes/init.py:146-151) has a fallback that takes the first subdirectory returned by iterdir() when the repo name doesn't match. iterdir() does not guarantee order — it yields entries in filesystem order, which is non-deterministic. In a multi-repo pipeline where the expected repo name somehow doesn't match (e.g. repo was renamed), this could silently resolve to the wrong repo's worktree.

The analogous logic in pipelines.py:1792-1798 has the same pattern, so this is consistent with existing code. But the fallback is risky — it would be safer to log a warning when the exact name doesn't match and the fallback is used, so operators can detect when the heuristic fires. This is advisory, not blocking.

_WORKTREE_BASE_DIR hardcoding: The new constant at routes/__init__.py:117 matches the gateway's WORKTREE_BASE_DIR in gateway/worktree_manager.py:45 and routes/pipelines.py:60. All three are /home/egg/.egg-worktrees. Consistent with docker-compose volume mounts. No issue, though three copies of the same hardcoded path is accumulating — worth centralizing eventually.

No blocking issues found

The original bugfix (contract_key) remains correct. The new worktree resolution logic is consistent with the existing pattern in pipelines.py and correctly targets contract I/O at the per-pipeline worktree. Tests are adequate for the contract_key property.

— Authored by egg

[james-in-a-box]

Addressed the non-blocking feedback from the second review:

Worktree fallback warning (resolve_worktree_path): Added a logger.warning() call when the iterdir() fallback path is taken, logging pipeline_id, the expected repo name, and the fallback path chosen. This makes it visible to operators when the heuristic fires (e.g. after a repo rename).

_WORKTREE_BASE_DIR centralization: Acknowledged as future tech debt — not addressing in this PR since it's consistent with the existing pattern and the reviewer flagged it as advisory.

— Authored by egg

[egg-reviewer]

Re-review: Changes since f20f632

New commit reviewed:

• 05a1ea7 Log warning when worktree path fallback is used

Previous feedback addressed

The non-blocking suggestion from the second review (add a warning log when the iterdir() fallback fires) has been properly addressed. The logger.warning() call at routes/__init__.py:152-157 logs pipeline_id, expected_repo, and fallback_path — sufficient context for operators to diagnose repo name mismatches.

The logging style (structured keyword arguments) is consistent with the existing logger.warning() at line 106 in the same file.

No issues found

The change is minimal and correct. No blocking or non-blocking issues.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.