Skip to content

test(OMN-19980): a CI guard holds savings to one definition in SQL - #3373

Merged
Patel230 merged 3 commits into
devfrom
lakshman/OMN-19980-single-savings-definition
Oct 5, 2026
Merged

Patel230 merged 3 commits into
devfrom
lakshman/OMN-19980-single-savings-definition

Conversation

@Patel230

@Patel230 Patel230 commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Update 2026-10-05 08:20Z (head 17a6f93ad)

  • Head is now 17a6f93ad (was a519dbc26), pushed. Commits since the body was written: b8ee0bb7e (binds AC1 to the guard test so dod-verify has evidence) and 17a6f93ad (a merge of dev cce192a59). The "head has not been pushed" line below is out of date.
  • CI at this head, read 2026-10-05 08:16-08:17Z: 148 checks SUCCESS, 3 SKIPPED, 1 NEUTRAL, none failing; merge state CLEAN.
  • pyproject.toml at this head reads version 0.4.299; dev reads 0.4.301.
  • The verification below was run at a519dbc26 and is not re-recorded at 17a6f93ad.

Summary

Adds tests/test_single_savings_definition.py, a CI guard that keeps savings defined once, in node_metering_summary_compute, and wires it into ci.yml as a named step. It is the guard half of OMN-19980 AC1. Nothing in the runtime changes: the live savings views were already rid of the hard-coded baseline by 089, 090 and 093 (OMN-19969), so the guard proves that and stops it coming back.

What changed

  • tests/test_single_savings_definition.py (new, 66 tests, imports nothing from src). Five rules:
    • R1: a string literal feeding a baseline_model column fails ('claude-opus-4.1' AS baseline_model, COALESCE(x, 'claude-opus-4.1') AS baseline_model, any case, CRLF and tabs). Comments and strings are ignored.
    • R2: the last migration defining each of projection_delegation_savings, projection_delegation_savings_series and projection_cost_savings_overview must carry no such literal. A definition in another node folder must be clean too.
    • R3: claude-opus-4.1 may appear under src/ only in the five frozen history migrations (076, 078, 079, 083, 087). A sixth file fails, and a frozen file that no longer holds it fails, so the list only shrinks. This is the plan's wider wording (T3.3), added beside the AC and not replacing it.
    • R4: SQL multiplying a token column by a price column fails outside node_metering_summary_compute, in .sql files, SQL strings inside Python (AST) and YAML text.
    • R5: ci.yml must run the guard as a named step, not continue-on-error, not conditional.
  • .github/workflows/ci.yml: one step, "Single savings definition guard (OMN-19980)", beside the other guard steps that run a single test file.
  • Size: 944 added lines, 913 of them the one test file, most of it planted controls and the false-positive population; no runtime or migration change, so a reviewer can judge it without tracing behaviour.
  • contracts/OMN-19980.yaml: binds AC1 to uv run pytest tests/test_single_savings_definition.py -q -p no:randomly.
  • Callers of the five views, as the ticket asks the PR to list: the three views are exposures delegation.savings.v1, delegation.savings-series.v1 and cost.savings-overview.v1 in node_projection_savings/contract.yaml; projection/morning_page.py reads delegation.savings.v1; omnidash local pages bind them (overview.contracts.yaml 7 bindings, runs.contracts.yaml 1, api-keys.contracts.yaml 1). They are read, so they are not retired, and no new migration is added because nothing is left to drop.
  • A finding from the first run on the real tree: 077_create_cost_savings_overview_projection_view.sql feeds baseline_model from COALESCE(NULLIF(model_cloud_baseline, ''), 'cloud-baseline'), a placeholder string. Superseded by 089; frozen as history in the guard.

How it was verified

Base for every comparison: origin/dev 8ee3d6103. Branch head a519dbc26 (signed). Run on 2026-10-04.

  1. RED at the merge base, the way the repo-evidence gate runs a bound check (base source with the branch's test laid over it, base ci.yml): uv run pytest tests/test_single_savings_definition.py -q -p no:randomly exits 1, 1 failed, 65 passed. The one failure is test_ci_runs_this_guard_as_a_named_blocking_step (no ci.yml step at the base), not a collection error. GREEN on the branch head: exit 0, 66 passed.
  2. Planted controls on the real tree (base source plus a planted 094_planted.sql holding 'claude-opus-4.1' AS baseline_model and 095_planted_price.sql holding tokens_input * price_per_token): exit 1, 4 tests fail (R1 freeze, R2 last definition, R3 holders, R4 arithmetic). Unit-level planted controls cover each rule in both directions, including the false-positive population (tokens_to_compliance * 2, tokens_input * success_ratio, comments, strings, pricing_manifest_version).
  3. Mutation run: 15 mutations of the guard's own functions (alias regex, expression walk, comment stripping, first-vs-last definition, yaml suffix, price names, metering exemption, Python SQL strings, continue-on-error, name check, empty walker, divisor step, skip dirs, stale-entry check, cross-folder definitions). 15 killed, 0 survived; two survivors in the first run (stale-entry check, unnamed step) led to a refactor and two new tests before this count. The unmutated control passes 66.
  4. Instrument checks on the real tree: the walker sees at least 19 savings migrations and 500 or more files, the five frozen files exist and R1 flags exactly the five plus 077. R4 measured 0 findings across 4,111 Python files plus the .sql and YAML in src, with a planted-expression positive control.
  5. Related suites, dev vs branch at 8ee3d6103: tests/unit/test_omn19969_savings_series_forward_migration.py, tests/test_omn17426_savings_aggregate_views_per_tenant.py, tests/nodes/node_metering_summary_compute, tests/nodes/node_projection_metering_summary: 46 passed, 13 skipped on both.
  6. ruff check src/ tests/ benchmarks/ clean, ruff format --check clean, mypy --strict tests/test_single_savings_definition.py clean. contracts/OMN-19980.yaml validates with the pinned OCC validator ([OK] ... (ticket_contract), exit 0).
  7. Lab readback (read-only SELECT piped over ssh onex-201, nothing written, 2026-10-04 12:41Z, database omnidash_analytics):
    • Shared dev lane (omnibase-infra-postgres): none of the three view definitions contains claude-opus-4.1. projection_delegation_savings.baseline_model values: claude-opus-4-6 (1), claude-sonnet-4 (3), claude-sonnet-5-5 (2), NULL (2).
    • Lakshman's lane (omnibase-infra-lakshman-postgres): projection_delegation_savings and projection_cost_savings_overview definitions are clean; projection_delegation_savings_series definition STILL contains the literal, because that lane's database predates 093 (its clones were weeks behind). Its baseline_model values are NULL (1 row). The series view exposes no baseline_model column, so no exposure names the literal on either lane. The lane needs the 093 migration applied; that is a write and is not done here.
    • Deviation from the ticket's "after the migration on the lab dev lane": this PR adds no migration, since 090 and 093 already carry the fix, so the readback is of the already-migrated views.

Not checked: this PR's own CI run (the head has not been pushed); the deployed series view on any lane other than the two above.

Failure paths

n/a: the guard writes no receipt, verdict or report. Its own failure paths: a scan that sees no files fails (test_instrument_sees_the_corpus, mutation M10); an unparseable Python file in src raises instead of being skipped; a missing ci.yml or a missing step is reported, not passed.

Open defects

none in this change. Known and named: Python (not SQL) price arithmetic in pricing.py, cost_pricing.py, handler_llm_delegation_call.py, and the ab-compare and demo cost nodes is outside this guard's SQL scope and is asserted as a known gap in the test, not scanned. SQL built from fragments at run time is not seen.

Local gates

  • No gate was bypassed — no --no-verify, no SKIP=, no --no-gpg-sign, no core.hooksPath override. Every pre-commit and pre-push hook ran.
  • Any hook that failed was fixed at the input, not worked around.

Commit hooks: 37 passed, 0 failed, the rest skipped for no matching files.

Overlapping open PRs

Read live 2026-10-04: 12 open PRs in this repository; none changes .github/workflows/ci.yml, tests/test_single_savings_definition.py or contracts/OMN-19980.yaml (PRs 3370, 3369, 3368, 3364, 3363, 3319 and 3183 touch other contracts/ or workflow files). Overlap-Reviewed: none needed. #3368 (OMN-20007, declares metering-summary.v1) is related, not overlapping.

Not in this change

  • AC2 (no page of the six shows a savings figure from an exposure other than metering-summary.v1; the T2.4 vitest page test): omnidash page work, Codex's dashboard lane, depends on OMN-20007. The guard's rules are plain functions over a root, so a page rule can be added beside them.
  • Removing totalSavingsUsd and fixtureSavingsUsd from delegation.summary.v1: deferred by decision. It needs a DROP VIEW and recreate and a contract edit, and breaks these omnidash readers first: DelegationEvidencePage.tsx:390, DelegationMetrics2D.tsx:228, DelegationMetrics3D.tsx:93, services/delegation-api.ts:35, services/event-dash-api.ts:219. No omnimarket source reads it.
  • A new view migration: none, the live definitions are clean.
  • The vendored copies in omnibase_infra (docker/migrations/forward/nodes/node_projection_savings/), which carry the same five files with the literal; a guard there is a separate PR.
  • Applying 093 to Lakshman's lab lane.
  • Linking OMN-19977 (merged, 532ec1f53) and the parent OMN-19915 is context only.

Evidence-Ticket: OMN-19980
Evidence-Source: OCC#12795

Savings is defined once, in node_metering_summary_compute. Nothing stopped a
second definition from coming back: five applied migrations still spell
'claude-opus-4.1' AS baseline_model, and a later migration could reintroduce a
hard-coded baseline or multiply tokens by a price in SQL without any check
noticing. The live views are already clean (090, 089 and 093 replaced them), so
the guard has to prove that, not assume it.

tests/test_single_savings_definition.py fails on: a string literal feeding a
baseline_model column; the last migration defining any of the three savings
views carrying one; claude-opus-4.1 anywhere under src/ outside the five frozen
history migrations (and a frozen entry that no longer holds it, so the list can
only shrink); SQL, in .sql, Python strings and YAML, that multiplies a token
column by a price outside node_metering_summary_compute; and ci.yml not running
the guard as a named blocking step. That last assertion is what makes the bound
evidence check fail at the merge base for a real reason.

Python price arithmetic is outside the ticket's SQL scope and is asserted as a
known gap, not scanned. 077 holds a placeholder baseline ('cloud-baseline')
that the guard found on the real tree; it is frozen as history, superseded by
089.

contracts/OMN-19980.yaml binds AC1 to the guard.

Onex-Lane: omn19980-single-savings-guard
Onex-Session: d99c149d-35af-4f1e-b791-28a28187bfc4
@Patel230
Patel230 requested a review from jonahgabriel October 4, 2026 12:55
@github-actions

github-actions Bot commented Oct 4, 2026

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).

@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

✅ Hostile Reviewer — PASSED

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


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)

onexbot-occ-writer Bot added a commit to OmniNode-ai/onex_change_control that referenced this pull request Oct 4, 2026
…nimarket#3373

The declared checks were executed for real in the OmniNode-ai/omnimarket checkout at the PR head by the product-repo OCC receipt runner, and the results written append-only into this companion. Producer-minted PENDING receipts are superseded by a net-new record; no existing receipt was edited.

Ticket: OMN-16859 (AC3b)
Onex-Workflow: OCC Receipt Runner
Onex-Run: https://github.com/OmniNode-ai/omnimarket/actions/runs/37203776312
jonahgabriel pushed a commit to OmniNode-ai/onex_change_control that referenced this pull request Oct 4, 2026
)

* evidence(OMN-19980): author OCC companion for OmniNode-ai/omnimarket#3373

OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head a519dbc26d0f733d2b72ae15a4039e22234ce677.

* evidence(OMN-19980): self-bind OCC#12795 + rebind contract_sha256

* evidence(OMN-16859): executed test_passes receipts for OmniNode-ai/omnimarket#3373

The declared checks were executed for real in the OmniNode-ai/omnimarket checkout at the PR head by the product-repo OCC receipt runner, and the results written append-only into this companion. Producer-minted PENDING receipts are superseded by a net-new record; no existing receipt was edited.

Ticket: OMN-16859 (AC3b)
Onex-Workflow: OCC Receipt Runner
Onex-Run: https://github.com/OmniNode-ai/omnimarket/actions/runs/37203776312

---------

Co-authored-by: omnimarket-bot <bot@omninode.ai>
Co-authored-by: onexbot-occ-writer[bot] <307849072+onexbot-occ-writer[bot]@users.noreply.github.com>
…e to control

repo-evidence / dod-verify (jobs 111440630371, 111441105546) refused the
PR with "zero bound checks": the contract's only item was a `command`
check with no binds_ac, so the gate had nothing to re-run at the merge
base. The item is now a test_passes check of the same command, bound to
AC1, and the contract states AC1 in the ticket's own words; AC2 is page
work in omnidash and is not claimed.

The binding fails at the merge base for a real reason, not a missing
file: with 8ee3d61's ci.yml and this branch's test laid over it, the
guard's own test_ci_runs_this_guard_as_a_named_blocking_step fails (1
failed, 65 passed); at the head all 66 pass.

Onex-Lane: ci-fix-OMN-19980
Onex-Session: d99c149d-35af-4f1e-b791-28a28187bfc4
The branch was 21 commits behind dev. Bringing dev in as a signed merge,
since the branch is already pushed and must not be rewritten, so CI and the
guard run against current dev. No conflicts; the branch's own three files are
unchanged.
@Patel230
Patel230 merged commit f859c7a into dev Oct 5, 2026
163 checks passed
@Patel230
Patel230 deleted the lakshman/OMN-19980-single-savings-definition branch October 5, 2026 19:33
@Patel230

Patel230 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Close-out: OMN-19980 AC1 landed from reviewed head 17a6f93ad9592b5ef052eddb4ed694ee04883c1b as merge commit f859c7af5e1d2a4df043bbb5d5e9d873a6c1c495. Live readback at 2026-10-06 01:03 IST shows 163 checks passed, and onex_change_control#12795 is merged. The separate AC2 page work remains open in omnidash#361; this merge does not complete the ticket.

Thank you!

jonahgabriel added a commit that referenced this pull request Oct 5, 2026
Merge of dev (#3373) added 8 lines to .github/workflows/ci.yml at line 488, shifting every ci.yml row at or after line 488 by 8. This caused test_every_workflow_action_ref_resolves_to_its_pre_burndown_commit to fail in Tests (Split 2/20). Shifted the affected golden rows in tests/ci/fixtures/omn20558_workflow_action_refs.json by +8. Local run: 6 passed.
jonahgabriel added a commit that referenced this pull request Oct 5, 2026
* refactor(OMN-20604): move lane-liveness models to shared package

Moved lane-liveness wire models from node_lane_liveness_compute to omnimarket.models.liveness.
The old module path is now a re-export, keeping contract.yaml unchanged.
The handler now imports from the shared package.
This satisfies the rule that one node must not import another node's models.

* fix(OMN-20604): bind the shared-models refactor to its own evidence item

The repo-evidence gate refused omnimarket#3431 because every bound check in contracts/OMN-20604.yaml was carried unchanged from the merge base. Add AC4 and dod-omn20604-ac4-liveness-shared-models, bound to tests/unit/test_liveness_shared_models.py, which exists only at the head so its base control fails.

Onex-Lane: jobs-briefs-9143
Onex-Session: 4f0bf5fdf94549c3bcbbb606e65385f3

* fix(OMN-20604): shift golden ci.yml rows by +8 for dev merge

Merge of dev (#3373) added 8 lines to .github/workflows/ci.yml at line 488, shifting every ci.yml row at or after line 488 by 8. This caused test_every_workflow_action_ref_resolves_to_its_pre_burndown_commit to fail in Tests (Split 2/20). Shifted the affected golden rows in tests/ci/fixtures/omn20558_workflow_action_refs.json by +8. Local run: 6 passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant