Repository navigation
fix(OMN-17985): the health monitor's ownership filter reads one validated name - #3266
Conversation
…ated name
OMN-17985 consolidated every `RUNTIME_PROFILE` read that decides CONTRACT
OWNERSHIP onto `resolve_runtime_profile_name()`, so an unregistered role name
is refused instead of quietly emptying the manifest. That consolidation was
reported complete. It was not: two raw reads still fed
`filter_manifest_for_runtime_profile` directly --
`service_runtime_health_monitor.py:92` `os.getenv("RUNTIME_PROFILE", "main")`
in `_discover_contracts`, and `:117` `os.environ.get("RUNTIME_PROFILE",
"main")` in `_filter_manifest_for_runtime_profile`.
Both spelled the same `"main"` default, so the practical exposure was small --
a booted runtime cannot reach them with an unknown value because
`load_runtime_profile()` refuses first at kernel boot. The claim "a single
validated read for ownership decisions" was nonetheless untrue as stated, and
this module is the surface that reports whether the fleet's subscriptions
exist: an unregistered name reaching it is filtered against a list no contract
declares, so every contract is skipped and the monitor concludes the runtime
owns nothing -- reporting that as a finding about the FLEET rather than as the
misconfiguration it is.
Consolidating adds the registry check; it does not move the default. Unset and
blank still resolve to `"main"`.
Also removes a stale comment the same change left behind:
`service_kernel.py:1303-1304` still read `unknown values fall back to
"default" ... with a structured warning` immediately above the
`load_runtime_profile()` call whose behaviour OMN-17985 replaced with a hard
raise. A comment documenting a removed defect as current behaviour is what the
next reader acts on.
RED: 4 of 6 assertions fail on dev @ eee4971 (both refusal tests, the
source-level raw-read assertion, and the stale-comment assertion). GREEN: 6
passed; 62 passed across the new module plus both runtime-profile suites; 98
passed across every dependent health-monitor and projection-liveness test.
POSITIVE CONTROL included: a registered profile ("effects") and an unset one
both still flow through, so the refusal is not "refuses everything".
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 6
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[MAJOR] hostile-reviewer (glm-review)
Default profile inconsistency between kernel and monitor left unexamined | The service_kernel comment says unset/blank resolves to 'default' while the monitor code claims unset/blank resolves to 'main'. If resolve_runtime_profile_name() truly uses 'main' as its fallback while the bootstrap kernel path uses 'default' (prefetch_policy='disabled'), then consolidation has silently coupled two ownership decisions to different profile identities. A process bootstrapping under 'default' will have its contracts fil
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
Fail-fast refusal introduced in monitor read path without caller handling | _discover_contracts and _filter_manifest_for_runtime_profile now raise ProtocolConfigurationError on unregistered names. The diff shows no corresponding change to the callers of these functions within the health monitor or its scheduling loop. A misconfigured environment no longer produces a wrong fleet finding, it now raises inside whatever calls _discover_contracts; if that call site is a periodic monitor tick, the process may cra
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Source-text scanning tests are brittle and enforce the wrong invariant | test_no_raw_runtime_profile_read_remains_in_this_module greps the module source for literal read patterns. It fails to catch equivalent raw reads: os.environ[...] with single quotes is covered but os.environ.keys-based access, a local alias of os.getenv, a helper in a sibling module, or f-string keys all pass. Conversely it breaks on harmless occurrences in comments or docstrings. The behavioral tests already cover the contract; the so
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
test_discover_contracts_refuses_an_unregistered_profile depends on real contract discovery side effects | _discover_contracts calls discover_contracts() before the profile resolution raises, meaning the test executes the real contract discovery scan against the repo. This couples a unit test to the filesystem and to whatever contracts exist, slows the suite, and can fail or pass for environmental reasons. If discovery raises first for any other reason the test still passes vacuously. | Evidence: + monkey
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Blank-string resolution claimed in comment but untested | The diff comments state 'Unset or blank resolves to "main"'. Tests cover unset only. Blank (RUNTIME_PROFILE="") is the historically common misconfiguration shape and is exactly the class of value the refusal targets. If resolve_runtime_profile_name treats empty string as refusal rather than default, the monitor now refuses on a blank env var while the kernel comment claims blank defaults; nothing in the tests detects this. | Evidence: + monkeypatc
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Cross-module source assertion test is path-fragile and tests a comment, not behavior | test_the_stale_service_kernel_comment_is_gone resolves service_kernel.py relative to the monitor's source file location and greps for a comment string. It fails on any file move or comment rewording and passes while stale or inverted behavioral documentation remains, e.g. if someone rewords to 'unknown values raise' while the kernel still falls back. String-grepping documentation is a proxy at best. | Evidence: + kerne
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Comment claims consolidation 'every RUNTIME_PROFILE read' without evidence | The test module docstring asserts OMN-17985 consolidated 'every RUNTIME_PROFILE read that decides CONTRACT OWNERSHIP' and calls the prior attempt incomplete. The diff provides no enumeration or guard demonstrating no third raw read exists elsewhere (e.g. in profile_ownership.py, auto-wiring loop, or CLI entrypoints). The comment-driven urgency invites the same incomplete-consolidation failure it describes. | Evidence: +OMN-17985 co
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
|
| Surface | Meaning | Blocks merge? |
|---|---|---|
| Review threads | Per-finding, posted by the reviewer | No (informational) |
Hostile Review Thread Gate |
Deterministic: unresolved hostile-reviewer threads exist | Fails until resolved (not yet a required context) |
degraded verdict |
Fewer than 2 models succeeded (infra) | No |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492)
#8502) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#3266 * evidence: OCC companion self-bind for #8502 * evidence(OMN-17985): the pr-2358 rebind probes the item it supersedes The autobind copied the contract entry's check verbatim into `dod-OmniNode-ai-omnimarket-pr-2358/command.supersede.3266.yaml`. That check reads omnimarket's uv.lock at a bare commit ref and names nothing the superseded item declares -- no path, no symbol, not the item id, and not PR 2358 -- so the Supersession Binding Ratchet (OMN-15459) refused it: "a substantive probe of the WRONG item is still a wrong-item rebind." Rewritten to resolve the SAME fact (the omnibase-core pin in uv.lock) through PR #2358's OWN merge commit, so the probe both discriminates the superseded item by its PR number and stays substantive: gh api repos/OmniNode-ai/omnimarket/pulls/2358 --jq .merge_commit_sha | xargs -I{} gh api ".../contents/uv.lock?ref={}" --jq '.content' | base64 -d | grep -cF '>=0.47.5,<0.48.0' Verified live: merge_commit_sha f21152190d7c47be483ecad1ad1bc3501cfbc504, count 2. The baseline is NOT padded and no `corrects:` record is appended -- the check now discriminates, which is the first remedy the gate itself names. RED: `check_receipt_hardening.py --supersession-corpus` reports 1 NEW violation against the frozen baseline with the autobind's version restored in place. GREEN: "2344 violating file(s); baseline 2344. Corpus matches the frozen baseline exactly." --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: jonahgabriel <jonah@omninode.ai>
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 2
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 0
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[CRITICAL] hostile-reviewer (glm-review)
Stale comment test resolves a nonexistent path and will fail at collection | test_the_stale_service_kernel_comment_is_gone computes the kernel path as _SOURCE.parent.parent / 'runtime' / 'service_kernel.py'. _SOURCE lives at tests/unit/services/, so parent.parent is tests/unit, yielding tests/unit/runtime/service_kernel.py, which does not exist. read_text raises FileNotFoundError and the whole test module errors, taking the four real behavioural tests down with it. | Evidence: kernel = _SOURCE.parent.parent
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Two divergent defaults for the same variable remain after consolidation | The health monitor's resolve_runtime_profile_name defaults to "main" while bootstrap's load_runtime_profile defaults to "default" (prefetch_policy disabled, per the rewritten comment). Consolidation was the stated goal of OMN-17985, yet the diff leaves two distinct implicit defaults for RUNTIME_PROFILE depending on which entry point reads it. A node with the variable unset wires the "default" profile but reports ownership against "mai
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Refusal turns the health monitor into a crash instead of a finding | The stated motivation is that the monitor should report misconfiguration 'as the misconfiguration it is' rather than as a fleet finding. But _discover_contracts now raises ProtocolConfigurationError. Nothing in the diff shows a caller catching it and converting it into a health finding. If the monitor loop does not catch it, the misconfigured node either crashes or silently stops reporting, which is worse than the original behaviour the di
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Positive control assertion is too weak to detect an over-refusing gate | test_a_registered_profile_still_flows_through passes an empty manifest and asserts the return 'is not None'. filter_manifest_for_runtime_profile returning an object with zero owned contracts and zero skipped contracts satisfies this. The test's own docstring says it must distinguish 'a working gate from one that refuses everything', but a gate that skips everything still returns a non-None result here. The assertion does not test what
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Source-scanning guard misses common raw-read spellings | The guard enumerates five literal substrings. os.environ.setdefault('RUNTIME_PROFILE', ...), environ['RUNTIME_PROFILE'] via a variable alias, f-string or concatenation of the variable name, or import of os.getenv under a different name all evade it. The test also reads the source file at test time, coupling the test to file layout rather than behaviour; a refactor that inlines the read elsewhere in the module chain is invisible. | Evidence: for raw in
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MINOR] hostile-reviewer (glm-review)
Blank-string env value untested despite being called out in comments | Both diff comments assert 'Unset and blank still resolve to...' yet no test sets RUNTIME_PROFILE to "" or whitespace. If resolve_runtime_profile_name treats blank as unset but a future edit treats it as an unregistered name, the refusal path fires on every deployment using blank env vars, and no test in this module would catch it. | Evidence: monkeypatch.delenv(_ENV_VAR, raising=False) is the only default-path test; no monkeypatch.setenv
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Two independent validated reads permit inconsistent profile within one check | _discover_contracts and _filter_manifest_for_runtime_profile each call resolve_runtime_profile_name separately. If they execute at different times against a mutating environment (or a future config-reload mechanism), discovery may filter ownership under one profile while filtering the manifest under another, producing a manifest the monitor attributes to a profile it never validated. The 'ONE validated read' comment in discover
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…s OMN-18012 now requires Six tests in tests/unit/topics/test_topic_provisioning_policy.py have been RED on omnibase_infra dev since #3258 (OMN-18012, merged 2026-09-07T02:35Z) added a model validator requiring non-empty sasl_plain_username / sasl_plain_password for PLAIN and the two SCRAM mechanisms. The fixtures construct SASL_SSL configs with a mechanism and no credentials, so they now raise ProtocolConfigurationError at construction, before the assertion under test runs. This is dev-wide, not this branch: the identical six FAILED lines appear on an unrelated peer branch (#3267, run 34086670202 job 101632962873) whose diff touches neither file. It blocks every omnibase_infra PR, including this one, so it is fixed here rather than cited as pre-existing. The validator is correct and stays untouched -- a SCRAM client with no credentials cannot authenticate. What was wrong is the fixtures: they assert what ModelTopicProvisioningPolicy DERIVES from a mechanism, not what a client authenticates with, and they were asserting policy on a config that could never connect. They now name the credentials the model requires. RED (this worktree, tests/unit/topics/test_topic_provisioning_policy.py): 6 failed, 28 passed. GREEN: 34 passed. Positive control, so this is not a weakened validator: dev's own tests/unit/event_bus/test_omn18012_sasl_scram_credentials.py still passes 10/10 -- a credential-less PLAIN/SCRAM config is still refused by name. The PR's own subject is unaffected: 62 passed across tests/unit/services/test_runtime_health_monitor_profile_read.py plus both runtime-profile suites.
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 6
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[CRITICAL] hostile-reviewer (glm-review)
Conflicting documented defaults for the same resolver call | The service_kernel comment states unset or blank RUNTIME_PROFILE resolves to "default". The health monitor comments state unset and blank resolve to "main". Both call sites now go through resolve_runtime_profile_name, which can have only one default. Either the resolver branches per caller (an API contract violation for a single-purpose function), or one of the two comments is false and the monitor's ownership default silently changed from "main"
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
Hard refusal inside the health monitor converts a diagnostic into a crash | The stated rationale is that the monitor should report a misconfiguration as a finding rather than as a fleet conclusion. The implemented behavior raises ProtocolConfigurationError from _discover_contracts and _filter_manifest_for_runtime_profile with no visible handler. If these run inside a monitoring loop or scheduler, an operator typo in RUNTIME_PROFILE now kills the observability surface itself, exactly when it is needed. The p
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Environment read twice per pass permits inconsistent profile decisions | _discover_contracts and _filter_manifest_for_runtime_profile each call resolve_runtime_profile_name() independently. If RUNTIME_PROFILE changes between the two reads (test harnesses, dynamic environments, or a reentrant call), the two ownership decisions in one pass disagree. The diff's own rationale argues for "the ONE validated read", yet the module performs two. | Evidence: runtime_profile = resolve_runtime_profile_name() in _discov
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Blank-string RUNTIME_PROFILE path is asserted in comments but untested | Both comments and the kernel docstring specifically call out blank (not merely unset) as resolving to the default. The test suite covers unset (delenv) and unregistered names, but never sets RUNTIME_PROFILE="" or whitespace. If resolve_runtime_profile_name distinguishes blank from unset incorrectly, no test in this diff catches it, despite the comment elevating blank handling to part of the contract. | Evidence: monkeypatch.delenv(_ENV
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Positive control is too weak to detect an over-refusing gate | test_a_registered_profile_still_flows_through constructs an empty ModelAutoWiringManifest and asserts the result is not None. A function that ignored the profile entirely and returned any manifest would pass. The docstring claims the test distinguishes a working gate from one that refuses everything, but it only distinguishes raise from no-raise on an input where ownership filtering does nothing. The registered name also only exercises "effects"
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Source-scanning tests are brittle and trivially bypassable | test_no_raw_runtime_profile_read_remains_in_this_module greps for exact literal strings. os.environ.get(_ENV_VAR) via a constant, f-strings, getattr(os.environ, ...), or reading os.environ.copy() all evade it while re-opening the gap. test_the_stale_service_kernel_comment_is_gone likewise scans the entire kernel file for one string, so any unrelated occurrence of that phrase elsewhere in the file breaks the test, and reworded stale comments pass.
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
No coverage for the refusal path in _filter via _discover's own module default | The refusal tests target the monitor module, but the diff also changes service_kernel's documented contract from warn-and-fallback to refuse. No test in the diff covers load_runtime_profile raising on an unregistered name or bootstrap's exit behavior when it does; the kernel change is comment-only in this diff, implying the raise landed elsewhere or is untested here. | Evidence: kernel_profile = load_runtime_profile() -- the di
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…repair this branch carried # Conflicts: # tests/unit/topics/test_topic_provisioning_policy.py
OMN-17985 AC3 — "a single validated read" was not yet true
OMN-17985 consolidated every
RUNTIME_PROFILEread that decides contractownership onto
resolve_runtime_profile_name(), so an unregistered role nameis refused instead of quietly emptying the manifest. omnibase_infra#3238's
close-out reported that consolidation complete: "a single validated read for
ownership decisions … twelve raw reads across
service_kernel.pyandruntime_host_process.pynow route through it."Two raw reads still fed
filter_manifest_for_runtime_profiledirectly ondev@eee49719c, in a third file the claim never named:services/service_runtime_health_monitor.py:92"main"defaultfilter_manifest_for_runtime_profilein_discover_contractsservices/service_runtime_health_monitor.py:117"main"default_filter_manifest_for_runtime_profileWhy it is worth closing even though the exposure is small
Both spelled the same
"main"default, and a booted runtime cannot reach themwith an unknown value because
load_runtime_profile()refuses first at kernelboot. So this is not a live outage — it is a true statement that was reported
as true and was not.
It is also not cosmetic. This module is the surface that reports whether the
fleet's subscriptions exist. An unregistered profile name reaching it is
filtered against a list no contract declares, so every contract is skipped, the
manifest empties, and the monitor concludes the runtime owns nothing — and
reports that as a finding about the FLEET rather than as the misconfiguration
it is. That is the same silent-orphan shape OMN-17985 exists to close, on the
one surface whose job is to notice it.
Consolidating adds the registry check; it does not move the default. Unset
and blank still resolve to
"main".Second fix in the same change: a stale comment the same work left behind
runtime/service_kernel.py:1303-1304still read:immediately above the
load_runtime_profile()call whose behaviour #3238replaced with a hard raise. A comment documenting a removed defect as current
behaviour is what the next reader acts on. Replaced with what the function
does now.
RED → GREEN
dev@eee49719c: 4 of 6 failed — both refusal tests(
_discover_contractsand_filter_manifest_for_runtime_profileaccepted anunregistered name), the source-level raw-read assertion, and the
stale-comment assertion.
test_runtime_profile.pyandtest_runtime_profile_fail_closed.py. 98passed across every dependent health-monitor and projection-liveness test
(
test_omn16994_projection_liveness,test_omn17448_projection_write_path,test_runtime_health_monitor_verdict_surface,test_service_runtime_health_monitor,test_runtime_health_monitor_profile_filter).on h201.3.
POSITIVE CONTROLS are in the test module, not just the refusals. A
registered profile (
effects) and an unset variable both still flow through,so a gate that refused everything would fail this module. A test that only ever
asserts a raise cannot tell a working gate from a broken one.
OMN-17985 — correction #4 on the ticket
(comment
cf0473a3-a93b-4510-b3ba-67d90612777e) records this gap against theclose-out that claimed it closed. dod_evidence: the RED/GREEN counts above are
reproducible from this branch against
dev@eee49719c; the pre-pushfull-suite escalation ran on the designated lab host.
Evidence-Ticket: OMN-17985
Evidence-Source: OCC#8502