Repository navigation
fix(OMN-18135): the state vocabulary reads the criteria it was written for - #3413
Conversation
…n for Measured against OMN-17771's real acceptance section, not a paraphrase: the first cut of the marker set recognised 2 of its 5 criteria, so _every_criterion_is_state_shaped returned False and the ticket the ruling names as its worked example would still have been held by the behaviour conjunct. Shipping that would have been a rule that reads well and does not do the thing it was ruled for. Five phrases added, each lifted from one of those criteria rather than invented, which is the standing rule for this set: "Zero `client_id=x` ... occurrences" -> zero ... occurrences "`beta-layout` checkers green" -> checkers green "a client that exists on the plane" -> exists on the "Merged to `main` and read back" -> merged to `x "returns HTTP 200 with a form" -> HTTP <status> The veto is unchanged and still wins ties, so the widening cannot become a general loosening. Four controls assert exactly that: a criterion carrying a widened state phrase AND behaviour language still reads behaviour-shaped. The worked example is now pinned end to end -- all five of OMN-17771's real criteria, asserted individually and through the aggregate. That assertion is what would have caught the first cut. node_version 1.12.0 -> 1.12.1.
|
| 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)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 1
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)
Regex typo: stray backtick yields single-character word match | The new marker
\bmerged\s+to\s+?\wcontains a stray trailing backtick after \w, producing the pattern?\w(optional backtick, then exactly one word character). This matches 'merged to m' in 'merged to main' but also matches 'merged to x', 'merged to 1', etc. The comment claims the phrase is 'lifted from a criterion', but the pattern as written would match 'merged tom' with a lone backtick-word fragment. The intended pattern was almost ceResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
HTTP \d{3} marker classifies error responses as state-shaped |
\bHTTP\s+\d{3}\bmatches any three-digit status, including HTTP 500, HTTP 502, HTTP 401. A behaviour criterion such as 'the endpoint returns HTTP 500 when the token is invalid' satisfies this marker as state-shaped evidence of success semantics, which inverts the marker's purpose. The worked example only exercises HTTP 200. | Evidence: | \bHTTP\s+\d{3}\b # "returns HTTP 200 with a form" | Fix: Constrain to success codes, e.Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
[^.] gap crosses newlines and AC boundaries |
\bzero\b[^.]{0,80}\boccurrences?\buses a negated character class that matches newlines in the raw triple-quoted pattern string. The 80-character gap can therefore span a sentence terminator that is a newline rather than a period, joining 'zero' and 'occurrences' from different acceptance criteria within one AC block and producing spurious matches. The worked example passes regardless because AC1 contains both words. | Evidence: | \bzero\b[^.]{0,80}\boccurrencResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Marker set lacks a regression guard against future criterion text | The comment establishes a 'standing rule' that markers earn inclusion only from existing criteria, yet nothing in the diff enforces that rule mechanically. The negative test cases are hand-picked behaviour sentences; a future widened phrase such as
exists on the(an unusually generic trigram-like phrase: 'the handler exists on the plane', 'a session exists on the node', 'the file exists on the disk') will admit behaviour criteria the vetoResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…ibase_infra#3413 (#8991) * evidence(OMN-18135): author OCC companion for OmniNode-ai/omnibase_infra#3413 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 05a97a9e97320b80a9bba8d39a5c76e1cbc17942. * evidence(OMN-18135): self-bind OCC#8991 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
…oven load-bearing (#3414) Review finding on #3413, accepted rather than argued: the vocabulary test asserted the whole OMN-17771 acceptance body classifies as state-shaped, so a marker that never fires, or one whose semantics are wrong, was invisible -- a neighbouring marker carried the assertion and the aggregate stayed green. That was not hypothetical on this body. Measured by deleting one alternation line at a time from the live pattern and re-running: delete the HTTP marker -> the end-to-end test stays GREEN delete the merged-to marker -> the end-to-end test stays GREEN because AC2 is already matched by the older `returns` marker and AC5 by `read back`. Two of the five phrases this PR added contributed nothing that any test could see, which is precisely what the reviewer said. Three things close it, per marker rather than per body: 1. Isolation. Each added phrase gets the shortest criterion carrying it and nothing else, lifted from the OMN-17771 criterion the phrase was taken from. Where the real criterion also carries a pre-existing marker the isolated form drops that word -- "gives HTTP 200" rather than "returns HTTP 200" -- because leaving it in is what made the row vacuous. 2. Mutation controls, which are the half isolation alone cannot do. Each test deletes that marker's own alternation line from the vocabulary and asserts the same criterion, held byte-identical, stops classifying. A misspelled, over-escaped or shadowed marker cannot pass its control. The helper refuses to run unless the fragment names exactly one line and that line is an alternation branch, so a control can never be built on a mangled pattern and quietly prove nothing. Each control also asserts the other four markers still fire, so an empty or inverted pattern cannot satisfy every row at once. 3. Near-miss and veto coverage. Each marker must refuse the neighbouring string it must not accept, which catches looseness rather than silence, and the behaviour veto is now asserted against every added marker individually rather than against four sampled combinations. The end-to-end blob test and the existing veto controls are unchanged and still present; this adds to them rather than replacing them. Attribution: a concurrent lane in the same worktree had independently staged the isolation and near-miss tests for this same finding. That work is kept here rather than discarded, with its HTTP row repaired -- it read "the URL returns HTTP 200" and so passed with the HTTP marker deleted, reproducing the reported defect inside the fix for it. No src change; vocabulary and node_version are untouched.
|
No OCC evidence companion was minted for this PR. this PR is already bound to OCC#8991; its evidence companion exists and nothing needs authoring To clear this: Nothing to do — the companion already exists. Reported by |
…n for (#3413) Measured against OMN-17771's real acceptance section, not a paraphrase: the first cut of the marker set recognised 2 of its 5 criteria, so _every_criterion_is_state_shaped returned False and the ticket the ruling names as its worked example would still have been held by the behaviour conjunct. Shipping that would have been a rule that reads well and does not do the thing it was ruled for. Five phrases added, each lifted from one of those criteria rather than invented, which is the standing rule for this set: "Zero `client_id=x` ... occurrences" -> zero ... occurrences "`beta-layout` checkers green" -> checkers green "a client that exists on the plane" -> exists on the "Merged to `main` and read back" -> merged to `x "returns HTTP 200 with a form" -> HTTP <status> The veto is unchanged and still wins ties, so the widening cannot become a general loosening. Four controls assert exactly that: a criterion carrying a widened state phrase AND behaviour language still reads behaviour-shaped. The worked example is now pinned end to end -- all five of OMN-17771's real criteria, asserted individually and through the aggregate. That assertion is what would have caught the first cut. node_version 1.12.0 -> 1.12.1.
…oven load-bearing (#3414) Review finding on #3413, accepted rather than argued: the vocabulary test asserted the whole OMN-17771 acceptance body classifies as state-shaped, so a marker that never fires, or one whose semantics are wrong, was invisible -- a neighbouring marker carried the assertion and the aggregate stayed green. That was not hypothetical on this body. Measured by deleting one alternation line at a time from the live pattern and re-running: delete the HTTP marker -> the end-to-end test stays GREEN delete the merged-to marker -> the end-to-end test stays GREEN because AC2 is already matched by the older `returns` marker and AC5 by `read back`. Two of the five phrases this PR added contributed nothing that any test could see, which is precisely what the reviewer said. Three things close it, per marker rather than per body: 1. Isolation. Each added phrase gets the shortest criterion carrying it and nothing else, lifted from the OMN-17771 criterion the phrase was taken from. Where the real criterion also carries a pre-existing marker the isolated form drops that word -- "gives HTTP 200" rather than "returns HTTP 200" -- because leaving it in is what made the row vacuous. 2. Mutation controls, which are the half isolation alone cannot do. Each test deletes that marker's own alternation line from the vocabulary and asserts the same criterion, held byte-identical, stops classifying. A misspelled, over-escaped or shadowed marker cannot pass its control. The helper refuses to run unless the fragment names exactly one line and that line is an alternation branch, so a control can never be built on a mangled pattern and quietly prove nothing. Each control also asserts the other four markers still fire, so an empty or inverted pattern cannot satisfy every row at once. 3. Near-miss and veto coverage. Each marker must refuse the neighbouring string it must not accept, which catches looseness rather than silence, and the behaviour veto is now asserted against every added marker individually rather than against four sampled combinations. The end-to-end blob test and the existing veto controls are unchanged and still present; this adds to them rather than replacing them. Attribution: a concurrent lane in the same worktree had independently staged the isolation and near-miss tests for this same finding. That work is kept here rather than discarded, with its HTTP row repaired -- it read "the URL returns HTTP 200" and so passed with the HTTP marker deleted, reproducing the reported defect inside the fix for it. No src change; vocabulary and node_version are untouched.
Evidence-Source: OCC#8991
Evidence-Ticket: OMN-18135
OMN-18135 — the state vocabulary reads the criteria it was written for
Follow-up to
omnibase_infra#3409(squashf1f72ccce074947d7038156dcb710bd06fda44f1), which landed the readback join. This fixes a defect I found by measuring rather than assuming, and it is worth stating plainly because it is the difference between a rule that reads well and a rule that does the thing it was ruled for.What was wrong
The ruling at
docs/tracking/ROLLING_WORK_LEDGER.md:6148names OMN-17771 as its worked example. Its five acceptance items all classifyreadbackon mergeddev, so the class half works. But when I ran the criterion-shape predicate against that ticket's real acceptance section rather than a paraphrase, it recognised 2 of 5:_every_criterion_is_state_shapedtherefore returned False, the behaviour conjunct still held the ticket, and the worked example would not have worked. My fixtures used criteria I wrote, which is exactly how a predicate passes its own tests and fails the corpus.The change
Five phrases, each lifted from one of those criteria rather than invented. That is the standing rule for this set, stated in the code: a marker earns its way in by appearing in a criterion that exists.
zero ... occurrencesclient_id=x... occurrences"checkers greenbeta-layoutcheckers green"exists on themerged to \x`mainand read back"HTTP <status>All five criteria now read state-shaped, and the aggregate is True.
Why this is not a general loosening
The behaviour veto is unchanged and still wins ties. Four controls assert precisely that a criterion carrying a widened state phrase and behaviour language still reads behaviour-shaped:
main, and the retry path stops after 3 attempts"The worked example is now pinned end to end, all five real criteria asserted individually and through the aggregate. That assertion is the one that would have caught the first cut, and it is the thing this PR really adds.
Verification
05a97a9e97320b80a9bba8d39a5c76e1cbc17942:REMOTE LAB RUN PASS ... h201.3 ran 27612 tests green ... (suite log sha256 241f3f93ffd0ff73d8e4bd21865cda1bfd3de7136b70da174fa9c85d17fe0f7e, 308s).mypy --strict,ruff format --check,ruff check: clean.node_version1.12.0 to 1.12.1.evidence-autoclose-sweepworkflow, not on the.201compose dev lane or theonex-laboverlay. No lab pass exercises it and none is claimed.