Skip to content

docs(auth): reconcile passkey classification with module catalog (HT-76) - #85

Merged
zaridan merged 1 commit into
mainfrom
docs/ht-76-passkey-core-reconciliation
Jul 19, 2026
Merged

docs(auth): reconcile passkey classification with module catalog (HT-76)#85
zaridan merged 1 commit into
mainfrom
docs/ht-76-passkey-core-reconciliation

Conversation

@zaridan

@zaridan zaridan commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • specs/auth/agents-and-auth.md (2026-07-18) classified passkey login as a marketplace-only add-on. It predates the module catalog decision made the same day (specs/modules/catalog.md §1/§2.2, HT-66), which made passkey login core — permanently free security hygiene, distinct from paid Enterprise Auth (SAML/OIDC SSO, SCIM, 2FA policy).
  • Flagged by CodeRabbit during review of PR docs(modules): reclassify KB and portal to paid, close 71-module gap audit (HT-75) #82 as out of scope there since it predates that PR.
  • Reclassifies passkey throughout agents-and-auth.md (§1, §3.2, §11, Changelog): passkey is core, just not yet built — it lands as a second core auth provider on the existing §4 provider seam, wired the same way PasswordAuthProvider is, never through the marketplace path. Google SSO, magic-link, and SAML/enterprise SSO remain marketplace, unchanged.
  • The agent_auth_identities provider-abstraction architecture is untouched — it already supports multiple core providers, not just marketplace ones, so no schema/migration change was needed.
  • specs/modules/catalog.md and CHARTER.md were not touched — they already state the correct line.

Test plan

  • Verified catalog.md §1/§2.2 and CHARTER.md free/paid line match the new wording in agents-and-auth.md
  • Confirmed no other references to passkey-as-marketplace remain (grep -n -i passkey specs/auth/agents-and-auth.md)
  • Human spec review (per repo convention: every substantive change gets real review before merge)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation
    • Clarified authentication-provider classifications, including passkeys (WebAuthn) as a core security capability.
    • Documented that passkey support is not included in the current increment.
    • Updated authentication identity guidance, scope details, status notes, and the changelog to reflect these clarifications.

agents-and-auth.md (2026-07-18) classified passkey login as a marketplace-only
add-on before the module catalog decision the same day (specs/modules/catalog.md
§1/§2.2, HT-66) made passkey login core, permanently free security hygiene.
Flagged by CodeRabbit on PR #82. Passkey is now core (not yet built — lands as a
second core auth provider on the §4 seam); Google SSO, magic-link, and
SAML/enterprise SSO remain marketplace, unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The authentication specification updates its draft status and identity contract, classifies passkey/WebAuthn as a core provider not built in this increment, revises marketplace-provider examples and scope language, and records the change in the draft.6 changelog.

Changes

Auth provider specification

Layer / File(s) Summary
Provider boundary and identity contract
specs/auth/agents-and-auth.md
Updates the draft scope and documents passkey/WebAuthn as a core provider rather than a marketplace provider, including its identity-row behavior and examples.
Scope and changelog alignment
specs/auth/agents-and-auth.md
Revises scope exclusions and adds the draft.6 changelog entry describing the passkey reclassification.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

  • Helpthread/helpthread#69: Establishes the core-versus-licensed-provider seam and identity model referenced by this specification update.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main doc change: reconciling passkey classification with the module catalog.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/ht-76-passkey-core-reconciliation

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 3-8: Update the introductory contract description to consistently
use “Agent management” for human support-staff administration and
“Agent-management UX” for the modeled experience. Replace the existing “user
management” and “user-management UX” references, while preserving the
distinction between Agents and Assistants.
🪄 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: 8eaf1a0f-3922-40cb-98d5-e1625a9e6d6b

📥 Commits

Reviewing files that changed from the base of the PR and between 071f715 and b4ebaa3.

📒 Files selected for processing (1)
  • specs/auth/agents-and-auth.md

Comment on lines +3 to +8
Status: **draft** (2026-07-18, amended 2026-07-19 — see Changelog draft.6) — the contract for
real per-Agent identity, login, and user management, replacing the single shared operator
password that HT-51 shipped as a deliberate placeholder. Authored native (Helpthread's own
domain model); the *experience* is modelled on the Help Scout / FreeScout user-management UX
(black-box observation only — never their source), rendered entirely in Helpthread's own
design system.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use “Agent management” terminology consistently.

“User management” and “user-management UX” are ambiguous here because this document describes human support staff. Rename these references to “Agent management” / “Agent-management UX” to preserve the repository’s vocabulary contract.

As per coding guidelines, human support staff must be referred to as “Agents” and AI actors as “Assistants”.

Proposed wording
- real per-Agent identity, login, and user management, replacing the single shared operator
+ real per-Agent identity, login, and Agent management, replacing the single shared operator
...
- (black-box observation only — never their source), rendered entirely in Helpthread's own
+ (black-box observation only — never their source), rendered entirely in Helpthread's own

Also replace “user-management UX” with “Agent-management UX” on Line 6.

🤖 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 3 - 8, Update the introductory
contract description to consistently use “Agent management” for human
support-staff administration and “Agent-management UX” for the modeled
experience. Replace the existing “user management” and “user-management UX”
references, while preserving the distinction between Agents and Assistants.

Source: Coding guidelines

@zaridan
zaridan merged commit bff8c97 into main Jul 19, 2026
5 checks passed
@zaridan
zaridan deleted the docs/ht-76-passkey-core-reconciliation branch July 19, 2026 19:39
zaridan added a commit that referenced this pull request Jul 19, 2026
…sics (#91)

PR #83 landed already-stale: main moved by nine PRs while it was in review,
and it shipped one claim that contradicts the charter.

- HT-71 operator guide was listed as an open PR under Next. #81 merged at
  17:48, ~2h before #83 merged at 19:39, and docs/modules/ has been on main
  since. Moved to Done with its precision follow-up (#84).
- Marketplace was listed under 'Not yet / deferred'. CHARTER §3/§4/§5 were
  amended the same day (HT-79, #86) to make it a launch-day component of
  Phase 3 — built now, proven as the dogfood install path. Removed from
  deferred; marketplace v1 spec (#87, draft) now leads Next.
- Added inbox basics (HT-76/77/78, #90, migrations 24-25): saved replies &
  macros, snooze, send & close — shipped engine features with no STATUS line.
- Added catalog reclassification (HT-75, #82): KB and end-user portal are
  paid, 71-module gap audit closed, open-core line restated. Passkeys stay
  core, reconciled in #85.
- Added passkey login spec (HT-75, specs/auth/passkeys.md, draft.3) to Next.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
zaridan added a commit that referenced this pull request Jul 19, 2026
)

* feat(engine): passkey (WebAuthn) login as a core auth provider (HT-75)

Implements specs/auth/passkeys.md end to end: migration 026
(webauthn_credentials, webauthn_challenges, webauthn_stepup_tokens),
the WebAuthnStore, the htw./htsu. signed-token pair, the shared
authentication-ceremony verifier (TOCTOU-safe FOR UPDATE counter
policy, two-tier clone/regression detection routed to the HT-44
health check, userHandle cross-check), the WebAuthnAuthProvider
(kind: 'webauthn' on the provider seam), and the full step-up +
registration + credential-management API surface. root.ts wires the
provider only when config.uiBaseUrl resolves to a domain-form
hostname; an IP-literal or unset uiBaseUrl degrades to
webauthn-absent rather than failing the whole engine boot. Engine +
API only — no web/UI (HT-75 is design-blocked; a separate ticket).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(engine): make webauthn counter-regression observability actually fire (HT-75)

Review findings (Codex SHIP, Fable lead-tier FIX-FIRST):

MAJOR — the spec §8 "reject AND alert" signal was non-functional:
- health.ts's runHealthCheck had no webauthn check at all; add
  webauthn-counter-regression, same 24h-growth idiom as
  forged-token-burst, trips the existing 200->503 pivot.
- The real @simplewebauthn verifyAuthenticationResponse throws its own
  counter-regression error using the unlocked pre-read counter, before
  our locked Tier-1/Tier-2 logic ever runs — markCounterRegression was
  reachable only in a narrow concurrent-race window, never the common
  sequential-replay case. Fixed by always passing credential.counter: 0
  to the library (structurally disables its internal throw) and relying
  entirely on our own FOR UPDATE-locked comparison, matching what spec
  §6.2 already describes as the intended split (signature verification
  vs. counter policy as two separate steps).
- The regression test mocked a resolved regressed counter, which the
  real library never produces (it throws) — false-green over a dead
  path. Replaced with a mock that faithfully reproduces the library's
  own throw-on-regression guard against whatever counter we actually
  pass, plus an explicit assertion that we pass counter: 0.
- Added the missing structured console.warn at the regression-detection
  point (spec §8: "the log line is what makes it investigable").

MINOR — src/store/webauthn.ts's module doc named a nonexistent
src/auth/webauthn-service.ts; corrected to webauthn-ceremony.ts.

Also corrects specs/auth/passkeys.md §1/§12: passkeys were already
core (specs/modules/catalog.md §1/§2.2, accepted 2026-07-18; reconciled
into agents-and-auth.md via HT-76/PR #85, merged same day as this
spec's draft.1-3). This spec inherited the pre-reconciliation
"licensed marketplace module, waiting on HT-5" framing rather than the
fix — corrected to draft.4, documentation only, no design change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(engine): close webauthn credential-delete account-lockout TOCTOU (HT-75)

CodeRabbit finding on PR #94: WebAuthnStore.deleteCredential took
FOR UPDATE on the target credential row only; the "does this Agent
have another credential" count was a separate, unlocked read. Two
concurrent deletes of DIFFERENT credentials for the same passwordless
Agent could each see the other's row as still present, both pass the
guard, and both commit -- leaving the Agent with zero credentials and
no password, locked out.

Fix: SELECT ... WHERE agent_id = $1 FOR UPDATE now locks every
credential row belonging to the Agent (not just the target) before
the guard runs, so a real concurrent Postgres session deleting a
different credential for the same Agent blocks on this same lock and
re-reads current state after the first commits.

Test note: a literal concurrent-call test does not reproduce this
race against PGlite -- verified empirically that PGlite serializes
whole transactions on its single connection (a second db.transaction()
call does not even begin until the first fully commits), so such a
test would pass identically against the old, buggy code and prove
nothing. Matches the identical, already-documented limitation in
src/store/agents.test.ts for the createFirstAdmin advisory-lock guard.
Added instead: an instrumented-Db test proving the lock SQL targets
the Agent's whole credential set (the structural fact real concurrent
Postgres sessions serialize on), and a test proving the guard's
arithmetic is correct against that locked set.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zaridan added a commit that referenced this pull request Aug 4, 2026
Restores PR references the changelog consolidation dropped. The previous
commit message claimed "ticket and PR references are kept as navigable
provenance" while the diff deleted PR #82, #88, #69 and #85 from the
consolidated entries. Claim and diff now agree.

Rewrites 28 further sites the earlier passes missed. They used phrasings
the patterns did not cover -- "an earlier revision", "the first draft
wrongly hung", "retracted here rather than quietly patched", "corrected
there in this same review round", "review round 2". Each is restated
forward and keeps its technical lesson:

  store/mailboxes.ts  "Why not tell the operator to disconnect and retry?
                       Because that instruction would be false: ..."
  providers/inbound-email.ts
                      "Why not return a NormalizedInboundEmail? That puts
                       the parse inside the provider ..."
  gmail-reconcile.ts  "Why not just ack? The tempting reasoning is ..."

Also adds a supersession note to decision point 4 in agents-and-auth.md,
which still recorded "no scoping behavior or UI" while section 3.4 manages
real grants. The recorded decision is left intact; the note points at what
superseded it.

Two artifacts from the previous pass fixed: an orphaned closing paren in
substrate-v1.md, and a substitution that made a date the grammatical
subject in migrate.ts.

Verified: typecheck, web typecheck, lint clean. Every edit is inside a
comment or markdown prose; no source line outside a comment changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant