fix: strip leading BOM before SKILL.md frontmatter detection - #471
fix: strip leading BOM before SKILL.md frontmatter detection#471MohammedAlkindi wants to merge 3 commits into
Conversation
…tection
decode_text() decodes with plain utf-8, which never strips a byte-order
mark, so a BOM-prefixed SKILL.md left content[0] == U+FEFF and the
content.startswith("---") sniff in _parse_manifest returned an empty
manifest. Every manifest-gated analyzer then early-exits as
not_applicable/manifest_absent, so LP1-LP4 and TP1-TP4 stop contributing
findings for that skill.
Strip leading BOMs in a loop, scoped to delimiter detection. decode_text()
itself is left alone because P2/TP1/P9 treat U+FEFF in file bodies as a
hidden-character injection signal.
Signed-off-by: Mohammed Alkindi <alkndymhmd692@gmail.com>
_extract_skill_name checks prefix.startswith(b"---") on raw bytes before decoding, so a BOM-prefixed sub-skill fell through to the fallback and was listed under its directory name instead of the name: declared in its frontmatter. Byte-level counterpart to the same strip in build_context._parse_manifest. Signed-off-by: Mohammed Alkindi <alkndymhmd692@gmail.com>
yashrajp22
left a comment
yashrajp22
left a comment
There was a problem hiding this comment.
I verified this end to end: the four new tests fail on main (3 of 4 — the no-BOM control passes, as it should) and all pass on this branch, plus the two full affected test files (110 passed). I also reproduced the verdict flip — a BOM'd skill with permissions: ["*"] goes from SAFE back to CAUTION with this fix. The premise checks out too: decode_text() is plain utf-8 + errors="replace", so it never strips a BOM, and the LP/TP analyzers really do gate on if not manifest.
Solid fix. One real gap inline (a third frontmatter sniff this PR doesn't cover) and a small simplification note.
| runtime_limit=runtime_limit, | ||
| ) | ||
| return {} | ||
| while content.startswith("\ufeff"): |
There was a problem hiding this comment.
The PR description says "both places that sniff SKILL.md frontmatter", but I found a third: _frontmatter_bounds in static_patterns_excessive_agency.py (around line 189) does re.match(r"\A---[ \t]*\r?\n", content) on decode_text() output, which keeps the BOM.
I ran it: with plain frontmatter it returns bounds (4, 17); add a leading BOM and it returns None. So EA5's frontmatter-key detection (model-switch keys, selection_surface: "frontmatter") still silently loses findings on a BOM-prefixed SKILL.md — the same silent-miss class this PR fixes.
It's the same one-line class of fix, so worth either adding it here or filing a named follow-up, so the "both places" claim doesn't get taken at face value.
| # scoped to delimiter detection — decode_text() itself stays untouched | ||
| # because P2/TP1/P9 treat U+FEFF as a hidden-character injection signal | ||
| # in file bodies. | ||
| content = content[1:] |
There was a problem hiding this comment.
Tiny simplification: for this string case, content = content.lstrip("\ufeff") does the same thing in one pass instead of the char-by-char loop. Each iteration here copies the whole remaining buffer — I measured the worst case (256 KiB frontmatter window filled with stacked BOMs, ~87k iterations) at ~0.15s, so it stays under the 1.0s MAX_MANIFEST_PARSE_SECONDS budget; it's just untidy, not a real problem.
One caution though: do NOT make the equivalent change to the bytes loop in multi_skill.py. prefix.lstrip(b"\xef\xbb\xbf") strips individual bytes from that set, so a malformed partial BOM like EF BB followed by --- would wrongly pass the sniff. The loop there is correct as written — I tested the partial-BOM case and it properly rejects.
_frontmatter_bounds anchored on \A---, so a BOM-prefixed SKILL.md returned None and EA5 silently reported no frontmatter model pin. This is the third frontmatter sniff in the same class as the two already fixed on this branch. The BOM is absorbed into the opening match rather than stripped, because the offsets this function returns index the caller's original content: _ea5_findings slices it and derives line numbers and context from it, so removing characters would shift every reported position by one.
|
Confirmed and fixed here rather than as a follow-up (db3068a). I matched the BOM rather than stripping it: Unpatched, it gives Keeping the |
2 participants
A UTF-8 BOM defeats both places that sniff
SKILL.mdfrontmatter for a leading---._parse_manifestreturns an empty manifest, so manifest-gated analyzers early-exit asnot_applicable / manifest_absentand LP1–LP4 and TP1–TP4 stop contributing findings._extract_skill_nameseparately falls back to the directory name.decode_text()decodes with plain"utf-8", which never strips a byte-order mark, socontent.startswith("---")fails. YAML 1.2.2 §5.2 permits a leading BOM andyaml.safe_loadparses one transparently, so this is a bug in the sniff rather than correct rejection.Three bytes can flip a verdict. Same skill,
permissions: ["*"]plus an HTML comment indescription:TP1,LP2P2onlyNot wholly silent:
analyzer_statusesreportsmanifest_absentand--verboselogs the skip, thoughlimitationsstays[].Both strip in a loop, covering stacked BOMs.
decode_text()is untouched because P2/TP1/P9 treat U+FEFF in bodies as an injection signal;P2still fires with an identicalmatch_fingerprint. Four tests added, each failing first. Found by inspection, not a user report.