fix(as3): preserve skill identity across input materialization - #580
Conversation
Temp git/zip extracts land in directories like `repo` while SKILL.md keeps the real skill name. Requiring path/manifest agreement dropped all identities and false-positived literal self-references as AS3. Treat scan-root basename and manifest name as independent identities so either self-path is suppressed while peer-skill paths still fire. Refs NVIDIA#500 Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com>
Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com>
Brings the fork up to upstream main c13f70e; the version is still 2.11.2. The fork history was squashed into one commit on 2026-09-13, so it shares no merge-base with upstream and git merge refuses. The range diff was applied with git apply -3 instead. The fork content equals 69dcdfb plus the registered divergences, so conflicts landed only on those seven files; the other 116 applied cleanly. FORK.md now documents this procedure. Divergences, resolved by each row's rule: - static_runner.py takes upstream NVIDIA#522 (SKILLSPECTOR_MAX_STATIC_ANALYSIS_SECONDS_PER_ARTIFACT, default 300s). The fork's SKILLSPECTOR_MAX_STATIC_SECONDS override and its seven tests are removed. Downstream gates must use the upstream name when their pin moves. - test_static_yara.py, test_build_context.py and test_input_handler.py take upstream (NVIDIA#501-NVIDIA#505, NVIDIA#518 fix the same Windows issues); 301 passed on Windows, rows deleted. - test_security_end_to_end.py: upstream's version still fails nine_case on Windows (YARA load and SC8 budgets stay hard-coded), so the relaxation helper is re-applied on top; row kept and rewritten. - .gitignore keeps the fork block; README.md stays Traditional Chinese and the upstream README goes to README.en.md. Two new Windows divergences from new upstream tests: - tests/unit/test_cli.py: a file name containing a backslash is split into two path parts on Windows; skipped by a capability probe added to tests/platform_support.py. - test_json_container_ownership.py: oversized payloads became test ids, which pytest copies into PYTEST_CURRENT_TEST, over Windows' 32,767 character environment limit; short ids added, content unchanged. Triage: 13 of PRs NVIDIA#528-NVIDIA#580 merged into upstream main and arrive here; 29 stay open (including NVIDIA#550, release 2.12.0). Upstream closed this fork's PR NVIDIA#486 on 2026-09-15; NVIDIA#490 builds on it and is open. Verified on Windows in fresh-process batches against this tree: tests/unit 1563 passed, 29 skipped; tests/nodes 3822 passed, 11 skipped, 4 xfailed (plus test_json_container_ownership 71 passed after the id fix); remaining tests 182 passed, 16 skipped; test_security_end_to_end.py 98 passed. ruff check and format clean, check_divergence OK (10 diverging, 10 registered), check_pin_bounds OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed exact head 86a740f3ca0f1e74e713ec144c5a67b740a7a06e.
Requesting changes because the new suppression trusts a contributor-controlled manifest name as an independent current-skill identity. A skill can therefore choose the name of a peer and hide an otherwise valid AS3 finding for skills/<peer>/SKILL.md. Preserve the temp-clone false-positive fix without allowing uncorroborated manifest data to suppress the security rule—for example, carry a source-derived repository/archive identity into analyzer state and require it to corroborate the manifest. Add an adversarial mismatched-name regression.
Temp git/zip roots still use ephemeral basenames like `repo`, but contributor-controlled manifest.name must not suppress AS3 alone. Carry a host/operator-selected repository/archive/path identity into analyzer state and only suppress self-paths that match that trusted identity or a non-ephemeral scan-root basename. Add an adversarial mismatched-name regression. Refs NVIDIA#500 Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed head a03641d6428537717e800920303127153b282f2a against merge base c13f70ebf14905912c616a58c9a8cb8112ef94a4.
Requesting changes for two reproduced peer-skill AS3 false negatives and one regression in local self-reference handling. The intended flat-ZIP case works, but conflicting archive identities and incorrect URL identities can suppress access to other skills, while basename heuristics reject legitimate local skill names. The three inline comments describe distinct issues in the current revision, beyond the earlier manifest-only alias concern.
Validation: 34 existing focused AS3/resolver tests passed. I also ran eight synthetic no-LLM CLI cases on both base and head (16 executions). Local directory and ZIP cases used real files/extraction; Git and HTTP delivery were stubbed while the resolver, analyzer, and report paths ran normally. The archive and URL cases retain the peer finding on base and lose it on this head; local repo, extracted, and skillspector_example directories acquire self-reference false positives on this head.
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
rng1995
changed the title
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
Reviewed the fixes in acfe4e3. All four review findings are addressed: manifest names cannot override the source identity; preserved archive roots take precedence over archive filenames; successful input resolution supplies the identity without guessing from ambiguous URL segments; and legitimate local directory names are retained.
Verified the eight reported reproduction scenarios, 26 new regression cases, 5,768 passing non-provider tests, and 14 deterministic integration tests. An independent review found no additional actionable issues. All review threads are resolved, and CI passed.
This supersedes my earlier changes-requested reviews. Codex assisted with implementation, validation, and review.
2 participants
Git clones, flat ZIP archives, and standalone local manifests are scanned from temporary directories whose basenames do not identify the selected skill. Preserve one source-derived identity during successful input resolution so literal references to the current skill remain exempt from AS3 after materialization.
The resolver uses the selected repository basename, preserved archive root, flat local archive stem, or original local
SKILL.mdparent. A preserved archive root takes precedence over its filename. Manifest names must agree with that identity when present; they cannot add another alias or override a mismatch. Actual resolver metadata distinguishes generated roots from valid local directories namedrepo,extracted, orskillspector_*.Ambiguous direct-download URLs and flat downloaded archives keep an unknown identity rather than guessing from repository, namespace, or Git-ref segments. Their AS3 findings remain detectable. See AS3 self-reference behavior for supported inputs and limitations.
Add 26 end-to-end and adversarial regression cases covering real directory/file/ZIP scans, Git and download flows with external delivery stubbed, conflicting archive names, nested GitLab namespaces, ambiguous URL refs, manifest mismatches, state reset, and finding-budget/ledger accounting. Peer references, obfuscated paths, explicit enumeration, AS1, and AS2 retain their detection behavior.
Validation:
Refs #500; extends the self-reference handling introduced in #506.
AI assistance: Codex assisted with investigation, implementation, regression tests, and independent review.