Conversation
docs: clarify install and plugin settings
Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com>
Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com>
Allow config overrides for logging and silence warn toasts
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces the Codex compaction pipeline with env-aware OpenCode prompt filtering, adds nested runtime-configurable logging (isLoggingEnabled/configureLogger), and augments SessionManager with prefix-change analysis used for cache-key decisions and structured warnings. Changes
Sequence Diagram(s)sequenceDiagram
participant ConfigLoader as Config Loader
participant Config as lib/config.ts
participant Logger as lib/logger.ts
participant Consumer as Request/Response Handler
ConfigLoader->>Config: read plugin config file
Config->>Config: getDefaultConfig()
Config->>Config: merge user config (shallow) and merge logging: { ...defaults.logging, ...user.logging }
Config->>Logger: pass merged PluginConfig
Logger->>Logger: applyLoggingOverrides(pluginConfig.logging)
Logger->>Logger: refreshLoggingState()
Consumer->>Logger: isLoggingEnabled()
Logger-->>Consumer: boolean
Consumer->>Logger: logRequest / logWarn (conditional)
sequenceDiagram
participant Session as SessionManager
participant Analyzer as Prefix Analyzer
participant Logger as lib/logger.ts
participant Cache as Prompt Cache
Session->>Analyzer: detect non-full-prefix match(oldPrefix, newPrefix)
Analyzer->>Session: PrefixChangeAnalysis
Session->>Logger: logWarn(prefixAnalysis)
alt analysis.sharedPrefixLength == 0
Session->>Cache: regenerate prompt_cache_key (fork)
else
Session->>Cache: fork with forkSessionId & forkPromptCacheKey metadata
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Remove plugin-side compaction
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
lib/logger.ts (1)
172-211: Warn toast gating and forwarding logic matches tests and intentThe new
warnToastEnabled/shouldForwardToAppLog/shouldLogToConsolelogic:
- Keeps disk logging tied to
LOGGING_ENABLED || DEBUG_ENABLED.- For warnings:
- Default: forward to
app.logand console, no toast.- When toasts are enabled and TUI is present: send a toast, suppress
app.logand console to avoid double surfacing.- For errors: always send a toast (if available) and log to console, regardless of the toast config.
This aligns with the new tests and yields predictable behavior without introducing regressions. The duplicated condition (
level !== "warn" || !warnToastEnabled) for app‑log vs console decisions is acceptable; you could extract it to a small helper for readability, but it’s not necessary.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (9)
README.mdis excluded by none and included by noneconfig/README.mdis excluded by none and included by nonedocs/README.mdis excluded by none and included by noneindex.tsis excluded by none and included by nonepackage-lock.jsonis excluded by!**/package-lock.jsonand included by nonepackage.jsonis excluded by none and included by nonespec/environment-variables.mdis excluded by none and included by nonespec/prefix-diagnostics.mdis excluded by none and included by nonespec/readme-cleanup.mdis excluded by none and included by none
📒 Files selected for processing (10)
lib/config.ts(2 hunks)lib/logger.ts(6 hunks)lib/request/response-handler.ts(2 hunks)lib/session/session-manager.ts(3 hunks)lib/types.ts(1 hunks)test/README.md(1 hunks)test/cache-warming.test.ts(1 hunks)test/logger.test.ts(5 hunks)test/plugin-config.test.ts(5 hunks)test/session-manager.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
lib/config.ts (2)
lib/types.ts (1)
PluginConfig(6-40)lib/logger.ts (1)
logWarn(157-159)
test/session-manager.test.ts (2)
lib/session/session-manager.ts (1)
SessionManager(286-640)lib/types.ts (2)
RequestBody(167-189)SessionContext(219-225)
lib/request/response-handler.ts (1)
lib/logger.ts (1)
isLoggingEnabled(22-24)
lib/session/session-manager.ts (2)
lib/types.ts (1)
InputItem(156-162)lib/logger.ts (1)
logWarn(157-159)
test/logger.test.ts (1)
lib/logger.ts (5)
isLoggingEnabled(22-24)configureLogger(106-128)logRequest(130-147)flushRollingLogsForTest(165-170)logWarn(157-159)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(30-70)
lib/logger.ts (2)
lib/utils/file-system-utils.ts (1)
getOpenCodePath(22-24)lib/types.ts (2)
PluginConfig(6-40)LoggingConfig(42-55)
🪛 GitHub Check: Lint & Typecheck
test/session-manager.test.ts
[warning] 173-173:
'context' is assigned a value but never used. Allowed unused vars must match /^_/u
[warning] 126-126:
'context' is assigned a value but never used. Allowed unused vars must match /^_/u
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Mutation Tests
🔇 Additional comments (18)
lib/types.ts (1)
35-55: LGTM!The new
LoggingConfiginterface is well-structured with clear documentation. Optional fields are appropriate for configuration overrides.test/README.md (1)
82-82: LGTM!Documentation correctly updated to reflect the new
isLoggingEnabled()function API.test/cache-warming.test.ts (1)
23-23: LGTM!Mock correctly updated to reflect the new
isLoggingEnabled()function signature.test/plugin-config.test.ts (2)
57-57: LGTM!Test assertions correctly updated to validate the new default
loggingconfiguration.Also applies to: 76-76, 91-92, 122-122, 142-142
95-108: LGTM!Excellent test coverage for the nested logging configuration merge behavior, validating that user overrides are applied while defaults are preserved.
test/session-manager.test.ts (1)
2-2: LGTM!Import additions support the new logging verification tests.
Also applies to: 5-5
lib/request/response-handler.ts (1)
2-2: LGTM!Clean refactor to use the new
isLoggingEnabled()function API.Also applies to: 53-53
lib/session/session-manager.ts (3)
80-111: LGTM!Helper functions are well-implemented with appropriate null checks and comprehensive pattern matching for tool messages.
113-190: LGTM!The prefix change analysis logic is well-structured:
- Correctly detects history pruning via suffix reuse
- Identifies system prompt changes
- Provides detailed diagnostic information for debugging
409-418: LGTM!Prefix change analysis is properly integrated into the session management flow, providing diagnostic information for both cache key regeneration and session forking scenarios.
Also applies to: 464-472
lib/config.ts (3)
17-19: LGTM!Default logging configuration added with
showWarningToasts: false.
45-45: LGTM!Nested logging configuration merge correctly implemented:
- User settings override defaults
- Unspecified user settings preserve defaults
- Spread operator prevents DEFAULT_CONFIG mutation
Also applies to: 50-59
67-67: LGTM!Error fallback correctly spreads DEFAULT_CONFIG to prevent mutation.
test/logger.test.ts (4)
68-76: isLoggingEnabled env + override coverage is solidThis test cleanly asserts the env-driven default and the pluginConfig override behavior for
isLoggingEnabled(), matching the new runtime-configurable design.
114-131: Good regression test for config overriding env‑enabled request loggingThis case correctly asserts that a
pluginConfig.logging.enableRequestLogging = falseoverride wins overENABLE_PLUGIN_REQUEST_LOGGING=1, both viaisLoggingEnabled()and disk‑write expectations.
155-175: Default warning behavior with TUI client is well‑specifiedThis test clearly documents the default semantics with a TUI‑capable client: no warning toast, one
app.logcall, and a console warn. That aligns with the newWARN_TOASTS_ENABLEDgating and is a good guard against regressions.
176-201: Config‑driven warning toasts and wrapping behavior are well coveredThese tests collectively:
- Assert that enabling
pluginConfig.logging.showWarningToastsflips behavior fromapp.log+console to TUI toast only.- Verify long warning messages are wrapped to ≤72 characters per line.
They tightly couple to the intended runtime behavior and should catch most regressions in toast handling.
Also applies to: 214-215
lib/logger.ts (1)
4-31: Env defaults + mutable logging state and isLoggingEnabled() look consistentThe introduction of
envLoggingDefaults, mutableLOGGING_ENABLED/DEBUG_FLAG_ENABLEDandisLoggingEnabled()cleanly preserves prior env semantics while enabling later overrides. InitialDEBUG_ENABLEDandCONSOLE_LOGGING_ENABLEDderivations are straightforward, and usinggetEnvNumberfor rotation/queue limits with hard minimums avoids pathological values like 0 or negatives. No blockers here.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
README.mdis excluded by none and included by nonedocs/configuration.mdis excluded by none and included by nonedocs/getting-started.mdis excluded by none and included by nonespec/codex-compaction.mdis excluded by none and included by nonespec/compaction-heuristics-22.mdis excluded by none and included by nonespec/remove-plugin-compaction.mdis excluded by none and included by none
📒 Files selected for processing (11)
lib/config.ts(2 hunks)lib/request/codex-fetcher.ts(1 hunks)lib/request/fetch-helpers.ts(1 hunks)lib/request/request-transformer.ts(4 hunks)lib/types.ts(1 hunks)test/codex-compaction.test.ts(0 hunks)test/codex-fetcher.test.ts(0 hunks)test/compaction-executor.test.ts(0 hunks)test/compaction-helpers.test.ts(0 hunks)test/fetch-helpers.test.ts(0 hunks)test/plugin-config.test.ts(5 hunks)
💤 Files with no reviewable changes (5)
- test/fetch-helpers.test.ts
- test/compaction-executor.test.ts
- test/codex-compaction.test.ts
- test/compaction-helpers.test.ts
- test/codex-fetcher.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(28-68)
lib/config.ts (2)
lib/types.ts (1)
PluginConfig(6-24)lib/logger.ts (1)
logWarn(157-159)
lib/request/codex-fetcher.ts (1)
lib/request/fetch-helpers.ts (1)
handleSuccessResponse(352-367)
lib/request/fetch-helpers.ts (1)
lib/types.ts (1)
PluginConfig(6-24)
lib/request/request-transformer.ts (1)
lib/request/tooling.ts (1)
normalizeToolsForCodexBody(5-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Mutation Tests
🔇 Additional comments (9)
lib/request/codex-fetcher.ts (1)
95-95: Clean removal of compaction handling.The direct assignment from
handleSuccessResponsesimplifies the flow now that compaction is removed. The change is correct and aligns with the broader removal of codex compaction paths.lib/config.ts (2)
48-57: Well-implemented nested configuration merge.The nested merge logic correctly preserves both default and user-provided logging values. Extracting
userLoggingseparately and then performing an explicit nested merge ensures that user settings override defaults at the field level rather than replacing the entire logging object.
43-43: Good practice: Cloning default config.Using spread operator to clone
DEFAULT_CONFIGprevents unintended mutations to the default configuration object. This is correctly applied in both the missing-file case (line 43) and error case (line 65).lib/types.ts (1)
26-39: Clear and well-documented type definitions.The
LoggingConfiginterface provides comprehensive logging customization options with clear JSDoc comments. All fields are appropriately optional, allowing granular control over logging behavior while maintaining backward compatibility.test/plugin-config.test.ts (2)
89-102: Excellent test coverage for nested merge logic.This test specifically validates that nested logging configuration is properly merged with defaults, preserving both user-provided values (
enableRequestLogging: false,logMaxFiles: 2) and default values (showWarningToasts: false). This ensures the merge behavior works correctly at the nested level.
52-56: Comprehensive test updates reflect new structure.All test expectations have been correctly updated to include the nested
logging: { showWarningToasts: false }structure across default, success, and error scenarios. The consistency across all test cases ensures the configuration behavior is properly validated.Also applies to: 69-73, 82-86, 111-115, 129-133
lib/request/request-transformer.ts (3)
108-108: Hardcoded parameter value after compaction removal.The second parameter to
normalizeToolsForCodexBodyis now hardcoded tofalse, meaningskipConversationTransformswill always be false. This is consistent with removing compaction logic, ensuring conversation transforms always occur.
23-31: Simplified public API surface.The removal of
compactionproperty fromTransformRequestOptionsandcompactionDecisionfromTransformResultcleanly reflects the elimination of compaction features. The interfaces are now more focused and easier to understand.
128-128: Return type simplified correctly.The return statement now only includes
{ body }, matching the updatedTransformResultinterface. This is consistent with the removal of compaction decision tracking.
|
/oc address the code review comments |
|
UnknownError: |
|
/oc address the coderabbit codereview comments |
|
ProviderInitError |
|
/oc In lib/request/fetch-helpers.ts around line 127, remove the unused optional |
|
ProviderInitError |
|
ProviderInitError |
Remove compaction paths and opt-in warning logging
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/request/request-transformer.ts (2)
42-63: Env-tail capture/reattachment path matches tests and keeps ID semantics clearThe refactored
transformInputForCodexcleanly separates concerns:
- Initial
filterInput(..., { preserveIds, preserveMetadata: true })keeps metadata around long enough for OpenCode/env detection.CODEX_APPEND_ENV_CONTEXTtoggles between simple filtering and capturing env segments, which are then reattached as a separate developer message while env-only/system messages are dropped.- A second
filterInputpass whenpreserveIds === falseensures statelessness (IDs stripped, metadata removed) before adding the bridge/tool messages.This aligns with the new tests and keeps the behavior explicit; the extra
workingInput = workingInput || []before the spread is a tiny redundancy but not worth changing unless you’re already touching this block.Also applies to: 64-83
99-106: Remove unused_pluginConfigparameter fromtransformRequestForCodexin fetch-helpers.tsThe call site at line 156 correctly passes the new
transformRequestBodysignature without_pluginConfig. However,transformRequestForCodex(line 127) still declares this parameter as unused—it's not referenced anywhere in the function body.Remove
_pluginConfig?: PluginConfig,from the function signature to complete the refactoring.
♻️ Duplicate comments (1)
lib/logger.ts (1)
66-77: Reconfirm semantics ofapplyLoggingOverrideswhenloggingis omittedCurrent behavior keeps any prior in-memory overrides when
options.pluginConfig?.loggingis omitted, only recomputing derived flags viarefreshLoggingState(). IfconfigureLoggermay be called with different plugin configs over time, you might instead want the “no logging config” path to reset all mutable logging state fromenvLoggingDefaults(including numeric limits viaensurePositiveNumber) before refreshing, so callers can reliably get back to env defaults.Also applies to: 78-92, 94-94, 109-117
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (29)
.eslintignoreis excluded by none and included by none.gitignoreis excluded by none and included by noneCHANGELOG.mdis excluded by none and included by noneREADME.mdis excluded by none and included by nonedocs/code-cleanup-summary.mdis excluded by none and included by nonedocs/code-quality-analysis-report.mdis excluded by none and included by nonedocs/configuration.mdis excluded by none and included by nonedocs/getting-started.mdis excluded by none and included by nonedocs/notes/2025.11.19.18.38.24.mdis excluded by none and included by noneeslint.config.mjsis excluded by none and included by nonepackage.jsonis excluded by none and included by nonescripts/inspect-codex-logs.mjsis excluded by none and included by nonespec/auto-compaction-summary.mdis excluded by none and included by nonespec/cache-analysis.mdis excluded by none and included by nonespec/complexity-reduction.mdis excluded by none and included by nonespec/issue-triage-2025-11-20.mdis excluded by none and included by nonespec/log-warnings-default-file-only.mdis excluded by none and included by nonespec/merge-conflict-resolution.mdis excluded by none and included by nonespec/open-issues-triage.mdis excluded by none and included by nonespec/plugin-log-settings-doc.mdis excluded by none and included by nonespec/pr-2-conflict-analysis.mdis excluded by none and included by nonespec/pr-20-review.mdis excluded by none and included by nonespec/pr-29-review-analysis.mdis excluded by none and included by nonespec/pr-commit-2025-11-21.mdis excluded by none and included by nonespec/readme-cleanup.mdis excluded by none and included by nonespec/request-transformer-refactor.mdis excluded by none and included by nonespec/review-pr-20-plan.mdis excluded by none and included by nonespec/review-v0.3.5-fixes.mdis excluded by none and included by nonespec/session-prefix-mismatch.mdis excluded by none and included by none
📒 Files selected for processing (16)
lib/compaction/codex-compaction.ts(0 hunks)lib/compaction/compaction-executor.ts(0 hunks)lib/config.ts(2 hunks)lib/logger.ts(6 hunks)lib/prompts/codex-compaction.ts(0 hunks)lib/request/compaction-helpers.ts(0 hunks)lib/request/input-filters.ts(3 hunks)lib/request/request-transformer.ts(6 hunks)lib/session/session-manager.ts(4 hunks)lib/types.ts(1 hunks)test/cache-e2e.test.ts(1 hunks)test/logger.test.ts(6 hunks)test/plugin-config.test.ts(5 hunks)test/prompts-codex.test.ts(4 hunks)test/request-transformer.test.ts(4 hunks)test/session-manager.test.ts(6 hunks)
💤 Files with no reviewable changes (4)
- lib/prompts/codex-compaction.ts
- lib/request/compaction-helpers.ts
- lib/compaction/compaction-executor.ts
- lib/compaction/codex-compaction.ts
🧰 Additional context used
🧬 Code graph analysis (10)
test/logger.test.ts (1)
lib/logger.ts (5)
isLoggingEnabled(23-25)configureLogger(109-131)logRequest(133-150)flushRollingLogsForTest(168-173)logWarn(160-162)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(29-69)
test/request-transformer.test.ts (3)
lib/types.ts (2)
InputItem(142-148)RequestBody(153-175)lib/request/input-filters.ts (1)
filterOpenCodeSystemPrompts(151-156)lib/request/request-transformer.ts (2)
filterOpenCodeSystemPrompts(20-20)transformRequestBody(99-149)
lib/request/request-transformer.ts (2)
lib/request/input-filters.ts (2)
filterOpenCodeSystemPromptsWithEnv(158-162)filterOpenCodeSystemPrompts(151-156)lib/request/tooling.ts (1)
normalizeToolsForCodexBody(5-31)
lib/session/session-manager.ts (2)
lib/types.ts (1)
InputItem(142-148)lib/logger.ts (1)
logWarn(160-162)
test/cache-e2e.test.ts (3)
lib/types.ts (2)
UserConfig(46-53)RequestBody(153-175)lib/session/session-manager.ts (1)
SessionManager(267-591)lib/request/fetch-helpers.ts (1)
transformRequestForCodex(120-198)
lib/request/input-filters.ts (2)
lib/types.ts (1)
InputItem(142-148)lib/utils/input-item-utils.ts (1)
extractTextFromItem(16-27)
lib/logger.ts (2)
lib/utils/file-system-utils.ts (1)
getOpenCodePath(22-24)lib/types.ts (2)
PluginConfig(6-24)LoggingConfig(26-41)
test/session-manager.test.ts (2)
lib/types.ts (2)
SessionContext(203-209)RequestBody(153-175)lib/session/session-manager.ts (1)
SessionManager(267-591)
lib/config.ts (2)
lib/types.ts (1)
PluginConfig(6-24)lib/logger.ts (1)
logWarn(160-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Mutation Tests
🔇 Additional comments (10)
test/prompts-codex.test.ts (1)
113-153: Logger-based warning assertions for prompt fallbacks are correctSwitching these tests from console-based checks to spying on
logger.logWarnaligns with the new logging API, and the dynamic import + explicitmockRestore()keep test isolation clean. No issues here.Also applies to: 242-272
test/cache-e2e.test.ts (1)
12-45: Env-churn cache stability scenario is well coveredThe
envMessagehelper plus the “env/files churn” test do a good job of validating that environment/file noise is stripped before session matching and thatprompt_cache_keyremains stable while user content is unchanged. The expectations oninputlength and role are tight but still robust. No changes needed.Also applies to: 52-89
test/logger.test.ts (2)
68-76: isLoggingEnabled and request-logging override coverage is solidThe new tests around
isLoggingEnabledandconfigureLogger({ pluginConfig: { logging: { enableRequestLogging: false } } })validate both env‑driven enablement and config‑driven disablement, and they tie that directly to disk I/O expectations vialogRequest. This gives good confidence that request logging will honour plugin config overrides at runtime.Also applies to: 114-131
145-221: logWarn behaviour under LoggingConfig is thoroughly exercisedThe added
logWarntests cover all key dimensions of the new logging config: default behaviour (rolling log only), toast suppression by default, toast enablement viashowWarningToasts, and console mirroring vialogWarningsToConsole. The use of anOpencodeClientmock andflushRollingLogsForTestkeeps the assertions precise without depending on implementation details.This suite provides strong regression protection for the new LoggingConfig surface.
Also applies to: 233-247
test/plugin-config.test.ts (1)
52-57: Logging defaults and merge behavior are exercised wellThe expectations for
loggingacross default, file-present, empty, malformed, and error cases align with the nested merge inloadPluginConfig, including the override behavior forenableRequestLoggingandlogMaxFiles. No blockers here.Also applies to: 69-73, 82-87, 89-103, 112-117, 131-135
lib/types.ts (1)
21-24: LoggingConfig extension of PluginConfig is coherent and non-breakingAdding
logging?: LoggingConfigwith all-optional fields keeps existing configs valid and documents the new overrides (request logging, debug, toasts, rotation) clearly; this looks consistent with how the tests useconfig.logging.Also applies to: 26-40
lib/logger.ts (2)
12-33: Logging defaults and runtime flags are wired coherently (no blockers)The env-derived defaults and initial module-level flags (including
LOGGING_ENABLED,DEBUG_FLAG_ENABLED, rotation and queue limits) are consistent and correctly funneled throughDEBUG_ENABLED/CONSOLE_LOGGING_ENABLED.isLoggingEnabled()cleanly exposes the intended boolean without leaking internal flags. No issues here.
175-215: Warn persistence and toast/console routing look correctThe updated
emitlogic consistently:
- Persists all
warn-level entries regardless of env flags.- Avoids double-reporting by skipping app-log forwarding when a warn toast is shown.
- Gates console output for warns via
WARN_CONSOLE_ENABLEDand toast presence, while always logging errors.
This matches the intended separation between persistence, app logs, toasts, and console output. No blockers.lib/request/input-filters.ts (2)
72-103: Env-block stripping is targeted and preserves non-env contentThe
stripOpenCodeEnvBlockshelper cleanly:
- Removes the known OpenCode env header phrase.
- Strips
<env>...</env>and<files>...</files>blocks while capturing their exact text intoremovedBlocks.- Returns trimmed residual text, so surrounding narrative isn’t lost.
This is a focused, low-risk transformation for the expected OpenCode shapes; behavior looks sound and doesn’t introduce blockers.
105-110: Internal filter + new env-capturing API look consistent with prior behavior
filterOpenCodeSystemPromptsInternalpreserves users, drops OpenCode system prompts as before, and now additionally:
- Strips env/file blocks from non-OpenCode non-user items, dropping items that become empty after stripping.
- Optionally accumulates removed blocks into
envSegmentswhencaptureEnvis set.- Returns
{ input: filteredInput, envSegments }, with wrappers providing either justinputor the fullFilterResult.The non-array input path preserves prior external behavior (
filterOpenCodeSystemPromptsstill returns the original value), while the newfilterOpenCodeSystemPromptsWithEnvgets a predictable{ envSegments: [] }fallback. Overall, the control flow and result typing are coherent; no blockers identified.Also applies to: 118-149, 151-162
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Add configurable env tail append option
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/request/fetch-helpers.ts (1)
120-167: Blocker: Unit tests don't pass pluginConfig, leaving the new parameter untestedThe call sites in
test/fetch-helpers.test.tsat lines 299, 340, and 380 omitpluginConfigentirely. While the e2e test (test/cache-e2e.test.ts:39) does exercise it, the unit test suite should pass this parameter to ensure the behavior is tested consistently with production call sites (lib/request/codex-fetcher.ts:55already passes it correctly).The error and early-return tests (lines 259, 269) reasonably skip it since they won't reach the
appendEnvContextlogic, but the happy-path tests should include it for complete coverage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/append-env-context-config.mdis excluded by none and included by none
📒 Files selected for processing (7)
lib/config.ts(2 hunks)lib/request/fetch-helpers.ts(1 hunks)lib/request/request-transformer.ts(5 hunks)lib/types.ts(1 hunks)test/cache-e2e.test.ts(1 hunks)test/plugin-config.test.ts(7 hunks)test/request-transformer.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/request-transformer.test.ts (3)
lib/types.ts (2)
InputItem(148-154)RequestBody(159-181)lib/request/request-transformer.ts (2)
filterOpenCodeSystemPrompts(20-20)transformRequestBody(101-159)lib/request/input-filters.ts (1)
filterOpenCodeSystemPrompts(151-156)
test/cache-e2e.test.ts (3)
lib/types.ts (4)
UserConfig(52-59)InputItem(148-154)RequestBody(159-181)PluginConfig(6-30)lib/session/session-manager.ts (1)
SessionManager(267-591)lib/request/fetch-helpers.ts (1)
transformRequestForCodex(120-200)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(32-74)
lib/config.ts (3)
lib/types.ts (1)
PluginConfig(6-30)lib/utils/file-system-utils.ts (1)
safeReadFile(56-62)lib/logger.ts (1)
logWarn(160-162)
lib/request/request-transformer.ts (3)
lib/types.ts (2)
RequestBody(159-181)SessionContext(209-215)lib/request/input-filters.ts (2)
filterOpenCodeSystemPromptsWithEnv(158-162)filterOpenCodeSystemPrompts(151-156)lib/request/tooling.ts (1)
normalizeToolsForCodexBody(5-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Mutation Tests
🔇 Additional comments (5)
lib/config.ts (1)
12-22: Default config + nested logging merge behave correctly
getDefaultConfignow returns a fresh object per call (including a freshloggingobject), andloadPluginConfigmerges user settings over those defaults while deep‑merginglogging. The missing/invalid‑file paths also clone defaults rather than sharing references.This resolves the earlier shared‑logging concern and matches the expectations in the plugin‑config tests; no changes needed here.
Also applies to: 43-72
test/cache-e2e.test.ts (1)
12-125: Cache E2E and prefix-diagnostics coverage look solidThe
runTransformhelper plus the two scenarios (“env/files churn” and “user_message_changed”) exercise the new session-prefix diagnostics end‑to‑end, while cleanly stubbing Codex and logger behavior. Using a minimalPluginConfigfocused onappendEnvContextkeeps the tests aligned with the new configuration surface.No issues or blockers from this file.
test/plugin-config.test.ts (1)
31-54: Plugin config tests correctly cover env + logging semanticsThe updated tests now fully cover the intended behavior of
loadPluginConfig—including env‑drivenappendEnvContext, config overrides, and nestedloggingmerges—while safely snapshotting and restoring both relevant env vars.Everything here lines up with
lib/config.ts; no changes needed.Also applies to: 56-193
test/request-transformer.test.ts (1)
242-257: Transformer tests capture env-stripping and cache-key stability wellThe added cases around env-only system messages, mixed env+AGENTS content, and the dedicated “caching stability” describe block closely mirror the new transformer behavior:
filterOpenCodeSystemPromptsspecs ensure env noise is stripped while AGENTS.md survives.- The caching tests validate that
prompt_cache_keyremains stable when only env/files change and that env/files can be reattached as a tail whenappendEnvContextis enabled.These provide good coverage of the new env-context flow; no blockers here.
Also applies to: 590-615, 638-675, 682-791
lib/types.ts (1)
20-47: PluginConfig and LoggingConfig shape matches downstream usageAdding
appendEnvContext?: booleanand a nestedlogging?: LoggingConfigaligns with how config loading and logging are now wired. The LoggingConfig fields are all optional and forward‑compatible, which works well with the default + merge strategy inlib/config.ts.No issues with these type changes.
Refactor session manager item equality
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/session-manager-items-equality.mdis excluded by none and included by none
📒 Files selected for processing (1)
lib/session/session-manager.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/session/session-manager.ts (2)
lib/types.ts (1)
InputItem(148-154)lib/logger.ts (1)
logWarn(160-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Mutation Tests
🔇 Additional comments (2)
lib/session/session-manager.ts (2)
117-183: Prefix-change analysis covers key cases without leaking content
analyzePrefixChangecleanly separateshistory_pruned,system_prompt_changed,user_message_changed, andunknown, and the diagnostics payload sticks to roles, counts, and short fingerprints instead of raw message bodies. This aligns well with the PR objectives and avoids privacy/log‑bloat pitfalls. I don’t see functional issues here; any further refinement (e.g., extra sub‑causes) would be purely iterative.
383-386:applyRequestintegration with prefix diagnostics and forking looks correct and non‑leakyThe new flow—cloning input, hashing once, computing
sharedPrefixLength, and then usinganalyzePrefixChangeonly when needed—preserves existing session behavior while adding clear diagnostics. Both the “regenerate cache key” path (sharedPrefixLength === 0) and the “fork session” path (partial prefix) now log only counts, hashes, and the derived analysis fields, with no rawInputItemcontent. Fork state propagation oflastCachedTokensandbridgeInjectedalso looks correct. I don’t see any blockers in this integration.Also applies to: 398-402, 406-414, 438-453, 455-468
1 participant
Summary