fix(tests): collect tests/, and correct the two assertions that never ran (#314)

The issue calls these order-dependent — passing under `pytest`, failing under
`pytest <file>`. They are not. `testpaths` listed `tests/integration`,
`tests/e2e`, `tests/benchmarks` and `tests/perf` but never `tests/` itself, so
the four modules directly under it were not collected by a bare `pytest` at all.
The tests "passed" by not running. 26 tests were in that gap, including a
lifespan hydration test and fifteen audit-log tests.

Replacing the four entries with `"tests"` collects them; the marker filter in
`addopts` still holds e2e and perf out, so nothing new runs in CI that should
not. That leaves exactly the two failures the issue describes, and both are the
test being wrong rather than the code.

`test_host_settings_ignores_env` asserted "HostSettings must NOT read env".
That is not this codebase's contract: precedence is env → DB → default and env
has to keep winning, or an upgrade silently changes a deployment's behaviour.
`HostSettings` declares `env_prefix="SM_"` for that reason. Rewritten as three
tests covering what the prefix actually guarantees — prefixed env wins, an
unprefixed name is ignored, the default applies when neither is set.

`test_session_wins_over_bad_bearer` asserted a valid session rescues a bad
`Authorization: Bearer`. `resolve_user` does the opposite. Keeping the code is
the deliberate call: falling through would make an invalid token
indistinguishable from no token, so a client whose credential expired keeps
working on whatever other identity it carries and its 401s depend on what else
is in the request — while gaining nothing, since the fall-through can only
resolve the session's own identity, which the caller already had. The precedence
is now stated where it is implemented, with a sibling test proving the session
alone still succeeds so the 401 is the header's doing.

Closes #295

Author: antosubash

modules/users/users/provider.py

@@ -74,6 +74,16 @@ class UsersAuthProvider:
     _is_auth_provider = True
 
     async def resolve_user(self, request: Request) -> UserContext | None:
+        # An ``Authorization`` header is an explicit claim, and it decides the
+        # request: a bad token returns None rather than falling through to the
+        # session cookie. Falling through would make an invalid token
+        # indistinguishable from no token, so a client whose credential expired
+        # silently keeps working on whatever other identity it carries and its
+        # 401s depend on what else is in the request. It gains nothing either —
+        # it can only resolve the session's own identity, which the caller
+        # already had. Pinned by ``test_a_bad_bearer_is_not_rescued_by_a_valid
+        # _session``, which asserted the opposite for as long as it went
+        # uncollected.
         auth_header = request.headers.get("authorization", "")
         if auth_header.startswith("Bearer "):
             return await self._resolve_bearer(request.scope, auth_header[7:])

pyproject.toml

@@ -117,7 +117,7 @@ invalid-assignment = "ignore"
 
 [tool.pytest.ini_options]
 asyncio_mode = "auto"
-testpaths = ["framework/cli/tests", "framework/core/tests", "framework/db/tests", "framework/hosting/tests", "framework/testing/tests", "host/tests", "modules/auth/tests", "modules/dashboard/tests", "modules/users/tests", "modules/permissions/tests", "modules/background_tasks/tests", "modules/file_storage/tests", "modules/settings/tests", "modules/feature_flags/tests", "modules/keycloak/tests", "modules/audit_log/tests", "modules/branding/tests", "modules/site_lock/tests", "scripts/tests", "tests/integration", "tests/e2e", "tests/benchmarks", "tests/perf"]
+testpaths = ["framework/cli/tests", "framework/core/tests", "framework/db/tests", "framework/hosting/tests", "framework/testing/tests", "host/tests", "modules/auth/tests", "modules/dashboard/tests", "modules/users/tests", "modules/permissions/tests", "modules/background_tasks/tests", "modules/file_storage/tests", "modules/settings/tests", "modules/feature_flags/tests", "modules/keycloak/tests", "modules/audit_log/tests", "modules/branding/tests", "modules/site_lock/tests", "scripts/tests", "tests"]
 markers = [
     "e2e: end-to-end tests requiring a live browser",
     "perf: performance benchmarks (opt-in; run via `make bench`)",

tests/test_bootstrap_settings.py

@@ -45,11 +45,36 @@ def test_bootstrap_placeholder_secret_blocks_production(monkeypatch):
         BootstrapSettings()
 
 
-def test_host_settings_ignores_env(monkeypatch):
-    # HostSettings must NOT read env — env-sprawl is what we're removing.
+def test_host_settings_reads_its_own_prefixed_env(monkeypatch):
+    """Env beats the default. This test asserted the opposite and never ran.
+
+    ``testpaths`` did not list ``tests/``, so nothing here was collected by a
+    bare ``pytest`` — the stale assertion sat green for as long as it took to
+    notice. The contract it claimed ("HostSettings must NOT read env") is not
+    the one the codebase has: precedence is env → DB → default, and env has to
+    keep winning or an upgrade silently changes a deployment's behaviour
+    (CLAUDE.md § Conventions). ``HostSettings`` declares ``env_prefix="SM_"``
+    for exactly that reason.
+    """
     monkeypatch.setenv("SM_MULTI_TENANT", "true")
-    hs = HostSettings()
-    assert hs.multi_tenant is False  # default wins; env ignored
+    assert HostSettings().multi_tenant is True
+
+
+def test_host_settings_ignores_unprefixed_env(monkeypatch):
+    """What the ``env_prefix`` is actually defending against.
+
+    Without it a bare ``HostSettings()`` would read unprefixed names, and
+    ``LOG_LEVEL`` in particular is a common variable that has nothing to do
+    with this app.
+    """
+    monkeypatch.delenv("SM_MULTI_TENANT", raising=False)
+    monkeypatch.setenv("MULTI_TENANT", "true")
+    assert HostSettings().multi_tenant is False
+
+
+def test_host_settings_default_wins_when_env_is_unset(monkeypatch):
+    monkeypatch.delenv("SM_MULTI_TENANT", raising=False)
+    assert HostSettings().multi_tenant is False
 
 
 def test_host_settings_default_locale_must_be_supported():

tests/test_principal_resolver_integration.py

@@ -78,14 +78,38 @@ async def test_no_auth_header_on_api_returns_401(pat_client):
 
 
 @pytest.mark.anyio
-async def test_session_wins_over_bad_bearer(authenticated_client):
-    """A valid session cookie + Bearer bad -> 200 via session; resolver not consulted.
-
-    The ``authenticated_client`` fixture already carries an admin session cookie;
-    here we additionally send a bad bearer to prove the session path wins.
-    Endpoint is any admin-readable, non-user-enumerating route."""
+async def test_a_bad_bearer_is_not_rescued_by_a_valid_session(authenticated_client):
+    """An explicitly presented credential that is invalid fails the request.
+
+    This test previously asserted the opposite — that the session cookie wins
+    and "the resolver is not consulted" — and had never run: ``testpaths`` did
+    not list ``tests/``, so a bare ``pytest`` collected nothing here. The
+    behaviour it described is not what ``UsersAuthProvider.resolve_user`` does;
+    the ``Authorization`` header is checked first and a bad token returns
+    ``None`` without falling through.
+
+    Keeping the code and correcting the test is the deliberate call. Falling
+    through would make an invalid token indistinguishable from no token at all,
+    so a client whose credential has expired or been revoked silently keeps
+    working on whatever other identity it happens to carry, and its 401s become
+    dependent on what else is in the request. Nothing is gained by the
+    fall-through either: it can only ever resolve the session's own identity,
+    which the caller already had.
+
+    The narrow cost is a browser that attaches a stale ``Authorization`` header
+    to a page request. Nothing in this app does that — pages authenticate with
+    the session cookie.
+    """
     resp = await authenticated_client.get(
         "/api/permissions/",
         headers={"Authorization": "Bearer bad"},
     )
+    assert resp.status_code == 401
+
+
+@pytest.mark.anyio
+async def test_the_same_session_succeeds_without_the_bad_header(authenticated_client):
+    """The other half: the session itself is fine, so the 401 above is the
+    header's doing and not a broken fixture."""
+    resp = await authenticated_client.get("/api/permissions/")
     assert resp.status_code == 200