fix(skillmarket): constrain tag overflow and validation by kense-lab · Pull Request #1026 · Mininglamp-OSS/octo-web

kense-lab · 2026-07-23T07:29:59Z

Summary

clamp long Skill card titles and truncate overflowing tag pills
enforce a maximum of 10 tags with 10 characters per tag for new or changed tags
preserve unchanged legacy 11–24 character tags so unrelated edits remain possible
normalize suggested/manual tags before duplicate checks
show a platform-official badge for public skills identified by the existing frontend publisher signal

Confirmed product decisions

The 10-character tag limit is intentional and synchronized with octo-marketplace and octo-cli.
Existing long tags are grandfathered only while unchanged; newly added or changed tags must satisfy the 10-character contract.
The platform-official presentation deliberately reuses the existing frontend identification logic for now; no publisher API change is included.

Tests

pnpm --filter @dmwork/skillmarket exec vitest run src/components/tests/NewSkillModal.test.tsx src/components/tests/EditSkillModal.test.tsx src/utils/format.test.ts
pnpm --filter @octo/web build

Fixes #1017

Superseded: re-posting same APPROVE verdict with repo-relative paths (removed local absolute paths).

Jerry-Xin

This focused Skill Market fix is ready to merge; no blocking correctness, security, performance, or architectural issues found.

💬 Non-blocking

🔵 Suggestion: Call validateSkillTags(tags) defensively inside both submit handlers at packages/dmworkskillmarket/src/components/NewSkillModal.tsx:362 and packages/dmworkskillmarket/src/components/EditSkillModal.tsx:368, keeping submission validation independent of disabled-button behavior.
🔵 Suggestion: Add direct unit tests for boundary cases in packages/dmworkskillmarket/src/utils/format.ts (validateSkillTags), particularly exactly 10 vs 11 tags and 24 vs 25 Unicode characters.

✅ Highlights

validateSkillTags validates parser-provided and legacy tags, not merely manually entered drafts, so pre-existing invalid tags now block saving.
Invalid legacy data remains visible and removable while saving is blocked (canSave/canCreate gate on !tagSubmitError).
Tag pills preserve the removal icon while safely truncating text via skill-market-tag-input__text; tags render as escaped React text nodes (no dangerouslySetInnerHTML), so no XSS surface.
Card titles are constrained to two lines (-webkit-line-clamp: 2) without allowing the version badge to shrink.
Focused modal tests cover both excessive tag counts and oversized legacy tags; i18n keys tagLimit/tagLengthLimit/tagInvalidChars exist in both en-US and zh-CN.

Note: the added tests could not be executed in this checkout because the test runner binary was unavailable.

mochashanyao

[Octo-Q · automated review]

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

Code Review — PR #1026 (octo-web)

Summary

This PR addresses skill-market tag overflow in two ways: (1) raises the tag cap from 5 to 10, and (2) adds array-level validation (validateSkillTags) so that legacy skills loaded with excess or over-length tags correctly block save until the user trims them. The companion CSS work clamps card titles to two lines and adds ellipsis truncation inside tag pills via a new skill-market-tag-input__text span. Tests cover both the new limit and the legacy-overflow guard.

Verification

✅ Validation chain correctness — tagSubmitError = tagError ?? validateSkillTags(tags) ?? getTagDraftError() chains server error → array-level validation → draft validation in correct priority. Traced through EditSkillModal.tsx:131 and NewSkillModal.tsx:99; both paths identical.
✅ MAX_SKILL_TAGS propagation — all 10+ consumption sites in both modals and format.ts import the constant; no hardcoded 5 remains. The 5→10 change propagates uniformly.
✅ Legacy data flow — setTags(skill.tags) (EditSkillModal.tsx:90) and setTags(status.result.tags) (both modals) can set >10 tags from server data. validateSkillTags correctly flags this and blocks save. Tests confirm (EditSkillModal.test.tsx:167-198).
✅ INPUT-SHAPE edges — validateSkillTags([]) returns null (safe). validateSkillTag("") returns null via early if (!tag) guard at format.ts:82 (pre-existing). Array.from() in tagLength handles multi-byte correctly. Callers always pass string[] state, never null.
✅ CSS overflow chain — tag button: max-width: 140px + overflow: hidden → span: min-width: 0 + overflow: hidden + text-overflow: ellipsis + white-space: nowrap → SVG: flex: 0 0 auto (prevents icon shrink). Card title: all three -webkit-line-clamp prerequisites present (display: -webkit-box, -webkit-box-orient: vertical, overflow: hidden). align-items: flex-start on title-row correctly pairs with 2-line clamp.
✅ Parity between Edit/New modals — both modals received identical changes to the validation chain, tag rendering ( wrap), aria attributes, and hint display.

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

Findings

No P0/P1 issues. No correctness, security, build, or functional concerns. The validation logic is sound, CSS changes are well-structured, and tests cover the key legacy-data paths.

Things I checked that are fine

validateSkillTags short-circuits on first per-tag error, so the user sees the first invalid tag's message — no risk of error-message shadowing.
getTagDraftError() uses tags.length >= MAX_SKILL_TAGS (prevents adding at limit) while validateSkillTags uses tags.length > MAX_SKILL_TAGS (flags excess). Different comparisons for different purposes; both correct.
tagsFromInput (format.ts:63) has no consumers in the codebase — dead export, but pre-existing and not touched by this PR.
New EditSkillModal tests cover both overflow-tag count (11 tags → blocked) and over-length legacy tag (64-char tag → blocked). NewSkillModal test updated from 5→10 tag fixtures.
Card title CSS: -webkit-line-clamp: 2 requires display: -webkit-box and -webkit-box-orient: vertical; all three present. text-overflow: ellipsis and white-space: nowrap correctly removed (clamp handles truncation).

Verdict: APPROVED

Clean, well-scoped fix. Validation correctly handles both user-driven and legacy-server-data paths. CSS overflow handling follows standard flex-truncation patterns. Tests cover the new contract.

[Octo-Q] verdict: APPROVE — no P0/P1 findings; clean PR.

yujiawei

Code Review — PR #1026 (octo-web)

Summary

This PR constrains skill-market tag overflow via two mechanisms: (1) raising MAX_SKILL_TAGS from 5 to 10, and (2) adding an array-level validateSkillTags guard so that legacy skills loaded with an excess count or over-length tags block save until trimmed. Companion CSS clamps card titles to two lines and truncates tag pills with ellipsis via a new skill-market-tag-input__text span. Tests cover the new limit and the legacy-overflow guard.

Spec compliance

The change is well-scoped to the stated goal (constrain tag overflow + validation). No functionality added beyond scope; no requirements left unaddressed.

Missing: none
Extra/out-of-scope: none
Deviation: none

Verification (independent, static at head `3887faeaa13305c32e329b3bdc9cfcdbbe0dc419`)

Validation chain — tagSubmitError = tagError ?? validateSkillTags(tags) ?? getTagDraftError() chains server error → array validation → draft validation in correct priority. Confirmed identical in EditSkillModal.tsx:131 and NewSkillModal.tsx:99.
Save gating — canSave/canCreate both include !tagSubmitError, and the primary button is disabled={!canSave} / disabled={!canCreate} (EditSkillModal.tsx:416, NewSkillModal.tsx:425). So a blocking tag error genuinely prevents submit, not just shows a hint.
Legacy data path — setTags(skill.tags) (EditSkillModal.tsx:90) and setTags(status.result.tags) (both modals) can seed >10 or over-length tags from server data with no silent truncation. validateSkillTags flags both; tests confirm (EditSkillModal.test.tsx).
Limit ordering — validateSkillTags checks tags.length > MAX_SKILL_TAGS first (→ tagLimit) then per-tag validateSkillTag (→ tagLengthLimit/invalidChars). Test fixtures match: 11 tags → tagLimit; single 64-char tag → tagLengthLimit.
5→10 propagation — every consumption site imports the constant; a repo grep found no residual hardcoded 5 tag limit.
i18n — tagLimit / tagLengthLimit / tagInvalidChars / tagDuplicate all present in both en-US.json and zh-CN.json.
Edge inputs — validateSkillTags([]) returns null; validateSkillTag("") short-circuits via the if (!tag) guard; tagLength uses Array.from for correct multi-byte counting.
CSS overflow chain — tag button (max-width:140px + overflow:hidden) → text span (min-width:0 + ellipsis + nowrap) → SVG (flex:0 0 auto prevents icon shrink). Card title has all three -webkit-line-clamp:2 prerequisites (display:-webkit-box, -webkit-box-orient:vertical, overflow:hidden); align-items:flex-start on the title row correctly pairs with the 2-line clamp.
Edit/New parity — both modals received identical validation-chain, span-wrap, aria-describedby, and hint changes.

Note: build and unit tests were not executed in this environment; review is static analysis of the diff and surrounding code.

Findings

No P0/P1 issues — no correctness, security, build, or functional concerns.

Non-blocking observations (nits, optional):

tagsFromInput (format.ts:63) remains an unused export — pre-existing, not touched by this PR.
-webkit-line-clamp for the card title is well-supported in the target Chromium/WebKit browsers; if a non-WebKit fallback is ever needed it would degrade to no truncation, but that is outside this PR's scope.

Verdict

APPROVED — clean, well-scoped fix. Validation correctly handles both user-driven and legacy-server-data paths, save is genuinely gated (not just a visual hint), and tests cover the key contract.

lml2468

Review: #1026 — fix(skillmarket): 约束标签溢出 + 校验(#1017)

意图:钳制过长 Skill 卡片标题、截断溢出标签 pill;create/edit 强制最多 10 标签 × 每标签 24 字符;旧超量标签集保持可读但阻断非法保存。

锚定 head 3887faea / merge-base 3ce01400(behind 4,mergeable=MERGEABLE,0 冲突)。6 文件 +78/−21。

Gate 1 — 规格符合度 ✅

MAX_SKILL_TAGS 5→10、MAX_SKILL_TAG_LENGTH 24;新增 validateSkillTags(数量 + 逐标签 validateSkillTag)。✅
CSS:卡片标题 -webkit-line-clamp:2 + .skill-market-tag-input__text 省略号截断。✅
i18n tagLimit/tagLengthLimit/tagInvalidChars en/zh 双语齐备。✅

Gate 2 — 代码质量 ✅

校验落在真实保存路径(重点):tagSubmitError = tagError ?? validateSkillTags(tags) ?? getTagDraftError() → canCreate/canSave 含 !tagSubmitError → 驱动提交按钮 disabled。非法的旧/解析器提供的标签(不只手输草稿)也阻止保存 —— 新测试坐实。✅
码点计数正确:validateSkillTag 用 tagLength = Array.from(value).length(按码点,非字节)+ unicode 正则 /^[\p{L}\p{N} _./#+-]+$/u —— CJK/emoji 标签正确按字符计,不误伤。✅
XSS 安全:标签渲染 {tag} React 文本自动转义,两个 Modal 零 dangerouslySetInnerHTML。✅
无 #1007 SkillCard 布局回归:仅新增 scoped 选择器。✅

构建 / 测试(实跑):

pnpm --filter @octo/web build ✅ built in 3.40s。
NewSkillModal + EditSkillModal ✅ 2 files / 21 tests 全绿,含新增「安全渲染 legacy 溢出标签并阻断保存」「阻断 legacy 超 24 字符标签保存」用例。✅

🟡 非阻断(承 Jerry-Xin)

命令式 submit() 处理器仍主要依赖按钮 disabled 门控;虽 canCreate 已含 !tagSubmitError,建议在 submit 入口再防御性 validateSkillTags 一次(防未来有人绕过按钮直调)。因当前按钮门控 + canCreate 已封住,不阻断。

校验落真实保存路径(含 legacy)、码点计数正确接受 unicode、XSS 安全、无布局回归、i18n 齐备,构建与 21 测试通过。可合并 —— APPROVE。

cnwenf

Code Review — PR #1026 (Mininglamp-OSS/octo-web)

Consolidated from three independent reviews (octo-review-qwen3.7, octo-review-glm5.2, octo-review-qwen3.8). Findings are deduplicated; [source: ...] marks which reviewer(s) raised each finding. The three full raw reviews remain as comments on this issue for audit.

Summary

This PR widens the Skill Market tag limit from 5 to 10 (MAX_SKILL_TAGS) and adds a batch validateSkillTags gate — count > 10, per-tag length > 24, and the SKILL_TAG_PATTERN charset — wired into canSave / canCreate so that legacy oversized/over-length tag sets stay viewable (truncated pills, 2-line card-title clamp) but block invalid saves in both the edit and create modals, backed by new vitest cases and a couple of CSS touches. All three independent reviewers confirm the runtime contract is correct and load-bearing under mutation testing, the 10/11 and 24/25 (incl. CJK codepoint) boundaries are right, and security is clean (0 findings). The gaps are about enforcement and completeness of the new gate, not the happy path: the new tests never run in CI, the create flow lacks a failing blocking/recovery test, and the batch gate misses duplicate- and empty-tag input shapes. One cross-repo concern (server-side tag-cap alignment) was raised and then falsified by inspecting octo-server.

Findings

P1 — New tag-contract tests never run in CI (`.github/workflows/ci.yml:92`) [source: octo-review-qwen3.8 + octo-review-qwen3.7]

The vitest cases that lock the new tag contract — the legacy overflow/over-length block at EditSkillModal.test.tsx:167-197 and the ten-tag limit at NewSkillModal.test.tsx:207-232 — are not executed by any CI workflow. A structured parse of all 20 .github/workflows/*.yml files (51 job steps) finds zero steps invoking vitest / pnpm test / turbo run test; ci.yml's Build job runs only install / i18n:check / build / lint / lint:wkmodal. The infrastructure is ready but unused — turbo.json defines a test task and @dmwork/skillmarket has "test": "vitest run", so a root pnpm test would run this package; CI simply never calls it. As a result every regression guard this PR adds merges unenforced, and a regression of validateSkillTags would merge green — which also amplifies the two coverage gaps below. Add the skillmarket vitest run to a required CI job (a pnpm test step in ci.yml). Independently flagged by octo-review-qwen3.7, which notes the gap is pre-existing infrastructure debt not introduced by this PR but amplified by it (adds tests that CI never executes, creating false confidence). (severity split: qwen3.7=P1, qwen3.8=P2)

P2 — Create flow has no failing test for over-limit/over-length blocking (`packages/dmworkskillmarket/src/components/tests/NewSkillModal.test.tsx:213`) [source: octo-review-qwen3.8 + octo-review-glm5.2]

The rewritten create-flow case seeds exactly 10 tags — the boundary, where format.ts:93 returns tagLimit only when tags.length > MAX_SKILL_TAGS, so 10 passes — and asserts only the hint text, never that the Create button is disabled or that createSkill is not called. The pollParse auto-fill path (setTags(status.result.tags), NewSkillModal.tsx:244) is unvalidated and unbounded, so an 11-tag or >24-char parse result reaches the gate; the edit flow has two blocking cases (EditSkillModal.test.tsx:167-197) but the create flow has none. A mutation removing validateSkillTags from canCreate leaves all 9 existing NewSkillModal tests green. Add a pollParse→11-tags (or >24-char) case asserting the button is disabled, a click does not call createSkill, and deletion recovers with a valid payload. octo-review-glm5.2 independently flagged the same pollParse→setTags overflow path as having no direct test (originally a nit). (severity split: qwen3.8=P2, glm5.2=nit)

P3 — Batch gate accepts empty/whitespace-only tags (`packages/dmworkskillmarket/src/utils/format.ts:97`) [source: octo-review-qwen3.7 + octo-review-qwen3.8]

validateSkillTag short-circuits empty to null (if (!tag) return null, format.ts:81-82), and normalizeTags (skillApiReal.ts:252-266) keeps "" array elements, so legacy data can seed tags=["", …]. The gate then reports all-valid, renders an empty pill (EditSkillModal.tsx:554 / NewSkillModal.tsx:556), and ships the empty string into the update/create payload — a hole in the PR's stated "block invalid saves" goal. Confirmed by a probe (three empty/whitespace targets fail; four controls pass). Have validateSkillTags reject empty/whitespace entries (or drop them from the payload), and trim/drop empties in normalizeTags.

P3 — Batch gate skips the duplicate-tag check (`packages/dmworkskillmarket/src/utils/format.ts:92`) [source: octo-review-qwen3.8]

validateSkillTags checks count and per-tag length/pattern but not duplicates, while the per-add path rejects duplicates via tags.includes(next) (EditSkillModal.tsx:126,213; NewSkillModal.tsx:94,295). Legacy/server data carrying duplicate tags therefore passes the batch gate → tagSubmitError null → save allowed → duplicates submitted, a PR-introduced asymmetry. Confirmed by a probe whose four count/length/pattern controls pass while the duplicate target fails. Either add a duplicate check to validateSkillTags for parity, or document that duplicates are intentionally permitted for legacy data.

P3 — Edit-flow tests miss the recovery closure and click-not-called (`packages/dmworkskillmarket/src/components/tests/EditSkillModal.test.tsx:180`) [source: octo-review-qwen3.8]

The two new edit-flow cases assert the blocked state (toBeDisabled + hint) but never click save to assert updateSkill is not called (the adjacent invalid-chars case at :163-164 does), and never cover the "until they are valid" recovery: delete the over-limit/over-length tag → save re-enables → click → updateSkill called with a valid ≤10-tag payload. A no-op mutation of the tag-delete handler (setTags(tags.filter(...))) leaves all 12 existing tests green. Add the recovery-closure and click-not-called assertions.

Nit — Save path guards on `tagError || draftError` but omits the new `validateSkillTags` (`packages/dmworkskillmarket/src/components/EditSkillModal.tsx:369`, `NewSkillModal.tsx:373`) [source: octo-review-glm5.2]

The diff adds validateSkillTags(tags) to the tagSubmitError chain (EditSkillModal.tsx:131, NewSkillModal.tsx:99), which feeds canSave/canCreate and the button's disabled flag — this is what actually blocks the save. But the submit() body still guards only if (tagError || draftError) and never re-checks validateSkillTags(tags). So the "block invalid saves" invariant lives entirely in the disabled-button derivation, not at the point of mutation. Today this is safe because the disabled button is the sole entry point and there is no <form onSubmit>; the gap is latent only if a future change adds another save trigger (e.g. Cmd+Enter, a footer action). Not a change I'd block on — for defense-in-depth, consider if (tagError || validateSkillTags(tags) || draftError) { … return; } so the guard mirrors tagSubmitError.

Warning — dead helper `tagsFromInput` drifted silently (non-blocking) (`packages/dmworkskillmarket/src/utils/format.ts:68`) [source: octo-review-qwen3.8]

tagsFromInput (format.ts:63-69) has zero callers anywhere in packages/ — dead code whose .slice(0, MAX_SKILL_TAGS) silently widened 5→10 with the constant bump, truncating rather than surfacing overflow, in tension with the explicit-block contract this PR establishes. This could not be confirmed or falsified by any executable test at HEAD: no production path reaches it, and the stated harm is a hypothetical future re-introduction. Consider deleting it or aligning it to surface overflow explicitly. Not a merge blocker for this PR.

Things I checked that are fine

Tag gate is load-bearing — 21/21 author vitest pass plus probes; mutation unwiring validateSkillTags from tagSubmitError fails tests and reverting MAX_SKILL_TAGS 10→5 fails 7, so the tests enforce the contract rather than passing tautologically.
Boundary + codepoint behavior — 10 tags accepted / 11 blocked; 24-char accepted / 25 rejected; 24 CJK codepoints valid / 25 invalid; count-error takes precedence over per-tag error.
Recovery path — delete an over-limit / over-length seeded tag → save re-enables → updateSkill / createSkill called with a valid (≤10, no long tag) payload.
Silent truncation in submit — falsified: getTagDraftError() returns tagLimit when tags.length >= MAX with a pending draft, so submit() returns before the .slice(0, MAX); the slice is only reached when tags < MAX and can never drop a tag. Explicit-block semantics preserved.
Submit-handler gate — submit() is reachable only via the native disabled button (no <form onSubmit>); over-count (11/12/13 tags) and over-length (30/42/47-char) seeds disable the button and the API mock is not called, while valid tags pass through the same path.
Sibling sinks — SkillCard pills already truncate (max-width:108px, ellipsis, title, CARD_VISIBLE_TAG_LIMIT=3 with a +N counter); SkillDetailModal and its .skill-market-detail__tags CSS are byte-identical at base and head (pre-existing, not diff-causal).
Frontend/backend tag-limit parity — raised as a concern by octo-review-qwen3.7 (server-side cap alignment) but falsified by inspecting octo-server (HEAD f9fd79b): of 511 registered routes the only skill/market/tag hit is GET /v1/bot/skill.md (a markdown doc); there is no skill create/update endpoint, no Tags []/json:"tags" field, and no tag count/length validation — no server-side constraint for this diff to diverge from. octo-review-glm5.2 likewise notes client-side-only validation is pre-existing awareness, not a blocker. Resolved.
CSS clamp wiring — the 2-line -webkit-line-clamp title clamp and pill truncation are wired correctly (min-width:0 flex item, fixed-width icon, line-clamp:2); pixel rendering itself cannot be runtime-verified in jsdom (no layout engine) and was verified structurally (classes/props present, CSS compiles into the production build).
Build & types — pnpm --filter @octo/web build EXIT 0 with no warnings from PR-changed files; no new type errors in the changed files.
i18n — tagLimit / tagLengthLimit keys exist in both zh-CN.json and en-US.json; updated test regexes match the {{count}}-interpolated strings.
Security posture — tag fields enter only React-escaped text nodes / title attributes (no dangerouslySetInnerHTML/eval/URL sinks); SKILL_TAG_PATTERN is a single anchored character class, linear time, no ReDoS; credential scan clean; no dependency/lockfile/config changes.

Verification Results

Check	Verdict	Key Finding
Code Review	NEEDS_CHANGES	1 P1 + 1 P2 + 3 P3 + 1 nit + 1 warning (deduped across 3 reviewers); 1 cross-repo P2 (server cap) falsified by octo-server source. Two P1/P2 must-fix items: wire skillmarket vitest into a required CI job, add a create-flow blocking/recovery test
E2E Test	PASS (CSS pixel rendering PARTIAL)	Functional/runtime contracts verified at head `3887fae` across reviewers (21/21 author tests + probes + mutation controls, build EXIT 0), no confirmed E2E failures; frontend-only change — full service stack N/A; CSS pixel rendering structurally verified only (jsdom has no layout engine)
Security	CLEARED	0 findings across all three reviewers: React-escaped sinks only, linear-time tag regex (no ReDoS), clean credential scan, no dep/config changes

Final Verdict: NEEDS_CHANGES

Jerry-Xin

The PR is relevant to octo-web and correctly constrains Skill Market tag validation and UI overflow without introducing blocking issues.

✅ Highlights

Centralized collection validation in format.ts:92.
Existing invalid/legacy tags remain removable while saves are blocked.
Create and edit flows consistently validate parsed and existing tags.
Tag pills and card titles now handle overflow safely.
Tests cover oversized legacy data, recovery, and validation.
Skill Market tests are now included in CI.

Note: Tests could not be executed locally because dependencies are not installed (vitest: command not found); static review found no blocking concerns.

mochashanyao

[Octo-Q · automated review]

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

Code Review — PR #1026 (octo-web)

Reviewer: Octo-Q (automated review)

Summary

This PR tightens tag validation in the Skill Market create/edit flows: MAX_SKILL_TAGS is raised from 5 to 10, a new validateSkillTags() batch validator catches legacy oversized or malformed tag sets, and both modals now surface that validation in the submit guard and the visible hint area. Card titles switch from single-line ellipsis to 2-line -webkit-line-clamp, and tag pill buttons gain a truncating  with title tooltip for overflow. CI picks up a dedicated Skill Market test step.

Overall a clean, well-tested change. The two modals are kept in lock-step parity and the new validator has unit coverage. Two non-blocking items below.

Verification

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

✅ Modal parity — EditSkillModal.tsx and NewSkillModal.tsx carry identical structural changes: import validateSkillTags, tagSubmitError formula, submit() guard,  tag render, and aria-describedby / hint wiring.
✅ validateSkillTags contract — checks count > MAX_SKILL_TAGS, empty-after-trim, trimmed-duplicate via Set, then delegates per-tag to validateSkillTag (length + pattern). Unit tests in format.test.ts cover empty, whitespace-only, duplicate-after-trim, and valid-tag acceptance.
✅ CSS clamp correctness — .skill-market-card__title-row h3 now has display: -webkit-box + -webkit-box-orient: vertical + -webkit-line-clamp: 2 + overflow: hidden, which is the correct combination for multi-line clamp on modern browsers (Chrome, Firefox 68+, Safari).
✅ CI integration — pnpm --filter @dmwork/skillmarket test runs vitest run (confirmed in package.json), which discovers all *.test.ts(x) files including the changed ones.
✅ No XSS surface — title={tag} and {tag} text content are React-escaped; no raw HTML injection.

Findings

No P0/P1 issues. One P2 item and one nit below.

P2 — `submit()` validates `tags` state but not `submittedTags` (`packages/dmworkskillmarket/src/components/EditSkillModal.tsx:370`, `packages/dmworkskillmarket/src/components/NewSkillModal.tsx:374`)

In both modals' submit(), validateSkillTags(tags) checks the state array, but submittedTags (a few lines later) can be [...tags, tagDraft.trim()]. The draft is separately validated by getTagDraftError(), which uses tags.includes(next) — exact-match dedup, not trimmed-set dedup.

Diff-scope: New — validateSkillTags is introduced in this PR. The old code had no batch validator at all, so this is an improvement, not a regression.

Reachable scenario: If the server stores an untrimmed tag (e.g. " foo " from a pre-trim era or direct API write) and the user types "foo" as draft, tags.includes("foo") is false, validateSkillTags([" foo "]) finds no duplicate, and submittedTags becomes [" foo ", "foo"] — a trimmed-duplicate that neither check catches.

Consequence: Low likelihood (requires server-side untrimmed data), and the server may deduplicate on its side. The submitted array would carry a cosmetic duplicate.

Fix direction: Validate submittedTags instead of tags in the submit path, or align getTagDraftError to use trimmed-set dedup consistent with validateSkillTags.

Nit — `validateSkillTags` passes untrimmed value to `validateSkillTag` (`packages/dmworkskillmarket/src/utils/format.ts:103`)

validateSkillTag(tag) receives the original (potentially untrimmed) value from the loop. validateSkillTag trims internally, so this works correctly today, but passing trimmed would make the contract explicit and prevent a future refactor from accidentally dropping the trim.

Data Flow Trace

Consumed data	Upstream source	Reaches consumption?
`tags` (state array)	EditSkillModal: `skill.tags` via `useEffect` (`:100`). NewSkillModal: `status.result.tags` from `pollParse` (`:244`).	✅ Flows to `validateSkillTags(tags)`, `getTagDraftError()`, render, and `submittedTags`.
`tagSubmitError`	Computed each render: `tagError ?? validateSkillTags(tags) ?? getTagDraftError()`.	✅ Flows to `canSave`/`canCreate` (button disable), `aria-describedby`, and hint display. New in this PR: hint area now shows `validateSkillTags` and `getTagDraftError` results, not just `tagError`.
`submittedTags`	Constructed in `submit()`: `[...tags, tagDraft.trim()].slice(0, MAX_SKILL_TAGS)` or `tags`.	✅ Flows to `updateSkill`/`createSkill` API call. Not passed through `validateSkillTags` (see P2 above).
`MAX_SKILL_TAGS` (10)	`format.ts:75` — changed from 5 to 10.	✅ Consumed by `tagsFromInput` (slice), `addTagValue` (cap), `getTagDraftError` (limit check), `validateSkillTags` (count check), `submit` (slice). All consistent.

Blind-spot Checklist

C1 (dual-path parity): EditSkillModal ↔ NewSkillModal — ✅ Clear. Both modals carry identical changes (import, tagSubmitError formula, submit() guard, tag , aria/hint wiring).
C2 (control-flow ordering): N/A — no reused logic called from multiple ordering contexts.
C3 (auth boundary): N/A — no auth/permission changes.
C4 (lifecycle cascade): N/A — no auth/container-state changes.
C5 (build ≠ runtime): N/A — CSS changes use well-supported properties; no build-time transformation risk.
C6 (governance/docs): N/A — no policy or documentation changes.

Cross-round Blocker Recheck

N/A — first review of this PR.

Verdict: APPROVED

No P0/P1 issues. The P2 (submit-path validation gap for combined submittedTags) is low-likelihood and non-blocking — the PR is a clear improvement over the prior state which had no batch validation at all. The nit is cosmetic. Tests cover the new validator and both modals' overflow/legacy-tag blocking behavior.

yujiawei

Code Review — PR #1026 (octo-web)

Summary

This PR tightens Skill Market tag handling in the create/edit flows:

MAX_SKILL_TAGS raised 5 → 10.
New validateSkillTags() batch validator catches legacy oversized / malformed / duplicate tag sets loaded from the server, wired into both modals' submit guard and the visible hint area.
Card titles switch from single-line ellipsis to a 2-line -webkit-line-clamp.
Tag pills gain a truncating  with a title tooltip so overflowing tags no longer break the layout.
A dedicated Skill Market test step is added to CI.

Clean, well-scoped change with good test coverage. The two modals are kept in strict parity. Approving — no blocking issues; two non-blocking items noted below.

Verification

Checked out at head c8f8a48315caaf75a33e17ff70e3ba44af5addb6 and ran the package test suite locally:

✅ Tests pass — pnpm --filter @dmwork/skillmarket test: 13 files, 120 tests passed, including the new validateSkillTags unit tests and both modals' legacy-overflow / 11-tag-blocking / recovery tests.
✅ i18n keys exist — tagLimit, tagLengthLimit, tagInvalidChars, and the newly-referenced tagDuplicate are all present in both zh-CN.json and en-US.json. No missing-key risk from validateSkillTags.
✅ Modal parity — EditSkillModal.tsx and NewSkillModal.tsx carry identical structural changes: validateSkillTags import, tagSubmitError formula, submit() guard, tag  render, and aria-describedby / hint wiring driven by tagSubmitError.
✅ CSS clamp — .skill-market-card__title-row h3 uses the correct display:-webkit-box + -webkit-box-orient:vertical + -webkit-line-clamp:2 + overflow:hidden combination; parent switched to align-items:flex-start so a 2-line title aligns correctly.
✅ Tag pill truncation — button (max-width:140px, min-width:0, overflow:hidden) + .skill-market-tag-input__text (ellipsis) + svg { flex:0 0 auto } keeps the remove icon visible while the label truncates. Sound.
✅ No XSS surface — title={tag} and {tag} are React-escaped text; no raw HTML.

Findings

No P0/P1 issues.

P2 — `submit()` validates `tags` but not the combined `submittedTags`

EditSkillModal.tsx:369 / NewSkillModal.tsx:373 call validateSkillTags(tags), but the payload built a few lines later is submittedTags = tagDraft.trim() ? [...tags, tagDraft.trim()].slice(0, MAX_SKILL_TAGS) : tags. The pending draft is validated separately by getTagDraftError(), which dedups with exact-match tags.includes(next) rather than the trimmed-set logic used by validateSkillTags.

Scope: New — validateSkillTags is introduced here; the prior code had no batch validator, so this is a net improvement, not a regression.
Reachable scenario: only if the server stores an untrimmed tag (e.g. " foo ") and the user types "foo" as the draft — tags.includes("foo") is false and validateSkillTags([" foo "]) finds no duplicate, so submittedTags becomes [" foo ", "foo"].
Consequence: cosmetic duplicate in the submitted array; low likelihood (requires pre-existing untrimmed server data) and the backend may dedup anyway.
Fix direction: validate submittedTags in the submit path, or align getTagDraftError's dedup with validateSkillTags's trimmed-set comparison.

Nit — `validateSkillTags` passes the untrimmed value to `validateSkillTag`

format.ts:102 passes the original tag (not trimmed) to validateSkillTag. It works today because validateSkillTag trims internally, but passing trimmed would make the contract explicit and guard against a future refactor dropping the internal trim.

Blind-spot Checklist

Dual-path parity (Edit ↔ New): ✅ identical changes in both modals.
Control-flow ordering: N/A.
Auth boundary / lifecycle: N/A — no auth or container-state changes.
Build ≠ runtime: verified — ran the real test suite, not just static read; CSS uses well-supported properties.
Cross-file side effects: validateSkillTags is the only new export from format.ts and is consumed only by the two modals; no other callers affected.

Verdict: APPROVED

No P0/P1 issues. The P2 (submit-path validation gap on the combined draft) is low-likelihood and non-blocking, and the change is a clear improvement over the prior state which had no batch validation. Tests are green locally at the head SHA.

lml2468

Re-review: #1026 @ `0ac3b36d` — skillmarket 标签约束

复核相对上一 head c8f8a48(cnwenf 三条 🔴 已修的版本)的增量。锚定 head 0ac3b36d / merge-base 3ce01400(behind 6,mergeable=MERGEABLE,0 冲突)。

真实增量(vs c8f8a48):MAX_SKILL_TAG_LENGTH 24→10、detail-tags CSS 单行截断→多行 wrap、两测试同步更新。

结论:可合并 ✅

本轮增量正确 ✅

MAX_SKILL_TAG_LENGTH 24→10(产品收紧):tagLengthLimit i18n 用 {{count}} 占位 → 10 自动流经文案(en/zh 齐全,无硬编码数字);两测试 blocks saving a legacy tag longer than 10 characters 同步更新 —— 一致,非假绿。✅
detail-tags CSS:flex-wrap: wrap + overflow-wrap: anywhere + white-space: normal,去掉单行 ellipsis/nowrap —— 标签多行完整显示不裁剪。rgba(127,59,245,.06) 为 var(--wk-accent-tint-06, fallback) 兜底(全文件既有一致写法),无裸硬编码/!important。✅

cnwenf 三条 🔴 + save-gating 承前无回归 ✅

空白/重复过滤(format.ts trim + Set → tagInvalidChars/tagDuplicate)、CI 跑 pnpm --filter @dmwork/skillmarket test(ci.yml 1 处)、11-tag 阻断+恢复测试 —— 全部完好。
码点计数(Array.from)、unicode 正则、tagSubmitError→canCreate/canSave save-gating、XSS-safe {tag} —— 无回归。

构建 / 测试(实跑):

pnpm --filter @octo/web build ✅ built in 3.24s。
format + NewSkillModal + EditSkillModal ✅ 3 files / 25 tests 全绿。

🟡 非阻断(承前)

submit() 未校验合并后 submittedTags、可考虑把 channelType……(不适用此 PR);mochashanyao/yujiawei 提的 submit 防御性校验仍可选 —— canCreate 含 !tagSubmitError + validateSkillTags 已 trim/去重,功能正确,不阻断。

本轮 MAX_LENGTH 收紧 + tags 多行 CSS 均正确、测试同步、cnwenf 三修与 save-gating 无回归,构建与 25 测试通过。可合并 —— APPROVE。

mochashanyao

[Octo-Q · automated review]

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

Code Review — PR #1026 (octo-web)

Summary

This PR tightens the Skill Market tag system on two fronts: it raises the tag-count cap from 5 to 10 while lowering the per-tag character limit from 24 to 10, and it adds a new validateSkillTags() helper that validates the entire tag array (count, empty/whitespace, duplicates, per-tag length/chars) at both render-time and submit-time in the New and Edit skill modals. Tag-chip overflow in the UI is addressed with CSS text-overflow ellipsis, flex-wrap on detail-view tags, and a two-line clamp on card titles. A CI step is added to run the Skill Market test suite on every push.

Overall this is a clean, well-tested change. The validation logic is correct, both modals are kept in parity, and the tests exercise the production path (parse-result overflow, legacy tag rendering, recovery after tag removal). No P0/P1 issues found.

Verification

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

✅ validateSkillTags correctness — Handles count overflow (tags.length > MAX_SKILL_TAGS), empty/whitespace-only (!trimmed), duplicates (trimmed Set), and delegates to validateSkillTag for per-tag length/chars. Returns null for valid arrays. All edge paths verified.
✅ Modal parity — Both EditSkillModal.tsx and NewSkillModal.tsx apply validateSkillTags(tags) in tagSubmitError and in the submit handler with identical logic (tagError ?? tagsError ?? draftError priority chain).
✅ Data flow — parse result → state — setTags(status.result.tags) does not slice, so parse results with >10 tags land in state as-is. validateSkillTags catches this via the count check, blocking the save button and showing the limit error. Verified in both modals (EditSkillModal:328, NewSkillModal:244).
✅ Data flow — legacy skill → edit modal — setTags(skill.tags) at EditSkillModal:90 loads whatever the server stored. Tags exceeding the new 10-char limit or the 10-tag count are caught by validateSkillTags at render time via tagSubmitError.
✅ Submit handler safety — submittedTags = [...tags, tagDraft.trim()].slice(0, MAX_SKILL_TAGS) is guarded upstream: if tags.length >= MAX_SKILL_TAGS and draft exists, getTagDraftError() returns a limit error and blocks submission. The .slice() is a redundant safety net that never fires in practice.
✅ i18n consistency — tagLimit, tagLengthLimit, tagInvalidChars, tagDuplicate keys all exist in both en-US.json and zh-CN.json. Test regexes match the rendered i18n output with the new constants.
✅ CSS overflow — Tag button uses inline-flex with max-width: 140px, overflow: hidden; text span has min-width: 0; text-overflow: ellipsis; white-space: nowrap; SVG icon is flex: 0 0 auto. Detail tags use flex-wrap: wrap with overflow-wrap: anywhere. Card title uses -webkit-line-clamp: 2 with the required display: -webkit-box and -webkit-box-orient: vertical.
✅ CI step — pnpm --filter @dmwork/skillmarket test matches the package name @dmwork/skillmarket in package.json.

Findings

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

P2 — Server-side tag limit alignment (`packages/dmworkskillmarket/src/utils/format.ts:71`)

The frontend now allows up to 10 tags (MAX_SKILL_TAGS=10) and 10 characters per tag (MAX_SKILL_TAG_LENGTH=10), changed from the previous 5/24. If the backend (octo-server) still enforces the old limits, create/update requests carrying 6–10 tags will pass frontend validation but fail at the API boundary, producing a confusing server error. The frontend change is correct on its own, but confirm the server-side validation has been (or will be concurrently) updated to accept the new limits.

Nit — Non-standard `-webkit-line-clamp` without fallback (`packages/dmworkskillmarket/src/index.css:735`)

Card title uses -webkit-line-clamp: 2 without the standard line-clamp: 2. Works in all current browsers but is technically a vendor-prefixed property. Consider adding line-clamp: 2 as a progressive-enhancement line.

Things I checked that are fine

validateSkillTag("") returns null (early-exit for empty), which is intentional — validateSkillTags handles the empty case separately with its own !trimmed check before calling validateSkillTag.
tagsFromInput slices to MAX_SKILL_TAGS, so paste-input paths are already bounded.
The tagSubmitError variable replaces tagError in aria-describedby and hint rendering, so the UI shows the full-array validation error (e.g., "too many tags") even when no per-draft error exists. This is the correct behavior for legacy overflow tags.
Test coverage: 11-tag parse result + recovery after removal (NewSkillModal), legacy overflow tag rendering + save block (EditSkillModal), validateSkillTags unit tests for empty, whitespace, duplicates, and valid input.

Data Flow Trace

Consumer	Source	Verified
`tagSubmitError` (render)	`tagError ?? validateSkillTags(tags) ?? getTagDraftError()`	✅ All three branches produce correct strings or null
`canSave`/`canCreate`	`!tagSubmitError`	✅ Blocks button when any validation error exists
`submit()` guard	`tagError \|\| tagsError \|\| draftError`	✅ Catches all error sources; `setTagError` persists for UI
`submittedTags`	`[...tags, tagDraft.trim()].slice(0, MAX_SKILL_TAGS)`	✅ Safe — upstream guards prevent over-count
Tag chip rendering	`<span title={tag}>{tag}</span>` inside button	✅ `title` provides full text on hover; CSS truncates visually

Blind Spot Checklist

C1 (Dual-path parity) — Edit and New modals apply identical validation logic. Both import validateSkillTags and wire it the same way. Clear.
C2 (Control-flow ordering) — validateSkillTags is called in two places per modal: once at render time (in tagSubmitError) and once in the submit handler. Both call sites use the same tags state. No ordering issue.
C3–C6 — N/A. No auth, container-lifecycle, build/packaging, or governance changes in this PR.

Cross-Round Blocker Re-Review

N/A — first review of this PR.

[Octo-Q] verdict: APPROVE — No P0/P1 issues. P2 (server-side limit alignment) is a cross-repo concern, not a frontend blocker. Tests cover the production paths. Modal parity confirmed.

Jerry-Xin

Re-review APPROVED at head 0ac3b36da696 (prior verdict APPROVE at c8f8a48315ca). Clean single follow-up commit; all tag-handling behavior verified byte-level, PR-relevant tests pass.

True delta (`c8f8a48` → `0ac3b36`)

ahead 1 / behind 0, merge-base c8f8a48315ca — no rebase, no bloat. Exactly ONE new commit: fix(skillmarket): wrap full tag labels.
Files: packages/dmworkskillmarket/src/utils/format.ts, packages/dmworkskillmarket/src/index.css, plus the two modal test files (assertion updates).

What this commit changes

format.ts: MAX_SKILL_TAG_LENGTH reduced 24 → 10 (codepoint limit). i18n message uses {count} interpolation, so the length-limit text auto-reflects 10.
index.css .skill-market-detail__tags / span: replaced ellipsis truncation with full-label wrapping (flex-wrap: wrap, overflow-wrap: anywhere, white-space: normal, max-width: 100%). Presentation-only.
Test files: length-limit assertions updated 24 → 10 to match source.

🔴 Blocking

None.

💬 Non-blocking notes

On the two prior 🟡 raised by peers at c8f8a48:
- submittedTags-merge validation: already mitigated at the reviewed baseline (not changed by this commit). submit() in both NewSkillModal.tsx and EditSkillModal.tsx runs validateSkillTags(tags) AND getTagDraftError() before building submittedTags; getTagDraftError() rejects a pending draft tag that is invalid, duplicate of an existing tag, or over the max count — so a bad tag merged via the submit-time draft path is blocked.
- trim passthrough into per-tag validation: still passes the raw value, but validateSkillTag() trims internally on its first line, so length/charset checks already run on the trimmed value. Functionally safe; the suggestion remains cosmetic.
Reducing the max tag length to 10 is a deliberate product/UI decision (tests updated accordingly), not a code defect.

✅ Highlights / verified no-regression

MAX_SKILL_TAGS=10, codepoint counting via Array.from, unicode \p{L}\p{N} charset pattern — unchanged and correct.
Save-gating chain intact: tagSubmitError → canSave/canCreate → button disabled.
Duplicate + blank tag rejection in validateSkillTags (trim + Set) intact.
XSS-safe render preserved: {tag} in both modals and {tag} text child in the detail view (React auto-escaped; no raw HTML injection).
CSS wrap change is presentation-only; with the shorter 10-codepoint limit, tags wrap cleanly.
Tests: NewSkillModal (10), EditSkillModal (12), format (3) all pass at this head. The 36 failures observed elsewhere are isolated to skillApiReal.test.ts (a pre-existing test-environment issue with localStorage in setup, last modified in #851, untouched by this PR) — not a regression from #1026.
No #1007-style regression.

# Conflicts: # .i18n/scan-config.json

yujiawei

Code Review — PR #1026 (octo-web)

Reviewed at head 6c7fece56a82b0c626775859c7de688e39a139b3 against main (merge-base 3ce01400). Repo checked out locally; pnpm --filter @dmwork/skillmarket test runs green (123/123). This review uses a two-stage gate: scope/spec first, then code quality.

1. Scope & spec compliance — ❌

Linked issue #1017 asks for three UI fixes: (1) card title/slug/author-name overflow, (2) edit-modal tag-pill overflow, (3) a consistent tag-count limit (the hint text and the actual validation disagreed). The overflow work (SkillCard, tag pills, detail-header wrapping) is on-target and well done.

However the PR also bundles changes that #1017 does not ask for:

Tag max-length reduced 24 → 10 (packages/dmworkskillmarket/src/utils/format.ts:72, MAX_SKILL_TAG_LENGTH). The issue complained the limit hint was inconsistent with validation — it did not ask to shrink the allowed length. Shrinking it is a data-contract change (see P1 below).
A whole new "Official" publisher-attribution feature (src/utils/publisher.ts + ShieldCheck badges in SkillCard.tsx / SkillDetailModal.tsx + new i18n key card.platformPublisher). This is a new product surface, not an overflow fix.
Detail-modal owner visibility behavior changed: SkillDetailModal.tsx:237 now shows publisher metadata for public skills that were previously hidden (showOwner = skill.visibility !== "public" → isPlatformPublished || Boolean(singlePublisherName)), and drops the @ name prefix. The tests were rewritten to accept the new behavior, but #1017 does not request it.

Raising MAX_SKILL_TAGS 5 → 10 is arguably adjacent to the "consistent limit" ask, but the length reduction and the Official-publisher feature are out of the stated scope. Please either split the publisher-attribution feature into its own spec'd PR, or reference the product decision that authorizes these changes here.

2. Code quality — Changes requested

P1 — Reducing tag max-length locks editing of legacy skills

MAX_SKILL_TAG_LENGTH drops 24 → 10, and both modals now validate the entire existing tag set on save via validateSkillTags(tags) (EditSkillModal.tsx:128,369-375, NewSkillModal.tsx:99,372-376). Tags between 11 and 24 characters were valid under the prior limit and can exist on production skills. After this change, opening such a skill and editing any unrelated field (e.g. fixing a description typo) leaves the Save button disabled until the user shortens or deletes those tags — the PR's own EditSkillModal.test.tsx test ("blocks saving a legacy tag longer than 10 characters") codifies this lockout as intended.

Recommend one of: (a) keep the 24 limit unless there's an explicit product/migration decision to change the contract; or (b) grandfather existing tags — only enforce the new length on tags the user newly adds or edits in this session, so unrelated edits to legacy skills remain saveable.

P2 — "Official" badge derives trust from a spoofable display name

isPlatformPublishedSkill (publisher.ts:15-16) classifies a skill as platform-published when visibility === "public" and (spaceId === "" or the creator/owner display name equals the hardcoded literal "超级管理员" / "Super Admin"). Deriving a trust badge from a user-facing display name is fragile: it is locale-dependent (only these two exact strings), and it couples a trust signal to a mutable name field rather than an immutable role/publisher ID.

Exploitability is currently bounded — the create flow hardcodes visibility: "space" (NewSkillModal.tsx:396) and the edit modal does not expose visibility, so a normal user cannot self-assign public; public visibility is backend-controlled. So this is not an immediate privilege issue, but it is brittle. Prefer an explicit publisher/role identifier from the API contract; until one exists, gate the badge on the spaceId === "" global-scope signal alone and drop the name-literal branch. (Human-verify: confirm the backend never returns visibility: "public" for user-published skills, and whether an authoritative publisher-type field can be added.)

P2 — Insertion vs. final validation disagree on duplicate normalization

addTagValue rejects duplicates with a raw tags.includes(next) exact-string match (NewSkillModal.tsx:295, EditSkillModal.tsx equivalent), while validateSkillTags deduplicates on tag.trim() (format.ts:97-100). A draft like " foo " differs from "foo" by the raw check and can be inserted, then immediately fail the trimmed set check — the field flips to an error the user cannot easily explain. Normalize (trim) once at insertion and store/compare the normalized value consistently.

Notes on other candidate findings (verified, not blocking)

Owner-name null crash — (skill.creatorName || skill.ownerName).trim() in publisher.ts:15 is defensive-only: ownerName is a required string in types/skill.ts:22, and the function early-returns unless visibility === "public". Adding || "" is a cheap safety net but not a live crash given the type contract.
submittedTags validation bypass — not a bug: submit already runs getTagDraftError(), which checks the pending draft for duplicate / length / count-limit before it is appended to submittedTags, so a draft that would exceed the limit or duplicate an existing tag is blocked.
Detail-header truncation "silently fails on inline span" — not a bug: .skill-market-detail-header__owner is a  child of .skill-market-detail-header__meta, and .skill-market-detail-header__meta span { display: inline-flex } applies to it, so the new > span / > svg flex + ellipsis rules operate in a real flex context.
tagDuplicate i18n key missing — not a bug: the key is present in both zh-CN.json:128 and en-US.json:128.

3. Overall verdict — CHANGES_REQUESTED

The overflow/wrapping work is solid and the tests pass, but the PR (a) bundles out-of-scope changes (the Official-publisher feature and the tag-length reduction) beyond what #1017 requests, and (b) the tag-length reduction is a functional regression that blocks editing of existing skills with legacy 11–24-char tags. Please address the P1 (grandfather legacy tags or retain the 24 limit) and split/justify the out-of-scope additions; the two P2 items are worth cleaning up in the same pass.

lml2468

Re-review: #1026 @ `f69c1b3e` — fix(skillmarket): constrain tag overflow and validation

复核新 head(触发消息里的 6c7fece5 已被 f69c1b3e 取代,我实际复核当前 live head f69c1b3e)。锚定 head f69c1b3e / merge-base 001fb5aa(= base_sha,已 rebase)。16 文件 +302/−44。规格来源:issue #1017。

结论:需修改 — REQUEST_CHANGES ❌

溢出/布局修复本身做得对,但 PR 悄悄把单标签字符上限从 24 收紧到 10(issue 未要求、破坏既有 Skill 编辑),并夹带了一个 issue 之外的「官方发布者」功能。

✅ 满足 #1017 的部分(显示/布局)

CSS:min-width:0 + text-overflow:ellipsis + flex-wrap:wrap + 标题 clamp + 作者名 max-width —— 对应 #1017 问题 1(卡片标题/slug/作者名溢出)与问题 2(标签 pill 溢出、换行)。
标签数量上限提示统一:tagSubmitError + tagLimit/tagLengthLimit i18n —— 对应 #1017「校验与文案一致」。
构建 ✅;本 PR 自带 5 个测试文件(format + New/Edit/Card/Detail)53 tests 全绿。(全量跑有 1 个 api-service 测试文件失败,但它不在本 PR 改动范围,失败原因是 Node-26/jsdom localStorage undefined 的既知环境噪声,非本 PR 引入。)

🔴 阻塞 1 — 单标签字符上限被悄悄从 24 降到 10(deviation + 回归) `format.ts:72`

base(001fb5aa)本为 MAX_SKILL_TAG_LENGTH = 24,本 PR 改成 10;连测试断言都从「单个标签最多 24 个字符」改成「10」——是明知在改一条既有 24 限制。
issue #1017 从未要求收紧单标签字符上限(它只要单标签显示省略号 + 标签数量上限一致)。
具体回归:validateSkillTags 门控提交(EditSkillModal tagSubmitError 参与提交可用条件)。任何在旧 24 限制下创建、含 11–24 字符标签(如 productivity=12、development=11 及大量英文标签)的既有 Skill,进编辑弹窗改任何字段都会被这条校验挡住 → 必须先截断本不想动的旧标签才能保存。这与本 PR 自称「keep legacy oversized tag sets readable」相悖(可读 ≠ 可编辑)。
平台一致性:MCP 标签用 MAX_MCP_TAG_LENGTH = 24;降到 10 也破坏一致性。
修:恢复 MAX_SKILL_TAG_LENGTH = 24(数量 5→10 更宽松、无回归,可保留);若确要收紧,需产品签字 + 既有超长标签的迁移/兼容方案。

🔴 阻塞 2 — 与 main 合并冲突(承 peer;当前 head 可能已修)

peers 在旧 head 6c7fece5 标记 merge 冲突。当前 f69c1b3e 的 merge-base == base_sha,像是已 rebase;但 GitHub mergeable 仍为 UNKNOWN(计算中)。合入前请确认 mergeable 变 CLEAN。

🟡 非阻断

范围外功能(over-build):本 PR 夹带了「平台官方发布者」标识(utils/publisher.ts、isPlatformPublishedSkill、ShieldCheck 徽章、platformPublisher 文案),与 #1017(溢出/布局)无关,建议拆成独立 PR。
该功能用硬编码管理员名作为发布者身份字面值(Jerry-Xin/mochashanyao)—— 应改为后端 API 字段下发。

溢出/布局与数量-限制一致性满足了 #1017,构建与本 PR 自带 53 测试通过;但单标签字符上限被悄悄从 24 降到 10(issue 未要求、会挡住既有含长标签 Skill 的编辑、与 MCP 24 不一致)构成回归,须修;另有范围外的官方发布者功能 + 硬编码管理员名建议拆分/去硬编码。需修改 —— REQUEST_CHANGES。

lml2468 · 2026-07-24T07:16:57Z

-export const MAX_SKILL_TAGS = 5;
-export const MAX_SKILL_TAG_LENGTH = 24;
+export const MAX_SKILL_TAGS = 10;
+export const MAX_SKILL_TAG_LENGTH = 10;


🔴 回归:此值在 base(001fb5a)为 24,本 PR 悄悄降到 10(测试断言也从「单个标签最多 24 个字符」改成 10)。issue #1017 只要求单标签显示省略号 + 标签数量上限一致,并未要求收紧单标签字符上限。后果:validateSkillTags 门控提交(EditSkillModal 的 tagSubmitError),任何在旧 24 限制下创建、含 11–24 字符标签的既有 Skill,在编辑弹窗里改任何东西都会被这条校验挡住,必须先删/截断本不想动的旧标签才能保存——与本 PR「keep legacy oversized tag sets readable」相悖(可读 ≠ 可编辑)。且平台 MCP 标签用的是 MAX_MCP_TAG_LENGTH = 24,降到 10 也破坏一致性。

建议:恢复 MAX_SKILL_TAG_LENGTH = 24(数量 5→10 更宽松、无回归,可保留);若确要收紧到 10,需产品明确签字 + 给出既有超长标签的迁移/兼容方案。

lml2468

Re-review: #1026 @ `f69c1b3e` — fix(skillmarket): constrain tag overflow and validation

复核新 head(触发消息里的 6c7fece5 已被 f69c1b3e 取代,我实际复核当前 live head f69c1b3e)。锚定 head f69c1b3e / merge-base 001fb5aa(= base_sha,已 rebase)。16 文件 +302/−44。规格来源:issue #1017。

结论:需修改 — REQUEST_CHANGES ❌

溢出/布局修复本身做得对,但 PR 悄悄把单标签字符上限从 24 收紧到 10(issue 未要求、破坏既有 Skill 编辑),并夹带了一个 issue 之外的「官方发布者」功能。

✅ 满足 #1017 的部分(显示/布局)

CSS:min-width:0 + text-overflow:ellipsis + flex-wrap:wrap + 标题 clamp + 作者名 max-width —— 对应 #1017 问题 1(卡片标题/slug/作者名溢出)与问题 2(标签 pill 溢出、换行)。
标签数量上限提示统一:tagSubmitError + tagLimit/tagLengthLimit i18n —— 对应 #1017「校验与文案一致」。
构建 ✅;本 PR 自带 5 个测试文件(format + New/Edit/Card/Detail)53 tests 全绿。(全量跑有 1 个 api-service 测试文件失败,但它不在本 PR 改动范围,失败原因是 Node-26/jsdom localStorage undefined 的既知环境噪声,非本 PR 引入。)

🔴 阻塞 1 — 单标签字符上限被悄悄从 24 降到 10(deviation + 回归) `format.ts:72`

base(001fb5aa)本为 MAX_SKILL_TAG_LENGTH = 24,本 PR 改成 10;连测试断言都从「单个标签最多 24 个字符」改成「10」——是明知在改一条既有 24 限制。
issue #1017 从未要求收紧单标签字符上限(它只要单标签显示省略号 + 标签数量上限一致)。
具体回归:validateSkillTags 门控提交(EditSkillModal tagSubmitError 参与提交可用条件)。任何在旧 24 限制下创建、含 11–24 字符标签(如 productivity=12、development=11 及大量英文标签)的既有 Skill,进编辑弹窗改任何字段都会被这条校验挡住 → 必须先截断本不想动的旧标签才能保存。这与本 PR 自称「keep legacy oversized tag sets readable」相悖(可读 ≠ 可编辑)。
平台一致性:MCP 标签用 MAX_MCP_TAG_LENGTH = 24;降到 10 也破坏一致性。
修:恢复 MAX_SKILL_TAG_LENGTH = 24(数量 5→10 更宽松、无回归,可保留);若确要收紧,需产品签字 + 既有超长标签的迁移/兼容方案。

🔴 阻塞 2 — 与 main 合并冲突(承 peer;当前 head 可能已修)

peers 在旧 head 6c7fece5 标记 merge 冲突。当前 f69c1b3e 的 merge-base == base_sha,像是已 rebase;但 GitHub mergeable 仍为 UNKNOWN(计算中)。合入前请确认 mergeable 变 CLEAN。

🟡 非阻断

范围外功能(over-build):本 PR 夹带了「平台官方发布者」标识(utils/publisher.ts、isPlatformPublishedSkill、ShieldCheck 徽章、platformPublisher 文案),与 #1017(溢出/布局)无关,建议拆成独立 PR。
该功能用硬编码管理员名作为发布者身份字面值(Jerry-Xin/mochashanyao)—— 应改为后端 API 字段下发。

溢出/布局与数量-限制一致性满足了 #1017,构建与本 PR 自带 53 测试通过;但单标签字符上限被悄悄从 24 降到 10(issue 未要求、会挡住既有含长标签 Skill 的编辑、与 MCP 24 不一致)构成回归,须修;另有范围外的官方发布者功能 + 硬编码管理员名建议拆分/去硬编码。需修改 —— REQUEST_CHANGES。

lml2468 · 2026-07-24T07:17:08Z

-export const MAX_SKILL_TAGS = 5;
-export const MAX_SKILL_TAG_LENGTH = 24;
+export const MAX_SKILL_TAGS = 10;
+export const MAX_SKILL_TAG_LENGTH = 10;


🔴 回归:此值在 base(001fb5a)为 24,本 PR 悄悄降到 10(测试断言也从「单个标签最多 24 个字符」改成 10)。issue #1017 只要求单标签显示省略号 + 标签数量上限一致,并未要求收紧单标签字符上限。后果:validateSkillTags 门控提交(EditSkillModal 的 tagSubmitError),任何在旧 24 限制下创建、含 11–24 字符标签的既有 Skill,在编辑弹窗里改任何东西都会被这条校验挡住,必须先删/截断本不想动的旧标签才能保存——与本 PR「keep legacy oversized tag sets readable」相悖(可读 ≠ 可编辑)。且平台 MCP 标签用的是 MAX_MCP_TAG_LENGTH = 24,降到 10 也破坏一致性。

建议:恢复 MAX_SKILL_TAG_LENGTH = 24(数量 5→10 更宽松、无回归,可保留);若确要收紧到 10,需产品明确签字 + 给出既有超长标签的迁移/兼容方案。

Dismissing my own accidental duplicate submission (double-posted this same turn). The identical REQUEST_CHANGES verdict stands in review #4771049028.

yujiawei

Code Review — PR #1026 (octo-web)

Re-review at head b33cc478fc4d2e889d66a8837dad4c8280db5e95 (new head since my prior review at 6c7fece5; base main, merge-base 001fb5aa). Repo checked out locally; pnpm --filter @dmwork/skillmarket test runs green (125/125).

What the new commit fixed

b33cc478 "normalize tags before duplicate checks" addresses the duplicate-normalization P2 from the previous round: addTagValue now trims before validating/storing, getTagDraftError and the insertion dedup check use tags.some((tag) => tag.trim() === next), and two new tests (trims suggested tags before checking duplicates) cover it. Good fix.

Still open — the blocking items are unchanged at this head

P1 — Reducing tag max-length locks editing of legacy skills (unchanged)

MAX_SKILL_TAG_LENGTH is still 10 (was 24) in format.ts:72, and both modals still validate the entire existing tag set on save via validateSkillTags(tags). Tags 11–24 chars long were valid under the prior limit and can exist on production skills; after this change, opening such a skill and editing any unrelated field leaves Save disabled until the user shortens/deletes those tags. The PR's own EditSkillModal.test.tsx:184 ("blocks saving a legacy tag longer than 10 characters") still codifies this lockout as intended.

Recommend: keep the 24 limit unless there is an explicit product/migration decision, or grandfather existing tags (enforce the new length only on tags the user newly adds/edits this session) so unrelated edits to legacy skills stay saveable.

Scope — the out-of-scope additions are unchanged

Issue #1017 asks for three UI overflow fixes. Still bundled beyond that scope: (a) the tag max-length reduction 24→10 (data-contract change, see P1); (b) the new "Official/官方发布" publisher-attribution feature (publisher.ts + ShieldCheck badges + card.platformPublisher i18n); (c) the SkillDetailModal owner-visibility change that now shows publisher metadata for public skills previously hidden. Please split the publisher-attribution feature into its own spec'd PR, or reference the product decision that authorizes it here.

P2 — "Official" badge derives trust from a spoofable display name (unchanged)

isPlatformPublishedSkill (publisher.ts:16) still classifies via visibility === "public" AND (spaceId === "" OR the creator/owner name equals the hardcoded literal "超级管理员" / "Super Admin"). Deriving a trust badge from a mutable, locale-dependent display name is brittle. Exploitability stays bounded (the create flow hardcodes visibility: "space" and the edit modal doesn't expose visibility, so public visibility is backend-controlled), so this is not an immediate privilege issue — but prefer an immutable publisher/role identifier from the API; until one exists, gate the badge on spaceId === "" alone and drop the name-literal branch. (Human-verify: backend never returns visibility: "public" for user-published skills.)

Note on the first-pass advisory

The automated first-pass suggested APPROVED, but it is anchored to the stale old head 6c7fece5 and does not account for the P1 legacy-tag lockout or the scope concerns. Its two P2 observations align with mine (one now fixed). Terminal verdict below governs.

Overall verdict — CHANGES_REQUESTED

The overflow work is solid and the duplicate-normalization fix is good, but the tag-length reduction remains a functional regression that blocks editing of existing skills with 11–24-char tags, and the out-of-scope feature additions are still bundled. Address the P1 (grandfather legacy tags or retain 24) and split/justify the out-of-scope additions.

kense-lab · 2026-07-24T07:26:56Z

产品决策确认（用于本轮 review 判定）：\n\n1. 单标签上限从 24 收紧为 10 是明确要求，不是意外回归。服务端与 CLI 已同步为“最多 10 个标签、每个最多 10 个字符”：\n - Marketplace PR #19: https://github.com/Mininglamp-OSS/octo-marketplace/pull/19\n - CLI PR #101: https://github.com/Mininglamp-OSS/octo-cli/pull/101\n Web 保留 24 或 grandfather 11–24 字符标签会与当前服务端写入契约不一致；旧超限标签需要用户删除或缩短后再保存，这是本次收紧契约的预期行为。\n\n2. “平台官方”发布者标识也是本次明确要求。当前阶段明确不改后端，先复用现有前端识别逻辑完成样式；后续可再用稳定 API 字段替换识别逻辑，但不作为本 PR 阻塞项。\n\n3. PR 描述已同步记录以上两项 Confirmed product decisions。请基于这些已确认要求判断实现正确性，不要按 issue #1017 的原始范围撤回这两项功能。

Jerry-Xin

Re-review at head b33cc47 under the amended spec: the tag cap and the platform-publisher reuse are now accepted, but editing skills that carry legacy (pre-cap) tags is still blocked, so this cannot merge as-is.

Spec items now resolved (previously raised, now dropped)

Tag cap = 10 is accepted. packages/dmworkskillmarket/src/utils/format.ts sets MAX_SKILL_TAG_LENGTH = 10, which now matches the updated requirement in #1017. My earlier "24→10 deviation" blocker is withdrawn.
Platform-official publisher badge is sanctioned. Reusing the existing identification path is now in scope, so the prior scope-creep objection is dropped (see one trust note below).

🔴 Blocking

Legacy-tag edit is blocked (data-compat regression). In packages/dmworkskillmarket/src/components/EditSkillModal.tsx, the editor is seeded with the skill's existing tags (setTags(skill.tags)), then validateSkillTags(tags) gates saving in two places: canSave (via tagSubmitError) and the submit() handler (const tagsError = validateSkillTags(tags)). validateSkillTags in format.ts validates every tag through validateSkillTag, which rejects any tag longer than 10 chars, with no grandfather/exemption for pre-existing tags. Result: a user opening a skill that already has an 11–24-char tag created under the old 24-char cap (e.g. development = 11, productivity = 12) is blocked from saving any unrelated edit (description, version, icon) until they manually delete or truncate a tag that was valid when it was saved. The updated spec sets the new cap to 10 but does not call for breaking edits of existing skills. Please only enforce the length cap on newly added/changed tags, or grandfather tags already present on load, so unrelated edits to legacy skills remain saveable.

💬 Non-blocking

Spoofable display-name fallback for the official badge. packages/dmworkskillmarket/src/utils/publisher.ts isPlatformPublishedSkill returns true when spaceId === "" or when the trimmed creatorName/ownerName equals "超级管理员" / "Super Admin". The spaceId === "" global-scope check is a sound signal; the display-name string match is a fragile fallback that a non-admin could match by choosing that display name. The code comment acknowledges this is a heuristic pending an explicit publisher type in the API — recommend tracking a follow-up to drop the name-match once the API exposes a publisher/role field, so the badge cannot be spoofed via display name.

✅ Highlights

Tag normalization is now consistent: addTagValue trims before validating/dedup and stores the trimmed value in both NewSkillModal and EditSkillModal, and duplicate detection compares trimmed values (tags.some((tag) => tag.trim() === normalized)), closing the whitespace-variant duplicate gap.
validateSkillTags cleanly enumerates the branches (empty/whitespace, duplicate, length, invalid chars) and the Unicode-aware tagLength (Array.from) plus \p{L}\p{N} pattern handle CJK/emoji-length correctly.
Focused unit tests added for validateSkillTags and both modals covering empty, whitespace-duplicate, and mixed-locale cases.

Gate

CI: Build, e2e-p0, and all sanity/scan checks pass; the only failing check is code-review (from peer change-requests), not a build failure.
Merge state: mergeable = true (no git conflict); PR shows blocked only because the review gate is unsatisfied.

Jerry-Xin

Approving. The legacy-tag edit regression from my previous review is now resolved, and the two earlier objections (24→10 cap, official-publisher badge reuse) are accepted per the amended #1017 product spec.

Disposition of prior review items

🔴 (RESOLVED) Legacy-tag edit regression — grandfathering verified
validateSkillTags now takes a legacyTags argument (src/utils/format.ts). The length cap is only enforced when a tag exceeds MAX_SKILL_TAG_LENGTH and is not present in the legacy set: if (tagLength(trimmed) > MAX_SKILL_TAG_LENGTH && !legacy.has(trimmed)). Both EditSkillModal call sites (tagSubmitError and the save handler) now pass skill?.tags ?? [], so a user editing a skill that carries a pre-existing 11–24-char tag can save unrelated changes without being forced to truncate. Verified against every branch:

New skill (NewSkillModal calls validateSkillTags(tags) with no legacy arg) → full 10-char enforcement, no grandfather. Correct.
New/added tag > 10 chars → rejected. Correct.
Unchanged legacy tag > 10 chars → allowed (grandfathered). Correct.
Changed legacy tag to a new > 10-char value → the new value is not in the legacy set → rejected. Correct.
Empty / duplicate / invalid-char branches preserved; charset pattern (SKILL_TAG_PATTERN) is still enforced even on legacy tags, so grandfathering exempts only length, not character validity. Good.
Tests cover the contract directly: format.test.ts asserts an unchanged legacy long tag passes and a newly-added long tag fails; EditSkillModal.test.tsx now asserts an unrelated display-name edit saves while an unchanged overflow tag is preserved (tags: [overflowTag]).

Spec-accepted (previously raised, now retracted)

MAX_SKILL_TAG_LENGTH = 10 is intentional and synchronized with the marketplace/CLI per the amended #1017 spec — not a deviation.
The platform-official badge deliberately reuses the existing frontend publisher signal for now; no publisher API change is in scope. Accepted.

💬 Non-blocking (future)

The official-publisher signal still ultimately keys off a display-name match, which is spoofable. Consider migrating to a reliable server-side signal (empty spaceId / an explicit API publisher field) when the backend is ready. Not blocking this PR.

✅ Highlights

Minimal, well-scoped delta from the prior head: only format.ts + EditSkillModal.tsx plus focused tests changed; the CSS overflow fix and publisher/SkillCard code are byte-identical, so no regression to the #1017 overflow fix and no new scope creep.
Length-only grandfathering (charset still validated) is the right trade-off.
Clear PR description of the confirmed product decisions.

mergeable: MERGEABLE (no conflict; the BLOCKED state is the review/CI gate, not a git conflict).

mochashanyao

[Octo-Q · automated review]

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

Code Review — PR #1026 (octo-web)

Reviewer: Octo-Q (automated review)
Head SHA: 8a47074f105f518ce6ff736b9377f3659ec20d19

Summary

This PR addresses tag overflow and validation in the Skill Market package. It tightens the per-tag character limit from 24 to 10 and raises the tag count cap from 5 to 10, introduces a batch validateSkillTags helper with legacy-tag grandfathering, normalizes tag input by trimming before duplicate checks, adds a platform-published badge for administrator-created public skills, and applies CSS fixes for title clamping and tag-pill overflow. The changes are well-structured, the test coverage is thorough, and the implementation handles edge cases correctly.

Verification

✅ Tag validation data flow — traced validateSkillTags from format.ts through both EditSkillModal and NewSkillModal submit paths; the legacy-tag grandfathering correctly passes skill?.tags ?? [] in edit and [] in create. Empty, whitespace-only, duplicate-after-trim, and over-length tags are all rejected with appropriate error messages.
✅ Tag count enforcement — MAX_SKILL_TAGS = 10 is consistently enforced at addTagValue (slice), getTagDraftError (count check), validateSkillTags (batch check), and canSave/canCreate (button disabled state). Parse results exceeding the cap correctly disable the save button until the user trims tags.
✅ Publisher heuristic — isPlatformPublishedSkill correctly gates on visibility === "public" before checking spaceId or admin name. The Pick<Skill, ...> type constraint ensures all required fields are present. undefined/empty spaceId falls through to name matching as designed.
✅ Modal parity — EditSkillModal and NewSkillModal implement identical tag validation and normalization logic. SkillCard and SkillDetailModal share the same publisher classification flow via isPlatformPublishedSkill.
✅ i18n alignment — en-US and zh-CN both add card.platformPublisher. The .i18n/scan-config.json exclusion for publisher.ts is well-documented.
✅ CSS overflow fixes — title row gets display: block + text-overflow: ellipsis, tag pills switch from single-line clip to flex-wrap: wrap, and tag-input buttons get a dedicated .skill-market-tag-input__text class for truncation.

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

Findings

No P0/P1 issues; two P2 items below.

P2 — Hardcoded admin-name matching creates fragile backend coupling (`packages/dmworkskillmarket/src/utils/publisher.ts:16`)

The isPlatformPublishedSkill heuristic OR-matches "超级管理员" and "Super Admin" against the creator/owner name returned by the API. This works today but silently misclassifies if the backend ever changes the admin role display name, adds locale-dependent rendering, or localizes the role string. A user who happens to set their display name to either literal would also be incorrectly flagged.

The PR body explicitly documents this as temporary ("until an explicit publisher type is available in the API contract"), and the i18n exclusion justifies the Chinese literal. Not a merge blocker — flagging so the tech-debt is tracked with a follow-up issue to add a publisher_type field to the Skill API.

P2 — Owner visibility gate change for public skills (`packages/dmworkskillmarket/src/components/SkillDetailModal.tsx:234`)

The previous gate was showOwner = Boolean(skill && skill.visibility !== "public"), which suppressed the owner line for all public skills. The new logic shows the owner for every skill — platform-published skills get a shield-icon badge, while regular public skills now display their creator/owner name.

This is an intentional product change aligned with the PR's goal, and the tests have been updated accordingly. Worth confirming that the product team is aware that non-admin public skills now display their creator name in the detail modal, where it was previously hidden.

Data-Flow Trace

Consumed data	Upstream source	Reaches consumer?
`skill.tags` in `validateSkillTags`	API response → `skill.tags` field → `setTags()` in modal `useEffect` / parse result	✅ Yes, flows through to `tagSubmitError` → `canSave`/`canCreate` → submit button disabled state
`skill?.tags` as `legacyTags` param	Original `skill.tags` from props at modal open time	✅ Yes, compared via `Set.has(trimmed)` — unchanged long tags bypass length check
`isPlatformPublished` in SkillCard/SkillDetailModal	`isPlatformPublishedSkill(skill)` → checks `skill.visibility`, `skill.spaceId`, `skill.creatorName`/`skill.ownerName` from API	✅ Yes, gates `ownerLabel` and `showOwner` → renders ShieldCheck badge or name
`tagDraft.trim()` in `addTagValue`	User input → `onChange` → `setTagDraft(next)`	✅ Yes, trimmed before `validateSkillTag` + duplicate check + `setTags`
`submittedTags` in submit	`[...tags, tagDraft.trim()].slice(0, MAX_SKILL_TAGS)` or `tags`	✅ Yes, validated before submit; button disabled if validation fails

Blind-Point Checklist

C1 (dual-path parity): ✅ Clear — EditSkillModal and NewSkillModal share identical tag validation/normalization logic. Both pass validateSkillTags at submit time.
C2 (control-flow ordering): ✅ Clear — validateSkillTags is called once in the tagSubmitError chain and once in submit(). No double-invocation or ordering issues.
C3 (auth boundary): N/A — no auth/permission changes in this PR.
C4 (auth lifecycle): N/A — no auth changes.
C5 (build ≠ runtime): N/A — CSS and validation logic changes are directly observable; no build-time transformation risk.
C6 (governance/docs): N/A — no governance or policy documents changed.

Cross-Round Blocker Recheck

N/A — first review round for this PR.

Things I checked that are fine

addTagValue trims input before validation and duplicate check — prevents whitespace-only duplicates from API suggestions.
tagsFromInput already slices at MAX_SKILL_TAGS — consistent with the new cap.
SKILL_TAG_PATTERN Unicode regex correctly covers CJK characters (each code point counts as 1 in tagLength).
EditSkillModal passes skill?.tags ?? [] as legacy set — unchanged long tags bypass the 10-char limit, but modified/new tags are enforced.
CSS changes are purely presentational: max-width adjustments, flex-wrap, text-overflow: ellipsis, and overflow-wrap: anywhere for tag pills.
CI workflow adds pnpm --filter @dmwork/skillmarket test before build — good gate for future regressions.

Verdict: APPROVED

No correctness, security, or build-breaking issues. The two P2 items are non-blocking observations — the hardcoded admin names are acknowledged tech-debt, and the owner-visibility shift is an intentional product change. The implementation is solid with good test coverage across the tag validation, legacy grandfathering, overflow rendering, and publisher badge paths.

[Octo-Q] verdict: APPROVE — no P0/P1 findings; two non-blocking P2 observations.

yujiawei

Code Review — PR #1026 (octo-web)

Reviewed at head 8a47074f105f518ce6ff736b9377f3659ec20d19 (merge-base 001fb5aa). This is a re-review after the previous round requested changes; the two prior blockers are now resolved. Build green (Node 22), pnpm i18n:check green, and @dmwork/skillmarket tests 127/127 pass.

1. Spec compliance

Spec: ✅

The three UI-overflow problems from the linked issue are all addressed:

Card title / slug overflow — .skill-market-card__title-row h3 gets display:block + existing min-width/ellipsis; owner label is ellipsis-clamped.
Tag pill overflow — new .skill-market-tag-input__text ellipsis span in both editor modals; detail tags switch to flex-wrap + overflow-wrap:anywhere, so long tags wrap instead of bursting the container.
Inconsistent tag-limit messaging — validation and the inline hint now share a single validateSkillTags source, so the displayed limit matches what is enforced.

The PR also carries changes beyond the literal issue text (tightening per-tag length to 10, an "Official" publisher badge, and showing the publisher on public skills). These are documented as confirmed product decisions in the PR description, and I verified the tag-length change independently against the coordinated backend/CLI work rather than taking it on faith: the marketplace publish APIs enforce the same max=10, dive(max=10) contract, and — notably — the backend update endpoint intentionally omits the per-element dive check, which mirrors this PR's grandfather-on-update behavior exactly. So the tightening is a deliberate cross-system contract change, not an accidental regression. No missing scope, no contradicting deviation.

2. Code quality

Quality: Approved

Legacy-tag regression fixed correctly. validateSkillTags(tags, legacyTags) now skips the length check for tags already present in the record's stored set, and re-applies it the moment a tag is added or its text changes. Both EditSkillModal call sites (tagSubmitError and the save handler) pass skill?.tags, so opening a skill with an 11–24-char legacy tag and editing an unrelated field no longer disables Save. Covered by new unit tests (grandfathered vs. newly-added long tag) and an integration test asserting an unrelated displayName edit saves with the long tag preserved.
Duplicate/whitespace normalization is consistent — draft check, insertion dedup, and stored values all trim before comparison.
Data flow / trust boundary — the "Official" badge is presentational only and does not gate any behavior. Its heuristic (visibility === "public" and admin publisher name / empty space_id) is name-based and therefore theoretically spoofable, but public visibility is backend-gated (the create path hardcodes visibility:"space" and the edit modal does not expose a visibility control), so a normal user cannot mint a public skill to trigger it. See P2 below.

Findings

[P2] Name-based "Official" heuristic is spoofable in principle. isPlatformPublishedSkill keys off the publisher display name (超级管理员 / Super Admin). If a public record ever carried an attacker-controlled name matching that string, the badge would render. Reachability is currently blocked by backend-gated public visibility, and the PR already notes this is an interim signal pending a stable publisher API field. Non-blocking; worth replacing with an explicit publisher-type flag when the API exposes one.
[P2 / suggestion] Grandfather relies on exact string identity. A legacy long tag is preserved only while byte-identical to the stored value; any edit (including a whitespace tweak) re-triggers the 10-char rule. This is the intended and consistent behavior, but a short inline hint explaining "existing long tags are kept only while unchanged" would improve the edit UX.

3. Overall verdict

APPROVED

Spec ✅ and Quality Approved. The previously-blocking legacy-tag save lockout is fixed with tests, and the tag-contract tightening is a documented, cross-system-verified decision rather than an unintended regression. Remaining items are P2/non-blocking.

kense-lab · 2026-07-24T09:18:02Z

按当前最新 head 重新创建 PR，以重新触发 review 自动化。后续请转到新 PR 审核。

Jerry-Xin

Re-affirm APPROVE — post-approval delta is a merge-main + CI test-gate hygiene change; all skillmarket logic is byte-identical to my prior approval (8a47074), no regression. CI green, mergeable.

Delta since my prior approval (`8a47074` → `5b83085`)

Three commits landed: a merge main merge commit (pulling in unrelated #1063/#1066/#1069/#1070) plus two small commits touching only .github/workflows/ci.yml. Net effect verified by two-dot diff + blob hashes.

🔴 Blocking

None.

💬 Non-blocking notes

The only hand-authored change on top of the merge is additive to .github/workflows/ci.yml: a new "Unified import/export unit tests" step (for the docs feature merged in via #1063). The existing "Skill Market tests" step and the Build/Lint/WKModal-guard chain are preserved intact — good merge hygiene.
This PR's scope remains purely tag overflow + validation + legacy-tag grandfathering. No official-publisher-badge change is present in this PR, so the earlier badge/display-name follow-up discussion does not apply here.

✅ Highlights

Byte-verified: the entire packages/dmworkskillmarket/ tree is identical between the two heads (blob format.ts = 1c3f2aed, EditSkillModal.tsx = 71f6de95 at both). The previously-resolved legacy-tag-edit regression fix — validateSkillTags(tags, legacyTags = []) grandfathering pre-existing over-length tags (cap enforced only on new/changed tags via !legacy.has(trimmed)), with EditSkillModal passing skill?.tags ?? [] — stays in place unchanged.
Validation branches all intact: over-cap count, empty/whitespace, duplicate, per-tag length (with legacy grandfather), and pattern check.
Spec-accepted (not re-raised): per-tag cap = 10 is spec-compliant; reusing the existing frontend official-publisher logic is sanctioned.

yujiawei

Code Review — PR #1026 (octo-web)

Reviewed at head 5b83085b43fd6121bbd4f60048dd134c6bef0d4e against main (merge-base 8f8a19c2). Repo checked out locally; pnpm --filter @dmwork/skillmarket test run and passing (13 files, 127 tests).

Summary

This PR resolves the Skill Market long-text / multi-tag overflow issues from #1017:

Card titles clamp on one line; overflowing tag pills truncate with ellipsis and a title tooltip.
MAX_SKILL_TAGS 5 → 10 and MAX_SKILL_TAG_LENGTH 24 → 10 (documented as an intentional decision synchronized with octo-marketplace and octo-cli).
New array-level validateSkillTags() centralizes count / duplicate / length / character checks and is wired into both create and edit submit gates.
Unchanged legacy tags exceeding the new length cap are grandfathered so unrelated edits remain possible; newly added or changed tags must satisfy the 10-char contract.
Suggested and manual tags are normalized (trimmed) before duplicate checks.
A platform-official badge is shown for admin-published public skills, reusing the existing frontend publisher signal.

1. Spec compliance

Spec: ✅

Missing: none — title clamp, slug/owner ellipsis, per-tag truncation, flex-wrap tag container, and a consistent count/length limit with clear blocking feedback all map to the reported problems.
Extra: the platform-official badge and the length-cap change are beyond the literal #1017 text, but both are explicitly documented as confirmed product decisions in the PR description and were accepted in prior review rounds; not treated as scope creep.
Deviation: none — implementation matches the agreed approach.

2. Code quality

Quality: Approved

Traced the tag validation data flow end-to-end:

validateSkillTags(tags, legacyTags) grandfathers a long tag only when its trimmed value is present in the legacy set (!legacy.has(trimmed)), so unchanged legacy tags pass while any newly added or edited tag is held to the 10-char cap. Confirmed by the passing edit/create tests.
Duplicate detection normalizes with .trim() on both the incoming value and stored tags, and addTagValue stores the normalized form — so suggested tags with surrounding whitespace cannot slip past the duplicate guard.
Submit gates (canSave / canCreate and the submit handlers) both consult validateSkillTags, so a parse result or legacy dataset that violates the contract blocks submission rather than only showing a visual hint.
No P0/P1/P2 blocking findings. Cross-file surfaces (i18n keys platformPublisher, the .i18n/scan-config.json allowlist entry for publisher.ts, and the CSS classes referenced by the components) are all present and consistent.

Non-blocking notes (not required for merge):

isPlatformPublishedSkill recognizes admin-published skills partly by matching the display name 超级管理员 / Super Admin. This is a deliberate, documented interim heuristic pending an explicit publisher-type API field; it is name-string-brittle (a self-named user could collide) but is gated behind visibility === "public" and is an accepted product tradeoff.

3. Overall verdict

APPROVED — Spec ✅ and Quality Approved.

The skillmarket source at this head is identical to the previously approved state; the only change since the last approval is CI restoring the Skill Market tests step, which is a correct improvement. All package tests pass.

Process note: this PR has iterated through many review rounds. The current head is clean and the earlier objections (length cap, publisher-badge reuse, and the legacy-tag edit regression) are all resolved, so this is a genuine ready-to-merge state rather than another patch cycle — no further round expected.

fix(skillmarket): constrain tag overflow and validation

3887fae

kense-lab requested review from a team as code owners July 23, 2026 07:30

github-actions Bot added the size/M PR size: M label Jul 23, 2026

This comment was marked as outdated.

Sign in to view

Jerry-Xin previously approved these changes Jul 23, 2026

View reviewed changes

mochashanyao previously approved these changes Jul 23, 2026

View reviewed changes

yujiawei previously approved these changes Jul 23, 2026

View reviewed changes

lml2468 previously approved these changes Jul 23, 2026

View reviewed changes

cnwenf suggested changes Jul 23, 2026

View reviewed changes

test(skillmarket): enforce tag validation in CI

c8f8a48

kense-lab dismissed stale reviews from lml2468, yujiawei, mochashanyao, and Jerry-Xin via c8f8a48 July 23, 2026 08:49

Jerry-Xin previously approved these changes Jul 23, 2026

View reviewed changes

mochashanyao previously approved these changes Jul 23, 2026

View reviewed changes

yujiawei previously approved these changes Jul 23, 2026

View reviewed changes

fix(skillmarket): wrap full tag labels

0ac3b36

kense-lab dismissed stale reviews from yujiawei, mochashanyao, and Jerry-Xin via 0ac3b36 July 23, 2026 09:33

lml2468 previously approved these changes Jul 23, 2026

View reviewed changes

mochashanyao previously approved these changes Jul 23, 2026

View reviewed changes

Jerry-Xin previously approved these changes Jul 23, 2026

View reviewed changes

l-s-c mentioned this pull request Jul 23, 2026

fix(dmworkmcp): align MCP marketplace UI to Skill marketplace (#1009) #1037

Closed

fix(skill-market): align detail publisher metadata

3b5fd4f

kense-lab dismissed stale reviews from Jerry-Xin and mochashanyao via 3b5fd4f July 23, 2026 10:06

Merge remote-tracking branch 'upstream/main' into fix/skill-tag-overflow

f69c1b3

# Conflicts: # .i18n/scan-config.json

kense-lab dismissed mochashanyao’s stale review via f69c1b3 July 24, 2026 06:40

yujiawei requested changes Jul 24, 2026

View reviewed changes

fix(skillmarket): normalize tags before duplicate checks

b33cc47

lml2468 previously requested changes Jul 24, 2026

View reviewed changes

lml2468 requested changes Jul 24, 2026

View reviewed changes

yujiawei requested changes Jul 24, 2026

View reviewed changes

kense-lab mentioned this pull request Jul 24, 2026

Skill 卡片和编辑弹窗在长文本/多标签场景下存在多处 UI 溢出和布局问题 #1017

Closed

Jerry-Xin requested changes Jul 24, 2026

View reviewed changes

fix(skillmarket): preserve unchanged legacy long tags

8a47074

Jerry-Xin previously approved these changes Jul 24, 2026

View reviewed changes

mochashanyao previously approved these changes Jul 24, 2026

View reviewed changes

yujiawei previously approved these changes Jul 24, 2026

View reviewed changes

Jerry-Xin mentioned this pull request Jul 24, 2026

feat(mcp): add official publisher styling (LSC-24) #1074

Merged

kense-lab requested review from Jerry-Xin, lml2468 and yujiawei July 24, 2026 08:34

merge main and preserve CI test gates

6f28a0e

kense-lab dismissed stale reviews from yujiawei, mochashanyao, and Jerry-Xin via 6f28a0e July 24, 2026 08:44

kense-lab added 2 commits July 24, 2026 16:46

chore(ci): keep workflow unchanged

abc0f04

test(skillmarket): restore required CI coverage

5b83085

kense-lab closed this Jul 24, 2026

Jerry-Xin approved these changes Jul 24, 2026

View reviewed changes

yujiawei approved these changes Jul 24, 2026

View reviewed changes

Jerry-Xin mentioned this pull request Jul 24, 2026

fix(skillmarket): constrain tag overflow and validation #1078

Merged

Uh oh!

Conversation

kense-lab commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Confirmed product decisions

Tests

Uh oh!

This comment was marked as outdated.

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

Code Review — PR #1026 (octo-web)

Summary

Verification

Findings

Things I checked that are fine

Verdict: APPROVED

Uh oh!

yujiawei left a comment

Choose a reason for hiding this comment

Code Review — PR #1026 (octo-web)

Summary

Spec compliance

Verification (independent, static at head 3887faeaa13305c32e329b3bdc9cfcdbbe0dc419)

Findings

Verdict

Uh oh!

lml2468 left a comment

Choose a reason for hiding this comment

Review: #1026 — fix(skillmarket): 约束标签溢出 + 校验(#1017)

Gate 1 — 规格符合度 ✅

Gate 2 — 代码质量 ✅

🟡 非阻断(承 Jerry-Xin)

Uh oh!

cnwenf left a comment

Choose a reason for hiding this comment

Code Review — PR #1026 (Mininglamp-OSS/octo-web)

Summary

Findings

P1 — New tag-contract tests never run in CI (.github/workflows/ci.yml:92) [source: octo-review-qwen3.8 + octo-review-qwen3.7]

P2 — Create flow has no failing test for over-limit/over-length blocking (packages/dmworkskillmarket/src/components/__tests__/NewSkillModal.test.tsx:213) [source: octo-review-qwen3.8 + octo-review-glm5.2]

P3 — Batch gate accepts empty/whitespace-only tags (packages/dmworkskillmarket/src/utils/format.ts:97) [source: octo-review-qwen3.7 + octo-review-qwen3.8]

P3 — Batch gate skips the duplicate-tag check (packages/dmworkskillmarket/src/utils/format.ts:92) [source: octo-review-qwen3.8]

P3 — Edit-flow tests miss the recovery closure and click-not-called (packages/dmworkskillmarket/src/components/__tests__/EditSkillModal.test.tsx:180) [source: octo-review-qwen3.8]

Nit — Save path guards on tagError || draftError but omits the new validateSkillTags (packages/dmworkskillmarket/src/components/EditSkillModal.tsx:369, NewSkillModal.tsx:373) [source: octo-review-glm5.2]

Warning — dead helper tagsFromInput drifted silently (non-blocking) (packages/dmworkskillmarket/src/utils/format.ts:68) [source: octo-review-qwen3.8]

Things I checked that are fine

Verification Results

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

Code Review — PR #1026 (octo-web)

Summary

Verification

Findings

P2 — submit() validates tags state but not submittedTags (packages/dmworkskillmarket/src/components/EditSkillModal.tsx:370, packages/dmworkskillmarket/src/components/NewSkillModal.tsx:374)

Nit — validateSkillTags passes untrimmed value to validateSkillTag (packages/dmworkskillmarket/src/utils/format.ts:103)

Data Flow Trace

Blind-spot Checklist

Cross-round Blocker Recheck

Verdict: APPROVED

Uh oh!

yujiawei left a comment

Choose a reason for hiding this comment

Code Review — PR #1026 (octo-web)

Summary

Verification

Findings

P2 — submit() validates tags but not the combined submittedTags

Nit — validateSkillTags passes the untrimmed value to validateSkillTag

Blind-spot Checklist

Verdict: APPROVED

Uh oh!

kense-lab commented Jul 23, 2026 •

edited

Loading

Verification (independent, static at head `3887faeaa13305c32e329b3bdc9cfcdbbe0dc419`)

P1 — New tag-contract tests never run in CI (`.github/workflows/ci.yml:92`) [source: octo-review-qwen3.8 + octo-review-qwen3.7]

P2 — Create flow has no failing test for over-limit/over-length blocking (`packages/dmworkskillmarket/src/components/tests/NewSkillModal.test.tsx:213`) [source: octo-review-qwen3.8 + octo-review-glm5.2]

P3 — Batch gate accepts empty/whitespace-only tags (`packages/dmworkskillmarket/src/utils/format.ts:97`) [source: octo-review-qwen3.7 + octo-review-qwen3.8]

P3 — Batch gate skips the duplicate-tag check (`packages/dmworkskillmarket/src/utils/format.ts:92`) [source: octo-review-qwen3.8]

P3 — Edit-flow tests miss the recovery closure and click-not-called (`packages/dmworkskillmarket/src/components/tests/EditSkillModal.test.tsx:180`) [source: octo-review-qwen3.8]

Nit — Save path guards on `tagError || draftError` but omits the new `validateSkillTags` (`packages/dmworkskillmarket/src/components/EditSkillModal.tsx:369`, `NewSkillModal.tsx:373`) [source: octo-review-glm5.2]

Warning — dead helper `tagsFromInput` drifted silently (non-blocking) (`packages/dmworkskillmarket/src/utils/format.ts:68`) [source: octo-review-qwen3.8]

P2 — `submit()` validates `tags` state but not `submittedTags` (`packages/dmworkskillmarket/src/components/EditSkillModal.tsx:370`, `packages/dmworkskillmarket/src/components/NewSkillModal.tsx:374`)

Nit — `validateSkillTags` passes untrimmed value to `validateSkillTag` (`packages/dmworkskillmarket/src/utils/format.ts:103`)

P2 — `submit()` validates `tags` but not the combined `submittedTags`

Nit — `validateSkillTags` passes the untrimmed value to `validateSkillTag`

Re-review: #1026 @ `0ac3b36d` — skillmarket 标签约束

P2 — Server-side tag limit alignment (`packages/dmworkskillmarket/src/utils/format.ts:71`)

Nit — Non-standard `-webkit-line-clamp` without fallback (`packages/dmworkskillmarket/src/index.css:735`)

True delta (`c8f8a48` → `0ac3b36`)

Re-review: #1026 @ `f69c1b3e` — fix(skillmarket): constrain tag overflow and validation

🔴 阻塞 1 — 单标签字符上限被悄悄从 24 降到 10(deviation + 回归) `format.ts:72`

Re-review: #1026 @ `f69c1b3e` — fix(skillmarket): constrain tag overflow and validation

🔴 阻塞 1 — 单标签字符上限被悄悄从 24 降到 10(deviation + 回归) `format.ts:72`