docs: cut the living docs for concision - #190
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request clarifies documentation for the Agent Inbox API, Agent authentication, passkeys, mail ingestion and delivery, Gmail reconciliation, runtime modules, database migration, and security decisions. It does not change SQL or executable behavior. ChangesContract clarifications
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@docs/modules/assistants-and-drafts.md`:
- Around line 159-163: The acting-Agent header requirements are inconsistent
between the draft-review guide and authentication spec. Update
specs/auth/agents-and-auth.md at lines 387-396 to include GET /api/v1/drafts,
POST .../approve, and POST .../discard in the per-endpoint header-required set,
preserving the existing Authorization and X-Helpthread-Agent-Id requirements
documented in docs/modules/assistants-and-drafts.md at lines 159-163; no direct
change is needed in the guide.
In `@specs/api/agent-inbox-v1.md`:
- Around line 379-388: Align the active assignee contract across specs: update
the assignee endpoint and ConversationSummary documentation in
specs/api/agent-inbox-v1.md (lines 379-388) to use assigneeAgentId: uuid | null,
matching the rollout body and summary field defined in
specs/auth/agents-and-auth.md (lines 472-478); alternatively, explicitly mark
the v1 specification as pre-rollout.
In `@specs/auth/agents-and-auth.md`:
- Around line 20-36: Replace lowercase marketplace extension-artifact references
with “Module” or “Modules” in the specified section of
specs/auth/agents-and-auth.md (lines 20-36) and the core/marketplace exception
text in specs/auth/passkeys.md (lines 20-25). Use “Modules” consistently for
these artifacts, reserving “plugin” only for the legal phrase “plugin exception”
or charter quotations.
In `@specs/auth/passkeys.md`:
- Around line 459-461: Add the text language tag to the fenced token-format
block containing htw.{keyId}.{payload-b64url}.{sig-b64url}, preserving its
content and formatting otherwise.
In `@specs/mail/sending.md`:
- Around line 66-74: Update the idempotency guarantee wording near appendThread
to describe an idempotent logical send rather than at-most-once delivery, or
explicitly qualify at-most-once as requiring provider-level deduplication. Keep
the existing atomic get-or-insert and no-key behavior unchanged.
- Around line 160-172: Require every self-reflecting transport to provide a
provider-stable identifier for its sent message before relying on self-echo
suppression. Update sendReply and the SendReplyDeps.selfEchoGuard contract so
missing EmailSendResult.providerMessageId is rejected or otherwise prevented for
these adapters, or establish an equivalent correlation available before sending;
do not silently no-op preSuppressOwnSend.
- Around line 85-100: Update assertLeaseExceedsSenderBound and the delivery flow
so the lease remains valid through the final status-update and release write,
not only EmailSender.send. Either include worst-case write latency in the
lease-bound validation or make releaseThreadLease perform an atomic
status-and-lease update that preserves the lease until completion; ensure both
retry paths continue rejecting configurations that could allow reclamation
during delivery.
In `@src/db/migrate.ts`:
- Around line 1898-1900: Update the prose comments around the migration 018
references to use “Agent” for the human who connected the account and “Module”
or “Modules” for extension artifacts, while preserving identifiers such as
connected_by_agent_id and module_installs unchanged.
- Around line 1968-1975: The migration documentation for module_install_events
incorrectly claims append-only behavior as a database-enforced contract. Update
the comment near module_install_events to describe this as an application-level
convention enforced by module-installs.ts, and acknowledge that cascading
deletion can remove events; do not imply database-level protection without
adding corresponding constraints.
- Around line 1952-1955: Update the state-transition documentation near the
state-flow comment to remove build_failed from the happy path. Document the
successful sequence as build_pending → bootstrap_pending → endpoint_verified →
active, and describe build_failed separately as a terminal branch reached from
build_pending that does not proceed to endpoint_verified.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cc976765-1cac-4513-9bc9-aa95e31e9dfb
📒 Files selected for processing (10)
docs/decisions/README.mddocs/modules/assistants-and-drafts.mddocs/modules/webhooks.mdspecs/api/agent-inbox-v1.mdspecs/auth/agents-and-auth.mdspecs/auth/passkeys.mdspecs/mail/gmail-push.mdspecs/mail/inbound-ingestion.mdspecs/mail/sending.mdsrc/db/migrate.ts
| Everything past this point is an **Agent** action. The core inbox UI does this for a | ||
| human clicking "approve" or "discard" and consumes exactly the same API, so these are | ||
| also the calls to build your own review tooling against. All three require | ||
| `Authorization: Bearer $HELPTHREAD_API_TOKEN` + `X-Helpthread-Agent-Id: <acting Agent's | ||
| uuid>` — missing either is `401`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)(assistants-and-drafts\.md|agents-and-auth\.md|agent-inbox-v1\.md)$' || true
echo
echo "== target excerpts =="
sed -n '145,170p' docs/modules/assistants-and-drafts.md 2>/dev/null || true
echo "---"
sed -n '375,402p' specs/auth/agents-and-auth.md 2>/dev/null || true
echo "---"
rg -n "draft|approve|discard|X-Helpthread-Agent-Id|Agent-Id|agents-and-auth|agents/" docs/modules/assistants-and-drafts.md specs/auth/agents-and-auth.md specs -S || trueRepository: Helpthread/helpthread
Length of output: 26163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== route/header occurrences =="
rg -n "approve|discard|draft" docs/modules/assistants-and-drafts.md specs auth 2>/dev/null || true
echo
echo "== focused spec/header context =="
for f in docs/modules/assistants-and-drafts.md specs/auth/agents-and-auth.md specs/api/agent-inbox-v1.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -C 5 "approve|discard|draft|X-Helpthread-Agent-Id|Authorization|Bearer" "$f" || true
fi
doneRepository: Helpthread/helpthread
Length of output: 37305
Align acting-Agent header requirements for draft-review API.
docs/modules/assistants-and-drafts.md documents X-Helpthread-Agent-Id for GET /api/v1/drafts, POST .../approve, and POST .../discard, but specs/auth/agents-and-auth.md §8 only lists /agents/*, /auth/me, and PUT /conversations/{id}/assignee. Make these spec sites agree: add the draft-review endpoints to the per-endpoint header-required set, or remove the header requirement from the guide and update the spec accordingly.
🧰 Tools
🪛 LanguageTool
[style] ~160-~160: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...ing "approve" or "discard" and consumes exactly the same API, so these are also the calls to bui...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
📍 Affects 2 files
docs/modules/assistants-and-drafts.md#L159-L163(this comment)specs/auth/agents-and-auth.md#L387-L396
🤖 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 `@docs/modules/assistants-and-drafts.md` around lines 159 - 163, The
acting-Agent header requirements are inconsistent between the draft-review guide
and authentication spec. Update specs/auth/agents-and-auth.md at lines 387-396
to include GET /api/v1/drafts, POST .../approve, and POST .../discard in the
per-endpoint header-required set, preserving the existing Authorization and
X-Helpthread-Agent-Id requirements documented in
docs/modules/assistants-and-drafts.md at lines 159-163; no direct change is
needed in the guide.
| ### 4f. `PUT /api/v1/conversations/{id}/assignee` — claim or release (v1.1) | ||
|
|
||
| Body: `{ assignee: 'me' | null }` — `null` means "Anyone". Anything else is | ||
| `400 validation_failed`. Returns the updated `ConversationSummary` (`200`). Does **not** | ||
| bump `updatedAt`. Missing or deleted conversation → `404 not_found`. | ||
| Body: `{ assignee: 'me' | null }` — `null` means "Anyone". Anything else is `400 | ||
| validation_failed`. Returns the updated `ConversationSummary` (`200`). Does **not** bump | ||
| `updatedAt`. Missing or deleted → `404 not_found`. | ||
|
|
||
| This is deliberately NOT identity: `'me'` is the deployment's one operator (the Bearer | ||
| token holder), stored as a flag, not a user id. It exists so the UI's "Mine" folder | ||
| works in v1; the multi-Agent increment replaces `'me'` with real Agent ids and this | ||
| endpoint's body shape is expected to change then (that is an acceptable v2 break — | ||
| dogfood-only, same reasoning as 's). | ||
| This is deliberately NOT identity: `'me'` is the deployment's one operator (the Bearer token | ||
| holder), stored as a flag, not a user id. It exists so the UI's "Mine" folder works in v1; | ||
| the multi-Agent increment replaces `'me'` with real Agent ids and this body shape is expected | ||
| to change then — an acceptable v2 break while dogfood-only. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)specs/(api/agent-inbox-v1|auth/agents-and-auth)\.md$' || true
echo "== agent-inbox v1 lines =="
if [ -f specs/api/agent-inbox-v1.md ]; then
sed -n '350,405p' specs/api/agent-inbox-v1.md | cat -n
fi
echo "== auth lines =="
if [ -f specs/auth/agents-and-auth.md ]; then
sed -n '450,490p' specs/auth/agents-and-auth.md | cat -n
fi
echo "== relevant assignee references =="
rg -n "assignee(AgentId)?|assignee:" specs -S || trueRepository: Helpthread/helpthread
Length of output: 9857
Publish one assignee wire contract.
PUT /api/v1/conversations/{id}/assignee still documents { assignee: 'me' | null }, while agents-and-auth.md defines the rollout as { assigneeAgentId: uuid | null } for both the body and summary field. Update the active API contract to the rollout shape, or mark the v1 API spec as pre-rollout.
📍 Affects 2 files
specs/api/agent-inbox-v1.md#L379-L388(this comment)specs/auth/agents-and-auth.md#L472-L478
🤖 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/api/agent-inbox-v1.md` around lines 379 - 388, Align the active
assignee contract across specs: update the assignee endpoint and
ConversationSummary documentation in specs/api/agent-inbox-v1.md (lines 379-388)
to use assigneeAgentId: uuid | null, matching the rollout body and summary field
defined in specs/auth/agents-and-auth.md (lines 472-478); alternatively,
explicitly mark the v1 specification as pre-rollout.
| - **Resonant IQ's own deployment, and paying customers:** want Google SSO, magic-link, | ||
| SAML/enterprise SSO. These are **licensed marketplace modules**, not part of the free | ||
| core. **Passkey login (WebAuthn) is the one exception — it is core, not a marketplace | ||
| module:** security hygiene is always free (decided 2026-07-18); when | ||
| built, it ships as a second **core** auth provider on this same seam (catalog §2.2), | ||
| never through the marketplace path below. It is not yet built in this increment (§11). | ||
| module:** security hygiene is always free (decided 2026-07-18). When built it ships as a | ||
| second **core** auth provider on this same seam (catalog §2.2), never through the | ||
| marketplace path. Not built in this increment (§11). | ||
|
|
||
| The mechanism that makes both true at once is an **auth-provider seam** (§4). The core ships | ||
| the seam and exactly one provider — `password`. A marketplace module is a package that | ||
| registers an additional provider against that seam. **This seam is a concrete instance of | ||
| the AGPL-3.0 §7 module-marketplace boundary** the charter is built around and that counsel | ||
| is defining (critical path). Building | ||
| the seam and the free `password` provider in core is AGPL-clean and can proceed now; it also | ||
| gives counsel a *concrete* boundary to write the exception text against. Shipping any | ||
| premium provider module waits on that text being counsel-final (charter: the §7 exception | ||
| must be final before the first external module merges). **Entitlement/licensing enforcement** | ||
| (how a deployment proves it bought a module) is separate marketplace infrastructure and is | ||
| out of scope here — the seam simply loads whatever providers are registered. | ||
|
|
||
| The free core must stand entirely on its own: password login is a *real* login, not a | ||
| crippled demo. Premium providers are pure additions that attach with **zero core-schema | ||
| change** (§3.2 is why). | ||
|
|
||
| ## 2. Vocabulary (charter §, fixed) | ||
|
|
||
| **Agent** = a human member of the support staff who operates the inbox. **Assistant** = an AI | ||
| actor. Never conflated. The identity records this spec introduces are **Agents**. We never | ||
| call them "users" loosely in schema, API, or UI copy; the API resource is `/agents`, the | ||
| records are Agents. Our copy says "Agents" or "Team" consistently. | ||
| the seam and exactly one provider — `password`. A marketplace module registers an additional | ||
| provider against it. **This seam is a concrete instance of the AGPL-3.0 §7 | ||
| module-marketplace boundary** the charter is built around and that counsel is defining | ||
| (critical path). Building the seam and the free `password` provider in core is AGPL-clean | ||
| and can proceed now; it also gives counsel a *concrete* boundary to write the exception text | ||
| against. Shipping any premium provider module waits on that text being counsel-final (the | ||
| charter requires the §7 exception be final before the first external module merges). | ||
| **Entitlement/licensing enforcement** is separate marketplace infrastructure and out of | ||
| scope — the seam simply loads whatever providers are registered. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Module terminology for extension artifacts.
specs/auth/agents-and-auth.md#L20-L36: replace lowercase marketplacemodulereferences withModuleorModules.specs/auth/passkeys.md#L20-L25: apply the same vocabulary to the core/marketplace exception text.
As per coding guidelines, call extension artifacts Modules, never plugins, except within the legal phrase plugin exception or charter quotations.
📍 Affects 2 files
specs/auth/agents-and-auth.md#L20-L36(this comment)specs/auth/passkeys.md#L20-L25
🤖 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/auth/agents-and-auth.md` around lines 20 - 36, Replace lowercase
marketplace extension-artifact references with “Module” or “Modules” in the
specified section of specs/auth/agents-and-auth.md (lines 20-36) and the
core/marketplace exception text in specs/auth/passkeys.md (lines 20-25). Use
“Modules” consistently for these artifacts, reserving “plugin” only for the
legal phrase “plugin exception” or charter quotations.
Source: Coding guidelines
| ``` | ||
| htw.{keyId}.{payload-b64url}.{sig-b64url} | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the token-format fence.
The fence at Line 459 has no language tag. markdownlint reports MD040. Use text because the block is a token format, not executable code.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 459-459: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/auth/passkeys.md` around lines 459 - 461, Add the text language tag to
the fenced token-format block containing
htw.{keyId}.{payload-b64url}.{sig-b64url}, preserving its content and formatting
otherwise.
Source: Linters/SAST tools
| **Caller-supplied idempotency key, scoped per conversation.** A caller needing at-most-once | ||
| delivery (`agent-inbox-v1.md` §4a) supplies an `idempotencyKey`. | ||
| `src/store/conversations.ts`'s `appendThread` resolves it as an atomic **get-or-insert**: | ||
| `INSERT ... ON CONFLICT (conversation_id, idempotency_key) WHERE idempotency_key IS NOT NULL | ||
| DO NOTHING RETURNING *`, falling back to a `SELECT` of the pre-existing row on conflict — | ||
| inside the same transaction holding the conversation row's `FOR UPDATE` lock, so two callers | ||
| racing with the identical key on the identical conversation are serialized rather than | ||
| double-inserting. Omitting the key is still legal: a fresh send every call, no dedup — a | ||
| deliberate, permanently-tested contract for callers that don't need it. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the idempotency guarantee wording.
Line 66 describes the idempotency key as providing “at-most-once delivery.” Lines 102-112 state that a provider can accept a message while the sent update fails, which can cause a later resend. The key provides an idempotent logical send, not at-most-once delivery. Replace the phrase or qualify it with provider-level deduplication.
🤖 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/mail/sending.md` around lines 66 - 74, Update the idempotency guarantee
wording near appendThread to describe an idempotent logical send rather than
at-most-once delivery, or explicitly qualify at-most-once as requiring
provider-level deduplication. Keep the existing atomic get-or-insert and no-key
behavior unchanged.
| **A lease keeps at most one attempt in flight per row.** Before either a keyed retry or the | ||
| delivery worker sends a `pending`/`failed` row, it must claim the row's delivery lease | ||
| (`claimThreadForDelivery`: an atomic `UPDATE ... WHERE claimed_until IS NULL OR claimed_until | ||
| < now`). A failed claim means someone else holds it; the caller does not send and reports | ||
| back rather than retrying the claim. A successful attempt releases the lease as it marks | ||
| `sent`/`failed`. | ||
|
|
||
| This holds **only if the lease strictly outlives the send it protects.** `DEFAULT_LEASE_MS` | ||
| (`src/mail/send.ts`) MUST strictly exceed the worst-case duration of the configured | ||
| `EmailSender`'s `send` call; a send that outlives its own lease can be re-claimed and retried | ||
| while the original call is still in flight — a genuine concurrent double-send. Enforced | ||
| mechanically, not by convention: the `EmailSender` contract requires each implementation to | ||
| declare the bound it itself enforces (`maxSendMs`, `src/providers/email-sender.ts`), and both | ||
| retry paths assert `maxSendMs < leaseMs` before claiming a row | ||
| (`assertLeaseExceedsSenderBound`, `src/mail/send.ts`) — a violating combination throws up | ||
| front, claiming and sending nothing. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'claimThreadForDelivery|release.*Delivery|mark.*(sent|failed)|assertLeaseExceedsSenderBound|maxSendMs|leaseMs' src || trueRepository: Helpthread/helpthread
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate send.ts =="
fd -a 'send\.ts$' src | sed 's#^\./##'
echo "== send.ts outline =="
ast-grep outline src/mail/send.ts --view compact || true
echo "== relevant send.ts ranges =="
sed -n '1,220p' src/mail/send.ts
echo "== lines 220-380 =="
sed -n '220,380p' src/mail/send.ts
echo "== relevant conversation store methods =="
rg -n -C 5 'claimThreadForDelivery|setThreadDeliveryStatus|release.*Delivery|maxSendMs|assertLeaseExceedsSenderBound' src/store | head -n 220
echo "== tests around mark-sent release =="
sed -n '880,970p' src/mail/send.test.ts
sed -n '280,365p' src/mail/send.test.ts
sed -n '1140,1190p' src/api/index.test.tsRepository: Helpthread/helpthread
Length of output: 49317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== send.ts lines 380-560 =="
sed -n '380,560p' src/mail/send.ts
echo "== send.ts lines 560-680 =="
sed -n '560,680p' src/mail/send.ts
echo "== releaseThreadLease implementation =="
rg -n -C 12 'releaseThreadLease|function releaseThreadLease|releaseThreadLease\(' src/store/conversations.ts src/store/conversations.test.ts
echo "== claimThreadForDelivery implementation =="
rg -n -C 16 'claimThreadForDelivery|function claimThreadForDelivery|claimThreadForDelivery\(' src/store/conversations.ts src/store/conversations.test.ts | sed -n '1,220p'Repository: Helpthread/helpthread
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== attemptDeliveryOfClaimedThread implementation =="
sed -n '680,880p' src/mail/send.ts
echo "== all releaseThreadLease and claimThreadForDelivery call sites =="
rg -n 'releaseThreadLease\(|claimThreadForDelivery\(' src/mail/src/store/conversations.test.ts || true
rg -n 'releaseThreadLease\(|claimThreadForDelivery\(' src/mail src/store/conversations.test.ts | head -n 120Repository: Helpthread/helpthread
Length of output: 6560
Cover write latency in the delivery lease guard.
assertLeaseExceedsSenderBound only checks maxSendMs < leaseMs. attemptDeliveryOfClaimedThread can hold the delivery lease until the final releaseThreadLease(... 'sent') write returns, while claimThreadForDelivery can reclaim a claimed row as soon as claimed_until < now() passes. Add the status-update/release latency to the guard, or keep the delivery lease held during an atomic status+lease update.
🤖 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/mail/sending.md` around lines 85 - 100, Update
assertLeaseExceedsSenderBound and the delivery flow so the lease remains valid
through the final status-update and release write, not only EmailSender.send.
Either include worst-case write latency in the lease-bound validation or make
releaseThreadLease perform an atomic status-and-lease update that preserves the
lease until completion; ensure both retry paths continue rejecting
configurations that could allow reclamation during delivery.
| **A self-reflecting transport requires ALSO suppressing the sent message's own echo.** | ||
| Placing the reply token in `References` unconditionally has a consequence: Gmail (confirmed | ||
| live) delivers the SENT message back into the mailbox it was sent from, and that self-echo | ||
| now carries a verifiable token — one `inbound-ingestion.md` §5's `Message-ID`-only loop guard | ||
| cannot recognize, since Gmail rewrites the echo's `Message-ID` too. Left alone, the echo | ||
| would `append` into its own conversation as a phantom inbound message. `sendReply` closes | ||
| this immediately after a successful send: if `EmailSender.send` returned an | ||
| `EmailSendResult.providerMessageId` — the SAME id the transport later reports during | ||
| reconcile — it resolves `SendReplyInput.from` to its `MailboxRecord` and pre-seeds | ||
| `(mailboxId, providerMessageId)` as an already-`suppressed` row in the inbound delivery | ||
| ledger (`InboundDeliveryStore.preSuppressOwnSend`; `inbound-ingestion.md` §5 has the | ||
| mechanism and its one known residual race). OPTIONAL (`SendReplyDeps.selfEchoGuard`) and a | ||
| no-op wherever absent or wherever the sender reports no `providerMessageId`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require providerMessageId for self-reflecting transports.
Lines 160-172 make selfEchoGuard optional and skip pre-seeding when the sender returns no providerMessageId. In specs/mail/gmail-push.md, Lines 127-142 intentionally do not skip a self-addressed message with both SENT and INBOX. In specs/mail/inbound-ingestion.md, Lines 219-241 explain that Gmail's rewritten Message-ID prevents the downstream loop guard from recognizing that echo. This path appends the Agent's own reply as an inbound phantom. Require providerMessageId for every self-reflecting adapter, or define another provider-stable correlation before sending.
🤖 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/mail/sending.md` around lines 160 - 172, Require every self-reflecting
transport to provide a provider-stable identifier for its sent message before
relying on self-echo suppression. Update sendReply and the
SendReplyDeps.selfEchoGuard contract so missing
EmailSendResult.providerMessageId is rejected or otherwise prevented for these
adapters, or establish an equivalent correlation available before sending; do
not silently no-op preSuppressOwnSend.
| * RESTRICT`** — step-up gating is meaningless if nobody can later answer | ||
| * "which agent connected this." No product path deletes an `agents` row | ||
| * today (agents are disabled, not deleted — migration 018). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply the required glossary terms in prose.
Use Agent for the human who connected the account. Use Module or Modules for extension artifacts. Keep identifiers such as connected_by_agent_id and module_installs unchanged.
As per coding guidelines, human support staff are Agents and extension artifacts are Modules.
Also applies to: 1904-1904, 1955-1955, 1959-1960
🤖 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 1898 - 1900, Update the prose comments around
the migration 018 references to use “Agent” for the human who connected the
account and “Module” or “Modules” for extension artifacts, while preserving
identifiers such as connected_by_agent_id and module_installs unchanged.
Source: Coding guidelines
| * - **`state`**: `planned` → `credentials_issued` → `project_created` → | ||
| * `artifact_uploaded` → `deployment_created` → `build_pending` → | ||
| * (`build_failed` | `bootstrap_pending`) → `endpoint_verified` → | ||
| * `active` is the happy path. No build step ever runs on module code, so |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: build_failed is not a direct happy-path predecessor of endpoint_verified.
rg -n -C 12 'build_failed|bootstrap_pending|endpoint_verified|transition\s*\(' \
src/store/module-installs.tsRepository: Helpthread/helpthread
Length of output: 4895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size =="
wc -l src/db/migrate.ts src/store/module-installs.ts
echo "== migrate doc around snippet =="
sed -n '1935,1970p' src/db/migrate.ts
echo "== orchestration / transition calls to build_failed, bootstrap_pending, endpoint_verified, active =="
rg -n -C 6 "build_failed|bootstrap_pending|endpoint_verified|build_pending|project_created|transition\\s*\\(" --glob '*.{ts,tsx,js,jsx}' . | head -n 260
echo "== state occurrences =="
rg -n "build_failed|bootstrap_pending|endpoint_verified|active" --glob '*.{ts,tsx,js,jsx}' .Repository: Helpthread/helpthread
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== installer build polling and bootstrap sections =="
sed -n '1190,1385p' src/modules/install/installer.ts
echo "== installer state handling summary =="
sed -n '1535,1648p' src/modules/install/installer.ts
echo "== failInstall definition =="
sed -n '628,818p' src/modules/install/installer.ts
echo "== relevant module install tests around build_failed and endpoint_verified =="
sed -n '535,675p' src/modules/install/installer.test.ts
sed -n '780,990p' src/modules/install/installer.test.ts
sed -n '1040,1125p' src/modules/install/installer.test.tsRepository: Helpthread/helpthread
Length of output: 42497
Move build_failed off the happy path.
The happy path only goes through bootstrap_pending; build_failed is reached from build_pending as a permanent failure and does not continue to endpoint_verified. Document the path as build_pending → bootstrap_pending → endpoint_verified → active, and describe build_failed separately as the terminal build-failure branch.
🤖 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 1952 - 1955, Update the state-transition
documentation near the state-flow comment to remove build_failed from the happy
path. Document the successful sequence as build_pending → bootstrap_pending →
endpoint_verified → active, and describe build_failed separately as a terminal
branch reached from build_pending that does not proceed to endpoint_verified.
| * ## `module_install_events` — append-only, never mutated | ||
| * | ||
| * One row per transition, `from_state` NULLABLE (the creation event has no | ||
| * prior state) and `to_state` NOT NULL. This migration does not — and | ||
| * cannot — forbid an `UPDATE`/`DELETE` at the SQL level (Postgres has no | ||
| * "insert-only table" primitive short of revoking those privileges from | ||
| * the app's own role, which would also break every other table's ordinary | ||
| * UPDATE paths if the app connects as one role for everything); the | ||
| * append-only contract is enforced by `src/store/module-installs.ts` simply | ||
| * never exposing an update/delete method for this table, matching how | ||
| * `_migrations` (migrate.ts, no update path either) already relies on | ||
| * "nothing in this codebase writes it any other way" rather than a | ||
| * database-level write-once constraint. `actor_agent_id uuid REFERENCES | ||
| * agents(id) ON DELETE SET NULL` — unlike `vercel_connections | ||
| * .connected_by_agent_id`, an audit-log attribution is allowed to go | ||
| * anonymous ("actor unknown, agent since removed") without invalidating the | ||
| * row it is attached to; the event itself (`from_state`, `to_state`, `at`, | ||
| * `detail`) remains meaningful with no actor at all, so `SET NULL` (not | ||
| * `RESTRICT`) is correct here. `detail jsonb NOT NULL DEFAULT '{}'::jsonb` | ||
| * carries whatever the transition needs to record (a Vercel deployment id, | ||
| * an error class, a challenge nonce) without a schema change per new field | ||
| * — mirroring `queue_jobs`' own use of a JSON payload column for | ||
| * heterogeneous per-row detail. | ||
| * One row per transition: `from_state` NULLABLE (the creation event has no | ||
| * prior state), `to_state` NOT NULL. Postgres has no insert-only-table | ||
| * primitive short of revoking UPDATE/DELETE from the app's role, which | ||
| * would break every other table, so the append-only contract is enforced by | ||
| * `src/store/module-installs.ts` never exposing an update or delete method | ||
| * — the same way `_migrations` relies on nothing writing it another way. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not claim database-level append-only behavior.
module_install_events.install_id uses ON DELETE CASCADE at Lines [2050]-[2053], so deleting a module_installs row deletes its audit events. The shown migration adds no trigger or table-specific REVOKE. The store API protects only callers that use the store, while the decision record says the application role owns the tables. Either enforce the invariant at the database boundary and remove the cascade, or describe append-only behavior as an application-level convention.
🤖 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 1968 - 1975, The migration documentation for
module_install_events incorrectly claims append-only behavior as a
database-enforced contract. Update the comment near module_install_events to
describe this as an application-level convention enforced by module-installs.ts,
and acknowledge that cascading deletion can remove events; do not imply
database-level protection without adding corresponding constraints.
CodeRabbit adjudication — 3 findings: 2 real, 1 false positiveBoth Major findings are real and verified against shipped code. Both are pre-existing drift, not introduced by this PR — and both are deliberately not fixed here, because correcting a published API wire contract is a normative change and this is a concision pass. Flagged for the maintainer instead. 1. 🟠 Major — assignee wire contract. REAL. Not fixed here.Confirmed against
Not a regression from this PR — the text was already wrong and was carried through verbatim. But this PR did rewrite §4f, so it is fair to raise. 2. 🟠 Major — acting-Agent header on draft-review endpoints. REAL. Not fixed here.Confirmed against
3. 🟡 Minor —
|
|
@coderabbitai review Re-review requested: the previous review landed on |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/mail/gmail-oauth.ts`:
- Around line 77-84: Update the getAccessToken refresh flow to coordinate
concurrent refreshes for the same mailbox across workers, using a per-mailbox
single-flight mechanism or conditional/last-winner persistence around rotated
refresh_token updates. Ensure a caller observing invalid_grant from a stale
concurrent refresh does not classify the mailbox as needs_reconnect when another
refresh has already succeeded, while preserving normal reconnect handling for
genuine invalid refresh tokens.
In `@src/mail/gmail-reconcile.ts`:
- Around line 174-186: Align the cursor contract with the behavior documented
around the Gmail reconciliation handler: update the referenced gmail-push.md §4
specification and related tests to recognize dead-letter as a durable terminal
outcome alongside stored and suppressed. Preserve the handler’s advancement to
newHistoryId and ensure all assertions use the same terminal-outcome set.
- Around line 152-163: Validate the split-label ordering assumption for the
Gmail reconciler before relying on it: obtain a live regression record or
recorded production history event for a self-addressed send where SENT and INBOX
arrive in separate events, and use it to verify the merge behavior in
listAddedMessageIds and the filtering in the reconciler. Add the evidence or a
targeted regression fixture to the existing Gmail history/reconcile tests.
In `@src/modules/install/installer.ts`:
- Around line 92-101: Update the documentation paragraph near
ensureAssistantMatchesCredentials to state that ciphertext is stored in
module_install_credential_escrow, while the credentials_issued event detail
contains only credentialEscrowId; preserve the existing recovery-flow
explanation and do not imply ciphertext is stored in the append-only event row.
In `@src/store/conversations.ts`:
- Around line 4-5: Update the conversation documentation prose to capitalize the
role names as “Agent” and “Assistant” (using plural forms where appropriate) for
human support staff and AI actors, including the related prose around the
additional referenced section. Leave lowercase author_kind schema literals
unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5121d14f-2930-4b3f-98ad-9612ea26b7d4
📒 Files selected for processing (10)
src/composition/health.tssrc/mail/gmail-connect.tssrc/mail/gmail-oauth.tssrc/mail/gmail-reconcile.tssrc/mail/send.tssrc/modules/install/installer.tssrc/providers/adapters/postgres-queue/index.tssrc/store/conversations.tssrc/store/inbound-deliveries.tssrc/store/module-installs.ts
| * ## No cross-call refresh locking | ||
| * | ||
| * If two `getAccessToken` calls for the SAME mailbox race while its cached | ||
| * token is stale, both may independently POST a refresh. This is wasted | ||
| * work, not a correctness bug: Google does not invalidate the loser's | ||
| * freshly-issued access token, both calls still return a valid token, and | ||
| * the store's last write simply wins for what gets cached next time. Adding | ||
| * a lock/single-flight guard would be a reasonable follow-up if refresh | ||
| * volume ever makes the duplicate calls matter, but is speculative | ||
| * complexity this ticket does not add — see the HT-38 implementation report. | ||
| * token is stale, both may independently POST a refresh. Wasted work, not a | ||
| * correctness bug: Google does not invalidate the loser's freshly-issued | ||
| * token, both calls return a valid one, and the store's last write wins for | ||
| * what gets cached next. A single-flight guard would be a reasonable | ||
| * follow-up if refresh volume ever makes the duplicates matter. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)src/mail/gmail-oauth\.ts$|gmail-oauth|oauth|mail' || true
echo "== outline =="
ast-grep outline src/mail/gmail-oauth.ts --view expanded || true
echo "== relevant source =="
cat -n src/mail/gmail-oauth.ts | sed -n '1,180p'
echo "== search refresh/token/reconnect mentions =="
rg -n "getAccessToken|refresh|invalid_grant|needs_reconnect|token|same|concurrent|race|rotation" src/mail/gmail-oauth.ts src || trueRepository: Helpthread/helpthread
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target lines =="
cat -n src/mail/gmail-oauth.ts | sed -n '1,150p'
echo "== token store usages in oauth/disconnect/connect =="
rg -n "MailboxTokenStore|getTokens|upsertTokens|deleteTokens|refresh|refreshToken|needs_reconnect|getAccessToken" src/mail/gmail-oauth.ts src/mail/gmail-connect.ts src/mail/gmail-disconnect.ts src/mail/gmail-oauth.test.ts || true
echo "== focused oauth tests =="
cat -n src/mail/gmail-oauth.test.ts | sed -n '1,240p' || true
echo "== focused connect/disconnect tests around races =="
cat -n src/mail/gmail-connect.test.ts | sed -n '580,730p' || true
cat -n src/mail/gmail-disconnect.test.ts | sed -n '1,260p' || trueRepository: Helpthread/helpthread
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gmail-oauth implementation refresh and store calls =="
cat -n src/mail/gmail-oauth.ts | sed -n '180,370p'
echo "== mailbox-tokens relevant implementation =="
cat -n src/store/mailbox-tokens.ts | sed -n '1,260p'
echo "== mailbox store relevant methods =="
rg -n "markNeedsReconnect|refreshToken|token" src/store/mailboxes.ts src/store/mailbox-tokens.ts src/db/migrate.ts | head -n 220Repository: Helpthread/helpthread
Length of output: 37396
🌐 Web query:
Google OAuth refresh token rotation revoke_token invalid_grant if old token used in request RFC6749
💡 Result:
When using Google's OAuth 2.0 implementation, attempting to use an old or previously used refresh token will result in an invalid_grant error [1][2][3]. While RFC 6749 defines invalid_grant as a general error for invalid, expired, or revoked refresh tokens [4][5], Google’s specific behavior is designed to maintain security through token invalidation [1][2]. Key aspects of this behavior include: 1. Invalidation Upon Use: If Google rotates a refresh token and issues a new one, the previous token is effectively invalidated [2]. Attempting to reuse an old refresh token—or using one that has been superseded by a newer issued token—will cause the token endpoint to reject the request with an invalid_grant error [2][3]. 2. OAuth 2.0 Standards: Per RFC 6749, the invalid_grant error code is the standard response when the provided refresh token is invalid for any reason, including revocation, expiration, or being bound to a different client [4][5]. 3. Additional Causes for invalid_grant: Beyond reuse, Google will return invalid_grant for several other reasons [1][3]: - User Revocation: The user has explicitly revoked the application's access via their Google Account settings [1][2]. - Token Limits: Each user can hold up to 100 refresh tokens per OAuth client ID; exceeding this limit causes the oldest token to be silently invalidated [1][2]. - Inactivity: A refresh token not used for six consecutive months is automatically invalidated [1][2]. - Password Changes/Policy Enforcement: Changes to user passwords (for specific scopes like Gmail) or the enforcement of Google Workspace/Cloud session control policies can trigger invalidation [1][3]. - Testing Mode Expiry: For apps in Testing mode with an External consent screen, refresh tokens may expire after 7 days [3]. Best practices for handling this error involve building your application to catch the invalid_grant response, discarding the invalidated token, and initiating a new authorization flow to prompt the user for re-consent [1][6][2]. Integrating with services like Cross-Account Protection can also help applications receive notifications of token revocation [6].
Citations:
- 1: https://developers.google.com/identity/protocols/oauth2
- 2: https://nango.dev/blog/google-oauth-invalid-grant-token-has-been-expired-or-revoked/
- 3: https://unified.to/blog/why_google_oauth_returns
- 4: https://oauth2.dev/registries/oauth/oauth-extensions-error-registry/invalid_grant
- 5: https://www.rfc-editor.org/rfc/rfc6749.html
- 6: https://developers.google.com/identity/protocols/oauth2/resources/best-practices
Make concurrent refreshes safe when refresh tokens rotate.
The module stores a rotated refresh_token, but concurrent calls can still post the same old value. Google invalidates the old refresh token after rotation, so the loser can receive invalid_grant and gmail-oauth.ts handles that as needs_reconnect instead of a transient failure. Add per-mailbox single-flight guard across workers, or conditional/last-winner writes that do not leave one caller reclassifying a valid concurrent refresh as reconnect.
[stability_and_availabilty]
🤖 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/mail/gmail-oauth.ts` around lines 77 - 84, Update the getAccessToken
refresh flow to coordinate concurrent refreshes for the same mailbox across
workers, using a per-mailbox single-flight mechanism or conditional/last-winner
persistence around rotated refresh_token updates. Ensure a caller observing
invalid_grant from a stale concurrent refresh does not classify the mailbox as
needs_reconnect when another refresh has already succeeded, while preserving
normal reconnect handling for genuine invalid refresh tokens.
| * **On the `SENT`+`INBOX` snapshot assumption:** the | ||
| * `SENT`-without-`INBOX` check assumes a self-addressed send's | ||
| * `messagesAdded` record carries BOTH labels in one snapshot. If Gmail ever | ||
| * instead records `SENT` at send time and applies `INBOX` via a LATER, | ||
| * separate history event, this check alone would misread the message as a | ||
| * pure self-echo and skip it — a silent, permanent drop, which invariant #1 | ||
| * forbids. `../providers/adapters/gmail/history.ts`'s `listAddedMessageIds` | ||
| * hardens against exactly that ordering by also reading `labelsAdded` | ||
| * history records for the same message id within the listed window and | ||
| * merging their added-label deltas in — see that module's doc for the | ||
| * mechanism. This has not been confirmed against a live self-addressed send | ||
| * (flagged in this ticket's report as still open); the hardening below is a | ||
| * defense against the *possible* split-record ordering, not a replacement | ||
| * for that live verification. | ||
| * instead records `SENT` at send time and applies `INBOX` via a LATER | ||
| * history event, this check alone would misread it as a pure self-echo and | ||
| * skip it — a silent permanent drop, which invariant #1 forbids. | ||
| * `../providers/adapters/gmail/history.ts`'s `listAddedMessageIds` hardens | ||
| * against that ordering by also reading `labelsAdded` records for the same | ||
| * message id within the listed window and merging their deltas in. **This | ||
| * has not been confirmed against a live self-addressed send** — the | ||
| * hardening defends against a *possible* split-record ordering; it does not | ||
| * replace that verification. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'labelsAdded|listAddedMessageIds|self-addressed|isSelfEchoMessage|SENT' src/mail src/providers --glob '*.ts'Repository: Helpthread/helpthread
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- gmail history adapter ---\n'
wc -l src/providers/adapters/gmail/history.ts
sed -n '1,260p' src/providers/adapters/gmail/history.ts
printf '\n--- concise test coverage summary for split-label/self-addressed cases ---\n'
rg -n -C 6 'a self-(addressed|echo)|SENT\+INBOX|INBOX.*SENT|SENT.*INBOX|labelsAdded|split.*label|labelsAdded|listAddedMessageIds\(.*\)' src/mail/gmail-reconcile.test.ts src/providers/adapters/gmail/history.test.ts src/providers/adapters/gmail/history.ts 2>/dev/null || trueRepository: Helpthread/helpthread
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- gmail history adapter ---'
wc -l src/providers/adapters/gmail/history.ts
sed -n '1,260p' src/providers/adapters/gmail/history.ts
printf '%s\n' ''
printf '%s\n' '--- concise test coverage summary for split-label/self-addressed cases ---'
rg -n -C 6 'a self-(addressed|echo)|SENT\+INBOX|INBOX.*SENT|SENT.*INBOX|labelsAdded|split.*label|labelsAdded|listAddedMessageIds\(.*\)' src/mail/gmail-reconcile.test.ts src/providers/adapters/gmail/history.test.ts src/providers/adapters/gmail/history.ts 2>/dev/null || trueRepository: Helpthread/helpthread
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '260,420p' src/providers/adapters/gmail/history.tsRepository: Helpthread/helpthread
Length of output: 6732
Use live split-label evidence before relying on the reconciler against this case.
history.ts and history.test.ts cover the messagesAdded/labelsAdded merge logic, and gmail-reconcile.test.ts covers a single-record self-addressed message. The remaining risk in gmail-reconcile.ts is still whether Gmail produces a self-addressed send as separate SENT and INBOX history events in production; a live regression record or recorded production event is still needed.
🤖 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/mail/gmail-reconcile.ts` around lines 152 - 163, Validate the split-label
ordering assumption for the Gmail reconciler before relying on it: obtain a live
regression record or recorded production history event for a self-addressed send
where SENT and INBOX arrive in separate events, and use it to verify the merge
behavior in listAddedMessageIds and the filtering in the reconciler. Add the
evidence or a targeted regression fixture to the existing Gmail
history/reconcile tests.
| * ## `dead-letter` advances the cursor — beyond gmail-push.md §4's prose | ||
| * | ||
| * gmail-push.md §4 says the cursor "advances only after the ingest | ||
| * pipeline confirms every message HANDED TO IT is `stored` or `suppressed`," | ||
| * without mentioning `dead-letter`. This handler treats | ||
| * `dead-letter` as ALSO cursor-advancing, because `dead-letter` (inbound- | ||
| * ingestion.md §4) is itself a TERMINAL, durably-recorded ledger outcome — | ||
| * "a message that exhausts its retry budget lands in dead-letter for | ||
| * manual review — visible and recoverable, never silently dropped." The | ||
| * never-drop invariant is about the message being durably recorded | ||
| * SOMEWHERE reachable, not about it reaching `stored` specifically; a | ||
| * dead-lettered message already satisfies that. Treating `dead-letter` as | ||
| * NON-advancing instead would wedge the cursor on that one poison message | ||
| * forever (every future reconcile run re-lists the same batch, re-fetches | ||
| * the same message, gets the same permanent `dead-letter` outcome again, | ||
| * and never advances past it) — which would also block every OTHER, | ||
| * healthy message behind it in history order from ever being reached by a | ||
| * FRESH batch. Flagged here explicitly for review, per this ticket's brief. | ||
| * §4 says the cursor advances only once every message handed to the | ||
| * pipeline is `stored` or `suppressed`, without mentioning `dead-letter`. | ||
| * This handler treats `dead-letter` as ALSO cursor-advancing, because it is | ||
| * itself a terminal, durably-recorded ledger outcome (inbound-ingestion.md | ||
| * §4: "visible and recoverable, never silently dropped"). The never-drop | ||
| * invariant is about the message being durably recorded somewhere | ||
| * reachable, not about reaching `stored` specifically. Treating it as | ||
| * non-advancing would wedge the cursor on that one poison message forever — | ||
| * every future run re-lists the same batch, gets the same permanent | ||
| * outcome, and never advances — which would also block every healthy | ||
| * message behind it in history order. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the cursor contract with gmail-push.md §4.
This module treats dead-letter as terminal and advances to newHistoryId. The referenced specification still requires stored or suppressed. Update the specification and tests to include dead-letter as a durable terminal ledger outcome, or change the handler to block. Do not leave these definitions inconsistent.
🤖 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/mail/gmail-reconcile.ts` around lines 174 - 186, Align the cursor
contract with the behavior documented around the Gmail reconciliation handler:
update the referenced gmail-push.md §4 specification and related tests to
recognize dead-letter as a durable terminal outcome alongside stored and
suppressed. Preserve the handler’s advancement to newHistoryId and ensure all
assertions use the same terminal-outcome set.
| * A conversation has many threads; a thread is exactly ONE message (inbound | ||
| * customer mail, or outbound agent/assistant mail). This is the layer the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Agent and Assistant as role names.
The prose uses lowercase agent/assistant and assistant for role names. Use Agent for human support staff and Assistant for AI actors. Keep the lower-case author_kind literals unchanged because they are schema values.
Proposed documentation fix
- * customer mail, or outbound agent/assistant mail).
+ * customer mail, or outbound mail from an Agent or an Assistant).
...
- * works with zero edits: none author assistant rows. Only
+ * works with zero edits: none of them creates Assistant-authored rows. OnlyAs per coding guidelines, call human support staff Agents and AI actors Assistants; never conflate them in schemas, code, documentation, or prose.
Also applies to: 89-97
🤖 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/conversations.ts` around lines 4 - 5, Update the conversation
documentation prose to capitalize the role names as “Agent” and “Assistant”
(using plural forms where appropriate) for human support staff and AI actors,
including the related prose around the additional referenced section. Leave
lowercase author_kind schema literals unchanged.
Source: Coding guidelines
Re-review adjudication — 5 findings: 2 real and fixed, 3 declinedThe first review covered 1. 🟠
|
|
@coderabbitai review Two comment-only fixes pushed in |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current commit and recent commits =='
git rev-parse --short HEAD
git log --oneline -8
printf '%s\n' '== Changed files since the prior review commit =='
git diff --name-status 79dec27..HEAD
printf '%s\n' '== Incremental changes from 713005b parent =='
git diff --unified=80 713005b^ 713005b -- \
src/mail/gmail-oauth.ts \
src/modules/install/installer.ts
printf '%s\n' '== Relevant source outlines =='
ast-grep outline src/mail/gmail-oauth.ts --items all --view expanded
ast-grep outline src/modules/install/installer.ts --items all --view expanded
ast-grep outline src/store/module-installs.ts --items all --view expanded
printf '%s\n' '== Credential escrow definitions and event writes =='
rg -n -C 5 \
'module_install_credential_escrow|credentialEscrowId|credentials_issued|transition\s*\(' \
src
printf '%s\n' '== OAuth refresh and reconnect handling =='
rg -n -C 8 \
'refresh|invalid_grant|needs_reconnect|single-flight|concurrent' \
src/mail/gmail-oauth.tsLength of output: 112263 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current related comments with line numbers =='
nl -ba src/modules/install/installer.ts | sed -n '245,265p;840,860p'
nl -ba src/store/module-installs.ts | sed -n '258,272p;586,612p'
printf '%s\n' '== Changes to those comment areas since the PR base =='
git diff --unified=20 79dec27..HEAD -- src/modules/install/installer.ts | \
rg -n -C 12 'encryptionKey|persist an ENCRYPTED envelope|audit event|credential|escrow' || true
printf '%s\n' '== Origin of the current contradictory lines =='
git blame -L 250,258 -L 845,854 -- src/modules/install/installer.ts
printf '%s\n' '== Check whether the current head modifies executable lines in the two fixed files =='
git diff --word-diff=porcelain 713005b^..713005b -- \
src/mail/gmail-oauth.ts src/modules/install/installer.ts | \
awk '
/^\+\+\+|^---|^@@/ { next }
/^[+-]/ {
line=$0
sub(/^[+-]/, "", line)
if (line !~ /^[[:space:]]*\*|^[[:space:]]*\/\*|^[[:space:]]*\*\/|^[[:space:]]*\/\//)
print
}
'Length of output: 1435 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current related comments with line numbers =='
awk 'NR >= 245 && NR <= 265 { printf "%6d %s\n", NR, $0 } NR >= 840 && NR <= 860 { printf "%6d %s\n", NR, $0 }' \
src/modules/install/installer.ts
awk 'NR >= 258 && NR <= 272 { printf "%6d %s\n", NR, $0 } NR >= 586 && NR <= 612 { printf "%6d %s\n", NR, $0 }' \
src/store/module-installs.ts
printf '%s\n' '== Changes to those comment areas since the prior review =='
git diff --unified=20 79dec27..HEAD -- src/modules/install/installer.ts | \
rg -n -C 12 'encryptionKey|persist an ENCRYPTED envelope|audit event|credential|escrow' || true
printf '%s\n' '== Origin of the current contradictory lines =='
git blame -L 250,258 -L 845,854 -- src/modules/install/installer.ts
printf '%s\n' '== Non-comment changed lines in the two fix files =='
git diff --word-diff=porcelain 713005b^..713005b -- \
src/mail/gmail-oauth.ts src/modules/install/installer.ts | \
awk '
/^\+\+\+|^---|^@@/ { next }
/^[+-]/ {
line=$0
sub(/^[+-]/, "", line)
if (line !~ /^[[:space:]]*\*|^[[:space:]]*\/\*|^[[:space:]]*\*\/|^[[:space:]]*\/\//)
print
}
'Length of output: 18215
One documentation inconsistency remains:
The OAuth refresh comment now accurately describes the refresh-token rotation race. The installer module comment now accurately describes credential escrow storage.
|
1 participant
🟡 NEEDS YOUR DECISION
Gates green (typecheck 0, biome 0, 2005 tests pass). CodeRabbit: 3 findings — 2 real, 1 wrong (it read a vocabulary rule about the word "plugins" as a rule about capitalising "module"). Encodes 3 decisions you have not made, and surfaces 2 pre-existing spec defects, all listed below.
Concision pass over the living docs: 2,689 lines removed, 1,921 added — a net 768-line cut across 9 docs and 1 code comment. Reference material (tables, TS interfaces, SQL, endpoint contracts, JSON samples, the legal exception text) was left intact; only prose around it was cut.
src/db/migrate.tsmigration-030 commentWorst sections went from 86–123 lines to 62–79. Every remaining section over 60 lines is either reference material or a dense invariant list.
Decision provenance
HELPTHREAD_UI_PASSWORDand a{v, iat}session as the live state. Both are gone: the code says the password is retired and the session is{v:2, iat, sub}. I replaced that bullet with a pointer tospecs/auth/agents-and-auth.md§8 rather than leave a false claim or silently rewrite the security story.specs/plugins/substrate-v1.mdhas never existed; the file is atspecs/modules/. Also a vocabulary violation ("plugins"). Fixed only in the file I was already rewriting; ~20 more live in code comments and are left alone.docs/modules/webhooks.mddescribing the checks run while drafting it. The code sample itself is untouched.No one-way doors. No charter, licensing, pricing, or public-promise text changed. No API, schema, or behaviour changed.
What was deliberately not done
docs/history/— out of scope per the request.src/mail/gmail-reconcile.ts251 lines,src/mail/send.ts172,src/store/inbound-deliveries.ts171). Sampled several: most are dense contract documentation rather than prose bloat. Only migration 030's was clearly over-argued, so only that one was cut. Say the word if you want the rest swept.Two real defects found, deliberately not fixed here
CodeRabbit caught two genuine spec-vs-code drifts. Both pre-date this PR and both are left alone, because correcting a published API contract is a normative change and this is a concision pass. Verified against source, adjudicated in a comment, and spun out as a follow-up:
specs/api/agent-inbox-v1.mddocuments an assignee body the server now rejects. Code ships{ assigneeAgentId: uuid | null }and 400s the old{ assignee: 'me' | null }; the spec still shows the old shape in §2 and §4f. A reader building against §4f today writes a broken client.agents-and-auth.md§8's header-required roster is missing the draft-review endpoints, which do require the acting-Agent header (src/api/drafts.tsL316, L414). §8 anticipated later additions; the roster just was not updated.Verification
Every cut file was checked term-by-term against its pre-cut version: every backticked identifier, SQL block, endpoint, error code, numeric limit, and section reference still present.
git diffonsrc/confirms zero non-comment lines changed.Summary by CodeRabbit