refactor(#276): extract QueryExecutionService — phases 0–1#278
Merged
Conversation
Phase 0: architecture guard build/check-boundaries.mjs (wired into pretest as check:arch) enforcing the day-1 rule — src/application/** must not import src/ui/** or src/editor/** (type-only imports count). Grows with later #276 phases. Characterization coverage was audited against the issue's Phase-0 list and already exists in tests/unit/app.test.ts and dashboard.test.ts (streaming, explicit FORMAT, cancel+partial, script retry matrix incl. the "may have executed" warning, Basic/OAuth refresh, export cancellation, schema-graph stale-write, save/share validation). Phase 1: src/application/query-execution-service.ts — the shared request/stream/normalize core (formerly app.runReadInto) plus runScript's per-statement transport loop (SESSION_IS_LOCKED / transient-read-only retry classification, stop-on-first-failure, fresh query_id per attempt published synchronously via onStatementStart so Cancel's KILL QUERY always targets the live statement). Constructible without App/AppState/ DOM; deps injected ({runQuery, killQuery, ctx, now, uid, retryMs, sleep, sqlString}) — deliberately no wallNow (the F6 wave-clock invariant stays caller-side) and deliberately kill(queryId) instead of the issue's cancel(operationId) registry (cancellation stays caller-owned; see the issue comment). Behavior, wire format, retry rules, and error strings are byte-identical. All four callers migrated in one pass and app.runReadInto deleted: run()/runScript() (app.ts), dashboard tiles + filter sources (dashboard.ts), detached Data view (results.ts) → app.exec.executeRead / executeScript; cancel() → app.exec.kill. fake-app + ~70 test-mock sites renamed to the exec shape. Type homes tightened along the way: ResultSort → core/sort.ts and ScriptEntry → core/script-result.ts (old paths re-export, zero importer churn); filterExecution/panelExecution params bags and tab.chSession now carry their real wire types, deleting every as-cast at the execution call sites. Metrics (issue Phase-0 requirement): app.ts 2964 → 2878 lines; the App contract loses runReadInto and gains one nested exec member; the new service is 264 lines at 100/97.87/100/100 per-file coverage. Gate: npm test (103 files / 2940 tests) green, npm run build green, e2e chromium+webkit fully green, firefox green serialized (--workers=1; the parallel-run timeouts are the documented environment flake). Part of #276 (phases 0-1 of 5; roadmap #68 updated). Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 17, 2026
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.
What & why
Part of #276 — phases 0–1 of the app.ts → application-services refactor (plan and considered API deviations recorded in the issue's first comment; roadmap #68 carries the per-phase track).
Phase 0 — guard + baseline. New
build/check-boundaries.mjs(zero-dependency import walker, wired intopretestascheck:arch) enforces the day-1 architecture rule:src/application/**must not importsrc/ui/**orsrc/editor/**(type-only imports count). Later phases extend the rule set. The issue's required characterization coverage was audited and already exists (app.test.ts/dashboard.test.ts: streaming run, explicitFORMAT, cancel + partial results, the script retry matrix incl. the verbatim "may have executed" warning, Basic + OAuth refresh, both export cancellations, schema-graph stale-write protection, save/share validation) — no re-adds needed. Metrics per the issue: app.ts 2964 → 2878 lines;Appcontract ~189 members,runReadIntoremoved, one nestedexecmember added.Phase 1 —
QueryExecutionService.src/application/query-execution-service.ts(264 lines, 100/97.87/100/100 per-file coverage) now owns the shared request/stream/normalize core (formerlyapp.runReadInto) andrunScript's per-statement transport loop:SESSION_IS_LOCKED/ transient-read-only retry classification, stop-on-first-failure, and a freshquery_idper attempt published synchronously viaonStatementStartso Cancel'sKILL QUERYalways targets the live statement. Constructible withoutApp/AppState/DOM; every side effect injected ({runQuery, killQuery, ctx, now, uid, retryMs, sleep, sqlString}). Behavior, wire format, retry rules, and error strings are byte-identical.All four callers migrated in one pass,
app.runReadIntodeleted (no compatibility shim): workbenchrun()/runScript(), dashboard tiles + filter sources, and the detached Data view useapp.exec.executeRead/executeScript;cancel()usesapp.exec.kill(queryId).fake-appplus ~70 test-mock sites renamed to theexecshape.Considered deviations (details in the issue comment):
kill(queryId)instead of the suggestedcancel(operationId)registry — cancellation stays caller-owned (AbortController+ generation counters), per the issue's own rule 5; a registry would add long-lived mutable service state nobody consumes.wallNow— the F6 wave-clock invariant (params snapshot before auth awaits) stays caller-side and cannot move by accident.Type tightening that fell out of review:
ResultSort→core/sort.ts,ScriptEntry→core/script-result.ts(old import paths re-export — zero importer churn);filterExecution/panelExecutionparams bags andtab.chSessionnow carry their real wire types (Record<string, string | number>/string), deleting everyas-cast at the execution call sites.Verification: full gate green (103 files / 2940 tests + tsc + check:arch),
npm run buildgreen, e2e chromium + webkit fully green, firefox green serialized (--workers=1; the parallel-run timeouts are this machine's documented environment flake).Acceptance criteria this PR completes:
QueryExecutionServiceconstructible/tested withoutApp/AppState/DOM ✅; workbench, dashboard tiles, and detached reads share one execution service ✅;src/application/**has nosrc/ui/**imports (enforced) ✅.Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updated if behavior or the deployed surface changed🤖 Generated with Claude Code