Skip to content

fix(markdown): preserve bounded inline and list-fence ownership - #950

Open
kshitijrajes wants to merge 1 commit into
NVIDIA:mainfrom
kshitijrajes:fix/markdown-list-fence-ownership
Open

kshitijrajes wants to merge 1 commit into
NVIDIA:mainfrom
kshitijrajes:fix/markdown-list-fence-ownership

Conversation

@kshitijrajes

@kshitijrajes kshitijrajes commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Two valid Markdown ownership cases could exhaust static parsing:

  • complete inline-code spans before prose possessives were treated as unresolved shell quoting
  • fenced code nested at valid list-container-relative indentation was not recognized

Fix

  • reuse bounded inline-delimiter pairing for same-line prose classification
  • accept fences only at CommonMark's 0-3 columns relative to a proven list container (top-level and nested, keeping the typed-fence label and literal-apostrophe state)
  • keep inline and fenced payloads visible to security rules
  • keep incomplete spans and malformed deep indentation fail-closed
  • runtime-command guard: an inert list marker (-, +, 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

  • The four analyzer test modules pass (552 tests), including regressions for the wrapped $CMD case, in-span and outside-span control operators, multiple spans, the * marker, the 3/4-column fence boundary, and nested typed fences.
  • Differential check against current main over 3,220 generated list-item variants: no case moves from COMPLETED to PARTIAL in the wrong direction, and every variant with a control operator stays PARTIAL / STATIC_PARSE_LIMIT.
  • CLI-based tests need click < 8.2 output semantics locally; the remaining local failures are the same stderr-in-output failures present on main.
  • Ruff check and format pass.
  • Rebased onto current 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.

@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]

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.

@kshitijrajes
kshitijrajes force-pushed the fix/markdown-list-fence-ownership branch from 092b33f to 5bcfc50 Compare October 30, 2026 18:47

@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.

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.

@kshitijrajes
kshitijrajes force-pushed the fix/markdown-list-fence-ownership branch from 5bcfc50 to 7db6766 Compare October 31, 2026 10:13

@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.

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.

@kshitijrajes
kshitijrajes force-pushed the fix/markdown-list-fence-ownership branch from 7db6766 to 9a07020 Compare October 31, 2026 14:01

@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]

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 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.

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>
@kshitijrajes
kshitijrajes force-pushed the fix/markdown-list-fence-ownership branch from 9a07020 to c462a4e Compare November 1, 2026 06:13
@kshitijrajes

Copy link
Copy Markdown
Contributor Author

Addressed in the force-push, which also rebases onto current main (including #902 and #947):

  • Clause ownership is now checked before each inline span and between spans, not only inside the span body. - ; \$CMD`, - `echo ready`; `$CMD`, and the pipeline variants all stay PARTIAL / STATIC_PARSE_LIMIT`; each is a regression.
  • A benign earlier span without an operator (- \echo ready` `$CMD``) still owns the command position through the list marker and completes.
  • A leading * is a shell glob, so it no longer owns the command position; it keeps the guard.
  • The rebase exposed that nested list fences did not carry main's new typed-fence label. They now do, with a regression for nested tcl/verilog fences.
  • Reconciled the apostrophe check with fix(static): stop backticks/apostrophes in comments and prose from tripping static_parse_limit #902: the prose view still blanks complete spans, then applies _blank_line_local_paired_backticks, so behavior outside inline spans matches main.

Since this changes the head after your approval, a re-check of the ownership change is welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants