fix: restore main's gates — file-size cap, and a fixture that ignored its own contract - #315
Merged
Merged
Conversation
… its own contract Two failures on main, neither of which any single PR's CI could have caught. **`provider.py` at 305 lines.** #309 and #314 were each green against the main they branched from; squash-merging both put the file over the 300-line cap. Split on the seam already there: `_resolve_bearer` moves to `token_strategy`, which is where `ExpiringDatabaseStrategy` lives. Those two are the only readers of `users_access_token` and they have to apply the same deadline and `session_version` rules — keeping them in one file is what stops them drifting. `provider.py` keeps a one-line delegate so the method stays on the provider's surface. 305 → 265, and `token_strategy` → 164. **`setup_pending_app` boots *with* an administrator.** `UsersModule.on_startup` seeds one from `SM_USERS_BOOTSTRAP_*`, read from the environment *and* from a `.env` on disk. A developer who followed `.env.example` has those set, so the fixture whose entire contract is "an app with no administrator" hands back an app that has two — the setup gate releases, the wizard routes 404, and eleven tests in `framework/hosting/tests` fail. CI has no `.env`, so it never saw this: the failure was local-only, which is the worst shape for a fixture to be wrong in. It also looked like test-ordering noise, because whether it reproduced depended on what else had booted an app first. The fixture now scrubs the bootstrap vars and stubs the dotenv reader for the app it builds, the same way `modules/users/tests/conftest.py` does for its own. Adding that pushed `fixtures.py` over the cap too, so the schema machinery (model imports, alembic heads, table creation) moves to `_schema.py` — that module declares fixtures, this one is what they stand on.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying simple-module-python with
|
| Latest commit: |
afa6eb1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://73cebe84.simple-module-python.pages.dev |
| Branch Preview URL: | https://fix-provider-file-size.simple-module-python.pages.dev |
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
1 participant
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.
Two failures on
mainright now, neither of which any single PR's CI could have caught. Found while merging the #292–#302 batch.1.
provider.pyis 305 lines — the 300-line cap fails onmain#309 and #314 were each green against the
mainthey branched from. Squash-merging both put the file over. This is environment-independent: CI onmainis red for this.Split on the seam already there —
_resolve_bearermoves totoken_strategy.py, whereExpiringDatabaseStrategylives. Those two are the only readers ofusers_access_token, and they have to apply the sameexpires_atandsession_versionrules; keeping them in one file is what stops them drifting apart.provider.pykeeps a one-line delegate so the method stays on the provider's surface and existing callers are untouched.provider.py305 → 265,token_strategy.py→ 164.2.
setup_pending_appbuilds an app that has an administratorThis one is more interesting, and it is why 11 tests in
framework/hosting/testsfail locally.UsersModule.on_startupseeds an administrator fromSM_USERS_BOOTSTRAP_*— read from the environment and from a.envon disk. A developer who followed.env.examplehasSM_USERS_BOOTSTRAP_EMAIL/_PASSWORDset, so the fixture whose entire contract is "An app with no administrator, so the first-run setup gate is engaged" hands back an app with two users. The gate releases,_require_pending_step404s, and every test built on it fails:CI never saw this — no
.envin CI. So it is a local-only failure, which is the worst shape for a shared fixture to be wrong in. Worse, it presented as test-ordering noise: whether it reproduced depended on what else had booted an app first, so it looked like flake rather than a fixture that does not do what it says. I confirmed the mechanism by probing the fixture directly —PENDING: [],USER ROWS: 2.The fixture now scrubs
SM_USERS_BOOTSTRAP_*and stubs the dotenv reader for the app it builds — exactly whatmodules/users/tests/conftest.pyalready does for its own apps. Notesimple_module_testis a published plugin, so consumers hit this too.Adding that pushed
fixtures.pyover the cap as well, so the schema machinery (model imports, alembic head resolution, table creation) moves to_schema.py: that module declares fixtures, this one is what they stand on.Verification
uv run pytest -q— 2914 passed, 60 deselecteduv run pytest framework/hosting/tests/test_setup_password_policy.py framework/hosting/tests/test_setup_routes.pyin isolation — 14 passed (was 11 failed / 3 passed onmain)check_file_size.py— OK (fails onmain)ruff format --check/ruff check/ty check framework modules host— passWorth noting for the future
Both failures come from the same gap: PR CI validates each branch against the
mainit forked from, not against themainit will land on. Six PRs merged in sequence today and the fifth and sixth interacted. If you want, a merge queue or a required "update branch before merge" would close it.