fix: owner-qualified skill reports for ambiguous slugs - #3172
Conversation
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
|
Someone is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
Codex review: needs real behavior proof before merge. Reviewed July 19, 2026, 5:08 AM ET / 09:08 UTC. Summary 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.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest 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 changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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 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. |
|
Maintainer proof at exact head 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 I also added Validation:
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:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Patrick-Erichsen
left a comment
There was a problem hiding this comment.
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 foundbefore 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.
Summary
POST /api/v1/skills/{slug}/reportnow acceptsowner/ownerHandlefrom the query or JSON body, matching GET disambiguation.Slug is used by multiple publishers. Use an owner-qualified skill URL.) instead of misleadingSkill not found.Fixes #3111
Test plan
VITE_CONVEX_URL=https://example.invalid bunx vitest run convex/httpApiV1.handlers.test.ts -t "skill report"(4 passed)aaa73625ed41: owner-qualified and bare requests both returned400 Skill not found34ef2ade: owner-qualified request returned200and persisted the requested publisher's exact skill ID; bare request returned400with owner-qualified guidancebun run ci:unit(4,799 passed, 1 skipped)bun run ci:types-buildbun run ci:static$autoreview(clean; no actionable findings)Notes
Maintainer follow-up
34ef2aderemoved the unnecessary HTTPskillIdoverride so the report target cannot disagree with the route.