feat(agent-templates): write the featured gallery's order in one transaction, and make curation admin-only

[saulmc]

A reorder was one PATCH per row, and convos.org renders those as they land — so a failure halfway through left the homepage on a blend of the old order and the new one. No client-side undo can rule that out, because the undo is itself a sequence of writes that can fail. The order is a single value; it was being written as many.

PUT /featured-order takes the gallery as an ordered list of ids and assigns the weights itself, in one transaction. It moves once or not at all.

Console side: xmtplabs/convos-assistants#2673 — deploy this first.

---

The weight stops being the dashboard's business

Before, the dashboard computed the weights (renumber densely, diff, PATCH each changed row) — the same arithmetic on both sides of the wire, and a rule (heaviest leads, dense, public rows only) that the backend enforced and the frontend re-derived. That split had already produced two bugs. Now the dashboard sends an order and the backend derives the weights from position; the console holds no weight logic at all.

It also deletes machinery rather than adding it: the write queue, the stop-at-first-failure, the captured prior weights, and the compensating undo all go. Nothing can half-apply, so there is nothing to put back — a stronger claim than "we put it back carefully."

Whole-gallery contract

The body must be the entire gallery. A partial list would leave the rows it omits holding weights that collide with the ones it sets — that's how two templates come to claim one slot. If the gallery changed under the caller (something published, featured, or dropped out mid-drag), the ids no longer match and the write is refused with 409 rather than clobbering a set the caller never saw.

It isn't a user endpoint

Curating the gallery is not something a user does, so this isn't a route a user can reach: it takes the agent key alone. A signed-in caller is turned away at the door (401) rather than admitted by the middleware and refused inside — and the key is the identity, so there's no account to require. No in-handler auth check survives, because a check for a key that must already be present is dead code shaped like a lock.

…and while I was in there, the front door was open

Any signed-in user could put their own template into the featured gallery. create accepts featured in the body and PATCH accepts it from whoever passes the ownership guard — which is the owner. Proven end to end with a plain JWT, no admin key anywhere:

Plain user JWT	Before	After
create with featured: true	201, featured stored	403
publish own template	200	200 (still theirs)
PATCH featured: true	200	403
PATCH featuredRank: 999	403	403
Lands in the public gallery?	YES (rank 0)	NO

Only the slot had been gated (#362), so they entered at weight 0 — dead last. That contains it while the gallery holds a homepage's worth of curated templates above them, but containment isn't a closed door: it inflates the featured set, and the moment the gallery holds fewer than eight weighted templates, an outsider renders on convos.org.

Withdrawal stays the owner's: featured: false can only ever take something off the homepage, and the runtime's builder sends it on every create — which is exactly why the gate keys on === true rather than !== undefined.

Correctness notes worth a reviewer's eye

The check and the write share one snapshot. The membership read originally ran outside the transaction — a check-then-act. The damaging half is a template leaving the gallery mid-write: it stays in the caller's list, takes a weight it is no longer entitled to, and (since a weight is a slot) silently reclaims that slot when it returns — walking around #362's invariant from the side. Note that simply moving the read inside the transaction does not fix this: Prisma's default is READ COMMITTED, where each statement takes a fresh snapshot. Verified rather than assumed:

isolation WITH the option   : serializable
isolation WITHOUT the option: read committed

A raw query's serialization failure isn't P2034. Prisma surfaces the driver's 40001 wrapped as P2010, so the expected outcome of a concurrent write was escaping as a 500 instead of the 409 that says "re-read and retry" — and only when the race actually landed, which is why it first showed as a test that failed once and passed on rerun. Both shapes are recognised, and a unit test pins them, including that an unrelated P2010 still surfaces as a 500 rather than being laundered into "try again".

One statement, and it isn't an edit. The whole order is written as a single UPDATE … FROM (VALUES …). That keeps the SERIALIZABLE window to one round trip, and it leaves updatedAt alone — Prisma's update touches it on every row, so a reorder was stamping the entire gallery as freshly edited, and updatedAt is a sort the list API offers. Ranking a template is not editing it.

Tests

Every rejection path asserts the stored weights are byte-identical afterwards — that's the point of the endpoint:

• writes the whole order, heaviest first (dense, top-down)
• a reorder doesn't mark the gallery as edited (updatedAt untouched)
• a partial list → 409, nothing written
• a gallery that changed underneath → 409 GALLERY_CHANGED, nothing written, and it succeeds after a re-read
• an id outside the gallery → 409, nothing written
• the same template twice → 400, nothing written
• a signed-in caller, and an anonymous one → 401, nothing written
• concurrent reorders leave a total order (dense, distinct, no 500s), and a template that leaves the gallery mid-write holds no weight — invariants, not "who won", so they don't hinge on timing
• serialization-failure classification (P2034, P2010+40001, and not an unrelated P2010)
• featuring authz: a user can't create-as-featured or self-feature; may still create with featured: false and unfeature their own; the dashboard still features

Full suite green (1492).

🤖 Generated with Claude Code

https://claude.ai/code/session_01DxFzZNgXaSnFWAYR8XPpCR

[claude]

Claude finished @saulmc's task —— View job

---

Code Review — PR #363

Comprehensive review of the atomic featured gallery ordering endpoint.

---

Summary

This PR successfully addresses a critical architectural problem: reordering the featured gallery used to be N PATCHes that could fail halfway through, leaving the homepage in an inconsistent state. The new PUT /featured-order endpoint writes the entire gallery order atomically in a single SERIALIZABLE transaction. The implementation is well-tested and the Macroscope-identified race condition was properly fixed in commit 3a121e4.

---

✅ Code Quality & Best Practices

Strong points:

1. Atomic transaction design — The entire order write happens in one SERIALIZABLE transaction (featured-order.ts:112-154), ensuring the gallery moves completely or not at all. This is the core value proposition and it's correctly implemented.

2. Excellent inline documentation — The handler includes a comprehensive comment block (lines 43-58, 97-111) explaining the "why" behind the design decisions. The serialization-failure comment (lines 23-31) explains both Prisma error codes.

3. Defensive validation — Multiple guard rails:

   • Duplicate detection (lines 85-92)
   • Gallery membership check inside transaction (lines 114-125)
   • Written row count verification (lines 144-150)
   • Max gallery size cap (line 7)

4. Zero updatedAt pollution — Using raw SQL (lines 134-143) instead of Prisma's update method ensures reordering doesn't stamp every template as "freshly edited", preserving updatedAt as a meaningful sort key.

Minor observations:

1. Error handling convention adherence — The author correctly declined CodeRabbit's AppError suggestion. This module has two distinct error-handling patterns:

   • CRUD handlers (list, create, patch, delete) use direct response with coded errors: { error: { code, message } }
   • Generation/attachment handlers throw AppError

   The sendError helper matches patch.ts's sendBadRequest pattern exactly (positional res + object payload). Forcing AppError here would actually violate module consistency since AppError emits { error: "string", details } while this endpoint's contract requires { error: { code, message } } for client-side handling (see GALLERY_CHANGED usage in tests).

2. Handler placement — Router correctly mounts /featured-order before /:id wildcard (agent-templates.router.ts:82-87) to prevent path collision.

---

🔒 Security

✅ No issues found

1. Authorization is correct — Dashboard API key required (lines 62-73), matching the existing featuredRank PATCH rule. Owner JWTs are rejected (tested in line 287-300).

2. Input validation is thorough:

   • UUIDs validated by Zod (line 11)
   • Array size capped at 200 (line 11)
   • Duplicates rejected (lines 85-92)
   • Gallery membership verified in-transaction (lines 114-125)

3. No injection vectors — Raw SQL uses parameterized Prisma.sql template literals (lines 137-142), properly escaped.

4. No sensitive data exposure — Error messages are appropriate for the caller (dashboard admin) and don't leak internal state.

---

🐛 Potential Bugs

✅ None identified

The Macroscope-caught race condition was the only bug and it's been fixed. The membership check now runs inside the SERIALIZABLE transaction (lines 112-154), closing the check-then-act window that would have allowed a template leaving the gallery to retain a weight.

Verification of the fix:

• isolationLevel: Prisma.TransactionIsolationLevel.Serializable (line 153) ensures the read and writes see one snapshot
• Both P2034 (Prisma) and P2010 with code 40001 (raw SQL) serialization failures map to 409 GALLERY_CHANGED (lines 32-41, 169-175)
• Tests verify invariants hold under concurrent writes (lines 227-254, 256-285)

---

⚡ Performance

✅ Appropriate for workload

1. Transaction scope is minimal — SERIALIZABLE increases abort risk under contention, but:

   • The transaction does one read + one write (not N writes)
   • Gallery reorders are infrequent (dashboard curation, not user-facing)
   • Tests confirm concurrent writes resolve correctly (one wins, one 409s)

2. Single UPDATE instead of loop — The raw SQL VALUES clause (lines 134-143) updates all rows in one round trip instead of N Prisma updates. This both minimizes the SERIALIZABLE window and avoids touching updatedAt.

3. No N+1 queries — Single findMany for membership check, single bulk update for weights.

4. Could optimize further (not recommended): A single UPDATE with CASE could replace the loop entirely, but as the author correctly notes, the gallery is ~dozens of rows (capped at 200), making this premature optimization.

---

🧪 Test Coverage

✅ Excellent coverage

The test suite covers every rejection path and asserts stored weights are unchanged when writes fail (the "write once or not at all" guarantee). Key scenarios:

Happy path:

• Writes correct weights (dense, descending) (lines 104-127)
• Doesn't touch updatedAt (lines 132-151)

Rejection paths:

• Partial list → 409, nothing written (lines 153-161)
• Stale gallery → 409 GALLERY_CHANGED, retry succeeds (lines 163-182)
• ID outside gallery → 409, nothing written (lines 184-208)
• Duplicate ID → 400 DUPLICATE_TEMPLATE, nothing written (lines 210-221)
• JWT caller (not API key) → 403, nothing written (lines 287-300)

Concurrency invariants:

• Concurrent reorders leave gallery in valid total order (lines 227-254)
• Template leaving gallery mid-write holds no weight (lines 256-285)

Serialization failure detection:

• Both Prisma (P2034) and raw SQL (P2010/40001) failure codes recognized (featured-order.serialization.test.ts)

Missing: Contract test for the request body schema. Per CLAUDE.md, client-facing request schemas under src/api/v2/** must be backwards-compatible, and the guidance says to pin legacy shapes with assertLegacyShapeValidates. However:

• This endpoint is not client-facing — it's dashboard-only (API key required), so mobile apps never call it
• The /featured-order path isn't exposed to shipped mobile clients
• The backwards-compatibility rule applies to endpoints that mobile apps invoke

Conclusion: Contract test not needed here; the endpoint is server-to-server (dashboard → backend).

---

📋 CLAUDE.md Compliance

✅ Fully compliant

1. Client-facing API contracts — N/A, this endpoint is dashboard-only (API key auth), not called by mobile clients.

2. Money/credits — N/A, this endpoint doesn't touch credits or balances.

3. PR checklist:

   • ✅ Request schema: Not client-facing (dashboard API key only)
   • ✅ Credits/balances: Not touched
   • ✅ No new files unless necessary: 3 new files, all essential (handler, 2 test files)

---

🎯 Recommendations

No blocking issues. The implementation is solid. Two optional suggestions:

1. Consider logging successful reorders — Currently only failed writes are logged (line 177-180). Adding a success log with { templateCount, caller: accountId } would help dashboard operators diagnose "who reordered the gallery when" without needing database forensics. This is a nice-to-have, not a requirement.

2. Response could include the written order — Current 200 response is { updated: N }. Returning { updated: N, order: [...] } (the weights or IDs) would let the console verify the write immediately instead of re-reading. Again, optional — the current contract is fine and the console re-reads anyway.

---

Final Verdict

Approved for merge ✅

This PR delivers on its promise: the featured gallery order is now a single value that moves atomically. The race condition identified by Macroscope was caught and fixed before merge. Tests are comprehensive. Error handling follows module conventions. No security, performance, or backwards-compatibility concerns.

The implementation is careful, well-documented, and demonstrates defensive programming (duplicate check, membership verification, row-count assertion). Great work.

---

Key files reviewed:

• src/api/v2/agent-templates/handlers/featured-order.ts:1-183
• tests/agent-templates.featured-order.test.ts:1-301
• tests/featured-order.serialization.test.ts:1-50
• src/api/v2/agent-templates/agent-templates.router.ts:82-87

[coderabbitai]

Walkthrough

Adds a protected PUT /api/v2/agent-templates/featured-order endpoint that validates complete gallery IDs, detects stale gallery state, and atomically updates featured ranks. Tests cover successful ordering, rejected mutations, authorization, and router middleware wiring.

Changes

Featured gallery ordering

Layer / File(s)	Summary
Request validation and authorization src/api/v2/agent-templates/handlers/featured-order.ts	Validates non-empty UUID lists capped at 200, rejects duplicates, and requires dashboard API-key authorization.
Gallery consistency and transactional ranks src/api/v2/agent-templates/handlers/featured-order.ts	Rejects incomplete or stale galleries with 409, then updates all featured ranks in one transaction.
Route wiring and endpoint coverage src/api/v2/agent-templates/agent-templates.router.ts, tests/agent-templates.featured-order.test.ts, tests/auth-middleware.test.ts	Mounts the protected endpoint and tests ordering, rejection cases, authorization, and middleware wiring.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• xmtplabs/convos-backend#178: Introduced the authentication middleware used by the new route.
• xmtplabs/convos-backend#207: Updated related agent-template router authentication wiring.

Suggested reviewers: fbac

Poem

I’m a rabbit arranging ranks,
With UUIDs lined in tidy banks.
One stale hop? The write says “no!”
Atomic paws make updates flow.
The dashboard key unlocks the show.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: transactional writing of the featured gallery order.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch saul/featured-order-bulk

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[macroscopeapp]

Approvability

Verdict: Needs human review

This PR adds a new API endpoint (PUT /featured-order) introducing new user-facing behavior with database transactions. The author does not own any of the modified files, which are all owned by xmtplabs/engineering. New features and changes to non-owned files warrant review by designated code owners.

^{You can customize Macroscope's approvability policy. Learn more.}

[saulmc]

Adjudicating this one against the review from Macroscope, since they disagree on the only thing that mattered.

"Transaction prevents race conditions" / "No security issues detected" — this was wrong, and Macroscope caught it (discussion). The membership check ran outside the transaction that wrote the weights, so the endpoint was a check-then-act across two round trips. A template unfeatured or unpublished between them stayed in the caller's list, took a weight it was no longer entitled to, and — because a weight is a slot — would silently reclaim that slot on the way back in. Fixed in 3a121e4: the read and the writes now share one SERIALIZABLE transaction.

Worth noting the fix isn't just "wrap it": Prisma's default is READ COMMITTED, where each statement takes a fresh snapshot, so moving the read inside a plain $transaction would have left the window open. Verified with SHOW transaction_isolation rather than assumed.

"Inconsistent error response shape" — declining. Both shapes are the house convention, and mixing them is what the codebase already does: Zod failures answer { error, details } (see agent-prompt-hints/handlers/reorder.ts and agent-templates/handlers/patch.ts, both of which do exactly this), while semantic failures answer { error: { code, message } }. Coercing the Zod branch into the coded shape would make this handler the odd one out rather than more consistent, and it would drop details, which is the useful part of a validation error.

"Extract hasDuplicates" — declining. It's a two-line check with a comment explaining why the guard exists; a named wrapper adds a hop without adding meaning. The sibling reorder handler expresses the same rule inline as a Zod .refine.

"Single UPDATE … CASE instead of N updates" — agreed, and deliberately not doing it. You reached the same conclusion in the comment: the gallery is a few dozen rows, the cap is 200, and the loop inside the transaction reads plainly. Worth revisiting only if AgentTemplate ever grows a gallery where that's measurable.

[coderabbitai]

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 `@src/api/v2/agent-templates/handlers/featured-order.ts`:
- Around line 104-110: Update the async handler containing the “Failed to set
featured gallery order” catch block to delegate failures to the global error
handler: remove the manual try/catch so Express 5 propagates rejected promises,
or rethrow an appropriate AppError instead of sending the response directly.
Preserve the existing success behavior while ensuring 500 errors use the
application-wide formatting and logging.
- Around line 13-19: Replace the local sendError helper with the established
AppError-based error flow, throwing AppError instances instead of directly
sending responses. Update each call site in the featured-order handler to pass
the appropriate error code, message, and status through AppError while
preserving the existing HTTP responses.

🪄 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: ab3973d6-6c17-44af-b646-5fdd444d148a

📥 Commits

Reviewing files that changed from the base of the PR and between 791f278 and 5bde8d1.

📒 Files selected for processing (4)

• src/api/v2/agent-templates/agent-templates.router.ts
• src/api/v2/agent-templates/handlers/featured-order.ts
• tests/agent-templates.featured-order.test.ts
• tests/auth-middleware.test.ts