Skip to content

feat(static): flag unpinned dependencies across manifest formats - #133

Merged
vineethsai7 merged 21 commits into
cisco-ai-defense:mainfrom
federicoroncallo-hub:feat/pr2-unpinned-deps
Aug 3, 2026
Merged

feat(static): flag unpinned dependencies across manifest formats#133
vineethsai7 merged 21 commits into
cisco-ai-defense:mainfrom
federicoroncallo-hub:feat/pr2-unpinned-deps

Conversation

@federicoroncallo-hub

@federicoroncallo-hub federicoroncallo-hub commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Skill packages are end-user applications, so an unpinned dependency (requests, requests>=2) lets a later — potentially compromised — release be installed automatically. The static analyzer did not surface this supply-chain risk.

This PR adds _check_dependency_pinning() to the static analyzer, emitting SUPPLY_CHAIN_UNPINNED_DEPENDENCY (MEDIUM for open/unpinned, LOW for wildcard ==1.* pins). It reads dependencies from every common declaration site:

  • requirements*.txt
  • pyproject.toml[project] dependencies and optional-dependencies (PEP 621)
  • setup.cfg[options] install_requires / extras
  • setup.pyinstall_requires=[...] string literals (parsed via AST, never executed)
  • Pipfile[packages] / [dev-packages]
  • manifest metadata.dependencies

A present lockfile (uv.lock, poetry.lock, pipfile.lock, requirements.lock) suppresses findings, since versions are already frozen. Exact == pins and artifact/VCS references are treated as safe.

Registered SUPPLY_CHAIN_UNPINNED_DEPENDENCY in data/packs/core/pack.yaml; updated the static-analyzer docs and threat taxonomy.

Note: TOML sources (pyproject.toml, Pipfile) rely on tomllib and are therefore only parsed on Python 3.11+; on 3.10 they are skipped (the feature degrades gracefully, other sources still scanned).

Test plan

  • tests/static_analysis/test_dependency_pinning.py — classifier unit tests + integration tests across all sources (TOML tests skip on <3.11).
  • uv run pytest tests/ passes.
  • uv run pre-commit run --all-files passes.
  • uv run python evals/runners/benchmark_runner.py — 100% precision/recall, no regression.

Made with Cursor

Summary by CodeRabbit

  • New Features

    • Added detection for dependencies without exact version pinning across common project configuration and manifest files.
    • Reports supply-chain risks with severity, source, and line details.
    • Recognized lockfiles suppress duplicate dependency warnings.
  • Documentation

    • Updated analyzer flow and threat coverage documentation to include dependency-pinning checks.

federicoroncallo-hub and others added 5 commits July 2, 2026 11:22
…et/localtunnel.me

ngrok migrated off ngrok.io to ngrok-free.dev / ngrok.app, so exfil to a
current ngrok endpoint slipped past the suspicious-domain checks. Add the
modern ngrok domains plus bore.pub, serveo.net and localtunnel.me to:

- ContextExtractor.SUSPICIOUS_DOMAINS (Python string-literal URL classification)
- tool_chaining_abuse_generic.yara / command_injection_generic.yara exfil dests
- cross_skill_scanner exfil pattern list

Add regression tests for the new domains (YARA true-positives + ContextExtractor).

Co-authored-by: Cursor <cursoragent@cursor.com>
Skill packages are end-user applications, so unpinned dependencies
(requests>=2 or a bare requests) let a later, potentially compromised
release be pulled in at install time -- a supply-chain risk. Only the LLM
prompt mentioned this previously; there was no deterministic check.

Add StaticAnalyzer._check_dependency_pinning() which parses requirements*.txt
files and a manifest metadata "dependencies" list, emitting
SUPPLY_CHAIN_UNPINNED_DEPENDENCY (MEDIUM for open ranges/bare names, LOW for
==x.* wildcard pins). Skips when a lockfile (uv.lock/poetry.lock/...) is
present, since versions are already resolved -- this is why the scanner's own
library pinning policy (ranges in pyproject.toml) is not affected.

Register the rule in the core pack.yaml, document the new pass in the
static-analyzer docs and threat taxonomy, and add unit tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
The suspicious/legitimate domain lists and matching logic previously lived
inside ContextExtractor and only ran over Python AST string literals, so a
tunnel/proxy endpoint hidden in a config value (e.g. base_url in config.yaml)
was never classified.

Refactor (behavior-preserving): extract SUSPICIOUS_DOMAINS/LEGITIMATE_DOMAINS
and the classification into skill_scanner/core/static_analysis/url_classifier.py
(classify_url + extract_urls). ContextExtractor imports from it and keeps the
lists as class attributes for backward compatibility; existing suspicious-URL
behavior is unchanged (covered by refactor-safety tests).

New pass: StaticAnalyzer._scan_config_files() parses config.yaml/.yml/.json,
settings.*, and *.toml (regex fallback on parse failure), runs each URL through
the shared classifier, and emits CONFIG_SUSPICIOUS_URL (HIGH). Registered in
the core pack.yaml.

Docs updated (static-analyzer, behavioral-analyzer). Adds unit tests plus a
labeled config-routed exfil eval sample; benchmark stays at 100% P/R.

Co-authored-by: Cursor <cursoragent@cursor.com>
Collapse the structured YAML/JSON/TOML parse-and-walk in config URL
scanning down to a single raw-text extract_urls() pass. The raw scan is
simpler, drops the json/yaml/tomllib imports and the _iter_string_values
recursion, and additionally catches suspicious URLs hidden in config
comments. classify_url() still only flags known tunnel/exfil domains, so
false-positive risk stays low.

Co-authored-by: Cursor <cursoragent@cursor.com>
Extend the unpinned-dependency check beyond requirements*.txt and
manifest metadata to also read pyproject.toml ([project] dependencies
and optional-dependencies), setup.cfg ([options] install_requires /
extras), setup.py (install_requires literals via AST), and Pipfile
([packages]/[dev-packages]). All sources are normalized to requirement
strings and run through the existing classifier, so a lockfile still
suppresses findings and pinned specs stay clean.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The static analyzer now checks Python dependency declarations for unpinned requirements across supported project files and manifest metadata. It suppresses findings for lockfiles, classifies pin types, emits supply-chain findings, registers the rule, and adds documentation and tests.

Changes

Dependency Pinning Detection

Layer / File(s) Summary
Core dependency pinning implementation
skill_scanner/core/analyzers/static.py
Adds dependency parsers, requirement classification, lockfile suppression, malformed-input handling, analyzer integration, and SUPPLY_CHAIN_UNPINNED_DEPENDENCY findings with source and line details.
Rule registration and documentation
skill_scanner/data/packs/core/pack.yaml, docs/architecture/analyzers/static-analyzer.md, docs/architecture/threat-taxonomy.md
Registers the enabled rule and documents its analysis pass and supply-chain coverage.
Dependency pinning validation
tests/static_analysis/test_dependency_pinning.py
Tests classification, supported dependency sources, finding metadata, lockfile suppression, VCS references, development requirements, and malformed setup.py input.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant StaticAnalyzer
  participant DependencyPinning as _check_dependency_pinning
  participant DependencyFiles
  participant Findings
  StaticAnalyzer->>DependencyPinning: Run dependency-pinning pass
  DependencyPinning->>DependencyFiles: Parse declarations and manifest metadata
  DependencyFiles-->>DependencyPinning: Return dependency entries
  DependencyPinning->>DependencyPinning: Classify pins and apply lockfile suppression
  DependencyPinning-->>Findings: Emit unpinned dependency findings
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: static analysis flags unpinned dependencies across supported manifest formats.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
skill_scanner/core/analyzers/static.py (2)

595-596: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Static analysis: annotate mutable class attribute with ClassVar.

Ruff RUF012 flags _LOCKFILE_NAMES as a mutable class-level default. It's never mutated here, so this is stylistic, but annotating clarifies intent and silences the linter.

♻️ Proposed fix
-    _LOCKFILE_NAMES = {"uv.lock", "poetry.lock", "pipfile.lock", "requirements.lock"}
+    _LOCKFILE_NAMES: ClassVar[set[str]] = {"uv.lock", "poetry.lock", "pipfile.lock", "requirements.lock"}
🤖 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 `@skill_scanner/core/analyzers/static.py` around lines 595 - 596, Annotate the
static analyzer’s `_LOCKFILE_NAMES` class attribute in `StaticAnalyzer` with
`ClassVar` to make its intended class-level, non-instance mutable default
explicit and satisfy Ruff RUF012. Update the declaration where `_LOCKFILE_NAMES`
is defined in `skill_scanner/core/analyzers/static.py`, keeping the existing
lockfile set unchanged and ensuring the type hint clearly marks it as a
`ClassVar`.

Source: Linters/SAST tools


685-704: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Comma-splitting install_requires blocks can fragment multi-specifier requirements.

block.replace(",", "\n").splitlines() splits on every comma, including commas inside a single requirement's specifier list (e.g. requests>=2.0,<3.0), producing an orphan line like <3.0 that _classify_requirement silently drops (fails _REQUIREMENT_RE). The primary spec requests>=2.0 is still correctly flagged as unpinned in this example, so there's no false negative here, but for lines where the first comma-separated part alone would classify as pinned (e.g. requests==2.0,!=2.5) the wildcard/exclusion nuance from the remainder is lost. Low practical impact given current test coverage passes.

🤖 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 `@skill_scanner/core/analyzers/static.py` around lines 685 - 704, The
comma-based splitting in `_entries_from_setup_cfg` is breaking valid requirement
specifiers that contain commas, which can cause `_classify_requirement` to miss
important constraint details. Update the parsing logic in
`_entries_from_setup_cfg` to avoid treating commas inside a single requirement
line as entry separators, and preserve the full requirement string when building
`entries`. Keep the fix localized to the setup.cfg handling path so
`install_requires` and `options.extras_require` are still extracted correctly
without fragmenting multi-specifier requirements.
🤖 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.

Nitpick comments:
In `@skill_scanner/core/analyzers/static.py`:
- Around line 595-596: Annotate the static analyzer’s `_LOCKFILE_NAMES` class
attribute in `StaticAnalyzer` with `ClassVar` to make its intended class-level,
non-instance mutable default explicit and satisfy Ruff RUF012. Update the
declaration where `_LOCKFILE_NAMES` is defined in
`skill_scanner/core/analyzers/static.py`, keeping the existing lockfile set
unchanged and ensuring the type hint clearly marks it as a `ClassVar`.
- Around line 685-704: The comma-based splitting in `_entries_from_setup_cfg` is
breaking valid requirement specifiers that contain commas, which can cause
`_classify_requirement` to miss important constraint details. Update the parsing
logic in `_entries_from_setup_cfg` to avoid treating commas inside a single
requirement line as entry separators, and preserve the full requirement string
when building `entries`. Keep the fix localized to the setup.cfg handling path
so `install_requires` and `options.extras_require` are still extracted correctly
without fragmenting multi-specifier requirements.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b7060b6e-11cd-4d8c-9d54-a90560e0de1b

📥 Commits

Reviewing files that changed from the base of the PR and between 41fec4a and d7d4790.

📒 Files selected for processing (5)
  • docs/architecture/analyzers/static-analyzer.md
  • docs/architecture/threat-taxonomy.md
  • skill_scanner/core/analyzers/static.py
  • skill_scanner/data/packs/core/pack.yaml
  • tests/static_analysis/test_dependency_pinning.py

Comment thread skill_scanner/core/analyzers/static.py
gyrospectre and others added 6 commits July 24, 2026 08:47
ProviderConfig.validate() required a truthy credential for every provider
except Bedrock and Ollama, and the only credential source it checked for
Vertex was GOOGLE_APPLICATION_CREDENTIALS. This blocked ambient auth via
a GCE/Cloud Run attached service account or Workload Identity, even though
LiteLLM/google-auth already fall back to it automatically when no explicit
credential is passed -- the same pattern already supported for Bedrock's
IAM role. Excludes is_vertex from the check, mirroring the Bedrock/Ollama
precedent, and documents the fallback.
- _resolve_api_key() now returns None for Vertex instead of the
  GOOGLE_APPLICATION_CREDENTIALS path, since vertex_ai/gemini-* models
  set both is_vertex and is_gemini, which was causing the file path to
  be written into GEMINI_API_KEY.
- Regenerated configuration-reference.md via generate_reference_docs.py
  instead of hand-editing, and updated the underlying descriptions so
  the doc doesn't drift on next regeneration.

Addresses CodeRabbit review feedbak on cisco-ai-defense#144.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/static_analysis/test_dependency_pinning.py (1)

225-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise malformed dependency declarations in this regression test.

The input on Line 227 contains no install_requires entry. A parser that skips all setup.py dependency declarations would still pass this test. Include an unpinned dependency before \x00, then verify that analysis completes without a SUPPLY_CHAIN_UNPINNED_DEPENDENCY finding for setup.py.

Proposed test input
-        skill = make_skill({"setup.py": "setup(name='demo')\x00"})
+        skill = make_skill(
+            {
+                "SKILL.md": _SKILL_MD,
+                "setup.py": (
+                    "setup(name='demo', "
+                    "install_requires=['requests>=2.0'])\x00"
+                ),
+            }
+        )
🤖 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 `@tests/static_analysis/test_dependency_pinning.py` around lines 225 - 231,
Update test_setup_py_with_null_bytes_is_ignored to include an unpinned
install_requires dependency before the null byte in setup.py, then assert
analysis completes without a SUPPLY_CHAIN_UNPINNED_DEPENDENCY finding for
setup.py. Keep the existing malformed-input and no-abort coverage intact.
🤖 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.

Nitpick comments:
In `@tests/static_analysis/test_dependency_pinning.py`:
- Around line 225-231: Update test_setup_py_with_null_bytes_is_ignored to
include an unpinned install_requires dependency before the null byte in
setup.py, then assert analysis completes without a
SUPPLY_CHAIN_UNPINNED_DEPENDENCY finding for setup.py. Keep the existing
malformed-input and no-abort coverage intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a200558b-7d01-4f87-882b-377e633d6608

📥 Commits

Reviewing files that changed from the base of the PR and between d7d4790 and 93ba6f0.

📒 Files selected for processing (2)
  • skill_scanner/core/analyzers/static.py
  • tests/static_analysis/test_dependency_pinning.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • skill_scanner/core/analyzers/static.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
tests/test_llm_analyzer.py (1)

65-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise request-time Vertex authentication.

This test stops after LLMAnalyzer construction. It cannot detect ADC or project-resolution failures during the first LiteLLM request. Add a mocked request test that supplies fake ADC/project resolution and asserts that Vertex uses ambient credentials without an API key. LiteLLM performs this resolution during request handling. (raw.githubusercontent.com)

This review uses the supplied LiteLLM 1.84.0 context.

🤖 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 `@tests/test_llm_analyzer.py` around lines 65 - 74, Extend
test_init_vertex_without_api_key to exercise a mocked LiteLLM request after
constructing LLMAnalyzer, mocking ADC credential and project-resolution calls
with fake values. Assert the request succeeds and Vertex authentication uses the
ambient credentials without supplying an API key, while preserving the existing
provider_config and analyzer.api_key assertions.

Source: MCP 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 `@docs/reference/dependencies-and-llm-providers.md`:
- Around line 111-112: Document Vertex project and region values as conditional
requirements rather than implying ambient ADC always supplies them. Update the
Vertex ADC wording in docs/reference/dependencies-and-llm-providers.md lines
111-112, mirror the conditional vertex_ai/* fallback requirement in
docs/reference/configuration-reference.md line 61, and revise the
GOOGLE_APPLICATION_CREDENTIALS guidance in scripts/generate_reference_docs.py
lines 417-422 so it explains that explicit credentials are needed only when the
environment does not provide the required Vertex project and region.

In `@skill_scanner/core/analyzers/llm_provider_config.py`:
- Around line 176-181: Update the Vertex AI documentation comment near the
Vertex branch to state that it returns None only when no explicit api_key is
supplied, while preserving the explanation of GOOGLE_APPLICATION_CREDENTIALS and
ambient Application Default Credentials.

---

Nitpick comments:
In `@tests/test_llm_analyzer.py`:
- Around line 65-74: Extend test_init_vertex_without_api_key to exercise a
mocked LiteLLM request after constructing LLMAnalyzer, mocking ADC credential
and project-resolution calls with fake values. Assert the request succeeds and
Vertex authentication uses the ambient credentials without supplying an API key,
while preserving the existing provider_config and analyzer.api_key assertions.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7249b1fe-83c0-4e40-bf4e-5c496126fb1e

📥 Commits

Reviewing files that changed from the base of the PR and between 93ba6f0 and 8b50b49.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • docs/architecture/analyzers/llm-analyzer.md
  • docs/features/index.md
  • docs/reference/configuration-reference.md
  • docs/reference/dependencies-and-llm-providers.md
  • pyproject.toml
  • scripts/generate_reference_docs.py
  • skill_scanner/core/analyzers/llm_provider_config.py
  • tests/test_llm_analyzer.py

Comment thread docs/reference/dependencies-and-llm-providers.md
Comment thread skill_scanner/core/analyzers/llm_provider_config.py
@codecov-commenter

codecov-commenter commented Aug 3, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 90.53254% with 16 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
skill_scanner/core/analyzers/static.py 90.53% 16 Missing ⚠️

📢 Thoughts on this report? Let us know!

# Conflicts:
#	skill_scanner/core/static_analysis/context_extractor.py
# Conflicts:
#	docs/architecture/analyzers/static-analyzer.md
#	skill_scanner/core/analyzers/static.py
#	skill_scanner/data/packs/core/pack.yaml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (4)
skill_scanner/core/analyzers/static.py (4)

304-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Update the docstring to reflect the actual pass order.

The docstring lists 6 passes, ending with "5. Dependency pinning checks" then "6. Reference file scanning". The actual analyze() body calls self._scan_config_files(skill) between _check_dependency_pinning and _scan_referenced_files (Line 321). This pass is not listed in the docstring, so the numbered contract at Lines 304-306 no longer matches the code.

📝 Proposed fix
         Performs multi-pass scanning:
         1. Manifest validation
         2. Instruction body scanning (SKILL.md)
         3. Script/code scanning
         4. Consistency checks
         5. Dependency pinning checks
-        6. Reference file scanning
+        6. Configuration file URL scanning
+        7. Reference file scanning

Also applies to: 316-329

🤖 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 `@skill_scanner/core/analyzers/static.py` around lines 304 - 306, Update the
pass-order docstring for the analyzer method containing analyze() to include
_scan_config_files(skill) between _check_dependency_pinning and
_scan_referenced_files, renumbering the subsequent entries so the documented
sequence matches the actual analyze() call order.

839-923: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Config-file URL scanning feature is absent from the PR objectives and AI summary.

_is_config_file, _scan_config_files, _find_line_number, and _redact_url_for_finding are marked as newly added in this diff, and the new call to _scan_config_files at Line 321 wires this feature into analyze(). The PR objectives, AI summary, and line_range_change_details describe this PR solely as adding unpinned-dependency detection (SUPPLY_CHAIN_UNPINNED_DEPENDENCY); none of them mention a CONFIG_SUSPICIOUS_URL rule or configuration-file URL scanning. If this block landed here from a different, already-reviewed layer of the stack, no action is needed; otherwise the summary should describe this second feature.

Separately, static analysis flags _CONFIG_FILE_STEMS = {"config", "settings"} (Line 840) for RUF012 (mutable class attribute). The file already uses frozenset(...) for other class-level constants (e.g. _TAG_BOUNDARY_CODEPOINTS); apply the same pattern here for consistency and to silence the lint.

🧹 Proposed fix
-    _CONFIG_FILE_STEMS = {"config", "settings"}
+    _CONFIG_FILE_STEMS = frozenset({"config", "settings"})
🤖 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 `@skill_scanner/core/analyzers/static.py` around lines 839 - 923, Update the
class-level _CONFIG_FILE_STEMS constant to use an immutable frozenset, matching
the existing pattern for class-level constants such as _TAG_BOUNDARY_CODEPOINTS;
leave the configuration URL scanning behavior unchanged.

Source: Linters/SAST tools


651-669: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent TOML skip on Python 3.10 gives no operator signal.

_safe_toml returns None when tomllib is unavailable (Python 3.10, per the PR summary: "TOML sources are parsed on Python 3.11+ and skipped on Python 3.10"). This means _entries_from_pyproject and _entries_from_pipfile silently produce zero entries on 3.10, so unpinned dependencies in pyproject.toml and Pipfile go completely undetected without any indication in logs. For a security scanner, a silent detection gap tied to interpreter version is easy to miss in production.

Add a one-time debug/warning log when tomllib is None so operators running under Python 3.10 know TOML-based dependency checks are skipped.

🔍 Proposed fix
     `@staticmethod`
     def _safe_toml(content: str) -> dict | None:
         """Parse TOML, returning None when unavailable (py<3.11) or malformed."""
         if tomllib is None:
+            logger.debug("tomllib unavailable (Python <3.11); skipping TOML dependency parsing")
             return None
         try:
             return tomllib.loads(content)
         except Exception:  # noqa: BLE001 - malformed manifest, treat as no data
             return None
🤖 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 `@skill_scanner/core/analyzers/static.py` around lines 651 - 669, Update
_safe_toml to emit a one-time debug or warning log when tomllib is None, clearly
stating that TOML dependency checks are skipped because TOML parsing is
unavailable on the current Python version. Reuse the analyzer’s existing logger
and one-time logging mechanism if present, while preserving the current None
return behavior.

671-687: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift

PEP 621-only parsing misses Poetry-native and build-system dependencies.

_entries_from_pyproject only reads project.dependencies and project.optional-dependencies (PEP 621). Two common dependency declarations in pyproject.toml are not covered:

  • [tool.poetry.dependencies] / [tool.poetry.dev-dependencies] (Poetry's legacy format, still widely used), which is a table of name = "constraint" pairs, not a list of PEP 508 strings.
  • [build-system] requires = [...], which can itself carry unpinned build dependencies (e.g. "setuptools>=61.0").

Skill packages using Poetry's native format would get zero SUPPLY_CHAIN_UNPINNED_DEPENDENCY findings even with fully open version ranges. Consider extending this parser (or adding a sibling parser) to cover tool.poetry.dependencies and build-system.requires.

🤖 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 `@skill_scanner/core/analyzers/static.py` around lines 671 - 687, The
_entries_from_pyproject method only parses PEP 621 dependency fields; extend it
to also collect Poetry dependencies from tool.poetry.dependencies and
tool.poetry.dev-dependencies, converting each name/constraint entry into the
scanner’s expected dependency-string format, and collect build dependencies from
build-system.requires. Preserve existing parsing and return tuples so unpinned
Poetry and build-system requirements are analyzed.
🤖 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.

Nitpick comments:
In `@skill_scanner/core/analyzers/static.py`:
- Around line 304-306: Update the pass-order docstring for the analyzer method
containing analyze() to include _scan_config_files(skill) between
_check_dependency_pinning and _scan_referenced_files, renumbering the subsequent
entries so the documented sequence matches the actual analyze() call order.
- Around line 839-923: Update the class-level _CONFIG_FILE_STEMS constant to use
an immutable frozenset, matching the existing pattern for class-level constants
such as _TAG_BOUNDARY_CODEPOINTS; leave the configuration URL scanning behavior
unchanged.
- Around line 651-669: Update _safe_toml to emit a one-time debug or warning log
when tomllib is None, clearly stating that TOML dependency checks are skipped
because TOML parsing is unavailable on the current Python version. Reuse the
analyzer’s existing logger and one-time logging mechanism if present, while
preserving the current None return behavior.
- Around line 671-687: The _entries_from_pyproject method only parses PEP 621
dependency fields; extend it to also collect Poetry dependencies from
tool.poetry.dependencies and tool.poetry.dev-dependencies, converting each
name/constraint entry into the scanner’s expected dependency-string format, and
collect build dependencies from build-system.requires. Preserve existing parsing
and return tuples so unpinned Poetry and build-system requirements are analyzed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6fd6d0d5-4710-40da-b26f-7e43b45e389e

📥 Commits

Reviewing files that changed from the base of the PR and between 8b50b49 and ea9eb7e.

📒 Files selected for processing (3)
  • docs/architecture/analyzers/static-analyzer.md
  • skill_scanner/core/analyzers/static.py
  • skill_scanner/data/packs/core/pack.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • skill_scanner/data/packs/core/pack.yaml
  • docs/architecture/analyzers/static-analyzer.md

@vineethsai7
vineethsai7 merged commit 0f44aff into cisco-ai-defense:main Aug 3, 2026
8 checks passed
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.

4 participants