Repository navigation
feat(cli): transactional integrate, launcher delegation, and bare state - #25
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 10 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe CLI now supports project-local delegation, bare project-state reporting, package-aware integration, transactional rollback, verification, and version alignment. Release automation now uses a guarded alpha publisher with npm 11.6.2 and protected ChangesCLI integration workflow
Guarded alpha package release
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant GlobalScribe
participant LocalScribe
participant Project
Developer->>GlobalScribe: run scribe integrate
GlobalScribe->>Project: resolve supported project and package manager
GlobalScribe->>LocalScribe: delegate when a project-local CLI exists
LocalScribe->>Project: plan and apply package and file changes
Project-->>LocalScribe: verification and rollback status
LocalScribe-->>Developer: integration result and exit status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
README.md (1)
24-32: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd the direct npm bootstrap command to
SKILL.md.The other docs now cover npm installation and the npm post-install path, but
SKILL.mdand the packagedSKILL.mdfiles still only showbunx@scribe-sdk/cli@alpha integratefor initial bootstrap. Usenpx@scribe-sdk/cli@alpha integratefor npm, with--dry-runfirst.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 24 - 32, Update the bootstrap documentation to show the npm equivalent using npx, with the --dry-run invocation before the apply invocation. Make this documentation change in README.md (anchor, lines 24-32), SKILL.md (lines 46-48), packages/cli/README.md (lines 24-32), packages/cli/SKILL.md (lines 46-48), packages/mdx/README.md (lines 24-32), packages/mdx/SKILL.md (lines 46-48), packages/react/README.md (lines 24-32), packages/react/SKILL.md (lines 46-48), packages/styles/README.md (lines 24-32), and packages/styles/SKILL.md (lines 46-48); retain the existing bunx guidance and clearly identify the npx commands for npm users.packages/cli/src/bare.ts (1)
32-34: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPass the existing
inspectionintointegratedOutputinstead of re-inspecting.
bareStateOutputalready computedinspectionon line 24.integratedOutputcallsinspectProject(root)a second time on line 57. That repeats the manifest and source scan on every bare invocation, and the two results can differ if files change between the reads. Lines 55 and 56 also import the same module twice.♻️ Proposed refactor
- if (integrated && runtimePresent) { - return integratedOutput(root, version); - } + if (integrated && runtimePresent) { + return integratedOutput(root, version, inspection); + }-async function integratedOutput(root: string, version: string): Promise<string> { - const { recommendStyleMode } = await import("./integrate.js"); - const { inspectProject } = await import("./integrate.js"); - const inspection = await inspectProject(root); - const mode = recommendStyleMode(inspection, undefined).mode; +async function integratedOutput( + root: string, + version: string, + inspection: Awaited<ReturnType<typeof import("./integrate.js").inspectProject>> +): Promise<string> { + const { recommendStyleMode } = await import("./integrate.js"); + const mode = recommendStyleMode(inspection, undefined).mode;Also applies to: 54-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/bare.ts` around lines 32 - 34, Update bareStateOutput and integratedOutput to pass the already computed inspection into integratedOutput, eliminating the second inspectProject(root) call while preserving the existing output behavior. Consolidate the duplicate module imports near integratedOutput so the inspection dependency is imported only once.scripts/test-portable-cli.mjs (1)
208-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific project-state string, not the substring "Scribe".
Both global bare runs execute in
directory, which line 116 already proves is a supported but unintegrated project. Every bare-output branch contains the substring "Scribe", including "No supported React project was found." and the inspection-failure message. The current assertion therefore passes even if project detection or delegation fails, while the failure message claims delegation was verified. Assert the same string used on line 116.♻️ Proposed refactor
- assert(npmGlobalBare.stdout.includes("Scribe"), "Global npm scribe did not delegate to print project state."); + assert( + npmGlobalBare.stdout.includes("Scribe is not integrated here."), + "Global npm scribe did not report the project integration state." + );- assert(bunGlobalBare.stdout.includes("Scribe"), "Global Bun scribe did not delegate to print project state."); + assert( + bunGlobalBare.stdout.includes("Scribe is not integrated here."), + "Global Bun scribe did not report the project integration state." + );Also applies to: 221-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-portable-cli.mjs` around lines 208 - 210, Update the stdout assertions for both global bare runs in the portable CLI tests to check for the specific supported-but-unintegrated project-state string already asserted near line 116, rather than the generic “Scribe” substring. Keep the existing status checks and failure-message intent while ensuring the assertions verify actual project-state delegation.packages/cli/src/launcher.ts (1)
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive delegation from
context.sourceinstead of a second comparison rule.
resolveExecutionContextnormalizes the local package root withrealpathbefore it classifies the source.shouldDelegatecomparescontext.localCli.packageRootwithcontext.packageRootas raw strings. The two rules can disagree. If a caller supplies a non-realpathpackageRoot, for example a symlinked global installation, thensourceis"project-local"whileshouldDelegatereturnstrue. The launcher then spawns a redundant child of itself. The recursion marker prevents an unbounded loop, but the extra process still runs.
resolveLocalClialready returns a realpathedpackageRoot, so therealpathImplcall on line 61 is also redundant.♻️ Proposed refactor
-export async function shouldDelegate(context: ExecutionContext): Promise<boolean> { - return !context.delegated - && context.projectRoot !== undefined - && context.localCli !== undefined - && context.localCli.packageRoot !== context.packageRoot; -} +export async function shouldDelegate(context: ExecutionContext): Promise<boolean> { + return context.source === "user-level" && context.localCli !== undefined; +}Also applies to: 80-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/launcher.ts` around lines 59 - 61, Update shouldDelegate to derive delegation directly from context.source, delegating only when the resolved source indicates a global installation rather than re-comparing package roots. Remove the redundant realpath call and raw package-root comparison while preserving the existing behavior for project-local and global contexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/bare.ts`:
- Around line 95-97: Update majorOf so it returns "unknown" when the normalized
specifier is empty, including inputs such as "*"; do not rely on the unreachable
nullish fallback after split. Preserve the existing extraction of the first
version segment for non-empty specifiers.
In `@packages/cli/src/integrate.ts`:
- Around line 209-212: Update the unsupported-package-manager fallback in
integrate to emit copyable, manager-specific install commands using each
reported package version, splitting runtime packages from `@scribe-sdk/cli` with
its --save-dev flag as supported integrations do; retain the warning and
manual-step context. Extend the relevant integration test to assert the exact
pnpm/Yarn command output and verify the development dependency remains separate.
Affected sites: packages/cli/src/integrate.ts lines 209-212 require the
command-generation change; packages/cli/src/integrate.test.ts lines 296-309
require coverage for the resulting behavior.
- Around line 343-346: Always pass plan.mode as stylesheetMode to
verifyIntegration in the integration flow, independent of installedSomething,
while retaining the conditional package list. In
packages/cli/src/integrate.test.ts lines 311-326, add a --yes scenario with
declared installed packages and a missing selected stylesheet, asserting
integration failure and rollback.
In `@packages/cli/src/package-manager.ts`:
- Around line 40-42: Update updateCommand in
packages/cli/src/package-manager.ts:40-42 to accept the expected version and
generate exact, manager-specific alignment commands for bun, npm, pnpm, and
yarn, using the correct package-manager syntax. Update the caller in
packages/cli/src/version-alignment.ts:30-40 to pass report.expected. Adjust
expectations in packages/cli/src/package-manager.test.ts:48-54 and
packages/cli/src/version-alignment.test.ts:26-40 to cover the pinned commands
for each manager.
In `@packages/cli/src/transaction.ts`:
- Around line 65-75: Update snapshotFiles() to treat only ENOENT read errors as
absent entries; rethrow all other errors so applyFileChanges() cannot proceed
without a valid snapshot. Add a regression test covering an existing unreadable
file and verifying the read error propagates before changes are applied.
In `@README.md`:
- Around line 177-181: Update runIntegrate so verifyIntegration receives
packages and stylesheetMode for supported-manager and idempotent plans even when
plan.commands is empty, then align the documented verification guarantee with
that behavior. Update README.md:177-181, packages/cli/README.md:177-181,
packages/mdx/README.md:177-181, packages/react/README.md:177-181, and
packages/styles/README.md:177-181 to describe the corrected checks; update
.changeset/tall-clis-delegate.md:5, SKILL.md:46-48, packages/cli/SKILL.md:46-48,
packages/mdx/SKILL.md:46-48, packages/react/SKILL.md:46-48, and
packages/styles/SKILL.md:46-48 to match verification behavior for unsupported
managers and no-command runs.
In `@scripts/publish-alpha-packages.mjs`:
- Around line 51-53: Update the package-publishing loop around
registry.publishTarball to fetch the published package’s dist-tags immediately
after each publish and assert that its latest tag remains unchanged before
proceeding to the next release. Preserve the existing failure behavior, and add
a regression test covering a first-package latest-tag change that verifies
subsequent packages are not published.
---
Nitpick comments:
In `@packages/cli/src/bare.ts`:
- Around line 32-34: Update bareStateOutput and integratedOutput to pass the
already computed inspection into integratedOutput, eliminating the second
inspectProject(root) call while preserving the existing output behavior.
Consolidate the duplicate module imports near integratedOutput so the inspection
dependency is imported only once.
In `@packages/cli/src/launcher.ts`:
- Around line 59-61: Update shouldDelegate to derive delegation directly from
context.source, delegating only when the resolved source indicates a global
installation rather than re-comparing package roots. Remove the redundant
realpath call and raw package-root comparison while preserving the existing
behavior for project-local and global contexts.
In `@README.md`:
- Around line 24-32: Update the bootstrap documentation to show the npm
equivalent using npx, with the --dry-run invocation before the apply invocation.
Make this documentation change in README.md (anchor, lines 24-32), SKILL.md
(lines 46-48), packages/cli/README.md (lines 24-32), packages/cli/SKILL.md
(lines 46-48), packages/mdx/README.md (lines 24-32), packages/mdx/SKILL.md
(lines 46-48), packages/react/README.md (lines 24-32), packages/react/SKILL.md
(lines 46-48), packages/styles/README.md (lines 24-32), and
packages/styles/SKILL.md (lines 46-48); retain the existing bunx guidance and
clearly identify the npx commands for npm users.
In `@scripts/test-portable-cli.mjs`:
- Around line 208-210: Update the stdout assertions for both global bare runs in
the portable CLI tests to check for the specific supported-but-unintegrated
project-state string already asserted near line 116, rather than the generic
“Scribe” substring. Keep the existing status checks and failure-message intent
while ensuring the assertions verify actual project-state delegation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d63a4adb-4724-437c-8c3a-778ba7323c93
📒 Files selected for processing (35)
.changeset/tall-clis-delegate.md.github/workflows/release.ymlREADME.mdRELEASING.mdSKILL.mdpackage.jsonpackages/cli/README.mdpackages/cli/SKILL.mdpackages/cli/src/bare.tspackages/cli/src/index.test.tspackages/cli/src/index.tspackages/cli/src/integrate.test.tspackages/cli/src/integrate.tspackages/cli/src/launcher.test.tspackages/cli/src/launcher.tspackages/cli/src/package-manager.test.tspackages/cli/src/package-manager.tspackages/cli/src/transaction.test.tspackages/cli/src/transaction.tspackages/cli/src/version-alignment.test.tspackages/cli/src/version-alignment.tspackages/mdx/README.mdpackages/mdx/SKILL.mdpackages/react/README.mdpackages/react/SKILL.mdpackages/styles/README.mdpackages/styles/SKILL.mdscripts/publish-alpha-packages.d.mtsscripts/publish-alpha-packages.mjsscripts/test-portable-cli.mjstests/release/changesets.test.tstests/release/ci.test.tstests/release/package-manifests.test.tstests/release/portability-scripts.test.tstests/release/publish-packages.test.ts
Add a user-level scribe launcher that delegates to the project-local CLI inside a supported project and runs directly elsewhere, print the project's Scribe state from a bare scribe invocation, and make scribe integrate own package installation as a reviewed transaction that snapshots and restores the manifest, lockfile, and source files on failure and verifies installed package versions, the selected stylesheet, and the reported files. pnpm and yarn installations defer to a precise manual step, and mismatched installed Scribe versions report the exact aligned update command. No-argument scribe exits 0; invalid usage stays 2.
collectSourceFiles walked into a project-local BUN_INSTALL cache (bun-global/install/cache/@scribe-sdk/*/dist/*.mjs), so a bun global install inside the project was misread as an integrated Scribe site. Resolve BUN_INSTALL (default ~/.bun) and skip that directory during source collection, with regression coverage for both the set and unset environments.
aetosdios27
force-pushed
the
agent/fail-closed-alpha-publishing
branch
from
August 5, 2026 17:24
e684220 to
f3acf0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/integrate.ts`:
- Around line 587-594: Update verificationPackageTargets to include every
declared Scribe package in plan.packages regardless of
installedScribePackageVersion results, while preserving version targets for
installed packages. Add the no-command integration test in
packages/cli/src/integrate.test.ts around lines 338-352 for a declared Scribe
package missing from node_modules, asserting exit code 1, rollback, and a “did
not resolve” diagnostic.
In `@packages/cli/src/package-manager.ts`:
- Around line 42-50: Update the npm branch in updateCommand to use npm install
with the pinned runtime packages, followed by a separate npm install --save-dev
command for the pinned `@scribe-sdk/cli` package. Preserve the existing bun, pnpm,
and yarn command behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f7fa596-795d-465b-b5a9-a5c84ad99e26
📒 Files selected for processing (24)
.changeset/tall-clis-delegate.mdREADME.mdSKILL.mdpackages/cli/README.mdpackages/cli/SKILL.mdpackages/cli/src/bare.tspackages/cli/src/integrate.test.tspackages/cli/src/integrate.tspackages/cli/src/launcher.tspackages/cli/src/package-manager.test.tspackages/cli/src/package-manager.tspackages/cli/src/transaction.test.tspackages/cli/src/transaction.tspackages/cli/src/version-alignment.test.tspackages/cli/src/version-alignment.tspackages/mdx/README.mdpackages/mdx/SKILL.mdpackages/react/README.mdpackages/react/SKILL.mdpackages/styles/README.mdpackages/styles/SKILL.mdscripts/publish-alpha-packages.mjsscripts/test-portable-cli.mjstests/release/publish-packages.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/mdx/SKILL.md
- scripts/test-portable-cli.mjs
- packages/cli/src/version-alignment.test.ts
- packages/cli/src/version-alignment.ts
- SKILL.md
- tests/release/publish-packages.test.ts
- packages/styles/SKILL.md
- packages/react/SKILL.md
- .changeset/tall-clis-delegate.md
- scripts/publish-alpha-packages.mjs
- packages/cli/SKILL.md
- packages/cli/src/transaction.test.ts
- packages/cli/src/package-manager.test.ts
- packages/cli/src/bare.ts
- packages/cli/src/transaction.ts
On macOS, /var is a symlink to /private/var, so process.cwd() (canonical) and BUN_INSTALL (as written) differ in form and the string prefix check missed a bun global-install cache inside the project. Resolve both paths through realpath before comparing.
This was referenced Aug 5, 2026
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.
Summary
Complete Scribe's CLI distribution and execution architecture, and land the guarded alpha publisher so the automated release can publish safely.
CLI distribution (feature)
scribedelegates to the project-local@scribe-sdk/cliinside a supported project and runs directly elsewhere; recursion is guarded with aSCRIBE_DELEGATEDmarker and the entry must resolve inside its package root.scribewith no arguments prints the project's Scribe state and exits0(invalid usage stays2).scribe integrate: owns package installation (bun/npm adapters) as a reviewed plan; snapshots and restores the manifest, lockfile, and source files on failure; verifies installed versions, the selected stylesheet, and reported files. pnpm/yarn defer to a precise manual step. Mismatched installed Scribe versions print the exact alignedupdatecommand.Release process (fail-closed publisher)
release:packagesnow asserts alpha prerelease mode, publishes inspected tarballs with--tag alphain styles→react→mdx→cli order, skips versions already on npm, and fails iflatestmoves. Workflow pinned to npm 11.6.2.Verification
bun run test: 287/287 passing (was 252)bun run typecheck,bun run docs:check,bun run build,bun run test:releaseall passbun run test:portabilityandbun run release:consumerspass locally against packed tarballsSummary by CodeRabbit
scribe integrateworkflow with dry-run previews, reviewed package/file changes, and project status reporting.