Repository navigation
fix: settings precedence, settings editor types, signup role, S3 download names, scaffolded worker (#486–#490) - #525
Merged
Merged
Conversation
…486) build_settings passed every DB override as an init kwarg, which pydantic-settings ranks above its env source, so for a class that reads env (BackgroundTasksSettings and its SM_BG_TASKS_* vars) a value saved in the admin UI silently beat the operator's variable. Drop overrides for fields the class's own non-init sources supply. DbBackedSettings has no such sources and is unchanged. The admin 'source' badge now reports env over db to match. Closes #486 Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
The generic editor sent a dict field's textarea text as a string and rendered an unset int | None as the literal 'null'. The field view now carries input_type/nullable (X | None unwrapped to X), so optional scalars get their scalar control with empty meaning None, and list/dict fields are parsed as JSON before they are sent. The server stores the validated value rather than the raw input, so a validator that json.loads text no longer leaves a JSON string in the store. Also relabels the 'DB override shadows env' badge: since #486 the env var wins. And test_put_rejects_and_does_not_persist_* read a separate engine, so its 'nothing stored' assertion could not fail; it now reads the app's DB. Closes #487 Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
On a single-tenant install a signup held no role, so it lacked every role-mapped permission (api_tokens.use included) until an admin assigned one. on_after_register now attaches the 'user' role for request-bound registrations (signup form, OAuth provisioning). Admin create/invite call create() without a request and keep exactly the roles the admin picked. The tests run single-tenant on purpose: api_tokens.use is also mapped onto tenant roles, which hid the bug under the default multi_tenant=True fixture. Closes #488 Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
presigned_get_url signed only Bucket/Key, so on S3 the browser was redirected to a response carrying the object's own metadata and saved the file under its opaque storage key. The backend protocol gains optional content_disposition/content_type, which S3 signs as ResponseContentDisposition/ResponseContentType; the authenticated download asks for attachment, the public route for inline (active content is still streamed there, never redirected). The streamed authenticated download built its header by hand, which 500'd on a non-Latin-1 filename and broke on a quote; it now uses the shared RFC 6266/5987 helper. The download result types and that helper move to file_storage.downloads (service.py was at the 300-line cap); both remain importable from file_storage.service. Closes #489 Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
…490) The run_worker.py that smpy new generates still built Celery from BackgroundTasksSettings() (env only) and resolved .env from a hardcoded path, so a scaffolded app's worker ignored routing/tuning saved in the admin UI and could read a different .env from its web process. The repo's own script had been fixed in #481; the template had not, which is the drift to remove. The bootstrap (.env via find_env_file, host settings, sync DB URL, default tenant, DB-hydrated settings) now lives in background_tasks.runner. Both scripts are the same three-line call, and a test asserts the two files stay identical. Closes #490 Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
Deploying simple-module-python with
|
| Latest commit: |
a0c3afc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee0d623d.simple-module-python.pages.dev |
| Branch Preview URL: | https://ccr-8a6b8262-wxns8z.simple-module-python.pages.dev |
…#490 review) The fallback only applied when find_env_file() returned the bare ./.env, so celery launched from inside another checkout loaded nothing; and when it did apply, only os.environ saw the file while BootstrapSettings anchored a relative sqlite path at the cwd. The fallback is now adopted by setting SM_PROJECT_ROOT (never overriding an explicit one), so every later reader resolves the same project. Tests cover the fallback branch and restore sync_db's process-wide tenant state; docs no longer say a stored value beats SM_BG_TASKS_*. Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
…488) - A save to a field the environment sets is refused with a 422 on that field. Hot reload built cls(**merged), where init kwargs outrank env, so the stored value went live in the saving process only, while every restart and every other worker hydrated the env value. - The editor's 'env' label now uses env_supplied_fields, the same rule hydration applies, so a value from the class's .env, a secrets file or an alias is labelled env rather than db. - Only json fields store their validated dump; scalars keep the submitted value, so a serializer such as SecretStr's can never write asterisks into the store. - FieldSource shows a bool | None field's description like a plain bool. - The default 'user' role is looked up, never created: a missing role means an admin deleted it, and a public signup must not undo that. This also removes the concurrent-signup insert race. Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
SM_REDIS_URL reaches broker_url/result_backend through their default factories, so pydantic's env source never reported it and a stored value won, contrary to env -> DB -> default. Settings classes can now name such env-seeded fields (env_seeded_fields), which env_supplied_fields adds; the hydrator then drops the stored override and the editor refuses to save one. SM_BG_TASKS_BROKER_URL / _RESULT_BACKEND still beat SM_REDIS_URL. Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
antosubash
marked this pull request as ready for review
October 18, 2026 11:41
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. |
This was referenced Oct 18, 2026
…p is open (#488 review) OAuth provisioning is not gated by allow_signup the way /register is, so attaching the user role there handed file_storage read/write and api_tokens.use to any account at a configured provider, even with signups closed. The default role is now attached to an OAuth-provisioned account only when allow_signup is on; local registration is unchanged (it is already gated). Whether OAuth provisioning itself happens is untouched. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
Self-registered users are given the 'user' role by lookup, but no migration ever inserted it, so on a fresh install every signup came out roleless and #488 did nothing. b5018b617142 inserts it (inline values, chained onto the outbox revision) unless a role with that name or id already exists; the downgrade is a no-op so an assigned role is never deleted. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
A json-typed field was stored from validated.model_dump(mode="json"), which masks SecretStr as asterisks inside `SecretStr | None` and containers such as dict[str, SecretStr]; every restart and other worker then hydrated the asterisks as the credential. The validated value now has SecretStr / SecretBytes unwrapped before it is made JSON-able. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
On an existing install a missing 'user' role most likely means an admin deleted it; re-inserting it on upgrade would silently restore the access they removed. b5018b617142 now seeds the role only when users_user is empty (and no role with that name or id exists); installs with users keep the manager's no-role warning path until an admin creates it. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
review) fastapi-users commits the user row inside create(), before on_after_register. The default role was only flushed, so a failure after it (event publish, the verification email) rolled the role back and left a committed, roleless account that could not re-register. The role link is now committed as soon as it is attached. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
…review) Json fields now store the real credential, but masking was name- and URL-based only, so `token: SecretStr | None` or `tokens: dict[str, SecretStr]` would leave the module editor, its API, the row listing, the tenant view and the known-keys list in clear text. A field whose annotation holds SecretStr/SecretBytes anywhere (unions, containers, nested models) is now masked with is_secret=True: directly in the editor view, and by its registered `<package>.<field>` key everywhere a stored row is shown. Since the editor flags it secret, a PUT echoing the mask is dropped and the stored secret is kept. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
#486 review) env_supplied_fields reported whatever a class's env source answered. A plain BaseSettings with no env_prefix reads bare, case-insensitive names, so USER/HOME/PATH pinned fields called user/home/path: admin saves got 422 and stored values were ignored. Such a class now reports no source-supplied fields (explicit env_seeded_fields still count), and the editor's source docstring states the rule again. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
It sat after the first statement, so it was a dead string expression. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
…installs only Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
…eachable unwrap branch Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
The module settings editor locked nothing for an env-pinned field, so editing it alongside other fields 422'd the whole save. Pinned controls are now disabled; the server still refuses a crafted write. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
require_verification is read nowhere: login refuses any unverified account (auth_local/api.py), so hiding the notice when the setting is off promised a signup could sign in straight away when it could not. The notice is shown again; the new default-role copy stays. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
pydantic's error JSON carries the rejected `input`, which for a broker URL or a secret field is the credential itself. The UI renders only `msg`, so dropping `input` costs nothing. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
- An env-set field is refused only when the write would change it, so a client echoing the GET payload back is not 422'd for fields it left alone. - `dict | None`, `list | None` and `Model | None` get the JSON editor instead of a single-line text box. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4
antosubash
pushed a commit
that referenced
this pull request
Oct 19, 2026
Resolve the CLAUDE.md conflict by keeping the rewrite and folding in main's additions from #550 and #525: pytest-xdist parallel runs (`-n auto --dist loadgroup`, `PYTEST_WORKERS=0`), the serial Postgres suite, single tests running in-process, the test plugin's reduced password-hash work factor and `SM_TEST_REAL_PASSWORD_HASHING`, and the xdist collection rules (deterministic parametrize ids, per-worker names). Claude-Session: https://claude.ai/code/session_01TJ3Gy55Ngyuj5o4UakoF2v
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.
One commit per issue, plus follow-up commits from code review. Closes #486, closes #487, closes #488, closes #489, closes #490.
#486: env beats a stored override
background_tasksis aDbBackedSettingswith no env source, so e.g.SM_FILE_STORAGE_MAX_UPLOAD_BYTESis ignored entirely and the repro as written can't happen. The real bug wasBackgroundTasksSettings(SM_BG_TASKS_*), where a stored row beat the env var.build_settingsnow drops overrides for fields that the class's own non-init sources (env,.env, secrets) supply. It doesn't probe withcls(), because the production broker validator can reject the env-only view.SM_REDIS_URLnow wins too. It reachesbroker_url/result_backendthrough their default factories, which no env source can see, so classes can name such fields viaenv_seeded_fields(). Order:SM_BG_TASKS_BROKER_URL/_RESULT_BACKEND→SM_REDIS_URL→ stored value → default.cls(**merged), so a stored value would otherwise go live in the saving process only..env, secrets files and aliases). "overridden in DB · shadows env" became "stored in DB · env var wins".#487: generic editor JSON / optional fields
input_type+nullable(X | Noneunwrapped toX).int | Nonegets a number box where empty meansNone,bool | Nonegets a Not set/On/Off select, andstr | Nonegets a text box.JsonInputsends parsed JSON for list/dict fields, flags invalid JSON inline, and resyncs after Revert.json.loadstext no longer leaves a JSON string in the store. Scalars keep the submitted value, so aSecretStrdump (asterisks) can never be stored.test_put_rejects_and_does_not_persist_*read a separate engine, so its "nothing stored" assertion could never fail.#488: signups get the
userroleon_after_registerattachesuserfor request-bound registrations (signup, OAuth provisioning). Admin create/invite pass no request and keep exactly the roles the admin picked, including none.api_tokens.useis also mapped onto tenant roles, which hides the bug under the defaultmulti_tenant=Truefixture.#489: presigned downloads keep the filename
StorageBackend.presigned_get_urlgains keyword-onlycontent_disposition/content_type, which S3 signs asResponseContentDisposition/ResponseContentType. Authenticated downloads useattachment; the public route usesinline(active content is still streamed, never redirected). An end-to-end test against moto checks the headers.Content-Dispositionby hand, which 500'd on a non-Latin-1 filename (reproduced) and broke on a".service.pywas at exactly the 300-line cap, so the download result types and header helpers moved tofile_storage.downloads. They are still importable fromfile_storage.service.#490: scaffolded worker
background_tasks.runner.build_runner_celery().scripts/run_worker.pyand the template are the same file, and a test asserts they stay identical..envfallback applies whenever nothing was found, and is adopted by settingSM_PROJECT_ROOT, so the SQLite anchor follows the same project instead of the cwd.Review findings left as is
env_supplied_fields: it costs a few env/file reads per hydration, and a cache would go stale under env changes.ensure_secret_keyviamerge_host_settings(): this predates this PR.Verification
make lintclean. JS 585/585. The full Python suite (4489 passed) ran on the five issue commits; the affected suites were re-run on each follow-up commit (latest: background_tasks + settings, 718 passed).Review round 2 (code review, adversarial security review, Codex, browser QA)
Fixed on this branch, each with a regression test that failed before the fix:
userrole only whileallow_signupis on. OAuth provisioning ignoresallow_signup, so any account at a configured provider would otherwise get file_storage read/write over every file on a single-tenant install.userrole. New data migrationb5018b617142(on the mainline tip, so no new head) seeds it only on a fresh install (no rows inusers_user), so a role an admin deleted is never brought back. Existing installs opt in by creating the role (CHANGELOG).SecretStr | None,dict[str, SecretStr]) were stored as asterisks. They are now stored intact, and any field typed as a secret is masked on every read path whatever its name. A PUT that echoes the mask keeps the stored value.BaseSettingsclasses are no longer pinned by bareHOME/USER/... env vars.require_verificationis on.Filed separately (pre-existing): #543 OAuth provisioning ignores
allow_signup· #544 file_storage has no owner scoping · #545 the active-content type denylist has gaps.Verified:
alembic checkreports drift onusers_access_token.expires_at. It reproduces onmain, so it predates this PR./ship (round 3 of review)
Optimize, then 2 converged rounds of code review and browser QA.
dict | None,list | NoneandModel | Noneget the JSON editor.require_verificationis read nowhere, and login always requires verification (users: require_verification setting is never read — unverified signups can never log in #547).require_verificationis a dead setting) and Settings validation gaps: negative file_storage quota_bytes, soft task time limit above the hard limit #548 (settings validation gaps).make lintpasses, the full Python suite passes (4509), and the JS suite passes.https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm
https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4