4 Commits

Author SHA1 Message Date
Nan Gao
cf556fa9d4
feat(agents): elide superseded write_file payloads from model-bound requests (#5374)
* feat(agents): elide superseded write_file payloads from model-bound requests

Step 2 of #5328. After a successful write_file the file on disk is the source
of truth, and the read-before-write gate forces a read_file before the next
modification of that path, so once a later successful read or write of the
same path exists the historical `content` argument is redundant with it. Long
report-writing runs (append-in-chunks) therefore carried every section twice,
once as the write argument and once as the following read output, until
summarization compacted the whole turn.

- ToolOutputBudgetMiddleware's model-call hooks now replace such superseded
  content with a short deterministic placeholder pointing at read_file, in the
  model-bound request only: state["messages"], checkpoints, receipts, loop
  detection, and the run journal keep the original arguments, and nothing is
  externalized to disk. The newest `keep_recent_writes` successful writes
  (default 1) always stay visible; str_replace payloads are never touched; a
  same-turn read never supersedes (parallel calls run in no fixed order); only
  results stamped deerflow_tool_meta.status == "success" count, so failed,
  gate-blocked, partial, or unstamped writes are never candidates.
- New `tool_output.elide_superseded_writes` (default on),
  `tool_output.superseded_write_min_chars` (default 2000), and
  `tool_output.keep_recent_writes` (default 1); config_version 41 -> 42 in
  config.example.yaml and the Helm chart.
- The per-occurrence call/result pairing the gate introduced in #5329 moves
  into the shared `tool_call_args.pair_tool_call_results` helper so both
  policies pair the same way; the gate now uses it.

* fix(agents): scope tool-call result pairing to the issuing turn

Review finding on #5374 (P2): pair_tool_call_results consumed results from a
history-wide per-id queue, so an interrupted write_file with no result whose
tool-call id a later turn reused inherited that later call's success. With
the default elision the unconfirmed draft was then replaced by a placeholder
claiming the write succeeded, and the gate's blocked-call pairing had the
mirror-image hole.

Pair results the way DanglingToolCallMiddleware does: walk in document order,
open each AIMessage's calls, and let a ToolMessage answer only a still-open
call of the most recent preceding AIMessage. A result never answers a call
from an earlier turn, so the interrupted call stays unanswered (never a
candidate, never labeled blocked) and stray or duplicate results are ignored.
Regressions cover the helper, the superseded-write policy, and the gate.

* fix(agents): never rewrite tool-call ids duplicated within one AIMessage

Review finding on #5374 (P2): the policies select calls per occurrence, but
every provider surface is addressed by tool-call id, so when a malformed
provider payload repeats an id inside one assistant turn the rewriter could
only replace all of its occurrences at once. A failed write_file sibling then
took on the superseded successful call's path and elided content and was
presented as a success; the gate's blocked-call elision had the mirror-image
hole (a successful sibling rewritten into the blocked call).

rewrite_messages_tool_call_args now never offers an id that repeats within
its message to the selector and leaves those calls untouched on every
surface. Both policies are covered by the shared helper; regressions cover
the helper, the superseded-write policy, and the gate.

* fix(agents): skip unhashable tool-call ids in the duplicate-id guard

Review finding on #5374 (round 3): _duplicated_call_ids fed every id into a
Counter before the string guard, so a list or dict id from a malformed
provider payload raised TypeError out of wrap_model_call and failed the whole
model call whenever the history also held a rewrite candidate. The pre-PR
loop and pair_tool_call_results skip such ids; only this helper regressed.

Count non-empty string ids only, and pin it with regressions for the helper,
the superseded-write policy, and the gate.

* docs(agents): keep middleware guidance within size limit

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
2026-09-13 18:04:51 +08:00
Nan Gao
3f0b6ecc81
feat(agents): elide blocked write payloads from model-bound requests (#5329)
* feat(agents): elide blocked write payloads from model-bound requests

A write_file / str_replace call rejected by the read-before-write gate never
runs, yet its payload (up to 80 KB for a non-append write, unbounded for
append) stayed verbatim in every later model request: nothing in the chain
rewrites AIMessage tool-call arguments, and ToolOutputBudgetMiddleware only
budgets ToolMessage output. The gate demands a re-read plus a fresh call, so
the model re-emits the content anyway and the original is pure dead weight.

- ReadBeforeWriteMiddleware stamps `deerflow_write_block` on the blocked
  ToolMessage and, in wrap_model_call, replaces the paired call's payload
  fields (content / old_str / new_str) with a short deterministic placeholder
  in the model-bound request only. state["messages"], receipts, loop
  detection, and the run journal keep the original arguments; nothing is
  externalized to disk, since a file reference to content the model must
  re-derive after reading the target would only invite bypassing the gate.
- New `tool_call_args` helper rewrites every provider surface together
  (structured tool_calls, raw additional_kwargs.tool_calls, tool_use content
  blocks, tool_call_chunks) so strict providers never see them disagree; the
  gate only supplies the policy (which calls, what placeholder).
- `read_before_write.elide_blocked_payloads` (default on) and
  `read_before_write.elide_min_chars` (default 2000) configure it; the
  runtime builder passes the config through and the middleware declares it
  via release_policy_parameters.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(agents): condense middleware guide entry 11 to fit the guidance budget

The agent-guidance CI check failed: the effective AGENTS.md chain for
agents/middlewares was 99673 bytes against a 98304-byte hard limit. The
chain already sat at 98459 on main, so the ReadBeforeWrite entry could not
grow. Rewrite entry 11 so it states the same facts (gate, lock scope,
fail-open, authorization scope, blocked-payload elision, shared
tool_call_args helper) in 1229 bytes instead of 2640; the chain is now
98262 bytes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(config): bump config_version for the read_before_write elision keys

Review follow-ups on #5329:

- `read_before_write.elide_blocked_payloads` / `elide_min_chars` are new
  user-settable YAML keys, i.e. a config schema change, so bump
  `config_version` 40 -> 41 in config.example.yaml; without it an existing
  config.yaml gets no outdated-config warning and `make config-upgrade` has
  nothing to signal.
- Say in the `elide_min_chars` description (and the example comment) that the
  threshold and the placeholder's size figure are Python character counts,
  not tokens: the same value spans roughly 3-4x in real context cost between
  ASCII and CJK text.
- The builder wiring test now asserts only the wired `elide_min_chars` value
  instead of the whole `ReadBeforeWriteConfig` dump, so future knobs do not
  have to edit an unrelated test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* chore(helm): bump chart config_version to 41

validate-chart's config_version drift check failed after config.example.yaml
moved to 41 in ef9ee267. Bare bump of the chart's embedded `config:` block
and the README example; the chart does not mirror the read_before_write
section, so no field changes are needed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(agents): rewrite Responses and v1 content-block arguments too

Review finding on #5329 (P2): the content rewriter only handled Anthropic
`tool_use` blocks. With `use_responses_api=true` and
`output_version='responses/v1'`, AIMessage.content carries `function_call`
blocks whose `arguments` still hold the full write payload, and
langchain_openai's Responses input builder emits that block instead of the
rewritten structured call whose `call_id` it already carries. Standard `v1`
`tool_call` blocks likewise keep `extras.arguments`, which the v1->Responses
translator prefers over the structured args. So the blocked payload was
still sent on every later Responses API request.

`tool_call_args` now rewrites every content dialect that carries its own
copy of the arguments: Anthropic `tool_use` (input, drop partial_json),
Responses `function_call` (arguments, matched by call_id, `fc_...` item id
and status preserved), and v1 `tool_call` / `tool_call_chunk` (args plus
`extras.arguments`). Tests assert against the real adapter serializers:
`_construct_responses_api_input` for responses/v1, v1, and v0 messages,
`_convert_message_to_dict` for chat completions, and Anthropic
`_format_messages` for native and v1 content, plus an end-to-end probe
through the gate's wrap_model_call.

* fix(agents): pair blocked writes per call occurrence and defeat Responses chaining

Two review findings on #5329:

- Tool-call ids may repeat across assistant turns (DanglingToolCallMiddleware
  pairs them with per-id queues). The gate matched blocked results against a
  history-wide id set, so a successful write sharing an id with a later (or
  earlier) blocked one also lost its payload and was labelled as blocked.
  `_blocked_call_occurrences` now pairs ToolMessages with call occurrences
  the same FIFO-per-id way and the selector keys on (message, call id).

- With `use_previous_response_id`, the OpenAI adapter sends only the messages
  after the last AIMessage carrying a `resp_` response id and lets the server
  rebuild the rest from its stored copy, which still holds the original
  arguments and cannot be edited; every later response chains back to it.
  `rewrite_messages_tool_call_args` now drops every `resp_` id from the
  model-bound copy whenever it rewrote anything, so the adapter replays the
  full rewritten history (the `use_previous_response_id=False` request
  shape). OpenAI bills chained input tokens as input either way, so replay
  costs no more; the state keeps its ids.

Tests cover success-before-block and block-before-success histories through
the Chat Completions serializer, and chaining through
`ChatOpenAI._get_request_payload` with `use_previous_response_id=True`:
unrewritten history chains and omits the call, rewritten history is replayed
with the placeholder and no `previous_response_id`.

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-11 19:05:48 +08:00
He Wang
fd41fdb065
feat(middleware): add structured tool result meta and tool-progress state machine (#3601)
* feat(middleware): add structured tool result meta and tool-progress state machine

feat:
- Add tool_result_meta.py: ToolResultMeta dataclass (status/error_type/retryable/
  recoverable_by_model/recommended_next_action/source) + normalize_tool_result and
  stamp_exception_meta utilities; classifies every ToolMessage regardless of path
- Add ToolProgressMiddleware: per-(thread_id, tool_name) state machine ACTIVE →
  WARNED (hint injected as HumanMessage) → BLOCKED (call short-circuited); Jaccard
  near-duplicate detection for repeated successful results; auth/config/internal
  errors bypass WARNED and go directly to BLOCKED; LRU-bounded thread state store
- Add ToolProgressConfig: all thresholds configurable (stagnation_threshold,
  warn_escalation_count, jaccard_similarity_threshold, exempt_tools, etc.);
  disabled by default (enabled: false)
- Wire ToolProgressMiddleware as outer wrapper around ToolErrorHandlingMiddleware
  in _build_runtime_middlewares so it receives results already carrying
  deerflow_tool_meta

fix:
- ToolErrorHandlingMiddleware now calls stamp_exception_meta on exception path and
  normalize_tool_result on success path so every ToolMessage carries deerflow_tool_meta

test:
- Add test_tool_result_meta.py: 26 cases covering all classification paths,
  stamp_exception_meta, and normalize_tool_result Command passthrough
- Add test_tool_progress_middleware.py: 27 cases including full async paths,
  Jaccard duplicate detection, LRU eviction, hint injection, and malformed meta
  passthrough
- Extend test_tool_error_handling_middleware.py: middleware ordering invariant and
  meta stamping on exception

docs:
- Add tool_progress section to config.example.yaml with all fields and descriptions
- Update CLAUDE.md middleware chain documentation (entries 8-9)

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

* fix(middleware): recoverable errors stay WARNED; fix auth keyword shadowing

fix:
- WARNED is terminal for recoverable_by_model=True errors (no_results, not_found,
  permission); hint re-injected on each problem call instead of escalating to
  BLOCKED, so the model can retry with different parameters (e.g. fresh query,
  new URL) without being hard-blocked by a prior stagnation count.
  Non-recoverable (rate_limited, transient) still escalate WARNED → BLOCKED
  after warn_escalation_count more problems; auth/config/internal remain
  immediately BLOCKED.
- Remove bare "api key" keyword from auth classification rule so "no api key
  configured" correctly classifies as config (not auth), producing the accurate
  block-reason text for the model.

docs:
- CLAUDE.md: document all three ToolProgressMiddleware transition paths
- config.example.yaml: update inline state-machine comment to match new paths

test:
- test_recoverable_errors_stay_warned_indefinitely: WARNED never escalates for
  recoverable errors regardless of how many problem calls accumulate
- test_recoverable_error_re_injects_hint_past_escalation: hints continue past
  the escalation zone for recoverable errors
- test_no_api_key_is_config_not_auth: regression guard for keyword shadowing fix

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

* fix(tool_result_meta): add JSON error extraction and fix source classification

fix:
- Fix non-standard error path: source was "exception" but should be "tool_return"
- Add _extract_json_error_text to isolate JSON error fields from noisy JSON bodies
  (e.g. Brave Search {"error": "...", "query": "..."} — query keywords no longer
  pollute error classification)
- Add success-path JSON extraction to catch tools that return HTTP 200 with a JSON
  error body (status="success" but {"error": "API key not configured"})
- Add _SEMANTIC_ZERO_ERROR_STRINGS frozenset to suppress false positives from tools
  that use {"error": "none"} / {"error": "null"} / {"error": "ok"} as success signals
- Document that stamp_exception_meta always overwrites existing TOOL_META_KEY
  (exception-derived classification is authoritative over tool return-time stamps)

test:
- Add parametrized regression tests for all semantic-zero error strings
- Add tests for non-standard error path source field
- Add tests for JSON error extraction (nonstd, success-path, numeric, falsy values)
- Correct test comment for test_no_api_key_is_config_not_auth

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

* fix(tool_progress_middleware): fix 6 bugs, add terminal guard and structured logging

fix:
- H1: fix exempt_tools empty-set silently ignored — use `is not None` instead of
  truthiness check so ToolProgressConfig(exempt_tools=set()) correctly disables all
  exemptions
- Fix _get_block_reason creating phantom LRU entries via _get_state (write path);
  now uses dict.get + explicit move_to_end on read path only
- Fix _pending memory leak: LRU eviction of _phase_states now synchronously removes
  all (evicted_thread, *) keys from _pending
- Fix _assess_and_transition missing terminal guard for blocked state — a recoverable
  error result could silently demote blocked → warned in concurrent-race scenarios;
  early return preserves terminal semantics
- Fix recent_word_sets window: stored [-5:] but is_near_duplicate only compared [-3:];
  align to [-3:] and change type list→tuple (prevents accidental in-place mutation
  across dataclasses.replace shallow copies)
- Fix _format_hint missing "success" key and "continue" action: Jaccard near-duplicate
  results produced the generic fallback instead of a specific actionable message

feat:
- Add structured state-transition logging (ACTIVE/WARNED/BLOCKED transitions, blocked
  intercepts, hint injection debug log)

test:
- Add regression tests for all 6 bug fixes (H1, phantom LRU, pending leak, terminal
  guard, window alignment, format_hint near-dup)
- Add Jaccard near-threshold boundary test (7/9 vs 8/9 Jaccard)
- Add production min_words=10 skip test for short content
- Add exempt_tools empty-set and None round-trip tests
- Add _augment_request deduplication test
- Add before_agent current-run preservation test
- Add structured logging tests (WARNED/BLOCKED/ACTIVE/intercepted/debug)

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

* chore(config): remove unused backward-compat fields from ToolProgressConfig

Remove max_calls_per_intent and window_size fields that were marked
"Retained for backward compatibility; not used by the current state machine"
when the state machine was introduced.  Pydantic v2 ignores unknown fields
by default, so existing config.yaml files with these keys remain valid.

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>

* fix(tool_progress): address PR review and multi-agent review findings

fix:
- Remove <80-char length gate for partial_success; only _PARTIAL_MARKERS now
- Add word-boundary regex for numeric HTTP codes (401/403/404/500) to avoid
  false positives like "500ms" or "4010 rows" triggering hard-block
- Add "task" to default exempt_tools (delegation primitive, not a search tool)
- Remove move_to_end() from _get_block_reason read path; blocked threads were
  permanently warm in LRU, starving active threads of eviction slots
- Add _reset_blocked_states in before_agent: scope BLOCKED and WARNED states
  to a single run; clear recent_word_sets so stale Jaccard windows don't cause
  false near-duplicate detections in the next run
- Compute word_set() lazily (only for success results); cap content at 8192
  chars to bound memory and CPU cost on large tool results
- Remove unused retryable field from ToolResultMeta (no consumer existed)
- Add isinstance-based ordering guard and warning log for missing meta
- Fix JSON-without-error-key fallback: use _UNKNOWN_ERROR instead of
  classifying incidental field values (e.g. {"user_id": 401} → auth → stop)
- Fix _extract_json_error_text: use json.dumps for dict/list error fields
  instead of str() which produced Python repr matching config rules spuriously
- Add "no results found"/"no content found"/"no images found" to _PARTIAL_MARKERS
  so success responses with empty results trigger stagnation detection
- Fix immediate-block path to increment consecutive_problems (was left at 0)
- Fix _queue_assessment: skip phantom _pending entries for evicted threads
- Bump config_version 13→16 (upstream added 14/15; tool_progress is additive)

test:
- Update test_short_content_is_partial → test_short_terse_success_is_not_partial
- Add parametrized test_numeric_keyword_word_boundary (8 positive + negative cases)
- Add test_before_agent_resets_blocked_states_for_new_run (strengthened assertions)
- Add test_before_agent_resets_warned_states_for_new_run
- Add test_missing_meta_on_non_exempt_tool_emits_warning
- Add test_middleware_ordering_guard_raises_when_progress_is_inner
- Add test_auth_error_immediately_blocked asserts consecutive_problems == 1
- Add tests for JSON-without-error-key, dict error field, no-results partial_success

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(tool_progress): address second PR review — perf, architecture doc, concurrency note

fix:
- Extract content.lower() once before _PARTIAL_MARKERS check in normalize_tool_message;
  previously computed up to 7× per call inside the generator (once per marker)

docs:
- Add division-of-labor paragraph to ToolProgressMiddleware module docstring explaining
  coexistence with LoopDetectionMiddleware: result-quality guard (per-tool BLOCK) vs
  call-pattern guard (whole-turn hard-stop); no shared state, no double-stop risk
- Add threading.Lock comment explaining why asyncio.Lock is not used (short critical
  sections, must also protect sync wrap_tool_call path from subagent executor threads)
- Update backend/CLAUDE.md entry 8 with division-of-labor summary; fix entry 9
  (remove stale retryable field reference, add missing recoverable_by_model/source)

test:
- Add test_tool_progress_and_loop_detection_coexist_without_interfering: drives both
  middlewares to WARNED state simultaneously, verifies independent state, independent
  hint queues, and no cross-contamination; uses snapshot copy for final assertion

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(tool_progress): reset all tool states at run boundary; fix semantic-zero test validity

fix:
- _reset_run_states (formerly _reset_blocked_states) drops the phase filter and
  resets all tracked (thread, tool) pairs unconditionally at before_agent; ACTIVE
  tools with sub-threshold consecutive_problems or cached recent_word_sets no longer
  bleed into the next run, preventing spurious WARNED transitions on clean R2 calls
- test_normalize_json_semantic_zero_error_string_not_treated_as_error: replace
  {error_value!r} f-string (produces invalid JSON with single quotes) with
  json.dumps so _extract_json_error_text actually parses the payload and the
  _SEMANTIC_ZERO_ERROR_STRINGS guard is exercised, not bypassed at json.loads

test:
- add test_before_agent_resets_active_state_consecutive_problems_and_word_sets to
  lock the ACTIVE-phase run-boundary reset: drives tool to active/cp=1/ws≠() in R1,
  asserts both fields are zero/empty after before_agent fires for R2

* docs(tool_progress): document intentional per-run reset vs LoopDetection thread-scoped retention

Addresses reviewer observation in PR #3601 that _reset_run_states diverges
from LoopDetectionMiddleware's cross-run scoping policy without explanation.

Expands the _reset_run_states docstring to record the intentional design
choice: ToolProgressMiddleware resets per-run because result-quality errors
(rate_limited, transient) are time-bound and may resolve between turns —
retaining stale counters would risk false-positive BLOCKED calls.
LoopDetectionMiddleware retains history across runs because call-pattern
loops are time-invariant. The divergence is by design, not oversight.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(middleware): restore ReadBeforeWriteMiddleware as outermost write gate

A merge conflict resolution had accidentally placed ReadBeforeWriteMiddleware
after ToolErrorHandlingMiddleware (inner), reversing the original intent from
b81334cc where it was the outermost write gate before ToolErrorHandling.

fix:
- Restore ReadBeforeWriteMiddleware to outer position: ReadBeforeWrite →
  ToolProgress → ToolErrorHandling. Blocked writes now return immediately
  without consuming a ToolProgress slot.
- Add normalize_tool_result call on blocked ToolMessages so they carry
  deerflow_tool_meta (recoverable_by_model=True) even though they bypass
  ToolErrorHandlingMiddleware.

test:
- Add test_blocked_write_has_deerflow_tool_meta (sync + async) to lock the
  normalize_tool_result behavior on blocked writes.
- Fix chain order assertions in TestChainWiring and
  test_build_lead_runtime_middlewares_chain_order_matches_agents_md.

docs:
- Renumber AGENTS.md items: 10→ReadBeforeWrite, 11→ToolProgress,
  12→ToolErrorHandling; update descriptions to reflect outermost-gate design.
- Fix stale cross-reference: LoopDetectionMiddleware (item 23) → (item 25).

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
2026-07-06 20:57:41 +08:00
Nan Gao
b81334ccfe
feat(middlewares): deterministic read-before-write version gate for file tools (#3911) (#3912)
* docs(specs): read-before-write gate design for issue #3857 output layer

* docs(plans): read-before-write gate implementation plan (#3857)

* refactor(sandbox): extract read_current_file_content helper (#3857)

* feat(middlewares): read-before-write version gate for file tools (#3857)

* test(middlewares): pin async read-before-write gate paths (#3857)

* feat(config): wire ReadBeforeWriteMiddleware into runtime chain, default on (#3857)

* docs(sandbox): document read-before-write gate in tool docstrings and AGENTS.md (#3857)

* docs(plans): align plan doc with landed config_version (17) and drop machine-specific paths

Addresses Copilot review comments on #3912.

* fix(middlewares): read-before-write gate — error-string sandboxes fail open; serialize gate+execution per path (#3912 review)

- AIO/E2B read_file reports failures (incl. missing files) as 'Error: ...'
  strings instead of raising; the gate treated that string as existing file
  content and blocked first-write creation. Error-string reads now count as
  uninspectable: gate fails open, no mark is stamped.
- LangGraph runs one AIMessage's tool calls concurrently, so two same-turn
  writes could both pass on one stale mark before either mutation landed
  (and a read mark could hash a version the model never saw). Gate check +
  tool execution (and read + mark stamping) now share a per-(thread, path)
  critical section, separate from the tool-internal file_operation_lock.
2026-07-03 17:21:04 +08:00