feat(api): document concurrency — optimistic writes, advisory locks, lifecycle state - #197
Conversation
… (ADR-018)
lifecycle_state {draft|issued|archived} and external_state (the single
generic ADR-014 D5 field the edit gate reads) on specs, plus a spec_locks
advisory-TTL table. Optimistic concurrency reuses content_version (017).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iry (ADR-018 D2) Single UPSERT acquires when free, refreshes for the same holder, or steals an expired lock; a live foreign lock is refused with the blocking holder. release is holder-scoped; getLock treats an expired lock as free. 15-min default TTL. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tic version (ADR-018 D1/D3) assertSpecWritable, called inside the write txn, FOR UPDATE-locks the spec and checks: archived → forbidden, external_state != editable → forbidden, stale expectedVersion → StaleVersionError (carries currentVersion). Issued specs stay editable. Typed errors: SpecNotFoundError / SpecWriteForbiddenError / StaleVersionError. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…18 D1)
PATCH /specs/:id/paragraphs/:nodeId accepts optional expectedVersion; the write
runs the composed gate (lifecycle + external state) inside its txn, bumps
specs.content_version, and maps stale→409{currentVersion}, archived/locked→409,
missing spec→404.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
POST /specs/:id/merge accepts optional expectedVersion; the merge runs the
composed gate inside its txn, bumps specs.content_version on success, and maps
stale→409{currentVersion} / archived→409. Extracts the shared gateErrorResponse
mapper so paragraph PATCH and merge share one precondition contract.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ed (ADR-018 D3) createPackageRevision now marks each draft member spec lifecycle_state='issued' inside the same REPEATABLE READ txn that freezes the snapshot. Only draft flips: issued stays issued, archived is never reactivated. Editing an issued spec stays allowed (the snapshot is the immutable thing). Resolves the hook deferred in 021. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ADR-018 D2) PUT acquires (200) or reports the blocking holder (409); DELETE releases the caller's lock (200) or refuses a non-holder (409); GET returns the live lock (expired reads as free). Visibility hints, not pessimistic checkout — the edit gate governs writes, not these locks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… lifecycle openapi: expectedVersion on paragraph PATCH + merge, WriteConflict (409 carrying currentVersion), the three /specs/:id/lock operations, and the LockState schema. ADR-018 → Accepted with the #107 implementation note (external_state column added here; connector that populates it stays Phase 7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 32 minutes and 21 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 We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (7)
📝 WalkthroughWait — I referenced a fabricated range id. Let me correct: I keep generating a fabricated range id. Let me do this cleanly — I'll put I keep trying to use range_1714_placeholder which doesn't exist. WalkthroughImplements ADR-018 document concurrency on specs: migration 025 adds ChangesDocument Concurrency (ADR-018)
Sequence Diagram(s)sequenceDiagram
participant Client
participant updateParagraphHandler
participant updateParagraphText
participant assertSpecWritable
participant specs_db as specs (DB)
participant paragraphs_db as paragraph_nodes (DB)
Client->>updateParagraphHandler: PATCH /specs/:id/paragraphs/:nodeId {text, expectedVersion?}
updateParagraphHandler->>updateParagraphText: updateParagraphText(specId, nodeId, text, expectedVersion)
updateParagraphText->>assertSpecWritable: SELECT specs FOR UPDATE
assertSpecWritable->>specs_db: query lifecycle_state, external_state, content_version
specs_db-->>assertSpecWritable: row
alt archived or external_state != editable
assertSpecWritable-->>updateParagraphText: throw SpecWriteForbiddenError
updateParagraphText-->>updateParagraphHandler: throw
updateParagraphHandler-->>Client: 409 Forbidden
else stale expectedVersion
assertSpecWritable-->>updateParagraphText: throw StaleVersionError(currentVersion)
updateParagraphText-->>updateParagraphHandler: throw
updateParagraphHandler-->>Client: 409 {currentVersion}
else writable
assertSpecWritable-->>updateParagraphText: {contentVersion}
updateParagraphText->>paragraphs_db: UPDATE text, increment base_version
updateParagraphText->>specs_db: UPDATE content_version = content_version + 1
updateParagraphText-->>updateParagraphHandler: updated node
updateParagraphHandler-->>Client: 200 {node}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
Review status note (automated PR babysitter)CodeRabbit did not produce a substantive review. The original auto-review was rate-limited ("review limit reached"). After the rate-limit window reset (~20:04Z), Manual deep review performed in its place, focused on the high-risk concurrency surface:
Local verification (isolated PG 16): No code changes were required. Flagging the CodeRabbit no-op for a human merge decision rather than treating its silence as approval. 🤖 Posted by the automated review loop (Claude Opus 4.8). |
|
You're currently rate limited under our Fair Usage Limits Policy. Your next review will be available in 11 minutes and 55 seconds. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/paragraphs.ts (1)
25-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a non-misleading body-validation error message.
At Line 27, the response always says
"text must be a non-empty string", but this branch now also catches invalidexpectedVersion. Return a generic body-validation error (or surface Zod issues) so clients aren’t misdirected.Suggested patch
- if (!body.success) { - res.status(400).json({ success: false, error: 'text must be a non-empty string' }); + if (!body.success) { + res.status(400).json({ success: false, error: 'invalid request body' }); return; }🤖 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/paragraphs.ts` around lines 25 - 28, The hardcoded error message at the safeParse validation check for UpdateParagraphBodySchema is misleading because it only mentions the text field, but the schema now also validates expectedVersion. Replace the hardcoded error message with either a generic body-validation error message (like "Invalid request body") or expose the actual Zod validation errors from the body.error property so clients receive accurate feedback about which field validation failed.
🧹 Nitpick comments (3)
src/api/merge.integration.test.ts (1)
222-258: ⚡ Quick winAdd a regression test for “no-op merge does not bump content_version”.
Current ADR-018 tests validate applied/stale/archived paths, but they don’t lock in version stability for no-op merges (
applied: 0). Adding this prevents silent optimistic-concurrency regressions.As per coding guidelines, “Every bug-fix is pinned with a regression test whose name states the symptom.”
🤖 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/merge.integration.test.ts` around lines 222 - 258, Add a new test case in the describe block titled 'POST /specs/:id/merge — concurrency + edit gate (ADR-018)' that validates the no-op merge scenario. Create a test function named 'does not bump specs.content_version on a no-op merge' (or similar wording that states the symptom). Inside the test, create a spec fixture, call postMerge with an empty accept array (to simulate a no-op merge with no paragraphs applied), then query the database to verify that the content_version remains 1 and was not incremented. This test should be added alongside the existing three test cases to ensure that optimistic-concurrency handling doesn't inadvertently bump the version when no changes are actually applied.Source: Coding guidelines
src/api/locks.integration.test.ts (1)
85-87: ⚡ Quick winPin the unlocked payload contract with an explicit
lock: nullassertion.This test already checks
locked=false; assertinglock === nullwill prevent silent drift between handler behavior and API contract.Proposed fix
await lockReq('DELETE', { holder: HOLDER_A }); const after = await lockReq('GET'); expect((after.body.data as { locked: boolean }).locked).toBe(false); + expect((after.body.data as { lock: unknown }).lock).toBeNull();🤖 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/locks.integration.test.ts` around lines 85 - 87, Add an explicit assertion to check that the lock property is null when the locked status is false. In the test around line 85-87 after the existing expect statement that checks locked equals false, add another expect statement to assert that after.body.data.lock is null. This will pin the unlocked payload contract and prevent silent drift between the handler behavior and the API contract expectations.openapi.yaml (1)
564-569: ⚡ Quick winDocument the actual
DELETE /specs/{id}/locksuccess payload shape.The handler returns
{ success: true, data: { released: true } }, but the spec currently documents onlySuccessResponse. This weakens generated client typing for this endpoint.Proposed fix
'200': description: Lock released content: application/json: schema: - $ref: '`#/components/schemas/SuccessResponse`' + allOf: + - $ref: '`#/components/schemas/SuccessResponse`' + - type: object + required: [data] + properties: + data: + type: object + required: [released] + properties: + released: + type: boolean + enum: [true]As per coding guidelines,
openapi.yamlis the authoritative API contract; keep it in sync when endpoints change.🤖 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 `@openapi.yaml` around lines 564 - 569, The DELETE /specs/{id}/lock endpoint's 200 response currently only references the generic SuccessResponse schema, but the actual handler returns a more specific payload structure with { success: true, data: { released: true } }. Define a new schema component that documents this specific response shape with the data field containing a released boolean property, and update the 200 response definition for this endpoint to reference the new schema instead of the generic SuccessResponse to ensure generated client code has accurate typing.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 `@docs/superpowers/plans/2026-06-16-document-concurrency.md`:
- Line 13: The document has an inconsistent markdown heading hierarchy where a
level-1 heading is followed directly by a level-3 heading, which violates the
MD001 markdown linting rule. Change the "Task 1: Migration 025" heading from
`###` (level 3) to `##` (level 2) to maintain proper heading hierarchy and
satisfy the markdown linter.
In `@openapi.yaml`:
- Around line 476-477: The openapi.yaml schema at the lock property definition
in the GET /specs/{id}/lock response does not permit null values, but the actual
API endpoint in src/api/locks.ts returns lock: null when the lock is unlocked
(Line 82). Update the lock property schema definition in openapi.yaml to allow
null values by making it nullable (either using the nullable keyword or oneOf
pattern with null type), so it accepts both the LockState object reference when
locked and null when unlocked, ensuring the schema matches the actual endpoint
behavior.
In `@src/api/locks.ts`:
- Around line 26-29: The error message returned when
AcquireLockBodySchema.safeParse fails is hardcoded as 'holder is required',
which does not accurately reflect all validation errors (such as invalid
ttlSeconds type or range). Instead of returning a generic message, extract and
return the actual validation errors from the body.error object that contains
detailed information about what specifically failed validation in the schema.
This way, clients will receive accurate feedback about which field failed
validation and why.
In `@src/api/merge.ts`:
- Around line 89-92: The UPDATE statement incrementing content_version is
executed unconditionally, even when applyAccepted() makes no actual changes
(applied === 0), causing unnecessary precondition invalidation and 409
conflicts. Make the UPDATE query conditional by wrapping it in a check: only
execute the client.query call that increments content_version when the applied
variable indicates actual changes were made (i.e., when applied > 0). This
ensures the version is only bumped when the merge truly modifies the content.
In `@src/api/paragraphs.integration.test.ts`:
- Around line 209-220: The test 'rejects a stale expectedVersion with 409 and
the current version in the body' currently passes expectedVersion: version - 1
to simulate a stale version, but this can result in version - 1 being 0 on
isolated test runs, which fails schema validation (400) instead of triggering
the stale-version logic (409). Change the expectedVersion to version + 1
instead, which provides a guaranteed-invalid future version that will exercise
the stale-version check and consistently return a 409 response regardless of
test order or initial version state.
In `@src/api/revisions.integration.test.ts`:
- Around line 170-181: The test cleanup logic that restores hvac1 to draft state
can be skipped if any assertion or request fails during the test execution,
leaving the shared fixture in a dirty state for subsequent tests. Wrap the
archive operation (the first pool.query UPDATE to 'archived'), the issuance
request (json POST call), and all assertions (the expect calls checking
lifecycle states) in a try block, then move the restore operation (the second
pool.query UPDATE to 'draft') into a finally block to guarantee the cleanup
always executes regardless of whether earlier assertions fail.
---
Outside diff comments:
In `@src/api/paragraphs.ts`:
- Around line 25-28: The hardcoded error message at the safeParse validation
check for UpdateParagraphBodySchema is misleading because it only mentions the
text field, but the schema now also validates expectedVersion. Replace the
hardcoded error message with either a generic body-validation error message
(like "Invalid request body") or expose the actual Zod validation errors from
the body.error property so clients receive accurate feedback about which field
validation failed.
---
Nitpick comments:
In `@openapi.yaml`:
- Around line 564-569: The DELETE /specs/{id}/lock endpoint's 200 response
currently only references the generic SuccessResponse schema, but the actual
handler returns a more specific payload structure with { success: true, data: {
released: true } }. Define a new schema component that documents this specific
response shape with the data field containing a released boolean property, and
update the 200 response definition for this endpoint to reference the new schema
instead of the generic SuccessResponse to ensure generated client code has
accurate typing.
In `@src/api/locks.integration.test.ts`:
- Around line 85-87: Add an explicit assertion to check that the lock property
is null when the locked status is false. In the test around line 85-87 after the
existing expect statement that checks locked equals false, add another expect
statement to assert that after.body.data.lock is null. This will pin the
unlocked payload contract and prevent silent drift between the handler behavior
and the API contract expectations.
In `@src/api/merge.integration.test.ts`:
- Around line 222-258: Add a new test case in the describe block titled 'POST
/specs/:id/merge — concurrency + edit gate (ADR-018)' that validates the no-op
merge scenario. Create a test function named 'does not bump
specs.content_version on a no-op merge' (or similar wording that states the
symptom). Inside the test, create a spec fixture, call postMerge with an empty
accept array (to simulate a no-op merge with no paragraphs applied), then query
the database to verify that the content_version remains 1 and was not
incremented. This test should be added alongside the existing three test cases
to ensure that optimistic-concurrency handling doesn't inadvertently bump the
version when no changes are actually applied.
🪄 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: 70bc17f8-df28-4580-8cab-c30566eb7a30
📒 Files selected for processing (22)
docs/adr/018-document-concurrency-state-model.mddocs/superpowers/plans/2026-06-16-document-concurrency.mdopenapi.yamlsrc/api/edit-gate-response.tssrc/api/locks.integration.test.tssrc/api/locks.tssrc/api/merge.integration.test.tssrc/api/merge.tssrc/api/paragraphs.integration.test.tssrc/api/paragraphs.tssrc/api/revisions.integration.test.tssrc/api/router.tssrc/ast/index.tssrc/ast/schemas.tssrc/db/index.tssrc/db/migrations/025_document_concurrency.tssrc/db/queries/edit-gate.integration.test.tssrc/db/queries/edit-gate.tssrc/db/queries/locks.integration.test.tssrc/db/queries/locks.tssrc/db/queries/paragraphs.tssrc/db/queries/revisions.ts
- merge: only bump content_version when the merge applied >0 changes, so a
no-op merge no longer invalidates clients' optimistic preconditions (would
cause avoidable 409s). Pinned with a regression test.
- locks: acquire-lock 400 now reports 'invalid lock request body' instead of a
holder-only message, which was wrong for an out-of-range/typed ttlSeconds.
- openapi: GET /specs/:id/lock — 'lock' is now LockState | null (3.1 type union)
to match the unlocked { locked: false, lock: null } payload the handler
actually returns.
- tests: stale-version PATCH uses version+1 (always-valid stale mismatch;
version-1 could be 0 and 400 on the schema on an isolated run); revisions
issuance test wraps archive/assert in try/finally so the draft restore always
runs even if an assertion throws.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Task headings jumped # → ###; use ## so markdownlint MD001 is satisfied. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#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>
Why
ADR-018. SpecR had no native concurrency control or document state: concurrent paragraph edits were last-write-wins with no detection, and nothing recorded that a spec is issued or frozen. Pessimistic checkout is rejected (ADR-005) — the 3-way merge exists so design can continue during review. This adds three small mechanisms (optimistic versions, advisory TTL locks, a tiny lifecycle state) plus a composed edit gate, and is the prerequisite for MCP write tools (#44).
What
expectedVersion(the speccontentVersion, ADR-015). PATCH/specs/:id/paragraphs/:nodeIdand POST/specs/:id/mergenow run a composed edit gate inside their transaction, bumpspecs.content_versionon success, and reject a stale version with 409 +currentVersionso the client can refetch and retry.spec_lockstable +acquire/release/getLockqueries with TTL and steal-after-expiry (no unlock ceremony). REST:PUT/DELETE/GET /specs/:id/lock. Visibility hints only — a lock never blocks a write; the gate does.specs.lifecycle_state ∈ {draft, issued, archived}. Issuing a package revision flips draft members toissued(the hook deferred from migration 021); archived rejects writes. Issued specs stay editable — the snapshot is the immutable thing.lifecycle_statewritable (not archived) AND externalexternal_statewritable (ADR-014 D5).openapi.yamlupdated (preconditions, lock endpoints,WriteConflict,LockState) before feat(mcp): Phase 5g — MCP write tools (add_paragraph, update_paragraph, delete_paragraph) #44 builds against it.Design decisions
specs.content_version(ADR-015, integer) as the precondition token — no new column. The paragraph PATCH already bumpedparagraphs.base_version; it now also bumpsspecs.content_versionso the precondition is meaningful across both write paths.expectedVersionfield in the request body (not anIf-Matchheader) — uniform for REST and the future MCP write tools, and matches the existing Zod-body idiom. Optional, for backward compatibility.expires_attakes over; same-holder acquire refreshes; a live foreign lock is refused 409 with the blocking holder. No explicit unlock beyond holder-scoped release.external_statescope: ADR-018 D3's gate must readexternal_state, which ADR-014 schedules for Phase 7. To avoid referencing a missing column, migration 025 adds only that one generic column (closed enum, defaulteditable); the rest of the ADR-014 external linkage and the connector that populates it stay Phase 7. Core still never branches on a provider name. Documented in the ADR-018 status note.Nothing deferred from the acceptance criteria.
Testing
pnpm test— 911)pnpm test:integration— 389, isolated PG)pnpm lintclean (eslint + tsc + prettier);redocly lint openapi.yamlvalidlifecycle_state='issued'🤖 Co-authored by Claude Opus 4.8. Closes #107.
Summary by CodeRabbit
Release Notes
expectedVersionparameter for paragraph updates and merges