Fix verify-review review findings: fail-closed gate, wired loop, honest CLI#2
Fix verify-review review findings: fail-closed gate, wired loop, honest CLI#2kurtvalcorza wants to merge 4 commits into
Conversation
…st CLI Addresses the six findings from the PR review: - review_units.py fails closed: a VERIFIED verdict now requires the universal-floor units (U_cite_external, U_cite_internal, U_consistency) to be present, so an empty or citation-less units map lists them under `missing_units` and can never gate a review complete (was: `all([])` → spurious VERIFIED, exit 0). - Wire verify-review into every operational representation in orchestrate-research: Tree 4 decision tree, Integration 4 pseudocode, the SKILL.md gate-enumeration sentence, and the README architecture flowchart. Phase 5c was previously in the canonical order only, so an agent following the operational logic skipped the loop entirely. - Implement the documented CLI: add `--dry-run` (preview, run nothing) and `--max-cycles` (with `--ceiling` kept as an alias); both were documented in the spec but errored in argparse. Remove the no-op `--strict` flag. - Resolve spec contradictions: header status Draft→Implemented, and §7 "replacing the single-pass fan-out" reworded to "runs after, not replacing" to match orchestrate-research (both modes run; snapshot is cycle 0). Aligned the SKILL.md "replaces/instead of/supersedes" language to match. - Fix plateau detection to catch thrash: detect_plateau now flags "no new best in the last K cycles", so oscillation that nets zero (8,9,8,9,8) trips PLATEAU instead of running to CEILING, while a genuinely descending run still continues. Doc §4 updated to match. - Give the anti-gaming floor-guard mechanical support: review_units.py records per-cycle `denominators` and a `floor_guard` status, flagging a denominator that fell without `exclusions_logged` as UNLOGGED — so a content-removal that games a unit to zero is written into the audit trail, not left to honest self-report. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ba1BaP68T9xVAr4bpkaQFN
…or-guard
Follow-up from reviewing the previous fix commit:
- Revert detect_plateau to the original consecutive-flat-or-worse logic.
The "no new best in K cycles" rewrite false-positived PLATEAU on a
legitimately-descending run: an early low weighted total (a partial
cycle-0 baseline, or scope growth then repair, e.g. 3,10,9,8) poisoned
the global-min anchor, so an actively-improving loop was aborted as a
stall and could never reach VERIFIED. The reverted logic never flags a
strict decrease, so descent-after-growth keeps going. The original
finding (doc over-claimed thrash detection) is now resolved on the doc
side: loop-protocol §4 states plainly that pure oscillation runs to the
ceiling rather than early-stopping — no code claims otherwise.
- floor_guard_status now iterates the union of previous+current
denominator keys, so a denominator whose key is *removed* entirely
(the biggest content removal) is flagged as `->(removed)`, not just a
lowered value. A cycle that supplies no denominators at all remains an
opt-out ("ok"), consistent with denominators being optional.
- derive_consistency_unit returns None (unit absent) when there is no
numeric score, so a consistency object without a score can no longer
fabricate a present-and-zero U_consistency that satisfies the floor
without a genuine >=75 result.
- Doc consistency: loop-protocol §2 output example includes U_cite_internal
(so missing_units:[] is correct); §5 manifest example carries the new
denominators/floor_guard fields; §1 documents carrying floor units
forward on cycles that re-run only changed checks; SKILL.md Step 4 routes
a non-empty missing_units to "run those checks first"; detailed-guide's
record-shape line lists the full field set.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ba1BaP68T9xVAr4bpkaQFN
Addresses three of the four follow-up items from the code review:
- units_in_scope (item 1/2): the input may now declare the frozen in-scope
unit set (spec §3.3). Every declared unit — not just the universal floor —
must be present and 0 before VERIFIED, so a systematic run that silently
omits an in-scope check (e.g. U_prisma) is caught via missing_units instead
of passing. Default (no declaration) still enforces the universal floor
alone, so existing callers are unaffected. orchestrate-research's pseudocode
now passes it.
- Fail-closed input validation (item 4): a new _as_count coerces unit / gate /
score / history / cycle values and rejects booleans (JSON false/true would
otherwise coerce to 0.0/1.0 and a `false` could satisfy the all-zero
predicate), null, and non-numeric strings. Structural type checks reject a
non-object top-level payload and non-object units/gates/consistency/
denominators. Malformed input now returns a clean {"error": ...} verdict with
a non-zero exit (fails closed) instead of crashing with a traceback or
slipping a bad value through the gate. Also guards append_to_manifest against
a null gates map and a non-dict manifest tail element.
Docs updated: loop-protocol §1 schema + rules, SKILL.md Step 1, and the module
docstring document units_in_scope and the fail-closed behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ba1BaP68T9xVAr4bpkaQFN
Item 3 from the code review, lightweight approach (per author's choice): add verify-review as an OPTIONAL verified-end-state step after each orchestrator's existing validation phase, mirroring orchestrate-research's framing (the single-pass snapshot is the loop's cycle 0; the snapshot alone suffices for a quick draft, verify-review drives it to a verified end-state for a submission-ready review). No phase/checkpoint renumbering — the existing workflows are unchanged; verify-review is an additive layer. - synthesize-research: added to the specialist list, the Phase 4 completion rule, the status dashboard, and the Related list (SKILL.md + README.md). - review-literature: added an optional verified-end-state note after Phase 7, the outputs list, the README phase list + Related, and the orchestrator decision-tree terminal node. Deep reference files (checkpoint-protocol, workflow-phases, dependency-matrix, JSON state templates) keep the core 7-phase workflow unchanged since the loop is optional and additive. - screen-literature canonical-order strings (README + SKILL) now include verify-review between the validation checks and prisma-flow, matching orchestrate-research's canonical order. (validate-evidence's upstream chain ends at GRADE and is left as-is — it is not a full-pipeline order.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ba1BaP68T9xVAr4bpkaQFN
|
@claude please re-review this PR. Since the last review, this branch has been updated with:
Focus areas for the re-review: the fail-closed logic in Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfbfc8be70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "weighted_total": round(weighted_total, 3), | ||
| "auto_units_zero": auto_zero, | ||
| "gates_remaining": gates_remaining, | ||
| "missing_units": missing_units, |
There was a problem hiding this comment.
When a required unit is missing but the weighted total has been flat, the state has already been selected by the plateau/ceiling branches before this field is returned. For example, a partial units file with {} and history: [0, 0, 0] now reports PLATEAU instead of a recoverable CONTINUE with missing checks, so the documented “run those checks first” path never executes. Give missing_units precedence over plateau/ceiling, or force CONTINUE, so fail-closed inputs can be repaired rather than stopped.
Useful? React with 👍 / 👎.
| guard = floor_guard_status(prev_denoms, denominators, | ||
| bool(data.get("exclusions_logged"))) |
There was a problem hiding this comment.
When a caller serializes exclusions_logged as a string such as "false", bool(...) treats it as true, so a denominator drop is recorded as logged-exclusion instead of UNLOGGED. That masks exactly the content-removal audit trail this floor guard is meant to preserve. Reject non-boolean values or compare explicitly before passing this flag to floor_guard_status.
Useful? React with 👍 / 👎.
| # predicate uses RAW counts: every required unit present AND all == 0 | ||
| auto_units_zero = not missing_units and all(c == 0 for c in units.values()) | ||
|
|
||
| raw_gates = data.get("gates") or {} |
There was a problem hiding this comment.
When gates is an empty array or empty string, data.get("gates") or {} coerces it to {} before the type check runs. With the universal floor units at zero, that malformed payload now returns VERIFIED with exit 0 instead of failing closed, so a bad gates artifact can silently clear all human gates. Preserve None/missing as the only default case and reject any non-object value, even when it is empty.
Useful? React with 👍 / 👎.
| "denominators": denominators, | ||
| "floor_guard": guard, |
There was a problem hiding this comment.
When --manifest detects a denominator drop with exclusions_logged: false, floor_guard is recorded as UNLOGGED, but the result state has already been left as VERIFIED and main() returns exit 0. In the content-removal scenario this guard is meant to catch, a pipeline using the exit code can still mark the review complete after deleting citations/studies/themes, contradicting the “no-op per §5” status. Fold UNLOGGED into the verdict before returning success, or otherwise make it a blocking state.
Useful? React with 👍 / 👎.
| snapshot_results={ # seed cycle 0 from the snapshots above (no re-run) | ||
| "citation_score": citation_score, | ||
| "verification": verification, | ||
| "consistency_score": consistency_score, | ||
| }, |
There was a problem hiding this comment.
For systematic/scoping/rapid/umbrella runs, scope_for(...) declares units such as U_prisma and U_grade, but this cycle-0 payload only seeds citation, source-verification, and consistency snapshots. Since the backend now refuses to verify declared-but-missing units, the orchestrated path can enter verify-review without PRISMA/GRADE values and stall on missing units before the later reporting phase runs; include the prisma-flow/validate-evidence snapshots or run those checks before invoking the loop.
Useful? React with 👍 / 👎.
| return breaks + max(0, CONSISTENCY_GATE - float(score)) | ||
| return None | ||
| breaks = _as_count(consistency.get("critical_breaks", 0), "consistency.critical_breaks") | ||
| return breaks + max(0, CONSISTENCY_GATE - _as_count(score, "consistency.score")) |
There was a problem hiding this comment.
When consistency.score is non-finite ("nan") or critical_breaks is negative, _as_count accepts the value and this formula can produce U_consistency == 0 (e.g. score 70 with critical_breaks: -5, or score "nan"). With the citation units at zero, the review is then marked VERIFIED even though the consistency gate was not genuinely ≥75 with no breaks. Validate finite, non-negative consistency inputs before computing the unit.
Useful? React with 👍 / 👎.
| flags the drop; a human/agent judges legitimacy — it never silently credits | ||
| a removal. | ||
| """ | ||
| if not curr_denoms or not prev_denoms: |
There was a problem hiding this comment.
If a previous manifest record has denominators and the next cycle omits denominators or passes {}, this early return reports floor_guard: "ok" before the union-of-keys removal logic runs. That misses the exact “key removed entirely” case the new guard is supposed to audit, so a cycle can delete all citation/study/theme denominators without an UNLOGGED marker. Only skip when there is no previous denominator baseline; otherwise compare the previous keys against the empty current set.
Useful? React with 👍 / 👎.
| raw_gates = data.get("gates") or {} | ||
| if not isinstance(raw_gates, dict): | ||
| raise InputError("gates: expected an object") | ||
| gates_remaining = sum(int(_as_count(raw_gates.get(k, 0), f"gates.{k}")) for k in GATE_KEYS) |
There was a problem hiding this comment.
When a gate count is fractional between 0 and 1, int(...) rounds it down to zero, so an input like {"H_rob": 0.9} can return VERIFIED with no human handoff. Gate counts are counts and should be non-negative integers; reject fractional values instead of truncating them before summing.
Useful? React with 👍 / 👎.
2 participants
Stacks onto #1 (base is
verify-review-loop) and addresses the six findings from the review of that PR, plus the follow-ups surfaced by a code-review of the fix itself.Correctness fixes (original six findings)
review_units.pyreturnedVERIFIED(exit 0) on an empty or partial units map —all([])is vacuously true andreview_typewas never consulted. AVERIFIEDverdict now requires the universal-floor units (U_cite_external,U_cite_internal,U_consistency) to be present; anything absent is listed under a newmissing_unitsfield and the verdict can never beVERIFIED. An empty/citation-less input can no longer gate a review complete.verify-reviewto the Tree 4 validation decision tree, the Integration 4 pseudocode (seeded from the snapshot as cycle 0), the SKILL.md gate-enumeration sentence, and the README architecture flowchart.--dry-run(prints the predicate / in-scope units / gates / ceiling, runs nothing) and--max-cycles(with--ceilingkept as an alias) — both were documented in the spec but errored in argparse. Removed the self-described no-op--strict.Draft — no implementation yet→Implemented; §7 "replacing the single-pass fan-out" reworded to "runs after, not replacing" to match orchestrate-research (both modes run, snapshot is cycle 0). Aligned the SKILL.md "replaces/instead of/supersedes" wording to the same.detect_plateaukeeps the original consecutive flat-or-worse definition — an actively-descending loop (even after a mid-run rise, e.g.3,10,9,8) is never falsely stalled. The original finding (the doc over-claimed that it caught oscillation/thrash) is resolved on the doc side: loop-protocol §4 now states plainly that pure oscillation runs to the ceiling rather than early-stopping. (An interim commit tried a "no new best in K cycles" rewrite; it false-positived PLATEAU on descent-after-growth and was reverted — see the code-review follow-up below.)Anti-gaming (altitude)
review_units.pynow records per-cycledenominatorsand afloor_guardstatus, flagging a denominator that fell — or whose key was removed entirely — withoutexclusions_loggedasUNLOGGED. Judging legitimacy still stays with the human/agent, but the drop is written into the audit trail instead of trusting self-report.Follow-ups from reviewing the fix
units_in_scopefield: when the caller declares the frozen in-scope set (spec §3.3), every listed unit — not just the universal floor — must be present and 0 beforeVERIFIED, so a systematic run that silently omitsU_prismais caught instead of passing. Default (no declaration) enforces the universal floor alone, so existing callers are unaffected._as_countcoerces every numeric field and rejects booleans (JSONfalse/truewould otherwise coerce to0.0/1.0and afalsecould satisfy the all-zero predicate), null, and non-numeric values; structural type checks reject a non-object payload and malformedunits/gates/consistency/denominators. Malformed input now returns a clean{"error": …}verdict with a non-zero exit rather than a traceback or a bad value slipping through.consistencyobject without a numeric score no longer fabricates a present-and-zeroU_consistency; it stays absent (fails closed) until a real ≥75 result exists.synthesize-researchandreview-literaturenow presentverify-reviewas an optional verified-end-state step after their existing validation phase (the snapshot is its cycle 0), without renumbering phases/checkpoints. The twoscreen-literaturecanonical-order strings now include it between the validation checks andprisma-flow, matching orchestrate-research's canonical order.Verification
review_units.pyexercised acrossVERIFIED/BLOCKED_ON_HUMAN/CONTINUE/PLATEAU/CEILING, the fail-closed andmissing_unitspaths, the plateau non-regression cases (stall → PLATEAU; descent-after-growth and real progress → CONTINUE),units_in_scopescoping (both directions), malformed-input fail-closed (bool/null/non-numeric → error + exit 2),--dry-run,--max-cycles/--ceiling, and the floor-guard value-drop / key-removal / logged / opt-out paths.py_compileclean; all touched-doc code fences balanced; everyverify-reviewcross-link resolves.🤖 Generated with Claude Code
Generated by Claude Code