fix: materialize linked resources before npm publishing - #579
Conversation
|
📁 PR Artifacts Notice This PR contains a |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: fix: materialize linked resources before npm publishing
Taste Rating: 🟢 Good taste
The change is minimal and targeted. cp -RL is the correct tool - it follows symlinks and creates independent regular files, which is exactly what npm needs since it omits symlinks. The PR description's reasoning for rejecting cp -aL (hard-link identity causing malformed archives) is sound and grounded in actual testing.
The approach of staging a release-only copy while retaining the canonical linked source layout in the checkout is clean. No runtime, loader, or installer behavior is changed.
One minor inconsistency (non-blocking)
The Validate package version matches release tag step (line 70) does not set working-directory, so it reads package.json from the original checkout ($GITHUB_WORKSPACE) rather than from the staged copy. This is functionally correct today because cp -RL copies package.json unchanged. However, if a future change to the staging step ever modifies package.json (e.g., stripping dev dependencies, rewriting paths), the validation would check the wrong copy. Consider adding working-directory: ${{ runner.temp }}/extensions-package to that step for consistency.
Evidence
The PR description includes concrete, pinned evidence: actual npm tarballs were packed before and after staging, linked resources were confirmed absent before and present as regular files with matching canonical bytes after, and all four standalone factory workers imported successfully from the extracted package. This satisfies the evidence requirement.
Repository fit
This change belongs in this repo - it modifies the release workflow for @openhands/extensions, which is published from this repository.
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: 🟢 LOW
The change is confined to release packaging. No runtime, loader, or import code is modified. The canonical source layout is preserved. The staging step uses a safe, standardcpcommand with no injection surface. The only operational risk is the validate-step working-directory inconsistency noted above, which is non-blocking.
VERDICT: ✅ Worth merging - Core logic is sound, solves a real packaging problem with a minimal, well-tested change.
KEY INSIGHT: The fix correctly identifies that npm's symlink omission is a packaging-time concern, solved at packaging time without touching the canonical source layout.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
Addressed the version-check consistency suggestion: it now reads package.json from the same staged directory as npm verification/publication. Workflow YAML parses and all three step directories agree. Existing actual npm archive evidence remains applicable. Requesting another all-hands-bot review after this audit. This comment was posted by an AI agent. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: fix: materialize linked resources before npm publishing
Taste Rating: Good taste
This is a clean, minimal fix for a real problem. npm omits symlinks from published tarballs, so consumers installing @openhands/extensions via npm were missing linked resources like skills/github/.claude-plugin/plugin.json and the shared scripts/github_client.py files across factory skills. The solution - staging a dereferenced copy with cp -RL and running the existing pack/verify/publish steps from that directory - is the simplest correct approach.
Analysis
Problem is real and verified. Confirmed in the workspace: 155 symlinks exist in the repo (e.g., skills/github/.claude-plugin -> .plugin, plugins/pr-review/skills/code-review -> ../../../skills/code-review). npm pack --dry-run from the source checkout omits these entirely. After cp -RL, they become regular files and are included.
cp -RL vs cp -aL choice is well-reasoned. The PR description documents that cp -aL preserves hard-link identity, which caused malformed archives and npm packaging failures. cp -RL creates independent regular files. This is a pragmatic, tested decision.
All three downstream steps correctly redirect. npm pack --dry-run, version validation, and npm publish all use working-directory: ${{ runner.temp }}/extensions-package. No step is left operating on the original symlinked checkout.
No security risk. All symlinks in the repo point to relative paths within the repository itself. cp -RL will not dereference anything external.
No breaking change. The canonical source layout in the checkout is unchanged. Git-based skill installation is unaffected. Only the release workflow's staging directory is materialized.
Evidence is thorough. The PR includes actual tarball extraction, byte-level comparison against canonical source, and import testing of all four factory workers from the extracted package. This is end-to-end proof, not just unit test output.
.pr/ artifacts are appropriate. The evidence files in .pr/ are PR-specific artifacts covered by the PR Artifacts workflow, which cleans them up on approval and post-merge. They will not be present on main when release tags are cut.
No material findings
No bugs, security issues, or design flaws found. The change is minimal, well-tested, and solves the stated problem correctly.
[RISK ASSESSMENT]
- Overall PR: LOW
- Change is confined to release packaging workflow; no runtime, loader, or API changes.
- All symlinks are internal to the repo, so
cp -RLcannot pull in external content. - Version validation still reads
package.jsonfrom the staged copy, preserving the existing safety check. - The only new code is two lines (
mkdir -p+cp -RL) plus threeworking-directoryadditions.
VERDICT: Worth merging
KEY INSIGHT: Staging a dereferenced copy is the right boundary - it fixes npm's symlink omission without touching the canonical source layout that git-based consumers depend on.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with thumbs up or thumbs down to give feedback.
3 participants
Why
npm omits source symlinks, so the published package loses linked skill resources and vendor manifests. Git-based skill installation correctly resolves them, but npm consumers receive an incomplete directory.
Summary
Stage a dereferenced copy with
cp -RL, then run the existing package verification and publication from that directory. Canonical source links and runtime/installer behavior stay unchanged.Issue Number
Closes #578
How to Test
Packed actual npm archives from clean main and composed factory checkouts before and after staging, then extracted them. Main's linked
skills/github/.claude-plugin/plugin.jsonand all four factoryscripts/github_client.pyresources are absent before, regular files with identical canonical bytes after. All four standalone workers import successfully from the extracted package. Archives are about 1.1–1.2 MB. Workflow syntax and whitespace checks pass.Video/Screenshots
This changes release packaging, not Canvas UI. The actual packed-package checks exercise the distribution artifact used by Canvas.
Notes
Review before #570–#573 are released. A raw
npm packfrom the source checkout still omits symlinks; the release workflow consistently packs and publishes its materialized copy. No npm release was published during validation. Usecp -RL, not archive mode:cp -aLretains hard-link identity, which produced malformed archives or npm failures during verification.Recorded evidence
Actual npm tarballs omit linked resources before staging and contain regular files with canonical bytes afterward. All four standalone factory workers import from the extracted package. No npm release was published. Report and artifacts. Links are pinned to the artifact commit so review cleanup cannot invalidate them.