feat(cli): Add support for federated auth database integrations - #327
Draft
tkislan wants to merge 85 commits into
Draft
feat(cli): Add support for federated auth database integrations#327tkislan wants to merge 85 commits into
tkislan wants to merge 85 commits into
Conversation
…lution; remove unnecessary TypeScript error suppression in integration tests.
…er to V8 and adjust lint-staged commands
…s for @inquirer/testing; modify lint-staged commands to use nvm; remove inline dependency configuration from vitest setup.
…l' into tk/cli-federated-auth
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/cli/src/federated-auth/federated-auth-tokens.ts (1)
174-184:⚠️ Potential issue | 🟡 MinorUpsert can silently drop existing invalid entries
Line 176 ignores parse
issues; rewriting on Line 184 can discard malformed entries without warning.Suggested fix
export async function saveTokenForIntegration(entry: FederatedAuthTokenEntry, filePath?: string): Promise<void> { const resolvedPath = filePath ?? getDefaultTokensFilePath() - const { tokens } = await readTokensFile(resolvedPath) + const { tokens, issues } = await readTokensFile(resolvedPath) + if (issues.length > 0) { + throw new Error(`Cannot safely update tokens file at ${resolvedPath}: found ${issues.length} invalid entry issue(s).`) + } const existingIndex = tokens.findIndex(t => t.integrationId === entry.integrationId) const updatedTokens =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/federated-auth/federated-auth-tokens.ts` around lines 174 - 184, The saveTokenForIntegration function currently ignores parse issues returned by readTokensFile and may overwrite the tokens file, discarding malformed entries; update saveTokenForIntegration to handle readTokensFile's issues: call readTokensFile(resolvedPath) and inspect the returned issues array, and if issues.length > 0 either (a) surface/log/throw a descriptive error including those issues so the user can fix the file, or (b) preserve the original raw/malformed entries when constructing updatedTokens by merging the new/updated entry only into the valid tokens array while keeping the raw invalid entries intact before calling writeTokensFile; reference the saveTokenForIntegration function, the readTokensFile result (tokens and issues), and the writeTokensFile call when implementing this change.packages/database-integrations/src/database-integration-env-vars.ts (1)
78-101:⚠️ Potential issue | 🟠 MajorFederated Trino branch still conflates valid cases
Lines 78-101 currently mix up:
- metadata-only Trino OAuth detection,
- missing/expired token, and
- truly unsupported auth method.
That causes misleading errors and can skip valid federated configs.
Suggested fix
- const isFederated = - integration.federated_auth_method != null && isFederatedAuthMethod(integration.federated_auth_method) + const topLevelFederated = + integration.federated_auth_method != null && isFederatedAuthMethod(integration.federated_auth_method) + const trinoOauthFromMetadata = + integration.type === 'trino' && + 'authMethod' in integration.metadata && + integration.metadata.authMethod === TrinoAuthMethods.Oauth + const isFederated = topLevelFederated || trinoOauthFromMetadata if (isFederated && params.federatedAuthTokenResolver) { try { const accessToken = await params.federatedAuthTokenResolver(integration) - if (accessToken && integration.type === 'trino' && integration.federated_auth_method === 'trino-oauth') { + if (integration.type === 'trino' && trinoOauthFromMetadata) { + if (!accessToken) { + errors.push(new Error(`Missing or expired federated token for integration: ${integration.id}`)) + continue + } const metadata = integration.metadata as Extract< DatabaseIntegrationMetadataByType['trino'], { authMethod: typeof TrinoAuthMethods.Oauth } > const sqlAlchemyInput = buildTrinoOAuthSqlAlchemyInput( integration.id, metadata, accessToken, params.projectRootDirectory ) envVarsForThisIntegration.push({ name: getSqlEnvVarName(integration.id), value: JSON.stringify({ integration_id: integration.id, ...sqlAlchemyInput }), }) } else { errors.push(new Error(`Unsupported federated auth method: ${integration.federated_auth_method}`)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database-integrations/src/database-integration-env-vars.ts` around lines 78 - 101, The federated Trino branch conflates three cases (valid Trino OAuth with metadata, missing/expired accessToken, and truly unsupported auth method); update the logic in the block that calls params.federatedAuthTokenResolver so that: if integration.type === 'trino' and integration.federated_auth_method === 'trino-oauth' then validate metadata.authMethod (the extracted metadata variable) and only when metadata matches the expected OAuth shape call buildTrinoOAuthSqlAlchemyInput and push the env var; if accessToken is falsy push a clear token-specific error (or skip adding the unsupported-method error) instead of treating it as an unsupported auth method; otherwise (federated_auth_method present but not 'trino-oauth') push the unsupported-method error using integration.federated_auth_method. Ensure you reference isFederated, params.federatedAuthTokenResolver, accessToken, integration.federated_auth_method, integration.type, metadata, buildTrinoOAuthSqlAlchemyInput, and errors.push when implementing these branches.
🤖 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/cli/src/federated-auth/federated-auth-tokens.ts`:
- Around line 171-173: Fix the cspell warning by editing the comment block above
the token upsert function: change the word "Upserts" to singular "Upsert" in the
sentence that reads "Upserts by integrationId - replaces existing or appends."
(Look for the comment immediately above the save/update token logic or the
function that handles token upsert for integrations to update the text.)
In `@packages/cli/src/utils/secure-file.ts`:
- Around line 10-11: The current write step using fs.writeFile(filePath,
content, { mode: 0o600 }) only applies mode on newly created files; update the
logic in secure-file.ts to always harden permissions by calling await
fs.chmod(filePath, 0o600) after the write (after the existing await
fs.writeFile(...) line), ensuring filePath is set to 0o600 regardless of prior
existence; keep the mkdir(...) and writeFile(...) calls as-is and just add the
chmod call (optionally wrap chmod in a try/catch if you need to surface a clear
error).
In `@packages/database-integrations/src/database-integration-env-vars.ts`:
- Around line 102-113: Replace the unsafe casts of unknown to Error in the catch
blocks by normalizing the thrown value before pushing to errors: when catching
in the branches that reference isFederated and getEnvVarForSqlCells (the block
that pushes into envVarsForThisIntegration), convert the caught unknown into a
proper Error instance (e.g., use instanceof Error to keep Error objects or
create a new Error(String(error)) for non-Errors) and then push that normalized
Error into the errors array so errors always contains Error instances without
using `error as Error`.
---
Duplicate comments:
In `@packages/cli/src/federated-auth/federated-auth-tokens.ts`:
- Around line 174-184: The saveTokenForIntegration function currently ignores
parse issues returned by readTokensFile and may overwrite the tokens file,
discarding malformed entries; update saveTokenForIntegration to handle
readTokensFile's issues: call readTokensFile(resolvedPath) and inspect the
returned issues array, and if issues.length > 0 either (a) surface/log/throw a
descriptive error including those issues so the user can fix the file, or (b)
preserve the original raw/malformed entries when constructing updatedTokens by
merging the new/updated entry only into the valid tokens array while keeping the
raw invalid entries intact before calling writeTokensFile; reference the
saveTokenForIntegration function, the readTokensFile result (tokens and issues),
and the writeTokensFile call when implementing this change.
In `@packages/database-integrations/src/database-integration-env-vars.ts`:
- Around line 78-101: The federated Trino branch conflates three cases (valid
Trino OAuth with metadata, missing/expired accessToken, and truly unsupported
auth method); update the logic in the block that calls
params.federatedAuthTokenResolver so that: if integration.type === 'trino' and
integration.federated_auth_method === 'trino-oauth' then validate
metadata.authMethod (the extracted metadata variable) and only when metadata
matches the expected OAuth shape call buildTrinoOAuthSqlAlchemyInput and push
the env var; if accessToken is falsy push a clear token-specific error (or skip
adding the unsupported-method error) instead of treating it as an unsupported
auth method; otherwise (federated_auth_method present but not 'trino-oauth')
push the unsupported-method error using integration.federated_auth_method.
Ensure you reference isFederated, params.federatedAuthTokenResolver,
accessToken, integration.federated_auth_method, integration.type, metadata,
buildTrinoOAuthSqlAlchemyInput, and errors.push when implementing these
branches.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
packages/cli/src/cli.tspackages/cli/src/commands/integrations/edit-integration.tspackages/cli/src/commands/run.tspackages/cli/src/constants.tspackages/cli/src/federated-auth/federated-auth-tokens.tspackages/cli/src/federated-auth/token-refresh.tspackages/cli/src/integrations/inject-integration-env-vars.tspackages/cli/src/utils/secure-file.tspackages/database-integrations/src/database-integration-env-vars.tspackages/database-integrations/src/index.ts
Comment on lines
+10
to
+11
| await fs.mkdir(dir, { recursive: true }) | ||
| await fs.writeFile(filePath, content, { encoding: 'utf-8', mode: 0o600 }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js fs/promises.writeFile mode option behavior existing files
💡 Result:
In fs/promises.writeFile(path, data, { mode }), the mode is only applied if the file is newly created; it does not change permissions on an existing file. To change an existing file’s permissions, call chmod/fs.promises.chmod separately. [1]
Also, writeFile with the default flag: 'w' replaces/truncates the file if it already exists; use a different flag (e.g. 'a' to append, 'wx' to fail if the path already exists) if you don’t want that behavior. [2]
Sources:
[1] Node.js File system docs — fsPromises.writeFile: “The mode option only affects the newly created file.” (nodejs.org)
[2] Node.js File system docs — fs.writeFile options (flag default 'w') and “replacing the file if it already exists” (nodejs.org)
Citations:
mode: 0o600 does not apply to existing files
On Line 11, writeFile(..., { mode: 0o600 }) only sets permissions on newly created files—it leaves existing files with their original permissions intact. Add fs.chmod() to ensure permissions are always hardened.
Suggested fix
await fs.mkdir(dir, { recursive: true })
await fs.writeFile(filePath, content, { encoding: 'utf-8', mode: 0o600 })
+await fs.chmod(filePath, 0o600)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/utils/secure-file.ts` around lines 10 - 11, The current
write step using fs.writeFile(filePath, content, { mode: 0o600 }) only applies
mode on newly created files; update the logic in secure-file.ts to always harden
permissions by calling await fs.chmod(filePath, 0o600) after the write (after
the existing await fs.writeFile(...) line), ensuring filePath is set to 0o600
regardless of prior existence; keep the mkdir(...) and writeFile(...) calls
as-is and just add the chmod call (optionally wrap chmod in a try/catch if you
need to surface a clear error).
Comment on lines
+102
to
113
| } catch (error) { | ||
| errors.push(error as Error) | ||
| } | ||
| } else if (!isFederated) { | ||
| try { | ||
| const envVar = getEnvVarForSqlCells(integration, params) | ||
| if (envVar) { | ||
| envVarsForThisIntegration.push(envVar) | ||
| } | ||
| } catch (error) { | ||
| errors.push(error as Error) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid unchecked unknown→Error casts
Lines 103 and 112 use error as Error. Normalize first so non-Error throws are preserved safely.
Suggested fix
- } catch (error) {
- errors.push(error as Error)
+ } catch (error) {
+ errors.push(error instanceof Error ? error : new Error(String(error)))
}
} else if (!isFederated) {
try {
const envVar = getEnvVarForSqlCells(integration, params)
if (envVar) {
envVarsForThisIntegration.push(envVar)
}
} catch (error) {
- errors.push(error as Error)
+ errors.push(error instanceof Error ? error : new Error(String(error)))
}
}As per coding guidelines: "Prefer type safety over convenience in TypeScript".
📝 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
| } catch (error) { | |
| errors.push(error as Error) | |
| } | |
| } else if (!isFederated) { | |
| try { | |
| const envVar = getEnvVarForSqlCells(integration, params) | |
| if (envVar) { | |
| envVarsForThisIntegration.push(envVar) | |
| } | |
| } catch (error) { | |
| errors.push(error as Error) | |
| } | |
| } catch (error) { | |
| errors.push(error instanceof Error ? error : new Error(String(error))) | |
| } | |
| } else if (!isFederated) { | |
| try { | |
| const envVar = getEnvVarForSqlCells(integration, params) | |
| if (envVar) { | |
| envVarsForThisIntegration.push(envVar) | |
| } | |
| } catch (error) { | |
| errors.push(error instanceof Error ? error : new Error(String(error))) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database-integrations/src/database-integration-env-vars.ts` around
lines 102 - 113, Replace the unsafe casts of unknown to Error in the catch
blocks by normalizing the thrown value before pushing to errors: when catching
in the branches that reference isFederated and getEnvVarForSqlCells (the block
that pushes into envVarsForThisIntegration), convert the caught unknown into a
proper Error instance (e.g., use instanceof Error to keep Error objects or
create a new Error(String(error)) for non-Errors) and then push that normalized
Error into the errors array so errors always contains Error instances without
using `error as Error`.
- Introduced comprehensive tests for the `authIntegration` function, covering various scenarios including successful authentication, error handling for missing files, and validation of integration types. - Added tests for the `runOAuthFlow` function in the local OAuth server, ensuring end-to-end functionality and error handling. - Refactored the `auth-integration.ts` to improve type handling and integrate the new token saving functionality. - Enhanced the local OAuth server to better manage server lifecycle and error responses.
- Updated the `runOAuthFlow` function to accept a port parameter, allowing for dynamic configuration of the local server port. - Introduced `DEFAULT_OAUTH_PORT` constant for default port usage in the `authIntegration` function and local server tests. - Modified tests to utilize the new port configuration, enhancing flexibility and maintainability.
…l' into tk/cli-federated-auth
- Added a new `extractOAuthCredentials` function to handle OAuth credential extraction specifically for Trino integrations. - Updated the `authIntegration` function to utilize the new extraction method, improving code clarity and maintainability. - Removed outdated checks for federated authentication methods, streamlining the integration process.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/cli/src/federated-auth/oauth-local-server.ts (1)
161-169:⚠️ Potential issue | 🟠 MajorHandle
app.listenbind errors explicitly.Lines 161-169 do not attach a server
'error'handler, so port bind failures are abrupt and unclear.🛠 Suggested fix
server = app.listen(port, () => { log('Opening browser to authenticate...') log('If the browser does not open automatically, visit:') log(startURL) open(startURL).catch(err => { log('Error opening browser:') log(err instanceof Error ? err.message : String(err)) }) }) + server.on('error', err => { + const listenError = err instanceof Error ? err : new Error(String(err)) + reject(listenError) + })#!/bin/bash rg -n "app\\.listen|server\\.on\\('error'" packages/cli/src/federated-auth/oauth-local-server.ts # Expected: app.listen plus a server.on('error', ...) handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/federated-auth/oauth-local-server.ts` around lines 161 - 169, The call to app.listen(...) assigns server but doesn’t attach an 'error' handler, so bind failures (e.g., EADDRINUSE) crash unclearly; update the server returned by app.listen (the server variable) to attach server.on('error', ...) immediately after creation (or supply an error callback to app.listen) and handle known errors by logging a clear message via log(...) and exiting/cleaning up (handle error.code like 'EADDRINUSE' and other errors by logging error.message/stack and process.exit(1) or rejecting the promise used by oauth-local-server.ts); reference server and app.listen in your change.packages/cli/src/federated-auth/federated-auth-tokens.ts (2)
171-173:⚠️ Potential issue | 🟡 MinorRename “Upserts” to clear CI.
Line 172 still fails CSpell (
Unknown word (Upserts)).🛠 Suggested fix
- * Upserts by integrationId - replaces existing or appends. + * Upsert by integrationId - replaces existing or appends.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/federated-auth/federated-auth-tokens.ts` around lines 171 - 173, Replace the word "Upserts" in the comment block that starts with "Save or update a token entry for an integration." with a CSpell-friendly phrase; for example change "Upserts by integrationId - replaces existing or appends." to "Performs an upsert by integrationId — replaces existing or appends." so the comment above the token save/update function uses common words and passes the spellchecker.
174-184:⚠️ Potential issue | 🟠 Major
saveTokenForIntegrationstill drops malformed entries on write.Line 176 reads only valid parsed entries; Line 184 rewrites the file from that subset, so malformed/raw entries are silently lost.
🛠 Minimal mitigation
export async function saveTokenForIntegration(entry: FederatedAuthTokenEntry, filePath?: string): Promise<void> { const resolvedPath = filePath ?? getDefaultTokensFilePath() - const { tokens } = await readTokensFile(resolvedPath) + const { tokens, issues } = await readTokensFile(resolvedPath) + if (issues.length > 0) { + console.warn(`Warning: ${issues.length} invalid token entries in ${resolvedPath} will be omitted.`) + } const existingIndex = tokens.findIndex(t => t.integrationId === entry.integrationId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/federated-auth/federated-auth-tokens.ts` around lines 174 - 184, The current saveTokenForIntegration (which calls readTokensFile and writeTokensFile) overwrites the tokens file using only successfully parsed entries and thus drops any malformed/raw entries; change the flow so malformed/raw entries are preserved: update readTokensFile (or add a companion reader) to return both parsed tokens and the unparsed/raw entries (or raw file content), then in saveTokenForIntegration locate existingIndex within the parsed tokens, replace or append the parsed entry as you do now, and finally write back by merging the updated parsed tokens with the preserved raw/unparsed entries (so writeTokensFile receives a combined list or implement a writer that accepts parsed + raw) instead of discarding the raw entries. Use the symbols saveTokenForIntegration, readTokensFile, writeTokensFile, tokens, and entry to locate the code to change.
🤖 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/cli/src/commands/integrations/auth-integration.test.ts`:
- Around line 13-15: The test mock for '../../federated-auth/oauth-local-server'
is missing the DEFAULT_OAUTH_PORT export and the expectation for runOAuthFlow is
missing the port argument; update the vi.mock call to export both runOAuthFlow
and DEFAULT_OAUTH_PORT (matching the real module) and change the runOAuthFlow
expectation(s) (references to runOAuthFlow in the test) to include the port
parameter when comparing calls/args so the mocked shape and assertions match the
real call signature.
In `@packages/cli/src/federated-auth/oauth-local-server.ts`:
- Around line 74-83: The 5-minute flowTimeout is only cleared in one place,
leaving timers running on early errors; update the completion paths to always
clear the timer by calling clearTimeout(flowTimeout) wherever the flow ends: add
clearTimeout(flowTimeout) inside the reject function (alongside closeServer and
pReject) and also ensure any success path (where pResolve is called) and other
error handlers (the code around verify and the handlers referenced in the
comment) clearTimeout(flowTimeout) before closing the server or
resolving/rejecting so the timeout cannot leak.
- Around line 165-168: The error handler passed to open(startURL).catch uses an
invalid instanceof check (err instanceof err) which throws; update the catch
callback in oauth-local-server.ts to test the thrown value using "err instanceof
Error" and log err.message (or String(err) if not an Error) accordingly so the
handler for open(startURL) safely prints the error message.
---
Duplicate comments:
In `@packages/cli/src/federated-auth/federated-auth-tokens.ts`:
- Around line 171-173: Replace the word "Upserts" in the comment block that
starts with "Save or update a token entry for an integration." with a
CSpell-friendly phrase; for example change "Upserts by integrationId - replaces
existing or appends." to "Performs an upsert by integrationId — replaces
existing or appends." so the comment above the token save/update function uses
common words and passes the spellchecker.
- Around line 174-184: The current saveTokenForIntegration (which calls
readTokensFile and writeTokensFile) overwrites the tokens file using only
successfully parsed entries and thus drops any malformed/raw entries; change the
flow so malformed/raw entries are preserved: update readTokensFile (or add a
companion reader) to return both parsed tokens and the unparsed/raw entries (or
raw file content), then in saveTokenForIntegration locate existingIndex within
the parsed tokens, replace or append the parsed entry as you do now, and finally
write back by merging the updated parsed tokens with the preserved raw/unparsed
entries (so writeTokensFile receives a combined list or implement a writer that
accepts parsed + raw) instead of discarding the raw entries. Use the symbols
saveTokenForIntegration, readTokensFile, writeTokensFile, tokens, and entry to
locate the code to change.
In `@packages/cli/src/federated-auth/oauth-local-server.ts`:
- Around line 161-169: The call to app.listen(...) assigns server but doesn’t
attach an 'error' handler, so bind failures (e.g., EADDRINUSE) crash unclearly;
update the server returned by app.listen (the server variable) to attach
server.on('error', ...) immediately after creation (or supply an error callback
to app.listen) and handle known errors by logging a clear message via log(...)
and exiting/cleaning up (handle error.code like 'EADDRINUSE' and other errors by
logging error.message/stack and process.exit(1) or rejecting the promise used by
oauth-local-server.ts); reference server and app.listen in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7c149f0-83a7-4107-8c4f-11de79da5ebb
📒 Files selected for processing (5)
packages/cli/src/commands/integrations/auth-integration.test.tspackages/cli/src/commands/integrations/auth-integration.tspackages/cli/src/federated-auth/federated-auth-tokens.tspackages/cli/src/federated-auth/oauth-local-server.test.tspackages/cli/src/federated-auth/oauth-local-server.ts
Comment on lines
+74
to
+83
| const reject = (err: Error) => { | ||
| closeServer() | ||
| pReject(err) | ||
| } | ||
|
|
||
| const flowTimeoutMs = 5 * 60 * 1000 | ||
| const flowTimeout = setTimeout(() => { | ||
| reject(new Error('OAuth flow timed out. Please try again.')) | ||
| }, flowTimeoutMs) | ||
|
|
Contributor
There was a problem hiding this comment.
Clear flow timeout on every completion path.
The timer is cleared at Line 102 only. Errors before verify still leave a live 5-minute timeout.
🛠 Suggested fix
return new Promise<FederatedAuthTokenEntry>((pResolve, pReject) => {
let server: Server | null = null
+ let flowTimeout: NodeJS.Timeout | null = null
+
+ const clearFlowTimeout = () => {
+ if (flowTimeout != null) {
+ clearTimeout(flowTimeout)
+ flowTimeout = null
+ }
+ }
const closeServer = () => {
if (server != null) {
server.closeAllConnections()
server.close()
server = null
}
}
const resolve = (entry: FederatedAuthTokenEntry) => {
+ clearFlowTimeout()
closeServer()
pResolve(entry)
}
const reject = (err: Error) => {
+ clearFlowTimeout()
closeServer()
pReject(err)
}
const flowTimeoutMs = 5 * 60 * 1000
- const flowTimeout = setTimeout(() => {
+ flowTimeout = setTimeout(() => {
reject(new Error('OAuth flow timed out. Please try again.'))
}, flowTimeoutMs)Also applies to: 95-103, 143-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/federated-auth/oauth-local-server.ts` around lines 74 - 83,
The 5-minute flowTimeout is only cleared in one place, leaving timers running on
early errors; update the completion paths to always clear the timer by calling
clearTimeout(flowTimeout) wherever the flow ends: add clearTimeout(flowTimeout)
inside the reject function (alongside closeServer and pReject) and also ensure
any success path (where pResolve is called) and other error handlers (the code
around verify and the handlers referenced in the comment)
clearTimeout(flowTimeout) before closing the server or resolving/rejecting so
the timeout cannot leak.
Comment on lines
+165
to
+168
| open(startURL).catch(err => { | ||
| log('Error opening browser:') | ||
| log(err instanceof err ? err.message : String(err)) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cli/src/federated-auth/oauth-local-server.ts | sed -n '165,168p'Repository: deepnote/deepnote
Length of output: 231
🏁 Script executed:
rg -n "instanceof err" packages/cli/src/federated-auth/oauth-local-server.tsRepository: deepnote/deepnote
Length of output: 124
Fix invalid instanceof check in error logging.
Line 167 uses err instanceof err, which throws TypeError at runtime since the right operand must be a constructor. Use err instanceof Error instead.
Fix
open(startURL).catch(err => {
log('Error opening browser:')
- log(err instanceof err ? err.message : String(err))
+ log(err instanceof Error ? err.message : String(err))
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/federated-auth/oauth-local-server.ts` around lines 165 -
168, The error handler passed to open(startURL).catch uses an invalid instanceof
check (err instanceof err) which throws; update the catch callback in
oauth-local-server.ts to test the thrown value using "err instanceof Error" and
log err.message (or String(err) if not an Error) accordingly so the handler for
open(startURL) safely prints the error message.
…or Redshift and Trino integrations - Consolidated authentication logic for Redshift and Trino integrations, introducing dedicated functions for handling different auth methods (e.g., IAM Role, OAuth). - Updated prompts to ensure correct field handling based on selected authentication methods, including clearing defaults when switching methods. - Added tests to verify integration behavior when changing authentication methods, ensuring proper state management and user experience. - Improved code organization and readability across integration files.
…l' into tk/cli-federated-auth
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/cli/src/commands/integrations/auth-integration.ts (1)
185-191:⚠️ Potential issue | 🔴 CriticalMissing return after
ExitPromptErrorhandling.
program.error()doesn't return/throw by default. Execution falls through to line 189-190, callingprogram.error()twice.Fix
if (error instanceof Error && error.name === 'ExitPromptError') { - program.error(chalk.yellow('Cancelled.'), { exitCode: ExitCode.Error }) + return program.error(chalk.yellow('Cancelled.'), { exitCode: ExitCode.Error }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/integrations/auth-integration.ts` around lines 185 - 191, The catch block handling ExitPromptError incorrectly falls through and calls program.error twice; inside the catch for error instanceof Error && error.name === 'ExitPromptError' (in the catch handling in auth-integration.ts) ensure control stops after logging the cancel message by returning or re-throwing (e.g., return after program.error(chalk.yellow('Cancelled.'), { exitCode: ExitCode.Error })) so the subsequent generic program.error(chalk.red(message), ...) is not executed for ExitPromptError cases.
🤖 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/cli/src/commands/integrations/auth-integration.ts`:
- Around line 166-170: The test mock for the oauth-local-server only exports
runOAuthFlow but the code under test also imports DEFAULT_OAUTH_PORT; update the
mock used in tests to export DEFAULT_OAUTH_PORT as a number (e.g. 21337)
alongside runOAuthFlow so imports resolve correctly—ensure the mock object
includes DEFAULT_OAUTH_PORT and runOAuthFlow (the latter kept as vi.fn()) to
match the module shape expected by the code that calls runOAuthFlow and
references DEFAULT_OAUTH_PORT.
---
Duplicate comments:
In `@packages/cli/src/commands/integrations/auth-integration.ts`:
- Around line 185-191: The catch block handling ExitPromptError incorrectly
falls through and calls program.error twice; inside the catch for error
instanceof Error && error.name === 'ExitPromptError' (in the catch handling in
auth-integration.ts) ensure control stops after logging the cancel message by
returning or re-throwing (e.g., return after
program.error(chalk.yellow('Cancelled.'), { exitCode: ExitCode.Error })) so the
subsequent generic program.error(chalk.red(message), ...) is not executed for
ExitPromptError cases.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1c8501cb-77e6-459d-925f-fd13387ce3a3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/cli/package.jsonpackages/cli/src/cli.tspackages/cli/src/commands/integrations/auth-integration.tspackages/cli/src/commands/integrations/edit-integration.tspackages/cli/src/commands/run.ts
Comment on lines
+166
to
+170
| const tokenEntry = await runOAuthFlow({ | ||
| integrationId: integration.id, | ||
| ...oauthCredentials, | ||
| port: DEFAULT_OAUTH_PORT, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the test file and check how oauth-local-server is mocked
rg -n "vi\.mock.*oauth-local-server" --type ts -A 10Repository: deepnote/deepnote
Length of output: 1218
🏁 Script executed:
# Check the imports in auth-integration.ts and verify DEFAULT_OAUTH_PORT usage
cat -n packages/cli/src/commands/integrations/auth-integration.ts | head -50Repository: deepnote/deepnote
Length of output: 2265
🏁 Script executed:
# Search for DEFAULT_OAUTH_PORT in auth-integration.ts
rg -n "DEFAULT_OAUTH_PORT" packages/cli/src/commands/integrations/auth-integration.tsRepository: deepnote/deepnote
Length of output: 188
🏁 Script executed:
# Check what oauth-local-server module actually exports
cat -n packages/cli/src/federated-auth/oauth-local-server.ts | head -100Repository: deepnote/deepnote
Length of output: 3918
Add DEFAULT_OAUTH_PORT to mock export.
Test mock of oauth-local-server exports only runOAuthFlow, but the code imports and uses DEFAULT_OAUTH_PORT. Update mock to include it:
Mock update needed
vi.mock('../../federated-auth/oauth-local-server', () => ({
DEFAULT_OAUTH_PORT: 21337,
runOAuthFlow: vi.fn(),
}))🧰 Tools
🪛 GitHub Actions: CI
[error] 169-169: [vitest] No "DEFAULT_OAUTH_PORT" export is defined on the "../../federated-auth/oauth-local-server" mock. port: DEFAULT_OAUTH_PORT is referenced but not exported from the vi.mock mock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/integrations/auth-integration.ts` around lines 166
- 170, The test mock for the oauth-local-server only exports runOAuthFlow but
the code under test also imports DEFAULT_OAUTH_PORT; update the mock used in
tests to export DEFAULT_OAUTH_PORT as a number (e.g. 21337) alongside
runOAuthFlow so imports resolve correctly—ensure the mock object includes
DEFAULT_OAUTH_PORT and runOAuthFlow (the latter kept as vi.fn()) to match the
module shape expected by the code that calls runOAuthFlow and references
DEFAULT_OAUTH_PORT.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cli/src/federated-auth/federated-auth-tokens.ts (1)
174-184:⚠️ Potential issue | 🟠 MajorPrevent silent token loss during upsert.
saveTokenForIntegrationrewrites from only valid parsed entries. If the file contains malformed entries, they get dropped on save without notice.Proposed minimal fix (warn on drop)
export async function saveTokenForIntegration(entry: FederatedAuthTokenEntry, filePath?: string): Promise<void> { const resolvedPath = filePath ?? getDefaultTokensFilePath() - const { tokens } = await readTokensFile(resolvedPath) + const { tokens, issues } = await readTokensFile(resolvedPath) + if (issues.length > 0) { + console.warn(`Warning: ${issues.length} invalid token entries were ignored from ${resolvedPath}`) + } const existingIndex = tokens.findIndex(t => t.integrationId === entry.integrationId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/federated-auth/federated-auth-tokens.ts` around lines 174 - 184, saveTokenForIntegration currently calls readTokensFile which silently discards malformed entries, causing silent token loss when writeTokensFile overwrites the file; modify the flow so readTokensFile returns metadata about dropped/malformed entries (e.g., a list or count) or expose the raw parsed results, then in saveTokenForIntegration detect when any malformed entries were present and emit a warning (via the project logger or console) before calling writeTokensFile; touch the functions readTokensFile, saveTokenForIntegration and writeTokensFile (and reference FederatedAuthTokenEntry, getDefaultTokensFilePath) to propagate/report the dropped-entry info so users are warned when an upsert would remove malformed entries.
🤖 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/cli/src/federated-auth/federated-auth-tokens.test.ts`:
- Around line 16-21: The test for getDefaultTokensFilePath uses an unescaped
RegExp against path.join(os.homedir(), '.deepnote') which can fail on Windows
due to backslash escapes; update the assertion to use startsWith by calling
expect(filePath).toStartWith? Actually use
expect(filePath.startsWith(path.join(os.homedir(), '.deepnote'))).toBe(true) or
prefer Jest's expect(filePath.startsWith(path.join(os.homedir(),
'.deepnote'))).toBeTruthy(), replacing the RegExp check in the test for
getDefaultTokensFilePath to ensure cross-platform safety and avoid regex escape
issues.
In `@packages/cli/src/federated-auth/federated-auth-tokens.ts`:
- Around line 209-218: The refreshAccessToken function currently extracts
tokenUrl, clientId, and clientSecret from integration.metadata without runtime
validation; add a Zod schema (e.g., OAuthMetadataSchema) and use
OAuthMetadataSchema.parse(integration.metadata) inside refreshAccessToken (or
where DatabaseIntegrationConfig is handled) to validate and coerce the metadata
at runtime, then destructure tokenUrl, clientId, clientSecret from the parsed
result and throw a clear error if parse fails; update any function/class
references (refreshAccessToken, DatabaseIntegrationConfig, integration.metadata)
to rely on the parsed type instead of unvalidated casts.
---
Duplicate comments:
In `@packages/cli/src/federated-auth/federated-auth-tokens.ts`:
- Around line 174-184: saveTokenForIntegration currently calls readTokensFile
which silently discards malformed entries, causing silent token loss when
writeTokensFile overwrites the file; modify the flow so readTokensFile returns
metadata about dropped/malformed entries (e.g., a list or count) or expose the
raw parsed results, then in saveTokenForIntegration detect when any malformed
entries were present and emit a warning (via the project logger or console)
before calling writeTokensFile; touch the functions readTokensFile,
saveTokenForIntegration and writeTokensFile (and reference
FederatedAuthTokenEntry, getDefaultTokensFilePath) to propagate/report the
dropped-entry info so users are warned when an upsert would remove malformed
entries.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 65278fa6-6906-4dcd-813f-e74a2b5f8b3b
📒 Files selected for processing (5)
cspell.jsonpackages/cli/src/commands/integrations/auth-integration.test.tspackages/cli/src/federated-auth/federated-auth-tokens-schema.test.tspackages/cli/src/federated-auth/federated-auth-tokens.test.tspackages/cli/src/federated-auth/federated-auth-tokens.ts
Comment on lines
+16
to
+21
| it('getDefaultTokensFilePath returns path in home directory', () => { | ||
| const filePath = getDefaultTokensFilePath() | ||
| expect(filePath).toContain('.deepnote') | ||
| expect(filePath).toContain('federated-auth-tokens.yaml') | ||
| expect(filePath).toMatch(new RegExp(`^${path.join(os.homedir(), '.deepnote')}`)) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cli/src/federated-auth/federated-auth-tokens.test.ts | head -50Repository: deepnote/deepnote
Length of output: 2142
🏁 Script executed:
# Check if there are any escaping utilities or patterns in similar tests
rg -A 3 -B 3 "toMatch.*RegExp.*path\.join\|startsWith.*path\.join" packages/cliRepository: deepnote/deepnote
Length of output: 43
🏁 Script executed:
# Check what path.join produces on different platforms and if it needs escaping in regex
python3 << 'EOF'
import re
import os
from pathlib import Path
# Simulate what happens with path.join on Windows vs Unix
# Windows path example (backslashes)
windows_path = "C:\\Users\\test\\.deepnote"
unix_path = "/home/test/.deepnote"
# Test 1: Does the Windows path cause regex issues?
test_string_win = "C:\\Users\\test\\.deepnote\\federated-auth-tokens.yaml"
test_string_unix = "/home/test/.deepnote/federated-auth-tokens.yaml"
# Current approach (unescaped)
try:
pattern_win = f"^{windows_path}"
regex_win = re.compile(pattern_win)
print(f"Windows unescaped pattern: {pattern_win}")
print(f"Matches test string: {bool(regex_win.match(test_string_win))}")
except Exception as e:
print(f"Windows unescaped error: {e}")
try:
pattern_unix = f"^{unix_path}"
regex_unix = re.compile(pattern_unix)
print(f"Unix unescaped pattern: {pattern_unix}")
print(f"Matches test string: {bool(regex_unix.match(test_string_unix))}")
except Exception as e:
print(f"Unix unescaped error: {e}")
# Correct approach (escaped)
try:
pattern_win_escaped = f"^{re.escape(windows_path)}"
regex_win_escaped = re.compile(pattern_win_escaped)
print(f"\nWindows escaped pattern: {pattern_win_escaped}")
print(f"Matches test string: {bool(regex_win_escaped.match(test_string_win))}")
except Exception as e:
print(f"Windows escaped error: {e}")
# startsWith approach (simpler)
print(f"\nstartsWith approach:")
print(f"Windows: {test_string_win.startswith(windows_path)}")
print(f"Unix: {test_string_unix.startswith(unix_path)}")
EOFRepository: deepnote/deepnote
Length of output: 320
Replace dynamic RegExp with startsWith for cross-platform safety.
The unescaped path in the RegExp causes regex escape errors on Windows (e.g., \U sequences). Use startsWith() instead—simpler and works everywhere.
Suggested change
expect(filePath).toContain('.deepnote')
expect(filePath).toContain('federated-auth-tokens.yaml')
- expect(filePath).toMatch(new RegExp(`^${path.join(os.homedir(), '.deepnote')}`))
+ expect(filePath.startsWith(path.join(os.homedir(), '.deepnote'))).toBe(true)📝 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
| it('getDefaultTokensFilePath returns path in home directory', () => { | |
| const filePath = getDefaultTokensFilePath() | |
| expect(filePath).toContain('.deepnote') | |
| expect(filePath).toContain('federated-auth-tokens.yaml') | |
| expect(filePath).toMatch(new RegExp(`^${path.join(os.homedir(), '.deepnote')}`)) | |
| }) | |
| it('getDefaultTokensFilePath returns path in home directory', () => { | |
| const filePath = getDefaultTokensFilePath() | |
| expect(filePath).toContain('.deepnote') | |
| expect(filePath).toContain('federated-auth-tokens.yaml') | |
| expect(filePath.startsWith(path.join(os.homedir(), '.deepnote'))).toBe(true) | |
| }) |
🧰 Tools
🪛 ast-grep (0.42.0)
[warning] 19-19: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${path.join(os.homedir(), '.deepnote')})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/federated-auth/federated-auth-tokens.test.ts` around lines
16 - 21, The test for getDefaultTokensFilePath uses an unescaped RegExp against
path.join(os.homedir(), '.deepnote') which can fail on Windows due to backslash
escapes; update the assertion to use startsWith by calling
expect(filePath).toStartWith? Actually use
expect(filePath.startsWith(path.join(os.homedir(), '.deepnote'))).toBe(true) or
prefer Jest's expect(filePath.startsWith(path.join(os.homedir(),
'.deepnote'))).toBeTruthy(), replacing the RegExp check in the test for
getDefaultTokensFilePath to ensure cross-platform safety and avoid regex escape
issues.
Comment on lines
+209
to
+218
| const metadata = integration.metadata as { | ||
| tokenUrl: string | ||
| clientId: string | ||
| clientSecret: string | ||
| } | ||
|
|
||
| const { tokenUrl, clientId, clientSecret } = metadata | ||
| if (!tokenUrl || !clientId || !clientSecret) { | ||
| throw new Error('Token refresh requires tokenUrl, clientId, and clientSecret in integration metadata') | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify DatabaseIntegrationConfig metadata typing and refreshAccessToken callsites.
rg -n --type=ts 'interface DatabaseIntegrationConfig|type DatabaseIntegrationConfig|metadata\??:'
rg -n --type=ts '\brefreshAccessToken\s*\('Repository: deepnote/deepnote
Length of output: 50373
🏁 Script executed:
# Find the DatabaseIntegrationConfig definition
fd DatabaseIntegrationConfig -t ts | head -20Repository: deepnote/deepnote
Length of output: 230
🏁 Script executed:
# Read the specific file mentioned in the review
cat -n packages/cli/src/federated-auth/federated-auth-tokens.ts | head -230 | tail -50Repository: deepnote/deepnote
Length of output: 1983
🏁 Script executed:
# Search for Zod schema definitions in database-integrations or nearby packages
rg "z\.object|z\.string" packages/database-integrations -t ts -A 3 | head -50Repository: deepnote/deepnote
Length of output: 5240
🏁 Script executed:
# Check if Zod is already a dependency
cat packages/cli/package.json | grep -i zodRepository: deepnote/deepnote
Length of output: 117
🏁 Script executed:
# Find DatabaseIntegrationConfig definition
rg "type DatabaseIntegrationConfig|interface DatabaseIntegrationConfig" -A 5Repository: deepnote/deepnote
Length of output: 5318
🏁 Script executed:
# Search for OAuth-related metadata schemas
rg "tokenUrl|clientId|clientSecret" packages/database-integrations -t ts -B 2 -A 2Repository: deepnote/deepnote
Length of output: 28415
🏁 Script executed:
# Check how DatabaseIntegrationConfig is imported in the target file
head -30 packages/cli/src/federated-auth/federated-auth-tokens.tsRepository: deepnote/deepnote
Length of output: 1140
🏁 Script executed:
# Find and read the DatabaseIntegrationConfig schema definition
cat packages/database-integrations/src/database-integration-config.tsRepository: deepnote/deepnote
Length of output: 5085
🏁 Script executed:
# Search for federation auth related schemas
rg "federatedAuth|FederatedAuth" packages/database-integrations -A 5 -B 2Repository: deepnote/deepnote
Length of output: 40901
🏁 Script executed:
# Check the context of refreshAccessToken function - where is it called and with what integration types
rg "refreshAccessToken" packages/cli/src/federated-auth -B 5 -A 5Repository: deepnote/deepnote
Length of output: 7165
🏁 Script executed:
# Look for the broader usage in the codebase to understand the function's intended scope
rg "refreshAccessToken" packages/cli/src -lRepository: deepnote/deepnote
Length of output: 220
🏁 Script executed:
# Check if similar patterns exist elsewhere in the codebase
rg "integration\.metadata as \{" packages/cli/src -B 2 -A 5Repository: deepnote/deepnote
Length of output: 712
🏁 Script executed:
# Check the test to understand the intended usage
cat -n packages/cli/src/federated-auth/token-refresh.test.ts | head -100Repository: deepnote/deepnote
Length of output: 3832
🏁 Script executed:
# Look at how the database-integration-env-vars handles metadata validation for comparison
rg "isFederatedAuthMetadata|Extract.*metadata" packages/database-integrations/src/database-integration-env-vars.ts -B 2 -A 3 | head -60Repository: deepnote/deepnote
Length of output: 370
Add Zod validation for OAuth metadata in refreshAccessToken.
The function accepts any DatabaseIntegrationConfig but needs specific OAuth fields. Using a Zod schema for parsing will enforce type safety at runtime and provide clearer error messages instead of relying on the runtime guard.
Safer validation approach
+const oauthMetadataSchema = z.object({
+ tokenUrl: z.string().trim().min(1),
+ clientId: z.string().trim().min(1),
+ clientSecret: z.string().trim().min(1),
+})
export async function refreshAccessToken(
tokenEntry: FederatedAuthTokenEntry,
integration: DatabaseIntegrationConfig
): Promise<FederatedAuthTokenEntry> {
- const metadata = integration.metadata as {
- tokenUrl: string
- clientId: string
- clientSecret: string
- }
-
- const { tokenUrl, clientId, clientSecret } = metadata
- if (!tokenUrl || !clientId || !clientSecret) {
+ const metadataResult = oauthMetadataSchema.safeParse(integration.metadata)
+ if (!metadataResult.success) {
throw new Error('Token refresh requires tokenUrl, clientId, and clientSecret in integration metadata')
}
+ const { tokenUrl, clientId, clientSecret } = metadataResult.dataThis aligns with "Use strict type checking in TypeScript files" and "Prefer type safety over convenience in TypeScript" from the coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/federated-auth/federated-auth-tokens.ts` around lines 209 -
218, The refreshAccessToken function currently extracts tokenUrl, clientId, and
clientSecret from integration.metadata without runtime validation; add a Zod
schema (e.g., OAuthMetadataSchema) and use
OAuthMetadataSchema.parse(integration.metadata) inside refreshAccessToken (or
where DatabaseIntegrationConfig is handled) to validate and coerce the metadata
at runtime, then destructure tokenUrl, clientId, clientSecret from the parsed
result and throw a clear error if parse fails; update any function/class
references (refreshAccessToken, DatabaseIntegrationConfig, integration.metadata)
to rely on the parsed type instead of unvalidated casts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests