feat(engine): module substrate wave 1 — actor model, drafts, assistants, webhooks, event outbox (HT-68) - #77
Conversation
…ts, webhooks, event outbox schema+store (HT-68) Ships the schema and store layer for the Helpthread module substrate (out- of-process AI-actor extensions): migrations 020-023 add `assistants`, `threads`' actor-model columns (author_kind/author_agent_id/ author_assistant_id, draft_status + audit fields, and the spec §2 delivery/ draft CHECK replacing migration 007's constraint), `webhook_endpoints`, and `event_outbox`. Store layer: every thread writer now supplies author_kind (derived from direction when omitted); new ConversationStore.appendDraft/ listAwaitingDrafts/resolveDraft methods; new AssistantStore, WebhookEndpointStore (encrypt-at-rest via the existing token-crypto envelope, auto-disable at 20 consecutive failures), and EventOutboxStore (transactional-outbox append + FOR UPDATE SKIP LOCKED claim/drain, mirroring the queue_jobs idiom). API routes, auth wiring, event emission call sites, approval orchestration, and webhook delivery are explicitly out of scope (waves 2/3). Found and fixed a NULL-trap bug in spec §2's literal CHECK predicate (an ordinary outbound row with draft_status/delivery_status both NULL would have wrongly passed) — the same trap migration 002's own doc comment already names; restored with the same IS NOT NULL guards. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…olveDraft double-resolution tests (HT-68) Opus review of the wave-1 commit (0 blockers/majors, 2 minors) — both closed: 1. migration 021: drop the `DEFAULT 'agent'` masking default on threads.author_kind and replace it with a real, enforced invariant — `threads_author_kind_direction_check CHECK ((direction = 'inbound') = (author_kind = 'customer'))`. A default only hid a hand-written INSERT that forgot the column; the biconditional actually rejects a mislabeled row (inbound-as-agent, outbound-as-customer) at write time, not just at migration time. Fixed every raw-SQL `threads` test fixture that ran against the full post-021 schema to supply an explicit, correct author_kind; added a test pinning the exact case the reviewer named (an inbound row labeled 'agent' is rejected). 2. Test pinning: (a) the migration-021 CHECK test now explicitly asserts the NULL-trap state itself — an ordinary outbound row with draft_status AND delivery_status both NULL — is rejected, pinned where the predicate lives; (b) resolveDraft gets a new test covering approve-then-discard and approve-twice, both finding no awaiting_review row and leaving the approved row's message_id/envelope untouched (not just discard-twice). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds migrations 020–023 and persistence stores for assistants, actor-aware conversation drafts, encrypted webhook endpoints, and transactional event outbox processing. It also expands migration, store, delivery, and schema integration tests and updates store barrel exports. ChangesPersistence and Store Extensions
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ConversationStore
participant Database
participant DeliveryWorker
ConversationStore->>Database: insert awaiting-review draft
ConversationStore->>Database: approve or discard draft
DeliveryWorker->>Database: query and claim deliverable threads
Database-->>DeliveryWorker: return approved drafts and ordinary deliverable threads
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/db/migrate.ts`:
- Around line 1117-1127: Update the foreign-key definitions for author_agent_id
and approved_by_agent_id in the threads migration to use ON DELETE SET NULL,
while leaving author_assistant_id’s foreign key strict without cascading
behavior. Preserve the existing column types, references, and constraints.
In `@src/store/event-outbox.ts`:
- Around line 140-159: Validate options.leaseMs at the start of claimBatch and
reject zero or negative values before executing the UPDATE query. Preserve the
existing claiming behavior for positive lease durations.
In `@src/store/webhook-endpoints.ts`:
- Around line 75-78: Update the documentation for CreatedWebhookEndpoint to
state that create returns the plaintext secret once, while normal endpoint
records omit the secret. Remove the inaccurate claim that the secret is never
re-derivable, and keep the interface shape unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf9ab0b3-0e4a-42db-bfeb-52542f0d6ff5
📒 Files selected for processing (12)
src/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/store/assistants.test.tssrc/store/assistants.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/event-outbox.test.tssrc/store/event-outbox.tssrc/store/index.tssrc/store/webhook-endpoints.test.tssrc/store/webhook-endpoints.ts
| ALTER TABLE threads ADD COLUMN author_agent_id uuid REFERENCES agents(id); | ||
| ALTER TABLE threads ADD COLUMN author_assistant_id uuid REFERENCES assistants(id); | ||
| ALTER TABLE threads ADD CONSTRAINT threads_author_identity_check CHECK ( | ||
| (author_kind = 'customer' AND author_agent_id IS NULL AND author_assistant_id IS NULL) | ||
| OR (author_kind = 'assistant' AND author_assistant_id IS NOT NULL AND author_agent_id IS NULL) | ||
| OR (author_kind = 'agent' AND author_assistant_id IS NULL) | ||
| ); | ||
| ALTER TABLE threads ADD COLUMN draft_status text; | ||
| ALTER TABLE threads ADD CONSTRAINT threads_draft_status_check CHECK (draft_status IS NULL OR draft_status IN ('awaiting_review','approved','discarded')); | ||
| ALTER TABLE threads ADD CONSTRAINT threads_draft_status_outbound_only CHECK (draft_status IS NULL OR direction = 'outbound'); | ||
| ALTER TABLE threads ADD COLUMN approved_by_agent_id uuid REFERENCES agents(id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find agent deletion paths and existing ON DELETE conventions on agents() FKs.
rg -nP "DELETE\s+FROM\s+agents\b" --type ts -C2
rg -nP "REFERENCES\s+agents\s*\(id\)" --type ts -C1Repository: Helpthread/helpthread
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map migrate.ts and inspect the relevant slice.
wc -l src/db/migrate.ts
sed -n '1088,1140p' src/db/migrate.ts
# Find the migration 020 doc comment / policy references.
rg -n "created_by_agent_id|assignee_agent_id|ON DELETE SET NULL|record outlives the pointer|migration 020" src/db/migrate.ts src -C 2
# Find other agents(id) foreign keys and their ON DELETE actions.
rg -n "REFERENCES agents\\(id\\)(?:\\s+ON DELETE [A-Z ]+)?" src -g '!**/node_modules/**' -C 1
# Search for agent deletion semantics outside direct DELETE SQL.
rg -n "\\bagent(s)?\\b.*\\b(delete|deleted|soft delete|archiv|deactivat|disable)\\b|\\b(delete|deleted|soft delete|archiv|deactivat|disable)\\b.*\\bagent(s)?\\b" src -i -C 2Repository: Helpthread/helpthread
Length of output: 40945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/db/migrate.ts
sed -n '1088,1140p' src/db/migrate.ts
rg -n "created_by_agent_id|assignee_agent_id|ON DELETE SET NULL|record outlives the pointer|migration 020" src/db/migrate.ts src -C 2
rg -n "REFERENCES agents\\(id\\)(?:\\s+ON DELETE [A-Z ]+)?" src -g '!**/node_modules/**' -C 1
rg -n "\\bagent(s)?\\b.*\\b(delete|deleted|soft delete|archiv|deactivat|disable)\\b|\\b(delete|deleted|soft delete|archiv|deactivat|disable)\\b.*\\bagent(s)?\\b" src -i -C 2Repository: Helpthread/helpthread
Length of output: 40945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo TESTRepository: Helpthread/helpthread
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find thread approval semantics and any field-level documentation.
rg -n "approved_by_agent_id|author_agent_id|author_assistant_id|threads_author_identity_check|draft_status|awaiting_review|approved|discarded" src -C 2
# Inspect the thread migration block around the field definitions and any nearby doc comments.
sed -n '940,1045p' src/db/migrate.ts
sed -n '1108,1135p' src/db/migrate.ts
# Look for tests that exercise deleting an Agent referenced by threads.
rg -n "approved_by_agent_id|author_agent_id|threads.*DELETE FROM agents|DELETE FROM agents WHERE id = .*threads|thread.*agent.*delete" src -C 2Repository: Helpthread/helpthread
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the migration 021 prose around the identity/audit columns.
sed -n '1036,1062p' src/db/migrate.ts
sed -n '1058,1068p' src/db/migrate.ts
# Look for tests around deleting agents referenced by threads.
rg -n "author_agent_id|approved_by_agent_id|DELETE FROM agents WHERE id = .*thread|thread.*delete.*agent|agents.*thread.*DELETE" src/db/migrate.test.ts src/store -C 2
# Check whether thread rows are ever inserted with approved_by_agent_id set and then an agent delete path is exercised.
rg -n "approvedByAgentId|approved_by_agent_id.*DELETE FROM agents|authorAgentId|author_agent_id.*DELETE FROM agents" src -C 2Repository: Helpthread/helpthread
Length of output: 13262
Add ON DELETE SET NULL to the Agent thread FKs. author_agent_id and approved_by_agent_id should follow the schema’s existing “record outlives the pointer” pattern; otherwise hard-deleting an Agent will fail on threads that reference it. author_assistant_id should stay strict because the assistant-authored branch requires a value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/migrate.ts` around lines 1117 - 1127, Update the foreign-key
definitions for author_agent_id and approved_by_agent_id in the threads
migration to use ON DELETE SET NULL, while leaving author_assistant_id’s foreign
key strict without cascading behavior. Preserve the existing column types,
references, and constraints.
| async claimBatch(options) { | ||
| // The subquery's `ORDER BY occurred_at` picks WHICH rows are claimed | ||
| // (the oldest-eligible batch) — it does NOT guarantee the outer | ||
| // UPDATE...RETURNING emits them in that order (Postgres makes no such | ||
| // promise for RETURNING). Sort the mapped results here so the | ||
| // interface's "oldest-occurred_at-first" contract holds regardless. | ||
| const rows = await db.query<OutboxEventRow>( | ||
| `UPDATE event_outbox | ||
| SET locked_until = now() + ($1::double precision * interval '1 millisecond') | ||
| WHERE event_id IN ( | ||
| SELECT event_id FROM event_outbox | ||
| WHERE dispatched_at IS NULL | ||
| AND (locked_until IS NULL OR locked_until < now()) | ||
| ORDER BY occurred_at | ||
| FOR UPDATE SKIP LOCKED | ||
| LIMIT $2 | ||
| ) | ||
| RETURNING ${OUTBOX_EVENT_COLUMNS}`, | ||
| [options.leaseMs, options.batchSize], | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the mentioned lines
wc -l src/store/event-outbox.ts
sed -n '1,220p' src/store/event-outbox.ts
# Find all call sites and option/type definitions
rg -n "claimBatch\(" src
rg -n "batchSize|leaseMs" src/store src | sed -n '1,200p'Repository: Helpthread/helpthread
Length of output: 22771
🌐 Web query:
PostgreSQL LIMIT -1 behavior negative limit no limit docs now() interval negative milliseconds reclaimable lease semantics
💡 Result:
PostgreSQL behavior regarding the requested topics is as follows: LIMIT and Negative Values In PostgreSQL, the LIMIT clause requires a non-negative integer [1][2]. Historically, while some older versions might have accepted negative values without error (returning zero rows), it is not a supported feature, and using negative numbers for LIMIT is generally considered invalid [1][3][4]. Omitting the LIMIT clause or using LIMIT ALL results in no limit being applied to the returned row set [5][2][6]. now and Interval Arithmetic The function now returns the current transaction's start time [7][8]. Subtracting an interval of negative milliseconds from now is mathematically equivalent to adding the corresponding positive duration [7][8][9]. While interval literals can be written with various units, to subtract specific millisecond values reliably, one can use expressions such as: SELECT now - (INTERVAL '1 millisecond' * 5); [9] Or by concatenating values: SELECT now - ('5 ms')::interval; [9] PostgreSQL handles these interval calculations accurately, treating negative signs in intervals according to specific rules, though it is best practice to avoid ambiguity by explicitly signing fields or using multiplication [7][8][9]. Lease Semantics in PostgreSQL "Reclaimable lease" is not a built-in semantic or keyword in the core PostgreSQL database engine [10][11]. Instead, lease management—including reclamation—is typically implemented at the application layer using PostgreSQL as a reliable transactional backend [10][11]. Distributed systems often implement leases by using rows in a dedicated table to represent locks, with expiration timestamps managed by the application [12][11]. Reclamation in this context refers to the process of detecting expired leases (where the current time exceeds the expiration timestamp) and making the resource available again, often by updating or deleting the lease row [13][14][11]. While some specific software (like the ISC Kea DHCP server) uses "lease reclamation" terminology for its internal management of DHCP address leases stored in PostgreSQL, this is an application-specific pattern rather than a native database feature [13][15]. Libraries like pg_lease or similar patterns in distributed systems provide the logic to acquire, verify, and expire these leases transactionally [10][12][16]. Top results: [5][2][7][8][9][13][14][10][11]
Citations:
- 1: https://www.postgresql.org/message-id/1197595141.15521.10.camel%40ebony.site
- 2: https://www.postgresql.org/docs/current/sql-select.html
- 3: https://www.datacamp.com/doc/postgresql/limit
- 4: https://www.postgresql.org/message-id/5443.1197867291%40sss.pgh.pa.us
- 5: https://www.postgresql.org/docs/current/queries-limit.html
- 6: https://www.enterprisedb.com/postgres-tutorials/how-use-limit-and-offset-postgresql
- 7: https://www.postgresql.org/docs/current/datatype-datetime.html
- 8: https://www.postgresql.org/docs/current/functions-datetime.html
- 9: https://stackoverflow.com/questions/75443569/how-to-add-or-subtract-milliseconds-with-calculated-value-or-from-a-field
- 10: https://docs.rs/pg_lease/latest/pg_lease/
- 11: https://www.centron.de/en/tutorial/lease-management-with-postgresql-for-multi-instance-applications/
- 12: https://pkg.go.dev/github.com/denpeshkov/warden
- 13: https://kea.readthedocs.io/en/kea-1.7.3/arm/lease-expiration.html
- 14: https://ftp.iij.ad.jp/pub/network/isc/kea/2.4.0/doc/html/arm/lease-expiration.html
- 15: https://ftp.ripe.net/mirrors/sites/ftp.isc.org/isc/kea/1.7.1/doc/html/arm/lease-expiration.html
- 16: https://docs.rs/pg_lease/latest/pg_lease/fn.verify_lease_held.html
Reject non-positive leaseMs before claiming rows. A zero or negative lease expires immediately, so the same events can be reclaimed again while a drain is still in flight.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/event-outbox.ts` around lines 140 - 159, Validate options.leaseMs
at the start of claimBatch and reject zero or negative values before executing
the UPDATE query. Preserve the existing claiming behavior for positive lease
durations.
| /** {@link WebhookEndpointStore.create}'s return shape: the stored fields plus the plaintext `secret`, returned exactly once (spec §5: "returned once") — never re-derivable after this call, since only the ciphertext persists. */ | ||
| export interface CreatedWebhookEndpoint extends StoredWebhookEndpoint { | ||
| secret: string | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the one-time-secret contract.
Line 75 says the secret is “never re-derivable,” but getSecret() at Lines 103-109 intentionally decrypts it for authorized callers. State that it is returned once by create and omitted from normal endpoint records instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/webhook-endpoints.ts` around lines 75 - 78, Update the
documentation for CreatedWebhookEndpoint to state that create returns the
plaintext secret once, while normal endpoint records omit the secret. Remove the
inaccurate claim that the secret is never re-derivable, and keep the interface
shape unchanged.
1 participant
Summary
Wave 1 of the module-substrate implementation (HT-68; spec:
specs/modules/substrate-v1.mdon #76). All substrate DDL lands here — waves 2/3 (#TBD) add no migrations.assistants→ threads actor model (author_kind+ identity FKs + draft lifecycle + audit columns, backfilled before constraints) →webhook_endpoints(encrypted secrets via the token-crypto envelope,moduleattribution) →event_outbox(claim/drain lease mirroring the queue_jobs idiom).author_kind;insertThread's?? 'pending'coercion is draft-aware;appendDraft(no reopen, noupdated_atbump — stronger than notes, regression-pinned both ways),listAwaitingDrafts(soft-deleted excluded in SQL),resolveDraft(atomic, double-resolution impossible, envelope/message-id taken as opaque inputs — derivation is wave 3); deliverable queries carry an explicitdraft_statusguard as belt over the CHECK's braces; assistants / webhook-endpoints (failure counter, auto-disable at exactly 20, manual-disable never overridden) / event-outbox stores.Review trail
Sonnet-authored → Opus adversarial review: SHIP (0 blockers, 0 majors, 2 minors — both fixed): the CHECK truth table was walked over every
(direction, draft_status, delivery_status)combination; migration ordering verified against upgrade-from-live data. Implementation also caught a real NULL-trap bug in the spec's §2 CHECK SQL (three-valued logic letoutbound+NULL+NULLpass) — spec amended on #76 withIS NOT NULLguards, credit to the failing store test.Gates
typecheck 0 · lint 0 · test 0 — 1116 passed (baseline 1070; +46 across migrations/stores).
Stacked: #76 (spec) is the contract this implements; waves 2 (events/webhooks/admin API) and 3 (assistant auth/drafts/approval) branch off this. Per the maintainer's overnight ground rules this PR stays open for morning review — nothing merges tonight.
🤖 Generated with Claude Code
Summary by CodeRabbit