feat(api): spec soft-delete (withdraw/restore/tombstone) — ADR-030 - #313
Merged
Conversation
Implement DELETE /specs/:id as a soft withdrawal of a library master, not a
hard row delete, per ADR-030. Hard-deleting a master would destroy the
chain-of-custody provenance of every derived project copy (ADR-015); a
reversible tombstone preserves it.
- Migration 036: add specs.withdrawn_at timestamptz NULL (reversible up/down).
- DELETE /specs/:id soft-withdraws a master -> 200 { specId, withdrawnAt };
409 on a project copy (use the membership endpoint); 404 unknown; idempotent
re-withdraw returns the original withdrawnAt (COALESCE).
- POST /specs/:id/restore clears the tombstone -> 200; mirrors withdraw on
ownership (409 on a project copy, 404 unknown), idempotent no-op when active.
- Read-path filtering (withdrawn_at IS NULL): listLibrarySpecs, project source
resolution (resolveSection), broken-ref availableFrom, the coordination
present set, and the source-library catalog. GET /specs/:id still surfaces
withdrawnAt so lineage/history resolves.
- openapi.yaml: both routes + withdrawnAt on SpecTree (contract gate green).
- Unit + integration tests: withdraw, restore, 409 project-copy, 404,
idempotency, read-path hiding/surfacing, resolution hide-then-restore.
Closes #310
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 34 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughImplements ADR-030 spec soft-withdrawal: adds a ChangesSpec Withdraw/Restore (ADR-030)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…030) The withdraw read-path invariant "hidden from every listed read path" had boundary tests for listLibrarySpecs and project source resolution, but the broken-ref availableFrom advisory was covered only at the query level. Add an isolated boundary test (own library + master + a project copy carrying a broken ref) asserting availableFrom names the source library while the master is active, drops it once withdrawn, and offers it again after restore. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thewrz
marked this pull request as ready for review
June 28, 2026 05:52
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/api/specs.ts (1)
38-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize independent spec metadata lookups.
styleSource,onboardingStatus, andwithdrawnAtare independent aftergetSpecTree(id)succeeds, so fetching them together avoids extra serial DB round-trip latency onGET /specs/:id.♻️ Proposed refactor
- const styleSource = await getSpecStyleSource(id); - const onboardingStatus = await getOnboardingStatus(id); // A withdrawn master (ADR-030) is still GET-able with its tombstone surfaced // (null when active), so lineage/history resolves. Same sibling-field pattern. - const withdrawnAt = await getSpecWithdrawnAt(id); + const [styleSource, onboardingStatus, withdrawnAt] = await Promise.all([ + getSpecStyleSource(id), + getOnboardingStatus(id), + getSpecWithdrawnAt(id), + ]);🤖 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/specs.ts` around lines 38 - 42, The metadata fetches in the spec lookup flow are still happening serially after getSpecTree(id) succeeds. Update the logic in the spec retrieval path around the getSpecStyleSource, getOnboardingStatus, and getSpecWithdrawnAt calls so they run in parallel (for example by grouping them in the same async await flow) and then assign the resolved values together. Keep the existing behavior and ordering of downstream use in the spec response assembly.src/db/queries/specs.ts (1)
318-321: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip no-op UPDATEs in withdraw/restore
src/db/queries/specs.ts:318-321,357-360
Both CTEs still issueUPDATEon already-withdrawn / already-active masters, so retries create unnecessary row versions/WAL and take extra locks. Adds.withdrawn_at IS NULL/IS NOT NULLguards so the no-op path stays read-only.🤖 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/db/queries/specs.ts` around lines 318 - 321, The withdraw and restore queries in the specs CTEs still perform UPDATEs even when the row is already in the desired state. Update the predicates in the relevant `UPDATE specs s` statements so `withdraw` only targets rows where `s.withdrawn_at IS NULL` and `restore` only targets rows where `s.withdrawn_at IS NOT NULL`, keeping retries as no-op reads instead of writing new row versions.src/api/contract.integration.test.ts (1)
97-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack these operations as response-covered, not allowlisted.
The new withdraw/restore endpoints already have explicit 200-body assertions in
src/api/specs.integration.test.ts, so putting them inRESPONSE_ALLOWLISTweakens this contract meta-test. Move them intoRESPONSE_COVEREDso the suite still fails if those response assertions disappear later.Suggested change
const RESPONSE_COVERED = new Set([ 'delete /projects/{}/revision-nomenclature', + 'delete /specs/{}', 'get /health', 'get /conventions', 'get /libraries', 'get /libraries/{}/specs', 'get /projects', 'get /projects/{}/revision-nomenclature', 'get /revision-nomenclature-profiles', + 'post /specs/{}/restore', 'get /templates', 'post /projects/{}/revision-nomenclature/clone', 'put /projects/{}/revision-nomenclature', ]); @@ const RESPONSE_ALLOWLIST = new Set([ 'delete /packages/{}', 'delete /projects/{}/specs/{}', - 'delete /specs/{}', 'delete /specs/{}/lock', @@ 'patch /projects/{}', 'delete /projects/{}', 'post /projects/{}/restore', - 'post /specs/{}/restore', ]);🤖 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/contract.integration.test.ts` around lines 97 - 98, The withdraw/restore endpoints are currently treated as allowlisted in the contract meta-test, but they already have explicit response-body assertions elsewhere and should be verified as response-covered instead. Update the entries for delete /specs/{} and post /specs/{}/restore in the contract test setup so they are included with RESPONSE_COVERED rather than RESPONSE_ALLOWLIST, keeping the existing explicit coverage in src/api/specs.integration.test.ts as the source of truth.src/api/specs.test.ts (1)
252-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing 500-path test for
restoreSpecHandler.
src/api/specs.tsgivesrestoreSpecHandlerits own catch branch, but this suite never exercises the case whererestoreSpec()rejects. That leaves the new 500 mapping unpinned while the siblingwithdrawSpecHandlersuite already covers the analogous branch.Suggested test
describe('restoreSpecHandler', () => { it('returns 200 with {specId} on a master', async () => { @@ it('returns 400 on a malformed (non-UUID) id', async () => { const { restoreSpecHandler } = await import('./specs.js'); const res = makeRes(); await restoreSpecHandler( { params: { id: 'nope' } } as unknown as Request, res as unknown as Response ); expect(res.status).toHaveBeenCalledWith(400); }); + + it('returns 500 on database error', async () => { + const { restoreSpec } = await import('../db/index.js'); + vi.mocked(restoreSpec).mockRejectedValueOnce(new Error('db down')); + const { restoreSpecHandler } = await import('./specs.js'); + const res = makeRes(); + await restoreSpecHandler( + { params: { id: VALID_ID } } as unknown as Request, + res as unknown as Response + ); + expect(res.status).toHaveBeenCalledWith(500); + }); });🤖 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/specs.test.ts` around lines 252 - 303, Add a missing test in the restoreSpecHandler suite to cover the rejection path from restoreSpec(): mock restoreSpec to reject, invoke restoreSpecHandler, and assert it returns 500 through the existing res helper. Use the existing restoreSpecHandler and restoreSpec symbols in specs.test.ts so the new catch-branch behavior in specs.ts is pinned the same way the withdrawSpecHandler tests cover their analogous error path.
🤖 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.
Nitpick comments:
In `@src/api/contract.integration.test.ts`:
- Around line 97-98: The withdraw/restore endpoints are currently treated as
allowlisted in the contract meta-test, but they already have explicit
response-body assertions elsewhere and should be verified as response-covered
instead. Update the entries for delete /specs/{} and post /specs/{}/restore in
the contract test setup so they are included with RESPONSE_COVERED rather than
RESPONSE_ALLOWLIST, keeping the existing explicit coverage in
src/api/specs.integration.test.ts as the source of truth.
In `@src/api/specs.test.ts`:
- Around line 252-303: Add a missing test in the restoreSpecHandler suite to
cover the rejection path from restoreSpec(): mock restoreSpec to reject, invoke
restoreSpecHandler, and assert it returns 500 through the existing res helper.
Use the existing restoreSpecHandler and restoreSpec symbols in specs.test.ts so
the new catch-branch behavior in specs.ts is pinned the same way the
withdrawSpecHandler tests cover their analogous error path.
In `@src/api/specs.ts`:
- Around line 38-42: The metadata fetches in the spec lookup flow are still
happening serially after getSpecTree(id) succeeds. Update the logic in the spec
retrieval path around the getSpecStyleSource, getOnboardingStatus, and
getSpecWithdrawnAt calls so they run in parallel (for example by grouping them
in the same async await flow) and then assign the resolved values together. Keep
the existing behavior and ordering of downstream use in the spec response
assembly.
In `@src/db/queries/specs.ts`:
- Around line 318-321: The withdraw and restore queries in the specs CTEs still
perform UPDATEs even when the row is already in the desired state. Update the
predicates in the relevant `UPDATE specs s` statements so `withdraw` only
targets rows where `s.withdrawn_at IS NULL` and `restore` only targets rows
where `s.withdrawn_at IS NOT NULL`, keeping retries as no-op reads instead of
writing new row versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cd6c679d-ab2b-430d-96d2-eb2d3af233fe
📒 Files selected for processing (14)
openapi.yamlsrc/api/contract.integration.test.tssrc/api/router.tssrc/api/specs.integration.test.tssrc/api/specs.test.tssrc/api/specs.tssrc/db/index.tssrc/db/migrations/036_add_spec_withdraw.tssrc/db/queries/coordination-implied.tssrc/db/queries/coordination.tssrc/db/queries/derive.tssrc/db/queries/libraries.tssrc/db/queries/project-refs.tssrc/db/queries/specs.ts
Contributor
Author
|
CodeRabbit body nitpicks addressed in b240d0d:
Post-push CodeRabbit re-review is currently rate-limited; local verification completed with lint, build, full unit, and full integration passing. Claude full and fallback reviews did not return usable output within their timeouts. |
3 tasks
thewrz
added a commit
that referenced
this pull request
Jul 1, 2026
#325) * docs(readme): sync capabilities to last month of merged PRs Reflect shipped work in the README's "Included Today", "API Surface", and MCP tool table, validated against the merged diffs and current main: - PDF ingest (text-layer + OCR + font-encoding recovery) accepted by POST /parse (#287, #290, #311) - coordination / E&O report + submittal register (#241, #269, #277, #282, #283, #284) and article-role tagging (#273) - onboarding pipeline: library import, editability review/override, reclassify, finalize/reopen, open-comments (#243, #247, #248, #249, #272) - spec/project soft-delete + restore (#257, #313), document concurrency (#197), revision/addendum manual rendering (#221), numbering profiles (#317, #322) - add missing MCP tools get_numbering_profile, submittal_register, open_comments_report; document GET /docs (Scalar) (#213, #285) - add Example Client pointer to examples/web_ui_demo (#225) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(roadmap): move shipped work to done; re-date to 2026-07-01 Reconcile the roadmap with merged reality (was stamped 2026-06-17). Moved from planned/in-progress to Included, each validated against the diff: - PDF ingest (#287, #290, #311) — remove from "Later" - deep paragraph nesting pr6/pr7 (#215) - revision nomenclature (#216) + revision/addendum manual rendering (#221) — the two "Near Term" Phase 2e items are done - coordination / E&O report, required-sections, article-role, submittal register (#239, #241, #269, #273, #277, #282, #283, #284) — new "Coordination and Semantics" section; removed "coordination report" from planned Phase 4 - onboarding APIs (#243, #247, #248, #249, #272) — API done; UI remains planned - soft-delete/withdraw (#257, #313), section-number format (#266, #271), external-content associations (#242), structural numbering profiles (#317) Kept as planned (foundation only): header/footer composition (#222, #314) and keynote surfacing (#315) — DB/AST exist, no resolution/render/export yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(architecture): reflect merged structural changes Update the architecture spec for shipped work, validated against the diffs and current schema/routes: - Tech Stack + Data Flow: Parse — PDF text-layer (unpdf/pdfjs-dist) + OCR (tesseract.js/@napi-rs/canvas) path and numberingProfileId override (#287, #290, #311, #317; ADR-034, ADR-039) - DB schema — specs.onboarding_status/withdrawn_at, projects.section_number_format /deleted_at/deleted_by, paragraphs.source_facts/classification/ editability_override; "Additional tables" summary for editing_conventions, paragraph_associations, required_sections, keynotes, header_footer_configs, numbering_profiles, revision_nomenclature_profiles (foundation-only tables flagged) (ADR-021/022/023/028/031/032; #187, #242) - new Coordination Report / E&O section (finding vocabulary) and Document Concurrency section (locks/optimistic/lifecycle) (#197, #241, #269, #277, #282, #283, #284; ADR-018, ADR-033/035/036/037) - AST meta.articleRole (#273, ADR-033); API-surface note pointing at the CI-enforced openapi.yaml + GET /docs; refreshed MCP tool list Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The web demo ships a spec-delete UI gated behind the
specDeleteflag (false), which degrades to "not available in this API build" becausemainhas noDELETE /specs/:id. ADR-030 (Accepted) settles the design: deleting a library master must be a reversible tombstone, not a hard row delete. ADR-015's layered chain-of-custody means hard-deleting a master would destroy the provenance of every derived project copy, and a master with clones can't be hard-deleted anyway (parent_spec_idFK is NO ACTION).What
DELETE /specs/:idperforms a soft withdrawal of a library master;POST /specs/:id/restorereverses it.specs.withdrawn_at timestamptz NULL(NULL = active). Reversible up/down. No change toparent_spec_idonDelete; row, paragraphs, and lineage edges stay intact.DELETE /specs/:id→200 { specId, withdrawnAt }; 409 on a project copy (steers toDELETE /projects/:id/specs/:specId); 404 unknown; idempotent re-withdraw returns the originalwithdrawnAt(COALESCE).POST /specs/:id/restore→200 { specId }; idempotent no-op when active; mirrors withdraw on ownership (409 project copy / 404 unknown).withdrawn_at IS NULL):listLibrarySpecs, project source resolution (resolveSection), broken-refavailableFrom, the coordination present set, and the source-library catalog.GET /specs/:idstill surfaceswithdrawnAt(null when active) so lineage/history resolves.openapi.yamlupdated for both routes +withdrawnAtonSpecTree(contract gate green three ways).Design decisions
withdrawn_bycolumn. ADR-030 / the issue specify onlywithdrawn_at. The projects soft-delete (ADR-031) carriesdeleted_by, but ADR-030 deliberately does not — kept minimal to spec. (When the user/auth model lands, feat(api): Phase 5f — authentication + multi-tenant (JWT, org isolation) #43, an actor can be added.)DELETE; it is silent on restore + copies. I mirrored withdraw so the withdraw/restore pair is symmetric and a project copy is consistently the wrong target for either — copies have no withdrawal lifecycle. One-line change if a reviewer prefers a 200 no-op.coordination.ts readPresentfilter is a documented no-op guard.project_specs/package_specsreference project copies (never withdrawn) in today's copy model, so the filter changes nothing today — but ADR-030 enumerates the "coordination present set" as an acceptance criterion, so it is pinned verifiably and stays correct if membership ever points at masters. The meaningful coordination filter is on the source-library catalog (readCatalog), which does ingest masters.readCatalogsource-branch filtered too (slightly beyond the literal enumeration). The coordination report ingests source-library masters in a second place (the implied-related-sections catalog). I filtered it so a withdrawn master is fully absent from coordination output — consistent with ADR-030's "hidden from resolution" intent. Flagging in case a reviewer wants the strictest reading of the enumerated set.content_version/updated_atare not bumped on withdraw/restore. Withdrawal is a lifecycle tombstone, not a content edit; bumpingcontent_versionwould falsely invalidate optimistic-concurrency (edit-gate) and merge base versions.specDeleteflag leftfalse. The demo's delete UI was modelled as a hard delete (cascade, 409-if-pinned). Flipping the flag without re-aligning that UI to soft-withdraw semantics would surface a mismatched UX, so it is a deliberate follow-up — this issue is backend-scoped ("only the spec-level soft withdrawal").Testing
pnpm test— 1190 passed; handler 200/409/404/400/500 mapping +withdrawnAtsurfacing pinned)pnpm test:integration— 705 passed, 0 failed: withdraw, restore, 409 project-copy, 404, idempotency, listing hide/restore, resolution hide-then-restore; ran againstdocker compose up -d postgres+pnpm migrate && pnpm seed)pnpm lintclean (eslint complexity caps,tsc --noEmit, prettier)migrate:down→ column dropped →migrate→ column re-added)🤖 Co-authored by Claude Opus 4.8. Closes #310.
Summary by CodeRabbit
New Features
Bug Fixes
Tests