Skip to content

fix(OMN-17887): declare the tenant domain in public, retire the tenant schema - #2847

Merged
jonahgabriel merged 1 commit into
devfrom
lakshman/OMN-17887-tenant-to-public
Sep 24, 2026
Merged

jonahgabriel merged 1 commit into
devfrom
lakshman/OMN-17887-tenant-to-public

Conversation

@Patel230

@Patel230 Patel230 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

OMN-17887, step 1 of 4. Operator ruling 2026-09-24: no tenant Postgres schema will be built, and the TENANT domain lives in public for good. This PR makes omnimarket declare public wherever it declared tenant.

What changed

  • Declarations: all 34 schema: tenant declarations now say schema: public, the schema the relations actually live in. That is 20 in 13 node contracts and 14 in scripts/application-relation-ownership.yaml.
  • Inventory: scripts/generate_application_relation_inventory.py maps public to TENANT and no longer knows tenant, so a stale tenant declaration is blocked as unresolvable instead of passing. The relation inventory under docs/evidence/ is regenerated:
    • 31 rows move from tenant to public and stay TENANT-domain.
    • hook_events' 2 rows move from PUBLIC to TENANT.
    • update_hook_events_updated_at, previously blocked as unresolved, is now classified.
    • Every other blocked row keeps its original reason.
  • Tests: a new test, tests/unit/scripts/test_tenant_schema_retired_omn17887.py, forbids tenant declarations, with a positive control on its YAML walker. Five tests that pinned tenant now pin public, and none was deleted. The strict-wiring test's guidance no longer says public is disallowed.

How it was verified

  • Runtime unchanged: the pinned omnibase-infra (0.38.36) already declares application.public as TENANT on all 9 profiles. _resolve_projection_database_target returns identical results before and after for all 180 targets across those profiles: same physical schema, domain and read and write bindings. A made-up schema name still raises.
  • New test: RED first (4 failed, 1 passed) against the unchanged tree, GREEN after.
  • Affected files: 121 passed after the rebase onto 1bb84db5.
  • Inventory: --check passes.
  • Full suite (-m "not kafka", serial): 23,500 passed, 19 failed, 8 errors. Those 27 are the identical set that fails on untouched dev 192b1ed8 on the same machine: pre-push hook, wire-compatibility release, pnpm, and real-Postgres fixtures. None comes from this change.
  • Why merging first is safe: omnibase_infra CI reads omnimarket contracts and this manifest at a committed pin, so this merge cannot turn infra red.

Local gates

  • Pre-commit ran on commit: 52 hooks passed, 0 failed.
  • 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.

Not in this change

  • omnibase_infra: the topology, the tenant bridge and the pin bumps are the next PR.
  • Shipped migrations: some still carry comments describing the old bridge. They stay unedited because their checksums are recorded.
  • Handler comment: handler_tenant_credentials_projection.py's comment about the bridge is left as it was. Editing it trips the projection write-path real-DB gate for a comment-only change; it goes with the infra follow-up.

Evidence-Ticket: OMN-17887
Evidence-Source: OCC#11079

…t schema

Operator ruling 2026-09-24: no tenant Postgres schema will be built; the
TENANT domain lives in public for good. Every node contract db_io
declaration and ownership-manifest entry that named schema tenant now names
public, the schema the relation actually lives in. The typed topology
already resolves application.public to TENANT on every shipped profile, so
domain, physical schema and binding are unchanged (180/180 resolver targets
identical across 9 profiles).

The relation inventory maps public to TENANT and drops tenant, so a stale
tenant declaration is blocked as unresolvable. Tests that pinned the old
name now pin public; a new test forbids tenant declarations.
@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).

@github-actions

Copy link
Copy Markdown

✅ Hostile Reviewer — PASSED

Blocking findings (critical/error): 0
Total findings: 16
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)

onexbot-occ-writer Bot added a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 24, 2026
…nimarket#2847

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/36004412875
onexbot-occ-writer Bot pushed a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 24, 2026
…2847

OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 7453e900377cd3dfe321a2f871f38087900ecb87.
onexbot-occ-writer Bot added a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 24, 2026
…nimarket#2847

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/36004523248
jonahgabriel pushed a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 24, 2026
…imarket#2847 (#11079)

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

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

* evidence(OMN-17887): self-bind OCC#11079 + rebind contract_sha256

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

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/36004523248

---------

Co-authored-by: omnimarket-bot <bot@omninode.ai>
Co-authored-by: onexbot-occ-writer[bot] <307849072+onexbot-occ-writer[bot]@users.noreply.github.com>
@jonahgabriel
jonahgabriel merged commit 3441453 into dev Sep 24, 2026
289 of 310 checks passed
@jonahgabriel
jonahgabriel deleted the lakshman/OMN-17887-tenant-to-public branch September 24, 2026 19:49
@Patel230

Copy link
Copy Markdown
Contributor Author

Close-out. Merged 2026-09-24T19:49:37Z as 3441453cde by jonahgabriel, for OMN-17887. One commit, squashed.

In short: this PR had no human review comments and no review threads. The two bot comments were the Architectural Review, which passed with 0 errors and 0 warnings, and the Hostile Reviewer, which passed with both models up: 16 findings, 0 blocking. None of the 16 reached quorum, so they were reported and not threaded, and I didn't answer them here before merge. The PR is a single commit, so nothing was changed for them. Most of them read the change as moving tenant data from a tenant schema into public. It doesn't move anything. The relations were already physically in public, and this PR changes only declarations, the inventory generator and tests, with no SQL file touched. Two points are fair and weren't acted on: a test whose name is more specific than its assertion (point 6), and omnimarket having no test of its own that nothing non-tenant is declared public (point 5). Removing an empty leftover tenant schema is separate work, in omnibase_infra#4197, which is still open. Details per point below.

Point by point

The findings are in the job log of the Hostile Reviewer run on the only head, 7453e90037 (09-24 13:17Z). qwen3-review raised 8 and qwen3-review-b raised 8, and the log lists 15 of those 16 lines. Some appear once per model, and I've grouped them. Line numbers are from 3441453cde. "The head CI" means run 36004415671, where all 20 test splits passed.

1. "Tenant data moved to public without RLS enforcement" (error from both models, one on scripts/generate_application_relation_inventory.py and one on node_delegation_routing_reducer/contract.yaml)
Nothing moved. At the time, the pinned omnibase_infra (0.38.36, uv.lock L2998-2999) mapped schema: tenant to physical public for these relations (physical_grant_schema_for_table, physical_schema_mapping.py L219-232 at v0.38.36), so they were already living in public. This PR declares where they really are. RLS lives in the node migrations, and this PR changes no .sql file: its 23 files are contracts, the ownership manifest, the generator, the inventory and tests. The domain doesn't change either. At v0.38.36, application_database.py L156-157 declares both public and tenant as TENANT in the application database, so any check keyed on TENANT still applies. The description has one more piece of evidence: _resolve_projection_database_target returned identical results for all 180 targets before and after. That was measured locally before merge, and I didn't re-run it for this close-out. The enforcement side also keys RLS on domain, not schema: since omnibase_infra#4079, every TENANT relation goes through _validate_tenant_state (application_database_domain_enforcement.py L1804-1818 at 21d5d62161).

2. "DOMAIN_BY_SCHEMA conflates physical schema with logical domain" / "collapses PUBLIC and TENANT" / "a single-value dict loses extensibility" (warnings from both models, and an info from qwen3-review-b)
public maps to TENANT (generate_application_relation_inventory.py L42-52) because that's what the typed topology says for the application database, which is the only database this inventory covers. Its database_ref is application. PUBLIC was the generator's own label, not a domain in the topology. Before this PR, the only rows it produced were the 2 for hook_events (point 3). The internal relations that are physically in public stay declared at their logical schema, omninode_internal, so this map never sees them. Each schema mapping to one domain matches the topology, where every schema has exactly one domain. The old dict worked the same way. Not changed.

3. "The update_hook_events_updated_at blocked entry was removed without justification" (warning from qwen3-review, info from qwen3-review-b)
It wasn't removed. It's now classified, and the generator line that does it is _target_schema L367-368. The function has no table dependencies, so its schema comes from what its owning node declares. node_hook_event_capture declares hook_events at public. The old code recognised only tenant and omninode_internal there, so the function fell through to unresolved and was blocked with "schema 'unresolved' does not resolve through deployment topology". Now it resolves to public/TENANT, the same as the table it serves. hook_events is tenant-attributed: tenant_id TEXT NOT NULL and its idempotency key is (tenant_id, event_sha) (node_hook_event_capture/migrations/0001_create_hook_events.sql L16, L57). I compared the inventory before and after. blocked_relations went from 12 to 11, and this is the only entry that left. The other 11 keep their original reasons. The description says the same thing under "Inventory". test_checked_in_inventory_is_source_derived_and_current passed in the head CI (split 19).

4. "No migration moves the existing tables from tenant to public" (warning from both models)
None is needed, because they're already in public (point 1). What's left is removing an empty tenant schema wherever a lane's bootstrap created one. That's open in omnibase_infra#4197: a node migration that drops tenant only if it's empty and owned.

5. "No test shows that no PUBLIC-domain relation is now misclassified" (info, qwen3-review)
Partly fair: omnimarket has no test of its own for that. The only PUBLIC rows before were hook_events' 2, which are tenant-attributed (point 3). The check that refuses a non-TENANT relation declared in public belongs to omnibase_infra. Since omnibase_infra#4079, a relation whose declared domain differs from the TENANT domain of public is refused there (_schema_owner_for, L1346-1350; test_red_control_public_application_table, test_application_database_domain_enforcement.py L1004, both at 21d5d62161). omnibase_infra's CI reads these contracts at a pinned omnimarket commit. At 21d5d62161 that pin is d0cf5753e4, which contains 3441453cde. Not changed here.

6. "test_savings_estimates_stays_tenant_domain is now misleading" (info, qwen3-review)
Partly fair. The test name talks about the domain, but the assertion checks the schema: == "public" (test_savings_correlation_finalizations_ownership_omn16770.py L105-114, with a comment at L113 saying public is the TENANT domain's schema). Its meaning is the same as before: it compared against "tenant", which was also a schema. It passed in the head CI (split 19). Asserting the domain through the generator's map would say it directly. I didn't do that here.

7. "The ownership YAML comment refers to a defect that doesn't exist" (info, qwen3-review)
It exists. The comment (scripts/application-relation-ownership.yaml L398-411) describes the OMN-16770 problem: an idempotency anti-join reading a TENANT relation under row-level security it can't bind. Its seam is _assert_idempotency_read_is_scoped, in omnibase_infra's node_savings_estimation_compute/handlers/handler_savings_correlation.py L217 at v0.38.36. This PR only reworded the line from "a schema: tenant declaration" to "a TENANT-domain declaration" (L410), because tenant is no longer a schema anyone can declare.

8. Style in the new test: loading the script with importlib, a walrus in a dict comprehension, and the comment style (info from qwen3-review; a warning and a hint from qwen3-review-b)
scripts/ isn't a package (it has no __init__.py), so the generator can't be imported by name. The existing inventory test loads it the same way (tests/unit/scripts/test_application_relation_inventory.py L8, L22-27), and 14 of the 36 files in tests/unit/scripts/ use spec_from_file_location. The walrus (test_tenant_schema_retired_omn17887.py L83-87) keeps the comprehension to one parse per contract. The comment style is a matter of taste. All five tests in the file passed in the head CI (split 19), and the description records that they went RED against the unchanged tree first (4 failed, 1 passed). Not changed.

The two issue comments

  • The Architectural Review passed with 0 errors and 0 warnings, so there was nothing to answer.
  • The Hostile Reviewer summary (09-24 13:17Z) covers the findings answered above.

Thank you!

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.

2 participants