fix(engine): reserve deterministic DM ids atomically - #303
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds workspace-scoped reservations for deterministic 1:1 DM conversation IDs. It backfills eligible conversations, rejects conflicting bindings with a 409 error, restores participants, audits migration data, and adds collision and concurrency tests. ChangesDeterministic 1:1 DM reservations
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant sendDm
participant ReservationTable
participant ConversationRecords
Client->>sendDm: Send a 1:1 DM
sendDm->>ReservationTable: Reserve workspace and participant pair
ReservationTable-->>sendDm: Return reservation or 409 collision
sendDm->>ConversationRecords: Create or reuse conversation metadata
ConversationRecords-->>Client: Return resolved conversation
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
scripts/audit-dm-reservations.mjsParsing error: /scripts/audit-dm-reservations.mjs was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProject. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d4e743dc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'dm_conversation_id_collision', | ||
| 409, |
There was a problem hiding this comment.
When a deterministic digest collision occurs, POST /dm now returns the new 409 dm_conversation_id_collision error, but this commit leaves the endpoint's OpenAPI responses declaring only 201 and does not update README.md. Add the new response to openapi.yaml and synchronize the README so generated API documentation and clients describe the observable failure mode, as required for API behavior changes.
AGENTS.md reference: AGENTS.md:L34-L36
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/engine/src/db/migrations/0033_dm_conversation_reservations.sql (1)
17-37: 🧹 Nitpick | 🔵 TrivialRun a pre-flight audit before applying this migration in production.
The backfill aborts the entire migration if any single legacy 1:1 conversation has zero or more than two distinct participants. This is intentional fail-closed behavior, but it means one corrupt legacy row blocks the whole deployment. Run a read-only audit query against production data before applying migration
0033, to find and remediate malformed rosters ahead of the cutover.🤖 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 `@packages/engine/src/db/migrations/0033_dm_conversation_reservations.sql` around lines 17 - 37, Before applying migration 0033, run a read-only audit grouped by 1:1 conversation to identify rows whose distinct participant count is not between 1 and 2, using the same dm_conversations and dm_participants criteria as the backfill. Remediate each malformed roster, then rerun the audit to confirm no invalid conversations remain before executing the migration.
🤖 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 `@packages/engine/src/db/schema.ts`:
- Around line 606-632: Add a composite foreign-key constraint to
dmConversationReservations linking workspaceId, participantOneId, and
participantTwoId to the corresponding sorted-pair key on dmParticipants,
ensuring each reservation matches an existing pair in the same workspace. Add or
reuse the required unique composite key on dmParticipants, and preserve the
existing sorted-pair check and uniqueness constraint.
---
Nitpick comments:
In `@packages/engine/src/db/migrations/0033_dm_conversation_reservations.sql`:
- Around line 17-37: Before applying migration 0033, run a read-only audit
grouped by 1:1 conversation to identify rows whose distinct participant count is
not between 1 and 2, using the same dm_conversations and dm_participants
criteria as the backfill. Remediate each malformed roster, then rerun the audit
to confirm no invalid conversations remain before executing the migration.
🪄 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: 66b6c072-65c4-4b64-a9a8-3ea3d0c39813
📒 Files selected for processing (8)
.agentworkforce/trajectories/completed/2026-08/traj_pzkvekhsexoq/summary.md.agentworkforce/trajectories/completed/2026-08/traj_pzkvekhsexoq/trajectory.jsonCHANGELOG.mdpackages/engine/CHANGELOG.mdpackages/engine/src/db/migrations/0033_dm_conversation_reservations.sqlpackages/engine/src/db/schema.tspackages/engine/src/engine/__tests__/dm.test.tspackages/engine/src/engine/dm.ts
| // ============================================ | ||
| // 1:1 DM Conversation Reservations | ||
| // ============================================ | ||
| export const dmConversationReservations = sqliteTable( | ||
| 'dm_conversation_reservations', | ||
| { | ||
| conversationId: text('conversation_id').primaryKey(), | ||
| workspaceId: text('workspace_id') | ||
| .notNull() | ||
| .references(() => workspaces.id, { onDelete: 'cascade' }), | ||
| participantOneId: text('participant_one_id').notNull(), | ||
| participantTwoId: text('participant_two_id').notNull(), | ||
| createdAt: integer('created_at', { mode: 'timestamp' }).notNull().default(sql`(unixepoch())`), | ||
| }, | ||
| (table) => [ | ||
| check( | ||
| 'dm_conversation_reservations_sorted_pair_check', | ||
| sql`${table.participantOneId} <= ${table.participantTwoId}`, | ||
| ), | ||
| uniqueIndex('dm_conversation_reservations_pair_unique').on( | ||
| table.workspaceId, | ||
| table.participantOneId, | ||
| table.participantTwoId, | ||
| ), | ||
| ], | ||
| ); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether dm_participants (and other agent-referencing tables) enforce an FK to agents, to compare with dmConversationReservations.
ast-grep run --pattern 'agentId: text($_).notNull().references($_)' --lang typescript packages/engine/src/db/schema.ts
rg -n -B3 -A3 'agentId' packages/engine/src/db/schema.ts | rg -n -B3 -A3 'dm_participants|dmParticipants'Repository: AgentWorkforce/relaycast
Length of output: 373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== schema agents and dm_participants/dmConversationReservations sections ==="
sed -n '520,660p' packages/engine/src/db/schema.ts
echo
echo "=== FK references to agents.id in schema ==="
rg -n '\.references\(\(\) => agents\.id|agentId[^:]*text\([^)]*\)\.notNull\(\)(\.[^;}]+)?\.references|agent_id' packages/engine/src/db/schema.ts
echo
echo "=== dm_participants table usage / delete paths ==="
rg -n 'dm_participants|dmParticipants|agentId|deleteFrom|where.*agentId|agentId.*where' packages/engine/src -g '*.ts'Repository: AgentWorkforce/relaycast
Length of output: 50380
Add a composite FK that enforces reserved DM pairs match dmParticipants rows.
dm_conversation_reservations.participantOneId and participantTwoId are not linked to dm_participants, while dmParticipants.agentId has a foreign key to agents.id. Add a SQL check/query constraint that each sorted pair exists in dm_participants for the same workspace_id, so deleted participants are not silently reclaimed as available valid reservation rows.
🤖 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 `@packages/engine/src/db/schema.ts` around lines 606 - 632, Add a composite
foreign-key constraint to dmConversationReservations linking workspaceId,
participantOneId, and participantTwoId to the corresponding sorted-pair key on
dmParticipants, ensuring each reservation matches an existing pair in the same
workspace. Add or reuse the required unique composite key on dmParticipants, and
preserve the existing sorted-pair check and uniqueness constraint.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Addresses PR #303 review feedback. The conditional upsert names conversation_id as its ON CONFLICT target, which SQLite limits to one. The pair_unique index on (workspace_id, participant_one_id, participant_two_id) is therefore not a conflict target, so the second collision shape -- same pair reserved under a DIFFERENT conversation_id -- raised an uncaught SQLITE_CONSTRAINT_UNIQUE and surfaced as a 500 rather than the controlled 409. It is reachable rather than theoretical: migration 0033 backfills whatever `dc.id` a legacy 1:1 already had, without requiring it to equal the current derivation, so the pair can end up reserved under an id the next send will not re-derive. An out-of-band write does the same. Failing closed was already true; failing closed with the DOCUMENTED code was not, and a caller cannot distinguish a refused collision from an engine fault. The insert now narrows that constraint failure -- matched on this table only, so an unrelated constraint error is never laundered into a tidy 409 -- and raises the same coded error as the primary-key path. Regression test added, and its negative control confirmed: with the handler disabled, exactly one test flips and reports "expected 'SQLITE_CONSTRAINT_UNIQUE' to be 'dm_conversation_id_collision'". Also from review: - openapi.yaml and README.md document the 409 and both shapes that produce it, per the AGENTS.md docs-hygiene rule for API behaviour changes. - Migration 0033 carries the read-only pre-flight audit query operators should run before applying it, since the fail-closed backfill aborts the deployment on a single malformed legacy roster. The query is verified to flag 0- and 3-participant 1:1s while ignoring valid and group rows. NOT taken: the suggestion to add a composite FK binding reservation participants to dm_participants. The reservation is written BEFORE any roster row exists -- that ordering is the point of the seam, since the reservation must win before conversation state is created -- so such an FK would reject every new conversation. engine 526/526, build 9/9, lint 3/3, git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review feedback addressed in Taken1. cubic P2 — pair-unique collision surfaced as a 500 ( SQLite accepts one Failing closed was already true; failing closed with the documented code was not, and a caller cannot tell a refused collision from an engine fault. The insert now narrows that constraint failure — matched on this table only, so an unrelated constraint error is never laundered into a tidy 409 — and raises the same coded error as the primary-key path. Regression test added, plus its negative control: with the handler disabled, exactly one test flips, with the message above. 2. Codex P2 — document the 409. Done in 3. CodeRabbit nitpick — pre-flight audit. Agreed, and rather than only warning, migration DeclinedCodeRabbit major — composite FK from reservations to The reservation is written before any roster row exists — The invariant the suggestion is reaching for — a reservation should not outlive its roster — is real, but a foreign key in this direction cannot express it. It would need to be a deletion-path concern instead. Happy to open a follow-up if you want that tracked. VerificationNote the count differs from the Still needs a |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Addresses the P1 and P3 raised on the previous review round. P1. The first version of this handler matched `.code` and `.message` on the top-level error only. That passes against better-sqlite3, which is what the test suite runs, and would have regressed in the hosted engine, which runs Cloudflare D1 via drizzle-orm/d1 -- D1 prefixes its message with "D1_ERROR: " and drizzle may re-wrap the driver error under `.cause` rather than surfacing it at the top level. This engine has already shipped that exact regression once: agent.ts gained isUniqueConstraintError in PR #193 after clean 409 handling became an uncaught 500 against D1 for the same reason, and observerToken.ts documents the trap. I reproduced the shape rather than reasoning about it, and reused the established cycle-safe walk instead of inventing a third variant. Detection now requires BOTH conditions somewhere in the cause chain: a unique violation, and a reference to dm_conversation_reservations. They are tracked independently because a wrapper can carry the code while only the wrapped cause carries the message. Keeping the table condition is what stops a FOREIGN KEY or NOT NULL failure on the same insert from being reported as a participant-pair conflict, which would be a tidy 409 that says something untrue. The walk is iterative with a WeakSet of visited objects, breaking on any revisit rather than a direct self-reference, so a multi-step cycle cannot blow the stack and turn the check meant to prevent a 500 into one. Tests assert all four shapes directly, plus the cases that must NOT match and a cyclic chain. Negative control: restricting detection to the top level fails the driver-shape test alone. P3. The concurrency test is renamed to "rejects the losing pair when two colliding sends interleave". Both real sendDm paths are launched before either is awaited, but better-sqlite3 is a single synchronous connection, so they serialize and there is no genuine database contention. It proves the collision branch and the ordering of the reservation seam; it does not prove multi-writer atomicity against a networked engine. The constraint is what provides atomicity and the DO NOTHING control shows it is load-bearing. Named for what it demonstrates. engine 529/529, build 9/9, lint 3/3, git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Second round addressed in cubic P1 — hosted collisions could still 500 · valid, fixedThis was the important one and it was right. My previous fix matched Worth saying plainly: this engine has already shipped that exact regression once. Detection now requires both conditions somewhere in the cause chain — a unique violation, and a reference to The walk is iterative with a Four shapes now asserted directly (better-sqlite3; D1 top-level; D1 re-wrapped under cubic P3 — the test says concurrency, the driver serializes · valid, renamedCorrect, and I had already flagged the same caveat to our partner rather than let the word carry more weight than it earns. Renamed to “rejects the losing pair when two colliding sends interleave.” Both real Codex P2 — document the 409 · already addressed in
|
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Raised in review of PR #303, and the reviewer is right. The audit I added last round checked roster shape only, then claimed an empty result meant the migration would apply cleanly. It does not. Two DISTINCT legacy 1:1 conversations in one workspace that resolve to the same sorted pair both pass the shape check and still abort the backfill on dm_conversation_reservations_pair_unique. Reproduced before fixing: shape audit flags: NOTHING (claims clean) backfill: ABORTED - SQLITE_CONSTRAINT_UNIQUE That is the likelier of the two failures in practice, since duplicate pairs are exactly what a pre-deterministic id scheme leaves behind - so the audit was silent on the case operators are most likely to hit. The comment now carries both queries, says both must be empty, and notes that remediating (b) means deciding which conversation survives, because the reservation can only bind one. Both queries verified against a seeded database: shape catches a 3-participant row, duplicate-pair catches the colliding pair, and valid 1:1s and group DMs are ignored by both. Documentation only; no schema or code change. engine 529/529, build 9/9, lint 3/3, git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Third round in cubic P3 — migration audit was incomplete · valid, fixedGood catch, and it was an overclaim in my own words. The audit I added checked roster shape only, then asserted an empty result meant the migration would apply cleanly. It does not. I reproduced it before fixing: Two distinct legacy 1:1 conversations in one workspace resolving to the same sorted pair both pass the shape check and still abort on The comment now carries both queries, states that both must be empty, and notes that remediating (b) means deciding which conversation survives, since the reservation can only bind one. Both queries verified against a seeded DB: (a) catches a 3-participant roster, (b) catches the colliding pair, and valid 1:1s plus group DMs are ignored by both. Documentation only — no schema or code change. Re-anchored comments from earlier roundsThree comments re-posted against the new line numbers but carry their original text; all were addressed before this round:
VerificationStill needs a |
AGENTS.md:45-46 asks for one short impact-first bullet per user-visible change and explicitly excludes PR links, internal review notes, and implementation backstory. My entries had grown to three bullets in the engine changelog and two at the root, describing which index was violated, which error shapes are walked, and citing PR #193 -- none of which a reader of release notes needs. Consolidated to one bullet each, stated as impact: a collision returns 409 dm_conversation_id_collision on every driver rather than aliasing another pair's conversation or failing with an uncaught database error. The engine entry keeps the migration reference, which AGENTS.md:43 does ask package changelogs to carry, and points at the migration header for the pre-flight audit. The regression backstory stays where it is useful: in the code comments beside the detector it explains. engine 529/529, build 9/9, lint 3/3, git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fourth round in cubic P3 — changelog carried backstory · valid, fixedCorrect, and I checked the rule rather than taking the quote on faith: My entries had grown to three bullets in the engine changelog and two at the root, describing which index was violated, which error shapes get walked, and citing PR #193 — none of which a reader of release notes needs. Consolidated to one bullet each, stated as impact:
The migration reference stays because Remaining comments are re-anchors from earlier roundsAll three were addressed before this commit and carry their original text against new line numbers:
VerificationFive review findings taken across four rounds, one declined with reasoning. Still needs a |
The two SQL audits in the 0033 header cover the failures that ABORT the migration. They do not cover the one that does not: a 1:1 conversation whose id is not what deriveDmPairKey produces today. That case is quiet and it is the dangerous one. The backfill reserves whatever `dc.id` the conversation already had, so the pair ends up bound to an id the send path will never re-derive. The migration succeeds, nothing complains, and then every subsequent DM between that pair returns 409 forever. Before this PR the same rows would have silently created a duplicate conversation instead, so the change converts a quiet data problem into a loud one - which is right, but it means latent mess surfaces at deploy time. It cannot be written in SQL because SQLite has no SHA-256, so it is a script. It runs all three checks, reads either a SQLite file or `wrangler d1 execute --json` on stdin so both deployment shapes are covered, and is read-only. Git history says the derivation never changed - the only edit swapped node crypto for web crypto with a byte-identical input string. This exists because "stable in git history" and "zero rows in production disagree" are different claims and only the second is evidence. Verified against a seeded database: flags a 3-participant roster, a duplicate pair, and two mismatched ids, while leaving a correct pair, a self-DM, the same pair in a different workspace, and a group DM alone. Clean fixtures exit 0. Docs and tooling only; no schema or engine change. engine 529/529, build 9/9, lint 3/3, git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deployment risk — closed the gap I found in my own auditPushed The three ways migration 0033 can go wrong are not equally visible:
The two SQL audits I added earlier only cover (a) and (b) — the loud ones. (c) was uncovered, and it is the dangerous one. The backfill reserves whatever It cannot be written in SQL — SQLite has no SHA-256 — so How likely is (c)? Low. Git history says the derivation never changed — the only edit swapped node crypto for web crypto with a byte-identical input string, and I checked the diff rather than assuming. But "stable in git history" and "zero rows in production disagree" are different claims, and only the second is evidence. Now it is one command to get. Verified against a seeded database: flags a 3-participant roster, a duplicate pair, and two mismatched ids, while leaving a correct pair, a self-DM, the same pair in a different workspace, and a group DM alone. Clean fixtures exit One behaviour change worth stating explicitly for whoever reviews thisBefore this PR, a case-(c) row would have silently created a duplicate conversation. After it, that same row returns a hard Deployment orderMigration engine 529/529, build 9/9, lint 3/3, clean diff. Still needs a |
Two properties this change depends on in production that nothing asserted. Self-DM. A self-DM has one roster row, so the reservation stores the same agent as both participants. That satisfies the sorted-pair CHECK only because the comparison is non-strict. `@self` is a documented request shape, so a stricter constraint would have broken a live feature with no test to catch it. Orphaned reservation. The reservation and the conversation/roster inserts are NOT one transaction, so a crash between them leaves a reservation with no conversation. Recovery depends on the retry presenting the identical tuple, which the conditional upsert accepts. I had asserted that was self-healing when reasoning about deployment risk; now it is tested. If re-resolution ever stopped accepting an identical tuple, one mid-write crash would lock that pair out of DMs permanently and nothing else in this suite would notice. Both pass today. Tests only; no behaviour change. engine 531/531, build 9/9, lint 3/3, git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/audit-dm-reservations.mjs (1)
49-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard against derivation drift with more than a comment.
deriveConversationIdduplicates the SHA-256 derivation thatderiveDmPairKeyimplements in dm.ts. The comment on Line 48 states it "Must stay byte-identical," but nothing enforces that beyond the comment itself. If either implementation changes independently, this script will silently pass or fail conversations incorrectly, and the failure would only surface as unexplained mismatches in production audits.Add a small cross-check (for example, a unit test that imports or calls both derivations with the same fixed input vector and asserts equality) so a future edit to either implementation fails a test instead of silently drifting.
🤖 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 `@scripts/audit-dm-reservations.mjs` around lines 49 - 56, Add a focused test that invokes both deriveConversationId and deriveDmPairKey with the same fixed workspace and agent IDs, then asserts their derived values are equal. Keep the existing derivation unchanged and ensure the test fails if either implementation drifts from the other.
🤖 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 `@scripts/audit-dm-reservations.mjs`:
- Around line 114-123: Update the roster-building loop around conversations and
introduce droppedRows tracking for every row missing id, workspaceId, or agentId
instead of silently continuing. Add a report finding for droppedRows.length
alongside malformed, duplicates, and mismatched, and include that finding in the
exit-code calculation so skipped rows fail the audit.
---
Nitpick comments:
In `@scripts/audit-dm-reservations.mjs`:
- Around line 49-56: Add a focused test that invokes both deriveConversationId
and deriveDmPairKey with the same fixed workspace and agent IDs, then asserts
their derived values are equal. Keep the existing derivation unchanged and
ensure the test fails if either implementation drifts from the other.
🪄 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: 8635c665-6bcd-4b2a-9d6b-d96d53e187ad
📒 Files selected for processing (6)
CHANGELOG.mdpackages/engine/CHANGELOG.mdpackages/engine/src/db/migrations/0033_dm_conversation_reservations.sqlpackages/engine/src/engine/__tests__/dm.test.tspackages/engine/src/engine/dm.tsscripts/audit-dm-reservations.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- packages/engine/src/engine/dm.ts
- packages/engine/CHANGELOG.md
| // Group the roster per conversation. | ||
| const conversations = new Map(); | ||
| for (const row of rows) { | ||
| const id = row.id ?? row.conversation_id; | ||
| const workspaceId = row.workspace_id; | ||
| const agentId = row.agent_id; | ||
| if (!id || !workspaceId || !agentId) continue; | ||
| if (!conversations.has(id)) conversations.set(id, { id, workspaceId, agents: new Set() }); | ||
| conversations.get(id).agents.add(agentId); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Report rows dropped for missing fields instead of silently skipping them.
The continue on Line 120 discards any row missing id, workspaceId, or agentId without recording it. This script exists to catch anomalies before an irreversible migration. A row with a corrupt agent_id is exactly the kind of anomaly checks (a)/(b)/(c) are designed to catch, but this loop removes it before the roster is even built.
Two failure modes follow:
- A conversation with a mix of valid and corrupt participant rows can end up with an apparently valid 1-2 person roster and pass check (a) undetected.
- A conversation whose only rows are corrupt never enters the
conversationsmap, so it is invisible to every subsequent check.
Track and report skipped rows so the audit surfaces them instead of silently hiding them.
🛠️ Proposed fix to surface dropped rows
+const droppedRows = [];
for (const row of rows) {
const id = row.id ?? row.conversation_id;
const workspaceId = row.workspace_id;
const agentId = row.agent_id;
- if (!id || !workspaceId || !agentId) continue;
+ if (!id || !workspaceId || !agentId) {
+ droppedRows.push(row);
+ continue;
+ }
if (!conversations.has(id)) conversations.set(id, { id, workspaceId, agents: new Set() });
conversations.get(id).agents.add(agentId);
}Then add a section that reports droppedRows.length as a finding (and includes it in the exit-code calculation) alongside malformed, duplicates, and mismatched.
🤖 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 `@scripts/audit-dm-reservations.mjs` around lines 114 - 123, Update the
roster-building loop around conversations and introduce droppedRows tracking for
every row missing id, workspaceId, or agentId instead of silently continuing.
Add a report finding for droppedRows.length alongside malformed, duplicates, and
mismatched, and include that finding in the exit-code calculation so skipped
rows fail the audit.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/audit-dm-reservations.mjs">
<violation number="1" location="scripts/audit-dm-reservations.mjs:120">
P2: The audit can certify incomplete stdin data as clean because rows missing required fields are silently skipped. Rejecting invalid rows, rather than continuing, prevents a partial export from hiding migration findings.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const id = row.id ?? row.conversation_id; | ||
| const workspaceId = row.workspace_id; | ||
| const agentId = row.agent_id; | ||
| if (!id || !workspaceId || !agentId) continue; |
There was a problem hiding this comment.
P2: The audit can certify incomplete stdin data as clean because rows missing required fields are silently skipped. Rejecting invalid rows, rather than continuing, prevents a partial export from hiding migration findings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/audit-dm-reservations.mjs, line 120:
<comment>The audit can certify incomplete stdin data as clean because rows missing required fields are silently skipped. Rejecting invalid rows, rather than continuing, prevents a partial export from hiding migration findings.</comment>
<file context>
@@ -0,0 +1,192 @@
+ const id = row.id ?? row.conversation_id;
+ const workspaceId = row.workspace_id;
+ const agentId = row.agent_id;
+ if (!id || !workspaceId || !agentId) continue;
+ if (!conversations.has(id)) conversations.set(id, { id, workspaceId, agents: new Set() });
+ conversations.get(id).agents.add(agentId);
</file context>
The audit found this against real production data, and as written the migration WOULD HAVE FAILED THE DEPLOYMENT. Production has 3562 1:1 conversations. The old backfill used MIN/MAX over 1-2 distinct participants, which produced 4 colliding pair groups and 30 mismatched ids. The colliding groups abort the migration on the pair-uniqueness index; the mismatches would have made those agents 409 on every subsequent DM. None of the 30 were self-DMs. `dm_participants.agent_id` references `agents.id` ON DELETE CASCADE, so deleting an agent silently removes its participant rows and an ordinary two-party 1:1 collapses to a ONE-ROW ROSTER while its id still encodes the ORIGINAL pair. The backfill read those as (X, X). Several orphans belonging to the same surviving agent then collapse onto the same tuple and collide. I checked the obvious explanation first and it was wrong: no current agent in those workspaces reproduces the stored id, so the peer agent rows are gone entirely, not merely departed. `left_at` is null on the survivors. The backfill now reserves only conversations with exactly two distinct participants. Against the same production data: 3425 reserved, zero duplicates, zero mismatches, 137 skipped (107 genuine self-DMs, 30 orphans). Skipping is safe rather than convenient. An unreserved conversation is in exactly the state every conversation was in before this migration - the first send through the reservation path claims it, and because a genuine self-DM's id already equals its derivation, that claim ADOPTS the existing conversation instead of forking a second one. That property is now a test; without it every pre-migration self-DM would silently split on first use and its history would disappear from the user's view. Malformed rosters (zero or more than two) are skipped for the same reason. The old version aborted the entire migration on them, which avoids inventing a tuple no more effectively than skipping does, and blocks a deployment. Two-party conversations, which are the ones that matter, were never at risk: 3425 of them, zero mismatches. The derivation has been stable exactly as git history suggested. engine 532/532, build 9/9, lint 3/3, git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/audit-dm-reservations.mjs">
<violation number="1" location="scripts/audit-dm-reservations.mjs:160">
P3: Zero-participant malformed conversations are never reported even though this branch classifies them as malformed: the inner-join input and grouping logic make an empty roster impossible. Preserve conversation rows with no participants in the audit input, or remove the unreachable zero-roster classification so the audit does not imply coverage it lacks.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| participants: agents.length, | ||
| kind: agents.length === 1 ? (looksLikeSelfDm ? 'self-DM' : 'orphaned two-party') : 'malformed', | ||
| }); | ||
| if (agents.length === 0 || agents.length > 2) { |
There was a problem hiding this comment.
P3: Zero-participant malformed conversations are never reported even though this branch classifies them as malformed: the inner-join input and grouping logic make an empty roster impossible. Preserve conversation rows with no participants in the audit input, or remove the unreachable zero-roster classification so the audit does not imply coverage it lacks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/audit-dm-reservations.mjs, line 160:
<comment>Zero-participant malformed conversations are never reported even though this branch classifies them as malformed: the inner-join input and grouping logic make an empty roster impossible. Preserve conversation rows with no participants in the audit input, or remove the unreachable zero-roster classification so the audit does not imply coverage it lacks.</comment>
<file context>
@@ -126,16 +133,37 @@ const malformed = [];
+ participants: agents.length,
+ kind: agents.length === 1 ? (looksLikeSelfDm ? 'self-DM' : 'orphaned two-party') : 'malformed',
+ });
+ if (agents.length === 0 || agents.length > 2) {
+ malformed.push({ id: conv.id, workspaceId: conv.workspaceId, participants: agents.length });
+ }
</file context>
The pre-flight failed once mid-session with 'could not find a results array', on the same query that had worked minutes earlier. A deploy gate that fails intermittently, with an error that says nothing about what it received, is worse than no gate. It now skips any preamble, searches for the first array whose members carry agent_id and workspace_id rather than enumerating envelope shapes, treats an empty result set as legitimate, and on failure prints what it actually got plus the aliases it expects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/audit-dm-reservations.mjs">
<violation number="1" location="scripts/audit-dm-reservations.mjs:86">
P2: A banner or warning containing `[` or `{` still breaks the deploy-gate parser, despite this change claiming to tolerate preambles; parse candidate JSON starts until a valid envelope is found rather than trusting the first delimiter.</violation>
<violation number="2" location="scripts/audit-dm-reservations.mjs:119">
P2: An empty bare-array result is reported as invalid input instead of a clean audit, so deployments with no 1:1 DMs can fail the pre-flight gate when Wrangler emits `[]`; treat an empty top-level array as an empty result set too.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const rows = find(parsed); | ||
| if (!rows) { | ||
| // An empty result set is legitimate: a deployment with no 1:1 DMs at all. | ||
| if (JSON.stringify(parsed).includes('"results":[]')) return []; |
There was a problem hiding this comment.
P2: An empty bare-array result is reported as invalid input instead of a clean audit, so deployments with no 1:1 DMs can fail the pre-flight gate when Wrangler emits []; treat an empty top-level array as an empty result set too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/audit-dm-reservations.mjs, line 119:
<comment>An empty bare-array result is reported as invalid input instead of a clean audit, so deployments with no 1:1 DMs can fail the pre-flight gate when Wrangler emits `[]`; treat an empty top-level array as an empty result set too.</comment>
<file context>
@@ -79,17 +79,50 @@ async function loadRows(argv) {
+ const rows = find(parsed);
+ if (!rows) {
+ // An empty result set is legitimate: a deployment with no 1:1 DMs at all.
+ if (JSON.stringify(parsed).includes('"results":[]')) return [];
+ usage(
+ 'could not find a row array containing agent_id and workspace_id.\n'
</file context>
| if (JSON.stringify(parsed).includes('"results":[]')) return []; | |
| if ((Array.isArray(parsed) && parsed.length === 0) | |
| || JSON.stringify(parsed).includes('"results":[]')) return []; |
| // depending on version and TTY attachment, and an operator running this as a | ||
| // deploy gate should not have to care. A brittle parser here fails exactly | ||
| // when it matters most. | ||
| const start = raw.search(/[[{]/); |
There was a problem hiding this comment.
P2: A banner or warning containing [ or { still breaks the deploy-gate parser, despite this change claiming to tolerate preambles; parse candidate JSON starts until a valid envelope is found rather than trusting the first delimiter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/audit-dm-reservations.mjs, line 86:
<comment>A banner or warning containing `[` or `{` still breaks the deploy-gate parser, despite this change claiming to tolerate preambles; parse candidate JSON starts until a valid envelope is found rather than trusting the first delimiter.</comment>
<file context>
@@ -79,17 +79,50 @@ async function loadRows(argv) {
+ // depending on version and TTY attachment, and an operator running this as a
+ // deploy gate should not have to care. A brittle parser here fails exactly
+ // when it matters most.
+ const start = raw.search(/[[{]/);
+ if (start === -1) usage(`stdin contained no JSON. First 200 chars:\n${raw.slice(0, 200)}`);
+
</file context>
2 participants
What changed
dm_conversation_reservationstuple table keyed by deterministic conversation ID(conversation_id, workspace_id, sorted participant pair)with one conditional upsert before creating DM metadatadm_conversation_id_collisionwhen the ID is already bound to a different tuple0033, aborting on malformed legacy rosterssendDmpathPromise.allSettledtest that launches two conflicting real sends concurrently and proves exactly one winsWhy
The previous resolver derived a stable ID, then inserted the conversation with
ON CONFLICT DO NOTHING. Because the stored conversation row did not include the participant tuple, a digest collision could silently resolve a second pair onto the first pair's conversation. A read-before-write check would still race; the tuple has to be claimed by a database uniqueness boundary and checked in the same upsert statement.Impact
Normal first resolution and idempotent re-resolution retain the same deterministic IDs. A conflicting tuple now receives a hard
409 dm_conversation_id_collisioninstead of aliasing another conversation. Existing 1:1/self-DM rows are backfilled during migration.This PR needs a
dm.tsowner review before merge.Validation
npx turbo build --env-mode=loose: 9/9 tasks passednpx vitest run packages/engine: 50 files, 536 tests passednpx turbo lint --filter=@relaycast/engine --env-mode=loose: 3/3 tasks passedNegative control: temporarily removed the
conversation_idprimary-key uniqueness constraint from migration0033, rebuilt, and reran the concurrent test. With no matching database uniqueness boundary, both conflict-target upserts rejected and the test failed exactly as intended:Additional repository-wide diagnostics:
npx vitest run: 107 files / 1,373 tests passed; 11 unrelated failures because root invocation lacks the React jsdom environment and types globalsnpx turbo test --env-mode=loose: two SDK tests timed out at 5 seconds under nine-package parallel load; both passed in the isolated SDK rerun above