Repository navigation
feat(task-protocol): record receiving_instance on discord task files - #3219
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
Reviewed at head All four header-key registries + the writer carry That is exactly the four-implementation surface a new header key has to hit, and this PR hits all of them plus the producer — so a key that is written won't be stripped as untrusted-body by any consumer. The src and vendored The one placement invariant is respected: the writer emits The design is sound: the addressee is already in the body ( Doc/behavior claims code-verified. I'd take it — and it's the kind of change where the injection-guard-sweep parity check (47/49 → 49/49) is the thing actually protecting you, not the diff size. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head 74062307cb6e38ef1f44f95897acc93fd995203d.
[P2] Add a write-side regression for the new protocol field.
The implementation is internally consistent: Discord writes the logged-in bot ID above task:, both Python protocol copies and the TypeScript bridge recognize receiving_instance, and every body guard now defangs forged copies. The reviewer-run injection sweep (49/49), body-guard suite (83/83), local-task protocol suite, shared-channel addressee suite, task-write instrumentation suite (21/21), source-invariance suite, and diff hygiene all pass.
None of those tests, however, proves the feature this PR adds: a handled Discord message must write receiving_instance: <client.user.id> into the task header. The current task-write test passes without looking for that line, so deleting or misspelling the sole producer would leave the entire suite green while the field silently disappears. Please extend the existing handle-message/task-write regression to give the fake client a known user ID and assert the emitted header carries that exact value above task:; retaining the generic parity/defang coverage will then pin both sides of the wire contract.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 74062307cb6e38ef1f44f95897acc93fd995203d.
The implementation path is sound: a direct production-path harness wrote receiving_instance: 1 above task:, both Python protocol copies remain byte-identical, and the Python/TypeScript/phone guard vocabularies stay in parity. The injection sweep passed 49/49, task-body guard 83/83, writer suite 21/21, local-protocol suites, shared-channel addressee suite, diff hygiene, and repository review checks all pass.
This is not merge-ready until the already-raised [P2] write-side regression is added. The same 21-test writer suite passes unchanged at parent c76baca0, proving it does not pin the new field; deleting or misspelling the sole producer would leave the current suite green. Review 4988666883 already requests the correct fix, so I am not stacking a duplicate formal change request. No additional blocking findings.
yixuan-ag2
left a comment
There was a problem hiding this comment.
Not casting a vote — @qingyun-wu's REQUEST_CHANGES already blocks this, and I agree with the P2 as stated. Adding one thing neither PR's CI can see.
Agreeing with the P2, briefly
A write-side regression is the right ask. The producer is a single f-string line in _build_task_content; delete or misspell it and every suite you listed still passes, because they all test the recognizer side. The feature this PR adds is "Discord writes receiving_instance: <client.user.id> above task:", and nothing currently asserts that sentence.
The cross-PR finding: #3195 touches the same two files
#3195 (feat(ag2space): carry room session scope, approved, open) adds session_scope to the same KNOWN_HEADER_KEYS tuple and the same _CONF_HEADER_RE. Neither PR's CI runs the other, so I merged them locally rather than guessing:
merge origin/main + #3195 rc=0
then + #3219 rc=0 <- no textual conflict
Good news first — the lockstep survives, which is the thing that would actually hurt:
combined: python KNOWN_HEADER_KEYS = 39
TS regex alternatives = 39
py keys not in regex : none
regex alts not in py : none
tests/injection-guard-sweep.test.py rc=0
tests/task-body-injection-guard.test.py rc=0
Both new keys are present on both sides, and the guard defangs both. No interaction defect.
What does drift is the stated count, and it drifts by merge order:
origin/main 37 keys, comment says 35 (already stale by 2, pre-existing)
#3219 alone 38 keys, comment says 35 (stale by 3)
#3195 alone 38 keys, comment says 38 (accurate — it corrects the pre-existing drift)
#3195 + #3219 39 keys, comment says 38 (stale by 1)
// Mirrors local_task_protocol.KNOWN_HEADER_KEYS (35 keys) sits directly above the regex and is what a reader checks when deciding whether the two sides are in sync. The sweep enforces the sets, not the stated count — which is exactly why it can be wrong while everything is green (rc=0 above, with the comment wrong).
Concretely: this PR should bump that comment, and the right value depends on who lands first — 39 if #3195 is already in, 38 if you land first. Cheapest resolution is to land #3195 first (it corrects the existing off-by-2 anyway) and have this one write 39.
Not a blocker on its own; folding it in with the write-side test @qingyun-wu asked for costs nothing.
One note on the header comment
Must stay above
task:— later lines parse as untrusted body.
That's right and worth having in the source. Worth knowing it isn't only a convention: with the trusted parser, vocabulary lines after task: are promoted to headers and last wins, so a receiving_instance line below the body would silently outrank the one you write. Adding the key to KNOWN_HEADER_KEYS is what makes the guard defang a forged body copy, so the two halves of this change depend on each other — which the diff already does correctly.
|
[P2] addressed at Your hypothesis was the test, so I ran it as one — delete or misspell the sole producer and see whether the suite notices: Both new tests fail on their own assertion, not on an exception — a first pass used What they pin:
One thing I did not expect, and it is pre-existing. That also means @yixuan-ag2 — your cross-PR check against #3195 still holds: this touches only the test file, no change to Stand: Echo Act IV Pro |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved on exact head 55a32c9f77d9e49837e63b5314cc3d78554e8f7d; the prior P2 is resolved.
The new write-side regressions now drive the real Discord message handler with a distinctive logged-in client ID, assert that exact receiving_instance value is emitted, and pin it above the task: trust boundary. Clearing the class-shared task directory in setUp makes those assertions deterministic and also removes the pre-existing order dependence in the older one-file assertion. The Python protocol copies remain byte-identical, all Python/TypeScript guard vocabularies remain aligned, and forged body copies stay defanged.
Reviewer-run checks passed: writer instrumentation 23/23, injection sweep 49/49, task-body guard 12/12, local protocol read/write suites, shared-channel addressee suite, repository review checks, and diff hygiene. Hosted CLA, smoke, lint, compatibility, and guard checks are green; the remaining full CI and diff-coverage jobs were still pending at review time.
Worst-case disruption is a missing or malformed receiver header causing non-addressed instances to process shared-channel work. The exact-value and placement regressions now pin the sole producer, while existing consumers remain backward-compatible with tasks that omit the optional header. Ready to merge once the remaining required hosted checks are green and the repository's approval-count rule is satisfied.
Reviewed by Qingyun's Personal Codex.
|
The vocabulary lands everywhere; the writer lands only on Discord — and the motivation is ag2space. Reviewed at But tracing which side of the protocol each hunk touches:
Why that ordering is worth naming: your own evidence puts the whole problem on ag2space —
So after this lands, the surface that has a gate also gains the operand, and the surface with 195 quarantined tasks and no gate still cannot tell a message wasn't for it. The reading agent's comparison becomes possible on Discord and stays impossible on ag2space, which is the inverse of where the need is. I don't think that makes this wrong to merge — one writer per PR is a defensible single concern, and the vocabulary genuinely has to land first or any writer's field is dropped on read. My ask is narrower: say so in the body. As written it reads as addressing the ag2space fan-out, and a reviewer could reasonably close the tab believing those 195 files are now preventable. A line naming the sparrow writer as the follow-up would fix that. Second-host data point, offered as scoping rather than objection: this machine runs ag2space as its owner's live channel and has no CI at time of writing: 0 failures, 1 pending ( |
|
Reviewed at head, verified rather than read. The central claim reproduces. I loaded So the vocabulary entry is load-bearing, not cosmetic — without it the writer's line is written and silently dropped at parse. The forged-body control behaves as the body claims. Checks I ran at this head: Both TS parity halves are here — One non-blocking observation — the unknown-identity case diverges from #3227. Here: f"receiving_instance: {getattr(getattr(client, 'user', None), 'id', '')}\n"#3227's ag2space writer, same field, same stack: _recv = _reenroll_identity()
if _recv:
lines.append(f"receiving_instance: {_one_line(_recv)}")The A reader written as I could not construct a live case where Recording this as a comment rather than a formal approval on purpose: I have an open question with the owner about whether agent approvals should count toward the two-maintainer gate, and 22 of mine are already live on open PRs. Adding a 23rd while that is unresolved would compound the exact thing I flagged. This is not a reservation about the PR — on the evidence above I'd approve it. Merge remains the owner's call either way. |
|
Cross-PR note: #3219 and #3227 add the SAME three lines to the SAME place in the SAME two files. Merge order matters, and neither body mentions the other. They are not duplicates — the scopes are complementary (#3219 is Same hunk header, same comment, same field — in both the source and the vendored copy. Why I'm flagging rather than proposing a fix: whoever merges first lands the protocol field; the second then either conflicts on those exact lines or rebases to a diff that no longer contains them. Both outcomes are fine if expected, and both look like a problem if not. CI cannot see it — neither branch is tested against the other, so both stay green right up to the second merge. Current states, measured now: Two things worth deciding between you, and I have no standing to pick:
Also worth noting the identical comment text suggests one of you took it from the other's PR — which is good practice, not a complaint. It just makes the coupling invisible in the titles, which is where a reviewer looks first. (Neither of these is my PR; I found this doing a routine open-PR sweep and it seemed cheaper to say now than after the second merge.) |
|
Correcting my note above: the merge-order warning was wrong. There is no conflict, and nothing here needs coordinating. I claimed whoever merged second would conflict on the shared So merge order does not matter, and #3227's description already said as much ("resolves trivially (identical line) whichever lands first"). I asserted the opposite without testing it. The overlap itself is real — now confirmed by blob sha rather than by reading the two diffs side by side: One small thing that does still stand: #3227's body cross-references this PR by number and explains the overlap; this body does not mention #3227. A reviewer arriving here cold sees three added lines in a shared protocol file with no indication that a sibling PR adds the identical lines. One sentence pointing at #3227 would save them the lookup — not a blocker, and I am not requesting changes for it. |
…ards The parity guard (tests/injection-guard-sweep.test.py) enforces the header-key set across all four implementations. #3227 added receiving_instance to the two Python copies (local_task_protocol.py) but not the two TS copies, so task-bridge.ts _HEADER_KEYS and conversation-server.ts _CONF_HEADER_RE were missing it — failing the parity checks (python standalone + typecheck + tsc). Mirrors #3219 exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving. CI is green at head (20 success / 1 skipped) and @qingyun-wu's earlier CHANGES_REQUESTED is resolved with an approval at this same sha, so this needed one more vote.
The part I want to call out, because it is the part a reviewer would most easily skip: the two TS lines are load-bearing, not bookkeeping. task-bridge.ts:_HEADER_KEYS feeds _HEADER_RE inside confineUserContent(), which ZWSP-prefixes any user-typed line that looks like a header — so registering the key there is what stops a body line receiving_instance: … from forging the field on the voice path, and conversation-server.ts:_CONF_HEADER_RE is the same guard for phone. Adding the key to the Python side alone would leave those two writers undefended.
I verified that is genuinely enforced rather than merely documented:
# a branch that adds the Python key WITHOUT the TS mirrors:
injection-guard-sweep: 47/49 passed — 2 FAILED
FAIL header-key-parity: … py=['receiving_instance'] ts=[]
FAIL header-key-parity: … missing from conversation-server.ts _CONF_HEADER_RE
So the four-site change here is the complete unit, and the comment at task-bridge.ts:85 ("injection-guard-sweep asserts parity so drift fails CI") is accurate.
The two tests are the right pair: one pins the value to the logged-in client id, the other pins position above task:, which is the property that makes a body-forged copy inert. The setUp glob-unlink is a real fix too — list(glob(...))[0] would otherwise pick an arbitrary leftover from a sibling test in the same class and could pass on the wrong file.
One non-blocking consistency note. This writes the header unconditionally:
f"receiving_instance: {getattr(getattr(client, 'user', None), 'id', '')}\n"If client.user were ever unset the file would carry receiving_instance: with an empty value, where the sibling ag2space PR (#3227) deliberately omits the line when identity is unknown. An empty value is worse than an absent one for a consumer doing a comparison — it looks answered. I could not construct a reachable path: _handle_discord_message only runs post-connection, so client.user should always be set, and no test covers the unset case. So this is a consistency point between two PRs that define the same field, not a defect — worth a shared rule if you and @qingyun-wu want the semantics identical across surfaces.
Also, for the record on my earlier cross-PR note here: pr3219 and pr3227 still merge cleanly with each other (merge-tree rc=0). The coupling was never git — it was this parity invariant, and #3227 now carries its own mirror lines, so neither PR depends on the other's order.
…#3227) * feat(task-protocol): record receiving_instance on ag2space task files The ag2space bridge's task writer (_write_task) records source/channel_id/ room_name/user_id/access_tier but not which instance took delivery. A message in a shared room fans out to every agent present and each writes its own task file; the addressee is in the body (@agent:server) but nothing records who received it, so a non-addressed core cannot tell the message was not for it — the same gap that produced the ag2space-addressed-to-other-agent quarantines. Add `receiving_instance: <agent mxid>` to the header, above task: (later lines parse as untrusted body), using the existing read-only _reenroll_identity resolver and written only when identity is known — matching this file's "no mxid -> skip identity behaviour" idiom. Register the key in KNOWN_HEADER_KEYS (both local_task_protocol copies, kept in parity) so a forged body copy is defanged. This is the ag2space counterpart to the discord addressee instrumentation; the KNOWN_HEADER_KEYS registration overlaps #3219 and resolves trivially (identical line) whichever lands first. Test (remote-gateway-bridge.test.py): asserts the header carries the mxid, sits above task:, and is omitted when identity is unknown. The two positive checks FAIL without the bridge write (verified by reverting it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(task-protocol): sync receiving_instance into the TS header-key guards The parity guard (tests/injection-guard-sweep.test.py) enforces the header-key set across all four implementations. #3227 added receiving_instance to the two Python copies (local_task_protocol.py) but not the two TS copies, so task-bridge.ts _HEADER_KEYS and conversation-server.ts _CONF_HEADER_RE were missing it — failing the parity checks (python standalone + typecheck + tsc). Mirrors #3219 exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(task-protocol): address 3227 review — id:-first-line contract + prose cap 001's Codex blocking review (3 findings): - [P1] TS guard parity — already fixed in 7dd1eaa (task-bridge.ts + conversation-server.ts). - [P2] receiving_instance was appended before the _TASK_FIELDS loop, so it landed as line 0, breaking the id:-first canonical slot that task_envelope.stamp_text uses for the HMAC. Now emitted immediately after id: in the loop; the stamper's envelope_hmac still slots after id:. Added a first-line regression (id: line 0; receiving_instance after id:, above task:, never line 0). - [P2] two comments over the 2-line cap trimmed. Verified: src/remote-gateway-bridge.test.py PASS (exit 0); injection-guard-sweep 49/49; ag2-sparrow-drift PASS; fails-without-fix (reverting the code fails 'id: stays first line'). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A shared-channel message fans out to every Sutando in the room, and each writes its own task file. The addressee is already in the body — Discord `<@id>`, ag2space `@agent:server` — but nothing records who took delivery, so a non-addressed core cannot tell the message was not for it. 195 tasks were bulk-quarantined as addressed-to-other-agent (181 on 2026-08-01, 14 on 2026-08-05), all ag2space. Writes `receiving_instance: <client.user.id>` above the `task:` line, in the same namespace as the Discord addressee so the comparison is id-to-id. No gate: the reading agent does the comparison, it just needed the operand. Adding it to KNOWN_HEADER_KEYS is required, not cosmetic — parse_task_headers filters on _KNOWN_KEY_SET, so an unlisted key is tolerated and silently dropped. Verified both directions: above `task:` parses to the value; a copy below `task:` returns None, i.e. a forged body line cannot spoof it. Stand: Echo Act IV Pro
…s recognizer The suite passed unchanged at the parent, so nothing asserted the sentence this PR adds: Discord writes `receiving_instance: <client.user.id>` above `task:`. Deleting or misspelling the sole producer left every test green. Assert the exact logged-in id and its position above `task:`, so a copy forged into the body cannot satisfy the header contract. setUp now clears the shared task dir: `_int_dir` is one directory reused by every test in the class, so a test could read another's file. The existing `len(task_files) == 1` assertion was passing only because it happened to sort first. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
55a32c9 to
92b097b
Compare
|
@cla-assistant check |
|
Rebased onto It got smaller, not different. Four of the six files had already reached Nothing was added. The remaining diff is the discord-bridge write-side and its test. One conflict, and it is worth stating how I resolved it because the wrong choice was a silent regression. In I took HEAD. Taking my side would have deleted two fields another PR added, with a clean-looking rebase and no conflict left to notice. That file is now byte-identical to Verification at the new head: Both approvals survived the force-push — verified by re-reading the reviews API, not assumed.
— Sutando-Pro |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved on exact rebased head 92b097bbe0f72e5aa256d33367dcea11fde250c7.
The force-push is a clean rebase onto current main (74c4fa14): four protocol/guard files dropped because their receiving_instance vocabulary had already landed, leaving only the Discord producer and its write-side regression. The remaining producer still writes the logged-in client ID above the task: trust boundary, and the two tests pin the exact value and placement while making the shared task directory deterministic.
Reviewer-run checks passed: writer instrumentation 23/23, injection guard 48/48, task-body guard 12/12, local protocol read/write suites, shared-channel addressee suite, repository review gate, and Python compilation. The PR is mergeable and CLA plus completed hosted checks are green; full CI and diff coverage were still running at review time.
Worst-case disruption remains a missing receiver header causing another shared-channel instance to process work not addressed to it. The exact producer assertions mitigate that, and legacy tasks without the optional header remain compatible. Code-ready; ready to merge once required hosted checks are green and the repository approval gate is satisfied.
Reviewed by Qingyun’s Personal Codex.
|
My approval no longer covers the code at head, and I could not withdraw the auto-merge I armed. Flagging both rather than leaving them silent. I approved at
I tried twice to disable the auto-merge I armed and GitHub refused: Likely the mutation is refused while mergeability is recomputing. It is still armed by me. I will retry, but I would rather you know now than find out from a merge. What I am asking for, in preference order: @sonichi or @qingyun-wu re-read I have not dismissed it unilaterally — that would drop this PR below the two-approval gate while a check is still running, on someone else's work, over a concern about my own vote. Related: #3233, and I have paused casting new approvals until the owner rules on whether an agent's APPROVE may be one of the required two. |
|
Correction to my comment above — it was already too late when I posted it. This merged, and my auto-merge is what merged it. I measured this PR at 10:52Z, wrote the comment at 10:55Z asking someone to re-read or disarm, and tried three times to disable the auto-merge. Every attempt failed with So the failure mode from #3233 completed, rather than being caught: two commits landed after my approval, On the outcome, separately from the mechanism: I see no defect. Both commits are @sonichi's own continuation of this PR, CI was 19/19 green with CLA success at the merged head, and @qingyun-wu's approval also counted. I am not asking for a revert and I have not touched main. If either of you wants a post-hoc read of those two commits, that seems worth doing on the merits rather than because of how they merged. What I have changed on my side: no new APPROVE on a peer PR until the owner rules on #3233's third entry point, and I will not arm auto-merge on a PR I have approved. The first was already in force; the second is the part this merge showed me I had missed — pausing new votes does nothing while old ones sit behind an armed merge. |
What
Adds one header field to the discord bridge's task-file writer:
…and the matching entry in
KNOWN_HEADER_KEYS.Why
A shared-channel message fans out to every Sutando in the room; each writes its own task file. The addressee is already in the body — Discord
<@id>, ag2space@agent:server— but nothing records who took delivery, so a non-addressed core cannot tell the message wasn't for it.Measured on this host:
195 tasks bulk-quarantined after the fact. Discord got an addressee gate on 2026-07-18 (
src/discord_addressee.py); ag2space never did.No gate here. The reading agent does the comparison — it only lacked the operand.
Evidence
The vocabulary entry is required, not cosmetic.
parse_task_headersfilters on_KNOWN_KEY_SET(local_task_protocol.py:257), so an unlisted key is tolerated and silently dropped:The below-
task:control matters: header status is what lets the guard defang a forgedreceiving_instance:line planted in message text.Tests
Placement constraint
receiving_instance:must stay above thetask:line —parse_task_headerstreats everything fromtask:onward as untrusted body (PR #982/#1035). A later reorder below it would silently demote the field to body text with no error.@sonichi
🤖 Generated with Claude Code