Harden against Windows-breaking symlinks (follow-up to #203) - #206
Closed
T-Nid (tyaginidhi) wants to merge 3 commits into
Closed
Harden against Windows-breaking symlinks (follow-up to #203)#206T-Nid (tyaginidhi) wants to merge 3 commits into
T-Nid (tyaginidhi) wants to merge 3 commits into
Conversation
…+ convert CLAUDE.md mirrors Follow-up to #203. That PR fixed the issue #201 install failure by replacing the legacy .claude-plugin manifest symlinks with real JSON files, but two gaps remained: 1. The validator only compared parsed JSON, so a manifest re-committed as a symlink would still pass on Linux/CI (which resolves symlinks) and let the #201 regression back in. Per the open review suggestion on #203, assert each legacy manifest is a committed regular file (not a symlink) before comparing — this is the assertion that actually guards the cross-platform fix. 2. The per-directory CLAUDE.md files were still symlinks to AGENTS.md, carrying the same Windows hazard (git materializes them as tiny text files on core.symlinks=off clones). Convert the 4 CLAUDE.md symlinks (root + canvas-apps/model-apps/power-pages) to real file copies and extend the validator to guard them (regular file + identical content to the sibling AGENTS.md, only where CLAUDE.md exists). The DRY shared-content symlinks (report-issue/telemetry workflows, telemetry/lib) are intentionally left as-is: they rely on the marketplace installer dereferencing them and converting them would duplicate shared content against the documented architecture. Verified: validator passes clean; fails on a re-introduced manifest symlink, a re-introduced CLAUDE.md symlink, and CLAUDE.md content drift. validate-plugin-names and validate-skill-descriptions still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR further hardens the repo against Windows-breaking symlink materialization by (1) strengthening validate-legacy-compatibility.js to reject legacy mirrors committed as symlinks (not just content-different), and (2) converting CLAUDE.md symlinks into real file copies of their sibling AGENTS.md, with validator checks to keep them identical.
Changes:
- Add an
lstat-based “must be a committed regular file” assertion to prevent.claude-pluginmirrors from being reintroduced as symlinks. - Add validation for
CLAUDE.mdmirrors (regular file + identical content toAGENTS.md) only in directories whereCLAUDE.mdexists. - Convert remaining
CLAUDE.mdsymlinks into real files (root + selected plugins) to avoid Windows checkout degradation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+78
to
+80
| if (!fs.existsSync(claudePath)) { | ||
| return; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in 9879a03. Switched the presence test from existsSync (which follows symlinks) to lstatSync, skipping only on ENOENT. Now a dangling CLAUDE.md symlink reaches assertRegularFile and fails instead of being silently skipped. Added a negative test covering exactly that case (dangling symlink → existsSync reports absent, validator now exits 1).
…not existsSync Address review feedback on #206: existsSync follows symlinks, so a dangling CLAUDE.md symlink (link to a missing target) reported as absent and skipped the very regular-file guard meant to reject symlinks. Switch the presence test to lstatSync (non-following) and skip only on ENOENT, so any committed symlink — resolving or dangling — reaches assertRegularFile and fails the check. Verified: a dangling CLAUDE.md symlink now fails (previously passed); clean tree still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the two non-blocking gaps raised in review of #206. Test coverage gap: CI only ran validate-legacy-compatibility.js against the real (always-clean) repo, exercising the happy path but never the symlink-rejection / drift-detection logic — a future refactor could drop assertRegularFile and CI would stay green. Add a LEGACY_COMPAT_ROOT seam so the script can validate a fixture tree, and scripts/tests/validate-legacy-compatibility.test.js (node:test) which plants each regression — symlinked manifest, symlinked CLAUDE.md, dangling CLAUDE.md symlink, content drift — and asserts the validator exits non-zero. Wired into CI as a new `node --test scripts/tests/` step in validate-repository-metadata. Symlink-creation cases self-skip where the platform forbids symlinks; the ubuntu CI runner runs them all. Doc gap: the Legacy Marketplace Compatibility section documented the manifest-mirror sync rule but not the new CLAUDE.md one. Add a paragraph stating each CLAUDE.md is a committed real-file mirror of its sibling AGENTS.md (never a symlink) that must be updated in the same change, enforced by the validator + its tests. Re-synced root CLAUDE.md to match the edited AGENTS.md. Verified: 7/7 tests pass; validator still green on the real repo; plugin-names and skill-descriptions checks unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
addressed in a different PR |
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.
Follow-up to #203 (which fixed #201). #203 replaced the legacy
.claude-pluginmanifest symlinks with real JSON files, unblocking marketplace install on Windows. This PR closes two remaining gaps surfaced during review of #203.1. Take in the open review suggestion on #203 (validator hardening)
Copilot noted on #203 that
assertJsonMirroronly compares parsed JSON, so a legacy manifest re-committed as a symlink would still pass on Linux/CI (which resolves symlinks) — letting the exact #201 regression back in undetected. CI runs onubuntu-latest, so a content-only check never catches the Windows-only breakage.validate-legacy-compatibility.jsnow asserts each legacy manifest is a committed regular file (not a symlink) before comparing content. Thislstat-based assertion is what actually guards the cross-platform fix.2. Convert the remaining
CLAUDE.mdsymlinks (the follow-up hazard)The four
CLAUDE.mdfiles (root +canvas-apps/model-apps/power-pages) were still symlinks to their siblingAGENTS.md, carrying the same #201 hazard: on a default Windows clone (core.symlinks=false, Developer Mode off) git materializes them as 9-byte text files containingAGENTS.md. They don't block marketplace registration (so they're out of scope for #203), but they degrade the installed plugin.AGENTS.md.AGENTS.md, checked only where aCLAUDE.mdexists (socode-apps/mcp-apps, which have none, don't false-fail).Deliberately out of scope
The DRY shared-content symlinks —
report-issue-workflow.md(×5),telemetry-workflow.md, andscripts/lib/telemetry/lib— are left as-is. They rely on the marketplace installer dereferencing them (documented inAGENTS.md/CLAUDE.md, PR #185), and converting them to real files would duplicate shared content against that documented architecture. Flagging for a separate decision if full raw-clone portability is wanted.Verification
node scripts/validate-legacy-compatibility.js→ passes on the clean tree.CLAUDE.mdsymlink → fails;CLAUDE.mdcontent drift → fails.validate-plugin-names.jsandvalidate-skill-descriptions.jsstill pass..claude-plugin/path or among theCLAUDE.mdfiles.🤖 Generated with Claude Code