add workos logic#35
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds hosted WorkOS auth across the repo: new typed contracts and facade methods, large refactor of auth-workos core (request normalization, profile/session shaping, mapper-driven identity sync), config/type normalization with provider selection, framework route handlers, UI links/forms, CLI scaffold changes, docs, and tests. ChangesWorkOS Authentication Integration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 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 unit tests (beta)
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 (12)
tests/example-app-auth-flow.mjs (1)
293-295: ⚡ Quick winAlso assert the callback redirect includes an
errorquery param.Right now this only validates
/loginpath, so a redirect without error context would still pass.🔧 Suggested test tightening
- assertRedirectsTo(await fetchAuthText('/api/auth/workos/callback', { - allowFailure: true, - }), '/login') + const workosCallback = await fetchAuthText('/api/auth/workos/callback', { + allowFailure: true, + }) + assertRedirectsTo(workosCallback, '/login') + const workosCallbackLocation = workosCallback.response.headers.get('location') + assert.ok(workosCallbackLocation, 'Expected WorkOS callback redirect to include a location header.') + const workosCallbackUrl = new URL(workosCallbackLocation, workosCallback.response.url) + assert.ok( + workosCallbackUrl.searchParams.get('error'), + 'Expected WorkOS callback failure redirect to include an error query parameter.', + )🤖 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 `@tests/example-app-auth-flow.mjs` around lines 293 - 295, The test currently only checks the path via assertRedirectsTo(await fetchAuthText('/api/auth/workos/callback', { allowFailure: true }), '/login'); update it to also assert the redirect includes an error query param: either tighten the expected redirect to include an error query (e.g. '/login?error=...') or fetch the Location/redirect URL from fetchAuthText's response and assert that new URL includes an "error" query key; look for and update the call to assertRedirectsTo and/or add a follow-up assertion after fetchAuthText('/api/auth/workos/callback', ...) to verify the presence of the error query param.packages/auth-workos/src/index.ts (6)
1107-1116: ⚡ Quick win
getStringFieldchecksvalue.trim()but returns the untrimmed value.When a WorkOS response includes a padded value like
" org_123 ", this helper returns it verbatim becausevalue.trim()is only used as a truthiness guard. That untrimmed value then flows intosessionId,accessToken, andorganizationIdderivation inauthenticateWorkosCode. Returning the trimmed value would keep the rest of the pipeline working with clean strings.♻️ Suggested fix
function getStringField(input: Readonly<Record<string, unknown>>, ...names: readonly string[]): string | undefined { for (const name of names) { const value = input[name] - if (typeof value === 'string' && value.trim()) { - return value - } + if (typeof value === 'string' && value.trim()) { + return value.trim() + } } return undefined }🤖 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-workos/src/index.ts` around lines 1107 - 1116, getStringField currently uses value.trim() only as a truthiness check but returns the original untrimmed string, causing padded values to propagate into sessionId, accessToken, and organizationId in authenticateWorkosCode; change getStringField (the function in packages/auth-workos/src/index.ts) to return the trimmed string (value.trim()) when returning a string so callers receive cleaned values, preserving the existing truthiness logic and types.
1528-1530: ⚡ Quick winModule-level caches survive
resetWorkosAuthRuntime.
workosDefaultProviderRuntimeCache(Line 125) andworkosJwksCache(Line 126) are populated lazily on first verification and are never cleared byresetWorkosAuthRuntime. In long-running tests or hot-reload scenarios where the sameclientId/apiKeyare re-used after a reset but the underlying keys/runtimes change, this can serve staleWorkosProviderRuntimeor stale JWKS. Consider clearing both caches insideresetWorkosAuthRuntimeso reset truly returns the module to a pristine state.♻️ Suggested fix
export function resetWorkosAuthRuntime(): void { getRuntimeState().bindings = undefined + workosDefaultProviderRuntimeCache.clear() + workosJwksCache.clear() }🤖 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-workos/src/index.ts` around lines 1528 - 1530, resetWorkosAuthRuntime currently only clears getRuntimeState().bindings but leaves module-level caches populated; modify resetWorkosAuthRuntime to also clear the module-level caches workosDefaultProviderRuntimeCache and workosJwksCache (e.g., call .clear() or reassign to a fresh Map/undefined as appropriate) so that any cached WorkosProviderRuntime or JWKS entries are removed and the module returns to a pristine state.
446-497: 💤 Low value
normalizeWorkosUserProfilereadsuser.emailas-is (Line 460) and stores it on the frozen profile. Downstream consumers (resolveEmailForCreation,syncIdentity,ensureNoUnexpectedEmailCollision) re-trim it everywhere, so the data isn't broken — but it leaves the canonical profile holding values like' user@app.test 'if a provider ever returns padded strings. Normalizing once at the boundary (similar to howfirstName/lastNameare passed through) would centralize the contract and let the rest of the module stop defensively trimming.🤖 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-workos/src/index.ts` around lines 446 - 497, The profile builder normalizeWorkosUserProfile currently assigns email directly from user.email which can contain leading/trailing whitespace; update it to trim the value once at the boundary (e.g., compute const email = typeof user.email === 'string' ? user.email.trim() : '') and then use that trimmed email everywhere in the returned WorkosIdentityProfile (including name fallback logic) so downstream functions can rely on a canonical, trimmed email.
134-142: 💤 Low valueDefining the runtime state with
configurable: falseand unwritable value blocks legitimate cleanup.
Object.definePropertyhere leaveswritableandconfigurableboth at their non-strict defaults offalse, so once the property is set onglobalThisit cannot be redefined or deleted, and the slot cannot be reassigned. The current code only mutatesstate.bindings, so it works, but any future attempt to swap the state object (e.g., to share with another module instance during HMR or test isolation) will throw. Either setwritable: true, configurable: trueor just use a plain assignment to the symbol-like key — the privacy benefit ofenumerable: falsealone is sufficient.♻️ Suggested fix
if (!runtimeGlobal[WORKOS_RUNTIME_STATE_KEY]) { Object.defineProperty(runtimeGlobal, WORKOS_RUNTIME_STATE_KEY, { value: {}, enumerable: false, - configurable: false, + configurable: true, + writable: true, }) }🤖 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-workos/src/index.ts` around lines 134 - 142, The property defined on runtimeGlobal using Object.defineProperty with WORKOS_RUNTIME_STATE_KEY is non-configurable and non-writable by default, preventing redefinition or deletion; update the definition in the module that creates the runtime state (the code around runtimeGlobal and WORKOS_RUNTIME_STATE_KEY, likely in the get runtime initializer) to either (a) set writable: true and configurable: true along with enumerable: false, or (b) replace the Object.defineProperty call with a plain assignment runtimeGlobal[WORKOS_RUNTIME_STATE_KEY] = {} while keeping enumerable: false behavior via a Symbol-style key; reference WORKOS_RUNTIME_STATE_KEY and runtimeGlobal to locate the change and ensure subsequent code that mutates state.bindings continues to work.
1417-1457: ⚡ Quick win
getAuthRuntime().guard(guard).user()is awaited but its result is unused.Inside
logoutWithWorkos, Line 1433 awaits.user()immediately before reading the WorkOS session payload, yet the returned user is never consumed. If the intent is just to hydratebindings.context/load the session, that side effect is already covered bysetSessionIdon Line 1431 and the directbindings.session.readinsidereadCurrentWorkosLogoutSession. If.user()is intentional (e.g., to surface auth/session errors before logout), a comment would help; otherwise the call can be dropped to avoid the extra adapter round-trip and the risk of mapping a non-WorkOS session error toworkos_logout_failedinstead ofworkos_session_missing.🤖 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-workos/src/index.ts` around lines 1417 - 1457, The await of getAuthRuntime().guard(guard).user() inside logoutWithWorkos is unused and causes an unnecessary adapter round-trip and potential misleading error mapping; remove that call (the await getAuthRuntime().guard(guard).user() line) unless you intentionally want to surface session/auth errors—if you do, instead keep it but wrap it in its own try/catch that returns a distinct error code (or adds a comment) so readCurrentWorkosLogoutSession and setSessionId remain the primary session checks (references: logoutWithWorkos, getAuthRuntime().guard(guard).user(), setSessionId, readCurrentWorkosLogoutSession).
564-596: 💤 Low value
bindings.config.workos.provideraccess still relies on the loose index signature.Because
NormalizedHoloAuthWorkosConfigwidens theproviderselector toNormalizedAuthWorkosProviderConfig | string | undefined, thetypeof === 'string'check on Line 571 is genuinely necessary at the type level, but it also means readers can't tell at a glance whether the selector ever overlaps with provider keys. If a provider entry is ever namedprovider,getWorkosProviderEntriesinpackages/config/src/defaults.tsfilters it out, but the runtime here would still resolve'provider'as a valid name later via the configured-provider branches. Worth either documenting the reserved key or rejectingprovideras a provider name during normalization to keep the two layers consistent.🤖 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-workos/src/index.ts` around lines 564 - 596, The runtime ambiguity comes from allowing a provider entry named "provider" in bindings.config.workos while resolveConfiguredProviderName also reads bindings.config.workos.provider; fix this by reserving the "provider" key at normalization time: update the NormalizedHoloAuthWorkosConfig creation (and getWorkosProviderEntries in packages/config/src/defaults.ts) to either filter out or throw on any provider entry whose key === 'provider' so provider names cannot collide, and then keep resolveConfiguredProviderName as-is (it can safely treat bindings.config.workos.provider as the reserved default string); ensure tests/validation reflect the new reserved-key rule.packages/auth-workos/tests/package.test.ts (1)
858-934: 💤 Low valueBody assertion relies on
JSON.stringifydropping undefined fields.The hosted callback fetch mock asserts
init?.bodyequals a JSON payload containing onlygrant_type,client_id,client_secret, andcode. The runtime inauthenticateWorkosCodealso addsip_addressanduser_agent, which only get dropped from the serialized body because the testRequesthas nox-forwarded-for/x-real-ip/user-agentheaders, leaving those fieldsundefined. This makes the assertion brittle — adding default headers anywhere upstream (e.g., a test util settinguser-agent) would break this exact-string match without any behavioral regression. Consider parsing the JSON and usingtoMatchObject(orexpect.objectContaining) for the four fields you actually care about.🤖 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-workos/tests/package.test.ts` around lines 858 - 934, The test's fetch mock currently asserts init?.body equals a specific JSON string, which is brittle because authenticateWorkosCode may include undefined ip_address/user_agent fields; update the assertion to parse the body (JSON.parse(String(init?.body))) and assert only the required keys using toMatchObject or expect.objectContaining for { grant_type: 'authorization_code', client_id: 'workos-client', client_secret: 'workos-key', code: 'code_123' } — change the assertion inside the vi.stubGlobal fetch in the "completes the hosted WorkOS callback with typed user mapping" test so it verifies the parsed body contains those fields rather than matching the exact JSON string.packages/auth-workos/src/contracts.ts (1)
117-122: ⚡ Quick win
WorkosAuthFacade.completeWorkosAuthreturn type is much looser than the implementation.The implementation in
packages/auth-workos/src/index.tsreturns a precise discriminated union (frozen objects withok: truecarryingprovider,guard,authProvider,status,user,identity,session,authSession, orok: falsewithcode/message). The facade contract here erases all of that to{ ok: boolean; code?: string; message?: string; user?: AuthUserLike }, so consumers typed against the facade lose access toidentity,authSession,session,status, etc., and cannot narrow onok. Mirroring the implementation’s discriminated union (or exporting it as a named type and referencing it here) would prevent the facade from being a typing downgrade for callers.🤖 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-workos/src/contracts.ts` around lines 117 - 122, The public contract for WorkosAuthFacade.completeWorkosAuth is too permissive and loses the implementation's discriminated union; update the return type to mirror the precise frozen discriminated union used in packages/auth-workos/src/index.ts (or export that union as a named type and reference it here) so callers can narrow on ok and access provider/guard/authProvider/status/identity/session/authSession/user; specifically change the signature of WorkosAuthFacade.completeWorkosAuth to return that exported union type instead of the current loose { ok: boolean; code?: string; message?: string; user?: AuthUserLike } shape.packages/config/src/defaults.ts (2)
1505-1535: ⚡ Quick winVariable shadowing in
normalizeWorkosConfigmakes the logic harder to follow.The outer
provideron Line 1511 (the selected provider name) is shadowed by the destructuredproviderparameter (anAuthWorkosProviderConfig) inside the.mapcallbacks on Lines 1520 and 1530. While the outer reference on Line 1524 still resolves correctly because the innerprovideris parameter-scoped, the dual meaning ofprovider(selector name vs. provider config object) within the same function is easy to misread.♻️ Suggested rename
- const providerEntries = getWorkosProviderEntries(config) - const provider = config?.provider?.trim() || undefined + const providerEntries = getWorkosProviderEntries(config) + const selectedProvider = config?.provider?.trim() || undefined if (providerEntries.length === 0) { - if (provider) { - throw new Error(`[Holo Auth] WorkOS provider "${provider}" is not configured.`) + if (selectedProvider) { + throw new Error(`[Holo Auth] WorkOS provider "${selectedProvider}" is not configured.`) } return holoAuthDefaults.workos } - const normalizedEntries = providerEntries.map(([name, provider]) => { + const normalizedEntries = providerEntries.map(([name, providerConfig]) => { const normalizedName = normalizeConnectionName(name, 'Auth WorkOS provider name') - return [normalizedName, provider] as const + return [normalizedName, providerConfig] as const }) - if (provider && !normalizedEntries.some(([name]) => name === provider)) { - throw new Error(`[Holo Auth] WorkOS provider "${provider}" is not configured.`) + if (selectedProvider && !normalizedEntries.some(([name]) => name === selectedProvider)) { + throw new Error(`[Holo Auth] WorkOS provider "${selectedProvider}" is not configured.`) } return Object.freeze({ - ...(typeof provider === 'undefined' ? {} : { provider }), - ...Object.fromEntries(normalizedEntries.map(([name, provider]) => [ + ...(typeof selectedProvider === 'undefined' ? {} : { provider: selectedProvider }), + ...Object.fromEntries(normalizedEntries.map(([name, providerConfig]) => [ name, - normalizeWorkosProvider(name, provider, guards, providers), + normalizeWorkosProvider(name, providerConfig, guards, providers), ])), })🤖 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/config/src/defaults.ts` around lines 1505 - 1535, The function normalizeWorkosConfig uses an outer variable named provider (selected provider name) and also destructures a provider parameter inside the providerEntries.map callbacks, causing shadowing and confusion; rename the inner mapped variable (e.g., to providerConfig or entryProvider) in the providerEntries.map that builds normalizedEntries and in the Object.fromEntries mapping so that calls to normalizeWorkosProvider use the new name (normalizeWorkosProvider(name, providerConfig, guards, providers)), and ensure all references in those callbacks are updated accordingly to avoid shadowing while preserving the same behavior.
1480-1503: 💤 Low valueType guard rejects string entries that the
HoloAuthWorkosConfigtype explicitly permits.
HoloAuthWorkosConfigdeclaresreadonly [provider: string]: AuthWorkosProviderConfig | string | undefined, butisWorkosProviderConfigreturnsfalsefor strings andgetWorkosProviderEntriesthen throws"WorkOS provider \"${name}\" must be an object.". If string values are intentionally not supported in normalized config, consider tightening the type (e.g., dropping| stringfrom the index signature intypes.ts) so the contract matches the runtime behavior; otherwise, this rejection is a behavior gap. Either way, the error message could mention the allowed shapes more accurately.🤖 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/config/src/defaults.ts` around lines 1480 - 1503, The type guard isWorkosProviderConfig currently rejects string entries while HoloAuthWorkosConfig permits them, causing getWorkosProviderEntries to throw incorrectly; either update the runtime to accept strings (e.g., change isWorkosProviderConfig to treat typeof value === 'object' || typeof value === 'string' and in getWorkosProviderEntries normalize string shorthand into an AuthWorkosProviderConfig object before pushing) or tighten the static type (remove | string from HoloAuthWorkosConfig) so types and runtime align, and update the thrown Error message in getWorkosProviderEntries to list the allowed shapes (object or string) when rejecting values.packages/config/src/types.ts (1)
628-707: ⚖️ Poor tradeoffIndex signature widening loses provider-config typing for known WorkOS entries.
Both
HoloAuthWorkosConfigandNormalizedHoloAuthWorkosConfiguse[provider: string]: AuthWorkosProviderConfig | string | undefinedso theprovider?: stringselector is structurally compatible. As a side effect, every provider lookup (e.g.,config.workos.dashboard) now widens toAuthWorkosProviderConfig | string | undefined, forcing consumers (likegetConfiguredProviderConfiginpackages/auth-workos/src/index.ts) to defensively narrow withtypeof configured !== 'object'even though the runtime never accepts string entries. If string values aren't actually supported, dropping| stringand keeping onlyAuthWorkosProviderConfig | undefinedwould let TS rejectprovider: stringcleanly via a discriminated wrapper such as:export type HoloAuthWorkosConfig = { readonly provider?: string } & { readonly [provider: string]: AuthWorkosProviderConfig | undefined }(or move the selector into a nested key) and remove the runtime guard duplication.
🤖 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/config/src/types.ts` around lines 628 - 707, The index signature on HoloAuthWorkosConfig and NormalizedHoloAuthWorkosConfig is too permissive (it allows string values) which widens lookups like config.workos.dashboard to AuthWorkosProviderConfig | string | undefined; change both types to an intersection style (keep readonly provider?: string but make the indexer readonly [provider: string]: AuthWorkosProviderConfig | undefined and similarly for NormalizedAuthWorkosConfig with NormalizedAuthWorkosProviderConfig) so string entries are disallowed, and then remove the defensive runtime/type guard in getConfiguredProviderConfig (packages/auth-workos/src/index.ts) that checks typeof configured !== 'object' because the type now enforces object-valued providers.
🤖 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 `@apps/blog-nuxt/server/api/auth/workos/callback.get.ts`:
- Around line 6-8: The redirect currently builds a query with
encodeURIComponent(result.code) which can produce error=undefined when
result.code is absent; update the callback error path (the branch that calls
sendRedirect) to use a stable fallback (e.g., const errCode = result.code ??
'unknown_error') before encoding and pass encodeURIComponent(errCode) to
sendRedirect so the URL always contains a meaningful error code; locate this in
the callback handler where result.ok is checked and modify the redirect
construction accordingly.
In `@packages/auth-workos/tests/package.test.ts`:
- Around line 74-77: The derived name assignment uses the expression
[session.identity.firstName, session.identity.lastName].filter(Boolean).join('
') which returns '' when both names are missing, preventing the following
nullish coalescing from running; change the middle operand so an empty join
becomes undefined/null (e.g., compute the joined string and return undefined if
it's empty) so the chain session.identity.name ?? <joined-or-undefined> ??
session.identity.email ?? session.identity.id properly falls through; update the
expression around the variable name (the join of firstName/lastName) to convert
'' to undefined.
In `@packages/auth/src/runtime/responseCookies.ts`:
- Around line 49-51: The current type guard in the for loop over
Object.values(config.workos) falsely narrows any non-null object to
HostedProviderConfig; change the runtime check to ensure the required property
exists before treating it as HostedProviderConfig—e.g., in the loop condition
verify ("sessionCookie" in value && typeof (value as any).sessionCookie ===
"string") or call a small predicate isHostedProviderConfig(value) that returns
true only when value is an object and has a string sessionCookie; update the
loop to use that predicate (references: config.workos, HostedProviderConfig,
provider.sessionCookie) so undefined sessionCookie values cannot be added to the
Set.
---
Nitpick comments:
In `@packages/auth-workos/src/contracts.ts`:
- Around line 117-122: The public contract for
WorkosAuthFacade.completeWorkosAuth is too permissive and loses the
implementation's discriminated union; update the return type to mirror the
precise frozen discriminated union used in packages/auth-workos/src/index.ts (or
export that union as a named type and reference it here) so callers can narrow
on ok and access
provider/guard/authProvider/status/identity/session/authSession/user;
specifically change the signature of WorkosAuthFacade.completeWorkosAuth to
return that exported union type instead of the current loose { ok: boolean;
code?: string; message?: string; user?: AuthUserLike } shape.
In `@packages/auth-workos/src/index.ts`:
- Around line 1107-1116: getStringField currently uses value.trim() only as a
truthiness check but returns the original untrimmed string, causing padded
values to propagate into sessionId, accessToken, and organizationId in
authenticateWorkosCode; change getStringField (the function in
packages/auth-workos/src/index.ts) to return the trimmed string (value.trim())
when returning a string so callers receive cleaned values, preserving the
existing truthiness logic and types.
- Around line 1528-1530: resetWorkosAuthRuntime currently only clears
getRuntimeState().bindings but leaves module-level caches populated; modify
resetWorkosAuthRuntime to also clear the module-level caches
workosDefaultProviderRuntimeCache and workosJwksCache (e.g., call .clear() or
reassign to a fresh Map/undefined as appropriate) so that any cached
WorkosProviderRuntime or JWKS entries are removed and the module returns to a
pristine state.
- Around line 446-497: The profile builder normalizeWorkosUserProfile currently
assigns email directly from user.email which can contain leading/trailing
whitespace; update it to trim the value once at the boundary (e.g., compute
const email = typeof user.email === 'string' ? user.email.trim() : '') and then
use that trimmed email everywhere in the returned WorkosIdentityProfile
(including name fallback logic) so downstream functions can rely on a canonical,
trimmed email.
- Around line 134-142: The property defined on runtimeGlobal using
Object.defineProperty with WORKOS_RUNTIME_STATE_KEY is non-configurable and
non-writable by default, preventing redefinition or deletion; update the
definition in the module that creates the runtime state (the code around
runtimeGlobal and WORKOS_RUNTIME_STATE_KEY, likely in the get runtime
initializer) to either (a) set writable: true and configurable: true along with
enumerable: false, or (b) replace the Object.defineProperty call with a plain
assignment runtimeGlobal[WORKOS_RUNTIME_STATE_KEY] = {} while keeping
enumerable: false behavior via a Symbol-style key; reference
WORKOS_RUNTIME_STATE_KEY and runtimeGlobal to locate the change and ensure
subsequent code that mutates state.bindings continues to work.
- Around line 1417-1457: The await of getAuthRuntime().guard(guard).user()
inside logoutWithWorkos is unused and causes an unnecessary adapter round-trip
and potential misleading error mapping; remove that call (the await
getAuthRuntime().guard(guard).user() line) unless you intentionally want to
surface session/auth errors—if you do, instead keep it but wrap it in its own
try/catch that returns a distinct error code (or adds a comment) so
readCurrentWorkosLogoutSession and setSessionId remain the primary session
checks (references: logoutWithWorkos, getAuthRuntime().guard(guard).user(),
setSessionId, readCurrentWorkosLogoutSession).
- Around line 564-596: The runtime ambiguity comes from allowing a provider
entry named "provider" in bindings.config.workos while
resolveConfiguredProviderName also reads bindings.config.workos.provider; fix
this by reserving the "provider" key at normalization time: update the
NormalizedHoloAuthWorkosConfig creation (and getWorkosProviderEntries in
packages/config/src/defaults.ts) to either filter out or throw on any provider
entry whose key === 'provider' so provider names cannot collide, and then keep
resolveConfiguredProviderName as-is (it can safely treat
bindings.config.workos.provider as the reserved default string); ensure
tests/validation reflect the new reserved-key rule.
In `@packages/auth-workos/tests/package.test.ts`:
- Around line 858-934: The test's fetch mock currently asserts init?.body equals
a specific JSON string, which is brittle because authenticateWorkosCode may
include undefined ip_address/user_agent fields; update the assertion to parse
the body (JSON.parse(String(init?.body))) and assert only the required keys
using toMatchObject or expect.objectContaining for { grant_type:
'authorization_code', client_id: 'workos-client', client_secret: 'workos-key',
code: 'code_123' } — change the assertion inside the vi.stubGlobal fetch in the
"completes the hosted WorkOS callback with typed user mapping" test so it
verifies the parsed body contains those fields rather than matching the exact
JSON string.
In `@packages/config/src/defaults.ts`:
- Around line 1505-1535: The function normalizeWorkosConfig uses an outer
variable named provider (selected provider name) and also destructures a
provider parameter inside the providerEntries.map callbacks, causing shadowing
and confusion; rename the inner mapped variable (e.g., to providerConfig or
entryProvider) in the providerEntries.map that builds normalizedEntries and in
the Object.fromEntries mapping so that calls to normalizeWorkosProvider use the
new name (normalizeWorkosProvider(name, providerConfig, guards, providers)), and
ensure all references in those callbacks are updated accordingly to avoid
shadowing while preserving the same behavior.
- Around line 1480-1503: The type guard isWorkosProviderConfig currently rejects
string entries while HoloAuthWorkosConfig permits them, causing
getWorkosProviderEntries to throw incorrectly; either update the runtime to
accept strings (e.g., change isWorkosProviderConfig to treat typeof value ===
'object' || typeof value === 'string' and in getWorkosProviderEntries normalize
string shorthand into an AuthWorkosProviderConfig object before pushing) or
tighten the static type (remove | string from HoloAuthWorkosConfig) so types and
runtime align, and update the thrown Error message in getWorkosProviderEntries
to list the allowed shapes (object or string) when rejecting values.
In `@packages/config/src/types.ts`:
- Around line 628-707: The index signature on HoloAuthWorkosConfig and
NormalizedHoloAuthWorkosConfig is too permissive (it allows string values) which
widens lookups like config.workos.dashboard to AuthWorkosProviderConfig | string
| undefined; change both types to an intersection style (keep readonly
provider?: string but make the indexer readonly [provider: string]:
AuthWorkosProviderConfig | undefined and similarly for
NormalizedAuthWorkosConfig with NormalizedAuthWorkosProviderConfig) so string
entries are disallowed, and then remove the defensive runtime/type guard in
getConfiguredProviderConfig (packages/auth-workos/src/index.ts) that checks
typeof configured !== 'object' because the type now enforces object-valued
providers.
In `@tests/example-app-auth-flow.mjs`:
- Around line 293-295: The test currently only checks the path via
assertRedirectsTo(await fetchAuthText('/api/auth/workos/callback', {
allowFailure: true }), '/login'); update it to also assert the redirect includes
an error query param: either tighten the expected redirect to include an error
query (e.g. '/login?error=...') or fetch the Location/redirect URL from
fetchAuthText's response and assert that new URL includes an "error" query key;
look for and update the call to assertRedirectsTo and/or add a follow-up
assertion after fetchAuthText('/api/auth/workos/callback', ...) to verify the
presence of the error query param.
🪄 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: bb0d5588-615e-4650-a4f2-099d3e924e58
📒 Files selected for processing (43)
apps/blog-next/app/api/auth/workos/callback/route.tsapps/blog-next/app/api/auth/workos/login/route.tsapps/blog-next/app/api/auth/workos/logout/route.tsapps/blog-next/app/api/auth/workos/register/route.tsapps/blog-next/app/auth-nav.tsxapps/blog-next/app/login/page.tsxapps/blog-next/app/register/page.tsxapps/blog-next/config/auth.tsapps/blog-nuxt/app/app.vueapps/blog-nuxt/app/pages/login/index.vueapps/blog-nuxt/app/pages/register/index.vueapps/blog-nuxt/config/auth.tsapps/blog-nuxt/server/api/auth/workos/callback.get.tsapps/blog-nuxt/server/api/auth/workos/login.get.tsapps/blog-nuxt/server/api/auth/workos/logout.post.tsapps/blog-nuxt/server/api/auth/workos/register.get.tsapps/blog-sveltekit/config/auth.tsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/src/routes/api/auth/workos/callback/+server.tsapps/blog-sveltekit/src/routes/api/auth/workos/login/+server.tsapps/blog-sveltekit/src/routes/api/auth/workos/logout/+server.tsapps/blog-sveltekit/src/routes/api/auth/workos/register/+server.tsapps/blog-sveltekit/src/routes/login/+page.svelteapps/blog-sveltekit/src/routes/register/+page.svelteapps/docs/docs/auth/workos.mdpackages/auth-workos/src/contracts.tspackages/auth-workos/src/index.tspackages/auth-workos/tests/package.test.tspackages/auth/src/runtime/responseCookies.tspackages/auth/tests/docs-smoke.test.tspackages/auth/tests/package.test.tspackages/cli/src/project/scaffold/config-renderers.tspackages/cli/src/project/scaffold/dependencies.tspackages/cli/src/project/scaffold/project-renderers.tspackages/cli/tests/cli.test.tspackages/config/src/defaults.tspackages/config/src/loader.tspackages/config/src/types.tspackages/config/tests/config.test.tspackages/config/tests/config.type.test.tspackages/core/src/portable/holo.tspackages/core/tests/auth-runtime.test.tstests/example-app-auth-flow.mjs
There was a problem hiding this comment.
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 `@packages/auth-workos/src/contracts.ts`:
- Around line 41-57: The two related result types disagree on whether
authSession is optional: WorkosCompleteAuthResult currently has authSession
required while WorkosAuthenticationResult declares authSession?:
AuthEstablishedSession; decide whether authSession can be absent and then make
the optionality consistent across both types (either make authSession optional
in WorkosCompleteAuthResult or required in WorkosAuthenticationResult) by
updating the type signatures for WorkosCompleteAuthResult and
WorkosAuthenticationResult (and any related readonly branches) so both reference
authSession with the same presence and the AuthEstablishedSession type.
🪄 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: 8b4babd8-809b-48d1-b70f-01b666272328
📒 Files selected for processing (12)
apps/blog-next/tests/run.mjsapps/blog-nuxt/server/api/auth/workos/callback.get.tsapps/blog-nuxt/tests/run.mjsapps/blog-sveltekit/tests/run.mjspackages/auth-workos/src/contracts.tspackages/auth-workos/src/index.tspackages/auth-workos/tests/package.test.tspackages/auth/src/runtime/responseCookies.tspackages/auth/tests/package.test.tspackages/config/src/defaults.tspackages/config/tests/config.test.tstests/example-app-auth-flow.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/example-app-auth-flow.mjs
- packages/auth/src/runtime/responseCookies.ts
- packages/config/src/defaults.ts
- packages/config/tests/config.test.ts
- packages/auth-workos/tests/package.test.ts
- packages/auth/tests/package.test.ts
- packages/auth-workos/src/index.ts
1 participant
Summary by CodeRabbit
New Features
Updates
Documentation
Tests