Skip to content

Commit 1a5049c

Browse files
agentscope-ai-javazhansheng.lzsluk384090-cloud
committed
acli: sync 0.6.6 agent-loop hardening and fix the retry budget (#197)
* sync(acli): agent-loop hardening A1-A4 from agenticCLI Vendors release/agentic-cli-5 (851c3a7..8ba15e5) plus the 0.6.5 version bump. A1 read_file now windows to 200 lines by default under a 100k-char cap, takes a 1-based offset matching the line numbers it prints, and ends with the omitted count and the offset that continues from there. A2 Tool success is a structured ToolOutcome (signal kind + exit code) instead of a text prefix the agent had to sniff back out. Non-zero exits count as failures except for commands whose exit status is the answer (grep, diff, test) — Reflexion's data favours false negatives over false positives. A3 Reflection/stagnation/convergence hints moved out of the system message into a trailing user message, keeping the prompt prefix byte-stable so provider-side caching actually hits. A4 Truncated tool results from read-only tools record the original call in the omission marker, so the model can re-read instead of reasoning over a hole. Idempotency now matches the marker by shape: read_file's own windowing trailer also says "[omitted", which was silently exempting every windowed read from shrinking. dashscope unit: 702 passed, 6 skipped. pre-commit --all-files clean. * sync(acli): A7 evidence-anchored reflection from agenticCLI Mirrors agenticCLI 556255d. The three-failure reflection hint was a template string about a count; it now quotes the failing tool's own output plus the structured verdict (signal_kind / exit_code), tail-biased so tracebacks and FAILED lines survive truncation, and asks which assumption that output falsifies. Diagnosis and fix are demanded in the same response, so anchoring adds no model call. Only the latest failure is kept — accumulating failure history is what AlphaCodium measured as "no improvement". The experience lesson moves from hint-injection time to after the stream, storing the model's own diagnosis with an evidence fallback. test_hint_deduplicates_tool_names asserted dedup by counting a raw substring across the whole hint. The verdict line now names the last failing tool a second time by design, so the count is scoped to the summary line that renders the deduplicated set. * sync(acli): A8 futility refusal and OpenHands-calibrated repeats Vendored from agenticCLI a59e505 + 56afb36. The stagnation detector only counted read-only streaks and answered with advice the model could ignore. It now also keys repeats on call identity (tool plus canonicalised arguments) and a fingerprint of the result, at OpenHands' production thresholds (repeat action-observation 4, repeat action-error 3). _execute_tool consults the tracker before execution, so a call that already failed identically with nothing changed in between is refused rather than run — a structural interruption in the spirit of ADaPT forcing decomposition on failure instead of offering it. The refusal is audited as a denial and still answers its tool_call id. Only a successful mutating call counts as a state change, so re-running the test suite after an edit is never mistaken for a loop; and only failures are refused, so a repeated read still runs and merely escalates the prompt to a replan demand. tests/unit gains the repeat-detection coverage. The agent-level tests stay in agenticCLI, which is where the agent harness lives. * sync(acli): A9 acceptance gate and shared markdown fence from agenticCLI 4 vendored files (acceptance.py new) plus the ported unit tests; the agent loop now withholds a oneshot final answer until the project's test command passes, against a baseline captured before the first mutating call. Upstream: bda8fda (fence helper moved to utils.text) + 104a0c9 (the gate). dashscope/acli/{acceptance,agent}.py, utils/text.py and memory/reflection.py are byte-identical to the agenticCLI sources modulo the dashscope. import prefix. Verification: tests/unit 855 passed / 6 skipped (net +50, all from the new test_acli_acceptance.py); pre-commit run --all-files green, mypy included. The agent-loop half of the upstream tests is not ported — dashscope has no counterpart for tests/ut/test_agent_core_fixes.py, and no dashscope test constructs an acli Agent. * sync(acli): 0.6.6 oneshot usage report from agenticCLI ACLI_USAGE_FILE names a path a oneshot run flushes its token totals to, so an external harness can report real usage instead of zeros. Flushed every 15s rather than once at the end: the runs that most need measuring are the ones a timeout kills, and those never reach a final write. Unset, nothing is written. Also carries the 0.6.5 -> 0.6.6 version bump. The Terminal-Bench adapter that reads the file back lives in agenticCLI's bench/, which is not vendored here. * fix(acli): stop the acceptance gate rejecting answers its check never evaluated Synced from agenticCLI 8a2c0f6. A check that cannot execute was being read as a failure of the change. The gate only excused a byte-identical result, on the theory that a suite which genuinely fails prints timings. That holds for a real failure but not for the two commonest ways a check dies: the shell cannot find the command (127), and pytest cannot import the project at collection time (exit 2), whose output ends in `1 error in 0.05s` -- a timing, so the two runs never match. The discovered command names sys.executable, which is the right interpreter only when acli is installed into the project environment. Under pipx, `uv tool`, or a benchmark image that installs the project during its grading step instead, every run fails collection with differing timings and the gate withholds a correct answer after burning a full model round-trip. Read the exit status instead: 127 and pytest's 2/3/4/5 mean the check never reached the code. pytest's 1 stays a genuine verdict. Requiring the baseline to be inert too is what keeps this from excusing real damage -- a green baseline proves the check does run here, so a collection error the agent introduced still reads as a REGRESSION. The pytest statuses are matched only when the command names pytest, because cargo reuses 2 for a build failure. * sync(acli): delegate no longer offers a model override it could not honour From agenticCLI d90b1bd. The advertised per-call `model` only ever retargeted `Agent.model_name`, which picks vision/audio normalization and the truncation window; the child shares the parent's provider, whose model is fixed at construction, so it prepared messages for a model that never saw them. The kwarg stays in `_delegate`'s signature so an already-learned call gets no TypeError. /subagents now labels `model` and `temperature` as stored but not applied — they have no reader anywhere. * sync(acli): thread-driven usage flush from agenticCLI The flush used to hang off the oneshot output loop, so a run wedged inside run_stream -- exactly the run a benchmark most needs to measure -- produced no usage file at all and the harness read back zeros. * test(acli): cover when the usage report lands, not just what it holds The flusher is the part that decides whether a bench run yields numbers at all, and it had no coverage here. Each new test was checked against a deliberately broken flusher: narrowing the tick's exception guard, dropping the join in stop(), and writing the destination in place instead of renaming each fail exactly one test. * fix(acli): stop the turn-level retry re-paying an exhausted budget Syncs agenticCLI a2c515f into the vendored tree. HardenedProvider now marks an error whose retries it already spent with RetriesExhausted, carrying the original message verbatim so is_retryable_error still answers True and ProviderChain keeps falling back to the next profile. Agent skips its turn-level retry for that type only. Without the mark, every production provider (all of them arrive wrapped in HardenedProvider) had its budget re-run by the agent: measured 12 HTTP attempts and ~819s of silent wall clock in one turn from a 60s request_timeout, against an endpoint that accepts and never replies. Now 4 attempts. Adds tests/unit/test_acli_provider_retries.py covering the vendored copy, including the fallback invariant that a reworded marker would silently break. * chore(version): update version to 1.27.6 * sync(acli): degrade a mid-stream break instead of ending the run Syncs agenticCLI ef05859 into the vendored tree. oneshot now reports an unabsorbed provider error on stderr and exits 1 rather than letting it escape asyncio.run as a traceback; RetriesExhausted carries the wall clock its budget cost, and Agent skips its turn-level retry only when that shows the attempts were stalling rather than failing fast; a retryable error after the first chunk keeps the partial turn and continues the loop, since no layer can retry it without duplicating output. * test(acli): cover the cost an exhausted budget actually burned The module docstring promised unconditionally that Agent never re-pays an exhausted budget, which stopped being the contract: the exception type alone cannot tell a stalled endpoint (268s) from a refused connection (~28s), and telling them apart is what elapsed_sec was added for. Covers a real fast exhaustion reporting the backoff it spent, and pins the threshold on both sides plus the unmeasured default, which must not claim the expensive case -- giving up on a number nobody collected would recreate the type-only skip. Each was checked against a deliberately broken implementation and each break fails exactly one. The first attempt at breaking elapsed_sec was a false negative worth recording: the chat() and chat_stream() raise sites are textually identical, so a single replace patched chat() while the test exercises chat_stream(), and all 8 passed. Confirming the anchor count is part of the check, or "did not hit" reads as "test too weak". * sync(acli): scope an [a]lways grant to one command, not the whole shell Vendored copy of the agenticCLI fix. Answering [a]lways at a run_command confirmation cached the tool name, so approving one harmless command silently approved every later shell command in the turn: `curl ... | sh` and `rm -rf ~/work` both came back "trusted this turn" without a prompt. Grants now key on the exact stripped command in their own set, both permission paths consult them, the prompt label states the scope actually granted, and the echoed command is escaped. The /trust handler goes through the new trust_snapshot/trust_tool/deny_tool API instead of the executor's privates. * test(acli): pin the command-scoped shell grant in the vendored tree Twelve tests over the vendored executor: a second command still prompts, grant matching is exact rather than prefix/wildcard/case-folded, a grant with no usable command string trusts nothing, other tools still cache by name, the turn reset and snapshot copies hold, and the sync prompt path echoes the command escaped. The read-only allowlist and the global permission policy are both neutralised per-test: each is consulted before the trust cache, so either could answer the check without the grant ever being exercised. --------- Co-authored-by: zhansheng.lzs <zhansheng.lzs@alibaba-inc.com> Co-authored-by: luk384090-cloud <lu384090@gmail.com>
1 parent 2bbf11a commit 1a5049c

24 files changed

Lines changed: 3419 additions & 227 deletions

‎dashscope/acli/__init__.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import uuid
55

6-
__version__ = "0.6.5"
6+
__version__ = "0.6.6"
77

88
# Per-process identifier sent as x-dashscope-sdk-session-id so the
99
# backend can group multi-turn requests from one CLI run.

‎dashscope/acli/acceptance.py‎

Lines changed: 355 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,355 @@
1+
# -*- coding: utf-8 -*-
2+
"""Hard acceptance gate for autonomous (oneshot) runs.
3+
4+
The system prompt already asks the model to verify its work before reporting
5+
completion, and the model already ignores that. Aider's ``--auto-test`` and
6+
Agentless' three-stage filter make verification a property of the loop rather
7+
than a request: a non-zero exit means the answer is not accepted yet.
8+
OpenHands shipping the advice without the gate is the counterexample.
9+
10+
Only oneshot runs are gated. Interactively the user *is* the acceptance
11+
criterion and is reading the output, so spending minutes on a test suite
12+
before every reply costs more than the near-miss it prevents.
13+
"""
14+
15+
from __future__ import annotations
16+
17+
import asyncio
18+
import os
19+
import shlex
20+
import sys
21+
from dataclasses import dataclass
22+
from pathlib import Path
23+
24+
from dashscope.acli.utils.text import markdown_fence, truncate_head_tail
25+
26+
# Quoted verbatim into the next prompt, so: large enough to contain the
27+
# failing test id and its assertion, small enough to read. Tail-biased,
28+
# because that is where pytest and cargo put the verdict.
29+
_OUTPUT_CHARS = 3000
30+
31+
DEFAULT_RETRIES = 1
32+
DEFAULT_TIMEOUT = 600
33+
34+
EXPLICIT = "explicit"
35+
DISCOVERED = "discovered"
36+
37+
# Exit statuses meaning the check never reached the code under test, so its
38+
# output describes the environment rather than the change. 127 is the shell's
39+
# command-not-found; 2/3/4/5 are pytest's interrupted (also what a collection
40+
# error such as a missing dependency reports), internal-error, usage-error and
41+
# nothing-collected. pytest's 1 is deliberately absent: tests ran and one
42+
# failed, which is the verdict this whole module exists to act on.
43+
_NOT_FOUND = 127
44+
_PYTEST_NEVER_RAN = frozenset({2, 3, 4, 5})
45+
46+
47+
def _never_ran(run: AcceptanceRun) -> bool:
48+
"""True when ``run``'s exit status shows the check could not execute."""
49+
if run.exit_code == _NOT_FOUND:
50+
return True
51+
return "pytest" in run.command and run.exit_code in _PYTEST_NEVER_RAN
52+
53+
54+
def _env_int(name: str, default: int) -> int:
55+
try:
56+
return int(os.environ.get(name, "") or default)
57+
except ValueError:
58+
return default
59+
60+
61+
def _env_flag(name: str, default: bool) -> bool:
62+
raw = os.environ.get(name)
63+
if raw is None or not raw.strip():
64+
return default
65+
return raw.strip().lower() not in ("0", "false", "no", "off")
66+
67+
68+
def _read_head(path: Path, limit: int = 65536) -> str:
69+
try:
70+
with path.open("r", encoding="utf-8", errors="replace") as fh:
71+
return fh.read(limit)
72+
except OSError:
73+
return ""
74+
75+
76+
def _is_pytest_project(root: Path) -> bool:
77+
if (root / "pytest.ini").is_file():
78+
return True
79+
for name, marker in (
80+
("pyproject.toml", "[tool.pytest"),
81+
("setup.cfg", "[tool:pytest]"),
82+
("tox.ini", "[pytest]"),
83+
):
84+
path = root / name
85+
if path.is_file() and marker in _read_head(path):
86+
return True
87+
# Most Python repos never declare pytest config at all. A tests/ directory
88+
# beside a project file is pytest's default layout, and is what bare
89+
# `pytest` collects.
90+
return (root / "tests").is_dir() and (root / "pyproject.toml").is_file()
91+
92+
93+
def discover_acceptance_command(
94+
cwd: str | os.PathLike[str] | None = None,
95+
) -> str | None:
96+
"""The conventional test entry point for the project at ``cwd``, if any.
97+
98+
The allowlist is deliberately short. Every entry runs code from the
99+
repository — ``conftest.py`` at collection time, ``build.rs``, ``TestMain``
100+
— so "conservative" here means "a standard test entry point for an
101+
ecosystem whose manifest is sitting right here", not "harmless".
102+
``npm test`` and ``make test`` are excluded because their bodies are
103+
arbitrary user scripts with no conventional meaning; declaring them via
104+
``ACLI_ACCEPTANCE_CMD`` is how you ask for those.
105+
"""
106+
root = Path(cwd or os.getcwd())
107+
if not root.is_dir():
108+
return None
109+
if _is_pytest_project(root):
110+
# This interpreter, not a bare `pytest`: the gate must run against the
111+
# same environment the agent's own run_command calls use.
112+
return f"{shlex.quote(sys.executable)} -m pytest -q"
113+
if (root / "Cargo.toml").is_file():
114+
return "cargo test --quiet"
115+
if (root / "go.mod").is_file():
116+
return "go test ./..."
117+
return None
118+
119+
120+
@dataclass(frozen=True)
121+
class AcceptanceRun:
122+
"""One execution of the acceptance check."""
123+
124+
command: str
125+
source: str
126+
ok: bool
127+
exit_code: int | None
128+
output: str
129+
130+
131+
@dataclass(frozen=True)
132+
class GateVerdict:
133+
"""What the loop should do about a check that did not pass.
134+
135+
At most one of ``rejection`` and ``note`` is non-empty. ``rejection``
136+
withholds the answer and sends the model back with the raw output;
137+
``note`` is appended to the final answer when sending it back would not
138+
help.
139+
"""
140+
141+
run: AcceptanceRun
142+
rejection: str = ""
143+
note: str = ""
144+
145+
146+
class AcceptanceGate:
147+
"""Runs the acceptance check and decides whether the answer stands.
148+
149+
Two runs per turn at most: a baseline captured before the first mutation,
150+
and the check at the loop's exit. The baseline is what turns "the suite is
151+
red" into "you made it red" — Agentless reproduces the issue on the
152+
unpatched repository for the same reason, and AlphaCodium's test anchors
153+
are the same obligation stated positively.
154+
"""
155+
156+
def __init__(
157+
self,
158+
command: str = "",
159+
source: str = EXPLICIT,
160+
retries: int = DEFAULT_RETRIES,
161+
timeout: int = DEFAULT_TIMEOUT,
162+
):
163+
self.command = command
164+
self.source = source
165+
self.retries = retries
166+
self.timeout = timeout
167+
self.baseline: AcceptanceRun | None = None
168+
self.attempts = 0
169+
self._baseline_task: asyncio.Task | None = None
170+
171+
@classmethod
172+
def from_env(cls, oneshot: bool) -> AcceptanceGate:
173+
"""Build the gate from the ``ACLI_ACCEPTANCE*`` environment variables.
174+
175+
An explicit command wins over discovery. Setting ``ACLI_ACCEPTANCE=0``
176+
turns the gate off, which is what a benchmark measuring raw model
177+
output wants.
178+
"""
179+
if not (oneshot and _env_flag("ACLI_ACCEPTANCE", True)):
180+
return cls()
181+
command = os.environ.get("ACLI_ACCEPTANCE_CMD", "").strip()
182+
source = EXPLICIT
183+
if not command:
184+
command = discover_acceptance_command() or ""
185+
source = DISCOVERED
186+
return cls(
187+
command=command,
188+
source=source,
189+
retries=_env_int("ACLI_ACCEPTANCE_RETRIES", DEFAULT_RETRIES),
190+
timeout=_env_int("ACLI_ACCEPTANCE_TIMEOUT", DEFAULT_TIMEOUT),
191+
)
192+
193+
@property
194+
def active(self) -> bool:
195+
"""An empty command is the off switch."""
196+
return bool(self.command)
197+
198+
def reset(self) -> None:
199+
"""Drop the previous turn's anchor and retry budget.
200+
201+
The baseline has to come from the workspace as *this* turn found it,
202+
and the retry budget is per-turn so one slow turn cannot spend the
203+
next one's. An in-flight capture is cancelled for the same reason:
204+
left running, it would write the previous turn's verdict into this
205+
turn's ``baseline``.
206+
"""
207+
if self._baseline_task is not None and not self._baseline_task.done():
208+
self._baseline_task.cancel()
209+
self._baseline_task = None
210+
self.baseline = None
211+
self.attempts = 0
212+
213+
async def ensure_baseline(self) -> None:
214+
"""Capture the pre-change verdict, once per turn.
215+
216+
Called before the first mutating tool call rather than at turn start:
217+
a read-only or pure-answer turn never pays for a test suite, and the
218+
baseline still comes from a workspace the agent has not touched.
219+
220+
Concurrent callers share one task rather than checking a flag. The
221+
flag form lets the second mutating call in a parallel batch proceed
222+
while the baseline is still running, so the "pre-change" verdict would
223+
be captured against a workspace that had already changed.
224+
"""
225+
if not self.active:
226+
return
227+
if self._baseline_task is None:
228+
self._baseline_task = asyncio.ensure_future(self._capture())
229+
await self._baseline_task
230+
231+
async def _capture(self) -> None:
232+
self.baseline = await self._run()
233+
234+
async def check(self) -> GateVerdict | None:
235+
"""Run the check at the loop's exit and rule on the answer.
236+
237+
``None`` means "not applicable" — gate off, no command found, or
238+
nothing was mutated this turn, in which case there is no work to
239+
accept.
240+
"""
241+
if not self.active or self.baseline is None:
242+
return None
243+
run = await self._run()
244+
self.attempts += 1
245+
if run.ok:
246+
return GateVerdict(run)
247+
if _never_ran(run) and self._baseline_never_ran():
248+
return GateVerdict(run, note=self._unrunnable_note(run))
249+
if self._unchanged_since_baseline(run):
250+
return GateVerdict(run, note=self._insensitive_note(run))
251+
if self.attempts > self.retries:
252+
return GateVerdict(run, note=self._unverified_note(run))
253+
return GateVerdict(run, rejection=self._rejection(run))
254+
255+
def _baseline_never_ran(self) -> bool:
256+
"""True when the pre-change check could not execute either.
257+
258+
This is what separates a broken environment from a broken change. The
259+
baseline is captured before the agent touches anything, so a check that
260+
already could not run then was not made unrunnable by the change.
261+
Requiring it also keeps the sharp edge: a *green* baseline proves the
262+
check does run here, so a collection error the agent introduced stays
263+
on the rejection path instead of being excused.
264+
"""
265+
base = self.baseline
266+
return bool(base and not base.ok and _never_ran(base))
267+
268+
def _unchanged_since_baseline(self, run: AcceptanceRun) -> bool:
269+
"""True when the check came out byte-identical to a failing baseline.
270+
271+
The catch-all behind ``_baseline_never_ran``, for a check whose exit
272+
statuses this module does not know — cargo, go, an immediate timeout —
273+
where reproducing the baseline exactly is the only evidence available
274+
that the break is environmental. Sending the model back to fix that
275+
burns the turn on a problem it neither created nor can solve, so the
276+
gate reports it instead of rejecting.
277+
278+
Byte-identical is the point: a suite that genuinely fails prints
279+
timings, so real failures differ between runs and still get rejected.
280+
Only a stable, environment-level break reproduces exactly.
281+
"""
282+
base = self.baseline
283+
return bool(
284+
base
285+
and not base.ok
286+
and base.exit_code == run.exit_code
287+
and base.output == run.output,
288+
)
289+
290+
async def _run(self) -> AcceptanceRun:
291+
# run_command rather than a raw subprocess: it carries the
292+
# blocked-pattern checks, the optional OS sandbox, the output caps, and
293+
# the exit-code trailer that the rest of the loop already classifies.
294+
from dashscope.acli.audit import get_audit_logger
295+
from dashscope.acli.executor import classify_tool_text
296+
from dashscope.acli.tools.shell import run_command
297+
298+
get_audit_logger().log_tool_call(
299+
"acceptance_gate",
300+
{"command": self.command},
301+
decision="allowed",
302+
reason=f"source: {self.source}",
303+
)
304+
text = await run_command(command=self.command, timeout=self.timeout)
305+
outcome = classify_tool_text(text, {"command": self.command})
306+
return AcceptanceRun(
307+
command=self.command,
308+
source=self.source,
309+
ok=outcome.ok,
310+
exit_code=outcome.exit_code,
311+
output=outcome.text,
312+
)
313+
314+
def _rejection(self, run: AcceptanceRun) -> str:
315+
quoted = truncate_head_tail(run.output, _OUTPUT_CHARS, ratio=0.25)
316+
fence = markdown_fence(quoted)
317+
if self.baseline and self.baseline.ok:
318+
verdict = (
319+
"a REGRESSION: it passed before anything in this turn "
320+
"changed, so the change broke it"
321+
)
322+
else:
323+
verdict = "still failing"
324+
return (
325+
"\n## ⛔ Not accepted: the verification check is "
326+
f"{verdict}\n"
327+
f"`{run.command}` exited {run.exit_code}.\n"
328+
f"{fence}\n{quoted}\n{fence}\n"
329+
"Your answer is being withheld, not discarded: fix what this "
330+
"output shows and the turn ends by itself. Work from the output "
331+
"above — do not restate your plan, and do not declare the check "
332+
"wrong unless this output says so.\n"
333+
)
334+
335+
def _unverified_note(self, run: AcceptanceRun) -> str:
336+
return (
337+
f"\n[Unverified: `{run.command}` exited {run.exit_code} and the "
338+
f"retry budget ({self.retries}) is spent, so the answer below is "
339+
"reported as-is rather than withheld.]\n"
340+
)
341+
342+
def _unrunnable_note(self, run: AcceptanceRun) -> str:
343+
return (
344+
f"\n[Unverified: `{run.command}` exited {run.exit_code} before "
345+
"and after these changes, a status that means it never reached "
346+
"the tests. The check does not run in this environment, so it "
347+
"says nothing about the answer below.]\n"
348+
)
349+
350+
def _insensitive_note(self, run: AcceptanceRun) -> str:
351+
return (
352+
f"\n[Unverified: `{run.command}` failed identically before and "
353+
"after these changes, so it says nothing about them — most likely "
354+
"the check itself does not run in this environment.]\n"
355+
)

0 commit comments

Comments
 (0)