Skip to content

fix(analyzer): keep Python and Perl syntax from exhausting the shell parser - #778

Merged
rng1995 merged 3 commits into
mainfrom
naren/fix-host-language-shell-ownership
Oct 6, 2026
Merged

rng1995 merged 3 commits into
mainfrom
naren/fix-host-language-shell-ownership

Conversation

@rng1995

@rng1995 rng1995 commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Valid Perl and Python source makes has_bounded_parse_exhaustion (static_patterns_tool_misuse) report static_parse_limit. The file is then recorded as partially inspected, and every SKILL.md reference to it becomes a HIGH AE1, which blocks downstream signing gates even though the content is harmless. All triggers below are new in 2.12.0 (2.11.2 scans the same files complete).

This PR recognizes Perl eval BLOCK, and gives Python string, f-string and comment tokens ownership of their bytes when the whole module is proven to be valid Python. Refs #694 (Python part; the Rust, JavaScript, PowerShell and Markdown-table cases there are not addressed).

Customer impact

Reported internally against SkillSpector 2.12.0 (via SkillEvaluator 1.6.0). Two internal skills with larger Python/Perl helper scripts fail their release gate with a critical "scan did not execute reliably" result.

Skill Files reported static_parse_limit AE1
Skill A 4 helpers: 1 Perl, 3 Python. This PR clears all 4 (one more helper is fixed by #777). 7 → 1
Skill B 2 Python files 1 → 0

Neither skill needs to change.

Root cause

Source Why it tripped
Perl eval { require $f; 1 } or die "x: $@"; _command_string_from_clause treats a clause-initial eval as shell eval. The dynamic $f operand makes the reparsed string unresolvable (None), which returns exhaustion immediately.
Python md = "\n```\n" followed by more than 4 KB of code A backtick inside double quotes opens shell command substitution, which never closes. unresolved_end - start > _SHELL_COMMAND_WORD_CHARS with unresolved_end at the end of the view.
Python docstring/comment apostrophes and backticks (helper's get_token() , # ... the caller's path) Same: the apostrophe opens a shell single quote that runs to EOF.
signal.alarm(timeout), if (timeout > 0 ... A clause starting with the wrapper name timeout is reparsed as a command string.
# Invoke with $0 = the launcher's path A runtime-selected command in a comment takes "operands" from the following code.

Fix

Perl. With file_type == "perl", a clause-initial bare eval followed by { is not a string reparse. { already starts a new clause, so the statements in the block are still scanned.

  • Still partial: eval $code, eval "...", eval qq{...}, quoted "eval {" inside a shell string, and any shell-file eval.
  • Unchanged: eval { `$TOOL -rf /` }, eval { qx(sh -c "$cmd") } and brace-expansion inside the block; eval { system("rm -rf /") } still produces TM1.

Python. A lazy _PythonSourceOwnership is used only for a complete .py view that:

  • ast.parse accepts (tokenize alone accepts $ and backticks);
  • is within MAX_PYTHON_AST_SOURCE_CHARS;
  • has no lone CR;
  • has no non-Python shebang.

Its string, f-string/t-string (outermost) and comment spans come from tokenize and are verified against the source. Valid Python code outside those tokens contains no shell quote or expansion opener, so:

  • an unresolved shell word is charged only up to the first code-level word break after it, instead of the end of the view;
  • a command-string clause (eval, sh -c, wrappers) that begins in Python code is not reparsed as shell;
  • a runtime-selected command inside a comment is rechecked against that comment alone, because no shell receives a comment. # $TOOL -rf / stays partial.

Unchanged:

  • Fragments, malformed source, shell-shebang polyglots, and any single string or comment that itself exceeds the 4096-character budget keep today's results.
  • So do parsed.limited, the printf, destructive-rm, brace-expansion and root-glob checks, runtime commands built from strings, all shell and Markdown behavior, and every rule finding from analyze().

The new section in docs/ANALYSIS_RESOURCE_BOUNDS.md records the ownership contract.

Tests

tests/nodes/analyzers/test_host_language_shell_ownership.py has 56 tests; 32 of them fail on main, and all fail-closed controls pass on both.

  • Perl: benign eval blocks (complete and fragment context) → False; string-eval forms → True; a shell-file control → True; commands inside the block still scanned and TM1 kept.
  • Python benign → False, each paired with a shell-text control that is True: fence in a string or f-string, unpaired backtick in a docstring, docstring apostrophe with double backticks, comment apostrophe, the static_parse_limit on valid Rust, Python and POSIX shell source leaves files partially inspected #694 regex character-class case, end="", implicit concatenation, timeoutwrapper names, comment$0`. Also CRLF, no trailing newline, non-ASCII, and exact span offsets.
  • Still → True: invalid Python, #!/bin/sh, lone CR, NUL, complete_context=False, a single literal/adjacent literal/comment over 4096 chars, a comment with $TOOL -rf /, and a .sh file with an unmatched backtick plus a long tail (ledger PARTIAL).
  • Runtime deadline honored by the token scan.
  • Scan level (graph.invoke): a SKILL.md referencing a Python and a Perl helper is complete with no AE1; the over-budget negative control stays partial with AE1.
  • make lint and make format-check pass. The unit suite (pytest -m "not integration and not provider" tests/) gives 10343 passed, 15 skipped, 4 xfailed, 0 failed.
  • End-to-end on the two affected skills: compared with main, the only changes are the removed static_parse_limit rows and their AE1s; no rule finding changes.

Residual

  • eval { `sh -c "$cmd"` } was partial on main only incidentally, through the eval-operand path. It is now complete, the same as my $x = `sh -c "$cmd"` already is. The underlying gap is that command strings inside backtick substitutions are not recognized as clauses; it predates this PR.
  • Python files over 256K characters, marker windows, and derived views that are not valid Python keep today's behavior.
  • A Python module that starts with a UTF-8 BOM gets no token ownership. The file cache decodes with utf-8, not utf-8-sig, so the leading U+FEFF fails ast.parse, and main already reports such a file as syntax_error in the AST analyzers. A test pins today's conservative result; stripping the BOM belongs with a separate decoding fix.

🤖 Generated with Claude Code

…parser

Valid host-language source could make has_bounded_parse_exhaustion report
static_parse_limit. The file was then recorded as partially inspected and
every SKILL.md reference to it became a HIGH AE1 finding. These triggers
are new in 2.12.0:

- Perl `eval BLOCK` (`eval { require $f; 1 } or die $@;`) reached the
  shell `eval` command-string reconstruction, and the dynamic `$f`
  operand made the reparsed string unresolvable.
- In Python, a string, docstring or comment containing a Markdown fence,
  an unpaired backtick or an apostrophe opened a shell quote or command
  substitution that ran to the end of the view. Any file with more than
  4096 characters after it was partial.
- Python code that uses a shell wrapper name (`signal.alarm(timeout)`,
  `if (timeout > 0 ...`) was reparsed as a `timeout` command string, and
  a `$0` in a comment was treated as a runtime-selected command whose
  operands ran on into later code.

Recognize Perl `eval BLOCK` in .pl files: a clause-initial `eval` followed
by `{` is not a string reparse. `{` already starts a new clause, so the
statements in the block are still scanned. String eval (`eval $code`,
`eval "..."`, `eval qq{...}`) and shell `eval` are unchanged.

For a complete .py module that ast.parse accepts (no non-Python shebang,
no lone CR, within the AST size bound), compute string, f-string and
comment token spans with tokenize and verify them against the source.
Valid Python code outside those tokens contains no shell quote or
expansion opener, so:

- an unresolved shell word is charged only up to the first code-level
  word break after it, which ends the string or comment token that
  opened the quote, instead of the end of the view;
- a command-string clause (eval, sh -c, wrappers) that begins in Python
  code is not reparsed as shell;
- a runtime-selected command inside a comment is rechecked against that
  comment alone, because no shell ever receives a comment.

Spans are computed lazily, only when one of these checks would otherwise
fail closed, and they honor the runtime deadline. Fragments, malformed
source, shell-shebang polyglots, a string or comment that itself exceeds
the command-word budget, the destructive rm, brace-expansion and printf
checks, runtime commands built from strings, and all shell and Markdown
behavior keep their existing conservative results. Rule findings are
unchanged.

Refs #694

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review: the ownership model is sound. Requiring a full ast.parse, verifying spans against the source, and bounding at the first code-level break rather than at the token end are the right conservative choices. The Perl eval BLOCK gate on the exact bare word keeps quoted "eval {" in shell strings failing closed. I checked tab-indented, coding-cookie, Python 2 (falls back) and invalid-escape sources. Two P3 items, no blockers.

Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py
The module parse that proves Python token ownership repeated every
compiler warning the runner's own parse had already reported, such as an
invalid `"\d"` escape. Ordinary skill code printed duplicate warnings,
and under `-W error::SyntaxWarning` the parse raised and silently
dropped ownership, so the file fell back to static_parse_limit.

Run that parse with warnings ignored. `warnings.catch_warnings` swaps the
process-global filter list, and analyzer nodes run on concurrent graph
worker threads, where two such blocks that exit out of order leave
their `ignore` filter installed for the whole process. Serialize the
block with a module lock.

Pin the existing result for a module that starts with a UTF-8 BOM: the
file cache decodes with utf-8, not utf-8-sig, so a leading U+FEFF fails
the parse and ownership stays unproven. That gap belongs with a separate
decoding fix.

Refs #694

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rng1995
rng1995 merged commit 025c65e into main Oct 6, 2026
6 checks passed
@rng1995
rng1995 deleted the naren/fix-host-language-shell-ownership branch October 6, 2026 23:57
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