Skip to content

Replace symlinks with real files and add conversion script#368

Open
jofoo277 wants to merge 1 commit into
nextlevelbuilder:mainfrom
jofoo277:feat/replace-symlinks-with-files
Open

Replace symlinks with real files and add conversion script#368
jofoo277 wants to merge 1 commit into
nextlevelbuilder:mainfrom
jofoo277:feat/replace-symlinks-with-files

Conversation

@jofoo277

Copy link
Copy Markdown

No description provided.

@mrgoonie

Copy link
Copy Markdown
Contributor

Summary: I’m deferring this in the cron-safe maintainer lane because the PR is too large for a bounded review pass.

Decision: deferred

Evidence:

  • This PR changes 37 files with +9,300/-2 lines.
  • Most of the diff replaces symlinked generated skill assets under .claude/skills/ui-ux-pro-max/data and scripts with real files, plus adds scripts/convert_symlinks.sh.
  • That overlaps with the existing packaging/release/version mismatch thread and related PRs, so it needs a grouped packaging review instead of a quick metadata-only approval.

Next step: please narrow this PR to either (1) the conversion script + documented generation process, or (2) the generated asset replacement, and explain how this should be reconciled with the current source-of-truth files and release workflow. A follow-up maintainer pass should compare this with the other CLI asset sync / symlink / release PRs before approving.

@mrgoonie mrgoonie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary: This PR addresses the real symlink-install problem by replacing .claude/skills/ui-ux-pro-max/data and scripts symlinks with real files, but it is not merge-ready in its current shape.

Mandatory gates:

  • Duplicate/prior implementation: overlaps the same packaging/symlink problem tracked by PR #336 and issue #334/#362, but this PR targets the .claude/skills/ui-ux-pro-max symlink copies specifically.
  • Project standards: source-of-truth should stay under src/ui-ux-pro-max/; mirrored .claude/skills/... assets must be reproducible and kept in sync.
  • Strategic necessity: the goal is valid because ZIP/plugin installs can reject symlinks, but the implementation must not introduce stale generated copies or broken helper scripts.

Requested changes:

  1. Fix scripts/convert_symlinks.sh: found=1 is set inside a piped while subshell, so the final if [ "$found" -eq 0 ] still sees 0 and prints No symlinks found. even after replacements. Use process substitution or another no-subshell loop.
  2. Do not add .claude/skills/ui-ux-pro-max/data/_sync_all.py as a generated one-off maintenance script inside the mirrored skill data directory. It embeds product-specific synchronization logic and should either live in a normal repo scripts/ path with docs/tests or be omitted from the packaged mirror.
  3. Add a reproducible validation path for the mirror: a command/test that confirms the repo/package contains no symlinks in installable plugin/skill artifacts and that mirrored files match the src/ui-ux-pro-max/ source-of-truth.
  4. Reconcile this with the related packaging/symlink PRs so maintainers do not merge two competing mirror strategies.

Verdict: Request changes. The problem is worth fixing, but this PR currently adds a very large copied mirror plus a broken conversion script without enough reproducibility guarantees.

@clark-cant

Copy link
Copy Markdown
Contributor

Cron maintainer follow-up: this PR is still mergeable and the review from 2026-06-24 requested specific changes that have not been addressed yet:

  1. Fix scripts/convert_symlinks.sh subshell variable scoping bug
  2. Remove _sync_all.py from the mirrored skill data directory
  3. Add a reproducible validation command proving mirrored files match src/ui-ux-pro-max/ source-of-truth
  4. Reconcile with overlapping packaging PRs (Fix codegen/validator bugs and a standalone-install break across four skills #346, fix: ship skill assets as real copies for Windows compatibility #336)

No new commits since the review (head: d7eeaac8). This remains deferred until the author responds or updates the branch.

Posted by github-maintain cron at 2026-06-26T08:50:46Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:github-maintain Processed by github-maintain automation maintain:deferred Deferred by maintain workflow pr:oversized Too large for cron-safe review budget

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants