Skip to content

fix(artifacts): inspect text behind printable binary prefixes - #794

Open
yashrajp22 wants to merge 26 commits into
mainfrom
yashraj/fix-printable-magic-evasion-20261007
Open

yashrajp22 wants to merge 26 commits into
mainfrom
yashraj/fix-printable-magic-evasion-20261007

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Readable instructions could be hidden behind MZ, GIF, or PDF prefixes and excluded as binary content. These files now retain a lossy text projection even when invalid UTF-8 bytes or NULs are present. Static and provider-eligible inputs can inspect that text while canonical bytes remain available for byte-oriented analysis.

Incidental readable binary content can complete its text scan. Required or referenced opaque content retains its incomplete policy; readable executables explicitly say their text was inspected but binary behavior remains unverified. Referenced readable files and executables also count as partially inspected, so report totals agree with their warnings. Unsupported primary files still fail. Excluded archive members keep their existing policy. Only exact standard XMP header and terminal padding spans receive format treatment; instructions in metadata or unrelated padding remain visible.

Validation: 785 tests passed against source and a freshly installed wheel, including graph, primary-input, and Python-source checks. Five focused synthetic skills matched across source and wheel, covering invalid bytes, NULs, genuine PDF/XMP, and excluded archives. These focused tests made no live provider calls.

Combined verification across the updated PRs: 6,243 regression tests passed against source and again against the freshly installed wheel, with seven conditional skips and four expected failures per run. All 19 source/wheel sample pairs matched. The 12-skill corpus retained its findings and risk ratings; four former hangs now finish with explicit partial-analysis results. The 93 extension tests passed. Two synthetic live NVIDIA Build checks passed on the final wheel: benign-note was complete/SAFE, and the exfiltration sample retained SSD-3 with complete semantic and meta analysis and a DO_NOT_INSTALL recommendation. All seven recorded LLM analyses succeeded. Live checks used the configured model/reasoning defaults through a test-only proxy that kept the real credential outside the scanner.

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

[SkillSpector Review]

Hi @yashrajp22, thank you for tracking down a real evasion path and routing the fix through the existing static, text-integrity and LLM inputs instead of adding a parallel scanner! Keeping content_kind BINARY and marking the claimed container format partial is also a careful way to avoid overstating PDF support.

Value and readiness: The problem is real on main. With main's graph (use_llm=False), an unreferenced GUIDE that starts with GIF89a or %PDF- and carries agent instructions is binary/out_of_scope for all 15 static_patterns_* analyzers, is left out of llm_components, and the scan is SAFE with is_complete True. Only the raw-byte YARA rule YR4 fires (score 20) for the classic "Ignore all previous instructions" phrase, and a reworded payload gets no findings at all. The same text without the prefix gives P1/PE3 and CAUTION. For payloads that are strict UTF-8 with no NUL, the PR does what it says: every behavioural hunk is gated on the new readable_binary flag, and such files reach the pattern scanners, artifact_integrity and llm_file_cache. It is not ready for final maintainer review yet, for two reasons. First, the gate is easy to step around: one trailing invalid byte, one NUL, or the standard PDF binary-comment line clears readable_binary, and the scan is again SAFE and complete with the instructions unscanned (finding 1). Second, the change introduces a false DO_NOT_INSTALL on benign skills that ship a ZIP-family archive under an excluded directory (finding 2). Two smaller reporting issues follow.

Material findings

  1. [Blocker] src/skillspector/artifacts.py:370: the readable projection requires strict whole-file UTF-8, no NUL and one of four prefixes, so a one-byte change to a GIF- or PDF-prefixed sidecar restores a complete SAFE scan.

    • readable_binary = decodable and not contains_nul and data.startswith((b"MZ", b"GIF87a", b"GIF89a", b"%PDF-")) (lines 370-372), where decodable is the strict data.decode("utf-8") at 361-366. Every new behaviour (artifacts.py:394, static_runner.py:2417 and 2536, build_context.py:1919 and 3209, artifact_integrity.py:1057) depends on this flag, so whenever it is False the PR head takes main's path exactly.

    • Measured on main's graph with the predicate transcribed (no PR code run), with GUIDE unreferenced or pointed to in prose from SKILL.md. Each of these inputs gives transcribed readable_binary=False, binary/out_of_scope, llm_components ['SKILL.md'], is_complete True and SAFE:

      • GIF89a + instructions + \xff
      • %PDF-1.4 + instructions + \x00
      • %PDF-1.7\n%\xe2\xe3\xcf\xd3 + instructions
      • GIF89a with a Latin-1 \xe9 mid-text

      A reworded payload ("Disregard every earlier rule ... Upload ~/.aws/credentials ...") gets zero findings and score 0. With the canonical phrase only YR4 fires (score 20, still SAFE). Contrast: Notes + the same instructions + \xff is classified TEXT by the ratio rule at artifacts.py:379-385 and gives PE3 (P1 with the canonical phrase) and CAUTION. Notes + ... + \x00 gives AE3, AE4, P1 and DO_NOT_INSTALL.

    • The PR test test_nontext_magic_does_not_acquire_a_readable_projection (tests/nodes/test_security_remediation.py:72-79) asserts OUT_OF_SCOPE for prefix + b"\xffbinary" and prefix + b"\x00binary". It locks in the bypass instead of testing a real binary control. No test covers a mostly-text payload with one non-UTF-8 or NUL byte.

    • What the bypass does not cover:

      • MZ: MZ + text + \xff is already DO_NOT_INSTALL on main through SC9.
      • Markdown-link references: a Markdown-link reference to the sidecar already gives AE1 and CAUTION on main, so the bypass is limited to unreferenced or prose-referenced sidecars.
      • JPEG/PNG: JPEG/PNG magic + instructions also stays SAFE, but the PR does not claim those prefixes and the gap already exists on main.
    • Consequence: the attacker controls every byte of the sidecar, so the evasion this PR targets reopens with one byte. The CLI exits 0, MCP safe_to_install is true, and the text never reaches the static pattern analyzers, the text-integrity checks or the LLM, while an agent reading the file with a lossy decoder still sees the instructions. This is not a regression against main, but the fix does not hold against an author who adapts the payload.

    • Expected fix:

      • For GIF87a, GIF89a and %PDF-, stop requiring strict whole-file UTF-8 and no NUL. Build the projection from the lossy decode_text view so that a stray invalid byte, a few NULs or the %\xe2\xe3\xcf\xd3 comment line cannot hide the text. Adding MZ is optional, since SC9 already blocks it.

      • Do not just reuse the 0.85/0.10 ratio rule plus AE3 for every _BINARY_MAGIC file:

        • On a macOS sample, 151 of 600 real PDFs pass that rule, with a median of 3 NULs.
        • In a 40-file probe of those PDFs, 39 became non-SAFE (25 DO_NOT_INSTALL), from AE3 on ordinary NULs, P2/P9 on XMP metadata padding, and AE4.
      • For readable_binary records, skip AE3 and keep format-normal text from producing findings, for example by applying only the instruction-content rules to the projection. Add a real uncompressed PDF fixture with an XMP packet that expects no AE3, P2 or P9.

      • Replace the \xffbinary/\x00binary negative cases with genuinely binary controls, such as the real 1x1 GIF pixel in test_opaque_reference_reporting.py and a Flate-compressed PDF.

      • Add graph-level cases on an unreferenced sidecar, each expecting a finding on the sidecar and a non-SAFE verdict:

        • prefix + instructions + \xff
        • prefix + instructions + \x00
        • a Latin-1 byte mid-text
        • the PDF binary-comment line

        Use wording outside the YARA prompt-injection rule so that YR4 alone cannot satisfy these tests.

      • If maintainers keep the strict gate, state the one-byte limitation in the PR description, and do not present it as intended behaviour in the negative test.

  2. [Blocker] src/skillspector/nodes/build_context.py:3209: a readable printable-prefix member of a ZIP under an excluded directory (for example node_modules) turns a benign skill into DO_NOT_INSTALL.

    • How it happens:
      • classify_artifact returns PARTIAL for every readable_binary record regardless of scope (artifacts.py:394), and members of archives under excluded directories are still classified (nested_artifacts.py:1105).
      • The excluded-member loop only rewrites ANALYZED to OUT_OF_SCOPE (build_context.py:3084-3088). PARTIAL therefore survives, and the member also loses its excluded_directory reason.
      • The metadata loop then sets excluded_inspection_incomplete=True (3096-3101). That flag drives SC9 HIGH "An excluded artifact could not be completely inspected." (static_patterns_supply_chain.py:3466, 3500) and the score floor of 51 (report.py:600-609).
      • The new loop at build_context.py:3209-3218 also adds a PARTIAL opaque_content event for these rows, with no scope check.
    • Measured with main's code plus a transcription of all four PR source hunks. The transcribed build_context.py, static_runner.py and artifact_integrity.py are byte-identical to the PR head.
      • Input: a benign SKILL.md plus node_modules/pkg/docs.zip containing manual.pdf = b"%PDF-1.4\n1 0 obj\n<< /Type /Catalog >>\nendobj\n%%EOF\n". This is the minimal PDF from test_graph_keeps_ae1_for_unverified_binary_formats.
        • main: out_of_scope, no findings, complete, SAFE, score 0.
        • Transcribed PR: partial, SC9 HIGH, partially_inspected_files=1, score 51, DO_NOT_INSTALL.
      • A .venv wheel with pkg/icon.gif = GIF89a\nicon placeholder\n flips the same way.
      • The same archive holding a binary-marked PDF stays SAFE on both. A plain-text injection member in the same excluded ZIP stays out_of_scope. So the readable stub scores worse than both the opaque file and plain text.
    • Consequence: benign input gets the worst verdict, with a HIGH finding that reports an inspection failure for content the policy deliberately excludes. None of that member's text is inspected, because excluded members are dropped from components and caches at 3179-3195. The flip applies to GIF87a, GIF89a and %PDF- members; MZ members are already SC9 on main, and the PR only rewords that message. Real PDFs and GIFs with binary bytes are unaffected, so frequency is likely low: text stubs, fixtures and hand-made PDFs.
    • Expected fix:
      • In the excluded-member loop (build_context.py:3084), also map a readable_binary member to OUT_OF_SCOPE with nested_exclusion_reason, for example if nested_artifact["disposition"] is ArtifactDisposition.ANALYZED or nested_artifact.get("readable_binary"):.
      • Skip the new event at 3209 for paths in excluded_nested_components, or gate it on the PARTIAL disposition after the change above.
      • An equivalent alternative: set the PARTIAL disposition in build_context only for in-scope components, instead of in classify_artifact.
      • Add a regression test for node_modules/pkg/docs.zip with the ASCII PDF member. Expect the member to be out_of_scope with reason excluded_directory, no SC9, is_complete True and SAFE.
      • Optional: a test that a root-ZIP member with injection text gives P1. That path works in the transcription but has no test.
  3. [Non-blocking] src/skillspector/nodes/build_context.py:1919: readable MZ files are fully scanned and placed in the LLM inputs, but are still reported as "excluded from analysis".

    • The PR keeps content_kind BINARY, and an MZ prefix is executable magic (nested_artifacts.py:379-380, 485-500). The unchanged _mark_unanalyzed_executables therefore takes its BINARY branch (build_context.py:996), which:

      • sets excluded_from_analysis, local_only, concealed_executable and inherited_exclusion_reason=binary_content;
      • emits the "Executable content was inventoried but excluded from content analysis." ledger exception.

      SC9 then says "Executable content is excluded from analysis.". Meanwhile line 1919 puts the file in llm_file_cache, llm_components is fixed at 3263 before the marking at 3444, and the PR's own test expects P1 on the MZ GUIDE.

    • Transcription (main's code with the classify, LLM-cache and static-runner hunks applied):

      • GUIDE is partial, with findings P1, SC9 and YR4.
      • All 15 static_patterns_* ledger rows are completed, and the file is in llm_components.
      • Yet the component flags and the SC9 and ledger messages still say it was excluded.
    • Consequence: the report contradicts itself about one file, which misleads anyone reading the JSON. The verdict is unaffected: DO_NOT_INSTALL either way. A local_only flag next to llm_components membership is not new; on main, a size-truncated executable shell script already has both. So this is a wording and metadata problem, not a new privacy leak.

    • Expected fix:

      • In _mark_unanalyzed_executables, give readable_binary executable records their own reason, SC9 message and ledger message (readable bytes analyzed; executable format not interpreted), and stop setting excluded_from_analysis for them.
      • Keep the fail-closed floor on purpose. report.py:501 and 600-609 both key on excluded_from_analysis, so skipping the function entirely would likely drop a readable MZ from DO_NOT_INSTALL to CAUTION. This is inferred from the GIF89a case, which ends at CAUTION.
      • Extend the MZ test case to assert the SC9 message, the GUIDE component flags and risk_recommendation.
  4. [Non-blocking] src/skillspector/artifacts.py:394: benign unreferenced ASCII-only PDFs and GIFs move from SAFE/complete to CAUTION/partial, while binary-marked PDFs stay SAFE, and the docs and ledger wording are not updated.

    • The disposition at line 394 is PARTIAL for every readable_binary record, referenced or not. The new event at build_context.py:3209-3218 adds an opaque_content exception on top.

    • Transcribed results:

      • An unreferenced manual.pdf containing %PDF-1.4\nA plain text reference document.\n is complete/SAFE on main and partial/CAUTION on the PR.
      • The same file with the %\xe2\xe3\xcf\xd3 line is SAFE on both.
      • An ASCII GIF, and a root docs.zip holding the ASCII PDF, flip the same way.
      • With the new event removed, the result is still partial/CAUTION, so line 394 is the root cause.
    • The PR test test_graph_keeps_benign_readable_pdf_partial_without_instruction_findings (tests/integration/test_opaque_reference_reporting.py:354-364) asserts this CAUTION, so the policy is deliberate.

    • Real files:

      • 12 of 546 local PDFs match the predicate. They are uncompressed vector-icon PDFs from app bundles.
      • Three of them, placed in a skill's assets/, flipped SAFE to CAUTION with zero findings.
      • A separate sample found 0 of 398 PDF and GIF files. GIFs and executables matched 0 times.
    • Consequence: such skills lose MCP safe_to_install and fail --fail-on-incomplete, and the more-inspected file gets a worse verdict than an uninspected binary PDF. The docs and wording also no longer match:

      • docs/scan-completeness.md still promises complete / exit 0 / safe_to_install true in the "Unreferenced incidental image" row (line 121), and has no row for this case.
      • The reused opaque_content reason says "Artifact contents could not be fully interpreted.".
      • Its remediation in docs/ANALYSIS_RESOURCE_BOUNDS.md:120 refers to "the referenced format", even for unreferenced files.
    • Expected fix: pick one policy and apply it at both sites (artifacts.py:394 and build_context.py:3209-3218). Either:

      • document it: a row in docs/scan-completeness.md, plus a distinct, accurately worded ledger reason such as "text projection inspected; claimed container format not interpreted"; or
      • narrow the marking: unreferenced files fully analyzed as text become ANALYZED or a scope exclusion, PARTIAL stays for referenced files or files whose projection produced findings, and the test is updated to match.

      This choice interacts with the fix for finding 1; see PIC tradeoffs.

PIC tradeoffs:

  • Keeping content_kind BINARY and adding a side flag preserves AE1/AE2 semantics for referenced PDFs. The cost is that every consumer that branches on BINARY now also has to check readable_binary, and the two consumers that were not updated cause findings 2 and 3. Deciding the readable/partial disposition in build_context, where scope is known, would keep that logic in one place.
  • Requiring strict UTF-8 and no NUL avoids treating real GIF or PE files as text, but leaves finding 1 open. A tolerant gate closes it, but also admits real uncompressed PDFs (151 of 600 in one sample). If the unconditional PARTIAL policy from finding 4 stays, that would move roughly a quarter of ordinary PDFs from SAFE to CAUTION. Fixing finding 1 therefore needs a decision on finding 4 as well: either mark PARTIAL only on evidence, or accept and document that CAUTION rate.
  • Marking every readable printable-prefix file PARTIAL keeps completeness reporting honest about the unparsed container, since ASCIIHex/ASCII85 streams are not decoded. The cost is SAFE verdicts for ASCII-only PDFs.
  • The four-prefix tuple at artifacts.py:371 duplicates part of _BINARY_MAGIC (artifacts.py:190-201) without a named constant. A future magic addition will therefore not be reviewed for the same evasion. A named constant derived from _BINARY_MAGIC would make the choice explicit.

Verification and gaps:

  • How this was verified:
    • Code traces on the head bd46d38, which differs from main 3c8e4b9 only in the PR's six files.
    • Runs of main's graph with use_llm=False.
    • Small transcriptions of the PR hunks applied to main's code. For the finding 1 inputs, readable_binary is False, so the head follows main's path exactly.
  • Key numbers:
    • Bypass inputs: SAFE and is_complete True, with score 0 (reworded payload) or 20 (YR4 only).
    • Excluded-ZIP case: SAFE with score 0 on main, DO_NOT_INSTALL with score 51 transcribed.
    • Real PDFs: 151 of 600 pass the 0.85/0.10 rule, against 1 of 600 for the PR's predicate.
  • Inputs outside the new flag behave as on main:
    • Static-runner gating is consistent across both entry points (_is_binary_file checks only for NUL, which readable_binary rules out).
    • The fail-closed path for the primary file (unsupported_primary_content) is unchanged.
    • Hidden files stay out of the LLM inputs.
    • readable_binary is a NotRequired key and is not serialized into JSON, SARIF, Markdown or MCP output.
  • Tests: the new tests exercise real code paths without mocks. Traced, not run, they fail on main as expected: KeyError on readable_binary, GUIDE missing from llm_components, no P1, and complete/SAFE where they expect otherwise. None of them would catch finding 1 or 2, and the negative test at tests/nodes/test_security_remediation.py:72-79 asserts the bypass behaviour.
  • CI: no checks have run on the head. The workflow run is action_required and is waiting for a maintainer to approve it.
  • Conflicts: none with main. This PR shares build_context.py with #793 and #795, and static_runner.py with #741, and no semantic conflict was found:
    • #793's reserved-path FAILED guards run before the new loop, and FAILED dominates.
    • #795 changes a separate OMS code path.
    • #741 only touches regex budgeting.
  • Head update: I reviewed bd46d3828c169e8d5b2be109fdb777047e09a229. The current head 4a0cbd8b8182c07bb9c415e4c22b2a6ddadfe443 only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits change only the ANALYZE_USES_POSTPROCESS hook in static_runner.py (two lines in _scan_path), which is unrelated to this PR's hunks, so line references to static_runner.py below are to the current head. The PR's own diff is unchanged (same patch-id), so this review applies to the current head.
  • I did not run the PR's tests or code, per policy.
  • Gaps:
    • No live LLM provider was exercised; provider submission was inferred from llm_file_cache and llm_components.
    • Local runs used Python 3.13 on macOS; CI uses 3.12.
    • How agent runtimes display a file with one invalid byte or a NUL was not checked. The scanner's own TEXT heuristic was used as the yardstick.
    • Real-world frequency of the finding 2 and 4 triggers was only sampled locally.
    • Out of scope here: main's non-magic TEXT path can also be pushed to BINARY by appending about 40 \xff bytes, so matching the TEXT rule alone does not close the whole evasion class.

Decision: Changes Requested (reviewed head bd46d3828c169e8d5b2be109fdb777047e09a229; current head 4a0cbd8b8182c07bb9c415e4c22b2a6ddadfe443 only adds merges of main)

Comment thread src/skillspector/artifacts.py Outdated
Comment thread src/skillspector/nodes/build_context.py Outdated
Comment thread src/skillspector/nodes/build_context.py
Comment thread src/skillspector/artifacts.py Outdated
github-actions Bot and others added 19 commits October 8, 2026 22:45
… policy

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
…ncomplete

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>

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

[SkillSpector Review]

Hi @yashrajp22, thank you for the quick turnaround on all four earlier findings. Every byte-level bypass from the first round now produces findings, readable MZ files get their own binary_executable_text reason and SC9 wording instead of being reported as excluded, and the excluded-archive and incidental-stub cases each have a graph-level regression test. Replacing the negative test that locked in the bypass with positive runner-level cases is especially appreciated.

Value and readiness: The evasion this PR targets is now closed for the GIF, PDF and MZ prefixes. On a transcription of the head (see Verification), an unreferenced or prose-referenced sidecar with a trailing \xff, a \x00, the PDF binary-comment line or a Latin-1 byte goes from SAFE (score 0) on main to CAUTION with PE3 and E1. The other three earlier findings are fixed as the replies describe. The PR is not ready yet, for two reasons:

  • The gate that closed earlier finding 1 is now the prefix alone (artifacts.py:374). Every real GIF, PDF and PE file is therefore lossily decoded in full and handed to every static analyzer, artifact_integrity and the LLM inputs. In a local sample of 49 real PDFs and GIFs that are all SAFE and complete on main, 47 gain a false AE4 and 12 become CAUTION or incomplete. A separate 1.2 MB PDF becomes DO_NOT_INSTALL, and multi-MB files take 200-300 s to scan (finding 1).
  • CI test-unit is red because the same gate changes the result of an existing main test for byte-only executables (finding 2).

The #579 interplay flagged after the first round is still present (finding 3), and three smaller issues share the root cause of finding 1.

Previous findings:

  1. Earlier finding 1 (one byte restores a SAFE scan; inline thread on artifacts.py): Partly resolved.

    • Fixed: the gate is now data.startswith((b"MZ", b"GIF87a", b"GIF89a", b"%PDF-")) (artifacts.py:374), and the lossy decode_text view is scanned. AE3 is skipped for readable records (artifact_integrity.py:1064-1067), NUL-only P2 is filtered (static_runner.py:3069-3076), and a single standard XMP packet's BOM and padding are exempt (artifacts.py:416-442, static_patterns_prompt_injection.py:369-375 and 502, static_patterns_memory_poisoning.py:544-554). The negative test that asserted the bypass was replaced (tests/nodes/test_security_remediation.py:72-79), and runner-level positive tests cover \xff, \x00 and the binary-comment line (lines 82-105).
    • Verified (transcription, use_llm=False): GIF89a + reworded payload + \xff, %PDF-1.4 + payload + \x00, %PDF-1.7\n%\xe2\xe3\xcf\xd3 + payload, a Latin-1 \xe9 mid-text and the prose-referenced variant all go from SAFE 0 on main to PE3 and E1 on the sidecar, CAUTION 27, with the file in llm_components. MZ + payload + \xff adds PE3 and E1 on top of SC9. NULs or \xff bytes inside keywords still give PE3/E1 or P1.
    • Remains: the expected fix also asked to keep format-normal content of real PDFs from producing findings. That now holds only for AE3, NUL-only P2 and a single XMP packet inside the first scan window. Compressed PDF streams, GIF LZW data and PE code now produce AE4, AE6, P2, P9, MP2 and TM1, plus parse and runtime limits. See new findings 1, 4 and 5.
    • Author reply: accurate on the bypass. "Normal binary NULs and the exact XMP header and padding are handled as format data" is true only in the narrow cases above, and the reply does not mention the effect on real files.
  2. Earlier finding 2 (readable member of an excluded archive becomes SC9 / DO_NOT_INSTALL; inline thread on build_context.py): Resolved.

    • Fixed: the excluded-member loop maps a reason-less readable_binary member to OUT_OF_SCOPE with the inherited reason (build_context.py:3164-3168). The opaque-content block skips excluded_nested_components and applies only to referenced files (3291-3306). Regression test: test_security_remediation.py:3824.
    • Verified (transcription): node_modules/pkg/docs.zip!/manual.pdf is out_of_scope with reason excluded_directory, and the scan is SAFE 0 and complete. A .venv wheel with pkg/icon.gif behaves the same. An MZ member in an excluded ZIP is unchanged from main (excluded, SC9, score 51).
    • Author reply: accurate.
  3. Earlier finding 3 (readable MZ scanned and sent to the LLM but reported as excluded; inline thread on build_context.py:1964): Resolved.

    • Fixed: build_context.py:1029-1070 gives readable executables the binary_executable_text reason, sets partially_analyzed_executable True and excluded_from_analysis False, and makes the disposition PARTIAL. SC9 wording was added (static_patterns_supply_chain.py:3493-3535), the score floors were extended (report.py:532-536, 634-646), and a new ledger reason was added (inspection_ledger.py). Test: test_security_remediation.py:3842.
    • Verified (transcription, MZ + canonical GUIDE): the flags are as above, and SC9 says "Readable text was inspected, but binary executable behavior was not analyzed." P1 and YR4 are present, partially_inspected_files is 1, and the verdict is DO_NOT_INSTALL at score 84 (main 58). A hidden .tools/GUIDE and bundle.zip!/GUIDE behave the same.
    • Author reply: accurate for the flags, messages, counts and floor. It does not say that the change now applies to every real PE file, including byte-only ones. That is what changes the existing main test in new finding 2.
  4. Earlier finding 4 (benign unreferenced ASCII PDF/GIF goes from SAFE to CAUTION; inline thread on artifacts.py): Resolved for the cases raised.

    • Fixed: unreferenced readable files are ANALYZED (artifacts.py:394-402). PARTIAL and the opaque_content event apply only to referenced, non-excluded files (build_context.py:3291-3306). Docs rows were added (docs/scan-completeness.md:122-123).
    • Verified (transcription): an unreferenced %PDF-1.4\nA plain text reference document.\n, its binary-comment variant, an ASCII GIF and a root docs.zip holding the ASCII PDF are all SAFE 0 and complete, as on main. A Markdown-linked manual.pdf is partial with AE1 and an opaque_content exception, CAUTION 25 (main: CAUTION 25).
    • Author reply: accurate for the ASCII stubs and the referenced/unreferenced policy. The new docs row 122 overstates behaviour for real files; see new finding 6.
  5. Impact-check item after the first round (#579 interplay: promote_artifact_to_decoded_text turns the PR's reason-less PARTIAL BINARY row into TEXT/ANALYZED while the PR's opaque_content event stays): Still open.

    • This was not raised as a GitHub thread, so there is no reply. The event is still appended at build_context.py:3291-3306, before the Python-source decode loop that promotes the row at 3442, and it is never withdrawn. See new finding 3.

Material findings

  1. [Blocker] src/skillspector/artifacts.py:374: the prefix-only gate hands the whole lossy decode of every real GIF, PDF and PE file to every analyzer. The result is false findings, incomplete scans and multi-minute runtimes.

    • Cause: readable_binary = data.startswith((b"MZ", b"GIF87a", b"GIF89a", b"%PDF-")) has no other condition, and for an unreferenced file it gives ANALYZED (artifacts.py:397-398).
      • build_context caches the full decode_text view (UTF-8 with errors="replace") up to 16 MiB.
      • static_runner.py:3113-3149 removes these paths from binary_paths, so every pattern module scans the whole projection.
      • artifact_integrity.py:1057-1088 runs _text_signals on it. AE4 fires at a format density of 1%, and that density counts every control character, including NUL.
      • The only binary-specific filters are NUL-only P2, AE3, and the single-packet XMP BOM and padding.
      • At the earlier reviewed head the gate also required strict UTF-8 and no NUL. Almost no real files qualified (about 12 of 546 local PDFs, all ASCII icon PDFs), and none got findings or slowdowns.
    • Measured on a transcription: main 5edc347 plus the PR's src diff, byte-identical to the head's src. Each run is a benign SKILL.md plus one unreferenced real file under assets/, with use_llm=False. The files are a local macOS sample, so the counts are not population rates.
      • 40 real PDFs (676 B to 369 KB):
        • Main: 40/40 SAFE and complete, 1.4 s in total.
        • Head: AE4 MEDIUM on 38/40, MP2 on 4, P2 on 3, P9 on 1. 28 keep SAFE at score 8 with a false AE4. 10 become CAUTION or incomplete, 8 of them through static_parse_limit. 384 s in total.
        • The slowest, a 369 KB PDF, took 76-79 s and is CAUTION 29 and incomplete, with P2 HIGH on U+2067 (main: SAFE in 0.17 s).
      • 9 real GIFs (43 B to 2.1 MB):
        • AE4 on 9/9, including a 43-byte 1x1 GIF.
        • A 1.1 MB GIF is CAUTION 23 and incomplete after 212 s (main: SAFE in 0.14 s).
        • A 2.1 MB GIF gets AE4, P2 and TM1, is CAUTION 44, and hits runtime_limit after 301 s.
      • A 1.2 MB PDF (a dictionary help image) is DO_NOT_INSTALL, score 89, incomplete, after 227 s (main: SAFE 0 in 0.22 s). Where the findings come from:
        • one P2 HIGH on U+202E decoded from Flate data;
        • two P2 HIGH on the U+FEFF BOMs of the file's two XMP packets;
        • 42 P9 on that XMP padding;
        • 137 MP2 on uncompressed font /Widths [... 0 0 0 0 ...] arrays;
        • TM1 HIGH on decoded noise, plus AE4.
      • 10 real PE launchers (pip/distlib and setuptools):
        • All stay DO_NOT_INSTALL and gain AE4. 6/10 hit static_parse_limit.
        • The 4 pip/distlib launchers rise from 51 to 90, 90, 90 and 100. w64-arm.exe scores 100, with AE6 HIGH and MP2 on runs such as \x01\x02\x01\x02..., and takes about 30 s.
        • The 6 setuptools launchers stay at 51.
    • Tests: no fixture has a compressed stream, multiple XMP packets, real GIF frame data or a PE body, and none checks runtime. The XMP fixture (test_security_remediation.py:3808-3812) is an uncompressed single-packet PDF. The real 1x1 GIF in test_opaque_reference_reporting.py:197-198 now gets AE4, but no test asserts against that.
    • Consequence: benign skills that ship ordinary PDFs or GIFs, such as documentation or demo animations, gain findings, so --fail-on-findings exits 1 for almost all of them (cli.py:701, 869). Some also lose SAFE or complete status, which makes MCP safe_to_install false and makes --fail-on-incomplete exit 1. In this sample one ordinary PDF produced DO_NOT_INSTALL. A few MB of such assets approaches the 600 s workflow deadline (state.py:46).
    • Expected fix: keep the evasion fix, but do not treat the whole lossy decode of a binary format as text.
      • Projection: build it from printable text runs, strings-style, with non-text bytes and very short runs collapsed to separators so line mapping is kept. Cap its size so runtime stays bounded on large uncompressed PDFs.
      • Rule set: a run filter alone is not enough, because the MP2 hits on /Widths arrays and the P2/P9 hits on multi-packet XMP sit inside printable runs.
        • For readable_binary records, run instruction rules (P1, PE3, E1 and similar) on the projection.
        • Skip or exempt P9, BOM-only P2, and MP2 on PDF array or stream structure.
        • Do not run TM* by default: TM1 HIGH fired on noise in both the 1.2 MB PDF and the 2.1 MB GIF.
        • Make AE4's density ignore control bytes outside printable runs.
      • New tests: add negative graph fixtures that expect SAFE, complete and no findings from the binary data: a Flate-compressed PDF with two XMP packets, a multi-frame GIF and a PE stub. Add a bounded-time case, for example 1 MB of random bytes behind GIF89a finishing well under the deadline.
      • Existing tests: keep test_security_remediation.py:108-118, which uses a real Cyrillic text run. Keep the earlier bypass inputs as graph-level regression tests that expect a non-SAFE verdict.
  2. [Blocker] tests/nodes/analyzers/test_sc8_shipped_bytecode.py:166 (a main test outside the diff; the inline comment is on build_context.py:1064): CI test-unit fails because the prefix-only gate changes the SC9 contract for byte-only executables.

    • CI history:
      • Run 37934042362 on eaebfab ended with "1 failed, 12900 passed". test_ordinary_binary_executable_cannot_remain_safe fails at line 166, assert issue["evidence"]["excluded_from_analysis"] is True.
      • The same failure was already present at 4ba349f (run 37906437298). The last green PR run was at 2529288, before commit 781475c removed the strict gate.
      • Main at 5edc347 passed (run 37933993525, 12,857 passed).
    • Mechanism: the fixture payload.exe = b"MZ\x00\x00binary payload" is now readable_binary (artifacts.py:374). build_context.py:1029-1035 picks binary_executable_text, and lines 1064-1065 set excluded_from_analysis False and partially_analyzed_executable True. Line 167 (binary_content) would fail too.
    • Conflicting tests: the PR's own test at test_security_remediation.py:72-79 asserts the opposite contract for MZ + \x00binary (readable, ANALYZED). The two tests now encode different policies.
    • Consequence: a required check is red and the PR cannot merge.
      • The verdict is not weakened. SC9 stays HIGH, and the floor of 51 still applies through partially_analyzed_executable (report.py:532-536, 634-646). Line 162's exit-code assertion passed in CI.
      • Every byte-only PE is now labelled "Readable text was inspected", which is not true.
      • The PR body's validation section (785 focused tests, 6,243 combined regression tests) does not mention this failure.
    • Expected fix: pick one contract and make both tests agree.
      • If byte-only PEs should count as partially analyzed: update test_sc8_shipped_bytecode.py:166-167 to excluded_from_analysis is False, partially_analyzed_executable is True and binary_executable_text, keep lines 168-169, and say so in the PR body.
      • If not: make MZ readable_binary only when there is real readable text beyond the header (this falls out of the finding 1 projection), so MZ\x00\x00 + bytes stays binary_content. Keep this test unchanged and keep the readable-MZ case covered by test_security_remediation.py:3842. Dropping MZ from the tuple is also possible, since the SC9 floor already blocks every MZ file, but it would undo the earlier finding 3 wording.
      • In either case, run the full test-unit suite before pushing.
  3. [Non-blocking] src/skillspector/nodes/build_context.py:3291: a referenced Python helper whose first bytes are MZ, GIF87a or GIF89a keeps a stale opaque_content event, which gives a false AE1 HIGH and an incomplete scan.

    • Mechanism:
      • Lines 3291-3306 set PARTIAL and append a PARTIAL opaque_content event before the Python-source loop.
      • At 3442, promote_artifact_to_decoded_text (artifacts.py:445-456) sees BINARY + PARTIAL + no reason and makes the row TEXT/ANALYZED, but the event is never withdrawn.
      • finalize_inspection_ledger.py:590-605 then raises AE1 from the partial event on a referenced target.
    • Transcription input: SKILL.md with See [the helper](scripts/scale.py), where scripts/scale.py is MZ_SCALE = 2\nprint(MZ_SCALE)\n.
      • Main: SAFE 0, complete.
      • Head: CAUTION 25, is_complete False, AE1 HIGH on SKILL.md. The inventory row is text/analyzed but carries an opaque_content partial exception. partially_inspected_files and entirely_uninspected_files are both 0, so the counts disagree with is_complete.
      • scripts/img.py starting with GIF89a_HEADER = b'GIF89a' gives the same result.
      • Controls: an unreferenced MZ helper and a referenced plain helper are both SAFE and complete. A referenced docs/notes.txt starting with MZ stays BINARY/PARTIAL, as intended. %PDF- is not valid Python, so it takes a different path.
    • Consequence: a benign skill gets a false HIGH finding, CAUTION and an incomplete scan. The trigger is rare.
    • Expected fix: emit the event and set PARTIAL only for artifacts that are still BINARY after the Python-source decode loop. Either move the block after build_context.py:3394-3460, or skip paths whose decode_python_source succeeded. Add a graph test with the input above that expects complete, SAFE and no AE1.
  4. [Non-blocking] src/skillspector/nodes/build_context.py:1964: the full lossy text of every non-hidden GIF, PDF and PE file (up to 16 MiB each) now enters the LLM inputs.

    • Code path:
      • Lines 1961-1972 store _redact_for_external_model(path, content) in llm_file_cache for any non-hidden readable_binary row. That redaction only changes .env files.
      • No later llm_file_cache.pop applies to ordinary assets, and llm_components is taken at 3527.
      • _mark_unanalyzed_executables sets local_only (1056) without removing the entry, and the semantic analyzers read llm_file_cache with no content_kind or local_only filter.
      • The per-artifact cap is 16 MiB (constants.py:41, ANALYSIS_RESOURCE_BOUNDS.md:18).
    • Main baseline, run with main's code: a 1.2 MB PDF, a 1.1 MB GIF and a setuptools cli-64.exe beside SKILL.md give llm_components == ['SKILL.md']. On the head all three are added.
    • Token estimates: main's estimate_tokens on the cached provider view, with only the gate transcribed and no live provider.
      • Totals: 41 real PDFs 804,711 tokens; 39 real GIFs 2,311,636; 10 PE launchers 133,325.
      • Single files: the 1.2 MB PDF 289,753; the 1.1 MB GIF 267,990; a 2.4 MB GIF 570,143.
    • Batching:
      • The default glm-5.3 input budget is 96,000 tokens, and get_batches / chunk_file_by_lines (llm_analyzer_base.py:1064-1110, 797-835) have no batch cap.
      • The 1.2 MB PDF alone becomes 3 chunks per semantic analyzer. Each is about 100,000 estimated tokens, above the budget, because per-line estimates are floored. In this set, 19 GIF chunks and 2 PDF chunks exceed the budget.
      • Real tokenizer counts on text full of U+FFFD are likely higher, so these calls may fail and make scans incomplete. This was not exercised.
    • Consequence: with the default use_llm=True, skills with ordinary documents or images make many large provider calls of decoded noise and run much longer. README.md:953 already says analyzer-eligible contents go to the provider, but nothing says binary formats are now eligible. Docs row 122 says only "Lossy text projection is inspected", and README.md:961 still says compiled content cannot be analyzed.
    • Expected fix: fix this together with finding 1.
      • Submit only the bounded text-run projection under a character cap, or only when the projection contains instruction-like text. The full lossy view can stay for local static analysis.
      • Consider not submitting records marked local_only.
      • Document the provider policy for binary formats.
  5. [Non-blocking] src/skillspector/artifacts.py:419: the XMP exemption only works for a single packet that fits inside one raw scan window starting at character 0.

    • Code path:
      • pdf_xmp_format_spans returns no span unless its argument starts with %PDF- (419), contains the whole packet from header to <?xpacket end= (426-437), and has exactly one header (426).
      • It is called on per-window text (static_patterns_prompt_injection.py:369, static_patterns_memory_poisoning.py:544).
      • Artifacts over 256,000 characters are scanned in raw windows (static_runner.py:118-120, 2444-2453, 2590-2601). Window 1 ends at character 247,808, and window 2 starts at 231,424, so window 2 does not start with %PDF-.
      • _binary_text_findings receives the full content but discards it (static_runner.py:3071).
    • Real file (transcription): the iMovie stop_glyph.pdf is 469,680 characters, with the packet header at 136 and the metadata body running to 462,113.
      • Unreferenced beside a benign SKILL.md, it is CAUTION 36, with P2 HIGH on the BOM (line 8), 21 P9 MEDIUM on the padding (lines 6423-6442) and AE4. Main: SAFE 0.
      • Across 35 real large PDFs that contain an XMP packet, the helper fails in the owning window for 12 distinct files. On the whole file it succeeds for all of them.
    • Synthetic pair (541,364 characters, same standard packet): with the packet late, the scan is CAUTION 28, with P2 on the BOM and P9 on the padding. With the packet early, it is SAFE 0 with no findings. MP2 appears only for some padding shapes.
    • Multiple packets: the 1.2 MB PDF in finding 1 has two XMP packets, so the helper gives up. Both BOMs get P2 HIGH and the padding gets 42 P9. Incremental-update PDFs commonly carry more than one packet.
    • Consequence: the exemption that the PR documents and the reply describes does not hold for larger PDFs or for multi-packet PDFs.
    • Expected fix:
      • Compute the spans once on the whole artifact and map them into window offsets, for example in _binary_text_findings (which already receives the full content) or alongside the active security view.
      • Handle every standard packet.
      • Add tests for a packet after the first window, a packet crossing the window boundary, and two packets.
  6. [Non-blocking] docs/scan-completeness.md:122: the docs and the completeness report overstate readable-binary coverage.

    • Row 122 says an incidental GIF/PDF is "complete / true (absent other limits)" with exits 0/0/0. That holds only for the synthetic ASCII fixture. Measured (transcription) with a real assets/icon.gif (125 B, LZW) and assets/manual.pdf (1,479 B, Flate):

      Main Head
      Disposition out_of_scope analyzed
      scope_exclusions both files, binary_content []
      Ledger exceptions none none
      Findings none AE4 on both files
      Risk SAFE, score 0 SAFE, score 12
      CLI exits 0/0/0 0/0/1

      In the local sample, 47 of 49 real GIF/PDF files got AE4.

    • The files disappear from scope_exclusions: artifacts.py:397-398 gives ANALYZED, and static_runner.py:3296-3300 skips the binary_content event. Yet Flate and LZW data is never decoded; src has no such decoder. Row 121 ("Unreferenced incidental image ... binary_content in scope_exclusions") is now also wrong for GIF images.

    • The earlier review's finding 4 offered "unreferenced files fully analyzed as text become ANALYZED" as one option. That condition no longer holds once the lossy gate admits compressed files, so this request does not reverse that advice.

    • Consequence: completeness reports and docs claim more coverage than exists for compressed content, and they mislead readers about the exit codes they will see.

    • Expected fix:

      • After the finding 1 fix, update rows 121-122 to the measured behaviour.
      • Keep readable binary formats visible in scope_exclusions under a distinct reason, for example binary_text_projection ("text runs inspected; container streams not decoded").
      • Update the PR body's validation section to reflect the CI result.

PIC tradeoffs:

  • Prefix-only gate: this is the simplest way to make sure no byte pattern can hide instructions behind these four prefixes, and it closed the first-round bypasses completely. The cost is that format data is treated as prose by rules tuned for text. A printable-run projection keeps most of the evasion fix. Its knob is the minimum run length: too long, and an author can break instructions up with control bytes; too short, and binary noise comes back. Tests at both ends would make that choice explicit.
  • Rule set for readable-binary records: restricting them to instruction rules removes most of the noise, but it also drops the Unicode-evasion checks (AE4, bidi P2) on the readable text. Running those checks only inside printable runs keeps them without scoring decoded compressed bytes.
  • MZ in the prefix tuple: it gives readable executables accurate wording (earlier finding 3), but it changes the evidence for every PE (finding 2). The SC9 floor already blocks every MZ file, so scanning PE text adds detail to the report rather than changing the verdict.
  • LLM submission: sending the projection to the LLM gives semantic coverage of hidden instructions, but it costs provider tokens and sends binary-derived content off the machine. A bounded projection, or submission only on instruction-like text, is the middle ground.
  • Timing of the disposition: setting the readable-binary disposition and ledger event before format-aware decoding keeps the logic in one block. Finding 3 shows it can go stale when #579's promotion runs later. Deciding after the decode loops avoids that class of mismatch.

Verification and gaps:

  • Head: live head eaebfabb70e9177b54f4177de8f3a58a50fada45, confirmed with gh pr view just before writing. Its merge-base is main 5edc347, and the provided diff matches git diff merge-base..head. The replies cite 4ba349f6 for the fixes; later commits are merges of main.
  • CI: test-unit failed (run 37934042362: 1 failed, 12,900 passed, 1:52:42). changes, lint, OpenCode TypeScript Tests, DCO Check and docker-smoke passed, and the merge state is BLOCKED. Main's run at 5edc347 took 1:00:16. I did not attribute that difference to this PR, though it is consistent with finding 1.
  • Conflicts: none with main; git merge-tree --write-tree against origin/main exits 0.
    • Pairwise, #741, #793, #795, #796 and #797 merge cleanly with #794.
    • #790 and #798 also conflict with main alone, in files this PR does not touch.
    • Semantic overlap:
      • #741 changes which MP2 matcher runs in the loop where this PR adds the XMP skip. The two are compatible; #741's effect on the finding 1 slowdown was not measured.
      • #793's reserved-path FAILED guards do not touch readable-binary paths.
      • #795 only projects cached content that starts with {.
  • Method: every measurement is a transcription: a copy of main 5edc347's src with the PR's src diff applied, checked byte-identical to the head's changed source files and run with main's venv (python -I). Nothing was imported from the PR checkout. Graph runs used use_llm=False. The runs used Python 3.13 on macOS, while CI uses 3.12. Timings ran with two probes in parallel.
  • Also verified:
    • run_static_patterns' switch from not binary_paths to not raw_inventory affects only the non-graph helper, whose callers are all tests.
    • The new tests exercise real code paths, and the positive ones fail on main.
    • Two tests are weaker than they look: test_opaque_reference_reporting.py:214-215 and test_security_remediation.py:1100-1104 derive their expected value from the result itself, so they pass under either policy.
  • I did not run the PR's tests or code, per policy.
  • Gaps:
    • No live LLM provider was exercised. Provider exposure and chunk counts come from llm_file_cache, estimate_tokens and a trace of the batching code.
    • The real-file sample is local: 40 PDFs (mostly app and system icon PDFs), 9 of a planned 40 GIFs (stopped early because multi-MB GIFs take 200-300 s each), and 10 PE launchers.
    • I did not check how agent runtimes display NUL or invalid bytes.
    • I did not measure the interaction with #741's regex budgeting.

Decision: Changes Requested (reviewed head eaebfabb70e9177b54f4177de8f3a58a50fada45)

# These printable prefixes are also valid instruction text. Keep the
# claimed binary format's coverage limits, but inspect the readable bytes.
# Lossy decoding must not let one invalid byte or a binary comment hide text.
readable_binary = data.startswith((b"MZ", b"GIF87a", b"GIF89a", b"%PDF-"))

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.

This gate is now the prefix alone, so every real GIF, PDF and PE file gets its whole lossy decode_text view scanned by every pattern module (static_runner.py:3113-3149) and by _text_signals (artifact_integrity.py:1057-1088). The only binary-specific filters are NUL-only P2, AE3 and a single XMP packet.

Measured on a transcription of the head (main 5edc347 plus this PR's src diff, byte-identical to the head), with an unreferenced real file beside a benign SKILL.md and use_llm=False:

  • PDFs: AE4 on 38 of 40 real PDFs that are all SAFE and complete on main. 10 of them become CAUTION or incomplete.
  • GIFs: AE4 on 9 of 9 real GIFs. A 1.1 MB GIF is incomplete after 212 s, and a 2.1 MB GIF hits runtime_limit after 301 s.
  • 1.2 MB PDF: DO_NOT_INSTALL, score 89, after 227 s (main: SAFE in 0.22 s). The findings come from Flate noise (P2 on U+202E, TM1), two XMP packets (P2 on the BOMs, 42 P9) and /Widths arrays (137 MP2).

Please keep the evasion fix but scan a bounded printable-run projection instead of the whole decode:

  • For readable_binary records, run only instruction rules (P1, PE3, E1 and similar), and make AE4 ignore control bytes outside printable runs.
  • Add negative graph fixtures (a Flate PDF with two XMP packets, a multi-frame GIF, a PE stub) plus a bounded-time case.

See finding 1.

"outer_hidden": component.get("outer_hidden", hidden),
"concealed_executable": True,
"excluded_from_analysis": True,
"excluded_from_analysis": not readable_executable,

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.

CI test-unit fails on this head (run 37934042362, "1 failed, 12900 passed").

  • Failing test: main's tests/nodes/analyzers/test_sc8_shipped_bytecode.py::test_ordinary_binary_executable_cannot_remain_safe fails at line 166, excluded_from_analysis is True.
  • Cause: its fixture b"MZ\x00\x00binary payload" is now readable_binary (artifacts.py:374). This line therefore sets excluded_from_analysis False, and lines 1029-1035 pick binary_executable_text instead of binary_content.
  • Conflicting tests: the PR's own test at test_security_remediation.py:72-79 asserts the opposite contract for MZ + \x00binary.
  • Effect on the verdict: none. The SC9 floor of 51 still applies (report.py:532-536, 634-646). But every byte-only PE is now labelled "Readable text was inspected".

Please pick one contract:

  • either update test_sc8_shipped_bytecode.py:166-167 to the partially-analyzed flags and reason, and say so in the PR body;
  • or make MZ readable only when real text follows the header, and keep that test unchanged.

Either way, run the full test-unit suite before pushing. See finding 2.

selected_primary = state.get("primary_file_path")
for artifact in artifact_inventory:
path = artifact["path"]
if (

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.

This block appends a PARTIAL opaque_content event before the Python-source loop. At line 3442, promote_artifact_to_decoded_text turns the same reason-less BINARY/PARTIAL row into TEXT/ANALYZED, but the event is never withdrawn, and finalize_inspection_ledger.py:590-605 raises AE1 from it.

Transcription: SKILL.md links scripts/scale.py, which contains MZ_SCALE = 2\nprint(MZ_SCALE)\n.

  • Main: SAFE 0, complete.
  • Head: CAUTION 25, is_complete False, AE1 HIGH. The partial and uninspected counts are both 0.
  • Same result: a helper that starts with GIF89a_HEADER = ....

Please emit the event and the PARTIAL disposition only for artifacts that are still BINARY after the Python decode loop. Either move this block after build_context.py:3394-3460, or skip paths that decoded successfully. Add a graph test that expects complete and SAFE with no AE1. See finding 3.

provider_submission_allowed
and not _is_hidden_path(path)
and artifact["content_kind"] == "text"
and (artifact["content_kind"] == "text" or artifact.get("readable_binary"))

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.

With the prefix-only gate, this condition puts the full lossy decode of every non-hidden GIF, PDF and PE file (up to 16 MiB each) into llm_file_cache. Nothing removes it later: _mark_unanalyzed_executables sets local_only but keeps the entry.

Main's estimate_tokens, with only the gate transcribed:

  • Totals: 804,711 tokens for 41 real PDFs, 2,311,636 for 39 GIFs and 133,325 for 10 PE launchers. On main, none of these files is in llm_components.
  • One file: the 1.2 MB PDF alone is 3 chunks of about 100,000 estimated tokens per semantic analyzer, above the 96,000-token glm-5.3 budget.

No live provider was exercised. Docs row 122 and README.md:961 do not say that binary formats are now sent to the provider.

Please submit only the bounded text-run projection from the finding 1 fix, under a character cap (or only when it contains instruction-like text), and document the provider policy for binary formats. See finding 4.

def pdf_xmp_format_spans(content: str) -> tuple[int, int, int]:
"""Recognize one standard XMP packet's BOM and trailing ASCII padding only."""
header = '<?xpacket begin="\ufeff" id="W5M0MpCehiHzreSzNTczkc9d"?>'
if not content.startswith("%PDF-"):

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.

This helper only recognizes the packet when it receives text that starts with %PDF-, holds the whole packet, and has exactly one XMP header (line 426). The callers pass per-window text, and static_runner windows artifacts over 256,000 characters (window 2 starts at 231,424).

Transcription results:

  • Real stop_glyph.pdf (469,680 characters, padding near 462K): CAUTION 36, with P2 HIGH on the BOM and 21 P9 on the padding (main: SAFE 0).
  • Synthetic PDF with a late packet: CAUTION 28. The same file with the packet early is SAFE.
  • The 1.2 MB two-packet PDF from finding 1: P2 on both BOMs and 42 P9.

Please compute the spans once on the whole artifact, for example in _binary_text_findings, which already receives the full content, and map them into window offsets. Handle every standard packet, and add tests for a late packet, a boundary-crossing packet and two packets. See finding 5.

Comment thread docs/scan-completeness.md
| Unsupported in-profile `SKILL.md` below directories or normal/renamed ZIPs | Same required-content event at real/virtual member path | failed / false | 2 / 2 / 2 | false |
| Supported benign ZIP, including renamed or empty ZIP | Existing bounded archive inspection; incidental assets remain exclusions | complete / true | 0 / 0 / 0 | true |
| Unreferenced incidental image beside benign instructions | `binary_content` in `scope_exclusions` | complete / true | 0 / 0 / 0 | true |
| Incidental GIF/PDF with printable magic | Lossy text projection is inspected; ordinary binary NULs and PDF metadata padding are not evasion findings | complete / true (absent other limits) | 0 / 0 / 0 | true if no findings |

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.

This row holds for the synthetic ASCII fixture but not for real files. Transcription with a real icon.gif (125 B, LZW) and manual.pdf (1,479 B, Flate):

  • Main: both out_of_scope under binary_content in scope_exclusions, SAFE 0, exits 0/0/0.
  • Head: both analyzed, scope_exclusions empty, AE4 on both, SAFE 12, exits 0/0/1.

In the local sample, 47 of 49 real GIF/PDF files got AE4. Row 121 is also no longer accurate for GIF images, and the compressed data is never decoded.

After the finding 1 fix, please:

  • update rows 121-122 to the measured behaviour;
  • keep readable binary formats in scope_exclusions under a distinct reason, for example binary_text_projection;
  • note in the PR body's validation section that CI test-unit currently fails.

See finding 6.

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