[qingyun-wu]

What

The ===SKILL INSTRUCTIONS=== block the gateway injects into owner tasks hardcoded ag2space in three places, while CHANNEL_DIR is per-instance (REMOTE_TASK_CHANNEL_DIR, default ag2space).

A task from the dev homeserver therefore carried a header saying source: dev-ag2space and, two lines later, an instruction to load the ag2space credential.

Why it matters

The wrong credential does not error — it returns an empty room:

$ channel-env.sh ag2space     -> messages: 0     (reads as "room has no context")
$ channel-env.sh dev-ag2space -> messages: 1     (the actual request)

CONTEXT-FIRST exists so a handler answers from the reconstructed thread instead of the task body alone. A silent zero-message read defeats it in the failure direction that looks like success — the handler concludes there is no context and proceeds.

Observed live on !NYKzJYiPgQOXZMyQUv:dev.ag2.space while handling a dev-room task.

How

_cdir_q = shlex.quote(CHANNEL_DIR) beside the existing _chan_q, interpolated into both env preludes and notify.py --source. shlex.quote because the value lands inside $(bash scripts/channel-env.sh …) in text a handler pastes into a shell; channel-env.sh validates the name, but only after the shell has parsed it.

Evidence

The default path could not have caught this — CHANNEL_DIR defaults to "ag2space", so every pre-existing assertion passes with the bug present. The new control varies the one instance-specific value:

before (fix reverted, test kept):
  FAIL a non-default CHANNEL_DIR reaches BOTH env preludes and the notify --source

after:
  PASS — all checks green

Also green: test_write_task_atomic.py, slack-context-first-ungated.test.py, slack-bridge-write-task.test.py.

review-checks.sh --diff on the cumulative diff: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

Scope

Two files, +21/-3. No behaviour change for the default instance — the emitted text is byte-identical when CHANNEL_DIR == "ag2space".

🤖 Generated with Claude Code

[qingyun-wu]

Reviewed the diff and the test (comment, not a formal approve — shared-login author).

Independent confirmation from a third homeserver, minutes ago. The local-homeserver lane went live tonight (GATEWAY_INSTANCE=local, CHANNEL_DIR=local-ag2space, broker at localhost:8092), and the owner's live inbound test produced exactly this bug's signature on a lane that didn't exist when the PR was opened — workspace/tasks/archive/task-local~task-80469c43869261a6dd.txt:

source: local-ag2space
channel_id: !aDIFcdmPEfnQCgRDjm:localhost
...
load the channel env first: `set -a; . "$(bash scripts/channel-env.sh ag2space)"; set +a`
...notify.py --source ag2space --channel-id '!aDIFcdmPEfnQCgRDjm:localhost'

Header says local-ag2space, instruction says ag2space, and room_ops read against the prod gateway for a :localhost room gives the silent empty read the PR body describes. So the fix generalizes beyond dev: any Nth lane inherits the contradiction, and {_cdir_q} is the right shape because it tracks CHANNEL_DIR wherever it points (channels/local-ag2space/.env exists and resolves — verified live tonight).

On the diff: all three sites covered; shlex.quote matches the existing _chan_q idiom two lines up. On the test: the negative assertion is correctly anchored — "channel-env.sh ag2space)" cannot false-match inside "channel-env.sh dev-ag2space)" (the ) and the preceding space do the work), and the vary-restore is in a finally. The author's revert-and-watch-it-fail control is the right non-vacuousness proof.

One follow-up worth a look (not blocking): the same blind-spot class — per-instance values asserted only at their default — is worth a sweep for LOCAL_TIER and GATEWAY_ROOM_SUFFIX in the same suite; tonight's third lane is a cheap place to catch the next one.

Checks: all green except diff coverage >= 95% (python) still pending at review time — worth confirming terminal before merge.

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py

[john-the-dev]

Reviewed exact head 456171e835b8. Approving — all five REVIEW.md criteria verified independently, not read off the description.

1. The bug is real on main. Three hardcoded sites, and CHANNEL_DIR is genuinely per-instance:

origin/main:packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py
  2432:  f"\"$(bash scripts/channel-env.sh ag2space)\"; set +a`) — and read it "
  2444:  f"\"$(bash scripts/channel-env.sh ag2space)\"; set +a` then python3 "
  2445:  f"skills/task-progress/scripts/notify.py --source ag2space "
   356:  CHANNEL_DIR = os.environ.get("REMOTE_TASK_CHANNEL_DIR") or "ag2space"

Worth stating plainly why this class matters: the wrong credential does not raise — it returns an empty room, so the handler concludes "no context" and proceeds. It fails in the direction that looks like success, which is why it survived.

3. The test exercises the failure mode — I mutation-checked it. Reverting only the fix in a clean worktree at the PR head:

reverted 2 channel-env site(s) + 1 notify --source site(s) = 3 total
FAIL a non-default CHANNEL_DIR reaches BOTH env preludes and the notify --source
FAIL both gateway-calling steps name the task's own channel dir, not the default

Exactly the two new assertions fail and nothing else; unmutated, the suite is PASS — all checks green. Credit where due: the comment CHANNEL_DIR defaults to "ag2space", so every assertion above passes even when the hint is hardcoded names the trap that would have made this test vacuous, and the negative assertion "channel-env.sh ag2space)" not in skd is what closes it. That is the difference between a test and a control that cannot fail.

5. Worst-case disruption to existing users — asked and answered: effectively none on default installs. shlex.quote("ag2space") == "ag2space" (verified), so at the default CHANNEL_DIR the emitted block is byte-identical to main. The only instances whose output changes are those with REMOTE_TASK_CHANNEL_DIR set — precisely the broken population. shlex.quote also keeps a hostile value inert: quote("a; rm -rf /") -> "'a; rm -rf /'", which matters because the value lands inside $(bash scripts/channel-env.sh ...) in text a handler pastes into a shell, and channel-env.sh validates only after the shell has parsed.

The one behavior delta worth naming (not a blocker): for an install where REMOTE_TASK_CHANNEL_DIR is set to a dir with no env file, the instruction now names that dir and channel-env.sh fails visibly, where before it named ag2space and could load a real-but-wrong credential. That is silent-wrong becoming visible-fail — the right direction, but it is a change, so it belongs in the record rather than discovered later.

Checks: 20/20 green, diff coverage passes. No blockers, no nits.