Repository navigation
feat(OMN-18949): gate census drift in CI, and report live lane drift to a person - #3909
jonahgabriel wants to merge 3 commits into
Conversation
…to a person Nothing refused on census drift and nothing reported it. The committed snapshot carried drift_count 5 at severity warning; the only surface that read it rendered the prose string "5 drift item(s) -- see census" into a doctrine file, where the next refresh normalised it away. Two halves, because they are two questions. CI (scripts/check_lane_census_drift.py, wired into the lane-census-staleness job): do the two COMMITTED files still describe the same world? A finding reporting a container as undeclared that the manifest now declares, a snapshot that miscounts its own findings, a lane one file knows and the other does not, and a manifest lane the table generator has no entry for are each a hard failure. It deliberately does NOT require drift_count to be zero: live drift on a mutable lane is a normal reportable state, and a gate refusing every PR over it would be switched off within a day. The .201 reporter (a census row): has the LIVE topology moved? CI cannot see the lab host's docker daemon at all. The row reuses the existing Slack poster, per-key de-duplication, hysteresis and the existing cron -- no new cron unit, no new credential. "More than one tick" is the shared CONFIRM_TICKS default and is deliberately NOT reimplemented in the probe, where it would be a second answer that could disagree. Ships with its incident replay, as the default-deny coverage gate requires. The artifacts are the census manifest and the table generator read out of the object store at 2e4eec0, where OMN-18320 had removed the lab prod lane and both of the generator's static maps still carried its rows. They stayed six days, invisible because the generator iterates the manifest, and were removed only when the pre-PR pool claimed the exact ports the dead row named. The discriminator requires the same functions to return nothing against the current tree, so a guard that refused everything could not satisfy it. Three defects found and fixed on the way. The generator's docstring claimed CI failed when the doctrine file's lane block drifted; no such check existed, and the workflow ran the generator against nothing as a smoke test. The dogfood lane was absent from both static maps and the fleet CI bus from the port map, so each rendered an em-dash for that reason alone, indistinguishable from a declared absence; both are declared now, and differently, behind explicit sentinels. And the probe's first draft discarded the checker's stderr, turning a missing PyYAML into the row "exit 1". --check <path> implements the comparison the docstring claimed, ignoring only the provenance timestamp. It is NOT run against the registry repo's doctrine file in CI: that is a different repository this job does not check out, and the residual is stated in the workflow rather than papered over.
…ibase_infra#3909 (#10645) * evidence(OMN-18949): author OCC companion for OmniNode-ai/omnibase_infra#3909 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head e04ed3fb6faa4ed5252146a4b3cd29acaa8ca211. * evidence(OMN-18949): self-bind OCC#10645 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@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: 0
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 7
Nit-level findings suppressed: 2
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.
Below quorum: 7 finding(s) raised by one model only (OMN-18479)
These are reported and NOT dropped, but they get no thread and do not block: a single model's finding no other model reproduced is not evidence enough to stop a merge. Read them; act on them if they are right.
- [MAJOR]
.github/workflows/lane-census-staleness.yml, 'Verify the lane table generator renders every declared lane' step(glm-review) — CI row-count check uses fragile grep pattern for rendered lanes | The workflow counts rendered table rows with grep -c '^| [a-z]'. This assumes every lane name begins with a lowercase ASCII letter and - [MINOR]
deploy/maintenance/omninode-system-slack-report.sh, check_census_drift()(glm-review) — Checker exit 1 with empty stdout is classified as WARNING, understating findings | In check_census_drift(), the branchif (( rc == 2 )) || [[ -z "$json" ]]fires before the rc==1 branch. Per the che - [MINOR]
deploy/maintenance/omninode-system-slack-report.sh, check_census_drift() staleness block(glm-review) — Staleness probe assumes naive emitted_at timestamps are UTC | The embedded Python replaces a missing tzinfo with UTC. If the hourly census writer on the host emits naive local-time timestamps, the com - [MINOR]
deploy/maintenance/omninode-system-slack-report.sh, check_census_drift() rc==1 branch(glm-review) — CRITICAL paging triggered by committed-artifact findings, not only live topology | The rc==1 branch unconditionally emits census|CRITICAL when the checker exits 1 against the live snapshot. The checke - [MINOR]
scripts/check_lane_census_drift.py, declared_containers()(glm-review) — declared_containers silently drops malformed service entries | The positive control refuses empty or unparseable inputs, but a service entry that is not a dict or lacks 'name' is filtered out of the d - [MINOR]
tests/unit/scripts/test_lane_census_sink_omn18949.py(glm-review) — Sink unit tests depend on the mutable committed lane-manifest | test_lane_census_sink_omn18949.py points OMNINODE_CENSUS_MANIFEST at the repository's live deploy/lane-census/lane-manifest.yaml. These - [MINOR]
scripts/check_lane_census_drift.py, evaluate()(glm-review) — Stale-check only recognizes two finding kinds; all other snapshot finding kinds are ungated | evaluate() gates stale_unexpected_container and stale_container_absent plus lane/count consistency. Any ce
|
| 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)
…e-clock a census test Two CI failures, one mine and one pre-existing in a file this change touches. The fleet sink's harness does not disable the census probe, so on a runner with no live snapshot the new row fires and moves that file's issue counts. It is switched off there for the same reason the other three probes already are; the census rows are asserted in their own module, which switches it back on. The refresh-decision test read the REAL committed snapshot for BOTH sides of its steady case, so its own docstring's claim to test the rule and not the clock was false of that half: the assertion held only while the committed file was under three days old and went red on the third day after each refresh. It went red today. That is the same defect the sibling test one function down records in its own docstring, reached by a different route. The steady case now uses a committed-side fixture, recent but an hour behind the candidate, which is what makes it a test of the threshold; the aging case still reads the real file, so the real artifact staying parseable is still asserted.
…stead of this branch's This branch de-clocked the refresh-decision test's steady case by giving the committed side a recent fixture. OMN-18980 landed on dev while this was open and does it more thoroughly: both documents are synthetic, the committed one is pinned a day old, and the docstring records that the original expired on 2026-09-21 and reddened a test split on EVERY open pull request in the repository -- during exactly the four-day margin the three-day threshold exists to create, so the refresh pull request itself could not land either. Their version is taken whole. Two fixes for one defect in one file is how the weaker one survives, and theirs is not the weaker one.
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 0
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 7
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.
Below quorum: 7 finding(s) raised by one model only (OMN-18479)
These are reported and NOT dropped, but they get no thread and do not block: a single model's finding no other model reproduced is not evidence enough to stop a merge. Read them; act on them if they are right.
- [MAJOR]
deploy/maintenance/omninode-system-slack-report.sh (check_census_drift)(glm-review) — CRITICAL drift row depends on unverified --json checker contract | check_census_drift invokes check_lane_census_drift.py with --json and branches on exit codes 0/1/2. If the checker's argparse does no - [MINOR]
deploy/maintenance/omninode-system-slack-report.sh (check_census_drift)(glm-review) — TOCTOU between staleness read and drift evaluation of live snapshot | The hourly census timer can rewrite the live snapshot between the emitted_at staleness read and the checker invocation. The checke - [MINOR]
deploy/maintenance/omninode-system-slack-report.sh (check_census_drift)(glm-review) — Unvalidated numeric env vars feed bash arithmetic | stale_h and the timeout duration are interpolated from environment without validation. A non-numeric OMNINODE_CENSUS_STALE_HOURS causes an arithmeti - [MINOR]
deploy/maintenance/omninode-system-slack-report.sh (check_census_drift)(glm-review) — Naive timestamps assumed UTC in staleness probe | The inline python treats a tz-naive emitted_at as UTC via replace(tzinfo=utc). If the census writer ever emits local time, staleness is computed again - [MINOR]
.github/workflows/lane-census-staleness.yml (generator verification step)(glm-review) — CI row-count regex is a fragile proxy for rendered lanes | The workflow counts rendered rows with grep -c '^| [a-z]'. Any lane name beginning with an uppercase letter, digit, or non-letter is not coun - [MINOR]
scripts/check_lane_census_drift.py (evaluate)(glm-review) — evaluate() behavior on missing drift_count or lanes_checked keys untested in visible diff | The docstring promises exit 2 for 'a snapshot with no lanes_checked', but the truncated evaluate() body read - [MINOR]
tests/unit/scripts/test_check_lane_census_drift_omn18949.py(glm-review) — test_a_manifest_lane_missing_from_a_generator_map_is_drift skips missing-key resolution path | That unit case passes undeclared=undeclared_lanes(m) explicitly, exercising the real helper, but the visi
|
Superseded by #3913, cut fresh at current dev. This branch was cut before two things landed: OMN-18944 added a structurally identical reporter sink in the same four places this change touches, and OMN-18980 fixed the census refresh test's end-to-end case that this branch had also touched. Both are taken as they stand on dev; #3913 carries only this sink on top and merges clean. |
Closes OMN-18949 (under OMN-18276).
The gap
Nothing refused on census drift, and nothing reported it. The committed snapshot carried a drift count of five at severity warning; the only surface anywhere that read it rendered the prose string
5 drift item(s) — see censusinto a doctrine file, where the next refresh normalised it away.A second, quieter half turned up on the way in. The table generator's own docstring claimed CI fails when the doctrine file's lane block drifts from what it would produce. No such check existed. The workflow invoked the generator with neither a comparison flag nor an output path, purely to prove it does not crash, and the claim stood for months.
What landed
Two halves, because they answer two different questions.
In CI, a new drift gate wired into the existing lane-census job: do the two committed files still describe the same world?
stale_unexpected_containerstale_container_absentsnapshot_lane_unknownfinding_count_mismatchgenerator_lane_undeclaredgenerator_map_orphanIt deliberately does not require the drift count to be zero. Live drift on a mutable lane is a normal, reportable state, and a gate refusing every pull request over it would be switched off within a day. A test pins that negative so a later change has to argue with it.
On the lab host, a census row in the system report: has the live topology moved? CI cannot see that host's docker daemon at all. The row reuses the existing poster, per-key de-duplication, hysteresis and cron. No new cron unit, no new credential, no new probe script — it drives the same checker CI runs, pointed at the live snapshot, so the two verdicts cannot diverge.
"More than one tick" is not implemented in the probe. The shared alert machinery already requires a key to hold its status for two consecutive ticks before it pages; a private counter would be a second answer that disagrees the first time either default changed. A test pins the reuse.
Three defects found and fixed on the way
exit 1. Captured now, so the row carries the cause.Incident replay, as the default-deny coverage gate requires. Both artifacts are read out of the object store at
2e4eec05c, where the lab prod lane had been removed from the manifest and both static maps still carried its rows. They stayed six days, invisible because the generator iterates the manifest, and were removed only when the pre-PR pool claimed the exact ports the dead row still named. The discriminator requires the same functions to return nothing against the current tree, so a guard refusing everything could not satisfy it, and a third case requires the finding to name only the retired lane.RED/GREEN on the undeclared-lane finding, by removing the two entries this PR adds:
Focused suites: 76 passed across the new drift tests, the new reporter-sink tests, the incident replay, and the two pre-existing census modules. The whole reporter suite (62 cases) re-run green after its harness gained the new probe's off-switch. Lint, format and strict type-check clean on both scripts. The coverage gate reports
62 guard(s) covered, 159 wired, 0 pending.An unreadable or empty input exits distinctly rather than reporting agreement, and every run prints the input sizes it derived its verdict from, so a zero is never reported bare.
Residual, stated rather than implied
--check <path>now implements the comparison the docstring claimed, ignoring only the provenance timestamp. It is not run against the registry repository's doctrine file in this job: that is a different repository this CI does not check out, and making a path-filtered gate depend on a cross-repo fetch trades one silent failure for an availability risk on unrelated pull requests. Closing it needs a read token and belongs on the six-hourly refresh workflow, which already mints one. The residual is written into the workflow beside the step.The reporter change reaches the lab host through the normal maintenance sync, so the census row appears on the first tick after that sync, not on merge.
Enforcement path
The job is path-filtered and is not itself a required context. A red check-run on a head is caught by the default-deny external sweep under the required summary umbrella, which landed in the commit this branch is based on. No branch-protection change is requested, and none should be made: adding a path-filtered job to the required set wedges every pull request that does not touch those paths.
DoD
Fresh-event: the first pull request touching the census paths after merge runs the gate, and the first host tick after the maintenance sync emits a census row. Deliberately not self-authored.
Operator action
None required. No branch-protection change, no credential change, no version bump (no source package change).
Evidence-Ticket: OMN-18949
Evidence-Source: OCC#10645