Quality hardening: real bug fixes + SOLID/DRY refactors + honest coverage + tooling spine#46
Open
alichherawalla wants to merge 196 commits into
Open
Quality hardening: real bug fixes + SOLID/DRY refactors + honest coverage + tooling spine#46alichherawalla wants to merge 196 commits into
alichherawalla wants to merge 196 commits into
Conversation
Correct the coverage measurement to the true testable surface: all:true + .ts include (was all:false, which hid ~63% of the surface behind a flattering 77%), exclude native/IPC/spawn shells (logic extracted to measured siblings) and e2e-covered .tsx components. Add a React render harness (jsdom + @testing-library/react + resolve aliases + .tsx test glob) so renderer components get real mount tests. Floors ratcheted to measured (95/90/93/96). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…merge) An explicit kvCacheType (q8_0) silently reverted to f16 because a performanceMode patch applied MODE_PRESETS and unconditionally overwrote the user's kvCacheType/flashAttn/ctxSize, persisting the clobber — re-triggered on every mode (re)pick. Fix: a pure applyModePreset() that MERGES — a preset only fills fields the user hasn't explicitly pinned (userExplicit set, persisted + reloaded). Extract buildLaunchArgs() as the single source for the launch argv. Terminal-artifact test drives the real persist->restart->launch-args round-trip (q8_0 still in --cache-type-k after a balanced pick + restart), red-proven 3 ways. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two data-layer/presentation drift bugs (§A). (1) Image composer kept local
copies of the active model + steps: the dropdown never wrote through to
setActiveModalModel (diverged from the Active-models panel) and a model-change
effect stomped the user's steps ("showed 10, generated 28"). Now the dropdown
binds to the owning source and steps/size/seed/negative persist per-model
(override ?? default resolver). (2) Reasoning/thinking showed live but was
never persisted, so it vanished on conversation remap — now carried through the
message context blob and restored by mapRagMessages. Terminal-artifact tests:
a real <MemoryChat/> mount asserts the generateImage payload carries the user's
steps + model; a real DB round-trip asserts reasoning restores. Red-proven.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tral) Pull the decision logic out of I/O/IPC shells into zero-import siblings the default runner can exercise, each unit-tested; originals re-import (no duplication, no behavior change): files-classify, tts-logic, vectors-predicates, mcp-parse-data-url, rag/prompt, skills-parse, ipc-query-logic, tools-parsers, + export isActionTool from the MCP connector extension. Full suite stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isProActive/toInfo + the Keygen JSON:API parsers pulled out of the Keychain/fetch shells (behavior-neutral) and unit-tested — the app-wide pro gate now has real coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… theme, reasoning round-trip) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
docs/CODE_AUDIT.md: ~40 evidence-based SOLID/DRY findings (4 real bugs + the renderer-has-no-store-layer root cause), prioritized, with a scope note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three divergent ext->MIME maps (ogcapture protocol in index.ts, the media HTTP server, image-attachment sniffing in data-url.ts) meant adding a format to one served the wrong/absent type on the other paths. Collapse to a single mimeForExt(ext, fallback) map in src/main/mime.ts; each caller keeps its own fallback (file-serving -> octet-stream, image attach -> image/png) so every existing path is byte-identical. Two real bugs fixed by the consolidation: - tools.ts inlined an endsWith(.png) ? png : jpeg guess, so a .webp attachment was labelled image/jpeg (vision model may reject). Now routed through mimeFromExt -> image/webp. - gif attachments resolved to image/png (wrong); now image/gif. Tests: mimeForExt map/dot-strip/case/fallback + a source-read guard that tools.ts no longer re-inlines the png/jpeg ternary; data-url gif assertion corrected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The knowledge-base file picker (rag-ipc) hand-maintained its own extension allowlist, separate from files-classify's IMAGE/AUDIO/VIDEO sets that the processor routes on. They had already drifted: the picker omitted gif/bmp/heic/opus/oga/aiff/aif/avi that the router handles, so a user could not select files the processor was perfectly able to ingest. Add DOC_EXT + uploadPickerExtensions() to files-classify (single source) and build DOC_FILTERS from it. Purely additive to the picker (no format removed); picker and router can no longer disagree. Tests: every router-classified format is offered; every offered format resolves to a real handler; deduped; source guard that rag-ipc builds from the helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n agentic mode The renderer's keyword auto-route (looksLikeImageRequest) and the agent's own tool decision both decided "is this an image request?" for the same turn and could disagree. With tools/connectors on, a "draw ..." message tripped the renderer's direct-generate fast-path and NEVER reached the generate_image tool — the root of the image-gen-as-tool bug. Extract the pre-decision into shouldAutoRouteImage() (pure, in image-intent) with the fix baked in: when the agentic path owns the turn, the renderer returns false and lets the model decide (image generation is a tool there). Behavior is unchanged in plain chat — the keyword auto-route still fires. Tests: shouldAutoRouteImage every branch (mode/availability/agentic/keyword); and a render-harness wiring proof on the terminal artifact — tools ON + "draw a dog" crosses on toolChat with NO direct generateImage; tools OFF auto-routes to generateImage with the cleaned "a dog" prompt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ervice The magic-byte + min-size integrity check was byte-identical in models-manager.importLocalModel and LLMService.validateGguf — two hand-kept copies. Extract to models/gguf.ts (isValidGgufHeader pure + isValidGgufFile with injected fs); both call sites delegate. Behavior-neutral. Tests: pure header judgement (size floor, magic match/mismatch) + isValidGgufFile against real temp files (valid, truncated, wrong-magic, missing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two label maps were each defined twice in the renderer: - artifact badge labels: identical in ArtifactCanvas and ProjectsScreen - model-kind labels: ModelsScreen vs StoragePanel (already drifted — StoragePanel carried an extra `other` bucket the other lacked) Collapse each to one source: lib/artifact-labels.ts (ARTIFACT_KIND_LABELS + artifactKindLabel) and lib/model-kind-labels.ts (MODEL_KIND_LABELS superset + modelKindLabel). The long-form fallback title in main/artifacts.ts (labelFor: "HTML page"/"React component") is a separate concern and left as-is. Behavior-neutral (model-kind gains `other` on the Models screen, previously an identity fallback anyway). Tests: exact label maps locked + fallback path for both helpers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e DIP/SRP tail Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The exact ffmpeg argv (-y -i <in> -vn -ar 16000 -ac 1 -f wav <out>) plus the 10-minute decode timeout were copy-pasted into whisper-cli, whisper-server, and parakeet-cli — a change to the sample rate / channels / timeout meant editing three sites in sync. Extract decodeToWavArgs + DECODE_TIMEOUT_MS to transcription/ffmpeg-decode.ts; all three call it. Each keeps its own ffmpeg resolution + tmp lifecycle. Behavior-neutral (byte-identical argv). Tests: exact argv + the 16kHz/mono/wav/no-video contract + input/output placement + the timeout constant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
chatStream (message -> answer) and streamChat (raw messages + tool calls) each
carried a ~40-line copy of the same streaming Promise body: buffered newline
split, parseSseLine, reasoning/answer routing, tool-call accumulation, timeout,
cooperative abort, and error handling. Extract to llm/stream.ts streamCompletion
(returns { content, toolCalls }); chatStream takes only content (it sends no
tools), streamChat returns the pair. Behavior-neutral. Also drops the now-unused
http / parseSseLine / splitter imports from llm.ts.
Tests: NEW integration test drives streamCompletion against a real local SSE
server — content stream, reasoning channel, delta split across TCP boundaries,
tool-call reassembly, non-200, timeout, mid-stream abort. This covers transport
logic that previously lived (twice) in the coverage-excluded llm.ts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iterals
modalityQueue.run({ tier: 2, label: 'chat', evicts: ['image'] }) was inlined 4×
in ipc.ts, and the image counterpart once in imagegen. Export frozen CHAT_JOB /
IMAGE_JOB from the queue module; both callers use them. The mutual-eviction
contract (chat evicts image, image evicts llm — they can't co-reside on unified
memory) is now stated once. Behavior-neutral.
Tests: the two descriptors' shape, mutual eviction, and frozen-ness.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The chat-list preview's markdown link was text-cyan-300 — cyan is not in the Off Grid palette (DESIGN.md: emerald is THE accent for links). Switch to the emerald token (text-green-500), matching the composer's link style. The three per-surface markdownComponents maps are intentionally styled per density (dense ChatDetail, compact MemoryChat, preview ChatList) and NOT merged — that would change visuals, not a behavior-neutral refactor. Test: source guard — no cyan class in ChatList, link uses the emerald token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`dateStr.replace(' ', 'T') + 'Z'` (parse a zone-less SQLite UTC timestamp) was
inlined in ChatDetail (2×) and ChatList. Export parseSqliteUtc from lib/time
(with the tz guard timeAgo already used) and route all three through it; toDate
delegates to it too, so the module has one parse. Behavior-neutral — timeAgo's
suite stays green and the new parse matches the old inline exactly.
Tests: parseSqliteUtc UTC handling + equivalence to the old inline + no
double-zone; timeAgo's 12 existing cases still pass over the unified parse.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ChatDetail and ChatList each parsed a session id ("<model>-<dashed-title>") into
model name / readable title / LLM label with identical inline code — a drift
risk on how titles render across the two views. Extract parseSessionId to
lib/session-id.ts; both consume it (ChatDetail's inline model-label replace now
uses the shared llmLabel too). Behavior-neutral.
Tests: model/title split, underscore->space label, no-dash fallback, empty-title
fallback to the raw id, leading-dash edge (indexOf > 0 guard).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ng() findBinary hand-rolled the first-existing-path loop that bin-resolution.existing() already owns (and whisper-cli/parakeet-cli use). Build the candidate paths and delegate. Behavior-neutral (existing() adds a defensive try/catch around existsSync; a bad path still resolves to "not found"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The LoRA/checkpoint extension set /\.(safetensors|ckpt|gguf|pt)$/i was inlined 3× in imagegen (the LoRA lister's include filter + display-name strip, and the LoRA-path builder's default-extension). Extract CHECKPOINT_EXT + hasCheckpointExt / stripCheckpointExt / ensureCheckpointExt into imagegen/model-filter.ts (pure, tested); all three sites use them. Behavior-neutral. Tests: the four extensions (case-insensitive), strip, and safetensors default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…engine refactors Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ranching
The agentic loop branched on tool identity: `if (c.name === 'search_memory')`
(build citations) `else if (=== 'generate_image')` (record prompt) `else` generic.
A tool with a side channel meant a new branch, and search logic was written twice
(the tool's text-only run + the loop's citation version).
Push the decision behind the interface: ToolDef.run(args, ctx) returns
string | ToolResult { text, sources?, imageRequest? }; the loop normalizes and
merges uniformly (sources deduped into `unified` across rounds; last non-empty
imageRequest wins). search_memory now owns its citations + self-exclusion (via
ctx.conversationId); generate_image owns its deferred imageRequest. Adding a tool
with a side channel needs zero loop edits. execute() folded into one runTool()
dispatch (ext → built-in → error text). Behavior-neutral for every existing path;
one intentional hardening — a throwing extension tool now yields error text
instead of aborting the whole turn (matches built-in behavior).
Tests (real loop, only the model + search boundaries faked): search_memory
citations surface in r.unified with excludeChatId self-exclusion; dedup across two
rounds (dynamic per-round hits); empty-hits text; + a DIP guard asserting the loop
source no longer contains `c.name === 'search_memory'/'generate_image'`. Existing
18 loop cases (generate_image last-wins/empty, extension routing, calculator, step
order) stay green; MemoryChat render wiring unaffected (return contract unchanged).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ADME CI ran `npm test` (no --coverage), so the coverage ratchet was only enforced on pre-push — a regression could land via CI. Switch the CI test step to `npm run test:coverage`, which runs the same suite AND fails on any drop below the vitest.config.ts thresholds (same gate as pre-push). Make the pro/** threshold group conditional on pro being checked out (existsSync guard), so a core-only CI run (fork without the cross-repo token) measures + gates core alone instead of erroring on an empty pro/** glob. README: four coverage badges (statements/branches/functions/lines, emerald token) reflecting the measured `npm run test:coverage` numbers; a comment notes the CI-enforced floor and to refresh them as it rises. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The optional app-name filter — the `appName && appName !== 'All'` guard plus the
`%${appName}%` LIKE wildcarding — was hand-inlined SEVEN times across
db:get-memories and the rag:chat vector / FTS-fallback / message / summary /
entity / entity-fact queries, each with a different column, so the facet filter
could silently disagree between result sets. Extract appNameLikeClause(appName,
column) to ipc-query-logic (pure, tested); every site builds its filter from it
and keeps its own connector / subquery shape. Behavior-neutral.
Tests: null for All/empty/undefined; clause+param for a specific app; per-column;
+ a source guard that ipc.ts has NO remaining inline appName+LIKE guard and uses
the helper (the guard caught 3 sites beyond the first pass).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The no-FTS LIKE search built its WHERE clause AND its bound params by hand in TWO
places per source — the Sources-rail facet COUNT query and the hit builder — for
chat / doc / meeting (byte-identical) plus frame. If a column were added to one
copy, a source's facet count would silently disagree with its actual results.
Extract LIKE_COLUMNS (the per-source column sets, single source of truth) +
likeMatch(columns, terms) → { where, args } into search-ranking (pure, tested);
facet and hit both build from it, so clause and params can never drift apart.
Behavior-neutral (frame's single column gains harmless parens; all other SQL
byte-identical — asserted in tests).
Tests: OR-per-term/AND-joined clause, per-term wildcard params, placeholder count
always equals params length across every column set, single-column + empty-terms
edges, and the column sets locked to what search.ts used.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-connector setup instructions (SETUP_HINTS) lived as a map inside the ConnectorsScreen view — catalog DATA in the presentation layer (§A). Move it beside CONNECTOR_CATALOG in connectorCatalog.ts as CONNECTOR_SETUP_HINTS + setupHintFor(id); the view reads via the helper. Behavior-neutral. Tests: every hint id is a real catalog connector (orphan-hint guard), setupHintFor known/unknown, and a source guard that the hints are no longer inlined in the view. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erified-only remainder Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The data-layer drift sweep's TIER-1 items were already fixed + tested this branch; verified each against the shipped code and moved them to RESOLVED with evidence (file:line + test names). Close the two remaining safe TIER-2 items: - preload setLlmSettings type now carries kvCacheType/flashAttn/gpuLayers/threads/ batchSize/performanceMode (was a type-check blind spot; runtime already passed them). - Settings identity fields commit on Enter too, not just blur (no more lost edit if the screen closes without blurring). ctxSize crash-recovery halving is documented as by-design; ephemeral view prefs are by-design. GAPS_BACKLOG OPEN section is now empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…progress on session limits 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
1 participant
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 this is
The
chore/quality-hardeningbranch: fix the real bugs, land the SOLID/DRY refactorbacklog, bring coverage to an honest floor, and stand up the code-quality tooling spine.
One PR per repo (pro is a separate repo — companion PR in
desktop-pro, same branch name).🐞 Real bugs fixed (each with a regression test)
src/main/mime.ts— fixed a.webpattachmentmislabelled
image/jpeg(vision model may reject) andgif→image/png.classify sets (was omitting gif/bmp/heic/opus/avi the router handles).
pre-decides when the agentic tool path owns the turn.
survives reload, KV-cache choice survives restart.
Refactors (SOLID/DRY — behavior-neutral, tested)
ToolDef.rununiform dispatch (DIP); onemimeForExt;appNameLikeClause(7 rag:chat sites);likeMatch/LIKE_COLUMNS(facet counts == results);epochMsSql; sharedisValidGguf,artifact-kind/model-kind label maps,
CHAT_JOB/IMAGE_JOB,ffmpeg-decode,streamCompletion(one SSE transport),
parseSqliteUtc,parseSessionId; ChatList link cyan→emerald token.Coverage — honest floor, enforced
all:true+ the true testable-surfaceinclude/exclude(native/IPC/spawn shells excluded, eachcovered by e2e/smoke/test:db). Ratchet floor in
vitest.config.ts, blocked on pre-push AND CI.Now 97.1 / 92.1 / 96.0 / 98.1 (stmts/branches/funcs/lines). README carries the badges.
Quality-tooling spine (new)
circular/unresolvable/dev-dep/orphans. 0 violations.
noUnusedLocals/noUnusedParameters/allowUnreachableCode:false.npm run knip) — module-graph dead code; first pass deleted 27 unused template UI files.no-unnecessary-condition(warn-ratchet) — dead-branch detection.Evidence
npx tsc --noEmit— bothtsconfig.node.jsonandtsconfig.web.json: 0 errors.npm run test:coverage— 2110 passed, 1 skipped; floor held (97.1/92.1/96.0/98.1).npm run depcruise— 0 violations.Honest gaps (not blocking this PR)
(regression safety) + structural health; they do NOT verify end-to-end functionality (real model
chat/image-gen, connectors, capture). Recommend the Playwright e2e tour + a live golden-path smoke
before release. Screenshots therefore pending the e2e run.
docs/CODE_AUDIT.md(now a clean "what's left" list): ImageRuntime,imagegen/ipc/database SRP splits, tabler→Phosphor, and the warn-ratchet lint backlog
(no-unnecessary-condition 289, sonarjs-on-pro 295, knip deps/exports). Each lands as its own
verified change — not this PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by Gitar
dependency-cruiserfor architectural boundary enforcement andknipfor dead-code elimination.ipc-query-logic.tsand refined tool dispatch using a uniformToolResultstructure.agent: false) forhttp.requestto preventECONNRESETin multi-round tool loops.userExplicitpin-set for LLM settings to ensure user-selected performance modes and cache configurations persist across restarts.noUncheckedIndexedAccessacross the project, resulting in 14/14 module migrations and associated guard logic.This will update automatically on new commits.