Skip to content

fix: address all five open issues (#257, #258, #262, #263, #264) - #265

Merged
antosubash merged 3 commits into
mainfrom
claude/github-issues-07jlrm
Aug 20, 2026
Merged

fix: address all five open issues (#257, #258, #262, #263, #264)#265
antosubash merged 3 commits into
mainfrom
claude/github-issues-07jlrm

Conversation

@antosubash

@antosubashantosubash commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Closes#257, closes#258, closes#262, closes#263, closes#264.

Three commits: the five fixes, a follow-up correcting a regression and finishing two fixes that were half done, then a merge of main. This body describes the final state.


#263 — expression-based index emitted twice under PostgreSQL

The idempotency guard the issue asks for was already in make_process_revision_directives — it just never fired. It scanned only the top level of upgrade_ops.ops, but autogenerate doesn't hand us a flat list: index operations are nested inside a ModifyTableOps group alongside the top-level CreateTableOp. Verified against alembic directly:

UPGRADE:
CreateTableOp users_user
ModifyTableOps users_user
CreateIndexOp users_user ix_users_user_email

So existing_index_names was always empty, the injection re-added an index PostgreSQL had already emitted, and the first make migrate died with DuplicateTable. The scan now recurses through nested op groups; same for DropIndexOp in the downgrade. Name-based and dialect-agnostic, as suggested.

The pre-existing test_does_not_double_inject_when_already_present passed throughout because it built a flat op list — the two new tests use the real nested shape and fail without the fix.

#262 — scaffolded apps migrate the wrong database

Ported this repo's two corrections into the templates:

  • templates/host/alembic.iniscript_location = %(here)s/migrations
  • templates/workspace/Makefilemigrate/migration run from the repo root

That alone was not enough: smpy new itself still ran the baseline autogenerate and upgrade heads with cwd=host/, so the scaffold's own first migration was the odd one out. It now runs from the project root too, with the ini path collapsing to alembic.ini for the flat create-host layout. env.py also logs the resolved database URL (password masked) so the target is never a guess.

Superseded in part by #267, which landed on main while this was open. That PR did the thing this branch explicitly declined as too broad — find_env_file walks up from the cwd, and relative sqlite URLs anchor to the project root — and it is the better fix. The root cause is now handled centrally; what remains here is the scaffold running everything from one cwd, which keeps the ini path, the .env walk's starting point, and the sqlite file all meaning the same thing. The merge commit rewrites the comments and the bootstrap test that had justified themselves with the old cwd-dependent behaviour, since that is no longer true of the merged tree.

#264 — identical tests/test_module.py basename

Went with your option (1): the template is now tests/test___PACKAGE__.py.tpl, and the existing __PACKAGE__ path-rewrite renders it per module — a module named Alpha gets tests/test_alpha.py. Option (2) is ruled out by an existing comment in the scaffolding tests — a shipped tests/__init__.py makes pytest register tests.conftest twice under importlib mode.

Note one residual: this fixes the scaffolded file, but two modules that later add their own tests/test_service.py would collide the same way. Option (3) (importmode = "importlib" in the workspace template) is the fix for that whole class. I left it out because I can't exercise a scaffolded workspace's pytest run in this environment, and it interacts with the conftest behaviour noted above — happy to add it if you want it.

#257get_db commits after the response is sent

Confirmed the mechanism rather than assuming it — with a send-spy around a FastAPI app, teardown ordering is:

  • success: ['response.start', 'teardown'] ← the bug
  • exception: ['teardown-exception', 'response.start']

Took fix option 1 (commit before the response is sent), so every caller benefits rather than just the one workflow. CommitBeforeResponseMiddleware is pure ASGI and intercepts http.response.start, installed innermost (added first — add_middleware is LIFO). That point is deliberate: late enough that FastAPI has already serialized the response, so committing can't interact with attribute expiry; early enough that nothing has reached the client, so a commit failure can still become a 500.

The first attempt at this had a write-loss bug, caught in review and fixed in the second commit. Finalizing at response.start claimed the session one-shot, but work legitimately continues after that message: Starlette runs BackgroundTasks once the body is sent, and a StreamingResponse writes its body afterwards — both on the same session. Those writes flushed and never committed, and get_db's fallback no-op'd because the session was already claimed. Reproduced: a background task adding a row persisted nothing with the middleware and persisted correctly without it.

finalize_session is now re-armable rather than one-shot — each call commits whatever is pending at that moment, and get_db's exit code still runs to catch work done after the response started. The pending-write marker is cleared on settle so a committed session is never re-committed, and a failed commit is settled too so the fallback cannot retry it. Read-only requests still pay for exactly one rollback. The error path is unchanged: FastAPI unwinds the dependency — rolling back — before the error response is sent.

An APIRoute subclass was the other candidate and was rejected: FastAPI 0.141 includes routers lazily via _IncludedRouter, so a route_class set on app.router no longer propagates to included module routers.

Known limitation, documented in the middleware: with more than one enlisted session (Depends(get_db, use_cache=False)), a failure part-way leaves earlier commits durable while the client sees a 500. No cross-session atomicity short of two-phase commit.

#258gen-pages omits wheel modules' components/

Same CSS fix as your PR #259 — that PR is still open, so this branch carries its own copy; drop whichever lands second. components/ now gets an absolute @source exactly as pages/ does, and appears in modules.assets.json. In-repo modules stay covered by the host's static glob.

That was also only half the fix. Vite's bare-specifier fallback resolver only fires for importers under a module's pages/ prefix, so a wheel widget's @simple-module-py/ui import still died with "Failed to resolve import" — the classes were compiled but the component couldn't build. components/ now contributes its prefix and an optimizeDeps entry too, in the host and in the scaffold template.


Notes on test honesty

  • test_created_row_is_readable_on_the_very_next_request documents the client-visible contract but does not fail without the fix — httpx's in-process ASGI transport awaits the full request, teardown included, before issuing the next one, so the follow-up can never lose the race there. The real regression guard is test_write_is_durable_before_the_response_is_delivered. Both docstrings say so.
  • test_commit_failure_becomes_a_500 originally stubbed finalize_session, bypassing its bookkeeping, so get_db's fallback committed the very row the test called lost — it asserted only the status code and would have passed with the guarantee broken. It now injects at AsyncSession.commit and asserts the table is empty.

Merge with main

main moved (#266, #267) and the PR went conflicted. One file conflicted: the scaffold's vite.config.ts. Both sides had restructured it for the same reason — the 300-line cap — main extracting vite.dev-url.ts, this branch extracting module-assets.ts. Kept both extractions; taking main's inline asset block would also have dropped this branch's components/ support.

Splitting the scaffold's vite.config.ts mirrors the split the host already made. The migration-bootstrap tests moved to their own file on the same grounds.

Verification (on the merged tree)

  • pytest: 2016 passed, 2 skipped
  • vitest: 48 passed · biome ci .: clean · tsc --noEmit: clean
  • ruff format --check + ruff check + ty check: clean
  • 300-line file cap: clean

Each fix's regression test was run against the unpatched code to confirm it actually fails, except where noted above. Generated frontend files are gitignored, so there's nothing to regenerate.

…CSS sourcing
Closes#257, #258, #262, #263, #264.
#263 — expression-based index emitted twice under PostgreSQL. The dedup guard
in make_process_revision_directives scanned only the top level of upgrade_ops,
but autogenerate nests CreateIndexOp inside a ModifyTableOps group next to the
CreateTableOp. The guard therefore never saw the index the dialect had already
emitted and re-injected it, so a fresh Postgres app died on its first
`make migrate` with DuplicateTable. The scan now recurses through nested op
groups; the same fix applies to DropIndexOp in the downgrade.
#262 — scaffolded apps migrated the wrong database. The scaffold's alembic.ini
resolved script_location against the invocation cwd, forcing the Makefile to
`cd host`, where BootstrapSettings' cwd-relative `.env` lookup missed the
repo-root file and silently fell back to the default SQLite URL. Ported this
repo's two corrections into the templates (`%(here)s/migrations`, migrate from
the repo root) and made env.py log the resolved URL, password masked, so a cwd
mismatch is visible on the first migration instead of silent.
#264 — `create-module` always wrote tests/test_module.py. With no
tests/__init__.py, pytest derives the module name from the basename alone, so
the second module in a repo broke collection with "import file mismatch" — and
root-level pytest is what the scaffold's own `make test` runs. The template is
now named after the package.
#257 — get_db committed in its yield-teardown, which FastAPI runs after the
response has been delivered, so create-then-immediately-use lost the race and
404'd deterministically. Added CommitBeforeResponseMiddleware, which finalizes
the request's sessions at the ASGI http.response.start message — the last point
still inside the request, late enough that response serialization has already
run and early enough that a commit failure can still become a 500. get_db keeps
its finalize as a fallback for when the middleware isn't in the stack; the
session is claimed once, so the error path (which unwinds the dependency, and
therefore rolls back, before the error response is sent) is unaffected.
#258 — gen-pages emitted @source only for a wheel module's pages/, leaving
widgets under components/ uncompiled unless the host hand-wrote a .venv-relative
glob. That path cannot be spelled portably (Windows uses Lib/site-packages) and
Tailwind drops a non-matching glob silently, so every widget class vanished from
the build on Windows with no error. components/ is now sourced by absolute path
like pages/, and surfaces in modules.assets.json.
Claude-Session: https://claude.ai/code/session_015fCFfMiqGce8unVpVezJG7
@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 15, 2026

Copy link
Copy Markdown

Deploying simple-module-python with Cloudflare Pages Cloudflare Pages

Latest commit:ab680a9
Status: ✅ Deploy successful!
Preview URL:https://c3c379bf.simple-module-python.pages.dev
Branch Preview URL:https://claude-github-issues-07jlrm.simple-module-python.pages.dev

View logs


Follow-up to the previous commit, from review findings.
**Write loss (regression in the #257 fix).** Finalizing at
http.response.start claimed the session one-shot, but work legitimately
continues after that message: Starlette runs BackgroundTasks once the body is
sent, and a StreamingResponse writes its body afterwards — both on the same
session. Those writes flushed and never committed, and get_db's fallback
no-op'd because the session was already claimed. Reproduced: a background task
that adds a row persisted nothing with the middleware and persisted correctly
without it.
finalize_session is now re-armable rather than one-shot — each call commits
whatever is pending at that moment, and get_db's exit code still runs to catch
work done after the response started. The pending-write marker is cleared on
settle so a committed session is never re-committed, and a failed commit is
settled too so the fallback cannot retry it. Read-only requests still pay for
exactly one rollback. Tests cover both the BackgroundTasks and StreamingResponse
paths; both fail against the previous commit.
**Test that did not test what it claimed.** test_commit_failure_becomes_a_500
stubbed finalize_session, bypassing the bookkeeping, so get_db's fallback
committed the row the test called lost — it asserted only the status code and
would have passed with the guarantee broken. It now injects the failure at
AsyncSession.commit and asserts the table is empty.
**#262 was only half fixed.** The Makefile hunk fixed later `make migrate`
runs, but `smpy new` still ran the baseline autogenerate and `upgrade heads`
with cwd=host/. The scaffolded default URL is itself root-relative
(sqlite+aiosqlite:///./host/app.db), so that bootstrap wrote host/host/app.db
while the app and the now-fixed `make migrate` use host/app.db — the same
defect one step earlier, and newly divergent. Both now run from the project
root, with the ini path collapsing for the flat create-host layout.
**#258 was only half fixed.** Tailwind now scans a wheel module's components/,
but Vite's bare-specifier fallback resolver only fires for importers under a
module's pages/ prefix, so a widget's `@simple-module-py/ui` import failed to
resolve. components/ now contributes its prefix and an optimizeDeps entry, in
the host and in the scaffold template.
Splitting the scaffold's vite.config.ts into module-assets.ts mirrors the split
the host already made for the same reason, and keeps both under the 300-line
cap; the migration-bootstrap tests move to their own file on the same grounds.
Known limitation, now documented in the middleware: with more than one enlisted
session (Depends(get_db, use_cache=False)), a failure part-way leaves earlier
commits durable while the client sees a 500.
Claude-Session: https://claude.ai/code/session_015fCFfMiqGce8unVpVezJG7
@antosubash
antosubash marked this pull request as ready for review August 17, 2026 08:10
Resolves the conflict in the scaffold's vite.config.ts and refreshes comments
that main's #267 made stale.
**Conflict.** Both sides restructured the same file for the same reason — the
300-line cap. main extracted the dev-server URL derivation into
`vite.dev-url.ts`; this branch extracted the module-asset loading into
`module-assets.ts`. Kept both: the config now imports `viteDevServer` and
`loadModuleAssets`. Taking either side alone would have dropped the other's
extraction, and taking main's inline asset block would also have dropped this
branch's components/ support (GH #258).
**Stale rationale.** main's #267 fixed cwd-independent .env discovery
(`find_env_file` walks up) and anchors relative sqlite URLs to the project
root — which is the alternative this branch explicitly declined for #262 as
too broad. It is the better fix, and it lands first. What that invalidates is
not the code here but its justification: `_alembic_argv`, both `env.py`
copies, and the bootstrap test all asserted that Settings resolves `.env`
against the cwd and that bootstrapping from host/ writes host/host/app.db.
Neither is true of the merged tree. Rewritten to say what is now true — the
bootstrap runs where `make migrate` and the app run so relative paths in the
scaffold mean one thing, with #262 defended in depth upstream.
The commit-before-response middleware survived the merge into
`_phase_helpers.py` unchanged, still innermost, now alongside #267's
csp_registry parameter.
Verified on the merged tree: pytest 2016 passed / 2 skipped; vitest 48 passed;
ruff format + ruff check + ty check clean; 300-line cap clean; tsc and
`biome ci .` clean.
Claude-Session: https://claude.ai/code/session_015fCFfMiqGce8unVpVezJG7
@antosubash
antosubash merged commit 0049853 into mainAug 20, 2026
13 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment