fix: never create the first marketplace repo from a change signal - #6013
fix: never create the first marketplace repo from a change signal#6013adaam2 wants to merge 3 commits into
Conversation
…ignal Addresses review on #6009. An enabled MCP server with no endpoint is not a Default-plugin member, so renaming one in a project that has never published took the first-publish path and created an empty marketplace repository (plus an API key for it). A toolset that was MCP-enabled before the attach-on-enable behaviour existed had the same problem. Rather than a membership lookup on each mutation path, the guard goes at the publish boundary: PublishProjectInput.SkipIfUnpublished short- circuits when the project has no github connection yet, and every change-driven signal sets it. A signal can now update a marketplace but never bring one into existence. Creating the repo stays with the paths that mean it — project creation and a first Default-plugin attach (both forced), and the dashboard Publish button — plus the rollout sweep, whose candidates are already scoped to projects with a Default plugin or a previous publish. Claude-Session: https://claude.ai/code/session_011G5NTsW2Qbm31ET9oRSMS9
|
|
Running ultrareview automatically — This changes the cross-cutting marketplace publish boundary and workflow flags that govern repository creation and API-key issuance across signals, forced paths, and rollout recovery, so a subtle gating or debouncing bug could suppress required publishes or create unintended repositories.. I'll post findings when complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 10m 9s
All reported issues were addressed across 5 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Addresses review on #6013. The guard flag is inverted: SkipIfUnpublished becomes AllowFirstPublish, so false is the safe value. A workflow params payload encoded before the field existed decodes as false, and so does a call site that forgets it — both now defer the repo to the rollout sweep instead of creating one a change signal never intended. That also covers the two Platform MCP mutation callbacks, which omitted the flag entirely; they are explicit now. TriggerPluginPublish takes allowFirstPublish separately from force. A legacy project whose Default plugin was lazily provisioned by a dashboard read reaches its first attach with pluginCreated false, and would otherwise have waited out the sweep for its marketplace: an attach may now create the repo without also forcing a fingerprint-blind republish. The connection lookup moves above resolvePluginInfos and generateConfig, so a signal for an unpublished project costs one query rather than a full in-memory generate it throws away. The skip test also asserts no API key is minted, and the workflow tests assert both flags reach the activity unchanged. Six existing PublishProject tests publish a project for the first time through the automated publisher and now opt in explicitly, which is the inversion working as intended. Claude-Session: https://claude.ai/code/session_011G5NTsW2Qbm31ET9oRSMS9
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Addresses review on #6013. The service helper passed allowFirstPublish as a hardcoded true, so a mutation that only edits an existing member — renaming an MCP-enabled toolset that has no membership, or a server already attached to Default — could still hand an unpublished project its first marketplace repo and an API key for it. triggerPluginPublish now takes publish and attached separately: publish says the generated output may have moved, attached says this request actually ran the attach, and only the latter licenses creating the marketplace. On the toolsets update path only the disabled-to-enabled transition attaches, so only it passes attached. Claude-Session: https://claude.ai/code/session_011G5NTsW2Qbm31ET9oRSMS9
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/internal/mcpendpoints/impl.go">
<violation number="1" location="server/internal/mcpendpoints/impl.go:229">
P1: When the server is already attached and the project has no GitHub connection, this line passes `attached=true` as `AllowFirstPublish` even though no membership was added, allowing a second endpoint to create the first marketplace repo. Return and propagate the actual attach result from `attachToDefaultPlugin` instead of unconditionally returning `true`.
(Based on your team's feedback about first-repo gating on real attaches.)</violation>
</file>
<file name="server/internal/toolsets/impl.go">
<violation number="1" location="server/internal/toolsets/impl.go:630">
P2: When an already-attached toolset is disabled and then re-enabled in an unpublished project, this line authorizes first-repo creation even though the attach helper no-ops on the existing membership. Return and pass the actual attachment result instead of the enabled-state transition.</violation>
</file>
<file name="server/internal/mcpservers/impl.go">
<violation number="1" location="server/internal/mcpservers/impl.go:758">
P2: When a disabled MCP server is already attached to the Default plugin, this line passes `allowFirstPublish=true` even though the attach operation no-ops. A project without a GitHub connection can therefore get its first marketplace repo from re-enabling that server; pass the actual insertion result from `attachToDefaultPlugin` (and the attach API) instead of the unconditional endpoint-path flag.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| } | ||
|
|
||
| s.triggerPluginPublish(ctx, authCtx, attached, pluginCreated) | ||
| s.triggerPluginPublish(ctx, authCtx, attached, attached, pluginCreated) |
There was a problem hiding this comment.
P1: When the server is already attached and the project has no GitHub connection, this line passes attached=true as AllowFirstPublish even though no membership was added, allowing a second endpoint to create the first marketplace repo. Return and propagate the actual attach result from attachToDefaultPlugin instead of unconditionally returning true.
(Based on your team's feedback about first-repo gating on real attaches.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/mcpendpoints/impl.go, line 229:
<comment>When the server is already attached and the project has no GitHub connection, this line passes `attached=true` as `AllowFirstPublish` even though no membership was added, allowing a second endpoint to create the first marketplace repo. Return and propagate the actual attach result from `attachToDefaultPlugin` instead of unconditionally returning `true`.
(Based on your team's feedback about first-repo gating on real attaches.) </comment>
<file context>
@@ -226,7 +226,7 @@ func (s *Service) CreateMcpEndpoint(ctx context.Context, payload *gen.CreateMcpE
}
- s.triggerPluginPublish(ctx, authCtx, attached, pluginCreated)
+ s.triggerPluginPublish(ctx, authCtx, attached, attached, pluginCreated)
return mv.BuildMcpEndpointView(created), nil
</file context>
There was a problem hiding this comment.
Not making this change — the insert/no-op distinction does not change the outcome here.
attachToPlugin no-ops precisely when GetPluginServerByBackend finds a live plugin_servers row (deleted IS FALSE). So "the attach no-opped" means the server or toolset is already a live member of the Default plugin: the project has plugin content, and the repo this creates is a populated marketplace, not the empty one this PR is about. That project is also already in the rollout sweep's candidate population (projects with a Default plugin), so the same repo appears within the hour regardless — creating it on the attach path just makes it prompt.
The hole that mattered was a mutation where no attach runs at all (a rename or a disable), because nothing there proves any membership exists; 46fe5a4 closes that. Threading an inserted-vs-no-op boolean back through AttachToDefaultPluginAudited and the three services would add plumbing without changing behaviour.
| // so only it may create the project's first marketplace. | ||
| s.triggerPluginPublish(ctx, authCtx, | ||
| existingToolset.McpEnabled || updatedToolset.McpEnabled, | ||
| !existingToolset.McpEnabled && updatedToolset.McpEnabled, |
There was a problem hiding this comment.
P2: When an already-attached toolset is disabled and then re-enabled in an unpublished project, this line authorizes first-repo creation even though the attach helper no-ops on the existing membership. Return and pass the actual attachment result instead of the enabled-state transition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/toolsets/impl.go, line 630:
<comment>When an already-attached toolset is disabled and then re-enabled in an unpublished project, this line authorizes first-repo creation even though the attach helper no-ops on the existing membership. Return and pass the actual attachment result instead of the enabled-state transition.</comment>
<file context>
@@ -620,8 +623,12 @@ func (s *Service) UpdateToolset(ctx context.Context, payload *gen.UpdateToolsetP
+ // so only it may create the project's first marketplace.
+ s.triggerPluginPublish(ctx, authCtx,
+ existingToolset.McpEnabled || updatedToolset.McpEnabled,
+ !existingToolset.McpEnabled && updatedToolset.McpEnabled,
+ pluginCreated)
</file context>
There was a problem hiding this comment.
Not making this change — the insert/no-op distinction does not change the outcome here.
attachToPlugin no-ops precisely when GetPluginServerByBackend finds a live plugin_servers row (deleted IS FALSE). So "the attach no-opped" means the server or toolset is already a live member of the Default plugin: the project has plugin content, and the repo this creates is a populated marketplace, not the empty one this PR is about. That project is also already in the rollout sweep's candidate population (projects with a Default plugin), so the same repo appears within the hour regardless — creating it on the attach path just makes it prompt.
The hole that mattered was a mutation where no attach runs at all (a rename or a disable), because nothing there proves any membership exists; 46fe5a4 closes that. Threading an inserted-vs-no-op boolean back through AttachToDefaultPluginAudited and the three services would add plumbing without changing behaviour.
| // enable transition this block attaches. A server disabled before and | ||
| // after contributes nothing either way and stays silent. | ||
| s.triggerPluginPublish(ctx, authCtx, attached || existing.Visibility != VisibilityDisabled, pluginCreated) | ||
| s.triggerPluginPublish(ctx, authCtx, attached || existing.Visibility != VisibilityDisabled, attached, pluginCreated) |
There was a problem hiding this comment.
P2: When a disabled MCP server is already attached to the Default plugin, this line passes allowFirstPublish=true even though the attach operation no-ops. A project without a GitHub connection can therefore get its first marketplace repo from re-enabling that server; pass the actual insertion result from attachToDefaultPlugin (and the attach API) instead of the unconditional endpoint-path flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/mcpservers/impl.go, line 758:
<comment>When a disabled MCP server is already attached to the Default plugin, this line passes `allowFirstPublish=true` even though the attach operation no-ops. A project without a GitHub connection can therefore get its first marketplace repo from re-enabling that server; pass the actual insertion result from `attachToDefaultPlugin` (and the attach API) instead of the unconditional endpoint-path flag.</comment>
<file context>
@@ -755,7 +755,7 @@ func (s *Service) UpdateMcpServer(ctx context.Context, payload *gen.UpdateMcpSer
// enable transition this block attaches. A server disabled before and
// after contributes nothing either way and stays silent.
- s.triggerPluginPublish(ctx, authCtx, attached || existing.Visibility != VisibilityDisabled, pluginCreated)
+ s.triggerPluginPublish(ctx, authCtx, attached || existing.Visibility != VisibilityDisabled, attached, pluginCreated)
if err := s.reconcileMcpServerCustomDomains(ctx, clearedRootDomainIDs); err != nil {
return nil, err
</file context>
There was a problem hiding this comment.
Not making this change — the insert/no-op distinction does not change the outcome here.
attachToPlugin no-ops precisely when GetPluginServerByBackend finds a live plugin_servers row (deleted IS FALSE). So "the attach no-opped" means the server or toolset is already a live member of the Default plugin: the project has plugin content, and the repo this creates is a populated marketplace, not the empty one this PR is about. That project is also already in the rollout sweep's candidate population (projects with a Default plugin), so the same repo appears within the hour regardless — creating it on the attach path just makes it prompt.
The hole that mattered was a mutation where no attach runs at all (a rename or a disable), because nothing there proves any membership exists; 46fe5a4 closes that. Threading an inserted-vs-no-op boolean back through AttachToDefaultPluginAudited and the three services would add plumbing without changing behaviour.
1 participant
Follow-up to #6009, which was already in the merge queue when this came out of review.
Summary
Adds
PublishProjectInput.SkipIfUnpublished: the publish short-circuits when the project has noplugin_github_connectionsrow yet. Every change-driven signal sets it, so a signal can update a marketplace but never bring one into existence.Creating the repo stays with the paths that mean it — project creation and a first Default-plugin attach (both forced), the dashboard Publish button, and the rollout sweep, whose candidate list is already scoped to projects with a Default plugin or a previous publish.
Motivation
publishProjecttreats a project with no connection asfirstPublish, which ignoresSkipIfUnchangedand creates the repository. #6009 gates each mutation path on whether the server or toolset could contribute generated output, but "could contribute" is not the same as "is a plugin member": an enabled MCP server with no endpoint is never attached, so renaming one in a never-published project would create an empty marketplace repo and mint an API key for it. An MCP-enabled toolset that predates attach-on-enable has the same shape.Gating at the publish boundary fixes the whole class in one place, rather than adding a membership lookup to each mutation path and leaving the next new callsite to rediscover the trap.
https://claude.ai/code/session_011G5NTsW2Qbm31ET9oRSMS9
Summary by cubic
Fixes change-driven plugin publishes so they can update an existing marketplace repo but never create the first one. A project with no
plugin_github_connectionsrow previously took the first-publish path on signals like renaming an enabled MCP server with no endpoint, creating an empty repo and minting an API key; those publishes now short-circuit.Behavior
PublishProjectInput.AllowFirstPublish, opt-in and false by default, so a change signal updates an existing marketplace but never creates one.TriggerPluginPublishtakesallowFirstPublishseparately fromforce, so a first attach can create the repo without a fingerprint-blind republish.triggerPluginPublishnow takespublishandattachedseparately: only an actual Default-plugin attach may create the repo, so editing an existing member never does.Written for commit 46fe5a4. Summary will update on new commits.