refactor(app): standardize log file path resolution#84
Conversation
- Introduce LoadedConfig startup flow that preserves config source path - Centralize logging.file path resolution policy in util config helpers - Apply log level, timestamp, and file sink consistently before app subsystem creation
📝 WalkthroughWalkthroughThis PR introduces file-based logging configuration for the application. It tracks the loaded configuration's source path, adds path resolution logic for logging file destinations (empty=console, absolute=as-is, relative=against config directory), dynamically creates a file sink during startup if configured, and includes comprehensive design documentation and tests for the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Startup
participant Config as Config Loading
participant PathRes as Path Resolution
participant Logging as Logging System
participant FS as File System
App->>Config: load_config_for_cli()
Config->>Config: Parse TOML/return defaults
Config-->>App: LoadedConfig{config, source_path}
App->>PathRes: apply_log_file(config, source_path)
PathRes->>PathRes: resolve_logging_file_path(logging.file, source_path)
alt Empty logging.file
PathRes-->>Logging: (empty path - skip)
else Absolute Path
PathRes->>Logging: set_log_file_path(absolute)
else Relative Path
PathRes->>PathRes: Resolve against source_path.parent()
PathRes->>Logging: set_log_file_path(resolved)
end
Logging->>FS: Create/open log file
alt File creation succeeds
Logging->>Logging: Attach file sink + update patterns
Logging-->>App: Success
else File creation fails
Logging->>Logging: Log warning, keep console sink
Logging-->>App: Error (non-fatal, continue startup)
end
App->>App: Continue subsystem initialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@config/goggles.template.toml`:
- Around line 58-62: The comment alignment for the example "logs/goggles.log"->
is missing a space before the arrow; edit the comment block containing the file
logging target examples so the third line matches the others by adding at least
one space between the closing quote and the arrow (i.e., change
"logs/goggles.log"-> to "logs/goggles.log" ->) so the 'file' key comment is
consistently formatted.
In `@tests/util/test_logging.cpp`:
- Around line 162-186: The test leaves the file sink added by set_log_file_path
registered on the global logger (created via initialize_logger / accessible via
get_logger()), causing a dangling open file after remove_all(temp_dir); fix by
removing that file sink at the end of the test before deleting the temp dir:
flush the logger (get_logger()->flush()), locate the file sink instance
previously returned by set_log_file_path (or inspect get_logger()->sinks()),
remove/unregister that sink from the logger and/or call the appropriate
drop/erase routine so the file is closed, then proceed to
std::filesystem::remove_all(temp_dir).
🧹 Nitpick comments (3)
tests/util/test_config.cpp (1)
236-261: Solid CWD-independence test with proper RAII guard.The
CurrentPathGuardpattern ensures CWD is restored even when assertions fail. Two minor observations:
- Temp directories (
goggles_config_cwd_a/b) use fixed names — parallel test runs could collide. Consider a unique suffix if parallelism is ever enabled.- If a
REQUIREfails before lines 259–260, the temp dirs won't be cleaned up (though they're in the system temp dir, so the impact is negligible).Neither is blocking.
openspec/changes/update-file-logging-path-policy/specs/app-window/spec.md (1)
11-16: Scenario step format deviates from coding guidelines.The coding guidelines prescribe
- **WHEN** [condition] - **THEN** [expected result]for scenario steps. The current format uses separate- **GIVEN**,- **WHEN**,- **THEN**,- **AND**lines. This pattern repeats across all scenarios in the file (lines 12–15, 18–21, 24–27, 34–37, 40–43, 46–48).For example, the first scenario would become:
Suggested format per guidelines
#### Scenario: Empty logging.file keeps console-only behavior -- **GIVEN** `[logging].file = ""` -- **WHEN** the application starts and applies logging configuration -- **THEN** no file sink SHALL be attached -- **AND** console logging SHALL remain active +- **WHEN** `[logging].file = ""` and the application starts and applies logging configuration - **THEN** no file sink SHALL be attached and console logging SHALL remain activeAs per coding guidelines, use format
- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements.src/util/logging.cpp (1)
109-115: File sink opens in truncate mode — is this intentional?
basic_file_sink_mt(path.string(), true)passestruefor thetruncateparameter, so the log file is wiped on every application launch. If the intent is to preserve logs across runs (e.g., for post-mortem debugging), append mode (false) would be more appropriate. If truncate is deliberate, a brief comment here would clarify the choice for future readers.If append is preferred:
- new_sink = std::make_shared<spdlog::sinks::basic_file_sink_mt>(path.string(), true); + new_sink = std::make_shared<spdlog::sinks::basic_file_sink_mt>(path.string(), /*truncate=*/false);
| # File logging target: | ||
| # "" -> console only | ||
| # "/abs/path.log" -> use absolute path as-is | ||
| # "logs/goggles.log"-> resolved relative to the loaded config file directory | ||
| file = "" |
There was a problem hiding this comment.
Missing space before -> on line 61.
The alignment arrow on line 61 is flush against the closing quote, unlike lines 59–60 which have padding. Add at least one space for readability.
Suggested fix
# File logging target:
# "" -> console only
# "/abs/path.log" -> use absolute path as-is
-# "logs/goggles.log"-> resolved relative to the loaded config file directory
+# "logs/goggles.log" -> resolved relative to the loaded config file directory📝 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.
| # File logging target: | |
| # "" -> console only | |
| # "/abs/path.log" -> use absolute path as-is | |
| # "logs/goggles.log"-> resolved relative to the loaded config file directory | |
| file = "" | |
| # File logging target: | |
| # "" -> console only | |
| # "/abs/path.log" -> use absolute path as-is | |
| # "logs/goggles.log" -> resolved relative to the loaded config file directory | |
| file = "" |
🤖 Prompt for AI Agents
In `@config/goggles.template.toml` around lines 58 - 62, The comment alignment for
the example "logs/goggles.log"-> is missing a space before the arrow; edit the
comment block containing the file logging target examples so the third line
matches the others by adding at least one space between the closing quote and
the arrow (i.e., change "logs/goggles.log"-> to "logs/goggles.log" ->) so the
'file' key comment is consistently formatted.
| TEST_CASE("set_log_file_path writes logs to file sink", "[logging]") { | ||
| initialize_logger("file_sink_test"); | ||
| set_log_level(spdlog::level::info); | ||
|
|
||
| const auto temp_dir = | ||
| std::filesystem::temp_directory_path() / "goggles_logging_test" / "file_sink"; | ||
| std::filesystem::remove_all(temp_dir); | ||
| std::filesystem::create_directories(temp_dir); | ||
| const auto log_path = temp_dir / "goggles.log"; | ||
|
|
||
| auto sink_result = set_log_file_path(log_path); | ||
| REQUIRE(sink_result.has_value()); | ||
|
|
||
| constexpr const char* FILE_TEST_MESSAGE = "file sink integration marker"; | ||
| GOGGLES_LOG_INFO("{}", FILE_TEST_MESSAGE); | ||
| get_logger()->flush(); | ||
|
|
||
| std::ifstream file(log_path); | ||
| REQUIRE(file.is_open()); | ||
|
|
||
| std::string content((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>()); | ||
| REQUIRE(content.find(FILE_TEST_MESSAGE) != std::string::npos); | ||
|
|
||
| std::filesystem::remove_all(temp_dir); | ||
| } |
There was a problem hiding this comment.
Good integration test for the file-sink happy path.
One concern: after remove_all(temp_dir) on line 185, the file sink attached on line 172 is still registered in the global logger. On Linux the open file descriptor keeps working, but on other platforms (or if the OS reclaims the inode) later test cases that trigger logging may hit unexpected I/O errors. Consider removing the file sink at the end of the test to leave the logger in a clean state:
Suggested cleanup
REQUIRE(content.find(FILE_TEST_MESSAGE) != std::string::npos);
+ // Remove file sink before deleting the directory
+ set_log_file_path({});
std::filesystem::remove_all(temp_dir);
}📝 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.
| TEST_CASE("set_log_file_path writes logs to file sink", "[logging]") { | |
| initialize_logger("file_sink_test"); | |
| set_log_level(spdlog::level::info); | |
| const auto temp_dir = | |
| std::filesystem::temp_directory_path() / "goggles_logging_test" / "file_sink"; | |
| std::filesystem::remove_all(temp_dir); | |
| std::filesystem::create_directories(temp_dir); | |
| const auto log_path = temp_dir / "goggles.log"; | |
| auto sink_result = set_log_file_path(log_path); | |
| REQUIRE(sink_result.has_value()); | |
| constexpr const char* FILE_TEST_MESSAGE = "file sink integration marker"; | |
| GOGGLES_LOG_INFO("{}", FILE_TEST_MESSAGE); | |
| get_logger()->flush(); | |
| std::ifstream file(log_path); | |
| REQUIRE(file.is_open()); | |
| std::string content((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>()); | |
| REQUIRE(content.find(FILE_TEST_MESSAGE) != std::string::npos); | |
| std::filesystem::remove_all(temp_dir); | |
| } | |
| TEST_CASE("set_log_file_path writes logs to file sink", "[logging]") { | |
| initialize_logger("file_sink_test"); | |
| set_log_level(spdlog::level::info); | |
| const auto temp_dir = | |
| std::filesystem::temp_directory_path() / "goggles_logging_test" / "file_sink"; | |
| std::filesystem::remove_all(temp_dir); | |
| std::filesystem::create_directories(temp_dir); | |
| const auto log_path = temp_dir / "goggles.log"; | |
| auto sink_result = set_log_file_path(log_path); | |
| REQUIRE(sink_result.has_value()); | |
| constexpr const char* FILE_TEST_MESSAGE = "file sink integration marker"; | |
| GOGGLES_LOG_INFO("{}", FILE_TEST_MESSAGE); | |
| get_logger()->flush(); | |
| std::ifstream file(log_path); | |
| REQUIRE(file.is_open()); | |
| std::string content((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>()); | |
| REQUIRE(content.find(FILE_TEST_MESSAGE) != std::string::npos); | |
| // Remove file sink before deleting the directory | |
| set_log_file_path({}); | |
| std::filesystem::remove_all(temp_dir); | |
| } |
🤖 Prompt for AI Agents
In `@tests/util/test_logging.cpp` around lines 162 - 186, The test leaves the file
sink added by set_log_file_path registered on the global logger (created via
initialize_logger / accessible via get_logger()), causing a dangling open file
after remove_all(temp_dir); fix by removing that file sink at the end of the
test before deleting the temp dir: flush the logger (get_logger()->flush()),
locate the file sink instance previously returned by set_log_file_path (or
inspect get_logger()->sinks()), remove/unregister that sink from the logger
and/or call the appropriate drop/erase routine so the file is closed, then
proceed to std::filesystem::remove_all(temp_dir).
1 participant
User description
PR Type
Enhancement
Description
Implement file logging sink with config-driven path resolution
Track config source path to resolve relative logging file paths
Apply log level, timestamp, and file sink before app subsystem creation
Add comprehensive tests and documentation for file logging behavior
Diagram Walkthrough
File Walkthrough
5 files
Add LoadedConfig struct and file sink setup flowAdd resolve_logging_file_path function declarationImplement logging file path resolution logicAdd set_log_file_path API for file sink configurationImplement file sink management with pattern updates2 files
Add tests for logging file path resolutionAdd file sink write and error handling tests6 files
Document logging.file path resolution semanticsClarify app logger scope and file logging policyAdd change proposal for file logging featureAdd design rationale and alternatives analysisAdd formal requirements for file logging behaviorAdd implementation task checklist and verificationSummary by CodeRabbit
New Features
logging.filesetting in the configuration.Documentation