Repository navigation
fix(OMN-16071): append-only guard rejects in-place writes by git status, not path membership - #2086
Merged
Conversation
… by git status, not just path membership Both OCC companion producers' final pre-push append-only assertion (OccCompanionEmitter._assert_append_only in node_pr_lifecycle_fix_effect, HandlerOccCompanionEffect._assert_append_only in node_occ_companion_effect) only rejected deletions and paths outside the writer's own allowlist. A path already listed as "allowed" by the writer's own plan could still be modified in place (git status M) and pass silently -- exactly the mutate-in-place defect named in the ticket title, and the reason two separate upstream point-fixes (OMN-15485's compute-plan assert_append_only_emissions, OMN-15785's legacy-emitter skip guard) were needed to compensate rather than this last-line-of-defense catching it directly. Both guards now require git status "A" unconditionally for every changed path, matching the hosted OCC Append-Only Gate's own semantics (evaluate_append_only, which flags any M/D/R/C status and requires corrections to be net-new .supersede.<NNNN>.yaml files) rather than trusting the writer's own membership claim. RED-first: added a test to each producer's suite proving a status-M write to a path within the writer's own allowed_paths previously sailed through undetected (DID NOT RAISE); both now raise "append-only violation" after the fix. Existing add/delete/out-of-set/forbidden-surface coverage in test_append_only_guard_omn_14785.py, test_append_only_forbidden_surfaces_omn_14941.py, test_occ_companion_emitter_friction_omn_14741.py::TestF01AppendOnly, and test_occ_append_only_producer_omn_15485.py all remain green (48/48 in the directly affected suites, 531/531 across the three touched node packages). Evidence-Ticket: OMN-16071
jonahgabriel
enabled auto-merge (squash)
August 17, 2026 11:28
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 55 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 112 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. 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 (4)
Comment |
|
| Rule | Description |
|---|---|
| ARCH-TOPIC-001 | No hardcoded Kafka topic strings in handler code |
| ARCH-DI-001 | No event_bus=None bypass |
| ARCH-DI-002 | No direct Handler instantiation outside workflow_runner/adapters |
| ARCH-DI-003 | No reinvented DI containers |
| ARCH-TOPIC-002 | contract.yaml topics follow `onex.{cmd |
ARCH-DI-002: Direct Handler instantiation (warning)\n\n\nsrc/omnimarket/nodes/node_occ_companion_effect/handlers/handler_occ_companion_effect.py:215: self._state_handler = state_handler or HandlerOccStateEffect()\n\n\nHandler classes should be wired only in workflow_runner.py or adapter modules. Direct instantiation elsewhere bypasses the contract system.\n\n
Static architectural lint — no model inference (OMN-14176).
✅ Hostile Reviewer — PASSEDBlocking findings (critical/error): 0 Gate semantics
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
onexbot-occ-writer Bot
pushed a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 17, 2026
jonahgabriel
added a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 17, 2026
node_occ_companion_effect's own pass-2 (self-bind + Evidence-Source stamp) never reached this ticket: its automated retry aborted with an append-only violation on contracts/OMN-16071.yaml -- omnimarket #2086's own hardened _assert_append_only guard correctly refusing a stale-based re-render, caused by heavy same-day concurrent OCC contract activity on this exact ticket (occ#6550, occ#6590 both landed today). That guard did its job: nothing corrupt was pushed. Hand-authoring the missing self-bind entry + receipt here, mirroring the occ-self-bind-pr-6550/-6590 precedent on this same contract exactly, so occ-preflight/eligibility and verify/verify on OCC#6606 do not fail closed with pr_ticket_mismatch. Pure append relative to the current dev tip (verified byte-identical to this branch's own pass-1 commit before appending -- not a repeat of the race): validator_occ_append_only.evaluate_append_only -> ok=True, zero violations. validate_occ_merge_eligibility -> eligible=True. yamlfmt -lint clean (hashes computed against the yamlfmt-stable bytes). Local pre-commit on both changed files: 0 failures. Evidence-Ticket: OMN-16071
jonahgabriel
added a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 17, 2026
…6606) * evidence: OCC companion pass 1 for OmniNode-ai/omnimarket#2086 * evidence(OMN-16071): self-bind OCC#6606 for OmniNode-ai/omnimarket#2086 node_occ_companion_effect's own pass-2 (self-bind + Evidence-Source stamp) never reached this ticket: its automated retry aborted with an append-only violation on contracts/OMN-16071.yaml -- omnimarket #2086's own hardened _assert_append_only guard correctly refusing a stale-based re-render, caused by heavy same-day concurrent OCC contract activity on this exact ticket (occ#6550, occ#6590 both landed today). That guard did its job: nothing corrupt was pushed. Hand-authoring the missing self-bind entry + receipt here, mirroring the occ-self-bind-pr-6550/-6590 precedent on this same contract exactly, so occ-preflight/eligibility and verify/verify on OCC#6606 do not fail closed with pr_ticket_mismatch. Pure append relative to the current dev tip (verified byte-identical to this branch's own pass-1 commit before appending -- not a repeat of the race): validator_occ_append_only.evaluate_append_only -> ok=True, zero violations. validate_occ_merge_eligibility -> eligible=True. yamlfmt -lint clean (hashes computed against the yamlfmt-stable bytes). Local pre-commit on both changed files: 0 failures. Evidence-Ticket: OMN-16071 * fix(OMN-16071): rebind pr-2086 downstream receipt's stale contract_sha256 The automated node_occ_companion_compute pass-1 minted drift/dod_receipts/OMN-16071/dod-OmniNode-ai-omnimarket-pr-2086/command.yaml with a legacy contract_sha256-only binding (no contract_entry_sha256, and evidence_item_id "dod-OmniNode-ai-omnimarket-pr-2086" is not itself declared as a dod_evidence item in contracts/OMN-16071.yaml -- a pre-existing gap in the compute plan's "second product PR on an already-companioned ticket" handling, unrelated to this fix). A legacy-only binding goes stale on every later contract append regardless of which entry changed (docs/standards/receipt_hashing_and_supersession.md section 2) -- and the prior commit on this branch (occ-self-bind-pr-6606) appended a new entry, staling this receipt's whole-file hash. Rebinding it to the current contract hash is a pure edit of a file this same open, unmerged PR (OCC#6606) already introduced -- it does not exist on dev, so it is still a net "A" relative to the merge base regardless of how many times it is edited before merge; not a mutate-in-place violation. scripts/validation/check_receipt_hardening.py run locally against every receipt in this PR's diff: 0 violations (was 1). validate_occ_merge_eligibility -> eligible=True. validator_occ_append_only.evaluate_append_only -> ok=True. Evidence-Ticket: OMN-16071 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: Jonah Gray <jonah@omninode.ai>
jonahgabriel
added a commit
that referenced
this pull request
Aug 23, 2026
…t growth, net-new for reused receipts (#2129) * fix(OMN-16356): OCC append-only guard is content-verified for contract growth, net-new for reused receipts The local pre-push append-only guard in both OCC companion producers (OccCompanionEmitter and HandlerOccCompanionEffect) was hardened by OMN-16071's PR #2086 to reject ANY non-'A' git status unconditionally, including the ticket contract's own doctrinally-sanctioned growth (new dod_evidence rows appended by _ensure_base_dod_evidence / _append_self_bind_evidence). That made the local guard STRICTER than the hosted OCC Append-Only Gate itself (validator_occ_append_only.evaluate_append_only, which is per-entry: a new id is always allowed, only a removed/altered existing id is a violation), and blocked every legitimate second companion for an already-companioned ticket. Live 2026-08-23: omnimarket#2124/OMN-15800, omnibase_infra#2790/OMN-15468, and onex_change_control#6926/OMN-16413 all hit this regression across both producers. A fourth failure (omnibase_infra#2766/OMN-16115) hit a related but distinct case: a re-mint for a PR whose OWN downstream/CI receipts had already merged under a stale/inherited Evidence-Source stamp (OMN-16386). Fixes: - _assert_append_only in both producers now re-verifies a contract-path 'M' against the SAME canonical evaluate_append_only judgment the hosted gate uses, instead of a blanket per-file status check. Receipts remain fully immutable (any non-'A' status still unconditionally rejected). - OccCompanionEmitter's downstream/CI receipt writes now check for a prior merge before writing (mirroring the existing OMN-15785 admissibility- receipt guard), so a stale-Evidence-Source re-mint never reopens its own already-merged receipts for write. Regression tests reproduce all four live failure shapes and are proven RED-vs-EXISTS-but-WRONG against the pre-fix code. Evidence-Ticket: OMN-16356 * fix(OMN-16356): pop PREPUSH_ALLOW_LOCAL_FULL_SUITE before the host-identity guard's refusal test test_guard_refuses_full_suite_escalation_on_non_200_host copies the ambient os.environ wholesale, unlike the file's own _run_hook helper which already pops PREPUSH_ALLOW_LOCAL_FULL_SUITE (and other known-leaky vars) for the identical reason. An ambient PREPUSH_ALLOW_LOCAL_FULL_SUITE=1 -- set by whoever is running this suite, e.g. a developer using the documented DEGRADED-HOST OVERRIDE to push past a loaded-host refusal moments earlier -- routes the hook through the override branch instead of the refusal this test asserts, a false negative discovered live while pushing the OMN-16356 OCC append-only guard fix under a legitimately loaded host. Evidence-Ticket: OMN-16356
jonahgabriel
added a commit
that referenced
this pull request
Aug 29, 2026
…filename shape (#2200) * fix(OMN-16071): the OCC companion writer is add-only by path, not by filename shape Defect 1, the defect this ticket is titled after, is the last open half of OMN-16071 (Defect 2 landed in #2081/#2086). Its AC is literal: "the autobind path must be strictly add-only -- never open an existing receipt file for write." Every prior PR in this ticket's chain hardened the GUARD; none touched the WRITER, which is why the ticket carries a standing DO-NOT-FLIP. WHY THE EXISTING MECHANISM DID NOT CATCH IT. `assert_append_only_emissions` (OMN-15485) is the producer-side invariant meant to make the class unreachable from any emission site. It carried one exemption: if _SUPERSEDE_FILENAME_RE.search(filename): continue That asks "is this filename shaped like a correction?", never "is this path already merged?" -- and filename shape is not evidence of novelty. The merged path files every rebind at `<check_type>.supersede.<pr_number>.yaml`, keyed to the PRODUCT PR number, which is CONSTANT across every authoring pass for that same PR. So the 2nd, 3rd ... pass renders the identical paths and the guard waves each one through while they are already merged bytes on dev. It could not have known better. `ModelOccContractState` carried `existing_entry_ids` -- a set of DIRECTORIES -- and nothing about which FILES are merged, so the blanket exemption was the only way to let a legal first correction through. LIVE, from this ticket's own record: OCC#6616 (companion for omninode_infra#925) re-rendered eight `.supersede.925.yaml` files an earlier pass for that same PR#925 had merged, bumping `created_at` 07:19:22Z -> 17:14:25Z in place, alongside the base receipt. Reproductions #1-#4 in the ticket are the same shape through both producers. WHAT CHANGED * `ModelOccContractState.merged_receipt_paths` gives the pure planner file-level truth. Defaults to `()`, which reproduces the prior semantics byte-for-byte, so nothing in the merged corpus moves until the read-EFFECT populates it. * `assert_append_only_emissions` refuses any emission at a path in that set, BEFORE the directory rule and regardless of filename -- the same judgment `evaluate_append_only` makes off git status. The supersede exemption survives only for paths nothing has merged. * `_next_supersede_path` allocates a fresh generation keyed to the OCC companion PR authoring the rebind when the product-PR-keyed name is taken, and only for the keys that actually collide. The suffix stays a SINGLE integer because both this producer's `_SUPERSEDE_FILENAME_RE` and omnibase_core's `_SUPERSEDE_SUFFIX_RE` parse `.supersede.<NNNN>.yaml` strictly -- a compound `.supersede.925.6616.yaml` would be filed where nothing resolves it. The record's `replacement.pr_number` still names the PRODUCT PR, which is what `resolve_supersession`'s tier-1 selection keys on; the generation is provenance, never the binding. With no OCC PR number yet (pass 1) the emission is OMITTED and logged, not written -- the merged record already binds this same product PR with the same commit_sha / branch / probe, so suppressing the re-mint loses no evidence while opening it for write is the defect. * Born path (`OccCompanionEmitter`): the slot receipt's skip was keyed to `contract_path.is_file()` -- a question about `contracts/<ticket>.yaml` used to decide whether to open a receipt under `drift/dod_receipts/`. Two different files; its siblings in the same loop already ask the direct question. It now also reads the receipt path itself. The contract half is RETAINED deliberately: minting a slot receipt into a pre-existing contract that does not declare that item would trade an append-only violation for an orphan receipt. * The read-EFFECT lists the merged receipt files, scoped to `existing_entry_ids` (1-10 dirs in practice, not the ticket's whole tree). FAILS SOFT to `()`: a listing hiccup degrades to the shipped semantics rather than refusing a mint, and the write-EFFECT's pre-push `_assert_append_only` plus the hosted gate remain fail-closed behind it. PROOF. Two new gates, 13 cases, RED-proven against dev tip b7eef36. The control is stated precisely because a sloppier one would prove nothing: `merged_receipt_paths` is a NEW member of a `extra="forbid"` model, so all 10 compute legs against a plain `git checkout origin/dev -- src/` fail on `extra_forbidden` -- a schema error, which says nothing about the guard. Isolating control, measured: dev `src/` with ONLY `events/occ_companion.py` restored from this commit (the field, none of the logic) -> **6 failed / 4 passed**. The 6 fail behaviourally on the shipped producer: it re-emits the already-merged paths. The 4 controls stay green under that same control (empty `merged_receipt_paths` is a no-op, a first supersede is still legal, a plain receipt in a frozen dir is still refused), so the fix is not passing by refusing everything. The born-path leg needs no such isolation -- it drives `_emit_companion_sync` directly. Against a plain dev `src/`: **1 failed / 2 passed**, the failure being the writer rewriting a merged receipt's bytes, and the 2 passes being controls that hold both directions (contract + receipt present still skips per OMN-15785; a fresh ticket still mints, so the fix cannot pass by never writing and trading the violation for a MISSING_RECEIPT companion). The consumer leg writes both passes' real output to one tree and asks omnibase_core's `resolve_supersession` the question occ-preflight asks. All 24 cases in the three touched gate files pass on this commit. `test_occ_append_only_producer_omn_15485.py`'s falsifier docstring claimed "filename shape is what decides" for the whole invariant; it only ever exercised the directory rule. Corrected in place rather than left to be read at face value -- that reading is how the exemption went unexamined. Re-measured after the rebase onto b7eef36, not carried over: 746 passed / 30 skipped across node_occ_companion_compute + node_pr_lifecycle_fix_effect + node_occ_companion_effect + node_occ_state_effect with OCC_REPO_DIR set. 6178 passed / 38 skipped / 1 xfailed on the full unit suite. mypy --strict clean across 3077 source files, ruff clean, pre-commit run --all-files clean. KNOWN INERTNESS, stated rather than implied: node_occ_companion_compute and the born-path emitter run in the .201 dev-lane effects runtime, not product-repo CI. This code takes live effect at the next dev-lane refresh, so the live-corpus proof for this fix is ARMED, not yet observed. Closes OMN-16116 as a duplicate of this defect. * fix(OMN-16071): the supersede-skip log names which of its two causes fired CodeRabbit, correctly, on omnimarket#2200: `_next_supersede_path` returns `None` for two distinct reasons and the log asserted the first one unconditionally. * Pass 1 has no `occ_pr_number` to key a fresh generation with, and pass 2 of the same mint WILL file it. * A key whose product-PR-keyed AND OCC-PR-keyed generations are both already merged has nothing left to add, and no later pass will ever emit it. The operator responses differ. Claiming "no OCC PR number is available yet" in the second case sends a reader hunting for a number that is set, and promises a pass that will never come. RED-proven, not assumed: the new case fails against the shipped log text, and its captured output is the false claim itself -- skipping supersession for OMN-15922/dod-deploy-assessment - its product-PR-keyed path is already merged and no OCC PR number is available yet ... (emitted with occ_pr_number=6616 SET) Both directions are pinned: the exhausted-generations branch must say "ALREADY merged" / "no later pass will emit it" and must NOT say "no OCC PR number is available yet"; the pass-1 branch must say the inverse. A one-directional assertion would pass on a log that always said both. 747 passed / 30 skipped across the four affected node suites. ruff, mypy --strict (3077 files) clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect (OMN-16071 defect 1)
Both OCC companion producers' final pre-push append-only guard
(
OccCompanionEmitter._assert_append_onlyinnode_pr_lifecycle_fix_effect,HandlerOccCompanionEffect._assert_append_onlyinnode_occ_companion_effect)only rejected deletions and paths outside the writer's own allowlist. A
path the writer's own plan already lists as "allowed" could still be
modified in place (git status
M) and pass this guard silently — that isexactly the mutate-in-place defect named in the ticket title, and observed
live on the autobind companion for
OmniNode-ai/omninode_infra#906, whichrewrote
drift/dod_receipts/OMN-16029/dod-occ-evidence-admissibility-validator/command.yaml,replacing PR #900's landed evidence with #906's.
This is why two separate upstream point-fixes were needed to compensate
rather than the last-line-of-defense assertion catching it directly:
OMN-15485's
assert_append_only_emissions(a compute-plan-level guard innode_occ_companion_compute) and OMN-15785's ticket-scoped skip guard inthe legacy emitter. Neither of those changed what the actual
git diffassertion checks — membership in
allowed_paths, never the git statusletter.
The fix
Both
_assert_append_onlymethods now require git statusA(added)unconditionally for every changed path, in addition to the existing
membership/forbidden-surface/deletion checks. This matches the hosted
OCC Append-Only Gate's own semantics
(
omnibase_core.validation.validator_occ_append_only.evaluate_append_only,which flags any
M/D/R/Cstatus and requires corrections to benet-new
.supersede.<NNNN>.yamlfiles) directly against the git statusletter, rather than trusting the writer's own claim that a path is
"allowed."
Verification
RED-first: added a test to each producer's existing append-only suite
proving a status-
Mwrite to a path within the writer's ownallowed_pathspreviously sailed through undetected (DID NOT RAISE).Confirmed both new tests failed against the pre-fix code before making the
change.
GREEN after fix: both new tests now raise
append-only violation; allpre-existing coverage remains green — the add/delete/rename/out-of-set/
forbidden-surface cases in
test_append_only_guard_omn_14785.py,test_append_only_forbidden_surfaces_omn_14941.py,test_occ_companion_emitter_friction_omn_14741.py::TestF01AppendOnly, andthe compute-plan-level
test_occ_append_only_producer_omn_15485.py(whichexercises the sibling OMN-15485 mechanism this PR complements, not
duplicates) all still pass.
Local gates:
ruff format --checkclean,ruff checkclean,mypy --strictclean on both changed source files,
pre-commit runclean (0 failures) onthe four changed files, full local pytest suite (pre-push governed selector
escalated) 7619 passed / 22 skipped / 1 xfailed / 0 failed.
Scope
This closes defect 1 as scoped by the ticket: the mutate-in-place receipt
writer. It does not touch defect 2 (already fixed by #2081) and does not
touch the related-but-distinct autobind in-place re-mint anti-pattern
family on the OCC side (a separate ticket) — this PR is confined to the two
producer modules living in this repo.
Evidence-Ticket: OMN-16071
Evidence-Source: OCC#6606