chore: audit guardrails and observability (#290)#291
chore: audit guardrails and observability (#290)#291flyingrobots wants to merge 8 commits intomainfrom
Conversation
Add ESLint 9 flat config with recommended rules and node globals. Fix two no-useless-escape errors in regex character classes and remove a stale eslint-disable directive in lens.js.
Replace shell-interpolated execSync with execFileSync array form to eliminate potential command injection via user-supplied SHA argument.
Add ESLint step to CI workflow before tests. Configure v8 coverage provider in vitest and add test:coverage script for local use.
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughReplaces unsafe shell-invoking git calls with execFileSync across CLI and core, adds a flat ESLint 9 config and CI lint step, enables Vitest v8 coverage with a coverage npm script and CI artifact upload, simplifies several regex literals and imports, and extends tests for commit-range sanitization. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI command
participant Node as Node process
participant Git as Git
participant FS as Filesystem
CLI->>Node: call execFileSync('git', ['log', '--pretty=format:%B', <args>])
Note right of Node: sanitizeGitArg validates args\n(rejects leading '-' and unsafe chars)
Node->>Git: spawn with argv array
Git-->>Node: returns stdout/stderr
Node->>FS: write/read diff / coverage files (as needed)
Node-->>CLI: return trimmed output / error code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~27 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 33-40: The CI workflow currently runs the "Install dependencies"
step with npm ci every run causing redundant downloads; update the job that sets
up Node (actions/setup-node@v4) to enable its built-in npm cache by adding the
cache input (e.g., cache: 'npm') so the "Install dependencies" step using npm ci
re-uses cached node_modules across runs and speeds up subsequent "Run linter"
and "Run tests" steps; locate the actions/setup-node@v4 invocation in the
workflow and add the cache configuration there.
- Around line 36-40: CI currently runs only "npm test" so Vitest's lcov reporter
and test coverage are never produced; update the workflow to invoke the
coverage-enabled test script (e.g., run the existing "test:coverage" npm script
or run "npm test" with the --coverage flag) so Vitest emits the ./coverage lcov
output, then add a step to upload the ./coverage directory (using the
upload-artifact action) and/or publish it to your coverage service; optionally
add coverage.thresholds in vitest.config.js so Vitest can fail the job when
thresholds are not met.
- Around line 36-40: Replace all remaining uses of execSync with safe,
argumentized process calls and input validation: specifically change the git
remote URL retrieval in merge.js (the execSync on line ~19) and each execSync in
context.js (the calls around lines ~89, ~118, ~135) to use
child_process.execFileSync or child_process.spawnSync with the command and an
args array (and set shell: false), and ensure any values interpolated into
arguments are validated or whitelisted rather than concatenated into a shell
string; mirror the approach used in processCommitCmd to pass args as arrays and
avoid shell invocation so no shell-injection vectors remain.
In `@eslint.config.js`:
- Around line 14-16: Update the ESLint rule configuration for no-unused-vars in
the rules object: change its severity from 'warn' to 'error' and add a
varsIgnorePattern to mirror the existing argsIgnorePattern so intentionally
prefixed variables are ignored; specifically update the 'no-unused-vars' entry
to include argsIgnorePattern: '^_' and varsIgnorePattern: '^_' (keeping the
existing options shape) so CI fails on real unused-variable issues while
allowing underscore-prefixed exceptions.
- Around line 1-2: The ESLint imports in eslint.config.js (js from '@eslint/js'
and globals from 'globals') must be explicit devDependencies; update
package.json to add "@eslint/js" and "globals" under devDependencies (pin
versions compatible with your installed eslint v9), then run your package
manager to install them (e.g., npm install --save-dev `@eslint/js` globals or yarn
add -D `@eslint/js` globals) so the imports in eslint.config.js resolve reliably.
In `@package.json`:
- Line 24: Add the missing `@vitest/coverage-v8` devDependency (pin it to the same
major version as the installed vitest) so the test:coverage script can use
provider: 'v8' from vitest.config.js; remove the ad-hoc --exclude flags from the
"test:coverage" script in package.json and instead add the excluded test paths
under coverage.exclude in vitest.config.js so excluded tests don't run but are
properly omitted from coverage instrumentation.
In `@src/cli/commands.js`:
- Around line 298-301: In processCommitCmd, validate the incoming sha before
calling git log to prevent git option injection: run git rev-parse --verify on
the sha (like the pattern used in src/epoch.js) via execFileSync and
reject/throw or return an error if rev-parse fails, then proceed to call
execFileSync('git', ['log', '-1', '--format=%B', sha], ...); this ensures
malformed refs (including ones starting with '-') cannot be interpreted as CLI
options.
In `@vitest.config.js`:
- Around line 10-14: The coverage config is missing coverage.include so
unimported source files are omitted; update the Vitest config object (the
coverage section) to add a coverage.include array that explicitly lists your
source patterns (e.g., 'src/**/*.ts' or similar) so coverage runs include all
intended source files even if not imported during tests, and keep existing keys
provider, reporter, and reportsDirectory unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci.ymleslint.config.jspackage.jsonsrc/cli/commands.jssrc/lens.jssrc/remote.jssrc/validators.jsvitest.config.js
💤 Files with no reviewable changes (1)
- src/lens.js
| "scripts": { | ||
| "test": "vitest run", | ||
| "test:watch": "vitest", | ||
| "test:coverage": "vitest run --coverage --exclude test/contracts.integration.test.js --exclude test/content.test.js --exclude test/version.test.js", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read package.json to verify the devDependencies
find . -name "package.json" -type f -not -path "*/node_modules/*" | head -5Repository: flyingrobots/git-mind
Length of output: 79
🏁 Script executed:
# Read the package.json to check devDependencies
cat -n package.json | grep -A 10 "devDependencies"Repository: flyingrobots/git-mind
Length of output: 272
🏁 Script executed:
# Check if `@vitest/coverage-v8` exists in node_modules
ls -la node_modules/@vitest/ 2>/dev/null | head -20 || echo "No `@vitest` directory found or node_modules not available"Repository: flyingrobots/git-mind
Length of output: 47
🏁 Script executed:
# Check for vitest.config.js or similar config files
find . -name "vitest.config.*" -o -name "vite.config.*" | grep -v node_modulesRepository: flyingrobots/git-mind
Length of output: 83
🏁 Script executed:
# Check the actual test:coverage script line in detail
sed -n '20,30p' package.jsonRepository: flyingrobots/git-mind
Length of output: 486
🏁 Script executed:
# Read the vitest.config.js to check coverage configuration
cat -n vitest.config.jsRepository: flyingrobots/git-mind
Length of output: 488
🏁 Script executed:
# Check vitest documentation info - try to see what's in vitest's own config
# and whether coverage is pre-included or requires a separate package
rg "coverage" vitest.config.jsRepository: flyingrobots/git-mind
Length of output: 118
🏁 Script executed:
# Let's also check if there are any test files that might give us hints about coverage setup
ls -la test/ 2>/dev/null | head -20Repository: flyingrobots/git-mind
Length of output: 1345
🏁 Script executed:
# Double-check: Does --exclude affect coverage instrumentation in vitest?
# Let me search for any documentation or hints in the codebase about this
rg "coverage.exclude" . --type js --type json 2>/dev/null | head -10Repository: flyingrobots/git-mind
Length of output: 47
🏁 Script executed:
# Verify the full context around test:coverage script to see lines 20-30 properly
sed -n '20,35p' package.jsonRepository: flyingrobots/git-mind
Length of output: 585
@vitest/coverage-v8 is missing from devDependencies — the test:coverage script will fail on any clean install.
The vitest.config.js explicitly specifies provider: 'v8' (line 11), which requires its own separate package. The interactive install prompt that Vitest offers does not fire in scripted contexts like npm run or CI/CD pipelines — you must declare the dependency explicitly.
The --exclude flags in the script prevent test files from executing, which incidentally prevents them from contributing to coverage reports (can't instrument code that never runs). However, this is an indirect mechanism. The proper way to exclude files from coverage scope is coverage.exclude in the config, not relying on test execution filters.
🔧 Add the missing package
"devDependencies": {
"eslint": "^9.0.0",
+ "@vitest/coverage-v8": "^3.0.0",
"prettier": "^3.0.0",
"vitest": "^3.0.0"
}Pin @vitest/coverage-v8 to the same major version as vitest to avoid internal mismatches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 24, Add the missing `@vitest/coverage-v8` devDependency
(pin it to the same major version as the installed vitest) so the test:coverage
script can use provider: 'v8' from vitest.config.js; remove the ad-hoc --exclude
flags from the "test:coverage" script in package.json and instead add the
excluded test paths under coverage.exclude in vitest.config.js so excluded tests
don't run but are properly omitted from coverage instrumentation.
There was a problem hiding this comment.
@coderabbitai This was addressed in the same batch of commits:
@vitest/coverage-v8added as devDependency (package.json line 38, pinned to^3.2.4matching vitest^3.0.0major)coverage.include: ['src/**/*.js']added to vitest.config.js
Re: --exclude flags → coverage.exclude: This is a false positive. The --exclude flags on vitest run prevent those test files from executing (they're integration/slow tests). coverage.exclude would only prevent instrumentation of source files but still run the tests. These serve different purposes — the current approach is correct.
Evidence:
# package.json line 38
"@vitest/coverage-v8": "^3.2.4",
# vitest.config.js lines 10-14
coverage: {
provider: 'v8',
reporter: ['text', 'lcov'],
reportsDirectory: './coverage',
include: ['src/**/*.js'],
},
# test:coverage script excludes integration tests from running, not from instrumentation
"test:coverage": "vitest run --coverage --exclude test/contracts.integration.test.js --exclude test/content.test.js --exclude test/version.test.js"
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the vitest.config.js content to confirm coverage.include is present
cat vitest.config.js
Length of output: 343
@flyingrobots You're absolutely right, and I apologize for the false positive. I've confirmed:
✅ @vitest/coverage-v8@^3.2.4 is present in devDependencies (line 38), correctly pinned to match vitest's major version
✅ coverage.include: ['src/**/*.js'] is configured in vitest.config.js (line 14)
✅ Your --exclude usage is correct: those flags prevent slow integration tests from executing during coverage runs, which is exactly what you want. Using coverage.exclude would still run the tests but skip instrumenting source files—a completely different purpose.
My review was working from incomplete context. Thank you for the detailed correction with evidence. The configuration is solid.
/\_/\
( o.o ) ← pedantic reviewer
> ^ < eating humble pie
/| |\
(_| |_)
Escalate no-unused-vars to error with varsIgnorePattern for _-prefixed vars. Add @eslint/js and globals as explicit devDependencies per ESLint 9 best practices. Remove 8 unused imports and prefix 2 unused variables.
Add @vitest/coverage-v8 as devDependency, configure coverage.include for src/**/*.js, add coverage step with artifact upload to CI workflow, and enable npm cache in setup-node for faster installs.
…290) Add git rev-parse --verify guard to reject malformed refs (including option-injection via leading dashes) before passing SHA to git log. Mirrors the validation pattern already used in src/epoch.js.
|
@coderabbitai Thanks for the thorough review! Addressing all 8 comments across commits Resolved (6 fixes):
Disputed (1 false positive) — remaining This was analyzed during the audit and intentionally scoped out. Here's the evidence:
The CI nit (npm cache): Also applied — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4636 🏁 Script executed: Length of output: 5621
|
| File | Line | Verdict |
|---|---|---|
src/merge.js:19 |
✅ SAFE — hardcoded command, no interpolation | |
src/context.js:89 |
✅ SAFE — hardcoded command, no interpolation | |
src/context.js:136 |
✅ SAFE — SHA validated with /^[0-9a-f]+$/ (hex-only, sufficient) |
|
src/context.js:114 |
🚨 INCOMPLETE PROTECTION — see below |
🔴 Critical gap: sanitizeGitArg at line 114
Current implementation (line 105-109):
function sanitizeGitArg(value) {
if (/[;&|`$(){}!#<>\n\r]/.test(value)) {
throw new Error(`Unsafe characters in git argument: ${value}`);
}
return value;
}Missing from blocklist:
-
Space (
range = "HEAD --pretty=format:%H" // Becomes: git log --format="%h %s" HEAD --pretty=format:%H 2>/dev/null // Attacker controls --pretty, altering output format
-
Tab (
\t) → same as space, separates arguments -
Backslash (
\) → potential escape sequences depending on shell context
Your defense: "Requires shell features (2>/dev/null || fallback), out of scope."
Counterargument:
This is a security audit PR. The shell fallback is refactorable:
let logOutput;
try {
logOutput = execFileSync('git', ['log', '--format=%h %s', range], { cwd, encoding: 'utf-8' });
} catch {
logOutput = execFileSync('git', ['log', '--format=%h %s', `-${limit}`], { cwd, encoding: 'utf-8' });
}No shell needed. No || operator. No incomplete sanitizer. Problem solved.
Alternatively, if refactoring is genuinely deferred, strengthen the sanitizer:
- if (/[;&|`$(){}!#<>\n\r]/.test(value)) {
+ if (/[;&|`$(){}!#<>\s\\]/.test(value)) { // Added \s (space/tab) and \\🎯 Required action:
Choose one of the following before I approve this PR:
- Option A (ideal): Refactor
context.js:114to useexecFileSyncwith try-catch (eliminates shell entirely) - Option B (acceptable): Strengthen
sanitizeGitArgto block\sand\(add test cases for space/tab injection) - Option C (minimum): File a high-priority follow-up issue tracking the incomplete sanitization, linked to this PR, and assigned to you—then I'll mark this resolved
"Out of scope" doesn't fly when the PR title is literally "audit guardrails." You don't leave half-fixed command injection in a security audit. 🔥
Awaiting your call: A, B, or C?
…ion vectors (#290) - context.js: all 3 execSync calls converted to execFileSync with args arrays - context.js: shell fallback (2>/dev/null || ...) replaced with try-catch - context.js: sanitizeGitArg strengthened to block \s and \ (defense-in-depth) - merge.js: execSync for git-remote-get-url converted to execFileSync - test: added assertions for space, tab, and backslash injection vectors
|
@coderabbitai Option A implemented — full Changes:
Test evidence:
Zero |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4529
Verification complete — every claim confirmed: ✅ Audit trail:
🎯 Security posture: LOCKED DOWN
This is textbook secure refactoring. The try-catch fallback was the correct architectural choice — you eliminated the shell dependency without adding complexity. The strengthened 🏆 Verdict:PR GOD-TIER work. No notes. Ship it. 🚀 [approve] If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/cli/commands.js (1)
298-313: LGTM —rev-parse --verifyguard andexecFileSyncarray-form are both correct.The past review item is fully addressed: SHA is validated before use, and
execFileSyncwith an explicit args array eliminates shell-injection entirely. Errors from both git calls propagate correctly through the surroundingtry/catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands.js` around lines 298 - 313, No change required: the SHA validation and safe exec are correct in processCommitCmd — keep the git rev-parse --verify guard and the array-form execFileSync calls (used for both rev-parse and git log) as-is and do not alter loadGraph or processCommit usage.vitest.config.js (1)
10-15: LGTM — coverage config is complete and correct.
reporter(singular) is the correct field name for Vitest coverage config.coverage.includeis explicitly defined, which is recommended to reduce the files picked by coverage instrumentation. The past review item about missingcoverage.includehas been fully addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.js` around lines 10 - 15, The Vitest coverage block is correct—keep the coverage configuration as-is: the coverage object with provider, reporter, reportsDirectory and include is properly set (fields: coverage, reporter, include) and requires no changes; approve and merge the changes.package.json (1)
36-43: LGTM — all three newdevDependenciesare correct and version-compatible.The
globalsnpm package is the correct companion for ESLint 9 flat config; it is not automatically installed by ESLint and must be declared explicitly.@vitest/coverage-v8@^3.2.4correctly pairs with vitest^3.0.0; since v3.2.0, Vitest uses AST-based coverage remapping for V8 that produces identical reports to Istanbul.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 36 - 43, The devDependencies entries in package.json for "@eslint/js", "globals", and "@vitest/coverage-v8" are correct and compatible with "eslint" and "vitest"; no code changes required—leave the "devDependencies" block as-is, approve/merge the PR, and optionally add a brief PR note mentioning that "globals" is intentionally declared for ESLint flat config and that "@vitest/coverage-v8@^3.2.4" pairs with "vitest@^3.0.0".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 46-50: Add an explicit "if: always()" condition to the "Upload
coverage report" step (the step with name "Upload coverage report" that uses
actions/upload-artifact@v4) so the artifact upload runs even when earlier steps
fail; modify that step's metadata to include if: always() so partial
coverage/lcov.info is uploaded on test failures for diagnosis.
- Around line 40-44: The CI workflow currently runs the full test suite twice:
the "Run tests" step invoking `npm test` and the "Run tests with coverage" step
invoking `npm run test:coverage`; remove the redundant `npm test` step (or
replace it with the coverage step) so the matrix runs only `npm run
test:coverage` for both pass/fail gating and coverage reporting, updating the
step named "Run tests" or deleting it and retaining the "Run tests with
coverage" step accordingly.
In `@src/context.js`:
- Around line 104-109: sanitizeGitArg currently blocks shell metacharacters but
allows leading hyphens, enabling option-injection when its output (the `range`
value) is passed to `git log` via `execFileSync`; update sanitizeGitArg to also
reject values that begin with a hyphen (e.g., /^-/) and throw the same Error for
consistency, then ensure the `git log` call that uses the `range` array still
falls back to the numeric `-${limit}` path for safe cases; add regression tests
for sanitizeGitArg (and the code path that invokes `git log`) asserting that
values like `--all`, `--all-match`, `-n5` (and other leading-dash git options)
are rejected.
In `@src/suggest.js`:
- Line 9: The comment "// edges.js imported for future use in context-aware
suggestions" is misleading because there is no actual import of edges.js; remove
or replace it with a forward-declaration that doesn't imply an import (e.g., "//
edges.js will be used in future for context-aware suggestions" or remove the
line entirely) in src/suggest.js, ensuring no mention of "imported" when no
import exists so readers won't search for a non-existent import.
---
Duplicate comments:
In `@package.json`:
- Around line 36-43: The devDependencies entries in package.json for
"@eslint/js", "globals", and "@vitest/coverage-v8" are correct and compatible
with "eslint" and "vitest"; no code changes required—leave the "devDependencies"
block as-is, approve/merge the PR, and optionally add a brief PR note mentioning
that "globals" is intentionally declared for ESLint flat config and that
"@vitest/coverage-v8@^3.2.4" pairs with "vitest@^3.0.0".
In `@src/cli/commands.js`:
- Around line 298-313: No change required: the SHA validation and safe exec are
correct in processCommitCmd — keep the git rev-parse --verify guard and the
array-form execFileSync calls (used for both rev-parse and git log) as-is and do
not alter loadGraph or processCommit usage.
In `@vitest.config.js`:
- Around line 10-15: The Vitest coverage block is correct—keep the coverage
configuration as-is: the coverage object with provider, reporter,
reportsDirectory and include is properly set (fields: coverage, reporter,
include) and requires no changes; approve and merge the changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/ci.ymleslint.config.jspackage.jsonsrc/cli/commands.jssrc/context.jssrc/dag.jssrc/doctor.jssrc/merge.jssrc/nodes.jssrc/suggest.jstest/context.test.jsvitest.config.js
- ci.yml: collapse redundant npm test + test:coverage into single coverage step - ci.yml: add if: always() to upload step for diagnosis on failure - context.js: block leading hyphens in sanitizeGitArg to prevent option injection - suggest.js: fix misleading "imported" comment to TODO forward-declaration - test: add regression tests for option-injection vectors (--all, -n99999, etc.)
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
1 participant
Summary
execSyncwithexecFileSyncinprocessCommitCmdand addrev-parse --verifySHA validation to eliminate shell/option injection@eslint/js,globals, and@vitest/coverage-v8as explicit devDependenciesProblem Statement
processCommitCmdwhere user-supplied SHA was interpolated intoexecSyncshell stringCloses #290
ADR Compliance (Required)
Relevant ADR(s)
Compliance Declaration
Architecture Laws Checklist (Hard Gates)
Canonical Truth & Context
--at,--observer,--trust) or deterministically defaulted.Determinism & Provenance
Artifact Hygiene
Contracts & Compatibility
Extension/Effects Safety (if applicable)
Scope Control
Backward Compatibility
execFileSyncproduces identical output toexecSyncfor the same git command;rev-parse --verifyfails fast on invalid refs that would have failed anyway ingit log.Test Plan (Required)
Unit
Integration
Determinism
/in character class)npm test -- test/remote.test.js test/validators.test.jsContract/Schema
npm test -- test/contracts.integration.test.jsPolicy Gates
# Pre-commit hooks pass on all 6 commitsSecurity / Trust Impact
processCommitCmdeliminated viaexecFileSync+ SHA validation withrev-parse --verify.rev-parse --verifywill reject invalid refs earlier (fail-fast, not a new failure — these refs would have failed ingit loganyway).Performance Impact
processCommitCmdis called once per post-commit hook invocation.git rev-parsecall per commit processing.Observability / Debuggability
Operational Notes
npm cinow installs@vitest/coverage-v8,@eslint/js, andglobalsas devDependenciesLinked Issues / Milestones