Skip to content

fix(analyzer): avoid parse limits for bare shell variables - #465

Closed
iammilaad wants to merge 1 commit into
NVIDIA:mainfrom
iammilaad:fix/464-bare-variable-static-parse-limit
Closed

iammilaad wants to merge 1 commit into
NVIDIA:mainfrom
iammilaad:fix/464-bare-variable-static-parse-limit

Conversation

@iammilaad

Copy link
Copy Markdown

Fixes #464

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Reviewed current head d43c946ace50e9e22e1e78ee2bbada002e892a05.

Changes requested: src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:732 treats a shell command word composed only of a plain parameter as non-dynamic. A plain parameter can still synthesize an executable command name: for example, CMD=printf; $($CMD echo) hello makes the inner expansion produce echo, which the outer substitution executes. In the dangerous analogue, this change causes _is_printf_substitution to stop recognizing the dynamic inner command and silently reports neither a finding nor a parse limit. Preserve fail-closed/dynamic handling when a parameter-only command word has arguments in an executable substitution; narrow the exemption to cases proven non-executable, and add a regression for command-name synthesis.

Hosted required checks pass, but the PR currently has merge conflicts. It is not merge-ready.

@rng1995

rng1995 commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator

@iammilaad - Please resolve review comments and merge conflicts to merge the PR.

@rng1995

rng1995 commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

[SkillSpector Review]

Hi @iammilaad, thank you for your contribution and for reporting the false parse-limit error on bare shell variables such as $ARGUMENTS.

The issue this PR targets, #464, was closed as completed by #507 ("fix: avoid false parser limits and reference accounting failures", merged 2026-09-09), and this branch now conflicts with main. The exemption here would also let a runtime-synthesized $CMD command skip the parse-limit signal, which the earlier review flagged. I'm closing this PR as superseded.

If we've missed something — for example, a case that still reproduces on current main — please comment here or open an issue with a minimal repro and we'll gladly reopen. Thanks again for helping make SkillSpector better!

@rng1995 rng1995 closed this Sep 28, 2026
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.

False positive error with backtick variables like $ARGUMENTS

2 participants