Repository navigation
chore(sync): merge upstream 2.5.3 into the fork - #104
Conversation
* fix: exclude valid OMS signatures from content analysis OMS bundles necessarily contain long base64-encoded payload, signature, and certificate fields. Generic obfuscated-code checks can misclassify these fields as hidden executable content. Recognize root-level skill.oms.sig files with the minimal OMS v0.3 DSSE/in-toto structure and inventory them as oms_signature components without sending their contents through static or LLM analyzers. Preserve normal scanning for malformed, nested, oversized, or unrecognized signature files. Add a pinned real-world fixture, regression coverage, completeness accounting, and documentation. Recognition is structural only and does not verify signatures, certificates, transparency logs, or signer identity. Signed-off-by: major-security <73253177+major-security@users.noreply.github.com> * fix: accept versioned OMS signature predicates Signed-off-by: Daniel Major <dmajor@nvidia.com> * test: cover OMS exclusions across report formats Signed-off-by: Daniel Major <dmajor@nvidia.com> * fix: preserve semantic ledger evidence for missing files Signed-off-by: Daniel Major <dmajor@nvidia.com> --------- Signed-off-by: major-security <73253177+major-security@users.noreply.github.com> Signed-off-by: Daniel Major <dmajor@nvidia.com>
…al (NVIDIA#321) * fix(static): markdown table and quote syntax is not an execution signal `_is_documentation_context` refuses to treat a line as prose when `_EXECUTION_SIGNAL` matches it, and that pattern includes `[|>]`. In markdown those two characters are structure, not shell metacharacters: `|` delimits table cells and `>` starts a block quote. A governed rule that lands on a table row or a quoted paragraph is therefore never classified as prose, whatever it says. The delimiters are now removed before the execution test, and only the delimiters. Inside a table cell a literal pipe has to be written `\|` (CommonMark), so a documented `cmd \| tee log` keeps its pipe and still counts as an execution signal — which is what makes this safe rather than a widening. Scope, stated plainly: this is a correctness fix, not a precision win. On a corpus of 65 real skill/plugin units (4415 findings, all triaged by hand) only 8 findings of the governed rules were blocked by markdown structure alone, across 4 files. After the change 1 remains, and it has a genuine execution signal on the line. The reason to fix it is that the classification is simply wrong, not that it is frequent. Nothing is added to `_SEMANTIC_STRING_DOC_PRONE_RULES`: the set stays {RA1, TM1, AR2}, and the reasoning that excludes PE3 is untouched. Tests: table row and block quote are classified as prose; a literal escaped pipe in a cell and a real redirection in a quote still are not; plus a unit test that `_strip_markdown_structure` touches delimiters and nothing else. Full suite: 1565 passed, 14 skipped, 6 xfailed. Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com> * fix(static): make the docstring raw so 3.12 stops warning The docstring quotes CommonMark's escaped bar, and \| is not a valid escape sequence: Python 3.12 emits SyntaxWarning on import, which makes a supported-Python test run noisy. The prefix is the whole fix. Guarded package-wide rather than per-file: the new test compiles every shipped module and fails on any SyntaxWarning, so the next one is caught where it is written instead of in a reviewer's terminal. Tests: tests/unit 727 passed, 12 skipped. Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com> --------- Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com> Co-authored-by: Mark2Mac <Mark2Mac@users.noreply.github.com>
…atch (NVIDIA#317) On Windows, run_agent_cli's TemporaryDirectory raised WinError 32 at __exit__ whenever any live process still held the per-invocation temp cwd — and the agent CLI's process tree routinely does: the binary is a .cmd shim whose node children can outlive the direct child, and a killed-but-unreaped child on the timeout path holds its cwd too. A directory that is any live process's working directory (or contains any open handle) cannot be removed on Windows, so cleanup raised *after* the model had already answered, and llm_analyzer_base recorded the batch as llm_batch_failed. Under batch concurrency the teardown window widens, which is why multi-file scans failed while single-file scans passed (NVIDIA#315). Each invocation already has a unique mkdtemp directory, so this was never a cross-worker path collision; it is delete-at-exit racing the process tree's teardown. Fix: mkdtemp + explicit best-effort cleanup. _cleanup_temp_dir retries briefly (10 x 0.2s) to reclaim the directory once the holder exits, then leaks it with a warning instead of raising. A cleanup failure never outranks a successful response. Verified on Windows 10 with a mechanism-faithful fake CLI (a .cmd shim that detaches a grandchild holding the temp cwd): 8 concurrent batches fail 8/8 with the exact WinError 32 signature from NVIDIA#315 on main, and pass 8/8 with this change. Controlled experiments confirm both hold modes (another process's cwd; an open file handle inside the dir) block rmtree with WinError 32. Tests: 5 new cases including a Windows-only real-handle hold and a regression test asserting run_agent_cli returns the response when rmtree keeps failing. tests/unit/test_agent_cli.py 87/87 on Windows. Closes NVIDIA#315 Signed-off-by: ppcvote <risky9763@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…fy (NVIDIA#318) (NVIDIA#319) * fix(supply-chain): treat only == / <= as version pins in requirements.txt `_extract_packages_from_requirements` kept the captured version for any operator, so a floor like `pillow>=10.0.0` was recorded as the exact release `10.0.0` and the OSV/CVE lookup attributed that version's vulnerabilities to an unpinned dependency — a false CRITICAL on a requirements file that pins nothing. Only `==` and `<=` bound the dependency to a concrete, CVE-checkable release; `>=`, `>`, `!=`, `~=` are floors/ranges. This mirrors the guard already present in `_extract_packages_from_setup_py` (`m.group(2) in ("==", "<=")`), so the two extractors now agree. Added a regression test asserting `>=`, `~=`, `!=` and bare names yield version=None while `==` / `<=` keep the version. Fixes NVIDIA#294 Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com> * fix(supply-chain): only exact pins resolve to a version, ranges do not Addresses the review on NVIDIA#302: the previous guard still admitted non-exact constraints. `<=8.1.0` matches every earlier release and `==1.*` is a wildcard, so both were handed to the vulnerability lookup as a version the dependency may never install. A vulnerability lookup answers "is THIS release affected?", which is only meaningful when the manifest admits exactly one release. That predicate is now explicit and shared instead of being re-derived at each call site: - `_pinned_version` (PEP 440): only `==` with a fully concrete version. Floors, caps, exclusions, compatible releases and wildcard equality yield None. - `_pinned_npm_version` (semver): only a bare `x.y.z`. npm defaults to caret ranges, so `"^1.8.3"` was being stripped into the concrete release `1.8.3`. Applied to all three extractors — requirements.txt, pyproject.toml and package.json — because the objection in the review holds verbatim for the two that were not touched by the original patch. Note for the maintainer: dropping these specifiers moves more dependencies to version=None, which NVIDIA#318 shows is currently reported as CRITICAL carrying the package's worst-ever advisory. The two fixes are complementary; happy to send the severity side as a separate PR. Regressions cover both cases named in the review (`<=` and `==1.*`) plus the npm caret/tilde/wildcard/range forms. Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com> * fix(supply-chain): SC4 must not claim a vulnerability it did not verify When a manifest admits a range, no version is resolved and OSV is queried by name alone. The advisories that come back are the package's history, not a match against the release that will be installed: the worst of them may predate every version the range admits. Using that as the finding's severity turns 'setuptools>=61' into a CRITICAL 'Known Vulnerable Dependency'. Scanning 65 skill/plugin units, every SC4 finding in the corpus came from this or from a range being read as a pin (NVIDIA#294/NVIDIA#302). Not one manifest pinned a vulnerable release. The lack of pinning is already reported by SC1, so what is left for SC4 to say is 'could not verify', and it must not outrank a real version match: severity capped at LOW, confidence 0.4, and wording that states the limit instead of implying a match. Version-matched findings are unchanged. Closes NVIDIA#318 Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com> * fix(supply-chain): preserve full PEP 440 pins Signed-off-by: keshavp <32313895+keshprad@users.noreply.github.com> * fix(supply-chain): parse pip requirement options Signed-off-by: keshavp <32313895+keshprad@users.noreply.github.com> * style: format supply chain tests Signed-off-by: keshavp <32313895+keshprad@users.noreply.github.com> --------- Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com> Signed-off-by: keshavp <32313895+keshprad@users.noreply.github.com> Co-authored-by: Mark2Mac <Mark2Mac@users.noreply.github.com> Co-authored-by: keshavp <32313895+keshprad@users.noreply.github.com>
`_is_layout_only_span()` is what keeps MP2 from reporting alignment as context
stuffing, and it has no test. The only layout case covered today is `"=" * 80`,
which never reaches the helper — the older single-character guard skips it first.
Measured on a corpus of 4406 files from 65 skill/plugin units, comparing the
analyzer before the helper landed with `main` today:
MP2 findings 279 -> 4
The four survivors come from MP2's prose patterns ("exceed context window"), not
from the repetition pattern. So the helper is carrying 275 of 279 findings, with
nothing pinning its behaviour.
The three cases added here are each reported when the helper is stubbed out, which
is what makes them regressions rather than restatements of the guard above them:
a dash rule, a run of padded columns, and a box edge with padding. The fourth test
locks the other side, `_MAX_LAYOUT_ONLY_SPAN`: past that width layout stops being a
plausible explanation and the run is reported again.
No source change.
Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com>
Co-authored-by: Mark2Mac <Mark2Mac@users.noreply.github.com>
The README has no mention of the Verified Skills pipeline, the skills catalog, or the hosted documentation, so a reader arriving at this repo has no path to any of them. - Overview: one line placing SkillSpector in the pipeline, linking the docs home and the skills catalog. - Documentation: lead with the hosted scanning guide, ahead of the repo-local developer docs, since that is what most readers want. Signed-off-by: Moshe Abramovitch <moshea@nvidia.com>
Signed-off-by: Keshav Prasad <32313895+keshprad@users.noreply.github.com>
* fix(output-handling): ignore RegExp exec parsing Signed-off-by: Christopher Kevin <christopherk@nvidia.com> * fix(output-handling): guard RegExp exec mutations Signed-off-by: Christopher Kevin <christopherk@nvidia.com> * fix(output-handling): avoid unverified mutation inference Signed-off-by: Christopher Kevin <christopherk@nvidia.com> * test(output-handling): cover regexp class syntax Signed-off-by: Christopher Kevin <christopherk@nvidia.com> * fix(output-handling): fail closed across line comments Signed-off-by: Christopher Kevin <christopherk@nvidia.com> * fix(output-handling): handle legacy HTML comments Signed-off-by: Christopher Kevin <christopherk@nvidia.com> --------- Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
…on (NVIDIA#332) * fix(analyzer): detect full environment reads via AST Signed-off-by: Mohit Gupta <mohgupta@nvidia.com> * refactor(analyzers): share Python AST parsing Signed-off-by: Mohit Gupta <mohgupta@nvidia.com> * fix(analyzer): detect keyword environment secret reads Signed-off-by: keshavp <32313895+keshprad@users.noreply.github.com> --------- Signed-off-by: Mohit Gupta <mohgupta@nvidia.com> Signed-off-by: keshavp <32313895+keshprad@users.noreply.github.com> Co-authored-by: keshavp <32313895+keshprad@users.noreply.github.com>
Signed-off-by: keshprad <32313895+keshprad@users.noreply.github.com>
Closes the fork's first upstream sync: 11 commits from NVIDIA/SkillSpector since merge-base a818f50. Resolutions: - src/skillspector/nodes/build_context.py — both intents kept. Upstream's shared Python AST prewarm (e6ebe70) and MAX_FILE_BYTES import sit beside the fork's detect_framework and ManifestStatus; _parse_manifest keeps the fork's tuple return. - docs/OWASP-AST10-COVERAGE.md — upstream deleted the file (c54967a, a revert of NVIDIA#288). The fork built its L4J-*/DA-*/MCP-* crosswalk on it, so the file is kept and becomes fork-owned rather than an inherited doc the fork edited. - README.md — below the marker, upstream's text taken as-is, including its removal of its own link to the deleted doc. The fork's contract table above the marker still references the doc, which is now correct because the doc is the fork's. - pyproject.toml — upstream's version 2.5.3 taken; the fork's tree-sitter dependencies kept. Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Three snapshots move, all from upstream e6ebe70 "fix(analyzers): share Python AST parsing for environment-read detection". Env-variable reads are now located by the shared Python AST instead of a regex, so matched_text is the exact call node rather than a truncated regex span, and end_line is populated where the regex could not produce one, which SARIF surfaces as endLine. No other snapshot moved, and no delta lacks an upstream commit behind it. Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
…ions Two fork guards failed on the sync. Neither was upstream breaking a test; both were the fork asserting something upstream's new code invalidated. MAX_FILE_BYTES is now a homonym. The fork retired that spelling when the per-file analysis cap changed meaning to characters; upstream introduced an unrelated read cap under the same name after the merge-base, compared against file_path.stat().st_size, which genuinely is bytes on disk. The guard exempts the two upstream modules by call site rather than dropping, and TestReadCap is the control: those modules must not also name the analysis cap, and the read cap must still be a size on disk. tests/fixtures/oms holds a signature sample, not a Skill, so the corpus guard counted it as a scan target that owes a snapshot. NON_TARGET_DIRS excludes it, with a control asserting the reason holds -- no manifest and no child directories, which every one of the 35 corpus members has. Verified non-vacuous: the control rejects a skill fixture, a family parent, a manifest-bearing fixture and a framework fixture. Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Upstream's merge adds packaging>=24.0 as a runtime dependency, so the distribution pulls in a license THIRD_PARTY_NOTICES.md did not know about. Apache-2.0 OR BSD-2-Clause; taken under Apache-2.0, whose text the file already reproduces, so no fourth license text is needed. uv.lock is regenerated against the merged pyproject.toml, restoring the fork's tree-sitter entries that upstream's own lock does not carry. Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Upstream deleted docs/OWASP-AST10-COVERAGE.md in c54967a while the fork's crosswalk of its own L4J-*, DA-* and MCP-* Rules lived in it. The file is kept, so it moves out of the skill's "inherited docs the fork edited" bullet -- upstream has no version of it left to win -- and into the fork-owned list, with the reason recorded so the next sync does not apply the wrong resolution rule. Two Applied Learning bullets for what the sync cost to work out. Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Upstream's revert is not an abandonment. NVIDIA#288 and NVIDIA#338 both say "We will bring updated wording next week with product team approval", so c54967a is a pending rewrite of docs/OWASP-AST10-COVERAGE.md rather than a decision to drop it. The previous commit filed the page under fork-owned and said upstream had no version left to win. That is wrong the moment upstream republishes, which their own comments date to within days -- and it is wrong in the expensive direction, because a sync following that bullet would keep the fork's page and discard upstream's reworded one. Keeping the file was still right for this sync: the fork's crosswalk of its L4J-*, DA-* and MCP-* Rules lived only there. But it is held, not owned. The next sync carrying an upstream version meets an add/add conflict and applies the inherited-doc rule -- upstream's text wins, the fork's rows re-applied on top. Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Corrections to this PR's body, after the mergeThe two review passes finished after the merge landed. Three findings are 1. The ledger's evidence column misattributed two commits. The no-delta table
The conclusion is unchanged: all four fixes survived the resolution, and the whole 2. 3.
The substance of Two code findings from the Standards pass, not fixed hereBoth are on
|
PR #104 landed the 2.5.3 sync as a squash, so main carried the merged tree with no merge lineage. 9426796 had one parent, the merge-base with upstream/main stayed at a818f50, and `git rev-list --count` still reported 11 commits behind despite every one of them being present in the tree. This is a `-s ours` merge, so the tree does not change -- main's tree already is the merge result, verified identical to 9426796's. It records the second parent, which is the only thing the squash dropped. The merge-base now advances to 0562b96 and the count is 0, so the next /upstream-sync measures from upstream's tip instead of re-merging eleven commits into a tree that already contains them, and the drift measurement #105 asks for can reach zero. Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Closes #100 — the fork's first upstream sync. Eleven commits from
NVIDIA/SkillSpector, merge-basea818f50, version2.5.1→2.5.3.The conflict surface was 11 files; only 2 actually conflicted
Everything else merged itself.
#100measured the surface and it had not moved,despite five fork commits landing after the issue was written (
95ahead is now100, includingc990bbc's 66-file header sweep).src/skillspector/nodes/build_context.pyMAX_FILE_BYTESimport sit beside the fork'sdetect_frameworkandManifestStatus;_parse_manifestkeeps the fork's tuple return.docs/OWASP-AST10-COVERAGE.mdREADME.mdpyproject.tomlversion = "2.5.3"; the fork'stree-sitterdependencies kept.docs/DEVELOPMENT.md,inspection_ledger.py,behavioral_ast.py,behavioral_taint_tracking.py,report.py,state.py,input_handler.pyUpstream deleted a doc the fork had built on
c54967areverts upstream's#288and deletesdocs/OWASP-AST10-COVERAGE.mdentirely — the whole 90-line file, plus its README link. The fork had added 23
lines to it, mapping
L4J-SHELL,L4J-WORKDIR,L4J-MCP-FILTER,L4J-TOOL-DESC,L4J-UNRESOLVED,DA-SHADOW,DA-SKILL-WRITABLE,DA-UNRESOLVED,DA-SUBAGENT-SKILLSand the fiveMCP-*posture checks ontothe AST10 categories, including the reasoned absences.
README.md:376— thefork's own contract, above the marker — names that file as where every detection
rule must be recorded.
The skill's step 4 stops for exactly this. Decision: keep the file.
And then the reason for the revert changed what that decision means. Upstream's
commit message says only "This reverts commit 5b07626". The comments on
NVIDIA/SkillSpector#288and#338say why:The deletion is a pending rewrite, not an abandonment — and
c54967alanded twodays before this PR. So the file is held by the fork, not owned by it: keeping it
was right, because the crosswalk lived only there, but the next sync carrying
upstream's republished page meets an add/add conflict whose correct resolution is the
inherited-doc rule — upstream's reworded text wins, the fork's
L4J-*,DA-*and
MCP-*rows re-applied on top..claude/skills/upstream-sync/SKILL.mdbriefly recorded the opposite (fork-owned,"upstream has no version left to win") and is corrected in
8516ecf. That mattered:a sync following the uncorrected bullet would have kept the fork's page and discarded
upstream's.
Snapshot ledger — 3 deltas, all one commit
Snapshots moved. On a sync that is correct, not a regression.
malicious_skill.jsonmatched_textfor key, val in os.environ.items()→os.environ.items();end_linenull→15; SARIF gainsendLinee6ebe70mcp_mismatched_skill.jsonscripts/greet.py:10e6ebe70mcp_underdeclared_skill.jsonos.environ.get("API_KEY→os.environ.get("API_KEY");end_linenull→10e6ebe70e6ebe70moves environment-read detection from a regex to the shared Python AST,so the match is the exact call node instead of a truncated regex span, and an end
position exists where the regex could not produce one. No delta lacks a commit
behind it.
The four behavior-affecting commits that produced no delta, and the evidence
each survived the resolution rather than being lost — upstream's own tests, which
the merge brought in and which pass:
bfdcd5emarkdown table/quote is not an execution signaltests/unit/test_patterns.py,tests/unit/test_patterns_new.pycf2e87fRegExp.execfalse positivestests/unit/test_patterns.py7e9c19dSC4 must not claim an unverified vulnerabilityee9612cexclude valid OMS signaturesskill.oms.sigtests/nodes/test_build_context.py,tests/integration/test_graph.pyTwo fork guards failed, and neither was upstream breaking a test
Both were the fork asserting something upstream's new code invalidated.
MAX_FILE_BYTESis now a homonym. The fork retired that spelling when theper-file analysis cap changed meaning to characters (
MAX_FILE_CHARS, issue#42). Upstream introduced an unrelated read cap under the same name afterthe merge-base —
constants.py, compared againstfile_path.stat().st_sizeinbuild_context.py. That genuinely is bytes on disk, so the name is right there.The guard exempts the two upstream modules by call site rather than being
dropped, per this repo's recorded practice for a homonym spelling.
TestReadCapis the control: those modules must not also name the analysis cap, and the read
cap must still be compared against a size on disk. The failure message no longer
claims the spelling has no legitimate use left, because it now does.
tests/fixtures/omsis not a Skill. Upstream's OMS recognizer ships asignature sample, which the corpus guard counted as a scan target owing a
snapshot.
NON_TARGET_DIRSexcludes it, with a control asserting the reasonholds — no manifest and no child directories, which every one of the 35 corpus
members has. Verified non-vacuous: the control rejects a skill fixture, a family
parent, a manifest-bearing fixture and a framework fixture. The corpus count
stays at 35.
Licensing
packaging>=24.0arrives as a new runtime dependency with noTHIRD_PARTY_NOTICES.mdentry — nothing in CI reads that file, so the mergewould have shipped it silently. Added: Apache-2.0 OR BSD-2-Clause, taken under
Apache-2.0, whose text the file already reproduces.
/license-audithas not been run against this branch yet. A merge is exactlywhen license drift enters, and this one is the worst case for it: upstream files
arrived, files the fork had modified gained upstream hunks, a dependency was
added, and
docs/OWASP-AST10-COVERAGE.mdnow redistributes upstream-authoredcontent in a file upstream itself deleted. That audit is user-invoked, so it is
outstanding.
Validations
make test-unit— 2831 passed, 13 skipped, 38 deselected, 4 xfailedmake lint— cleanmake format-check— 208 files already formattedmake update-snapshotsre-run: no further movementCorrections to #100
f96de1d(release 2.5.2)and
082048b(test(mp2)) are the two missing, both harmless.c54967aas possibly a design question. It is one, and thedeletion is of the whole file rather than of a behavior.
Follow-ups filed
#105— nothing triggers the next sync, so the gap will reach eleven again.#100asked for a cadence and this PR does not deliver one.
#106— the fork holdsdocs/OWASP-AST10-COVERAGE.mdand upstream is coming backfor it. Records the add/add conflict the next sync meets, and the three options,
which cannot be chosen until upstream's replacement exists.
Outstanding
/license-auditagainst this branch. It is user-invoked, and this merge is the worstcase for license drift.
A human merges this.