feat(engine): per-Agent identity, login & user management — engine layer (HT-54) - #70
Conversation
Adds agents, agent_auth_identities (with the one-password-per-agent partial unique index), and agent_mailbox_access (schema-only, no behavior yet) per specs/auth/agents-and-auth.md §3, plus the conversations.assignee -> assignee_agent_id breaking swap (§3.3). AgentStore implements the last-admin invariant (§5) with the same pg_advisory_xact_lock discipline migrate.ts already uses for its own cross-instance race, on a distinct lock key shared between createFirstAdmin's zero-Agents guard and the role/status mutation guard in updateAgent/deleteAgent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- src/auth/password-hash.ts: scrypt (node:crypto), per-identity random salt, fixed cost params, self-describing encoding; DUMMY_HASH for timing-comparable rejection of unknown emails (spec §9). - src/auth/provider.ts + password-provider.ts: the AuthProvider seam (spec §4) and the core's one implementation, dispatched by email + scrypt-verified password, uniform null on any failure (unknown email, wrong password, non-active Agent). - src/auth/invite-token.ts: hti.-prefixed signed invite tokens, mirroring gmail-connect.ts's gmc. state-token pattern off the same Keyring, 72h TTL. One-time-ness comes from AgentStore.acceptInvite's atomic status transition, not the token itself. - src/auth/invite-email.ts: builds the invite OutboundEmail via the EmailSender transport directly (never sendReply — an invite has no conversation to thread against). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New src/api/agents.ts handlers for the full auth-bootstrap + Agent
management surface (spec §6): /auth/providers, /setup, /auth/verify,
/auth/me, /auth/invite/accept, and /agents CRUD + /password +
/invite. New src/api/acting-agent.ts resolves the X-Helpthread-Agent-Id
header to the current, active Agent row (spec §8) — required on every
/agents/* route, /auth/me, and PUT .../assignee; missing/disabled maps
to a uniform 401 everywhere.
GET /agents is open to any ACTIVE acting Agent (not admin-gated) per
the coordinator's amendment: the inbox's assignee picker needs the
roster to render names for a non-admin's own assignee menu.
Breaking (spec §3.3/§10, coordinated with the assignee_agent_id
migration): PUT /conversations/{id}/assignee's body is now
{ assigneeAgentId: uuid | null } (was { assignee: 'me' | null }); the
old shape is a plain 400 now. The endpoint requires the acting-Agent
header; a non-null assigneeAgentId must name an existing Agent or the
request is 400 validation_failed.
InboxApiDeps.agents is REQUIRED (agents/auth is core, not an
absent-by-default feature like openTracking/gmailPush).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ss (HT-54) - config.ts: optional HELPTHREAD_UI_BASE_URL (uiBaseUrl) — absent by default, validated as a bare http(s) origin when set. Invite email deps are simply absent without it; the admin-set-password path still works on any deployment. - root.ts: builds createAgentStore(db) and the provider registry [createPasswordAuthProvider], wires them into createInboxApi's new required `agents` deps. - scripts/dev-api.ts: same required deps for the local dev harness (no UI base URL in this harness, so invites always report inviteSent: false — the admin-set-password path is what the harness exercises). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ignee UI (HT-54) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1 copy (HT-54)
Review fixes (two-lens adversarial review):
- /auth/verify now rejects >256-char candidates before any KDF work — the
one pre-session, rate-limit-free entry point could otherwise be fed
unbounded scrypt input; the cap constant moves next to the KDF
(password-hash.ts) so the API validation and login path can never drift
- /auth/verify's uniform 401 gets sign-in phrasing ('Invalid email or
password.') instead of the acting-Agent header copy
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds multi-agent persistence, password and invite authentication, authenticated agent-management routes, acting-agent authorization, and agent-based conversation assignment. The local API harness and composition root now inject agent stores and authentication providers, with coverage spanning migrations, stores, routing, handlers, and end-to-end behavior. ChangesAgents, authentication, and assignment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant InboxApi
participant AuthProvider
participant AgentStore
Client->>InboxApi: POST /auth/verify
InboxApi->>AuthProvider: authenticate(AuthAttempt)
AuthProvider->>AgentStore: getPasswordIdentityByEmail()
AgentStore-->>AuthProvider: PasswordIdentity
AuthProvider->>AgentStore: getAgent()
AgentStore-->>AuthProvider: active AgentRecord
AuthProvider-->>InboxApi: VerifiedIdentity
InboxApi-->>Client: authentication 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 |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
src/auth/password-hash.test.ts (1)
51-57: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winProve that
DUMMY_HASHis decodable and actually runs scrypt.These assertions also pass for a malformed four-segment value because
verifyPasswordreturnsfalseon decode failure. GenerateDUMMY_HASHfrom a documented test preimage and assert that preimage verifies successfully; keep the unrelated-password assertion as the rejection check.As per coding guidelines, “continue until the result is verified rather than merely plausible.”
🤖 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/auth/password-hash.test.ts` around lines 51 - 57, Update the DUMMY_HASH test to use a documented test preimage and assert that verifyPassword accepts that preimage, proving the hash is decodable and executes scrypt. Retain the existing unrelated-password assertion to confirm rejection, while keeping the split-length and scrypt-prefix checks in the test.Source: Coding guidelines
🤖 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/api/agents.ts`:
- Around line 281-282: Update the GET /api/v1/agents handler around listAgents
to return the mapped Agent array directly, matching the documented Agent[] wire
contract rather than wrapping it in an agents property. Add an exact
response-shape test that first fails against the current wrapper and verifies
the endpoint returns only the expected array.
In `@src/api/conversations.ts`:
- Around line 697-703: Update the assignee validation and assignment flow in the
conversation handler around getAgent and setConversationAssignee to handle an
Agent deleted between those operations. Catch the store’s known foreign-key
violation and return the documented 400 validation_failed response for
assigneeAgentId; preserve existing behavior for successful assignments and
unrelated errors.
In `@src/auth/password-hash.ts`:
- Around line 119-122: Replace synchronous scrypt usage in hashPassword() and
verifyPassword() with async crypto.scrypt, updating both functions to return
Promises while preserving their current hashing and verification behavior.
Propagate the async flow through every caller in setup, login, password reset,
and invite acceptance, awaiting the results before continuing.
- Around line 89-111: Validate the parsed scrypt parameters in the hash-parsing
flow before returning from the relevant function: cap n, r, and p to the
established safe work-factor bounds, and reject decoded hash values whose length
exceeds the expected derived-key size. Update the validation around the existing
n/r/p checks and salt/hash decoding so verifyPassword cannot pass
attacker-controlled resource requirements to scryptSync.
In `@src/composition/config.ts`:
- Around line 290-295: Update the HELPTHREAD_UI_BASE_URL validation around
parsed.protocol to require HTTPS for all non-loopback hosts, while allowing HTTP
only for explicit loopback hosts such as localhost or 127.0.0.1. Preserve
rejection of other protocols, and add tests covering HTTPS acceptance,
non-loopback HTTP rejection, and permitted loopback HTTP development URLs.
In `@src/db/migrate.ts`:
- Line 910: Add an index for the conversations.assignee_agent_id column
alongside the ALTER TABLE migration, using the project's established
migration/index naming conventions so agent deletions can efficiently update
referencing conversations.
- Around line 892-903: Update the agent_auth_identities schema to enforce that
rows with provider = 'password' must have a non-null secret_hash, while
preserving nullable secret_hash for other providers. Add a regression test that
attempts to insert a password identity without secret_hash and verifies the
database rejects it.
In `@src/store/agents.test.ts`:
- Around line 67-81: Replace the raw pg_advisory_xact_lock assertion with
coverage of createFirstAdmin: use an instrumented Db to verify the advisory lock
is acquired before the zero-Agents check, then add a two-connection concurrent
bootstrap test asserting exactly one Agent is created. Reuse the existing
freshStore and createFirstAdmin setup, while keeping the test focused on
ordering and the single-winner race behavior.
In `@src/store/conversations.ts`:
- Around line 489-503: Update setConversationAssignee so agent existence
validation and the assignee update occur atomically, preventing deletion between
validation and write from causing an uncontrolled FK error. Use a transaction
with appropriate locking, or explicitly translate the FK violation into the
documented validation response, while preserving null release and
missing/deleted conversation behavior.
---
Nitpick comments:
In `@src/auth/password-hash.test.ts`:
- Around line 51-57: Update the DUMMY_HASH test to use a documented test
preimage and assert that verifyPassword accepts that preimage, proving the hash
is decodable and executes scrypt. Retain the existing unrelated-password
assertion to confirm rejection, while keeping the split-length and scrypt-prefix
checks in the test.
🪄 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: 280340aa-5dc3-4821-ba01-e49241df2069
📒 Files selected for processing (29)
scripts/dev-api.tsspecs/auth/agents-and-auth.mdsrc/api/acting-agent.tssrc/api/agents.test.tssrc/api/agents.tssrc/api/conversations.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/auth/invite-email.test.tssrc/auth/invite-email.tssrc/auth/invite-token.test.tssrc/auth/invite-token.tssrc/auth/password-hash.test.tssrc/auth/password-hash.tssrc/auth/password-provider.test.tssrc/auth/password-provider.tssrc/auth/provider.tssrc/composition/config.test.tssrc/composition/config.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/store/agents.test.tssrc/store/agents.tssrc/store/conversations.test.tssrc/store/conversations.ts
- migration 018: CHECK (password identities carry a secret_hash); index on
conversations.assignee_agent_id (FK without an index full-scans on delete)
- password-hash decode: ceilings on embedded N/r/p and salt/hash sizes — a
corrupted or hostile stored value can't buy unbounded scrypt work
- config: HELPTHREAD_UI_BASE_URL refuses plain http except loopback (invite
links carry a signed credential)
- assignee FK race: agent deleted between existence check and UPDATE now
maps to 400 validation_failed via a translated store outcome, not a 500
- agents store test: instrumented-Db assertion that the advisory lock is
acquired BEFORE the zero-Agents check inside createFirstAdmin
- spec: §6 documents the as-built response envelopes ({agent}/{agents})
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements the engine half of specs/auth/agents-and-auth.md (HT-54). The web half follows in its own PR; the two breaking changes deploy coordinated (spec §10).
What's here
agents,agent_auth_identities(one-password-per-Agent partial unique index),agent_mailbox_access(schema only, nothing consults it — §3.4 decision), andconversations.assigneegraduates toassignee_agent_id uuid FK(breaking:'me'rows become NULL by construction).AgentStore— first-admin bootstrap, CRUD, last-admin invariant; every active-admin-reducing mutation serialized viapg_advisory_xact_lockinside one transaction (predicates stay as inner guards).AuthProviderinterface + registry (wired at the composition root),PasswordAuthProvider(scrypt, per-identity salt, dummy-hash timing equalization), invite tokens (hti.domain-separated fromht./gmc., 72h TTL, one-time via the atomicinvited→activetransition)./setup,/auth/providers,/auth/verify(uniform 401, no enumeration),/auth/me,/agentsCRUD +/password+/invite+/auth/invite/accept; acting-Agent (X-Helpthread-Agent-Id) enforcement on/agents/*,/auth/me, andPUT .../assignee(breaking: body is now{ assigneeAgentId }).EmailSendertransport directly — neversendReply(no phantom threads).Decisions for the maintainer's eye (defaults taken, flagged, not silent)
GET /agentsis open to any active Agent (spec draft said admin-only; amended in this PR's spec commit + changelogdraft.4). The assignee UI needs the roster — a non-admin couldn't render an assignee's name otherwise. Mutations stay admin-only.sendInviteon a deploy with noHELPTHREAD_UI_BASE_URLcreates the Agent and returnsinviteSent:false(recoverable via configure-then-resend); the web UI will surface it.Review & verification
PostgresDb.transaction(one pinned pooled client; xact-scoped locks — the flavor that survives Supabase's transaction-mode pooler).vitest run✅ 1019 tests / 50 files, incl. migration-upgrade path over 1..17, authz matrix, uniform-401, token domain-separation, last-admin concurrency guards, invite one-time replay.crypto.scryptif/when a public login sees real concurrency (with HT-53).Charter notes: no new dependencies; no copyleft-derived code; Agents/Assistants vocabulary enforced; mail semantics untouched (invites use the sender transport only).
🤖 Generated with Claude Code
Summary by CodeRabbit
401behavior and uniform error envelopes for malformed/invalid cases.