ci: project-health gates and release repackage#112
Conversation
A `photoshop-v*` tag failed deterministically: the job copied `apps/photoshop/proscenio_export.jsx`, removed in the UXP migration. Build the plugin (`dist/` is gitignored) and zip its contents so manifest.json sits at the archive root the host loads from. Add a workflow_dispatch dry-run that packages without uploading a release - the stale line rotted unnoticed because the job only ran on tags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The lint-photoshop job ran only tsc plus vitest, so the no-unsafe-* family - the any-flow guard at the untyped UXP host boundary that tsc cannot see - was decorative. Wire `pnpm run lint` into CI and mirror it as a pre-commit hook. The src tree already passes clean, so the gate lands green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proscenio-models is the schema source of truth and proscenio-codegen emits every downstream binding from it, yet both sat outside the type gate. Add strict-strict [tool.mypy] blocks (3.11 floor, the package's real runtime) plus CI steps and pre-commit hooks. disallow_any_explicit is dropped from this profile: pydantic's plugin synthesizes model methods that carry explicit Any, so the flag fires on the model definitions themselves, not on author looseness. The gate caught one real bug - a set literal joining to the wrong per-module _Strict base in the union dispatcher resolver. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The plugin is editor-only; a consumer runs the imported character with it disabled. If a builder ever attaches an addon script to an output node, the packed .scn bakes that script path and the scene breaks at load. Mirror importer.gd's owner-set + pack + ResourceSaver.save, reload from disk, and walk every node asserting get_script() == null - across all four fixture documents in the existing headless pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
slot_cycle and simple_psd were the only image-loading fixtures whose build scripts lacked the explicit bpy.path.relpath rewrite that blink_eyes / automesh / slot_swap already carry. Add it (with the Windows cross-drive ValueError guard) so a regeneration cannot bake a machine-absolute path into the committed .blend. The committed .blends already store relative // paths: save_as_mainfile remaps relative by default, verified via image.filepath_raw, and the full run_tests.py re-export stays green. So no rebake was needed - the script change makes the guarantee explicit for the next regeneration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Five of the six now-items landed this PR: the release UXP repackage, the eslint gate, the models/codegen mypy gate, the saved-scene assert, and the fixture relpath hardening. The mixed-feature fixture is carried to a focused follow-up PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPR implements six interconnected improvements for Spec 035 project health: strict mypy type checking for Python packages with CI and pre-commit integration, ESLint linting gate for Photoshop, enhanced release workflow supporting manual dry-run dispatch and improved Photoshop packaging, Godot scene save validation to guard against baked addon scripts, Blender fixture image path rewriting for cross-platform portability, and documentation updates. ChangesSpec 035 Project Health Checkpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
54-54: ⚖️ Poor tradeoffConsider pinning actions to commit SHAs for supply chain security.
Using version tags (
@v4,@v2) instead of commit SHAs makes the workflow vulnerable to tag-moving attacks. This is a broader pattern across the workflow file (lines 34, 54, 59, 98).Pinning to SHAs with version comments improves supply chain security and follows GitHub's recommended practices.
Example SHA-pinned format
- uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2Also applies to: 59-59, 98-98
🤖 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 @.github/workflows/release.yml at line 54, The workflow pins third-party actions by mutable tags (e.g., "uses: actions/setup-node@v4") which is susceptible to tag-moving attacks; replace each occurrence of a tag-pinned action with the corresponding commit SHA (for example "uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8") and add a trailing comment with the human-friendly tag (e.g., "# v4.0.2"); apply this change to all "uses:" entries currently using tags (including the shown actions/setup-node@v4 and the other tag-pinned actions referenced in the file) so every action is pinned to a specific commit SHA and annotated with the version comment.Source: Linters/SAST tools
packages/fixtures/simple_psd/build_blend.py (1)
103-123: ⚡ Quick winExtract
_rewrite_images_to_relpath()to a shared fixture utility module.The helper in
packages/fixtures/simple_psd/build_blend.pyandpackages/fixtures/slot_cycle/build_blend.pyduplicates identical logic already present inpackages/fixtures/automesh/build_blend.pyandpackages/fixtures/blink_eyes/build_blend.py. The only variation is the log prefix string. Consolidating into a shared module (e.g.,packages/fixtures/_shared/blend_utils.py) would establish a single source of truth for the relpath rewrite pattern and simplify future changes to error handling or logging.🤖 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 `@packages/fixtures/simple_psd/build_blend.py` around lines 103 - 123, Duplicate helper _rewrite_images_to_relpath is defined in multiple fixture build_blend files; extract it into a shared fixture utility module (e.g., _shared/blend_utils.py) as a single function (e.g., rewrite_images_to_relpath) that accepts a log_prefix or logger argument to preserve per-fixture prefix differences, move the implementation (including the bpy.path.relpath try/except and stderr print) into that module, and update the existing callers (functions named _rewrite_images_to_relpath in the various build_blend.py files) to import and call the shared rewrite_images_to_relpath with their specific prefix so behavior and logging remain identical but centralized.
🤖 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 @.github/workflows/release.yml:
- Around line 39-48: The current shell directly expands inputs.component and
inputs.version into the script which allows template/shell injection; instead
read the raw inputs into safe intermediate variables and emit them to
GITHUB_OUTPUT using a safe, non-evaluating write (e.g., assign to local vars
like component_raw/version_raw and use printf '%s' or GitHub safe write
semantics to append to GITHUB_OUTPUT), and when parsing tags keep the
tag/GITHUB_REF handling unchanged but operate on a copied variable (tag_raw) to
avoid accidental evaluation; update references to component and version to use
these sanitized intermediate variables before echoing to GITHUB_OUTPUT.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Line 54: The workflow pins third-party actions by mutable tags (e.g., "uses:
actions/setup-node@v4") which is susceptible to tag-moving attacks; replace each
occurrence of a tag-pinned action with the corresponding commit SHA (for example
"uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8") and add a
trailing comment with the human-friendly tag (e.g., "# v4.0.2"); apply this
change to all "uses:" entries currently using tags (including the shown
actions/setup-node@v4 and the other tag-pinned actions referenced in the file)
so every action is pinned to a specific commit SHA and annotated with the
version comment.
In `@packages/fixtures/simple_psd/build_blend.py`:
- Around line 103-123: Duplicate helper _rewrite_images_to_relpath is defined in
multiple fixture build_blend files; extract it into a shared fixture utility
module (e.g., _shared/blend_utils.py) as a single function (e.g.,
rewrite_images_to_relpath) that accepts a log_prefix or logger argument to
preserve per-fixture prefix differences, move the implementation (including the
bpy.path.relpath try/except and stderr print) into that module, and update the
existing callers (functions named _rewrite_images_to_relpath in the various
build_blend.py files) to import and call the shared rewrite_images_to_relpath
with their specific prefix so behavior and logging remain identical but
centralized.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6aacf9f6-ff68-4d54-a899-7dd29d2c6df3
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/release.yml.pre-commit-config.yamlapps/godot/tests/test_importer.gdpackages/codegen/pyproject.tomlpackages/codegen/src/proscenio_codegen/godot_emit.pypackages/fixtures/simple_psd/build_blend.pypackages/fixtures/slot_cycle/build_blend.pypackages/models/pyproject.tomlspecs/035-project-health/TODO.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
apps/godot/**/*.{gd,gdc,gdns,gdst,tscn,scn}
📄 CodeRabbit inference engine (AGENTS.md)
The Godot plugin runs only at editor import time. Generated scenes must use only built-in nodes (
Skeleton2D,Bone2D,Polygon2D,Sprite2D,Node2D,AnimationPlayer,AnimationLibrary). No GDExtension or native runtime
Files:
apps/godot/tests/test_importer.gd
🪛 LanguageTool
specs/035-project-health/TODO.md
[uncategorized] ~9-~9: The official name of this software platform is spelled with a capital “H”.
Context: ...ce the stale .jsx copy in release.yml with a UXP build...
(GITHUB)
[uncategorized] ~16-~16: The official name of this software platform is spelled with a capital “H”.
Context: ...p to the lint-photoshop job in ci.yml. - [x] Mirror it as a...
(GITHUB)
🪛 zizmor (1.25.2)
.github/workflows/release.yml
[error] 41-41: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 54-54: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 59-59: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 54-54: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
[info] 83-83: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 86-86: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 98-98: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 98-98: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🔇 Additional comments (25)
specs/035-project-health/TODO.md (6)
9-11: LGTM!
15-17: LGTM!
21-23: LGTM!
27-28: LGTM!
32-33: LGTM!
37-37: LGTM!packages/models/pyproject.toml (1)
18-43: LGTM!packages/codegen/pyproject.toml (1)
22-44: LGTM!packages/codegen/src/proscenio_codegen/godot_emit.py (1)
124-133: LGTM!.github/workflows/ci.yml (1)
31-34: LGTM!Also applies to: 54-56
.pre-commit-config.yaml (1)
52-79: LGTM!.github/workflows/release.yml (5)
9-25: LGTM!
52-67: LGTM!
80-83: Packaging logic correctly implements the manifest-at-root requirement.The workflow archives
apps/photoshop/distcontents (where webpack placesmanifest.jsonafter build) and preserves the flat structure required by UDT's plugin loader.
91-96: LGTM!
98-99: LGTM!packages/fixtures/simple_psd/build_blend.py (1)
48-49: LGTM!packages/fixtures/slot_cycle/build_blend.py (1)
57-58: LGTM!apps/godot/tests/test_importer.gd (7)
80-80: LGTM!
128-129: LGTM!
170-171: LGTM!
246-247: LGTM!
330-362: LGTM!
364-371: LGTM!
373-378: LGTM!
CodeRabbit/zizmor flagged a template-injection vector: the free-text
`inputs.version` expanded straight into the meta step's shell body.
Route the dispatch inputs and the derived component/version through env
vars in the meta and build steps so no attacker-influenceable value
reaches the script as `${{ }}` text.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Spec 035 (project health and shipping), first PR: the five cheap, high-value toolchain and shipping items. The sixth now-item, the end-to-end mixed-feature fixture, is split into a focused follow-up PR.
What
release.yml): aphotoshop-v*tag failed deterministically - the job copied the retiredproscenio_export.jsx. It now builds the UXP plugin (dist/is gitignored) and zips its contents withmanifest.jsonat the archive root. Adds aworkflow_dispatchdry-run that packages without uploading a release.strictTypeChecked+stylisticTypeCheckedconfig was decorative (CI ran only tsc + vitest). Wirespnpm run lintintolint-photoshopplus a pre-commit hook - theno-unsafe-*family now guards the untyped UXP host boundary. Thesrctree already passes clean.[tool.mypy]blocks + CI steps + pre-commit hooks.disallow_any_explicitis dropped from this profile (pydantic's plugin-synthesized methods carry explicitAnyon every model); the rest of the strict profile stays. The gate caught one real bug - a set literal joining to the wrong per-module_Strictbase in the union dispatcher resolver.test_importer.gd): packs each built character, saves, reloads, and asserts every node'sget_script() == null- the shipped.scnmust carry no editor-only addon script refs. Runs for all four fixtures in the existing headless pass.bpy.path.relpathrewrite (with the Windows cross-drive guard). The committed.blends already store relative//paths (save_as_mainfileremaps by default), so no re-bake was committed - the change makes the guarantee explicit for the next regeneration.Verification
workflow_dispatchdry-run exercised for all three components: photoshop zip carriesmanifest.jsonat the root, blender the addon tree, godotaddons/proscenio/.run_tests.pyre-export stays green after the fixture script change;test_importer.gdpasses 65 assertions (the saved-scene guard was verified to fail on an injected script, then reverted).Follow-up
The mixed-feature fixture (skinned body + sprite_frame + slot + atlas + drive-from-bone + animation, baked Blender-to-Godot golden) lands in its own PR. Its precondition is met: the export-correctness writer fixes (picker-first
find_armature,MeshElement.polygons) are already in code.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Documentation