test(coverage): close 20 audited test gaps + fix 4 bugs surfaced - #151
Merged
Conversation
…lience gaps
Adds 23 new test files (≈90 new tests) and fixes 4 real bugs surfaced while
writing them.
New test surface:
* Security: negative-authz sweep across every protected endpoint
(users/permissions/settings/feature_flags/background_tasks); SameSite=Lax
+ HttpOnly assertion on the session cookie; safe_referer_or_root direct
unit tests with 14 adversarial Referer vectors; invite token single-use,
reset-token-invalidated-after-password-change, disabled-user can't accept
invite.
* Framework contracts: middleware LIFO order snapshot for both
multi_tenant=True/False; lifespan startup forward / shutdown reverse;
check_migrations drift-detection; production-strict module discovery
wiring; Postgres schema-per-module (live test skipped without
SM_POSTGRES_TEST_URL).
* DB mixins: direct AuditMixin/SoftDeleteMixin/VersionedMixin/composite
tests.
* Operational resilience: sweep_stuck_tasks worker-crash recovery;
filesystem put failure leaves no DB row; compensation-delete failure
doesn't mask the original error; feature-flags hydrate-failure falls back
to registry defaults.
* Coverage breadth: dashboard/settings view-route smoke tests; React
StatCard/SectionTitle unit tests; dotenv parser / environments constant
/ case helpers / _env helper unit tests; check_hardcoded_strings rules
test; scaffold rollback on partial failure.
Bugs fixed:
* Settings API and per-module-settings API had no permission enforcement —
any authenticated user could read or rewrite system settings including
encrypted secrets. Added RequiresPermission gates on every route.
* check_hardcoded_strings silently skipped every line containing a string
token (the very thing it was meant to flag); replaced with AST-based
docstring detection. The fix surfaced 6 real magic-string violations
(depends_on=["Users"], inertia.render("Settings/Browse"), etc.) which
are now using named constants.
* to_snake_case("My Feature") returned "my__feature" instead of the
documented "my_feature"; collapsed runs of underscores.
* create_module left a partial scaffold on disk after a mid-pipeline
failure; added rollback that nukes the destination iff we created it.
Also adds a non-admin `user_client` fixture in modules/users/tests/conftest.py
and updates feature_flags.on_startup to tolerate a hydrate failure by
falling back to registry defaults.
https://claude.ai/code/session_01Dz6X3jfHhq4suPgS9YpNby
Deploying simple-module-python with
|
| Latest commit: |
6f1534a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee6c555b.simple-module-python.pages.dev |
| Branch Preview URL: | https://claude-review-test-coverage.simple-module-python.pages.dev |
Round-trip of the three review-agents from /simplify. All findings are non-blocking cleanups; net change is mostly mechanical. * Move duplicated ``sync_sqlite`` fixture from ``test_sweep_stuck.py`` to ``background_tasks/tests/conftest.py`` so future sync-DB tests can share it. ``test_signals.py`` still has its own (function-local — pytest's resolution prefers it), no regression. * Reuse ``_FakeEntryPoint`` / ``_patch_entry_points`` / ``_boom_loader`` from ``framework/core/tests/test_discovery.py`` in ``test_strict_discovery_wiring.py`` via a one-line sys.path side-load (no ``__init__.py`` lives in these test dirs). * Tighten ``pytest.raises((RuntimeError, OSError))`` to ``pytest.raises(RuntimeError, match="DB write failure")``. To make that contract real, the file_storage service now swallows the cleanup exception (after logging) so the original upload error reaches the caller — Python's ``raise`` would otherwise be clobbered when ``backend.delete()`` raised during compensation. * Drop the POST-vs-GET fallback in ``test_session_cookie_security.py``; a GET / reliably trips ``SessionMiddleware.save()``. * Reduce the ``test_repeated_upserts_last_write_wins`` loop from 100 to 5 iterations — the insert-vs-update branch stabilises after two, and 100 sequential aiosqlite commits is wasteful. * Hoist ``import logging`` to module-scope in ``feature_flags/module.py``; keep ``FeatureFlagService`` deferred (circular-import avoidance). * Trim "the audit found..." narration from four test docstrings — that belongs in the commit log, not the tree. No new bugs found; no production behaviour changed except the file_storage compensation, which now reliably surfaces the trigger exception (the test verifies this). https://claude.ai/code/session_01Dz6X3jfHhq4suPgS9YpNby
There was a problem hiding this comment.
Pull request overview
This PR focuses on closing audited test-coverage gaps across the framework/modules while fixing several bugs uncovered by the new tests (notably: Settings API authorization gating, hardcoded-string linting correctness, file-storage compensation robustness, CLI scaffolding rollback, and snake-case normalization).
Changes:
- Added broad new test coverage across hosting, DB, CLI, and multiple modules (authz, redirects, cookie security, middleware/lifespan order, migrations, mixins, background recovery, storage failure modes, etc.).
- Fixed production issues surfaced by tests (Settings permission enforcement, hardcoded-string script docstring handling, file-storage cleanup error masking, scaffolding rollback, snake-case underscore collapsing).
- Reduced “magic string” usage by introducing named constants for module dependencies/pages in several modules.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/test_check_hardcoded_strings.py | Adds unit tests pinning each magic-string lint rule and skip behavior. |
| scripts/check_hardcoded_strings.py | Fixes docstring detection to avoid disabling rules by skipping all string-token lines. |
| packages/ui/src/components/StatCard.test.tsx | Adds smoke tests validating StatCard render behavior. |
| packages/ui/src/components/SectionTitle.test.tsx | Adds smoke tests validating SectionTitle heading/slots/description. |
| modules/users/tests/test_negative_authz.py | Adds a negative-auth sweep ensuring protected endpoints return 403 to non-admins. |
| modules/users/tests/test_invite_reuse.py | Adds invite/reset token lifecycle regression tests. |
| modules/users/tests/conftest.py | Adds user_client fixture (standard user session) for authz testing. |
| modules/settings/tests/test_concurrent_overrides.py | Adds tests pinning override semantics under repeated/interleaved operations. |
| modules/settings/settings/endpoints/views.py | Replaces inline Inertia page strings with file-private constants. |
| modules/settings/settings/endpoints/module_api.py | Adds RequiresPermission gates to module-settings admin endpoints. |
| modules/settings/settings/endpoints/api.py | Adds RequiresPermission gates to all Settings API routes. |
| modules/permissions/permissions/module.py | Replaces depends_on=[\"...\"] literals with named module constants. |
| modules/permissions/permissions/constants.py | Introduces _MODULE_* constants for module dependency names. |
| modules/file_storage/tests/test_upload_failure_modes.py | Adds tests for backend put failures, cleanup failures, and oversize uploads. |
| modules/file_storage/file_storage/service.py | Ensures cleanup failures don’t mask the original DB-write error; logs cleanup failure. |
| modules/file_storage/file_storage/module.py | Replaces module dependency literal with a named constant. |
| modules/file_storage/file_storage/constants.py | Adds _MODULE_SETTINGS dependency constant. |
| modules/feature_flags/tests/test_store_outage_fallback.py | Adds test asserting boot continues with registry defaults when hydrate fails. |
| modules/feature_flags/feature_flags/module.py | Wraps hydrate in try/except; logs and continues on store outage. |
| modules/dashboard/tests/test_view_routes.py | Adds view-route smoke tests (dashboard/settings/feature_flags + anon redirect). |
| modules/background_tasks/tests/test_sweep_stuck.py | Adds deterministic tests for stuck-task sweep behavior and env cutoff. |
| modules/background_tasks/tests/conftest.py | Adds sync_sqlite fixture for sync DB test isolation. |
| modules/background_tasks/background_tasks/module.py | Replaces module dependency literal with a named constant. |
| modules/background_tasks/background_tasks/constants.py | Adds _MODULE_USERS dependency constant. |
| framework/hosting/tests/test_strict_discovery_wiring.py | Adds tests pinning strict module discovery wiring by environment. |
| framework/hosting/tests/test_session_cookie_security.py | Adds test pinning SameSite=Lax + HttpOnly on the session cookie. |
| framework/hosting/tests/test_redirects.py | Adds direct unit tests for safe_referer_or_root hostile referer vectors. |
| framework/hosting/tests/test_middleware_order.py | Adds tests pinning middleware pipeline order for single/multi tenant. |
| framework/hosting/tests/test_lifespan_order.py | Adds tests for module startup order and reverse shutdown order. |
| framework/hosting/tests/test_check_migrations.py | Adds tests for migration drift behavior + no-alembic sentinel. |
| framework/db/tests/test_postgres_schema_per_module.py | Adds Postgres schema-per-module test (skipped without PG URL) + metadata checks. |
| framework/db/tests/test_mixins.py | Adds direct unit tests for DB mixins and composition compatibility. |
| framework/core/tests/test_environments.py | Adds tests for NON_PROD_ENVIRONMENTS invariants. |
| framework/core/tests/test_dotenv.py | Adds direct unit tests for dotenv parsing/loading and env helpers. |
| framework/cli/tests/test_scaffold_rollback.py | Adds tests asserting scaffold rollback on partial failures. |
| framework/cli/tests/test_env_helper.py | Adds tests for .env mutation helper behavior and idempotency. |
| framework/cli/tests/test_case.py | Adds direct tests for case conversion helpers (snake/kebab/pascal). |
| framework/cli/simple_module_cli/scaffolding.py | Adds rollback to remove newly-created dest directory on scaffold failure. |
| framework/cli/simple_module_cli/case.py | Fixes to_snake_case to collapse underscore runs (prevents double separators). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+35
to
+41
| A GET ``/`` reliably trips ``SessionMiddleware.save()`` because every | ||
| request flows through the middleware stack regardless of route handler. | ||
| """ | ||
| resp = await client.get("/") | ||
| raw = _set_cookie_for("session", resp) | ||
|
|
||
| assert raw, "GET / didn't set the session cookie — has SessionMiddleware been removed?" |
Comment on lines
+28
to
+34
| _CORE_TESTS = Path(__file__).resolve().parents[2] / "core" / "tests" | ||
| sys.path.insert(0, str(_CORE_TESTS)) | ||
| from test_discovery import ( # noqa: E402 # ty: ignore[unresolved-import] | ||
| _boom_loader, | ||
| _FakeEntryPoint, | ||
| _patch_entry_points, | ||
| ) |
Comment on lines
+39
to
+50
| sync_db._engine = None | ||
| sync_db._session_factory = None | ||
|
|
||
| factory = sync_db.get_sync_session_factory() | ||
| TaskExecution.metadata.create_all(factory.kw["bind"]) | ||
|
|
||
| yield db_file | ||
|
|
||
| if sync_db._engine is not None: | ||
| sync_db._engine.dispose() | ||
| sync_db._engine = None | ||
| sync_db._session_factory = None |
Comment on lines
+56
to
+58
|
|
||
| monkeypatch.setattr( | ||
| ff_module, "FeatureFlagService", lambda *a, **kw: _BoomService(), raising=False |
* test_session_cookie_security.py — hit /dashboard/ instead of /. ``GET /`` doesn't touch ``request.session``; the cookie was emitted incidentally by other middleware that read the session, which is brittle. AuthMiddleware deterministically writes ``session["next"]`` before redirecting unauthenticated requests to a protected route, guaranteeing a Set-Cookie regardless of how other middleware evolves. * test_strict_discovery_wiring.py — load test_discovery.py via ``importlib.util.spec_from_file_location`` instead of mutating ``sys.path``. Adding ``framework/core/tests`` to sys.path exposed every ``test_*.py`` in there for the rest of the session and could collide with similarly-named test modules elsewhere. * background_tasks/tests/conftest.py — use ``sync_db.dispose_sync_engine()`` for setup and teardown. The previous manual reset cleared ``_engine`` and ``_session_factory`` but not ``_url_override``, so a prior call to ``set_database_url(...)`` could keep shadowing the ``SM_DATABASE_URL`` the fixture sets. * feature_flags/tests/test_store_outage_fallback.py — patch ``feature_flags.service.FeatureFlagService`` instead of ``feature_flags.module.FeatureFlagService``. ``on_startup()`` does the import inside the method body, so the module-attr patch was a no-op; the test was incidentally passing because ``MagicMock(...)`` failed to await later, not because ``_BoomService`` ran. https://claude.ai/code/session_01Dz6X3jfHhq4suPgS9YpNby
…37277/git/antosubash/simple_module_python into claude/review-test-coverage-koc2T
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
Closes every gap identified in the test-coverage audit on this branch. 23 new test files (≈90 new tests; 1161 py + 16 js passing, lint green) and 4 real bugs the new tests surfaced.
New test surface
modules/users/tests/test_negative_authz.py(+user_clientfixture)framework/hosting/tests/test_redirects.pysafe_referer_or_rootagainst 14 adversarial Referer vectorsmodules/users/tests/test_invite_reuse.pyframework/hosting/tests/test_session_cookie_security.pyframework/hosting/tests/test_strict_discovery_wiring.pyapp_builderpassesstrict=not is_developmentframework/hosting/tests/test_middleware_order.pyframework/hosting/tests/test_lifespan_order.pyframework/hosting/tests/test_check_migrations.pyframework/db/tests/test_mixins.pyframework/db/tests/test_postgres_schema_per_module.pySM_POSTGRES_TEST_URLmodules/settings/tests/test_concurrent_overrides.pymodules/background_tasks/tests/test_sweep_stuck.pymodules/file_storage/tests/test_upload_failure_modes.pymodules/feature_flags/tests/test_store_outage_fallback.pymodules/dashboard/tests/test_view_routes.pypackages/ui/src/components/{StatCard,SectionTitle}.test.tsxframework/core/tests/test_dotenv.py,test_environments.py,framework/cli/tests/test_case.py,test_env_helper.pyscripts/tests/test_check_hardcoded_strings.pyframework/cli/tests/test_scaffold_rollback.pyBugs fixed
modules/settings/settings/endpoints/{api,module_api}.py). Any authenticated user could read or rewrite system settings, including encrypted secrets likereset_password_token_secret. AddedRequiresPermission(PERM_VIEW/CREATE/EDIT/DELETE)to every route.check_hardcoded_strings.pysilently disabled every rule by skipping any line containing a string token — which is exactly the lines the rules want to flag. Rewrote the docstring detection on AST instead oftokenize. The fix surfaced 6 real magic-string violations (depends_on=["Users"],inertia.render("Settings/Browse"), etc.) which are now using named_MODULE_*/_PAGE_*constants.to_snake_case("My Feature")returned"my__feature"(double underscore), contradicting the docstring. Collapsed runs of underscores.create_moduleleft partial scaffold on disk after mid-pipeline failure. Added rollback that removes the destination iff we created it.What was not changed
SM_POSTGRES_TEST_URLset — left as skip.make doctornot run — relevant invariants are covered by existing tests.Test plan
make lint— greenuv run pytest— 1161 passed, 1 skipped (PG), 2 deselectednpx vitest run— 16 passed (4 files)feature_flags.on_startupfallback is the desired contract (alt: hard-fail on DB outage)https://claude.ai/code/session_01Dz6X3jfHhq4suPgS9YpNby
Generated by Claude Code