Repository navigation
fix(OMN-18420): a lab-pass receipt is this job's own, not one left on the shared deploy host - #3609
Conversation
… the shared host Every lab-pass emitter in this repository and in the sibling reusable runs on the same omnibase-deploy host and wrote its receipt and checks to a FIXED absolute path under the shared temp root. mkdir -p on an existing directory is a no-op, so a receipt.json from an earlier job survived into the next one; when that job's emit did not overwrite it, the presence assertion found the stale file, passed, and upload-artifact published it under THIS job's sha-keyed name. Measured: omnimarket run 35035178406 uploaded artifact 10424013456 named lab-pass-receipt-compose-dev-bc21f733... (an omnimarket commit) carrying a payload sha of a7ea64f... (an omnibase_infra commit), with a deployed_revision check citing check_dev_lane_staleness.py -- this workflow's emitter shape, not the sibling one's sibling_revision / check_lane_sibling_revision.py. The direction was safe: evaluate_gate cross-checks the artifact name against the payload sha and refuses the disagreement. The cost is that the sibling sha can then never receipt PASS, and rule 24(b) refuses it for staging. Two changes, both fail-closed: * each emitting job claims RUNNER_TEMP/lab-pass-<run id>-<attempt>-<job> and removes it before use, so no file can survive from another job. * a new 'verify' subcommand asserts the receipt on disk carries this job's own sha and lane, run as a separate step before every upload. Kept out of 'emit', which writes the file and so can only agree with itself. config/ci_evidence_policy.yaml follows the three shared-host entries. The boot-gate entry keeps its literal path deliberately: that job runs on an ephemeral GitHub-hosted runner where no second job shares the filesystem. Tests parse the workflow YAML and pin the properties rather than the spelling: no lab-pass path under a shared temp root, every path derived from a per-run variable, the uploaded directory is the one emit wrote into, and every emitter verifies before uploading. Positive control: all eight fail against the pre-change workflows.
|
No OCC evidence companion was minted for this PR. this PR is already bound to OCC#9753; its evidence companion exists and nothing needs authoring To clear this: Nothing to do — the companion already exists. Reported by |
|
| 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)
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 3
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)
Verify failure does not prevent upload of the stale receipt | The verify steps run with
if: always(), and the upload-artifact steps also run unconditionally (the workflow comments state 'always() on emit and upload' so that a FAILED lab pass is still recorded). An exit code of 1 from verify marks the step red but does not gate the subsequent upload step. The stale, mismatched receipt is still published under the sha-keyed artifact name; only the presence assert upstream of it is bypassed. The docstring exResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Verify step with if: always() can obscure the original failure | Because verify runs
if: always()after a possibly-failed emit, a job whose probe or emit already failed will additionally fail at verify on a missing file, producing two errors where the comment contract ('a FAILED lab pass is the record that tells it failed apart from nobody ran it') expects one receipt carrying FAIL. If the upload gating is fixed per the prior finding, this double-failure path will suppress the upload of legitimate failureResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
rm -rf on an interpolated RUNNER_TEMP path without guarding emptiness |
${RUNNER_TEMP}/lab-pass-...is expanded withset -uin effect for the assignment, so RUNNER_TEMP unset would abort, which is acceptable. Howeverrm -rfof a path derived from environment variables is a pattern worth pinning: if RUNNER_TEMP were ever set to empty or to/by runner misconfiguration, the glob-free but variable-rooted rm -rf becomes destructive. The variables GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT and GITHUB_JOB are numeResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
verify_emitted does not validate receipt_version or result | verify_emitted compares sha and lane only. A receipt from a future schema version or a receipt whose result is not PASS will verify successfully. The gate downstream checks PASS, so correctness of the delivery decision is preserved, but the emit-side assertion advertised as 'the receipt on disk is this job's own' accepts files it cannot fully vouch for; a stale receipt could in principle share sha and lane if a prior attempt of the same run retrie
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Interpreter inconsistency in new verify step | Sibling steps use
python3 scripts/ci/assert_evidence_artifact.py; the new verify steps useuv run python. The diff does not establish that uv is installed and synced in every job where these steps run (the k3s overlay job installs with --frozen, but environment provisioning differs across jobs). A missing or unsynced uv environment turns the identity assertion into an infrastructure failure that masks the evidence question it exists to answer. | Evidence: uResolve 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)
ci_evidence_policy.yaml now contains an unexpanded env var reference | required_paths entries change from literal absolute paths to
${LAB_PASS_DIR}/receipt.json. Nothing in the diff shows that consumers of this policy file perform environment expansion. If assert_evidence_artifact.py or any downstream checker treats required_paths as literal filesystem paths, the check will look for a directory literally named${LAB_PASS_DIR}and fail, or worse, silently pass via a different code path. The comment in thResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Workflow-YAML string-matching tests are brittle heuristics | test_no_lab_pass_path_is_rooted_in_a_shared_temp_directory scans every line containing the substrings 'lab-pass', 'lab_pass' or 'LAB_PASS_' for '/tmp/' etc. Comments inside run blocks (the new 30-line OMN-18420 commentary itself contains 'under /tmp') would trip the denylist if phrased with a trailing slash, and a future step naming a shared path in a way that omits these substrings escapes the check entirely. test_every_lab_pass_receipt_path_is_d
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
No test that verify rejects a zero-byte or partially written receipt at the on-disk path | The diff's stated threat model includes partially-written receipts (cited from OMN-18247), and assert_evidence_artifact.py exists specifically because of the zero-byte gap. The new verify_emitted handles read errors, but the test suite only covers well-formed JSON with wrong sha/lane and a missing file. A zero-byte file present at receipt.json is the exact residue shape the fix is meant to catch and is untested. | Evi
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…ibase_infra#3609 (#9753) * evidence(OMN-18420): author OCC companion for OmniNode-ai/omnibase_infra#3609 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 3d05d9298d7252c8f191cfa7792452aef126c0de. * evidence(OMN-18420): self-bind OCC#9753 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
… the shared host (#3609) Every lab-pass emitter in this repository and in the sibling reusable runs on the same omnibase-deploy host and wrote its receipt and checks to a FIXED absolute path under the shared temp root. mkdir -p on an existing directory is a no-op, so a receipt.json from an earlier job survived into the next one; when that job's emit did not overwrite it, the presence assertion found the stale file, passed, and upload-artifact published it under THIS job's sha-keyed name. Measured: omnimarket run 35035178406 uploaded artifact 10424013456 named lab-pass-receipt-compose-dev-bc21f733... (an omnimarket commit) carrying a payload sha of a7ea64f... (an omnibase_infra commit), with a deployed_revision check citing check_dev_lane_staleness.py -- this workflow's emitter shape, not the sibling one's sibling_revision / check_lane_sibling_revision.py. The direction was safe: evaluate_gate cross-checks the artifact name against the payload sha and refuses the disagreement. The cost is that the sibling sha can then never receipt PASS, and rule 24(b) refuses it for staging. Two changes, both fail-closed: * each emitting job claims RUNNER_TEMP/lab-pass-<run id>-<attempt>-<job> and removes it before use, so no file can survive from another job. * a new 'verify' subcommand asserts the receipt on disk carries this job's own sha and lane, run as a separate step before every upload. Kept out of 'emit', which writes the file and so can only agree with itself. config/ci_evidence_policy.yaml follows the three shared-host entries. The boot-gate entry keeps its literal path deliberately: that job runs on an ephemeral GitHub-hosted runner where no second job shares the filesystem. Tests parse the workflow YAML and pin the properties rather than the spelling: no lab-pass path under a shared temp root, every path derived from a per-run variable, the uploaded directory is the one emit wrote into, and every emitter verifies before uploading. Positive control: all eight fail against the pre-change workflows.
…ibase_infra#3609 (#9753) * evidence(OMN-18420): author OCC companion for OmniNode-ai/omnibase_infra#3609 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 3d05d9298d7252c8f191cfa7792452aef126c0de. * evidence(OMN-18420): self-bind OCC#9753 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
Closes OMN-18420.
The defect, measured
Every compose-dev and onex-lab-k3s lab-pass emitter in this repository, and the sibling emitter in
runtime-rebuild-trigger-reusable.ymlthat every other repository calls, runs on the sameomnibase-deployhost and wrote its receipt and its checks to a fixed absolute path under the shared temp root.mkdir -pon an existing directory is a no-op, so areceipt.jsonwritten by an earlier job survived into the next one. When that job'semitdid not overwrite it,assert_evidence_artifact.pyfound the stale file, reported evidence present, andupload-artifactpublished it under this job's sha-keyed name.omnimarket run
35035178406uploaded artifact10424013456:lab-pass-receipt-compose-dev-bc21f733ee76087a686b8dd535ad44e06db22ff3(an omnimarket commit)shaa7ea64f56f14285ad985d3ec6a653d0d706fb1e7(anomnibase_infracommit)deployed_revision, citingcheck_dev_lane_staleness.pyThat check name is this repository's own emitter shape. The sibling emitter's is
sibling_revisioncitingcheck_lane_sibling_revision.py, so the payload is not a mislabelled sibling receipt — it is another job's output republished under an omnimarket sha.The direction was safe.
evaluate_gatecross-checks the artifact name against the payload sha and refuses the disagreement, so this can never produce a false PASS. What it does produce is a sha that can never receipt PASS, and rule 24(b) then refuses that sha for staging. That is the gate the named-revision delivery path on the parent ticket is built on.What changed
Isolation. Each emitting job's window-start step now claims
${RUNNER_TEMP}/lab-pass-<run id>-<attempt>-<job>and removes it before use.RUNNER_TEMPalone is not enough — a self-hosted runner reuses it across jobs — so the run id, attempt and job name narrow it to one execution, and the removal is what makes the claim true rather than merely likely.Identity. A new
verifysubcommand onscripts/ci/lab_pass_receipt.pyasserts the receipt on disk carries this job's own sha and lane, wired as a separate step before every upload. Deliberately not folded intoemit:emitwrites the file, so it can only ever agree with itself. The value is that this runs over whatever is actually on disk at upload time. It moves the same assertionevaluate_gatealready makes to the side that can see what happened, instead of leaving it to surface an hour later in another repository's logs as an unexplained refusal.config/ci_evidence_policy.yamlfollows for the three shared-host entries. The boot-gate entry keeps its literal path on purpose — that job runs on an ephemeral GitHub-hosted runner where no second job shares the filesystem.Tests
tests/ci/test_lab_pass_receipt_isolation_omn18420.pyparses the workflow YAML and pins properties rather than spellings, because a text match would pass the moment somebody wrote a different shared path:--out/--checks-json/--pathargument is derived from a per-run variableupload-artifactpublishes is the oneemitwrote intoplus four cases against the real
verifyimplementation: it accepts this job's own receipt (the positive control), refuses one carrying another sha naming both, refuses one carrying another lane, and refuses a missing file.Verification
Positive control. With the two workflow files restored to their
origin/devcontent and the new script left in place, all eight workflow-property cases fail and only the four implementation cases pass. The zero is therefore a measurement, not an empty sweep.Pushed through the governed pre-push hook; no local gate was bypassed.
Lab proof
This change alters what three lab emitters write and where. It does not alter what they measure, and its own correctness is provable from the parsed workflow graph, which is what the tests above do. The live confirmation is the next runtime-path merge on either repository: the emitted artifact's payload sha must equal the name's sha. That readback is recorded on the parent ticket rather than asserted here.
Evidence-Ticket: OMN-18420
Evidence-Source: OCC#9753