[efegokdemir]

Fixes #769.

Malformed dependency manifests now make static inspection incomplete with dependency_parse_error; the package.json fallback still extracts findings when possible. Regression coverage includes malformed npm lockfiles and oversized TOML integers. The regression selection failed on the pre-fix implementation (12 failed); it passes on this branch.

pytest -q tests/nodes/analyzers/test_static_patterns_supply_chain_lockfiles.py tests/nodes/test_analysis_completeness.py — 28 passed. Ruff check and format checks pass for the changed Python files.

[rng1995]

[SkillSpector Review]

@efegokdemir thank you for picking up #769 so directly, and for keeping the package.json fallback findings while making the parse failure visible.

Value and readiness

The problem is real scan-completeness territory. A malformed pyproject.toml, package.json or uv.lock was indistinguishable from an empty manifest, so --fail-on-incomplete passed. The callback design is small, and the valid dependency-free controls guard against false partials. Two changes are needed before final review: the recorded reason describes a different failure, and two parse failures still escape the new handling.

Material findings

1. Blocker, src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:2770: static_parse_limit misdescribes a malformed manifest.
   • Its message (inspection_ledger.py:212) reads "A security-relevant expression exceeded a bounded static parser's span limit." Every report format shows that text for {"dependencies": {.
   • When SKILL.md references the manifest, the AE1 remediation (finalize_inspection_ledger.py:128-132) and docs/ANALYSIS_RESOURCE_BOUNDS.md:117 tell users to correct the scanner. The right action is to fix the manifest.
   • Expected: use a dependency-parse reason. #765 adds LedgerReason.DEPENDENCY_PARSE_ERROR ("Dependency metadata could not be parsed into supported registry coordinates.") for exactly this case. Add the same member here, with the same value and message, so both PRs agree, and update the test assertions.
2. Blocker: two parse failures still escape the new handling.
   • npm lockfiles. package-lock.json and npm-shrinkwrap.json that fail JSON parsing still return [] with no limitation (_npm_lock_entries, L1538-1541; the call at L2808-2809 passes no callback). A truncated lockfile therefore still reports complete, which is #769's symptom, and this PR closes #769.
   • Oversized integers. except tomllib.TOMLDecodeError (L1424, L1611) does not catch the plain ValueError that tomllib raises for integers over CPython's 4,300-digit limit (verified on Python 3.12). A pyproject.toml or uv.lock containing one fails the whole supply-chain analyzer, and guard_analyzer_node then drops unrelated SC2/SC4 findings. #765 had the same issue for Pipenv and now catches (ValueError, RecursionError).
   • Expected: pass the callback through the npm lock path, widen both TOML handlers, and add cases to the parametrized and CLI tests.

PIC tradeoffs

• Dependency files are matched by substring ("package.json" in lower_path, "pyproject.toml" in lower_path). Templates such as package.json.hbs or Jinja-templated manifests will now make scans partial. That is honest fail-closed behavior, but it may add noise for scaffolding skills.
• Overlap with #765: same function, a textual conflict in this module, and different reason codes. One shared code is preferable.

Verification and gaps

• I read the full diff against ef0f96d7 and traced the path from record_parse_error through OsvQueryLimitation(error_class=...), record_limitation, a PARTIAL SYSTEM ledger row and partial finalization to the CLI exit-1 gate. The (path, analyzer, reason) dedup prevents duplicate rows when _collect_locked_versions_detailed parses uv.lock first, without a callback.
• Tests cover three malformed and three valid manifests, fallback retention, and the CLI JSON gate. They assert the reason code, so they need updating with finding 1.
• Main (b07f181d) is 54 commits ahead, and git merge-tree is clean. The PR conflicts with #765.
• CI: all 6 checks are green on b7af89e. I did not run PR code, per policy.

[yashrajp22]

Rechecked this revision with fresh base/head wheels and source scans. The dedicated parse reason and nonempty malformed npm-lock handling are fixed.

The oversized-TOML part of the earlier finding still needs a follow-up: a 5,000-digit integer in pyproject.toml exits 2 in the full CLI. The later dependency_sources._parse_poetry call still catches only TOMLDecodeError, so its ValueError aborts the supply-chain node. A combined node check also loses an earlier SC5 finding. Please cover that later parser as well as this extractor. The two additional gaps are inline below.

Validation: 158 selected pytest executions passed across base/head and source/wheel; all 156 offline CLI reports and 212 direct observations were collected and adjudicated, including the reproduced failures above. Source/wheel results agree. Nine of twelve corpus samples retain unrelated partial-analysis limitations. The valid 1,500-level JSON fixtures did not trigger the predicted recursion errors, so actual recursion-error handling remains uncovered; no recursion-failure claim is made. Live services and global detection accuracy were outside this verification.

[yashrajp22]

Could we limit this parse-error reporting to actual manifest files? A skill containing ordinary prose in docs/pyproject.toml.md now gets dependency_parse_error and exits 1 with --fail-on-incomplete; the base revision completes with exit 0. The same happens with docs/package.json.md and docs/uv.lock.md. The substring checks route these Markdown documents into the parsers, so the new callback turns unrelated documentation into a scan failure. Please distinguish actual manifests and supported templates from ordinary documentation before recording the limitation.

[efegokdemir]

Fixed on 6a31b319: manifest matching keeps requirements/dev.txt and requirements/base.txt in scope while excluding Markdown documentation such as these files. The 29 October review confirms the SC4 advisory cases are retained; all six hosted checks pass on this head.

[yashrajp22]

Could we let zero-byte JSON manifests reach this callback? The node skips them at if not content, so an empty package.json, package-lock.json, or npm-shrinkwrap.json still reports complete and exits 0 with --fail-on-incomplete on this revision. The detailed parser reports the error correctly when called directly. Please distinguish a missing cache entry from an empty string and add full-node or CLI coverage, while preserving the valid empty-TOML controls.

[efegokdemir]

Fixed on 6a31b319: the node distinguishes a missing cache entry from present empty content, so zero-byte JSON manifests reach the parse-error handler while empty TOML remains valid. The 29 October review confirms those controls; all six hosted checks pass on this head.

[rng1995]

[SkillSpector Review]

Value and readiness

The three prior blockers are fixed: the dedicated dependency_parse_error reason is used, malformed npm locks are reported and oversized TOML integers are caught. However, new filename narrowing also removes existing advisory coverage for ordinary requirements directory layouts. That regression needs correction before final review.

Material findings

Blocker: src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:1490-1495 (_is_requirements_file) and :3756 (_is_dependency_manifest gate): requirements/dev.txt or requirements/base.txt containing pyyaml==5.3.1 was previously admitted because its path contained requirements; the new basename-only predicate rejects dev.txt/base.txt and the dependency node skips it. Such files are normal inputs to pip install -r requirements/dev.txt. The bundle can now lose SC4 advisory findings without a parse or coverage exception. Preserve supported requirements-directory layouts while excluding prose templates, and add a regression asserting the vulnerable pin still reaches the advisory check.

PIC tradeoffs

The stricter prose/template filtering is useful, but changing recognized dependency layouts is broader than parse-error reporting and must preserve existing coverage.

Verification and gaps

This was a static review of the complete diff, pinned-head source, added regression assertions, discussion/review history and hosted check results. No contributor code or tests were executed. Traced the new predicate through both static SC1 and dependency-node admission, read parser callback propagation and added malformed/valid/CLI tests. Existing tests cover canonical names and prose suffixes but do not cover requirements/dev.txt. All six hosted checks succeeded; this uncovered path is not covered by them. No PR code was executed.

Reviewed head: eb2fa389fdf76f888c0d83a3373d64a4ee37b32f.

[rng1995]

[SkillSpector Review]

Value and readiness

Ready on the updated head. The previously blocking requirements-directory regression is fixed, and the earlier dedicated-reason, npm-lock, oversized-TOML, documentation and zero-byte concerns remain addressed.

Material findings

No new or still-open required changes found. Verified the latest blocker directly: requirements/dev.txt and requirements/base.txt are admitted by the parent-directory predicate, and node-level tests assert both the exact vulnerable package query and retained SC4 advisory. The other reviewer's still-unresolved UI threads about prose filenames and empty JSON cache values are fixed in the code and have node/CLI controls.

PIC tradeoffs

Filename recognition now distinguishes supported requirements layouts and actual manifests from prose suffixes. The package.json fallback remains available alongside a dependency_parse_error limitation. The duplicate identical DEPENDENCY_PARSE_ERROR message entry is a harmless cleanup opportunity.

Verification and gaps

Read the complete current diff, linked #769, every prior review, inline finding and reply, plus pinned parser/node/ledger context. Verified callback propagation through npm and TOML parsers, widened later Poetry/Cargo handlers, missing-versus-empty cache handling, valid empty-TOML controls, preservation of an earlier SC5 finding and strict CLI outcomes. Hosted checks: changes SUCCESS, lint SUCCESS, test-unit SUCCESS, OpenCode TypeScript Tests SUCCESS, DCO Check SUCCESS, docker-smoke SUCCESS. This is a read-only static review; no contributor code, tests, imports, builds or scanner invocations were executed.

Reviewed head: 6a31b31915b1813c0753e10581c1e0ac2a64a66c.