Repository navigation
fix(compaction): skip microcompact when compaction is off - #1800
Conversation
Skip automatic query-loop microcompact when the message-count compaction threshold is explicitly set to off, while preserving default, numeric, and explicit compact behavior.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (9)src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,py,json,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.test.{ts,tsx,js}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe PR updates compaction threshold handling so unset and explicit ChangesMax-message compaction handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/query.ts`:
- Around line 512-528: The explicit off setting is still allowing automatic
compaction because there is another deps.microcompact() path in queryLoop that
is not covered by the new guard. Update query.ts so every automatic microcompact
call is gated by the same maxMessagesCompactionThreshold !== 'off' check, while
keeping querySource === 'compact' as the only bypass; use the queryLoop and
deps.microcompact symbols to locate and apply the fix consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 229a586a-6562-4cc6-b28f-87658251ade4
📒 Files selected for processing (3)
src/query.tssrc/query/autoCompactCooldown.test.tssrc/utils/config.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/query/autoCompactCooldown.test.tssrc/utils/config.tssrc/query.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/query/autoCompactCooldown.test.tssrc/utils/config.tssrc/query.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/query/autoCompactCooldown.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/query/autoCompactCooldown.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/query/autoCompactCooldown.test.tssrc/utils/config.tssrc/query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/query/autoCompactCooldown.test.tssrc/utils/config.tssrc/query.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/query/autoCompactCooldown.test.tssrc/utils/config.tssrc/query.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/query/autoCompactCooldown.test.tssrc/utils/config.tssrc/query.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/query/autoCompactCooldown.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/config.ts
🪛 GitHub Check: smoke-and-tests
src/query/autoCompactCooldown.test.ts
[failure] 236-236: error: expect(received).not.toHaveBeenCalled()
Expected number of calls: 0
Received number of calls: 1
at <anonymous> (/home/runner/work/openclaude/openclaude/src/query/autoCompactCooldown.test.ts:236:28)
🔇 Additional comments (3)
src/utils/config.ts (1)
699-747: LGTM!Also applies to: 1154-1165
src/query.ts (1)
17-17: LGTM!Also applies to: 454-459, 584-584
src/query/autoCompactCooldown.test.ts (1)
17-76: LGTM!Also applies to: 178-286
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/query/autoCompactCooldown.test.ts (1)
216-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
nevercast with the real deps type.successfulQueryDeps()andrunSuccessfulQuery()should use the actualQueryDeps/Parameters<typeof query>[0]['deps']type so this fixture still catches deps-shape changes at compile time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/query/autoCompactCooldown.test.ts` around lines 216 - 220, The query test fixture is using an incorrect `never` type for the `deps` parameter in `runSuccessfulQuery`, which prevents compile-time detection of dependency shape changes. Update `successfulQueryDeps()` and `runSuccessfulQuery()` to use the real `QueryDeps` type or `Parameters<typeof query>[0]['deps']` from `loadQuery()`/`query` so the fixture stays aligned with the actual deps contract.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/query/autoCompactCooldown.test.ts`:
- Around line 216-220: The query test fixture is using an incorrect `never` type
for the `deps` parameter in `runSuccessfulQuery`, which prevents compile-time
detection of dependency shape changes. Update `successfulQueryDeps()` and
`runSuccessfulQuery()` to use the real `QueryDeps` type or `Parameters<typeof
query>[0]['deps']` from `loadQuery()`/`query` so the fixture stays aligned with
the actual deps contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1e18587-d2f3-452f-8db8-547cab1329c6
📒 Files selected for processing (1)
src/query/autoCompactCooldown.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (9)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/query/autoCompactCooldown.test.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/query/autoCompactCooldown.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/query/autoCompactCooldown.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/query/autoCompactCooldown.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/query/autoCompactCooldown.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/query/autoCompactCooldown.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/query/autoCompactCooldown.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/query/autoCompactCooldown.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/query/autoCompactCooldown.test.ts
🔇 Additional comments (2)
src/query/autoCompactCooldown.test.ts (2)
14-28: LGTM!
185-189: LGTM!
|
@CodeRabbit final review |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
) * fix(compaction): respect disabled microcompact setting Skip automatic query-loop microcompact when the message-count compaction threshold is explicitly set to off, while preserving default, numeric, and explicit compact behavior. * test(query): isolate auto-compact config regression * test(query): type auto-compact deps fixture
Cherry-pick 320d63c (fix(compaction): skip microcompact when compaction is off Twigpine#1800) introduced a call to normalizeMaxMessagesCompactionThreshold at src/query.ts:473, but during conflict resolution the upstream import line did not land. query.ts imported only getGlobalConfig from './utils/config.js' — the new helper was referenced without being imported. Symptom (caught by opencc-full-verify /phase3-tui): ReferenceError: normalizeMaxMessagesCompactionThreshold is not defined at every node dist/cli.mjs -p invocation. Fix: extend the existing ./utils/config.js named import to include normalizeMaxMessagesCompactionThreshold.
3 participants
Summary
maxMessagesCompactionThresholdis explicitly"off"."off"can be detected without raw config file reads.Implementation
queryLoop, normalize it once for later message-count logic, and gate only the automatic microcompact call.maxMessagesCompactionThreshold; migration normalizes only present values.Tests
bun test src/query/autoCompactCooldown.test.tsbun test src/query*.test.tsbun test src/query/*.test.tsbun test src/services/compact/autoCompact.test.ts src/services/compact/microCompact.test.tsbun test src/services/contextCollapse/*.test.tsbun run typecheckbun run typecheck:type-testsbun run buildbun run smokebun run security:pr-scan -- --base upstream/maingit diff --checkRisk
"off"disables that automatic path.bun test src/services/compact/*.test.tsfails whencompact.test.tsruns beforeautoCompact.test.tsbecause of an existing order-dependent test fixture leak; the targeted compact tests above pass.Fixes #1715