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 145-149: The catch around resolveEnvVarRefsFromMap silently
swallows errors causing later cryptic validation failures; update the catch in
the block that calls resolveEnvVarRefsFromMap(integrationRawJson, envVars) so it
either re-throws a new Error with a clear message referencing the env:
resolution failure (including the original error) or emits a user-visible
warning via the CLI logger (e.g., logger.warn or console.error) that explains
env var resolution failed for integrationRawJson and advises the user to check
environment variables; ensure you reference the original error when re-throwing
or logging so the root cause is preserved.
- Line 1: The file imports debug from the wrong module ("node:console") which
bypasses the CLI's --quiet/--debug flags; replace the import statement `import {
debug } from 'node:console'` with an import from the CLI output wrapper (e.g.
`import { debug } from '../../output'`) and ensure all uses of the debug
identifier (the call sites in this module) now call the exported debug from
../../output so the CLI flags are respected.

In `@packages/cli/src/federated-auth/federated-auth-tokens-schema.test.ts`:
- Around line 8-69: The tests miss edge cases for invalid expiresAt and
empty-string credentials; update the
federated-auth/federated-auth-tokens-schema.test.ts to add tests that assert
federatedAuthTokenEntrySchema rejects entries with a non-ISO or invalid
expiresAt (e.g., 'not-a-date' or malformed timestamp) and rejects entries where
accessToken or refreshToken are empty strings (''), and similarly add a case to
baseTokensFileSchema to ensure tokens containing such invalid entries are
rejected; use the existing FederatedAuthTokenEntry shape and the
federatedAuthTokenEntrySchema and baseTokensFileSchema identifiers to locate
where to add these test cases.

In `@packages/cli/src/federated-auth/federated-auth-tokens-schema.ts`:
- Around line 10-14: The schema federatedAuthTokenEntrySchema currently allows
empty strings for integrationId, accessToken, refreshToken (and for expiresAt
when present); update those fields to disallow empty strings by using Zod's
nonempty (or .min(1)) validator: change integrationId, accessToken, and
refreshToken to z.string().nonempty(), and make expiresAt
z.string().datetime().nonempty().optional() so an empty string does not pass
validation when provided.

In `@packages/cli/src/federated-auth/federated-auth-tokens.test.ts`:
- Around line 15-89: Add two tests to federated-auth-tokens.test.ts that
exercise readTokensFile's error paths: one "malformed YAML" test that writes
syntactically invalid YAML to a test file (use fs.writeFile) then calls
readTokensFile(testPath) and asserts result.tokens is [] and result.issues
contains a parse/error entry; and one "invalid-entry schema" test that writes
YAML with entries missing required fields or wrong types (e.g., an object
without integrationId or with non-string accessToken), then calls
readTokensFile(testPath) and asserts result.tokens excludes the invalid entries
and result.issues lists the schema validation problems. Use the existing testDir
pattern and reference the readTokensFile (and writeTokensFile if helpful)
functions to locate the logic under test.

In `@packages/cli/src/federated-auth/federated-auth-tokens.ts`:
- Around line 132-143: saveTokenForIntegration currently calls readTokensFile
which returns only valid tokens, so when you upsert via saveTokenForIntegration
(function) any malformed/raw entries from the original file are dropped
silently; change saveTokenForIntegration to preserve non-parsed/raw entries or
at minimum warn the user when malformed entries exist: modify readTokensFile (or
call a new reader) to return both parsed tokens and rawEntries, then in
saveTokenForIntegration merge the updated/inserted FederatedAuthTokenEntry into
the parsed tokens list while reassembling the final file contents to include
untouched rawEntries (or log a warning if rawEntries are present) before calling
writeTokensFile; reference tokens, entry, writeTokensFile and
getDefaultTokensFilePath when implementing this behavior.

In `@packages/cli/src/federated-auth/oauth-local-server.ts`:
- Around line 125-134: The current app.listen(...) call that creates server does
not handle bind errors (e.g., EADDRINUSE) so failures are ambiguous; attach an
explicit error handler to the returned server from app.listen (the server
variable) and in that handler check err.code (e.g., 'EADDRINUSE') to log a clear
message including OAUTH_PORT and startURL and exit the process (or suggest an
alternative port) rather than letting the process crash silently; ensure any
cleanup (closing server) is done as needed and keep the existing browser-open
logic (open(startURL)) in the listen callback.
- Around line 114-115: The HTML response embeds err.name and err.message
unescaped (the html variable), which allows XSS; update the OAuth error handler
to HTML-escape these values before interpolation (e.g., implement/use an
escapeHtml helper that replaces &, <, >, ", ' and /) and use escapedName =
escapeHtml(err.name) and escapedMessage = escapeHtml(err.message) when building
html, or alternatively render the error as plain text/JSON via
res.status(400).send to avoid HTML interpolation.
- Around line 107-112: The Express app is missing Passport initialization; call
passport.initialize() on the Express instance before any route handlers to
ensure Passport sets up req._passport even when using session: false. Add
app.use(passport.initialize()) immediately after creating app (before the routes
that reference START_PATH and CALLBACK_PATH and before the
passport.authenticate(...) calls) so passport.authenticate('trino-oauth', {
session: false }) runs with Passport properly initialized.

In `@packages/cli/src/federated-auth/token-refresh.test.ts`:
- Around line 47-126: Add unit tests in
packages/cli/src/federated-auth/token-refresh.test.ts to cover the error paths
of refreshAccessToken: simulate token endpoint responses for (1) OAuth
"invalid_grant" body (ok: true but error payload), (2) OAuth "invalid_client"
body, (3) non-OK HTTP response (ok: false, status/text), and (4) invalid JSON
(mock fetch.json to throw). For each case call refreshAccessToken with a valid
FederatedAuthTokenEntry and DatabaseIntegrationConfig, stub global fetch with
vi.stubGlobal/vi.fn() (as in the existing test), assert that refreshAccessToken
rejects with an appropriate error message or type, and clean up globals with
vi.unstubAllGlobals(); keep test structure consistent with the existing "should
call token endpoint..." test and use the same tokenUrl/clientId/clientSecret
setup.
- Around line 96-125: The fetch stub created with vi.stubGlobal('fetch') in the
test around mockFetch can leak if assertions fail; wrap the test execution and
assertions in a try/finally so vi.unstubAllGlobals() always runs. Locate the
test in token-refresh.test.ts that calls refreshAccessToken(token, integration,
'/tmp/test-tokens.yaml') and replace the current linear setup/assert/cleanup
with a try { ... assertions including
expect(mockFetch).toHaveBeenCalledWith(...); ... } finally {
vi.unstubAllGlobals(); } to guarantee the global stub is removed even on
failure.

In `@packages/cli/src/federated-auth/token-refresh.ts`:
- Around line 75-77: The code computes expiresAt using responseBody.expires_in
without validating it; update the logic around responseBody/expires_in
(affecting variables newRefreshToken, expiresIn, expiresAt) to first coerce and
validate expires_in is a finite positive number (e.g.,
Number(responseBody.expires_in)), and if it is NaN, <=0, or not finite, fall
back to the default (3600) before computing expiresAt; then compute expiresAt
from the validated expiresIn so negative/invalid values cannot produce bad
timestamps.
- Around line 42-46: The fetch call that posts to tokenUrl (using
getOAuth2ClientBasicAuthHeaders and tokenEntry.refreshToken) needs a timeout to
avoid hanging; wrap the fetch in an AbortController, pass controller.signal into
fetch, start a timer (e.g., 5–10s) that calls controller.abort() on expiry, and
clear the timer after fetch completes; also catch/handle the abort error path so
token refresh fails fast and surfaces a clear timeout error instead of hanging.
- Around line 23-27: In refreshAccessToken ensure you validate that the
tokenEntry is intended for the provided integration before using integration
metadata: check a stable identifier (e.g. tokenEntry.integrationId or
tokenEntry.id) matches integration.id (or integration.integrationId) at the
start of refreshAccessToken, and if it does not match, abort the flow (throw an
error or return a rejected Promise) rather than sending tokenEntry.refreshToken
to the integration endpoint; update any callers/tests to handle the new error
path if needed.
- Around line 93-99: The isTokenExpired function can compute expiryTime as NaN
for invalid expiresAt values, causing the subsequent comparison to incorrectly
mark an invalid timestamp as not expired; update isTokenExpired to detect
invalid dates (e.g., check Number.isNaN(expiryTime) or isNaN(new
Date(expiresAt).getTime())) and return true when expiryTime is invalid so any
malformed or non-parsable expiresAt is treated as expired; keep the existing
bufferSeconds logic and references to tokenEntry.expiresAt and expiryTime when
implementing this guard.

In `@packages/database-integrations/src/database-integration-env-vars.ts`:
- Around line 73-93: The federated detection and unsafe cast are causing Trino
OAuth configs stored only in integration.metadata to be missed and bypassing
runtime validation; update the logic in the block around isFederated and the
call to getValidFederatedAuthToken so that you check both
integration.federated_auth_method and integration.metadata.authMethod (or
equivalent) for 'trino-oauth' instead of relying solely on the top-level field,
and validate metadata at runtime before calling buildTrinoOAuthSqlAlchemyInput:
fetch metadata as DatabaseIntegrationMetadataByType['trino'], assert/guard that
metadata.authMethod === TrinoAuthMethods.Oauth, then call
buildTrinoOAuthSqlAlchemyInput with that validated metadata and push the env var
via getSqlEnvVarName only when validation succeeds.

In `@packages/database-integrations/src/federated-auth-tokens.ts`:
- Around line 171-177: The current try/catch around
refreshAccessToken(tokenEntry, integration, tokensFilePath) swallows failures
and returns undefined; instead let refresh errors propagate (or rethrow with
context) so callers can handle/report them. Remove the empty catch block (or
change it to catch (err) { throw new Error(`refreshAccessToken failed for
${integration}: ${err}`); }) and keep finalToken assignment as before; keep
references to tokenEntry, isTokenExpired, refreshAccessToken, finalToken and
tokensFilePath to locate the change.
- Around line 107-114: The token refresh fetch to tokenUrl should use an
AbortController with a timeout to avoid hanging: create an AbortController, pass
controller.signal to fetch, start a setTimeout (e.g., 10s) that calls
controller.abort(), and clear the timer when the response arrives; ensure fetch
rejections (including AbortError) are caught and the error is thrown/rejected up
(do not suppress) so callers see failures. Update the refresh logic that uses
tokenEntry.refreshToken and the fetch call to propagate errors instead of
swallowing them so token refresh failures are visible to callers.

---

Outside diff comments:
In `@packages/database-integrations/README.md`:
- Around line 160-162: The example calls getEnvironmentVariablesForIntegrations
synchronously but that function now returns a Promise; update the example to
await the async API by awaiting the call (or using .then) before destructuring
the returned object so envVars is obtained from the resolved value of
getEnvironmentVariablesForIntegrations(integrations, { projectRootDirectory:
"/path/to/project" }); reference getEnvironmentVariablesForIntegrations,
envVars, integrations, and projectRootDirectory when making the change.

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 83-108: Replace the manual type-and-empty checks in
extractTrinoOAuthParams with a zod schema validation: define a
trinoOAuthParamsSchema using z.object({ authUrl: z.string().min(1, ...),
tokenUrl: z.string().min(1,...), clientId: z.string().min(1,...), clientSecret:
z.string().min(1,...) }), run safeParse(metadata) inside
extractTrinoOAuthParams, and on failure throw a single Error composed from
result.error.issues messages; on success return result.data (the typed authUrl,
tokenUrl, clientId, clientSecret). Ensure zod is imported.
- Around line 197-203: In the catch block handling errors in the auth
integration, stop execution after handling ExitPromptError by returning (or
using an else) right after the program.error(chalk.yellow('Cancelled.'), {
exitCode: ExitCode.Error }) call so the subsequent generic error handler
(program.error(chalk.red(...), { exitCode: ExitCode.Error })) is not executed
for ExitPromptError; update the catch in the function containing that try/catch
to either add a return after the ExitPromptError branch or convert it to an
if/else.

In `@packages/cli/src/federated-auth/oauth-local-server.ts`:
- Around line 52-58: The OAuth flow promise can hang if the browser callback
never arrives; add an overall timeout to reject the flow after a configurable
interval (e.g., 2 minutes) by wiring a setTimeout that calls rejectOnce (or
rejects the outer Promise) and clearing that timer on successful resolution;
implement this by racing the existing completion promise against a timeout
promise (or invoking rejectOnce from the timer) in the function that creates
resolveOnce/rejectOnce and do the same for the second identical block referenced
(the block around identifiers completion, resolveOnce, rejectOnce and the
Promise returned later at lines ~138-146) so the CLI never waits indefinitely.

In `@packages/database-integrations/src/database-integration-env-vars.ts`:
- Around line 79-96: The current branch treats a missing/undefined accessToken
the same as an unsupported federated_auth_method and pushes "Unsupported
federated auth method"; update the conditional in the block handling
integration.type === 'trino' && integration.federated_auth_method ===
'trino-oauth' to first check for a truthy accessToken and, if missing, push a
clear error describing the missing/expired token (include integration.id and
federated_auth_method for context) instead of falling through to the generic
unsupported-auth error; keep the existing path that calls
buildTrinoOAuthSqlAlchemyInput and pushes envVarsForThisIntegration when
accessToken is present, and leave errors.push(new Error(`Unsupported federated
auth method: ${integration.federated_auth_method}`)) only for truly unsupported
methods.

In `@packages/database-integrations/src/federated-auth-tokens.ts`:
- Around line 127-130: Token files are being created with default filesystem
permissions that can be group/world-readable; update the write path in the code
that builds and persists tokens (the block that computes dir, calls fs.mkdir,
builds content via stringify and calls fs.writeFile) to create the directory and
file with restrictive modes (e.g., mkdir with mode 0o700 and fs.writeFile with
mode 0o600) so only the owner can read/write the token file; ensure you pass the
mode option to fs.writeFile and to fs.mkdir (or perform a chmod after creation)
when writing the updatedTokens to filePath.

---

Duplicate comments:
In `@packages/cli/src/federated-auth/oauth-local-server.ts`:
- Around line 116-117: The HTML response embeds untrusted err.name and
err.message directly; escape both values before interpolation to prevent XSS. In
oauth-local-server.ts where html is constructed (the const html = `...` line
inside the error handler that uses err and res), replace the direct
interpolation with escaped versions (use an HTML-escape helper function or small
utility that replaces &, <, >, ", ' with entities) and then send the escaped
content via res.status(400).send(html); ensure the helper is imported or defined
near that handler and used for both err.name and err.message.
- Around line 127-136: The current app.listen call creates server without
handling bind errors; update the logic around server = app.listen(OAUTH_PORT,
...) to attach an 'error' handler on the returned server (server.on('error',
...)) that catches failures like EADDRINUSE and rejects/throw a deterministic
error for the OAuth flow instead of letting it hang; ensure the same promise or
control flow that resolves on successful listen is rejected when server emits an
error, and include clear context (port/OAUTH_PORT and startURL) in the rejection
path so callers can handle the failure.

In `@packages/cli/src/federated-auth/token-refresh.test.ts`:
- Around line 55-132: The test suite for refreshAccessTokenAndSave in
token-refresh.test.ts lacks coverage for error paths (invalid_grant,
invalid_client/unauthorized_client, non-OK HTTP responses, invalid JSON
responses, and failures when saving tokens); add tests that stub global fetch
(mockFetch) to return: 1) a 200 with { error: 'invalid_grant' } and assert the
function rejects with a clear error, 2) a 400/401 with body indicating
invalid_client/unauthorized_client and assert rejection, 3) a non-ok response
(e.g., ok: false) and assert rejection, 4) a resolved fetch whose json() throws
(invalid JSON) and assert rejection, and 5) simulate a successful token response
but make the token-save path fail (mock file write or the internal save
function) and assert the save error is propagated; reference
refreshAccessTokenAndSave, mockFetch/vi.stubGlobal('fetch'), and the test token
fixtures to locate where to insert each new it() case.

In `@packages/database-integrations/src/federated-auth-tokens.ts`:
- Around line 207-214: The current block around isTokenExpired(tokenEntry)
swallows any error from refreshAccessToken(tokenEntry, integration) or
saveTokenForIntegration(finalToken, tokensFilePath) by returning undefined;
change the catch to propagate the error (include the caught error details)
instead of returning undefined so callers can handle or surface auth/config
failures — for example rethrow the caught error or throw a new Error that wraps
it; update the surrounding logic that expects undefined accordingly. Ensure this
change is applied to the tokens refresh flow involving finalToken,
refreshAccessToken, saveTokenForIntegration and tokenEntry.
- Around line 137-152: In refreshAccessToken, ensure the provided tokenEntry is
bound to the same integration before posting the refresh token: validate that
tokenEntry's integration identifier (e.g., tokenEntry.integrationId or
equivalent field on FederatedAuthTokenEntry) matches the integration passed in
(integration.id or another unique integration key), and if not throw an error;
only call fetchTokenWithTimeout(tokenUrl, clientId, clientSecret,
tokenEntry.refreshToken) after this check to prevent sending a refreshToken to
the wrong tokenUrl.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cli/src/commands/integrations/edit-integration.ts`:
- Around line 294-297: The catch block after calling resolveEnvVarRefsFromMap
sets debug using an interpolated unknown error; update the catch to explicitly
stringify the caught value before logging (e.g., use String(error) or check
error instanceof Error and use error.message/stack) so the debug call for
integrationRawJson resolution includes a safe, explicit error string; modify the
debug invocation that references resolveEnvVarRefsFromMap and integrationRawJson
to use that stringified error.

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.

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.