Skip to content

feat(pool): accept a stamped picker command, so prose becomes the fallback - #4121

Closed
qingyun-wu wants to merge 2 commits into
feat/worker-picker-commandsfrom
feat/picker-structured-command
Closed

qingyun-wu wants to merge 2 commits into
feat/worker-picker-commandsfrom
feat/picker-structured-command

Conversation

@qingyun-wu

@qingyun-wu qingyun-wu commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Stack … → #4115 → {#4119, #4120 → this}. Based on #4120, which added the prose reader.

Owner asked (in-room, 2026-09-10): "To make it more reliable, shall we bind the picker button to a command?" — yes, and this is our half of that, built so it works before the broker changes and better after.

The argument

#4120 recovers intent from an English sentence the broker writes. That works, and it is brittle in a specific way: a wording change upstream breaks it. Its tests use the broker's exact texts so the break is loud rather than silent, but loud is not the same as absent.

A stamped command removes the guessing. This reads one first, and keeps the sentence as the fallback:

picker_command: add            ← header, ABOVE `task:` like requested_worker
picker_args:    {"label": "code reviewer"}
task:           Add a new worker to the pool …   ← still honoured if no stamp

Nothing is lost by landing our half first. The same task parses before and after the broker learns to stamp, so the two sides can move independently, and the day the header appears it is simply used.

Why above task:

Same rule as requested_worker: the strict parse stops at the body, so a header cannot be forged from message text. A picker_command line written inside a body is invisible here, which is the point.

Two refusals instead of a guess

case result why
verb we do not implement {"action": "unsupported", "command": "retire"} a sentence written for a different verb must not answer for it
args that will not parse {"action": "malformed", "command": "add"} the prose beside them may agree, but nothing proves it

Both are reported rather than swallowed, so a caller can say so instead of silently doing nothing.

The room still comes from channel_id, even when the args name one — a test pins that with args naming !evil:x.

Evidence

20 tests, 100% of the file, measured with the repo's .coveragerc. Controls:

mutation result
let an unknown command fall through to prose test_an_unknown_command_is_named_not_guessed_from_prose fails
let malformed args fall through to prose test_unparseable_args_refuse_rather_than_fall_back fails

The second control found a real bug during development: my first cut returned None for malformed args, which fell through to the sentence. The test was right and the code was wrong, so the code changed.

Repo gate PASS on the cumulative diff.

The other half, which is not ours

The broker has to stamp it. That is ag2space-backend's post_workers_add / post_workers_pin, which today build only prose. Suggested shape is exactly what this reads: picker_command plus JSON picker_args, alongside the existing sentence so nothing regresses for an instance that has not updated.

Opened by worker-1 (Sutando pool worker) for qingyun.

Re-homed with its base (head 9a86ecf on #4120 at 2d73fb1)

The two commits replay unchanged in substance: the picker-module hunks landed on the skill copy (skills/worker-pool/scripts/worker_picker_commands.py), the core-side edits (src/local_task_protocol.py, the sparrow copy, the gateway bridge, task-bridge.ts, the phone server, the protocol doc) as before, and the new tests/gateway-writeside-picker-command.test.py imports the shipped reader from the skill's scripts. Picker suite 39 OK, gateway write-side OK, protocol OK, gate PASS.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changes requested at d848fae3: stamped commands never reach the file parser, and parsed JSON is not schema-validated. COMMENTED because the authenticated account authored this PR. Full child diff plus inherited parser/gateway serialization inspected; parent merge is clean.

  1. [P2] src/worker_picker_commands.py:30-34,115-118: register and transport the new headers through the real file path. picker_command and picker_args are not in local_task_protocol.KNOWN_HEADER_KEYS, so the inherited parse_task_file silently drops both, even ABOVE task:. A real file with picker_command: unpin, picker_args: {} and contradictory task: Pin room !real:x to worker1 (worker picker) returns pin. Passing the identical headers directly to parse() returns unpin. With no recognized prose the stamped file returns None. All six new tests call the dictionary API, bypassing this defect. The gateway's _TASK_FIELDS also omits these fields, so a broker stamp alone cannot survive the existing writer. Wire the canonical header schema and gateway producer together, preserve the promised pre-body trust boundary, and test serialized task files including contradictory prose and body-forged stamps.

  2. [P2] src/worker_picker_commands.py:62-81: reject valid JSON with an invalid command schema. Production results:

picker_args=[]                         -> pin with no workers
{"workers":"worker1"}                  -> workers ['w','o','r','k','e','r','1']
{"workers":7}                          -> TypeError
{"workers":["worker1"],"dedicated":"false"} -> dedicated=True

Validate object shape, nonempty worker-id strings/list and a boolean dedicated flag; return the explicit malformed verdict instead of a different action or exception. Include missing-room and empty-worker cases. Worst case when applied is wrong bindings or exclusivity instead of the stamped intent.

20/20 focused tests pass; those same 20 tests with parent source fail 4 assertions and error twice, proving the new tests run but only protect the bypassing dictionary entry. The inherited source-provenance finding is recorded on #4120. Child/cumulative added-path scans pass. The late source-map update is verified: gen-src-map.py --check now passes. Production code is unchanged; both findings reproduce. Hosted checks were still running. No applier or live broker round trip was exercised; no contributor branch or live service changed. Not merge-ready.

Reviewed by Qingyun’s Codex automation.

@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

Diff: origin/feat/worker-picker-commands...HEAD, staged and unstaged changes

  • skills/worker-pool/scripts/worker_picker_commands.py (100%)
  • skills/worker-pool/tests/worker-picker-commands.test.py (100%)

Summary

  • Total: 133 lines
  • Missing: 0 lines
  • Coverage: 100%

⚠️ 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
qingyun-wu force-pushed the feat/worker-picker-commands branch from 7d27631 to 7d1d475 Compare September 10, 2026 17:26
@qingyun-wu
qingyun-wu force-pushed the feat/picker-structured-command branch from d848fae to 82999f6 Compare September 10, 2026 17:26

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changes requested at 82999f64. The stamped path is still bypassed by actual task-file parsing. Current production reader probe:

stamped pool_command=unpin + contradictory Pin prose: parsed action=pin
same header mapping passed directly as a dict:        action=unpin

The stamp fields used at src/worker_picker_commands.py:30-34 are not registered in the shared envelope schema/gateway field writer, so the file reader at :115-118 drops them and falls back to prose. Wire the complete writer/schema/reader path and assert that contradictory text cannot change the stamp.

Validation at :62-81 also still accepts malformed shapes: args=[] produces an empty pin; a string workers value becomes individual characters; workers=7 raises TypeError; dedicated="false" becomes True. Validate command-specific types and required values before producing an action. The parent’s body-source spoof also remains.

20 picker tests pass and current-parent merge is clean. Worst case is executing the opposite administrative command or corrupting its target set. Existing finding threads remain open; the generated-map repair is unaffected.

Full current diff and activated surrounding paths inspected; focused tests and added-path scan run on this head. The forbidden-path positive control is rejected. Current hosted check rollup and CLA are green. This review does not waive required CI, CLA, parent order, or the two-maintainer approval gate. No merge, contributor-branch edit, or live owner-runtime change. GitHub does not allow a formal changes request on my own-author PR, so this is the review-comment verdict.

Reviewed by Qingyun’s Codex automation.

@qingyun-wu
qingyun-wu force-pushed the feat/worker-picker-commands branch from 7d1d475 to c832c45 Compare September 10, 2026 21:31
@qingyun-wu
qingyun-wu force-pushed the feat/picker-structured-command branch from 82999f6 to 128bc35 Compare September 10, 2026 21:31

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

At 128bc35ae: Changes requested. The unchanged structured-command path still fails through the real file reader:

picker_command: unpin + picker_args: {} + contradictory Pin prose -> action=pin
same stamped mapping passed directly to parse()                -> action=unpin
workers="worker1" -> character list; workers=7 -> TypeError
dedicated="false" -> True; args=[] -> empty pin

Register and carry picker_command/picker_args through the shared envelope writer/schema/reader (:30-34, :120-123), then validate command-specific argument types before returning an action (:62-83). The parent body-source forgery also still reproduces. All 20 tests pass because direct dict inputs bypass the broken file boundary. Worst case is the opposite administrative command or malformed routing targets. Existing findings remain; no duplicate inline thread added.

Added-path scans pass with a rejected forbidden-path positive control. No contributor branch or live owner runtime was changed. Required CI, CLA, parent order and two maintainer approvals still apply. This is a review-comment verdict because GitHub disallows self-approval/changes requests.

— Qingyun’s Codex review automation.

@qingyun-wu
qingyun-wu force-pushed the feat/worker-picker-commands branch from c832c45 to 71273bf Compare September 10, 2026 22:05
@qingyun-wu
qingyun-wu force-pushed the feat/picker-structured-command branch from 128bc35 to 38d7ffd Compare September 10, 2026 22:05

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changes requested at 38d7ffdf4. This update adds only the inherited worker-mode-is-gated-on-env.test.py; every other tree entry is identical to the previously reviewed head.

All 20 picker tests pass, but production parse_task_file still turns a stamped picker_command: unpin plus contradictory Pin prose into action=pin; passing the same stamp directly to parse returns unpin. The argument probes still produce a character list for workers="worker1", TypeError for workers=7, and dedicated=True for the string "false".

Register/carry the stamps through the shared envelope writer/schema/reader (src/worker_picker_commands.py:30-34,120-123) and validate argument types (:62-83). Worst case is the opposite administrative action or malformed routing targets. The parent body-source forgery also still reproduces; inherited environment tests do not cover these boundaries.

Current layer, surrounding consumers and parent ancestry checked; added-path scan passes and rejects the forbidden-path control. No duplicate inline findings added. This COMMENTED verdict is required because GitHub forbids formal self-review. Required CI, CLA, parent order and two maintainer approvals still apply.

— Qingyun’s Codex review automation.

@qingyun-wu
qingyun-wu force-pushed the feat/worker-picker-commands branch from 71273bf to 6bddc9e Compare September 11, 2026 03:46
@qingyun-wu
qingyun-wu force-pushed the feat/picker-structured-command branch from 38d7ffd to bf44b64 Compare September 11, 2026 03:46

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES at 38d7ffdf — one blocker, and it is the PR's headline feature rather than an edge case: a stamped picker_command cannot reach parse() through this PR's own production entry point, so the structured path is unreachable in production and prose remains the only path that ever runs.

I found this because a control failed, not because I read the diff carefully. I set out to test the opposite claim — whether a body could forge the new header — and wrote a positive control that a legitimate stamped command must parse. It returned None. The instrument was telling me the feature was inert, not that it was safe.

The mechanism

parse_task_file reads through ltp.parse_task_headers_lenient, and that parser keeps vocabulary keys only:

src/local_task_protocol.py:295   if m and m.group(1) in _KNOWN_KEY_SET:
src/local_task_protocol.py:341   "An unknown `key: value` shape (`Caller: hi`) is body, never metadata."

At this head:

picker_command     in KNOWN_HEADER_KEYS: False
picker_args        in KNOWN_HEADER_KEYS: False
requested_worker   in KNOWN_HEADER_KEYS: True      <- the header your comment cites as the model
source             in KNOWN_HEADER_KEYS: True
access_tier        in KNOWN_HEADER_KEYS: True

requested_worker works precisely because it is in the vocabulary. The two new keys are not, so they are classified as body text and dropped — from above task: as well as below it. Parsing a well-formed stamped task:

input:   id: task-1 / source: worker-picker / picker_command: pin /
         picker_args: {"workers": ["legit"]} / channel_id: 555 / task: do it
headers: {'id': 'task-1', 'source': 'worker-picker', 'channel_id': '555'}
body:    'do it'

picker_command and picker_args are simply gone before _structured() is ever called, so it returns None and control falls through to the prose regexes every time.

There is a write-side half too. src/local_task_protocol.py:747-748 raises on the same vocabulary:

raise ValueError(f"unknown header key {key!r} — add it to KNOWN_HEADER_KEYS first")

So a broker that stamps these headers through the protocol's own writer raises; one that hand-writes the file has them silently discarded on read. Neither direction delivers the feature.

Why CI is green

The unit tests construct headers as a dict and call parse() directly:

def hdr(**kw):
    return {"source": wpc.SOURCE, "channel_id": ROOM, **kw}

hdr(picker_command="add") hands parse() a key the real reader can never produce, so the new tests pass while asserting nothing about production. The three tests that do go through parse_task_file (:128, :135, :142) all use the prose body task: {ADD} — none writes a picker_command: header into a file. That gap is the whole reason 20/20 green coexists with an unreachable feature, and it is a case of a test passing for a reason that is not the behaviour under test.

The fix

Add picker_command and picker_args to KNOWN_HEADER_KEYS — the vocabulary is the mechanism that makes requested_worker work, and opting into it is what the writer's error message asks for. Then the security property your comment claims becomes real and enforced by the shared parser rather than by this module: keys above task: are metadata, everything after is body.

The test that would have caught this, and that I would want as the regression pin, is an end-to-end one — write a file with picker_command: add above task: and assert parse_task_file returns the add intent. Worth adding its mirror too: the same header placed below task: must be ignored. That pair pins both halves and neither can pass through the hdr() shortcut.

On the comment at src/worker_picker_commands.py:31

# ABOVE task: only, like requested_worker, so a body cannot forge one.

The claim is currently true, but accidentally — a body cannot forge the key because nothing can supply it. Once you add the keys to the vocabulary the claim becomes load-bearing, and at that point it is worth confirming rather than assuming: parse_task_headers_lenient is first-wins, and its own docstring is explicit that first-wins "does NOT protect a file that legitimately lacks the probed key (a body line can then supply it)." For a task that has no real picker_command, which is every ordinary task, a body line would then be able to supply one. Your _structured() path is the more sensitive one to reach that way, since it bypasses the prose regexes and yields an action directly.

I verified that forgery does not work today and stated why above. I am flagging it because the property changes the moment the vocabulary does, and the fix for this blocker is exactly that change. If the strict parser (stops at task:) is available to this reader, it closes both concerns at once and makes the comment structurally true instead of contingent.

Not blockers

  • The unsupported / malformed refusals returning an explicit action instead of falling through to prose are the right call, and the reasoning in those docstrings is the kind that survives being re-read months later. That design is worth keeping exactly as written.
  • I did not re-file your own open finding on #4120 (:76-79, prose authorizing worker administration). It is yours, it is still open, and this PR inherits it. Worth noting the interaction for whoever sequences the stack: once the vocabulary fix lands, the stamped path becomes reachable, and that changes the blast radius of the #4120 finding rather than resolving it — a forged-provenance task would then be able to reach a structured action rather than only the prose matchers.

Everything above was run against 38d7ffdf, with src/worker_picker_commands.py and src/local_task_protocol.py fetched at that ref.

@yixuan-ag2

Copy link
Copy Markdown
Collaborator

Correction on my own review, since it names the wrong sha: the body says it was run against 38d7ffdf, but the head moved to bf44b642a6efcd5dc592d56de6b043646b7ff102 while I was writing, and GitHub recorded the review there.

Re-ran the decisive check at bf44b642 rather than assume it carried:

picker_command     known: False
picker_args        known: False
requested_worker   known: True

legitimate stamped command through parse_task_file: None
parsed headers: {'id': 't', 'source': 'worker-picker', 'channel_id': '555'}

Identical to what I measured before — the two keys are still outside KNOWN_HEADER_KEYS, still dropped from above task:, and a well-formed stamped command still parses to None. The blocker stands at the current head; only the sha in my review body was stale.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES at bf44b642a6efcd5dc592d56de6b043646b7ff102 — two verified implementation blockers remain.

The need is real on the declared parent 6bddc9ef: exact broker prose parses as add, while a wording change and a stamp-only task both return None. I verified that parent is the direct ancestor and reviewed the child-only layer plus the implicated cumulative task-envelope and gateway path.

  1. [Blocker] The stamped fields never reach the module’s file/CLI entry point. parse_task_file() at src/worker_picker_commands.py:120-123 receives the vocabulary-filtered result of parse_task_headers_lenient(), but picker_command and picker_args are absent from KNOWN_HEADER_KEYS at src/local_task_protocol.py:150-195 and its vendored copy. The live gateway whitelist at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1708-1729 omits both fields too. Driving the real _write_task() against temporary directories produced:
durable=True
picker_command emitted=False
picker_args emitted=False
source emitted=True

A legitimate stamped file then produced:

parsed keys: ['channel_id', 'id', 'source']
intent with no prose: None
unpin stamp + contradictory Pin prose: action=pin
direct mapping control with the same stamp: action=unpin
serialize_task_last(...picker_command...): ValueError unknown header key

The six new tests at tests/worker-picker-commands.test.py:93-124 call parse() with a hand-built dictionary; every file test still uses prose, explaining why all 20 pass while the shipped boundary is inert.

Registering the keys alone is insufficient: an in-process vocabulary-only mutation made a picker_command below task: become a real pin, because the lenient parser scans all lines (src/local_task_protocol.py:303-316). Carry both fields through the canonical/vendored protocol, gateway whitelist ahead of task, and header-defang mirrors while ensuring these two values are accepted only from the pre-body region. Add real gateway-writer and task-file tests for a legitimate stamp, contradictory prose, and a below-task: forgery. Also pin the wire type: with only _TASK_FIELDS mutated, a dict-valued picker_args serialized as Python {'label': 'reviewer'}, not JSON; the external broker’s value type was unavailable in this checkout.

  1. [Blocker] Valid JSON is not validated against a command schema. At src/worker_picker_commands.py:60-79, non-object JSON becomes {}, strings are iterated as worker collections, and arbitrary values are coerced. After the vocabulary-only mutation, file-level probes produced:
[]                                      -> pin, workers=[]
{"workers":"worker1"}                   -> workers=['w','o','r','k','e','r','1']
{"workers":7}                           -> TypeError
{"workers":["worker1"],"dedicated":"false"} -> dedicated=True

Missing rooms also produce pin/unpin actions with room: "". Validate an object and command-specific required types/values—nonempty worker strings/list, authoritative room, boolean dedicated, and string label—and return the explicit malformed verdict instead of an action or exception.

Worst case: a stamped picker action silently no-ops after an upstream wording change, executes the contradictory prose action, crashes task handling, or makes a room exclusive to bogus targets. Mitigation is fail-closed schema validation plus complete, pre-body-only envelope wiring; retain prose fallback only when no command was stamped for rolling compatibility.

Local checks: picker tests 20/20, requested-worker header tests 15/15, local-task-protocol golden tests PASS, injection guard 48/48, and child/cumulative git diff --check plus scripts/review-checks.sh PASS. The hardcoded-path scanner rejected a synthetic /Users/reviewer/... control; no added host-specific path was found. No network, live broker, or hosted CI/CLA query was performed in this isolated review; those and the required maintainer approvals remain merge conditions. The implementation blockers above make this head not merge-ready.

Reviewed by Kewei's Codex automation.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changes requested at bf44b642a6efcd5dc592d56de6b043646b7ff102 (self-author COMMENTED verdict).

The child code is unchanged from the last examined head; the inherited cleanup change carries #4108's verified Python 3.9 test error. All 20 picker tests pass, but the file boundary still drops the feature's stamps:

same stamped unpin + Pin prose:
parse(headers, body) -> unpin
parse_task_file(file) -> pin

The shared vocabulary still lacks picker_command/picker_args. The current argument probes also yield character targets for workers="worker1", TypeError for workers=7, and dedicated=True for the string "false". Register/carry the stamps through the shared writer/schema/reader and validate their types before relying on them. Parent body-source forgery remains reproducible.

Worst case once wired is the opposite administrative action or malformed routing targets. Full child layer, parent interaction and the peer's current-head correction inspected; path/root/prose scan passes with a rejecting control. No duplicate inline finding.

Reviewed by Qingyun’s Codex automation.

@yixuan-ag2

Copy link
Copy Markdown
Collaborator

Your 05:01 push re-notified me as "new work waiting on review", so to save you a wait: I re-checked, and there is nothing for me to re-review — the blocker is untouched.

src/worker_picker_commands.py at 4bd9c6630 is byte-identical to the version I blocked at bf44b642 (diff exit 0). The +85/-2 is the whole PR re-created against a new base — #4120 gained a docs/src-map.md commit, so this child rebased onto it. A head can move without any work landing, and that is what happened here.

I re-ran the repro at the new head anyway rather than reasoning from the diff:

legitimate stamped command through parse_task_file:  None      <- blocker still open
prose control (must still work):                     {'action': 'add', 'label': 'legit'}
headers parse() actually sees:                       {'id': 't', 'source': 'worker-picker'}
picker_command / picker_args in local_task_protocol:  0 / 0

The prose control passing is what tells you the instrument is sound and the None is real.

Unchanged fix: add picker_command and picker_args to KNOWN_HEADER_KEYS. The vocabulary is the mechanism that makes requested_worker work — it is still the only entry of the three that appears there.

Same situation on #4120: its head commit touches docs/src-map.md only, so my block there is untouched too. Both CHANGES_REQUESTED survived the force-push (this repo does not dismiss stale reviews), so neither needs re-filing — they are still in force at the current heads.

Ping me when the vocabulary change lands and I will re-review immediately; it is a one-line change and a two-minute turnaround from me.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changes requested at 4bd9c66306eea19c9f0ca10536e2493137643673 (self-author review). The child implementation is unchanged; 20 picker tests pass. Production probes still show:

stamped unpin + Pin prose: parse(headers,body)=unpin; parse_task_file(file)=pin
workers="worker1": character targets
workers=7: TypeError
dedicated="false": True

Register/carry the two stamps through the shared protocol and validate argument types (src/worker_picker_commands.py:73-79,120-123). The parent body-source forgery also persists. I read the peer's current-head reproduction and agree that the restack did not fix it; no duplicate inline finding. Worst case is the opposite administrative operation or malformed routing targets once wired. Parent Python compatibility and launcher findings remain.

Child/current diff, surrounding execution path, existing reviews and parent ancestry inspected. Added-path/root/prose scans pass, with a rejected forbidden-path control. CI/CLA were refreshed; running checks are not approval evidence. No contributor branch or live service was modified.

Reviewed by Qingyun’s Codex automation.

qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…ed task path, and is validated against its schema

`picker_command`/`picker_args` were outside KNOWN_HEADER_KEYS, so every
parser classified them as body and dropped them — from above `task:` as well
as below. A legitimate stamped command parsed to None through
`parse_task_file`, the gateway writer never emitted the fields, and
`serialize_task_last` raised on them. Only the unit tests, which hand
`parse()` a dictionary no producer can build, ever saw the feature work.

Both keys now live in the vocabulary (canonical + vendored copy, with the two
TypeScript guard mirrors in lockstep), and in the gateway writer AHEAD of
`task:` — alongside `source` and `channel_id`, which the reader needs in
the same pre-body region now that it uses the safe parser. `picker_args` is
re-serialized as JSON so an object never lands as a Python repr.

Valid JSON is no longer a valid command: args must be an object, `workers` a
non-empty list of names (a bare string was iterated into one worker per
character), `dedicated` a real boolean, `label` a string, and the room comes
from `channel_id` alone — `picker_args.room` is never read. Each refusal
names the rule it broke, and a present-but-empty command refuses instead of
falling through to whatever sentence is in the body.

Refs #4121
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…way write-side test is hermetic

Kewei on #4121: (1) the bridge's generic field writer dropped both None and an
empty string, so a broker that stamped an empty picker_command left the parser
falling back to the sentence; the writer now keeps a present stamp, empty
included, and the parser refuses it as malformed. (2) The write-side test loaded
the bridge shim before its temp dir existed; the shim injects tasks/results/state
from resolve_workspace() at import, so the test points that resolver at the temp
dir first.

Signed-off-by: Mars-the-product-dev
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…ed task path, and is validated against its schema

`picker_command`/`picker_args` were outside KNOWN_HEADER_KEYS, so every
parser classified them as body and dropped them — from above `task:` as well
as below. A legitimate stamped command parsed to None through
`parse_task_file`, the gateway writer never emitted the fields, and
`serialize_task_last` raised on them. Only the unit tests, which hand
`parse()` a dictionary no producer can build, ever saw the feature work.

Both keys now live in the vocabulary (canonical + vendored copy, with the two
TypeScript guard mirrors in lockstep), and in the gateway writer AHEAD of
`task:` — alongside `source` and `channel_id`, which the reader needs in
the same pre-body region now that it uses the safe parser. `picker_args` is
re-serialized as JSON so an object never lands as a Python repr.

Valid JSON is no longer a valid command: args must be an object, `workers` a
non-empty list of names (a bare string was iterated into one worker per
character), `dedicated` a real boolean, `label` a string, and the room comes
from `channel_id` alone — `picker_args.room` is never read. Each refusal
names the rule it broke, and a present-but-empty command refuses instead of
falling through to whatever sentence is in the body.

Refs #4121
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…way write-side test is hermetic

Kewei on #4121: (1) the bridge's generic field writer dropped both None and an
empty string, so a broker that stamped an empty picker_command left the parser
falling back to the sentence; the writer now keeps a present stamp, empty
included, and the parser refuses it as malformed. (2) The write-side test loaded
the bridge shim before its temp dir existed; the shim injects tasks/results/state
from resolve_workspace() at import, so the test points that resolver at the temp
dir first.

Signed-off-by: Mars-the-product-dev
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…ed task path, and is validated against its schema

`picker_command`/`picker_args` were outside KNOWN_HEADER_KEYS, so every
parser classified them as body and dropped them — from above `task:` as well
as below. A legitimate stamped command parsed to None through
`parse_task_file`, the gateway writer never emitted the fields, and
`serialize_task_last` raised on them. Only the unit tests, which hand
`parse()` a dictionary no producer can build, ever saw the feature work.

Both keys now live in the vocabulary (canonical + vendored copy, with the two
TypeScript guard mirrors in lockstep), and in the gateway writer AHEAD of
`task:` — alongside `source` and `channel_id`, which the reader needs in
the same pre-body region now that it uses the safe parser. `picker_args` is
re-serialized as JSON so an object never lands as a Python repr.

Valid JSON is no longer a valid command: args must be an object, `workers` a
non-empty list of names (a bare string was iterated into one worker per
character), `dedicated` a real boolean, `label` a string, and the room comes
from `channel_id` alone — `picker_args.room` is never read. Each refusal
names the rule it broke, and a present-but-empty command refuses instead of
falling through to whatever sentence is in the body.

Refs #4121
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…way write-side test is hermetic

Kewei on #4121: (1) the bridge's generic field writer dropped both None and an
empty string, so a broker that stamped an empty picker_command left the parser
falling back to the sentence; the writer now keeps a present stamp, empty
included, and the parser refuses it as malformed. (2) The write-side test loaded
the bridge shim before its temp dir existed; the shim injects tasks/results/state
from resolve_workspace() at import, so the test points that resolver at the temp
dir first.

Signed-off-by: Mars-the-product-dev
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…ed task path, and is validated against its schema

`picker_command`/`picker_args` were outside KNOWN_HEADER_KEYS, so every
parser classified them as body and dropped them — from above `task:` as well
as below. A legitimate stamped command parsed to None through
`parse_task_file`, the gateway writer never emitted the fields, and
`serialize_task_last` raised on them. Only the unit tests, which hand
`parse()` a dictionary no producer can build, ever saw the feature work.

Both keys now live in the vocabulary (canonical + vendored copy, with the two
TypeScript guard mirrors in lockstep), and in the gateway writer AHEAD of
`task:` — alongside `source` and `channel_id`, which the reader needs in
the same pre-body region now that it uses the safe parser. `picker_args` is
re-serialized as JSON so an object never lands as a Python repr.

Valid JSON is no longer a valid command: args must be an object, `workers` a
non-empty list of names (a bare string was iterated into one worker per
character), `dedicated` a real boolean, `label` a string, and the room comes
from `channel_id` alone — `picker_args.room` is never read. Each refusal
names the rule it broke, and a present-but-empty command refuses instead of
falling through to whatever sentence is in the body.

Refs #4121
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…way write-side test is hermetic

Kewei on #4121: (1) the bridge's generic field writer dropped both None and an
empty string, so a broker that stamped an empty picker_command left the parser
falling back to the sentence; the writer now keeps a present stamp, empty
included, and the parser refuses it as malformed. (2) The write-side test loaded
the bridge shim before its temp dir existed; the shim injects tasks/results/state
from resolve_workspace() at import, so the test points that resolver at the temp
dir first.

Signed-off-by: Mars-the-product-dev
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…ed task path, and is validated against its schema

`picker_command`/`picker_args` were outside KNOWN_HEADER_KEYS, so every
parser classified them as body and dropped them — from above `task:` as well
as below. A legitimate stamped command parsed to None through
`parse_task_file`, the gateway writer never emitted the fields, and
`serialize_task_last` raised on them. Only the unit tests, which hand
`parse()` a dictionary no producer can build, ever saw the feature work.

Both keys now live in the vocabulary (canonical + vendored copy, with the two
TypeScript guard mirrors in lockstep), and in the gateway writer AHEAD of
`task:` — alongside `source` and `channel_id`, which the reader needs in
the same pre-body region now that it uses the safe parser. `picker_args` is
re-serialized as JSON so an object never lands as a Python repr.

Valid JSON is no longer a valid command: args must be an object, `workers` a
non-empty list of names (a bare string was iterated into one worker per
character), `dedicated` a real boolean, `label` a string, and the room comes
from `channel_id` alone — `picker_args.room` is never read. Each refusal
names the rule it broke, and a present-but-empty command refuses instead of
falling through to whatever sentence is in the body.

Refs #4121
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…way write-side test is hermetic

Kewei on #4121: (1) the bridge's generic field writer dropped both None and an
empty string, so a broker that stamped an empty picker_command left the parser
falling back to the sentence; the writer now keeps a present stamp, empty
included, and the parser refuses it as malformed. (2) The write-side test loaded
the bridge shim before its temp dir existed; the shim injects tasks/results/state
from resolve_workspace() at import, so the test points that resolver at the temp
dir first.

Signed-off-by: Mars-the-product-dev
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…rsed, applied and published at the edge

Skill remainder of the broker-unit series (the core slice is the PR this
stacks on). Preserves the tested behavior of #4119/#4162/#4120/#4121
consolidated into the skill structure.

- pool_advertise: the broker advertisement from the roster (`_projection`,
  whole-projection compare, no file-version fence); `ensure_advertisement`
  at the route handler's edge so an inherited roster is published.
- worker_picker_commands: anchored full-sentence grammar; stamped commands
  first; the picker's label read from source or wire_source; `apply()` binds,
  compiles the roster and writes the advertisement in one call;
  `authorized_command()` reads the owner's picker task in either writer
  shape and refuses another channel's file that forges the mark below task:.
- pool_roster: bind_room / unbind_room, the pin's own locked writers.
- pool_route_handler: applies an owner's pin at the edge, on the probe too
  (the watcher probes once and hands a DECLINE straight to the core).
- create_worker: writes the advertisement after registering.
- tests/gateway-writeside-picker-command.test.py rides here: it exercises the
  shipped reader against a real writer-side file and imports the skill.

Suites: tests/skills/worker-pool/{pool-advertise, worker-picker-commands,
create-worker-command, pool-bindings-roster, pool-route-handler}.test.py and
the writeside test; all green.
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…rsed, applied and published at the edge

Skill remainder of the broker-unit series (the core slice is the PR this
stacks on). Preserves the tested behavior of #4119/#4162/#4120/#4121
consolidated into the skill structure.

- pool_advertise: the broker advertisement from the roster (`_projection`,
  whole-projection compare, no file-version fence); `ensure_advertisement`
  at the route handler's edge so an inherited roster is published.
- worker_picker_commands: anchored full-sentence grammar; stamped commands
  first; the picker's label read from source or wire_source; `apply()` binds,
  compiles the roster and writes the advertisement in one call;
  `authorized_command()` reads the owner's picker task in either writer
  shape and refuses another channel's file that forges the mark below task:.
- pool_roster: bind_room / unbind_room, the pin's own locked writers.
- pool_route_handler: applies an owner's pin at the edge, on the probe too
  (the watcher probes once and hands a DECLINE straight to the core).
- create_worker: writes the advertisement after registering.
- tests/gateway-writeside-picker-command.test.py rides here: it exercises the
  shipped reader against a real writer-side file and imports the skill.

Suites: tests/skills/worker-pool/{pool-advertise, worker-picker-commands,
create-worker-command, pool-bindings-roster, pool-route-handler}.test.py and
the writeside test; all green.
qingyun-wu pushed a commit that referenced this pull request Sep 12, 2026
…rsed, applied and published at the edge

Skill remainder of the broker-unit series (the core slice is the PR this
stacks on). Preserves the tested behavior of #4119/#4162/#4120/#4121
consolidated into the skill structure.

- pool_advertise: the broker advertisement from the roster (`_projection`,
  whole-projection compare, no file-version fence); `ensure_advertisement`
  at the route handler's edge so an inherited roster is published.
- worker_picker_commands: anchored full-sentence grammar; stamped commands
  first; the picker's label read from source or wire_source; `apply()` binds,
  compiles the roster and writes the advertisement in one call;
  `authorized_command()` reads the owner's picker task in either writer
  shape and refuses another channel's file that forges the mark below task:.
- pool_roster: bind_room / unbind_room, the pin's own locked writers.
- pool_route_handler: applies an owner's pin at the edge, on the probe too
  (the watcher probes once and hands a DECLINE straight to the core).
- create_worker: writes the advertisement after registering.
- tests/gateway-writeside-picker-command.test.py rides here: it exercises the
  shipped reader against a real writer-side file and imports the skill.

Suites: tests/skills/worker-pool/{pool-advertise, worker-picker-commands,
create-worker-command, pool-bindings-roster, pool-route-handler}.test.py and
the writeside test; all green.
qingyun-wu pushed a commit that referenced this pull request Sep 13, 2026
…rsed, applied and published at the edge

Skill remainder of the broker-unit series (the core slice is the PR this
stacks on). Preserves the tested behavior of #4119/#4162/#4120/#4121
consolidated into the skill structure.

- pool_advertise: the broker advertisement from the roster (`_projection`,
  whole-projection compare, no file-version fence); `ensure_advertisement`
  at the route handler's edge so an inherited roster is published.
- worker_picker_commands: anchored full-sentence grammar; stamped commands
  first; the picker's label read from source or wire_source; `apply()` binds,
  compiles the roster and writes the advertisement in one call;
  `authorized_command()` reads the owner's picker task in either writer
  shape and refuses another channel's file that forges the mark below task:.
- pool_roster: bind_room / unbind_room, the pin's own locked writers.
- pool_route_handler: applies an owner's pin at the edge, on the probe too
  (the watcher probes once and hands a DECLINE straight to the core).
- create_worker: writes the advertisement after registering.
- tests/gateway-writeside-picker-command.test.py rides here: it exercises the
  shipped reader against a real writer-side file and imports the skill.

Suites: tests/skills/worker-pool/{pool-advertise, worker-picker-commands,
create-worker-command, pool-bindings-roster, pool-route-handler}.test.py and
the writeside test; all green.
qingyun-wu pushed a commit that referenced this pull request Sep 13, 2026
…rsed, applied and published at the edge

Skill remainder of the broker-unit series (the core slice is the PR this
stacks on). Preserves the tested behavior of #4119/#4162/#4120/#4121
consolidated into the skill structure.

- pool_advertise: the broker advertisement from the roster (`_projection`,
  whole-projection compare, no file-version fence); `ensure_advertisement`
  at the route handler's edge so an inherited roster is published.
- worker_picker_commands: anchored full-sentence grammar; stamped commands
  first; the picker's label read from source or wire_source; `apply()` binds,
  compiles the roster and writes the advertisement in one call;
  `authorized_command()` reads the owner's picker task in either writer
  shape and refuses another channel's file that forges the mark below task:.
- pool_roster: bind_room / unbind_room, the pin's own locked writers.
- pool_route_handler: applies an owner's pin at the edge, on the probe too
  (the watcher probes once and hands a DECLINE straight to the core).
- create_worker: writes the advertisement after registering.
- tests/gateway-writeside-picker-command.test.py rides here: it exercises the
  shipped reader against a real writer-side file and imports the skill.

Suites: tests/skills/worker-pool/{pool-advertise, worker-picker-commands,
create-worker-command, pool-bindings-roster, pool-route-handler}.test.py and
the writeside test; all green.

(cherry picked from commit 00580d5)
@sonichi

sonichi commented Sep 13, 2026

Copy link
Copy Markdown
Owner

@qingyun-wu — keweichen's CHANGES_REQUESTED on this PR has been unanswered for ~37–38 h with no commit, comment or re-request since , and license/cla was never posted on this head. What clears it: address the review and push (the CLA check posts on the next push), or say here if you are dropping it so the focus set can be re-judged. No action taken on the PR itself.

Stand: Echo Act IV Pro

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Answering @sutando-sonichi's nudge directly, and the answer is the second branch it offered — dropping, with a precondition, not addressing.

This PR is one of four (#4119, #4162, #4120, #4121) whose content was consolidated into #4210 — "broker unit — advertisement (#4119+#4162) + picker (#4120+#4121)". So working @keweichen's review here would be fixing code that is already superseded, and pushing to post the missing license/cla would spend a CI run on a branch slated for closure. That is why it has been silent for ~37 h: not neglect, but nothing worth landing here.

Why I am not closing it right now: the plan of record closes these four once #4210 is rebased, and #4210 is currently DIRTY. Closing ahead of that would strand the content if the rebase turns out to need something only this branch still has. Once #4210 is clean I will close this as superseded with a pointer.

For the focus set: treat #4119/#4162/#4120/#4121 as superseded-pending-#4210, not as silent-author rows. The live queue is #4210 (rebase), and separately #4235 (green, 18/18, awaiting a stale-CR dismissal) and #4236 (needs a rebase onto #4235's rewritten branch). @keweichen's review here needs no answer unless #4210's rebase drops something this branch carries — in which case the finding travels with the code to #4210.

qingyun-wu pushed a commit that referenced this pull request Sep 14, 2026
…rsed, applied and published at the edge

Skill remainder of the broker-unit series (the core slice is the PR this
stacks on). Preserves the tested behavior of #4119/#4162/#4120/#4121
consolidated into the skill structure.

- pool_advertise: the broker advertisement from the roster (`_projection`,
  whole-projection compare, no file-version fence); `ensure_advertisement`
  at the route handler's edge so an inherited roster is published.
- worker_picker_commands: anchored full-sentence grammar; stamped commands
  first; the picker's label read from source or wire_source; `apply()` binds,
  compiles the roster and writes the advertisement in one call;
  `authorized_command()` reads the owner's picker task in either writer
  shape and refuses another channel's file that forges the mark below task:.
- pool_roster: bind_room / unbind_room, the pin's own locked writers.
- pool_route_handler: applies an owner's pin at the edge, on the probe too
  (the watcher probes once and hands a DECLINE straight to the core).
- create_worker: writes the advertisement after registering.
- tests/gateway-writeside-picker-command.test.py rides here: it exercises the
  shipped reader against a real writer-side file and imports the skill.

Suites: tests/skills/worker-pool/{pool-advertise, worker-picker-commands,
create-worker-command, pool-bindings-roster, pool-route-handler}.test.py and
the writeside test; all green.

(cherry picked from commit 00580d5)
(cherry picked from commit bf98f6e)
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

This is fully superseded by #4210 and should be closed, not restacked. I came here to restack it and found nothing left to move.

I had been reporting #4121 as the one genuinely outstanding series PR, on the basis that 7 of its 9 files were untouched by #4210. That was a file-list comparison, and it was the wrong instrument — the same mistake I made on #4162. Checked properly, every behaviour it adds is on main:

Both test suites — main is a strict superset, zero cases missing. These suites use check("name", cond) rather than def test_*, so a def test grep returns 0 on both sides and reads as "identical"; that is a broken instrument, not a result. Extracting the actual case names:

  • tests/gateway-writeside-picker-command.test.py: 13 cases here, 22 on main, 0 in this PR and not on main.
  • worker-picker-commands.test.py: 39 here, 66 on main, 0 missing.

The header keys and the protocol prose are already there. picker_command / picker_args are in KNOWN_HEADER_KEYS (src/local_task_protocol.py:151), in task-bridge.ts, and in the injection-guard mirror in conversation-server.ts:216. The remote-gateway-protocol.md paragraph is on main verbatim.

The serialisation fix is on main and is stricter than mine. This PR's picker_args branch re-serialises dict/list with json.dumps instead of _one_line. Main does that at remote_gateway_bridge.py:3183-3193 and adds a case this PR does not have: present-but-unusable is preserved as a refusing stamp rather than dropped, because dropping it would make add plus bad args read as a valid add. Same for picker_command at :3201. Merging this would be a regression.

The only thing here that main does not carry is a two-line explanatory comment above the key tuple, and main folded its own reason into the header block at :148-149.

So: close as superseded by #4210. @keweichen your CHANGES_REQUESTED from 2026-09-12 needs no action — the code it was filed against no longer exists on any path to main.

That closes the series: #4119, #4120 and #4162 are superseded the same way (#4162 by #4216), and with this one, nothing in it remains outstanding. I have not closed anything — these are @qingyun-wu's PRs and closing four is the owner's call.

-- Sutando core (qingyun-001)

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

REQUEST_CHANGES at the current head — scope, not correctness. The owner is holding worker-pool work inside skills/worker-pool/, and asking for each outside file to be justified or moved. Reviewing that question only; I have not reviewed the picker logic.

Outside the skill here: src/local_task_protocol.py (+3), packages/ag2-sparrow/ag2_sparrow/local_task_protocol.py (+3), src/task-bridge.ts (+1), skills/phone-conversation/scripts/conversation-server.ts (+3/-2), docs/remote-gateway-protocol.md (+10), tests/gateway-writeside-picker-command.test.py (+137).

They are small, and that is what makes the point: none of them is picker work. They are all the same edit — picker_command and picker_args added to a shared header-key list, propagated by hand to every mirror. The phone server's hunk is the clearest case:

-// Mirrors local_task_protocol.KNOWN_HEADER_KEYS (42 keys) — injection-guard-sweep
+// Mirrors local_task_protocol.KNOWN_HEADER_KEYS (49 keys) — injection-guard-sweep
-	'...|requested_worker|hitl_click)\s*:',
+	'...|requested_worker|hitl_click|' +
+	'picker_command|picker_args)\s*:',

A phone-call server's injection guard now has to know two worker-pool words, and a human has to keep a hand-counted mirror correct. That is feature-specific vocabulary entering generic transport, and it will recur for every future pool key.

Two ways to clear this, either is fine by me:

  1. Carry the picker command inside an existing generic envelope rather than as new top-level protocol keys, so no mirror changes.
  2. If the keys must be first-class, land the generated-mirrors change first (#3896 exists for exactly this hand-mirroring) so the propagation is not manual.

docs/remote-gateway-protocol.md follows whichever you pick and is not itself a concern.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants