Fix issue #20: safer dump output and diff mode handling#21
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 (1)
WalkthroughAdds a CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ConfigLoader
participant ConfigFactory
participant DumpRunner
User->>CLI: run diff --left=a.js --right=b.js --mode=development
CLI->>ConfigLoader: loadConfigFile(a.js, mode="development")
ConfigLoader->>ConfigFactory: call exported({}, { mode: "development" })
ConfigFactory-->>ConfigLoader: return configA
CLI->>ConfigLoader: loadConfigFile(b.js, mode="development")
ConfigLoader->>ConfigFactory: call exported({}, { mode: "development" })
ConfigFactory-->>ConfigLoader: return configB
CLI->>CLI: compare configA vs configB
CLI-->>User: print diff summary
User->>CLI: run dump config.js [--no-warn-sensitive] [--environment=staging]
CLI->>ConfigLoader: loadConfigFile(config.js, mode=(parsed.mode||"production"))
ConfigLoader->>ConfigFactory: call exported({}, { mode })
ConfigFactory-->>ConfigLoader: return config
CLI->>DumpRunner: run dump with config, warnSensitive flag, environment label
DumpRunner-->>User: emit YAML/JSON (header sanitized), maybe warn about sensitive output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: 2ce46a0186
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review: Fix issue #20 — safer dump output and diff mode handlingOverall this is a solid, well-scoped PR. The security hardening (YAML header injection fix, Notable issues1.
2. Dump warning has no suppression path (see inline comment on The warning fires on every Minor
What looks good
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
555-567:⚠️ Potential issue | 🟠 MajorValidate all
--envkeys before mutatingprocess.env.Line 556 can throw after earlier entries in the same list have already been written at Line 566, so
run(["dump", ..., "--env=NODE_ENV=production", "--env=INVALID-NAME=value"])returns2but leavesNODE_ENVbehind for the rest of the process. The same partial-apply bug also affects invalid keys coming frombuild.environment.💡 Suggested fix
function applyEnvVariables(entries: string[]): () => void { const parsedEntries = entries.map((entry) => { const delimiterIndex = entry.indexOf("="); if (delimiterIndex <= 0) { throw new Error(`Invalid --env value: ${entry}. Expected KEY=VALUE.`); } return { key: entry.slice(0, delimiterIndex), value: entry.slice(delimiterIndex + 1), }; }); + + parsedEntries.forEach(({ key }) => { + if (!isValidEnvVarName(key)) { + throw new Error( + `Invalid --env key: ${key}. Expected a shell-style env var name (e.g. NODE_ENV).`, + ); + } + }); const previousValues = new Map<string, string | undefined>(); parsedEntries.forEach(({ key, value }) => { - if (!isValidEnvVarName(key)) { - throw new Error( - `Invalid --env key: ${key}. Expected a shell-style env var name (e.g. NODE_ENV).`, - ); - } - if (!previousValues.has(key)) { previousValues.set(key, process.env[key]); } process.env[key] = value; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 555 - 567, Validate all candidate env keys before mutating process.env: first iterate parsedEntries (and any build.environment inputs) and run isValidEnvVarName on every key, throwing if any invalid are found, then in a separate loop perform previousValues.set(key, process.env[key]) and assign process.env[key] = value; reference parsedEntries, isValidEnvVarName, previousValues and process.env to locate the logic to split validation and mutation so no partial updates occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/cli.ts`:
- Around line 555-567: Validate all candidate env keys before mutating
process.env: first iterate parsedEntries (and any build.environment inputs) and
run isValidEnvVarName on every key, throwing if any invalid are found, then in a
separate loop perform previousValues.set(key, process.env[key]) and assign
process.env[key] = value; reference parsedEntries, isValidEnvVarName,
previousValues and process.env to locate the logic to split validation and
mutation so no partial updates occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8897ff09-1331-48b2-9082-e69a5436c1f0
📒 Files selected for processing (13)
README.mddocs/cli-reference.mddocs/input-formats.mdsrc/buildConfigFile.tssrc/cli.tssrc/configCleaner.tssrc/configSerializer.tssrc/envVar.tssrc/fileWriter.tssrc/objectIntrospection.tssrc/yamlSerializer.tstest/cli.integration.test.jstest/yamlSerializer.test.js
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Misleading error message for build config env keys
- Updated env key validation messaging to use source-agnostic wording and added an integration test covering invalid build-config environment keys.
Or push these changes by commenting:
@cursor push 7adce3bd88
Preview (7adce3bd88)
diff --git a/src/cli.ts b/src/cli.ts
--- a/src/cli.ts
+++ b/src/cli.ts
@@ -555,7 +555,7 @@
parsedEntries.forEach(({ key, value }) => {
if (!isValidEnvVarName(key)) {
throw new Error(
- `Invalid --env key: ${key}. Expected a shell-style env var name (e.g. NODE_ENV).`,
+ `Invalid environment key: ${key}. Expected a shell-style env var name (e.g. NODE_ENV).`,
);
}
diff --git a/test/cli.integration.test.js b/test/cli.integration.test.js
--- a/test/cli.integration.test.js
+++ b/test/cli.integration.test.js
@@ -386,10 +386,36 @@
expect(code).toBe(2);
expect(errorSpy).toHaveBeenCalledWith(
- expect.stringContaining("Invalid --env key: INVALID-NAME"),
+ expect.stringContaining("Invalid environment key: INVALID-NAME"),
);
});
+ test("dump --build reports invalid environment keys without --env wording", () => {
+ const configPath = path.join(tempDir, "webpack.config.js");
+ const buildConfig = path.join(tempDir, "pack-config-diff-builds.yml");
+
+ fs.writeFileSync(configPath, "module.exports = { mode: 'production' }\n", "utf8");
+ fs.writeFileSync(
+ buildConfig,
+ [
+ "builds:",
+ " prod:",
+ ` config: "${configPath}"`,
+ " environment:",
+ " INVALID-NAME: value",
+ ].join("\n"),
+ "utf8",
+ );
+
+ const code = run(["dump", "--build=prod", `--config-file=${buildConfig}`]);
+
+ expect(code).toBe(2);
+ expect(errorSpy).toHaveBeenCalledWith(
+ expect.stringContaining("Invalid environment key: INVALID-NAME"),
+ );
+ expect(errorSpy).not.toHaveBeenCalledWith(expect.stringContaining("Invalid --env key"));
+ });
+
test("dump --annotate injects inline docs for known keys", () => {
const configPath = path.join(tempDir, "webpack.config.js");
fs.writeFileSync(configPath, "module.exports = { mode: 'production' }\n", "utf8");You can send follow-ups to this agent here.
|
Replying to #21 (comment): addressed the selected actionable items in f44ba0a. Specifically, dump now has independent |
|
Replying to #21 (comment): thanks for the walkthrough. No additional action needed from that summary/status comment beyond the concrete items already handled in f44ba0a. |
Review: Fix issue #20 — safer dump output and diff mode handlingOverall this is a well-structured PR. The changes are logically grouped, tests cover the new behavior, and documentation is updated in sync. A few observations: Solid fixes
Concerns / questionsNODE_ENV info message is not suppressed by Two minor issues are left as inline comments. |
Fix the no-useless-escape lint error in test/cli.integration.test.js that was failing CI. Add lefthook for pre-commit hooks (inspired by shakacode/react_on_rails) that run ESLint, Prettier, and trailing newline checks in parallel on staged files before each commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20fa89cb8a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lefthook/check-trailing-newlines`:
- Around line 18-20: The pre-commit hooks are running in parallel which causes
concurrent file modifications (the trailing-newlines script writes to files and
then git adds them), producing race conditions and index lock errors; update the
lefthook configuration by setting the top-level or relevant hook group option
parallel: false (change the existing parallel: true to parallel: false) so
eslint, prettier and trailing-newlines run serially when stage_fixed: true is
used, preventing concurrent git index writes.
In `@bin/lefthook/lint-branch`:
- Line 10: The script stores git paths in plain strings (changed_files,
js_files, format_files) and later expands them unquoted, which breaks on
filenames with spaces or names starting with '-'; update the git commands to
emit NUL-delimited output (use git diff --name-only -z or git ls-files -z), read
that into arrays with mapfile -d $'\0' -t (e.g., mapfile -d $'\0' -t
changed_files < <(git diff --name-only -z "$base_branch"...HEAD)), then iterate
and pass paths using quoted array expansions like "${changed_files[@]}",
"${js_files[@]}", and "${format_files[@]}" so no word-splitting or
option-parsing occurs; replace any uses of unquoted $js_files/$format_files in
loops and commands with the quoted array forms.
In `@package.json`:
- Line 64: The current POSIX-only "postinstall" npm script using shell built-ins
should be replaced with a cross-platform Node-based check: create a small Node
script (e.g., scripts/install-lefthook.js) that checks for the presence of
"lefthook.yml" and a .git directory (using fs.existsSync or fs.stat) and, if
present, spawns "npx lefthook install" (using child_process.spawnSync or
execSync) and exits gracefully on failure; then update the "postinstall" entry
to run "node ./scripts/install-lefthook.js" instead of the POSIX command so
installation works on Windows, macOS, and Linux while preserving the original
behavior around lefthook.yml and npx lefthook install.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26485055-b98a-4cbc-8f75-76eb207d634d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.gitignoreREADME.mdbin/lefthook/check-trailing-newlinesbin/lefthook/lint-branchdocs/cli-reference.mddocs/input-formats.mdlefthook.ymlpackage.jsonsrc/cli.tstest/cli.integration.test.js
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- lefthook.yml
- README.md
- test/cli.integration.test.js
- docs/cli-reference.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/input-formats.md
- src/cli.ts
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Bash-only
postinstallbreaks Windows consumers' installs- Removed the bash-only
postinstalland moved lefthook installation into a cross-platform Node-basedpreparecommand gated onlefthook.ymland.gitpresence.
- Removed the bash-only
- ✅ Fixed: Dev-only lefthook scripts shipped to npm consumers
- Narrowed the published
filesentry frombin/tobin/pack-config-diffso internal lefthook helper scripts are excluded from npm packages.
- Narrowed the published
Or push these changes by commenting:
@cursor push 767f8d2480
Preview (767f8d2480)
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -29,7 +29,7 @@
},
"files": [
"dist/",
- "bin/"
+ "bin/pack-config-diff"
],
"engines": {
"node": ">=16"
@@ -60,8 +60,7 @@
},
"scripts": {
"build": "tsc",
- "prepare": "npm run build",
- "postinstall": "test -f lefthook.yml && test -d .git && npx lefthook install || true",
+ "prepare": "npm run build && node -e \"const fs=require('node:fs');const cp=require('node:child_process');if(fs.existsSync('lefthook.yml')&&fs.existsSync('.git')){cp.execSync('npx lefthook install',{stdio:'inherit'});}\"",
"test": "jest",
"lint": "eslint . && prettier --check .",
"lint:fix": "eslint --fix . && prettier --write .",You can send follow-ups to this agent here.
|
Replying to #21 (comment): addressed in 0139d67. The build NODE_ENV label stderr note is now gated by |
|
Replying to #21 (comment) and #21 (comment): treating these as summary/superseded feedback for this pass. The selected correctness issues are addressed in 0139d67, and the remaining minor points are intentionally deferred. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 744-748: The fallback NODE_ENV warning is incorrectly tied to
parsed.warnSensitive; change the condition so the build fallback warning is
shown independently of parsed.warnSensitive. Update the if in the block
referencing resolvedEnvironment.source === "build-node-env" to not include
parsed.warnSensitive (or replace it with a new dedicated flag like
parsed.warnFallbackLabel if you want explicit opt-out), ensuring the
console.error about using build.name NODE_ENV envLabel is emitted when
resolvedEnvironment.source === "build-node-env" regardless of the
--no-warn-sensitive flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5859a68f-eb6f-43ac-a2a6-cbbc6527cc76
📒 Files selected for processing (5)
bin/lefthook/lint-branchlefthook.ymlpackage.jsonsrc/cli.tstest/cli.integration.test.js
✅ Files skipped from review due to trivial changes (2)
- lefthook.yml
- test/cli.integration.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- bin/lefthook/lint-branch
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0139d67cdb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Developer tooling scripts leak into published npm package
- I removed the consumer-facing postinstall lifecycle hook by moving lefthook installation into prepare and restricted published files to
bin/pack-config-diffsobin/lefthook/*is no longer packaged.
- I removed the consumer-facing postinstall lifecycle hook by moving lefthook installation into prepare and restricted published files to
- ✅ Fixed: Flag overloads unrelated warning and informational message
- I decoupled the build NODE_ENV label note from
warnSensitiveso--no-warn-sensitivenow suppresses only the sensitive-output warning while preserving the environment-label diagnostic.
- I decoupled the build NODE_ENV label note from
Or push these changes by commenting:
@cursor push ab5bda8874
Preview (ab5bda8874)
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -29,7 +29,7 @@
},
"files": [
"dist/",
- "bin/"
+ "bin/pack-config-diff"
],
"engines": {
"node": ">=16"
@@ -60,8 +60,7 @@
},
"scripts": {
"build": "tsc",
- "prepare": "npm run build",
- "postinstall": "node -e \"const fs=require('fs');const cp=require('child_process');if(fs.existsSync('lefthook.yml')&&fs.existsSync('.git')){try{cp.execSync('npx --no-install lefthook install',{stdio:'inherit'});}catch{}}\"",
+ "prepare": "npm run build && node -e \"const fs=require('fs');const cp=require('child_process');if(fs.existsSync('lefthook.yml')&&fs.existsSync('.git')){try{cp.execSync('npx --no-install lefthook install',{stdio:'inherit'});}catch{}}\"",
"test": "jest",
"lint": "eslint . && prettier --check .",
"lint:fix": "eslint --fix . && prettier --write .",
diff --git a/src/cli.ts b/src/cli.ts
--- a/src/cli.ts
+++ b/src/cli.ts
@@ -741,7 +741,7 @@
try {
const resolvedEnvironment = resolveBuildEnvironmentLabel(build, parsed);
const envLabel = resolvedEnvironment.label;
- if (resolvedEnvironment.source === "build-node-env" && parsed.warnSensitive) {
+ if (resolvedEnvironment.source === "build-node-env") {
console.error(
`[pack-config-diff] Using build "${build.name}" NODE_ENV="${envLabel}" as dump environment label. Pass --environment to override.`,
);
diff --git a/test/cli.integration.test.js b/test/cli.integration.test.js
--- a/test/cli.integration.test.js
+++ b/test/cli.integration.test.js
@@ -487,7 +487,7 @@
expect(errorOutput).not.toContain("Warning: dump output without --clean");
});
- test("dump --build --no-warn-sensitive suppresses NODE_ENV label note", () => {
+ test("dump --build keeps NODE_ENV label note when suppressing sensitive warning", () => {
const configPath = path.join(tempDir, "webpack.config.js");
const buildConfig = path.join(tempDir, "pack-config-diff-builds.yml");
@@ -528,9 +528,7 @@
expect(suppressedCode).toBe(0);
errorOutput = errorSpy.mock.calls.map((args) => String(args[0])).join("\n");
- expect(errorOutput).not.toContain(
- 'Using build "dev" NODE_ENV="development" as dump environment',
- );
+ expect(errorOutput).toContain('Using build "dev" NODE_ENV="development" as dump environment');
});
test("dump --annotate injects inline docs for known keys", () => {You can send follow-ups to this agent here.
|
Posting rationale for skipped summary/superseded issue comments:
|
1 participant
Addresses issue #20 by adding diff mode control for JS/TS config factories, warning when dump runs without --clean, and validating --env key names. It fixes resolveRealPath path stitching, hardens YAML metadata headers against newline injection, and makes build-matrix NODE_ENV fallback explicit at runtime. The change also deduplicates constructor-name/env-var helper logic and updates README/CLI/input-format docs to document the new behavior and safety notes. Validation: npm test and npm run lint.
Note
Medium Risk
Touches core CLI execution paths for loading configs and dumping outputs, which could change behavior in CI/local workflows; changes are well-scoped and covered by new tests but affect config evaluation and file writing.
Overview
Adds new CLI controls and safety hardening for config evaluation and exports.
diffanddumpnow accept--modeto set theargv.modepassed to JS/TS function-export configs (defaultproduction), whiledumpkeeps--environmentas metadata-only.Improves dump security/robustness by warning when running without
--clean(suppressible via--no-warn-sensitive), validating--envkeys before mutatingprocess.env, and sanitizing YAML header metadata to prevent newline/header injection. Also fixesFileWriter.resolveRealPathpath stitching, clarifies trusted build-matrix path/env expansion behavior, deduplicates helper logic (envVar,objectIntrospection), and addslefthookpre-commit/pre-push automation plus updated docs/tests.Written by Cursor Bugbot for commit b88875e. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests