Skip to content

test(references): cover nvcarps 154 acceptance cases - #551

Open
rng1995 wants to merge 32 commits into
mainfrom
naren/fix-NVCARPS-154-reference-regressions
Open

rng1995 wants to merge 32 commits into
mainfrom
naren/fix-NVCARPS-154-reference-regressions

Conversation

@rng1995

@rng1995 rng1995 commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add explicit regression coverage for the reference-resolution acceptance boundaries tracked by NVCARPS-154:

  • literal and Markdown SKILL.md self-references resolve deterministically;
  • an existing references/windows-host-setup.md target resolves and remains analyzed;
  • genuine missing targets and duplicate-basename ambiguity remain fail-closed.

Reproduction and root cause

The reported valid targets could not be reproduced as unresolved on current main. A replay of the original revision likewise showed those exact targets as resolved and analyzed; its incomplete result came from other analysis limitations and unrelated unresolved candidates. A production resolver change would therefore be speculative.

This PR makes the smallest defensible change: it turns the ticket's stated boundaries into full-graph regression tests without weakening missing or ambiguous reference handling.

Changes

  • Add a full-graph fixture covering SKILL.md, ./SKILL.md, and references/windows-host-setup.md.
  • Assert no AE1/reference-resolution exception and a complete SAFE result for those valid targets.
  • Extend the existing fail-closed test to cover both a missing target and duplicate-basename ambiguity.

Validation

  • Focused acceptance cases: 3 passed.
  • tests/nodes/test_security_remediation.py: 315 passed.
  • Ruff lint and formatting checks passed.
  • git diff --check passed.

Compatibility and risk

Tests only; there is no runtime or public API change. The main risk is brittleness in the regression assertions, mitigated by validating both the resolver records and the final full-graph completeness/risk result.

Rollback

Revert the test commit; no production behavior or data migration is involved.

@rng1995 rng1995 changed the title test(references): cover NVCARPS-154 acceptance cases test(references): cover nvcarps 154 acceptance cases Sep 16, 2026

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Manual Review Needed — no required test change found

Reviewed current head 70f0fa10039cca6fba951b35295a17c2e6439e67 for NVCARPS-154. This is a focused test-only change and appropriately avoids a speculative production modification when the reported valid-reference failure cannot be reproduced. The full-graph positive case pins literal and Markdown self-references plus the existing nested reference, while the parameterized negative case preserves fail-closed behavior for both missing and duplicate-basename ambiguity. The assertions cover resolver status, target identity, AE1 behavior, completeness, and the final SAFE result, so the reported boundary would regress visibly.

I found no required code or test change. Merge still requires successful completion of all exact-head checks and an independent maintainer review.

The authenticated reviewer rng1995 is also this PR's author, so this comment is not an independent approval.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Manual Review Needed — no required test change found

Re-reviewed current head 8e49150ce13c24b2ecb9c67eb963c0c7b14a529d after the automatic synchronization with main. This merge has the previously reviewed head 70f0fa10039cca6fba951b35295a17c2e6439e67 and current main e19ec01cc85b9891b2976a1157fce492504cfc13 as its exact parents. The only intervening tree changes are #558's three batch-scan files; they do not overlap the sole PR-owned test file, whose reviewed blob is unchanged.

The NVCARPS-154 acceptance coverage remains sound: it pins literal and Markdown self-references and the nested positive case while preserving fail-closed missing-target and duplicate-basename behavior across resolver identity, AE1, completeness, and final risk. There are no review threads and I found no required test change.

Exact-head CI run 35136923449 concluded action_required without starting jobs, leaving no check results. The authenticated reviewer rng1995 is also this PR's author, so this comment is not an independent approval. Successful exact-head checks and independent maintainer review remain required before merge.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Manual Review Needed — no required test change found

Re-reviewed current head 337124c7a4af31dfafbc0d8ae4ee0f948fcc3dcc after synchronization with current main, including the complete test-only diff, surrounding resolver/full-graph behavior, prior marked reviews, and current checks.

The NVCARPS-154 acceptance coverage remains sound. The positive case pins literal and Markdown self-references plus a real nested reference, including target multiplicity, AE1 absence, ledger completeness, and the final SAFE decision. The parameterized negative case retains fail-closed behavior for both a missing target and duplicate-basename ambiguity. I found no required code or test changes.

The authenticated reviewer rng1995 is also the PR author, so this COMMENT is not an independent approval. Five checks have completed successfully (Docker smoke is intentionally skipped), while test-unit is still running; independent approval and successful exact-head CI remain merge gates.

Priority: P0 — this is direct acceptance coverage for customer-tracked NVCARPS-154 behavior.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Manual Review Needed — no required test change found

Re-reviewed current head 20a6255a859b2e2e38756911b0414bbf20ed69fe after the final synchronization with settled main d162d9b343e559be13df8ebba093df3bc9d58c90. The merge retains the previously reviewed NVCARPS-154 test-only diff; I also checked its interaction with #526's reference/completeness changes in the same test module.

The acceptance coverage remains sound. It pins self-reference and existing nested-reference completeness, target multiplicity, AE1 absence, ledger state, and SAFE output, while preserving fail-closed missing and ambiguous-reference cases. I found no required code or test changes.

The authenticated reviewer rng1995 is also the PR author, so this COMMENT is not an independent approval. GitHub currently reports no checks and mergeStateStatus=UNSTABLE; exact-head CI and independent approval remain required.

Priority: P0 — direct acceptance coverage for customer-tracked NVCARPS-154 behavior.

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

One failing regression case needs an adjustment: the ambiguous variant expects the missing-reference ledger reason.

assert result["risk_recommendation"] == "SAFE"


@pytest.mark.parametrize("case", ["missing", "ambiguous"])

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.

The new ambiguous case produces reference_unresolved, but the shared assertion below always expects reference_missing, so this case fails. Please choose the expected reason from case: reference_missing for missing and reference_unresolved for ambiguous.

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.

2 participants