10x: error envelopes, quality gates, and app.py / app.js decomposition#29
Merged
Conversation
Every error response now uses one canonical JSON shape from the new
services/errors.py:
{"error": {"code", "message", "hint", "request_id"}}
- services/errors.py: error_envelope() + code_for_status() (stable
machine-readable slugs like "not_found" / "validation_error" /
"internal_error").
- app.py: exception handlers for HTTPException, RequestValidationError,
and a catch-all Exception handler (500s no longer surface as bare
tracebacks). The legacy top-level "detail" field is preserved so the
frontend and existing callers keep working.
- Request-ID middleware: honors a client-supplied X-Request-ID or mints
a uuid, echoes it in the X-Request-ID response header on every
response, embeds it in error envelopes, and stamps it onto all log
records via a contextvar-backed logging filter ([rid=...] in the log
format).
- Converted the silent broad excepts on user-facing chat/request paths
to logged warnings (MCP OAuth token substitution, shell plot
auto-capture, write_file checkpoint, verification fallback +
audit-log append, verification screenshot provider). Tool results
keep their existing {"error": ...} contract with the model —
envelopes apply to HTTP responses and (next commit) SSE error events.
- tests/test_api.py: TestErrorEnvelopes covering 404 / unknown-route
404 / 422 / 400 envelope shape, legacy detail preservation, request-id
header echo on success and error, and a forced internal error.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
Streaming endpoints no longer end silently when the upstream dies, and
idle streams stay alive through proxies.
- services/sse_proxy.py: proxy_sse() now emits an `event: error` SSE
message carrying the canonical envelope when the upstream generator
raises mid-stream (the `_done` sentinel is reserved for clean
completion), and yields `: keepalive` comment lines every ~15s while
the upstream is idle. Non-dict JSON payloads are wrapped as
{"raw": ...} instead of crashing the stream.
- services/routes.py: the CLK/AutoGUI stream endpoints pass the
request id through so mid-stream error envelopes are correlatable.
- app.py /api/chat: the run_loop error events now carry the envelope
({"message": ..., "error": {code, message, hint, request_id}} — the
legacy "message" field is kept for older clients and the e2e SSE
helpers), failures are logged server-side, and the event stream emits
`: keepalive` comments whenever the queue is idle for 15s (model
thinking, long tool runs, approval dialogs).
- static/app.js: the chat `error` SSE handler reads the structured
envelope (message + hint + request-id ref) for the transcript entry
and additionally surfaces the failure via the existing flash() toast.
Keepalive comment lines are already ignored by consumeSSE (no data:).
- tests/test_services.py: TestProxySSE (clean stream, mid-stream and
immediate upstream failure envelopes, keepalive comments) and
TestChatSSEErrors (internal error envelope, HTTPException
status→code mapping, chat keepalive comments).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
- Add pyproject.toml configuring ruff (E, F, W, I; E501 exempted since the existing style favours long lines) and mypy (lenient globally with ignore_missing_imports; services/ fully typed via per-module overrides). - Fix all ruff findings mechanically: import sorting (I001), one-line multiple imports (E401), mid-file import moved to top (test_services.py), ambiguous variable name l -> lite (E741), and two intentional late imports marked noqa: E402 with justification. - Fully annotate services/: return types on all route handlers and the stream_task async generators, typed dicts in health.py/clients.py, and typed _PROVIDER_META plus a variable rename in oauth.py to fix an Optional reuse. No behavior change. - CI lint job: replace the pyflakes-only step with ruff check + mypy services/ (pinned versions, requirements installed so mypy sees real fastapi/httpx types). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
- Add self-contained eslint flat config (eslint.config.js, no plugin deps so `npx eslint` works with zero node_modules) targeting genuine errors only; .prettierrc documents the existing style (2-space, double quotes, semis) and is intentionally not gated to avoid a mass reformat. - Fix the only lint findings: prefer-const on pendingWorkspaceFiles, plus two missing browser globals declared in config (Element, ImageCapture). - Extract 5 pure helpers from app.js into new static/lib.js: escape, humanLabelForTool, friendlyError, fillTemplate, and parseSSEBlock (SSE block parsing pulled out of consumeSSE). lib.js loads before browser-store.js/app.js via a classic script tag (zero-build preserved) and has a module.exports guard so node can require it. - Add tests/js/lib.test.js: 16 unit tests runnable with `node --test "tests/js/*.test.js"` (plain node test runner). - CI: new js-quality job (Node 22) running eslint + node --test; lint job file checks now cover static/lib.js. - tests/test_frontend.py updated for the extraction (helpers asserted in lib.js, load-order test, no-duplication test): suite now 379 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
- app.py: fold the Phase-1 logging setup into _configure_logging() — level now comes from BWUI_LOG_LEVEL (default INFO), rotating file + stderr handlers, request-id filter attached to every root handler so rid= is stamped consistently (background tasks log rid=-). - Background paths: the lifespan MCP-reconcile print() is now a logged exception; the boot-time upload sweep and per-server MCP shutdown failures are logged warnings instead of silent passes; scheduler tick failures now log with a traceback (log.exception). No control flow changed. - Concurrency audit (scheduler.py + services/state.py): the scheduler tick held a pre-run snapshot of scheduled_tasks.json across the awaited run_callback (minutes for LLM runs) and wrote it back afterwards, silently reverting any /api/scheduled-tasks add/edit/delete made during the run and resurrecting deleted tasks. The tick now re-reads the file after each run and merges the outcome into the live task by id, under a new _TASKS_LOCK; the decide-what-is-due + next_run_at backfill phase is locked too. The sync CRUD helpers stay lock-free — they are await-free and thus event-loop-atomic — with that invariant documented at the lock and in services/state.py (whose helpers are safe for the same reason). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
app.py (4,700 lines) is now a 391-line composition root: FastAPI init,
request-id middleware, exception handlers, lifespan, static mounts, and
router registration. Everything else moved out:
Domain logic → services/ (all strict-mypy clean):
catalog.py static registries (endpoint profiles, MCP/CLI, onboarding)
storage.py path constants + JSON persistence + conversation store
request_ctx.py request-id contextvar / log filter / accessor
session.py approvals, file responses, session trust, local-caller gate
mcp.py MCP stdio client + manager singleton
skills.py skill files + config lints
llm.py chat_complete, endpoint discovery, models, message shaping
prompt_builder.py tool protocol + system-prompt assembly + tool-call parsing
tools.py shell, checkpoints, media/web tools, subagents, execute_tool
transient.py TTL-swept per-chat uploads
scheduled.py scheduled-task runner + notification queue
Route handlers → routers/ (one APIRouter per domain, included by app.py):
admin, skills_prompts, session_trust, workspaces, onboarding, project,
mcp_cli, uploads, scheduled, oauth_routes, conversations, chat
(chat.py also carries the BWUI_TEST_MODE-only test endpoints).
Compatibility invariants (test suite passes unchanged, 379 passed):
- `from app import X` keeps working via identity re-exports.
- Names the tests rebind (conftest's path constants, WORKSPACE_DIR
assignment, patch("app.chat_complete"), patch("app._sse_keepalive_interval"))
are served by data-descriptor properties on a ModuleType subclass that
forward get/set/delete to the owning services module; handlers read those
values via module-attribute access at call time, so patches stay effective.
The delete path parks a sentinel so mock.patch's delattr-then-restore exit
sequence restores the original into the owning module.
- Routers import only from services/, never from app — no import cycles.
mypy gate extended: routers/ now checked with the same strict settings as
services/ (pyproject override + ci.yml runs `mypy services/ routers/`).
verification.py got a one-line explicit None-guard so the strict gate can
follow its import from routers/chat.py.
Gates: pytest 379 passed · ruff clean · mypy services/ routers/ clean ·
eslint clean · node --test 16 pass · uvicorn boot + curl smoke all 200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
static/app.js (3,849 lines) is now 15 native ES modules under static/js/,
loaded via <script type="module" src="/static/js/main.js"> — still no build
step. Code is verbatim from the old file; the only changes are `export`
keywords on top-level declarations and generated `import` lines (derived
mechanically from eslint no-undef until the graph converged).
state.js shared `state` object + $/$$ helpers (imports nothing)
api.js fetch wrappers + local-download helpers
render.js markdown/KaTeX + message rendering + read-aloud
settings.js display settings, Settings tab, models, prompts, skills
workspaces.js workspace CRUD/export/import
tools_tab.js MCP servers + CLI shortcuts
conversations.js list/search/pin/tag/fork
composer.js attachments, image annotation, voice input
panels.js task-plan + file-tree panes
dialogs.js approval / file-request / generic focus-trapped dialog
chat.js send pipeline + /api/chat SSE loop
onboarding.js first-run wizard
bundles_memories.js IndexedDB bundles + user memories
scheduled.js scheduled-tasks tab
main.js keyboard shortcuts, tab wiring, init() (entry point)
Window-global inventory (checked before the split): index.html has ZERO
inline onclick/onchange handlers and app.js assigned no window globals, so
nothing needed export-and-attach. The globals the modules consume — lib.js
helpers (escape, humanLabelForTool, friendlyError, fillTemplate,
parseSSEBlock), browser-store.js's `bws`, and the CDN libs (marked,
DOMPurify, katex, renderMathInElement) — stay classic scripts loaded before
the module entry, exactly as before. lib.js is untouched, so
`node --test tests/js/` still passes (16/16).
static/app.js remains as a 12-line forwarding shim (`import("/static/js/
main.js")`) so browsers with a stale cached index.html still boot and
tests/test_api.py::TestStatic (which fetches /static/app.js and may not be
edited) keeps passing.
eslint: new sourceType:"module" block for static/js/** with no-import-assign.
tests/test_frontend.py updated minimally: APP_JS is now the concatenation of
static/js/*.js; the two link-order assertions target /static/js/main.js. All
other assertions (function presence, load order, no-duplication, no inline
handlers, no console.log) are unchanged and still pass.
ci.yml: smoke now curls every static/js/*.js (+ lib.js/browser-store.js),
UTF-8 and required-files checks point at the module tree.
Verified: eslint clean · node --test 16 pass · pytest 379 passed · every
module 200 via uvicorn with text/javascript MIME · module graph parses under
`node --check` and all 14 non-entry modules evaluate in Node without error
(no import-cycle TDZ at eval time).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
static/style.css (2,142 lines) is now five stylesheets under static/css/, loaded via ordered <link> tags in index.html (cascade order preserved — verified byte-equivalent to the original when concatenated): base.css 256 design tokens, a11y themes, skip link, focus, type, forms layout.css 400 sidebar, 3-pane layout + right rail, plan/file panes chat.css 568 chat header, messages, composer components.css 505 dialogs, spinner, scrollbars, rich content, Tools tab overlays.css 413 onboarding, modal overlays, toasts, responsive, cards (The suggested "panels.css" bucket folded into layout.css because the pane rules are contiguous with the 3-pane layout section; "overlays.css" covers the tail sections instead.) static/style.css stays as a 5-line @import aggregator in the same order, so the old URL keeps serving real CSS for stale cached index.html copies and for tests/test_api.py::TestStatic::test_style_css_served, which may not be edited. tests/test_frontend.py updated minimally: STYLE_CSS is the concatenation of the split files in cascade order, and test_style_css_linked now asserts the five links appear in order. Every rule assertion is unchanged and passes. ci.yml: smoke curls every static/css/*.css (+ the aggregator); UTF-8 and required-files checks cover the split tree. Verified: pytest 379 passed · ruff/mypy/eslint clean · node --test 16 pass · uvicorn boot with all six CSS URLs 200 (text/css) and index.html linking the five files in order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
The services./routers. mypy overrides enable check_untyped_defs, which surfaces import-untyped errors for yaml (services/storage.py) and aiofiles (routers/uploads.py) — the global ignore_missing_imports does not suppress import-untyped for installed-but-unstubbed packages. CI's fresh environment lacked the stub packages, so the lint job failed even though the split itself is type-clean. Install the stubs in the lint job. Verified: `mypy services/ routers/` → Success, no issues in 33 files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
There was a problem hiding this comment.
Pull request overview
This PR delivers a three-phase overhaul of BetterWebUI focused on (1) structured error handling with request correlation IDs, (2) stricter quality gates (ruff/mypy/eslint + JS unit tests), and (3) a large-scale decomposition of the previous “god files” into Python services/routers and native ES-module frontend code—while preserving the zero-build deployment model and existing UI behavior.
Changes:
- Introduces structured error envelopes (
{error:{code,message,hint,request_id}}) across HTTP and SSE, plus keepalive/error events for SSE streams. - Adds and enforces quality gates in CI (ruff, mypy for
services/+routers/, eslint, andnode --test), with corresponding new/updated tests. - Decomposes backend and frontend into domain modules (FastAPI routers/services;
static/js/*ES modules; split CSS), updating static serving and regression assertions accordingly.
Reviewed changes
Copilot reviewed 66 out of 68 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| verification.py | Tightens judge-run guard to satisfy Optional typing / runtime safety. |
| tests/test_verification.py | Fixes import ordering with explicit noqa for sys.path setup. |
| tests/test_services.py | Expands service tests; adds SSE proxy + chat SSE error/keepalive coverage. |
| tests/test_frontend.py | Updates structural assertions for split CSS + ES-module frontend; validates lib.js helpers. |
| tests/test_api.py | Adds coverage for structured error envelopes and request-id header behavior. |
| tests/js/lib.test.js | Adds Node unit tests for extracted pure JS helpers in static/lib.js. |
| static/lib.js | Introduces pure helper library usable in browser + Node via module.exports guard. |
| static/js/workspaces.js | Workspace UI module extracted from former monolithic app.js. |
| static/js/state.js | Central shared client state + DOM helper module. |
| static/js/scheduled.js | Scheduled tasks UI module extracted from former monolithic app.js. |
| static/js/panels.js | Right-rail panel logic (plan + file tree) extracted into module. |
| static/js/onboarding.js | Onboarding wizard extracted into module. |
| static/js/dialogs.js | Modal dialogs (approval/file picker/generic) extracted into module with focus trapping. |
| static/js/conversations.js | Conversation list/search/pin/fork module extracted into module. |
| static/js/chat.js | Chat send pipeline + SSE consumption loop extracted into module; uses structured error events. |
| static/js/bundles_memories.js | Bundles + memories (IndexedDB-backed) extracted into module. |
| static/js/api.js | Fetch wrapper + download helpers extracted into module (relies on lib.js globals for helper funcs). |
| static/index.html | Switches to ordered split CSS + classic lib.js + ES-module entrypoint js/main.js. |
| static/css/layout.css | Split stylesheet: layout/sidebar/right-rail/task plan/file tree. |
| static/css/base.css | Split stylesheet: tokens, accessibility themes, focus rings, typography, forms. |
| services/transient.py | Adds transient upload TTL sweep helpers for per-chat bundle uploads. |
| services/storage.py | Centralizes storage paths + persistence helpers; documents attribute-access requirement for rebinding. |
| services/state.py | Documents concurrency invariants around enabled/disabled service state persistence. |
| services/sse_proxy.py | Adds keepalive comments + explicit SSE event: error emission on upstream failure. |
| services/skills.py | Extracts skills loading + linting for skills/MCP/CLI config. |
| services/session.py | Extracts in-memory session stores + local-caller gate logic. |
| services/scheduled.py | Extracts scheduled-task execution + notification queue. |
| services/routes.py | Service integration routes updated; streams now pass request_id into SSE proxy. |
| services/request_ctx.py | Adds request-id contextvar + logging filter helpers. |
| services/oauth.py | Tightens typing and minor variable naming cleanup in OAuth flow. |
| services/mcp.py | Adds MCP stdio client + manager extraction. |
| services/llm.py | Extracts model IO (chat completion, model discovery/listing) + context trimming. |
| services/health.py | Tightens typing and client typing in service health checks. |
| services/errors.py | Defines canonical error envelope + status-to-code mapping. |
| services/clients.py | Adds typing + ensures service stream generators yield JSON payloads without data: prefix. |
| scripts/mock-server.py | Minor import formatting cleanup in mock server helper. |
| scheduler.py | Adds lock + merge-by-id strategy to avoid clobbering concurrent scheduled-task CRUD during long runs. |
| routers/workspaces.py | Adds workspace CRUD/export/import endpoints and project_root validation/normalization. |
| routers/uploads.py | Extracts uploads routes incl. transient uploads and local-only TTS/transcribe endpoints. |
| routers/skills_prompts.py | Extracts system prompt + skill CRUD routes. |
| routers/session_trust.py | Extracts approvals/session trust + file-response endpoint. |
| routers/scheduled.py | Extracts scheduled-task CRUD + verification log + notifications drain endpoints. |
| routers/project.py | Extracts project tree/file/checkpoint/revert endpoints with path traversal protections. |
| routers/onboarding.py | Extracts onboarding templates + completion endpoints. |
| routers/oauth_routes.py | Extracts OAuth status/connect/disconnect endpoints. |
| routers/mcp_cli.py | Extracts MCP server + CLI shortcut configuration routes. |
| routers/conversations.py | Extracts conversation list/search/summary/pin/tag/fork/delete endpoints. |
| routers/admin.py | Extracts config/models/lint/branding/health/explain-command/memory-extract endpoints. |
| routers/init.py | Centralizes router registration order for app composition root. |
| pyproject.toml | Adds ruff + mypy configuration; enforces strict typing for services.* and routers.*. |
| eslint.config.js | Adds self-contained ESLint flat config for classic scripts + ES modules + node tests. |
| .prettierrc | Adds Prettier configuration (non-gating). |
| .github/workflows/ci.yml | Updates CI to add ruff/mypy + eslint + node tests; updates static-asset smoke checks for split assets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+71
to
+76
| @router.post("/api/file-response") | ||
| async def post_file_response(r: FileResponseIn) -> dict: | ||
| ok = session.file_responses.resolve(r.request_id, r.files or []) | ||
| if not ok: | ||
| raise HTTPException(404, "Unknown file request id") | ||
| return {"ok": True} |
Comment on lines
+102
to
+104
| data = storage.load_workspaces() | ||
| wid = w.id or "".join(c for c in w.name.lower() if c.isalnum() or c in "-_ ").strip().replace(" ", "-") or uuid.uuid4().hex[:8] | ||
| payload = w.model_dump(exclude_none=True) |
…ds, fix Docker COPY
- routers/session_trust.py: restrict POST /api/file-response to local callers
via session._require_local_caller (matching /api/approve, /api/session/trust)
so a remote host can't resolve in-flight file-picker requests.
- routers/workspaces.py: validate client-supplied workspace id against
^[A-Za-z0-9_-]{1,64}$ (rejecting HTML/attribute-injection chars) before it
flows into URLs and HTML attributes; server-generated slugs/uuids already
satisfy the pattern so legitimate creates/updates are unaffected.
- Dockerfile: COPY routers/ so the Phase 3 router package ships in the image
(app.py imports routers at startup; its absence broke the e2e container).
- tests: file-response non-local-caller rejection + unknown-id 404; workspace
id allowlist (valid id, name-derived slug accepted; quote/angle-bracket/
space/overlong rejected).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
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.
Three-phase overhaul of BetterWebUI. Every commit kept CI green (
pytest tests/+ uvicorn smoke, plusruff/mypy/eslint/node --testfrom Phase 2 onward). The 60-spec Playwright suite runs in CI and was treated as the regression guard for all structural changes.Phase 1 — Robustness
{error: {code, message, hint, request_id}}shape; request-id middleware echoesX-Request-IDand stamps log records.event: errorenvelope instead of silently dropping;: keepalivecomments every 15s; the frontend surfaces the error as a toast/transcript entry.Suite 361 → 375.
Phase 2 — Quality gates + concurrency
ruff+mypy(services/ fully typed) replace the pyflakes-only lint;eslint+ first-ever JS unit tests (node --test) for pure helpers extracted fromapp.js.Suite 375 → 379.
Phase 3 — Full decomposition (zero-build preserved)
Each god-file became modules behind thin shims — existing tests pass unchanged (only static-path assertions and CI smoke curls updated in the same commits):
app.py(4,700) → 391 lines (composition root) + 12routers/+ 11services/modulesstatic/app.js(3,849) → 15 native ES modules loaded via<script type="module">(no bundler introduced);app.jsremains a 12-line forwarding shimstatic/style.css(2,142) → 5 ordered files (concatenation verified byte-equivalent)A 33-URL uvicorn smoke sweep confirmed every asset still serves 200; the module conversion preserved all DOM ids/classes and required no
window-global changes (index.html had zero inline handlers).Notes
AutoGUI/,CognitiveLoopKernel/,OSScreenObserver/) intentionally left unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_012jUkqbwroQmYRAdFDsp5r9
Generated by Claude Code