[jonahgabriel]

Ticket: OMN-20368

Why

On 2026-10-02 OMN-20180, OMN-20181 and OMN-20182 reached Done after dod_verify refused them with NO_ACCEPTANCE_CHECKS. The done-flip guard admitted them on its merged-PR path (durable_evidence:all_prs_merged): the only acceptance check it applied everywhere was the unchecked-box scan, and the closing lane had ticked the boxes itself. Their contracts (e.g. contracts/OMN-20180.yaml on onex_change_control origin/dev) declare no binds_ac, so no criterion was ever bound to a check.

What changes

• One sufficient condition for Done: the bound-receipt bar. The OCC contract on origin/dev binds every labelled acceptance criterion (of today's description and of the one the call writes) through binds_ac, each discharged by a PASS receipt. A merged PR, a deploy-readback marker, the close-if-done label and OMN-15712's superseded attachments become further conditions or nothing; none allows on its own.
• Stale receipts are refused: a receipt must match the current contract entry (contract_entry_sha256, recomputed with omnibase_core's compute_contract_entry_sha256), or the legacy whole-file contract_sha256; a receipt naming neither is refused. A receipt behind a merged PR is judged by its commit and contract hash; the no-PR freshness window still applies to no-PR tickets.
• Ticking an acceptance box needs the same receipt: a description or patch edit that checks a box not checked before is refused without it; a create with a checked box is refused; when the current description cannot be read, every checked box counts as a tick.
• A state passed by id is refused (it cannot be classified without a network read, and an unclassified Done would skip every check).
• Fail closed: the wrapper no longer passes through outside an omninode cwd, in lite mode, under the hooks mask, when the library is missing, or when the library errors; main() refuses when decide() raises. The inventory entry records lite_mode_exit: false and no mask gate.
• Bounded OCC fetch: the fetch runs in its own process group with no captured output and is killed at its deadline. During the live probe a captured-output fetch outlived the harness hook timeout and two writes went through unchecked.

Tests

tests/hooks/test_done_flip_guard_requires_bound_receipt.py (28 tests, real git OCC fixture): -k no_acceptance_checks, -k ticked, -k stale, -k bound_pass, -k fail_closed. RED control: 22 of them fail against the origin/dev guard. Existing guard, false-positive and hook-inventory suites updated to the new contract: 147 passed locally.

Lab proof

Hook-side change on the operator Mac's local_macos_claude_hooks profile; no runtime lane is involved. Live proof on the hot-patched load path: on scratch ticket OMN-20373 (canceled), an in-session save_issue patch that ticks a box and an in-session save_issue state=Done were both refused by pre_tool_use_done_flip_guard.sh with ac_tick_without_receipt and no_bound_dod_receipt.

Not in this PR

Node-side Done writers that call the Linear API directly do not pass through this hook (node_linear_triage _mark_done, the sync-revert watchdog, the evidence-autoclose closer). The closer already requires binds_ac coverage; the other two are listed on OMN-20368.

Evidence-Ticket: OMN-20368
Evidence-Source: OCC#12364

[github-actions]

✅ Hostile Reviewer — PASSED

Blocking findings (critical/error): 0
Total findings: 15
Models succeeded: qwen3-review,gpt-oss-review

Findings

Severity	Location	Finding
error	tests/hooks/test_done_flip_guard.py, around line 783:1	ai-reviewer:qwen3-review:correctness Self-referential _BOUND_RECEIPT definition causes NameError at module load | The test fixture _BOUND_RECEIPT is defined as _BOUND_RECEIPT + f'...', which references the variable before it is assigned. This is a NameError that will crash the entire test module at import time, preventing any tests in the file from running. The intent was likely to append the entry hash to a base receipt string, but the base string was never defined independently. | Evidence: _BOUND_RECEIPT = ( _BOUND_RECEIPT + f'c
warning	plugins/onex/hooks/lib/no_pr_bound_evidence.py, bounded_fetch:1	ai-reviewer:qwen3-review:security bounded_fetch kills process group but may orphan git helper processes | When proc.wait(timeout=timeout) raises TimeoutExpired, the code calls os.killpg(proc.pid, signal.SIGKILL). However, start_new_session=True creates a new session for the git process, but any child processes spawned by git (ssh, https helpers) may or may not be in the same process group. If a helper is in a different process group (e.g., ssh forks a background process), SIGKILL to the group will not reach it. The docstring acknowled
warning	plugins/onex/hooks/lib/done_flip_guard.py, apply_description_patch:1	ai-reviewer:qwen3-review:correctness apply_description_patch replace_range logic is fragile and likely incorrect | The replace_range operation finds start and end as unique substrings, then replaces the text between them. However, the logic j = text.find(end, i + len(start)) assumes end appears after start, but does not verify that end is actually after start in the original text. If end appears before start, find will return -1 or find a later occurrence, leading to incorrect replacement. Additionally, the check `text.co
warning	plugins/onex/hooks/lib/done_flip_guard.py, decide():1	ai-reviewer:qwen3-review:performance decide() loads OCC evidence eagerly even when not needed | The decide() function now eagerly calls load_ticket_occ_evidence (which performs a git fetch and multiple git show/list operations) whenever ticket_id is present and occ_probe is not injected. This happens before the decision logic determines whether the bound-receipt bar is actually required. For non-Done edits that do not tick any boxes, this is unnecessary I/O. The previous code only loaded evidence when it fell through to the no-PR path.
warning	plugins/onex/hooks/lib/done_flip_guard.py, decide():1	ai-reviewer:qwen3-review:correctness State-by-id refusal may break legitimate Linear API usage | The guard now refuses any call where state is a UUID, on the grounds that it cannot classify the state without a network read. However, the Linear MCP may legitimately accept state IDs in some contexts, and the refusal message instructs the user to 'pass the state by name'. This is a breaking change for any automation that uses state IDs. The guard does not attempt to resolve the UUID to a state name via the linear_fetcher (which is already ava
warning	tests/hooks/test_done_flip_guard_requires_bound_receipt.py:1	ai-reviewer:qwen3-review:completeness No test coverage for apply_description_patch edge cases | The new apply_description_patch function handles multiple patch operation types (replace, insert_before, insert_after, prepend, append, replace_range), but the test suite only exercises the replace operation via the tick-gate tests. There are no tests for insert_before, insert_after, prepend, append, or replace_range. The replace_range logic is particularly fragile (see separate finding) and is untested. The function returns None on a
info	plugins/onex/hooks/scripts/pre_tool_use_done_flip_guard.sh:1	ai-reviewer:qwen3-review:architecture Shell wrapper fail-closed change may break existing CI pipelines | The shell wrapper pre_tool_use_done_flip_guard.sh now refuses (exit 2) when common.sh fails to source, when the Python library is missing, or when the Python process exits with a non-zero, non-2 code. Previously, these cases resulted in exit 0 (allow). This is a significant behavioral change that may break existing CI pipelines or developer workflows that rely on the guard being a no-op in degraded environments. The change is intentional
info	plugins/onex/hooks/lib/done_flip_guard.py, _CHECKED_BOX_RE:1	ai-reviewer:qwen3-review:correctness checked_acceptance_boxes regex may not match all GFM task-list variants | The regex _CHECKED_BOX_RE matches [x] or [X] but not other possible checked-box representations (e.g., [✓], [✔]). Linear's GFM task-list syntax uses [x]/[X], but if the description contains non-standard checked boxes, they will not be detected. The regex also requires the box to be at the start of a line (with optional whitespace), which is correct for GFM, but the re.MULTILINE flag ensures ^ matches line starts. Thi
info	tests/hooks/test_done_flip_guard_requires_bound_receipt.py, test_fail_closed_occ_fetch_is_bounded_even_when_a_helper_holds_its_pipes:1	ai-reviewer:qwen3-review:testing Test for bounded_fetch uses a fake git that sleeps 30s, but timeout is 1s | The test test_fail_closed_occ_fetch_is_bounded_even_when_a_helper_holds_its_pipes creates a fake git that runs sleep 30 & and sleep 30. The bounded_fetch is called with timeout=1. The test asserts that the call completes in under 10 seconds. However, the fake git's sleep 30 & backgrounds a process that is not in the same process group (it is a child of the shell, not of git). The os.killpg in bounded_fetch will kil
warning	plugins/onex/hooks/lib/no_pr_bound_evidence.py:1	ai-reviewer:gpt-oss-review:correctness MERGED_PR_ENVIRONMENT_FIELDS constant never used | The new constant MERGED_PR_ENVIRONMENT_FIELDS is introduced to allow receipts for merged PRs to include the commit_sha field, but evaluate_bound_evidence always calls receipt_defect with the default _ENVIRONMENT_FIELDS tuple. As a result, receipts that contain the required commit_sha are incorrectly rejected as missing environment fields, breaking the merged‑PR path and causing false negatives. | Evidence: no_pr_bound_evidence.py: added MERGED_PR_ENVIRONMEN
warning	plugins/onex/hooks/lib/no_pr_bound_evidence.py:1	ai-reviewer:gpt-oss-review:security Bounded git fetch silently ignores failures | bounded_fetch runs "git fetch" in a subprocess, kills the process group on timeout, but never checks the exit status. If the fetch fails (network error, authentication failure, repository corruption) the function returns silently, leaving the OCC clone at a stale ref. Subsequent contract loading may use outdated data, allowing a Done transition that should be blocked. | Evidence: no_pr_bound_evidence.py: bounded_fetch creates subprocess, catches OSError, but doe
info	plugins/onex/hooks/lib/done_flip_guard.py:1	ai-reviewer:gpt-oss-review:performance Increased regex processing and contract loading per guard invocation | The rewritten done_flip_guard.py adds multiple regular‑expression scans for checked/unchecked task‑list boxes, description patch application, and loads the OCC contract for every call. This adds CPU and I/O overhead, potentially impacting latency for high‑throughput Linear calls. | Evidence: done_flip_guard.py: added _CHECKED_BOX_RE, _NORMALISE_BOX_TEXT, checked_acceptance_boxes, newly_ticked_boxes, apply_description_patch, plus contract
warning	plugins/onex/hooks/lib/done_flip_guard.py:1	ai-reviewer:gpt-oss-review:correctness State‑by‑ID handling introduces API contract breakage | The guard now refuses Done transitions when the state is supplied as a UUID, returning a custom "state_by_id" reason. Existing clients that pass state identifiers may now receive a block instead of the previous allow, breaking compatibility. | Evidence: done_flip_guard.py: if _UUID_RE.match(state_value): return _refuse(... "state_by_id" ...) | Fix: Document the new requirement that Done states must be passed by name, or add a compatibility mode that tr
info	plugins/onex/hooks/lib/done_flip_guard.py:1	ai-reviewer:gpt-oss-review:architecture Decision function complexity increased dramatically | The decide() function now contains nested helper closures, multiple early returns, and extensive injected‑dependency handling. This makes reasoning about control flow harder, increases the surface for bugs, and complicates testing of individual branches. | Evidence: done_flip_guard.py: extensive inline definitions (_live, _bar, _accepts_merged_pr, etc.) and many conditional branches. | Fix: Refactor decide() into smaller, well‑named functions or classes,
hint	plugins/onex/hooks/lib/no_pr_bound_evidence.py:1	ai-reviewer:gpt-oss-review:style Hard‑coded magic numbers for receipt freshness window | NO_PR_RECEIPT_MAX_AGE is defined as a constant but its value is not documented; callers rely on its default without explicit context, reducing readability. | Evidence: no_pr_bound_evidence.py: NO_PR_RECEIPT_MAX_AGE = timedelta(days=30) | Fix: Add a comment explaining the business rationale for the 30‑day window, or make it configurable via environment variable.

---

Gate semantics (pilot phase)

Verdict	Meaning	Blocks merge?
passed	No critical/error findings	No
blocked	CRITICAL or ERROR findings found	Yes
degraded	Reviewer ran but every model failed mid-run	No (pilot)
infra_error	cli_review failed to run at all (install/crash) after 2 attempts	Yes
unavailable_fork_isolation	Fork PR — the LAN-only model is unreachable and the fleet is off limits	Yes

Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)