Skip to content

fix(analyzer): stop value-only parameter expansions from marking files partial - #686

Merged
rng1995 merged 3 commits into
mainfrom
naren/fix-parameter-operator-parse-limit
Sep 30, 2026
Merged

rng1995 merged 3 commits into
mainfrom
naren/fix-parameter-operator-parse-limit

Conversation

@rng1995

@rng1995 rng1995 commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

static_patterns_tool_misuse marks a file partial (static_parse_limit) when a backtick or $(...) span begins with an ordinary parameter expansion that uses an operator (${VAR:-default}, ${VAR#prefix}, "${ARR[@]}"), or when a prefix assignment's value contains $(printf ...). Every SKILL.md link to such a file then becomes a HIGH AE1 finding, which blocks signing gates even though the content is harmless documentation or a harmless script.

This PR gives value-only expansions and assignment values the same treatment ${NAME} already gets. Expansions that can evaluate code stay fail-closed.

Customer impact

Reported internally against SkillSpector 2.11.2 (SkillEvaluator 1.5.6) for skills/vss-build-vision-ai in NVIDIA-AI-Blueprints/video-search-and-summarization#2386. The gate reported 11 HIGH AE1 findings, which also block the catalog sync in NVIDIA/skills#587. The skill documents shell defaults in prose and comments; the content does not need to change.

The issue still reproduces on main (8831219): skillspector scan skills/vss-build-vision-ai --no-llm at the PR head (fbf0232) reports 16 of 61 files partial (73.8% coverage) and 35 AE1 findings.

Root cause

_invocation_expansion_marker classifies a braced expansion as either a runtime parameter or a dynamic word. Only the bare forms matched by _SIMPLE_BRACED_PARAMETER_RE (${NAME}, ${1}, ${@}, ...) counted as runtime parameters; everything else was labelled dynamic because it "may contain nested command substitutions". When a dynamic word is the first word of a backtick or $(...) span, _consume_printf_invocation reports a possible runtime-built printf, _parse_shell_command_word sets limited, and has_bounded_parse_exhaustion returns True.

Source (VSS skill) Why it tripped
# A `${VSS_CONTAINER_TAG:-...}` fallback ... (Python comment) Operator form was dynamic
Defaults may nest (`${A:-${B}/x:${C}}`) (docstring) Operator form was dynamic
A `${...}` or `$NAME` that survived expansion Placeholder was dynamic
DEPLOYMENT=$("${VSS[@]}" configure show) Array expansion was dynamic
ES_URL=$(printf '%s' "${DEPLOYMENT}" | jq ...) Assignment value treated as a command word

The last row only fires when the variable name starts with one of the command-word candidate letters (r R d D e E): RESULT=$(printf '%s' "$X") was partial, MY_URL=$(printf '%s' "$X") was not. The shell recognizes NAME=value before expansion and never runs the value as the command name.

Fix

In src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:

  • _is_value_parameter_expansion replaces _SIMPLE_BRACED_PARAMETER_RE. It accepts an expansion only when it and every nested ${...} match a value-only head (optional # length prefix; a name with an optional [@], [*] or numeric subscript, or a numeric/special parameter; then } or a value operator :- - := = :? ? :+ + # ## % %% / // /# /% ^ ^^ , ,,). The expansion must also contain no backtick, $(, $[, <( or >(. The documentation placeholder ${...} is accepted because every shell rejects it as a bad substitution. Accepted forms get _RUNTIME_SHELL_PARAMETER_SENTINEL, so they still need the same printf invocation evidence as $NAME.
  • _is_assignment_word recognizes a prefix assignment (NAME=/NAME+= after a clause boundary, a reserved word such as then/do/if, or export/local/declare/readonly/typeset), including a candidate at the value's opening quote. _parse_shell_command_word skips only the printf reconstruction check for such a word. Nested command bodies remain independent candidates and are still scanned.

What stays fail-closed

Each of these is a regression test and still reports static_parse_limit:

  • Expansions that evaluate code carried by a value: ${X@P}, ${!X}, ${ARR[i]}, ${X:offset}, zsh ${(e)X}, ${ cmd; }, and any expansion containing $(, backticks or $[ (${X:-$(id)}, ${X:-${Y@P}}).
  • Runtime command selection with printf or destructive evidence: $(printf ${FORMAT:-%s}) -rf /, $(${TOOL:-printf} 'r%s' m) -rf /, $("${VSS[@]}" configure show) -rf /.
  • Words that are not prefix assignments: alias rmall="$(printf ...)", echo ES_URL=$(printf ...), a/ES_URL=$(printf ...), a quoted command after an assignment, and eval "RESULT=$(printf ...)".
  • The existing test_runtime_printf_arguments_and_nested_reconstruction_stay_partial cases are unchanged.

Validation

  • New tests/nodes/analyzers/test_parameter_expansion_reconstruction.py (48 tests): hook-level clean and fail-closed cases, ledger outcomes, assignment recognition, and two CLI end-to-end scans (shell-defaults bundle is complete with no AE1; runtime-command control keeps AE1). On the pre-fix(analyzer): avoid false parse limits on quoted shell assignments #634 main, the 20 bug-case tests fail and all control tests pass. Controls include a later $RESULT -rf / use of an assigned value, which stays partial.
  • Full non-integration suite on main (with fix(analyzer): avoid false parse limits on quoted shell assignments #634) plus this PR: 8482 passed, 14 skipped, 4 xfailed.
  • ruff check and ruff format --check pass on src and tests.
  • No fixture is executed; shell semantics for the excluded forms were confirmed with harmless echo in bash, dash and zsh.

Relationship to #634 and remaining work

The VSS skill hits two independent parser defects. #634 (merged as 7822cb1) fixed quote ownership. This PR fixes the operator/printf family above. Measured with --no-llm on the VSS skill:

Build Partial files AE1
before #634 (8831219) 16 35
main with #634 (7822cb1) 8 4
main + this PR 5 0

After #634, quote-after-= candidates skip the printf and operand checks in _has_shell_command_word_exhaustion. The quoted-value branch of _is_assignment_word is kept so _parse_shell_command_word stays consistent for any caller, as the code comment now explains.

Corpus check against main (7822cb1), 6,073 files from NVIDIA/skills, the VSS develop skills and anthropics/skills: 0 files newly partial, 14 newly complete, and identical TM1–TM4 findings in every file.

The 5 remaining partial files come from prose apostrophes and non-shell quoting in YAML/logstash comments and Python string literals. They lower coverage but are not referenced from SKILL.md, so they produce no AE1.

🤖 Generated with Claude Code

A file that documents shell defaults such as `${VAR:-default}`, uses
`"${ARR[@]}"` as a command, or assigns `NAME=$(printf ...)` was marked
partial with static_parse_limit, and every SKILL.md link to it became a
HIGH AE1 finding.

The printf reconstruction check labelled every braced expansion other
than `${NAME}` as a dynamic word, so a backtick or `$(...)` span that
started with one looked like a runtime-built printf. Expansions whose
operator words contain no command substitution, arithmetic, indirection,
transformation or zsh flag now get the same runtime-parameter treatment
as `${NAME}`. Prefix assignment values no longer count as command-name
reconstruction; their nested command bodies are still scanned.

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.

[SkillSpector Review]

Self-review of 7662059 after #634 merged to main (7822cb1). The core change holds up: #686 on top of the new main merges cleanly, the VSS skill from NVIDIA-AI-Blueprints/video-search-and-summarization#2386 scans with 0 AE1 (5 partial files, none referenced from SKILL.md), and both PRs' test files pass together (76 tests).

Corpus check. 6,073 files from NVIDIA/skills, the VSS develop skills and anthropics/skills, new main vs this branch merged with main:

  • has_bounded_parse_exhaustion: 0 newly exhausted, 14 newly complete.
  • static_patterns_tool_misuse.analyze findings (TM1–TM4): identical in every file.

Findings

  1. [Process] CI predates #634. Every check passed, but against the old main. Merge main so CI covers the combined parser.
  2. [Tests] Pin the "a value never names the command" argument. A later use of the assigned value as a command must stay partial: RESULT=$(printf 'r%s' m); $RESULT -rf /, the quoted "$RESULT" -rf / form, and export TOOL=... && $TOOL -rf /. All three are partial today, but no test protects them (inline).
  3. [Comment accuracy] Quoted-value branch of _is_assignment_word (inline). #634 now skips the printf and operand checks for quote-after-= candidates, so this branch no longer changes an outcome. Across the corpus it was reached 4,787 times, always from those candidates. Keep it so _parse_shell_command_word stays consistent for any caller and skips a wasted printf evaluation, but the comment should say that.
  4. [Docs] PR description. The "Relationship to open PRs" section still describes #634 as open and uses pre-merge numbers.

I'll push the fixes for all four.

Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py
Comment thread tests/nodes/analyzers/test_parameter_expansion_reconstruction.py
Skipping the printf reconstruction check on `NAME=$(printf ...)` is safe
because a later `$NAME` command word is still checked. Pin that with
controls for `RESULT=$(printf 'r%s' m); $RESULT -rf /`, the quoted
`"$RESULT" -rf /` form and an exported value run after `&&`. All three
stay partial.

Explain why `_is_assignment_word` still recognizes a quoted value now that
the exhaustion sweep skips quote-after-`=` candidates itself: it keeps the
parser consistent for any caller and avoids a discarded printf evaluation.

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.

[SkillSpector Review]

All four findings from the self-review are addressed at fce4c43:

  1. Branch includes #634. The branch contains the main merge 77d2df4 with #634 (7822cb1). The full non-integration suite on this head passes: 8482 passed, 14 skipped, 4 xfailed. Ruff lint and format pass.
  2. Later-use controls added. $RESULT -rf /, "$RESULT" -rf / and export TOOL=... && $TOOL -rf / after a printf assignment all assert partial.
  3. Comment updated. The comment on the quoted-value branch of _is_assignment_word now says the exhaustion sweep already skips those candidates and why the branch is kept.
  4. PR description updated. It now reflects #634 being merged, with current numbers.

Validation on main + this PR


Decision: Ready to approve (reviewed head fce4c4304ba29a8f5b128e702fd60f73bf5e0da3). This account opened the PR, so GitHub does not allow it to approve. A maintainer approval is needed to merge.

@rng1995
rng1995 merged commit 2226747 into main Sep 30, 2026
6 checks passed
@rng1995
rng1995 deleted the naren/fix-parameter-operator-parse-limit branch September 30, 2026 04:58
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