fix(static): markdown table and quote syntax is not an execution signal - #321
fix(static): markdown table and quote syntax is not an execution signal#321Mark2Mac wants to merge 2 commits into
Conversation
`_is_documentation_context` refuses to treat a line as prose when
`_EXECUTION_SIGNAL` matches it, and that pattern includes `[|>]`. In markdown
those two characters are structure, not shell metacharacters: `|` delimits table
cells and `>` starts a block quote. A governed rule that lands on a table row or
a quoted paragraph is therefore never classified as prose, whatever it says.
The delimiters are now removed before the execution test, and only the
delimiters. Inside a table cell a literal pipe has to be written `\|`
(CommonMark), so a documented `cmd \| tee log` keeps its pipe and still counts
as an execution signal — which is what makes this safe rather than a widening.
Scope, stated plainly: this is a correctness fix, not a precision win. On a
corpus of 65 real skill/plugin units (4415 findings, all triaged by hand) only 8
findings of the governed rules were blocked by markdown structure alone, across
4 files. After the change 1 remains, and it has a genuine execution signal on
the line. The reason to fix it is that the classification is simply wrong, not
that it is frequent.
Nothing is added to `_SEMANTIC_STRING_DOC_PRONE_RULES`: the set stays
{RA1, TM1, AR2}, and the reasoning that excludes PE3 is untouched.
Tests: table row and block quote are classified as prose; a literal escaped pipe
in a cell and a real redirection in a quote still are not; plus a unit test that
`_strip_markdown_structure` touches delimiters and nothing else. Full suite:
1565 passed, 14 skipped, 6 xfailed.
Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com>
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Approved. Markdown table separators and leading blockquote markers are removed only for the documentation-context execution check, while escaped literal pipes and later redirections remain signals. Rule scoping is unchanged and the focused filtering suite passes (52 tests). One non-blocking Python escape warning is noted inline.
| """Drop markdown delimiters that would otherwise read as shell metacharacters. | ||
|
|
||
| In a table row an unescaped ``|`` separates cells; a literal pipe inside a cell has to be | ||
| written ``\|`` (CommonMark). That distinction is what makes this safe: the delimiters are |
There was a problem hiding this comment.
Non-blocking: this new docstring contains \| in a normal string and emits SyntaxWarning: invalid escape sequence '\|' on Python 3.12. Please make the docstring raw or write \\| so the supported-Python test run stays warning-free.
There was a problem hiding this comment.
Fixed in c485b6b: the docstring is now raw, which is the whole change to the source.
I guarded it package-wide rather than only here — a new test compiles every module under src/skillspector/ and fails on any SyntaxWarning, so the next stray escape is caught in CI instead of in a reviewer terminal. The package is clean today (76 files, zero warnings), and the test fails on the previous commit.
Full suite: 1569 passed, 12 skipped, 6 xfailed.
The docstring quotes CommonMark's escaped bar, and \| is not a valid escape sequence: Python 3.12 emits SyntaxWarning on import, which makes a supported-Python test run noisy. The prefix is the whole fix. Guarded package-wide rather than per-file: the new test compiles every shipped module and fails on any SyntaxWarning, so the next one is caught where it is written instead of in a reviewer's terminal. Tests: tests/unit 727 passed, 12 skipped. Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com>
The problem
_is_documentation_contextdeclines to treat a line as prose when_EXECUTION_SIGNALmatches it. That pattern includes[|>]— but in markdown those characters are structure:|delimits table cells>starts a block quoteSo a governed finding (
RA1,TM1,AR2) landing on a table row or a quoted paragraph is never classified as prose, no matter what the sentence says. Two real examples from the corpus below:Both are documentation. Neither runs anything.
The fix, and why it cannot hide a real pipe
The delimiters are stripped before the execution test — and only the delimiters. In a table cell a literal pipe has to be escaped as
\|(CommonMark), so:keeps its pipe after stripping, still matches
_EXECUTION_SIGNAL, and is still reported. Same for a redirection inside a quote:> run rm -rf /opt/example > logstays flagged. That escaping rule is what makes this a correction rather than a widening._SEMANTIC_STRING_DOC_PRONE_RULESis untouched — the set stays{RA1, TM1, AR2}, and the documented reasoning that keepsPE3out of it is not affected by this PR.Scope, stated plainly
This is a correctness fix, not a precision win, and I would rather say so than oversell it.
Measured on a corpus of 65 real skill/plugin units, 4415 findings, every one triaged by hand:
The reason to fix it is that the classification is wrong, not that it is frequent. The volume was small precisely because the governed set is deliberately narrow.
Tests
_strip_markdown_structuretouches delimiters and nothing else (including: a line with a dangling|is not a table row, andecho a | boutside a table is untouched)Full suite:
1565 passed, 14 skipped, 6 xfailed.