Allow config overrides for logging and silence warn toasts

[riatzukiza]

Summary

• allow ~/.opencode/openhax-codex-config.json to override logging/env behavior including log rotation and warn toasts
• default warning toasts to off while preserving error toasts; add tests to cover new config priority
• document env vars (including new CODEX_SHOW_WARNING_TOASTS) and pass logger tests

[coderabbitai]

Summary by CodeRabbit

Release Notes

• New Features

  • Added plugin configuration options for logging behavior, including warning toast visibility, request logging, and log rotation settings.
  • Logging configuration now supports per-instance customization without affecting defaults.

• Improvements

  • Enhanced configuration file handling with safer default management when files are missing or invalid.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Walkthrough

Adds a nested logging section to plugin config and merges it with defaults, converts static logging flags into mutable state with runtime overrides, replaces exported LOGGING_ENABLED with isLoggingEnabled(), and updates tests and call sites to use the new API and behavior.

Changes

Cohort / File(s)	Summary
Type Definitions lib/types.ts	Adds LoggingConfig interface and extends PluginConfig with optional logging fields (enableRequestLogging, debug, showWarningToasts, logMaxBytes, logMaxFiles, logQueueMax)
Configuration Loading lib/config.ts	Shallow-copies DEFAULT_CONFIG on missing/invalid file or errors; merges DEFAULT_CONFIG.logging with userConfig.logging so user logging settings override defaults without mutating the default object
Logger Implementation lib/logger.ts	Replaces static exports with mutable runtime state; introduces envLoggingDefaults, refreshLoggingState, ensurePositiveNumber, applyLoggingOverrides; exposes isLoggingEnabled() and accepts pluginConfig?: PluginConfig via LoggerOptions; configureLogger() applies plugin logging overrides
Request Handler Update lib/request/response-handler.ts	Replaces import/use of LOGGING_ENABLED with isLoggingEnabled() for logging condition checks
Tests & Mocks — Logger test/logger.test.ts, test/cache-warming.test.ts, test/README.md	Update mocks and tests to import/call isLoggingEnabled() instead of using LOGGING_ENABLED; add assertions for pluginConfig.logging overrides (e.g., disabling request logging, controlling warning toasts)
Tests — Plugin Config test/plugin-config.test.ts	Adds/updates tests validating nested logging config merging, default propagation, and that user logging fields override defaults

Sequence Diagram(s)

sequenceDiagram
    participant Init as Plugin Init
    participant Config as Config Loader
    participant Logger as Logger
    participant State as Logging State

    Init->>Config: loadPluginConfig()
    Note right of Config: Read user config file\nShallow-copy DEFAULT_CONFIG\nMerge DEFAULT_CONFIG.logging with userConfig.logging
    Config-->>Init: PluginConfig { logging: { ... } }

    Init->>Logger: configureLogger({ pluginConfig })
    Logger->>Logger: applyLoggingOverrides(pluginConfig.logging)
    Logger->>State: update mutable flags (debug, enableRequestLogging,\nshowWarningToasts, rotation, queue)
    Logger->>Logger: refreshLoggingState()
    Logger-->>Init: configured

    Init->>Logger: handleRequest()/emitWarning()
    alt isLoggingEnabled()
        Logger->>Logger: log to file/queue
    end
    alt warnToastEnabled
        Logger->>Init: show toast
    else
        Logger->>Logger: forward to app log or console
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Focus review on:
  • lib/logger.ts: initialization ordering for env defaults vs. applied overrides, correctness of ensurePositiveNumber and refreshLoggingState, and public API change (LOGGING_ENABLED → isLoggingEnabled()).
  • lib/config.ts: confirm shallow-copy behavior in all branches and that merging doesn't mutate DEFAULT_CONFIG.
  • Tests/mocks: ensure mocks export callable isLoggingEnabled() and updated assertions reflect new merging semantics.

Possibly related PRs

• feat: add gpt-5.1-codex-max support with xhigh reasoning and persistent logging #27 — modifies lib/logger.ts; likely overlaps with state/flag refactors and configureLogger changes.
• Release #29 — touches logger/config surfaces and plugin-based overrides; strongly related to added LoggingConfig.
• Guard disk logging and clarify clone/role utilities #33 — changes logger gating and disk write conditions; may conflict with enableRequestLogging behavior and guards.

Poem

Constants loosen, configs embrace,
Flags turn fluid, state finds its place.
Defaults copied, user fields win,
Toasts and logs now dance within. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main changes: allowing config file overrides for logging behavior and disabling warning toasts by default.
Description check	✅ Passed	The description directly relates to the changeset, covering config file overrides for logging, warning toast defaults, tests, and environment variable documentation.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch logging-config-overrides

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5aa5dcd and e0f2509.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json and included by none

📒 Files selected for processing (1)

• test/logger.test.ts (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

test/logger.test.ts (1)

lib/logger.ts (5)

• isLoggingEnabled (22-24)
• configureLogger (106-128)
• logRequest (130-147)
• flushRollingLogsForTest (165-170)
• logWarn (157-159)

🔇 Additional comments (3)

test/logger.test.ts (3)

155-174: Default warning toast behavior correctly validated.

The test confirms that warning toasts are off by default, even when the tui client is available. The assertions properly verify that showToast is not called while console warnings and app logs still occur.

---

176-201: Opt-in warning toast behavior properly tested.

The test correctly validates that when showWarningToasts: true is set via config, warning toasts are displayed and console/app log output is suppressed. The toast payload assertions ensure the correct structure is passed to showToast.

---

214-214: Config update aligns with new API.

The addition of pluginConfig: { logging: { showWarningToasts: true } } correctly enables warning toasts for the long message wrapping test.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80c9101 and f5871c4.

⛔ Files ignored due to path filters (2)

• index.ts is excluded by none and included by none
• spec/environment-variables.md is excluded by none and included by none

📒 Files selected for processing (5)

• lib/config.ts (2 hunks)
• lib/logger.ts (6 hunks)
• lib/types.ts (1 hunks)
• test/logger.test.ts (4 hunks)
• test/plugin-config.test.ts (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (4)

lib/config.ts (2)

lib/types.ts (1)

• PluginConfig (6-40)

lib/logger.ts (1)

• logWarn (154-156)

test/logger.test.ts (1)

lib/logger.ts (4)

• configureLogger (103-125)
• logRequest (127-144)
• flushRollingLogsForTest (162-167)
• logWarn (154-156)

lib/logger.ts (2)

lib/utils/file-system-utils.ts (1)

• getOpenCodePath (22-24)

lib/types.ts (2)

• PluginConfig (6-40)
• LoggingConfig (42-55)

test/plugin-config.test.ts (1)

lib/config.ts (1)

• loadPluginConfig (30-70)

🔇 Additional comments (18)

lib/logger.ts (8)

4-4: LGTM: Type imports align with new configuration surface.

The imports for LoggingConfig and PluginConfig support the new pluginConfig-based override mechanism.

---

12-19: Good consolidation of environment-based defaults.

Centralizing environment variable parsing into envLoggingDefaults improves maintainability and makes the configuration source clearer.

---

61-71: LGTM: Helper functions implement correct validation and state refresh logic.

• refreshLoggingState() properly recomputes derived flags from base configuration
• ensurePositiveNumber() provides robust validation with appropriate fallback handling

---

73-86: LGTM: Override logic correctly handles partial configuration.

The nullish coalescing operator (??) ensures that undefined config values preserve existing settings, while explicit false values are applied as intended. Numeric validation through ensurePositiveNumber prevents invalid configuration.

---

32-36: LGTM: LoggerOptions extension maintains backward compatibility.

Adding optional pluginConfig parameter allows configuration-driven overrides without breaking existing usage.

---

103-110: LGTM: configureLogger correctly applies plugin-based overrides.

The call to applyLoggingOverrides(options.pluginConfig?.logging) safely handles undefined values and applies configuration before any logging occurs.

---

169-208: LGTM: Warning toast behavior correctly implements opt-in configuration.

The updated logic ensures warning toasts are only displayed when explicitly enabled via configuration (WARN_TOASTS_ENABLED), while error toasts remain always enabled. This prevents noisy warnings while preserving critical error notifications.

The three-way routing (app log, toast, console) for warning-level messages is correct:

• When toasts disabled: warnings → app log + console
• When toasts enabled: warnings → toast only (no app log, no console)

---

88-88: LGTM: Initial state refresh ensures derived flags are computed.

Calling refreshLoggingState() at module initialization correctly establishes DEBUG_ENABLED and CONSOLE_LOGGING_ENABLED from the environment-based defaults.

lib/types.ts (1)

36-55: LGTM: Type definitions correctly model optional configuration overrides.

The LoggingConfig interface with all optional fields properly supports partial overrides, and the JSDoc comments clearly document the purpose of each field.

lib/config.ts (3)

17-19: LGTM: Default logging configuration establishes sensible baseline.

Setting showWarningToasts: false by default prevents noisy notifications while allowing explicit opt-in via configuration.

---

41-61: LGTM: Nested merge logic correctly preserves defaults and applies overrides.

The configuration loading properly:

• Shallow-copies DEFAULT_CONFIG to prevent mutation (line 45)
• Extracts user logging settings with safe fallback (line 50)
• Creates a new logging object merging defaults with user values (lines 56-59), ensuring user settings take precedence

This approach correctly handles partial overrides (e.g., only debug: true while preserving showWarningToasts: false default).

---

62-69: LGTM: Error handling preserves defaults safely.

Shallow-copying DEFAULT_CONFIG on errors prevents DEFAULT_CONFIG mutation and ensures the cached config remains independent.

test/plugin-config.test.ts (2)

47-63: LGTM: Test correctly validates default configuration shape.

The test properly verifies that missing configuration files result in the default configuration including logging: { showWarningToasts: false }.

---

95-108: LGTM: Nested merge test validates partial override behavior.

This test correctly verifies that user-provided logging fields merge with defaults, ensuring partial configuration works as expected. Good coverage of the nested merge logic.

test/logger.test.ts (4)

110-122: LGTM: Test correctly validates configuration override priority.

This test properly verifies that pluginConfig.logging.enableRequestLogging: false overrides the environment variable, ensuring no disk writes occur when explicitly disabled via configuration.

---

146-165: LGTM: Test validates default warning toast behavior is disabled.

The test correctly verifies that warning toasts are not displayed by default (even with TUI support), and that warnings are instead routed to app.log and console. This aligns with the PR objective to reduce noise.

---

167-192: LGTM: Test validates opt-in warning toast configuration.

This test properly confirms that warning toasts are displayed only when explicitly enabled via pluginConfig.logging.showWarningToasts: true, and that duplicate logging (to app.log and console) is suppressed when toasts are enabled.

---

194-219: LGTM: Long message wrapping test updated for opt-in behavior.

The test correctly includes showWarningToasts: true in the configuration to enable toast display before testing message wrapping functionality.

[riatzukiza]

/oc In lib/logger.ts around line 21, exported mutable variable LOGGING_ENABLED is
unsafe; change the module to keep the boolean as an internal (non-exported)
variable and replace the exported let with an exported function
isLoggingEnabled(): boolean that returns the internal value; update any
configureLogger or override functions to mutate the internal variable instead of
reassigning an exported binding; after this change update all external callers
to import and call isLoggingEnabled() rather than reading LOGGING_ENABLED
directly.

[opencode-agent]

Excellent! All tests are passing. Let me create a summary of the changes made:

Summary

I have successfully implemented the requested changes to fix the unsafe exported mutable variable LOGGING_ENABLED in the logger module. Here's what was changed:

Changes Made:

1. lib/logger.ts (line 21-23):

   • Changed export let LOGGING_ENABLED to private let LOGGING_ENABLED
   • Added exported function isLoggingEnabled(): boolean that returns the internal value

2. lib/request/response-handler.ts:

   • Updated import to use isLoggingEnabled instead of LOGGING_ENABLED
   • Updated the conditional check from LOGGING_ENABLED to isLoggingEnabled()

3. test/logger.test.ts:

   • Updated test to use isLoggingEnabled() function instead of accessing LOGGING_ENABLED directly
   • Renamed test from "LOGGING_ENABLED reflects env state" to "isLoggingEnabled reflects env state"

4. test/cache-warming.test.ts:

   • Updated mock configuration to use isLoggingEnabled: () => false instead of LOGGING_ENABLED: false

5. test/README.md:

   • Updated documentation reference from "LOGGING_ENABLED constant" to "isLoggingEnabled() function"

Benefits:

• Encapsulation: The logging state is now properly encapsulated within the logger module
• Safety: External code can no longer directly reassign the logging state, preventing inconsistent state
• API Stability: The new getter function provides a stable interface that won't be affected by internal implementation changes
• Maintainability: All mutations to the logging state now go through the controlled applyLoggingOverrides() function

Verification:

• All 410 tests pass successfully
• TypeScript compilation completes without errors
• The existing functionality remains unchanged - only the API surface is made safer

The change maintains backward compatibility for functionality while improving the safety and encapsulation of the logging system.

opencode session  |  github run

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

test/README.md (1)

78-85: Logger tests description now matches API; consider mentioning new config behavior

Switching the bullet to isLoggingEnabled() correctly reflects the new logger surface. As a follow‑up, you might also call out that logger tests now cover config‑driven logging overrides and warning‑toast behavior so this section stays in sync with future changes.

test/logger.test.ts (2)

38-45: Reset CODEX_SHOW_WARNING_TOASTS in beforeEach to avoid flaky toast tests

The new warning‑toast tests assume CODEX_SHOW_WARNING_TOASTS is unset/false, but beforeEach only deletes the older logging env vars. If a developer or CI environment sets CODEX_SHOW_WARNING_TOASTS=1, tests around “no toast by default” will fail spuriously.

Consider adding this to the setup:

 beforeEach(() => {
 	vi.resetModules();
 	Object.assign(process.env, originalEnv);
 	delete process.env.ENABLE_PLUGIN_REQUEST_LOGGING;
 	delete process.env.DEBUG_CODEX_PLUGIN;
 	delete process.env.CODEX_LOG_MAX_BYTES;
 	delete process.env.CODEX_LOG_MAX_FILES;
 	delete process.env.CODEX_LOG_QUEUE_MAX;
+	delete process.env.CODEX_SHOW_WARNING_TOASTS;

Also applies to: 146-192

---

146-165: Warning‑toast tests accurately codify the new behavior; confirm you’re okay with replacing app/console logs

The toast‑focused tests correctly exercise the intended behavior:

• By default, logWarn routes through app.log and console, with no toast even when TUI is present.
• When pluginConfig.logging.showWarningToasts is true, warnings go to toasts only (no app.log and no console) and long messages are wrapped.

This is a deliberate trade‑off: enabling warning toasts means you no longer see those warnings in app.log or on the console unless request/debug logging is also on. If you ever want both toast and structured/app logging for warnings, you may want to relax the shouldForwardToAppLog / shouldLogToConsole conditions instead of treating toasts as a replacement.

Also applies to: 167-192, 205-205

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5871c4 and 5aa5dcd.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json and included by none

📒 Files selected for processing (5)

• lib/logger.ts (6 hunks)
• lib/request/response-handler.ts (2 hunks)
• test/README.md (1 hunks)
• test/cache-warming.test.ts (1 hunks)
• test/logger.test.ts (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

lib/request/response-handler.ts (1)

lib/logger.ts (1)

• isLoggingEnabled (22-24)

test/logger.test.ts (1)

lib/logger.ts (5)

• isLoggingEnabled (22-24)
• configureLogger (106-128)
• logRequest (130-147)
• flushRollingLogsForTest (165-170)
• logWarn (157-159)

lib/logger.ts (2)

lib/utils/file-system-utils.ts (1)

• getOpenCodePath (22-24)

lib/types.ts (2)

• PluginConfig (6-40)
• LoggingConfig (42-55)

🔇 Additional comments (2)

lib/request/response-handler.ts (1)

2-2: Use of isLoggingEnabled() here looks correct and side‑effect free

The import and guard now rely on the accessor instead of a mutable export, preserving the previous behavior while aligning with the safer logger API.

Also applies to: 53-55

test/cache-warming.test.ts (1)

19-24: Logger mock update matches new API surface

Exposing isLoggingEnabled: () => false keeps the mock compatible with the logger module without affecting existing cache‑warming assertions.

[riatzukiza]

/oc In test/logger.test.ts around lines 68 to 72, add an extra assertion to validate
that isLoggingEnabled() reflects config overrides as well as the env flag: after
setting process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1" and importing logger,
call configureLogger({ pluginConfig: { logging: { enableRequestLogging: false }
} }) and then assert isLoggingEnabled() returns false; repeat the same pattern
in the other related test block (around lines 110-122) to ensure the accessor
respects config overrides when env enables logging.

[opencode-agent]

ProviderInitError

opencode session  |  github run

[riatzukiza]

/oc In test/logger.test.ts around lines 68 to 72, add an extra assertion to validate
that isLoggingEnabled() reflects config overrides as well as the env flag: after
setting process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1" and importing logger,
call configureLogger({ pluginConfig: { logging: { enableRequestLogging: false }
} }) and then assert isLoggingEnabled() returns false; repeat the same pattern
in the other related test block (around lines 110-122) to ensure the accessor
respects config overrides when env enables logging.

[opencode-agent]

Excellent! The changes have been successfully implemented. Here's a summary of what was added:

Summary

I've added the requested assertions to validate that isLoggingEnabled() reflects config overrides as well as the env flag in both test blocks:

Changes Made:

1. test/logger.test.ts:68-72 - Added assertion in the first test block:

   • After setting process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1" and importing logger
   • Called configureLogger({ pluginConfig: { logging: { enableRequestLogging: false } } })
   • Added assertion expect(isLoggingEnabled()).toBe(false) to verify the config override is reflected

2. test/logger.test.ts:110-122 - Added assertion in the second test block:

   • Imported isLoggingEnabled alongside the other logger functions
   • Added assertion expect(isLoggingEnabled()).toBe(false) after calling configureLogger() to verify the config override is reflected before testing the file operations

Verification:

• All 13 logger tests pass successfully
• TypeScript compilation completes without errors
• The new assertions ensure that the isLoggingEnabled() accessor properly respects both environment variables and plugin configuration overrides

These changes strengthen the test coverage by explicitly verifying that the isLoggingEnabled() function correctly reflects the state when configuration overrides are applied, ensuring the accessor respects the priority hierarchy (config overrides env).

opencode session  |  github run