fix: skip unsupported PowerShell scripts in Windows signing by wwwillchen · Pull Request #3169 · dyad-sh/dyad

wwwillchen · 2026-04-08T17:55:11Z

Summary

remove the two bundled node-pty PowerShell helper scripts before Windows signing runs
keep the Azure Trusted Signing flow unchanged for signable binaries and libraries
add a unit test covering the packaging cleanup helper

Test plan

npm run fmt
npm run lint:fix
npm run ts
npm test

Generated with Claude Code

wwwillchen · 2026-04-08T17:55:48Z

@BugBot run

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d2a5ac410

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/lib/windows_signing.ts

gemini-code-assist

Code Review

This pull request introduces a mechanism to remove specific PowerShell scripts from native dependencies that cause Windows Authenticode signing failures. It adds a utility function to handle the file removal, integrates it into the Electron Forge configuration via an afterCopy hook, and includes a corresponding test suite. The review feedback suggests refactoring the afterCopy hook to use an async function for improved readability and to simplify the promise handling.

forge.config.ts

Copilot

Pull request overview

Adds a packaging-time cleanup step to remove bundled PowerShell helper scripts (from node-pty) that cause Windows signing to fail, and documents the rationale.

Changes:

Introduces removeUnsupportedWindowsSigningFiles() and a list of unsupported paths.
Hooks the cleanup into Electron Forge packaging via packagerConfig.afterCopy when Windows signing is enabled.
Adds a Vitest unit test for the cleanup helper and updates native module documentation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/lib/windows_signing.ts	Adds helper + path list to delete unsupported `.ps1` files before signing.
src/tests/windows_signing.test.ts	Adds unit test for the cleanup helper.
forge.config.ts	Wires cleanup into Forge packaging when Windows signing is enabled.
rules/native-modules.md	Documents the Windows signing failure mode and mitigation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/lib/windows_signing.ts

forge.config.ts

src/__tests__/windows_signing.test.ts

- fix Windows signing cleanup paths for Forge afterCopy hooks - keep the existing callback hook shape for packager type compatibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

wwwillchen · 2026-04-08T18:03:36Z

@BugBot run

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 9dabf6c. Configure here.}

wwwillchen · 2026-04-08T18:08:00Z

🤖 Claude Code Review Summary

PR Confidence: 5/5

All trusted review threads were addressed or resolved, and the branch passes formatting, lint, type-checks, and tests after the follow-up fix.

Unresolved Threads

No unresolved threads

Resolved Threads

Issue	Rationale	Link
Windows signing cleanup targeted the wrong pre-asar paths	Updated the cleanup helper to remove the two PowerShell scripts from in the build directory, which is the actual pre-sign location used by Forge packaging.	View
Callback-style hook kept as-is	Replied and resolved: kept the callback form because the current hook type is callback-based in this repo, so it remains type-safe without casts while preserving identical behavior.	View

Product Principle Suggestions

No suggestions

🤖 Generated by Claude Code

wwwillchen · 2026-04-08T18:08:30Z

Claude Code Review Summary

PR Confidence: 5/5

All trusted review threads were addressed or resolved, and the branch passes formatting, lint, type-checks, and tests after the follow-up fix.

Unresolved Threads

No unresolved threads

Resolved Threads

Issue	Rationale	Link
Windows signing cleanup targeted the wrong pre-ASAR paths	Updated the cleanup helper and its test to remove the two node-pty PowerShell scripts from node_modules/... in the afterCopy build directory, which is the actual pre-sign location used by Forge packaging.	View
Callback-style afterCopy hook kept as-is	Replied and resolved: kept the callback form because the current @electron/packager hook type is callback-based in this repo, so it remains type-safe without casts while preserving identical behavior.	View

Product Principle Suggestions

No suggestions

Generated by Claude Code

dyad-assistant

Multi-agent review: 4 MEDIUM issue(s) and 2 LOW notes

dyad-assistant · 2026-04-08T18:10:18Z

src/lib/windows_signing.ts

@@ -0,0 +1,18 @@
+import fs from "node:fs/promises";


🟡 MEDIUM | missing-why-comment

No explanation of the signtool failure mode

This module has zero comments explaining why these specific files must be removed. A future maintainer (or anyone debugging a regression) will see a file that deletes two random .ps1 files and have no context for why. The "why" exists in rules/native-modules.md but is not discoverable from the code.

💡 Suggestion: Add a file-level JSDoc comment explaining: (1) signtool.exe recursively signs .ps1 files inside packaged native dependencies, (2) node-pty's winpty debug helpers are not Authenticode-signable and are not loaded at runtime, (3) removing them is the minimal intervention to let signing succeed. Reference rules/native-modules.md.

dyad-assistant · 2026-04-08T18:10:18Z

src/lib/windows_signing.ts

+export const UNSUPPORTED_WINDOWS_SIGNING_RELATIVE_PATHS = [
+  "node_modules/node-pty/deps/winpty/misc/ConinMode.ps1",
+  "node_modules/node-pty/deps/winpty/misc/IdentifyConsoleWindow.ps1",
+] as const;


🟡 MEDIUM | brittleness

Hardcoded file list will silently break on node-pty upgrades

This pins two exact paths inside node-pty's winpty deps. If node-pty adds, renames, or moves .ps1 helpers in a future version, signtool.exe will fail again with the same Number of errors: 2 error this PR is fixing — and it will need to be re-diagnosed from scratch. Because fs.rm({ force: true }) silently succeeds on missing files, the CI error message will be the signtool failure, not a clear "this file no longer exists."

💡 Suggestion: Either switch to a glob-based removal (e.g., recursively delete *.ps1 under node_modules/node-pty/deps/winpty/), or at minimum add a comment pinning the verified node-pty version and reminding maintainers to revisit this list on upgrade.

dyad-assistant · 2026-04-08T18:10:18Z

src/lib/windows_signing.ts

+      await fs.rm(absolutePath, { force: true });
+    }),
+  );
+}


🟡 MEDIUM | file-placement

Build-time helper lives under src/lib/ alongside runtime app code

src/lib/ in this repo contains runtime app code (toast.tsx, errors.ts, queryKeys.ts, etc.) that ships in the Electron bundle. This new helper is only consumed by forge.config.ts at package time and has no runtime purpose. There is already a windowsSign.ts at the repo root — that's the natural neighbor for a build-time Windows signing helper. Placing build tooling under src/lib/ blurs the runtime/build boundary and risks future runtime code importing signing internals.

💡 Suggestion: Move this file (and its test) next to windowsSign.ts at the repo root, or into a dedicated build/ or scripts/ directory.

dyad-assistant · 2026-04-08T18:10:18Z

src/__tests__/windows_signing.test.ts

+        ).rejects.toThrow();
+      }),
+    );
+  });


🟡 MEDIUM | weak-test

Test is tautological and provides little signal

The test iterates UNSUPPORTED_WINDOWS_SIGNING_RELATIVE_PATHS, creates files at exactly those paths, calls the function, and verifies files at exactly those paths are gone. This only proves fs.rm works — it doesn't validate that the constant points to the right locations in a real Electron Forge afterCopy build tree, nor that the removal is scoped (the test would also pass if the function did fs.rm(buildPath, { recursive: true })).

This is the same shape of test that failed to catch the resources/app.asar.unpacked/ path bug flagged earlier in this PR — a tautological test over the constant can never catch an incorrect constant.

💡 Suggestion: Make the constant module-private and have the test assert on concrete, hardcoded path literals (e.g., directly write "node_modules/node-pty/deps/winpty/misc/ConinMode.ps1" in the test). Additionally, create a sibling file that should not be deleted and assert it is still present after the call — this catches over-broad deletion and anchors the test to expected behavior rather than the implementation's own data.

dyad-assistant · 2026-04-08T18:10:41Z

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues

Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. The functional fix looks correct (paths were corrected in commit 9dabf6c and afterCopy runs against the pre-asar app tree where node_modules/node-pty/... lives). No HIGH severity issues, but 4 MEDIUM items around code quality, test effectiveness, and maintainability are worth considering before merge.

Issues Summary

Severity	File	Issue
🟡 MEDIUM	`src/lib/windows_signing.ts:1`	No `why` comment explaining the signtool failure mode
🟡 MEDIUM	`src/lib/windows_signing.ts:7`	Hardcoded file list will silently break on node-pty upgrades
🟡 MEDIUM	`src/lib/windows_signing.ts:18`	Build-time helper placed under `src/lib/` (runtime app code)
🟡 MEDIUM	`src/__tests__/windows_signing.test.ts:42`	Test is tautological — loops over the constant and asserts on the same paths

🟢 Low Priority Notes (2 items)

Missing-file idempotency not tested — src/__tests__/windows_signing.test.ts — the happy path assumes fs.rm({ force: true }) silently handles missing files, but no test guards against a regression that drops force: true.
Constant exported solely for test consumption — src/lib/windows_signing.ts:4 — UNSUPPORTED_WINDOWS_SIGNING_RELATIVE_PATHS exists in the public surface only so the test can loop over it; this is the root cause of the tautological test.

🚫 Dropped False Positives (2 items)

~~afterCopy gated on WINDOWS_SIGN~~ — Dropped: current code is correct; hypothetical future signing flows that bypass WINDOWS_SIGN are speculative.
~~Test name describes what, not why~~ — Dropped: the existing test description "removes the node-pty PowerShell scripts that signtool cannot sign" does reference the signtool failure.

Generated by Dyadbot multi-agent code review

claude added 2 commits April 8, 2026 10:52

fix: skip unsupported windows signing scripts

662ffba

docs: record session learnings

8d2a5ac

Copilot AI review requested due to automatic review settings April 8, 2026 17:55

wwwillchen had a problem deploying to ai-bots April 8, 2026 17:55 — with GitHub Actions Error

wwwillchen temporarily deployed to ai-bots April 8, 2026 17:55 — with GitHub Actions Inactive

Copilot started reviewing on behalf of wwwillchen April 8, 2026 17:56 View session

chatgpt-codex-connector bot reviewed Apr 8, 2026

View reviewed changes

src/lib/windows_signing.ts Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

gemini-code-assist bot reviewed Apr 8, 2026

View reviewed changes

forge.config.ts Show resolved Hide resolved

Copilot AI reviewed Apr 8, 2026

View reviewed changes

src/lib/windows_signing.ts Show resolved Hide resolved

forge.config.ts Show resolved Hide resolved

src/__tests__/windows_signing.test.ts Show resolved Hide resolved

Address PR review comments

9dabf6c

- fix Windows signing cleanup paths for Forge afterCopy hooks - keep the existing callback hook shape for packager type compatibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

wwwillchen requested a review from a team April 8, 2026 18:03

This comment was marked as resolved.

Sign in to view

wwwillchen temporarily deployed to ai-bots April 8, 2026 18:03 — with GitHub Actions Inactive

cursor bot reviewed Apr 8, 2026

View reviewed changes

wwwillchen merged commit 8fdf9d7 into dyad-sh:main Apr 8, 2026
10 of 12 checks passed

dyad-assistant bot reviewed Apr 8, 2026

View reviewed changes

Conversation

wwwillchen commented Apr 8, 2026 • edited by devin-ai-integration bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

wwwillchen commented Apr 8, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

wwwillchen commented Apr 8, 2026

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

wwwillchen commented Apr 8, 2026

🤖 Claude Code Review Summary

PR Confidence: 5/5

Unresolved Threads

Resolved Threads

Uh oh!

wwwillchen commented Apr 8, 2026

Claude Code Review Summary

PR Confidence: 5/5

Unresolved Threads

Resolved Threads

Uh oh!

Uh oh!

dyad-assistant bot left a comment

Choose a reason for hiding this comment

Uh oh!

dyad-assistant bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dyad-assistant bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dyad-assistant bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dyad-assistant bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dyad-assistant bot commented Apr 8, 2026

🔍 Dyadbot Code Review Summary

Issues Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wwwillchen commented Apr 8, 2026 •

edited by devin-ai-integration bot

Loading