Repository navigation
fix: run Vite and Vite+ commands selected by autoconfig - #540
petebacondarwin wants to merge 21 commits into
Conversation
|
LGTM! |
commit: |
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 221.0 ms | 2.4 ms | 217.6 ms | 225.6 ms |
| base | 222.0 ms | 2.3 ms | 217.6 ms | 225.7 ms |
Δ (PR vs base): ↓ -1.0 ms (-0.5%)
Measured with hyperfine on ubuntu-latest (3 warmup runs, 20 timed runs). CI numbers carry ±a few ms of runner jitter; treat small deltas as noise.
4c962ab to
61791ce
Compare
|
LGTM |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
297d2290.
This review, of 61791ce9
✅ Approved — No problems found. The native Vite lookup matches how the plugin is found, the subprocess handling is unchanged, and tests cover the missing-Vite and parent-folder cases.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. A minor changeset fits a pre-1.0 behaviour change.
🔒 Security: ✅ LGTM
No issues found. Vite is looked up only inside the project, not in parent folders.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
The architecture doc, the AGENTS.md line and the changeset all describe the native Vite fallback correctly.
Reviewed commit: 61791ce9
61791ce to
297d229
Compare
|
LGTM |
There was a problem hiding this comment.
Superseded by the review of
e0307a01.
This review, of 297d2290
✅ Approved — No problems found. The project's own Vite is found the same way the plugin is, process handling is unchanged, and the test update only repoints fixtures and adds a check for the install hint.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The repository's changeset rules use minor for behaviour changes before 1.0, so a minor changeset fits.
🔒 Security: ✅ LGTM
No issues found. The path to Vite is built from fixed parts inside the project, with no user input, and parent folders are not searched. Commands are still started with an argument array and no shell.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
No issues found.
resolveViteBinarylooks fornode_modules/vite/bin/vite.jsin the project folder, the same way plugins are found.- Process handling is unchanged, apart from comments.
- The new error for a missing Vite, and its install hint, have tests.
- Since the last review,
dev/index.test.tsonly points its fixtures atvite.jsinstead ofcf-viteand adds a check for the install hint.
⚡ Performance: ✅ LGTM
No issues found. The change adds one existsSync call per command, before a long-running process starts.
📝 Documentation: ✅ LGTM
The architecture doc, the AGENTS.md line and the changeset all describe the native Vite fallback correctly.
Reviewed commit: 297d2290
petebacondarwin
changed the title
There was a problem hiding this comment.
Superseded by the review of
6a3f45b1.
This review, of e0307a01
💬 Approved with comments — One WARNING: the detached POSIX process group can leave a dev server orphaned when the terminal closes or cf dies. There is also one INFO about the pre-release autoconfig pin, which must be replaced before shipping.
Findings: 1 warning · 1 info (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #540.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] Detached POSIX command group is orphaned when the terminal closes (SIGHUP) or cf dies, and a second Ctrl+C no longer escalates.
- **File:** packages/cli/src/lib/autoconfig.ts:197
## INFO (1)
### [Release] @cloudflare/autoconfig is pinned to a pkg.pr.new commit build; replace it with the released version before publishing cf.
- **File:** packages/cli/package.json:83
🚀 Release: 💬 1 info
A minor changeset is present; the pre-release autoconfig pin must be replaced before shipping.
Recommendation: 💬 Optional Nits
- [INFO] @cloudflare/autoconfig is pinned to a pkg.pr.new commit build; replace it with the released version before publishing cf. (
packages/cli/package.json:83)
View full review
Risk: Low. A minor changeset is right for a behaviour change before 1.0. @cloudflare/autoconfig is pinned to a pkg.pr.new commit build. The description plans to replace it with a registry release before cf ships, and Vite+ selection depends on that upstream change.
📋 Code Quality: ⚠️ 1 warning
Routing Vite/Vite+ through autoconfig is correct; the detached POSIX process group has lifecycle gaps.
Recommendation: 📝 Minor Comments
- [WARNING] Detached POSIX command group is orphaned when the terminal closes (SIGHUP) or cf dies, and a second Ctrl+C no longer escalates. (
packages/cli/src/lib/autoconfig.ts:197)
View full review
packages/cli/src/lib/autoconfig.ts:197:detached: trueputs the framework command in a new session (setsid). Only SIGINT and SIGTERM are relayed to it (lines 229-232).- If the terminal closes, cf gets SIGHUP and exits, but the detached group does not get the hangup.
- The same happens if cf crashes or is SIGKILLed.
- In both cases
vite/vp devkeeps running and holds its port. Before this change, the child shared cf's terminal session and would have received the hangup.
- After the first relayed signal,
controller.terminatereturns early (process.ts:75-81). A second Ctrl+C therefore no longer reaches the child or escalates; nothing happens until the 5s SIGKILL fallback fires. - SIGINT now also arms that 5s SIGKILL fallback.
runProjectCommandis shared with theinitinstall command, so installs are detached too.- The PATH handling is sound: the
PATH/Pathkey is found case-insensitively, and ancestor.bindirectories are prepended.
✅ All other checks passed
🔒 Security: ✅ LGTM
- Project commands are parsed with shell-quote, operators are rejected, and the command runs as an argv array with no shell.
CLOUDFLARE_CF_AUTHis set after the dotenv overrides are applied, and no new secrets enter the child's environment.- The Windows
taskkill.execall passes its arguments as an array and uses a numeric pid. - The new dependency overrides keep
blockExoticSubdeps, and the pre-release autoconfig URL is pinned to a full commit SHA.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The architecture doc, the AGENTS.md line and the changeset describe how cf now runs the Vite and Vite+ commands that autoconfig selects.
Reviewed commit: e0307a01
There was a problem hiding this comment.
Superseded by the review of
e36d270d.
This review, of 6a3f45b1
💬 Approved with comments — One WARNING remains: the detached POSIX process group can leave a dev server orphaned on SIGHUP or when cf is killed. There is also one INFO about the pre-release autoconfig pin, which must be replaced before shipping.
Findings: 1 warning · 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #540.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] Detached POSIX command group is orphaned when the terminal closes (SIGHUP) or cf dies, and a second Ctrl+C no longer escalates.
- **File:** packages/cli/src/lib/autoconfig.ts:197
## INFO (1)
### [Release] @cloudflare/autoconfig is pinned to a pkg.pr.new commit build; replace it with the released version before publishing cf.
- **File:** packages/cli/package.json:83
🚀 Release: 💬 1 info
The pre-release autoconfig pin must be replaced before shipping cf.
Recommendation: 💬 Optional Nits
- [INFO] @cloudflare/autoconfig is pinned to a pkg.pr.new commit build; replace it with the released version before publishing cf. (
packages/cli/package.json:83)
View full review
Risk: Low. A minor changeset is right for this behaviour change. @cloudflare/autoconfig is still pinned to a pkg.pr.new commit build, and Vite+ selection depends on that upstream change.
📋 Code Quality: ⚠️ 1 warning
The detached POSIX process group can still be orphaned, and a repeated signal does not escalate.
Recommendation: 📝 Minor Comments
- [WARNING] Detached POSIX command group is orphaned when the terminal closes (SIGHUP) or cf dies, and a second Ctrl+C no longer escalates. (
packages/cli/src/lib/autoconfig.ts:197)
View full review
packages/cli/src/lib/autoconfig.ts:197:detached: process.platform !== "win32"starts the framework command in its own session. cf relays only SIGINT and SIGTERM to it (lines 228-231).- If the terminal closes, cf gets SIGHUP and exits, but the group never gets the hangup.
- The same happens if cf crashes or is SIGKILLed.
- In both cases
viteorvp devkeeps running and holds its port.
controller.terminatereturns early onceterminationRequestedis set (process.ts:75-81). A second Ctrl+C therefore does nothing until the 5s SIGKILL fallback fires.runProjectCommandis also used forinitinstalls, so those run detached as well.
✅ All other checks passed
🔒 Security: ✅ LGTM
- Project commands still run as an argv array with no shell.
CLOUDFLARE_CF_AUTHis set after the dotenv overrides are applied.- The PATH prepend adds only the project's own and ancestor
node_modules/.bindirectories. - The
taskkill.execall takes an argv array and a numeric pid.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The architecture doc, the CLI AGENTS.md line and the changeset describe how cf now runs the Vite and Vite+ commands that autoconfig selects.
Reviewed commit: 6a3f45b1
There was a problem hiding this comment.
Superseded by the review of
7377dc6d.
This review, of e36d270d
✅ Approved — The earlier warning about orphaned POSIX command groups is fixed with SIGHUP relay, escalation on repeated signals, and exit-time cleanup. No new issues were found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. A minor changeset fits this behaviour change. @cloudflare/autoconfig is still pinned to a pre-release commit build and should be replaced with the released version before publishing cf.
🔒 Security: ✅ LGTM
No security issues found:
- Project commands still run as an argv array with no shell.
CLOUDFLARE_CF_AUTHis applied after the dotenv overrides.- The
PATHprepend adds only the project's and ancestors'node_modules/.bin. - Group kills target only the child's own process group.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
The detached POSIX command group now gets SIGHUP, and a repeated signal is escalated to SIGKILL. The group is also force-killed when cf exits first (packages/cli/src/lib/autoconfig.ts:216-249, packages/cli/src/lib/process.ts).
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The architecture doc, the CLI AGENTS.md and the changeset describe how cf now runs the Vite and Vite+ commands that autoconfig selects.
Reviewed commit: e36d270d
e36d270 to
7377dc6
Compare
There was a problem hiding this comment.
Superseded by the review of
32ae58a4.
This review, of 7377dc6d
✅ Approved — No new issues in the changes since the last review. Native Vite/Vite+ command selection, the process-tree shutdown and the credential boundaries look correct.
Not opened by the correctness review: 1 changed file it was meant to read
pnpm-workspace.yaml
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. A minor changeset fits this behaviour change. The @cloudflare/autoconfig commit-build pin must be replaced with the released version before cf ships.
🔒 Security: ✅ LGTM
No security problems found:
- Commands run as argv arrays with no shell.
CLOUDFLARE_CF_AUTHis applied after the dotenv overrides.- Group kills target only the child's own process group.
blockExoticSubdepsis still set.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
The changed code has no problems:
- Shutdown:
runProjectCommandand the process controller handle group signalling, escalation on repeated signals, exit-time cleanup and listener removal correctly. On Windows, the process tree is terminated withtaskkill. - Vite+: Native Vite+ support comes from the
vpcommand that autoconfig selects.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The changeset, docs/architecture.md and the CLI AGENTS.md describe the native command selection.
Reviewed commit: 7377dc6d
|
LGTM |
There was a problem hiding this comment.
Superseded by the review of
ae6403e4.
This review, of 32ae58a4
💬 Approved with comments — No defects found in the changes since the last review. One informational note: the autoconfig commit-build pin must be replaced with the released version before cf ships.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🚀 Release: 💬 1 info
One pre-release follow-up: replace the autoconfig commit-build pin
Recommendation: 💬 Optional Nits
- [INFO] Replace the autoconfig commit-build pin with the released version before publishing cf. (
packages/cli/package.json:83)
View full review
Risk: Low. A minor changeset fits this behaviour change. packages/cli/package.json:83 pins autoconfig to a pkg.pr.new commit build. Replace it with the released version before publishing cf.
✅ All other checks passed
🔒 Security: ✅ LGTM
- Commands run as argv arrays with no shell.
- Group kills target only the child's own process group.
- The scoped
pnpm-workspace.yamloverrides pin autoconfig's shared dependencies to registry versions.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
- Shutdown: group signalling, escalation on repeated signals, exit-time cleanup and listener removal are handled correctly.
- Environment:
CLOUDFLARE_CF_AUTHis set afteroptions.env. Local.bindirectories are prepended to the inherited PATH. - Plugin version check: it raises an error only for installed v2 versions older than the minimum.
⚡ Performance: ✅ LGTM
No performance concerns. The shutdown work runs only at shutdown, and the startup checks run once.
📝 Documentation: ✅ LGTM
The changeset, docs/architecture.md and the CLI AGENTS.md describe the native command selection.
Reviewed commit: 32ae58a4
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review: 1 findings. Posted 1 inline finding. |
There was a problem hiding this comment.
Superseded by the review of
9148e972.
This review, of ae6403e4
💬 Approved with comments — No defects found in the changes since the last review. One informational release note (replace the autoconfig commit-build pin) and one optional suggestion remain.
Findings: 1 info · 1 suggestion (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🚀 Release: 💬 1 info
Replace the autoconfig commit-build pin with the released version before publishing cf.
Recommendation: 💬 Optional Nits
- [INFO] Replace the autoconfig commit-build pin with the released version before publishing cf (
packages/cli/package.json:83)
View full review
Risk: Low. A minor changeset fits this behaviour change. packages/cli/package.json:83 still pins autoconfig to a pkg.pr.new commit build.
📋 Code Quality: 💬 1 suggestion
No correctness defects. One optional note about matching on upstream error text.
Recommendation: 💬 Optional Nits
- [SUGGESTION] The Vite config hint depends on the exact wording of an upstream BuildOutputError message (
packages/cli/src/commands/build/index.ts:98)
View full review
packages/cli/src/commands/build/index.ts:93-107: the Vite config hint is added only when the error message starts with an exact upstream string. If that wording changes, the hint quietly stops appearing.- Checked and fine:
assertProjectCommandsAvailableruns after thesetupNeededcheck in build and afterprepareProjectin dev. Configured projects are not blocked before their dependencies are installed.
✅ All other checks passed
🔒 Security: ✅ LGTM
Commands run as argv arrays with no shell, and group kills target only the child's own process group. The CLOUDFLARE_CF_AUTH marker is set after options.env, so callers cannot override it. Local .bin directories are prepended to the inherited PATH rather than replacing it.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The changeset, docs/architecture.md and packages/cli/AGENTS.md describe the native Vite and Vite+ command selection.
Reviewed commit: ae6403e4
ae6403e to
9148e97
Compare
There was a problem hiding this comment.
Superseded by the review of
790873d7.
This review, of 9148e972
💬 Approved with comments — Nothing changed since the last review introduces a defect. One optional suggestion remains about matching an upstream error message by its exact wording.
Findings: 1 suggestion
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 suggestion
No new defects; one earlier optional suggestion remains.
Recommendation: 💬 Optional Nits
- [SUGGESTION] Vite config hint depends on the exact wording of an upstream BuildOutputError message (
packages/cli/src/commands/build/index.ts:98)
View full review
packages/cli/src/commands/build/index.ts:93-107: the hint to addcloudflare()to the Vite config only appears when the error message starts with an exact upstream string. If@cloudflare/build-output-utilschanges that wording, the hint silently stops showing.
The overrides and tests added since the last review look correct. Shutdown handling (listener cleanup, escalation on a repeated signal, ignoring ESRCH, taskkill /T /F on Windows) holds up on inspection.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Medium. This change alters how project commands are selected and how subprocesses shut down. A minor changeset fits that change in behaviour. Tests cover build, dev, deploy, Preview and POSIX shutdown. As noted before, the autoconfig commit-build pin has to be replaced with the released version before cf is published.
🔒 Security: ✅ LGTM
Commands still run as argv arrays with no shell, and process-group kills only target the child's own group. Nothing changed since the last review alters the auth marker or the PATH handling.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
The child-process changes run once per command and do nothing that grows with load.
📝 Documentation: ✅ LGTM
The changeset, docs/architecture.md and packages/cli/AGENTS.md describe the native Vite and Vite+ command selection.
Reviewed commit: 9148e972
04658c6 to
7847b91
Compare
There was a problem hiding this comment.
Superseded by the review of
ed675695.
This review, of 7847b914
✅ Approved — The one change since the last review is the scaffold's remoteBindings: false default, which is correct and tested. The only open note is the temporary autoconfig pin, which the description tracks as a step before publishing.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Medium. The change affects how cf dev, cf build and build-before-deploy workflows choose and run commands, and it adds shutdown handling for subprocesses. The minor changeset fits the repository's pre-1.0 convention.
🔒 Security: ✅ LGTM
Keeping development bindings local in the scaffold is a safe default.
- Child commands are launched with argument arrays, so there is no shell injection.
- The auth environment variable is set only on the child's environment.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
The only change since the last review is packages/cli/src/commands/init/template.ts:87. The scaffolded vite.config.ts now calls cloudflare({ remoteBindings: false }), which matches the description and the autoconfig generators. init.test.ts has an updated test for it.
⚡ Performance: ✅ LGTM
No performance regressions.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: 7847b914
|
LGTM |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
LGTM |
There was a problem hiding this comment.
Superseded by the review of
4bf4c757.
This review, of ed675695
✅ Approved — Nothing changed since the last approved review introduces a defect. The only open items are the autoconfig pin, which the description already tracks as a step before publishing, and a stale line in a fixture README.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Medium. The change affects how cf dev, cf build and build-before-deploy workflows choose and run commands, and it adds shutdown handling for subprocesses.
- The changeset describes the user-facing behaviour, including the removal of the
--localoverride. - Replace the autoconfig commit-build pin with the npm release before publishing, as the checklist in the description says.
- Checks against earlier version tags were not performed, because tag history is not available here.
🔒 Security: ✅ LGTM
- Child commands are started as a command file plus an argument array, so there is no shell injection.
CLOUDFLARE_CF_AUTHis set only in the child's environment.- Process-group kills only reach the detached child's own group.
- On Windows,
taskkill.exeis run with an argument array and a numeric pid.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
Dev and build both check that project commands are available only after prepareProject, so setup can install missing tools first. Both also put autoconfig's nodeModulesPaths on PATH, and both fit with the shutdown and escalation logic in process.ts. Migration detects Vite from declared dependencies, so it does not depend on the delegate registry.
⚡ Performance: ✅ LGTM
No performance regressions.
📝 Documentation: ✅ LGTM
The docs/architecture.md and changeset updates match the behaviour. One line outside this diff is now out of date; the pull request body describes it.
Reviewed commit: ed675695
There was a problem hiding this comment.
Superseded by the review of
b10746ba.
This review, of 4bf4c757
✅ Approved — Since the last approved review, only a fixture README line changed, and it fixes the stale wording noted then. The autoconfig pin is the only open item, and the description already tracks it as a step before publishing.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Medium. The change affects how cf dev, cf build and build-before-deploy workflows choose and run commands.
- The changeset describes the change, including the removal of the
--localoverride. - Replace the autoconfig commit-build pin with the npm release before publishing, as the description's checklist says.
- Earlier version tags were not checked, because tag history is not available here.
🔒 Security: ✅ LGTM
taskkill.exeis run with an argument array and a numeric pid.- Process-group kills only reach the detached child's own group.
- Child commands are started as a command file plus an argument array, so there is no shell injection.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
Dev and build check that commands are available only after prepareProject, and both put autoconfig's nodeModulesPaths on PATH. Process shutdown handling cleans up its handlers, escalates repeated interrupts and falls back correctly on Windows. Migration detects Vite from the declared dependencies.
⚡ Performance: ✅ LGTM
No performance regressions. The synchronous taskkill call only runs on shutdown, and it has a timeout.
📝 Documentation: ✅ LGTM
The fixture README, docs/architecture.md and the changeset match the new Vite and Vite+ command routing.
Reviewed commit: 4bf4c757
|
🤖 I've done a thorough review of your PR. Native Vite/Vite+ command delegation and shutdown handling were added.
|
There was a problem hiding this comment.
Superseded by the review of
f099a08d.
This review, of b10746ba
💬 Approved with comments — Only the fixture and lockfile changed since the last approved review, and those changes are correct. The single remaining note is the documented autoconfig pin, which the description already plans to replace before publishing.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🚀 Release: 💬 1 info
Medium risk. The changeset fits the minor-release convention. The autoconfig pin still needs replacing before publishing.
Recommendation: 💬 Optional Nits
- [INFO] The autoconfig dependency is still pinned to a commit build. Replace it with the npm release before publishing cf. (
packages/cli/package.json:83)
View full review
Risk: Medium. The change alters how cf dev, cf build and build-before-deploy workflows choose and run Vite and Vite+ commands.
- The changeset describes the change for users, including the removal of the
--localoverride. packages/cli/package.json:83still pins@cloudflare/autoconfigto a commit build. Before publishing, replace it with the npm release and regenerate the lockfile, as the description's checklist says.- Earlier version tags were not checked, because tag history is not available here.
✅ All other checks passed
🔒 Security: ✅ LGTM
No issues found.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
Dev and build check that commands are available only after project setup, and both resolve executables through autoconfig's nodeModulesPaths. Shutdown handling removes its handlers when done, escalates repeated interrupts and falls back correctly on Windows. On POSIX the detached child runs in its own session (setsid), so inheriting the terminal's stdin does not cause job-control stops.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
The fixture README, docs/architecture.md and the changeset match the new Vite and Vite+ command routing.
Reviewed commit: b10746ba
| "@clack/core": "1.5.1", | ||
| "@clack/prompts": "^1.8.1", | ||
| "@cloudflare/autoconfig": "0.12.1", | ||
| "@cloudflare/autoconfig": "https://pkg.pr.new/cloudflare/workers-sdk/@cloudflare/autoconfig@6e6223620e1967986d5c3a722691d436a8401010", |
There was a problem hiding this comment.
ℹ️ INFO · Release
The autoconfig dependency is still pinned to a commit build. Replace it with the npm release before publishing cf.
@cloudflare/autoconfig still points at a commit build (6e6223620e). As the description's checklist says, replace it with the npm release that contains workers-sdk#16803 before publishing cf. Then regenerate the lockfile and remove any scoped overrides that are no longer needed.
|
Review: 1 findings. 🤖 I've done a thorough review of your PR. Native Vite/Vite+ command routing and shutdown handling were added. Posted 1 inline finding on delegated Ctrl+C force-killing Vite. |
There was a problem hiding this comment.
Superseded by the review of
4a4a4d1e.
This review, of f099a08d
💬 Approved with comments — One WARNING: global cf now ignores a SIGINT sent only to its own process, so a programmatic interrupt can leave the delegated cf running. The autoconfig commit-build pin, which the description already plans to replace, is the only other note.
Findings: 1 warning · 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #540.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] Global cf ignores a SIGINT sent only to its own pid, so programmatic interrupts never reach the delegated local cf, and the caller hangs.
- **File:** packages/cli/src/lib/delegate.ts:297
## INFO (1)
### [Release] `@cloudflare/autoconfig` is still pinned to a pkg.pr.new commit build. Replace it with the npm release before publishing cf.
- **File:** packages/cli/package.json:83
🚀 Release: 💬 1 info
Medium release risk. The autoconfig commit-build pin still needs to be replaced before publishing.
Recommendation: 💬 Optional Nits
- [INFO]
@cloudflare/autoconfigis still pinned to a pkg.pr.new commit build. Replace it with the npm release before publishing cf. (packages/cli/package.json:83)
View full review
Risk: Medium. The change alters how cf dev, cf build and build-before-deploy workflows choose and run Vite and Vite+ commands, and how command trees are shut down. The changeset marks this as a minor release and calls out the removed cf-vite --local override. packages/cli/package.json:83 still pins @cloudflare/autoconfig to a commit build. Before publishing, replace it with the npm release, as the description's checklist says.
📋 Code Quality: ⚠️ 1 warning
The delegated SIGINT handler ignores interrupts that are sent only to the global cf process.
Recommendation: 📝 Minor Comments
- [WARNING] Global cf ignores a SIGINT sent only to its own pid, so programmatic interrupts never reach the delegated local cf, and the caller hangs. (
packages/cli/src/lib/delegate.ts:297)
View full review
packages/cli/src/lib/delegate.ts:297-300: onSigInt is now a no-op. Because a handler is installed, Node no longer exits on SIGINT, and the signal is not forwarded to the local cf either. This works when the terminal sends SIGINT to the whole foreground process group. A SIGINT aimed at the global cf pid alone is lost, though. Examples are kill -INT, timeout -s INT, a harness calling child.kill('SIGINT'), or an IDE stop button. In those cases both processes keep running and the caller hangs. The new test in project-command-process.test.ts signals the local pid directly, so it does not cover this case. The SIGTERM relay and the handler cleanup are correct.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found. The child environment and credential handling are unchanged. node_modules/.bin directories are added to PATH only for project tools that cf dev and cf build already run. Process-group kills target only the child's own group. The Windows taskkill call passes its arguments as an array, not through a shell.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
The changeset, docs/architecture.md and the fixture README match the new Vite and Vite+ command routing and the SIGINT delegation behaviour.
Reviewed commit: f099a08d
|
LGTM |
There was a problem hiding this comment.
Superseded by the review of
910dca5d.
This review, of 4a4a4d1e
💬 Approved with comments — Non-interactive callers now get SIGINT relayed, which fixes the earlier warning. The one remaining note (INFO) is the documented TTY-only case where a SIGINT sent just to the global cf PID is not forwarded.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
The earlier SIGINT warning is fixed for non-interactive callers; a narrow TTY-only gap remains as a note.
Recommendation: 💬 Optional Nits
- [INFO] With a TTY attached, a SIGINT sent only to the global cf PID is not relayed, so the delegated cf keeps running. (
packages/cli/src/lib/delegate.ts:303)
View full review
packages/cli/src/lib/delegate.ts:298-305: when none of stdin, stdout or stderr is a TTY,onSigIntnow relays SIGINT to the child. SIGTERM is always relayed. Thedelegated-programmaticcase inproject-command-process.test.tschecks that the local cf and the tool both shut down cleanly.- Remaining gap: the TTY check cannot tell whether the SIGINT came from the terminal's process group. A SIGINT sent only to the global PID while cf has a TTY is still dropped. The description documents this trade-off.
process.tshandles process-group kills, SIGKILL escalation and the Windowstaskkillfallback consistently.
✅ All other checks passed
🚀 Release: ✅ LGTM
Overall risk: Medium. The change alters how cf selects and runs project dev and build commands, including builds that run before a deploy, and adds command-tree shutdown handling. The minor changeset follows the repository's pre-1.0 convention. The @cloudflare/autoconfig commit-build pin should be replaced with the npm release before publishing, as the description's checklist says.
🔒 Security: ✅ LGTM
taskkill.exeis called with an argument array, not through a shell.process.kill(-pid)targets only the child's own process group.- The child environment and credential isolation are unchanged.
- The
pnpm-workspace.yamloverrides pin registry versions.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
The new work is limited to shutdown-time process-group kills and a one-off synchronous taskkill on Windows. None of it is on a hot path.
📝 Documentation: ✅ LGTM
The changeset, docs/architecture.md and the fixture README match the new Vite and Vite+ command routing and the SIGINT delegation behaviour.
Reviewed commit: 4a4a4d1e
|
LGTM |
There was a problem hiding this comment.
💬 Approved with comments — No blocking or warning issues. Two informational notes remain: a SIGINT sent only to the global cf PID is not relayed when a terminal is attached (documented), and the autoconfig commit-build pin has to be replaced before publishing.
Findings: 2 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🚀 Release: 💬 1 info
Medium release risk. Before publishing, the autoconfig commit-build pin needs to be replaced with the npm release.
Recommendation: 💬 Optional Nits
- [INFO] Before publishing, replace the autoconfig commit-build pin with the npm release and regenerate the lockfile. (
packages/cli/package.json:83)
View full review
Overall risk: Medium. The change alters how cf selects and runs project dev and build commands, including builds that run before a deploy, and adds command-tree shutdown handling. The minor changeset follows the repository's pre-1.0 convention.
packages/cli/package.json:83 still pins @cloudflare/autoconfig to a commit build. Before publishing, replace it with the npm release and regenerate the lockfile, as the description's checklist says.
📋 Code Quality: 💬 1 info
One informational note: the documented TTY-only SIGINT trade-off in delegate.ts.
Recommendation: 💬 Optional Nits
- [INFO] With a TTY attached, a SIGINT sent only to the global cf PID is not relayed, so the delegated cf keeps running. This trade-off is documented. (
packages/cli/src/lib/delegate.ts:305)
View full review
packages/cli/src/lib/delegate.ts:298-306: when any of stdin, stdout or stderr is a TTY,onSigIntdoes not relay SIGINT. It relies on the terminal delivering Ctrl+C to both CLIs, so a SIGINT sent only to the global cf PID leaves the delegated cf running. SIGTERM is always relayed, and both the code comment and the description name it as the signal for programmatic shutdown.- The rest of the shutdown handling looks consistent:
- In
process.ts, a SIGKILL override clears the pending force-kill timer,ESRCHis tolerated, and the Windowstaskkillfallback is guarded. - On POSIX,
runProjectCommandstarts the command in its own process group (detached) and kills it on exit. - The dev and build paths check that the selected tools exist after
prepareProject, so setup gets a chance to install them first.
- In
✅ All other checks passed
🔒 Security: ✅ LGTM
taskkill.exe is called with an argument array, not through a shell. Process-group kills target only the child's own group. Child environment and credential isolation are unchanged.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
The new work happens only at shutdown: process-group kills, plus a single synchronous taskkill.exe call on Windows with a 5-second timeout. None of it is on a hot path.
📝 Documentation: ✅ LGTM
The updated comment in delegate.ts agrees with the description and with docs/architecture.md on how SIGINT is handled when cf delegates to a local install.
Reviewed commit: 910dca5d
|
LGTM |
Run the Vite or Vite+ dev/build commands selected by autoconfig instead of requiring the internal
cf-viteexecutable. Cover native fallbacks and workflows that build before deploying, while preserving canonical framework commands, Pages detection, migration checks, unsupported-project guards, argument/mode forwarding, Preview context, and credential isolation.Resolve executables through autoconfig's ordered application-to-workspace
nodeModulesPaths, and validate missing tools and plugin compatibility at the dev/build execution boundary after setup. Let migration and framework setup install or upgrade tools before reanalyzing the project; dry-run setup skips execution. Let Vite evaluate dynamic configurations and custom config files. If a successful Vite build produces no root Build Output, suggest checkingcloudflare(); preserve other validation errors and document the upstream error-message coupling.Keep the Vite plugin's binding defaults unchanged. Starters use
cloudflare(), and users can explicitly disable remote bindings in their plugin configuration. Removingcf-viteremoves its optional--localoverride without changing the default. Preserve migration's Vite detection by reading declared dependencies independently of the delegate registry.Forward shutdown signals to the command tree and attempt cleanup when cf exits early. POSIX uses a separate process group, forwards SIGHUP, and escalates repeated interrupts. With inherited terminal streams, global-to-local cf delegation relies on the terminal's SIGINT delivery, so the first Ctrl+C allows the local tool to finish cleanup. Non-interactive delegation relays SIGINT, and SIGTERM is always relayed. With a terminal attached, programmatic shutdown should use SIGTERM: a SIGINT sent only to the global cf PID cannot be distinguished from terminal delivery. Windows retains console Ctrl+C delivery and uses tree termination for SIGTERM. Cleanup cannot run if cf receives SIGKILL.
Release order: workers-sdk#16803 is merged. Release autoconfig containing that change, then release this cf change, then remove
cf-vitein workers-sdk#16759.6e6223620e) with the npm release containing workers-sdk#16803, regenerate the lockfile, and remove any scoped overrides no longer needed.cf-viteremoval PR.Validation (Node.js 22.23.4, pnpm 12.18.0):
pnpm check: all 7 tasks passed, including generation, builds, types, lint, and formatting. Generated files have no diff;git diff --checkpassed.cf dev --mode test -- --port 0 --host 127.0.0.1selectedvp dev, and fetching/varreturnedTest var. The fixture declares Vite+ directly so autoconfig selects its CLI rather than the CLI-less Vite core alias.