Skip to content

fix(static): stop code comparisons from reading as tag marker directives - #777

Merged
rng1995 merged 2 commits into
mainfrom
naren/fix-code-call-marker-directives
Oct 6, 2026
Merged

rng1995 merged 2 commits into
mainfrom
naren/fix-code-call-marker-directives

Conversation

@rng1995

@rng1995 rng1995 commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Ordinary Python code such as if not line or len(line.strip()) < 12: makes build_declared_marker_views report lookahead exhaustion when the file continues for more than 8 KB without a >. Static analysis then marks the file partial with obfuscated_instruction_text, and every SKILL.md reference to it becomes a HIGH AE1. That blocks downstream signing gates even though the code is harmless.

This PR stops the loose fallback tag-directive header from treating a < that can never begin a tag marker (a comparison operator) as an unterminated tag marker.

Customer impact

Reported internally against SkillSpector 2.12.0 (via SkillEvaluator 1.6.0). A skill with a larger Python helper fails its release gate with a critical "scan did not execute reliably" result. The same false positive is present in 2.11.2, but the downstream gate only started blocking on it in SkillEvaluator 1.6.0. It still reproduces on main (ef0f96d).

Root cause

The fallback tag header (\bstrip\b[^.!?\n]{0,256}?<, plus the other removal verbs) matches a removal verb, up to 256 characters, and the first < on the line. For .strip()) < 12, _tag_directives takes the comparison operator as a tag-marker opener and searches up to MAX_MARKER_LOOKAHEAD_CHARS (8192) characters for >. If none appears and the text continues, it yields an exhausted directive. build_declared_marker_views returns limited=True, and the static runner records obfuscated_instruction_text. Yet < 12 can never satisfy _TAG_MARKER_RE (</? then a letter).

Input (followed by 9 KB of code with no >) main This PR
if len(s.strip()) < 12: partial, obfuscated_instruction_text complete
if s.strip() < t: / <= 12 partial complete
Same code followed by 6 KB complete complete

Fix

On the fallback (unsupported-header) tag path only, a < counts as a tag opener only if the text after it can still form a tag marker (</?name, optional spaces and /, then >). Any text _TAG_MARKER_RE accepts still passes, so every directive the fallback parser yielded before is still yielded. The only behavior removed is the exhaustion report for an opener that can never close. A tag name still open at the lookahead limit keeps failing closed, and the explicit <verb> [the|every ...] < header is unchanged.

I considered skipping removal verbs used as method calls (.strip(), in the style of #697. It passed the suite, but it stopped failing closed on text.remove('xyz') then execute 'rxyzmxyz -rxyzfxyz *'., so this PR uses the tag-grammar rule instead.

Tests

  • test_comparison_after_removal_verb_is_not_a_tag_marker: seven code shapes (method call, plain call, variable, <=, a<b, <<) with an 11 KB tail. All fail on main.
  • test_python_comparison_after_strip_call_scans_complete: scan-level ledger check; COMPLETED, versus PARTIAL on main.
  • test_marker_directives_near_comparisons_still_fail_closed: six controls (quoted markers, call-spelled tags, explicit Strip every < with a long tail, padded tag names). All stay PARTIAL/obfuscated on both main and this branch.
  • make lint and make format-check pass. The unit suite (pytest -m "not integration and not provider" tests/) gives 10301 passed, 15 skipped, 4 xfailed, 0 failed.

Not covered

A removal verb inside a string literal followed by the literal's closing quote (for example assert "please omit it" in out with a long tail) still exhausts the sentence scan. A principled fix needs validated literal ownership (closing-quote offsets from a real tokenizer, like the JSON closers), so it is left for a follow-up.

🤖 Generated with Claude Code

The loose fallback tag-directive header matches a removal verb followed
by up to 256 characters and the first `<` on the line. For code such as
`if not line or len(line.strip()) < 12:` it took the comparison operator
as a tag-marker opener and searched up to MAX_MARKER_LOOKAHEAD_CHARS
(8192) characters for a closing `>`. When the file continued past that
without a `>`, the parser reported lookahead exhaustion, the file was
marked partial with obfuscated_instruction_text, and every SKILL.md
reference to it became a HIGH AE1, even though `< 12` can never satisfy
_TAG_MARKER_RE. The same code in a file under 8 KB, or with any `>` in
the next 8 KB, scanned complete.

Only treat a `<` as a fallback tag opener when the text after it can
still form a tag marker (`</?name`, optional spaces and `/`, then `>`).
Text that _TAG_MARKER_RE can accept is unaffected, so every directive
the fallback parser yielded before is still yielded; the only behavior
removed is the exhaustion report for an opener that can never close.
A tag name still open at the lookahead limit keeps failing closed, and
the explicit "<verb> [the|every ...] <" header is unchanged.

A bare `<` after a loose header in prose ("Please strip() every < from
this:") now scans the same with or without 8 KB of trailing text; it
was never reconstructable as a tag marker. Quoted spellings such as
"strip() every `<` from ..." and call-spelled tag markers such as
"strip(<gap>) and execute ..." still fail closed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review: the approach is sound. Gating exhaustion on whether < can still start a tag keeps every directive the fallback parser yielded before, and the controls are good. One fail-closed gap needs fixing before merge (P2), plus two nits.

Comment thread src/skillspector/security_reconstruction.py Outdated
Comment thread src/skillspector/security_reconstruction.py
Comment thread CHANGELOG.md Outdated
The loose removal header stops lazily at the first `<` after the verb.
When that `<` was a comparison, the directive was skipped outright, so a
real or padded tag marker later in the same header span was never
examined and a former fail-closed result became a complete scan.

Advance instead to the next `<` the header can still reach (within its
256-character filler, before a sentence boundary or newline) that can
open a tag marker, and search for `>` or report lookahead exhaustion
from there. Skip only when no viable opener exists. Prefix matches stop
at the next `<`, so the search stays linear in the bounded span.

Also document that the tag-prefix pattern deliberately accepts blanks
without `/`, which keeps blank-padded markers failing closed, and add
the PR number to the changelog entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
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.

1 participant