Repository navigation
AE1 classifies opaque-by-type assets as analysis-evasion HIGH, discarding the ledger reason_code that distinguishes them from real coverage failures #596
Description
Activity
There is a new PR open and working on it: #597.
It is currently a draft while I implement the reason-aware AE1 fix and complete multiple local review passes and end-to-end validation. I will mark it ready for review after those checks.
PR #597 is now ready for review. All changes are pushed at
2fd5f97.Completed two independent local implementation review passes, fixed the identified edge cases, and passed real CLI, MCP stdio, and packaged Docker end-to-end checks. All six hosted checks are green, including 6,007 passing tests; the local integration sweep passed all 34 tests. The PR description records the validation and scope.
GitHub did not allow assigning this issue to @chrisknvidia, so issue assignment still needs a maintainer.
PR #597 is ready for review at
2f59a5bd9010392762610580119531a504d3e3e1.After moving it back to Draft for the deeper passes, I fixed the additional ledger/schema and executable-signature edge cases found by Review Guru, Bugbot, and security review. All six exact-head hosted checks are green; the hosted unit job reported 6,059 passed, 14 skipped, 62 deselected, and 4 xfailed. Packaged CLI, repository Docker smoke, and a 14-scan adversarial Docker matrix also passed.
The PR remains unapproved pending human review. GitHub still does not allow @chrisknvidia to assign this upstream issue because the account has read-only repository permission, so a maintainer must make that assignment.
Reacted by Mohit GuptaPR #597 has merged. The supported reproduction is fixed: a rendered Markdown image with complete, verified minimal PNG bytes and only format-related ledger limitations no longer receives AE1. Current-main graph regressions confirm that behavior.
The final merged scope is narrower than the issue's general opaque-by-type proposal. JPEG/GIF/PDF and other unverified formats, ordinary/active/ambiguous references, and genuine inspection failures still retain AE1. The coverage denominator and incomplete-scan policy are deliberately unchanged: even the supported PNG case remains
CAUTION, incomplete, and MCPsafe_to_install: false.Keeping this issue open as partially resolved for the remaining format-classification/reporting scope, or an explicit maintainer decision to narrow that scope. The merged PNG fix is on
mainat94f5cc8but is not yet in the latest published release, v2.11.2.Independent verification of the merged fix on Windows, and a correction to something I wrote in my own review of #597.
I had measured
224ba29(the #597 merge) before your comment landed; re-ran everything against94f5cc8since that is the head you cite. All four fixtures return identical numbers on both, so the scan-completeness and Markdown-reference work merged in between does not move any of them.Environment: Windows 11, Python 3.14.7, fresh clone,
uv syncper commit, 2.11.2 built from source. Fixtures rebuilt from scratch per leg rather than shared. Every row isuv run skillspector scan <fixture> --no-llm --format json --fail-on-findings.Correction to my #597 review
In that review I offered an adversarial fixture — an ELF binary with an embedded
curl … | bash, namedicon.pngand referenced as a rendered Markdown image — and wrote that because it still raised AE1 and still blocked, the new condition "holds only for a uniquely inventoried target whose limitations are solelybinary_content/opaque_content… that condition is the entire safety property of this change."That was wrong, and the way it is wrong is worth recording.
commit score severity recommendation findings first ledger reason_codeeca8df3(pre-merge parent)64 HIGH DO_NOT_INSTALL AE1, SC9, LP3 excluded_executable_content2fd5f97(the head I reviewed)64 HIGH DO_NOT_INSTALL AE1, SC9, LP3 excluded_executable_content224ba29(merge)64 HIGH DO_NOT_INSTALL AE1, SC9, LP3 excluded_executable_content94f5cc8(current main)64 HIGH DO_NOT_INSTALL AE1, SC9, LP3 excluded_executable_contentByte-identical on the commit before the change. The exception is
excluded_executable_content, notopaque_content, so the fixture never enters the branch the PR touched — the block comes from the older executable path and would have come from it regardless. I presented a control as a test, and "it still blocks" was never evidence about the new condition.In hindsight that also explains why I did not find the gap you did. My fixture varied the content behind a known format; the changed branch keys on how the reference is used. Nothing I built could have reached it.
For anyone auditing this later: the executable path has its own coverage in
test_referenced_executable_binary_keeps_ae1_and_concealment_signal, asserting on that reason code. This is a correction to my reasoning, not a gap in the suite.Your review case, verified against the head that was broken
Rebuilt from your CHANGES_REQUESTED description — a 1 KiB non-UTF-8 blob reached through
Run [the helper](assets/payload.asset).commit score severity findings --fail-on-findingseca8df325 MEDIUM AE1 (HIGH) exit 1 2fd5f970 LOW none exit 0 224ba2925 MEDIUM AE1 (HIGH) exit 1 94f5cc825 MEDIUM AE1 (HIGH) exit 1 The bypass reproduces exactly on the reviewed head and is absent on both sides of it. Worth stating plainly: merged behaviour for this case is identical to pre-merge
main, so the regression existed only on the intermediate head and never reached a release.The supported case behaves as you describe — a rendered Markdown image with a structurally valid minimal PNG goes from 25 / MEDIUM / AE1 on
eca8df3to 0 / LOW / no findings on224ba29and94f5cc8. Note the severity drop as well as the score, since that is a reported field changing value.The completeness numbers, since they were asserted without any
Agreed on scope, and no argument from me about keeping this open. Adding measurements for the part you describe as deliberately unchanged, on
94f5cc8, eight referenced PNGs:total_components: 9 scanned_components: 1 coverage_percent: 11.1 status: partial fully_inspected_files: 1 entirely_uninspected_files: 8 ledger_exceptions: 8 x opaque_content risk_assessment: score 0, LOW, CAUTION, max_issue_severity NONEEight of nine components entirely uninspected, reported at LOW with an empty issue list. That matches your description exactly —
CAUTION, incomplete, nothing claiming the assets were inspected — so I am recording it as confirmation rather than as a complaint. If the remaining scope gets narrowed by an explicit decision rather than fixed, this is the number I would want written into that decision.Thanks to you both for the turnaround.
Disclosure: investigation and testing done with Claude Code on my own Windows machine; Claude drafted this comment. Every number above is from a live run on that box. The retracted claim was mine, published in the #597 review thread.
Related AE1 work has merged in PR #661: missing/ambiguous reference-resolution caveats no longer turn a self-reference to
SKILL.mdinto an AE1 content-inspection finding. Genuine content limitations and incomplete-analysis reporting remain visible.That change explicitly does not resolve this issue's broader opaque-format classification, severity, or coverage-policy scope. The rendered-PNG slice from PR #597 remains the previously delivered subset; keeping this issue open for the remaining requirements.
Summary
AE1 emits
severity: HIGH,confidence: 1.0,category: analysis-evasionfor any resolved reference whose target has a non-clean disposition. It does not read the ledgerreason_code, so a PNG correctly classifiedopaque_contentis reported identically to an artifact that genuinely failed inspection or resolved out of tree.This is the follow-up to #413 that @rng1995 asked for in #413 (comment) — the AE1 policy question, filed separately now that PR #422 has closed the AE3/AE4 slice.
I want to lead with a correction to my own original report, because it no longer holds on
main.Correction: the blocking impact from #413 does not reproduce
#413 reported an 85-byte PNG producing
55/100 HIGH DO_NOT_INSTALL. After #422 that is no longer the behavior, and the remaining AE1 finding does not restore it. Measured onmain@d162d9b343e559be13df8ebba093df3bc9d58c90, SkillSpector2.11.2:skillspector scan ./ae1-png --no-llm --format jsonskillspector scan ./ae1-png4 --no-llm --format jsonskillspector scan ./ae1-png8 --no-llm --format jsonThe score saturates at 43 and never reaches
DO_NOT_INSTALL. So the "preserves the reported blocking impact" premise for keeping #413 open was not correct — the install gate is no longer the problem. What remains is a classification and reporting problem, which is why I think it deserves its own issue rather than continued life on #413.Reproduction
Skill directory, nothing else in it:
SKILL.md:Run
skillspector scan ./ae1-png --no-llm --format json.--no-llmis deliberate — the finding is purely static, so this is deterministic and reproducible without a provider.Observed
One issue:
{ "id": "AE1", "category": "analysis-evasion", "severity": "HIGH", "confidence": 1.0, "finding": "diagram.png (partial)", "explanation": "Referenced artifact was not completely inspected", "remediation": "Make the referenced artifact locally available and fully analyzable, or remove the reference.", "tags": ["coverage", "reference", "target-disposition:partial"] }And, in the same report, the ledger has already recorded exactly why the artifact was not inspected:
{ "outcome": "partial", "phase": "static", "reason_code": "opaque_content", "message": "Artifact contents could not be fully interpreted.", "path": "diagram.png", "fatal": false, "analyzers": ["static_patterns_agent_snooping", "...15 static pattern analyzers..."] }Why I think this is a defect and not just a policy preference
1. The discriminating information exists and is discarded.
LedgerReasonalready separates the cases —BINARY_CONTENT("Binary content is unsupported by this analyzer") andOPAQUE_CONTENT("Artifact contents could not be fully interpreted") atsrc/skillspector/inspection_ledger.py:52and:84— from outcomes likefailed. Ledger events carry that value:event["reason_code"] = reasonatinspection_ledger.py:437and:502. But_reference_coverage_findingsreads only the outcome:reason_codeis never consulted, soopaque_contentand a genuine inspection failure collapse into the samepartialand produce identical findings.2. The codebase already has the idiom, in two places. Ledger events are filtered on their reason elsewhere —
event.get("reason_code") == LedgerReason.OUTPUT_LIMITatinspection_ledger.py:720. And the neighbouring rule in AE1's own module,_size_coverage_findings(AE7), scopes itself the same way:AE7 scopes itself to one reason and stays honest about what it is claiming. AE1 is the outlier in its own file, which suggests an oversight rather than a deliberate policy.
(Minor note while I was in here: AE7 reads
reasonon inventory items while ledger events usereason_code. Two near-identical concepts under two field names in one module is an easy trap for anyone writing the next rule — happy to normalize that separately if you want it, but I've kept it out of the proposal below.)3.
analysis-evasionis the wrong category for this case. Evasion implies an actor did something to avoid inspection. A PNG referenced fromSKILL.md, present on disk, correctly typed, and hashable is not evading anything — the scanner simply does not read image formats. Filing that under evasion atconfidence: 1.0means the category cannot distinguish "someone hid something" from "we don't parse this format," which is the distinction the category exists to make.To be clear about what I am not arguing: the confidence of 1.0 is correct. AE1 states an observed fact — the artifact was not inspected — and that fact is true. My objection is to severity and category, not to the finding's existence or its confidence.
4. The completeness block becomes unreadable, which is the part I actually rely on. The 8-PNG fixture reports:
A reviewer reading that sees a skill 89% uninspected. In reality every inspectable byte was inspected; the other eight files are 70-byte images that no static pattern analyzer will ever read, in this or any future version. Counting permanently-unparseable formats against coverage means
coverage_percentmeasures the scanner's format support rather than how much of the skill got reviewed, and it will read as alarming on every skill that ships a diagram. For anyone using the completeness block as the trust signal — which your own reporting design encourages — this is the most damaging part of the behavior, more so than the score.The counter-argument, stated fairly
Binary assets in skills are a real attack surface: polyglot files, steganographic payloads, an executable renamed
.png. If the project's position is that any uninspectable binary must surface loudly, that is defensible and I am not asking you to drop the signal. I am asking that it not be called evasion, not be scored as HIGH, and not be counted as missing coverage.Proposed change
The defect fix, which I think stands on its own:
reason_codeintoexceptional_outcomesalongside the outcome in_reference_coverage_findings.analysis-evasion, confidence 1.0) forpartial/failedand for anyout_of_scopewhose reason is notBINARY_CONTENT/OPAQUE_CONTENT. Those are the cases where something genuinely went wrong or a reference escaped the tree, and they should stay loud.The policy question, which is yours and not mine: items 1-2 remove a signal without putting anything in its place, and you may not want that. The obvious replacement is a distinct rule for the opaque-by-type case — something like
AE8,category: uninspectable-asset,severity: MEDIUM,confidence: 1.0, remediation along the lines of "this asset's format cannot be statically analyzed; confirm its provenance independently." Same factual claim, honest category, no evasion implication.I'm not proposing that as part of the fix, because it adds a rule id and a category and that is a product decision rather than a defect repair. #422 was scrupulous about not changing AE1 policy, artifact classification, or analyzer registration unasked, and I'd rather not be the PR that does it unasked either.
There's also a reporting question I've deliberately left alone: whether artifacts whose only exception is
BINARY_CONTENT/OPAQUE_CONTENTshould be excluded from thecoverage_percentdenominator and surfaced as a separateuninspectable_by_formatcount. That changes a reported metric other consumers may key on, so it seems squarely yours.Offer
I'm happy to open a PR for items 1-2 — contained to
finalize_inspection_ledger.pyplus test updates. Note for whoever reviews it: there are 33 AE1 assertions across 9 test files, so I'd expect to touch fixtures beyond the obvious ones, and I'd want to confirm none of them are asserting AE1 on a binary deliberately.If you decide you want the
AE8replacement rule as well, I'll write that too — but only on your say-so, and as a separate PR so the defect fix isn't held up behind a policy call. Equally happy to just leave you the reproduction and let a maintainer take it; the fixtures are trivial to regenerate and I can push them as test fixtures if that's useful.Environment
2.11.2,main@d162d9b343e559be13df8ebba093df3bc9d58c90uv run skillspector scan <fixture> --no-llm --format jsonfrom a clean shallow cloneuvRefs #413, #422