test: normalize Ollama tool-call transcripts#377
Conversation
📝 WalkthroughWalkthroughThe PR normalizes multi-tool Ollama transcripts across test harnesses, preserves original assistant messages with ordered tool results, adds credential-free regression coverage, and improves model, proxy, and scenario failure reporting. ChangesTranscript normalization and runner validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Ollama
participant TestRunner
participant GovernanceProxy
participant ToolHandler
Ollama->>TestRunner: assistant message with multiple tool calls
TestRunner->>GovernanceProxy: evaluate each tool call
GovernanceProxy-->>TestRunner: PERMIT or DENY decision
TestRunner->>ToolHandler: execute permitted tool calls
ToolHandler-->>TestRunner: ordered serialized results
TestRunner->>Ollama: original assistant message plus tool results
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
python/tests/run_cloud_model_test.py (1)
600-637: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDENY decisions are conflated with real failures, causing false-negative run failures.
report["errors"](populated at Lines 494-500) mixes three unrelated things: model/provider exceptions, "no tool calls and no content", and legitimateDENYgovernance decisions. The newrun_completed = not report["errors"](Line 600) and theraise RuntimeError(...)whenreport["errors"]is non-empty (Lines 628-633) now treat any proxyDENY— a correct, expected governance outcome givenforbidden_tools=["delete_file", "execute_shell"]— as a fatal run failure alongside real provider/proxy failures.This directly conflicts with the pattern used in
run_adversarial_suite.py, which correctly keepsdenialsseparate fromerrors(only entries where the decision is outside{PERMIT, DENY}count as errors). No regression test in this PR exercises the DENY path throughmain(), so this gap isn't caught.Suggest separating
report["denials"]fromreport["errors"], and gatingrun_completed/the raise only on genuine failures.🔧 Proposed fix
- if status != 200 or decision.get("decision") != "PERMIT": + if status != 200: print( f" DENIED: {tool_name}({list(tool_args.keys())}) → {decision.get('decision', 'UNKNOWN')}" ) report["errors"].append( { "tool": tool_name, "args_keys": list(tool_args.keys()), "decision": decision, } ) result = {"status": "denied", "reason": str(decision)} + elif decision.get("decision") != "PERMIT": + print( + f" DENIED: {tool_name}({list(tool_args.keys())}) → {decision.get('decision', 'UNKNOWN')}" + ) + report.setdefault("denials", []).append( + { + "tool": tool_name, + "args_keys": list(tool_args.keys()), + "decision": decision, + } + ) + result = {"status": "denied", "reason": str(decision)}🤖 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 `@python/tests/run_cloud_model_test.py` around lines 600 - 637, Separate legitimate DENY governance decisions from genuine failures in the report-building and completion logic around main(). Track DENY results in report["denials"], while retaining provider/proxy exceptions, missing output, and decisions outside PERMIT/DENY in report["errors"]; make run_completed and the RuntimeError raise depend only on report["errors"], while preserving denial reporting.
🧹 Nitpick comments (1)
python/tests/run_adversarial_suite.py (1)
1040-1110: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared "ordered tool-result + single assistant append" logic into a common test helper. The exact same three-step pattern — build a list of
role: "tool"messages keyed bytool_name, append the original assistant message once, then extend with the ordered results — is duplicated six times across five files to satisfy the same Issue#374contract. A single shared helper would guarantee future fixes to this transcript-normalization logic apply uniformly to all five harnesses instead of relying on manual copy-paste consistency.
python/tests/run_adversarial_suite.py#L1040-L1110: replace the inlinetool_resultsaccumulation/append with a call to a shared helper (e.g.,build_tool_result_messages(tool_calls_results)+append_turn(messages, resp.message, tool_results)).python/tests/run_cloud_model_test.py#L474-L550: use the same shared helper for itstool_resultsaccumulation and finalmessages.append/extend.python/tests/test_ardur_comprehensive_integration.py#L919-L971: use the same shared helper.python/tests/test_ollama_integration.py#L555-L587: use the same shared helper (it can still add its extracall_indexfield via a parameter or by extending the base result dict).python/tests/test_ardur_overhead_ab.py#L263-L307: use the same shared helper inrun_without_ardur.python/tests/test_ardur_overhead_ab.py#L368-L426: use the same shared helper inrun_with_ardur.🤖 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 `@python/tests/run_adversarial_suite.py` around lines 1040 - 1110, Extract the duplicated ordered tool-result and single-assistant-append sequence into one shared test helper, preserving role/tool_name fields and ordered results; support Ollama’s extra call_index field. Apply the helper in python/tests/run_adversarial_suite.py:1040-1110, run_cloud_model_test.py:474-550, test_ardur_comprehensive_integration.py:919-971, test_ollama_integration.py:555-587, and both test_ardur_overhead_ab.py:263-307 and :368-426, including run_without_ardur and run_with_ardur.
🤖 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 `@python/tests/run_cloud_model_test.py`:
- Around line 600-637: Separate legitimate DENY governance decisions from
genuine failures in the report-building and completion logic around main().
Track DENY results in report["denials"], while retaining provider/proxy
exceptions, missing output, and decisions outside PERMIT/DENY in
report["errors"]; make run_completed and the RuntimeError raise depend only on
report["errors"], while preserving denial reporting.
---
Nitpick comments:
In `@python/tests/run_adversarial_suite.py`:
- Around line 1040-1110: Extract the duplicated ordered tool-result and
single-assistant-append sequence into one shared test helper, preserving
role/tool_name fields and ordered results; support Ollama’s extra call_index
field. Apply the helper in python/tests/run_adversarial_suite.py:1040-1110,
run_cloud_model_test.py:474-550,
test_ardur_comprehensive_integration.py:919-971,
test_ollama_integration.py:555-587, and both test_ardur_overhead_ab.py:263-307
and :368-426, including run_without_ardur and run_with_ardur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5834fd40-15f8-481c-9a04-06c4cf22c24b
📒 Files selected for processing (6)
python/tests/run_adversarial_suite.pypython/tests/run_cloud_model_test.pypython/tests/test_ardur_comprehensive_integration.pypython/tests/test_ardur_overhead_ab.pypython/tests/test_ollama_integration.pypython/tests/test_ollama_transcript_regressions.py
e93563c to
7e7130a
Compare
|
Hosted-review resolution at amended signed/DCO head
The docstring-coverage warning was reviewed but not applied: these are private fixtures and descriptively named tests, and this repository has no docstring-coverage gate. Adding repetitive test docstrings would be unrelated churn. The shared-helper nitpick was also reviewed but not applied in this focused PR. Only the two-line final append invariant is common; the independently executable harnesses differ in decision, execution, reporting, and payload behavior. A shared helper would add cross-script import coupling without centralizing the risky logic, while the credential-free regression suite now enforces the invariant across all implementations. A local CodeRabbit retry after the correction was attempted but rate-limited for 30 minutes, so it is not claimed as a clean local review. Fresh exact-head GitHub Actions and hosted CodeRabbit review are now running; merge remains held until both complete. |
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 `@python/tests/run_all_models.py`:
- Around line 84-89: Update the legacy fallback in the report classification
logic to count only entries whose decision is explicitly “DENY” as denials,
classify entries with “UNKNOWN” or provider-failure decisions as exceptions, and
count entries lacking both a recognized decision and an error as failures so
they cannot produce a clean result. Add regression coverage for DENY, PERMIT,
UNKNOWN/provider failures, and empty entries such as errors=[{}].
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e75f471f-a0c7-4631-8c1e-5311e171ff13
📒 Files selected for processing (7)
python/tests/run_adversarial_suite.pypython/tests/run_all_models.pypython/tests/run_cloud_model_test.pypython/tests/test_ardur_comprehensive_integration.pypython/tests/test_ardur_overhead_ab.pypython/tests/test_ollama_integration.pypython/tests/test_ollama_transcript_regressions.py
Checkpoint: architect/sessions/release-v0.2.0-qualification/2026-07-22-journal.md Signed-off-by: Gnani Rahul Nutakki <gnani.nutakki@gmail.com>
7e7130a to
7066d6b
Compare
1 participant
Relevant issues
Closes #374
Type
Changes
The implementation follows the official Ollama tool-calling contract: https://docs.ollama.com/capabilities/tool-calling
README and public documentation were reviewed. No public API or operator workflow changed, so no documentation update is required.
AI assistance was used to implement and review this change. The human author retains responsibility and verified the tests and resulting diff.
Testing
No live provider, credential, Docker registry, or Kubernetes cluster was used in these tests.
Summary by CodeRabbit
Bug Fixes
Tests