fix(report): escape paths and reasons in Markdown tables

[fallintoplace]

Add regression coverage for Markdown component paths, suppressed-finding locations, and suppression reasons containing pipes, line breaks, and backticks. The tests verify safe rendered code spans/table cells and unchanged values in JSON.

The broad Markdown containment implementation from #735 is now merged into main; this head's report.py is identical to current main. Paths use _markdown_code, and prose/reasons use _markdown_cell.

Validation: 414 focused report/completeness tests passed, plus an independent 150-case rendered-Markdown containment review. The rebased commit now has a matching DCO sign-off, and DCO passes. All 301 focused tests pass on the new head, and 313 report tests pass when integrated with current main.

[rng1995]

[SkillSpector Review]

Hi @fallintoplace, thank you for tracking down how pipes and line breaks in file names split the component and suppressed-finding tables, and for the thorough parametrized tests!

Value and readiness: The change does what the description says. A component path or suppressed location containing |, LF, CR or CRLF now stays in one table cell, multiline baseline reasons stay on one row, and the JSON output keeps the original values. It is not ready to land on its own, though: the same two cells are still wrapped in a fixed single-backtick code span, so a backtick in a file name breaks out of the span and lets a scanned skill put a link or raw HTML into the table. Separately, #735 (opened the next day) fixes both of these tables, plus every other dynamic field in the Markdown report, with backtick-safe code spans, and the two PRs conflict. My recommendation is to land #735 and close this PR as superseded. If maintainers would rather land this one first, the blocker below needs fixing.

Material findings

1. [Blocker] src/skillspector/nodes/report.py:1491 (rendered at :1496), and the suppressed location at :1560-1561: the value is escaped for | and line breaks but still rendered as f"| `{path}` |". A single file name with no slash, x` [Verify this scan](https://evil.example) `y.md, renders in the Components table as code x, then a clickable link to https://evil.example, then code y.md. A file named a`<img src=x>`b.md yields an inline <img>. The suppressed-row location has the same problem. Expected fix: size the delimiter to one more than the longest backtick run in the value (as _render_markdown_completeness already does for exclusion patterns since #635), pad with a space when the value starts or ends with a backtick or space, and add a backtick case to the new tests. The test should assert a single code_inline holding the original text and no link_open/html_inline, for example with markdown_it.
2. [Non-blocking] Overlap with #735. Both PRs rewrite _markdown_cell and the suppressed-row lines, so git merge-tree refs/pr/723 refs/pr/735 reports two conflicts in report.py. Git auto-merges the component-path hunk, but the result keeps this PR's path = _markdown_cell(...) and #735's _markdown_code(path, table_cell=True) together, which escapes every path twice (it would show as SKILL\.md inside the code span). Your new tests would catch that. By my trace, both new tests also pass unchanged against #735's output, so they could be carried over if #735 lands first.

PIC tradeoffs: Choosing between #723 and #735. #735 covers everything this PR covers. It also closes higher-impact paths that this PR leaves open on main. For example, a SKILL.md name of demo\n<!-- makes everything after the **Skill:** line an HTML comment, which hides the risk table and all findings in the rendered report. A file name containing a newline does the same from the Issues **Location:** line. Landing this PR first is possible, but it adds a conflict resolution to #735 for little extra coverage.

Verification and gaps: I read the diff against merge base 4a550627 and the current main versions of _format_markdown and _render_markdown_completeness. This PR does not touch the #635 exclusion-pattern code spans, the #663 analyzer-name status rows, or the #621 SARIF code, and git merge-tree against current main is clean. I built the rendered rows for the attack inputs by hand and parsed them with markdown-it-py (CommonMark + tables). Pipe, CR and LF inputs keep the row at 4 cells. The backtick inputs produce link_open and html_inline tokens. All 6 CI checks passed on this head. That run was against an older main, but no later main commit touches these functions. Tests not executed locally per policy.

---

Decision: Changes Requested (reviewed head 0945fef71987bd627a036694525e3b262427f199)

[rng1995]

[SkillSpector Review]

Hi @fallintoplace, thank you for the fast and careful fix for backticks in the table paths, and for the seven well-chosen test cases!

Value and readiness: The backtick blocker from my last review is fixed. Both table cells now use a code span that a backtick in the file name cannot close, and CI is green. The change is correct within its scope. The open question is landing order, not code quality:

• #735 contains the same helper.
• #735 also closes the higher-impact paths that this PR leaves open on main.
• Both PRs change the same three hunks in report.py.

I recommend landing #735 first, then rebasing this PR as a tests-only change or closing it. I am requesting changes only to hold the merge until that happens. If the PIC prefers to land this PR first instead, it needs no code change.

Previous findings:

• [Blocker] Backtick paths inject links: Resolved. 854e182a adds _markdown_code at src/skillspector/nodes/report.py:1375. It is used for the component path (:1507) and the suppressed location (:1572). The helper:

  • sets the delimiter one longer than the longest backtick run in the value;
  • pads with a space when the value starts or ends with a backtick or a space;
  • escapes | inside table cells.

  test_report_markdown_table_paths_keep_backticks_inside_code_spans (tests/nodes/test_report.py:1386) covers the link, <img>, leading and trailing backtick or space, and mixed-run cases. It asserts the exact code content and that no link_open, html_inline or image token appears, so it would fail on 0945fef7. I parsed my own model of the helper with markdown-it-py: both attack file names stay as one code span in both cells.

• [Non-blocking] Overlap with #735: Partly resolved. The double-escaping hazard is gone. This commit drops path = _markdown_cell(...), so the path, reason and location lines are now identical in both PRs. The textual conflict remains: three report.py hunks, in either landing order.

Material findings

1. [Blocker: landing order, not a code defect] Please hold this PR until #735 lands. Then rebase onto main and keep main's side in the three report.py hunks:

   • the _markdown_cell and _markdown_code definitions;
   • the component row;
   • the suppressed row.

   After that, this PR's report.py diff is empty. Please keep your three tests. By my trace they pass unchanged against #735's output, and they add end-to-end raw-row and JSON round-trip coverage that #735's structural tests do not have. Closing the PR as superseded is also fine.

   Why #735 should go first: with this PR alone, these inputs still break the report, and #735 fixes all of them:

   • A SKILL.md name of demo\n<!-- turns everything after the **Skill:** line into an HTML comment. The rendered report then shows no risk table and no findings.
   • The same backtick file name still injects a link through the Issues **Location:** line (:1543) whenever that file has an unsuppressed finding.
   • The suppressed rule_id cell (:1573) is written raw.
   • The suppressed reason cell escapes only pipes and line breaks. With --use-shipped-baseline, that reason comes from the scanned skill.

   Landing this PR first would make that P0 fix wait for a manual rebase of the same three hunks.

2. [Non-blocking] The PR description still says "Reuse _markdown_cell in both tables". Paths now use _markdown_code, so please update the description if you keep the PR as tests-only.

PIC tradeoffs: Landing order. My recommendation is the same as in my review of #735: land #735 first, then rebase this PR as tests-only or close it.

The alternative is to land this PR first. Its CI is green, and it credits the first contributor who fixed the table cells. #735 would then need a manual rebase that keeps its own side in all three hunks. The auto-merge double-escaping hazard I flagged last time no longer exists. The cost is that #735, the P0 fix, waits for that rebase.

Verification and gaps:

• Diff: I read the new commit 854e182a (yours, DCO-signed) and the full diff against merge base 4a550627. git merge-tree with current main is clean.
• Landing orders: I simulated both orders with merge-tree. Each produces the same three conflict hunks in report.py. tests/nodes/test_report.py auto-merges, with a single markdown_it import.
• Tests against #735: I traced this PR's test inputs through my own model of #735's helpers: 7 path cases, 6 reason cases and 7 backtick cases. All expected rows and code spans match.
• CI: All 6 checks passed on this head. That run predates #496, which touched report.py and tests/nodes/test_report.py. #496 changed JSON metadata only and shares no function with this PR.
• Not run: Tests were not executed locally, per policy.

---

Decision: Changes Requested (reviewed head 854e182a3e0029933b9a94f86811b7d5928375a4)

[rng1995]

Re-reviewed exact head 71145f586c708c3e9b16de4a5c66556961d192d6 against every earlier review and the complete current diff. The backtick/Markdown containment blocker is fixed; #735 has merged and report.py is byte-identical to current main, satisfying the landing-order hold. I refreshed the description to match the helpers and remaining regression-test scope. 414 focused report/completeness tests passed; an independent reviewer passed 150 containment cases. No new code issue found. Code approved. Merge is held because the DCO Check fails on this head; please provide the required contributor sign-off and rerun the check. I did not bypass the failed check.

[rng1995]

Re-reviewed the new signed and rebased head after the DCO correction. The remaining diff is exactly one test file with 93 added lines; its content is byte-identical to the previously reviewed tests, and production report.py is identical to current main. All prior review feedback is addressed; no new actionable finding remains.

Validation: 301 report/sanitizer/completeness tests pass on this exact head. Integrating it onto actual main 17e9e85 (including merged #704 and #709) passes all 313 report/sanitizer/provenance/occurrence-column tests in a clean isolated checkout. DCO, lint, and OpenCode checks pass. Docker smoke is legitimately skipped by the workflow's test-only path filter; the full hosted unit check is still running. The live repository protection and rulesets require no CI status contexts, and the entire test-only change passes the exact-head and current-main suites above. All configured merge requirements are satisfied; a normal protected merge is appropriate. Approving this reviewed head.