fix(rp1): prevent npx matches across lines

[agentsope]

Summary

Closes #639.

• Keep shell npx/pnpx matches on one line, preventing frontmatter and wrapped prose from becoming commands.
• Add a bounded YAML MCP command/args matcher for npx/pnpx and path-qualified runners. Match sibling args before or after command/cmd, including list-item mappings and intervening fields; preserve version-pin checks on the package operand.
• Accept indentless YAML block sequences, including default PyYAML output. Nested env/description lines no longer consume the eight-sibling search window.
• Strip unquoted YAML comments and preserve empty quoted arguments without crashing or shifting a later argument into the package position.
• Preserve mapping/list-item boundaries, finding locations and shared runtime checks.

Bounds and scope

The sibling search considers up to eight sibling keys in each direction, with a separate hard cap of 256 physical lines per direction. Argument collection has a 128-physical-line cap. Extremely large configurations beyond these bounds can remain unrecognized; this is a bounded text matcher, not full YAML semantic analysis. Tests cover both sides of each limit.

JSON MCP configuration support, npx option-value parsing (-p/--package/--registry), cross-line handling for uvx/pip/docker and optional single-line report presentation are separate follow-ups, not addressed here.

Validation

• Latest review reproductions: 24 failures and 24 passing controls before this follow-up.
• Both MCP rug-pull test files: 163 passed after the fix.
• Independent synthetic validation: 1,152 parsed YAML layouts across field order, runners, mapping/list forms, flow/block arguments, pin states and LF/CRLF. Only configuration text was analyzed; no sample commands executed.
• Repository-wide Ruff lint/format, targeted mypy and whitespace checks passed.
• Full non-integration/non-provider suite: 8,763 passed, 14 skipped, 134 deselected, 4 xfailed in 352.54 seconds. Four warnings concern existing timeout markers and a duplicate ZIP-member fixture. No live or paid provider calls. The local Anaconda readline workaround was bypassed only in the pytest launcher, without editing project or environment files.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[rng1995]

[SkillSpector Review]

Hi @agentsope, thank you for your contribution to SkillSpector — we really appreciate the time you put into this! A few items need attention before it can be merged; details below.

This PR makes _RP1_NPX_CMD match on a single line only ([ \t]+ instead of \s+) and adds \b before npx. That fixes the cross-line false positives in #639: name: npx followed by the next frontmatter line, and prose that wraps after "npx". I compared the old and new regexes, including the pin check, on shell, JSON, YAML and TOML MCP-config forms.

The single-line restriction is correct for shell commands. npx -y …, npx --yes …, unpinned pkg@latest and pinned @x.y.z behave exactly as on main, and CRLF is unaffected. However, the change also removes two things that main currently detects.

Findings

1. [Blocking] src/skillspector/nodes/analyzers/mcp_rug_pull.py:125, tests/test_mcp_rug_pull.py:62-66: the \b stops pnpx from matching. pnpx is pnpm's npx-style runner (an alias of pnpm dlx). Like npx, it fetches and runs the latest version of an unpinned package. pnpx @scope/mcp-server reports RP1 on main and nothing on this head, and the new test_rp1_npx_requires_a_word_boundary asserts that miss.

   Please keep matching the runner, e.g. \bp?npx[ \t]+… or (?<![\w-])p?npx[ \t]+…, and change the test to expect RP1 for pnpx. If you want a word-boundary test, use a non-runner identifier. Optionally, as a follow-up: bunx, pnpm dlx and yarn dlx are not detected on main either.

2. [Blocking] src/skillspector/nodes/analyzers/mcp_rug_pull.py:125: YAML MCP configs lose RP1 coverage. On main, command: npx (or Goose's cmd: npx) on its own line, followed by args:, matched across the newline, and the pin check then read the args line. For example:

   mcpServers:
     fs:
       command: npx
       args: ["-y", "@scope/server"]

   This reported RP1 on main, as did the block-list form used by Continue and Goose (args: followed by - "-y" / - "@scope/server"). The pinned flow form ("@scope/server@1.2.3") was correctly skipped. On this head neither form produces RP1.

   Nothing else covers them. The manifest check runs the same regex over str(manifest), where a quote follows npx ('command': 'npx'). The coverage on main was accidental (the matched text is npx\n args), but it is the only RP1 coverage YAML MCP configs have. Before dropping cross-line matching, please add an explicit, bounded matcher: a command/cmd key whose value is npx/pnpx, followed within a few lines by an args list (flow or block) whose package token is checked with _VERSION_PIN_RE. Please include tests where an unpinned config fires and a pinned one produces no finding.

3. [Non-blocking] JSON configs ("command": "npx", "args": ["-y", "pkg@latest"], single-line or multi-line) are detected neither on main nor on this head, because the regex needs whitespace right after npx and JSON has a closing quote there. This is not a regression. The matcher from (2) could also cover .mcp.json / claude_desktop_config.json, which are the most common MCP config format.

4. [Non-blocking] _RP1_UVX_CMD, _RP1_PIP_INSTALL and _RP1_DOCKER_CMD (L128–139) still use \s+ and have the same cross-line false positive. For example, name: uvx\ndescription: repro produces uvx\ndescription, and "…with docker run" followed by --rm example produces docker run\n--rm. Consider applying the same fix, together with the config-aware handling, here or in a follow-up.

5. [Note] #641 also edits mcp_rug_pull.py (_RugPullBudget.emit) and tests/test_mcp_rug_pull.py (test_rp1_npx_unpinned). Those hunks do not overlap with this PR's and merge cleanly.

Tests/CI
All checks pass. The new tests cover the #639 false positives and same-line flags. However, the word-boundary test locks in a false negative, and no test covers MCP config forms (YAML in particular) or tabs.

---

Decision: Changes Requested (reviewed head 0db07011e7db94302b6bb4f4120a4a8d33ce5cea)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[rng1995]

[SkillSpector Review]

Hi @agentsope, thank you for the follow-up commit that brings back pnpx and adds a dedicated YAML matcher!

Value and readiness: The single-line npx regex fixes the #639 false positives from frontmatter and wrapped prose, and pnpx is detected again. The new YAML matcher restores RP1 for the common layout where args comes right after command: npx. However, other YAML layouts that main reports are still lost. In two of them, a skill author can hide an unpinned server just by reordering or adding keys. One more change is needed before merge.

Previous findings:

• 1. pnpx dropped by \b: Resolved. _RP1_NPX_CMD is now \bp?npx[ \t]+… (mcp_rug_pull.py:125). test_rp1_pnpx_unpinned asserts the finding, and the word-boundary test now uses foonpx.
• 2. YAML MCP configs lose RP1: Partly resolved, still open. _iter_config_npx_commands covers command:/cmd: followed directly by a flow or block args list, with unpinned and pinned tests. The remaining gaps are in Material finding 1.
• 3. JSON configs not detected (non-blocking): Still open. This is not a regression and is fine as a follow-up.
• 4. Cross-line false positives in uvx/pip/docker (non-blocking): Still open. _RP1_UVX_CMD, _RP1_PIP_INSTALL and _RP1_DOCKER_CMD still use \s+. Fine as a follow-up.
• 5. Overlap with #641: Resolved. #641 is merged, and this head merges cleanly with current main.

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/mcp_rug_pull.py:202, :128: the matcher only accepts args as the next sibling key. It also never matches when command is the first key of a list item. Both of these valid configs report RP1 on main and nothing on this head:

   mcpServers:
     fs:
       command: npx
       env:
         FOO: bar
       args: ["-y", "@scope/server"]

   servers:
     - command: npx
       args: ["-y", "@scope/server"]

   The same happens with type: stdio, cwd: or description: between command and args. At L202, the first key at the same indent that is not args ends the search. _RP1_CONFIG_RUNNER (L128) does not allow a leading - . Two more layouts that main reports are also missed: args placed before command, and a path-qualified runner (command: /usr/local/bin/npx). Adding env: {} between the two keys is enough to hide an unpinned server.

   Please search all sibling keys of the same mapping, before and after the command line. Stop when indentation drops below the command key, and keep the line bound. Treat - command: as a key at the column after - , and preferably accept a runner path ending in /npx or /pnpx. Please add tests for an intervening key, the list-item form and args before command, each with a pinned version that produces no finding.

2. [Non-blocking] src/skillspector/nodes/analyzers/mcp_rug_pull.py:226-227: YAML comments on the args line are tokenized as arguments. Take args: # "@scope/server@1.2.3" followed by - "@scope/server": the commented string becomes the package, the pin check passes, and the unpinned server is not reported. main has the same gap, so this is not a regression. The new matcher already handles a # comment on the command line. Dropping an unquoted #… tail from each args line before tokenizing would close this gap.

3. [Non-blocking] src/skillspector/nodes/analyzers/mcp_rug_pull.py:338: the YAML finding's message and matched_text include the raw multi-line block, e.g. 'command: npx\n args: [...]'. Consider naming the runner and package in the message (e.g. npx @scope/server) so terminal, Markdown and SARIF output stay on one line.

PIC tradeoffs: None identified.

Verification and gaps:

• Since the last review, the only new commit is 3f8e7d2. It is an author change, not a merge of main. The full diff against the merge base contains only the regex change, the YAML matcher and its tests.
• I compared main's and this head's npx regex literals in an isolated harness, and traced _iter_config_npx_commands by hand on the YAML inputs above. Adjacent flow and block args fire, and the pinned adjacent forms are skipped. The intervening env/type, list-item, args-first and path-qualified forms fire on main and not here.
• Shell forms behave as before: npx -y …, pnpx …, CRLF. A YAML line with a full shell command (command: npx -y pkg) is reported once, because the config runner regex requires a bare runner value.
• Overlap with #683: the two PRs change different hunks, and git merge-tree of both heads is clean. #683's check, which only accepts a pin on the package token, is consistent with this matcher's check of the first package token.
• CI: all 6 checks are green. Per policy, I did not run the tests locally.

---

Decision: Changes Requested (reviewed head 3f8e7d2331097ff3529a822ff9bbd25b1abe065a)

[agentsope]

Thanks for the follow-up review. I've addressed the remaining YAML coverage blocker and the related comment-tokenization gap, and merged current main without rewriting the previously reviewed commits.

Updated head: 7f3ddfd8f1b533378d308dd30eabb78363fbf2b3 (DCO signed).

The bounded matcher now searches sibling keys on either side of command/cmd, permits intervening configuration fields, handles a list item's first key at the column after - , and recognizes runner paths ending in /npx or /pnpx. Traversal stops at the enclosing mapping/list-item boundary and retains the eight-line search bound and shared runtime checks. Unquoted YAML comments are removed before argument tokenization, while quoted hashes are retained.

Regression controls cover unpinned and pinned packages in ten layouts, separate mappings/list items, nested decoy args, fake pins in comments, CRLF, command-line locations and both sides of the line bound. The original frontmatter/wrapped-prose false-positive controls and pnpx behavior remain covered. An independent synthetic check also verified all 288 permutations of four sibling fields, three runners, mapping/list forms and pin states against parsed YAML semantics.

Validation:

• Before the fix: 12 failures and 16 passes in the new reproduction/control selection.
• After the fix: all 75 tests in the two MCP rug-pull test files pass.
• Whole-source/test Ruff lint and format checks, targeted mypy, and git diff --check pass.
• Full offline unit selection: 8,675 passed, 14 skipped, 134 deselected and 4 expected failures. Live integration/provider tests were excluded. The local Anaconda readline issue was bypassed only in the pytest launcher; project files/environment were not changed for that workaround.

This follow-up does not expand JSON support, other runner regexes, or the optional multi-line report presentation change. Please re-check the YAML blocker on the updated head once hosted CI completes.

[Copilot]

Copilot review overview

🟡 Changes recommended

Address the empty-argument crash and bounded-memory concerns before approval.

Review effort: Lite
Findings: 1

Open (1)

• Preserve empty quoted arguments to prevent analyzer crash · New

[agentsope]

Thanks for flagging the empty quoted argument case. I've reproduced and fixed the crash when an empty argument is encountered before the first package operand.

Updated head: ed2d3a6777490dab526cdfba975cd776f6b1e69a (DCO signed).

The tokenizer now chooses the quoted capture with an explicit is not None check, preserving ""/'' instead of converting them to None. If the first positional argument is empty, the matcher stops without treating a later argument as the package. Empty arguments after an actual package retain the existing behavior: unpinned packages are reported and pinned packages are skipped. The trailing-empty example itself already stops after selecting the package; that case is now an explicit regression control too.

Added 29 deterministic cases covering single/double quotes, flow/block YAML args, leading/flag-following/trailing empty arguments, pinned controls, and continued scanning of another server and file. The existing line bounds and runtime checks are unchanged; this is scoped to empty-argument handling.

Validation:

• Before the fix: 20 failures and 8 passes in the empty-argument matrix.
• After the fix: all 104 tests in both MCP rug-pull test files pass.
• An independent check of 84 parsed synthetic configs covering npx/pnpx/path runners, args-before/after-command, and LF/CRLF passes with completed ledger accounting.
• Repository-wide Ruff lint/format checks, targeted mypy and git diff --check pass.
• Full offline unit selection: 8,704 passed, 14 skipped, 134 deselected and 4 expected failures. Live provider/LLM integration tests were excluded. As before, the local Anaconda readline problem was bypassed only in the pytest launcher, without changing project files/environment.

Please re-check the empty-argument finding on the updated head once hosted CI completes.

[Copilot]

Copilot review overview

🟡 Changes recommended

Address YAML item-boundary traversal, quoted keys, and --package option parsing.

Review effort: Lite
Findings: 1

Open (1)

• Handle npx package options before identifying the server package · New

Resolved since last review (1)

• Preserve empty quoted arguments to prevent analyzer crash

[rng1995]

[SkillSpector Review]

Hi @agentsope, thank you for the sibling-key rework, the YAML comment stripping and the thorough layout and empty-argument test matrices!

Value and readiness: This now fixes #639, plus a newer false positive on current main. There, cross-line matching combined with #683's operand-only pin check reads the next YAML key (args) as the package. As a result, every YAML command: npx config is reported, pinned or not.

With this head:

• pinned YAML configs are silent;
• every layout from the last review (intervening keys, list items, args before command, path-qualified runners) reports unpinned servers.

However, two ordinary YAML layouts that main reports are still missed, so a config can hide an unpinned server just by its formatting:

• block sequences written at the key's own indentation, which is PyYAML's default yaml.dump output and kubectl's;
• an args key more than 8 physical lines from command, for example after an env block with seven variables.

One more change is needed before merge.

Previous findings (review at 3f8e7d2). Since then:

• 7f3ddfd merges main at 4a55062, which includes #683, together with your matcher rework.
• ed2d3a6 fixes the empty-argument handling.

The merge was conflict-free, and only the PR's two files differ from an automatic merge of the same parents.

• [Blocker] The YAML matcher only accepted args as the next sibling and rejected - command:: Resolved for every layout I listed.

  • These now report unpinned servers and skip pinned ones: intervening env/type/cwd/description keys, - command: npx list items, args before command, and /usr/local/bin/npx or ./node_modules/.bin/pnpx runners.
  • test_rp1_yaml_sibling_layouts_preserve_pin_behavior covers 10 layouts, each pinned and unpinned.
  • args from another mapping or list item is not bound (test_rp1_yaml_does_not_bind_args_from_another_mapping).

  A related layout gap remains; see Material finding 1.

• [Non-blocking] A # comment on the args line could supply a fake pin: Resolved. _strip_yaml_comment (mcp_rug_pull.py:178) drops unquoted comments and keeps a quoted #, and test_rp1_yaml_comments_cannot_supply_a_fake_package_pin covers it.

• [Non-blocking] Multi-line message and matched_text for YAML findings: Still open (mcp_rug_pull.py:410-419). Optional.

• [Non-blocking, first review] JSON MCP configs are not detected: Still open. Not a regression; main misses them too.

• [Non-blocking, first review] _RP1_UVX_CMD, _RP1_PIP_INSTALL and _RP1_DOCKER_CMD still match across lines with \s+ (:144-154): Still open. Fine as a follow-up.

• [Copilot] An empty quoted argument crashed the analyzer: Resolved in ed2d3a6 (:284-291), covered by a 28-case matrix.

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/mcp_rug_pull.py:277, :252, :227, :270: two ordinary YAML layouts that main reports produce no RP1 at this head.

   (a) Block sequences at the key's indentation. YAML allows a block sequence value to start at the same column as its key. Both PyYAML's default yaml.dump and kubectl write configs this way.

   • At L277 the args collector stops at the first line with indent <= args_indent, so it collects nothing.
   • At L252 the sibling search stops at a - item line in the command's column, because that line is not a key.

   Each of these reports nothing here and RP1 on main:

   mcpServers:
     fs:
       command: npx
       args:
       - -y
       - "@scope/server"

   servers:
   - command: npx
     args:
     - -y
     - "@scope/server"

   mcpServers:
     fs:
       command: npx
       autoApprove:
       - read_file
       args: ["-y", "@scope/server"]

   (b) The 8-line window counts a sibling's nested lines. I asked you to keep a bound last round, and that is still right; the problem is what it counts. L227 and L270 count physical lines, so a sibling's value uses up the bound. Main reports all of the following, and this head reports none:

   • an env block with seven variables between command: npx and args: ["-y", "@scope/server"], such as the PGHOST, PGPORT, ... settings for a Postgres server;
   • a 7-line description: | in the same position;
   • an args list with eight flag lines before the package.

   Expected fix:

   • When collecting args, also accept - lines at the args key's column.
   • In the sibling search, treat a - line at the command's column as part of the previous sibling's value and skip it.
   • Count only same-column sibling keys (and sequence items) toward _RP1_CONFIG_MAX_LINES. Skip deeper-indented lines without counting them, under a larger hard cap plus the existing runtime checks.

   Tests to add, each unpinned and pinned:

   • the indentless mapping form;
   • the indentless list-item form;
   • an indentless sibling list between command and args;
   • an env block longer than the window.

2. [Non-blocking] mcp_rug_pull.py:283-296, re Copilot's --package comment. Both the YAML path and the shell path treat the first non-flag token as the package, so option values are misread:

   • ["-p", "@scope/helper@1.2.3", "-p", "@scope/server", "server-bin"] reads as pinned, although both packages are installed.
   • ["--registry", "http://10.0.0.1:8080", "@scope/server"] also reads as pinned, because :8080 matches the pin regex.

   The shell path has applied the same first-operand rule since #683, and npx -p … -p … is missed on main as well. Main reports the YAML forms only through the cross-line match that also flags pinned configs. A follow-up that parses -p/--package/--registry values for both paths fits better than this PR.

   Copilot's exact example is not an unpinned fetch: with --package, npm runs the first positional as a command and does not install it.

3. [Non-blocking] PR description: it still describes only the regex change. Please add the YAML matcher, which is now most of the diff.

PIC tradeoffs: None identified.

Verification and gaps:

• History: 7f3ddfd is a merge commit (parents 3f8e7d2 and main 4a55062) that also carries author changes.
  • An automatic merge of the same parents is conflict-free, and 7f3ddfd differs from it only in mcp_rug_pull.py and tests/test_mcp_rug_pull.py.
  • The PR diff against its merge-base touches only those two files.
  • Main has not changed mcp_rug_pull.py since 4a55062, and git merge-tree against current main (e9f7427) is clean.
• #683: it is included through the merge, and the shell loop still calls _operand_has_version_pin unchanged. The YAML matcher pin-checks only the first positional token, which is consistent with #683.
• Harness: I transcribed the main and head regex literals (checked verbatim against the source) and wrote my own model of _iter_config_npx_commands from the diff.
  • The model reproduces all 71 expectations in the PR's new tests.
  • I then ran 25 layouts, each pinned and unpinned, against main and this head; finding 1 comes from those results.
  • PyYAML's yaml.dump of {"command": "npx", "args": [...]} emits the indentless form from finding 1(a).
• Shell forms: unchanged. npx -y … and pnpx … are reported, foonpx does not match, CRLF is handled, and command: npx -y pkg is reported once.
• Not run: contributor tests (policy).
• CI: all 6 checks green on ed2d3a6. GitHub's mergeability is still UNKNOWN; the local merge-tree is clean.
• Overlaps: no other open PR changes RP1. #234 only adds an unrelated test in a different file.

---

Decision: Changes Requested (reviewed head ed2d3a6777490dab526cdfba975cd776f6b1e69a)

[rng1995]

[Blocker] A block sequence at the key's own indentation is valid YAML and is PyYAML's default yaml.dump output and kubectl's (args: followed by - -y / - "@scope/server" in the same column). This check stops at the first - line, so nothing is collected and command: npx plus that unpinned args list reports no RP1 (main reports it). The same happens for the list-item form (- command: npx / args: / - "@scope/server"). The sibling search has the same problem at L252: a - read_file line under a sibling such as autoApprove: ends the search, so a later args is never found. Please accept - lines at the args key's column here, skip them as part of the previous sibling's value at L252, and add unpinned/pinned tests for these forms.

[rng1995]

[Blocker] The bound counts physical lines, including a sibling's nested value lines. A realistic env block with seven variables (e.g. PGHOST, PGPORT, ... for a Postgres server), or a 7-line description: |, between command: npx and args: ["-y", "@scope/server"] hides the unpinned server; main reports it. The args collector at L270 has the same limit, so eight flag lines before the package also hide it. Please count only same-column sibling keys (and sequence items) toward _RP1_CONFIG_MAX_LINES, skipping deeper-indented lines without counting them, under a larger hard cap plus the existing runtime checks. Please also add a test with an env block longer than the window.

[agentsope]

Addressed the remaining YAML layout blocker and updated the PR description. New head: ddb05fc87d90e9b880b18f1542956d262cecf8fe (all three follow-up commits carry DCO sign-offs).

• Accept indentless block args in mapping and list-item forms, and traverse indentless sibling sequence values without ending the sibling search.
• Count same-column sibling keys rather than physical lines in nested env/description values. Keep an eight-key structural window, a 256-physical-line hard cap per direction, a separate 128-line args collection cap, and shared runtime checks.
• Retain server mapping/list-item boundaries and command-line finding locations. Regressions cover pinned/unpinned configs, args before/after command, LF/CRLF, both sides of the structural/physical/args limits, and deadline checks during long-value traversal.

Validation: the new layout selection reproduced 24 failures with 24 passing controls before the implementation change. Both MCP rug-pull test files now pass 163 tests. An independent check validates 1,152 parsed YAML layouts. The full non-integration/non-provider suite passed 8,763 tests (14 skipped, 134 deselected, 4 xfailed). Repository-wide Ruff lint/format, targeted mypy and whitespace checks pass. No sample commands or live provider calls were executed.

The description now covers the YAML matcher and its bounds. Option-value parsing, JSON support, other runners' cross-line behavior and single-line presentation remain separate follow-ups as suggested. Please re-check the layout blocker once hosted CI completes.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Three moderate findings remain in matching and package-argument validation.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Handle npx package options before identifying the server package