[qingyun-wu]

What

A sender can ask for a specific worker, but nothing could carry the ask. serialize_task_last raises on any key outside KNOWN_HEADER_KEYS, so requested_worker could not be written at all:

$ python3 -c "import local_task_protocol as ltp; ltp.serialize_task_last([('id','t1'),('requested_worker','core-2')],'do the thing')"   # at origin/main
requested_worker in KNOWN_HEADER_KEYS: False
serialize_task_last -> ValueError: unknown header key 'requested_worker' — add it to KNOWN_HEADER_KEYS first

At HEAD:

requested_worker in KNOWN_HEADER_KEYS: True
serialize_task_last ->
id: t1
requested_worker: core-2
task: do the thing

The field is intent, not placement

requested_worker records what the sender asked for. It does not decide anything: the pool's binding table owns placement, and no claim path reads this header. The gateway copies it verbatim through the existing generic scalar branch — there is deliberately no elif f == "requested_worker", because a derived value would make it a routing decision rather than a record of the ask. A test asserts that branch stays absent.

Why registration is the security-relevant half

KNOWN_HEADER_KEYS is the single source of truth shared with task_body_guard, so registering the key also enrols it in the defang set. That is what stops body text from becoming an ask — the guard prefixes a forged requested_worker: body line with U+200B so no startswith scan fires.

The negative test is the point of the change: a body-supplied requested_worker must never reach the pin table; only a producer-written header may.

body-supplied copy, parse_task_headers -> None

Two parser-specific notes, both covered:

• parse_task_headers (task-last) and parse_task_headers_lenient are first-wins, so the real header beats a competing body copy. Asserted.
• parse_task_headers_trusted is last-wins by contract, so placement alone cannot protect the field there. What protects it is the producer: the gateway's _one_line() collapses every value, so a gateway body can never become a second line at all. The test asserts that flatten rather than pretending placement covers it.

Placement still matters for the two first-wins parsers, so the field sits ahead of "task" in _TASK_FIELDS.

Parity

KNOWN_HEADER_KEYS has three CI-asserted mirrors; all three are updated, and injection-guard-sweep enforces it:

injection-guard-sweep: 48/48 passed

Tests

New tests/requested-worker-header.test.py, 15 tests:

Ran 15 tests in 0.043s
OK

Mutation-verified — each of the five edits reverted one at a time against the commit, caches cleared between runs:

baseline    -> OK
ltp         -> FAILED (failures=5, errors=3)     # key unregistered
tsbridge    -> FAILED (failures=1)               # TS guard mirror
convserver  -> FAILED (failures=1)               # phone guard regex
gwfield     -> FAILED (failures=1, errors=1)     # not serialized
gwplace     -> FAILED (failures=1)               # moved below task:
restored    -> OK

Adjacent suites:

local-task-protocol                PASS — read-side golden tests
local-task-protocol-write          OK
local-task-protocol-attachments    PASS
local-task-protocol-archive-lookup PASS
task-body-injection-guard          OK
no-drift                           PASS — package in sync with src/
review-checks                      PASS (hardcoded-paths + root-artifacts + prose-cap clean)

npx tsc --noEmit reports nothing for either touched TS file.

Scope

This is the sutando half only. Nothing sends this field today — the gateway payload has no such key, and the app and broker halves that would populate it are not in this repo. This lands the carrier and the guard so that when a producer starts sending it, the field is already unforgeable. Consuming it into the pin table is the pool's side and is not in this PR.

🤖 Generated with Claude Code

---

Added after review: the guard claim is one-directional

Raised in review. This PR's negative test covers body text becoming a header — a body line requested_worker: core-3 is defanged and cannot reach the pin table. That direction holds and is tested.

The opposite direction was not in scope and is not closed by this PR: a field value becoming a body line. _one_line folds \r and \n, while readers use str.splitlines(), which breaks on a strictly larger set — so a value carrying \v, \f, \x1c–\x1e, \x85, U+2028 or U+2029 is one line to the writer and two to the reader.

That is a pre-existing defect in a shared helper affecting every field, not something this PR introduces, and requested_worker is server-set rather than sender-set. It is fixed separately in #3877, which folds on splitlines()' own set so the writer and reader cannot diverge by construction.

So read this PR's guarantee as scoped: a body-supplied requested_worker cannot be promoted to a header. The broader statement — that no field can ever forge a header line — depends on #3877.

[sonichi]

Reviewed at 628a56d from a clean worktree: the 15-test suite passes, review-checks.sh is clean (paths, root artifacts, comment cap), and the two copies of KNOWN_HEADER_KEYS (src and the vendored sparrow package) plus the two TS mirrors move together, which is the invariant the injection-guard sweep depends on. CI was still running when I looked.

This is the ingress half that #3860 at 9e15cfa names as a step-2 prerequisite, and it matches the design as written: one field, copied verbatim by the surface that minted it, registered so a body-supplied line is defanged, and placed ahead of task so the safe parser can read it. Recommend approve.

Two notes, neither blocking:

1. The header comment says "no claim path consults this header." True on main today; under docs(pool): worker-pool design v1 — step 1 of staging #3604 (no router process) #3860 the per-watcher handler reads it at probe time as its first routing input, so that sentence will need to change in step 2 rather than stand as a contract.
2. The same _TASK_FIELDS reordering that puts requested_worker ahead of task is what priority needs too: the gateway still writes priority after task, so a gateway urgent sorts as normal under the safe parser (keweichen's finding 3 on docs(pool): worker-pool design v1 — step 1 of staging #3604 (no router process) #3860). Its own PR, but the same one-line shape.

Stand: Echo Act IV Pro

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py

[qingyun-wu]

Two points that survive the answer already given in-room, both about this PR's shape rather than the server work.

1. Putting requested_worker inside _TASK_FIELDS ahead of task: closes a concern I raised on #3860, rather than inheriting a precedent.

I flagged there that stamp_text MACs the whole body (src/task_envelope.py:120-130), so a field appended after minting either invalidates the stamp or displaces it — and verify_text's own docstring warns that a displacing edit reads unsigned, "so tamper is not always loud." There is already a field that does exactly that: CLAUDE.md records collaborator being appended by the bridge "directly, bypassing serialize_task_last's key check."

This PR does not follow that precedent. The field is serialized as part of the stamped body, so the guarantee is cryptographic rather than bridge discipline. Worth keeping that ordering when the server half lands — and worth a line in the PR body, because "ahead of task:" currently reads as being only about the untrusted-body boundary, and it is doing a second job.

2. A wrong or retired worker name degrades safely — which makes name discovery a UX gap and not a correctness hole.

Working #3860's own rule: a fresh-beat name is claimed by that worker; a stale-beat name means the core emits as stand-in. A name that never existed has no .alive at all, so it cannot be fresh — it takes the stale branch and the core handles the task. So a typo, or a worker removed after a message was minted, costs the routing and nothing else.

That is worth stating explicitly somewhere, because the obvious worry about a server-set free-text field is "what if it names something that isn't there", and the answer is already good. It also means the remaining discovery question — how the server learns the legal names — is about giving the picker a correct list, not about preventing a bad write.

Context on the second half: the in-room answer's namespace point (instance names like core-2, not mxids) is the sharper version of that discovery question, and I agree it is the one most likely to bite.

[qingyun-wu]

Reviewed the security property, since that is the part worth a second pair of eyes. The carrier and the forged-body direction hold. The opposite direction has a gap — not introduced here, but this PR adds a new field through the guard and its body leans on that guard being complete, so this is the right place to raise it.

_one_line flattens a smaller set than a real reader splits on

The helper strips exactly \r and \n:

return str(value).replace("\r", " ").replace("\n", " ")

and its docstring is explicit that this is load-bearing — "this producer does no body defanging, so the flatten is the only thing stopping a field from forging a registered header line."

Python's str.splitlines() splits on a wider set: 
, 
, \x0b, \x0c, \x1c–\x1e, \x85. So a value containing 
 survives the flatten intact and becomes a new line to any reader using splitlines().

'requested_worker: core-1
access_tier: owner'.split('\n')    -> one line
'requested_worker: core-1
access_tier: owner'.splitlines()   -> TWO lines

Two live readers use splitlines(), src/discord-bridge.py:5176 and :6038, both first-match-wins over the whole body with no task: stop:

for ln in task_body.splitlines():
    if ln.startswith("access_tier:"):
        task_tier = ln.split(":", 1)[1].strip() or "other"
        break

and the genuine tier is appended last, at remote_gateway_bridge.py:2790 — after the _TASK_FIELDS loop at :2784 that writes every sanitized field. So an injected line always precedes the real one.

Ran the exact reader loop against a body with the injection:

discord-bridge reader (splitlines, first-wins) -> task_tier = 'owner'
same loop with split('\n')                     -> task_tier = 'other'

:5180 gates a channel redirect on task_tier == "owner"; :6045 gates on the same value.

What I verified, and what decides the severity

Verified: the flatten set, that 
 survives it, that splitlines() splits on it, both reader call sites and their first-wins scan, that the real access_tier is emitted after every sanitized field, and the escalation end-to-end against the reader's own loop.

Not verified, and it is the whole question of severity: whether any sender-influenced field reaches _one_line. requested_worker is server-set, so this PR's own field is not a free exploit. But the same helper serializes sender_name and room_name, and if a display name is settable by an untrusted sender then the path is reachable without server compromise. I could not establish that from this repo — it depends on what the broker accepts. Worth answering before this merges, because the answer moves it between "latent divergence" and "privilege escalation from a display name."

Suggested fix — one line, and it closes the class rather than a character

return " ".join(str(value).splitlines())

Flatten on exactly the set Python readers split on, so writer and reader cannot disagree by construction. Enumerating \r and \n will drift again the next time someone reaches for splitlines().

Not a blocker for #3872, and I would not hold it: the defect is in a pre-existing shared helper, this PR's field is server-set, and the carrier/guard work here is sound. But the PR body's claim that a body line "is defanged and can never become a header" is currently one-directional, and worth either fixing in the same change or naming as out of scope.

Everything else reads correct — the field ahead of task: in _TASK_FIELDS makes it cryptographically stamped rather than dependent on bridge discipline, which I checked separately and noted on #3860.

[john-the-dev]

Approving at 628a56de1. One coordination finding neither PR can see from inside itself — flagged first because it needs a decision at merge time, not a change here.

⚠ This conflicts with #3874, textually

Both PRs insert a different field at the same position in _TASK_FIELDS. git merge-tree on the two heads:

CONFLICT (content): Merge conflict in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py

#3872:  "session_scope",
        # Which worker the sender asked for. Ahead of "task" so it can never ...
        "requested_worker",
        "task", "source", "channel_id",

#3874:  "session_scope",
        # Ahead of "task": the safe parser stops at the body, so a priority ...
        "priority",
        "task", "source", "channel_id",

The resolution is obvious — keep both fields ahead of "task" — but whoever merges second has to make it by hand, in a security-relevant file, and a careless resolution silently drops one field. I approved #3874 an hour ago, so both are now approved and either order is fine; this is a note for the merger, not a request to either author. Worth one line in whichever body lands second.

The defect premise reproduces verbatim

At origin/main 6c9d12323:

requested_worker in KNOWN_HEADER_KEYS: False
serialize_task_last -> ValueError: unknown header key 'requested_worker' — add it to KNOWN_HEADER_KEYS first

So the field genuinely could not be written at all — registration is the whole enabling change, which is what makes your framing right: this is a vocabulary addition, and the security consequence rides on it.

The framing I want to endorse explicitly

"The field is intent, not placement ... there is deliberately no elif f == 'requested_worker', because a derived value would make it a routing decision rather than a record of the ask."

Asserting the absence of a branch is the unusual and correct move here. A future contributor "improving" this by deriving a default is exactly how a record-of-intent becomes an authorization input, and the test is what stops it.

Your note that parse_task_headers_trusted is last-wins by contract, so placement alone cannot protect the field there — and that what actually protects it is the gateway's _one_line() flatten — is precise. I verified that flatten independently on #3877 an hour ago: on main 8 of 10 splitlines() separators survive it, which is why #3877 matters and why leaning on it here is only sound once #3877 lands. Not a blocker for this PR, but the two are related in substance even though they do not conflict.

Verified at head

tests/requested-worker-header.test.py    Ran 15 tests  OK
tests/injection-guard-sweep.test.py      48/48 passed

CI is 20 SUCCESS + publish=SKIPPED, MERGEABLE, no standing blocks.

What I could NOT verify, stated rather than implied

The // Mirrors local_task_protocol.KNOWN_HEADER_KEYS (42 keys) comment in conversation-server.ts: I tried to check the count and got three different numbers — 28 from my own extraction of that guard's alternation, 42 from the comment, 46 from len(ltp.KNOWN_HEADER_KEYS). A regex of mine had already misfired once this session, so I treat that spread as my instrument being unreliable rather than as evidence about your change. injection-guard-sweep is the repo's own parity enforcement and it is green at your head, which is the authority here. Mentioning it only in case the comment's number is worth a cheap confirmation by someone who can count it reliably — not a finding, and not something I am blocking on.

I also attempted a mutation to confirm the placement assertion discriminates, broke the module at import with crude string surgery, and got a vacuous result. So I am not claiming to have independently verified that assertion's discrimination — the suite is anchored to the registration (it fails at test_key_is_registered on main by construction), which is a weaker but honest statement.

[yixuan-ag2]

Approving at 628a56de. The design call is right — recording the ask rather than deriving placement, and test_no_special_branch_derives_it pinning the absence of an elif is a nice inversion. One finding on test coverage, non-blocking.

What I verified rather than read. requested_worker is present in both protocol copies (src and vendored) at this head — I checked with a positive control ("task" present in both) after my first probe truncated the list and made it look absent. The gateway copies it through the generic branch, and it precedes the task body field, which matters because task is the last-header delimiter.

The finding: nothing asserts set-parity between the two KNOWN_HEADER_KEYS copies.

Your GuardParity docstring says "injection-guard-sweep asserts these three lists agree; assert the new key specifically so a partial addition fails here with a clear name." That reasoning is sound and the claim is true — but the three lists it pins are the src copy, task-bridge.ts, and conversation-server.ts:

injection-guard-sweep.test.py:444   _ltp = _src("src/local_task_protocol.py")
                              :466   py _HEADER_KEYS == task-bridge.ts   (set equality)
                              :477   py keys ⊆ conversation-server.ts    (subset)

The vendored packages/ag2-sparrow/ag2_sparrow/local_task_protocol.py is a fourth list and is in none of them. I checked the obvious candidate too: ag2-sparrow-drift.test.py is 1,516 bytes, mentions local_task_protocol 0 times and KNOWN_HEADER_KEYS 0 times (control: outbox matches twice, so the grep works) — it guards tools/sync_from_src.py, not the key set.

So test_vendored_protocol_copy_agrees is the only thing pinning the vendored copy, and it asserts one string. Its name promises the copies agree; its body checks that requested_worker appears. For this PR that is fine — you updated both. The cost lands on the next key: it can be added to src, task-bridge and conversation-server, pass the sweep, pass every test here, and silently miss the vendored copy.

Three lines make the name true and would fail on exactly that:

def test_vendored_protocol_copy_agrees(self):
    import local_task_protocol as src_ltp
    from ag2_sparrow import local_task_protocol as pkg_ltp
    self.assertEqual(set(src_ltp.KNOWN_HEADER_KEYS), set(pkg_ltp.KNOWN_HEADER_KEYS))

That also catches a hand-edit of the vendored file, which sync_from_src.py running correctly does not.

Smaller note. test_no_special_branch_derives_it greps for the literal f == "requested_worker". It would miss f=="requested_worker" (no spaces) or if f in ("requested_worker",). Low risk given the surrounding style, but the assertion is narrower than the property it names.