Skip to content

fix(analyzer): retain fenced SKILL.md findings - #276

Open
rodboev wants to merge 8 commits into
NVIDIA:mainfrom
rodboev:pr/skill-md-fenced-example-filtering
Open

fix(analyzer): retain fenced SKILL.md findings#276
rodboev wants to merge 8 commits into
NVIDIA:mainfrom
rodboev:pr/skill-md-fenced-example-filtering

Conversation

@rodboev

@rodboev rodboev commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

The static runner now retains findings located inside fenced commands in canonical SKILL.md. Inline and prose examples in SKILL.md remain subject to the generic code-example filter, as do examples in ordinary Markdown and non-canonical *skill.md files.

Root cause

_scan_path() treated SKILL.md as non-executable Markdown, then dropped findings whose context contained triple backticks. The TM1 analyzer emitted the finding, but the shared per-path runner removed it before reporting. The first exception was path-only, so inline examples could bypass the same filter after the rework.

Changes

  • Kept the existing loose _is_skill_md(path) helper at the three legacy SKILL.md documentation-filter sites, and retained _is_canonical_skill_md(path) for the exact-basename runner boundary shared by run_static_patterns() and run_static_patterns_with_ledger().
  • Added _is_fenced_code_block() and made the canonical exception depend on the finding's actual source line being inside a triple-backtick block.
  • Added runner coverage for fenced SKILL.md, inline and post-fence SKILL.md examples, fenced guide.md, non-canonical *skill.md paths, and the loose-versus-canonical path-helper boundary.
  • Added a no-LLM CLI regression that writes the issue-shaped fixture to tmp_path / "SKILL.md" and asserts TM1 plus nonzero before and after filtering counts.

Scope

This implements only the fenced SKILL.md example-filtering slice of issue #268. It doesn't change meta_analyzer.py, scoring bands, report schema, analyzer registration, provider code, or other issue slices. The retained finding remains in the current LOW/SAFE aggregate band; this change surfaces and counts it without retuning scoring. Analyzer-local fenced-example policies remain unchanged.

Verification

  • pytest tests/nodes/analyzers/test_static_runner_filtering.py tests/nodes/analyzers/test_static_patterns_anti_refusal.py tests/nodes/analyzers/test_static_false_positive_controls.py tests/unit/test_cli.py -q -> 190 passed, 4 xfailed
  • The nine maintainer-named CLI and SC8 regressions passed, 9 passed
  • uv run ruff check src/ tests/ -> All checks passed
  • uv run ruff format --check src/ tests/ -> 168 files already formatted
  • git diff --check
  • Every commit in the current PR range has a Signed-off-by: trailer

Base reproduction: issues: [], findings_before_filtering: 0, and findings_after_filtering: 0.

Head reproduction: the CLI regression asserts TM1 in issues and both filtering counts greater than zero.

Upstream

Refs #268

rng1995
rng1995 previously approved these changes Jul 24, 2026

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

[Automated SkillSpector Review]

Approved. Canonical SKILL.md fenced instructions no longer create a runner-level detection blind spot, while ordinary Markdown and noncanonical suffix names retain the existing example filtering. Runner and CLI regressions cover the boundary without changing report schemas.

@rng1995

rng1995 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

@rodboev - Please address review comments and resolve merge conflicts.

Signed-off-by: Rod Boev <rod.boev@gmail.com>
@rodboev
rodboev force-pushed the pr/skill-md-fenced-example-filtering branch from f1010a7 to b95352d Compare August 12, 2026 15:28
@rodboev

rodboev commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, I've rebased onto current main and resolved the conflicts.

  1. I preserved the new _scan_path() architecture and migrated the exact-basename SKILL.md exemption into that shared per-path filter, so both run_static_patterns() and run_static_patterns_with_ledger() use the same behavior.
  2. Canonical SKILL.md fenced findings still survive, while ordinary Markdown and non-canonical *skill.md filenames retain the existing code-example filtering.
  3. The runner and no-LLM CLI regressions remain aligned with the rebased implementation.

rng1995
rng1995 previously approved these changes Aug 14, 2026

@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-review approved at the current head. The previously reviewed fenced-SKILL.md behavior remains acceptable and there are no unresolved review threads. Merge remains blocked: the merge-updated head has nine unit-test regressions in CLI JSON/exit-code behavior plus an unsigned merge commit; the owner must repair CI before merge.

@rng1995

rng1995 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

@rodboev Re-review is approved, but please fix the current CI regressions before merge. The merge-updated head fails 9 unit tests: test_sc8_single_pyc_blocks_install_and_cli_exit; test_cli_writes_report_then_exits_two_for_execution_failure; test_cli_scan_slack_p6_pe3_regression; test_cli_scan_required_table_keeps_malicious_pe3; test_cli_baseline_generate_then_scan_round_trip; test_cli_scan_excludes_selected_baseline_inside_skill; test_cli_scan_excludes_only_the_selected_baseline; test_recursive_single_skill_scan_still_accepts_baseline; and test_cli_scan_json_preserves_single_skill_contract. The failures cluster around CLI JSON output and exit-code behavior after merging current main. The latest merge commit also needs a DCO Signed-off-by trailer. Please push the fixes and get every required check green; I will not merge until then.

rng1995 and others added 5 commits August 14, 2026 16:52
Signed-off-by: Rod Boev <rod.boev@gmail.com>
Signed-off-by: Rod Boev <rod.boev@gmail.com>
…ample-filtering

Signed-off-by: Rod Boev <rod.boev@gmail.com>
…ample-filtering

Signed-off-by: Rod Boev <rod.boev@gmail.com>
@rodboev
rodboev force-pushed the pr/skill-md-fenced-example-filtering branch from 9cf9891 to 636bb8e Compare August 14, 2026 21:02
@rodboev

rodboev commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the CI details. I repaired the unsigned merge commit and narrowed the SKILL.md exception in _scan_path() so only findings inside fenced Markdown blocks bypass the generic code-example filter. Inline and prose examples remain filtered, including the anti-refusal case that was leaking through the path-only guard.

I also added a regression beside the existing fenced SKILL.md, ordinary Markdown, non-canonical path, and CLI JSON coverage. The merge commit and the behavioral fix are both DCO-signed.

…ample-filtering

Signed-off-by: Rod Boev <rod.boev@gmail.com>
@rodboev
rodboev force-pushed the pr/skill-md-fenced-example-filtering branch from fb6f3da to 18f1836 Compare August 14, 2026 21:04

@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-review approved at the final automatic base-update head. The previously reviewed fenced-SKILL.md behavior remains acceptable and there are no unresolved review threads. Merge remains gated on the active full CI run.

@rng1995

rng1995 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

@rodboev - Fix the CI issues

…ample-filtering

Signed-off-by: Rod Boev <rod.boev@gmail.com>
@rodboev
rodboev force-pushed the pr/skill-md-fenced-example-filtering branch from f12b67d to 941854d Compare August 14, 2026 21:38
@rodboev

rodboev commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

I rewrote the latest merge-only refreshes on the current head so every commit in the PR range carries a Signed-off-by: trailer. The reviewed code and three-file diff are unchanged. The branch is force-pushed and ready for the replacement CI run.

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.

2 participants