Document building a plugin without a running bb - #1006
Conversation
e3e53db to
5b6ccf0
Compare
b229964 to
135f2e0
Compare
b9bca4f to
07e78f4
Compare
07e78f4 to
71520e0
Compare
|
🚨 SLOP COP 🚨 · Performance review complete. I found no performance issue in #1006. The cache and cold-fetch descriptions match the implementation. |
|
🚨 SLOP COP 🚨 · Security review complete. I found one medium-severity issue in #1006. The CI advice can reuse an installed toolchain from an untrusted cache. I added an inline comment with a safe change. |
|
|
||
| `bb plugin build` talks to no server. Depending on `bb-app@X` builds with | ||
| exactly that release's shim configuration, so the bundle cannot be built | ||
| against a mismatched host runtime. Cache the toolchain directory in CI to skip |
There was a problem hiding this comment.
🚨 slopcop/review — Do not recommend this installed toolchain directory as a general CI cache. The resolver checks its marker and versions, but it does not verify each executable or JavaScript file. A shared cache that an untrusted job can write can run modified code in a later trusted build. Recommend the npm content cache, or state that only trusted jobs can restore and save this directory.
|
🚨 SLOP COP 🚨 · Code quality review complete. I found two medium-severity documentation conflicts and one low-severity cache description issue in #1006. The generated template matches its source. The architecture scan found no duplicate implementation that requires a refactor. |
| polling never resolves a dependency tree or builds. A candidate that fails to | ||
| build is reported as available and fails when you apply it. | ||
|
|
||
| bb ships no build toolchain. The first time a git or path plugin is built on |
There was a problem hiding this comment.
🚨 slopcop/review — This download is not one time for the machine. The cache key includes the pinned versions, so each pin change downloads a new set. Say “the first use of each pinned toolchain set.”
| replaced by the bundles bb builds. Path installs compile dist/ at install time | ||
| from dependencies you have already installed. A build failure fails the | ||
| install. npm packages must ship a metadata-validated prebuilt app or the | ||
| install is refused. The server rebuilds source-built apps after a bb upgrade. |
There was a problem hiding this comment.
🚨 slopcop/review — This sentence includes managed git builds, but the upgrade path only rebuilds path and builtin sources. State that limited scope. Otherwise, plugin authors can expect a managed git bundle to rebuild after a BB upgrade when it does not.
There was a problem hiding this comment.
🚨 slopcop/review — Follow-up: the rebuild also requires a plugin SDK version change. Document both conditions. Only mutable path and source-builtin applications rebuild; managed git and npm artifacts remain unchanged.
| installs and git installs without a prebuilt app build it automatically at | ||
| install time when their imported dependencies are already available. Git | ||
| plugins may instead ship a metadata-validated prebuilt app. | ||
| and git installs build it automatically at install time. Git installs also |
There was a problem hiding this comment.
🚨 slopcop/review — Update the builtin bb-cli skill with this install contract. Its plugin section still says only npm sources need npm. It also says git builds use existing dependencies and can use a prebuilt app. Agents can therefore receive the old rules after this pull request lands.
There was a problem hiding this comment.
🚨 slopcop/review — Follow-up: the same old contract also appears in the plugin-authoring “consumers never need npm” text, the scaffold README, and the configuration guide. The cold path-build documentation also omits its npm prerequisite and cache location. Update all user, agent, and scaffold surfaces, then regenerate the templates.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Result: I found no independent runtime defect in this documentation pull request.
The text depends on the toolchain cache from #1005. That cache needs the integrity fix reported on #1005.
Two small documentation gaps remain:
- The CI cache text does not give the standalone path,
~/.bb/plugins/toolchain-*. - The example hardcodes
bb-app@^0.35.1, which will become stale.
The generated template matches its source.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Code-quality phase complete.
Low: docs/configuration.md still says npm is necessary only for npm and git installs.
A first path plugin build can now fetch the toolchain through npm. The data-directory table also omits the new toolchain cache.
Update that configuration document with the new npm requirement and cache location.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Final review summary for PR #1006.
I found one low documentation issue.
docs/configuration.md omits the new toolchain cache. It also omits the npm requirement for a first path plugin build.
The remaining documentation and generated template changes match the full stack.
I submitted only comments. I did not approve or request changes.
bb-app is published and exposes the bb CLI, and bb plugin build makes no server calls, so a plugin author can already build in CI with a devDependency and a build script. That was undocumented. Also updates both surfaces for the two preceding layers: git installs now resolve third-party dependencies, and bb downloads its build toolchain on first use rather than shipping it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 633e7c9)
Drops the claim that a dependency-free git plugin installs with only git on PATH: git installs always run npm now, and a shipped artifact also fetches its build toolchain. Drops the claim that node_modules is discarded. States the check/apply split explicitly, since it is the reason polling for updates is cheap and does not touch the network beyond the clone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 135f2e0)
The generated guide is checked in and verified by generate-templates.mjs --check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Final review: three medium findings and one low finding remain in #1006.
- Medium security: the CI cache advice needs a trust condition.
The resolver does not verify each cached executable or JavaScript file. Recommend the npm cache, or limit the toolchain cache to trusted jobs.
- Medium quality: the new install contract conflicts with other agent, user, and scaffold text.
Update the bb-cli skill, plugin-authoring text, scaffold README, and configuration guide. State the cold path-build npm requirement and cache location.
- Medium quality: the upgrade text overstates automatic rebuilds.
Only mutable path and source-builtin applications rebuild. A plugin SDK version change must also occur. Managed git and npm artifacts remain unchanged.
- Low quality: the download happens once for each pinned toolchain set, not once for each machine.
The generated template matches its source. No duplicate implementation requires a refactor.
All GitHub checks passed. The focused Turbo typechecks and tests also passed.
I used a comment-only review. This rule forbids approval and request-changes actions.
fbff69e to
bc48683
Compare
**Layer 4 of 4.** Depends on #1006. Addresses the one P2 from macOS QA. A first install sat silent for **17.22 s** (vs **1.64 s** cached), because the first plugin build on a machine downloads the toolchain. Measured on macOS by `thr_3wmkakaaix`. ## Dialog `Installing plugin…` plus an indeterminate bar, replacing the full-trust warning — which has served its purpose by the time you've clicked install. It deliberately **does not name the phase.** The server does the work behind one blocking `POST`, so the client cannot know whether it's downloading tools, resolving dependencies, or bundling. Saying so would mean either guessing, or adding a realtime phase contract maintained forever for a window that happens once per machine. Reduced motion gets a static half-width fill rather than a frozen sweep. ## CLI `resolvePluginBuildToolchain` gains `onFetchDone(elapsedMs)`. `bb plugin build` and `bb plugin dev` resolve the toolchain in-process, so they now print the pinned versions on start and the elapsed time on completion. `bb plugin install` is **unchanged** — the server fetches on its behalf, so the CLI has no more visibility than the browser does. The server logs the duration instead. ## Verification - New `AddPluginDialog` test asserts the label, the bar, and the warning being replaced. Confirmed to **fail** with the label change reverted. - The gated fetch test now asserts both `onFetchStart` and `onFetchDone` fire on a real download. - Observed in the running app with the install response held open — label, `role="progressbar"`, and no trust warning. - Full-repo `turbo run typecheck --force`: 57/57, 0 errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Layer 3 of 3. Depends on #1005. Documentation only.
bb-appis published and exposes thebbbinary, andbb plugin buildmakes no server calls (apps/cli/src/commands/plugin.ts:697-730). So an author can already build in CI today:That was undocumented. Depending on
bb-app@Xbuilds with exactly that release's shim configuration, so a bundle cannot be built against a mismatched host runtime.Also updates both discoverable surfaces for the two layers below: git installs resolving third-party dependencies, and the on-demand toolchain download (including caching
<dataDir>/plugins/toolchain-*in CI).Surfaces:
packages/templates/src/templates/bb-guide-plugins.mdand thebb-plugin-authoringbuiltin skill.🤖 Generated with Claude Code