Repository navigation
fix(markdown): preserve bounded inline and list-fence ownership - #950
kshitijrajes wants to merge 1 commit into
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Value and readiness
The list-container fence handling and inline-code ownership changes address real Markdown false positives. Blanking complete inline spans for prose classification creates a new gap in the runtime-command guard, so this head needs a focused correction.
Material findings
Blocker — src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:5311: the new prose view removes all bytes inside a complete inline span, including the shell syntax that previously prevented apostrophe masking. Adapt the existing negative control test_possessive_cannot_hide_runtime_command_operands by putting backticks around its $CMD:
`$CMD` reads each reference's
probe' -rf /
Follow this with the same _OBSERVATIONS tail used by that test. The shell projection keeps $CMD while dropping its backtick delimiters. The classification view now lacks $, schedules the possessive apostrophe for masking, and the final ownership pass only parses the $CMD word to its following space, so it does not protect that later apostrophe. Removing the apostrophe makes the first newline terminate the runtime command; the later -rf / no longer belongs to it, and the prose retry can clear the incomplete-coverage result. The original negative control retains this signal. Keep shell-syntax protection for inline content that can still supply a runtime command, or establish command/operand ownership before changing its later quotes. Add this wrapped version of the existing regression. The new positive inline-command test keeps the destructive operands inside the span and does not cover this split.
PIC tradeoffs
Prose punctuation should not create spurious shell quotes, but the classification and executable projections must agree about runtime syntax before a quote can be removed. Complete Markdown delimiter pairing alone does not prove that the remainder is independent prose.
Verification and gaps
Reviewed exact head 092b33f09622e4f72372780199b828b98c2b4667: full three-file diff, pinned surrounding implementation/tests, description and available review/comment history. Read the new list/fence controls and the existing runtime-command possessive control; independently corroborated the full source trace through span blanking, apostrophe collection, ownership parsing, operand tokenization and the retry return. No hosted checks are available on this exact head. Author-reported baseline and focused test results were read without rerunning them. Static review only: no contributor code, tests, imports, builds, scanner invocations, supplied snippets or regex probes were executed.
092b33f to
5bcfc50
Compare
rng1995
left a comment
There was a problem hiding this comment.
Review of head 5bcfc50 (static read only; I did not run the tests). The earlier blocker (wrapped $CMD) looks fixed by inline_runtime_guard plus its regression test. One test gap remains before I'd approve; not approving yet.
5bcfc50 to
7db6766
Compare
rng1995
left a comment
There was a problem hiding this comment.
Rechecked head 7db6766c23f031c120b53bb73d01722ba86ab6cc. The requested list-item regression and 3/4-column fence boundary cases are present. The plain list-item COMPLETED rationale holds for that exact test, and independent nested-list boundary probes preserve the payload.
One blocker remains in the same guard exemption: an inline shell control operator can put $CMD back in command position after the Markdown marker. The attached finding was independently reproduced through the public static-runner interface against base and head: base reports PARTIAL / STATIC_PARSE_LIMIT; this head reports COMPLETED with no findings.
Validation: both changed test modules pass (435 tests); changed-file Ruff check/format pass; all six hosted checks pass on this exact head. The passing tests omit the reported control-operator variant. This review does not establish a combined scanner verdict or a full local repository-suite result.
7db6766 to
9a07020
Compare
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Value and readiness
The wrapped $CMD regression is now covered, and the new guard preserves runtime-command ownership for the reported control operators inside inline code. The list-fence change retains payloads and has the requested three/four-column boundary controls. The same ownership gap remains when the operator is immediately outside the inline span, so this head still needs a focused correction.
Material findings
Blocker — src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:5189-5201: account for the clause before each inline span. The list-marker exemption considers _shell_clause_starts(body) only. Move the semicolon from the last reported reproducer to immediately before the inline span:
- ; `$CMD` reads each reference's
probe' -rf /
Append the existing _OBSERVATIONS tail from test_prose_shell_context.py. The span body is only $CMD, so it has no additional clause; list_marker_owns_initial_command=True suppresses the guard despite the preceding semicolon. The classification view blanks $CMD, and its remaining semicolon is permitted by _MARKDOWN_PROSE_SHELL_SYNTAX_RE, allowing the possessive apostrophe to be masked at lines 5428-5455. The security projection retains the semicolon and $CMD, which is now a separate command after the list marker. Before masking, the paired apostrophes join the second line and its destructive operands to that runtime command. After masking, _bounded_shell_tokens ends it at the first newline; the quote following probe is not a new command-word candidate because it follows an ordinary letter. Consequently the prose retry can clear the original incomplete-coverage result. This is the same loss as the prior blocker with only the Markdown delimiter boundary moved; the new parameterized controls keep every operator inside the span and miss it. Determine ownership using the projected clause prefix, or conservatively retain the guard if an earlier unquoted operator can put this span in command position. Add the outside-span variant and a multiple-span variant as regressions requiring PARTIAL / STATIC_PARSE_LIMIT.
PIC tradeoffs
The proven list marker can justify treating the first inline command-looking word as an argument, but that proof ends at a shell control operator regardless of its Markdown ownership. Keep the precision improvement while preserving coverage when executable clauses cross inline boundaries.
Verification and gaps
Reviewed pinned head 9a07020d9842b9881bf5419dd02d6674f93be0e6: complete three-file diff, surrounding delimiter/list/fence projection, shell-clause and word parsing, operand tokenization, retry logic, and relevant positive/negative tests. Rechecked the original wrapped-command blocker, the later list-marker control-operator blocker, all review threads and author replies, and both resolved test requests. All six hosted checks succeeded on this head. The hosted unit log reports 16,278 passed, 24 skipped, 258 deselected and 4 expected failures, including both changed test modules; the outside-span case is absent. The finding above is established by static source tracing, without a runtime reproduction or a claim about a combined scanner verdict. No contributor code, tests, imports, builds, scanner invocations or supplied snippets were executed.
rng1995
left a comment
There was a problem hiding this comment.
Re-reviewed head 9a07020 (static read + green CI). Both of my comments are addressed: the list-marker guard exemption now has positive and control-operator negative tests, and the 3/4-column fence boundary is covered. LGTM. Note the branch currently shows merge conflicts with main and needs a rebase before merging.
Complete inline-code spans before prose possessives were misread as unresolved shell quotes, producing static_parse_limit. Fenced blocks indented relative to a proven list container were also left unowned despite being valid Markdown. Reuse bounded inline-delimiter pairing for same-line prose classification and recognize fences only within CommonMark's 0-3 container-relative indentation, for top-level and nested list items alike, keeping the typed-fence label and literal-apostrophe state. Preserve runtime-command ownership when inline payloads can reach destructive operands. An inert list marker (-, +, 1., 1)) owns only the initial command position, and only when no unquoted shell control operator precedes the inline span, sits between spans, or appears inside the span; any such operator retains the runtime guard for the following clause. A leading * is a shell glob, so it never owns the command position. Keep inline and fenced payloads visible to security rules. Incomplete spans, runtime-selected clauses, and malformed four-column relative fence indentation remain fail-closed. Verification after rebasing onto current upstream main (including NVIDIA#902 and NVIDIA#947): the four analyzer test modules pass (552 tests). A 3,220-variant differential against main finds no case that moves from COMPLETED to PARTIAL and none where a control operator clears the parse-limit result. CLI-based tests only pass locally with click < 8.2 output semantics; the remaining local failures are the same stderr-in-output failures present on main. Ruff check and format pass. Signed-off-by: kshitijrajes <kshitijrajes@nvidia.com>
9a07020 to
c462a4e
Compare
|
Addressed in the force-push, which also rebases onto current
Since this changes the head after your approval, a re-check of the ownership change is welcome. |
Problem
Two valid Markdown ownership cases could exhaust static parsing:
Fix
-,+,1.,1)) owns only the initial command position. Any unquoted control operator before the span, between spans, or inside the span retains the guard. A leading*is a glob and never owns it.Verification
$CMDcase, in-span and outside-span control operators, multiple spans, the*marker, the 3/4-column fence boundary, and nested typed fences.mainover 3,220 generated list-item variants: no case moves fromCOMPLETEDtoPARTIALin the wrong direction, and every variant with a control operator staysPARTIAL / STATIC_PARSE_LIMIT.main.main, including fix(static): stop backticks/apostrophes in comments and prose from tripping static_parse_limit #902 and fix(scanner): tighten bounded parser ownership #947.