Repository navigation
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 (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughUpdates the shared tool-result marker, OpenAI shim boundary injection, streamed assistant handling, continuation nudging, terminal reporting, and regression coverage for marker and stop-hook behavior. ChangesTool-result marker flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 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/query.ts`:
- Around line 1433-1440: Add or update tests for the new continuation path in
query handling: verify that the exact “[Tool results received]” text in
message.content is treated like tool_use and triggers follow-up execution. Cover
the affected provider/model path used by the continuation logic in src/query.ts,
and assert behavior both when msgToolUseBlocks is present and when only the
marker is present.
- Around line 1435-1439: The marker detection in query.ts is too brittle because
hasToolResultsMarker relies on an exact text equality check, so make it
resilient by normalizing the text (for example trimming and using a
case-insensitive or includes-style comparison) when scanning
message.message.content. Also avoid duplicating the "[Tool results received]"
literal across query.ts and openaiShim.ts by extracting it into a shared
constant in a common module and referencing that constant from both locations.
🪄 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: d05e6991-2eb8-4b00-9646-771a0d82d55f
📒 Files selected for processing (1)
src/query.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports in source and test files.
Files:
src/query.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
In the files you touch, preserve the existing code style.
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.
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/query.ts
| // Treat '[Tool results received]' placeholder as a synonym for tool_use | ||
| // to ensure execution continues when using self-hosted llama-server | ||
| const hasToolResultsMarker = message.message.content.some( | ||
| content => | ||
| content.type === 'text' && | ||
| content.text === '[Tool results received]', | ||
| ) | ||
| if (msgToolUseBlocks.length > 0 || hasToolResultsMarker) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing test coverage for the new continuation marker.
This changes follow-up execution behavior for the exact [Tool results received] placeholder path, but no test changes are included. The path instructions call for testing this exact continuation marker behavior against the affected provider/model path.
As per path instructions, "add or update tests for the exact continuation marker behavior ([Tool results received]) and the affected provider/model path when possible."
🤖 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 1433 - 1440, Add or update tests for the new
continuation path in query handling: verify that the exact “[Tool results
received]” text in message.content is treated like tool_use and triggers
follow-up execution. Cover the affected provider/model path used by the
continuation logic in src/query.ts, and assert behavior both when
msgToolUseBlocks is present and when only the marker is present.
Source: Path instructions
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Guard marker-only follow-ups so they cannot recurse without progress
src/query.ts:1440
When the marker is present without anytool_useblocks, this now setsneedsFollowUp = truewhile still pushing onlymsgToolUseBlocks, so the continuation branch runs withtoolUseBlocks.length === 0. That branch produces no tool results, appends only the placeholder assistant message tostate.messages, and immediately starts another model call; if the local provider emits the same[Tool results received]placeholder again, the loop repeats with no new user/tool-result progress. This also bypasses the capped continuation-nudge path because theif (!needsFollowUp)block never runs, and top-level callers may not providemaxTurns, so a stuck provider can keep burning turns/API calls instead of terminating. Please do not flipneedsFollowUpfor marker-only responses unless the path also proves progress or is capped like the existing continuation nudge. -
[P2] Reuse the capped post-tool stall design from the earlier self-hosted compat PR
src/query.ts:1433
The same author previously opened #1730 for the same self-hosted auto-continue problem, and that closed PR handled[Tool results received]as a stalled post-tool response: it stripped/withheld placeholder assistant messages and routed recovery through the capped continuation nudge mechanism. This PR reimplements only a small slice of that behavior by treating the placeholder as atool_usesynonym, which both creates the empty-tool loop above and prevents the existing nudge block from running. Please align this fix with the earlier capped placeholder-stripping approach, or otherwise explain why this divergent uncapped trigger is safe for the same provider failure mode. -
[P2] Complete CodeRabbit's request to cover marker-only continuation
src/query.ts:1435
This changes the query loop so a plain text[Tool results received]assistant block setsneedsFollowUpeven whenmsgToolUseBlocksis empty, which sends the turn through the tool-execution/next-turn path without any actual tool blocks. The existing tests cover the OpenAI shim injecting that text into provider requests, but there is no query-loop regression test proving that this new marker-only response triggers exactly the intended follow-up behavior and does not accidentally terminate the turn or recurse incorrectly. Please add focused coverage for both the existing realtool_usepath and the marker-only path before relying on this provider workaround. -
[P3] Share the tool-result marker literal instead of duplicating it
src/query.ts:1438
The exact[Tool results received]string is now hard-coded here and separately insrc/services/api/openaiShim.tswhere the shim injects the semantic assistant boundary. If the boundary text changes in one place but not the other, the response-side continuation check silently stops matching the shim's own marker. Please extract a shared constant and use it from both the injection and detection paths.
0xghost42
left a comment
0xghost42
left a comment
There was a problem hiding this comment.
I hit this same class of problem working on continuation detection for non-Claude providers, so I get what this is solving: llama-server doesn't emit real tool_use blocks, so the loop never sets needsFollowUp and execution stalls. The intent is right. Two things about this specific approach worry me though.
The one I'd want resolved before merge is the follow-up loop. On the marker path, needsFollowUp = true is set but nothing is pushed into toolUseBlocks (there are no tool_use blocks to push). So the next turn continues with an empty tool-use set. If the self-hosted server emits [Tool results received] again on that follow-up (or on every assistant turn), hasToolResultsMarker stays true and needsFollowUp is re-armed every iteration with no actual tool work happening — that's a non-terminating continuation. What guarantees the marker stops appearing, or otherwise breaks the cycle? A safe version would only treat the marker as a continuation signal when the previous turn actually dispatched tools (so a genuine tool round-trip is in flight), or cap consecutive marker-only follow-ups, so a chatty server can't spin the loop.
Second, the match is an exact string equality on content.text === '[Tool results received]'. That's brittle in both directions: any surrounding whitespace, a trailing newline, or the model folding the marker into a larger text block makes it silently miss; and conversely a legitimate message that happens to contain exactly that text (a user pasting logs, the model quoting the marker back) would trigger a spurious follow-up. At minimum I'd pull the literal into a named constant and match with a trim, but see the last point.
More structurally — is queryLoop the right layer for this? Pattern-matching a provider-specific sentinel string in the core query loop means the loop now carries knowledge of one particular shim's output format, and the next provider that stalls the same way needs another special-case here. The cleaner boundary is usually the shim/adapter that produces [Tool results received] in the first place: have it surface real tool_use blocks (or a normalized continuation flag) so queryLoop keeps reasoning about tool_use blocks only. If that's not feasible from the adapter side, a short comment explaining why the sentinel has to be interpreted here would help the next person.
None of this is to say the need isn't real — just that a raw string check that arms needsFollowUp without a termination guard is the part I'd tighten first.
…elf-hosted LLMs - Share TOOL_RESULTS_RECEIVED_MARKER constant between shim and query - Strip marker-only assistant responses (shim artifacts) before yield to UI - Force continuation nudge on marker-only stall (self-hosted echo) - Force continuation nudge when stop_hook_active (goal/hook blocking errors) - Add test coverage for marker handling behavior
|
Sorry for the delay in replying. I’ve fixed the algorithm for resuming operations, and it seems to be working now. I tested it during a very long, unattended run on a large task, and there were no unexpected stops. |
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 1481-1516: Update the marker-only handling near
hasToolResultsMarker so it computes the remaining text after removing
markerPattern and sets withheld and markerOnlyStall only when no substantive
text remains and msgToolUseBlocks is empty. Preserve marker stripping for stored
content, but keep messages containing real continuation text visible. Add a
regression test in toolResultsMarker.test.ts covering marker text followed by
real continuation text without tool_use blocks.
🪄 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: 748ba201-43db-4c28-a683-cbe0841272d1
📒 Files selected for processing (5)
.gitignoresrc/constants/messages.tssrc/query.tssrc/query/toolResultsMarker.test.tssrc/services/api/openaiShim.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/constants/messages.tssrc/query/toolResultsMarker.test.tssrc/query.tssrc/services/api/openaiShim.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/constants/messages.tssrc/query/toolResultsMarker.test.tssrc/query.tssrc/services/api/openaiShim.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/toolResultsMarker.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/openaiShim.ts
🪛 ast-grep (0.44.1)
src/query.ts
[warning] 1475-1479: 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(
\\s*${TOOL_RESULTS_RECEIVED_MARKER .replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*,
'g',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (6)
src/constants/messages.ts (1)
2-10: LGTM!src/services/api/openaiShim.ts (1)
115-115: LGTM!Also applies to: 1378-1392
src/query.ts (2)
68-68: LGTM!Also applies to: 999-1003
2335-2356: LGTM!src/query/toolResultsMarker.test.ts (1)
1-275: LGTM!.gitignore (1)
28-28: LGTM!
Apply CodeRabbit feedback: compute remaining text after marker removal and only set withheld/markerOnlyStall when no real continuation text remains. Add regression test for marker + real text without tool_use.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Blockers
None.
Suggestions (recommended before merge)
S1 — Strip path corrupts mid-sentence assistant text (MEDIUM, correctness)
markerPattern is built with GREEDY \s* on both sides (src/query.ts:1476-1480) and the SAME pattern is reused both for detection and for removal via text.replace(markerPattern, '') (src/query.ts:1507-1510). When the marker is embedded inside a larger text block, the surrounding spaces are eaten:
input : "Please continue [Tool results received] and finish"
output: "Please continueand finish" // spaces adjacent to the marker removed
Reproduced locally with a standalone regex check. The \s* is justified for detection (find the marker even when wrapped in newlines), but removal should strip only the marker itself, not adjacent whitespace. Consequence: a provider that echoes the marker mid-sentence produces a corrupted user-visible message (and corrupted history), or — if no real text remains — is silently withheld. This only bites when the marker is fused with prose; the common case (the whole assistant turn is the marker) is unaffected.
Fix: keep \s* only in the detection regex; for stripping, replace with a pattern that matches the marker without consuming neighboring spaces (e.g. a separate edge-only regex, or replace only the escaped literal).
S2 — stopHookActive forced-nudge is an untested, scope-expanding change (MEDIUM)
src/query.ts:2356-2364 adds a second, independent behavior: whenever stopHookActive is truthy, the loop forces a continuation nudge regardless of analyzeContinuationIntent. This PR is nominally about the tool-results marker for self-hosted LLMs, yet this branch also fires on STANDARD Anthropic/Claude paths (it is not gated to the shim transport) and has NO regression test (the existing goalContinuation.test.ts mocks the hook to a no-op, so the branch is never exercised; nudgeReason === "stop_hook_active" is never asserted from queryLoop). Per AGENTS.md ("keep changes focused on one problem"), this belongs either with a dedicated test or in its own PR.
Fix: add a focused test asserting the stop_hook_active -> nudge transition, or split it out. At minimum, document why forcing a nudge on every standard-path stopHookActive turn is intended.
S3 — Stuck/misconfigured provider is reported as completed after the nudge cap (LOW–MEDIUM)
After 20 marker-only nudges the cap guard (src/query.ts:2330) is exceeded, the nudge block is skipped, and control falls through to return { reason: "completed" } (src/query.ts:2407) EVEN THOUGH the model produced zero real output. The loop now terminates (the prior P1 infinite-loop risk is gone) but a genuinely stuck provider is masked as success with no user-visible warning.
Fix: consider a distinct terminal reason (e.g. "marker_stall_exhausted") and/or a log/telemetry signal so a stuck self-hosted endpoint is not silently reported as a successful task completion.
S4 — Empty content:[] assistant message is mutated in place and forwarded (LOW, latent)
On a marker-only response the content is rewritten to [] (src/query.ts:1524) on the SAME object that was already pushed to assistantMessages (src/query.ts:1465), and that empty message is forwarded to the next API call via the nudge state (src/query.ts:2376). On the ONLY reachable path (the OpenAI shim, which injects the marker) convertMessages drops empty assistant content (src/services/api/openaiShim.ts:1341, invoked at :3836), so it does not reach the wire — NO 400 in production today. Two residual concerns:
- It violates the documented prompt-cache invariant "the original message is left untouched … mutating it would break prompt caching" (src/query.ts:1352-1356). Impact is negligible here because the message is dropped before the request, but the in-place mutation is a footgun.
- A transcript containing this content:[] message replayed to an Anthropic endpoint (resume / provider switch) would 400.
Fix: in the marker-only branch, skip pushing the now-empty message to assistantMessages (drop it) rather than mutating it in place.
S5 — Case-sensitivity could miss a re-cased echoed marker (LOW, provider-dependent)
Detection uses no i flag (src/query.ts:1476-1480). The shim always injects the exact casing, so legitimate detection is fine, but if a provider re-cases the echoed marker (e.g. lowercases assistant content), detection misses it and the shim artifact leaks into history/UI. Given the marker is a fixed constant, this is a latent risk, not an active bug.
S6 — Up to 20 extra billable model calls with no backoff (LOW)
Each continuation nudge is a full-history model/API call (continue at src/query.ts:2393) with no jitter/backoff and only a logForDebugging entry (src/query.ts:2367). On a stuck/misconfigured paid endpoint this is up to 20 unexpected billable requests. Bounded (good), but worth a user-visible signal (ties into S3).
S7 — Test coverage gaps (LOW–MEDIUM)
The new suite is sound overall (6/0 passing, isolated, Windows-compatible, deterministic mock). Weaknesses:
- T2 (marker with tool_use blocks …, toolResultsMarker.test.ts:136) never asserts needsFollowUp or that the marker was NOT stripped in the tool_use branch (src/query.ts:1525 deliberately does not strip) — the tool_use_id user message exists regardless, so the title claim is unverified.
- No test covers the mid-sentence corruption from S1, the empty-content forwarding from S4, or that a marker+real-text response is NOT withheld.
- No test asserts transition.reason (the cleanest proof of the marker-specific path).
- The hardcoded modelCalls === 21 (toolResultsMarker.test.ts:288, :395) is brittle to a change of MAX_CONTINUATION_NUDGES (also pinned in src/tests/bugfixes.test.ts:276).
S8 — Minor lint/const (TRIVIAL)
let remaining at src/query.ts:1497 is never reassigned; use const to avoid a prefer-const warning.
…tive nudge - S1: Use separate stripPattern (literal marker only) for text.replace() to avoid mid-sentence whitespace corruption when detection uses \s*. - S2: Add dedicated test for stop_hook_active -> continuation nudge path. - S3: Return distinct 'marker_stall_exhausted' reason instead of 'completed' when nudge cap is hit with zero real output. - S4: Push assistant messages after marker processing; drop empty content messages instead of mutating in place (S4 preserves prompt-cache invariant). - S7: Add tests for mid-sentence stripping, stall-exhausted reason, and stop_hook_active nudge. - S8: Change 'let remaining' to 'const' (prefer-const).
Review Fixes SummaryS1 — Mid-sentence marker corruption (MEDIUM) ✅ FixedAdded a separate Files changed: S2 —
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/query.ts (1)
999-1003: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
markerOnlyStallper assistant message. It’s only ever set totruein this scope and later reused by the continuation checks, so a marker-only response can leave the flag sticky and trigger an unnecessary nudge ormarker_stall_exhaustedon a later real response.🤖 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 999 - 1003, Reset markerOnlyStall at the start of processing each assistant message in the streaming loop, before marker detection and continuation checks. Keep setting it for marker-only responses, but ensure each new assistant message begins with false so a prior response cannot trigger an unnecessary nudge or marker_stall_exhausted outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/query.ts`:
- Around line 999-1003: Reset markerOnlyStall at the start of processing each
assistant message in the streaming loop, before marker detection and
continuation checks. Keep setting it for marker-only responses, but ensure each
new assistant message begins with false so a prior response cannot trigger an
unnecessary nudge or marker_stall_exhausted outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12ae39b7-1a3e-41b7-8024-89e9d57ee518
📒 Files selected for processing (4)
src/query.tssrc/query/goalContinuation.test.tssrc/query/toolResultsMarker.test.tssrc/query/transitions.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/query/transitions.tssrc/query/goalContinuation.test.tssrc/query/toolResultsMarker.test.tssrc/query.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/query/transitions.tssrc/query/goalContinuation.test.tssrc/query/toolResultsMarker.test.tssrc/query.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/query/goalContinuation.test.tssrc/query/toolResultsMarker.test.ts
🪛 ast-grep (0.44.1)
src/query.ts
[warning] 1476-1480: 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(
\\s*${TOOL_RESULTS_RECEIVED_MARKER .replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*,
'g',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 1491-1497: Do not use variable for regular expressions
Context: new RegExp(
TOOL_RESULTS_RECEIVED_MARKER.replace(
/[.*+?^${}()|[]\]/g,
'\$&',
),
'g',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🔇 Additional comments (9)
src/query.ts (6)
68-68: LGTM!
1474-1498: LGTM! Whitespace-tolerant detection pattern paired with a literal strip pattern correctly resolves the earlier brittle exact-match concern.
1515-1553: LGTM! Filtering blocks bycleaned.trim().length > 0correctly distinguishes marker-only from marker-plus-real-text, addressing the previously flagged marker-present-vs-marker-only bug. The empty-array placeholder avoids sendingcontent: []on the wire.
2373-2394: LGTM! Override ordering (markerOnlyStallthenstopHookActive, both gated on!shouldNudge) is correct and non-conflicting.
2437-2447: LGTM! Distinct terminal reason correctly prevents masking a genuinely stalled self-hosted endpoint ascompleted.
1554-1558: 🎯 Functional CorrectnessNo empty assistant bubble here.
assistantMessages.push(message)is intentional: marker-only stalls are withheld fromyieldMessage, and the message is kept only for recovery and downstream bookkeeping.> Likely an incorrect or invalid review comment.src/query/toolResultsMarker.test.ts (1)
118-118: LGTM! Good regression coverage of the fix: marker-only exhaustion (updated to the newmarker_stall_exhaustedreason, consistent withMAX_CONTINUATION_NUDGES = 20→ 21 calls), plus a focused mid-sentence stripping assertion that validates adjacent word boundaries aren't corrupted.Also applies to: 225-225, 315-375, 377-402
src/query/goalContinuation.test.ts (1)
245-332: LGTM! Test isolates its ownAppState/deps, correctly forces a blocking error only on the first stop-hook call, and asserts both thestopHookActivetransition and the injected meta nudge message on the follow-up model call — solid coverage for the newstop_hook_activecontinuation path.src/query/transitions.ts (1)
19-19: 🎯 Functional CorrectnessNo switch update is needed here.
marker_stall_exhaustedis a query transition reason, and there isn’t an exhaustiveTerminal['reason']consumer in this change that needs a new case.> Likely an incorrect or invalid review comment.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep yielding non-assistant model events
src/query.ts:1558
Movingyield yieldMessageinside themessage.type === 'assistant'branch drops everystream_eventandsystemmessage produced bycallModel. The normal streaming implementation emits raw stream events for UI/SDK deltas and emits system API-retry status messages; neither now reaches the consumer. Restore the generic, non-withheld yield outside the assistant-only handling so the marker logic does not turn off streaming and retry progress. -
[P1] Re-register tool calls with the streaming executor
src/query.ts:1554
This rewrite removes the onlystreamingToolExecutor.addTool(toolBlock, message)loop. Whentengu_streaming_tool_execution2is enabled, the executor is still constructed and the later branch selectsgetRemainingResults()instead ofrunTools(), but its queue is empty. Consequently every nativetool_usein that feature-gated path is neither executed nor returned to the model. Add the registration back after collectingmsgToolUseBlocks(independently of whether the marker is present). -
[P2] Do not let an earlier marker block override later real output
src/query.ts:1540
markerOnlyStallis scoped to the whole model-stream iteration and is never cleared. A provider stream can produce a marker-only content block and then a normal text block;queryModelWithStreamingyields an assistant message per completed content block. After the real response is accepted and yielded, the stale flag still forces a continuation nudge (or returnsmarker_stall_exhaustedwhen the cap is already reached), charging an unnecessary request and potentially reporting a failed turn despite valid output. Track the final/last assistant response, or clear the flag whenever a later substantive assistant message arrives, and cover that multi-block stream.
…arkerOnlyStall - P1: yield non-assistant events (StreamEvent, SystemAPIErrorMessage) from callModel instead of silently dropping them — restores streaming progress visibility. - P2: re-add streamingToolExecutor.addTool() loop removed in 780f53f so tools with streamingToolExecution2 enabled are actually registered and executed. - P2: clear markerOnlyStall when a substantive assistant message arrives after a marker-only one, preventing unnecessary continuation nudges.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Scope marker suppression to a shim-generated tool boundary
src/query.ts:1499
Every assistant response is currently matched against this public literal, without establishing that the preceding request contained the shim-injected tool-result boundary. A user can ask a model to reply exactly[Tool results received](or a model can emit it naturally); the response is then withheld, replaced with empty content, and retried up to 20 times. This loses legitimate output and can create 20 unsolicited billable requests on providers that never use the shim. Carry provenance from the shim, or at least gate this recovery path on the relevant preceding tool-result boundary. -
[P1] Surface an exhausted marker retry to the normal query consumers
src/query.ts:2460
Returningmarker_stall_exhaustedonly changes the async generator's return value. The normal REPL consumesquery()withfor await(src/screens/REPL.tsx:3034) and then unconditionally callsonTurnComplete(src/screens/REPL.tsx:3066), so it discards this reason and shows neither an error nor a failure after 20 empty, billable requests. Yield a user-visible terminal/error event or propagate and handle the terminal outcome in these consumers. -
[P2] Clear the stall flag when a later marker-containing block has real text
src/query.ts:1561
A streamed response can yield a marker-only assistant block followed by[Tool results received]\nAll done.in a later block. The latter is correctly retained and yielded, but this condition leavesmarkerOnlyStalltrue whenever the later block also contains the marker and no tool use. The continuation logic then sends an unnecessary follow-up request despite real output (and at the cap can returnmarker_stall_exhausted). Clear the flag based on whether the current block has substantive remaining text, and cover this multi-block stream. -
[P2] Do not nudge after the current stop-hook pass accepts completion
src/query.ts:2410
stopHookActivedescribes the previous turn's blocking hook result. On the retry,handleStopHooks()has already run again and can return no blocking errors, but this stale flag still forces another model request and clears itself only afterward. That extra turn can spend tokens or execute new tool calls after the hook has accepted the completed answer. Base the decision on the current stop-hook result (or reset the flag after a successful pass); the new test only assertsmodelCalls >= 2and does not catch the extra request.
…t event, fix stop-hook stale flag - Add shimToolResultBoundary provenance check so marker suppression only applies when the preceding request contained a tool-result boundary, preventing false suppression of legitimate user output. - Yield visible system message on marker_stall_exhausted so REPL consumers see the failure instead of discarding it silently. - Clear markerOnlyStall flag when the marker-containing block has real remaining text after stripping. - Use current stopHookResult.stopHookActive instead of stale state flag in continuation nudge decision. Fixes: P1 marker suppression scope, P1 marker_stall_exhausted surface, P2 stall flag clearing, P2 stop-hook stale flag
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 1605-1609: Update the condition in the marker-only stall handling
to also clear markerOnlyStall when !shimToolResultBoundary, alongside the
existing msgToolUseBlocks, hasToolResultsMarker, and remainingText checks.
Preserve the current behavior for stripped tool-result boundaries and
substantive remaining text.
- Around line 1285-1298: Update the shimToolResultBoundary logic to initialize
false, scan messagesForQuery backward from the end, and inspect user messages
for tool_result blocks. Stop the scan at the most recent tool_use so results
from earlier turns cannot qualify; set the boundary true only when a matching
tool_result is found within that scope.
In `@src/query/toolResultsMarker.test.ts`:
- Around line 35-44: Update the synthetic shim-boundary message created in the
test to use the canonical user role instead of assistant, while leaving its
tool_result content and other fields unchanged.
🪄 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: 4923ea3d-c8fe-4378-82b7-8d3a56925f8f
📒 Files selected for processing (2)
src/query.tssrc/query/toolResultsMarker.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/query/toolResultsMarker.test.tssrc/query.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/query/toolResultsMarker.test.tssrc/query.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/query/toolResultsMarker.test.ts
…ring Rewrite shimToolResultBoundary logic: initialize false, scan messages backward, stop at most recent tool_use so earlier-turn results don't qualify. Add !shimToolResultBoundary to markerOnlyStall clearing so marker-only responses properly trigger continuation nudges. Update test shimBoundaryMessage to use user role.
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 1294-1305: In src/query.ts:1294-1305, update both backward scans
around mostRecentToolUseIndex and shimToolResultBoundary to start at
messagesForQuery.length - 1 so the final tool_result is examined. In
src/query/toolResultsMarker.test.ts:30-51, remove the artificial
createUserMessage workaround and its explanatory comment, ensuring the
regression test leaves tool_result as the final message and validates the
user-visible boundary behavior.
🪄 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: 56a3c4ba-d8c7-4602-acfe-32cf1b074f80
📒 Files selected for processing (2)
src/query.tssrc/query/toolResultsMarker.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/query.tssrc/query/toolResultsMarker.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/toolResultsMarker.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/toolResultsMarker.test.ts
| for (let i = messagesForQuery.length - 2; i >= 0; i--) { | ||
| const msg = messagesForQuery[i] | ||
| if (msg?.type === 'assistant') { | ||
| const content = msg.message.content as { type: string }[] | undefined | ||
| if (content?.some(c => c.type === 'tool_use')) { | ||
| mostRecentToolUseIndex = i | ||
| break | ||
| } | ||
| } | ||
| } | ||
| // Scan backward from end, looking for user message with tool_result | ||
| for (let i = messagesForQuery.length - 2; i >= 0; i--) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Off-by-one bug in backward scan skips the active tool_result.
Both backward loops in query.ts start at messagesForQuery.length - 2, completely skipping the final message in the array. During standard tool continuation, the tool_result message is appended as the very last message (length - 1). Skipping it causes shimToolResultBoundary to always evaluate to false in production. The test suite artificially injected an extra user message after the boundary message to bypass this bug, hiding the failure.
src/query.ts#L1294-L1305: Start both backward loops atmessagesForQuery.length - 1so the final message is evaluated.src/query/toolResultsMarker.test.ts#L30-L51: Remove the artificialcreateUserMessageworkaround and its explanatory comment so the test accurately reflects the real runtime state where thetool_resultis the final message. As per path instructions, block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
🐛 Proposed fixes
src/query.ts
- for (let i = messagesForQuery.length - 2; i >= 0; i--) {
+ for (let i = messagesForQuery.length - 1; i >= 0; i--) {
const msg = messagesForQuery[i]
if (msg?.type === 'assistant') {
const content = msg.message.content as { type: string }[] | undefined
if (content?.some(c => c.type === 'tool_use')) {
mostRecentToolUseIndex = i
break
}
}
}
// Scan backward from end, looking for user message with tool_result
- for (let i = messagesForQuery.length - 2; i >= 0; i--) {
+ for (let i = messagesForQuery.length - 1; i >= 0; i--) {src/query/toolResultsMarker.test.ts
- // Include a synthetic user message with tool_result content BEFORE
- // the next user message to establish the "shim boundary". The shim injects
- // TOOL_RESULTS_RECEIVED_MARKER when prev.role === 'tool' (i.e. when the
- // message before the user message contains a tool_result). Without this,
- // shimToolResultBoundary is false and marker suppression is skipped.
const shimBoundaryMessage = {
type: 'user' as const,
uuid: 'assistant-shim-boundary',
timestamp: new Date().toISOString(),
message: {
role: 'user' as const,
content: [
{ type: 'tool_result', tool_use_id: 'tool-1', content: 'ok' },
] as { type: string; tool_use_id?: string; content: string }[],
},
toolUseResult: { output: 'ok' },
}
return {
messages: [
shimBoundaryMessage,
- createUserMessage({ content: 'do something' }),
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (let i = messagesForQuery.length - 2; i >= 0; i--) { | |
| const msg = messagesForQuery[i] | |
| if (msg?.type === 'assistant') { | |
| const content = msg.message.content as { type: string }[] | undefined | |
| if (content?.some(c => c.type === 'tool_use')) { | |
| mostRecentToolUseIndex = i | |
| break | |
| } | |
| } | |
| } | |
| // Scan backward from end, looking for user message with tool_result | |
| for (let i = messagesForQuery.length - 2; i >= 0; i--) { | |
| for (let i = messagesForQuery.length - 1; i >= 0; i--) { | |
| const msg = messagesForQuery[i] | |
| if (msg?.type === 'assistant') { | |
| const content = msg.message.content as { type: string }[] | undefined | |
| if (content?.some(c => c.type === 'tool_use')) { | |
| mostRecentToolUseIndex = i | |
| break | |
| } | |
| } | |
| } | |
| // Scan backward from end, looking for user message with tool_result | |
| for (let i = messagesForQuery.length - 1; i >= 0; i--) { |
📍 Affects 2 files
src/query.ts#L1294-L1305(this comment)src/query/toolResultsMarker.test.ts#L30-L51
🤖 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 1294 - 1305, In src/query.ts:1294-1305, update
both backward scans around mostRecentToolUseIndex and shimToolResultBoundary to
start at messagesForQuery.length - 1 so the final tool_result is examined. In
src/query/toolResultsMarker.test.ts:30-51, remove the artificial
createUserMessage workaround and its explanatory comment, ensuring the
regression test leaves tool_result as the final message and validates the
user-visible boundary behavior.
Source: Path instructions
Both backward scans in shimToolResultBoundary logic started at length-2, skipping the last message. Fix the tool_result scan to start at length-1 so the final tool_result is examined. Update regression test to leave tool_result as the final message.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/query.ts (1)
1604-1612: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the assistant message immutable here.
yieldMessagecan still point at the same object, andassistantMessages.push(message)stores it for later API/history use. Mutatingmessage.message.contentleaks the normalized placeholder into the transcript and can break prompt-cache byte matching. Build a separate normalized copy for the stripped/empty case instead.🤖 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 1604 - 1612, The empty-content normalization currently mutates the original assistant message, affecting yieldMessage and stored history. Update the surrounding normalization flow to create and use a separate message copy when assigning the placeholder or remaining content, while leaving the original message object unchanged for transcript and prompt-cache use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/query.ts`:
- Around line 1604-1612: The empty-content normalization currently mutates the
original assistant message, affecting yieldMessage and stored history. Update
the surrounding normalization flow to create and use a separate message copy
when assigning the placeholder or remaining content, while leaving the original
message object unchanged for transcript and prompt-cache use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f0cf4d4-6016-4a2d-89eb-e66e178f51ff
📒 Files selected for processing (2)
src/query.tssrc/query/toolResultsMarker.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/query/toolResultsMarker.test.tssrc/query.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/query/toolResultsMarker.test.tssrc/query.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/query/toolResultsMarker.test.ts
🔇 Additional comments (2)
src/query.ts (1)
68-68: LGTM!Also applies to: 999-1011, 1271-1318, 1648-1651, 2384-2390, 2468-2491, 2534-2549
src/query/toolResultsMarker.test.ts (1)
26-44: LGTM!Also applies to: 101-148, 150-206, 208-240, 242-284, 286-323, 325-327, 329-389, 391-417
…r strip The empty-content normalization at the shim boundary mutated message.message.content in place, which also affected yieldMessage when no prior backfill clone existed. Now yieldMessage gets its own shallow clone so the original message stays untouched for transcript and prompt-cache.
…ction echo loop When a self-hosted provider echoes the shim-injected tool-result marker, the retained marker text in message.message gets appended to messagesForQuery on continuation nudge. The shim re-injects the marker on each iteration, causing an infinite echo loop that exhausts after 20 nudges. Strip the marker from message.message immediately after detection so subsequent iterations see a clean assistant message and the shim doesn't re-inject it.
|
I think there is better solution for llama-server. |
3 participants
Summary
When using OpenAI compatible LLM provider like llama-cpp running on non-local address (even with domain), there is a problem - pipeline may stop after tool call. [Tool results received] message appears in console.
Now we accepts [Tool results received] as continuation marker to allow pipeline run further.
Summary by CodeRabbit