feat(libraries): includeWithdrawn listing to discover withdrawn masters - #441
Conversation
…rs (#416) Withdrawn library masters (ADR-030) were undiscoverable through the API/MCP: `GET /libraries/:id/specs` and `list_library_specs` hid them with no opt-in, and `POST /specs/:id/restore` / `restore_spec` need a spec UUID that nothing could surface once the master was hidden. A withdrawn master was effectively unrestorable unless its UUID was recorded beforehand. Add an opt-in that closes the browse-and-restore loop, surfacing the UUID `restore` needs: - `GET /libraries/:id/specs?includeWithdrawn=true` returns withdrawn masters; every row now carries `withdrawnAt` (null for active). Default behaviour (withdrawn hidden) is unchanged. - `list_library_specs` gains a parity `includeWithdrawn` param (read tier). - `listLibrarySpecs` takes `includeWithdrawn` (default false); the withdrawn filter becomes `($2 OR withdrawn_at IS NULL)`. - openapi.yaml documents the query param and the `withdrawnAt` field (required, nullable date-time) — keeps both contract gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe library spec listing API and MCP tool now accept ChangesWithdrawn spec discovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant listLibrarySpecsHandler
participant listLibrarySpecs
participant Database
Client->>listLibrarySpecsHandler: Request specs with includeWithdrawn
listLibrarySpecsHandler->>listLibrarySpecs: Forward libraryId and flag
listLibrarySpecs->>Database: Query specs with conditional withdrawal filter
Database-->>listLibrarySpecs: Return withdrawn_at data
listLibrarySpecs-->>Client: Return specs with withdrawnAt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
The sonarjs/no-alphabetical-sort rule rejects a bare .sort() on the string arrays in the includeWithdrawn assertion; supply an explicit localeCompare comparator so ordering is well-defined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/libraries.ts (1)
46-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating
includeWithdrawnwith Zod instead of a raw string comparison.The rest of the handler follows the established pattern correctly (404 on missing library, then call
listLibrarySpecs). However,includeWithdrawnis derived fromreq.queryvia a bare=== 'true'check rather than through a Zod schema.As per coding guidelines: "Validate external input, including request bodies, environment variables, and parsed XML/OOXML, with Zod."
♻️ Proposed refactor
- const includeWithdrawn = req.query['includeWithdrawn'] === 'true'; + const includeWithdrawn = z + .enum(['true', 'false']) + .optional() + .default('false') + .transform((v) => v === 'true') + .parse(req.query['includeWithdrawn']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/libraries.ts` around lines 46 - 55, Validate the includeWithdrawn query parameter in the handler using a Zod schema rather than directly comparing req.query values to 'true'. Define or reuse a schema that parses the query input into a boolean, handle validation failures consistently with the handler’s existing error response pattern, and pass the parsed value to listLibrarySpecs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp/library-management-handlers.ts`:
- Around line 60-64: Update the validation failure branch in the list library
specs handler to use the parsed Zod error via issues(parsed.error), rather than
the fixed “libraryId must be a UUID” message, so failures identify whether
libraryId or includeWithdrawn is invalid.
---
Nitpick comments:
In `@src/api/libraries.ts`:
- Around line 46-55: Validate the includeWithdrawn query parameter in the
handler using a Zod schema rather than directly comparing req.query values to
'true'. Define or reuse a schema that parses the query input into a boolean,
handle validation failures consistently with the handler’s existing error
response pattern, and pass the parsed value to listLibrarySpecs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb00b017-6222-41b9-bf55-ee76d68d2deb
📒 Files selected for processing (7)
openapi.yamlsrc/api/libraries.integration.test.tssrc/api/libraries.tssrc/db/queries/libraries.tssrc/mcp/library-management-handlers.tssrc/mcp/library-management-tools.tssrc/mcp/library-management.integration.test.ts
The handler hard-coded 'libraryId must be a UUID', but the schema now also validates the optional includeWithdrawn boolean; a bad includeWithdrawn reported the wrong field. Use the existing issues() helper so the message names the actual failing field, matching the rename_library and create_client_library handlers in this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex (GPT-5.5, xhigh) adversarial review — 2 P2 findings, 0 P1. Outcomes: [P2] [P2] Codex verdict: approve after the two small cleanups — the SQL uses bound params, the boolean predicate and |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 24 minutes. |
1 participant
Why
Withdrawn library masters (ADR-030 soft-delete/tombstone) were undiscoverable through the API and MCP.
GET /libraries/:id/specsand thelist_library_specstool hid them with no opt-in, whilePOST /specs/:id/restoreandrestore_specrequire the spec UUID — which nothing could surface once the master was hidden. Net effect: a withdrawn master was effectively unrestorable unless its UUID had been recorded beforehand. A live MCP chat session hit exactly this ("I also don't have a tool here to list withdrawn library specs directly by section before restoring").PR #417 already fixed the most common recovery path (re-upload revives the master). This closes the remaining deliberate browse-and-restore discoverability gap.
What
GET /libraries/:id/specs?includeWithdrawn=truereturns withdrawn masters. Every row now carrieswithdrawnAt(nullfor active, ISO-8601 when withdrawn). The default listing still hides withdrawn masters — unchanged behaviour.list_library_specsgains a parityincludeWithdrawnboolean param (still read tier), so an agent can find the UUIDrestore_specneeds.listLibrarySpecstakesincludeWithdrawn(defaultfalse); the withdrawn filter becomes($2 OR withdrawn_at IS NULL), always selectingwithdrawn_at.openapi.yamldocuments the query param and thewithdrawnAtfield (required, nullabledate-time), keeping the REST contract gate green; the MCP contract gate stays green (mapping unchanged,list_library_specsremains anINV5_READ_PENDINGread tool).Design decisions
withdrawnAtis always present on every row (not only whenincludeWithdrawn=true),nullfor active masters. This mirrorsGET /specs/:id, which already surfaceswithdrawnAt: nullfor active masters, and lets oneLibrarySpecschema cover both cases (no discriminated response shape). "Default behaviour unchanged" is read as the set of rows returned is unchanged — the extrawithdrawnAt: nullfield is additive and non-breaking.includeWithdrawnparam onlistLibrarySpecs(libraryId, includeWithdrawn, db), matching the house convention wheredb = poolstays last (e.g.removeSectionFromProject(projectId, specId, force, pool)), rather than an options object.ListLibrarySpecsShapein the MCP layer rather than extending the sharedLibraryIdShape(whichrename_libraryalso spreads), so the new param is scoped to the one tool that uses it.web_ui_demo/is present in this worktree, and a greyed-out/Restore render is an independent, UI-only change. Filing as follow-up keeps this PR one demonstrable change.Testing
pnpm test— 1616 passed)pnpm lint— eslint + tsc + prettier)libraries+ MCPlibrary-management+ REST & MCP contract gates — 51 passed in isolation)withdrawnAt: null;includeWithdrawn=truereturns the withdrawn master with a timestamp and the exact UUIDrestoreneeds🤖 Co-authored by Claude Fable 5. Closes #416.
Summary by CodeRabbit
includeWithdrawn=trueoption to library specification listings.withdrawnAttimestamp for withdrawn entries, ornullfor active entries.