feat(engine): mailbox-access grants + admin API; admin IA fidelity doc (HT-54) - #72
Conversation
… (HT-54) - specs/ui/admin-ia.md: the three-scope IA contract from TJ's fidelity review (Manage/mailbox-gear/avatar), surface index of the black-box reference, module-extensibility + inference-ban rules, deliberate deviations, better-than deltas (module directory search) - agents-and-auth.md draft.5: mailbox-access semantics pinned (admins implicit-all, auto-grant-on-create, admin-only grant endpoints); conversation-visibility enforcement deferred to multi-mailbox Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…st (HT-54) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements spec §3.4/§6's pinned mailbox-access semantics: AgentStore
auto-grants every existing mailbox to a new Agent (any role, both
createFirstAdmin and createAgent) in the same transaction as the agents
insert, plus listAgentMailboxIds/replaceAgentMailboxAccess (replace-set,
FK-translated invalid_mailbox); MailboxStore gains an unfiltered
listMailboxes for the Permissions roster. Adds the admin-only
GET /api/v1/mailboxes, GET/PUT /api/v1/agents/{id}/mailboxes endpoints,
wired through the router and composition root.
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughMailbox access administration is added across mailbox and agent stores, authenticated API routes, request dispatch, runtime wiring, specifications, seed data, and end-to-end tests. Agents receive existing mailbox grants transactionally, while admins can list mailboxes and replace Agent grant sets. ChangesMailbox access administration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant InboxAPI
participant MailboxHandlers
participant AgentStore
participant MailboxStore
Client->>Router: Request mailbox endpoint
Router->>InboxAPI: Return mailbox route match
InboxAPI->>MailboxHandlers: Resolve acting Agent and dispatch
MailboxHandlers->>AgentStore: Read or replace Agent grants
MailboxHandlers->>MailboxStore: List or validate mailboxes
MailboxHandlers-->>Client: Return response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…(HT-54) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 158-173: Update the stale Permissions section introduction around
the “behavior deferred” wording to state that grant management, the Permissions
UI, and mailbox-grant data are shipped and active now; only
conversation-visibility enforcement remains deferred until conversations include
mailbox_id. Remove claims that scoping behavior/UI is absent or that nothing
reads or writes the grants table, while preserving the later semantics and
enforcement plan.
In `@src/api/agents.test.ts`:
- Around line 1214-1240: The test “an empty array clears every grant — 200” must
verify persistence rather than only the response body. After clearing the
mailbox grants, issue another PUT/readback request through the same API flow and
assert the returned mailboxIds is empty, confirming the existing grant was
deleted from storage.
- Around line 1073-1088: In the mailbox roster test, remove the duplicate const
body declaration so the test compiles, then create a disconnected mailbox using
the mailboxStore API before requesting /api/v1/mailboxes. Update the expected
statuses in the test around the includes disconnected/paused mailboxes case to
assert active, paused, and disconnected entries.
In `@src/api/agents.ts`:
- Around line 143-164: Update validateMailboxIds so each UUID is normalized to
lowercase before membership checks and insertion into seen and ids. Preserve
validation of the original string with isUuid, and ensure the returned
deduplicated array contains lowercase UUIDs in first-occurrence order.
In `@src/store/agents.ts`:
- Around line 621-624: Update the grant-replacement transaction’s agent
existence query to lock the matching row for update before processing mailbox
replacements. Keep the not_found result when no row is locked, and perform this
check before both empty and nonempty replacement paths so deleteAgent() cannot
race the operation or cause agent foreign-key errors to be classified as
invalid_mailbox.
In `@src/store/mailboxes.test.ts`:
- Around line 523-540: Update the listMailboxes test to insert a mailbox with
status needs_reconnect, include its ID in the expected sorted ID set, and retain
the existing active, paused, and disconnected cases so every lifecycle status is
verified.
🪄 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: 8e0e0403-8521-4dd2-b610-d80f86bd33f6
📒 Files selected for processing (16)
scripts/dev-api.tsspecs/auth/agents-and-auth.mdspecs/ui/admin-ia.mdsrc/api/agents.test.tssrc/api/agents.tssrc/api/gmail-webhook.test.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/composition/root.tssrc/mail/gmail-reconcile.test.tssrc/store/agents.test.tssrc/store/agents.tssrc/store/mailboxes.test.tssrc/store/mailboxes.ts
| const agentRows = await tx.query<{ id: string }>('SELECT id FROM agents WHERE id = $1', [ | ||
| agentId, | ||
| ]) | ||
| if (agentRows.length === 0) return 'not_found' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Lock the target Agent row during grant replacement.
The unlocked existence check can race with deleteAgent(). An empty replacement may return 'ok' after deletion, while a nonempty replacement can hit the agent_id FK and be misclassified as 'invalid_mailbox' because every 23503 is accepted.
Proposed fix
- const agentRows = await tx.query<{ id: string }>('SELECT id FROM agents WHERE id = $1', [
- agentId,
- ])
+ const agentRows = await tx.query<{ id: string }>(
+ 'SELECT id FROM agents WHERE id = $1 FOR UPDATE',
+ [agentId],
+ )📝 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.
| const agentRows = await tx.query<{ id: string }>('SELECT id FROM agents WHERE id = $1', [ | |
| agentId, | |
| ]) | |
| if (agentRows.length === 0) return 'not_found' | |
| const agentRows = await tx.query<{ id: string }>( | |
| 'SELECT id FROM agents WHERE id = $1 FOR UPDATE', | |
| [agentId], | |
| ) | |
| if (agentRows.length === 0) return 'not_found' |
🤖 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/agents.ts` around lines 621 - 624, Update the grant-replacement
transaction’s agent existence query to lock the matching row for update before
processing mailbox replacements. Keep the not_found result when no row is
locked, and perform this check before both empty and nonempty replacement paths
so deleteAgent() cannot race the operation or cause agent foreign-key errors to
be classified as invalid_mailbox.
There was a problem hiding this comment.
Applied in af8f069, via FK discrimination rather than a row lock: isMailboxFkViolation now requires the mailbox_id constraint (driver constraint field, message fallback) since both of the table's FKs raise 23503, and an agent_id FK violation (target hard-deleted between check and INSERT) maps to 'not_found' — the correct caller outcome — instead of a bogus 'invalid_mailbox'. A FOR UPDATE lock would serialize every grant write against agent deletion for a race whose only requirement is a correct response code.
There was a problem hiding this comment.
@zaridan, the FK discrimination is a good correction for the non-empty path: an agent_id FK violation now maps to 'not_found' instead of incorrectly becoming 'invalid_mailbox'.
However, the empty replacement case remains unresolved. After the unlocked existence check, deleteAgent() can delete the target before this transaction commits; because mailboxIds.length === 0 bypasses the INSERT and therefore has no FK violation to translate, this method still returns 'ok' for a no-longer-existing Agent.
SELECT ... FOR UPDATE is needed if the contract requires the result to reflect a stable target-Agent existence check throughout the replacement transaction. The extra serialization is limited to concurrent deletion of that same Agent, not all grant writes.
✏️ Learnings added
Learnt from: zaridan
Repo: Helpthread/helpthread PR: 72
File: src/store/agents.ts:639-642
Timestamp: 2026-07-19T00:58:18.152Z
Learning: In `src/store/agents.ts`, `agent_mailbox_access` has both `agent_id` and `mailbox_id` foreign keys that can raise SQLSTATE `23503`; `isMailboxFkViolation` must identify the `mailbox_id` constraint specifically so a concurrent target-Agent deletion maps to `ReplaceMailboxAccessResult` `'not_found'` rather than `'invalid_mailbox'`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- validateMailboxIds lowercases uuids before dedupe/storage (a mixed-case duplicate would collide with the PK inside the store's single INSERT) - isMailboxFkViolation requires the mailbox_id constraint (both FKs on agent_mailbox_access raise 23503); an agent_id FK violation now maps to not_found instead of a bogus invalid_mailbox/500 - spec §3.4 heading updated (grants managed now; only enforcement deferred) - tests: disconnected mailbox actually created in the roster test; needs_reconnect added to the all-status test; PUT-clear re-reads via GET Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Follow-up to #70, from the maintainer's fidelity review (HT-54). Pairs with the PR #71 web restructure (Permissions screen consumes these endpoints).
What's here
mailbox_id— verified, documented, tracked on HT-55).specs/ui/admin-ia.md— the admin IA & fidelity contract from the maintainer's review: three-scope rule (Manage / mailbox gear / avatar-personal-only), full surface index, module-extensibility as structure, the observed≠core inference ban, core-vs-module classification from the maintainer's purchased-module list, deliberate deviations, better-than deltas. Roadmap tickets HT-55…HT-65 filed against it.MailboxStore.listMailboxes()(unfiltered roster);AgentStore.listAgentMailboxIds/replaceAgentMailboxAccess(replace-set in one transaction, FK-translatedinvalid_mailbox, prior grants survive rollback); auto-grant viaINSERT … SELECTinside the create transactions.GET /api/v1/mailboxes(id/address/status only — no provider internals),GET/PUT /api/v1/agents/{id}/mailboxes; routed ahead ofAGENT_ITEMso the suffix never mis-matches; acting-Agent enforcement identical to the other/agents/*routes.Review & verification
Implemented (Sonnet) from the amended spec; coordinator review of the diff (store transaction semantics, FK translation, authz-first ordering, roster field exposure); gates run by the coordinator with real exit codes: typecheck ✅ lint ✅ 1055 tests / 50 files ✅. Browser-level verification happens with the PR #71 walkthrough against this branch's dev API.
No new dependencies; no mail-path changes.
🤖 Generated with Claude Code
Summary by CodeRabbit