mirror of
https://github.com/usestrix/strix.git
synced 2026-10-01 02:03:55 +00:00
Scope fix validation and warn on repeated commands
This commit is contained in:
parent
77bd5dade5
commit
868ba53927
7 changed files with 191 additions and 8 deletions
2
Makefile
2
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
|
||||
|
|
|
|||
|
|
@ -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` /
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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],
|
||||
|
|
|
|||
108
tests/test_fix_repetition.py
Normal file
108
tests/test_fix_repetition.py
Normal file
|
|
@ -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
|
||||
Loading…
Add table
Reference in a new issue