From 3793a96ae8b1e5f8ebc7773c566797804c63641c Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Fri, 4 Sep 2026 15:19:16 +0800 Subject: [PATCH 1/2] =?UTF-8?q?feat(context):=20Tier1=20=E8=BF=B7=E4=BD=A0?= =?UTF-8?q?=E5=8D=A1=20+=20=E6=94=B6=E7=B4=A7=20handle=20=E4=B8=8E=20host?= =?UTF-8?q?=20metadata=20=E5=A5=91=E7=BA=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 四件事，同一条线：Tier 1 丢掉的东西要么别丢，要么留得住，而"留得住" 依赖 handle 语义统一。 `KeepLastNToolResultsCompactor` 原先把老 tool body 整条换成占位符，丢掉调用 args 和 body 里所有 URL —— 恰好是后续轮次判断「这条 query 我是不是已经跑过」 最需要的两样。Tier 2 的摘要保留这两样，但 Tier 2 只在 Tier 1 没减够时才触发， 所以 Tier1-only 的轮次里模型看到的严格更少。 两个字段都是免费的：args 在请求方 assistant message 上，URL 在即将被丢弃的 body 里。没有 LLM 调用，没有额外存储，也没有第二套模型可见的恢复路径 —— 卡片只说"调了什么"，不提供取回手段（那仍归下面的 recovery 脚注）。 实测 +218 字符/条，硬上限 400，URL 上限 3 条。 placeholder 仍是第一行，所以既有的 `startswith` 幂等判据一字不改继续成立。 卡片不比原文短就保留原文 —— 但**仅当没配 spill**。有 handle 一律替换，即使 卡片更长：handle 只能经 `spill_refs` → Tier 2 recovery index 到达模型，留着 body 反而让上游 spill 掉的全文永久不可恢复，而那种 body 本身已经是截断预览。 本仓库这条判据的作用域比同源实现窄一档：配了 spill 而被拒收时，既有分支已经 无条件保留 body，所以这里不需要额外的最小尺寸门槛。新增测试锁住它依赖的那条 既有分支。 `KeepLastNToolResultsCompactor` 此前没有任何直接测试，只被 tiered 用例借道 覆盖过两次；连同这次改动补 14 条。 `spill_refs` 描述的是消息上**当前还在**的内容，`result_store_ref` 描述的是 上游截断前的 body。先读后者会把已存过的 body 再存一遍，更糟的是把错的 handle 钉进 recovery index。 `manifest_max_paths` / `manifest_max_chars`，任一可为 `None` 解除。默认值不变 (20 / 3000)，那是按「handle 渲染成文件路径」(60+ 字符/条) 定的尺寸。handle 是 短内容寻址 id 的产品每条只花约 17 字符，应当自己放开：上限一旦生效丢的是**最旧** 的 handle，而实测过决定性的早期证据正因为它的 handle 老化掉而在一段无关长弯路 之后不可恢复。 上限按**渲染后字符数**计，因为那是两种 handle 形态唯一共享的量 —— 也正因如此， 上限和 handle 形态不是可独立选择的两件事。 `result_metadata` 返回的东西原样带到 `render_tool_result`。这是产品自己给模型 措辞「这个调用重复了」所需的接缝：**算不算重复**是 per-tool 的产品策略 (`repeat_count`)，**正文是否字节相同**是另一件始终实测的事实、没有对应字段， 而两者缺一就会对着一个实际不同的 body 断言 "identical output" —— 模型能对着 自己 history 验证的谎言。 透传是**原样**而非"AgentCore 不认识的剩余 key"：剩余语义会在这里新增保留 key 的那天悄悄改变产品能看到的东西，正是下面这条检查存在的理由。 `scripts/check_unconsumed_fields.py` 进 CI —— 被监视模型上的字段，若在 `agent_core/` 内没有任何属性读取，必须在某个 `docs/*-boundary.md` 里被点名。 这条规则不是"给字段写文档"，而是：**决定不消费某个字段本身是一个边界决策**， 没写下来的边界决策和疏漏无法区分。0.4.0 有四个字段正是这个状态 —— 类型对、测试过、pyright 干净，零消费者，而它们在产品侧的消费者全留在产品自己 那份 loop 拷贝里。产品换用 `run_agent_loop` 会丢掉据此措辞的提示，且**静默** 丢掉：不抛异常，不挂测试，模型原先读到的那句话直接消失。canary merge 看不见 这一类，因为它只能发现硬冲突。 脚本写完立刻抓到本次新增的 `host_metadata`，已按规则补进 boundary doc。 0.4.0 → 0.5.0。改了模型读到的文本，按 `docs/versioning.md` 属于 compaction-decision 变更，即 breaking，走 MINOR。 - 新增 `tests/test_keep_last_n_compactor.py` 14 条 - `tests/test_tiered_compact.py` +2（上限可配置 / 丢最旧） - `tests/test_tool_exec.py` +2（透传原样、默认空且不共享实例） - `uv run pytest -q` → 1164 passed - ruff / pyright / check_unconsumed_fields / uv build 全过 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 4 + CHANGELOG.md | 83 +++++++++ agent_core/loop_types.py | 15 ++ agent_core/runtime/loop/compact.py | 164 ++++++++++++++++- agent_core/runtime/loop/tiered_compact.py | 36 +++- agent_core/runtime/loop/tool_exec.py | 1 + docs/agent-loop-boundary.md | 58 ++++++ pyproject.toml | 2 +- scripts/check_unconsumed_fields.py | 92 ++++++++++ tests/test_keep_last_n_compactor.py | 209 ++++++++++++++++++++++ tests/test_tiered_compact.py | 50 ++++++ tests/test_tool_exec.py | 52 ++++++ uv.lock | 2 +- 13 files changed, 753 insertions(+), 15 deletions(-) create mode 100644 scripts/check_unconsumed_fields.py create mode 100644 tests/test_keep_last_n_compactor.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a25dd0d..b2112a8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,10 @@ jobs: - run: uv sync --frozen --extra dev - run: uv run ruff check agent_core tests scripts - run: uv run pyright agent_core + # A host-supplied field that agent_core/ never reads is invisible to every + # other check here and to a canary merge: it types, it tests, and the note + # a product worded from it vanishes on adoption. See the script's docstring. + - run: python3 scripts/check_unconsumed_fields.py - run: uv run pytest -q - run: uv build diff --git a/CHANGELOG.md b/CHANGELOG.md index de38697..2b2e233 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,91 @@ the GitHub Release body, so a release with no entry here fails. Versioning follows [docs/versioning.md](docs/versioning.md). +## [0.7.0] - 2026-09-04 + +### Changed + +- Tier 1 deterministic compaction (`KeepLastNToolResultsCompactor`) no longer + leaves a bare placeholder where it drops a tool body. It now appends a bounded + card naming the call (tool name + a single-line, 120-char arguments preview) + and up to three source URLs found in the discarded body, above the existing + recovery pointer. Both fields already existed in the history and in the body, + so this adds no LLM call, no storage, and no second model-visible recovery + route. Measured cost is ~+220 characters per elided result, hard-capped at 400. + + Rationale: Tier 2's summary preserves arguments and URLs, but Tier 2 only fires + when Tier 1 did not free enough, so on a Tier1-only turn the model lost exactly + the two things it needs in order not to re-issue a query it already ran. + + A card that would not be shorter than the body it replaces is skipped and the + body kept verbatim — but only when no spill callback is configured. With a + recovery pointer the body is always replaced even if the card is longer: the + pointer reaches the model only through `spill_refs` → the Tier 2 recovery + index, and such a body is itself already an upstream-truncated preview, so + keeping it would strand the spilled full text as unrecoverable. + +- `KeepLastNToolResultsCompactor` now resolves an existing recovery handle with + `spill_refs` taking precedence over `result_store_ref`. A ref pinned by an + earlier compaction pass describes the content still on the message, while the + loop-cap handle describes the pre-truncation body upstream shed; reading the + latter first re-spilled a body that was already stored and pinned the wrong + handle into the recovery index. + +### Added + +- `TieredCompactor` takes `manifest_max_paths` and `manifest_max_chars`, either + of which may be `None` to remove that bound. The defaults are unchanged (20 / + 3,000) and are sized for a handle rendered as a filesystem path. A product + whose handles are short content-addressed ids pays a fraction of that per entry + and should raise or remove the cap: when a cap binds, the OLDEST handles are + dropped, and a product measured decisive early evidence becoming unrecoverable + after a long unrelated detour for exactly that reason. The cap is charged + against rendered characters, which is the only quantity the two handle shapes + share — cap and handle shape are therefore not independent choices. +- `ToolResult.host_metadata` carries whatever `ToolExecutionHooks.result_metadata` + returned, verbatim, through to `AgentLoopHooks.render_tool_result`. This is the + seam for a product that words its own note about a repeated call: whether a call + *counts* as a repeat is per-tool product policy (`repeat_count`), while whether + the body came back byte-identical is a separate observed fact with no typed + field, and both are needed to avoid asserting "identical output" for a body that + differs. The pass-through is verbatim rather than filtered to unrecognised keys, + so adopting a new reserved key here cannot silently remove something a product + already reads. + +### Consumer action + +- This changes text the model reads and is therefore a compaction-decision + change under `docs/versioning.md`. A consumer asserting equality against + `OMITTED_TOOL_RESULT_PLACEHOLDER` must switch to `startswith`; the placeholder + remains the first line precisely so that check keeps working. +- No API change for the card. No configuration flag for it either: it has no + failure mode of its own, and a switch would be one more configuration dimension + to maintain. +- A product that words a note from `repeat_count`, `repeat_recovery_id`, + `result_id` or `error_kind` in its own loop copy **must port that note into + `render_tool_result` before adopting `run_agent_loop`**. AgentCore reads none of + those fields, so nothing fails if the note is forgotten — it just stops reaching + the model. `docs/agent-loop-boundary.md` records why, and + `scripts/check_unconsumed_fields.py` now fails CI on a new field in that state. + +### Documented + +- `docs/agent-loop-boundary.md` now records that `ToolResult.repeat_count`, + `repeat_recovery_id`, `result_id` and `error_kind` have no consumer inside + AgentCore, that this is by construction, and that a product moving onto + `run_agent_loop` therefore loses any note it words from them *silently*. The + two ways to close it are stated, with `host_metadata` as the chosen route. +- `scripts/check_unconsumed_fields.py` runs in CI: a field on a watched model with + no attribute read anywhere in `agent_core/` must be named in a boundary + document. Deciding not to consume a field is a boundary decision, and an + undocumented one is indistinguishable from an oversight — which is how four + fields reached 0.4.0 with no consumer and no note. + ## [0.6.0] - 2026-09-04 +**Never published.** Merged to `main` but never tagged; its contents ship in +0.7.0. Nothing pins it. + ### Added - `agent_core.components.middleware.base.MiddlewareChain` — the phase/tool diff --git a/agent_core/loop_types.py b/agent_core/loop_types.py index 02c5038..0bae9a6 100644 --- a/agent_core/loop_types.py +++ b/agent_core/loop_types.py @@ -315,6 +315,21 @@ class ToolResult: # Host-provided repeated-invocation metadata. Execution is never skipped. repeat_count: int = 1 repeat_recovery_id: str = "" + # Whatever ``ToolExecutionHooks.result_metadata`` returned, verbatim. + # + # Passed through rather than filtered down to "the keys AgentCore did not + # recognise": a residue-based rule silently changes what the host can see + # whenever AgentCore adopts a new reserved key, which is exactly the class of + # quiet breakage this field exists to avoid. The reserved keys are also + # promoted to the typed fields above; seeing them twice costs nothing. + # + # The immediate reason it exists: a product wording a note about a repeated + # call needs both whether the call COUNTS as a repeat (its own per-tool + # policy, reported through ``repeat_count``) and whether the body came back + # byte-identical (a separate observed fact with no field of its own). Without + # a pass-through it has to keep a side table keyed by ``tool_call_id``. See + # ``docs/agent-loop-boundary.md``. + host_metadata: dict[str, Any] = field(default_factory=dict[str, Any]) @dataclass diff --git a/agent_core/runtime/loop/compact.py b/agent_core/runtime/loop/compact.py index c5822ff..d3adee4 100644 --- a/agent_core/runtime/loop/compact.py +++ b/agent_core/runtime/loop/compact.py @@ -118,6 +118,125 @@ def tool_names_by_call_id(messages: list[Message]) -> dict[str, str]: _tool_names_by_call_id = tool_names_by_call_id +# --------------------------------------------------------------------------- +# Elided-tool-result mini card +# +# Tier 1 used to leave ONLY ``OMITTED_TOOL_RESULT_PLACEHOLDER``, dropping the +# call's arguments and every source URL — precisely the two things a later turn +# needs in order not to re-issue a query it already ran. Tier 2's summary does +# preserve both, but Tier 2 only fires when Tier 1 did not free enough, so on a +# Tier1-only turn the model saw strictly less than it had to. +# +# Both fields are free: the arguments are on the requesting assistant message, +# the URLs are in the body about to be discarded. No LLM call, no extra storage, +# and no second model-visible index — the card names the call, it does not offer +# a way to fetch anything (that stays with the recovery footnote below it). +# +# The budget matters: a single web_search body can carry dozens of URLs, and an +# unbounded card would hand back the context Tier 1 just freed. Fill args first +# (they identify the call), then URLs until the budget runs out. +# --------------------------------------------------------------------------- + +_MINI_CARD_ARGS_MAX_CHARS = 120 +_MINI_CARD_BODY_MAX_CHARS = 400 +_MINI_CARD_MAX_URLS = 3 +_WHITESPACE_RE = re.compile(r"\s+") + + +def _args_preview(raw: object) -> str: + """Collapse a tool call's arguments to one short single-line preview. + + ``bash`` commands and ``web_fetch`` payloads carry newlines and heredocs; a + multi-line card would cost more rows than the body it replaces. + """ + rendered = raw if isinstance(raw, str) else str(raw or "") + collapsed = _WHITESPACE_RE.sub(" ", rendered).strip() + if len(collapsed) <= _MINI_CARD_ARGS_MAX_CHARS: + return collapsed + return collapsed[:_MINI_CARD_ARGS_MAX_CHARS] + "\u2026" + + +def _tool_args_by_call_id(messages: list[Message]) -> dict[str, str]: + """Map ``tool_call_id`` → bounded preview of the arguments it was sent. + + Kept private, unlike :func:`tool_names_by_call_id`: no product facade + resolves arguments by call id, so there is no older spelling to honour. + """ + out: dict[str, str] = {} + for msg in messages: + if not is_assistant_msg(msg): + continue + for tc_value in cast(list[Any], msg.get("tool_calls") or []): + if not isinstance(tc_value, dict): + continue + tc = cast(dict[str, Any], tc_value) + fn_value = tc.get("function") + fn = cast(dict[str, Any], fn_value) if isinstance(fn_value, dict) else None + raw = ( + fn.get("arguments", tc.get("args", "")) + if fn is not None + else tc.get("arguments", tc.get("args", "")) + ) + tid = tc.get("id") or (fn.get("id") if fn is not None else None) + if not isinstance(tid, str) or not tid: + continue + preview = _args_preview(raw) + if preview: + out[tid] = preview + return out + + +def _elided_tool_card(tool_name: str, args_preview: str, content: str) -> str: + """Render the card lines that stand in for a discarded tool body. + + Returns ``""`` when there is nothing worth saying (no name, no arguments, no + URLs), so the caller falls back to the bare placeholder rather than emitting + an empty line. + """ + budget = _MINI_CARD_BODY_MAX_CHARS + lines: list[str] = [] + if tool_name or args_preview: + call_line = ( + f"[Called: {tool_name}({args_preview})]" + if args_preview + else f"[Called: {tool_name}]" + ) + lines.append(call_line) + budget -= len(call_line) + + urls: list[str] = [] + for url in dict.fromkeys(URL_RE.findall(content)): + if len(urls) >= _MINI_CARD_MAX_URLS: + break + # A web_fetch card would otherwise print its own url twice. + if url in args_preview: + continue + cost = len(url) + 3 # " | " separator + if cost > budget: + break + urls.append(url) + budget -= cost + if urls: + lines.append("[Source URLs] " + " | ".join(urls)) + return "\n".join(lines) + + +def _message_recovery_ref(message: Message) -> str: + """Return a handle that already backs this body, so we never store it twice. + + ``spill_refs`` wins over ``result_store_ref``: a ref pinned by an EARLIER + compaction pass describes the content that is actually still on the message, + whereas the loop-cap handle describes the pre-truncation body upstream shed. + Reading the latter first would re-spill a body that is already stored, and — + worse — would pin the wrong handle into the recovery index. + """ + refs = [r for r in (message.get("spill_refs") or []) if r] + canonical = str(message.get("result_store_ref") or "") + if canonical and canonical not in refs: + refs.append(canonical) + return refs[0] if refs else "" + + def _condense(content: str, max_chars: int) -> str: """Head + tail + URLs of *content*, never longer than the original.""" prefix = f"[Compressed tool result: {len(content):,} characters]\n" @@ -434,10 +553,17 @@ def should_compact( class KeepLastNToolResultsCompactor: - """Replace older ``ToolMessage`` bodies with a short placeholder. - - Keeps the last ``keep_tool_result`` tool results verbatim and replaces - the content of every earlier one with :data:`OMITTED_TOOL_RESULT_PLACEHOLDER`. + """Replace older ``ToolMessage`` bodies with a short mini card. + + Keeps the last ``keep_tool_result`` tool results verbatim and replaces the + content of every earlier one with :data:`OMITTED_TOOL_RESULT_PLACEHOLDER` + followed by a bounded card naming the call (tool + arguments preview) and up + to :data:`_MINI_CARD_MAX_URLS` source URLs found in the discarded body, then + the recovery pointer when the body was spilled. The card is free — both + fields already exist in the history and in the body — and it is what keeps a + later turn from re-issuing a query whose result it can no longer see. When no + spill is configured and the card would not be shorter than the body it + replaces, the body is kept verbatim instead. ``SystemMessage``, ``HumanMessage``, and every ``AIMessage`` (including its thinking trace) are left intact, so the model retains its full chain of reasoning and tool-call metadata while dropping the bulk of @@ -489,7 +615,10 @@ def compact( if len(keep_set) == len(tool_indices): return messages + # Names and arguments are needed unconditionally now: the mini card names + # the call it replaced even when nothing is protected and nothing spills. id_to_name = tool_names_by_call_id(messages) + id_to_args = _tool_args_by_call_id(messages) out: list[Message] = [] for idx, msg in enumerate(messages): @@ -505,12 +634,12 @@ def compact( ): out.append(msg) continue + call_id = msg.get("tool_call_id", "") placeholder = OMITTED_TOOL_RESULT_PLACEHOLDER - spill_path = str(msg.get("result_store_ref") or "") + spill_path = _message_recovery_ref(msg) if not spill_path and self._spill is not None: - tool_name = id_to_name.get(msg.get("tool_call_id", ""), "tool") try: - spill_path = self._spill(tool_name, content) + spill_path = self._spill(id_to_name.get(call_id, "tool"), content) except Exception: spill_path = None # A configured spill callback is a promise that discarded content @@ -520,9 +649,28 @@ def compact( if self._spill is not None and not spill_path: out.append(msg) continue + card = _elided_tool_card( + id_to_name.get(call_id, ""), id_to_args.get(call_id, ""), content, + ) + if card: + placeholder += "\n" + card if spill_path: placeholder += f"\n[Full text] {spill_path}" - replacement = tool_msg(placeholder, msg.get("tool_call_id", "")) + # Without a pointer, replacing the body DESTROYS it, so a card that + # is not even shorter is a pure loss and we keep the body. With one + # we always replace, even when the card is longer: the pointer only + # reaches the model through ``spill_refs`` → the Tier 2 recovery + # index, so keeping the body here would strand the spilled full text + # as unrecoverable — and such a body is itself already a truncated + # preview, not the full content. + # + # Reachable only when NO spill callback is configured: a configured + # one that declined already returned the body verbatim above, at any + # size. That is why this needs no minimum-size threshold of its own. + if not spill_path and len(placeholder) >= len(content): + out.append(msg) + continue + replacement = tool_msg(placeholder, call_id) if spill_path: # The text is for the model; this is for us. ``TieredCompactor`` # collects refs from the field, so nothing has to recognise a diff --git a/agent_core/runtime/loop/tiered_compact.py b/agent_core/runtime/loop/tiered_compact.py index b0705b1..45511c0 100644 --- a/agent_core/runtime/loop/tiered_compact.py +++ b/agent_core/runtime/loop/tiered_compact.py @@ -86,6 +86,15 @@ def _spill_can_recover(spill: Callable[[str, str], str | None] | None) -> bool: _FULL_TEXT_PREFIX = "[Full text] " + +# Defaults sized for a handle rendered as a filesystem path (60+ chars each), so +# an unbounded index would itself become a context cost. A product whose handle +# is a short content-addressed id pays ~17 chars each and can raise or remove the +# cap — see ``manifest_max_paths`` / ``manifest_max_chars`` on ``TieredCompactor``. +# +# The cap is charged against the RENDERED characters, not a handle count, because +# that is the only quantity the two handle shapes share. This is why the cap and +# the handle shape cannot be chosen independently. _SPILL_MANIFEST_MAX_PATHS = 20 _SPILL_MANIFEST_MAX_CHARS = 3_000 @@ -205,6 +214,8 @@ def __init__( summary_retries: int = 2, summary_retry_timeout_s: float | None = None, prompt_builder: SummaryPromptBuilder = compaction_prompt, + manifest_max_paths: int | None = _SPILL_MANIFEST_MAX_PATHS, + manifest_max_chars: int | None = _SPILL_MANIFEST_MAX_CHARS, ) -> None: if spill is not None and spill_store is not None: raise ValueError("pass spill or spill_store, not both") @@ -263,6 +274,13 @@ def __init__( #: What the most recent ``compact`` selected. Read by the agent loop to #: notify ``on_compaction`` observers; ``None`` until the first call. self.last_event: CompactionEvent | None = None + # ``None`` on either knob removes that bound. Dropping the OLDEST handles + # is not a neutral trim: a product measured the decisive early evidence + # becoming unrecoverable after a long unrelated detour, precisely because + # the handle naming it had aged out. A product whose rendered handles are + # short should say so here rather than inherit a cap sized for paths. + self._manifest_max_paths = manifest_max_paths + self._manifest_max_chars = manifest_max_chars self._relief_target = relief_target # Optional: the same gauge that drives the trigger. When present, the # relief check is expressed in REAL tokens instead of the raw estimate. @@ -381,26 +399,34 @@ def _spill_changed_tool_results( refs.append(path) return refs - @staticmethod def _with_spill_manifest( - messages: list[Message], refs: list[str], + self, messages: list[Message], refs: list[str], ) -> list[Message]: - """Keep a bounded session-local recovery index when Tier 1 is replaced. + """Keep a session-local recovery index when Tier 1 is replaced. - The index is its own message, carrying the paths in ``spill_refs`` and + The index is its own message, carrying the handles in ``spill_refs`` and rendering them as text for the model. Keeping it separate from the summary is what removes a whole class of bug: it used to be appended into the summary message, so replacing it meant finding where the index started inside prose the summarizer wrote — and the summarizer sees the previous index and quotes its header. + + Bounded by ``manifest_max_paths`` / ``manifest_max_chars``, either of + which may be ``None`` to remove that bound. When a bound does apply the + NEWEST handles are kept, which is the lesser of two bad options rather + than a good one — see the note on those knobs in ``__init__``. """ if not refs: return messages + max_paths = self._manifest_max_paths + max_chars = self._manifest_max_chars selected: list[str] = [] used = 0 for path in reversed(refs): cost = len(path) + 3 - if len(selected) >= _SPILL_MANIFEST_MAX_PATHS or used + cost > _SPILL_MANIFEST_MAX_CHARS: + if max_paths is not None and len(selected) >= max_paths: + break + if max_chars is not None and used + cost > max_chars: break selected.append(path) used += cost diff --git a/agent_core/runtime/loop/tool_exec.py b/agent_core/runtime/loop/tool_exec.py index 56e5953..10c97cb 100644 --- a/agent_core/runtime/loop/tool_exec.py +++ b/agent_core/runtime/loop/tool_exec.py @@ -359,6 +359,7 @@ async def _run_one(call: dict[str, Any], index: int) -> ToolResult: result_id=str(metadata.get("result_id") or ""), repeat_count=repeat_count, repeat_recovery_id=str(metadata.get("repeat_recovery_id") or ""), + host_metadata=dict(metadata), ) except TimeoutError: elapsed = int((time.monotonic() - start) * 1000) diff --git a/docs/agent-loop-boundary.md b/docs/agent-loop-boundary.md index 757e090..3d0fcb2 100644 --- a/docs/agent-loop-boundary.md +++ b/docs/agent-loop-boundary.md @@ -39,3 +39,61 @@ never separates a call from its reply. Host hooks are advisory: a raising and falls back to core behavior rather than failing the batch, and a fan-in interrupt waiter that returns `False` or raises leaves the tool running instead of discarding its collected work. + +## Repeated-invocation metadata has no core-side consumer, by construction + +`ToolResult.repeat_count`, `repeat_recovery_id`, `result_id` and `error_kind` are +populated from `ToolExecutionHooks.result_metadata` and then read by **nothing +inside AgentCore**. That is deliberate — AgentCore owns the *facts* about a tool +result, while what to say to the model about them is prompt policy, which varies +per product and per profile — but it is load-bearing enough to state here rather +than leave to be rediscovered. + +The reason it needs stating: a product that today appends its own note in its own +loop copy loses that note the moment it adopts `run_agent_loop`, and loses it +*silently*. Nothing raises, no test fails, the field is still there and still +correct — the sentence the model used to read is simply gone. Verify against your +own loop before switching, because a canary merge cannot see this class of gap. + +`scripts/check_unconsumed_fields.py` enforces this in CI: a field on a watched +model with no attribute read anywhere in `agent_core/` must be named in one of +these boundary documents. The rule is not "document your fields" — it is that +deciding *not* to consume a field is a boundary decision, and an undocumented one +is indistinguishable from an oversight. + +Two ways to close the gap were considered. + +**A — AgentCore words the note.** Uniform across products, one place to fix the +wording. Costs a metadata contract change: the note cannot be worded correctly +from `repeat_count` alone. Whether a call *counts as* a repeat is per-tool policy +(a stateless retrieval query with the same arguments is the same query even when +the provider reshuffled its snippets; a `bash` re-read over mutated sandbox state +is a legitimate second call), while whether the body came back byte-identical is +a separate, always-observed fact. Collapsing the two makes the note assert +"identical output" for a body that differs — a falsehood the model can check +against its own history. So A requires an `identical_body`-equivalent alongside +`repeat_count`, and AgentCore would then own wording that products may need to +diverge on. + +**B — the product words the note.** No contract change to the shape of the loop: +`AgentLoopHooks.render_tool_result` already receives the `ToolResult` and returns +the body that becomes the history message, which is exactly the right seam. This +keeps prompt wording with the layer that owns prompts. The one gap is the same +`identical_body` fact: `result_metadata` maps only the four known keys onto +`ToolResult` and drops everything else, so a product cannot currently carry that +bit from where it is observed to where it words the note without keeping its own +side table keyed by `tool_call_id`. + +**B is the chosen route.** `ToolResult.host_metadata` carries whatever +`result_metadata` returned, verbatim, through to `render_tool_result`, so +`identical_body` and anything else a product observes reaches the place where the +product words its note. Two consequences worth stating: + +- The pass-through is verbatim rather than "the keys AgentCore did not + recognise". A residue rule would silently change what a product sees the day + AgentCore adopts a new reserved key — the same quiet breakage this whole + section is about. Reserved keys therefore appear both as typed fields and in + `host_metadata`. +- AgentCore still words nothing about repeats. A product moving off its own loop + copy must port its note into `render_tool_result`; nothing here will fail if it + forgets, which is why this paragraph exists. diff --git a/pyproject.toml b/pyproject.toml index b261d40..e4f5918 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "apodex-agent-core" -version = "0.6.0" +version = "0.7.0" description = "Shared, product-neutral runtime primitives for Apodex agents" readme = "README.md" license = "Apache-2.0" diff --git a/scripts/check_unconsumed_fields.py b/scripts/check_unconsumed_fields.py new file mode 100644 index 0000000..2142d12 --- /dev/null +++ b/scripts/check_unconsumed_fields.py @@ -0,0 +1,92 @@ +"""Fail a pull request that adds a host-supplied field nothing consumes or documents. + +AgentCore was extracted by canary-merging a product branch against it and closing +whatever refused to build. That finds hard conflicts — a missing attribute, a +required monkeypatch — and is blind to the opposite shape: a field carried into +``ToolResult`` from a product hook, correctly typed, fully tested, and read by +nothing. Every consumer of it stayed behind in the product's own loop copy, so a +product adopting ``run_agent_loop`` loses whatever it worded from that field and +loses it *silently*. Nothing raises. No test fails. The sentence the model used +to read is simply gone. + +Four fields reached 0.4.0 in exactly that state. The rule that keeps it from +recurring: a field AgentCore does not read must be named in a boundary document, +which forces whoever adds it to write down who is expected to consume it. + +Exit 0 when every unconsumed field is documented, 1 otherwise. +""" + +from __future__ import annotations + +import ast +import re +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +PACKAGE = ROOT / "agent_core" +DOCS = ROOT / "docs" + +# Models whose fields are populated by a product hook rather than by core logic. +# Add a model here when it grows a host-supplied field; the check then holds it +# to the same rule. +WATCHED = {"ToolResult": PACKAGE / "loop_types.py"} + + +def declared_fields(class_name: str, module: Path) -> list[str]: + tree = ast.parse(module.read_text()) + for node in ast.walk(tree): + if isinstance(node, ast.ClassDef) and node.name == class_name: + return [ + stmt.target.id + for stmt in node.body + if isinstance(stmt, ast.AnnAssign) and isinstance(stmt.target, ast.Name) + ] + raise SystemExit(f"{class_name} not found in {module.relative_to(ROOT)}") + + +def consumer_count(field: str, declared_in: Path) -> int: + """Attribute reads of *field* anywhere in the package but its own module. + + Attribute access is the whole signal: a keyword argument at the construction + site (``repeat_count=repeat_count``) is the field being FILLED, which is + exactly the state this check exists to catch, so it must not count. + """ + pattern = re.compile(rf"\.{re.escape(field)}\b") + total = 0 + for path in PACKAGE.rglob("*.py"): + if path == declared_in: + continue + total += len(pattern.findall(path.read_text())) + return total + + +def documented(field: str) -> bool: + return any(field in doc.read_text() for doc in DOCS.glob("*-boundary.md")) + + +def main() -> int: + undocumented: list[str] = [] + for class_name, module in WATCHED.items(): + for field in declared_fields(class_name, module): + if consumer_count(field, module) or documented(field): + continue + undocumented.append(f"{class_name}.{field}") + + if not undocumented: + return 0 + + print("Host-supplied fields with no consumer in agent_core/ and no mention") + print("in any docs/*-boundary.md:") + for name in undocumented: + print(f" - {name}") + print() + print("A field AgentCore does not read reaches a product only if that product") + print("wires it up. Name it in the boundary document that owns it and say who") + print("consumes it — or read it here. Silence is how the note a product used") + print("to word from it disappears without a single test failing.") + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_keep_last_n_compactor.py b/tests/test_keep_last_n_compactor.py new file mode 100644 index 0000000..1dd046e --- /dev/null +++ b/tests/test_keep_last_n_compactor.py @@ -0,0 +1,209 @@ +"""Tier 1 elided-result mini card: what survives a blanked tool body. + +The card carries the call's arguments and the source URLs from the body being +discarded — the two things a later turn needs in order not to re-issue a query +it already ran. These tests pin its budget, its idempotency against the existing +placeholder check, and the one case where the card is skipped in favour of the +body it would have replaced. +""" + +from __future__ import annotations + +from agent_core.messages import system_msg, tool_msg +from agent_core.runtime.loop.compact import ( + _LEGACY_OMITTED_TOOL_RESULT_PLACEHOLDERS, + _MINI_CARD_ARGS_MAX_CHARS, + _MINI_CARD_BODY_MAX_CHARS, + _MINI_CARD_MAX_URLS, + OMITTED_TOOL_RESULT_PLACEHOLDER, + KeepLastNToolResultsCompactor, +) + + +def _ai(tid: str, name: str, arguments: str = "{}") -> dict: + return { + "role": "assistant", + "content": "", + "tool_calls": [{"id": tid, "function": {"name": name, "arguments": arguments}}], + } + + +def _one_call(name: str, arguments: str, body: str, **tool_fields: object) -> list[dict]: + """A minimal history whose single tool result is old enough to be blanked.""" + msg = tool_msg(body, "c1") + msg.update(tool_fields) # type: ignore[arg-type] + return [system_msg("S"), _ai("c1", name, arguments), msg] + + +def _blanked(messages: list[dict], **kwargs: object) -> str: + compactor = KeepLastNToolResultsCompactor(keep_tool_result=0, **kwargs) # type: ignore[arg-type] + out = compactor.compact(messages, 0) + bodies = [m["content"] for m in out if m.get("role") == "tool"] + assert len(bodies) == 1 + return bodies[0] + + +def _card_of(content: str) -> str: + """The card lines only: everything after the placeholder's first line.""" + assert content.startswith(OMITTED_TOOL_RESULT_PLACEHOLDER) + return content[len(OMITTED_TOOL_RESULT_PLACEHOLDER) :].lstrip("\n") + + +# --- what the card carries ------------------------------------------------- + + +def test_card_names_the_call_and_its_arguments(): + body = "RESULT " + "x" * 2_000 + args = '{"query": "NVIDIA H100 market share 2025"}' + content = _blanked(_one_call("web_search", args, body)) + assert "[Called: web_search(" in content + assert "NVIDIA H100 market share 2025" in content + + +def test_card_carries_source_urls_from_the_discarded_body(): + body = "see https://nvidianews.nvidia.com/q3 and https://tomshardware.com/h100 " + "x" * 2_000 + content = _blanked(_one_call("web_search", '{"query": "h100"}', body)) + assert "[Source URLs]" in content + assert "https://nvidianews.nvidia.com/q3" in content + assert "https://tomshardware.com/h100" in content + + +def test_url_already_in_the_arguments_is_not_repeated(): + url = "https://example.com/report" + body = f"fetched {url}\n" + "x" * 2_000 + content = _blanked(_one_call("web_fetch", f'{{"url": "{url}"}}', body)) + assert content.count(url) == 1 + + +def test_url_heavy_body_stays_within_the_card_budget(): + urls = [f"https://example{i}.com/{'p' * 60}" for i in range(20)] + body = " ".join(urls) + " " + "x" * 5_000 + content = _blanked(_one_call("web_search", '{"query": "many"}', body)) + card = _card_of(content) + assert len(card) <= _MINI_CARD_BODY_MAX_CHARS + assert sum(card.count(u) for u in urls) <= _MINI_CARD_MAX_URLS + + +def test_overlong_arguments_are_truncated(): + args = '{"command": "' + "a" * 500 + '"}' + content = _blanked(_one_call("bash", args, "OUT " + "x" * 2_000)) + call_line = _card_of(content).splitlines()[0] + assert "…" in call_line + assert len(call_line) < _MINI_CARD_ARGS_MAX_CHARS + 60 + + +def test_multiline_arguments_are_flattened_to_one_line(): + args = '{"command": "cat <= 1 + + +def _manifest_of(messages: list[dict]) -> str: + return next( + m["content"] for m in messages + if m.get("role") == "user" and m.get("spill_refs") + ) + + +def test_manifest_caps_are_configurable_and_removable(): + """The cap and the handle shape cannot be chosen independently. + + Defaults are sized for a handle rendered as a filesystem path. A product + whose handles are short content-addressed ids pays a fraction of that per + entry, and dropping the OLDEST handle is what makes decisive early evidence + unrecoverable after a long unrelated detour — so it must be able to say so. + """ + refs = [f"/spill/{i:03d}" for i in range(40)] + messages = [system_msg("S"), tool_msg("BODY", "c1")] + + capped = TieredCompactor( + keep_tool_result=1, summary_llm=_FakeLLM(), relief_target=10**9, + manifest_max_paths=5, + )._with_spill_manifest(messages, refs) + assert len(_manifest_of(capped).splitlines()) == 6 # header + 5 + + uncapped = TieredCompactor( + keep_tool_result=1, summary_llm=_FakeLLM(), relief_target=10**9, + manifest_max_paths=None, manifest_max_chars=None, + )._with_spill_manifest(messages, refs) + assert len(_manifest_of(uncapped).splitlines()) == 41 # header + all 40 + + # A char cap still binds independently of the path cap. + char_capped = TieredCompactor( + keep_tool_result=1, summary_llm=_FakeLLM(), relief_target=10**9, + manifest_max_paths=None, manifest_max_chars=60, + )._with_spill_manifest(messages, refs) + assert 1 < len(_manifest_of(char_capped).splitlines()) < 41 + + +def test_capped_manifest_keeps_the_newest_handles(): + refs = ["/spill/old", "/spill/mid", "/spill/new"] + out = TieredCompactor( + keep_tool_result=1, summary_llm=_FakeLLM(), relief_target=10**9, + manifest_max_paths=2, + )._with_spill_manifest([system_msg("S"), tool_msg("BODY", "c1")], refs) + manifest = _manifest_of(out) + assert "/spill/new" in manifest + assert "/spill/mid" in manifest + assert "/spill/old" not in manifest diff --git a/tests/test_tool_exec.py b/tests/test_tool_exec.py index 979549d..7676c3d 100644 --- a/tests/test_tool_exec.py +++ b/tests/test_tool_exec.py @@ -93,6 +93,58 @@ async def await_call(awaitable, name, args, timeout): assert results[0].repeat_count == 2 assert results[0].repeat_recovery_id == "spill-0" assert events == ["meter:echo", "enter:tc:12", "await:echo:3:12", "exit"] + # Reserved keys are promoted to typed fields AND left in host_metadata: the + # pass-through is verbatim so that adopting a new reserved key here cannot + # silently remove something a product was already reading. + assert results[0].host_metadata == { + "error_kind": "command_exit", + "result_id": "spill-1", + "repeat_count": 2, + "repeat_recovery_id": "spill-0", + } + + +@pytest.mark.asyncio +async def test_host_metadata_carries_keys_agent_core_does_not_know() -> None: + """The seam a product needs to word its own note about a repeated call. + + ``identical_body`` has no typed field: whether a call COUNTS as a repeat is + per-tool product policy, while whether the body came back byte-identical is a + separate observed fact. A product wording a note needs both, and this is how + the second one reaches ``render_tool_result``. + """ + hooks = ToolExecutionHooks( + result_metadata=lambda _name, _args, _raw, _rendered: { + "repeat_count": 3, + "identical_body": False, + }, + ) + results = await execute_tools( + [{"name": "echo", "args": {"x": 1}, "id": "tc"}], + {"echo": FakeTool("echo", "value")}, + timeout=5, + turn=1, + count_offset=0, + hooks=hooks, + ) + assert results[0].repeat_count == 3 + assert results[0].host_metadata["identical_body"] is False + + +@pytest.mark.asyncio +async def test_host_metadata_defaults_to_empty_and_is_not_shared() -> None: + results = await execute_tools( + [ + {"name": "echo", "args": {"x": 1}, "id": "a"}, + {"name": "echo", "args": {"x": 2}, "id": "b"}, + ], + {"echo": FakeTool("echo", "value")}, + timeout=5, + turn=1, + count_offset=0, + ) + assert results[0].host_metadata == {} + assert results[0].host_metadata is not results[1].host_metadata @pytest.mark.asyncio diff --git a/uv.lock b/uv.lock index 5709051..d7829c1 100644 --- a/uv.lock +++ b/uv.lock @@ -50,7 +50,7 @@ wheels = [ [[package]] name = "apodex-agent-core" -version = "0.6.0" +version = "0.7.0" source = { editable = "." } dependencies = [ { name = "anthropic", extra = ["bedrock"] }, From 9e58dec3374500e1cb37e27588d20a028249b85c Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Fri, 4 Sep 2026 16:06:47 +0800 Subject: [PATCH 2/2] fix(context): enforce compaction budget contracts --- CHANGELOG.md | 5 ++- agent_core/runtime/loop/compact.py | 14 +++--- agent_core/runtime/loop/tiered_compact.py | 39 +++++++++++++---- docs/agent-loop-boundary.md | 3 +- scripts/check_unconsumed_fields.py | 26 ++++++++---- tests/test_check_unconsumed_fields.py | 45 ++++++++++++++++++++ tests/test_keep_last_n_compactor.py | 11 +++++ tests/test_tiered_compact.py | 52 +++++++++++++++++++++-- 8 files changed, 165 insertions(+), 30 deletions(-) create mode 100644 tests/test_check_unconsumed_fields.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b2e233..4b360f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,10 @@ Versioning follows [docs/versioning.md](docs/versioning.md). dropped, and a product measured decisive early evidence becoming unrecoverable after a long unrelated detour for exactly that reason. The cap is charged against rendered characters, which is the only quantity the two handle shapes - share — cap and handle shape are therefore not independent choices. + share — cap and handle shape are therefore not independent choices. The + character cap covers the complete rendered index, including its header and + list syntax. Non-`None` bounds must be large enough to retain at least one + entry; invalid zero or header-only bounds fail at construction time. - `ToolResult.host_metadata` carries whatever `ToolExecutionHooks.result_metadata` returned, verbatim, through to `AgentLoopHooks.render_tool_result`. This is the seam for a product that words its own note about a repeated call: whether a call diff --git a/agent_core/runtime/loop/compact.py b/agent_core/runtime/loop/compact.py index d3adee4..3a46e72 100644 --- a/agent_core/runtime/loop/compact.py +++ b/agent_core/runtime/loop/compact.py @@ -153,7 +153,7 @@ def _args_preview(raw: object) -> str: collapsed = _WHITESPACE_RE.sub(" ", rendered).strip() if len(collapsed) <= _MINI_CARD_ARGS_MAX_CHARS: return collapsed - return collapsed[:_MINI_CARD_ARGS_MAX_CHARS] + "\u2026" + return collapsed[: _MINI_CARD_ARGS_MAX_CHARS - 1] + "\u2026" def _tool_args_by_call_id(messages: list[Message]) -> dict[str, str]: @@ -193,7 +193,6 @@ def _elided_tool_card(tool_name: str, args_preview: str, content: str) -> str: URLs), so the caller falls back to the bare placeholder rather than emitting an empty line. """ - budget = _MINI_CARD_BODY_MAX_CHARS lines: list[str] = [] if tool_name or args_preview: call_line = ( @@ -201,8 +200,9 @@ def _elided_tool_card(tool_name: str, args_preview: str, content: str) -> str: if args_preview else f"[Called: {tool_name}]" ) + if len(call_line) > _MINI_CARD_BODY_MAX_CHARS: + call_line = call_line[: _MINI_CARD_BODY_MAX_CHARS - 1] + "\u2026" lines.append(call_line) - budget -= len(call_line) urls: list[str] = [] for url in dict.fromkeys(URL_RE.findall(content)): @@ -211,11 +211,11 @@ def _elided_tool_card(tool_name: str, args_preview: str, content: str) -> str: # A web_fetch card would otherwise print its own url twice. if url in args_preview: continue - cost = len(url) + 3 # " | " separator - if cost > budget: + candidate_urls = [*urls, url] + candidate_lines = [*lines, "[Source URLs] " + " | ".join(candidate_urls)] + if len("\n".join(candidate_lines)) > _MINI_CARD_BODY_MAX_CHARS: break - urls.append(url) - budget -= cost + urls = candidate_urls if urls: lines.append("[Source URLs] " + " | ".join(urls)) return "\n".join(lines) diff --git a/agent_core/runtime/loop/tiered_compact.py b/agent_core/runtime/loop/tiered_compact.py index 45511c0..4090e4f 100644 --- a/agent_core/runtime/loop/tiered_compact.py +++ b/agent_core/runtime/loop/tiered_compact.py @@ -97,6 +97,7 @@ def _spill_can_recover(spill: Callable[[str, str], str | None] | None) -> bool: # the handle shape cannot be chosen independently. _SPILL_MANIFEST_MAX_PATHS = 20 _SPILL_MANIFEST_MAX_CHARS = 3_000 +_SPILL_MANIFEST_MIN_CHARS = len(_SPILL_MANIFEST_HEADER) + len("\n- x") class InputTokenGauge(BaseObserver): @@ -219,6 +220,16 @@ def __init__( ) -> None: if spill is not None and spill_store is not None: raise ValueError("pass spill or spill_store, not both") + if manifest_max_paths is not None and manifest_max_paths < 1: + raise ValueError("manifest_max_paths must be >= 1 or None") + if ( + manifest_max_chars is not None + and manifest_max_chars < _SPILL_MANIFEST_MIN_CHARS + ): + raise ValueError( + "manifest_max_chars must fit the manifest header and one handle " + f"(>= {_SPILL_MANIFEST_MIN_CHARS}) or be None" + ) spill_callback = spill or ( spill_store.spill_compacted_body if spill_store is not None else None ) @@ -421,7 +432,9 @@ def _with_spill_manifest( max_paths = self._manifest_max_paths max_chars = self._manifest_max_chars selected: list[str] = [] - used = 0 + # The configured cap applies to the exact model-visible text. Each item + # adds a newline, the bullet marker, and the handle itself. + used = len(_SPILL_MANIFEST_HEADER) for path in reversed(refs): cost = len(path) + 3 if max_paths is not None and len(selected) >= max_paths: @@ -431,22 +444,32 @@ def _with_spill_manifest( selected.append(path) used += cost selected.reverse() - index: Message = user_msg( - _SPILL_MANIFEST_HEADER + "\n" + "\n".join(f"- {path}" for path in selected), - ) - index["spill_refs"] = selected + index: Message | None = None + if selected: + index = user_msg( + _SPILL_MANIFEST_HEADER + "\n" + "\n".join(f"- {path}" for path in selected), + ) + index["spill_refs"] = selected out: list[Message] = [] replaced = False for message in messages: - # Exactly one message can be the index: the one that says it is. - if not replaced and message.get("role") == "user" and message.get("spill_refs"): - out.append(index) + # The data field is authoritative; the header recognises a legacy + # checkpoint. Remove duplicate or now-empty indices while here. + is_index = message.get("role") == "user" and ( + "spill_refs" in message + or text_of(message.get("content")).startswith(_SPILL_MANIFEST_HEADER) + ) + if is_index: + if not replaced and index is not None: + out.append(index) replaced = True continue out.append(message) if replaced: return out + if index is None: + return out insert_at = 0 while insert_at < len(out) and out[insert_at].get("role") == "system": diff --git a/docs/agent-loop-boundary.md b/docs/agent-loop-boundary.md index 3d0fcb2..dbec126 100644 --- a/docs/agent-loop-boundary.md +++ b/docs/agent-loop-boundary.md @@ -42,7 +42,8 @@ of discarding its collected work. ## Repeated-invocation metadata has no core-side consumer, by construction -`ToolResult.repeat_count`, `repeat_recovery_id`, `result_id` and `error_kind` are +`ToolResult.repeat_count`, `ToolResult.repeat_recovery_id`, `ToolResult.result_id` +and `ToolResult.error_kind` are populated from `ToolExecutionHooks.result_metadata` and then read by **nothing inside AgentCore**. That is deliberate — AgentCore owns the *facts* about a tool result, while what to say to the model about them is prompt policy, which varies diff --git a/scripts/check_unconsumed_fields.py b/scripts/check_unconsumed_fields.py index 2142d12..5ef96fb 100644 --- a/scripts/check_unconsumed_fields.py +++ b/scripts/check_unconsumed_fields.py @@ -45,31 +45,39 @@ def declared_fields(class_name: str, module: Path) -> list[str]: raise SystemExit(f"{class_name} not found in {module.relative_to(ROOT)}") -def consumer_count(field: str, declared_in: Path) -> int: - """Attribute reads of *field* anywhere in the package but its own module. +def consumer_count(field: str) -> int: + """Count real attribute reads of *field* anywhere in the package. Attribute access is the whole signal: a keyword argument at the construction site (``repeat_count=repeat_count``) is the field being FILLED, which is exactly the state this check exists to catch, so it must not count. """ - pattern = re.compile(rf"\.{re.escape(field)}\b") total = 0 for path in PACKAGE.rglob("*.py"): - if path == declared_in: - continue - total += len(pattern.findall(path.read_text())) + tree = ast.parse(path.read_text()) + total += sum( + 1 + for node in ast.walk(tree) + if ( + isinstance(node, ast.Attribute) + and isinstance(node.ctx, ast.Load) + and node.attr == field + ) + ) return total -def documented(field: str) -> bool: - return any(field in doc.read_text() for doc in DOCS.glob("*-boundary.md")) +def documented(class_name: str, field: str) -> bool: + """Require the qualified model field, not an unrelated substring match.""" + pattern = re.compile(rf"\b{re.escape(class_name)}\.{re.escape(field)}\b") + return any(pattern.search(doc.read_text()) for doc in DOCS.glob("*-boundary.md")) def main() -> int: undocumented: list[str] = [] for class_name, module in WATCHED.items(): for field in declared_fields(class_name, module): - if consumer_count(field, module) or documented(field): + if consumer_count(field) or documented(class_name, field): continue undocumented.append(f"{class_name}.{field}") diff --git a/tests/test_check_unconsumed_fields.py b/tests/test_check_unconsumed_fields.py new file mode 100644 index 0000000..ccb19b6 --- /dev/null +++ b/tests/test_check_unconsumed_fields.py @@ -0,0 +1,45 @@ +"""Regression tests for the host-field boundary guard.""" + +from __future__ import annotations + +from scripts import check_unconsumed_fields as check + + +def test_consumer_count_counts_only_real_attribute_reads(tmp_path, monkeypatch): + package = tmp_path / "agent_core" + package.mkdir() + declared_in = package / "types.py" + declared_in.write_text("class Model:\n field: str\n") + consumer = package / "consumer.py" + consumer.write_text( + "# A comment mentioning .field is not a consumer.\n" + "note = 'A string mentioning .field is not a consumer either.'\n" + "obj.field = 'write-only'\n" + ) + monkeypatch.setattr(check, "PACKAGE", package) + + assert check.consumer_count("field") == 0 + + consumer.write_text(consumer.read_text() + "value = obj.field\n") + assert check.consumer_count("field") == 1 + + declared_in.write_text( + "class Model:\n" + " field: str\n" + " def read(self) -> str:\n" + " return self.field\n" + ) + assert check.consumer_count("field") == 2 + + +def test_documented_requires_the_qualified_model_field(tmp_path, monkeypatch): + docs = tmp_path / "docs" + docs.mkdir() + boundary = docs / "tool-boundary.md" + monkeypatch.setattr(check, "DOCS", docs) + + boundary.write_text("AnotherTool.result_id and ToolResult.result_identifier\n") + assert check.documented("ToolResult", "result_id") is False + + boundary.write_text("The host consumes `ToolResult.result_id`.\n") + assert check.documented("ToolResult", "result_id") is True diff --git a/tests/test_keep_last_n_compactor.py b/tests/test_keep_last_n_compactor.py index 1dd046e..e513bf4 100644 --- a/tests/test_keep_last_n_compactor.py +++ b/tests/test_keep_last_n_compactor.py @@ -17,6 +17,7 @@ _MINI_CARD_MAX_URLS, OMITTED_TOOL_RESULT_PLACEHOLDER, KeepLastNToolResultsCompactor, + _args_preview, ) @@ -84,12 +85,22 @@ def test_url_heavy_body_stays_within_the_card_budget(): assert sum(card.count(u) for u in urls) <= _MINI_CARD_MAX_URLS +def test_exact_rendered_card_stays_within_budget(): + # The header and newline are part of the model-visible card too. Choose URL + # lengths that made the old per-URL accounting overshoot the cap. + urls = [f"https://e.com/{letter * 68}" for letter in "abc"] + args = "x" * _MINI_CARD_ARGS_MAX_CHARS + content = _blanked(_one_call("web_search", args, " ".join(urls) + " " + "x" * 2_000)) + assert len(_card_of(content)) <= _MINI_CARD_BODY_MAX_CHARS + + def test_overlong_arguments_are_truncated(): args = '{"command": "' + "a" * 500 + '"}' content = _blanked(_one_call("bash", args, "OUT " + "x" * 2_000)) call_line = _card_of(content).splitlines()[0] assert "…" in call_line assert len(call_line) < _MINI_CARD_ARGS_MAX_CHARS + 60 + assert len(_args_preview(args)) == _MINI_CARD_ARGS_MAX_CHARS def test_multiline_arguments_are_flattened_to_one_line(): diff --git a/tests/test_tiered_compact.py b/tests/test_tiered_compact.py index 1d1260a..4948c4e 100644 --- a/tests/test_tiered_compact.py +++ b/tests/test_tiered_compact.py @@ -9,10 +9,11 @@ from agent_core.llm import LLMResponse from agent_core.loop_types import LoopConfig, TurnContext -from agent_core.messages import system_msg, tool_msg +from agent_core.messages import system_msg, tool_msg, user_msg from agent_core.runtime.loop.agent_loop import run_agent_loop from agent_core.runtime.loop.compact import ( OMITTED_TOOL_RESULT_PLACEHOLDER, + SPILL_MANIFEST_HEADER, KeepLastNToolResultsCompactor, estimate_tokens, ) @@ -206,12 +207,16 @@ def test_manifest_caps_are_configurable_and_removable(): )._with_spill_manifest(messages, refs) assert len(_manifest_of(uncapped).splitlines()) == 41 # header + all 40 - # A char cap still binds independently of the path cap. + # A char cap still binds independently of the path cap and includes the + # complete model-visible rendering, not only the handle payloads. + char_cap = len(SPILL_MANIFEST_HEADER) + sum(len(ref) + 3 for ref in refs[-2:]) char_capped = TieredCompactor( keep_tool_result=1, summary_llm=_FakeLLM(), relief_target=10**9, - manifest_max_paths=None, manifest_max_chars=60, + manifest_max_paths=None, manifest_max_chars=char_cap, )._with_spill_manifest(messages, refs) - assert 1 < len(_manifest_of(char_capped).splitlines()) < 41 + manifest = _manifest_of(char_capped) + assert len(manifest) == char_cap + assert manifest.splitlines() == [SPILL_MANIFEST_HEADER, "- /spill/038", "- /spill/039"] def test_capped_manifest_keeps_the_newest_handles(): @@ -224,3 +229,42 @@ def test_capped_manifest_keeps_the_newest_handles(): assert "/spill/new" in manifest assert "/spill/mid" in manifest assert "/spill/old" not in manifest + + +@pytest.mark.parametrize( + ("kwargs", "message"), + [ + ({"manifest_max_paths": 0}, "manifest_max_paths must be >= 1 or None"), + ({"manifest_max_chars": len(SPILL_MANIFEST_HEADER)}, "manifest_max_chars must fit"), + ], +) +def test_manifest_caps_reject_values_that_cannot_hold_an_index(kwargs, message): + with pytest.raises(ValueError, match=message): + TieredCompactor( + keep_tool_result=1, + summary_llm=_FakeLLM(), + relief_target=10**9, + **kwargs, + ) + + +def test_overlong_handle_removes_an_old_index_without_inserting_an_empty_one(): + cap = len(SPILL_MANIFEST_HEADER) + len("\n- x") + old_index = user_msg(f"{SPILL_MANIFEST_HEADER}\n- /spill/old") + old_index["spill_refs"] = ["/spill/old"] + messages = [system_msg("S"), old_index, tool_msg("BODY", "c1")] + compactor = TieredCompactor( + keep_tool_result=1, + summary_llm=_FakeLLM(), + relief_target=10**9, + manifest_max_chars=cap, + ) + + once = compactor._with_spill_manifest(messages, ["/spill/" + "x" * 100]) + twice = compactor._with_spill_manifest(once, ["/spill/" + "x" * 100]) + + assert not any( + m.get("role") == "user" and "spill_refs" in m + for m in twice + ) + assert not any(SPILL_MANIFEST_HEADER in str(m.get("content")) for m in twice)