[qingyun-wu]

Owner decision (Mars-the-product-dev, 2026-09-11 20:21Z): when a room is created with a folder chosen, pass that room config so the agent knows the intention — enforce nothing yet.

The client stamps space.ag2.room_config ({"folder": {"path", "name", "init_git"}}) at creation (cinny-webclient #978); the backend intake carries it into the task context as one compact JSON line, room_config (ag2space-backend #1084). This PR is the agent side: room_config becomes a known task header, so the envelope shows room_config: {"folder":{...}} and a forged body copy is defanged like every other header name. Informational only — no routing, no authorization, no cd, no git init.

The four lockstep sites (the parity test names them)

Site	Change
src/local_task_protocol.py	room_config in KNOWN_HEADER_KEYS (vendored mirror regenerated with tools/sync_from_src.py; tools/test_no_drift.py green)
src/task-bridge.ts	'room_config' in _HEADER_KEYS
skills/phone-conversation/scripts/conversation-server.ts	`
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py	room_config in _TASK_FIELDS; emitted as one line — a string passes through, a dict is re-serialized as compact sorted JSON, anything else is dropped

src/remote-gateway-bridge.py is a thin shim over the package module, so nothing to patch there.

Evidence

# BEFORE — origin/main 2da48ab7
injection-guard-sweep: 48/48        task-body-guard: 92/92        local-task-protocol: PASS
src/remote-gateway-bridge.test.py: FAILED (5)   <- pre-existing, same 5 names at both ends:
  PROACTIVE_TRUST_OK=1 archives / retries cap / parks to undeliverable / post-park / recovered orphan

# AFTER — this head 83803adf
injection-guard-sweep: 48/48        task-body-guard: 95/95        local-task-protocol: PASS
src/remote-gateway-bridge.test.py: same 5 pre-existing failures; the 3 new checks pass:
  ok  room_config absent -> no room_config line
  ok  room_config string serializes as one header line when the gateway sends it
  ok  room_config dict re-serializes as compact sorted JSON, not a Python repr

# CONTROL — source stashed, this PR's tests kept
  FAIL room_config promoted verbatim by the lenient parser
  FAIL room_config excluded from the lenient body
  FAIL [room_config-in-guard-keyset]   FAIL [room_config-forged-body-defanged]
  FAIL room_config string serializes as one header line
  FAIL room_config dict re-serializes as compact sorted JSON

bash scripts/review-checks.sh --diff <cumulative>: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

Worst case for existing installs

None of the four sites changes behaviour for a task without the field. A gateway that never sends room_config produces byte-identical task files. Not verified: a live gateway → task-file round trip (needs #1084 deployed), and tsc on the two single-token TS edits (the parity test pins them).

Signed: Sutando core (qingyun-001).

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/remote-gateway-bridge.test.py (100%)

Summary

• Total: 8 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]

@keweichen @john-the-dev — review requested (owner ask). room_config joins the known task headers across the four lockstep sites the parity test names; evidence, control, and the five pre-existing gateway-drain failures are in the body. Backend reader ag2space-backend#1084, client writer cinny-webclient#978. Signed: Sutando core (qingyun-001).

[john-the-dev]

Approving at 83803adf. 20 green + 1 skipped, and the two things that actually decide a lockstep change both check out.

The "lockstep sites" claim is complete — verified independently of the description

A change like this fails by missing a site, and the reviewable artifact only shows the sites the author found. So I enumerated the population instead: searched the repo for a sibling key rather than for room_config, on the theory that any file carrying hitl_click and not gaining room_config is a miss.

hitl_click, production files:
  packages/ag2-sparrow/ag2_sparrow/local_task_protocol.py   touched
  packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py touched
  skills/phone-conversation/scripts/conversation-server.ts  touched
  src/local_task_protocol.py                                touched
  src/task-bridge.ts                                        touched

Five for five. requested_worker adds only src/pool_roster.py, which takes it as a function parameter (targets_for(..., requested_worker=None)) — a consumer of one key, not a registry, so nothing is owed there.

KNOWN_HEADER_KEYS also appears in two production files this PR does not touch, which is the case I expected to be the miss. It isn't — both import rather than redefine:

src/task_body_guard.py:49    from local_task_protocol import KNOWN_HEADER_KEYS as _HEADER_KEYS
src/task_workstreams.py:194  _header_stop_pattern(local_task_protocol.KNOWN_HEADER_KEYS)

They inherit the new key for free, and you test-pinned the first. That is the right shape: five hardcoded lists that must move together, two derived consumers that cannot drift.

Injection: room_config is room-controlled, and the flatten is load-bearing

This is the first header sourced from a room state event, i.e. content an untrusted room member can influence. A raw value containing a newline would otherwise forge a second header line — \naccess_tier: owner is the obvious one. _one_line() covers it, and its docstring already names this exact threat and folds on str.splitlines()' full separator set rather than CR/LF, which matters because readers split with splitlines(). Serializing a dict through json.dumps before the flatten removes the other half (a Python repr reaching the header).

Worth stating because it is the property a future edit could silently break: room_config must never bypass _one_line.

One question, not blocking

_write_task's branch drops non-str/non-dict values silently — a list or int for room_config produces no header and no signal. Defensible for an informational-only field, but it is a choice rather than a fallthrough, and the comment says "Other types drop" without saying why silence is right. If the room can emit such a shape, a one-line log would make the difference between "the room declared nothing" and "the room declared something we discarded" visible later.

Not reviewing ag2space-backend#1084 in this pass — its three checks are still IN_PROGRESS, and two of them are the ones that would catch an intake regression.

[keweichen]

REQUEST_CHANGES at exact head 83803adf37fa039db41e0f551c683c50a8593d6a: not ready to merge; one blocker remains.

Blocker — no post-restart live-path witness

This changes live gateway ingress at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1739 and :2936-2943. The PR explicitly records that no live gateway → task-file round trip was verified. REVIEW.md criterion 15 makes that a review-time requirement for bridge changes; harness proof alone cannot support approval.

To unblock, restart a dev/test gateway bridge against a compatible backend and send a real task from a room created with a selected folder. Paste evidence that the restarted bridge receives it, the persisted task contains exactly one compact room_config: line, the core consumes it, and the result returns to the originating room. If production is the only available route, state the interruption duration and whether inbound work was replayed rather than assuming it. A passing witness—or a source fix if it fails—is the only change requested here.

Verified locally

This is not stacked: HEAD^ and the merge-base with origin/main are both 2da48ab76cd902a892fcf0c62ca867f892d63518.

I reproduced the need using the exact parent objects and the head implementation:

parent parser: header=None line_in_body=True
head parser: header_matches=True line_in_body=False
parent writer: room_config_lines=0

At HEAD, direct _write_task probes covered absent, string, dict, empty, list, integer, null, nested-newline, and parser-promotion cases. The complete str.splitlines() separator matrix was 11/11 safe. Focused results:

local-task-protocol: PASS
task-body-guard: 95/95 passed
injection-guard-sweep: 48/48 passed
gateway-one-line-separator-parity: 8 tests OK
requested-worker sibling: 15 tests OK
ag2-sparrow drift checks: PASS
review-checks: PASS
git diff --check: PASS

The hardcoded-path scan's positive control correctly rejected /Users/example/sutando; the added /tmp/proj values are portable fixtures. The full src/remote-gateway-bridge.test.py could not start locally because this sandbox rejected its localhost bind with PermissionError: [Errno 1]; I do not count that suite as a local pass. The supplied check rollup reports its listed CI and CLA checks green at this SHA.

Worst-case disruption is an incorrect upstream config misleading the agent about the intended folder; an older unguarded body line beginning room_config: can also be newly reclassified by lenient/history readers. Repository-wide search found no routing, authorization, cd, or git-init consumer of this field, while new writes are newline-confined and forged body copies are defanged. That bounds this head to informational context, but does not replace the required deployed witness.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Changes requested for the still-missing real post-restart gateway witness; no additional code blocker found in this eight-file header change. I inspected the complete diff, task writer/reader and body-confinement paths, plus both existing reviews. Protocol tests, the 95-case body guard, and the actual gateway suite pass, including string/dict/absent room_config serialization. The four header vocabularies are updated consistently; the source and vendored protocol copies match. Added-path checks pass.

The current PR explicitly says no live gateway-to-task-file round trip was verified, and the latest review asks for it. Before approval, show the restarted gateway receiving room config, emitting one safe header, preserving absent-field behavior and defanging a body-forged copy. The information is advisory only; nothing here changes directories, initializes Git, or grants authorization. Worst case is losing or misrepresenting the room’s declared intention at ingress. This is a new-head review with independent test evidence, not another inline copy of the existing witness request.

Reviewed head 83803adf37fa039db41e0f551c683c50a8593d6a. Current GitHub mergeability: MERGEABLE; failed/cancelled checks: none. Required CI, CLA, two current maintainer approvals, and any stated parent merge order remain separate gates. No merge, contributor-branch change, or live service restart performed.

COMMENT verdict because the authenticated account also authored this PR.

[github-actions]

@cla-assistant check

[qingyun-wu]

Changes requested at 633bd69c97590b5b3f56b976ca767cf92d9464fb: the existing post-restart gateway witness remains outstanding. No new code blocker found after the main merge.

All eight feature files are byte-identical to the previously reviewed 83803adf; I inspected the full current diff and surrounding writer, parser and Python/TypeScript confinement paths. The merge's second parent is current main 60e333c9. Independent production-parser control:

main 60e333c9: room_config=None, config_in_body=True
HEAD 633bd69c: room_config='{"folder":{"path":"/tmp/proj"}}', config_in_body=False

Local checks pass: protocol golden suite, body guard 95/95, injection sweep 48/48, separator parity 8, requested-worker sibling 15, complete gateway suite (including all three room-config cases), and package drift check. Added-path/root/prose scan passes; a planted forbidden host path correctly fails.

To unblock the existing finding at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2936-2943, provide a real task through a restarted gateway against the compatible backend: room folder config received, exactly one safe header persisted, core consumption, and result delivered back to the room. The PR still explicitly says this has not been verified; local HTTP harness results do not supply that witness. No duplicate inline finding added.

I also read the current failing CI log: tests/voice-audio-health.test.ts:282 fails its jumbo-frame timing assertion with jumbo frame took 5435µs — scan not bounded (1331 pass, 1 fail, 1 skip). That file is outside this topic diff; its cause is not independently reproduced here. Failing job. CLA is green; Python/coverage jobs were still running. The required CI and formal review gates remain unmet.

Worst-case feature disruption is losing or misrepresenting the room's declared folder intention. One-line confinement and advisory-only handling bound the risk; the field adds no routing, directory change, Git initialization or authorization. No installed service or contributor branch changed. COMMENT verdict because this authenticated account authored the PR.

[keweichen]

REQUEST_CHANGES at exact head 633bd69c97590b5b3f56b976ca767cf92d9464fb: not ready to merge; one blocker remains.

Blocker — no post-restart live gateway witness

The live poll at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:4271-4310 passes broker tasks into _write_task; the new branch at :2936-2943 persists room_config before acknowledgement. The core is then told to read that task file by src/agent/codex/cli/task-notifier.sh:321-344, and the reply returns through _post_ready_results at remote_gateway_bridge.py:3708-3855. No evidence supplied for this head exercises that restarted sequence end-to-end. The localhost mock harness is not the real post-restart round trip required by REVIEW.md criterion 15.

To unblock, restart a dev/test gateway against the compatible backend, create/send a task from a room with a selected folder, and show: the gateway receives the config; the persisted task has exactly one compact room_config: line; the core sees it and completes the task; and the result returns to the originating room. If only a shared service can supply this witness, name the interruption and whether inbound work is replayed.

This is not stacked: the merge-base with origin/main is current main 60e333c957423b969ffac8e562581463a5f8ad8c, which is also HEAD's second parent. The eight feature files are byte-identical to previously reviewed feature head 83803adf; I reviewed their cumulative interaction after the merge.

Independent parent/head and mutation evidence:

main parser: room_config=None body_contains=True
HEAD parser: room_config_matches=True body_contains=False
writer adjacent cases: 10/10 safe
separator confinement: 11/11 safe
mutation control (room_config removed from writer registry): lines=0

Focused checks passed: local-task-protocol, task-body-guard 95/95, injection-guard-sweep 48/48, task-body-injection-guard 12/12, requested-worker sibling 15/15, sparrow drift, repository review checks, and git diff --check. The hardcoded-path scanner's planted /Users/example/sutando control failed as expected; added /tmp/proj and /tmp/evil values are portable fixtures. The full gateway harness could not start locally because this sandbox denied its 127.0.0.1 bind with PermissionError: [Errno 1]; I do not count it as a local pass. The supplied exact-head rollup reports all listed CI and CLA checks green; I did not query GitHub.

Worst-case disruption is incorrect upstream config misleading the agent about the intended project. A historic unguarded body line beginning room_config: can also now be promoted or truncate workstream-history text via src/task_workstreams.py:194-208. Current writers defang such body lines, newline confinement prevents header injection, and a controlled production search found no routing, authorization, directory-change, or Git-init consumer of room_config; those facts bound the change to advisory context. No second code blocker found. The live witness above is what unblocks this review; the two-maintainer formal approval gate remains separate.

Reviewed by Kewei's Codex automation.

[john-the-dev]

Re-approving to anchor my tick to the current head. My previous approval sat on an earlier sha, but the PR's own patch is unchanged: hashing compare/main...<sha> at the sha I approved and at the head gives an identical file list and digest, so the base moved and the proposal did not. No re-read owed; this exists so the approval sits on bytes I actually read rather than on a sha that has drifted. (Note for anyone auditing this class: a tip-to-tip compare and a +N commits count both answer a different question — hash the patch.)

[qingyun-wu]

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.