Override the default log file path by environment variable MAGIC_CONTEXT_LOG_PATH#183
Override the default log file path by environment variable MAGIC_CONTEXT_LOG_PATH#183kecsap wants to merge 1 commit into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
alfonso-magic-context
left a comment
alfonso-magic-context
left a comment
There was a problem hiding this comment.
Thanks for this, the MAGIC_CONTEXT_LOG_PATH override is a good idea and the plugin-side change is clean (single choke point in getMagicContextLogPath, trims and ignores blank, with tests). A couple of changes needed before merge:
-
Please rebase onto
master. The branch currently conflicts. We landed the config-location cutover (config + artifacts moved to the shared CortexKit location) since this PR was opened, which touches nearby docs/paths. -
The dashboard doc change overstates support. The PR updates
packages/dashboard/README.mdand the docsdashboard.mdto say the dashboard Logs tab honorsMAGIC_CONTEXT_LOG_PATH, but the dashboard's Rust log resolver (packages/dashboard/src-tauri/src/log_parser.rs,resolve_log_path_for) readsstd::env::temp_dir()and does not read that env var. As written, a user who setsMAGIC_CONTEXT_LOG_PATHwould have the plugin write to the custom path while the dashboard keeps reading the default tmp path, so the docs would be wrong.Two ways to resolve, either is fine:
- Simplest: drop the dashboard doc edits from this PR and keep it plugin-only (the env override still works for the plugin's own log file).
- Complete: also make
resolve_log_path_forhonorMAGIC_CONTEXT_LOG_PATH(read the env var first, fall back to the tmp path) so the dashboard Logs tab reads the same file the plugin writes. If you'd rather not touch Rust, say so and I'll add that part.
Everything else looks good. Once it's rebased and the dashboard claim is either dropped or backed by the Rust read, I'll merge.
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{resolve_log_path_for, Harness}; | ||
| use std::path::PathBuf; | ||
|
|
||
| #[test] | ||
| fn resolve_log_path_for_uses_harness_fallback_when_env_unset() { | ||
| std::env::remove_var("MAGIC_CONTEXT_LOG_PATH"); | ||
|
|
||
| assert_eq!( | ||
| resolve_log_path_for(Harness::Opencode), | ||
| std::env::temp_dir() | ||
| .join("opencode") | ||
| .join("magic-context") | ||
| .join("magic-context.log") | ||
| ); | ||
| assert_eq!( | ||
| resolve_log_path_for(Harness::Pi), | ||
| std::env::temp_dir() | ||
| .join("pi") | ||
| .join("magic-context") | ||
| .join("magic-context.log") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_log_path_for_honors_magic_context_log_path_override() { | ||
| let custom = std::env::temp_dir() | ||
| .join("custom") | ||
| .join("magic-context.log"); | ||
| std::env::set_var( | ||
| "MAGIC_CONTEXT_LOG_PATH", | ||
| custom.to_string_lossy().to_string(), | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| resolve_log_path_for(Harness::Opencode), | ||
| PathBuf::from(&custom) | ||
| ); | ||
| assert_eq!(resolve_log_path_for(Harness::Pi), PathBuf::from(&custom)); | ||
|
|
||
| std::env::remove_var("MAGIC_CONTEXT_LOG_PATH"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_log_path_for_ignores_blank_magic_context_log_path() { | ||
| std::env::set_var("MAGIC_CONTEXT_LOG_PATH", " "); | ||
|
|
||
| assert_eq!( | ||
| resolve_log_path_for(Harness::Pi), | ||
| std::env::temp_dir() | ||
| .join("pi") | ||
| .join("magic-context") | ||
| .join("magic-context.log") | ||
| ); | ||
|
|
||
| std::env::remove_var("MAGIC_CONTEXT_LOG_PATH"); | ||
| } | ||
| } |
There was a problem hiding this comment.
All three new tests manipulate MAGIC_CONTEXT_LOG_PATH via std::env::set_var/remove_var, which is global, unsynchronised process state. Cargo runs tests in parallel threads by default, so resolve_log_path_for_uses_harness_fallback_when_env_unset can read the env var mid-assignment from either of the other two tests, producing intermittent failures.
The standard fix is either to gate each test with a Mutex around the env-mutation window, add the serial_test crate and annotate each test with #[serial], or run with RUST_TEST_THREADS=1 in CI. Without serialization, these tests will produce flaky results on any machine with more than one CPU core.
2 participants
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Allow overriding the Magic Context log file path via
MAGIC_CONTEXT_LOG_PATH. Both the plugin and dashboard honor the env var; blank values are ignored. If unset, logs fall back to${TMPDIR}/<harness>/magic-context/magic-context.log(e.g.,opencodeorpi).packages/plugin:getMagicContextLogPathreadsMAGIC_CONTEXT_LOG_PATH(trimmed), with fallback to the harness temp dir.packages/dashboard:resolve_log_path_forin Rust honorsMAGIC_CONTEXT_LOG_PATH(trimmed), with tests for override, blank values, and fallback.README.md, dashboard README, and dashboard reference to document the env var and fallback path.Written for commit 47a8ef8. Summary will update on new commits.
Greptile Summary
This PR adds a
MAGIC_CONTEXT_LOG_PATHenvironment variable that overrides the default per-harness log file path in both the TypeScript plugin and the Rust dashboard. When the variable is unset or blank, behaviour is unchanged — the log resolves to${TMPDIR}/<harness>/magic-context/magic-context.log.data-path.ts):getMagicContextLogPathtrims and checks the env var before falling back to the harness-based path; tests and env-state teardown are handled correctly.log_parser.rs):resolve_log_path_formirrors the TypeScript logic; three new tests cover override, fallback, and blank-value cases, but all three mutate global env state (std::env::set_var/remove_var) without synchronisation, making them racy under Cargo's default parallel test runner.Confidence Score: 4/5
Safe to merge for production use; the Rust tests are racy and may produce intermittent CI failures.
The production path change is small and correct in both TypeScript and Rust. The only real concern is in the new Rust tests: all three tests modify MAGIC_CONTEXT_LOG_PATH via std::env::set_var/remove_var without any mutex or serial guard, and Cargo runs tests in parallel threads by default. This means the test that expects the env var to be absent can observe it being set by a concurrent test, causing intermittent failures on multi-core machines.
packages/dashboard/src-tauri/src/log_parser.rs — the new test block needs thread-safe env-var handling before it is reliable in CI.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Log path requested] --> B{MAGIC_CONTEXT_LOG_PATH set and non-blank?} B -- Yes --> C[Return env var value] B -- No --> D{Which harness?} D -- opencode --> E["${TMPDIR}/opencode/magic-context/magic-context.log"] D -- pi --> F["${TMPDIR}/pi/magic-context/magic-context.log"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[Log path requested] --> B{MAGIC_CONTEXT_LOG_PATH set and non-blank?} B -- Yes --> C[Return env var value] B -- No --> D{Which harness?} D -- opencode --> E["${TMPDIR}/opencode/magic-context/magic-context.log"] D -- pi --> F["${TMPDIR}/pi/magic-context/magic-context.log"]Comments Outside Diff (1)
packages/plugin/src/shared/data-path.ts, line 39-46 (link)MAGIC_CONTEXT_LOG_PATHis set. Both harnesses will resolve to the same file, interleaving their session traces. The comment should be updated to reflect the override, and users should be warned about this behaviour.Reviews (2): Last reviewed commit: "Override the default log file path by en..." | Re-trigger Greptile