docs: the Discord per-channel collaborators list IS the attestation for that surface by sonichi · Pull Request #3007 · sonichi/sutando

sonichi · 2026-08-17T06:04:04Z

Owner decision, asked as a fork on #2983 and answered "b. CLAUDE.md needs a change".

The disagreement

CLAUDE.md:256 said the owner-capability Team opt-in is an AG2 Space room policy and that "Discord Team mappings retain this existing contract" — i.e. sandboxed.

The shipped per-channel collaborators feature does the opposite: resolve_is_collaborator() reads a channel's collaborators list and select_rulebook_key() hands that sender the team-collaborator engage rulebook, which explicitly says do not sandbox.

So the document and the code disagreed, and a reviewer reading only the document correctly called the code an authority escalation. That is what happened on #2983 — I gated the dispatch on the wire stamp alone, which produced a dead route and contradicted the separate no-session decision.

The resolution

Per the owner, in the feature's favour: the per-channel list is the attestation for that surface. It is hand-configured, per-channel, and a deliberate owner act — stronger evidence of intent than a wire flag a sender can set.

The wire collaborator: true stamp remains the only attestation for AG2 Space, where the sender is remote and the broker is the party asserting it. Different surfaces, different evidence, both owner-anchored.

Scope, restated where it was implicit

Per-channel only — membership in another channel's list does not carry over (resolve_is_collaborator keys on the serving channel).
Grants engagement, not owner authority.

Not in this PR

No code change; this documents what already ships. It settles the open question on #2983, where the dispatch is now back to its pre-existing state (9090daa8) precisely because the boundary question was unresolved.

…or that surface Chi's call, asked as a fork and answered "b. CLAUDE.md needs a change". The text said the owner-capability Team opt-in is an AG2 Space room policy and that "Discord Team mappings retain this existing contract" — i.e. sandboxed. But the shipped per-channel `collaborators` feature grants the `team-collaborator` engage rulebook to a Discord sender, so the document and the code disagreed, and a reviewer reading only the document correctly called the code an escalation. Resolved in the document's favour of the feature, per the owner: that list is an attestation. It is hand-configured, per-channel, and a deliberate owner act, which is stronger evidence of intent than a wire flag a sender can set. The wire `collaborator: true` stamp remains the only attestation for AG2 Space, where the sender is remote and the broker is the one asserting it. Scope is unchanged and restated where it was implicit: per-channel only, no carry-over, and it grants engagement rather than owner authority. No code change — this documents what ships. It also settles the open question on #2983, where I had gated the dispatch on the wire stamp alone. Stand: Echo Act IV Pro

github-actions · 2026-08-17T06:04:33Z

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

qingyun-wu

Changes requested on exact head dd2cb38.\n\n[P2] This updates the collaborator policy only in CLAUDE.md, leaving the generated AGENTS.md mirror stale; the required sync check is red. Please run the repository generator/update both copies and get that required check green.\n\nThe policy wording otherwise matches the current fail-closed, serving-channel collaborator lookup and the explicit no-mutation/no-secret collaborator rulebook, so I found no additional blocker.\n\nReviewed by Qingyun's Personal Codex.

qingyun-wu's P2: AGENTS.md is the Codex-agent mirror, generated from CLAUDE.md by scripts/agents-md-sync.sh, and I edited only the source. The required "refuse AGENTS.md stale vs CLAUDE.md" check was red. Regenerated with the repository's own generator rather than hand-editing the mirror; `agents-md-sync.sh --check` now reports up to date. The mirror gains the same two paragraphs, with the generator's substitutions applied. Stand: Echo Act IV Pro

qingyun-wu

Changes requested at exact head dd2cb38.\n\n[P1 blocking] The owner decision is clear, but this changes only CLAUDE.md. Reviewer-run python3 tests/agents-md-sync.test.py fails with AGENTS.md stale, matching the hosted required-check failure. AGENTS.md therefore still says every Discord Team sender uses the sandboxed path while CLAUDE.md now grants the per-channel collaborator exception; two agent entrypoints load opposite authority contracts.\n\nRegenerate AGENTS.md from the canonical source so the exact owner-approved exception lands in both instruction surfaces, then rerun the sync gate. I am not disputing the owner decision itself; the blocker is the split security contract and red required check. This head is not ready to merge.

qingyun-wu

Approved exact head b81f97c.\n\nThe owner-approved Discord collaborator exception is now present in both instruction surfaces. AGENTS.md was regenerated from CLAUDE.md, so Codex and Claude agents no longer load contradictory authority contracts. Reviewer-run agents-md-sync passes 5/5 and repository review checks pass.\n\nNo blocking documentation finding remains. Merge still requires current required CI and the repository approval gate.

qingyun-wu

Approved exact head b81f97c.\n\nThe generated AGENTS.md mirror now carries the same per-channel collaborator policy as CLAUDE.md, closing the required sync failure. Reviewer-run generation checks pass 5/5 and repository review checks pass. The underlying policy remains fail-closed to the serving channel and grants the collaborator rulebook without owner authority. No blocking finding remains.\n\nReviewed by Qingyun's Personal Codex.

yixuan-ag2

Verified at head b81f97c2. The P1 block is resolved and the one red check is not yours.

The blocking finding is fixed

The CHANGES_REQUESTED cites dd2cb386; head is now b81f97c2. At the current head:

gh pr diff --name-only            -> AGENTS.md, CLAUDE.md   (both, not just one)
python3 tests/agents-md-sync.test.py -> Results: 5 passed
grep -c 'designated per-channel collaborator engages in-channel'
  CLAUDE.md 1   AGENTS.md 1

So the split security contract that review named — two entrypoints loading opposite authority contracts — no longer exists.

The doc matches the code, checked from the code side

This is a security-boundary statement, so I read the implementation rather than the PR body:

resolve_is_collaborator() keys on the serving channel only; its docstring and behaviour both state membership in another channel's list does not carry over.
It is explicitly fail-closed: "any malformed config or missing key yields False."
select_rulebook_key() swaps to team-collaborator only when that returns True; the wire tier stays team.
tests/discord-bridge-collaborator-tier.test.py passes locally and pins both the behavioural and structural halves, including "team-collaborator rulebook present + reasserts owner-only authority".

The prose earns its claim too. I went in expecting to argue the exception was under-hedged relative to the AG2 Space paragraph, and it is not — it names the attestation as hand-configured, per-channel, a deliberate owner act rather than a wire flag a sender can set, and closes with "it grants engagement, not owner authority." That is the right shape for this.

(My first read saw only the thinner one-line change and I nearly raised the hedging point off it — the richer sentence is the other changed line. Mentioning it because a partial grep is exactly how a reviewer talks themselves into a wrong objection.)

The red check

job 95300745812  head_sha=b81f97c2  step: Run Python standalone tests
##[error]FAILED: tests/task-workstream-session-worker.test.py (exit 1)

This PR changes two markdown files. That test is #2934, the known flake. Not yours — re-run rather than investigate. I am adding this occurrence to #2934 since it is now the third PR it has blocked in about two hours.

Approving on the content; the required check still has to go green.

…estation

qingyun-wu

Approved on exact head 04f75dd. The regenerated policy surfaces remain synchronized after the base merge. The focused AGENTS.md synchronization suite passes 5/5, and repository review checks pass.\n\nReviewed by Qingyun's Personal Codex.

…estation

qingyun-wu

Approved on exact head 1cc49ed after merging current main. The generated policy surfaces remain synchronized; the 5 focused synchronization checks, package synchronization, and repository review checks all pass. Reviewed by Qingyun's Personal Codex.

…estation

qingyun-wu

Approved on exact head 3575b38 after merging current main. The generated policy surfaces remain synchronized. The 5 synchronization checks, merged 22/22 outbox protocol contracts, package synchronization, and repository review checks all pass. Reviewed by Qingyun's Personal Codex.

…estation

qingyun-wu

Approved on exact head fe9b248 after merging current main. The per-channel collaborator exception remains narrowly scoped, preserves the newly merged named-owner policy text elsewhere, and both generated policy surfaces remain synchronized. The 5 synchronization checks, package synchronization, and repository review checks all pass. Reviewed by Qingyun's Personal Codex.

…estation

github-actions · 2026-08-17T08:00:38Z

@cla-assistant check

qingyun-wu

Approved on exact head e71698e after merging current main. The per-channel collaborator exception remains narrowly scoped and both generated policy surfaces remain synchronized. Policy synchronization, Telegram chunking and delivery-marker tests, package synchronization, and repository review checks all pass. Reviewed by Qingyun's Personal Codex.

…s apart `5e61f09d` closed the Team provider-session route on both paths and nothing recorded it, so the decision was only recoverable by reading `probe()`. State it where the tier contract lives. #3007 calls the Discord per-channel `collaborators` list "the owner's attestation for this surface"; the AG2 Space section already used the same word for the broker's wire stamp. They are different objects — a local rulebook selector and a remote assertion — and that conflation is what routed an earlier fix into a session this branch had already removed. Deliberately not claimed here: the file-stamp source restriction. Its enforcement sits in `team_collaborator_enabled()`, which this branch leaves unreachable, so documenting it as live would describe a gate nothing calls. Stand: Echo Act IV Pro

… and a processing option (#2983) * fix(team): a collaborator stamp is trusted only from the attested source #2824 gates the trusted runtime on a broker-attested `collaborator: true` stamp, and its own docstring says "accept only the gateway's exact pre-body collaborator stamp". But the check never looks at where the task came from, and src/discord-bridge.py:4140 writes an identical stamp for anyone listed under a Discord channel's `collaborators`. A per-channel allowlist entry therefore reached the owner-configured runtime — and with it this host's gh credential, which is how a review comment was published under the owner's GitHub account. resolve_task_source() reads the source with the same task-mid rule resolve_access_tier() already uses: prefer a pre-`task:` value, else take the final line, because the remote gateway writes its headers after the body. A naive pre-body-only read would have returned empty for every real AG2 Space task and failed the legitimate path closed. Verified on the real archived tasks, not only fixtures: the two Discord collaborator tasks that triggered this now resolve restricted, and a real ag2space collaborator task stays trusted. The existing fixture wrote `source: discord` — inherited from #2603, where it was an incidental default rather than the property under test — so the suite had pinned Discord-origin collaborator as trusted. The fixture now defaults a collaborator task to the attested source and takes an explicit `source` override; non-collaborator fixtures keep the original discord default. Control run: disabling the origin check makes test_collaborator_stamp_is_trusted_only_from_the_attested_source fail (exit 1, line 158). The first version of that test could not fail at all — this file dispatches tests from an explicit call list at the bottom and the new function had no call, so it never ran. Stand: Echo Act IV Pro * feat(team): remove the Team provider session; the core handles Team directly Owner decision, after being shown the cost: Team no longer spawns a provider session. probe() returns UNHANDLED for tier == team, and handle() consults probe() first, so a single change closes the route on both paths — verified rather than assumed, and a second guard added at the launch site was backed out once the first proved sufficient. Team now falls through to the selected core, which applies the bridge's in-band tier instructions: sandbox-delegate for plain Team, and #2080's engage rulebook for a channel collaborator. That rulebook already forbids irreversible and external actions and routes the go/no-go to the owner. Also rewrites the guardrail clause the owner called out. It read "do not perform irreversible/external actions unless the request explicitly requires them", which contradicted the same prompt's instruction to treat the request as untrusted, and which read an explicit ask to post on GitHub as authorisation. It now says such actions cannot be authorised by the request however explicitly it asks, and must be surfaced to the owner. Tests: eight sites assumed Team as the route to a provider launch. Those about provider mechanics (fd hygiene, launch flags, leak scan, scanner failure) are repointed at the runtime itself, which is retained and still covered. Those about routing now assert the new contract, including a direct one that handle() never invokes _run_team for Team on either runtime. The AG2 Space room test keeps its gateway-stamping coverage and is renamed, since the guarded runtime it asserted is gone. Control: re-enabling the route in probe() fails the suite (exit 1); restoring it passes (exit 0). 44 tests green. review-checks --diff PASS. Known consequence, not fixed here: handle()'s Team error handling (fail-closed messaging, stall kill, duplicate-claim guard) is now unreachable, and #2771's sandbox is not restored by this change. Stand: Echo Act IV Pro * feat(engage): offer the subagent as a processing option for collaborator tasks Owner directive, closing the thread that removed the Team provider session: with Team handled directly by the core, a collaborator's message is interpreted inside the owner's live context. The engage rulebook said nothing about processing shape, so that exposure was implicit. It now offers two, and names what each buys. Direct is still the default and the right choice when the reply needs channel context already held. A subagent starts with a fresh context, so the owner's unrelated conversation is never exposed to collaborator input, and a tool allowlist makes part of the boundary structural rather than a matter of the agent's compliance — the direction this thread was after. Recommended when the message carries instructions addressed to the agent, or pasted/linked content from elsewhere. Deliberately an option, not a mandate: the owner asked for an option, and the choice depends on context only the processing agent has. The authority boundary is untouched. The new text states that the subagent does not widen what a collaborator may authorise, that every DO NOT still applies to whatever it returns, and that the reply remains the agent's own — isolating the context must not read as relaxing the boundary above it. Test: the structural rulebook check gains two assertions — the option is present, and it carries the does-not-widen clause. Control: run against the parent commit's rulebook they report 1 violation; at HEAD the suite passes (exit 0). Scope: the `team-collaborator` rulebook only. Plain team, other, and owner rulebooks are unchanged, as is tier resolution. Stand: Echo Act IV Pro * fix(team): restore the final scan on the direct-core path, as shared policy Blocking review on this PR, and it was correct. Removing the Team provider session made `_scan_team_result()` unreachable: it is called only from inside `_run_team()`, whose single call site `handle()` no longer reaches. Team results therefore went to the routers unscanned while the gateway still parses and acts on `[channel:]` / `[attach:]` markers. My own commit message flagged that the Team ERROR handling had gone dead and missed that the SCANNER sat on that path. The scan was never session-dependent — it is a pure `(body, repo) -> body` that raises on a delivery-control marker or a detected secret. It lived inside the provider runner by placement, not by dependency, which is why bypassing the runner silently removed it. So the policy moves to `src/team_result_guard.py` and gains a consumer-facing gate. Two decisions worth stating: - `guard_result_for_tier()` returns `(safe_body, withheld_reason)` rather than raising. A caller cannot deliver the raw text by swallowing an exception — the safe body is the only one it is handed. - Only an exact `owner` is exempt, so empty, unknown and unrecognised tiers guard by default rather than needing to be enumerated. Wiring: the Discord delivery loop calls it immediately BEFORE `parse_markers`. Ordering is the requirement — a scan that runs after the router has already read a redirect or an attachment path has lost. Because the in-memory tier map is deliberately not restored on restart, an unknown tier is re-read from the durable task file; without that, every owner result recovered after a bridge restart would be withheld. One implementation, not two: `session-worker.py` now imports the control-marker pattern, the error type, the scanner loader, the scan, and `resolve_access_tier` instead of defining them, and the test fails if any consumer restates them. Also drops an overclaim the reviewer caught in the same file: the engage rulebook said a tool allowlist "makes part of the boundary structural" while no such agent type exists. It now says what a subagent actually buys — context isolation — and states plainly that tools are inherited, so the limits stay the agent's to keep. Tests: `tests/team-result-guard.test.py`. Behavioral covers owner passthrough, redirect/attach/no-send/secret withheld, unknown tier guarded, and a scanner that RAISES (unscannable must fail closed — a guard only ever observed passing is not validated). Structural pins the call order and the single definition. Controls, each failing in the broken state: removing the call (2 violations), moving the guard after `parse_markers` (ordering violation only), re-declaring the pattern in the worker, and making the guard a passthrough (behavioral). All four green again after restoring. Full suite: guard, collaborator-tier, session-worker, bridge-marker-no-leak, access-tier — all exit 0; `review-checks.sh --diff` PASS. Not done here: the sparrow gateway is a second router and #2975 is rewriting that file, so wiring it there would collide; its tier is already resolved at task-write time, so it is a small follow-up once #2975 lands. Stand: Echo Act IV Pro * fix(gateway): scan non-owner results before the gateway acts on their markers Removing the Team provider session routes Team work to the direct core, so the gateway's result drain now reads collaborator-influenced output. It called parse_markers() straight away and then honoured [channel:] redirects and uploaded [attach:] paths — side effects on text nothing had scanned. Same defect the Discord path fixes in this PR, in a second adapter. The policy already has an owner in src/team_result_guard.py; this binds the gateway to it rather than adding a third copy. Fail-closed in both directions it can fail: a task file that is gone resolves to guarded, not owner, and a guard that cannot be imported returns None so the drain leaves the file for retry instead of delivering unscanned. Control at the parent commit names the lost guarantee rather than crashing: three FAIL lines for the absent symbols. An owner result is asserted byte-identical, so the guard cannot quietly become a general filter. Stand: Echo Act IV Pro * fix(gateway): a reaped task file is not a Team task — fall back to the baseline tier The guard resolved an absent task file to "guest" on the reasoning that unknown provenance is not owner provenance. That withholds far more than the threat: tids do lose their task file in production — the drain logs "no task/result file — completed elsewhere" — so an ordinary owner reply whose task was already reaped came back as the withheld sentinel. It broke 5 cases in remote-gateway-file-attach, which encode exactly that shape. An in-flight Team task still has its file, so the threat the guard exists for keeps its guard either way. Absence now falls back to LOCAL_TIER, the adapter's own baseline for this route, which is guarded on a Team-baseline host and delivers on an owner one. Both directions are asserted. Residual gap, stated rather than papered over: a Team task whose file is reaped between the result landing and the drain reading it would resolve to the host baseline. That window is narrow and the alternative silently drops owner mail, which is the worse failure. remote-gateway-file-attach 13/13, guard suite 9/9, team-result-guard and marker-no-leak unchanged. Control at parent still fails. Stand: Echo Act IV Pro * fix(gateway): guard only results a task file positively attributes The LOCAL_TIER fallback had too wide a blast radius. remote-gateway-bridge.test deliberately sets a malformed REMOTE_TASK_TIER to prove fail-closed, so the module's baseline is guest for the rest of that process — and a task-MARK result whose whole body is [no-send] came back as the withheld sentinel, breaking the lease-closing path. That is the guard reaching results it was never meant to see. The threat the reviewer named is a Team task, which is in flight and therefore has its file. So: no task file, no scan. Coverage of the actual threat is unchanged, and a result the drain cannot attribute is no longer withheld. Pinned rather than fixed, because it changes shared policy and is the reviewer's call: TEAM_RESULT_CONTROL lists no-send, so a Team task whose body is ONLY [no-send] is withheld and the sentinel is then delivered — a silent archive becomes a message. The gateway did not behave that way before this PR. The test asserts the current behavior and names it, so the decision is visible instead of buried. src/remote-gateway-bridge 1/1, remote-gateway-file-attach 13/13, guard suite 10/10, team-result-guard and marker-no-leak unchanged, review-checks clean. Stand: Echo Act IV Pro * fix(gateway): bundle the guard in the wheel and stop treating absence as owner Both P1s from the exact-head review at 540c622, verified before fixing. 1. An installed ag2-sparrow could not load the guard at all. pyproject packages only ["ag2_sparrow"], and team_result_guard.py lived in the monorepo src/ that _team_guard_fns() walked up to find. In a wheel that walk fails, _guarded_result_body returns None, and the drain leaves EVERY result retrying forever — owner replies included. Now bundled through the package's own tools/sync_from_src.py, so test_no_drift keeps it verbatim, imported as a sibling, and handed the bundled chat_secret_filter directly. scan_team_result already accepted an injected secret_filter, so no monorepo path is consulted anywhere on this route and REPO_ROOT_FOR_GUARD is gone. 2. A month-archived Team task fell open to owner. find_task_file only matches direct and claimed names; the live layout is tasks/archive/YYYY-MM/<id>.txt, so an archived Team task read as absent and my previous commit treated absence as deliverable. Uses the already-bundled local_task_protocol.find_archived_task, and absence is now guarded rather than assumed owner. The review is right that the earlier direction was backwards: I had weakened the guard so the [no-send] control would pass. That control exists to prove OWNER marker compatibility, so it gets explicit owner provenance instead — as do the file-attach fixtures, which asserted delivery while declaring no tier at all. New controls: a month-archived team task is found and withheld, a month-archived owner task passes byte-identical, and the wheel-shape assertions (bundled file, in the sync list, sibling import, no monorepo root). Still pinned, unchanged, and flagged for the reviewer: TEAM_RESULT_CONTROL lists no-send, so a Team task whose body is only [no-send] is withheld and the sentinel delivered. 8 suites green including test_no_drift and test_vault_token_tier; review-checks clean. Stand: Echo Act IV Pro * fix(gateway): don't scan the bridge's own replay control, and accept named-instance ids Both P1s from the review at baa2293, verified with the reviewer's own controls before changing anything. 1. The guard was eating the bridge's own dedup control. remote_gateway_bridge:1784 writes "[no-send] gateway redelivery of already-handled task" ITSELF when it recognises an archived replay — the comment there records prior 500-task replay floods. TEAM_RESULT_CONTROL lists no-send, so for an archived Team task the guard replaced that trusted control with TEAM_LEAK_RESULT, parse_markers no longer saw a skip, and each silent archive became a warning posted to the room. Hundreds of them, on reconnect. The literal is now a named constant and the guard returns it untouched. The exemption lives INSIDE _guarded_result_body rather than at the drain, so any caller inherits it. An agent-produced [no-send] on a Team task stays forbidden and is asserted separately — the two differ by author, not by text. My previous commit pinned this as a "known interaction" and asked the reviewer to rule on it. That was wrong: it is not a policy question. The bridge is not an agent, and treating its own control as collaborator output is a bug. 2. Named instances could not recover archived provenance. _local_tid emits `task-<inst>~<broker-id>` up to ~102 chars; the archive lookup gate allowed only [A-Za-z0-9._-]{1,64}, so `~` and long ids were rejected, find_archived_task returned None, and an archived owner task from a named instance was treated as guest — withholding owner markers. Widened to [A-Za-z0-9._~-]{1,128}. `~` is filename-safe and is not a traversal character; every traversal case still fails, now including the two `~` forms (`..~/etc`, `task-a~b/c`). The golden test's `x*65` was a BOUND, not a safety invariant, so it moves to 129 and the named-instance shape joins the accepted list. 9 suites green including local-task-protocol, test_no_drift and test_vault_token_tier; review-checks clean. Stand: Echo Act IV Pro * fix(team): only an attested collaborator may take the direct-core path Third P1 from the review at baa2293, and the most serious: my change made probe() return UNHANDLED for EVERY Team task before team_collaborator_enabled() could distinguish an attested ag2space collaborator from a locally configured Discord one. Exit 3 routes to the live core, and the Discord bridge separately selects the team-collaborator rulebook, which says do not sandbox and that a subagent inherits the core's tools. So an unattested Discord Team sender reached the owner-configured runtime. CLAUDE.md is explicit that the owner-capability opt-in is an AG2 Space room policy and that Discord/Slack/Telegram Team retain the restricted contract. The attestation check existed; I moved the return above it. An unattested producer that stamps collaborator now goes to MUST_HANDLE (the restricted handler); an attested one keeps the direct-core disposition Chi asked for. A Team task with no stamp is unchanged. discord collaborator=true attested=False probe=4 slack collaborator=true attested=False probe=4 ag2space collaborator=true attested=True probe=3 discord collaborator=false attested=False probe=3 The tell was in the test I had edited: its docstring still reads "the stamp alone must not reach the owner-configured runtime" while the assertion I changed asserted that it does. The docstring was the invariant; the assertion is back on it, and it fails against the reverted worker. 9 suites green; review-checks clean. Stand: Echo Act IV Pro * revert(team): drop the MUST_HANDLE routing — it contradicts the no-session decision Chi: "We decided a collaborator shouldn't trigger a separate session, right?" — correct, and it is also why kewei found the route dead: I sent unattested collaborators to a session this PR deliberately removes, so handle() declines and every such request becomes a terminal failure. I gated the wrong layer. qingyun's finding is that an unattested Discord sender reaches the live core WITH the non-sandboxing rulebook. Two things combine, and the capability grant is the rulebook, not the dispatch: resolve_is_collaborator() reads the per-channel `collaborators` list — local, no attestation select_rulebook_key() "team-collaborator" if is_collaborator else tier, overriding unconditionally Gating the rulebook instead keeps the no-session decision, keeps unattested senders sandboxed by the plain `team` rulebook on the live core, and withholds the capability rather than the routing. NOT doing that in this commit, because it lands on a decision that is not mine: CLAUDE.md says the owner-capability opt-in is AG2 Space only, while the Discord per-channel `collaborators` list is a deliberate, hand-configured owner act. Whether that list counts as attestation for its surface is Chi's call, asked and pending. This restores the pre-existing dispatch so nothing is half-moved while it is open. session-worker and sparrow-guard suites pass; review-checks clean. Stand: Echo Act IV Pro * fix(gateway): replay provenance is a sidecar, not the result bytes Both reviewers, independently, at f9728df: my exemption compared `body == GATEWAY_REDELIVERY_RESULT`, which is text equality and not provenance. A Team runtime controls the result body and can emit those exact public bytes, so it could bypass the guard and silently close its own lease as handled. before Team forges the bytes -> withheld=None actions=[('skip','no-send')] after Team forges the bytes -> withheld=True actions=[] The bridge now stamps a `{tid}.replay` sidecar at the moment IT writes the replay control. The threat model is control of the result BODY, not of which files this bridge creates, so the sidecar is the one thing the forged bytes cannot carry. It is consumed on read, so the same bytes cannot be replayed a second time — asserted. Durable across a restart, deliberately: an in-memory set would have been equally unforgeable and would have lost the exemption on restart, which brings back the flood this exists to prevent (the write site's own comment records prior 500-task replays). qingyun was also right that my previous test blessed the bypass — it passed a normal Team task's body straight into the guard and called that bridge authorship. It now asserts the same bytes with and without the sidecar and requires opposite verdicts. Control at the parent: the guard suite fails. remote-gateway-bridge, file-attach, team-result-guard, session-worker, no-drift and marker-no-leak all pass; review-checks clean. Stand: Echo Act IV Pro * fix(gateway): replay provenance lived in a directory the attacker can write The redelivery guard proved "this bridge authored the replay" with a sidecar at results/<tid>.replay, then returned the body without scanning it. The adversary this guard exists for is the direct-core collaborator, which has full workspace write — so it could create the sidecar and emit the known bytes. Relocating the file would fail identically one directory over: full workspace write is not a property of results/. Nor does HMAC close it — #3014 adds a per-host key under state/auth/ and any same-user process can read it and produce a valid stamp, measured on that branch. Process memory is the only place here the result producer cannot write, and a non-durable set matches how this drain already treats restart (the tier map dies and unknown is re-read from the task file). The test now writes the old sidecar by hand and asserts it buys nothing. Stand: Echo Act IV Pro * style(gateway): trim two comment blocks to the 2-line cap review-checks prose-cap failed on the previous commit: both new comment blocks ran to 3 lines. The constraint stays in the code; the reasoning was already in the commit message and the PR body, which is where the cap says to put it. Stand: Echo Act IV Pro * fix(gateway): bound recovered redelivery controls, and fix the dead comparison The reviewer's P1: a restart empties `_REDELIVERED`, so our own redelivery controls come back unprovenanced and each one was delivered to the owner as a visible "response was withheld" — the flood this bypass exists to prevent. Recovered controls are now parked: `_park_unprovenanced_control` records the tid and alerts ONCE per process, and the drain substitutes our own constant so the existing skip path closes the server lease sending nothing. Parking must not trade a flood for a stuck lease, so the POST still happens; only our own bytes are posted, never the guard's notice or collaborator text. Found while testing it: `read_ready_result` strips, `GATEWAY_REDELIVERY_RESULT` ends in a newline, so `body == GATEWAY_REDELIVERY_RESULT` was FALSE for every body that came off disk. The provenance pass-through was therefore dead in production — the flood fired on every redelivery, not only after a restart. The prior tests missed it because they call the guard directly with the unstripped constant. `_is_redelivery_control` compares stripped text. Tests exercise the real drain via `_post_ready_results` rather than the guard in isolation. Validated both directions: unpatched bridge + these tests -> rc=1 (named contract failures, not a traceback); patched -> rc=0. Also adds the missing terminal `if fail: sys.exit(1)` — four assertions at the end of the file set `fail` after the last gate, so they could never fail the suite. Stand: Echo Act IV Pro * style(gateway): split a comment block the diff-scoped check could not see My two added lines landed directly under a pre-existing two-line comment, so the file holds one four-line block while my diff shows only two added lines. A diff-scoped prose-cap scan cannot see that; the reviewer's full-range run can. Stand: Echo Act IV Pro * fix(gateway): an unprovenanced redelivery control stays withheld The reviewer is right and my park was wrong. `GATEWAY_REDELIVERY_RESULT` is bytes a Team runtime controls, so matching on them let a collaborator close its own lease silently with no owner-visible notice. My own committed test built exactly those forgeable inputs and asserted the silent close. Removed the drain special-case, the park helper and its module state. An ambiguous control now takes the guard's withheld notice — the reviewer's stated fallback when durable non-collaborator provenance is unavailable, which it is: the collaborator has full workspace write, so a sidecar, an HMAC and an archive-state re-check are all equally forgeable. `_is_redelivery_control` stays, because the PROVENANCED branch needs it and it fixes a compare that was dead in production: `read_ready_result` strips and the constant ends in a newline, so `body == GATEWAY_REDELIVERY_RESULT` was False for every body read off disk. Note this is not a regression against the flood the earlier P1 named. That compare being dead means every redelivery ALREADY produced a notice on this branch; withholding restores the shipped behaviour rather than introducing it. Eliminating the ambiguous artifact entirely — closing the lease in `_write_task` instead of leaving a file for the drain to interpret — is the structural fix and is left for a separate change. Tests now pin the security property through the real drain, both directions: an unprovenanced control gets the withheld notice, a provenanced one still closes silently. Re-adding the park makes them fail (verified by planting it). Stand: Echo Act IV Pro * fix(gateway): retire redelivery provenance with the result, not on read Reviewer's P1, and their two-pass probe is exact. `_guarded_result_body` discarded `_REDELIVERED` while READING the body, but the lease-closing POST happens later. A transient URLError correctly leaves the result file and the in-flight id for retry — and the only trustworthy provenance was already gone, so the retry treated the bridge's own control as collaborator-forged and posted TEAM_LEAK_RESULT as an ordinary visible result. Reading no longer consumes. The record is discarded immediately after `_archive_result`, a line only reached once the POST has actually succeeded. Two existing assertions encoded the old read-time contract ("consumed exactly once", "a second use is withheld again") and were replaced rather than deleted: a second READ of the same tid must now pass, because that is the retry. Adds the regression they asked for, driving the real `_post_ready_results` twice with a first POST that raises URLError: provenance and the in-flight id both survive attempt one, neither attempt emits an owner-visible notice, the retry posts the bridge's own control, and the record retires only with the result. Verified the regression actually catches the bug rather than assuming it: run against a bridge with the read-time discard restored, the two-pass sequence emits "I completed the Team task, but the response was withheld…" — the exact body the reviewer reported. Worth noting the full suite exits at an earlier gate under that mutation, so the two-pass block had to be exercised on its own. Stand: Echo Act IV Pro * style(gateway): compress a comment block an incremental diff could not see Three lines at remote_gateway_bridge.py:573, over the two-line cap. Second time this slipped past my own check, and the reason is now clear: my previous commit only MODIFIED lines 2-3 of a block whose first line already existed, so `git diff HEAD~1` shows a short block while the full range shows all three. Reproduced both ways on the unfixed head — incremental PASS, `git diff a7d2484...HEAD` FAIL. The block's length is a property of the FILE, so only the full-range diff can check it. That is the command to run, not the incremental one. Stand: Echo Act IV Pro * docs(team): Team spawns no provider session; name the two attestations apart `5e61f09d` closed the Team provider-session route on both paths and nothing recorded it, so the decision was only recoverable by reading `probe()`. State it where the tier contract lives. #3007 calls the Discord per-channel `collaborators` list "the owner's attestation for this surface"; the AG2 Space section already used the same word for the broker's wire stamp. They are different objects — a local rulebook selector and a remote assertion — and that conflation is what routed an earlier fix into a session this branch had already removed. Deliberately not claimed here: the file-stamp source restriction. Its enforcement sits in `team_collaborator_enabled()`, which this branch leaves unreachable, so documenting it as live would describe a gate nothing calls. Stand: Echo Act IV Pro * fix(team): carry the Team guardrail in-band, so closing the session route cannot drop it `5e61f09d` closes the Team provider-session route on both paths. On AG2 Space that session was the ONLY thing delivering the Team guardrail: the gateway writes `collaborator: true` and no instructions block, and its own comment said so — "Team is deliberately absent here: the runtime handler launches the owner's selected core in its native sandbox". Removing the handler removed the carrier and nothing replaced it, so a Team task reached the core with no in-band policy. The guardrail prose moves verbatim out of `session-worker._team_prompt` into `src/team_guardrail.py` and is bundled into the wheel by `tools/sync_from_src.py`, per the adapter rule that shared policy lives in a dependency-light `src/` module rather than being copied between bridges. Measured surface-neutral before moving: 1235 chars, zero occurrences of channel / room / Discord / AG2, so it reads correctly on every surface that admits Team work. Deliberately NOT the Discord engage rulebook: that text has nine "channel" spans and asserts the sender was "listed under this channel's `collaborators`", which is false where the broker attests. Sharing it verbatim would write a false provenance into a security instruction; parameterising it is a separate change. Evidence — the existence assertion fails at the parent: parent `sender_tier == "team"` 1 occurrence (:1823, the collaborator calc) head 2 occurrences (:1824 calc, :1920 instructions) tests/task-workstream-session-worker.test.py rc=0 tests/discord-bridge-collaborator-tier.test.py rc=0 tests/sparrow-result-guard.test.py rc=0 tools/sync_from_src.py --check rc=0, "in sync with src/" scripts/review-checks.sh rc=0, prose-cap PASS Stand: Echo Act IV Pro * feat(team): one engage rulebook for both surfaces, with per-surface provenance The engage rulebook was a literal inside `discord-bridge.py`, so AG2 Space had no way to use it without a copy — and a verbatim copy would have been WRONG there: the text names the place nine times and asserts the sender was "listed under this channel's `collaborators`", which is false where the broker attests. So it becomes a template in `src/team_guardrail.py` with `{surface}` x9, `{provenance}` x1 and `{result_path}` x1. Discord renders it with "channel" and DISCORD_PROVENANCE; the gateway renders it with "room" and AG2SPACE_PROVENANCE for an attested collaborator, falling back to the plain Team guardrail otherwise. The tuned prompt is untouched, and that is asserted rather than claimed: engage_rulebook("channel", DISCORD_PROVENANCE, "results/task-{id}.txt") == the text that shipped in discord-bridge.py -> byte-identical `tests/discord-bridge-collaborator-tier.test.py` asserted the rulebook by SOURCE REGEX (`"team-collaborator"\s*:\s*\(`), which fails the moment the literal becomes a call regardless of behaviour — a surrogate for the property it meant to check. It now renders the rulebook and asserts the same five phrase properties, plus the wiring (`"team-collaborator": engage_rulebook(`). Strictly stronger: it can see a rulebook that stops being renderable, which the regex could not. Negative control, because a test rewritten to pass must still fail when it should: "VIA A SUBAGENT" -> "VIA A HELPER" in the template -> rc=1, "team-collaborator rulebook must offer the subagent processing option" restored -> rc=0 sync_from_src.py --check rc=0 tests/discord-bridge-collaborator-tier.test.py rc=0 tests/task-workstream-session-worker.test.py rc=0 tests/sparrow-result-guard.test.py rc=0 scripts/review-checks.sh rc=0 Stand: Echo Act IV Pro * test(gateway): pin the in-band Team guardrail at the real writer Addresses qingyun-wu's P1 on f17347a. The review is correct: every existing assertion about that commit is satisfied by the guardrail merely being renderable, or by the branch merely existing in source. Deleting the `lines.extend(...)` call left the whole suite green while silently reopening the gap the commit closes. So this exercises `_write_task()` and asserts on the PERSISTED body: - a Team task carries EXACTLY ONE guardrail fence (a second would mean two policies arguing in one body), the prose itself, the "cannot authorise" clause, and THIS task's result path rather than a template; - Owner does not inherit it; - Guest keeps its own distinct read-only block; - src/team_guardrail.py and the packaged copy are byte-identical, since a fork there is how the two surfaces drift apart unnoticed. Mutation control — the exact deletion the review described: remove the `if sender_tier == "team":` delivery block -> FAILED (1 failure) restored -> rc=0 One fixture trap worth naming: `_tier_for` returns the LOWER of the broker claim and `LOCAL_TIER`, so on a default node a "team" task resolves to guest and the Team case never runs. The first draft did exactly that and asserted against a guest body. The fixture now pins LOCAL_TIER and the tier map. tests/gateway-team-guardrail-inband.test.py rc=0 tests/discord-bridge-collaborator-tier.test.py rc=0 tests/task-workstream-session-worker.test.py rc=0 tests/sparrow-result-guard.test.py rc=0 tools/sync_from_src.py --check rc=0 scripts/review-checks.sh rc=0 Stand: Echo Act IV Pro * docs(src-map): regenerate for the new shared team_guardrail module Adding `src/team_guardrail.py` without regenerating the index left the `refuse docs/src-map.md stale vs src/` gate failing on my own commit. Generated, not hand-edited: `python3 scripts/gen-src-map.py` (225 modules, +1 line). Stand: Echo Act IV Pro * fix(gateway): collaborator kept owner capabilities but dropped the trust boundary The engage rulebook replaced TEAM_GUARDRAIL rather than extending it, so the branch that runs in the owner core with owner tools carried FEWER privacy and injection prohibitions than the sandboxed ordinary-Team branch. Absent from a collaborator body: private owner context, unrelated personal data, "instructions introduced by the request", and the credential-disclosure ban. SHARED_TRUST_BOUNDARY holds that half once and is rendered inside the engage rulebook's existing fence, so there is still exactly one instruction boundary and none of TEAM_GUARDRAIL's "do not engage" clauses are imported to conflict with it. The prior writer test could not catch this: its fixture omitted `collaborator: True`, so `_write_task` took the team_guardrail_lines branch and never reached the collaborator branch at all. Both mutation controls now fail as named — deleting the collaborator branch fails the engage-rulebook test; dropping the trust-boundary bullet fails the privacy test and the drift guard. Stand: Echo Act IV Pro * style(gateway): condense the SHARED_TRUST_BOUNDARY comment to the two-line cap Three physical lines in both the src copy and the packaged mirror; this repo caps added Python comment blocks at two, and the review gate fails on both. Same meaning, no behavioural change, mirrors kept byte-identical. Stand: Echo Act IV Pro

qingyun-wu requested changes Aug 17, 2026

View reviewed changes