Skip to content

feat: add experimental Rust local skill inspector - #450

Open
SH4DY wants to merge 4 commits into
mainfrom
fm/rustport
Open

feat: add experimental Rust local skill inspector#450
SH4DY wants to merge 4 commits into
mainfrom
fm/rustport

Conversation

@SH4DY

@SH4DY SH4DY commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Adds an additive Rust inspect --skills --json local vertical slice without changing the released Python CLI. It delegates frontmatter and redaction to the exact Python implementation, fails closed, and leaves discovery, MCP, network, packaging, and Windows deferred.

On an M5 Pro: help 367.2→5.1 ms; full corpus 8.815→9.023 s; aliased skills 5.694→1.099 s. The docs define the supported boundary and reproducible benchmark.

@SH4DY
SH4DY requested a review from a team as a code owner August 24, 2026 16:18
@qodo-merge-etso

Copy link
Copy Markdown

PR Summary by Qodo

Add experimental Rust local skill inspector

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Adds an experimental Rust command for explicit, local skill JSON inspection.
• Preserves Python frontmatter and secret-redaction semantics while failing closed.
• Adds equivalence tests, CI validation, benchmarks, and supported-scope documentation.
Diagram

graph TD
  CLI["Rust CLI"] --> Resolver["Path Resolver"] --> Walker["Skill Walker"] --> Cache{"Cached content?"}
  Cache -->|No| Worker["Python Worker"] --> Buffer["JSON Buffer"] --> Output["Standard Output"]
  Cache -->|Yes| Buffer
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Optimize the Python collector
  • ➕ Keeps one implementation and release path
  • ➕ Can apply the same identity-cache optimization
  • ➕ Avoids cross-process protocol and Rust maintenance
  • ➖ Retains Python startup overhead
  • ➖ Does not independently validate a Rust migration boundary
2. Port parsing and redaction fully to Rust
  • ➕ Could remove Python runtime and worker overhead
  • ➕ Could eventually enable a standalone binary
  • ➖ Risks diverging YAML and detect-secrets behavior
  • ➖ Greatly expands security and compatibility scope
  • ➖ Requires substantially broader regression coverage

Recommendation: Keep the narrow hybrid slice as the safest way to evaluate Rust traversal without weakening established parsing or redaction semantics. If alias-heavy scan performance is the product objective, prioritize the equivalent Python identity cache; defer a full Rust port until exact redaction compatibility and packaging requirements justify the added risk.

Files changed (6) +1026 / -0

Enhancement (1) +575 / -0
main.rsImplement fail-closed local skill inspection in Rust +575/-0

Implement fail-closed local skill inspection in Rust

• Adds the constrained Rust CLI, explicit-path resolution, deterministic recursive traversal, symlink-cycle rejection, binary hashing, and per-invocation target-identity caching. Frontmatter parsing and text redaction use one private Python worker, and output remains buffered until that worker exits successfully.

rust/src/main.rs

Tests (1) +121 / -0
test_local_inspect_equivalence.pyVerify Rust output, redaction, aliases, and failure behavior +121/-0

Verify Rust output, redaction, aliases, and failure behavior

• Compares Rust and Python JSON over the repository skill corpus and symlink aliases, confirms detect-secrets removes an AWS key fixture, and ensures worker startup failure returns exit code 2 without stdout.

tests/rust/test_local_inspect_equivalence.py

Documentation (1) +128 / -0
rust-local-inspect.mdDocument the supported Rust slice, exclusions, and benchmarks +128/-0

Document the supported Rust slice, exclusions, and benchmarks

• Defines invocation requirements, accepted path forms, Python delegation, fail-closed guarantees, deferred functionality, validation steps, and a reproducible performance methodology with recorded results.

docs/rust-local-inspect.md

Other (3) +202 / -0
rust-local-inspect.ymlValidate the Rust inspector and Python compatibility in CI +33/-0

Validate the Rust inspector and Python compatibility in CI

• Adds a GitHub Actions job that installs stable Rust and Python test dependencies, enforces formatting and Clippy, runs locked tests, builds a release binary, and executes the cross-language equivalence suite.

.github/workflows/rust-local-inspect.yml

Cargo.tomlDefine the experimental Rust binary and dependencies +21/-0

Define the experimental Rust binary and dependencies

• Introduces the non-publishable Rust package, binary target, runtime libraries for CLI parsing, serialization, hashing, and errors, plus tempfile for unit tests.

Cargo.toml

bench_local_inspect.pyBenchmark Python and Rust fresh-process inspection workloads +148/-0

Benchmark Python and Rust fresh-process inspection workloads

• Adds a standard-library benchmark for startup, full-corpus inspection, and repeated symlink aliases. It records raw samples, summary statistics, corpus shape, commands, and machine metadata as JSON.

rust/bench_local_inspect.py

@qodo-merge-etso

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (4) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Non-UTF-8 paths collide 🐞 Bug ≡ Correctness
Description
display_path and relative_path replace invalid Unix filename bytes with the same Unicode
replacement character, so distinct valid paths can be emitted identically. Because these lossy
strings are also used as JSON map keys, a later explicit path can silently overwrite an earlier
result, while distinct files can receive duplicate incorrect files[].path values.
Code

rust/src/main.rs[R230-232]

+fn display_path(path: &Path) -> String {
+    path.to_string_lossy().into_owned()
+}
Relevance

●●● Strong

Concrete non-UTF-8 collisions can overwrite scan results; recent path correctness fixes were
accepted.

PR-#442
PR-#321

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rust applies to_string_lossy to both displayed paths and relative filenames, stores those values
in output records, and inserts displayed paths into a map where equal keys replace prior values. The
Python collector computes and emits relative paths directly from filesystem strings, so Unix
undecodable bytes remain distinguishable through Python's filesystem encoding behavior.

rust/src/main.rs[230-232]
rust/src/main.rs[309-314]
rust/src/main.rs[352-360]
rust/src/main.rs[525-529]
src/agent_scan/skill_client.py[125-147]
src/agent_scan/skill_client.py[188-199]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Unix paths are arbitrary byte sequences, but the Rust inspector converts them with `to_string_lossy()`. Distinct paths can therefore collapse to the same displayed path or JSON key and silently overwrite results.

## Issue Context
The Python implementation traverses filesystem strings using surrogate-escape semantics, whereas the Rust conversion inserts `�`. Use a reversible representation compatible with the expected JSON output, or reject non-UTF-8 paths explicitly before producing any output; do not silently replace bytes.

## Fix Focus Areas
- rust/src/main.rs[230-232]
- rust/src/main.rs[309-314]
- rust/src/main.rs[525-528]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. main exposes internal errors 📘 Rule violation ☼ Reliability
Description
The CLI prints the full ScanError display text, including absolute scanned paths and underlying OS
I/O error details from ScanError::Read. User-facing failures should remain generic so local
filesystem internals are not exposed.
Code

rust/src/main.rs[R539-540]

+    if let Err(error) = run(Cli::parse()) {
+        eprintln!("snyk-agent-scan-rust: {error}");
Relevance

●●● Strong

The team accepts sanitizing filesystem paths and internal error details from user-facing
diagnostics.

PR-#320
PR-#433
PR-#214

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 6 prohibits exposing exception and internal details to clients. ScanError::Read embeds both
path and the source std::io::Error, and main formats that complete error directly to stderr.

Rule 6: Handle errors explicitly; don't swallow; clean up; don't leak internals
rust/src/main.rs[83-91]
rust/src/main.rs[538-542]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The CLI renders raw `ScanError` details to stderr, which can expose absolute paths and underlying OS error text.

## Issue Context
Compliance rule 6 requires generic user-facing errors rather than exception or internal-system details. Preserve detailed diagnostics only in an explicitly safe debug channel that does not include scanned content or sensitive paths.

## Fix Focus Areas
- rust/src/main.rs[78-106]
- rust/src/main.rs[538-542]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Alias count is duplicated 📘 Rule violation ☼ Reliability
Description
The alias corpus size is encoded separately as the range(1, 9) boundary and two literal 8 values
in benchmark metadata. Changing only one occurrence would produce inconsistent benchmark setup and
reported results.
Code

rust/bench_local_inspect.py[95]

+        for number in range(1, 9):
Relevance

●● Moderate

Duplication concerns are sometimes accepted, but generic magic-number cleanup is often rejected.

PR-#387
PR-#439

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 40 requires duplicated unexplained numeric values to be named. The loop creates eight aliases
using range(1, 9), while the output independently reports and multiplies by 8.

Rule 40: Replace magic numbers with named constants
rust/bench_local_inspect.py[94-98]
rust/bench_local_inspect.py[116-120]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The benchmark alias count is repeated through a range boundary and metadata literals.

## Issue Context
Use one named constant to drive alias creation, the reported alias count, and expected record calculation.

## Fix Focus Areas
- rust/bench_local_inspect.py[21-24]
- rust/bench_local_inspect.py[94-120]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. inspect_path exceeds 40 SLOC 📘 Rule violation ☼ Reliability
Description
inspect_path spans roughly 70 logical lines and combines path normalization, metadata validation,
file handling, skill-directory handling, generic-directory discovery, and response construction.
Extracting mode-specific helpers would keep each branch focused and within the required size.
Code

rust/src/main.rs[R435-439]

+fn inspect_path(
+    supplied: &Path,
+    redactor: &mut PythonRedactor,
+    cache: &mut ContentCache,
+) -> Result<(String, InspectedPath), ScanError> {
Relevance

● Weak

Recent function-length findings were rejected when behavior was unchanged and refactoring was the
sole concern.

PR-#391
PR-#433
PR-#320

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 45 limits functions to 40 SLOC. The new inspect_path function runs from lines 435 through 516
and contains multiple independent validation and inspection branches.

Rule 45: Limit function length to ≤ 40 lines (SLOC)
rust/src/main.rs[435-516]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`inspect_path` exceeds the 40-SLOC limit and handles several distinct path modes plus output construction.

## Issue Context
Separate the `SKILL.md` file case, direct skill-directory case, and generic parent-directory case into clearly named helpers while preserving attribution behavior.

## Fix Focus Areas
- rust/src/main.rs[435-516]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View low (1)
5. benchmark.main exceeds 40 SLOC 📘 Rule violation ☼ Reliability
Description
The benchmark main function spans about 80 lines and mixes argument parsing, validation, temporary
corpus construction, execution, and report assembly. Splitting these responsibilities into helpers
would satisfy the function-length requirement and make the benchmark easier to verify.
Code

rust/bench_local_inspect.py[R63-65]

+def main() -> None:
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--python-cli", default=str(ROOT / ".venv/bin/snyk-agent-scan"))
Relevance

● Weak

Recent team precedent repeatedly rejects function-length refactoring as maintainability-only
guidance.

PR-#391
PR-#388
PR-#433

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 45 limits functions to 40 SLOC. The new benchmark main occupies lines 63 through 144 and
performs parsing, filesystem setup, process execution, and nested report creation.

Rule 45: Limit function length to ≤ 40 lines (SLOC)
rust/bench_local_inspect.py[63-144]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The benchmark `main` function exceeds the 40-SLOC limit and owns several separate responsibilities.

## Issue Context
Extract argument parsing, alias-corpus setup, benchmark execution, and output construction into named helpers without changing the recorded methodology.

## Fix Focus Areas
- rust/bench_local_inspect.py[63-144]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 8 rules

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread rust/src/main.rs
Comment on lines +539 to +540
if let Err(error) = run(Cli::parse()) {
eprintln!("snyk-agent-scan-rust: {error}");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

1. main exposes internal errors 📘 Rule violation ☼ Reliability

The CLI prints the full ScanError display text, including absolute scanned paths and underlying OS
I/O error details from ScanError::Read. User-facing failures should remain generic so local
filesystem internals are not exposed.
Agent Prompt
## Issue description
The CLI renders raw `ScanError` details to stderr, which can expose absolute paths and underlying OS error text.

## Issue Context
Compliance rule 6 requires generic user-facing errors rather than exception or internal-system details. Preserve detailed diagnostics only in an explicitly safe debug channel that does not include scanned content or sensitive paths.

## Fix Focus Areas
- rust/src/main.rs[78-106]
- rust/src/main.rs[538-542]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

for name in SUBSET:
(source / name).symlink_to(SKILLS / name, target_is_directory=True)
aliases = []
for number in range(1, 9):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

2. Alias count is duplicated 📘 Rule violation ☼ Reliability

The alias corpus size is encoded separately as the range(1, 9) boundary and two literal 8 values
in benchmark metadata. Changing only one occurrence would produce inconsistent benchmark setup and
reported results.
Agent Prompt
## Issue description
The benchmark alias count is repeated through a range boundary and metadata literals.

## Issue Context
Use one named constant to drive alias creation, the reported alias count, and expected record calculation.

## Fix Focus Areas
- rust/bench_local_inspect.py[21-24]
- rust/bench_local_inspect.py[94-120]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread rust/src/main.rs
Comment on lines +230 to +232
fn display_path(path: &Path) -> String {
path.to_string_lossy().into_owned()
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

5. Non-utf-8 paths collide 🐞 Bug ≡ Correctness

display_path and relative_path replace invalid Unix filename bytes with the same Unicode
replacement character, so distinct valid paths can be emitted identically. Because these lossy
strings are also used as JSON map keys, a later explicit path can silently overwrite an earlier
result, while distinct files can receive duplicate incorrect files[].path values.
Agent Prompt
## Issue description
Unix paths are arbitrary byte sequences, but the Rust inspector converts them with `to_string_lossy()`. Distinct paths can therefore collapse to the same displayed path or JSON key and silently overwrite results.

## Issue Context
The Python implementation traverses filesystem strings using surrogate-escape semantics, whereas the Rust conversion inserts `�`. Use a reversible representation compatible with the expected JSON output, or reject non-UTF-8 paths explicitly before producing any output; do not silently replace bytes.

## Fix Focus Areas
- rust/src/main.rs[230-232]
- rust/src/main.rs[309-314]
- rust/src/main.rs[525-528]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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.

1 participant