Skip to content

fix: resolve plugin market detail mix-up for same-named plugins - #23

Merged
BegoniaHe merged 2 commits into
masterfrom
sync/upstream-9852285bd
Sep 4, 2026
Merged

fix: resolve plugin market detail mix-up for same-named plugins#23
BegoniaHe merged 2 commits into
masterfrom
sync/upstream-9852285bd

Conversation

@BegoniaHe

@BegoniaHe BegoniaHe commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Market plugin details mixed up two registry entries that share the same metadata name but have different authors, repos, and versions. Navigation, list keys, random sampling, detail lookup, and install requests now use the 2026-06-27 plugin_id coordinate author/name.

Related issue

Related: AstrBotDevs#9925 (Fixes AstrBotDevs#9924)

Root cause

The Dashboard treated metadata name as a unique market identity. Two plugins can publish the same name, so the first match won for the detail route, Vue reused the wrong card instances, and random sampling collapsed the pair.

Reproduction

  1. Open the plugin marketplace with two entries that share name and differ by author.
  2. Open the second card.
  3. Before this change the detail page showed the first entry.

Implementation notes

Adapt of upstream 9852285bdafcbfd0078ae5a4c5f326a068a0dab7 onto this fork's TypeScript Dashboard. Unique-key helpers live in dashboard/src/utils/marketPluginKey.ts. Market navigation keeps the fork route ExtensionDetails with #market instead of ExtensionMarketDetails.

This fork does not copy upstream's market_plugin_id || repo || name fallback. repo is not plugin identity; name-only market deep links do not resolve. Ingest skips $meta and ignores source-emitted market_plugin_id. Repeatable route param /extension/:pluginId+ carries author/name without encoding the slash as %2F. Installed-tab routes still use the local plugin name (spec §16).

Validation

cd dashboard && pnpm exec vitest run tests/marketPluginKey.vitest.ts tests/extensionRuntimeSmokes.vitest.ts tests/coverageCloseout.vitest.ts
cd dashboard && pnpm typecheck
cd dashboard && pnpm exec eslint src/utils/marketPluginKey.ts src/router/MainRoutes.ts src/stores/common.ts src/types/extensions.ts src/views/extension/useExtensionPage.ts src/views/extension/MarketPluginsTab.vue src/views/extension/InstalledPluginsTab.vue src/views/ExtensionPage.vue tests/marketPluginKey.vitest.ts
cd dashboard && pnpm test

Compatibility and risk

Dashboard-only. Market detail URLs change from /extension/{name}#market to /extension/{author}/{name}#market. Old name-only market deep links no longer resolve (they pick neither entry). Installed-plugin URLs stay /extension/{name}#installed. New install requests send market_plugin_id. No OpenAPI, docs, or Python runtime change.

Checklist

  • The change is focused and does not include unrelated refactoring.
  • I added or updated a regression test, or explained why a test is not practical.
  • I ran the relevant formatting, lint, build, and test commands.
  • User-visible behavior updates both docs/zh/ and docs/en/ when needed.
  • OpenAPI, generated client, docs/public/openapi.json, and tests change together when routes or schemas change.
  • No secrets committed. Runtime Python deps update pyproject.toml, requirements.txt, and uv.lock together.
  • I did not restore legacy shims, Python <3.14 fallbacks, or upstream publish/docs URLs as fork artifacts.
  • Breaking API or behavior changes use ! and a BREAKING CHANGE: footer.
  • I will not merge this PR myself. Merge needs a human maintainer review plus a separate AI-assisted review (AI_POLICY.md).
  • AI use follows AI_POLICY.md. Keep exactly one author note below. Do not fabricate the other.

Agent note

Goal: absorb upstream AstrBotDevs#9925 without a direct cherry-pick, because this fork converted useExtensionPage.js to TypeScript and uses a unified ExtensionDetails route. Identity is author/name (fork market spec), not upstream's repo/name fallback. Paths: dashboard/src/utils/marketPluginKey.ts, dashboard/src/router/MainRoutes.ts, dashboard/src/stores/common.ts, dashboard/src/views/extension/useExtensionPage.ts, dashboard/src/views/extension/MarketPluginsTab.vue, dashboard/src/views/extension/InstalledPluginsTab.vue, dashboard/src/views/ExtensionPage.vue, dashboard/tests/marketPluginKey.vitest.ts, plus cursor files upstream-sync.yaml and upstream-decisions.jsonl. History was rewritten to one adapt commit (dd93de80f6add6f586f8cadbb4b0e74f08c5dd79) plus one cursor commit. Checks run: focused Vitest, pnpm typecheck, ESLint on touched files, and full pnpm test (86 Vitest files / 296 tests, plus Node tests). Residual risk: installed→market lookup is case-sensitive while StarMetadata.plugin_id lowercases; invalid sources that omit name on an author/name root key would double-prefix; records missing author cannot open market details. Tools: grok-4.6 via opencode, sync-upstream skill.

@BegoniaHe BegoniaHe self-assigned this Sep 4, 2026
@BegoniaHe

Copy link
Copy Markdown
Collaborator Author

AI-assisted review (project philosophy)

This is an independent AI-assisted review for the merge bar in AI_POLICY.md. GitHub rejected REQUEST_CHANGES because the review was submitted from the PR author account; treat the verdict below as changes requested, not approval. It does not merge.

Verdict: request changes. The final Dashboard identity (author/name) is the right fork design. The PR as currently written cannot be merged, because the absorb record and the PR body still describe the first commit’s upstream-shaped fallback (market_plugin_id || repo || name), which this fork’s market spec forbids.

I re-ran:

cd dashboard && pnpm exec vitest run tests/marketPluginKey.vitest.ts tests/extensionRuntimeSmokes.vitest.ts tests/coverageCloseout.vitest.ts

3 files / 17 tests passed. I did not re-run full pnpm test or pnpm typecheck in this review. CI was still pending for several jobs at review time.

What is aligned

  • Absorbing fix: resolve plugin market detail mix-up for same-named plugins AstrBotDevs/AstrBot#9925 as adapt (not a blind cherry-pick) is correct: this fork’s Dashboard is TypeScript and keeps ExtensionDetails + #market.
  • The third commit is the philosophy-correct identity:
    • docs/en/dev/plugin-market/2026-06-27.md defines plugin_id as metadata.author + "/" + metadata.name.
    • repo MUST NOT be used as plugin identity.
    • $meta MUST NOT be ingested as a PluginRecord.
    • market_plugin_id is a runtime request field and MUST NOT be taken from market source JSON.
  • That is the current path, not a compatibility shim. Copying upstream’s market_plugin_id || repo || name chain (commit 1) would have resurrected a weaker identity this fork already replaced. AGENTS.md: do not preserve shims; when touching an old/new split, build on the current path and remove the old one.
  • Sending market_plugin_id on market install matches spec §12. No OpenAPI change is required; the field already exists on the install payloads.
  • Installed-tab routes still using local name matches spec §16 (installed REST identity is the local name/path key). That split should stay, and should be described honestly.
  • Focused Vitest covers the same-name mix-up on author/name.

Blocking: absorb record does not match the code

.agents/skills/sync-upstream/SKILL.md requires one implementation commit per absorbed upstream SHA, and a cursor note that honestly describes the adaptation.

This PR has three commits:

  1. 9363a0cd — adapt of 9852285b using market_plugin_id || repo || name (upstream-shaped, conflicts with fork spec).
  2. 265bf181 — cursor/ledger update that records only (1).
  3. f8837058 — extra implementation commit that changes the identity to author/name, skips $meta, strips source-emitted market_plugin_id, widens /extension/:pluginId(.*), and sends install market_plugin_id.

Problems:

  • The ledger fork_adaptation and upstream-sync.yaml note still describe commit 1. After commit 3, that record is false.
  • Two implementation commits for one upstream SHA. The cursor commit sits between them and cannot describe the behavior that actually lands.
  • PR Summary / Implementation notes / Compatibility still say unique identity is market_plugin_id || repo || name, and that name-only deep links still resolve. Tests now assert resolveSelectedMarketPlugin(..., 'weather', 'market', null) is null. The Agent note still lists the commit-1 residual risks (normalizeRepoUrl, name-only deep links).

Please do one of:

  • Squash 1+3 into a single adapt commit whose message, ledger, and cursor note describe author/name (preferred; matches the skill), or
  • Keep the history, but update upstream-decisions.jsonl, upstream-sync.yaml, the PR body, and the Agent note so they describe the landed coordinate, name the extra commit, and drop the stale fallback claims.

Do not leave the durable sync record describing a design this fork rejected.

Code / test gaps to fix or explicitly accept

  1. getMarketPluginKey = getMarketPluginId is a leftover alias from commit 1. KISS: one name.
  2. Random sampling Map keys empty strings when author or name is missing, so incomplete records collapse onto ''. Skip empty keys, or do not insert them.
  3. Route param with /: :pluginId(.*) is necessary and PluginPageHost is registered first, which looks safe. Tests only assert router.push({ params: { pluginId: 'bob/weather' } }), not the resolved path. selectedPluginId takes pluginId[0] if the param is an array, which would truncate author/name to author. Add a route-resolution test, or stop treating the param as an array of path segments.
  4. Install market_plugin_id is the right spec behavior and is untested.
  5. Case: StarMetadata.plugin_id lowercases; Dashboard construction does not. Backend PluginService.get_market_plugin_id also does not. Not introduced here, but installed→market lookup is now identity-based and will miss case-only mismatches. Worth a sentence in residual risk; do not add an ad-hoc lowercase shim unless the spec/backend comparison is changed together.
  6. Name-key compatibility exception: ingest still uses the root key as name when name is omitted. Valid for slash-free name keys. If a source omits name on a author/name root key, the client would build author/author/name. Invalid sources only; mention as residual risk rather than adding a parser unless you already have such fixtures.

Docs

User-visible market detail URLs change from /extension/{name}#market to /extension/{author}/{name}#market. The protocol doc already defines plugin_id; no extra user guide appears to document the old URL. The breaking deep-link behavior must be stated in Compatibility (it is currently denied). I do not require a new docs page for this.

Security / runtime

Dashboard-only. No TLS, MCP, DOMPurify, or config-redaction changes. Catch-all is limited to /extension/:pluginId(.*) after the more specific plugin-page route. No secrets.

Merge bar

  • Human maintainer review is still required.
  • This comment is the AI-assisted review. It is not an approval.
  • Do not merge on CI green until the absorb record matches the landed identity.

Hayston1001 and others added 2 commits September 4, 2026 04:56
…BotDevs#9925)

Identify market plugins with author/name, keep ExtensionDetails + #market,
and skip $meta / source-emitted market_plugin_id during ingest.

Upstream-Commit: 9852285
Upstream-Author: Hayston <160645940+Hayston1001@users.noreply.github.com>
Upstream-PR: AstrBotDevs#9925
Sync-Disposition: adapt
Fork-Adaptation: Use the 2026-06-27 plugin_id coordinate (author/name) instead of upstream's market_plugin_id || repo || name fallback; implement helpers in dashboard/src/utils/marketPluginKey.ts; keep ExtensionDetails + #market; send market_plugin_id on install; add focused Vitest coverage.
Tested: cd dashboard && pnpm exec vitest run tests/marketPluginKey.vitest.ts tests/extensionRuntimeSmokes.vitest.ts tests/coverageCloseout.vitest.ts && pnpm typecheck && pnpm test
AI-Generated: true
Generated-At: 2026-09-04T02:55:58Z
Record the adapt decision for AstrBotDevs#9925 and advance the upstream
cursor to 9852285.

AI-Generated: true
Generated-At: 2026-09-04T02:56:28Z
@BegoniaHe
BegoniaHe force-pushed the sync/upstream-9852285bd branch from f883705 to 3fd393c Compare September 4, 2026 02:57
@BegoniaHe

Copy link
Copy Markdown
Collaborator Author

Addressed the AI-assisted review:

  • Squashed the two implementation commits into one adapt (dd93de80f6add6f586f8cadbb4b0e74f08c5dd79) plus a cursor commit. Ledger, upstream-sync.yaml, and this PR body now describe the landed author/name coordinate. Name-only market deep links are documented as non-resolving.
  • Removed the getMarketPluginKey alias. Random sampling skips empty ids. /extension/:pluginId+ plus toRoutePluginIdParam / readRoutePluginId keep author/name as path segments instead of %2F. marketPluginIdFields covers install market_plugin_id in Vitest.

Human maintainer review is still required. This is not an approval or merge.

@BegoniaHe
BegoniaHe merged commit ef6f600 into master Sep 4, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 插件市场中两个 metadata name 相同的插件,详情页信息互相错乱

2 participants