REVIEW-OF-RECORD — APPROVE (#690, head 19c84feb7c). I reviewed the fresh-install skills sync path fix and found no blockers.
Path resolution: finalize.ts now resolves framework helper scripts through tools/_scripts/<name> first, then falls back to legacy bin/<name> only when the current-layout script is absent. The helper is used for all finalize helper-script call sites in this file: mosaic-link-runtime-assets, mosaic-sync-skills, and mosaic-doctor. Missing-script errors point at the current tools/_scripts layout, which matches the framework installer.
Fresh install / legacy behavior: the updated skills tests cover the fresh tools/_scripts/mosaic-sync-skills path and the legacy bin/mosaic-sync-skills fallback. I also ran a temp MOSAIC_HOME framework sync-only install and verified the fresh layout has tools/_scripts/mosaic-sync-skills, no legacy bin/mosaic-sync-skills, and the script installs a selected fake lint skill into the runtime skill directory while not linking an unselected skill. Legacy fallback remains intact by construction and test coverage.
Scope/docs: the diff is limited to finalize path resolution, its skills tests, framework README helper paths, and a scratchpad. No unrelated installer/wizard/provider setup changes leaked in.
Reviewer verification on head 19c84feb: pnpm --filter @mosaicstack/mosaic test -- finalize-skills pass (6/6); pnpm --filter @mosaicstack/mosaic test pass (626/626); mosaic typecheck pass; mosaic lint pass; repo format:check pass; repo build pass (23/23); fresh temp framework sync + selected skill install smoke pass. PR CI reported green on pipeline 1641. APPROVE.
REVIEW-OF-RECORD — APPROVE (#690, head 19c84feb7cb0d2adc8973cabf03ef4d195f2bc3c). I reviewed the fresh-install skills sync path fix and found no blockers.
Path resolution: `finalize.ts` now resolves framework helper scripts through `tools/_scripts/<name>` first, then falls back to legacy `bin/<name>` only when the current-layout script is absent. The helper is used for all finalize helper-script call sites in this file: `mosaic-link-runtime-assets`, `mosaic-sync-skills`, and `mosaic-doctor`. Missing-script errors point at the current `tools/_scripts` layout, which matches the framework installer.
Fresh install / legacy behavior: the updated skills tests cover the fresh `tools/_scripts/mosaic-sync-skills` path and the legacy `bin/mosaic-sync-skills` fallback. I also ran a temp `MOSAIC_HOME` framework sync-only install and verified the fresh layout has `tools/_scripts/mosaic-sync-skills`, no legacy `bin/mosaic-sync-skills`, and the script installs a selected fake `lint` skill into the runtime skill directory while not linking an unselected skill. Legacy fallback remains intact by construction and test coverage.
Scope/docs: the diff is limited to finalize path resolution, its skills tests, framework README helper paths, and a scratchpad. No unrelated installer/wizard/provider setup changes leaked in.
Reviewer verification on head 19c84feb: `pnpm --filter @mosaicstack/mosaic test -- finalize-skills` pass (6/6); `pnpm --filter @mosaicstack/mosaic test` pass (626/626); mosaic typecheck pass; mosaic lint pass; repo format:check pass; repo build pass (23/23); fresh temp framework sync + selected skill install smoke pass. PR CI reported green on pipeline 1641. APPROVE.
Independent review (reviewer ≠ author). Blocker B2 (greenfield wizard: "Skills sync script not found at ~/.config/mosaic/bin/mosaic-sync-skills" → "Skills: install failed").
Root cause/fix: framework migration moved helper scripts bin/ → tools/_scripts/, but finalize.ts still resolved mosaicHome/bin. Fix resolves tools/_scripts/<name> first with a legacy bin/<name> fallback for pre-migration installs; missing-script warning points at the current layout; README examples updated.
Tests: finalize-skills spec updated to current layout + new fallback-path test. Fresh-install verification documented (temp MOSAIC_HOME, MOSAIC_SYNC_ONLY=1).
CI: PR-event pipeline 1641 fully green (ci-postgres, typecheck, lint, format, test all success).
Approving for squash-merge to next.
REVIEW-OF-RECORD — APPROVE
Independent review (reviewer ≠ author). Blocker **B2** (greenfield wizard: "Skills sync script not found at ~/.config/mosaic/bin/mosaic-sync-skills" → "Skills: install failed").
**Root cause/fix:** framework migration moved helper scripts `bin/` → `tools/_scripts/`, but `finalize.ts` still resolved `mosaicHome/bin`. Fix resolves `tools/_scripts/<name>` first with a legacy `bin/<name>` fallback for pre-migration installs; missing-script warning points at the current layout; README examples updated.
**Tests:** finalize-skills spec updated to current layout + new fallback-path test. Fresh-install verification documented (temp MOSAIC_HOME, MOSAIC_SYNC_ONLY=1).
**CI:** PR-event pipeline 1641 fully green (ci-postgres, typecheck, lint, format, test all success).
Approving for squash-merge to `next`.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Refs #462
Verification
REVIEW-OF-RECORD — APPROVE (#690, head
19c84feb7c). I reviewed the fresh-install skills sync path fix and found no blockers.Path resolution:
finalize.tsnow resolves framework helper scripts throughtools/_scripts/<name>first, then falls back to legacybin/<name>only when the current-layout script is absent. The helper is used for all finalize helper-script call sites in this file:mosaic-link-runtime-assets,mosaic-sync-skills, andmosaic-doctor. Missing-script errors point at the currenttools/_scriptslayout, which matches the framework installer.Fresh install / legacy behavior: the updated skills tests cover the fresh
tools/_scripts/mosaic-sync-skillspath and the legacybin/mosaic-sync-skillsfallback. I also ran a tempMOSAIC_HOMEframework sync-only install and verified the fresh layout hastools/_scripts/mosaic-sync-skills, no legacybin/mosaic-sync-skills, and the script installs a selected fakelintskill into the runtime skill directory while not linking an unselected skill. Legacy fallback remains intact by construction and test coverage.Scope/docs: the diff is limited to finalize path resolution, its skills tests, framework README helper paths, and a scratchpad. No unrelated installer/wizard/provider setup changes leaked in.
Reviewer verification on head
19c84feb:pnpm --filter @mosaicstack/mosaic test -- finalize-skillspass (6/6);pnpm --filter @mosaicstack/mosaic testpass (626/626); mosaic typecheck pass; mosaic lint pass; repo format:check pass; repo build pass (23/23); fresh temp framework sync + selected skill install smoke pass. PR CI reported green on pipeline 1641. APPROVE.REVIEW-OF-RECORD — APPROVE
Independent review (reviewer ≠ author). Blocker B2 (greenfield wizard: "Skills sync script not found at ~/.config/mosaic/bin/mosaic-sync-skills" → "Skills: install failed").
Root cause/fix: framework migration moved helper scripts
bin/→tools/_scripts/, butfinalize.tsstill resolvedmosaicHome/bin. Fix resolvestools/_scripts/<name>first with a legacybin/<name>fallback for pre-migration installs; missing-script warning points at the current layout; README examples updated.Tests: finalize-skills spec updated to current layout + new fallback-path test. Fresh-install verification documented (temp MOSAIC_HOME, MOSAIC_SYNC_ONLY=1).
CI: PR-event pipeline 1641 fully green (ci-postgres, typecheck, lint, format, test all success).
Approving for squash-merge to
next.