Skip to content

[codex] Prune renderer dependencies from production manifest - #463

Merged
100yenadmin merged 7 commits into
mainfrom
codex/workbench-renderer-deps-prune-452
Jun 27, 2026
Merged

[codex] Prune renderer dependencies from production manifest#463
100yenadmin merged 7 commits into
mainfrom
codex/workbench-renderer-deps-prune-452

Conversation

@100yenadmin

@100yenadmin 100yenadmin commented Jun 27, 2026

Copy link
Copy Markdown
Owner

Closes #452.

Summary

  • Moves 35 renderer-only direct packages from root dependencies to devDependencies so production packaging stops treating them as raw runtime node_modules inputs.
  • Keeps the renderer contract intact: Vite still bundles these packages into out/renderer for the Electron UI.
  • Adds docs/evaos/workbench-renderer-dependency-ledger.md with the move rule, moved package evidence, deferred packages, runtime-transitive keepers, and package inspection requirements.
  • Leaves main/preload/native/updater/runtime and ambiguous packages in dependencies; broad unused-dependency removal is intentionally out of scope.
  • Keeps diff and eventemitter3 in dependencies after Thin App Smoke proved they ship for runtime-transitive reasons via @office-ai/aioncli-core, @office-ai/platform, and @wecom/aibot-node-sdk.

Local focused validation

  • ./node_modules/.bin/oxfmt --check package.json bun.lock scripts/evaosVerifyRendererDependencyPrune.js tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts docs/evaos/workbench-renderer-dependency-ledger.md
  • ./node_modules/.bin/oxlint scripts/evaosVerifyRendererDependencyPrune.js tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
  • bunx vitest run tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts --maxWorkers=1
  • actionlint .github/workflows/pr-checks.yml
  • git diff --check
  • expanded runtime import scan over packages/desktop/src/common, packages/desktop/src/process, packages/desktop/src/preload, packages/web-host/src, packages/web-cli/src, and scripts found no hits for the 35 moved packages
  • manifest assertion verified 35 moved packages are in devDependencies only and diff/eventemitter3 remain in dependencies
  • pwd && bun install --frozen-lockfile passed from /Volumes/LEXAR/repos/worktrees/evaos-gui-build-cost-sprint with no file changes; note this repo command runs the local postinstall/native rebuild hook

Required remote proof before merge

  • Current-head PR Checks green, including Thin App Smoke.
  • Thin App Smoke must run scripts/evaosVerifyRendererDependencyPrune.js against the unpacked .app, proving moved renderer libraries are absent from raw packaged app.asar/node_modules and app.asar.unpacked/node_modules.
  • CodeRabbit/current-head review clean.

Release boundary

This does not change default release behavior, signing/notary, updater/appcast, evaOS/Hermes broker protocols, ACP behavior, or Bridge/AionCore/hub resource inclusion.

Summary by CodeRabbit

  • New Features
    • Added an automated verifier that checks shipped Electron bundles to ensure renderer-only libraries aren’t included in runtime areas.
  • Documentation
    • Added a renderer dependency “ledger” documenting eligibility rules, required evidence, and verification steps.
  • CI / Workflows
    • Extended thin-app smoke checks to validate pruning for both app.asar and unpacked node_modules, plus matching rules against package.json.
  • Chores
    • Reclassified several UI/editor/framework packages from runtime dependencies to development dependencies.
  • Tests
    • Added unit tests to confirm correct behavior and detection of renderer leaks and missing runtime-transitive packages.

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 77232f6c-ede8-4899-acd4-08378efb4289

📥 Commits

Reviewing files that changed from the base of the PR and between 1961c0e and b0207f7.

📒 Files selected for processing (3)
  • docs/evaos/workbench-renderer-dependency-ledger.md
  • scripts/evaosVerifyRendererDependencyPrune.js
  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: Thin App Smoke (macos-arm64)
  • GitHub Check: Unit Tests (macos-14)
  • GitHub Check: Coverage Test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions

Files:

  • scripts/evaosVerifyRendererDependencyPrune.js
  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*[!.module].ts

📄 CodeRabbit inference engine (AGENTS.md)

Utility files must use camelCase naming (e.g., formatDate.ts)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use @icon-park/react for icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases: @/, @process/, @renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Vitest 4 for testing with a coverage target of at least 80%

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
🔇 Additional comments (3)
tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts (1)

35-58: LGTM!

Also applies to: 74-78, 121-126

docs/evaos/workbench-renderer-dependency-ledger.md (1)

47-48: LGTM!

scripts/evaosVerifyRendererDependencyPrune.js (1)

90-108: LGTM!


📝 Walkthrough

Walkthrough

Adds a renderer dependency ledger, reclassifies renderer/UI/editor packages in package.json, and adds a CLI plus CI check to verify packaged app contents and dependency bucket assignments.

Changes

Renderer-only dependency pruning

Layer / File(s) Summary
Criteria and dependency relocation
docs/evaos/workbench-renderer-dependency-ledger.md, package.json
The ledger defines renderer-only package criteria and records moved, kept, and deferred package groups while matching entries are removed from dependencies and added to devDependencies.
ASAR file scanning
scripts/evaosVerifyRendererDependencyPrune.js
The Node CLI adds ASAR header reading, recursive path collection, and package path matching helpers used to inspect packaged app contents.
Dependency verification CLI
scripts/evaosVerifyRendererDependencyPrune.js
The verifier checks package bucket assignments, inspects unpacked app contents for renderer-only leaks and runtime-transitive presence, and exposes CLI entry points and exports.
Tests and CI wiring
tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts, .github/workflows/pr-checks.yml
The unit suite builds fake packaged app layouts to cover pass and leak cases, and the macOS ARM64 smoke workflow runs the verifier and reports the result in the step summary.

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers scope and testing, but it does not follow the required template sections like Type of Change or the checklist. Add the missing template sections: Description, Related Issues, Type of Change, Testing checklist, Screenshots, and Additional Context.
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the main change: pruning renderer dependencies from the production manifest.
Linked Issues check ✅ Passed The changes satisfy #452 by moving renderer-only packages to devDependencies, keeping runtime packages in dependencies, and adding packaging verification.
Out of Scope Changes check ✅ Passed All changes support the renderer dependency pruning goal; no unrelated feature work or broad cleanup was introduced.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/workbench-renderer-deps-prune-452

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@docs/evaos/workbench-renderer-dependency-ledger.md`:
- Around line 44-54: The verification commands in the workbench renderer
dependency ledger only check a subset of moved packages, so they can miss
imports or packaged copies of other renderer-only dependencies. Update the rg
and asar-list checks to include every package in the Moved Renderer-Only
Packages set, referencing the existing proof commands and the package groups
already listed there, so the ledger fails if any moved dependency still appears
outside the renderer or inside app.asar/node_modules.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3e1cdd3f-68f8-4e04-aa37-2df7de2b52ec

📥 Commits

Reviewing files that changed from the base of the PR and between d28fca6 and 2b551b8.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • docs/evaos/workbench-renderer-dependency-ledger.md
  • package.json
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: Thin App Smoke (macos-arm64)
  • GitHub Check: Unit Tests (macos-14)
  • GitHub Check: Coverage Test
🧰 Additional context used
🪛 LanguageTool
docs/evaos/workbench-renderer-dependency-ledger.md

[style] ~11-~11: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...derer Vite build into out/renderer. - It is not imported by `packages/desktop/sr...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ackages/web-cli/, or scripts/. - It is not explicitly included in packages...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~35-~35: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..._modulesandasarUnpack` allowlist. - Proven API, bridge, agent, or preload keepers:...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~38-~38: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: .... - Browser/node polyfill packages that need a separate audit before removal: `buffe...

(EN_REPEATEDWORDS_NEED)

🔇 Additional comments (2)
docs/evaos/workbench-renderer-dependency-ledger.md (1)

1-39: LGTM!

Also applies to: 56-58

package.json (1)

130-144: LGTM!

Also applies to: 166-207

Comment thread docs/evaos/workbench-renderer-dependency-ledger.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@scripts/evaosVerifyRendererDependencyPrune.js`:
- Around line 6-44: The hardcoded rendererOnlyPackages list in
evaosVerifyRendererDependencyPrune.js can drift from package.json and miss
regressions. Add a guard in the verifier (or a dedicated test) that compares
rendererOnlyPackages against the renderer-only entries in package.json, or
refactor the script to derive this list from one source of truth so the check
stays in sync automatically. Use the rendererOnlyPackages constant and the
verifier logic in evaosVerifyRendererDependencyPrune.js as the place to anchor
the fix.
- Around line 50-64: Add JSDoc comments for the exported/public functions in
this module, especially verifyRendererDependencyPrune and readAsarHeader.
Document each function’s purpose, parameters, and return value in a concise
JSDoc block placed directly above the function definition so the public API is
properly documented.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d43e115a-896b-4f31-90fa-8b1d05541457

📥 Commits

Reviewing files that changed from the base of the PR and between 2b551b8 and ffdaf06.

📒 Files selected for processing (4)
  • .github/workflows/pr-checks.yml
  • docs/evaos/workbench-renderer-dependency-ledger.md
  • scripts/evaosVerifyRendererDependencyPrune.js
  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
  • GitHub Check: Thin App Smoke (macos-arm64)
  • GitHub Check: Unit Tests (macos-14)
  • GitHub Check: Coverage Test
  • GitHub Check: Code Quality
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts

📄 CodeRabbit inference engine (AGENTS.md)

Utility files must use camelCase naming (e.g., formatDate.ts)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
  • scripts/evaosVerifyRendererDependencyPrune.js
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use @icon-park/react for icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases: @/, @process/, @renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Vitest 4 for testing with a coverage target of at least 80%

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
🪛 ast-grep (0.44.0)
tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts

[warning] 49-49: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(path.join(resources, 'app.asar'), Buffer.concat([sizePickle, headerPickle]))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename-typescript)

🔇 Additional comments (4)
docs/evaos/workbench-renderer-dependency-ledger.md (1)

49-57: LGTM!

scripts/evaosVerifyRendererDependencyPrune.js (1)

96-103: 🎯 Functional Correctness | 💤 Low value

Nested node_modules copies are not detected.

The leak match only catches top-level /node_modules/<pkg>; a renderer-only package hoisted into a nested location (e.g. /node_modules/<other>/node_modules/react) inside app.asar would pass undetected. Likely acceptable for direct-dependency pruning, but confirm the packaging layout never nests these so the verification goal holds.

tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts (1)

27-51: The path-traversal static-analysis hint on Line 49 is a false positive — the path derives from a controlled mkdtempSync temp dir, not external input. No action needed.

Otherwise the fake-asar writer faithfully mirrors the verifier's pickle parsing and the pass/leak cases are well covered.

.github/workflows/pr-checks.yml (1)

710-710: LGTM!

Also applies to: 719-719

Comment thread scripts/evaosVerifyRendererDependencyPrune.js
Comment thread scripts/evaosVerifyRendererDependencyPrune.js
@coderabbitai coderabbitai Bot added the kind:integration Integration implementation issue label Jun 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@docs/evaos/workbench-renderer-dependency-ledger.md`:
- Around line 46-47: The dependency ledger check in the grep command only
catches ES module “from” imports, so CommonJS requires and bare side-effect
imports can still evade detection. Update the search used in this docs entry to
also match require-based imports and any bare imports for the same dependency
list, keeping the existing target directories and symbols like the rg command
and the packages/desktop and scripts paths in scope.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3911e460-44ac-43d2-b3ca-4224fa7a3218

📥 Commits

Reviewing files that changed from the base of the PR and between ffdaf06 and 4a95793.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • docs/evaos/workbench-renderer-dependency-ledger.md
  • package.json
  • scripts/evaosVerifyRendererDependencyPrune.js
💤 Files with no reviewable changes (1)
  • scripts/evaosVerifyRendererDependencyPrune.js
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: Code Quality
  • GitHub Check: Unit Tests (macos-14)
  • GitHub Check: Coverage Test
  • GitHub Check: PR Check Plan
  • GitHub Check: Code Quality
⚠️ CI failures not shown inline (10)

GitHub Actions: PR Checks / I18n Check: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Run bun install --frozen-lockfile
 �[36;1mbun install --frozen-lockfile�[0m
 shell: /usr/bin/bash -e {0}
 env:
   BUN_INSTALL_REGISTRY: https://registry.npmjs.org/
   PR_BASE_REF: main
 ##[endgroup]
 bun install v1.3.14 (0d9b296a)
 Resolving dependencies
 Resolved, downloaded and extracted [1]
 $ node scripts/postinstall.js
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / PR Check Plan: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Fetching the repository
 [command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +refs/heads/main*:refs/remotes/origin/main* +refs/tags/main*:refs/tags/main*
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / Coverage Test: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Run bun install --frozen-lockfile
 �[36;1mbun install --frozen-lockfile�[0m
 shell: /usr/bin/bash -e {0}
 env:
   BUN_INSTALL_REGISTRY: https://registry.npmjs.org/
   PR_BASE_REF: main
 ##[endgroup]
 bun install v1.3.14 (0d9b296a)
 Resolving dependencies
 Resolved, downloaded and extracted [1]
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / Code Quality: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Environment details
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / 6_PR Check Plan.txt: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Fetching the repository
 [command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +refs/heads/main*:refs/remotes/origin/main* +refs/tags/main*:refs/tags/main*
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / Unit Tests (macos-14): [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Run oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6
 with:
   bun-version: latest
   cache: true
   no-cache: false
   ***REDACTED***
 env:
   BUN_INSTALL_REGISTRY: https://registry.npmjs.org/
   PR_BASE_REF: main
 ##[endgroup]
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / 4_Coverage Test.txt: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Run bun install --frozen-lockfile
 �[36;1mbun install --frozen-lockfile�[0m
 shell: /usr/bin/bash -e {0}
 env:
   BUN_INSTALL_REGISTRY: https://registry.npmjs.org/
   PR_BASE_REF: main
 ##[endgroup]
 bun install v1.3.14 (0d9b296a)
 Resolving dependencies
 Resolved, downloaded and extracted [1]
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / 3_I18n Check.txt: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Run bun install --frozen-lockfile
 �[36;1mbun install --frozen-lockfile�[0m
 shell: /usr/bin/bash -e {0}
 env:
   BUN_INSTALL_REGISTRY: https://registry.npmjs.org/
   PR_BASE_REF: main
 ##[endgroup]
 bun install v1.3.14 (0d9b296a)
 Resolving dependencies
 Resolved, downloaded and extracted [1]
 $ node scripts/postinstall.js
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / 2_Unit Tests (macos-14).txt: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Run oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6
 with:
   bun-version: latest
   cache: true
   no-cache: false
   ***REDACTED***
 env:
   BUN_INSTALL_REGISTRY: https://registry.npmjs.org/
   PR_BASE_REF: main
 ##[endgroup]
 ##[error]The operation was canceled.

GitHub Actions: PR Checks / 5_Code Quality.txt: [codex] Prune renderer dependencies from production manifest

Conclusion: failure

View job details

##[group]Environment details
 ##[error]The operation was canceled.
🧰 Additional context used
🪛 LanguageTool
docs/evaos/workbench-renderer-dependency-ledger.md

[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tform, @sentry/electron, openai`. - Proven runtime-transitive keepers found by Thi...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🔇 Additional comments (1)
package.json (1)

94-100: LGTM!

Comment thread docs/evaos/workbench-renderer-dependency-ledger.md Outdated
@coderabbitai coderabbitai Bot removed the kind:integration Integration implementation issue label Jun 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/evaos/workbench-renderer-dependency-ledger.md (1)

45-48: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Scan the full packages/web-host/** and packages/web-cli/** trees.

The rule in Lines 9-12 bans these packages anywhere under packages/web-host/** and packages/web-cli/**, but the proof command only searches their src/ folders. That lets non-src imports pass the documented check.

Suggested update
 rg -n "..." \
-  packages/desktop/src/common packages/desktop/src/process packages/desktop/src/preload packages/web-host/src packages/web-cli/src scripts
+  packages/desktop/src/common packages/desktop/src/process packages/desktop/src/preload packages/web-host packages/web-cli scripts
🤖 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 `@docs/evaos/workbench-renderer-dependency-ledger.md` around lines 45 - 48,
Update the dependency-check proof command to scan the entire packages/web-host
and packages/web-cli trees instead of only their src folders, so it matches the
ban enforced by the documented rule. Modify the rg command in the dependency
ledger doc to include the full packages/web-host/** and packages/web-cli/**
paths, and keep the existing import-pattern regex intact. Use the command block
in the dependency ledger entry as the place to adjust this validation.
🤖 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/pr-checks.yml:
- Line 710: Move the manifest-only verification in the pr-checks workflow so
`scripts/evaosVerifyRendererDependencyPrune.js --verify-package-json` runs
before the `Build unpacked app only` step, since it does not require `APP_PATH`.
Keep the check in the same workflow job but place it earlier to fail fast and
avoid spending time on the macOS packaging build when the manifest is already
invalid.

In `@scripts/evaosVerifyRendererDependencyPrune.js`:
- Around line 108-120: The current runtime-transitive check in
evaosVerifyRendererDependencyPrune only validates package.json placement and
misses whether those packages are actually shipped in the packaged app. Update
the existing verification flow around evaosVerifyRendererDependencyPrune to also
reuse the app.asar and app.asar.unpacked/node_modules scan, and require each
runtimeTransitivePackage (including diff and eventemitter3) to be present in at
least one of those locations. If any runtime-required package is absent from the
packaged app, add an error before the final throw so CI fails appropriately.

---

Outside diff comments:
In `@docs/evaos/workbench-renderer-dependency-ledger.md`:
- Around line 45-48: Update the dependency-check proof command to scan the
entire packages/web-host and packages/web-cli trees instead of only their src
folders, so it matches the ban enforced by the documented rule. Modify the rg
command in the dependency ledger doc to include the full packages/web-host/**
and packages/web-cli/** paths, and keep the existing import-pattern regex
intact. Use the command block in the dependency ledger entry as the place to
adjust this validation.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a437515f-4fab-4276-877f-6fe78036bbb4

📥 Commits

Reviewing files that changed from the base of the PR and between 4a95793 and e76f65b.

📒 Files selected for processing (4)
  • .github/workflows/pr-checks.yml
  • docs/evaos/workbench-renderer-dependency-ledger.md
  • scripts/evaosVerifyRendererDependencyPrune.js
  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: Thin App Smoke (macos-arm64)
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts

📄 CodeRabbit inference engine (AGENTS.md)

Utility files must use camelCase naming (e.g., formatDate.ts)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
  • scripts/evaosVerifyRendererDependencyPrune.js
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use @icon-park/react for icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases: @/, @process/, @renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Vitest 4 for testing with a coverage target of at least 80%

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
🪛 ast-grep (0.44.0)
scripts/evaosVerifyRendererDependencyPrune.js

[warning] 96-96: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(packageJsonPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename)

🔇 Additional comments (1)
docs/evaos/workbench-renderer-dependency-ledger.md (1)

46-47: Match require() and bare imports in this proof command.

Line 46 still only catches from 'pkg', so CommonJS and side-effect imports outside the renderer can evade the documented verification.

Comment thread .github/workflows/pr-checks.yml Outdated
Comment thread scripts/evaosVerifyRendererDependencyPrune.js

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@docs/evaos/workbench-renderer-dependency-ledger.md`:
- Around line 47-48: The dependency ledger check only matches static imports and
require calls, so it misses dynamic import() usage. Update the existing rg-based
check in the ledger script so it also searches for import('...') of
MOVED_RENDERER_PACKAGE_PATTERN, and keep the scope covering
packages/desktop/src/common, packages/desktop/src/process,
packages/desktop/src/preload, packages/web-host, packages/web-cli, and scripts.
Ensure the pattern is added alongside the current import/require matches so
moved renderer packages fail the check no matter how they are loaded.

In `@scripts/evaosVerifyRendererDependencyPrune.js`:
- Around line 90-93: The packagePathExistsInAsar matcher in
packagePathExistsInAsar only checks for /node_modules/<pkg> at the ASAR root, so
it misses nested dependency copies. Update the path matching logic to detect any
node_modules/<pkg> segment anywhere in asarPaths, not just the root-level path,
while keeping the existing packageName-based check structure intact.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1af4fc44-3b41-4aad-b56b-37a25d4974b4

📥 Commits

Reviewing files that changed from the base of the PR and between d12f31d and 1961c0e.

📒 Files selected for processing (4)
  • .github/workflows/pr-checks.yml
  • docs/evaos/workbench-renderer-dependency-ledger.md
  • scripts/evaosVerifyRendererDependencyPrune.js
  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: Thin App Smoke (macos-arm64)
  • GitHub Check: Coverage Test
  • GitHub Check: Unit Tests (macos-14)
  • GitHub Check: Code Quality
  • GitHub Check: Code Quality
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts

📄 CodeRabbit inference engine (AGENTS.md)

Utility files must use camelCase naming (e.g., formatDate.ts)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
  • scripts/evaosVerifyRendererDependencyPrune.js
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use @icon-park/react for icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases: @/, @process/, @renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Vitest 4 for testing with a coverage target of at least 80%

Files:

  • tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts
🔇 Additional comments (3)
scripts/evaosVerifyRendererDependencyPrune.js (1)

135-179: LGTM!

tests/unit/process/evaosVerifyRendererDependencyPrune.test.ts (1)

15-124: LGTM!

.github/workflows/pr-checks.yml (1)

607-609: LGTM!

Also applies to: 713-713, 722-722

Comment thread docs/evaos/workbench-renderer-dependency-ledger.md Outdated
Comment thread scripts/evaosVerifyRendererDependencyPrune.js
@100yenadmin
100yenadmin merged commit 339fdd7 into main Jun 27, 2026
24 of 27 checks passed
@100yenadmin
100yenadmin deleted the codex/workbench-renderer-deps-prune-452 branch June 27, 2026 12:13
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.

Prune renderer-only dependencies from raw app.asar/node_modules

1 participant