Skip to content

fix: thinking block content leak in text extraction#10

Merged
bkrabach merged 1 commit into
mainfrom
fix/thinking-block-text-leak
Apr 20, 2026
Merged

fix: thinking block content leak in text extraction#10
bkrabach merged 1 commit into
mainfrom
fix/thinking-block-text-leak

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

Summary

Fixes a thinking-block text leak at two locations in amplifier_module_loop_events/__init__.py.

Root Cause

There are two content block model systems in amplifier-core:

  • content_models.ThinkingContenthas .text (was the bug hazard)
  • message_models.ThinkingBlock → has .thinking (already safe)

The old execute() inline text extraction used hasattr(block, "text") at both the main loop path (line ~268) and the max-iterations fallback path (line ~621). Because ThinkingContent has a .text attribute, this guard let thinking text leak into final_response.

Fix

Extracts a shared _extract_text_from_content helper (module-level, following the existing _normalize_tool_call convention) that uses an explicit block.type == "text" check. Replaces both call sites with the helper — eliminating the duplication and the bug in one pass.

Pattern

Same fix as:

This module has two locations with the bug, so extracting the shared helper is especially important for DRY compliance.

Tests Added

New file tests/test_thinking_block_leak.py with 8 tests:

Test Purpose
test_thinking_content_does_not_leak_into_final_response Primary regression — Location 1 (main loop path)
test_thinking_content_filtered_at_max_iterations_fallback Primary regression — Location 2 (max-iterations fallback)
test_helper_directly_filters_thinking_content Direct unit test of the shared helper
test_text_content_passes_through Regression guard — TextContent still works
test_text_block_passes_through Regression guard — TextBlock still works
test_thinking_block_does_not_leak_into_final_response Documents ThinkingBlock was already safe
test_dict_text_block_passes_through Dict {"type": "text"} blocks pass through
test_dict_thinking_block_is_filtered Dict {"type": "thinking"} blocks filtered

TDD Verification

RED: 4 tests failed before fix (bug tests + import error for helper not yet existing)
GREEN: All 8 tests pass after fix

Note: test_tool_post_no_modify_uses_original in test_hook_modify.py was already failing on main before this branch — not introduced by this change.

content_models.ThinkingContent has a .text attribute (distinct from
message_models.ThinkingBlock which uses .thinking). The hasattr-based
filter at both the main loop path and the max-iterations fallback was
letting thinking text leak into final_response.

Extracts a shared _extract_text_from_content helper and replaces both
call sites. Same fix pattern as amplifier-module-loop-streaming PR #25
(merge df5c0e1) and amplifier-module-loop-basic PR #11.

Adds regression tests verifying ThinkingContent is filtered at both
code paths while TextBlock/TextContent pass through.
@bkrabach bkrabach merged commit 77af438 into main Apr 20, 2026
1 check passed
@bkrabach bkrabach deleted the fix/thinking-block-text-leak branch April 20, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant