Skip to content

fix(detection): preserve raw codepoints in tool-result extraction - #47

Open
Rahul-s-007 wants to merge 1 commit into
uber:mainfrom
Rahul-s-007:fix/tool-result-extraction
Open

fix(detection): preserve raw codepoints in tool-result extraction#47
Rahul-s-007 wants to merge 1 commit into
uber:mainfrom
Rahul-s-007:fix/tool-result-extraction

Conversation

@Rahul-s-007

@Rahul-s-007 Rahul-s-007 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Related issue: Closes #46

What changed?
SessionManager._parse_message (main_benchmark.py) now extracts real text from tool_result content via a new _extract_text_from_content method instead of calling str() on the raw content list, which was silently corrupting any non-printable Unicode into literal backslash text via Python's repr() before it was ever written to disk. Mirrors the two already-proven implementations in Sensor/adr_sensor/parsers/claude_parser.py/claude_desktop_parser.py, including the toolUseResult root-level override both apply. Also fixes 3 edge cases found during review (dict-shaped content, a falsy toolUseResult['result'] discarding real content, one toolUseResult clobbering multiple tool_result blocks), and a regression the review surfaced: ToolAnalyzer.analyze_tool_usage detected failed tool calls by string-matching repr artifacts that only existed because of the bug above — fixing the bug correctly made that detection permanently silent. Fixed by tracking failure structurally via a new failed_tool_use_ids field instead of string-matching.

Why?
This is the root-cause fix underlying a Unicode-obfuscation detection feature (separate PR to follow) — without it, that feature can never see real payloads on the actual benchmark execution path, only mangled text. Split out per review as its own, independently-reviewable harness bug fix.

How did you test it?
24 tests in tests/test_main_benchmark.py (TestSessionManagerContentExtraction, TestToolAnalyzer) covering the core fix, all 3 edge cases, and the regression fix. Verified the regression tests actually catch the bugs — temporarily reverted just this fix, reran, confirmed 6 tests fail with the exact predicted symptoms (literal \U000e... text, KeyError on the missing field, real content clobbered by an empty override), then confirmed all pass again with the fix restored.

24 passed — click to expand 
============================= test session starts ==============================
platform darwin -- Python 3.14.0, pytest-9.1.1, pluggy-1.6.0 -- /private/tmp/adr_final_verify_venv/bin/python3
cachedir: .pytest_cache
rootdir: /Users/test4/Desktop/OSS/Uber ADR/Detection
configfile: pyproject.toml
plugins: asyncio-1.4.0, anyio-4.14.2
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collecting ... collected 24 items

tests/test_main_benchmark.py::TestConfig::test_default_max_concurrent_tasks PASSED [  4%]
tests/test_main_benchmark.py::TestConfig::test_disallowed_tools_loaded_from_config PASSED [  8%]
tests/test_main_benchmark.py::TestCommandBuilder::test_builds_claude_command PASSED [ 12%]
tests/test_main_benchmark.py::TestCommandBuilder::test_adds_permission_bypass_flag PASSED [ 16%]
tests/test_main_benchmark.py::TestTaskManager::test_filter_tasks_by_range PASSED [ 20%]
tests/test_main_benchmark.py::TestTaskManager::test_filter_tasks_by_csv_and_range PASSED [ 25%]
tests/test_main_benchmark.py::TestTaskManager::test_validate_task_requires_fields PASSED [ 29%]
tests/test_main_benchmark.py::TestMCPServerManager::test_create_mcp_config_writes_workspace_file PASSED [ 33%]
tests/test_main_benchmark.py::TestMCPServerManager::test_process_arg_template_replaces_workspace_path PASSED [ 37%]
tests/test_main_benchmark.py::TestConcurrencyGuard::test_rejects_zero_concurrency PASSED [ 41%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_kills_process_on_timeout PASSED [ 45%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_kills_grandchild_process_holding_inherited_stdio PASSED [ 50%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_returns_parsed_json_on_success PASSED [ 54%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_reports_nonzero_exit_without_leaving_error_message_empty PASSED [ 58%]
tests/test_main_benchmark.py::TestSessionManagerContentExtraction::test_tag_block_payload_survives_tool_result_extraction PASSED [ 62%]
tests/test_main_benchmark.py::TestSessionManagerContentExtraction::test_tool_use_result_override_takes_precedence PASSED [ 66%]
tests/test_main_benchmark.py::TestSessionManagerContentExtraction::test_plain_string_content_passes_through_unchanged PASSED [ 70%]
tests/test_main_benchmark.py::TestSessionManagerContentExtraction::test_falsy_tool_use_result_does_not_discard_real_content PASSED [ 75%]
tests/test_main_benchmark.py::TestSessionManagerContentExtraction::test_multiple_tool_result_blocks_not_clobbered_by_single_tool_use_result PASSED [ 79%]
tests/test_main_benchmark.py::TestSessionManagerContentExtraction::test_dict_shaped_content_preserves_unicode_via_json PASSED [ 83%]
tests/test_main_benchmark.py::TestSessionManagerContentExtraction::test_failed_tool_use_ids_extracted_from_is_error_blocks PASSED [ 87%]
tests/test_main_benchmark.py::TestToolAnalyzer::test_failed_tool_call_excluded_from_stats PASSED [ 91%]
tests/test_main_benchmark.py::TestToolAnalyzer::test_successful_tool_call_counted PASSED [ 95%]
tests/test_main_benchmark.py::TestToolAnalyzer::test_text_based_tool_not_found_fallback_still_works PASSED [100%]

============================== 24 passed in 0.39s ==============================

Potential risks
Low-medium. Touches a parsing path with zero prior test coverage (confirmed via grep before this PR). The ToolAnalyzer fix changes what counts as a "failed" tool call — verified via dedicated tests that both the new structural detection and the pre-existing text-based fallback work correctly.

@Rahul-s-007 @claude
fix(detection): preserve raw codepoints in tool-result extraction
7c29e9b 
Split out of PR uber#43 per review - this is a distinct benchmark-harness
data-integrity bug, not the Unicode-detection feature itself. Fixes uber#46.

SessionManager._parse_message passed tool_result message content -
always a list of content blocks for Claude Code's JSONL - straight
into _truncate_content, whose non-str branch called str() on it.
str() on a list repr()s every element, which escapes non-printable
Unicode (Tag Block "ASCII smuggling" characters are category Cf) into
literal backslash text before json.dump ever writes
claude_conversation.json. Irreversible: nothing downstream un-reprs
it. Hits every MCP tool_result in a real recorded run (8112/~19k
message contents).

Adds SessionManager._extract_text_from_content, mirroring the two
already-proven implementations in
Sensor/adr_sensor/parsers/claude_parser.py and
claude_desktop_parser.py, including the toolUseResult root-level
override both reference parsers apply.

Also fixes three edge cases found during review:
- dict-shaped content now uses json.dumps(ensure_ascii=False) instead
  of falling to the same str()/repr() bug for a different content shape
- a falsy toolUseResult['result'] (e.g. "") no longer overwrites a
  tool_result block's own real content - only a truthy override applies
- a single root-level toolUseResult is only applied when there's
  exactly one tool_result block in the message, instead of clobbering
  every block with the same value when there are several (e.g.
  parallel tool calls)

And fixes a regression the review surfaced in the same code path:
ToolAnalyzer.analyze_tool_usage detected failed tool calls by
string-matching repr artifacts ("'is_error': True", "'tool_use_id':
'...'") that only existed because of the str()/repr() bug above - a
correct fix for that bug makes the string match always fail, so
failed_tool_ids stays permanently empty and every failed tool call
gets counted as successful, for every task in tasks.json. Fixed by
tracking failure structurally instead: SessionManager now extracts
each tool_result block's own is_error/tool_use_id fields directly via
a new _extract_failed_tool_ids helper, populating a
failed_tool_use_ids field that ToolAnalyzer reads instead of
string-matching. The pre-existing text-based fallback for a
tool-not-found error (unrelated to this bug, not reported as broken)
is left untouched.

New test coverage: SessionManager content extraction (tag-block
payload survival, toolUseResult override precedence, all three edge
cases, failed_tool_use_ids extraction) and ToolAnalyzer (failed calls
correctly excluded, successful calls still counted, the pre-existing
text fallback still works). Verified the regression tests actually
catch the bugs: temporarily reverted this fix, reran, confirmed 6
tests fail with the exact symptoms described (literal \U000e... text,
KeyError on the new field, real content clobbered by an empty
override), then confirmed all pass again with the fix restored.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 18, 2026
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pengyuzhang pengyuzhang pengyuzhang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SessionManager._parse_message repr-mangles non-str tool_result content, and ToolAnalyzer's error detection silently depends on that mangling

2 participants

@Rahul-s-007 @pengyuzhang