fix(db): re-import revives a withdrawn master instead of hiding the fresh parse - #417
Conversation
…resh parse Withdrawing a library master (ADR-030 tombstone) and then re-uploading the same section made the upload vanish: persistParsedSpec's upsert rebuilt the paragraphs and bumped content_version inside the tombstoned row but never cleared withdrawn_at, so the master stayed hidden from listings, project source resolution, and MCP list_library_specs — with no discoverable UUID to restore. Live repro: a master reached content_version 4 while invisible. The conflict update now sets withdrawn_at = NULL: a deliberate re-import is an unambiguous "this master lives again". onboarding_status is still intentionally untouched (a prior finalize stands). The row upsert moves to its own helper (max-lines-per-function); openapi.yaml documents the revival on importLibraryMaster and withdrawSpec. Regression test pins the symptom: 're-import after withdraw: upload landed hidden in the tombstoned row — revives instead (#415)'. Closes #415 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 (2)
📝 WalkthroughWalkthroughThe specs upsert logic in persistParsedSpec now clears withdrawn_at on conflict, reviving previously withdrawn library masters when a matching section is re-imported. A new integration test validates this behavior, and openapi.yaml descriptions for the withdraw and import endpoints are updated to document it. ChangesRevive-on-reimport behavior
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@openapi.yaml`:
- Around line 223-224: The API contract wording is too broad about
restore/upsert identity and should match the actual conflict key. Update the
relevant OpenAPI descriptions in openapi.yaml to say revival/replacement happens
only for the same section/source within the same library, not section alone. Use
the existing restore/import text around the `POST /specs/{id}/restore` and the
section import wording so the contract no longer promises replacement when the
parsed source differs.
In `@src/db/queries/specs.integration.test.ts`:
- Around line 306-316: Harden the regression test in specs.integration.test.ts
by asserting the withdrawn precondition before the re-import path runs. In the
`re-import after withdraw` test, verify `withdrawSpec` actually tombstones the
row by checking its result or querying `withdrawn_at` on the existing spec
before calling `persistParsedSpec` again; then keep the revival assertions
against `persistParsedSpec`, `withdrawSpec`, and the final `SELECT` on `specs`.
Also ensure the test name clearly states the symptom so the regression is pinned
to the tombstoned-row revival behavior.
🪄 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: 085519ce-260e-47ca-a6fe-e2bf9f0820f9
📒 Files selected for processing (3)
openapi.yamlsrc/db/queries/specs.integration.test.tssrc/db/queries/specs.ts
#415 test CodeRabbit review on the #415 fix: - The library re-import upsert revives on the (section, source, library_id) conflict key, not section alone. The restore and import descriptions promised replacement for "the same section", overstating the contract when the parsed source differs. Qualify both as "same section/source" so the authoritative openapi.yaml matches the actual upsert identity. - Harden the #415 regression test: assert withdrawSpec tombstones the row (result shape + withdrawn_at NOT NULL) before re-import, so the test cannot pass via an ordinary same-row reimport if withdrawal ever stops tombstoning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex (GPT-5.5, xhigh) ran as a second adversarial reviewer against |
Why
Withdraw a library master, then upload the same section into the same library again — the import reports success but the spec never reappears. The upsert in
persistParsedSpecrebuilt paragraphs and bumpedcontent_versioninside the tombstoned row without clearingwithdrawn_at, so the fresh parse landed invisibly: hidden fromGET /libraries/:id/specs, project source resolution, and MCPlist_library_specs, with no discoverable UUID to restore (#416 tracks that gap separately). Live repro on the demo DB: ABC Fab's 08 11 13 reachedcontent_version4 while invisible; an MCP chat agent trying to restore it found "no active specs at all".What
ON CONFLICT DO UPDATEnow also setswithdrawn_at = NULL— a deliberate re-import is an unambiguous "this master lives again".onboarding_statusstays untouched on re-import (a prior finalize still stands), andPOST /specs/:id/restoreremains the content-preserving restore.upsertParsedSpecRowhelper (max-lines-per-functioncap).openapi.yaml: revival documented onimportLibraryMasterandwithdrawSpecdescription prose.re-import after withdraw: upload landed hidden in the tombstoned row — revives instead (#415).Testing
POST /specs/:id/restore🤖 Co-authored by Claude Fable 5. Closes #415.
Summary by CodeRabbit
Documentation
Bug Fixes