chore: upgrade toolchain and dependencies#458
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpgrades Nx to 22.7.5, TypeScript to 6.0.3, ESLint to 10, and several runtime dependencies (axios, listr2, commander, scalar packages). Removes ChangesTooling and Dependency Upgrades
TypeScript 6 Strictness Fixes in CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/cli/src/server/index.ts (1)
64-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix server handle ownership and startup/shutdown error propagation.
At Line 73, the main server listener is assigned to
serverStatus, while the readiness server started at Line 81 is never stored for shutdown. Also, startup/shutdown promises resolve on success paths only, so listen/close failures are not surfaced.As per coding guidelines, "Every function return value must be checked for errors (if applicable); errors must be properly handled, not ignored or silently swallowed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/server/index.ts` around lines 64 - 83, The server initialization code has two issues: first, the main server listener handle at line 73 is assigned to serverStatus but the readiness server listener at line 81 is never stored, preventing proper cleanup during shutdown; second, both Promise chains only resolve on success but never reject on listen errors, violating the error handling guideline that all function return values must be checked. Fix this by storing both server handles (the return value from this.server!.listen() and this.expressStatus.listen()) so they can be properly closed during shutdown, and add error event listeners to both server instances within their respective Promise chains to catch and reject on listen failures instead of silently swallowing errors.Source: Coding guidelines
apps/cli/src/command/sync.command.ts (1)
101-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure backend listener cleanup runs on both success and failure paths.
On Line 101-110,
cancel()is skipped whensync()throws, leaving event subscriptions active. Wrap execution intry/finallyand validatectx.backend/ctx.diffbefore use.Proposed fix
title: 'Sync configuration', task: async (ctx, task) => { - const cancel = addBackendEventListener(ctx.backend!, task); - await lastValueFrom( - ctx.backend! - .sync(ctx.diff!, { - exitOnFailure: true, - concurrent: opts.requestConcurrent, - }) - .pipe(toArray()), - ); - cancel(); + if (!ctx.backend) throw new Error('Backend is not initialized'); + if (!ctx.diff) throw new Error('Diff is not initialized'); + const cancel = addBackendEventListener(ctx.backend, task); + try { + await lastValueFrom( + ctx.backend + .sync(ctx.diff, { + exitOnFailure: true, + concurrent: opts.requestConcurrent, + }) + .pipe(toArray()), + ); + } finally { + cancel(); + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/command/sync.command.ts` around lines 101 - 110, The cancel() cleanup function from addBackendEventListener is not being called when the sync() operation throws an error, leaving event subscriptions active. Wrap the sync() operation in a try/finally block, moving the cancel() call into the finally block to ensure it executes regardless of success or failure. Additionally, validate that ctx.backend and ctx.diff are not null before entering the try block to prevent errors during the cleanup phase.Source: Pipeline failures
apps/cli/src/command/ingress-server.command.ts (1)
80-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop non-null assertions by normalizing option defaults in-handler.
On Line 80, Line 89, Line 90, and Line 97,
listen!/listenStatus!produce lint violations and rely on assertion semantics. Use local defaulted values once, then reference those.Proposed fix
.handle( async ({ listen, listenStatus, tlsCertFile, tlsKeyFile, caCertFile }) => { - if (listen!.protocol === 'https:' && (!tlsCertFile || !tlsKeyFile)) { + const listenUrl = listen ?? new URL('http://127.0.0.1:3000'); + const statusPort = listenStatus ?? 3001; + if (listenUrl.protocol === 'https:' && (!tlsCertFile || !tlsKeyFile)) { console.error( chalk.red( 'Error: When using HTTPS, both --tls-cert-file and --tls-key-file must be provided', ), ); return; } const server = new ADCServer({ - listen: listen!, - listenStatus: listenStatus!, + listen: listenUrl, + listenStatus: statusPort, tlsCert: tlsCertFile ? readFileSync(tlsCertFile, 'utf-8') : undefined, tlsKey: tlsKeyFile ? readFileSync(tlsKeyFile, 'utf-8') : undefined, tlsCACert: caCertFile ? readFileSync(caCertFile, 'utf-8') : undefined, }); await server.start(); logger.info( - `ADC server is running on: ${listen!.protocol === 'unix:' ? listen!.pathname : listen!.origin}`, + `ADC server is running on: ${listenUrl.protocol === 'unix:' ? listenUrl.pathname : listenUrl.origin}`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/command/ingress-server.command.ts` around lines 80 - 97, Remove the non-null assertions on `listen`, `listenStatus`, and related variables in the ingress-server.command.ts file. Create local variables at the beginning of the code block that capture these values with proper defaults, then use these local variables throughout the rest of the block (in the HTTPS validation check, the ADCServer instantiation, and the logger.info call) instead of using the assertion operator. This eliminates the need for the `!` syntax on `listen!`, `listenStatus!`, and similar variables while maintaining the same logic.Source: Pipeline failures
apps/cli/src/command/convert.command.ts (1)
108-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently swallow conversion failures.
On Lines 108-112, the error is caught and ignored, so the command can fail internally without surfacing a failure status/message.
Proposed fix
- try { - await tasks.run(); - } catch (err) { - /* ignore */ - } + try { + await tasks.run(); + } catch (err) { + console.error(String(err)); + process.exit(1); + }As per coding guidelines, "errors must be properly handled, not ignored or silently swallowed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/command/convert.command.ts` around lines 108 - 112, The catch block in the convert.command.ts file (lines 108-112) is silently ignoring errors from the await tasks.run() call, preventing proper error handling and status reporting. Replace the empty catch block that contains only /* ignore */ with proper error handling that logs the error and/or surfaces the failure to the user, such as logging the error details and either rethrowing it or returning an appropriate error status/message to indicate the conversion operation failed.Sources: Coding guidelines, Pipeline failures
apps/cli/src/command/ingress-sync.command.ts (2)
85-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact sensitive response fields before writing failed sync details to stdout.
Lines 92–97 serialize raw
axiosResponse.headersandaxiosResponse.data. Those payloads can carry auth headers, tokens, cookies, or connection details and should not be emitted unredacted.As per coding guidelines, “Scan for code that logs, serializes, or returns API keys, tokens (Bearer, JWT, access_token, refresh_token), OAuth client secrets, plugin configuration payloads, authentication headers, and database connection strings without redaction.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/command/ingress-sync.command.ts` around lines 85 - 99, The code in the faileds mapping function is including raw axiosResponse.headers and axiosResponse.data in the response object without redacting sensitive information like authentication tokens, cookies, API keys, or database connection strings. Create a utility function to redact sensitive fields (such as authorization headers, bearer tokens, cookies, and other sensitive data fields) and apply it to both axiosResponse.headers and axiosResponse.data before they are included in the response object that gets serialized to stdout. The redaction should mask or filter out values from known sensitive keys while preserving the structure for debugging purposes.Source: Coding guidelines
103-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow sync execution errors inside the task.
Lines 103–105 catch and print errors but do not rethrow. That can let the Listr pipeline continue and return a success exit path after a failed sync operation.
Suggested fix
- } catch (err) { - process.stderr.write(toString(err)); - } + } catch (err) { + process.stderr.write(toString(err)); + throw err; + }As per coding guidelines, “Every function return value must be checked for errors (if applicable); errors must be properly handled, not ignored or silently swallowed.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/command/ingress-sync.command.ts` around lines 103 - 105, The catch block in the ingress-sync command is catching errors and writing them to stderr but not rethrowing them, which allows the Listr pipeline to incorrectly return a success status even after a failed sync. After the process.stderr.write(toString(err)) call that logs the error, rethrow the error by adding a throw statement to propagate the error up the pipeline and cause it to properly fail.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/command/convert.command.ts`:
- Around line 81-84: The error casting on line 81 using `error as Error` assumes
the caught value is always an Error object, but JavaScript allows throwing any
value, which can cause a secondary failure when accessing `.message` and
`.stack` properties. Normalize the error first by checking if the caught value
is actually an Error instance; if not, wrap it in a new Error object. Only after
this normalization should you perform the sanitization operations like replacing
newlines in the message and clearing the stack.
In `@apps/cli/src/command/dump.command.ts`:
- Around line 65-72: Replace the non-null assertions on ctx.remote at lines 65
and 72 in the task function with explicit validation. At the beginning of the
task function in the 'Write to dump file' block, add an explicit check to
validate that ctx.remote exists, and if it doesn't, fail fast by throwing an
error or returning early. Then use the validated ctx.remote without non-null
assertions in both the recursiveRemoveMetadataField call and the
dump/resortConfiguration calls.
In `@apps/cli/src/command/ping.command.ts`:
- Line 30: The code uses a non-null assertion operator on ctx.backend when
calling the ping() method, which violates lint rules and hides potential
initialization failures. Instead of using ctx.backend!, add an explicit guard
clause to check if ctx.backend exists before calling ping(). If it doesn't
exist, handle the failure appropriately by either throwing an error or returning
a failed result. This approach makes the missing initialization failure
actionable and removes the non-null assertion.
In `@apps/cli/src/server/logger.ts`:
- Around line 35-41: Create a centralized sanitizer function (e.g., in a utility
module) that redacts sensitive fields like API keys, tokens, Bearer/JWT
credentials, access_token, refresh_token, OAuth client secrets, authentication
headers, and database connection strings. In apps/cli/src/server/logger.ts lines
35-41, apply this sanitizer to req.body before assigning it to requestBody in
the middleware log. In apps/cli/src/server/sync.ts lines 80-95, sanitize the
Axios request/response headers and payload fields before calling logger.log. In
apps/cli/src/server/validate.ts lines 79-95, apply the same sanitizer function
to Axios debug logs. In apps/cli/src/command/utils.ts lines 331-343, use the
sanitizer to redact headers case-insensitively and redact sensitive payload keys
before constructing the debug output. This ensures sensitive data is
consistently redacted across all logging emit points.
In `@apps/cli/src/tasks/experimental.ts`:
- Line 13: The YAML.load statement on line 13 uses a force type cast (as
ADCSDK.Configuration) without validating that the parsed YAML actually matches
the expected Configuration shape. If the YAML file contains a scalar, array, or
object with an invalid structure, the runtime will fail later with unclear
errors. Parse the YAML content first, then validate that the resulting object
conforms to the ADCSDK.Configuration type structure (checking for required
properties and correct types) before assigning it to ctx.remote. Only perform
the assignment after successful validation, and throw a meaningful error if
validation fails.
In `@apps/cli/src/tasks/load_remote.ts`:
- Around line 30-35: The backend event listener added by addBackendEventListener
is not being released if an error occurs during ctx.backend!.dump(). Wrap the
await lastValueFrom(ctx.backend!.dump()) call and the subsequent structuredClone
operation in a try-finally block, ensuring that cancel() is called in the
finally block to guarantee the listener is released whether the dump succeeds or
fails.
In `@libs/converter-openapi/package.json`:
- Around line 25-26: The upgrade to `@scalar/openapi-parser` 0.28.7 and
`@scalar/openapi-types` ^0.9.1 introduces breaking changes that need validation.
First, verify that TypeScript compilation succeeds, as the stricter parameter
typing (where the `in` property is now a literal union instead of string) may
cause type failures in parameter-handling code. Second, update any imports that
rely on wildcard exports from `@scalar/openapi-types` to use explicit named
imports or the new versioned barrel exports (/2.0, /3.0, /3.1). Third, audit the
converter's parse-output handling logic to ensure it properly handles the
migration from `collectionFormat` to `style` and `explode` properties in parsed
OpenAPI documents. Run the test suite to confirm the converter behaves correctly
with the updated dependencies, and add tests if missing to cover these critical
contract changes.
In `@package.json`:
- Around line 8-14: The TypeScript package is incompatible with the current
version of typescript-eslint in package.json. Resolve this by either downgrading
the TypeScript dependency to a version less than 6.0.0, or upgrading the
typescript-eslint dependency to version 8.58.0 or higher. Since TypeScript 6.0.3
is present in the dependencies and typescript-eslint v8.57.2 only supports
TypeScript versions below 6.0.0, you must choose one of these two solutions to
ensure CI linting and type-checking tasks work correctly.
---
Outside diff comments:
In `@apps/cli/src/command/convert.command.ts`:
- Around line 108-112: The catch block in the convert.command.ts file (lines
108-112) is silently ignoring errors from the await tasks.run() call, preventing
proper error handling and status reporting. Replace the empty catch block that
contains only /* ignore */ with proper error handling that logs the error and/or
surfaces the failure to the user, such as logging the error details and either
rethrowing it or returning an appropriate error status/message to indicate the
conversion operation failed.
In `@apps/cli/src/command/ingress-server.command.ts`:
- Around line 80-97: Remove the non-null assertions on `listen`, `listenStatus`,
and related variables in the ingress-server.command.ts file. Create local
variables at the beginning of the code block that capture these values with
proper defaults, then use these local variables throughout the rest of the block
(in the HTTPS validation check, the ADCServer instantiation, and the logger.info
call) instead of using the assertion operator. This eliminates the need for the
`!` syntax on `listen!`, `listenStatus!`, and similar variables while
maintaining the same logic.
In `@apps/cli/src/command/ingress-sync.command.ts`:
- Around line 85-99: The code in the faileds mapping function is including raw
axiosResponse.headers and axiosResponse.data in the response object without
redacting sensitive information like authentication tokens, cookies, API keys,
or database connection strings. Create a utility function to redact sensitive
fields (such as authorization headers, bearer tokens, cookies, and other
sensitive data fields) and apply it to both axiosResponse.headers and
axiosResponse.data before they are included in the response object that gets
serialized to stdout. The redaction should mask or filter out values from known
sensitive keys while preserving the structure for debugging purposes.
- Around line 103-105: The catch block in the ingress-sync command is catching
errors and writing them to stderr but not rethrowing them, which allows the
Listr pipeline to incorrectly return a success status even after a failed sync.
After the process.stderr.write(toString(err)) call that logs the error, rethrow
the error by adding a throw statement to propagate the error up the pipeline and
cause it to properly fail.
In `@apps/cli/src/command/sync.command.ts`:
- Around line 101-110: The cancel() cleanup function from
addBackendEventListener is not being called when the sync() operation throws an
error, leaving event subscriptions active. Wrap the sync() operation in a
try/finally block, moving the cancel() call into the finally block to ensure it
executes regardless of success or failure. Additionally, validate that
ctx.backend and ctx.diff are not null before entering the try block to prevent
errors during the cleanup phase.
In `@apps/cli/src/server/index.ts`:
- Around line 64-83: The server initialization code has two issues: first, the
main server listener handle at line 73 is assigned to serverStatus but the
readiness server listener at line 81 is never stored, preventing proper cleanup
during shutdown; second, both Promise chains only resolve on success but never
reject on listen errors, violating the error handling guideline that all
function return values must be checked. Fix this by storing both server handles
(the return value from this.server!.listen() and this.expressStatus.listen()) so
they can be properly closed during shutdown, and add error event listeners to
both server instances within their respective Promise chains to catch and reject
on listen failures instead of silently swallowing errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be8693f3-3f04-43ea-87f4-83f5eb3481cf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
.gitignore.prettierignoreapps/cli/package.jsonapps/cli/src/command/convert.command.tsapps/cli/src/command/dump.command.tsapps/cli/src/command/helper.tsapps/cli/src/command/ingress-server.command.tsapps/cli/src/command/ingress-sync.command.tsapps/cli/src/command/ping.command.tsapps/cli/src/command/sync.command.tsapps/cli/src/command/utils.tsapps/cli/src/server/index.tsapps/cli/src/server/logger.tsapps/cli/src/server/sync.tsapps/cli/src/server/validate.tsapps/cli/src/tasks/diff.tsapps/cli/src/tasks/experimental.tsapps/cli/src/tasks/init_backend.tsapps/cli/src/tasks/lint.tsapps/cli/src/tasks/load_local.tsapps/cli/src/tasks/load_remote.tsapps/cli/src/tasks/validate.tsapps/cli/src/utils/listr.tslibs/backend-api7/tsconfig.lib.jsonlibs/backend-apisix-standalone/tsconfig.lib.jsonlibs/backend-apisix/tsconfig.lib.jsonlibs/converter-openapi/package.jsonlibs/converter-openapi/tsconfig.lib.jsonlibs/differ/tsconfig.lib.jsonlibs/sdk/tsconfig.lib.jsonpackage.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (4)
- libs/backend-apisix/tsconfig.lib.json
- libs/sdk/tsconfig.lib.json
- libs/backend-api7/tsconfig.lib.json
- libs/converter-openapi/tsconfig.lib.json
bzp2010
added
test/api7
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/server/index.ts`:
- Line 66: Remove the error listeners that are attached during server startup to
prevent them from swallowing runtime errors after successful bind. The
`once('error', reject)` listeners on lines 66 and 84 are attached to handle
startup failures but are never removed after the promise resolves successfully.
After startup completes, any subsequent server error events will trigger the
stale reject handler which has no effect on the already-settled promise,
effectively silencing the error. Use `removeListener('error', reject)` or store
the handler function and remove it after the server successfully listens,
ensuring that only actual runtime errors are being ignored in the cleanup, not
swallowed by a settled promise rejection handler.
- Around line 74-75: The parseInt(listen.port) call on line 74 can return NaN
when the URL has no explicit port, which causes the listen() method to fail with
a bind error. Add validation before the listen() call to guard against an
invalid or empty port value: check if listen.port is empty or if parseInt
produces NaN, then either use a sensible default port value or throw a clear
validation error explaining that the port is invalid or missing. This ensures
predictable behavior and proper error handling instead of silent bind failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc6f30b9-fd5a-4776-9bce-d4e1350ee54b
📒 Files selected for processing (9)
apps/cli/src/command/convert.command.tsapps/cli/src/command/dump.command.tsapps/cli/src/command/ingress-server.command.tsapps/cli/src/command/ingress-sync.command.tsapps/cli/src/command/ping.command.tsapps/cli/src/command/sync.command.tsapps/cli/src/server/index.tsapps/cli/src/tasks/experimental.tsapps/cli/src/tasks/load_remote.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cli/src/command/ingress-sync.command.ts
- apps/cli/src/tasks/load_remote.ts
- apps/cli/src/command/convert.command.ts
juzhiyuan
approved these changes
Jun 16, 2026
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
2 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.
Description
Checklist
Summary by CodeRabbit
commander, OpenAPI converter libraries, Nx/tooling, TypeScript, ESLint tooling, and updated workspace peer-install behavior and transitive overrides.