Repository navigation
fix(analyzer): treat a command wrapper with no command as complete - #779
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@elliottwaves-20 thank you for tracing the timeout/sudo/nice/xargs vs env/nohup/command asymmetry from #694 back to the one (True, None) branch, and for fixing it with a small, clearly documented helper.
Value and readiness
This fixes a real completeness false positive. On current main (7ee4371), a Markdown table row such as | timeout | float | 150 | ... | or | sudo | ... |, or JS if (timeout) {, still makes has_bounded_parse_exhaustion return True. At scan level, run_static_patterns_with_ledger then records the SKILL.md as partial/static_parse_limit. That false partial is what turns benign references into AE1.
The change only reclassifies a wrapper that is followed directly by ;, |, ) or & (but not &>): it goes from "unresolved command string" to "no command". That matches shell semantics. None of these wrappers can run a command at that point, and anything after the operator starts its own clause or candidate, which is still checked. Redirections, ( and the end of the view stay partial. The change can only move results toward "complete", and every path to that I traced is a genuinely empty wrapper. Ready for final maintainer review.
Material findings
None blocking.
- Optional: the
python-callcase (signal.alarm(timeout)) intests/nodes/analyzers/test_command_wrapper_without_command.pyalready passes on current main. #778 (025c65e) now skips clauses that start in Python code. The case is harmless, but it no longer guards this change. The Markdown, JS and shell cases still do. The description's "17 fail on main" was measured on the older base.
PIC tradeoffs
sudo -s/sudo -i, or any of these wrappers, directly before;,|or&now counts as "no command string", soecho "$CMD" | sudo -s ;becomes complete. That matches main's current handling ofecho "$CMD" | sh ;and| sudo bash ;, which I confirmed are both complete on main. The new docstring says stdin-fed shells are not modelled. This is consistent, but it remains a known gap for stdin-fed shells.
Verification and gaps
- Read the diff and, after merging it onto main,
_command_string_from_clause,_next_shell_invocation_word(returnsNoneonly at end of input or on);|&()<>),_shell_clause_starts,_shell_command_stringsandhas_bounded_parse_exhaustion.git merge-treeagainst main is clean, and the merged file differs from main only in the two branches and the helper. - Traced the controls.
sudo >log $CMD -rf /,sudo <in ...,sudo &>log ...andtimeout ($CMD) -rf /still return(True, None).x | sudo sh -c "$CMD",sudo && eval "$CMD"andcase $a in sudo) $CMD -rf / ;;stay partial through their own clause or candidate. - Ran only trusted main code, never PR code. It confirmed that the table, JS and shell cases are partial on main, that the Python case is already complete, that the TM1 finding for a
`sudo rm -rf /`table cell exists, and that the ledger enums match the tests' assertions. - Gap: CI is green, but it last ran on 2026-10-06. Since then main has changed this file in five commits (#577, #578, #579, #778, #784). The merge is textually clean; please let the refreshed CI pass before merging. I did not reproduce the corpus or fuzzing claims.
A clause such as `| timeout |` in a Markdown parameter table, or `signal.alarm(timeout)` in host code, makes `_command_string_from_clause` see the wrapper `timeout` (also `sudo`, `nice`, `xargs`) followed directly by a control operator. Those branches returned "unresolved command string", so the file was recorded as `static_parse_limit`. The `env`, `command` and `nohup` branches already treat the same situation as "no command". When the wrapper's next token is `;`, `|`, `)` or `&` (but not `&>`), no other command can run, so report no command string. A redirection, a `(`, `&>` and the end of the view stay unresolved. Refs NVIDIA#694 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: elliottwaves-20 <pail1217@web.de>
3cd031b to
623284f
Compare
|
@rng1995 Rebased onto main (b07f181) so CI runs on the current base; the change itself is unchanged. On this base 16 of the 29 tests fail on 🤖 Generated with Claude Code |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@elliottwaves-20 thank you for rebasing onto current main so the change could get a fresh CI run.
Value and readiness
This is a re-review of the rebased head 623284fa. It is still approved and ready to merge. The rebase settles the one gap I raised in my earlier review: CI had last run before five main commits touched static_patterns_tool_misuse.py.
Material findings
Prior findings
- Resolved (gap): stale CI against
main's changes to_command_string_from_clause. The branch now sits directly onmain(b07f181d), and all 6 checks passed on this head (test-unitfinished 12:15 UTC). - Still open (optional, unchanged): the
python-calltest case already passes onmainsince #778. It is harmless but no longer guards this change.
New: none.
PIC tradeoffs
Unchanged from my earlier review. A wrapper directly before ;, | or & (for example | sudo -s ;) is now treated as "no command string". That matches main's existing handling of stdin-fed shells, which remain unmodelled.
Verification and gaps
- Diff: the added and removed lines of the PR's own diff are byte-identical between the approved
3cd031b0and623284fa. The rebase changed only the base. - Context on
main: no files undersrc/skillspector/nodes/analyzers/changed between themaincommit my earlier review traced against (789f5175) and the current base (b07f181d). The behaviour I traced earlier therefore still holds. - CI and state: 6/6 checks are green on this head, mergeable is CLEAN, there are no unresolved threads, and the PR has no closing references.
Summary
A Markdown parameter table such as
makes
has_bounded_parse_exhaustion(static_patterns_tool_misuse) reportstatic_parse_limit, so the file is recorded as partially inspected and everySKILL.mdreference to it becomes anAE1. The same happens in host code such assignal.alarm(timeout)orif (timeout) {. Refs #694 (the Markdown table-cell case reported there).Root cause
_shell_clause_startsopens a new clause after every|and(, sotimeoutabove is read as the wrapper commandtimeout. Its next token is the control operator|, so_next_shell_invocation_wordreturnsNone. In_command_string_from_clausethesudo/nice/xargsandtimeoutbranches answerNonewith(True, None), which means "unresolved command string" and leads to exhaustion. Theenv/command/nohuppath answers the same situation with(False, None)(no command). That is whytimeout,sudo,niceandxargsreproduce andenv,nohupandcommanddo not, as noted in #694.timeout 5 |was already complete, because there the empty command is seen by the generic path.Fix
The wrappers that report no command string are
sudo,nice,xargsandtimeout. They do so when their next token is;,|,)or&, as long as the&is not followed by>. None of these wrappers can run another command at that point:sudoandtimeoutfail with usage,niceprints the niceness, andxargsdefaults toecho. A new helper_ends_wrapped_clausedecides this.Unchanged (stay partial):
sudo >log $CMD -rf /,sudo <in …,sudo &>log …).(, and the end of the view, where a fragment may continue.--handling.sh -c/evalcommand strings.analyze(). For example,`sudo rm -rf /`in a table cell still produces TM1.Tests
tests/nodes/analyzers/test_command_wrapper_without_command.pyhas 29 tests. Rebased ontomainb07f181, 16 of them fail onmain, and all 12 fail-closed controls pass on both. The Pythonsignal.alarm(timeout)case already passes onmainsince #778 (it failed on the original base); it stays as a pin.timeout,sudo,niceandxargs, with and without a long tail.sudo | cat,sudo |& cat,timeout; echo,timeout && echo,xargs || true,nice & wait,(nice), anice ;;case item andsudo -u | cat.signal.alarm(timeout)and JavaScriptif (timeout).>,<,&>),timeout ($CMD) -rf /,x | sudo sh -c "$CMD",sudo |& sh -c "$CMD",sudo && eval "$CMD",sudo -s; eval "$CMD",case $a in sudo) $CMD -rf / ;; esac, and a wrapper at the end of the view (complete and fragment context).`sudo rm -rf /`keeps TM1, and`sudo $CMD -rf /`stays partial.run_static_patterns_with_ledgergives COMPLETED for the table and PARTIAL/STATIC_PARSE_LIMITfor the redirected wrapper.mainand are unrelated: release/CLI path tests and three parametrized collection errors.ruff checkandruff format --checkare clean.urllib3/connectionpool.py,grpc/_channel.pyand two Tavily reference pages. No file goes from complete to partial, and the TM1 count is unchanged (60).main: I generated about 3000 random shell fragments built from wrappers, operators, quotes, substitutions andrm/-rf /pieces. Every input that went from partial to complete was checked. The ones thatbash -naccepts are heredoc text, comments or wrappers without a command, such astimeout |echoand$(sudo ). No input went from complete to partial.Residual
sudo -s/sudo -ibefore a control operator now counts as "no command string", the same asshat the end of a pipeline already does. Stdin-fed shells are outside what this check models, and the docstring says so.#778 also handles the
timeoutwrapper names inside valid Python through Python token ownership. This PR covers the same root cause for every file type, including Markdown, JavaScript and Python fragments, and does not conflict with #778.🤖 Generated with Claude Code