chore(docs): write specs and comments for their reader, not as a record of drafting - #188
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR updates authentication and mail specifications, source documentation, comments, and test descriptions. It removes historical review-tool attribution wording and consolidates changelogs. Runtime behavior, public APIs, and test assertions remain unchanged. ChangesSpecification and documentation wording cleanup
Estimated code review effort: 2 (Simple) | ~15 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 |
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 `@specs/auth/agents-and-auth.md`:
- Around line 555-560: Update the consolidated changelog statements around the
passkey migration and agent_mailbox_access so they no longer contradict the
current specification. Either explicitly label the “no schema changes” and
“schema-only” claims as superseded historical statements, or rewrite them to
reflect webauthn_credentials and the managed grant behavior, auto-granting, and
grant endpoints defined in the relevant sections.
In `@src/api/assistants.test.ts`:
- Line 283: Rename the describe block title in the assistant auth failure
containment test to use “Assistant authentication failure containment,”
preserving the test behavior and all other wording.
In `@src/api/drafts.ts`:
- Line 228: Remove review-process narration from all seven changed locations: in
src/api/drafts.ts lines 228, 299, and 370, replace the HT-70 review-fix wording
with direct descriptions of the body-length rule, approval preflight, and
authoritative catch; in src/api/drafts.test.ts lines 516 and 656, remove “review
fix” from the suite titles; in src/api/imap-connect.test.ts line 242, remove
“review fix” from the comment; and in src/api/imap-connect.ts line 155, replace
“until review caught it” with the current authorization invariant. Retain HT or
PR references only as provenance.
In `@src/composition/health.test.ts`:
- Line 492: Remove review-process narration from the comments: in
src/composition/health.test.ts:492 remove “(review, 2026-08-02)”; in
src/db/migrate.ts:1732 retain HT-101 while removing “review of”; in
src/db/migrate.ts:1827 remove “review (2026-07-31)” and state the provenance
violation directly; in src/modules/deploy/vercel-adapter.ts:4 retain HT-119
while removing “design review”; in src/store/imap-watch-state.test.ts:153
replace the review-fix prefix with the lease-token fencing invariant; and in
src/store/mailboxes.test.ts:375 replace the review-fix prefix with the
provider-conflict invariant.
In `@src/store/conversations.ts`:
- Line 870: Remove residual review-process wording from the specified comments
and test titles: in src/store/conversations.ts lines 870-870 and 1759-1759,
replace “review fix” with the direct status-check explanation and remove it
while correcting punctuation before the em dash; in
src/providers/adapters/imap/client.test.ts lines 291-291, remove the dated
review wording and state the shared-flag requirement; in
src/store/conversations.test.ts lines 1833-1833 and 1868-1868, remove “review
fix” from the test titles; and in src/store/imap-watch-state.ts lines 56-56 and
91-91, replace review/date references with the direct lease-fencing and
quarantine invariants.
🪄 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: 5082fdbb-8adf-4e79-a140-e3c7a920ff3c
📒 Files selected for processing (41)
specs/auth/agents-and-auth.mdspecs/auth/passkeys.mdspecs/mail/gmail-push.mdspecs/mail/mailbox-connection.mdspecs/mail/sending.mdspecs/mail/spam-classification.mdspecs/modules/substrate-v1.mdspecs/ui/admin-ia.mdsrc/api/assistant-auth.tssrc/api/assistants.test.tssrc/api/drafts.test.tssrc/api/drafts.tssrc/api/imap-connect.test.tssrc/api/imap-connect.tssrc/api/index.test.tssrc/api/index.tssrc/auth/webauthn-ceremony.tssrc/composition/health.test.tssrc/composition/health.tssrc/db/migrate.tssrc/mail/approve-draft.test.tssrc/mail/approve-draft.tssrc/mail/delivery-worker.test.tssrc/mail/message-id.test.tssrc/mail/parse.test.tssrc/mail/reply-token.test.tssrc/mail/send.test.tssrc/mail/sender-resolver.test.tssrc/mail/thread.test.tssrc/modules/deploy/vercel-adapter.tssrc/providers/adapters/imap/client.test.tssrc/providers/adapters/imap/client.tssrc/providers/adapters/smtp/verify.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/imap-watch-state.test.tssrc/store/imap-watch-state.tssrc/store/mailboxes.test.tssrc/store/module-installs.tssrc/store/webauthn.test.tssrc/store/webauthn.ts
Adjudication — 5 findings, all real, all fixed
Findings 3–5 were the same defect reported three ways; following them out brought the total to 78 sites. On finding 1 — two claims in the consolidated entry no longer matched the spec:
The line drawn: ticket and PR references stay ( |
|
🐇 ✓ ✏️ Learnings added
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 26 minutes. |
Adjudication — second passCodeRabbit was rate-limited on the preceding push, so an independent adversarial pass was run in its place and its findings adjudicated below. Disclosed as a substitution: CodeRabbit had not reviewed those commits at the time.
On finding 4 — the remaining phrasings were "an earlier revision", "the first draft", "originally anticipated", "retracted here", "review round 2", "amended during implementation", "flagged in review", and "used to read". Each is restated forward with its technical lesson intact. Also surfaced: Deliberately out of scope: |
|
I noted the scope boundary for 🐇 🔍 ✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/adapters/imap/client.ts (1)
347-352: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the revision-history wording.
The phrase
which the previous revision left unguardeddescribes review history, not current behavior. Keep the comment forward-facing.Proposed wording
- // imapflow's own `close()` can throw synchronously on an already-broken connection, - // which the previous revision left unguarded (2026-07-31). + // imapflow's own `close()` can throw synchronously on an already-broken connection.This follows the PR objective to remove process narration from source comments.
🤖 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/providers/adapters/imap/client.ts` around lines 347 - 352, Update the cleanup comment near the IMAP client close logic to remove the revision-history phrase “which the previous revision left unguarded.” Keep the explanation focused on the current behavior: imapflow’s close operation may throw synchronously on a broken connection and must not let cleanup errors replace the original outcome.
🤖 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/auth/agents-and-auth.md`:
- Around line 600-604: Standardize extension-artifact terminology to “Modules”
across the specified passages: in specs/auth/agents-and-auth.md lines 600-604,
replace “plugin loader” with “Module loader” and lowercase “modules” with
“Modules”; in lines 550-556, use “catalog of Modules” and “marketplace Module”;
apply the same replacements in specs/auth/passkeys.md lines 11-12, update
“module exception” and “third-party modules” to approved “Modules” terminology
in lines 36-40, and change “paid module” to “paid Module” in line 1029.
In `@src/mail/ingest.test.ts`:
- Line 622: Update the test description in the self-echo scenario to refer to
the human support role as “Agent” with the requested capitalization, while
preserving the existing meaning and scenario details. Use “Agent” for the human
support role and reserve “Assistant” for AI actors.
In `@src/mail/parse.test.ts`:
- Line 254: Remove the “Follow-up:” process label from the comments near the
References parsing cases, including the comments at the identified locations,
while preserving the technical explanation about CFWS/comments between IDs.
---
Outside diff comments:
In `@src/providers/adapters/imap/client.ts`:
- Around line 347-352: Update the cleanup comment near the IMAP client close
logic to remove the revision-history phrase “which the previous revision left
unguarded.” Keep the explanation focused on the current behavior: imapflow’s
close operation may throw synchronously on a broken connection and must not let
cleanup errors replace the original outcome.
🪄 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: 7de6f81e-810e-45bd-803a-8935f78c0aae
📒 Files selected for processing (75)
scripts/dev-api.tsspecs/api/agent-inbox-v1.mdspecs/auth/agents-and-auth.mdspecs/auth/passkeys.mdspecs/mail/gmail-connect.mdspecs/mail/gmail-push.mdspecs/mail/inbound-ingestion.mdspecs/mail/mailbox-connection.mdspecs/mail/sending.mdspecs/mail/spam-classification.mdspecs/modules/substrate-v1.mdspecs/ui/admin-ia.mdsrc/api/assistant-auth.tssrc/api/assistants.test.tssrc/api/conversations.tssrc/api/drafts.test.tssrc/api/drafts.tssrc/api/gmail-webhook.test.tssrc/api/imap-connect.test.tssrc/api/imap-connect.tssrc/api/index.test.tssrc/api/index.tssrc/auth/webauthn-ceremony.tssrc/composition/health.test.tssrc/composition/health.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/mail/approve-draft.test.tssrc/mail/approve-draft.tssrc/mail/delivery-worker.test.tssrc/mail/delivery-worker.tssrc/mail/gmail-disconnect.test.tssrc/mail/gmail-disconnect.tssrc/mail/gmail-reconcile.test.tssrc/mail/gmail-reconcile.tssrc/mail/imap-fetch.test.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/mail/message-id.test.tssrc/mail/parse.test.tssrc/mail/reply-token.test.tssrc/mail/send.test.tssrc/mail/send.tssrc/mail/sender-resolver.test.tssrc/mail/sender-resolver.tssrc/mail/thread.test.tssrc/modules/deploy/vercel-adapter.tssrc/providers/adapters/gmail/history.test.tssrc/providers/adapters/gmail/history.tssrc/providers/adapters/imap/client.test.tssrc/providers/adapters/imap/client.tssrc/providers/adapters/imap/fetch.tssrc/providers/adapters/smtp/verify.tssrc/providers/email-sender.tssrc/providers/inbound-email.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/imap-credentials.tssrc/store/imap-watch-state.test.tssrc/store/imap-watch-state.tssrc/store/inbound-deliveries.test.tssrc/store/inbound-deliveries.tssrc/store/mailboxes.test.tssrc/store/mailboxes.tssrc/store/module-installs.tssrc/store/vercel-connection.tssrc/store/webauthn.test.tssrc/store/webauthn.tssrc/store/webhook-endpoints.tsweb/src/app/(shell)/layout.tsxweb/src/components/ConnectInboxForm.tsxweb/src/components/FolderNav.tsxweb/src/components/InboxSettingsShell.tsxweb/src/components/TopBar.tsx
| disabled, leaking no status; the §7 boundary claim is scoped to the interface plus a | ||
| provider-agnostic schema — not a plugin loader — and adds an identity-service seam so | ||
| modules never write core tables directly; invites use the `EmailSender` *transport*, | ||
| not `sendReply`, with a distinct `hti.` token prefix; email is immutable in v1; and the | ||
| bearer-token-bypasses-role-checks boundary is stated plainly. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Modules for extension artifacts.
These changed passages use inconsistent extension-artifact terminology. Replace the lower-case and alternate terms with Modules terminology.
specs/auth/agents-and-auth.md#L600-L604: replaceplugin loaderwithModule loaderandmoduleswithModules.specs/auth/agents-and-auth.md#L550-L556: replacemodule catalogandmarketplace modulewithcatalog of Modulesandmarketplace Module.specs/auth/passkeys.md#L11-L12: replacemodule catalogandmarketplace modulewithcatalog of Modulesandmarketplace Module.specs/auth/passkeys.md#L36-L40: replacemodule exceptionandthird-party moduleswith the approvedModulesterminology.specs/auth/passkeys.md#L1029-L1029: replacepaid modulewithpaid Module.
📍 Affects 2 files
specs/auth/agents-and-auth.md#L600-L604(this comment)specs/auth/agents-and-auth.md#L550-L556specs/auth/passkeys.md#L11-L12specs/auth/passkeys.md#L36-L40specs/auth/passkeys.md#L1029-L1029
🤖 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 600 - 604, Standardize
extension-artifact terminology to “Modules” across the specified passages: in
specs/auth/agents-and-auth.md lines 600-604, replace “plugin loader” with
“Module loader” and lowercase “modules” with “Modules”; in lines 550-556, use
“catalog of Modules” and “marketplace Module”; apply the same replacements in
specs/auth/passkeys.md lines 11-12, update “module exception” and “third-party
modules” to approved “Modules” terminology in lines 36-40, and change “paid
module” to “paid Module” in line 1029.
Source: Coding guidelines
| // (`InboundDeliveryStore.preSuppressOwnSend`), reconcile's later `ingest` | ||
| // call for that SAME provider id is suppressed instead — never appended. | ||
| it("HT-49 review fix: a self-echo of the agent's own sent reply — From the mailbox's OWN address, a foreign (Gmail-rewritten) Message-ID, our token as the FINAL References entry — is suppressed, not appended, once send.ts has pre-seeded its providerMessageId", async () => { | ||
| it("HT-49: a self-echo of the agent's own sent reply — From the mailbox's OWN address, a foreign (Gmail-rewritten) Message-ID, our token as the FINAL References entry — is suppressed, not appended, once send.ts has pre-seeded its providerMessageId", async () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Agent for the human support role.
Line 622 describes a human support staff member as agent. Change it to Agent.
As per coding guidelines, call human support staff Agents and AI actors Assistants.
Proposed wording
- it("HT-49: a self-echo of the agent's own sent reply — From the mailbox's OWN address, a foreign (Gmail-rewritten) Message-ID, our token as the FINAL References entry — is suppressed, not appended, once send.ts has pre-seeded its providerMessageId", async () => {
+ it("HT-49: a self-echo of the Agent's own sent reply — From the mailbox's OWN address, a foreign (Gmail-rewritten) Message-ID, our token as the FINAL References entry — is suppressed, not appended, once send.ts has pre-seeded its providerMessageId", async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("HT-49: a self-echo of the agent's own sent reply — From the mailbox's OWN address, a foreign (Gmail-rewritten) Message-ID, our token as the FINAL References entry — is suppressed, not appended, once send.ts has pre-seeded its providerMessageId", async () => { | |
| it("HT-49: a self-echo of the Agent's own sent reply — From the mailbox's OWN address, a foreign (Gmail-rewritten) Message-ID, our token as the FINAL References entry — is suppressed, not appended, once send.ts has pre-seeded its providerMessageId", async () => { |
🤖 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/ingest.test.ts` at line 622, Update the test description in the
self-echo scenario to refer to the human support role as “Agent” with the
requested capitalization, while preserving the existing meaning and scenario
details. Use “Agent” for the human support role and reserve “Assistant” for AI
actors.
Source: Coding guidelines
| }) | ||
|
|
||
| // CodeRabbit follow-up: References may contain CFWS/comments between ids; | ||
| // Follow-up: References may contain CFWS/comments between ids; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the remaining process labels.
Follow-up still describes review or process history. The PR objective requires forward-facing explanations or removal of process narration. Start each comment with its technical statement instead.
Proposed wording
- // Follow-up: References may contain CFWS/comments between ids;
+ // References may contain CFWS/comments between ids;
- // Follow-up (RFC 6854): group-form From must not drop the sender —
+ // RFC 6854: group-form From must not drop the sender —
- // Follow-up: header names come from untrusted senders. A header
+ // Header names come from untrusted senders. A headerAlso applies to: 275-275, 293-293
🤖 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/parse.test.ts` at line 254, Remove the “Follow-up:” process label
from the comments near the References parsing cases, including the comments at
the identified locations, while preserving the technical explanation about
CFWS/comments between IDs.
Specs narrated how they were made -- "an earlier draft of this section said it would", "the earlier wording overclaimed" -- and two carried draft-numbered changelogs naming the tools that reviewed them. A reader has one version of a document and no interest in what the others said. Every instance is restated forward. Where a self-correction carried a real argument, the argument stays: passkeys.md section 8 now asks "why not log-only across the board?" and answers it. Where it carried none, it is gone. The two draft-numbered changelogs become dated entries. agents-and-auth.md keeps every substantive change from its seven former entries -- mailbox access, the closed status lifecycle, the acting-Agent header rule, the advisory-lock serialization, the hardening list -- with PR references retained. passkeys.md collapses to one entry; none of it has shipped. Two corrections to the specs themselves, surfaced while rewriting: - agents-and-auth.md decision point 4 recorded "no scoping behavior or UI" while section 3.4 manages real grants with endpoints and a UI. The recorded decision is left intact; a supersession note points at 3.4. - inbound-ingestion.md section 2 had a truncated sentence. Repaired. Also: spam-classification.md used "the author's judgment" as its INFERRED marker. A spec has no author-narrator; it now reads "an inference". The INFERRED and DECIDED markings are unchanged -- who authorized a normative choice is content. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Source comments named the tooling that prompted a fix -- "CodeRabbit
(Major): claimThreadForDelivery's WHERE clause checked only the lease",
"HT-70 review fix (Codex)", "found by adversarial review" -- and several
recounted what an earlier revision got wrong instead of stating current
behaviour.
Each keeps its technical content and loses the narration. Where a comment
argued against an alternative by describing a past mistake, it now asks the
question directly:
store/mailboxes.ts "Why not tell the operator to disconnect and retry?
Because that instruction would be false:
markDisconnected only sets status ..."
providers/inbound-email.ts
"Why not return a NormalizedInboundEmail? That puts
the parse inside the provider ..."
mail/gmail-reconcile.ts
"Why not just ack? The tempting reasoning is 'the
holder will advance the cursor' ..."
Ticket and PR references are kept -- "HT-70:", "(PR #94)" -- as navigable
provenance. Vocabulary corrected in three places: "Agent" for a human
support role, "Assistant" for an AI actor, and "module loader" rather than
"plugin loader", since the substrate reserves "plugin" for the legal phrase
"plugin exception".
.coderabbit.yaml is untouched: a tool's own configuration has to name it.
No logic changed. Every edit is inside a comment except eight test titles,
which are descriptive strings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
97f8868 to
6b35be1
Compare
🟢 SAFE TO MERGE
All 8 gates green, including the full test suite. No behaviour change — comments, spec prose, and test titles only. Three INFERRED items below are style calls, listed for the record rather than blocking.
Review: CodeRabbit reviewed this tree and its findings were adjudicated (see conversation). Its current check reads "Review skipped: incremental reviews are disabled" — a force-push rewrote the commit messages, leaving the tree byte-identical to the version it reviewed. No review coverage was lost, but this check is not a fresh review of this exact head.
The problem
Public documents narrated how they were made, in two forms:
draft.3 (2026-07-19, CodeRabbit review, PR #88),// CodeRabbit (Major):,found by adversarial review. That publishes how the work gets reviewed, and it dates instantly.78 instances across 8 specs and 40 source files.
What changed
Specs. Every instance restated forward. Where a self-correction carried a real argument, the argument stays —
passkeys.md§8 now asks "why not log-only across the board?" and answers it. Where it carried none, it's gone.Two draft-numbered changelogs became dated entries.
agents-and-auth.mdkeeps every substantive change from its seven former entries — mailbox access, the closedstatuslifecycle, the acting-Agent header rule, the advisory-lock serialization, the hardening list — with PR references retained.passkeys.mdcollapses to one entry, since none of it has shipped.Source comments. Attribution dropped, technical content kept:
store/mailboxes.ts— "Why not tell the operator to disconnect and retry? Because that instruction would be false:markDisconnectedonly setsstatus…"providers/inbound-email.ts— "Why not return aNormalizedInboundEmail? That puts the parse inside the provider…"mail/gmail-reconcile.ts— "Why not just ack? The tempting reasoning is 'the holder will advance the cursor'…"Ticket and PR references are kept —
HT-70:,(PR #94). Those are navigable provenance rather than process narration..coderabbit.yamlis untouched: a tool's own configuration has to name it.Two corrections to the specs themselves
Surfaced while rewriting the changelogs, and worth flagging separately because they are content, not style:
agents-and-auth.mddecision point 4 recorded "model the table; no scoping behavior or UI" while §3.4 manages real grants with endpoints and a UI. The recorded decision is left intact — a supersession note now points at §3.4.inbound-ingestion.md§2 had a truncated sentence ("hands attachment ownership to the transport. changes the seam"). Repaired.Declined
Capitalizing
Modulesthroughout.substrate-v1.md's vocabulary note uses lower-case in prose — "the extension artifacts are modules" — and "module catalog" names an actual document. That would be a repo-wide style change on contested ground, not a hygiene fix.Decision provenance
.coderabbit.yamlout of scope;Modulescapitalization declinedOne-way doors
None. Comment and prose edits, fully reversible. No API, schema, licensing term, or public promise touched.
Verification
typecheck✅ ·web typecheck✅ ·lint✅ · full test suite ✅ (CI, 14m)Every source edit is inside a comment except test titles, which are descriptive strings — verified mechanically.
Prevention
A pre-commit hook blocks these patterns, scoped to this organization's repositories. The same history stays useful in private repos and is not blocked there.