Skip to content

feat(namespace): allow member overwrite of published skills via namespace setting - #797

Open
felix021 wants to merge 2 commits into
iflytek:mainfrom
felix021:feat/namespace-allow-member-overwrite
Open

feat(namespace): allow member overwrite of published skills via namespace setting#797
felix021 wants to merge 2 commits into
iflytek:mainfrom
felix021:feat/namespace-allow-member-overwrite

Conversation

@felix021

@felix021 felix021 commented Sep 1, 2026

Copy link
Copy Markdown

Summary

  • Add a per-namespace allowMemberOverwrite setting (default false): namespace governance decides whether members may publish new versions to a (namespace, slug) coordinate already published by another member
  • When disabled (default), publish owner-isolation behaves exactly as before, line-for-line, including validateOnly dry-runs
  • When enabled, any namespace member (per namespace_member; SUPER_ADMIN gets no exemption) can publish a new version onto the coordinate:
    • the version attaches to the original owner's Skill record (Skill.ownerId unchanged, SkillVersion.createdBy records the actual publisher)
    • the requested visibility is ignored; the target skill's current visibility is kept, so a PRIVATE request cannot hide an already-published skill
    • member overwrites of non-PRIVATE targets go through the existing PENDING_REVIEW review flow
  • The setting is mutable via the existing PUT /api/v1/namespaces/{slug} (OWNER/ADMIN only, reuses assertAdminOrOwner) and exposed as a toggle in the web namespace edit dialog (en/zh/ru)

Motivation: publishing is owner-isolated today — the first publisher permanently owns a (namespace, slug) coordinate and there is no owner transfer. When a skill author leaves or moves on, namespace admins cannot take over updates without deleting and recreating the skill, losing version history and download stats.

Validation

  • Backend tests passed
  • Frontend typecheck/build passed
  • OpenAPI SDK regenerated or checked when API contracts changed
  • Smoke test run when relevant

Commands run:

./mvnw -pl skillhub-app -am test   # 813 tests, 0 failures, 0 errors, 1 skipped
cd web && pnpm run typecheck
cd web && pnpm exec vitest run     # 707 tests, 198 files, all passed
SPRING_PROFILES_ACTIVE=local java -jar skillhub-app/target/skillhub-app-0.1.0.jar  # on a scratch postgres/redis
pnpm exec openapi-typescript http://127.0.0.1:18080/v3/api-docs -o src/api/generated/schema.d.ts
  • Backend: full app suite green, including 9 new cases (ownership/createdBy after overwrite, visibility inheritance, non-member rejection, archived-target rejection, dry-run version-conflict parity); existing owner-isolation tests pass unchanged
  • Frontend: tsc --noEmit clean; full Vitest suite green (707 tests, 198 files)
  • OpenAPI: web/src/api/generated/schema.d.ts regenerated from a live server; diff contains only the new allowMemberOverwrite field
  • Manual e2e on single-node docker compose with four identities (regular account / namespace OWNER / namespace MEMBER / non-member):
    1. toggle off → OWNER publishing to another member's published slug fails with 400 nameConflict; CLI --dry-run reports the same conflict
    2. toggle on → OWNER / MEMBER overwrite succeeds: single Skill record, owner_id = original author, new version created_by = actual publisher, status PENDING_REVIEW, effective after approval
    3. non-member still rejected with notMember; disabling the toggle restores rejection

Risk

  • User-facing impact: none until a namespace OWNER/ADMIN enables the toggle; afterwards members can take over updates to published coordinates, still subject to the existing review flow
  • Deployment or migration impact: additive migration V47 (namespace.allow_member_overwrite BOOLEAN NOT NULL DEFAULT FALSE)
  • Rollback approach: revert the toggle (or the deploy); default-off keeps current behavior for all existing namespaces

Notes

  • Related issue: [Feature] Skill Maintainer:支持团队多人协作维护与发布(生产级完整方案) #732 (comprehensive Skill Maintainer proposal). This PR is intentionally a much smaller, namespace-level governance opt-in: it reuses the existing publish path, review flow and audit attribution, and adds no new permission surface beyond one optional field
  • Known follow-ups (non-blocking): the CLI publish response echoes the requested visibility instead of the effective target visibility; dry-run stays conservative when the caller's own record for the slug is archived
  • Docs: no operator runbook changes; default-off namespace setting

…pace setting

Add per-namespace allowMemberOverwrite (default false). When off, publish
owner-isolation behaves exactly as before. When on, any namespace member may
publish a new version to a (namespace, slug) coordinate already published by
another member; the version attaches to the original owner's Skill record
(Skill.ownerId unchanged, SkillVersion.createdBy records the actual publisher).
validateOnly dry-run applies the same rule. Setting is mutable via the existing
namespace update API (OWNER/ADMIN only) and exposed in the web namespace edit
dialog.
- Overwrite inherits the target skill's visibility: the requested visibility
  must not change another owner's skill reach (PRIVATE request no longer hides
  a published skill or moves its pointer to an unpublished version)
- Pick the first non-archived published record as overwrite target; reject with
  error.skill.publish.archived when only an archived record carries the slug
- validateOnly mirrors publish target selection: archived check and
  version-exists check now apply to the overwrite target
- Fix NamespaceRequest 4-arg call sites, web test fixtures (tsc), duplicate
  version-save capture, and super-admin membership lookup assertion
- Add tests: non-member rejection, archived-target rejection, dry-run
  version conflict on target
@CLAassistant

CLAassistant commented Sep 1, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@XiaoSeS

XiaoSeS commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the detailed implementation. The underlying problem is valid, but we should not merge the current namespace-wide toggle as-is.

The current model grants every namespace member content-write access to every existing skill coordinate. That is broader than the recovery use case, and it is especially risky for PRIVATE skills because the current flow can update the target without the normal review path. Resolving the target from namespace + slug is also ambiguous; collaborative writes should target an explicit skillId. In addition, V47__namespace_allow_member_overwrite.sql now conflicts with the V47__skill_rating_reviews.sql migration already on main.

We prefer to separate two product capabilities:

  1. Ownership recovery

    • Explicit action on a specific skillId.
    • Restricted to the namespace OWNER (not every ADMIN/MEMBER).
    • The new owner must be an active member of the same namespace.
    • Require a reason and record the actor, previous owner, new owner, and timestamp in the audit log.
    • Preserve the existing Skill record, versions, download counts, reviews, and coordinate.
    • Reject coordinate/ownership conflicts deterministically.
    • Do not transfer ownership automatically when a member leaves.
  2. Collaborative maintenance

    • Add explicit Skill-level maintainer grants rather than a namespace-wide overwrite switch.
    • Owner grants/revokes maintainers; maintainers publish to an explicit targetSkillId.
    • Maintainers may add versions but cannot change ownership, visibility, archive/delete the Skill, or grant other maintainers.
    • PUBLIC/NAMESPACE_ONLY updates keep the existing review flow.
    • PRIVATE update semantics need an explicit product decision before implementation.
    • Keep Skill.ownerId as the owner and SkillVersion.createdBy as the actual publisher.

These should be separate, reviewable slices rather than one PR implementing both. A smaller ownership-recovery PR would directly solve the “author left the team” case. Skill Maintainers can then follow as a separate feature when the permission and lifecycle matrix is approved.

Please do not spend time rebasing the current implementation yet. First let us agree whether you would like to revise this PR toward the ownership-recovery slice or close it in favor of a new focused PR.

@FenjuFu FenjuFu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through this — the engineering is solid and the safety-conscious defaults are the right call. Flagging a blocker and a couple of points that I think need a maintainer decision rather than a plain +1 from me.

Blocker: DCO is failing. The sign-off email doesn't match the commit author/committer, so the DCO check is red. Please git commit --amend -s (or rebase with --signoff) so the Signed-off-by: line matches your author identity, then force-push.

What's good:

  • Default-off (allowMemberOverwrite=false) keeps the current owner-isolation path line-for-line, including validateOnly dry-runs — no behavior change until a namespace OWNER/ADMIN opts in.
  • Visibility is inherited from the target, so a member's PRIVATE request can't hide an already-published skill. Good — that closes the obvious hijack vector.
  • The version still attaches to the original owner (Skill.ownerId unchanged, SkillVersion.createdBy = actual publisher) and non-PRIVATE overwrites still go through PENDING_REVIEW. Attribution + review flow stay intact.
  • SUPER_ADMIN gets no exemption — the toggle is the only gate. I read this as deliberate (a platform super-admin still can't silently overwrite another owner's coordinate unless the namespace opted in). Worth confirming that's the intended semantics, since it's the opposite of the usual "super-admin bypasses everything" default elsewhere in the codebase.

Points for a maintainer to own:

  1. This overlaps #732 (Skill Maintainer — full collaborative maintenance). You've scoped this deliberately smaller (one namespace-level opt-in, reusing the existing publish/review/audit paths, no new permission surface), which I think is the right increment — but whether this lands as the interim step vs. waiting for the #732 design should be a maintainer call.
  2. The overwrite path writes to the same (namespace, slug) coordinate that's guarded by the publish-time unique constraints. Worth a sanity check that concurrent overwrite + the existing conflict handling don't interact badly (there's related concurrency work in flight on that publish path).

Because this touches the ownership/authorization boundary I'm not going to rubber-stamp an approve — leaving that to the maintainers. Happy to re-look once DCO is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants