feat: expose Space review auto-approval policy - #1624
Conversation
Uploading a skill used to publish it to the whole org immediately. The backend now lands every tenant create as a private draft and refuses to modify a listed plugin through the ordinary write path, so the market needs a UI for the review workflow that replaces it. - Publish scope is an explicit choice in NewSkillModal: keep it private, or submit for org review with an editable version label (pre-filled with a suggested bump) and changelog. A skill upgrade with a fresh zip submits via parse_task_id so the backend materializes the package itself. - New 组织审核 queue (SpaceReviewPage / ReviewQueue) with 待审核 and 已处理 sub-tabs, a detail drawer carrying the frozen snapshot preview and the "current vX → requested vY" callout, and a reason-required RejectReasonModal. - MineTable derives per-row状态 badges and actions (提交审核 / 撤回 / 重新提交 / 发布新版本) by joining the mode=mine request list onto the skill list. Review state stays an independent entity — no review column on Skill and no new Visibility member — because a listed v1 and an in-review v2 coexist. - useSpaceRole reads space_roles from the auth context to decide whether to show the queue. The gate is cosmetic: the server is authoritative and answers 403/404 regardless. - useReviewRequests plus the six review endpoints in skillApiReal, mirrored in skillApiMock, with the snake_case wire shapes in pluginWire. - i18n keys for both locales; SpaceReviewPage is re-exported from index.tsx so dmworkmcp can mount it without reaching into internals.
The Space review workflow covers every plugin type, so the MCP and expert markets need the same private-draft lifecycle the skill market just got. - Creates default to visibility "private" instead of publishing straight to the org; going org-wide is now a review submission. - ReviewSubmitModal and useMyReviewState give connectors, experts and expert teams the submit / withdraw / resubmit / publish-new-version actions and the matching status badges, funnelled through api/pluginReview.ts so the coupling to the skill market's contract stays in one file. - Submitting a container (expert / expert_team) sends its relation graph with the request, including per-member data keys, so the reviewer approves the membership that will actually ship rather than whatever the live graph held at approve time. - MarketSidebar mounts the shared 组织审核 entry (page owned by @dmwork/skillmarket) and reveals it once the role probe resolves; module.tsx routes it alongside the skills tab so both markets share one NavRail entry. - i18n keys for both locales.
Follows the octo-marketplace listing_state change. The market UI had three
problems this fixes together, because they share the same vocabulary.
**Publishing is one button.** The create form's 发布范围 radio ("提交组织审核" /
"仅自己可见（私有）") encoded a routing decision the server owns, and threw the
author's actual intent away — every plugin was created `private` regardless of
what they picked. It is now a real 可见范围 choice that is STORED, and the footer
offers 保存草稿 and 发布. 发布 calls /plugins/publish and the backend decides from
the stored visibility whether that lists immediately or opens a review; the
response says which, so the toast does not guess. 提交审核 / 重新提交 /
发布新版本 collapse into 发布 and 升级版本.
发布 is also a row action, not only a modal footer. Connectors are authored
through a wizard and experts through a bot flow, so "open the editor just to
press 发布" would be the only way to publish three of the four types.
**Status comes from the server.** `display_status` replaces a five-value badge
that three pages each re-derived from a client-side join of the review list, and
disagreed about: a listed plugin with a pending upgrade read 已上架 in one place
and 审核中 in another. The join survives only to look up the rejection TEXT,
which is a property of the request and has nowhere else to live.
**One table, one vocabulary.** 我的发布 was a 5-or-7 column grid (connectors hid
the counters) and 组织审核 was a headerless flex card list; they shared no markup
or CSS. Both now render the same 9-column MineTable — 名称/描述/分类/版本号/
可见范围/浏览量/下载量/状态/操作 — with 描述 and 状态 promoted to real columns.
utils/labels.ts becomes the single place an enum becomes text, which is what
stops 专家团/专家团队, 待审核/审核中 and 已上架/已通过/已发布 from naming the same
things differently depending on the page. The retired keys are deleted, not left
to rot; zh and en stay at parity (258 / 352 keys each).
Also: 组织审核 gains 下架 on a still-listed approved request, gated on the
plugin's current listing state rather than the request outcome, so it is not
offered on something already down. 我的发布 gains an 全部 tab over the
plugin_type-optional listing — the only view that answers "what am I waiting on"
without visiting four tabs. It is read-only by design: 编辑 and 升级版本 need the
owning market's modal, and reproducing that dispatch would guarantee drift.
The skills tab's four review buttons previously rendered aria-label={undefined}
because the aria keys lived only in the mcp namespace; they are in the shared
namespace now and every action carries one.
Tests: skillmarket 208/208, dmworkmcp 261/261. Fixtures that expressed "listed"
as visibility alone now say listing_state too, and gained the cases the old model
could not represent (published-private, space-intent draft, delisted — all
editable). One of those new tests caught a real bug: a single canCreate gate
disabled BOTH footer buttons when 仅本组织可见 had no changelog yet, making 保存草稿
unreachable for exactly the author who most needs it.
…ry market SkillListPage still passed `skillMarket.card.*AriaLabel` (编辑 X) while the connector and expert pages passed the shared `skillMarket.plugin.aria*` (编辑「X」). One MineTable affordance, two accessible names depending on which market you reached it from — the exact inconsistency the shared label module exists to remove, missed on the one page that predates it. `skillMarket.card.*AriaLabel` stays: SkillCard and SkillDetailModal are a different affordance and legitimately keep their own wording.
In the table the value sits next to a lock / building / globe icon inside a pill, so 仅自己可见 spent three characters restating what the column header and the icon already say, and made the pill wide enough to crowd the row. 仅自己 / 本组织 / 全平台. The create-form radio uses the same key and still reads clearly, because each option has an explanatory hint underneath it. The two test regexes that located those radios spelled the old strings out; they now match the new ones. Everything else reads the label through utils/labels.ts, so this is a one-value change per locale.
The four type tabs mount a market page whose own content area supplies the scroll container and the inset padding; the 全部 tab renders MineTable directly, so it matched none of the panel's child selectors and the table sat flush against the panel edges while every sibling tab was inset. It now has a root of its own in that selector list, padded to match .skill-market-content. Avatars are drawn per type as well. A skill rendered here keyed its tile off the connector helper (id-based, two CJK characters) while the same skill in the 技能 tab keyed off the name — so one row had two different faces depending on which tab you found it in. Each type now uses its own market's helper.
The column header said 操作 over a cell that was always empty, because the tab was built read-only. The three actions that do not need a market's authoring surface are now wired: 发布, 取消审核 and 删除 each go through an endpoint that takes a plugin id and nothing else, so one implementation covers all four plugin types. 编辑 and 升级版本 stay out — a connector edits through a wizard and an expert through a bot flow, and reproducing that dispatch here would guarantee this tab drifts from the four that own those flows. Clicking a row opens the tab that can do them. Rows disable themselves until the reload settles, so a second click cannot race an in-flight action, and the list reloads after a failure too: a 409 means the server already knows a state this page does not.
The edit modal had no visibility control and preserved the stored value with a comment saying so. Under the old model that was honest — visibility could not be raised through this path at all, so a control would have been a lie. It is a declared intent now, freely changeable on an unlisted row, and 编辑（基础信息和 可见范围）was in the spec from the start. The hint is the part worth having: widening the audience of a published plugin un-lists it server-side, and an author who is not told beforehand reads that as their plugin disappearing. It shows only when the change would actually do that. Skills only. The connector wizard and the expert bot flow have no visibility control either, and giving them one is a separate change against two very different authoring surfaces.
An author met two different contracts depending on whether the plugin already existed: the create modal offered 保存草稿 and 发布, the edit modal a single 保存 that left you to find 发布 somewhere else. The secondary action is only labelled 保存草稿 when the save actually leaves a draft behind — either the plugin is already unlisted, or widening the audience is about to un-list it. On a published plugin whose audience is unchanged it is a plain 保存, and 发布 is not rendered at all rather than sitting there permanently disabled: there is nothing to publish, and the backend would answer 409 already_published. 发布 saves first, so what gets published is what the author just wrote, and the toast comes from the response — the backend decides whether that meant listing it or opening a review.
…dmin rows Two fixes in the skill write forms. **The leave-guard no longer stacks a dialog.** It put a scrim over a scrim, and the dialog it stacked hid the very thing it asked about — "已上传的文件和填写的 信息将丢失" covered up the file and the information it named. The footer swaps in place instead: one layer, the form stays visible, and the answer gets made while looking at what is at stake. It also asks a better question. The old copy said the work would be lost, which stopped being true when 保存草稿 shipped — and was already false whenever the create had succeeded and only the publish failed, since the plugin was saved. The bar offers 保存草稿并离开 as the primary action, so the honest escape is the easy one. Mid-upload there is nothing a save could keep, so that variant offers only 继续上传 / 确认离开. **The visibility control no longer demotes admin-managed rows.** It narrowed the stored value to "private" | "space" and always sent it, so editing a 全平台 (`system`) plugin — or a legacy `public` row — silently demoted it to private and delisted it from everyone. The state holds the raw value now; the radio renders only for the two audiences a tenant may choose, and an admin-managed one is shown read-only and carried through the save untouched. `.skill-market-form__hint` had no CSS rule anywhere, so the "changing the audience returns this to a draft" warning — the one thing an author must read before saving — rendered as plain body text. It has one now, shared with the expert form. IconCropModal is also stacked and deliberately left alone: it is a task, not a question about the dialog underneath, and a footer bar cannot hold a cropper.
createMcp hardcoded `visibility: "private"`, so the declared audience was thrown away on every create — 本组织 was simply unreachable from the connector form, and nothing downstream complained because the value it discarded is the one the backend reads to route 发布. The wizard gains the 可见范围 radio and the same 取消 / 保存草稿-or-保存 / 发布 footer the skill form uses, publishing through the one backend door that decides from the declared visibility whether to list immediately or open a review. 发布 saves first, so what ships is what the author just wrote, and the toast comes from the response rather than being guessed. The state-adaptive rule carries over too: 发布 is not rendered when a listed connector's audience is unchanged, since there is nothing to publish and the backend would answer 409 already_published. A legacy `system`/`public` row keeps its stored audience rather than being defaulted into one of the two the form can express — the same demotion bug the skill form had. The service test pins the regression this is most vulnerable to: a declared "space" must reach the wire rather than being flattened back to private.
They had no client-side write form at all: 编辑 jumped straight to the Bot prompt, which authors CONTENT through octo-cli. Visibility is marketplace metadata, not content, so making an author round-trip a Bot to change who can see their expert would have been absurd. 编辑 now opens a small form that owns exactly that one field, with the Bot handoff still one click away for content. Hanging it off 编辑 rather than a new row action is deliberate: the backend's gate on WRITING visibility (409 listed_requires_review when published+space, review_pending while a request is open) is verbatim the gate on editing, so reusing the affordance means the two cannot drift. Folding it into ReviewSubmitModal was rejected for the opposite reason — that dialog is reachable only via 升级版本, i.e. exactly the population forbidden from changing visibility, so a private draft would never have seen the control. The load-bearing part is the write. There is no visibility-only endpoint, and /plugins/upsert is a full replace: an omitted relation is soft-deleted. So the save reads the record back and echoes it whole with one field changed — manifest and package verbatim, relations carrying their `relation_id` AND `data`, or a squad silently loses the member_key/is_leader wiring that makes it installable. Verified against the live API on an expert with relations: ids and data survive the round trip intact. The save is skipped entirely when the value is unchanged, because every upsert mints a version snapshot server-side and a no-op save would record a version that changed nothing. A platform-published row shows its audience read-only rather than defaulting into one the tenant path would accept, which would demote an official listing into a single Space.
**A retry after a failed publish no longer drops what was fixed.** Both write forms remembered the id from a create whose publish then failed, so a second press would not mint a duplicate — but they also SKIPPED the write entirely on that remembered id. Anything the author changed after the failure, usually the very thing that caused it, was silently discarded while the toast reported the draft saved. The retry now updates the row it already created. The no-duplicate property is unchanged; only the missing write is added. **A connector publish that opens a review now carries a changelog.** The wizard collected one only in 升级版本 mode, so an organization review arrived with an empty 更新说明 — the text the reviewer decides on. It is asked for exactly when the declared audience is 本组织, which is the branch that opens a review; a 仅自己 publish has nobody to explain anything to. 发布 is gated on it, 保存草稿 is not, so an author who is not ready to describe the change can still save. **ReviewSubmitModal had class names and no rules at all** — the notice, both fields, the frozen-children line and the error all rendered as undifferentiated body text. That dialog is the 发布/升级版本 surface for three of the four plugin types.
The 操作 track was `minmax(140px, max-content)`, but the header and every row are SEPARATE grid containers, so a content-sized track resolves independently in each one. A row holding 通过 + 拒绝 got a wider 操作 than the header holding the word 操作, and every column to its left shifted to compensate. Across tabs it was worse: 全部 has no row actions at all, so its whole grid resolved differently from 技能's — which is what "表头还是不一样" was. A fixed 180px clears the widest real combination (two labelled buttons, or one plus the 编辑/删除 icon pair) and makes the layout independent of content. `.wk-mcp-mine__all` also had no background. The four market pages carry `--wk-bg-surface` on their own root, so the 全部 panel sat on the base colour while its siblings sat on the surface one.
… server Mirrors the backend rule so a bad label is objected to before the round trip: three numeric parts (1.0.1), and only forward or unchanged. Parts compare numerically — 1.10.0 follows 1.9.0, which a string comparison gets backwards. One helper, four inputs: create, edit, the connector wizard's 升级版本 step, and the shared ReviewSubmitModal that experts and expert teams submit through. Each compares against the label the SERVER will compare against — the stored one — so the form cannot disagree with the response. The message renders under the input rather than in the form-wide banner, which has no way to point at a field. The two exemptions are mirrored too: an unchanged label always passes, and an unorderable stored one cannot block a well-formed replacement. Without them the form would refuse to save the rows carrying labels minted before the format was tightened, which is the same trap the server-side rule had to avoid. The server remains the gate; this only saves a 400.
…ate today The 已处理 status column answered two different questions at once. It took pending/approved/rejected from the REQUEST, but let a later 下架 overwrite an approval — so a decision appeared to change months after it was made, and two approvals of the same plugin rendered identically, saying nothing about either. A rejected record meanwhile kept reading 驳回 after the author had fixed and republished: the same column following live state for one outcome and history for another. A row in 已处理 is a record. It now says what was decided — 已通过 / 驳回 / 已撤回 — and nothing else. The plugin's current listing state is still read, but only to gate 下架, since you cannot take down what is not up. MineTable gained an explicit statusLabel/statusTone override for this. The alternative was merging review outcomes into PluginDisplayStatus, which is the conflation that caused the bug: 已通过 is not a plugin state, and a plugin state is not a decision.
The page stopped being only a review queue when it gained 下架: an admin now approves, rejects and delists from it, which is publication management rather than review alone. English follows as "Publication management", sitting next to the existing "My publications" sibling. Two keys carried the label, not one — the sidebar entry (mcp.sidebar.review) and the page title, which the review table also uses as its accessible name (skillMarket.review.orgTab). Moving one without the other would have made the sidebar and the page it opens disagree. Only the MENU label changes. The route, the component names, the API and every other 审核 string — 待审核, 审核中, 取消审核, 提交审核, 审核人 — stay: the underlying workflow really is a review, and renaming it would be a far larger and riskier change for no user-visible gain. The two comments that quoted the old label as UI copy were corrected, and the page's doc block now records why the label diverges from the code's wording so the next reader does not "fix" it.
The 操作 column was right-aligned inside a track much wider than any real button set, so the header word sat hard against the band's right edge with a dead run of space between 状态 and it. 我的发布 全部 showed this worst — most of its rows carry one or two buttons — which read as a broken header rather than as a column. Left-align the cell (the header shares the class) so the label sits directly over the buttons it names, matching the other eight columns. Size the track at a fixed 160px at every breakpoint instead of 180px / a content-sized max-content. Measured in a browser, the widest combination any page wires is 通过 + 拒绝 at 155.3px in en-US; zh-CN's widest is 发布 + 编辑 + 删除 at 127px. The two content-sized breakpoints resolved the header and each row independently: at 760px the header's track came out 96px while rows came out 122–127px, tearing the columns apart, and rows carrying different actions disagreed with each other. 下架 gains the mirror of the Upload arrow that marks 发布/升级版本, so the one labelled action that used to render bare now reads like its siblings.
The sidebar's pending-count badge and the 待审核 list were two independent reads of the same queue with no link between them. The badge is its own `useReviewRequests` (a page_size=1 probe mounted by MarketSidebar's <ReviewGateProbe />) and fetched only on mount; ReviewQueue refreshed its own list after a decision and told nobody. So 通过 / 拒绝 emptied the list while the badge kept showing the count from page load, and only a full reload corrected it. The badge cannot simply read the queue's state — it has to render while ReviewQueue is unmounted (the user standing on 技能 / 连接器 / 我的发布), so two reads are necessary. What they can share is the moment both become wrong, and that moment is a review mutation. `api/reviewSignal.ts` adds that signal and `api/skillApi.ts` attaches it to the endpoints themselves rather than to their callers, so approve / reject / cancel / publish / delist / plugin delete all invalidate whether they are invoked from the queue table, the detail drawer, the applicant's own rows in three market pages, or anything added later. Every live `useReviewRequests` re-reads. No polling: every event that moves this Space's count in this session goes through those endpoints. A refused call invalidates too — a 409 from a decision endpoint is precisely the case where our copy is already stale. Verified end to end with a new e2e case (RB1): with the fix reverted the badge stays at 1 while the queue empties; with it, the badge clears on the same page instance.
我的发布's chrome — the page title and the 全部/技能/连接器/专家/专家团 tab strip — carries no responsive rule: it stays inset by var(--wk-sp-6) at every width. Four of the five tabs' content areas hold that inset. The 技能 tab did not: `.skill-market-content` drops to a flat var(--wk-sp-3) under 760px, so below that width the 技能 table alone rendered 12px wider on each side than 全部, 连接器, 专家, 专家团 and than the tab strip directly above it, visibly stepping out from under its own tabs. Measured in Chromium at 740px before the fix: the tab strip's content edge is x=380 and the 全部 table is x=380 w=336, while the 技能 table is x=368 w=360. At 1600 / 1280 / 900 every tab already agreed, which is why this only shows up on a narrow window. The 12px is right where the rule was written — the standalone 技能市场 route, a phone, no hero and no tab strip to line up with — so it is scoped rather than removed: `.wk-mcp-mine__panel .skill-market-content` re-pins the inset inside the 我的发布 panel only, next to the existing `.skill-market-topbar` override that already re-chromes the hosted page there. /mcp-market/skills standalone still measures 12px at 740 and 640. Pinned unconditionally rather than in a matching media query: above 760px this is already the computed value, so it is a no-op there, and stating it once keeps the panel aligned if the skills page adds another breakpoint later.
…selves
`withReviewInvalidation` moved review-queue invalidation onto the seven
mutating endpoints, so every live `useReviewRequests` re-reads the moment one
of them settles. Eight call sites still poked their own review hook afterwards,
which is not free: `refresh()` aborts the in-flight fetch the signal just
started and issues an identical one, so each publish / cancel / listing outcome
made two `GET /plugins/review_requests?mode=mine&page_size=100` round trips and
threw the first away.
Observed at 我的发布 → 技能 in the e2e-mock browser before:
POST /plugins/publish
GET review_requests mode=space status=pending page_size=1 (sidebar badge)
GET review_requests mode=mine page_size=100 (aborted)
GET plugins?mode=mine
GET review_requests mode=mine page_size=100 (the duplicate)
GET review_requests mode=mine status=pending page_size=1
and after, without the fourth line. The row still moves 草稿 → 审核中 → 草稿
across publish and 取消审核 on all three pages, because what drives the row's
status and its 取消审核 button is `display_status` / `review_id` on the PLUGIN —
`list.refresh()` / `loadData()` / `reload()`, which stay. The review read feeds
only the rejection-reason tooltip, and the signal keeps that current.
Left alone deliberately:
- the three `space-changed` handlers — a Space switch is not a mutation and
`useReviewRequests` keys its fetch on mode/status/pageSize only;
- `SkillListPage.handleCreated` — NewSkillModal's 保存草稿 branch reaches only
`createSkill` / `updateSkill`;
- `ExpertMarketListPage.handleListingChanged` — ExpertEditModal's 保存草稿
branch calls only `updateExpertVisibility`, and a narrowed audience can
un-list the record and close an open request. Commented so it does not look
like an oversight next to its neighbours;
- all of `ReviewQueue`, which owns private fetchers rather than
`useReviewRequests` and is therefore not a subscriber at all.
Verified: RB1 passes, 14/15 of the skills + experts + mcp e2e suites pass with
the one failure (C40, connector delete) failing identically on the parent
commit; dmworkskillmarket 229/229 and dmworkmcp 270/270 unit tests pass; tsc
per-file error counts unchanged in both packages.
The backend tightened plugin version labels to x.y.z and added a grandfathering exemption for the labels that reached production first (`1.0`, `v1.2.3`, `2.0.0-beta.1`, `v999`, `1.0.0lll`): a malformed label is accepted when it is byte-equal to the one the ROW already stores. `versionErrorKey` mirrored the format and ordering rules but not the exemption, and EditSkillModal seeds its version field from exactly that stored label — so every legacy-labeled skill opened with a value its own client rule rejected, `canSave` was false, and Save was permanently dead with no way forward. The exemption was unreachable from the browser. `versionErrorKey` now takes the stored label as a separate argument and exempts it the way the server does — byte equality modulo surrounding space, keyed on the fetched row, never on anything typed into the form. A DIFFERENT malformed label stays refused, because the server refuses it too; permitting it would only move the dead end to the response. Two rules deliberately do not change. Forward-only still refuses a downgrade between two well-formed labels, and an unparseable current label still blocks nothing — that is what lets a row stranded on `v999` be repaired to a real version. 发布 does not inherit the exemption: on 本组织 it routes through SubmitReview, which gates on validVersion with no grandfathering. It is gated separately and says why, so it does not become the next dead button. On 仅自己 the server ignores the submitted label entirely. `bumpPatch` no longer mangles a legacy label after a reupload. Bumping the last dot-part of `v1.2.3` produced `v1.2.4` — neither well formed nor the stored label, the one combination /plugins/import refuses outright. Verified in the e2e-mock browser: all five legacy labels went from a dead 保存草稿 to a save that puts the stored label on the wire, `9.9` stays refused, and `2.0.0` → `0.0.1` stays refused.
NewSkillModal's review mode computed `versionError` and rendered it under the version field, but `canCreate` never consulted it — only the create branch did. So the form could say "版本号格式为 1.0.1（三段数字）" and let 提交 through anyway, straight into a 400 from SubmitReview. The seed made that reachable rather than theoretical: the local `bumpPatch` bumped the last dot-part of whatever the row stored, turning `v1.2.3` into `v1.2.4` and `2.0.0-beta.1` into `2.0.1.1`. SubmitReview gates on validVersion with no grandfathering, so a review submission has to be well formed even when the plugin's stored label is not. `nextPatch` falls back to 1.0.0 for anything unorderable, which the forward-only rule accepts precisely because the stored label cannot be compared. The exemption added for the save path is deliberately NOT passed here. This modal only ever writes through POST /plugins/review_requests.
Deleting a plugin settles its pending review request in the same transaction server-side (octo-marketplace `cancelPendingReviewFor`, reason "plugin deleted"), because a request whose plugin is gone can be neither seen nor decided by anyone. The Space's pending count therefore really does drop on a delete. `deleteSkill` has been wrapped for exactly this since the invalidation moved to the endpoints. `deleteMcp`, `deleteExpert` and `deleteSquad` hit the SAME `POST /plugins/delete` from this package's own api modules, so they went unwrapped and their callers refreshed nothing: deleting a connector or an expert that had a request open left the 组织发布管理 badge and the 待审核 list counting a plugin that no longer existed. Reproduced in the browser before the fix — the row disappeared and the badge sat on 1. Wrapped on the endpoint, not on the ~4 call sites, for the reason reviewSignal.ts already gives: the row's 删除, the detail modal's inline confirm and whatever surface comes next all inherit it without knowing it exists. The import is a deep one on purpose. `withReviewInvalidation` has to be the same module instance `useReviewRequests` subscribes to, but these two files must not reach the @dmwork/skillmarket barrel — the suites covering them mock only `axios` + `@octo/base`, which is why api/pluginReview.ts exists as a separate bridge. The subpath resolves to a leaf module with no imports of its own, so it costs their graph nothing.
The badge counts pending requests in the ACTIVE Space — every read carries `X-Space-Id` — but nothing made it re-read when that Space changed, so the previous Space's number stayed on screen. Three things had to line up for this to be reachable, and they did: `useReviewRequests` keys its fetch on mode/status/pageSize, none of which a switch moves; `enabled` is `isReviewer`, which an owner switching to another Space they own leaves true, so the reviewer gate does not incidentally clear it; and the sidebar, unlike the market pages in the right pane, is never remounted by the switch — MarketSidebar replaces only the right pane. Reproduced in the browser first: after the switch no `review_requests` request was issued at all and the badge held its old count, three runs out of three. Fixed with an explicit `refresh()` from a `space-changed` handler, which is what SkillListPage, McpMarketListPage and ExpertMarketListPage each already do. Deliberately NOT `notifyReviewsChanged()`: a Space switch is not a review mutation, and telling every subscriber the queue moved would be a different and false claim.
`onAction?: () => void` was invoked at five sites inside ReviewQueue and passed by none of its mounts — SpaceReviewPage is the only one, and it renders `<ReviewQueue mode="space" />`. Five decision paths announced themselves to nobody while reading, at every call site, as if they were wired. Refreshing siblings is the endpoints' job now: every review mutation the queue issues is withReviewInvalidation-wrapped, so the sidebar badge and any other live useReviewRequests re-read on their own. Left a note on the props interface pointing at that signal, so the next person who needs to react to a decision subscribes instead of threading a prop back through here.
…overflows MineTable's hosts are scrolling column flex containers, so the table is a flex item. Its own `overflow: hidden` (which clips rows to the rounded border) zeroes the item's automatic minimum size, so a list taller than the viewport was shrunk to fit instead of overflowing: nothing scrolled, the rows past the fold were clipped away, and the 44px header — min-content 18px — absorbed the squeeze. Reported as 我的发布's 全部 tab having a shorter header than 技能; 全部 lists all four plugin types so it is simply the tab that overflows first. Measured at 40 rows / 1440x900: 全部 header 18px vs 技能 44px, and the 全部 scroller reported scrollHeight == clientHeight with 30 rows unreachable. With flex-shrink: 0 both headers measure 44px and the 全部 scroller reports 2677px against a 791px client.
The 待审核/已处理 strip painted its active tab and underline with --wk-color-accent — the AI/link purple, reserved for AI-flavoured elements — so it was the only tab strip in the market shell that did not render black. Switch to --wk-brand-primary, the 选中态 token that 我的发布's 全部/技能/连接器/专家/专家团 strip and the expert strip already use, which also keeps dark-mode parity with them (that token maps to the accent under body[theme-mode=dark]). Measured at 1440x900, active tab: color/underline rgb(127, 59, 245) before, rgb(28, 28, 35) after — identical to 我的发布's strip. Inactive rgb(85, 91, 97), hover rgb(31, 35, 41) and the UA focus ring already matched and are untouched; the active weight went 500 -> 600 to match, since every other underline strip in the two market packages already bumps its active tab to semibold.
The MSW mock had no /plugins/review_requests handler, so the badge probe fell through to the live proxy and the CI gate failed on the proxy error. Add a mocked fallback that returns an empty pending set, and correct the C40 delete-failure spec's stale aria label to match the rendered name. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
A Space change left the connector create/detail modal stack open over the new org's list, and AllAssetsList never re-read after the switch. Close the modal stack on space-changed and subscribe AllAssetsList to the same event. Also guard onCancelReview so it only fires with a resolved review id. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1624 (octo-web)
Reviewed at head 6783714, against merge-base bff055a (46 commits, 89 files, +12784/−789).
Scoping note. This branch is stacked on #1614 (head 7d478ba, already approved) but targets main, so the raw diff contains both. I reviewed the incremental delta 7d478ba..6783714 (45 files, +1244/−145) in depth, and treated the #1614 portion as covered by its existing approval since 7d478ba is a direct ancestor here. See Q1 for why the base branch itself needs a decision.
1. Spec compliance
Spec: ❌
Checked against #1623's acceptance list.
Missing
-
"Space switches cannot display stale member/role/policy state." The policy panel does not meet this.
SpaceReviewPage.tsx:38initialisesenabledtotrueandloadPolicy()(:48) never resets it, so after a Space switch the previous Space's value keeps rendering while the newGETis in flight — and if thatGETfails,loadinggoes false (:65) and the switch becomes interactive while showing a value that was never fetched for this Space. On first mount the same default means a failed load renders "自动审核 已开启" — the weakest posture — as though it were server truth. Detail in Q1.Note the contrast with the other surfaces in this same PR, which do satisfy the criterion:
AllAssetsList.tsx:83-90andMcpMarketListPage.tsx:255-281both clear their identifiers synchronously onspace-changed. The policy panel is the one that does not.
Out of scope
useSpaceRole.ts:20addsisSpaceOwnerRole, exported from the package barrel atindex.tsx:19, with zero production consumers — only its own test. #1623 grants the policy to owners and admins, so no owner-only predicate is required. Please drop it or land it with its consumer.- Frozen-relation rendering (
pluginWire.ts:164-172,ReviewDetailDrawer.tsx:239-262,types/skill.ts:97-105), the contacts role-badge module (Contacts/spaceRoleBadge.ts), the MCP all-assets isolation work, the e2e voice-seeding retry (settings-center-voice-support.ts:52-71) and thechat-baseline.tsMSW additions are all outside "policy API mapping + review-management UI". They may each be defensible, but they enlarge an already-stacked delta and are what makes this PR hard to land — see Q1/Q5.
Divergence — none material.
2. Code quality
Quality: Changes-Requested
First, the part that is right — please do not "fix" it
The role-encoding change at SpaceService.tsx:233-235 is correct, and it fixes a real pre-existing bug. I want to state the evidence explicitly, because reading only the diff makes it look like an inverted mapping was introduced.
octo-server is the service that answers GET /v1/space/my and GET /v1/space/:id/members:
modules/space/model.go:38—Role int // 成员角色 0.普通成员 1.管理员 2.拥有者modules/space/api.go:320— the creator is inserted withRole: 2, // owner; new members at:347/:808getRole: 0modules/space/api.go:663-682—mySpacesreturnsRole: sp.Role, a raw passthrough with no remapmodules/space/db.go:862—queryAdminsAndOwnerselectsrole>=1;db.go:439-449updateMemberRoleis documented "仅用于非 owner 角色（0/1）" and guardsrole <> 2modules/space/api_member_role_test.go:57,93-99— owner fixture isrole=2, androle=1(admin) is denied permission to change roles
So the old comment (1: owner, 2: admin, 3: member) was simply wrong, and the old call sites were wrong with it. Concretely, SpaceMembers.isAdmin() was return space.role <= 2, which is true for role 0 — every ordinary member was rendered the member-management action row. And SpaceSettings.isOwner() was role === 1, i.e. it treated admins as owners. Both are fixed here, and the new canManageRoles() = isSpaceOwner(...) now matches the server, which permits role changes to owners only. Good change.
I audited the rest of the codebase for un-migrated space-role integers. The migration is complete with one exception, below (P2).
P1 — Policy panel shows an unfetched value after a failed or superseded load
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx:38,48-66
enabled starts at true and is never invalidated. Two reachable states:
- Space A → B: A's value renders until B's
GETresolves. Disabled during that window, so cosmetic — but if B'sGETrejects,loadingclears at:65,errorrenders, and the toggle is now live on A's value. - First mount with a failing
GET: the panel asserts "自动审核 已开启" — auto-approval on, no manual review — purely from theuseStatedefault.
For a control that decides whether org publications get human review, the UI should not be able to state a posture it has not read. Please carry an unresolved state (boolean | undefined, or a {spaceId, value} pair) and render the control only once the current generation has resolved; on error, render the error instead of a switch.
The generation guards themselves (requestGenerationRef, :46) are sound — this is about the value's initial/failed state, not about ordering.
P1 — New policy panel renders against undefined tokens, breaking dark theme
packages/dmworkskillmarket/src/index.css:3336-3351
border: 1px solid var(--wk-border-color, #e5e7eb);
background: var(--wk-bg-color, #fff);
.skill-market-review-policy p { margin: 6px 0 0; color: #6b7280; }
.skill-market-review-policy__error { color: #dc2626 !important; }
.skill-market-review-policy__toggle input { ... accent-color: #2563eb; }--wk-bg-color and --wk-border-color are not defined in packages/dmworkbase/src/theme/{semantic,primitive,tokens}.css. Both var() calls therefore always resolve to their literal fallbacks, so this panel is a hard #fff card with #6b7280 text in both themes. semantic.css:386-466 overrides --wk-bg-surface to --wk-neutral-800 under body[theme-mode=dark], which is the token this should use (as .skill-market-detail__readme at :1538 already does). #2563eb should be var(--wk-color-accent) — already used for exactly this purpose at index.css:3244. !important at :3349 is called out at DEVELOPMENT.md:547, and var(--wk-color-error) needs no specificity boost.
The adjacent .skill-market-review-policy-confirm block in this same hunk (:3353-3392) does all of this correctly with var(--wk-color-error) / var(--wk-text-secondary) / var(--wk-sp-*) — so this is an inconsistency within one commit, not a house-style disagreement.
P1 — Branch is CONFLICTING, and the stacking needs a landing decision
gh pr view reports mergeable: CONFLICTING. The conflict is in apps/web/e2e-kit/msw-handlers/chat-baseline.ts against main@ca71616 (#1619, merged). Mechanical, but it has to be resolved.
The larger issue: this PR targets main while containing #1614's 36 commits, and #1614 is separately open and approved at 7d478ba — which is this branch's own ancestor. Whichever merges first empties the other. Please either re-base this onto #1614's branch, or state the landing order explicitly so #1614 is closed rather than left to merge into a no-op.
P2 — Aggregate handled-list finalizers skip the generation check
packages/dmworkskillmarket/src/components/ReviewQueue.tsx:255-256, 267
Each individual fetchHandledPage correctly compares spaceGeneration against spaceGenerationRef.current, but the two wrappers do not:
void Promise.all(TERMINAL_STATUSES.map((s) => fetchHandledPage(s, null, false))).finally(() => {
setHandledLoading(false);
});On a Space switch, handleSpaceChanged aborts the old controllers; those inner calls resolve on the next microtask (AbortError is swallowed) and this .finally clears handledLoading while the replacement fetches are still in flight — a brief "no handled requests" flash. Capturing the generation alongside the Promise.all fixes both sites.
P2 — One un-migrated space-role literal
apps/web/src/Pages/Main/index.tsx:259 still reads allSpaces.some(s => s.role === 1 || s.role === 2). {1,2} happens to select admin+owner under both the old and the new encoding, so this is behaviour-neutral today — but it is the last place a future reader can be misled. Please route it through isSpaceAdminOrOwner.
P2 — Policy read is issued for members who deep-link
SpaceReviewPage.tsx:70 calls loadPolicy() unconditionally, while the panel is gated on isReviewer (:126). A member who deep-links fires a GET /plugin_review_policies whose 403 is stored into error and never rendered. Harmless given the server is authoritative (as the file's own docstring states), but gating the load on the resolved role would drop a guaranteed-403 request.
P2 — Frozen relations are rendered as raw identifiers
ReviewDetailDrawer.tsx:246-252 renders relation.targetPluginId, targetPluginType and relationType verbatim. The stated goal is "so reviewers see the graph being approved", but a reviewer cannot tell which expert is being added from a plugin id. Resolving names would make the section actually decision-useful. (Escaping is fine — the <pre>{JSON.stringify(...)}</pre> at :256 is React-escaped.)
P2 — Blanket retry hides an e2e race
apps/web/e2e-kit/tests/settings-center/voice/settings-center-voice-support.ts:64-71 wraps the whole seed-and-open sequence in try { ... } catch { retry once }, discarding the first failure's reason. The comment identifies a real ordering race between the mock runtime and the conversation list's first snapshot; retrying makes CI green without fixing it, and silently absorbs a genuine regression the second time it passes. Unrelated to this PR's subject.
Verified as correct (noting so these are not "fixed" into regressions)
ReviewQueue.refreshAll(:295-303) clearserror/detailId/rejectTarget/delistTarget/actingId, andhandleSpaceChanged(:305) calls it. I checked this specifically, because adding the new generation guards to thefinallyblocks (:385,:407) would otherwise have leaked theactingIdslot and made the reason modals un-cancellable via theif (actingId) returnclose guards at:594/:627. It does not — the clears are there.McpMarketListPage.handleSpaceChanged_(:255-281) clears every modal identifier before reloading.useSpaceRole's generation counter covers thespace-changedpayload fast-path, not just the probe.- All 12 new
skillMarket.review.policy*/relations*keys plusplugin.publishedToastare present in bothzh-CNanden-US. Deletingcontacts.role.3is safe — the old badge only rendered roles 1–2, so it was already dead. mapReviewPolicy's wire shape matchesrequest()'s.dataunwrap atskillApiReal.ts:225and the e2e handler's{data:{...}}envelope.
3. Overall verdict
REQUEST_CHANGES
Spec ❌ (one acceptance criterion unmet, several out-of-scope additions) AND Quality Changes-Requested. The core direction is right and the role-encoding fix is a genuine improvement; the blockers are the policy panel's unfetched-value state, the dark-theme token regression, and the merge/stacking state.
4. Suggested changes
SpaceReviewPage.tsx— makeenabledboolean | undefined; render the switch only after the current generation resolves; on error render the error in place of the switch.index.css:3336-3351—var(--wk-bg-surface),var(--wk-border-subtle),var(--wk-text-secondary),var(--wk-color-accent),var(--wk-color-error),var(--wk-sp-*)/var(--wk-r-*); drop the!important.ReviewQueue.tsx:255,267— capturespaceGenerationRef.currentnext to eachPromise.alland check it in the.finally.- Rebase to resolve
chat-baseline.ts, and state the intended landing order relative to #1614. - Drop
isSpaceOwnerRole(and its barrel export) until something consumes it. Main/index.tsx:259— useisSpaceAdminOrOwner.- Consider splitting the frozen-relations rendering and the e2e stabilisations into their own PRs.
Add one regression for the failed-policy-load path — that the switch is not rendered with a fabricated value — since that is the acceptance criterion currently missed.
5. Coverage gaps in this review
Stated so they are not read as "checked and clean":
- octo-marketplace was not inspected. Unverified: the
/plugin_review_policiesGET/PATCH contract; whether admins (not only owners) may PATCH — if the server is owner-only, admins get a 403 behind a visible switch, andpolicyDescription("组织所有者和管理员均可修改") would be wrong; the documented "on when no override exists" default; and whetherfrozen_relationsis[]or absent for relation-less plugin types. If it is[],ReviewDetailDrawer.tsx:239renders a "无关联插件" section on every skill review. - Whether the server binds a review id to the request's Space header. This decides whether a cross-Space action continuation 403s or silently succeeds. Not answerable from this repo.
- Suites not re-run here. The PR reports all package suites,
pnpm buildandpnpm i18n:checkpassing; I did not reproduce that, and CI on this head reports onlylabelandpr-title-lintgreen with no test job. Worth confirming the encoding change against thedmworkbasesuite in particular, since it touches shared Space components. - The role-encoding unit tests are self-consistent, not contract-proving.
useSpaceRole.test.ts,spaceRoleBadge.test.tsand therole: 1 → 2edits acrosschat-baseline.ts/sp1/sp2/rb3mocks were all flipped together with the code, so they pin intent but cannot detect the encoding being wrong. The octo-server references in §2 are what actually establish it. A wire-contract test against a recordedspace/mypayload would close this. fetchAllSpaceMembers(Contacts/index.tsx:362-373) has no abort or generation check inside its paging loop, so a discarded Space's fetch runs to completion. In practicePAGE_SIZE = 10000with an earlybreakmeans one request for any Space under 10k members, and the function is untouched by this PR — noting it as pre-existing, not a finding against this diff.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Re-review — PR #1624 (octo-web), Round 3
Reviewed at head 67837145 against merge-base bff055a. This is a re-review of my Round-2 APPROVE (which the subsequent push dismissed); I re-verified the full delta independently at this head.
🔴 Blocking
🔴 1. Policy panel renders a policy value it never fetched — and the switch goes live on it
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx
enabledis initialised totrue(line 38) andloadPolicy()(lines 48–66) never resets it: it only setsloading/saving/errorbefore firing the GET.- On a successful GET the value is written (generation-guarded — that part is sound). On a failed GET the catch stores the error and the finally clears
loading— so the checkbox becomes interactive while still showing a value that was never read for this Space. - Two reachable states verified byte-by-byte:
- Space A → B switch: A's value keeps rendering while B's GET is in flight (disabled during the window, so cosmetic only) — but if B's GET rejects, the toggle is live on A's value in B's panel.
- First mount with a failing GET: the panel asserts "自动审核 已开启" — the weakest posture for a control that decides whether org publications get human review — purely from the
useStatedefault, with the error line beside it.
- This violates the PR's own acceptance criterion "Space switches cannot display stale member/role/policy state": the policy panel is the one surface in this PR that does not clear its value synchronously on
space-changed(contrastAllAssetsList.tsxlines 83–90 andMcpMarketListPage.tsxlines 255–281, which do). - I own this one: the defect was already present at the Round-2 head and my APPROVE there missed it. Credit to yujiwei's review on this head (id 5110997382) for raising it; I re-verified it independently against the source.
Fix: carry an unresolved state (boolean | undefined, or a {spaceId, value} pair), render the switch only once the current generation has resolved, and on error render the error in place of the switch. Add a regression for the failed-load path asserting the switch is not rendered with a fabricated value.
🔴 2. New policy-panel CSS resolves against undefined tokens, hard-codes light values, and uses !important — dark theme is broken for this panel
packages/dmworkskillmarket/src/index.css lines 3336–3351 (new in this PR):
border: 1px solid var(--wk-border-color, #e5e7eb)andbackground: var(--wk-bg-color, #fff):--wk-border-colorand--wk-bg-colorare defined nowhere in the theme files (packages/dmworkbase/src/theme/semantic.cssetc. — verified by repo-wide search), so bothvar()calls always resolve to their literal light fallbacks.- The panel is therefore a hard
#fffcard with#6b7280text and a#2563ebcheckbox in both themes. Dark mode is a shipped, persisted feature (packages/dmworkbase/src/App.tsxtogglesbody[theme-mode="dark"];semantic.cssre-tints--wk-bg-surfaceto--wk-neutral-800under it), and every other rule this same PR adds deliberately relies on semantic tokens for exactly that reason (e.g. the.wk-plugin-statusblock documents it). .skill-market-review-policy__error { color: #dc2626 !important; }violates the explicit!importantban inDEVELOPMENT.md§13 (CSS 编写禁止事项);var(--wk-color-error)needs no specificity boost.- The adjacent
.skill-market-review-policy-confirmblock in the same hunk does all of this correctly withvar(--wk-color-error)/var(--wk-text-secondary)/var(--wk-sp-*)— so this is an inconsistency within one commit, and the fix is mechanical:var(--wk-bg-surface),var(--wk-border-subtle),var(--wk-text-secondary),var(--wk-color-accent)(already used for this exact purpose elsewhere in this file),var(--wk-color-error), and drop the!important.
Merge state — must be resolved (not a code defect)
- Live status:
mergeable_state = dirty(CONFLICTING) at this head. The conflict is inapps/web/e2e-kit/msw-handlers/chat-baseline.tsagainst currentmain; please rebase. - Stacking: this branch targets
mainwhile containing #1614's commits — #1614 is still OPEN and its head7d478bais a direct ancestor of this head (verified). Whichever merges first empties the other; please state the landing order explicitly (or re-base onto #1614's branch) so #1614 is closed rather than left to merge into a no-op.
💬 Non-blocking
- 🟡 Multi-step save/publish continuations are not Space-bound (raised by the second-opinion pass as blocking; I verified it independently and rate it 🟡 here). The request interceptors resolve the Space header per request (
packages/dmworkskillmarket/src/api/skillApiReal.tslines 84–89;packages/dmworkmcp/src/api/mcpService.tslines 428–430), and the multi-step chains do not capture the originating Space:ExpertEditModal.tsx's close is unguarded while saving (onCancel={onClose}, with mask/Esc close enabled by default), andEditSkillModal.tsx/NewSkillModal.tsx's leave-confirm closes without aborting the submit chain (itsabortRefonly covers the upload/parse flow). A user who closes one of these modals right after clicking 发布 and switches Space leaves the continuation firing the second step under the new Space's header.McpCreateModal.tsxis already guarded (its close handler returns while submitting), which is the pattern to copy. Impact is bounded — records are Space-scoped server-side (the mock contract documents the real endpoint 404-ing on another Space's record), so the continuation fails rather than writes across — but the failure is silent (the error state renders on an already-closed modal; no toast), leaving a draft saved with the publish lost. Recommend capturing the originating Space at submit start and dropping the continuation onspace-changed, consistent with the generation guards this PR already uses elsewhere. Whether the server binds the record to the request's Space header in every path is not answerable from this repo — another reason to make the client side robust. - 🟡
ReviewQueue.tsxlines 255–256 and ~267: the twoPromise.all(...).finally(...)wrappers for the handled list clearhandledLoading/handledLoadingMorewithout the generation check their innerfetchHandledPagecalls do have — on a Space switch the stale finally can clear the flag while the replacement fetches are in flight (brief empty flash). Capture the generation beside eachPromise.all. - 🟡
apps/web/src/Pages/Main/index.tsxline 259 still readss.role === 1 || s.role === 2— behaviour-neutral under both encodings, but the last un-migrated literal; route it through the shared predicate. - 🟡
SpaceReviewPage.tsxline 70 loads the policy unconditionally while the panel is gated onisReviewer; a deep-linked member fires a guaranteed-403 GET whose error is stored but never rendered. Gate the load on the resolved role. - 🟡 Frozen relations render raw identifiers (
ReviewDetailDrawer.tsxlines 246–252); a reviewer cannot tell which expert is being added from a plugin id. Resolving names would make the section decision-useful. - 🟡 The voice e2e stabilisation (
settings-center-voice-support.tslines 64–71) wraps the whole seed-and-open sequence in a retry that discards the first failure's reason — the ordering race it documents is real, but the blanket retry can also absorb a genuine regression; narrowing it to the snapshot-ordering step would be safer. - 🟡
isSpaceOwnerRole(useSpaceRole.ts) still has zero production consumers (carried from Round 2); drop it or land it with its consumer.
Verified on this head (so these are not re-flagged)
- Round-3 delta is test-only.
3f2244f5..67837145touches exactly three files: the new guard regression suite, one MSW baseline handler addition, and the voice e2e retry wrapper. Zero production-code bytes changed (file list + diff verified). - The new guard regression test is real load, not an existence check.
packages/dmworkmcp/src/pages/__tests__/McpMarketListPage.spaceIsolation.test.tsxdrives the actual timing: starthandleEditFromCard/openPublishVersion, switch Space viahandleSpaceChanged_(whose first synchronous statement bumpsrequestVersion), then resolve/reject the in-flight detail fetch and assert no modal opens and no toast fires — both entry points, both success and error paths. I re-read the production guards at this head (McpMarketListPage.tsxlines 255–281, 522–533, 574–585) and they match. Minor observation: there is no positive control (no-switch → modal opens) anywhere in the suite, so an inverted guard would still pass these four tests; worth adding one. - MineTable's nine-column rework is internally consistent: header renders from a nine-entry
COLUMNS, rows render nine cells with matching modifier classes, and each responsive breakpoint hides the same classes its shrunken grid template drops. The removed--nostatsrule has zero remaining consumers. - Local suites green at this head:
packages/dmworkmcp283/283 (baseline 279 + the 4 new isolation tests),packages/dmworkskillmarket255/255. CI on this head runs only label/title checks (no build/test job); thecode-reviewcommit status is red pending a current approval. - The role-encoding fix (
SpaceService.tsxlines 233–235) is corroborated by yujiwei's audit against the octo-server role model, and the rest of the migration is complete except the one neutral literal above.
Verdict
REQUEST_CHANGES — the policy panel's unfetched-value state and the dark-theme/!important token regression are both small, mechanical fixes on this PR's own flagship surface, but they block until done; the rebase and the #1614 landing order need an explicit decision. The core direction — generation guards, server-authoritative authorization, frozen relations — is sound, and the Round-3 test additions genuinely cover the Round-1 race.
…auto-approve # Conflicts: # apps/web/e2e-kit/msw-handlers/chat-baseline.ts
|
Addressed the latest requested changes in
Stacking plan: #1614 should merge first because its head is a direct ancestor of this PR. After #1614 lands, this PR’s effective diff against main will automatically shrink to the review-policy follow-up; #1624 should merge second. No force-push or history rewrite is needed. Verified so far: skillmarket 255 tests, MCP 283 tests, i18n check, production build, focused TES13 + S26 e2e, and diff whitespace checks. Additional base/contacts verification is running. |
|
CI update for head |
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Re-review — PR #1624 (octo-web), Round 4
Reviewed at head 92ee6992 (fix commit 491b7ee8 + merge of upstream/main ca716168). Both Round-3 blockers are verified fixed at byte level; the merge is clean; CI is fully green; local suites at baseline.
🔴 1 (Round 3): policy panel CSS — FIXED
packages/dmworkskillmarket/src/index.css, the .skill-market-review-policy* blocks:
var(--wk-border-color, #e5e7eb)/var(--wk-bg-color, #fff)→var(--wk-border-subtle)/var(--wk-bg-surface)— both defined inpackages/dmworkbase/src/theme/semantic.csswith dark-theme overrides.#6b7280→var(--wk-text-secondary);#dc2626 !important→var(--wk-color-error)(no!important);#2563eb→var(--wk-brand-primary); gap/margin/padding/radius now--wk-sp-*/--wk-r-md.- I checked every
var()in this file's PR diff against the theme: all resolve to tokens defined insemantic.css; no hardcoded light literals and no!importantin the added lines.
🔴 2 (Round 3): enabled toggle fail-closed posture — FIXED, with tests
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx:
enabledis nowboolean | undefined, initialisedundefined;loadPolicy()resets it toundefinedbefore every fetch;space-changedbumps the generation and resets value/loading/error before reloading (reloads only for payload role 1–2; role 0 clears loading with the value left undefined); the non-reviewer path clears everything.- The toggle renders only when
enabled !== undefined && !error. A failed GET shows the error text and renders no switch at all — there is no path left that renders an unfetched value as 已开启. Saves are not optimistic:savePolicywrites back the server-echoed value only. - New regression tests: "does not render an unfetched policy value when loading fails", and members no longer trigger policy reads ("avoids policy reads for ordinary members" asserts
getPolicyis never called). The existing space-switch generation tests still cover the stale-response races.
Round-3 findings, current status
- yujiwei P2 (handled-list finalizers skipping the generation check,
ReviewQueue.tsx) — FIXED in491b7ee8: both.finallysites now capture and comparespaceGenerationRef.current. - yujiwei P2 (un-migrated role literal
Main/index.tsx:259) — FIXED: routed throughisSpaceAdminOrOwner, which is exported from@octo/basevia the barrel (export * from "./Service/SpaceService"). - yujiwei P2 (policy read issued for deep-linked members) — FIXED: the load is gated on the resolved role.
- yujiwei P2 (blanket voice-e2e retry) — ADDRESSED: the retry is narrowed to
TimeoutError; other failures rethrow immediately. isSpaceOwnerRole— REMOVED from the hook, the barrel export, and its test; repo-wide grep shows zero remaining references.- Conflict against main (
chat-baseline.ts) — RESOLVED; PR is MERGEABLE again. The combined merge diff is tiny:handlers.tskeeps both main's MSW probe handler and this PR's review-badge handlers, and the chat-baseline resolution keeps main's contact-bot mocks alongside this branch'srole: 2+/api/v1variants. Main-side changes (#1596 red-dot, #1619 communication, #1612/#1616) are all present in the merge result — nothing swallowed.
Second-opinion 🔴 claim — independently adjudicated non-blocking
The second-opinion pass flagged packages/dmworkskillmarket/src/components/ReviewDetailDrawer.tsx: when reviewId changes or a retry starts, the previous review stays in state while canAct ignores loading, so Approve could act on the stale request. The mechanism is real as written (the load effect does not clear review), but it is unreachable in the current wiring:
- The drawer is a WKModal (Semi Modal, mask on by default); the only writer that sets a non-null
reviewIdis a row'sonOpen, which cannot be clicked through the mask while the drawer is open. - Every other transition passes through
reviewId=null(onClose, orrefreshAllclearingdetailId), and the null path clearsreviewbefore any next open. - Retry reloads the same
reviewId; the footer always acts on the request shown in the header, so even on the error path the button approves exactly what is displayed.
Clearingreviewwhen a load starts and/or adding!loadingtocanActis still worth doing as defense-in-depth with a slow-B-after-A regression test — noted as P2, not blocking.
P2 notes (non-blocking, carried)
- Frozen relations still render raw identifiers (
ReviewDetailDrawer.tsx); resolving names would make the section decision-useful (yujiwei's point, unchanged). .skill-market-review-noticeadds one newvar(--wk-color-primary)use vs main (index.css) — that token is defined nowhere in the theme, so the declaration is invalid and the accent border silently drops. Cosmetic only;var(--wk-color-accent)is the house equivalent. (The other three--wk-color-primaryuses in the mine-table already exist on main via #1584.)- The guard regression suite still has no no-switch control case — an inverted guard would still pass the four race tests.
- Multi-step save/publish chains (
ExpertEditModal/EditSkillModal/NewSkillModal) remain unbound to the originating Space; server-side scoping means a stale continuation fails rather than cross-writes, but the failure is silent.McpCreateModal's close guard remains the pattern to copy. - Process: this PR still carries #1614 (open at
7d478ba, an ancestor of this branch); whichever lands first no-ops the other — please state the landing order when merging.
Verified on this head
- Local suites:
packages/dmworkmcp283/283 andpackages/dmworkskillmarket255/255 (the latter needs Node's--localstorage-fileflag in this environment because Node 22.23 does not expose the localStorage global by default — environment quirk, not a code regression; CI's Unit tests are green). - CI on
92ee6992: Build, Unit tests, e2e-p0, install-build, secret-scan, dependency-review — all green.
Verdict
APPROVE — both Round-3 blockers are fixed with regression coverage; the merge introduced no new blockers; the residual items are P2.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1624 (octo-web)
Reviewed at head 92ee6992 (a merge of 491b7ee8 with main@ca716168), against merge-base ca716168. Round 4.
Scoping: main's tip is the merge-base, so ca716168..HEAD is exactly this PR's own change (90 files, +12829/−792). Round 3 reviewed 6783714; the only new work here is 491b7ee8 "fix: address review policy feedback" plus the merge. I verified each round-3 item on this head and then re-reviewed the flagship surface (SpaceReviewPage, useSpaceRole, ReviewQueue, ReviewDetailDrawer) independently, because that is where the fix landed.
1. Spec compliance
Spec: ❌
Checked against #1623's Scope and Acceptance lists.
Missing — none. The acceptance criterion that failed in round 3 is now met: "Space switches cannot display stale member/role/policy state." SpaceReviewPage.tsx:38 now holds boolean | undefined, loadPolicy() resets it to undefined at :51, and both the space-changed handler (:71-81) and the role effect (:89-101) clear it before any new read. The member/role side is unchanged and was already correct.
Out of scope (超建) — carried from round 3, unaddressed:
- Frozen-relation rendering (
api/pluginWire.ts:194,types/skill.ts,ReviewDetailDrawer.tsx:239-262,RejectReasonModal-adjacent CSS). This is a new user-visible reviewer feature. #1623's scope is "policy API mapping and review-management UI", "role behavior aligned with the authoritative role encoding", "prevent stale responses from crossing Space switches", "deterministic tests and E2E mocks". None of those covers rendering an approval-time relation graph. It may well be worth shipping — it just is not this issue. - Unrelated e2e flake fix (
apps/web/e2e-kit/tests/settings-center/voice/settings-center-voice-support.ts:52-72). The retry is now correctly narrowed (see §2), but the voice settings flow has nothing to do with the review policy. This is the kind of change that makes an already-large PR hard to land.
The contacts role badge, the MCP all-assets isolation and the chat-baseline.ts MSW additions do fall under "role behavior aligned with the authoritative encoding" / "prevent stale responses" / "E2E mocks", so I am not counting those against scope.
Divergence (偏离) — none material.
To be clear about weight: scope creep alone would not be my blocker at round 4. The two P1s below are.
2. Code quality
Quality: Changes-Requested
First — the round-3 fixes I verified as correct, so nobody "fixes" them back
- CSS tokens.
index.css:3336-3369now uses--wk-bg-surface,--wk-border-subtle,--wk-text-secondary,--wk-color-error,--wk-sp-{1,2,4,5,6},--wk-r-md,--wk-brand-primary, and the!importantis gone. I confirmed every one of those is actually defined inpackages/dmworkbase/src/theme/semantic.css(:290/:465for--wk-bg-surface,:323/:524for--wk-border-subtle,:313/:516,:334/:535,:14/:393,:239-253) — including thebody[theme-mode=dark]overrides, so the panel now re-tints. The previous--wk-bg-color/--wk-border-colorwere undefined; this is a real fix, not a rename. - Handled-list generation guards.
ReviewQueue.tsx:255,265capturespaceGenerationRef.currentbeside eachPromise.alland check it in the.finally. Correct. isSpaceOwnerRoleremoved fromuseSpaceRole.tsand from the barrel atindex.tsx:19. Its test was dropped with it.Main/index.tsx:259now routes throughisSpaceAdminOrOwner(s.role), preserving the priorrole === 1 || role === 2semantics.- Voice e2e retry (
settings-center-voice-support.ts:60-68) now rethrows anything that is not aTimeoutErrorand retries only the conversation-visibility wait, not the whole seed-and-open sequence. This is the right narrowing. - Merge resolution.
92ee6992's conflict resolution inapps/web/e2e-kit/msw-handlers/chat-baseline.tskeeps both sides:main's newrobot/my_bots,robot/space_bots, populatedgroup/myandspace/:spaceId/membersfixtures, and this branch'srole: 1 → 2owner fixtures (:57,:70) and*/api/v1/conversations/.../extrahandlers. Nothing frommainwas clobbered.mergeableis nowMERGEABLE, and CIBuildis green on this head. - Member deep-link no longer fires a guaranteed-403 policy GET —
SpaceReviewPage.tsx:89-101gatesloadPolicy()on the resolved role, andSpaceReviewPage.test.tsx:124-130assertsgetPolicyis never called forrole=0.
P1 — A failed policy save permanently removes the policy control
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx:113-116 and :154
} catch (err) {
if (requestGeneration !== requestGenerationRef.current) return false;
setError(err instanceof Error ? err.message : t("skillMarket.review.policySaveFailed"));
return false;
}
...
{enabled !== undefined && !error && (
<label className="skill-market-review-policy__toggle">The round-3 fix was "on error render the error in place of the switch" — but !error was applied to every error, not just a load error. savePolicy leaves enabled at its correctly-loaded value and sets error, so the render guard now hides the switch after a save failure. There is no retry affordance in the section — recovery requires a full remount or a Space switch.
Two reachable paths, both verified by running them against this head:
- Failed enable. Click the switch on →
handlePolicyChange(true)→savePolicy(true)rejects →enabled=false, error≠null→ the checkbox is gone. - Failed disable, then cancel. Click off → confirm modal →
confirmDisable()→savePolicy(false)rejects → error renders inside the modal (correct), user cancels the modal →enabled=true, error≠null→ the checkbox is gone from the panel behind it.
I wrote two throwaway probes against the real component (mocking only skillApi and useSpaceRole, same harness as the existing suite) and both assert-not-null checks failed:
× PROBE A: a failed enable-save must not remove the toggle
AssertionError: expected null not to be null (screen.queryByRole("checkbox"))
× PROBE B: a failed disable-save then cancel must not remove the toggle
AssertionError: expected null not to be null
This matters beyond a transient 500: whether admins (not only owners) may PATCH /plugin_review_policies is still unverified server-side (see §5). If the endpoint is owner-only, every admin who touches the switch gets a 403 and then watches the control vanish — the panel still says "组织所有者和管理员均可修改" while offering nothing.
Fix: track the load failure separately from the save failure (e.g. loadError vs saveError), gate the switch on enabled !== undefined && !loadError, and render saveError next to a still-interactive switch. The existing suite has no save-failure case at all — SpaceReviewPage.test.tsx:41 only ever resolves updatePolicy — so please add one in both directions.
P1 — ReviewDetailDrawer leaks its action state across reviewId, auto-opening the reject dialog on a different review
packages/dmworkskillmarket/src/components/ReviewDetailDrawer.tsx:42-48, :36, :277-281
useEffect(() => {
if (!reviewId) {
setReview(null);
setError(null);
setIconError(false);
return;
}rejectOpen (:36) and acting (:35) are not reset here. The drawer is permanently mounted — ReviewQueue.tsx:586 renders it with reviewId={detailId} — and ReviewQueue.refreshAll() (:299-304), which handleSpaceChanged (:309-326) calls, closes it by setDetailId(null). So the queue's own generation guards, which are otherwise thorough, stop at the child.
Concrete sequence, verified by running it:
- Space A, open review A's detail, click 拒绝 →
setRejectOpen(true), the reason dialog is up for A. - Switch Space →
refreshAll()→setDetailId(null)→reviewbecomesnull, so{review && <RejectReasonModal …>}unmounts and the dialog visually disappears.rejectOpenstaystrue. - In Space B, open any review's detail →
reviewresolves to B →RejectReasonModalremounts withvisible={rejectOpen}stilltrue.
DIALOG COUNT: 2 REJECT-MODAL-VISIBLE: true
× PROBE C: rejectOpen must not survive a drawer close and re-open on another review
AssertionError: expected true to be false (/拒绝「Skill B」/ present in the DOM)
The dialog that pops up is titled with B's plugin name and its confirm calls rejectReview(review.id, reason) against B's id. A reviewer who was interrupted mid-reject on A can type a reason into an unsolicited dialog and reject a request they never chose. It is a destructive decision on the wrong record, in exactly the Space-switch scenario this PR exists to harden.
RejectReasonModal.tsx:28-38 shows the author already reasoned about this hazard — "this instance survives a change of review … one confirm would write the previous plugin's reason here" — and correctly clears reason/error when visible goes false. The same reasoning needed to reach the parent's rejectOpen.
acting has the same leak, with a milder symptom: an approve issued in Space A that is still in flight leaves acting=true, so the newly opened detail in Space B renders its 通过/拒绝 footer disabled (:148, :155) until A's request settles; and if A's approve then resolves, onDecided(); onClose() (:75-76) closes the drawer the user just opened in B, while a rejection writes A's message into B's error.
Fix: reset rejectOpen, acting and loading in the !reviewId branch (and on any reviewId change), and drop the approve/reject continuations when the reviewId they started under is no longer current — the same generation pattern ReviewQueue already uses. Worth a regression: close the drawer mid-reject, reopen on another id, assert no reject dialog.
P2 — c.spaceRole = m.role is a type error on Contacts
packages/dmworkcontacts/src/Contacts/index.tsx:507 and :522
const c = new Contacts()
...
c.spaceRole = m.rolec is typed Contacts (imported from @octo/base at :3, i.e. the class at packages/dmworkbase/src/Service/DataSource/DataSource.ts:69), which has no spaceRole and no index signature. spaceRole lives only on the local SpaceContact interface (:68-70). Annotating the array (items: SpaceContact[], :478; memberItems: SpaceContact[], :513) does not retype c. Reduced to a minimal case and run through this repo's own tsc:
a.ts(8,5): error TS2339: Property 'spaceRole' does not exist on type 'Contacts'.
This does not break CI — pnpm build is vite build (esbuild, transpile-only), no package here runs tsc --noEmit, and Build is green on this head — which is precisely why it is worth fixing now rather than discovering it the day someone adds a typecheck job. Note the change replaced a previously valid (c as any)._spaceRole = m.role, so the cleanup introduced the error. const c = new Contacts() as SpaceContact (or building a plain SpaceContact literal) resolves it.
I'll flag the corollary, not as a finding: nothing in CI type-checks these packages, so no reviewer and no gate can catch this class of defect. That is worth its own issue.
P2 — The role encoding is re-inlined in the very file that centralises it
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx:79
if (typeof role === "number" && role >= 1 && role <= 2) loadPolicy();isSpaceReviewerRole is exported from ../hooks/useSpaceRole (:12-18) and is already imported into this file's dependency graph; it is literally role >= SPACE_ROLE_ADMIN && role <= SPACE_ROLE_OWNER. Re-spelling it as bare integers here re-creates the drift this PR is fixing everywhere else.
Related fragility in the same handler: when the payload's role is not a number, neither branch at :79-80 runs, so loading stays true and enabled stays undefined. It recovers only because useSpaceRole.resolveFromServer() (useSpaceRole.ts:55-81) flips its own loading false→true→false, and roleLoading is in the :101 dependency array. That is a real but entirely implicit coupling between two hooks. An explicit else { loadPolicy() } — or dropping the payload fast-path here and letting the role effect own it — removes the need for the reader to discover it.
P2 — Dead disabled condition
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx:159: disabled={loading || saving}. Since loadPolicy sets enabled=undefined whenever it sets loading=true, and the switch only renders when enabled !== undefined, loading can no longer be true at this point. Harmless, but it now reads as protection that isn't doing anything.
P2 — Hardcoded spacing in the new confirm block
packages/dmworkskillmarket/src/index.css:3371-3388: .skill-market-review-policy-confirm { padding: 2px 0 4px } and .skill-market-review-policy-confirm__icon { width: 40px; height: 40px; border-radius: 12px } (:3378-3388). The colour tokens in this block were always right; the spacing and radius still bypass --wk-sp-* / --wk-r-* that the sibling rule now uses.
P2 — Carried from round 3, still open
- Multi-step publish continuations are not Space-bound.
packages/dmworkmcp/src/components/ExpertEditModal.tsx:156is stillonCancel={onClose}with no in-flight guard, andEditSkillModal/NewSkillModal's leave-confirm still closes without aborting the submit chain.McpCreateModalis guarded and is the pattern to copy. Bounded impact (records are Space-scoped server-side, so the continuation should fail rather than write across) but the failure is silent. - Frozen relations render raw identifiers.
ReviewDetailDrawer.tsx:249-252printstargetPluginId/targetPluginType/relationTypeverbatim. The stated goal is "so reviewers see the graph being approved"; a reviewer cannot tell which expert is being added from an opaque id. Escaping is fine — the<pre>{JSON.stringify(...)}</pre>at:255is React-escaped. - #1614 landing order. #1614 is still
OPENand its head7d478bacis a direct ancestor of this head (verified withgit merge-base --is-ancestor). Merging this empties that. The merge withmainresolved the conflict, not the stacking. Please state the intended order so #1614 is closed rather than left to merge into a no-op.
Findings I checked and am NOT raising
Stated so they are not re-litigated:
SpaceMembers/index.tsx:156-180— thewk-spacemembers-item-actionswrapper does not render empty for admins. The 移除 button at:174-179is unconditional inside it; only the two role-change buttons gained the owner-onlycanManageRolesgate. Restricting role changes to owners matches octo-server (api_member_role_test.godeniesrole=1), so this is correct as written.expertService.ts:632—expertListErrorI18nKeyis declared(err: unknown)atexpertListError.ts:35. No cast needed; passing a barecatchbinding is fine.reviewSignal.ts—withReviewInvalidationbumping infinally(including on refusal) is deliberate and cannot loop: listeners only issue reads, and reads are not wrapped. Iterating a copy of the set and swallowing listener throws are both correct.useReviewRequests.ts:52-139— aborting before the!enabledearly return (:59-70), sharing the page controller with the badge probe (:113), and thecontroller.signal.abortedguard on every write are all correct.frozenRelationsis mapped from an optionalfrozen_relations?(pluginWire.ts:194), so an absent field yieldsundefinedandReviewDetailDrawer.tsx:239renders nothing. The "empty relations section on every skill review" risk only materialises if the server sends[]— see §5.
3. Overall verdict
REQUEST_CHANGES
Spec ❌ (out-of-scope additions) AND Quality Changes-Requested. Both P1s are small and mechanical, and both are in the code this round added or touched: the render guard that fixed the unfetched-value bug over-reaches onto save errors, and the drawer is the one component the Space-switch hardening skipped. Everything round 3 asked for is otherwise done and done properly.
4. Suggested changes
SpaceReviewPage.tsx— splitloadErrorfromsaveError; gate the switch onenabled !== undefined && !loadError; rendersaveErrorbeside a live switch. Add tests for a failed enable and a failed-then-cancelled disable.ReviewDetailDrawer.tsx:42-48— resetrejectOpen,actingandloadingwhenreviewIdchanges or clears; drop approve/reject continuations whose originatingreviewIdis stale. Add a regression that reopens the drawer on a different id and asserts no reject dialog.Contacts/index.tsx:507,522—const c = new Contacts() as SpaceContact.SpaceReviewPage.tsx:79— useisSpaceReviewerRole(role); make the non-numeric-role branch explicit instead of relying onuseSpaceRole'sloadingtoggle.index.css:3371-3388—--wk-sp-*/--wk-r-*for the confirm block's padding and radius.- State the landing order relative to #1614.
- Consider splitting the frozen-relation rendering and the voice e2e fix into their own PRs. Not a merge blocker; it is what would have made rounds 2–4 cheaper.
Separately, worth its own issue: no CI job type-checks these packages, so finding 3 was invisible to every gate on this PR.
5. Round count — please escalate rather than iterate
This is round 4 (8f3f4e2b → 666c20b6 → 6783714 ×2 → 92ee6992). Both P1s here are consequences of round-3 fixes rather than original defects, which is the signature of whack-a-mole rather than convergence. Before round 5, I'd suggest the author and whoever is shepherding this agree on scope out loud: land the policy panel and the role-encoding fix as one small PR, and move the frozen-relation feature, the MCP all-assets isolation and the e2e stabilisations to follow-ups. A 90-file, +12.8k-line diff is why a two-line render guard took three rounds to get right.
6. Coverage gaps in this review
Stated so they are not read as "checked and clean":
- octo-marketplace was not inspected. Still unverified: the
/plugin_review_policiesGET/PATCH contract; whether admins may PATCH (this decides how often finding 1 fires in practice, and whetherpolicyDescription's "组织所有者和管理员均可修改" is accurate); the documented "auto-approve on when no override exists" default; and whetherfrozen_relationsis[]or absent for relation-less plugin types. - Whether the server binds a review/plugin id to the request's Space header in every mutation path. This decides whether the stale continuations above 403 or silently succeed. Not answerable from this repo.
Unit testsande2e-p0were stillpendingon this head when I reviewed;Buildand the scanners are green. I ran only the twodmworkskillmarketsuites I needed for the probes above; I did not reproduce the full matrix the PR description reports.- Bulk of the diff not re-read.
MineTable's column rework,NewSkillModal(+903),McpCreateModal(+433),skillApiReal/skillApiMock,AllAssetsList,MarketSidebarand theexpertServicevisibility work were verified in rounds 1–3 and are unchanged here; I re-read them only where a finding pointed. If you want a fresh pass on those, say so. - No positive control in the isolation suites. Noted in round 3 and still true:
McpMarketListPage.spaceIsolation.test.tsxasserts only that guards suppress; an inverted guard would still pass. Same now applies to whatever regression covers finding 2. - Dark mode was verified by token existence, not by rendering. I confirmed each
var()resolves to a token defined under both:rootandbody[theme-mode=dark]; I did not open the panel in a browser in either theme.
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Code Review — PR #1624 (octo-web)
Reviewer: Octo-Q (automated review)
Head SHA: 92ee6992f68610e16d301ca0f88a21ad95fbe9de | Base: main (merge-base ca716168) | 90 files, +12829/−792 | 首轮审查
Summary
本 PR 把「组织发布管理（Space review）」完整落到前端：技能/连接器/专家/专家团四个市场统一走 /plugins/review_requests + 单一 /plugins/publish 发布门（后端按 visibility 决定直接上架还是开审核），新增审核队列页（SpaceReviewPage + ReviewQueue）、侧边栏待审徽标（失效信号挂在 mutation 端点上）、自动批准策略开关（/plugin_review_policies）、全部资产页与统一的 display_status 渲染契约；同时把前端 space 角色编码整体校正为 octo-server 的 0/1/2 编码。整体工程质量很高：竞态（generation/abort）、空间切换重载、fail-closed 枚举收窄都有成体系的防护和配套 e2e（RB1/RB2/RB3）。未发现 P0/P1；5 个 P2 如下。
Verification
Static analysis only at head 92ee6992; build and tests not executed in this environment.
- ✅ Space 角色编码与后端一致 — 对照 octo-server 源码核实：
modules/space/db_manager.go转让 owner 时role=2 → role=1（owner=2, admin=1），modules/space/db.goqueryAdminsAndOwner用role>=1。新常量（SpaceService.tsx:233-241）及全部消费点（SpaceMembers/SpaceSettings/MainPage/Contacts badge/各 MSW mock）均为 0=member/1=admin/2=owner。旧注释1:owner,2:admin,3:member才是错误的一方——本 PR 是修复，不是引入（旧代码下 member(role=0) 会穿过role<=2的 admin 判断、成员管理按钮对真实编码全错位）。 - ✅ 审核读取的空间隔离 —
skillApiReal.ts:78-89getAuthHeaders()请求时读currentSpaceId挂X-Space-Id；Main 页先同步更新currentSpaceId再 emitspace-changed；四个市场页 + AllAssetsList + ReviewQueue + SpaceReviewPage + 侧边栏 probe 全部订阅 space-changed 并带 generation 守卫；RB3 e2e 钉住「双空间 1→3」场景。 - ✅ 失效信号架构 —
withReviewInvalidation挂在 endpoint 而非调用点：deleteSkill/deleteMcp/deleteExpert/deleteSquad/publishPlugin/delistPlugin/createReviewRequest/approveReview/rejectReview/cancelReview全覆盖；拒绝（409）也触发失效（队列已动的最强证据）；dmworkmcp 通过深导入@dmwork/skillmarket/src/api/reviewSignal保证同一模块实例（包无exports字段，深导入可解析）。 - ✅ facade mock/real 对偶 —
skillApi.ts引用的 19 个名字在skillApiMock与skillApiReal均存在（逐一 grep 核实），VITE_USE_MOCK下不会解析到undefined。 - ✅ XSS — 冻结 readme（不可信申请人内容）经
ReactMarkdown + rehypeSanitize（ReviewDetailDrawer.tsx）；relation.data 经JSON.stringify文本渲染；其余用户字符串均为文本节点。 - ✅ 版本规则与后端镜像 —
utils/version.ts格式门 + 数值化前向比较 + stored-label grandfathering，且刻意区分保存面（带stored）与审核提交面（不带，镜像SubmitReview无豁免）；四个模态框调用点均正确。 - ✅ i18n — 变更文件引用的 411 个
t()key 在 en-US/zh-CN 两语言均存在（脚本核对，0 缺失）。 - ✅ 授权门均为装饰性 —
useSpaceRolefail-closed、注释与实现一致；成员深链/mcp-market/review得到队列自身 403 错误态 + 侧边栏角色解析后移走；ExpertEditModal对 system/public 行只读、不提供任何动作。
Findings
无 P0/P1；5 个 P2。
P2 — McpMarketListPage 的 ReviewSubmitModal 路径是死代码 (packages/dmworkmcp/src/pages/McpMarketListPage.tsx:554)
diff-scope：本 PR 新引入。openReviewSubmit 是 reviewTarget 的唯一 setter，但全文件（含 detail 弹窗回调）无任何调用点：连接器升级走 openPublishVersion（McpCreateModal review 模式），首发/重新提交走 handlePublish→publishPluginListing。挂载的 <ReviewSubmitModal target={this.state.reviewTarget}>（:1129）因此永远不会打开。后果：两套竞争性提交 UI 并存，后续维护者可能把需要 changelog 的重新提交流程接到这个不带内容的模态框上。建议：要么按 SkillListPage 的方式把「被拒后重新提交（携带 initialChangelog）」接到它，要么连方法+模态框+reviewTarget 状态一起删除。
P2 — 默认自动批准策略下，专家/专家团提交成功 toast 误导 (packages/dmworkmcp/src/components/ReviewSubmitModal.tsx:221)
diff-scope：本 PR 新引入。submitPluginReview（api/pluginReview.ts）返回 void，把 createReviewRequest 的返回请求丢弃，模态框只能无条件显示「已提交审核」；而默认 Space 策略是自动批准开启（docs/space-review-auto-approve.md「no override exists 时开关为 on」），此时请求已 approved、新版本立即上架。技能侧同场景按 review.status === "approved"（NewSkillModal）/outcome.displayStatus（publish 路径）切换到「已发布」文案。后果：默认配置下专家/专家团升级作者被告知「审核中」而实际已生效（列表重载后状态正确，无持久错误数据，故不升级 P1）。建议：让 submitPluginReview 返回映射后的 ReviewRequest，按 status 分支文案。
P2 — 全部页 50 条静默截断 (packages/dmworkmcp/src/pages/AllAssetsList.tsx:60)
diff-scope：本 PR 新增页面，且已成为「我的发布」默认 tab（MyAssetsPage.initialType 默认 "all"）。getMySkills({ limit: 50 }, { pluginType: "all" }) 单页拉取，无 loadMore/分页/截断提示。拥有 >50 个跨类型插件的 owner 在默认视图静默丢行（含可能需要 发布/取消审核/删除 的行）。分类型 tab 仍可达，故非阻塞。建议：按分类型列表的方式分页，或至少渲染「仅显示前 50 条」提示。
P2 — useMyReviewState 100 条上限丢拒绝理由 (packages/dmworkmcp/src/hooks/useMyReviewState.ts:32)
diff-scope：本 PR 新引入。mode=mine 以 pageSize: 100 单页拉取且不翻页；deriveSkillReviewState 按 pluginId join。历史请求超过 100 条的用户，窗口外被拒行的 reason 查不到（状态徽章本身来自服务端 display_status，仍正确，仅「查看理由」缺失）。建议：分页拉取或至少在注释/文档中标注该上限。
P2 — 策略加载失败后无重试入口 (packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx:62)
diff-scope：本 PR 新引入。getReviewPolicy 失败后 enabled 保持 undefined 且 error 置位，而开关渲染条件是 enabled !== undefined && !error——自动批准策略区域只剩错误文本，除非切换空间或重进路由否则无法重试。保存失败路径可经确认模态框重试，加载失败路径建议补一个重试按钮（或保留禁用态开关 + 重试）。
建议
- P2-1 与 P2-2 建议一并处理：若保留
ReviewSubmitModal，把它接成连接器「被拒重新提交」入口并让submitPluginReview返回请求对象；否则整体删除，避免四个市场出现三套提交语义。 - 全部页与我的审核态两处上限（50/100）要么分页要么显式提示，防止资产规模大的空间静默丢行。
额外发现
NewSkillModal/McpCreateModal的 create-then-publish 重试：若 publish 实际已在服务端成功但客户端观察到失败（回包丢失），重试会对已开审核的行调updateSkill/updateMcp，被服务端 409 挡下并报错——状态无损，但用户会卡住；属于罕见的双故障窗口，不阻塞。buildPluginListParams对pluginType:"all"以「缺省」而非哨兵值表达全类型，与服务端语义一致（已核对注释与getMySkills调用链）。
数据流回溯
space.role←GET /space/my（SpaceService.getMySpaces）与space-changedpayload（Space 对象本身）→isSpaceAdminOrOwner/isSpaceOwner、useSpaceRole、SpaceReviewPage.handleSpaceChanged。已回溯到服务端赋值（octo-serverspace_member.role，0/1/2），运行时数据确实流到全部消费点；发送侧updateMemberRole的 1/0 同编码。display_status/listing_state/review_id←GET /plugins?mode=mine与/plugins/detail（服务端仅对 owner 行/详情计算）→mapSkill/mapListItem/narrow*→resolveReviewRowState→ MineTable 状态列与动作门控。公共目录网格无此字段 →undefined→ 不渲染徽章（安全，未默认成 "draft"）。pendingCount←GET /plugins/review_requests?mode=space&status=pending&page_size=1（带X-Space-Id）→ 侧边栏徽标；enabled: isReviewer门控，成员不触发 403 探测。policy.isAutoApproveEnabled←GET /plugin_review_policies→ SpaceReviewPage 开关；默认值由服务端解析（无 override 时为 on），客户端不猜测。ReviewRequest.*←/plugins/review_requests*行 →mapReviewRequest→ 队列/抽屉；plugin_icon是裸存储键（后端缺陷，代码已注释），<img onError>回退字母头像，运行时不会破图。
盲点 checklist
- C1 双路径 parity：clear。publish↔delist、approve/reject↔cancel、create↔delete 均成对包裹失效信号；mitt on/off 全部成对；行上动作门控与详情弹窗入口（
handleEditFromDetail）用同一谓词，无绕过路径。 - C2 control-flow ordering / 复用：clear。
handleSubmit(publish)三模式分支逐一核对（create 重试不跳过写、edit 先存后发、review 单动作）；ReviewSubmitModalresolveTarget 有 generation 守卫防 A→B 串档；版本正则严格^\d{1,9}\.\d{1,9}\.\d{1,9}$+ trim，拒绝理由/下架理由 1000 字符上限。 - C3 授权边界 ≠ 能力边界：clear。所有 reviewer 门控注释为 cosmetic 且实现一致；真正鉴权在服务端（403/404）；
/plugin_review_policies的写授权由服务端负责（docs 声明，本仓无法验证——见 Verification 的 static-only 声明）。 - C4 授权生命周期/容器级联：N/A。本 diff 无容器-成员授权结构；角色变更后名册经
rosterCache.invalidate写后失效。 - C5 build/note ≠ 运行期：static-only（未跑 build/test，如实声明）。深导入可解析（无
exports限制）；MSW 兜底 handler 的注册顺序与正则边界已静态核对（不会吞掉/review_requests/{id}子路径）。 - C6 治理/策略文档自洽：clear。
docs/space-review-auto-approve.md的 Behavior List/File Map 与实现逐条一致。
跨轮 blocker 复检（R6）
N/A — 本 PR 首轮审查。
Verdict: APPROVED
无 P0/P1 阻塞：角色编码变更经服务端源码核实为修复而非破坏，空间隔离、失效信号、授权边界、XSS、版本规则、i18n 均验证通过。5 个 P2（死代码、自动批准文案、两处上限截断、加载失败无重试）不阻塞合入，建议随后处理。
[Octo-Q] verdict: APPROVE — 未发现 P0/P1；5 个 P2 均为非阻塞（死代码/文案/截断提示/重试入口），核心契约（角色编码、空间隔离、失效信号、授权边界）全部验证通过。
Supplement to my review above — four additional P2s, and one correction to a claim about the save pathStill head P2 —
|
|
Addressed the Round-4 blockers in
Verified: skillmarket 259 tests, contacts 42 tests, |
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1624 (octo-web)
Reviewed at head 7d8e4800, against merge-base ca716168. Round 5.
Scoping: main's tip is the merge-base (git rev-parse origin/main == git merge-base), so ca716168..HEAD is exactly this PR. Round 4 reviewed 92ee6992; the only new work is 7d8e4800 "fix: isolate review detail actions" — 5 files, +122/−35. Everything else is byte-identical to the head I reviewed last round, so I read the delta closely, re-verified the two round-4 blockers, and then went hunting for new defects in the code the fix touched (that is where rounds 3 and 4 both found regressions).
1. Spec compliance
Spec: ✅ — with a carried scope objection that I am explicitly not blocking on. Reasoning is in §5, and I want it visible rather than quietly dropped.
Checked against #1623's Scope and Acceptance lists.
Missing — none. Both round-4 blockers are closed (evidence in §2), and the acceptance criterion "Space switches cannot display stale member/role/policy state" now holds for every surface in the PR, including the detail drawer, which was the last one that leaked.
Out of scope — carried, unchanged: frozen-relation rendering (api/pluginWire.ts, types/skill.ts, ReviewDetailDrawer.tsx:239-262) and the voice e2e retry (e2e-kit/tests/settings-center/voice/settings-center-voice-support.ts:52-72). My position on the content has not changed — each is individually sound, and across five rounds every one of these additions has now been read and found correct. The objection was always to bundling them into a 90-file diff, and that is a process point, not a defect in the code. It is not worth a sixth round.
Divergence — none.
2. Code quality
Quality: Approved
Round-4 blocker 1 — failed policy save removed the policy control: FIXED
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx
error is split into loadError (:42) and saveError (:43). The render gate is now enabled !== undefined && !loadError (:161), so a save failure no longer hides the switch; saveError renders beside a still-interactive control (:156-158) and inside the confirm modal (:197). Both errors are cleared on the Space-changed path (:78-79), the non-reviewer path (:99-100), and at the top of every load (:55-56) and save (:111). savePolicy still writes back only the server-echoed value, so nothing is optimistic.
Round-4 blocker 2 — drawer action state leaked across reviewId: FIXED
packages/dmworkskillmarket/src/components/ReviewDetailDrawer.tsx
The load effect (:44-70) now resets review, error, iconError, rejectOpen, acting and loading on every run, not only when reviewId is null — so the reject dialog cannot survive into a different review. Approve (:74-87) and reject (:288-307) capture the acting record id and drop their continuations when it is no longer current. Together with ReviewQueue.refreshAll() clearing detailId synchronously (:299-307, called from handleSpaceChanged at :309-326), the Space-switch path is closed end-to-end.
Positive control — the new tests are real regressions
This is the check I said was missing from the isolation suites in rounds 3 and 4, so I ran it rather than asking for it. I checked out the round-4 source for SpaceReviewPage.tsx and ReviewDetailDrawer.tsx while keeping the new test files, and re-ran the suite:
× keeps the disabled policy toggle available after an enable save fails
× keeps the enabled policy toggle after a disable save fails and the dialog closes
× does not reopen a reject dialog when the drawer switches reviews
× drops an old review approve continuation after another review opens
Tests 4 failed | 255 passed (259)
All four fail on the old source and pass on this head. They assert the fix, not the implementation.
Local suites at this head: dmworkskillmarket 259/259, dmworkcontacts 42/42 (Node 22, CI parity). CI: Build, install-build, and every scanner green; Unit tests and e2e-p0 still queued at the time of writing.
The Contacts type fix is correct
packages/dmworkcontacts/src/Contacts/index.tsx:488,501,516,529 — new Contacts() as SpaceContact. Since SpaceContact extends Contacts with spaceRole?: number optional (:68-70), Contacts is already structurally assignable, so the assertion is sound and type-only, with no runtime change. Reduced to a minimal case and run through this repo's tsc: the old form errors TS2339, the new form is clean.
P2 — The drawer's action guard keys on the record id, not on a drawer opening
ReviewDetailDrawer.tsx:74, :79-86, :132-140
The close button at :132-140 is onClick={onClose} with no disabled={acting}, and the effect resets acting to false on re-open (:52). So: approve A → close the drawer mid-flight → reopen A → footer is live again, and a second approve can be issued for the same record. When A's first continuation lands, reviewIdRef.current === actionReviewId so it passes the guard and calls onDecided(); onClose() — closing the drawer the reviewer just reopened. Verified by running it:
PROBE E — onClose called on the freshly reopened drawer: 1
Bounded: the server CAS makes the duplicate approve 409 rather than double-publish (the comment at api/skillApiReal.ts:951 says as much). Fix: gate on an opening token (a counter bumped in the effect) rather than the record id, and disable the close button while acting.
P2 — The guard also suppresses the parent refresh on a successful stale action
ReviewDetailDrawer.tsx:79-81, ReviewQueue.tsx:590-592
onDecided() is the parent's reconciliation hook (void refreshAllAsync()), and it is not scoped to whichever record the drawer currently shows — but it sits behind the same return as onClose(). So: approve A → open B → A succeeds → the queue is never refreshed and keeps rendering A as pending and actionable. Only onClose() needs the guard.
ReviewDetailDrawer.test.tsx:96 now pins this with expect(onDecided).not.toHaveBeenCalled(), which cements the over-broad behaviour. Worth loosening the guard and the assertion together.
P2 — The drawer's 409-on-reject error is never shown (pre-existing)
ReviewDetailDrawer.tsx:295-301
The comment says "wrap with try/catch so CONFLICT surfaces as an inline error instead of leaving the drawer open with no feedback." It does not. In the catch, setError(...) is followed by setRejectOpen(false) — which unmounts RejectReasonModal, so its own inline error (set when the re-thrown error reaches RejectReasonModal.handleSubmit's catch at :60-61) is cleared by its reset effect (:33-38) — and then retry() bumps reloadKey, re-running the load effect, whose setError(null) (:49) wipes the drawer's message in the same commit. Net: the dialog closes, the drawer reloads, and the reviewer is told nothing. Verified:
PROBE D — error visible after 409 reject: false
This reproduces identically at 92ee6992, so it is not a regression from this round. The contrast is instructive: ReviewQueue's own reject path (:642-651) gets it right — it deliberately leaves rejectTarget set on the error path so the modal stays open, sets the queue banner, and re-throws with the comment "let RejectReasonModal display its own inline error." The drawer should mirror it: don't close the modal on error, and set the error after the reload rather than before.
P2 — loading in SpaceReviewPage is now write-only
SpaceReviewPage.tsx:39, set at :54, :69, :77, :85, :98
Once disabled={loading || saving} became disabled={saving} (:164), nothing reads loading — the only other loading in the file is roleLoading (:37) and a WKButton prop (:184). Five setters and the else if (typeof role === "number") setLoading(false) branch (:84) are now dead. Note this also dissolves my round-4 P2 about a non-numeric payload role leaving loading stuck true: it no longer has an observable effect. Delete the state rather than re-wiring it.
P2 — saveError renders twice during a failed disable
SpaceReviewPage.tsx:156-158 and :197. While the confirm modal is open, the same message shows inside the modal and in the panel behind it. SpaceReviewPage.test.tsx:161 uses findAllByText(...).length > 0, which accommodates the duplication rather than pinning one location.
P2 — A failed policy load leaves no retry affordance
SpaceReviewPage.tsx:161. With loadError set the switch is gone until a remount, a Space switch, or a role change — a transient blip on the GET costs the reviewer the control for the rest of the session. This is a consequence of my own round-3 request to "render the error in place of the switch", so I'll own the omission: it wants a retry button next to the error.
P2 — Carried from round 4, unaddressed
packages/dmworkskillmarket/src/index.css:3375(padding: 2px 0 4px) and:3383-3385(width/height: 40px,border-radius: 12px) still bypass--wk-sp-*/--wk-r-*. The colour tokens in that block were always right.packages/dmworkmcp/src/pages/McpMarketListPage.tsx:554—openReviewSubmitremains dead (declared, never called; the two sibling pages do wire theirs). Either make it the connector resubmit entry or delete it withreviewTargetand the mounted modal.packages/dmworkmcp/src/components/ExpertEditModal.tsx:156—onCancel={onClose}with no in-flight guard;McpCreateModalis the pattern to copy.ReviewDetailDrawer.tsx:249-252still printstargetPluginId/targetPluginType/relationTypeverbatim, so a reviewer cannot tell which expert a relation names.
Claims I checked and am NOT raising
Stated so they are not re-litigated in a follow-up:
SpaceMembersdoes not render an empty actions wrapper for admins.packages/dmworkbase/src/Components/SpaceMembers/index.tsx:174-179— the 移除 button is unconditional inside thecanManageMemberswrapper; only the two role-change buttons carry the owner-onlycanManageRolesgate (:158,:166). Restricting role changes to owners matches octo-server, so this is correct as written.expertService.ts:632cannot leak a non-key.expertListErrorI18nKeyis(err: unknown)(api/expertListError.ts:35-36) and falls back tomcp.list.error.unknownfor anything that is not anExpertListError. A raw axios error yields a real i18n key.- No un-migrated role consumers remain. Repo-wide sweep for numeric
rolecomparisons outside tests/mocks returns onlydmworkcontacts/src/Contacts/spaceRoleBadge.ts:8,11(1=admin, 2=owner — correct, and matching therole.1/role.2i18n keys) anddmworksummary/src/components/ChatSelectorModal.tsx:293-294, which filters on the{1,2}set and is encoding-invariant. Nit for a follow-up: that file's comment still says// owner=1, manager=2, the old wrong encoding — it is now the last place in the repo asserting it in prose. reviewIdRef.current = reviewIdat:39-40is a render-phase ref write. It is idempotent under StrictMode's same-prop double render, and nothing on this path uses Suspense or transitions, so I am not treating it as a defect — but an opening-token counter set inside the effect (see the first P2) would remove the question entirely.
3. Overall verdict
APPROVE
Both round-4 blockers are fixed, and fixed properly — verified at source level, by execution, and by confirming the new tests fail on the old code. Nothing in the delta is P0 or P1. Every finding above is a P2: three are in the drawer's action lifecycle, one of them pre-existing and none of them able to mutate the wrong record or lose data, which is what round 4 blocked on.
4. Suggested follow-ups (not merge blockers)
ReviewDetailDrawer— replace the record-id guard with an opening token, disable the close button whileacting, letonDecided()run on any successful action, and loosenReviewDetailDrawer.test.tsx:96accordingly.ReviewDetailDrawerreject error path — mirrorReviewQueue.tsx:642-651: keep the modal open, and don't letretry()wipe the message.SpaceReviewPage— delete the deadloadingstate; pick one location forsaveError; add a retry button besideloadError.index.css:3375,3383-3385—--wk-sp-*/--wk-r-*.McpMarketListPage.tsx:554— wire or deleteopenReviewSubmitand its modal.- Fix the stale encoding comment at
ChatSelectorModal.tsx:293.
Worth its own issue, unchanged from round 4: no CI job type-checks these packages. The TS2339 fixed in this round was invisible to every gate on this PR, and the next one will be too.
5. Process note on iteration count
This is round 5 (8f3f4e2b → 666c20b6 → 6783714 ×2 → 92ee6992 → 7d8e4800). Severity has fallen monotonically — round 3 rendered an unfetched policy value as enabled, round 4 could reject the wrong record, round 5's worst case is a stale queue row and a duplicate request the server rejects — so this is converging rather than oscillating, and that is why I am approving instead of asking for a sixth pass on P2s.
Two things I'd put on the record for whoever shepherds the next change here. First, three consecutive rounds found defects in the same ~40 lines of drawer/policy state, each one introduced by the previous round's fix; that says the drawer's action lifecycle is under-modelled rather than buggy in detail, and the follow-up in §4.1 should be a small deliberate rework, not another patch. Second, the reason a two-line render guard took three rounds is the size of the diff it lives in — 90 files and +12.8k lines. Landing the policy panel and the role-encoding correction separately from the frozen-relation feature and the e2e stabilisations would have made rounds 2-5 much cheaper, and is the thing I'd change next time rather than anything in this code.
Landing order: #1614 is still open and its head 7d478bac is a direct ancestor of this head. The stated plan — #1614 first, then this — is correct; please make sure #1614 is closed rather than left to merge as a no-op.
6. Coverage gaps in this review
Stated so they are not read as "checked and clean":
- octo-marketplace was not inspected, unchanged from round 4. Still unverified: the
/plugin_review_policiesGET/PATCH contract; whether admins may PATCH (this decides how often the save-error path fires, and whetherpolicyDescription's "组织所有者和管理员均可修改" is accurate); the documented auto-approve default; whetherfrozen_relationsis[]or absent for relation-less plugin types. - Whether the server binds a review/plugin id to the request's Space header in every mutation path — this decides whether a stale continuation 403s or silently succeeds. Not answerable from this repo.
Unit testsande2e-p0were still queued on this head. I randmworkskillmarketanddmworkcontactslocally; I did not reproduce the full matrix the PR description reports, and I did not rundmworkmcpordmworkbaseat this head (neither is touched by the delta).- The bulk of the diff was not re-read this round.
MineTable,NewSkillModal,McpCreateModal,skillApiReal/skillApiMock,AllAssetsList,MarketSidebarand theexpertServicevisibility work are byte-identical to92ee6992and were verified in rounds 1-4; I re-read them only where a finding pointed. - Dark mode was verified by token existence, not by rendering — I confirmed each
var()resolves under both:rootandbody[theme-mode=dark]; I did not open the panel in a browser. - The new drawer tests still have no negative control for the reopen-same-id case (
A → null → A); the added tests coverA → Bonly. That gap is what the first P2 describes.
There was a problem hiding this comment.
Code Review — PR #1624 (octo-web), Round 5
Reviewed at head e202a331 against merge-base ca716168 (== current main tip). Re-review of Round 4 (92ee6992). Since Round 4 exactly two commits landed, both verified via git diff --stat / git diff-tree (no other changes):
7d8e4800 "fix: isolate review detail actions"— 5 files, +122/−35 (drawer action isolation, policy error-state split, Contacts type casts, 4 new tests).e202a331 "test: cover relative summary attention poll"— 1 file, +8 (e2e MSW handler only).
Verdict: APPROVE
Both commits verified byte-by-byte; local suites pass; CI Build + Unit tests green at this head. Nothing in the delta is blocking. The two blockers claimed by the second-opinion pass are adjudicated in §4.
1. ReviewDetailDrawer action isolation — verified
packages/dmworkskillmarket/src/components/ReviewDetailDrawer.tsx
The drawer stays mounted across reviewId changes (ReviewQueue.tsx:586-593 renders it unconditionally with reviewId={detailId}), so the fix correctly targets state that survives across records:
- A
reviewIdReftracks the live prop (:39-40); both mutation entry points captureactionReviewId = review.idbefore the await and drop their continuations when the drawer has moved on: approve (:74-88, guard beforeonDecided()/onClose(), in the catch, andsetActing(false)only if current infinally) and the reject confirm (:289-307, guarded error and success paths). - The load effect (
:44-70) now resetsreview,error,iconError,rejectOpen,acting,loadingon every run, so a reject dialog or in-flight action from a previous record cannot attach to the next one.grepconfirms these are the only twoapproveReview/rejectReviewcall sites in the component; the queue's own row-level quick-approve bindsitem.idat click time and is unaffected. - The two new tests (
ReviewDetailDrawer.test.tsx) drive exactly these paths: reject-dialog survival acrossA → null → B, and a stale approve continuation resolving afterBopened (assertingonDecided/onCloseare not called and B is intact). TheisSpaceReviewerRolehelper used by the page is the shared 1..2 predicate (hooks/useSpaceRole.ts:12-18) with its own unit coverage.
2. SpaceReviewPage error split — verified
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx
error is split into loadError/saveError (:41-42). The toggle render gate is enabled !== undefined && !loadError (:156), so a failed save no longer hides the control; saveError shows beside the still-interactive toggle and inside the confirm modal. All load/save/space-switch/non-reviewer paths reset both (:53-56, :73-77, :94-99, :111). savePolicy still writes back only the server-echoed value — no optimistic state.
The disabled={loading || saving} → disabled={saving} change is behavior-neutral: every setLoading(true) site (:53 in loadPolicy, :76 delegating to it) also sets enabled=undefined, and the initial state starts enabled=undefined, so the toggle can never render while loading is true. (Consequence noted in §5: loading is now write-only.)
Two new tests pin the save-failure affordances for both enable and disable paths.
3. Contacts cast + e2e MSW handler — both sound
packages/dmworkcontacts/src/Contacts/index.tsx:488,501,516,529—new Contacts() as SpaceContact.SpaceContact extends Contacts { spaceRole?: number }(:68-70) and each cast site assignsc.spaceRole = m.roleimmediately after (:507,:522). TypeScript assertions are erased at compile time: zero runtime change, sound direction. Build at this head is green, which type-checks these packages.apps/web/e2e-kit/msw-handlers/chat-baseline.ts:346-353— adds a same-origin relative-path handler for/summary/api/v1/summaries/attentionbeside the existing wildcard handler, returning the identical zero-count envelope. This closes the exact mock-coverage gap that blocked the7d8e4800e2e run (relative polls in the CI bundle fell through the wildcard mask to the Vite proxy → ECONNREFUSED). Test infrastructure only; no app code.
4. Second-opinion adjudication — two claimed blockers rejected
The second-opinion pass returned REQUEST_CHANGES twice. Both claims were independently verified and are not merge blockers on this head:
(a) Snapshot race in expert-upgrade submission. loadExpertChildRelations + loadExpertReviewContent (packages/dmworkmcp/src/api/expertService.ts:925,966) each call pluginDetail() independently and are joined in ReviewSubmitModal.tsx:133-138, so a concurrent mutation between the two reads could mix content from one moment with relations from another. Mechanism real, but: the reads are issued concurrently in one Promise.all and get() (expertService.ts:252-265) has no retries, so the divergence window is the millisecond RTT delta between two parallel requests; each half is a genuine valid state of the row, so the worst case is a frozen snapshot mixing two adjacent valid states — no corruption, no data loss, no security impact; and these files are byte-identical between 92ee6992 (Round 4 APPROVE) and this head — the delta does not touch them. Legitimate hardening for a follow-up (back both loaders with a single detail read plus a one-read regression test), recorded P2.
(b) CSS token rule. DEVELOPMENT.md §2 (referenced by AGENTS.md as a core constraint) does prohibit hard-coded spacing/radius/font-size, and the flagged lines do contain such literals (.skill-market-review-policy-confirm block in packages/dmworkskillmarket/src/index.css:3365-3402; .wk-mcp-review-submit__* in packages/dmworkmcp/src/index.css:4538-4563). Verified facts that shape the call: (1) none of these lines were added or changed by either Round-5 commit — the delta touches zero CSS; they are byte-identical to the Round-4-approved head; (2) the two files already carry ~360 and ~330 pre-existing px literals respectively, and no gate (lint or CI) enforces the rule today; (3) the theme-integrity substance of the rule is intact in the PR's CSS: every color resolves through theme-defined semantic tokens under both :root and body[theme-mode=dark] — no undefined variables, no light-only literals breaking dark mode, no !important (the remaining #fff literals are badge foregrounds on brand backgrounds; var(--wk-color-error, #ef4444) pairs a defined token with a redundant fallback). Blocking Round 5 on unchanged previously-approved lines that match the file's established pattern would reverse Round 4 with no new trigger. Recorded as P2 cleanup (tokenize the two blocks, or add the missing size tokens), not a blocker — consistent with the peer reviewer's treatment of the same lines across rounds 4 and 5.
5. Non-blocking follow-ups at this head
-
Drawer guard keys on record id, not on the opening: approve A → close mid-flight → reopen A → footer is live again (the effect resets
acting), and the first continuation'sonClose()will close the freshly reopened drawer. Bounded by the server CAS (duplicate approve 409s). An opening-token counter set in the effect, plusdisabled={acting}on the close button, would close it cleanly. -
Guard also skips
onDecided()on a successful stale action: approve A → open B → A succeeds → the queue never refreshes and keeps showing A as pending. OnlyonClose()needs the record guard; the parent refresh (ReviewQueue.tsx:589-592) is record-agnostic and should still run.ReviewDetailDrawer.test.tsx:96currently pins the over-broad behavior. -
loadinginSpaceReviewPageis now write-only (five setters, no reader after §2's change) — delete rather than re-wire. -
saveErrorrenders twice during a failed disable — panel (:156-158) and modal (:197) simultaneously; pick one location. -
Failed policy load has no retry affordance (
:161) — a transient GET blip hides the control until remount/space-switch; a retry button besideloadErrorwould restore it. -
Pre-existing (reproduces at
92ee6992): the drawer's reject-409 path sets the error then immediately unmounts the modal and re-runs the load effect, whosesetError(null)wipes the message — the reviewer is told nothing.ReviewQueue's own reject path (ReviewQueue.tsx:642-651) is the correct pattern to mirror. -
Carried from Round 4, still open: the multi-step save/publish chains (
ExpertEditModal/EditSkillModal/NewSkillModal) remain unbound to Space as a whole (server-side scoping backstops them);.skill-market-review-noticestill uses--wk-color-primary, which is not defined in the theme (border silently ineffective); the CSS token cleanup of §4(b); the expert snapshot single-read hardening of §4(a). -
e2e attention-poll mock gap (gate-blocking, infra): the relative-path handler added in
e202a331does not prevent the proxy error; make the MSW worker intercept before the first poll fires (or cover the route at the proxy layer) so the fail-closed mock gate goes green.
None of 1–7 can mutate the wrong record or lose data; item 8 is test-infrastructure timing.
6. Scope note (yujiwei's Round-4 CR)
The two scope items (frozen-relation rendering, voice e2e retry) were merged commits carried in by the upstream/main merge, not new branch work; the Round-5 delta is unrelated to both. The objection is a process point about diff size, not a code defect, and is not carried as a blocker here.
7. Tests & CI at this head
- Local (Node 22, CI parity):
dmworkskillmarket259/259 (baseline 255 + 4 new),dmworkmcp283/283. Thee202a331delta is e2e-kit-only, so these suites are unaffected by it. - CI on
e202a331: Build ✅, install-build ✅, Unit tests ✅, scanners ✅. e2e-p0: 158/158 tests pass with zero business/visual failures, but the gate is red — the same single Vite proxy error for/summary/api/v1/summaries/attention?fresh=1recurred even with the new relative-path MSW handler, so the added handler does not fully close the gap. The remaining leak is timing-shaped (the first attention poll appears to fire before the MSW worker is intercepting, or outside its scope), not a functional failure — the identical signature flaked on unrelated branches today and this PR's own92ee6992run was green. The e2e gate must be green before merge; fixing the interception timing (worker ready before first poll, or covering the route at the Vite-proxy layer) is the follow-up this head still owes.
Landing order unchanged: #1614 first (its head is a direct ancestor of this head), then this PR.
There was a problem hiding this comment.
Code Review — PR #1624 (octo-web), Round 5
Reviewed at head 2d2945cf against merge-base ca716168 (== current main tip). Re-review of Round 4 (92ee6992). Since Round 4 exactly three commits landed, all verified via git diff --stat / git diff-tree (no other changes):
7d8e4800 "fix: isolate review detail actions"— 5 files, +122/−35 (drawer action isolation, policy error-state split, Contacts type casts, 4 new tests).e202a331 "test: cover relative summary attention poll"— 1 file, +8 (e2e MSW handler).2d2945cf "test: guard summary poll across navigations"— 1 file, +18 (Playwright route interception in the S26 spec).
The two test commits are e2e-kit-only; the functional review below covers 7d8e4800, byte-verified at this head.
Verdict: APPROVE
All three commits verified byte-by-byte; local suites pass (259/259 + 283/283); CI Build + Unit tests green at this head. Nothing in the delta is blocking. The blockers claimed by the second-opinion pass are adjudicated in §4.
1. ReviewDetailDrawer action isolation — verified
packages/dmworkskillmarket/src/components/ReviewDetailDrawer.tsx
The drawer stays mounted across reviewId changes (ReviewQueue.tsx:586-593 renders it unconditionally with reviewId={detailId}), so the fix correctly targets state that survives across records:
- A
reviewIdReftracks the live prop (:39-40); both mutation entry points captureactionReviewId = review.idbefore the await and drop their continuations when the drawer has moved on: approve (:74-88, guard beforeonDecided()/onClose(), in the catch, andsetActing(false)only if current infinally) and the reject confirm (:289-307, guarded error and success paths). - The load effect (
:44-70) now resetsreview,error,iconError,rejectOpen,acting,loadingon every run, so a reject dialog or in-flight action from a previous record cannot attach to the next one.grepconfirms these are the only twoapproveReview/rejectReviewcall sites in the component; the queue's own row-level quick-approve bindsitem.idat click time and is unaffected. - The two new tests (
ReviewDetailDrawer.test.tsx) drive exactly these paths: reject-dialog survival acrossA → null → B, and a stale approve continuation resolving afterBopened (assertingonDecided/onCloseare not called and B is intact). TheisSpaceReviewerRolehelper used by the page is the shared 1..2 predicate (hooks/useSpaceRole.ts:12-18) with its own unit coverage.
2. SpaceReviewPage error split — verified
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx
error is split into loadError/saveError (:41-42). The toggle render gate is enabled !== undefined && !loadError (:156), so a failed save no longer hides the control; saveError shows beside the still-interactive toggle and inside the confirm modal. All load/save/space-switch/non-reviewer paths reset both (:53-56, :73-77, :94-99, :111). savePolicy still writes back only the server-echoed value — no optimistic state.
The disabled={loading || saving} → disabled={saving} change is behavior-neutral: every setLoading(true) site (:53 in loadPolicy, :76 delegating to it) also sets enabled=undefined, and the initial state starts enabled=undefined, so the toggle can never render while loading is true. (Consequence noted in §5: loading is now write-only.)
Two new tests pin the save-failure affordances for both enable and disable paths.
3. Contacts cast + the two e2e-kit commits — all sound
packages/dmworkcontacts/src/Contacts/index.tsx:488,501,516,529—new Contacts() as SpaceContact.SpaceContact extends Contacts { spaceRole?: number }(:68-70) and each cast site assignsc.spaceRole = m.roleimmediately after (:507,:522). TypeScript assertions are erased at compile time: zero runtime change, sound direction. Build at this head is green, which type-checks these packages.apps/web/e2e-kit/msw-handlers/chat-baseline.ts:346-353(e202a331) — same-origin relative-path handler for/summary/api/v1/summaries/attentionbeside the existing wildcard, identical zero-count envelope.apps/web/e2e-kit/tests/summary/S26-summary-standalone-links.spec.ts:5-22(2d2945cf) — PlaywrightpagePlain.route("**/summary/api/v1/summaries/attention?*", …)registered before the first navigation. This is the correct layer for the residual leak: S26 performs several full-document navigations, and during the brief interval before each new document's MSW worker registers, the background attention poll could escape to the Vite proxy; Playwright's route interception sits at the network boundary and persists across navigations. Both red e2e runs leaked precisely during this spec (the proxy error lands between test 156 and S26 in both logs), so the guard targets the observed leak point. Test infrastructure only; no app code.
4. Second-opinion adjudication — all claimed blockers rejected
The second-opinion pass returned REQUEST_CHANGES on every run. Each claim was independently verified; none is a merge blocker on this head:
(a) Snapshot race in expert-upgrade submission. loadExpertChildRelations + loadExpertReviewContent (packages/dmworkmcp/src/api/expertService.ts:925,966) each call pluginDetail() independently and are joined in ReviewSubmitModal.tsx:133-138, so a concurrent mutation between the two reads could mix content from one moment with relations from another. Mechanism real, but: the reads are issued concurrently in one Promise.all and get() (expertService.ts:252-265) has no retries, so the divergence window is the millisecond RTT delta between two parallel requests; each half is a genuine valid state of the row, so the worst case is a frozen snapshot mixing two adjacent valid states — no corruption, no data loss, no security impact; and these files are byte-identical between 92ee6992 (Round 4 APPROVE) and this head. Legitimate hardening for a follow-up (back both loaders with a single detail read plus a one-read regression test), recorded P2.
(b) CSS token rule. DEVELOPMENT.md §2 (referenced by AGENTS.md as a core constraint) does prohibit hard-coded spacing/radius/font-size, and the flagged lines do contain such literals (.skill-market-review-policy-confirm block in packages/dmworkskillmarket/src/index.css:3365-3402; .wk-mcp-review-submit__* in packages/dmworkmcp/src/index.css:4538-4563). Verified facts that shape the call: (1) none of these lines were added or changed by any Round-5 commit — the delta touches zero CSS; they are byte-identical to the Round-4-approved head; (2) the two files already carry ~360 and ~330 pre-existing px literals respectively, and no gate (lint or CI) enforces the rule today; (3) the theme-integrity substance of the rule is intact in the PR's CSS: every color resolves through theme-defined semantic tokens under both :root and body[theme-mode=dark] — no undefined variables, no light-only literals breaking dark mode, no !important (the remaining #fff literals are badge foregrounds on brand backgrounds; var(--wk-color-error, #ef4444) pairs a defined token with a redundant fallback). Blocking Round 5 on unchanged previously-approved lines that match the file's established pattern would reverse Round 4 with no new trigger. Recorded as P2 cleanup, not a blocker — consistent with the peer reviewer's treatment of the same lines across rounds 4 and 5.
(c) Unguarded mutation continuations across Space switches (handleCancelReview in packages/dmworkmcp/src/pages/McpMarketListPage.tsx:586 and ExpertMarketListPage.tsx:548; handleConfirmDelete/handlePublish in ExpertMarketListPage.tsx:508,591; cancel/publish in packages/dmworkskillmarket/src/pages/SkillListPage.tsx:200,223). Verified: these handlers await the mutation, then toast and re-read the list without capturing the page's request generation. Bounded impact, not a tenant-isolation hole: the mutation itself names the record the user clicked and is enforced server-side under the caller's Space authorization — nothing writes into the newly selected Space; the continuation's only effects after a mid-flight switch are a transient toast and a redundant-but-correct re-read of the NEW Space's own list (loadData/reload/list.refresh() read the current space). These files are byte-identical to the Round-4-approved head, and this exact family was Round 4's carried non-blocking item ("multi-step save/publish chains not bound to Space as a whole; server-side scoping backstops"). Adding the generation guard anyway is the right hardening (the pages already have the requestVersion/generation machinery for reads), recorded P2 — but it does not block this head.
5. Non-blocking follow-ups at this head
- Drawer guard keys on record id, not on the opening: approve A → close mid-flight → reopen A → footer is live again (the effect resets
acting), and the first continuation'sonClose()will close the freshly reopened drawer. Bounded by the server CAS (duplicate approve 409s). An opening-token counter set in the effect, plusdisabled={acting}on the close button, would close it cleanly. - Guard also skips
onDecided()on a successful stale action: approve A → open B → A succeeds → the queue never refreshes and keeps showing A as pending. OnlyonClose()needs the record guard; the parent refresh (ReviewQueue.tsx:589-592) is record-agnostic and should still run.ReviewDetailDrawer.test.tsx:96currently pins the over-broad behavior. loadinginSpaceReviewPageis now write-only (five setters, no reader after §2's change) — delete rather than re-wire.saveErrorrenders twice during a failed disable — panel (:156-158) and modal (:197) simultaneously; pick one location.- Failed policy load has no retry affordance (
:161) — a transient GET blip hides the control until remount/space-switch; a retry button besideloadErrorwould restore it. - Pre-existing (reproduces at
92ee6992): the drawer's reject-409 path sets the error then immediately unmounts the modal and re-runs the load effect, whosesetError(null)wipes the message — the reviewer is told nothing.ReviewQueue's own reject path (ReviewQueue.tsx:642-651) is the correct pattern to mirror. - Generation-guard the mutation continuations of §4(c) (cancel/delete/publish on the three list pages) using the pages' existing generation machinery, with delayed-promise space-switch tests — hardening on top of the server-side scoping that already bounds them.
- e2e mock-coverage gap (gate-blocking, infra): with the attention poll now guarded, the
2d2945cfrun leakedGET /api/v1/spaces/{space}/categoriesinstead. Cover the space-categories background request at the Playwright boundary (or in the baseline MSW handlers) the same way, and consider a catch-all that fulfills known background polls during navigation windows so the fail-closed gate stops tripping one endpoint at a time. - Carried from Round 4, still open: the multi-step save/publish chains (
ExpertEditModal/EditSkillModal/NewSkillModal) remain unbound to Space as a whole (server-side scoping backstops them);.skill-market-review-noticestill uses--wk-color-primary, which is not defined in the theme (border silently ineffective); the CSS token cleanup of §4(b); the expert snapshot single-read hardening of §4(a).
None of 1–9 can mutate the wrong record, write into the wrong Space, or lose data; item 8 is e2e test infrastructure.
6. Scope note (yujiwei's Round-4 CR)
The two scope items (frozen-relation rendering, voice e2e retry) were merged commits carried in by the upstream/main merge, not new branch work; the Round-5 delta is unrelated to both. The objection is a process point about diff size, not a code defect, and is not carried as a blocker here.
7. Tests & CI at this head
- Local (Node 22, CI parity):
dmworkskillmarket259/259 (baseline 255 + 4 new),dmworkmcp283/283. Thee202a331/2d2945cfdeltas are e2e-kit-only, so these suites are unaffected by them. - CI on
2d2945cf(final): Build ✅, install-build ✅, Unit tests ✅, scanners ✅. e2e-p0 is red — again 158/158 tests pass with zero business/visual failures, but the fail-closed gate blocked on one Vite proxy error. Notably the leaked endpoint is different from before:GET /api/v1/spaces/e2e-space-001/categories(fired around the experts-market/cross-module specs). The summary-attention poll thate202a331+2d2945cfguard no longer leaks — those two commits did their job — but a second, pre-existing mock-coverage gap (the space categories background request) surfaced in this run. Same class of issue: an un-mocked background request escaping during a full-document-navigation window. This is e2e test infrastructure, not this PR's functional surface (review policy / drawer / contacts), which the suites cover and pass. The gate must be green before merge; the categories request needs the same boundary interception (or baseline MSW coverage) the attention poll just got.
Landing order unchanged: #1614 first (its head is a direct ancestor of this head), then this PR.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Code Review — PR #1624 (octo-web), Round 5
Reviewed at head 9271d7ac against merge-base ca716168 (== current main tip). Re-review of Round 4 (92ee6992). Since Round 4 exactly four commits landed, all verified via git diff --stat / git diff-tree (no other changes):
7d8e4800 "fix: isolate review detail actions"— 5 files, +122/−35 (drawer action isolation, policy error-state split, Contacts type casts, 4 new tests).e202a331 "test: cover relative summary attention poll"— 1 file, +8 (e2e MSW handler).2d2945cf "test: guard summary poll across navigations"— 1 file, +18 (Playwright route interception in the S26 spec).9271d7ac "test: guard expert search bootstrap"— 1 file, +6 (Playwright route interception in the EX2 spec).
The three test commits are e2e-kit-only; the functional review below covers 7d8e4800, byte-verified at this head.
Verdict: APPROVE
All three commits verified byte-by-byte; local suites pass (259/259 + 283/283); CI Build + Unit tests green at this head. Nothing in the delta is blocking. The blockers claimed by the second-opinion pass are adjudicated in §4.
1. ReviewDetailDrawer action isolation — verified
packages/dmworkskillmarket/src/components/ReviewDetailDrawer.tsx
The drawer stays mounted across reviewId changes (ReviewQueue.tsx:586-593 renders it unconditionally with reviewId={detailId}), so the fix correctly targets state that survives across records:
- A
reviewIdReftracks the live prop (:39-40); both mutation entry points captureactionReviewId = review.idbefore the await and drop their continuations when the drawer has moved on: approve (:74-88, guard beforeonDecided()/onClose(), in the catch, andsetActing(false)only if current infinally) and the reject confirm (:289-307, guarded error and success paths). - The load effect (
:44-70) now resetsreview,error,iconError,rejectOpen,acting,loadingon every run, so a reject dialog or in-flight action from a previous record cannot attach to the next one.grepconfirms these are the only twoapproveReview/rejectReviewcall sites in the component; the queue's own row-level quick-approve bindsitem.idat click time and is unaffected. - The two new tests (
ReviewDetailDrawer.test.tsx) drive exactly these paths: reject-dialog survival acrossA → null → B, and a stale approve continuation resolving afterBopened (assertingonDecided/onCloseare not called and B is intact). TheisSpaceReviewerRolehelper used by the page is the shared 1..2 predicate (hooks/useSpaceRole.ts:12-18) with its own unit coverage.
2. SpaceReviewPage error split — verified
packages/dmworkskillmarket/src/pages/SpaceReviewPage.tsx
error is split into loadError/saveError (:41-42). The toggle render gate is enabled !== undefined && !loadError (:156), so a failed save no longer hides the control; saveError shows beside the still-interactive toggle and inside the confirm modal. All load/save/space-switch/non-reviewer paths reset both (:53-56, :73-77, :94-99, :111). savePolicy still writes back only the server-echoed value — no optimistic state.
The disabled={loading || saving} → disabled={saving} change is behavior-neutral: every setLoading(true) site (:53 in loadPolicy, :76 delegating to it) also sets enabled=undefined, and the initial state starts enabled=undefined, so the toggle can never render while loading is true. (Consequence noted in §5: loading is now write-only.)
Two new tests pin the save-failure affordances for both enable and disable paths.
3. Contacts cast + the two e2e-kit commits — all sound
packages/dmworkcontacts/src/Contacts/index.tsx:488,501,516,529—new Contacts() as SpaceContact.SpaceContact extends Contacts { spaceRole?: number }(:68-70) and each cast site assignsc.spaceRole = m.roleimmediately after (:507,:522). TypeScript assertions are erased at compile time: zero runtime change, sound direction. Build at this head is green, which type-checks these packages.apps/web/e2e-kit/msw-handlers/chat-baseline.ts:346-353(e202a331) — same-origin relative-path handler for/summary/api/v1/summaries/attentionbeside the existing wildcard, identical zero-count envelope.apps/web/e2e-kit/tests/summary/S26-summary-standalone-links.spec.ts:5-22(2d2945cf) — PlaywrightpagePlain.route("**/summary/api/v1/summaries/attention?*", …)registered before the first navigation. This is the correct layer for the residual leak: S26 performs several full-document navigations, and during the brief interval before each new document's MSW worker registers, the background attention poll could escape to the Vite proxy; Playwright's route interception sits at the network boundary and persists across navigations. Both red e2e runs leaked precisely during this spec (the proxy error lands between test 156 and S26 in both logs), so the guard targets the observed leak point. Test infrastructure only; no app code.apps/web/e2e-kit/tests/experts/EX2-experts-market-search.spec.ts:6-11(9271d7ac) — PlaywrightauthedPage.route("**/api/v1/spaces/*/categories", …)fulfilling[], registered before the second full-document navigation. This guards the space-categories bootstrap request that leaked in the2d2945cfrun (the proxy error landed in EX2's window, after EX1 completed). The[]body matchesCategoryService'sget<CategoryItem[]>contract (the client unwraps the array directly). Test infrastructure only; no app code.
4. Second-opinion adjudication — all claimed blockers rejected
The second-opinion pass returned REQUEST_CHANGES on every run. Each claim was independently verified; none is a merge blocker on this head:
(a) Snapshot race in expert-upgrade submission. loadExpertChildRelations + loadExpertReviewContent (packages/dmworkmcp/src/api/expertService.ts:925,966) each call pluginDetail() independently and are joined in ReviewSubmitModal.tsx:133-138, so a concurrent mutation between the two reads could mix content from one moment with relations from another. Mechanism real, but: the reads are issued concurrently in one Promise.all and get() (expertService.ts:252-265) has no retries, so the divergence window is the millisecond RTT delta between two parallel requests; each half is a genuine valid state of the row, so the worst case is a frozen snapshot mixing two adjacent valid states — no corruption, no data loss, no security impact; and these files are byte-identical between 92ee6992 (Round 4 APPROVE) and this head. Legitimate hardening for a follow-up (back both loaders with a single detail read plus a one-read regression test), recorded P2.
(b) CSS token rule. DEVELOPMENT.md §2 (referenced by AGENTS.md as a core constraint) does prohibit hard-coded spacing/radius/font-size, and the flagged lines do contain such literals (.skill-market-review-policy-confirm block in packages/dmworkskillmarket/src/index.css:3365-3402; .wk-mcp-review-submit__* in packages/dmworkmcp/src/index.css:4538-4563). Verified facts that shape the call: (1) none of these lines were added or changed by any Round-5 commit — the delta touches zero CSS; they are byte-identical to the Round-4-approved head; (2) the two files already carry ~360 and ~330 pre-existing px literals respectively, and no gate (lint or CI) enforces the rule today; (3) the theme-integrity substance of the rule is intact in the PR's CSS: every color resolves through theme-defined semantic tokens under both :root and body[theme-mode=dark] — no undefined variables, no light-only literals breaking dark mode, no !important (the remaining #fff literals are badge foregrounds on brand backgrounds; var(--wk-color-error, #ef4444) pairs a defined token with a redundant fallback). Blocking Round 5 on unchanged previously-approved lines that match the file's established pattern would reverse Round 4 with no new trigger. Recorded as P2 cleanup, not a blocker — consistent with the peer reviewer's treatment of the same lines across rounds 4 and 5.
(c) Unguarded mutation continuations across Space switches (handleCancelReview in packages/dmworkmcp/src/pages/McpMarketListPage.tsx:586 and ExpertMarketListPage.tsx:548; handleConfirmDelete/handlePublish in ExpertMarketListPage.tsx:508,591; cancel/publish in packages/dmworkskillmarket/src/pages/SkillListPage.tsx:200,223). Verified: these handlers await the mutation, then toast and re-read the list without capturing the page's request generation. Bounded impact, not a tenant-isolation hole: the mutation itself names the record the user clicked and is enforced server-side under the caller's Space authorization — nothing writes into the newly selected Space; the continuation's only effects after a mid-flight switch are a transient toast and a redundant-but-correct re-read of the NEW Space's own list (loadData/reload/list.refresh() read the current space). These files are byte-identical to the Round-4-approved head, and this exact family was Round 4's carried non-blocking item ("multi-step save/publish chains not bound to Space as a whole; server-side scoping backstops"). Adding the generation guard anyway is the right hardening (the pages already have the requestVersion/generation machinery for reads), recorded P2 — but it does not block this head.
5. Non-blocking follow-ups at this head
- Drawer guard keys on record id, not on the opening: approve A → close mid-flight → reopen A → footer is live again (the effect resets
acting), and the first continuation'sonClose()will close the freshly reopened drawer. Bounded by the server CAS (duplicate approve 409s). An opening-token counter set in the effect, plusdisabled={acting}on the close button, would close it cleanly. - Guard also skips
onDecided()on a successful stale action: approve A → open B → A succeeds → the queue never refreshes and keeps showing A as pending. OnlyonClose()needs the record guard; the parent refresh (ReviewQueue.tsx:589-592) is record-agnostic and should still run.ReviewDetailDrawer.test.tsx:96currently pins the over-broad behavior. loadinginSpaceReviewPageis now write-only (five setters, no reader after §2's change) — delete rather than re-wire.saveErrorrenders twice during a failed disable — panel (:156-158) and modal (:197) simultaneously; pick one location.- Failed policy load has no retry affordance (
:161) — a transient GET blip hides the control until remount/space-switch; a retry button besideloadErrorwould restore it. - Pre-existing (reproduces at
92ee6992): the drawer's reject-409 path sets the error then immediately unmounts the modal and re-runs the load effect, whosesetError(null)wipes the message — the reviewer is told nothing.ReviewQueue's own reject path (ReviewQueue.tsx:642-651) is the correct pattern to mirror. - Generation-guard the mutation continuations of §4(c) (cancel/delete/publish on the three list pages) using the pages' existing generation machinery, with delayed-promise space-switch tests — hardening on top of the server-side scoping that already bounds them.
- e2e mock-coverage gap (gate-blocking, infra): the fail-closed gate has tripped on one un-mocked background request per run (attention poll → space categories), each patched by a per-spec Playwright route guard. Consider a catch-all that fulfills known background polls during navigation windows so the gate stops tripping one endpoint at a time, rather than continuing the per-endpoint whack-a-mole.
- Carried from Round 4, still open: the multi-step save/publish chains (
ExpertEditModal/EditSkillModal/NewSkillModal) remain unbound to Space as a whole (server-side scoping backstops them);.skill-market-review-noticestill uses--wk-color-primary, which is not defined in the theme (border silently ineffective); the CSS token cleanup of §4(b); the expert snapshot single-read hardening of §4(a).
None of 1–9 can mutate the wrong record, write into the wrong Space, or lose data; item 8 is e2e test infrastructure.
6. Scope note (yujiwei's Round-4 CR)
The two scope items (frozen-relation rendering, voice e2e retry) were merged commits carried in by the upstream/main merge, not new branch work; the Round-5 delta is unrelated to both. The objection is a process point about diff size, not a code defect, and is not carried as a blocker here.
7. Tests & CI at this head
- Local (Node 22, CI parity):
dmworkskillmarket259/259 (baseline 255 + 4 new),dmworkmcp283/283. Thee202a331/2d2945cfdeltas are e2e-kit-only, so these suites are unaffected by them. - CI on
9271d7ac: Build ✅, install-build ✅, scanners ✅; Unit tests and e2e-p0 still running at the time of writing. History at the prior heads:2d2945cfBuild/Unit/scanners all green, and its e2e run passed 158/158 tests but was blocked by a single Vite proxy error onGET /api/v1/spaces/e2e-space-001/categories(the summary-attention poll thate202a331+2d2945cfguard no longer leaked — those commits did their job; the categories request is the next pre-existing mock-coverage gap, which9271d7acnow guards). Same class of issue throughout: an un-mocked background request escaping during a full-document-navigation window — e2e test infrastructure, not this PR's functional surface (review policy / drawer / contacts), which the suites cover and pass.
Landing order unchanged: #1614 first (its head is a direct ancestor of this head), then this PR.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1624 (octo-web)
Reviewed at head 9271d7ac, against merge-base ca716168 (origin/main's tip is the merge-base, so ca716168..HEAD is exactly this PR: 92 files, +12,952/−796). Round 6.
Delta since the head I last reviewed (7d8e4800): three commits, +32/−0, all e2e/test-infra — e202a331 (relative attention handler in chat-baseline.ts), 2d2945cf (S26 attention route), 9271d7ac (EX2 categories route). No production source changed. I read that delta line by line, then went back over the two areas where I had the least evidence last round: the Space-role authorization gates (this time checked against octo-server source rather than reasoned from the client) and the completeness of the cross-Space hardening in dmworkmcp's modals.
That second pass found two blockers I missed in rounds 1–5. Neither is a regression from the recent fixes — both have been in the diff since the role-encoding and connector-flow commits — and both fixes are small and already patterned elsewhere in this same PR.
1. Spec compliance
Spec: ❌ — one Scope item of issue #1623 is not met, and one explicit PR-body claim is falsified.
Divergence (1) — SpaceSettings is stricter than the server and removes an admin capability
#1623 Scope: "Keep owner/admin role behavior aligned with the authoritative role encoding."
packages/dmworkbase/src/Components/SpaceSettings/index.tsx:154-156
isOwner() { return isSpaceOwner(this.props.space.role); } // role === 2
isAdmin() { return isSpaceAdminOrOwner(this.props.space.role); }isOwner() gates the Space name input (:182), the description textarea and its voice button (:195, :197), the save button (:209) — and disband (:265). One predicate, two different server policies:
handleSave→SpaceService.updateSpace→PUT space/:id(Service/SpaceService.tsx:416-418). Server:octo-server/modules/space/api.go:74routes it toupdateSpace, whose gate ismember.Role < 1 → reject(:372) and whose own doc comment reads "用户侧修改空间基础信息（owner / admin 自服务）" (:353). Admin is explicitly permitted.disbandSpace's gate ismember.Role != 2 → reject. Owner-only.isOwner()is right here.
Before this PR isOwner() was role === 1, which under the corrected encoding is admin — owners were locked out, admins could edit. After it, owners can edit and admins cannot. The disband half is genuinely fixed; the rename half is now wrong in the opposite direction, and the net effect for a Space admin is the silent loss of a working, server-sanctioned action. Neither the PR body nor #1623 mentions narrowing it, and SpaceSettings has no tests (no __tests__ anywhere under it), so nothing else catches this.
Fix: gate name / description / save on this.isAdmin(), keep isOwner() for disband. If owner-only rename is the intended product policy, say so in the PR body and add the test — because the server disagrees.
Divergence (2) — the "stale mutations cannot touch a new Space" claim does not hold for two modals
PR body, COMPREHENSION §2: "Late old-Space requests or mutations could … send old IDs with a new Space header … Generation guards, synchronous clearing, action-continuation checks, and relation-detail tests cover those failures."
They do not cover ExpertEditModal or McpCreateModal. Both contain multi-await submit chains with zero space guard — grep -n 'generation|spaceId|currentSpaceId|abort|useRef' returns nothing in ExpertEditModal.tsx and no guard identifiers in McpCreateModal.tsx. Detail in §2. The sibling ReviewSubmitModal.tsx:116-167 — same PR, same author — carries exactly the guard these two need, commented "Mirrors useSpaceRole's generationRef", which is what makes this an oversight rather than a design choice.
Missing — none.
Both round-4 blockers stay fixed at this head (re-verified at source level, not carried over on trust).
Out of scope — carried, unchanged, still not blocking
Frozen-relation rendering, the voice e2e retry, and this round's three e2e-infra commits are none of them in #1623's scope. Same position as round 5: each addition is individually sound and has now been read across six rounds; the objection was always to bundling them into a 92-file diff, which is a process point, not a defect.
Acceptance
| Criterion | Verdict |
|---|---|
| Owners and admins can view and update the policy | ✅ SpaceReviewPage.test.tsx:52-65 (owner), :117-123 (admin, role 1) |
| Members cannot access review management | MarketSidebar.test.tsx:139,148,202-231 — real, well covered). Not true of the page: SpaceReviewPage.tsx:173 renders <ReviewQueue mode="space" /> outside the isReviewer block, by documented design (:23-26), relying on a server 403 that nothing in this repo or any fixture ever exercises. The source is honest about it; the acceptance wording overstates it. |
| Space switches cannot display stale member/role/policy state | ✅ for every display surface in the PR. See Divergence (2) for mutations. |
| Affected package tests, production build, i18n pass | ✅ ran locally at this head |
Local suites at 9271d7ac, Node 22: dmworkskillmarket 259/259, dmworkmcp 283/283, dmworkcontacts 42/42, dmworkbase 4327/4327, dmworksummary 941/941, apps/web 1426/1426 — all green. CI: Build, install-build and every scanner green; Unit tests and e2e-p0 still queued at the time of writing. PR body leaves Manually verified unchecked.
2. Code quality
Quality: Changes-Requested — two P1. Everything else is P2.
P1-1 — Space admins lose name/description editing
See §1, Divergence (1). Not repeated.
P1-2 — ExpertEditModal.submit and McpCreateModal.submit can issue the second mutation with the new Space's header and the old Space's plugin id
packages/dmworkmcp/src/components/ExpertEditModal.tsx:112-148
if (dirty) {
await updateExpertVisibility(item.id, visibility); // :120
}
if (publish) {
const outcome = await publishPluginListing(item.id, { // :123 — issued after the switchX-Space-Id is attached per request, at issue time, from WKApp.shared.currentSpaceId (api/expertService.ts:180-182, api/mcpService.ts:428-430), and space-changed fires after currentSpaceId flips (Chat/vm.ts:129-130). So a Space switch landing between :120 and :123 sends the new Space's header with the old Space's item.id. The parent does setEditItem(null) on switch (ExpertMarketListPage.tsx:274), which closes the modal — but closing does not cancel the running closure, which captured item at click time. Its continuation then calls onSaved(...) + onClose() (:128-140) → handleListingChanged (ExpertMarketListPage.tsx:576-580) → toast + reload() + myReviews.refresh(): an old-Space outcome announced in the new Space.
McpCreateModal is the same shape with a wider window, because the first stage is not local work:
const content = await buildConnectorReviewContent(editing.id, payload); // :1122 — 2-3 RTTs
await submitPluginReview({ pluginId: editing.id, … }); // :1124(buildConnectorReviewContent fans out to GET /plugins/detail plus category resolution, api/mcpService.ts:984-993 → :897-916.) Three more straddles in the same function: :1065 uploadMcpIcon → :1159 createMcp / :1133 updateMcp; :1133 updateMcp → :1139 publishSaved; :1161 updateMcp → :1166 publishSaved.
Nothing in the suite drives a two-stage mutation across a switch, so this is both the widest hole in the hardening and an untested one.
On severity, plainly: the demonstrated harm is P2 — a misattributed success toast, an error setError'd into a modal the parent already unmounted, and a reload in the wrong Space. What I cannot rule out from this repo is the P1 case: whether these endpoints scope plugin_id against X-Space-Id, i.e. whether an old-Space record actually gets published. This repo's own note for a sibling endpoint says "A cross-Space target is still 404" (skillApiReal.ts, delistPlugin doc), which is encouraging but is about a different endpoint. I am calling it P1 because the PR body asserts this class is covered, the fix is the four-line generationRef pattern already sitting in ReviewSubmitModal.tsx:116-167, and "the server probably rejects it" is not something a review should wave through. Point me at the server-side scope check and I will downgrade it.
P2 — The new S26 attention-poll guard cannot match the request it was written for
apps/web/e2e-kit/tests/summary/S26-summary-standalone-links.spec.ts:8
await pagePlain.route("**/summary/api/v1/summaries/attention?*", async (route) => {The comment says "Intercept the background attention poll at the Playwright boundary." The background poll sends no query string, and this glob requires one.
Playwright puts ? in its escapedChars set, so it is matched literally. Resolved against this repo's own playwright-core@1.61.1:
glob → ^(.*/)summary/api/v1/summaries/attention\?([^/]*)$
.../summaries/attention → false
.../summaries/attention?fresh=1 → true
And the background poll is explicitly the non-fresh caller:
packages/dmworksummary/src/module.tsx:299-302— "后台轮询【不】传 fresh",readSummaryAttentionCount()with no optionspackages/dmworksummary/src/api/summaryApi.ts:650-652—params: options?.fresh ? { fresh: 1 } : undefined
So the interception covers only refreshSummaryAttentionBadge's fresh=1 reads (summaryAttentionBadge.ts:348) and silently misses the poll. Since the e2e gate is fail-closed on proxy errors (.github/workflows/e2e.yml:128,164-166), if e2e-p0 comes back red on summaries/attention, this is why. One-character fix: drop the ? — **/summary/api/v1/summaries/attention* matches both forms (verified).
I have this as P2 rather than P1 on the reasoning that it cannot produce a false green — a guard that does not fire leaves the existing gate to do its job. A second reviewer rated it P1 on the grounds that the commit does not accomplish what it claims; recording the disagreement rather than splitting it.
P2 — The third commit's relative MSW handler may add no coverage at all
apps/web/e2e-kit/msw-handlers/chat-baseline.ts:346-352 adds http.get("/summary/api/v1/summaries/attention", …) beside the existing "*/summary/…" mask, justified as "a leading-wildcard mask does not consistently match in the browser worker." Under the installed MSW 2.15 matcher the existing mask already matches same-origin absolute request URLs and already ignores query parameters, so the twin is a no-op; and if the client has not registered with MSW yet, neither handler can intercept anything. The comment attributes the failure to the wrong matching behaviour. Harmless as code, but it means the actual cause of that leak is still unidentified. (I could not reproduce or refute the wildcard claim empirically — see §6.)
P2 — Both new page.route stubs contradict a documented in-repo decision, and never unroute
apps/web/src/mocks/swControl.ts:29-32 (untouched by this PR) argues against exactly this pattern: "之所以在这里等、而不是在各 spec 里逐个补 page.route 兜底 … 逐个补是打地鼠，而且补的是症状。" This PR then adds two: S26:8-22 and EX2:10-12. Neither is unrouted, so for the rest of each test those endpoints are hard-stubbed above MSW and a real contract drift on spaces/{id}/categories becomes undetectable in EX2. The [] body does match today's baseline fixture exactly (chat-baseline.ts:177-182 returns a bare array, no envelope), so it is behaviour-equivalent right now — it just stops tracking. Relatedly, the same leak class was fixed globally for two endpoints this round but locally, in one spec for spaces/{id}/categories; chat-baseline.ts:177 still has only the bare-wildcard form, so any other spec booting a market page still leaks it.
P2 — ReviewGateProbe fires the 403-ing mode=space read on an owner→member Space switch
packages/dmworkmcp/src/components/MarketSidebar.tsx:186-191, contradicting its own comment at :156-157 ("held back entirely until the role resolves so a plain member never fires the 403-ing mode=space read").
const handleSpaceChanged = () => refreshRef.current();refresh closes over fetchPage, whose enabled is isReviewer from the last render. Both handlers run inside one synchronous mittBus.emit, and React batches setRole, so when the probe refetches, enabled is still the previous Space's role. An owner switching into a Space where they are a plain member issues one mode=space read with the new X-Space-Id. Harmless (the catch sets pendingCount 0 and the sidebar ignores error) but it is one uncovered request per such switch, and the comment claims it cannot happen. Read isReviewer through a ref, or key the refetch off the resolved role.
P2 — Success toasts are unguarded where failure toasts are guarded
packages/dmworkmcp/src/pages/AllAssetsList.tsx guards the error path (:107 if (version !== requestRef.current) return;) but the success toasts sit inside the action() closure, upstream of any check: :206-210 after publishPlugin, :223 after cancelReview, :263 after deleteSkill. An old-Space publish/cancel/delete that succeeds mid-switch still toasts success in the new Space; the new test covers only the reject path (AllAssetsList.test.tsx:133-134). Same class, with no version snapshot at all: McpMarketListPage.tsx:586-604, :660-676; ExpertMarketListPage.tsx:508-521, :548-566, :591-605. Cosmetic plus one wasted refetch — those mutations are single-await, so no header/id mismatch.
P2 — Only one of the three list surfaces clears rows synchronously
AllAssetsList.tsx:86-91 bumps the generation then clears items/error/busyId/deleting before reloading. McpMarketListPage.handleSpaceChanged_ (:254-280) clears the whole modal stack but not items; ExpertMarketListPage.handleSpaceChanged (:269-287) clears every modal target but not myAgentsData / mySquadsData / agentsData / squadsData. In both, the stale rows are hidden by the loading gate (:1053, :870), and I could not construct a user-reachable click on an old-Space row — latent and inconsistent with the sibling, not live. One real asymmetry worth noting: ExpertMarketListPage sets loading:true in the same batch as the handler, while McpMarketListPage sets it in the setState callback (:276), leaving one committed render with old items and loading:false. Intra-task, no paint.
Related: useMyReviewState is refreshed but never cleared (McpMarketListPage.tsx:277-279, ExpertMarketListPage.tsx:286), so stateByPlugin (hooks/useMyReviewState.ts:40) keeps mapping old-Space plugin ids → rejection reasons until the new fetch settles. Space-scoped ids make a collision impossible, so latent only.
P2 — useSpaceRole's re-probe fallback leaves the previous Space's role in state
packages/dmworkskillmarket/src/hooks/useSpaceRole.ts:55-81 — resolveFromServer sets loading true (:64) without clearing role, so isReviewer keeps the old Space's answer for the duration of the probe and MarketSidebar.tsx:118 visible: (gate) => gate.isReviewer keeps the 组织发布管理 row on screen. To be fair to the author: all three production emit sites pass a full Space (Chat/vm.ts:130, Main/index.tsx:186,196), so the numeric-role fast path (:87-93) always wins and this fallback is currently reached only by useSpaceRole.test.ts:130. Worth a setRole(undefined) anyway, since the fallback exists precisely for the payload shape that has no role.
P2 — The policy mapper trusts the wire with no coercion
packages/dmworkskillmarket/src/api/skillApiReal.ts:1332-1336
function mapReviewPolicy(raw: PluginReviewPolicyWire): PluginReviewPolicy {
return { isAutoApproveEnabled: raw.is_auto_approve_enabled, … };
}request<T> is an unchecked cast (:219-224); the only runtime check is "data" in body. Two consequences, traced to the rendered output:
is_auto_approve_enabled: nullpassesenabled !== undefined(SpaceReviewPage.tsx:160) and renderspolicyDisabled= 「已关闭」 — whichi18n/zh-CN.json'spolicyDescriptiondefines as "由组织管理员人工审核". An admin would be told submissions are human-gated while the server may be auto-approving. No error, no warning.- A
204on the PATCH returns{ data: undefined }by design (:203-206), somapReviewPolicy(undefined)throws andSpaceReviewPage.tsx:119renderserr.message— i.e. the literal stringCannot read properties of undefined (reading 'is_auto_approve_enabled').
Both contingent on server behaviour I cannot verify (§6). raw.is_auto_approve_enabled === true plus a genuine error otherwise closes it. Related: review.policyLoadFailed / review.policySaveFailed are unreachable — SkillMarketApiError and TypeError are both Error, so the t() branch at :64/:119 is dead and raw server text shows instead.
P2 — Mutation-proven test holes
Found by flipping the source and re-running the suite, not by reading:
SpaceReviewPage.tsx:151— replacingisReviewer &&withtrue &&leaves all 10SpaceReviewPagetests green. The member test (SpaceReviewPage.test.tsx:125-131) only asserts the checkbox is absent, and the checkbox is separately gated byenabled !== undefined, trivially true for a member because no read fires. A regression leaking the policy heading, description and error text to plain members would ship. (The read-suppression half does have teeth — mutating:94correctly fails.)- Three of the five stale-continuation guards the PR body claims are untested duplicated logic: cancel (
ReviewQueue.tsx:398-414), reject (:632-660), delist (:601-625). Approve (:280-302) and the drawer path (ReviewDetailDrawer.test.tsx:75-99) are covered. McpMarketListPage.spaceIsolation.test.tsxdoes bite on the counter (removingthis.requestVersion += 1at:259fails it), but it doesnew McpMarketListPage()(:80) and stubssetStateas a plain object merge that drops the callback (:81-84), so React never renders andloadData()never runs. It tests the counter, not the page — which is why the batching asymmetry above is invisible to it.MarketSidebar.test.tsx:250-256asserts onlyexpect(h.reviewRefresh).toHaveBeenCalledTimes(1). The badge value comes from apendingCountthe test fully controls, so "the old count left the DOM" was cheap and available.SpaceReviewPage.test.tsx:168-175assertscontainer.querySelectorAll(".skill-market-state")has length 0 — satisfied by a blank render, a renamed class, or a thrown child, andReviewQueueis mocked out entirely (:10-14).plugin_listing_state → pluginListingStatehas zero coverage (onlypluginWire.ts:182andskillApiReal.ts:967mention it), yetReviewQueue.tsx:516gates the 下架 button on it. It is the newest and most confusable field in the mapper.
P2 — The new /plugins/review_requests catch-all is broader than its stated purpose
apps/web/src/mocks/handlers.ts:38-56, registered last (:79). The stated reason (a scenario rendering the market shell but not modelling review leaks the badge probe to the proxy) is legitimate, and I checked the obvious risk: RB1/RB2 both assert toHaveText("1") before acting, which proves the scenario handler is live, so their closing toHaveCount(0) is not made vacuous by it. What does follow: the regex accepts both /api/v1/… and /market/api/v1/…, so a base-path regression is unobservable, and no e2e can ever see a 403 or a wrong mode on the endpoint whose 403 is this feature's only real authorization boundary.
P2 — Role encoding is declared in three places with two different predicates
SpaceService.tsx:233-242— exact equality,role === 1 || role === 2useSpaceRole.ts:9-18— local re-declaration plus a range check,role >= 1 && role <= 2spaceRoleBadge.ts:8,11— bare literals1/2, with the encoding baked into the i18n key names themselves (contacts.role.1/.2)
All three agree for 0/1/2/−1/undefined today, and @octo/base already re-exports the constants (dmworkbase/src/index.tsx:148) — which is how Main/index.tsx:259 consumes them. They diverge the moment the server adds a role ≥ 3.
P2 — Two SpaceMembers gaps this PR passed next to
SpaceMembers/index.tsx:128— the 邀请 button has nocanManageMembersguard, whilecreateInviterequiresRole >= 1(api.go:898), so a plain member gets an error toast. Pre-existing, but this PR introducedcanManageMembers25 lines away, andSpaceSettings:221does gate its invite section.SpaceMembers/index.tsx:156,174— 移除 is offered for admin rows to an admin. Server skipsmember.Role <= target.Roleand stillResponseOK()s (api.go:759-772);handleRemove(:70-78) then drops the row locally and toasts success, and the row returns on the next fetch. A shifted pre-existing bug (before the fix the same false success applied to owner rows), but now in the set an admin will actually click.
P2 — Smaller items
utils/review.ts:25-28—reviewKindLabelhas no default, so any unknownkindrenders 首次发布.reviewStatusLabel(:20-21) andlabels.ts:16-17both fall back to the raw wire value, by the conventionlabels.tsstates explicitly.utils/version.ts—versionErrorKey("1.2.3", "01.02.03")returnsnull(accepted); the string differs from the published label, so the server 409s and the pre-flight validator whose stated purpose is "saves the user a 400" waved it through.nextPatch("1.2.999999999")emits"1.2.1000000000", which its ownisValidVersionrejects — andnextPatchseeds the version field in both modals (NewSkillModal.tsx:122,EditSkillModal.tsx:39) with zero tests of its own.McpCreateModal.tsx:677-694—editingis read inside a[visible]-keyed effect. Thealiveflag is correct and the effect is space-safe; only theeditingread is stale-prone, and I found no live path that changes it whilevisiblestays true. Hygiene.- No async continuation in the
dmworkmcppages checks mounted-ness (AllAssetsList.tsx:62,65,67,104,116;ExpertMarketListPage.tsx:215-244;McpMarketListPage.tsx:386-400,445-455).react@^17still warns on setState-after-unmount. Pre-existing pattern, extended here. - Carried and unaddressed from rounds 4–5:
index.css:3375,3383-3385bypass--wk-sp-*/--wk-r-*;McpMarketListPage.tsx:554openReviewSubmitstill dead;ExpertEditModal.tsx:156onCancel={onClose}with no in-flight guard (now compounded by P1-2);ReviewDetailDrawer.tsx:249-252still prints raw ids; plus the four round-5 drawer P2s (opening-token guard,onDecidedsuppressed on a successful stale action, the 409-reject error never rendered, and the now-deadloadingstate inSpaceReviewPage).
Claims I checked and am NOT raising
Stated so they are not re-litigated:
tis imported inexpertService.ts—:2,import { WKApp, buildAcceptLanguage, t, DEFAULT_REQUEST_TIMEOUT_MS } from "@octo/base". A claim that:631throwsReferenceErroris wrong.- The voice e2e retry has no stale-locator problem.
conversationispage.getByText(...), a lazyLocatorre-resolved on every wait, so it survives the reload. The retry does convert an acknowledged intermittent race into a green test and can hide ~30 s, but the re-throw narrowing at:70is right and no assertion was removed. - No pre-existing e2e assertion was deleted or weakened to reach green. C40's
"删除 X"→"删除「X」"tracks a real aria-label change and staysexact: trueand name-scoped; therole: 1 → 2fixture updates track the encoding remap and flip no gate (both 1 and 2 are reviewer roles). - The encoding itself is right, and the bug it replaced was severe.
octo-server/modules/space/model.go:38:Role int // 成员角色 0.普通成员 1.管理员 2.拥有者.SpaceMembers' oldisAdmin() { role <= 2 }was always true (−1included), so every plain member saw the remove and role-change buttons. That fix is the most valuable thing in this part of the diff.canManageRoles = isSpaceOwneralso correctly matchesupdateMemberRole'smember.Role != 2(api.go:822), andRoleColors' keys now line up. - No caller was left reading the old shape. Repo-wide sweep of
Space.role/SpaceMember.roleconsumers, thecontacts.role.*keys (role.3deleted and unreferenced),_spaceRole(0 hits), andloadAllData's new signature — all updated.ChatSelectorModal.tsx:293and everything underfeatures/channelSetting/useGroupRole, a different enum, correctly untouched. - Both
space-changedemit sites setcurrentSpaceIdbefore emitting (Chat/vm.ts:129-130,Main/index.tsx:182-186), so every synchronous refetch inside a handler already sees the new Space. No ordering bug there. - The sidebar does not double-mount the right pane on a Space switch.
replaceToRootrenders intoqueues:[view]withkey={i}(WKViewQueue/index.tsx:109-121,203-210), so React reconciles rather than remounts — which is what makes the pages' ownspace-changedhandlers load-bearing, and means there is no double-fetch. MyReviewStateProbe'sonChangeeffect does not loop (useMyReviewState.ts:119-121):resultis double-memoized down to[items], andhandleReviewStateChangeis a stable class field.McpCreateModal's rememberedcreatedPluginId(an old-Space id kept for publish retry,:1156-1163) is reset by the prefill effect on reopen (:733, deps[visible, editing]), which commits before any click can land. Not exploitable — this is the one thing that would have made P1-2 a P0.
3. Overall verdict
CHANGES_REQUESTED — on two focused items.
SpaceSettings.isOwner()gating the Space name/description/save controls contradicts the server (updateSpaceadmitsRole >= 1, doc-commented owner / admin 自服务) and #1623's own "keep owner/admin role behavior aligned" scope item, and it removes an action admins could perform before this PR.ExpertEditModal.submitandMcpCreateModal.submitcarry multi-awaitmutation chains with no space guard, which falsifies the PR body's explicit claim that this class is covered — while the siblingReviewSubmitModalin the same PR shows the pattern to copy.
Nothing else here is P0/P1.
I want to be straight about provenance: I approved this same production code at 7d8e4800 last round, and both findings predate the recent fixes. I only caught the first by reading octo-server/modules/space/api.go directly instead of reasoning from the client, and the second by grepping the modals for guard identifiers rather than trusting the PR body's coverage claim.
4. Suggested follow-ups (not merge blockers)
S26-summary-standalone-links.spec.ts:8— drop the?from the glob (or delete the route and fixchat-baseline.tsinstead, perswControl.ts:29-32).MarketSidebar.tsx:186-191— readisReviewerthrough a ref so the space-switch refetch respects the resolved role.skillApiReal.ts:1332-1336— coerceis_auto_approve_enabledwith=== true, raise a real error otherwise, handle the 204 PATCH.AllAssetsList.tsx:206,223,263— move the success toasts behind the samerequestRefcheck the error path already has; add a version snapshot to the five sibling handlers in the two list pages.SpaceReviewPage.test.tsx— assert the policy section is absent for a member, not just the checkbox (the mutation above is the regression test). Tests for the cancel / reject / delist guards, and one forplugin_listing_state.SpaceMembers.tsx:128— gate 邀请 oncanManageMembers;:174— gate 移除 oncanManageRolesfor admin rows, or stop treating 200 as proof.- Import
SPACE_ROLE_*from@octo/baseinuseSpaceRole.tsandspaceRoleBadge.ts. reviewKindLabeldefault;nextPatchoverflow; leading-zero version labels.
Worth its own issue, unchanged from rounds 4–5: no CI job type-checks these packages. I tried to close this myself with a direct tsc -p at this head and the run was environmentally broken (react type resolution, lib target), so I can report it neither as confirmed nor as clean — which is itself the point.
5. Process note — round 6
Round history: 8f3f4e2b → 666c20b6 → 6783714 ×2 → 92ee6992 → 7d8e4800 → 9271d7ac. Severity has fallen monotonically and is still falling: round 3 rendered an unfetched policy value as enabled, round 4 could reject the wrong record, round 5's worst case was a stale queue row, and this round's blockers are a disabled input and a guard-shaped gap in two modals. The delta itself is 32 lines of e2e infra.
Given that, I do not think a seventh full pass is the right shape:
- Land the
isOwner()→isAdmin()decision and the twogenerationRefcopies (plus a test each) and nothing else on this branch. Round 7 should be a ~20-line diff, not another sweep. - Move every P2 in §2 and §4 into a single follow-up issue against the market/review area. There are roughly fifteen, they have been accumulating for three rounds, and none is worth another round-trip on a 92-file branch.
- The deeper cause is unchanged and worth saying once more: a two-line render guard took three rounds because of the size of the diff it lives in. Both of this round's blockers are also size artifacts — one sits in a shared component nobody was looking at, the other in two modals that the hardening pass simply did not reach. Landing the policy panel and the role-encoding correction separately from the connector flow, the frozen-relation feature and the e2e stabilisations would have made rounds 2–6 much cheaper, and is the thing I would change next time rather than anything in this code.
Landing order: #1614 is still open and its head 7d478bac is a direct ancestor of this head. The stated plan — #1614 first, then this — remains correct; please make sure #1614 is closed rather than left to merge as a no-op.
6. Coverage gaps in this review
Stated so they are not read as "checked and clean":
- octo-marketplace was not inspected, unchanged from rounds 4–5. Still unverified: the
/plugin_review_policiesGET/PATCH contract; whether admins may PATCH; whetheris_auto_approve_enabledcan be absent ornull, and whether the PATCH can answer 204 (both gate a P2 above); the documented auto-approve default; whether/plugins/upsert,/plugins/publishand/plugins/reviewsscopeplugin_idagainstX-Space-Id(this is what decides whether P1-2 is a cross-Space write or a 404); whether the status enum iscanceled(as sent verbatim atskillApiReal.ts:1108) orcancelled— if the latter, the 已取消 tab silently returns zero rows forever, and only fixtures written by this same PR say otherwise; whetherfrozen_relationsis[]or absent for relation-less types. SpaceMembers/SpaceSettingshave no in-repo mount site and no tests (repo-wide grep finds only the barrel re-exports atdmworkbase/src/index.tsx:146-147), so P1-1 and the twoSpaceMembersP2s are verified against client source plus octo-server source, not against a running app.- Out-of-repo consumers of the flipped
Space.rolewere not swept — octo-admin / octo-ios / octo-android are separate repos. If any still encodes 1 = owner, this desynchronizes web from them. Worth one grep before merge. Unit testsande2e-p0were still queued on this head, and I ran no Playwright locally. The six vitest suites above are the executed evidence, plus direct input probes ofversion.tsand of Playwright's glob matcher. I could not empirically reproduce or refute the MSW leading-wildcard claim behind thechat-baseline.tstwin.- The bulk of the diff was not re-read this round.
MineTable,NewSkillModal,AllAssetsList's rendering, and theexpertServicevisibility work are byte-identical to7d8e4800and were verified in rounds 1–5; I re-read them only where a finding pointed. - Dark mode verified by token existence, not by rendering.
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Code Review — PR #1624 (octo-web)
Reviewer: Octo-Q (automated review)
Summary
本 PR 把 Space 维度的插件发布审核完整搬上前端：技能 / 连接器 / 专家 / 专家团四个市场统一走 /plugins/review_requests 生命周期（提交、撤回、通过、拒绝）、单一 发布 门（后端按存储的 visibility 决定直接上架还是开审核）、/plugins/publish|delist、以及空间级的自动通过策略开关（本 PR 标题所示）。同时把空间角色编码统一为服务端真实语义（0=member / 1=admin / 2=owner），修复了通讯录角色徽标与成员管理中角色显示/写值的既有错位；新增 全部 聚合 tab、组织发布管理 侧边栏入口（含待审计数徽标）与配套 e2e/MSW 回归守护。整体设计克制：客户端只做展示门（cosmetic gate），授权与状态推导全部留在服务端，display_status 由后端计算、客户端不重推导。审查结论：未发现 P0/P1 阻塞；两条 P2 建议如下。
Verification
- ✅ 静态审查 — 对
pr-base...HEAD（merge-baseca716168，head9271d7ac）全部 92 个变更文件做了逐批审查；本环境未执行构建与测试（Static analysis only at head9271d7ac; build and tests not executed in this environment）。 - ✅ 角色编码上游核实 — 针对本 PR 最关键的语义变更（空间角色 0/1/2），只读核对 octo-server：
modules/space/api_member_search.go:13（spaceRoleAdmin = 1）、modules/space/api_manager.go:742（role=2触发 owner 转让）、updateMemberRole接受 0..2。与本 PR 采用的编码完全一致；旧代码注释（1:owner/2:admin/3:member）为陈旧错误语义。 - ✅ i18n 键一致性 — 脚本化核对三个改动包 en↔zh 键集合完全对齐，且变更文件中引用的 447 个 i18n 键全部可解析（含跨包使用
skillMarket.review.*）。 - ✅ 数据流回溯 — 对每个被消费数据回溯到上游来源，详见下方「Things I checked」。
Findings
无 P0/P1；两条 P2 如下。
P2 — 全部 tab 单页 50 条静默截断，无分页也无截断提示 (packages/dmworkmcp/src/pages/AllAssetsList.tsx:60)
load() 只调用一次 getMySkills({ limit: PAGE_SIZE }, { pluginType: "all" })（PAGE_SIZE=50，:19），随后丢弃了 PagedResult 的 total / nextCursor。拥有 50+ 条跨类型资产的用户会静默看不到后续行——而 全部 正是本次新增、且已设为「我的发布」默认视图、用来回答「还有什么在待审」的入口，待审行完全可能落在第 50 条之后。各类型子 tab 仍有自己的分页，故不构成既有路径回归；建议为 全部 tab 增加 load-more/分页，或至少按 total 渲染一条截断提示。
P2 — 行级 发布 未做版本格式预校验，legacy 标签会撞 400 (packages/dmworkskillmarket/src/pages/SkillListPage.tsx:225)
handlePublish 把行上存储的 skill.version 原样传给 publishPlugin；对 visibility=space 的草稿，该调用经后端 SubmitReview，按 packages/dmworkskillmarket/src/utils/version.ts 自身注释，其以 validVersion 门禁且无 grandfathering。携带格式收紧前标签（1.0 / v1.2.3 / 2.0.0-beta.1——version.ts 明确这类标签在生产存在）的 space 草稿点击行级 发布 必然得到原始服务端 400 toast；packages/dmworkmcp/src/pages/AllAssetsList.tsx:202 的同一动作同病。存在绕行（编辑弹窗内可改出合法版本再发布），不构成死路，但其余提交面均用 versionErrorKey（不带 stored 参数）预先拦截，唯此两处行级入口漏掉。建议行级 发布 前用同一规则预校验，并在校验失败时引导到编辑/升级弹窗。
Things I checked that are fine
- 角色编码联动（本 PR 最高风险点）：
SpaceSettings.isOwner/isAdmin、SpaceMembers的提权/降权写值（旧代码「设为管理员」实际写 2=owner、「取消管理员」写 3 会被服务端 400 拒绝）、Contacts角色徽标（旧代码把 admin 显示为「创建者」、owner 显示为「管理员」）全部按服务端真实编码修正；apps/web/src/Pages/Main/index.tsx:259同步迁移到isSpaceAdminOrOwner。dmworksummary/ChatSelectorModal的既有写法（1/2=管理、0=普通）恰与新编码自洽，无需改动。 - 失效信号对称性（C1）：两个包的全部变更型端点（
deleteSkill/deleteMcp/deleteExpert/deleteSquad、createReviewRequest/approve/reject/cancel、publishPlugin/delistPlugin）均经同一withReviewInvalidation模块实例包裹（含 409 拒绝场景的故意失效）；唯一未包裹的updateReviewPolicy不改变待审计数，正确。 - 空间切换竞态：
useSpaceRole用代际计数而非仅卸载标记；ReviewQueue/SpaceReviewPage/Contacts/AllAssetsList/McpMarketListPage均在space-changed时先作废旧请求与旧 Space 标识再重取；CommunicationShell的无rolepayload 路径经useSpaceRole重查 →roleLoading翻转 →SpaceReviewPageeffect 重新loadPolicy()，策略区不会卡在 loading（逐 emit 点核实过两处发射器的 payload 形状）。 - 授权边界：侧边栏/页面门均为 cosmetic，
mode=space读与决策端点服务端 403/404 强制；深链成员会看到队列自身 403 错误态并被侧边栏迁走；ExpertEditModal对平台发布（system/public）记录拒发可见范围控件，防止租户把官方记录降级进自己空间。 - 冻结快照完整性：容器类型（专家/专家团）提交审核显式携带当前子关系图（含
relation_id与data，防止软删重插丢失member_key/is_leader接线）；升级提交携带新内容（连接器经buildConnectorReviewContent保留未建模附件与服务端键，专家经loadExpertReviewContent逐字回显）；首发提交不带内容、由服务端冻结草稿行——与pluginReview.ts的三值relations语义一致。 - 图标 404 回退：
plugin_icon为原始存储键（非预签名 URL），ReviewQueue/ReviewDetailDrawer均保留onError→ 字母头像回退。 - 申请人内容安全：
ReviewDetailDrawer渲染冻结 SKILL.md 用ReactMarkdown + rehypeSanitize。 - 版本规则镜像：保存面（
/plugins/upsert）带 grandfathering、审核提交面（SubmitReview）不带，EditSkillModal/NewSkillModal/ReviewSubmitModal/McpCreateModal各自按面正确选用versionErrorKey的stored参数；isVersionForward数值比较（1.10.0 > 1.9.0）正确。 - 测试与 e2e：新增 14 个测试文件覆盖 hook/API/页面契约（含
spaceIsolation、pluginDelete.reviewInvalidation、角色编码单测），RB1/RB2/RB3 三个 MSW 场景为有状态实现（能区分「真的重读」与「恰好数字对」）。
Verdict: APPROVED
无 P0/P1：审核生命周期的客户端契约与服务端语义对齐、授权全部留在服务端、空间切换与并发决策的竞态均有代际/中止防护，两条 P2（全部 tab 截断、行级发布的 legacy 版本 400）均为非阻塞的正确性/体验缺口，可在后续跟进。
Corrections and two additions to my review aboveA parallel review pass raised points that made me re-check two things in my own review. One of my statements was wrong and one carried stale citations. Both corrections are below, followed by two findings I had missed. The verdict is unchanged — see the end for why. Correction 1 — I was wrong that the
|
| Claim | Current upstream |
|---|---|
| Route | modules/space/api.go:87 — auth.PUT("/:space_id", s.updateSpace) |
| Doc comment | modules/space/api.go:409 — "updateSpace 用户侧修改空间基础信息（owner / admin 自服务）。" |
| Gate | modules/space/api.go:428 — if member == nil || member.Role < 1 → reject. Admin permitted. |
| Disband gate | modules/space/api.go:594 — member.Role != 2 → owner only |
| Encoding | modules/space/model.go:38 — Role int // 成员角色 0.普通成员 1.管理员 2.拥有者 |
Also confirmed at current upstream, corroborating the encoding from a second angle: modules/space/api_member_search.go:13 — const spaceRoleAdmin = 1, commented "the minimum role allowed to manage space-scoped resources." (That file does not exist in my stale clone, which is why I could not see it earlier.)
So P1-1 is not a disagreement about the encoding — the encoding in this PR is right, and I said so. It is that isOwner() carries two different server policies at once: owner-only for disband (correct) and owner-only for rename (the server says owner or admin). A check that verifies "the encoding was fixed" passes cleanly and still misses this.
Addition 1 — P2: the 全部 tab silently truncates at 50 rows
packages/dmworkmcp/src/pages/AllAssetsList.tsx:19,60
const PAGE_SIZE = 50;
…
const page = await getMySkills({ limit: PAGE_SIZE }, { pluginType: "all" });
if (version !== requestRef.current) return;
setItems(page.items); // `page.total` and `page.nextCursor` discardedConfirmed: grep for nextCursor|loadMore|hasMore|IntersectionObserver|total in that file returns nothing, so there is no load-more and no truncation notice. 全部 is new in this PR and is the default view of 我的发布, and it is the surface a user goes to in order to answer "what else is waiting for review" — a pending row past #50 is invisible. The per-type sub-tabs keep their own pagination, so this is not a regression on an existing path. Add load-more, or at minimum render a truncation hint from total.
Addition 2 — P2: row-level 发布 skips the version pre-check that every other submit surface performs
packages/dmworkskillmarket/src/pages/SkillListPage.tsx:225 and packages/dmworkmcp/src/pages/AllAssetsList.tsx:202-205
const outcome = await publishPlugin({ pluginId: skill.id, version: skill.version });The stored label is passed straight through with no versionErrorKey call. For a visibility=space draft this routes through the review-submit path, and utils/version.ts:76-78 states the rule itself: "OMIT it on the review-submit surfaces. SubmitReview … gates on validVersion with no grandfathering at all." version.ts also documents that pre-tightening labels (1.0, v1.2.3, 2.0.0-beta.1) exist in production. So a space draft carrying a legacy label gets a raw server 400 toast from the row button, while EditSkillModal, NewSkillModal, ReviewSubmitModal and McpCreateModal all pre-validate. There is a workaround (edit the version in the modal first), so it is not a dead end — but these two row entry points are the only submit surfaces that skip the check. Pre-validate and route the user to the edit/upgrade modal on failure.
Note this compounds with Addition 1 in one specific way: on the 全部 tab, both the truncation and the unvalidated 发布 sit on the same rows.
Verdict: unchanged — still CHANGES_REQUESTED
The parallel pass suggested approval, and I have weighed it rather than set it aside. It is right about a great deal (the encoding correction, invalidation symmetry, the frozen-snapshot semantics, the cosmetic-gate design, the plugin_icon fallback, sanitized applicant markdown, and the CommunicationShell payload path where it was right and I was wrong). Neither of my two P1s falls inside what it examined:
- P1-1 — it verified that the role encoding now matches the server, which is true. It did not compare the policy on
isOwner()againstupdateSpace's gate, which is where the admin capability is lost. - P1-2 — its space-switch race review enumerates
useSpaceRole,ReviewQueue,SpaceReviewPage,Contacts,AllAssetsListandMcpMarketListPage. It does not reachExpertEditModal.submitorMcpCreateModal.submit, which are the two multi-awaitmutation chains with no guard at all. (It does mentionExpertEditModal, but about visibility-control gating for platform-published records — a different concern, and correct.)
Both P1s remain small, and the ask is unchanged: the isOwner() → isAdmin() decision for name/description/save, and the generationRef pattern from ReviewSubmitModal.tsx:116-167 copied into the two modals. The two P2s above belong in the same follow-up issue as the rest.
5 participants
Summary
Expose Space review auto-approval controls to authorized owners/admins and harden all affected review/asset UI against cross-Space stale state.
Related Issue
Refs #1623
Changes
Architecture / Module Boundary
dmworkskillmarket,dmworkmcp,dmworkcontacts,dmworkbase, web E2E handlersTesting
Commands:
pnpm --dir packages/dmworkbase test -- --runpnpm --dir packages/dmworkskillmarket test -- --run— 255 testspnpm --dir packages/dmworkcontacts test -- --runpnpm --dir packages/dmworkmcp test -- --run— 279 testspnpm i18n:checkpnpm buildgit diff HEAD^ HEAD --checkExisting React
act(...), Vite dynamic-import/chunk, and third-party lottieevalwarnings remain; all commands pass.Checklist
pnpm i18n:checkCOMPREHENSION
What does this change actually do?
It loads the Space review policy and authoritative role for Publication management, limits policy changes to owners/admins, invalidates all tenant-scoped UI state before loading a new Space, and displays the frozen relation graph included in a review snapshot.
What could break?
Late old-Space requests or mutations could disclose rows, show privileged controls, or send old IDs with a new Space header; omitting frozen relations could let reviewers approve unseen membership changes. Generation guards, synchronous clearing, action-continuation checks, and relation-detail tests cover those failures.
How do you know it works?
Affected package suites, production build, and i18n checks pass. New regressions cover review queue and asset-list Space switches, stale action completion, policy request generations, role visibility, API relation mapping, and relation rendering.