Repository navigation
feat(OMN-18970): default-deny the CI Summary umbrella's external check-runs - #2290
Merged
jonahgabriel merged 4 commits intoSep 21, 2026
Merged
Conversation
…k-runs Third repository of OMN-18943, after omnibase_infra OMN-18960 (merged) and omnibase_core OMN-18965. Layer 4 is a whitelist lookup over two expected sets: it walks the expected names and asks the head for each, and never walks the head's check-run list the other way. A check-run whose name is in neither set is read by nothing and can conclude a refusal unseen. Measured over the 16 dev pull requests merged 2026-09-19T12:32:51Z to 2026-09-21T04:14:24Z, scoped to rows started at or before each merge decision: 41 to 46 unregistered external check-run names per head, 53 distinct across the window, against an expected tuple of 14. Event attribution: 680 pull_request and 22 rows written by a GitHub App rather than Actions, with no push, schedule or review-event rows. This is the repository where the number was not a zero. One of the 16 heads, pull request 2279, merged with the autobind outcome context concluded failure at merge time. That failure is a transient error from the version-control step inside the companion-authoring effect, not a verdict about the pull request. Across the window that context is neutral 15 times and failure once, and never success. So this lands with the first real entry in the exclusion registry, rather than an empty one. The entry carries the measured rate, the ticket that owns the producer, the date and an absolute expiry 45 days out. A test proves it is load-bearing by replaying that real head both ways: without the entry the head fails and names the context, with it the head passes and the report names the exclusion applied. Admitting a roughly 6 percent automation flake into a merge-blocking position would convert it into a merge outage, and the root cause is known and is not the pull request. skipped and neutral are not failures in this layer: eight of the 53 names are never green in the window by design. Resolution goes through the same state and effective-row helpers layer 4 uses, so the two layers cannot disagree about which row is current for a name, which is the one way a red could be judged by neither. The check-run state gains the producer URL, read only to resolve which event wrote the row. Event scoping is a deny list of non-pull-request events rather than an allow list of pull-request ones, so a row written by a GitHub App, a row whose run is absent from the index, and a row with no resolvable URL are all swept. An allow list would have exempted all 22 App rows for free. Wiring is pinned, because this module's own entry point carries the record of a port that changed a sibling's gate and not its poller and shipped completely inert with every test green. Four assertions read the poll step out of ci.yml and the entry point's own behaviour: the workflow-runs fetch, both new flags, the stale-file cleanup, the event-name declaration, and that a forgotten event name enforces rather than skipping. Proven end to end on real heads through the command-line entry point: a clean head reports 45 unregistered contexts judged and passes, the head that merged red reports the same 45 with the exclusion named, and the same payload under a push event is silent.
jonahgabriel
enabled auto-merge (squash)
September 21, 2026 05:01
jonahgabriel
pushed a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Sep 21, 2026
…iclaude#2290 (#10633) * evidence(OMN-18970): author OCC companion for OmniNode-ai/omniclaude#2290 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 85e1b72845e5127176a23d10a4326e7a66d653f1. * evidence(OMN-18970): self-bind OCC#10633 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
Contributor
✅ Hostile Reviewer — PASSEDBlocking findings (critical/error): 0 Findings
Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
…e the eight Operator ruling, 2026-09-21, firm, and it changes this layer's design rather than tuning it. The swept population now takes the same bar layer 4 holds its own names to: only success passes, and skipped, neutral, cancelled and stale all fail. The reasoning is the part worth keeping. The first revision failed only on a refusal conclusion and shipped a registry with one entry, on the measured argument that eight of the fifty-three names are never green by design and failing them would wedge every pull request here. That argument was right about the wedge and wrong about the remedy: a weaker default for unregistered names sitting beside a near-empty registry is a hidden allowlist, which is the shape this ticket exists to remove. The honest form of the same decision is a populated registry where every tolerance is written down with a reason, an owner, a date and an absolute expiry, so a person re-reads it. So the eight names are now eight entries, one each, covering every name the measurement found non-green on any head: the docs-validation caller and the imperative-contract guard, which skip when their change-control preflight dependency does not succeed; the two manual re-publish entrypoints, gated to the manual-dispatch event and so always skipped on a pull request; the outstanding-marker audit job, which is post-merge; and the three App-written status and outcome rows, which are neutral placeholders. Every entry expires 2026-12-20, ninety days out, including the ones that look structural, because the cap is what forces a premise that has held for a quarter to be re-read. The bar changed and the grace did not. A cancelled or skipped row still goes through the existing re-run window before it can fail, so a producer that is demonstrably about to re-run is pending rather than refused on the poll that sees it. A test pins each half. One key is assembled from parts rather than written as a literal. The outstanding-marker audit context's display name would trip this repository's bare-marker pre-commit hook, which reads one in source as an untracked marker even inside a string. Assembly keeps the runtime key byte-exact, and rather than comparing it to a second copy of the literal, a test asserts it against the check-run names GitHub actually published on the captured heads. New falsification control, generalised from the single-entry one: for each real head, stripping the registry must make the sweep fail. A clean result with the registry in place could otherwise mean the sweep found nothing rather than that the entries did their work. Proven end to end on real heads through the command-line entry point: 39 unregistered contexts judged with 7 exclusions named in the report, a SUCCESS verdict, and silence under a push event. 126 tests in the umbrella's own file, lint, format and strict type checking clean over the module and over the block this change adds to the test file.
The adversarial gate blocked with 1 qualifying and 16 total findings. Two are real and are fixed here. The rest are answered with evidence below rather than with a change, because acting on a false positive to clear a gate is how a gate stops meaning anything. REAL, one. The deny list of non-pull-request events omitted the queue event. It cannot fire today, since the sweep runs on pull requests only, and the deny list is where a reader looks to learn which events are not pull-request verdicts, so an omission there is a trap for the next person. Added, with a test. REAL, two. Two models called the fetch-failure path untested. The loader returning nothing was already pinned; what the sweep then DOES with an absent index was not, and that is the half that matters. A test now asserts both directions on the same row: attributed to a schedule it is left alone, and with the index gone it is judged. BOUNDED, on the merit of the finding rather than its framing. Two models raised unbounded pagination. The index is now capped at 500 runs, and the cap cannot change a verdict in the permissive direction: a check-run whose producing run falls past it resolves to an unknown event, and an unknown event is swept. Measured 21 to 65 runs per head across four repositories, so the cap is an order of magnitude of headroom and exists only to bound what the slurp can be made to allocate on a re-run-heavy commit. NOT A DEFECT, and the evidence, since the blocking finding is here. The repository slug is said to enable injection. It comes from the runner's own repository context, and it is used double-quoted in an argv position rather than interpolated into a shell string, so it can neither word-split nor inject. The two fetches immediately above the new one have used that exact form in production for months. A shape assertion is added anyway, once, before the poll loop, because it costs one comparison and makes a malformed value fail loudly here instead of reaching the API. That is defence in depth, not a repair. Also declined, with reasons. The producer URL is read from both url fields already, so the claim that one is missed is not the code's behaviour. The fixture is large because it is real unfiltered API output, which is the whole point of it under this ticket's acceptance criteria, and the repository already carries fixtures of that shape. The merge-time filter compares ISO-8601 instants that sort lexicographically, which is the same rule the module's own resolver uses.
Second pass on the adversarial gate. It accepted the shape guard added in the
previous commit and then objected, from both models, that a glob is not
strict enough: it admits a doubled slash, and it does not exclude shell
metacharacters.
That is a fair reading of the glob, so the guard is now a character class:
^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$
After it, the value provably contains nothing but two non-empty runs of the
allowed characters separated by exactly one slash. A metacharacter cannot be
present at all, so the second finding's question of escaping does not arise
rather than being argued about. The glob it replaces would have admitted
owner//repo, which the first finding was right about.
The pattern is now exercised rather than only asserted present: ten cases
covering a real slug, a doubled slash, a third path segment, a bare owner,
the empty string, and three injection shapes.
Recorded for completeness, because it matters for how the next reader treats
this: the value still comes from the runner's own repository context and is
still used double-quoted in an argv position, so nothing here was reachable
in practice. The guard is defence in depth that now costs a single grep.
jonahgabriel
deleted the
jonah/omn-18970-umbrella-external-default-deny
branch
September 21, 2026 08:39
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.
Ticket: OMN-18970 (parent OMN-18943, epic OMN-18527; gate: C27).
41 to 46 check-runs per head sit on every pull request here that the required gate never looks at, and one of them merged red. Third repository of the OMN-18943 landing order.
The defect
Layer 4 is a whitelist lookup over two expected sets: it walks the expected names and asks the head for each. It never walks the head's check-run list the other way, so a check-run whose name is in neither set is read by nothing and can conclude a refusal with the umbrella green.
Measured on the real tree
16 dev pull requests merged 2026-09-19T12:32:51Z to 2026-09-21T04:14:24Z, scoped to check-runs that had started at or before each merge decision.
Event attribution of the swept population: 680
pull_requestand 22 rows written by a GitHub App rather than Actions. No push, schedule or review-event rows.Pull request 2279 merged on 2026-09-19 with the autobind outcome context concluded
failureat merge time. Its own summary names the cause as a transient error from the version-control step inside the companion-authoring effect, not a verdict about the pull request.The bar is strict, by operator ruling
Only
successpasses.skipped,neutral,cancelledandstaleall fail.This revision of the pull request changed that. The first revision failed only on a refusal conclusion and shipped a registry with one entry, on the measured argument that eight of the fifty-three names are never green by design and failing them would wedge every pull request here. The operator ruled that pairing a weaker default with a near-empty registry is a hidden allowlist, which is the shape this epic exists to remove. The argument was right about the wedge and wrong about the remedy: the honest form is the strict bar plus a populated registry, where every tolerance is written down with a reason, an owner, a date and an expiry, so a person re-reads it.
The eight
Every name the measurement found non-green on any head now has an entry, all expiring 2026-12-20:
The cap applies even to the entries that look structural, deliberately: it is what forces a premise that has held for a quarter to be re-read.
The bar changed; the grace did not
A
cancelledorskippedrow still passes through the existing re-run window before it can fail, so a producer demonstrably about to re-run is pending rather than refused on the poll that sees it. Tests pin both halves.One key is assembled from parts
The outstanding-marker audit context's display name would trip this repository's bare-marker pre-commit hook, which reads one in source as an untracked marker even inside a string. Assembly keeps the runtime key byte-exact. Rather than comparing it to a second copy of the literal, which would defeat the purpose, a test asserts it against the check-run names GitHub actually published on the captured heads.
The mechanism, and where it sits
A fifth layer under the same required context. It takes the head's check-runs, subtracts this run's own job names, both expected sets, the poller's own context and rows attributable to a non-pull-request event, and fails on any remainder that did not succeed.
Resolution goes through the same state and effective-row helpers layer 4 uses, so the two layers cannot disagree about which row is current for a name. A disagreement is the one way a red could be judged by neither. The check-run state gains the producer URL, read only to resolve which event wrote the row.
Event scoping is a deny list, not an allow list, so a row written by a GitHub App, a row whose run is absent from the index, and a row with no resolvable URL are all swept. An allow list would exempt all 22 App rows for free.
Wiring, because a module-only change would ship inert
This module's entry point carries the record of a port that changed a sibling's gate and not its poller and shipped completely inert with every test green. Layer 5 has that shape and a worse one, since it is off for any event name but
pull_request. Four assertions read the poll step out of the workflow file and the entry point's own behaviour: the workflow-runs fetch, both new flags, the stale-file cleanup, the event-name declaration, and that a forgotten event name enforces rather than skipping.Falsification controls
Two, and both are real rather than decorative. The single-entry one replays the head that merged red both ways: without the entry the sweep fails and names the context, with it the head passes and the report names the exclusion. The generalised one asserts that stripping the whole registry makes every real captured head fail, so a clean result cannot mean the sweep found nothing.
Evidence
Tests. 126 in the umbrella's own file; 325 across every test file importing the module. Format, lint and strict type checking clean over the module and over the block added to the test file.
End to end through the command-line entry point, on real heads:
Precedent. The same layer merged in omnibase_infra as pull request 3902, whose live readback on its own run reads
external default-deny sweep: 42 unregistered context(s) judgedwith a SUCCESS verdict. That repository is being brought to this same strict bar under OMN-18979.Version and branch protection
No version bump: this repository has no release-identity gate and nothing under a packaged source tree is touched. No branch-protection change requested; this strengthens what an existing required context means and adds no context.
Residual, stated
A swept row still running does not hold the verdict at pending. Every other layer's pending is backed by a presence promise and an unregistered row carries none, so waiting on one would let the poller's deadline be spent on a job that never terminalizes. In-flight rows are named in the report instead, and the remedy for a name that matters is to register it.
Evidence-Ticket: OMN-18970
Evidence-Source: OCC#10633