Skip to content

fix(OMN-18415): route the lab-touching Hostile Reviewer to the fleet and stop discarding its stderr - #2187

Merged
jonahgabriel merged 6 commits into
devfrom
jonah/omn-18415-hostile-reviewer-to-the-fleet
Sep 16, 2026
Merged

jonahgabriel merged 6 commits into
devfrom
jonah/omn-18415-hostile-reviewer-to-the-fleet

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

OMN-18415

The Hostile Reviewer calls a LAN-only model on the lab host. Its job was selected by a variable that resolves to a GitHub-hosted runner for this repository, so on every real pull request it landed somewhere the model is unreachable and reported infra_error on a required check. It also discarded its own stderr, so the cause was never in CI.

The placement, and why the routing node could not do it

The fleet side of OMN-18415 is already done: the local-LLM signing key is on 60 of 60 runners and the endpoint CIDR boundary on 59 of 60. None of it helps a job that runs somewhere else. Run 35043714512 attempt 4 of #2181 executed on a hosted ubuntu-24.04 image.

Two candidate mechanisms were checked before a third was used.

  • The cross-repo routing shim exists (route-runner-reusable.yml, feat(OMN-18423): a caller outside omnibase_infra can reach the routing node #2184, pinned to omnibase_infra 341ac00c) and cannot express this. node_ci_runner_route_compute treats the caller's trusted seam as a CEILING it may only DOWNGRADE from — step S2 returns seam_ceiling_hosted verbatim when the seam names no fleet label. This repository is PUBLIC and its seam is ["ubuntu-latest"] at org and repo scope, so the node returns hosted on every run. It also has no lab-touching classification; its lab model is host LOAD, not what a job reaches.
  • A bare fleet literal placed the job correctly but answers to no routing variable, and the hourly runner-routing audit reads Actions variables — so a label written into a workflow file is invisible to it. That is the blind spot rule 14 names.

So: a narrower per-job variable, ahead of the seam, with the fleet labels as the operating fallback. Identical in shape to the carve-out the OCC publishers use for the tailnet-only broker, whose own guard tests sit beside the new ones.

|| fromJSON(vars.OMNI_HOSTILE_REVIEW_RUNS_ON_JSON || '["self-hosted","omnibase-ci"]')

No Actions variable was written, and none needs to be. OMNI_HOSTILE_REVIEW_RUNS_ON_JSON is unset at every scope and stays that way: the literal is the operating value, and the variable exists only as an operator kill switch for the day another runner class can reach the endpoint. Nothing in omnibase_infra/config/runner_routing_policy.yaml changes, because no expected_json moved — the seam is untouched. The single-owner rule is unaffected.

The selector also loses the base.ref == 'dev' term, which is what actually sent every same-repo pull request down the fork-isolation branch. Fork ORIGIN is the trust question; the base branch is not. That term was already removed on dev by #2181; this keeps it gone and pins it with a test.

Fork-origin pull requests

They stay hosted — untrusted code never reaches LAN-attached runners at any idle level, and the fork branch is still the first term of the selector so no classification argument can be applied before it. The consequence is that the model is unreachable there by construction, so the job now refuses with a named verdict, unavailable_fork_isolation, written into the job log and the verdict artifact. A typed skip was considered and rejected: the gate already treats skipped as the absence of a verdict and fails closed on it, so skipping would produce a red check with no reason attached. This is a red check with one.

AC3 — the stderr

The redirect to the null device is gone. Per attempt, stderr is captured, redacted, echoed into the job log as its own group, and appended to a capture that is uploaded as hostile-review-verdict-<run>-<attempt>. The most informative line is lifted into a named_cause step output so it reaches the PR comment too. The artifact is written on the success path as well, because degraded is a success exit in the pilot and its cause exists only in stderr.

Redaction runs before anything is echoed or uploaded, in python reading os.environ, so no value is ever placed on a command line where ps would show it. GitHub masks registered Actions secrets, not runner environment variables, and the fleet declares this one in the ambient environment.

Red-first

The 13 new guards were run against dev at 1aaaa9fbd before the change: 10 failed, 3 passed. The AC3 falsifier failed with exactly the output the ticket describes — the whole job log was two lines of ::warning::cli_review attempt N/2 failed (exit 1) and no cause anywhere.

Four of them execute the workflow's own review script against a stubbed reviewer rather than asserting on its text, so a rewrite that keeps the words and loses the behaviour fails.

Verification

check result
new guards, after 13 passed
reviewer + carve-out suites together 45 passed
shell syntax of every run block clean
ruff format, ruff check, mypy clean
governed pre-push passed, push completed

Two assertions in the pre-existing tests/hooks/test_hostile_reviewer_workflow.py pinned the bare literal and were updated to the variable-with-fleet-fallback form. Their intent is preserved: the trusted branch still resolves to the fleet, and the shared seam is still refused.

The OMN-18409 exit-code contract from #2181 is carried through unchanged — exit 0 and exit 2 both mean a real verdict, only exit 1 is retried and then fails closed.

Evidence-Ticket: OMN-18415
Evidence-Source: OCC#9788

… says why when it cannot

The Hostile Reviewer calls a LAN-only model on the lab host. Its job was
selected by a variable that resolves to a GitHub-hosted runner for this
repository, so on every real pull request it landed somewhere the model is
unreachable and reported infra_error on a required check. It also discarded
its own stderr, so the cause was never in CI.

- the job now selects its runner from a dedicated per-job variable with the
  fleet labels as the operating fallback, ahead of the shared seam, with fork
  isolation still first
- fork-origin pull requests stay hosted and now refuse with a named verdict
  instead of failing anonymously
- the reviewer's stderr is captured, redacted, echoed per attempt, and
  uploaded as a verdict artifact
@jonahgabriel
jonahgabriel enabled auto-merge (squash) September 16, 2026 05:53
jonahgabriel pushed a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 16, 2026
…iclaude#2187 (#9788)

* evidence(OMN-18415): author OCC companion for OmniNode-ai/omniclaude#2187

OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 406d54a02d5ad21df7214db51c9c8dac5dd00eaf.

* evidence(OMN-18415): self-bind OCC#9788 + rebind contract_sha256

---------

Co-authored-by: omnimarket-bot <bot@omninode.ai>
@github-actions

github-actions Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

✅ Hostile Reviewer — PASSED

Blocking findings (critical/error): 0
Total findings: 0
Models succeeded: deepseek-r1

Reviewer stderr (named cause): WARNING: DEGRADED review — only 1 model(s) succeeded. A minimum of 2 models is required for a full pass.


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)

…that blocked it

The gate's first live run on a real diff blocked with a finding count of 1
and nothing anywhere naming the finding. The reviewer's JSON went to a shell
variable, was parsed for counts, and was discarded. A required gate whose
refusal cannot be read is not reviewable, only obstructive.

Findings now reach the verdict artifact, a job-log group, and the PR comment.
The comment reads them from the artifact rather than a step output, because a
finding is multi-line free text from a model and any delimiter chosen for
GITHUB_OUTPUT is one the model could emit.
…he one this code guessed

The first revision read title/message/file. The reviewer emits rule_id,
file_path, line_start and normalized_message, so the live run rendered a row
of three empty cells, which reads as a reviewer with nothing to say rather
than as this code reading the wrong keys.

Each value now falls back through its alternatives and the finding is kept
verbatim alongside, so a rename upstream costs a worse-looking row instead of
a lost finding. A test covers a finding with no mapped key at all.
Raised by this gate's own adversarial reviewer on this pull request, which is
the first finding it has produced since it started reaching its model.

A plain substring replace only catches the value spelled verbatim. Anything
that passes through a JSON body, a query string or an encoded header reaches
stderr in a form that does not match, and was printed in full. Each
transformation the value can plausibly survive now gets its own pass, longest
first so a shorter variant cannot cut a longer one in half.

Over-redaction is the accepted direction: a value that happens to appear
inside an unrelated token leaves a marked-up log line, which is legible. The
other direction leaves a signing value in CI output.
@jonahgabriel
jonahgabriel merged commit 7c8d477 into dev Sep 16, 2026
145 checks passed
@jonahgabriel
jonahgabriel deleted the jonah/omn-18415-hostile-reviewer-to-the-fleet branch September 16, 2026 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant