Supersede #905: all-builds validation fix with lint unblock#914
Supersede #905: all-builds validation fix with lint unblock#914
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds runtime validations to the config exporter CLI (save-dir path safety and annotate+format compatibility), adds tests for those validations, inserts a GitHub auth check into the release preflight, reformats a docs table, and updates a CI checkout version and a small comment in runner.rb. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds missing security and input validation to
Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A["run(args)"] --> B["parseArguments(args)"]
B --> C{options.allBuilds?}
C -->|Yes| D["runAllBuildsCommand(options)"]
C -->|No| E["Continue to other handlers"]
D --> F["findAppRoot() + setupNodePath()"]
F --> G["applyDefaults(options)"]
G --> H{"saveDir set?"}
H -->|Yes| I["safeResolvePath(appRoot, saveDir)"]
H -->|No| J{"annotate && format != yaml?"}
I -->|Path outside root| K["Throw: Path traversal detected"]
I -->|Valid| J
J -->|Yes| L["Throw: Annotation requires YAML"]
J -->|No| M["Load config + export builds"]
K --> N["catch: console.error + return 1"]
L --> N
style I fill:#e6f3ff,stroke:#0066cc
style J fill:#e6f3ff,stroke:#0066cc
style K fill:#ffe6e6,stroke:#cc0000
style L fill:#ffe6e6,stroke:#cc0000
Last reviewed commit: ebcfd0b |
justin808
force-pushed
the
codex/supersede-905-all-builds-validation
branch
from
ebcfd0b to
f10e908
Compare
justin808
added
codex
|
@claude review this PR |
justin808
added
defer
Code ReviewOverall this is a well-structured PR. SIGTERM forwarding in Runner, the manifest LoadResult struct, the new bundler utility functions, and the security-safe CORS defaults are all solid improvements. The release automation expansion is substantial and generally well-done. A few issues to address: Bug: actions/checkout@v6 does not exist. The latest stable is v4. This will break the Claude Code Review workflow on every PR. Should be reverted to @v4. Bug: spec/dummy/package-lock.json has a yalc artifact committed. The lockfile contains a .yalc/shakapacker entry at version 9.6.0-rc.3, which is a local development artifact from yalc package linking. Only registry-published packages should appear in a committed lockfile. Release automation: GitHub auth not checked during preflight. verify_gh_auth is only called inside sync_github_release_after_publish, which runs after both npm and RubyGems OTP prompts. A missing or misconfigured gh token is only discovered at the very end of a multi-minute interactive flow. Adding verify_gh_auth(gem_root: gem_root) to the existing unless dry_run preflight block alongside verify_npm_auth would surface this upfront. Minor: SIGTERM race in Runner. Between trap(TERM) being installed and child_pid = spawn(...) returning, there is a narrow window where an incoming SIGTERM finds child_pid nil and raises SignalException instead of forwarding to the child. The window is tiny and the nil initialisation partially mitigates it, but a comment explaining the intent would help. Minor: safeResolvePath result discarded in runAllBuildsCommand. The call is used only for its throw-on-invalid side effect. A brief inline comment clarifying this is intentional (not a no-op) would help future readers. Nit: CHANGELOG beta-to-stable rename mixed into this PR. v9.3.4-beta.0 is renamed to v9.3.4 here. Per CLAUDE.md this kind of cleanup is better in its own PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
Bug: actions/checkout@v6 does not exist. The latest stable release is v4. Using @v6 here will cause the workflow to fail on every PR it runs on.
| uses: actions/checkout@v6 | |
| uses: actions/checkout@v4 |
| puts "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ" | ||
| puts "PRE-FLIGHT CHECKS" | ||
| puts "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ" | ||
| verify_npm_auth |
There was a problem hiding this comment.
Consider adding verify_gh_auth(gem_root: gem_root) here alongside verify_npm_auth. Currently GitHub auth is checked inside sync_github_release_after_publish, which runs after the npm and RubyGems OTP prompts. A missing or misconfigured gh token is only discovered at the very end of a multi-minute interactive flow, forcing the maintainer to run sync_github_release manually.
| verify_npm_auth | |
| verify_npm_auth | |
| verify_gh_auth(gem_root: gem_root) |
| }, | ||
| ".yalc/shakapacker": { | ||
| "version": "9.5.0", | ||
| "version": "9.6.0-rc.3", |
There was a problem hiding this comment.
Bug: yalc development artifact in committed lockfile. The .yalc/shakapacker entry at 9.6.0-rc.3 is a local dev artifact produced by yalc for local package-link testing. It should not be committed — the lockfile should only reference registry-published packages. This entry will cause incorrect dependency resolution for anyone running npm install from the repo.
| Dir.chdir(@app_path) do | ||
| system(env, *cmd) | ||
| child_pid = nil | ||
| trap("TERM") do |
There was a problem hiding this comment.
There is a narrow race window here: the TERM trap is installed before child_pid is assigned, so a signal arriving between trap and the spawn return would hit the else branch and raise SignalException. child_pid is already initialised to nil, so the handler is logically correct once spawn completes — but it may be worth a comment explaining that the else path is the legitimate pre-spawn case, not an error state, to prevent someone later from removing the raise thinking it's unreachable.
package/configExporter/cli.ts
Outdated
|
|
||
| // Validate paths for security in all-builds mode | ||
| if (resolvedOptions.saveDir) { | ||
| safeResolvePath(appRoot, resolvedOptions.saveDir) |
There was a problem hiding this comment.
The return value of safeResolvePath is intentionally discarded here — it is called only to validate the path and throw on directory traversal, consistent with the same pattern in run(). A short comment would prevent future readers from treating this as dead code:
| safeResolvePath(appRoot, resolvedOptions.saveDir) | |
| // Validates for path traversal; throws on unsafe paths. Return value intentionally discarded. | |
| safeResolvePath(appRoot, resolvedOptions.saveDir) |
justin808
force-pushed
the
codex/supersede-905-all-builds-validation
branch
from
a9d39ad to
1985635
Compare
package/configExporter/cli.ts
Outdated
| const resolvedOptions = applyDefaults(options) | ||
|
|
||
| // Validate paths for security in all-builds mode | ||
| if (resolvedOptions.saveDir) { |
There was a problem hiding this comment.
Nit: This if guard is always true in --all-builds mode. applyDefaults() unconditionally sets saveDir to DEFAULT_EXPORT_DIR whenever neither --stdout nor --output is present (lines ~450-456), and --all-builds never sets either of those flags.
The validation is still correct and harmless, but it reads as though saveDir might be absent here when it can't be. Either drop the guard and validate unconditionally, or add a comment clarifying that applyDefaults ensures it's always set — consistent with how the same // Set by applyDefaults comment is used on targetDir below.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ReviewThe core fix is correct and well-targeted. Specific observations:
Overall this is a clean, focused fix. The redundant guard is the only thing worth tidying before merge. |
Review: All-Builds Validation FixThe core logic is sound - adding path-traversal and annotate/format validation to Two issues worth addressing: 1. Missing
Fix: 2. Noisy / inconsistent comment (cli.ts line 717) The Everything else looks good: the |
package/configExporter/cli.ts
Outdated
|
|
||
| // Validate paths for security in all-builds mode | ||
| if (resolvedOptions.saveDir) { | ||
| // Validates for path traversal; throws on unsafe paths. Return value intentionally discarded. |
There was a problem hiding this comment.
This comment is unnecessary and inconsistent — the identical safeResolvePath calls in run() (lines 114 and 117) have no such annotation. The function's own JSDoc in pathValidation.ts already documents that it throws on unsafe paths. Consider removing the comment to match the established pattern.
| // Validates for path traversal; throws on unsafe paths. Return value intentionally discarded. | |
| safeResolvePath(appRoot, resolvedOptions.saveDir) |
## Summary - updates `spec/dummy/Gemfile.lock` so the local path gem is locked at stable `shakapacker 9.6.0` instead of `9.6.0.rc.3` - keeps the Bundler/Ruby support-policy discussion out of `9.x`; that broader work is tracked in #960 for `10.0.0` ## Test plan - [ ] CI passes - [ ] Lockfile-only diff reviewed ## Notes - no application code changes - no Bundler baseline change in this PR --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add clarifying comment for intentionally discarded safeResolvePath return value - Add verify_gh_auth to pre-flight checks to fail fast before OTP prompts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justin808
force-pushed
the
codex/supersede-905-all-builds-validation
branch
from
b05bb10 to
8fa03fb
Compare
|
Code Review: The fix is correct. run() early-returns into runAllBuildsCommand() before validations run, so --all-builds silently skipped path-traversal and annotate+format checks. The fix is right. Key observations: (1) Duplicate validation in run() lines 111-125 and runAllBuildsCommand() lines 715-726 - a cross-reference comment would help. (2) output path not validated in runAllBuildsCommand - fine since --all-builds skips --output, but a comment explaining why would help. (3) safeResolvePath called for side effect only - comment is adequate. (4) Tests look good. (5) .prettierignore has Temporarily comment without a tracking issue - consider linking one. (6) verify_gh_auth pre-flight in release.rake is a good improvement. Overall: bug is correctly fixed, tests added, scope is tight. Looks good to merge. |
| throw new Error( | ||
| "Annotation requires YAML format. Use --no-annotate or --format=yaml." | ||
| ) | ||
| } |
There was a problem hiding this comment.
This validation block mirrors the one in run() (lines 121-125). Since run() early-exits to runAllBuildsCommand() before those lines execute, the duplication is necessary — but it creates a maintenance risk. A cross-reference comment (e.g. // Keep in sync with validation in run()) or a small shared helper would make the relationship explicit and reduce the chance of one copy drifting.
package/configExporter/cli.ts
Outdated
| // Validate paths for security in all-builds mode | ||
| if (resolvedOptions.saveDir) { | ||
| // Validates for path traversal; throws on unsafe paths. Return value intentionally discarded. | ||
| safeResolvePath(appRoot, resolvedOptions.saveDir) |
There was a problem hiding this comment.
The run() path also validates resolvedOptions.output (line 113-114), but that is intentionally omitted here since --all-builds always writes to a directory and never uses --output. A short comment noting that omission would save the next reader a trip to run() to verify it was not overlooked.
- Fix actions/checkout@v6 to @v4 (v6 does not exist) - Add clarifying comment about TERM trap race window in runner.rb - Remove always-true saveDir guard in --all-builds mode, add comments explaining applyDefaults guarantee and --output omission - Add cross-reference comment for duplicated validation block Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
The original PR had actions/checkout@v6, which doesn't exist — the latest stable release is v4. This was flagged by the automated review and has been fixed to @v4.
### Summary Adds the v9.7.0 changelog section with release notes for all user-visible changes since v9.6.1: - **Added**: rspack v2 support (PR #975) - **Fixed**: Config exporter path traversal and annotation format validation (PR #914) - **Fixed**: `webpack-subresource-integrity` v5 named export handling (PR #978, fixes #972) Version diff links at the bottom of the file are updated accordingly. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] Update CHANGELOG file ### Other Information Non-user-visible PRs (#920, #965, #970, #971, #977, #979, #981, #982) were intentionally excluded per changelog policy. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only change updating `CHANGELOG.md`; no runtime code or dependency changes are introduced in this PR. > > **Overview** > Adds a new `v9.7.0` section to `CHANGELOG.md` documenting user-visible changes (rspack v2 support and two fixes around config export security/validation and `webpack-subresource-integrity` v5 exports). > > Updates the compare links at the bottom so `[Unreleased]` now compares from `v9.7.0`, and adds the new `[v9.7.0]` tag link. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8942a43. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added rspack v2 support * **Bug Fixes** * Improved security and validation handling <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
### Summary Adds the v9.7.0 changelog section with release notes for all user-visible changes since v9.6.1: - **Added**: rspack v2 support (PR #975) - **Fixed**: Config exporter path traversal and annotation format validation (PR #914) - **Fixed**: `webpack-subresource-integrity` v5 named export handling (PR #978, fixes #972) Version diff links at the bottom of the file are updated accordingly. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] Update CHANGELOG file ### Other Information Non-user-visible PRs (#920, #965, #970, #971, #977, #979, #981, #982) were intentionally excluded per changelog policy. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only change updating `CHANGELOG.md`; no runtime code or dependency changes are introduced in this PR. > > **Overview** > Adds a new `v9.7.0` section to `CHANGELOG.md` documenting user-visible changes (rspack v2 support and two fixes around config export security/validation and `webpack-subresource-integrity` v5 exports). > > Updates the compare links at the bottom so `[Unreleased]` now compares from `v9.7.0`, and adds the new `[v9.7.0]` tag link. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8942a43. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added rspack v2 support * **Bug Fixes** * Improved security and validation handling <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Supersedes #905. Closes #782.
Summary
runAllBuildsCommandvalidation fixes from Validate all-builds path and annotate format options #905safeResolvePathrun().prettierignoreupdate for Claude workflow files (from Ignore Claude workflow files in Prettier checks #908) so Node lint is not blocked by unrelated workflow formattingValidation
Note
Medium Risk
Touches release automation, installer conflict handling, and process execution/manifest logic; while well-tested, these paths are user-facing and can impact publishing and developer workflows if edge cases are missed.
Overview
This release bumps Shakapacker to
v9.6.0, stamps a newCHANGELOG.mdsection, and updates release documentation to make changelog-first releases the default (including correct prerelease header formats).Release automation is significantly expanded in
rakelib/release.rake:create_releasecan now infer the target version fromCHANGELOG.md, performs stricter version/tag policy validation (with an explicit override), runs dry runs in a temporary git worktree, refreshes dummy app lockfiles, and automatically creates/updates the matching GitHub release viagh.Installer behavior is hardened and made more CI-friendly: adds
SKIP=truemode (and unified truthy parsing) to preserve existing files, fixes transpiler config updates and Babel-only installs, and ensurespackage.jsonretains the exact shakapacker dependency source/version requested. Dev server defaults no longer ship permissive CORS headers (users must opt in), entrypoint discovery now ignores dotfiles, and manifest handling/error messages are improved (including avoiding ENOENT forwebpack-assets-manifestmerge mode).Node/package-facing improvements include new bundler-agnostic exports (
getBundler,getCssExtractPlugin*,get*Plugin,isWebpack/isRspack) with typings/docs, extra validation in config exporter all-builds mode, and routing log output to stderr when--jsonis requested to keep stdout valid JSON. CI/workflows are updated for Claude tooling permissions/tooling, and dependency constraints are updated (e.g., allowcompression-webpack-pluginv12).Written by Cursor Bugbot for commit d8b6e9f. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores