Add metadata fixture safety checks#112
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a Python metadata fixture checker and unittest harness enforcing data-URI/JSON/URI-scheme and animation-HTML wrapper/script-boundary rules, wires them into Makefile/check scripts and CI (including syntax checks and a dedicated fixture-safety step), and updates docs, changelog, release checksums, and ops state. ChangesMetadata Fixture Validation Gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/check_metadata_fixtures.py (1)
236-237: Add symmetric structural validation for off-chain final tokenURI suffix/pattern
validate_fixture_setvalidatesoffchain_pendingwith safe schemes and enforces it ends with/{STATE_PENDING}(/pending), butoffchain_finalonly gets the safe-schemes check—no correspondingendswith/shape constraint. If the “final” tokenURI is expected to follow a specific pattern (e.g.,/{STATE_FINAL}or/final), apply the same structural validation tooffchain_finalas well. [optional improvement]🤖 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_metadata_fixtures.py` around lines 236 - 237, validate_fixture_set currently enforces that offchain_pending ends with f"/{STATE_PENDING}" but does not apply a symmetric structural check to offchain_final; add a corresponding validation in validate_fixture_set to verify offchain_final.endswith(f"/{STATE_FINAL}") (or the appropriate STATE_FINAL constant) and raise MetadataFixtureError with a clear message like "off-chain final tokenURI must end with /final" when it fails, using the offchain_final identifier and STATE_FINAL constant to locate and implement the check.
🤖 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.
Inline comments:
In `@scripts/check_metadata_fixtures.py`:
- Around line 106-108: validate_external_uri currently returns early for uri ==
"" causing metadata entries with {"image": ""} to pass; change the validation so
that when uri == "" and label == "image" it raises an error (or returns a
failing validation) instead of returning silently. Update the
validate_external_uri function (and its callers like validate_metadata_json) to
enforce non-empty image URIs while keeping existing off-chain tokenURI logic
unchanged (do not add any `/final` suffix checks).
---
Nitpick comments:
In `@scripts/check_metadata_fixtures.py`:
- Around line 236-237: validate_fixture_set currently enforces that
offchain_pending ends with f"/{STATE_PENDING}" but does not apply a symmetric
structural check to offchain_final; add a corresponding validation in
validate_fixture_set to verify offchain_final.endswith(f"/{STATE_FINAL}") (or
the appropriate STATE_FINAL constant) and raise MetadataFixtureError with a
clear message like "off-chain final tokenURI must end with /final" when it
fails, using the offchain_final identifier and STATE_FINAL constant to locate
and implement the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35e98dff-a715-4deb-9bee-dfedd04f744d
📒 Files selected for processing (16)
.github/workflows/ci.ymlCHANGELOG.mdMakefiledocs/known-blockers.mddocs/metadata.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonscripts/check.ps1scripts/check.shscripts/check_metadata_fixtures.pyscripts/test_metadata_fixtures.pytest/README.md
|
@coderabbitai review Latest head Local validation after the fix:
|
|
✅ Action performedReview finished.
|
Summary
make check, Unix/Windows check wrappers, and CI, then update metadata docs, roadmap/run-state traceability, changelog, release manifest, and checksum bundle.Continues #51. This intentionally keeps production bytecode unchanged and leaves production URI enforcement, invalid UTF-8 policy, semantic/structured attributes, and full browser execution sandboxing for later P1-META-006 slices.
Local validation
python scripts\test_metadata_fixtures.pypython scripts\check_metadata_fixtures.pypython -m py_compile scripts\check_metadata_fixtures.py scripts\test_metadata_fixtures.pybash -n scripts/check.sh scripts/bootstrap-ec2.shscripts\check.ps1andscripts\bootstrap-windows.ps1python scripts\test_changelog_check.pypython scripts\check_changelog.pygit diff --checkmake release-checksumsmake checkpowershell -ExecutionPolicy Bypass -File scripts\check.ps1Production bytecode is unchanged; the size gate still reports
StreamCoreat 24,461 runtime bytes with 115 bytes of EIP-170 headroom.Summary by CodeRabbit
New Features
Chores
Tests
Documentation