Repository navigation
Support assets-only deployments without delegation - #520
Conversation
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 189.7 ms | 4.8 ms | 178.3 ms | 196.4 ms |
| base | 198.8 ms | 13.2 ms | 187.2 ms | 237.1 ms |
Δ (PR vs base): ↓ -9.1 ms (-4.6%)
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.
|
Review: 1 findings. 🤖 I've done a thorough review of your PR.
|
51d2398 to
ed94034
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
bcff1c9f.
This review, of ed940342
💬 Approved with comments — Solid, well-tested change. There is one WARNING: the project root is accepted as the static assets directory, which could publish source and dotenv files. There is also one SUGGESTION: progress output ignores --quiet.
Findings: 1 warning · 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.
🔧 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 #520.
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] The project root is explicitly accepted as the static assets directory, so source files, config and dotenv files could be published as public assets
- **File:** packages/cli/src/commands/deploy/static-assets.ts:82
## SUGGESTION (1)
### [Code Quality] Static-assets progress output ignores `--quiet`, unlike the delegated build path
- **File:** packages/cli/src/commands/deploy/static-assets.ts:96
📋 Code Quality: ⚠️ 1 warning · 1 suggestion
The project root is accepted as the assets directory; progress lines ignore --quiet.
Recommendation: 📝 Minor Comments
- [WARNING] The project root is explicitly accepted as the static assets directory, so source files, config and dotenv files could be published as public assets (
packages/cli/src/commands/deploy/static-assets.ts:82) - [SUGGESTION] Static-assets progress output ignores
--quiet, unlike the delegated build path (packages/cli/src/commands/deploy/static-assets.ts:96)
View full review
- WARNING:
packages/cli/src/commands/deploy/static-assets.ts:81-88: thesourceDirectory !== rootexemption letscf deploy .(orcf deploy "") use the project root as the assets directory. The root holdscloudflare.config.ts,package.json,.env*,.gitandnode_modules. UnlesswriteAssetsfilters these out, they become publicly served assets, and copying the root into.cloudflare/output/...(which sits inside the root) may also recurse. No test covers this case. - SUGGESTION:
packages/cli/src/commands/deploy/static-assets.ts:95-107: thelog.section,log.messageandlog.successprogress lines are always written, even with--quiet. The delegated build runs silently when--quietis set.
✅ All other checks passed
🚀 Release: ✅ LGTM
The changeset is present. Risk: low.
🔒 Security: ✅ LGTM
No security issues beyond the root-directory concern noted in the code quality section.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: ed940342
|
LGTM |
There was a problem hiding this comment.
Superseded by the review of
8146fa7d.
This review, of bcff1c9f
💬 Approved with comments — Solid, well-tested change. The --quiet note is fixed. One WARNING remains: cf deploy . accepts the project root as the static assets directory, which could publish source, config and dotenv files.
Findings: 1 warning
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 #520.
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] The project root is still accepted as the static assets directory, so `cf deploy .` may publish source, config and dotenv files as public assets
- **File:** packages/cli/src/commands/deploy/static-assets.ts:83
📋 Code Quality: ⚠️ 1 warning
The --quiet note is fixed. Accepting the project root as the assets directory is still open.
Recommendation: 📝 Minor Comments
- [WARNING] The project root is still accepted as the static assets directory, so
cf deploy .may publish source, config and dotenv files as public assets (packages/cli/src/commands/deploy/static-assets.ts:83)
View full review
WARNING: packages/cli/src/commands/deploy/static-assets.ts:82-89
- The
sourceDirectory !== rootexemption meanscf deploy .(orcf deploy "") passes both checks. writeAssets({ root, sourceDirectory: root })then copies the whole project, includingcloudflare.config.ts,package.json,.env*,.gitandnode_modules, and the.cloudflare/outputdirectory it is writing into.- Unless
writeAssetsfilters these out, they are served publicly on a production deploy. - No test covers the root case.
The --quiet handling is fixed (static-assets.ts:96-112) and covered by a new test.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset is present.
🔒 Security: ✅ LGTM
The isWithin checks and realpath resolution correctly stop symlink escapes into .cloudflare/output. The exposure risk of accepting the project root as the assets directory is covered in the code quality section.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: bcff1c9f
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
There was a discussion about whether this should also be a named (not positional) parameter. But I think that it makes sense for this since it would be the primary target of the command.
We won't support a positional worker javascript file - that will require a configuration file.
|
LGTM |
There was a problem hiding this comment.
Superseded by the review of
77dc4b6f.
This review, of 8146fa7d
💬 Approved with comments — Well-tested change with no blocking or warning-level issues. One INFO note: no test covers deploying the project root.
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
One INFO note: no test covers the project-root (cf deploy .) branch.
Recommendation: 💬 Optional Nits
- [INFO] No test covers the
sourceDirectory === rootexemption (cf deploy .) (packages/cli/src/commands/deploy/static-assets.ts:86)
View full review
The earlier WARNING about accepting the project root as the assets directory is no longer raised. Deploying the root needs an explicit cf deploy ., which matches the existing assets.directory: "." behaviour and its .assetsignore exclusions.
INFO: no test covers the sourceDirectory === root branch (static-assets.ts:85-92). That leaves the claim that writeAssets skips .cloudflare when it copies the root untested in this pull request.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset is present.
🔒 Security: ✅ LGTM
Path handling resolves the directory with realpath and rejects locations inside .cloudflare/output, so a symlink cannot point the source into the output directory. The change adds no process spawning and builds no shell commands.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: 8146fa7d
8146fa7 to
77dc4b6
Compare
There was a problem hiding this comment.
Superseded by the review of
31cb54a2.
This review, of 77dc4b6f
💬 Approved with comments — Well-tested change with no blocking or warning-level issues. One INFO note: no test covers deploying the project root.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
One INFO note: no test covers the project-root (cf deploy .) branch.
Recommendation: 💬 Optional Nits
- [INFO] No test covers the
sourceDirectory === rootexemption (cf deploy .) (packages/cli/src/commands/deploy/static-assets.ts:86)
View full review
The earlier WARNING about accepting the project root as the assets directory is no longer raised. Deploying the root needs an explicit cf deploy ., which matches the existing assets.directory: "." behaviour and its .assetsignore exclusions.
INFO: no test covers the sourceDirectory === root branch (static-assets.ts:85-92). That leaves the claim that writeAssets skips .cloudflare when it copies the root untested in this pull request.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset is present.
🔒 Security: ✅ LGTM
Path handling resolves the directory with realpath and rejects locations inside .cloudflare/output, so a symlink cannot point the source into the output directory. The change adds no process spawning and builds no shell commands.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: 77dc4b6f — same changes as 8146fa7d, so this is that commit's review (no new review was run; line numbers are as of 8146fa7d)
|
LGTM! |
|
LGTM! |
There was a problem hiding this comment.
Superseded by the review of
10e89083.
This review, of 31cb54a2
💬 Approved with comments — No blocking or warning-level issues. One INFO note remains: no test covers the project-root (cf deploy .) exemption.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
One INFO note: the project-root exemption has no test.
Recommendation: 💬 Optional Nits
- [INFO] No test covers the
sourceDirectory === rootexemption (cf deploy .) (packages/cli/src/commands/deploy/static-assets.ts:86)
View full review
The code has not changed since the last review. The positional directory added to deploy, previews deploy and workers versions create, the formatConfigIssues extraction, and the metadata and generated JSON updates are consistent and have tests.
INFO: no test covers the sourceDirectory === root exemption (static-assets.ts:85-92), so this pull request never exercises the claim that writeAssets skips .cloudflare when it copies the project root.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset is present.
🔒 Security: ✅ LGTM
Path handling resolves the directory and refuses locations inside .cloudflare/output. The change adds no process spawning and builds no shell commands.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
The clarification in docs/architecture.md about the assets-only source config exception matches the code.
Reviewed commit: 31cb54a2
31cb54a to
10e8908
Compare
There was a problem hiding this comment.
Superseded by the review of
db02872e.
This review, of 10e89083
✅ Approved — No findings. The earlier INFO note about the cf deploy . project-root path is resolved: a new test checks that .cloudflare is excluded from the uploaded assets.
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 changeset is present.
🔒 Security: ✅ LGTM
Path handling resolves the directory and refuses locations inside .cloudflare/output. The change adds no process spawning and builds no shell commands.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
🔄 Since last review: 1 finding resolved, 0 preserved, 0 new
The earlier note about the sourceDirectory === root exemption (static-assets.ts) is resolved. The new test in assets.test.ts runs cf deploy . with .cloudflare state and stale Build Output in place. It checks that the build delegate is not called and that no /.cloudflare/ entries end up in the uploaded manifest or in the written assets.
On the discussion thread about deploying the project root: the author's explanation matches the code. The directory form keeps the semantics of assets.directory: ".", with .assetsignore controlling exclusions, and .cloudflare is excluded explicitly.
The new handling of a positional directory in deploy, previews deploy and workers versions create looks correct and has tests.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
The documentation updates match the code.
Reviewed commit: 10e89083
|
Review: 1 findings. Adds assets-only directory uploads for deploy, Preview deploy, and version creation. Posted 1 inline suggestion for missing Container-rejection coverage. |
There was a problem hiding this comment.
Superseded by the review of
bca49278.
This review, of db02872e
✅ Approved — No findings. Since the last approved review, only tests were added, and they cover the new assets-only directory deployment path.
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 changeset is present.
🔒 Security: ✅ LGTM
The only change since the last review is a test file. It raises no security concerns.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
Only assets.test.ts changed since the last review. The new tests set up their state correctly and check the behaviour they name. The earlier note about the cf deploy . project-root exemption stays resolved: the test confirms .cloudflare is excluded from the manifest and from the written assets.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
The documentation still matches the code.
Reviewed commit: db02872e
db02872 to
bca4927
Compare
|
@jamesopstad Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
There was a problem hiding this comment.
✅ Approved — No findings. Since the last approved review, only tests were added, and they cover the new assets-only directory deployment path.
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 changeset is present.
🔒 Security: ✅ LGTM
The only change since the last review is a test file. It raises no security concerns.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
Only assets.test.ts changed since the last review. The new tests set up their state correctly and check the behaviour they name. The earlier note about the cf deploy . project-root exemption stays resolved: the test confirms .cloudflare is excluded from the manifest and from the written assets.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
The documentation still matches the code.
Reviewed commit: bca49278 — same changes as db02872e, so this is that commit's review (no new review was run; line numbers are as of db02872e)
|
LGTM! |
Deploy any prebuilt static asset directory directly with
cf deploy [directory]and the equivalent Preview deployment and version creation commands.The directory form creates assets-only Build Output, reuses optional assets-only Worker configuration, and skips project build delegation. Projects containing a configured Worker entrypoint or Container continue to omit the directory and use the delegated build path.