Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates CI checks into a single Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
📝 Coding Plan for PR comments
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 99.44% 99.53% +0.08%
==========================================
Files 50 50
Lines 4533 4528 -5
==========================================
- Hits 4508 4507 -1
+ Misses 25 21 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR closes a security gap where a crafted Changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["TMPDIR env assignment detected"] --> B{"tmpdirValue == ''?"}
B -- Yes --> BLOCK["🚫 Block (empty TMPDIR)"]
B -- No --> C["normalize(tmpdirValue)"]
C --> D{"isPathOrSubpath(normalized, /tmp)\nor /var/tmp\nor sysTmpdir?"}
D -- Yes --> ALLOW["✅ Allow"]
D -- No --> BLOCK2["🚫 Block (non-temp path)"]
subgraph "Before fix"
E["/tmp/../root"] -- "raw string" --> F{"startsWith /tmp/?"}
F -- "Yes ❌ (bypassed!)" --> ALLOW2["✅ Allow (incorrect)"]
end
subgraph "After fix"
G["/tmp/../root"] -- "normalize()" --> H["/root"]
H --> I{"startsWith /tmp/?"}
I -- "No ✅" --> BLOCK3["🚫 Block (correct)"]
end
Last reviewed commit: 3e7df7f |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/publish.yml (1)
69-72: Use--frozen-lockfilein the publish job too.The
publishjob is gated byfull-check, which validates the lockfile with--frozen-lockfile. Reinstalling dependencies inpublishwith the same strict mode ensures consistency and makes the release path deterministic and fail-closed.Suggested fix
- name: Install dependencies - run: bun install + run: bun install --frozen-lockfile env: BUN_INSTALL_ALLOW_SCRIPTS: "@ast-grep/cli"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 69 - 72, In the publish job's "Install dependencies" step replace the plain bun install invocation with bun install --frozen-lockfile to enforce the same lockfile validation as full-check; update the command where "Install dependencies" is defined (the step that currently runs "bun install" and sets BUN_INSTALL_ALLOW_SCRIPTS) so the install runs with the --frozen-lockfile flag to make the publish path deterministic and fail if the lockfile is out of date.
🤖 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 7-18: The CI push trigger currently excludes package.json from the
push.paths list, so updates to dependencies or scripts skip post-merge jobs;
update the paths block in .github/workflows/ci.yml (the paths: entries) to
include "package.json" (or remove the exclusion logic) so that changes to
package.json will trigger the push workflow and run the build/draft-release
steps after merge.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 69-72: In the publish job's "Install dependencies" step replace
the plain bun install invocation with bun install --frozen-lockfile to enforce
the same lockfile validation as full-check; update the command where "Install
dependencies" is defined (the step that currently runs "bun install" and sets
BUN_INSTALL_ALLOW_SCRIPTS) so the install runs with the --frozen-lockfile flag
to make the publish path deterministic and fail if the lockfile is out of date.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54673cf6-f4ec-43c0-b833-b01ca5f15184
⛔ Files ignored due to path filters (2)
dist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/publish.ymlpackage.jsonsrc/core/analyze/tmpdir.tssrc/core/shell.tstests/bin/doctor/system-info.test.tstests/bin/explain/command.test.tstests/core/analyze/analyze-coverage.test.tstests/core/analyze/parsing-helpers.test.tstests/core/analyze/tmpdir.test.tstests/core/rules-rm.test.tstests/scripts/ci-workflow.test.ts
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 @.github/workflows/ci.yml:
- Around line 7-19: The workflow's push.paths filter omits assets/** so the
auto-commit that pushes assets/cc-safety-net.schema.json (the auto-commit step
that updates the schema) can bypass CI; update the push.paths section to include
"assets/**" (or change the auto-commit to create a PR instead of pushing
directly) so that changes to assets/cc-safety-net.schema.json always retrigger
the ci.yml workflow; specifically modify the push.paths block to add the
"assets/**" entry (or alter the auto-commit logic that pushes
assets/cc-safety-net.schema.json).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4ef16f4-be3e-46d4-a4c9-60085a6d5370
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/scripts/ci-workflow.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/scripts/ci-workflow.test.ts
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)
.github/workflows/ci.yml (1)
68-84:⚠️ Potential issue | 🟠 MajorFreeze the lockfile in
buildtoo.The
buildjob depends onfull-checkbut uses plainbun installinstead of--frozen-lockfile. This allows the artifact-producing job to resolve a different dependency graph than the one validated infull-check, making release artifacts less reproducible and potentially inconsistent with the validated state.- name: Install dependencies - run: bun install + run: bun install --frozen-lockfile env: BUN_INSTALL_ALLOW_SCRIPTS: "@ast-grep/cli"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 68 - 84, The build job currently runs a non-frozen install ("bun install" in the Install dependencies step), which can produce a different dependency graph than full-check; update that step in the build job to run bun install --frozen-lockfile (preserving the existing env BUN_INSTALL_ALLOW_SCRIPTS) so the lockfile is enforced and the artifact-producing job uses the exact, validated dependency graph from full-check.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
49-51: Pin Bun to a specific version in all workflows.Using
bun-version: latestmakes CI behavior change whenever Bun ships a new release. This affects 6 workflow locations across 4 files (.github/workflows/ci.yml, publish.yml, test-windows.yml). Sincepackage.jsondoes not declare a Bun version, pin a specific version directly in the workflows or add version declaration topackage.jsonand reference it consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 49 - 51, CI workflows currently use "uses: oven-sh/setup-bun@v2" with the input key "bun-version: latest", which causes nondeterministic CI when Bun releases new versions; update each workflow location (where oven-sh/setup-bun@v2 is used and bun-version is set) to pin a concrete Bun version string (e.g., "bun-version: 1.0.0" or whatever your project requires) or alternatively add a Bun version field to package.json and read that in the workflows, ensuring all occurrences of the bun-version input across .github/workflows/ci.yml, publish.yml, and test-windows.yml are changed to reference the fixed version variable/value.
🤖 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 @.github/workflows/ci.yml:
- Around line 68-84: The build job currently runs a non-frozen install ("bun
install" in the Install dependencies step), which can produce a different
dependency graph than full-check; update that step in the build job to run bun
install --frozen-lockfile (preserving the existing env
BUN_INSTALL_ALLOW_SCRIPTS) so the lockfile is enforced and the
artifact-producing job uses the exact, validated dependency graph from
full-check.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 49-51: CI workflows currently use "uses: oven-sh/setup-bun@v2"
with the input key "bun-version: latest", which causes nondeterministic CI when
Bun releases new versions; update each workflow location (where
oven-sh/setup-bun@v2 is used and bun-version is set) to pin a concrete Bun
version string (e.g., "bun-version: 1.0.0" or whatever your project requires) or
alternatively add a Bun version field to package.json and read that in the
workflows, ensuring all occurrences of the bun-version input across
.github/workflows/ci.yml, publish.yml, and test-windows.yml are changed to
reference the fixed version variable/value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba09b154-42f5-42db-94aa-139d3495a434
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/scripts/ci-workflow.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/scripts/ci-workflow.test.ts
|
@greptileai review the PR |
1 participant
Summary
TMPDIR=/tmp/../rootwas incorrectly allowed)full-checkjobChanges
src/core/analyze/tmpdir.ts— Normalize paths withnode:pathbefore comparison; use platformsepinstead of hardcoded/src/core/shell.ts— Remove unreachableundefinedcheck, use proper type assertion, remove unusedparseEnvAssignment().github/workflows/ci.yml— Mergequality+testintofull-checkjob usingcheck:ci; add trigger path filters forpush/pull_request; use--frozen-lockfile.github/workflows/publish.yml— Mergetest+typecheckintofull-checkjob usingcheck:ci; use--frozen-lockfilepackage.json— Addcheck:ci(non-write lint + typecheck + knip + ast-grep + tests w/ lcov) andlint:ciscriptstests/core/analyze/tmpdir.test.ts[NEW] — Unit tests for traversal detection across/tmp,/var/tmp, and system tmpdirtests/scripts/ci-workflow.test.ts[NEW] — Validates CI trigger path filters stay in synctests/core/rules-rm.test.ts— Integration tests forrm -rfwith traversal-based TMPDIRtests/core/analyze/analyze-coverage.test.ts,parsing-helpers.test.ts— Edge-case coverage for env assignment parsingTesting
Related Issue
PR Checklist
bun run checkpasses (lint, types, dead code, rules, tests)package.jsonSummary by CodeRabbit
Bug Fixes
Tests
Chores