Skip to content

fix: owner-qualified skill reports for ambiguous slugs - #3172

Merged
Patrick-Erichsen merged 2 commits into
openclaw:mainfrom
pgm-norbert:fix/skill-report-owner-disambiguation
Jul 21, 2026
Merged

fix: owner-qualified skill reports for ambiguous slugs#3172
Patrick-Erichsen merged 2 commits into
openclaw:mainfrom
pgm-norbert:fix/skill-report-owner-disambiguation

Conversation

@pgm-norbert

@pgm-norbert pgm-norbert commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes skill listing reports for ambiguous slugs (SECURITY.md: report handling).
  • POST /api/v1/skills/{slug}/report now accepts owner / ownerHandle from the query or JSON body, matching GET disambiguation.
  • Bare-slug collisions now return the standard message (Slug is used by multiple publishers. Use an owner-qualified skill URL.) instead of misleading Skill not found.
  • Keeps the URL slug plus owner qualifier authoritative; the endpoint does not accept a conflicting direct skill ID.

Fixes #3111

Test plan

  • VITE_CONVEX_URL=https://example.invalid bunx vitest run convex/httpApiV1.handlers.test.ts -t "skill report" (4 passed)
  • Authenticated local Convex duplicate-slug proof:
    • merge base aaa73625ed41: owner-qualified and bare requests both returned 400 Skill not found
    • PR head 34ef2ade: owner-qualified request returned 200 and persisted the requested publisher's exact skill ID; bare request returned 400 with owner-qualified guidance
  • bun run ci:unit (4,799 passed, 1 skipped)
  • bun run ci:types-build
  • bun run ci:static
  • $autoreview (clean; no actionable findings)

Notes

Maintainer follow-up 34ef2ade removed the unnecessary HTTP skillId override so the report target cannot disagree with the route.

Report API/CLI previously resolved bare slugs only, so collisions
collapsed into "Skill not found" and blocked listing reports.
Accept ownerHandle/owner query/body and optional skillId, and surface
the standard ambiguous-slug guidance.

Fixes openclaw#3111
@pgm-norbert
pgm-norbert requested review from a team and Patrick-Erichsen as code owners July 18, 2026 06:48
@vercel

vercel Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Someone is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. labels Jul 19, 2026
@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 19, 2026, 5:08 AM ET / 09:08 UTC.

Summary
The PR lets the skill-report endpoint resolve a target by owner-qualified slug or supplied skill ID, and returns the standard ambiguous-slug guidance for unresolved bare collisions.

Reproducibility: no. high-confidence live reproduction was provided in this PR: the linked report and source diff establish the affected path, but the PR body leaves the authenticated manual check unchecked.

Review metrics: 1 noteworthy metric.

  • Changed surface: 3 files affected; 74 additions, 2 deletions. The patch is narrowly scoped to the HTTP handler, report resolver, and its focused unit coverage.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Run the unchecked authenticated duplicate-slug report scenario and attach redacted terminal output, logs, or a recording.
  • Show the owner-qualified request reaches the intended listing and that an unqualified collision returns the standard guidance message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR reports a focused mocked Vitest run, but its manual authenticated duplicate-slug API check remains unchecked; provide redacted live output or a terminal recording showing the successful report and the bare-slug ambiguity response before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] An authenticated live report against a real duplicate-slug listing has not been shown, so merge review cannot yet confirm that HTTP parsing, token authentication, Convex resolution, and moderation-report persistence work together.

Maintainer options:

  1. Decide the mitigation before merge
    Run the documented authenticated API flow against a real duplicate-slug fixture, show that the owner-qualified report targets the intended listing and that a bare collision returns the guidance message, then merge this focused fix and close Listing reports are impossible when a slug collides with another publisher's (report handler ignores owner) #3111.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining merge blocker is contributor-supplied real behavior proof, which an automated repair lane cannot create on the contributor's authenticated setup.

Security
Cleared: The inspected diff adds authenticated target disambiguation to an existing reporting flow and does not introduce new privileges, dependency sources, workflow permissions, or secret handling.

Review details

Best possible solution:

Run the documented authenticated API flow against a real duplicate-slug fixture, show that the owner-qualified report targets the intended listing and that a bare collision returns the guidance message, then merge this focused fix and close #3111.

Do we have a high-confidence way to reproduce the issue?

No high-confidence live reproduction was provided in this PR: the linked report and source diff establish the affected path, but the PR body leaves the authenticated manual check unchecked.

Is this the best way to solve the issue?

Yes, conditionally: forwarding the existing owner qualifier into the existing owner-aware resolver is a narrow solution that preserves the established ambiguous-slug message; live proof should confirm the complete path before merge.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against aaa73625ed41.

Label changes

Label changes:

  • add P1: This repairs a currently blocked authenticated reporting route for colliding listings, which can delay moderation of a potentially abusive listing.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports a focused mocked Vitest run, but its manual authenticated duplicate-slug API check remains unchecked; provide redacted live output or a terminal recording showing the successful report and the bare-slug ambiguity response before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: This repairs a currently blocked authenticated reporting route for colliding listings, which can delay moderation of a potentially abusive listing.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports a focused mocked Vitest run, but its manual authenticated duplicate-slug API check remains unchecked; provide redacted live output or a terminal recording showing the successful report and the bare-slug ambiguity response before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Linked defect and scope: The linked report describes an authenticated POST with ?owner=kingaiwork being ignored for a colliding slug; this branch forwards the owner qualifier and passes it into the report mutation. (convex/httpApiV1/skillsV1.ts:3165, 9bc402984c75)
  • Ambiguity handling: The internal report mutation switches from the bare-slug resolver to the owner-aware resolver and explicitly returns the same owner-qualified guidance when a bare slug remains ambiguous. (convex/skills.ts:4374, 9bc402984c75)
  • Focused regression coverage: The added handler test covers forwarding a query owner plus body ownerHandle and skillId to the internal report mutation; it does not demonstrate a real authenticated report against a live collision. (convex/httpApiV1.handlers.test.ts:3519, 9bc402984c75)
  • Current implementation status: The supplied PR context identifies current main as aaa7362 and this change only exists on the PR head, so the linked defect is not yet implemented on main. (aaa73625ed41)

Likely related people:

  • pgm-norbert: The available review context connects this contributor to the concrete owner-qualified report-path repair; current-main feature-history attribution could not be established from the read-only inspection environment. (role: proposed repair author; confidence: low; commits: 9bc402984c75; files: convex/httpApiV1/skillsV1.ts, convex/skills.ts, convex/httpApiV1.handlers.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix: owner-qualified skill reports for ambiguous slugs This is item 1/1 in the current shard. Shard 11/20.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@Patrick-Erichsen

Copy link
Copy Markdown
Collaborator

Maintainer proof at exact head 34ef2ade (base aaa73625ed41).

I exercised the real authenticated HTTP path against disposable local Convex state containing two publishers with the same slug, a hashed API token, and moderation-report readback.

Unpatched merge base

{"mode":"unpatched","scopedStatus":400,"scopedMessage":"Skill not found","bareStatus":400,"bareMessage":"Skill not found"}

Candidate

{"mode":"candidate","scopedStatus":200,"persistedTargetMatchesOwner":true,"bareStatus":400,"bareGuidance":true}

The candidate's owner-qualified POST returned 200; the stored report's skillId matched the requested publisher's listing. The same bare slug returned 400 with Slug is used by multiple publishers. Use an owner-qualified skill URL.

I also added 34ef2ade to keep the route slug and owner authoritative. The original optional HTTP skillId could disagree with the URL and silently target another listing; the signed-in web dialog uses its separate public mutation and did not require that bypass.

Validation:

  • focused report tests: 4 passed
  • full unit coverage: 4,799 passed, 1 skipped
  • ci:types-build: passed
  • ci:static: passed
  • required schema and CLI TypeScript checks: passed
  • autoreview: clean, no actionable findings

Best-fix verdict: best. This reuses the established owner-aware resolver at the report mutation boundary, preserves the shared bare-slug ambiguity guidance, and avoids widening the HTTP target contract.

Alternatives considered:

  • Parse owner/slug inside the slug path: rejected because existing GET and sibling write routes already use ownerHandle.
  • Accept a direct HTTP skillId: rejected because it can conflict with the route and is unnecessary for the web dialog.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@Patrick-Erichsen Patrick-Erichsen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This fixes the authenticated reporting path for owner-scoped slug collisions and preserves a truthful error for unresolved bare slugs.

LOC: +58/-1 (3 files)

No blocking findings.

Best-fix verdict: best. The HTTP handler forwards the established owner qualifier to the existing owner-aware resolver, and the mutation owns the ambiguity decision. Maintainer commit 34ef2ade removes the unnecessary direct-ID override so the URL target cannot disagree with the persisted report.

Evidence checked:

  • exact-head authenticated local Convex proof with duplicate publisher slugs, hashed API token, POST, and report-list readback
  • identical merge-base replay demonstrating 400 Skill not found before the fix
  • focused handler tests, full unit coverage, static/types/build gates, and clean autoreview
  • issue #3111, surrounding resolver/report code, CLI caller, web-dialog caller, CI, discussion, and current review state

Remaining uncertainty: the fork cannot create an authorized Vercel preview, but this is a backend-only change and local exact-head runtime proof covers the affected HTTP/Convex path.

@Patrick-Erichsen
Patrick-Erichsen merged commit 34ad6ab into openclaw:main Jul 21, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 High-priority user-facing bug, regression, or broken workflow. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Listing reports are impossible when a slug collides with another publisher's (report handler ignores owner)

2 participants