test: harden release artifact audit coverage#3
Open
Boulea7 wants to merge 2 commits into
Open
Conversation
Change-Id: I6e160c1ca845ad3768711e9075bed5496554e82d
Reviewer's GuideThis PR strengthens the release artifact audit by dynamically deriving required skill-body files for sdists and wheels from the source tree and by adding focused tests around skill bundle coverage and wheel dist-info metadata validation, including RECORD payload coverage. Flow diagram for deriving required skill body files for sdists and wheelsflowchart TD
A[SKILL_BODIES_ROOT] --> B{root.is_dir}
B -- no --> C[Return empty set]
B -- yes --> D[rglob * over root]
D --> E[_is_required_skill_body_file]
E --> F{path.is_file}
F -- no --> G[Skip path]
F -- yes --> H{component in SKILL_BODY_EXCLUDED_COMPONENTS}
H -- yes --> G
H -- no --> I{path.name is .DS_Store or Thumbs.db}
I -- yes --> G
I -- no --> J{path.name endswith SKILL_BODY_EXCLUDED_SUFFIXES}
J -- yes --> G
J -- no --> K[Include path]
K --> L[_skill_body_files_for_sdist]
L --> M[relative_to PROJECT_ROOT]
M --> N[Add to REQUIRED_SDIST_FILES]
K --> O[_skill_body_files_for_wheel]
O --> P[relative_to SRC_ROOT]
P --> Q[Add to REQUIRED_WHEEL_FILES]
N --> R[Final REQUIRED_SDIST_FILES]
Q --> S[Final REQUIRED_WHEEL_FILES]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR refactors release artifact validation to dynamically discover and include required ChangesRelease artifact validation and skill-body packaging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens the release artifact audit by dynamically deriving required bundled skill-body files from src/agy_mcp/_skill_bodies (instead of maintaining manual lists) and expands test coverage around wheel .dist-info metadata validation and RECORD payload coverage.
Changes:
- Derive
REQUIRED_SDIST_FILES/REQUIRED_WHEEL_FILESskill-body entries dynamically from the source tree while filtering out common noise files. - Add tests ensuring the dynamic skill-body sets are included in the required sets for both sdist and wheel.
- Add tests for
_check_wheel_metadatato validate required dist-info files and detect payload files missing fromRECORD.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/test_release_artifacts.py |
Adds coverage for dynamic required file derivation and wheel dist-info/RECORD validation. |
scripts/check_release_artifacts.py |
Refactors required file sets to be derived from _skill_bodies dynamically and introduces wheel dist-info metadata checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+52
to
+53
| if not root.is_dir(): | ||
| return set() |
Comment on lines
+67
to
+68
| if not root.is_dir(): | ||
| return set() |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Change-Id: Icf710f69475b12f03244045e386b32519f6443ff
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check_release_artifacts.py (1)
456-467:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
RECORDagainst all shipped wheel files.This only compares non-
.dist-infopaths againstRECORD, so a wheel that shipsMETADATA/WHEELbut forgets to record them still passes the audit. That leaves a malformed wheel case uncovered by the new metadata gate.Suggested fix
try: entries = record.data.decode("utf-8").splitlines() except UnicodeDecodeError: entries = [] recorded_paths = {line.split(",", 1)[0] for line in entries if line} - # Every shipped python file must be listed in RECORD. Skipping - # the dist-info entries themselves keeps the check honest about - # the wheel payload rather than the metadata bookkeeping. - payload_paths = { - f.path for f in files if not _DIST_INFO_RE.match(f.path) - } - missing_from_record = sorted(payload_paths - recorded_paths) + shipped_paths = {f.path for f in files} + missing_from_record = sorted(shipped_paths - recorded_paths) for path in missing_from_record: problems.append( f"[{label}] wheel ships {path} but RECORD does not list it", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_release_artifacts.py` around lines 456 - 467, The current check ignores .dist-info files when computing payload_paths so missing metadata like METADATA/WHEEL/RECORD can go undetected; update the check in check_release_artifacts.py to include .dist-info entries in the comparison (or add an explicit assertion that all .dist-info paths are present in RECORD). Locate the variables recorded_paths, payload_paths and missing_from_record and either remove the _DIST_INFO_RE filter when building payload_paths (use all file paths from files) or compute dist_info_paths = {f.path for f in files if _DIST_INFO_RE.match(f.path)} and assert dist_info_paths ⊆ recorded_paths so METADATA/WHEEL/RECORD are validated as recorded.
🧹 Nitpick comments (1)
tests/test_release_artifacts.py (1)
33-44: ⚡ Quick winMake the wheel skill-body expectation independent of the helper under test.
REQUIRED_SDIST_FILESandREQUIRED_WHEEL_FILESare built from_skill_body_files_for_*()at import time, so the subset checks here will still pass if either helper drops or mis-maps a file. The sdist half already has an external oracle viarglob; the wheel half needs the same treatment.Suggested test shape
- sdist_skill_files = _skill_body_files_for_sdist() - wheel_skill_files = _skill_body_files_for_wheel() + repo_root = Path(__file__).resolve().parents[1] + expected_source_files = { + path for path in skill_root.rglob("*") if _is_required_skill_body_file(path) + } + sdist_skill_files = _skill_body_files_for_sdist() + wheel_skill_files = _skill_body_files_for_wheel() assert sdist_skill_files assert wheel_skill_files assert sdist_skill_files <= REQUIRED_SDIST_FILES assert wheel_skill_files <= REQUIRED_WHEEL_FILES - assert { - path.relative_to(Path(__file__).resolve().parents[1]).as_posix() - for path in skill_root.rglob("*") - if _is_required_skill_body_file(path) - } == sdist_skill_files + assert { + path.relative_to(repo_root).as_posix() + for path in expected_source_files + } == sdist_skill_files + assert { + path.relative_to(repo_root / "src").as_posix() + for path in expected_source_files + } == wheel_skill_files🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_release_artifacts.py` around lines 33 - 44, The test currently verifies wheel outputs by comparing wheel_skill_files to REQUIRED_WHEEL_FILES (both derived from _skill_body_files_for_wheel at import time), which makes the check tautological; change the wheel assertion to mirror the sdist oracle: compute the set of required wheel paths by walking skill_root via skill_root.rglob("*") and filtering with _is_required_skill_body_file, mapping each path to a relative posix string (the same expression already used for the sdist assertion), then assert that resulting set equals wheel_skill_files instead of comparing to REQUIRED_WHEEL_FILES; update references to wheel_skill_files and _skill_body_files_for_wheel accordingly so the test no longer depends on the helper under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/check_release_artifacts.py`:
- Around line 456-467: The current check ignores .dist-info files when computing
payload_paths so missing metadata like METADATA/WHEEL/RECORD can go undetected;
update the check in check_release_artifacts.py to include .dist-info entries in
the comparison (or add an explicit assertion that all .dist-info paths are
present in RECORD). Locate the variables recorded_paths, payload_paths and
missing_from_record and either remove the _DIST_INFO_RE filter when building
payload_paths (use all file paths from files) or compute dist_info_paths =
{f.path for f in files if _DIST_INFO_RE.match(f.path)} and assert
dist_info_paths ⊆ recorded_paths so METADATA/WHEEL/RECORD are validated as
recorded.
---
Nitpick comments:
In `@tests/test_release_artifacts.py`:
- Around line 33-44: The test currently verifies wheel outputs by comparing
wheel_skill_files to REQUIRED_WHEEL_FILES (both derived from
_skill_body_files_for_wheel at import time), which makes the check tautological;
change the wheel assertion to mirror the sdist oracle: compute the set of
required wheel paths by walking skill_root via skill_root.rglob("*") and
filtering with _is_required_skill_body_file, mapping each path to a relative
posix string (the same expression already used for the sdist assertion), then
assert that resulting set equals wheel_skill_files instead of comparing to
REQUIRED_WHEEL_FILES; update references to wheel_skill_files and
_skill_body_files_for_wheel accordingly so the test no longer depends on the
helper under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f19c0b10-3484-44a0-9266-e59512d37ce9
📒 Files selected for processing (2)
scripts/check_release_artifacts.pytests/test_release_artifacts.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by Sourcery
Harden release artifact auditing by deriving required skill bundle contents from the source tree and expanding wheel metadata validation coverage.
Enhancements:
Tests:
Summary by cubic
Hardened the release artifact audit by deriving bundled
agy_mcpskill-body files dynamically for sdist and wheel, validating wheel dist-info metadata/RECORD, and scanning only git-tracked files. Prevents missing files, packaging drift, and junk file leaks from local changes.Refactors
src/agy_mcp/_skill_bodies; compute sdist and wheel paths dynamically.__pycache__,.pyc/.pyo,.DS_Store,Thumbs.db.Bug Fixes
METADATA/WHEELand payloads not listed inRECORD.Written for commit a800d96. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Chores
Tests