From b9b705eda9218e94c4b94d047c64f18b4542853c Mon Sep 17 00:00:00 2001 From: Jonah Gray Date: Mon, 17 Aug 2026 07:08:59 -0400 Subject: [PATCH] fix(OMN-16071): make the OCC append-only guard reject in-place writes 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..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 --- .../handlers/handler_occ_companion_effect.py | 27 +++++++++++++++++++ .../handlers/occ_companion_emitter.py | 22 +++++++++++++++ .../test_append_only_guard_omn_14785.py | 25 +++++++++++++++++ ...cc_companion_emitter_friction_omn_14741.py | 13 +++++++++ 4 files changed, 87 insertions(+) diff --git a/src/omnimarket/nodes/node_occ_companion_effect/handlers/handler_occ_companion_effect.py b/src/omnimarket/nodes/node_occ_companion_effect/handlers/handler_occ_companion_effect.py index de69e4cf67..09e9b790b4 100644 --- a/src/omnimarket/nodes/node_occ_companion_effect/handlers/handler_occ_companion_effect.py +++ b/src/omnimarket/nodes/node_occ_companion_effect/handlers/handler_occ_companion_effect.py @@ -514,6 +514,24 @@ def _assert_append_only( (the OMN-13418 prod-promotion gate's source of truth) or an allowlist would otherwise sail through the membership check. Plan membership must never be able to authorize a write outside the evidence surfaces. + + (d) OMN-16071: membership in ``allowed_paths`` is not, by itself, + proof that a path is genuinely new at ``base_sha`` — it only proves + the compute plan *intended* to write that path this run. A shared, + ticket-scoped evidence id whose directory a PRIOR companion already + merged renders to the SAME deterministic path every time + (``compute_companion_plan`` is a pure function of ticket + evidence + id), so it is, by construction, always present in this run's own + allowed set too — meaning a status-``M`` write there previously + passed this guard silently despite ``assert_append_only_emissions`` + (OMN-15485) existing precisely to prevent the compute plan from ever + emitting such a file. This is the belt to that mechanism's suspenders: + even if a future/legacy caller's plan-level guard has a gap, this + final pre-push assertion now independently refuses any status other + than ``A`` (added), matching the hosted OCC Append-Only Gate's own + semantics (which flags any M/D/R/C status and requires corrections + to be net-new ``.supersede..yaml`` files) directly against the + git status letter rather than trusting path membership alone. """ diff = run_git( ["git", "diff", "--no-renames", "--name-status", base_sha, "HEAD"], @@ -542,6 +560,15 @@ def _assert_append_only( f"{status} {path} (outside the contracts//drift/ evidence " "roots — never machine-mintable, OMN-14941)" ) + elif not status.startswith("A"): + # OMN-16071: unconditional — a path in allowed_paths is only + # a claim of intent, never proof the path is new. + violations.append( + f"{status} {path} (not a net-new add — a receipt or " + "contract may never be opened for write once it exists " + "at the clone base; express a genuine change as a " + "net-new .supersede..yaml file, OMN-16071)" + ) elif path not in allowed_paths: violations.append(f"{status} {path}") if violations: diff --git a/src/omnimarket/nodes/node_pr_lifecycle_fix_effect/handlers/occ_companion_emitter.py b/src/omnimarket/nodes/node_pr_lifecycle_fix_effect/handlers/occ_companion_emitter.py index 6a6fc8232b..fa0b236f6e 100644 --- a/src/omnimarket/nodes/node_pr_lifecycle_fix_effect/handlers/occ_companion_emitter.py +++ b/src/omnimarket/nodes/node_pr_lifecycle_fix_effect/handlers/occ_companion_emitter.py @@ -1860,6 +1860,21 @@ def _assert_append_only( assertion-only "all-adds" comment the force-push previously trusted — the exact gap that let generated companions mutate already-merged receipts (OCC#4293/4295/4296). + + OMN-16071: membership in ``allowed_paths`` alone is NOT sufficient — + it only proves this run's writer *intended* to touch a path, not that + the path is genuinely new at ``base_sha``. A shared, ticket-scoped + evidence id whose directory a PRIOR companion already merged is, by + construction, always present in THIS run's own allowed set too (the + writer renders the same deterministic path every time), so a status- + ``M`` change there previously sailed through this guard silently — + the mutate-in-place defect named in the ticket title. The git status + letter is now checked directly, matching 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/C status and requires corrections to be net-new + ``.supersede..yaml`` files): only ``A`` (added) ever passes, + unconditionally, regardless of whether the path is nominally allowed. """ diff = self._run_git( ["git", "diff", "--name-status", base_sha, "HEAD"], cwd=str(clone_dir) @@ -1874,6 +1889,13 @@ def _assert_append_only( path = parts[-1] # rename → dest path is last field if status.startswith("D"): violations.append(f"deletes {path}") + elif not status.startswith("A"): + violations.append( + f"{status} {path} (not a net-new add — a receipt or " + "contract may never be opened for write once it exists " + "at the clone base; express a genuine change as a " + "net-new .supersede..yaml file, OMN-16071)" + ) elif path not in allowed_paths: violations.append(f"{status} {path}") if violations: diff --git a/tests/unit/nodes/node_occ_companion_effect/test_append_only_guard_omn_14785.py b/tests/unit/nodes/node_occ_companion_effect/test_append_only_guard_omn_14785.py index 7ffee57209..11d92ac980 100644 --- a/tests/unit/nodes/node_occ_companion_effect/test_append_only_guard_omn_14785.py +++ b/tests/unit/nodes/node_occ_companion_effect/test_append_only_guard_omn_14785.py @@ -121,3 +121,28 @@ def test_modifying_a_merged_receipt_is_rejected(self, tmp_path: Path) -> None: HandlerOccCompanionEffect()._assert_append_only( str(tmp_path), base, {"contracts/OMN-9999.yaml"} ) + + def test_modifying_a_merged_receipt_the_writer_itself_allowlists_is_rejected( + self, tmp_path: Path + ) -> None: + """OMN-16071: membership in ``allowed_paths`` is not proof a path is + genuinely new at ``base_sha``. A shared, ticket-scoped evidence id a + PRIOR companion already merged occupies a path this run's OWN compute + plan may legitimately list as something it intends to touch (e.g. it + renders the same deterministic path for every companion on that + ticket) -- so the guard must reject a status-``M`` change to that path + even when the writer's own plan calls it "allowed". Before the fix, + checking only ``path not in allowed_paths`` let this sail through + silently: the OCC Append-Only Gate is the only thing that ultimately + caught the mutation (OMN-16071's title incident), because this local + pre-push guard never independently verified the git status letter. + """ + base = _init_repo_with_merged_receipt(tmp_path) + merged_rel = "drift/dod_receipts/OMN-1/dod-001/command.yaml" + (tmp_path / merged_rel).write_text("status: FAIL\n", encoding="utf-8") + _git(tmp_path, "add", "-A") + _git(tmp_path, "commit", "-q", "-m", "mutate merged receipt in place") + with pytest.raises(RuntimeError, match="append-only violation"): + HandlerOccCompanionEffect()._assert_append_only( + str(tmp_path), base, {merged_rel} + ) diff --git a/tests/unit/nodes/node_pr_lifecycle_fix_effect/test_occ_companion_emitter_friction_omn_14741.py b/tests/unit/nodes/node_pr_lifecycle_fix_effect/test_occ_companion_emitter_friction_omn_14741.py index 6ef1b918a5..6b93228310 100644 --- a/tests/unit/nodes/node_pr_lifecycle_fix_effect/test_occ_companion_emitter_friction_omn_14741.py +++ b/tests/unit/nodes/node_pr_lifecycle_fix_effect/test_occ_companion_emitter_friction_omn_14741.py @@ -631,6 +631,19 @@ def test_append_only_guard_accepts_in_set_changes(self) -> None: f"A\tdrift/dod_receipts/OMN-9999/{self._EID}/command.yaml" ) + def test_append_only_guard_rejects_in_set_modify(self) -> None: + """OMN-16071: a status-``M`` change to a path THIS RUN'S OWN + ``_allowed_paths`` call lists must still be rejected. Membership in + the writer's own allowed set only proves it intended to touch that + path this run -- it says nothing about whether the path already + existed (and was already merged) at ``base_sha``. The exact incident + this proves: a shared, ticket-scoped evidence id whose path a prior + companion already merged is (by construction) always in THIS run's + own allowed set too, so the guard must not trust membership alone. + """ + with pytest.raises(RuntimeError, match="append-only violation"): + self._guard(f"M\tdrift/dod_receipts/OMN-9999/{self._EID}/command.yaml") + # --------------------------------------------------------------------------- # F-06 — runtime probe is the GraphQL `gh pr view --json files` (OMN-14766)