Auto-deny ignored pnpm builds#3843
Conversation
|
@BugBot run |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58f4d4538b
ℹ️ 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".
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This is a well-designed and carefully implemented feature that solves a real user-facing problem (apps stuck on Issues Summary
🟢 Low Priority Notes (2 items)
Generated by Dyadbot persona-based code review |
|
@BugBot run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e656ab7f11
ℹ️ 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".
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge This PR implements a well-designed auto-deny mechanism for pnpm builds not on Dyad's curated allow-list, with proactive denial recording, self-healing for custom commands, promotion when the allow-list evolves, and telemetry for curation. The plan doc is thorough, tests are comprehensive, and the core YAML manipulation logic is correct (backward splice iteration, scoped package name parsing, idempotent writes all verified). One HIGH issue should be fixed before merge. Issues Summary
🔴 Unbounded processOutput accumulation — 🟡 Async IIFE in close handler — The 🟡 Inconsistent node_modules removal — 🟡 Silent self-heal — When self-heal triggers during app runtime (custom command), the user sees the process exit and restart with no explanation. The add-dependency flow provides a clear security-policy note, but the runtime path is silent. Adding an 🟢 Low Priority Notes (3 items)
Generated by Dyadbot persona-based code review |
… chain, bounded output tail - pnpm 10.x/11.x write node_modules/.modules.yaml as JSON; parse JSON first with the block-YAML line parser as fallback, so the proactive auto-deny path actually detects ignored builds on real installs - Replace the inline "(pnpm rebuild 'x' || true)" chain with a shared getBestEffortPnpmRebuildCommand that emits unquoted validated names and an echo fallback, both of which work under cmd.exe as well as sh - Cap the self-heal process-output buffer at a 64KB rolling tail and only accumulate when a self-heal callback is attached, so long-running dev servers don't grow memory unboundedly - Strip the trailing period/box borders in the ignored-builds output parser - Add unit coverage: real JSON .modules.yaml fixture, rebuild command quoting/filtering, output-parser forms Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@BugBot run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a4ed54530
ℹ️ 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".
- Record ignored builds after successful default app installs (host and Docker) once the dev-server URL appears, so first-install-via-run apps also get pkg:false entries committed - Detect "Ignored build scripts" in cloud sandbox log streams and record denials locally (node_modules never syncs back from the sandbox) - Record denials against existing config when the remote allow-list fetch fails instead of skipping entirely - Wrap the process close-handler async IIFE in a try/catch that falls back to removeAppIfCurrentProcess - Emit a user-visible app:output message when runtime self-heal records denials and reinstalls - Document why the upgrade-path self-heal retry doesn't need to remove node_modules Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@BugBot run |
Custom install && start chains run strict pnpm inside the container too; mirror the host retry path (no host node_modules cleanup — the container volume is what matters and explicit false entries pass fast-path installs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All 12 review threads were addressed with verified fixes and passing unit tests, typecheck, and lint; the remaining risk is that the Docker and cloud-sandbox self-heal paths rely on shared mechanisms rather than dedicated E2E coverage. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — Principle #4 (Transparent Over Magical) directly resolved the one judgment-call thread; all other threads were objective correctness/robustness issues. 🤖 Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fac6aa7059
ℹ️ 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".
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This is a well-structured PR that solves a real user-facing problem (apps stuck on Key correctness points verified:
Issues Summary
🟢 Low Priority Notes (2 items)
Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
- Convert pnpm 11 placeholder allowBuilds entries ("set this to true or
false") into tagged denials during every Dyad rewrite; verified the
placeholder is written by non-strict installs, does not satisfy strict
mode, and previously shadowed the auto-deny pass entirely
- Create the allowBuilds key when the offline denial-only path runs against
a config that lost it
- Extract recordAndReportDeniedPnpmBuilds + resolvePnpmIgnoredBuilds into
pnpm_denied_builds.ts so the record+telemetry contract has one owner
across add-dependency, app-run, self-heal, and cloud-sandbox flows
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All 16 review threads across both rounds are addressed with empirically verified fixes and 122 passing unit tests; remaining risk is limited to paths covered by shared mechanisms rather than dedicated E2E (Docker/cloud self-heal). Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — all four threads were objective correctness/maintainability issues; two were settled by direct empirical testing of pnpm behavior rather than judgment calls. 🤖 Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4dad15948
ℹ️ 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".
| const packagesToDeny = [ | ||
| ...autoDeniedPackageNames, | ||
| ...removePlaceholderAllowBuildsEntries(lines), |
There was a problem hiding this comment.
When an existing pnpm-workspace.yaml has a pnpm placeholder such as sharp: set this to true or false and the refreshed allow-list now includes sharp, these placeholder names are only fed into packagesToDeny; insertAutoDeniedBuilds then filters them out because source.packages allows them, while promotedPackages stays empty. Callers therefore skip the rebuild command used for # dyad-auto-denied promotions, so a dependency that was already installed with its build script skipped can remain unrepaired after Dyad converts it to sharp: true.
Useful? React with 👍 / 👎.
| if (deniedBuilds.length === 0) { | ||
| return { deniedBuilds: [] }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 MEDIUM
Uncommitted workspace file when recordDeniedPnpmBuilds promotes without denying
In recordDeniedPnpmBuilds, when updatePnpmAllowBuildsConfigContentWithSource produces promotions (removing dyad-auto-denied lines) but no new denials, the file is written to disk (content !== existingContent) but the git commit is skipped (deniedBuilds.length === 0). This leaves an uncommitted pnpm-workspace.yaml change. In the normal app-run path this is harmless because commitPnpmAllowBuildsConfigIfChanged already committed promotions. However, in the self-heal path (called from selfHealDeniedPnpmBuilds without a prior commitPnpmAllowBuildsConfigIfChanged), a concurrent allow-list update could leave a dirty working tree. The scenario is narrow (self-heal + concurrent curated list update) and the uncommitted change is benign (a promotion), but it breaks the invariant that recordDeniedPnpmBuilds leaves the repo clean.
💡 Suggestion: After writing the file, also commit when updateResult.promotedPackages.length > 0 even if deniedBuilds is empty, or early-return before writing when deniedPackages is empty and promotedPackages would be handled by a separate commit.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Well-engineered PR that solves a real user-facing problem (pnpm 11 Issues Summary
MEDIUM detail: In 🟢 Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 764 passed, 5 failed, 6 flaky, 238 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/package_manager.spec.ts
|
1 participant
Summary
Auto-deny ignored pnpm builds so users never get stuck on
ERR_PNPM_IGNORED_BUILDS(pnpm 11 defaultsstrictDepBuildsto true, so a fresh install of an app with an unapproved-build dependency exits 1 — e.g. Rebuild on apps with custom install commands, or deploys/CI of exported repos).Implements
plans/pnpm-auto-deny-ignored-builds.md:node_modules/.modules.yaml(JSON on pnpm 10/11, block-YAML fallback) and recordpkg: false # dyad-auto-deniedentries inpnpm-workspace.yamloutside the Dyad-managed block, then commit — so plainpnpm installsucceeds everywhere (CI, Vercel, terminals).pkg: true, and callers run a best-effortpnpm rebuild <pkgs>(cross-shell-safe, works under cmd.exe) after install. User-authored entries are never touched.ERR_PNPM_IGNORED_BUILDSfrom custom install commands or the Capacitor/component-tagger flows, record denials, clearnode_modules, and retry once.<dyad-add-dependency>results note when build scripts were denied.pnpm:build-auto-deniedevents (bypassing non-Pro 10% sampling via thepnpm:build-prefix) feed curation of the remote allow-list.Tests
.modules.yamlfixture, rebuild-command quoting/filtering, output-parser forms, telemetry bypass.