Skip to content

Validate all-builds path and annotate format options#905

Closed
justin808 wants to merge 1 commit intomainfrom
codex/issue-782-run-all-builds-validation
Closed

Validate all-builds path and annotate format options#905
justin808 wants to merge 1 commit intomainfrom
codex/issue-782-run-all-builds-validation

Conversation

@justin808
Copy link
Copy Markdown
Member

Closes #782

Summary

  • add safeResolvePath validation for --all-builds --save-dir in runAllBuildsCommand
  • enforce --annotate requires YAML format in runAllBuildsCommand (matching normal run() behavior)
  • add regression tests through run() for both validation paths

Why

runAllBuildsCommand previously skipped validations that run() already applied in non---all-builds paths, creating inconsistent behavior and weaker path-safety guarantees.

Validation

  • yarn test test/package/configExporter.test.js

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 15, 2026

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 26 minutes and 11 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/issue-782-run-all-builds-validation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Feb 15, 2026

Greptile Summary

Added missing security and format validations to runAllBuildsCommand to match the behavior of the regular run() function, closing a consistency gap.

  • Path security: Applied safeResolvePath validation for --save-dir in --all-builds mode to prevent directory traversal attacks
  • Format enforcement: Added validation requiring YAML format when --annotate is used in --all-builds mode
  • Test coverage: Added two integration tests through run() verifying both validation paths work correctly and produce appropriate error messages

The changes ensure --all-builds has the same security guarantees and format constraints as the single-build export path.

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The changes are minimal, well-tested, and correctly apply existing validation patterns to a code path that was previously missing them. The implementation mirrors the validation logic from run() (lines 113-125), ensuring consistency. Both validations are covered by integration tests that verify proper error handling.
  • No files require special attention

Important Files Changed

Filename Overview
package/configExporter/cli.ts Added path traversal and annotate/format validations to runAllBuildsCommand matching existing run() behavior
test/package/configExporter.test.js Added integration tests through run() verifying both path traversal and annotate/format validations

Flowchart

flowchart TD
    A[run with --all-builds] --> B[runAllBuildsCommand]
    B --> C[Apply defaults]
    C --> D{saveDir provided?}
    D -->|Yes| E[safeResolvePath validation]
    D -->|No| F{annotate enabled?}
    E --> F
    F -->|Yes| G{format === yaml?}
    F -->|No| H[Load config file]
    G -->|No| I[Throw Error: requires YAML]
    G -->|Yes| H
    H --> J[Export each build]
    J --> K[Success]
    I --> L[Exit with code 1]
Loading

Last reviewed commit: bc56af2

@justin808
Copy link
Copy Markdown
Member Author

CI note: current Node Linting failure is the known Prettier workflow-file issue; unblock tracked in #908.

@justin808
Copy link
Copy Markdown
Member Author

Superseded by #914, which carries the same all-builds validation fix and also includes the Prettier unblocker for clean CI.

@justin808
Copy link
Copy Markdown
Member Author

Closing in favor of #914.

@justin808 justin808 closed this Feb 15, 2026
justin808 added a commit that referenced this pull request Mar 11, 2026
Supersedes #905. Closes #782.

## Summary
- carry forward the `runAllBuildsCommand` validation fixes from #905
  - save-dir path validation via `safeResolvePath`
  - annotate+format validation parity with `run()`
- include `.prettierignore` update for Claude workflow files (from #908)
so Node lint is not blocked by unrelated workflow formatting

## Validation
- yarn test test/package/configExporter.test.js

<!-- CURSOR_SUMMARY -->
---

> [!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 new
`CHANGELOG.md` section, 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_release` can now infer the target
version from `CHANGELOG.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 via `gh`.
> 
> Installer behavior is hardened and made more CI-friendly: adds
`SKIP=true` mode (and unified truthy parsing) to preserve existing
files, fixes transpiler config updates and Babel-only installs, and
ensures `package.json` retains 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 for `webpack-assets-manifest` merge 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 `--json`
is requested to keep stdout valid JSON. CI/workflows are updated for
Claude tooling permissions/tooling, and dependency constraints are
updated (e.g., allow `compression-webpack-plugin` v12).
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
d8b6e9f. 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

* **Bug Fixes**
  * Enforced YAML when using annotations with build exports.
* Added security checks to prevent path traversal in export save paths.

* **Tests**
* Added tests covering annotation format validation and path traversal
rejection.

* **Documentation**
  * Reflowed and aligned tables in the Node package API docs.

* **Chores**
* Added a GitHub authentication preflight for releases and updated
workflow checkout step.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Opus 4.6 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve runAllBuildsCommand consistency and testability

1 participant