feat(market): unified plugin backend migration + market UI restructure

[l-s-c]

Summary

Migrates the octo-web marketplace frontend (skills, MCP connectors, experts/squads) off the legacy per-type catalog endpoints onto the octo-marketplace unified plugin backend (/plugins, /plugins/detail, /plugin_categories, /plugins/upsert|publish), and restructures the market UI on top of it. This branch supersedes and closes #1532 (its unified-plugin migration is folded in here) and continues with the UI/UX work.

The backend counterpart is the unified-plugin work in octo-marketplace: the initial surface landed in #67 (merged), and the 2.0 contract this branch actually speaks (2.0 $schema ids, the skill-edit version field, connector default-scene placement) landed in #72 (merged). This branch must not be released until #72 is deployed (or shipped atomically) — see the deploy-coupling section below.

Related Issue

• Supersedes / replaces feat(market): migrate marketplace API layer to the unified plugin backend #1532 (feat(market): migrate marketplace API layer to the unified plugin backend).
• Frontend half of the unified-plugin work; backend: Mininglamp-OSS/octo-marketplace#67 + feat(oidc): bind self-service flow + login page UX revamp #72 (both merged; feat(oidc): bind self-service flow + login page UX revamp #72 carries the 2.0 contract this branch speaks and must be deployed before release).

Changes

• Unified API layer: dmworkmcp + dmworkskillmarket now read/write the unified plugin surface (pluginWire, mcpWireParams, expertWire, mcpService, skillApiReal, expertService). current_version, visibility (system/space/private), dynamic categories from /plugin_categories.
• Forward-to-Bot everywhere: skill "添加到 Bot", connector "添加到 Bot" (McpConnectModal + mcpConnectPrompt), skill "Bot 上架", MCP 上架, expert/squad 上架 all render the shared PromptForwardModal (copy / pick an owned Bot / forward into its DM). Secrets are never posted from the web — the Bot authors via octo-cli.
• Market UI restructure: 我的发布 horizontal row cards (avatar + content + 编辑/删除); discovery sort reduced to 最新/最热 (default 最新); removed the "共 N 个" total and detail view/download counters; card primary action moved bottom-right; sidebar order 技能 → 连接器 → 专家 → 我的发布 with a divider above 我的发布; expert page tabs moved to the top; page titles de-prefixed, subtitles dropped.
• Expert/squad create+edit use the Bot flow (ExpertBotPublishModal); skill edit uses EditSkillModal. The earlier full-page editors were removed as dead code (see below).
• e2e: market MSW handlers migrated to /plugins; SK1/SK2/SK4/EX1/EX2/EX4 specs + case-specs updated to drop the intentionally-removed "共 N 个" total.

Architecture / Module Boundary

• Affected module(s): packages/dmworkmcp, packages/dmworkskillmarket, packages/dmworkbase (fetch-telemetry rules only).
• New or changed user-visible entry point: no new entry point — the existing /mcp-market/* pages are re-skinned and re-wired onto the unified backend.
• Shared layer touched: yes — beyond the FetchRules telemetry, the exported @octo/base PromptForwardActions changed its public props (removed preview, added kind / copyTrackEvent) and PromptForwardModal.title became optional; both are consumed only in-repo and updated in this diff. Added 9 promptForward.* i18n keys + TrackRules/DAP_EVENTS telemetry entries.
• Test-harness change (disclosed, not marketplace-functional): dmworkbase/src/__tests__/setup.ts extends the jsdom ProseMirror geometry stub (getClientRects/getBoundingClientRect on Range + Text) to fix an intermittent unit-CI flake in the chat-composer teardown.
• Read-path behaviour change: a connector's shared (non-user-supplied) env/header/query whose key name is secret-shaped now renders as a fillable ${KEY} placeholder on the detail snippet (no author opt-out), since the unified backend blanks nothing on read.
• If shared code changed, impact scope: PromptForwardActions/PromptForwardModal consumers are all in this repo (the five forward-to-Bot surfaces) and updated here; the setup.ts stub is test-env only.
• Duplicate entry point checked: yes — no duplicate market entry points added.

Testing

• pnpm --filter @dmwork/skillmarket test — 136 pass

• pnpm --filter @dmwork/mcp test — 253 pass

• pnpm i18n:check — pass (locale keys balanced, 0 candidates over baseline)

• npx stylelint (both index.css) — 0 errors

• pnpm build — pass

• e2e market specs (SK/EX/C37) + MSW handlers updated for the unified surface; assertions on the removed total dropped.

• Unit tests added/updated

• Manually verified (dev + local marketplace)

Checklist

• I have read CONTRIBUTING.md
• PR description is in English
• Added tests for my changes
• Updated documentation
• Ran pnpm i18n:check
• Confirmed the change follows module ownership and does not add duplicate user-visible entry points
• Described impact scope when shared components, messages, bridge, or services are changed
• Followed commit message conventions (Conventional Commits)

Linked Spec

• Backend spec/brief: octo-marketplace .octospec/tasks/unified-plugin-backend/brief.md; backend PR Mininglamp-OSS/octo-marketplace#67 (merged).
• No octo-web .octospec/tasks brief — the frontend contract is defined by the backend brief above.

COMPREHENSION

1. What does this change actually do to the load-bearing path?
   All three marketplaces stop calling the legacy per-type catalog endpoints and instead call the unified /plugins* surface. Reads map PluginListItemWire/PluginDetailWire → the existing McpListItem/Skill/Expert view types (visibility system/space/private, current_version, dynamic categories). Writes build a single /plugins/upsert body (+ optional /plugins/publish placement). Create/edit of experts & squads is delegated to a Bot via a generated prompt (PromptForwardModal) rather than a direct write; secret values are never posted from the web.

2. What could break because of it (dependents + failure mode)?
   Dependents are the market pages (McpMarketListPage, SkillListPage, ExpertMarketListPage, MyAssetsPage) and the create/edit modals. Failure modes guarded here: (a) the connector mcp.json mcpServers key must be the ASCII slug, not the display name — a regression of the historical feat(market): migrate marketplace API layer to the unified plugin backend #1532 P0 was found and fixed, with regression tests (slug key + CJK-name fallback); (b) the forward-to-Bot prompt must resolve spaceId with the localStorage fallback the rest of the package uses (fixed for InstallPromptModal); (c) category/visibility mapping is fail-closed on the write path.

3. How do you know it works (specific test/repro/trace)?
   @dmwork/mcp (253) and @dmwork/skillmarket (136) unit suites cover the wire mappers, secret round-trip, category fail-close, sort mapping, and the new slug-key + connect-prompt contracts; i18n:check, stylelint, and pnpm build are green; the market e2e specs + MSW handlers exercise list/detail/search/pagination/official-publisher against the unified surface. A pre-PR review (4 read-only passes + a targeted deletion pass) fixed 1 P0 (slug key), 1 P1 (spaceId fallback), the e2e total assertions, and removed ~3,600 lines of unreachable editor code.

---

⚠️ Cross-repo deploy coupling (added after review)

This PR speaks the octo-marketplace #72 unified protocol. It must ship in
lockstep with that backend PR; against the currently-deployed backend the
write paths fail. Disclosed explicitly per reviewer request:

• Depends on octo-marketplace#72 (merge + deploy first, or atomically).
• Write paths emit the 2.0 $schema ids (cowork-plugin-*-2.0.json); a
  pre-2.0 backend hard-rejects them (400).
• Skill metadata edit sends an optional version on /plugins/upsert; the
  field only exists on the feat(oidc): bind self-service flow + login page UX revamp #72 backend (strict decoder 400s otherwise).
• Connector create is a single upsert with no publish step — feat(oidc): bind self-service flow + login page UX revamp #72's create
  auto-attaches the default-scene placement and treats the save as a version
  snapshot. On a pre-feat(oidc): bind self-service flow + login page UX revamp #72 backend the new connector has no placement and is
  invisible in every scene-scoped list.
• The forward-to-Bot flow includes an editable prompt (draft + 编辑提示词);
  this is intentional and shared across the five forward surfaces.

No client-side dual-version tolerance is added — the deploy is coordinated, not
gradual.

[yujiawei]

Code Review — PR #1584 (octo-web)

Reviewed at head a879f3dad, merge-base 797243d9f (110 files, +6331/−2058).

The delta since the previously reviewed head 14781cb9b is one substantive commit — 9c3f4ef68, "fix(mcp): rebuild redacted mcp.json from a whitelist (close fail-open class)" (2 files, +140/−81) — plus a879f3dad, a CI retrigger whose tree is byte-identical to its parent (git rev-parse 9c3f4ef68^{tree} == a879f3dad^{tree} → 99912d01b). Every claim below was re-derived at this head; nothing is carried forward on trust, and the security-relevant functions were verified by executing them, not by reading them.

Evidence gathered on this head

Check	Result
CI	fully green — Build ✅, install-build ✅, Unit tests ✅, e2e-p0 ✅, secret-scan ✅, osv-scan ✅, dependency-review ✅, history ✅
pnpm --filter @dmwork/mcp test (local)	245/245 pass (22 files)
pnpm --filter @dmwork/skillmarket test (local)	134/134 pass (14 files)
pnpm i18n:check (local)	pass — "0 candidates within baseline; locale keys healthy"
Backend contract gate	octo-marketplace#72 merged 2026-08-31T10:09:53Z; backend main = f9216835. The 2.0 $schema ids this branch emits are present in internal/service/plugin/schema.go. Gate discharged.

---

1. Specification compliance

Spec: ✅

• Missing work: none. The delta implements exactly the hardening the previous round asked for — the whitelist rebuild — and the rest of the migration is unchanged and was cleared previously.
• Extra work (scope creep): none. The commit touches one utility and its test.
• Deviation: none, but one intentional posture change is worth recording because it is not in the PR description: redactMcpConfig previously returned null (→ localized "unavailable" notice) for an unmodeled key, and now silently drops it. That is the correct trade for the security goal and the docblock was updated in the same commit to describe it, so it is documented — but it has a user-visible consequence, filed as P2-3 below.

The prior round's blocker is genuinely fixed, and the new tests are not vacuous. I mutation-tested them:

Mutation applied to redactMcpConfig.ts	Result
re-serialize the parsed doc ({...doc, mcpServers}) instead of a fresh object	2 tests fail — "DROPS a root sibling key", "DROPS a VS Code top-level inputs array"
seed redactServer with {...s} instead of {}	2 tests fail — "DROPS an unmodeled server key…", "DROPS a non-string url and a non-array args"

and the vacuous assertion flagged last round (redactMcpConfig.test.ts:120, which passed only because mcpServers was absent) has been replaced with a genuine root-sibling case. Executed against the real function at this head:

{"mcpServers":{"safe":{"command":"npx"}},"secrets":{"token":"sk-live-ROOTSIBLING"}}
  → {"mcpServers":{"safe":{"command":"npx"}}}                              ✅ sibling dropped
{"inputs":[{"id":"tok","password":true,"default":"sk-live-VSCODEINPUT"}],…}
  → {"mcpServers":{"safe":{"command":"npx","args":["-y","pkg"]}}}          ✅ VS Code inputs dropped
{"mcpServers":{"x":{"url":{"href":"…sk-live-OBJURL"},"args":{…}}}}
  → {"mcpServers":{"x":{}}}                                                ✅ wrong-typed dropped
{"mcpServers":{"x":{"env":{"A":{"b":"sk-live-NESTED"}}}}}
  → {"mcpServers":{"x":{"env":{"A":"••••••"}}}}                            ✅ non-string env masked
{"mcpServers":{"x":{"autoApprove":["tool1",{"token":"sk-live-INARR"}]}}}
  → {"mcpServers":{"x":{"autoApprove":["tool1"]}}}                         ✅ object member filtered

The whole "an un-walked key survives verbatim" class is gone. That was the architectural defect and this is the right fix.

---

2. Code quality

Quality: Changes-Requested

One theme blocks: this PR converts the marketplace write paths to full-replace upserts, but only one of the three paths echoes the state it is replacing. The other two silently discard it. Both items below are narrow and each can be closed in one round by either a small fix or a one-sentence confirmation from the backend owner.

🔴 P1-1 — A connector metadata edit rebuilds mcp.json from a 6-field model, destroying every stored field outside it

packages/dmworkmcp/src/api/mcpWireParams.ts:79-111, packages/dmworkmcp/src/api/mcpService.ts:817-856

The update is a documented full replace (mcpService.ts:817 — "Full-replace update via upsert"), and the body is built from scratch:

const server: Record<string, unknown> = {};
if (params.transport) server.type = params.transport;
if (params.url) server.url = params.url;
if (params.command) server.command = params.command;
if (params.args?.length) server.args = params.args;
if (env) server.env = env;
if (headers) server.headers = headers;
…
const attachments: PluginAttachmentBody[] = [
  rawAtt("mcp.json", goCanonicalJSON({ mcpServers: { [serverKey]: server } })),
  rawAtt("connector/tools.json", …), rawAtt("connector/examples.json", …),
  rawAtt("connector/faqs.json", …), rawAtt("connector/notes.json", …),
];

and the read side only ever recovers those same six fields, from the first server entry only (mcpService.ts:634-655):

const serverName = Object.keys(servers)[0] ?? "";
const server = servers[serverName] ?? {};
…
transport: server.type ?? "stdio", url: server.url, command: server.command,
args: server.args, env: …, headers: …,

So anything else the stored mcp.json carries — cwd, disabled, timeout, autoApprove, alwaysAllow, a second mcpServers entry, or any sixth attachment — is silently destroyed the moment a user edits an unrelated metadata field such as the slogan. There is no error, no warning, and the save reports success.

Three things make this reachable rather than theoretical:

1. Connectors are designed to be authored outside this form. The whole forward-to-Bot flow exists so a Bot writes the connector via octo-cli with real secrets; the web never posts them. A Bot-authored mcp.json is whatever the author's real config contained.
2. There is no gate on the edit entry point. packages/dmworkmcp/src/pages/McpMarketListPage.tsx:847 wires onEdit: () => this.handleEditFromCard(item) for every 我的发布 row unconditionally; grep -n "createdByType" McpMarketListPage.tsx McpCreateModal.tsx finds no guard on the edit path. A CLI-/bot-/import-created connector is editable through this lossy round-trip.
3. The same PR takes the opposite approach for skills, and says why. packages/dmworkskillmarket/src/api/skillApiReal.ts:605-607 echoes the stored package through, filtering only manifest.json, precisely so a full replace does not destroy it. Same feature, same author, opposite posture — and unlike the skill path, the connector rebuild carries no comment justifying the loss as intentional.

A second, independent analysis pass reached this same finding from the same two call sites without prompting, and added a concrete symptom worth quoting: a Cline connector stored with disabled: true is rewritten as enabled configuration after an unrelated metadata edit.

Discharge condition — either is fine, and both are cheap:

• Echo unknown mcpServers keys and non-modeled attachments through on update, mirroring skillApiReal.ts:605-607; or
• Confirm in writing that octo-cli / the backfill never writes a connector mcp.json containing a field outside {type,url,command,args,env,headers} or a second server entry, and that /plugins/upsert merges rather than replaces plugin_json.attachments.

I want to be precise about what I did and did not establish: the mechanism and the reachability are proven from this diff. Whether any connector row currently stores such a field is backend data I cannot query, so I have not confirmed live data loss. I am blocking on it anyway because the failure is silent, unrecoverable, and the confirmation costs one message — not because I have a reproduction.

For balance: the lossy-form-model pattern is not brand new (the merge-base PATCH /mcps/{id} also sent the whole form — git show 797243d9f:…/mcpService.ts:769-782). What this PR changes is the storage substrate: the legacy endpoint had typed columns, whereas the unified backend stores a free-form mcp.json attachment, so the set of fields that can exist — and therefore be lost — is now open-ended.

🔴 P1-2 — A skill re-upload sends no visibility at all, while the sibling metadata edit deliberately preserves it

packages/dmworkskillmarket/src/api/skillApiReal.ts:534-557 and :570-574

const body: Record<string, unknown> = {
  parse_task_id: form.parseTaskId,
  …
  visibility: form.visibility,      // :546

The only production caller never sets it — packages/dmworkskillmarket/src/components/EditSkillModal.tsx:386-395:

const updated = await updateSkill(skill.id, {
  version,
  ...(parseTaskId ? { parseTaskId, changelog } : {}),
  name, displayName, description, categoryId,
  tags: submittedTags,
  ...(iconUrl !== undefined ? { iconUrl } : {}),
});           // ← no visibility, ever

JSON.stringify drops visibility: undefined, so /plugins/import receives no visibility key — on a path the code itself documents as "On a re-upload (existing plugin_id) the import is a full replace" (:550). Whether a private skill stays private across a web re-upload is therefore decided entirely by a backend default the client neither sends nor asserts.

The metadata-only branch two functions down goes out of its way to do the opposite (:587 const visibility = form.visibility ?? plugin.visibility;). Same file, same feature, opposite posture — which is what makes this look like an oversight rather than a decision.

The test that appears to cover it does not: skillApiReal.test.ts:498-512 calls updateSkill("new-skill", { parseTaskId: "task-2", visibility: "private" }), passing visibility explicitly — which no real caller does. Green test, uncovered path.

Fix: thread skill.visibility through from EditSkillModal, or echo the current value in the reupload branch, failing closed to private when absent.

---

P2 findings — not blocking

P2-1 — The args redaction layer is bypassable in mainstream config shapes

packages/dmworkmcp/src/utils/redactMcpConfig.ts:84-116

Context first, because it changes the severity: at the merge base, ExpertSpecView rendered mcpConfig completely raw — git show 797243d9f:…/ExpertSpecView.tsx:61 is <pre className="wk-mcp-expert-code">{mcpConfig}</pre>, and redactMcpConfig.ts does not exist at 797243d9f. This guard is entirely new in this PR. So everything below is pre-existing exposure this PR partially closes, not a regression, and blocking the PR on it would leave the strictly worse unredacted state deployed. That is why these are P2 despite being real leaks.

All executed against the real function at this head (sk-live-DEADBEEF = the planted secret):

control  ["--token=SEC"]                    → "--token=••••••"                        ✅
CR       ["--token=SEC\r"]                  → "--token=sk-live-DEADBEEF\r"            ❌
LF       ["--api-key=\nSEC"]                → "--api-key=\nsk-live-DEADBEEF"          ❌
windows  ["/token=SEC"]                     → verbatim                                ❌
msbuild  ["/p:Password=SEC"]                → verbatim                                ❌
colon    ["--api:key=SEC"]                  → verbatim                                ❌
U+2011   ["--api‑key=SEC"]                  → verbatim                                ❌
arm fail ["--token\r", "SEC"]               → ["--token\r","sk-live-DEADBEEF"]         ❌
arm fail ["--token ", "SEC"]                → ["--token ","sk-live-DEADBEEF"]          ❌
desync   ["--token", 5, "SEC"]              → ["--token","••••••","sk-live-DEADBEEF"]  ❌
desync   ["--token", "", "SEC"]             → ["--token","••••••","sk-live-DEADBEEF"]  ❌
desync   ["--token", "--", "SEC"]           → ["--token","••••••","sk-live-DEADBEEF"]  ❌
docker   ["run","-i","-e","GITHUB_PERSONAL_ACCESS_TOKEN=SEC","ghcr.io/…"] → verbatim  ❌
curl     ["-u","admin:SEC","https://h"]     → verbatim                                ❌
header   ["--header","Authorization: Bearer SEC"] → "authorization: Bearer sk-live-…" ❌
oauth    ["--static-oauth-client-info","{\"client_secret\":\"SEC\"}"] → verbatim      ❌

And two URL shapes:

relative ["/sse#access_token=SEC"]          → verbatim                                ❌
relative ["host/sse#token=SEC"]             → verbatim                                ❌
qry name ["https://h/sse?SEC=x"]            → "https://h/sse?sk-live-DEADBEEF=REDACTED" ❌
qry bare ["https://h/sse?SEC"]              → "https://h/sse?sk-live-DEADBEEF=REDACTED" ❌

The relative branch (:64-66) strips only a query string, so a fragment on a relative or schemeless URL never reaches the u.hash masking at :76 — even though the absolute case handles it correctly. And searchParams.keys() (:71-73) rebuilds param names verbatim, so a bare ?TOKEN query string — which real SSE bridges do use — is re-emitted as a key with a REDACTED value. Both are cheap: run the relative branch through the same hash strip, and mask any query key that isn't in a known-safe set.

Root causes, each one-line:

• Line terminators. :98 /^(--?[A-Za-z0-9_.-]+)=(.*)$/ — . excludes \n/\r/U+2028/U+2029 and $ (no m flag) anchors at true end-of-input, so one trailing \r makes the inline match fail. The token then also fails the bare-flag test at :104 (it contains =) and falls through to the positional branch at :113, where new URL() throws and it is returned untouched.
• Flag charset. [A-Za-z0-9_.-] misses /, :, +, and U+2011 — so both the inline path (:98) and the maskNext arming path (:104-107) miss.
• maskNext desync (:93-96). The arm is consumed by whatever token comes next, including a number, "", or a second flag — and the •••••• it emits makes the output look redacted while the real secret rides through on the next token. This one is worth fixing regardless of the rest: a misleading mask is worse than an obvious miss.
• --header / -H / --headers. isSecretKey("header") is false against SECRET_KEY_PATTERN (utils/constants.ts:22), so the separate-token form leaks while the inline --header=… form is masked. mcp-remote --header "Authorization: Bearer …" is the documented shape for the most common HTTP MCP bridge, so this is the likeliest of the set to be hit in practice.
• NAME=VALUE positionals. docker run -e KEY=secret, --env KEY=secret, and --static-oauth-client-info '{"client_secret":…}' are how people actually write MCP configs. These are "bare positionals", which the docblock lists as an accepted residual — but the docblock also promises --flag=value is masked, which invites the reader to assume NAME=value is too. Masking the RHS of a NAME=… token when the preceding arg is -e/--env/-E would cover the common case.

Also unredacted, and inconsistent with the rest of the function:

{"mcpServers":{"x":{"autoApprove":["https://h/sse?token=SEC","Bearer SEC"]}}}
  → both members verbatim                                                   ❌

:130-134 type-filters autoApprove / alwaysAllow members but never masks them — so the identical URL string that is correctly masked in url or in a positional arg leaks here. That asymmetry was introduced by the rebuild and is the cheapest item on this list to close (redactUrl each member).

And the ${…} passthrough is content-blind:

{"mcpServers":{"x":{"env":{"GITHUB_TOKEN":"${ghp_16C7e42F292c6912E7710c838347Ae178B4a}"}}}}
  → rendered verbatim, full PAT visible                                     ❌

PLACEHOLDER_PATTERN (api/pluginWire.ts:182) is /^\$\{[A-Za-z0-9_]+\}$/, and most real credential formats — GitHub PATs especially — are entirely [A-Za-z0-9_]. Wrapping a literal in ${} makes it survive every masking site (maskValue :49, redactUrl :59, inline args :101, masked-value args :94). Cheap mitigation: reject inner names matching known credential prefixes, or cap the inner length.

The recommendation I actually care about is not "patch these." See §4.1 — the args array is an arbitrary argv for an arbitrary program, so no allowlist of flag names will ever be complete, and command is a free-form shell string by design. This function cannot be a security control; it should be scoped and documented as a courtesy filter, with the real control moved to write time or the backend.

P2-2 — The connector detail surface is not covered by the new guard at all

packages/dmworkmcp/src/api/quickStartTemplates.ts:76-125 vs packages/dmworkmcp/src/components/ExpertSpecView.tsx:42

redactMcpConfig's docblock states the threat model as "a plugin's mcp.json is rendered to every viewer of its detail" — but its only call site is the expert/squad surface. The connector detail renders the same stored mcp.json (read at mcpService.ts:635) through quickStartTemplates, which masks only keys the author explicitly toggled "user-supplied" (:115-125) and publishes the URL and every shared env/headers value verbatim, by documented choice (:83-90). No query/userinfo masking, no args redaction.

This is an accepted product decision that predates the PR, and it is already on the human-verification list below — but it is worth stating plainly that after this PR the two surfaces that render the same artifact have different guarantees, and the one with the careful 175-line guard is not the one most connectors are viewed through.

P2-3 — Silent field drops make the rendered config look complete when it isn't

packages/dmworkmcp/src/utils/redactMcpConfig.ts:119-146

The posture change from "return null" to "drop the key" is right for security but removed the honesty signal. Executed side-by-side against the previous head:

Input server	at 14781cb9b	at a879f3dad
Windsurf {serverUrl}	null → "unavailable" notice	{"mcpServers":{"x":{}}}
Cline {transportType:"sse",url}	null → "unavailable" notice	{"x":{"url":"https://h/sse"}} — transport gone
{command,name,description,icon}	all four rendered	{"command":"npx"}
{command,timeout:"60s"}	rendered	timeout gone
{command,disabled:"true"}	rendered	disabled gone — a disabled server renders as enabled

The first two are the concerning shapes: an empty {}, or a URL with no transport, reads as "this connector is broken" rather than "we couldn't show part of this". name / description / icon were deliberately whitelisted before (the old comment: "Structural, non-secret keys common clients add; safe to keep verbatim"), so their loss is a straight regression in the delta. Suggested fix, which keeps the security property intact: track whether anything was dropped and append a localized "N field(s) hidden" line, or re-emit unknown keys as "<key>": "••••••" so the key names survive and the value cannot.

Minor, same function: whitespace-only input is returned as-is (:149), giving an empty <pre> with no notice.

P2-4 — Every write path echoes visibility unvalidated, while every read path guards it

	read (defends)	write (raw)
connector	mcpService.ts:591-597 mapVisibility → "space"	mcpService.ts:854 visibility: current.plugin.visibility
skill	skillApiReal.ts:320-326 enumerates then → "space"	skillApiReal.ts:587,618
expert	utils/visibility.ts:9-17 normalizeVisibility → "space"	no web write — clean

PluginListItemWire.visibility is declared required (pluginWire.ts:60), so TypeScript raises nothing — but the read mappers exist precisely because the authors don't trust that. If the field is absent or null on a legacy/partial row, the key vanishes from the full-replace body and a backend default decides the stored exposure. Byte-echo of a known value is correct and I verified no widening in either direction; the gap is strictly the absent/unknown case.

Note normalizeVisibility would be the wrong fix here — it defaults to "space", which would widen an absent-visibility private row. A write echo must fail closed to private, or abort, mirroring the precedent resolveWriteCategory (mcpService.ts:567-584) already sets in this same file for categories.

P2-5 — "Secrets are never posted from the web" is overstated

The PR body and mcpWireParams.ts:73-78 both say the client "never SENDS secret values". mcpWireParams.ts:154-176 placeholder-izes only keys in the userSupplied set:

out[key] = supplied.has(key)
  ? placeholderFor(key)
  : value === SECRET_PLACEHOLDER ? "" : value;   // ← verbatim otherwise

Every key with the toggle off is posted as typed, so an edit of an unrelated field re-posts a real stored secret verbatim. The create modal states this outright (McpCreateModal.tsx:106-113: "the value itself IS persisted verbatim and returned on every read… Owners must not type real secrets"), and there is a live pre-submit warning (:1023-1032, rendered at :1216-1225), so this is a disclosed product decision, not a hidden bug. But the claim as written is wrong and should be narrowed to "the web never posts secrets for keys the author marked user-supplied, and never widens visibility." The skill package path has the same echo shape (skillApiReal.ts:605-607).

To be clear about the part that is true and that I verified: no secret value ever reaches a forwarded prompt. All five prompt builders interpolate only {id, spaceId, apiBaseUrl}.

P2-6 — Backend-supplied ids reach forwarded prompts ungated; a sibling builder gates the same value class

packages/dmworkmcp/src/utils/mcpConnectPrompt.ts:22,28 and packages/dmworkskillmarket/src/utils/installPrompt.ts:8,15 interpolate mcpId / skillId straight into a prompt delivered to a Bot that will run shell commands, while spaceId beside them is sanitized. expertBotPublishPrompt.ts:55-59 gates the equivalent id with isValidMcpSpaceId — so the pattern is understood, just not applied consistently.

Filed P2, not P1: plugin_id is server-generated (create never sends it), the prompt is rendered in a <pre> the user can read and edit before forwarding, and delivery is to a user-selected owned Bot. I could not construct an attacker path. It is still an asymmetric trust boundary where the builder validates the value it controls and trusts the value it does not, and the assertion is cheap. Coverage nit: the skill builders have poisoned-input tests (installPrompt.test.ts:41, botPublishPrompt.test.ts:28); mcpConnectPrompt.test.ts has none. If any deployment lets an author choose plugin_id, this becomes P0 — worth one confirmation.

P2-7 — Forwarded prompts select an octo-cli profile by Space ID alone, ignoring the API origin

packages/dmworkmcp/src/utils/mcpConnectPrompt.ts:37, packages/dmworkskillmarket/src/utils/installPrompt.ts:24, botPublishPrompt.ts:29, mcpBotPublishPrompt.ts:64, expertBotPublishPrompt.ts:113

All five prompts instruct the Bot to "选择 space_id 等于 ${spaceId} 的唯一 Profile", and only use the authoritative --api-base-url when creating or updating a profile. If a local profile matches the Space ID but points at a different deployment, every subsequent --profile command publishes, installs, or connects against the wrong environment. The sanitizer explicitly admits readable slug forms, so a Space ID like default plausibly exists in more than one deployment.

Pre-existing, not introduced here — I checked: the wording at installPrompt.ts:18 is byte-identical at the merge base (git show 797243d9f:…/installPrompt.ts). What this PR does is extend the same instruction to a fifth surface (mcpConnectPrompt.ts is new in this PR). Worth fixing while the file is open, since it is one sentence: match on both Space ID and API origin.

P2-8 — Connector edit re-slugifies two identity-bearing fields

mcpWireParams.ts:52 re-runs slugifyServerName on the value read back from manifest.name, and utils/constants.ts:43-52 strips _ and .. Verified: my_server → myserver, github.com-mcp → githubcom-mcp. That value is both the mcpServers JSON key (:98,100) and connector.source (:124), so an unrelated metadata edit renames the connector's stable identifier. Web-created rows are already normalized; CLI/import rows are not. The function is idempotent, so this is a one-time mutation rather than drift.

P2-9 — Skill write has no service-level category fail-closed, while the connector write does

mcpService.ts:567-584 refetches once and throws rather than write a null category, with the split-brain rationale spelled out. The skill path just omits the key (skillApiReal.ts:544, :589, :615), so a blank category silently produces the exact NULL-category state the connector path refuses to create. An unknown category cannot become a real one, so this is integrity-of-state, not over-exposure. Only gate today is modal validation (EditSkillModal.tsx:366).

P2-10 — Carried from prior rounds, re-verified unchanged at this head

git rev-parse confirms these files are byte-identical to 14781cb9b, so every prior finding in them is still open:

• MineTable stat columns render on 3 of 4 tabs of the same page — MineTable.tsx:78 defaults showStats = true; the connector tab passes false (McpMarketListPage.tsx:854) and 技能/专家/专家团 omit it (SkillListPage.tsx:346, ExpertMarketListPage.tsx:774, :723). Column count changes as you switch tabs. One of the two intents is wrong.
• Dead expression: ExpertMarketListPage.tsx:829 showStats={variant === "mine"} sits in the kind !== "mine" else-branch. I re-derived reachability rather than inheriting the claim: setKind is only called at :501/:508, inside a variant !== "mine" guard (:495), so variant === "mine" && kind !== "mine" is unreachable and the expression is always false.
• Stale docs: PromptForwardActions/index.tsx:51 documents a preview prop that no longer exists; expertService.ts:758,763,821,831 and ExpertMarketListPage.tsx:389 still document retired endpoints.
• MyAssetsPage.tsx:16-26 reads ?type= but never writes it back, so the tab is deep-linkable but not shareable or back-navigable.
• PR body test counts are stale: it claims 136 / 220; actual at this head is 134 / 245.
• Unused import: mcpService.ts:26 imports SECRET_PLACEHOLDER and never uses it (no lint script in the package).

---

Things I checked that are fine

• Secret placeholder round-trip is byte-stable in all four directions, including the cross-referential ${SHARED_TOKEN}-under-key-TOKEN case (preserved verbatim, not renamed) and key-normalization collisions (X-API-Key / X_API_Key both → ${X_API_KEY}, both round-trip, keys stay distinct). selfPlaceholder (pluginWire.ts:187-190) and placeholderFor (mcpWireParams.ts:143-149) are character-for-character identical, which is what makes it hold. A placeholder can never overwrite a real secret, and a real secret can never become a placeholder without an explicit toggle flip.
• No visibility widening via any web write. Connector create hardcodes "space" (mcpService.ts:812); update echoes; skill create is "space"; experts/squads have no web write path at all (grepped expertService.ts: only /plugins/delete and /plugins/install, ids only). The web cannot set public or system anywhere. Unknown wire values degrade to space, never to system/public.
• sanitizeShellSpaceId is sound and consistent. Allowlist ^[A-Za-z0-9._-]{1,128}$, rejects .. and leading -/., falls back to the inert <space-id>. All five prompt builders bottom out in it. The duplicated copy is byte-identical to the original as its header claims — diff <(tail -n +7 dmworkskillmarket/src/utils/spaceId.ts) dmworkbase/src/Utils/spaceId.ts is empty.
• No XSS on the redaction surface. ExpertSpecView.tsx:71-73 renders the string as a React text child; no dangerouslySetInnerHTML anywhere in packages/dmworkmcp/src/. A server name of </pre><img src=x onerror=…> round-trips as inert escaped text.
• No ReDoS and no unhandled throw in the redaction path. SECRET_KEY_PATTERN is anchored at both ends with no nesting → linear; a 200k-char flag measured 8 ms, a 100k query-param URL 89 ms. 200k randomized structured fuzz cases (control chars, lone surrogates, BOM/ZWSP/U+2028, __proto__/constructor keys, mixed types) produced 0 throws and 0 non-JSON outputs. __proto__ yields no pollution escape.
• The "~3,600 deleted lines" coverage gap is closed — there is nothing to audit. Every prior round flagged the removed full-page editor cluster as unread and relied on "Build is green plus a grep". It turns out no file is deleted between the merge base and this head at all: git diff --diff-filter=D --name-only 797243d9f a879f3dad is empty. The seven files removed by 98be0821 (ExpertEditorPage.tsx, SquadEditorPage.tsx, SkillEditorPage.tsx, SkillFileTree.tsx and three expertService.*.test.ts) were all created earlier in this same branch and removed before the PR — git cat-file -e 797243d9f:<path> fails for all seven. So the deletions are branch-internal churn, no behaviour reachable from main was removed, and the "type-correct deletion of still-reachable behaviour" risk that every round listed as unverified does not exist here.
• Prototype-pollution and duplicate-key handling. {"mcpServers":{"__proto__":{…},"a":{…}}} → only a survives; duplicate JSON keys are last-wins at both levels with both branches redacted.
• goCanonicalJSON (pluginWire.ts:142-175) drops undefined, sorts keys, and matches Go's escaping — deterministic, so no spurious diff on re-save.
• isSafeAttachmentPath (skillApiReal.ts:252-260) blocks empty/absolute/backslash/NUL/.. before echoing backend paths into a trusted write.
• parseTeamAgentsMarkdown (expertWire.ts:221-267) is fail-closed — if (!inCollaboration) continue stops injected ### headings in prose from seeding team config.
• Presigned-upload hygiene. assertSafeUploadURL (mcpService.ts:95-105) and assertSafeExternalURL (skillApiReal.ts:230-242) gate schemes; both PUTs use interceptor-free clients so no session token crosses to storage.
• fetchSkillPackage (expertService.ts:886-931): the auth-header branch is gated on same-origin, the external branch is scheme-guarded and header-free, and the 20 MiB cap is enforced by streaming with reader.cancel() rather than trusting content-length.
• Connector category write is genuinely fail-closed; read-side category/tag filters return empty rather than widening to the whole catalog (mcpService.ts:734-744, expertService.ts:356-375).
• copyTrackEvent telemetry is intact — I checked this because it was raised as a possible regression. Four of the five forward surfaces pass it explicitly (McpBotPublishModal.tsx:58, McpConnectModal.tsx:55, InstallPromptModal.tsx:47, BotPublishModal.tsx:55); the fifth (expert/squad publish) omits it by documented decision (TrackRules.ts:153, PromptForwardActions/index.tsx:63-68). No metric is silently lost. Relatedly, the draft-reset effect at PromptForwardActions/index.tsx:136-139 does not clobber user edits: prompt is a string, so the dep compares by value, and four of five callers additionally memoize it.
• The e2e-p0 failure on the tree-identical parent 9c3f4ef68 was not caused by this PR. All 154 cases passed; the run was failed by the proxy-error gate on a single escaped request, /summary/api/v1/summaries?page=1&page_size=1. That request originates in packages/dmworksummary/src/utils/summaryMenuBadge.ts:56, a timer-driven sidebar badge poller in a package this PR does not touch, whose __MSW_READY__ guard (:49-55) is racy. The retrigger at a879f3dad is green, which is consistent with that diagnosis rather than with a PR-caused failure.

---

3. Overall verdict

REQUEST_CHANGES

Spec is ✅ and the delta is genuinely good work — the whitelist rebuild is the right architectural fix, it is correctly implemented, and its tests survive mutation. CI is fully green and the cross-repo backend gate is discharged.

What blocks is a single coherent theme that is not in the delta and has not been examined in the prior rounds: this PR converts the marketplace writes to full-replace upserts, and two of the three paths replace state they never read back — the connector mcp.json rebuild (P1-1) and the skill re-upload visibility (P1-2). Both destroy stored state silently, on success, with no signal. Both are cheap to fix or to discharge with one confirmation, and the skill metadata path in this same PR already shows the correct pattern.

Everything else is P2 and should not hold the merge.

---

4. Recommendations

4.1 Stop treating redactMcpConfig as a security control — scope it honestly

The whitelist rebuild fixed the real architectural defect and I want that credited. But the remaining leaks in §P2-1 are not a to-do list, they are a signal about what this function can be. args is an arbitrary argv for an arbitrary program and command is a free-form shell string that is explicitly an accepted residual — so no allowlist of flag names will ever be complete, and each round will keep finding one more shape. Concretely:

1. Fix the three cheap, unambiguous ones now — maskNext desync (a mask that lies is worse than no mask), autoApprove/alwaysAllow members (inconsistent with the identical string masked elsewhere), and the ${credential} passthrough.
2. Rewrite the docblock to promise only what it delivers. It currently claims --flag=value and secret-named-flag coverage without qualification; it should say "best-effort cosmetic masking of the common shapes; not a guarantee, and command/args can carry anything."
3. Put the real control where it can be complete — a high-entropy-literal check at publish time (refuse or warn before the value is ever stored), or reinstate a backend secret scanner. The docblock notes the scanner was deliberately removed; that removal is what makes a display-side filter carry weight it cannot bear.

4.2 Make full-replace writes echo what they replace

P1-1 and P1-2 are the same bug shape. A general guard worth adding while fixing them: on any full-replace upsert, assert that every key present in the fetched record is either re-emitted or explicitly listed as intentionally dropped. skillApiReal.ts:605-607 is the model.

4.3 Round-count escalation — this needs a human decision, not another lap

This PR now carries 37 formal reviews from three reviewers across roughly 34 rounds on one branch, and the last six rounds have all been about the same 175-line utility. That pattern is its own finding. For whoever owns the merge:

1. Fix P1-1 and P1-2 — they are narrow and bounded.
2. Split the branch. redactMcpConfig is a small, security-critical, heavily-contested unit sitting inside a 110-file migration. It would review far better on its own terms, and the other ~105 files have been stable for many rounds.
3. If the merge window matters more than the split, accept the §P2-1 residuals explicitly and in writing with §4.1 tracked as a follow-up. That is a legitimate call — the guard is still a large net improvement over the unredacted merge-base — but it should be a human's decision, not something a review round absorbs silently.

4.4 For human verification (security-sensitive)

1. Release ordering. octo-marketplace#72 is merged, but the gate is deployed. There is no client-side dual-version tolerance, so confirm the ordering is enforced somewhere rather than only documented.
2. P1-1's blast radius — does any stored connector mcp.json carry a field outside {type,url,command,args,env,headers}, a second mcpServers entry, or a sixth attachment? One query settles it.
3. Does /plugins/upsert / /plugins/import preserve, default, or reject an absent visibility? This decides whether P1-2 and P2-4 are latent or live. One look at the Go handlers.
4. Accepted exposure. Shared, non-user-supplied env/headers literals and the connector URL are shown to every viewer by design (quickStartTemplates.ts:83-90, :115-125), and the backend has no secret scanner. Please have security confirm that "authors must put credentials in user-supplied slots" is an acceptable documented contract — it is the assumption the entire guard rests on.
5. Two non-catalog endpoints survive on the migrated service — /mcps/_probe (mcpService.ts:792) and /mcp_icon_uploads (:892). They pre-date this PR and are correctly out of scope, but if octo-marketplace retires the /mcps/* namespace wholesale, probing and icon upload break with no compile-time signal.
6. Backfilled key shape. The reader takes server.type (mcpService.ts:644) while the redaction whitelist models both type and transport. If the backfill emitted transport, the detail/quick-start transport silently degrades to the stdio default.

---

5. Coverage — what I did not check

Stated plainly so the gaps are not read as clearances.

• Whether P1-1 causes live data loss. The mechanism and reachability are proven from the diff; whether any stored connector currently holds an affected field is backend data I cannot query. Same for whether /plugins/upsert merges or replaces attachments.
• The backend half of P1-2 / P2-4 / connector version. Whether an absent visibility or version is preserved, defaulted, or rejected is decided in octo-server, which is not in this workspace. I graded these on client shape (sends nothing, asserts nothing) and on the intra-file asymmetry, not on a confirmed widening.
• No browser run. Everything is static tracing plus the unit suites plus direct execution of the redaction functions. I did not drive an edit-then-inspect-request cycle against a live backend, which is what would turn P1-1 and P1-2 from "unasserted" into "confirmed".
• e2e was not executed locally. I read the migrated MSW handlers and the spec diffs and relied on CI e2e-p0 (green at this head) for behaviour. Note that green e2e-p0 does empirically exclude the "handler mocks a slightly different URL, so the real request escapes" failure mode, because the job fails on any http proxy error: — so a handler/request mismatch on an exercised path would have shown up. What I did check by hand is that the dropped assertions are the intentionally-removed "共 N 个" totals rather than silent weakening; the two expect(await …count()).toBe(0) → await expect.poll(…).toBe(0) conversions (SK2, EX2) are anti-flake changes that could in principle pass trivially before the list renders, but each is preceded by a toBeVisible() on the matching row, which closes that window.
• Deleted code. Nothing to check — see the clearance above; net vs. main this PR deletes zero files.
• Not examined: the CSS restructure, Storybook, the icon-upload presigned flow beyond scheme gating, client-side ZIP extraction, category/tag pagination across the three markets, and full expert/squad relation ordering.
• storage_uri semantics (skillApiReal.ts:357 treats it as a download URL while :605-607 echoes it into a write). One of those two is wrong; deciding which needs to know whether the backend presigns it on read. The write half is live; fileUrl has no consumer.
• Out of repo: the entire octo-marketplace side. Every wire-shape claim here is "the client is self-consistent", not "the client matches the server" — except the 2.0 $schema ids, which I confirmed exist in internal/service/plugin/schema.go.
• Process note. Alongside my own read I ran four independent analysis passes: an executed-probe pass over the redaction function, a write-path/trust-boundary pass, and two cross-model adversarial passes. Three reported; the pass assigned to the e2e/MSW handler-vs-request comparison did not return in time, so that area rests on the CI reasoning above plus my own spot-check of the assertion diffs rather than on a dedicated audit. I am recording that as reduced coverage, not as a clearance. Every finding I adopted from another pass, I re-derived and re-executed myself before including it — two claims put to me were refuted that way and are listed under "checked and fine" (the copyTrackEvent telemetry regression and the draft-reset edit-clobber).

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Reviewer: Octo-Q (automated review)
Head: a879f3dad3efac745015916064c303f3d2fbe65d · Base(merge-base): 797243d9f2b77e39f59dd84f1ab0fce7f66e4e81 (origin/main)
Diff: 110 files, +6331/-2058 · Routing: security_sensitive · 审查方式：本地 PR checkout 静态审查（pr-base...HEAD），未运行 build/tests

---

Code Review — PR #1584 (octo-web)

Summary

This PR migrates all three market catalogs (skills, connectors/MCP, experts) off the legacy per-type endpoints onto the octo-marketplace unified plugin surface (/plugins, /plugins/detail, /plugin_categories, /plugins/*), restructures the market UI into a four-entry sidebar (技能 / 连接器 / 专家 / 我的发布), and replaces per-modal copy-prompt flows with a shared forward-to-Bot component (PromptForwardModal / PromptForwardActions). Secret handling on the unified surface is a client-side control: user-supplied env/header keys are persisted as ${KEY} placeholders and never sent as real values, a whitelist-rebuilt redaction (redactMcpConfig) masks literal secrets on public detail views, and category resolution fails closed on both read and write paths. 110 files, +6331/-2058; the migration is careful and heavily commented.

Verification

Static analysis only at head a879f3da; build and tests not executed in this environment.

• ✅ Secret round-trip pinned by tests — mcpService.visibility.test.ts locks the placeholderSecretMap/splitUserSupplied round-trip including digit-leading keys, colliding placeholders, cross-referential ${SHARED_TOKEN}, and the redaction sentinel; owner real values for user-supplied keys are never transmitted.
• ✅ Fail-closed category resolution — mcpService.fetchMcpListPath and expertService.listPathReal return an explicit empty result (never widen to the unfiltered catalog) on an unresolved category; resolveWriteCategory refetches once then throws instead of writing NULL category_id. mcpService.connector.test.ts exercises the production service through a mocked axios instance.
• ✅ Redaction is whitelist-rebuilt and fail-closed — redactMcpConfig re-emits only known server fields, masks env/header values, URL query/userinfo/fragment, and secret-named arg tokens; unmodelable input returns null → localized "unavailable" notice instead of the untrusted original. Applied on the public expert/squad detail surface (ExpertSpecView); no dangerouslySetInnerHTML anywhere in the market packages.
• ✅ Session teardown scoped — marketplace 401 logs out; fleet 401 and the fire-and-forget /metrics/track beacon never tear down the session; the skill client mirrors this via skipAuthRedirect.
• ✅ Icon canonical-value echo — both edit paths send the write-canonical icon (never the expiring presigned icon_url): MCP via the undefined sentinel + updateMcpReal echo, skill via plugin.icon echo when form.iconUrl is absent.
• ✅ Per-Space caches — connector/expert category maps and the Loop workspace/runtime caches are keyed by Space with poison-on-failure clearing and explicit invalidation on space-changed, so one Space's data cannot leak into another.

Findings

No P0/P1 issues; two P2 items and one nit below.

P2 — Presigned-URL scheme guard relaxed to any http(s) host (packages/dmworkskillmarket/src/api/skillApiReal.ts:237)

assertSafeExternalURL previously allowed only https or loopback http; it now accepts http to any host, so a presigned upload/download URL returned by the marketplace can point at http://10.x or http://169.254.169.254 and the client will PUT/GET it. The URL source is the trusted authenticated backend and the PUT runs without app credentials, so this is defense-in-depth erosion rather than an exploitable hole — but it is asymmetric with the stricter guard the expert path kept (packages/dmworkmcp/src/api/expertService.ts:852). Confirm server-side that presign responses can never carry internal/link-local hosts in production, or restore parity between the two guards.

P2 — Combined "creator · owner" publisher label can never render again (packages/dmworkskillmarket/src/api/skillApiReal.ts:313)

mapSkill now synthesizes creatorId from owner_id, so creatorId === ownerId for every row and the creatorId !== ownerId branch in SkillCard.tsx:102 / SkillDetailModal.tsx:227 never fires; the name-comparison fallback is unreachable whenever owner_id is present. The unified wire genuinely has no creator_id field and the displayed creator_name remains correct, so this is a display simplification rather than wrong data — but the SkillCard comment "Catalog responses intentionally omit owner_id" is now stale and worth refreshing.

Nit — escapeLikeGo overclaims Go parity (packages/dmworkmcp/src/api/pluginWire.ts:167)

The comment claims Go json.Marshal parity including U+2028/U+2029 escapes, but Go emits those two code points raw. The output is still valid deterministic JSON; this only matters if a server-side process ever byte-compares a Go-reserialized attachment against stored content. Fix the comment or drop the two replaces.

Human-verify

1. Backfill of user-supplied secrets — the client now never sends real values for user-supplied env/header keys (they are emitted as ${KEY}), while the legacy backend used to persist the owner's value and blank it for non-owner reads. Legacy rows must have been migrated so their user-supplied values become ${KEY} placeholders; this server-side migration is outside this checkout and worth confirming. Not a merge blocker for this PR.
2. Presign endpoint host policy — see the P2 above: confirm octo-marketplace never returns internal/link-local hosts in presigned URLs for production tenants. Not a merge blocker for this PR.
3. goCanonicalJSON byte parity — if any server-side path re-serializes attachment JSON with Go and compares bytes/hashes, the U+2028/U+2029 divergence in the nit above would matter. Not a merge blocker for this PR.

Things I checked that are fine

• Prompt builders (buildInstallPrompt, buildMcpConnectPrompt, MCP/expert bot-publish prompts) all sanitize interpolated space ids through the shell-safe whitelist; only server-generated UUIDs are interpolated raw.
• fetchSkillPackage streams with a 20 MiB cap and cancels over budget; fleet list payloads fail loud on non-list shapes instead of coercing to an empty picker.
• Relation fan-out in expert/squad detail drops only confirmed 404 dangling targets; 500/403/network failures surface instead of silently shrinking skill/member lists.
• Skill pagination cursor synthesis (String(page+1) ⇄ cursorToPage) round-trips through useSkills infinite scroll; mine-vs-market variants pin their data source at mount.
• New i18n keys exist in both locales for every new surface (sidebar, mine table, visibility chips, forward actions, error states); e2e MSW fixtures were re-shaped to the plugin wire contract.
• Visibility mapping degrades unknown wire values to space (non-permissive) on every surface; skill creation hardcodes visibility: "space", so the public official badge stays admin-only.

Verdict: APPROVED

The migration preserves the runtime contracts it replaces, the security-sensitive surfaces (secret placeholders, public-detail redaction, shell-safe prompts, session teardown scoping) are deliberately engineered and test-pinned, and the two P2s are non-blocking polish/confirmation items.

---

Octo 专属补充段（automated review内部材料，供终审参考）

1. 验证结论

• ✅ 统一 plugin wire 契约（pluginWire.ts ×2）→ 三个 service 的 mapper 消费路径完整：mapSkill/mapSkillDetail（skillApiReal.ts:299/338）、mapListItem/mapDetail（mcpService.ts）、mapPluginAgentListItem/mapPluginSquadListItem/fromSkillPlugin（expertWire.ts）。
• ✅ 密钥占位 round-trip（write placeholderSecretMap ↔ read splitUserSupplied）逻辑互洽且有单测锁定（mcpService.visibility.test.ts:127-190），含数字开头 key、占位符碰撞、跨引用 ${SHARED_TOKEN} 不改名、sentinel 不落库。
• ✅ 分类解析读写两端均 fail-closed（list 返回显式空、write 抛错），不会静默放宽为全量目录。
• ✅ 会话拆除面收敛：仅 marketplace 401 触发 logout；fleet 401 与 /metrics/track beacon 401 不拆会话（expertService.ts 响应拦截器、skillApiReal skipAuthRedirect）。
• ⚠️ 两处 P2 + 一处 Nit 见正文（均不阻塞）。

2. 发现问题定级（diff-scope 三问）

1. P2 presigned URL 守卫放宽（skillApiReal.ts:237）：本 PR 修改（weakened）既有守卫；非既有行为——旧代码只放行 https + loopback http。不满足 R1（URL 来源为可信已鉴权后端、PUT 不带应用凭证，未让可用路径不可用/未产生错误数据）→ P2 + human-verify。
2. P2 creator/owner 联合署名不再渲染（skillApiReal.ts:313）：本 PR 新引入（unified wire 无 creator_id 字段，客户端合成）；显示简化，展示值（creator_name）本身正确 → 非错误数据，不满足 R1 → P2。
3. Nit goCanonicalJSON U+2028/U+2029 注释与 Go 行为不符（pluginWire.ts:167）：新代码；输出仍为合法确定性 JSON，字节对比场景（旧 manifest byte-match 规则）已明确退役 → Nit。

3. 建议

• P2-1：与 octo-marketplace 侧确认 presign 产物在生产不会出现内网/链路本地主机；否则恢复与 expert 路径（expertService.ts:852）一致的严格守卫。
• P2-2：刷新 SkillCard.tsx:99-101 过时注释；如需保留联合署名展示，可改用 creator_name !== publisher 名称比较（当前 ID 相等使名称比较分支不可达）。
• Nit：修正 escapeLikeGo 注释或移除 U+2028/U+2029 两个 replace。

4. 额外发现

• 旧 /skills/{id}/download、downloadSkill、getDownloadUrl 移除后无任何残留调用方（base 上也无组件调用），非悬空。
• 旧 expert-v1 wire mapper（mapAgentDetail/mapSquadDetail 等）在 HEAD 仅被测试引用，生产已切换 unified mapper；保留为迁移期测试覆盖，非问题。
• .i18n/scan-config.json 中 RichTextContent 的 ignore 移除来自 upstream/main 合并（ac409b49e），非本 PR 变更，不计。
• 我的发布 >100 条时截断已有显式提示（mcp.expert.truncatedNotice），非静默。

5. 数据流回溯（被消费数据 → 上游来源 → 是否真流到消费点）

• nextCursor（skill 无限滚动）：listPlugins 由 offset 分页合成 String(page+1) → useSkills.fetchPage 透传 cursor → cursorToPage 解析回页码。✅ 真流到消费点。
• visibility（卡片/表格徽章）：wire visibility → mapVisibility/normalizeVisibility/mapSkill 三处均把未知值降级为 space（非放权桶）；system 在 skill/mcp/expert 三侧分别驱动官方徽章。✅
• icon（编辑回写）：详情显示用 icon_url || icon（presigned 会过期），写回路径 MCP 用 undefined 哨兵 → updateMcpReal 回显 current.plugin.icon；skill 用 plugin.icon 回显（form.iconUrl 仅在选新图时定义）。✅ 过期 URL 不会被持久化。
• env/headers（用户自填槽）：写侧 placeholderSecretMap 只发 ${KEY}，真实值从不出网；读侧 splitUserSupplied 把自引用占位符清空为填写槽、跨引用占位符原样保留。✅ 与 mcpWireParams 注释声明一致（已逐分支核对）。
• mcpConfig（公开详情展示）：getExpertReal/loadSquadMember 从 mcp.json attachment 读原始串 → ExpertSpecView 经 redactMcpConfig 白名单重建后渲染；解析失败返回 null → 渲染"不可用"文案而非原文。✅ 无未脱敏直出路径（已 grep 全包，唯一消费点即 ExpertSpecView）。
• Loop 目标缓存：getLoopWorkspaces/getLoopRuntimes 按 Space 键控 + syncLoopCacheSpace 每次读取校验 + space-changed 显式清空。✅ 无跨 Space 泄漏。
• 分类映射缓存：per-Space 键控 + 失败清缓存（不毒化）+ 写路径 miss 时强制 refetch 一次。✅

6. 盲点 checklist（security_sensitive，全项）

• C1 双路径 parity：已核对 create↔update（/plugins/upsert 单一入口，toPluginUpsert 共用；icon 哨兵语义两端一致）、create↔delete（upsert vs /plugins/delete）、all↔mine（mode=mine 参数化同一 /plugins，McpMarketListPage mine variant 在 mount 时钉死 mode）、expert agent↔squad（pluginTypeOf 对称、删除共用 deletePluginReal）。skill 安装/连接器接入的 prompt 构建器同构且均做 spaceId 消毒。✅
• C2 control-flow ordering / 嵌套复用：PromptForwardActions 被 5 个表面复用（安装/接入/三种上架），逐一核对 props 传递与 copyTrackEvent 归属；listPlugins 被 getSkills/getMySkills 复用、listPathReal 被 4 个 list 入口复用，mode/kind 参数化无串扰。安全控件试穿：spaceId 消毒拒绝空白/元字符/../前导 -/.；isSafeAttachmentPath 拒绝绝对路径/反斜杠/NUL/.. 段。✅
• C3 授权边界 ≠ 能力边界：新增可达端点 /plugins/*、/plugin_categories、/plugin_tags、/metrics/track、/plugins/install、fleet /workspaces、/runtimes 均经同一鉴权拦截器（token + X-Space-Id）；/plugins/delete、upsert 的 owner 校验在服务端（本 checkout 外）——客户端仅在 mine 列表（服务端 mode=mine 过滤）暴露编辑/删除入口。⚠️ 服务端授权不在本仓，见 human-verify 表述（非阻塞）。
• C4 授权生命周期/容器-成员级联：可见性为行级 visibility 字段，列表/详情均由服务端按 Space+visibility 过滤（客户端只传 X-Space-Id）；system 官方行不泄漏掩码创作者名（e2e mcp-official fixture 显式断言 redacted creator 不上屏）。✅（前端侧）
• C5 build/note 通过 ≠ 运行期路径正确：未依赖 build 结论；所有关键路径做了运行时推演（见 §5）。未运行 build/tests（环境约束），已在 Verification 段如实标注。✅ 方法论合规
• C6 治理/策略文档自洽性：本 PR 无安全政策/披露流程类文档变更；redactMcpConfig 的"documented residuals"（command 内嵌密钥、URL 路径段）与"shared env/headers 值属作者主动公开"的设计立场在代码注释、create modal 注释、quickStartTemplates 注释三处表述一致。✅

7. 跨轮 blocker 复检（R6）

N/A —— 本 issue 为本automated review腿对该 head（a879f3da）的首轮审查，无上一轮未解决 blocker 需复检。（注：commit 历史显示该分支此前经历过多轮修复迭代——含 4 个连续的 redaction 加固提交与"八审"级别埋点修复——本次已按当前 head 独立复核，未采信任何历史轮次结论。）

---

[Octo-Q] verdict: APPROVE —— 无 P0/P1；仅 2 个 P2（presigned URL 守卫放宽需服务端确认、联合署名显示退化）+ 1 个 Nit（Go 转义注释）。统一 plugin 迁移的密钥占位、公开详情脱敏、fail-closed 分类、会话拆除收敛等安全面均经数据流回溯验证且有测试锁定。

[yujiawei]

Addendum to my review at a879f3dad — one analysis pass reported late

My review noted that the pass assigned to the e2e/MSW and i18n/deletion audit did not return in time, and recorded that as reduced coverage. It has since reported. The verdict is unchanged (still CHANGES_REQUESTED on P1-1 / P1-2) — but it surfaced findings I have now verified myself, and it also lets me retract one concern and correct one piece of evidence I published. Posting both so the record is accurate and so this can all be addressed in one pass.

⚠️ Correction to my own evidence: pnpm i18n:check does not cover the two packages this PR grows

I cited pnpm i18n:check passing as evidence. That check is structurally blind to mcp.* and skillMarket.*, so its green result says nothing about either package.

scripts/i18n-scan.mjs:17-26 — namespaceByDir lists dmworkappbot, dmworkbase, dmworkcontacts, dmworkdatasource, dmworklogin, dmworksummary. dmworkmcp and dmworkskillmarket are absent. Then:

const knownNamespaces = new Set(["app", ...Object.values(namespaceByDir)]);
…
function isTranslationKey(value) {          // :44-49
  const namespace = value.split(".")[0];
  return knownNamespaces.has(namespace);    // rejects "mcp" and "skillMarket"
}

so every t("mcp.…") / t("skillMarket.…") call is dropped before the missing-key check runs. There is a compounding mismatch: getPackageInfo() (:138-145) derives dmworkskillmarket → skillmarket (lowercase) while the module registers skillMarket, so simply adding the namespace surfaces 262 false positives — which is presumably why it was never added.

Failure scenario: the next change that adds t("mcp.foo.bar") without a locale entry ships a raw dotted key to users and CI stays green. There is no namespace fallback (I18nService.ts:137-140 falls back on locale only), so a missing key renders as the literal mcp.foo.bar.

To be clear about what this does and does not mean for this PR: the late pass independently swept 2,938 key literals against all 9 bundles and reports 0 missing keys, 0 zh↔en asymmetry, 0 placeholder mismatches for this diff. So this PR's i18n appears fine — but it was verified by that sweep, not by the CI gate, and the gate will not protect the next one. Fixing the scanner is out of scope here; it should be its own issue. I am flagging it because I presented the gate as evidence and it isn't.

Retracted: the sort enum concern is a non-issue

The pass flagged that the three markets send different sort values for 最热 — skills downloads (skillApiReal.ts:386-390), connectors and experts installs (mcpService.ts:695-696, expertService.ts:283-286) — and that the three source comments make apparently contradictory claims, with the risk that one market's 最热 button is silently inert.

I resolved this against the backend rather than leaving it open. octo-marketplace internal/repository/plugin/read.go:13-14:

// ListFilter restricts a scoped Plugin listing. Sort accepts newest (default), oldest, updated, name, placement, views, installs, downloads, or …

with the switch at :152-173 handling views, installs, downloads and a weighted comprehensive. Both downloads and installs are valid enum members, so no button is inert and all three comments are individually correct — the enum is simply a superset of what each names. Nothing to fix.

The only residual is a product-consistency question, not a defect: 最热 ranks skills by download count and connectors/experts by install count. Worth a deliberate answer, but it does not affect the merge.

Additional verified findings — all P2, none change the verdict

Vacuous assertion in SK4. apps/web/e2e-kit/tests/skills/SK4-skills-market-pagination.spec.ts:25:

await expect.poll(() => skillRequests.some((url) => !url.includes("cursor="))).toBe(true);

On main this was meaningful because page 2 sent cursor=page-2. At this head buildPluginListParams (skillApiReal.ts:424-438) emits only scene_code, plugin_type, mode, q, category_id, tag, sort, page, page_size; cursor survives purely as an internal UI value funnelled through cursorToPage (:394, :436). No request URL contains cursor= at any point, so the assertion is satisfied by the first request and can never fail. The sibling assertion on :27 was migrated (cursor=page-2 → page=2); this one was left as dead weight. This is the one thing in the e2e diff that is silently weakened rather than intentionally dropped — the "共 N 个" removals I checked by hand are all legitimate.

The manual-publish funnel's terminal step no longer fires for connectors. At the merge base both halves were mapped (git show 797243d9f:…/FetchRules.ts:279-280 — POST /mcps and POST /skills → market_manual_publish_submitted). At this head only FetchRules.ts:287 remains, on /plugins/import, which is skill-only (skillApiReal.ts:561, :571); connector create and update both post to /plugins/upsert (mcpService.ts:811, :847), deliberately unmapped and pinned so by FetchRules.test.ts:214. The reason is documented at FetchRules.ts:278 (the fetch layer cannot distinguish create from edit on a shared endpoint) and I think the decision is defensible — but the consequence isn't recorded anywhere: the funnel documented at DAP_EVENTS.md:206 will report 0% terminal conversion for connectors, which reads as a product collapse rather than an instrumentation gap. Either emit it imperatively from the create path or note the gap in DAP_EVENTS.md.

Four of this PR's own copy edits are no-ops. navRail.settingsCenter.value.{granted,denied,unauthorized,unsupported} are duplicated in both packages/dmworkbase/src/i18n/locales/zh-CN.json and en-US.json (around :1458-1461 and :1749-1753). JSON.parse keeps the later occurrence, and the diff edits the earlier one. I compared post-parse values at base vs. head:

navRail.settingsCenter.value.granted      base='已允许'      head='已允许'      → NO-OP
navRail.settingsCenter.value.unsupported  base='当前环境不支持'  head='当前环境不支持'  → NO-OP
navRail.settingsCenter.value.denied       base='已拒绝'      head='已拒绝'      → NO-OP
navRail.settingsCenter.value.unauthorized base='尚未授权'     head='尚未授权'     → NO-OP

So 已授权 → 已允许 and 当前不支持 → 当前环境不支持 do not reach users. The PR deduped three sibling duplicates and left these four; flattenMessages in the scanner runs on the already-parsed object, so the gate cannot see this class at all.

Case-spec markdowns are out of lockstep with the specs. Spec code changed for 9 cases; 6 markdowns were touched, and all 12 market case-specs still document the legacy per-type endpoints — e.g. case-specs/mcp/C37-mcp-official-publisher.md:20-22 still lists GET /market/api/v1/mcps / /mcp_categories / /mcps/:id even though this PR migrated that spec's own assertion to /plugins?. Two behavioural changes went undocumented: C39 and C40 now deep-link to /mcp-market/mine?type=mcp and C40 asserts a table row, while case-specs/mcp/C39-…md:19 and C40-…md:19,23 still describe /mcp-market/mcp?sid=e2etest, a 「全部」/「我的」toggle this PR deleted, and a card rather than a row. The markdown is what a human reads to decide whether a spec still tests the right thing.

Two latent e2e gaps (not active — the market suite runs clean with 0 proxy errors, which the late pass confirmed by executing it: 14/14 pass):

• /plugin_tags has a live caller (McpMarketListPage.tsx:282, fired when the tag popover opens) and no MSW handler anywhere. No current spec opens that popover, so nothing escapes today; the first one that does will fail the whole e2e run via the PROXY_ERRORS gate.
• No market spec uses the sanityCheck / startRequestMonitor net from apps/web/e2e-kit/_lib/sanity.ts, while 20 summary/appbot specs do. That net catches same-origin API 401s, which the proxy-error grep cannot see — and given this head's commit message is itself about an escaped unmocked request, it is load-bearing.

Corroborated: the deletion audit is clean

This independently confirms the clearance in my review. Beyond --diff-filter=D being empty, 17 of the 19 symbols the removal commits claim to drop have zero occurrences at both the merge base and this head — they never existed on main. Of the 5 symbols genuinely removed relative to main, all have zero importers at head, and SECRET_PLACEHOLDER_SENTINEL is a rename rather than a loss (now SECRET_PLACEHOLDER at pluginWire.ts:15, with read-side normalization intact). Routes went 15 → 16 with zero removals, and mcpListQuery.ts is byte-identical, so no addressable state was lost.

Net effect

Nothing here changes the verdict or adds a blocker. P1-1 (connector mcp.json full-replace) and P1-2 (skill re-upload visibility) remain the only two blocking items. Everything above is P2 and can ship as follow-ups — with the exception of the i18n:check blindness, which I would raise as its own issue since it is a repo-wide gate defect this PR merely happens to expose.

[yujiawei]

Second addendum — one correction to my own review, one new P2

Verdict unchanged (still CHANGES_REQUESTED on P1-1 / P1-2). Three small items, each verified by execution rather than by reading.

Correction to my review: goCanonicalJSON does not match Go on \b / \f

Under "things I checked that are fine" I wrote that goCanonicalJSON "matches Go's <>&/U+2028/U+2029/\b/\f escapes". The first two are right; the \b/\f part is wrong, and in the opposite direction from what the code assumes.

packages/dmworkmcp/src/api/pluginWire.ts:169-174 converts \b → � and \f → , commented "Go emits �/ where JSON.stringify emits the short \b/\f forms." Executed against a real toolchain (go1.26.5):

json.Marshal        : "a
b
c<>&\b\f"
SetEscapeHTML(false): "a
b
c<>&\b\f"

Go emits the short forms, same as JSON.stringify. Source: encoding/json/encode.go has explicit case '\b': dst = append(dst, '\\', 'b') and the same for \f. That changed at a known boundary:

Go	short-form case '\b':
1.19, 1.21	absent — emits � /
1.22, 1.23, 1.24+	present — emits \b / \f

octo-marketplace/go.mod declares go 1.25.0, so the backend emits short forms and this conversion now introduces the divergence it was written to remove. Nit only — it needs a literal 0x08/0x0C inside a string value plus a server-side byte-comparison path, and the manifest byte-match rule that would have cared is retired. Worth fixing the comment and dropping the two replaces.

Separately, and to pre-empt a wrong fix in the same function: the U+2028/U+2029 replaces are correct and should stay. Go escapes both unconditionally — encode.go says so in as many words ("It is valid JSON to escape them, so we do so unconditionally") and the run above confirms it even with SetEscapeHTML(false).

New P2 — the combined "creator · owner" publisher label is now unreachable

packages/dmworkskillmarket/src/api/skillApiReal.ts:310 sets ownerId: raw.owner_id and :313 sets creatorId: raw.owner_id — the same field. So creatorId === ownerId for every row, and in SkillCard.tsx:102-108 (mirrored at SkillDetailModal.tsx:227) hasComparableIds is true whenever owner_id is present while creatorId !== ownerId is always false. The combined label never renders and the name-comparison fallback is unreachable.

The unified wire genuinely has no creator_id (pluginWire.ts:52,55 — only owner_id and creator_name), and the displayed creator_name is still correct, so this is a display simplification rather than wrong data. Two follow-ups: the comment at SkillCard.tsx:99-101 ("Catalog responses intentionally omit owner_id") is stale now that owner_id is a required wire field, and if the combined signature is still wanted, comparing creator_name !== publisher would work where the ID comparison cannot.

Not a finding, recorded so it isn't filed against this PR

assertSafeExternalURL in skillApiReal.ts:230-243 accepts http: to any host, whereas the expert-side guard (expertService.ts:852-863) allows http: only for loopback. That asymmetry is real but pre-existing — git show 797243d9f:…/skillApiReal.ts:226 is byte-identical to head and the function is untouched by this diff. Worth its own issue to restore parity; it is not a change this PR made.

[Jerry-Xin]

Re-review — PR #1584 (octo-web) @ 4519494

Verdict: APPROVE — re-review after yujiwei's REQUEST_CHANGES (review 5073553715) on head a879f3dad3ef. The substantive delta is one commit, 6c56b5da5 "fix(mcp): echo unmodeled state on full-replace connector/skill writes" (6 files, +168/−5), which addresses exactly the two blocking findings via the discharge paths yujiwei prescribed. The reviewed head 45194942325a on top is an empty CI-retrigger commit — tree byte-identical to 6c56b5da5 (git rev-parse → both 99b6485a09c6), so all evidence below applies to it unchanged. Both blockers were independently byte-verified REAL at the old head and are now closed; every prior redaction fix is intact.

Head composition & drift note

• a879f3dad3ef and 9c3f4ef68f6b remain tree-byte-identical (tree 99912d01bb2f); the whitelist-rebuild approved last round stands.
• 6c56b5da5 is the author's response to yujiwei's two blockers (details below).
• 45194942325a (this head) adds no code — its tree equals 6c56b5da5's; it exists to retrigger CI after two failures unrelated to this PR (verified from the CI logs, see CI section).
• Review drift: my first submit attempt targeted 6c56b5da5; this head landed mid-review, so the review was re-anchored here.

Adjudication of yujiwei's two blockers

🔴 P1-1 (connector edit destroys unmodeled stored state) — REAL at the old head, FIXED here. At a879f3dad3ef, toPluginUpsert built the server object from scratch (const server: Record<string, unknown> = {}) from the six modeled fields and emitted exactly five modeled attachments, while /plugins/upsert replaces plugin_json wholesale — so a slogan edit silently destroyed cwd/disabled/timeout/autoApprove, any second mcpServers entry, and any non-modeled attachment. Verified reachable: McpMarketListPage wires onEdit unconditionally for every 我的发布 row, and Bot/CLI-authored connectors are this PR's own design.

The fix is the exact discharge option (a) from yujiwei's review ("echo unknown mcpServers keys and non-modeled attachments through, mirroring skillApiReal.ts:605-607"), and goes one step further by also seeding the modeled server's own unmodeled keys:

• server is seeded from the RAW stored modeled-server object ({ ...(opts.rawServer ?? {}) }), the six modeled keys are deleted, and the form overlays them — so unmodeled keys survive while clearing a modeled field (deleting env/headers, clearing args/url) still takes effect instead of leaving a stale seeded value.
• Other mcpServers entries are re-emitted verbatim (extraServers, minus the key being written — guarded against collision with the modeled key).
• Stored attachments outside MODELED_ATTACHMENT_PATHS (the five rebuilt ones) are re-emitted verbatim, with the modeled rebuild winning on any path collision.
• The first-key anchor (Object.keys(currentServers)[0]) matches the reader's own "one connector = one MCP server" anchor (mapDetail reads the same first key), so the write echoes exactly what the form displayed.
• The create path is unchanged: createMcpReal passes no rawServer/extras, so the seed is empty and behavior is byte-identical to before.
• The new test is not vacuous — it asserts cwd/disabled: true/timeout survival on the modeled server, a second server not collapsed, and a sixth attachment preserved; each assertion fails against the old toPluginUpsert (which had none of these opts).

Secret-safety of the echo, checked explicitly: env/headers are in MODELED_SERVER_KEYS, so they are dropped from the seed and rebuilt from the form through placeholderSecretMap exactly as before — the seed cannot bypass the placeholder contract. Anything unmodeled that is echoed (a custom key, a second server, an extra attachment) is a value the same backend just served over this session's authenticated detail fetch, written straight back to it — a round-trip, not a new exposure. The disclosed posture for shared env/header literals (P2-5 in yujiwei's review) is unchanged by this commit.

🔴 P1-2 (skill re-upload sends no visibility) — REAL at the old head, FIXED here. At the old head, importBody sent visibility: form.visibility and the only production caller (EditSkillModal) never set it — JSON.stringify drops undefined, so on a documented full-replace import a backend default decided whether a private skill stayed private. The fix does both halves of yujiwei's suggested fix: EditSkillModal now threads skill.visibility, and importBody fails closed with form.visibility ?? "private" — so even a future caller that omits the field cannot widen. Two new tests pin both behaviors (fail-closed default; explicit value threaded through), closing the green-but-uncovered gap yujiwei flagged in the old test.

redactMcpConfig state — unchanged, and the display-vs-functional split

redactMcpConfig.ts is byte-identical between a879f3dad3ef and this head (git diff empty), so the whitelist rebuild approved last round stands as-is: fragment masking, unconditional redactUrl on positional args, root-sibling isolation, per-field type checks. Redaction suite 20/20 locally.

Consumer trace for the display-side silent-drop concern (yujiwei's P2-3): redactMcpConfig has exactly one call site in the codebase — ExpertSpecView.tsx renders the result into a read-only <pre> (with a localized "unavailable" fallback for null). The redacted string is never written back, persisted, posted, or used to spawn/sync a server; the owner's edit flow reads the RAW stored config via /plugins/detail. So the display-side drop of unmodeled keys is a rendering-fidelity issue (correctly graded P2, non-blocking, tracked in yujiwei's review), while the genuinely functional silent-drop was the write path — P1-1 — now fixed. The function's docblock already documents the drop posture ("any root sibling key or unmodeled / wrong-typed server field can never survive verbatim"); the optional user-facing "N fields hidden" notice from P2-3 remains open as a non-blocking follow-up.

Second-pass flag — checked and over-ruled

Codex flagged a potential sidebar state regression (market opens the connector pane while Skills is highlighted) because MARKET_ITEMS[0] is now Skills and remains the path-miss fallback. Traced the full event ordering and it does not hold:

• Fresh NavRail click: Main/index.tsx runs syncPath("/mcp-market") → onPress (which does replaceToRoot(McpMarketListPage) and syncPath("/mcp-market/mcp")) → only then emits wk:nav-menu-activated. React 18 batches the render to after the handler, so MarketSidebar's constructor seeds activeId from currentPath === "/mcp-market/mcp" → mcp. Highlight and content agree.
• Re-entry with the sidebar still mounted: handleNavMenuActivated resolves the item from the same already-synced path and updates activeId — that handler exists precisely to avoid a stale highlight, and it runs after the MCP sync, not before.
• Refresh/deep-link on any /mcp-market/<tab>: constructor seeds from the path; componentDidMount mounts the matching page. Consistent.
• The only genuinely new behavior is a refresh on bare /mcp-market: both path lookups miss, the fallback resolves MARKET_ITEMS[0] (now Skills), and componentDidMount mounts the Skills page — Skills highlight + Skills content, internally consistent, and the deliberate consequence of the PR's announced 技能-first order, which the updated comment above MARKET_ITEMS documents ("this array only drives the sidebar's visual order + the path-miss fallback"). The merge-base comment ("Keep MCP first — it's the original tenant") was retired together with the invariant it described. If the team wants bare-/mcp-market to keep landing on MCP, that is a one-line product call, not a state contradiction — filed here as non-blocking.

Also carried as non-blocking: codex's style-token note on PromptForwardActions/index.css (hard-coded spacing/colors vs token-only rule).

Tests & CI

Local at the reviewed tree:

• pnpm --filter @dmwork/mcp test — 246/246 (22 files; was 245, +1 new connector echo suite).
• pnpm --filter @dmwork/skillmarket test — 136/136 (14 files; was 134, +2 new visibility tests).
• Redaction suite alone — 20/20.

CI failures at 6c56b5da5, both verified from the job logs as unrelated to this PR (which is why the empty retrigger exists):

• Unit tests: Test Files 470 passed (470) / Tests 4299 passed (4299) — the job failed solely on 1 unhandled error, TypeError: target.getClientRects is not a function in prosemirror-view scrollToSelection, originating in chat-composer/ui/__tests__/recoveryHydration.test.tsx teardown ("after unmount" async leak in the tiptap editor). That file and the whole chat-composer surface are untouched by this PR's diff, and the identical-tree-region run at the prior head was green — a teardown-timing flake.
• e2e-p0: 153/154 passed with 0 proxy errors; the single failure is X3-summary-share-space-isolation (Summary-module permission error-state visibility), outside this PR's touched surface (all market specs SK/EX/C37/C39/C40 passed), and green at the prior head on the same tree region.

At this head (45194942325a), Unit tests and e2e-p0 are re-running (in progress at review time); Build / install-build / secret-scan / osv-scan / dependency-review / history are green. Both reruns should land green for merge; nothing in the delta touches the two flaked surfaces.

Remaining items (non-blocking, tracked in yujiwei's review)

P2-1…P2-10 stand as filed: the args redaction residuals (§4.1 recommends rescoping the function as a documented courtesy filter rather than a security control — agreed, and the right long-term home for the real control is publish-time/backend), the connector-detail quick-start surface (accepted product decision), the display-honesty notice (P2-3), write-path visibility echo on absent values (P2-4), the overstated "never posts secrets" wording (P2-5), prompt-id gating (P2-6), profile-by-space-id-only (P2-7), re-slugification (P2-8), skill category fail-closed (P2-9), and the carried cosmetic/doc items (P2-10). None of them block this head per yujiwei's own grading, and none are made worse by this commit.

Credit to yujiwei for both blocker findings — the mechanism proofs, the discharge options, and the precise "mirror skillApiReal.ts:605-607" pointer that this commit follows. The fix is exactly the bounded, one-round closure the review asked for.

[yujiawei]

Code Review — PR #1584 (octo-web)

Reviewed at head 45194942325a7680000d4a23958d99c9b196f35f, merge-base 797243d9f2b77e39f59dd84f1ab0fce7f66e4e81 (110 files, +6494/−2058, 83 commits). This is an independent pass against the current head; no earlier verdict is carried forward.

This PR is classified security-sensitive and carries the needs-human-review label. The review is split into a spec-compliance gate and a code-quality gate.

---

1. Spec compliance

Spec: ✅

Judged against the scope the PR body declares (unified /plugins migration + market UI restructure + forward-to-Bot flow + editor deletion).

• Missing: none identified.
• Extra: none identified. The PR is very large, but every area it touches is declared in the description.
• Divergent: none. I cross-checked the contract against the merged backend and it lines up:
  • $schema ids cowork-plugin-manifest-2.0.json / cowork-plugin-package-2.0.json (mcpWireParams.ts:89,166, skillApiReal.ts:595,627) match the backend's internal/service/plugin/schema.go constants exactly.
  • version is a real field on the upsert request (internal/api/handler/plugin/handler.go, pluginWriteRequest.Version), so skillApiReal.ts:624 is valid.
  • Every sort value emitted (newest / installs / downloads / views / comprehensive) is in the backend's accepted set (internal/repository/plugin/read.go).
  • The icon convention is honoured: the write-canonical icon is echoed, never the presigned icon_url (mcpService.ts:844-845, skillApiReal.ts:591). This matches the backend's documented Icon / IconURL split.
  • Treating public as equivalent to system for the platform badge (publisher.ts:6-7) matches the backend's NormalizeLegacyVisibility.
  • parseTeamAgentsMarkdown (expertWire.ts:221-266) is a faithful inverse of the backend's plugindoc.TeamAgentsMarkdown generator — heading text, - Leader:, numbered ### 策略, - 阻塞: / - 推荐:, ### 权限 all agree.
  • Dropping /plugins/publish is correct — that route does not exist in the merged backend.
  • Echoing package attachments back without storage_uri is the supported round-trip shape (reinjectUpdateStorageKeys in internal/service/plugin/documents.go).

One disclosure gap, not a spec failure: the COMPREHENSION section lists the guarded failure modes for the write path ("secrets are never posted from the web") but does not mention that the migration changes the read path's exposure. See P1-1.

---

2. Code quality

Quality: Changes-Requested

P1-1 — Migrating connector detail onto /plugins/detail publishes literal credentials that the legacy endpoint blanked

packages/dmworkmcp/src/api/mcpService.ts:628-655

const servers = jsonAttachment<McpJSONWire>(raw.plugin_json, "mcp.json")?.mcpServers ?? {};
...
env: env.values,
headers: headers.values,
url: server.url,

packages/dmworkmcp/src/api/quickStartTemplates.ts:115-124

/** ... Values for shared keys pass through verbatim — the author chose to
 *  publish them, and the copy-paste snippet has to include them ... */
out[k] = supplied.has(k) ? formatTokenPlaceholder(k) : v;

The connector detail modal renders buildQuickStartTabs(detail.quickStart) (McpDetailModal.tsx:37,409). Any env/header value that is not flagged user-supplied — plus the full URL including its query string — is emitted verbatim into a copy-paste snippet shown to every viewer of the connector.

This is not merely a pre-existing posture. The PR's own diff records that the protection which made it acceptable is gone:

• Before, McpCreateModal.tsx documented: "the value itself IS persisted (§5.1 relaxation) ... but non-owner reads are blanked server-side (§5.3)".
• This PR replaces that with: "the value itself IS persisted verbatim and returned on every read of the plugin (the backend has NO secret scanner and does NOT blank values)".

I verified this against the merged backend rather than taking the comment on trust. Service.Detail (internal/service/plugin/service.go:349-370) resolves the icon and returns the full Plugin — including PluginJSON — to any caller that passes the visibility scope check. There is no owner check and no value blanking on that path. So for a space- or system-visible connector, this PR turns a field that used to be blanked for non-owners into one that is rendered to everyone in scope.

Compounding this: McpCreateModal.tsx:879-886 still carries the stale justification —

// Backend no longer rejects secret-shaped shared values (rule 2 was removed);
// non-owner blanking (§5.3) is the sole guard keeping author tokens
// out of consumer-facing responses.

— while the only remaining guard is a non-blocking ⚠️ warning next to Submit (sharedSecretLeaks, lines 1024-1033, 1216-1228). The stated rationale for keeping it non-blocking ("§5.3 blanks it anyway") no longer holds.

The PR added redactMcpConfig precisely because "a plugin's mcp.json is rendered to every viewer of its detail", but wired it only into ExpertSpecView.tsx:42. The connector surface — the one whose own author-typed values are most likely to be real credentials — is left unprotected.

Asks: apply a fail-closed redaction (or a hard publish-time block on secret-shaped shared values) to the connector detail/copy output, and fix the stale comment at McpCreateModal.tsx:879-886 so the next reader is not told a guard exists that does not.

P1-2 — The new redaction helper misses the most common real-world credential shapes

packages/dmworkmcp/src/utils/redactMcpConfig.ts:58-116

The docstring claims "the URL's query + userinfo + fragment are masked, args are redacted". I extracted the two functions verbatim and ran them; five inputs pass through unmasked:

input	output
redactUrl("//user:password@mcp.example/sse")	//user:password@mcp.example/sse
redactUrl("/sse#access_token=sk-live-SECRET")	/sse#access_token=sk-live-SECRET
redactArgs(["--header","Authorization: Bearer sk-live-SECRET"])	["--header","authorization: Bearer sk-live-SECRET"]
redactArgs(["run","-i","--rm","-e","API_KEY=sk-live-SECRET","img"])	unchanged — secret intact
redactArgs(["some-mcp","sk-live-POSITIONAL"])	unchanged — secret intact

Root causes:

1. redactUrl lines 60-67 — a URL without a scheme throws in new URL(), and the catch branch only strips a query string. Userinfo and fragment survive on protocol-relative and relative URLs. The absolute path masks the fragment correctly (line 76), so this is an inconsistency, not a design choice.
2. redactArgs line 104-108 — maskNext only fires when the flag name itself is secret-shaped. --header and -e are not, so the following token is treated as a bare positional. The docker run -e API_KEY=... form is how a large share of MCP servers are actually configured.
3. redactArgs line 113 — a bare positional only goes through redactUrl, which returns it unchanged when there is no query string.

There is also a correctness side effect at line 113: redactUrl is applied unconditionally to every bare positional, and new URL("Authorization: Bearer x") succeeds (it parses authorization: as a scheme). That is why row 3 above comes back lowercased. Any positional shaped Word:rest is silently mutated on the display surface.

Asks: mask userinfo/fragment on the relative-URL branch; extend the value-following-flag rule to cover --header/-H-style flags and KEY=VALUE tokens with no leading dash; and either narrow redactUrl's use on positionals to values that actually look like URLs, or tighten the docstring's residual list — right now it promises more than the code delivers.

P2-1 — Connector edit echoes stored attachment paths into the trusted upsert without validation

packages/dmworkmcp/src/api/mcpWireParams.ts:152-154

for (const att of opts.extraAttachments ?? []) {
  if (!MODELED_ATTACHMENT_PATHS.has(att.path)) attachments.push(att);
}

Fed from current.plugin.plugin_json?.attachments (mcpService.ts:873) — a backend response promoted straight back into a write. The sibling skill path already guards exactly this with isSafeAttachmentPath (skillApiReal.ts:252-260, rejecting empty / absolute / backslash / NUL / .. paths). The connector path should reuse it. Low likelihood, near-zero cost to fix, and the asymmetry between two paths written in the same PR is itself worth closing.

P2-2 — Renaming a connector's slug onto a sibling server key silently drops that server

packages/dmworkmcp/src/api/mcpWireParams.ts:129-136

const serverKey = slug || name;
for (const [k, v] of Object.entries(opts.extraServers ?? {})) {
  if (k !== serverKey) mcpServers[k] = v;
}
mcpServers[serverKey] = server;

For a stored document {a: edited, b: other}, renaming the edited server's slug from a to b skips b in the loop and then overwrites it. Since /plugins/upsert is full-replace, other is permanently gone.

Narrow but real: the web JSON import takes only the first server (importJson.ts:12), so multi-server documents come from octo-cli / backfill / direct API writes — which is precisely the case extraServers was added to protect. Reject the collision (or key the write by the stored server name and rename separately).

P2-3 — Unknown skill visibility degrades to space and is then written back

packages/dmworkskillmarket/src/api/skillApiReal.ts:320-326 maps an unrecognized wire value to "space", and EditSkillModal.tsx:394 feeds that normalized value straight into the full-replace update:

visibility: skill.visibility,

space is more permissive than private, so on a write path this is fail-open. Note updateSkill already has the right behaviour built in — const visibility = form.visibility ?? plugin.visibility (line 590) would echo the raw wire value if the modal simply omitted the field. The connector path does exactly that (mcpService.ts:869 echoes current.plugin.visibility unmapped) and is correct.

Not reachable today — the backend enum is closed and visibility is non-optional in the response — so this is defense-in-depth, not a live bug. Either drop visibility from the modal's payload or make the unknown fallback private.

P2-4 — spaceId localStorage fallback bypasses the app's canonical resolver

packages/dmworkskillmarket/src/components/InstallPromptModal.tsx:12-19 and packages/dmworkmcp/src/components/McpConnectModal.tsx:16-23 both add:

WKApp.shared?.currentSpaceId ||
  (typeof localStorage !== "undefined" ? localStorage.getItem("currentSpaceId") || "" : "")

The app-wide X-Space-Id resolver has no such fallback — apps/web/src/index.tsx:62-64 returns WKApp.shared.currentSpaceId and nothing else. The resolved id is used both to list owned bots and to stamp the outgoing DM (PromptForwardActions/index.tsx:226), so a stale value during an account switch or before space hydration targets the previous selection.

I acknowledge this matches the existing pattern in skillApiReal.getAuthHeaders / BotPublishModal (pre-existing, outside this diff), so the PR is being internally consistent. But it propagates the deviation to two new surfaces. Worth a decision: either align the package on WKApp.shared.currentSpaceId, or write down why the market packages need the fallback that no other caller does.

P2-5 — Dead code the deletion pass missed

• packages/dmworkmcp/src/api/expertWire.ts:30-203 — mapAgentListItem, mapAgentDetail, mapSquadListItem, mapSquadDetail, fromSkillWire and their wire interfaces (SkillWire, ExpertAgent*Wire, ExpertSquad*Wire, SquadMemberWire) have no production caller after the migration. Only expertWire.test.ts keeps them alive, which means part of the reported "220 pass" covers code nothing reaches.
• expertService.ts:520 maps permission from the parsed AGENTS.md, but nothing renders it — the only consumer was the deleted full-page editor. (strategies is still rendered at ExpertDetailModal.tsx:217.)
• skillApiReal.ts:357 — fileUrl: managedZip?.storage_uri ?? .... Per the backend, the 2.0 read path strips storage_uri and never returns it, and no UI in the package consumes fileUrl. Dead field.

P2-6 — SkillCard re-introduces an unused prop binding that was previously suppressed on purpose

packages/dmworkskillmarket/src/components/SkillCard.tsx:78

export default function SkillCard({ skill, categories, onOpen, ... })

The prior code was categories: _categories plus an explicit void _categories;. This PR renames it back to categories and drops the void, but the binding is still never read anywhere in the file — and categories remains a required prop that all callers still pass. Note the CI check list on this PR has no lint job, so an unused-binding or exhaustive-deps regression would not be caught here.

Related, same class: McpCreateModal.tsx:566-582 — the category-loading effect reads editing but declares only [visible] as its dependency.

Nits

• PromptForwardActions/index.css — .wk-prompt-forward__copy-icon is position: absolute; z-index: 1; opacity: 0 with no pointer-events: none. It is inside the hovered container, so there is no invisible click trap on desktop, but it does occlude a 28×28 region of the editable textarea, and on a touch device (no hover) the only copy affordance is permanently invisible yet clickable.
• installPrompt.ts:11 reassigns the spaceId parameter in place.
• The e2e MSW fixtures pin $schema: "cowork-plugin-package-1.0.json" (e.g. skill-market-list.ts:56) while the write paths emit 2.0. Reads do not validate $schema, so this is harmless today, but it is a fixture that will not catch a contract drift.
• McpMarketListPage.componentDidMount applies category / tag from the URL in the mine variant, where the category pills are hidden — a bookmarked filter would apply with no visible control to clear it.
• MyAssetsPage mounts SkillListPage from @dmwork/skillmarket and MineTable is imported cross-package into dmworkmcp pages. It works (both i18n namespaces register at startup), but it is worth a note in the module-boundary section of the description.

---

3. Overall verdict

REQUEST_CHANGES

Spec ✅, but Quality is Changes-Requested on P1-1 and P1-2, so the combined gate blocks.

To be clear about what is not blocking: the wire mapping, the fail-closed category resolution, the icon-intent sentinel, the unmodeled-state echo on full-replace writes, the 404-only relation drop, the shell-safe space-id sanitizer, and the parseTeamAgentsMarkdown region gating are all well built, and the unit tests behind them assert real behaviour rather than tautologies. The two P1s are both about the same thing: literal credentials on read surfaces.

---

4. Suggested direction

1. P1-1 — decide the posture explicitly, then implement it: either redact the connector quick-start snippet the way ExpertSpecView redacts the expert one, or block publish when a shared (non-user-supplied) value is secret-shaped. Whichever you pick, correct the stale §5.3 comment at McpCreateModal.tsx:879-886 in the same change.
2. P1-2 — three targeted fixes in redactMcpConfig.ts: mask userinfo + fragment in the relative-URL catch branch; extend the flag-value rule to --header/-H and to dashless KEY=VALUE tokens; gate redactUrl on positionals that actually look like URLs. Add the five inputs from the table above as regression cases — the current suite covers the shapes that already work.
3. P2-1/P2-2 — reuse isSafeAttachmentPath on the connector path; reject a slug rename that collides with an existing mcpServers key.
4. P2-5 — delete expertWire.ts's legacy mappers and their tests in the same sweep that removed the editors.

---

5. Additional observations

Please verify manually before merge (security-sensitive):

• Deploy coupling. octo-marketplace#67 (merged 2026-08-26) and #72 (merged 2026-08-31, f9216835) are both merged, but I cannot verify either is deployed. By the author's own disclosure this branch hard-breaks the write paths against a pre-#72 backend (2.0 $schema → 400, skill version → strict-decoder 400, connector create silently placement-less). There is no client-side dual-version tolerance. Confirm the backend is live before releasing this.
• Exposure decision on P1-1. Whether org-wide (space) or platform-wide (system) visibility of author-typed connector env/header/url values is acceptable is a product call, not a code call. It needs a named owner.
• Round count. This PR has accumulated 40 formal reviews across 83 commits and 110 files. Well past the point where incremental review rounds are the right tool: each pass is finding a new instance of the same class rather than converging. My recommendation beyond this verdict is to stop iterating on this branch and split it — the unified-backend API migration (which I verified is contract-correct and is the load-bearing half) can land on its own, separately from the UI restructure and the editor deletion. That gives each half a review surface a reviewer can actually hold in one pass. This needs the dispatcher's decision, not another round.

Verification coverage / what I did not check:

• Unit tests and e2e-p0 were still pending on this head at review time; Build, secret-scan, osv-scan, dependency-review were green. I could not run the suites locally (no installed node_modules in this checkout), so the "136 pass / 220 pass" claim in the description is unverified by me.
• I verified the frontend↔backend contract by reading the merged octo-marketplace sources (schema ids, upsert fields, sort enum, visibility model, team-markdown generator, storage-key round-trip, Service.Detail scoping). I did not audit the backend's space/private authorization beyond the detail path.
• No browser was driven: CSS, responsive layout, and real a11y behaviour of the new MineTable / sort rows / editable prompt are unexercised.
• There is no lint job in this PR's check list, so lint-class regressions (P2-6) are not covered by CI either.
• A second opinion pass was run over the dmworkbase slice only; its three flagged items (telemetry silently dropped via the removed route gate, the preview prop removal breaking callers, and the draft-reset effect) I checked and refuted — all four forward surfaces do pass copyTrackEvent (McpBotPublishModal:58, McpConnectModal:55, InstallPromptModal:47, BotPublishModal:55) with expert/squad intentionally omitting it; no remaining caller passes preview; and the draft-reset effect keys on a string, so it only fires on a genuine content change.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Reviewer: Octo-Q (automated review)
PR: #1584 — feat(market): unified plugin backend migration + market UI restructure
Head SHA: 45194942325a7680000d4a23958d99c9b196f35f · merge-base: 797243d9f2b77e39f59dd84f1ab0fce7f66e4e81 (origin/main) · Routing: security_sensitive
规模: 110 files, +6494/−2058。审查方式: 本地 PR checkout 上 git diff pr-base...HEAD 全量逐文件过 + 数据流回溯 + 高风险 bucket 证明；未运行构建/测试（静态审查）。

---

Code Review — PR #1584 (octo-web)

Summary

This PR migrates all three market surfaces (connector/MCP, expert, skill) off the legacy per-type catalog endpoints onto the octo-marketplace unified plugin API (/plugins, /plugins/detail, /plugin_categories, /plugins/upsert|import|delete|install, plugin_type=connector|expert|expert_team|skill) and restructures the market UI: the per-market 全部/我的 tabs are replaced by a single 我的发布 page (/mcp-market/mine, MyAssetsPage + shared MineTable), card primary actions become prompt-forwarding flows (连接/添加), and sorts narrow to 最新/最热. The connector write path is rebuilt as a full-replace upsert that reshapes the flat form into manifest_json + plugin_json (mcp.json + connector/* attachments) with explicit echo of unmodeled state (rawServer / extraServers / extraAttachments) and a client-side-only secret regime: user-supplied keys are written as ${KEY} placeholders because the backend secret scanner was deliberately removed. The migration is carefully constructed overall — fail-closed category resolution, 404-only relation drops, an icon write-intent sentinel, and solid tests around the new write path. No P0/P1 issues found; two P2 items and two nits below, all non-blocking.

Verification

Static analysis only at head 45194942325a7680000d4a23958d99c9b196f35f (merge-base 797243d9); build and tests not executed in this environment.

• ✅ Secret round-trip is stable — writer placeholderFor (packages/dmworkmcp/src/api/mcpWireParams.ts:183) and reader selfPlaceholder (packages/dmworkmcp/src/api/pluginWire.ts:186) apply identical normalization (trim → non-alphanumerics→_ → uppercase, VALUE fallback); cross-referential ${SHARED_TOKEN} values pass through verbatim on both sides; SECRET_PLACEHOLDER blanked on read and write. Pinned by mcpService.visibility.test.ts.
• ✅ Full-replace upsert preserves unmodeled state — toPluginUpsert re-emits unmodeled server keys, other mcpServers entries, and non-modeled attachments; round-trip proven by mcpService.connector.test.ts against the real service functions (axios mocked at the instance boundary).
• ✅ Category resolution fails closed on list and write paths — unresolved filter returns an explicit empty result with pills (mcpService.ts:fetchMcpListPath), unresolved write category refetches once then throws (resolveWriteCategory); covered by mcpService.connector.test.ts and expertService.categoryFailClosed.test.ts.
• ✅ No dangling references to removed surfaces — downloadSkill / getDownloadUrl, SECRET_PLACEHOLDER_SENTINEL, buildMcpCategoryParams, and the old /mcps|/skills|/experts|/squads CRUD paths have zero remaining callers; FetchRules/TrackRules migrated in lockstep.
• ✅ i18n completeness — every new key used by MineTable, 我的发布 page, sort/visibility labels, and prompt-forward chrome exists in both zh-CN and en-US bundles of all three packages.
• ✅ e2e/MSW fixtures re-shaped to plugin wire and specs updated to the new routes (/mcp-market/mine?type=mcp), matching the client's request shape (page pagination, plugin_id query params).

Findings

No P0/P1 issues; two P2 items and two nits, all non-blocking.

P2 — Multi-server mcp.json: primary-server pick disagrees between read and write (packages/dmworkmcp/src/api/mcpService.ts:637)

mapDetail treats the FIRST mcpServers key as the connector's server (:637, mirrored by updateMcpReal at :854 when seeding rawServer/extraServers), but stored mcp.json is serialized with sorted keys (goCanonicalJSON, packages/dmworkmcp/src/api/pluginWire.ts:153), so the read primary is the alphabetically-first key — while the write targets serverKey = slug || name derived from manifest.name (packages/dmworkmcp/src/api/mcpWireParams.ts:133-136). Records this UI creates are single-server and slug-keyed, so this is latent; but for any externally-created/backfilled record with multiple mcpServers entries whose alphabetically-first key differs from manifest.name, the detail/edit form shows one server's connection fields while save overwrites the slug-keyed entry with them, destroying that entry's original config. Resolve the primary by manifest.name / connector.source (falling back to first key) and add a multi-server edit round-trip test — the existing test asserts preservation but not primary-pick consistency.

P2 — Expert /plugin_tags call omits scene_code (packages/dmworkmcp/src/api/expertService.ts:582)

listExpertTagsReal sends only { plugin_type }, while the connector and skill callers of the same unified endpoint always send scene_code: SCENE_CODE (mcpService.ts fetchMcpTagsReal, skillApiReal.ts getSkillTags). With a single scene today behavior is identical, but the asymmetry would make expert tag aggregation span scenes the moment a second scene exists. Add scene_code: SCENE_CODE for parity.

Nit — Stale comment on connector card primary action (packages/dmworkmcp/src/components/McpCard.tsx:133)

Comment says the visible text is 添加到 Bot, but the label is t("mcp.card.connect") (连接).

Nit — Dead ArrowDown branch (packages/dmworkskillmarket/src/pages/SkillListPage.tsx:274)

ArrowDown renders behind option.descending, but neither new SORT_OPTIONS entry sets descending, so the icon never renders.

Human-verify

1. The legacy-bearer migration shim was deleted with the mapDetail rewrite: the old mapper synthesized a user-supplied Authorization slot for pre-toggle records carrying auth_type: "bearer". If the marketplace backfill does not materialize that marker as an explicit Authorization: ${AUTHORIZATION} header row in mcp.json, pre-toggle connectors lose the Authorization line from their quick-start snippet after migration. The backfill is outside this repo — not a merge blocker for this PR, but worth confirming.
2. Secret posture change: the backend no longer blanks anything, and values under shared (non-user-supplied) env/header keys are returned to every viewer of a plugin detail. The new write path is strictly safer (user-supplied secrets are never sent; placeholders only) and the expert spec view masks via redactMcpConfig, but legacy records whose owners stored real secrets under shared keys relying on the old server-side non-owner blanking are now exposed to all viewers. Worth a backend-side audit/masking decision — not a merge blocker for this frontend PR.

Things I checked that are fine

• Relation fan-out (loadSkills / loadSquadMember) drops only CONFIRMED 404s; 500/403/network rethrow; cancellation propagates; skills[]/pluginIds[] stay index-aligned.
• Skill package fetch keeps the streaming 20 MiB cap, pre-parse zip entry-count cap, and timeout; marketplace-relative download paths attach token + X-Space-Id headers; external URLs remain scheme-guarded.
• Visibility fallbacks degrade unknown values to space (never public) in both mapVisibility and normalizeVisibility; system preserved end-to-end for the official badge (publisher.ts in both packages).
• Shell-safe space-id sanitizer duplicated byte-identically into packages/dmworkskillmarket/src/utils/spaceId.ts; all prompt builders interpolate the sanitized value and derive apiBaseUrl from new URL(...).origin.
• Skill metadata-edit upsert validates echoed attachment paths (isSafeAttachmentPath) and fails closed on absent visibility (?? "private" on import; current visibility echoed on edit/re-upload).
• parseTeamAgentsMarkdown fails closed until the ## 协作方式 region, so summary-prose injection cannot seed team config.
• Copy tracking moved to the clipboard-success branch with per-surface copyTrackEvent; category-cache failures do not poison the per-Space cache; handleSpaceChanged_ preserves the forced mine mode.

Verdict: APPROVED

The migration is contract-complete and fail-closed where it matters; the two P2 items are latent robustness/parity issues (multi-server record pick, missing scene_code) and the nits are cosmetic. Nothing here makes a working path unavailable or produces wrong data on any path this product can create, so this is an approve with non-blocking suggestions.

---

Octo automated review补充段

数据流回溯（被消费数据 → 上游来源 → 是否真流到消费点）

• McpDetail.quickStart.{transport,url,command,args,env,headers} ← mapDetail 解析 plugin_json 的 mcp.json attachment（mcpService.ts:631-640）→ 上游是 upsert 写入的 goCanonicalJSON({mcpServers})（mcpWireParams.ts:121）。单服务器记录键=slug，读端取首键，闭环成立；多服务器记录存在读/写主键不一致（见 P2-1）。
• env/headers 显示值 ← splitUserSupplied（pluginWire.ts:196）：自引用占位符 → 空槽 + userSupplied；跨引用 ${SHARED_TOKEN} → 原样；SECRET_PLACEHOLDER → 空。写端 placeholderSecretMap（mcpWireParams.ts:203）完全对称（归一化算法逐字一致，"${" + normalized + "}"，fallback VALUE）。round-trip 字节稳定，mcpService.visibility.test.ts 钉住。
• McpDetail icon 写入值 ← 模态框 iconIntent 三态（新上传 key / "" 删除 / undefined 未动，McpCreateModal.tsx:908-916）→ updateMcpReal 对 undefined 回显 current.plugin.icon（canonical 列，非 presigned icon_url，mcpService.ts:canonicalIcon）→ 测试 mcpService.connector.test.ts 三态全覆盖。数据真实流动，无预设空值短路。
• Skill 列表/详情 ← listPlugins→mapSkill / getSkill→mapSkillDetail（skillApiReal.ts）：readmeContent/fileName/fileSize 全部改从 plugin_json attachments 重建（legacy skill/ref.json 与 tree 形态双路径），nextCursor 由 offset 分页合成（page*pageSize < total）。useSkills 对 cursor 保持 opaque，翻页链路成立（skill-market-pagination.ts MSW 夹具按 page_size=1/total=2 验证 page=2 追加）。
• ExpertAgent/ExpertSquad 详情 ← pluginDetail(include_relations=true) + liveRelations 按 relation_type 过滤 + sort_order 排序 → loadSkills/loadSquadMember fan-out。expertSkillIndex/squadSkillIndex 位置对齐缓存（仅 fulfilled 成对 push），冷缓存时 skillPluginIdForExpert/Squad 重取 detail 重建索引 —— 深链直达场景可达。
• 分类映射（name↔UUID）← /plugin_categories 按 Space 缓存；写路径 resolveWriteCategory 未命中 → 清缓存重取一次 → 仍未命中抛错（不写 NULL category）；列表过滤未命中 → 显式空结果不放宽。两条路径都 fail-closed，测试钉住。
• MineTable 行数据 ← 三个市场页各自把 Skill/McpListItem/ExpertItem 映射为 MineRow；visibility 经 normalizeVisibility（expert）或直接透传（已知集合），标签经 t(\mcp.visibility.${v}`)/t(`skillMarket.visibility.${v}`)`，key 全集在双语包中存在（已逐一核验）。

盲点 checklist（security_sensitive → C1–C6 全项）

• C1 双路径 parity — ✅ 通过。create↔update：create 走 POST /plugins/upsert（visibility 硬编码 space，与旧 POST /mcps 不带 visibility 的默认语义一致）；update 先读当前记录回显 visibility + rawServer/extraServers/extraAttachments，两端均经 toPluginUpsert 同一构造器。读↔写占位符对称（见上）。SECRET_PLACEHOLDER_SENTINEL 删除后全仓无残留引用；spaceId.ts 本地副本与 dmworkbase/src/Utils/spaceId.ts 函数体逐字一致。守卫等价写法：/plugins/* 全部字面路径，无 :id 通配，FetchRules 旧 IGNORE 钉子随之移除且有测试锁定（FetchRules.test.ts 新增断言）。
• C2 control-flow ordering / 嵌套复用 — ✅ 通过（1 处 P2 残留）。toPluginUpsert 被 create/update 两处调用，参数契约一致；PromptForwardActions 被 5 个模态框复用，kind/copyTrackEvent 按面传入，复制计数只在剪贴板成功分支发一次。非 canonical 形式试穿：sanitizeShellSpaceId 拒绝前导 -/.（option 注入）、..、空白与 shell 元字符（正则 [A-Za-z0-9._-]{1,128} + 双守卫）；parseTeamAgentsMarkdown 对 summary 区注入 ### 策略/- Leader: 闭区间（未进 ## 协作方式 一律忽略）。残留：多服务器记录读/写主键选择（P2-1）。
• C3 授权边界 ≠ 能力边界 — ✅ 通过。无新增免鉴权面：所有 /plugins/* 调用经同一 axios 拦截器（token + X-Space-Id + Accept-Language）或 requestEnvelope（auth 头 + 401 处理；指标 beacon 401 不触发登出重定向，skipAuthRedirect）。fetchSkillPackage 对 marketplace 相对路径补 auth 头、对外部 URL 保留 scheme 白名单守卫。可见性过滤依赖服务端（mode=mine、visibility 作用域），客户端未自行放宽：未知 visibility 一律降级 space 而非 public。
• C4 授权生命周期 / 掩码 ≠ 授权 — ⚠️ 已通过（带 2 条 human-verify）。redactMcpConfig 明确定位为 display-only（专家详情 mcp.json 展示），不作为授权边界；真实授权在服务端按 plugin visibility 执行。掩码构造白名单重建、fail-closed（不可解析 → null → 本地化"不可用"提示）、URL query/userinfo/fragment 全掩、args 密钥旗标掩 —— 实现本身无绕过。掩码之下的数据面变化（服务端不再 blank 非 owner 读取）列入 human-verify #2。
• C5 build/静态通过 ≠ 运行期路径 — ✅ 通过（静态推演）。桌面端（绝对 apiURL）：resolveSkillMarketApiBaseURL / resolveBaseURL 都取 new URL(apiURL).origin 前缀，fetchSkillPackage 同源拼接一致；Web 端 same-origin 走网关。未以"构建会过"缓解任何疑点；下载/探针等运行期路径均按请求构造逐一核对。
• C6 治理/策略文档自洽 — ✅ 通过（N/A 偏多）。本 PR 无 SECURITY.md/披露流程类治理改动；.i18n/scan-config.json 新增豁免均附理由且与既有同类豁免一致（prompt 模板与 wire-format 解析标记不可翻译）；e2e case-specs 与 UI 删除（结果摘要）同步更新，无漂移。

跨轮 blocker 复检（R6）

N/A —— 本 issue 无上一轮审查记录（首次审查，评论历史为空）。

每个 finding 的 diff-scope 三问

• P2-1 多服务器主键：① 本 PR 新引入（旧 PATCH 从不重建 mcp.json，读端旧路径用服务端 quick_start 块）；② 非既有行为——本 PR 同时引入"读=字母序首键"与"写=slug 键"两个新契约；③ 按 R1 检验：仅对外部创建的多服务器记录（本产品内不可创建，import 只取首服务器）产生错误数据，产品内路径不可达 → 定 P2 非 P1。
• P2-2 scene_code 缺失：① 本 PR 新引入（新端点调用）；② 无既有行为可援引；③ 单 scene 下无用户可见影响 → P2。
• human-verify #1（bearer shim 删除）：① 本 PR 删除；② 旧行为（合成 Authorization 槽）被本 PR 显式移除，是否回归取决于 checkout 外的 backfill → 按不可验证契约处理，列 human-verify 不阻塞。
• human-verify #2（secret posture）：① amplified（服务端去 blank 是后端决策，本 PR 客户端如实消费并加 display 掩码 + 写端不再发送用户密钥，净效果对新建记录更强）；② 旧 §5.3 服务端掩码契约被上游废弃；③ 前端无法单方面恢复该保障 → human-verify 不阻塞。

[Octo-Q] verdict: APPROVE — 统一插件 API 迁移契约完整、失败路径 fail-closed、密钥 round-trip 与全量替换回显均有生产路径级测试钉住；零 P0/P1。仅 2 个潜在健壮性/一致性 P2（多服务器 mcp.json 读/写主键不一致、expert /plugin_tags 缺 scene_code）与 2 个 nit，均不阻塞；2 条 human-verify（legacy bearer backfill、共享键密钥暴露 posture）需人工向后端确认，均非本 PR 合并阻塞。

[yujiawei]

Review addendum — head 45194942325a7680000d4a23958d99c9b196f35f

A follow-up pass surfaced three more items and closed one open question from my earlier review. The verdict is unchanged (changes requested) — the two P1s stand. These are additions, plus one correction to my own earlier report.

---

New P2 — a no-op open-and-save destroys a multi-server mcp.json (supersedes my earlier "slug rename" item)

Read and write disagree on which mcpServers entry is the connector's server:

• Read takes the first key: Object.keys(servers)[0] — mcpService.ts:637 (mapDetail) and mcpService.ts:854 (updateMcpReal, seeding rawServer/extraServers).
• Write targets serverKey = slug || name, derived from manifest.name — mcpWireParams.ts:129-136.

Because stored documents are serialized with sorted keys (goCanonicalJSON → stringifySortedKeys, pluginWire.ts:153), "first key" means alphabetically first, which need not be the slug.

I simulated the round-trip with the real key logic. Stored {alpha: {...}, zeta: {...}}, manifest.name = "zeta", user changes nothing and saves:

read primary key    : alpha   -> form shows alpha's url; form slug = zeta
write serverKey     : zeta
RESULT              : {"zeta": {"url":"https://alpha.example","env":{"A":"1"}}}
servers before/after: 2 -> 1

alpha is dropped (it is excluded from extraServers as the "current" server, then never re-emitted under its own key) and zeta's original config is overwritten with alpha's. Since /plugins/upsert is full-replace, both are permanently gone. No rename and no user error required.

This is the same root cause as the slug-rename collision I filed earlier — please treat them as one bug, not two. Suggested fix: resolve the primary server by manifest.name / connector.source, falling back to the first key, and use that same key on both sides. A multi-server round-trip test would pin it; the existing test asserts preservation but not primary-pick consistency.

Reachability caveat: this UI cannot create multi-server documents (importJson.ts:12 takes the first server and warns), so it only affects externally-created or backfilled records. That is exactly the population the extraServers machinery was added to protect, so if backfilled multi-server connector records exist in production, this should be re-rated P1. Worth a data check before merge.

New P2 — expert tag aggregation omits scene_code

packages/dmworkmcp/src/api/expertService.ts:580-586

const data = await get<{ name: string; count: number }[] | null>(
  "/plugin_tags",
  { plugin_type: pluginTypeOf(kind) }
);

Both sibling callers of the same endpoint send it — mcpService.ts:1063-1066 (fetchMcpTagsReal) and skillApiReal.ts:486-487 (getSkillTags). Identical behaviour under a single scene, but expert tag aggregation would silently span scenes the moment a second one exists. Add scene_code: SCENE_CODE for parity.

New nit — dead descending branch

packages/dmworkskillmarket/src/pages/SkillListPage.tsx:274 renders <ArrowDown/> behind option.descending, but neither surviving SORT_OPTIONS entry sets descending (lines 34-37), so the icon is now unreachable.

---

Closed: the deleted legacy-bearer shim is fine

I flagged nothing here earlier, and it is worth recording that I checked it. The merge-base mcpService.ts:590-610 carried a migration shim that synthesized a user-supplied Authorization row for pre-toggle records with auth_type: "bearer"; this PR's mapDetail rewrite removes it, and no auth_type handling remains in the API layer.

That removal is correct — the marker is now materialized server-side. octo-marketplace/internal/backfill/plugin/connector.go:87-93 folds a legacy authType: "bearer" into headers["Authorization"] = "${AUTHORIZATION}" when no explicit Authorization row exists, and its placeholderFor / envPlaceholderName normalization mirrors the frontend's, so splitUserSupplied reads the slot back correctly. No action needed and no backend confirmation required.

Correction to my earlier report

I wrote that the comment at McpCard.tsx:133 should be checked against the button label. It is accurate as written: mcp.card.connect resolves to "添加到 Bot" (zh-CN) / "Add to Bot" (en-US), matching the comment. The key name reads like "连接" but the value does not. No change needed there.

[yujiawei]

Code Review — PR #1584 (octo-web)

Reviewed at head 542925e03ec7966e93e538d686e011c6db3707b9, merge-base 797243d9f2b77e39f59dd84f1ab0fce7f66e4e81 (111 files, +6663/−2079, 84 commits). Independent pass against this head; no earlier verdict is carried forward.

The delta since the previously reviewed head 45194942325a is exactly one commit — 542925e03 "fix(mcp): mask secret-shaped values on connector detail + harden redaction" (6 files, +196/−48) — responding to the P1-1 / P1-2 / P2-1 asks from the last round. Everything below is verified against this head.

---

1. Spec compliance

Spec: ✅

• Missing: none new. The delta implements the three asks it set out to: connector-detail masking, redactMcpConfig hardening, and attachment-path validation on the connector write path.
• Extra: none. No scope was added; the commit touches only packages/dmworkmcp.
• Divergence: none at the spec level. The cross-repo deploy coupling on octo-marketplace#72 is disclosed in the description, and the unified-plugin wire contract (2.0 $schema ids, /plugins/upsert shape, visibility model, sort mapping, team-markdown round-trip) is unchanged from the head I verified against the merged backend last round.

One disclosure gap, not a spec failure: the delta changes read-path rendering behaviour — a shared (non-user-supplied) env/header whose key name is secret-shaped now renders as a <把这里换成你的 KEY> placeholder instead of its literal value, with no author opt-out. That is a deliberate and defensible posture change, but it is not mentioned in the PR description, and it means a genuinely shared service-account value can no longer be published in a working copy-paste snippet. Worth one line in the description.

What was verified as correctly fixed

I extracted the two changed helper modules and executed them against the exact inputs from the last round's failure table. All four previously-failing redactMcpConfig shapes are now masked:

input	before	now
redactUrl("//user:pw@mcp.example/sse")	passthrough	//REDACTED@mcp.example/sse
redactUrl("/sse#access_token=…")	passthrough	/sse#REDACTED
redactArgs(["--header","Authorization: Bearer …"])	passthrough + silently lowercased	["--header","••••••"]
redactArgs(["run","-i","--rm","-e","API_KEY=…","img"])	passthrough	["run","-i","--rm","-e","••••••","img"]

The hasUrlShape guard also fixes the correctness side effect: ["Authorization: Bearer keepme"] now comes back byte-identical instead of being mutated to authorization: … by new URL(). Benign tokens (--rm, img, -p 5432:5432, REGION=us-east-1) survive. mcpWireParams.ts:43-53's isSafeAttachmentPath is a verbatim match for skillApiReal.ts:251-259, closing P2-1. The stale §5.3 comment at McpCreateModal.tsx:879-885 is corrected and now describes the backend's actual posture.

---

2. Code quality

Quality: Changes-Requested

The commit fixes the two P1s it names. The blocking problem is that it stops one field short on the surface it was written to protect: args on the connector detail is not redacted at all, and that is the field the commit's own new test says must be.

P1-1 — Connector detail publishes args verbatim; the PR's own redactArgs is never wired to this surface

packages/dmworkmcp/src/api/quickStartTemplates.ts:99-102

const server: Record<string, unknown> = {
  command: qs.command ?? "npx",
  args: qs.args ?? [],          // ← no masking of any kind
};

packages/dmworkmcp/src/api/quickStartTemplates.ts:248-252 (prompt tab, same data)

const args = (qs.args ?? [])
  .map((a) => (/\s/.test(a) ? `"${a.replace(/(["\\])/g, "\\$1")}"` : a))
  .join(" ");

The payload is raw backend data: mcpService.ts:650 maps args: server.args straight off the stored mcp.json, and McpDetailModal.tsx:409 renders buildQuickStartTabs(detail.quickStart) to every viewer within the plugin's visibility scope — org-wide for space, platform-wide for system. The commit closed url (partly), env, and headers on that surface and left the fourth credential carrier open.

This is not a hypothetical shape. The commit's own new regression test pins exactly it:

packages/dmworkmcp/src/utils/__tests__/redactMcpConfig.test.ts:103-118

args: ["run", "-i", "--rm", "-e", "API_KEY=sk-live-DOCKER", "img"],
...
expect(out).not.toContain("sk-live-DOCKER");

— but redactMcpConfig has exactly one production caller, ExpertSpecView.tsx:42. The connector surface never calls it. A URL carrying a token inside args is equally exposed: args: ["-y","mcp-remote","https://mcp.example/sse?access_token=sk-live-ARG"] renders verbatim in both tabs, even though the very next field (url) is now masked.

It is reachable through the flow the codebase itself flags as dangerous. importJson.ts:9-11 documents:

//   - Env / header VALUES are always dropped. A user pasting a config from a
//     README may leak real tokens; keys are enough to seed the KV UI and the
//     user re-enters values.

args gets no such treatment — importJson.ts:136-137 lifts them verbatim, McpCreateModal.tsx:1144 writes them into argsRaw, and mcpWireParams.ts:127 persists them. So the canonical Docker MCP config an author pastes from a README (docker run -i --rm -e GITHUB_TOKEN=ghp_… ghcr.io/…) carries its token straight into args and out to every viewer of the detail modal. The author-side warning does not cover it either: McpCreateModal.tsx:1024-1034's sharedSecretLeaks iterates only headersEntries and envEntries.

Ask: apply the redactArgs rules to qs.args in both buildJson and buildPrompt. Note the connector surface wants the placeholder form (formatTokenPlaceholder(key)), not ••••••, so the snippet stays fillable — so this is one shared rule set with two renderers, not a call to redactMcpConfig. Extend sharedSecretLeaks to scan argsRaw in the same change, and extend importJson's value-dropping rule to -e KEY=VALUE / --header pairs inside args.

P1-2 — Connector-detail URL masking covers only secret-named query params: userinfo and fragment still pass through

packages/dmworkmcp/src/api/quickStartTemplates.ts:124-137

function maskUrlSecretParams(url: string): string {
  if (!url) return url;
  return url.replace(
    /([?&])([^=&#]+)=([^&#]*)/g,
    (whole, sep, rawKey) => { ... isSecretKey(key) ? ... : whole; }
  );
}

The regex anchors on ?/&, so it can only ever see query parameters. Executed side by side against the sibling helper added in the same PR:

input	maskUrlSecretParams (connector detail)	redactUrl (expert spec)
https://u:sk-live-PW@mcp.ex/sse	unchanged	https://REDACTED:REDACTED@mcp.ex/sse
https://mcp.ex/sse#access_token=sk-live-FRAG	unchanged	https://mcp.ex/sse#REDACTED
https://mcp.ex/sse?sig=sk-live-Q3	unchanged	https://mcp.ex/sse?sig=REDACTED
https://mcp.ex/sse?api_key=sk-live-Q	masked ✅	masked ✅

Two functions in the same PR mask the same field to different depths, and the shallower one is on the surface with the wider audience. Userinfo-in-URL (https://token@host/sse) is a common way to carry an MCP bearer, and the fragment form shows up on OAuth-ish endpoints. redactUrl already has the correct logic in both its absolute and relative branches — run the same three rules here and substitute a placeholder instead of REDACTED.

Two smaller notes on the same function: the docstring at quickStartTemplates.ts:120-123 claims it "stays ASCII-free of percent-encoding artifacts", but the inserted placeholder is CJK with spaces, so the copied URL is not a valid URL until hand-edited (presumably intended — the highlighter marks it — but the comment says the opposite of what the code does); and because the regex keys on [?&], an &-joined pair inside a fragment is masked while the first one after # is not, which is arbitrary either way.

P1-3 — secret-scan (gitleaks) is RED on this head, and this commit is the cause

CI job 99732635207 — leaks found: 1, exit 1:

Finding:  ...: { s: { url: "/sse#access_token=REDACTED" } } })
RuleID:   generic-api-key   Entropy: 3.546594
File:     packages/dmworkmcp/src/utils/__tests__/redactMcpConfig.test.ts
Line:     147
Commit:   542925e03ec7966e93e538d686e011c6db3707b9

redactMcpConfig.test.ts:147's sk-live-FRAG2 fixture clears the generic-api-key entropy threshold. It is a false positive, but the check is red, so the PR cannot merge as-is. .gitleaks.toml's allowlist covers only apps/web/e2e-kit/** — nothing under packages/**/__tests__. Either lower the fixture's entropy (NOT-A-REAL-SECRET-frag) or add a scoped allowlist entry. Please do not blanket-allowlist packages/** — that would silence the scanner over the whole source tree. (Unit tests and e2e-p0 both went green on this head while I was reviewing; secret-scan is the only red check.)

P2 — heuristic tails and false positives on the new masking (each verified by execution)

• P2-1 — key-name-only masking leaks the near misses. shouldPlaceholder (quickStartTemplates.ts:116-118) is supplied.has(key) || isSecretKey(key), and values are never inspected. SECRET_KEY_PATTERN (constants.ts:23) matches whole words, so these publish verbatim: X-Auth-Header, X-Signature, GITHUB_PAT (bare pat matches, github_pat does not), OPENAI_ORG, DATABASE_URL (→ postgres://user:dbpass@db/prod in both tabs), and ?sig=… in a URL. The sibling redactMcpConfig masks every env/header value regardless of key name; this one masks a named subset. The asymmetry is a defensible product tradeoff — the snippet has to stay usable — but it should be written down as the posture, because the code comment currently says the guard keeps "author tokens out of consumer-facing output" without qualifying which ones.
• P2-2 — three redactArgs bypasses remain (redactMcpConfig.ts:104-141), all verified unchanged on output: the compact single-token flag form -HAuthorization: Bearer sk-… (VALUE_INJECT_FLAGS is an exact-match Set); API_KEY+=sk-… (the inline regex's key class is [A-Za-z0-9_.-]+, so += misses); and the opaque-URL form https:example.com#access_token=sk-… (hasUrlShape requires ://, a leading //, or a ?).
• P2-3 — -e KEY VALUE masks the name and keeps the value. maskNext masks exactly one following token, so ["-e","API_KEY","sk-live-X"] → ["-e","••••••","sk-live-X"]. Rare, but the output is actively misleading about what was hidden.
• P2-4 — --auth-header-style flags miss both rules. isSecretKey("auth-header") is false (the pattern needs the word to end in auth) and it is not in VALUE_INJECT_FLAGS, so ["--auth-header","X-Custom: sk-live-…"] passes through. A substring check would cover this whole family better than an exact-match set.
• P2-5 — new false positives corrupt legitimate values. VALUE_INJECT_FLAGS is command-agnostic, so Ruby's ["-e","puts 42"] renders as ["-e","••••••"], and the widened inline regex turns ["--query","token=identifier"] into ["--query","token=••••••"] (that second one was verbatim before this commit). Display-only, but it makes a published config look wrong.
• P2-6 — bare KEY=<url-with-credentials> escapes both branches. redactMcpConfig.ts:124-131: the inline branch matches DATABASE_URL=postgres://u:pw@host/db, finds isSecretKey("DATABASE_URL") false, and continues — so it never reaches the positional branch that would have masked the userinfo.
• P2-7 — isSafeAttachmentPath does not reject Windows-absolute or dot-segment paths. mcpWireParams.ts:43-53 (and its twin at skillApiReal.ts:251-259) accepts C:/Windows/System32/hosts, connector/./tools.json, and "mcp.json " (trailing space — which also slips past the raw MODELED_ATTACHMENT_PATHS collision check one line later). The predicate is correct for the traversal case it was written for; these are the gaps if it is meant to be a general guard. Also: it is now duplicated byte-for-byte in two packages — lift it into a shared util before the copies drift.
• P2-8 — the masked template is also the copy-to-run payload. McpDetailModal.tsx:37-44 builds the tabs and navigator.clipboard.writeText(current.content) copies them unchanged, so masking a genuinely org-shared credential makes the advertised copy-ready config non-functional for viewers who do not hold it. This is the intended tradeoff, but it is the concrete UX cost of the posture in P2-1 and deserves the description line noted in §1.
• P2-9 — whitespace-only draft passes the forward guard. packages/dmworkbase/src/Components/PromptForwardActions/index.tsx:188 — const promptReady = Boolean(draft) && !disabled;. A draft of " " is truthy, so Copy and Forward stay enabled and an effectively empty prompt can be dispatched into a Bot DM. Boolean(draft.trim()) is fail-closed.

Nit: redactMcpConfig.ts:146-170 copies command, cwd, type, transport and every string in autoApprove / alwaysAllow through untouched. command is called out as an accepted residual at lines 27-29; the tool allow-lists are not, and should be added to that sentence so the whitelist's coverage stays self-documenting.

Carried over from the previous head — still open, still non-blocking

mcpWireParams.ts:142-149 slug-rename collision silently dropping a sibling server; skillApiReal.ts:320-326 + EditSkillModal.tsx:397 unknown-visibility → space fail-open; the localStorage spaceId fallback in InstallPromptModal.tsx:12-19 / McpConnectModal.tsx:16-23; the dead expertWire.ts:30-203 legacy mappers kept alive only by their own tests; SkillCard.tsx:78's unread required categories prop. Full rationale in the previous review — none changed in this delta.

---

3. Overall verdict

REQUEST_CHANGES

Spec ✅, Quality Changes-Requested. The AND gate blocks on P1-1 (unredacted args on the connector detail), P1-2 (URL userinfo/fragment on the same surface), and P1-3 (red secret-scan).

To be explicit about the shape of this: the delta is good work. The redactMcpConfig hardening is complete and correct against every case that was raised, the attachment-path fix is exact, and the corrected comment is honest about the backend's posture. The block is that the same commit's connector-side fix covers three of four credential carriers, and the missing one is the one its own test file exercises.

---

4. Suggested direction

1. One rule set, two renderers. Extract the arg / URL / kv secret rules into a single module with two output modes: •••••• for the read-only expert spec, <把这里换成你的 KEY> for the copy-paste connector snippet. There are currently two independent implementations (redactMcpConfig.ts and quickStartTemplates.ts) with different coverage, and this is the second round in a row where the gap between them was the finding. Wiring args and full URL masking into the connector renderer falls out of that refactor instead of being a third patch.
2. Close the intake side too. importJson already drops env/header values for exactly this reason; extend it to -e KEY=VALUE / --header pairs inside args, and extend sharedSecretLeaks to scan argsRaw, so the author sees the warning before publishing rather than the consumer seeing the token after.
3. Fix the gitleaks fixture with a scoped allowlist entry or a low-entropy literal — not a packages/** blanket rule.
4. P2-2 / P2-3 / P2-4 are a handful of lines in redactArgs; worth folding into the same commit.

---

5. Additional observations

Please verify manually before merge (security-sensitive):

• The exposure posture needs a named owner. Whether space/system-visible connectors may render author-typed env / header / url / args values at all — and whether a key-name heuristic is an acceptable filter for that — is a product decision, not a code one. The state after this commit is: secret-named keys masked, everything else published. Someone should own that sentence, because every round so far has re-litigated it implicitly.
• Deploy coupling. octo-marketplace#67 and #72 are both merged, but I cannot verify either is deployed. By the author's own disclosure the write paths hard-break against a pre-#72 backend (2.0 $schema → 400, skill version → strict-decoder 400, connector create silently placement-less), with no client-side dual-version tolerance. Confirm the backend is live before releasing.
• Round count — this needs a decision, not another round. This PR now carries 42 formal reviews across 84 commits and 111 files. Every round has closed real issues and surfaced a new instance of the same class in the same neighbourhood; that is the signature of a review surface too large to hold in one pass, not of a careless author. My recommendation, unchanged from last round and now stronger: stop iterating on this branch and split it. The unified-backend API migration is contract-correct and is the load-bearing half — it can land on its own, with the UI restructure, the editor deletion, and the connector secret-redaction work as separate PRs. The redaction work in particular deserves its own PR with its own written threat model, because it is currently being designed incrementally inside a 111-file UI branch. That call belongs to whoever owns this branch's schedule, not to another review pass.

Verification coverage / what I did not check:

• On this head: Unit tests pass (9m17s), e2e-p0 pass (11m23s), Build, osv-scan, dependency-review, Block removed enterprise modules green; secret-scan red (P1-3). No node_modules in this checkout, so I did not run the suites locally — but I did verify the two changed helper modules by extracting them and executing them directly against every input in the tables above.
• I re-read the merged octo-marketplace sources last round for the wire contract (schema ids, upsert fields, sort enum, visibility model, Service.Detail scoping); nothing in this delta touches the wire contract, so I did not re-verify it. I have not audited the backend's space/private authorization beyond the detail path, nor its attachment-path normalization / archive-extraction behaviour (which is what would decide whether P2-7's C:/… and ./ cases matter downstream).
• No browser was driven: CSS, responsive layout, and the real a11y behaviour of MineTable, the sort row, and the editable prompt remain unexercised. There is still no lint job in this PR's check list, so lint-class regressions are not covered by CI either.
• Two independent second-opinion passes were run alongside this review. One raised the preview-prop removal and the copyTrackEvent telemetry change as P1s; I refuted both — no remaining caller passes preview, and at merge-base the tracking was route-gated to /mcp-market/mcp (PromptForwardActions/index.tsx:189), which the four surfaces that pass copyTrackEvent today cover, with the expert surface never having been tracked. Its third item became P2-9. The other pass independently found the args gap and the URL-depth asymmetry (agreeing with P1-1/P1-2) and contributed P2-2, P2-5, P2-7 and the autoApprove nit, all of which I re-verified by execution before including. Neither pass could run the test suites either.

[Jerry-Xin]

Code Review — PR #1584 (octo-web) — re-review round 5

Reviewed at head e10c76c20228da550a8cae7e96451d9fa33ba606, merge-base 797243d9f2b77e39f59dd84f1ab0fce7f66e4e81. Two commits since the previously reviewed head 45194942325a: fix(mcp): mask secret-shaped values on connector detail + harden redaction (542925e, the substantive response to yujiwei's RC 5073826675) and test(mcp): use low-entropy canaries in redaction fixtures (fix gitleaks) (e10c76c, test-fixture literals only). All production-code changes are in packages/dmworkmcp. Codex produced no output this round (0-byte, 2 attempts); this verdict is from an independent byte-level pass.

Verdict: APPROVE

Both blockers byte-verified REAL at the old head; both fixed at this head with non-vacuous regression tests. Credit yujiwei for the catches.

1. P1-1 (detail read path leaks previously-blanked credentials) — REAL at old head

Byte-verified end-to-end across both repos:

• Legacy read path (pre-migration): octo-marketplace internal/service/mcp.go:468-475 — detailForCaller + blankMapValues blank ALL QuickStart env/header values on non-owner reads; the backend tests call it the sole guardrail keeping author tokens out of consumer responses.
• Unified read path (this PR's surface): octo-marketplace internal/service/plugin/service.go:349-370 — Service.Detail (served at /plugins/detail, handler.go:333) returns the full Plugin including PluginJSON after a visibility-scope check only. No owner check, no blanking anywhere on that path.
• Old-head frontend: packages/dmworkmcp/src/api/mcpService.ts:643-654 lifts server.env / server.headers / server.url verbatim from the mcp.json attachment into quickStart; old-head quickStartTemplates.ts substituted placeholders only for author-marked user-supplied keys and passed everything else — including the full URL — verbatim into the copy-paste snippet rendered to every viewer (McpDetailModal.tsx:409).

So the migration moved connector detail from a blanking read path onto a non-blanking one, and the snippet rendered raw shared values to everyone in scope. Exactly as claimed.

2. P1-2 (redaction helper misses common credential shapes) — REAL at old head

I extracted the old-head redactUrl / redactArgs verbatim and ran them on all five inputs from the RC's table: protocol-relative userinfo survives, relative-URL fragment survives, the --header token survives (and is mutated — scheme lowercased via new URL()), the container-env-injection flag form survives, and the bare positional token survives. All five reproduced byte-for-byte.

3. Fix verification at this head (code executed, not just read)

• quickStartTemplates.ts: shouldPlaceholder = user-supplied OR secret-shaped key, applied to env and headers in both the JSON tab and the prompt tab (applyUserSuppliedPlaceholder + both buildPrompt branches); maskUrlSecretParams masks secret-shaped URL query params as fillable placeholders in both tabs. Empirical run of buildQuickStartTabs: secret-shaped shared env/header values now render as fillable placeholders; secret query params masked; endpoint + non-secret params intact; non-secret shared values pass through (intentional — the snippet stays runnable, and the legacy blanking of non-secret shared values was a posture, not a credential guard).
• redactMcpConfig.ts: the relative / protocol-relative catch branch now masks userinfo + query + fragment; VALUE_INJECT_FLAGS covers --header / -H / --headers / --env / -e; bare KEY=value positionals mask iff the key is secret-shaped (benign FOO=bar kept); hasUrlShape gating ends the new URL() mutation of non-URL positionals (verified byte-identical passthrough of the colon-bearing case). Docstring now matches behaviour. Four of the five table rows masked; row 5 (bare positional token with no structural cue) still passes — outside the three asks, covered by the documented residual posture.
• The stale §5.3 comment in McpCreateModal.tsx is rewritten to describe the actual guards (P1-1's second ask).
• Bonus fix: mcpWireParams.ts validates preserved attachment paths (isSafeAttachmentPath, mirroring skillApiReal.ts) — closes P2-1.
• No regression of any prior-round redaction fix: positional-URL query tokens, absolute-URL fragment + userinfo, whitelist rebuild (root siblings dropped), malformed-input fail-closed null, exact-placeholder preservation — all re-verified by execution.

4. Tests & CI

Local (node 22): @dmwork/mcp 252/252 at this head, @dmwork/skillmarket 136/136 at the prior head (its package tree is byte-identical between the two — the second commit touches only dmworkmcp test fixtures). The new cases assert real leak-absence, not tautologies. CI on e10c76c: Build and secret-scan green; e2e-p0 still in progress at review time. The Unit tests check is red on log-verified flake, not failure: the run reports 470/470 files and 4299/4299 tests passed, and the non-zero exit comes from the known prosemirror getClientRects unhandled teardown error in the @octo/base chat-composer spec (coverage-enforce step) — untouched by this PR and the same signature documented in 4519494's retrigger note. The workspace turbo run itself was 8/8 green, including both marketplace packages. Note on the earlier red: at 542925e, gitleaks generic-api-key flagged a fake fixture literal in packages/dmworkmcp/src/utils/__tests__/redactMcpConfig.test.ts:147 — a test-fixture false positive; e10c76c swaps the fixture literals for low-entropy canaries and secret-scan is green again. (secret-scan is not in the main-branch ruleset's required list — code-review / Build / Unit tests / e2e-p0 — so it was never merge-blocking.)

5. Non-blocking residuals (documented, none worse than pre-migration posture)

• Snippet URL masking is query-only: a userinfo credential or a fragment token in the connector URL still renders in both tabs. Legacy detailForCaller blanked only env/headers, never the URL, so this is not a regression — but it is the same class, worth a follow-up.
• stdio snippets emit command / args verbatim; a secret placed in an arg (flag value, KEY=value, or bare positional) renders to viewers. Same posture as legacy and as the documented redactMcpConfig residual.
• The durable fix is server-side: restore §5.3-style non-owner blanking (or owner-scoped field projection) on octo-marketplace /plugins/detail. Client-side masking is defense-in-depth, not a substitute.

6. Deploy coupling (carried)

This branch speaks octo-marketplace #72's 2.0 contract; confirm the backend is deployed before release (write paths hard-break against a pre-#72 backend).

[Jerry-Xin]

Code Review — PR #1584 (octo-web) — re-review round 6

Reviewed at head 5d9db51106f2b41c4a880d18ec0b4d97dfe4ceda, merge-base 797243d9f2b77e39f59dd84f1ab0fce7f66e4e81. Delta vs e10c76c2 is one commit — test(base): stub getClientRects/getBoundingClientRect on Range + Text (fix flaky unit CI) — touching only packages/dmworkbase/src/__tests__/setup.ts. All five security-bearing files (quickStartTemplates.ts, redactMcpConfig.ts, mcpWireParams.ts, mcpService.ts, McpCreateModal.tsx) are blob-identical to 542925e03 at this head (blob-hash verified). Codex produced no output this round; this verdict is from an independent byte-level pass.

Verdict: REQUEST_CHANGES

Correction of my previous vote first. I APPROVED e10c76c2 (review 5074017080, 04:28 UTC) without having seen yujiwei's RC 5073955064 (submitted 04:15 UTC on 542925e0). I independently byte-verify his two blocking findings below and confirm both are live at this head; my earlier classification of the same exposures as "non-blocking residuals" was an under-call, and that approval is being dismissed. Credit yujiwei for both catches.

🔴 B1 (yujiwei P1-1) — connector detail publishes args verbatim; confirmed live at this head

The connector quick-start snippet is the PR's own redaction surface, and args is not on it:

• packages/dmworkmcp/src/api/quickStartTemplates.ts:101 — JSON tab: args: qs.args ?? [], no masking of any kind.
• packages/dmworkmcp/src/api/quickStartTemplates.ts:248 — prompt tab joins the same array verbatim.
• Payload is raw backend data: packages/dmworkmcp/src/api/mcpService.ts:650 lifts args: server.args straight off the stored mcp.json; McpDetailModal.tsx:409 renders the tabs to every viewer within the plugin's visibility scope.

I executed buildQuickStartTabs at this head with low-entropy canaries (so gitleaks stays quiet): a docker-style -e API_KEY=<canary> arg, a --token <canary> flag-value pair, and an mcp-remote positional URL carrying access_token=<canary> — all three render verbatim in both tabs. Meanwhile the same run confirms the sibling carriers ARE masked: secret-shaped env/header keys and secret-named URL query params all render as fillable placeholders.

The hardened redactArgs added by this PR's own fix commit is wired only to ExpertSpecView.tsx (read-only expert spec) — never to the connector surface. Intake is open too: importJson.ts lifts args verbatim from a pasted config, and the author-side warning sharedSecretLeaks (McpCreateModal.tsx:1024) scans only env/header entries. So a canonical pasted Docker MCP config carries its token into args and out to every viewer.

Adopting yujiwei's ask: one shared rule set with two renderers (•••••• for the expert spec, the fillable placeholder for the snippet), applied to qs.args in both buildJson and buildPrompt; extend sharedSecretLeaks to argsRaw and importJson's value-dropping to -e KEY=VALUE / --header pairs inside args.

🔴 B2 (yujiwei P1-2) — snippet URL masking is query-only; userinfo, fragment, and non-secret-named params pass through

maskUrlSecretParams (quickStartTemplates.ts:124-137) anchors on [?&] and only masks secret-named keys. Executed at this head with canaries: a userinfo credential (https://u:<canary>@host/sse), a fragment token (…#access_token=<canary>), and a non-secret-named query param (?sig=<canary>) all render verbatim in both tabs. The sibling redactUrl added in the same commit masks all three (absolute and relative branches) — two functions in the same PR mask the same field to different depths, and the shallower one sits on the surface with the wider audience. Fix per yujiwei: run the same userinfo/query/fragment rules here, substituting the fillable placeholder instead of REDACTED.

Verified fixed / intact at this head

• P1-3 (secret-scan): fixed by e10c76c2's low-entropy canaries — green live at both heads; the fixture false positive is gone and no blanket allowlist was added.
• Prior-round fixes (A''/B'' class): intact — blob-identical files, and the same canary run confirms secret-shaped env/header keys and secret-named URL query params are masked while endpoints and non-secret params survive; all previously verified redactMcpConfig hardening (relative userinfo/fragment, value-inject flags, KEY=*** positionals, hasUrlShape` anti-mutation, whitelist rebuild, malformed→null) unchanged.
• This head's delta: the setup.ts stub is the correct fix for the CI flake I log-verified at e10c76c2 (470/470 files & 4299/4299 tests passed there; the non-zero exit was the prosemirror singleRect → Range/Text getClientRects uncaught error in the @octo/base chat-composer teardown) — it stubs exactly the prototypes in that stack without hiding real errors behind a global handler. Test-infra only; no product behaviour touched.

🟡 still open

The PR description still does not mention the read-path behaviour change (a shared env/header whose key name is secret-shaped now renders as a placeholder with no author opt-out — verified against the live PR body). One line in the description, per yujiwei's note.

Non-blocking (agree with yujiwei's classification)

His P2-1..P2-9 (key-name-only heuristic near-misses incl. DATABASE_URL-style values; the three redactArgs bypasses; -e KEY VALUE ordering; --auth-header family; new false positives; KEY=<url-with-credentials>; isSafeAttachmentPath Windows/dot gaps + the byte-duplicated predicate; copy-to-run UX cost; whitespace-only forward draft), the autoApprove/alwaysAllow docstring nit, and the carried-over items. Same class as the residuals I documented last round; none block, several worth folding into the B1/B2 fix commit.

Tests & CI

Local (node 22) at this head: @dmwork/mcp 252/252, @dmwork/skillmarket 136/136. CI at review time: Build and secret-scan green; Unit tests and e2e-p0 in progress (e2e-p0 was green at e10c76c2).

Carried

Deploy coupling: this branch speaks octo-marketplace #72's 2.0 contract — confirm the backend is deployed before release. And yujiwei's meta-recommendation stands for whoever owns this branch's schedule: this review surface is converging one instance of the same class per round; splitting the migration from the redaction work (with its own written threat model) would serve everyone.

[yujiawei]

Code Review — PR #1584 (octo-web)

Reviewed at head 5d9db51106f2b41c4a880d18ec0b4d97dfe4ceda, diffed against merge-base 797243d9f2b7 (112 files, +6693/−2087).

Verified locally at this head: pnpm install ✅ · pnpm --filter @dmwork/skillmarket test 136 pass ✅ · pnpm --filter @dmwork/mcp test 252 pass ✅ · pnpm --filter @octo/base test 4299 pass ✅ · pnpm build ✅ · pnpm i18n:check ✅ (every new key I spot-checked resolves in both zh-CN and en-US). The migration itself is careful work — the fail-closed category resolution, the ${KEY} placeholder write path, isSafeAttachmentPath on echoed attachments, and the allSettled + 404-only-drop relation fan-out are all the right shapes. The findings below are the places where that care has a gap.

---

1. Scope / spec compliance

Spec: ❌ — no functional shortfall, but the shared-layer disclosure in the PR body does not match the diff, and one unrelated change rode along.

Understated shared-layer impact. The PR body states "Shared layer touched: none new — reuses the existing @octo/base PromptForwardModal" and "dmworkbase change is limited to FetchRules telemetry patterns for the unified endpoints." The diff does more than that to @octo/base:

• packages/dmworkbase/src/Components/PromptForwardActions/index.tsx:56 — the preview prop is removed and kind / copyTrackEvent added. That is a breaking public API change on an exported shared component (packages/dmworkbase/src/index.tsx:111-112). It happens to be safe today (PromptForwardModal is the only in-repo consumer and is updated in the same diff), but it is not "no shared layer touched".
• packages/dmworkbase/src/Components/PromptForwardModal/index.tsx:35 — title becomes optional with a new default.
• packages/dmworkbase/src/Service/TrackRules.ts and DAP_EVENTS.md — telemetry beyond FetchRules.
• packages/dmworkbase/src/i18n/locales/*.json — 9 new promptForward.* keys.

Out-of-scope change. packages/dmworkbase/src/__tests__/setup.ts:18-48 replaces the ProseMirror/jsdom geometry stub (adds getBoundingClientRect on Element / Range / CharacterData) to fix an intermittent chat-composer test failure. packages/dmworkbase/src/Service/__tests__/channelUniqueness.test.ts also loses two lines. Neither has anything to do with the marketplace migration, and neither is mentioned in the description. It is a benign fix, but it should be disclosed or split out — a shared test harness change hidden inside a 6.7k-line market PR is exactly the kind of thing that gets bisected onto the wrong commit later.

No missing work and no unrequested feature was found against the stated scope; both items above are fixed by editing the PR description (and optionally splitting the setup.ts change).

---

2. Code quality

Quality: Changes-Requested — two P1s, both silent-failure classes.

P1-1 — Multi-server mcp.json: first-key selection destroys the stored config on a metadata-only edit

packages/dmworkmcp/src/api/mcpService.ts:637 (read) and :854 (write) both pick the server by position:

const serverName = Object.keys(servers)[0] ?? "";        // mapDetail, :637
const currentServerName = Object.keys(currentServers)[0] ?? "";  // updateMcpReal, :854

but the write keys the entry by the manifest slug:

const serverKey = slug || name;   // mcpWireParams.ts:142
mcpServers[serverKey] = server;   // :149

slug comes from manifest.name (mcpService.ts mapDetail → slug: manifest.name; seeded into the form at McpCreateModal.tsx:184). When the stored document has more than one server and the connector's own server is not first, these two disagree and a pure metadata edit is destructive. Note that goCanonicalJSON (pluginWire.ts:146-160) sorts object keys, so after any write through this client the first key is simply the alphabetically first one — not the connector's.

Concrete failure with mcpServers = {"a-helper": A, "github-mcp": G} and manifest.name = "github-mcp", user edits only the description:

1. mapDetail reads A → the form is populated with a-helper's url/command/args/env, but slug is github-mcp.
2. updateMcpReal sets rawServer = A, extraServers = {"github-mcp": G}.
3. toPluginUpsert computes serverKey = "github-mcp", then for (const [k,v] of extraServers) if (k !== serverKey) … (mcpWireParams.ts:146-148) drops G, and writes mcpServers["github-mcp"] = <A's config>.

Net result: github-mcp's real configuration is overwritten with the helper's, and a-helper disappears entirely. The extraServers preservation logic was added precisely because multi-server documents exist (backfill / octo-cli publishing), so this is reachable — the JSON-import path is not, since it takes only the first server by design.

Suggested fix: resolve the server by the manifest slug first, falling back to the single key only when the slug is absent:

const keys = Object.keys(servers);
const serverName = (manifest.name && manifest.name in servers) ? manifest.name : (keys[0] ?? "");

and apply the same resolution in updateMcpReal so rawServer / extraServers are computed against the same key the write will use. A regression test with two servers where the manifest slug is not first would pin it — mcpService.connector.test.ts currently only covers the already-first case.

P1-2 — Skill visibility fails open on an unrecognized value, and the widened value is written back

packages/dmworkskillmarket/src/api/skillApiReal.ts:319-327 maps anything outside the four known scopes to "space":

visibility:
  raw.visibility === "public" || raw.visibility === "private" ||
  raw.visibility === "space"  || raw.visibility === "system"
    ? raw.visibility
    : "space",

skillApiReal.test.ts:976-998 pins this: visibility: null → "space". That is a reasonable display default, but it does not stay display-only — EditSkillModal.tsx:397 echoes the mapped value straight back into the full-replace upsert:

visibility: skill.visibility,

So a record whose stored visibility is null/absent/unmodeled (a plausible state for a backfilled row, or for any future enum value) renders as 本组织 and is then persisted as space the next time its owner touches the description. For a privacy control, space is not the conservative default; private is. The connector-side comment (packages/dmworkmcp/src/utils/visibility.ts:8) calls space "the least-surprising, non-permissive bucket" — that reasoning holds for a read-only chip label, but not for a value that round-trips into a write.

Suggested fix: keep "space" for the chip if you like, but make the unknown case non-writable — either map unknown → "private", or have EditSkillModal omit visibility unless the source value was one of the four recognized scopes (and have updateSkill then preserve plugin.visibility verbatim, which it already does at skillApiReal.ts in the form.visibility ?? plugin.visibility branch).

---

P2 findings

P2-1 — Published connector credentials are masked only for recognized key names; args, command and URL userinfo are never masked.
packages/dmworkmcp/src/api/quickStartTemplates.ts:100-101 emits command and args verbatim, and maskUrlSecretParams (:124-136) masks only query values whose parameter name matches isSecretKey. Meanwhile packages/dmworkmcp/src/api/mcpWireParams.ts:226 only placeholders keys the author explicitly marked user-supplied; every other value is sent to /plugins/upsert as typed. Given this PR's own stated threat model — the backend has no secret scanner and does not blank on read — a connector published with args: ["--token", "sk-live-…"], command: "sh -c 'TOKEN=… npx …'", url: "https://user:pw@host/…", or a header named X-Signature will render its credential to every viewer of a space/system connector detail. The inline warning at McpCreateModal.tsx:1017-1033 is advisory and non-blocking.
Notably, the expert surface already handles most of this: redactMcpConfig.redactArgs / redactUrl (packages/dmworkmcp/src/utils/redactMcpConfig.ts:110-144, 59-91) mask secret-shaped KEY=value positionals, values after --header/-e, URL userinfo and the whole query string. The connector detail path does not reuse any of it. Aligning the two would close most of the gap cheaply.
One residual on the expert side too: redactMcpConfig.ts:141 deliberately leaves a bare colon-bearing positional untouched, and __tests__/redactMcpConfig.test.ts:130-139 asserts args: ["Authorization: Bearer keepme"] survives redaction. The intent (don't let new URL() mangle a non-URL token) is right, but the test as written blesses a credential passthrough — worth masking the value after a Header: value-shaped positional while keeping the header name.

P2-2 — Dual publisher attribution ("creator · owner") is now unreachable by construction.
skillApiReal.ts:313 sets creatorId: raw.owner_id (the unified wire has no creator_id; the previous mapper used raw.creator_id ?? raw.owner_id). Downstream, SkillCard.tsx:102-108 and SkillDetailModal.tsx:227-233 do:

const hasComparableIds = Boolean(skill.creatorId && skill.ownerId);
hasComparableIds ? skill.creatorId !== skill.ownerId : creatorName !== skill.ownerName

With creatorId === ownerId always, the ID branch is always false and it makes hasComparableIds always true, so the name-based fallback (creatorName !== ownerName) is now dead too. A bot-authored skill (creator_name = "CodeReview Bot", publisher = "李衡") that used to render CodeReview Bot · 李衡 now renders only CodeReview Bot. The component tests still pass because SkillCard.test.tsx:73/93/114 and SkillDetailModal.test.tsx:100 hand-construct a distinct creatorId that the mapper can no longer produce — green tests over a dead path. Simplest fix: leave creatorId undefined (it is optional in types/skill.ts:23) so the name comparison is used again.

P2-3 — McpCreateModal's secret-model comments now contradict the code.
McpCreateModal.tsx:107-113 and :135-145 state that for a userSupplied key "the value itself IS persisted verbatim and returned on every read" and "the owner sees it again on their own edit." Neither is true any more: placeholderSecretMap (mcpWireParams.ts:226) replaces user-supplied values with ${KEY} before the write, and splitUserSupplied (pluginWire.ts:206-217) blanks the self-placeholder on read. The code is safer than the comment, but this is the file a future maintainer will read to reason about the secret model — please correct it (and note the owner-side behaviour change: a typed value for a user-supplied key is no longer round-tripped).

P2-4 — Two divergent copies of the unified wire contract.
packages/dmworkmcp/src/api/pluginWire.ts and packages/dmworkskillmarket/src/api/pluginWire.ts are near-duplicates that have already drifted: the skill copy models space_id (:53) and the connector copy does not; the connector copy uniquely owns SECRET_PLACEHOLDER / goCanonicalJSON / splitUserSupplied / OffsetPaginationWire. dmworkmcp already depends on @dmwork/skillmarket (it imports SkillListPage and now MineTable), so one owning module is achievable. As-is, the next backend field addition will land in one market and be silently dropped in the other.

P2-5 — Copy is hidden behind :hover in the split layout.
PromptForwardActions/index.css:177-198: .wk-prompt-forward__copy-icon { opacity: 0 }, revealed only by .wk-prompt-forward__preview:hover or :focus-visible. This replaced a visible primary 复制 button (the split layout's left column now shows only 编辑提示词). On a touch/hover-incapable pointer the copy affordance is invisible and effectively undiscoverable. Suggest keeping it at a low but non-zero opacity, or adding @media (hover: none) { opacity: 1 }.

P2-6 — nextCursor ignores the cursor metadata the envelope still declares.
skillApiReal.ts:459 derives nextCursor purely from page * pageSize < total, while SuccessEnvelope (:37-45) still declares has_more and next_cursor and neither is read. A response that sets has_more: true without offset totals is treated as exhausted after page 1. Either consume has_more/next_cursor when present, or drop them from the envelope type so the contract is unambiguous.

P2-7 — Expert "我的发布" silently caps at 100 records.
expertService.ts:355 defaults page_size to 100 and ExpertMarketListPage.tsx:188 fetches exactly one page for listMyExperts() / listMySquads(); keyword and tag filtering then runs client-side over that slice (:392-411). Pre-existing, but this PR makes 我的发布 the only place to edit or delete an owned expert, so the 101st owned expert becomes unmanageable. Worth at least a truncation notice on the mine view (the discovery view already has mcp.expert.truncatedNotice).

P2-8 — /plugin_tags query is inconsistent across the three markets.
expertService.ts:580-584 sends only plugin_type, while mcpService.ts:1063-1067 and skillApiReal.ts:486-490 also send scene_code. If the endpoint scopes on scene, expert tag suggestions will not match the expert list.

P2-9 — The editable-prompt test does not assert the security-relevant behaviour.
PromptForwardActions/__tests__/PromptForwardActions.test.tsx covers that the edit button stays actionable, but nothing asserts that handleCopy / handleForward send draft rather than the original prompt (index.tsx:194, 226). A regression that silently forwards the un-edited original — i.e. ignores whatever the user removed from the prompt — would keep this suite green.

P2-10 — Small dead bits left by the sort reduction.
SkillListPage.tsx:38-41 and ExpertMarketListPage.tsx:44-47: no SORT_OPTIONS entry sets descending any more, so the <ArrowDown> branches (SkillListPage.tsx:274, ExpertMarketListPage.tsx:685) and the descending?: boolean field are unreachable. Also ListMcpParams.createdByType (types/mcp.ts:163) is no longer forwarded to the connector list query by the real path — only the mock still reads it. Neither breaks anything; both are cleanup.

Nit — SkillListPage.tsx:191 drops autoFocus from the discovery SearchBar with no mention in the description; intentional?

---

3. Overall verdict

REQUEST_CHANGES — scope disclosure needs correcting (§1) and two P1 correctness/privacy defects should be fixed before merge (P1-1, P1-2). Everything else is P2 and can land as follow-ups.

---

4. Items I'd like a human to verify manually

This PR is labelled security-sensitive and the following cannot be settled from this repository alone:

1. The secret posture changed, and the client is now the only control. The code documents that the unified backend has no secret scanner and performs no read-time blanking, replacing the previous per-owner blanking. Please confirm that is actually the deployed behaviour, and that shipping "any secret-shaped value an author types under a shared key is readable by every viewer of a space/system connector" is an accepted posture — see P2-1 for what still leaks even with the client-side masking in place.
2. Cross-repo deploy coupling. The write paths emit cowork-plugin-*-2.0.json $schema ids, send an optional version on /plugins/upsert, and rely on connector create auto-attaching the default-scene placement — all of which require octo-marketplace#72 deployed, and there is deliberately no dual-version tolerance. This must not release ahead of that backend.
3. /metrics/track contract. Both skillApiReal.trackSkillView and expertService.trackExpertViewReal now send resource_type: "plugin" instead of the per-type value. Confirm the backend accepts it and that view counters still increment.
4. Authenticated package download. expertService.fetchSkillPackage now issues a bare fetch to ${BASE}/plugins/download?... with hand-attached token / X-Space-Id headers (expertService.ts:866-885). Confirm that endpoint's CORS config permits those request headers, and that the size cap still applies to the streamed response.
5. e2e coverage is contract-shaped, not contract-verified. The MSW handlers were rewritten to the /plugins shape, but they ignore query parameters — e.g. skill-market-list.ts returns the same fixture regardless of mode, q, tag, category_id, or sort. A parameter-name mismatch against the real backend (q vs keyword, tag vs tags, sort=newest|installs|downloads) would not be caught by these specs. Worth one manual pass against a real #72 deployment before release.

---

5. Additional observations (non-blocking)

• fetchMcpListPath (mcpService.ts:686-700) now awaits /plugin_categories before issuing the list request, where the legacy code ran both in a single Promise.all. Every connector list load is one extra serialized round-trip, and it deliberately bypasses the getConnectorCategoryMaps cache. Intentional per the comment, but it is a visible latency regression on the busiest screen.
• expertSkillIndex / squadSkillIndex (expertService.ts:325-326) are module-level maps that are never pruned or cleared on space switch. Bounded by the number of experts a session opens, so not a practical leak, but a stale entry after a delete will send a /plugins/skill_md request for a dead plugin id.
• McpMarketListPage.handleSpaceChanged_ (:216-230) resets filters and closes the connect modal but leaves detailId, editingDetail, createVisible and deletingItem mounted across a space switch; handleEditFromCard (:474-477) also awaits fetchMcpDetail without an in-flight guard, so a slow response can reopen the previous space's editor. Pre-existing, not introduced here, but the new 我的发布 surface makes the editor path more prominent.
• MineTable uses role="table" with role="row" children and no role="rowgroup" wrapper (MineTable.tsx:80-111); some screen readers will not announce the grid structure correctly.

[yujiawei]

Code Review — PR #1584 (octo-web)

Reviewed at head e4345462d437d134195ae287da8c4004c41b4159, merge-base 797243d9f2b77e39f59dd84f1ab0fce7f66e4e81 (112 files, +6809/−2091). Independent pass against this head; no earlier verdict is carried forward.

The delta since the previously reviewed head 5d9db51106f2 is exactly one commit — e4345462d "fix(mcp): unify connector/expert redaction + fail-closed multi-server & visibility" (9 files, +223/−111) — responding to the two blockers and several P2s from the last round. Everything below is verified against this head.

---

1. Spec compliance

Spec: ✅

• Missing: none. The delta implements the four things it set out to: args + full-depth URL redaction on the connector snippet, manifest-slug server selection on the multi-server read/write paths, a fail-closed skill-visibility default, and the creatorId / comment corrections.
• Extra: none. Production changes are confined to packages/dmworkmcp and packages/dmworkskillmarket; no scope was added.
• Divergence: none. The three disclosure gaps flagged last round are now closed in the PR body — the @octo/base PromptForwardActions prop change is called out under "Shared layer touched: yes", the dmworkbase/src/__tests__/setup.ts harness change is disclosed as a test-only rider, and the read-path posture change has its own bullet.

One stale line, not a spec failure: the "Read-path behaviour change" bullet still describes the previous head's narrower behaviour (only secret-shaped key names placeholdered). At this head the snippet masks every URL query value regardless of key name, plus secret-bearing args. Worth updating that sentence before merge — on a security_sensitive PR the posture statement is the thing a future reader will trust.

---

2. Code quality

Quality: Approved

Both round-6 blockers are closed, and I verified the fixes by execution rather than by reading. I extracted redactUrlDeep / redactArgs from this head and ran them against every input from the last round's failure tables, using low-entropy canaries.

B1 — args on the connector snippet — fixed

packages/dmworkmcp/src/api/quickStartTemplates.ts:106 (JSON tab) and :239 (prompt tab) now both route through the shared redactArgs(..., snippetMask):

input	previous head	this head (snippet)
["run","-i","--rm","-e","API_KEY=<canary>","img"]	verbatim	…,"-e","API_KEY=<把这里换成你的 API_KEY>","img"
["--token","<canary>"]	verbatim	["--token","<把这里换成你的 token>"]
["-y","mcp-remote","https://x/sse?access_token=<canary>"]	verbatim	query value placeholdered
["--header","Authorization: Bearer <canary>"]	verbatim	Authorization: <把这里换成你的 Authorization>

B2 — URL masking depth — fixed

redactUrlDeep (packages/dmworkmcp/src/utils/redactMcpConfig.ts:67-98) replaced the new URL() implementation with pure string ops, so the absolute and relative branches now have identical depth and an arbitrary (including CJK) mask token round-trips without percent-encoding. Verified on both renderers:

input	opaque (expert spec)	fillable (connector snippet)
https://u:<canary>@mcp.ex/sse	https://REDACTED@mcp.ex/sse	https://<把这里换成你的 userinfo>@mcp.ex/sse
https://mcp.ex/sse#access_token=<canary>	#REDACTED	#<把这里换成你的 fragment>
https://mcp.ex/sse?sig=<canary>	?sig=REDACTED	?sig=<把这里换成你的 sig>
//u:<canary>@mcp.ex/sse	//REDACTED@mcp.ex/sse	placeholdered
https://mcp.ex/sse?a=1&a=<canary>	both values masked	both values masked
${API_KEY} (exact placeholder)	preserved	preserved

No regression of the prior-round hardening: hasUrlShape still keeps a non-URL colon token byte-identical, benign positionals (--rm, img:tag, -p 5432:5432, FOO=bar, REGION=us-east-1) survive, the whitelist rebuild still drops root siblings, and malformed input still fails closed to null.

Multi-server selection — fixed

mcpService.ts:636-645 (mapDetail) and :860-870 (updateMcpReal) now resolve the modeled server by the manifest slug, which is the same key mcpWireParams.ts:142 re-emits it under (serverKey = slug || name, and slug is slugifyServerName(...), so manifest.name === serverKey for anything this client has written). The new regression test at mcpService.connector.test.ts:317-360 pins the exact failure case — modeled server not alphabetically first — and asserts both that its own config survives and that the sibling is preserved.

Other delta items — verified

• skillApiReal.ts:317 — creatorId: undefined restores the name-based dual "creator · owner" fallback in SkillCard.tsx:102-108 / SkillDetailModal.tsx:227-233.
• McpCreateModal.tsx:106-113 and :139-149 — the secret-model comments now match placeholderSecretMap / splitUserSupplied.
• CI at this head: Build, Unit tests, e2e-p0, secret-scan, osv-scan, dependency-review, Block removed enterprise modules — all green.

---

P2 — non-blocking, but the first two are the ones I would actually fix

P2-1 — The unified rules over-mask the snippet: non-secret query params and the docker -e VARNAME passthrough are destroyed.

The last round asked for "the same userinfo / query / fragment rules" on the snippet, and that is exactly what landed — but the snippet's contract is different from the expert <pre>: it is advertised as copy-paste-runnable, and it now removes values a viewer cannot re-derive.

Executed at this head:

url:  https://mcp.ex/sse?region=us&api_key=<canary>
  ->  https://mcp.ex/sse?region=<把这里换成你的 region>&api_key=<把这里换成你的 api_key>

args: ["run","-i","--rm","-e","GITHUB_PERSONAL_ACCESS_TOKEN","ghcr.io/github/github-mcp-server"]
  ->  ["run","-i","--rm","-e","<把这里换成你的 e>","ghcr.io/github/github-mcp-server"]

The second one is the canonical docker MCP shape (-e VARNAME passes the variable through from the environment — the token itself lives in the env block rendered directly below). maskNext fires on -e and falls to out.push(mask(maskHint)) at redactMcpConfig.ts:150 with maskHint = "e", so a variable name — not a value — is replaced, and the placeholder key is meaningless. The new test at quickStartTemplates.test.ts:60-72 also pins region=us as masked, so this ships as intended behaviour unless it is revisited.

This also contradicts the file's own documentation: quickStartTemplates.ts:112-119 still says "Non-secret shared values (X-Client, region, …) still pass through so the snippet works as authored" — true for env/headers, no longer true for the URL. And the posture is now internally inconsistent in the opposite direction: a shared header literally named X-Signature still renders verbatim (shouldPlaceholder is key-name-based) while ?region=us is destroyed.

Suggested shape: keep one rule set, but let the two renderers differ in depth where the contracts differ — for the snippet, mask userinfo / fragment / secret-named query params (which is what the round-5 finding was actually about), and in the maskNext branch keep a bare ^[A-Za-z_][A-Za-z0-9_]*$ token after -e / --env as-is, since a variable name is not a value.

P2-2 — Unknown skill visibility is synthesized to private and then written back, and there is no UI to undo it.

skillApiReal.ts:326-333 now maps an unrecognized/absent visibility to "private", and EditSkillModal.tsx:397 echoes that synthesized value into the full-replace upsert. The direction is the safer one for a privacy control, but the underlying issue from last round — a guessed value is persisted — is unchanged, and the consequence is now removal rather than widening: a backfilled record whose stored visibility this client does not model disappears from its org on the owner's next description edit. There is no visibility control anywhere in the web market (NewSkillModal.tsx:397 hardcodes "space" on create; the edit modal has none), so the owner cannot restore it.

updateSkill already has the right fallback — skillApiReal.ts:596: const visibility = form.visibility ?? plugin.visibility;. Omitting visibility from the edit payload when the source value was unrecognized preserves the raw stored value and needs no new plumbing.

P2-3 — ?=<value> (empty query key) is a redaction regression vs. the previous head. redactMcpConfig.ts:76's key class is [^=&#]+, so https://h/sse?=<canary> passes through. The new URL() implementation it replaced produced ?=REDACTED. Degenerate URL shape, but it is a strict regression on the expert-spec surface too.

P2-4 — Fragment-only relative arg escapes hasUrlShape. redactMcpConfig.ts:108-110 requires ://, a leading //, or a ?, so args: ["mcp-remote","/sse#access_token=<canary>"] is emitted verbatim on both surfaces despite the fragment guarantee. Adding || a.includes("#") covers it.

P2-5 — Slug rename onto an existing sibling key silently destroys that sibling. mcpWireParams.ts:146-149: with {main: modeled, helper: helperCfg} and the modeled slug renamed main → helper, the loop skips helper (k === serverKey) and then overwrites it with the modeled server; main is dropped too. The UI never shows sibling servers, so the collision is invisible. Carried from the previous round; worth a guard now that slug resolution is load-bearing.

P2-6 — Manifest slug that names no stored server still falls back positionally. mcpService.ts:641-645 / :864-869. Strictly better than the previous positional-always behaviour, but for a document whose manifest_json.name matches nothing the original corruption class survives. Failing closed (render read-only, block the metadata write) would be more honest than guessing.

P2-7 — transport is read unnormalized from the raw mcp.json. mcpService.ts:652: transport: server.type ?? "stdio". The value is typed McpTransport but is an unvalidated wire string. A connector published outside this client with type: "streamable_http" (the very spelling this app's own JSON tab emits) or with a transport field instead of type fails isRemote(), renders a stdio snippet with no URL, and — via detailToForm at McpCreateModal.tsx:192 — opens the edit modal on the stdio branch with the URL field hidden. importJson.ts's inferTransport already has the normalization; the read mapper should reuse it.

P2-8 — Dead export + stale comment left by the refactor. redactMcpConfig.ts:101-103's redactUrl now has no caller (redactServer:195 uses redactUrlDeep(..., OPAQUE_URL) directly), and __tests__/redactMcpConfig.test.ts:235's title still says "redactUrl runs unconditionally" although hasUrlShape gates it. Also, the SecretMask docstring at redactMcpConfig.ts:53-57 and the commit message both say the snippet uses a ${KEY} placeholder; the actual token is <把这里换成你的 KEY>, and ${KEY} is a different, load-bearing shape (PLACEHOLDER_PATTERN at pluginWire.ts:182). Worth disambiguating so nobody wires the two together later.

P2-9 — Heuristic tails unchanged from last round (all re-verified by execution, none regressed): -HAuthorization: Bearer … (compact single-token flag), API_KEY+=…, https:example.com#… (opaque scheme), --auth-header X-Custom: …, ["-e","API_KEY","<value>"] (masks the name, keeps the value), DATABASE_URL=postgres://u:pw@host/db, and isSafeAttachmentPath's Windows-absolute / dot-segment / trailing-space gaps plus its byte-for-byte duplication across the two packages.

P2-10 — Carried, unchanged: whitespace-only forward draft passes the guard (PromptForwardActions/index.tsx:188 — Boolean(draft) should be Boolean(draft.trim())); the localStorage spaceId fallback; the dead expertWire.ts legacy mappers; MineTable's role="table" without rowgroup; the expert 我的发布 100-record cap; /plugin_tags sending scene_code for two markets but not the third; nextCursor ignoring the envelope's has_more / next_cursor; the extra serialized round-trip in fetchMcpListPath. One addition: PromptForwardActions/index.tsx:137-140 resets draft and exits edit mode whenever the prompt prop changes by value — correct when the host swaps prompts, but it will discard in-progress user edits if a host ever refetches the source data while the modal is open.

---

3. Overall verdict

APPROVE

Spec ✅, Quality Approved. Both round-6 blockers are closed and verified by execution, the two P1s from the same round are addressed, CI is fully green at this head, and every remaining finding is either a fail-safe-direction tradeoff or a narrow heuristic tail that is no worse than the pre-migration posture. P2-1 and P2-2 are the two I would fix, but neither loses data nor leaks credentials, so neither should hold the merge.

---

4. Suggested direction

1. Split the snippet renderer's depth from the expert-spec renderer's depth (P2-1). One rule set, two policies — not two mask tokens over one policy. The -e VARNAME case is a two-line guard.
2. Omit visibility from the skill edit payload when the source value was unrecognized (P2-2); updateSkill:596 already preserves the raw value.
3. Fold P2-3 / P2-4 into the same commit — both are one-line predicate widenings.
4. Update the PR body's "Read-path behaviour change" bullet to describe this head, not the previous one.

---

5. Additional observations

Please verify manually before merge (this PR is labelled security-sensitive):

1. The exposure posture still needs a named owner. The state at this head is: on a space/system connector detail, URL query/userinfo/fragment and secret-bearing args are masked; env/header values are masked only when the key name is secret-shaped or the author marked the key user-supplied; command is emitted verbatim. Whether that filter is acceptable — and whether client-side masking is an acceptable substitute for the read-time blanking the legacy path did server-side — is a product decision this repository cannot settle. The durable fix is owner-scoped field projection on the backend's /plugins/detail.
2. Cross-repo deploy coupling. The write paths emit cowork-plugin-*-2.0.json $schema ids, send an optional version on /plugins/upsert, and rely on connector create auto-attaching the default-scene placement. All require the corresponding backend PR deployed, and there is deliberately no dual-version tolerance. This must not release ahead of that backend.
3. /metrics/track contract — both skill and expert tracking now send resource_type: "plugin" instead of the per-type value; confirm counters still increment.
4. Authenticated package download — expertService.fetchSkillPackage issues a bare fetch to ${BASE}/plugins/download with hand-attached token / X-Space-Id headers; confirm CORS permits those request headers and the size cap still applies.
5. e2e coverage is contract-shaped, not contract-verified. The MSW handlers ignore query parameters, so a parameter-name mismatch against the real backend (q vs keyword, tag vs tags, sort enum values) would not be caught. Worth one manual pass against a real deployment.

Round count — this needs a scheduling decision, not another review round. This PR now carries 46 formal reviews across 89 commits and 112 files. Two of the three items I would still change (P2-1, P2-2) are direct consequences of fixes the previous round explicitly asked for — the review loop is now correcting its own asks, which is the signature of a review surface too large to hold in one pass rather than of a careless author. I am approving rather than opening round 8 for exactly that reason. My recommendation, echoing the last two rounds: land this, and take the connector secret-redaction work out into its own PR with its own written threat model, because it is currently being designed incrementally inside a 112-file UI branch. That call belongs to whoever owns this branch's schedule.

Verification coverage / what I did not check:

• No node_modules in this checkout, so I did not run the suites locally; CI is green at this head (Unit tests, e2e-p0, Build, secret-scan, osv-scan, dependency-review). I verified the two changed helper modules by extracting them and executing them directly against every input in the tables above.
• I did not re-verify the unified wire contract against the backend sources this round — nothing in this delta touches it. I have not audited the backend's space/private authorization, its attachment-path normalization, or its archive-extraction behaviour.
• No browser was driven: CSS, responsive layout, and the real a11y behaviour of MineTable, the sort row, and the editable prompt remain unexercised. There is still no lint job in this PR's check list.
• Two independent second-opinion passes were run alongside this review. One contributed P2-3, P2-4, P2-6 and P2-7, and independently raised P2-2 at a higher severity than I settled on — I re-verified each by execution before including it, and downgraded its multi-server findings after confirming the slug fix closes the reachable case. The other raised the preview-prop removal and the copyTrackEvent telemetry change as blockers; I refuted both — PromptForwardActions has no remaining in-repo consumer that passes preview (all five surfaces go through PromptForwardModal), and four of the five surfaces pass copyTrackEvent with the expert/squad surface documented as intentionally uncounted. Its third item became the addition in P2-10.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Code Review — PR #1584 (octo-web)

Reviewer: Octo-Q (automated review)

Summary

This PR migrates all three market catalogs (skills / connectors / experts) off the legacy per-type octo-marketplace endpoints onto the unified plugin surface (/plugins, /plugins/detail, /plugin_categories, /plugins/import|upsert|delete|install|skill_md|versions, with a plugin_type discriminator), and restructures the market UI: the per-market 全部/我的 tabs are replaced by a new /mcp-market/mine page (MyAssetsPage) that mounts each market page in variant="mine", discovery cards get prompt-forwarding primary actions (连接/安装), and the bot publish / install / connect modals consolidate onto the shared PromptForwardModal with prop-driven telemetry. Because the marketplace backend's secret scanner was deliberately removed, the client now owns the secret contract: user-supplied env/header values are persisted only as ${KEY} placeholders (typed values are never sent), display surfaces mask secret-shaped values via the new redactMcpConfig, and visibility / category resolution fail closed. The migration is unusually disciplined; I found no blocking defects, one P2 display-redaction residual, and one duplication nit.

Verification

Static analysis only at head e4345462; build and tests not executed in this environment.

• ✅ Secret round-trip never sends values — placeholderFor (writer, packages/dmworkmcp/src/api/mcpWireParams.ts:194) and selfPlaceholder/splitUserSupplied (reader, packages/dmworkmcp/src/api/pluginWire.ts:196) use byte-identical key normalization; user-supplied values are substituted with placeholders at write time (placeholderSecretMap), cross-referential ${OTHER} values pass through unrenamed, and the legacy __OCTO_SECRET_PLACEHOLDER__ sentinel blanks on both paths. Locked by packages/dmworkmcp/src/api/mcpService.visibility.test.ts.
• ✅ Fail-closed write/list paths — importBody defaults absent visibility to "private" on full-replace (packages/dmworkskillmarket/src/api/skillApiReal.ts:505); unresolved categories abort create/update (resolveWriteCategory) and list queries return an explicit empty result instead of widening to the whole catalog (pinned by packages/dmworkmcp/src/api/expertService.categoryFailClosed.test.ts).
• ✅ Authenticated download wiring — /plugins/download URLs are only consumed through fetchSkillPackage (packages/dmworkmcp/src/components/ExpertSkillBrowser.tsx:249), which attaches marketplace auth headers for /market paths, scheme-guards external URLs, and keeps the 20 MiB streaming cap.
• ✅ 401/teardown semantics preserved — metrics beacons opt out of session teardown on both clients (skipAuthRedirect in skillApiReal.ts:145; exact-path exclusion in expertService.ts:191), and FetchRules/TrackRules remap matches the new endpoints (upsert deliberately unmapped for create-vs-edit ambiguity).
• ✅ Read-modify-write hardening — attachment paths re-emitted into /plugins/upsert are validated (isSafeAttachmentPath: absolute / backslash / NUL / .. traversal rejected) in both the skill and connector paths.

Findings

No P0/P1 issues; one P2 and one Nit below.

P2 — Display redaction leaves command/cwd/URL-path secrets exposed on public plugins (packages/dmworkmcp/src/utils/redactMcpConfig.ts:39)

redactServer copies STRING_FIELDS (type/transport/command/cwd) verbatim and redactUrlDeep keeps URL path segments, while env/headers/args/query/userinfo are all masked. This PR simultaneously documents that the backend has no secret scanner and blanks nothing on read (packages/dmworkmcp/src/api/pluginWire.ts:9), and the rendered mcp.json is shown to every viewer of a public/system plugin's detail. A hand-written credential in a stdio command one-liner (e.g. sh -c "curl -H 'Authorization: Bearer sk-...'") or embedded in a URL path therefore renders unmasked to all authenticated viewers, while the same secret under env/headers/args would be masked; the create form's sharedSecretLeaks advisory also only inspects env/header rows, so authors get no warning for the command case. Fix direction: run command through the same redactArgs token rules (flag value / KEY=value / Header: value shapes) before copying, or restore server-side scanning for public/system rows.

Nit — Duplicated attachment-path guard across packages (packages/dmworkskillmarket/src/api/skillApiReal.ts:252)

isSafeAttachmentPath is duplicated verbatim in skillApiReal.ts and packages/dmworkmcp/src/api/mcpWireParams.ts:43 (and pluginWire.ts itself is duplicated across the two packages). If this security validation evolves (new unsafe patterns), the copies can diverge and leave one package with weaker protection. Consider hoisting it into @octo/base or a shared security utility.

Human-verify

1. Legacy bearer backfill — the removed client shim used to synthesize a user-supplied Authorization slot for pre-toggle auth_type: "bearer" records; the new pipeline assumes backfill wrote an explicit Authorization entry into those records' mcp.json attachments. If a row was missed, its quick-start snippet silently loses the Authorization line. Not verifiable from this diff; not a merge blocker for this PR.
2. Unified list total contract — skill-list cursor synthesis (page * pageSize < total, packages/dmworkskillmarket/src/api/skillApiReal.ts:458) trusts pagination.total. The MCP path already relied on it pre-PR, but worth confirming the unified /plugins envelope always includes total for plugin_type=skill (if omitted, infinite scroll would stop after page 1). Not a merge blocker from this diff.
3. Server-side ownership on unified writes — owner-only enforcement for /plugins/upsert|delete now lives entirely on the backend (client sends only plugin_id). Worth confirming the unified endpoints enforce ownership and visibility on write the way the legacy per-type endpoints did. Not a merge blocker for this frontend diff.

Things I checked that are fine

• Publish-modal kind wiring is consistent: publish button only renders in the mine variant, and MyAssetsPage always passes an explicit mineType; removal of publish from discovery tabs is intentional restructure.
• creatorId left undefined by mapSkill on purpose; SkillCard.tsx:102 / SkillDetailModal.tsx:227 guard with hasComparableIds and fall back to name comparison, preserving bot-authored attribution.
• Multi-server mcp.json handling selects the modeled server by manifest slug (not position) on both read (mapDetail) and write (updateMcpReal), avoiding the alphabetical-sort overwrite hazard; unmodeled servers/attachments/keys are echoed back, not dropped.
• parseTeamAgentsMarkdown fails closed outside the ## 协作方式 region, so summary prose cannot inject leader/策略/依赖/权限 config.
• Shell-injection surface in prompts: sanitizeShellSpaceId guards the space id in install/connect/publish prompts; mcpId/skillId are server-issued and apiBaseUrl is origin-only.
• All new i18n keys exist in both locales; e2e msw fixtures migrated to plugin-wire shapes consistent with the mappers; tests drive production functions through mocked fetch/axios rather than hand-feeding internal mappers.

Verdict: COMMENT

No correctness, security, or build-breaking issues found; the migration preserves the legacy runtime contracts (auth redirect opt-outs, byte caps, fail-closed visibility/category, placeholder round-trip) while moving them onto the unified surface. The P2 is a display-hardening gap on a documented residual, and the nit is maintainability — both non-blocking.

数据流回溯（Octo 补充段）

• Skill.{viewCount,downloadCount} ← wire view_count/download_count（列表项直映）；readmeContent/fileUrl ← mapSkillDetail 从 plugin_json.attachments 解析（legacy skill/ref.json 指针 vs tree 形态），下载按钮已随 downloadSkill 导出一起从 UI 移除，fileUrl 当前无 UI 消费点（非悬空渲染）。
• McpQuickStart.{env,headers,*UserSupplied} ← mcp.json attachment → splitUserSupplied；snippet 渲染前经 shouldPlaceholder 二次掩码；写入经 placeholderSecretMap 还原占位符 —— 读写两端归一化函数逐字一致，round-trip 有测试锁定。
• 专家成员/技能 ← expert_team_expert/expert_skill relations fan-out；仅 confirmed 404 丢弃、其余错误重抛，列表不会被瞬时错误静默截短。
• 技能列表分页 ← offset+total 合成游标，与 PR 前 MCP 路径同契约（见 Human-verify 2）。

盲点 checklist（Octo C1–C6）

• C1 双路径 parity：create/upsert ↔ /plugins/delete 三类型均已核对；mode=mine ↔ 全量列表同函数参数化；安装回滚在服务端契约内。clear。
• C2 复用/嵌套：toPluginUpsert（create+update 共用）、redactArgs/redactUrlDeep（专家 spec + 连接器 snippet 共用，掩码深度不漂移）、parseTeamAgentsMarkdown 边界（注入子标题不生效）已验证。clear。
• C3 授权边界：客户端仅传 plugin_id，写端点鉴权完全依赖后端 → 列入 Human-verify 3。
• C4 授权生命周期：unknown visibility 展示侧降级为 space（非放宽），导入缺省 private（fail closed），编辑回显保留原 scope；容器级级联在后端范围外（并入 Human-verify 3）。clear（前端侧）。
• C5 build≠运行期：本环境未跑 build/测试（静态审查），下载鉴权头附带、分页游标、401 opt-out 均做了运行期路径推演并注明。
• C6 治理文档：本 PR 未触碰 SECURITY/治理类文档。N/A。

跨轮 blocker 复检（R6）

N/A — 本 PR 首轮审查，无上一轮未解决 blocker。

[Octo-Q] verdict: APPROVE — automated review未发现 P0/P1：唯一实质 finding 为 P2（public 插件 mcp.json 展示侧 command/URL-path 密钥残留掩码）+ 1 Nit（路径守卫跨包重复）。按 R4 仅 P2/Nit → APPROVE；对外终审 verdict 以 Opus 复核为准。

[Jerry-Xin]

Code Review — PR #1584 (octo-web) — re-review round 8 (supersedes my RC 5074089012)

Reviewed at head e4345462d437d134195ae287da8c4004c41b4159 (delta vs 5d9db51106f2 is a single commit, e4345462 "fix(mcp): unify connector/expert redaction + fail-closed multi-server & visibility"). Both 🔴 blockers from my round-7 REQUEST_CHANGES (5074089012) are byte-verified fixed — verified independently, not adopted from peer approvals.

B1 — connector quick-start args redaction (FIXED)

Both tabs now route args/url through the expert-side rules before rendering:

• packages/dmworkmcp/src/api/quickStartTemplates.ts — buildJson (:106) and buildPrompt (:239) call redactArgs(qs.args, snippetMask); both url sites call redactUrlDeep (:94, :231). Headers/env go through shouldPlaceholder (secret-shaped keys → fillable placeholder).
• Runtime-verified with a low-entropy canary probe (LEAKcanary* literals, since removed): docker-style -e API_TOKEN=…, --token …, a positional https://u:…@host/v1?sig=…#access_token=…, an mcp-remote-style positional URL with ?token=…, and a shared secret env value are all masked on BOTH the 提示词 and JSON tabs, while endpoint host/path and non-secret values stay readable.
• Surface enumeration: McpDetailModal QuickAccess renders only buildQuickStartTabs output; mcpConnectPrompt.ts embeds only mcpId/spaceId/apiBaseUrl (spaceId sanitized); McpCreateModal raw reads are the documented owner-edit flow. No unredacted connector surface remains.

B2 — URL redaction depth unified across surfaces (FIXED)

Expert and connector now share ONE implementation, redactUrlDeep(url, mask) (packages/dmworkmcp/src/utils/redactMcpConfig.ts:67): userinfo, EVERY query-param value — including non-secret-named ones like ?sig=, canary-verified — and #access_token fragments are masked; only exact ${KEY} placeholders pass through. Surfaces can differ only in mask renderer (opaque ••••••/REDACTED vs fillable placeholder), i.e. the unified rule set I asked for — the wider-audience/shallower-function asymmetry is structurally gone.

yujiwei's round-7 findings — verified fixed (credit yujiwei)

• 🔴 Description accuracy: the live PR body now discloses the @octo/base shared-layer changes (PromptForwardActions prop break: preview removed, kind/copyTrackEvent added; optional PromptForwardModal.title; TrackRules/DAP_EVENTS; 9 promptForward.* i18n keys), the packages/dmworkbase/src/__tests__/setup.ts test-harness stub, and the read-path behaviour change for secret-shaped shared keys. Matches the diff.
• 🔴 Multi-server read/write inconsistency: mapDetail (packages/dmworkmcp/src/api/mcpService.ts:636) and updateMcpReal (:862) now resolve the modeled server by manifest slug (with an identical first-key fallback), matching the write key slug || name in packages/dmworkmcp/src/api/mcpWireParams.ts — the a-helper/github-mcp corruption scenario yujiwei described can no longer occur. Regression test added (mcpService.connector.test.ts, slug-not-first case).
• Visibility fail-closed: unknown/absent skill visibility now maps to private (packages/dmworkskillmarket/src/api/skillApiReal.ts:326-332), and the re-upload write fails closed to private at :555 — pinned by updated tests.

mochashanyao's 🟡 notes — independently assessed

1. command/cwd/URL-path values pass through unmasked: REAL but a documented, pre-existing accepted residual (redactMcpConfig doc comment: display-only <pre>, "the endpoint is not the secret"). Not a blocker; the suggested command-tokenization or server-side scanning for public/system rows is a reasonable follow-up.
2. isSafeAttachmentPath duplicated verbatim between packages/dmworkskillmarket/src/api/skillApiReal.ts and packages/dmworkmcp/src/api/mcpWireParams.ts: REAL nit; hoist to a shared utility so the copies can't diverge. Non-blocking.

Fix lineage — spot-checked, no regression

Positional-URL-arg token masking ✓ · fragment #access_token masking ✓ · redactMcpConfig whitelist rebuild (root siblings dropped, non-object server → null fail-closed) ✓ · rawServer-seed + extraServers preservation on full-replace writes ✓ · visibility fail-closed on read and write ✓ · detail-read credential redaction ✓.

Non-blocking (codex second pass)

• 🟡 packages/dmworkmcp/src/pages/MyAssetsPage.tsx:64 — tab switch doesn't sync ?type=; refresh/share can reopen a different tab.
• 🟡 packages/dmworkmcp/src/api/expertService.ts:500 — squad detail fans out one request per member (+ per member skill); large squads burst. Consider batching/lazy hydration.
• 🟡 packages/dmworkbase/src/Components/PromptForwardActions/index.css:181 — hard-coded spacing/sizes instead of design tokens.

Tests

• pnpm --filter @dmwork/mcp test: 256 passed (253 suite + 3 temporary canary probes, removed afterwards).
• pnpm --filter @dmwork/skillmarket test: 92/136 pass locally — the 44 skillApiReal.test.ts failures reproduce byte-identically at the prior head and a trivial localStorage probe fails the same way, so this is a local Node v26/jsdom storage-env quirk, not caused by this PR; CI Unit tests is green on this head.
• All CI checks green at this head: unit, build, e2e-p0, gitleaks, install-build.

APPROVE — supersedes my REQUEST_CHANGES 5074089012. Credit yujiwei for the multi-server + description findings; mochashanyao for the residual-redaction and duplication notes.