Skip to content

fix(httpApiV1): surface 400-class delete validation errors instead of opaque 500#2488

Merged
momothemage merged 3 commits into
openclaw:mainfrom
momothemage:feature/fix_error_code
Jun 4, 2026
Merged

fix(httpApiV1): surface 400-class delete validation errors instead of opaque 500#2488
momothemage merged 3 commits into
openclaw:mainfrom
momothemage:feature/fix_error_code

Conversation

@momothemage
Copy link
Copy Markdown
Contributor

@momothemage momothemage commented Jun 4, 2026

Why

DELETE /api/v1/packages/<name> (and the parallel skill / soul soft-delete endpoints) currently return an opaque 500 Internal Server Error with no body context whenever the underlying mutation throws anything outside a tiny keyword whitelist.

The case #2482 that surfaced this:

$ npx --yes clawhub package delete xrow/xrow-self-improvement --yes
✖ Internal Server Error (remaining: 2997/3000, reset in 24s)

Under the hood:

  1. The CLI URL-encodes the name and the route resolves packageName = "xrow/xrow-self-improvement".
  2. softDeletePackageInternal calls normalizePackageName, whose regex ^(?:@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$ accepts either a bare name or @scope/name. Bare scope/name (no @) is rejected with ConvexError("Package name must be lowercase and npm-safe (example: @scope/name or plugin-name)").
  3. softDeleteErrorToResponse only knows four keywords (unauthorized, forbidden, not found, slug required); everything else is rewritten to literal "Internal Server Error" + 500. The actual reason is dropped.

That is a textbook silent-drop: a 4xx user-input error reported as 5xx with the diagnostic text discarded. CLI users have no way to recover without server logs.

What

convex/httpApiV1/shared.ts — rewrite softDeleteErrorToResponse:

  • Run the raw message through cleanUserFacingErrorMessage first so [CONVEX] / ConvexError: / [Request ID:...] prefixes don't hide the real text from the keyword check.
  • New SOFT_DELETE_BAD_REQUEST_HINTS whitelist for input-validation failures (slug required, package name required, package name must be, must be lowercase, npm-safe, reserved, version required). Matches now return 400 + cleaned message so the CLI / HTTP client sees the reason.
  • The 500 fallback is unchanged on the wire: status 500 with the bare body Internal Server Error. The cleaned message is computed but intentionally not appended, so unexpected exceptions do not leak diagnostic detail across the public HTTP boundary.

convex/httpApiV1.shared.test.ts — adds dedicated regression coverage for both the new 400 path (cleaned validation message) and the unknown-failure generic-500 boundary, so the security-boundary contract is pinned.

No schema / route / CLI changes. The 401, 403, and 404 paths and formatAuthzMessage fallbacks are unchanged.

Behavioral diff (HTTP)

Trigger Before After
DELETE /packages/xrow/xrow-self-improvement 500 Internal Server Error 400 Package name must be lowercase and npm-safe (example: @scope/name or ...)
Mutation throws Slug required 400 Slug required (hard-coded body) 400 Slug required (cleaned message, same status)
Mutation throws Forbidden: hidden by moderation 403 Forbidden: hidden by moderation unchanged
Mutation throws Skill not found 404 Skill not found unchanged
Mutation throws new Error("boom") (truly unexpected) 500 Internal Server Error 500 Internal Server Error (unchanged)

Tests

  • bunx oxfmt convex/httpApiV1/shared.ts convex/httpApiV1.shared.test.ts — clean.
  • read_lints on shared.ts — no findings.
  • Local bun run ci:static + bun run ci:types-build + bun run ci:unit — all green on the squashed prep head before push.
  • Repository CI on the PR head: static, unit, e2e-http, types-build, packages, playwright-smoke, playwright-local-auth, all CodeQL Light analyses, and Scan for Verified Secrets — all SUCCESS. Only the external Vercel – clawhub status is FAILURE (Vercel team authorization), which is not a required check.

Risk

  • httpApiV1.handlers.test.ts already had explicit cases for 401 / 403 / 404 / moderation 403 and a generic-500 unknown assertion; that 500 assertion still pins the bare Internal Server Error body, matching the on-the-wire contract.
  • A grep for "Internal Server Error" across convex/ finds only this test, the new shared.ts constant, and the new shared-helper regression — no other call site relies on the exact body string.
  • Status codes for 401 / 403 / 404 / 500 are unchanged on the wire; the only behavioral change is that a previously-500-with-no-context class of input errors is reclassified to 400 with the cleaned message.

Behavioral sweep

  • status: pass
  • silentDropRisk: this PR removes a documented silent drop in the soft-delete HTTP boundary; no new silent drops introduced.
  • Notes for reviewer: there is a follow-up worth tracking — the keyword-substring routing in softDeleteErrorToResponse is itself fragile. A cleaner long-term shape would be to thread an explicit code field on the underlying ConvexErrors (e.g. INVALID_INPUT, NOT_FOUND) and route on that. Out of scope for this fix, which is intentionally minimal.

Out of scope

  • normalizePackageName itself is not changed. Whether bare scope/name should be auto-prefixed with @ is a UX question for the CLI / schema packages, not for this HTTP-edge fix.

Behavior proof (after fix)

ClawSweeper P1 asked for real post-fix behavior, not just unit asserts. The end-to-end output below was captured locally against the real softDeleteErrorToResponse driven by real ConvexErrors from real normalizePackageName(...). No mocks on the code under test.

Note on the captured transcript below. The raw transcript was first captured on a working commit (45448721) that briefly enriched the 500 body with the cleaned message. ClawSweeper subsequently flagged that as a sensitive-diagnostic exposure across the public HTTP boundary; the final PR HEAD reverts the 500-body enrichment back to the bare Internal Server Error. The transcript's unknown error 'boom' row therefore shows Internal Server Error: boom — that is the interim behavior, not the final HEAD. The final HEAD returns the bare Internal Server Error for unknown 500s; this is asserted by the new convex/httpApiV1.shared.test.ts and by the existing convex/httpApiV1.handlers.test.ts unknown case. All other rows below (the 400 / 403 / 404 results that fix #2482) are accurate for the final HEAD.

1. End-to-end HTTP boundary — before vs after (final HEAD)

Driver script: .local/proof-handler.ts — calls real normalizePackageName(badInput) to throw a real ConvexError, hands it to real softDeleteErrorToResponse("package", err, headers), then prints the real Response.status and Response.body.

Trigger Before (origin/main) After (final HEAD)
DELETE /packages/xrow/xrow-self-improvement 500 Internal Server Error 400 Package name must be lowercase and npm-safe (example: @scope/name or plugin-name)
empty package name 500 Internal Server Error 400 Package name required
ConvexError("Slug required") 400 Slug required 400 Slug required (unchanged)
ConvexError("Forbidden: hidden by moderation") 403 Forbidden: hidden by moderation 403 Forbidden: hidden by moderation (unchanged)
ConvexError("Package not found") 404 Package not found 404 Package not found (unchanged)
new Error("boom") (truly unexpected) 500 Internal Server Error 500 Internal Server Error (unchanged on the wire)

Raw transcript — interim capture (45448721, pre-revert; see note above)

$ bun .local/proof-handler.ts
HTTP behavioral proof — softDeleteErrorToResponse('package', err)
================================================================

=== xrow/xrow-self-improvement (the original 500 case) ===
  source error: Package name must be lowercase and npm-safe (example: @scope/name or plugin-name)
  HTTP status : 400
  HTTP body   : Package name must be lowercase and npm-safe (example: @scope/name or plugin-name)

=== empty package name ===
  source error: Package name required
  HTTP status : 400
  HTTP body   : Package name required

=== Slug required (pre-existing 400) ===
  source error: Slug required
  HTTP status : 400
  HTTP body   : Slug required

=== Forbidden: hidden by moderation (pre-existing 403) ===
  source error: Forbidden: hidden by moderation
  HTTP status : 403
  HTTP body   : Forbidden: hidden by moderation

=== Package not found (pre-existing 404) ===
  source error: Package not found
  HTTP status : 404
  HTTP body   : Package not found

=== unknown error 'boom' (truly unexpected 500) ===
  source error: boom
  HTTP status : 500
  HTTP body   : Internal Server Error: boom    <-- interim only; final HEAD returns "Internal Server Error"

Raw transcript — BEFORE (PR base, origin/main)

=== xrow/xrow-self-improvement (the original 500 case) ===
  source error: Package name must be lowercase and npm-safe (example: @scope/name or plugin-name)
  HTTP status : 500
  HTTP body   : Internal Server Error

=== empty package name ===
  source error: Package name required
  HTTP status : 500
  HTTP body   : Internal Server Error

=== Slug required (pre-existing 400) ===
  source error: Slug required
  HTTP status : 400
  HTTP body   : Slug required

=== Forbidden: hidden by moderation (pre-existing 403) ===
  source error: Forbidden: hidden by moderation
  HTTP status : 403
  HTTP body   : Forbidden: hidden by moderation

=== Package not found (pre-existing 404) ===
  source error: Package not found
  HTTP status : 404
  HTTP body   : Package not found

=== unknown error 'boom' (truly unexpected 500) ===
  source error: boom
  HTTP status : 500
  HTTP body   : Internal Server Error

2. Vitest regression run (final HEAD)

$ bun run ci:unit
...
✓ convex/httpApiV1.shared.test.ts (2 passed)
✓ convex/httpApiV1.handlers.test.ts (passed, including the `unknown` 500 case)
...

convex/httpApiV1.shared.test.ts adds two pinned assertions on the final HEAD:

  1. The 400 path returns the cleaned validation message.
  2. The unknown-failure path stays a generic 500 with body "Internal Server Error".

The convex/httpApiV1.handlers.test.ts unknown 500 assertion continues to pin the bare "Internal Server Error" body, so the on-the-wire 500 contract is unchanged.

Repro

# After (final HEAD)
git fetch origin pull/2488/head
git checkout FETCH_HEAD
bun .local/proof-handler.ts

# Before
git checkout origin/main -- convex/httpApiV1/shared.ts
bun .local/proof-handler.ts
git checkout FETCH_HEAD -- convex/httpApiV1/shared.ts
Clipboard_Screenshot_1780545603

proof-handler.ts

@momothemage momothemage requested review from a team and Patrick-Erichsen as code owners June 4, 2026 03:41
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Jun 4, 2026

@momothemage is attempting to deploy a commit to the Amantus Machina Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Jun 4, 2026

Codex review: passed. Reviewed June 4, 2026, 3:30 AM ET / 07:30 UTC.

Summary
The PR changes HTTP API v1 soft-delete error mapping to clean Convex-wrapped errors, return 400 for whitelisted validation failures, keep unexpected 500 bodies generic, and adds regression tests.

Reproducibility: yes. Source inspection shows current main maps wrapped package-name validation failures through the generic 500 path because it checks the raw message and only has a hard-coded slug required 400 case.

Review metrics: 1 noteworthy metric.

  • HTTP helper surface: 1 helper changed, 2 regression tests added. The patch is focused on the shared soft-delete response mapper and pins both the new 400 path and the generic 500 boundary.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • none.

Risk before merge

  • [P1] The PR intentionally exposes cleaned validation messages at a public HTTP boundary, so maintainers should be comfortable with the whitelist until a typed error-code path replaces substring routing.

Maintainer options:

  1. Accept the narrow whitelist (recommended)
    Merge after confirming the final head keeps unknown failures generic and only returns cleaned messages for intended validation errors.
  2. Require typed error codes first
    Pause the PR and ask for explicit ConvexError codes if maintainers do not want substring-based public error routing to expand further.

Next step before merge

  • [P2] No repair lane is needed; the PR is already a focused implementation with sufficient behavior proof and no actionable code findings.

Security
Cleared: The final diff preserves the generic unknown-500 body and limits newly exposed text to whitelisted soft-delete validation failures.

Review details

Best possible solution:

Merge the narrow HTTP boundary fix if maintainers accept the whitelist-based exposure as an interim path toward typed Convex error codes.

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

Yes. Source inspection shows current main maps wrapped package-name validation failures through the generic 500 path because it checks the raw message and only has a hard-coded slug required 400 case.

Is this the best way to solve the issue?

Yes, if maintainers accept the interim whitelist. The patch fixes the shared HTTP mapper, keeps unknown failures generic, and avoids changing CLI or package-name semantics.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 909a47106e0f.

Label changes

Label justifications:

  • P2: This is a bounded HTTP API bug fix for delete error reporting with limited blast radius.
  • merge-risk: 🚨 security-boundary: The diff changes which backend validation messages cross the public HTTP API boundary.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (terminal): The PR body includes after-fix terminal output from a real driver exercising softDeleteErrorToResponse with real ConvexErrors, plus before/after rows for the affected HTTP boundary.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from a real driver exercising softDeleteErrorToResponse with real ConvexErrors, plus before/after rows for the affected HTTP boundary.
Evidence reviewed

What I checked:

  • Current main still has the opaque fallback: softDeleteErrorToResponse on current main checks the raw error message, only special-cases slug required as 400, and otherwise returns a generic 500. (convex/httpApiV1/shared.ts:522, 909a47106e0f)
  • Package validation throws the diagnostic that currently gets hidden: normalizePackageName throws Package name must be lowercase and npm-safe... for invalid package names such as bare scope/name. (convex/lib/packageRegistry.ts:164, 909a47106e0f)
  • Final PR head cleans and whitelists validation failures: The final head cleans Convex transport wrappers, checks a validation-hint whitelist, returns 400 with the cleaned message, and keeps unknown failures as Internal Server Error. (convex/httpApiV1/shared.ts:536, cc6ac9d31a70)
  • Regression tests pin both sides of the boundary: The added tests assert the cleaned validation message becomes 400 and an unknown soft-delete failure remains a generic 500 body. (convex/httpApiV1.shared.test.ts:36, cc6ac9d31a70)
  • Earlier security regression was repaired: Diffing the interim commit against the final head shows the body-enriched unknown 500 fallback was removed and replaced with the generic Internal Server Error response. (convex/httpApiV1/shared.ts:549, cc6ac9d31a70)
  • Feature history provenance: Blame attributes the current soft-delete response mapper and package-name normalization to the same recent HTTP API implementation commit. (convex/httpApiV1/shared.ts:517, 707d39092391)

Likely related people:

  • Vyctor H. Brzezowski: Current-main blame attributes softDeleteErrorToResponse, package soft-delete routing, and normalizePackageName to commit 707d390. (role: introduced behavior; confidence: high; commits: 707d39092391; files: convex/httpApiV1/shared.ts, convex/httpApiV1/packagesV1.ts, convex/lib/packageRegistry.ts)
  • Patrick Erichsen: Recent commits touched the HTTP API v1 shared helper, shared tests, and package HTTP surface shortly before this PR. (role: recent area contributor; confidence: medium; commits: e8cfbddf17a6, ff48b2cc7020; files: convex/httpApiV1/shared.ts, convex/httpApiV1.shared.test.ts, convex/httpApiV1/packagesV1.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 added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jun 4, 2026
@momothemage
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Jun 4, 2026

🦞🧹
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.

Re-review progress:

@momothemage
Copy link
Copy Markdown
Contributor Author

@clawsweeper automerge

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Jun 4, 2026

🦞🔧
ClawSweeper saw the passing review, but the PR needs another repair pass before merge.

Source: clawsweeper[bot]
Feedback: structured ClawSweeper verdict: pass (sha=cc6ac9d31a70c7911ab1579fe56bc23b7ee30f4f); current checks are failing: Vercel – clawhub:FAILURE
Action: repair worker queued. Run: https://github.com/openclaw/clawsweeper/actions/runs/26937558404
Model: gpt-5.5

I will update this PR branch, or open a safe credited replacement, if the repair worker finds a narrow CI fix.

Automerge progress:

  • 2026-06-04 04:08:48 UTC review requested repair 454487210a24 (structured ClawSweeper marker: fix-required (finding=security-review sha=454487...)
  • 2026-06-04 04:09:46 UTC review queued 4c96815f26b4 (after repair)
  • 2026-06-04 04:16:20 UTC review passed 4c96815f26b4 (structured ClawSweeper verdict: pass (sha=4c96815f26b4781489386fbc5bd4b49af1926...)
  • 2026-06-04 07:30:40 UTC review passed cc6ac9d31a70 (structured ClawSweeper verdict: pass (sha=cc6ac9d31a70c7911ab1579fe56bc23b7ee30...)

@clawsweeper clawsweeper Bot added the clawsweeper:automerge Maintainer opted this ClawSweeper PR into bounded ClawSweeper-reviewed automerge label Jun 4, 2026
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 4, 2026
@momothemage momothemage merged commit b0ea80d into openclaw:main Jun 4, 2026
19 of 20 checks passed
@momothemage
Copy link
Copy Markdown
Contributor Author

Merged via squash.

Thanks @momothemage!

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

Labels

clawsweeper:automerge Maintainer opted this ClawSweeper PR into bounded ClawSweeper-reviewed automerge merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug delete skill not working

1 participant