Architecture remediation: 11-phase review fixes (backend + client) - #1
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
One register_exception_handlers() per app is now the single source of
truth for status codes and response shapes. Routers raise domain
exceptions; ten local try/except blocks deleted. Clipboard service
speaks SnippetNotFoundError/SnippetValidationError instead of leaking
KeyError/ValueError; file_service raises InvalidFileRequestError.
Error shape standardized on {"detail": ...} (client reads only
ApiError.status, verified). +21 handler-mapping tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eStore (phase 2a) get_state_store()'s module-level _stores dict made every app instance in a process share one connection+lock, contradicting main.py's documented no-singletons design. create_app now constructs one store per app (open_state_store), attaches it to app.state.store, and injects it into ClipboardService/FileRequestService/ShareLinkService/upload_index. New isolation regression test: two apps on one data dir get independent stores. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…letons (phase 2b) config, mount registry, session signer, account store, file-TTL db, dropbox app/client, and the mount-registration rate limiter now live on a per-app RelayState filled by lifespan; the 11 getter/setter functions and services/account_store.py are deleted. init_dropbox returns (app, client); access_policy and user_storage take explicit deps; is_admin_username takes config. Module-level app removed — relay.cli boots uvicorn with factory=True. slowapi's limiter (+ its rate values) stays process-global by third-party design and is documented as the single sanctioned exception. Tests migrate from set_*() choreography to wiring app.state.relay fields; 6 singleton-mechanics tests deleted with the mechanism they tested. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(phase 3) All ServerStateStore calls from request paths now run via asyncio.to_thread at the service layer; ShareLinkService's public methods are async (they did sqlite writes inline from async routes). upload_index is now honestly async. Store remains sync sqlite3 by documented contract — create_app is a synchronous factory and the test architecture is lifespan-less; the plan's full-aiosqlite option is recorded as a deviation in the remediation plan. shared/sqlite_kernel.py extracts the duplicated aiosqlite bootstrap (path validation, WAL, schema+commit) for relay's sqlite_registry and file_ttl_db. accounts/ deliberately keeps its own bootstrap: leaf packages import no project packages, shared included. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…guarded sends (phase 4) - agent_auth carries PROTOCOL_VERSION; relay refuses mismatches before registration so frame-format changes fail loudly at connect time. - tunnel/metadata.py is the typed OPEN/WS_OPEN wire contract: 16KiB cap enforced at serialization (can never hit FrameTooLargeError) and structural validation at parse; the agent answers a 400 to malformed OPENs instead of dying on KeyError. - The agent now heartbeats at 30s alongside the relay's 15s pings — a half-dead relay socket previously left the agent believing it was mounted while browsers got 503s. - TunnelConnection sends are guarded: transport failures surface as TunnelSendError, which the proxy maps to 503 (and 431 for pathological header sets) instead of crashing the endpoint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ize/undecodable (phase 5) The proxy buffered entire text/html bodies with no cap and hard-decoded UTF-8; any mislabeled or non-UTF-8 page crashed the request with an unhandled UnicodeDecodeError. Rewriting is cosmetic — serving unrewritten bytes is strictly better than a 500. Extracted relay/app/services/html_rewriter.py (charset detection, decode-failure passthrough, 5MiB cap); bodies over the cap stream through unchanged with their original content-length. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The drop box's wiring spanned three files with two code-equality special cases and a bespoke WS bridge inside mount_proxy. It is now a LocalAsgiMount (app + forwarding client + WebSocket bridge + lifecycle) registered in RelayState.local_mounts; the proxy dispatches on map membership and contains no drop-box-specific code. Lifespan owns the mount's aclose(). Plan deviation recorded: no persisted MountKind column — live ASGI apps cannot be persisted; the registry already encodes locality as connection=None and local_mounts membership is the kind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (phase 7) A client that reached the agent's local server port directly (e.g. on the LAN, bypassing the tunnel) could forge X-WFS-User/Role/Auth-Bypass headers and assume an allowlisted identity, because the server trusted them whenever mount_code was set. Now each agent mints a per-mount secret at connect, shares it only with this relay (agent_auth) and this mount's embedded server (ServerConfig.identity_secret); the relay HMAC-signs the injected identity tuple and the server verifies before trusting. A forging client lacks the secret, so its headers carry no valid signature and are ignored (fail closed on identity). Closed a latent hole: AuthMiddleware bypassed the password gate on a raw x-wfs-auth-bypass header without verifying the signature — it now routes through is_auth_bypassed (config + HMAC). Local mounts strip inbound X-WFS-*. Signature covers user|role|bypass (not path — see plan deviation). AccessMode.LEGACY makes the pre-v1.3 fail-open an explicit, logged, enum-visible state instead of a silent exception catch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oop (phase 8) (a) server/app/bootstrap.py is now the composition root — build_mount_app, run_mount_agent, and run_lan_server live there; cli.py is parsing + delegation and no longer imports the agent package. The import-boundaries whitelist moves to bootstrap.py accordingly. (b) TunnelConnection gains run_receive_loop_with_handlers(on_open, on_ws_open) sharing a private core with the relay's run_receive_loop. The agent's two hand-rolled loops — which read conn._ws and called conn._dispatch_frame directly — are deleted; _OpenFrameHandlers owns handler-task spawn + drain, and expired_files moves to a registered control handler. Frame-routing logic now lives in exactly one place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(phase 9) Hand-written response types drifted silently from the FastAPI schemas. Now server/app/openapi_dump.py emits the OpenAPI schema and scripts/gen_api_types.sh runs openapi-typescript into client/src/types/api.gen.ts; the client type modules derive from it (Schemas["FileEntry"] etc.), keeping only the runtime consts (FileType, RequestStatus). Added response_model to the files routes and expires_at to FileEntry so the schema is the real contract. A new api-types CI job regenerates and git-diffs the result, so a backend schema change that isn't reflected in the client types fails the build instead of users. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…se 10) The file listing was useState + manual loadFiles() after every mutation, which raced WebSocket pushes (double-fetch) with no dedup. BrowseProvider now backs it with @tanstack/react-query useQuery(["files", path]); loadFiles is queryClient.invalidateQueries and mutations invalidate on success — concurrent refetches dedup, and mutation errors live in a separate opError state so a failed delete doesn't blank a valid listing. Scoped to the listing (the highest-value race); other slices can adopt the pattern incrementally. Routing: resolveAppMode(pathname) + an AppMode const-object replace main.tsx's pickRoot if-chain, making the static-route decision a single testable function. (Const-object, not a TS enum — erasableSyntaxOnly.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…board (phase 11) The three most stateful client hooks were untested. Added: - useWebSocket: a MockWebSocket + fake timers exercise the reconnect backoff state machine — open/connect, reconnect after the delay, exponential escalation on close-before-stable, counter reset after a stable connection, the MAX_DELAY cap, message dispatch, and that unmount cancels pending reconnects. - useUpload: the 3-way concurrency cap, onUploadComplete, the 409->conflict flow with SKIP and OVERWRITE, and failure->retryFailed. - useClipboard: mount load, debounce coalescing (one send with the latest value), WS created/updated/deleted reconciliation, and optimistic-title rollback on API failure. +18 tests (113 total). This is the final phase of the architecture remediation plan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Architecture remediation — 11 phases
Implements the full set of fixes from the codebase architecture review. Each phase is an atomic commit with its own detailed message; this PR is the overview. Every plan deviation is recorded in
docs/architecture-remediation-plan.md.Backend (phases 1–8)
register_exception_handlers()per app maps domain exceptions to HTTP; routers raise, never build error responses. New domain exceptions replace leakedValueError/KeyError.RelayStatedataclass onapp.state(built in lifespan); the server's process-level_storescache → oneServerStateStoreinjected per app. Two apps can now coexist in one process.asyncio.to_thread;ShareLinkServiceis async;upload_indexis honestly async. Newshared/sqlite_kernel.pyunifies the relay's aiosqlite bootstrap.PROTOCOL_VERSIONnegotiation in the handshake (skewed agents rejected at connect), typed + size-cappedRequestMetadata/WsOpenMetadata, agent-side heartbeat (detects half-dead relay sockets), guarded sends (TunnelSendError→ 503 instead of crashing the proxy).html_rewriter.py: charset-aware decode, passthrough on undecodable/oversize bodies (5 MiB cap). The proxy can no longer crash on non-UTF-8 HTML.LocalAsgiMount(app + client + WS bridge + lifecycle) inRelayState.local_mounts;mount_proxyhas zero drop-box-specific code.X-WFS-*identity tuple and the server verifies before trusting. Closes the LAN spoof vector. Also closed a latent hole:AuthMiddlewarebypassed the password gate on a rawx-wfs-auth-bypassheader without verifying the signature.AccessMode.LEGACYmakes the pre-v1.3 fail-open explicit and logged.server/app/bootstrap.pyis the composition root (build_mount_app,run_mount_agent,run_lan_server);cli.pyis parsing + delegation and no longer imports the agent. The agent's two hand-rolled receive loops are unified intoTunnelConnection.run_receive_loop_with_handlers— no more pokingconn._ws/_dispatch_frame.Client (phases 9–11)
server/app/openapi_dump.py+scripts/gen_api_types.shgenerateclient/src/types/api.gen.ts; the type modules derive from it. New CIapi-typesjob regenerates andgit diff --exit-codes — a backend schema change that isn't reflected in the client types fails the build.BrowseProviderbacks the file listing withuseQuery(["files", path]);loadFilesisinvalidateQueriesand mutations invalidate on success (no manual-refetch race, requests dedup).resolveAppMode()+ anAppModeconst-object replacemain.tsx's scattered routing if-chain.useWebSocket(MockWebSocket + fake timers: reconnect backoff escalation, stable-connection reset, delay cap, dispatch, unmount safety),useUpload(concurrency cap, 409→conflict→SKIP/OVERWRITE, failure→retry),useClipboard(debounce coalescing, WS reconciliation, optimistic-title rollback).Verification
ruff✓ ·mypy --strict✓ · 974 pytest ✓eslint✓ ·tsc✓ · 113 vitest ✓ ·vite build✓api-typesdrift job added).Notes / follow-ups (not in scope here)
client_dist / pathstatic lookup relies on Starlette URL normalization to block..traversal — worth an explicit hardening pass.🤖 Generated with Claude Code