Fix HITL decisions missing phase and options

[james-in-a-box]

Fix HITL decisions missing phase and options

When agents create HITL decisions during a pipeline (via egg-orch decision create or egg-contract add-decision), the decisions were queued without a
phase field. This caused the egg-sdlc CLI to show "unknown" phase, fail
to locate draft files (e.g., 943-unknown.md instead of 943-analysis.md),
and fall back to generic menus instead of choice-specific numbered options.

Four-layer fix applied across the stack:

1. Server-side auto-infer (orchestrator/decision_queue.py): When phase
   is None, auto-set it from the pipeline's current phase. Highest-impact fix
   — all future decisions get the correct phase regardless of CLI source.

2. CLI args (sandbox/egg_lib/orch_cli.py): Added --phase and
   --decision-type arguments to egg-orch decision create for explicit
   control when needed.

3. Display fallback (sandbox/egg_lib/sdlc_hitl.py): When the regex
   phase detector returns "unknown", fetch the pipeline's current phase from
   the orchestrator API. Handles existing decisions created without phase.

4. Contract CLI (sandbox/egg_lib/contract_cli.py): Added --phase arg
   to egg-contract add-decision and auto-populate from contract's
   current_phase field.

Issue: none

Test plan:

• All 256 existing tests pass (122 decision/contract + 134 sdlc_hitl)
• Create a decision via egg-orch decision create --question "test?" --options "A" "B" and verify it gets the pipeline's current phase
• Verify egg-sdlc watch loop shows proper phase and options for pending decisions

Authored-by: egg

[egg-reviewer]

Review: Fix HITL decisions missing phase and options

The approach is sound — auto-inferring phase at the server is the right place
to fix this, and the layered fallbacks (explicit → server-inferred → regex → API
fetch) are well-structured. However, there are a few issues to address.

1. --decision-type choices are incomplete (orch_cli.py)

File: sandbox/egg_lib/orch_cli.py, line ~1155

The CLI defines:

choices=["choice", "feedback"]

But the server accepts three valid values:

VALID_DECISION_TYPES = ("phase_gate", "choice", "feedback")

(orchestrator/routes/decisions.py:172)

phase_gate is omitted from the CLI choices. While phase_gate decisions are
typically created programmatically by the orchestrator, omitting it from the CLI
makes the CLI an incomplete interface to the API. If someone needs to manually
create a phase gate for debugging or recovery, they can't.

Fix: Add "phase_gate" to the choices list, or document why it's intentionally
excluded (e.g., a help string saying "phase_gate decisions are created
automatically by the orchestrator").

2. Silent except Exception: pass swallows failures (sdlc_hitl.py)

File: sandbox/egg_lib/sdlc_hitl.py, lines 974–975

except Exception:
    pass

This catches everything — network errors, JSON decode failures, auth errors,
even KeyboardInterrupt (no, Python 3 Exception doesn't catch that, but it
does catch TimeoutError, ConnectionError, etc.). The bare pass means the
operator gets no signal that the fallback was attempted and failed.

This is a display-path fallback so it won't corrupt data, but at minimum log a
debug/warning so operators can diagnose why phase shows as "unknown" when they
expect it not to.

Suggested fix:

except Exception:
    logger.debug("Failed to fetch pipeline phase for decision display", exc_info=True)

Or if there's no logger in this module, at minimum use pass # noqa: ... with
a comment explaining the rationale.

3. No tests for any of the new behavior

The PR description says "All 256 existing tests pass" — but none of the four
changes have corresponding test coverage:

• decision_queue.py: No test that queue_decision(phase=None) auto-infers
  phase from pipeline.current_phase. This is the "highest-impact fix" per the
  PR description, and it should have a test proving it works.
• sdlc_hitl.py: No test for the new API-fetch fallback when _detect_phase
  returns "unknown".
• orch_cli.py: No test that --phase and --decision-type args are wired
  correctly into the request payload.
• contract_cli.py: No test that args.phase takes precedence over
  contract.get("current_phase"), or that the fallback works when args.phase
  is None.

These are all straightforward to test. The server-side auto-infer in particular
is the kind of thing that could silently regress if someone refactors
queue_decision later.

4. Minor: _detect_phase returns "analysis" mapping inconsistency

In sdlc_hitl.py, _detect_phase returns "refine" when it matches
\banalysis\b, which is correct (the phase enum value is "refine"). But
elsewhere in the same file (line 569), there's:

phase_label = "analysis" if phase == "refine" else phase

This means the display says "analysis" but the phase value is "refine". The new
API-fetch fallback at line 971 will return "refine" from the pipeline state,
which will display as "refine" (not "analysis") because it bypasses the
_detect_phase path. This is consistent with how it already works when phase
is explicitly set on the decision, so it's not a regression — but it does mean
the display label will differ depending on how phase was determined. Not blocking,
just worth being aware of.

Summary

The core logic changes are correct and well-placed. The main gaps are:

1. Missing phase_gate in --decision-type choices (minor)
2. Silent exception swallowing (minor, but affects debuggability)
3. No test coverage for the new behavior (blocking — this is infrastructure code
   that should have tests)

— Authored by egg

[james-in-a-box]

All three actionable items addressed in 27083d7:

1. phase_gate in --decision-type choices — Added phase_gate to the choices list with a help string explaining it's typically created by the orchestrator but available for debugging/recovery.

2. Silent except Exception: pass — Added logging.getLogger(__name__) and replaced the bare pass with logger.debug("Failed to fetch pipeline phase for decision display", exc_info=True).

3. Test coverage — Added 16 tests across all four changed files:

   • test_decision_queue.py: 2 tests — auto-infer phase from pipeline, explicit phase not overridden
   • test_sdlc_hitl.py: 2 tests — API-fetch fallback on unknown phase, API failure stays unknown
   • test_orch_client.py: 5 tests — --phase/--decision-type arg parsing, phase_gate accepted, payload wiring, omitted args absent from payload
   • test_contract_cli.py: 4 tests — --phase parsing, default None, explicit phase overrides contract, fallback to contract's current_phase

Re item #4 (display label inconsistency): Acknowledged, not a regression. The display will show the phase value as-is regardless of source, which is consistent with existing behavior when phase is explicitly set on the decision.

All 2568 tests pass, linter clean.

— Authored by egg

[egg-reviewer]

Re-review: Fix HITL decisions missing phase and options

Reviewed delta: 025449e..27083d7 (commit "Address review feedback").

Previous feedback verification

All three actionable items from the initial review have been properly addressed:

1. phase_gate in --decision-type choices — Added to orch_cli.py:1155 with a clear help string explaining it's typically orchestrator-created but available for debugging/recovery. The choices now match VALID_DECISION_TYPES on the server side (orchestrator/routes/decisions.py:172).

2. Silent except Exception: pass — Replaced with logger.debug(...) with exc_info=True at sdlc_hitl.py:978. Logger is properly initialized at module level (sdlc_hitl.py:22). This provides the debug traceability requested.

3. Test coverage — 16 tests added across four test files covering:

   • test_decision_queue.py: Auto-infer from pipeline + explicit phase not overridden
   • test_sdlc_hitl.py: API fallback on unknown phase + API failure stays unknown
   • test_orch_client.py: Arg parsing, payload wiring, omission behavior
   • test_contract_cli.py: Phase parsing, default None, explicit override, contract fallback

New code review

Production code changes are correct. Verified:

• sdlc_hitl.py:973-974: pipeline_info.get("pipeline", pipeline_info).get("current_phase") correctly navigates the OrchClient.get_pipeline() return format ({"pipeline": {"current_phase": "..."}}).
• orch_cli.py:687-690: Phase and decision_type are conditionally added to the payload only when provided — omitted args don't send null values to the server.
• contract_cli.py:507: args.phase or contract.get("current_phase") correctly falls back. Note: if args.phase is None and contract.get("current_phase") is also None, decision_phase will be None, which is fine — the server-side auto-infer in decision_queue.py handles it.
• CLI --phase choices (["refine", "plan", "implement", "pr"]) match PipelinePhase enum values exactly.

Tests are well-structured. Verified:

• Mock return values match actual API response formats.
• OrchestratorError is correctly imported and instantiated with a single string arg (status_code defaults to 0).
• The capturing_make_request pattern in test_contract_cli.py correctly captures original_make_request before the patch context manager applies, so GET calls route to the mock server while POST payloads are captured for assertion.
• Decision queue tests use the existing fixture's _load_pipeline()/_save_pipeline() pattern consistently.

No issues found

The feedback has been thoroughly addressed. Code is correct, tests are meaningful and cover the right scenarios, and the changes integrate cleanly with the existing codebase patterns.

Advisory (non-blocking)

The test_orch_client.py file now has orch_cli tests appended to it (lines 528-614). These test egg_lib.orch_cli functions, not egg_lib.orch_client. A dedicated test_orch_cli.py would be more discoverable, but this is a naming/organization nit, not a correctness issue.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

3 previous review(s) hidden.