refactor authentication package to be maintainable - #29
Merged
Conversation
📝 WalkthroughWalkthroughThis PR extracts many auth runtime implementations from a monolithic runtime.ts into focused modules under packages/auth/src/runtime/* (context, cookies, secrets/hasher, request/response, optional-security, error mappers, result helpers) and moves form failure normalization into packages/forms/src/failure.ts, updating contracts, client imports, and tests. ChangesAuth Runtime Module Extraction
Forms Failure Module Extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (4)
packages/auth/src/runtime/context.ts (1)
71-74: 💤 Low valueInconsistent optional chaining on
*AccessToken/*RememberToken.
resolveContext()returns aMemoryAuthContext, which always definesgetAccessToken/setAccessToken/getRememberToken/setRememberToken(no question marks at lines 9-12). The?.()here suggests these may be absent, but they cannot be given the factory's contract. Either drop the optional chaining for clarity, or — if you intend the wrapper to also accept a leanerAuthRuntimeContextsomeday — make that explicit inresolveContext's return type.🤖 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 `@packages/auth/src/runtime/context.ts` around lines 71 - 74, The wrapper uses optional chaining when calling resolveContext().getAccessToken/setAccessToken/getRememberToken/setRememberToken, but resolveContext() returns a MemoryAuthContext that always defines those methods; remove the unnecessary ?. invocation so the lines become direct calls to resolveContext().getAccessToken(…) etc.; locate the four occurrences of getAccessToken, setAccessToken, getRememberToken and setRememberToken in this file and replace the optional-chained invocations with plain calls (or alternatively, if you intend to support a leaner AuthRuntimeContext, update resolveContext's return type to reflect optional methods—prefer removing the ?. for clarity given the current factory contract).packages/auth/src/runtime/secrets.ts (1)
9-31: 🏗️ Heavy liftDigest format omits cost parameters;
needsRehashcannot detect drift.The stored digest
scrypt$<saltHex>$<hashHex>does not encodeN/r/p, andneedsRehashalways returnsfalse. Two consequences:
- If Node's scrypt defaults ever change (or you intentionally raise cost),
verifywill silently fail for all existing hashes becauseawait scrypt(password, salt, expected.length)re-derives with whatever the current defaults are, producing a completely different output.- There is no path to roll forward to stronger parameters for already-stored credentials.
OWASP 2026 guidance recommends N=2^17 as the minimum; Node's default (N=2^14) is below that floor.
Consider a PHC-style format like
scrypt$N=16384,r=8,p=1$salt$hashand pin those values explicitly when callingscrypt. ThenneedsRehashcan returntruewhen the parsed params are weaker than the current target, enabling migration.🔧 Sketch of pinned-cost format
-const SCRYPT_PREFIX = 'scrypt' +const SCRYPT_PREFIX = 'scrypt' +const SCRYPT_PARAMS = { N: 2 ** 17, r: 8, p: 1 } as const +const SCRYPT_KEYLEN = 64 +const SCRYPT_OPTIONS = { ...SCRYPT_PARAMS, maxmem: 256 * 1024 * 1024 } @@ async hash(password: string): Promise<string> { const salt = randomBytes(16) - const derived = await scrypt(password, salt, 64) as Buffer - return `${SCRYPT_PREFIX}$${salt.toString('hex')}$${derived.toString('hex')}` + const derived = await scrypt(password, salt, SCRYPT_KEYLEN, SCRYPT_OPTIONS) as Buffer + const { N, r, p } = SCRYPT_PARAMS + return `${SCRYPT_PREFIX}$N=${N},r=${r},p=${p}$${salt.toString('hex')}$${derived.toString('hex')}` }, async verify(password: string, digest: string): Promise<boolean> { - const [prefix, saltHex, hashHex] = digest.split('$') - if (prefix !== SCRYPT_PREFIX || !saltHex || !hashHex) { + const parsed = parseScryptDigest(digest) + if (!parsed) { return false } - const salt = Buffer.from(saltHex, 'hex') - const expected = Buffer.from(hashHex, 'hex') - const derived = await scrypt(password, salt, expected.length) as Buffer + const derived = await scrypt(password, parsed.salt, parsed.expected.length, { + ...parsed.params, + maxmem: 256 * 1024 * 1024, + }) as Buffer - return derived.length === expected.length && timingSafeEqual(derived, expected) + return derived.length === parsed.expected.length && timingSafeEqual(derived, parsed.expected) }, - needsRehash() { - return false - }, + needsRehash(digest: string) { + const parsed = parseScryptDigest(digest) + if (!parsed) return true + return parsed.params.N < SCRYPT_PARAMS.N + || parsed.params.r < SCRYPT_PARAMS.r + || parsed.params.p < SCRYPT_PARAMS.p + },🤖 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 `@packages/auth/src/runtime/secrets.ts` around lines 9 - 31, The current createDefaultPasswordHasher stores digests as scrypt$<saltHex>$<hashHex> and calls scrypt without pinned cost options, so verify and needsRehash can't detect or enforce cost drift. Fix by encoding scrypt parameters in a PHC-style digest (e.g. scrypt$N=16384,r=8,p=1$<saltHex>$<hashHex>) inside the hash() method, call scrypt with explicit options (cost/N, r, p and keylen) when deriving in both hash() and verify(), update verify() to parse and use the embedded N/r/p when re-deriving, and implement needsRehash() to parse the stored params and return true when they are weaker than current target constants (compare parsed N/r/p to the desired values defined in the module, e.g. SCRYPT_TARGET_N/R/P).packages/auth/src/runtime/lifecycleFailures.ts (1)
58-94: ⚡ Quick winDuplicate
password_confirmation_mismatchmapping withsessionFailures.ts.The
password_confirmation_mismatchbranch (lines 63-76) is byte-for-byte identical to the corresponding case increateRegistrationFailureinpackages/auth/src/runtime/sessionFailures.ts(lines 63-76 there). Since this PR's stated objective is maintainability, consider extracting a shared helper (e.g.createPasswordConfirmationMismatchFailure) intofailureFields.tsand reusing it from both factories.♻️ Suggested shared helper
In
packages/auth/src/runtime/failureFields.ts:export function createPasswordConfirmationMismatchFailure< TCode extends AuthErrorCode, TInput extends Readonly<Record<string, unknown>>, >( code: TCode, message: string, input: TInput, ): AuthFailure<TCode, Partial<Record<InputFieldName<TInput>, readonly string[]>>> { const fields = [ pickInputField(input, ['password']), pickInputField(input, ['passwordConfirmation']), ].filter((field): field is InputFieldName<TInput> => typeof field === 'string') return createAuthFailurePayload( code, message, 422, createFieldErrors( fields.length > 0 ? fields : [resolveRequiredFieldName(input, ['password'])], message, ), ) }Then both call sites become a single line:
- case 'password_confirmation_mismatch': { - const message = error.message - const fields = [ - pickInputField(input, ['password']), - pickInputField(input, ['passwordConfirmation']), - ].filter((field): field is InputFieldName<TInput> => typeof field === 'string') - - return createAuthFailurePayload( - error.code, - message, - 422, - createFieldErrors(fields.length > 0 ? fields : [resolveRequiredFieldName(input, ['password'])], message), - ) - } + case 'password_confirmation_mismatch': + return createPasswordConfirmationMismatchFailure(error.code, error.message, input)🤖 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 `@packages/auth/src/runtime/lifecycleFailures.ts` around lines 58 - 94, Extract the duplicated logic in the password_confirmation_mismatch case into a shared helper named createPasswordConfirmationMismatchFailure (e.g. in failureFields.ts) that accepts (code, message, input) and uses pickInputField, resolveRequiredFieldName, createFieldErrors and createAuthFailurePayload to build the AuthFailure; then replace the password_confirmation_mismatch branches in createPasswordResetConsumeFailure and createRegistrationFailure with a single call to createPasswordConfirmationMismatchFailure(error.code, message, input). Ensure the helper returns AuthFailure with the same generic signature so both call sites compile unchanged.packages/auth/src/runtime/optionalSecurity.ts (1)
34-49: ⚡ Quick winUse
error.codeinstead of message matching for detecting missing packages.Matching by
error.messagesubstring is brittle across runtimes and Node versions. Node.js setserror.codeto'ERR_MODULE_NOT_FOUND'(ESM) or'MODULE_NOT_FOUND'(CommonJS) as the documented stable signal for missing modules. Check the code first, then fall back to message matching for runtimes that don't set it.♻️ Suggested change
function isMissingOptionalPackageError(error: unknown): boolean { if (!(error instanceof Error)) { return false } + const code = (error as NodeJS.ErrnoException).code + if (code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND') { + return true + } + const message = error.message const mentionsSecurityPackage = message.includes('@holo-js/security') return mentionsSecurityPackage && ( message.includes('Cannot find package') || message.includes('Cannot find module') || message.includes('Failed to resolve module specifier') || message.includes('Failed to load url') || message.includes('Could not resolve') ) }🤖 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 `@packages/auth/src/runtime/optionalSecurity.ts` around lines 34 - 49, Update isMissingOptionalPackageError to first narrow to Error and then check error.code for 'ERR_MODULE_NOT_FOUND' or 'MODULE_NOT_FOUND' (returning true if the code matches and the message mentions '@holo-js/security'); if error.code is absent, fall back to the existing substring checks on error.message (mentions of '@holo-js/security' plus the previous message patterns). Keep the function name isMissingOptionalPackageError and preserve the original message-based checks as the fallback path.
🤖 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 `@packages/auth/src/runtime/context.ts`:
- Around line 50-66: The code currently lazy-initializes the AsyncLocalStorage
via storage.enterWith in resolveContext which can leak contexts; stop lazy-init
and make activate the explicit initializer: remove storage.enterWith(created)
from resolveContext and have resolveContext throw (or return a clear error) when
storage.getStore() is missing, then change activate() to create a
MemoryAuthContext and call storage.enterWith(created) (so callers must call
activate before using any getters). Update any getters that call resolveContext
to rely on that throw when the store is missing so the contract is explicit
(functions to touch: createAsyncAuthContext, resolveContext, activate, and any
getters that use resolveContext).
In `@packages/auth/src/runtime/cookieSerialization.ts`:
- Around line 28-30: The current guard in cookieSerialization that only pushes
`Max-Age` when (options.maxAge ?? 0) > 0 drops `Max-Age=0` used by forgetCookie
to delete cookies; change the condition so `Max-Age` is emitted for zero and
positive values (i.e. treat undefined or negative as “skip”, but allow 0).
Update the check around attributes.push(`Max-Age=${options.maxAge}`) to
explicitly test for maxAge !== undefined && maxAge >= 0 (or equivalent) so
forgetCookie in responseCookies.ts gets `Max-Age=0`.
In `@packages/auth/src/runtime/setCookieParser.ts`:
- Around line 86-89: parseSetCookieDefinition currently calls decodeURIComponent
on nameValue.slice(0, separator) which can throw URIError for malformed %
sequences and break the function's null-return contract; wrap the
decodeURIComponent call in a try/catch (or call a safe-decode helper) inside
parseSetCookieDefinition so that any decode errors cause the function to return
null instead of throwing, preserving the existing behavior for malformed headers
and ensuring callers can rely on a null result.
---
Nitpick comments:
In `@packages/auth/src/runtime/context.ts`:
- Around line 71-74: The wrapper uses optional chaining when calling
resolveContext().getAccessToken/setAccessToken/getRememberToken/setRememberToken,
but resolveContext() returns a MemoryAuthContext that always defines those
methods; remove the unnecessary ?. invocation so the lines become direct calls
to resolveContext().getAccessToken(…) etc.; locate the four occurrences of
getAccessToken, setAccessToken, getRememberToken and setRememberToken in this
file and replace the optional-chained invocations with plain calls (or
alternatively, if you intend to support a leaner AuthRuntimeContext, update
resolveContext's return type to reflect optional methods—prefer removing the ?.
for clarity given the current factory contract).
In `@packages/auth/src/runtime/lifecycleFailures.ts`:
- Around line 58-94: Extract the duplicated logic in the
password_confirmation_mismatch case into a shared helper named
createPasswordConfirmationMismatchFailure (e.g. in failureFields.ts) that
accepts (code, message, input) and uses pickInputField,
resolveRequiredFieldName, createFieldErrors and createAuthFailurePayload to
build the AuthFailure; then replace the password_confirmation_mismatch branches
in createPasswordResetConsumeFailure and createRegistrationFailure with a single
call to createPasswordConfirmationMismatchFailure(error.code, message, input).
Ensure the helper returns AuthFailure with the same generic signature so both
call sites compile unchanged.
In `@packages/auth/src/runtime/optionalSecurity.ts`:
- Around line 34-49: Update isMissingOptionalPackageError to first narrow to
Error and then check error.code for 'ERR_MODULE_NOT_FOUND' or 'MODULE_NOT_FOUND'
(returning true if the code matches and the message mentions
'@holo-js/security'); if error.code is absent, fall back to the existing
substring checks on error.message (mentions of '@holo-js/security' plus the
previous message patterns). Keep the function name isMissingOptionalPackageError
and preserve the original message-based checks as the fallback path.
In `@packages/auth/src/runtime/secrets.ts`:
- Around line 9-31: The current createDefaultPasswordHasher stores digests as
scrypt$<saltHex>$<hashHex> and calls scrypt without pinned cost options, so
verify and needsRehash can't detect or enforce cost drift. Fix by encoding
scrypt parameters in a PHC-style digest (e.g.
scrypt$N=16384,r=8,p=1$<saltHex>$<hashHex>) inside the hash() method, call
scrypt with explicit options (cost/N, r, p and keylen) when deriving in both
hash() and verify(), update verify() to parse and use the embedded N/r/p when
re-deriving, and implement needsRehash() to parse the stored params and return
true when they are weaker than current target constants (compare parsed N/r/p to
the desired values defined in the module, e.g. SCRYPT_TARGET_N/R/P).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afa38d08-ca39-4105-8a19-d805d8b7f94b
📒 Files selected for processing (17)
packages/auth/src/runtime.tspackages/auth/src/runtime/context.tspackages/auth/src/runtime/cookieSerialization.tspackages/auth/src/runtime/expectedErrors.tspackages/auth/src/runtime/failureFields.tspackages/auth/src/runtime/lifecycleFailures.tspackages/auth/src/runtime/optionalSecurity.tspackages/auth/src/runtime/requestAccess.tspackages/auth/src/runtime/responseCookies.tspackages/auth/src/runtime/result.tspackages/auth/src/runtime/secrets.tspackages/auth/src/runtime/sessionFailures.tspackages/auth/src/runtime/setCookieParser.tspackages/forms/src/client.tspackages/forms/src/contracts.tspackages/forms/src/failure.tspackages/forms/tests/contracts.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/auth/src/runtime/failureFields.ts (2)
30-32: 💤 Low valueInner
[message]arrays increateFieldErrorsare not frozen
Object.freezeis shallow. The per-field[message]arrays are mutable at runtime despite thereadonly string[]type annotation. If downstream code stores a reference and pushes to it, the cached error payload is silently mutated.Object.freezeeach inner array alongside the outer object to honour the immutability intent.🔧 Proposed fix
return Object.freeze( - Object.fromEntries(fields.map(field => [field, [message] as readonly string[]])) as AuthFieldErrors<TField>, + Object.fromEntries(fields.map(field => [field, Object.freeze([message]) as readonly string[]])) as AuthFieldErrors<TField>, )🤖 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 `@packages/auth/src/runtime/failureFields.ts` around lines 30 - 32, The per-field message arrays created in createFieldErrors (the Object.fromEntries(...) call) are mutable because Object.freeze is applied only to the outer object; modify the construction so each inner array is frozen as well (e.g., create a frozen array for each field like Object.freeze([message]) before placing it into the entries), then freeze the resulting object as currently done; ensure the returned value is typed as AuthFieldErrors<TField> and remains readonly.
5-10: 💤 Low value
inoperator traverses the prototype chain
field in inputreturnstruefor prototype-inherited properties such asconstructor,toString, andhasOwnProperty. If a caller passes a crafted field name that collides withObject.prototype,hasInputFieldwould incorrectly report the field as present.Object.prototype.hasOwnProperty.call(input, field)restricts the check to own enumerable properties, which is the intended semantic here.🔧 Proposed fix
export function hasInputField<TInput extends Readonly<Record<string, unknown>>>( input: TInput, field: string, ): field is InputFieldName<TInput> { - return field in input + return Object.prototype.hasOwnProperty.call(input, field) }🤖 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 `@packages/auth/src/runtime/failureFields.ts` around lines 5 - 10, The hasInputField type-guard currently uses the `in` operator which traverses the prototype chain and can return true for inherited names like "constructor"; change the check in hasInputField to use Object.prototype.hasOwnProperty.call(input, field) so only own properties are detected, preserving the function signature and type-narrowing behavior for InputFieldName<TInput>.packages/auth/src/runtime/scryptPasswordHasher.ts (1)
16-35: 💤 Low valueOptional: harden
parseScryptParamsagainst string forms of NaN.
Number('NaN')returnsNaN, which hastypeof === 'number', butNumber.isSafeInteger(NaN)isfalse, so the existing guard already rejects it. This is fine today — just calling it out so the order of checks isn't accidentally flipped later. Consider replacingNumber(rawValue)withNumber.parseInt(rawValue, 10)for clearer numeric parsing intent.🤖 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 `@packages/auth/src/runtime/scryptPasswordHasher.ts` around lines 16 - 35, parseScryptParams currently uses Number(rawValue) which can produce NaN strings and is less explicit about integer parsing; update parseScryptParams to parse N, r, p using Number.parseInt(rawValue, 10) (or an equivalent integer-parsing step), trim entry and key strings before parsing, and preserve the subsequent typeof/Number.isSafeInteger and positive checks so NaN/invalid values are still rejected; reference the parseScryptParams function and the N, r, p parameter handling when making the change.
🤖 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 `@packages/auth/src/runtime/failureFields.ts`:
- Around line 22-23: pickInputField currently falls back to the first key of
input and uses hasInputField redundantly; update pickInputField so it only
returns a field when it matches one of the provided candidates and otherwise
returns undefined (do not return Object.keys(input)[0]). Remove the redundant
hasInputField guard around that fallback and ensure the function checks
membership against the candidates list (referencing pickInputField, candidates,
and input) so callers receive undefined when no candidate is present.
In `@packages/auth/src/runtime/scryptPasswordHasher.ts`:
- Around line 37-85: The parseScryptDigest function is too permissive:
Buffer.from(..., 'hex') silently accepts non-hex input which can produce
zero-length buffers and cause verify to return true; update parseScryptDigest to
strictly validate that saltHex and hashHex are non-empty and contain only hex
characters (e.g. /^[0-9a-f]+$/i) and for non-legacy digests also validate that
hashHex length corresponds to an expected key length (or at least > 0); adjust
the verify path in createScryptPasswordHasher.verify to treat any parsed result
with an empty salt or empty expected hash as invalid (return false) if you
prefer a defensive check there too. Ensure you reference parseScryptDigest,
createScryptPasswordHasher.verify, and deriveScryptKey when making these checks.
---
Nitpick comments:
In `@packages/auth/src/runtime/failureFields.ts`:
- Around line 30-32: The per-field message arrays created in createFieldErrors
(the Object.fromEntries(...) call) are mutable because Object.freeze is applied
only to the outer object; modify the construction so each inner array is frozen
as well (e.g., create a frozen array for each field like
Object.freeze([message]) before placing it into the entries), then freeze the
resulting object as currently done; ensure the returned value is typed as
AuthFieldErrors<TField> and remains readonly.
- Around line 5-10: The hasInputField type-guard currently uses the `in`
operator which traverses the prototype chain and can return true for inherited
names like "constructor"; change the check in hasInputField to use
Object.prototype.hasOwnProperty.call(input, field) so only own properties are
detected, preserving the function signature and type-narrowing behavior for
InputFieldName<TInput>.
In `@packages/auth/src/runtime/scryptPasswordHasher.ts`:
- Around line 16-35: parseScryptParams currently uses Number(rawValue) which can
produce NaN strings and is less explicit about integer parsing; update
parseScryptParams to parse N, r, p using Number.parseInt(rawValue, 10) (or an
equivalent integer-parsing step), trim entry and key strings before parsing, and
preserve the subsequent typeof/Number.isSafeInteger and positive checks so
NaN/invalid values are still rejected; reference the parseScryptParams function
and the N, r, p parameter handling when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e80f48b2-a949-477c-b25d-5ff09d647fc4
📒 Files selected for processing (10)
packages/auth/src/runtime/context.tspackages/auth/src/runtime/cookieSerialization.tspackages/auth/src/runtime/failureFields.tspackages/auth/src/runtime/lifecycleFailures.tspackages/auth/src/runtime/optionalSecurity.tspackages/auth/src/runtime/scryptPasswordHasher.tspackages/auth/src/runtime/secrets.tspackages/auth/src/runtime/sessionFailures.tspackages/auth/src/runtime/setCookieParser.tspackages/auth/tests/package.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/auth/src/runtime/cookieSerialization.ts
- packages/auth/src/runtime/setCookieParser.ts
- packages/auth/src/runtime/optionalSecurity.ts
- packages/auth/src/runtime/sessionFailures.ts
- packages/auth/src/runtime/lifecycleFailures.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/auth/src/runtime/failureFields.ts (1)
86-88: ⚡ Quick win
resolveRequiredFieldNameerror message omits the candidate list, making failures hard to diagnoseWhen this throws in production (e.g., via the
createPasswordConfirmationMismatchFailurefallback path), the message'Expected auth failure mapping to resolve at least one input field.'gives no clue which candidates were tried or what the actual input keys were.🔧 Proposed fix — include candidates in the error message
- throw new Error('[`@holo-js/auth`] Expected auth failure mapping to resolve at least one input field.') + throw new Error( + `[`@holo-js/auth`] Expected auth failure mapping to resolve at least one input field. ` + + `Tried candidates: [${candidates.join(', ')}]. Available fields: [${Object.keys(input).join(', ')}].` + )🤖 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 `@packages/auth/src/runtime/failureFields.ts` around lines 86 - 88, The error thrown in resolveRequiredFieldName currently omits which candidate names were tried and the actual input keys, making diagnostics hard; update the thrown Error inside resolveRequiredFieldName to include the candidates list and the input's keys (or a short serialized representation) so the message shows which candidate names were evaluated and what keys were present (reference resolveRequiredFieldName and callers like createPasswordConfirmationMismatchFailure to locate the failing path), ensuring the message remains concise and safe to log.
🤖 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 `@packages/core/src/portable/holo.ts`:
- Around line 3951-3959: The authRuntime is used in callbacks
(resolveDefaultActor, resolveGuardActor) and exposed via runtime.auth without
activating the async auth context, causing runtime errors; fix by wrapping
authRuntime calls so authContext.activate() runs before any method invocation:
in the configureAuthorizationAuthIntegration call replace direct uses of
authRuntime.user() and authRuntime.guard(...).user() with wrapper functions that
first call authContext.activate() then delegate to authRuntime, and similarly
change the public runtime.auth getter to return a wrapped object whose methods
call authContext.activate() before delegating to the original authRuntime
methods.
---
Nitpick comments:
In `@packages/auth/src/runtime/failureFields.ts`:
- Around line 86-88: The error thrown in resolveRequiredFieldName currently
omits which candidate names were tried and the actual input keys, making
diagnostics hard; update the thrown Error inside resolveRequiredFieldName to
include the candidates list and the input's keys (or a short serialized
representation) so the message shows which candidate names were evaluated and
what keys were present (reference resolveRequiredFieldName and callers like
createPasswordConfirmationMismatchFailure to locate the failing path), ensuring
the message remains concise and safe to log.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aff2b1dc-deea-44a5-9975-e7c4037d8cdb
📒 Files selected for processing (7)
packages/auth/src/runtime.tspackages/auth/src/runtime/context.tspackages/auth/src/runtime/failureFields.tspackages/auth/src/runtime/scryptPasswordHasher.tspackages/auth/tests/package.test.tspackages/core/src/portable/holo.tspackages/core/tests/runtime.test.ts
💤 Files with no reviewable changes (1)
- packages/core/tests/runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/auth/src/runtime/context.ts
- packages/auth/src/runtime/scryptPasswordHasher.ts
- packages/auth/tests/package.test.ts
- packages/auth/src/runtime.ts
Comment on lines
+3951
to
+3959
| const authRuntime = authModule.getAuthRuntime() | ||
|
|
||
| if (authorizationModule) { | ||
| authorizationModule.authorizationInternals.configureAuthorizationAuthIntegration({ | ||
| hasGuard(guardName: string) { | ||
| return guardName in loadedConfig.auth.guards | ||
| }, | ||
| resolveDefaultActor: async () => boundAuthRuntime.user(), | ||
| resolveGuardActor: async (guardName: string) => boundAuthRuntime.guard(guardName).user(), | ||
| resolveDefaultActor: async () => authRuntime.user(), | ||
| resolveGuardActor: async (guardName: string) => authRuntime.guard(guardName).user(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Async auth context contract =="
sed -n '1,220p' packages/auth/src/runtime/context.ts
echo
echo "== Places that call or expose the raw auth runtime =="
rg -n -C3 'getAuthRuntime\(\)|resolveDefaultActor|resolveGuardActor|get auth\(' packages/core/src/portable/holo.ts
echo
echo "== Auth-context activation sites in core =="
rg -n -C2 '\.activate\(' packages/core/src/portable/holo.ts packages/auth/src/runtime/context.tsRepository: cobraprojects/holo-js
Length of output: 5616
Wrap authRuntime to activate context before method calls, or activate authContext before exposing/using authRuntime.
The authorization actor resolvers (resolveDefaultActor, resolveGuardActor) and the public runtime.auth getter will fail at runtime because the async auth context is never activated. When these callbacks execute or when downstream code accesses holo.auth.*, the underlying context access will throw: "Async auth context is not active. Call activate() before reading or writing auth context state." Fix by ensuring authContext.activate() runs before any auth runtime method invocations.
🤖 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 `@packages/core/src/portable/holo.ts` around lines 3951 - 3959, The authRuntime
is used in callbacks (resolveDefaultActor, resolveGuardActor) and exposed via
runtime.auth without activating the async auth context, causing runtime errors;
fix by wrapping authRuntime calls so authContext.activate() runs before any
method invocation: in the configureAuthorizationAuthIntegration call replace
direct uses of authRuntime.user() and authRuntime.guard(...).user() with wrapper
functions that first call authContext.activate() then delegate to authRuntime,
and similarly change the public runtime.auth getter to return a wrapped object
whose methods call authContext.activate() before delegating to the original
authRuntime methods.
Owner
Author
There was a problem hiding this comment.
@CodeRabbit
This finding is stale. @holo-js/core intentionally no longer wraps auth runtime methods. @holo-js/auth owns async context activation through getRuntimeBindings(), which calls context.activate() before returning runtime bindings. Auth facade methods such as user(), guard(...).user(), check(), login(), etc. all go through getRuntimeBindings() before reading context state. Reintroducing a core wrapper would duplicate auth runtime behavior in core and undo the intended abstraction boundary. Only flag missing activation if a method reads bindings.context without first going through getRuntimeBindings() or an equivalent auth-owned activation path.
There was a problem hiding this comment.
@cobraprojects, thank you for the clarification — you're right, and I apologize for the noise. I was unaware that @holo-js/auth's getRuntimeBindings() internally calls context.activate() before surfacing bindings, making core-level wrapping both unnecessary and contrary to the intended abstraction boundary. I'll make sure not to flag this pattern again.
✏️ Learnings added
Learnt from: cobraprojects
Repo: cobraprojects/holo-js PR: 29
File: packages/core/src/portable/holo.ts:3951-3959
Timestamp: 2026-05-09T00:16:10.347Z
Learning: In the holo-js monorepo (`packages/core/src/portable/holo.ts` and `packages/auth/src/runtime.ts`), `holo-js/auth` owns async auth context activation. All auth facade methods (user(), guard(...).user(), check(), login(), etc.) go through an internal `getRuntimeBindings()` helper that calls `context.activate()` before returning runtime bindings. `holo-js/core` intentionally does NOT wrap auth runtime methods to activate the context — doing so would duplicate behaviour and violate the abstraction boundary. Only flag missing activation if a method reads `bindings.context` directly without first going through `getRuntimeBindings()` or an equivalent auth-owned activation path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This was referenced May 11, 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.
Summary by CodeRabbit
Refactor
New Features
Tests