Skip to content

fix(subagents): report an explicit zero batch limit instead of defaulting it - #5609

Merged
WillemJiang merged 3 commits into
bytedance:mainfrom
Lesereingrape:fix/batch-explicit-zero-limits
Sep 22, 2026
Merged

WillemJiang merged 3 commits into
bytedance:mainfrom
Lesereingrape:fix/batch-explicit-zero-limits

Conversation

@Lesereingrape

@Lesereingrape Lesereingrape commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #5608

Why

SubagentBatchService.submit() resolved the two caller-supplied limits with or (batch_service.py:134-135), which reads an explicit 0 as "not supplied" and substitutes the configured default — so the 1 <= … <= … guards on the very next lines never see the value the caller actually asked for. Measured on main 71087f2f:

{'max_running_items': 0}                        -> accepted, persisted {'max_live_items': 100, 'max_running_items': 3}
{'max_live_items': 1, 'max_running_items': 0}   -> ValueError: max_running_items must not exceed max_live_items
{'max_live_items': -1}                          -> ValueError: max_live_items must be between 1 and 1000

The pain is twofold: a batch_task call that asked for zero concurrent items silently runs three at a time and stores the substituted number as the batch's own limit, and when the caller pairs the 0 with a small max_live_items the request is refused for a reason it did not violate. The third row is the tell — the adjacent negative is reported correctly, so 0 was an accident of or, not a documented "0 means unset" convention (and SubagentBatchesConfig itself declares these knobs ge=1, so the config layer already treats 0 as out of range).

This is the same falsy-vs-unset class as #5555, which this account filed and which is now merged: timeout_seconds written as an explicit 0 there also had to be handled as a value rather than as a blank, and the guard added in that PR deliberately does not swallow 0 (config.py:52 tests value is None or blank-string). This brings submit() to the same rule.

What changed

  • Both resolutions now compare against None, so 0 reaches the guard that names it: max_running_items must be between 1 and 64 / max_live_items must be between 1 and 1000. None (absent key, YAML null, the tool's own default) keeps meaning "use the configured default", which is the only behaviour any existing caller depends on.
  • Added a four-line comment recording why is None matters here, because the or form is otherwise locally indistinguishable and invites the same regression.
  • Tests: test_submit_reports_an_explicit_zero_limit_by_name (3 cases, including the one whose message used to blame the wrong field) and test_submit_defaults_only_the_limits_the_caller_omitted (3 cases pinning that None still defaults and a valid explicit value still wins), next to the existing test_submit_keeps_batch_running_limit_separate_from_one_process_capacity which already asserts the persisted kwargs.

Surface area

submit()'s limit resolution in subagents/batch_service.py (+5/-2) and its test file (+46), nothing else. No schema, persistence, prompt or dependency change: the values written to the batch row are the same fields, and no other function reads request.max_live_items / request.max_running_items. Callers are unaffected for every input that produced a working batch before — the only inputs whose outcome changes are the ones where a 0 was being silently replaced, and both callers (batch_task via batch_task_tool.py:230-231, and the Gateway submit path) already map ValueError from submit() to an error tool result, so the new refusal surfaces as the message the caller can act on rather than as a crash.

Bug fix verification

Reproduction is tests/test_subagent_batch_service.py::test_submit_reports_an_explicit_zero_limit_by_name, which goes red on main and green here. Both runs were done on this branch's base main at 71087f2f, reverting only batch_service.py in place and keeping the new tests:

# main 71087f2f + new tests, fix reverted      (-k "explicit_zero or defaults_only")
3 failed, 3 passed, 5 deselected in 5.84s
FAILED ...::test_submit_reports_an_explicit_zero_limit_by_name[overrides0-max_running_items must be between 1 and 64]
FAILED ...[overrides1-max_running_items must be between 1 and 64]
FAILED ...[overrides2-max_live_items must be between 1 and 1000]
   (DID NOT RAISE <class 'ValueError'>)
# the 3 "defaults only what the caller omitted" cases pass before and after, which is the point

# this branch ebf3c75b
62 passed in 25.54s

The scripted repro in the issue was also re-run against the fix: case 1 and case 2 now both raise max_running_items must be between 1 and 64, case 3 is unchanged.

Validation

cd backend
pytest tests/test_subagent_batch_service.py tests/test_batch_task_tool.py tests/test_subagent_capacity_config.py tests/test_subagent_batch_repository.py tests/test_batch_acceptance.py -q
  -> 62 passed in 25.54s     (56 on main before this PR + the 6 new cases)
ruff check packages/harness/deerflow/subagents/batch_service.py tests/test_subagent_batch_service.py            -> All checks passed!
ruff format --check <those two files>   -> 2 files already formatted

I did not run make test (the whole backend suite): on this Windows machine it has pre-existing environmental failures unrelated to this diff, documented in a comment on #5553, so I ran the five durable-batch suites that cover batch_service.py instead. Related work I checked for overlap: #5253 also edits backend/packages/harness/deerflow/subagents/batch_service.py (+109) — its hunks are in the item teardown/cancel path and none of them touch lines 129-141, so this diff and that one do not overlap in content.

AI assistance

  • Tool(s) used: Claude Code (Anthropic), plus a code-search agent pass over subagents/.
  • How you used it: the candidate came out of that search. Its claims were then re-checked line by line by the agent against main (batch_service.py:134-135, batch_runtime.py:26-27, batch_task_tool.py:143-144/230-231, subagent_batches_config.py:13-16), and the repro, the fix, both tests and every number above were produced by commands the agent ran on this branch. No human has read this diff yet — it was opened during an unattended contribution run, so the box below is deliberately left unticked; the account owner will read it before this PR is marked ready for review.
  • I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

Follow-up (a523818)

Took the non-blocking suggestion from review: the two batch_task arg docstrings now say
when set it must be >= 1, so the constraint is visible in the tool schema the model
reads rather than only in the runtime ValueError.

Re-measured at this head: the same five files still give 62 passed, and ruff check /
ruff format --check over all three changed files
(batch_service.py, batch_task_tool.py, test_subagent_batch_service.py) report
All checks passed! and 3 files already formatted.

…ting it

SubagentBatchService.submit resolved max_live_items / max_running_items with
`or`, so a caller that explicitly passed 0 got the configured default
(100 / 3) persisted and the 1..N range guards never saw the value, while a
negative already failed there. Resolve the defaults on 'is None' so an
explicit 0 reaches the guard that names it.

@willem-bd willem-bd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at ebf3c75. The fix is correct and minimal: BatchSubmitRequest.max_live_items/max_running_items are typed int | None (batch_runtime.py), so is None is the right "unset" test and an explicit 0 now reaches the range guard that names it, matching the ge=1 stance SubagentBatchesConfig already takes at the config layer. Independently verified on this head:

  • Red/green: with base batch_service.py restored and the new tests kept, all three test_submit_reports_an_explicit_zero_limit_by_name cases fail (DID NOT RAISE ValueError); at head, all 62 tests across test_subagent_batch_service.py, test_batch_task_tool.py, test_subagent_capacity_config.py, test_subagent_batch_repository.py, and test_batch_acceptance.py pass.
  • ruff check and ruff format --check are clean on both changed files.
  • Blast radius confirmed: the only production caller is batch_task (batch_task_tool.py:221-231), which passes the int | None values through unchanged and already wraps submit() in try/except, so the new refusal surfaces as an error tool result rather than a crash, and no other code reads these two request fields.

Non-blocking: the batch_task arg docstrings ("Optional queued-plus-running item window" / "Optional per-batch real execution concurrency") could eventually state that both values must be >= 1 so a model learns the constraint from the tool schema too — the ValueError message already covers the runtime path. No inline findings; nothing to change in this diff.

@Lesereingrape

Copy link
Copy Markdown
Contributor Author

Took the non-blocking suggestion in a523818: both batch_task arg docstrings now say the value must be >= 1 when set, so a model sees the constraint in the tool schema and not only in the runtime ValueError. No functional change — tests/test_subagent_batch_service.py, test_batch_task_tool.py, test_subagent_capacity_config.py, test_subagent_batch_repository.py and test_batch_acceptance.py still give 62 passed, ruff check/format clean. Thanks for running the red/green comparison independently.

@WillemJiang
WillemJiang marked this pull request as ready for review September 22, 2026 12:51
@github-actions github-actions Bot added area:agents Agents, subagents, graph wiring, prompts, langgraph.json area:backend Gateway / runtime / core backend under backend/ needs-validation Touches front/back contract surface; needs real-path validation risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/S PR changes 20-100 lines labels Sep 22, 2026
@WillemJiang WillemJiang added this to the 2.2.0 milestone Sep 22, 2026
@WillemJiang
WillemJiang merged commit 41ed506 into bytedance:main Sep 22, 2026
16 checks passed
xiaodu55 added a commit to xiaodu55/deer-flow that referenced this pull request Sep 22, 2026
main moved (bytedance#5609) and the base comparison flipped the nearly-over chain
back to an AG002 error (98441 > 98304). Trim the entry to the facts that
are not documented elsewhere: seams, allowlist, order, enqueue boundary.
WillemJiang added a commit that referenced this pull request Sep 25, 2026
…#5577)

* feat(memory): redact queued memory payloads (pii_redaction follow-up)

Closes the remaining vector from #3190 (memory storage): request-scoped
redaction leaves originals in thread state, and MemoryMiddleware queued
that raw conversation into the durable extraction buffer — so the
extraction model's input and the persisted facts carried raw PII even
with pii_redaction.enabled.

- MemoryMiddleware gains pii_redaction_config (same pattern as
  summarization / durable-context / title); the payload returned by
  _resolve_add_args() is redacted at the enqueue boundary before
  add()/aadd(), covering both after_agent and aafter_agent
- Reuses the shared redact_text() seam from #5527 via
  active_pii_detectors/_Redactor/_redact_content; the buffered payload
  itself becomes clean, not just the extraction model's view
- Wired at the lead-agent assembly site
- Tests: queue-payload redaction enabled/disabled, detector toggles,
  message immutability, token stability across turns

* fix(memory): reserve existing placeholder indices across the queued batch

Review round 1 on #5577: thread state mixes raw user turns with
already-redacted tool results and summaries, so a token in a later
message must not collide with a new value in an earlier one. Pre-scan
the whole queued batch with redactor.reserve() before allocating.

Also extend the two MemoryMiddleware stubs in
test_lead_agent_model_resolution.py to accept the new
pii_redaction_config kwarg (CI backend-unit-tests red).

* fix(memory): redact the compaction-triggered memory_flush_hook queue too

Review round 2 on #5577: with memory + summarization enabled,
memory_flush_hook() queues list(event.messages_to_summarize) directly
into MemoryManager.add_nowait(), bypassing MemoryMiddleware entirely —
and compaction removes those messages from state right after, so the
after-agent redaction can never repair a raw batch queued there.

Extract the batch redaction (reserve pre-scan + rebuild) into a shared
redact_queued_messages() used by both MemoryMiddleware's enqueue
boundary and the hook; the hook gains an optional pii_redaction_config
wired via functools.partial at the summarization factory (subagents use
skip_memory_flush=True and are unaffected). Regression covers the hook
path enabled/disabled/absent.

* test(summarization): unwrap the partial in the factory hook identity check

edd916e wrapped memory_flush_hook in functools.partial to carry the
pii_redaction config, but missed committing the matching update to
test_factory_attaches_memory_flush_hook_by_default — a functools.partial
is not equal to its wrapped function, so the identity assertion failed
deterministically (caught by static review on #5577).

* fix(memory): cover tool-call arguments and lenient text blocks in queued payloads

Review round 4 on #5577 — two reproduced gaps still allowed raw PII into
memory payloads with pii_redaction.enabled=true:

- OpenViking retains the full message object, so parsed tool_calls args
  and provider-format arguments in additional_kwargs bypassed the
  content-only redaction; queued payloads now redact tool-call args
  recursively (and reserve them in the pre-scan).
- DeerMem's format_conversation_for_update reads p.get("text") of any
  dict block, not only type=="text" — _redact_content now redacts every
  string-valued text field so a lenient consumer cannot bypass it.

Both repro shapes are pinned as regressions.

* fix(pii): value-derived placeholders for stable capture identity

Review round 5 on #5577: sequential allocation is order-dependent — the
same unchanged message re-redacted in a later batch can get a different
token, which flips OpenViking's content-signature and replays already
recorded turns into persistent memory sessions. Placeholders are now a
pure function of the matched value (sha256-6 per category): identities
stay stable across batches, enqueues, seams, and backends with no shared
state and no stored mapping.

- _Redactor becomes stateless; the reserve machinery (index pre-scans,
  _PLACEHOLDER_PATTERN) is removed along with its call sites
- redact_texts keeps its signature, now a simple per-text redaction
- reserve-semantics tests are superseded by cross-enqueue identity tests
  (two successive captures where the second batch carries higher existing
  placeholders); the compiled-graph echo regex matches the new format

* fix(pii): 128-bit placeholder digests; redact tool-argument keys

Review round 6 on #5577:
- [P2] The 24-bit truncation collided distinct identities (reproduced:
  contact3513@ / contact3727@example.com both rendered [EMAIL_575dbb]).
  Retain 128 bits of the digest so separate people stay separate at any
  realistic volume; the compiled-graph echo regex and the AGENTS.md entry
  follow the new format.
- [P2] _redact_strings redacted values but preserved mapping keys, so
  {"contacts": {"alice@example.com": "manager"}} kept the raw email
  as a key. Redact string keys along with values; deterministic 128-bit
  tokens make distinct-key collisions a hash-collision non-event, and a
  redacted key equal to another key merges the entries. Regression pins
  the email-keyed arguments shape.

* docs(agents): trim PiiRedactionMiddleware entry; drop stale reserve wording

The chain grew past the 98304-byte AG002 hard limit again (main growth
plus the round-6 format example). Trim the entry to fit, and replace the
stale reserve-indices wording with the value-derived semantics that
replaced it.

* docs(agents): shrink PiiRedactionMiddleware entry below the chain budget

main moved (#5609) and the base comparison flipped the nearly-over chain
back to an AG002 error (98441 > 98304). Trim the entry to the facts that
are not documented elsewhere: seams, allowlist, order, enqueue boundary.

* fix(pii): letters-only value-derived tokens; wire the factory SDK path

Review round 7 on #5577:
- [P2] Hex digests carry digit runs that later digit-anchored detectors
  re-scan, corrupting minted tokens into nested placeholders (reproduced:
  user280@example.com -> [EMAIL_…[PHONE_…]…], ~1/140 emails). The 128-bit
  digest is now encoded over a-z, so a minted token survives the full
  pinned detector order.
- [P2] create_deerflow_agent (the SDK path) constructed MemoryMiddleware
  and DurableContextMiddleware without pii_redaction_config; the knob is
  now an explicit optional parameter threaded through
  _assemble_from_features (the factory reads no config files by design,
  so SDK callers pass it explicitly).
- [Nit] hoist the Command rebuild comprehension out of the per-message
  loop in _redact_result.
- [Nit] drop the dead _iter_tool_calls helper and Iterator import.

* test(factory): pin pii_redaction_config wiring; document the parameter

Review round 8 on #5577:
- [P3] test_create_deerflow_agent.py had zero coverage of the
  pii_redaction_config parameter; two regressions now assert that
  create_deerflow_agent(pii_redaction_config=cfg) builds a
  MemoryMiddleware and a DurableContextMiddleware carrying cfg, and that
  the default is None (redaction off).
- [Nit] document the parameter in the create_deerflow_agent docstring:
  None leaves redaction off; a middleware= takeover bypasses the
  assembly that would apply it.

* fix(pii): HMAC-scoped placeholder tokens; install model-bound redaction in the SDK chain

Review round 10 on #5577:
- [P2] sha256(category||value) is an unkeyed, globally deterministic
  fingerprint: a provider or memory backend can confirm candidate values
  offline and link identical values across users and threads, contradicting
  the documented no-re-identification guarantee. Placeholders are now
  HMAC-derived over a deployment-scoped token_secret (new config field);
  the docstring documents the linkability scope for both modes.
- [P2] _assemble_from_features() never appended PiiRedactionMiddleware, so
  the SDK knob enabled only the downstream seams while the primary model
  still received raw content. The middleware is now appended in the same
  relative position as the lead-agent base chain when the gate is on,
  with a model-call regression through the assembled SDK chain.

* docs(pii): document token_secret; update the placeholder format examples

Follow-up to the round-10 review on #5577:
- config.example.yaml documents the new token_secret knob: a
  deployment-scoped secret makes placeholders HMAC-derived and only
  linkable within one deployment; leaving it unset keeps publicly
  computable fingerprints (documented trade).
- the PiiRedactionConfig class docstring's placeholder example now
  matches the shipped format (128-bit HMAC digests over base-26
  letters).

* docs(pii): update the module docstring's placeholder format example

The module docstring still showed the sequential [EMAIL_1] example;
align it with the shipped value-derived format (128-bit HMAC digests
over base-26 letters).

* fix(pii): require a non-empty token_secret when redaction is enabled

Review round 11 on #5577: with the secret optional, the default enabled
configuration minted empty-key HMAC digests — publicly computable, globally
linkable fingerprints of the raw values, confirmable by offline guessing for
low-entropy identifiers such as phone numbers.

- PiiRedactionConfig now rejects enabled=true without a non-empty
  token_secret (minimum 16 characters); the unkeyed mode is gone entirely.
- The request-scoped redactor in _process_request now carries the configured
  key: it built its redactor without it, so user-message placeholders
  silently diverged from every other seam for the same value. The empty-key
  guard in _placeholder_token is the backstop that keeps every path keyed.
- Module/config docstrings and config.example.yaml no longer describe the
  placeholders as irreversible or the secret as optional; the stale
  reserve-semantics wording and the "memory path not covered" note are
  corrected.
- Regressions: validation errors for missing/blank/short secrets, and the
  user-message boundary minting the same keyed token as redact_text.

* fix(memory): redact original_user_content on queued memory payloads

Review round 12 on #5577: UploadsMiddleware preserves the raw user turn in
additional_kwargs.original_user_content, but redact_queued_messages rewrote
only content, tool_calls, and provider-format tool_calls — the queued copy
still carried the raw value, so the memory queue and backend received raw PII
with redaction enabled.

The shared helper now redacts the provenance string field on the queued copy
(same batch redactor, merged with the existing additional_kwargs rewrite,
rebuilt via model_copy — thread-state messages stay untouched), covering both
the enqueue boundary and the compaction flush hook. Regressions pin the
redacted provenance field through both paths and the untouched original.

* fix(memory): redact human_input_response on queued memory payloads

Review round 13 on #5577: clarification replies keep the raw user answer in
additional_kwargs.human_input_response.value (DeerMem reads that mapping via
its read_human_input_response mirror), but redact_queued_messages rewrote
only content, tool_calls, and original_user_content — the queued copy still
carried the raw value into the memory queue and backend.

The shared helper now rebuilds that mapping with value redacted (same batch
redactor) while preserving the protocol fields (version/kind/source/
request_id/option_id/response_kind), merged with the existing
additional_kwargs rewrite and rebuilt via model_copy so thread-state
messages stay untouched. Regressions pin the redacted value, preserved
protocol fields, the untouched original, and the flush-hook path.

* chore(config): bump config_version to 47; generate token_secret on upgrade

Review round 13 on #5577: existing deployments can legitimately sit at
config_version 46 with pii_redaction.enabled: true and no token_secret; the
mandatory-secret validator would fail their startup while config.example.yaml
still said 46, so the upgrade checker considered them current and emitted no
migration warning.

- Bump config_version to 47 so the outdated-config warning fires for v46.
- Add a v47 migration to scripts/config-upgrade.sh: when pii_redaction is
  enabled and token_secret is missing/blank, generate a random deployment-
  scoped secret (secrets.token_urlsafe) and persist it, so the upgraded
  deployment starts under the mandatory validation.
- Regressions: a v46 enabled config is flagged outdated against the real
  example; the upgrade run generates a >= 16-char secret and the result
  passes AppConfig validation; a disabled config gets no secret.
- Also switch the two v41 script tests from a hardcoded "bash" to
  SCRIPT_BASH/skipif, matching the documented Windows constraint and the
  neighboring tests (hardcoded bash resolves to the WSL stub there).

* chore(helm): sync chart config_version to 47

Review round 14 on #5577: check_config_version.sh fails whenever the chart's
embedded config_version lags config.example.yaml, and the chart workflow is
triggered by config.example.yaml — so the v47 bump left this head failing
that check. Update the values.yaml config block and the README example to
47; scripts/check_config_version.sh now passes.

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
hawkli-1994 pushed a commit to rongxinzy/deer-flow that referenced this pull request Sep 27, 2026
…bytedance#5577)

* feat(memory): redact queued memory payloads (pii_redaction follow-up)

Closes the remaining vector from bytedance#3190 (memory storage): request-scoped
redaction leaves originals in thread state, and MemoryMiddleware queued
that raw conversation into the durable extraction buffer — so the
extraction model's input and the persisted facts carried raw PII even
with pii_redaction.enabled.

- MemoryMiddleware gains pii_redaction_config (same pattern as
  summarization / durable-context / title); the payload returned by
  _resolve_add_args() is redacted at the enqueue boundary before
  add()/aadd(), covering both after_agent and aafter_agent
- Reuses the shared redact_text() seam from bytedance#5527 via
  active_pii_detectors/_Redactor/_redact_content; the buffered payload
  itself becomes clean, not just the extraction model's view
- Wired at the lead-agent assembly site
- Tests: queue-payload redaction enabled/disabled, detector toggles,
  message immutability, token stability across turns

* fix(memory): reserve existing placeholder indices across the queued batch

Review round 1 on bytedance#5577: thread state mixes raw user turns with
already-redacted tool results and summaries, so a token in a later
message must not collide with a new value in an earlier one. Pre-scan
the whole queued batch with redactor.reserve() before allocating.

Also extend the two MemoryMiddleware stubs in
test_lead_agent_model_resolution.py to accept the new
pii_redaction_config kwarg (CI backend-unit-tests red).

* fix(memory): redact the compaction-triggered memory_flush_hook queue too

Review round 2 on bytedance#5577: with memory + summarization enabled,
memory_flush_hook() queues list(event.messages_to_summarize) directly
into MemoryManager.add_nowait(), bypassing MemoryMiddleware entirely —
and compaction removes those messages from state right after, so the
after-agent redaction can never repair a raw batch queued there.

Extract the batch redaction (reserve pre-scan + rebuild) into a shared
redact_queued_messages() used by both MemoryMiddleware's enqueue
boundary and the hook; the hook gains an optional pii_redaction_config
wired via functools.partial at the summarization factory (subagents use
skip_memory_flush=True and are unaffected). Regression covers the hook
path enabled/disabled/absent.

* test(summarization): unwrap the partial in the factory hook identity check

edd916e wrapped memory_flush_hook in functools.partial to carry the
pii_redaction config, but missed committing the matching update to
test_factory_attaches_memory_flush_hook_by_default — a functools.partial
is not equal to its wrapped function, so the identity assertion failed
deterministically (caught by static review on bytedance#5577).

* fix(memory): cover tool-call arguments and lenient text blocks in queued payloads

Review round 4 on bytedance#5577 — two reproduced gaps still allowed raw PII into
memory payloads with pii_redaction.enabled=true:

- OpenViking retains the full message object, so parsed tool_calls args
  and provider-format arguments in additional_kwargs bypassed the
  content-only redaction; queued payloads now redact tool-call args
  recursively (and reserve them in the pre-scan).
- DeerMem's format_conversation_for_update reads p.get("text") of any
  dict block, not only type=="text" — _redact_content now redacts every
  string-valued text field so a lenient consumer cannot bypass it.

Both repro shapes are pinned as regressions.

* fix(pii): value-derived placeholders for stable capture identity

Review round 5 on bytedance#5577: sequential allocation is order-dependent — the
same unchanged message re-redacted in a later batch can get a different
token, which flips OpenViking's content-signature and replays already
recorded turns into persistent memory sessions. Placeholders are now a
pure function of the matched value (sha256-6 per category): identities
stay stable across batches, enqueues, seams, and backends with no shared
state and no stored mapping.

- _Redactor becomes stateless; the reserve machinery (index pre-scans,
  _PLACEHOLDER_PATTERN) is removed along with its call sites
- redact_texts keeps its signature, now a simple per-text redaction
- reserve-semantics tests are superseded by cross-enqueue identity tests
  (two successive captures where the second batch carries higher existing
  placeholders); the compiled-graph echo regex matches the new format

* fix(pii): 128-bit placeholder digests; redact tool-argument keys

Review round 6 on bytedance#5577:
- [P2] The 24-bit truncation collided distinct identities (reproduced:
  contact3513@ / contact3727@example.com both rendered [EMAIL_575dbb]).
  Retain 128 bits of the digest so separate people stay separate at any
  realistic volume; the compiled-graph echo regex and the AGENTS.md entry
  follow the new format.
- [P2] _redact_strings redacted values but preserved mapping keys, so
  {"contacts": {"alice@example.com": "manager"}} kept the raw email
  as a key. Redact string keys along with values; deterministic 128-bit
  tokens make distinct-key collisions a hash-collision non-event, and a
  redacted key equal to another key merges the entries. Regression pins
  the email-keyed arguments shape.

* docs(agents): trim PiiRedactionMiddleware entry; drop stale reserve wording

The chain grew past the 98304-byte AG002 hard limit again (main growth
plus the round-6 format example). Trim the entry to fit, and replace the
stale reserve-indices wording with the value-derived semantics that
replaced it.

* docs(agents): shrink PiiRedactionMiddleware entry below the chain budget

main moved (bytedance#5609) and the base comparison flipped the nearly-over chain
back to an AG002 error (98441 > 98304). Trim the entry to the facts that
are not documented elsewhere: seams, allowlist, order, enqueue boundary.

* fix(pii): letters-only value-derived tokens; wire the factory SDK path

Review round 7 on bytedance#5577:
- [P2] Hex digests carry digit runs that later digit-anchored detectors
  re-scan, corrupting minted tokens into nested placeholders (reproduced:
  user280@example.com -> [EMAIL_…[PHONE_…]…], ~1/140 emails). The 128-bit
  digest is now encoded over a-z, so a minted token survives the full
  pinned detector order.
- [P2] create_deerflow_agent (the SDK path) constructed MemoryMiddleware
  and DurableContextMiddleware without pii_redaction_config; the knob is
  now an explicit optional parameter threaded through
  _assemble_from_features (the factory reads no config files by design,
  so SDK callers pass it explicitly).
- [Nit] hoist the Command rebuild comprehension out of the per-message
  loop in _redact_result.
- [Nit] drop the dead _iter_tool_calls helper and Iterator import.

* test(factory): pin pii_redaction_config wiring; document the parameter

Review round 8 on bytedance#5577:
- [P3] test_create_deerflow_agent.py had zero coverage of the
  pii_redaction_config parameter; two regressions now assert that
  create_deerflow_agent(pii_redaction_config=cfg) builds a
  MemoryMiddleware and a DurableContextMiddleware carrying cfg, and that
  the default is None (redaction off).
- [Nit] document the parameter in the create_deerflow_agent docstring:
  None leaves redaction off; a middleware= takeover bypasses the
  assembly that would apply it.

* fix(pii): HMAC-scoped placeholder tokens; install model-bound redaction in the SDK chain

Review round 10 on bytedance#5577:
- [P2] sha256(category||value) is an unkeyed, globally deterministic
  fingerprint: a provider or memory backend can confirm candidate values
  offline and link identical values across users and threads, contradicting
  the documented no-re-identification guarantee. Placeholders are now
  HMAC-derived over a deployment-scoped token_secret (new config field);
  the docstring documents the linkability scope for both modes.
- [P2] _assemble_from_features() never appended PiiRedactionMiddleware, so
  the SDK knob enabled only the downstream seams while the primary model
  still received raw content. The middleware is now appended in the same
  relative position as the lead-agent base chain when the gate is on,
  with a model-call regression through the assembled SDK chain.

* docs(pii): document token_secret; update the placeholder format examples

Follow-up to the round-10 review on bytedance#5577:
- config.example.yaml documents the new token_secret knob: a
  deployment-scoped secret makes placeholders HMAC-derived and only
  linkable within one deployment; leaving it unset keeps publicly
  computable fingerprints (documented trade).
- the PiiRedactionConfig class docstring's placeholder example now
  matches the shipped format (128-bit HMAC digests over base-26
  letters).

* docs(pii): update the module docstring's placeholder format example

The module docstring still showed the sequential [EMAIL_1] example;
align it with the shipped value-derived format (128-bit HMAC digests
over base-26 letters).

* fix(pii): require a non-empty token_secret when redaction is enabled

Review round 11 on bytedance#5577: with the secret optional, the default enabled
configuration minted empty-key HMAC digests — publicly computable, globally
linkable fingerprints of the raw values, confirmable by offline guessing for
low-entropy identifiers such as phone numbers.

- PiiRedactionConfig now rejects enabled=true without a non-empty
  token_secret (minimum 16 characters); the unkeyed mode is gone entirely.
- The request-scoped redactor in _process_request now carries the configured
  key: it built its redactor without it, so user-message placeholders
  silently diverged from every other seam for the same value. The empty-key
  guard in _placeholder_token is the backstop that keeps every path keyed.
- Module/config docstrings and config.example.yaml no longer describe the
  placeholders as irreversible or the secret as optional; the stale
  reserve-semantics wording and the "memory path not covered" note are
  corrected.
- Regressions: validation errors for missing/blank/short secrets, and the
  user-message boundary minting the same keyed token as redact_text.

* fix(memory): redact original_user_content on queued memory payloads

Review round 12 on bytedance#5577: UploadsMiddleware preserves the raw user turn in
additional_kwargs.original_user_content, but redact_queued_messages rewrote
only content, tool_calls, and provider-format tool_calls — the queued copy
still carried the raw value, so the memory queue and backend received raw PII
with redaction enabled.

The shared helper now redacts the provenance string field on the queued copy
(same batch redactor, merged with the existing additional_kwargs rewrite,
rebuilt via model_copy — thread-state messages stay untouched), covering both
the enqueue boundary and the compaction flush hook. Regressions pin the
redacted provenance field through both paths and the untouched original.

* fix(memory): redact human_input_response on queued memory payloads

Review round 13 on bytedance#5577: clarification replies keep the raw user answer in
additional_kwargs.human_input_response.value (DeerMem reads that mapping via
its read_human_input_response mirror), but redact_queued_messages rewrote
only content, tool_calls, and original_user_content — the queued copy still
carried the raw value into the memory queue and backend.

The shared helper now rebuilds that mapping with value redacted (same batch
redactor) while preserving the protocol fields (version/kind/source/
request_id/option_id/response_kind), merged with the existing
additional_kwargs rewrite and rebuilt via model_copy so thread-state
messages stay untouched. Regressions pin the redacted value, preserved
protocol fields, the untouched original, and the flush-hook path.

* chore(config): bump config_version to 47; generate token_secret on upgrade

Review round 13 on bytedance#5577: existing deployments can legitimately sit at
config_version 46 with pii_redaction.enabled: true and no token_secret; the
mandatory-secret validator would fail their startup while config.example.yaml
still said 46, so the upgrade checker considered them current and emitted no
migration warning.

- Bump config_version to 47 so the outdated-config warning fires for v46.
- Add a v47 migration to scripts/config-upgrade.sh: when pii_redaction is
  enabled and token_secret is missing/blank, generate a random deployment-
  scoped secret (secrets.token_urlsafe) and persist it, so the upgraded
  deployment starts under the mandatory validation.
- Regressions: a v46 enabled config is flagged outdated against the real
  example; the upgrade run generates a >= 16-char secret and the result
  passes AppConfig validation; a disabled config gets no secret.
- Also switch the two v41 script tests from a hardcoded "bash" to
  SCRIPT_BASH/skipif, matching the documented Windows constraint and the
  neighboring tests (hardcoded bash resolves to the WSL stub there).

* chore(helm): sync chart config_version to 47

Review round 14 on bytedance#5577: check_config_version.sh fails whenever the chart's
embedded config_version lags config.example.yaml, and the chart workflow is
triggered by config.example.yaml — so the v47 bump left this head failing
that check. Update the values.yaml config block and the README example to
47; scripts/check_config_version.sh now passes.

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:agents Agents, subagents, graph wiring, prompts, langgraph.json area:backend Gateway / runtime / core backend under backend/ needs-validation Touches front/back contract surface; needs real-path validation risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/S PR changes 20-100 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SubagentBatchService.submit() treats an explicit 0 batch limit as unset and persists the default instead

3 participants