HT-12: signed reply-token mint/verify (threading crypto core) - #8
Merged
Conversation
src/mail/reply-token.ts: mint a signed token into an outbound Message-ID, verify it on inbound. HMAC-SHA256 (full digest, base64url) over keyId.conversationId.threadId. keyId is inside the signed payload, so a token's key can't be swapped. Keyring model: current mints+verifies, retired keys verify-only, so secret rotation never invalidates tokens in customers' mailboxes. Security: verify is TOTAL (never throws on hostile input — returns null), constant-time comparison with an explicit length guard, strict mint (throws on malformed ids). 45 unit tests; 100% coverage on reply-token.ts. Independently adversarially verified: forged/tampered/key-swapped/ truncated tokens all rejected, 1000 random sigs none forged, rotation works. Resolves the threading spec's keyId open question (§2(d)) — keyId ships in v1 — and updates spec §2 to the implemented format (was a pre-impl draft with no keyId and a truncated sig). 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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR finalizes the signed reply-token specification and adds a token engine for minting, parsing, HMAC verification, key rotation, and validation testing. ChangesSigned reply token flow
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant mintReplyMessageId
participant Keyring
participant verifyReplyMessageId
Caller->>mintReplyMessageId: provide conversationId, threadId, mailDomain
mintReplyMessageId->>Keyring: read current signing key
mintReplyMessageId-->>Caller: return signed Message-ID
Caller->>verifyReplyMessageId: provide Message-ID and keyring
verifyReplyMessageId->>Keyring: inspect current and retired keys
Keyring-->>verifyReplyMessageId: return matching key
verifyReplyMessageId-->>Caller: return payload or null
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (1)
src/mail/reply-token.test.ts (1)
326-330: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert verification behavior, not only signature equality.
Comparing the two signature segments does not prove that a token with a changed
mailDomainis accepted byverifyReplyMessageId. Add an assertion thatbverifies to the expected payload.Proposed test assertion
expect(segments(a).parts[4]).toBe(segments(b).parts[4]) + expect(verifyReplyMessageId(b, ringA)).toEqual({ + keyId: 'k1', + conversationId: 'c42', + threadId: 't7', + })🤖 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/reply-token.test.ts` around lines 326 - 330, Add an assertion in the “mailDomain is not signed” test that calls verifyReplyMessageId with token b and confirms it returns the expected payload, while retaining the existing signature-equality check. Use the existing ringA and verification helpers to validate acceptance after changing mailDomain.
🤖 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/mail/threading.md`:
- Line 11: The outbound-message list in the threading documentation conflates
actor types. Update the wording to explicitly distinguish human Agent replies
from Assistant responses and other non-AI automated replies, using the
repository’s reserved terminology consistently.
- Around line 13-15: Add the text language identifier to the fenced code block
containing the token format, changing its opening fence to ```text while
preserving the existing content.
In `@src/mail/reply-token.ts`:
- Around line 110-111: Update DOMAIN_PATTERN and the mailDomain validation used
by the reply-token creation flow to require non-empty dot-separated labels,
rejecting leading, trailing, and consecutive dots while preserving valid
letters, digits, hyphens, and dots. Add regression tests covering these three
malformed domains and confirm valid domains still pass.
- Around line 150-156: Reject non-string values before validation: update the
mailDomain guard in mintReplyMessageId and assertIdField to require typeof value
=== 'string' before applying regex checks, so undefined, null, and numbers
cannot be coerced into valid identifiers.
---
Nitpick comments:
In `@src/mail/reply-token.test.ts`:
- Around line 326-330: Add an assertion in the “mailDomain is not signed” test
that calls verifyReplyMessageId with token b and confirms it returns the
expected payload, while retaining the existing signature-equality check. Use the
existing ringA and verification helpers to validate acceptance after changing
mailDomain.
🪄 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: a7a902f4-db7e-46ae-a590-484a73c6f8d1
📒 Files selected for processing (3)
specs/mail/threading.mdsrc/mail/reply-token.test.tssrc/mail/reply-token.ts
| ## 2. The reply token | ||
|
|
||
| Every outbound message (agent reply, auto-response, and any future first-party auto-reply) embeds a signed token in its `Message-ID`. Proposed format: | ||
| Every outbound message (agent reply, auto-response, and any future first-party auto-reply) embeds a signed token in its `Message-ID`. Format (implemented in `src/mail/reply-token.ts`): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Disambiguate Agents and Assistants in the outbound-message list.
Use the repository’s reserved terms explicitly: distinguish an Agent reply from an Assistant response or non-AI automated reply.
As per coding guidelines, human support staff are Agents and AI actors are Assistants; do not conflate them in documentation.
🤖 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/threading.md` at line 11, The outbound-message list in the
threading documentation conflates actor types. Update the wording to explicitly
distinguish human Agent replies from Assistant responses and other non-AI
automated replies, using the repository’s reserved terminology consistently.
Source: Coding guidelines
Comment on lines
13
to
15
| ``` | ||
| <ht.{conversationId}.{threadId}.{sig}@{mailDomain}> | ||
| <ht.{keyId}.{conversationId}.{threadId}.{sig}@{mailDomain}> | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced token-format block.
markdownlint-cli2 reports MD040 on Line 13. Use text (or another appropriate language) for the fence.
Proposed fix
-```
+```text📝 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
| ``` | |
| <ht.{conversationId}.{threadId}.{sig}@{mailDomain}> | |
| <ht.{keyId}.{conversationId}.{threadId}.{sig}@{mailDomain}> | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: 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/mail/threading.md` around lines 13 - 15, Add the text language
identifier to the fenced code block containing the token format, changing its
opening fence to ```text while preserving the existing content.
Source: Linters/SAST tools
Independent Codex (OpenAI-lineage) review found two Majors that CodeRabbit (Opus) and the author (both Claude) missed — the payoff of a different model lineage on security-critical crypto: - MAJOR (Codex): empty/weak secrets were accepted → trivial forgery on misconfig. Now assertValidKeyring enforces a 32-char minimum secret. - MAJOR (Codex): duplicate keyId in the ring let a leaked retired secret verify under a live keyId. Now keyIds MUST be unique; rotation uses a new keyId and revocation drops the key. Rotation is safe by construction. - MINOR (Codex): verify's 'never throws for any input' overclaimed — a malformed keyring (undefined secret, non-array retired) threw. Reframed: verify is total over the untrusted messageId given a valid keyring; a bad keyring is trusted-config and fails loud via assertValidKeyring. - MINOR (Codex + CodeRabbit): DOMAIN_PATTERN accepted junk (.., a..b, -x.y). Now proper per-label DNS validation. - MINOR (CodeRabbit): RegExp.test coerces non-strings — undefined/number ids could slip through. assertIdField now requires typeof string. +14 tests (45 -> 59). Re-verified with an independent forge probe: forgery/ tamper/key-swap/2000-random-sigs rejected, new guards fire, verify total over hostile message-ids. typecheck/lint/tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mail/reply-token.ts (1)
197-218: 🚀 Performance & Scalability | 🔵 TrivialRe-validating the keyring on every verify adds avoidable work to the ingest hot path.
verifyReplyMessageIdruns on the untrusted inbound path (per the module doc) and callsassertValidKeyringon every message, which rebuilds aSetand iterates all keys each time even though the keyring is static trusted config. Consider validating the keyring once at config load and passing a pre-validated ring, or memoizing validation per ring instance, to keep per-message cost minimal.Correctness is unaffected — this is purely a throughput consideration on a per-message code path.
🤖 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/reply-token.ts` around lines 197 - 218, Remove the per-message keyring validation from verifyReplyMessageId by validating the trusted keyring once during configuration loading and passing a pre-validated ring, or by memoizing validation per ring instance. Preserve validation at the trust boundary while ensuring verifyReplyMessageId only parses the message and checks candidate signatures.
🤖 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.
Nitpick comments:
In `@src/mail/reply-token.ts`:
- Around line 197-218: Remove the per-message keyring validation from
verifyReplyMessageId by validating the trusted keyring once during configuration
loading and passing a pre-validated ring, or by memoizing validation per ring
instance. Preserve validation at the trust boundary while ensuring
verifyReplyMessageId only parses the message and checks candidate signatures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ec76aba-c52e-49e5-8dcd-dcbb9a054e9b
📒 Files selected for processing (2)
src/mail/reply-token.test.tssrc/mail/reply-token.ts
Two CI failures from the hardening commit, both mine: - Lint: a long test assertion needed Biome's line-wrapping (format). - Secret scan: gitleaks flagged the FAKE test secrets (HMAC token fixtures) as real credentials. Added .gitleaks.toml allowlisting *.test.ts — test files hold fixture secrets, never real ones (real secrets live in gitignored .env). Extends the default ruleset. Also replaced 'as any' test casts with 'as unknown as' (no noExplicitAny warnings) so lint is 0 warnings / 0 errors. Verified locally: lint clean, typecheck exit 0, 79 tests pass, gitleaks no leaks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
This was referenced Jul 10, 2026
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.
The cryptographic heart of Helpthread's threading — the signed tokens that let the engine trust which conversation a reply belongs to, per the charter's 'threading authority lives on the outbound side' principle.
src/mail/reply-token.ts— pure, no I/O:mintReplyMessageIdembeds a signed token in an outbound Message-ID:<ht.{keyId}.{conversationId}.{threadId}.{sig}@{domain}>,sig = base64url(HMAC-SHA256(secret, keyId.conversationId.threadId)).verifyReplyMessageIdreturns the payload for a valid token,nullfor anything else. Total — never throws on hostile input (a crash on the ingest path would violate 'never lose customer mail'). Constant-time comparison with a length guard. Strict mint (throws on malformed ids).keyIdis inside the signature (can't be swapped), and a keyring separates thecurrentminting key fromretiredverify-only keys — so rotating a secret never breaks tokens already in mailboxes.Thoroughness (this is the crown jewel): 45 unit tests, 100% coverage. Beyond the suite, I ran an independent adversarial forge probe — forged signatures, tampered fields, key-swap, truncated sigs, a real Gmail Message-ID, and 1000 random signatures — all correctly rejected; rotation verified.
Also resolves the threading spec's keyId open question (§2(d) — keyId ships in v1) and updates spec §2 to the implemented format (it was a pre-implementation draft with no keyId and a truncated sig; a spec that contradicts the code is exactly what misleads future work).
Next: increment 3 wires this into the inbound threading decision (the 5-rule algorithm) against the HT-7 fixtures.
Jira: https://resonantiq.atlassian.net/browse/HT-12
🤖 Generated with Claude Code
Summary by CodeRabbit
Message-IDtokens with a defined format and full HMAC-SHA256 base64url signature.mailDomainis verified for format but does not affect the signature.