Sitelet https://github.com/NVIDIA/SkillEvaluator/pull/107
Skip to content

fix(hygiene): detect unpinned dependencies with environment markers - #107

Merged
chrisknvidia merged 7 commits into
NVIDIA:mainfrom
rioyu123:codex/fix-requirement-marker-pinning
Sep 8, 2026
Merged

fix(hygiene): detect unpinned dependencies with environment markers#107
chrisknvidia merged 7 commits into
NVIDIA:mainfrom
rioyu123:codex/fix-requirement-marker-pinning

Conversation

@rioyu123

@rioyu123 rioyu123 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Detect unpinned dependencies even when a PEP 508 environment marker contains comparison operators, such as requests; python_version < "3.13".
  • Inspect only the requirement portion before the marker when deciding whether a package has a version constraint.
  • Preserve the existing policy that any requirement comparator counts as constrained; this does not redefine ?pinned? to require ==.
  • Keep direct-reference behavior unchanged because direct-reference URLs may legally contain semicolons.
  • Document the newly emitted warning in the Unreleased changelog.

Before this change, the validator searched the entire physical line for [=<>!], so marker operators could hide an unversioned package:

Requirement Previous result New result
requests; python_version < "3.13" no warning unpinned warning
requests; sys_platform == "win32" no warning unpinned warning
requests>=2; python_version < "3.13" accepted accepted
requests @ https://example.invalid/a;v=1/requests.whl unchanged unchanged

Known adjacent behavior remains out of scope: direct references keep their existing constraint policy, inline comments are not parsed, and backslash continuations are still evaluated as physical lines.

Verification

  • I am familiar with the Contributing Guidelines
  • Added or updated focused tests
  • Updated documentation for user-visible changes
  • Ran make lint
  • Ran make test ? ran the complete affected validator domain instead
  • Ran make build
  • Did not add credentials, private datasets, or proprietary benchmark content

Results:

  • uv run pytest -q tests/validators/test_hygiene.py ? 26 passed, 1 skipped
  • uv run pytest -q tests/validators on Linux ? 847 passed
  • make PYTHON=.venv/bin/python lint ? passed
  • make PYTHON=.venv/bin/python build ? passed

Release Impact

  • No user-visible release note needed
  • Updated CHANGELOG.md

@rioyu123
fix(hygiene): detect unpinned marked requirements
97c0cd2 
Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
rng1995
rng1995 previously requested changes Aug 28, 2026
View reviewed changes

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

One blocking marker-parsing case remains.

Comment thread src/skillevaluator/validators/hygiene.py Outdated
@rioyu123
Merge upstream main and address marker review
5afe819 
Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
@rioyu123
fix(hygiene): accept plain direct references
29877b2 
Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
@chrisknvidia

chrisknvidia commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

@rioyu123 : Can you resolve the merge conflicts?

@rioyu123
Merge remote-tracking branch 'upstream/main' into codex/fix-requireme…
f43b304 
…nt-marker-pinning

# Conflicts:
#	CHANGELOG.md
@rioyu123

rioyu123 commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Resolved by merging the latest main and retaining both the upstream changelog entries and this PR's PEP 508 marker fix. The implementation and regression tests merged without conflicts; the focused hygiene suite passes (38 passed, 1 skipped). The updated head is ready for CI once GitHub Actions is approved.

@rioyu123
rioyu123 force-pushed the codex/fix-requirement-marker-pinning branch from 71caa17 to 0c2cb2e Compare September 4, 2026 00:23
@rioyu123

rioyu123 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

The previous head 71caa17 passed every CI and security check except DCO, which flagged one unsigned commit (docs(hygiene): clarify direct reference handling). I re-signed just that commit and recreated the merge on top of it — the tree at the new head 0c2cb2e is identical to the old one, so the PR diff is unchanged. The new runs are waiting on workflow approval; could you approve them when you get a chance?

@chrisknvidia
fix(hygiene): ignore inline requirement comments
b93037e 
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia
chrisknvidia dismissed rng1995’s stale review September 8, 2026 01:01

All requested changes and review threads are resolved on current head b93037e; dismissing this stale review after maintainer approval.

chrisknvidia
chrisknvidia approved these changes Sep 8, 2026
View reviewed changes

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

LGTM

@chrisknvidia
chrisknvidia merged commit f2d8f5d into NVIDIA:main Sep 8, 2026
17 checks passed
mimran-khan added a commit to mimran-khan/SkillEvaluator that referenced this pull request Sep 9, 2026
@mimran-khan
chore: merge main into fix/refine-harbor-trial-folders
d22ee52 
Resolve CHANGELOG.md after merging upstream NVIDIA#99, NVIDIA#107, and NVIDIA#112.
mimran-khan added a commit to mimran-khan/SkillEvaluator that referenced this pull request Sep 9, 2026
@mimran-khan
chore: merge main into fix/no-llm-negative-case
39d759a 
Resolve CHANGELOG.md after merging upstream NVIDIA#99, NVIDIA#107, and NVIDIA#112.
rng1995 pushed a commit to mimran-khan/SkillEvaluator that referenced this pull request Sep 14, 2026
@mimran-khan
chore: merge main into fix/no-llm-negative-case
0c46f3a 
Resolve CHANGELOG.md after merging upstream NVIDIA#99, NVIDIA#107, and NVIDIA#112.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rng1995 rng1995 rng1995 left review comments
@chrisknvidia chrisknvidia chrisknvidia approved these changes
@mosheabr mosheabr Awaiting requested review from mosheabr mosheabr is a code owner
@rmalani-nv rmalani-nv Awaiting requested review from rmalani-nv rmalani-nv is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rioyu123 @chrisknvidia @rng1995