docs(modules): module substrate v1 spec — events, webhooks, assistant actors (HT-67) - #76
Merged
Merged
Conversation
… actors (HT-67) The contract for the substrate every out-of-process module rides on (catalog §4 step 1), and the counsel raw material for the §7 plugin exception (HT-5): - Actor-model completion: names the charter-§4-vs-schema gap (threads has direction only; the promised author-kind/draft states were never implemented) and closes it — author_kind + author identity FKs + draft lifecycle, with a full replacement predicate for migration 007's delivery-status CHECK making unapproved-draft-with-delivery unrepresentable. - Assistant principals: ht_asst_<id>_<secret> tokens, hashed at rest, fixed capability set (read, draft, note — never send/approve). - Thin typed events (no bodies/subjects/addresses — consumers fetch via API) via transactional outbox + the existing queue drain pattern; soft delete fires nothing, ever. - Webhook delivery: Stripe-shape HMAC signatures, retries via QueueProvider, auto-disable with /internal/health visibility, SSRF posture. - Draft-review API with the approval path fully specified (mint for the existing thread id + §4a envelope derivation + unchanged delivery worker — sendReply cannot be reused and the spec says so), fixture- asserted equivalence to sendReply output for unedited approvals. - Explicit agent-inbox-v1 wire amendments (authorKind/draftStatus, deliveryStatus invariant, preview/threadCount exclusions, auth-model statement). Draft revised after a same-tier adversarial review against the shipped code: 13 findings (1 blocker, 5 major, 7 minor), all applied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSubstrate v1 specifies assistant-authored drafts, agent approval, typed transactional events, signed webhook delivery, assistant authentication, and related Agent Inbox contract changes. ChangesPlugin Substrate v1
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Assistant
participant Agent
participant ThreadState
participant DeliveryWorker
Assistant->>ThreadState: create awaiting-review draft
Agent->>ThreadState: approve draft
ThreadState->>ThreadState: derive envelope and set delivery pending
ThreadState->>DeliveryWorker: hand off approved delivery
sequenceDiagram
participant ThreadState
participant Outbox
participant WebhookDelivery
participant WebhookEndpoint
ThreadState->>Outbox: write typed event transactionally
Outbox->>WebhookDelivery: enqueue event delivery
WebhookDelivery->>WebhookEndpoint: POST signed envelope
WebhookEndpoint-->>WebhookDelivery: return response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 12
🤖 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 `@specs/plugins/substrate-v1.md`:
- Around line 101-105: Update the fixed capability-set specification to
explicitly require row-level filtering for Assistant draft visibility:
authenticated Assistants may see only drafts they authored, while Agent and
service callers retain their separate visibility rules. Apply the same
clarification to the corresponding draft-visibility statement referenced later
in the document.
- Around line 191-193: Update the idempotency-key contract described in the
specification so reply and draft operations are namespaced symmetrically. Ensure
both operation types include their distinct operation kind in the stored key and
corresponding uniqueness and lookup behavior, preventing a reply request using a
draft-prefixed key from colliding with a draft resource.
- Around line 19-21: Update the prose in the draft-reply assistant description
and the additionally affected passages to use the canonical capitalized term
“Assistants” (and “Agents” where applicable). Preserve lower-case schema or
wire-contract literals such as author_kind='assistant'.
- Around line 136-140: The specification must address already-outboxed events
that could be delivered after a conversation is soft-deleted. Update the
soft-delete behavior described in the relevant section to require deletion-time
cancellation or a delivery-time check that suppresses all events, including
draft.* events; if neither is implemented, explicitly document the accepted
delivery race.
- Around line 108-114: Update the “Author identity going forward” contract to
state that every authoring endpoint resolves the acting Agent only from a
present, well-formed, known, active X-Helpthread-Agent-Id header; absent,
malformed, unknown, or non-active headers must yield NULL identity while
preserving author_kind='agent' for service-token calls. Keep the assistant-token
identity rule unchanged.
- Around line 170-176: Update the Delivery signature specification to define the
HMAC input as the exact transmitted UTF-8 request bytes, without parsing or
re-serializing the JSON envelope. Specify the timestamp’s exact Unix-second
decimal formatting and require constant-time comparison of the computed and
received hexadecimal signatures, while preserving the existing t + "." + body
structure.
- Around line 203-226: The draft approval and discard flows must resolve drafts
through a single compare-and-set transition. Update the approval/discard
transaction to lock the row or atomically update it only when draft_status is
awaiting_review, and treat failed conditions as a defined loser/retry response
without minting tokens, message IDs, handing off delivery, or firing resolution
events. Ensure only the successful transition performs the existing approval
derivations and delivery handoff.
- Around line 182-183: Expand the SSRF policy statement to reject every
non-public destination class, including loopback, unspecified, multicast,
reserved, IPv6 ULA, IPv4-mapped IPv6, private, and link-local ranges. Require
resolving and validating all A/AAAA records, then pinning the outbound
connection to the validated address to prevent DNS rebinding.
- Around line 38-50: Update the schema definition in the author and draft
lifecycle sections to include explicit CHECK predicates: enforce each
author_kind’s valid author-ID combination, require non-NULL draft_status only
for outbound assistant-authored rows, and constrain approved_by_agent_id,
draft_resolved_at, and draft_edited to the appropriate approved/discarded
resolved states. Ensure unresolved drafts cannot carry approval audit fields or
indicate editing, while resolved drafts require the corresponding audit values.
- Around line 123-134: Reconcile the closed v1 event vocabulary with the test
endpoint’s test.ping behavior: either define test.ping with its payload,
filtering rules, and any conversationId envelope exception, or explicitly
document it as out-of-band. Update the event vocabulary and related
envelope/webhook sections consistently, including the content referenced around
the additional lines.
- Around line 198-207: The draft discard endpoint must treat soft-deleted
conversations as indistinguishable from missing resources. Update POST
/api/v1/drafts/{threadId}/discard to return the established 404-shaped response
before locating or mutating the draft or emitting draft.resolved, matching the
approval behavior while preserving normal discard behavior for active
conversations.
- Around line 118-121: Revise the “Thin events” description to avoid calling
webhook payloads PII-free, explicitly acknowledging that assigneeAgentId,
assistantId, conversation/thread IDs, and tags can be sensitive or linkable.
Define an allowlisted, non-sensitive tag representation, and state webhook
privacy, retention, and access-control expectations while preserving the rule
that message bodies, subjects, and addresses are excluded.
🪄 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: 85a2c341-c2b4-4574-b42b-7ce4e53d9643
📒 Files selected for processing (1)
specs/plugins/substrate-v1.md
Comment on lines
+19
to
+21
| The first consumer is the draft-reply assistant (catalog §3.1), which needs all three | ||
| and nothing more: it hears about inbound mail (events → webhook), reads the conversation | ||
| (API), and posts a draft (assistant API) that an Agent approves in core UI. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Agents and Assistants consistently in prose.
Replace lower-case role references such as “draft-reply assistant” and “assistant calls” with Assistants. Keep lower-case schema literals like author_kind='assistant' unchanged because they are wire-contract values.
Also applies to: 108-114
🤖 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 `@specs/plugins/substrate-v1.md` around lines 19 - 21, Update the prose in the
draft-reply assistant description and the additionally affected passages to use
the canonical capitalized term “Assistants” (and “Agents” where applicable).
Preserve lower-case schema or wire-contract literals such as
author_kind='assistant'.
Source: Coding guidelines
Comment on lines
+38
to
+50
| - `author_kind text NOT NULL CHECK (author_kind IN ('customer','agent','assistant'))` — | ||
| backfilled: `inbound` → `customer`, `outbound`/`note` → `agent`. | ||
| - `author_agent_id uuid NULL REFERENCES agents(id)`, `author_assistant_id uuid NULL | ||
| REFERENCES assistants(id)`, tied by CHECK: `customer` rows carry neither; `assistant` | ||
| rows carry `author_assistant_id` and never `author_agent_id`; `agent` rows may carry | ||
| `author_agent_id` and never `author_assistant_id` (NULL stays legal — backfilled | ||
| history and service-token callers are honest about what they don't know). | ||
| - **Draft lifecycle**: `draft_status text NULL CHECK (draft_status IN | ||
| ('awaiting_review','approved','discarded'))`, NULL for everything that is not an | ||
| assistant draft, and legal only on `direction = 'outbound'`. | ||
| - **Audit**: `approved_by_agent_id uuid NULL REFERENCES agents(id)`, | ||
| `draft_resolved_at timestamptz NULL`, `draft_edited boolean NOT NULL DEFAULT false` | ||
| (did the approving Agent change the body before sending). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the actor and draft audit invariants explicit CHECK constraints.
The spec says the author IDs are “tied by CHECK,” but does not define that predicate. It also does not require drafts to have author_kind='assistant', nor constrain approved_by_agent_id, draft_resolved_at, and draft_edited to valid resolved states. Add the exact SQL predicates so invalid combinations are unrepresentable.
🤖 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 `@specs/plugins/substrate-v1.md` around lines 38 - 50, Update the schema
definition in the author and draft lifecycle sections to include explicit CHECK
predicates: enforce each author_kind’s valid author-ID combination, require
non-NULL draft_status only for outbound assistant-authored rows, and constrain
approved_by_agent_id, draft_resolved_at, and draft_edited to the appropriate
approved/discarded resolved states. Ensure unresolved drafts cannot carry
approval audit fields or indicate editing, while resolved drafts require the
corresponding audit values.
Comment on lines
+101
to
+105
| - **Fixed capability set, not a scopes system** (v1 simplicity): an assistant may read | ||
| conversations/threads, create drafts, and create notes. It may **not** send, approve, | ||
| change status/tags/assignee, touch admin surfaces, or read soft-deleted conversations | ||
| (which are indistinguishable from nonexistent on this surface too). Any wider | ||
| capability waits for a module that needs it and ships publicly (charter: zero |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Specify row-level filtering for Assistant draft visibility.
State explicitly that an authenticated Assistant can see only drafts authored by that Assistant, while Agent/service callers follow their separate visibility rules. Otherwise the shared conversation endpoint may expose another Assistant’s in-review content.
Also applies to: 254-256
🤖 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 `@specs/plugins/substrate-v1.md` around lines 101 - 105, Update the fixed
capability-set specification to explicitly require row-level filtering for
Assistant draft visibility: authenticated Assistants may see only drafts they
authored, while Agent and service callers retain their separate visibility
rules. Apply the same clarification to the corresponding draft-visibility
statement referenced later in the document.
Comment on lines
+108
to
+114
| **Author identity going forward**: from this increment on, every new thread row carries | ||
| `author_kind`, and authoring identity where the caller asserts one — assistant calls | ||
| from the token itself; Agent-authored replies, notes, and draft resolutions from the | ||
| `X-Helpthread-Agent-Id` acting-agent header (HT-54 §8), which the web client extends to | ||
| these authoring calls. A service-token caller without the header still writes | ||
| `author_kind='agent'` with NULL identity (the pre-HT-54 posture, preserved rather than | ||
| broken). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Carry the full acting-Agent resolution rule into this contract.
The shipped resolver returns no acting Agent for absent, malformed, unknown, or non-active headers. This section only specifies the absent-header case; state the same active-only/null-fallback behavior for every authoring endpoint.
🤖 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 `@specs/plugins/substrate-v1.md` around lines 108 - 114, Update the “Author
identity going forward” contract to state that every authoring endpoint resolves
the acting Agent only from a present, well-formed, known, active
X-Helpthread-Agent-Id header; absent, malformed, unknown, or non-active headers
must yield NULL identity while preserving author_kind='agent' for service-token
calls. Keep the assistant-token identity rule unchanged.
Comment on lines
+118
to
+121
| **Thin events**: an event carries identifiers and small typed facts — never message | ||
| bodies, subjects, or addresses. Consumers fetch full content through the API with their | ||
| own credentials. This keeps webhook payloads free of message content and PII by | ||
| construction and matches the own-your-data posture (charter §2). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not describe these envelopes as PII-free.
assigneeAgentId, assistantId, and conversation/thread IDs are linkable identifiers, while tags may contain user-controlled sensitive data. Qualify the claim and specify an allowlisted tag representation plus webhook privacy, retention, and access expectations.
Also applies to: 125-134
🤖 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 `@specs/plugins/substrate-v1.md` around lines 118 - 121, Revise the “Thin
events” description to avoid calling webhook payloads PII-free, explicitly
acknowledging that assigneeAgentId, assistantId, conversation/thread IDs, and
tags can be sensitive or linkable. Define an allowlisted, non-sensitive tag
representation, and state webhook privacy, retention, and access-control
expectations while preserving the rule that message bodies, subjects, and
addresses are excluded.
Comment on lines
+170
to
+176
| - **Delivery** (a queue topic; handler per delivery): HTTP POST, JSON envelope, 10s | ||
| timeout, redirects not followed. Headers: | ||
| - `X-Helpthread-Event`: type | ||
| - `X-Helpthread-Delivery`: unique delivery id (differs per attempt) | ||
| - `X-Helpthread-Signature`: `t=<unix-ts>, v1=<hex HMAC-SHA256(secret, t + "." + body)>` | ||
| — Stripe-shape; consumers reject stale `t` (recommended window 5 minutes) to kill | ||
| replays. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Define HMAC input as the exact transmitted bytes.
Specify UTF-8 raw request bytes, timestamp formatting, and comparison rules. Signing parsed or re-serialized JSON can make valid deliveries fail because whitespace, key order, or encoding changes the signed 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 `@specs/plugins/substrate-v1.md` around lines 170 - 176, Update the Delivery
signature specification to define the HMAC input as the exact transmitted UTF-8
request bytes, without parsing or re-serializing the JSON envelope. Specify the
timestamp’s exact Unix-second decimal formatting and require constant-time
comparison of the computed and received hexadecimal signatures, while preserving
the existing t + "." + body structure.
Comment on lines
+182
to
+183
| - SSRF posture: https only, no redirects, and the delivery handler refuses URLs | ||
| resolving to private/link-local ranges (impl note: resolve-then-connect pinning). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Expand the SSRF destination policy.
“Private/link-local” is incomplete: loopback, unspecified, multicast, reserved, IPv6 ULA, IPv4-mapped IPv6, and other non-public ranges must also be rejected. Define resolution of all A/AAAA answers and pin the connection to the validated address to prevent DNS rebinding.
🤖 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 `@specs/plugins/substrate-v1.md` around lines 182 - 183, Expand the SSRF policy
statement to reject every non-public destination class, including loopback,
unspecified, multicast, reserved, IPv6 ULA, IPv4-mapped IPv6, private, and
link-local ranges. Require resolving and validating all A/AAAA records, then
pinning the outbound connection to the validated address to prevent DNS
rebinding.
Comment on lines
+191
to
+193
| `Idempotency-Key` required; the engine stores it prefixed (`draft:<key>`) so the | ||
| shared `(conversation_id, idempotency_key)` uniqueness namespace (migration 004) | ||
| can never replay a reply as a draft or vice versa. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Namespace reply and draft idempotency keys symmetrically.
Prefixing only draft keys does not prevent a reply caller from submitting draft:<key>. The shared uniqueness constraint can then collide across operation types and return the wrong replayed resource. Prefix both operations or include the operation kind in the uniqueness and lookup contract.
🤖 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 `@specs/plugins/substrate-v1.md` around lines 191 - 193, Update the
idempotency-key contract described in the specification so reply and draft
operations are namespaced symmetrically. Ensure both operation types include
their distinct operation kind in the stored key and corresponding uniqueness and
lookup behavior, preventing a reply request using a draft-prefixed key from
colliding with a draft resource.
Comment on lines
+198
to
+207
| Agent-authenticated (the core draft-review UI consumes exactly this — API-first rule): | ||
| - `GET /api/v1/drafts?status=awaiting_review` — cross-conversation review queue, | ||
| newest first, keyset cursor. **Excludes drafts whose conversation is soft-deleted** | ||
| (§4d indistinguishability holds here too; such drafts are unreachable everywhere and | ||
| simply never surface — no resolution event, no discard needed). | ||
| - `POST /api/v1/drafts/{threadId}/approve` — optional `{ bodyText, bodyHtml }` override | ||
| ("approve with edits", recorded as `draft_edited`). Refused (404-shaped, per §4d) on | ||
| soft-deleted conversations; refused (409) on `spam`. | ||
| - `POST /api/v1/drafts/{threadId}/discard` — `draft_status='discarded'`, row kept for | ||
| audit. Fires `draft.resolved`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Apply soft-delete indistinguishability to discard.
Approval explicitly refuses soft-deleted conversations, but discard has no equivalent rule. A direct thread ID could otherwise reveal or mutate a deleted draft and emit draft.resolved. Specify the same 404-shaped behavior before lookup or event emission.
🤖 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 `@specs/plugins/substrate-v1.md` around lines 198 - 207, The draft discard
endpoint must treat soft-deleted conversations as indistinguishable from missing
resources. Update POST /api/v1/drafts/{threadId}/discard to return the
established 404-shaped response before locating or mutating the draft or
emitting draft.resolved, matching the approval behavior while preserving normal
discard behavior for active conversations.
Comment on lines
+203
to
+226
| - `POST /api/v1/drafts/{threadId}/approve` — optional `{ bodyText, bodyHtml }` override | ||
| ("approve with edits", recorded as `draft_edited`). Refused (404-shaped, per §4d) on | ||
| soft-deleted conversations; refused (409) on `spam`. | ||
| - `POST /api/v1/drafts/{threadId}/discard` — `draft_status='discarded'`, row kept for | ||
| audit. Fires `draft.resolved`. | ||
|
|
||
| **What approval actually does** (specified, because `sendReply` cannot be reused — it | ||
| mints and INSERTs a *new* thread row, and has no entry point for a pre-existing one): | ||
| approval is a state transition on the draft row that performs, in one transaction, the | ||
| same derivations `sendReply` performs pre-insert, then joins the existing delivery | ||
| machinery: | ||
|
|
||
| 1. Mint the reply token and Message-ID **for the draft's existing thread id** | ||
| (`specs/mail/threading.md` §2a — same mint, same key rotation). | ||
| 2. Derive the envelope exactly per agent-inbox-v1 §4a: recipient/subject from the | ||
| conversation, `In-Reply-To`/`References` from the latest inbound thread, with the | ||
| minted id as the **final** `References` entry (the HT-49 rule). | ||
| 3. Apply open-tracking pixel injection before persist iff enabled (HT-32 semantics | ||
| unchanged — absent config, byte-identical mail). | ||
| 4. Write envelope snapshot + message id + `draft_status='approved'` + | ||
| `delivery_status='pending'` + approving-Agent audit fields onto the row. | ||
| 5. Hand off to the **unchanged** delivery path — `attemptDeliveryOfClaimedThread`, the | ||
| delivery worker, its lease and retry semantics. Fires `draft.resolved` (and later | ||
| `conversation.reply_sent` when delivery confirms). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make draft resolution a single compare-and-set transition.
Two Agents, or a retried request, can currently resolve the same awaiting_review draft concurrently. Require a row lock or atomic update conditioned on draft_status='awaiting_review', and define the loser/retry response so only one token, message ID, delivery handoff, and resolution event can exist.
🤖 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 `@specs/plugins/substrate-v1.md` around lines 203 - 226, The draft approval and
discard flows must resolve drafts through a single compare-and-set transition.
Update the approval/discard transaction to lock the row or atomically update it
only when draft_status is awaiting_review, and treat failed conditions as a
defined loser/retry response without minting tokens, message IDs, handing off
delivery, or firing resolution events. Ensure only the successful transition
performs the existing approval derivations and delivery handoff.
…trofits (HT-67) TJ's standing constraint recorded in the spec: the substrate must remain correct unmodified at marketplace phase. Licensing stays distribution- side (never runtime); module attribution (`module` slug) added to webhook_endpoints mirroring assistants; capability enforcement pinned to one gate; dogfood modules built product-shaped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-67) specs/plugins/ -> specs/modules/, title and wording follow TJ's vocabulary decision (Modules everywhere; "plugin" only in the legal phrase "plugin exception"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zaridan
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
specs/modules/substrate-v1.md (3)
117-121: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject disabled Assistants during token authentication.
The table defines
status ('active','disabled'), but token verification only describes digest comparison. State that disabled Assistants—and rotated/revoked credentials—cannot authenticate or use Assistant APIs.🤖 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 `@specs/modules/substrate-v1.md` around lines 117 - 121, Update the “Token format & verification” section to require rejecting authentication when the referenced Assistant has status disabled, and when the credential has been rotated or revoked. State that these credentials cannot authenticate or access any Assistant APIs, while preserving the existing digest-comparison verification behavior for active, valid credentials.
203-206: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the consecutive-failure counter after success.
The contract increments the counter on dead-lettered failures but never states that a successful 2xx resets it. Without that reset, intermittent failures can eventually auto-disable otherwise healthy endpoints.
🤖 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 `@specs/modules/substrate-v1.md` around lines 203 - 206, Update the webhook acknowledgment and retry contract in substrate-v1.md to explicitly reset the endpoint’s consecutive-failure counter after any successful 2xx acknowledgment, while preserving the existing increment and auto-disable behavior for dead-lettered failures.
41-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake webhook module attribution enforceable.
This rule says assistants and webhook endpoints both carry a module slug, but
webhook_endpoints.moduleis later declared nullable (Line 190). Requiremodule NOT NULL, or explicitly define how unattributed webhooks participate in marketplace installation and attribution.🤖 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 `@specs/modules/substrate-v1.md` around lines 41 - 44, Update the webhook endpoint schema and related specification around webhook_endpoints.module to make module attribution enforceable by declaring the field NOT NULL and ensuring the marketplace installation and attribution flows use that required module slug consistently.
🤖 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.
Outside diff comments:
In `@specs/modules/substrate-v1.md`:
- Around line 117-121: Update the “Token format & verification” section to
require rejecting authentication when the referenced Assistant has status
disabled, and when the credential has been rotated or revoked. State that these
credentials cannot authenticate or access any Assistant APIs, while preserving
the existing digest-comparison verification behavior for active, valid
credentials.
- Around line 203-206: Update the webhook acknowledgment and retry contract in
substrate-v1.md to explicitly reset the endpoint’s consecutive-failure counter
after any successful 2xx acknowledgment, while preserving the existing increment
and auto-disable behavior for dead-lettered failures.
- Around line 41-44: Update the webhook endpoint schema and related
specification around webhook_endpoints.module to make module attribution
enforceable by declaring the field NOT NULL and ensuring the marketplace
installation and attribution flows use that required module slug consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c1cb7d8-d2c9-4403-b881-2e71746688bb
📒 Files selected for processing (1)
specs/modules/substrate-v1.md
…T-67) The literal SQL let (outbound, draft_status NULL, delivery_status NULL) pass — IN tests over NULL yield NULL, and a NULL CHECK is accepted. IS NOT NULL guards added; caught by a failing HT-68 store test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 tasks
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.
Summary
specs/plugins/substrate-v1.md— the contract for the three surfaces every out-of-process module needs (catalog §4 step 1; HT-67):QueueProvider/cron pattern. Soft delete fires nothing, ever (§4d indistinguishability extends to events).t=…,v1=HMAC-SHA256signatures with replay windows, at-least-once via the queue's retry/dead-letter, auto-disable with/internal/healthvisibility, SSRF posture (https-only, no redirects, private-range refusal).assistantsprincipals withht_asst_<id>_<secret>tokens (hashed at rest, constant-time), a fixed capability set (read, draft, note — never send/approve/admin), and the draft-review API. Approval is fully specified: token/Message-ID minted for the draft's existing thread id, §4a envelope derivation, HT-32 pixel semantics, then the unchanged delivery worker — with fixture-asserted equivalence tosendReplyoutput for unedited approvals (charter invariant HT-8: mail threading behavioral spec #5).It also names and resolves a charter-vs-schema contradiction: CHARTER §4 claims the actor model (author kinds, draft-before-send) shipped as day-one schema; migrations 001/007 show it never did. The spec closes the gap additively —
author_kind, author identity FKs, draft lifecycle, and a full replacement predicate for migration 007's delivery CHECK that makes "unapproved draft reachable by the delivery worker" unrepresentable.Wire-contract changes are explicit amendments to
agent-inbox-v1.md(ThreadViewauthorKind/draftStatus, wideneddeliveryStatusinvariant, preview/threadCountdraft exclusions, auth-model statement), not silent inheritance.Review
Lead-authored, then adversarially reviewed by a parallel same-tier agent against the shipped code (
migrate.ts,conversations.ts,send.ts,delivery-worker.ts,queue.ts, api/, both existing specs): 13 findings — 1 blocker, 5 major, 7 minor — all applied (changelog lists them; the blocker was the draft's original claim that approval could reusesendReply, which mints and inserts a fresh row and cannot send an existing one).Decision points for the maintainer (spec §9)
reply_sentfired at delivery-confirmed (rec: yes)Tickets
Docs-only change; no code paths touched.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation / Bug Fixes