Replace Deno yt-dlp runtime with Electron Node#98
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/YtDlp.ts (1)
556-573:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid partial prepare state when Electron Node probe fails.
prepare()commits_ytDlpPath/_ffmpegPathbefore probing runtime, then throws on probe failure. On the next call, Line 580 skipsprepare()because_ytDlpPathis already set, and execution proceeds withjsRuntime: null.🐛 Suggested fix
async prepare(onStatus?: StatusReporter): Promise<void> { - this._ytDlpPath = await this.binaryManager.ensureYtDlp(onStatus) - this._ffmpegPath = await this.binaryManager.ensureFFmpeg(onStatus) + const ytDlpPath = await this.binaryManager.ensureYtDlp(onStatus) + const ffmpegPath = await this.binaryManager.ensureFFmpeg(onStatus) await this.binaryManager.ensureFFprobe(onStatus) const electronNode = await probeElectronNodeRuntime() if (electronNode.ok) { const ytDlpSource = this.binaryManager.getLastDiagnostic?.('yt-dlp')?.source - this._jsRuntime = {...electronNode.runtime, allowRemoteComponents: shouldAllowRemoteEjsComponents(ytDlpSource)} + this._ytDlpPath = ytDlpPath + this._ffmpegPath = ffmpegPath + this._jsRuntime = {...electronNode.runtime, allowRemoteComponents: shouldAllowRemoteEjsComponents(ytDlpSource)} return } + this._ytDlpPath = null + this._ffmpegPath = null this._jsRuntime = null throw new Error(`Electron Node runtime unavailable: ${electronNode.reason}`) } async run(req: YtDlpRequest, signal?: YtDlpSignal): Promise<YtDlpResult> { - if (!this._ytDlpPath) await this.prepare() + if (!this._ytDlpPath || !this._jsRuntime) await this.prepare()Also applies to: 579-580
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/services/YtDlp.ts` around lines 556 - 573, prepare() assigns _ytDlpPath/_ffmpegPath (and calls ensureFFprobe) before probing Electron Node runtime, so a probe failure leaves the instance in a partially-prepared state; change prepare() to delay assigning this._ytDlpPath and this._ffmpegPath (and any state changes like this._jsRuntime) until after probeElectronNodeRuntime() succeeds, or if you prefer to keep the current call order, ensure you rollback/clear those fields on probe failure (e.g., set this._ytDlpPath = undefined; this._ffmpegPath = undefined) before throwing; update the prepare() implementation (the method named prepare, and use of probeElectronNodeRuntime and this.binaryManager.ensureYtDlp/ensureFFmpeg/ensureFFprobe) accordingly so subsequent calls do not skip initialization due to leftover state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/installer-smoke.yml:
- Around line 201-211: Replace the floating tag actions/upload-artifact@v7 with
a pinned commit SHA in every workflow that references it (search for the exact
string actions/upload-artifact@v7), fetching the commit hash that corresponds to
the v7 release from the upstream repo and replacing the tag usage with
actions/upload-artifact@<COMMIT_SHA>; ensure you use the same pinned commit SHA
in both workflow files that previously used `@v7` so both workflows reference the
identical immutable ref.
In @.github/workflows/runtime-binaries.yml:
- Line 20: Replace all GitHub Action version tags with pinned full commit SHAs:
find every usage like actions/checkout@v6 (and other tagged refs noted: lines
referencing actions at 24, 28, 43, 60, 64, 68, 75, 92) and change each to the
corresponding commit SHA for that action release; update workflow YAML anchors
that reference docker/setup-buildx-action, actions/setup-node, actions/cache,
etc., to use the exact commit SHA found on the action's GitHub releases/tags
page, and verify the SHA resolves and the workflow still runs (optionally add a
comment with the tag->SHA mapping for future updates).
- Around line 88-89: Add a brief inline comment next to the permissions:
contents: write setting explaining that this elevated scope is required to
create GitHub releases and upload build artifacts (release assets) so the
workflow can publish runtime binaries and maintain an audit trail; update the
workflow entry where "permissions: contents: write" is defined to include that
explanatory comment.
- Around line 3-6: Add a top-level concurrency key to the workflow to prevent
overlapping runs: under the root of the YAML (near the existing on: block) add a
concurrency stanza that defines a unique group name (e.g., using the workflow or
repo name and optionally matrix or github.ref) and set cancel-in-progress to
true so a new manual or scheduled run cancels any in-progress run. Ensure the
concurrency block is added at the same indentation level as on:, jobs:, etc.,
and reference the workflow identity (workflow name or github.workflow) in the
group to ensure correct grouping.
In `@AGENTS.md`:
- Around line 40-41: Update the Deno glossary entry in AGENTS.md to reflect that
Deno is no longer an active runtime/dependency: remove or rephrase the sentence
stating "Deno is required but not embedded in packaged resources" and instead
mark Deno as removed from runtime flows or as a historical/reference item;
adjust the description to point readers to the new source-of-truth architecture
and relevant implementation files (src/main/services/binary/DenoBinarySource.ts
and scripts/build/denoResolver.ts) if the entry is kept for archival or tooling
reference. Ensure the entry no longer implies Deno is part of the runtime
dependency chain.
In `@electron-builder.json5`:
- Around line 6-8: Update the build-tooling resolution so no 26.8.x artifacts
remain: add explicit overrides/resolutions for electron-builder-squirrel-windows
and app-builder-lib (pin to >=26.9.0 or the same 26.15.2 range used for
electron-builder) in package.json (or your package manager equivalent), then
regenerate the lockfile (bun.lock) by running the package manager's install
command to update dependencies; verify bun.lock no longer lists 26.8.x and
commit the updated lockfile.
In `@package.json`:
- Around line 29-32: The package.json scripts dev:local-manifest and
dev:fresh:local-manifest are duplicates of the runtime-manifest variants and
therefore never select the local manifest; update those two entries so they
invoke the local-manifest flow (e.g., call scripts/with-local-manifest.sh
instead of scripts/with-runtime-manifest.sh or set an explicit env flag like
LOCAL_MANIFEST=1 before running "bun run dev" / "bun run dev:fresh") while
keeping dev:runtime-manifest and dev:fresh:runtime-manifest pointing to the
runtime-manifest behavior.
In `@src/main/services/binary/BinaryDownloader.ts`:
- Around line 450-479: The loop over options.urls currently breaks on first
successful downloadFile call but performs integrity/size validation later, which
prevents trying subsequent URLs if validation fails; change the logic in the
loop inside BinaryDownloader (the code using downloadFile, pipeline,
cacache.put.stream, DownloadSizeMismatchError and mapCacheError) to perform the
stat/size check and cacache put (including pipeline) immediately after each
download attempt and only break/return on successful validation and cache
insert; on validation or cache errors remove the temporary downloadPath, set
lastError appropriately, respect options.signal abort, and continue to the next
URL, and only after the loop finishes without a valid artifact throw lastError
or a generic error.
In `@src/main/services/binary/BundledRuntimeBinaryIndex.ts`:
- Line 3: The bundled fallback index BUNDLED_RUNTIME_BINARY_INDEX currently has
entries: [], leaving no candidates for managed-binary recovery; update
BUNDLED_RUNTIME_BINARY_INDEX to include at least the known-good
RuntimeBinaryIndex entries for supported runtimes (populate the entries array
with the canonical runtime descriptors used as last-resort fallbacks), ensure
schemaVersion and generatedAt remain correct, and verify the entries conform to
the RuntimeBinaryIndex entry shape used by the runtime resolution logic so the
fallback path can resolve managed binaries when remote/last-known-good sources
are unavailable.
In `@src/main/services/binary/RuntimeBinaryIndexService.ts`:
- Around line 109-112: The code treats writeLastKnownGood failure as fatal when
a verified remote index is available; wrap the call to
this.writeLastKnownGood(remote.raw, remote.signature) in a try/catch inside
RuntimeBinaryIndexService so disk/permission errors don't abort loading: attempt
the write, and if it throws, catch the error, log it (including error details)
and still return the remote index object ({primary: remote.index, fallbacks:
[...] , source: 'remote'}); do not rethrow the error or change the returned
value on write failure.
In `@src/main/services/binary/RuntimeBinaryMaterializer.ts`:
- Around line 299-310: The existingInstall check incorrectly compares extracted
executable size/hash against archive values for zip entries; update the install
metadata (RuntimeBinaryInstallMetadata) to include executableSize and
executableSha256 (or similar names), ensure the code that writes the metadata
after extraction populates these fields, and then change existingInstall (method
existingInstall in RuntimeBinaryMaterializer) to validate
metadata.executableSize and metadata.executableSha256 (and keep checking
metadata.cacheKey, manifestHash and executablePath) instead of comparing to
entry.size/entry.sha256 so zip-extracted binaries use the stored extracted-file
values for size/hash verification.
In `@src/shared/dependencyPolicy.ts`:
- Around line 3-6: The change incorrectly hard-coded
RUNTIME_REQUIRED_DEPENDENCY_IDS and removed the harness-exception (skipDeno)
contract; revert blockingDependencyFailures to use the full DEPENDENCY_IDS set
and apply the DependencyRequirementPolicy/harness exception logic: iterate
DEPENDENCY_IDS, consult each dependency's DependencyDiagnostic and its
requiredBy.skipDeno flag (or use DependencyRequirementPolicy.isRuntimeRequired /
equivalent) to decide blocking state, and only include ids whose diagnostics
indicate non-runnable after honoring skipDeno; update blockingDependencyFailures
signature to reference DEPENDENCY_IDS, DependencyRequirementPolicy (or the
requiredBy.skipDeno check) and DependencyDiagnostic/DependencyId symbols.
In `@tests/unit/binary-manager-retry.test.ts`:
- Around line 27-29: The test helper currently escapes type-safety with "as
unknown as RuntimeBinaryMaterializer" in the materializer factory; replace the
cast by creating a properly typed test-double: declare a concrete materialize
mock (e.g., const materializeMock = vi.fn<Promise<string>,
[RuntimeBinaryManifestEntry]>(async candidate => ...)) and return { materialize:
materializeMock } typed as RuntimeBinaryMaterializer without using "as unknown
as". Do the same for the other occurrences in this file (the remaining
double-casts) by exposing minimal “port” objects typed to only the methods used
(e.g., a typed download/materializer port interface or specific mock variables)
instead of blanket unknown casts so the compiler will catch contract drift.
In `@tests/unit/browser-mock-scenarios.test.ts`:
- Around line 253-255: The test currently only checks
allMissing.blockingFailures length which can mask wrong blocking items; update
the assertion in the test that references allMissing and blockingFailures to
assert the exact blocking dependency IDs (e.g., verify the set contains 'yt-dlp'
and 'ffmpeg' and also include 'ffprobe' if expected) or compare equality with
the expected array of IDs so the test fails if the wrong dependency is marked
blocking; locate the assertions that reference allMissing.blockingFailures and
allMissing.dependencies (and the dependency keys 'yt-dlp' and ffmpeg) and
replace the length-only check with a deterministic check for the exact
IDs/contents of the blockingFailures set.
In `@tests/unit/smoke-all-script.test.ts`:
- Around line 54-56: Remove the brittle hard-coded assertion by deleting the
second expectation that compares Number(output) to 20; keep the derived
assertion using expectedPayloadCountFromSources() as the source of truth (i.e.,
retain expect(Number(output)).toBe(expectedPayloadCountFromSources()) and remove
expect(Number(output)).toBe(20)), referencing the test variable output and the
helper expectedPayloadCountFromSources to locate the check in
smoke-all-script.test.ts.
---
Outside diff comments:
In `@src/main/services/YtDlp.ts`:
- Around line 556-573: prepare() assigns _ytDlpPath/_ffmpegPath (and calls
ensureFFprobe) before probing Electron Node runtime, so a probe failure leaves
the instance in a partially-prepared state; change prepare() to delay assigning
this._ytDlpPath and this._ffmpegPath (and any state changes like
this._jsRuntime) until after probeElectronNodeRuntime() succeeds, or if you
prefer to keep the current call order, ensure you rollback/clear those fields on
probe failure (e.g., set this._ytDlpPath = undefined; this._ffmpegPath =
undefined) before throwing; update the prepare() implementation (the method
named prepare, and use of probeElectronNodeRuntime and
this.binaryManager.ensureYtDlp/ensureFFmpeg/ensureFFprobe) accordingly so
subsequent calls do not skip initialization due to leftover state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8bb3ecc8-16d0-4d3c-bb0f-04674ae2e44c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lockand included by*,**/*
📒 Files selected for processing (78)
.github/workflows/e2e-cold-start.yml.github/workflows/installer-smoke.yml.github/workflows/release.yml.github/workflows/runtime-binaries.ymlAGENTS.mdelectron-builder.json5package.jsonscripts/build/denoResolver.tsscripts/build/runtimeBinaryManifest.tsscripts/smoke-playlist-presets.tsscripts/test-binaries/smoke-all.shscripts/with-runtime-manifest.shskills-lock.jsonsrc/main/e2eHarness.tssrc/main/index.tssrc/main/ipc/registerIpcHandlers.tssrc/main/runtimeSmoke.tssrc/main/services/BinaryManager.tssrc/main/services/WarmupService.tssrc/main/services/YtDlp.tssrc/main/services/binary/BinaryDownloader.tssrc/main/services/binary/BundledRuntimeBinaryIndex.tssrc/main/services/binary/DenoBinarySource.tssrc/main/services/binary/ManagedSetup.tssrc/main/services/binary/ManagedSourcePlan.tssrc/main/services/binary/RuntimeBinaryIndexService.tssrc/main/services/binary/RuntimeBinaryMaterializer.tssrc/main/services/binary/RuntimeBinaryTrust.tssrc/main/services/binary/YtDlpBinarySource.tssrc/main/services/binary/ZippedBinaryInstaller.tssrc/main/services/ytDlpJsRuntime.tssrc/main/utils/process.tssrc/renderer/src/browserMock.tssrc/renderer/src/components/system/RepairPanel.tsxsrc/renderer/src/components/system/WarmupSplash.tsxsrc/renderer/src/dev/browserMockScenarios.tssrc/renderer/src/dev/scenarios/diagnosticScenarios.tssrc/renderer/src/store/systemSlice.tssrc/shared/dependencyPolicy.tssrc/shared/runtimeBinaryManifest.tssrc/shared/schemas.tssrc/shared/types.tstests/e2e/cold-start.spec.tstests/integration/download-service.test.tstests/renderer/warmup-splash.test.tsxtests/shared/mockAppApi.tstests/unit/binary-downloader.test.tstests/unit/binary-manager-analytics.test.tstests/unit/binary-manager-classify.test.tstests/unit/binary-manager-platform.test.tstests/unit/binary-manager-retry.test.tstests/unit/binary-manager.test.tstests/unit/browser-mock-scenarios.test.tstests/unit/deno-binary-source.test.tstests/unit/dependency-policy.test.tstests/unit/download-service-cancel.test.tstests/unit/download-service-crash.test.tstests/unit/download-service-disk-full.test.tstests/unit/download-service-subs.test.tstests/unit/e2e-harness.test.tstests/unit/electron-vite-config.test.tstests/unit/ipc-handlers.test.tstests/unit/managed-source-plan.test.tstests/unit/probe-service.test.tstests/unit/release-asset-names.test.tstests/unit/runtime-binary-index-service.test.tstests/unit/runtime-binary-manifest-generator-parallel.test.tstests/unit/runtime-binary-manifest-generator.test.tstests/unit/runtime-binary-manifest.test.tstests/unit/runtime-binary-materializer.test.tstests/unit/runtime-smoke.test.tstests/unit/smoke-all-script.test.tstests/unit/warmup-service.test.tstests/unit/ytdlp-args-paths.test.tstests/unit/ytdlp-args.test.tstests/unit/ytdlp-js-runtime.test.tstests/unit/ytdlp-retry.test.tstests/unit/zipped-binary-installer.test.ts
💤 Files with no reviewable changes (10)
- src/main/services/binary/ZippedBinaryInstaller.ts
- tests/unit/deno-binary-source.test.ts
- tests/unit/zipped-binary-installer.test.ts
- src/main/services/binary/ManagedSourcePlan.ts
- tests/unit/managed-source-plan.test.ts
- scripts/build/denoResolver.ts
- tests/unit/e2e-harness.test.ts
- tests/unit/binary-manager-classify.test.ts
- src/main/services/binary/DenoBinarySource.ts
- src/main/services/binary/ManagedSetup.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 1x install/uninstall cycle
- GitHub Check: Cold-state install
- GitHub Check: Cold start (windows)
🧰 Additional context used
📓 Path-based instructions (23)
electron-builder.json5
📄 CodeRabbit inference engine (AGENTS.md)
electron-builder configuration: do not use custom NSIS installer.nsh; pin electron-builder ≥ 26.9.0 (26.8.x has buffer over-read); any custom NSIS Page custom callback must open with ${If} ${Silent} \n Abort \n ${EndIf}; npm latest dist-tag 26.8.1 is broken; pin from next channel
Files:
electron-builder.json5
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enforce strict typing throughout the codebase; favor strict typing, exhaustive checks, and discriminated unions;
anyandunknownare escape hatches and must be justified when used
Files:
tests/unit/download-service-crash.test.tssrc/renderer/src/store/systemSlice.tssrc/main/services/binary/BundledRuntimeBinaryIndex.tstests/e2e/cold-start.spec.tssrc/renderer/src/components/system/WarmupSplash.tsxsrc/main/services/binary/RuntimeBinaryTrust.tstests/renderer/warmup-splash.test.tsxtests/unit/download-service-subs.test.tstests/unit/ytdlp-retry.test.tstests/unit/download-service-disk-full.test.tssrc/main/utils/process.tssrc/main/services/binary/RuntimeBinaryIndexService.tstests/unit/download-service-cancel.test.tstests/unit/electron-vite-config.test.tssrc/renderer/src/components/system/RepairPanel.tsxtests/unit/runtime-binary-manifest.test.tstests/unit/dependency-policy.test.tstests/unit/runtime-binary-manifest-generator-parallel.test.tssrc/main/services/ytDlpJsRuntime.tstests/unit/binary-manager.test.tstests/unit/browser-mock-scenarios.test.tstests/integration/download-service.test.tstests/shared/mockAppApi.tssrc/main/e2eHarness.tstests/unit/runtime-smoke.test.tstests/unit/runtime-binary-manifest-generator.test.tstests/unit/binary-downloader.test.tssrc/shared/dependencyPolicy.tstests/unit/ipc-handlers.test.tssrc/shared/runtimeBinaryManifest.tstests/unit/release-asset-names.test.tssrc/shared/schemas.tssrc/renderer/src/browserMock.tstests/unit/smoke-all-script.test.tssrc/main/index.tstests/unit/probe-service.test.tstests/unit/runtime-binary-materializer.test.tssrc/shared/types.tssrc/main/ipc/registerIpcHandlers.tssrc/main/runtimeSmoke.tstests/unit/binary-manager-platform.test.tsscripts/smoke-playlist-presets.tstests/unit/ytdlp-args-paths.test.tstests/unit/ytdlp-args.test.tssrc/main/services/YtDlp.tssrc/main/services/WarmupService.tstests/unit/warmup-service.test.tstests/unit/runtime-binary-index-service.test.tstests/unit/binary-manager-analytics.test.tstests/unit/ytdlp-js-runtime.test.tsscripts/build/runtimeBinaryManifest.tssrc/main/services/binary/RuntimeBinaryMaterializer.tssrc/renderer/src/dev/scenarios/diagnosticScenarios.tstests/unit/binary-manager-retry.test.tssrc/main/services/binary/BinaryDownloader.tssrc/main/services/binary/YtDlpBinarySource.tssrc/renderer/src/dev/browserMockScenarios.tssrc/main/services/BinaryManager.ts
tests/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When adding idempotent IPC registration (ipcMain.removeHandler, autoUpdater.removeAllListeners), add the method as vi.fn() to the matching vi.mock blocks in tests
Files:
tests/unit/download-service-crash.test.tstests/renderer/warmup-splash.test.tsxtests/unit/download-service-subs.test.tstests/unit/ytdlp-retry.test.tstests/unit/download-service-disk-full.test.tstests/unit/download-service-cancel.test.tstests/unit/electron-vite-config.test.tstests/unit/runtime-binary-manifest.test.tstests/unit/dependency-policy.test.tstests/unit/runtime-binary-manifest-generator-parallel.test.tstests/unit/binary-manager.test.tstests/unit/browser-mock-scenarios.test.tstests/integration/download-service.test.tstests/unit/runtime-smoke.test.tstests/unit/runtime-binary-manifest-generator.test.tstests/unit/binary-downloader.test.tstests/unit/ipc-handlers.test.tstests/unit/release-asset-names.test.tstests/unit/smoke-all-script.test.tstests/unit/probe-service.test.tstests/unit/runtime-binary-materializer.test.tstests/unit/binary-manager-platform.test.tstests/unit/ytdlp-args-paths.test.tstests/unit/ytdlp-args.test.tstests/unit/warmup-service.test.tstests/unit/runtime-binary-index-service.test.tstests/unit/binary-manager-analytics.test.tstests/unit/ytdlp-js-runtime.test.tstests/unit/binary-manager-retry.test.ts
tests/**
⚙️ CodeRabbit configuration file
Prioritize meaningful coverage, determinism, and whether tests validate behavior. Do not nitpick formatting or implementation style unless it makes the test unreliable.
Files:
tests/unit/download-service-crash.test.tstests/e2e/cold-start.spec.tstests/renderer/warmup-splash.test.tsxtests/unit/download-service-subs.test.tstests/unit/ytdlp-retry.test.tstests/unit/download-service-disk-full.test.tstests/unit/download-service-cancel.test.tstests/unit/electron-vite-config.test.tstests/unit/runtime-binary-manifest.test.tstests/unit/dependency-policy.test.tstests/unit/runtime-binary-manifest-generator-parallel.test.tstests/unit/binary-manager.test.tstests/unit/browser-mock-scenarios.test.tstests/integration/download-service.test.tstests/shared/mockAppApi.tstests/unit/runtime-smoke.test.tstests/unit/runtime-binary-manifest-generator.test.tstests/unit/binary-downloader.test.tstests/unit/ipc-handlers.test.tstests/unit/release-asset-names.test.tstests/unit/smoke-all-script.test.tstests/unit/probe-service.test.tstests/unit/runtime-binary-materializer.test.tstests/unit/binary-manager-platform.test.tstests/unit/ytdlp-args-paths.test.tstests/unit/ytdlp-args.test.tstests/unit/warmup-service.test.tstests/unit/runtime-binary-index-service.test.tstests/unit/binary-manager-analytics.test.tstests/unit/ytdlp-js-runtime.test.tstests/unit/binary-manager-retry.test.ts
src/renderer/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx}: Reuse installed shadcn/ui components (Field, InputGroup, ButtonGroup, ToggleGroup, Card, Alert, Empty, Separator, Badge, etc.) via composition; create custom primitives only when shadcn has no fitting component or the component encodes Arroxy-specific product behavior
Do not add Radix UI dependencies; this project does not use@radix-ui/* — the base-nova registry doesn't depend on Radix primitives; install new shadcn components vianpx shadcn@latest add <component>
Files:
src/renderer/src/store/systemSlice.tssrc/renderer/src/components/system/WarmupSplash.tsxsrc/renderer/src/components/system/RepairPanel.tsxsrc/renderer/src/browserMock.tssrc/renderer/src/dev/scenarios/diagnosticScenarios.tssrc/renderer/src/dev/browserMockScenarios.ts
src/renderer/src/**/*.{css,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Poppins font (weights 400/500/600/700 from
@fontsource/poppins) exclusively; do not reintroduce Outfit or Geist
Files:
src/renderer/src/store/systemSlice.tssrc/renderer/src/components/system/WarmupSplash.tsxsrc/renderer/src/components/system/RepairPanel.tsxsrc/renderer/src/browserMock.tssrc/renderer/src/dev/scenarios/diagnosticScenarios.tssrc/renderer/src/dev/browserMockScenarios.ts
src/{shared,main,renderer}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
New shared modules go under src/shared/ so renderer + main both import without IPC; modules with electron/node-only deps stay in src/main/services/ (or src/renderer/...) and surface their seam via re-exports of pure helpers
Files:
src/renderer/src/store/systemSlice.tssrc/main/services/binary/BundledRuntimeBinaryIndex.tssrc/main/services/binary/RuntimeBinaryTrust.tssrc/main/utils/process.tssrc/main/services/binary/RuntimeBinaryIndexService.tssrc/main/services/ytDlpJsRuntime.tssrc/main/e2eHarness.tssrc/shared/dependencyPolicy.tssrc/shared/runtimeBinaryManifest.tssrc/shared/schemas.tssrc/renderer/src/browserMock.tssrc/main/index.tssrc/shared/types.tssrc/main/ipc/registerIpcHandlers.tssrc/main/runtimeSmoke.tssrc/main/services/YtDlp.tssrc/main/services/WarmupService.tssrc/main/services/binary/RuntimeBinaryMaterializer.tssrc/renderer/src/dev/scenarios/diagnosticScenarios.tssrc/main/services/binary/BinaryDownloader.tssrc/main/services/binary/YtDlpBinarySource.tssrc/renderer/src/dev/browserMockScenarios.tssrc/main/services/BinaryManager.ts
src/renderer/**
⚙️ CodeRabbit configuration file
Review React/UI code for state correctness, accessibility, i18n regressions, unsafe DOM usage, IPC misuse, race conditions, and user-facing error handling. Avoid style-only nitpicks unless they affect maintainability.
Files:
src/renderer/src/store/systemSlice.tssrc/renderer/src/components/system/WarmupSplash.tsxsrc/renderer/src/components/system/RepairPanel.tsxsrc/renderer/src/browserMock.tssrc/renderer/src/dev/scenarios/diagnosticScenarios.tssrc/renderer/src/dev/browserMockScenarios.ts
src/main/services/binary/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Runtime: BinaryManager.getFfmpegPath() returns process.resourcesPath/ffmpeg(.exe) in prod, build/embedded//ffmpeg(.exe) in dev; spawnYtDlp + spawnFFmpeg + probeBinary inject LD_LIBRARY_PATH=
at spawn time on Linux only
Files:
src/main/services/binary/BundledRuntimeBinaryIndex.tssrc/main/services/binary/RuntimeBinaryTrust.tssrc/main/services/binary/RuntimeBinaryIndexService.tssrc/main/services/binary/RuntimeBinaryMaterializer.tssrc/main/services/binary/BinaryDownloader.tssrc/main/services/binary/YtDlpBinarySource.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
electron-updater v6, GitHub provider: auto-download disabled; main calls autoUpdater.checkForUpdates() 5s after launch, emits updater:available to renderer with { version, currentVersion, installChannel }; installChannel detection (flatpak/scoop/homebrew/portable/direct) lives in src/main/installChannel.ts
Files:
src/main/services/binary/BundledRuntimeBinaryIndex.tssrc/main/services/binary/RuntimeBinaryTrust.tssrc/main/utils/process.tssrc/main/services/binary/RuntimeBinaryIndexService.tssrc/main/services/ytDlpJsRuntime.tssrc/main/e2eHarness.tssrc/main/index.tssrc/main/ipc/registerIpcHandlers.tssrc/main/runtimeSmoke.tssrc/main/services/YtDlp.tssrc/main/services/WarmupService.tssrc/main/services/binary/RuntimeBinaryMaterializer.tssrc/main/services/binary/BinaryDownloader.tssrc/main/services/binary/YtDlpBinarySource.tssrc/main/services/BinaryManager.ts
src/main/**
⚙️ CodeRabbit configuration file
This is Electron main-process code. Prioritize security, IPC boundaries, filesystem access, updater/download behavior, child process usage, path handling, and validation of renderer-provided input.
Files:
src/main/services/binary/BundledRuntimeBinaryIndex.tssrc/main/services/binary/RuntimeBinaryTrust.tssrc/main/utils/process.tssrc/main/services/binary/RuntimeBinaryIndexService.tssrc/main/services/ytDlpJsRuntime.tssrc/main/e2eHarness.tssrc/main/index.tssrc/main/ipc/registerIpcHandlers.tssrc/main/runtimeSmoke.tssrc/main/services/YtDlp.tssrc/main/services/WarmupService.tssrc/main/services/binary/RuntimeBinaryMaterializer.tssrc/main/services/binary/BinaryDownloader.tssrc/main/services/binary/YtDlpBinarySource.tssrc/main/services/BinaryManager.ts
src/renderer/src/**/*.{css,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{css,tsx}: Use CSS tokens from src/renderer/src/styles.css: --border-strong (darker border), --text-subtle (muted label), dark mode --border (hsla(0,0%,100%,0.07)); apply glow patterns: primary buttons shadow-[0_4px_14px_var(--brand-glow)], active radio shadow-[0_0_0_2px_var(--brand-dim)], section labels text-[9px] font-bold uppercase tracking-[0.12em] text-[var(--text-subtle)], back/ghost buttons border-[1.5px] border-[var(--border-strong)]
Use Tailwind v4 default mobile-first breakpoints (sm 640px, md 768px, lg 1024px, xl 1280px, 2xl 1536px); do not add custom Tailwind breakpoint overrides
Files:
src/renderer/src/components/system/WarmupSplash.tsxsrc/renderer/src/components/system/RepairPanel.tsx
scripts/**
⚙️ CodeRabbit configuration file
Review shell and Node/Bun scripts for unsafe command construction, unquoted variables, path traversal, platform-specific assumptions, missing error handling, and release/build reproducibility.
Files:
scripts/with-runtime-manifest.shscripts/smoke-playlist-presets.tsscripts/build/runtimeBinaryManifest.tsscripts/test-binaries/smoke-all.sh
.github/workflows/**
⚙️ CodeRabbit configuration file
Review GitHub Actions for least-privilege permissions, unsafe secret exposure, untrusted PR execution, shell quoting, release/tag correctness, artifact handling, and whether commands can fail silently.
Files:
.github/workflows/installer-smoke.yml.github/workflows/release.yml.github/workflows/runtime-binaries.yml.github/workflows/e2e-cold-start.yml
src/shared/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Store pure helpers (no I/O) in their own module with a fixture-driven test alongside; pattern: tests/fixtures/yt-dlp-stderr//*.txt + tests/unit/yt-dlp-errors.test.ts
Files:
src/shared/dependencyPolicy.tssrc/shared/runtimeBinaryManifest.tssrc/shared/schemas.tssrc/shared/types.ts
src/shared/dependencyPolicy.ts
📄 CodeRabbit inference engine (AGENTS.md)
DependencyRequirementPolicy computes blocking failures from dependency diagnostics with explicit harness exceptions (skipDeno), keeping full DEPENDENCY_IDS diagnostic set intact
Files:
src/shared/dependencyPolicy.ts
src/shared/schemas.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/shared/schemas.ts: Usez.enum([...])→type Foo = z.infer<…>→const FOOS = fooSchema.optionspattern in schemas.ts; all enum types must be derived from Zod schemas, not redeclared as TS union literals
SUPPORTED_LANGS in src/shared/schemas.ts is the canonical locale list for renderer + main process i18n; LOCALES in readme-src/strings.mjs is the canonical locale list for README generation; both must stay in lockstep with landing-site locale list in arroxy-web repo
QueueStatus: 7-value union (pending, running, paused-held, paused-active, done, error, cancelled); paused-held = in queue never spawned (resume → pending); paused-active = had running job, user paused (resume → re-spawn, possibly across app restart via persisted tempDir + lastJobId)
Files:
src/shared/schemas.ts
src/shared/{schemas,constants}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Store
QUEUE_STATUSandSTATUS_KEYenum constants in schemas.ts (not constants.ts); storeDEFAULTSin constants.ts
Files:
src/shared/schemas.ts
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
AGENTS.md: Document agent implementations in AGENTS.md with clear descriptions of purpose, capabilities, and configuration
Maintain up-to-date agent documentation in AGENTS.md whenever agents are added, modified, or removed
Files:
AGENTS.md
src/{shared,main}/types.ts
📄 CodeRabbit inference engine (AGENTS.md)
Migrate LocalizedError from old { key, rawMessage? } shape; always populate both kind and raw; 'unknown' covers unmatched-stderr fallback
Files:
src/shared/types.ts
src/shared/types.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/shared/types.ts: Each Diagnostic carries a stable FAILURE_CODE (ARX-NNN) so users can search the repair UI codes language-independently
Job: user-initiated download identified by DownloadJob.id (UUID); lifecycle: created in DownloadService.start, runs through Phases, finalized via JobLifecycle.finalize
Files:
src/shared/types.ts
scripts/build/runtimeBinaryManifest.ts
📄 CodeRabbit inference engine (AGENTS.md)
RuntimeBinaryManifest automation: CI/build helper resolves upstream releases, writes concrete URLs + size/SHA-256, validates entries through RuntimeBinaryMaterializer, smoke-probes host-compatible entries, signs runtime-index-v1.json, publishes to fixed runtime manifest release; local dev can run
bun run dev:runtime-manifest
Files:
scripts/build/runtimeBinaryManifest.ts
src/main/services/binary/YtDlpBinarySource.ts
📄 CodeRabbit inference engine (AGENTS.md)
YtDlp binary-source module exposes: platform asset names, concrete GitHub nightly/stable URL construction, SourceForge stable mirror URL construction, and SourceForge latest-version parsing for CI manifest automation
Files:
src/main/services/binary/YtDlpBinarySource.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Keep concerns separated and dependencies pointing inward; ensure business logic is decoupled from frameworks and I/O (clean architecture principle)
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Prefer mature, production-proven libraries over bespoke code for validation, HTTP, ORM, auth, logging, and similar plumbing
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Push back when a proposal looks incorrect, incomplete, or inconsistent with the codebase; name the specific reason rather than defaulting to agreement
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Verify facts before acting on uncertainty; when not confident about an API, library behavior, or codebase functionality, check the source, docs, or run a test
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Reuse the domain glossary terms exactly when extending or refactoring; add new entries when extracting modules instead of letting parallel vocabularies sprout
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: When encountering the same error 3 times, research the web and GitHub to find 3-5 possible fixes, then implement the most efficient solution or notify the user if no clean fix exists
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Do not prioritize migration paths, backward compatibility shims, or preserving existing implementations; delete old code, types, and state fields without ceremony or deprecation notices
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Write failing tests first for non-trivial changes, implement minimally, then refactor (TDD); skip only for typo fixes and single-line edits
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Use the translate-arroxy-i18n project skill for app locale changes, PO/POT sync, generated locale JSON, and i18n audits
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Treat every agent session as scoped to the task context; do not repair, reformat, or restructure files outside scope even if you notice problems there
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: State assumptions explicitly; present multiple interpretations rather than picking silently; name what is confusing and ask for clarification
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Define success criteria and verify goals; transform tasks into verifiable steps with tests before and after refactoring
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Store persistent memory in `./.agents/memory/` following the `name`/`description`/`type` frontmatter convention; do not use user-scoped or tool-specific memory locations
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Keep installable third-party skills out of Git; their source directories are local-restored state; recovery metadata lives in skills-lock.json; run `bun run agents:skills:restore` after clone
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: After any implementation task, run `bun run check` (lint + typecheck + knip + madge) and fix all errors caused by the change before reporting done
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Use `bun run test` (vitest), not `bun test` (Bun's built-in runner); for single files use `bunx vitest run --project node <path>` or `--project jsdom <path>`
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Use Fixture Product E2E as the acceptance owner for real user workflows; keep logic tests, renderer tests, and scenario workbench to their owned layers; only use Fixture Product E2E for workflows requiring real Electron, IPC, yt-dlp, filesystem, or realistic interaction
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Do not use unit tests to claim a real paste → metadata → queue → download workflow is fixed; use Fixture Product E2E for acceptance of real workflows
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Use fast logic tests (tests/unit, Vitest) for pure rules and closed interfaces such as QueueEvent transition, ProgressNormalizer, URL parsing, yt-dlp argument construction, and error classification
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Use Scenario Workbench (tests/browser, `bun run dev:mock`) for visual inspection, layout stress, screenshots, and fast review of many UI states
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Group Fixture Product E2E scenarios by risk case (bulk input, metadata fetching, wizard navigation, queue controls, failure/recovery, persistence, scale/UX stress) so failures are local and useful
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Prefer visible UI milestones first in E2E tests; use E2E-only diagnostics only when UI is insufficient; gate diagnostics behind ARROXY_E2E=1; avoid hardcoded sleeps and wait for specific UI/diagnostic/process/filesystem/network milestones
Learnt from: CR
Repo: antonio-orionus/Arroxy
Timestamp: 2026-06-12T19:13:46.031Z
Learning: Use agent-browser for browser automation, screenshots, responsive layout checks, and interactive UI dogfooding; use Playwright only when agent-browser is unavailable or the task specifically needs Playwright traces/tests or Electron _electron.launch()
📚 Learning: 2026-06-05T08:32:42.878Z
Learnt from: antonio-orionus
Repo: antonio-orionus/Arroxy PR: 65
File: scripts/alias-latest-release-assets.sh:1-71
Timestamp: 2026-06-05T08:32:42.878Z
Learning: In this repository, the `rtk` command is a local agent-only proxy meant for interactive AI-agent shell use. Do not use `rtk` in files under `scripts/` (including one-time helpers like migration/release asset aliases). Scripts in `scripts/` must remain portable for maintainers and CI environments that only have standard shell tools plus `gh`; use plain commands/tools (e.g., `gh`, `grep`, `mv`, `rm`, etc.) instead of any `rtk` prefix.
Applied to files:
scripts/with-runtime-manifest.shscripts/test-binaries/smoke-all.sh
🪛 ast-grep (0.43.0)
tests/unit/runtime-binary-materializer.test.ts
[info] 92-95: Make sure your server uses the https protocol
Context: http.createServer((_req, res) => {
res.writeHead(200, {'content-length': String(body.length)})
res.end(body)
})
Note: Security best practice.
(https-protocol-missing-typescript)
🪛 LanguageTool
AGENTS.md
[uncategorized] ~39-~39: The official name of this software platform is spelled with a capital “H”.
Context: ...cripts/build/runtimeBinaryManifest.ts, .github/workflows/runtime-binaries.yml`. - **De...
(GITHUB)
🪛 OpenGrep (1.22.0)
tests/unit/runtime-binary-manifest-generator-parallel.test.ts
[ERROR] 37-37: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
src/main/services/ytDlpJsRuntime.ts
[ERROR] 49-49: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
src/shared/schemas.ts
[WARNING] 224-224: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
🪛 zizmor (1.25.2)
.github/workflows/installer-smoke.yml
[error] 203-203: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
.github/workflows/release.yml
[error] 217-217: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
.github/workflows/runtime-binaries.yml
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 28-28: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 43-43: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 60-60: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 64-64: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 68-68: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 75-75: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 92-92: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[warning] 89-89: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
[info] 16-16: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 51-51: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 84-84: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[warning] 3-6: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🔇 Additional comments (67)
tests/unit/runtime-binary-manifest-generator.test.ts (1)
1-46: LGTM!tests/unit/runtime-binary-manifest.test.ts (1)
1-67: LGTM!tests/unit/runtime-binary-materializer.test.ts (1)
1-223: LGTM!tests/unit/smoke-all-script.test.ts (1)
1-53: LGTM!Also applies to: 57-58
tests/unit/warmup-service.test.ts (1)
1-115: LGTM!tests/unit/runtime-smoke.test.ts (1)
1-79: LGTM!tests/unit/ytdlp-args-paths.test.ts (1)
10-17: LGTM!Also applies to: 30-31, 39-43
tests/unit/ytdlp-args.test.ts (1)
9-9: LGTM!Also applies to: 16-23, 34-40, 46-50, 773-826
tests/unit/ytdlp-js-runtime.test.ts (1)
1-115: LGTM!tests/unit/ytdlp-retry.test.ts (1)
21-21: LGTM!src/renderer/src/browserMock.ts (1)
231-235: LGTM!Also applies to: 238-242
src/shared/types.ts (1)
49-54: LGTM!Also applies to: 326-333
tests/renderer/warmup-splash.test.tsx (1)
49-58: LGTM!tests/shared/mockAppApi.ts (1)
9-9: LGTM!tests/unit/binary-manager-analytics.test.ts (1)
10-12: LGTM!Also applies to: 17-48, 50-67, 83-92
tests/unit/dependency-policy.test.ts (1)
13-13: LGTM!tests/unit/download-service-crash.test.ts (1)
20-20: LGTM!tests/unit/probe-service.test.ts (1)
12-19: LGTM!Also applies to: 34-35, 45-45
src/renderer/src/components/system/RepairPanel.tsx (1)
26-28: LGTM!src/renderer/src/components/system/WarmupSplash.tsx (1)
34-37: LGTM!src/renderer/src/dev/scenarios/diagnosticScenarios.ts (1)
20-22: LGTM!Also applies to: 34-34
src/renderer/src/store/systemSlice.ts (1)
81-81: LGTM!src/shared/runtimeBinaryManifest.ts (1)
1-82: LGTM!src/shared/schemas.ts (1)
192-225: LGTM!Also applies to: 402-403
src/renderer/src/dev/browserMockScenarios.ts (1)
173-174: LGTM!tests/e2e/cold-start.spec.ts (1)
16-16: LGTM!Also applies to: 86-87
tests/unit/download-service-cancel.test.ts (1)
22-22: LGTM!tests/unit/download-service-subs.test.ts (1)
64-64: LGTM!tests/unit/electron-vite-config.test.ts (1)
14-15: LGTM!tests/unit/runtime-binary-manifest-generator-parallel.test.ts (1)
1-100: LGTM!tests/integration/download-service.test.ts (1)
13-13: LGTM!Also applies to: 231-231
tests/unit/binary-downloader.test.ts (1)
5-8: LGTM!Also applies to: 27-33, 66-290
tests/unit/binary-manager-platform.test.ts (1)
4-4: LGTM!Also applies to: 20-22, 59-63
tests/unit/binary-manager.test.ts (1)
60-61: LGTM!tests/unit/download-service-disk-full.test.ts (1)
59-59: LGTM!tests/unit/ipc-handlers.test.ts (1)
63-63: LGTM!tests/unit/release-asset-names.test.ts (1)
25-31: LGTM!Also applies to: 33-44, 46-56, 58-67
tests/unit/runtime-binary-index-service.test.ts (1)
1-129: LGTM!.github/workflows/e2e-cold-start.yml (1)
85-145: LGTM!AGENTS.md (1)
36-39: LGTM!Also applies to: 42-43, 530-530
scripts/with-runtime-manifest.sh (1)
1-33: LGTM!src/main/services/YtDlp.ts (1)
10-10: LGTM!Also applies to: 19-19, 26-35, 163-163, 226-228, 240-240, 246-246, 248-248, 544-544, 563-569, 596-596
src/main/services/ytDlpJsRuntime.ts (1)
1-84: LGTM!src/main/utils/process.ts (1)
21-23: LGTM!.github/workflows/installer-smoke.yml (1)
45-54: LGTM!Also applies to: 86-94, 114-158
.github/workflows/release.yml (1)
99-100: LGTM!Also applies to: 124-214
scripts/build/runtimeBinaryManifest.ts (1)
1-380: LGTM!src/main/services/WarmupService.ts (1)
13-26: LGTM!Also applies to: 28-33, 133-154
src/main/services/binary/RuntimeBinaryMaterializer.ts (1)
1-77: LGTM!Also applies to: 78-161, 163-250, 252-298, 312-434
src/main/services/binary/RuntimeBinaryTrust.ts (1)
1-7: LGTM!src/main/services/binary/YtDlpBinarySource.ts (1)
1-52: LGTM!scripts/smoke-playlist-presets.ts (1)
13-13: LGTM!Also applies to: 37-37, 75-75, 80-80, 100-124, 166-172, 174-176, 178-192, 307-307, 313-313, 318-318, 326-326
skills-lock.json (1)
20-20: LGTM!Also applies to: 38-38
src/main/index.ts (1)
30-30: LGTM!Also applies to: 186-186, 197-204, 328-328, 394-394
src/main/ipc/registerIpcHandlers.ts (1)
43-45: LGTM!src/main/runtimeSmoke.ts (4)
89-120: LGTM!
122-174: LGTM!
181-204: LGTM!
206-306: LGTM!src/main/services/BinaryManager.ts (6)
8-15: LGTM!
30-32: LGTM!Also applies to: 62-92
124-145: LGTM!
207-253: LGTM!
255-300: LGTM!
302-352: Robust cache validation with proper security checks.The managed artifact cache validation includes comprehensive security measures: path containment checks (line 344), SHA-256 verification (line 346), and executable permission validation (line 347). This prevents path traversal and ensures cache integrity.
scripts/test-binaries/smoke-all.sh (1)
67-73: LGTM!src/main/e2eHarness.ts (1)
66-66: LGTM!
| - name: Upload runtime smoke logs | ||
| if: always() | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: runtime-smoke-logs | ||
| path: | | ||
| ${{ runner.temp }}/runtime-smoke-logs | ||
| ${{ runner.temp }}/Arroxy Runtime Ω Installed/logs | ||
| ${{ runner.temp }}/Arroxy Runtime Ω Portable/logs | ||
| if-no-files-found: ignore | ||
| retention-days: 7 |
There was a problem hiding this comment.
Pin actions/upload-artifact to a commit hash in .github/workflows/installer-smoke.yml and .github/workflows/release.yml.
Both workflow files use actions/upload-artifact@v7 without a commit hash pin. Pinning to a specific commit hash ensures reproducible builds and mitigates supply-chain risks if the tag is moved maliciously. The same pinned reference should be used in both files.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 203-203: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/installer-smoke.yml around lines 201 - 211, Replace the
floating tag actions/upload-artifact@v7 with a pinned commit SHA in every
workflow that references it (search for the exact string
actions/upload-artifact@v7), fetching the commit hash that corresponds to the v7
release from the upstream repo and replacing the tag usage with
actions/upload-artifact@<COMMIT_SHA>; ensure you use the same pinned commit SHA
in both workflow files that previously used `@v7` so both workflows reference the
identical immutable ref.
Source: Linters/SAST tools
| on: | ||
| workflow_dispatch: | ||
| schedule: | ||
| - cron: '17 7 * * *' |
There was a problem hiding this comment.
Add concurrency limits to prevent overlapping runs.
The workflow lacks a concurrency setting. Concurrent runs from cron + manual dispatch can conflict during publish, potentially causing race conditions when uploading release assets.
Proposed fix
on:
workflow_dispatch:
schedule:
- cron: '17 7 * * *'
+
+concurrency:
+ group: runtime-binaries
+ cancel-in-progress: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on: | |
| workflow_dispatch: | |
| schedule: | |
| - cron: '17 7 * * *' | |
| on: | |
| workflow_dispatch: | |
| schedule: | |
| - cron: '17 7 * * *' | |
| concurrency: | |
| group: runtime-binaries | |
| cancel-in-progress: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 3-6: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/runtime-binaries.yml around lines 3 - 6, Add a top-level
concurrency key to the workflow to prevent overlapping runs: under the root of
the YAML (near the existing on: block) add a concurrency stanza that defines a
unique group name (e.g., using the workflow or repo name and optionally matrix
or github.ref) and set cancel-in-progress to true so a new manual or scheduled
run cancels any in-progress run. Ensure the concurrency block is added at the
same indentation level as on:, jobs:, etc., and reference the workflow identity
(workflow name or github.workflow) in the group to ensure correct grouping.
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Pin actions to full commit SHAs for supply-chain security.
All action references use version tags (@v6, @v4, @v2.2.0) rather than commit hashes. A compromised upstream action tag could inject malicious code into your CI. Pin to full SHAs for actions that handle secrets or publish releases.
Also applies to: 24-24, 28-28, 43-43, 60-60, 64-64, 68-68, 75-75, 92-92
🧰 Tools
🪛 zizmor (1.25.2)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/runtime-binaries.yml at line 20, Replace all GitHub Action
version tags with pinned full commit SHAs: find every usage like
actions/checkout@v6 (and other tagged refs noted: lines referencing actions at
24, 28, 43, 60, 64, 68, 75, 92) and change each to the corresponding commit SHA
for that action release; update workflow YAML anchors that reference
docker/setup-buildx-action, actions/setup-node, actions/cache, etc., to use the
exact commit SHA found on the action's GitHub releases/tags page, and verify the
SHA resolves and the workflow still runs (optionally add a comment with the
tag->SHA mapping for future updates).
Source: Linters/SAST tools
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document the elevated permissions.
The contents: write permission enables creating releases and uploading assets. Add a brief comment explaining why this scope is required to satisfy the audit trail expectation.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 89-89: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/runtime-binaries.yml around lines 88 - 89, Add a brief
inline comment next to the permissions: contents: write setting explaining that
this elevated scope is required to create GitHub releases and upload build
artifacts (release assets) so the workflow can publish runtime binaries and
maintain an audit trail; update the workflow entry where "permissions: contents:
write" is defined to include that explanatory comment.
Source: Linters/SAST tools
| - **Deno binary-source module** — Deno target facts and checksum helpers: supported target triples, asset/executable names, latest-version validation, checksum parsing, dl.deno.land primary URL construction, concrete GitHub release mirror URLs, and shell-env formatting for smoke scripts. Deno is required but not embedded in packaged resources. `src/main/services/binary/DenoBinarySource.ts`, `scripts/build/denoResolver.ts`. | ||
| - **YtDlp binary-source module** — yt-dlp target/source facts: platform asset names, concrete GitHub nightly/stable URL construction, SourceForge stable mirror URL construction, and SourceForge latest-version parsing for CI manifest automation. `src/main/services/binary/YtDlpBinarySource.ts`. |
There was a problem hiding this comment.
Remove or reframe the Deno glossary entry to match the runtime migration.
Line 40 still documents Deno as an active required runtime path, but this PR’s stated objective removes Deno from runtime/dependency flows. Keep the glossary aligned with the new source-of-truth architecture to avoid drift.
📝 Suggested doc fix
-- **Deno binary-source module** — Deno target facts and checksum helpers: supported target triples, asset/executable names, latest-version validation, checksum parsing, dl.deno.land primary URL construction, concrete GitHub release mirror URLs, and shell-env formatting for smoke scripts. Deno is required but not embedded in packaged resources. `src/main/services/binary/DenoBinarySource.ts`, `scripts/build/denoResolver.ts`.As per coding guidelines, “Maintain up-to-date agent documentation in AGENTS.md whenever agents are added, modified, or removed.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Deno binary-source module** — Deno target facts and checksum helpers: supported target triples, asset/executable names, latest-version validation, checksum parsing, dl.deno.land primary URL construction, concrete GitHub release mirror URLs, and shell-env formatting for smoke scripts. Deno is required but not embedded in packaged resources. `src/main/services/binary/DenoBinarySource.ts`, `scripts/build/denoResolver.ts`. | |
| - **YtDlp binary-source module** — yt-dlp target/source facts: platform asset names, concrete GitHub nightly/stable URL construction, SourceForge stable mirror URL construction, and SourceForge latest-version parsing for CI manifest automation. `src/main/services/binary/YtDlpBinarySource.ts`. | |
| - **YtDlp binary-source module** — yt-dlp target/source facts: platform asset names, concrete GitHub nightly/stable URL construction, SourceForge stable mirror URL construction, and SourceForge latest-version parsing for CI manifest automation. `src/main/services/binary/YtDlpBinarySource.ts`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 40 - 41, Update the Deno glossary entry in AGENTS.md
to reflect that Deno is no longer an active runtime/dependency: remove or
rephrase the sentence stating "Deno is required but not embedded in packaged
resources" and instead mark Deno as removed from runtime flows or as a
historical/reference item; adjust the description to point readers to the new
source-of-truth architecture and relevant implementation files
(src/main/services/binary/DenoBinarySource.ts and scripts/build/denoResolver.ts)
if the entry is kept for archival or tooling reference. Ensure the entry no
longer implies Deno is part of the runtime dependency chain.
Source: Coding guidelines
| private async existingInstall(entry: RuntimeBinaryManifestEntry, cacheKey: string, metadataPath: string, executablePath: string): Promise<boolean> { | ||
| try { | ||
| const [metadataRaw, stat] = await Promise.all([fsPromises.readFile(metadataPath, 'utf8'), fsPromises.lstat(executablePath)]) | ||
| const metadata = JSON.parse(metadataRaw) as Partial<RuntimeBinaryInstallMetadata> | ||
| if (metadata.cacheKey !== cacheKey || metadata.manifestHash !== runtimeBinaryManifestHash(entry) || metadata.executablePath !== normalizeRuntimeExecutablePath(entry.executablePath)) return false | ||
| if (!stat.isFile() || stat.size !== entry.size) return false | ||
| if (process.platform !== 'win32') await fsPromises.access(executablePath, fs.constants.X_OK) | ||
| return (await sha256ForFile(executablePath)) === entry.sha256 | ||
| } catch { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
existingInstall check may fail for zip format entries.
The validation at lines 304 and 306 compares the extracted executable's size and SHA-256 against entry.size and entry.sha256, which represent the downloaded archive's size and checksum—not the extracted file's. This works correctly for raw format (where the download is the executable), but for zip format, the extracted file's size and hash will differ from the archive's, causing valid cached installs to be re-downloaded on every call.
Consider storing and checking the extracted executable's size/hash in the install metadata instead of comparing against the archive manifest values.
💡 Suggested approach
Store the actual executable size and hash in RuntimeBinaryInstallMetadata:
export interface RuntimeBinaryInstallMetadata {
cacheKey: string
manifest: RuntimeBinaryManifestEntry
manifestHash: string
executablePath: string
installedAt: string
+ executableSize: number
+ executableSha256: string
}Then update existingInstall to check against these stored values instead of entry.size/entry.sha256.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/binary/RuntimeBinaryMaterializer.ts` around lines 299 -
310, The existingInstall check incorrectly compares extracted executable
size/hash against archive values for zip entries; update the install metadata
(RuntimeBinaryInstallMetadata) to include executableSize and executableSha256
(or similar names), ensure the code that writes the metadata after extraction
populates these fields, and then change existingInstall (method existingInstall
in RuntimeBinaryMaterializer) to validate metadata.executableSize and
metadata.executableSha256 (and keep checking metadata.cacheKey, manifestHash and
executablePath) instead of comparing to entry.size/entry.sha256 so zip-extracted
binaries use the stored extracted-file values for size/hash verification.
| const RUNTIME_REQUIRED_DEPENDENCY_IDS: readonly DependencyId[] = ['yt-dlp', 'ffmpeg', 'ffprobe'] as const | ||
|
|
||
| export interface DependencyRequirementContext { | ||
| skipDeno?: boolean | ||
| } | ||
|
|
||
| function requiredDependencyIds(ctx: DependencyRequirementContext = {}): readonly DependencyId[] { | ||
| return ctx.skipDeno ? RUNTIME_REQUIRED_DEPENDENCY_IDS.filter(id => id !== 'deno') : RUNTIME_REQUIRED_DEPENDENCY_IDS | ||
| } | ||
|
|
||
| export function blockingDependencyFailures(dependencies: Partial<Record<DependencyId, DependencyDiagnostic>>, ctx: DependencyRequirementContext = {}): DependencyId[] { | ||
| return requiredDependencyIds(ctx).filter(id => dependencies[id]?.state !== 'runnable') | ||
| export function blockingDependencyFailures(dependencies: Partial<Record<DependencyId, DependencyDiagnostic>>): DependencyId[] { | ||
| return RUNTIME_REQUIRED_DEPENDENCY_IDS.filter(id => dependencies[id]?.state !== 'runnable') |
There was a problem hiding this comment.
Restore the explicit harness-exception contract in dependency blocking.
This change drops the skipDeno context contract and hard-codes the required set, which breaks the file-specific dependency policy rule.
Suggested patch
+export interface DependencyRequirementContext {
+ skipDeno?: boolean
+}
+
-const RUNTIME_REQUIRED_DEPENDENCY_IDS: readonly DependencyId[] = ['yt-dlp', 'ffmpeg', 'ffprobe'] as const
+const RUNTIME_REQUIRED_DEPENDENCY_IDS: readonly DependencyId[] = ['yt-dlp', 'ffmpeg', 'ffprobe', 'deno'] as const
+
+function requiredDependencyIds(ctx: DependencyRequirementContext): readonly DependencyId[] {
+ return ctx.skipDeno ? RUNTIME_REQUIRED_DEPENDENCY_IDS.filter(id => id !== 'deno') : RUNTIME_REQUIRED_DEPENDENCY_IDS
+}
-export function blockingDependencyFailures(dependencies: Partial<Record<DependencyId, DependencyDiagnostic>>): DependencyId[] {
- return RUNTIME_REQUIRED_DEPENDENCY_IDS.filter(id => dependencies[id]?.state !== 'runnable')
+export function blockingDependencyFailures(
+ dependencies: Partial<Record<DependencyId, DependencyDiagnostic>>,
+ ctx: DependencyRequirementContext = {}
+): DependencyId[] {
+ return requiredDependencyIds(ctx).filter(id => dependencies[id]?.state !== 'runnable')
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shared/dependencyPolicy.ts` around lines 3 - 6, The change incorrectly
hard-coded RUNTIME_REQUIRED_DEPENDENCY_IDS and removed the harness-exception
(skipDeno) contract; revert blockingDependencyFailures to use the full
DEPENDENCY_IDS set and apply the DependencyRequirementPolicy/harness exception
logic: iterate DEPENDENCY_IDS, consult each dependency's DependencyDiagnostic
and its requiredBy.skipDeno flag (or use
DependencyRequirementPolicy.isRuntimeRequired / equivalent) to decide blocking
state, and only include ids whose diagnostics indicate non-runnable after
honoring skipDeno; update blockingDependencyFailures signature to reference
DEPENDENCY_IDS, DependencyRequirementPolicy (or the requiredBy.skipDeno check)
and DependencyDiagnostic/DependencyId symbols.
Source: Coding guidelines
| function materializer(run: (candidate: RuntimeBinaryManifestEntry) => Promise<string>): RuntimeBinaryMaterializer { | ||
| return {materialize: vi.fn(async candidate => ({executablePath: await run(candidate), cacheKey: `${candidate.id}-${candidate.channel}-${candidate.provider}`, metadataPath: '/metadata.json', manifest: candidate}))} as unknown as RuntimeBinaryMaterializer | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining unknown double-casts in this test file.
rg -n '\bas\s+unknown\s+as\b' tests/unit/binary-manager-retry.test.tsRepository: antonio-orionus/Arroxy
Length of output: 1332
Remove the as unknown as RuntimeBinaryMaterializer cast in the test helper
The materializer stub at line 28 double-casts to RuntimeBinaryMaterializer, hiding contract drift; replace it with a strictly typed test double (e.g., a “port” exposing only materialize).
Also, the same file contains additional as unknown as double-casts (lines 38, 48, 77, 139, 164); refactor those similarly or justify each escape hatch inline.
Suggested refactor
-function materializer(run: (candidate: RuntimeBinaryManifestEntry) => Promise<string>): RuntimeBinaryMaterializer {
- return {materialize: vi.fn(async candidate => ({executablePath: await run(candidate), cacheKey: `${candidate.id}-${candidate.channel}-${candidate.provider}`, metadataPath: '/metadata.json', manifest: candidate}))} as unknown as RuntimeBinaryMaterializer
-}
+type RuntimeBinaryMaterializerPort = Pick<RuntimeBinaryMaterializer, 'materialize'>
+
+function materializer(run: (candidate: RuntimeBinaryManifestEntry) => Promise<string>): RuntimeBinaryMaterializerPort {
+ return {
+ materialize: vi.fn(async candidate => ({
+ executablePath: await run(candidate),
+ cacheKey: `${candidate.id}-${candidate.channel}-${candidate.provider}`,
+ metadataPath: '/metadata.json',
+ manifest: candidate
+ }))
+ }
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/binary-manager-retry.test.ts` around lines 27 - 29, The test
helper currently escapes type-safety with "as unknown as
RuntimeBinaryMaterializer" in the materializer factory; replace the cast by
creating a properly typed test-double: declare a concrete materialize mock
(e.g., const materializeMock = vi.fn<Promise<string>,
[RuntimeBinaryManifestEntry]>(async candidate => ...)) and return { materialize:
materializeMock } typed as RuntimeBinaryMaterializer without using "as unknown
as". Do the same for the other occurrences in this file (the remaining
double-casts) by exposing minimal “port” objects typed to only the methods used
(e.g., a typed download/materializer port interface or specific mock variables)
instead of blanket unknown casts so the compiler will catch contract drift.
Source: Coding guidelines
| expect(allMissing.blockingFailures).toHaveLength(3) | ||
| expect(allMissing.dependencies['yt-dlp'].state).toBe('failed') | ||
| expect(allMissing.dependencies.ffmpeg.state).toBe('failed') |
There was a problem hiding this comment.
Strengthen the diagnostics-all-missing assertion to validate the actual blocking set.
Checking only length allows false positives if the wrong dependency becomes blocking. Assert exact IDs (or include ffprobe) to keep this regression test meaningful.
Suggested tightening
- expect(allMissing.blockingFailures).toHaveLength(3)
+ expect(new Set(allMissing.blockingFailures)).toEqual(new Set(['yt-dlp', 'ffmpeg', 'ffprobe']))
expect(allMissing.dependencies['yt-dlp'].state).toBe('failed')
expect(allMissing.dependencies.ffmpeg.state).toBe('failed')
+ expect(allMissing.dependencies.ffprobe.state).toBe('failed')As per coding guidelines, tests should prioritize meaningful coverage and determinism.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(allMissing.blockingFailures).toHaveLength(3) | |
| expect(allMissing.dependencies['yt-dlp'].state).toBe('failed') | |
| expect(allMissing.dependencies.ffmpeg.state).toBe('failed') | |
| expect(new Set(allMissing.blockingFailures)).toEqual(new Set(['yt-dlp', 'ffmpeg', 'ffprobe'])) | |
| expect(allMissing.dependencies['yt-dlp'].state).toBe('failed') | |
| expect(allMissing.dependencies.ffmpeg.state).toBe('failed') | |
| expect(allMissing.dependencies.ffprobe.state).toBe('failed') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/browser-mock-scenarios.test.ts` around lines 253 - 255, The test
currently only checks allMissing.blockingFailures length which can mask wrong
blocking items; update the assertion in the test that references allMissing and
blockingFailures to assert the exact blocking dependency IDs (e.g., verify the
set contains 'yt-dlp' and 'ffmpeg' and also include 'ffprobe' if expected) or
compare equality with the expected array of IDs so the test fails if the wrong
dependency is marked blocking; locate the assertions that reference
allMissing.blockingFailures and allMissing.dependencies (and the dependency keys
'yt-dlp' and ffmpeg) and replace the length-only check with a deterministic
check for the exact IDs/contents of the blockingFailures set.
Source: Coding guidelines
| expect(Number(output)).toBe(expectedPayloadCountFromSources()) | ||
| expect(Number(output)).toBe(30) | ||
| expect(Number(output)).toBe(20) | ||
| }) |
There was a problem hiding this comment.
Remove the fixed payload-count assertion to avoid brittle test failures.
Line 55 hard-codes 20, which can fail on valid target/source matrix updates even when script behavior is correct. Keep the derived assertion from Line 54 as the source of truth.
As per coding guidelines, tests should prioritize meaningful coverage and determinism by validating behavior rather than stale fixed constants.
Suggested patch
expect(Number(output)).toBe(expectedPayloadCountFromSources())
- expect(Number(output)).toBe(20)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(Number(output)).toBe(expectedPayloadCountFromSources()) | |
| expect(Number(output)).toBe(30) | |
| expect(Number(output)).toBe(20) | |
| }) | |
| expect(Number(output)).toBe(expectedPayloadCountFromSources()) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/smoke-all-script.test.ts` around lines 54 - 56, Remove the brittle
hard-coded assertion by deleting the second expectation that compares
Number(output) to 20; keep the derived assertion using
expectedPayloadCountFromSources() as the source of truth (i.e., retain
expect(Number(output)).toBe(expectedPayloadCountFromSources()) and remove
expect(Number(output)).toBe(20)), referencing the test variable output and the
helper expectedPayloadCountFromSources to locate the check in
smoke-all-script.test.ts.
Source: Coding guidelines
1 participant
Summary
Validation
Notes
Summary
This PR replaces the Deno runtime with Electron-as-Node for yt-dlp JavaScript runtime execution and introduces a new runtime binary manifest system for managed binary distribution.
User-Facing Changes
Internal/Refactor Changes
Binary Management System Overhaul:
DenoBinarySource.ts,ManagedSetup.ts,ManagedSourcePlan.ts, andZippedBinaryInstaller.tsRuntimeBinaryIndexService.ts: Loads signed manifests (local/remote/last-known-good/bundled fallback)RuntimeBinaryMaterializer.ts: Materializes artifacts to cache with ZIP extraction, integrity verification, and per-cache-key file lockingBundledRuntimeBinaryIndex.ts: Empty bundled fallback indexRuntimeBinaryTrust.ts: Hardcoded manifest URL, signature URL, and public key for verificationscripts/build/runtimeBinaryManifest.ts: CLI and library for generating, signing, and validating manifest indexesBinaryManager.tsrefactored to resolve yt-dlp via manifest entries using materializer, with fallback to managed artifact cache and system PATHBinaryDownloader.tsswitched fromgottomake-fetch-happen(fetch-based); addeddownloadArtifactToCache()andcopyCachedArtifactToFile()helpers with cacache integrationJS Runtime Handling:
src/main/services/ytDlpJsRuntime.ts: Electron Node runtime probing, environment setup (ELECTRON_RUN_AS_NODE=1), and args constructionYtDlp.tsupdated to usejsRuntimeparameter instead ofdenoPath; probes Electron Node runtime duringprepare()electron-builder.json5: AddedelectronFuses.runAsNodeconfigTest Infrastructure:
src/main/runtimeSmoke.ts: Runtime smoke-test mode that validates Node >=22, stdin probe success, managed yt-dlp source, disabled remote components, and yt-dlp-ejs presencesrc/main/index.ts: Detects and runs runtime smoke before normal startup.github/workflows/runtime-binaries.yml: Generates, signs, validates, and publishes runtime manifest (daily + manual dispatch).github/workflows/e2e-cold-start.yml,.github/workflows/installer-smoke.yml,.github/workflows/release.yml: Added packaged runtime smoke testing with fakenode/deno/yt-dlpshims in PATHscripts/with-runtime-manifest.sh: Wrapper to set up local manifest environmentDependency Changes:
make-fetch-happen,@types/cacache,@types/make-fetch-happengotDEPENDENCY_IDS; updatedDependencySourcetype (removeddeno-landprovider, addedmanagedCachevariant for local cache sources)IPC/Mode Handling:
E2eHarnessMode.skipDenofield andARROXY_E2E_SKIP_DENOenv vare2eModefromIpcDependenciestype;WarmupServiceno longer conditionally skips DenospawnYtDlpsignature (no longer directly handles e2eMode; added optionaltransformEnvcallback)Risk Areas
Electron Security: Added
runAsNodefuse in electron-builder config. This enables Node script execution in Electron context; verify its use is restricted to yt-dlp probing only.Binary Source Hierarchy: Manifest resolution now depends on signed remote index verification, last-known-good caching, and bundled fallback. Failure to fetch/verify manifests falls back to managed artifact cache and system PATH. Verify manifest URL and public key are correctly hardcoded in
RuntimeBinaryTrust.ts.CI/Workflow Changes: Runtime smoke tests added to cold-start, installer, and release workflows with fake binaries in PATH. Windows
.cmdand POSIX shell implementations differ; verify cross-platform smoke test behavior, especially on Windows portable artifact.File Locking:
RuntimeBinaryMaterializeruses filesystem-based locks with PID/timestamp staleness detection. Verify lock cleanup on process kill and mtime comparisons work correctly across time-zone changes.ZIP Extraction Safety: Materializer validates ZIP entry names (rejects path traversal, symlinks) and enforces extraction byte limits. Verify constraints are sufficient for expected yt-dlp binaries.
Dependency Type Narrowing:
DependencyIdtype now excludes'deno'. Verify all diagnostic/telemetry code paths handle onlyyt-dlp,ffmpeg,ffprobe.Tests/Checks to Run
As noted in PR objectives:
Additional coverage: