Repository navigation
docs: settle the bestax-migrate bundling call, create-bestax ships it - #512
Conversation
…#385) Direction (B) from #385: the skill stays in the create-bestax bundle. Drop the two remaining not-bundled statements (skills intro, migrate page), replace the contested note in bestax-migrate/CLAUDE.md with the settled policy, and cite the decision from create-bestax/CLAUDE.md's per-skill rule. No behavior changes; sync-skills.mjs and setupSkills() are untouched, and pnpm gen produced no diff.
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe changes align ChangesBundling guidance alignment
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: ⚪ Minimal · up to This documentation-only change aligns the bundling guidance without changing runtime behavior; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Aligns documentation with the settled policy that create-bestax bundles the migration skill.
Changes:
- Removes outdated “not bundled” guidance.
- Documents the bundled, idle-until-needed behavior.
- Records the synchronization policy for maintainers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
docs/docs/skills/migrate.mdx |
Describes preinstalled migration behavior. |
docs/docs/skills/intro.md |
Removes obsolete bundling warning. |
create-bestax/CLAUDE.md |
Records the settled bundling decision. |
bestax-migrate/CLAUDE.md |
Documents policy and synchronization points. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Preview DeploymentPreview URL: https://04de1f91.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 1 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | Three "keep in agreement" guidance lists enumerate different roster sets (e.g. bestax-migrate/CLAUDE.md omits skills/README.md; create-bestax/CLAUDE.md omits sync-skills.mjs) — a future doc-drift risk, not a current inconsistency. |
bestax-migrate/CLAUDE.md:77 |
Overall: Sound, low-risk prose-only change that ratifies the settled #385 decision in the last four docs surfaces. I verified the code actually bundles the skill (sync-skills.mjs:23 SKILLS array and constants.ts:215 CLAUDE_MD roster both list bestax-migrate, and setupSkills() copies the whole bundle with no per-skill filter), so every new sentence matches shipped behavior and the #385 "Direction (B)" decision word-for-word. The riskiest thing here is simply whether the docs are now complete — and they are.
Residual risk:
- Stale "not bundled" claims elsewhere — refuted:
git grep -ni "not bundled"over*.md/*.mdxleaves only one hit in the 2-to-3 migration guide about bulma CSS bundling, unrelated to this skill;skills/README.mdtakes no bundling stance and lists the skill in all three of its rosters (table, install block, layout tree). pnpm genshould have produced a regen commit — refuted:gen-mcp-index.mjsreadsdocs/docs/api/andskills/*/SKILL.md, neverdocs/docs/skills/*.mdx, and greppingbestax-mcp/data/for the changed prose ("not bundled", "Unlike the other skills") returns nothing; the indexed migrate description comes fromSKILL.md, which this PR doesn't touch.- Rosters out of agreement after the edit — refuted: all functional rosters (
sync-skills.mjs,constants.tsCLAUDE_MD,skills/README.md,docs/docs/skills/intro.md) currently includebestax-migrate; only the prose guidance lists differ (advisory #1).
Pure paperwork, dude — the code's been riding this wave since #345, the docs just finally paddled out to catch up with #385. Clean, no gnarly wipeouts, ship it.
The deep review flagged that this file's keep-in-agreement list and the one in create-bestax/CLAUDE.md enumerate different sets, which is how the original drift happened. One canonical list beats two copies: reference create-bestax/CLAUDE.md's sync rules instead of restating.
|
Addressed the advisory in f3810b2: instead of aligning two hand-copied rosters (which is exactly how the original drift happened), bestax-migrate/CLAUDE.md now points at the canonical list in create-bestax/CLAUDE.md's sync rules and restates nothing. One list, one owner. On the second half of the finding: create-bestax/CLAUDE.md's list does cover sync-skills.mjs, just as "the allowlist", which the same bullet defines two sentences earlier. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/docs/skills/migrate.mdx:17
create-bestaxdoes not preinstall skills unconditionally: users can decline the prompt or pass--no-skills, andsetupSkills()only runs when that choice is enabled (create-bestax/src/project-creator.ts:571-602). Qualify this sentence so the docs do not promise the migration skill in every scaffold.
Like the rest of the bundle, `create-bestax` preinstalls this skill; it stays idle until the
agent meets code that still imports `react-bulma-components`.
create-bestax/CLAUDE.md:27
- The new
bestax-migrate/CLAUDE.mdtext delegates the canonical bundling-surface roster to this rule, but this list omitsdocs/docs/skills/migrate.mdx, which now contains the explicit bundling claim. That leaves the newly corrected page outside the sync policy and does not fully implement the PR's stated “docs skills pages” agreement list.
decision (bestax-migrate's was settled as bundled, #385) — when adding a skill, decide it
explicitly and keep the allowlist, `skills/README.md`, `docs/docs/skills/intro.md`, and
the `CLAUDE_MD` roster in `src/constants.ts` in agreement. **Never edit the bundled copy** — change `skills/` at the
Preview DeploymentPreview URL: https://1194c5bb.bestax.pages.dev |
Two suppressed Copilot findings, both real. setupSkills() only runs when the user accepts the prompt or passes --skills, so migrate.mdx now says offers to preinstall, matching intro.md. And with bestax-migrate/CLAUDE.md delegating to the canonical roster, that roster now names per-skill docs pages that state bundling, so migrate.mdx sits inside the sync policy.
|
Also picked up Copilot's two suppressed comments in 3be3db5, both fair: migrate.mdx now says "offers to preinstall" (setupSkills() only runs when the user accepts the prompt or passes --skills, and intro.md already used that phrasing), and the canonical roster in create-bestax/CLAUDE.md now covers per-skill docs pages that state bundling, so migrate.mdx sits inside the sync policy it prompted. |
Preview DeploymentPreview URL: https://1dac66f0.bestax.pages.dev |
|
🎉 This PR is included in version 5.11.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.1.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What
Aligns the last docs with the call made in #385: the
bestax-migrateskill is part of the create-bestax bundle. Four files, prose only:docs/docs/skills/intro.md: the Migrate bullet loses its "Not bundled bycreate-bestax" sentence.docs/docs/skills/migrate.mdx: the "Unlike the other skills, this one is not bundled" paragraph becomes the bundled framing (preinstalled, idle until legacy imports show up).bestax-migrate/CLAUDE.md: the "contested, see [Bug] Settled: create-bestax bundles the bestax-migrate skill; two docs still say otherwise #385" note becomes the settled policy, with the keep-in-agreement list (sync-skills.mjs, theCLAUDE_MDroster, the docs skills pages).create-bestax/CLAUDE.md: the per-skill bundling rule now cites [Bug] Settled: create-bestax bundles the bestax-migrate skill; two docs still say otherwise #385 as settled instead of open.Why
The code has shipped the skill since #345 (
sync-skills.mjsallowlist, the scaffoldedCLAUDE_MDroster,setupSkills()), while two docs pages still said it was deliberately not bundled. #385 asked for a direction decision; the decision is recorded on the issue: one uniform bundle, no per-skill carve-out to keep in sync, and the skill costs a fresh scaffold nothing.Not in this PR
sync-skills.mjsandsetupSkills()are untouched.pnpm genproduced no diff (the MCP index and skill catalog are unchanged), so there is no regeneration commit.Testing
pnpm allgreen locally. The first cold run tripped the known pre-existingbestax-mcpsync-skills race (ENOTEMPTYondata/skills/); the constituent tasks pass sequentially, as documented in feat(bulma-ui): scheme-aware bgColor backgrounds (scheme-main-bis/-ter bands without custom CSS) #508.git grep -ni "not bundled"over*.md/*.mdx: no skill-related hits remain.Closes #385
Summary by CodeRabbit
create-bestaxprojects.