fix(references): distinguish quoted versions from file references by goodhee · Pull Request #525 · NVIDIA/SkillSpector

goodhee · 2026-09-11T01:51:50Z

Powered by Codex, on behalf of the repository contributor.

Quoted pinned versions such as version: "1.2.3" and metadata.version: "1.0.0" were counted as missing files, making otherwise complete scans incomplete. This change recognizes real quoted version scalars in YAML block frontmatter while preserving file references in quotes, code spans, Markdown links, comments, and multiline descriptions.

The metadata check reads bounded YAML parser events without constructing objects or expanding aliases. Headers without the literal version skip that extra pass because they cannot produce an exemption. Exemptions are tied to absolute source offsets so differing Unicode line-separator rules cannot hide body references. Malformed, oversized, or unsupported metadata retains normal reference accounting. Wildcard/latest version advisories and unrelated security findings remain active.

This update preserves goodhee's original commits and addresses the outstanding missing-numeric-file feedback: absent tool.1, docs/tool.1, and explicit numeric targets remain missing; known files resolve and ambiguous basenames remain partial. Fixes #524.

Focused follow-up validation on 7abe499e373b93d39d8d73a480ddb2f1f875e470:

59 version/reference tests passed, including two new checks that headers without version metadata skip event parsing while body references retain normal resolution. Ruff lint/format and diff checks passed.
The unchanged alias-amplification test passed twice with coverage at 2.96 and 2.97 seconds, matching paired baseline calls of 2.97 and 2.96 seconds. The previous head failed hosted CI at 3.083 seconds and took 4.001 and 3.990 seconds in the local covered comparisons. The baseline also crossed the three-second threshold once during initial triage, so timing sensitivity remains; the limit is unchanged. Fresh hosted CI is required.
This follow-up used focused runs only. Full-suite and dataset results below belong to the explicitly recorded prior head.

Prior validation on c70823a8ef67a8b72f7ed3fd4c1999cf93f48117:

57 focused version/reference tests passed, including multiline YAML, Unicode line separators, numeric filenames, ambiguity, strict CLI rejection, and preserved threat/advisory findings.
Full frozen 48-case regression/security/stress replay: 43/48, up from 41/48; exactly the two quoted-version cases improve, with no previously passing regressions. All eight security smoke and three full-size stress contracts pass. The separate 14-case AST robustness probe remains 7/14, unchanged.
Five unrelated baseline failures remain: reference punctuation, JSON capacity, duplicate padding findings, and two benign Markdown parser cases. No ordinary scan crashed or timed out.
Full selected Python unit suite: 7,917 passed, 14 skipped, 4 xfailed; one unchanged MCP FIFO test exceeded its five-second subprocess deadline and passed unchanged on isolated retry. The local coverage-enabled run was interrupted during an existing stress test; it is not recorded as passing.
65 JavaScript tests, local Docker build/smoke tests, Ruff lint/format, diff whitespace checks, and all commit signoffs passed.
Three independent review lenses and an evidence-bounded judge found no remaining code blocker. Exact-head hosted checks and fresh maintainer re-review remain required; this is not a maintainer approval.

rng1995

[SkillSpector Review]

Reviewed head 1f2739f1fbe004fa10a3a8a809609f25473a646c — REQUEST_CHANGES.

src/skillspector/references.py:137: the numeric-suffix heuristic rejects real paths, including paths with an explicit directory signal such as docs/tool.1. Numeric extensions are valid (manual pages and numbered artifacts are common), and this rejection happens before the resolver can check known_paths, so a shipped, explicitly referenced file becomes invisible to reference coverage. Apply the version heuristic only to genuinely ambiguous metadata tokens, or first preserve candidates that resolve to a known bundled path. Add a regression for a known docs/tool.1 reference.

Required checks pass, but the correctness regression and mergeStateStatus=BEHIND block merging.

A directory signal ("/") already disambiguates a path from a bare version string, so docs/tool.1 no longer gets rejected before the known_paths resolver ever sees it. Addresses review feedback on PR NVIDIA#525. Co-Authored-By: Claude Sonnet 5 <[email protected]>

A directory signal ("/") already disambiguates a path from a bare version string, so docs/tool.1 no longer gets rejected before the known_paths resolver ever sees it. Addresses review feedback on PR NVIDIA#525.

goodhee · 2026-09-15T01:31:41Z

Thanks for the catch — fixed.

_normalize_candidate now only applies the numeric-suffix ("version number") heuristic to a bare token with no directory component. A candidate carrying an explicit directory signal (e.g. docs/tool.1) is no longer rejected before the known_paths resolver gets to see it, so it can still resolve normally.

Added test_numeric_suffix_path_with_directory_signal_still_resolves covering the docs/tool.1 case you flagged, and confirmed the existing test_quoted_version_number_is_not_a_reference (bare "1.2", "v1.2", "1.2.0-beta.1") still passes.

Branch has been rebased onto latest main.

A directory signal ("/") already disambiguates a path from a bare version string, so docs/tool.1 no longer gets rejected before the known_paths resolver ever sees it. Addresses review feedback on PR NVIDIA#525. Signed-off-by: goodhee <[email protected]>

metadata.version values like "1.2" matched the quoted/code-span path pattern (name="1", ext="2"), so a version-only metadata field made an otherwise clean scan report is_complete=false and raised the recommendation from SAFE to CAUTION. Reject candidates whose extension is purely numeric ("1.2", "v1.2", "1.2.0-beta.1"), since real file extensions are never all digits. This is the same class of ambiguity as NVIDIA#451 for NVIDIA#450: a bare version number is indistinguishable from a root-level file name without another path signal, so this only narrows the existing quoted/code path pattern rather than trying to fully disambiguate intent. Fixes NVIDIA#524. Signed-off-by: goodhee <[email protected]>

A directory signal ("/") already disambiguates a path from a bare version string, so docs/tool.1 no longer gets rejected before the known_paths resolver ever sees it. Addresses review feedback on PR NVIDIA#525. Signed-off-by: goodhee <[email protected]>

rng1995

[SkillSpector Review]

Re-reviewed current head 68d3ea74f66837eb1d4f6628d47b2625cd6c463b against the prior requested change. The new slash guard preserves docs/tool.1, but the known-path case is still incomplete: _normalize_candidate() rejects a bare quoted tool.1 at lines 140–143 before resolve_bundle_references_with_metadata() can compare it with known_paths or its basename index. Numeric extensions are valid for root-level artifacts too (for example, man-page-style files), so a bundled file that is explicitly present can still be silently omitted from semantic analysis.

Please defer the version-like rejection until after exact/basename resolution, or otherwise preserve a bare numeric-extension token when it uniquely names a known bundled path. Add a regression where known_paths contains root-level tool.1 and "tool.1" resolves, while unrecognized quoted version metadata remains rejected.

Required checks are green, but this remaining correctness gap and mergeStateStatus=BEHIND block merging.

The numeric-suffix heuristic previously ran inside _normalize_candidate before known_paths/basename resolution ever saw the candidate, so a real bundled root-level file like tool.1 was rejected outright. Move the heuristic into resolve_bundle_references_with_metadata and only apply it once exact and basename resolution have both failed, so a bundled tool.1 still resolves while an unrecognized quoted version string like "1.2" stays rejected. Addresses follow-up review feedback on PR NVIDIA#525. Signed-off-by: goodhee <[email protected]>

goodhee · 2026-09-17T00:12:03Z

Fixed — deferred the version-number rejection as suggested.

_normalize_candidate no longer rejects a numeric-suffix candidate at all; it just normalizes the path. The heuristic moved into resolve_bundle_references_with_metadata and is now applied only after both exact (known_paths) and basename-index resolution have failed for a bare token. So:

A root-level bundled tool.1 (in known_paths) resolves via the exact-match branch before the heuristic is ever consulted.
A basename-only match (e.g. a nested docs/tool.1 referenced as bare tool.1) resolves via the basename-index branch, same priority.
An ambiguous basename match still reports ambiguous, since that check also runs before the heuristic.
Only a token that matches nothing in the bundle and looks like a version number ("1.2", "v1.2", "1.2.0-beta.1") is rejected.

Added test_root_level_numeric_suffix_known_path_resolves: known_paths=["SKILL.md", "tool.1"], text references both "tool.1" (resolves) and "1.2" (rejected, no known match) — covers exactly the case you flagged.

Signed-off-by: goodhee <[email protected]>

rng1995

[SkillSpector Review]

Re-reviewed exact head cc018bcd1941e13437fa82ff0f2fcf183c740cf1. The prior known-path issue is partly resolved: exact and basename resolution now happen before the numeric-suffix heuristic, so a bundled root tool.1 and bundled docs/tool.1 resolve. The fallback still rejects real missing paths too broadly.

src/skillspector/references.py:232 calls _looks_like_bare_version_token(PurePosixPath(target).name) without verifying that the target is actually a bare version-shaped token. Therefore an explicit unresolved path such as docs/tool.1—and even a root tool.1 absent from known_paths—is classified rejected/out-of-scope instead of missing/partial. That silently removes an unresolved artifact from reference completeness. Limit rejection to an entire genuinely version-shaped bare token (for example v?digits.digits...), preserve directory-signaled and filename-like numeric extensions as missing when absent, and add regressions for absent docs/tool.1 and tool.1 alongside the known-path cases.

All exact-head hosted checks pass, but this install-gating completeness regression requires a code and test change before approval.

Signed-off-by: Mohit Gupta <[email protected]>

mohgupta-ship-it · 2026-09-24T14:04:25Z

Powered by Codex, on behalf of the repository contributor: scoped adversarial/council review and implementation update.

This is a request for maintainer re-review, not a maintainer approval. The original contributor commits are preserved. The remaining missing-file blocker is addressed: absent quoted/code/Markdown tool.1 stays missing, and known or ambiguous numeric filenames retain normal resolution. The prior review's absent docs/tool.1 claim did not reproduce on the old head; that path already remained missing and is now an explicit regression control.

flowchart LR
  A[Untrusted skill text] --> B[Bounded YAML scalar positions]
  B --> C[Reference extraction and resolution]
  C --> D[Completeness ledger]
  D --> E[Strict CLI result]

The exclusion applies only to a version scalar at its exact source offsets. Text inside multiline YAML descriptions and body text after frontmatter remain eligible references. Unrecognized or limited YAML receives no exemption; existing coverage limits still report incomplete analysis.

Reviewed head: c70823a8ef67a8b72f7ed3fd4c1999cf93f48117, based on c7958a3268d9498644b22edb75d0f051bbc8cbfc.

The spec/regression, security/release, and runtime/architecture specialists independently reviewed the changed surface. Parent reproductions and the final judge verified the fixes for multiline-scalar confusion and Unicode source-coordinate collisions before publication. Rating: bugfix. Code/dataset verdict: READY for maintainer review. Merge disposition remains BLOCKED_BY_CI_OR_CONFLICT pending exact-head hosted checks and fresh maintainer review. No unresolved actionable code finding remains in the supplied evidence. Confidence is high for reference extraction, completeness accounting, and static CLI behavior.

The unchanged 48-case replay improved 41/48 → 43/48, fixing exactly the quoted top-level and nested versions. All eight smoke and all three full-size stress cases passed; the 14-case AST probe remains 7/14. The nested-version case retains its P9 finding and score 8, wildcard/latest RP3 advisories remain, and the explicit missing-file case still exits 1 under the strict gate. Input hashes were verified before and after.

57 focused tests and 65 JavaScript tests passed. Full unit selection: 7,917 passed, 14 skipped, 4 xfailed, and one five-second FIFO subprocess timeout that passed unchanged on isolated retry (provisionally flaky). Local Docker build/smoke tests, lint, format, diff checks, and signoffs pass. The interrupted local coverage run is not a passing CI result; fresh hosted CI must complete.

Conservative limits remain: unsupported or oversized YAML retains normal reference accounting, and parser cancellation is cooperative between bounded events. The five unrelated baseline dataset failures and seven existing AST-probe failures are unchanged.

Authored by Codex on behalf of Mohit Gupta. Avoid redundant event parsing for headers that cannot produce a version exemption, while retaining real body reference accounting. Signed-off-by: Mohit Gupta <[email protected]>

rng1995 · 2026-09-26T05:28:39Z

@goodhee - Please address review comments to merge the PR. Thanks for your contribution!

rng1995

[SkillSpector Review]

Reviewed exact head 7abe499e373b93d39d8d73a480ddb2f1f875e470, including the previous requested changes. The former numeric-filename rejection is removed: absent tool.1 and docs/tool.1 remain missing; known files resolve; ambiguous basenames retain partial coverage. The version exception is now tied to a real quoted YAML scalar and absolute source offsets, preserving body, comment, and multiline-description references.

Validation: 650 targeted tests passed (version/reference regressions, security remediation, and related Markdown destination coverage), including the unchanged alias-amplification resource test. An additional 36 independent controls passed for CRLF, quoted keys, nested metadata, version syntax, and a real missing numeric file. I found no remaining actionable code regression in this diff. These are focused results, not a full-suite claim.

GitHub currently reports merge conflicts with main. Resolve those conflicts and rerun checks on the resulting head before merging; this review covers only the commit above.

Review performed with Codex on behalf of Narendran Raghavan.

rng1995

[SkillSpector Review]

Re-reviewed exact head 7abe499e373b93d39d8d73a480ddb2f1f875e470, including the complete three-file diff, reference extraction/resolution context, previous reviews and author responses, and the regression tests. The prior numeric-filename findings are addressed: known root tool.1 and directory-qualified docs/tool.1 resolve, absent numeric filenames remain missing/partial, and ambiguous basename matches remain ambiguous. The old broad numeric-suffix rejection is gone.

The version exemption is now restricted to quoted version-shaped YAML frontmatter scalars at exact source offsets. Multiline descriptions, body references, comments, explicit paths, malformed/oversized frontmatter, and Unicode line-separator cases retain reference accounting. The bounded event parser does not construct YAML objects or expand aliases, and existing runtime/candidate/output limits remain conservative. The added resolver and strict-CLI assertions check these success, failure, ambiguity, and security-preservation cases.

All six exact-head hosted checks pass. Static source/test inspection only; contributor code/tests were not executed locally in this re-review. No remaining required change found in this PR; approved at the commit above. This review does not perform a merge.

rng1995

[SkillSpector Review]

Second pass at 7abe499e373b93d39d8d73a480ddb2f1f875e470 found one introduced boundary defect in the quoted-version exemption. An artificial 64 KiB prefix ending can be mistaken for a closing frontmatter delimiter, suppressing a missing numeric reference from malformed input. The inline comment includes a minimal reproduction.

Independent verification: all 12 boundary variants retain the reference on the base and lose it on this head; 16 additional scalar ownership, body state, event-budget and deadline controls pass on the head. These checks supplement the first review rather than repeat its suites. The existing manifest error still prevents complete analysis; this finding concerns reference accounting.

Please fix the delimiter-boundary check and add a regression test. The reported conflict with main also needs resolution and checks on the resulting head before merging. Approval is withheld for this commit because of the confirmed finding.

Review performed with Codex on behalf of Narendran Raghavan.

rng1995 · 2026-09-28T13:40:34Z

+    if not text.startswith("---\n") and not text.startswith("---\r\n"):
+        return set()
+    prefix = text[:_MAX_VERSION_FRONTMATTER_CHARS]
+    closing = re.search(r"^---[ \t]*\r?$", prefix[4:], re.MULTILINE)


[P2] Require a real delimiter boundary before exempting references

The $ anchor also matches the artificial end of the 65,536-character slice. When that slice ends with \n--- but the source continues with not-a-delimiter, this accepts a nonexistent closing delimiter and exempts the earlier numeric reference even though frontmatter is malformed/unclosed. For example:

lead = '---\nversion: "1.2"\n#' text = lead + 'x' * (65532 - len(lead)) + '\n---not-a-delimiter\n'

The base reports the missing 1.2 reference; this head reports none with reference extraction marked complete. Twelve cutoff/whitespace variants reproduce the regression. The manifest parser independently stays incomplete, so this is a lost reference diagnostic rather than a claim that installation becomes allowed. Require a complete delimiter line within the bounded prefix, or verify that the match ends at actual source EOF, and retain references when the boundary is truncated.

goodhee force-pushed the fix/524-quoted-version-metadata-reference branch from f785d45 to 1f2739f Compare September 11, 2026 01:52

rng1995 requested changes Sep 12, 2026

View reviewed changes

goodhee force-pushed the fix/524-quoted-version-metadata-reference branch from 1f2739f to 4305b84 Compare September 15, 2026 01:15

goodhee force-pushed the fix/524-quoted-version-metadata-reference branch from 8d59519 to 6274002 Compare September 15, 2026 01:31

goodhee force-pushed the fix/524-quoted-version-metadata-reference branch from 6274002 to 6d9b525 Compare September 15, 2026 01:56

goodhee added 2 commits September 15, 2026 16:15

goodhee force-pushed the fix/524-quoted-version-metadata-reference branch from 6d9b525 to 68d3ea7 Compare September 15, 2026 07:16

rng1995 requested changes Sep 16, 2026

View reviewed changes

rng1995 mentioned this pull request Sep 16, 2026

Quoted version metadata creates a false missing-file reference and changes score 0 from SAFE to CAUTION #524

Open

style: apply ruff format to the new regression test

cc018bc

Signed-off-by: goodhee <[email protected]>

rng1995 requested changes Sep 18, 2026

View reviewed changes

mohgupta-ship-it added 3 commits September 24, 2026 18:41

Merge main into quoted-version reference fix

6e0e88a

Signed-off-by: Mohit Gupta <[email protected]>

fix(references): recognize quoted versions only in YAML metadata

c03b9b3

Signed-off-by: Mohit Gupta <[email protected]>

fix(references): bind metadata exemptions to absolute source offsets

c70823a

Signed-off-by: Mohit Gupta <[email protected]>

mohgupta-ship-it changed the title ~~fix(references): reject quoted version numbers as file references~~ fix(references): distinguish quoted versions from file references Sep 24, 2026

fix(references): skip YAML pass without version metadata

7abe499

Authored by Codex on behalf of Mohit Gupta. Avoid redundant event parsing for headers that cannot produce a version exemption, while retaining real body reference accounting. Signed-off-by: Mohit Gupta <[email protected]>

rng1995 reviewed Sep 28, 2026

View reviewed changes

rng1995 approved these changes Sep 28, 2026

View reviewed changes

rng1995 requested changes Sep 28, 2026

View reviewed changes

nigel-upstart mentioned this pull request Oct 8, 2026

CLI still reports zero-finding scans as partial/CAUTION when the only exceptions are reference_missing (#510 fixed MCP only) #804

Open

Navigation Menu

Repository navigation

fix(references): distinguish quoted versions from file references - #525

fix(references): distinguish quoted versions from file references#525
goodhee wants to merge 8 commits into
NVIDIA:mainfrom
goodhee:fix/524-quoted-version-metadata-reference

goodhee commented Sep 11, 2026 •

edited by mohgupta-ship-it

Loading

Uh oh!

rng1995 left a comment

Uh oh!

goodhee commented Sep 15, 2026

Uh oh!

rng1995 left a comment

Uh oh!

goodhee commented Sep 17, 2026

Uh oh!

rng1995 left a comment

Uh oh!

mohgupta-ship-it commented Sep 24, 2026

Uh oh!

rng1995 commented Sep 26, 2026

Uh oh!

rng1995 left a comment

Uh oh!

rng1995 left a comment

Uh oh!

rng1995 left a comment

Uh oh!

rng1995 Sep 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

goodhee commented Sep 11, 2026 • edited by mohgupta-ship-it Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

goodhee commented Sep 15, 2026

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

goodhee commented Sep 17, 2026

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

mohgupta-ship-it commented Sep 24, 2026

Uh oh!

rng1995 commented Sep 26, 2026

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

rng1995 Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

goodhee commented Sep 11, 2026 •

edited by mohgupta-ship-it

Loading