From 868ba539279f7416c04b4ff9abbddeb63d5b3968 Mon Sep 17 00:00:00 2001 From: Jonathan Singer Date: Wed, 30 Sep 2026 03:47:36 -0400 Subject: [PATCH] Scope fix validation and warn on repeated commands --- Makefile | 2 +- docs/fix-preparation.md | 21 ++++- strix/agents/prompts/fix_repair.jinja | 6 +- strix/agents/prompts/fix_review.jinja | 11 ++- strix/agents/prompts/fix_workspace.jinja | 5 ++ strix/fix/runtime.py | 46 ++++++++++ tests/test_fix_repetition.py | 108 +++++++++++++++++++++++ 7 files changed, 191 insertions(+), 8 deletions(-) create mode 100644 tests/test_fix_repetition.py diff --git a/Makefile b/Makefile index d9ef04ee2..bb7a3592d 100644 --- a/Makefile +++ b/Makefile @@ -96,4 +96,4 @@ tui-lint: .PHONY: test-fix-reliability test-fix-reliability: - uv run pytest tests/test_fix_preparation.py tests/test_fix_completion.py tests/test_fix_reliability.py tests/test_fix_runtime.py tests/test_fix_cli.py -q + uv run pytest tests/test_fix_preparation.py tests/test_fix_completion.py tests/test_fix_reliability.py tests/test_fix_runtime.py tests/test_fix_cli.py tests/test_fix_repetition.py -q diff --git a/docs/fix-preparation.md b/docs/fix-preparation.md index 7fbe366a2..fbaf2f09d 100644 --- a/docs/fix-preparation.md +++ b/docs/fix-preparation.md @@ -10,7 +10,8 @@ and available reproduction details. It makes a minimal fix, adds a regression te using the repository's framework, and hands test locations, commands, results, and failed approaches to review. Once it understands the affected path, it starts the change rather than expanding the investigation. It preserves legitimate behavior, -not the behavior that enables the vulnerability. +not the behavior that enables the vulnerability. Once its focused regression passes, +repair hands off rather than expanding into the full customer suite. Review receives the finding, patch, repair summary, and command history. It runs the customer's relevant existing unit tests and the regression test, then judges whether the change addresses the issue without obvious regressions. It challenges @@ -18,13 +19,27 @@ the repair's central assumption with the strongest plausible bypass and checks legitimate behavior. Required tests must pass, exercise the actual security decision, and include any helpers needed to reproduce them in the delivered patch. The reviewer can make small corrections and rerun affected tests. Optional hardening is follow-up -work; a remaining path to the reported attack is not optional. +work; a remaining path to the reported attack is not optional. Existing customer unit +tests remain mandatory: start with the changed component and its direct consumers. +Run the full suite only when small or justified by broad effects, explaining the +reason before starting. Finish once the relevant tests pass, the attack is blocked, +and legitimate use works; additional reassurance alone is not a reason to expand. Both agents use documented setup and targeted recovery, avoid repeating failed experiments without a new hypothesis, and hand off or report a blocker when they -cannot progress. Test commands must retain their actual exit status. These are +cannot progress. Unrelated failures are investigated enough to establish a baseline +and then documented, without taking on repair of the entire test environment. Required +validation that remains blocked is reported as a blocker. Test commands must retain +their actual exit status. These are agent instructions, not a separate controller that selects or interprets tests. +The existing fix hooks also warn when the same completed command, directory, shell, +exit status, and process output recur three times within twelve recent completed +commands. Timing and chunk IDs are excluded from the comparison. Changed results +reset that command's history; native patch calls reset the window. Running commands +and `write_stdin` polling are excluded. The warning asks the agent to change approach +or hand off; it never blocks a tool, waives tests, or decides the review outcome. + ## Completion and handoffs Before preparation, a source-backed scan report must supply paired `fix_before` / diff --git a/strix/agents/prompts/fix_repair.jinja b/strix/agents/prompts/fix_repair.jinja index 4c16d9a02..e02bde11f 100644 --- a/strix/agents/prompts/fix_repair.jinja +++ b/strix/agents/prompts/fix_repair.jinja @@ -7,9 +7,13 @@ Once you understand the affected path, make the change. Add a focused regression test using the repository's existing framework and run it. Exercise the real security decision without building a larger test environment than necessary. +Once the focused regression passes, hand off to review. Review owns the existing +customer unit tests; do not expand into the full unit, integration, or end-to-end +suite before handing off. + Hand the patch, test locations, commands, results, and remaining blockers to review. Include failed approaches so the reviewer can continue without repeating your -investigation. Review owns the relevant existing customer unit suite. +investigation. Call agent_finish with result_summary and outcome done when the patch is ready for review, or blocked when you cannot continue. Put unresolved limitations and required diff --git a/strix/agents/prompts/fix_review.jinja b/strix/agents/prompts/fix_review.jinja index 6110cef92..773307530 100644 --- a/strix/agents/prompts/fix_review.jinja +++ b/strix/agents/prompts/fix_review.jinja @@ -3,12 +3,17 @@ repair's central assumption: could an attacker still achieve the same harm with different inputs or through an allowed branch? Check the strongest plausible bypass and legitimate behavior through the relevant application code. -Run the customer's relevant existing unit tests and the new regression test. +Run the new regression and the customer's existing unit tests covering the changed +component and its direct consumers. These tests are required. Run the full suite +only when it is small or the change has broad effects that require it; explain why +broader testing is necessary before starting it. + Do not mock away the security decision being checked. Confirm that helpers needed to run the regression are included in the patch. -Approve when the required tests pass and the change addresses the reported issue -without obvious regressions. If existing tests assert the vulnerable behavior, +Finish when these tests pass, the reported attack is blocked, and legitimate use +still works. Do not expand testing merely for additional reassurance. +If existing tests assert the vulnerable behavior, update their expectations while preserving meaningful coverage; do not simply dismiss their failures. diff --git a/strix/agents/prompts/fix_workspace.jinja b/strix/agents/prompts/fix_workspace.jinja index 9b44ababd..e779e1124 100644 --- a/strix/agents/prompts/fix_workspace.jinja +++ b/strix/agents/prompts/fix_workspace.jinja @@ -3,6 +3,11 @@ and test setup. Install needed dependencies, but avoid turning unrelated infrastructure failures into another development project. Attempt a targeted recovery; if still blocked, preserve the work and explain what is needed. +Investigate unrelated test failures only enough to establish whether they occur +without the fix, then document them. Do not repair the repository's entire test +environment. If required validation remains blocked, preserve the patch and report +the blocker rather than claiming approval. + Do not repeat an experiment without a new hypothesis or a relevant change. When attempts stop producing useful evidence, simplify the approach, hand off, or report a blocker. diff --git a/strix/fix/runtime.py b/strix/fix/runtime.py index de7b4234b..3e7d09908 100644 --- a/strix/fix/runtime.py +++ b/strix/fix/runtime.py @@ -15,6 +15,7 @@ import subprocess import tempfile import uuid import zipfile +from collections import deque from dataclasses import dataclass, field, replace from pathlib import Path from typing import TYPE_CHECKING, Any, cast @@ -78,6 +79,8 @@ if TYPE_CHECKING: from agents.sandbox.session import BaseSandboxSession _MAX_TOOL_OUTPUT_CHARS = 30_000 +_REPEAT_WINDOW = 12 +_REPEAT_THRESHOLD = 3 def _output_text(text: str, *, max_chars: int | None = _MAX_TOOL_OUTPUT_CHARS) -> str: @@ -94,6 +97,8 @@ class _FixHooks(ReportUsageHooks): self.review = review self.turns = 0 self.completion_digest: str | None = None + self._recent_commands: deque[tuple[str, int, str]] = deque(maxlen=_REPEAT_WINDOW) + self._repetition_warning = False async def on_llm_start( self, context: Any, agent: Any, system_prompt: Any, input_items: Any @@ -107,6 +112,40 @@ class _FixHooks(ReportUsageHooks): raise MaxTurnsExceeded("The agent turn budget was reached.") self.turns += 1 await super().on_llm_start(context, agent, system_prompt, input_items) + if self._repetition_warning: + input_items.append( + { + "role": "user", + "content": ( + "[Repeated command] The same completed command returned the same exit " + f"status and output {_REPEAT_THRESHOLD} times in your recent commands. " + "Change approach or hand off the work and results. If required validation " + "is blocked, report the blocker. Do not repeat the command without a " + "relevant change or a concrete new hypothesis." + ), + } + ) + self._repetition_warning = False + + def _track_repetition(self, command: dict[str, Any], exit_code: int, output: str) -> None: + identity = json.dumps( + ( + command.get("cmd"), + command.get("workdir") or self.environment.session.state.manifest.root, + command.get("shell") or "bash", + command.get("login", True), + ) + ) + entry = (identity, exit_code, hashlib.sha256(output.encode()).hexdigest()) + # A changed result is new evidence. Forget earlier outcomes for that command. + self._recent_commands = deque( + (old for old in self._recent_commands if old[0] != identity or old == entry), + maxlen=_REPEAT_WINDOW, + ) + self._recent_commands.append(entry) + if self._recent_commands.count(entry) >= _REPEAT_THRESHOLD: + self._repetition_warning = True + self._recent_commands.clear() def _turns_used(self, _context: RunContextWrapper[dict[str, Any]], /) -> int: # SDK usage starts over when review sends repair feedback; our counter does not. @@ -152,6 +191,10 @@ class _FixHooks(ReportUsageHooks): } with (env.workspace.parent / "fix-tool-results.jsonl").open("a") as stream: stream.write(json.dumps(event) + "\n") + if context.tool_name == "apply_patch": + self._recent_commands.clear() + self._repetition_warning = False + return if context.tool_name == "agent_finish": try: completion = json.loads(raw) @@ -182,6 +225,9 @@ class _FixHooks(ReportUsageHooks): elif context.tool_name == "write_stdin": env.pending_commands.pop(arguments["session_id"], None) exit_code = int(code[1]) if code else None + # Only immediately completed commands: polling/running processes are not repetition. + if context.tool_name == "exec_command" and exit_code is not None: + self._track_repetition(command, exit_code, output) env.record_command( CheckResult( name=str(command.get("cmd", context.tool_name))[:200], diff --git a/tests/test_fix_repetition.py b/tests/test_fix_repetition.py new file mode 100644 index 000000000..8babe27a0 --- /dev/null +++ b/tests/test_fix_repetition.py @@ -0,0 +1,108 @@ +"""Repetition warnings reach native agents without blocking testing or process polling.""" + +from __future__ import annotations + +import json +from typing import Any + +import pytest +from agents import Agent +from agents.tool_context import ToolContext + +from strix.fix import PreparationState +from strix.fix.runtime import _FixHooks +from tests.test_fix_completion import ScriptedModel, finish, patch, scenario, shell, suite_commands +from tests.test_fix_reliability import environment + + +@pytest.mark.asyncio +@pytest.mark.parametrize("review", [False, True]) +async def test_repeated_native_command_warns_then_agent_can_finish(tmp_path, monkeypatch, review): + # Interleaved commands and different SDK chunk IDs must not hide the repeated read. + repeated = [ + shell("cat app.py"), + shell("pwd"), + shell("cat app.py"), + shell("ls tests"), + shell("cat app.py"), + ] + model = ScriptedModel( + [*patch(), *([] if review else repeated), finish("done")], + [*(repeated if review else []), *suite_commands(), finish("approved")], + ) + result, _ = await scenario(tmp_path, monkeypatch, model) + assert result.state is PreparationState.READY, result.model_dump_json() + role = "review" if review else "repair" + assert any("[Repeated command]" in json.dumps(turn) for turn in model.inputs[role]) + # The warning does not rewrite command evidence or replace required test execution. + assert all(check.exit_code == 0 for check in result.checks) + assert all("Ran 1 test" in check.output for check in result.checks[-2:]) + + +async def tool_result( + hooks: _FixHooks, + tool: str = "exec_command", + *, + output: str = "unchanged", + code: int | None = 0, + workdir: str | None = None, + session_id: int = 42, +) -> None: + arguments: dict[str, Any] = {"cmd": "cat state.txt", "workdir": workdir} + if tool == "write_stdin": + arguments = {"session_id": session_id, "chars": ""} + context = ToolContext( + context={"agent_id": "repair"}, + tool_name=tool, + tool_call_id="call", + tool_arguments=json.dumps(arguments), + ) + state = ( + f"Process exited with code {code}" + if code is not None + else f"Process running with session ID {session_id}" + ) + result = f"Chunk ID: chunk\nWall time: 0.1 seconds\n{state}\nOutput:\n{output}" + await hooks.on_tool_end(context, Agent(name="repair"), None, result) + + +@pytest.mark.asyncio +async def test_running_commands_and_polling_never_trigger_repetition(tmp_path): + hooks = _FixHooks(environment(tmp_path / "repository", tmp_path)) + for _ in range(5): + await tool_result(hooks, code=None) + await tool_result(hooks, "write_stdin", code=None) + await tool_result(hooks, "write_stdin", code=0) + assert not hooks._repetition_warning + assert len(hooks.environment.repair_checks) == 15 + + +@pytest.mark.asyncio +async def test_changed_output_status_or_directory_is_not_repetition(tmp_path): + hooks = _FixHooks(environment(tmp_path / "repository", tmp_path)) + for output, code, workdir in [ + ("old", 0, None), + ("old", 0, None), + ("new", 0, None), + ("new", 0, None), + ("old", 0, None), + ("old", 0, None), + ("old", 1, None), + ("old", 1, None), + ("old", 1, "/another-component"), + ("old", 1, "/another-component"), + ]: + await tool_result(hooks, output=output, code=code, workdir=workdir) + assert not hooks._repetition_warning + + +@pytest.mark.asyncio +async def test_native_patch_resets_repetition_before_revalidation(tmp_path): + hooks = _FixHooks(environment(tmp_path / "repository", tmp_path)) + for _ in range(3): + await tool_result(hooks) + assert hooks._repetition_warning + await tool_result(hooks, "apply_patch") + for _ in range(2): + await tool_result(hooks) + assert not hooks._repetition_warning