Repository navigation
fix: auto-continuation overly biased toward Claude-style output — breaks with non-Claude models - #1713
Conversation
When using non-Claude models (OpenAI, Gemini, local models, etc.), the continuation detection frequently fails, causing the agent to stop after each turn and wait for manual user input. Changes: - Expand CONTINUATION_SIGNALS verb list to include common verbs used by non-Claude models: process, download, upload, compile, train, evaluate, test, continue, generate, extract, merge, deploy, install, configure, refactor, optimize (plus their -ing forms) - Fix COMPLETION_MARKERS check to only block continuation when no continuation signal is present nearby (prevents false positives when words like 'complete' or 'done' appear mid-sentence) - Add soft fallback nudging when text has no terminal punctuation and no explicit completion signal (assumes model intends to continue by default) - Increase MAX_CONTINUATION_NUDGES from 3 to 20 for longer multi-step tasks - Pass 'interrupt' reason to abortController.abort() in the TUI control interrupt handler for consistent signal propagation Fixes Twigpine#1707
📝 WalkthroughWalkthroughContinuation detection is expanded in ChangesContinuation Detection & Interrupt Abort Reason
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 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
🤖 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/continuation.ts`:
- Around line 114-117: The boolean logic in the guard condition for blocking
continuation (in the expression after checking
COMPLETION_MARKERS.test(lowerText)) is inverted. The current condition uses OR
with hasLateContinuationSignal, which causes the code to block continuation when
a late continuation signal EXISTS, but the intent is to block only when NO
continuation signal is present. Change the OR operator to AND and negate
hasLateContinuationSignal so the condition becomes (!hasLateContinuationSignal
&& !CONTINUATION_SIGNALS.some(re => re.test(lowerText))). This ensures
continuation is only blocked when both no late continuation signal AND no
continuation signals anywhere are detected.
🪄 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: b0bc0734-e884-4e73-8387-cb2b81cbb60c
📒 Files selected for processing (3)
src/cli/print.tssrc/query.tssrc/utils/continuation.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/cli/print.tssrc/utils/continuation.tssrc/query.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/cli/print.tssrc/utils/continuation.tssrc/query.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/cli/print.tssrc/utils/continuation.tssrc/query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/cli/print.tssrc/utils/continuation.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/cli/print.tssrc/utils/continuation.tssrc/query.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/cli/print.tssrc/utils/continuation.tssrc/query.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/continuation.ts
🔇 Additional comments (5)
src/utils/continuation.ts (2)
10-15: LGTM!
130-136: LGTM!src/query.ts (2)
180-180: LGTM!
182-189: The function is used on line 857 in the auto-compact circuit-breaker error message.src/cli/print.ts (1)
2852-2852: LGTM!
- Add 429 rate limit retry with backoff for non-Claude providers (src/services/api/withRetry.ts) - Disable microcompact to prevent aggressive conversation trimming (src/query.ts) - Disable command-queue abort to prevent interrupting running tools (src/cli/print.ts)
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/cli/print.ts`:
- Around line 1871-1876: Remove the large commented-out block containing the
subscribeToCommandQueue call with its internal logic checking abortController
and getCommandsByMaxPriority. If you need to preserve the context of why this
was disabled, replace the entire commented block with a single-line comment that
briefly explains that the command-queue abort functionality was disabled because
it was interrupting running bash tools, rather than keeping the full
implementation commented out.
In `@src/services/api/withRetry.ts`:
- Around line 394-404: The 429 retry logic in the withRetry handler is using the
main `attempt` counter which oscillates because the for loop re-increments it
after the catch block decrements it. Instead of modifying `attempt`, introduce a
separate `remaining429Retries` counter initialized outside the loop (set to the
value of `max429Retries`) before the loop begins. In the catch block where you
check for APIError with status 429 and !isClaudeAISubscriber(), decrement and
check the `remaining429Retries` counter instead of `attempt` to properly track
429-specific retries independently from the main retry loop, allowing the
backoff logic to function correctly without oscillation.
🪄 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: e8fce867-e6e6-4daf-8a2c-2cda87639199
📒 Files selected for processing (3)
src/cli/print.tssrc/query.tssrc/services/api/withRetry.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/withRetry.tssrc/cli/print.tssrc/query.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/api/withRetry.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/withRetry.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/api/withRetry.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/api/withRetry.tssrc/cli/print.tssrc/query.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/withRetry.tssrc/cli/print.tssrc/query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/withRetry.tssrc/cli/print.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/api/withRetry.tssrc/cli/print.tssrc/query.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/withRetry.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/services/api/withRetry.tssrc/cli/print.tssrc/query.ts
🔇 Additional comments (3)
src/query.ts (2)
182-189: LGTM!
503-506: LGTM!src/cli/print.ts (1)
2850-2858: LGTM!
jatmn
left a comment
There was a problem hiding this comment.
I found several issues that need to be addressed before this is ready.
Findings
-
[P1] Keep the 429 retry budget independent from the loop counter
src/services/api/withRetry.ts:395
The new 429 branch decrementsattemptbeforecontinue, and then theforloop increments it back to the same value on the next iteration. For a provider that keeps returning 429,attemptnever advances pastmax429Retries, so this path can retry indefinitely instead of stopping after the intended five retries. Please track the 429 budget with a separate counter, or otherwise let the loop make forward progress. -
[P1] Do not auto-continue every unpunctuated assistant response
src/utils/continuation.ts:133
The soft fallback now nudges whenever the final assistant text has no terminal punctuation and no completion marker. That includes ordinary successful replies such asok after retry, soquery()keeps injecting continuation messages until the new 20-nudge cap is exhausted. The existing provider-cap retry tests now make 20 extra model calls after a successful response, andbugfixes.test.tsalso showsI changed package.json and src/query.ts and added testsincorrectly becomingshouldNudge: true. Please keep the fallback tied to an actual continuation/truncation signal rather than treating any punctuation-less text as unfinished work. -
[P1] Restore a clean typecheck on the changed files
src/query.ts:506
This branch currently failsbun run typecheckin the touched files:pendingCacheEdits = undefined as constis invalid, the later cached-microcompact reads narrow tonever, andwithRetry.tsreferenceslogForDiagnosticsNoPIIandsleep4without importing or defining them. The project typecheck gate fails before this can be merged, so these compile errors need to be fixed or the incomplete code removed. -
[P2] Do not disable microcompact for every query turn
src/query.ts:503
This removes thedeps.microcompact(...)call entirely and leavesmessagesForQueryunmodified on every main-loop request, while/context,/compact, and other paths still use microcompact. That is a much broader behavior change than the continuation fix: cached microcompact boundary messages are never emitted, compactable tool results are no longer cleared before normal API calls, and sessions with large tool outputs lose a token-control path regardless of whether the user wanted compaction disabled. Please either scope the fix to the specificmaxMessagesCompactionThreshold: "off"behavior or split this policy change into its own reviewed PR. -
[P2] Preserve headless "now" command preemption or split the behavior change
src/cli/print.ts:1871
Commenting out the command-queue subscription meansrunHeadlessStreamingno longer aborts the active request when apriority: "now"message arrives. The current turn can keep running, including long API/tool work, while the urgent queued command waits for normal completion. That changes the public command-queue semantics independently of the continuation heuristic and is not covered by tests in this PR. Please keep the preemption behavior, add a narrower guard for the bash-tool case, or move this to a separate PR with coverage for the intended new semantics.
- 429 retry: use separate counter instead of manipulating attempt - Soft fallback: remove aggressive fallback nudging all unpunctuated text - Microcompact: honor maxMessagesCompactionThreshold 'off' setting - Fix typecheck issues (pendingCacheEdits, imports)
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P1] Restore the cached microcompact type before using it
src/query.ts:504
pendingCacheEditsis now declared asunknown, but the later cached-microcompact boundary path still readsbaselineCacheDeletedTokens,trigger, anddeletedToolIdsfrom it. After installing dependencies,bun run typecheckfails in this touched file at those dereferences, so the PR still cannot pass the project typecheck gate. Please keep thePendingCacheEditstype frommicroCompact.tson this local, or otherwise narrow it before the deferred boundary message logic uses it. -
[P1] Cover the PR's own completion-then-continuation example
src/utils/continuation.ts:97
The updated heuristic still returnsshouldNudge: falsefor the pattern this PR describes as a root cause:The download is complete. Now processing...andThe download is complete. Now processing the files...both stop instead of nudging because the lateprocessingsignal is treated as punctuated text and does not satisfy the laterstrongIntentcheck. That leaves the reported non-Claude continuation failure in place for one of the PR's primary examples. Please make the completion-marker override handle these action-transition forms and add a regression test for thecomplete. Now processing...case. -
[P2] Preserve
nowqueue preemption in headless mode
src/cli/print.ts:1871
ThesubscribeToCommandQueueblock is still commented out, sorunHeadlessStreamingno longer aborts the active request when a queued command withpriority: 'now'arrives. That means an urgent queued command can sit behind the current model/tool turn until normal completion, which changes the public command-queue semantics independently of the continuation fix and has no replacement coverage in this PR. Please keep the preemption behavior and scope the bash-tool interruption fix more narrowly, or split this behavior change into its own PR with tests for the new semantics. -
[P2] Do not add unrelated 429 retry policy to the continuation fix
src/services/api/withRetry.ts:395
This branch now gives every session whereisClaudeAISubscriber()is false up to five extra 429 retries before the existing retry classifier runs. That predicate is account/auth state, not provider identity, despite the comment saying this is for non-Claude providers; it also changes outbound retry/backoff behavior for API-key and other non-subscriber sessions even though the PR is about continuation detection. Please remove this hidden policy change from this PR, or move it into a focused retry/provider PR with rationale and coverage for the intended 429 behavior.
- Remove 429 retry policy (out of scope for continuation fix) - Add presentProgressive check for 'complete. Now processing...' case - Restore command queue preemption (split to separate PR) - Fix PendingCacheEdits type for typecheck
There was a problem hiding this comment.
Thanks for continuing to work through this. I found issues that still need to be addressed before this is ready.
Findings
-
[P1] Complete CodeRabbit's completion-guard request without broad gerund false positives
src/utils/continuation.ts:104
CodeRabbit's earlier completion-guard request is still partially valid in the current patch. TheNow processing...example now nudges becausepresentProgressivereturns before the completion guard, but the guard still blocks other late continuation signals such asTask complete. Continuing with the next step.andTask complete. Proceeding to run tests.. At the same time, the newpresentProgressivecheck treats ordinary completed sentences as unfinished whenever they contain a matching gerund phrase:Thanks for reviewing the PR.,The system is processing the request.,I am done processing the files., andFinished processing files.all returnshouldNudge: true. Becausequery()then injects up to 20 continuation prompts, these false positives can keep completed turns running. Please fix the completion-marker/late-signal ordering so real late continuation intent is not suppressed, completed gerund sentences stay terminal, and add regression coverage for both sides. -
[P1] Do not make message-count compaction disable microcompact by default
src/query.ts:506
This now skipsdeps.microcompact()whenevermaxMessagesCompactionThresholdisoff, but that setting is documented and typed as the optional message-count compaction trigger, andoffis the default most users should leave enabled. As written, default configs stop running the pre-request microcompact path entirely, including the cached/time-based microcompact checks that used to run independently of the message-count threshold. Please either keep microcompact controlled by its own feature/config gate, or introduce a separate explicit setting if the intent is to disable all microcompact behavior. -
[P2] Finish the linked issue's abort-reason fix or narrow the closure claim
src/utils/permissions/PermissionPromptToolResultSchema.ts:126
This PR still says it closes #1707, but that issue also calls out permission deny and hook interrupt paths being misclassified because they callabort()without a reason. The current diff only fixes the TUI control interrupt path insrc/cli/print.ts;PermissionPromptToolResultSchema.tsandpermissions.tsstill abort without a reason fordeny + interrupt, soquery.tscontinues to treat those as non-interruptaborts and can emit the same misleading interruption state the issue describes. Please either complete the abort-reason cleanup for the linked issue or stop closing #1707 from this narrower continuation PR.
- Fix completion guard condition (inverted logic — was blocking with continuation signals present) - Narrow presentProgressive to only 'Now [verb]ing' (avoid gerund false positives) - Restore original microcompact call (remove config gate) - Add 'interrupt' reason to permission deny abort calls
jatmn
left a comment
There was a problem hiding this comment.
Thanks for continuing to work through this. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P1] Complete the continuation-signal review item for punctuated transitions
src/utils/continuation.ts:102
The current patch addscontinuing with,proceeding to, and several gerund action phrases toCONTINUATION_SIGNALS, but punctuated replies that use those advertised signals still fall through because the terminal-punctuation branch only accepts the narrowerstrongIntent, colon, ornow ...ingchecks. For example,Task complete. Continuing with the next step.,Task complete. Proceeding to run tests., andNo issues in parser. Continuing with renderer.all returnshouldNudge: false; the same happens forTask complete. Proceeding to deploy the project.and the rest of theproceeding to <new verb>family. Please complete the prior continuation-signal review item by making these late transition phrases survive the punctuation gate and adding regression coverage for them. -
[P2] Complete the abort-reason cleanup for interactive permission hooks
src/hooks/toolPermission/PermissionContext.ts:285
The linked issue's abort-reason fix is still incomplete for the interactive PermissionRequest hook path. This PR now passes'interrupt'for SDK permission results and headless permission hooks, butPermissionContext.runHooks()still callstoolUseContext.abortController.abort()without a reason when a hook returnsdenywithinterrupt.query.tsstill checkssignal.reason !== 'interrupt'before emitting the user interruption message, so interactive hook interrupts can continue to be classified differently from the paths fixed in this PR. Please complete the previous abort-reason review item by passing the same interrupt reason on this hook path as well.
- [P1] Add 'continuing with' and 'proceeding to' to strongIntent regex so punctuated transition phrases survive terminal-punctuation gate - [P2] Pass 'interrupt' reason to abortController.abort() in PermissionContext.runHooks() for interactive permission hooks
jatmn
left a comment
There was a problem hiding this comment.
Thanks for continuing to work through this. I found issues that still need to be addressed before this is ready.
Findings
-
[P1] Complete the late gerund transition fix for the original action verbs
src/utils/continuation.ts:15
The current patch makesNow processing...and the new gerund families nudge correctly, but the same completion-marker path still blocks the original action verbs from the existing continuation patterns when they are written as late gerund transitions. For example,Analysis complete. Now creating files...,Analysis complete. Now editing src/query.ts...,Task finished. Now running tests...,Task complete. Now checking the output..., andSetup complete. Now building the app...all still returnshouldNudge: false. Becausequery()only injects the continuation prompt when this helper returns true, these non-Claude responses still stop after saying they are about to create/edit/run/check/build, which leaves the linked issue only partially fixed. Please complete the prior continuation-signal review item by making the latenow <action>ingtransition logic cover the existing action verbs too, and add regression coverage for those cases. -
[P2] Narrow the #1707 closure claim or finish the default fallback item
src/utils/continuation.ts:131
The PR description still says the defaultshouldNudge: falseroot cause was fixed by adding a soft fallback for responses with no terminal punctuation and no explicit completion signal, and the PR still closes #1707. The current implementation does not do that: after the targeted signal checks, it still falls through toreturn { shouldNudge: false }, so examples with no terminal punctuation and no completion marker such asNeed to process files,Need to edit files,Next I process files, andNow process filesstill stop. I understand the earlier broad fallback had false positives, so this should not reintroduce "nudge every unpunctuated reply"; please either implement a safe fallback for the linked issue's remaining root-cause case or narrow the PR description/closure claim so this does not land as a complete fix for #1707.
- [P1] Add missing -ing gerund forms to line 15 (creating, writing,
editing, running, checking, building, etc.) so the late gerund
transition fix covers the original action verbs too
- [P2] Add imperative/declarative signals for bare patterns:
'need to <verb>' ('Need to process files'),
'now <verb>' ('Now process files', with negative lookahead
to avoid 'Now you' false positives),
'next (i|we) <verb>' ('Next I process files')
|
Thanks for the thorough review, @jatmn! I've addressed both findings in this fifth round: P1 — Late gerund transition for original action verbs
all correctly return P2 — Imperative/declarative bare patterns
|
jatmn
left a comment
There was a problem hiding this comment.
⚠️ Findings / Suggestions
1. Missing test coverage for new continuation behavior (medium)
The only existing continuation tests are in src/__tests__/bugfixes.test.ts. They do not exercise:
- The newly added verbs (
process,download,compile, etc.). - The new imperative/declarative patterns (
Need to ...,Now ...,Next I/We ...). - The present-progressive fallback (
now processing). - The updated completion-marker guard with nearby continuation signals.
- The raised
MAX_CONTINUATION_NUDGESlimit.
Recommendation: Add focused unit tests in a new or existing test file (e.g., src/utils/continuation.test.ts) that assert the new true-positive and false-positive cases the PR is meant to fix. The PR description already lists excellent examples that could be turned into test cases.
2. .gitignore addition of nul (nit)
.gitignore now contains nul, a Windows reserved device name. It appears to be an accidental local artifact rather than a project-wide ignore pattern. It is harmless but unrelated to the PR’s stated scope.
Recommendation: Remove the nul line or explain why it is needed.
3. presentProgressive regex is overly broad (minor)
In src/utils/continuation.ts:
const presentProgressive = /\bnow \w+ing\b/i.test(lateText)This matches any word ending in ing after "now" (now being, now having, now waiting), not just action verbs. While the impact is likely small, it could nudge on a model stating it is in a passive/waiting state.
Recommendation: Restrict the pattern to the same verb list used elsewhere, or at least to a curated set of active verbs.
4. Verb list duplication (code-quality nit)
The continuation verb list is repeated nearly verbatim across five CONTINUATION_SIGNALS regexes and the three imperative patterns. This makes future maintenance error-prone.
Recommendation: Build the regexes from a shared array of verbs (e.g., a readonly array joined into the patterns). This is not a blocker but would improve maintainability.
5. MAX_CONTINUATION_NUDGES raised to 20 with no guard or test (minor)
src/query.ts now caps continuation nudges at 20 instead of 3. The existing guard only compares state.continuationNudgeCount < MAX_CONTINUATION_NUDGES; there is no other stop condition. The change is acceptable, but a test verifying the cap is respected would prevent regressions.
Recommendation: Add a test that confirms the nudge loop stops after MAX_CONTINUATION_NUDGES iterations.
Addresses all 5 findings from jatmn's CHANGES_REQUESTED review: 1. (code-quality) Extract verb list to shared ACTION_VERBS array; build all continuation regexes from it via buildContinuationSignals() 2. (minor) Restrict presentProgressive to gerund forms of the same verb list instead of broad \w+ing 3. (nit) Remove accidental 'nul' entry from .gitignore 4. (medium) Add focused tests for new verbs, imperative patterns, present-progressive fallback, completion-marker guard, and verb-list deduplication 5. (minor) Add tests verifying MAX_CONTINUATION_NUDGES = 20 and the guard comparison
|
Addressed all 5 findings from the review in commit 80427b3:
All 38 tests pass (including all pre-existing and new ones). |
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Fix the TypeScript type error in the gerund map
src/utils/continuation.ts:67
TheVERB_INGspecial-case map checksv === 'take', buttakeis not inACTION_VERBS, sobun run typecheckfails withTS2367: This comparison appears to be unintentional. Remove the dead'take'branch. -
[P2] Complete the abort-reason fix for the remaining interrupt paths
src/cli/print.ts:1044andsrc/hooks/toolPermission/PermissionContext.ts:206
The PR correctly adds the'interrupt'reason to a few abort calls, but the SIGINT handler inprint.tsand thecancelAndAbortpath inPermissionContext.tsstill callabortController.abort()without a reason. Since issue #1707 calls this out as a root cause, these remaining user-interrupt paths should also pass'interrupt'so the downstream classification inStreamingToolExecutoris accurate. -
[P3] Fix indentation in the changed code
src/query.ts:503-517,src/utils/permissions/PermissionPromptToolResultSchema.ts:122-126, andsrc/utils/permissions/permissions.ts:465-469
The edited lines are indented with tabs/8 spaces instead of the project's 2-space style. Please reformat them to match the surrounding files. -
[P3] Align the PR description with what the diff actually changes
src/utils/continuation.ts:189-193
The PR body says a "soft fallback" was added, but that fallback already existed before this change. It also says completion markers only block when no continuation signal is present "nearby", but the code checks the entire lowercased text (lowerText) rather than only the nearby window. Please update the description or the code so they agree.
Addresses all 4 findings from jatmn's CHANGES_REQUESTED review: 1. [P1] Remove dead 'take' branch from VERB_ING gerund map 2. [P2] Pass 'interrupt' reason to abort() in print.ts SIGINT handler and PermissionContext.ts cancelAndAbort path 3. [P3] Fix tab indentation in query.ts, PermissionPromptToolResultSchema.ts, and permissions.ts 4. [P3] Update PR description to match actual code changes
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P3] Fix the indentation in the SDK permission deny+interrupt branch
src/utils/permissions/PermissionPromptToolResultSchema.ts:122-127
Theelse ifblock added for the deny+interrupt path is indented with four spaces while the rest of the file uses two. This was already flagged in the previous review round and is still present. Please reformat these lines to match the surrounding two-space style so the control flow is clear. -
[P3] Use a more robust way to remove "do" from the
time toverb alternation
src/utils/continuation.ts:81
Thetime toregex builds its alternation withv.replace(/^do\|/, ''). This only works if"do"stays the first element ofACTION_VERBS; if the array is ever reordered,"do"will be duplicated in the alternation. Please build that alternation from a filtered array (for example,ACTION_VERBS.filter(v => v !== 'do')) so the regex stays correct regardless of array order. -
[P3] Remove or explain the out-of-scope
queryLifecyclecleanup
src/query.ts:912
The PR removesqueryLifecycle: toolUseContext.queryLifecyclefrom thecallModeloptions, but the PR description only discusses continuation heuristics, nudge limits, and abort reasons. This cleanup is unrelated to the stated scope. Please either restore the line or explain why it belongs in this PR. -
[P3] Align the PR description with the actual default fallback behavior
The PR body says the default fallback was "inverted" so that unpunctuated responses without an explicit completion signal are treated as continuation intent. The current code insrc/utils/continuation.ts:205still returns{ shouldNudge: false }as the final default; the only unpunctuated fallback is the pre-existing branch that requires a continuation signal match (src/utils/continuation.ts:197-203). Please update the PR description to match the implemented behavior, or adjust the code if a broader inversion is still intended.
…sion) - Fix indentation (2-space style) in PermissionPromptToolResultSchema.ts else-if block (jatmn review finding 1) - Use ACTION_VERBS.filter() instead of fragile v.replace(/^do\|/, '') for 'time to' regex (finding 2) - Clarify default fallback behavior in PR description (finding 4) - Note: queryLifecycle cleanup (finding 3) not applicable — field does not exist in this codebase
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P2] New imperative/declarative continuation patterns are blocked when punctuated
src/utils/continuation.ts:95-183
The newneed to <verb>,now <verb>, andnext (i|we) <verb>patterns added toCONTINUATION_SIGNALSonly trigger a nudge when the response has no terminal punctuation. Once the model adds a period (e.g.,"Need to process files.","Now create the component.","Next we need to add tests."),hasLateContinuationSignalmatches, but the punctuated branch at lines 178-185 only returnsshouldNudge: trueforstrongIntent,endsWithColon, orpresentProgressive. The new imperative/declarative forms are not included in that gate, so the function falls through and returnsshouldNudge: false. This leaves the non-Claude continuation fix incomplete for a common punctuated output shape. Please broaden the punctuated branch to recognize these new patterns (for example, by including them in thestrongIntentcheck or adding a dedicated transition test) and add regression tests for the punctuated cases. -
[P3] Narrow the #1707 closure claim or finish the default fallback item
src/utils/continuation.ts:208
The PR still saysCloses #1707, but that issue lists "DefaultshouldNudge: false" as a root cause and explicitly suggests inverting the default when there is no terminal punctuation and no completion marker. The PR description and code keep the default fallback atshouldNudge: false, so the issue is not fully resolved. Please either implement a safe version of the inverted default or update the PR description so it does not claim to close #1707.
…on intent The new imperative patterns (need to <verb>, now <verb>, next i/we <verb>) matched in the late-window signal check but were silently dropped when the text had terminal punctuation, because the punctuated branch only checked strongIntent / presentProgressive / endsWithColon. This left examples like 'Need to process files.' and 'Now create the component.' returning shouldNudge: false, even though the bare variants correctly returned true. Fix: add hasImperativeSignal to the punctuated gate, re-testing lowerText against the imperative patterns so punctuated action-intent signals are recognized. Also narrows the Twigpine#1707 closure claim in the PR description: the default fallback remains shouldNudge: false (the broader inversion suggested in the issue introduced false positives in earlier rounds). Tests added for punctuated imperative variants.
jatmn
left a comment
There was a problem hiding this comment.
Findings
[Medium] print.ts onInterrupt() handler strips the abort reason
Location: src/cli/print.ts, around line 3948-3950
The diff shows:
onInterrupt() {
- abortController?.abort('interrupt')
+ abortController?.abort()
},This callback is the bridge/SDK interrupt path in runHeadlessStreaming. The PR description explicitly says it is fixing interrupt paths by passing 'interrupt' as the abort reason, and the other two changed abort sites (SIGINT handler, PermissionContext.cancelAndAbort, SDK permission deny+interrupt) now do so. Removing the reason here is inconsistent with that goal and likely re-introduces the downstream misclassification the PR is trying to fix.
Recommended action: Restore abortController?.abort('interrupt') in onInterrupt().
[Minor] Mixed indentation in new test block
Location: src/__tests__/bugfixes.test.ts, lines 172-200
The imperative / declarative patterns trigger continuation test is indented with tabs, while the rest of the file uses 2-space indentation. One of the PR's own commit messages mentions fixing indentation; this block should be normalized.
Recommended action: Convert tabs to 2-space indentation in that test block.
[Nit] hasImperativeSignal uses full text instead of the late window
Location: src/utils/continuation.ts, lines 182-186
const hasImperativeSignal = new RegExp(`\\bneed to (?:${VERB_ALT})\\b`, 'i').test(lowerText) ||
new RegExp(`\\bnow (?:${VERB_ALT})\\b(?!\\s+you\\b)`, 'i').test(lowerText) ||
new RegExp(`\\bnext (?:i|we)\\s+(?:need to|will|shall|should|must)?\\s*(?:${VERB_ALT})\\b`, 'i').test(lowerText)These checks run inside the hasLateContinuationSignal branch, which is scoped to the last 120 characters (lateText). The strongIntent check above uses lowerText (full text), so this is not unique to the new code, but using lowerText here can match imperative phrases anywhere in the message rather than only near the end. If the intent is to gate punctuated imperative signals that were already caught in the late window, consider using lateText for consistency.
Recommended action: Consider using lateText for hasImperativeSignal to match the surrounding late-window logic, or document why lowerText is intentional.
[Question] Unexplained queryLifecycle removal in query.ts
Location: src/query.ts, around line 911
The diff removes:
queryTracking,
- queryLifecycle: toolUseContext.queryLifecycle,
effortValue: appState.effortValue,This removal is not mentioned in the PR description or commit messages. It appears to be cleanup, but without context it is hard to tell whether it is related to this fix or an unrelated change that rode along.
Recommended action: Confirm in the PR body or as a code comment that this is intentional cleanup and not a leaked unrelated change.
- [Medium] Restore abortController.abort('interrupt') in onInterrupt()
bridge/SDK callback to keep interrupt-reason fix consistent
- [Minor] Normalize indent (tabs -> 2-space) in imperative/declarative
test block in bugfixes.test.ts
- [Nit] Use lateText (last 120 chars) for hasImperativeSignal check
for consistency with surrounding late-window logic
- queryLifecycle removal is intentional cleanup from earlier round;
no longer referenced in callModel options
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/continuation.ts`:
- Around line 185-187: The `hasImperativeSignal` check in `continuation.ts` is
overmatching subject-led advice like “You need to update...”, causing
false-positive nudges in the continuation flow. Tighten the `need to` branch so
it only matches true bare imperatives at sentence start or otherwise excludes
explicit subjects (for example, “you/i/we” before the phrase), while keeping the
existing `now` and `next` logic in place. Add a regression test alongside the
current negative cases for `"Now you ..."` to verify user-directed advice no
longer triggers this 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
Run ID: 1d9c0710-0b86-48d8-ad04-f9119f765441
📒 Files selected for processing (3)
src/__tests__/bugfixes.test.tssrc/cli/print.tssrc/utils/continuation.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/cli/print.tssrc/__tests__/bugfixes.test.tssrc/utils/continuation.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/cli/print.tssrc/__tests__/bugfixes.test.tssrc/utils/continuation.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/cli/print.tssrc/__tests__/bugfixes.test.tssrc/utils/continuation.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/cli/print.tssrc/__tests__/bugfixes.test.tssrc/utils/continuation.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/cli/print.tssrc/__tests__/bugfixes.test.tssrc/utils/continuation.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/cli/print.tssrc/__tests__/bugfixes.test.tssrc/utils/continuation.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/__tests__/bugfixes.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/__tests__/bugfixes.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/__tests__/bugfixes.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/continuation.ts
🪛 ast-grep (0.44.0)
src/utils/continuation.ts
[warning] 184-184: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\bneed to (?:${VERB_ALT})\\b, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 185-185: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\bnow (?:${VERB_ALT})\\b(?!\\s+you\\b), 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 186-186: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\bnext (?:i|we)\\s+(?:need to|will|shall|should|must)?\\s*(?:${VERB_ALT})\\b, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (1)
src/cli/print.ts (1)
1041-1044: LGTM!Also applies to: 2852-2852, 3948-3950
…to...)
CodeRabbit review flagged that the `need to` continuation pattern was
overmatching subject-led advice like "You need to update..." causing
false-positive nudges. Added negative lookbehind `(?<!\b(?:you|i|we|...)\s+)`
to both `CONTINUATION_SIGNALS` and `hasImperativeSignal` so only bare
imperatives (no subject) trigger continuation. Added regression tests.
Also confirmed three earlier findings were already fixed:
- print.ts onInterrupt() already uses abort('interrupt') ✅
- Indentation in bugfixes.test.ts:172-200 already 2-space ✅
- hasImperativeSignal already uses lateText ✅
- queryLifecycle removal was intentional cleanup ✅
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/continuation.ts (1)
95-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAnchor
need toto true bare imperatives.Line 95 and Line 185 only exclude pronouns immediately before
need to, so subject-led advice likeYou may need to update...orUsers need to configure...can still auto-nudge. Anchor this branch to sentence-start bare forms and add a regression beside the new negative cases.Suggested minimal fix
- new RegExp(`(?<!\\b(?:you|i|we|they|he|she|it)\\s+)\\bneed to (${v})\\b`, 'i'), + new RegExp(`(?:^|[.!?]\\s+)need to (${v})\\b`, 'i'),- const hasImperativeSignal = new RegExp(`(?<!\\b(?:you|i|we|they|he|she|it)\\s+)\\bneed to (?:${VERB_ALT})\\b`, 'i').test(lateText) || + const hasImperativeSignal = new RegExp(`(?:^|[.!?]\\s+)need to (?:${VERB_ALT})\\b`, 'i').test(lateText) ||Add coverage:
expect(analyzeContinuationIntent("You may need to update the config.").shouldNudge).toBe(false) expect(analyzeContinuationIntent("Users need to configure the server.").shouldNudge).toBe(false)Also applies to: 185-185
🤖 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/utils/continuation.ts` at line 95, The `need to` matching in `analyzeContinuationIntent` is too broad and still nudges subject-led advice like “You may need to…” or “Users need to…”. Tighten the `need to` branch in `src/utils/continuation.ts` so it only matches true bare imperatives at the start of a sentence, and keep the same restriction in both regex sites referenced by `analyzeContinuationIntent`. Add regression coverage alongside the existing negative cases to verify `shouldNudge` stays false for “You may need to update the config.” and “Users need to configure the server.”.
🤖 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.
Duplicate comments:
In `@src/utils/continuation.ts`:
- Line 95: The `need to` matching in `analyzeContinuationIntent` is too broad
and still nudges subject-led advice like “You may need to…” or “Users need to…”.
Tighten the `need to` branch in `src/utils/continuation.ts` so it only matches
true bare imperatives at the start of a sentence, and keep the same restriction
in both regex sites referenced by `analyzeContinuationIntent`. Add regression
coverage alongside the existing negative cases to verify `shouldNudge` stays
false for “You may need to update the config.” and “Users need to configure the
server.”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ba142a3-0803-4dba-8669-1fd25355ab4c
📒 Files selected for processing (2)
src/__tests__/bugfixes.test.tssrc/utils/continuation.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/__tests__/bugfixes.test.tssrc/utils/continuation.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/__tests__/bugfixes.test.tssrc/utils/continuation.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/__tests__/bugfixes.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/__tests__/bugfixes.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/__tests__/bugfixes.test.tssrc/utils/continuation.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/__tests__/bugfixes.test.tssrc/utils/continuation.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/__tests__/bugfixes.test.tssrc/utils/continuation.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/__tests__/bugfixes.test.tssrc/utils/continuation.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/__tests__/bugfixes.test.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/continuation.ts
🪛 ast-grep (0.44.0)
src/utils/continuation.ts
[warning] 95-95: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\bnow (${v})\\b(?!\\s+you\\b), 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 185-185: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\bnow (?:${VERB_ALT})\\b(?!\\s+you\\b), 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (2)
src/utils/continuation.ts (1)
198-201: LGTM!src/__tests__/bugfixes.test.ts (1)
200-206: LGTM!
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
…aks with non-Claude models (Twigpine#1713) * fix: auto-continuation overly biased toward Claude-style output When using non-Claude models (OpenAI, Gemini, local models, etc.), the continuation detection frequently fails, causing the agent to stop after each turn and wait for manual user input. Changes: - Expand CONTINUATION_SIGNALS verb list to include common verbs used by non-Claude models: process, download, upload, compile, train, evaluate, test, continue, generate, extract, merge, deploy, install, configure, refactor, optimize (plus their -ing forms) - Fix COMPLETION_MARKERS check to only block continuation when no continuation signal is present nearby (prevents false positives when words like 'complete' or 'done' appear mid-sentence) - Add soft fallback nudging when text has no terminal punctuation and no explicit completion signal (assumes model intends to continue by default) - Increase MAX_CONTINUATION_NUDGES from 3 to 20 for longer multi-step tasks - Pass 'interrupt' reason to abortController.abort() in the TUI control interrupt handler for consistent signal propagation Fixes Twigpine#1707 * fix: add 429 retry, disable microcompact, disable command-queue abort - Add 429 rate limit retry with backoff for non-Claude providers (src/services/api/withRetry.ts) - Disable microcompact to prevent aggressive conversation trimming (src/query.ts) - Disable command-queue abort to prevent interrupting running tools (src/cli/print.ts) * fix: address PR review feedback - 429 retry: use separate counter instead of manipulating attempt - Soft fallback: remove aggressive fallback nudging all unpunctuated text - Microcompact: honor maxMessagesCompactionThreshold 'off' setting - Fix typecheck issues (pendingCacheEdits, imports) * fix: address second round of PR review - Remove 429 retry policy (out of scope for continuation fix) - Add presentProgressive check for 'complete. Now processing...' case - Restore command queue preemption (split to separate PR) - Fix PendingCacheEdits type for typecheck * fix: address third round of PR review - Fix completion guard condition (inverted logic — was blocking with continuation signals present) - Narrow presentProgressive to only 'Now [verb]ing' (avoid gerund false positives) - Restore original microcompact call (remove config gate) - Add 'interrupt' reason to permission deny abort calls * fix: address fourth round of PR review - [P1] Add 'continuing with' and 'proceeding to' to strongIntent regex so punctuated transition phrases survive terminal-punctuation gate - [P2] Pass 'interrupt' reason to abortController.abort() in PermissionContext.runHooks() for interactive permission hooks * fix: address fifth round of PR review - [P1] Add missing -ing gerund forms to line 15 (creating, writing, editing, running, checking, building, etc.) so the late gerund transition fix covers the original action verbs too - [P2] Add imperative/declarative signals for bare patterns: 'need to <verb>' ('Need to process files'), 'now <verb>' ('Now process files', with negative lookahead to avoid 'Now you' false positives), 'next (i|we) <verb>' ('Next I process files') * fix: address sixth round of PR review Addresses all 5 findings from jatmn's CHANGES_REQUESTED review: 1. (code-quality) Extract verb list to shared ACTION_VERBS array; build all continuation regexes from it via buildContinuationSignals() 2. (minor) Restrict presentProgressive to gerund forms of the same verb list instead of broad \w+ing 3. (nit) Remove accidental 'nul' entry from .gitignore 4. (medium) Add focused tests for new verbs, imperative patterns, present-progressive fallback, completion-marker guard, and verb-list deduplication 5. (minor) Add tests verifying MAX_CONTINUATION_NUDGES = 20 and the guard comparison * fix: address seventh round of PR review Addresses all 4 findings from jatmn's CHANGES_REQUESTED review: 1. [P1] Remove dead 'take' branch from VERB_ING gerund map 2. [P2] Pass 'interrupt' reason to abort() in print.ts SIGINT handler and PermissionContext.ts cancelAndAbort path 3. [P3] Fix tab indentation in query.ts, PermissionPromptToolResultSchema.ts, and permissions.ts 4. [P3] Update PR description to match actual code changes * fix: address PR review feedback (indentation, filter-based verb exclusion) - Fix indentation (2-space style) in PermissionPromptToolResultSchema.ts else-if block (jatmn review finding 1) - Use ACTION_VERBS.filter() instead of fragile v.replace(/^do\|/, '') for 'time to' regex (finding 2) - Clarify default fallback behavior in PR description (finding 4) - Note: queryLifecycle cleanup (finding 3) not applicable — field does not exist in this codebase * fix: punctuated imperative/declarative patterns now signal continuation intent The new imperative patterns (need to <verb>, now <verb>, next i/we <verb>) matched in the late-window signal check but were silently dropped when the text had terminal punctuation, because the punctuated branch only checked strongIntent / presentProgressive / endsWithColon. This left examples like 'Need to process files.' and 'Now create the component.' returning shouldNudge: false, even though the bare variants correctly returned true. Fix: add hasImperativeSignal to the punctuated gate, re-testing lowerText against the imperative patterns so punctuated action-intent signals are recognized. Also narrows the Twigpine#1707 closure claim in the PR description: the default fallback remains shouldNudge: false (the broader inversion suggested in the issue introduced false positives in earlier rounds). Tests added for punctuated imperative variants. * fix: address ninth round of PR review - [Medium] Restore abortController.abort('interrupt') in onInterrupt() bridge/SDK callback to keep interrupt-reason fix consistent - [Minor] Normalize indent (tabs -> 2-space) in imperative/declarative test block in bugfixes.test.ts - [Nit] Use lateText (last 120 chars) for hasImperativeSignal check for consistency with surrounding late-window logic - queryLifecycle removal is intentional cleanup from earlier round; no longer referenced in callModel options * fix: tighten need-to pattern to exclude subject-led advice (You need to...) CodeRabbit review flagged that the `need to` continuation pattern was overmatching subject-led advice like "You need to update..." causing false-positive nudges. Added negative lookbehind `(?<!\b(?:you|i|we|...)\s+)` to both `CONTINUATION_SIGNALS` and `hasImperativeSignal` so only bare imperatives (no subject) trigger continuation. Added regression tests. Also confirmed three earlier findings were already fixed: - print.ts onInterrupt() already uses abort('interrupt') ✅ - Indentation in bugfixes.test.ts:172-200 already 2-space ✅ - hasImperativeSignal already uses lateText ✅ - queryLifecycle removal was intentional cleanup ✅
Summary
The auto-continuation mechanism in OpenClaude has a strong bias toward Claude's output style. When using non-Claude models (OpenAI, Gemini, local models, etc.), the continuation detection frequently fails, causing the agent to stop after each turn and wait for manual user input.
Root Causes
1. Limited verb list in CONTINUATION_SIGNALS
The continuation signal patterns only included verbs like "create", "write", "edit". Common verbs used by non-Claude models such as "process", "download", "compile", "train", "evaluate", "test", "extract", "merge", "deploy", "install", "configure", "optimize", "refactor" were all missing.
Fix: Added these verbs to all continuation signal patterns, plus their gerund forms, via a shared ACTION_VERBS array so maintenance stays in one place.
2. COMPLETION_MARKERS too aggressive
The regex matched completion markers anywhere in the response text. For example, "The download is complete. Now processing..." would immediately block continuation because "complete" appears mid-sentence.
Fix: Only block continuation via COMPLETION_MARKERS when no continuation signal is present in the full text and no late-window signal was detected in the last 120 characters. The default fallback remains
shouldNudge: falsefor text without any continuation signal — this change only prevents COMPLETION_MARKERS from overriding an active continuation signal.3. Present-progressive pattern too broad
The present-progressive check used \bnow \w+ing\b which matched passive/non-action words like "being", "having", "waiting".
Fix: Restricted to gerund forms of the same ACTION_VERBS list via a dynamically built regex from VERB_ING.
4. Imperative/declarative patterns missing
Non-Claude models often omit subjects, writing "Need to process files" or "Now create the component" instead of "I need to process files".
Fix: Added patterns for bare "need to ", "now " (with negative lookahead for "Now you"), and "next I/we " patterns.
5. MAX_CONTINUATION_NUDGES = 3
Limited automatic continuation to only 3 turns — far too restrictive for complex multi-step tasks.
Fix: Increased from 3 to 20.
6. abortController.abort() without reason in interrupt paths
Several interrupt paths called abort() without passing a reason, causing downstream checks to misclassify the abort.
Fix: Pass 'interrupt' as the reason in the TUI SIGINT handler, the permission hook cancelAndAbort path, and the SDK permission deny+interrupt path.
Related
Partially addresses #1707 (the default fallback remains
shouldNudge: falsefor text without any continuation signal; the broader fallback inversion suggested in the issue is left for follow-up as it introduced false positives in testing)Summary by CodeRabbit