Conversation
Single canonical RTK Query instance used by davinci-client, journey-client, and oidc-client — prerequisite for shared Redux store cache deduplication.
… inject into oidc davinci() and journey() now expose an opaque SdkStore handle on their return value. oidc() accepts that handle as an optional second argument and lazily injects its reducer and middleware into the shared store via combineSlices.inject() and createDynamicMiddleware.addMiddleware(), so both clients share a single Redux store with one deduplicated wellknown cache entry. - sdk-types: add SdkStore opaque type (symbol brand) - sdk-wellknown: add passWithNoTests to vite.config.ts - davinci-client: migrate to combineSlices + InjectableStore + toSdkStore/fromSdkStore - journey-client: same migration pattern, expose store: SdkStore on JourneyClient - oidc-client: add injectIntoStore() helper; oidc(opts, sharedStore?) API - oidc-client: shared-store.test.ts — 7 integration tests covering injection contract and cache deduplication behaviour toSdkStore/fromSdkStore internal helpers use object type to avoid api-extractor isExternal/isInternal conflict on SdkStore; cast to SdkStore at the public boundary.
…dStore requestMiddleware has no effect when oidc() uses an injected store — the owning store's thunk.extraArgument is used instead. Emit an explicit warn so callers aren't left debugging silent behavior. Test coverage: adds idempotency, behavioral subscribe, cache-reuse, requestMiddleware-warning, and state-after-injection assertions.
Migrate all packages from effect@^3.20 to effect@4.0.0-beta.103. Key changes: - Replace Micro with Effect throughout oidc-client and davinci-client - Migrate Either to Result in sdk-utilities and journey-client - Migrate MicroExit/exitIsFail/exitIsSuccess to Exit/Cause API - Update e2e/mock-api-v2 from @effect/platform to effect/unstable/httpapi - Bump vitest catalog to ^4.1.0 to match @effect/vitest peer requirement - Remove consolidated packages: @effect/platform, @effect/cli - Fix handleMicroExit → handleExit (renamed) - Add vi.clearAllMocks() to oidc-client afterEach (accumulated mock counts)
|
📝 WalkthroughWalkthroughThe PR migrates Effect HTTP APIs and runtime effects, adds shared Redux store support across clients, extracts the well-known API into a package, updates Result and schema APIs, and adjusts workspace configuration. ChangesMock API migration
Shared store and client integration
Effect and Result migration
Workspace integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Davinci
participant Journey
participant Oidc
participant SharedStore
participant WellknownApi
Davinci->>SharedStore: Create and expose opaque SdkStore
Journey->>SharedStore: Create and expose opaque SdkStore
Oidc->>SharedStore: Inject OIDC reducer and middleware
Oidc->>WellknownApi: Query shared well-known cache
WellknownApi-->>SharedStore: Store response and notify subscribers
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit 504562d
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
e2e/mock-api-v2/src/main.ts (1)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the service’s built-in layer where available.
Context.Serviceprovides a layer from its constructor; useSessionStorage.layerinstead of reconstructing it withLayer.effect(SessionStorage, SessionStorage.make).🤖 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 `@e2e/mock-api-v2/src/main.ts` around lines 39 - 42, Update the SessionLayer definition to use the built-in SessionStorage.layer provided by the Context.Service, replacing the manually reconstructed Layer.effect(SessionStorage, SessionStorage.make) while preserving the existing Layer.provide wiring.
🤖 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 `@e2e/mock-api-v2/src/middleware/Authorization.ts`:
- Around line 35-37: Update the bearer-token check in Authorization middleware
to stop passing tokenValue to Effect.log; keep the log entry limited to the
non-secret message while preserving the surrounding credential validation flow.
In `@e2e/mock-api-v2/src/middleware/Session.ts`:
- Around line 30-45: The session creation flow in the Session middleware must
retain the generated session ID and persist it to the client. Update
SessionStorage.createSession to return both the generated ID and SessionData,
capture that result when creating a session, and set the sessionId cookie on the
downstream response before returning it; preserve the refresh path for existing
sessions.
In `@e2e/mock-api-v2/src/services/mock-env-helpers/index.ts`:
- Around line 66-71: The validateCapabilitiesResponse function passes only
formData.value to validator, which expects a complete CapabilitiesRequestBody
and therefore bypasses unauthorized handling. Pass the complete body expected by
validator, or replace it with a validator matching the extracted value shape
while preserving Unauthorized for invalid credentials.
In `@e2e/mock-api-v2/src/services/session.service.ts`:
- Around line 76-97: Update refreshSession and updateSession in the session
service so every missing or expired session path uses Effect.fail with the
appropriate error, rather than returning an Effect or Error as a successful
value. Keep successful session updates returning the session data, and ensure
their inferred error channels allow the existing Effect.orDie calls in
Session.ts to handle these failures.
In `@packages/davinci-client/src/lib/client.store.utils.ts`:
- Around line 135-141: Validate the sharedStore argument at the oidc() boundary
before calling injectIntoStore, requiring the store, rootReducer.inject, and
dynamicMiddleware.addMiddleware members used by injection. For invalid objects,
return the established controlled argument_error result; do not use the SdkStore
runtime brand as the validation mechanism.
In `@packages/oidc-client/src/lib/authorize.request.micros.test.ts`:
- Around line 76-87: Ensure every equivalent failure assertion in
packages/oidc-client/src/lib/authorize.request.micros.test.ts at lines 76-87 and
packages/oidc-client/src/lib/session.micros.test.ts at lines 196-211 fails
explicitly when Cause.findErrorOption returns None, instead of returning early.
Preserve the existing typed-error assertions for Some results so defect-only
failures cannot pass.
---
Nitpick comments:
In `@e2e/mock-api-v2/src/main.ts`:
- Around line 39-42: Update the SessionLayer definition to use the built-in
SessionStorage.layer provided by the Context.Service, replacing the manually
reconstructed Layer.effect(SessionStorage, SessionStorage.make) while preserving
the existing Layer.provide wiring.
🪄 Autofix
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: 34fd29c7-fe7a-4d2d-b4b6-45c6076d316d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (93)
e2e/mock-api-v2/package.jsone2e/mock-api-v2/src/handlers/authorize.handler.tse2e/mock-api-v2/src/handlers/capabilities.handler.tse2e/mock-api-v2/src/handlers/end-session.handler.tse2e/mock-api-v2/src/handlers/healthcheck.handler.tse2e/mock-api-v2/src/handlers/open-id-configuration.handler.tse2e/mock-api-v2/src/handlers/revoke.handler.tse2e/mock-api-v2/src/handlers/token.handler.tse2e/mock-api-v2/src/handlers/userinfo.handler.tse2e/mock-api-v2/src/helpers/match.tse2e/mock-api-v2/src/main.tse2e/mock-api-v2/src/middleware/Authorization.tse2e/mock-api-v2/src/middleware/CookieMiddleware.tse2e/mock-api-v2/src/middleware/Session.tse2e/mock-api-v2/src/schemas/authorize.schema.tse2e/mock-api-v2/src/schemas/capabilities/capabilities.request.schema.tse2e/mock-api-v2/src/schemas/capabilities/capabilities.response.schema.tse2e/mock-api-v2/src/schemas/open-id-configuration/open-id-configuration-response.schema.tse2e/mock-api-v2/src/schemas/return-success-response-redirect.schema.tse2e/mock-api-v2/src/schemas/revoke/revoke.schema.tse2e/mock-api-v2/src/schemas/token/token.schema.tse2e/mock-api-v2/src/services/mock-env-helpers/index.tse2e/mock-api-v2/src/services/session.service.tse2e/mock-api-v2/src/services/tokens.service.tse2e/mock-api-v2/src/services/userinfo.service.tse2e/mock-api-v2/src/spec.tsgoals/centralize-redux-store/facts-result.jsongoals/centralize-redux-store/facts-review.jsongoals/centralize-redux-store/facts.mdgoals/centralize-redux-store/facts.meta.jsongoals/centralize-redux-store/goal.mdgoals/centralize-redux-store/interview-result.jsongoals/centralize-redux-store/interview.jsongoals/centralize-redux-store/plan.mdpackage.jsonpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/package.jsonpackages/davinci-client/src/lib/client.store.effects.test.tspackages/davinci-client/src/lib/client.store.effects.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.store.utils.tspackages/davinci-client/src/lib/fido/fido.tspackages/davinci-client/src/lib/password-policy.rules.tspackages/davinci-client/src/lib/wellknown.api.tspackages/davinci-client/tsconfig.jsonpackages/davinci-client/tsconfig.lib.jsonpackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/package.jsonpackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/client.store.utils.tspackages/journey-client/src/lib/journey.utils.test.tspackages/journey-client/src/lib/journey.utils.tspackages/journey-client/src/lib/wellknown.api.tspackages/journey-client/tsconfig.lib.jsonpackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/package.jsonpackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/client.store.utils.tspackages/oidc-client/src/lib/exchange.request.tspackages/oidc-client/src/lib/exchange.utils.test.tspackages/oidc-client/src/lib/exchange.utils.tspackages/oidc-client/src/lib/logout.request.test.tspackages/oidc-client/src/lib/logout.request.tspackages/oidc-client/src/lib/session.micros.test.tspackages/oidc-client/src/lib/session.micros.tspackages/oidc-client/src/lib/shared-store.test.tspackages/oidc-client/tsconfig.lib.jsonpackages/sdk-effects/wellknown/eslint.config.mjspackages/sdk-effects/wellknown/package.jsonpackages/sdk-effects/wellknown/src/index.tspackages/sdk-effects/wellknown/src/lib/wellknown.api.tspackages/sdk-effects/wellknown/tsconfig.jsonpackages/sdk-effects/wellknown/tsconfig.lib.jsonpackages/sdk-effects/wellknown/tsconfig.spec.jsonpackages/sdk-effects/wellknown/vite.config.tspackages/sdk-types/src/index.tspackages/sdk-types/src/lib/store.types.tspackages/sdk-utilities/src/lib/config/config.effects.tspackages/sdk-utilities/src/lib/config/config.test.tspackages/sdk-utilities/src/lib/config/config.types.tspackages/sdk-utilities/src/lib/config/config.utils.tspackages/sdk-utilities/src/lib/micro.utils.tspnpm-workspace.yamltools/release/package.jsontools/user-scripts/package.jsontsconfig.json
💤 Files with no reviewable changes (4)
- tools/user-scripts/package.json
- tools/release/package.json
- packages/journey-client/src/lib/wellknown.api.ts
- packages/davinci-client/src/lib/wellknown.api.ts
Comment on lines
35
to
37
| Effect.gen(function* () { | ||
| const tokenValue = Redacted.value(bearerToken); | ||
| const tokenValue = Redacted.value(credential); | ||
| yield* Effect.log('checking bearer token', tokenValue); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file relevant section =="
sed -n '1,80p' e2e/mock-api-v2/src/middleware/Authorization.ts
echo
echo "== package / Effect dependency context =="
for f in package.json e2e/mock-api-v2/package.json e2e/mock-api-v2/package-lock.json e2e/mock-api-v2/pnpm-lock.yaml e2e/mock-api-v2/yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n '"effect"|effect|`@effect/platform`|effect/Effect' "$f" | head -80 || true
fi
done
echo
echo "== search for Effect.log definition/imports in local code =="
rg -n "Effect\.log|from ['\"]effect/Effect|log:" e2e/mock-api-v2/src package.json e2e/mock-api-v2/package.json 2>/dev/null || trueRepository: ForgeRock/ping-javascript-sdk
Length of output: 4500
🌐 Web query:
Effect Effect.log effect library default logger signature Effect.log documentation
💡 Result:
In the Effect library, Effect.log is a utility function used to log messages at the default INFO level [1][2]. As of recent versions (specifically following changes in v2.0.0 and related updates), the function signature is variadic, allowing it to accept any number of arguments [3][4]. The signature is defined as follows: Effect.log(...message: ReadonlyArray): Effect<void, never, never> [3] Key details regarding Effect.log and logging in Effect: - Behavior: It logs messages using the configured logger, which defaults to the INFO level [1][2]. - Observability: Effect logs include metadata such as timestamps, log levels, fiber identifiers, and message content [1][2]. - Extensibility: You can control log levels (e.g., using Logger.withMinimumLogLevel) or implement custom loggers to handle output formats like JSON or structured data [1][5][2]. - Related Functions: Effect also provides specific variants for different log levels, such as Effect.logDebug, Effect.logInfo, Effect.logWarning, and Effect.logError, all of which share the same variadic signature structure [3][6]. For more advanced logging requirements, such as writing to files, you can utilize PlatformLogger or configure custom logging layers [1][7].
Citations:
- 1: https://effect.website/docs/observability/logging/
- 2: https://www.effect.website/docs/v3/observability/logging
- 3: Effect-TS/effect@a18f594
- 4: support variadic arguments in Effect.log Effect-TS/effect#2452
- 5: https://effect-ts.github.io/effect/effect/Logger.ts.html
- 6: https://effect-ts.github.io/effect/effect/Effect.ts.html
- 7: https://effect.website/docs/platform/platformlogger/
Sensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
e2e/mock-api-v2/src/main.ts:51
UserInfoMockHandler
│
▼
● Hop
e2e/mock-api-v2/src/handlers/userinfo.handler.ts
│
▼
● Hop
e2e/mock-api-v2/src/spec.ts:194
Authorization
│
▼
● Sink
e2e/mock-api-v2/src/middleware/Authorization.ts
Do not log the bearer credential.
Effect.log(...message: ReadonlyArray<any>) forwards every argument through the logger. Passing tokenValue = Redacted.value(credential) exposes the bearer token in logs. Keep this log entry limited to non-secret metadata, such as the log message only.
Proposed fix
- yield* Effect.log('checking bearer token', tokenValue);
+ yield* Effect.log('checking bearer token');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Effect.gen(function* () { | |
| const tokenValue = Redacted.value(bearerToken); | |
| const tokenValue = Redacted.value(credential); | |
| yield* Effect.log('checking bearer token', tokenValue); | |
| Effect.gen(function* () { | |
| const tokenValue = Redacted.value(credential); | |
| yield* Effect.log('checking bearer token'); |
🤖 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 `@e2e/mock-api-v2/src/middleware/Authorization.ts` around lines 35 - 37, Update
the bearer-token check in Authorization middleware to stop passing tokenValue to
Effect.log; keep the log entry limited to the non-secret message while
preserving the surrounding credential validation flow.
Comment on lines
+30
to
+45
| session = yield* sessionStorage | ||
| .createSession({ | ||
| userId: request.cookies.userId, | ||
| createdAt: new Date(), | ||
| expiresAt: new Date(Date.now() + 60 * 60 * 1000), // 1 hour | ||
| data: {}, | ||
| }) | ||
| .pipe(Effect.orDie); | ||
| } else { | ||
| yield* sessionStorage | ||
| .refreshSession(request.cookies.sessionId, sessionData.expiresAt) | ||
| .pipe(Effect.orDie); | ||
| session = sessionData; | ||
| } | ||
|
|
||
| return sessionData; | ||
| }); | ||
| return yield* httpEffect.pipe(Effect.provideService(Session, session)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Return and persist the generated session ID.
SessionStorage.createSession generates the map key but returns only SessionData. This middleware then returns the downstream response without setting a sessionId cookie. A client cannot reference the new session on its next request, so the middleware creates a new session again.
Change the session-storage contract to return the generated ID. Set that ID on the response before returning it.
🤖 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 `@e2e/mock-api-v2/src/middleware/Session.ts` around lines 30 - 45, The session
creation flow in the Session middleware must retain the generated session ID and
persist it to the client. Update SessionStorage.createSession to return both the
generated ID and SessionData, capture that result when creating a session, and
set the sessionId cookie on the downstream response before returning it;
preserve the refresh path for existing sessions.
Comment on lines
66
to
+71
| const validateCapabilitiesResponse = (body: any) => | ||
| pipe( | ||
| body, | ||
| Option.fromNullable, | ||
| Option.map((body) => body.parameters), | ||
| Option.map((parameters) => parameters.data), | ||
| Option.map((data) => data.formData), | ||
| Option.map((formData) => formData.value), | ||
| Effect.flatMap(validator), | ||
| ); | ||
| Effect.gen(function* () { | ||
| if (body == null) return yield* Effect.fail(new HttpApiError.InternalServerError()); | ||
| const value = body?.parameters?.data?.formData?.value; | ||
| if (value == null) return yield* Effect.fail(new HttpApiError.InternalServerError()); | ||
| return yield* validator(value); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the shape that validator expects.
validator matches a complete CapabilitiesRequestBody with parameters.data.formData.username and password. Line 69 passes only formData.value. The match falls through to Match.orElse(() => Effect.succeed(true)), so invalid credentials succeed instead of returning Unauthorized.
Pass the complete request body, or add a validator that explicitly matches the extracted value shape.
🤖 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 `@e2e/mock-api-v2/src/services/mock-env-helpers/index.ts` around lines 66 - 71,
The validateCapabilitiesResponse function passes only formData.value to
validator, which expects a complete CapabilitiesRequestBody and therefore
bypasses unauthorized handling. Pass the complete body expected by validator, or
replace it with a validator matching the extracted value shape while preserving
Unauthorized for invalid credentials.
Comment on lines
+76
to
+97
| refreshSession: Effect.fn('RefreshSessionMiddleware')(function* ( | ||
| sessionId: string, | ||
| expiryDate?: Date, | ||
| ) { | ||
| const session = _store.get(sessionId); | ||
|
|
||
| if (!session) { | ||
| return Effect.fail(new Error('Session not found')); | ||
| } | ||
|
|
||
| cleanupExpiredSessions: Effect.fn('CleanupExpiredSessionsMiddleware')(function* () { | ||
| for (const [sessionId, session] of _store.entries()) { | ||
| if (isSessionExpired(session)) { | ||
| _store.delete(sessionId); | ||
| return new Error('Session has expired'); | ||
| } | ||
|
|
||
| // Update expiry date | ||
| const newExpiryDate = expiryDate || new Date(Date.now() + 24 * 60 * 60 * 1000); // Default: 24 hours from now | ||
| session.expiresAt = newExpiryDate; | ||
| _store.set(sessionId, session); | ||
|
|
||
| return session.data; | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Signal failures with Effect.fail, not with returned values.
Inside a generator passed to Effect.fn, a return produces a success value. Line 83 therefore succeeds with an un-executed Effect as the value, and line 88 succeeds with an Error instance. The declared error channel stays never, so the Effect.orDie calls in e2e/mock-api-v2/src/middleware/Session.ts lines 25-40 never trigger. A missing or expired session is reported as a successful refresh.
updateSession at lines 65-74 has the same pattern, and the mixed union is why refreshSession is typed Effect.Effect<unknown, never, never> at line 19.
🐛 Proposed fix for failure signalling
- updateSession: (
- sessionId: string,
- data: SessionData,
- ) => Effect.Effect<SessionData | Error, never, never>;
- refreshSession: (sessionId: string, expiryDate?: Date) => Effect.Effect<unknown, never, never>;
+ updateSession: (sessionId: string, data: SessionData) => Effect.Effect<SessionData, Error, never>;
+ refreshSession: (
+ sessionId: string,
+ expiryDate?: Date,
+ ) => Effect.Effect<Record<string, unknown> | undefined, Error, never>; updateSession: Effect.fn('UpdateSessionMiddleware')(function* (
sessionId: string,
data: SessionData,
) {
if (!_store.has(sessionId)) {
- return new Error('Session not found');
+ return yield* Effect.fail(new Error('Session not found'));
}
_store.set(sessionId, data);
return data;
}),
@@
const session = _store.get(sessionId);
if (!session) {
- return Effect.fail(new Error('Session not found'));
+ return yield* Effect.fail(new Error('Session not found'));
}
if (isSessionExpired(session)) {
_store.delete(sessionId);
- return new Error('Session has expired');
+ return yield* Effect.fail(new Error('Session has expired'));
}Note that Session.ts already applies Effect.orDie, so the middleware behavior stays defined after this change.
🤖 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 `@e2e/mock-api-v2/src/services/session.service.ts` around lines 76 - 97, Update
refreshSession and updateSession in the session service so every missing or
expired session path uses Effect.fail with the appropriate error, rather than
returning an Effect or Error as a successful value. Keep successful session
updates returning the session data, and ensure their inferred error channels
allow the existing Effect.orDie calls in Session.ts to handle these failures.
Comment on lines
+135
to
+141
| export function toSdkStore(injectable: InjectableStore): object { | ||
| return injectable as unknown as object; | ||
| } | ||
|
|
||
| /** Recover the InjectableStore from an opaque SdkStore handle. */ | ||
| export function fromSdkStore(sdkStore: object): InjectableStore { | ||
| return sdkStore as unknown as InjectableStore; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/oidc-client/src/lib/client.store.utils.ts --items all
ast-grep outline packages/oidc-client/src/lib/client.store.ts --items all
rg -n -C 8 \
'fromSdkStore|toSdkStore|sharedStore|rootReducer\.inject|addMiddleware' \
packages/oidc-client/src/lib packages/journey-client/src/lib packages/davinci-client/src/libRepository: ForgeRock/ping-javascript-sdk
Length of output: 35023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n packages/oidc-client/src/lib/client.store.utils.ts
printf '\n--- oidc signature/body ---\n'
sed -n '53,100p' packages/oidc-client/src/lib/client.store.ts | cat -n
printf '\n--- SdkStore type ---\n'
rg -n -C 4 'export interface SdkStore|export type SdkStore|interface SdkStore|type SdkStore' packages -g '*.ts'
printf '\n--- error/create argument patterns ---\n'
rg -n -C 3 "type: 'argument_error'|argument_error|GenericError|isGenericError" packages/oidc-client/src/lib client.store.utils.ts packages/davinci-client/src/lib packages/journey-client/src/lib packages -g '*.ts' | head -n 220
python3 - <<'PY'
# Read-only behavioral probe: show how fake SdkStore values fail when destructure/inject path runs.
handles = [
{},
{store: object()},
{store: object(), rootReducer: {}},
{store: object(), rootReducer: {}, dynamicMiddleware: {}},
{store: object(), rootReducer: {}, dynamicMiddleware: {}},
{store: object(), rootReducer: {}, dynamicMiddleware: {addMiddleware: None}},
]
for i, h in enumerate(handles, 1):
try:
{store, rootReducer, dynamicMiddleware} = h
try:
rootReducer.inject("oidcApi")
print(f"handle {i}: inject succeeded")
except Exception as e:
print(f"handle {i}: inject threw {type(e).__name__}: {e}")
except AttributeError as e:
print(f"handle {i}: destructured invalid handle threw {type(e).__name__}: {e}")
PYRepository: ForgeRock/ping-javascript-sdk
Length of output: 26906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n packages/sdk-types/src/lib/store.types.ts
printf '\n--- argument_error helper usages in oidc-client ---\n'
rg -n -C 4 "typeof|create.*argument|argument_error|GenericError" packages/oidc-client/src/lib packages/sdk-utilities/src/lib packages/sdk-types/src/lib packages/sdk-request-middleware/src/lib -g '*.ts'
printf '\n--- exports containing GenericError/argument_error ---\n'
rg -n "GenericError|argument" packages/sdk-types/src packages/sdk-utilities/src packages/sdk-request-middleware/src -g '*.ts'Repository: ForgeRock/ping-javascript-sdk
Length of output: 50386
Reject invalid shared-SdkStore handles before injecting OIDC.
injectIntoStore(sharedStore) directly dereferences store, rootReducer.inject, and dynamicMiddleware.addMiddleware, so objects missing those properties cause unhandled failures during oidc() initialization. Validate sharedStore at the oidc() boundary and return a controlled argument_error; do not rely on the SdkStore runtime brand to block invalid values.
🤖 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/davinci-client/src/lib/client.store.utils.ts` around lines 135 -
141, Validate the sharedStore argument at the oidc() boundary before calling
injectIntoStore, requiring the store, rootReducer.inject, and
dynamicMiddleware.addMiddleware members used by injection. For invalid objects,
return the established controlled argument_error result; do not use the SdkStore
runtime brand as the validation mechanism.
Comment on lines
+76
to
87
| Effect.gen(function* () { | ||
| vi.spyOn(sdkOidc, 'generateAndStoreAuthUrlValues').mockImplementation(() => { | ||
| throw new Error('storage unavailable'); | ||
| }); | ||
| const exit = yield* Micro.exit(generateAuthValuesµ(config, wellknown)); | ||
| expect(Micro.exitIsFailure(exit)).toBe(true); | ||
| if (!Micro.exitIsFailure(exit) || !Micro.causeIsFail(exit.cause)) return; | ||
| expect(exit.cause.error.type).toBe('auth_error'); | ||
| expect(exit.cause.error.error).toBe('PAR_PARAM_BUILD_ERROR'); | ||
| const exit = yield* Effect.exit(generateAuthValuesµ(config, wellknown)); | ||
| expect(Exit.isFailure(exit)).toBe(true); | ||
| if (!Exit.isFailure(exit)) return; | ||
| const errorOpt = Cause.findErrorOption(exit.cause); | ||
| if (!Option.isSome(errorOpt)) return; | ||
| expect(errorOpt.value.type).toBe('auth_error'); | ||
| expect(errorOpt.value.error).toBe('PAR_PARAM_BUILD_ERROR'); | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail when the cause has no typed error.
Exit.isFailure(exit) also succeeds for defects. The return after Option.isSome(errorOpt) lets a defect-only failure pass without checking the expected error mapping.
packages/oidc-client/src/lib/authorize.request.micros.test.ts#L76-L87: throw or assert whenerrorOptisNone. Apply this change to every equivalent failure assertion in this file.packages/oidc-client/src/lib/session.micros.test.ts#L196-L211: throw or assert whenerrorOptisNone. Apply this change to every equivalent failure assertion in this file.
Proposed fix
const errorOpt = Cause.findErrorOption(exit.cause);
-if (!Option.isSome(errorOpt)) return;
+if (!Option.isSome(errorOpt)) {
+ throw new Error('Expected a typed failure');
+}
expect(errorOpt.value.error).toBe('PAR_PARAM_BUILD_ERROR');📍 Affects 2 files
packages/oidc-client/src/lib/authorize.request.micros.test.ts#L76-L87(this comment)packages/oidc-client/src/lib/session.micros.test.ts#L196-L211
🤖 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/oidc-client/src/lib/authorize.request.micros.test.ts` around lines
76 - 87, Ensure every equivalent failure assertion in
packages/oidc-client/src/lib/authorize.request.micros.test.ts at lines 76-87 and
packages/oidc-client/src/lib/session.micros.test.ts at lines 196-211 fails
explicitly when Cause.findErrorOption returns None, instead of returning early.
Preserve the existing typed-error assertions for Some results so defect-only
failures cannot pass.
Collaborator
Author
|
Superseded by a clean branch off main — see new PR |
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
Migrates the entire SDK from
effect@^3.20(v3) toeffect@4.0.0-beta.103(v4 beta).Changes by package
pnpm-workspace.yaml(catalog)effect: '4.0.0-beta.103'@effect/vitest: '4.0.0-beta.103'@effect/platform-node: '4.0.0-beta.103'@effect/platformand@effect/cli(consolidated intoeffectcore)vitestcatalog to^4.1.0packages/sdk-utilitiesmicro.utils.ts:MicroExit/exitIsFail/exitIsSuccess/causeIsDie→Exit/Cause; renamedhandleMicroExit→handleExitconfig.types/utils/effects/test:Either→Result(succeed/fail/isSuccess/isFailure)packages/journey-clientjourney.utils.ts:Either.right/left→Result.succeed/failclient.store.ts:Either.match→Result.match(onFailure/onSuccess)_tag: 'Right'/'Left'→'Success'/'Failure';.right/.left→.success/.failurepackages/oidc-client(12 files)Micro.*→Effect.*Cause.failureOption→Cause.findErrorOption(returnsOption<E>)exitIsFail(exit)→Cause.findErrorOption(exit.cause)+Option.isSomevi.clearAllMocks()inafterEachto fix accumulated mock call countspackages/davinci-client(5 files)Micro.*→Effect.*Effect.repeat({while})→Effect.gen+ manual loop (v4whilereceives schedule output type, not effect output)password-policy.rules.ts:Array.filterMapnow expectsResultcallbacks;Option.some/none()→Result.succeed/failVoidContext.Tag→Context.Servicee2e/mock-api-v2(full migration)from '@effect/platform'→from 'effect/unstable/httpapi'Context.Tag→Context.ServiceEffect.if→Effect.suspend(() => bool ? ... : ...)Option.fromNullable→Option.fromNullishOrSchema.Union(a, b)→Schema.Union([a, b])Schema.Record({key, value})→Schema.Record(key, value)Schema.Schema<T, T>→Schema.Schema<T>HttpApiMiddleware.Tag→HttpApiMiddleware.Service<Self, {provides: T}>()nxBuild→buildVerification
Summary by CodeRabbit