Skip to content

fix(ollama-native): keep tool batches open across assistant commentary - #6509

Closed
adtumk wants to merge 4 commits into
lidge-jun:devfrom
adtumk:fix/ollama-native-assistant-commentary
Closed

adtumk wants to merge 4 commits into
lidge-jun:devfrom
adtumk:fix/ollama-native-assistant-commentary

Conversation

@adtumk

@adtumk adtumk commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Codex can record assistant commentary between a tool-call batch and its genuine results. The native Ollama adapter previously settled the batch at that commentary, emitted unknown-status placeholders, then rejected the recorded results as ollama-native orphan tool result.

Keep the batch open across assistant text/thinking that introduces no new tool calls while results are outstanding. Serialize genuine results beside their originating calls, then release deferred messages in arrival order. For example, calls A/B -> commentary -> result A/B becomes calls A/B -> result A/B -> commentary on the native wire.

A new tool-call batch still settles its predecessor. Missing results retain their explicit unknown-status marker; orphan IDs, duplicates, and mismatched tool names remain invalid. The input history is not mutated. This extends #4848's user/developer deferral to assistant commentary, including routed compaction replay. Architecture and user documentation describe the resulting behavior.

Verification

Scoped local validation is complete on Windows with Bun 1.4.0. No live provider continuation was exercised.

  • bun test tests/providers/ollama/ollama-native.test.ts: 32 pass / 0 fail, 144 assertions. The six added cases cover parallel and partially completed batches, text/thinking order and input immutability, completed batches, interrupted batches followed by new calls, and routed compaction. Existing strict validation still passes.
  • Regression ablation with the adapter restored to upstream dev at 3bae88cce7400e47a5e69f9fd28749ed25f919f6: 28 pass / 4 fail. The four failures detect commentary before genuine results.
  • With CI=true, bun run test --parallel=1 --timeout=60000 ./tests/providers/ollama/ ./tests/adapters/adapter-buffered-tool-conformance.test.ts ./tests/adapters/adapter-tool-conformance.test.ts ./tests/adapters/adapter-registry-authority.test.ts ./tests/adapters/identity-subagent.test.ts ./tests/adapters/anthropic/anthropic-tool-declaration-constraints.test.ts: 167 pass / 0 fail across 13 files, 831 assertions, on final head de5bb1f215c613670e03585e918a344c510ba97a.
  • Re-ran all 16 files associated with the earlier changed-mode failures in fresh processes on both the PR branch and unmodified dev: 397 pass / 0 fail on each branch. Every one of the 63 earlier named failures matches a fresh passing case; the nine unnamed hook failures are absent from the complete fresh-file runs. Each file used CI=true and bun run test --parallel=1 --timeout=60000 ./tests/<file>, matching the repository CI's per-test budget. The slow Desktop file took 404 seconds on the PR branch and 403 seconds on stock dev.
  • bun run typecheck, bun run privacy:scan, bun run structure:check, and bun scripts/file-size-ratchet.ts: passed after the documentation follow-ups. The last commit adds only a fixture docstring; its file-size check also passed, and the final-head 167-test run includes that fixture.
  • In docs-site/, bun install --frozen-lockfile and bun run build: passed; 561 pages built and 77,923 internal links checked. These documentation files have not changed since that build.
  • git diff --cached --check: passed before each commit; the working tree is clean.
Files re-run on both the PR branch and stock dev
server/loopback-listener-integration.test.ts
config/settings-stream-mode.test.ts
lab/lab-activation.test.ts
responses/responses-grok-devin-preflight.test.ts
claude-integration/claude-desktop-first-party-guards.test.ts
claude-integration/claude-desktop-first-party.test.ts
routing/compatibility-provider-equivalence.test.ts
routing/routing-compatibility-boundaries.test.ts
lab/lab-read-surfaces.test.ts
claude-integration/claude-desktop-picker-routes.test.ts
routing/routing-compatibility.test.ts
claude-integration/claude-desktop-picker.test.ts
routing/routing-policy-surface-parity.test.ts
routing/routing-profile-management-editor.test.ts
claude-integration/claude-messages-endpoint.test.ts
codex-integration/bearer-admission-routed-provider.test.ts

Full-suite scope exception: The full 1,985-file bun run test suite was not run locally. The earlier bun run test:changed selected 530 files but exceeded its 900-second limit with Bun's default five-second per-test budget (exit 124; 8,402 passing records and 72 failing records, without a completed suite summary). Windows process/ACL operations in the affected Desktop cases take 8–25 seconds, and even an isolated Desktop file takes almost seven minutes. A full run is disproportionate on this host. Under the repository's AGENTS.md scoped-validation exception, coverage consists of the complete Ollama/shared conformance set plus the complete affected-file re-runs and stock-dev comparison above. The old incomplete run is not passing evidence; no failure persists in the fresh comparison. Remaining whole-repository and Linux/macOS coverage is left to required CI.

CodeRabbit reviewed the final head and reported no actionable comments; there are no open review threads. Its docstring-coverage warning remains advisory. The branch contains the current dev tip. Cross-platform CI on the final head reports action_required and awaits maintainer approval; it has not passed and must succeed before merge. Review readiness uses the documented local scope exception and does not claim merge readiness.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Fixtures are synthetic; no credential, authentication, workflow, dependency, or logging surface is changed.

Review readiness checklist

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 623d92a6-4e69-4587-82ee-b6c63c98bd4f
📥 Commits

Reviewing files that changed from the base of the PR and between a47a6d0 and de5bb1f.

📒 Files selected for processing (1)
  • tests/providers/ollama/ollama-native.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The Ollama native adapter tracks unresolved tool calls and defers assistant commentary while results are pending. It preserves tool-result order, settles an earlier batch when a new batch starts, and documents and tests these behaviors.

Changes

Ollama Native Tool Batches

Layer / File(s) Summary
Track and serialize pending tool batches
src/adapters/ollama-native.ts, tests/providers/ollama/ollama-native.test.ts, structure/providers/chat-compat.md, docs-site/src/content/docs/reference/adapters.md
The adapter tracks unresolved calls and defers assistant text and thinking until pending results are recorded or a new batch begins. Tests cover parallel and partial results, deferred messages, batch settlement, and compaction. Documentation describes handling for pending, missing, orphan, and duplicate results.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to de5bb

No specific merge-blocking behavior is established. Complete the outstanding broader validation before treating the reported test results as a full-suite pass.

Architecture Summary

Architecture risk: 🔵 Low · up to de5bb

The change affects 4 systems.

Changed systems: docs-site, src, structure, tests

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — docs-site (service) was modified; 1 changed file maps to changed impact.
  • observed — src (service) was modified; 1 changed file maps to changed impact.
  • observed — structure (service) was modified; 1 changed file maps to changed impact.
  • observed — tests (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in docs-site/src/content/docs/reference/adapters.md: Documents how ollama-native handles commentary and thinking during pending tool-result batches: it defers them until settlement, settles a prior batch when a new one begins, marks missing results as unknown, and rejects orphan or duplicate results.
  • observed — Modified behavior in structure/providers/chat-compat.md: buildNativeMessages now defers assistant text and thinking without new tool calls while results remain outstanding, in addition to deferring user and developer messages. Deferred messages retain arrival order after recorded results; an unresolved counter tracks pending calls, and a new call batch settles its predecessor. Missing-result markers and errors for orphan IDs, duplicate results, or mismatched tool names are retained.
  • observed — Modified behavior in src/adapters/ollama-native.ts: PendingToolBatch adds unresolvedCount to track tool calls still awaiting results.
  • observed — Modified behavior in src/adapters/ollama-native.ts: A new buildNativeMessages documentation block describes replaying tool results beside their originating batch, deferring intervening conversation until settlement, reserving call IDs, marking missing results, and possible replay errors.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: keeping Ollama native tool batches open while assistant commentary arrives.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

Hygiene

✅ Deterministic PR hygiene checks passed.

@adtumk

adtumk commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@adtumk

adtumk commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@adtumk

adtumk commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@adtumk

adtumk commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions
github-actions Bot marked this pull request as ready for review October 3, 2026 13:29
@lidge-jun

Copy link
Copy Markdown
Owner

Superseded by #6519, merged into dev as a141b83623a3f1677f23477e91b9a42a74f495f7. All four source commits were carried with original authors and cherry-pick provenance; contributor credit is retained in the replacement. The unresolved tool-batch/commentary behavior and documentation are included, with additional validation, ordering, EOF and deeply frozen input regressions. No implementation part was dropped.

Current-head scoped CI passed: https://github.com/lidge-jun/opencodex/actions/runs/37131234397. Independent replay review exercised 2,316 synthetic permutations; this is not a live Ollama or physical tool execution claim. Final integrated cross-platform regression and production publication remain ahead. Thank you for the original fix.

@lidge-jun lidge-jun closed this Oct 3, 2026
ZehuaKcrissLi pushed a commit to ZehuaKcrissLi/opencodex that referenced this pull request Oct 3, 2026
Carry source PR lidge-jun#6509 at de5bb1f with original authored commits and add adversarial regression coverage.

Co-authored-by: potota90 <85318310+adtumk@users.noreply.github.com>
@lidge-jun lidge-jun mentioned this pull request Oct 4, 2026
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready superseded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants