Repository navigation
evidence(OMN-14714): fail-closed replacements for two fail-open negative probes - #5277
Conversation
…ive probes Adversarial verification of OCC#5272 found two of its five landed probes FAIL OPEN. Both are `! <gh api ...> | base64 -d | grep -q ...`; the `!` negates the pipeline exit status, i.e. grep is. When `gh api` yields no output (GC-d ref, API outage, rate limit, auth failure) grep exits 1 and `!` inverts it to exit 0 - a vacuous PASS that would be written into the evidence store as a receipt. Proven by execution against ref deadbeef...deadbeef through the same `sh -c` path run_contract_compliance_check uses: dod-14714-merged-0747500e-stale-width-removed GREEN=0 RED=1 DEAD=0 FAIL-OPEN dod-14714-merged-0747500e-xfail-flipped GREEN=0 RED=1 DEAD=0 FAIL-OPEN dod-14714-failclosed-stale-width-removed GREEN=0 RED=1 DEAD=1 ok dod-14714-failclosed-xfail-flipped GREEN=0 RED=1 DEAD=1 ok Nothing upstream catches the fail-open shape: validate_pr_deploy_required skips a leading `!` when locating command position, so both still classify falsifiable=True, and scripts/lint_contract_check_values.py has no rule for it. Two appended entries carry supersedes_dod_evidence markers retiring the fail-open pair. The repair captures the API response to a variable and asserts it non-empty BEFORE negating the grep. The xfail entry is also scope-narrowed: the superseded form asserted zero pytest.mark.xfail anywhere in the module, so any future unrelated xfail in that file would flip OMN-14714 evidence RED. The replacement windows the assertion to the six lines above the target test (where the strict xfail sat at parent 848d3333) and first asserts the test function is still present. The new entry description also corrects a false "verified by execution" claim in the superseded entry (append-only, so it cannot be edited in place): the self-bind shape it said "classifies INERT" does not - _is_inert_check returns False and classify_check_value returns falsifiable=True for it. Append-only: no landed entry edited or removed. Evidence integrity only; deploy-gate falsifiability is report-mode and this changes no gate verdict.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…acements Two PASS receipts under drift/dod_receipts/OMN-14714/, one per appended entry, bound to contract_sha256 f49b4e7d (the committed contract bytes) and per-entry contract_entry_sha256 (OMN-13888 canonical-JSON scheme, cross-validated by recomputing an already-merged receipt of this ticket and matching its landed digest exactly). Each receipt records the full three-ref exit matrix, including exit 1 at the nonexistent ref where the superseded entry returns exit 0 - the fail-open defect being retired. yamlfmt is a byte-exact no-op on both receipts on the first generation pass (literal block-scalar dumper), so the OMN-14714 corruption mode is not reintroduced by this PR is own artifacts.
1 participant
OMN-14714 — fail-closed replacements for two fail-open negative probes
Evidence-Source: OmniNode-ai/onex_change_control
Evidence-Ticket: OMN-14714
Third-party remediation of OCC#5272 (merged
b2147b5a43f663742b2a9448d316be8c78914367). I am neither #1927's nor #5272's implementer.The defect
Two of the five probes OCC#5272 landed fail open:
dod-14714-merged-0747500e-stale-width-removeddod-14714-merged-0747500e-xfail-flippedBoth have the shape
! <gh api ...> | base64 -d | grep -q .... The!negates the pipeline exit status — grep's. Whengh apiproduces no output (GC'd ref, API outage, rate limit, auth failure), grep exits 1 and!inverts it to exit 0 = PASS. The probe reports GREEN while observing nothing, and a re-execution during any API hiccup writes a falsePASSreceipt into the evidence store.This is strictly worse than the durability defect OCC#5272 existed to fix. Its own argument was that a dead ref should make a probe error rather than go honestly RED; two of its five replacements go silently GREEN instead.
Nothing upstream catches it.
validate_pr_deploy_required._command_ofexplicitly skips a leading!when locating command position, so both still classifyfalsifiable=True;scripts/lint_contract_check_values.pyhas no rule for this shape.Execution matrix (
/bin/sh -c, the same pathrun_contract_compliance_check._check_commanduses)Run against the check_values parsed out of the landed / proposed YAML, at three refs:
GREEN =
0747500e682c903301bc1bd9eb56578c6dad72af(omnimarket#1927 squash), RED = parent848d3333553b5838d9ad1635be89e9d89f0f850b, DEAD =deadbeefdeadbeefdeadbeefdeadbeefdeadbeef.The repair
Two appended entries, each carrying
supersedes_dod_evidence:<fail-open id>:dod-14714-failclosed-stale-width-removeddod-14714-merged-0747500e-stale-width-removed$out, asserttest -n "$out"before negating the grepdod-14714-failclosed-xfail-flippeddod-14714-merged-0747500e-xfail-flippedScope narrowing (second defect). The superseded xfail probe asserted zero
pytest.mark.xfailoccurrences anywhere in the module, so any future unrelated xfail added to that test file would flip OMN-14714's evidence RED for reasons unrelated to OMN-14714. Simulated by execution on a copy of the merged file with one unrelated xfail inserted 40 lines above the target test: superseded form exits 1 (false RED), replacement exits 0. The replacement windows the absence assertion to the six lines directly abovedef test_merged_path_supersession_is_yamlfmt_clean— where the strict xfail actually sat at parent848d3333(lines 454-459) — and first asserts the test function is still present, so a rename cannot make the window vacuous.Correction of a false claim. OCC#5272's
dod-14714-merged-0747500e-xfail-flippeddescription states: "Verified: the obvious candidate (gh api .../pulls/5272/files --jq .patch | grep ...) classifies INERT." That is false and was not verified by execution. Re-run against both real evaluators:contract_compliance_check._is_inert_check→False(ACTIVE);validate_pr_deploy_required.classify_check_value→falsifiable=True; noSELF_REFERENTIAL_PATTERNSentry matches (the string contains neithercontracts/OMN-nordod_receipts). The two mechanisms also differ — OCC's_INERT_CHECK_PATTERNSuses a negative lookbehind(?<![\w/])contracts/OMN-, so even a genuine own-contract read via a contents-API path is not inert to OCC; only the deploy-gate's lookbehind-free pattern catches it. Omitting a self-bind entry stands as a decision; the justification recorded for it was wrong. Since dod_evidence is append-only, the correction is recorded in the superseding entry's description and in a comment on #5272.Verified against the real evaluators (not asserted)
validate_pr_deploy_required.classify_check_value(fromomniclaude@main): both new entriesfalsifiable=True(['gh-api']);has_deploy_evidence→True.contract_compliance_check(fromonex_change_control@dev):_is_inert_check→ False for both (ACTIVE);_non_hermetic_reason→None;_has_effective_check→ True;_superseded_dod_ids→ 6 (the 4 from evidence(OMN-14714): bind #1927's deliverables at the durable squash commit, retire the hollow entries #5272 plus the 2 new retirements).scripts/lint_contract_check_values.py→ exit 0.yamlfmtwith OCC's.yamlfmtis a byte-exact no-op on the contract and both receipts (the OMN-14714 failure mode itself).Honest residuals
supersedes_dod_evidencedoes not affect deploy-gate.has_deploy_evidenceis purely existential (if falsifiable: return True) and never subtracts. Retirement is honored only bycontract_compliance_check.py/lint_contract_check_values.py. The fail-open entries stay in the file (append-only) and remain readable by any consumer that ignores supersession.!-negated pipeline in any contract in this store has the same false-green mode. A lint rule rejecting!-negated pipelines that lack a non-empty assertion is the mechanism; filed separately rather than smuggled into this PR.reportmode org-wide and deploy-gate is not a required context on omnimarketdev— this changes no gate verdict. Evidence integrity only.proof_class: replay-proven (evaluator verdicts recomputed from canonical remote refs) + live-readback (all four probes re-executed against the live GitHub API at three refs).