feat(api): agent-inbox API v1 — foundation + read paths (HT-17) - #14
Merged
Conversation
Helpthread's first public API, designed native (FreeScout wire-compat
dropped — zero customers, no cutover pressure). Framework-agnostic
Request→Response; a Vercel adapter is a later thin wrapper.
- specs/api/agent-inbox-v1.md — native domain model + contract; deletes the
superseded conversations-v1.md.
- src/api/: constant-time Bearer auth (runs BEFORE routing so nothing leaks
to an unauthenticated caller), native { error: { code, message } } envelope,
Cache-Control: no-store on every response, keyset-cursor pagination.
GET /api/v1/conversations (inbox list, newest-activity-first, status
filter) + GET /api/v1/conversations/{id} (conversation + threads).
- src/store: listConversations (keyset by updated_at,id; deleted never
surfaced); getConversation gains { includeDeleted } to exclude deleted at
the lookup on the public path.
Codex adversarial review (auth/leak boundary, standing rule) → all fixed:
- forged cursor / non-UUID path id would hit the uuid column and throw an
uncaught 500 → shape-guard UUIDs at the boundary (400 for a bad cursor,
404 for a bad id);
- no top-level try/catch → a handler throw escaped the envelope + no-store →
wrapped dispatch, generic 500, leaks nothing (and used `return await` so
async rejections are actually caught);
- an empty apiToken made `Bearer ` authenticate → fail closed at
construction (min token length);
- deleted-vs-nonexistent latency side-channel → exclude deleted in the
conversation lookup, before threads load.
193 tests pass; typecheck + Biome clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds the Agent Inbox API v1 specification and implements authenticated conversation listing and retrieval with pagination, routing, standardized responses, UUID validation, and conversation-store support. The previous draft Conversations API specification is removed. ChangesAgent Inbox API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant createInboxApi
participant authenticateRequest
participant matchRoute
participant ConversationStore
Client->>createInboxApi: Send API request
createInboxApi->>authenticateRequest: Check Bearer token
authenticateRequest-->>createInboxApi: Return authentication result
createInboxApi->>matchRoute: Match route and method
matchRoute-->>createInboxApi: Return route outcome
createInboxApi->>ConversationStore: Query conversations
ConversationStore-->>createInboxApi: Return summaries or details
createInboxApi-->>Client: Return JSON 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/api/agent-inbox-v1.md`:
- Around line 10-17: The documentation uses “agent” ambiguously for human
support staff; consistently capitalize and use “Agent” for human roles
throughout the document, including the noted references, while reserving
“Assistant” exclusively for AI actors.
In `@src/api/auth.ts`:
- Around line 5-8: Update the terminology in the documentation comment near the
v1 credential description: replace “per-agent” with “per-Agent” to consistently
distinguish human Agents from AI Assistants.
- Around line 40-45: Update the authorization-header validation in the relevant
auth helper to recognize the Bearer scheme case-insensitively while preserving
case-sensitive token comparison via constantTimeEquals. Normalize or otherwise
compare only the scheme portion before slicing the token, and add regression
tests covering lowercase “bearer” and multiple whitespace cases.
- Line 27: Remove the node:crypto import from src/api/auth.ts and replace
timingSafeEqual usage with a Web Crypto–compatible constant-time comparison,
preserving equal-length and mismatch handling; alternatively, explicitly mark
the API Node-only and remove its Edge compatibility declaration in
src/api/index.ts.
In `@src/api/cursor.test.ts`:
- Around line 52-55: Update the bad-date fixture in the decodeCursor test to use
a valid UUID for the payload’s i field while keeping u as an invalid date,
ensuring validation reaches the date-parsing path.
In `@src/api/responses.ts`:
- Around line 34-57: Extend the shared response helpers json and apiError to
accept optional internal extra headers, preserving existing callers, and ensure
every 401 response includes the WWW-Authenticate: Bearer header. Update API
specification §3 to document this required Bearer challenge for 401 responses.
- Around line 47-53: Update the API response documentation near the message
safety contract to refer to the human token holder as an “Agent” and the AI
actor as an “Assistant,” replacing the current role terminology while preserving
the existing security guidance.
🪄 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: e47a3f9d-71ad-447b-9020-d3ef3f38d7ca
📒 Files selected for processing (16)
specs/api/agent-inbox-v1.mdspecs/api/conversations-v1.mdsrc/api/auth.test.tssrc/api/auth.tssrc/api/conversations.tssrc/api/cursor.test.tssrc/api/cursor.tssrc/api/index.test.tssrc/api/index.tssrc/api/responses.tssrc/api/router.test.tssrc/api/router.tssrc/api/uuid.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/index.ts
💤 Files with no reviewable changes (1)
- specs/api/conversations-v1.md
Comment on lines
+40
to
+45
| if (header === null || !header.startsWith(BEARER_PREFIX)) { | ||
| return false | ||
| } | ||
|
|
||
| const provided = header.slice(BEARER_PREFIX.length) | ||
| return constantTimeEquals(provided, token) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Accept case-insensitive Bearer schemes.
startsWith('Bearer ') rejects valid bearer <token> credentials. HTTP authentication scheme identifiers are case-insensitive; keep the extracted token comparison case-sensitive. Add lowercase-bearer and multiple-SP regression cases. (datatracker.ietf.org)
Proposed fix
-const BEARER_PREFIX = 'Bearer '
+const BEARER_HEADER = /^Bearer +(.+)$/i
export function authenticateRequest(request: Request, token: string): boolean {
const header = request.headers.get('authorization')
- if (header === null || !header.startsWith(BEARER_PREFIX)) {
+ const match = header === null ? null : BEARER_HEADER.exec(header)
+ if (match === null) {
return false
}
- const provided = header.slice(BEARER_PREFIX.length)
+ const provided = match[1]
return constantTimeEquals(provided, token)
}📝 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.
Suggested change
| if (header === null || !header.startsWith(BEARER_PREFIX)) { | |
| return false | |
| } | |
| const provided = header.slice(BEARER_PREFIX.length) | |
| return constantTimeEquals(provided, token) | |
| const BEARER_HEADER = /^Bearer +(.+)$/i | |
| export function authenticateRequest(request: Request, token: string): boolean { | |
| const header = request.headers.get('authorization') | |
| const match = header === null ? null : BEARER_HEADER.exec(header) | |
| if (match === null) { | |
| return false | |
| } | |
| const provided = match[1] | |
| return constantTimeEquals(provided, token) | |
| } |
🤖 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/api/auth.ts` around lines 40 - 45, Update the authorization-header
validation in the relevant auth helper to recognize the Bearer scheme
case-insensitively while preserving case-sensitive token comparison via
constantTimeEquals. Normalize or otherwise compare only the scheme portion
before slicing the token, and add regression tests covering lowercase “bearer”
and multiple whitespace cases.
Comment on lines
+52
to
+55
| const badDate = Buffer.from(JSON.stringify({ u: 'not-a-date', i: 'conv-1' }), 'utf8').toString( | ||
| 'base64url', | ||
| ) | ||
| expect(decodeCursor(badDate)).toBeNull() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a valid UUID in the bad-date fixture.
'conv-1' is rejected by isUuid before new Date(payload.u) runs, so this test does not cover invalid-date handling.
Proposed fix
- JSON.stringify({ u: 'not-a-date', i: 'conv-1' })
+ JSON.stringify({ u: 'not-a-date', i: UUID })📝 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.
Suggested change
| const badDate = Buffer.from(JSON.stringify({ u: 'not-a-date', i: 'conv-1' }), 'utf8').toString( | |
| 'base64url', | |
| ) | |
| expect(decodeCursor(badDate)).toBeNull() | |
| const badDate = Buffer.from(JSON.stringify({ u: 'not-a-date', i: UUID }), 'utf8').toString( | |
| 'base64url', | |
| ) | |
| expect(decodeCursor(badDate)).toBeNull() |
🤖 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/api/cursor.test.ts` around lines 52 - 55, Update the bad-date fixture in
the decodeCursor test to use a valid UUID for the payload’s i field while
keeping u as an invalid date, ensuring validation reaches the date-parsing path.
Comment on lines
+34
to
+57
| export function json(status: number, body: unknown): Response { | ||
| return new Response(JSON.stringify(body), { | ||
| status, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Cache-Control': 'no-store', | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Build an error `Response` in the standard envelope (spec §3). | ||
| * | ||
| * `message` MUST be user-safe: never a stack trace, never a raw SQL error, | ||
| * never an upstream provider's response body, never an id the caller didn't | ||
| * already supply themselves. It is shown to whoever is holding the service | ||
| * Bearer token — today that's the operator, but the contract is written for | ||
| * the day an AI assistant or a less-trusted integration holds it instead. | ||
| * When in doubt, write a generic message and let server-side logging (not | ||
| * this response) carry the detail. | ||
| */ | ||
| export function apiError(status: number, code: string, message: string): Response { | ||
| const body: ApiError = { error: { code, message } } | ||
| return json(status, body) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the required Bearer challenge to 401 responses.
apiError cannot attach WWW-Authenticate, so 401s produced through the shared helper are non-compliant. HTTP requires every 401 response to include at least one authentication challenge. (rfc-editor.org) Extend json/apiError with internal extra headers and emit WWW-Authenticate: Bearer for status 401; document it in API spec §3 as well.
🤖 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/api/responses.ts` around lines 34 - 57, Extend the shared response
helpers json and apiError to accept optional internal extra headers, preserving
existing callers, and ensure every 401 response includes the WWW-Authenticate:
Bearer header. Update API specification §3 to document this required Bearer
challenge for 401 responses.
Comment on lines
+47
to
+53
| * `message` MUST be user-safe: never a stack trace, never a raw SQL error, | ||
| * never an upstream provider's response body, never an id the caller didn't | ||
| * already supply themselves. It is shown to whoever is holding the service | ||
| * Bearer token — today that's the operator, but the contract is written for | ||
| * the day an AI assistant or a less-trusted integration holds it instead. | ||
| * When in doubt, write a generic message and let server-side logging (not | ||
| * this response) carry the detail. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required role names in API prose.
Call the human token holder an Agent and the AI actor an Assistant.
As per coding guidelines, human support staff are Agents, and AI actors are Assistants; never conflate them in schema, code, documentation, or prose.
🤖 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/api/responses.ts` around lines 47 - 53, Update the API response
documentation near the message safety contract to refer to the human token
holder as an “Agent” and the AI actor as an “Assistant,” replacing the current
role terminology while preserving the existing security guidance.
Source: Coding guidelines
Address CodeRabbit on #14: - index.ts (Major): drop the misleading "Vercel Edge Function" claim. The engine is Node-bound anyway — reply-token.ts uses node:crypto's createHmac (no sync Web Crypto equivalent), so Edge was never achievable; auth.ts's node:crypto import is that same constraint, not a new one. Documented as Node-runtime; the Request/Response shape stays portable across Node hosts. - Use the project's defined vocabulary (CLAUDE.md): human support staff are 'Agent', not lowercase 'agent' — corrected across agent-inbox-v1.md and auth.ts (Minor). 193 tests pass; typecheck + Biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
Contributor
Author
|
@CodeRabbit resolve all of the comments that are good to go |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api graphql -f query='
query {
repository(owner: "Helpthread", name: "helpthread") {
pullRequest(number: 14) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
comments(first: 20) {
nodes {
id
databaseId
author { login }
body
createdAt
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes'Length of output: 17817 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== auth.ts ==="
cat -n src/api/auth.ts
echo "=== cursor.test.ts (bad date section) ==="
grep -n "not-a-date\|conv-1\|UUID" src/api/cursor.test.ts
echo "=== responses.ts ==="
cat -n src/api/responses.tsLength of output: 6589
The other threads (terminology fixes in Since none of the four above show corresponding code changes, I'll leave them open rather than resolve them. Let me know if you'd like me to go ahead and resolve any of them anyway, or if you plan to address them in a follow-up commit. |
This was referenced Jul 12, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes HT-17. Helpthread's first public API — the agent/inbox surface (mail lands → see it → reply), designed native now that FreeScout wire-compat is dropped (zero customers, no cutover pressure). Framework-agnostic
Request → Response; a Vercel adapter is a later thin wrapper. This PR is the foundation + read paths; HT-18 adds the write paths (reply, close/reopen).What's here
specs/api/agent-inbox-v1.md— the native domain model + contract (UUID ids, our ownopen/closedvocab, no_embedded, no integer surrogates). Deletes the supersededconversations-v1.md.src/api/(framework-agnostic): constant-time Bearer auth that runs before routing (an unauthenticated caller learns nothing about which routes exist), native{ error: { code, message } }envelope,Cache-Control: no-storeon every response, keyset-cursor pagination.GET /api/v1/conversations— inbox list, newest-activity-first,statusfilter, cursor paging; adeletedconversation never appears.GET /api/v1/conversations/{id}— conversation + threads; missing / deleted / malformed-id all → identical 404.src/store—listConversations(keyset byupdated_at,id) andgetConversation({ includeDeleted }).Codex adversarial review (auth/leak boundary — standing rule)
Codex returned DON'T-SHIP with 4 findings; all fixed with tests:
uuidcolumn and throw an uncaught 500 → shape-guard UUIDs at the boundary (bad cursor → clean 400, bad id → 404).try/catch→ a handler/store throw escaped the error envelope +no-storeand let the host decide the 500 shape (stack leak risk) → wrapped dispatch, generic 500 that leaks nothing. (Fixing this surfaced areturn awaitsubtlety — a barereturn promiseescapes the catch — which a test caught.)apiTokenmadeAuthorization: Bearerauthenticate → fail closed at construction (minimum token length).Testing
Real PGlite + real
Requestobjects: auth (missing/scheme/wrong-token/empty-token), list ordering/filter/pagination/deleted-exclusion/bad-cursor, get incl. deleted→404 and non-UUID→404, the 500 envelope+no-store path, and cross-cutting conventions. 193 tests pass, typecheck + Biome clean.Security-critical (auth) so a Codex confirm pass on the fixes is running; I'll note the verdict.
🤖 Generated with Claude Code
Summary by CodeRabbit
limit, and opaque cursor pagination; standardized thread ordering and wire shaping.401/404/405, includingAllowheaders and generic error envelopes withCache-Control: no-store.