Skip to content

fix(analyzer): preserve inline JSON and nested list ownership - #829

Open
zozozeezee wants to merge 6 commits into
mainfrom
fix/markdown-inline-json-ownership
Open

zozozeezee wants to merge 6 commits into
mainfrom
fix/markdown-inline-json-ownership

Conversation

@zozozeezee

@zozozeezee zozozeezee commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Complete inline JSON values could leave their closing quotes available to the shell parser, causing ordinary documentation later in the file to exhaust its bounded word parser. Nested-list continuation text could also be treated as indented shell code, letting inline delimiters borrow operands from later blocks.

Reuse validated JSON-string spans within proven paired inline code, and account for a single list container when determining code indentation. Preserve conservative continuation handling for multiple list markers and unsupported table containers. Genuine shell constructs retain their existing limits and findings.

Validation: 246 Markdown table/prose tests pass; synthetic cases cover both false limits, destructive commands, nested fences, indented code, and oversized shell input. Full upstream suite: 13,684 passed, 14 skipped, 245 deselected, 4 expected failures. No release or deployment is included. Independent review found no remaining issue after correcting a nested-table regression caught by the full suite. Ruff and changed-diff secret scan pass. The same production patch passes the 8,021-test 2.12 backport suite.

Signed-off-by: Chandrashekar Ramachandran <cramachandra@nvidia.com>
@zozozeezee
zozozeezee force-pushed the fix/markdown-inline-json-ownership branch from 2e3e094 to 1cab14c Compare October 14, 2026 15:07

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

The inline JSON and nested-prose cases are fixed, but two Markdown boundary regressions remain: fences can make valid guides fail strict scanning, and nested tables can lose required incomplete-analysis results. Both reproduced in source and newly built, noneditable wheel installs.

Validation: 380 base and 387 HEAD tests passed in each mode; all 96 offline CLI scans completed with source/wheel agreement. Existing partial corpus results remain explicit. This is targeted verification, not a claim of global accuracy or live-provider coverage.

Comment on lines +4857 to +4859
else paragraph_list_indent
if paragraph_in_list
else 0,

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.

Could we update fence ownership when a list ends? A top-level fence immediately after a list inherits indentation 2 here, so its column-zero closer is ignored. For example, append 500 lines of Ordinary documentation. to this prefix:

- Item
```sh
printf ok
```
The interface doesn't match the spec.

The referenced guide completes with exit 0 on base, but HEAD reports static_parse_limit/AE1 and exits 1 with --fail-on-incomplete, in both source and fresh-wheel scans.

Source inspection also shows the related reverse transition: a valid nested fence such as ~~~sh stays active when a nonblank line leaves its list. Please clear stale list ownership before opening an outdented fence, and end an existing list-owned fence when its container ends. The latter variant was checked against source, not separately executed.

indentation = len(stripped[: len(stripped) - len(leading)].expandtabs(4))
# List continuation indentation belongs to its container. Only four
# additional columns introduce literal indented code in that paragraph.
code_indent = (paragraph_list_indent if paragraph_in_list else 0) + 4

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.

Could we preserve table-cell boundaries when allowing this deeper list indentation? With list markers on separate lines, code_indent becomes 8, but _markdown_table_delimiter_columns still rejects a delimiter row at column 4. This valid nested table therefore enters ordinary paragraph handling and pairs backticks across different cells:

- Modes:
  - | Left | Right |
    | --- | --- |
    | `$(resolve_tool).example | ` -rf / |

The same issue occurs across table rows. On base these referenced guides retain static_parse_limit/AE1 and fail strict scanning; HEAD reports complete analysis and exits 0 in both source and fresh-wheel scans. Please make table recognition use the same container indentation, or retain literal delimiters when that ownership is unproven. The existing nested-table test uses - - on one line and misses this shape.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i see but it depends on the identation one follows. can cause FPs too?

@mohgupta-ship-it
mohgupta-ship-it marked this pull request as ready for review October 15, 2026 04:37
@mohgupta-ship-it

Copy link
Copy Markdown
Member

Powered by Codex: reviewed on Mohit's behalf.

NEEDS_CHANGES — P1 and P2 independently reproduced. Rating: bugfix; Tier 1. Frozen head edc2cc1dc1b5ca60f79cf84c5fdede862101ddbb, base 240421219409dc0bafef9f618a10d80c2d7e83a3.

  1. P1 / high confidence — nested tables lose incomplete-analysis evidence. The changed list-relative cutoff at src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:4739–4741 permits deeper paragraph handling, while the table delimiter helper at 4626–4627 still rejects absolute indentation >=4. Separate-line nested lists containing valid tables consequently pair backticks across separate cells/rows. Both strict CLI cases change from base AE1 / incomplete / exit 1 to HEAD SAFE / complete / exit 0. Independent Markdown parsing confirms actual table cells, so the indentation concern raised in the existing thread does not invalidate these cases. This corroborates the existing table finding; it proves lost completeness signaling, not harmful execution.

  2. P2 / high confidence — stale list ownership makes valid guides fail strict scanning. Lines 4852–4859 retain list indentation when opening a top-level fence; closure at 4809–4821 also fails to end list-owned fence context on container exit. Both the list-to-top-level-fence case and the reverse nested-fence-to-outdented-prose case change from base success to HEAD AE1 / exit 1. The reverse case was executed independently as well, corroborating the existing fence finding.

Repair table recognition relative to the actual container, never pair delimiters across cells/rows, and reset fence/list ownership at container transitions. Add projection, completeness-ledger and strict-CLI tests for both table variants and both fence transitions.

Validation: 1,061 existing test executions passed across source base/head; 12 paired offline CLI cases completed, with network connections blocked. The intended inline-JSON fix works and the concrete destructive-command control still emits TM1. I also reproduced the table failure on the exact wheel adopted by evaluator !4. That backport comparison introduces the table failure relative to 2.12.0; its fence failures were already present in 2.12.0.

Current head is unchanged, review remains changes-requested, and no hosted status checks are exposed. These are targeted offline results, not live-provider or global-accuracy certification.

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.

3 participants