feat: add interface mapping, api-report tool, and client re-export guards#564
Merged
feat: add interface mapping, api-report tool, and client re-export guards#564
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 7 seconds. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (103)
📝 WalkthroughWalkthroughAdds API-extractor based reporting and an interface-mapping validator/generator, migrates Git hooks from Husky to Lefthook, centralizes and expands package type/value re-exports, integrates api-report and mapping validation into CI, and includes numerous tests and tooling files. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant IM as Interface-Mapping Validator
participant Legacy as Legacy SDK
participant New as New SDK Packages
participant Doc as interface_mapping.md
participant GH as GitHub (PR Comments)
CI->>IM: run validator (tools/interface-mapping-validator/src/main.ts)
IM->>Legacy: extract legacy exports
IM->>New: extract new SDK exports
IM->>Doc: parse documented mappings
IM->>IM: diff legacy vs new vs documented
IM->>IM: format report
alt errors found
IM->>GH: find or create PR comment (<!-- interface-mapping-check -->) with "Out of Date" and report
else no errors
IM->>GH: update PR comment to "Up to Date"
end
sequenceDiagram
participant Dev as Developer
participant Nx as Build System (nx)
participant API as API Extractor
participant Fixer as Forgotten-export Fixer (tools/api-report/src/fixer.ts)
participant Types as package `src/types.ts`
Dev->>Nx: run `pnpm api-report` / `nx run-many api-report`
Nx->>API: run api-extractor per entry point
API->>API: emit ae-forgotten-export messages
API-->>Fixer: collect forgotten-export messages
Fixer->>Fixer: parse symbol, resolve source, determine kind
Fixer->>Types: insert or extend re-export statement in `src/types.ts`
Types-->>Fixer: file updated
Fixer-->>Dev: report fixed symbols (if --fix)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
🦋 Changeset detectedLatest commit: 15d5af3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ryanbas21
force-pushed
the
migration-guide
branch
2 times, most recently
from
b02a17e to
7ab96b1
Compare
Contributor
|
View your CI Pipeline Execution ↗ for commit 15d5af3
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
migration-guide
branch
from
7ab96b1 to
f0fefe0
Compare
| | ------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------ | ----- | | ||
| | `import { Callback } from '@forgerock/javascript-sdk'` | `import { Callback } from '@forgerock/journey-client/types'` | None | | ||
| | `import { AttributeInputCallback } from '@forgerock/javascript-sdk'` | `import { AttributeInputCallback } from '@forgerock/journey-client/types'` | None | | ||
| | `import { BaseCallback } from '@forgerock/javascript-sdk'` | `import { BaseCallback } from '@forgerock/journey-client/types'` | None | |
Contributor
There was a problem hiding this comment.
I think you meant FRCallback here instead of BaseCallback, because BaseCallback does not exist in legacy sdk
ryanbas21
force-pushed
the
migration-guide
branch
from
f0fefe0 to
9d5f9c3
Compare
ancheetah
reviewed
Apr 13, 2026
Collaborator
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
review pt 1. interface mapping
Comment on lines
+18
to
+19
| packages/sdk-effects/*/src/index.ts, | ||
| packages/sdk-effects/*/package.json} |
Collaborator
There was a problem hiding this comment.
What about packages/sdk-effects/*/src/types.ts
Comment on lines
+7
to
+15
| new_packages: | ||
| journey: '@forgerock/journey-client' | ||
| oidc: '@forgerock/oidc-client' | ||
| device: '@forgerock/device-client' | ||
| protect: '@forgerock/protect' | ||
| types: '@forgerock/sdk-types' | ||
| utilities: '@forgerock/sdk-utilities' | ||
| logger: '@forgerock/sdk-logger' | ||
| storage: '@forgerock/storage' |
Collaborator
There was a problem hiding this comment.
What about sdk-oidc, middleware, and iFrame manager?
|
|
||
| | Legacy Symbol | New SDK Equivalent | | ||
| | ---------------------------------- | ------------------------------------------------------------------------------------------------- | | ||
| | `AuthResponse` | `import type { AuthResponse } from '@forgerock/journey-client/types'` | |
Collaborator
There was a problem hiding this comment.
This isn't actually re-exported from journey client. It comes from sdk-types package. There are probably more instances of this in here for other types that we should check.
| | Legacy Symbol | New SDK Equivalent | | ||
| | ---------------------------------- | ------------------------------------------------------------------------------------------------- | | ||
| | `AuthResponse` | `import type { AuthResponse } from '@forgerock/journey-client/types'` | | ||
| | `Callback` | `import type { Callback } from '@forgerock/sdk-types'` — AM callback interface | |
Collaborator
There was a problem hiding this comment.
This is re-exported from journey client. It's probably preferable to grab this from journey client since this is a journey client migration guide. Can we check for other instances of this? Step is another example.
| | `LoggerFunctions` | Removed — use `CustomLogger` from `@forgerock/sdk-logger` instead | | ||
| | `MessageCreator` | `import type { MessageCreator } from '@forgerock/journey-client/policy'` | | ||
| | `NameValue` | `import type { NameValue } from '@forgerock/sdk-types'` | | ||
| | `OAuth2Tokens` | `import type { Tokens } from '@forgerock/sdk-types'` — renamed to `Tokens` | |
Collaborator
There was a problem hiding this comment.
This is correct but I don't believe we use this type anywhere in our new SDK. Perhaps we should remove this in favor of OIDC client token types.
|
|
||
| > **Migration note:** If your app relies on `suspendedId`, `RelayState`, or `authIndexValue` URL parameters being auto-parsed, you must extract them manually from the URL and pass them via `options.query` in the new SDK. | ||
|
|
||
| ### Before/After: Authentication Flow |
Collaborator
There was a problem hiding this comment.
The legacy and new snippets below don't seem relevant to resume()
|
|
||
| | Legacy | New | Notes | | ||
| | ------------------------------------------ | ---------------- | -------------------------------------------- | | ||
| | `StepType.Step` (`'Step'`) | `'Step'` | Same values, now from `@forgerock/sdk-types` | |
Collaborator
There was a problem hiding this comment.
preferable to import from journey-client?
| | `forceRenew?: boolean` | `forceRenew?: boolean` | Same behavior | | ||
| | `login?: 'embedded' \| 'redirect'` | Removed | Login mode determined by client usage pattern | | ||
| | `skipBackgroundRequest?: boolean` | `backgroundRenew?: boolean` | **Inverted semantics**: legacy `skipBackgroundRequest: true` = new `backgroundRenew: false` | | ||
| | `query?: StringDict<string>` | `authorizeOptions?: GetAuthorizationUrlOptions` | Authorization customization moved to nested options | |
Collaborator
There was a problem hiding this comment.
Not sure if we want to be more specific here. query was moved to authorizeOptions?.query
| 3. If tokens will expire within `oauthThreshold` (default: 30s) → attempt silent refresh | ||
| 4. If `skipBackgroundRequest: false` (default) → attempt iframe-based silent auth (`prompt=none`) | ||
| 5. If iframe fails with "allowed error" (consent required, CORS, timeout) → fall back to redirect | ||
| 6. If `skipBackgroundRequest: true` → skip iframe, go straight to redirect-based auth |
Collaborator
There was a problem hiding this comment.
Did you mean login: 'redirect'? I'm seeing this in the legacy sdk indicating what's stated here should be inverted.
* @param {boolean} [options.skipBackgroundRequest] - If true, skips the background request to get tokens without redirect.
*
| | **Policy advice parsing (IG)** — Parsed `Advices` from Identity Gateway 401/redirect responses | Not supported. If using IG-protected resources, parse advice manually | | ||
| | **Policy advice parsing (REST)** — Parsed `AuthenticateToServiceConditionAdvice` and `TransactionConditionAdvice` from AM REST responses | Not supported. Parse AM authorization advice manually | | ||
| | **Automatic authorization tree execution** — Ran additional auth trees to satisfy policy advice | Must implement the full advice → auth tree → retry flow manually | | ||
| | **Request timeout** — Configurable via `serverConfig.timeout` | Use `AbortController` with `setTimeout` for fetch timeout | |
Collaborator
There was a problem hiding this comment.
Do we really need AbortController here? Just wondering because I don't currently do that in our sample app
ancheetah
reviewed
Apr 13, 2026
Collaborator
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
review pt 2. migration guide comments.
Overall looks great! Very detailed interface mapping. I reviewed everything but the mapping tool which I'm just going to trust is solid :)
Comment on lines
+949
to
+952
| | `protect(options).start()` | `protect(options).start()` | `Promise<void \| { error }>` | Same | | ||
| | `protect(options).getData()` | `protect(options).getData()` | `Promise<string \| { error }>` | Same | | ||
| | `protect(options).pauseBehavioralData()` | `protect(options).pauseBehavioralData()` | `void \| { error }` | Same | | ||
| | `protect(options).resumeBehavioralData()` | `protect(options).resumeBehavioralData()` | `void \| { error }` | Same | |
Collaborator
There was a problem hiding this comment.
Technically there was a change on the return types. Legacy threw while new returns an error object.
| | Legacy API | New API | Return Type Change | Behavioral Notes | | ||
| | --------------------------------------------------- | ---------------------------------------------- | ------------------------------ | -------------------- | | ||
| | `import { protect } from '@forgerock/ping-protect'` | `import { protect } from '@forgerock/protect'` | — | Package renamed only | | ||
| | `protect(options).start()` | `protect(options).start()` | `Promise<void \| { error }>` | Same | |
Collaborator
There was a problem hiding this comment.
In legacy you called start with your config. In the new sdk you pass the config to the factory function.
Comment on lines
+969
to
+970
| | Error mechanism | `throw new Error(...)` caught via `try/catch` | Return `GenericError` object in union type | | ||
| | Detection | `catch (err) { ... }` | `if ('error' in result) { ... }` | |
Collaborator
There was a problem hiding this comment.
There's a caveat here that some journey client methods still throw... we're not very consistent about it and it's tech debt
|
|
||
| --- | ||
|
|
||
| ## 19. Type Exports |
Collaborator
There was a problem hiding this comment.
Again might want to recommend importing from the client if we can as opposed to sdk-types
Comment on lines
+36
to
+40
| const config: JourneyClientConfig = { | ||
| serverConfig: { | ||
| wellknown: 'https://am.example.com/am/oauth2/alpha/.well-known/openid-configuration', | ||
| }, | ||
| }; |
Collaborator
There was a problem hiding this comment.
I think I already commented about this in interface mapping but alternatively we could init both with the same config and leave a note about how most params will be ignored by journey client.
|
|
||
| ## Error Handling Pattern | ||
|
|
||
| The new SDK returns error objects instead of throwing exceptions: |
Collaborator
There was a problem hiding this comment.
Same comment as interface mapping. Some journey client methods still throw and additionally may return GenericError
Collaborator
There was a problem hiding this comment.
Did you mean to include these tests here?
Comment on lines
+39
to
+40
| "vitest": "catalog:vitest", | ||
| "vitest-canvas-mock": "catalog:vitest" |
Collaborator
There was a problem hiding this comment.
seems like these should be dev deps?
ryanbas21
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
migration-guide
branch
2 times, most recently
from
4450937 to
a75858c
Compare
ryanbas21
force-pushed
the
migration-guide
branch
from
a75858c to
6aac711
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We fixed six failing lint/typecheck tasks by addressing prettier formatting (via eslint --fix), replacing string literals with ExtractorLogLevel enum values to satisfy TypeScript's nominal enum typing, removing unused imports and destructured variables, and converting unused type-assertion patterns (type _Foo = [...] / const _pk = ...) to void expressions that keep the compile-time checks intact without triggering no-unused-vars errors.
Tip
✅ We verified this fix by re-running @forgerock/oidc-client:lint, @forgerock/davinci-client:lint, @forgerock/journey-client:lint and 1 more.
Warning
The suggested diff is too large to display here, but you can view it on Nx Cloud ↗
Or Apply changes locally with:
npx nx-cloud apply-locally J5wb-7zWr
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
ryanbas21
force-pushed
the
migration-guide
branch
from
2878158 to
4b892c2
Compare
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #564 +/- ##
===========================================
- Coverage 70.90% 15.67% -55.24%
===========================================
Files 53 154 +101
Lines 2021 26666 +24645
Branches 377 1126 +749
===========================================
+ Hits 1433 4180 +2747
- Misses 588 22486 +21898
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Contributor
|
Deployed ed59e57 to https://ForgeRock.github.io/ping-javascript-sdk/pr-564/ed59e575a0ad3adf2d8beda84a42f3e6a7249086 branch gh-pages in ForgeRock/ping-javascript-sdk |
Contributor
Interface Mapping Up to DateThe |
Contributor
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-9.7 KB, -100.0%) 📊 Minor Changes📈 @forgerock/device-client - 9.9 KB (+0.2 KB) ➖ No Changes➖ @forgerock/sdk-utilities - 11.2 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
tools/interface-mapping-validator/src/validate-client-imports.ts-12-15 (1)
12-15:⚠️ Potential issue | 🟡 MinorHandle double-quoted imports to avoid validator false negatives.
Current matching only detects single quotes, so lines like
from "@forgerock/..."bypass validation.Suggested change
- const importRegex = /from\s+'(`@forgerock`\/[^']+)'/g; + const importRegex = /from\s+['"](`@forgerock`\/[^'"]+)['"]/g; @@ - if (!line.includes("from '@forgerock/")) continue; + if (!line.includes("from '@forgerock/") && !line.includes('from "@forgerock/')) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/validate-client-imports.ts` around lines 12 - 15, The current importRegex and the line filtering in the for (const line of lines) loop only match single-quoted imports, causing double-quoted imports to be skipped; update importRegex to accept either single or double quotes (e.g. use a pattern like /from\s+['"](`@forgerock`\/[^'"]+)['"]/g) and replace the simple string check if (!line.includes("from '@forgerock/")) continue; with a regex test against importRegex (or a separate /from\s+['"]@forgerock\// check) so both " and ' forms are detected by validate-client-imports (ensure captures still return the module name in the same group).MIGRATION.md-68-74 (1)
68-74:⚠️ Potential issue | 🟡 MinorFix broken legacy snippet:
Configis used but not imported.Line 70 calls
Config.set(...), but Line 68 does not importConfig. The snippet will fail as written.📝 Proposed doc fix
-import { FRAuth, StepType } from '@forgerock/javascript-sdk'; +import { Config, FRAuth, StepType } from '@forgerock/javascript-sdk';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MIGRATION.md` around lines 68 - 74, The snippet calls Config.set(...) but Config is not imported; update the import from '@forgerock/javascript-sdk' to include Config (or add a separate import for Config) so that Config, along with the existing FRAuth and StepType identifiers, is correctly imported before calling Config.set in the snippet.tools/interface-mapping-validator/package.json-16-24 (1)
16-24:⚠️ Potential issue | 🟡 MinorRemove
vitestfromdependencies.
vitestis declared on both line 19 (dependencies) and line 23 (devDependencies). Sincevitestis a testing framework, it should only be indevDependencies. Thevitest.config.tsconfiguration file imports fromvitest/config, which is a legitimate development-time dependency and does not require a runtime copy.Proposed fix
"dependencies": { "ts-morph": "^25.0.0", - "tsx": "^4.20.0", - "vitest": "catalog:vitest" + "tsx": "^4.20.0" }, "devDependencies": { "@forgerock/javascript-sdk": "4.9.0", "vitest": "catalog:vitest" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/package.json` around lines 16 - 24, Remove the accidental runtime dependency: delete the "vitest" entry from the "dependencies" section in package.json and keep the existing "vitest" entry only under "devDependencies"; ensure the duplicate key under "dependencies" (the string "vitest") is removed so tests remain available at dev time but not bundled as a production dependency.packages/journey-client/src/__tests__/types-reexport.typecheck.ts-18-18 (1)
18-18:⚠️ Potential issue | 🟡 MinorUpdate stale comment to match current expectation.
Line 18 says these types are “not yet” re-exported, but this file now enforces that they are available. The comment is misleading.
Proposed comment fix
- // These should be re-exported but are not yet + // Additional public types that must remain re-exported🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/__tests__/types-reexport.typecheck.ts` at line 18, Update the stale inline comment that currently reads "These should be re-exported but are not yet" to reflect the current behaviour enforced by the test (e.g., "These types are expected to be re-exported and are validated by this test"), so the comment matches that the types are now required to be available; locate the existing comment string on the line with that message and replace it with the new, accurate phrasing.tools/interface-mapping-validator/README.md-106-122 (1)
106-122:⚠️ Potential issue | 🟡 MinorAdd a language to this fenced block.
markdownlint is already flagging Line 106. Using
textorplaintexthere keeps the README lint-clean.Suggested fix
-``` +```text src/ ├── extractors/ │ ├── legacy.ts # Parse legacy SDK dist/index.d.ts → symbol list │ ├── new-sdk.ts # Parse new SDK package.json exports + barrel files → symbol list │ └── markdown.ts # Parse interface_mapping.md tables → documented mappings ├── mapping-config.ts # Symbol mapping data (the "rename database") ├── generator.ts # Config + exports → markdown table strings ├── writer.ts # Replace sections in markdown, preserve everything else ├── differ.ts # Compare extracted vs. documented → findings ├── fixer.ts # Apply findings as patches to markdown ├── validate-client-imports.ts # Ensure doc only references *-client package imports ├── reporter.ts # Format findings for stdout ├── types.ts # Shared type definitions ├── config.ts # Constants (package paths, section names, allowed packages) └── main.ts # CLI entry point</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@tools/interface-mapping-validator/README.mdaround lines 106 - 122, Update
the fenced code block containing the src/ tree in README.md to include a
language specifier (e.g., change the openingtotext) so markdownlint
stops flagging it; specifically modify the fenced block that shows the list (the
block with "src/" and the extractors/new-sdk.ts/legacy.ts lines) to use ```text
as the opening fence and keep the rest of the block identical.</details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/extractors/markdown.ts-38-41 (1)</summary><blockquote> `38-41`: _⚠️ Potential issue_ | _🟡 Minor_ **Potential issue: empty string returned as valid entry point.** When `match[2]` is `undefined`, this returns an empty string instead of `null`, which could result in empty strings being added to the entry points list. <details> <summary>🛠️ Proposed fix</summary> ```diff function extractEntryPoint(text: string): string | null { const match = text.match(IMPORT_SYMBOL_RE); - return match ? (match[2] ?? '') : null; + return match?.[2] ?? null; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/extractors/markdown.ts` around lines 38 - 41, The function extractEntryPoint currently returns an empty string when the regex capture group is missing (using match[2] ?? ''), which can introduce empty entry points; update extractEntryPoint to return match[2] if present or null otherwise (i.e., return match && match[2] ? match[2] : null) and keep the signature string | null, referencing IMPORT_SYMBOL_RE and the extractEntryPoint function to locate the change. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (22)</summary><blockquote> <details> <summary>packages/journey-client/src/lib/qr-code/qr-code.ts (1)</summary><blockquote> `98-99`: **Avoid re-exporting internal callback base type from the QR module.** `BaseCallback` appears to be an internal callback implementation detail; exposing it here hardens an internal type into public API. Prefer exporting only `JourneyStep` from this module and keep callback internals private. <details> <summary>Suggested change</summary> ```diff export type { JourneyStep } from '../step.utils.js'; -export type { BaseCallback } from '../callbacks/base-callback.js'; ``` </details> Based on learnings, in `packages/journey-client`, callback classes are internal implementation details and the callbacks barrel intentionally only exports the base JourneyCallback class. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/qr-code/qr-code.ts` around lines 98 - 99, Remove the internal callback type export from the QR module: stop re-exporting BaseCallback from the qr-code barrel and only export JourneyStep; specifically delete or revert the export of the BaseCallback type (the line exporting "BaseCallback") so callback internals remain private while keeping the "JourneyStep" export intact. ``` </details> </blockquote></details> <details> <summary>tools/api-report/eslint.config.mjs (1)</summary><blockquote> `8-19`: **Remove no-op ESLint overrides to reduce config noise.** Line 8 through Line 19 add file-specific overrides with empty `rules` objects; they currently do nothing and make future intent harder to read. <details> <summary>♻️ Proposed cleanup</summary> ```diff ...baseConfig, - { - files: ['**/*.ts', '**/*.tsx', '**/*.js', '**/*.jsx'], - rules: {}, - }, - { - files: ['**/*.ts', '**/*.tsx'], - rules: {}, - }, - { - files: ['**/*.js', '**/*.jsx'], - rules: {}, - }, { files: ['**/*.json'], ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/api-report/eslint.config.mjs` around lines 8 - 19, The ESLint config contains three override objects with files arrays ('**/*.ts', '**/*.tsx', '**/*.js', '**/*.jsx'), ('**/*.ts', '**/*.tsx'), and ('**/*.js', '**/*.jsx') whose rules are empty (no-op) and add noise; remove these empty override entries from eslint.config.mjs so the config only contains meaningful rules/overrides, leaving any non-empty override objects intact and ensuring behavior is unchanged. ``` </details> </blockquote></details> <details> <summary>packages/oidc-client/src/__tests__/types-reexport.typecheck.ts (1)</summary><blockquote> `15-16`: **Comment wording looks stale.** If `ResponseType` is now exported, Line 15 should be updated to avoid misleading future readers. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/oidc-client/src/__tests__/types-reexport.typecheck.ts` around lines 15 - 16, The inline comment above the re-export list is stale: since ResponseType is now exported, update or remove the comment near the ResponseType entry in the __tests__/types-reexport.typecheck.ts re-export list so it no longer says "Should be re-exported but is not yet"; either change it to indicate it is exported (e.g., "Already re-exported") or delete the comment to avoid misleading readers. ``` </details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/fixtures/mock-package/src/index.ts (1)</summary><blockquote> `1-1`: **Prefer a non-empty fixture stub to avoid lint suppression/exclusions.** A tiny noop body/comment keeps fixture intent clear and lint-friendly. <details> <summary>Proposed tweak</summary> ```diff -export function journey() {} +export function journey(): void { + // fixture noop +} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/fixtures/mock-package/src/index.ts` at line 1, The exported stub function journey() is empty which triggers lint suppression/exclusions; update the journey function to contain a minimal non-empty body (e.g., a brief inline comment or a no-op statement) so the fixture clearly expresses intent and satisfies linters—locate the export function journey and replace the empty body with a tiny comment or simple statement (such as a noop) to make it non-empty. ``` </details> </blockquote></details> <details> <summary>nx.json (1)</summary><blockquote> `122-122`: **Consider narrowing the fixture exclude glob.** `tools/**/fixtures/**/*` is very broad; scoping it to the specific tool fixture roots would reduce accidental lint blind spots later. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@nx.json` at line 122, The exclude glob in the "exclude" array currently uses the very broad pattern "tools/**/fixtures/**/*"; narrow this by targeting the actual fixture roots for each tool instead of matching every nested file—update the value under the exclude key to specific patterns (e.g., "tools/<tool-name>/fixtures/**" or a small list of explicit tool fixture globs) so only known fixture directories are ignored and you avoid unintentionally skipping other files. ``` </details> </blockquote></details> <details> <summary>.github/workflows/ci.yml (1)</summary><blockquote> `71-73`: **Use a dynamic `$GITHUB_OUTPUT` delimiter to avoid accidental truncation.** If validator output ever contains `MAPPING_EOF` on its own line, the captured report can be cut early. <details> <summary>Proposed hardening</summary> ```diff - echo 'report<<MAPPING_EOF' >> "$GITHUB_OUTPUT" - echo "$OUTPUT" >> "$GITHUB_OUTPUT" - echo 'MAPPING_EOF' >> "$GITHUB_OUTPUT" + DELIM="MAPPING_EOF_$(date +%s%N)" + echo "report<<$DELIM" >> "$GITHUB_OUTPUT" + echo "$OUTPUT" >> "$GITHUB_OUTPUT" + echo "$DELIM" >> "$GITHUB_OUTPUT" ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 71 - 73, Replace the fixed MAPPING_EOF here with a dynamically generated delimiter to avoid accidental truncation: generate a random/unique token (e.g., via uuidgen, date+random, or openssl) into a variable (DELIM), use echo "report<<$DELIM" >> "$GITHUB_OUTPUT" to start the block, append the output as before, and then echo "$DELIM" >> "$GITHUB_OUTPUT" to close it; update the three occurrences in the snippet that reference MAPPING_EOF so they use the generated DELIM variable instead. ``` </details> </blockquote></details> <details> <summary>tools/api-report/src/resolve-entries.ts (1)</summary><blockquote> `9-21`: **Handle conditional/object `exports` targets instead of silently dropping them.** The function currently filters out non-string export targets, which excludes all conditional exports (objects with `types`, `import`, `default`, `require` keys). This produces incomplete API reports for packages that use modern conditional export patterns—all packages in this repository do (e.g., `@forgerock/protect`, `@forgerock/sdk-utilities`, `@forgerock/sdk-types`). The proposed approach correctly prioritizes the `types` field when present (ideal for generating `.d.ts` paths), then falls back to `import`, `default`, or `require` as needed. <details> <summary>Proposed refactor</summary> ```diff export interface EntryPoint { subpath: string; dtsPath: string; } +function pickExportTarget(target: unknown): string | undefined { + if (typeof target === 'string') return target; + if (!target || typeof target !== 'object') return undefined; + + const t = target as Record<string, unknown>; + // Prefer explicit types target when present. + return ( + (typeof t.types === 'string' && t.types) || + (typeof t.import === 'string' && t.import) || + (typeof t.default === 'string' && t.default) || + (typeof t.require === 'string' && t.require) || + undefined + ); +} + export function resolveEntryPoints( - exports: Record<string, string>, + exports: Record<string, unknown>, packageDir: string, ): EntryPoint[] { return Object.entries(exports) - .filter(([subpath, target]) => { + .flatMap(([subpath, target]) => { if (subpath === './package.json') return false; - if (typeof target !== 'string') return false; - return true; + const resolvedTarget = pickExportTarget(target); + if (!resolvedTarget) return []; + return [{ + subpath, + dtsPath: resolve(packageDir, resolvedTarget.replace(/\.js$/, '.d.ts')), + }]; }) - .map(([subpath, target]) => ({ - subpath, - dtsPath: resolve(packageDir, target.replace(/\.js$/, '.d.ts')), - })); + ; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/api-report/src/resolve-entries.ts` around lines 9 - 21, The current map in the function returning EntryPoint[] (processing exports and packageDir) drops non-string targets; change it to accept object/conditional export targets by resolving a string target from objects using the priority keys ['types', 'types', 'import', 'default', 'require'] (prefer 'types' first, then 'import', then 'default', then 'require') before building dtsPath, keep filtering out './package.json', and when you have the resolved string replace /\.js$/ with '.d.ts' to form dtsPath; ensure the logic is applied where Object.entries(exports) is filtered/mapped (the subpath/target handling) so conditional exports produce EntryPoint entries instead of being silently skipped. ``` </details> </blockquote></details> <details> <summary>tools/api-report/src/main.spec.ts (1)</summary><blockquote> `28-33`: **Avoid brittle exact-length assertion for package exports.** Line 32 (`toHaveLength(2)`) will fail on legitimate future export additions. Prefer asserting required subpaths and forbidden ones only. <details> <summary>Proposed refactor</summary> ```diff it('should resolve device-client with correct entry count', () => { const info = resolvePackageInfo('packages/device-client'); expect(info.packageName).toBe('@forgerock/device-client'); - // device-client has ".", "./types" (./package.json is filtered out) - expect(info.entries).toHaveLength(2); + expect(info.entries.find((e) => e.subpath === '.')).toBeDefined(); + expect(info.entries.find((e) => e.subpath === './types')).toBeDefined(); + expect(info.entries.find((e) => e.subpath === './package.json')).toBeUndefined(); }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/api-report/src/main.spec.ts` around lines 28 - 33, The test currently asserts an exact entries length which is brittle; change the assertion in the test that calls resolvePackageInfo('packages/device-client') (variable info) to instead assert presence of the required export subpaths (e.g., expect(info.entries).toContain('.') and expect(info.entries).toContain('./types')) and assert that filtered/forbidden paths are not present (e.g., expect(info.entries).not.toContain('./package.json')), removing the toHaveLength(2) check so future valid exports won't break the test. ``` </details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/eslint.config.mjs (1)</summary><blockquote> `8-22`: **Remove no-op ESLint override blocks.** Lines 8–22 don’t change behavior (`rules: {}` only) and can be dropped to keep config maintainable. <details> <summary>Proposed cleanup</summary> ```diff ...baseConfig, - { - files: ['**/*.ts', '**/*.tsx', '**/*.js', '**/*.jsx'], - // Override or add rules here - rules: {}, - }, - { - files: ['**/*.ts', '**/*.tsx'], - // Override or add rules here - rules: {}, - }, - { - files: ['**/*.js', '**/*.jsx'], - // Override or add rules here - rules: {}, - }, { files: ['**/*.json'], ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/eslint.config.mjs` around lines 8 - 22, Remove the no-op ESLint override blocks that only contain empty rules objects—specifically the three override entries with files: ['**/*.ts','**/*.tsx','**/*.js','**/*.jsx'], ['**/*.ts','**/*.tsx'], and ['**/*.js','**/*.jsx']—because they don't change behavior; delete those override objects from eslint.config.mjs so the configuration is leaner and only contains meaningful rule overrides. ``` </details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/fixer.ts (1)</summary><blockquote> `100-107`: **Minor inconsistency in nullish coalescing fallbacks.** Line 103 uses `?? 0` while line 106 uses `?? 1`. Both are guarded by the filter on line 102 that checks `f.lineNumber !== undefined`, so this is functionally correct, but the inconsistency could be confusing during maintenance. <details> <summary>🧹 Suggested cleanup</summary> ```diff const linesToRemove = new Set( removals .filter((f) => { - const lineIndex = (f.lineNumber ?? 0) - 1; + const lineIndex = f.lineNumber! - 1; return lineIndex >= 0 && lineIndex < lines.length && !isProtectedRow(lines[lineIndex]); }) - .map((f) => (f.lineNumber ?? 1) - 1), + .map((f) => f.lineNumber! - 1), ); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/fixer.ts` around lines 100 - 107, The code in the linesToRemove computation uses inconsistent nullish fallbacks: the filter guards f.lineNumber, but the mapper uses (f.lineNumber ?? 1) - 1 while the filter earlier uses ?? 0; update the mapper inside the .map to use the same fallback as above—change the map expression to use (f.lineNumber ?? 0) - 1 so removals, f.lineNumber, linesToRemove, isProtectedRow and lines are consistent and less confusing to maintain. ``` </details> </blockquote></details> <details> <summary>package.json (1)</summary><blockquote> `29-29`: **Consider cross-platform compatibility for `api-report:fix` script.** The inline bash `for` loop won't work on Windows Command Prompt or PowerShell. If Windows developer support is needed, consider using a Node script or `npm-run-all`/`concurrently` to iterate packages. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@package.json` at line 29, The current "api-report:fix" script uses a bash for-loop which is not cross-platform; replace it with a cross-platform approach by creating a small Node script (e.g., tools/api-report/fix.js) or use a cross-platform task runner (npm-run-all or concurrently) to iterate the packages array ["packages/journey-client","packages/oidc-client","packages/device-client","packages/davinci-client"] and invoke "pnpm tsx tools/api-report/src/main.ts <pkg> --fix" for each; then update the package.json "api-report:fix" script to call the Node script (node tools/api-report/fix.js) or the chosen runner so Windows shells will execute it correctly. ``` </details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/extractors/new-sdk.test.ts (1)</summary><blockquote> `7-67`: **Add one cross-package re-export case to this suite.** Every fixture export here is declared locally, but the real client barrels are dominated by re-exports from other packages. Without at least one external-module case, this suite will not catch the resolver path `extractNewSdkExports` uses against files like `packages/journey-client/src/types.ts`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/extractors/new-sdk.test.ts` around lines 7 - 67, The test suite for extractNewSdkExports lacks a case covering cross-package re-exports; add one by extending tools/interface-mapping-validator/src/extractors/new-sdk.test.ts to include a fixture entry that simulates an external-module re-export and assert extractNewSdkExports returns the re-exported symbol with the correct packageName and importPath (use symbols like extractNewSdkExports, FIXTURE_DIR, and the test's exported symbol name) so the resolver path for external-module re-exports is exercised and validated. ``` </details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/integration.test.ts (1)</summary><blockquote> `100-107`: **Variable shadowing: `packageDirs` redeclared.** `packageDirs` is declared both at module scope (line 35) and inside this `beforeAll` (line 102). The inner declaration shadows the outer one. Since they resolve to the same value, you can simply use the outer variable. <details> <summary>♻️ Proposed fix</summary> ```diff beforeAll(() => { const legacyP = resolve(TOOL_ROOT, LEGACY_SDK_INDEX_PATH); - const packageDirs = NEW_SDK_PACKAGES.map((p) => resolve(WORKSPACE_ROOT, p)); legacy = extractLegacyExports(legacyP); newSdk = extractNewSdkExports(packageDirs); generated = generateSections(legacy, newSdk, SYMBOL_MAP, PACKAGE_MAP); }, 60_000); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/integration.test.ts` around lines 100 - 107, The inner declaration of packageDirs inside the beforeAll callback shadows the module-scoped packageDirs; remove the local "const packageDirs = ..." and instead assign to the existing outer variable (e.g., packageDirs = NEW_SDK_PACKAGES.map(p => resolve(WORKSPACE_ROOT, p))); update the beforeAll to use the module-scope packageDirs so extractNewSdkExports(packageDirs) uses the same variable and no shadowing occurs. ``` </details> </blockquote></details> <details> <summary>tools/api-report/src/fixer.spec.ts (1)</summary><blockquote> `37-46`: **Integration test depends on workspace file system.** This test reads the actual workspace structure and could fail if the `sdk-effects/logger` package structure changes. Consider documenting this dependency or adding a skip condition if the path doesn't exist. The test provides valuable real-world validation, so keeping it is reasonable. Just be aware this test may need updates if the package structure evolves. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/api-report/src/fixer.spec.ts` around lines 37 - 46, The integration test 'should resolve sdk-effects subpackage from real workspace' depends on actual workspace files; update the spec to guard against missing paths by checking the workspace/package existence before running assertions: compute workspaceRoot and sourcePath (as already done), then if fs.existsSync(sourcePath) or fs.existsSync(path.join(workspaceRoot, 'packages/sdk-effects/logger/package.json')) is false, call test.skip() or return early to skip the test; otherwise proceed to call resolveSourcePackage(sourcePath, workspaceRoot) and assert the result, so the test won't fail on CI or machines where the package structure is different. ``` </details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/extractors/markdown.ts (1)</summary><blockquote> `149-154`: **Consider using a Set for deduplication efficiency.** The current `includes()` check is O(n) per entry point. While unlikely to be a bottleneck given typical document sizes, using a Set in the state would be more idiomatic. <details> <summary>♻️ Proposed optimization</summary> Change the state to use a `Set<string>` for `entryPoints` tracking: ```diff type ParseState = { readonly mappings: readonly DocumentedMapping[]; - readonly entryPoints: readonly string[]; + readonly entryPointsSet: ReadonlySet<string>; readonly currentSection: SectionInfo | null; readonly inCodeBlock: boolean; readonly headerRowSeen: boolean; }; const initialState: ParseState = { mappings: [], - entryPoints: [], + entryPointsSet: new Set(), // ... }; ``` Then at the end, convert to array: `entryPoints: [...finalState.entryPointsSet]` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/extractors/markdown.ts` around lines 149 - 154, The current deduplication uses state.entryPoints.includes(...) which is O(n) per check; change the state to track entry points as a Set<string> (e.g., entryPointsSet) and update all uses to add into and check against that Set; specifically, when building newEntryPoints (from extractEntryPoint and firstCol) use entryPointsSet.has(...) instead of state.entryPoints.includes(...), add new unique entries with entryPointsSet.add(...), and finally convert the Set back to an array where the state needs an array (e.g., entryPoints: [...entryPointsSet]) and update any references to state.entryPoints to use the Set variant (or a thin getter) so uniqueness checks and inserts are O(1). ``` </details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/differ.ts (2)</summary><blockquote> `76-84`: **Regex executed repeatedly in map chain.** The regex is executed for each callback mapping. For large mapping lists, this could be optimized by extracting the parsing logic or pre-computing the new symbols. This is a minor performance consideration and likely not impactful given typical document sizes. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/differ.ts` around lines 76 - 84, The map currently runs the regex /import\s+(?:type\s+)?{\s*(\w+)\s*}/ for every entry when building documentedCallbackNewSymbols from callbackMappings; extract that regex and the parsing logic into a single reusable constant or helper (e.g., const importNameRe = /.../ or a function parseImportName) and then iterate callbackMappings once (for..of or map using the shared regex/function) to add parsed names to the Set, referencing documentedCallbackNewSymbols, callbackMappings, and each mapping's newImport to locate the change. ``` </details> --- `190-190`: **Duplicate regex constant.** `IMPORT_SYMBOL_RE` is defined identically in `extractors/markdown.ts` (line 5). Consider extracting it to a shared constants module to avoid duplication. <details> <summary>♻️ Proposed refactor</summary> Create a shared constants file: ```typescript // tools/interface-mapping-validator/src/constants.ts export const IMPORT_SYMBOL_RE = /import\s+(?:type\s+)?{\s*(\w+)\s*}\s+from\s+['"]([^'"]+)['"]/; ``` Then import from both `differ.ts` and `extractors/markdown.ts`. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/differ.ts` at line 190, Duplicate regex IMPORT_SYMBOL_RE is defined in multiple places; extract it into a shared constants module (e.g., constants.ts) and import it where needed (currently used in differ.ts and extractors/markdown.ts). Move the regex (consider using the more complete form /import\s+(?:type\s+)?{\s*(\w+)\s*}\s+from\s+['"]([^'"]+)['"]/) into the new constant EXPORT, update both files to import IMPORT_SYMBOL_RE from that module, and remove the local duplicate definitions so there is a single source of truth. ``` </details> </blockquote></details> <details> <summary>tools/api-report/README.md (3)</summary><blockquote> `56-67`: **Add language specifier to fenced code block.** The markdownlint tool flags this block as missing a language specifier. For plain text output, use `text` or `plaintext`. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text x . - 2 error(s) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@tools/api-report/README.mdaround lines 56 - 67, The fenced code block that
starts with triple backticks and contains the plain-text report ("x . - 2
error(s) ... Fix all packages: pnpm api-report:fix") is missing a language
specifier; update the opening fence fromtotext (or ```plaintext) so
markdownlint recognizes it as plain text and the block passes linting.</details> --- `102-113`: **Add language specifier to architecture diagram block.** For ASCII diagrams, use `text` or `plaintext` to satisfy markdownlint. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text src/ ├── resolve-entries.ts # package.json exports → .d.ts entry points ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@tools/api-report/README.mdaround lines 102 - 113, Update the fenced code
block containing the ASCII architecture diagram so it includes a language
specifier (use "text" or "plaintext") after the opening triple backticks; locate
the block that begins withand the "src/" tree diagram and change it totext (or ```plaintext) to satisfy markdownlint.</details> --- `117-127`: **Add language specifier to pipeline diagram block.** <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text api-extractor message (ae-forgotten-export) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@tools/api-report/README.mdaround lines 117 - 127, The fenced code block
used for the pipeline diagram lacks a language specifier; update the opening
fence that currently readstotext so the block header becomes ```text
and the diagram lines (starting with "api-extractor message
(ae-forgotten-export)" and the indented arrows) are treated as plain text.</details> </blockquote></details> <details> <summary>tools/interface-mapping-validator/src/generator.ts (1)</summary><blockquote> `213-214`: **Verbose null type assertion pattern.** The pattern `null as string | null` appears multiple times. Consider defining a helper type or using a more concise approach. This is a minor style observation. The current approach works correctly, but the explicit casts could be avoided by letting TypeScript infer the union type from the return object structure, or by defining a result type alias. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/generator.ts` around lines 213 - 214, The explicit casts `null as string | null` are verbose; simplify by giving the function a declared return type or a small alias (e.g., `type MappingResult = { qr: QRType; pkg: PkgType; unmapped: string | null }`) and then return `unmapped: null` (or just return the object without casts) so TypeScript infers the union; locate the return that creates `{ qr, pkg, unmapped: null as string | null }` in generator.ts and replace the casted `null` with a plain `null` after declaring the appropriate return type or alias for the result shape. ``` </details> </blockquote></details> <details> <summary>packages/davinci-client/api-report/davinci-client.types.api.md (1)</summary><blockquote> `221-236`: **Consider replacing the inferred `davinci()` client shape with a named public interface.** This report still has to inline a huge RTK Query-heavy return object because the exported alias is `Awaited<ReturnType<typeof davinci>>`. Flipping that around—define a named client type/interface and annotate `davinci()` with it—would make the public surface much more stable and reviewable. Also applies to: 527-528 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/api-report/davinci-client.types.api.md` around lines 221 - 236, Create and export a named interface (e.g. DaVinciClient) that describes the full public return shape currently inferred for davinci (methods like subscribe, externalIdp, flow, next, resume, start and their signatures), then change the davinci function signature to return Promise<DaVinciClient> (or the appropriate generic variant) instead of relying on Awaited<ReturnType<typeof davinci>>; update the other two places noted (lines referenced in the comment) to use this new DaVinciClient type so the public API surface is stable and reviewable. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@packages/device-client/api-report/device-client.api.md:
- Around line 282-296: Update the Hardware type so numeric fields use the
generic number type rather than hard-coded literals: change properties on the
Hardware type (storage, memory, cpu, display.width, display.height,
display.orientation, and camera.numberOfCameras) from their current literal
values to type number in the type definition (see the Hardware type in
updateDeviceProfile.types.ts) so the type accepts any numeric device
configuration.In
@packages/device-client/api-report/device-client.types.api.md:
- Around line 234-244: The exported types DeviceMetadata, Hardware, Network,
Telephony, and DeviceProfile in updateDeviceProfile.types.ts currently use
literal-constrained values (e.g., version: 34, jailBreakScore: 0, alias:
'Test3', storage: 5939, memory: 2981, cpu: 4, connected: true,
networkCountryIso: 'us', carrierName: 'T-Mobile', recoveryCodes: []) which break
the real API contract; update these type definitions to use flexible primitives
(number, string, boolean, string[]) matching the ProfileDevice shape (or remove
the incorrect types and re-export or alias ProfileDevice) so consumers can pass
valid values when calling the updateDeviceProfile mutation and ensure
DeviceMetadata/Hardware/Network/Telephony/DeviceProfile align with
ProfileDevice.In
@packages/journey-client/api-report/journey-client.api.md:
- Around line 30-109: The package is accidentally re-exporting internal concrete
callback implementations and helpers (e.g., AttributeInputCallback,
ChoiceCallback, ConfirmationCallback, DeviceProfileCallback, createCallback and
any InternalJourneyClientConfig) from the public barrel; remove those concrete
class and helper exports so only the abstract/public callback surface
(BaseCallback, CallbackFactory/Callback type aliases and any intentionally
public types like CallbackType) remain exported from the journey-client
entrypoint; update the barrel/index export list and regenerate the API snapshot
so only BaseCallback and the intended public types are present and the internal
classes (AttributeInputCallback, ChoiceCallback, ConfirmationCallback,
DeviceProfileCallback, createCallback, InternalJourneyClientConfig, etc.) are no
longer exported.In
@packages/journey-client/src/index.ts:
- Line 9: The public re-export in index.ts currently exposes concrete callback
classes from types.ts; update the public API so only BaseCallback and the
factory are exported. Move or stop exporting concrete implementations (e.g.,
NameCallback, ChoiceCallback, TextCallback, etc.) out of types.ts (place them in
an internal module like callbacks/*.ts or mark them non-exported), ensure
types.ts only exports BaseCallback and the factory function (e.g.,
createCallbackFactory or similar), and keep index.ts exporting only those public
symbols so the concrete classes are not re-exported from the package root.In
@packages/journey-client/src/lib/recovery-codes/recovery-codes.ts:
- Around line 74-76: The file is re-exporting internal callback types
(TextOutputCallback and BaseCallback) which should not be part of the public
API; remove the two export type lines for TextOutputCallback and BaseCallback
from recovery-codes.ts so only public types remain (keep export type {
JourneyStep } and, if the public API requires a callback type, re-export only
the public JourneyCallback class/type from the callbacks barrel instead of
internal types). Ensure any callers use the public JourneyCallback symbol (not
TextOutputCallback/BaseCallback) and update imports if necessary.In
@packages/journey-client/src/lib/webauthn/webauthn.ts:
- Around line 510-512: Remove the concrete callback class type exports from the
WebAuthn public surface: stop exporting HiddenValueCallback, MetadataCallback,
and TextOutputCallback from webauthn.ts so these internal implementation types
remain private; update any internal imports to reference the original callback
modules (e.g., ../callbacks/hidden-value-callback.js, metadata-callback.js,
text-output-callback.js) without re-exporting them from the WebAuthn public
entry point, and if public-facing abstractions are required expose only stable
interfaces/types (not the concrete callback classes).In
@packages/oidc-client/src/__tests__/types-reexport.typecheck.ts:
- Around line 1-4: The current types-only test (types-reexport.typecheck.ts)
provides type coverage but not runtime coverage; add a tiny runtime smoke test
that imports the same module (require or import '@forgerock/oidc-client/types')
and asserts that the runtime export is an object and contains the expected
export keys (the same symbols you expect to re-export), so the file contributes
to test coverage; alternatively, if you prefer not to run runtime checks,
explicitly add this file pattern to the coverage exclude list in the test config
so type-only files do not cause patch-coverage failures.In
@tools/api-report/src/fixer.ts:
- Around line 165-180: The multi-line insert path (multiLineBlockRegex /
multiLineMatch) blindly appends ",\n " which creates a double comma if the
existing block already has a trailing comma; change the logic in the multi-line
branch (the code that builds updated from beforeClose.trimEnd() and closeAndFrom
using symbolName) to detect whether beforeClose (or its last non-whitespace
character) already ends with a comma and only insert "\n " (no leading comma)
when it does, otherwise insert ",\n " as now; update the construction of
updated accordingly so symbolName is added without producing a double comma.- Around line 70-95: The determineExportKind function currently returns 'type'
by default and inserts raw symbolName into RegExp, causing misclassification and
regex errors; update determineExportKind to return the union 'type' | 'value' |
'unknown', change the default return to 'unknown', and replace every use of
symbolName in RegExp constructions with escapeRegex(symbolName) (use the
existing escapeRegex utility) inside determineExportKind; then update the call
site that consumes determineExportKind (the loop that builds exports/skipped
lists) to treat kind === 'unknown' by skipping those symbols and adding them to
the skipped list instead of emitting a type export.In
@tools/api-report/src/main.ts:
- Around line 126-148: The current --fix branch mutates src/types.ts but then
falls through with exit code 0, allowing automation to proceed before
declaration files are re-generated; after calling applyFixes(forgottenExports,
typesFilePath, WORKSPACE_ROOT) you should fail the process when any fixes were
applied or remaining errors exist so callers know to rebuild and re-run: inspect
the returned fixed and skipped arrays and totalErrors/forgottenExports, log the
summary as already done, and if fixed.length > 0 || skipped.length > 0 ||
totalErrors > 0 call process.exit(1) (with a clear message about needing rebuild
and re-run); only return exit 0 when no fixes were made and there are no
remaining errors.In
@tools/interface-mapping-validator/src/differ.test.ts:
- Around line 248-399: The tests flag every symbol ending with "Callback" from
'@forgerock/journey-client/types' as public; update the callback-detection logic
in diff so it only treats intentionally public base callbacks (or an explicit
allowlist) as candidates for missing/stale-callback findings. Specifically,
change the code that currently filters exports by importPath and symbol suffix
(used by diff and the CALLBACKS handling that produces categories
'missing-callback' and 'stale-callback') to consult a configurable allowlist of
public callback symbols (or detect only the documented base callback names) and
ensure documented rename mappings (e.g., legacySymbol -> newImport rows in
SECTIONS.CALLBACKS) are respected so renamed callbacks like
FRCallback->BaseCallback are not flagged.In
@tools/interface-mapping-validator/src/extractors/new-sdk.test.ts:
- Line 5: The test uses __dirname to build FIXTURE_DIR (const FIXTURE_DIR =
resolve(__dirname, '../fixtures/mock-package')) which fails in ESM; replace
usage of __dirname with import.meta.dirname (or the provided polyfill) when
constructing FIXTURE_DIR so the module runs under ESM—update the reference where
FIXTURE_DIR is defined and any resolve(...) calls that depend on __dirname (look
for FIXTURE_DIR and resolve usage in new-sdk.test.ts) to use import.meta.dirname
instead.In
@tools/interface-mapping-validator/src/extractors/new-sdk.ts:
- Around line 25-33: The current resolveSourcePath function should not guess a
“src” location from a dist path and let callers silently ignore missing files;
instead, change it to use the package.exports mapping (or the package.json
declaration target) to locate the public .d.ts path and resolve that exact file,
and if no declaration target exists or the mapped file is missing, surface a
hard failure (throw or return an explicit error) rather than returning
undefined/allowing callers to return an empty result; apply the same fix pattern
to the other similar logic referenced around the second extractor block (the
code at the other occurrence) so both places consult package.json exports for
the declaration file and explicitly fail when the expected declaration is
absent.In
@tools/interface-mapping-validator/src/fixtures/legacy-sample.d.ts:
- Around line 1-5: The import statements for FRAuth, CallbackType, NameCallback,
Step, FailureDetail, Config, and ConfigOptions use extensionless paths (e.g.,
'./fake/fr-auth'); update each import to include the proper file extension
(e.g., './fake/fr-auth.ts' or '.d.ts' as appropriate) so they comply with the
import/extensions lint rule—locate the imports for FRAuth, CallbackType,
NameCallback, Step/FailureDetail, and Config/ConfigOptions in legacy-sample.d.ts
and append the correct extension to each module path.In
@tools/interface-mapping-validator/src/main.ts:
- Around line 58-115: The client-import guard validateClientImportsOnly() is
only run before edits, so after the --generate and --fix flows the script may
exit 0 even if new forbidden imports were introduced; re-run
validateClientImportsOnly() after the generation block (just before the final
process.exit(...) at the end of the shouldGenerate branch) and likewise after
the --fix branch's changes (before its process.exit(...)), and if it reports any
violations return a non-zero exit code; reference the validateClientImportsOnly
function and the process.exit(...) calls in the generate and fix branches to
locate where to invoke the re-check and gate the final exit.In
@tools/interface-mapping-validator/src/validate-client-imports.spec.ts:
- Around line 4-10: The test's ALLOWED_PACKAGES array includes a non-client
package '@forgerock/protect' which violates the client-only rule; update the
ALLOWED_PACKAGES constant in validate-client-imports.spec.ts by removing
'@forgerock/protect' (or replacing it with the correct '@forgerock/*-client'
package if intended) so the list only contains packages that end with '-client',
ensuring the ALLOWED_PACKAGES symbol reflects the validator's client-import-only
constraint.
Minor comments:
In@MIGRATION.md:
- Around line 68-74: The snippet calls Config.set(...) but Config is not
imported; update the import from '@forgerock/javascript-sdk' to include Config
(or add a separate import for Config) so that Config, along with the existing
FRAuth and StepType identifiers, is correctly imported before calling Config.set
in the snippet.In
@packages/journey-client/src/__tests__/types-reexport.typecheck.ts:
- Line 18: Update the stale inline comment that currently reads "These should be
re-exported but are not yet" to reflect the current behaviour enforced by the
test (e.g., "These types are expected to be re-exported and are validated by
this test"), so the comment matches that the types are now required to be
available; locate the existing comment string on the line with that message and
replace it with the new, accurate phrasing.In
@tools/interface-mapping-validator/package.json:
- Around line 16-24: Remove the accidental runtime dependency: delete the
"vitest" entry from the "dependencies" section in package.json and keep the
existing "vitest" entry only under "devDependencies"; ensure the duplicate key
under "dependencies" (the string "vitest") is removed so tests remain available
at dev time but not bundled as a production dependency.In
@tools/interface-mapping-validator/README.md:
- Around line 106-122: Update the fenced code block containing the src/ tree in
README.md to include a language specifier (e.g., change the opening ``` tothat shows the list (the block with "src/" and the extractors/new-sdk.ts/legacy.ts lines) to use ```text as the opening fence and keep the rest of the block identical. In `@tools/interface-mapping-validator/src/extractors/markdown.ts`: - Around line 38-41: The function extractEntryPoint currently returns an empty string when the regex capture group is missing (using match[2] ?? ''), which can introduce empty entry points; update extractEntryPoint to return match[2] if present or null otherwise (i.e., return match && match[2] ? match[2] : null) and keep the signature string | null, referencing IMPORT_SYMBOL_RE and the extractEntryPoint function to locate the change. In `@tools/interface-mapping-validator/src/validate-client-imports.ts`: - Around line 12-15: The current importRegex and the line filtering in the for (const line of lines) loop only match single-quoted imports, causing double-quoted imports to be skipped; update importRegex to accept either single or double quotes (e.g. use a pattern like /from\s+['"](`@forgerock`\/[^'"]+)['"]/g) and replace the simple string check if (!line.includes("from '@forgerock/")) continue; with a regex test against importRegex (or a separate /from\s+['"]@forgerock\// check) so both " and ' forms are detected by validate-client-imports (ensure captures still return the module name in the same group). --- Nitpick comments: In @.github/workflows/ci.yml: - Around line 71-73: Replace the fixed MAPPING_EOF here with a dynamically generated delimiter to avoid accidental truncation: generate a random/unique token (e.g., via uuidgen, date+random, or openssl) into a variable (DELIM), use echo "report<<$DELIM" >> "$GITHUB_OUTPUT" to start the block, append the output as before, and then echo "$DELIM" >> "$GITHUB_OUTPUT" to close it; update the three occurrences in the snippet that reference MAPPING_EOF so they use the generated DELIM variable instead. In `@nx.json`: - Line 122: The exclude glob in the "exclude" array currently uses the very broad pattern "tools/**/fixtures/**/*"; narrow this by targeting the actual fixture roots for each tool instead of matching every nested file—update the value under the exclude key to specific patterns (e.g., "tools/<tool-name>/fixtures/**" or a small list of explicit tool fixture globs) so only known fixture directories are ignored and you avoid unintentionally skipping other files. In `@package.json`: - Line 29: The current "api-report:fix" script uses a bash for-loop which is not cross-platform; replace it with a cross-platform approach by creating a small Node script (e.g., tools/api-report/fix.js) or use a cross-platform task runner (npm-run-all or concurrently) to iterate the packages array ["packages/journey-client","packages/oidc-client","packages/device-client","packages/davinci-client"] and invoke "pnpm tsx tools/api-report/src/main.ts <pkg> --fix" for each; then update the package.json "api-report:fix" script to call the Node script (node tools/api-report/fix.js) or the chosen runner so Windows shells will execute it correctly. In `@packages/davinci-client/api-report/davinci-client.types.api.md`: - Around line 221-236: Create and export a named interface (e.g. DaVinciClient) that describes the full public return shape currently inferred for davinci (methods like subscribe, externalIdp, flow, next, resume, start and their signatures), then change the davinci function signature to return Promise<DaVinciClient> (or the appropriate generic variant) instead of relying on Awaited<ReturnType<typeof davinci>>; update the other two places noted (lines referenced in the comment) to use this new DaVinciClient type so the public API surface is stable and reviewable. In `@packages/journey-client/src/lib/qr-code/qr-code.ts`: - Around line 98-99: Remove the internal callback type export from the QR module: stop re-exporting BaseCallback from the qr-code barrel and only export JourneyStep; specifically delete or revert the export of the BaseCallback type (the line exporting "BaseCallback") so callback internals remain private while keeping the "JourneyStep" export intact. In `@packages/oidc-client/src/__tests__/types-reexport.typecheck.ts`: - Around line 15-16: The inline comment above the re-export list is stale: since ResponseType is now exported, update or remove the comment near the ResponseType entry in the __tests__/types-reexport.typecheck.ts re-export list so it no longer says "Should be re-exported but is not yet"; either change it to indicate it is exported (e.g., "Already re-exported") or delete the comment to avoid misleading readers. In `@tools/api-report/eslint.config.mjs`: - Around line 8-19: The ESLint config contains three override objects with files arrays ('**/*.ts', '**/*.tsx', '**/*.js', '**/*.jsx'), ('**/*.ts', '**/*.tsx'), and ('**/*.js', '**/*.jsx') whose rules are empty (no-op) and add noise; remove these empty override entries from eslint.config.mjs so the config only contains meaningful rules/overrides, leaving any non-empty override objects intact and ensuring behavior is unchanged. In `@tools/api-report/README.md`: - Around line 56-67: The fenced code block that starts with triple backticks and contains the plain-text report ("x . - 2 error(s) ... Fix all packages: pnpm api-report:fix") is missing a language specifier; update the opening fence from ``` to ```text (or ```plaintext) so markdownlint recognizes it as plain text and the block passes linting. - Around line 102-113: Update the fenced code block containing the ASCII architecture diagram so it includes a language specifier (use "text" or "plaintext") after the opening triple backticks; locate the block that begins with ``` and the "src/" tree diagram and change it to ```text (or ```plaintext) to satisfy markdownlint. - Around line 117-127: The fenced code block used for the pipeline diagram lacks a language specifier; update the opening fence that currently reads ``` to ```text so the block header becomes ```text and the diagram lines (starting with "api-extractor message (ae-forgotten-export)" and the indented arrows) are treated as plain text. In `@tools/api-report/src/fixer.spec.ts`: - Around line 37-46: The integration test 'should resolve sdk-effects subpackage from real workspace' depends on actual workspace files; update the spec to guard against missing paths by checking the workspace/package existence before running assertions: compute workspaceRoot and sourcePath (as already done), then if fs.existsSync(sourcePath) or fs.existsSync(path.join(workspaceRoot, 'packages/sdk-effects/logger/package.json')) is false, call test.skip() or return early to skip the test; otherwise proceed to call resolveSourcePackage(sourcePath, workspaceRoot) and assert the result, so the test won't fail on CI or machines where the package structure is different. In `@tools/api-report/src/main.spec.ts`: - Around line 28-33: The test currently asserts an exact entries length which is brittle; change the assertion in the test that calls resolvePackageInfo('packages/device-client') (variable info) to instead assert presence of the required export subpaths (e.g., expect(info.entries).toContain('.') and expect(info.entries).toContain('./types')) and assert that filtered/forbidden paths are not present (e.g., expect(info.entries).not.toContain('./package.json')), removing the toHaveLength(2) check so future valid exports won't break the test. In `@tools/api-report/src/resolve-entries.ts`: - Around line 9-21: The current map in the function returning EntryPoint[] (processing exports and packageDir) drops non-string targets; change it to accept object/conditional export targets by resolving a string target from objects using the priority keys ['types', 'types', 'import', 'default', 'require'] (prefer 'types' first, then 'import', then 'default', then 'require') before building dtsPath, keep filtering out './package.json', and when you have the resolved string replace /\.js$/ with '.d.ts' to form dtsPath; ensure the logic is applied where Object.entries(exports) is filtered/mapped (the subpath/target handling) so conditional exports produce EntryPoint entries instead of being silently skipped. In `@tools/interface-mapping-validator/eslint.config.mjs`: - Around line 8-22: Remove the no-op ESLint override blocks that only contain empty rules objects—specifically the three override entries with files: ['**/*.ts','**/*.tsx','**/*.js','**/*.jsx'], ['**/*.ts','**/*.tsx'], and ['**/*.js','**/*.jsx']—because they don't change behavior; delete those override objects from eslint.config.mjs so the configuration is leaner and only contains meaningful rule overrides. In `@tools/interface-mapping-validator/src/differ.ts`: - Around line 76-84: The map currently runs the regex /import\s+(?:type\s+)?{\s*(\w+)\s*}/ for every entry when building documentedCallbackNewSymbols from callbackMappings; extract that regex and the parsing logic into a single reusable constant or helper (e.g., const importNameRe = /.../ or a function parseImportName) and then iterate callbackMappings once (for..of or map using the shared regex/function) to add parsed names to the Set, referencing documentedCallbackNewSymbols, callbackMappings, and each mapping's newImport to locate the change. - Line 190: Duplicate regex IMPORT_SYMBOL_RE is defined in multiple places; extract it into a shared constants module (e.g., constants.ts) and import it where needed (currently used in differ.ts and extractors/markdown.ts). Move the regex (consider using the more complete form /import\s+(?:type\s+)?{\s*(\w+)\s*}\s+from\s+['"]([^'"]+)['"]/) into the new constant EXPORT, update both files to import IMPORT_SYMBOL_RE from that module, and remove the local duplicate definitions so there is a single source of truth. In `@tools/interface-mapping-validator/src/extractors/markdown.ts`: - Around line 149-154: The current deduplication uses state.entryPoints.includes(...) which is O(n) per check; change the state to track entry points as a Set<string> (e.g., entryPointsSet) and update all uses to add into and check against that Set; specifically, when building newEntryPoints (from extractEntryPoint and firstCol) use entryPointsSet.has(...) instead of state.entryPoints.includes(...), add new unique entries with entryPointsSet.add(...), and finally convert the Set back to an array where the state needs an array (e.g., entryPoints: [...entryPointsSet]) and update any references to state.entryPoints to use the Set variant (or a thin getter) so uniqueness checks and inserts are O(1). In `@tools/interface-mapping-validator/src/extractors/new-sdk.test.ts`: - Around line 7-67: The test suite for extractNewSdkExports lacks a case covering cross-package re-exports; add one by extending tools/interface-mapping-validator/src/extractors/new-sdk.test.ts to include a fixture entry that simulates an external-module re-export and assert extractNewSdkExports returns the re-exported symbol with the correct packageName and importPath (use symbols like extractNewSdkExports, FIXTURE_DIR, and the test's exported symbol name) so the resolver path for external-module re-exports is exercised and validated. In `@tools/interface-mapping-validator/src/fixer.ts`: - Around line 100-107: The code in the linesToRemove computation uses inconsistent nullish fallbacks: the filter guards f.lineNumber, but the mapper uses (f.lineNumber ?? 1) - 1 while the filter earlier uses ?? 0; update the mapper inside the .map to use the same fallback as above—change the map expression to use (f.lineNumber ?? 0) - 1 so removals, f.lineNumber, linesToRemove, isProtectedRow and lines are consistent and less confusing to maintain. In `@tools/interface-mapping-validator/src/fixtures/mock-package/src/index.ts`: - Line 1: The exported stub function journey() is empty which triggers lint suppression/exclusions; update the journey function to contain a minimal non-empty body (e.g., a brief inline comment or a no-op statement) so the fixture clearly expresses intent and satisfies linters—locate the export function journey and replace the empty body with a tiny comment or simple statement (such as a noop) to make it non-empty. In `@tools/interface-mapping-validator/src/generator.ts`: - Around line 213-214: The explicit casts `null as string | null` are verbose; simplify by giving the function a declared return type or a small alias (e.g., `type MappingResult = { qr: QRType; pkg: PkgType; unmapped: string | null }`) and then return `unmapped: null` (or just return the object without casts) so TypeScript infers the union; locate the return that creates `{ qr, pkg, unmapped: null as string | null }` in generator.ts and replace the casted `null` with a plain `null` after declaring the appropriate return type or alias for the result shape. In `@tools/interface-mapping-validator/src/integration.test.ts`: - Around line 100-107: The inner declaration of packageDirs inside the beforeAll callback shadows the module-scoped packageDirs; remove the local "const packageDirs = ..." and instead assign to the existing outer variable (e.g., packageDirs = NEW_SDK_PACKAGES.map(p => resolve(WORKSPACE_ROOT, p))); update the beforeAll to use the module-scope packageDirs so extractNewSdkExports(packageDirs) uses the same variable and no shadowing occurs.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+282
to
+296
| export type Hardware = { | ||
| hardware: string; | ||
| manufacturer: string; | ||
| storage: 5939; | ||
| memory: 2981; | ||
| cpu: 4; | ||
| display: { | ||
| width: 1440; | ||
| height: 2678; | ||
| orientation: 1; | ||
| }; | ||
| camera: { | ||
| numberOfCameras: 2; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the Hardware type definition in the source code
rg -n -A 20 'type Hardware\s*=' --type=ts packages/device-client/src/Repository: ForgeRock/ping-javascript-sdk
Length of output: 1826
Fix Hardware type to use generic number types instead of literal values.
The Hardware type definition in packages/device-client/src/lib/types/updateDeviceProfile.types.ts contains hard-coded literal values (storage: 5939, memory: 2981, cpu: 4, width: 1440, height: 2678, orientation: 1, numberOfCameras: 2). These should be generic number types to allow the type to work with any device configuration, not just the sample data. Update numeric properties to use number type instead of literal values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/device-client/api-report/device-client.api.md` around lines 282 -
296, Update the Hardware type so numeric fields use the generic number type
rather than hard-coded literals: change properties on the Hardware type
(storage, memory, cpu, display.width, display.height, display.orientation, and
camera.numberOfCameras) from their current literal values to type number in the
type definition (see the Hardware type in updateDeviceProfile.types.ts) so the
type accepts any numeric device configuration.
Comment on lines
+234
to
+244
| export type DeviceMetadata = { | ||
| platform: string; | ||
| version: 34; | ||
| device: string; | ||
| deviceName: string; | ||
| model: string; | ||
| brand: string; | ||
| locale: string; | ||
| timeZone: string; | ||
| jailBreakScore: 0; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether literal-constrained public types originate from source declarations.
set -euo pipefail
echo "== Locate source type files =="
fd -i "updateDeviceProfile.types.ts|types.ts|index.ts" packages/device-client
echo
echo "== Inspect likely source declarations for literal-constrained fields =="
rg -n -C3 "alias: 'Test3'|version:\s*34|jailBreakScore:\s*0|networkCountryIso:\s*'us'|carrierName:\s*'T-Mobile'|connected:\s*true|storage:\s*5939|memory:\s*2981|cpu:\s*4|recoveryCodes:\s*\[\]" packages/device-client/src
echo
echo "== Confirm generated API report contains same literal constraints =="
rg -n -C2 "alias: 'Test3'|version:\s*34|jailBreakScore:\s*0|networkCountryIso:\s*'us'|carrierName:\s*'T-Mobile'|connected:\s*true|storage:\s*5939|memory:\s*2981|cpu:\s*4|recoveryCodes:\s*\[\]" packages/device-client/api-report/device-client.types.api.mdRepository: ForgeRock/ping-javascript-sdk
Length of output: 8286
🏁 Script executed:
cat -n packages/device-client/src/lib/types/updateDeviceProfile.types.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 1761
🏁 Script executed:
# Check what is exported as public API
echo "=== Main index exports ==="
cat -n packages/device-client/src/index.ts | head -50
echo -e "\n=== Types.ts exports ==="
cat -n packages/device-client/src/types.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 901
🏁 Script executed:
cat -n packages/device-client/src/lib/types/index.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 925
🏁 Script executed:
# Check if these types are used for requests (where literals would be problematic) or responses
echo "=== Check usage patterns in implementation ==="
rg -n "DeviceMetadata|DeviceProfile|Hardware|Network|Telephony" packages/device-client/src --type ts -B2 -A2 | head -100
echo -e "\n=== Check API documentation or JSDoc ==="
rg -n "@param|@returns|Represents" packages/device-client/src/lib/types/updateDeviceProfile.types.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 6178
🏁 Script executed:
# Check the actual structure of how DeviceProfile is used in updateDeviceProfile mutation
echo "=== Check profile-device.types.ts which might be what's actually sent ==="
cat -n packages/device-client/src/lib/types/profile-device.types.ts
echo -e "\n=== Check the mutation query structure ==="
rg -n "updateDeviceProfile|ProfileDevicesQuery" packages/device-client/src/lib/services/index.ts -A10 | head -40Repository: ForgeRock/ping-javascript-sdk
Length of output: 2970
🏁 Script executed:
# Verify these types from updateDeviceProfile.types.ts are actually exported as public API
echo "=== Confirm updateDeviceProfile.types.ts is exported ==="
rg -n "updateDeviceProfile.types" packages/device-client/src/lib/types/index.ts
echo -e "\n=== Check if updateDeviceProfile types are actually used elsewhere in codebase ==="
rg "DeviceMetadata|DeviceProfile" packages/device-client/src --type ts | grep -v "profile-device.types.ts" | grep -v "mock-data"Repository: ForgeRock/ping-javascript-sdk
Length of output: 1492
Remove or fix literal-constrained types that contradict the actual API contract.
The exported types DeviceMetadata, Hardware, Network, Telephony, and DeviceProfile from updateDeviceProfile.types.ts contain hardcoded literal values (version: 34, jailBreakScore: 0, alias: 'Test3', storage: 5939, memory: 2981, cpu: 4, connected: true, networkCountryIso: 'us', carrierName: 'T-Mobile', recoveryCodes: []) that do not match the actual types used in mutations. The genuine type ProfileDevice from profile-device.types.ts correctly uses flexible types (number, string, boolean, string[]). These literal-constrained exported types will prevent consumers from passing valid data and should either be removed or corrected to match ProfileDevice.
Also applies to: lines 247-255, 282-296, 309-311, 424-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/device-client/api-report/device-client.types.api.md` around lines
234 - 244, The exported types DeviceMetadata, Hardware, Network, Telephony, and
DeviceProfile in updateDeviceProfile.types.ts currently use literal-constrained
values (e.g., version: 34, jailBreakScore: 0, alias: 'Test3', storage: 5939,
memory: 2981, cpu: 4, connected: true, networkCountryIso: 'us', carrierName:
'T-Mobile', recoveryCodes: []) which break the real API contract; update these
type definitions to use flexible primitives (number, string, boolean, string[])
matching the ProfileDevice shape (or remove the incorrect types and re-export or
alias ProfileDevice) so consumers can pass valid values when calling the
updateDeviceProfile mutation and ensure
DeviceMetadata/Hardware/Network/Telephony/DeviceProfile align with
ProfileDevice.
Comment on lines
+30
to
+109
| // @public | ||
| export class AttributeInputCallback<T extends string | number | boolean> extends BaseCallback { | ||
| constructor(payload: Callback); | ||
| getFailedPolicies(): PolicyRequirement[]; | ||
| getName(): string; | ||
| getPolicies(): Record<string, unknown>; | ||
| getPrompt(): string; | ||
| isRequired(): boolean; | ||
| // (undocumented) | ||
| payload: Callback; | ||
| setValidateOnly(value: boolean): void; | ||
| setValue(value: T): void; | ||
| } | ||
|
|
||
| export { AuthResponse } | ||
|
|
||
| // @public | ||
| export class BaseCallback { | ||
| constructor(payload: Callback); | ||
| getInputValue(selector?: number | string): unknown; | ||
| getOutputByName<T>(name: string, defaultValue: T): T; | ||
| getOutputValue(selector?: number | string): unknown; | ||
| getType(): CallbackType; | ||
| // (undocumented) | ||
| payload: Callback; | ||
| setInputValue(value: unknown, selector?: number | string | RegExp): void; | ||
| } | ||
|
|
||
| export { Callback } | ||
|
|
||
| // @public (undocumented) | ||
| export type CallbackFactory = (callback: Callback) => BaseCallback; | ||
|
|
||
| export { CallbackType } | ||
|
|
||
| export { callbackType } | ||
|
|
||
| // @public | ||
| export class ChoiceCallback extends BaseCallback { | ||
| constructor(payload: Callback); | ||
| getChoices(): string[]; | ||
| getDefaultChoice(): number; | ||
| getPrompt(): string; | ||
| // (undocumented) | ||
| payload: Callback; | ||
| setChoiceIndex(index: number): void; | ||
| setChoiceValue(value: string): void; | ||
| } | ||
|
|
||
| // @public | ||
| export class ConfirmationCallback extends BaseCallback { | ||
| constructor(payload: Callback); | ||
| getDefaultOption(): number; | ||
| getMessageType(): number; | ||
| getOptions(): string[]; | ||
| getOptionType(): number; | ||
| getPrompt(): string; | ||
| // (undocumented) | ||
| payload: Callback; | ||
| setOptionIndex(index: number): void; | ||
| setOptionValue(value: string): void; | ||
| } | ||
|
|
||
| // @public (undocumented) | ||
| export function createCallback(callback: Callback): BaseCallback; | ||
|
|
||
| export { createWellknownError } | ||
|
|
||
| export { CustomLogger } | ||
|
|
||
| // @public | ||
| export class DeviceProfileCallback extends BaseCallback { | ||
| constructor(payload: Callback); | ||
| getMessage(): string; | ||
| isLocationRequired(): boolean; | ||
| isMetadataRequired(): boolean; | ||
| // (undocumented) | ||
| payload: Callback; | ||
| setProfile(profile: DeviceProfileData): void; | ||
| } |
There was a problem hiding this comment.
The root entrypoint is re-exporting internal journey callback implementations.
This snapshot now exposes concrete callback classes, createCallback, and InternalJourneyClientConfig from @forgerock/journey-client itself. That expands the package contract far beyond the base callback abstraction and makes these internals semver-bound.
Based on learnings: In packages/journey-client, callback classes are internal implementation details not part of the public API, and only the base callback barrel is intended to be public.
Also applies to: 149-172, 250-495
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/journey-client/api-report/journey-client.api.md` around lines 30 -
109, The package is accidentally re-exporting internal concrete callback
implementations and helpers (e.g., AttributeInputCallback, ChoiceCallback,
ConfirmationCallback, DeviceProfileCallback, createCallback and any
InternalJourneyClientConfig) from the public barrel; remove those concrete class
and helper exports so only the abstract/public callback surface (BaseCallback,
CallbackFactory/Callback type aliases and any intentionally public types like
CallbackType) remain exported from the journey-client entrypoint; update the
barrel/index export list and regenerate the API snapshot so only BaseCallback
and the intended public types are present and the internal classes
(AttributeInputCallback, ChoiceCallback, ConfirmationCallback,
DeviceProfileCallback, createCallback, InternalJourneyClientConfig, etc.) are no
longer exported.
| */ | ||
|
|
||
| export * from './lib/client.store.js'; | ||
| export * from './types.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether journey-client root/type surfaces export concrete callback classes.
set -euo pipefail
echo "== Inspecting callback-related exports in journey-client entrypoints =="
rg -n --type=ts -C2 'export\s+.*\b[A-Za-z0-9_]*Callback\b|from\s+["\']\.\/lib\/callbacks\/' \
packages/journey-client/src/index.ts \
packages/journey-client/src/types.ts
echo
echo "Expected: only base JourneyCallback should be publicly exported."Repository: ForgeRock/ping-javascript-sdk
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking types.ts exports =="
cat packages/journey-client/src/types.ts
echo
echo "== Checking index.ts =="
cat packages/journey-client/src/index.ts
echo
echo "== Checking callbacks barrel (index.ts) =="
cat packages/journey-client/src/lib/callbacks/index.ts
echo
echo "== Finding concrete callback class files =="
find packages/journey-client/src/lib/callbacks -name "*.ts" -type f | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 3619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking name-callback.ts exports =="
head -30 packages/journey-client/src/lib/callbacks/name-callback.ts
echo
echo "== Checking base-callback.ts exports =="
head -30 packages/journey-client/src/lib/callbacks/base-callback.ts
echo
echo "== Checking factory.ts to see what it exports =="
head -50 packages/journey-client/src/lib/callbacks/factory.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 4160
Remove concrete callback class exports from types.ts public API.
The wildcard re-export at line 9 of index.ts exposes internal callback implementations (NameCallback, ChoiceCallback, etc.) as part of the public API. These are implementation details that consumers should not depend on; only the base BaseCallback and the factory function should be public. Move concrete callback class exports out of types.ts to prevent them from being re-exported in the root index.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/journey-client/src/index.ts` at line 9, The public re-export in
index.ts currently exposes concrete callback classes from types.ts; update the
public API so only BaseCallback and the factory are exported. Move or stop
exporting concrete implementations (e.g., NameCallback, ChoiceCallback,
TextCallback, etc.) out of types.ts (place them in an internal module like
callbacks/*.ts or mark them non-exported), ensure types.ts only exports
BaseCallback and the factory function (e.g., createCallbackFactory or similar),
and keep index.ts exporting only those public symbols so the concrete classes
are not re-exported from the package root.
Comment on lines
+74
to
+76
| export type { JourneyStep } from '../step.utils.js'; | ||
| export type { TextOutputCallback } from '../callbacks/text-output-callback.js'; | ||
| export type { BaseCallback } from '../callbacks/base-callback.js'; |
There was a problem hiding this comment.
Avoid exposing internal callback types in this public module.
Line 75 and Line 76 export concrete/internal callback types, which expands the public contract and conflicts with the intended callback encapsulation in journey-client.
Proposed fix
export type { JourneyStep } from '../step.utils.js';
-export type { TextOutputCallback } from '../callbacks/text-output-callback.js';
-export type { BaseCallback } from '../callbacks/base-callback.js';Based on learnings: In packages/journey-client, callback classes are internal implementation details not part of the public API; the callbacks barrel intentionally only exports the base JourneyCallback class.
📝 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
| export type { JourneyStep } from '../step.utils.js'; | |
| export type { TextOutputCallback } from '../callbacks/text-output-callback.js'; | |
| export type { BaseCallback } from '../callbacks/base-callback.js'; | |
| export type { JourneyStep } from '../step.utils.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/journey-client/src/lib/recovery-codes/recovery-codes.ts` around
lines 74 - 76, The file is re-exporting internal callback types
(TextOutputCallback and BaseCallback) which should not be part of the public
API; remove the two export type lines for TextOutputCallback and BaseCallback
from recovery-codes.ts so only public types remain (keep export type {
JourneyStep } and, if the public API requires a callback type, re-export only
the public JourneyCallback class/type from the callbacks barrel instead of
internal types). Ensure any callers use the public JourneyCallback symbol (not
TextOutputCallback/BaseCallback) and update imports if necessary.
| import { resolve } from 'node:path'; | ||
| import { extractNewSdkExports } from './new-sdk.js'; | ||
|
|
||
| const FIXTURE_DIR = resolve(__dirname, '../fixtures/mock-package'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Node.js ESM expose __dirname, and what is the recommended replacement in Vitest/TypeScript test files?
💡 Result:
No, Node.js ESM does not expose dirname. It was explicitly removed from ES modules as stated in the official Node.js documentation. The recommended replacements are: 1. Native (Node.js v20.11.0+ / v21.2.0+): import.meta.dirname (no longer experimental as of v22.16.0+ / v24.0.0+) 2. Polyfill for older versions: import { dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; const dirname = dirname(fileURLToPath(import.meta.url)); In Vitest/TypeScript test files, use the same replacements. Vitest (powered by Vite/vite-node) supports ESM and provides import.meta.dirname after fixes in recent versions (e.g., issue #5010 fixed). Vitest historically injected CJS globals like dirname for compatibility, but this is not standard ESM behavior—use import.meta.dirname for true ESM compatibility. Ensure Node.js >= v20 and recent Vitest/Vite versions. TypeScript supports this with "module": "NodeNext" or "ESNext" in tsconfig.json and @types/node.
Citations:
- 1: https://nodejs.org/api/esm.html
- 2: https://stackoverflow.com/questions/46745014/alternative-for-dirname-in-node-js-when-using-es6-modules
- 3: https://reactdevelopers.org/docs/nodejs-scripts/esm-vs-commonjs
- 4: Vitest incorrectly injects __dirname when type is module vitest-dev/vitest#2841
- 5:
import.meta.filenameandimport.meta.dirnameisundefinedon Node.js Latest LTSv20.11.0vitest-dev/vitest#5010
🏁 Script executed:
# Check Node.js version requirement
cat package.json | grep -A5 '"engines"' || echo "No engines field"Repository: ForgeRock/ping-javascript-sdk
Length of output: 185
🏁 Script executed:
# Check TypeScript configuration
cat tsconfig.json | head -40Repository: ForgeRock/ping-javascript-sdk
Length of output: 749
🏁 Script executed:
# Examine the test file to understand context better
head -20 tools/interface-mapping-validator/src/extractors/new-sdk.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 925
Replace __dirname here; this test file is authored as ESM.
With import syntax and .js specifiers, this module will execute as ESM, where __dirname is undefined. The suite will throw a ReferenceError before the first assertion.
Since the project requires Node.js v20+, use the native import.meta.dirname:
Suggested fix
import { describe, it, expect } from 'vitest';
-import { resolve } from 'node:path';
+import { resolve } from 'node:path';
import { extractNewSdkExports } from './new-sdk.js';
-const FIXTURE_DIR = resolve(__dirname, '../fixtures/mock-package');
+const FIXTURE_DIR = resolve(import.meta.dirname, '../fixtures/mock-package');Alternatively, for broader compatibility with older Node versions, use the polyfill:
-import { resolve } from 'node:path';
+import { dirname, resolve } from 'node:path';
+import { fileURLToPath } from 'node:url';
import { extractNewSdkExports } from './new-sdk.js';
-const FIXTURE_DIR = resolve(__dirname, '../fixtures/mock-package');
+const FIXTURE_DIR = resolve(dirname(fileURLToPath(import.meta.url)), '../fixtures/mock-package');📝 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
| const FIXTURE_DIR = resolve(__dirname, '../fixtures/mock-package'); | |
| const FIXTURE_DIR = resolve(import.meta.dirname, '../fixtures/mock-package'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/src/extractors/new-sdk.test.ts` at line 5,
The test uses __dirname to build FIXTURE_DIR (const FIXTURE_DIR =
resolve(__dirname, '../fixtures/mock-package')) which fails in ESM; replace
usage of __dirname with import.meta.dirname (or the provided polyfill) when
constructing FIXTURE_DIR so the module runs under ESM—update the reference where
FIXTURE_DIR is defined and any resolve(...) calls that depend on __dirname (look
for FIXTURE_DIR and resolve usage in new-sdk.test.ts) to use import.meta.dirname
instead.
Comment on lines
+25
to
+33
| function resolveSourcePath(packageDir: string, distPath: string): string | undefined { | ||
| const srcPath = distPath | ||
| .replace(/^\.\//, '') | ||
| .replace(/^dist\//, '') | ||
| .replace(/\.d\.ts$/, '.ts') | ||
| .replace(/\.js$/, '.ts'); | ||
|
|
||
| return resolve(packageDir, srcPath); | ||
| } |
There was a problem hiding this comment.
Don't validate against a guessed src path and then fail open.
package.json#exports already tells us which declaration target is public. Rewriting dist/... back to src/... means a harmless layout change can make an entire entrypoint disappear from validation, and the return [] here hides that drift instead of surfacing it.
Also applies to: 94-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/src/extractors/new-sdk.ts` around lines 25
- 33, The current resolveSourcePath function should not guess a “src” location
from a dist path and let callers silently ignore missing files; instead, change
it to use the package.exports mapping (or the package.json declaration target)
to locate the public .d.ts path and resolve that exact file, and if no
declaration target exists or the mapped file is missing, surface a hard failure
(throw or return an explicit error) rather than returning undefined/allowing
callers to return an empty result; apply the same fix pattern to the other
similar logic referenced around the second extractor block (the code at the
other occurrence) so both places consult package.json exports for the
declaration file and explicitly fail when the expected declaration is absent.
Comment on lines
+1
to
+5
| import { default as FRAuth } from './fake/fr-auth'; | ||
| import { CallbackType } from './fake/enums'; | ||
| import { default as NameCallback } from './fake/name-callback'; | ||
| import { Step, FailureDetail } from './fake/interfaces'; | ||
| import { default as Config, ConfigOptions } from './fake/config'; |
There was a problem hiding this comment.
Lint blocker: fixture imports are missing required file extensions.
This currently violates import/extensions and can fail CI lint checks.
Suggested change
-import { default as FRAuth } from './fake/fr-auth';
-import { CallbackType } from './fake/enums';
-import { default as NameCallback } from './fake/name-callback';
-import { Step, FailureDetail } from './fake/interfaces';
-import { default as Config, ConfigOptions } from './fake/config';
+import { default as FRAuth } from './fake/fr-auth.js';
+import { CallbackType } from './fake/enums.js';
+import { default as NameCallback } from './fake/name-callback.js';
+import { Step, FailureDetail } from './fake/interfaces.js';
+import { default as Config, ConfigOptions } from './fake/config.js';📝 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
| import { default as FRAuth } from './fake/fr-auth'; | |
| import { CallbackType } from './fake/enums'; | |
| import { default as NameCallback } from './fake/name-callback'; | |
| import { Step, FailureDetail } from './fake/interfaces'; | |
| import { default as Config, ConfigOptions } from './fake/config'; | |
| import { default as FRAuth } from './fake/fr-auth.js'; | |
| import { CallbackType } from './fake/enums.js'; | |
| import { default as NameCallback } from './fake/name-callback.js'; | |
| import { Step, FailureDetail } from './fake/interfaces.js'; | |
| import { default as Config, ConfigOptions } from './fake/config.js'; |
🧰 Tools
🪛 ESLint
[error] 1-1: Missing file extension for "./fake/fr-auth"
(import/extensions)
[error] 2-2: Missing file extension for "./fake/enums"
(import/extensions)
[error] 3-3: Missing file extension for "./fake/name-callback"
(import/extensions)
[error] 4-4: Missing file extension for "./fake/interfaces"
(import/extensions)
[error] 5-5: Missing file extension for "./fake/config"
(import/extensions)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/src/fixtures/legacy-sample.d.ts` around
lines 1 - 5, The import statements for FRAuth, CallbackType, NameCallback, Step,
FailureDetail, Config, and ConfigOptions use extensionless paths (e.g.,
'./fake/fr-auth'); update each import to include the proper file extension
(e.g., './fake/fr-auth.ts' or '.d.ts' as appropriate) so they comply with the
import/extensions lint rule—locate the imports for FRAuth, CallbackType,
NameCallback, Step/FailureDetail, and Config/ConfigOptions in legacy-sample.d.ts
and append the correct extension to each module path.
Comment on lines
+4
to
+10
| const ALLOWED_PACKAGES = [ | ||
| '@forgerock/journey-client', | ||
| '@forgerock/oidc-client', | ||
| '@forgerock/device-client', | ||
| '@forgerock/davinci-client', | ||
| '@forgerock/protect', | ||
| ]; |
There was a problem hiding this comment.
ALLOWED_PACKAGES currently permits a non-*-client package.
Line 9 includes @forgerock/protect, which contradicts the stated client-import-only rule and can mask validator drift in tests.
Proposed fix
const ALLOWED_PACKAGES = [
'@forgerock/journey-client',
'@forgerock/oidc-client',
'@forgerock/device-client',
'@forgerock/davinci-client',
- '@forgerock/protect',
];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/src/validate-client-imports.spec.ts` around
lines 4 - 10, The test's ALLOWED_PACKAGES array includes a non-client package
'@forgerock/protect' which violates the client-only rule; update the
ALLOWED_PACKAGES constant in validate-client-imports.spec.ts by removing
'@forgerock/protect' (or replacing it with the correct '@forgerock/*-client'
package if intended) so the list only contains packages that end with '-client',
ensuring the ALLOWED_PACKAGES symbol reflects the validator's client-import-only
constraint.
cerebrl
reviewed
Apr 15, 2026
Collaborator
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This is just a review of MIGRATION.md. I will review the mapping file tomorrow.
Comment on lines
+7
to
+11
| | Legacy | New | Purpose | | ||
| | --------------------------- | --------------------------- | ------------------------------------------------------------------ | | ||
| | `@forgerock/javascript-sdk` | `@forgerock/journey-client` | Authentication tree/journey flows | | ||
| | `@forgerock/javascript-sdk` | `@forgerock/oidc-client` | OAuth2/OIDC token management, user info, logout | | ||
| | `@forgerock/javascript-sdk` | `@forgerock/device-client` | Device profile & management (OATH, Push, WebAuthn, Bound, Profile) | |
Collaborator
There was a problem hiding this comment.
Since the "Legacy" column is just the whole SDK package, can we identify the class from that package? In other words:
`@forgerock/javascript-sdk` - `FRAuth` class | `@forgerock/journey-client`
`@forgerock/javascript-sdk` - `TokenManager` class | `@forgerock/oidc-client`
| | `FRAuth.next(step, { tree: 'Login' })` | `journeyClient.next(step)` | Tree is set at `start()`, not repeated on `next()` | | ||
| | `FRAuth.redirect(step)` | `await journeyClient.redirect(step)` | Now async. Step stored in `sessionStorage` (was `localStorage`) | | ||
| | `FRAuth.resume(resumeUrl)` | `await journeyClient.resume(resumeUrl)` | Previous step retrieved from `sessionStorage` (was `localStorage`) | | ||
| | No equivalent | `await journeyClient.terminate()` | **New.** Ends the AM session via `/sessions` endpoint | |
Collaborator
There was a problem hiding this comment.
Technically, this isn't correct. In the legacy SDK, we have a class called SessionManager, which has a logout method: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/session-manager/index.ts. This would be an equivalent to terminate.
| | Legacy | New | Notes | | ||
| | ------------------------------------------------- | --------------------------------------------------------------------------------------------- | ------------------------------------------------------ | | ||
| | `TokenManager.getTokens({ forceRenew: true })` | `await oidcClient.token.get({ forceRenew: true, backgroundRenew: true })` | Single call with auto-renewal. Returns tokens or error | | ||
| | `TokenManager.getTokens()` then manual code+state | `await oidcClient.authorize.background()` then `await oidcClient.token.exchange(code, state)` | Two-step when you need explicit control | |
Collaborator
There was a problem hiding this comment.
I'm not sure the legacy part of this is accurate. getTokens does everything. If you just want to call authorize and get the Authorization Code, I'm pretty sure you'd have to use getAuthCodeByIframe. Then call getTokens passing code and state as params.
|
|
||
| ## Error Handling Pattern | ||
|
|
||
| The new SDK primarily returns error objects instead of throwing exceptions. However, some `journey-client` methods may still throw in certain edge cases (known tech debt). Defensive code should handle both patterns: |
Collaborator
There was a problem hiding this comment.
Let's mention that Client initialization functions throw errors when misconfigured, but Client methods do not?
ryanbas21
force-pushed
the
migration-guide
branch
2 times, most recently
from
0724246 to
7b21957
Compare
cerebrl
approved these changes
Apr 15, 2026
ryanbas21
force-pushed
the
migration-guide
branch
from
7b21957 to
0ed8afd
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
packages/journey-client/src/lib/webauthn/webauthn.ts (1)
510-512:⚠️ Potential issue | 🟠 MajorKeep concrete callback classes out of the WebAuthn public exports.
These type re-exports expose internal callback implementations as public API, which increases long-term contract lock-in. Please avoid exporting concrete callback classes from this entrypoint.
Suggested change
export type { AttestationType, UserVerificationType } from './interfaces.js'; -export type { HiddenValueCallback } from '../callbacks/hidden-value-callback.js'; -export type { MetadataCallback } from '../callbacks/metadata-callback.js'; -export type { TextOutputCallback } from '../callbacks/text-output-callback.js'; export type { JourneyStep } from '../step.utils.js'; -export type { BaseCallback } from '../callbacks/base-callback.js';Based on learnings: In
packages/journey-client, callback classes are internal implementation details not part of the public API surface.Also applies to: 514-514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/webauthn/webauthn.ts` around lines 510 - 512, The three type re-exports HiddenValueCallback, MetadataCallback, and TextOutputCallback in webauthn.ts expose concrete/internal callback classes as part of the public API; remove these re-exports and instead export only the public callback interfaces or types (not the concrete implementations) from the module, or move the concrete classes into an internal-only barrel and update consumers to import internal implementations from that internal entrypoint; update any references to HiddenValueCallback/MetadataCallback/TextOutputCallback to use the public interface names or the new internal import location so the public entrypoint no longer exposes implementation details.tools/interface-mapping-validator/src/fixtures/legacy-sample.d.ts (1)
1-5:⚠️ Potential issue | 🔴 CriticalLint blocker: missing file extensions remain unfixed.
The import statements still lack the required
.jsfile extensions, which violates theimport/extensionsESLint rule and can fail CI lint checks. This issue was previously flagged but has not been resolved.🔧 Proposed fix
-import { default as FRAuth } from './fake/fr-auth'; -import { CallbackType } from './fake/enums'; -import { default as NameCallback } from './fake/name-callback'; -import { Step, FailureDetail } from './fake/interfaces'; -import { default as Config, ConfigOptions } from './fake/config'; +import { default as FRAuth } from './fake/fr-auth.js'; +import { CallbackType } from './fake/enums.js'; +import { default as NameCallback } from './fake/name-callback.js'; +import { Step, FailureDetail } from './fake/interfaces.js'; +import { default as Config, ConfigOptions } from './fake/config.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/fixtures/legacy-sample.d.ts` around lines 1 - 5, The imports for FRAuth, CallbackType, NameCallback, Step, FailureDetail, Config and ConfigOptions are missing the required .js file extensions; update each import's module specifier to include the .js extension (append ".js" to the existing './fake/...' paths) so the import/extensions ESLint rule is satisfied and CI lint passes.
🧹 Nitpick comments (8)
tools/interface-mapping-validator/src/fixer.test.ts (1)
31-40: Prefer derivinglineNumberfrom fixture content instead of hard-coding.Line 39, Line 78, Line 144, Line 163, and Line 179 hard-code absolute line indices from
SAMPLE_DOC. Small fixture edits will make these tests brittle and harder to maintain.Refactor sketch
+const lineNumberOf = (doc: string, contains: string): number => { + const idx = doc.split('\n').findIndex((l) => l.includes(contains)); + if (idx === -1) throw new Error(`Could not find row containing: ${contains}`); + return idx + 1; +}; ... - lineNumber: 8, + lineNumber: lineNumberOf(SAMPLE_DOC, '`OldThing`'), ... - lineNumber: 20, + lineNumber: lineNumberOf(SAMPLE_DOC, '`OldCallback`'),Also applies to: 70-79, 136-145, 155-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/fixer.test.ts` around lines 31 - 40, Tests in fixer.test.ts hard-code absolute lineNumber values (e.g., in the "removes a stale row from Quick Reference by lineNumber" test) which makes them brittle; update each test that sets Finding.lineNumber (those referencing SAMPLE_DOC) to derive the lineNumber dynamically by locating the expected line in SAMPLE_DOC (for example search SAMPLE_DOC.split('\n') for the row text or a unique marker like 'OldThing') and use that index + 1 as the Finding.lineNumber; adjust tests that reference SECTIONS.QUICK_REFERENCE and other Finding fixtures to compute lineNumber from SAMPLE_DOC content instead of using literal numbers so small fixture edits won't break tests.packages/davinci-client/src/types.ts (1)
36-41: Consider simplifying the davinci import/export pattern.The current approach has both a re-export (line 37) and an import (line 39) for the same symbol from the same module. While this works correctly, it could be streamlined.
♻️ Suggested simplification
-// Re-export the davinci function so DavinciClient type alias can be resolved -export { davinci } from './lib/client.store.js'; - -import { davinci } from './lib/client.store.js'; +import { davinci } from './lib/client.store.js'; + +// Re-export the davinci function so DavinciClient type alias can be resolved +export { davinci };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/types.ts` around lines 36 - 41, The file redundantly re-exports and imports the same symbol "davinci" which is unnecessary; remove the duplicate export or import so there is a single import that supplies the type alias "DavinciClient" (i.e., keep only one statement referencing davinci and ensure "export { davinci }" remains if you need to re-export the runtime, otherwise keep "import { davinci }" if only the type alias uses it), then ensure the type alias uses Awaited<ReturnType<typeof davinci>> unchanged.tools/interface-mapping-validator/src/generator.test.ts (1)
9-28: Consider extracting shared test helpers.The
makeLegacyandmakeNewSdkhelpers are duplicated betweendiffer.test.tsandgenerator.test.ts. Extracting them to a sharedtest-utils.tsfile would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/generator.test.ts` around lines 9 - 28, Extract the duplicated test helpers makeLegacy and makeNewSdk into a shared test-utils file: create test-utils.ts exporting functions makeLegacy and makeNewSdk (preserving signatures and return types LegacyExport and NewSdkExport), import any needed types (LegacyExport, NewSdkExport) from their original module, and update generator.test.ts and differ.test.ts to remove the local helpers and import { makeLegacy, makeNewSdk } from './test-utils'; ensure makeNewSdk still applies the same defaults (entryPoint => '.', packageName fallback from importPath.split('/').slice(0,2).join('/')) and that exported functions keep the 'variable' kind typing.tools/interface-mapping-validator/src/integration.test.ts (1)
100-107: Path resolution is duplicated from the first describe block.Lines 101-102 duplicate the path resolution logic from lines 33-35. Consider extracting the resolved paths to module-level constants or a shared setup.
♻️ Optional: Deduplicate path resolution
const WORKSPACE_ROOT = resolve(__dirname, '..', '..', '..'); const TOOL_ROOT = resolve(__dirname, '..'); +const legacyPath = resolve(TOOL_ROOT, LEGACY_SDK_INDEX_PATH); +const mappingPath = resolve(WORKSPACE_ROOT, INTERFACE_MAPPING_PATH); +const packageDirs = NEW_SDK_PACKAGES.map((p) => resolve(WORKSPACE_ROOT, p)); -const legacyPath = resolve(TOOL_ROOT, LEGACY_SDK_INDEX_PATH); -const mappingPath = resolve(WORKSPACE_ROOT, INTERFACE_MAPPING_PATH); -const packageDirs = NEW_SDK_PACKAGES.map((p) => resolve(WORKSPACE_ROOT, p)); describe('integration: full pipeline against real workspace data', () => { // ... beforeAll(() => { legacy = extractLegacyExports(legacyPath); newSdk = extractNewSdkExports(packageDirs); documented = extractDocumentedMappings(mappingPath); }, 60_000); describe('integration: generation', () => { // ... beforeAll(() => { - const legacyP = resolve(TOOL_ROOT, LEGACY_SDK_INDEX_PATH); - const packageDirs = NEW_SDK_PACKAGES.map((p) => resolve(WORKSPACE_ROOT, p)); - - legacy = extractLegacyExports(legacyP); + legacy = extractLegacyExports(legacyPath); newSdk = extractNewSdkExports(packageDirs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/integration.test.ts` around lines 100 - 107, The path resolution in the beforeAll setup duplicates logic from the earlier describe block; extract the resolved paths into module-level constants (e.g., const LEGACY_INDEX_PATH = resolve(TOOL_ROOT, LEGACY_SDK_INDEX_PATH) and const NEW_SDK_PACKAGE_DIRS = NEW_SDK_PACKAGES.map(p => resolve(WORKSPACE_ROOT, p))) or a shared helper, then update the beforeAll to call extractLegacyExports(LEGACY_INDEX_PATH), extractNewSdkExports(NEW_SDK_PACKAGE_DIRS) and keep generateSections(legacy, newSdk, SYMBOL_MAP, PACKAGE_MAP) to avoid duplication.tools/interface-mapping-validator/src/extractors/markdown.ts (1)
153-153: Consider using a Set for deduplication lookup.The
includes()call on an array performs O(n) lookup on each iteration. While this is unlikely to be a performance bottleneck for typical markdown files, using a Set in the state would be more idiomatic for deduplication.♻️ Optional: Use Set for O(1) lookups
type ParseState = { readonly mappings: readonly DocumentedMapping[]; - readonly entryPoints: readonly string[]; + readonly entryPoints: ReadonlySet<string>; readonly currentSection: SectionInfo | null; readonly inCodeBlock: boolean; readonly headerRowSeen: boolean; }; const initialState: ParseState = { mappings: [], - entryPoints: [], + entryPoints: new Set<string>(), // ... }; // Then in the reduce: -const uniqueEntryPoints = newEntryPoints.filter((ep) => !state.entryPoints.includes(ep)); +const uniqueEntryPoints = newEntryPoints.filter((ep) => !state.entryPoints.has(ep)); return { // ... - entryPoints: [...state.entryPoints, ...uniqueEntryPoints], + entryPoints: new Set([...state.entryPoints, ...uniqueEntryPoints]), }; // And convert to array at the end: return { mappings: finalState.mappings as DocumentedMapping[], - entryPoints: finalState.entryPoints as string[], + entryPoints: [...finalState.entryPoints] as string[], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/extractors/markdown.ts` at line 153, The current uniqueEntryPoints computation uses array.includes which is O(n) per check; change to use a Set for O(1) lookups by converting state.entryPoints to a Set (or creating a local Set like const existing = new Set(state.entryPoints)) and then filter newEntryPoints with existing.has(ep) instead of state.entryPoints.includes(ep); update any places that mutate or read state.entryPoints accordingly (or keep state.entryPoints as an array but maintain a derived Set for lookup) to ensure deduplication is efficient in the uniqueEntryPoints logic.tools/interface-mapping-validator/src/generator.ts (1)
262-267: Callback mapping assumes identical symbol names in legacy and new SDK.The callback table generation uses
exp.symbolfor both the legacy and new import statements. This works for 1:1 name matches but won't handle cases where a callback was renamed (e.g., ifFRCallbackbecameBaseCallback). Renamed callbacks would need explicit config entries inSYMBOL_MAPto be handled correctly.If this is the intended behavior (callbacks are either auto-matched by name or explicitly configured), consider adding a brief comment to clarify.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/generator.ts` around lines 262 - 267, The callback table generation assumes callbacks keep the same symbol name by using exp.symbol for both legacy and new imports in cbRows; update the generation to consult SYMBOL_MAP (or similar mapping) so renamed callbacks use the mapped legacy symbol (e.g., look up exp.symbol in SYMBOL_MAP to get legacyName or fall back to exp.symbol), referencing newSdk, isCallbackFromJourneyClient, exp.symbol, LEGACY_PKG, exp.importPath and SYMBOL_MAP in your change; alternatively, if name-equality is intended, add a concise comment next to cbRows explaining that callbacks must either have identical names or be explicitly listed in SYMBOL_MAP.tools/interface-mapping-validator/src/differ.ts (1)
190-190: Consider extracting the import regex to a shared constant.
IMPORT_SYMBOL_REis defined here and a similar pattern exists inmarkdown.ts(line 5). Extracting toconfig.tswould reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/interface-mapping-validator/src/differ.ts` at line 190, The import regex IMPORT_SYMBOL_RE used in differ.ts is duplicated in markdown.ts; extract it into a shared exported constant (e.g., EXPORT_IMPORT_SYMBOL_RE or IMPORT_SYMBOL_RE) in a new or existing config module (config.ts) and replace the local declarations in both differ.ts (where IMPORT_SYMBOL_RE is currently defined) and markdown.ts to import that shared constant; ensure the exported name matches usages and update any imports/exports accordingly so both files reference the single source of truth.tools/api-report/src/fixer.spec.ts (1)
37-46: Prefer fixture-based input instead of real workspace paths in this unit test.This case depends on
packages/sdk-effects/logger/package.jsonexisting with a specific name. A temporary fixture package would make the test deterministic and resilient to repo restructures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/api-report/src/fixer.spec.ts` around lines 37 - 46, The test 'should resolve sdk-effects subpackage from real workspace' relies on the real repo layout; instead create a temporary fixture package and point the test at it: use a temp dir (fs.mkdtempSync or a Jest/tmp utility) to create a package folder, write a package.json with "name": "@forgerock/sdk-logger", create the nested path matching 'logger/src/lib/logger.types.ts' (empty file is fine), set workspaceRoot and sourcePath to the fixture paths, call resolveSourcePackage(sourcePath, workspaceRoot) and assert the expected name, and finally cleanup the temp fixture; update the test to reference resolveSourcePackage, workspaceRoot, and sourcePath accordingly so it no longer depends on the real workspace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/davinci-client/package.json`:
- Around line 49-53: The package's "inputs" array for the api-report cache is
missing project metadata and TypeScript config files, so update the "inputs"
entry in packages/davinci-client/package.json to include
"{projectRoot}/package.json" and "{projectRoot}/tsconfig.json" plus any
lib-specific config like "{projectRoot}/tsconfig.lib.json" (and keep existing
"{workspaceRoot}/api-extractor.base.json" and tool sources). This ensures the
api-report tool (referenced by api-report logic) invalidates cache when package
metadata or compiler settings change.
In `@tools/api-report/README.md`:
- Around line 112-113: The README currently hard-codes test totals for
integration.spec.ts (e.g., "4 tests" at the integration.spec.ts line and the
total on lines referenced), which will become stale; update the README.md by
removing explicit numeric counts and using non-numeric wording (e.g.,
"integration.spec.ts — end-to-end tests with fixtures" or "X tests" generated
dynamically), or replace the static totals with an auto-generated summary if
your docs build supports it; specifically edit the README entries that reference
integration.spec.ts and the total on the later block (lines around 132-133) to
eliminate hard-coded numbers.
- Around line 56-67: Several fenced code blocks are missing language identifiers
(markdownlint MD040) — add a language tag (e.g., ```text) to each unlabeled
block that contains the snippets shown (the block starting with "x . - 2
error(s)", the block containing "src/" and related tree output, and the block
with "api-extractor message (ae-forgotten-export)") so each triple-backtick
fence becomes ```text (or another appropriate language) and closes with ``` to
silence the lint warnings; update those three unlabeled fenced blocks
accordingly.
In `@tools/interface-mapping-validator/README.md`:
- Around line 106-122: The fenced code block in the README (the ASCII project
tree) is missing a language specifier; update the opening triple-backtick to
include a language such as "text" or "plaintext" (e.g., change ``` to ```text)
so the static analysis tool stops flagging it and the ASCII diagram is rendered
as plain text.
In `@tools/interface-mapping-validator/src/fixer.test.ts`:
- Around line 256-265: The test currently only asserts addedLine is after the
Quick Reference heading; add an upper-bound assertion to ensure the inserted row
is before the callbacks section by locating the callbacks section heading (e.g.
find callbacksLine via lines.findIndex((l) => /^##\s+.*Callbacks/.test(l))) and
assert expect(addedLine).toBeLessThan(callbacksLine); update the test near the
variables tocLine, headingLine, addedLine to compute callbacksLine and add the
new expectation so the row is placed between the Quick Reference heading and the
Callbacks section.
- Around line 113-134: The test "preserves non-table content byte-for-byte"
currently only asserts presence of fragments and can miss spacing/ordering
changes; update the test to verify exact byte-for-byte preservation by
extracting the preserved sections from SAMPLE_DOC and comparing them exactly to
the corresponding slices in the result returned by applyFixes (use the same
boundaries used by the fixer or derive them from SECTIONS and the original
headings), so use applyFixes(SAMPLE_DOC, findings) and assert equality of the
exact substring(s) (or the whole document if appropriate) rather than using
expect(...).toContain for headings and prose; reference symbols: applyFixes,
SAMPLE_DOC, Finding, SECTIONS, and the test case title to locate and replace the
loose contains assertions with strict equality checks.
In `@tools/interface-mapping-validator/src/fixtures/mock-package/src/index.ts`:
- Line 1: The empty fixture function journey currently violates
`@typescript-eslint/no-empty-function`; update the journey function body to
include a harmless no-op statement (e.g., void 0; or a simple return) so the
fixture behavior is preserved while satisfying the linter; locate the export
function journey() in index.ts and insert the no-op inside its body.
---
Duplicate comments:
In `@packages/journey-client/src/lib/webauthn/webauthn.ts`:
- Around line 510-512: The three type re-exports HiddenValueCallback,
MetadataCallback, and TextOutputCallback in webauthn.ts expose concrete/internal
callback classes as part of the public API; remove these re-exports and instead
export only the public callback interfaces or types (not the concrete
implementations) from the module, or move the concrete classes into an
internal-only barrel and update consumers to import internal implementations
from that internal entrypoint; update any references to
HiddenValueCallback/MetadataCallback/TextOutputCallback to use the public
interface names or the new internal import location so the public entrypoint no
longer exposes implementation details.
In `@tools/interface-mapping-validator/src/fixtures/legacy-sample.d.ts`:
- Around line 1-5: The imports for FRAuth, CallbackType, NameCallback, Step,
FailureDetail, Config and ConfigOptions are missing the required .js file
extensions; update each import's module specifier to include the .js extension
(append ".js" to the existing './fake/...' paths) so the import/extensions
ESLint rule is satisfied and CI lint passes.
---
Nitpick comments:
In `@packages/davinci-client/src/types.ts`:
- Around line 36-41: The file redundantly re-exports and imports the same symbol
"davinci" which is unnecessary; remove the duplicate export or import so there
is a single import that supplies the type alias "DavinciClient" (i.e., keep only
one statement referencing davinci and ensure "export { davinci }" remains if you
need to re-export the runtime, otherwise keep "import { davinci }" if only the
type alias uses it), then ensure the type alias uses Awaited<ReturnType<typeof
davinci>> unchanged.
In `@tools/api-report/src/fixer.spec.ts`:
- Around line 37-46: The test 'should resolve sdk-effects subpackage from real
workspace' relies on the real repo layout; instead create a temporary fixture
package and point the test at it: use a temp dir (fs.mkdtempSync or a Jest/tmp
utility) to create a package folder, write a package.json with "name":
"@forgerock/sdk-logger", create the nested path matching
'logger/src/lib/logger.types.ts' (empty file is fine), set workspaceRoot and
sourcePath to the fixture paths, call resolveSourcePackage(sourcePath,
workspaceRoot) and assert the expected name, and finally cleanup the temp
fixture; update the test to reference resolveSourcePackage, workspaceRoot, and
sourcePath accordingly so it no longer depends on the real workspace.
In `@tools/interface-mapping-validator/src/differ.ts`:
- Line 190: The import regex IMPORT_SYMBOL_RE used in differ.ts is duplicated in
markdown.ts; extract it into a shared exported constant (e.g.,
EXPORT_IMPORT_SYMBOL_RE or IMPORT_SYMBOL_RE) in a new or existing config module
(config.ts) and replace the local declarations in both differ.ts (where
IMPORT_SYMBOL_RE is currently defined) and markdown.ts to import that shared
constant; ensure the exported name matches usages and update any imports/exports
accordingly so both files reference the single source of truth.
In `@tools/interface-mapping-validator/src/extractors/markdown.ts`:
- Line 153: The current uniqueEntryPoints computation uses array.includes which
is O(n) per check; change to use a Set for O(1) lookups by converting
state.entryPoints to a Set (or creating a local Set like const existing = new
Set(state.entryPoints)) and then filter newEntryPoints with existing.has(ep)
instead of state.entryPoints.includes(ep); update any places that mutate or read
state.entryPoints accordingly (or keep state.entryPoints as an array but
maintain a derived Set for lookup) to ensure deduplication is efficient in the
uniqueEntryPoints logic.
In `@tools/interface-mapping-validator/src/fixer.test.ts`:
- Around line 31-40: Tests in fixer.test.ts hard-code absolute lineNumber values
(e.g., in the "removes a stale row from Quick Reference by lineNumber" test)
which makes them brittle; update each test that sets Finding.lineNumber (those
referencing SAMPLE_DOC) to derive the lineNumber dynamically by locating the
expected line in SAMPLE_DOC (for example search SAMPLE_DOC.split('\n') for the
row text or a unique marker like 'OldThing') and use that index + 1 as the
Finding.lineNumber; adjust tests that reference SECTIONS.QUICK_REFERENCE and
other Finding fixtures to compute lineNumber from SAMPLE_DOC content instead of
using literal numbers so small fixture edits won't break tests.
In `@tools/interface-mapping-validator/src/generator.test.ts`:
- Around line 9-28: Extract the duplicated test helpers makeLegacy and
makeNewSdk into a shared test-utils file: create test-utils.ts exporting
functions makeLegacy and makeNewSdk (preserving signatures and return types
LegacyExport and NewSdkExport), import any needed types (LegacyExport,
NewSdkExport) from their original module, and update generator.test.ts and
differ.test.ts to remove the local helpers and import { makeLegacy, makeNewSdk }
from './test-utils'; ensure makeNewSdk still applies the same defaults
(entryPoint => '.', packageName fallback from
importPath.split('/').slice(0,2).join('/')) and that exported functions keep the
'variable' kind typing.
In `@tools/interface-mapping-validator/src/generator.ts`:
- Around line 262-267: The callback table generation assumes callbacks keep the
same symbol name by using exp.symbol for both legacy and new imports in cbRows;
update the generation to consult SYMBOL_MAP (or similar mapping) so renamed
callbacks use the mapped legacy symbol (e.g., look up exp.symbol in SYMBOL_MAP
to get legacyName or fall back to exp.symbol), referencing newSdk,
isCallbackFromJourneyClient, exp.symbol, LEGACY_PKG, exp.importPath and
SYMBOL_MAP in your change; alternatively, if name-equality is intended, add a
concise comment next to cbRows explaining that callbacks must either have
identical names or be explicitly listed in SYMBOL_MAP.
In `@tools/interface-mapping-validator/src/integration.test.ts`:
- Around line 100-107: The path resolution in the beforeAll setup duplicates
logic from the earlier describe block; extract the resolved paths into
module-level constants (e.g., const LEGACY_INDEX_PATH = resolve(TOOL_ROOT,
LEGACY_SDK_INDEX_PATH) and const NEW_SDK_PACKAGE_DIRS = NEW_SDK_PACKAGES.map(p
=> resolve(WORKSPACE_ROOT, p))) or a shared helper, then update the beforeAll to
call extractLegacyExports(LEGACY_INDEX_PATH),
extractNewSdkExports(NEW_SDK_PACKAGE_DIRS) and keep generateSections(legacy,
newSdk, SYMBOL_MAP, PACKAGE_MAP) to avoid duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 392c12f3-154f-4f1e-b51f-c87e2e5bda53
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (103)
.changeset/config.json.changeset/deep-spies-kick.md.github/workflows/ci.yml.gitignore.husky/commit-msg.husky/install.mjs.husky/pre-commit.husky/pre-push.husky/prepare-commit-msgMIGRATION.mdapi-extractor.base.jsoninterface_mapping.mdlefthook.ymlnx.jsonpackage.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/index.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/types.tspackages/device-client/api-report/device-client.api.mdpackages/device-client/api-report/device-client.types.api.mdpackages/device-client/package.jsonpackages/device-client/src/index.tspackages/device-client/src/lib/types/updateDeviceProfile.types.tspackages/device-client/src/types.tspackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.device.api.mdpackages/journey-client/api-report/journey-client.policy.api.mdpackages/journey-client/api-report/journey-client.qr-code.api.mdpackages/journey-client/api-report/journey-client.recovery-codes.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/api-report/journey-client.webauthn.api.mdpackages/journey-client/package.jsonpackages/journey-client/src/__tests__/types-reexport.typecheck.tspackages/journey-client/src/index.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/device/device-profile.tspackages/journey-client/src/lib/policy/policy.tspackages/journey-client/src/lib/qr-code/qr-code.tspackages/journey-client/src/lib/recovery-codes/recovery-codes.tspackages/journey-client/src/lib/webauthn/webauthn.tspackages/journey-client/src/types.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/package.jsonpackages/oidc-client/src/__tests__/types-reexport.typecheck.tspackages/oidc-client/src/index.tspackages/oidc-client/src/types.tstools/api-report/README.mdtools/api-report/eslint.config.mjstools/api-report/package.jsontools/api-report/src/config.spec.tstools/api-report/src/config.tstools/api-report/src/fixer.spec.tstools/api-report/src/fixer.tstools/api-report/src/integration.spec.tstools/api-report/src/main.spec.tstools/api-report/src/main.tstools/api-report/src/resolve-entries.spec.tstools/api-report/src/resolve-entries.tstools/api-report/tsconfig.jsontools/api-report/tsconfig.lib.jsontools/api-report/tsconfig.spec.jsontools/api-report/vitest.config.tstools/interface-mapping-validator/README.mdtools/interface-mapping-validator/eslint.config.mjstools/interface-mapping-validator/package.jsontools/interface-mapping-validator/src/config.tstools/interface-mapping-validator/src/differ.test.tstools/interface-mapping-validator/src/differ.tstools/interface-mapping-validator/src/extractors/legacy.test.tstools/interface-mapping-validator/src/extractors/legacy.tstools/interface-mapping-validator/src/extractors/markdown.test.tstools/interface-mapping-validator/src/extractors/markdown.tstools/interface-mapping-validator/src/extractors/new-sdk.test.tstools/interface-mapping-validator/src/extractors/new-sdk.tstools/interface-mapping-validator/src/fixer.test.tstools/interface-mapping-validator/src/fixer.tstools/interface-mapping-validator/src/fixtures/legacy-sample.d.tstools/interface-mapping-validator/src/fixtures/mock-package/package.jsontools/interface-mapping-validator/src/fixtures/mock-package/src/index.tstools/interface-mapping-validator/src/fixtures/mock-package/src/types.tstools/interface-mapping-validator/src/fixtures/sample-mapping.mdtools/interface-mapping-validator/src/generator.test.tstools/interface-mapping-validator/src/generator.tstools/interface-mapping-validator/src/integration.test.tstools/interface-mapping-validator/src/main.tstools/interface-mapping-validator/src/mapping-config.tstools/interface-mapping-validator/src/reporter.test.tstools/interface-mapping-validator/src/reporter.tstools/interface-mapping-validator/src/types.tstools/interface-mapping-validator/src/validate-client-imports.spec.tstools/interface-mapping-validator/src/validate-client-imports.tstools/interface-mapping-validator/src/writer.test.tstools/interface-mapping-validator/src/writer.tstools/interface-mapping-validator/tsconfig.jsontools/interface-mapping-validator/tsconfig.lib.jsontools/interface-mapping-validator/tsconfig.spec.jsontools/interface-mapping-validator/vitest.config.tstsconfig.json
💤 Files with no reviewable changes (5)
- .husky/commit-msg
- .husky/install.mjs
- .husky/pre-commit
- .husky/pre-push
- .husky/prepare-commit-msg
✅ Files skipped from review due to trivial changes (55)
- .gitignore
- tsconfig.json
- api-extractor.base.json
- packages/device-client/package.json
- tools/api-report/tsconfig.json
- packages/journey-client/src/lib/client.store.ts
- tools/interface-mapping-validator/tsconfig.json
- .changeset/deep-spies-kick.md
- packages/journey-client/src/lib/policy/policy.ts
- tools/api-report/src/main.spec.ts
- tools/api-report/package.json
- packages/oidc-client/src/index.ts
- tools/api-report/src/config.spec.ts
- tools/interface-mapping-validator/src/validate-client-imports.spec.ts
- tools/interface-mapping-validator/package.json
- tools/interface-mapping-validator/vitest.config.ts
- tools/api-report/vitest.config.ts
- packages/journey-client/src/lib/recovery-codes/recovery-codes.ts
- packages/oidc-client/src/tests/types-reexport.typecheck.ts
- tools/interface-mapping-validator/src/fixtures/mock-package/src/types.ts
- tools/interface-mapping-validator/src/fixtures/sample-mapping.md
- tools/interface-mapping-validator/src/extractors/legacy.ts
- lefthook.yml
- tools/interface-mapping-validator/src/fixtures/mock-package/package.json
- tools/api-report/tsconfig.spec.json
- tools/interface-mapping-validator/src/reporter.test.ts
- tools/interface-mapping-validator/tsconfig.spec.json
- tools/interface-mapping-validator/src/mapping-config.ts
- tools/api-report/src/resolve-entries.ts
- tools/interface-mapping-validator/src/extractors/legacy.test.ts
- tools/api-report/eslint.config.mjs
- tools/interface-mapping-validator/src/extractors/markdown.test.ts
- tools/interface-mapping-validator/src/writer.ts
- tools/interface-mapping-validator/tsconfig.lib.json
- tools/interface-mapping-validator/src/extractors/new-sdk.test.ts
- MIGRATION.md
- tools/interface-mapping-validator/src/config.ts
- tools/interface-mapping-validator/src/extractors/new-sdk.ts
- packages/journey-client/api-report/journey-client.policy.api.md
- tools/api-report/tsconfig.lib.json
- tools/interface-mapping-validator/src/writer.test.ts
- tools/api-report/src/resolve-entries.spec.ts
- packages/journey-client/api-report/journey-client.qr-code.api.md
- packages/device-client/api-report/device-client.api.md
- packages/journey-client/api-report/journey-client.recovery-codes.api.md
- packages/davinci-client/api-report/davinci-client.types.api.md
- packages/journey-client/api-report/journey-client.device.api.md
- packages/oidc-client/api-report/oidc-client.api.md
- packages/journey-client/api-report/journey-client.types.api.md
- tools/interface-mapping-validator/src/types.ts
- packages/journey-client/api-report/journey-client.webauthn.api.md
- packages/oidc-client/api-report/oidc-client.types.api.md
- packages/journey-client/api-report/journey-client.api.md
- packages/davinci-client/api-report/davinci-client.api.md
- packages/device-client/api-report/device-client.types.api.md
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/oidc-client/package.json
- .changeset/config.json
- nx.json
- packages/journey-client/package.json
- packages/device-client/src/index.ts
- packages/device-client/src/types.ts
- packages/journey-client/src/index.ts
- .github/workflows/ci.yml
- packages/journey-client/src/tests/types-reexport.typecheck.ts
- tools/interface-mapping-validator/src/validate-client-imports.ts
- tools/interface-mapping-validator/src/reporter.ts
- packages/davinci-client/src/index.ts
- packages/journey-client/src/lib/device/device-profile.ts
- packages/oidc-client/src/types.ts
- tools/interface-mapping-validator/src/fixer.ts
- tools/interface-mapping-validator/eslint.config.mjs
- packages/device-client/src/lib/types/updateDeviceProfile.types.ts
- package.json
- packages/davinci-client/src/lib/collector.types.ts
- tools/interface-mapping-validator/src/main.ts
- packages/journey-client/src/lib/qr-code/qr-code.ts
- tools/api-report/src/main.ts
- tools/api-report/src/fixer.ts
- packages/journey-client/src/types.ts
Comment on lines
+49
to
+53
| "inputs": [ | ||
| "{projectRoot}/dist/**/*.d.ts", | ||
| "{workspaceRoot}/api-extractor.base.json", | ||
| "{workspaceRoot}/tools/api-report/src/**/*.ts" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, pathlib
p = pathlib.Path("packages/davinci-client/package.json")
j = json.loads(p.read_text())
inputs = j["nx"]["targets"]["api-report"]["inputs"]
print("Configured inputs:")
for i in inputs:
print(" -", i)
needed = [
"{projectRoot}/package.json",
"{projectRoot}/tsconfig.json",
"{projectRoot}/tsconfig.lib.json",
]
print("\nMissing recommended inputs:")
missing = [x for x in needed if x not in inputs]
for x in missing:
print(" -", x)
if not missing:
print(" - none")
PY
echo
echo "Evidence that api-report consumes these files:"
rg -n "package\\.json|tsconfig\\.lib\\.json|tsconfig\\.json|packageJsonFullPath|tsconfigFilePath" \
tools/api-report/src/main.ts tools/api-report/src/config.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 1027
Include package metadata and TypeScript config files in api-report cache inputs.
The api-report tool reads package.json and tsconfig.json/tsconfig.lib.json from the project root (as evidenced in tools/api-report/src/main.ts:20-32 and tools/api-report/src/config.ts:26,45), but these files are missing from the target's inputs configuration. This causes cache invalidation to fail when exports or compiler configuration change without corresponding changes to the type definitions.
Suggested fix
"inputs": [
+ "{projectRoot}/package.json",
+ "{projectRoot}/tsconfig.json",
+ "{projectRoot}/tsconfig.lib.json",
"{projectRoot}/dist/**/*.d.ts",
"{workspaceRoot}/api-extractor.base.json",
"{workspaceRoot}/tools/api-report/src/**/*.ts"
],📝 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
| "inputs": [ | |
| "{projectRoot}/dist/**/*.d.ts", | |
| "{workspaceRoot}/api-extractor.base.json", | |
| "{workspaceRoot}/tools/api-report/src/**/*.ts" | |
| ], | |
| "inputs": [ | |
| "{projectRoot}/package.json", | |
| "{projectRoot}/tsconfig.json", | |
| "{projectRoot}/tsconfig.lib.json", | |
| "{projectRoot}/dist/**/*.d.ts", | |
| "{workspaceRoot}/api-extractor.base.json", | |
| "{workspaceRoot}/tools/api-report/src/**/*.ts" | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/package.json` around lines 49 - 53, The package's
"inputs" array for the api-report cache is missing project metadata and
TypeScript config files, so update the "inputs" entry in
packages/davinci-client/package.json to include "{projectRoot}/package.json" and
"{projectRoot}/tsconfig.json" plus any lib-specific config like
"{projectRoot}/tsconfig.lib.json" (and keep existing
"{workspaceRoot}/api-extractor.base.json" and tool sources). This ensures the
api-report tool (referenced by api-report logic) invalidates cache when package
metadata or compiler settings change.
Comment on lines
+56
to
+67
| ``` | ||
| x . - 2 error(s) | ||
|
|
||
| Missing re-exports: | ||
| - ProfileData | ||
| - SomeEnum | ||
|
|
||
| 2 error(s) found. | ||
|
|
||
| Fix this package: tsx tools/api-report/src/main.ts packages/journey-client --fix | ||
| Fix all packages: pnpm api-report:fix | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (markdownlint MD040).
Line 56, Line 102, and Line 117 start unlabeled fenced blocks, which triggers current lint warnings.
🛠️ Suggested doc fix
-```
+```text
x . - 2 error(s)
...
-```
+```
-```
+```text
src/
...
-```
+```
-```
+```text
api-extractor message (ae-forgotten-export)
...
-```
+```Also applies to: 102-113, 117-127
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/api-report/README.md` around lines 56 - 67, Several fenced code blocks
are missing language identifiers (markdownlint MD040) — add a language tag
(e.g., ```text) to each unlabeled block that contains the snippets shown (the
block starting with "x . - 2 error(s)", the block containing "src/" and related
tree output, and the block with "api-extractor message (ae-forgotten-export)")
so each triple-backtick fence becomes ```text (or another appropriate language)
and closes with ``` to silence the lint warnings; update those three unlabeled
fenced blocks accordingly.
Comment on lines
+112
to
+113
| └── integration.spec.ts # 4 tests — end-to-end detect → fix with fixtures | ||
| ``` |
There was a problem hiding this comment.
Avoid hard-coded test totals; these counts are already stale.
Line 112 says integration.spec.ts has 4 tests, but the current file has 6. Line 132 also hard-codes a total that will drift. Prefer non-numeric wording (or keep counts auto-generated) to prevent doc rot.
Also applies to: 132-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/api-report/README.md` around lines 112 - 113, The README currently
hard-codes test totals for integration.spec.ts (e.g., "4 tests" at the
integration.spec.ts line and the total on lines referenced), which will become
stale; update the README.md by removing explicit numeric counts and using
non-numeric wording (e.g., "integration.spec.ts — end-to-end tests with
fixtures" or "X tests" generated dynamically), or replace the static totals with
an auto-generated summary if your docs build supports it; specifically edit the
README entries that reference integration.spec.ts and the total on the later
block (lines around 132-133) to eliminate hard-coded numbers.
Comment on lines
+106
to
+122
| ``` | ||
| src/ | ||
| ├── extractors/ | ||
| │ ├── legacy.ts # Parse legacy SDK dist/index.d.ts → symbol list | ||
| │ ├── new-sdk.ts # Parse new SDK package.json exports + barrel files → symbol list | ||
| │ └── markdown.ts # Parse interface_mapping.md tables → documented mappings | ||
| ├── mapping-config.ts # Symbol mapping data (the "rename database") | ||
| ├── generator.ts # Config + exports → markdown table strings | ||
| ├── writer.ts # Replace sections in markdown, preserve everything else | ||
| ├── differ.ts # Compare extracted vs. documented → findings | ||
| ├── fixer.ts # Apply findings as patches to markdown | ||
| ├── validate-client-imports.ts # Ensure doc only references *-client package imports | ||
| ├── reporter.ts # Format findings for stdout | ||
| ├── types.ts # Shared type definitions | ||
| ├── config.ts # Constants (package paths, section names, allowed packages) | ||
| └── main.ts # CLI entry point | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block.
The static analysis tool flagged this code block as missing a language specifier. For ASCII diagrams, text or plaintext is appropriate.
📝 Proposed fix
-```
+```text
src/
├── extractors/🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/README.md` around lines 106 - 122, The
fenced code block in the README (the ASCII project tree) is missing a language
specifier; update the opening triple-backtick to include a language such as
"text" or "plaintext" (e.g., change ``` to ```text) so the static analysis tool
stops flagging it and the ASCII diagram is rendered as plain text.
Comment on lines
+113
to
+134
| it('preserves non-table content byte-for-byte', () => { | ||
| const findings: Finding[] = [ | ||
| { | ||
| category: 'stale-legacy-symbol', | ||
| severity: 'warning', | ||
| section: SECTIONS.QUICK_REFERENCE, | ||
| message: 'OldThing is stale', | ||
| action: 'remove', | ||
| lineNumber: 8, | ||
| }, | ||
| ]; | ||
|
|
||
| const result = applyFixes(SAMPLE_DOC, findings); | ||
|
|
||
| // The heading, separator, and trailing prose must be preserved exactly | ||
| expect(result).toContain('# Interface Mapping'); | ||
| expect(result).toContain('## 0. Quick Reference'); | ||
| expect(result).toContain('---'); | ||
| expect(result).toContain('## 5. Callbacks'); | ||
| expect(result).toContain('### Callback Type Mapping'); | ||
| expect(result).toContain('Some trailing prose that must not be touched.'); | ||
| }); |
There was a problem hiding this comment.
“Byte-for-byte” claim is not actually validated.
Line 113 says byte-for-byte preservation, but assertions on Line 128–133 only check presence. This can pass even if unrelated prose/spacing changed.
Stronger assertion pattern
it('preserves non-table content byte-for-byte', () => {
+ const removedRow = "| `OldThing` | `import { old } from '@forgerock/old-package'` |";
const findings: Finding[] = [
{
...
- // The heading, separator, and trailing prose must be preserved exactly
- expect(result).toContain('# Interface Mapping');
- expect(result).toContain('## 0. Quick Reference');
- expect(result).toContain('---');
- expect(result).toContain('## 5. Callbacks');
- expect(result).toContain('### Callback Type Mapping');
- expect(result).toContain('Some trailing prose that must not be touched.');
+ expect(result.replace(`${removedRow}\n`, '')).toBe(
+ SAMPLE_DOC.replace(`${removedRow}\n`, ''),
+ );
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/src/fixer.test.ts` around lines 113 - 134,
The test "preserves non-table content byte-for-byte" currently only asserts
presence of fragments and can miss spacing/ordering changes; update the test to
verify exact byte-for-byte preservation by extracting the preserved sections
from SAMPLE_DOC and comparing them exactly to the corresponding slices in the
result returned by applyFixes (use the same boundaries used by the fixer or
derive them from SECTIONS and the original headings), so use
applyFixes(SAMPLE_DOC, findings) and assert equality of the exact substring(s)
(or the whole document if appropriate) rather than using expect(...).toContain
for headings and prose; reference symbols: applyFixes, SAMPLE_DOC, Finding,
SECTIONS, and the test case title to locate and replace the loose contains
assertions with strict equality checks.
Comment on lines
+256
to
+265
| // The new row should appear after the Quick Reference table, not after the ToC line | ||
| const lines = result.split('\n'); | ||
| const tocLine = lines.findIndex((l) => l.includes('[Quick Reference]')); | ||
| const addedLine = lines.findIndex((l) => l.includes('FRStep')); | ||
| const headingLine = lines.findIndex((l) => /^##\s+.*Quick Reference/.test(l)); | ||
|
|
||
| expect(tocLine).toBeGreaterThan(-1); | ||
| expect(headingLine).toBeGreaterThan(tocLine); | ||
| expect(addedLine).toBeGreaterThan(headingLine); | ||
| }); |
There was a problem hiding this comment.
ToC test needs an upper-bound assertion for section placement.
Line 264 only verifies the new row is after the Quick Reference heading. It can still pass if insertion happens in a later section. Add a check that the row is before the callbacks section heading.
Assertion hardening
const headingLine = lines.findIndex((l) => /^##\s+.*Quick Reference/.test(l));
+ const callbacksLine = lines.findIndex((l) => /^##\s+.*Callbacks/.test(l));
expect(tocLine).toBeGreaterThan(-1);
expect(headingLine).toBeGreaterThan(tocLine);
expect(addedLine).toBeGreaterThan(headingLine);
+ expect(callbacksLine).toBeGreaterThan(headingLine);
+ expect(addedLine).toBeLessThan(callbacksLine);📝 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
| // The new row should appear after the Quick Reference table, not after the ToC line | |
| const lines = result.split('\n'); | |
| const tocLine = lines.findIndex((l) => l.includes('[Quick Reference]')); | |
| const addedLine = lines.findIndex((l) => l.includes('FRStep')); | |
| const headingLine = lines.findIndex((l) => /^##\s+.*Quick Reference/.test(l)); | |
| expect(tocLine).toBeGreaterThan(-1); | |
| expect(headingLine).toBeGreaterThan(tocLine); | |
| expect(addedLine).toBeGreaterThan(headingLine); | |
| }); | |
| // The new row should appear after the Quick Reference table, not after the ToC line | |
| const lines = result.split('\n'); | |
| const tocLine = lines.findIndex((l) => l.includes('[Quick Reference]')); | |
| const addedLine = lines.findIndex((l) => l.includes('FRStep')); | |
| const headingLine = lines.findIndex((l) => /^##\s+.*Quick Reference/.test(l)); | |
| const callbacksLine = lines.findIndex((l) => /^##\s+.*Callbacks/.test(l)); | |
| expect(tocLine).toBeGreaterThan(-1); | |
| expect(headingLine).toBeGreaterThan(tocLine); | |
| expect(addedLine).toBeGreaterThan(headingLine); | |
| expect(callbacksLine).toBeGreaterThan(headingLine); | |
| expect(addedLine).toBeLessThan(callbacksLine); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/src/fixer.test.ts` around lines 256 - 265,
The test currently only asserts addedLine is after the Quick Reference heading;
add an upper-bound assertion to ensure the inserted row is before the callbacks
section by locating the callbacks section heading (e.g. find callbacksLine via
lines.findIndex((l) => /^##\s+.*Callbacks/.test(l))) and assert
expect(addedLine).toBeLessThan(callbacksLine); update the test near the
variables tocLine, headingLine, addedLine to compute callbacksLine and add the
new expectation so the row is placed between the Quick Reference heading and the
Callbacks section.
| @@ -0,0 +1,2 @@ | |||
| export function journey() {} | |||
There was a problem hiding this comment.
Avoid empty fixture function to satisfy lint rules.
journey is currently empty and triggers @typescript-eslint/no-empty-function. Keep fixture behavior but add a no-op statement.
Suggested fix
-export function journey() {}
+export function journey(): void {
+ return;
+}📝 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
| export function journey() {} | |
| export function journey(): void { | |
| return; | |
| } |
🧰 Tools
🪛 ESLint
[error] 1-1: Unexpected empty function 'journey'.
(@typescript-eslint/no-empty-function)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/interface-mapping-validator/src/fixtures/mock-package/src/index.ts` at
line 1, The empty fixture function journey currently violates
`@typescript-eslint/no-empty-function`; update the journey function body to
include a harmless no-op statement (e.g., void 0; or a simple return) so the
fixture behavior is preserved while satisfying the linter; locate the export
function journey() in index.ts and insert the no-op inside its body.
… guide updates Adds forgotten-export detection via api-extractor, interface mapping validation between legacy and new client packages, migration to lefthook, and initial API report generation for all client packages. Updates interface_mapping.md to match current SDK exports and syncs api-report files that drifted during the rebase.
ryanbas21
force-pushed
the
migration-guide
branch
from
0ed8afd to
15d5af3
Compare
2 tasks
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
5 participants
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
interface_mapping.md— comprehensive migration guide mapping every legacy@forgerock/javascript-sdksymbol to its new SDK equivalenttools/api-report— forgotten export detector that catches types in client package public APIs that aren't explicitly re-exported, including inferred return typestools/interface-mapping-validator— ensures the migration guide only references*-clientpackage imports*-clientindex.ts re-exports from its own types.tsImport Principle
Consumers should only import from
*-clientpackages. Internal packages (sdk-types,sdk-logger,sdk-request-middleware,storage) are implementation details. Two automated guards enforce this:exportsfield. Catches forgotten re-exports including inferred return types. Supports--fixto auto-add missing re-exports with correct type/value distinction.New tools
tools/api-report(51 tests)package.jsonexports, resolves.d.tspaths, runs api-extractor programmatically--fixmode follows import chains to resolve source packages and determine type vs valuepnpm api-reportto check,pnpm api-report:fixto fix + rebuild + verifytools/interface-mapping-validatorupdates (107 tests)@forgerock/sdk-typesetc. in the migration guideOidcClient) — no manual SYMBOL_MAP entry neededFRCallback→BaseCallback)Test plan
pnpm nx run-many -t typecheck -p journey-client oidc-client device-client davinci-client— all passpnpm api-report— all 13 entry points clean across 4 client packagespnpm nx run api-report:test— 51 tests (6 integration tests with fixtures)pnpm nx run interface-mapping-validator:test— 107 testspnpm tsx tools/interface-mapping-validator/src/main.ts— 0 errorsSummary by CodeRabbit