test: update security audit tests for current schemas - #21
Merged
Merged
Conversation
PR #12 added tests/test_security_edge_cases.py against a schema/session shape that has since evolved; main's CI has been red since PR #12+#20 merged. Root causes, each a stale test assumption, none a production issue: - test_fastapi_app_registers_both_audit_routers: newer FastAPI/ Starlette wraps each include_router() call in an _IncludedRouter object with no .path of its own (the real sub-routes live on .original_router.routes) instead of flattening them into app.routes directly. Reproduced only against a freshly-installed environment matching what CI's unpinned fastapi/starlette resolve to today (not reproducible against this checkout's older installed versions) -- confirmed via an isolated venv. Fixed with a version-robust path collector that walks both shapes; test's actual intent (every audit router got registered) is unchanged. - test_audit_event_schema_supports_orm_attributes_and_nullable_identity / test_audit_event_list_response_preserves_pagination_contract: AuditEventOut gained a required tenant_scope field, and AuditEventListResponse gained source/source_availability/ generated_at/source_checked_at/freshness/retention/warnings (the Track E2/E3 source-of-truth reader/freshness/retention reporting) since this test file was written. Updated both fixtures to the current, real contract (values modeled on api/routes_audit_events.py's own construction site), asserting the same pagination/identity behavior as before plus the new fields. - test_get_db_closes_session_after_normal_iteration / test_get_db_closes_session_when_consumer_raises: db/session.py's V2-003 reader/writer split repointed get_db() at ReaderSessionLocal; these tests still patched the now-unrelated SessionLocal (the writer factory worker/main.py uses), so the monkeypatch silently did nothing and get_db() built a real session. Patched the correct name. test_worker_nogroup_recovery.py::test_recreation_attempt_is_rate_limited_not_every_failure is deliberately NOT touched here. It exposes a real, if currently low-severity, bug in worker/main.py: the NOGROUP recreate rate-limiter compares time.monotonic() against a 0.0 "never attempted" sentinel, but time.monotonic()'s epoch is explicitly unspecified by Python's own docs -- on a freshly-booted host/container where monotonic() hasn't yet exceeded WORKER_NOGROUP_RETRY_BACKOFF_SECONDS (default 2.0s in production; the test's own 9999s override makes this reliably reproduce on any CI runner), the very first legitimate recreate attempt after a NOGROUP is silently skipped. Real-world impact is narrow given the 2.0s default, but the fix (a None sentinel, not 0.0) is production code and explicitly out of scope for this test-maintenance PR -- left failing/unmodified, reported separately for its own authorized bug-fix PR. Focused: 20 passed (tests/test_security_edge_cases.py), reproduced against both this checkout's installed FastAPI/Starlette and a fresh venv matching CI's unpinned versions. Full suite (CI's ignore scope): 495 passed, 23 skipped, 0 failed -- 491 (baseline's 4 failing + 491 passing) + the 4 now-fixed = 495; worker_nogroup's rate-limit test isn't in that count as newly-fixed -- it already passes in this environment for the same monotonic-uptime reason it fails in CI, and remains unmodified. No production source changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
main's CI has been red since PR #12 (test_security_edge_cases.py) merged with tests written against an older schema/session shape. This PR fixes the 4 stale-test root causes and leaves 1 finding deliberately unfixed (see below).Confirmed PR #20 was not the source of any of this — isolating CI runs shows PR #12's own merge commit already had these exact 6 failures before #20 merged.
Root causes
test_fastapi_app_registers_both_audit_routers— newer FastAPI/Starlette wraps eachinclude_router()call in an_IncludedRouterobject with no.path(real sub-routes live on.original_router.routes). Not reproducible against this checkout's installed versions; reproduced by building an isolated venv matching CI's unpinnedfastapi/starlette. Fixed with a version-robust path collector — test's actual intent (every audit router registered) is unchanged.test_audit_event_schema_supports_orm_attributes_and_nullable_identity—AuditEventOutgained a requiredtenant_scopefield since this test was written. Fixture updated.test_audit_event_list_response_preserves_pagination_contract—AuditEventListResponsegainedsource/source_availability/generated_at/source_checked_at/freshness/retention/warnings(Track E2/E3 source-of-truth reporting). Fixture updated to the real contract, values modeled onapi/routes_audit_events.py's own construction site.test_get_db_closes_session_after_normal_iteration/test_get_db_closes_session_when_consumer_raises—db/session.py's reader/writer split repointedget_db()atReaderSessionLocal; these tests still patched the now-unrelatedSessionLocal, so the monkeypatch silently did nothing. Patched the correct name.Deliberately NOT fixed — real production bug, out of scope here
test_worker_nogroup_recovery.py::test_recreation_attempt_is_rate_limited_not_every_failureexposes a genuine bug inworker/main.py: the NOGROUP recreate rate-limiter comparestime.monotonic()against a0.0"never attempted" sentinel, buttime.monotonic()'s epoch is explicitly unspecified by Python's own docs. On a freshly-booted host/container wheremonotonic()hasn't yet exceededWORKER_NOGROUP_RETRY_BACKOFF_SECONDS(default 2.0s in production — low real-world impact — but the test's own 9999s override makes it reproduce reliably on any CI runner), the first legitimate recreate attempt after a NOGROUP is silently skipped. The correct fix (aNonesentinel, not0.0) is production code and out of scope for this test-maintenance PR. Left unmodified — needs its own authorized bug-fix PR.mainwill not be fully green after this PR merges because of this one remaining, unrelated failure.Test plan
pytest -q tests/test_security_edge_cases.py— 20 passed, 0 failed (both against this checkout's installed FastAPI and the CI-matching venv)test_worker_nogroup_recovery.py's rate-limit test isn't newly-fixed by this PR — it already passes in this environment for the same monotonic-uptime reason it fails in CI)ruff check— all checks passedgit diff --check— clean🤖 Generated with Claude Code