From e406a006f7334c86c1ff84b3256c0a32d982ead4 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:50:14 +0000 Subject: [PATCH 01/15] =?UTF-8?q?T071:=2013.2=20and=2013.3=20=E2=80=94=20l?= =?UTF-8?q?oad=5Fsettings=20refuses=20a=20non-PostgreSQL=20DSN=20and=20a?= =?UTF-8?q?=20collapsed=20DSN=20pair=20(plan=20034)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Realizes #1144 13.2 and 13.3, falsifier F13.1's `load_settings` block. - 13.2: `_refuse_non_postgresql_dsn` refuses either DSN (`OPENDOX_DATABASE_URL` or `OPENDOX_MIGRATION_DATABASE_URL`) whose URI scheme is not `postgresql://` or `postgres://`, naming the setting and the dialect kept. The keyword/value conninfo form (`host=h dbname=d …`) names no dialect at all and is unaffected — that syntax is libpq's own grammar, and no other driver reads it. - 13.3: `OPENDOX_MIGRATION_DATABASE_URL` stops being optional in `load_settings` (the `Setting` row's `required` flag, `_require` in place of `_optional`, and `RuntimeSettings.migration_database_url`'s type). A new `_refuse_the_same_dsn_in_both_settings` refuses the two DSNs being the exact same STRING, naming `OPENDOX_MIGRATION_DATABASE_URL`, once they are already known to agree on where they land (`_refuse_two_dsns_that_select_different_schemas`, unchanged, now called first): two DIFFERENT secrets for one role still pass, as the existing "single-role install" case documents. - Explicitly NOT in this task: 13.4-13.6 (`OPENDOX_INSTALL_MODE`, T070). Nothing here reads or names that setting, and `load_settings`'s only new required input is the migration DSN itself. Every existing call site that built an environment without `OPENDOX_MIGRATION_DATABASE_URL` needed one once it became required: `tests_runtime/conftest.py` gains a `migration_dsn` fixture (a `postgres_dsn` distinguished by a URI fragment, invisible to every DSN reader this module has); `test_api_endpoints.py`, `test_migrations_apply.py`, `test_runtime_cli.py` and `test_runtime_surface.py` thread it or a literal peer through. `test_two_dsns_that_select_different_schemas_are_refused`'s "a migration DSN that is simply absent" case is rewritten from accepted to refused, which is the behavior 13.3 changes. Two new tests (`test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept`, `test_the_same_dsn_in_both_settings_is_refused_naming_the_migration_one`) cover the two new refusals directly. Measured locally against this change (own Postgres container, bridge IP — this sandbox's host-mapped loopback ports are unreachable): `python -m pytest -q` reports 2469 passed, 11 skipped, 1 failed — the one failure is `tests/test_model_provider_broker.py::test_the_broker_child_inherits_no_ credential_shaped_environment`, already red against unmodified `main` (2d116415) in the same environment (an `LC_CTYPE` ambient in this sandbox, unrelated to runtime/config.py). Against `main`'s own reading (2479 selected / 2468 passed / 11 skipped, matching this repo's last recorded CI triple), this change is +2/+2/+0 for the two new tests — `validate.yml`'s `Pin the triple` floors (`MIN_SELECTED=2476`, `MIN_PASSED=2465`, `EXPECT_SKIPPED=11`) permit the rise unchanged. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 104 ++++++++++++++++++--- tests_runtime/conftest.py | 31 ++++++- tests_runtime/test_api_endpoints.py | 18 +++- tests_runtime/test_migrations_apply.py | 7 ++ tests_runtime/test_runtime_cli.py | 124 ++++++++++++++++++++++++- tests_runtime/test_runtime_surface.py | 2 + 6 files changed, 264 insertions(+), 22 deletions(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 5a832e12..046a46ab 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -81,7 +81,7 @@ class Setting: "it must not be the identity migrations run as", ), Setting( - PREFIX + "MIGRATION_DATABASE_URL", None, False, True, + PREFIX + "MIGRATION_DATABASE_URL", None, True, True, "the PRIVILEGED DSN ordered-SQL migrations are applied with, used by " "`opendox runtime migrate` alone and never by the served application", ), @@ -180,10 +180,14 @@ class RuntimeSettings: """The resolved configuration of one runtime process. Construct with :func:`load_settings`; the fields are in `SETTINGS` order. + + `migration_database_url` is never `None` from either loader (plan 034, + 13.3): `load_settings` requires it exactly as it requires `database_url`, + and `load_migration_settings` already refused to load without one. """ database_url: str - migration_database_url: str | None + migration_database_url: str oidc_issuer: str oidc_audience: str oidc_jwks_url: str | None @@ -1288,6 +1292,63 @@ def effective_schema(dsn: str) -> str | None: return user_named_by(dsn) +#: THE ONLY DIALECT THIS RUNTIME KEEPS (plan 034, 13.2). `psycopg` is the one +#: driver `runtime` depends on and it speaks PostgreSQL alone, but a DSN is a +#: string and nothing stopped an operator writing `sqlite:///…` into either +#: setting and discovering the mismatch however far the code got before the +#: driver refused it. RULING Q1 keeps this database DOCUMENT-FREE, which is +#: why a second dialect is refused HERE rather than supported: it would double +#: every migration and every schema test forever, for a database that holds no +#: document. `postgres://` is accepted beside `postgresql://` because libpq +#: treats the two as one scheme. +POSTGRESQL_SCHEMES = frozenset({"postgresql", "postgres"}) + + +def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: + """`name`'s DSN is refused unless it selects a PostgreSQL scheme. + + A DSN in the keyword/value form (`host=h dbname=d …`) names NO DIALECT AT + ALL — that syntax is libpq's own conninfo grammar, and no other driver + reads it — so only the URI form is checked: `urlsplit` reports an EMPTY + scheme for the keyword/value form (there is no `://` to split on), and an + empty scheme is read as "says nothing" here, exactly as `schema_selected_by` + reads a DSN that names no schema as `None` rather than as a refusal. + """ + scheme = urllib.parse.urlsplit(dsn).scheme + if scheme and scheme not in POSTGRESQL_SCHEMES: + raise ConfigurationError( + f"{name} names the {scheme!r} dialect. PostgreSQL " + "(`postgresql://` or `postgres://`) is the only dialect this " + "runtime keeps: a second one would double every migration and " + "every schema test forever, for a database that holds no " + "document (RULING Q1)") + + +def _refuse_the_same_dsn_in_both_settings(served: str, migration: str) -> None: + """One credential pasted into both settings is refused (plan 034, 13.3). + + `OPENDOX_DATABASE_URL` is the least-privileged identity the API serves + with; `OPENDOX_MIGRATION_DATABASE_URL` is the privileged one ordered-SQL + migrations run as — the whole point of keeping two settings. A + single-user install is not a reason to collapse them into one: this is + the two settings simply BEING each other, which is different from + `_refuse_two_dsns_that_select_different_schemas` below, where they + DISAGREE about where they land. It is different too from the accepted + "single-role install" (`test_a_dsn_that_names_no_database_still_reaches_ + one`): two DSNs for the same ROLE with two DIFFERENT secrets are two + credentials, not one pasted twice, and this checks the value actually + given, not the identity it happens to resolve to. + """ + if served == migration: + raise ConfigurationError( + f"{PREFIX}MIGRATION_DATABASE_URL is the same value as " + f"{PREFIX}DATABASE_URL. The identity migrations run as must not " + "also be the identity the API serves with; give the migration " + "credential its own DSN, even where both reach the same " + "database (the values are not repeated: a DSN carries a " + "password)") + + def _refuse_two_dsns_that_select_different_schemas( served: str, migration: str | None) -> None: """Both DSNs must land in one schema, or neither answer means anything. @@ -1400,21 +1461,42 @@ def load_settings(env: Mapping[str, str] | None = None) -> RuntimeSettings: accept `HS256` would verify a token signed with the public key anybody can fetch from the broker's JWKS, and discovering that on the first request means it is already serving. + + BOTH DSNs ARE REQUIRED (plan 034, 13.3): `OPENDOX_MIGRATION_DATABASE_URL` + used to default to `None`, so the collapsed, single-role shape this + refuses (below) was reachable only by accident, through a caller who + happened to set both to the same value — an install that left the + migration credential unset was never asked the question at all. Naming it + explicitly, even at the same database a served DSN already names, is what + keeps the two identities two DECISIONS rather than one remembered twice. """ env = os.environ if env is None else env algorithms = _algorithms(env) - # AND THE TWO DSNs LAND IN ONE SCHEMA. See - # `_refuse_two_dsns_that_select_different_schemas`: this is the half of - # that invariant a string can answer, and it is asked here because this is - # the one loader that holds BOTH values. - _refuse_two_dsns_that_select_different_schemas( - _require(env, _by_name(PREFIX + "DATABASE_URL")), - _optional(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL"))) + served = _require(env, _by_name(PREFIX + "DATABASE_URL")) + migration = _require(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")) + # THE DIALECT FIRST: a scheme this module cannot parse as PostgreSQL is not + # yet a DSN worth comparing at all. + _refuse_non_postgresql_dsn(PREFIX + "DATABASE_URL", served) + _refuse_non_postgresql_dsn(PREFIX + "MIGRATION_DATABASE_URL", migration) + # THEN WHETHER THEY DISAGREE. See `_refuse_two_dsns_that_select_different_ + # schemas`: this is the half of that invariant a string can answer, and it + # is asked here because this is the one loader that holds BOTH values. A + # DSN compared against ITSELF can never disagree, so this step passes + # silently on exactly the pair the next one exists to catch. + _refuse_two_dsns_that_select_different_schemas(served, migration) + # AND, LAST, WHETHER THEY ARE SIMPLY EACH OTHER. Two DSNs that agree on + # where they land are ordinarily two credentials for the one database + # (`test_a_dsn_that_names_no_database_still_reaches_one`'s "single-role + # install" is exactly that, two DIFFERENT secrets for one role) — but + # agreement bought by pasting the SAME value into both settings is not a + # second decision at all, and this is the check the one before it cannot + # make. + _refuse_the_same_dsn_in_both_settings(served, migration) return RuntimeSettings( - database_url=_require(env, _by_name(PREFIX + "DATABASE_URL")), - migration_database_url=_optional(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")), + database_url=served, + migration_database_url=migration, oidc_issuer=_broker_url(env, _by_name(PREFIX + "OIDC_ISSUER"), required=True, is_a_base_url=True) or "", oidc_audience=_require(env, _by_name(PREFIX + "OIDC_AUDIENCE")), diff --git a/tests_runtime/conftest.py b/tests_runtime/conftest.py index 2c6e0b7b..93f53a4d 100644 --- a/tests_runtime/conftest.py +++ b/tests_runtime/conftest.py @@ -219,6 +219,29 @@ def postgres_dsn() -> str: return dsn +@pytest.fixture(scope="session") +def migration_dsn(postgres_dsn: str) -> str: + """A DSN for `OPENDOX_MIGRATION_DATABASE_URL`, distinct from `postgres_dsn`. + + Plan 034, 13.3: `load_settings` now REQUIRES this setting and refuses a + value that is simply `postgres_dsn` repeated (13.3's own collapse + refusal), so every fixture built from the pair below needs a second, + genuinely different string — not a second database. None of this + module's fixtures apply migrations through this identity: `database` + applies them directly with `MigrationRunner`, and the served app under + test is never asked to `migrate`. So `load_settings` is the only reader + that cares about this value at all, and what it asks of a DSN is a + PostgreSQL scheme (13.2), a STRING distinct from `postgres_dsn` (13.3), + and a database and schema that agree with it + (`_refuse_two_dsns_that_select_different_schemas`). A URI FRAGMENT is + invisible to `database_named_by`, `user_named_by` and `schema_selected_by` + alike — none of the three inspects `urlsplit(...).fragment` — so + appending one changes the STRING without moving the identity those + functions compare. + """ + return postgres_dsn + "#opendox-test-migration-identity" + + @pytest.fixture def database(postgres_dsn: str) -> Iterator[object]: """A `Database` on a throwaway schema, with the migrations already applied.""" @@ -357,7 +380,7 @@ def verifier(jwks_path: str): @pytest.fixture() -def client(database, postgres_dsn: str, verifier): +def client(database, postgres_dsn: str, migration_dsn: str, verifier): """A `TestClient` over the REAL application, on this test's own schema. The application is given its OWN `Database` on the same schema rather than @@ -376,6 +399,7 @@ def client(database, postgres_dsn: str, verifier): settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -399,8 +423,8 @@ def project_repository_root(tmp_path): @pytest.fixture() -def client_with_repositories(database, postgres_dsn: str, verifier, - project_repository_root): +def client_with_repositories(database, postgres_dsn: str, migration_dsn: str, + verifier, project_repository_root): """`client`, with the repository root pointed at this test's own directory.""" fastapi_testclient = _import_fastapi_testclient() from opendox.runtime.app import create_app @@ -409,6 +433,7 @@ def client_with_repositories(database, postgres_dsn: str, verifier, settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PROJECT_REPOSITORY_ROOT": str(project_repository_root), diff --git a/tests_runtime/test_api_endpoints.py b/tests_runtime/test_api_endpoints.py index 21897203..84caccfe 100644 --- a/tests_runtime/test_api_endpoints.py +++ b/tests_runtime/test_api_endpoints.py @@ -425,7 +425,7 @@ def test_the_unauthenticated_surface_is_exactly_the_two_probes( def test_the_schema_viewers_appear_only_when_the_install_says_so( - database, postgres_dsn: str, verifier) -> None: + database, postgres_dsn: str, migration_dsn: str, verifier) -> None: from fastapi.testclient import TestClient from opendox.runtime.app import create_app @@ -435,6 +435,7 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PUBLISH_OPENAPI": "true", @@ -448,7 +449,7 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( def test_readiness_refuses_an_unmigrated_database_by_name( - postgres_dsn: str, verifier) -> None: + postgres_dsn: str, migration_dsn: str, verifier) -> None: """`select 1` succeeds against a schema with no tables in it at all. Readiness without a migration check therefore turns a fresh install READY @@ -471,6 +472,7 @@ def test_readiness_refuses_an_unmigrated_database_by_name( try: settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -588,7 +590,8 @@ def test_draft_pagination_returns_the_principals_drafts_not_an_empty_page( def test_readiness_refuses_a_database_whose_migration_file_has_changed( - database, postgres_dsn: str, verifier, tmp_path) -> None: + database, postgres_dsn: str, migration_dsn: str, verifier, + tmp_path) -> None: """Nothing pending is not the same as matching this tree.""" import shutil @@ -609,6 +612,7 @@ def test_readiness_refuses_a_database_whose_migration_file_has_changed( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(tmp_path), @@ -852,7 +856,8 @@ def test_a_hidden_user_and_a_missing_one_answer_byte_for_byte_the_same( def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( - database, postgres_dsn: str, verifier, tmp_path) -> None: + database, postgres_dsn: str, migration_dsn: str, verifier, + tmp_path) -> None: """An EMPTY directory made `plan()` and `drift()` both empty. `discover()` returns `[]` for a directory that exists and holds no @@ -874,6 +879,7 @@ def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( empty.mkdir() settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(empty), @@ -1552,7 +1558,8 @@ def transaction(): def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( - database, postgres_dsn: str, rsa_key_pair, tmp_path: Path) -> None: + database, postgres_dsn: str, migration_dsn: str, rsa_key_pair, + tmp_path: Path) -> None: """The end of A25-2, measured where it was reported: at the HTTP boundary. A realm publishing an RSA and an EC signing key — Keycloak, the moment a @@ -1593,6 +1600,7 @@ def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) diff --git a/tests_runtime/test_migrations_apply.py b/tests_runtime/test_migrations_apply.py index 2afc0fe1..1b657628 100644 --- a/tests_runtime/test_migrations_apply.py +++ b/tests_runtime/test_migrations_apply.py @@ -345,6 +345,11 @@ def test_status_reports_a_reachable_database_and_its_applied_migrations( # libpq startup parameter `Database(schema=…)` sets for the harness. scoped = (f"{postgres_dsn}?options=-c%20search_path%3D{database.schema}") monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) + # A DISTINCT STRING (13.3's collapse refusal) THAT SELECTS THE SAME SCHEMA + # (`_refuse_two_dsns_that_select_different_schemas`): a URI FRAGMENT moves + # neither, since neither reader inspects one. + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker.test/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) @@ -720,6 +725,8 @@ def test_status_calls_an_unmigrated_database_unhealthy_and_blames_the_tree( conn.execute(f"create schema {schema}") try: monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 4a6d9d13..363caced 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -211,6 +211,8 @@ def test_init_creates_the_project_repository_root_and_touches_no_database( root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -362,6 +364,8 @@ def test_status_reports_every_declared_setting_and_none_as_null( from opendox.runtime.config import SETTING_NAMES monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://u:pw@127.0.0.1:1/x") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://m:pw@127.0.0.1:1/x") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "RUNTIME_PG_ROLE", "opendox_runtime") @@ -432,6 +436,8 @@ def test_init_refuses_a_repository_root_that_is_not_a_directory( root.write_text("not a directory", encoding="utf-8") monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -496,6 +502,8 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -541,6 +549,8 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -624,6 +634,8 @@ def __exit__(self, *exc: object) -> None: db_stub.Database = _Exploding # type: ignore[attr-defined] monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", dsn) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@db.internal:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -677,6 +689,8 @@ def __init__(self, config: object) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody:hunter2@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -794,6 +808,8 @@ def connection(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime:hunter2@127.0.0.1:5432/opendox") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", @@ -862,6 +878,8 @@ def transaction(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime@127.0.0.1:5432/opendox") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(tmp_path)) @@ -1148,6 +1166,7 @@ def test_no_broker_url_this_runtime_prints_can_carry_a_credential() -> None: redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ("OIDC_ISSUER", "OIDC_JWKS_URL"): env = dict(base, **{PREFIX + "OIDC_ISSUER": "https://broker/realms/x"}) @@ -1195,6 +1214,7 @@ def test_a_credential_in_a_broker_urls_query_is_refused_like_one_in_its_userinfo redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} for name, value, why in ( @@ -1243,6 +1263,7 @@ def test_the_broker_url_must_be_https_because_it_is_the_trust_anchor() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker/realms/x", "http://localhost:8080/realms/x", "http://127.0.0.1:8080/realms/x", "http://[::1]:8080/x"): @@ -1424,6 +1445,7 @@ def test_a_broker_url_that_names_no_host_is_refused_at_the_door() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https:///realms/x", "https://", "https:///", "broker/realms/x", "https://user:hunter2@/realms/x"): @@ -1509,6 +1531,7 @@ def test_a_broker_url_whose_port_is_not_a_number_is_refused_at_the_door( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker:not-a-port/realm", "https://broker:99999/realm", @@ -1554,6 +1577,7 @@ def test_the_issuer_carries_no_query_or_fragment_because_paths_are_appended( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer, component in (("https://broker/realms/x?tenant=a", "query"), ("https://broker/realms/x#frag", "fragment"), @@ -1599,6 +1623,7 @@ def test_the_loopback_exception_is_for_http_and_not_for_every_other_scheme( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("ftp://localhost/realms/x", "file://127.0.0.1/realms/x", "ws://localhost:8080/realms/x", "ftp://[::1]/realms/x"): @@ -1910,6 +1935,7 @@ def test_a_malformed_broker_url_never_prints_its_own_password() -> None: issuer = "https://svc:hunter2@broker\u2100evil.example/realms/x" env = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": issuer} @@ -2126,9 +2152,16 @@ def test_two_dsns_that_select_different_schemas_are_refused() -> None: PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db"}) - # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is the documented single-role - # deployment, not a mismatch. - assert load_settings({**base, PREFIX + "DATABASE_URL": served}) + # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is refused NOW (plan 034, + # 13.3), where it used to be accepted as "the documented single-role + # deployment, not a mismatch": that reading is exactly what `load_settings` + # reading the setting as OPTIONAL bought, and the setting stopped being + # optional. A single-role install still names both DSNs, distinctly — + # `test_a_dsn_that_names_no_database_still_reaches_one`'s "two different + # secrets for one role" is what single-role now looks like. + with pytest.raises(ConfigurationError) as absent: + load_settings({**base, PREFIX + "DATABASE_URL": served}) + assert PREFIX + "MIGRATION_DATABASE_URL" in str(absent.value) # THE OTHER DIRECTION IS REFUSED TOO: a served DSN that names no schema # beside a migration DSN that names one is the same split, and the @@ -2140,6 +2173,86 @@ def test_two_dsns_that_select_different_schemas_are_refused() -> None: assert "the connection default" in str(either_way.value) +def test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept() -> None: + """13.2: a second dialect is refused, not supported. + + RULING Q1 keeps this database DOCUMENT-FREE, so a second dialect would + double every migration and every schema test forever for a database that + holds nothing. `_refuse_non_postgresql_dsn` asks it of both DSNs + `load_settings` holds, before either reaches the checks above that + compare them. + """ + from opendox.runtime.config import ConfigurationError, load_settings + + base = {PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox"} + with pytest.raises(ConfigurationError) as served: + load_settings({**base, + PREFIX + "DATABASE_URL": "sqlite:///x.db", + PREFIX + "MIGRATION_DATABASE_URL": + "postgresql://m:p@h/db"}) + assert "postgres" in str(served.value).lower() + assert PREFIX + "DATABASE_URL" in str(served.value) + + with pytest.raises(ConfigurationError) as migration: + load_settings({**base, + PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "mysql://m:p@h/db"}) + assert "postgres" in str(migration.value).lower() + assert PREFIX + "MIGRATION_DATABASE_URL" in str(migration.value) + + # `postgres://` IS THE OTHER SPELLING LIBPQ ACCEPTS, not a second dialect. + assert load_settings({**base, + PREFIX + "DATABASE_URL": "postgres://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": + "postgres://m:p@h/db"}) + + # AND THE KEYWORD/VALUE FORM NAMES NO DIALECT AT ALL, so it is not refused + # here: libpq's own conninfo grammar reaches no other driver, and this + # module already reads an empty scheme as "says nothing" the way + # `schema_selected_by` does for a DSN that names no schema. + assert load_settings({**base, + PREFIX + "DATABASE_URL": "host=h dbname=db", + PREFIX + "MIGRATION_DATABASE_URL": + "host=h dbname=db user=m"}) + + +def test_the_same_dsn_in_both_settings_is_refused_naming_the_migration_one( +) -> None: + """13.3: one credential pasted into both settings is refused. + + `_refuse_the_same_dsn_in_both_settings` is asked only once the two DSNs + are known to AGREE on where they land + (`_refuse_two_dsns_that_select_different_schemas`, above it): agreement + bought by two DIFFERENT secrets for the one role is the accepted + single-role shape + (`test_a_dsn_that_names_no_database_still_reaches_one`'s last case); + agreement bought by writing the SAME value into both settings is this + refusal instead. + """ + from opendox.runtime.config import ConfigurationError, load_settings + + base = {PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox"} + one = "postgresql://one:hunter2@h/opendox" + with pytest.raises(ConfigurationError) as collapsed: + load_settings({**base, + PREFIX + "DATABASE_URL": one, + PREFIX + "MIGRATION_DATABASE_URL": one}) + message = str(collapsed.value) + assert PREFIX + "MIGRATION_DATABASE_URL" in message + assert PREFIX + "DATABASE_URL" in message + # THE VALUE IS NOT REPEATED: a DSN carries a password. + assert "hunter2" not in message and "one:" not in message + + # DIFFERENT STRINGS THAT STILL AGREE are NOT this refusal, whether the + # difference is the secret alone (single-role) or the whole identity. + assert load_settings({**base, + PREFIX + "DATABASE_URL": "postgresql://a:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": + "postgresql://b:p@h/db"}) + + def test_two_dsns_naming_different_databases_are_refused_too() -> None: """The schema comparison means nothing across two databases. @@ -2348,6 +2461,7 @@ def test_a_credential_shaped_parameter_name_is_a_WORD_and_not_a_substring() -> N assert redacted_url("https://broker/certs?token=x" ) == "https://broker/certs?token=" base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox"} settings = load_settings({**base, @@ -2548,6 +2662,8 @@ def test_init_creates_the_repository_root_private_whatever_the_umask_is( root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -2584,6 +2700,8 @@ def test_init_reports_an_existing_root_s_mode_and_does_not_change_it( os.chmod(root, 0o755) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) diff --git a/tests_runtime/test_runtime_surface.py b/tests_runtime/test_runtime_surface.py index ff6a73a3..b36c6203 100644 --- a/tests_runtime/test_runtime_surface.py +++ b/tests_runtime/test_runtime_surface.py @@ -258,6 +258,7 @@ def test_only_asymmetric_algorithms_can_be_configured_and_they_keep_pyjwts_spell ) base = {PREFIX + "DATABASE_URL": "postgresql://x/y", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m@x/y", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ASYMMETRIC_ALGORITHMS: @@ -318,6 +319,7 @@ def test_every_integer_setting_is_bounded_above_as_well_as_below() -> None: ) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} From 91f797329ce81fa325f3c9b24512f209478316de Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 19:04:30 +0000 Subject: [PATCH 02/15] Fix round: _refuse_non_postgresql_dsn never raises a bare ValueError (Copilot review of this PR) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `urlsplit` itself raises for a DSN it cannot parse — MEASURED, ValueError("Invalid IPv6 URL") for an unbracketed IPv6 host, which tests_runtime/conftest.py's own postgres_dsn docstring names as "the ordinary way to mis-set this variable". `_refuse_non_postgresql_dsn` called `urlsplit(dsn).scheme` unguarded, so that ValueError escaped load_settings as a bare exception instead of the promised ConfigurationError — the CLI's boundary catches only ConfigurationError, so a malformed OPENDOX_DATABASE_URL or OPENDOX_MIGRATION_DATABASE_URL would have printed a traceback instead of a redacted refusal. Wrapped the same way _split_url already wraps it for the broker settings (Copilot review of openDox-code#25, round 24), with DSN-appropriate wording rather than reused verbatim ("set it to the broker endpoint" does not fit a database DSN). New test test_an_unparseable_dsn_is_refused_and_never_raises_a_bare_valueerror proves both DSNs are covered and that the value is never repeated in the message. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 17 ++++++++++++- tests_runtime/test_runtime_cli.py | 40 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 046a46ab..ecc53d65 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -1313,8 +1313,23 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: scheme for the keyword/value form (there is no `://` to split on), and an empty scheme is read as "says nothing" here, exactly as `schema_selected_by` reads a DSN that names no schema as `None` rather than as a refusal. + + `urlsplit` ITSELF RAISES for a DSN it cannot parse — MEASURED, + `ValueError("Invalid IPv6 URL")` for an unbracketed IPv6 host, which + `tests_runtime/conftest.py`'s own `postgres_dsn` docstring names as "the + ordinary way to mis-set this variable". `_split_url` exists for exactly + this shape in the broker settings (Copilot review of openDox-code#25, + round 24); this is its DSN-flavoured twin; a bad `OPENDOX_DATABASE_URL` + is not "set it to the broker endpoint", so it is not reused verbatim. """ - scheme = urllib.parse.urlsplit(dsn).scheme + try: + scheme = urllib.parse.urlsplit(dsn).scheme + except ValueError as exc: + raise ConfigurationError( + f"{name} is not a DSN this runtime can parse " + f"({type(exc).__name__}); the value is not repeated here, " + "because a DSN this runtime cannot parse can still carry a " + "password") from None if scheme and scheme not in POSTGRESQL_SCHEMES: raise ConfigurationError( f"{name} names the {scheme!r} dialect. PostgreSQL " diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 363caced..2c3c97eb 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -2217,6 +2217,46 @@ def test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept() -> None: "host=h dbname=db user=m"}) +def test_an_unparseable_dsn_is_refused_and_never_raises_a_bare_valueerror( +) -> None: + """`urlsplit` itself raises for a DSN it cannot parse, and this module's + whole contract is that a bad variable produces a named + `ConfigurationError`, never a bare exception the CLI's boundary does not + catch (Copilot review of this PR). + + MEASURED: `urllib.parse.urlsplit("postgresql://u:p@[::1/db")` raises + `ValueError("Invalid IPv6 URL")` — an unbracketed IPv6 host, which + `tests_runtime/conftest.py`'s own `postgres_dsn` docstring names as "the + ordinary way to mis-set this variable". `_split_url` exists for exactly + this shape in the broker settings (Copilot review of openDox-code#25, + round 24); `_refuse_non_postgresql_dsn` is its own boundary for the two + DSNs, asked of both. + """ + from opendox.runtime.config import ConfigurationError, load_settings + + base = {PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox"} + broken = "postgresql://opendox:hunter2@[::1/opendox" + + with pytest.raises(ConfigurationError) as served: + load_settings({**base, + PREFIX + "DATABASE_URL": broken, + PREFIX + "MIGRATION_DATABASE_URL": + "postgresql://m:p@h/db"}) + message = str(served.value) + assert PREFIX + "DATABASE_URL" in message + assert "ValueError" in message + # THE VALUE IS NOT REPEATED: a DSN this runtime cannot parse can still + # carry a password. + assert "hunter2" not in message and "opendox:" not in message + + with pytest.raises(ConfigurationError) as migration: + load_settings({**base, + PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": broken}) + assert PREFIX + "MIGRATION_DATABASE_URL" in str(migration.value) + + def test_the_same_dsn_in_both_settings_is_refused_naming_the_migration_one( ) -> None: """13.3: one credential pasted into both settings is refused. From b5296f91c5df003964bf5c8f0126f6005da1fbbd Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 22:45:45 +0000 Subject: [PATCH 03/15] Rework #60 to Brett's ruling: OPENDOX_MIGRATION_DATABASE_URL required only for migrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brett ruled on the held conflict (openxFactory#656, on the claim thread for plan 034's T071, 2026-09-28), choosing "Required only for migrate (Recommended)" over making the setting required everywhere: - OPENDOX_MIGRATION_DATABASE_URL goes back to OPTIONAL in `load_settings` (the `Setting` row's `required` flag, `RuntimeSettings.migration_ database_url`'s type back to `str | None`, `_optional` in place of `_require`). `load_migration_settings` is unaffected either way — it already independently required one, for `migrate`/`reset` alone. - Both refusals from the previous commits stay, and are now no-ops on an ABSENT migration DSN rather than being unreachable: `_refuse_non_ postgresql_dsn` and `_refuse_the_same_dsn_in_both_settings` each return early when the migration value is falsy, exactly the way `_refuse_two_ dsns_that_select_different_schemas` already treated "nothing to compare" as nothing to fault. When BOTH are given, every check still runs, in the same order as before (dialect, then schema-mismatch, then collapse). It is never defaulted from OPENDOX_DATABASE_URL. - This matches #1144 13.3's own text and `deploy/compose/docker-compose. yaml`'s separation (the `opendox` service never gets a migration DSN; `docs/runtime.md` § 3 never lists it as required) — neither file needed a change; both already said the now-ruled behavior. The plan's "stops being optional" line is a holder-side correction, not part of this PR, and #1144's own wording is unchanged. Reverted the 27-call-site ripple the `required` flip had forced, now that it is not needed: `tests_runtime/conftest.py`'s `migration_dsn` fixture is gone; `test_api_endpoints.py`, `test_migrations_apply.py`, `test_runtime_ cli.py` and `test_runtime_surface.py` are back to threading only the served DSN through every call site that does not itself test the migration path. All four files after conftest.py are byte-for-byte `main` again. `test_two_dsns_that_select_different_schemas_are_refused`'s "absent migration" case is back to ACCEPTED (with a note on why it was briefly the opposite), which is what the setting being optional again means for that test. Added three tests showing the ruled behavior, at the CLI dispatch level rather than only `load_settings` directly, next to the existing `migrate` counterpart: - `test_serve_and_status_load_with_no_migration_dsn_configured`: `status` reports no configuration refusal and `settings[…MIGRATION_DATABASE_URL] ` as `null` with only the served DSN set; `serve` starts (`ok: true`) the same way. - `test_the_collapse_is_refused_through_the_served_workload_too`: 13.3's collapse refusal still fires through `status`, not only through `load_settings` called directly, the moment both DSNs are given and are the same value. - `test_migrate_refuses_rather_than_borrowing_the_served_identity` (pre-existing, untouched) already covers "migrate refuses without it". Measured locally against this change (own Postgres container, bridge IP): `python -m pytest -q` reports 2472 passed, 11 skipped, 1 failed — the one failure is the same `tests/test_model_provider_broker.py:: test_the_broker_child_inherits_no_credential_shaped_environment` LC_CTYPE sandbox artifact already characterized as pre-existing and unrelated in the first commit on this branch. Against main's 2479 selected / 11 skipped in this same environment, this change is +5/+5/+0 (five tests: the three already on this branch plus the two new ones above) — `validate.yml`'s `Pin the triple` floors (`MIN_SELECTED=2476`, `MIN_PASSED=2465`, `EXPECT_SKIPPED=11`) permit the rise unchanged, and the exact skip count is unchanged. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Sonnet 5 --- src/opendox/runtime/config.py | 61 +++++++++++---- tests_runtime/conftest.py | 31 +------- tests_runtime/test_api_endpoints.py | 18 ++--- tests_runtime/test_migrations_apply.py | 7 -- tests_runtime/test_runtime_cli.py | 101 +++++++++++++++---------- tests_runtime/test_runtime_surface.py | 2 - 6 files changed, 113 insertions(+), 107 deletions(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index ecc53d65..deaf53e9 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -81,7 +81,7 @@ class Setting: "it must not be the identity migrations run as", ), Setting( - PREFIX + "MIGRATION_DATABASE_URL", None, True, True, + PREFIX + "MIGRATION_DATABASE_URL", None, False, True, "the PRIVILEGED DSN ordered-SQL migrations are applied with, used by " "`opendox runtime migrate` alone and never by the served application", ), @@ -181,13 +181,17 @@ class RuntimeSettings: Construct with :func:`load_settings`; the fields are in `SETTINGS` order. - `migration_database_url` is never `None` from either loader (plan 034, - 13.3): `load_settings` requires it exactly as it requires `database_url`, - and `load_migration_settings` already refused to load without one. + `migration_database_url` is `None` from `load_settings` whenever + `OPENDOX_MIGRATION_DATABASE_URL` is unset — RULED "required only for + migrate" (openxFactory#656, on the claim thread for plan 034's T071, + 2026-09-28): the served workload never needs it, `deploy/compose/ + docker-compose.yaml`'s `opendox` service and `docs/runtime.md` § 3 never + supply it, and `load_migration_settings` is the loader that actually + requires one (unaffected by this: it already refused to load without one). """ database_url: str - migration_database_url: str + migration_database_url: str | None oidc_issuer: str oidc_audience: str oidc_jwks_url: str | None @@ -1304,9 +1308,16 @@ def effective_schema(dsn: str) -> str | None: POSTGRESQL_SCHEMES = frozenset({"postgresql", "postgres"}) -def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: +def _refuse_non_postgresql_dsn(name: str, dsn: str | None) -> None: """`name`'s DSN is refused unless it selects a PostgreSQL scheme. + `None` OR EMPTY IS A NO-OP, not a refusal: `OPENDOX_MIGRATION_DATABASE_URL` + is optional for `load_settings` (RULED "required only for migrate", + openxFactory#656, on the claim thread for plan 034's T071, 2026-09-28), + so an absent migration DSN has no dialect to check — the same shape + `_refuse_two_dsns_that_select_different_schemas` below already reads as + "nothing to compare" rather than as a fault. + A DSN in the keyword/value form (`host=h dbname=d …`) names NO DIALECT AT ALL — that syntax is libpq's own conninfo grammar, and no other driver reads it — so only the URI form is checked: `urlsplit` reports an EMPTY @@ -1322,6 +1333,8 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: round 24); this is its DSN-flavoured twin; a bad `OPENDOX_DATABASE_URL` is not "set it to the broker endpoint", so it is not reused verbatim. """ + if not dsn: + return try: scheme = urllib.parse.urlsplit(dsn).scheme except ValueError as exc: @@ -1339,9 +1352,18 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: "document (RULING Q1)") -def _refuse_the_same_dsn_in_both_settings(served: str, migration: str) -> None: +def _refuse_the_same_dsn_in_both_settings( + served: str, migration: str | None) -> None: """One credential pasted into both settings is refused (plan 034, 13.3). + A NO-OP WHEN MIGRATION IS ABSENT, exactly like + `_refuse_two_dsns_that_select_different_schemas` below: with nothing to + compare, there is nothing to have collapsed. `OPENDOX_MIGRATION_DATABASE_ + URL` is optional (RULED "required only for migrate", openxFactory#656, on + the claim thread for plan 034's T071, 2026-09-28) — but WHEN BOTH ARE + GIVEN, this refusal still applies, on every path `load_settings` serves, + not only the falsifier's. + `OPENDOX_DATABASE_URL` is the least-privileged identity the API serves with; `OPENDOX_MIGRATION_DATABASE_URL` is the privileged one ordered-SQL migrations run as — the whole point of keeping two settings. A @@ -1354,6 +1376,8 @@ def _refuse_the_same_dsn_in_both_settings(served: str, migration: str) -> None: credentials, not one pasted twice, and this checks the value actually given, not the identity it happens to resolve to. """ + if not migration: + return if served == migration: raise ConfigurationError( f"{PREFIX}MIGRATION_DATABASE_URL is the same value as " @@ -1477,21 +1501,26 @@ def load_settings(env: Mapping[str, str] | None = None) -> RuntimeSettings: fetch from the broker's JWKS, and discovering that on the first request means it is already serving. - BOTH DSNs ARE REQUIRED (plan 034, 13.3): `OPENDOX_MIGRATION_DATABASE_URL` - used to default to `None`, so the collapsed, single-role shape this - refuses (below) was reachable only by accident, through a caller who - happened to set both to the same value — an install that left the - migration credential unset was never asked the question at all. Naming it - explicitly, even at the same database a served DSN already names, is what - keeps the two identities two DECISIONS rather than one remembered twice. + `OPENDOX_MIGRATION_DATABASE_URL` STAYS OPTIONAL HERE (RULED "required only + for migrate", openxFactory#656, on the claim thread for plan 034's T071, + 2026-09-28): the served workload never needs it — + `deploy/compose/docker-compose.yaml`'s `opendox` service and + `docs/runtime.md` § 3 never supply it, keeping the two identities in + different containers — and `load_migration_settings` below is the loader + that actually requires one. It is never DEFAULTED from + `OPENDOX_DATABASE_URL` either way. WHEN BOTH ARE GIVEN, though, the two + checks below still apply: a non-PostgreSQL migration DSN is refused + (13.2), and the two being the exact same value is refused (13.3) — + optional does not mean unchecked. """ env = os.environ if env is None else env algorithms = _algorithms(env) served = _require(env, _by_name(PREFIX + "DATABASE_URL")) - migration = _require(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")) + migration = _optional(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")) # THE DIALECT FIRST: a scheme this module cannot parse as PostgreSQL is not - # yet a DSN worth comparing at all. + # yet a DSN worth comparing at all. A no-op on an ABSENT migration DSN — + # see `_refuse_non_postgresql_dsn`. _refuse_non_postgresql_dsn(PREFIX + "DATABASE_URL", served) _refuse_non_postgresql_dsn(PREFIX + "MIGRATION_DATABASE_URL", migration) # THEN WHETHER THEY DISAGREE. See `_refuse_two_dsns_that_select_different_ diff --git a/tests_runtime/conftest.py b/tests_runtime/conftest.py index 93f53a4d..2c6e0b7b 100644 --- a/tests_runtime/conftest.py +++ b/tests_runtime/conftest.py @@ -219,29 +219,6 @@ def postgres_dsn() -> str: return dsn -@pytest.fixture(scope="session") -def migration_dsn(postgres_dsn: str) -> str: - """A DSN for `OPENDOX_MIGRATION_DATABASE_URL`, distinct from `postgres_dsn`. - - Plan 034, 13.3: `load_settings` now REQUIRES this setting and refuses a - value that is simply `postgres_dsn` repeated (13.3's own collapse - refusal), so every fixture built from the pair below needs a second, - genuinely different string — not a second database. None of this - module's fixtures apply migrations through this identity: `database` - applies them directly with `MigrationRunner`, and the served app under - test is never asked to `migrate`. So `load_settings` is the only reader - that cares about this value at all, and what it asks of a DSN is a - PostgreSQL scheme (13.2), a STRING distinct from `postgres_dsn` (13.3), - and a database and schema that agree with it - (`_refuse_two_dsns_that_select_different_schemas`). A URI FRAGMENT is - invisible to `database_named_by`, `user_named_by` and `schema_selected_by` - alike — none of the three inspects `urlsplit(...).fragment` — so - appending one changes the STRING without moving the identity those - functions compare. - """ - return postgres_dsn + "#opendox-test-migration-identity" - - @pytest.fixture def database(postgres_dsn: str) -> Iterator[object]: """A `Database` on a throwaway schema, with the migrations already applied.""" @@ -380,7 +357,7 @@ def verifier(jwks_path: str): @pytest.fixture() -def client(database, postgres_dsn: str, migration_dsn: str, verifier): +def client(database, postgres_dsn: str, verifier): """A `TestClient` over the REAL application, on this test's own schema. The application is given its OWN `Database` on the same schema rather than @@ -399,7 +376,6 @@ def client(database, postgres_dsn: str, migration_dsn: str, verifier): settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -423,8 +399,8 @@ def project_repository_root(tmp_path): @pytest.fixture() -def client_with_repositories(database, postgres_dsn: str, migration_dsn: str, - verifier, project_repository_root): +def client_with_repositories(database, postgres_dsn: str, verifier, + project_repository_root): """`client`, with the repository root pointed at this test's own directory.""" fastapi_testclient = _import_fastapi_testclient() from opendox.runtime.app import create_app @@ -433,7 +409,6 @@ def client_with_repositories(database, postgres_dsn: str, migration_dsn: str, settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PROJECT_REPOSITORY_ROOT": str(project_repository_root), diff --git a/tests_runtime/test_api_endpoints.py b/tests_runtime/test_api_endpoints.py index 84caccfe..21897203 100644 --- a/tests_runtime/test_api_endpoints.py +++ b/tests_runtime/test_api_endpoints.py @@ -425,7 +425,7 @@ def test_the_unauthenticated_surface_is_exactly_the_two_probes( def test_the_schema_viewers_appear_only_when_the_install_says_so( - database, postgres_dsn: str, migration_dsn: str, verifier) -> None: + database, postgres_dsn: str, verifier) -> None: from fastapi.testclient import TestClient from opendox.runtime.app import create_app @@ -435,7 +435,6 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PUBLISH_OPENAPI": "true", @@ -449,7 +448,7 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( def test_readiness_refuses_an_unmigrated_database_by_name( - postgres_dsn: str, migration_dsn: str, verifier) -> None: + postgres_dsn: str, verifier) -> None: """`select 1` succeeds against a schema with no tables in it at all. Readiness without a migration check therefore turns a fresh install READY @@ -472,7 +471,6 @@ def test_readiness_refuses_an_unmigrated_database_by_name( try: settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -590,8 +588,7 @@ def test_draft_pagination_returns_the_principals_drafts_not_an_empty_page( def test_readiness_refuses_a_database_whose_migration_file_has_changed( - database, postgres_dsn: str, migration_dsn: str, verifier, - tmp_path) -> None: + database, postgres_dsn: str, verifier, tmp_path) -> None: """Nothing pending is not the same as matching this tree.""" import shutil @@ -612,7 +609,6 @@ def test_readiness_refuses_a_database_whose_migration_file_has_changed( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(tmp_path), @@ -856,8 +852,7 @@ def test_a_hidden_user_and_a_missing_one_answer_byte_for_byte_the_same( def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( - database, postgres_dsn: str, migration_dsn: str, verifier, - tmp_path) -> None: + database, postgres_dsn: str, verifier, tmp_path) -> None: """An EMPTY directory made `plan()` and `drift()` both empty. `discover()` returns `[]` for a directory that exists and holds no @@ -879,7 +874,6 @@ def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( empty.mkdir() settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(empty), @@ -1558,8 +1552,7 @@ def transaction(): def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( - database, postgres_dsn: str, migration_dsn: str, rsa_key_pair, - tmp_path: Path) -> None: + database, postgres_dsn: str, rsa_key_pair, tmp_path: Path) -> None: """The end of A25-2, measured where it was reported: at the HTTP boundary. A realm publishing an RSA and an EC signing key — Keycloak, the moment a @@ -1600,7 +1593,6 @@ def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) diff --git a/tests_runtime/test_migrations_apply.py b/tests_runtime/test_migrations_apply.py index 1b657628..2afc0fe1 100644 --- a/tests_runtime/test_migrations_apply.py +++ b/tests_runtime/test_migrations_apply.py @@ -345,11 +345,6 @@ def test_status_reports_a_reachable_database_and_its_applied_migrations( # libpq startup parameter `Database(schema=…)` sets for the harness. scoped = (f"{postgres_dsn}?options=-c%20search_path%3D{database.schema}") monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) - # A DISTINCT STRING (13.3's collapse refusal) THAT SELECTS THE SAME SCHEMA - # (`_refuse_two_dsns_that_select_different_schemas`): a URI FRAGMENT moves - # neither, since neither reader inspects one. - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker.test/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) @@ -725,8 +720,6 @@ def test_status_calls_an_unmigrated_database_unhealthy_and_blames_the_tree( conn.execute(f"create schema {schema}") try: monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 2c3c97eb..3b220eee 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -206,13 +206,66 @@ def test_migrate_refuses_rather_than_borrowing_the_served_identity( assert PREFIX + "MIGRATION_DATABASE_URL" in evidence["message"] +def test_serve_and_status_load_with_no_migration_dsn_configured( + monkeypatch: pytest.MonkeyPatch) -> None: + """Brett's ruling on #1144 13.3 ("Required only for migrate + (Recommended)"): `OPENDOX_MIGRATION_DATABASE_URL` stays OPTIONAL for the + served workload — `serve` and `status` both go through `load_settings`, + and this is the CLI-level proof that neither refuses at configuration + when only the served DSN is set. This is the shape + `deploy/compose/docker-compose.yaml`'s `opendox` service and + `docs/runtime.md` § 3 already document: the migration credential lives + only in the separate `migrate` service/profile. + """ + monkeypatch.delenv(PREFIX + "MIGRATION_DATABASE_URL", raising=False) + monkeypatch.setenv(PREFIX + "DATABASE_URL", + "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") + monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") + + code, evidence = _run(cli.build_parser().parse_args( + ["runtime", "status", "--probe-timeout", "0.2"])) + assert evidence.get("refusal") != "configuration", evidence + assert "settings" in evidence, evidence + assert evidence["settings"][PREFIX + "MIGRATION_DATABASE_URL"] is None + + def _ok(self) -> None: + self.started = True + + _stub_uvicorn(monkeypatch, _ok) + monkeypatch.delenv(PREFIX + "MIGRATION_DATABASE_URL", raising=False) + code, evidence = _run_serve() + assert code == 0, evidence + assert evidence["ok"] is True, evidence + + +def test_the_collapse_is_refused_through_the_served_workload_too( + monkeypatch: pytest.MonkeyPatch) -> None: + """13.3's collapse refusal is `load_settings`'s own, not the falsifier's + special case: it fires for `status` (and every other served verb) too, + the moment an operator gives BOTH DSNs and they happen to be the exact + same value — even though migration is optional here (Brett's ruling, + above). + """ + same = "postgresql://opendox:hunter2@127.0.0.1:1/none" + monkeypatch.setenv(PREFIX + "DATABASE_URL", same) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", same) + monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") + monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") + + code, evidence = _run(cli.build_parser().parse_args( + ["runtime", "status", "--probe-timeout", "0.2"])) + assert code == 1, evidence + assert evidence["refusal"] == "configuration", evidence + assert PREFIX + "MIGRATION_DATABASE_URL" in evidence["message"] + assert "hunter2" not in evidence["message"], evidence["message"] + + def test_init_creates_the_project_repository_root_and_touches_no_database( monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -364,8 +417,6 @@ def test_status_reports_every_declared_setting_and_none_as_null( from opendox.runtime.config import SETTING_NAMES monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://u:pw@127.0.0.1:1/x") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://m:pw@127.0.0.1:1/x") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "RUNTIME_PG_ROLE", "opendox_runtime") @@ -436,8 +487,6 @@ def test_init_refuses_a_repository_root_that_is_not_a_directory( root.write_text("not a directory", encoding="utf-8") monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -502,8 +551,6 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -549,8 +596,6 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -634,8 +679,6 @@ def __exit__(self, *exc: object) -> None: db_stub.Database = _Exploding # type: ignore[attr-defined] monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", dsn) - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@db.internal:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -689,8 +732,6 @@ def __init__(self, config: object) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody:hunter2@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -808,8 +849,6 @@ def connection(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime:hunter2@127.0.0.1:5432/opendox") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", @@ -878,8 +917,6 @@ def transaction(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime@127.0.0.1:5432/opendox") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(tmp_path)) @@ -1166,7 +1203,6 @@ def test_no_broker_url_this_runtime_prints_can_carry_a_credential() -> None: redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ("OIDC_ISSUER", "OIDC_JWKS_URL"): env = dict(base, **{PREFIX + "OIDC_ISSUER": "https://broker/realms/x"}) @@ -1214,7 +1250,6 @@ def test_a_credential_in_a_broker_urls_query_is_refused_like_one_in_its_userinfo redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} for name, value, why in ( @@ -1263,7 +1298,6 @@ def test_the_broker_url_must_be_https_because_it_is_the_trust_anchor() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker/realms/x", "http://localhost:8080/realms/x", "http://127.0.0.1:8080/realms/x", "http://[::1]:8080/x"): @@ -1445,7 +1479,6 @@ def test_a_broker_url_that_names_no_host_is_refused_at_the_door() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https:///realms/x", "https://", "https:///", "broker/realms/x", "https://user:hunter2@/realms/x"): @@ -1531,7 +1564,6 @@ def test_a_broker_url_whose_port_is_not_a_number_is_refused_at_the_door( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker:not-a-port/realm", "https://broker:99999/realm", @@ -1577,7 +1609,6 @@ def test_the_issuer_carries_no_query_or_fragment_because_paths_are_appended( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer, component in (("https://broker/realms/x?tenant=a", "query"), ("https://broker/realms/x#frag", "fragment"), @@ -1623,7 +1654,6 @@ def test_the_loopback_exception_is_for_http_and_not_for_every_other_scheme( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("ftp://localhost/realms/x", "file://127.0.0.1/realms/x", "ws://localhost:8080/realms/x", "ftp://[::1]/realms/x"): @@ -1935,7 +1965,6 @@ def test_a_malformed_broker_url_never_prints_its_own_password() -> None: issuer = "https://svc:hunter2@broker\u2100evil.example/realms/x" env = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": issuer} @@ -2152,16 +2181,11 @@ def test_two_dsns_that_select_different_schemas_are_refused() -> None: PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db"}) - # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is refused NOW (plan 034, - # 13.3), where it used to be accepted as "the documented single-role - # deployment, not a mismatch": that reading is exactly what `load_settings` - # reading the setting as OPTIONAL bought, and the setting stopped being - # optional. A single-role install still names both DSNs, distinctly — - # `test_a_dsn_that_names_no_database_still_reaches_one`'s "two different - # secrets for one role" is what single-role now looks like. - with pytest.raises(ConfigurationError) as absent: - load_settings({**base, PREFIX + "DATABASE_URL": served}) - assert PREFIX + "MIGRATION_DATABASE_URL" in str(absent.value) + # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is the documented single-role + # deployment, not a mismatch (RULED "required only for migrate", + # openxFactory#656, on the claim thread for plan 034's T071, 2026-09-28 — + # this assertion was briefly the opposite of itself, reverted here). + assert load_settings({**base, PREFIX + "DATABASE_URL": served}) # THE OTHER DIRECTION IS REFUSED TOO: a served DSN that names no schema # beside a migration DSN that names one is the same split, and the @@ -2501,7 +2525,6 @@ def test_a_credential_shaped_parameter_name_is_a_WORD_and_not_a_substring() -> N assert redacted_url("https://broker/certs?token=x" ) == "https://broker/certs?token=" base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox"} settings = load_settings({**base, @@ -2702,8 +2725,6 @@ def test_init_creates_the_repository_root_private_whatever_the_umask_is( root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -2740,8 +2761,6 @@ def test_init_reports_an_existing_root_s_mode_and_does_not_change_it( os.chmod(root, 0o755) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) diff --git a/tests_runtime/test_runtime_surface.py b/tests_runtime/test_runtime_surface.py index b36c6203..ff6a73a3 100644 --- a/tests_runtime/test_runtime_surface.py +++ b/tests_runtime/test_runtime_surface.py @@ -258,7 +258,6 @@ def test_only_asymmetric_algorithms_can_be_configured_and_they_keep_pyjwts_spell ) base = {PREFIX + "DATABASE_URL": "postgresql://x/y", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m@x/y", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ASYMMETRIC_ALGORITHMS: @@ -319,7 +318,6 @@ def test_every_integer_setting_is_bounded_above_as_well_as_below() -> None: ) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} From f097fd889c1b07bad291794d41bad5a1969f6c5c Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 22:57:28 +0000 Subject: [PATCH 04/15] Fix round: load_migration_settings gets the same dialect gate load_settings has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review of this PR (thread on _refuse_non_postgresql_dsn's own definition): 13.2's dialect gate was wired into `load_settings` only. `load_migration_settings` — the loader `runtime migrate`/`reset` actually use — read OPENDOX_MIGRATION_DATABASE_URL, checked only that it was non-empty, and handed it straight to `Database`, so a non-PostgreSQL migration DSN (`sqlite:///x.db`, say) reached the driver instead of being refused by name at configuration. That is the same un-named failure 13.2 exists to prevent for the served loader, just reachable through the one path F13.1's falsifier does not call. One call to the existing `_refuse_non_postgresql_dsn`, right after the existing empty-DSN refusal and before `database_url`/`migration_database_ url` are both set to the same value. New test `test_migrate_refuses_a_non_postgresql_migration_dsn_at_configuration` is the dialect-refused twin of the existing `test_migrate_and_reset_need_ no_served_identity_and_no_broker`, which already shows an unreachable but valid-dialect migration DSN getting PAST configuration — this one shows a wrong-dialect one refused AT configuration, naming the setting and never repeating the DSN. Measured locally (own Postgres container, bridge IP): 2473 passed (+1), 11 skipped, 1 failed (the same pre-existing, unrelated LC_CTYPE sandbox artifact) — the new test is the only change to the count. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Sonnet 5 --- src/opendox/runtime/config.py | 8 ++++++++ tests_runtime/test_runtime_cli.py | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index deaf53e9..5c755006 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -1590,6 +1590,14 @@ def load_migration_settings(env: Mapping[str, str] | None = None) -> RuntimeSett f"{PREFIX}MIGRATION_DATABASE_URL is required to apply migrations; " f"{PREFIX}DATABASE_URL is the served runtime's least-privileged " "identity and is deliberately not used for schema changes") + # THE SAME DIALECT GATE `load_settings` ASKS, asked here too (Copilot + # review of this PR): this loader is the one path 13.2's own falsifier + # does not reach, and without this call a non-PostgreSQL migration DSN + # sailed past configuration entirely and reached `Database` instead, + # which is exactly the un-named, un-refused failure 13.2 exists to + # prevent for `load_settings`. `database_url` is set to this same `dsn` + # immediately below, so one call here covers both fields. + _refuse_non_postgresql_dsn(PREFIX + "MIGRATION_DATABASE_URL", dsn) return RuntimeSettings( database_url=dsn, migration_database_url=dsn, diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 3b220eee..feadfa1a 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -386,6 +386,29 @@ def test_migrate_and_reset_need_no_served_identity_and_no_broker( assert evidence["ok"] is False +def test_migrate_refuses_a_non_postgresql_migration_dsn_at_configuration( + monkeypatch: pytest.MonkeyPatch) -> None: + """13.2 covers `load_migration_settings` too, not only `load_settings`. + + The test above shows an unreachable but VALID-dialect migration DSN + getting past configuration; this is its dialect-refused twin (Copilot + review of this PR): `load_migration_settings` read the DSN and handed it + straight to `Database` with no dialect check of its own, so a + non-PostgreSQL migration DSN reached the driver instead of being refused + by name here — the same un-named failure 13.2 exists to prevent for the + served loader. + """ + for name in ("DATABASE_URL", "OIDC_ISSUER", "OIDC_AUDIENCE"): + monkeypatch.delenv(PREFIX + name, raising=False) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", "sqlite:///x.db") + code, evidence = _run(cli.build_parser().parse_args( + ["runtime", "migrate", "--connect-timeout", "0.2"])) + assert code == 1, evidence + assert evidence["refusal"] == "configuration", evidence + assert "postgres" in evidence["message"].lower(), evidence + assert PREFIX + "MIGRATION_DATABASE_URL" in evidence["message"] + + def test_the_entrypoint_turns_an_escaped_exception_into_evidence( monkeypatch: pytest.MonkeyPatch) -> None: """Every outcome is one redacted JSON object; none is a traceback.""" From b50e3b1dcb8c01a2619c987f3242ab9ba11e577d Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 30 Sep 2026 00:21:49 +0000 Subject: [PATCH 05/15] =?UTF-8?q?T070:=2013.4,=2013.5=20and=2013.6=20?= =?UTF-8?q?=E2=80=94=20OPENDOX=5FINSTALL=5FMODE=20and=20generate-and-open?= =?UTF-8?q?=20--local=20(plan=20034)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OPENDOX_INSTALL_MODE (`local` | `hosted`, default `hosted`) is read in runtime/config.py beside OPENDOX_OIDC_ISSUER and decides the install shape (#1144 13.4). `generate-and-open --local` makes the same selection (R1Q15 (b), as T007 batch H's 13.4 addendum reads); with neither the install is hosted (13.5). - A flag and a setting that disagree (`--local` beside OPENDOX_INSTALL_MODE=hosted) are refused, naming both. This is plan 034's fail-closed reading (Principle VII); no answer rules it and batch H does not write it into #1144. - LOCAL needs no broker: issuer, audience and key-set URL are empty. - LOCAL binds loopback only, with no opt-in. A non-loopback `--host` or OPENDOX_BIND_HOST is refused, naming the rule. The set is serve.py's own LOOPBACK_HOSTS, and a test holds the two equal. - HOSTED, set or by default, with no issuer refuses, naming OPENDOX_OIDC_ISSUER. generate-and-open asks the issuer first, so a run with nothing configured names it and `--local`. The hosted mode is otherwise unchanged (13.6). Holder readings on openxFactory#656 (Brett may overrule): - `runtime serve` refuses under local, because the API's identity is the broker's. - `runtime status` under local reports broker_keys "not configured (local mode)" and does not count it as a fault. - A broker setting beside local is refused by name. - An unrecognised mode value is refused, case-sensitively. The document server's generate-and-open resolves the shape before it scans, mints or binds anything. The hosted path loads the whole runtime configuration (R1Q16 (i); 13.4a). Also: - deploy/compose/.env.example gains OPENDOX_INSTALL_MODE=hosted, which test_every_runtime_setting_is_documented_in_env_example requires of every SETTINGS entry. - tests/test_doxbench_entrypoint.py's fixture now selects `--local` and scrubs the runtime settings, since the unset default is hosted and refuses with no issuer. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- deploy/compose/.env.example | 9 + src/opendox/cli.py | 64 ++++- src/opendox/runtime/cli.py | 39 ++- src/opendox/runtime/config.py | 240 ++++++++++++++++++- tests/test_doxbench_entrypoint.py | 11 + tests/test_install_mode_entrypoint.py | 208 ++++++++++++++++ tests_runtime/test_install_mode.py | 329 ++++++++++++++++++++++++++ 7 files changed, 889 insertions(+), 11 deletions(-) create mode 100644 tests/test_install_mode_entrypoint.py create mode 100644 tests_runtime/test_install_mode.py diff --git a/deploy/compose/.env.example b/deploy/compose/.env.example index 7d579a30..f8783755 100644 --- a/deploy/compose/.env.example +++ b/deploy/compose/.env.example @@ -48,6 +48,15 @@ OPENDOX_DATABASE_URL=postgresql://opendox_runtime:change-me-local-only-too@postg # `opendox-runtime runtime migrate` alone; the served application never receives it. OPENDOX_MIGRATION_DATABASE_URL=postgresql://opendox:change-me-local-only@postgres:5432/opendox +# The install shape: `hosted` or `local`, and this package is HOSTED — a +# broker, its pinned issuer and this file's own database. Unset means hosted +# too, which is the point: a hosted install that forgets its issuer REFUSES +# rather than falling into the local single-user mode (plan 034 T070; #1144 +# 13.4, 13.5). `local` is the one-user install `opendox generate-and-open +# --local` starts on a laptop, with no broker and loopback only; it refuses the +# broker settings below, so it is never selected here. +OPENDOX_INSTALL_MODE=hosted + # REQUIRED. The Keycloak broker's issuer, pinned (RULING Q2): a token from any # other issuer is refused rather than trusted. OPENDOX_OIDC_ISSUER=https://keycloak.example/realms/opendox diff --git a/src/opendox/cli.py b/src/opendox/cli.py index 994111dc..c7923abd 100644 --- a/src/opendox/cli.py +++ b/src/opendox/cli.py @@ -83,6 +83,11 @@ # `opendox.corpus_adapter` besides the stdlib. from opendox import corpus_adapter # noqa: E402 from opendox.runtime import local_git_adapter # noqa: E402 +# THE INSTALL SHAPE (plan 034 T070; #1144 13.4-13.6): `generate-and-open` +# resolves `--local` against `OPENDOX_INSTALL_MODE` here, before it generates +# or serves anything. Stdlib-only, like `local_git_adapter` above, which +# already imports it, so this adds no reach and no import weight. +from opendox.runtime import config as runtime_config # noqa: E402 from opendox.boundary import ( # noqa: E402 BoundaryViolation, HumanGate, OutputBoundary, ) @@ -409,11 +414,57 @@ def _validate(written: Path, args: argparse.Namespace, *, return 0 +def _resolve_install_shape(args: argparse.Namespace, + env=None) -> str: + """The install shape this run serves as, or `ConfigurationError` naming why. + + `--local` and `OPENDOX_INSTALL_MODE` are resolved by + `runtime_config.install_mode`, the one reading of the selector, which + refuses the two disagreeing (plan 034 T070's fail-closed reading) and + defaults to HOSTED (#1144 13.4, 13.5). Then each shape asks what it needs: + + * LOCAL binds loopback only, with no opt-in: a non-loopback `--host` is + refused naming the rule (13.4), and so is anything a local install + cannot be (`refuse_what_a_local_install_cannot_be`: a broker setting + beside it, or a non-loopback `OPENDOX_BIND_HOST`). It needs no broker. + Its datastore is 13.1's, and arrives with T072. + * HOSTED, set or by default, refuses with no issuer, NAMING THE ISSUER + (13.5), and then loads the runtime's whole configuration, because the + serving process is the one whose settings are the install's (13.4a; + R1Q16 (i)). Otherwise unchanged (13.6). + + Asked before anything is scanned, minted or bound, so a refused run leaves + nothing behind and exits at once rather than starting a server that a + bound would have to kill (F13.1's `test "$rc" -ne 124`). + """ + env = os.environ if env is None else env + mode = runtime_config.install_mode( + env, local_flag=bool(getattr(args, "local", False))) + if mode == runtime_config.INSTALL_MODE_LOCAL: + runtime_config.refuse_a_non_loopback_local_bind("--host", args.host) + runtime_config.refuse_what_a_local_install_cannot_be(env) + else: + runtime_config.require_the_hosted_issuer(env) + runtime_config.load_settings(env) + return mode + + def cmd_generate_and_open(args: argparse.Namespace, *, opener=webbrowser.open) -> int: """Regenerate the snapshot from the working tree into a run dir, start the local server, print the URL (ALWAYS), and open the browser. `--no-open` suppresses the browser; `--no-serve` returns after printing the URL without - blocking (used by tests). `opener` is injectable for testing.""" + blocking (used by tests). `opener` is injectable for testing. + + THE INSTALL SHAPE IS RESOLVED FIRST (plan 034 T070): `--local`, or + `OPENDOX_INSTALL_MODE=local`, selects the local single-user install, and + with neither the install is hosted — see `_resolve_install_shape`. A + refusal there is printed on stderr and the command exits 1, before any + other work.""" + try: + args.install_mode = _resolve_install_shape(args) + except runtime_config.ConfigurationError as exc: + print(f"generate-and-open refused: {exc}", file=sys.stderr) + return 1 # Ahead of minting the run dir, so a refused root leaves not even an empty # temp directory behind. `_generate_and_write` is still the guard that MATTERS # (it is the one no caller can skip); these are the same checks, earlier. @@ -968,6 +1019,17 @@ def build_parser(*, subcommand_extensions: tuple = ()) -> argparse.ArgumentParse gao.add_argument("--actor", default=None, help="human identity for loopback gate actions " "(default: the checkout's git user.name)") + # THE INSTALL SHAPE'S FLAG (plan 034 T070; R1Q15 (b), as T007 batch H's + # 13.4 addendum reads): the documented command is + # `opendox generate-and-open --local …`. The same selection as + # `OPENDOX_INSTALL_MODE=local`; with neither the install is hosted, and the + # flag beside `OPENDOX_INSTALL_MODE=hosted` is refused. + gao.add_argument(runtime_config.LOCAL_FLAG, action="store_true", + dest="local", + help="the LOCAL single-user install: no identity broker, " + "loopback only (the same selection as " + "OPENDOX_INSTALL_MODE=local; with neither, the " + "install is hosted and needs its broker's issuer)") gao.add_argument("--host", default=serve_mod.DEFAULT_HOST, help="bind host (default: 127.0.0.1, loopback only)") gao.add_argument("--port", type=int, default=0, help="bind port (default: ephemeral)") diff --git a/src/opendox/runtime/cli.py b/src/opendox/runtime/cli.py index 42921498..af0ac390 100644 --- a/src/opendox/runtime/cli.py +++ b/src/opendox/runtime/cli.py @@ -90,6 +90,8 @@ from opendox.runtime import identity, migrations from opendox.runtime.config import ( + INSTALL_MODE_LOCAL, + LOCAL_FLAG, SECRET_NAMES, SETTINGS, ConfigurationError, @@ -341,6 +343,11 @@ def _redacted_settings(settings: RuntimeSettings) -> dict[str, Any]: values = { "OPENDOX_DATABASE_URL": settings.database_url, "OPENDOX_MIGRATION_DATABASE_URL": settings.migration_database_url, + # THE INSTALL SHAPE this process loaded (plan 034 T070): a name and + # never a credential, and the first thing an operator reading `status` + # needs to know, because it decides whether the broker lines below + # mean anything at all. + "OPENDOX_INSTALL_MODE": settings.install_mode, # THE BROKER URLS ARE REDACTED HERE TOO. `load_settings` refuses # userinfo in the issuer and in an explicit JWKS URL — but this report # prints a DERIVED value, and a settings object can also be built by @@ -565,10 +572,31 @@ def cmd_migrate(args: argparse.Namespace) -> int: def cmd_serve(args: argparse.Namespace) -> int: - """Run the API. The pool is opened by the application's lifespan.""" + """Run the API. The pool is opened by the application's lifespan. + + NOT IN A LOCAL INSTALL (plan 034 T070; a holder reading on + openxFactory#656 that Brett may overrule). Every `/api/v1` route verifies + a token the BROKER signed (`oidc.build_verifier`), and the local mode has + no broker (#1144 13.4), so there is no identity this API could serve + with: started anyway, it would either refuse every request or, worse, + stand a local principal up that no task text defines. A local install is + served by `opendox generate-and-open --local`, and in release 1 its + document surface reads nothing from the store (R1Q16 (ii)). Refused + BEFORE anything is imported or bound, as evidence like every refusal. + """ settings = _settings_or_refusal(args) if isinstance(settings, int): return settings + if settings.install_mode == INSTALL_MODE_LOCAL: + return _emit({"verb": "serve", "refusal": "local-mode-has-no-broker", + "message": "the runtime API authenticates every request " + "with a token its identity broker signed, " + "and a LOCAL install has no broker, so this " + "API has no identity to serve with. A local " + "install is served by `opendox " + f"generate-and-open {LOCAL_FLAG}`; the " + "runtime API is a HOSTED install's surface " + "(13.4)"}, ok=False) try: import uvicorn @@ -744,6 +772,15 @@ def cmd_status(args: argparse.Namespace) -> int: f"unreachable: {type(exc).__name__}: {_safe_message(exc)}") ok = False + # A LOCAL INSTALL HAS NO BROKER TO PROBE (plan 034 T070; #1144 13.4), + # and that is its configuration rather than a fault: reported by name, and + # NOT counted against `ok`, so a healthy local install's `status` exits 0 + # — F13.1 runs it under `set -e`, and a verdict of "unhealthy" for a + # broker the install was never meant to have would be false. + if settings.install_mode == INSTALL_MODE_LOCAL: + report["broker_keys"] = "not configured (local mode)" + report["broker_discovery"] = None + return _emit(report, ok=ok) try: from opendox.runtime.oidc import build_verifier diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 5c755006..6edd5c88 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -85,10 +85,23 @@ class Setting: "the PRIVILEGED DSN ordered-SQL migrations are applied with, used by " "`opendox runtime migrate` alone and never by the served application", ), + # THE INSTALL SHAPE, READ BESIDE THE ISSUER IT DECIDES ABOUT (plan 034 + # T070; #1144 13.4, 13.5). Not `required`: its default is the SAFE value, + # and "unset" is the case 13.4 names as the one that must be safe. + Setting( + PREFIX + "INSTALL_MODE", "hosted", False, False, + "the install shape: `hosted` (the default — the broker, the pinned " + "issuer and an operator's database, exactly as before) or `local` " + "(one user, no broker, loopback only). `generate-and-open --local` " + "makes the same selection; the two may not disagree, and with " + "neither the install is hosted, so a hosted install with no issuer " + "refuses rather than falling into local mode (13.4, 13.5)", + ), Setting( PREFIX + "OIDC_ISSUER", None, True, False, "the Keycloak broker's issuer, pinned: a token from any other issuer " - "is refused rather than trusted (RULING Q2)", + "is refused rather than trusted (RULING Q2). Required by a HOSTED " + "install; a LOCAL install has no broker and refuses one given here", ), Setting( PREFIX + "OIDC_AUDIENCE", None, True, False, @@ -188,10 +201,19 @@ class RuntimeSettings: docker-compose.yaml`'s `opendox` service and `docs/runtime.md` § 3 never supply it, and `load_migration_settings` is the loader that actually requires one (unaffected by this: it already refused to load without one). + + `install_mode` is `INSTALL_MODE_HOSTED` or `INSTALL_MODE_LOCAL` (plan 034 + T070; #1144 13.4). A LOCAL install has no broker, so its `oidc_issuer` and + `oidc_audience` are EMPTY and its `oidc_jwks_url` is `None` — never a + placeholder that looks like an endpoint — and `jwks_url()` and + `discovery_url()` answer the empty string for it rather than a path glued + onto nothing. A hosted install always carries a real issuer, because + `load_settings` refuses one without it. """ database_url: str migration_database_url: str | None + install_mode: str oidc_issuer: str oidc_audience: str oidc_jwks_url: str | None @@ -213,6 +235,7 @@ def __repr__(self) -> str: "RuntimeSettings(database_url=, " "migration_database_url=" f"{'' if self.migration_database_url else 'None'}, " + f"install_mode={self.install_mode!r}, " # REDACTED TOO, and not because `load_settings` allows userinfo # here — it refuses it. A `RuntimeSettings` built by hand, in a # test or by a future caller, does not go through that door, and @@ -241,13 +264,25 @@ def jwks_url(self) -> str: it saves one variable in the common case; setting it explicitly is what a broker behind a rewriting proxy needs, which is why the variable exists at all rather than the URL always being computed. + + EMPTY FOR A LOCAL INSTALL, which has no issuer to derive one from + (plan 034 T070): the derivation would otherwise answer the bare path + `/protocol/openid-connect/certs`, which `status` would print as if it + were a configured endpoint. """ if self.oidc_jwks_url: return self.oidc_jwks_url + if not self.oidc_issuer: + return "" return self.oidc_issuer.rstrip("/") + "/protocol/openid-connect/certs" def discovery_url(self) -> str: - """The issuer's discovery document, for `opendox runtime status`.""" + """The issuer's discovery document, for `opendox runtime status`. + + Empty for a local install, for the reason `jwks_url` gives. + """ + if not self.oidc_issuer: + return "" return self.oidc_issuer.rstrip("/") + "/.well-known/openid-configuration" @@ -1488,7 +1523,171 @@ def _refuse_two_dsns_that_select_different_schemas( "carries a password)") -def load_settings(env: Mapping[str, str] | None = None) -> RuntimeSettings: +#: THE TWO INSTALL SHAPES (plan 034 T070; #1144 13.4). One selector decides +#: the whole shape at once — the identity mode here, and the datastore source +#: 13.1 adds — because requirements 12 and 13 both describe "the standalone +#: install" and one deliberate choice should decide both. +INSTALL_MODE_HOSTED = "hosted" +INSTALL_MODE_LOCAL = "local" +INSTALL_MODES: tuple[str, ...] = (INSTALL_MODE_LOCAL, INSTALL_MODE_HOSTED) + +#: The flag that makes the same selection as `OPENDOX_INSTALL_MODE=local`, +#: spelled ONCE: `opendox.cli` declares `generate-and-open`'s option with this +#: constant, and every refusal below names it with the same one (R1Q15 (b), as +#: T007 batch H's 13.4 addendum reads). +LOCAL_FLAG = "--local" + +#: LOOPBACK, AS THE DOCUMENT SERVER ALREADY JUDGES IT. `serve.py` makes its +#: `session` capability conditional on a bind in exactly this set +#: (`serve.LOOPBACK_HOSTS`), and 13.4 asks the local mode to "make the same +#: judgement at the mode's own boundary" — so it is the same set, not +#: `_is_loopback` above: that one reads `127.0.0.0/8` as loopback, and a local +#: install bound to `127.0.0.2` would then pass here while the server it starts +#: treats that very bind as off-loopback. This module cannot import `serve` +#: (the import weight in `opendox/runtime/__init__.py`), so the set is spelled +#: here and `tests_runtime/test_install_mode.py` holds it equal to serve's. +LOCAL_BIND_HOSTS: frozenset[str] = frozenset({"127.0.0.1", "::1", "localhost"}) + +#: THE SETTINGS ONLY A HOSTED INSTALL READS. Given beside the local mode, each +#: is REFUSED, by name (a holder reading on openxFactory#656, plan 034 T070, +#: the same fail-closed reading as the disagreeing flag and setting): an issuer +#: next to `local` says a broker was meant, and honouring `local` over it would +#: silently drop the authentication the operator configured. T072 adds the two +#: DSNs, which the local install supplies itself (13.1). +HOSTED_ONLY_SETTINGS: tuple[str, ...] = ( + PREFIX + "OIDC_ISSUER", + PREFIX + "OIDC_AUDIENCE", + PREFIX + "OIDC_JWKS_URL", +) + + +def install_mode(env: Mapping[str, str] | None = None, *, + local_flag: bool = False) -> str: + """`INSTALL_MODE_LOCAL` or `INSTALL_MODE_HOSTED`, or a refusal naming why. + + THE DEFAULT IS HOSTED, and it is the default because it is the safe one + (#1144 13.4: "It is UNSET, not `local`, that must be safe"): an install + that sets nothing is hosted, and a hosted install with no issuer refuses + (13.5), so single-user operation is never reached by forgetting to + configure something. A BLANK value is unset, the reading `_optional` gives + every other setting. + + `local_flag` is `generate-and-open --local` (R1Q15 (b)). It selects local + exactly as `OPENDOX_INSTALL_MODE=local` does, and the two may not + DISAGREE: `--local` beside `OPENDOX_INSTALL_MODE=hosted` is refused naming + both, so no explicit selection is silently overridden by the other. No + answer on #656 rules that pair; the refusal is plan 034's fail-closed + reading (Principle VII, T070), recorded for Brett in + `evidence/analyze-round-2.md` and NOT written into #1144. + + AN UNRECOGNISED VALUE IS REFUSED, matched case-sensitively (a holder + reading on #656, T070): `Local` or `single-user` is not a spelling of + either shape, and guessing which one was meant is the one thing a + selector whose default is a safety property must not do. + """ + env = os.environ if env is None else env + setting = PREFIX + "INSTALL_MODE" + raw = env.get(setting, "").strip() + if raw and raw not in INSTALL_MODES: + raise ConfigurationError( + f"{setting} is {raw!r}, which is neither `local` nor `hosted` (the " + "two values are matched exactly, case included). Unset means " + "hosted; a single-user install selects `local` explicitly, with " + f"{setting}=local or `generate-and-open {LOCAL_FLAG}`") + if local_flag and raw == INSTALL_MODE_HOSTED: + raise ConfigurationError( + f"{LOCAL_FLAG} selects the LOCAL install and {setting}=hosted " + "selects the HOSTED one. Both are explicit selections and they " + "disagree, so neither is allowed to override the other: drop the " + f"flag for a hosted install, or unset {setting} (or set it to " + "`local`) for a local one") + if local_flag: + return INSTALL_MODE_LOCAL + return raw or INSTALL_MODE_HOSTED + + +def refuse_a_non_loopback_local_bind(name: str, host: str) -> None: + """A LOCAL install binds loopback only, with NO opt-in (#1144 13.4). + + `name` is what set the address — `--host` on `generate-and-open`, or + `OPENDOX_BIND_HOST` for the runtime's own listener — so the refusal names + the thing the operator actually typed. The local mode has no broker, so a + local install other machines can reach is an unauthenticated multi-user + service wearing the word "local"; an install that must be reachable from + another machine is a HOSTED install, with a broker. + """ + if host in LOCAL_BIND_HOSTS: + return + raise ConfigurationError( + f"{name} {host!r} is not a loopback address, and a LOCAL install binds " + f"LOOPBACK ONLY ({', '.join(sorted(LOCAL_BIND_HOSTS))}). The local " + "mode has no identity broker, so a local install another machine can " + "reach would be an unauthenticated multi-user service. There is no " + "opt-in: an install that must be reachable from another machine is a " + "HOSTED install, with a broker (13.4)") + + +def refuse_what_a_local_install_cannot_be(env: Mapping[str, str]) -> None: + """The two refusals a LOCAL install makes of its own environment. + + Every hosted-only setting given beside it (`HOSTED_ONLY_SETTINGS`), and a + non-loopback `OPENDOX_BIND_HOST`, the runtime's own listener. One function, + because `load_settings` and `generate-and-open --local` both ask it and the + two must not come to disagree about what a local install is. + """ + _refuse_hosted_only_settings(env) + refuse_a_non_loopback_local_bind( + PREFIX + "BIND_HOST", + _optional(env, _by_name(PREFIX + "BIND_HOST")) or "127.0.0.1") + + +def _refuse_hosted_only_settings(env: Mapping[str, str]) -> None: + """Every setting in `HOSTED_ONLY_SETTINGS` given beside `local`, named.""" + given = [name for name in HOSTED_ONLY_SETTINGS + if env.get(name, "").strip()] + if given: + raise ConfigurationError( + f"{' and '.join(given)} {'are' if len(given) > 1 else 'is'} set, " + "and this is a LOCAL install, which has no broker and reads " + f"{'none of them' if len(given) > 1 else 'none'}. A broker " + "setting beside the local mode says a HOSTED install was meant, " + "and honouring `local` over it would silently drop that " + f"authentication: unset {'them' if len(given) > 1 else 'it'} for " + f"a local install, or drop {LOCAL_FLAG} / " + f"{PREFIX}INSTALL_MODE=local for a hosted one (the values are not " + "repeated here)") + + +def require_the_hosted_issuer(env: Mapping[str, str] | None = None) -> None: + """A HOSTED install with no issuer refuses, NAMING THE ISSUER (#1144 13.5). + + `load_settings` asks for the served DSN before it asks for the issuer, so + a hosted `generate-and-open` run with NOTHING configured would otherwise + be refused naming `OPENDOX_DATABASE_URL` — true, and not the refusal 13.5 + and plan 034's requirement-13 scenario ask for, which is the one that + tells an operator this install is HOSTED and how to select the other one. + So the document server's entry point asks this first, and `load_settings` + keeps its own order for every verb that already relies on it (13.6: the + hosted mode is otherwise unchanged). + """ + env = os.environ if env is None else env + issuer = PREFIX + "OIDC_ISSUER" + if env.get(issuer, "").strip(): + return + selected = (f"{PREFIX}INSTALL_MODE=hosted" + if env.get(PREFIX + "INSTALL_MODE", "").strip() + else f"{PREFIX}INSTALL_MODE is unset, and unset means hosted") + raise ConfigurationError( + f"{issuer} is required and is not set, and this install is HOSTED " + f"({selected}). A hosted install authenticates through the broker " + "whose issuer this names, and it does NOT fall back to single-user " + "operation without one (13.5). A single-user install selects the " + f"local mode explicitly: `generate-and-open {LOCAL_FLAG}` or " + f"{PREFIX}INSTALL_MODE=local") + + +def load_settings(env: Mapping[str, str] | None = None, *, + local_flag: bool = False) -> RuntimeSettings: """Resolve :class:`RuntimeSettings` from `env` (default `os.environ`). Refuses with :class:`ConfigurationError` naming the variable — never with a @@ -1512,9 +1711,22 @@ def load_settings(env: Mapping[str, str] | None = None) -> RuntimeSettings: checks below still apply: a non-PostgreSQL migration DSN is refused (13.2), and the two being the exact same value is refused (13.3) — optional does not mean unchecked. + + THE INSTALL MODE FIRST (plan 034 T070; #1144 13.4-13.6). `local_flag` is + `generate-and-open --local`, resolved against `OPENDOX_INSTALL_MODE` by + `install_mode`, which refuses the two disagreeing. A HOSTED install — the + default — is exactly what this function has always loaded, in the same + order, with the issuer and audience required (13.6). A LOCAL install needs + no broker: its issuer, audience and key-set URL are empty, and any of the + three GIVEN beside it is refused (`HOSTED_ONLY_SETTINGS`); and its own + listener, `OPENDOX_BIND_HOST`, must be loopback, with no opt-in. """ env = os.environ if env is None else env + mode = install_mode(env, local_flag=local_flag) + local = mode == INSTALL_MODE_LOCAL + if local: + refuse_what_a_local_install_cannot_be(env) algorithms = _algorithms(env) served = _require(env, _by_name(PREFIX + "DATABASE_URL")) migration = _optional(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")) @@ -1538,18 +1750,23 @@ def load_settings(env: Mapping[str, str] | None = None) -> RuntimeSettings: # make. _refuse_the_same_dsn_in_both_settings(served, migration) + bind_host = _optional(env, _by_name(PREFIX + "BIND_HOST")) or "127.0.0.1" + return RuntimeSettings( database_url=served, migration_database_url=migration, - oidc_issuer=_broker_url(env, _by_name(PREFIX + "OIDC_ISSUER"), - required=True, is_a_base_url=True) or "", - oidc_audience=_require(env, _by_name(PREFIX + "OIDC_AUDIENCE")), - oidc_jwks_url=_broker_url(env, _by_name(PREFIX + "OIDC_JWKS_URL"), - required=False), + install_mode=mode, + oidc_issuer="" if local else _broker_url( + env, _by_name(PREFIX + "OIDC_ISSUER"), + required=True, is_a_base_url=True) or "", + oidc_audience="" if local else _require( + env, _by_name(PREFIX + "OIDC_AUDIENCE")), + oidc_jwks_url=None if local else _broker_url( + env, _by_name(PREFIX + "OIDC_JWKS_URL"), required=False), oidc_algorithms=algorithms, oidc_jwks_ttl_seconds=_positive_int(env, _by_name(PREFIX + "OIDC_JWKS_TTL_SECONDS")), oidc_leeway_seconds=_positive_int(env, _by_name(PREFIX + "OIDC_LEEWAY_SECONDS")), - bind_host=_optional(env, _by_name(PREFIX + "BIND_HOST")) or "127.0.0.1", + bind_host=bind_host, bind_port=_positive_int(env, _by_name(PREFIX + "BIND_PORT")), runtime_pg_role=_role_name(env), served_schema=_served_schema(env), @@ -1601,6 +1818,11 @@ def load_migration_settings(env: Mapping[str, str] | None = None) -> RuntimeSett return RuntimeSettings( database_url=dsn, migration_database_url=dsn, + # READ, SO AN UNRECOGNISED VALUE IS REFUSED HERE TOO (plan 034 T070): + # a migration run is part of the same install and one reading of the + # selector serves every verb. It changes nothing else a migration run + # does; the broker fields below are sentinels in either shape. + install_mode=install_mode(env), oidc_issuer=MIGRATION_SENTINEL_ISSUER, oidc_audience=MIGRATION_SENTINEL_AUDIENCE, oidc_jwks_url=None, diff --git a/tests/test_doxbench_entrypoint.py b/tests/test_doxbench_entrypoint.py index 0eac4803..2488fdf0 100644 --- a/tests/test_doxbench_entrypoint.py +++ b/tests/test_doxbench_entrypoint.py @@ -72,6 +72,7 @@ from opendox import doxbench_install as install_mod from opendox import doxbench_model from opendox import serve as serve_mod +from opendox.runtime import config as runtime_config def _handler_class(httpd): @@ -163,8 +164,18 @@ def _capture(*args, **kwargs): checkout = tmp_path / "checkout" checkout.mkdir() session_root = tmp_path / "model-sessions" + # THE LOCAL INSTALL, SELECTED EXPLICITLY (plan 034 T070; #1144 13.4, as + # T007 batch H's addendum reads). With neither `--local` nor + # `OPENDOX_INSTALL_MODE=local` the install is HOSTED, and a hosted install + # with no issuer refuses (13.5) before `build_server` is ever reached. This + # fixture drives the single-user entrypoint a student runs, so it says so, + # and it scrubs every runtime setting first: a broker setting inherited + # from the shell would be refused beside the local mode, by design. + for name in runtime_config.SETTING_NAMES: + monkeypatch.delenv(name, raising=False) args = cli_mod.build_parser().parse_args([ "generate-and-open", + runtime_config.LOCAL_FLAG, "--repo-root", str(checkout), "--repository", "fixture-repo", "--source-revision", PINNED_REVISION, diff --git a/tests/test_install_mode_entrypoint.py b/tests/test_install_mode_entrypoint.py new file mode 100644 index 00000000..2a121ef4 --- /dev/null +++ b/tests/test_install_mode_entrypoint.py @@ -0,0 +1,208 @@ +"""`generate-and-open`'s install shape: F13.1's refusals, run the way F13.1 +runs them (plan 034 T070; #1144 13.4, 13.5, 13.6). + +F13.1's refusal probes take the SAME `generate-and-open` path its local probe +takes, under `timeout 30`, and each must exit nonzero, must not be the bound's +124, and must name its rule on stderr. These cases run that path in a child +process for the same reason: a regression that silently STARTED a server +would block in-process forever, where a child is killed by the bound and the +case fails on `TimeoutExpired` instead of hanging the suite. The child is +`python -m opendox.cli`, which `cli.py`'s `__main__` block hands to the +package's `main()`, so it is the same `main()` the `opendox` console script +runs. + +Each probe gives a well-formed `--repo-root` (a fresh git repository) and every +other setting its case needs, so the install shape is the only fault; and the +install shape is resolved BEFORE the repo root is scanned, so the refusal is +the install's, whatever the tree holds. No child reaches a database, a broker +or a socket. + +The disagreeing flag and setting are plan 034's fail-closed reading (T070), +not a line of #1144; the rest is #1144's. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from opendox import cli as cli_mod +from opendox import serve as serve_mod +from opendox.runtime import config as runtime_config + +SRC = Path(__file__).resolve().parents[1] / "src" +PREFIX = runtime_config.PREFIX +MODE = PREFIX + "INSTALL_MODE" + +#: F13.1's hosted probes' environment: every setting a hosted install needs +#: EXCEPT the issuer, so the issuer is the only fault. +HOSTED_WITHOUT_ISSUER = { + PREFIX + "DATABASE_URL": "postgresql://serve@127.0.0.1:1/opendox", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://migrate@127.0.0.1:1/opendox", + PREFIX + "OIDC_AUDIENCE": "fixture", +} + + +@pytest.fixture() +def corpus(tmp_path: Path) -> Path: + """A fresh repository, as F13.1's preamble makes one.""" + root = tmp_path / "plain-documents" + root.mkdir() + (root / "note.md").write_text("# A note\n\nPlain text.\n", encoding="utf-8") + env = {**os.environ, + "GIT_AUTHOR_NAME": "fixture", "GIT_AUTHOR_EMAIL": "fixture@example.invalid", + "GIT_COMMITTER_NAME": "fixture", + "GIT_COMMITTER_EMAIL": "fixture@example.invalid"} + for argv in (["git", "init", "-q"], ["git", "add", "-A"], + ["git", "commit", "-qm", "fixture"]): + subprocess.run(argv, cwd=root, env=env, check=True) + return root + + +def _probe(corpus: Path, tmp_path: Path, *extra: str, + env: dict[str, str] | None = None) -> tuple[int, str]: + """One bounded `generate-and-open` child: `(returncode, stderr)`.""" + child_env = {name: value for name, value in os.environ.items() + if name not in runtime_config.SETTING_NAMES} + child_env.update(env or {}) + child_env["PYTHONPATH"] = os.pathsep.join( + [str(SRC), child_env.get("PYTHONPATH", "")]).rstrip(os.pathsep) + try: + done = subprocess.run( + [sys.executable, "-m", "opendox.cli", "generate-and-open", + "--repo-root", str(corpus), "--repository", "fixture", + "--run-dir", str(tmp_path / "run"), "--no-open", *extra], + env=child_env, capture_output=True, text=True, timeout=30) + except subprocess.TimeoutExpired as exc: # F13.1's rc 124 + raise AssertionError( + "generate-and-open did not refuse: it was still running when the " + "30-second bound killed it, which is a server that STARTED") from exc + return done.returncode, done.stderr + + +def test_local_mode_refuses_a_non_loopback_bind_naming_the_rule( + corpus: Path, tmp_path: Path) -> None: + """F13.1: `OPENDOX_INSTALL_MODE=local … --host 0.0.0.0` is refused, BOUNDED, + and stderr names loopback (13.4).""" + rc, err = _probe(corpus, tmp_path, "--host", "0.0.0.0", "--port", "0", + env={MODE: "local"}) + assert rc != 0, err + assert "loopback" in err.lower(), err + assert "--host" in err, err + + +def test_the_flag_refuses_a_non_loopback_bind_exactly_as_the_setting_does( + corpus: Path, tmp_path: Path) -> None: + rc, err = _probe(corpus, tmp_path, runtime_config.LOCAL_FLAG, + "--host", "0.0.0.0") + assert rc != 0, err + assert "loopback" in err.lower(), err + + +@pytest.mark.parametrize("mode", ["hosted", None]) +def test_a_hosted_or_unset_install_with_no_issuer_refuses_naming_it( + corpus: Path, tmp_path: Path, mode) -> None: + """F13.1's last two probes: hosted, and then the selector UNSET, each with + every hosted setting except the issuer; each refuses, BOUNDED, naming + `OPENDOX_OIDC_ISSUER` (13.5). The second is what proves the unset DEFAULT + refuses exactly as `hosted` does.""" + env = dict(HOSTED_WITHOUT_ISSUER) + if mode is not None: + env[MODE] = mode + rc, err = _probe(corpus, tmp_path, "--port", "0", env=env) + assert rc != 0, err + assert PREFIX + "OIDC_ISSUER" in err, err + + +def test_with_nothing_configured_the_refusal_names_the_issuer_and_the_flag( + corpus: Path, tmp_path: Path) -> None: + """Plan 034's requirement-13 scenario 2: with no setting at all the + install is hosted, and the refusal is about the ISSUER and names how to + select local — not `OPENDOX_DATABASE_URL`, which `load_settings` would + have asked for first.""" + rc, err = _probe(corpus, tmp_path, "--port", "0") + assert rc != 0, err + assert PREFIX + "OIDC_ISSUER" in err, err + assert runtime_config.LOCAL_FLAG in err, err + assert PREFIX + "DATABASE_URL" not in err, err + + +def test_a_flag_and_a_setting_that_disagree_are_refused_naming_both( + corpus: Path, tmp_path: Path) -> None: + """T070's own case (plan 034's fail-closed reading): `--local` beside + `OPENDOX_INSTALL_MODE=hosted`, with a COMPLETE hosted configuration, so + either selection alone would have been accepted.""" + env = {**HOSTED_WITHOUT_ISSUER, + PREFIX + "OIDC_ISSUER": "https://issuer.example.invalid/realms/x", + MODE: "hosted"} + rc, err = _probe(corpus, tmp_path, runtime_config.LOCAL_FLAG, env=env) + assert rc != 0, err + assert runtime_config.LOCAL_FLAG in err, err + assert f"{MODE}=hosted" in err, err + + +def test_a_broker_setting_beside_the_local_flag_is_refused_by_name( + corpus: Path, tmp_path: Path) -> None: + rc, err = _probe(corpus, tmp_path, runtime_config.LOCAL_FLAG, + env={PREFIX + "OIDC_ISSUER": + "https://issuer.example.invalid/realms/x"}) + assert rc != 0, err + assert PREFIX + "OIDC_ISSUER" in err, err + + +# -- in process: what each shape resolves to ---------------------------------- + + +def _args(*extra: str): + return cli_mod.build_parser().parse_args( + ["generate-and-open", "--repo-root", "/nonexistent", + "--repository", "fixture", *extra]) + + +def test_the_local_shape_needs_no_broker_and_no_setting_at_all() -> None: + """13.4: `local` needs no broker. Resolved with an EMPTY environment.""" + assert cli_mod._resolve_install_shape( + _args(runtime_config.LOCAL_FLAG), env={}) == \ + runtime_config.INSTALL_MODE_LOCAL + assert cli_mod._resolve_install_shape( + _args(), env={MODE: "local"}) == runtime_config.INSTALL_MODE_LOCAL + + +@pytest.mark.parametrize("host", sorted(serve_mod.LOOPBACK_HOSTS)) +def test_the_local_shape_accepts_each_loopback_host(host: str) -> None: + assert cli_mod._resolve_install_shape( + _args(runtime_config.LOCAL_FLAG, "--host", host), env={}) == \ + runtime_config.INSTALL_MODE_LOCAL + + +def test_a_complete_hosted_configuration_resolves_hosted_unchanged() -> None: + """13.6: a hosted install with its broker configured serves as before.""" + env = {**HOSTED_WITHOUT_ISSUER, + PREFIX + "OIDC_ISSUER": "https://issuer.example.invalid/realms/x"} + assert cli_mod._resolve_install_shape(_args(), env=env) == \ + runtime_config.INSTALL_MODE_HOSTED + # a hosted document server may still bind beyond loopback, as it always + # could: the loopback rule is the LOCAL mode's + assert cli_mod._resolve_install_shape( + _args("--host", "0.0.0.0"), env=env) == \ + runtime_config.INSTALL_MODE_HOSTED + + +def test_the_local_bind_rule_is_the_document_servers_own_loopback_set() -> None: + """13.4: "the same judgement at the mode's own boundary". `config` cannot + import `serve`, so it spells the set; this holds the two equal.""" + assert runtime_config.LOCAL_BIND_HOSTS == serve_mod.LOOPBACK_HOSTS + + +def test_the_flag_is_declared_on_generate_and_open_and_follows_the_verb() -> None: + """10.1: every option follows its verb; `--local` is `generate-and-open`'s.""" + assert _args(runtime_config.LOCAL_FLAG).local is True + assert _args().local is False + with pytest.raises(SystemExit): + cli_mod.build_parser().parse_args( + [runtime_config.LOCAL_FLAG, "generate-and-open", "--repo-root", + "/x", "--repository", "fixture"]) diff --git a/tests_runtime/test_install_mode.py b/tests_runtime/test_install_mode.py new file mode 100644 index 00000000..de9ca524 --- /dev/null +++ b/tests_runtime/test_install_mode.py @@ -0,0 +1,329 @@ +"""`OPENDOX_INSTALL_MODE`: the local single-user install, and the hosted one +it cannot be reached from by omission (plan 034 T070; #1144 13.4, 13.5, 13.6). + +HERMETIC: standard library plus `opendox.runtime.config` and the runtime CLI, +both stdlib-only at import. No database is reached: every DSN below is a +well-formed PostgreSQL URI aimed at port 1 of the loopback, so a refusal here +is always a CONFIGURATION refusal, which is the whole of what 13.4-13.6 ask of +`load_settings`. + +WHAT IS RULED AND WHAT IS READ, so a reviewer can tell them apart: + + * RULED: the selector, its two values and its hosted default (#1144 13.4); + a hosted install with no issuer refuses naming it (13.5); the hosted mode + unchanged (13.6); local binds loopback only with no opt-in (13.4); + `generate-and-open --local` is the same selection (R1Q15 (b), T007 batch + H's 13.4 addendum). + * PLAN 034's FAIL-CLOSED READING, not in #1144: a `--local` flag and an + `OPENDOX_INSTALL_MODE` setting that disagree are refused, naming both + (T070; `evidence/analyze-round-2.md` U2-1, V2-6). + * HOLDER READINGS on openxFactory#656 that Brett may overrule (T070): an + unrecognised value is refused, case-sensitively; a broker setting given + beside `local` is refused by name; `runtime serve` refuses under `local`; + `runtime status` under `local` reports the broker as not configured and + does not count it as a fault. +""" + +from __future__ import annotations + +import argparse +import io +import json +from contextlib import redirect_stdout + +import pytest + +from opendox.runtime import cli +from opendox.runtime.config import ( + HOSTED_ONLY_SETTINGS, + INSTALL_MODE_HOSTED, + INSTALL_MODE_LOCAL, + INSTALL_MODES, + LOCAL_BIND_HOSTS, + LOCAL_FLAG, + PREFIX, + SETTING_NAMES, + ConfigurationError, + install_mode, + load_migration_settings, + load_settings, + require_the_hosted_issuer, +) + +MODE = PREFIX + "INSTALL_MODE" + +#: Two DSNs that pass T071's three refusals — one dialect, one database, two +#: different values — so the install shape is the only thing under test. +DSNS = { + PREFIX + "DATABASE_URL": "postgresql://serve@127.0.0.1:1/opendox", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://migrate@127.0.0.1:1/opendox", +} +#: Every setting a HOSTED install needs, well-formed. +HOSTED = {**DSNS, + PREFIX + "OIDC_ISSUER": "https://issuer.example.invalid/realms/fixture", + PREFIX + "OIDC_AUDIENCE": "fixture"} + + +def _refusal(env: dict, **kwargs) -> str: + try: + load_settings(env, **kwargs) + except ConfigurationError as exc: + return str(exc) + raise AssertionError(f"accepted: {env} {kwargs}") + + +@pytest.fixture() +def scrubbed(monkeypatch: pytest.MonkeyPatch) -> pytest.MonkeyPatch: + """No runtime setting inherited from the shell reaches a CLI verb.""" + for name in SETTING_NAMES: + monkeypatch.delenv(name, raising=False) + return monkeypatch + + +def _run(argv: list[str]) -> tuple[int, dict]: + args: argparse.Namespace = cli.build_parser().parse_args(argv) + buffer = io.StringIO() + with redirect_stdout(buffer): + code = args.func(args) + return code, json.loads(buffer.getvalue()) + + +# -- the selector ------------------------------------------------------------- + + +def test_the_selector_has_two_values_and_its_default_is_hosted() -> None: + """13.4: `local` and `hosted`, defaulting to `hosted`; UNSET is the safe one.""" + assert set(INSTALL_MODES) == {"local", "hosted"} + assert install_mode({}) == INSTALL_MODE_HOSTED + assert install_mode({MODE: ""}) == INSTALL_MODE_HOSTED + assert install_mode({MODE: " "}) == INSTALL_MODE_HOSTED + assert install_mode({MODE: "hosted"}) == INSTALL_MODE_HOSTED + assert install_mode({MODE: "local"}) == INSTALL_MODE_LOCAL + + +def test_the_flag_selects_local_exactly_as_the_setting_does() -> None: + """R1Q15 (b), as T007 batch H's 13.4 addendum reads.""" + assert LOCAL_FLAG == "--local" + assert install_mode({}, local_flag=True) == INSTALL_MODE_LOCAL + assert install_mode({MODE: "local"}, local_flag=True) == INSTALL_MODE_LOCAL + assert install_mode({MODE: "local"}) == install_mode({}, local_flag=True) + + +def test_a_flag_and_a_setting_that_disagree_are_refused_naming_both() -> None: + """T070's falsifier's second half: the disagreeing pair (plan 034's reading). + + Neither explicit selection may silently override the other, in either + direction the loader can see: the flag cannot win over a hosted setting, + and the setting cannot win over the flag. + """ + with pytest.raises(ConfigurationError) as caught: + install_mode({MODE: "hosted"}, local_flag=True) + message = str(caught.value) + assert LOCAL_FLAG in message, message + assert f"{MODE}=hosted" in message, message + # ...and the same refusal on the loader every served verb goes through, + # so the pair cannot be resolved one way here and another way there. + assert f"{MODE}=hosted" in _refusal({**HOSTED, MODE: "hosted"}, + local_flag=True) + + +@pytest.mark.parametrize("value", ["Local", "LOCAL", "Hosted", "single-user", + "locals", "loc al"]) +def test_an_unrecognised_value_is_refused_naming_the_two(value: str) -> None: + """A holder reading (#656, T070): matched case-sensitively, never guessed.""" + with pytest.raises(ConfigurationError) as caught: + install_mode({MODE: value}) + message = str(caught.value) + assert MODE in message and "`local`" in message and "`hosted`" in message + # every loader reads the one selector, so every loader refuses it + assert MODE in _refusal({**HOSTED, MODE: value}) + with pytest.raises(ConfigurationError): + load_migration_settings({**DSNS, MODE: value}) + + +# -- 13.5 and 13.6: the hosted install --------------------------------------- + + +@pytest.mark.parametrize("mode", [None, "hosted", ""]) +def test_a_hosted_install_with_no_issuer_refuses_naming_it(mode) -> None: + """13.5, set or by default, with every other hosted setting well-formed.""" + env = {k: v for k, v in HOSTED.items() if k != PREFIX + "OIDC_ISSUER"} + if mode is not None: + env[MODE] = mode + assert PREFIX + "OIDC_ISSUER" in _refusal(env) + with pytest.raises(ConfigurationError) as caught: + require_the_hosted_issuer(env) + assert PREFIX + "OIDC_ISSUER" in str(caught.value) + assert LOCAL_FLAG in str(caught.value), ( + "the refusal must say how a single-user install selects local") + + +def test_the_hosted_issuer_is_asked_first_by_the_entry_point_check() -> None: + """With NOTHING set, the entry point's refusal names the issuer (13.5), + not the served DSN `load_settings` happens to ask for first.""" + with pytest.raises(ConfigurationError) as caught: + require_the_hosted_issuer({}) + assert PREFIX + "OIDC_ISSUER" in str(caught.value) + assert "unset" in str(caught.value) + require_the_hosted_issuer(HOSTED) # and a present issuer passes + + +def test_the_hosted_mode_is_unchanged() -> None: + """13.6: same broker, same pinned issuer, loaded exactly as before.""" + for env in (HOSTED, {**HOSTED, MODE: "hosted"}): + settings = load_settings(env) + assert settings.install_mode == INSTALL_MODE_HOSTED + assert settings.oidc_issuer == HOSTED[PREFIX + "OIDC_ISSUER"] + assert settings.oidc_audience == "fixture" + assert settings.jwks_url() == (HOSTED[PREFIX + "OIDC_ISSUER"] + + "/protocol/openid-connect/certs") + # and a hosted install may still bind wherever its operator says + assert load_settings({**HOSTED, PREFIX + "BIND_HOST": "0.0.0.0"} + ).bind_host == "0.0.0.0" + # and it still refuses a missing audience, as it always did + env = {k: v for k, v in HOSTED.items() if k != PREFIX + "OIDC_AUDIENCE"} + assert PREFIX + "OIDC_AUDIENCE" in _refusal(env) + + +# -- 13.4: the local install ------------------------------------------------- + + +@pytest.mark.parametrize("selection", ["setting", "flag"]) +def test_a_local_install_needs_no_broker(selection: str) -> None: + env = dict(DSNS) + kwargs = {} + if selection == "setting": + env[MODE] = "local" + else: + kwargs["local_flag"] = True + settings = load_settings(env, **kwargs) + assert settings.install_mode == INSTALL_MODE_LOCAL + assert settings.oidc_issuer == "" + assert settings.oidc_audience == "" + assert settings.oidc_jwks_url is None + # no endpoint is derived from an issuer that does not exist + assert settings.jwks_url() == "" + assert settings.discovery_url() == "" + assert "install_mode='local'" in repr(settings) + + +@pytest.mark.parametrize("name", HOSTED_ONLY_SETTINGS) +def test_a_broker_setting_beside_the_local_mode_is_refused_by_name( + name: str) -> None: + """A holder reading (#656, T070): a broker setting says hosted was meant.""" + assert set(HOSTED_ONLY_SETTINGS) == {PREFIX + "OIDC_ISSUER", + PREFIX + "OIDC_AUDIENCE", + PREFIX + "OIDC_JWKS_URL"} + secret = "https://svc:hunter2@broker.example.invalid/realms/x" + message = _refusal({**DSNS, MODE: "local", name: secret}) + assert name in message, message + assert "hunter2" not in message, "the value must not be repeated" + # and the flag spelling of the same selection refuses it the same way + assert name in _refusal({**DSNS, name: secret}, local_flag=True) + + +def test_every_broker_setting_given_is_named_at_once() -> None: + env = {**HOSTED, MODE: "local"} + message = _refusal(env) + for name in (PREFIX + "OIDC_ISSUER", PREFIX + "OIDC_AUDIENCE"): + assert name in message, message + + +@pytest.mark.parametrize("host", sorted(LOCAL_BIND_HOSTS)) +def test_a_local_install_binds_each_loopback_spelling(host: str) -> None: + settings = load_settings({**DSNS, MODE: "local", + PREFIX + "BIND_HOST": host}) + assert settings.bind_host == host + + +@pytest.mark.parametrize("host", ["0.0.0.0", "::", "192.0.2.10", + "127.0.0.2", "example.invalid"]) +def test_a_local_install_refuses_a_non_loopback_bind_naming_the_rule( + host: str) -> None: + """13.4: loopback ONLY, and no opt-in. `127.0.0.2` is refused too: the + document server does not treat it as loopback (`serve.LOOPBACK_HOSTS`), + and the mode makes the SAME judgement at its own boundary.""" + message = _refusal({**DSNS, MODE: "local", PREFIX + "BIND_HOST": host}) + assert PREFIX + "BIND_HOST" in message, message + assert "loopback" in message.lower(), message + assert "no opt-in" in message.lower(), message + + +# -- the runtime CLI under the two modes -------------------------------------- + + +def test_runtime_serve_refuses_under_the_local_mode(scrubbed) -> None: + """A holder reading (#656, T070): the API's identity is the broker's. + + uvicorn and the application are STUBBED, so that a regression which + served anyway returns at once — and fails the assertions below — instead + of binding a real listener and blocking the suite forever (measured: the + un-stubbed form of this case hung under exactly that mutant). + """ + import sys + import types + + from opendox.runtime import app as app_module + + served = [] + + class _Config: + def __init__(self, app: object, **kwargs: object) -> None: + pass + + class _Server: + def __init__(self, config: object) -> None: + self.started = False + + def run(self) -> None: + served.append(True) + self.started = True + + stub = types.ModuleType("uvicorn") + stub.Config, stub.Server = _Config, _Server + scrubbed.setitem(sys.modules, "uvicorn", stub) + scrubbed.setattr(app_module, "create_app", lambda **kwargs: object()) + for name, value in DSNS.items(): + scrubbed.setenv(name, value) + scrubbed.setenv(MODE, "local") + code, evidence = _run(["runtime", "serve"]) + assert served == [], "the API was started for a LOCAL install" + assert code == 1 + assert evidence["ok"] is False + assert evidence["refusal"] == "local-mode-has-no-broker", evidence + assert f"generate-and-open {LOCAL_FLAG}" in evidence["message"] + + +def test_runtime_status_under_the_local_mode_probes_no_broker( + scrubbed, monkeypatch: pytest.MonkeyPatch) -> None: + """`broker_keys` is reported as not configured, and the verifier is never + built: a local install has no broker to reach, and a status verb that + called that a fault would exit nonzero for a healthy install.""" + from opendox.runtime import oidc + + def _no_broker(_settings): + raise AssertionError("status built a broker verifier for a LOCAL " + "install, which has no broker") + + monkeypatch.setattr(oidc, "build_verifier", _no_broker) + for name, value in DSNS.items(): + scrubbed.setenv(name, value) + scrubbed.setenv(MODE, "local") + code, evidence = _run(["runtime", "status", "--probe-timeout", "0.2"]) + assert evidence.get("refusal") is None, evidence + assert evidence["broker_keys"] == "not configured (local mode)" + assert evidence["broker_discovery"] is None + assert evidence["settings"][MODE] == INSTALL_MODE_LOCAL + assert evidence["settings"][PREFIX + "OIDC_ISSUER"] == "" + assert evidence["settings"][PREFIX + "OIDC_JWKS_URL"] == "" + # the database half (port 1, unreachable) is the ONLY reason `ok` is false + assert evidence["database"].startswith("unreachable"), evidence + assert code == 1 + + +def test_runtime_status_reports_the_hosted_mode_it_loaded(scrubbed) -> None: + for name, value in HOSTED.items(): + scrubbed.setenv(name, value) + _code, evidence = _run(["runtime", "status", "--probe-timeout", "0.2"]) + assert evidence["settings"][MODE] == INSTALL_MODE_HOSTED + assert evidence["broker_keys"].startswith("unreachable"), evidence From 32683e8f2ec5aa9443e572739ced835d4628161a Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 30 Sep 2026 00:48:46 +0000 Subject: [PATCH 06/15] Fix round: the install-mode fixture's repository ignores the user's git config (Copilot review) `tests/test_install_mode_entrypoint.py`'s `corpus` fixture ran `git commit` under the caller's global and system git configuration. A global `commit.gpgsign=true` therefore failed the setup before any install-mode probe ran. Measured with a hostile global config (`commit.gpgsign = true`, `gpg.program = /bin/false`): 7 errors at b50e3b1, 14 passed here. The fixture now sets GIT_CONFIG_GLOBAL=/dev/null and GIT_CONFIG_NOSYSTEM=1, as tests/test_checkout_head.py does. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_install_mode_entrypoint.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/test_install_mode_entrypoint.py b/tests/test_install_mode_entrypoint.py index 2a121ef4..9c4c6203 100644 --- a/tests/test_install_mode_entrypoint.py +++ b/tests/test_install_mode_entrypoint.py @@ -49,11 +49,19 @@ @pytest.fixture() def corpus(tmp_path: Path) -> Path: - """A fresh repository, as F13.1's preamble makes one.""" + """A fresh repository, as F13.1's preamble makes one. + + Made APART FROM the user's own git configuration (`GIT_CONFIG_GLOBAL`, + `GIT_CONFIG_NOSYSTEM`), as `tests/test_checkout_head.py` makes its + repositories, so a global signing rule or hook cannot fail the setup + before the install-mode probe it exists for ever runs (Copilot review of + this PR). + """ root = tmp_path / "plain-documents" root.mkdir() (root / "note.md").write_text("# A note\n\nPlain text.\n", encoding="utf-8") env = {**os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_NOSYSTEM": "1", "GIT_AUTHOR_NAME": "fixture", "GIT_AUTHOR_EMAIL": "fixture@example.invalid", "GIT_COMMITTER_NAME": "fixture", "GIT_COMMITTER_EMAIL": "fixture@example.invalid"} From 525f61c31db5df8b5264bb6a4f485ea0325d76b4 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 30 Sep 2026 00:58:12 +0000 Subject: [PATCH 07/15] Fix round: runtime migrate and reset refuse what a local install cannot be (Copilot review) load_migration_settings recorded OPENDOX_INSTALL_MODE=local but never asked refuse_what_a_local_install_cannot_be. So `runtime migrate` and a confirmed `runtime reset` accepted OPENDOX_OIDC_ISSUER, OPENDOX_OIDC_AUDIENCE, OPENDOX_OIDC_JWKS_URL or a non-loopback OPENDOX_BIND_HOST beside `local`, which load_settings and generate-and-open both refuse. They now refuse them at configuration, before any database is reached. Seven new cases: - the three broker settings x {migrate, reset}; - the bind. All seven fail at 32683e8 and pass here. Full suite: 2538 selected, 2527 passed, 11 skipped, 0 failed. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 19 +++++++++++++----- tests_runtime/test_install_mode.py | 31 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 6edd5c88..326144e4 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -1801,6 +1801,16 @@ def load_migration_settings(env: Mapping[str, str] | None = None) -> RuntimeSett not, because those are the served runtime and must have the real thing. """ env = os.environ if env is None else env + # THE ONE READING OF THE SELECTOR, AND OF WHAT A LOCAL INSTALL CANNOT BE + # (plan 034 T070; Copilot review of openDox-code#67). A migration run is + # part of the same install as the served one, so `runtime migrate` and + # `runtime reset` refuse what `load_settings` and `generate-and-open` + # refuse beside `local` — a broker setting, or a non-loopback + # `OPENDOX_BIND_HOST` — rather than accepting it in the one loader that + # never reads it. + mode = install_mode(env) + if mode == INSTALL_MODE_LOCAL: + refuse_what_a_local_install_cannot_be(env) dsn = env.get(PREFIX + "MIGRATION_DATABASE_URL", "").strip() if not dsn: raise ConfigurationError( @@ -1818,11 +1828,10 @@ def load_migration_settings(env: Mapping[str, str] | None = None) -> RuntimeSett return RuntimeSettings( database_url=dsn, migration_database_url=dsn, - # READ, SO AN UNRECOGNISED VALUE IS REFUSED HERE TOO (plan 034 T070): - # a migration run is part of the same install and one reading of the - # selector serves every verb. It changes nothing else a migration run - # does; the broker fields below are sentinels in either shape. - install_mode=install_mode(env), + # READ ABOVE, SO AN UNRECOGNISED VALUE IS REFUSED HERE TOO (plan 034 + # T070). It changes nothing else a migration run does; the broker + # fields below are sentinels in either shape. + install_mode=mode, oidc_issuer=MIGRATION_SENTINEL_ISSUER, oidc_audience=MIGRATION_SENTINEL_AUDIENCE, oidc_jwks_url=None, diff --git a/tests_runtime/test_install_mode.py b/tests_runtime/test_install_mode.py index de9ca524..28932ab3 100644 --- a/tests_runtime/test_install_mode.py +++ b/tests_runtime/test_install_mode.py @@ -321,6 +321,37 @@ def _no_broker(_settings): assert code == 1 +@pytest.mark.parametrize("verb", [["runtime", "migrate"], + ["runtime", "reset", "--confirm", + cli.RESET_CONFIRMATION]]) +@pytest.mark.parametrize("name", [PREFIX + "OIDC_ISSUER", + PREFIX + "OIDC_AUDIENCE", + PREFIX + "OIDC_JWKS_URL"]) +def test_migrate_and_reset_refuse_a_broker_setting_beside_the_local_mode( + scrubbed, verb: list[str], name: str) -> None: + """The migration loader asks what every other loader asks of `local` + (Copilot review of openDox-code#67): refused at CONFIGURATION, before any + database is reached — `reset` included, confirmation and all.""" + scrubbed.setenv(PREFIX + "MIGRATION_DATABASE_URL", DSNS[PREFIX + "MIGRATION_DATABASE_URL"]) + scrubbed.setenv(MODE, "local") + scrubbed.setenv(name, "https://issuer.example.invalid/realms/x" + if name != PREFIX + "OIDC_AUDIENCE" else "fixture") + code, evidence = _run(verb) + assert code == 1 + assert evidence["refusal"] == "configuration", evidence + assert name in evidence["message"], evidence + + +def test_migrate_refuses_a_non_loopback_bind_beside_the_local_mode( + scrubbed) -> None: + scrubbed.setenv(PREFIX + "MIGRATION_DATABASE_URL", DSNS[PREFIX + "MIGRATION_DATABASE_URL"]) + scrubbed.setenv(MODE, "local") + scrubbed.setenv(PREFIX + "BIND_HOST", "0.0.0.0") + code, evidence = _run(["runtime", "migrate"]) + assert code == 1 and evidence["refusal"] == "configuration", evidence + assert "loopback" in evidence["message"].lower(), evidence + + def test_runtime_status_reports_the_hosted_mode_it_loaded(scrubbed) -> None: for name, value in HOSTED.items(): scrubbed.setenv(name, value) From 02dadc5590dafe69a2ef156dc3d77bf275e24d67 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 30 Sep 2026 01:12:23 +0000 Subject: [PATCH 08/15] Fix round: status reports a local install's broker on its early return too (Copilot review) When the runtime extra is absent, `runtime status` returns early, and that return said `broker_keys: "not probed"` for every install. A local install's broker is not configured whether or not the extra is present. That answer comes from the configuration, not from a probe, so the early return now gives the local install the answer the full report gives: `"not configured (local mode)"`, with `broker_discovery: null`. Both returns write it through one helper, so the two cannot drift. A hosted install's early return still reads "not probed", as before (13.6). The branch is covered now, so its `pragma: no cover` goes. A new case runs both shapes with `opendox.runtime.db` absent from `sys.modules`. Before (`525f61c`'s runtime/cli.py): local 1 failed and hosted passed. After: both pass. Four mutants of the fix are killed. Full suite: 2540 selected, 2529 passed, 11 skipped, 0 failed. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Arc: neutral-product-standalone-operability Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/cli.py | 28 ++++++++++++++++++++++++---- tests_runtime/test_install_mode.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/opendox/runtime/cli.py b/src/opendox/runtime/cli.py index af0ac390..a69bfd41 100644 --- a/src/opendox/runtime/cli.py +++ b/src/opendox/runtime/cli.py @@ -664,6 +664,19 @@ def cmd_serve(args: argparse.Namespace) -> int: "exiting normally"}, ok=True) +def _report_the_local_broker(report: dict[str, Any]) -> None: + """What `status` says of a LOCAL install's broker, on every path. + + Its broker is NOT CONFIGURED (plan 034 T070; #1144 13.4): a statement + about the install's configuration rather than a probe's result, so there + is no discovery URL to report and nothing counts against `ok`. `status` + returns from two places, and both write it here, so the two answers + cannot drift apart (Copilot review of openDox-code#67). + """ + report["broker_keys"] = "not configured (local mode)" + report["broker_discovery"] = None + + def cmd_status(args: argparse.Namespace) -> int: """Report, never change: configuration, the schema pin, the ledger, the broker. @@ -693,10 +706,18 @@ def cmd_status(args: argparse.Namespace) -> int: try: from opendox.runtime.db import Database - except ImportError as exc: # pragma: no cover - the extra is absent + except ImportError as exc: report["runtime_extra"] = f"absent: {_safe_message(exc)}" report["database"] = "not probed" - report["broker_keys"] = "not probed" + # A LOCAL INSTALL'S BROKER IS NOT CONFIGURED WHETHER OR NOT THE EXTRA + # IS PRESENT (plan 034 T070; Copilot review of openDox-code#67). That + # answer comes from its configuration, not from a probe, so this early + # return gives the same one the full report gives below. A hosted + # install's broker was never probed, and says so, as before. + if settings.install_mode == INSTALL_MODE_LOCAL: + _report_the_local_broker(report) + else: + report["broker_keys"] = "not probed" return _emit(report, ok=False) report["runtime_extra"] = "present" @@ -778,8 +799,7 @@ def cmd_status(args: argparse.Namespace) -> int: # — F13.1 runs it under `set -e`, and a verdict of "unhealthy" for a # broker the install was never meant to have would be false. if settings.install_mode == INSTALL_MODE_LOCAL: - report["broker_keys"] = "not configured (local mode)" - report["broker_discovery"] = None + _report_the_local_broker(report) return _emit(report, ok=ok) try: from opendox.runtime.oidc import build_verifier diff --git a/tests_runtime/test_install_mode.py b/tests_runtime/test_install_mode.py index 28932ab3..f9c4bf9e 100644 --- a/tests_runtime/test_install_mode.py +++ b/tests_runtime/test_install_mode.py @@ -352,6 +352,34 @@ def test_migrate_refuses_a_non_loopback_bind_beside_the_local_mode( assert "loopback" in evidence["message"].lower(), evidence +@pytest.mark.parametrize("mode", [INSTALL_MODE_LOCAL, INSTALL_MODE_HOSTED]) +def test_runtime_status_without_the_runtime_extra_reports_the_broker_by_mode( + scrubbed, mode: str) -> None: + """`status` returns early when the runtime extra is absent, and that + return gives the local broker the same answer the full report gives + (Copilot review of openDox-code#67). A hosted install's broker reads + "not probed", unchanged (13.6). `None` in `sys.modules` is how an absent + module is simulated: the import raises `ImportError`, as it would without + the extra.""" + import sys + + scrubbed.setitem(sys.modules, "opendox.runtime.db", None) + for name, value in (HOSTED if mode == INSTALL_MODE_HOSTED else DSNS).items(): + scrubbed.setenv(name, value) + scrubbed.setenv(MODE, mode) + code, evidence = _run(["runtime", "status", "--probe-timeout", "0.2"]) + assert evidence["runtime_extra"].startswith("absent"), evidence + assert evidence["database"] == "not probed", evidence + assert code == 1 + if mode == INSTALL_MODE_LOCAL: + assert evidence["broker_keys"] == "not configured (local mode)", evidence + assert "broker_discovery" in evidence, evidence + assert evidence["broker_discovery"] is None + else: + assert evidence["broker_keys"] == "not probed", evidence + assert "broker_discovery" not in evidence, evidence + + def test_runtime_status_reports_the_hosted_mode_it_loaded(scrubbed) -> None: for name, value in HOSTED.items(): scrubbed.setenv(name, value) From 859b37b60a6c70818bcfdd8749ce232dcdfb7741 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:04:37 +0000 Subject: [PATCH 09/15] Fix round: a healthy local status is proven to exit 0, and RuntimeSettings' broker invariants are scoped (Copilot review) A local `status` returns `ok` on the database's verdict alone. Both earlier local cases forced a database fault and asserted exit 1, so a regression that also counted the absent broker as a fault would still have passed. A DB-backed case now runs `status` for a local install against a migrated schema on the suite's own server (`database` and `postgres_dsn`, with the schema selected in the DSN). It asserts `ok` true, exit 0, the database reachable with nothing pending and no drift, and the broker reported as not configured and never probed. Measured: with the local return mutated to `ok=False`, this case fails and the other 40 in the module pass. `RuntimeSettings`' docstring said that a local install's issuer and audience are empty and that a hosted one always carries a real issuer. That is true of `load_settings` alone. `load_migration_settings` carries the migration sentinels in either shape. The docstring now scopes each statement to its loader. Full suite: 2541 selected, 2530 passed, 11 skipped, 0 failed. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Arc: neutral-product-standalone-operability Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 22 +++++++++++++++------ tests_runtime/test_install_mode.py | 31 ++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 326144e4..ad8c518d 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -203,12 +203,22 @@ class RuntimeSettings: requires one (unaffected by this: it already refused to load without one). `install_mode` is `INSTALL_MODE_HOSTED` or `INSTALL_MODE_LOCAL` (plan 034 - T070; #1144 13.4). A LOCAL install has no broker, so its `oidc_issuer` and - `oidc_audience` are EMPTY and its `oidc_jwks_url` is `None` — never a - placeholder that looks like an endpoint — and `jwks_url()` and - `discovery_url()` answer the empty string for it rather than a path glued - onto nothing. A hosted install always carries a real issuer, because - `load_settings` refuses one without it. + T070; #1144 13.4), whichever loader built the object. + + THE BROKER FIELDS DEPEND ON THE LOADER, and what follows holds for + `load_settings` only (Copilot review of openDox-code#67): + * From `load_settings`, a LOCAL install has no broker. Its + `oidc_issuer` and `oidc_audience` are EMPTY and its `oidc_jwks_url` + is `None`, never a placeholder that looks like an endpoint, and + `jwks_url()` and `discovery_url()` answer the empty string rather + than a path glued onto nothing. A HOSTED one always carries a real + issuer, because `load_settings` refuses one without it. + * From `load_migration_settings`, in EITHER shape, the issuer and + audience are `MIGRATION_SENTINEL_ISSUER` and + `MIGRATION_SENTINEL_AUDIENCE`. A migration run reaches no broker at + all, and anything that tried to with those values would fail naming + them. So neither statement above applies to it. `install_mode` there + records the shape the run belongs to, and nothing else. """ database_url: str diff --git a/tests_runtime/test_install_mode.py b/tests_runtime/test_install_mode.py index f9c4bf9e..8fcc2a7b 100644 --- a/tests_runtime/test_install_mode.py +++ b/tests_runtime/test_install_mode.py @@ -352,6 +352,37 @@ def test_migrate_refuses_a_non_loopback_bind_beside_the_local_mode( assert "loopback" in evidence["message"].lower(), evidence +def test_runtime_status_of_a_healthy_local_install_exits_zero( + scrubbed, monkeypatch: pytest.MonkeyPatch, postgres_dsn: str, + database) -> None: + """THE EXIT CODE IS THE DATABASE'S VERDICT ALONE (Copilot review of + openDox-code#67). A migrated, reachable database is the only thing a + local install's `status` needs, and with it the verb exits 0. The broker + is reported as not configured and is never probed, so F13.1's `set -e` + survives. The other local cases force a database fault and exit 1, so + they cannot tell a broker counted as a fault from a database that failed. + """ + from opendox.runtime import oidc + + def _no_broker(_settings): + raise AssertionError("status built a broker verifier for a LOCAL " + "install, which has no broker") + + monkeypatch.setattr(oidc, "build_verifier", _no_broker) + joiner = "&" if "?" in postgres_dsn else "?" + scrubbed.setenv(PREFIX + "DATABASE_URL", + f"{postgres_dsn}{joiner}options=-c%20search_path%3D" + f"{database.schema}%2Cpublic") + scrubbed.setenv(MODE, "local") + code, evidence = _run(["runtime", "status", "--probe-timeout", "5"]) + assert evidence["database"] == "reachable", evidence + assert evidence["pending_migrations"] == [], evidence + assert not evidence["migration_drift"], evidence + assert evidence["broker_keys"] == "not configured (local mode)", evidence + assert evidence["broker_discovery"] is None + assert evidence["ok"] is True and code == 0, evidence + + @pytest.mark.parametrize("mode", [INSTALL_MODE_LOCAL, INSTALL_MODE_HOSTED]) def test_runtime_status_without_the_runtime_extra_reports_the_broker_by_mode( scrubbed, mode: str) -> None: From 026f00ea7ce8eda3dfc513e3cff848c9a08a472d Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:51:48 +0000 Subject: [PATCH 10/15] Fix round: the install-mode module says which of its cases is DB-backed (Copilot review) The module docstring called every case hermetic. Since the fourth fix round, one is not: `test_runtime_status_of_a_healthy_local_install_exits_zero` takes the suite's `postgres_dsn` and `database` fixtures, because a healthy local `status` exits 0 only against a database that answers. The docstring now names that case and says it is skipped without Postgres and fails under CI, like every DB-backed case. It says the rest stay hermetic. Docstring only: the module runs 41 passed. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Arc: neutral-product-standalone-operability Co-Authored-By: Claude Opus 5.5 (1M context) --- tests_runtime/test_install_mode.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests_runtime/test_install_mode.py b/tests_runtime/test_install_mode.py index 8fcc2a7b..24d44eb6 100644 --- a/tests_runtime/test_install_mode.py +++ b/tests_runtime/test_install_mode.py @@ -1,12 +1,20 @@ """`OPENDOX_INSTALL_MODE`: the local single-user install, and the hosted one it cannot be reached from by omission (plan 034 T070; #1144 13.4, 13.5, 13.6). -HERMETIC: standard library plus `opendox.runtime.config` and the runtime CLI, -both stdlib-only at import. No database is reached: every DSN below is a -well-formed PostgreSQL URI aimed at port 1 of the loopback, so a refusal here -is always a CONFIGURATION refusal, which is the whole of what 13.4-13.6 ask of +HERMETIC, WITH ONE EXCEPTION. Every case but one uses the standard library +plus `opendox.runtime.config` and the runtime CLI, both stdlib-only at import, +and reaches no database. Each DSN in those cases is a well-formed PostgreSQL +URI aimed at port 1 of the loopback, so a refusal there is always a +CONFIGURATION refusal, which is the whole of what 13.4-13.6 ask of `load_settings`. +The exception is `test_runtime_status_of_a_healthy_local_install_exits_zero`, +which is DB-BACKED (Copilot review of openDox-code#67). A healthy local +`status` exits 0 only against a database that answers, so that case takes the +suite's `postgres_dsn` and `database` fixtures (`tests_runtime/conftest.py`). +Like every DB-backed case, it is skipped where there is no Postgres, and it +fails under CI. + WHAT IS RULED AND WHAT IS READ, so a reviewer can tell them apart: * RULED: the selector, its two values and its hosted default (#1144 13.4); From c8fac05e328bd879a220e43bd8c985a81a43b653 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:04:39 +0000 Subject: [PATCH 11/15] T070, owed at the merge round: the standalone generate-and-open runs say --local Since this PR, generate-and-open with neither --local nor OPENDOX_INSTALL_MODE=local is a HOSTED install, which refuses without its broker's issuer (#1144 13.4, 13.5). Four cases that landed on main with phase 2 run generate-and-open as the single-user install and relied on the old default, so each now says --local: - tests/test_standalone_generate_path.py (T056), case 3: the server starts, answers and stops. The module docstring names the change and moves F10.1's plain-install run to T077. - tests/test_post_render_validator.py (T058), test_generate_and_open_gives_the_same_verdicts, both fixtures. - tests/test_projection_seams.py (T055), test_generate_and_open_refuses_an_empty_source_option_before_its_run_dir. No case means hosted, so none takes a hosted fixture. Before this commit, all four fail on the merged tree with the hosted issuer refusal; after it they pass. Three mutants of the local path are killed, each failing all four cases: --local ignored, local refusing its own loopback default, and local also asking for the hosted issuer. Full suite: 3117 selected, 3106 passed, 11 skipped, 0 failed. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Arc: neutral-product-standalone-operability Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_post_render_validator.py | 10 ++++++---- tests/test_projection_seams.py | 8 +++++--- tests/test_standalone_generate_path.py | 19 +++++++++++++------ 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/tests/test_post_render_validator.py b/tests/test_post_render_validator.py index f2772edf..a18ddd22 100644 --- a/tests/test_post_render_validator.py +++ b/tests/test_post_render_validator.py @@ -141,11 +141,13 @@ def test_F7_2_the_malformed_fixture_is_refused_without_strict_too(tmp_path) -> N @pytest.mark.parametrize("fixture,expected", [(PLAIN, 0), (MALFORMED, 1)]) def test_generate_and_open_gives_the_same_verdicts(tmp_path, fixture, expected) -> None: - """`generate-and-open --no-open --no-serve --strict`: the good fixture - builds its server and prints its URL, and the malformed one stops before - a server is built, naming the rule.""" + """`generate-and-open --local --no-open --no-serve --strict`: the good + fixture builds its server and prints its URL, and the malformed one stops + before a server is built, naming the rule. `--local` because this is the + single-user install: since plan 034 T070, an unflagged run is HOSTED and + refuses without its broker's issuer, before the validator is reached.""" repo = fresh_repository(fixture, tmp_path) - child, status = run_module(tmp_path, "opendox.cli", "generate-and-open", + child, status = run_module(tmp_path, "opendox.cli", "generate-and-open", "--local", "--repo-root", str(repo), "--repository", "fixture", "--no-open", "--no-serve", "--strict", "--run-dir", str(tmp_path / "run")) diff --git a/tests/test_projection_seams.py b/tests/test_projection_seams.py index 1d5c5883..766a848a 100644 --- a/tests/test_projection_seams.py +++ b/tests/test_projection_seams.py @@ -1397,9 +1397,11 @@ def test_generate_and_open_refuses_an_empty_source_option_before_its_run_dir( _declaring_generator(calls) repo = _repository(tmp_path) run_dir = tmp_path / "run" - rc = cli.main(["generate-and-open", "--repo-root", str(repo), "--repository", - "garden", "--run-dir", str(run_dir), "--no-open", "--no-serve", - "--possibles", ""]) + # `--local`: the single-user install. Since plan 034 T070 an unflagged run + # is HOSTED, and its issuer refusal would come first. + rc = cli.main(["generate-and-open", "--local", "--repo-root", str(repo), + "--repository", "garden", "--run-dir", str(run_dir), + "--no-open", "--no-serve", "--possibles", ""]) assert rc == 1 assert ("generate-and-open refused: --possibles was given an empty path" in capsys.readouterr().err) diff --git a/tests/test_standalone_generate_path.py b/tests/test_standalone_generate_path.py index 0a95efca..b71f3650 100644 --- a/tests/test_standalone_generate_path.py +++ b/tests/test_standalone_generate_path.py @@ -17,7 +17,7 @@ role keys, the verb reports it, naming the document, the value and the six keys, and the snapshot it writes reads that document as a source. T054 tests the projection's half in process. -3. `python -m opendox.cli generate-and-open --no-open` STARTS a server, which +3. `python -m opendox.cli generate-and-open --local --no-open` STARTS a server, which answers `/index.html`, `/snapshot.json`, `/capabilities` and `/source/`, refuses `/source/.git/config`, and stops on an interrupt with status 0. 4. `python -m opendox.serve`, the server's own entry point, starts and answers @@ -46,8 +46,14 @@ seconds). T056 flushes it in both entry points, and cases 3 and 4 fail without that. -NOT HERE: F10.1's run through a plain install, with the console script and no -`--local`, arrives in phase 3 (T070, and T077 as batch H amends it). +`--local` (plan 034 T070; #1144 13.4, 13.5): case 3 is the single-user install, +so it says so. Since T070, `generate-and-open` with neither `--local` nor +`OPENDOX_INSTALL_MODE=local` is a HOSTED install, which refuses without its +broker's issuer. That refusal is what an unflagged run of this case would now +hit, and it is T070's own subject, held in `tests/test_install_mode_entrypoint.py`. + +NOT HERE: F10.1's run through a plain install, with the console script, arrives +in phase 3 (T077, as batch H amends it). A CREATED FILE: no carve-manifest row (RULED OQ-C). """ @@ -256,9 +262,10 @@ def test_the_unedited_fixture_declares_that_document_a_candidate(tmp_path) -> No # --------------------------------------------------------------------------- def test_generate_and_open_starts_a_server_that_answers_with_no_sibling(tmp_path) -> None: - """`python -m opendox.cli generate-and-open --no-open`, with no + """`python -m opendox.cli generate-and-open --local --no-open`, with no `--no-serve`: the server starts, says where on a buffered pipe, answers - the core routes, and stops on an interrupt with status 0. + the core routes, and stops on an interrupt with status 0. `--local` + because this is the single-user install (T070). Both lines it prints before blocking in `serve_forever()`, the URL and "serving until interrupted", are read WHILE IT RUNS, before the @@ -267,7 +274,7 @@ def test_generate_and_open_starts_a_server_that_answers_with_no_sibling(tmp_path e3574774, r4146289331).""" repo = _fresh_repository(tmp_path) run_dir = tmp_path / "run" - child = Child(tmp_path, "opendox.cli", "generate-and-open", + child = Child(tmp_path, "opendox.cli", "generate-and-open", "--local", "--repo-root", str(repo), "--repository", "fixture", "--no-open", "--port", "0", "--run-dir", str(run_dir)) try: From c39d960e4144a934fb2d39c2361ddd2b5ed0f8d4 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:28:35 +0000 Subject: [PATCH 12/15] Fix round: a PostgreSQL scheme libpq would not read as a URI is refused (Copilot review) Copilot's review at the merge-from-main head (adeb6fed) noted that `postgresql:foo` is still accepted, although the supported URI forms need `://`. Measured, it is worse than that. urlsplit reads `postgresql:` with no `//`, and any capitalized `PostgreSQL://` or `POSTGRES://`, as the PostgreSQL scheme, so the dialect gate passed all of them. libpq reads none of them as a URI. It recognizes only the exact, lower-case `postgresql://` and `postgres://`, parses the rest as keyword/value, and refuses them with a message that repeats the whole value (psycopg 3.3.6: `missing "=" after "postgresql:svc:hunter2@db/x" in connection info string`). That is the un-named failure at the driver that 13.2 exists to stop, and it carries the password. _refuse_non_postgresql_dsn now refuses a PostgreSQL scheme in any spelling other than libpq's two. The refusal names the setting and the two spellings, and does not repeat the value. Both loaders ask it. The new case covers four spellings for each of the two settings. All 8 fail against adeb6fed's config.py and pass here. The two accepted spellings and the keyword/value form still load. Five mutants are killed: the check dropped, case-insensitive matching, a check of only the `//`, every PostgreSQL DSN refused, and the value repeated. Full suite: 3069 selected, 3058 passed, 11 skipped, 0 failed. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Arc: neutral-product-standalone-operability Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 20 ++++++++++++++++++++ tests_runtime/test_runtime_cli.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 5c755006..7ae28936 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -1350,6 +1350,26 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str | None) -> None: "runtime keeps: a second one would double every migration and " "every schema test forever, for a database that holds no " "document (RULING Q1)") + # A POSTGRESQL SCHEME IS A URI ONLY IN LIBPQ'S OWN SPELLING (Copilot + # review of openDox-code#60, at its merge-from-main round). `urlsplit` + # reads `postgresql:` with no `//`, and any capitalized `PostgreSQL://`, + # as the PostgreSQL scheme. libpq does not: it recognizes a URI only by + # the exact, lower-case `postgresql://` or `postgres://`, and parses + # anything else as keyword/value, which it then refuses with a message + # that REPEATS THE WHOLE VALUE (measured, psycopg 3.3.6: + # `missing "=" after "postgresql:svc:hunter2@db/x" in connection info + # string`). That is the un-named failure at the driver that 13.2 exists to + # stop, and it carries the password with it. So it is refused here, named, + # and the value is not repeated. + if scheme in POSTGRESQL_SCHEMES and not dsn.startswith( + tuple(f"{known}://" for known in sorted(POSTGRESQL_SCHEMES))): + raise ConfigurationError( + f"{name} reads as the PostgreSQL scheme but is not a URI libpq " + "reads: libpq recognizes only the exact, lower-case " + "`postgresql://` or `postgres://` prefix, and would refuse any " + "other spelling with a message that repeats the whole value. " + "Write the scheme as one of those two (the value is not " + "repeated here, because it can carry a password)") def _refuse_the_same_dsn_in_both_settings( diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index feadfa1a..c3e13bb6 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -2264,6 +2264,34 @@ def test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept() -> None: "host=h dbname=db user=m"}) +@pytest.mark.parametrize("dsn", ["postgresql:svc:hunter2@db.invalid/x", + "postgresql:/svc:hunter2@db.invalid/x", + "PostgreSQL://svc:hunter2@db.invalid/x", + "POSTGRES://svc:hunter2@db.invalid/x"]) +@pytest.mark.parametrize("setting", ["DATABASE_URL", "MIGRATION_DATABASE_URL"]) +def test_a_postgresql_scheme_libpq_would_not_read_as_a_uri_is_refused( + dsn: str, setting: str) -> None: + """`urlsplit` reads each of these as the PostgreSQL scheme. libpq reads + none of them as a URI: it knows only the exact, lower-case + `postgresql://` and `postgres://`, so it parses the rest as keyword/value + and refuses them with a message that repeats the whole value, password + and all. So each is refused at configuration, named, and the value is + not repeated (Copilot review of this PR, at its merge-from-main round). + The two spellings libpq does read stay accepted (the case above).""" + from opendox.runtime.config import ConfigurationError, load_settings + + good = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:q@h/db"} + with pytest.raises(ConfigurationError) as refused: + load_settings({PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox", + **good, PREFIX + setting: dsn}) + message = str(refused.value) + assert PREFIX + setting in message, message + assert "postgresql://" in message, message + assert "hunter2" not in message and dsn not in message, message + + def test_an_unparseable_dsn_is_refused_and_never_raises_a_bare_valueerror( ) -> None: """`urlsplit` itself raises for a DSN it cannot parse, and this module's From cdf7382b7e3f30884e0c93703b8254fe782bdb2b Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:35:03 +0000 Subject: [PATCH 13/15] Fix round: the --local callers inherit none of the runner's runtime settings (Copilot review) Copilot's review at c8fac05e opened three threads, and all three are real. The four callers this PR gave --local took the runner's environment along: tests/standalone_child.py's Child copied os.environ, and the in-process projection-seams case read it directly. An exported OPENDOX_INSTALL_MODE=hosted, or a broker issuer, made each --local run refuse before it reached what it tests. - tests/standalone_child.py: every child's environment drops each name in opendox.runtime.config.SETTING_NAMES. This covers T056's case 3 and T058's two post-render cases, and any later child. - tests/test_projection_seams.py: the in-process empty-option case scrubs SETTING_NAMES first, as the doxBench entrypoint fixture does. - tests/test_standalone_generate_path.py: a harness case exports a hosted install's four settings and asserts that a child sees none of them. Measured with OPENDOX_INSTALL_MODE=hosted and OPENDOX_OIDC_ISSUER exported: all 5 cases fail against 9ae5e72's harness and pass here. Mutant "the child keeps the runner's settings" is killed by the harness case in a plain environment. Mutant "the in-process case keeps them" is killed under the exported hosted mode. Full suite: 3126 selected, 3115 passed, 11 skipped, 0 failed. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Arc: neutral-product-standalone-operability Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/standalone_child.py | 11 ++++++++- tests/test_projection_seams.py | 11 ++++++++- tests/test_standalone_generate_path.py | 31 +++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/tests/standalone_child.py b/tests/standalone_child.py index 37d1fe5f..34bbda01 100644 --- a/tests/standalone_child.py +++ b/tests/standalone_child.py @@ -20,6 +20,12 @@ * It is read on threads while it runs (`Child.wait_for_line`), so a server that never exits can still be asked where it serves, and then interrupted (`Child.interrupt`, SIGINT, as Ctrl-C sends). +* IT INHERITS NO RUNTIME SETTING. Every `OPENDOX_*` name the runtime reads + (`opendox.runtime.config.SETTING_NAMES`) is taken out of the child's + environment, so an `OPENDOX_INSTALL_MODE=hosted` or a broker issuer the + runner happens to export cannot make a `generate-and-open --local` child + refuse before the case it exists for (plan 034 T070; Copilot review of + openDox-code#67). * CTRL-C REACHES IT AS IT WOULD AT A TERMINAL, whatever the runner's own disposition. The same `sitecustomize` sets SIGINT back to Python's KeyboardInterrupt handler. A runner started as a background job @@ -140,7 +146,10 @@ def __init__(self, workdir: Path, module: str, *args: str) -> None: blocker.mkdir(parents=True, exist_ok=True) (blocker / "sitecustomize.py").write_text(_BLOCKER, encoding="utf-8") self.refused_log = workdir / "refused-imports.log" - env = dict(os.environ) + from opendox.runtime.config import SETTING_NAMES + + env = {name: value for name, value in os.environ.items() + if name not in SETTING_NAMES} env.pop("PYTHONUNBUFFERED", None) env["PYTHONPATH"] = os.pathsep.join( [str(blocker), *filter(None, [env.get("PYTHONPATH")])]) diff --git a/tests/test_projection_seams.py b/tests/test_projection_seams.py index 766a848a..02c5ec65 100644 --- a/tests/test_projection_seams.py +++ b/tests/test_projection_seams.py @@ -1392,7 +1392,16 @@ def test_a_given_source_option_is_resolved_and_an_unset_one_is_not_passed( def test_generate_and_open_refuses_an_empty_source_option_before_its_run_dir( - tmp_path, capsys) -> None: + tmp_path, capsys, monkeypatch) -> None: + """In process, so the runtime settings the runner exports are scrubbed + first, as the doxBench entrypoint fixture does: an exported + `OPENDOX_INSTALL_MODE=hosted` or broker issuer would otherwise make + `--local` refuse before the empty option is reached (Copilot review of + openDox-code#67).""" + from opendox.runtime.config import SETTING_NAMES + + for name in SETTING_NAMES: + monkeypatch.delenv(name, raising=False) calls: list = [] _declaring_generator(calls) repo = _repository(tmp_path) diff --git a/tests/test_standalone_generate_path.py b/tests/test_standalone_generate_path.py index b71f3650..a199de51 100644 --- a/tests/test_standalone_generate_path.py +++ b/tests/test_standalone_generate_path.py @@ -22,7 +22,8 @@ refuses `/source/.git/config`, and stops on an interrupt with status 0. 4. `python -m opendox.serve`, the server's own entry point, starts and answers the same way. -5. The harness itself: a child that ignores the interrupt is killed at the +5. The harness itself: a child inherits none of the runner's runtime + settings, and a child that ignores the interrupt is killed at the deadline, and the timeout is raised, so a server that will not stop is reported rather than waited out. And a child stops on the interrupt even when the RUNNER ignores SIGINT, as a suite started as a background job @@ -321,6 +322,34 @@ def test_serve_main_starts_a_server_that_answers_with_no_sibling(tmp_path) -> No # 5 — the harness itself: an ignored interrupt is reported, not waited out # --------------------------------------------------------------------------- +def test_a_child_inherits_none_of_the_runners_runtime_settings( + tmp_path, monkeypatch) -> None: + """The runner exports a HOSTED install's settings, and the child sees + none of them (plan 034 T070; Copilot review of openDox-code#67). The + `--local` cases above would otherwise refuse before they reach what they + test, for a reason that is the runner's configuration and not theirs.""" + from opendox.runtime.config import PREFIX, SETTING_NAMES + + exported = {PREFIX + "INSTALL_MODE": "hosted", + PREFIX + "OIDC_ISSUER": "https://issuer.example.invalid/realms/x", + PREFIX + "OIDC_AUDIENCE": "fixture", + PREFIX + "DATABASE_URL": "postgresql://s@127.0.0.1:1/x"} + for name, value in exported.items(): + monkeypatch.setenv(name, value) + blocker = tmp_path / "sibling-blocker" + blocker.mkdir() + (blocker / "t070_env_probe.py").write_text(textwrap.dedent(""" + import json, os + print(json.dumps(sorted(n for n in os.environ if n.startswith("OPENDOX_"))), + flush=True) + """), encoding="utf-8") + child, status = run_module(tmp_path, "t070_env_probe") + assert status == 0, child.stderr_text() + seen = set(json.loads(child.stdout_text().strip().splitlines()[-1])) + assert not seen & set(SETTING_NAMES), sorted(seen & set(SETTING_NAMES)) + assert set(exported) <= set(SETTING_NAMES) + + def test_a_child_that_ignores_the_interrupt_is_killed_at_the_deadline( tmp_path, monkeypatch) -> None: """The regression path cases 3 and 4 guard: a server that does not stop From d1de1fd90402ab7f3fe65de4cabc67fe6a652040 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:49:17 +0000 Subject: [PATCH 14/15] Fix round: the healthy-local status case sets its schema with make_conninfo (Copilot review) OPENDOX_TEST_DATABASE_URL may be libpq's keyword/value form as well as a URI (the runtime reads both, and schema_selected_by reads the schema out of both). The case appended "?options=..." to it. On the keyword/value form that suffix becomes part of dbname, so the case failed for a reason that was the fixture's spelling, while the fixtures themselves still connected. psycopg.conninfo.make_conninfo now sets "options" on either form. Measured with a keyword/value OPENDOX_TEST_DATABASE_URL: the case failed before and passes now, and it passes on the URI form both times. A mutant that drops the search_path option is killed on both forms. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- tests_runtime/test_install_mode.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests_runtime/test_install_mode.py b/tests_runtime/test_install_mode.py index 24d44eb6..0c5477a4 100644 --- a/tests_runtime/test_install_mode.py +++ b/tests_runtime/test_install_mode.py @@ -377,10 +377,14 @@ def _no_broker(_settings): "install, which has no broker") monkeypatch.setattr(oidc, "build_verifier", _no_broker) - joiner = "&" if "?" in postgres_dsn else "?" - scrubbed.setenv(PREFIX + "DATABASE_URL", - f"{postgres_dsn}{joiner}options=-c%20search_path%3D" - f"{database.schema}%2Cpublic") + # `make_conninfo`, NOT a `?options=` suffix: `OPENDOX_TEST_DATABASE_URL` + # may be libpq's keyword/value form as well as a URI, and a suffix on + # `… dbname=opendox` names the database `opendox?options=…` instead of + # selecting the schema (Copilot review of openDox-code#67). + from psycopg.conninfo import make_conninfo + + scrubbed.setenv(PREFIX + "DATABASE_URL", make_conninfo( + postgres_dsn, options=f"-c search_path={database.schema},public")) scrubbed.setenv(MODE, "local") code, evidence = _run(["runtime", "status", "--probe-timeout", "5"]) assert evidence["database"] == "reachable", evidence From 105f2f1222fbdd19d245110d443c3fc6475ebeb9 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 2 Oct 2026 22:22:13 +0000 Subject: [PATCH 15/15] Fix round: no runtime setting the runner exports reaches a tests_runtime case (Copilot review) Since T070 the runtime reads a selector, OPENDOX_INSTALL_MODE. A runner that exported OPENDOX_INSTALL_MODE=local, as a local install's own shell would, turned every hosted case that leaves it unset into a configuration refusal. Measured: 20 cases failed, 18 in test_runtime_cli.py and 2 in test_migrations_apply.py (status then has no "settings" key, so the case raises KeyError). The scrubbed fixture covered only test_install_mode.py. tests_runtime/conftest.py gains an autouse fixture that clears every name in opendox.runtime.config.SETTING_NAMES before each case, and a case that wants one sets it. OPENDOX_TEST_DATABASE_URL is not a runtime setting, so the harness's own DSN is kept. The production refusal is unchanged. Measured full-suite results: - with OPENDOX_INSTALL_MODE=local and a broker issuer and audience exported: 3126 selected, 3115 passed, 11 skipped, 0 failed; - in a clean environment: the same. A mutant that clears nothing is killed: the 20 cases fail. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- tests_runtime/conftest.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests_runtime/conftest.py b/tests_runtime/conftest.py index 2c6e0b7b..55553539 100644 --- a/tests_runtime/conftest.py +++ b/tests_runtime/conftest.py @@ -57,6 +57,31 @@ ) +@pytest.fixture(autouse=True) +def _no_inherited_runtime_setting(monkeypatch: pytest.MonkeyPatch) -> None: + """No `OPENDOX_*` runtime setting the runner exports reaches a case. + + Since plan 034 T070 the runtime reads a selector, `OPENDOX_INSTALL_MODE`, + and a runner that exports `local` (as a local install's own shell would) + turned every hosted case that leaves it unset into a configuration + refusal: 20 cases in `test_runtime_cli.py` and `test_migrations_apply.py` + (measured; Copilot review of openDox-code#67). So every name the runtime + reads (`opendox.runtime.config.SETTING_NAMES`) is cleared before each + case, and a case that wants one sets it. `OPENDOX_TEST_DATABASE_URL` is + not one of them (see `TEST_DSN_ENV`), so the harness's own DSN is kept. + The production refusal is unchanged. + + An `opendox` that cannot be imported leaves nothing to clear: the case + then fails on its own import, which is the failure worth seeing. + """ + try: + from opendox.runtime.config import SETTING_NAMES + except ImportError: + return + for name in SETTING_NAMES: + monkeypatch.delenv(name, raising=False) + + def in_ci() -> bool: """Whether this run is CI's, by CI's own variable.