Skip to content

feat(bridge): accept room_members + room_member_count in the task envelope - #2685

Merged
john-the-dev merged 5 commits into
mainfrom
feat/task-envelope-room-members
Aug 5, 2026
Merged

john-the-dev merged 5 commits into
mainfrom
feat/task-envelope-room-members

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

The gateway writer side now enriches tasks with room-membership context — a capped one-line member list plus the true joined total — alongside the existing room_name/sender_name/reply_to_* fields. This adds the two fields to the bridge's _TASK_FIELDS serialization whitelist so they reach the task file; same contract as the rest of that group: present only when the wire sends them, newline-stripped by _one_line, absent for other sources.

Evidence

python3 src/remote-gateway-bridge.test.py            -> PASS — all checks green
  (whitelist change stashed)                         -> 2 FAILs (new checks)
  (restored)                                         -> PASS — all checks green

New check: room_members: @a:x, @b:x (+3 more) + room_member_count: 5 serialize through _write_task.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KiaMLCxgKNAbmhty29WGN6

…elope

The gateway writer side now sends room-membership context (a capped
one-line mxid list + the true joined total) alongside the existing
room_name/sender_name/reply_to_* enrichment. Same contract as those
fields: serialized only when the wire sends them, newline-stripped by
_one_line, absent for other sources.

Test: fails-without-fix control run — 2 FAILs with the whitelist change
stashed, PASS all green with it. Full src/remote-gateway-bridge.test.py
green both matrices of the change.
@qingyun-wu
qingyun-wu requested a review from john-the-dev August 5, 2026 19:15

@bassilkhilo-ag2 bassilkhilo-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.

Reviewed head c96eb361. The intended change is small and correct — verified independently in a scratch clone.

The actual fix (whitelist addition) is sound.

  • packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py: adds "room_members", "room_member_count" to _TASK_FIELDS, matching the existing contract of the surrounding fields (present only when the wire sends them, newline-stripped by _one_line).
  • src/remote-gateway-bridge.test.py: PASS — all checks green at head.
  • Mutation-tested: reverted the whitelist to origin/main's version and reran → 1 failure (the new serialization check). Restored, byte-identical.

Blocker: a compiled .pyc binary is committed in this diff, and it shouldn't be.

tests/__pycache__/task-workstream-session-worker.test.cpython-312.pyc | Bin 0 -> 50681 bytes

__pycache__/ is genuinely in .gitignore (line 6) — confirmed directly, not assumed — so this had to be force-added (git add -f or an editor/tool that bypassed the ignore) rather than picked up by a normal git add .. It's a real artifact: git log origin/main -1 on that path shows nothing, so this is new, not a pre-existing tracked file the PR merely touches. Compiled bytecode is interpreter/architecture-specific and has no business in git history — worth dropping before merge:

git rm --cached tests/__pycache__/task-workstream-session-worker.test.cpython-312.pyc

Once that's removed, this is approve-ready from my side — no other concerns.

@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.

Reviewed exact head c96eb3610485783e1e15a0f3006af9f95d2cd7fe.

Changes requested: remove the committed generated bytecode file tests/__pycache__/task-workstream-session-worker.test.cpython-312.pyc (50,681 bytes). __pycache__/ is explicitly ignored by .gitignore; this interpreter-specific binary is unrelated to the two-field envelope change and should not ship in source or release artifacts.

The functional two-file change is otherwise code-ready: current main lacks both fields; the whitelist addition routes them through the existing _one_line() header-safety path; the full bridge suite passes, Python 3.9 parsing passes, and diff/path checks are clean. Hosted CI and coverage were still running; CLA and the completed hosted gates were green.

Worst-case runtime disruption from the intended change is minimal and additive, but the generated binary is an avoidable repository/release artifact. Not merge-ready until it is removed. Formal REQUEST_CHANGES is unavailable because the authenticated reviewer account authored this PR, so this COMMENT review records the blocking verdict.

git add -A swept a __pycache__ artifact from an earlier local test run
into the previous commit despite the .gitignore rule — both reviewers
caught it. Interpreter-specific binaries do not belong in history.
@github-actions

github-actions Bot commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

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

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

Summary

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

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Both reviews addressed at 4761d1c — the .pyc is gone: git rm --cached tests/__pycache__/task-workstream-session-worker.test.cpython-312.pyc, and git ls-files | grep -c __pycache__ now returns 0 on the branch. It was swept in by a git add -A after running that test locally in this clone; thank you both for the catch. The stacked #2686 is rebased onto the fixed base (f17ccff) — its tree also carries no bytecode — and src/remote-gateway-bridge.test.py passes on both branches post-rebase.

@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.

Reviewed exact head c96eb3610485783e1e15a0f3006af9f95d2cd7fe.

Blocking finding:

  • Remove the committed generated bytecode file tests/__pycache__/task-workstream-session-worker.test.cpython-312.pyc. __pycache__/ artifacts are interpreter-specific build output, unrelated to the envelope whitelist change, and should not ship in source or release artifacts.

The functional change is otherwise sound: room_members and room_member_count are added to the existing task-field whitelist, ride the same one-line serialization path as the neighboring context fields, and the bridge test covers that both fields survive _write_task().

Validation run:

  • git diff --check origin/main...HEAD
  • python3 src/remote-gateway-bridge.test.py (PASS)
  • python3 scripts/check-python39-compat.py --target packages/ag2-sparrow/ag2_sparrow --target src (106 files)

Not merge-ready until the generated .pyc is removed. Formal REQUEST_CHANGES is unavailable because the authenticated reviewer account authored this PR, so this comment records the blocking verdict.

Reviewed by Qingyun's Personal Codex.

@bassilkhilo-ag2 bassilkhilo-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.

Re-reviewed at head 4761d1c0. The blocker is fixed correctly, not just removed.

git show 4761d1c0 --stat confirms a proper deletion (Bin 50681 -> 0 bytes), not just an untrack-and-forget — the commit message ("both reviewers caught it") lines up with my finding. Verified directly: git ls-tree -r HEAD --name-only | grep pycache → empty, no lingering artifact anywhere in the tree.

Re-ran python3 src/remote-gateway-bridge.test.py at this head → still PASS, all checks green — the actual whitelist change (the point of the PR) is untouched by the cleanup commit.

No other concerns from my side (the whitelist addition itself was already verified correct in my first pass, mutation-tested against origin/main). Approving.

@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.

Re-reviewed head 4761d1c0b1a0d37422ccf0435411fa1cf54be784.

The generated .pyc artifact is gone; the PR now only carries the intended bridge whitelist/test changes. I do not see remaining blockers. room_members and room_member_count now serialize through the same existing one-line task-envelope path as the neighboring context fields, and the focused regression covers both fields surviving _write_task().

Validation run:

  • git diff --check origin/main...HEAD
  • git ls-files '*__pycache__*' '*.pyc' (no tracked bytecode)
  • python3 src/remote-gateway-bridge.test.py
  • python3 scripts/check-python39-compat.py --target packages/ag2-sparrow/ag2_sparrow --target src

Formal approval is unavailable because the authenticated reviewer account authored this PR, so this comment is my approval signal for the fixed head.

Reviewed by Qingyun's Personal Codex.

@john-the-dev john-the-dev 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.

Approving after a full read + running the PR's test (head 4761d1c0).

Verified:

  • Correct + minimal: adds room_members + room_member_count to the gateway writer's header-field allowlist in _token_from_vault_ag2space. They ride the same _one_line newline-stripping path as room_name/sender_name, so a room/member string can't forge an extra header line (access_tier: injection, etc.) — the security contract is preserved, not widened.
  • Test: python3 src/remote-gateway-bridge.test.py passes locally, including the new assertion that room_members/room_member_count serialize into the task header. CI green (publish is a benign skip).
  • Head-timing: bassil's approval (19:26Z) post-dates the head commit (19:21Z), so this is a clean 2nd approval on the current head.

One optional strengthening (non-blocking): the existing newline-forgery test targets sender_name; a parallel assertion that a newline inside room_members can't forge a second access_tier: line would pin the guard for these new fields specifically. Since they share the _one_line path it's covered in practice — just would make the invariant explicit. Not required for merge.

@john-the-dev
john-the-dev enabled auto-merge (squash) August 5, 2026 22:58
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit 4558e10 into main Aug 5, 2026
20 checks passed
@john-the-dev
john-the-dev deleted the feat/task-envelope-room-members branch August 5, 2026 23:04
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.

3 participants