Address #3563 review: uniform ledger shape + reviewer candidates rubric - #3565
Conversation
…tes rubric - Emit a uniform key set (registered, resolved, explicit_none, attested_by, missing, candidates_considered) across all three branches of _collect_decision_ledger_status so decision_ledger consumers never guess whether an absent key is inapplicable or forgotten (suggestion 2). - Add the enumerated-candidates (candidates_considered) requirement to the refine and plan reviewer criteria, incl. the plan-side cross-check of the refiner's deferred candidates (suggestion 4). Suggestion 1 (deterministic propose-time deferral cross-check) deferred to #3564: hard-closing the leak needs a matching-semantics design decision the review did not settle.
There was a problem hiding this comment.
Review: #3565 — uniform ledger shape + reviewer candidates rubric
Thorough review of all four changed files, tracing every added key/string through its consumers. No blocking issues. This is a tight, correctly-scoped follow-up to the #3563 review suggestions.
What I verified
1. _ledger.py — uniform summary shape (verified correct).
All three return branches of _collect_decision_ledger_status now emit exactly the same 6-key set — {registered, resolved, explicit_none, attested_by, missing, candidates_considered}:
- registered branch (
_ledger.py:506): addsattested_by=None,missing=False✓ - explicit-none branch (
_ledger.py:524): addsmissing=False✓ - MISSING branch (
_ledger.py:541): addsattested_by=None✓
Traced the single consumer: the summary flows to _persist_decision_ledger_summary (_decision_candidates.py:149) and is stored verbatim on PhaseExecution.decision_ledger, which is a free-form dict[str, Any] | None (models/_execution.py:276). _run_hitl_gate.py drives logic off the tuple returns (_ledger_missing / _ledger_explicit_none), never off summary keys. Adding keys is therefore behavior-neutral — the uniformity is purely a downstream-consumer ergonomics win, exactly as the docstring claims.
2. _criteria.py — reviewer rubric text (factually accurate against the code).
I checked each factual claim the new rubric text makes, because a rubric that misinstructs reviewers is worse than none:
- "plan is the last decision surface, so
deferred_to_planis rejected at propose time" — confirmed:_validate_decision_attestation_shape(routes/signals/_validation.py:428-444) hard-fails a plan-phase attestation carrying anydeferred_to_plancandidate. The refine rubric correctly does not make this claim (refine legitimately allows deferral). - disposition semantics (
not_operator_grade= design call the planner/implementer owns;deferred_to_plan= potentially operator-grade) — matchConsideredCandidate(attestation_schemas.py:68-89) andCANDIDATE_DISPOSITIONS(egg_contracts/decisions.py:205). - "Cross-check against the refiner's deferred candidates surfaced in the plan prompt" — the referenced mechanism is real:
_build_deferred_candidates_section→_find_deferred_plan_candidates(_decision_candidates.py:37,85), wired into the plan prompt at_prompt_phase.py:424.
3. Tests — exercise the production path, no anti-patterns.
Both suites import and call the real production functions (_collect_decision_ledger_status, _get_refine_review_criteria, _get_plan_review_criteria) — not hand-built fixtures, not self-seeded goldens. The pre-existing exact-dict assertion at test_decision_ledger_gate.py:112 was correctly updated with the two new keys; the new set(summary) == {...} assertions lock the uniform shape on the other two branches. Neighboring assertions (:659 single-key, :782 identity) survive the key addition unchanged. Rubric assertions match the exact strings added to the criteria.
Non-blocking
- Deferring suggestion 1 (deterministic propose-time deferral cross-check) to #3564 is a sound call — as the PR body notes, hard-closing that leak needs a matching-semantics decision the review didn't settle. Not this PR's job.
Behavior is unchanged except for two additional keys on an opaque persisted dict and prose in two reviewer rubrics. Approving.
— Authored by egg
|
egg review completed. View run logs |
Addresses the non-blocking review suggestions on #3563. Stacked onto that PR's branch (
fix/issue-3526-decision-surfacing) because the gateway only lets egg push to egg-owned branches — merging this into the review branch folds the fixes into #3563.Changes
_collect_decision_ledger_statusnow emits the same key set (registered,resolved,explicit_none,attested_by,missing,candidates_considered) on all three branches, so adecision_ledgerconsumer never has to guess whether an absent key is inapplicable or forgotten. Tests lock the uniform shape.candidates_considered(suggestion 4). The refine and plan reviewer rubrics now describe the enumerated-candidates requirement (and the plan-side cross-check of the refiner's deferred candidates), so reviewers verify the enumeration, not just the free-form rationale.Suggestion 1 (deterministic propose-time deferral cross-check) is deferred to #3564 — hard-closing the leak needs a matching-semantics design decision the review did not settle.
Test Plan
orchestrator/tests/test_decision_ledger_gate.py(uniform-shape assertions),orchestrator/tests/test_pipeline_prompts.py::*reviewer_carries_unsurfaced*(rubric assertions). Both pass locally;ruff checkclean.Issue: #3526
Authored-by: egg