fix(security): resolve tool paths absolutely, close digest evidence gaps - #56
Conversation
security-004: rg/git launched via Command::new(bare-name) with current_dir()
set to the repo under analysis, letting a scanned repo's own rg.exe/git.exe
win the Windows PATH search. Add crates/code-intel-cli/src/tool_path.rs,
a shared absolute-path resolver reusing doctor_adapter.rs's existing
tool-inventory candidate/PATH-scan logic, and route every production
Command::new("rg")/Command::new("git") call site through it
(hardened_git.rs, sentrux_analysis.rs, snapshot.rs, capability_inventory.rs).
doctor_adapter.rs's tool_available() now delegates to the same resolver so
presence-checking and launch-time resolution can't drift apart. Mirrored the
same fix in tools/sentrux-shim/sentrux-shim.ps1's rg/git invocations.
supply-chain-008: 4 of 14 orchestration/integrations.json capability entries
(doctor.envelope.compat, repository.snapshot.compat,
advisory.workflow-recommend.compat, inventory.rg.compat) declared
toolchainDigests with no toolchainDigestEvidence.inputs, so they could not be
drift-checked. Added evidence.inputs mapping each entry to the actual source
file(s) implementing it, and recomputed toolchainDigests via sha256 over
those inputs. Because tool_path.rs changed the byte content of
doctor_adapter.rs, capability_inventory.rs, and snapshot.rs, their digests
moved; that shift cascades through crates/code-intel-cli/tests/artifact_ref.rs
and tests/capability_exec.rs (hardcoded request fixtures validated against
the registry by capability.rs's cohere()), through
orchestration/internalization/{git,rg,ast-grep,sentrux}.json (provenance
records whose sha256 fields are live-recomputed and asserted by
internalization_record.rs/ast_grep_internalization.rs), and through
run-code-intel.ps1 (a hardcoded advisory.workflow-recommend.compat request
fixture). All of these were updated to keep cargo test -p code-intel green;
none of them are logic changes, only digest-value corrections.
supply-chain-009: tools/sentrux-shim/sentrux-shim.ps1 auto-activated a
fabricated local Pro-tier license for third-party sentrux on every
bootstrapped machine, opt-out only, with no upstream license evidence
anywhere in the tree to justify it. No genuine provenance could be found, so
auto-activation is now opt-in: it only fires when SENTRUX_AUTO_PRO is
explicitly set to an affirmative value ("1"/"true"), default off.
Addresses security-004 from #33, supply-chain-008/009 from #35.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds shared absolute-path resolution for ChangesAbsolute Tool Resolution
Explicit Pro Activation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CapabilityAdapters
participant tool_path
participant ExternalTools
CapabilityAdapters->>tool_path: resolve("rg") or resolve("git")
tool_path->>tool_path: locate executable outside current directory
tool_path-->>CapabilityAdapters: absolute executable path
CapabilityAdapters->>ExternalTools: launch command with resolved path
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
supply-chain-009 flipped SENTRUX_AUTO_PRO from opt-out to opt-in in this branch; README.md and the install-plan text still described auto-activation as the default. Update both to the opt-in reality.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
install-code-intel-pipeline.ps1 (1)
846-846: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse named parameters for
Add-InstallPlan.This changed call has many positional arguments, making future parameter additions easy to misbind. Named arguments also resolve the reported PSScriptAnalyzer warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install-code-intel-pipeline.ps1` at line 846, Update the Add-InstallPlan invocation for "sentrux-shim" to pass every argument using its declared parameter names instead of positional ordering. Preserve the existing values and ordering semantics while eliminating positional binding and the associated PSScriptAnalyzer warning.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/code-intel-cli/src/tool_path.rs`:
- Around line 42-57: Update executable discovery in the prefix and PATH search
branches of the tool-path lookup function to canonicalize each matching
candidate with std::fs::canonicalize before returning it, ensuring relative or
empty PATH entries produce absolute paths. Add focused cargo test coverage for
relative and empty PATH components, plus the relevant integration-contract
checks.
In `@tools/sentrux-shim/sentrux-shim.ps1`:
- Around line 108-112: Retain the opt-in activation logic in sentrux-shim.ps1 at
lines 108-112. In install-code-intel-pipeline.ps1, update the installer health
checks at lines 490-493 and 903 to accept free/inactive status when
SENTRUX_AUTO_PRO is unset, while requiring Pro status only when the environment
variable is explicitly affirmative.
---
Nitpick comments:
In `@install-code-intel-pipeline.ps1`:
- Line 846: Update the Add-InstallPlan invocation for "sentrux-shim" to pass
every argument using its declared parameter names instead of positional
ordering. Preserve the existing values and ordering semantics while eliminating
positional binding and the associated PSScriptAnalyzer warning.
🪄 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 Plus
Run ID: 3b313e48-1462-4359-a058-2f508db04b18
📒 Files selected for processing (17)
README.mdcrates/code-intel-cli/src/capability_inventory.rscrates/code-intel-cli/src/doctor_adapter.rscrates/code-intel-cli/src/hardened_git.rscrates/code-intel-cli/src/sentrux_analysis.rscrates/code-intel-cli/src/snapshot.rscrates/code-intel-cli/src/tool_path.rscrates/code-intel-cli/tests/artifact_ref.rscrates/code-intel-cli/tests/capability_exec.rsinstall-code-intel-pipeline.ps1orchestration/integrations.jsonorchestration/internalization/ast-grep.jsonorchestration/internalization/git.jsonorchestration/internalization/rg.jsonorchestration/internalization/sentrux.jsonrun-code-intel.ps1tools/sentrux-shim/sentrux-shim.ps1
| if let Some(prefix) = prefix { | ||
| if let Some(found) = names | ||
| .iter() | ||
| .map(|candidate| prefix.join(candidate)) | ||
| .find(|full| full.is_file()) | ||
| { | ||
| return Some(found); | ||
| } | ||
| } | ||
| let path = env::var_os("PATH")?; | ||
| env::split_paths(&path).find_map(|dir| { | ||
| names | ||
| .iter() | ||
| .map(|candidate| dir.join(candidate)) | ||
| .find(|full| full.is_file()) | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Canonicalize located executables before returning them.
A relative or empty PATH component (for example .) makes dir.join(candidate) relative. Downstream Command::new calls then apply the scanned repository as current_dir, allowing a planted rg/git there to execute. Return std::fs::canonicalize(full) for found candidates and add a regression test for relative/empty PATH entries.
Proposed fix
+use std::fs;
use std::env;
+fn existing_absolute(path: PathBuf) -> Option<PathBuf> {
+ if !path.is_file() {
+ return None;
+ }
+ fs::canonicalize(path).ok()
+}
+
pub(crate) fn locate(name: &str, prefix: Option<&Path>) -> Option<PathBuf> {
let names = candidates(name);
if let Some(prefix) = prefix {
- if let Some(found) = names
+ if let Some(found) = names
.iter()
.map(|candidate| prefix.join(candidate))
- .find(|full| full.is_file())
+ .find_map(existing_absolute)
{
return Some(found);
}
}
let path = env::var_os("PATH")?;
env::split_paths(&path).find_map(|dir| {
names
.iter()
.map(|candidate| dir.join(candidate))
- .find(|full| full.is_file())
+ .find_map(existing_absolute)
})
}As per coding guidelines, “Rust changes require focused cargo test coverage plus the relevant integration-contract checks.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(prefix) = prefix { | |
| if let Some(found) = names | |
| .iter() | |
| .map(|candidate| prefix.join(candidate)) | |
| .find(|full| full.is_file()) | |
| { | |
| return Some(found); | |
| } | |
| } | |
| let path = env::var_os("PATH")?; | |
| env::split_paths(&path).find_map(|dir| { | |
| names | |
| .iter() | |
| .map(|candidate| dir.join(candidate)) | |
| .find(|full| full.is_file()) | |
| }) | |
| use std::fs; | |
| fn existing_absolute(path: PathBuf) -> Option<PathBuf> { | |
| if !path.is_file() { | |
| return None; | |
| } | |
| fs::canonicalize(path).ok() | |
| } | |
| if let Some(prefix) = prefix { | |
| if let Some(found) = names | |
| .iter() | |
| .map(|candidate| prefix.join(candidate)) | |
| .find_map(existing_absolute) | |
| { | |
| return Some(found); | |
| } | |
| } | |
| let path = env::var_os("PATH")?; | |
| env::split_paths(&path).find_map(|dir| { | |
| names | |
| .iter() | |
| .map(|candidate| dir.join(candidate)) | |
| .find_map(existing_absolute) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/code-intel-cli/src/tool_path.rs` around lines 42 - 57, Update
executable discovery in the prefix and PATH search branches of the tool-path
lookup function to canonicalize each matching candidate with
std::fs::canonicalize before returning it, ensuring relative or empty PATH
entries produce absolute paths. Add focused cargo test coverage for relative and
empty PATH components, plus the relevant integration-contract checks.
Source: Coding guidelines
The shared tool_path resolver (security-004) adds one module imported by six call sites, moving coupling 45.13 -> 45.33 and quality 3968 -> 3967. This is the fix's intrinsic structural cost — the same documented re-baseline procedure used for the ast-grep internalization suite (45.07 -> 45.13). Cycles and god files unchanged. Gate and full self-scan verified green locally after the re-baseline.
Flipping Pro auto-activation to opt-in left three health checks still requiring Tier: pro unconditionally, so a default install now failed its own installer verification, the tool:sentrux-pro required check, and check-code-intel-tools' missing-list (CI Install portable pipeline step red on all four runners). The expected tier now follows the opt-in: pro when SENTRUX_AUTO_PRO is affirmative, pro-or-free otherwise, mirroring the shim's own truthiness set.
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)
install-code-intel-pipeline.ps1 (1)
850-850: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the
SENTRUX_AUTO_PROguidance PowerShell-appropriate.SENTRUX_AUTO_PRO=1is valid CMD/POSIX syntax, not PowerShell syntax; either update these hints to$Env:SENTRUX_AUTO_PRO='1'for PowerShell users or explicitly say it applies to CMD/cmd.exe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install-code-intel-pipeline.ps1` at line 850, Update the SENTRUX_AUTO_PRO guidance in the Add-InstallPlan entry and the related guidance at install-code-intel-pipeline.ps1 lines 907-910 to use PowerShell syntax ($Env:SENTRUX_AUTO_PRO='1') or explicitly identify the existing syntax as CMD/cmd.exe syntax; keep the opt-in behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@install-code-intel-pipeline.ps1`:
- Line 850: Update the SENTRUX_AUTO_PRO guidance in the Add-InstallPlan entry
and the related guidance at install-code-intel-pipeline.ps1 lines 907-910 to use
PowerShell syntax ($Env:SENTRUX_AUTO_PRO='1') or explicitly identify the
existing syntax as CMD/cmd.exe syntax; keep the opt-in behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44ff834c-6df9-4ee9-95bb-81f2e36514f6
📒 Files selected for processing (2)
check-code-intel-tools.ps1install-code-intel-pipeline.ps1
…olution Resolving the sentrux.json conflict with a whole-file --theirs take kept main's post-#45 sentrux_gate.rs digest but discarded this branch's own re-pin for the edited sentrux_analysis.rs, failing internalization_record.rs ticket_r03 on every platform. Recomputed and swept all 35 internalization records for stale digests (only this one). Full suite green locally: 44 suites, 2082 passed, 0 failed.
1 participant
Summary
Fixes three self-audit findings in one PR:
security-004 (info/low-confidence):
Command::new("rg")/Command::new("git")were launched by bare program name while the child'scurrent_dir()was set to the repo under analysis — a scanned repo that plants its ownrg.exe/git.execould win Windows's PATH search. Addedcrates/code-intel-cli/src/tool_path.rs, a shared absolute-path resolver that reusesdoctor_adapter.rs's existing tool-inventory candidate/PATH-scan logic (no new resolution logic written). Routed every production call site through it:hardened_git.rs(the single choke point for allgitinvocations — coverssnapshot.rs,sentrux_analysis.rs,sentrux_gate.rs,audit_report/cli.rstransitively),sentrux_analysis.rs,snapshot.rs,capability_inventory.rs.doctor_adapter.rs::tool_available()now delegates to the same resolver so presence-checking and launch-time resolution can never drift apart. Mirrored the same fix intools/sentrux-shim/sentrux-shim.ps1'srg/gitinvocations.supply-chain-008 (low): 4 of 14
orchestration/integrations.jsoncapability entries (doctor.envelope.compat,repository.snapshot.compat,advisory.workflow-recommend.compat,inventory.rg.compat) declaredtoolchainDigestswith notoolchainDigestEvidence.inputs, making them unverifiable/non-drift-checkable. AddedtoolchainDigestEvidence.inputsto each, mapping to the actual source file(s) each capability key implements (traced through code, not guessed from theentrypointfield — e.g.inventory.rg's entrypoint string saysrun-code-intel.ps1but its actual implementation iscapability_inventory.rs::inventory()). RecomputedtoolchainDigestsvia sha256 over those inputs.supply-chain-009 (info):
tools/sentrux-shim/sentrux-shim.ps1fabricated a local Pro-tier license for third-partysentruxand auto-activated it on every bootstrapped machine, opt-out only, with no license justification anywhere in the tree. SearchedREADME.mdandinstall-code-intel-pipeline.ps1for any genuine upstream license evidence — found none. Per the task's explicit instruction not to fabricate a justification that can't be substantiated, chose option (b): flipped auto-activation from opt-out to opt-in viaSENTRUX_AUTO_PRO(only fires when explicitly set to"1"/"true", default off).Why the diff is bigger than the 3 findings alone
tool_path.rschanged the byte content ofdoctor_adapter.rs,capability_inventory.rs, andsnapshot.rs, which moved their sha256 digests. That shift cascades three layers deep, all required to keepcargo test -p code-intelgreen (an explicit gate for this change):orchestration/integrations.jsondigests corrected (the supply-chain-008 fix itself).crates/code-intel-cli/tests/artifact_ref.rsandtests/capability_exec.rshardcodeinventory.rg/advisory.workflow-recommendrequest digests thatcapability.rs'scohere()validates against the registry; these were pinned to the pre-fix values and had to move with the registry.orchestration/internalization/{git,rg,ast-grep,sentrux}.jsonindependently embed sha256 of the same source/conformance files, live-recomputed and asserted byinternalization_record.rs/ast_grep_internalization.rs; andrun-code-intel.ps1hardcodes the sameadvisory.workflow-recommend.compatrequest fixture as the Rust tests. All updated to the corrected values — value-only corrections, no logic changes, same "critical fix" character as thesentrux-shim.ps1edit underAGENTS.md's carve-out.Every one of these files was pulled in by an actual test failure that was root-caused, not by scope creep. All 35
toolchainDigestEvidence.inputsslots across the registry and all 84operationTraceentries in the internalization records were independently re-verified against live file content after the fact.Out of scope, deliberately:
README.md/install-code-intel-pipeline.ps1still describe the old default-Pro-activation behavior post supply-chain-009's opt-in flip. Flagged as a separate follow-up rather than folded in here, to keep this PR's diff limited to what the three findings and the test gate actually require.Test plan
cargo build -p code-intel— 0 errorscargo test -p code-intel— 2025 passed, 44 suites, 0 failedpwsh -NoProfile -Command "[scriptblock]::Create(...)"on both edited.ps1files — parses cleanlyorchestration/integrations.jsonand all touchedorchestration/internalization/*.json— valid JSONtoolchainDigestEvidenceentries + the 4 other entries whose digest values shifted — independently re-verified as correct (not just plausible-looking hex)Addresses security-004 from #33, supply-chain-008/009 from #35.