Skip to content

Conversation

egorpavlikhin
Copy link
Contributor

Change the location of the log file to be where the entry point file is located, instead of the current node active directory.

  • Could this create an issue if the node process doesn't have write access to the folder? Seems unlikely and it should be okay to just fail in such cases.
  • What happens if multiple processes try to use the MCP at the same time? Would that create a write conflict? Would not be a great experience but there is a workaround in that the log file path can be specified manually.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR changes the default log file location from the current working directory to the directory containing the entry point file. This ensures logs are consistently placed relative to the MCP server executable rather than the arbitrary working directory of the calling process.

  • Updates logger configuration to use entry directory for log file placement
  • Adds a new setEntryDirectory function to configure the logger with the entry point location
  • Updates documentation to reflect the new default log file name

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/utils/logger.ts Adds entry directory configuration and updates log file path resolution logic
src/index.ts Sets the entry directory using ESM __dirname equivalent and updates CLI help text
README.md Documents logging command line options and adds missing "type" field to config example

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 45 to 46
if (!filePath.includes('/') && !filePath.includes('\\') && config.entryDirectory) {
config.logFilePath = path.join(config.entryDirectory, filePath);
Copy link
Preview

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic falls back to the original behavior when config.entryDirectory is undefined, but this could result in using process.cwd() implicitly through the else clause. Consider explicitly handling the case when entryDirectory is not set to maintain predictable behavior.

Suggested change
if (!filePath.includes('/') && !filePath.includes('\\') && config.entryDirectory) {
config.logFilePath = path.join(config.entryDirectory, filePath);
if (!filePath.includes('/') && !filePath.includes('\\')) {
if (config.entryDirectory) {
config.logFilePath = path.join(config.entryDirectory, filePath);
} else {
// Explicitly use process.cwd() if entryDirectory is not set
config.logFilePath = path.join(process.cwd(), filePath);
// Optionally, log a warning (if you have a logger, otherwise use console.warn)
// console.warn(`[Logger] entryDirectory not set; using current working directory for log file: ${config.logFilePath}`);
}

Copilot uses AI. Check for mistakes.

Comment on lines 33 to 36
// Update log file path if it's still the default
if (config.logFilePath === DefaultLogFileName) {
config.logFilePath = path.join(directory, DefaultLogFileName);
}
Copy link
Preview

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison will fail if setLogFilePath was called before setEntryDirectory with the default filename, because setLogFilePath would have already modified the path. Consider tracking whether the path has been explicitly set by the user separately from the actual path value.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@andrewabest andrewabest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and simple, I like it.

Co-authored-by: Andrew Best <andrew.best@octopus.com>
@egorpavlikhin egorpavlikhin merged commit 206b6ca into main Sep 30, 2025
1 check passed
@egorpavlikhin egorpavlikhin deleted the egorp/log-file branch September 30, 2025 05:00
andrewabest pushed a commit that referenced this pull request Oct 1, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.3.0](v0.2.5...v0.3.0)
(2025-10-01)


### Features

* Project variables tool
([#41](#41))
([4c7d1f2](4c7d1f2))


### Bug Fixes

* Change log file path to match entry point instead of node folder
([#44](#44))
([206b6ca](206b6ca))
* Fix SBOM build
([#42](#42))
([9500aa4](9500aa4))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants