[peco-engineer-bot]

Summary

Addresses #494 (LOGGING-002). A filepath that names an existing directory now writes to <dir>/databricks-sql-nodejs.log instead of being opened as a file, in line with JDBC and ODBC LogPath and the kernel.

The directory case never crashed. winston's File transport swallows stream open errors, so the file never opened, and once its buffer filled the logger stalled for every transport, including the console. Any unwritable path did the same. DBSQLLogger now checks that the file can be opened for appending before adding the transport. If it can't, it logs a warning and logs to the console only.

A trailing separator is not treated as a directory unless the path already is one. In 3.0.0, filepath: 'logs/' created a file named logs. Resolving that to logs/databricks-sql-nodejs.log crashed the process with ENOTDIR on upgrade, so the 3.0.0 behaviour is kept.

Testing

Unit tests cover:

• directory resolution, with and without a trailing separator
• reading log lines back from the resolved file
• a trailing separator on an existing file (the previous revision of this PR crashes on it)
• the console-only fallback

The live-warehouse e2e spec is removed because the unit tests cover the same behaviour. npm test (1488 passing), lint and prettier pass. npm run type-check only reports the existing examples/tokenFederation errors.

Still open for maintainers: the default file name, and whether to add a CHANGELOG entry. databricks-sql-nodejs.log follows the kernel's naming. JDBC and ODBC use databricks_jdbc.log and databricks_odbc.log.

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Medium · 2 Nit

Looks good overall: the directory-to-file resolution is small and correct. The main gap is that there's no unit test for it, even though the PR description lists one; the e2e spec also leaves the transport open and its listeners attached.

Other findings

• ⚪ Nit — runWorkload only removes the uncaughtException listener in finally. The error listeners added to logger.logger and logger.transports.file are never removed, and the winston logger/file stream is never closed (no logger.logger.close()). afterEach then rmSyncs the directory while the file descriptor may still be open. On Linux this leaks the fd. On Windows it makes rmSync fail with EBUSY/EPERM. Close the logger (and wait for its finish event) before removing the temp dir.
• ⚪ Nit — LoggerOptions.filepath now has a new public behavior: an existing directory resolves to <dir>/databricks-sql-nodejs.log. Nothing documents this, and the PR author says they picked the file name themselves. This is a public API, so add a JSDoc comment on filepath (and/or a README note) describing the directory behavior and the default file name so callers can find their logs.

[peco-review-bot]

Verdict: 1 Medium · 2 Low

The fix is small and correct, with a few low/medium concerns. Checking for an existing directory before creating the winston File transport resolves the EISDIR problem, and the unit and e2e tests cover both the directory and the file-path forms. The concerns are a possible teardown race in the new unit tests and a few edge-case and documentation gaps.

[peco-review-bot]

Verdict: 2 Low

Looks good overall. The directory-to-default-file resolution is correct, safely falls back to the old behavior when stat fails, and has unit and e2e tests. Two low concerns: the e2e spec doesn't clean up the logger it creates, and the default file name plus trailing-separator behavior need a maintainer's sign-off. Also, the PR description's "Files changed" list leaves out lib/contracts/IDBSQLLogger.ts and tests/unit/DBSQLLogger.test.ts, which the diff does touch.

[peco-review-bot]

🔵 Low — Treating a trailing / (or path.sep) as "directory, create it" goes beyond the issue's scope, which was an existing directory. It also changes behavior that callers could see: before, filepath: 'logs/' was handed straight to winston. Defaulting the file name to databricks-sql-nodejs.log is the PR author's own choice (as the description admits) and becomes part of the public contract through the new JSDoc. A maintainer should confirm this name and the trailing-separator semantics are wanted before merge, and a CHANGELOG entry may be needed because this is user-visible logging behavior.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

I agree with the point, but a maintainer has to make the call, so I haven't changed any code. resolveLogFilePath in lib/DBSQLLogger.ts does two things. First, it covers the reported bug: a filepath that names an existing directory now writes to a file inside it instead of failing with EISDIR. Second, it adds behavior the issue didn't ask for: a trailing / or path.sep is treated as a directory, and winston creates that directory if it's missing. Before this PR, 'logs/' went straight to winston, so the second part changes what callers see. A maintainer needs to decide: (1) whether databricks-sql-nodejs.log is the right default file name, now that the JSDoc publishes it; (2) whether to keep the trailing-separator handling or limit the fix to existing directories, which means deleting the endsWith('/') || endsWith(path.sep) branch and its tests; (3) whether to add a CHANGELOG entry. CHANGELOG.md has no Unreleased section and its entries go under maintainer-chosen version headings (latest is ## 3.0.0), so I can't add an entry without picking a release version. Once those are decided, either change is small and can go in this PR.

[peco-review-bot]

Verdict: 1 Nit

Looks good. The directory check is small and correct: statSync follows symlinks, and errors from missing or inaccessible paths fall back to using the path as given. The new doc comment on the contract matches what the code does. I have one low test-precision note. Separately, the PR description's "Files changed" list leaves out lib/contracts/IDBSQLLogger.ts and tests/unit/DBSQLLogger.test.ts, which this diff also modifies.

Other findings

• ⚪ Nit — The directory-case e2e test only checks that some regular file with content shows up in the directory. It doesn't check that the file is databricks-sql-nodejs.log, which is the name IDBSQLLogger.ts now documents as the contract. Compare the file-path case at line 105, which does assert the exact list of files. Asserting deep.equal([path.join(dir, 'databricks-sql-nodejs.log')]) here would catch an accidental rename or a stray extra file. The unit test covers the name, but only through winston's internal filename/dirname fields, not the file on disk.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks reasonable overall, with one medium issue: the console-only fallback warning can be filtered out by the configured log level. Separately, the PR description and "Files changed" list name tests/e2e/logging.test.ts and describe EISDIR/uncaughtException e2e assertions, but that file isn't in the diff. The coverage that did land is unit-only, and the diff also changes lib/contracts/IDBSQLLogger.ts, which the list doesn't include, so the description should be reconciled with the actual change.

[peco-review-bot]

Verdict: 2 Low

Looks good: the directory resolution and the console-only fallback are correct, and the trailing-separator case keeps the 3.0.0 behaviour. Two low findings: the fallback is logged at error while the description says it's a warning, and there's no test for creating missing parent directories. The PR says the live-warehouse e2e spec was removed, but that deletion isn't in the diff. Check that it actually landed.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low concern. The directory resolution and the writability probe before attaching the file transport are correct, and they keep the 3.0.0 trailing-separator behaviour for existing files. Unit tests cover the resolution branches and the console-only fallback.

[peco-review-bot]

🔵 Low — The fallback notice is sent at level: error with the text [warn] ... (non-fatal ...). The comment says the wording keeps log-based alerting from treating this as a driver failure. But structured consumers filter on the winston level field, not the message text, so error-level alerting will still fire on a config fallback. To surface it without breaking the level contract, you could write it straight to console.warn/process.emitWarning, or send it at warn and accept that level: error users won't see it. If you keep error on purpose, adjust the comment so it doesn't promise the message text changes alerting behaviour.

[peco-engineer-bot]

The smallest fix is the reviewer's own fallback option: keep error (the tests check it, and it keeps the notice visible at level: error) and correct the comment.

Pushed c1e837b.

[peco-review-bot]

Keeping level: error is fine with me. The head I checked hasn't caught up with the change described in c1e837b, though. Lines 62–63 still say the message is "explicitly marked non-fatal so log-based alerting / readers don't mistake this config fallback for a driver failure". That's the claim my finding was about: alerting filters on the level field, so the wording won't stop it from firing. Something like "the text marks it non-fatal for human readers; level-based alerting will still see an error" would resolve this. If the commit didn't land on this branch, please push it again. Otherwise this needs a manual edit to the comment.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

The corrected comment is already on this branch and I didn't make any new edit. The working tree is clean and ai/issue-494 matches origin/ai/issue-494. In lib/DBSQLLogger.ts (around lines 60–65 of the DBSQLLogger constructor) the comment now reads: "Trade-off: consumers filtering on the winston level field (e.g. error-level alerting) WILL see this as an error; the "[warn] ... non-fatal" wording only helps human readers tell this config fallback apart from a driver failure." The old claim that the wording stops alerting from firing is gone. The review most likely ran against a head from before c1e837b landed. We agree on the outcome (keep level: error, fix the comment), so this thread has converged. A human only needs to confirm the current head shows the updated comment and resolve the thread.

[peco-review-bot]

Verdict: 2 Low

Looks good, with two low-severity design notes. The directory resolution and writability check are correct, the 3.0.0 trailing-separator behaviour is kept, and the unit tests cover the new branches well.

Nits:

• The default file name databricks-sql-nodejs.log and the CHANGELOG entry are still open questions for maintainers.
• path.join(path.dirname(p), path.basename(p)) is a roundabout way to strip a trailing separator. A short helper name or comment would make it clearer.

[peco-review-bot]

🔵 Low — The fallback notice goes out at error level with a [warn] prefix. The comment explains why: so that level: error can't filter it out. The cost is that anyone alerting on level === 'error' will now get a driver "error" for a config problem that doesn't affect the driver.

Two alternatives:

• Log it at warn, accepting that level: error users won't see it.
• Write it directly to process.stderr / console.warn, which skips winston's level filter without labelling it an error.

It's a judgement call, but this is a public connector, so a maintainer should decide it explicitly rather than leave it to the code comment.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

I agree with the reviewer that this is a policy decision for a maintainer, so I made no code change. Right now (lib/DBSQLLogger.ts around line 70), when the log file can't be written, the driver logs one notice through winston at error level with a [warn] ... non-fatal prefix. The goal is that level: error users still see it. The cost is that anyone alerting on level === 'error' will get an alert for a config fallback that doesn't affect the driver. There are three options, and each is a small one-line change I can make once someone picks: (a) keep error as it is now, (b) log at warn and accept that level: error users won't see it, or (c) write it straight to process.stderr / console.warn, which skips winston's level filter and transports entirely, so it won't reach custom transports or structured log pipelines. This is a public connector, so a maintainer needs to choose which behaviour users get.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low test-robustness note. Directory resolution, keeping the 3.0.0 trailing-separator behaviour, and the up-front writability check with a console-only fallback are all correct and covered by unit tests. Remaining nit: the PR description says the logger "logs a warning", but the code logs at error level (deliberately, and documented in the code comment).

[peco-review-bot]

✅ No issues identified by the review bot.