Graduate Extensions from experiments - #1360
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am now reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
This change makes Extensions available to all users. It moves plugin controls from Settings into Extensions and keeps old links active.
Findings
I found three issues.
- A legacy plugin link can prevent later access to Settings. The app saves the old path, so the Settings link returns to Extensions.
- An unknown hash leaves a document-wide observer active. Live thread updates can then cause repeated element searches.
- A plugin with a skill starts a full skill search for one optional link. This search runs again after each component mount.
I added an inline comment for each issue.
Security and architecture
I found no security issue. The removed experiment controlled the user interface and did not control server access.
The change cleanly removes the duplicate plugin manager from Settings. I found no stale toolsHub reference outside historical migration tests.
One small refactor remains. Three changed files build the same plugin configuration URL with #configuration. A shared route helper can prevent drift.
Checks
- The affected Turbo type checks passed for the app, server, domain, and plugin SDK.
- The affected Turbo tests passed. The app completed 332 test files and 2,502 tests.
git diff --checkpassed.- The Extensions plugin list and plugin detail pages worked in the browser.
- The old plugin settings route opened and focused the new Configuration section.
- The browser reproduced the saved Settings destination defect.
698756d to
a0f7a6c
Compare
b78ffbc to
11ba428
Compare
11ba428 to
cc998fb
Compare
# Conflicts: # packages/templates/src/generated/plugin-sdk-dts.generated.ts
# Conflicts: # packages/templates/src/generated/plugin-sdk-dts.generated.ts
# Conflicts: # packages/templates/src/generated/plugin-sdk-dts.generated.ts # packages/templates/src/generated/templates.generated.ts
Moves plugin configuration into Settings, where configuration lives — and is the top layer of the native GitHub stack for the Extensions rework (#1358 → #1360 → #1471 → #1472 → #1473 → #1474 → here). - **/settings/plugins/:id** hosts each plugin's settings form, under a Plugins sidebar group where each row uses the plugin's own icon. - **Mirrored anatomies**: the plugin settings page mirrors the Extensions detail page's header (icon + title + one-line description) and section stack; its "Plugin details" section is one sentence linking across to the plugin page, and the detail page's "Configuration" section is one sentence linking here — each page owns its half and points at the other. - The per-plugin settings route participates in Settings route memory like any other section; only the bare /settings/plugins list remains a legacy redirect to Extensions. Bundle budget: this integration layer raises the compressed boot allowance from 435.3 KB to 438.0 KB to account for the persistent plugin rows added to Settings. The final measured boot payload is 436.2 KB Brotli; the raw boot payload remains within its existing limit. BB-Thread-ID: thr_kuixkqt8n2 > AGENT GENERATED: by GPT-5
## Summary Bumping the plugin SDK from 0.4.2 to 0.5.0 in #1360 unloaded every installed plugin whose manifest carried a caret range. Under semver `^0.4.1` stops at the next minor, so plugins that `bb plugin new` scaffolded went `incompatible` and stayed that way until each author shipped a manifest edit. On my install that was four plugins at once, including two third-party ones I do not control. The bump was a marker for the Extensions graduation, not a break. The only API change in #1360 was removing the `toolsHub` experiment key. - Read `engines.bbPluginSdk` as the API level a plugin needs, not a ceiling the host must honor. A range that fails only because the SDK moved forward inside the same major is accepted. A plugin asking for a newer SDK than this host provides, or pinned to a different major, stays incompatible. - Share one check across both enforcement sites: load-time in `plugin-runtime`, and install/update-time in `evaluateCompatibility` (which the catalog service also routes through). - Return the SDK to `0.4.3`, since #1360 was a compatible change. - Fix the policy comment in `plugin-sdk-version.ts`, which had the rule backwards. It said a break bumps the 0.x minor; that is what made the marker bump legal. The major is the compatibility number. - Move the scaffold, the 11 first-party plugins, and the three agent-facing surfaces (`bb-plugin-authoring`, `bb-cli`, `bb-guide-plugins`) to a floor range (`>=0.4.3`) so new plugins never inherit the problem. ## Known trade-off — read before merging The `0.5.0` to `0.4.3` revert is a version decrease, and it leaves **two official catalog plugins uninstallable from the Extensions store**: `thread-hover-cards` and `improve-prompt`. Both declare `^0.5.0`, whose floor is above the host SDK. Their registry entries are deliberately left unchanged. They mirror manifests pinned in `brsbl/bb-plugins`, so lowering only the entry would re-enable the install button and then fail later against the real remote manifest. A disabled button with an accurate reason is the better failure. The cross-repo fix is tracked in #1572. Already-installed copies are unaffected. This is a deliberate call: the floor rule alone would resolve the reported breakage without the revert, and shipping the revert anyway was chosen with this cost stated. No `HOST_DAEMON_PROTOCOL_VERSION` bump: `PLUGIN_SDK_VERSION` and `bbPluginSdk` do not appear in `apps/host-daemon` or `packages/host-daemon-contract`, so nothing on the wire changes. ## Testing - `pnpm exec turbo run test --filter=@bb/server -- --run plugin` — 307 passed - `pnpm exec turbo run test --filter=@bb/templates` — 23 passed - `pnpm exec turbo run test --filter=@bb/cli` — 420 passed - `pnpm exec turbo run typecheck` — `@bb/server`, `@bb/domain`, `@bb/templates`, `@bb/plugin-sdk`, `@bb/app`, `@bb/cli` - New `sdk-compat.test.ts` covers the regression directly: a caret range the running SDK has grown past, a floor above the running SDK, a different major, and a malformed range. Two existing scaffold assertions encoded the old behavior and are updated. In `plugin-scaffold-external.test.ts` the hardcoded `"0.5.0"` is now `PLUGIN_SDK_VERSION`, which turns a brittle literal into a real check that the packed package and the domain constant agree. ## Review SlopCop raised three findings. Two are fixed in dd20b03 (stale agent instructions; a `semver.validRange` guard so the shared helper stays total). The blocking third is answered above and tracked in #1572. 🤖 Generated with [Claude Code](https://claude.com/claude-code) > AGENT GENERATED: by Claude Opus 5 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Graduates Extensions out of the
toolsHubexperiment: every install now gets the Extensions surface, and the experiment-era forks are deleted rather than left to rot.toolsHubexperiment gate and the gated legacy Settings tabs it kept alive (−3.4k lines, almost all deletions of the pre-Extensions plugin/skill management UI).BB-Thread-ID: thr_kuixkqt8n2