docs: expand populate_contract error reason codes [doc-updater]

[james-in-a-box]

Update docs/reference/orchestrator-cli.md to reflect changes from #2628 (commit 81e9cb2):

• Replace the single generic populate_contract_failed | 500 reason code with the six structured reason codes now returned by the endpoint:
  • draft_missing (404) — plan draft not found in worktree
  • no_draft_path (404) — worktree has no draft path configured
  • parse_failed (422) — draft found but could not be parsed
  • empty_result (422) — draft parsed but produced zero tasks
  • contract_load_failed (500) — populated but unreadable
  • egg_contracts_unavailable (500) — package missing
  • unexpected_exception (500) — unhandled exception

Previously the endpoint silently returned 200 for all outcomes or 500 for unhandled exceptions via the outer catch block. The #2628 changes introduced PopulateOutcome as a structured discriminator, making the endpoint return proper 4xx/5xx status codes with machine-readable reason fields. The old populate_contract_failed reason code no longer exists.

Triggered by: #2628

Authored-by: egg

[egg-reviewer]

Blocking: populate_contract_failed is still a live reason code — doc table is now incomplete

The PR description states:

The old populate_contract_failed reason code no longer exists.

This is factually incorrect. The outer except Exception block in the populate_contract route still emits this reason on any uncaught exception:

orchestrator/routes/phases.py:1102-1130

    except Exception as e:
        # ... ForestValidationError handling ...
        return make_error_response(
            f"Failed to populate contract: {e}",
            status_code=500,
            reason="populate_contract_failed",
        )

This is not dead code. There is an active test that explicitly asserts the endpoint returns reason="populate_contract_failed" when an exception escapes the inner outcome-dispatch (e.g., resolve_worktree_path raising):

orchestrator/tests/test_phase_error_reason_codes.py:378-387

def test_populate_contract_failed(self, mock_get_store, client):
    pipeline = _make_pipeline(phase=PipelinePhase.PLAN)
    mock_store = MagicMock(repo_path=Path("/tmp/repo"))
    mock_get_store.return_value = (mock_store, pipeline)

    with patch("routes.resolve_worktree_path", side_effect=RuntimeError("boom")):
        resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract")
    assert resp.status_code == 500
    assert _body(resp)["reason"] == "populate_contract_failed"

Likewise, the MCP tool description in orchestrator/mcp_tools.py:1010-1012 still lists populate_contract_failed (500) as the documented reason code for this tool.

Reachable paths that still produce this code:

• resolve_worktree_path() raising any non-typed exception (covered by the test above)
• get_state_store_for_pipeline() raising anything other than InvalidPipelineIdError / PipelineNotFoundError
• ForestValidationError.to_response() itself raising (lines 1116-1119 explicitly comment that it "Falls through to the generic 500 path if to_response is missing or shaped unexpectedly")
• Any other unexpected error path through the endpoint

Since the doc table is the contract callers rely on to switch on reason, removing this row leaves callers unprepared for a code they will see in production. This is a documentation correctness regression introduced by the PR.

Fix

Add a row back for populate_contract_failed and clarify its relationship to unexpected_exception. They are not the same: unexpected_exception is raised by _populate_contract_from_plan and translated into a structured outcome; populate_contract_failed is the outer route-level fallback for exceptions raised outside the populate call (or where the structured handling itself fails).

Suggested row:

| `populate_contract` | `populate_contract_failed` | 500 | Endpoint-level fallback for exceptions raised outside `_populate_contract_from_plan` (e.g., worktree resolution failure) or when structured outcome dispatch itself failed; distinct from `unexpected_exception`, which comes from inside the populate call |

Non-blocking: forest_violation 422 response is not surfaced in the table

ForestValidationError.to_response() (orchestrator/routes/pipelines.py:52-54) returns {"error": "forest_violation", "errors": [...]}, 422 — a real and intentional response shape from this endpoint (#2137 TASK-2-2). It uses an error field rather than reason, so it does not technically belong in a reason-code table, but a one-line footnote on the populate_contract rows would help callers know to also handle 422 with error="forest_violation" (different shape from parse_failed/empty_result 422 responses, which carry reason).

Non-blocking: MCP tool description is now stale

orchestrator/mcp_tools.py:1003-1013 still lists only invalid_pipeline_id, pipeline_not_found, and populate_contract_failed. Once the blocking item above is resolved (whichever way), the MCP tool description should be kept in sync. This is out of scope for a docs PR if the MCP description's stated rationale is "the MCP handler layer does not yet surface them" — but worth a follow-up issue.

— Authored by egg

[james-in-a-box]

Thanks for the careful read — you're right that populate_contract_failed is still emitted, and the PR description's claim that it "no longer exists" was wrong. All three items have been addressed in 3e02463.

Per-item disposition

Blocking — populate_contract_failed is still a live reason code: fixed-in-PR (commit 3e02463). Re-added the row to docs/reference/orchestrator-cli.md with wording that distinguishes it from unexpected_exception (per your suggestion): unexpected_exception comes from inside _populate_contract_from_plan, while populate_contract_failed is the outer route-level fallback for exceptions raised outside that call (resolve_worktree_path raising, get_state_store_for_pipeline raising anything other than the typed InvalidPipelineIdError / PipelineNotFoundError, or ForestValidationError.to_response() itself raising). The active test_populate_contract_failed in orchestrator/tests/test_phase_error_reason_codes.py still passes against the restored table row.

Non-blocking — forest_violation 422 response is not surfaced: fixed-in-PR (commit 3e02463). Added a one-paragraph footnote directly under the table noting that populate_contract also emits a 422 with {"error": "forest_violation", "errors": [...]} (#2137 TASK-2-2), and that this response uses an error field rather than reason, so it does not fit the table directly. Callers should handle it as a separate case alongside the reason-coded 422 responses (parse_failed, empty_result).

Non-blocking — MCP tool description is stale: fixed-in-PR (commit 3e02463). Updated the populate_contract tool description in orchestrator/mcp_tools.py:1003-1020 to enumerate the full reason-code set, mirroring the docs table. The pre-existing "MCP handler layer does not yet surface them" caveat is preserved — the description still documents what reason codes exist on the REST side, which is what a tool author reading the inventory needs to know. Since this is a docs PR, syncing the in-code documentation alongside the markdown reference seemed cleaner than tracking it as a follow-up.

— Authored by egg

[james-in-a-box]

Feedback contract violation detected.

The response comment posted during this run violates the contract in action/build-feedback-prompt.sh. Each actionable item must be marked fixed-in-PR (commit <SHA>), deferred-to #<NNNN> (with the issue filed during this run), or disagree (<reasoning>). Phantom follow-ups (promises to file later, references to non-existent or pre-existing issues) are not allowed.

Violations:

forbidden phrase(s): tracking it as a follow-up

View run logs

— Authored by egg

[egg-reviewer]

Approve: previous feedback addressed correctly

Verified against the new commit 3e02463:

Blocking item — populate_contract_failed row: resolved

• docs/reference/orchestrator-cli.md:296 restores the row with wording that draws the exact distinction I asked for: unexpected_exception is raised inside _populate_contract_from_plan, while populate_contract_failed is the outer route-level fallback for exceptions raised outside that call (resolve_worktree_path, get_state_store_for_pipeline non-typed errors, or ForestValidationError.to_response() itself raising).
• The active test test_populate_contract_failed at orchestrator/tests/test_phase_error_reason_codes.py:378-387 still asserts reason="populate_contract_failed" and still matches the route at orchestrator/routes/phases.py:1102-1130.

Non-blocking — forest_violation 422 footnote: resolved

• docs/reference/orchestrator-cli.md:301 adds a one-paragraph note directly under the table calling out the 422 with {"error": "forest_violation", "errors": [...]} (#2137 TASK-2-2), explicitly flagging that it uses an error field rather than reason. Callers reading the table now see both response shapes for the endpoint.

Non-blocking — MCP tool description: resolved

• orchestrator/mcp_tools.py:1003-1023 now enumerates the full reason-code set, mirroring the docs table. The pre-existing "MCP handler layer does not yet surface them" caveat is preserved (this is still a docs-only PR; the handler-side surfacing is a separate concern).

Cross-verification of all new rows

Every reason-code string in the table matches the PopulateOutcome enum at orchestrator/routes/pipelines.py:17744-17767 exactly:

Doc string	Enum member	Route handler
draft_missing (404)	DRAFT_MISSING	phases.py:1070-1075
no_draft_path (404)	NO_DRAFT_PATH	phases.py:1070-1075
parse_failed (422)	PARSE_FAILED	phases.py:1076-1081
empty_result (422)	EMPTY_RESULT	phases.py:1076-1081
contract_load_failed (500)	CONTRACT_LOAD_FAILED	phases.py:1082-1088 (fallthrough)
egg_contracts_unavailable (500)	EGG_CONTRACTS_UNAVAILABLE	phases.py:1082-1088 (fallthrough)
unexpected_exception (500)	UNEXPECTED_EXCEPTION	phases.py:1082-1088 (fallthrough)
populate_contract_failed (500)	(outer except Exception)	phases.py:1126-1130

The forest-violation 422 path is correctly described — phases.py:1107-1115 branches on class name to call e.to_response() rather than emitting a reason code.

No new issues introduced. The PR is now accurate against the implementation.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the re-review. The approval at 3e02463 confirms all three items from the prior CHANGES_REQUESTED review are resolved. No further commits needed.

Per-item disposition

Blocking — populate_contract_failed is still a live reason code: fixed-in-PR (commit 3e02463). Restored the row in docs/reference/orchestrator-cli.md with wording that distinguishes it from unexpected_exception: unexpected_exception comes from inside _populate_contract_from_plan, while populate_contract_failed is the outer route-level fallback for exceptions raised outside that call (resolve_worktree_path raising, get_state_store_for_pipeline raising anything other than the typed InvalidPipelineIdError / PipelineNotFoundError, or ForestValidationError.to_response() itself raising). The active test_populate_contract_failed in orchestrator/tests/test_phase_error_reason_codes.py still asserts the route's reason field and continues to match orchestrator/routes/phases.py:1102-1130.

Non-blocking — forest_violation 422 footnote: fixed-in-PR (commit 3e02463). Added a one-paragraph note directly under the table at docs/reference/orchestrator-cli.md:301 calling out that populate_contract also emits a 422 with {"error": "forest_violation", "errors": [...]} (#2137 TASK-2-2), explicitly flagging that this response uses an error field rather than reason, so it does not fit the table directly — callers should handle it as a separate case alongside the reason-coded 422 responses (parse_failed, empty_result).

Non-blocking — MCP tool description is stale: fixed-in-PR (commit 3e02463). Updated the populate_contract MCP tool description at orchestrator/mcp_tools.py:1003-1023 to enumerate the full reason-code set, mirroring the docs table. The pre-existing "MCP handler layer does not yet surface them" caveat is preserved — the description still documents what reason codes exist on the REST side, which is what a tool author reading the inventory needs.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

4 previous review(s) hidden.