fix(mcp-rug-pull): inspect npx commands in JSON configs - #776

[SkillSpector Review]

@efegokdemir thank you for picking up #775 and fixing a gap: RP1 reported the prose mention of an MCP server, but not the JSON definition that actually runs it.

Value and readiness

The approach suggested in the issue is sound. The PR parses .json files, rebuilds command + args, and runs the existing npx matcher and pin check on the result. The single-server case from the issue now reports, and the pinned case stays quiet. One correctness problem blocks merge: every rebuilt command is reported at the line of the file's first "command" key, so in a config with more than one server, findings point at the wrong server.

Material findings

1. Blocker src/skillspector/nodes/analyzers/mcp_rug_pull.py:298-303: command_line_number is computed once per file, from the first "command"\s*: match, and is then used for every rebuilt command.

   • Example: a pretty-printed config whose first server is "local": {"command": "node", "args": ["server.js"]} and whose second is "ctx": {"command": "npx", "args": ["-y", "@upstash/context7-mcp"]}. The ctx finding points at the line of the local server.
   • With two unpinned npx servers, both findings carry the first server's line.
   • SARIF, Markdown and terminal output therefore send reviewers to the wrong server.

   Expected fix:

   • Attribute each command to its own object. For example, locate the "command" key that belongs to each matching object in document order, or search for the package token after that key.
   • Fall back to the first "command" line only when no position can be found.
   • Add a two-server test, with a non-npx or pinned server first and an unpinned one second, that asserts start_line.

2. Optional mcp_rug_pull.py:234: if any args item is not a string (for example a numeric port), the whole object is skipped, and an unpinned package in the same args is missed. Consider joining str() of the scalar items, or joining only the string items.

3. Optional: JSONC files (VS Code mcp.json allows comments) and BOM-prefixed files fail json.loads and silently skip this path. This is no worse than main, so a note or a follow-up is enough.

4. Optional: A few more tests would make the new path safer to change later:

   • a path-qualified runner ("command": "/usr/local/bin/npx");
   • a wrapped runner ("command": "cmd", "args": ["/c", "npx", "-y", "pkg"]);
   • invalid JSON, which should keep the text scan and not raise.

PIC tradeoffs

• Overlap with #646. #646 adds a YAML command/args matcher in the same _check_rp1 and tightens _RP1_NPX_CMD to \bp?npx[ \t]+.
  • The two PRs merge cleanly and do not double-report: #646 needs an unquoted command: key, and this PR only parses .json.
  • After both land, this helper uses #646's regex, and the reconstructed lines still match it.
  • JSON and YAML findings for the same problem would still differ in matched_text shape, message and remediation text. The PIC may want one format before both ship.
• Shared package rule. As in #646's YAML path, and in the shell path since #683, the first non-flag argument is treated as the package. Handling -p/--package/--registry values is a known shared follow-up.

Verification and gaps

• Diff and merge:
  • I read the diff from merge-base ef0f96d.
  • mcp_rug_pull.py is unchanged on main since then, and git merge-tree with current main is clean.
  • git merge-tree with #646 and with #782 is also clean. I inspected the merged _check_rp1 with #646.
• Traces:
  • I traced _iter_config_commands and _check_rp1_npx on the issue's fixture, a two-server config, and a config with a full command string.
  • The raw _RP1_NPX_CMD does not match "command": "npx", because a quote follows npx, so the issue fixture yields exactly one finding.
  • deduplicate() groups findings by fingerprint, so a command reported by both the text scan and the reconstruction collapses into occurrences.
• CI: all 6 checks are green. I did not run the tests locally (policy).

[SkillSpector Review]

Value and readiness

Ready. JSON commands now reuse the existing npx pin logic, and the previously blocking multi-server location problem is fixed for ordinary JSON command keys.

Material findings

No new or still-open required changes found. Verified the prior blocker: the iterator advances one source command-key match per visited command field and yields that field's line. The added node test places a non-npx server first and checks that the unpinned second server is reported on line 8.

PIC tradeoffs

The previous optional limits remain: JSONC/BOM inputs and non-string args do not enter this reconstruction path; npx option-value handling remains a shared detector limitation. Escaped spellings or duplicate JSON command keys can also make regex-derived locations imperfect; a source-aware parser would improve those unusual inputs in a follow-up.

Verification and gaps

Reviewed the complete diff, description, linked issue material, all previous reviews/comments/replies and pinned-head surrounding source at e429a649f0af1216d115b9e6ef7eb2c1a3719d7b. Read the complete earlier marked review, inline blocker, resolution and author reply. Traced depth-first JSON traversal, budget checks, shared pin parsing and raw-text fallback; inspected all three new node regressions. GitHub reports changes: SUCCESS, lint: SUCCESS, test-unit: SUCCESS, OpenCode TypeScript Tests: SUCCESS, DCO Check: SUCCESS, docker-smoke: SUCCESS. No contributor code, tests, imports, build steps or scanner invocations were executed during this review.