Skip to content

fix(release): the blockers and hardening from the 1.8.0 review - #174

Merged
EtienneLescot merged 5 commits into
release/1.8.0from
fix/release-1-8-0-review
Jul 27, 2026
Merged

fix(release): the blockers and hardening from the 1.8.0 review#174
EtienneLescot merged 5 commits into
release/1.8.0from
fix/release-1-8-0-review

Conversation

@EtienneLescot

@EtienneLescot EtienneLescot commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Findings from the review of release/1.8.0 (main…release = 164 commits, 563 files, +110k/−26k), applied. Three commits, reviewable independently.

−1848 / +665. Most of it is deletion.


🚨 The installers had no speech-to-text

electron/native/bin/ is gitignored, and build-whisper-stt.yml publishes the binaries as artifacts of its own workflow run. Nothing carried them into build.yml — its only download-artifact steps collect finished installers in the publish job. There is no runtime download either: modelManager.ts fetches the model, resolveBinaryPath() only probes paths and returns null.

Result: transcription and captions — headline 1.8.0 features — would fail in every shipped build with whisper-stt-server binary not found; build it via scripts/build-whisper-stt.sh, a developer message shown to end users.

scripts/stage-whisper-stt.sh now stages them in all four build legs and fails the build if they are missing. A red build beats a silent one. Artifact retention 30 → 90 days, since a release build now depends on it.

🚨 CI never ran on this branch

ci.yml triggered on [main, feat/ai-edition] only. gh pr checks confirms it: PRs #167, #168 and #169 — 30k+ lines of deletion and refactor — merged into release/1.8.0 with lint, typecheck, tests, build and the PR-title check all skipped, and CodeRabbit reporting "reviews are disabled for this base branch". Added release/** to both triggers.

⚖️ Providers shipping borrowed vendor credentials

openai-oauth and copilot-proxy reached a user's subscription by presenting the vendor's own OAuth client ID (app_EMoamEEZ73f0CkXaXp7hrann, Iv1.b507a08c87ecfe98) and Editor-Version: vscode/1.96.2 against endpoints reserved for first-party clients. In a signed installer on the Microsoft Store and winget, so the exposure landed on our users' accounts.

Both vendors now offer a sanctioned surface, so this is a migration, not an amputation — tracked in #173. Removed here because the constants had to leave the binary; hiding the UI would not have done it.

The eight API-key providers are untouched.

🐛 Lost saves on the timeline path

DocumentService serialises saves of a project through a per-instance queue — its own writeProject comment records that this race destroyed two real project files. handlers.ts built a fresh instance for every runTimelineOperation, so a timeline op and a document.save ran on two queues against one path. temp+rename still kept the file valid JSON, so the symptom was a silently lost save rather than corruption. Hoisted to one instance, alongside LlmConfigStore (two sync readFileSync + a safeStorage decrypt, on every chat message).

🔒 Smaller hardening

  • open-external-url passed the renderer's string straight to shell.openExternal, which hands it to the OS handler — file:, ms-msdt:, UNC paths are launch primitives there. Allowlisted to http/https/mailto.
  • The native bridge marked every INTERNAL_ERROR retryable and forwarded error.message verbatim, so main-process errors quoted absolute paths into UI strings. Now non-retryable by default, path-redacted for the renderer, logged in full on the main side.
  • OPENSCREEN_WHISPER_HOST overrode an explicit --host, so an env var could widen a shipped unauthenticated server's bind address. Fallback only now, and loopback-only.

📄 LGPL notice

fetch-ffmpeg.mjs goes to real lengths to keep the build LGPL rather than GPL — pinned dated tag, SHA-256 before extract, -L/-buildconf/-encoders all checked. But nothing carried the attribution or the source offer into the installer. THIRD-PARTY-NOTICES.md added and shipped into resources/ with LICENSE.

Also stops shipping the static ffmpeg.exe on Windows: the compositor addon dlopens the av*.dll set, but no runtime code spawns the exe (verified — electron/media/ffmpegCapabilities.ts, its old runtime twin, went with the web export path). It was adding a large binary and an LGPL obligation to every installer for a file nothing opens.

🧪 Test flake

document-service.test.ts > "leaves valid JSON when a long and a short save race" timed out at vitest's 5s default in the full run and passed alone in 1.7s — 12 rounds of real fsync+rename, sensitive to parallel load. Explicit 30s timeout: the "this actually hung" threshold, not the expected runtime.


Verification

tsc --noEmit clean
biome check . 0 errors, 7 warnings (all pre-existing, in tests)
vitest --run 1119 / 1119 across 99 files

Baseline before this branch was 1108 tests with 1 failing.

Two things a reviewer should check

  1. The four CI build legs are not exercised by this PR. stage-whisper-stt.sh passes bash -n, the YAML parses, and darwin-${{ matrix.arch }} matches the whisper workflow's tags — but gh run download has not run for real. Worth one workflow_dispatch of build.yml before cutting the RC, rather than finding out during the release.
  2. typecheck-tests is advisory on purpose (continue-on-error: true). tsconfig.json includes only src + electron and excludes **/*.test.ts, so no test file has ever been typechecked and 82 errors have accumulated. The job makes the drift visible and stops it growing; flip the flag off once the count is zero. Fixing all 82 here would have buried everything above.

Also noted, not touched: npm run i18n:check already fails on main (missing captions.* keys in ar). Unrelated to this branch and not part of CI.

…ials

`openai-oauth` (ChatGPT) and `copilot-proxy` (GitHub Copilot) reached a
user's subscription by presenting GitHub's and OpenAI's own OAuth client
IDs (Iv1.b507a08c87ecfe98, app_EMoamEEZ73f0CkXaXp7hrann) and an
`Editor-Version: vscode/1.96.2` User-Agent against endpoints reserved for
first-party clients — api.github.com/copilot_internal and
chatgpt.com/backend-api. That shipped in a signed installer, so the risk
landed on our users' GitHub and OpenAI accounts.

Both vendors expose a sanctioned surface now, so this is a migration, not
an amputation:
  - GitHub: the Copilot SDK, where we register our own OAuth App and pass
    the user's `gho_` token. Their own client ID does not work against
    copilot_internal, which is exactly why every tool on that path
    borrows VS Code's.
  - OpenAI: `codex app-server` (JSON-RPC over stdio), which inherits the
    user's own `codex login` — no client ID ships at all.

Both are agent runtimes rather than /chat/completions, so they land in
their own PR. Removed here: the two registry entries, the four device
flows, the Copilot runtime-token swap, the `chatgpt-account-id` threading
through chat-service/caption-translate/chat-model, the bridge actions,
their contracts, and the device-challenge screen (the provider cards
themselves disappear with the registry entries). Credential blobs written
by an older build still load — getCredential() matches on a usable
`apiKey`, not on `kind`, so no migration is needed.

provider-registry.test.ts is the guard: it fails if a provider comes back
with a non-api-key auth kind or a first-party-only base URL.
…only STT server

Four small hardening fixes found while reviewing release/1.8.0.

DocumentService serialises saves of a project through a per-INSTANCE
queue — its own writeProject comment records that this race destroyed two
real project files. handlers.ts built a fresh instance for every
runTimelineOperation, so a timeline op and a document.save ran on two
queues against one path. temp+rename still kept the file valid JSON, so
the symptom was a silently lost save rather than corruption. Hoisted to
one instance, alongside LlmConfigStore, whose constructor was doing two
sync readFileSync plus a safeStorage decrypt on every chat message.

open-external-url passed the renderer's string straight to
shell.openExternal, which hands it to the OS handler — `file:`,
`ms-msdt:` and UNC paths are launch primitives there. Allowlisted to
http/https/mailto.

The native bridge marked every INTERNAL_ERROR retryable, including
permanent ones, and forwarded error.message verbatim to the renderer,
where main-process errors quote absolute paths into UI strings. Now
non-retryable by default, path-redacted for the renderer, and logged in
full on the main side.

whisper-stt-server let OPENSCREEN_WHISPER_HOST override an explicit
--host, so an env var could widen a shipped, unauthenticated server's
bind address. It is now a fallback only, and refuses anything but
loopback.

Also gives the save-race test an explicit 30s timeout: 12 rounds of real
fsync+rename take ~1.7s idle but exceed vitest's 5s default when the
suite runs files in parallel — it failed in the full run and passed
alone.
…nto installers

Two ways this release could ship broken, and one obligation it was missing.

CI never ran on this branch. ci.yml triggered on [main, feat/ai-edition]
only, so PRs #167, #168 and #169 — 30k+ lines of deletion and refactor —
merged into release/1.8.0 with lint, typecheck, tests, build and the
PR-title check all skipped, and CodeRabbit reporting "reviews are
disabled for this base branch". Added release/** to both triggers.

The installers had no speech-to-text. electron/native/bin/ is gitignored
and build-whisper-stt.yml publishes the binaries as artifacts of its own
run; nothing carried them into build.yml. The model is downloaded at
runtime, the binary is not, so resolveBinaryPath() would find nothing and
transcription and captions would fail with a "build it via
scripts/build-whisper-stt.sh" message shown to end users.
scripts/stage-whisper-stt.sh now fetches them in all four build legs and
fails the build if they are missing — a red build beats a silent one.
Artifact retention 30 -> 90 days, since a release build now depends on it.

Added THIRD-PARTY-NOTICES.md and shipped it (with LICENSE) into
resources/. We redistribute FFmpeg's LGPL shared libraries and
whisper.cpp; fetch-ffmpeg.mjs went to real lengths to keep the build LGPL
rather than GPL, but nothing carried the attribution or the source offer
into the installer.

Also stops shipping the static ffmpeg.exe on Windows
("!win32-*/ffmpeg.exe"): the compositor addon dlopens the av*.dll set, but
no runtime code spawns the exe — it is there for bench-export.mjs. It was
adding a large binary and an LGPL obligation to every installer for
nothing. Its licence-gate comment also still pointed at
electron/media/ffmpegCapabilities.ts, deleted with the web export path.

tsconfig.test.json makes the untypechecked half of the repo visible:
tsconfig.json includes only src + electron and excludes **/*.test.ts, so
no test file has ever been typechecked. It runs as an advisory CI job —
82 pre-existing errors, so gating on it today would just be red.
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1a869e56-8050-49e4-859f-0ec992554255

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/release-1-8-0-review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

continue-on-error left the job reporting "fail" on every PR forever. The
run concluded success and merges were never blocked, but a check nobody
can fix is a check everyone learns to ignore — which is worse than not
having one.

It now fails only when the count GROWS past a baseline, so the backlog
stays visible in the job summary without crying wolf, and a PR that adds
a new fixture-vs-schema drift is caught the day it lands. Baseline 80,
measured on this branch; lower it when you fix some, delete the logic at
zero.
@EtienneLescot
EtienneLescot merged commit 18a9696 into release/1.8.0 Jul 27, 2026
9 checks passed
@EtienneLescot
EtienneLescot deleted the fix/release-1-8-0-review branch July 27, 2026 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant