Skip to content

test(dogfood): converge two-factor-lockout on the shared TOTP helper - #11323

Merged
os-sam merged 1 commit into
mainfrom
claude/issue-11111-totp-shared-helper
Aug 23, 2026
Merged

test(dogfood): converge two-factor-lockout on the shared TOTP helper#11323
os-sam merged 1 commit into
mainfrom
claude/issue-11111-totp-shared-helper

Conversation

@os-sam

Copy link
Copy Markdown
Collaborator

Fixes#11111

two-factor-lockout.dogfood.test.ts carried a private base32Decode / totp pair. The shared packages/qa/dogfood/test/totp.ts was extracted from that file while adding a second caller (#10681) and deliberately did not rewrite its internals, so the package held two spellings of RFC 6238. This converges them. Mechanical: no assertion, timeout or fixture-semantics changes.

What changed

  • The lockout test now imports secretFromTotpUri, totp from ./totp.js — the same import its sibling two-factor-backup-code-reveal.dogfood.test.ts already uses.
  • Its local base32Decode and totp are deleted, along with the createHmac import that only they used.
  • The inline otpauth URI parsing collapses into secretFromTotpUri.
  • The shared module's header carried a stale warning that the lockout file "still carries its own private copy". That stopped being true in this commit, so it is replaced with a note that this is now the package's single copy.

Net: 48 lines deleted, 7 added.

Divergence check — the thing that could have made this unsafe

Both implementations were compared before anything was touched. Extracting the function bodies and diffing them yields exactly one difference each, the export keyword:

base32Decode: 1c1 export function base32Decode(...) vs function base32Decode(...)
totp: 1c1 export function totp(...) vs function totp(...)

No divergence. Nothing behavioural is being smuggled in under a dedupe.

The one semantic nuance, stated rather than hidden

The collapsed block previously guarded the URI with expect(uriSecret, 'no secret in the otpauth URI').toBeTruthy(). secretFromTotpUri instead throws Error('no secret in the otpauth URI') — same message, same red/green outcome, a thrown error rather than an assertion. The card sanctions this collapse explicitly ("plus the secret param lookup and its toBeTruthy guard, so that block can collapse too").

Why the green suite here is not a tautology

A dedupe PR's real risk is not that the suite goes red — it is that the suite never exercised the helper, so repointing it at a different implementation proves nothing. Predicted in writing before running: mutating the SHARED totp() must fail inside beforeAll, at the enrolment confirmation, because that is the first consumer of a TOTP in the file.

Measured, with the shared totp() mutated to return a fixed wrong code:

injected marker count : 1 (must be 1)
removed original count: 0 (must be 0)
FAIL |isolated| test/two-factor-lockout.dogfood.test.ts (beforeAll hook)
AssertionError: verify-totp (enrolment): {"message":"Invalid code","code":"INVALID_CODE"}: expected 401 to be 200
Test Files 1 failed (1) Tests 5 skipped (5)

Restored byte-identically — git hash-object equal on both sides (f5b906f55752e74ae4ba5c4be2d67891b621cbc9), injected marker absent, original line back — and re-run to a real green: Test Files 1 passed (1) / Tests 5 passed (5). The mutation ran under a trap ... EXIT INT TERM restore, so a cap kill mid-mutation could not have left the tree mutated.

Two honest deviations from the prediction: the rejection status is 401 INVALID_CODE, not the 400 I guessed, and vitest accounts a beforeAll failure as 1 file failed / 5 skipped rather than 5 failed. Location and cause matched the prediction.

Pre-change the same mutation could not have moved this file at all — it referenced ./totp.js nowhere, which is the contrast that makes the ablation a statement about the new wiring.

No dist preflight applies: test/totp.ts is a relative sibling module compiled from source by vitest, and @objectstack/dogfood declares no build script.

Verification

Baseline on the same ref, before the change: Test Files 1 passed (1) / Tests 5 passed (5). After the change: identical.

Gate union derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (no hand-supplied paths) and run at commit 3c6028f1b2. 11 matched + 7 convention-triggered families, every one exit 0:

check:empty-state · check:liveness · check:published-files · check:slot-lookup · check:strictness-ledger · check:test-source-alias · check:type-source-resolution · check:variant-docs · check-ci-filter-parity · check-plugin-teardown-shape · check-affected-docs · check:query-options-erasure · check:type-check-coverage · check:type-check-debt · check:engine-double-contract · check:cross-package-test-inputs · check:where-matcher · check:nul-bytes · @objectstack/dogfood typecheck

check:type-check-debt first refused with PREREQUISITE NOT MET@objectstack/service-knowledge had no built type entry point, and the gate is explicit that measuring from there measures a different world. That is NOT MEASURED, not a pass, so the closure was built and the gate re-run to its own verdict line:

check-type-check-coverage --re-measure: OK — 33 ledger entr(ies) re-measured in 449.8s,
1897 raw tsc error(s) total, none above its recorded number.

Changeset

skip-changeset. @objectstack/dogfood is "private": true and this PR touches only two of its test files, so it publishes nothing — the textbook case the changeset workflow's own comments name.

Generated by Claude Code


Generated by Claude Code

`two-factor-lockout.dogfood.test.ts` carried a private `base32Decode` /
`totp` pair, byte-for-byte identical to the ones in `test/totp.ts` — the
shared module was extracted FROM this file (#10681) without rewriting its
internals, so the package held two spellings of RFC 6238.
Point the file at `./totp.js`, delete both local functions, and collapse
the inline otpauth:// parsing into `secretFromTotpUri`. Mechanical: no
assertion, timeout or fixture-semantics changes.
The hand-rolled rationale (`@better-auth/utils/otp` is a transitive
dependency; taking a direct one to make six digits would tie these
fixtures to an internal package's resolution) already lives in the shared
module's header, so it survives. That header's stale "the lockout file
still carries its own copy" warning is replaced by the fact that this is
now the package's single copy.
Verified the file really drives the shared helper rather than merely
importing it: mutating the shared `totp()` to return a fixed wrong code
turns the suite red in `beforeAll` (enrolment confirmation, 401
INVALID_CODE vs the expected 200); restoring it byte-identically returns
it to 5 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/qa/dogfood/test/totp.ts) — pages documenting those are invisible to this run

Coarse fallback — 2 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json b9e9227e36d8964a60bb4e0614c1300bedd1fd51packageMentionDocs.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] two-factor-lockout.dogfood.test.ts still carries a private TOTP copy now that test/totp.ts exists

2 participants

@os-sam@claude