fix(autocompact): force-compaction through breaker + hard message-count cap (closes #1373)#1615
Conversation
…nt cap (closes Gitlawb#1373) Issue Gitlawb#1373: OpenClaude can crash with a JS heap OOM after ~3h when a single summarization failure trips the auto-compact circuit breaker. The breaker's cool-down then blocks the only path that would shrink state.messages, so each new turn keeps appending and the heap grows without bound. This change has two parts: 1. autoCompactIfNeeded now honors forceReason even when the breaker is in cool-down. The breaker still increments consecutiveFailures and re-trips on a forced failure, so retry-storm protection is preserved. 2. A new MAX_ACTIVE_MESSAGES_HARD_CAP (1000, env override OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP) is consulted in the query loop alongside the existing opt-in maxMessagesCompactionThreshold. The tighter of the two wins, so the user setting keeps its semantics and the hard cap acts as a guaranteed safety net. Also adds a BreakerTripState store (compactWarningState.ts) and wires recordBreakerTripped into the trip path so the REPL/SDK can surface a 'auto-compact paused' status in a follow-up PR. No new dependencies, no API surface change, no provider-specific code. All 64 pre-existing autoCompact tests still pass; 8 new tests cover the bypass + env-var override paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,py,json,md}📄 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:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds a hard operator cap for active messages (default 1000, env override ChangesHard Message Cap and Circuit Breaker Override
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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)
src/query.ts (1)
572-606:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winactiveCap logic breaks when user cap is set but hard cap is disabled.
The current logic on line 590:
const activeCap = userCap > 0 ? Math.min(userCap, hardCap) : hardCapfails when
userCap=100andhardCap=0(operator disables safety net viaOPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP=0, but user sets opt-in threshold). Current result:activeCap = Math.min(100, 0) = 0, which disables forced compaction and ignores the user's explicit preference.The user cap and hard cap should be independent: disabling the operator-level safety net shouldn't break user-level compaction preferences.
🔧 Proposed fix
- const activeCap = userCap > 0 ? Math.min(userCap, hardCap) : hardCap + let activeCap = 0 + if (userCap > 0 && hardCap > 0) { + activeCap = Math.min(userCap, hardCap) + } else if (userCap > 0) { + activeCap = userCap + } else { + activeCap = hardCap + }Or more concisely:
- const activeCap = userCap > 0 ? Math.min(userCap, hardCap) : hardCap + const activeCap = + userCap > 0 && hardCap > 0 + ? Math.min(userCap, hardCap) + : userCap > 0 + ? userCap + : hardCap🤖 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.ts` around lines 572 - 606, The activeCap calculation treats hardCap=0 as stricter than a positive userCap and thus disables user-configured compaction; change the logic in the block that computes activeCap (after getMaxActiveMessagesHardCap(), normalizeMaxMessagesCompactionThreshold(), and userCap) so that hardCap=0 means "disabled" rather than minimal: implement something like - if (userCap > 0 && hardCap > 0) activeCap = Math.min(userCap, hardCap); else activeCap = userCap > 0 ? userCap : hardCap; - this preserves the min when both are set, uses the userCap when operator hard cap is disabled, and falls back to hardCap when no userCap is provided.
🤖 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/autoCompactCooldown.test.ts`:
- Around line 442-517: Add a new test mirroring "forced message-count compaction
overrides an active cool-down" that specifically covers the user-cap-only
scenario: set process.env.OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP = '0' and
process.env.OPENCLAUDE_MAX_ACTIVE_MESSAGES = '5' (or similar >0), build messages
>5 using overAutoCompactThresholdMessage(), call query(...) with the same
deps/mocks (mock autocompact should assert tracking.forceReason ===
'message-count' and that breaker behavior is correct), use drain(query(...)) and
assert the call model ran (terminal.reason === 'max_turns'), seenTracking
contains the forced message-count reason, and no isApiErrorMessage was yielded;
follow the existing test structure and helper functions (query, drain,
overAutoCompactThresholdMessage, autoCompactTracking shape) to keep assertions
consistent.
---
Outside diff comments:
In `@src/query.ts`:
- Around line 572-606: The activeCap calculation treats hardCap=0 as stricter
than a positive userCap and thus disables user-configured compaction; change the
logic in the block that computes activeCap (after getMaxActiveMessagesHardCap(),
normalizeMaxMessagesCompactionThreshold(), and userCap) so that hardCap=0 means
"disabled" rather than minimal: implement something like - if (userCap > 0 &&
hardCap > 0) activeCap = Math.min(userCap, hardCap); else activeCap = userCap >
0 ? userCap : hardCap; - this preserves the min when both are set, uses the
userCap when operator hard cap is disabled, and falls back to hardCap when no
userCap is provided.
🪄 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 Plus
Run ID: 71cadbd9-2f24-4122-9025-f6070b732ee2
📒 Files selected for processing (5)
src/query.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.tssrc/services/compact/compactWarningState.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/services/compact/compactWarningState.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.tssrc/query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/services/compact/compactWarningState.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.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/services/compact/compactWarningState.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.tssrc/query.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/services/compact/compactWarningState.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.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/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.ts
🔇 Additional comments (7)
src/services/compact/compactWarningState.ts (1)
19-73: LGTM!src/services/compact/autoCompact.ts (3)
90-139: LGTM!
436-481: LGTM!
603-609: LGTM!src/services/compact/autoCompact.test.ts (1)
757-928: LGTM!src/query/autoCompactCooldown.test.ts (1)
27-30: LGTM!src/query.ts (1)
578-586: Avoid concern:maxMessagesCompactionThresholdis already constrained; invalid values won’t reachparseInt
normalizeMaxMessagesCompactionThreshold()only allows'off' | '100' | '200' | '500' | '1000'and maps anything else to'off', so"abc"in config won’t produceNaN.- If
OPENCLAUDE_MAX_ACTIVE_MESSAGESparses toNaN,userCap > 0is false andactiveCapfalls back tohardCap(so no crash; behavior degrades to the safety net cap).
Two follow-up tweaks from CodeRabbit review on PR Gitlawb#1615: 1. activeCap in src/query.ts no longer collapses to 0 when the operator disables the hard cap (OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP=0) but the user has set their own opt-in threshold. The previous 'Math.min(userCap, hardCap)' returned 0 and silently disabled user-configured compaction in that configuration. The new logic treats 0 on either side as 'disabled' and picks the survivor. 2. New regression test (autoCompactCooldown.test.ts) covering the user-cap-only path: hardCap=0, OPENCLAUDE_MAX_ACTIVE_MESSAGES=5, 6 messages, asserts forceReason='message-count' fires. 3. JSDoc on recordBreakerTripped to lift docstring coverage above 80%.
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P2] Preserve cooldown after a failed forced hard-cap compaction
src/query.ts:600
When a conversation remains over the new hard cap and the forced compaction fails,autoCompactIfNeededreturns a freshnextRetryAtMs, but the next turn immediately re-enters this block and setsforceReason: 'message-count'again. BecauseautoCompactIfNeededskips the breaker cooldown wheneverforceReasonis present, a persistent summarization/provider failure now retries compaction on every over-cap turn instead of waiting for the recorded cooldown. That contradicts the stated retry-storm protection and can add repeated latency/API attempts in exactly the failure mode this breaker is meant to contain. Please keep the hard-cap recovery path from bypassing an activenextRetryAtMsafter a failed forced attempt, or add equivalent backoff before re-forcing.
|
please address comments by @jatmn |
…nt compaction jatmn's review on PR Gitlawb#1615: when a forced message-count compaction fails and the conversation stays over the hard cap, the next turn was re-entering the cap check and re-setting forceReason='message-count'. autoCompactIfNeeded then bypassed the breaker cool-down because forceReason was present, fired compactConversation again, and on a continuing provider failure re-tripped the breaker — exactly the retry storm the breaker is meant to contain. This change introduces a new tracking field, lastForcedFailureAtMs, populated only when a forced attempt fails. The cap check in src/query.ts suppresses the re-trigger when both nextRetryAtMs and lastForcedFailureAtMs are within the cool-down window; after the cool-down elapses, the cap re-trips naturally and compaction eventually happens. Token-threshold trips (no forceReason) still bypass as before, so the safety-net guarantee from Gitlawb#1373 is preserved. Files: - src/services/compact/autoCompact.ts Extend AutoCompactTrackingState and the autoCompactIfNeeded return shape with lastForcedFailureAtMs; write it on the forced-failure path only. - src/query.ts Gate the forceReason='message-count' assignment on the new field and thread the new return value through the breaker-metadata nextTracking block. - src/query/autoCompactCooldown.test.ts New 'hard-cap re-trigger respects a recent forced-failure cool-down' test (gate fires) and 'hard-cap re-trigger fires again once the cool-down has elapsed' positive-control test. - src/services/compact/autoCompact.test.ts Extend the existing 'still records cooldown on a continuing failure' test to assert lastForcedFailureAtMs is bounded by the wall clock; add 'non-forced failure leaves lastForcedFailureAtMs undefined' to keep the token-threshold path from being conflated with a forced-attempt failure. Checks: - bun run typecheck — clean - bun run typecheck:type-tests — 10 files pass - bun test src/services/compact/ — 73 pass (was 72, +2 new) - bun test src/query/autoCompactCooldown.test.ts — 9 pass (was 7, +2 new) - bun test src/QueryEngine.autoCompactCooldown.test.ts — 1 pass - bun run security:pr-scan — clean - bun run verify:privacy — all banned-pattern checks pass - bun run smoke — builds 0.18.0 (OpenClaude) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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)
src/query.ts (1)
631-657:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe hard cap still disappears when auto-compact is disabled.
This branch sets
forceReason: 'message-count', butshouldAutoCompact(..., true)still returns early on!isAutoCompactEnabled()insrc/services/compact/autoCompact.tsLines 333-375. WithDISABLE_AUTO_COMPACT=1orautoCompactEnabled=false, an over-cap session neither compacts nor blocks, so the new "always active" safety net is effectively off and the message array can keep growing past the hard cap.🤖 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.ts` around lines 631 - 657, The hard cap safety net is ineffective when auto-compact is disabled because the shouldAutoCompact function in src/services/compact/autoCompact.ts returns early when isAutoCompactEnabled() is false, ignoring the forceReason flag set in the query.ts code. To fix this, modify the shouldAutoCompact function to check whether forceReason is set (indicating a forced compaction due to hard cap violation) and bypass the early return for disabled auto-compact when forceReason is present, ensuring the hard cap is always enforced as a critical safety mechanism regardless of the autoCompactEnabled setting.
🤖 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 782-790: The code in the conditional block handling
lastForcedFailureAtMs is prematurely clearing the timestamp when
autoCompactIfNeeded() doesn't return it. Since autoCompactIfNeeded() only
includes lastForcedFailureAtMs on forced failures and not on subsequent
non-forced skips, the else branch that deletes this field causes the cooldown
timestamp to be lost after the first skipped turn. Instead of unconditionally
deleting lastForcedFailureAtMs when it's undefined, preserve the previous
timestamp from nextTracking unless you have explicit confirmation that the
cooldown window has actually expired (such as successful compaction completion
or verification that the cooldown duration has fully elapsed). This ensures the
forced failure timestamp persists across multiple turns until the cooldown
period genuinely ends.
In `@src/query/autoCompactCooldown.test.ts`:
- Around line 599-602: The test file sets the environment variable
OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS in the test cases at lines 599-602
and 685-687, but this variable is not being restored after each test, causing a
60-second cooldown to leak into subsequent test suites and cause unrelated tests
to fail. Add OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS to the SAVED_ENV object
at the top of the file to capture its original value, then ensure the beforeEach
hook clears this variable along with the other environment knobs so that each
test starts with a clean state and prevents test pollution.
---
Outside diff comments:
In `@src/query.ts`:
- Around line 631-657: The hard cap safety net is ineffective when auto-compact
is disabled because the shouldAutoCompact function in
src/services/compact/autoCompact.ts returns early when isAutoCompactEnabled() is
false, ignoring the forceReason flag set in the query.ts code. To fix this,
modify the shouldAutoCompact function to check whether forceReason is set
(indicating a forced compaction due to hard cap violation) and bypass the early
return for disabled auto-compact when forceReason is present, ensuring the hard
cap is always enforced as a critical safety mechanism regardless of the
autoCompactEnabled setting.
🪄 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 Plus
Run ID: ae6275c8-1c7f-4be4-b48c-f25ccf7ac105
📒 Files selected for processing (4)
src/query.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/query/autoCompactCooldown.test.tssrc/query.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/query/autoCompactCooldown.test.tssrc/query.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.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/query.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.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.tssrc/services/compact/autoCompact.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/query/autoCompactCooldown.test.tssrc/query.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
…ip turns CodeRabbit review on PR Gitlawb#1615 (commit a8a2321) flagged two issues in the new tracking branch: 1. The nextTracking block in src/query.ts was clearing lastForcedFailureAtMs on any turn where autoCompactIfNeeded() returned it as undefined. But autoCompactIfNeeded only writes the field on a forced-attempt failure; on a non-forced skip (token-threshold skip, half-open skip, success) the field is deliberately undefined. Clearing it on those turns dropped the cap-check cool-down signal after the first skipped turn and re-armed the original retry-storm. The field is now only overwritten when the call hands us a fresh forced-failure timestamp; the natural cap-check gate ('now - lastForcedFailureAtMs < cooldownMs') is what eventually expires the cool-down. 2. The new tests set OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS to '60000' but the value was not captured in SAVED_ENV nor cleared in beforeEach, so a 60-second cooldown could leak into unrelated test files. Added the env var to SAVED_ENV and a delete in beforeEach to keep each test's cool-down policy isolated. Adds a regression test 'lastForcedFailureAtMs persists across non-forced skip turns' that seeds the field once and runs two query() calls in sequence; both turns must surface blocking_limit, proving the cap-check gate fires on the second turn even though autoCompactIfNeeded didn't return the field the first time. Checks: - bun run typecheck — clean - bun test src/query/autoCompactCooldown.test.ts — 10 pass (was 9, +1 new) - bun test src/services/compact/ — 73 pass - bun test src/QueryEngine.autoCompactCooldown.test.ts — 1 pass - bun run smoke — builds 0.18.0 (OpenClaude) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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/autoCompactCooldown.test.ts`:
- Around line 771-861: The test does not actually verify that
lastForcedFailureAtMs persists through the query loop because both calls to
query() use the same static seedTracking object. To properly validate
persistence, add an onAutoCompactTrackingChange callback to the first query()
call to capture the output tracking state, then pass the captured state (not the
original seedTracking) as the autoCompactTracking parameter to the second
query() call. This ensures the test verifies that the field actually persists
through the query loop's tracking updates rather than simply reusing the same
seed.
🪄 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 Plus
Run ID: 9eb2fd94-4d33-4b6d-b8a5-f41d700a9176
📒 Files selected for processing (2)
src/query.tssrc/query/autoCompactCooldown.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/query.tssrc/query/autoCompactCooldown.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/query.tssrc/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.tssrc/query/autoCompactCooldown.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/query.tssrc/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.ts (1)
782-796: LGTM!src/query/autoCompactCooldown.test.ts (1)
31-32: LGTM!Also applies to: 48-52
CodeRabbit review on PR Gitlawb#1615: the persistence test was feeding the same static seedTracking to both query() calls, which only proved the seed itself was unchanged — not that the query loop actually preserved lastForcedFailureAtMs through the nextTracking branch. Add an onAutoCompactTrackingChange callback on the first call to capture what the loop published, then pass that captured state (not the original seed) into the second call. The test now exercises the real process-local tracking path the bug lives in, and asserts the captured state still carries lastForcedFailureAtMs === forcedFailureAtMs. Checks: - bun run typecheck — clean - bun test src/query/autoCompactCooldown.test.ts — 10 pass (56 calls) - bun test src/services/compact/ + QueryEngine — 74 pass - bun run smoke — builds 0.18.0 (OpenClaude) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P2] Complete CodeRabbit's request to keep the hard cap active when auto-compact is disabled
src/services/compact/autoCompact.ts:333
CodeRabbit's earlier request about this path is still valid on the current head.src/query.tsnow setsforceReason: 'message-count'when the new hard cap is exceeded and documents that the cap is always active unlessOPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP=0, butautoCompactIfNeeded()still routes that forced call throughshouldAutoCompact(..., true), and this guard returnsfalsewheneverDISABLE_AUTO_COMPACTis set orautoCompactEnabledis false. In that configuration an over-cap session proceeds without compacting, so users who disabled token-threshold autocompact lose the new OOM safety net even though they did not disable the hard cap. Please complete the CodeRabbit request by letting the hard-cap forced path bypass the auto-compact-enabled guard, or update the feature contract/tests if the hard cap is intentionally disabled with token autocompact.
The forced message-count path was routing through shouldAutoCompact with skipTokenCheck=true, but isAutoCompactEnabled() still short- circuited to false whenever DISABLE_AUTO_COMPACT was set or autoCompactEnabled was false in user config. In those configurations an over-cap session proceeded without compacting, so users who opted out of token-threshold autocompact lost the new OOM safety net (issue Gitlawb#1373) even though they did not disable the hard cap. shouldAutoCompact now takes a typed forceReason and bypasses isAutoCompactEnabled() for forced calls (message-count, memory- pressure). The other guards — recursion in forked agents, REACTIVE_COMPACT, CONTEXT_COLLAPSE — still apply because they are real safety constraints, not user opt-outs. The hard cap remains gated only by OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP=0. Co-Authored-By: Claude <noreply@anthropic.com>
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 (2)
src/query/autoCompactCooldown.test.ts (1)
606-878:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe cooldown regressions still miss the low-token / over-cap path.
Every new case here uses
overAutoCompactThresholdMessage(), soshouldAutoCompact()stays true and the suite never exercises the hard-cap-only branch where a non-forced skip can clearnextRetryAtMs. That is the branch where the retry storm can still re-arm. Add a many-short-messages scenario that stays below the token threshold acrossforced failure -> cooled-down skip -> next turn.As per coding guidelines, "Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state... Block when risky runtime changes lack focused regression coverage."
🤖 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 606 - 878, The test suite for the autocompact cooldown regression only covers scenarios with messages exceeding the token threshold (using overAutoCompactThresholdMessage()), which exercises the token-threshold forcing path. Add a new test case that uses short messages staying below the token threshold to cover the hard-cap-only branch where a non-forced skip can incorrectly clear nextRetryAtMs during the cooldown period. This test should verify that when the hard-cap-only condition is active (no token threshold exceeded) and a cooldown is in effect, the nextRetryAtMs field is preserved across multiple turns, preventing the retry storm from re-arming. The test should follow the pattern of the existing three test cases (hard-cap re-trigger respects a recent forced-failure cool-down, hard-cap re-trigger fires again once the cool-down has elapsed, and lastForcedFailureAtMs persists across non-forced skip turns) but use short/normal-sized messages instead of overAutoCompactThresholdMessage() calls.Source: Coding guidelines
src/query.ts (1)
618-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe forced-failure cool-down re-arms once
nextRetryAtMsis cleared.This gate returns
falseunless bothlastForcedFailureAtMsandnextRetryAtMsare present. In the hard-cap case with many short messages, a cooled-down non-forced skip can clearnextRetryAtMswhile preservinglastForcedFailureAtMs; the next over-cap turn then restampsforceReason: 'message-count'before the forced-failure cool-down has elapsed, which recreates the retry storm.Suggested fix
const forcedCooldownActive = (() => { const lastForced = tracking?.lastForcedFailureAtMs const nextRetry = tracking?.nextRetryAtMs - if (typeof lastForced !== 'number' || typeof nextRetry !== 'number') { + if (typeof lastForced !== 'number' || !Number.isFinite(lastForced)) { return false } - if (!Number.isFinite(lastForced) || !Number.isFinite(nextRetry)) { - return false - } const now = Date.now() - if (now < nextRetry) { + if ( + typeof nextRetry === 'number' && + Number.isFinite(nextRetry) && + now < nextRetry + ) { return true } // Belt-and-suspenders: also honor the cool-down window from // the last forced attempt, in case nextRetryAtMs has been // cleared by a separate code path.🤖 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.ts` around lines 618 - 635, The forcedCooldownActive gate requires both lastForcedFailureAtMs and nextRetryAtMs to be valid numbers before checking any conditions, but when nextRetryAtMs is cleared by a separate code path, the function returns false immediately even though lastForcedFailureAtMs may still indicate an active cool-down. Fix this by allowing the belt-and-suspenders check (the final return statement comparing now - lastForced against getAutoCompactFailureCooldownMs()) to work independently when nextRetryAtMs is not present. Refactor the logic so that the function checks if we're within the cool-down window based on lastForcedFailureAtMs validity alone, and returns true if either the nextRetry check or the lastForced cool-down window check succeeds, rather than requiring both timestamps to be present before performing any checks.
🤖 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/services/compact/autoCompact.ts`:
- Around line 439-443: The forced compaction contract is incomplete: the early
return guard checking DISABLE_COMPACT at line 432 of autoCompact.ts still blocks
forced calls before forceReason reaches shouldAutoCompact(), contradicting the
documented behavior. In autoCompact.ts around line 432, modify the
DISABLE_COMPACT guard to only apply for non-forced calls by checking if
forceReason is not set before returning early. In autoCompact.test.ts around
lines 927-1047, add a unit test that verifies forced compaction bypasses the
DISABLE_COMPACT guard. In autoCompactCooldown.test.ts around lines 890-962, add
a corresponding end-to-end regression test with DISABLE_COMPACT=1 to ensure the
hard-cap memory-pressure safety net functions correctly when forced.
---
Outside diff comments:
In `@src/query.ts`:
- Around line 618-635: The forcedCooldownActive gate requires both
lastForcedFailureAtMs and nextRetryAtMs to be valid numbers before checking any
conditions, but when nextRetryAtMs is cleared by a separate code path, the
function returns false immediately even though lastForcedFailureAtMs may still
indicate an active cool-down. Fix this by allowing the belt-and-suspenders check
(the final return statement comparing now - lastForced against
getAutoCompactFailureCooldownMs()) to work independently when nextRetryAtMs is
not present. Refactor the logic so that the function checks if we're within the
cool-down window based on lastForcedFailureAtMs validity alone, and returns true
if either the nextRetry check or the lastForced cool-down window check succeeds,
rather than requiring both timestamps to be present before performing any
checks.
In `@src/query/autoCompactCooldown.test.ts`:
- Around line 606-878: The test suite for the autocompact cooldown regression
only covers scenarios with messages exceeding the token threshold (using
overAutoCompactThresholdMessage()), which exercises the token-threshold forcing
path. Add a new test case that uses short messages staying below the token
threshold to cover the hard-cap-only branch where a non-forced skip can
incorrectly clear nextRetryAtMs during the cooldown period. This test should
verify that when the hard-cap-only condition is active (no token threshold
exceeded) and a cooldown is in effect, the nextRetryAtMs field is preserved
across multiple turns, preventing the retry storm from re-arming. The test
should follow the pattern of the existing three test cases (hard-cap re-trigger
respects a recent forced-failure cool-down, hard-cap re-trigger fires again once
the cool-down has elapsed, and lastForcedFailureAtMs persists across non-forced
skip turns) but use short/normal-sized messages instead of
overAutoCompactThresholdMessage() calls.
🪄 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 Plus
Run ID: 37812e49-c12b-478c-887e-924e1706bbd6
📒 Files selected for processing (4)
src/query.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/query.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/query.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.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.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/query.tssrc/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.tssrc/services/compact/autoCompact.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/services/compact/autoCompact.test.tssrc/query/autoCompactCooldown.test.ts
The DISABLE_COMPACT early-return in autoCompactIfNeeded() ran before forceReason was resolved, so users who flipped DISABLE_COMPACT still lost the OOM safety net even after the previous fix bypassed isAutoCompactEnabled(). DISABLE_COMPACT is the stricter opt-out (it kills manual /compact too), but the hard cap and memory pressure are runtime guards — a user who set DISABLE_COMPACT did not opt out of OOM prevention. Resolve forcedBy first, then gate the DISABLE_COMPACT return on !forcedBy. Forced calls (message-count, memory-pressure) bypass it; non-forced calls still honor it for manual /compact and token- threshold auto-compact. Co-Authored-By: Claude <noreply@anthropic.com>
The cap-check cool-down gate at src/query.ts demanded BOTH lastForcedFailureAtMs AND nextRetryAtMs on the early-exit, so a non-forced skip on a tripped breaker (e.g. token-threshold path) that returned breaker metadata with no nextRetryAtMs would let the query loop `delete` the field — and the next over-cap turn would restamp forceReason: 'message-count' while the provider was still recovering, recreating the retry storm the breaker is supposed to contain. Refactor the IIFE so the belt-and-suspenders check on lastForcedFailureAtMs runs independently of nextRetryAtMs. Either check is enough to suppress the re-fire; the natural cap-check gate (now - lastForcedFailureAtMs < cooldownMs) is what eventually expires the cool-down. Add a regression test that exercises the cap-only branch (short messages, no token-threshold forcing) the other Gitlawb#1373 tests don't reach. Mock returns a non-forced skip with no nextRetryAtMs; the test asserts the cap-check gate held (no forceReason restamped) and the call model ran (max_turns, not blocking_limit). Co-Authored-By: Claude <noreply@anthropic.com>
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found an issue that still needs to be addressed.
Findings
- [P2] Keep memory-pressure compaction on the breaker cooldown
src/query.ts:675
The newlastForcedFailureAtMsgate only suppresses re-forcing for the message-count cap, but the memory-pressure path below it still unconditionally stampsforceReason: 'memory-pressure'wheneverconsumeCompactionRequest()returns true. That request is raised repeatedly while RSS remains elevated/critical (src/utils/memoryPressure.ts:128-134), andautoCompactIfNeeded()now lets any forced request bypass the tripped-breaker skip (src/services/compact/autoCompact.ts:500). So if a forced memory-pressure compaction fails, subsequent pressure checks can keep forcing compaction during the 5-minute cooldown instead of honoring the breaker, recreating the retry storm this PR carefully fixed formessage-count. Please apply the same forced-failure cooldown gate to memory-pressure requests, or otherwise suppress repeated pressure-forced compactions until the breaker cooldown expires.
The pressure monitor in src/utils/memoryPressure.ts keeps re-arming `compactionRequested` for as long as RSS stays elevated/critical — re-arming on every 30s tick. The previous round (Gitlawb#1373 follow-up) gated the message-count re-trigger on `forcedCooldownActive`, but the memory-pressure path at src/query.ts:675-680 still unconditionally stamped `forceReason: 'memory-pressure'` whenever `consumeCompactionRequest()` returned true. Combined with the earlier change that made `autoCompactIfNeeded` bypass the breaker's skip action for any forced call, a failing forced memory-pressure attempt would re-fire on every subsequent turn while the provider was still down — recreating the retry storm the breaker is supposed to contain. Hoist `forcedCooldownActive` out of the message-count block so the memory-pressure site can also reference it, and gate the `consumeCompactionRequest` body on the same flag. The two forced-reason sources share `lastForcedFailureAtMs` (only written on forced-attempt failures regardless of reason) and share the cool-down, so one symmetric gate is enough. Update the stale comment in memoryPressure.ts to point at the query-loop gate instead of the now-removed "one-shot prevents storms" claim. Add a regression test that stubs `consumeCompactionRequest` to fire unconditionally and asserts the second query turn's autocompact mock saw no `forceReason` — the gate held. Co-Authored-By: Claude <noreply@anthropic.com>
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)
src/query/autoCompactCooldown.test.ts (1)
48-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear cap env vars in
beforeEachto keep test baseline deterministic.
OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAPandOPENCLAUDE_MAX_ACTIVE_MESSAGESare restored inafterEach, but not reset inbeforeEach. Clearing both up front avoids host-environment influence on default-path tests.Suggested minimal change
delete process.env.DISABLE_AUTO_COMPACT delete process.env.DISABLE_COMPACT + delete process.env.OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP + delete process.env.OPENCLAUDE_MAX_ACTIVE_MESSAGES // Some tests below override this to a 60s window to drive theAs per coding guidelines, “Review tests for ... isolation of global/env/config state...”.
🤖 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 48 - 53, The test file needs to clear environment variables in the beforeEach hook to ensure test isolation and deterministic baselines. In addition to the OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS cleanup shown in the afterEach block, add cleanup for OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP and OPENCLAUDE_MAX_ACTIVE_MESSAGES in the beforeEach hook by deleting these process.env properties. This ensures each test starts with a clean state unaffected by host environment settings, consistent with the pattern already established for the cooldown environment variable.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.
Inline comments:
In `@src/query/autoCompactCooldown.test.ts`:
- Around line 1162-1164: The test mocks the consumeCompactionRequest function
but does not assert that it is actually called, allowing the test to pass
without exercising the memory-pressure request code path. Capture the mocked
consumeCompactionRequest function returned by mock.module in the test setup,
then add an assertion (such as assert.called or equivalent test framework
assertion) to verify that consumeCompactionRequest was invoked during the test
execution. Apply this same assertion pattern at all three locations where this
mock is set up: the anchor location at lines 1162-1164 and the sibling locations
at lines 1185-1186 and 1237-1238.
---
Outside diff comments:
In `@src/query/autoCompactCooldown.test.ts`:
- Around line 48-53: The test file needs to clear environment variables in the
beforeEach hook to ensure test isolation and deterministic baselines. In
addition to the OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS cleanup shown in the
afterEach block, add cleanup for OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP and
OPENCLAUDE_MAX_ACTIVE_MESSAGES in the beforeEach hook by deleting these
process.env properties. This ensures each test starts with a clean state
unaffected by host environment settings, consistent with the pattern already
established for the cooldown environment variable.
🪄 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 Plus
Run ID: 6c9f142f-a8ef-46da-9c94-1aea1b5ffe1d
📒 Files selected for processing (3)
src/query.tssrc/query/autoCompactCooldown.test.tssrc/utils/memoryPressure.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/utils/memoryPressure.tssrc/query.tssrc/query/autoCompactCooldown.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/utils/memoryPressure.tssrc/query.tssrc/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/utils/memoryPressure.tssrc/query.tssrc/query/autoCompactCooldown.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/memoryPressure.tssrc/query.tssrc/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.ts (1)
607-652: LGTM!Also applies to: 681-697, 793-833
src/utils/memoryPressure.ts (1)
128-134: LGTM!
The memory-pressure cool-down test stubs `consumeCompactionRequest` but never asserts it was invoked — meaning the test could pass even if the query loop stopped calling it (e.g. the import broke, the gate was moved to a different file, or the monitor's consult site was deleted). The autocompact assertions alone don't prove the gate was exercised. Capture the mock in a local const so we can assert against it, and add `expect(consumeCompactionRequest).toHaveBeenCalledTimes(1)` matching the single turn the test runs. CodeRabbit round 4 follow-up. Co-Authored-By: Claude <noreply@anthropic.com>
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
-
[P1] Complete CodeRabbit's request to remove the duplicate session-memory compact path
src/commands/compact/compact.ts:84
CodeRabbit's current review item is still valid: the new session-memory branch callstrySessionMemoryCompaction(...)at line 61 and then falls into the existing progress-wrapped attempt at line 90 instead of returning or closing the branch. The resulting control flow leaves the outertrystructurally unbalanced, sobun run buildfails withUnexpected catchat this file's outer catch andbun run typecheckreports parse errors in the same file. Please complete that review request by keeping a single session-memory compaction attempt, with the successful cleanup/breaker reset wired into that one path, so/compactbuilds and cannot compact twice. -
[P2] Keep subagent compactions from changing the main breaker-trip store
src/services/compact/autoCompact.ts:588
runPostCompactCleanup(querySource)deliberately skips main-thread module-level resets foragent:*sources because subagents share the same process and would otherwise corrupt the main session's state. The new breaker-trip store is also module-level, but the new success paths callclearBreakerTrippedState()unconditionally afterrunPostCompactCleanup(querySource), and the failure path callsrecordBreakerTripped()unconditionally. That means a subagent auto-compact success can clear the main session's "auto-compact paused" signal, or a subagent failure can make the main session look paused, even though no main-session compaction recovered or failed. Please key this state by query source/session, or apply the same main-thread guard used by the cleanup helper before recording or clearing it. -
[P3] Document that
offno longer disables every message-count compaction path
docs/advanced-setup.md:500
The user-facing docs still say that setting Message-count compaction tooffdisables the message-count trigger, and the settings summary says "Disabled message-count compaction". This PR adds a default-on hard message-count cap at 1000 messages, and the new tests show it still fires throughDISABLE_AUTO_COMPACT,DISABLE_COMPACT, andautoCompactEnabled: false; the only opt-out isOPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP=0. Please update the docs/settings copy so users understand thatoffonly disables the user-configured threshold, not the operator hard cap.
…rd cap Two follow-ups from issue Gitlawb#1373 review: 1. The breaker-trip store is module-level, but the new clear/record call sites in autoCompact.ts (lines 599, 637, 679) were unconditional. A subagent (agent:*) auto-compact success could silently clear the main session's 'auto-compact paused' signal, or a subagent failure could mark the main session tripped — even though no main-session compaction recovered or failed. Gate all three sites on the same isMainThreadCompact(querySource) predicate that runPostCompactCleanup uses, and export that predicate from postCompactCleanup.ts so the two stay in sync. 2. The user-facing docs and settings copy still said setting 'Message-count compaction' to off disables the message-count trigger. This PR adds a default-on hard cap (1000 messages) that bypasses DISABLE_AUTO_COMPACT, DISABLE_COMPACT, and autoCompactEnabled:false; the only opt-out is OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP=0. Update advanced-setup.md to describe the two independent layers (user-configured threshold + operator hard cap) and update the Config.tsx change-summary so users see 'Disabled user-configured message-count compaction (operator hard cap still applies)'. Adds 3 regression tests: subagent success does not clear a tripped main-session breaker, subagent failure does not record a trip on the main-session breaker, and a main-thread success still clears (regression guard for the new gate). All 148 tests in src/services/compact/, src/commands/compact/, src/query/ pass. Co-Authored-By: Claude <noreply@anthropic.com>
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and found an issue that still needs to be addressed.
Findings
-
[P1] Complete the duplicate session-memory compact cleanup
src/commands/compact/compact.ts:84
The current head still leaves the session-memory/compactbranch structurally broken: it callstrySessionMemoryCompaction(...)at line 61, enters the success branch without returning or closing that block, and then falls into the progress-wrapped second call at line 90. That keeps the earlier CodeRabbit/maintainer request unresolved and now blocks validation outright:bun run buildfails withUnexpected catchat the outer catch, andbun run typecheckreports parse errors in this file. Please keep a single session-memory compaction attempt, wire the breaker reset/cleanup into that successful path, and return before falling through to the traditional compaction path. -
[P2] Key breaker-trip state by session before exposing it
src/services/compact/compactWarningState.ts:44
The PR describes the new breaker-trip store as session-scoped and defers REPL/SDK display wiring to a follow-up, but the state added here is a single module-level store withrecordBreakerTripped(),getBreakerTripState(), andclearBreakerTrippedState()taking no session key. REPL already runs multiple main-thread sessions in one process and keepsautoCompactTrackingin a map keyed bygetSessionId(); background sessions also callquery()with the samerepl_main_threadquery source. With the current API, one main-thread session tripping the breaker will make the future UI/API read the trip for every session, and a successful compaction in another main-thread session will clear it. Please key this store by session ID (or thread the existing per-session tracking state through the future reader) before merging the follow-up-facing API.
…y breaker-trip store by session ID P1: The session-memory /compact branch was structurally broken from a rebase merge conflict — the first trySessionMemoryCompaction attempt (line 61) opened an if-block that was never closed, then a second progress-wrapped attempt (line 89) fell into it, producing a syntax error where a bare try had no matching catch/finally before the outer catch at line 166. This caused both 🔇 no-telemetry: stubbed 10 modules ✓ Built openclaude v0.18.0 → dist/cli.mjs Building SDK bundle... 🔇 no-telemetry: stubbed 10 modules ✓ Built SDK bundle → dist/sdk.mjs 🔄 feature-flags: transformed 193 files during bundling ✓ SDK bundle: no React/Ink leakage detected Validating external lists... ✓ CLI bundle: All dependencies accounted for (0 missing, 10 external) ✓ SDK bundle: All dependencies accounted for (0 missing, 14 external) ✓ INTENTIONALLY_BUNDLED: All entries still exist in package.json (58 entries) ✓ All external lists valid. ✓ SDK type declarations in sync (62 exports match). ✓ Bundle guard: no unexpected missing-module stubs (Unexpected catch) and (TS1005: 'try' expected) to fail. Fix: replace the entire malformed region with a single progress-wrapped session-memory attempt. On success it runs the full cleanup chain (runPostCompactCleanup + clearBreakerTrippedState + cache-break notify + markPostCompaction + suppressCompactWarning) and returns before the traditional compaction path. On null it falls through as before. P2: The breaker-trip store was a single module-level Store<BreakerTripState> with no session key. The REPL runs multiple main-thread sessions in one process and keys autoCompactTracking by getSessionId(); background sessions also call query() with the same repl_main_thread query source. With the old API, one session tripping the breaker would make every session look paused, and a successful compaction in another session would silently clear the first session's recovery signal. Fix: replace the single store with a Map<string, Store<BreakerTripState>> keyed by session ID. All three API functions take an explicit sessionId parameter: recordBreakerTripped(sessionId, args), getBreakerTripState (sessionId), clearBreakerTrippedState(sessionId). Added subscribeBreakerTripState(sessionId, listener) for the future REPL/SDK reader (deferred per PR scope), scoped per session. All callers updated: autoCompact.ts (3 sites), compact.ts (3 sites). Tests updated: autoCompact.test.ts (3 subagent-isolation tests), plus 4 new session-isolation tests in breakerTripReset.test.ts. Fixes Gitlawb#1373
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)
src/services/compact/autoCompact.test.ts (1)
1347-1373:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake breaker-store cleanup unconditional in this describe block.
These tests clear breaker state manually at the end of specific tests; if an assertion fails first, cleanup is skipped and state can leak across tests.
Suggested patch
describe('subagent (agent:*) compactions do not corrupt the main breaker-trip store', () => { beforeEach(() => { process.env.CLAUDE_AUTOCOMPACT_PCT_OVERRIDE = '1' process.env.OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS = '5000' }) + + afterEach(async () => { + const { clearBreakerTrippedState } = await import('./compactWarningState.js') + clearBreakerTrippedState(getSessionId()) + }) @@ - clearBreakerTrippedState(sessionId) @@ - clearBreakerTrippedState(sessionId)As per coding guidelines,
{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....Also applies to: 1434-1455
🤖 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/services/compact/autoCompact.test.ts` around lines 1347 - 1373, The test cleanup using clearBreakerTrippedState is currently called at the end of the test method, which means it will be skipped if an assertion fails beforehand, causing state to leak across tests. Move the clearBreakerTrippedState call from the end of this test into an afterEach hook at the describe block level so that the cleanup executes unconditionally after every test, ensuring breaker state is properly reset regardless of test outcomes.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.
Inline comments:
In `@src/services/compact/autoCompact.ts`:
- Around line 2-3: The file imports from the bootstrap/state module using two
different path specifiers: the absolute path 'src/bootstrap/state.js' for
markPostCompaction and the relative path '../../bootstrap/state.js' for
getSdkBetas and getSessionId. Consolidate these into a single import statement
using one consistent path specifier (preferably the relative path to match the
rest of the codebase), combining all imported symbols (markPostCompaction,
getSdkBetas, getSessionId) in a single import statement from bootstrap/state to
ensure they reference the same module instance.
In `@src/services/compact/breakerTripReset.test.ts`:
- Around line 167-207: Replace the dynamic require() calls for
subscribeBreakerTripState with a proper ESM import statement at the top of the
test file to align with ESM guidelines for src/**/*.ts files. Additionally, wrap
the unsubA() and unsubB() cleanup calls in a finally block to ensure
subscriptions are properly unsubscribed and module state is cleared after the
test completes, preventing listener leaks across subsequent test runs where the
module-level breakerTripStores Map would otherwise retain stale subscriptions.
---
Outside diff comments:
In `@src/services/compact/autoCompact.test.ts`:
- Around line 1347-1373: The test cleanup using clearBreakerTrippedState is
currently called at the end of the test method, which means it will be skipped
if an assertion fails beforehand, causing state to leak across tests. Move the
clearBreakerTrippedState call from the end of this test into an afterEach hook
at the describe block level so that the cleanup executes unconditionally after
every test, ensuring breaker state is properly reset regardless of test
outcomes.
🪄 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 Plus
Run ID: 7d278b0e-5e51-4206-a5eb-1c6212701ed2
📒 Files selected for processing (5)
src/commands/compact/compact.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/compactWarningState.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/compact/compactWarningState.tssrc/commands/compact/compact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/compact/compactWarningState.tssrc/commands/compact/compact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/compact/compactWarningState.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/compact/compactWarningState.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/compact/compactWarningState.tssrc/commands/compact/compact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/compact/compactWarningState.tssrc/commands/compact/compact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/compact/compactWarningState.tssrc/commands/compact/compact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.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/services/compact/compactWarningState.tssrc/commands/compact/compact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/services/compact/compactWarningState.tssrc/commands/compact/compact.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
{src/commands/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
commanderfor CLI argument parsing
Files:
src/commands/compact/compact.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
🔇 Additional comments (2)
src/services/compact/compactWarningState.ts (1)
21-35: LGTM!Also applies to: 54-75, 78-144
src/commands/compact/compact.ts (1)
3-3: LGTM!Also applies to: 76-80, 143-143, 230-230
…lative path autoCompact.ts imported markPostCompaction via absolute 'src/bootstrap/ state.js' and getSdkBetas/getSessionId via relative '../../bootstrap/ state.js'. Different path specifiers can produce duplicate module instances in Bun, breaking identity checks. Consolidate into a single relative import matching the file's existing convention.
jatmn
left a comment
There was a problem hiding this comment.
I found a runtime gap that still leaves part of the reported OOM path unbounded, plus one test cleanup item from the existing review thread.
Findings
-
[P2] Prune teammate history when hard-cap compaction yields a boundary
src/utils/swarm/inProcessRunner.ts:1244
Issue #1373 also calls out the in-process teammateallMessagesbuffer as an unbounded accumulator. This PR adds the hard cap insidequery.ts, andrunAgentrecords/yieldssystem compact_boundarymessages from that query path, but the teammate runner still appends every yielded message to its lifetimeallMessagesarray without applying the same compact-boundary pruning thatQueryEnginedoes. As a result, a teammate can trigger the new hard-cap compaction insidequery()while the runner keeps the entire pre-compact prefix, including full tool results, alive inallMessagesuntil a separate token-threshold compaction happens. Please pruneallMessages/task state when a compact boundary comes back fromrunAgent, or route this path through the same post-boundary replacement logic used by the explicit teammate compaction branch. -
[P3] Complete CodeRabbit's breaker-trip test cleanup requests
src/services/compact/breakerTripReset.test.ts:171
src/services/compact/autoCompact.test.ts:1372
CodeRabbit's cleanup request is still only partially addressed. The subscription test registersunsubA/unsubBbut only calls them after all assertions complete, so any assertion failure before the bottom of the test leaves those module-level breaker-trip listeners registered for the rest of the process. The subagent breaker tests also still seed the main session's breaker state and clear it only at the end of the test body, so an early failure can leave that state behind. Please complete that review request by moving the breaker-state cleanup into the describe-levelafterEachand wrapping the subscription test's unsubscribe calls in afinallyblock.
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/utils/swarm/inProcessRunner.ts`:
- Around line 1243-1249: The compact_boundary message handling in the block
checking message.subtype === 'compact_boundary' is using the wrong field name
and performing a destructive splice even when metadata is absent. Change
message.compactMetadata?.preservedPrefix to
message.compactMetadata?.preservedSegment to align with the compact-boundary
contract used in related code, and add a guard condition to only perform the
splice operation when preservedSegment is actually present, preventing the loss
of valid message history when the metadata is missing.
🪄 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 Plus
Run ID: 0cf3ab77-c67a-466a-8368-8b0fd35b14ab
📒 Files selected for processing (3)
src/services/compact/autoCompact.test.tssrc/services/compact/breakerTripReset.test.tssrc/utils/swarm/inProcessRunner.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
🔇 Additional comments (5)
src/services/compact/breakerTripReset.test.ts (1)
175-183: Dynamicrequire()still violates ESM guideline.The subscription leak is now fixed with try/finally cleanup—nice. However, the dynamic
require('./compactWarningState.js')at lines 177 and 181 still contradicts the ESM import guideline forsrc/**/*.tsfiles. AddsubscribeBreakerTripStateto the static import at line 7–11 and call it directly.Source: Coding guidelines
src/services/compact/autoCompact.test.ts (4)
31-32: LGTM!
1350-1377: LGTM!
1395-1432: LGTM!
1449-1471: LGTM!
c824dc2 to
0353f80
Compare
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/utils/swarm/inProcessRunner.ts`:
- Around line 1243-1249: The issue is in the inProcessRunner.ts file where the
code attempts to spread preservedSegment as if it were an array of messages, but
preservedSegment is actually a metadata object and not iterable, which causes a
runtime crash. In the compact_boundary message handler where allMessages.splice
is called, remove the spread operator and instead handle preservedSegment as the
object metadata it is. Verify what the correct property or structure is that
contains the actual messages array to prune, and either extract that array from
within the preservedSegment object or reassign it appropriately based on the
actual data structure returned by the QueryEngine's compaction logic.
🪄 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 Plus
Run ID: caaefc11-8fac-421f-85f6-5b6de5919877
📒 Files selected for processing (3)
src/services/compact/autoCompact.test.tssrc/services/compact/breakerTripReset.test.tssrc/utils/swarm/inProcessRunner.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/utils/swarm/inProcessRunner.tssrc/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.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/services/compact/breakerTripReset.test.tssrc/services/compact/autoCompact.test.ts
🔇 Additional comments (7)
src/services/compact/breakerTripReset.test.ts (3)
175-183: ⚡ Quick winDynamic
require()still violates ESM guidelines.The
finallyblock from the previous review was added—nice. But the dynamicrequire()calls remain. The comment claims "hoisting issues," butsubscribeBreakerTripStateis a plain export likeclearBreakerTrippedState/getBreakerTripState/recordBreakerTripped, all of which are imported statically at the top of this file without issue.Add
subscribeBreakerTripStateto the existing static import and remove the inlinerequire()calls.Source: Coding guidelines
1-166: LGTM!
214-218: LGTM!src/services/compact/autoCompact.test.ts (4)
31-33: LGTM!
1350-1377: LGTM!
1395-1432: LGTM!
1449-1471: LGTM!
Address runtime OOM concerns by ensuring inProcessRunner.prunes allMessages when receiving compact_boundary messages from runAgent. This prevents unbounded accumulation of messages while maintaining alignment with QueryEngine's compacted state. Also complete CodeRabbit's breaker-trip test cleanup requests by wrapping subscription logic and breaker state cleanup in try/finally blocks to ensure execution even when assertions fail early. Co-authored-by: openhands <openhands@all-hands.dev>
0353f80 to
9a120a4
Compare
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that need to be addressed before this is ready.
Findings
-
[P2] Prune teammate history for every compact boundary
src/utils/swarm/inProcessRunner.ts:1244
This only prunesallMessageswhen the yieldedcompact_boundaryhascompactMetadata.preservedSegment, but the normal auto-compact path throughcompactConversation()returns an unannotated boundary (nomessagesToKeep/ preserved segment). In that common hard-cap/full-compact case, the teammate keeps the entire pre-compactallMessagesbuffer, appends the boundary and summary, and then passes the bloated history back asforkContextMessageson the next iteration. That means the secondary OOM path from issue #1373 is still not actually bounded for regular teammate auto-compactions. Please reset the buffer on every compact boundary to mirror the post-compact message stream, not only the preserved-segment variant. -
[P3] Complete CodeRabbit's request to use the static ESM import
src/services/compact/breakerTripReset.test.ts:177
CodeRabbit's earlier review item asked forsubscribeBreakerTripStateto be added to the existing module-scope import, but the current patch still dynamically callsrequire('./compactWarningState.js')in this TypeScript test. Please complete that review request by importingsubscribeBreakerTripStatealongsideclearBreakerTrippedState,getBreakerTripState, andrecordBreakerTripped, then call it directly in the session-isolation test.
…ESM imports [P2] Reset teammate history for every compact boundary: - Prune allMessages on ALL compact boundaries, not just those with preservedSegment - This prevents unbounded accumulation in the common auto-compact path through compactConversation() - Mirrors the post-compact message stream for all hard-cap/full-compact cases [P3] Use static ESM import: - Add subscribeBreakerTripState to existing static import in breakerTripReset.test.ts - Replace dynamic require() calls with the imported function - Fixes CodeRabbit's review request for static ESM imports Co-authored-by: openhands <openhands@all-hands.dev>
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I found one issue that still needs to be addressed.
Findings
- [P2] Block over-cap low-token histories while forced compaction cools down
src/query.ts:979
The new hard-cap cooldown gate can deferforceReason: 'hard-message-count', but the follow-up blocking check still only considers token pressure (isAboveAutoCompactThreshold/tokenUsage >= getAutoCompactThreshold(model)). That leaves the exact low-token/high-message-count path this cap is meant to bound free to continue calling the model during cooldown after repeated forced failures. The new regression test demonstrates the problem: with two short messages, hard cap1, a recent forced failure, and a tripped breaker, the test expectsterminal.reasonto bemax_turnsrather thanblocking_limit(src/query/autoCompactCooldown.test.ts:1136). In a real long-running session with many small tool-result turns, this meansstate.messagescan keep growing while the provider is down, so the “absolute cap” no longer prevents the #1373 runaway path. Please include the active hard-cap-overrun/forced-cooldown state in the blocking condition, or otherwise stop before sending another over-cap request when the safety-net compaction is cooling down.
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 983-988: In the isInForcedCooldown constant definition, change the
field reference from `tracking?.lastFailureAtMs` to
`tracking?.lastForcedFailureAtMs`. These are distinct fields where
lastFailureAtMs tracks natural cooldown while lastForcedFailureAtMs tracks
forced-attempt cooldown. Using the wrong field breaks the forced-cooldown
blocking logic in the isInMessageCountCooldown check that follows, which
prevents retry storms on the message-count blocking path.
🪄 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 Plus
Run ID: d736696b-8921-4395-a634-cc51a07b9a8d
📒 Files selected for processing (1)
src/query.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/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.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/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.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.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.ts
[P2] Block over-cap requests during forced hard-cap cooldown: - Include forced cooldown check in the blocking condition - Prevent requests from being sent during hard-cap cooldown - Fix the low-token/high-message-count path that was previously unbounded - Updated error messages to distinguish message-count cooldowns The new hard-cap cooldown gate can defer forceReason: 'hard-message-count', but the follow-up blocking check still only considered token pressure. This left the exact low-token/high-message-count path free to continue calling the model during cooldown after repeated forced failures. The fix now blocks requests when in a forced cooldown state (regardless of token pressure), ensuring the absolute cap prevents the Gitlawb#1373 runaway path even when the provider is down. Co-authored-by: openhands <openhands@all-hands.dev>
caf86b4 to
553d92e
Compare
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I found issues that still need to be addressed.
Findings
-
[P2] Complete the over-cap cooldown block for low-token histories
src/query.ts:983
The current forced-cooldown block still depends ontracking.forceReason, but the realautoCompactIfNeeded()path consumes that flag before returning (src/services/compact/autoCompact.ts:450-455), and the cool-down deferral branch intentionally does not re-stamp it. That means after a forced hard-cap failure, the next low-token/high-message-count turn haslastForcedFailureAtMsandnextRetryAtMsbut noforceReason, soisInForcedCooldownis false andisAboveBreakerThresholdis also false for small messages. The new regression test captures the bad behavior by expectingterminal.reasonto bemax_turnsfor two short messages over a hard cap of 1 while forced cooldown is active (src/query/autoCompactCooldown.test.ts:1059-1145). In production that still sends another model request whilestate.messagesis over the absolute cap and compaction is cooling down, so the safety net does not prevent the #1373 runaway path for many small tool-result turns. Please complete the earlier review request by deriving the blocking condition from the over-active-cap plus forced-cooldown state, not from the one-shotforceReasonflag. -
[P3] Clear breaker-trip state after partial compact
src/screens/REPL.tsx:5047
This PR moved breaker-trip clearing out ofrunPostCompactCleanup(), so every successful compaction path now has to clear the newcompactWarningStatebreaker store explicitly. The message-selector partial compact path clears the oldautoCompactTrackingstate (setAutoCompactTrackingForSession(..., undefined)), then callsrunPostCompactCleanup(), but it never callsclearBreakerTrippedState(getSessionId()). After a user successfully summarizes via that path, the newly exposed breaker-trip state can remain stuck on "tripped" even though the session just recovered. Please clear the breaker-trip store alongside the existing tracking reset in this successful partial-compact path.
Fixes #1373.
What changed
src/services/compact/autoCompact.ts—autoCompactIfNeedednow honorsforceReasoneven when the breaker is in cool-down. The breaker still incrementsconsecutiveFailuresand re-trips on a forced failure, so retry-storm protection is preserved.src/services/compact/autoCompact.ts— newMAX_ACTIVE_MESSAGES_HARD_CAP = 1000constant +getMaxActiveMessagesHardCap()helper. Override viaOPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP;0disables.src/query.ts— the existing opt-inmaxMessagesCompactionThresholdcheck now also consults the new hard cap. Uses the tighter of the two.src/services/compact/compactWarningState.ts— newBreakerTripStatestore +recordBreakerTripped/getBreakerTripState/clearBreakerTrippedStateexports (REPL/SDK wiring deferred to a follow-up PR).src/services/compact/autoCompact.ts— callsrecordBreakerTripped()when the breaker trips.Why
Issue #1373 reports an OOM after ~3h of normal use. Root cause: a single summarization failure trips the breaker, which then blocks the only path that would shrink
state.messageswhile every new turn keeps appending. The user's stated expected behavior was:(1) is partly already implemented (the half-open breaker does re-attempt); the missing piece is honoring forced compaction through the breaker. (2) is the new hard cap.
Impact
maxMessagesCompactionThresholdusers see no behavior change for their setting.Checks
bun run typecheck— cleanbun run typecheck:type-tests— 10 files passbun run security:pr-scan— cleanbun run verify:privacy— all banned-pattern checks passbun run smoke— builds,0.18.0 (OpenClaude)bun test src/services/compact/— 72 pass (was 64, +8 new)bun test src/query/autoCompactCooldown.test.ts— 6 pass (was 5, +1 new)bun test src/QueryEngine.autoCompactCooldown.test.ts— 1 passbun run test:full— 14 pre-existing failures (verified identical with changes stashed); all are unrelated teammate-routing +/exportenv-dependent tests onmain.Follow-up
getBreakerTripState()into the REPL/SDK so users see "auto-compact paused" instead of silent failure. The store is in place.🤖 Generated with Claude Code
Summary by CodeRabbit
OPENCLAUDE_MAX_ACTIVE_MESSAGES_HARD_CAP, default 1000;0disables) that can trigger auto-compaction independently of user settings./compact.#1373, including hard-cap/cooldown interactions and session isolation/reset behavior.