RFC: Custody Acknowledgement — Implementation Plan - #2470
Open
carlosvirreira wants to merge 33 commits into
Open
RFC: Custody Acknowledgement — Implementation Plan#2470carlosvirreira wants to merge 33 commits into
carlosvirreira wants to merge 33 commits into
Conversation
added 28 commits
March 31, 2026 12:31
Adds a detailed implementation plan for the custody acknowledgement feature — verifiable proof that custodians received assigned assets. 12 phases covering: schema changes, JWT-based acceptance links, email templates, public acceptance page, asset index column, in-app acknowledgement for logged-in users, and feature gating. Plan is for review prior to implementation.
not separate addon Update plan to make clear that custody acknowledgement is included in Plus/Team plans by default, not sold as a separate addon. Free tier users see upgrade prompt.
- Add 30-day token expiry + rotation on resend via tokenIssuedAt field (rejects old tokens) - Token decoded ID is sole authority for DB ops, URL param is routing only - Privacy: raw IP/UA only on ephemeral Custody fields, activity notes store summary without raw PII - Narrow public path: /accept-custody/:custodyId - Two explicit auth modes: public token vs in-app session - Add acknowledgementBatchId for bulk assign grouping - Persist decline state: declinedAt + declineReason fields - Fix markdownlint MD040: add language to fenced blocks
- CRITICAL: use dedicated CUSTODY_TOKEN_SECRET instead of reusing INVITE_TOKEN_SECRET (token confusion vuln) - Token rotation check moved inside verifier function - Add verifyBatchAcknowledgementToken for bulk flow - Add PII retention policy: 2-year auto-cleanup job - Add explicit error handling table for all edge cases - Loader branches on token purpose (single vs batch) - Org-scoped in-app acknowledgement prevents cross-org - Privacy notice added to email template spec
- Standardize "add-on" spelling in user-facing copy - Fix time unit mismatch: convert Prisma DateTime to Unix seconds before comparing to JWT iat (RFC 7519) - Add token rotation check to batch verifier (was missing, old batch links would remain valid) - Use pg-boss queue for notification emails after DB commit (not inline sends) to prevent partial success
- Scope batch tokens per-custodian: verifier enforces single teamMemberId membership across batch records - Add rate limiting on resend/copy-link: 60s cooldown per custody using tokenIssuedAt timestamp check
- Add @relation FK constraint on assignedByUserId with onDelete: SetNull for referential integrity - Document batch-per-custodian scoping in design decisions table explicitly
- Mutually exclusive state transitions: accept/decline use conditional WHERE (both null) to prevent races, idempotent on retry - Fallback admin notification routing to org owner when assignedByUserId is null (user deleted) - Release notes distinguish pending vs disputed vs acknowledged custody states
- CRITICAL: generateBatchAcknowledgementToken now async (was sync but writes to DB) - CRITICAL: decline uses same conditional WHERE guard as accept (prevents accept/decline race) - P1: checkbox coercion — z.union + transform for "on" string from FormData (follows column visibility pattern) - P1: add verifyKitCustodyAcknowledgementToken with purpose "custody-ack-kit" fetching from KitCustody - P2: include declinedAt in asset index loader select (needed for disputed status rendering) - P2: single owner for email side effects — service is DB-only, route action enqueues after commit - Atomic resend cooldown via conditional updateMany (TOCTOU-safe, no read-then-write) - Fix duplicate section numbering (two 3.3 sections)
and token leak mitigations - Batch token: DB write (tokenIssuedAt) completes BEFORE JWT is signed, eliminating revocation window - Token-in-URL leak mitigations: Referrer-Policy header, log redaction guidance, expiry limits exposure - Skip "addon" spelling comment — refers to code identifiers matching existing codebase conventions
Custody hard-delete already purges IP/UA on release. Activity notes contain no PII. Scheduled cleanup for long-lived custodies is trivial to add later if needed. Added explicit deferral note with reasoning so automated reviewers understand this is intentional, not missing.
logger redaction - Replace tokenIssuedAt (DateTime) with tokenVersion (monotonic Int). Eliminates same-second rotation race. JWT embeds ver claim, verifier checks integer equality. - Add logger.ts to modified files list — redact token query param on /accept-custody routes (implementation step, not just guidance) - Atomic cooldown uses updatedAt + tokenVersion increment
normalization and cooldown cleanup - Batch token: SET all rows to MAX(tokenVersion)+1 instead of increment. Normalizes divergent versions from prior per-asset resends. - Remove unreachable tokenVersion:0 cooldown bypass — initial token generation already sets version >= 1. - Skip addon spelling comment (code identifiers).
- CRITICAL: fix sign-then-increment self-invalidation. Now strictly increment-then-sign: atomic UPDATE RETURNING gives new version, JWT signed with it. - Batch rotation: SELECT FOR UPDATE lock prevents concurrent resend from minting same-version tokens. - DB CHECK constraint: NOT(acceptedAt IS NOT NULL AND declinedAt IS NOT NULL) on Custody and KitCustody. Defense-in-depth beyond app logic.
- Fix stray code fence at line 62 (markdown lint) - Replace updatedAt cooldown with dedicated lastTokenRotatedAt field (not coupled to unrelated custody writes) - Fix batch rotation: SELECT rows FOR UPDATE first, then compute MAX in app code. Postgres cannot lock rows via aggregate queries.
- CUSTODY_TOKEN_SECRET lazy-loaded, not required at startup. Assert at callsite only. Phased rollout safe. - declineReason: VarChar(500), trimmed, validated via z.string().trim().max(500).optional() - Resend cooldown guards against terminal states: requires requiresAcceptance=true, acceptedAt=null, declinedAt=null. No resend for settled custody.
- Use getEnv with isOptional flag for CUSTODY_TOKEN_SECRET (follows codebase secret pattern, doesn't crash startup) - Replace updateMany with raw UPDATE RETURNING for resend cooldown (atomically increments + returns new tokenVersion in single query, no drift)
- recordCustodyAcknowledgement returns { transitioned }
flag so callers skip notes/emails on retry (prevents
duplicates on refresh/double-submit)
- Cooldown uses NOW() - INTERVAL '60 seconds' in SQL
instead of app-calculated timestamp (clock-skew safe)
- Standardize "add-on" spelling in prose
- Fix inaccurate "no PII" wording: custodian name IS personal data. Reworded to "no sensitive network metadata". Added legitimate interest basis for name retention in business records. - Fix inconsistency: design decisions table said updateMany but implementation uses raw SQL UPDATE RETURNING. Aligned to single mechanism.
- Notes/emails gated behind transitioned/declined flag (prevents duplicates on idempotent retry/refresh) - Resend API now batch/kit-aware: rotates ALL rows in group atomically, never single row from a group - Server-side validateCustodyAcknowledgementEnabled on action (prevents free-tier bypass via crafted form) - Kit resend rotates KitCustody + child rows together
- Block single-row resend when custody belongs to batch or kit (prevents version desync that breaks group verification) - Add inverse User relation note for assignedBy (Prisma requires opposite relation field) - Add generateKitCustodyAcknowledgementToken spec (was missing — had verifier but no generator) - Align Phase 3 batch generation with Phase 11: single atomic UPDATE with subquery, no app-side MAX - Standardize "add-on" spelling in remaining prose
- SECURITY: verify JWT signature before reading purpose (jwt.decode is forgeable, must jwt.verify first) - Batch resend rotates ALL rows including settled ones (verifier checks all versions, accepted rows with old version would break batch token) - Add index on acknowledgementBatchId + partial indexes for pending acknowledgement queries - Fix addon spelling in remaining prose lines
- Validate CUSTODY_TOKEN_SECRET at feature enablement boundary (not just callsite). Prevents orgs from entering broken state with 500s at runtime. - Guard against zero rows in batch token generation and batch resend before signing JWT.
fixed before opening PR - Fix verifier architecture: verify-once-then-dispatch. verifyTokenSignature() validates JWT once, passes verified payload to purpose-specific verifiers. No double-verification, no unverified branching. - Flesh out kit resend (was a stub): full SQL with transaction, cooldown, zero-row guards on both KitCustody and child Custody updates - Add server-side validateCustodyAcknowledgementEnabled explicitly to kit (6.2) and bulk (6.3) actions - Add zero-row guards to kit token generator - Fix all section numbering (no duplicates, no 3.3b)
- CRITICAL: add zero-row guard on single resend before signing (was present for batch/kit, missing for single) - Add DB CHECK constraint: requiresAcceptance must be true for acceptedAt/declinedAt to be set - Batch cooldown uses MAX(lastTokenRotatedAt) not MIN (MIN allowed bypass when oldest row exceeded cooldown) - Add zero-row guard to single token generation spec
Rewrote entire plan from scratch incorporating all 21 issues from adversarial audit + 5 rounds of PR review: CRITICAL: - getEnv uses isRequired:false (not isOptional) - KitCustody.custodianId vs Custody.teamMemberId documented with explicit schema blocks - User model inverse relation added - KitCustody gets full Prisma code block - Batch generation uses pg_advisory_xact_lock - JWT pinned to HS256 + issuer/audience claims MAJOR: - Fixed all cross-references (3.9→3.11) - Batch cooldown: removed incorrect MIN comment - Kit resend guard checks KitCustody existence - createAcknowledgementNote now async - Batch subquery uses COALESCE + requiresAcceptance - Kit resend filters by custodianId - Added declineReason→declinedAt CHECK constraint - CREATE INDEX CONCURRENTLY for production - Removed unchanged file from modified list MINOR: - tokenVersion 0 semantics documented - VerifiedCustodyPayload type defined - Valid SQL in pseudocode examples - Consistent wireframe dates - _auth+ prefix precedent documented
CRITICAL: - All resend queries now scope to organizationId via asset/kit join (multi-tenant boundary at query level) MAJOR: - Cache-Control: no-store on acceptance page responses - Kit child cascade guards pending state - Batch cooldown subqueries include requiresAcceptance and organizationId filters
strategy for token generation vs resend paths
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements a custody acknowledgement feature: DB schema fields and constraints, org feature toggle, purpose-scoped/versioned JWT tokens with rotation and batch semantics, public token-auth accept route, email/resend flows, UI surfaces, and operational/token-leak mitigations. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI
participant Server as App Server
participant DB as Database
participant Queue as pg-boss
participant Email as Email Service
participant Custodian as Custodian
Admin->>Server: create/assign custody (single/kit/batch, requireAcknowledgement)
Server->>DB: insert/update Custody/KitCustody (ackBatchId, tokenVersion)
Server->>Server: persist-before-sign -> rotate/persist tokenVersion
Server->>Server: generate purpose-scoped JWT (HS256)
Server->>Queue: enqueue send-email job (after commit)
Queue->>Email: send acknowledgement email with token link
Email->>Custodian: deliver email
Custodian->>Server: GET /accept-custody/:custodyId?token=JWT
Server->>Server: verify JWT (purpose, issuer/aud, expiry, tokenVersion)
alt token valid
Server->>DB: conditional idempotent update to set acceptedAt/declinedAt and metadata
DB-->>Server: success / conflict
Server->>Queue: enqueue admin notification
Server-->>Custodian: render confirmation
else token invalid/expired/revoked/rotated/already-settled
Server-->>Custodian: render error/expired message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.claude/plans/custody-acknowledgement.md (1)
33-34: ConstrainacceptanceIp/acceptanceUserAgentfield lengths to avoid unbounded storage.Using unconstrained
String?for user-agent/IP can create avoidable bloat and noisy data. Add practical DB bounds (@db.VarChar) in bothCustodyandKitCustody.Suggested schema adjustment
- acceptanceIp String? - acceptanceUserAgent String? + acceptanceIp String? `@db.VarChar`(64) + acceptanceUserAgent String? `@db.VarChar`(1024)Also applies to: 129-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 33 - 34, Update the schema for the Custody and KitCustody models by constraining the acceptanceIp and acceptanceUserAgent fields to fixed varchar sizes instead of unconstrained String?; change both acceptanceIp fields to String? with `@db.VarChar`(45) (to cover IPv4/IPv6) and both acceptanceUserAgent fields to String? with a practical bound like `@db.VarChar`(512) to prevent unbounded storage, and run your migration so the database columns are altered accordingly (also apply the same edits referenced around the other occurrence at lines 129-130).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 318-320: The initial kit token flow updates "all child Custody
records" without scoping by custodian, which can affect unrelated rows; modify
the update in the initial token generation to include the same guard used in the
kit resend path (teamMemberId = custodianId) — i.e., add WHERE kitId = <kitId>
AND teamMemberId = custodianId (or equivalent custodianId param) when updating
Custody rows; after executing the UPDATE, check the affected row count and throw
ShelfError("No child custody records found") if zero; ensure you reference the
same identifiers used elsewhere (custodianId, teamMemberId, kitId, and
ShelfError) so behavior matches the kit resend path logic.
- Around line 919-920: The request contract must enforce that exactly one of
custodyId, acknowledgementBatchId, or kitCustodyId is provided; update the API
request schema/DTO validation for the resend endpoint to validate a single
mutually-exclusive field (use a JSON Schema oneOf/xor rule or equivalent
validator for the request DTO) and return a clear 400 error when zero or more
than one are present, referencing the three fields custodyId,
acknowledgementBatchId, and kitCustodyId in the validation message.
---
Nitpick comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 33-34: Update the schema for the Custody and KitCustody models by
constraining the acceptanceIp and acceptanceUserAgent fields to fixed varchar
sizes instead of unconstrained String?; change both acceptanceIp fields to
String? with `@db.VarChar`(45) (to cover IPv4/IPv6) and both acceptanceUserAgent
fields to String? with a practical bound like `@db.VarChar`(512) to prevent
unbounded storage, and run your migration so the database columns are altered
accordingly (also apply the same edits referenced around the other occurrence at
lines 129-130).
🪄 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
Run ID: 69ccacf2-65b7-489d-86d5-f04d176f7948
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.claude/plans/custody-acknowledgement.md (1)
236-236: Minor wording cleanup for hyphenated compounds.Use hyphenation for compound modifiers (“add-on”, “kit-level”/“child-custody” style) to keep wording consistent with the rest of the doc.
Also applies to: 1141-1141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md at line 236, Update compound modifiers for consistency: change the text fragment "**Sell as separate add-on**" to use correct hyphenation/grammar (e.g., "**Sell as a separate add-on**" and ensure "add-on" is hyphenated), and scan the document for similar compounds such as "kit level" and "child custody" and convert them to "kit-level" and "child-custody" respectively so compound modifiers are consistently hyphenated (also apply the same fixes to the other occurrence noted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 169-177: The rollout plan omits the required non-transactional
step to create the partial indexes; add an explicit separate step (outside
Prisma migrations) to run CREATE INDEX CONCURRENTLY for idx_custody_pending_ack
and idx_custody_batch_pending on the "Custody" table (the partial WHERE clause
for requiresAcceptance/acceptedAt/declinedAt must be preserved), and update the
build/test sequence and deployment checklist to run this non-transactional
migration (or a raw SQL script) before switching to the new schema so that
Prisma’s transactional migrations are not used for these indexes.
- Around line 426-433: The examples for recordCustodyAcknowledgement (and
recordCustodyDecline) omit tenant scoping: update the conditional SQL/ORM
conditional update statements used by recordCustodyAcknowledgement and
recordCustodyDecline to include organizationId in the WHERE clause (e.g., WHERE
id = :custodyId AND state = 'PENDING' AND organization_id = :organizationId) so
the transition is tenant-scoped, or alternatively remove organizationId from the
function signature and all callers if multi-tenant scoping is not required;
ensure the unique identifiers custodyId and organizationId are referenced in the
conditional update logic and update any example docs/snippets to match.
- Around line 318-321: The SQL snippet uses an undefined identifier kit.id;
replace references to kit.id with an explicit subquery that derives the kit id
from the provided kitCustodyId (e.g. (SELECT "kitId" FROM "KitCustody" WHERE
"id" = kitCustodyId)) so the UPDATE against "Custody" (setting "tokenVersion" =
newVersion and scoped by "assetId" IN (SELECT "id" FROM "Asset" WHERE "kitId" =
... ) and by "teamMemberId" = (SELECT "custodianId" FROM "KitCustody" WHERE "id"
= kitCustodyId)) is copy-safe and does not rely on an undefined kit variable;
apply the same change to the other occurrences (lines referencing kit.id).
---
Nitpick comments:
In @.claude/plans/custody-acknowledgement.md:
- Line 236: Update compound modifiers for consistency: change the text fragment
"**Sell as separate add-on**" to use correct hyphenation/grammar (e.g., "**Sell
as a separate add-on**" and ensure "add-on" is hyphenated), and scan the
document for similar compounds such as "kit level" and "child custody" and
convert them to "kit-level" and "child-custody" respectively so compound
modifiers are consistently hyphenated (also apply the same fixes to the other
occurrence noted).
🪄 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
Run ID: 2579853e-0081-4289-b644-3d6441169002
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.claude/plans/custody-acknowledgement.md (3)
318-321:⚠️ Potential issue | 🟠 MajorReplace undefined
kit.idwith akitCustodyId-derived subquery.Line 320 and Line 1027 still use
kit.id, which is not defined in these snippets and is easy to implement incorrectly.Suggested plan-text patch
-// WHERE "assetId" IN (SELECT "id" FROM "Asset" WHERE "kitId" = kit.id) +// WHERE "assetId" IN ( +// SELECT "id" FROM "Asset" +// WHERE "kitId" = (SELECT "kitId" FROM "KitCustody" WHERE "id" = kitCustodyId) +// )Also applies to: 1026-1028
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 318 - 321, The SQL uses an undefined kit.id in the UPDATE for "Custody"; change the WHERE to derive the kit id from kitCustodyId via a subquery: replace "assetId" IN (SELECT "id" FROM "Asset" WHERE "kitId" = kit.id) with "assetId" IN (SELECT "id" FROM "Asset" WHERE "kitId" = (SELECT "kitId" FROM "KitCustody" WHERE "id" = kitCustodyId)), and ensure the custodian scoping still uses teamMemberId = (SELECT "custodianId" FROM "KitCustody" WHERE "id" = kitCustodyId) so the UPDATE on "Custody" (setting "tokenVersion" = newVersion) is correctly scoped by the KitCustody-derived kit id and custodian.
426-435:⚠️ Potential issue | 🟠 MajorAlign state-transition WHERE clauses with
organizationIdparam.Line 431 and Line 471 include
organizationIdin params, but Line 434 and Line 474 examples only key onid+ state flags. This mismatch can lead to tenant-scope omission in implementation.Suggested plan-text patch
-// WHERE id = custodyId AND acceptedAt IS NULL AND declinedAt IS NULL +// WHERE id = custodyId +// AND "assetId" IN (SELECT "id" FROM "Asset" WHERE "organizationId" = organizationId) +// AND acceptedAt IS NULL +// AND declinedAt IS NULLAlso applies to: 468-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 426 - 435, The conditional state-transition in recordCustodyAcknowledgement currently only keys on custodyId plus acceptedAt/declinedAt, which can omit tenant scope; update the conditional UPDATE (and any subsequent SELECT that reads the current custody row) to include organizationId in the WHERE clause so the transition is scoped to the organization (i.e., WHERE id = custodyId AND organizationId = organizationId AND acceptedAt IS NULL AND declinedAt IS NULL), and do the same for any duplicate logic elsewhere in this file that performs idempotent checks or fetches the custody row (use the custodyId and organizationId together when locating the row and returning its state).
1100-1112:⚠️ Potential issue | 🟡 MinorAdd an explicit rollout step for non-transactional concurrent indexes.
The plan explains this requirement in Line 169-177, but the execution checklist (Line 1100-1112) still doesn’t explicitly include the separate
CREATE INDEX CONCURRENTLYstep.Suggested checklist update
1. **Phase 1** -> Run migration -> `pnpm db:deploy-migration` +2. **Phase 1.1 (required)** -> Run non-transactional SQL step for: + - `idx_custody_pending_ack` + - `idx_custody_batch_pending` + using `CREATE INDEX CONCURRENTLY` outside Prisma migration transaction -2. **Phase 2-3** -> Service + gating (no UI yet) -> Run unit tests +3. **Phase 2-3** -> Service + gating (no UI yet) -> Run unit tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 1100 - 1112, The checklist is missing an explicit rollout step for non-transactional concurrent indexes: add a new numbered step immediately after "Phase 1 -> Run migration -> `pnpm db:deploy-migration`" that instructs operators to run the separate CREATE INDEX CONCURRENTLY step (or run a dedicated non-transactional migration) for any new/altered indexes; mention the exact SQL phrase "CREATE INDEX CONCURRENTLY ..." and note that it must be executed outside the regular transactional migration (e.g., via psql or a separate migration job) before proceeding to "Phase 2-3", and preserve the existing pnpm and phase labels so reviewers can locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 318-321: The SQL uses an undefined kit.id in the UPDATE for
"Custody"; change the WHERE to derive the kit id from kitCustodyId via a
subquery: replace "assetId" IN (SELECT "id" FROM "Asset" WHERE "kitId" = kit.id)
with "assetId" IN (SELECT "id" FROM "Asset" WHERE "kitId" = (SELECT "kitId" FROM
"KitCustody" WHERE "id" = kitCustodyId)), and ensure the custodian scoping still
uses teamMemberId = (SELECT "custodianId" FROM "KitCustody" WHERE "id" =
kitCustodyId) so the UPDATE on "Custody" (setting "tokenVersion" = newVersion)
is correctly scoped by the KitCustody-derived kit id and custodian.
- Around line 426-435: The conditional state-transition in
recordCustodyAcknowledgement currently only keys on custodyId plus
acceptedAt/declinedAt, which can omit tenant scope; update the conditional
UPDATE (and any subsequent SELECT that reads the current custody row) to include
organizationId in the WHERE clause so the transition is scoped to the
organization (i.e., WHERE id = custodyId AND organizationId = organizationId AND
acceptedAt IS NULL AND declinedAt IS NULL), and do the same for any duplicate
logic elsewhere in this file that performs idempotent checks or fetches the
custody row (use the custodyId and organizationId together when locating the row
and returning its state).
- Around line 1100-1112: The checklist is missing an explicit rollout step for
non-transactional concurrent indexes: add a new numbered step immediately after
"Phase 1 -> Run migration -> `pnpm db:deploy-migration`" that instructs
operators to run the separate CREATE INDEX CONCURRENTLY step (or run a dedicated
non-transactional migration) for any new/altered indexes; mention the exact SQL
phrase "CREATE INDEX CONCURRENTLY ..." and note that it must be executed outside
the regular transactional migration (e.g., via psql or a separate migration job)
before proceeding to "Phase 2-3", and preserve the existing pnpm and phase
labels so reviewers can locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db5da748-43d6-4d78-bfd1-55b7b74d027a
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
- Replace undefined kit.id with subquery from kitCustodyId in both token gen and resend SQL - Add organizationId scoping to recordCustody Acknowledgement and recordCustodyDecline WHERE clauses (multi-tenant boundary in service layer) - Add explicit CONCURRENTLY index execution step to build order (runs after Prisma migration)
Contributor
Author
|
@coderabbitai resolve |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 258-266: Add an explicit enablement-time check that prevents
toggling custodyAcknowledgementEnabled to true when CUSTODY_TOKEN_SECRET is
missing: in the Stripe webhook handler that processes org settings updates
(where custodyAcknowledgementEnabled is set) and in the admin/settings toggle
handler (the function that saves org settings), validate
process.env.CUSTODY_TOKEN_SECRET and if absent throw a ShelfError with message
"Cannot enable custody acknowledgement: CUSTODY_TOKEN_SECRET is not
configured."; keep the callsite defense-in-depth by also asserting
CUSTODY_TOKEN_SECRET before any token sign/verify calls.
🪄 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
Run ID: 2dcd534a-8bf1-4a1a-af78-3a17886d69fa
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.claude/plans/custody-acknowledgement.md (1)
259-260:⚠️ Potential issue | 🟠 MajorSpecify concrete implementation files for enablement-time secret validation.
The guard is defined conceptually, but the plan still doesn’t identify exact webhook/settings handlers to modify. That makes this easy to miss during implementation.
Also applies to: 1049-1053, 1079-1097
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 259 - 260, Add explicit runtime validation for CUSTODY_TOKEN_SECRET when enabling custody acknowledgement in the webhook and settings handlers: inside the Stripe webhook processor (e.g., StripeWebhookController.processEvent or handleStripeWebhook) and the admin/org settings toggle (e.g., OrgSettingsService.toggleCustodyAcknowledgement or updateOrgSettings), detect attempts to set custodyAcknowledgementEnabled = true, check process.env.CUSTODY_TOKEN_SECRET (or config.get('CUSTODY_TOKEN_SECRET')), and if missing throw/reject with the exact message "Cannot enable custody acknowledgement: CUSTODY_TOKEN_SECRET is not configured." Also ensure the webhook path that handles plan/metadata/org feature updates (customer/org update events) performs the same guard and add unit/integration tests covering both code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 633-639: Update the acknowledge action flow to handle batch/kit
tokens by ensuring the route handler performs a full token verification
(including rotation check), calls recordCustodyAcknowledgement() inside a DB
transaction (and uses its { transitioned, custody } result), and — only when
transitioned === true — creates the acknowledgement activity note within the
same transaction (or immediately after), and for kit custody cascades acceptedAt
to all child custody rows still pending (WHERE acceptedAt IS NULL AND declinedAt
IS NULL) while creating notes for each updated asset; after committing, enqueue
admin and custodian notification emails via pg-boss; if transitioned === false,
do nothing (idempotent retry).
- Around line 579-603: The route currently requires a path param `:custodyId`
(apps/webapp/app/routes/_auth+/accept-custody.$custodyId.tsx) but batch tokens
only carry `batchId`, so update the route and related handling to accept either
a missing custodyId or an alternative batch token flow: make the `custodyId`
path optional (or add a no-param route `/accept-custody`) and change the
loader/action in `accept-custody` to ignore the URL id for authority and instead
extract/verify the JWT `token` (checking `token.ver`) and use the decoded id (or
`batchId` from the token payload) for DB access; also adjust link generation for
batch emails to point to the no-param `/accept-custody?token=...` form and
ensure logger redaction and publicPaths handling include both
`/accept-custody/:custodyId` and `/accept-custody` variants.
---
Duplicate comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 259-260: Add explicit runtime validation for CUSTODY_TOKEN_SECRET
when enabling custody acknowledgement in the webhook and settings handlers:
inside the Stripe webhook processor (e.g., StripeWebhookController.processEvent
or handleStripeWebhook) and the admin/org settings toggle (e.g.,
OrgSettingsService.toggleCustodyAcknowledgement or updateOrgSettings), detect
attempts to set custodyAcknowledgementEnabled = true, check
process.env.CUSTODY_TOKEN_SECRET (or config.get('CUSTODY_TOKEN_SECRET')), and if
missing throw/reject with the exact message "Cannot enable custody
acknowledgement: CUSTODY_TOKEN_SECRET is not configured." Also ensure the
webhook path that handles plan/metadata/org feature updates (customer/org update
events) performs the same guard and add unit/integration tests covering both
code paths.
🪄 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
Run ID: 6899df30-15c9-4d5a-be56-58adba0f40c8
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
validation location CUSTODY_TOKEN_SECRET check runs in Stripe webhook handler and settings action via shared validateCustodyTokenSecretConfigured() helper. Added stripe-webhook/handlers.server.ts to modified files list.
Contributor
Author
|
@coderabbitai resolve |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.claude/plans/custody-acknowledgement.md (2)
635-643:⚠️ Potential issue | 🟠 MajorAcknowledge action flow still lacks an explicit batch transition path.
Step 2 calls single-record
recordCustodyAcknowledgement(); this conflicts with the “Acknowledge All” batch behavior. Add explicit branching forcustody-ack-batchto transition all pending rows atomically (parallel to single/kit handling), with notes/emails gated on transitioned rows only.Also applies to: 752-753
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 635 - 643, The acknowledge flow must explicitly handle the new batch path: add branching in the acknowledge handler to detect the custody-ack-batch action and call a new atomic batch transition routine (e.g., recordCustodyAcknowledgementBatch) instead of the single-record recordCustodyAcknowledgement(); ensure the batch routine performs a DB transaction that transitions all pending rows, returns { transitioned, custodyRows }, and that creation of acknowledgement activity notes, kit-child acceptedAt cascade (WHERE acceptedAt IS NULL AND declinedAt IS NULL) and per-asset notes happen only for rows that were actually transitioned; after the transaction commit enqueue admin + custodian emails via pg-boss for transitioned rows only, and leave the single-record and kit paths unchanged for idempotency and rendering of current state.
582-583:⚠️ Potential issue | 🟠 MajorBatch/kit token route contract is still brittle with mandatory
:custodyId.The plan still requires
/accept-custody/:custodyIdwhile batch tokens are keyed bybatchId. Please define a non-authoritative generic segment (e.g.,:tokenRef) or no-param variant and explicitly document batch link shape to avoid placeholder-dependent implementations.Also applies to: 590-591, 605-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 582 - 583, The route file accept-custody.$custodyId.tsx forces a mandatory :custodyId segment which breaks batch-token links keyed by batchId; rename the route param to a generic non-authoritative segment (e.g., change accept-custody.$custodyId.tsx -> accept-custody.$tokenRef.tsx) or add a no-param variant (accept-custody.tsx) and update any loader/action handlers (the functions in that file) to resolve tokenRef into either custodyId or batchId at runtime; also update the route documentation/comments to explicitly state the expected batch link shape (that batch tokens use batchId) so callers construct either /accept-custody/:tokenRef where tokenRef may be a custodyId or batchId, or use the no-param entrypoint for out-of-band token resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 1082-1101: The "Modified files (18)" header and following list are
inconsistent (the list shows 17 entries with a duplicate number 17); update the
count and numbering in .claude/plans/custody-acknowledgement.md so they match
the actual files changed: either change the header to "Modified files (17)" and
renumber items 1–17, or add the missing file entry and renumber items 1–18 so
there is no duplicate 17; ensure the header value and the enumerated list (the
lines starting with the numbered file entries) are consistent.
---
Duplicate comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 635-643: The acknowledge flow must explicitly handle the new batch
path: add branching in the acknowledge handler to detect the custody-ack-batch
action and call a new atomic batch transition routine (e.g.,
recordCustodyAcknowledgementBatch) instead of the single-record
recordCustodyAcknowledgement(); ensure the batch routine performs a DB
transaction that transitions all pending rows, returns { transitioned,
custodyRows }, and that creation of acknowledgement activity notes, kit-child
acceptedAt cascade (WHERE acceptedAt IS NULL AND declinedAt IS NULL) and
per-asset notes happen only for rows that were actually transitioned; after the
transaction commit enqueue admin + custodian emails via pg-boss for transitioned
rows only, and leave the single-record and kit paths unchanged for idempotency
and rendering of current state.
- Around line 582-583: The route file accept-custody.$custodyId.tsx forces a
mandatory :custodyId segment which breaks batch-token links keyed by batchId;
rename the route param to a generic non-authoritative segment (e.g., change
accept-custody.$custodyId.tsx -> accept-custody.$tokenRef.tsx) or add a no-param
variant (accept-custody.tsx) and update any loader/action handlers (the
functions in that file) to resolve tokenRef into either custodyId or batchId at
runtime; also update the route documentation/comments to explicitly state the
expected batch link shape (that batch tokens use batchId) so callers construct
either /accept-custody/:tokenRef where tokenRef may be a custodyId or batchId,
or use the no-param entrypoint for out-of-band token resolution.
🪄 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
Run ID: 64677d25-2ca4-4a40-b5a3-2634fb018107
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.claude/plans/custody-acknowledgement.md (2)
1082-1101:⚠️ Potential issue | 🟡 MinorFix file count and numbering in summary.
Line 1082 claims "Modified files (18):" but the enumerated list contains only 17 entries with a duplicate item number "17." appearing at both Line 1099 and Line 1100.
Either correct the header to "Modified files (17)" and renumber items 1–17 sequentially, or add the missing 18th file entry.
🔢 Proposed fix
-### Modified files (18): +### Modified files (17): 1. `packages/database/prisma/schema.prisma` — Custody, KitCustody, Organization, User models 2. `apps/webapp/server/index.ts` — Add public path ... 16. `apps/webapp/server/logger.ts` — Redact token query param on acceptance routes 17. `apps/webapp/app/modules/stripe-webhook/handlers.server.ts` — Validate CUSTODY_TOKEN_SECRET on feature enablement -17. `apps/webapp/app/utils/env.ts` — Add CUSTODY_TOKEN_SECRET +18. `apps/webapp/app/utils/env.ts` — Add CUSTODY_TOKEN_SECRET(If there are truly 18 files, renumber the last item. If there are only 17, update the header.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 1082 - 1101, Update the changes summary in .claude/plans/custody-acknowledgement.md to fix the file count and list numbering: either change the header "Modified files (18):" to "Modified files (17):" and renumber the enumerated items to 1–17, or if there really are 18 files, add the missing entry and renumber so there are no duplicated "17." lines; ensure the header count matches the actual number of list items and that each item is sequentially numbered.
582-590:⚠️ Potential issue | 🟠 MajorSpecify concrete URL param values for batch and kit tokens.
The route is defined as
accept-custody.$custodyId.tsx(Line 582) and the public path uses:custodyId(Line 590), but:
- Batch tokens carry
batchIdin their payload (Line 352), not acustodyId- Kit tokens carry
id(thekitCustodyId), not acustodyIdfor individual assets- Line 605 says the URL param is "for routing only" but doesn't specify what concrete value to use
This creates implementation ambiguity. When generating links for batch/kit acknowledgement emails, what value should be substituted into
/accept-custody/:custodyId?Suggested approach: Either rename the route param to something generic like
:tokenRefand document placeholder conventions (e.g., use"batch"for batch tokens, or use deterministic IDs likebatchId/kitCustodyId), OR add separate routes for batch and kit cases.📝 Proposed plan clarification
Add to the route section (after Line 590):
Add: `"/accept-custody/:custodyId"` (narrow — no wildcard) +**URL param values by token type:** +- Single custody: Use the `custodyId` for readability (e.g., `/accept-custody/clx123abc`) +- Batch: Use the `acknowledgementBatchId` (e.g., `/accept-custody/clx456def`) +- Kit: Use the `kitCustodyId` (e.g., `/accept-custody/clx789ghi`) + +In all cases, the URL param is **non-authoritative** — the verified JWT payload is the sole source of truth for DB operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 582 - 590, The route parameter :custodyId is ambiguous because batch tokens carry batchId and kit tokens carry id/kitCustodyId; update accept-custody.$custodyId.tsx and the publicPaths entry in server/index.ts to resolve this by either (A) renaming the route and public path to a generic param (e.g., accept-custody/:tokenRef) and documenting the substitution convention (use "batch" or the concrete batchId / kitCustodyId as appropriate) and update the email link generation to inject that tokenRef, or (B) create two explicit routes (e.g., accept-custody/batch.$batchId.tsx and accept-custody/kit.$kitCustodyId.tsx) with matching publicPaths and ensure link generation uses batchId for batch tokens and kitCustodyId (id) for kits; apply the corresponding changes to token parsing logic in accept-custody.$custodyId.tsx (or the new route handlers) so it extracts payload.batchId or payload.id per route convention.
🧹 Nitpick comments (2)
.claude/plans/custody-acknowledgement.md (2)
752-753: Clarify where the batch acceptance page is rendered and how it differs from single custody.Lines 752-753 describe the batch acceptance UX: "Acceptance page for batch: queries all custodies with matching
acknowledgementBatchId, verifies all belong to same custodian, shows list, one 'Acknowledge All' click updates all records in transaction."However, the plan defines only one route —
accept-custody.$custodyId.tsx(Line 582). It's not clear whether:
- The same route/component handles both single and batch cases (branching based on loader data), or
- A separate batch-specific route/component is needed
If it's the same route, the component mockup (Lines 658-701) should include a batch variant showing the list UI and "Acknowledge All" button. If it's separate, add it to the new files list.
📐 Suggested plan clarification
Add to Phase 5.5 Component section (after Line 701):
**Batch variant (when loader returns multiple custodies):** ```text +------------------------------------------+ | [Shelf Logo] | | | | You've been assigned {count} assets | | | | [List of asset cards with images/titles]| | | | By acknowledging, you confirm you have | | received all of these assets. | | | | +------------------------------------+ | | | [Acknowledge All] | | | +------------------------------------+ | | | | I don't have one or more of these items | | | +------------------------------------------+The same route component branches on
custodies.length > 1to render the batch UI.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/plans/custody-acknowledgement.md around lines 752 - 753, The plan
describes a batch acceptance UX but only lists a single route file
accept-custody.$custodyId.tsx; clarify and implement whether batching is handled
in the same route or a separate route: update the plan and component mockups so
the accept-custody.$custodyId.tsx loader can return either a single custody or
an array (e.g., custodies) and the React component branches on custodies.length1 to render a "Batch variant" UI (list of asset cards and an "Acknowledge All"
button) otherwise render the single-custody UI, or alternatively add a new
batch-specific route/component to the new files list if you prefer separation —
mention accept-custody.$custodyId.tsx, its loader, and the component mockup when
making the change.</details> --- `293-314`: **Note PostgreSQL version requirement for `hashtext()` function.** The batch token generation uses `pg_advisory_xact_lock(hashtext(batchId))` (Line 296) to prevent concurrent races. The `hashtext()` function is a PostgreSQL built-in, but its availability and behavior may vary across PostgreSQL versions or forks. Consider adding a note about the minimum PostgreSQL version required, or provide a fallback approach (e.g., using `hashtext()` if available, or manually hashing the `batchId` with a simple hash function in the application layer and passing a bigint to `pg_advisory_xact_lock()`). <details> <summary>🛡️ Alternative approach using application-layer hashing</summary> If `hashtext()` compatibility is a concern, you can hash the `batchId` in the application and pass a `bigint` to the advisory lock: ```typescript // In the service function: const crypto = require('crypto'); const hash = crypto.createHash('sha256').update(batchId).digest(); const lockId = hash.readBigInt64BE(0); // Extract first 8 bytes as bigint await tx.$queryRaw`SELECT pg_advisory_xact_lock(${lockId})`;This avoids reliance on
hashtext()and works across all PostgreSQL versions that support advisory locks (≥8.2).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 293 - 314, The advisory-lock call in generateBatchAcknowledgementToken currently uses pg_advisory_xact_lock(hashtext(batchId)) which may not exist on all Postgres versions; either document the minimum Postgres version required or replace the DB-side hash with an app-side bigint lock id: compute a stable 8-byte bigint from batchId (e.g., SHA-256 then readBigInt64BE) and pass that bigint into tx.$queryRaw`SELECT pg_advisory_xact_lock(${lockId})`; update generateBatchAcknowledgementToken to use the app-derived lockId and add a short comment noting this avoids hashtext() compatibility issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 636-643: The action flow expects purpose-specific acknowledgement
handlers but only recordCustodyAcknowledgement (single-record) exists; add two
new functions and wire the action to call them: implement
recordBatchCustodyAcknowledgement(params: {batchId, method, ip, userAgent,
organizationId}) to conditionally update pending Custody rows for the batch
(WHERE acknowledgementBatchId = batchId AND acceptedAt IS NULL AND declinedAt IS
NULL AND asset org-scoped), set
acceptedAt/acceptanceMethod/acceptanceIp/acceptanceUserAgent, and return {
transitioned: number, custodies: Custody[] }; implement
recordKitCustodyAcknowledgement(params: {kitCustodyId, method, ip, userAgent,
organizationId}) to run a DB transaction that conditionally updates the
KitCustody (org-scoped), cascades acceptedAt to pending child Custody rows, and
returns { transitioned: boolean, kitCustody, childCustodies }; then update the
action flow to branch by token purpose and call
recordBatchCustodyAcknowledgement or recordKitCustodyAcknowledgement (instead of
only recordCustodyAcknowledgement), and ensure callers only create notes/enqueue
emails when transitioned > 0 or transitioned === true to preserve idempotency.
---
Duplicate comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 1082-1101: Update the changes summary in
.claude/plans/custody-acknowledgement.md to fix the file count and list
numbering: either change the header "Modified files (18):" to "Modified files
(17):" and renumber the enumerated items to 1–17, or if there really are 18
files, add the missing entry and renumber so there are no duplicated "17."
lines; ensure the header count matches the actual number of list items and that
each item is sequentially numbered.
- Around line 582-590: The route parameter :custodyId is ambiguous because batch
tokens carry batchId and kit tokens carry id/kitCustodyId; update
accept-custody.$custodyId.tsx and the publicPaths entry in server/index.ts to
resolve this by either (A) renaming the route and public path to a generic param
(e.g., accept-custody/:tokenRef) and documenting the substitution convention
(use "batch" or the concrete batchId / kitCustodyId as appropriate) and update
the email link generation to inject that tokenRef, or (B) create two explicit
routes (e.g., accept-custody/batch.$batchId.tsx and
accept-custody/kit.$kitCustodyId.tsx) with matching publicPaths and ensure link
generation uses batchId for batch tokens and kitCustodyId (id) for kits; apply
the corresponding changes to token parsing logic in
accept-custody.$custodyId.tsx (or the new route handlers) so it extracts
payload.batchId or payload.id per route convention.
---
Nitpick comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 752-753: The plan describes a batch acceptance UX but only lists a
single route file accept-custody.$custodyId.tsx; clarify and implement whether
batching is handled in the same route or a separate route: update the plan and
component mockups so the accept-custody.$custodyId.tsx loader can return either
a single custody or an array (e.g., custodies) and the React component branches
on custodies.length > 1 to render a "Batch variant" UI (list of asset cards and
an "Acknowledge All" button) otherwise render the single-custody UI, or
alternatively add a new batch-specific route/component to the new files list if
you prefer separation — mention accept-custody.$custodyId.tsx, its loader, and
the component mockup when making the change.
- Around line 293-314: The advisory-lock call in
generateBatchAcknowledgementToken currently uses
pg_advisory_xact_lock(hashtext(batchId)) which may not exist on all Postgres
versions; either document the minimum Postgres version required or replace the
DB-side hash with an app-side bigint lock id: compute a stable 8-byte bigint
from batchId (e.g., SHA-256 then readBigInt64BE) and pass that bigint into
tx.$queryRaw`SELECT pg_advisory_xact_lock(${lockId})`; update
generateBatchAcknowledgementToken to use the app-derived lockId and add a short
comment noting this avoids hashtext() compatibility issues.
🪄 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
Run ID: 1bca8555-7538-43f1-bc43-27c6d389ea4c
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
- Add recordBatchCustodyAcknowledgement() and recordKitCustodyAcknowledgement() functions to Phase 3 (were missing — action referenced them but only single-record function existed) - Fix modified files numbering (duplicate 17, count said 18 but had 17 entries) - Update action to branch by token purpose and call matching record function
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/plans/custody-acknowledgement.md (1)
1042-1054: Batch cooldown check logic may be inefficient but is correct.The batch resend cooldown check (Lines 1042-1054) uses a subquery
SELECT MAX("lastTokenRotatedAt")repeated in both NULL check and interval comparison. While correct, this duplicates the expensive MAX computation. Consider refactoring to compute MAX once in a CTE or variable.♻️ Proposed optimization
Use a CTE to avoid duplicate MAX computation:
+WITH batch_last_rotated AS ( + SELECT MAX("lastTokenRotatedAt") as max_rotated + FROM "Custody" + WHERE "acknowledgementBatchId" = ${batchId} + AND "requiresAcceptance" = true + AND "assetId" IN (SELECT "id" FROM "Asset" WHERE "organizationId" = ${organizationId}) +) UPDATE "Custody" SET "tokenVersion" = (...), "lastTokenRotatedAt" = NOW(), "updatedAt" = NOW() WHERE "acknowledgementBatchId" = ${batchId} AND "requiresAcceptance" = true AND "assetId" IN (SELECT "id" FROM "Asset" WHERE "organizationId" = ${organizationId}) - AND ( - (SELECT MAX("lastTokenRotatedAt") FROM "Custody" WHERE ...) IS NULL - OR - (SELECT MAX("lastTokenRotatedAt") FROM "Custody" WHERE ...) < NOW() - INTERVAL '60 seconds' - ) + AND ( + (SELECT max_rotated FROM batch_last_rotated) IS NULL + OR + (SELECT max_rotated FROM batch_last_rotated) < NOW() - INTERVAL '60 seconds' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 1042 - 1054, The duplicated expensive subquery computing MAX("lastTokenRotatedAt") for the Custody rows (filtered by acknowledgementBatchId, requiresAcceptance and assetId -> Asset.organizationId) should be computed once and reused; refactor the SQL by introducing a CTE or lateral subquery that selects MAX("lastTokenRotatedAt") AS max_lastTokenRotatedAt from "Custody" (using the same filters) and then replace both occurrences with that alias, so the final WHERE clause checks (max_lastTokenRotatedAt IS NULL OR max_lastTokenRotatedAt < NOW() - INTERVAL '60 seconds') instead of repeating the MAX(...) expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 258-262: The documentation currently claims the feature guard runs
in both the Stripe webhook handler and an "Organization settings action
(apps/webapp/app/routes/_layout+/settings.general.tsx)"; remove that
settings.general.tsx reference or mark it explicitly as future/optional to avoid
implying it's implemented in this PR—leave the Stripe webhook handler
(apps/webapp/app/modules/stripe-webhook/handlers.server.ts) and the call to
validateCustodyTokenSecretConfigured() as the only enforced path for now and, if
kept, rephrase to "— if/when a manual toggle is added in future" next to
settings.general.tsx.
---
Nitpick comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 1042-1054: The duplicated expensive subquery computing
MAX("lastTokenRotatedAt") for the Custody rows (filtered by
acknowledgementBatchId, requiresAcceptance and assetId -> Asset.organizationId)
should be computed once and reused; refactor the SQL by introducing a CTE or
lateral subquery that selects MAX("lastTokenRotatedAt") AS
max_lastTokenRotatedAt from "Custody" (using the same filters) and then replace
both occurrences with that alias, so the final WHERE clause checks
(max_lastTokenRotatedAt IS NULL OR max_lastTokenRotatedAt < NOW() - INTERVAL '60
seconds') instead of repeating the MAX(...) expression.
🪄 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
Run ID: 5938ce46-4ebb-49c2-bf88-d984e57b29a0
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
Contributor
Author
|
@coderabbitai resolve |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.claude/plans/custody-acknowledgement.md (1)
621-630:⚠️ Potential issue | 🟠 MajorUnify the public route contract with batch/kit token purposes.
Line 621/629 still require
:custodyId, but Line 969 supports batch/kit payloads where authority comes from token claims (not a custody ID). This leaves batch/kit link construction brittle and underdefined again.Proposed plan-text adjustment
- **New file:** `apps/webapp/app/routes/_auth+/accept-custody.$custodyId.tsx` + **New file:** `apps/webapp/app/routes/_auth+/accept-custody.$tokenRef.tsx` - Add: `"/accept-custody/:custodyId"` (narrow — no wildcard) + Add: `"/accept-custody/:tokenRef"` (routing-only param, non-authoritative) - The decoded `id` from the token is the **sole key** for all DB operations — ignore the URL `custodyId` param for data access (use it only for routing) + The decoded token payload is the **sole key** for all DB operations (`id` for single/kit, `batchId` for batch) — ignore URL `tokenRef` for authority.Also applies to: 644-645, 969-970, 1173-1173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 621 - 630, The public route contract is inconsistent: the new route and server publicPaths currently require a :custodyId path param while batch/kit flows expect authority from token claims only; update the contract so the route and publicPaths accept token-only links. Concretely, modify the apps/webapp/app/routes/_auth+/accept-custody.$custodyId.tsx route to handle an optional custodyId (or no path param) by reading authority from the token when custodyId is absent, and change the publicPaths entry in server/index.ts (the publicPaths array) from a narrow "/accept-custody/:custodyId" to a non-parameterized "/accept-custody" (or add both patterns) so batch/kit links that rely solely on token claims are valid; ensure any helper logic that parses custodyId (in the route handler) falls back to token claims.
🧹 Nitpick comments (1)
.claude/plans/custody-acknowledgement.md (1)
472-483: Document org scoping in the KitCustody transition step (or removeorganizationIdfrom params).Line 477 includes
organizationId, but Line 481’s KitCustody update condition is not org-scoped in the spec. That mismatch makes this easy to implement incorrectly.Proposed plan-text clarification
-// 1. Update KitCustody: acceptedAt = now(), etc. -// WHERE id = kitCustodyId AND acceptedAt IS NULL AND declinedAt IS NULL +// 1. Update KitCustody: acceptedAt = now(), etc. +// WHERE id = kitCustodyId AND acceptedAt IS NULL AND declinedAt IS NULL +// AND "kitId" IN (SELECT "id" FROM "Kit" WHERE "organizationId" = organizationId)Also applies to: 486-487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/plans/custody-acknowledgement.md around lines 472 - 483, The spec for recordKitCustodyAcknowledgement is inconsistent: the function signature accepts organizationId but the KitCustody update step (the transactional WHERE condition that checks id, acceptedAt IS NULL and declinedAt IS NULL) is not scoped by organization; either document and enforce org scoping or remove the parameter. Fix by updating the KitCustody update/query in recordKitCustodyAcknowledgement to include organizationId = :organizationId in the WHERE clause (and propagate the same org filter when cascading updates to child Custody/Asset rows), OR remove organizationId from the RecordKitAcknowledgementParams and all related code paths and docs; ensure the chosen approach updates the transaction logic in recordKitCustodyAcknowledgement, and update any tests/comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 621-630: The public route contract is inconsistent: the new route
and server publicPaths currently require a :custodyId path param while batch/kit
flows expect authority from token claims only; update the contract so the route
and publicPaths accept token-only links. Concretely, modify the
apps/webapp/app/routes/_auth+/accept-custody.$custodyId.tsx route to handle an
optional custodyId (or no path param) by reading authority from the token when
custodyId is absent, and change the publicPaths entry in server/index.ts (the
publicPaths array) from a narrow "/accept-custody/:custodyId" to a
non-parameterized "/accept-custody" (or add both patterns) so batch/kit links
that rely solely on token claims are valid; ensure any helper logic that parses
custodyId (in the route handler) falls back to token claims.
---
Nitpick comments:
In @.claude/plans/custody-acknowledgement.md:
- Around line 472-483: The spec for recordKitCustodyAcknowledgement is
inconsistent: the function signature accepts organizationId but the KitCustody
update step (the transactional WHERE condition that checks id, acceptedAt IS
NULL and declinedAt IS NULL) is not scoped by organization; either document and
enforce org scoping or remove the parameter. Fix by updating the KitCustody
update/query in recordKitCustodyAcknowledgement to include organizationId =
:organizationId in the WHERE clause (and propagate the same org filter when
cascading updates to child Custody/Asset rows), OR remove organizationId from
the RecordKitAcknowledgementParams and all related code paths and docs; ensure
the chosen approach updates the transaction logic in
recordKitCustodyAcknowledgement, and update any tests/comments accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 911d29db-f1f4-48c2-92bb-47e21d4f0c27
📒 Files selected for processing (1)
.claude/plans/custody-acknowledgement.md
not v1 — only webhook handler validates secret
Contributor
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
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.
Implementation plan for Custody Acknowledgement. Plan only, no code.
Complete rewrite + 4 additional fixes from last review round. Self-audited before opening.
Full plan:
.claude/plans/custody-acknowledgement.mdSummary by CodeRabbit