Repository navigation
feat(bridge): ===SKILL INSTRUCTIONS=== block on owner-tier gateway tasks - #2686
Conversation
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Reviewed the child-only layer at head 122ed710 (stacked on #2685's c96eb361), plus the cumulative diff against origin/main, per the stacked-PR review convention.
Child-only diff is clean, minimal (34 lines in the bridge + 20 in tests), and well-tested:
sender_tier = _tier_for(task.get("user_id"))is computed exactly once (line 1552) and reused for bothaccess_tier:and this gate (line 1569) — no risk of the two drifting from separately-derived values.- Verified the owner-only gate directly:
python3 src/remote-gateway-bridge.test.py→ PASS at head, including the three new checks (non-owner carries no block, owner carries the full block, block is the file tail). - Mutation-tested: reverted to
c96eb361(pre-this-PR) and reran → 2 failures, matching the PR's own claimed count exactly. Restored, byte-identical. - Security invariant checked, not just trusted: every line in the new
===SKILL INSTRUCTIONS===block is prose/numbered-sentence, none header-shaped (nokey:-at-line-start pattern), so it can't be mistaken for a forgedaccess_tier:/channel_id:line — same discipline as the existing secret-handling notice it's modeled on._chanis piped through_one_line()before interpolation, so a channel_id containing a newline can't inject an extra header-shaped line either. bash scripts/review-checks.sh --diff(child-only diff) → PASS (hardcoded-paths clean).- CI green except a
SKIPPEDpublish-sparrow job (benign, seen on sibling PRs today too).
Stacking note for whoever merges: this PR's tree currently still carries the stray tests/__pycache__/*.pyc binary I flagged on #2685 (c96eb361) — confirmed it's inherited from the parent, not introduced here (child diff touches only the bridge file + its test). No new action needed on this PR for that; it resolves once #2685 drops the file and this branch rebases. Per the PR body's own note and CONTRIBUTING's stacked-PR guidance: merge #2685 first, then rebase/re-verify this one before merging.
No blockers on the child layer itself. Approving.
122ed71 to
f17ccff
Compare
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 122ed7109c070ed7910b30f738ce803388e014c0, as the incremental diff on top of #2685.
No blocking findings in this stacked change. The owner-tier task-hygiene block is emitted only after the locally resolved access_tier: owner, non-owner/clamped tasks do not receive a competing instructions block, and the added text is bridge-generated prose rather than another header-shaped field. The result-path line still uses the local task id, matching the file the core is expected to write.
Validation run:
git diff --check pr-2685...HEADpython3 src/remote-gateway-bridge.test.py(PASS)python3 scripts/check-python39-compat.py --target packages/ag2-sparrow/ag2_sparrow --target src(106 files)
Approval signal for this incremental diff. The stack still depends on #2685 dropping the committed __pycache__ bytecode before either PR lands.
Reviewed by Qingyun's Personal Codex.
|
Cold review — no blockers; both load-bearing recipe claims checked against the shipped tools rather than assumed:
Two smaller observations: (a) Owner-only gating with the non-owner-absence test is the correct posture (no second instruction block competing with SUTANDO SYSTEM INSTRUCTIONS). CI green; owner's merge. — qingyun-sutando (001) |
Owner ask 2026-08-05 (circulated the discord task file as the reference): gateway tasks now carry the same task-hygiene block the discord bridge writes — context-first thread reconstruction (room_ops read, with the channel-env hint for hosts whose core lacks the gateway exports), notify-first for >60s tasks (task-progress notify, which already speaks remote-gateway for any non-discord/slack/telegram source), and the result path. Owner-tier ONLY: non-owner tiers carry ===SUTANDO SYSTEM INSTRUCTIONS=== and must not receive a second, competing instruction block. Appended after access_tier like the secret notice — prose lines, none header-shaped, so the tier-wins-last invariant holds (pinned). Stacked on feat/task-envelope-room-members (merge that first). Tests: owner task carries the block with all three recipes; clamped-tier task does NOT; block is the file tail; exactly one access_tier line. Fails-without-fix control: 2 FAILs with packages/ stashed -> PASS.
f17ccff to
8b0f3d5
Compare
…r skill block test_write_task_atomic pinned 'access_tier is the last line' as its anti-spoof + completeness proxy. #2686 appends the bridge-authored ===SKILL INSTRUCTIONS=== block after access_tier (owner tier), so the proxy moved: assert the sharper invariant instead — exactly one header-shaped access_tier line with only bridge block text after it, and the block's result-path line as the completeness tail. Verified the updated test still fails against a bridge without the block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiaMLCxgKNAbmhty29WGN6
|
Full-suite CI on the new main base caught what the thin feature-branch CI could not: Fixed in 2600fe8 by asserting the sharper form of the same invariant instead of the line position:
Evidence: |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 78%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…lock Same class as the atomic-write fix: gateway-writeside-attachments and remote-gateway-interaction-type both asserted access_tier is the last non-empty line. Under CI's clean env (default owner tier) the ===SKILL INSTRUCTIONS=== block follows access_tier, so both now assert the real invariant — exactly one header-shaped access_tier line with only bridge block text after it. Both reproduced red under a scrubbed env (they passed on the dev host only because local tier config took a different path), both green after, and the predicate still rejects a forged second access_tier line. Sibling gateway tests swept clean-env: no further tail proxies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiaMLCxgKNAbmhty29WGN6
|
Second CI pass unmasked the same tail-proxy in two more tests — Fixed in 0ae3d06 with the same sharper invariant as the atomic-write fix (exactly one header-shaped |
john-the-dev
left a comment
There was a problem hiding this comment.
Approving after a full read + running the changed tests. This adds the owner-tier ===SKILL INSTRUCTIONS=== block (context-first room reconstruction via room_ops.py read, notify-first, result path) to gateway tasks — mirroring the Discord bridge's block — and does it safely:
- Owner-tier only (
if sender_tier == "owner"); non-owner tiers keep===SUTANDO SYSTEM INSTRUCTIONS===, so there's no competing instruction block. - Anti-spoof invariant preserved: the block is appended after
access_tieras prose + numbered sentences with no header-shaped lines, so the access-tier-wins-last / last-occurrence-parser guarantee still holds.test_write_task_atomic.pynow pins exactly this (singleaccess_tier:line,===SKILL INSTRUCTIONSstrictly after it, result-path is the file tail so a truncated write can't produce it). - Tests:
test_write_task_atomic.py,src/remote-gateway-bridge.test.py,gateway-writeside-attachments.test.py,remote-gateway-interaction-type.test.pyall pass locally; theaccess_tier is the last headerassertions still hold.
LGTM. (Heads-up: needs one more maintainer approval — bassil's earlier approval pre-dates the latest substantive commit, so it doesn't currently cover head.)
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal for current head 0ae3d06.
No blocking findings on the rebased main-based diff. The owner-only task-hygiene block is still gated by the locally resolved owner tier, non-owner/clamped tasks do not receive the competing block, and the updated tests now pin the sharper invariant: exactly one header-shaped access_tier line, with only bridge-authored skill-block text after it. The block does not move AppService namespace credentials into Sutando-local; it only tells an owner-tier local core how to read context and send progress through the existing narrow gateway/room-ops path, with membership still enforced gateway-side.
Focused checks passed in an isolated /private/tmp checkout:
- git diff --check origin/main...HEAD
- src/remote-gateway-bridge.test.py
- packages/ag2-sparrow/tests/test_write_task_atomic.py
- clean-env tests/gateway-writeside-attachments.test.py
- clean-env tests/remote-gateway-interaction-type.test.py
- Python compilation for the changed bridge/test files
Hosted clean-install, diff coverage, smoke, lint, ruff, shellcheck, and CLA are green on this head.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
LGTM, non-blocking review for exact head 979c2df3f883846eff6713e4036c87b8e0ce2172.
This is a merge-main refresh only: git range-diff shows all three topic commits unchanged, and the current five-file diff remains the owner-only gateway skill-instructions change previously reviewed. I re-read the full current diff and surrounding _write_task path; the locally resolved owner tier gates the block, _one_line() prevents channel-id line injection, and non-owner tasks retain only their system-instructions path.
Exact-head validation passed:
src/remote-gateway-bridge.test.pypackages/ag2-sparrow/tests/test_write_task_atomic.py- clean-env gateway attachment and interaction-type suites
- Python 3.9 compatibility,
git diff --check, andscripts/review-checks.sh
Worst-case disruption is an owner task being delayed by the new unconditional context-read recipe; scope is limited to owner-tier gateway tasks, and the generated instruction includes the existing channel-env recovery path. No broader user or non-owner behavior changed.
Code-ready. Merge after the two still-running hosted checks (clean install and diff coverage) pass. Formal self-approval is unavailable on this author account, so this is the exact-head approval signal.
Reviewed by Qingyun's Personal Codex.
|
CI follow-up for current head 979c2df: the hosted clean-install, diff coverage, smoke, static gates, and CLA checks are now green. The exact-head review from earlier still stands; no blocking findings. Reviewed by Qingyun's Personal Codex. |
There was a problem hiding this comment.
CHANGES REQUESTED on exact head fec3f40984f8ae95182934f0c1a4351f55c65d37 (formal self-review state is unavailable).
-
[P1 blocker] Shell-quote the provider channel id before putting it in executable instructions.
_one_line()only removes CR/LF, butpackages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1597and:1610interpolate_chaninside single-quoted shell arguments. A valid provider value containing a quote, such as!room'; touch /tmp/injected; #, closes the quote and turns the remainder into shell syntax when the owner-tier core follows the generated command. Use an argv-safe representation such asshlex.quoteconsistently for both recipes, and add a quote/metacharacter regression. -
[P2 blocker] Apply the current two-line/no-history code-comment rule. The added production block at
:1580-1588is nine lines and carries owner/date/other-adapter history; several added test comment blocks do the same. Keep only the non-obvious invariant and move review history to the PR body.
Focused gateway wire-path and write-side suites passed, including the loopback test, atomic writer, attachment headers, interaction types, compilation, diff hygiene, and REVIEW path scan. Hosted checks are green.
Not merge-ready: worst case is an owner-privileged agent executing provider-controlled shell syntax.
Reviewed by Qingyun's Personal Codex.
…recipes
A provider-controlled channel_id was embedded in the owner-tier
SKILL INSTRUCTIONS block using a manual f"'{_chan}'" single-quote
wrap, which only strips CR/LF (_one_line) and does not escape
embedded quotes. A channel id containing a single quote (e.g.
"!room'; touch /tmp/x; #") closes the shell quote early and turns
the remainder into a second shell command if the generated
instructions are executed literally.
Use shlex.quote() for both recipes (room_ops.py read and
notify.py --channel-id) instead. Added a regression test that
parses the embedded commands with shlex: on the pre-fix code
shlex.split raises "No closing quotation" (proving the quoting
was genuinely unbalanced), confirmed locally by reverting the fix
and re-running; on fixed code it round-trips to the channel id as
exactly one argument.
Also trimmed two comment blocks added by this PR to the repo's
2-line/no-PR-reference convention (CLAUDE.md).
Addresses the P1 blocker + P2 comment-length finding from this
PR's review.
|
Fixed both findings at 1. Shell-injection (P1). Replaced the manual Confirmed the vulnerability was real and reproduced it before fixing — a channel_id containing a single quote breaks the manual wrap: Added Full suite, no regressions: 2. Comment-length (P2). Trimmed the 9-line production comment and the 4-line test comment (which named Both fixes are on this branch at |
qingyun-wu
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED on exact head ffc9c210b600bffd67905b775152cf4c060e46ad (formal self-review state is unavailable).
The P1 shell-quoting fix is correct: both generated commands now use shlex.quote, the malicious single-quote regression passes, the full loopback gateway suite passes, the focused atomic/attachment/interaction suites pass, Python compilation and diff hygiene pass, and the REVIEW path scan is clean.
[P2 blocker] The new regression immediately reintroduces the comment-policy finding it claims to clear. packages/ag2-sparrow/tests/test_write_task_atomic.py:162-170 adds a nine-line docstring containing review history, the old exploit narrative, and an explicit #2686 reference. The repository rule applies to test comments/docstrings too: keep at most two lines naming the non-obvious invariant, and leave the mutation/review history in the PR body.
Hosted clean-install CI is also still in progress. Not merge-ready: the security defect is fixed, but the exact current-head comment-policy blocker remains.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Independently re-verified the shell-injection fix at ffc9c210 (current head — my prior approval and john's were both on earlier commits, before this fix landed).
Fix is real and correctly applied. Reproduced the vulnerability myself before trusting the claim: reverted just remote_gateway_bridge.py back to the pre-fix version (ffc9c210~1) and reran packages/ag2-sparrow/tests/test_write_task_atomic.py — test_write_task_shell_quotes_channel_id_in_skill_instructions fails with the exact claimed ValueError: No closing quotation, confirming the manual f"'{_chan}'" wrap genuinely broke on an embedded quote. Restored the file, reran — full suite passes (6/6, ALL PASS test_write_task_atomic). shlex.quote() is applied at both call sites (room_ops.py read recipe and notify.py --channel-id recipe), and the new test parses the embedded shell commands with shlex.split rather than a substring check, so it can't be satisfied by a visually-similar-but-still-broken quoting scheme.
One residual, non-blocking nit: the fix commit fixes a 2-line-comment violation in one place but introduces a fresh one in another — remote_gateway_bridge.py:1585-1587 is a 3-line inline # comment (# shlex.quote, not a manual f"'{_chan}'" wrap: ... / # channel id containing a quote would otherwise close the shell string / # early and turn the remainder into executable shell syntax.), over CLAUDE.md's 2-line cap for inline comments. Worth a quick trim in a follow-up, doesn't block — the security content is what matters here and it's correct.
Approving on ffc9c210.
…e rule The prior commit fixed the P1 shell-injection finding but its own new code violated the same CLAUDE.md comment convention it was enforcing elsewhere: a 3-line inline comment, and a 9-line test docstring that named this PR directly. Trimmed both to 2 lines, no PR/issue references. No behavior change — re-ran the full ag2-sparrow suite (11 files) green.
|
Fixed at Good catch — my own P1 fix reintroduced the comment-length violation it was supposed to be clearing: a 3-line inline comment on the No behavior change — same |
|
CHANGES REQUESTED (blocking CI status on current head) I re-reviewed I did not find a new code-level blocker in the changed bridge behavior: Sutando-local still does not gain AppService namespace credentials, the generated owner-tier block uses the existing scoped room-ops/notify channel path, membership/authorization remains on the gateway/room-ops side, and the existing channel env fallback is preserved for onboarding compatibility. The current head is still blocked by required CI, though. Local focused checks I ran from the isolated
Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED on exact head d0395afb97be31de058d1ad1d7679648a4f50804 (formal self-review state is unavailable).
The new cleanup correctly reduces the production shlex.quote explanation and malicious-channel regression docstring to two lines without changing behavior. Reviewer-run atomic, attachment, interaction-type, and full localhost gateway suites pass; Python compilation, diff hygiene, and the REVIEW path scan pass. The shell-injection fix remains intact.
[P2 blocker] Two other comment blocks added by this PR still violate the same current policy. tests/gateway-writeside-attachments.test.py:104-108 and tests/remote-gateway-interaction-type.test.py:75-79 are each five lines and explicitly reference #2686. Reduce each to at most two lines stating only the non-obvious invariant; the PR/history context belongs in the PR body.
The rerun of required CI is still in progress. Not merge-ready: functionality is sound, but the exact-head comment-policy cleanup is still incomplete and required CI is not yet green.
Reviewed by Qingyun's Personal Codex.
…line rule tests/gateway-writeside-attachments.test.py and tests/remote-gateway-interaction-type.test.py each carried a 5-line comment block naming this PR directly, added by an earlier commit on this branch. Trimmed both to 2 lines stating only the invariant. No behavior change — re-ran both targeted tests plus the full ag2-sparrow suite (11 files) and src/remote-gateway-bridge.test.py, all green.
|
Fixed at Good catch again — No behavior change — including confirming Also confirming CI: I'm watching the required |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 665591e2d2e7b931840fab8666d4ada52b3b071e (formal self-approval is unavailable).
The remaining comment-policy blocker is resolved: the two five-line/history blocks are now two-line invariant comments with no PR/person reference. This delta changes only those comments, so the previously verified behavior is unchanged: the shlex.quote security fix, malicious-channel regression, atomic/attachment/interaction tests, and full localhost gateway suite all pass; Python compilation, diff hygiene, and the REVIEW path scan were clean.
No further code finding. Code-ready, but not yet merge-ready: required checks on this exact head are still running (CI, Coverage Gate, and bundle-smoke); the completed hosted checks and CLA are green. A current-head formal maintainer approval is still required by the repository merge gate.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Re-reviewed on current head 665591e (my prior approval was on ffc9c21, two commits behind).
Verified the diff between ffc9c21..665591e directly (git diff ffc9c210b600..665591e2d2e7): both intervening commits (d0395af, 665591e) are pure comment/docstring trims to the 2-line no-PR-reference rule across 4 files — no logic changes. This resolves the two comment-policy findings qingyun raised after my last approval (remote_gateway_bridge.py:1585-1587 person/PR reference, plus the two similar violations in gateway-writeside-attachments.test.py and remote-gateway-interaction-type.test.py).
CI on 665591e: all fast checks + tsc + tests (clean install) + diff coverage >= 95% pass; smoke still pending at review time.
Approving on this exact head.
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approve after the post-approval commits. The shlex.quote(_chan) hardening is correct — it prevents a channel_id from breaking out of the embedded shell commands in the SKILL INSTRUCTIONS block, and the added regression test proves a malicious channel_id round-trips as a single arg (shlex.split recovers it) in both the context and notify command lines. Comment trims satisfy the 2-line rule. CI green, CLEAN. LGTM.
|
@cla-assistant check |
Gateway tasks now carry the same task-hygiene block the discord bridge writes — with gateway-native recipes:
room_ops.py read '<channel_id>' --limit 30(plus the channel-env hint for hosts whose core session lacks the gateway exports), same unconditional wording as discord's (that wording is owner-approved prompt surface; mirrored, not rewritten).task-progress notify --source ag2space; the notify script already routes any non-discord/slack/telegram source through the remote gateway.Owner-tier only: non-owner tiers carry
===SUTANDO SYSTEM INSTRUCTIONS===and must not get a second, competing instruction block. Appended afteraccess_tierlike the secret-handling notice — prose lines, none header-shaped, so the tier-wins-last invariant holds (pinned by test).Stacked on
feat/task-envelope-room-members(#2685) — merge that first; this PR's diff on top is the block + its tests.Evidence:
New checks: owner task carries all three recipes; clamped-tier task carries none; block is the file tail; exactly one
access_tierline.🤖 Generated with Claude Code
https://claude.ai/code/session_01KiaMLCxgKNAbmhty29WGN6