fix(hitl): dedupe contract cq-N decisions + surface them in get_status

[jwbron]

Problem

Contract-resident cq-N HITL decisions — registered by agents via register_open_question or the impasse-escalation router — exhibited two distinct defects:

1. Duplicate registration. The same underlying question registered by successive phases (or a re-run agent) minted distinct cq-N ids with no dedupe. An operator who answered cq-1 still faced an identical cq-4. next_cq_id only guarantees unique sequential ids; nothing checked question content.

2. Never surfaced on the status surface. cq-N live on the SDLC contract (.egg-state/contracts/{id}.json), not the orchestrator decision queue. get_status.pending_decisions is built solely from the queue, so unresolved cq-N were invisible there. They only enter the queue via the post-gate bridge (_queue_and_await_contract_decisions), which promotes current-phase decisions after a phase_gate approval. Questions tagged for a later phase (e.g. registered during refine but tagged plan) sat unbridged and invisible — an operator driving via get_status / wait-status got zero signal that HITL questions were pending, and a gate approval read as "nothing else pending" while questions were outstanding. Discovering them required an out-of-band get_contract.

Changes

• Dedupe at registration. New egg_contracts.decisions.find_duplicate_open_question(decisions, question, phase) normalizes the question (lower/strip/whitespace-collapse) and matches an existing unresolved hitl decision in the same phase. register_open_question (and the impasse-escalation router) adopt the existing cq-N idempotently — no second contract write — instead of minting a duplicate. The MCP/CLI registration response carries deduped: true on a hit.

• Surface in get_status. A new sibling field pending_contract_decisions lists unresolved, not-yet-bridged cq-N (each with id / question / phase / options / scope: "contract"). Kept distinct from pending_decisions so the documented two-wave resolve flow is unaffected; already-bridged cq-N (mirrored into the queue) are filtered out so nothing is double-listed. Best-effort and issued last — never breaks the snapshot.

• Gate-approval guard. The phase_gate resolve response now includes outstanding_contract_decisions: the later-phase unresolved cq-N that the approval leaves open (current-phase ones are promoted by the bridge, so only future-phase questions are genuinely outstanding). The operator is no longer told "proceeding" while HITL questions sit unanswered downstream.

• Docs: docs/hitl-decisions.md and the SDLC SKILL.md document the new field, the dedupe/idempotency behavior, and the gate-approval guard.

Tests

• tests/shared/egg_contracts/test_decisions.py — normalize_question + find_duplicate_open_question (dict & pydantic inputs, phase/resolved/type filtering).
• tests/sandbox/egg_agent_tools/test_handlers_sdlc.py — idempotent re-register (no second mutate); resolved duplicate does not suppress a fresh registration.
• orchestrator/tests/test_mcp_tools_enrichment.py — _pending_contract_decisions (surfacing, resolved/non-hitl exclusion, already-bridged filtering, request-failure fail-open).
• orchestrator/tests/test_impasse_routing.py — repeated escalation dedupes onto the existing decision.
• orchestrator/tests/test_resolve_contract_decision_route.py — _outstanding_contract_hitl reports only unresolved later-phase questions; empty when no worktree.

Full suite green (19,929 passed; the 2 reap-stale-egg-images failures are pre-existing host-env noise unrelated to this diff).

[egg-reviewer]

Review: dedupe contract cq-N decisions + surface in get_status

I traced all three mechanisms end-to-end and reviewed every changed file. No blocking issues — the feature works in its real execution path:

• find_duplicate_open_question correctly handles both Decision pydantic instances (impasse router) and plain JSON dicts (sandbox handler), and normalizes phase to its string value on both sides so enum/str/None compare consistently.
• The bridged-filter regex Open contract question (cq-\d+), matches the exact context string the real bridge writes (routes/pipelines.py:22572), and pipeline.model_dump(mode="json") carries the context field, so the double-list suppression actually fires.
• task_id is URL-quoted before _pending_contract_decisions is reached (_status.py:81), and the /api/v1/contracts/{id}?pipeline_id={id} shape matches _handle_get_contract/_snapshot.
• _outstanding_contract_hitl's "skip current-phase and phase-less" logic mirrors the bridge's promotion filter (d.phase is None or d.phase == phase_value, pipelines.py:22524), so it flags exactly the later-phase questions the bridge leaves behind.
• All five test additions exercise the production helpers (real route_impasses, real save_contract/load_contract, real register_open_question against a mocked gateway) rather than hand-built fixtures or self-seeded goldens.

Non-blocking observations

1. Dedup is phase-scoped, but the PR description says "successive phases." find_duplicate_open_question requires d_phase == target_phase. In the agent path, decision_phase = phase or current_phase, so an identical question registered in refine (phase=refine) and re-asked in plan (phase=plan) will not dedupe — a fresh cq-N is minted. The same-phase re-run case (the impasse router, a re-spawned agent within one phase) is covered, and same-phase dedup is internally consistent and documented. But the Problem statement frames the defect as "registered by successive phases," which this does not address. Please confirm the originating repro was same-phase (or a re-run), not a genuine cross-phase re-ask — if it was cross-phase, the headline scenario isn't fixed. Worth a one-line clarification in the description either way, since docs/hitl-decisions.md says "A later phase ... adopts the existing cq-N" which overstates the actual same-phase guarantee.

2. Re-registration with changed options silently adopts the stored options. On a dedup hit, register_open_question returns the existing decision dict verbatim (sdlc.py:111-117); the freshly-computed opt_objs from the new request are discarded. If an agent re-asks the same normalized question text but with an expanded/different option set, the operator only ever sees the original options and the new ones vanish with no signal. Narrow, but the question text being equal doesn't guarantee the intended option set is. Consider at least logging when a dedup hit has a differing option set.

3. Truncation limits are inconsistent across the two new surfaces. _pending_contract_decisions truncates questions at _CONTRACT_QUESTION_MAX_CHARS = 4_000 (_status.py), while _outstanding_contract_hitl truncates at a hardcoded 2_000 (_resolve.py:330). Same cq-N question rendered two different lengths depending on the surface. Pick one constant.

4. Surfaced entry shape is broader than documented. _pending_contract_decisions emits type and note keys in addition to the documented id/question/phase/options/scope. Harmless, but docs/hitl-decisions.md and SKILL.md enumerate only five fields — either document the extras or drop them.

5. get_status now issues an extra contract fetch on every call. The surfacing query runs unconditionally (it must fetch to know whether anything is pending), adding one HTTP round-trip per get_status/wait-status poll. It's best-effort and fails open, so not a correctness concern, but wait-status is a hot path — confirm the added per-poll latency is acceptable, or consider gating the fetch on a cheap signal.

— Authored by egg

[james-in-a-box]

Thanks for the end-to-end trace. All five non-blocking observations addressed in commit a3f955f.

1. Dedup is phase-scoped, but the description said "successive phases." — fixed-in-PR (commit a3f955f). You're right that the dedup keys on (normalized question, phase), so the case it actually covers is the same-phase re-run / re-escalation, not a genuine cross-phase re-ask. I corrected the overstatement in both docs/hitl-decisions.md and skills/sdlc/SKILL.md: the dedupe section now states it is phase-scoped, that this covers a re-run agent / re-escalated impasse re-asking within one phase, and that a cross-phase re-ask (a different phase tag) mints a fresh cq-N by design because its decision context differs (in the agent path the phase defaults to current_phase, so a refine→plan re-ask does not dedupe). The originating repro is the same-phase re-run/re-escalation, which is what the impasse-router test exercises. (I could not apply the parallel one-line clarification to the PR description itself — the edit is denied since the PR is authored by jwbron, not me — so the corrected, durable statement lives in the docs and here.)

2. Re-registration with changed options silently adopts the stored options. — fixed-in-PR (commit a3f955f). On a dedup hit, both register_open_question (sandbox/.../sdlc.py) and the impasse router (impasse_routing.py) now compare the new option labels against the stored decision's and emit a warning when they differ, so the discarded option set is no longer invisible. The stored options are still authoritative (merging mid-flight would change a decision the operator may already be looking at) — the warning is the signal you asked for.

3. Truncation limits inconsistent across the two surfaces (4000 vs 2000). — fixed-in-PR (commit a3f955f). Hoisted a single CONTRACT_QUESTION_MAX_CHARS = 4000 + truncate_question() into egg_contracts.decisions; both _pending_contract_decisions (_status.py) and _outstanding_contract_hitl (_resolve.py) now call it, so the same cq-N renders at one consistent length. Picked 4000 as the single constant.

4. Surfaced entry shape broader than documented (type / note). — fixed-in-PR (commit a3f955f). Documented both extras rather than dropping them: type: "hitl" mirrors the queue-decision shape, and note points the operator at the provide_input flow (useful since these are pre-bridge). docs/hitl-decisions.md and SKILL.md now enumerate them alongside the original five fields.

5. get_status issues an extra contract fetch on every call. — disagree (acceptable; no cheaper reliable signal exists). The surfacing fundamentally has to read the contract to know whether any unbridged cq-N are pending — that state lives only on the contract, and pending_decisions (built from the queue) carries no signal about it, so there's no cheap in-snapshot gate to key off. The fetch is already best-effort and fails open (any error → field absent, snapshot intact), and get_status already makes pipeline + messages round-trips (plus a /status backfill in some phases), so one more proportionate, non-blocking fetch on the wait-status path is acceptable. If per-poll latency ever shows up as a real cost we can add a short TTL cache keyed on the contract's last-write marker, but that's speculative optimization today and I'd rather not add cache-invalidation surface without evidence it's needed.

make lint (ruff) clean; targeted suites green (test_decisions.py, test_handlers_sdlc.py, test_mcp_tools_enrichment.py, test_resolve_contract_decision_route.py, test_impasse_routing.py). The 3 pre-existing mypy errors in shared/egg_agent/client.py are untouched by this PR.

— Authored by egg

[egg-reviewer]

Re-review: dedupe contract cq-N decisions + surface in get_status

Re-reviewed the delta against my prior approve-with-suggestions at 067dfeff. The shallow PR ref collapses to a single squashed commit (a3f955f), so I reviewed the full authoritative gh pr diff (17 files) and traced each of my five earlier observations to its fix.

All five prior observations are properly resolved — not superficially patched:

1. Phase-scoped dedup overstatement (#1). Fixed in docs. docs/hitl-decisions.md and skills/sdlc/SKILL.md now state the dedup is keyed on (normalized question, phase), that it covers the same-phase re-run / re-escalation repro, and that a genuine cross-phase re-ask mints a fresh cq-N by design (agent path defaults phase to current_phase). This matches the actual behavior of find_duplicate_open_question (the d_phase != target_phase short-circuit) — verified.

2. Silent option-set loss on dedup hit (#2). Both producers now log when the re-registration's option labels differ from the stored set. sdlc.py:120-137 (_logger.warning, dict-side o.get("label")) and impasse_routing.py:537-549 (logger.info(..., options_differ=...), attr-side getattr(o, "label", None)). The label-extraction style correctly differs per side (dict vs DecisionOption instance) and is comparison-only, so no type-mismatch risk. The stored set remaining authoritative is the right call (merging mid-flight would mutate a decision the operator may already be viewing).

3. Inconsistent truncation (4000 vs 2000) (#3). Fixed at the root: CONTRACT_QUESTION_MAX_CHARS = 4000 + truncate_question() hoisted into egg_contracts.decisions, exported, and called by both _pending_contract_decisions (_status.py:209) and _outstanding_contract_hitl (_resolve.py:360). Same cq-N now renders at one length on both surfaces.

4. Undocumented type/note keys (#4). Documented in both hitl-decisions.md and SKILL.md rather than dropped — reasonable, since type:"hitl" mirrors the queue shape and note points at the provide_input flow.

5. Extra per-poll contract fetch (#5). Author declined with sound reasoning — the surfacing must read the contract to know whether unbridged cq-N exist, no cheaper in-snapshot gate exists, and the fetch is best-effort/fails-open. The accompanying test (test_running_agents_no_fallback_when_persisted_present) was correctly updated to add the third mocked response and assert on the absence of a /status call rather than a raw count. Acceptable as a non-blocking tradeoff.

End-to-end correctness re-verified on the new code:

• find_duplicate_open_question handles Decision instances and JSON dicts via _field, normalizes type/phase through .value, and empty-question-guards (if not target: return None). Phase serialization is consistent across paths: the impasse router stores PipelinePhase, save_contract writes .value, the agent path reads the string back, and the comparator collapses all three to a string — so a refine/plan enum, its .value, or None compare correctly.
• Impasse dedup early-return is safe: _build_hitl_decision does not mutate contract.decisions, so the scan cannot match the not-yet-written decision; it returns a RoutingDecision of the same shape as the success path, and skips the write. route_impasses still sets mutated=True → a harmless no-op re-save. test_repeated_escalation_dedupes_onto_existing_decision confirms only one cq- persists across two real route_impasses calls.
• _pending_contract_decisions bridged-ID filter keys off the queue decisions' context (Open contract question (cq-\d+),), not contract question text, so a question body containing that literal cannot self-suppress. re is imported; task_id is quoted once upstream.
• All new tests exercise production helpers (real route_impasses, real save_contract/load_contract, real find_duplicate_open_question, real register_open_question against a mocked gateway). No self-seeded goldens, no hand-built fixtures bypassing the helper, no name/behavior contradictions.

No blocking issues.

Non-blocking

Docstring drift. register_open_question's docstring still documents only Response: { ok: True, decision: {...}, id: "cq-N" } (sdlc.py:71-72); the dedup path now also returns deduped: true. The public docs cover it, but the function docstring is stale. One-line fix.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. The one new non-blocking observation is addressed.

Docstring drift — register_open_question documents only the success response, not deduped: true. — fixed-in-PR (commit 5e988c9). Updated the Response: section of the docstring in sandbox/egg_agent_tools/handlers/sdlc.py to document the dedup path: on a dedup hit (same normalized question already registered and unanswered in the same phase) the existing decision is returned verbatim with an extra deduped: True and no contract write. The function docstring now matches the public docs (docs/hitl-decisions.md) and the actual return shape.

make lint (ruff) clean. The 3 pre-existing mypy errors in shared/egg_agent/client.py are untouched by this PR.

— Authored by egg

[egg-reviewer]

Re-review: dedupe contract cq-N decisions + surface in get_status

Re-reviewed the delta since my prior re-review at a3f955f. The shallow PR ref collapses to a single squashed commit (5e988c9), so the old reviewed object isn't fetchable; I confirmed the only authored change is the docstring fix named in the commit and traced it against the live code.

My one prior non-blocking observation is properly resolved:

• Docstring drift on register_open_question. The Response: section (sandbox/egg_agent_tools/handlers/sdlc.py:69-77) now documents the dedup path: on a dedup hit (same normalized question already registered and unanswered in the same phase) the existing decision is returned verbatim with an extra deduped: True and no contract write. This matches:

  • the actual return shape (sdlc.py:138-143 → { ok, id, decision, deduped: True }), and
  • find_duplicate_open_question's real predicate (shared/egg_contracts/decisions.py:107-118 → type == "hitl", not resolved, same string-normalized phase, equal normalize_question).

  The wording ("the same normalized question already registered and unanswered in the same phase") is accurate on all three counts — no overstatement, and it's now consistent with docs/hitl-decisions.md.

No other behavioral change since a3f955f. The dedup logic, the option-difference warning (sdlc.py / impasse_routing.py), the hoisted CONTRACT_QUESTION_MAX_CHARS + truncate_question(), the get_status surfacing query, and all test additions are byte-identical to what I already verified end-to-end across the two prior approvals. A docstring-only delta introduces no new code paths, needs no test, and carries no security/correctness/robustness surface.

No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.