From ea52735b126b68c3fefdb4a1008a16321baa317e Mon Sep 17 00:00:00 2001 From: debont80 Date: Thu, 2 Jul 2026 12:21:57 -0400 Subject: [PATCH] docs(audit): check off CL4 + CC5 (Batch 21) Co-Authored-By: Claude Fable 5 --- CODE-AUDIT.md | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index aa4d0a9..848d2f5 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -1971,15 +1971,14 @@ split — but that style is unenforced and several "do the same thing two ways" *Fixed: every listed duplicate now has one home — renderer `lib/formatters`/`urlHelpers`/`id` + `@shared/format`, `MediaKind` single-sourced (L105); main `lib/exec.ts` (the five `execFile` wrappers) and now `lib/lineBuffer.ts` (the two stdout-split loops). The `lib/` dirs exist on both sides.* -- [ ] **CC5 — Async patterns are mixed.** Three I/O idioms: callback-wrapped `new Promise` (execFile ×5), +- [x] **CC5 — Async patterns are mixed.** Three I/O idioms: callback-wrapped `new Promise` (execFile ×5), `async/await`+`fetch` (updater check, sync), and event-emitter-wrapped Promise (`net.request` in updater). Renderer mixes `.then().catch()` (stores) with `async/await`+try/catch (components). **Standard:** `async/await` everywhere; a shared `execFileAsync`; wrap event-emitter APIs once in a helper. - *Partly done: the `execFile ×5` idiom is gone (one `execFileAsync`, CC4), so the app-code default is - `async/await`. The one remaining event-emitter-wrapped Promise is the updater's `net.request` streaming - download — its "wrap once in a helper" is **CL4**, deferred with it (it's security-critical installer- - download code — host re-validation per redirect + SHA-256 verify + truncation guards — that can't be - live-verified in this environment, so it isn't refactored unattended). Left open until CL4 lands.* + *Closed with CL4 (Batch 21): the `execFile ×5` idiom was already gone (one `execFileAsync`, CC4), and the + last event-emitter-wrapped Promise — the updater's `net.request` streaming download — is now wrapped once + in `streamInstallerToFile`, extracted under 14 behavior-pinning tests. `async/await` + single wrappers hold + across the app code.* - [x] **CC6 — Five failure-signaling conventions.** `{ ok, error }` result objects (download/updater/ ytdlp/backup/IPC), `throw` (`assertHttpUrl`), `null` (indexerCore), `[]` (history/sources read errors), and a bare **error string** (`reveal.safeOpenPath`). `cleanError` is applied to some spawn stderr but not @@ -2131,14 +2130,18 @@ filed. Several listed categories are **N/A** to this stack and worth recording s `startDownload` is now a linear pre-flight (binary checks → URL normalise → concurrency guard → cookies → spawn → metadata probe → wire). `getTitle` is a getter because the parallel metadata probe may fill the resolved title in after wiring is set up. Behaviour unchanged.* -- [ ] **CL4 — Deep nesting: `updater.downloadAppUpdate`** — Promise → `net.request` → `response` → `data` +- [x] **CL4 — Deep nesting: `updater.downloadAppUpdate`** — Promise → `net.request` → `response` → `data` with nested conditionals/teardown is the hardest-to-follow block. **Fix:** extract a `streamToFile` helper. - *Deferred (deliberate, needs a watched pass): this is the security-critical installer download — - per-redirect host re-validation, streaming SHA-256 verification, truncation detection, idle-timeout, and - backpressure teardown all interlock in that block. Extracting `streamToFile` is worthwhile but a mistake - here is a security regression, and an app-update download can't be live-verified in this environment, so it - isn't refactored unattended. Grouped with CC5's "wrap the event-emitter API once" — both land in a watched - session on the updater.* + *Fixed (Batch 21, pin-then-refactor): the security-critical invariants were first locked with 14 + behavior-pinning tests at the public `downloadAppUpdate` boundary + ([test/updaterDownload.test.ts](test/updaterDownload.test.ts), scripted `net.request` fake) — untrusted + initial URL, per-hop redirect re-validation on both the checksum fetch and the installer, missing/malformed + checksum refusal, SHA-256 mismatch + partial cleanup, truncation detection, idle-timeout, checksum-phase and + mid-download cancel, HTTP-error status, combined-checksum line matching, and progress events. Then the + nested block was extracted verbatim into `streamInstallerToFile` (redirects/hashing/timeout/truncation/ + teardown in one once-wrapped helper); `downloadAppUpdate` is now a linear pre-flight (trust-check → + checksum → stream), with the B6 cancel-slot ownership kept beside the module state it guards via + `onCancelReady`/`onSettled`. All pins passed before AND after the extraction — no behavior change.* - [x] **CL5 — Superfluous exports.** `PROGRESS_TEMPLATE` (L167) and `getManagedBinDir` are `export`ed but used only within their own module. **Fix:** make them module-private. - [x] **CL6 — Redundant wrappers** (minor): `MediaThumb`'s `kind === 'audio' ? 'audio' : 'video'` (L15),