Enforce contract existence before implement phase

[james-in-a-box]

Coordinator-mode pipelines must always have a contract before work begins.
Without a contract, reviewers have nothing to review against, the
reviewer_contract role is meaningless, and phase gates cannot validate
that work aligns with requirements.

This adds an orchestrator-side guardrail in the coordinator phase-advance
endpoint that blocks advancement to implement or pr phase when
contract_synced is false (HTTP 409). The coordinator agent rules are
also updated to document contract existence as a hard requirement rather
than a judgment call.

Contracts are already created automatically during pipeline startup in
_run_pipeline(). This guardrail catches the edge case where contract
creation failed silently or was skipped, preventing agents from starting
work without a shared source of truth.

Issue: #1070

Test plan:

• 5 new functional tests in test_coordinator_routes_functional.py
• test_advance_to_implement_blocked_without_contract — sequential advance blocked
• test_skip_to_implement_blocked_without_contract — skip-to-target blocked
• test_skip_to_pr_blocked_without_contract — PR phase also guarded
• test_advance_to_implement_allowed_with_contract — happy path still works
• test_advance_to_plan_allowed_without_contract — early phases unaffected
• All 48 tests pass

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1, "Lint/Custom Checks": 1}

[egg-reviewer]

Review: Enforce contract existence before implement phase

The core feature — blocking phase advancement to implement/pr when contract_synced is false — is sound and well-motivated. The tests are thorough and the coordinator rules documentation is a good addition. However, this PR has two blocking issues that must be fixed before merge.

---

BLOCKING: Massive accidental worktree commit

916 files, 308,000+ lines from .claude/worktrees/agent-acada541/ are committed to this PR. This is the entire contents of an agent worktree — full source code copies, .egg-state/ pipeline data, contracts, drafts, agent outputs, check results, and more. These files comprise 99.9% of the diff.

Additionally, .claude/settings.local.json (contains user-specific permission settings including local filesystem paths like /home/jwies/khan/webapp/) and .claude/scheduled_tasks.lock (session-specific runtime state) are committed.

Fix: Remove all three from the branch:

git rm -r --cached .claude/worktrees/ .claude/settings.local.json .claude/scheduled_tasks.lock

Prevent recurrence: Add to .gitignore:

.claude/worktrees/
.claude/settings.local.json
.claude/scheduled_tasks.lock

---

BLOCKING: Contract check runs after state mutation

orchestrator/routes/coordinator.py — The contract enforcement check at the new code block runs after pipeline.current_phase = target_phase has already been set (lines ~664 and ~676 in the if/else branches). If the check fails, the error is returned without calling store.save_pipeline(), so no persistent corruption occurs. However, the pipeline object is left in a mutated state when the error response is constructed, which is a maintenance hazard.

The check is a precondition — it should execute before the state mutation. Refactor to compute target_phase and action without mutating pipeline.current_phase, run validations (including the contract check), then apply the mutation:

# In both branches, compute target_phase and action
# WITHOUT setting pipeline.current_phase yet
if target_phase_str:
    # ... compute target_phase, action ...
    # REMOVE: pipeline.current_phase = target_phase
else:
    # ... compute target_phase, action ...
    # REMOVE: pipeline.current_phase = target_phase

# Validate contract BEFORE mutating state
if (
    target_phase in (PipelinePhase.IMPLEMENT, PipelinePhase.PR)
    and not pipeline.contract_synced
):
    return make_error_response(...)

# NOW mutate
pipeline.current_phase = target_phase

This makes the validation-then-mutation ordering explicit and prevents future code between the mutation and the check from seeing incorrect state.

---

Non-blocking observations

uv.lock change is out of scope: The lockfile was re-resolved changing requires-python from >=3.11 to >=3.13 and dropping ~314 lines of old wheel entries. This is correct (matches pyproject.toml) but unrelated to contract enforcement. Consider splitting into a separate commit for cleaner history.

check-hardcoded-ports.py: Adding .claude to the skip list is reasonable — the worktree directory can contain source copies that produce false positives. This fix is correct regardless of whether the worktree files are removed from the PR.

Tests: The 5 new tests in TestContractEnforcement cover the key scenarios well: sequential advance blocked, skip-to-implement blocked, skip-to-PR blocked, happy path with contract, and early phases unaffected. Test structure matches existing patterns. No issues.

Coordinator rules: The new "Contract Requirement (CRITICAL)" section in sandbox/.claude/rules/coordinator.md is clear and actionable. No issues.

— Authored by egg

[james-in-a-box]

Both blocking issues have been addressed in 6a8fd5f:

1. Accidental worktree commit — Removed all 916 worktree files, settings.local.json, and scheduled_tasks.lock via git rm -r --cached. Added .claude/worktrees/, .claude/settings.local.json, and .claude/scheduled_tasks.lock to .gitignore to prevent recurrence.

2. Contract check after state mutation — Refactored the coordinator phase endpoint so target_phase and action are computed in both branches without mutating pipeline.current_phase. The contract enforcement check now runs as a precondition before any state mutation. pipeline.current_phase = target_phase is applied only after all validations pass.

Non-blocking notes:

• uv.lock change: Acknowledged as out of scope but leaving as-is since it correctly aligns the lockfile with pyproject.toml.
• check-hardcoded-ports.py .claude skip: Agreed it's correct regardless of worktree cleanup.

All 48 coordinator route tests pass including the 5 TestContractEnforcement tests.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

The contract enforcement guardrail is technically enforced at the orchestrator API level (HTTP 409), not via prompt-level instructions — this aligns well with the core principle that the sandbox is the constraint. The coordinator rules update correctly documents the behavior rather than relying on the agent to self-enforce.

— Authored by egg

[egg-reviewer]

Re-review: Both blocking issues resolved

Verified the delta since 2f2e37b (commit 6a8fd5f).

Previously blocking issues — now resolved

1. Worktree files removed. The 916 accidentally committed worktree files, settings.local.json, and scheduled_tasks.lock are all gone. The PR now contains only 6 files. .gitignore entries added to prevent recurrence.

2. Contract check now runs before state mutation. The refactoring is correct: both pipeline.current_phase = target_phase assignments were removed from the if/else branches. target_phase and action are now computed without mutation, the contract enforcement check runs as a precondition, and pipeline.current_phase = target_phase is applied exactly once after the "All validations passed" comment. Clean separation of validation and mutation.

Current state of the PR

The remaining diff is clean:

• .gitignore: Three new entries — correct.
• orchestrator/routes/coordinator.py: Contract enforcement guard at the right location. Returns 409 with a clear error message. Guards both IMPLEMENT and PR phases, which is correct since you shouldn't reach PR without a contract either.
• Tests: 5 tests covering sequential advance blocked, skip-to-implement blocked, skip-to-PR blocked, happy path with contract, and early phase unaffected. All use explicit contract_synced assignment, which is correct since the model default (True) differs from the state store creation default (False).
• sandbox/.claude/rules/coordinator.md: Clear documentation of the contract requirement.
• scripts/check-hardcoded-ports.py: Adding .claude to the skip set is correct.
• uv.lock: Out of scope (acknowledged previously) but harmless.

No new issues found.

Non-blocking suggestion

There's no test for the loopback case (e.g., PR → IMPLEMENT without a contract). The code handles it correctly since the check is phase-target-based regardless of action type, but a test_loopback_to_implement_blocked_without_contract would strengthen coverage.

— Authored by egg

[james-in-a-box]

Agreed. Added test_loopback_to_implement_blocked_without_contract covering the PR → IMPLEMENT loopback case (c3ba656). All 49 coordinator route tests pass.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

Delta since last review (6a8fd5f→c3ba656) is a single test addition (test_loopback_to_implement_blocked_without_contract). No agent-mode design changes.

— Authored by egg

[egg-reviewer]

Loopback test is correct. Follows the same mocking and assertion pattern as the other contract enforcement tests. Exercises the target_phase code path with a PR → IMPLEMENT transition, which correctly hits the contract guard.

No issues found. All previous feedback has been addressed.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
sandbox/.claude/rules/coordinator.md	Additive	Both sections included — PR's "Contract Requirement" section and main's "Phase-Role Mappings" section

Details: Both the PR and main added new sections at the same location (after "Available Tools"). The PR added a "Contract Requirement (CRITICAL)" section documenting that contracts must exist before implement phase. Main added a "Phase-Role Mappings (CRITICAL)" section documenting valid role-phase combinations. These are independent, complementary documentation additions — both are included in order (contract requirement first, then phase-role mappings).

Auto-merged files (no conflicts): orchestrator/routes/coordinator.py, orchestrator/tests/test_coordinator_routes_functional.py

Verification: Lint passes. All 56 coordinator functional tests pass (including both the PR's 6 contract enforcement tests and main's 7 phase-role validation tests). Full orchestrator suite: 1926 passed, 5 pre-existing failures (missing starlette module, unrelated).

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

Delta since last review (c3ba656→dfa95d6) is a merge of origin/main resolving a conflict in coordinator.md. The merge correctly integrates both the contract enforcement section (this PR) and the phase-role mappings section (from #1071) without duplication or ordering issues. No agent-mode design changes in the delta.

— Authored by egg

[egg-reviewer]

Re-review: Merge conflict resolution verified

Delta since c3ba656 is a single merge commit (dfa95d6) that incorporates main into the PR branch. The merge brought in PR #1071 (phase-role validation) and PR #1073 (eliminate local pipeline mode).

Merge conflict resolution — correct

The only manual conflict was in sandbox/.claude/rules/coordinator.md. Main (via PR #1071) replaced the "Contract Requirement" section with "Phase-Role Mappings". This PR had the "Contract Requirement" section. The resolution correctly includes both sections in order: Contract Requirement first, then Phase-Role Mappings. Both are independent, complementary documentation — no content was lost.

Auto-merged files — correct

orchestrator/routes/coordinator.py: Main (PR #1073) moved pipeline.current_phase = target_phase back into both if/else branches and removed the contract enforcement check. The merge correctly resolved this in the PR's favor: both branch-level mutations are removed, the contract enforcement check remains as a precondition, and pipeline.current_phase = target_phase is applied exactly once after the "All validations passed" comment. The validation-then-mutation ordering from my first review's feedback is preserved.

orchestrator/tests/test_coordinator_routes_functional.py: Main added TestPhaseRoleValidation tests. The PR had TestContractEnforcement tests. Both test classes appear cleanly in the merged result — no overlap or conflict.

Previous feedback — still addressed

1. Worktree files removed, .gitignore entries in place — confirmed.
2. Contract check runs before state mutation — confirmed in final diff.
3. Loopback test present — confirmed.

Full PR diff against main

The final diff is clean and minimal: 5 files changed with only the intended additions (.gitignore entries, contract enforcement guard, 6 contract enforcement tests, coordinator rules documentation, and .claude skip in hardcoded-ports check).

No issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

20 previous review(s) hidden.