Skip to content

Fix verify-review review findings: fail-closed gate, wired loop, honest CLI#2

Draft
kurtvalcorza wants to merge 6 commits into
verify-review-loopfrom
claude/agentic-research-pr-review-gwsusw
Draft

Fix verify-review review findings: fail-closed gate, wired loop, honest CLI#2
kurtvalcorza wants to merge 6 commits into
verify-review-loopfrom
claude/agentic-research-pr-review-gwsusw

Conversation

@kurtvalcorza

@kurtvalcorza kurtvalcorza commented Jul 5, 2026

Copy link
Copy Markdown
Owner

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)

  • Fail-closed gate. review_units.py returned VERIFIED (exit 0) on an empty or partial units map — all([]) is vacuously true and review_type was never consulted. A VERIFIED verdict now requires the universal-floor units (U_cite_external, U_cite_internal, U_consistency) to be present; anything absent is listed under a new missing_units field and the verdict can never be VERIFIED. An empty/citation-less input can no longer gate a review complete.
  • Loop wired into orchestrate-research. Phase 5c had been added to the canonical order but left out of every representation an agent actually executes from. Added verify-review to 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.
  • CLI matches the docs. Implemented --dry-run (prints the predicate / in-scope units / gates / ceiling, runs nothing) and --max-cycles (with --ceiling kept as an alias) — both were documented in the spec but errored in argparse. Removed the self-described no-op --strict.
  • Spec contradictions resolved. Status header Draft — no implementation yetImplemented; §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.
  • Plateau doc corrected (no code behavior change). detect_plateau keeps 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)

  • The floor-guard was prose-only and the manifest record carried no denominators, so a content-removal that games a unit to zero was undetectable even after the fact. review_units.py now records per-cycle denominators and a floor_guard status, flagging a denominator that fell — or whose key was removed entirely — without exclusions_logged as UNLOGGED. 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

  • Declared review-type scope. Optional units_in_scope field: 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 before VERIFIED, so a systematic run that silently omits U_prisma is caught instead of passing. Default (no declaration) enforces the universal floor alone, so existing callers are unaffected.
  • Malformed input fails closed. A new _as_count coerces every numeric field 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 values; structural type checks reject a non-object payload and malformed units/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.
  • Consistency floor tightened. A consistency object without a numeric score no longer fabricates a present-and-zero U_consistency; it stays absent (fails closed) until a real ≥75 result exists.
  • Verify-review wired into the other orchestrators (lightweight, optional). synthesize-research and review-literature now present verify-review as an optional verified-end-state step after their existing validation phase (the snapshot is its cycle 0), without renumbering phases/checkpoints. The two screen-literature canonical-order strings now include it between the validation checks and prisma-flow, matching orchestrate-research's canonical order.

Verification

review_units.py exercised across VERIFIED / BLOCKED_ON_HUMAN / CONTINUE / PLATEAU / CEILING, the fail-closed and missing_units paths, the plateau non-regression cases (stall → PLATEAU; descent-after-growth and real progress → CONTINUE), units_in_scope scoping (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_compile clean; all touched-doc code fences balanced; every verify-review cross-link resolves.

🤖 Generated with Claude Code


Generated by Claude Code

claude added 4 commits July 5, 2026 12:15
…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

Copy link
Copy Markdown
Owner Author

@claude please re-review this PR.

Since the last review, this branch has been updated with:

  • Fail-closed gatereview_units.py requires the universal-floor units (U_cite_external, U_cite_internal, U_consistency) to be present before VERIFIED; empty/partial input now surfaces missing_units and can never pass.
  • Declared review-type scope — optional units_in_scope makes every in-scope unit (not just the floor) required before VERIFIED, so a systematic run silently omitting U_prisma is caught.
  • Malformed input fails closed — non-numeric/boolean/null counts and wrong field types now return a clean {"error": …} verdict with a non-zero exit instead of a traceback or a bad value slipping through.
  • Plateau — kept the original consecutive-flat-or-worse detect_plateau (an interim "no new best in K cycles" rewrite regressed on descent-after-growth and was reverted); the doc was corrected instead of over-claiming thrash detection.
  • Floor-guard — records per-cycle denominators + a floor_guard status, flagging a value drop or a removed key without exclusions_logged.
  • Wiringverify-review added to orchestrate-research's operational paths (decision tree, pseudocode, gate sentence, flowchart), and as an optional verified-end-state step in synthesize-research / review-literature + the screen-literature canonical-order strings.

Focus areas for the re-review: the fail-closed logic in review_units.py (compute / verdict / _as_count), the reverted detect_plateau, and the floor_guard_status key-union handling.


Generated by Claude Code

@kurtvalcorza

Copy link
Copy Markdown
Owner Author

@codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep missing-unit verdicts recoverable

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 👍 / 👎.

Comment on lines +294 to +295
guard = floor_guard_status(prev_denoms, denominators,
bool(data.get("exclusions_logged")))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate exclusion logging as a boolean

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 {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject empty malformed gates

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 👍 / 👎.

Comment on lines +304 to +305
"denominators": denominators,
"floor_guard": guard,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Block unlogged floor-guard drops

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 👍 / 👎.

Comment on lines +1477 to +1481
snapshot_results={ # seed cycle 0 from the snapshots above (no re-run)
"citation_score": citation_score,
"verification": verification,
"consistency_score": consistency_score,
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Seed all declared verify-review units

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"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject invalid consistency values before deriving zero

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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Flag removal of all denominator keys

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not truncate fractional human gates

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 👍 / 👎.

The fail-closed hardening had covered the units half of the predicate but left
holes on the gates half, the consistency floor, and non-finite/error paths. This
closes every fail-open an internal code review and the Codex bot review surfaced
(the two sets overlapped almost entirely):

Spurious-VERIFIED closures:
- U_consistency is now derived ONLY from the `consistency` object; a value placed
  directly in `units` is ignored, so it can't fabricate a present-and-zero floor.
- Gate counts are validated as non-negative integers: a negative count can no
  longer cancel a positive one, and a fractional 0.9 no longer truncates to 0 and
  drops a pending human gate.
- Declaring `units_in_scope` now also requires the `gates` key to be present, so
  an omitted gates object cannot silently assert all human gates confirmed.
- consistency `critical_breaks`/`score` are validated non-negative + finite, so a
  negative break count can't cancel the sub-gate gap to a spurious 0.
- NaN/Infinity are rejected (they blinded plateau detection and emitted invalid
  JSON); empty non-objects ([]/"") where an object is expected are rejected
  instead of coercing to {} via `or {}`.

Other correctness/robustness:
- A missing in-scope unit is reported as CONTINUE (or CEILING), never mislabeled
  PLATEAU — incomplete input is recoverable, not a false stall.
- An UNLOGGED floor-guard denominator drop (incl. wiping all denominators) now
  HOLDS a would-be VERIFIED as BLOCKED_ON_HUMAN with a hold_reason, so the exit
  code can't mark a review complete when its units may have been zeroed by
  removing content. Denominator values are validated numeric so a crafted value
  can't blind the guard; exclusions_logged must be a real boolean.
- The input file is read inside the try block (missing/unreadable file → clean
  error verdict + exit 2, not a traceback).

Docs updated: module docstring, loop-protocol §1/§2/§6, SKILL.md floor-guard,
orchestrate-research cycle-0 seeding (must seed every declared unit incl.
prisma/grade), and the review-literature checkpoint label.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ba1BaP68T9xVAr4bpkaQFN

Copy link
Copy Markdown
Owner Author

Thanks @codex — all nine suggestions are addressed in f8b8d3c (they also matched an internal review run in parallel, so this was well-corroborated). Summary:

Codex comment Resolution
exclusions_logged as boolean (L295) exclusions_logged must be a real boolean; is True compare — a "false" string no longer reads as logged
Reject empty malformed gates (L153) gates/units/denominators reject any non-object (incl. []/""); only absent/null defaults to {}
Missing-unit recoverable (L221) A missing in-scope unit now reports CONTINUE/CEILING, never PLATEAU
Block unlogged floor-guard drops (L305) An UNLOGGED drop holds a would-be VERIFIED as BLOCKED_ON_HUMAN (with hold_reason), so the exit code can't mark a content-removal complete
Seed all declared units (guide L1481) Cycle-0 pseudocode now seeds prisma/grade snapshots and notes every declared in-scope unit + gates must be present before the loop
Reject invalid consistency (L116) score/critical_breaks validated finite + non-negative, so critical_breaks:-5 can't cancel the gap to a spurious 0
Don't truncate fractional gates (L156) Gate/cycle/denominator counts validated as non-negative integers (no truncation, no negative cancellation)
Flag removal of all denominator keys (L248) Wipe-all is flagged: early-return only when there's no prior baseline

Also closed a related fail-open the reviews surfaced: U_consistency supplied directly in units bypassed the score requirement — it's now derived only from the consistency object. NaN/Infinity are rejected (they blinded plateau detection and emitted invalid JSON), and a missing input file now yields a clean error verdict instead of a traceback.


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

Copy link
Copy Markdown
Owner Author

@claude please re-review — latest state is f8b8d3c.

Since the last @claude review, the branch has had two more rounds of hardening driven by an internal code-review and a @codex review (whose findings matched almost 1:1):

  • Fail-open closures in review_units.py: U_consistency is now derived only from the consistency object (a value in units is ignored); gate/cycle/denominator counts must be non-negative integers (no negative-cancellation, no 0.90 truncation); critical_breaks/score validated finite + non-negative; NaN/Infinity and empty non-objects ([]/"") rejected; declaring units_in_scope also requires the gates key present.
  • Anti-gaming now blocking: an UNLOGGED denominator drop (incl. wiping all denominators) holds a would-be VERIFIED as BLOCKED_ON_HUMAN with a hold_reason, so the exit code can't mark a content-removal complete.
  • Recoverability: a missing in-scope unit reports CONTINUE/CEILING, never a false PLATEAU; a missing/unreadable input file yields a clean error verdict instead of a traceback.

Focus areas: compute / verdict / _as_count / _as_int_count and the gate + consistency validation in review_units.py, and the floor_guard_status blocking path in append_to_manifest / main.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8b8d3c039

ℹ️ 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".

Comment on lines +363 to +365
prev = history[-1] if history and isinstance(history[-1], dict) else {}
prev_denoms = prev.get("denominators") if isinstance(prev.get("denominators"), dict) else {}
guard = floor_guard_status(prev_denoms, denominators, excl is True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep unlogged denominator drops sticky

When a denominator drops during a non-final cycle, or after the first held BLOCKED_ON_HUMAN record is retried, this uses the immediately previous record's already-reduced denominators as the next baseline. The following cycle with the same reduced denominators gets floor_guard: ok, so an unlogged content removal can still exit VERIFIED after one more invocation. The baseline needs to stay anchored to the last non-UNLOGGED/adjudicated denominator set, or the hold needs to remain sticky until the drop is logged.

Useful? React with 👍 / 👎.

Comment on lines +210 to +211
if declared and "gates" not in data:
raise InputError("gates: required (even as {}) when units_in_scope is declared")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat an empty declared scope as requiring gates

When callers include units_in_scope: [] for a floor-only scoped run, declared is falsey here, so an omitted gates object bypasses the new fail-closed check and defaults every human gate to zero. That lets a scoped invocation reach VERIFIED without ever providing the gates object the protocol now requires; check whether the units_in_scope key was present rather than relying on list truthiness.

Useful? React with 👍 / 👎.

Comment on lines +172 to +173
units = {key: _as_count(count, f"units.{key}")
for key, count in raw_units.items() if key != "U_consistency"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject negative unit counts before computing totals

Unit counts are still parsed with _as_count, which allows negative values. With malformed input such as a negative required citation/screen count, the weighted total can go negative and dominant-unit routing can become null or misleading instead of returning an error, so the fail-closed gate can stall or report fake progress; use the non-negative validator for unit counts as is already done for gates and denominators.

Useful? React with 👍 / 👎.

Comment on lines +199 to +200
required = list(UNIVERSAL_FLOOR) + [u for u in declared if u not in UNIVERSAL_FLOOR]
missing_units = [u for u in required if u not in units]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate units_in_scope entries before using them

If units_in_scope contains a non-string JSON value such as a nested array, the membership check here raises TypeError and prints a traceback instead of the promised clean error verdict. Since this field is now part of the fail-closed gate, validate each entry is a unit-name string before building required, so malformed scope declarations cannot escape the InputError path.

Useful? React with 👍 / 👎.

Comment on lines +268 to +272
elif missing_units:
# Incomplete input (a required in-scope unit was not reported) is not a
# repair stall: never mislabel it PLATEAU. Keep going so the agent can
# supply the missing check; the ceiling still bounds a misconfigured run.
state = "CEILING" if cycle >= ceiling else "CONTINUE"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear missing units before routing by dominant_unit

When a required unit is missing but another reported unit has a positive contribution, this branch still leaves the result as CONTINUE, and the response later exposes that other unit as dominant_unit. Clients following the existing CONTINUEdominant_unit routing contract can keep repairing reported defects while the absent in-scope check is never run, even though the protocol says non-empty missing_units must be cleared first; this state should suppress or replace dominant routing with the missing checks.

Useful? React with 👍 / 👎.

Comment thread skills/screen-literature/SKILL.md Outdated
- `appraise-risk-of-bias` — downstream of extraction; per-study RoB with the design-appropriate instrument (RoB 2 / ROBINS-I / Newcastle-Ottawa / QUADAS-2), **human-gated**. Like dual screening here, RoB appraisal benefits from dual review, but stays human-gated (LLM appraisal accuracy is the weakest link).
- `prisma-flow` — downstream reporting; assembles the PRISMA 2020 flow diagram from this skill's screening/eligibility counts plus the upstream identification/duplicate counts.
- Canonical order: `design-review-protocol` → `generate-screening-criteria` → `acquire-corpus` → `dedupe-records` → **`screen-literature`** (single-pass or DUAL) → extract/synthesize → `appraise-risk-of-bias` → `validate-evidence` (GRADE) / draft → `prisma-flow`.
- Canonical order: `design-review-protocol` → `generate-screening-criteria` → `acquire-corpus` → `dedupe-records` → **`screen-literature`** (single-pass or DUAL) → extract/synthesize → `appraise-risk-of-bias` → `validate-evidence` (GRADE) / draft → `validate-*` + `verify-sources` → `verify-review` (loop to verified end-state) → `prisma-flow`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Move PRISMA before verify-review in the canonical order

For systematic/scoping-style runs, verify-review now requires U_prisma to be seeded in units_in_scope, and the detailed guide explicitly says to run prisma-flow before invoking the loop. This canonical order places verify-review before prisma-flow, so users following it will start the loop without the PRISMA baseline and hit missing_units instead of a valid cycle-0 snapshot.

Useful? React with 👍 / 👎.

Comment on lines +104 to +108
elif isinstance(x, str):
try:
v = float(x)
except ValueError:
raise InputError(f"{ctx}: not a number ({x!r})")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject numeric strings as count fields

This still accepts strings and coerces them with float(), so malformed JSON such as "cycle":"0", gate counts of "0", or unit counts of "0" can satisfy the gate and even return VERIFIED. The new contract says wrong field types fail closed, so count-like fields should require JSON numbers rather than accepting numeric-looking strings.

Useful? React with 👍 / 👎.

├─ Phase 6 complete?
│ └─ Run validate-consistency (AUTO FINAL GATE — single-pass snapshot)
│ Score ≥75? → PASS (continue to Phase 5c)
│ Score <75? → HALT (user fixes issues, re-validate)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Let verify-review handle failing snapshots

This workflow still halts when the consistency snapshot scores below 75, before Phase 5c can run. For exactly those cases review_units.py derives a positive U_consistency and the new loop is supposed to repair and re-check from the cycle-0 snapshot; gating Phase 5c on a passing snapshot bypasses the self-correcting loop and falls back to manual fixes instead.

Useful? React with 👍 / 👎.

Closes findings from an internal code-review and a second Codex review of
f8b8d3c (the two overlapped heavily):

- [P1] Floor-guard is now STICKY. The baseline for the cross-cycle comparison is
  the denominators of the last ACCEPTED cycle (floor_guard not UNLOGGED), not the
  immediately previous record. Previously an unlogged drop became the next
  cycle's baseline, so repeating the reduced denominators one more cycle read as
  `ok` and could exit VERIFIED. Now an unlogged drop stays flagged/held until the
  count is restored or a logged exclusion re-baselines it.
- Unit counts are validated non-negative (were `_as_count`, which allowed
  negatives that skew the weighted total and routing).
- Unknown/misnamed gate keys are rejected (e.g. a typo'd `H_robb` no longer
  silently drops a pending human gate and reaches VERIFIED).
- Declaring `units_in_scope` (any list incl. `[]`) requires `gates` to be a
  present object; a null or omitted gates value is rejected, closing the
  empty-scope / `gates:null` bypass.
- `units_in_scope` entries are validated as strings (a nested-array entry used to
  raise a TypeError traceback instead of a clean error verdict).
- Numeric strings are rejected as counts — the "wrong field types fail closed"
  contract now holds for `"0"` etc.
- `missing_units` non-empty now suppresses `dominant_unit`, so a client can't keep
  repairing a reported unit while the absent in-scope check is never run.
- `--max-cycles`/`--ceiling` < 1 is rejected (a negative ceiling forced CEILING
  at cycle 0).

Docs:
- Canonical-order strings now place `prisma-flow` before `verify-review` (the loop
  consumes the prisma reconciliation as U_prisma), in orchestrate-research and
  screen-literature.
- Tree 4 routes a sub-75 consistency snapshot INTO Phase 5c (verify-review repairs
  it via the loop) instead of halting before the loop can run.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ba1BaP68T9xVAr4bpkaQFN

Copy link
Copy Markdown
Owner Author

Thanks @codex — all eight addressed in 6e88f81 (they overlapped closely with an internal review I ran in parallel).

Codex comment Resolution
[P1] Sticky unlogged drops (L365) Baseline is now the last accepted cycle's denominators (floor_guard not UNLOGGED), not the previous record — a repeated reduced count stays held until restored or a logged exclusion re-baselines it
Negative unit counts (L173) Units validated with _as_nonneg_count
Clear missing units before routing (L272) Non-empty missing_units now suppresses dominant_unit
Validate units_in_scope entries (L200) Entries must be strings — a nested value now yields a clean error, not a traceback
Empty declared scope requires gates (L211) Presence of the units_in_scope key (incl. []) requires gates to be a present object; null/omitted rejected
Reject numeric strings (L108) _as_count requires a JSON number; "0" etc. fail closed
PRISMA before verify-review (screen-lit L94) Canonical-order strings reordered prisma-flow → verify-review (the loop consumes the reconciliation as U_prisma); applied in orchestrate-research too
Let verify-review handle failing snapshots (guide L1351) Tree 4 now routes a sub-75 consistency snapshot into Phase 5c so the loop repairs it, instead of halting before the loop runs

Also fixed an internal-review find not in your set: an unknown/typo'd gate key (e.g. H_robb) silently dropped a pending human gate and reached VERIFIED — unknown gate keys are now rejected. Verified across the verdict states plus a sticky-drop sequence (unlogged → held on repeat → restored/logged → accepted).


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants