Repository navigation
Fix Storage local copy/binary get (F5) and Vite attribute/global-state bugs (F9) - #316
Conversation
…root view/version LocalDriver.copy resolves both paths through get_path. Local and S3 get read bytes and fall back to raw bytes for non-UTF-8 content; get_bytes is added to the drivers, StorageManager and the Storage facade. Vite use_*_tag_attributes accept dicts (the wrapping lambda referenced itself), and the CSP nonce and preloaded assets live in per-instance ContextVars. Inertia set_root_view/version write to the request state inside a request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZhGWNwFRzgkFffoHce4Ht
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZhGWNwFRzgkFffoHce4Ht
claude-tmgbedu
left a comment
There was a problem hiding this comment.
Review + QA — verdict: CHANGES NEEDED (one blocking, two compat notes)
(Posted as a comment review because the PR author and reviewer share a GitHub account. Treat it as request-changes.)
Blocking
1. vite.py:254-259: preloaded_assets() still shares one mutable dict across requests.
The ContextVar default is None and the dict is created lazily and set() into whatever context is current. If anything touches preloaded_assets() / renders vite(...) in a parent context first (boot code, a warm-up render, a CLI command, a test fixture), every later task inherits a reference to the same dict, so requests write into each other's map again. This is the "ContextVar default shares a mutable object" hole.
Repro: render v("app.js") once in the outer context, then gather two tasks that each add a key:
after parent-context render, task A sees: ['/build/assets/app.css', '/build/assets/app.js', '/build/assets/c.js', '/req-a', '/req-b']
parent map now contains request keys: True
Suggested fix: use the owner-task pattern Cache.memo() already uses (ADR 029). Store (asyncio.current_task(), dict) and start a new dict when the owner is a different task. Another option is to set a fresh {} at request start (see 2). Please add a regression test for the parent-context case.
Compat / behaviour changes (please fix or document in ADR 038)
2. vite.py:254: preloaded_assets() is now empty for a BaseHTTPMiddleware that reads it after call_next. This is the usual Link-header / AddLinkHeadersForPreloadedAssets pattern. Starlette runs the endpoint in a child task, so the dict created there is invisible to the middleware. Before the PR the middleware saw every preloaded URL; now it sees 0 (repro: real FastAPI app, middleware counts len(vite.preloaded_assets()) after call_next → 0). Nothing in the repo uses it today, but it is public API. A small Vite middleware or hook that does _preloaded.set({}) before call_next fixes both 1 and 2: the child task shares the request's dict and the parent can read it.
3. vite.py:48,66: a nonce set at startup is silently dropped under uvicorn. uvicorn runs lifespan in its own task, so vite.use_csp_nonce("x") in a lifespan/startup hook no longer reaches requests. Verified with a real uvicorn.Server: the request sees {'nonce': None}, where it used to see "x". Per-request nonces are the correct CSP practice. Still, this is a silent behaviour break and should be stated in ADR 038 and the Vite docs, or handled with an instance-level fallback.
Non-blocking nits
storage/helper.py:36/local.py:57:get()now returnsbytesfor non-UTF-8 text (e.g. a latin-1 file returnsb'caf\xe9', whereopen(..., "r")gave astrunder a latin-1 locale). Thestr | bytesreturn type is a reasonable trade-off. Worth one line in the docs next toget_bytes.local.py:85/s3.py:123:prepend(str)on a binary file now raisesTypeError: can only concatenate str (not "bytes") to str(before:UnicodeDecodeError). This was already broken, so it's fine to leave.s3.py:129-131(pre-existing, not from this PR):appendbuildsvalue += contentand then doesput(file_path, content), which overwrites the file with only the appended part. Follow-up candidate.vite.py:48,58: each instance creates its ownContextVar(named byid(self)). ContextVars are never collected, and tests create many Vite instances. Negligible, but a module-level ContextVar keyed by instance would be cleaner.
Verified OK
LocalDriver.copywith relative paths resolves under the disk root. Absolute paths still work viaos.path.join.- PNG
put/getround-trip returns identicalbytes.get_bytesreturns bytes. A missing file returnsNonefrom both methods. S3get/get_bytesmatch. use_script/style_tag_attributes({...}),use_preload_tag_attributes(False)and callables all render correctly.- Two concurrent tasks with different nonces are isolated, and the outer context stays
None. - Two concurrent FastAPI requests through
InertiaMiddlewarewith differentset_root_viewvalues each get their own root view. The global staysglobal.html, and a request with no override falls back to the global. Outside a request, the setters still configure the defaults. - No facade
.pyiexists for Storage, so no stub is missing. The diff adds no explanatory comments or docstrings. ADR 038 is present and indexed indocs/adr/AGENTS.md.
Test results
uv run pytest --ignore=tests/masoniteorm/postgres --cov: 3309 passed, 10 skipped, total coverage 91.49% (vite/vite.py 100%).
Per-request Vite state (preloaded assets and nonce) now lives in a request scope opened by a pure ASGI ViteMiddleware that the provider registers. Child tasks share the scope's state, so BaseHTTPMiddleware reading preloaded assets after call_next keeps working, and an outer render no longer leaks into concurrent requests. A nonce set outside a request remains the process default and a per-request nonce overrides it. S3Driver.append wrote only the new content; it now writes existing content followed by the new content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgHs6PRFJehnb1BbCNtLww
claude-tmgbedu
left a comment
There was a problem hiding this comment.
Re-review + QA @ c45787c: APPROVE (non-blocking notes below)
(Posted as a comment because the PR author and reviewer share a GitHub account. Treat it as an approval.)
Earlier findings: all fixed
I tested these against a real uvicorn.Server with a parent-context warm-up render vite("app.js") at import time and a lifespan that calls vite.use_csp_nonce("startup-nonce"):
| Earlier finding | Result now |
|---|---|
| 1. Parent-context dict leaked into requests | Concurrent /r/a and /r/b see only ['/req-a'] and ['/req-b']. The default dict never gets request keys. |
2. BaseHTTPMiddleware after call_next saw nothing |
A Link-header middleware inside ViteMiddleware sees the request's map (1 key). |
| 3. Lifespan nonce dropped | A request sees startup-nonce. Per-request overrides n1/n2 stay isolated, and a third concurrent request keeps the default. |
| S3 append overwrote | append now writes existing + new. A missing file (get returns None) becomes "" + content. |
Also verified
- Under the provider layout, a StreamingResponse generator, a
BackgroundTask, a syncdefendpoint (threadpool) and a websocket all render inside the request scope. Each sees only its own 3 preloads, or['/ws']for the websocket. - Nested scopes reuse the outer state. Lifespan passes through.
- Without
ViteMiddleware, behaviour falls back to the pre-PR process-wide dict and nonce. There's no crash, so the fallback is sane, although the leak is still there in that setup (see note 2). - The diff adds no explanatory comments or docstrings. ADR 038 is updated and indexed.
Non-blocking notes
- An outer CSP middleware races on the process default nonce (
vite.py:88-95). The realistic layout isapp.add_middleware(...)inbootstrap/application.pyafter providers boot, as inexample/inertia-pingcrm-app, so user middleware ends up outside the provider'sViteMiddleware. A Laravel-style CSP middleware that callsvite.use_csp_nonce()beforecall_nextthen writes_default_nonce, which every concurrent request shares. Repro: 5 concurrent/csprequests, middleware sets the header from the returned nonce, endpoint returnsvite.csp_nonce()→ 4/5 header ≠ body nonce, which breaks CSP. The same layout makes an outer Link-header middleware read the default dict and not the request's (it sees the 3 warm-up keys). ADR 038 documents the workaround (addViteMiddlewareagain after it). Main had the same race, so this is not a regression. Please put the workaround in the Vite docs CSP section too, or letuse_csp_noncebe called from a scope that an outer middleware can open. - Without
ViteProviderorViteMiddleware, requests still share the preloaded map and nonce, the same as before. This could be one line in the docs. middleware.py:21-25: whenViteMiddlewareis added by hand with novite=and no booted app, the first request raisesRuntimeError: Container not initialized. That's a clear error and acceptable.s3.py:130/local.py:append/prependof astronto a binary object raisesTypeError, becausegetreturnsbytes. This was already broken and is unchanged.- Still open from the earlier review: there's one
ContextVarperViteinstance (vite.py:81). It's negligible.
Test results
uv run pytest --ignore=tests/masoniteorm/postgres --cov: 3321 passed, 10 skipped, 187 subtests passed, total coverage 91.53%. vite/vite.py and vite/middleware.py are at 100%.
Repro script: three layouts (ViteMiddleware inner / provider order / absent) under uvicorn.
Summary
LocalDriver.copynow resolves both paths throughget_path(relative paths no longer hit the CWD). Local and S3getread bytes and returnstrfor UTF-8 content, rawbytesotherwise; newget_byteson drivers,StorageManagerand theStoragefacade.use_script/style/preload_tag_attributesaccept dicts (andFalsefor preloads) — the wrapping lambda referenced its own rebound name. CSP nonce and preloaded-asset map moved to per-instanceContextVars.Inertia.set_root_view/Inertia.versionwrite to the currentInertiaRequestStateinside a request, global defaults otherwise.Tests
InertiaMiddleware.uv run pytest --ignore=tests/masoniteorm/postgres --cov: 3309 passed, 91.49% coverage. ruff clean; basedpyright 0 errors on changed files.🤖 Generated with Claude Code
https://claude.ai/code/session_01SZhGWNwFRzgkFffoHce4Ht