Hard-close the refine-to-plan deferral leak with a propose-time coverage gate - #3581
Conversation
…age gate Closes #3564. Follow-up to #3563, which made refine's deferred_to_plan candidates a prompt-level handoff but left the gap that nothing deterministically verified, at plan propose time, that each deferred question was actually registered or re-dispositioned. Matching contract: stable-identity echo. Each deferred candidate gets a content-derived id, dq-<first 8 hex of sha256(normalize_question(q))>, computed by the new egg_contracts.decisions.deferred_question_id. The plan prompt renders the id next to each candidate; the plan producer echoes every id in a new attestation field, deferred_resolutions, as either {deferred_id, resolution: "registered", cq: "cq-N"} or {deferred_id, resolution: "not_operator_grade", why}. The propose-time validator recomputes the ids from the same message-store scan the prompt used and NACKs the proposal when any id is unaccounted, unknown, or claims a cq-N absent from the attestation's own decisions_registered. Exact matching is safe because identity rides on the id, not the question text, so the planner can freely reframe the question as the design firms up (the false-NACK risk that ruled out text matching), and the validator stays fully deterministic (no LLM-assisted comparison). - shared/egg_contracts/decisions.py: deferred_question_id, DQ_ID_PATTERN, DEFERRED_RESOLUTION_KINDS, deferred_resolution_errors; the shared decision_attestation_errors gains an optional deferred_resolutions param so the Pydantic model and the signal validator stay in lockstep - orchestrator/attestation_schemas.py: DeferredResolution model + deferred_resolutions field on DecisionSurfacingAttestation - routes/pipelines/_decision_candidates.py: the deferred section renders per-candidate dq- ids and the --deferred echo syntax - routes/signals/_validation.py: _validate_deferred_candidate_coverage, scoped to the architect (the role whose prompt carries the section), hoisted with the shape check ahead of the branch-verification early return so a degraded fetch cannot skip it; message-store outages degrade to a logged skip, and the NACK message carries each missing question verbatim so a producer that never saw the prompt section can act on the rejection alone. A non-plan attestation carrying deferred_resolutions is rejected at shape time. - sandbox: repeatable --deferred flag on egg-orch consensus propose and the mcp__brc__propose schema note - plan reviewer rubric now focuses on the substance the gate cannot judge (a reframing that loses the fork, a vacuous why); coverage itself is deterministic - docs/hitl-decisions.md documents the dq- id contract and the new hard-reject conditions
There was a problem hiding this comment.
Review: propose-time coverage gate for refine→plan deferrals (#3564)
I traced the full data flow — refine deferred_to_plan candidates → _find_deferred_plan_candidates → prompt dq- id rendering → producer --deferred echo → deferred_resolutions shape check → the _validate_deferred_candidate_coverage cross-check → 400 rejection. The design (identity-on-id, not on question text) is sound, the gate is correctly wired, and the coverage/degradation matrix is well-tested. No blocking issues found.
Verified end-to-end:
- The reachable render path is the concurrent-mode architect prompt (
_prompt_agent.py:816), and_DEFERRAL_COVERAGE_ROLES = {"architect"}matches it exactly._validate_producer_artifactsreaches the architect (it hasarchitect-output/architect-slicesspecs, so thespecs_forearly-return at_validation.py:900does not fire), and the hoisted call at_validation.py:914runs ahead of the degraded-fetch early return —test_coverage_enforced_on_degraded_fetch_pathlocks that in. - Shape validation (
_validate_decision_attestation_shape) runs before coverage on both paths, so the coverage loop's non-registered/not_operator_gradebranch is defensively unreachable in production, as the docstring claims. - The
ValueErrorraised by the gate is caught at_consensus_verdicts.py:857→make_error_response(str(e), 400), so a dropped deferral produces an actionable propose rejection, not a 500. The tracker is not mutated (validation precedeshandle_propose), so a rejected refine propose is never recorded — which is what makes the empty-question case in note (2) below unreachable. - Arity change on
_extract_attested_decision_fields(3→4 tuple) is applied at all four call sites;decision_attestation_errors' new 4th param is defaulted, and the PydanticDecisionSurfacingAttestation.validate_ledger_shapefeeds it through the same shared helper, so the model and the signal validator cannot drift.
Non-blocking suggestions
1. Redundant message-store scan on the happy path. On a non-degraded architect propose, _find_deferred_plan_candidates(pipeline_id) runs twice — once via the hoisted call at _validation.py:914, then again inside _validate_decision_attestation → _validate_deferred_candidate_coverage at _validation.py:664. Each does a get_messages(pipeline_id, limit=500) fetch. It's read-only and idempotent (no race — refine is frozen by plan-propose time), so this is purely a minor inefficiency, not a correctness problem. Consider gating the second call, or memoizing the scan for the request, if it ever shows up in propose latency.
2. Prompt/gate use different fallbacks for an empty deferred question. _format_deferred_candidates_with_ids (_decision_candidates.py:103) renders deferred_question_id("?") for a missing/blank question, while _validate_deferred_candidate_coverage (_validation.py:418-420) computes off "" and then skips it via if question:. I confirmed the ids differ (dq-8a8de823 vs dq-e3b0c442). If a blank-question deferred candidate ever reached both surfaces, the architect would echo the prompt's id and the gate would false-NACK it as "matches no refine-deferred question" with no way to satisfy the gate — a plan-phase deadlock. This is unreachable today because candidate_considered_errors requires a non-empty question and a shape-invalid refine propose is rejected before it's recorded, so _find_deferred_plan_candidates only returns non-empty questions. Still, the two helpers encode the same defensive case with divergent fallbacks; aligning them (both skip, or both use the same sentinel) removes a latent trap if that refine-side invariant is ever relaxed.
3. Doc/comment references a "single-agent plan prompt". The _DEFERRAL_COVERAGE_ROLES comment (_validation.py:482) and the PR description both say the section renders into "the single-agent plan prompt" via _build_phase_prompt. In practice the plan phase always runs concurrent, and _build_agent_prompt only delegates to _build_phase_prompt for coder/refiner — so the phase == "plan" deferred-section branch in _build_phase_prompt (_prompt_phase.py:424) appears unreachable, and the architect gets the section solely through the concurrent path the gate already covers. Pre-existing structure (the call sites aren't touched by this PR), but the comment slightly overstates the render surface; worth a note or a follow-up to prune the dead branch.
4. --deferred replaces a --file attestation. Like --considered, layering --deferred into req["attestation"] overrides a --file payload's attestation wholesale. --deferred alone (no --decisions-registered/--no-decisions-rationale) yields an attestation missing the exactly-one-of field and is shape-NACKed. Expected usage combines the flags and this mirrors the existing --considered behavior, so it's fine — just flagging that the prompt's egg-orch consensus propose ... --deferred ... example elides the accompanying ledger flags a real propose needs.
Nice work on the test matrix — accept/reject/degrade/exemption plus the degraded-fetch hoist and the dq-id stability/shape cases are all covered, and the fixtures exercise the real validators rather than hand-built payloads.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
Thanks for the thorough end-to-end trace — appreciate the confirmation that the gate is correctly wired and the coverage/degradation matrix holds. Dispositions for the four non-blocking suggestions below.
1. Redundant message-store scan on the happy path — 2. Prompt/gate divergent fallbacks for an empty deferred question — 3. Comment references a "single-agent plan prompt" — 4. Verification: — Authored by egg |
|
egg feedback addressed. View run logs 1 previous review(s) hidden. |
Closes #3564. Follow-up to #3563.
The gap
#3563 turned refine's
deferred_to_plancandidates into a prompt-level handoff: they are injected into the plan prompts as pre-seeded items, and the plan reviewer rubric cross-checks them manually. But nothing deterministically verified, at plan propose time, that the producer actually registered or re-dispositioned each deferred question; a plan producer could attest explicit-none with unrelatedcandidates_consideredand a deferred question would silently vanish.Design decision: the matching contract
The hard part flagged in #3564 was matching a refine-deferred free-text question against the plan's registrations, with no obviously-correct default: strict text matching false-NACKs the legitimate reframing case (refine defers "Should we support pagination?", plan registers "cq-3: default page size, 25 or 50?"), loose matching doesn't close the leak, and semantic matching would put an LLM inside what is today a purely deterministic propose-time validator.
This PR takes the stable-identity-echo option: identity rides on an id, not on the question text, so exact matching is safe while the planner freely reframes.
dq-<first 8 hex of sha256(normalize_question(question))>(egg_contracts.decisions.deferred_question_id). Content-derived rather than ordinal so the id is stable no matter how or when the candidate list is recovered.dq-id and the echo syntax.deferred_resolutions: one{deferred_id, resolution: "registered", cq: "cq-N"}or{deferred_id, resolution: "not_operator_grade", why}entry per id. CLI: repeatable--deferred "dq-<hash> :: registered :: cq-3"/--deferred "dq-<hash> :: not_operator_grade :: <why>"._validate_deferred_candidate_coveragerecomputes the ids from the same_find_deferred_plan_candidatesscan the prompt used and NACKs the proposal when a deferred id is unaccounted, when an echoed id matches no deferred question, or when aregisteredecho cites a cq-N absent from the attestation's owndecisions_registered(whose contract existence the existing ledger cross-check enforces).Scoping and failure modes
_validate_producer_artifacts's branch-verification early return: a degraded fetch cannot skip the gate (same rationale as the Guarantee HITL decision registration: hard-fail on missing decision ledger + reviewer obligation against un-surfaced decisions #3390 shape hoist).deferred_resolutionsis rejected at shape time (only plan producers have dq- ids to echo).decision_attestation_errorshelper, so the Pydantic model and the signal validator cannot drift (same pattern as Guarantee HITL decision registration: hard-fail on missing decision ledger + reviewer obligation against un-surfaced decisions #3390/HITL decisions declining: agents may be under-surfacing operator decisions, especially at refine #3526).The plan reviewer rubric now focuses on the substance the gate cannot judge: a
registeredreframing that loses the deferred question's actual fork, or anot_operator_gradewhy that doesn't hold up.Testing
make lintclean.make test(changeset-aware): 21768 passed; the 5 failures are pre-existing local-environment issues (gateway worktree-path detection and docker-reap tests reproduce identically on pristine HEAD viagit stash; the session-expiry boundary test is a timing flake that passes in isolation).deferred_resolution_errors(shared), the coverage gate's accept/reject/degrade/exemption matrix incl. the degraded-fetch hoist (test_signals.py::TestDeferredCandidateCoverageGate), prompt rendering of ids + echo contract (test_decision_ledger_gate.py), and--deferredCLI parsing (test_orch_cli_consensus_push.py).Refs: #3526, #3563