Skip to content

fix(scripts): install manifest assets before linting registry items (#7279) - #7282

Open
xunguangzlj-cloud wants to merge 2 commits into
heygen-com:mainfrom
xunguangzlj-cloud:fix/lint-registry-manifest-assets-7279
Open

xunguangzlj-cloud wants to merge 2 commits into
heygen-com:mainfrom
xunguangzlj-cloud:fix/lint-registry-manifest-assets-7279

Conversation

@xunguangzlj-cloud

@xunguangzlj-cloud xunguangzlj-cloud commented Nov 6, 2026 •

Copy link
Copy Markdown

Closes #7279

What this changes

Registry lint mounted the composition HTML alone. Items such as yt-lower-third and texture-mask-text failed even though their manifests listed the missing assets.

  • Mount each declared file at its target path. Use the existing registry containment checks before copying, so the manifest cannot read or write outside its allowed directory. Files missing from the item directory remain lint findings.
  • Report an error when the lint subprocess fails to start.
  • Register the new test in test:scripts so CI runs it.

Test

  • The original regression fails on the pre-PR script for the two registry items. The patched script lints both successfully.
  • Two new tests reject escaping source and target paths. Both fail at the previously reviewed head and pass on this revision. The focused suite passes 5/5 on Windows.
  • Full workspace build, test reachability, changed-file formatting and lint, and the local Fallow audit gate pass.
  • The existing registry symlink fixture could not start on this Windows account because creating its test symlinks requires additional OS privileges.

Prepared and revised with AI assistance.

SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Nov 6, 2026
Re-verified the nine merged PRs in heygen-com#7266-heygen-com#7288 against fork file:line and
reverse-apply checks; none needs a new port. The six open PRs (heygen-com#7266, heygen-com#7269,
heygen-com#7281, heygen-com#7282, heygen-com#7285, heygen-com#7286) are pinned as monitored with revisit conditions;
monitored 99 -> 105.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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

Reviewed at 951cee8ca7279c0940df4436b431be99a0c68e9f.

Registry lint mounts manifest-declared assets at their target paths and reports spawn failure instead of treating it as a pass (scripts/lint-registry-items.mjs:113). Genuine missing assets remain findings.

CI integration prerequisite (non-blocking for code approval): add scripts/lint-registry-items.test.mjs to root package.json's explicit test:scripts list. node scripts/check-test-reachability.mjs currently fails with no CI runner selects this test; the main control passes. The runtime fix is clean, but this head cannot satisfy the normal merge gate until that small runner-wiring change lands.

Validation at this head: node --test scripts/lint-registry-items.test.mjs → 3 passed, including the real script on both reported items. Focused tests ran after a full build on current main; built shared parser dependencies were reused. No full render replay or native Windows run locally. git merge-tree --write-tree origin/main 951cee8ca7279c0940df4436b431be99a0c68e9f passed against main 539c70cee. No prior substantive reviews or inline comments were present.

Need: yes, requested starter issue · Size: ok · Reuse: existing lint/parser and manifest contracts · Simpler: already minimal · Structure: fine
Risk: low — bounded diagnostic or registry-lint change with focused coverage

— hf-review

Verdict: APPROVE
Reasoning: The requested behavior is implemented using the existing contracts, and the focused regression tests pass. The missing test-runner entry is a small integration fix; merge still needs normal green checks.

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

Reviewed at 0faec0a1c0be90b2751a7c99d26a484e32cbf2e3 (delta from 951cee8ca7279c0940df4436b431be99a0c68e9f).

The prior test-runner integration concern is resolved: root package.json:57 explicitly selects the new test, and node scripts/check-test-reachability.mjs reports Test reachability verified: zero orphan tests. Manifest copies now check both source and target with the existing filesystem-aware containment helper before copying (scripts/lint-registry-items.mjs:116). No Important findings from the independent adversarial re-review.

Validation: a fresh-worktree bun install --frozen-lockfile --ignore-scripts and full bun run build passed. node --test scripts/lint-registry-items.test.mjs scripts/registry-target-paths.test.mjs passed 17/17, including the real CLI lint on both reported items and filesystem symlink cases. Both new escaping-entry tests fail with the previous reviewed script (0/2 pass), confirming they detect this guard change. Separate real-script probes verified that an escaping source symlink and unavailable Bun fail closed. Changed-file oxfmt/oxlint and merge-tree against main f004622a6 passed. No native Windows run or full registry/render replay locally.

CI remains in progress; merge still requires the normal green checks.

Need: yes, fixes requested registry lint behavior · Size: ok · Reuse: existing registry containment and CLI lint contracts · Simpler: already minimal · Structure: fine
Risk: low — bounded developer lint script with focused integration and containment coverage

— hf-review

Verdict: APPROVE
Reasoning: The manifest assets are mounted under the existing containment contract, and the focused regression suite now participates in CI. The earlier runner concern is fixed and the tested escape cases are rejected.

This branch has not been deployed

No deployments
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.

lint-registry-items.mjs reports missing assets that the item manifest declares

2 participants