fix(wasteland): add .trim() to forceUnclaimWantedItem reason validation#3159
fix(wasteland): add .trim() to forceUnclaimWantedItem reason validation#3159jrf0110 wants to merge 4 commits intowasteland-staging-v3from
Conversation
…Client (#3150) * feat(wasteland): add Claims page with listClaims procedure and ClaimsClient UI Add listClaims tRPC procedure that filters browseWantedBoard to claimed items, optionally by rigHandle, and enriches each with pending PR info from DoltHub open pulls. Build full ClaimsClient with filter bar, stats strip, claims table, empty/error/loading states, and drawer integration. Extract shared STATUS_COLORS/PRIORITY_COLORS/TYPE_COLORS to status-colors.ts. * fix: address review feedback on PR #3150 - Dismiss toast on unmount in useSlowOperationToast cleanup - Guard claimed_by Link against null (render plain span instead) - Filter empty strings from claimedItemIds set
) * WIP: container eviction save * feat(wasteland): add force unclaim mutation for admin claims page Admin-only forceUnclaimWantedItem tRPC mutation that writes directly to DoltHub wanted table (scratch branch → PR → merge) since wl CLI has no admin-unclaim command. Includes rate limiting, analytics event, and billing meter event. Frontend adds confirmation dialog with required reason textarea, warning callout, and red destructive confirm button. Gated on isOwner (wasteland owner or site admin) or isUpstreamAdmin. * fix(wasteland): address PR #3153 review feedback - Move forceUnclaimWantedItem from adminProcedure to procedure, relying on requireOwnerAccess + isUpstreamAdmin (matches setUpstreamRigTrust pattern and fixes frontend/backend auth mismatch) - Track branchCreated flag and delete scratch branch on merge failure to prevent orphaned branches on DoltHub - Wrap user-controlled reason in blockquote in DoltHub PR description to prevent misleading markdown formatting - Align frontend canForceUnclaim gate with backend (isOwner && isUpstreamAdmin)
* feat(wasteland): add Claims page tRPC tests + docs * fix: address review feedback - remove non-null assertions and as casts in test - Replace pending_pr! with optional chaining (?.) after not.toBeNull() guards - Replace fragile try/catch with .then() pattern that fails if unexpected resolve - Remove 'as TRPCError' cast, use instanceof narrowing instead * fix: collapse double-invocation test to rejects.toMatchObject per review
…nd fix test assertion
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| delivery: 'trpc', | ||
| userId: ctx.userId, | ||
| wastelandId: input.wastelandId, | ||
| label: `${input.itemId}:${targetHandle}:${adminHandle}:${input.reason}`, |
There was a problem hiding this comment.
WARNING: Free-form user text in analytics label
The input.reason field (up to 500 chars of arbitrary user-provided text) is embedded verbatim in the Cloudflare Analytics Engine label blob. While reason is not a credential, it could contain sensitive information the admin typed (e.g. names, internal details). Consider hashing or truncating it, or omitting it from AE entirely — the reason is already preserved in the DoltHub PR description (line 1231), which is the durable audit trail. AE is better suited for structured, non-sensitive labels like itemId:targetHandle:adminHandle.
| if (orphanedPullId) { | ||
| await doltApi.closePull(upstream, token, orphanedPullId).catch(() => {}); | ||
| } | ||
| if (mergeConfirmed) { |
There was a problem hiding this comment.
SUGGESTION: Redundant branch in finally cleanup
Both if (mergeConfirmed) and else if (branchCreated) call identical deleteBranch logic. Since the branch should be deleted in all cases where it was created (whether or not the merge succeeded), this can be simplified:
| if (mergeConfirmed) { | |
| if (branchCreated) { |
This replaces both branches with a single unconditional delete-if-created, which is what both arms do today anyway.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by claude-sonnet-4.6 · 1,128,310 tokens |
1 participant
Summary
Fix whitespace-only reason validation in
forceUnclaimWantedItem:.trim()to thereasonZod schema so whitespace-only strings like' 'are properly rejected (previously passed.min(1)since length ≥ 1).code: 'BAD_REQUEST'(Zod validation error) instead of generictoThrow(), which was passing for the wrong reason (TypeError from undefined mock return).Verification
Ran
pnpm --filter cloudflare-wasteland test— all 10 router tests pass, including the whitespace-only reason test now asserting on the correct error code.Visual Changes
N/A
Reviewer Notes
Defense-in-depth fix — the UI already trims and disables the button, but the API must also reject whitespace-only reasons from direct API calls.