Remove compaction paths and opt-in warning logging#75
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR removes the codex-based conversation compaction subsystem (modules, prompts, helpers, and session APIs), adds an optional config/env flag to mirror warnings to the console, and updates OpenCode prompt filtering to strip environment blocks with optional env-segment reattachment. Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant RequestTransformer
participant InputFilters
participant SessionManager
participant Logger
participant Console
Note over RequestTransformer,InputFilters: Codex transform with optional env-tail
Config->>RequestTransformer: CODEX_APPEND_ENV_CONTEXT flag
RequestTransformer->>InputFilters: filterOpenCodeSystemPromptsWithEnv(input)
InputFilters-->>RequestTransformer: { input, envSegments? }
alt envSegments present && flag enabled
RequestTransformer->>RequestTransformer: append envSegments as developer message
end
RequestTransformer->>SessionManager: produce transformed RequestBody (prompt_cache_key preserved)
alt warning emitted (prefix mismatch / user change)
RequestTransformer->>Logger: logWarn(warningPayload)
alt WARN_CONSOLE_ENABLED && level == "warn"
Logger->>Console: console.warn(...)
end
Logger->>Logger: appendFile to rolling log
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)test/session-manager.test.ts (2)
🔇 Additional comments (7)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
test/request-transformer.test.ts (1)
243-252: Host metadata test update matches new semantics; consider stronger assertionSwitching the sample item to
content: "Custom host metadata message"withmetadata: { source: "host-metadata" }is consistent with the new host-metadata flow and keeps the behavior (preserve metadata, drop IDs) intact.If you want tighter regression coverage, you could assert the metadata value itself, e.g.
expect(result![0].metadata).toEqual({ source: "host-metadata" }), instead of only checking thatmetadataexists.test/logger.test.ts (1)
38-45: Clear new logging env flags in tests to avoid env‑dependent flakinessThe setup resets several logging env vars, but not
CODEX_LOG_WARNINGS_TO_CONSOLE(andCODEX_SHOW_WARNING_TOASTS), while later tests assume warnings stay off the console by default and toasts are disabled unless explicitly enabled.If those vars are set in the outer environment, tests like the new
logWarncases can start failing spuriously. It’s safer to normalize them inbeforeEach.Apply this change to the test 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; + delete process.env.CODEX_LOG_WARNINGS_TO_CONSOLE;lib/logger.ts (1)
12-20: Warn logging defaults and overrides are wired correctly; consider error persistence symmetryThe new pieces all fit together cleanly:
envLoggingDefaults.logWarningsToConsoleseedsWARN_CONSOLE_ENABLED, andapplyLoggingOverridesletsLoggingConfig.logWarningsToConsoleoverride the env default.emitnow:
- Always appends
warnentries to the rolling log (LOGGING_ENABLED || DEBUG_ENABLED || level === "warn"), matching the goal of keeping warnings in file logs even when request logging is “off”.- Sends warn‑level console output only when
WARN_CONSOLE_ENABLEDis true and warning toasts aren’t being used, which is exactly what the tests assert.- Leaves
debug/infoconsole behavior controlled byCONSOLE_LOGGING_ENABLED, and errors always hit the console.One thing to sanity‑check: with
shouldPersist = LOGGING_ENABLED || DEBUG_ENABLED || level === "warn", error logs are not persisted unless logging or debug is enabled, while warnings are. If you intended “errors should never be less durable than warnings”, you might want to extend the condition to includelevel === "error"as well. Otherwise, the implementation looks coherent.Also applies to: 27-32, 78-92, 187-188, 211-215
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (25)
CHANGELOG.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 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/remove-plugin-compaction.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 (14)
lib/compaction/codex-compaction.ts(0 hunks)lib/compaction/compaction-executor.ts(0 hunks)lib/config.ts(1 hunks)lib/logger.ts(5 hunks)lib/prompts/codex-compaction.ts(0 hunks)lib/request/compaction-helpers.ts(0 hunks)lib/request/input-filters.ts(0 hunks)lib/session/session-manager.ts(2 hunks)lib/types.ts(1 hunks)test/logger.test.ts(3 hunks)test/plugin-config.test.ts(6 hunks)test/prompts-codex.test.ts(4 hunks)test/request-transformer.test.ts(1 hunks)test/session-manager.test.ts(0 hunks)
💤 Files with no reviewable changes (6)
- lib/prompts/codex-compaction.ts
- lib/compaction/compaction-executor.ts
- lib/request/compaction-helpers.ts
- lib/request/input-filters.ts
- test/session-manager.test.ts
- lib/compaction/codex-compaction.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/logger.test.ts (1)
lib/logger.ts (3)
logWarn(160-162)flushRollingLogsForTest(168-173)configureLogger(109-131)
🔇 Additional comments (5)
test/plugin-config.test.ts (1)
52-56: Config tests correctly cover newlogWarningsToConsoledefault across pathsThe added
logging: { showWarningToasts: false, logWarningsToConsole: false }expectations on default, file-load, merge, and error/fallback paths match the new default config and nested logging merge behavior. Looks consistent and thorough.Also applies to: 69-73, 82-86, 97-102, 112-116, 131-135
lib/types.ts (1)
33-35:LoggingConfigextension forlogWarningsToConsoleis consistentThe new
logWarningsToConsole?: booleanfield and its documentation align with the logger’s override logic and the DEFAULT_CONFIG shape; no typing or behavioral concerns here.lib/config.ts (1)
15-18: Default plugin config now correctly includeslogWarningsToConsoleAdding
logWarningsToConsole: falsetoDEFAULT_CONFIG.loggingkeeps the runtime config shape in sync withLoggingConfigand the tests, and it participates in the nested logging merge as expected.test/logger.test.ts (1)
145-158:logWarntests correctly exercise new console‑mirroring behaviorThe new tests verify that:
- By default,
logWarnappends to the rolling log but doesn’t touchconsole.warn.- With a TUI client and default config, warnings go to the app log (file), not toasts or console.
- When
logging.logWarningsToConsoleis enabled viaconfigureLogger,logWarnboth appends and emits a prefixed console warning.This matches the updated
emitlogic (shouldPersistincluding warn andshouldLogToConsolegating warn onWARN_CONSOLE_ENABLED) and gives good coverage of the new flag.Also applies to: 160-180, 209-220
test/prompts-codex.test.ts (1)
113-117: Prompts tests now correctly assert vialogger.logWarninstead ofconsole.warnSwapping the expectations to spy on
logger.logWarnin the “fetch fails” and “no cache available” cases lines the tests up with the new logging pathway, while still usingconsole.errorfor the actual fetch error details. This keeps behavior the same from a user perspective but decouples tests from console‑warning defaults.Also applies to: 144-145, 151-153, 242-246, 263-264, 270-272
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/session-manager.test.ts (1)
108-145: Test name vs behavior for user-only changes.The
"does not warn on user-only content changes"test does expect a"prefix mismatch"warning and assertsprefixCause: "user_message_changed". Behavior-wise this is correct and useful, but the name is now misleading. Consider renaming to better match the semantics (e.g.,"labels user-only prefix mismatches as user_message_changed").
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
.eslintignoreis excluded by none and included by none.gitignoreis excluded by none and included by noneREADME.mdis excluded by none and included by noneeslint.config.mjsis excluded by none and included by nonescripts/inspect-codex-logs.mjsis excluded by none and included by none
📒 Files selected for processing (6)
lib/request/input-filters.ts(3 hunks)lib/request/request-transformer.ts(2 hunks)lib/session/session-manager.ts(5 hunks)test/cache-e2e.test.ts(1 hunks)test/request-transformer.test.ts(4 hunks)test/session-manager.test.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
lib/request/request-transformer.ts (1)
lib/request/input-filters.ts (2)
filterOpenCodeSystemPromptsWithEnv(158-162)filterOpenCodeSystemPrompts(151-156)
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)
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)
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/session/session-manager.ts (1)
lib/logger.ts (1)
logWarn(160-162)
🪛 GitHub Actions: CI
test/session-manager.test.ts
[error] 99-101: SessionManager regenerates cache key when prefix differs. AssertionError: expected { sessionId: 'conv-789', …(12) } to match object { …(3) }
🪛 GitHub Check: Test (20.x)
test/session-manager.test.ts
[failure] 99-99: test/session-manager.test.ts > SessionManager > regenerates cache key when prefix differs
AssertionError: expected { sessionId: 'conv-789', …(12) } to match object { …(3) }
(10 matching properties omitted from actual)
- Expected
-
Received
{
"incomingRole": "system",
"prefixCause": "system_prompt_changed",
- "previousRole": "system",
- "previousRole": "user",
}
❯ test/session-manager.test.ts:99:25
🪛 GitHub Check: Test (22.x)
test/session-manager.test.ts
[failure] 99-99: test/session-manager.test.ts > SessionManager > regenerates cache key when prefix differs
AssertionError: expected { sessionId: 'conv-789', …(12) } to match object { …(3) }
(10 matching properties omitted from actual)
- Expected
-
Received
{
"incomingRole": "system",
"prefixCause": "system_prompt_changed",
- "previousRole": "system",
- "previousRole": "user",
}
❯ test/session-manager.test.ts:99:25
🔇 Additional comments (8)
test/session-manager.test.ts (2)
147-189: History-pruning warning expectations align with implementation.This test nicely targets the
history_prunedpath: pruning earlier tool/tool_result messages yieldsremovedCount: 4andremovedRolesincluding"tool", matching thefindSuffixReuseStartandisToolMessagelogic inanalyzePrefixChange. No changes needed here.
191-318: Session usage, metrics, and capacity tests track the updated flow correctly.Using
getContext(...); applyRequest(...);in these tests mirrors the real SessionManager lifecycle and validates:
recordResponsecorrectly updateslastCachedTokens.getMetricssurfaces session summaries.- Host-provided
prompt_cache_keyreuse, fork-specific keys, TTL eviction, and MAX_ENTRIES capping all behave as intended.These tests look consistent with the current SessionManager API and state fields.
lib/session/session-manager.ts (3)
6-7: UsingcloneInputItemsbefore hashing is the right call.Cloning
body.inputviacloneInputItemsbefore computinginputHashand storinglastInputprevents external mutation from invalidating session prefix comparisons or cache-key decisions. This keeps SessionManager’s view of the conversation stable across callers.Also applies to: 371-377
105-171: Prefix-change analysis covers system, history-prune, and user-only changes cleanly.The updated
PrefixChangeCauseandanalyzePrefixChangelogic now distinguish:
history_prunedviafindSuffixReuseStartandremovedSegmentanalysis,system_prompt_changedwhen either side of the mismatch is system-like,user_message_changedwhen both mismatching roles are"user",with an explicit
"unknown"fallback. This matches how tests introspectprefixCause,previousRole,incomingRole, and pruning details, and gives good diagnostic data without changing the underlying fork vs reset behavior.
392-404: Expanded warn payloads are consistent and useful for debugging.Including
promptCacheKey,previousHash, andincomingHashin both the reset and fork warning logs, alongsideprefixCauseand detailed analysis fields, makes it much easier to trace cache-key churn and mismatches in production without changing control flow. The structure matches what the new tests assert against.Also applies to: 432-456
lib/request/input-filters.ts (1)
72-103: Env-block stripping and optional env capture behave as intended.The combination of
stripOpenCodeEnvBlocksandfilterOpenCodeSystemPromptsInternal:
- Drops OpenCode system prompts entirely via
isOpenCodeSystemPrompt.- Removes the standard OpenCode env header plus
<env>/<files>blocks, capturing them intoenvSegmentswhen requested.- Discards env-only system/developer messages while preserving user messages and non-OpenCode prompts.
- Produces sanitized content (e.g., keeps AGENTS.md text) and a clean
FilterResultAPI for callers that want to reattach env/files later.The implementation aligns with the new tests around env-only drops and AGENTS.md preservation.
Also applies to: 105-149, 158-162
lib/request/request-transformer.ts (1)
8-10: Env-tail behavior is well-isolated behindCODEX_APPEND_ENV_CONTEXT.In
codexMode:
- When
CODEX_APPEND_ENV_CONTEXT === "1", you correctly:
- Run
filterOpenCodeSystemPromptsWithEnvto both sanitize input and collect env/files blocks.- Append a final developer message containing
envSegments.join("\n")only when segments exist.- When the flag is unset, you retain the previous
filterOpenCodeSystemPromptsbehavior.Subsequent ID-stripping and bridge injection logic are unchanged and still gated on
preserveIdsandhasNormalizedTools, so the new feature is opt-in and backward-compatible.Also applies to: 64-83
test/request-transformer.test.ts (1)
242-257: Env-block tests and caching stability coverage match the transformer logic.The added/updated tests:
- Ensure host metadata is preserved when requested, decoupled from legacy compaction tags.
- Verify that env-only system messages are dropped, while AGENTS.md content survives env/files stripping.
- Confirm that
transformRequestBodykeepsprompt_cache_keystable when only env/files churn and, whenCODEX_APPEND_ENV_CONTEXTis enabled, reattaches env/files as a trailing developer message containing<env>and<files>.These assertions align with
stripOpenCodeEnvBlocks,filterOpenCodeSystemPrompts*, and the new env-tail branch intransformInputForCodex.Also applies to: 590-615, 638-675, 682-774
1 participant
Summary
Testing