Skip to content

fix(OMN-19400): negative control no longer collides with widened causal markers - #2870

Merged
jonahgabriel merged 1 commit into
devfrom
jonah/omn-19400-fix-negative-control-marker-collision
Sep 25, 2026
Merged

jonahgabriel merged 1 commit into
devfrom
jonah/omn-19400-fix-negative-control-marker-collision

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

OMN-19400

Dev-wide red on Tests (Split 14/20), visible on every PR running that shard (e.g. omnimarket#2859, jobs 107816799092/107820471172).

Root cause: OMN-19401 (omnimarket#2851, merged same day) widened the step_by_step_explanation heuristic markers to also accept causal connectives (because, therefore, consequently, since, as a result, this means), so a correct causal explanation is no longer wrongly refused. OMN-19400's own test (merged via omnimarket#2852) used a negative-control string — "9 sheep are left, because all but 9 of them died." — that now legitimately contains "because", so _apply_heuristic_check no longer flags it, and the test's assertion that it SHOULD be flagged (is not None) failed. Two independently-correct merged PRs interacting, not a flake.

Fix: tests/unit/delegation/test_step_by_step_directive_reaches_the_model_omn19400.py — replace the negative-control string with "9 sheep are left.", which carries none of the ten accepted markers, and assert that directly so a future marker addition reddens here instead of on an unrelated PR's shard.

Proof

uv run pytest tests/unit/delegation/test_step_by_step_directive_reaches_the_model_omn19400.py -v
5 passed in 3.71s

ruff format / ruff check --fix clean, mypy --strict on the changed file: Success: no issues found in 1 source file. Pre-commit ran clean on push (all applicable hooks passed).

Not runtime-affecting (test-only change, no src/ touched).

[x] Title contains OMN-XXXX
[x] Body cites OMN-19400
[x] uv run ruff format / ruff check --fix
[x] Focused local tests (5 passed)
[x] pre-commit ran clean on push

Evidence-Ticket: OMN-19400
Evidence-Source: OCC#11152

…l marker

OMN-19401 (omnimarket#2851, merged same day) widened the
step_by_step_explanation heuristic to also accept causal connectives
(because/therefore/consequently/since/as a result/this means) so a
correct causal explanation is not wrongly refused.

test_an_answer_that_follows_the_directive_meets_the_rule's negative
control, "9 sheep are left, because all but 9 of them died.", now
contains "because" and is legitimately no longer flagged, so the
assertion that it SHOULD be flagged failed dev-wide on every PR that
runs Tests (Split 14/20).

Replace the negative control with an answer that carries none of the
ten accepted markers, and assert that directly so a future marker
addition fails loudly here instead of red on an unrelated PR.
@jonahgabriel jonahgabriel added the hold:auto-merge Holds auto-merge (check_auto_merge_hold.py); remove to release label Sep 24, 2026
@onexbot-occ-writer

Copy link
Copy Markdown
Contributor

OCC autobind did not mint a companion for this PR: no changed-file candidate could be proven RED against the merge base, and emitting a PR-existence probe instead would be non-falsifiable evidence (OMN-15247). Hand-authored evidence is required.

@github-actions

Copy link
Copy Markdown

✅ Hostile Reviewer — PASSED

Blocking findings (critical/error): 0
Total findings: 0
Models succeeded: qwen3-review,qwen3-review-b


Gate semantics

Verdict Meaning Blocks merge?
passed >=2 models succeeded, no critical/error findings No
blocked CRITICAL or ERROR findings found Yes
degraded Fewer than 2 models succeeded (infra unavailable/timeout) Yes (OMN-15110)

Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)

@github-actions

Copy link
Copy Markdown

✅ Architectural Review — PASSED

Errors: 0
Warnings: 0

What this checks

Rule Description
ARCH-TOPIC-001 No hardcoded Kafka topic strings in handler code
ARCH-DI-001 No event_bus=None bypass
ARCH-DI-002 No direct Handler instantiation outside workflow_runner/adapters
ARCH-DI-003 No reinvented DI containers
ARCH-TOPIC-002 contract.yaml topics follow `onex.{cmd

No architectural violations found.

Static architectural lint — no model inference (OMN-14176).

@jonahgabriel
jonahgabriel merged commit 437b34f into dev Sep 25, 2026
247 of 261 checks passed
@jonahgabriel
jonahgabriel deleted the jonah/omn-19400-fix-negative-control-marker-collision branch September 25, 2026 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hold:auto-merge Holds auto-merge (check_auto_merge_hold.py); remove to release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant