Repository navigation
fix(api): quiet expected side-task aborts - #1868
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (4)src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughTwo superseded abort reasons are added and carried through shared abort classification, query lifecycle mapping, tool-failure guarding, Claude retry handling, and cancellation flows for agent summary and memory extraction. ChangesSuperseded abort reason support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/api/claude.ts`:
- Around line 2837-2846: The streaming and fallback catch paths repeat the same
short-circuit flow, so extract that logic into a shared helper to prevent
divergence. Add a helper around getClaudeExpectedSideTaskApiAbortLogMessage, the
conditional logForDebugging call, releaseStreamResources, and the early return,
then replace both duplicated blocks in the Claude API handling code with that
helper. Keep the helper near the existing error-handling logic so the call sites
stay clear.
In `@src/services/extractMemories/extractMemories.ts`:
- Around line 508-522: The catch block in extractMemories should not treat every
aborted signal as expected cancellation; it needs an abort-specific error check
like the sibling handlers use. Update the error branch in extractMemories to
only return early when extractionAbortController.signal is aborted and the
caught error is actually an abort-related error, otherwise keep logging and
emitting tengu_extract_memories_error so real failures are not swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3122d03-b268-4f9a-a13c-4a7305edbb40
📒 Files selected for processing (14)
src/query/toolFailureLoopGuard.test.tssrc/query/toolFailureLoopGuard.tssrc/services/AgentSummary/agentSummary.test.tssrc/services/AgentSummary/agentSummary.tssrc/services/api/claude.abortClassification.test.tssrc/services/api/claude.tssrc/services/api/withRetry.test.tssrc/services/api/withRetry.tssrc/services/extractMemories/extractMemories.abort.test.tssrc/services/extractMemories/extractMemories.tssrc/utils/abortReasons.test.tssrc/utils/abortReasons.tssrc/utils/queryLifecycle.test.tssrc/utils/queryLifecycle.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: PR Checks / smoke-and-tests: fix(api): quiet expected side-task aborts
Conclusion: failure
e > allows exactly one half-open retry after cooldown expires [6.00ms]
(pass) resolveAutoCompactCircuitBreakerState > derives active cooldown from failure time when retry time is absent [2.00ms]
(pass) resolveAutoCompactCircuitBreakerState > derives active cooldown from failure time when retry time is NaN [1.00ms]
(pass) resolveAutoCompactCircuitBreakerState > derives active cooldown from failure time when retry time is Infinity [1.00ms]
(pass) resolveAutoCompactCircuitBreakerState > uses explicit retry time before deriving cooldown from failure time [1.00ms]
(pass) resolveAutoCompactCircuitBreakerState > allows half-open retry after derived cooldown expires [1.00ms]
(pass) autoCompactIfNeeded circuit breaker > trips after three non-user failures and records a retry time [3.00ms]
(pass) autoCompactIfNeeded circuit breaker > active cooldown skips compaction attempts [8.00ms]
(pass) autoCompactIfNeeded circuit breaker > expired cooldown allows a half-open compaction attempt [2.00ms]
(pass) autoCompactIfNeeded circuit breaker > half-open failure immediately re-trips instead of growing unbounded [2.00ms]
(pass) autoCompactIfNeeded circuit breaker > failed compaction cooldown starts at failure time, not attempt start [1.00ms]
(pass) autoCompactIfNeeded circuit breaker > user abort does not increment failures or trip cooldown [1.00ms]
(pass) autoCompactIfNeeded circuit breaker > user abort during half-open retry clears expired cooldown without retripping [2.00ms]
(pass) autoCompactIfNeeded circuit breaker > below-threshold conversations clear stale breaker state [7.00ms]
##[endgroup]
##[group]src/services/compact/snipCompact.test.ts:
(pass) isSnipRuntimeEnabled > returns true [5.00ms]
(pass) SNIP_NUDGE_TEXT > is a non-empty string mentioning snip
(pass) snipCompactIfNeeded > no-ops when nothing is pending
(pass) snipCompactIfNeeded > removes a message whose short ID was marked for snip [1.00ms]
(pass) snipCompactIfNeeded > returns a boundary message...
GitHub Actions: PR Checks / 2_smoke-and-tests.txt: fix(api): quiet expected side-task aborts
Conclusion: failure
ace [2.00ms]
(pass) repairs truncated structured Bash JSON in streaming responses [1.00ms]
(pass) does not normalize incomplete streamed Bash commands when finish_reason is length [1.00ms]
(pass) repairs truncated JSON objects even without command field [2.00ms]
(pass) preserves raw input for unknown plain string tool arguments [1.00ms]
(pass) preserves parsed string input for unknown JSON string tool arguments [1.00ms]
(pass) sanitizes malformed MCP tool schemas before sending them to OpenAI [2.00ms]
(pass) optional tool properties are not added to required[] — fixes Groq/Azure 400 tool_use_failed [1.00ms]
(pass) coalesces consecutive user messages to avoid alternation errors (issue `#202`) [2.00ms]
(pass) coalesces consecutive assistant messages preserving tool_calls (issue `#202`) [1.00ms]
(pass) non-streaming: reasoning_content emitted as thinking block only when content is null [1.00ms]
(pass) non-streaming: empty string content does not fall through to reasoning_content as text [1.00ms]
(pass) non-streaming: real content takes precedence over reasoning_content [1.00ms]
(pass) non-streaming: preserves response body when usage parsing fails [1.00ms]
(pass) non-streaming: preserves response.url routing metadata after body read [2.00ms]
(pass) non-streaming: strips <think> tag block from assistant content [1.00ms]
(pass) streaming: thinking block closed before tool call [1.00ms]
(pass) streaming: strips <think> tag block from assistant content deltas [1.00ms]
(pass) streaming: strips <think> tag split across multiple content chunks [2.00ms]
(pass) streaming: preserves prose without tags (no phrase-based false positive) [1.00ms]
(pass) strips credentials and query params from URL in fetch network error message [1.00ms]
(pass) classifies localhost transport failures with actionable category marker [2.00ms]
(pass) transport failures are not labeled with HTTP status 503 [1.00ms]
(pass) propagates AbortError without wrapping it as transport failur...
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Use TypeScript with strict mode
Use ESM imports in TypeScript source files
Files:
src/utils/queryLifecycle.test.tssrc/services/AgentSummary/agentSummary.tssrc/utils/abortReasons.test.tssrc/utils/queryLifecycle.tssrc/services/AgentSummary/agentSummary.test.tssrc/query/toolFailureLoopGuard.tssrc/services/api/claude.abortClassification.test.tssrc/services/api/withRetry.tssrc/services/extractMemories/extractMemories.abort.test.tssrc/query/toolFailureLoopGuard.test.tssrc/services/api/withRetry.test.tssrc/utils/abortReasons.tssrc/services/api/claude.tssrc/services/extractMemories/extractMemories.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.
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/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/utils/queryLifecycle.test.tssrc/services/AgentSummary/agentSummary.tssrc/utils/abortReasons.test.tssrc/utils/queryLifecycle.tssrc/services/AgentSummary/agentSummary.test.tssrc/query/toolFailureLoopGuard.tssrc/services/api/claude.abortClassification.test.tssrc/services/api/withRetry.tssrc/services/extractMemories/extractMemories.abort.test.tssrc/query/toolFailureLoopGuard.test.tssrc/services/api/withRetry.test.tssrc/utils/abortReasons.tssrc/services/api/claude.tssrc/services/extractMemories/extractMemories.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/queryLifecycle.test.tssrc/services/AgentSummary/agentSummary.tssrc/utils/abortReasons.test.tssrc/utils/queryLifecycle.tssrc/services/AgentSummary/agentSummary.test.tssrc/query/toolFailureLoopGuard.tssrc/services/api/claude.abortClassification.test.tssrc/services/api/withRetry.tssrc/services/extractMemories/extractMemories.abort.test.tssrc/query/toolFailureLoopGuard.test.tssrc/services/api/withRetry.test.tssrc/utils/abortReasons.tssrc/services/api/claude.tssrc/services/extractMemories/extractMemories.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/utils/queryLifecycle.test.tssrc/utils/abortReasons.test.tssrc/services/AgentSummary/agentSummary.test.tssrc/services/api/claude.abortClassification.test.tssrc/services/extractMemories/extractMemories.abort.test.tssrc/query/toolFailureLoopGuard.test.tssrc/services/api/withRetry.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/claude.abortClassification.test.tssrc/services/api/withRetry.tssrc/services/api/withRetry.test.tssrc/services/api/claude.ts
🔇 Additional comments (14)
src/utils/abortReasons.ts (1)
8-22: LGTM!Also applies to: 66-76, 87-90, 121-124, 148-150, 172-175
src/utils/abortReasons.test.ts (1)
23-28: LGTM!Also applies to: 99-114, 131-136, 152-153, 186-187
src/query/toolFailureLoopGuard.ts (1)
16-17: LGTM!src/query/toolFailureLoopGuard.test.ts (1)
182-183: LGTM!Also applies to: 232-248
src/utils/queryLifecycle.ts (1)
87-88: LGTM!src/utils/queryLifecycle.test.ts (1)
31-46: LGTM!Also applies to: 111-122
src/services/api/withRetry.ts (1)
10-13: LGTM!Also applies to: 284-293
src/services/api/withRetry.test.ts (1)
3-5: LGTM!Also applies to: 21-21, 57-57, 79-93, 192-292
src/services/api/claude.ts (1)
62-63: LGTM!Also applies to: 817-833
src/services/api/claude.abortClassification.test.ts (1)
2-8: LGTM!Also applies to: 47-68, 85-117
src/services/AgentSummary/agentSummary.ts (1)
27-27: LGTM!Also applies to: 164-173
src/services/AgentSummary/agentSummary.test.ts (1)
1-99: LGTM!src/services/extractMemories/extractMemories.ts (1)
44-44: LGTM!Also applies to: 65-67, 319-321, 397-399, 409-409, 437-437, 523-526, 590-592
src/services/extractMemories/extractMemories.abort.test.ts (1)
1-175: LGTM!
jatmn
left a comment
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] Do not advance the memory cursor after a superseded extraction abort
src/services/extractMemories/extractMemories.ts:441
When a second extraction arrives, the current run is aborted withmemory-extraction-superseded, but the production fork path can now resolve normally:queryModelshort-circuits the expected abort,query()returns an aborted terminal state, andrunForkedAgent()ignores that generator return value and returns the collected messages.runExtraction()then continues pastawait runForkedAgent(...)and advanceslastMemoryMessageUuidfor the stale context even though the extraction was cancelled and wrote no memories. The trailing extraction computes itsnewMessageCountfrom that advanced cursor, so messages covered only by the aborted run can be skipped permanently. Please detect the superseded/aborted controller before advancing the cursor, or make the forked extraction surface the abort so the existing catch path handles it.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
3 participants
Summary
Impact
Testing
bun test src/services/AgentSummary/agentSummary.test.tsbun test src/services/AgentSummary/agentSummary.test.ts src/services/extractMemories/extractMemories.abort.test.ts src/utils/abortReasons.test.ts src/utils/queryLifecycle.test.ts src/services/api/claude.abortClassification.test.ts src/services/api/withRetry.test.ts src/services/tools/StreamingToolExecutor.test.ts src/services/tools/toolExecution.test.ts tests/sdk/query-lifecycle.test.ts src/query/stopHooks.goal.test.ts src/query/toolFailureLoopGuard.test.tsbun run typecheckbun run buildbun run smokegit diff --checkbun run check(not run locally; includes deadcode and the full single-concurrency test suite)Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests