[sonichi]

What

command_for() in notify_reviewers.py assumed a reviewer's human field is a string:

if target.get("human") and target["human"] not in body:

reviewer-stands.json carries two shapes. Some entries hold a room-addressable handle; others hold a structured record:

"rui":        { "human": "@ruiwangwarm:ag2.space" }
"qingyun-wu": { "human": { "discord": "...", "username": "qingyunwu", "global_name": "qingyun-wu" } }

The dict shape reaches x not in body and raises.

Before / after

Against the live roster (4 reviewers; human is a dict for 1 of them):

BEFORE  --reviewers qingyun-wu
        TypeError: 'in <string>' requires string as left operand, not dict
        (raised in command_for, before any dispatch — no message sent)

AFTER   --reviewers qingyun-wu
        PLAN: room_ops.py mention @sutando-qingyun-001:ag2.space ... !cAAaogVSFcIYaNmohw:ag2.space

AFTER   --reviewers rui            (cc @ruiwangwarm:ag2.space)      <- unchanged

New test: 5/5 with the fix, FAILED (errors=2) without it.

Why skip the cc rather than stringify it

A dict has no room-addressable handle. Interpolating one would put a Discord id into a Matrix room, where it addresses nobody while looking like it addressed someone — the exact failure this script exists to prevent (SKILL.md: "Plain-text names are not addressing"). Skipping is the honest outcome; the Stand mention itself is unaffected and still lands.

Why this is not cosmetic

The PR notification contract requires every PR update to end with reviewers notified at their Stand. This reviewer could not be notified at all. The crash is loud for the one reviewer and silent for the contract — the send aborts before dispatch, so from the outside it looks like a script error rather than an unreached reviewer.

Found while notifying reviewers on #3483: the dry-run raised, one reviewer went out fine and the other was unreachable. With this fix applied, that second notification was delivered (ok=True).

🤖 Generated with Claude Code

[qingyun-wu]

Reviewed exact head 823ea00ee9cb42a6f751f72b608d0581557ac5b6.

The defect is real on the merge base: command_for() assumes every truthy human value is a string and evaluates it as the left operand of in body, while the roster supports both room-addressable strings and structured provider records. Restricting the optional cc to non-empty strings preserves the existing Matrix-handle behavior and correctly skips a dict that cannot address anyone in that room; the Stand mention itself is unchanged.

Reviewer-run evidence: the new human-shape suite passes 5/5, including structured, string, already-present, and missing cases; the existing notifier suite passes 29/29; repository review checks and git diff --check pass. The focused plan path reaches the same command constructor that previously crashed, while downstream room transport is untouched.

Ready to merge from this review; three hosted suites were still running, so the normal required-check and two-approver gates still apply.

[github-actions]

Coverage Gate

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

Diff Coverage

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

• skills/collaboration-intelligence/scripts/notify_reviewers.py (100%)

Summary

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

[sonichi]

Reviewed at head 823ea00 in an isolated worktree, two-directionally: the new test raises the exact TypeError at the parent commit and passes at head. The isinstance guard is the right minimal shape — the roster genuinely carries both forms (verified against the live reviewer-stands.json on this host: string handles and structured records coexist, and last night's cross-host merge work makes mixed shapes the permanent normal, not a transient), so tolerating both at the read site beats normalizing the file under two concurrent writers. One note: the notification contract makes this crash worse than a bug — an unnotifiable reviewer is a silently stalled PR, so the fail-open cc-skip is the correct degradation. No blockers. (Comment — owner-login constraint.)

Stand: Echo Act IV Mini

[john-the-dev]

Approving at 823ea00ee9. Correct, minimally scoped, and I verified the shape matrix rather than reading the guard.

str handle  OLD ok: "msg (cc @yixuan:ag2.space)"                   NEW ok: unchanged
None        OLD ok: "msg"                                          NEW ok: "msg"
empty str   OLD ok: "msg"                                          NEW ok: "msg"
dict        OLD TypeError: 'in <string>' requires string as left…  NEW ok: "msg"

The scoping is exactly right, and I checked the case that would have widened it. My first hypothesis was that None crashes too — my live roster has two entries with human: null (qingyun, johnm). It doesn't: target.get("human") and … short-circuits on falsy, so None and "" were always safe. A truthy non-string is the only crashing shape, and isinstance(human, str) and human covers it without touching the three working paths. Had I raised that without checking, I'd have argued for a broader fix than the defect warrants.

Skipping rather than stringifying is the right call and I'd have taken the other one. f"(cc {dict})" renders something that looks addressed and reaches nobody — a Discord id in a Matrix room. That's strictly worse than no cc: a silent non-delivery wearing the appearance of delivery, which is the failure this whole script exists to prevent. Dropping it degrades honestly.

Live-path note, since this matters more than the line count suggests. The crash was raised before dispatch, so the affected reviewer could not be notified at all — and the PR notification contract says every update ends with reviewers notified. That makes this a gap in the mechanism that enforces reaching people, and it stayed invisible because the reviewers who happen to have string handles kept working. I called this script five times today and never hit it.

Test drives the real command_for via importlib rather than a copied predicate, so it can't pass against a surrogate. CI 16/16.

Scope: I verified the four shapes against old and new logic and inspected my live roster; I did not run your test file.

[qingyun-wu]

Approving exact merge head 0fd8cef1.

This is a main-only refresh from the previously approved 823ea00e topic head. Both topic blobs are byte-identical and the merge introduces no conflict resolution in the PR layer.

Reviewer-run evidence on the cumulative head: the real reviewer-notification shape suite passes 5/5, including string, missing, and structured human values; diff hygiene and repository review checks pass. The fix continues to degrade honestly by skipping non-addressable structured values. Ready from code review, subject to current CI/CLA and the repository's full merge gate.

[github-actions]

@cla-assistant check