From 162c7bc759372104dc3a255060d4ea140e8efd3f Mon Sep 17 00:00:00 2001 From: Jonathan Segev Date: Wed, 10 Jun 2026 12:57:10 -0400 Subject: [PATCH 1/6] fix(strands-command): inline full PR diffs within a total budget get_pr_files capped every file's patch at 50 lines, so the reviewer silently lost the tail of any larger change and reasoned about code it never saw. Replace the per-file line cap with a total character budget: inline full patches until the budget is reached, then list remaining files for on-demand fetch instead of truncating mid-file. --- .../scripts/python/github_tools.py | 61 +++++++++++++------ .../scripts/python/tests/test_github_tools.py | 50 +++++++++++++++ 2 files changed, 91 insertions(+), 20 deletions(-) create mode 100644 strands-command/scripts/python/tests/test_github_tools.py diff --git a/strands-command/scripts/python/github_tools.py b/strands-command/scripts/python/github_tools.py index 5630344..58fb139 100644 --- a/strands-command/scripts/python/github_tools.py +++ b/strands-command/scripts/python/github_tools.py @@ -77,6 +77,10 @@ console = console_util.create() +# Total characters of PR diff inlined into a single prompt by get_pr_files before +# remaining files are listed for on-demand fetch instead of being inlined. +TOTAL_DIFF_BUDGET_CHARS = 100_000 + class GitHubOperation(TypedDict): """Type definition for GitHub operation records in JSONL files.""" @@ -528,32 +532,49 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: return result output = f"Files changed in PR #{pr_number}:\n\n" - + + # Bound the total diff size we inline into one prompt rather than capping each + # file at a fixed line count: a per-file cap silently hides the tail of any + # larger change, so the reviewer reasons about code it never saw. Instead we + # inline full patches until a total budget is reached, then list the remaining + # files so they can be fetched on demand. + used = 0 + overflow: list[str] = [] + for file in result: filename = file['filename'] status = file['status'] additions = file['additions'] deletions = file['deletions'] changes = file['changes'] - - output += f"📄 **{filename}** ({status})\n" - output += f" +{additions} -{deletions} (~{changes} changes)\n" - - if file.get('patch'): - output += f" Diff:\n" - # Limit diff size to avoid overwhelming output - patch_lines = file['patch'].split('\n') - if len(patch_lines) > 50: - output += '\n'.join(patch_lines[:50]) - output += f"\n ... (truncated, {len(patch_lines) - 50} more lines)\n" - else: - output += file['patch'] - output += "\n" - else: - output += " (Binary file or no diff available)\n" - - output += "\n" - + + header = ( + f"📄 **{filename}** ({status})\n" + f" +{additions} -{deletions} (~{changes} changes)\n" + ) + patch = file.get('patch') + + if not patch: + output += header + " (Binary file or no diff available)\n\n" + continue + + if used + len(patch) > TOTAL_DIFF_BUDGET_CHARS: + overflow.append(f"{filename} (+{additions} -{deletions})") + output += header + " (diff omitted to stay within budget; fetch on demand)\n\n" + continue + + used += len(patch) + output += header + " Diff:\n" + patch + "\n\n" + + if overflow: + listing = "\n".join(f" - {f}" for f in overflow) + output += ( + "Some diffs were omitted to stay within the prompt budget. Fetch their " + "full contents on demand (e.g. with the shell tool) rather than skipping " + "them:\n" + f"{listing}\n" + ) + console.print(Panel(escape(output), title=f"[bold green]PR #{pr_number} Files", border_style="blue")) return output diff --git a/strands-command/scripts/python/tests/test_github_tools.py b/strands-command/scripts/python/tests/test_github_tools.py new file mode 100644 index 0000000..5fe3559 --- /dev/null +++ b/strands-command/scripts/python/tests/test_github_tools.py @@ -0,0 +1,50 @@ +"""Tests for github_tools PR helpers.""" +from unittest.mock import patch + +from .. import github_tools + + +def test_get_pr_files_inlines_full_diff_under_budget(): + """A large patch (well over the old 50-line cap) is inlined in full.""" + big_patch = "\n".join(f"+line {i}" for i in range(120)) + files = [{ + "filename": "a.py", "status": "modified", + "additions": 120, "deletions": 0, "changes": 120, "patch": big_patch, + }] + + with patch.object(github_tools, "_github_request", lambda *a, **k: files): + out = github_tools.get_pr_files(1, repo="o/r") + + assert "truncated" not in out + assert "line 0" in out + assert "line 119" in out # tail of the diff is present, not cut at 50 + + +def test_get_pr_files_lists_overflow_for_on_demand_fetch(): + """When the total diff blows the budget, the file is listed, not silently cut.""" + huge = "\n".join(f"+x{i}" for i in range(50000)) + files = [{ + "filename": "big.py", "status": "modified", + "additions": 50000, "deletions": 0, "changes": 50000, "patch": huge, + }] + + with patch.object(github_tools, "_github_request", lambda *a, **k: files): + out = github_tools.get_pr_files(1, repo="o/r") + + assert "big.py" in out + assert "omitted" in out.lower() + assert "fetch" in out.lower() + + +def test_get_pr_files_handles_binary_file(): + """Files without a patch (binary) are reported, not crashed on.""" + files = [{ + "filename": "logo.png", "status": "added", + "additions": 0, "deletions": 0, "changes": 0, + }] + + with patch.object(github_tools, "_github_request", lambda *a, **k: files): + out = github_tools.get_pr_files(1, repo="o/r") + + assert "logo.png" in out + assert "Binary file or no diff available" in out From e6f5ff49a7af46876530c019a2b8c2facbe6c503 Mon Sep 17 00:00:00 2001 From: Jonathan Segev Date: Thu, 11 Jun 2026 12:55:12 -0400 Subject: [PATCH 2/6] fix(strands-command): inline head of over-budget file instead of dropping it A file whose patch alone exceeds the budget previously produced no inline diff at all, leaving the reviewer dependent on fetching it. Inline a line-trimmed head slice when enough budget remains so there is always some inline signal, and note the omitted remainder is fetchable. Adds tests for the partial-head and mixed inline/overflow cases. --- .../scripts/python/github_tools.py | 43 ++++++++++++++----- .../scripts/python/tests/test_github_tools.py | 36 ++++++++++++++++ 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/strands-command/scripts/python/github_tools.py b/strands-command/scripts/python/github_tools.py index 58fb139..39325f1 100644 --- a/strands-command/scripts/python/github_tools.py +++ b/strands-command/scripts/python/github_tools.py @@ -81,6 +81,11 @@ # remaining files are listed for on-demand fetch instead of being inlined. TOTAL_DIFF_BUDGET_CHARS = 100_000 +# When a single file does not fully fit the remaining budget, inline this much of +# its head (trimmed to a line boundary) so the reviewer always has some inline +# signal, unless less than this remains. +MIN_PARTIAL_HEAD_CHARS = 2_000 + class GitHubOperation(TypedDict): """Type definition for GitHub operation records in JSONL files.""" @@ -533,11 +538,10 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: output = f"Files changed in PR #{pr_number}:\n\n" - # Bound the total diff size we inline into one prompt rather than capping each - # file at a fixed line count: a per-file cap silently hides the tail of any - # larger change, so the reviewer reasons about code it never saw. Instead we - # inline full patches until a total budget is reached, then list the remaining - # files so they can be fetched on demand. + # Bound total inlined diff by a character budget rather than a per-file line + # cap (which silently hid the tail of larger changes). Inline full patches + # while the budget allows; for a file that does not fully fit, inline a head + # slice so there is always some inline signal, and note the rest is fetchable. used = 0 overflow: list[str] = [] @@ -558,13 +562,32 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: output += header + " (Binary file or no diff available)\n\n" continue - if used + len(patch) > TOTAL_DIFF_BUDGET_CHARS: - overflow.append(f"{filename} (+{additions} -{deletions})") - output += header + " (diff omitted to stay within budget; fetch on demand)\n\n" + remaining = TOTAL_DIFF_BUDGET_CHARS - used + + if len(patch) <= remaining: + used += len(patch) + output += header + " Diff:\n" + patch + "\n\n" continue - used += len(patch) - output += header + " Diff:\n" + patch + "\n\n" + # Patch does not fully fit. Inline a head slice trimmed to a line + # boundary if enough budget remains, otherwise defer the whole file. + if remaining >= MIN_PARTIAL_HEAD_CHARS: + head = patch[:remaining] + newline = head.rfind("\n") + if newline != -1: + head = head[:newline] + used += len(head) + omitted = len(patch) - len(head) + overflow.append(f"{filename} (+{additions} -{deletions}, partially shown)") + output += ( + header + + " Diff (head only):\n" + + head + + f"\n ... ({omitted} more chars omitted; fetch full diff on demand)\n\n" + ) + else: + overflow.append(f"{filename} (+{additions} -{deletions})") + output += header + " (diff omitted to stay within budget; fetch on demand)\n\n" if overflow: listing = "\n".join(f" - {f}" for f in overflow) diff --git a/strands-command/scripts/python/tests/test_github_tools.py b/strands-command/scripts/python/tests/test_github_tools.py index 5fe3559..3b293d5 100644 --- a/strands-command/scripts/python/tests/test_github_tools.py +++ b/strands-command/scripts/python/tests/test_github_tools.py @@ -36,6 +36,42 @@ def test_get_pr_files_lists_overflow_for_on_demand_fetch(): assert "fetch" in out.lower() +def test_get_pr_files_inlines_head_of_over_budget_file(): + """A single over-budget file still shows a head slice, not zero diff.""" + huge = "\n".join(f"+line{i}" for i in range(50000)) + files = [{ + "filename": "big.py", "status": "modified", + "additions": 50000, "deletions": 0, "changes": 50000, "patch": huge, + }] + + with patch.object(github_tools, "_github_request", lambda *a, **k: files): + out = github_tools.get_pr_files(1, repo="o/r") + + assert "Diff (head only):" in out + assert "+line0" in out # head content present + assert "more chars omitted" in out + assert "partially shown" in out + + +def test_get_pr_files_mixed_inline_and_overflow(): + """Earlier files are inlined in full while a later large file overflows.""" + small = "\n".join(f"+s{i}" for i in range(5)) + huge = "\n".join(f"+h{i}" for i in range(50000)) + files = [ + {"filename": "small.py", "status": "modified", "additions": 5, + "deletions": 0, "changes": 5, "patch": small}, + {"filename": "huge.py", "status": "modified", "additions": 50000, + "deletions": 0, "changes": 50000, "patch": huge}, + ] + + with patch.object(github_tools, "_github_request", lambda *a, **k: files): + out = github_tools.get_pr_files(1, repo="o/r") + + assert "+s4" in out # small file fully inlined + assert "huge.py" in out + assert "huge.py" in out.split("Some diffs were omitted")[1] # listed in overflow + + def test_get_pr_files_handles_binary_file(): """Files without a patch (binary) are reported, not crashed on.""" files = [{ From b4bd8e646bb814b1485d9eb2e62aad799f0ded8b Mon Sep 17 00:00:00 2001 From: Jonathan Segev Date: Fri, 12 Jun 2026 09:49:22 -0400 Subject: [PATCH 3/6] fix(strands-command): defer files with empty head slice in get_pr_files Guard the partial-head branch so a patch starting with a newline (whose line-boundary trim leaves an empty head) is fully deferred instead of emitting an empty "Diff (head only):" block. Add tests covering the full-deferral branch, the exact budget boundary, and the empty-head edge, merge the two near-duplicate over-budget tests, and note the accepted file-order dependence of the diff budget. --- .../scripts/python/github_tools.py | 9 ++ .../scripts/python/tests/test_github_tools.py | 91 +++++++++++++++---- 2 files changed, 82 insertions(+), 18 deletions(-) diff --git a/strands-command/scripts/python/github_tools.py b/strands-command/scripts/python/github_tools.py index 39325f1..0f9ec7a 100644 --- a/strands-command/scripts/python/github_tools.py +++ b/strands-command/scripts/python/github_tools.py @@ -542,6 +542,9 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: # cap (which silently hid the tail of larger changes). Inline full patches # while the budget allows; for a file that does not fully fit, inline a head # slice so there is always some inline signal, and note the rest is fetchable. + # The budget is consumed in GitHub API file order, so a large early file can + # push later files into overflow; this order dependence is accepted (we do not + # sort, to keep inline output in the same order the API returns). used = 0 overflow: list[str] = [] @@ -571,11 +574,14 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: # Patch does not fully fit. Inline a head slice trimmed to a line # boundary if enough budget remains, otherwise defer the whole file. + head = "" if remaining >= MIN_PARTIAL_HEAD_CHARS: head = patch[:remaining] newline = head.rfind("\n") if newline != -1: head = head[:newline] + + if head: used += len(head) omitted = len(patch) - len(head) overflow.append(f"{filename} (+{additions} -{deletions}, partially shown)") @@ -586,6 +592,9 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: + f"\n ... ({omitted} more chars omitted; fetch full diff on demand)\n\n" ) else: + # Either too little budget remains for a head slice, or trimming to a + # line boundary left an empty head (patch starts with a newline); + # defer the whole file rather than emit an empty "head only" diff. overflow.append(f"{filename} (+{additions} -{deletions})") output += header + " (diff omitted to stay within budget; fetch on demand)\n\n" diff --git a/strands-command/scripts/python/tests/test_github_tools.py b/strands-command/scripts/python/tests/test_github_tools.py index 3b293d5..40fcc3f 100644 --- a/strands-command/scripts/python/tests/test_github_tools.py +++ b/strands-command/scripts/python/tests/test_github_tools.py @@ -2,6 +2,7 @@ from unittest.mock import patch from .. import github_tools +from ..github_tools import MIN_PARTIAL_HEAD_CHARS, TOTAL_DIFF_BUDGET_CHARS def test_get_pr_files_inlines_full_diff_under_budget(): @@ -20,24 +21,8 @@ def test_get_pr_files_inlines_full_diff_under_budget(): assert "line 119" in out # tail of the diff is present, not cut at 50 -def test_get_pr_files_lists_overflow_for_on_demand_fetch(): - """When the total diff blows the budget, the file is listed, not silently cut.""" - huge = "\n".join(f"+x{i}" for i in range(50000)) - files = [{ - "filename": "big.py", "status": "modified", - "additions": 50000, "deletions": 0, "changes": 50000, "patch": huge, - }] - - with patch.object(github_tools, "_github_request", lambda *a, **k: files): - out = github_tools.get_pr_files(1, repo="o/r") - - assert "big.py" in out - assert "omitted" in out.lower() - assert "fetch" in out.lower() - - def test_get_pr_files_inlines_head_of_over_budget_file(): - """A single over-budget file still shows a head slice, not zero diff.""" + """A single over-budget file shows a head slice and is listed for on-demand fetch.""" huge = "\n".join(f"+line{i}" for i in range(50000)) files = [{ "filename": "big.py", "status": "modified", @@ -50,7 +35,8 @@ def test_get_pr_files_inlines_head_of_over_budget_file(): assert "Diff (head only):" in out assert "+line0" in out # head content present assert "more chars omitted" in out - assert "partially shown" in out + assert "partially shown" in out # noted in overflow listing + assert "fetch" in out.lower() # on-demand fetch instruction present def test_get_pr_files_mixed_inline_and_overflow(): @@ -72,6 +58,75 @@ def test_get_pr_files_mixed_inline_and_overflow(): assert "huge.py" in out.split("Some diffs were omitted")[1] # listed in overflow +def test_get_pr_files_defers_whole_file_when_budget_nearly_exhausted(): + """A later file is fully deferred (not partially shown) once = MIN_PARTIAL_HEAD_CHARS + # Second file is fully deferred, not shown with an empty head. + assert "(diff omitted to stay within budget" in out + # No empty "Diff (head only):" block (header immediately followed by the omitted line). + assert "Diff (head only):\n\n ..." not in out + + def test_get_pr_files_handles_binary_file(): """Files without a patch (binary) are reported, not crashed on.""" files = [{ From 52c3ecf68fdf924b07a6f8a9e5ecce0105b1f06a Mon Sep 17 00:00:00 2001 From: Jonathan Segev Date: Mon, 22 Jun 2026 09:21:07 -0400 Subject: [PATCH 4/6] refactor(strands-command): offload large tool results via ContextOffloader Replace the bespoke per-call diff budgeting in get_pr_files with the SDK's ContextOffloader plugin. get_pr_files now returns the full untruncated diff; the plugin offloads any oversized tool result (diffs, file reads, shell output) to storage and registers a retrieval tool so the agent fetches the full content on demand. This handles truncation generically for every tool rather than just this one, per review feedback. --- .../scripts/python/agent_runner.py | 13 +- .../scripts/python/github_tools.py | 68 +--------- .../scripts/python/tests/test_github_tools.py | 123 ++++-------------- 3 files changed, 40 insertions(+), 164 deletions(-) diff --git a/strands-command/scripts/python/agent_runner.py b/strands-command/scripts/python/agent_runner.py index 9e93835..2fa10c9 100644 --- a/strands-command/scripts/python/agent_runner.py +++ b/strands-command/scripts/python/agent_runner.py @@ -17,6 +17,7 @@ from strands.agent.conversation_manager import SlidingWindowConversationManager from strands.session import S3SessionManager from strands.models import BedrockModel, CacheConfig +from strands.vended_plugins.context_offloader import ContextOffloader, InMemoryStorage from botocore.config import Config from strands_tools import http_request, shell @@ -229,17 +230,25 @@ def run_agent(query: str): else: raise ValueError("Both SESSION_ID and S3_SESSION_BUCKET must be set") + # Offload oversized tool results (large PR diffs, file reads, shell output) + # to storage instead of letting them crowd out the context window. The + # plugin registers a retrieval tool so the agent can fetch the full content + # on demand. In-memory storage is sufficient: the agent runs as a single + # process per invocation, so offloaded content only needs to outlive the run. + context_offloader = ContextOffloader(storage=InMemoryStorage()) + # Create agent with optional trace attributes for Langfuse agent_kwargs = { "model": model, "system_prompt": system_prompt, "tools": tools, "session_manager": session_manager, + "plugins": [context_offloader], } - + if trace_attributes: agent_kwargs["trace_attributes"] = trace_attributes - + agent = Agent(**agent_kwargs) print("Processing user query...") diff --git a/strands-command/scripts/python/github_tools.py b/strands-command/scripts/python/github_tools.py index 0f9ec7a..76ec33f 100644 --- a/strands-command/scripts/python/github_tools.py +++ b/strands-command/scripts/python/github_tools.py @@ -77,15 +77,6 @@ console = console_util.create() -# Total characters of PR diff inlined into a single prompt by get_pr_files before -# remaining files are listed for on-demand fetch instead of being inlined. -TOTAL_DIFF_BUDGET_CHARS = 100_000 - -# When a single file does not fully fit the remaining budget, inline this much of -# its head (trimmed to a line boundary) so the reviewer always has some inline -# signal, unless less than this remains. -MIN_PARTIAL_HEAD_CHARS = 2_000 - class GitHubOperation(TypedDict): """Type definition for GitHub operation records in JSONL files.""" @@ -538,16 +529,10 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: output = f"Files changed in PR #{pr_number}:\n\n" - # Bound total inlined diff by a character budget rather than a per-file line - # cap (which silently hid the tail of larger changes). Inline full patches - # while the budget allows; for a file that does not fully fit, inline a head - # slice so there is always some inline signal, and note the rest is fetchable. - # The budget is consumed in GitHub API file order, so a large early file can - # push later files into overflow; this order dependence is accepted (we do not - # sort, to keep inline output in the same order the API returns). - used = 0 - overflow: list[str] = [] - + # Return each file's full patch. Large results are managed centrally by the + # ContextOffloader plugin (configured on the agent), which offloads oversized + # tool results to storage and gives the agent a tool to retrieve them on + # demand -- so this function does not truncate diffs itself. for file in result: filename = file['filename'] status = file['status'] @@ -561,51 +546,10 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: ) patch = file.get('patch') - if not patch: - output += header + " (Binary file or no diff available)\n\n" - continue - - remaining = TOTAL_DIFF_BUDGET_CHARS - used - - if len(patch) <= remaining: - used += len(patch) + if patch: output += header + " Diff:\n" + patch + "\n\n" - continue - - # Patch does not fully fit. Inline a head slice trimmed to a line - # boundary if enough budget remains, otherwise defer the whole file. - head = "" - if remaining >= MIN_PARTIAL_HEAD_CHARS: - head = patch[:remaining] - newline = head.rfind("\n") - if newline != -1: - head = head[:newline] - - if head: - used += len(head) - omitted = len(patch) - len(head) - overflow.append(f"{filename} (+{additions} -{deletions}, partially shown)") - output += ( - header - + " Diff (head only):\n" - + head - + f"\n ... ({omitted} more chars omitted; fetch full diff on demand)\n\n" - ) else: - # Either too little budget remains for a head slice, or trimming to a - # line boundary left an empty head (patch starts with a newline); - # defer the whole file rather than emit an empty "head only" diff. - overflow.append(f"{filename} (+{additions} -{deletions})") - output += header + " (diff omitted to stay within budget; fetch on demand)\n\n" - - if overflow: - listing = "\n".join(f" - {f}" for f in overflow) - output += ( - "Some diffs were omitted to stay within the prompt budget. Fetch their " - "full contents on demand (e.g. with the shell tool) rather than skipping " - "them:\n" - f"{listing}\n" - ) + output += header + " (Binary file or no diff available)\n\n" console.print(Panel(escape(output), title=f"[bold green]PR #{pr_number} Files", border_style="blue")) return output diff --git a/strands-command/scripts/python/tests/test_github_tools.py b/strands-command/scripts/python/tests/test_github_tools.py index 40fcc3f..02e34c3 100644 --- a/strands-command/scripts/python/tests/test_github_tools.py +++ b/strands-command/scripts/python/tests/test_github_tools.py @@ -2,11 +2,15 @@ from unittest.mock import patch from .. import github_tools -from ..github_tools import MIN_PARTIAL_HEAD_CHARS, TOTAL_DIFF_BUDGET_CHARS -def test_get_pr_files_inlines_full_diff_under_budget(): - """A large patch (well over the old 50-line cap) is inlined in full.""" +def test_get_pr_files_returns_full_diff_untruncated(): + """The full patch is returned, with no per-file line cap. + + Guards the original bug: get_pr_files used to truncate each file's patch to + 50 lines, hiding the tail. Oversized results are now managed by the agent's + ContextOffloader plugin rather than truncated here. + """ big_patch = "\n".join(f"+line {i}" for i in range(120)) files = [{ "filename": "a.py", "status": "modified", @@ -18,113 +22,24 @@ def test_get_pr_files_inlines_full_diff_under_budget(): assert "truncated" not in out assert "line 0" in out - assert "line 119" in out # tail of the diff is present, not cut at 50 - - -def test_get_pr_files_inlines_head_of_over_budget_file(): - """A single over-budget file shows a head slice and is listed for on-demand fetch.""" - huge = "\n".join(f"+line{i}" for i in range(50000)) - files = [{ - "filename": "big.py", "status": "modified", - "additions": 50000, "deletions": 0, "changes": 50000, "patch": huge, - }] + assert "line 119" in out # tail is present, not cut at 50 - with patch.object(github_tools, "_github_request", lambda *a, **k: files): - out = github_tools.get_pr_files(1, repo="o/r") - - assert "Diff (head only):" in out - assert "+line0" in out # head content present - assert "more chars omitted" in out - assert "partially shown" in out # noted in overflow listing - assert "fetch" in out.lower() # on-demand fetch instruction present - -def test_get_pr_files_mixed_inline_and_overflow(): - """Earlier files are inlined in full while a later large file overflows.""" - small = "\n".join(f"+s{i}" for i in range(5)) - huge = "\n".join(f"+h{i}" for i in range(50000)) +def test_get_pr_files_inlines_every_changed_file(): + """All changed files are inlined in full, in API order.""" files = [ {"filename": "small.py", "status": "modified", "additions": 5, - "deletions": 0, "changes": 5, "patch": small}, - {"filename": "huge.py", "status": "modified", "additions": 50000, - "deletions": 0, "changes": 50000, "patch": huge}, + "deletions": 0, "changes": 5, "patch": "\n".join(f"+s{i}" for i in range(5))}, + {"filename": "big.py", "status": "modified", "additions": 5000, + "deletions": 0, "changes": 5000, "patch": "\n".join(f"+b{i}" for i in range(5000))}, ] with patch.object(github_tools, "_github_request", lambda *a, **k: files): out = github_tools.get_pr_files(1, repo="o/r") - assert "+s4" in out # small file fully inlined - assert "huge.py" in out - assert "huge.py" in out.split("Some diffs were omitted")[1] # listed in overflow - - -def test_get_pr_files_defers_whole_file_when_budget_nearly_exhausted(): - """A later file is fully deferred (not partially shown) once = MIN_PARTIAL_HEAD_CHARS - # Second file is fully deferred, not shown with an empty head. - assert "(diff omitted to stay within budget" in out - # No empty "Diff (head only):" block (header immediately followed by the omitted line). - assert "Diff (head only):\n\n ..." not in out def test_get_pr_files_handles_binary_file(): @@ -139,3 +54,11 @@ def test_get_pr_files_handles_binary_file(): assert "logo.png" in out assert "Binary file or no diff available" in out + + +def test_get_pr_files_returns_error_string_on_request_failure(): + """A request error is surfaced as-is rather than formatted as a file list.""" + with patch.object(github_tools, "_github_request", lambda *a, **k: "Error: boom"): + out = github_tools.get_pr_files(1, repo="o/r") + + assert out == "Error: boom" From 7cf0161d1a2cc9dbba15deb55d32d393f6e55599 Mon Sep 17 00:00:00 2001 From: Jonathan Segev Date: Mon, 22 Jun 2026 12:12:19 -0400 Subject: [PATCH 5/6] fix(strands-command): use S3 storage for offloaded results so resumed sessions resolve references --- strands-command/scripts/python/agent_runner.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/strands-command/scripts/python/agent_runner.py b/strands-command/scripts/python/agent_runner.py index 2fa10c9..9787d1a 100644 --- a/strands-command/scripts/python/agent_runner.py +++ b/strands-command/scripts/python/agent_runner.py @@ -17,7 +17,7 @@ from strands.agent.conversation_manager import SlidingWindowConversationManager from strands.session import S3SessionManager from strands.models import BedrockModel, CacheConfig -from strands.vended_plugins.context_offloader import ContextOffloader, InMemoryStorage +from strands.vended_plugins.context_offloader import ContextOffloader, S3Storage from botocore.config import Config from strands_tools import http_request, shell @@ -233,9 +233,13 @@ def run_agent(query: str): # Offload oversized tool results (large PR diffs, file reads, shell output) # to storage instead of letting them crowd out the context window. The # plugin registers a retrieval tool so the agent can fetch the full content - # on demand. In-memory storage is sufficient: the agent runs as a single - # process per invocation, so offloaded content only needs to outlive the run. - context_offloader = ContextOffloader(storage=InMemoryStorage()) + # on demand. Use S3 storage rather than in-memory: the offloaded result is + # replaced in the conversation with a storage reference that the session + # manager persists, so a resumed session must still be able to resolve that + # reference in a later process — which in-memory storage could not. + context_offloader = ContextOffloader( + storage=S3Storage(bucket=s3_bucket, prefix=f"{s3_prefix}/offload"), + ) # Create agent with optional trace attributes for Langfuse agent_kwargs = { From e746a38e59e0a2331c1e168a5e683ea5e46df250 Mon Sep 17 00:00:00 2001 From: Jonathan Segev Date: Mon, 22 Jun 2026 12:27:23 -0400 Subject: [PATCH 6/6] fix(strands-command): paginate get_pr_files so all changed files are reviewed GitHub list endpoints cap at 100 items per page, so reading a single response silently dropped changed files past the first page on large PRs -- the reviewer would never see them. Add a paginated GET helper that follows Link rel="next" (bounded by a page cap) and use it for the PR files list. --- .../scripts/python/github_tools.py | 57 ++++++++++++++- .../scripts/python/tests/test_github_tools.py | 70 +++++++++++++++++-- 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/strands-command/scripts/python/github_tools.py b/strands-command/scripts/python/github_tools.py index 76ec33f..bde426a 100644 --- a/strands-command/scripts/python/github_tools.py +++ b/strands-command/scripts/python/github_tools.py @@ -151,6 +151,61 @@ def _github_request( return f"Error {e!s}" +# Max pages to follow when fetching a paginated list endpoint, as a safety bound +# (100 items/page x 30 pages = 3000 items) against an unbounded follow loop. +_MAX_LIST_PAGES = 30 + + +def _github_get_all_pages(endpoint: str, repo: str | None = None) -> list[dict[str, Any]] | str: + """GET a paginated list endpoint, following Link rel="next" until exhausted. + + GitHub list endpoints (e.g. a PR's files) return at most 100 items per page, + so reading a single response silently drops the rest. This follows the + pagination links and concatenates every page. + + Args: + endpoint: API endpoint path (e.g., "pulls/123/files") + repo: Repository in "owner/repo" format + + Returns: + The combined list of items, or an error string on failure. + """ + if repo is None: + repo = os.environ.get("GITHUB_REPOSITORY") + if not repo: + return "Error: GITHUB_REPOSITORY environment variable not found" + + token = os.environ.get("GITHUB_TOKEN", "") + if not token: + return "Error: GITHUB_TOKEN environment variable not found" + + headers = { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github.v3+json", + } + url: str | None = f"https://api.github.com/repos/{repo}/{endpoint}" + params: dict | None = {"per_page": 100} + items: list[dict[str, Any]] = [] + + try: + for _ in range(_MAX_LIST_PAGES): + response = requests.get(url, headers=headers, params=params, timeout=30) + response.raise_for_status() + page = response.json() + if not isinstance(page, list): + return f"Error: expected a list from {endpoint}, got {type(page).__name__}" + items.extend(page) + # The "next" link already carries the page/per_page query params. + url = response.links.get("next", {}).get("url") + params = None + if not url: + return items + # Hit the page cap; return what we have rather than looping unbounded. + return items + except Exception as e: + return f"Error {e!s}" + + def check_should_call_write_api_or_record(func): """Decorator that checks if a write api should be called, or if the tool should record to JSONL.""" @wraps(func) @@ -522,7 +577,7 @@ def get_pr_files(pr_number: int, repo: str | None = None) -> str: Returns: Formatted list of changed files with their diffs """ - result = _github_request("GET", f"pulls/{pr_number}/files", repo) + result = _github_get_all_pages(f"pulls/{pr_number}/files", repo) if isinstance(result, str): console.print(Panel(escape(result), title="[bold red]Error", border_style="red")) return result diff --git a/strands-command/scripts/python/tests/test_github_tools.py b/strands-command/scripts/python/tests/test_github_tools.py index 02e34c3..f928a1e 100644 --- a/strands-command/scripts/python/tests/test_github_tools.py +++ b/strands-command/scripts/python/tests/test_github_tools.py @@ -1,6 +1,8 @@ """Tests for github_tools PR helpers.""" from unittest.mock import patch +import requests + from .. import github_tools @@ -17,7 +19,7 @@ def test_get_pr_files_returns_full_diff_untruncated(): "additions": 120, "deletions": 0, "changes": 120, "patch": big_patch, }] - with patch.object(github_tools, "_github_request", lambda *a, **k: files): + with patch.object(github_tools, "_github_get_all_pages", lambda *a, **k: files): out = github_tools.get_pr_files(1, repo="o/r") assert "truncated" not in out @@ -34,7 +36,7 @@ def test_get_pr_files_inlines_every_changed_file(): "deletions": 0, "changes": 5000, "patch": "\n".join(f"+b{i}" for i in range(5000))}, ] - with patch.object(github_tools, "_github_request", lambda *a, **k: files): + with patch.object(github_tools, "_github_get_all_pages", lambda *a, **k: files): out = github_tools.get_pr_files(1, repo="o/r") assert "small.py" in out and "+s4" in out @@ -49,7 +51,7 @@ def test_get_pr_files_handles_binary_file(): "additions": 0, "deletions": 0, "changes": 0, }] - with patch.object(github_tools, "_github_request", lambda *a, **k: files): + with patch.object(github_tools, "_github_get_all_pages", lambda *a, **k: files): out = github_tools.get_pr_files(1, repo="o/r") assert "logo.png" in out @@ -58,7 +60,67 @@ def test_get_pr_files_handles_binary_file(): def test_get_pr_files_returns_error_string_on_request_failure(): """A request error is surfaced as-is rather than formatted as a file list.""" - with patch.object(github_tools, "_github_request", lambda *a, **k: "Error: boom"): + with patch.object(github_tools, "_github_get_all_pages", lambda *a, **k: "Error: boom"): out = github_tools.get_pr_files(1, repo="o/r") assert out == "Error: boom" + + +class _FakeResponse: + """Minimal requests.Response stand-in for pagination tests.""" + + def __init__(self, payload, next_url=None): + self._payload = payload + self.links = {"next": {"url": next_url}} if next_url else {} + + def raise_for_status(self): + pass + + def json(self): + return self._payload + + +def test_get_pr_files_follows_pagination_across_pages(monkeypatch): + """All pages of a PR's files are fetched, not just the first. + + Guards against silently reviewing only the first ~30/100 changed files on + large PRs. The first page advertises a Link rel="next"; the helper must + follow it and concatenate every page. + """ + monkeypatch.setenv("GITHUB_TOKEN", "t") + page1 = [{"filename": f"p1_{i}.py", "status": "modified", "additions": 1, + "deletions": 0, "changes": 1, "patch": f"+a{i}"} for i in range(100)] + page2 = [{"filename": "p2_final.py", "status": "modified", "additions": 1, + "deletions": 0, "changes": 1, "patch": "+final"}] + responses = [ + _FakeResponse(page1, next_url="https://api.github.com/next-page"), + _FakeResponse(page2), + ] + + def fake_get(url, **kwargs): + return responses.pop(0) + + monkeypatch.setattr(requests, "get", fake_get) + out = github_tools.get_pr_files(1, repo="o/r") + + assert "p1_0.py" in out + assert "p1_99.py" in out + assert "p2_final.py" in out # file from the second page is present + assert not responses # both pages were consumed + + +def test_get_pr_files_caps_pagination(monkeypatch): + """An endpoint that always advertises a next page is bounded, not infinite.""" + monkeypatch.setenv("GITHUB_TOKEN", "t") + + def fake_get(url, **kwargs): + return _FakeResponse( + [{"filename": "f.py", "status": "modified", "additions": 1, + "deletions": 0, "changes": 1, "patch": "+x"}], + next_url="https://api.github.com/always-next", + ) + + monkeypatch.setattr(requests, "get", fake_get) + # Should terminate (at the page cap) rather than hang. + out = github_tools.get_pr_files(1, repo="o/r") + assert "f.py" in out