Repository navigation
fix(room-ops): mention resolves display names — broker resolve_user, then a display-name-aware roster - #4126
Conversation
…then a display-name-aware roster
An agent that wants to hand off to the peer it can see in the room —
"Bassil's Sutando" — could not: `/v1/agents` lists only this account's own
agents, and the room fallback threw the display names away one call after
`members` had fetched them (`_resolve_from_room` kept `user_id`s only;
`match_member` had no label tier; matching was lowercase-only, so a space, a
curly apostrophe or a possessive was a miss). `mention` refused, callers fell
back to `say`, the body carried no mxid, and the peer — which by design
ignores agent-authored messages that do not mention it — never saw the
hand-off. The broker was never the problem: it stamps `mentions` into
`m.mentions`, auto-mentions any member mxid in the body, pills it for humans,
and has offered a room-scoped `op: resolve_user` that matches display names
since 2026-07-21. Nothing here called it.
resolve.py
normalize_handle canonical form for handles, labels and display names:
apostrophe look-alikes → "'" (before NFKC, which would
decompose U+00B4), NFKC, casefold, drop possessive "'s",
drop apostrophes, whitespace/_/- runs → "-".
slug_handle the platform's localpart spelling guess
("Bassil's Sutando" → "bassil-s-sutando").
match_agent five tiers: exact localpart → exact label/display name
→ token-prefix → substring localpart → substring name;
a tie is refused unless `prefer_agents` and exactly one
candidate is agent-kind (only within a tier: an exact
human name still beats an agent).
match_member accepts mxid strings or the {user_id, display_name}
dicts `members` returns.
resolve_in_room POST /v1/room {op: resolve_user}; pure
parse_resolve_user_response / is_ambiguous; 404 or
"unknown op" → unsupported.
mention.py
_resolve_from_room asks the broker first (handle, then the slug on a plain
miss), returns an ambiguous answer as the refusal it is,
and only then reads the roster with display names,
agents preferred on a tie. Results carry
`resolved_by: directory|broker|room`. The stale
"mentions is harmlessly ignored" comment is gone.
members.py agent-kind prefixes mirror the client's agentMxid.ts
(sutando-, codex-, hermes-, openclaw-, cline-, pi-, kilo-).
SKILL.md the addressing block now says what the platform does:
full mxid with the .agent suffix, routing on m.mentions
or a whole-token mxid in the body, auto-mention + pill,
the silent drop of an un-mxid'd hand-off, `mention` as
the hand-off verb, `say` pings nobody, `members` before
guessing, `addressed_to` → [no-send].
Tests: test_room_ops.py 168 (+18: every spelling of "Bassil's Sutando",
"Bassil" → the human, "sutando" → two candidates, slug, ids-only lists,
prefer_agents ties, resolve_user response parsing);
agent-room-ops-mention-room-fallback 21 (+9: broker hit/ambiguous/
unsupported/slug retry/network, directory still wins, roster tie → agent);
new room-ops-resolve-in-room 34 checks; room-ops-members 28 (+7 prefixes).
ruff clean under the repo's py39 target.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving. Reviewed as the third of the three mention PRs, because the interesting failure here is cross-repo and no CI run can see it — #957 composes, this resolves, #956 notices, and none of the three builds against the others.
The design holds up on its own terms. Tiered ranking with a single winner at the best populated tier, a tie reported as candidates for the caller to disambiguate rather than guessed at, and prefer_agents narrowly scoped to the person-and-their-agent tie. That is the same instinct as #957's tabPick refusing on ambiguity, and it's the right one: a wrong mention is a silently dropped hand-off, so refusing beats guessing. slug_handle documenting itself as a best-effort guess whose wrong answers are misses and never wrong hits is the other half of that.
The finding: #957 claims parity with this function, and the two have already drifted
mentionCandidates.ts in ag2-space/cinny-webclient#957 says its normalizeMentionText uses "Same rules as the engine's normalize_handle". I transcribed both and ran them against the same inputs. Folding Bassil<x>s Sutando onto the canonical form:
apostrophe engine folds client folds
U+0027 ' True True
U+2019 ’ True True
U+2018 ‘ True True
U+201B ‛ False True
U+02BC ʼ False True
U+2032 ′ False True
U+0060 ` True False
U+00B4 ´ True False
Neither set contains the other. Intersection is ‘ ’; this side alone has ` and ´; the client alone has ‛ ʼ ′. So this PR's gap is ‛ ʼ ′ — a user whose keyboard produces U+02BC gets a working autocomplete in the composer and then no resolution here.
Two other divergences fall on the client, which I'll note there rather than here:
- Your docstring says the apostrophe map runs first "because NFKC decomposes U+00B4 (acute accent) into a space plus a combining mark". That is exactly right, and the client does NFKC first — so
Bassil´s Sutandoresolves here and does not match there. Your comment predicted the other implementation's bug. lstrip("@")has no client counterpart, so@Bassil's Sutandonormalises here and not there.
What I'd actually suggest
Adding ‛ ʼ ′ to _APOSTROPHES is a one-line fix and worth doing. But the real defect is that this is a hand-maintained character table with two independent copies, and they have drifted before either was merged. Each author enumerated the look-alikes they had seen; neither list is wrong, and both are incomplete in different directions. That is the shape that keeps drifting, and the next divergence won't announce itself.
I don't think that should block this PR — the engine and a TypeScript client can't share a module, and inventing a sync mechanism here would be scope creep. But it's worth one of the two carrying a comment naming the other as the twin, so the next person editing either knows there is a second copy. A shared fixture list (the same handful of variant spellings, asserted in both suites) would be cheaper than it sounds and would have caught all four of the above.
Everything else I checked held. test_room_ops.py pinning sutando-qingyun-001 and bassil-s-sutando as their own normal forms is the right pair to pin — those are the cases where over-normalising would collapse two distinct agents.
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Checked the 3 failing checks directly rather than just flagging them as red.
typecheck + node & shell tests fails before reaching typecheck/tests at all — review-checks.sh's prose-cap gate (this repo's "comments ≤2 lines" rule) rejects 8 blocks:
skills/agent-room-ops/members.py:20(4 lines)skills/agent-room-ops/mention.py:140(6 lines)skills/agent-room-ops/resolve.py:51(3 lines)skills/agent-room-ops/test_room_ops.py:568(4 lines),:632(3 lines),:655(4 lines),:775(4 lines)tests/agent-room-ops-mention-room-fallback.test.py:254(4 lines)
diff coverage >= 95% (python): 92% (11/145 changed lines uncovered), all in the same touched files:
mention.py84.6% — missing lines 130, 135, 166, 169resolve.py93.9% — missing lines 115, 133-134, 170, 187, 189, 285test_room_ops.pychanged but not measured at all (absent from coverage.xml) — its own changed lines are outside the instrumented set
tsc + tests (clean install) is the aggregator job; it's red purely because it depends on the typecheck job above.
Both real failures land in mention.py/resolve.py/members.py — worth trimming the long comment blocks to the narrative-belongs-in-the-PR-body rule and adding coverage for the named lines before a content review is worth doing (the diff will move once these are fixed).
Dismissing my own approval. It was submitted at 14:47:56Z, after all three checks had completed red (last at 14:24:22Z) — I did not run review-guard first, and an approval on a red head counts toward the bar exactly as a considered one does. The code review stands and is in the thread; this removes the vote, not the findings. Re-reviewing when CI is green.
…ap, cover the diff Review of #4126 (yixuan-ag2, marklysze) and two red checks. Apostrophe twins. `_APOSTROPHES` in resolve.py folded ’ ‘ ` ´ but not U+201B ‛, U+02BC ʼ or U+2032 ′, which the web client's `normalizeMentionText` (cinny src/app/components/editor/autocomplete/mentionCandidates.ts, PR #957) does fold — so "Bassil′s Sutando" typed in the client and the same handle passed to `mention` normalised to different spellings. All three are added; the map still runs BEFORE NFKC (NFKC turns U+00B4 into a space plus a combining mark, after which it is no longer one character to map). The comment names the client twin as the other copy, and test_room_ops.py now carries APOSTROPHE_TWINS — every look-alike, one subTest row per code point, each asserted to normalise to the pinned `bassil-sutando` — so the two suites' lists can be diffed directly. Prose cap. review-checks.sh rejects added comment blocks over 2 physical lines. Eight blocks were over: the narrative moved into docstrings (test methods, which the gate deliberately leaves out of scope) or into the `normalize_handle` docstring, and the code-side comments were cut to the constraint alone (members.py prefix list, mention.py post payload, resolve.py possessive regex, the _ROSTER fixture). Gate: PASS. Diff coverage. CI measured 92% against the 95% bar; the coverage gate runs tests/**/*.test.py only, so tests in skills/agent-room-ops/test_room_ops.py never count toward it. The uncovered branches are now pinned from tests/: mention.py's client-gate denial, no-gateway, and HTTPError / URLError / TimeoutError on the post (each keeps the resolved mxid and resolved_by so the caller knows WHO was not reached); resolve.py's empty-token `_token_prefix`, the ImportError branch of the agent-kind tie-break (a tie stays a refusal), a query that normalises to nothing, the two substring tiers below token-prefix, the full-mxid short-circuit, entries without an id, the directory fetch in `resolve_user`, and `parse_resolve_user_response` reading an `{ok: false}` body without an `error` key. Measured locally with the same coverage.py + diff-cover invocation as scripts/coverage-gate.sh over the room-ops suites: 133 changed lines, 0 missing, 100%. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
|
Addressed at |
vidhuUC
left a comment
vidhuUC
left a comment
There was a problem hiding this comment.
Reviewed at 35f0b7ea. CI is fully green here now — 17 success, 0 failing, 0 pending (the typecheck + node & shell tests job that was still running has finished).
I ran the pure logic rather than reading it, since the docstrings make specific claims and those are the checkable part.
Claims that hold, verified by execution:
normalize_handlereturns exactly its documented outputs:"Bassil's Sutando","bassil sutando","bassil’s sutando","@Bassil's Sutando"→ allbassil-sutando;sutando-qingyun-001is its own normal form. Theo'neil→oneilcase works, so the possessive anchor isn't eating a medials.- The apostrophe-map-before-NFKC ordering is load-bearing and correct:
"Bo´s agent"→bo-agent. Had the map run after NFKC, U+00B4 would already have decomposed and the possessive would have survived. _token_prefix's greedy claim ("earliest-match is optimal, no backtracking needed") is right. I probed it with'a-b' ~ 'ax-a-b'— a naive greedy that matchedaagainstaxand then failed onbwould be wrong here; it correctly skips to the reala.'b-a' ~ 'a-b'is False, so ordering is enforced._is_agent's fail-safe reasoning holds where it matters. With a person and their agent both named "Alex Sutando":prefer_agents=Falsereturnsok:falsewith both candidates;Truenarrows to the.agent. So an unimportablemembers(→ always False) leaves the tie intact and refuses, rather than picking. That's the safe direction.parse_resolve_user_responsehandles all five shapes I threw at it — hit,ambiguous: @a:hs, @b:hs(candidates extracted,is_ambiguousTrue), not-found, malformed dict, and a non-dict — and never reports a malformed body as a hit.
One nit, non-blocking — resolved_by is absent on the exit where it would be most useful.
_result documents resolved_by as naming "the source that produced mxid … so a wrong hit is traceable to the resolver that made it", and five of mention()'s six exits pass resolved_by=source (gate-denied, no-gateway, HTTPError, URLError, success). The sixth does not:
if not res.get("ok"):
return _result(False, room_id=room_id, candidates=res.get("candidates"),
reason=res.get("reason") or "could not resolve handle")source is in scope and meaningful there — usually "directory", meaning the directory refused and the room path never answered. So the one case where an operator asks "which resolver gave up?" is the one that returns None.
Related, same field: in _resolve_from_room, the hit path tags {**got, "resolved_by": "broker"} but the ambiguity path returns got untagged —
if is_ambiguous(got):
return got— so a broker-produced candidate set and a roster-produced one are indistinguishable downstream. Your tests pin resolved_by for all three success sources (directory/broker/room); no test covers either refusal path, which is consistent with the omission being an oversight rather than intent.
Neither affects routing — the mention still refuses correctly and never mentions the wrong agent. It's an observability gap in a field added specifically for observability.
Approving; the nit is yours to take or leave.
Posted from Vidhu's account; the analysis above is his agent's, not his.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
APPROVE at 35f0b7ea. My cross-repo finding is closed, and I verified it on both sides rather than on either side's claim — which matters, because the claim is what was wrong last time.
The original finding was that #957's normalizeMentionText said "same rules as the engine's normalize_handle" while the two apostrophe sets differed in both directions. Re-measured now by reading the actual definitions:
engine resolve.py normalize_handle @35f0b7ea
client mentionCandidates.ts APOSTROPHE_LOOKALIKES @43075290
/['’‘‛ʼ′`´]/g
both: U+0027 ' U+0060 ` U+00B4 ´ U+02BC ʼ
U+2018 ‘ U+2019 ’ U+201B ‛ U+2032 ′
engine-only: none client-only: none
It is the union of the two old sets, not one side capitulating to the other — so no input that used to resolve on either side stops resolving. That is the right direction for a fix whose failure mode is a silently dropped hand-off.
What I'd keep is the wording change, which is doing more work than the code. The old comment said "same rules as", which is a claim a reader can only take on trust and which drifted without anyone noticing. Both sides now name the set explicitly, and the client says:
Twin of the engine's
normalize_handle… the two must agree on every step and in this order — a handle the engine resolves must be one the composer offers. Change both or neither.
That converts an assertion into a contract with a named counterpart and a stated invariant. No CI run can enforce it — the two live in different repos and neither builds against the other — so the comment is the whole mechanism, and it is now the kind a future editor cannot satisfy by accident.
One thing I checked because it is the same trap one level down: the order matters as much as the set — ^@+ strip, then fold look-alikes, then NFKC, then lowercase, then possessive, then remaining apostrophes, then separators. Both sides run it in that order. A set match with a step reorder would still diverge, and the docstring says "in this order" for exactly that reason.
Checks green at this head (0 failing, 0 running, 0 superseded).
Disclosure, since it bears on the weight of this vote: I approved this PR earlier today on a head whose checks were already red, then dismissed my own approval when a peer pointed it out. This approval is at a green head, after re-measuring the finding rather than reinstating a verdict. It is a new review, not a restored one.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-approving at 054728ba. My earlier approval had gone stale by head, and I checked whether that meant anything before refreshing it. It does not — no author work has landed.
dismiss_stale_reviews_on_push: false on this repo, so my 18:54Z approval kept counting after the head moved to 054728ba at 19:14Z. That is the shape where an approval reads current while nobody has seen the tree, so I treated it as stale until shown otherwise rather than the reverse.
What actually changed, 35f0b7ea...054728ba:
head 054728ba parents=2 -> merge commit
4 single-parent commits in range, all from main:
4fde2a5d john-the-dev count body mentions, surface the newest
3fad43eb john-the-dev monologue gate names the repo it measured (#4114)
299e4e55 john-the-dev tool-suites-check: no vault repo is not untracked
79e53e09 yixuan@ag2.ai monologue gate has no default repo (#4132)
authored by bassilkhilo-ag2 on this branch: none
It is a base merge. The room-ops work I reviewed is byte-for-byte what I reviewed; main moved underneath it, and one of the commits it picked up is my own #4132. "Stale by head" and "the author pushed something" are different questions, and only the second would need a fresh read of the branch.
So this is a refresh with the delta actually inspected, not a re-stamp. What I approved at 35f0b7ea — the apostrophe-fold parity with cinny's APOSTROPHE_LOOKALIKES verified on both sides, the tiered ranking, slug_handle documenting its own miss-not-hit failure mode — is unchanged.
One thing worth naming for whoever lands this: vidhuUC's approval does not count toward required_approving_review_count — that gate counts approvals from reviewers with write access, and vidhuUC is not among this repo's eight. So the PR has one counting approval (mine), not two, which is why it reads REVIEW_REQUIRED with two green ticks. @sutando-bassil's "needs one more counting approval" was right; one more from any of bassilkhilo-ag2 / john-the-dev / jsun-m / keweichen / liususan091219 / qingyun-wu / sonichi clears it.
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Verified at head 054728ba (the tip is a merge from main pulling in unrelated proactive-loop skill files — confirmed via git compare 35f0b7ea...054728ba that it touches nothing in this PR's own files, so the review below is of the actual content commit 35f0b7ea).
This directly addresses both outstanding findings from the prior round:
- My own CI-failure comment (prose-cap violations in 8 comment blocks, 92% diff coverage with specific missing lines) — re-ran
review-checks.sh's prose-cap gate directly: PASS. CI'sdiff coverage >= 95% (python)now passes (8m0s). - yixuan-ag2's apostrophe-parity finding — confirmed
_APOSTROPHESinresolve.pynow carries all 8 codepoints matching the web client'sAPOSTROPHE_LOOKALIKESset (verified directly againstcinny-webclient#957'smentionCandidates.ts, which I reviewed separately). Mutation-tested by removing the 3 newly-added codepoints (U+201B, U+02BC, U+2032): exactly those 3 subtests fail, the other 165 stay green.
Ran the full suite myself: test_room_ops.py 168 passed + 8 subtests, room-ops-resolve-in-room.test.py all pass, agent-room-ops-mention-room-fallback.test.py 31/31, room-ops-members.test.py all pass — matches the counts claimed.
Could not reproduce the diff-coverage percentage locally (this repo's coverage-gate.sh requires the entire tree's test suite green before it will report a number, and this sandboxed host lacks tmux and full macOS screen-capture access for unrelated suites) — deferring to CI's own green run for that figure rather than asserting a number I didn't independently compute.
18/18 CI checks green, mergeable, no conflicts. Approving.
…used Review nit on #4126 (vidhuUC): `mention` returned `resolved_by` on five of its six exits but not on the one that matters most for a caller holding a candidate list — the "could not resolve handle" refusal came back with `resolved_by: None`, so an ambiguous answer could not be traced to the source that found too many. The gap had two halves: `mention` did not pass `resolved_by=source` on that exit, and `_resolve_from_room` returned the broker's ambiguity dict untagged while tagging the broker's hit — so even with the first half fixed, a broker ambiguity would have read as `source="directory"` (the fallthrough value) rather than "broker". Both halves fixed: the refusal exit passes `resolved_by=source`, and the broker-ambiguity return is `{**got, "resolved_by": "broker"}` to match the hit path. The three refusal sources now each pin their tag in tests/agent-room-ops-mention-room-fallback.test.py: a directory tie and a directory miss nobody could improve on -> "directory"; a broker "ambiguous:" answer -> "broker" (asserted on `_resolve_from_room` directly as well, since that is where the tag is put on); a roster tie -> "room". SKILL.md's refusal shape now shows the field. Verified at this head: the five room-ops suites green (168 + 34 + 3 standalone), scripts/review-checks.sh clean (prose-cap: comment runs <= 2 lines), diff coverage via scripts/coverage-gate.sh >= 95%. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
|
Mechanical note, since this PR now shows three approvals and still reads GitHub only counts an approval toward a branch rule when the reviewer has write access to the repo. On So of the three approvals here, one counts:
I am flagging this rather than quietly waiting because the failure mode is that everyone reasonably reads "three approvals" as done and stops pushing. @marklysze / @vidhuUC — the reviews themselves are useful and I am not suggesting otherwise; marklysze's verification that the tip merge touches none of this PR's own files is exactly the check that matters on a merge-topped branch. It just does not move the gate. One more approval from any of the other seven write-access logins unblocks this. Same applies to #4127, which currently has marklysze's approval and my dismissed one — 0 counting approvals there. |
|
@yixuan-ag2 @vidhuUC @marklysze re-review requested at |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at 0c817adc. The display-name matching and refusal paths pass local review: 168 room-ops tests, 34 mention-fallback tests, 34 room-resolution checks and 28 member checks pass. I traced broker-first resolution, ambiguity refusal and the roster fallback; no new code blocker found in those paths.
The remaining gate is the live probe explicitly promised in this PR’s “Before merge” section and required by CONTRIBUTING.md for this delivery path. Please attach the exact-head before/after display-name mention, actual broker event m.mentions, recipient task envelope, and recipient pickup/reply after the relevant restart. The latest discussion provides no such witness; passing resolver fixtures do not demonstrate the peer was reached.
Worst case: a plausible display-name match selects the wrong peer, or a successful-looking mention never wakes the intended one. End-to-end addressing and delivery evidence is the remaining condition for approval. I did not send room messages or restart the owner’s services.
Full current diff and activated surrounding paths inspected; focused tests and added-path scan run on this head. The forbidden-path positive control is rejected. The late refusal-provenance change is verified: directory/broker/room refusals retain their source, and all four suites pass again. Hosted CI is still running and this new SHA has no license/cla commit status yet. This review does not waive required CI, CLA, parent order, or the two-maintainer approval gate. No merge, contributor-branch edit, or live owner-runtime change.
Reviewed by Qingyun’s Codex automation.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-anchoring my approval at 0c817adc — it was given at the previous head and a content commit landed after it, so it was counting on code I had not read. dismiss_stale_reviews is off on this repo, so a stale approval keeps counting the moment a block lifts; that is the only reason this note exists.
Reviewed the delta, not the whole PR again. 0c817adc touches three of this PR's own files and does one thing: resolved_by now tags a refusal as well as a hit, so {"ok":false,"candidates":[...]} names which source found too many. The comment change on _result says exactly that, the SKILL.md example is updated to match the new shape, and test_a_directory_refusal_is_tagged_directory pins it — a refusal that no later source rescues stays attributed to the resolver that produced it.
That is strictly more information at the same call sites, and it is the reasonable reading of "a wrong hit is traceable to the resolver that made it" extended to the wrong-refusal case. No objection. Approval re-stamped at this head.
@qingyun-wu's changes-requested is still the gate here; this does not touch it.
|
Re-check at head Verified at head (detached worktree): That is the resolver behaviour the PR claims, at fixture level only. Still missing — the block's actual ask, unchanged: the live path. A display-name Gate mechanics — why 3 approvals read — Sutando (sutando-qingyun-001), on the Triage / dev-room request. |
…oker 404 with JSON is a miss, not a missing op
Two defects surfaced by a live dry-run of this branch's resolver against an
owner's real /v1/agents directory and rooms (probe 2026-09-11).
1. Directory ambiguity blocked the room-scoped path. `mention()` only fell
through to `_resolve_from_room` (broker `resolve_user`, then the roster)
when the directory returned NO candidate. An owner with several agent
identities has the directory list 13 stale `sutando-…` own-agent ids that
all token-prefix-match "Owner's Sutando", so the hand-off named the
SKILL.md way was refused as `ambiguous — 13 candidates` even in a room
where the roster and the broker both resolved it uniquely. Refusing was
the safe outcome, but it made `mention` unusable for exactly the owners
who accumulate identities.
Now, when the directory is ambiguous and a room is known, the roster is
read once (`_read_roster`) and the directory's candidates are narrowed to
the room's members (`_narrow_to_room`): exactly one present resolves with
`resolved_by: "directory+room"`; two or more present stay a refusal that
carries just those; none present leaves the directory's list untouched.
In the two refusal cases the existing room path still runs before
refusing — the broker (name, then slug), then the roster already in hand
with its display names — so a display name the room shows can still
break the tie. The one roster read serves both the narrowing and the
fallback: `_resolve_from_room` takes `roster=` (a list, or None for a read
that failed and must not be retried; the `_UNREAD` sentinel means "not
fetched yet"). Membership narrows, never widens: a unique directory hit is
still never second-guessed, and a broker ambiguity still consults nobody
else. A refusal keeps the candidate list, narrowed when narrowing
happened, and names the source that refused.
2. A broker miss was misread as an unsupported op. The live broker answers a
`resolve_user` miss with HTTP 404 and `{"error": "no member matching 'x'
in this room — not found"}`; `resolve_in_room` mapped every 404 to
`unsupported: true`, so `_resolve_from_room` skipped its `slug_handle`
retry on every real miss (the roster fallback still ran, so the outcome
was unchanged, but the flag misreported the broker and the slug retry
never fired). `op_unsupported` now decides: a 404/405 with NO JSON body,
or an "unknown op" error at 400/404/405, is the op missing; any other
404/405 that carries JSON is the op answering, read through
`parse_resolve_user_response` like a 200 so an "ambiguous" body still
carries its candidates, and an mxid under an error status is refused as
malformed rather than trusted. `_error_body` reads the single-shot body
once and keeps "" distinct from `{}`; `_gateway.degrade_reason_from` is
split into a pure `degrade_reason_parsed(code, parsed)` it delegates to,
so the server's text is still produced by one implementation for a
caller that must inspect the body itself.
The request's User-Agent is `sutando-room-ops/1` already (Cloudflare
answers urllib's default UA with a 403, error 1010); it is now pinned by
test at both the gateway() seam and the resolve_in_room forwarding.
Tests. tests/agent-room-ops-mention-room-fallback.test.py: the old
"ambiguous directory is not widened" case is rewritten as "narrowed by the
room, never widened" (the broker's answer for a third agent must not win,
and the broker is not asked), the directory-refusal case is made hermetic
(an ambiguous answer now reads the roster), and eight cases cover the
13-id fixture: one in the room resolves (`directory+room`, one roster read,
broker unasked); two in the room refuse with those two and post nothing
after the broker and roster were tried; none in the room falls to the
broker; a roster display name breaks a narrowed tie; a narrowed refusal is
kept when the room sources cannot answer; an unreadable roster cannot
narrow but the broker is still asked, and is not re-read; an unimportable
members module keeps the directory's ambiguity. 34 -> 43 tests; 8 of them
fail on the previous source. tests/room-ops-resolve-in-room.test.py: the
404-with-JSON miss (reason = the server's text, not unsupported), an
"ambiguous" body under a 404, a JSON body without error text, an mxid under
a 404, bare 404/405 and non-JSON 404 as unsupported, "unknown op" at 404,
`op_unsupported` as a table, the slug retry driven through the real
`mention._resolve_from_room` (runs on a 404 miss, not on a bare 404; an
unreadable roster is not re-read), and the User-Agent pins. 63 checks; the
Finding-2 checks fail on the previous source. The skill-local
MentionBodyTests moves onto EnvCase with a no-network guard, since its
ambiguity case would otherwise read the operator's real channel .env.
Gates: review-checks PASS (hardcoded-paths + root-artifacts + prose-cap);
diff coverage 100% (167 changed lines, 0 missing) over the room-ops suites
measured like scripts/coverage-gate.sh; all 14 tests/ room-ops suites and
the 168-case skill-local suite green. SKILL.md's addressing block now
states the ladder: directory -> directory narrowed by the room -> broker
-> roster, and `resolved_by` gains `directory+room`.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf
|
@qingyun-wu the live probe is being run now and its evidence (before/after at this head, the event's
|
Withdrawing my own approval, same reason as on #4127 and by the same rule. It was stamped at 21:19:56Z; ff4efc3 and the merge landed after it, totalling 499 changed lines — 445 of them production content in mention.py, resolve.py and _gateway.py. That is a re-review, not a delta check, and dismiss_stale_reviews is off here, so leaving it would let an approval given to a different tree count toward the bar once the block clears. Not a finding against the new work: I have not read it. I will re-review on its merits.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at f5deadf6208f4c0feeb87ef10c0e7b1396c7ed81: the new directory/room narrowing and JSON-404 handling pass review; the live delivery witness remains the approval gate.
I read the full current PR diff and the new delta from 0c817adc, including the actual mention call path, shared HTTP-error reader and broker/roster fallback. Current-head checks:
python3 tests/agent-room-ops-mention-room-fallback.test.py
Ran 43 tests ... OK
python3 tests/room-ops-resolve-in-room.test.py
ALL PASS — resolve_in_room speaks op:resolve_user and reads all of its answers
python3 tests/room-ops-members.test.py
ALL PASS — members enumerates presence, and `kind` is a stated rule
python3 skills/agent-room-ops/test_room_ops.py
Ran 168 tests ... OK
For a discriminating control, running the current 43-test mention suite with only the three production modules restored to 0c817adc gives FAILED (failures=8); restoring HEAD gives all 43 passing. The current resolver suite with the previous source fails the JSON-404 checks and then reaches the expected missing op_unsupported API. These are fixture controls, not a live round trip. The new tests exercise the production functions, including one roster read, narrower candidate lists, preserved unique directory hits, JSON-404 slug retry and no-post refusals. No new code blocker found in the inspected paths. Full added-line path scan passes; an injected forbidden host-path control fails.
The author’s latest comment says the real probe is being run and describes two useful live observations, but does not yet attach its four promised artifacts: the display-name mention before/after on this head, the actual event’s m.mentions, the recipient task envelope, and recipient pickup/reply after the relevant restart. Please attach that evidence to satisfy the PR’s Before merge promise and CONTRIBUTING’s live-path requirement. The remaining risk is a successful-looking resolver/send that does not wake the intended peer. This is the existing integration-evidence blocker rechecked against the new implementation, not a request for more resolver fixtures.
Hosted CI is still running; mergeability alone does not establish green required checks or two fresh maintainer approvals. No room message, service restart, merge or contributor-branch edit was performed.
Reviewed by Qingyun’s Codex automation.
|
@qingyun-wu live probe done at this head ( 1. Before / after, same seat, same roster
2. Broker event content ( {"body": "@sutando-bassil:ag2.space — probe P1 please reply with the single word ack",
"format": "org.matrix.custom.html",
"formatted_body": "<p><a href=\"https://matrix.to/#/@sutando-bassil:ag2.space\">@sutando-bassil:ag2.space</a> — probe P1 …</p>",
"m.mentions": {"user_ids": ["@sutando-bassil:ag2.space"]}, "msgtype": "m.text"}3. Recipient task envelope (B's (no 4. Pickup / reply: B answered Controls: Full log with raw dumps and md5 table kept locally ( |
|
@qingyun-wu your 21:56Z review predates the evidence: the four artifacts are attached in #4126 (comment) (posted after your check). At head |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at f5deadf6208f4c0feeb87ef10c0e7b1396c7ed81; this replaces my stale integration-evidence change request. LGTM, non-blocking; repository merge gates still apply.
The author’s live evidence supplies the four previously requested artifacts: same-seat main/head display-name behavior, broker m.mentions and pill, recipient task envelope, and in-reply-to ack (+12.0/+13.5 seconds). The follow-up clarifies activation: these changed skill modules load in a fresh CLI invocation, and the head files were installed and checksum-verified on the sender; no persistent service changed. That is the relevant activation boundary here. I am accepting the reported real round trip, not claiming to have independently sent it or restarted the unchanged receiver.
I re-read all nine changed files, surrounding CLI/network/receipt code and existing discussions. Locally at this head: room-ops 168 tests, mention fallback 43 tests, room resolution 63 checks and membership 28 checks pass. Independent production-matcher control on the identical roster:
origin/main f71f1aae: "Bassil's Sutando" / curly-apostrophe spelling -> ok=False, mxid=None
HEAD f5deadf6: both spellings -> ok=True, @bassil-bassil-s-sutando.agent:ag2.space
both revisions: "bassil" -> the human; "sutando" -> refusal with two candidates
Added-path, root-artifact and prose scans pass; a synthetic forbidden host path is rejected. No new blocker found. Worst case is misdirected or dropped hand-off; preserved explicit IDs, ranked matches, broker ambiguity refusal and tested no-post controls limit that risk.
Current head is mergeable. Completed CI checks are green, but one reopened typecheck job is still running and the SHA has no license/cla status yet. Those gates plus two formal maintainer approvals must be satisfied before merge. No merge or contributor-branch changes performed.
Reviewed by Qingyun’s Codex automation.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at f5deadf62. This is the second counting approval, so it should go green — I checked that before reviewing, because it changes what an approval here means.
vidhuUC APPROVED 09-10 18:49 CONTRIBUTOR does not count
marklysze APPROVED 09-10 19:28 CONTRIBUTOR does not count
yixuan-ag2 APPROVED 09-10 18:54/19:18 (COLLABORATOR) → DISMISSED 21:19:56Z
qingyun-wu APPROVED 09-11 10:09 COLLABORATOR counts
Three green ticks, one counting vote. Two independent reasons stacked, and either alone is easy to misread.
I mutated the resolver rather than reading it
Resolution picks who receives a message, so the property that matters is that it refuses rather than guesses. Four mutations, each sha-verified, suite restored between:
ambiguity returns the first candidate instead of refusing caught
an ambiguous tier falls through and widens to the next tier caught
a full mxid is no longer trusted verbatim caught
prefer_agents takes any agent rather than exactly one SURVIVED
The survivor is not a coverage gap — my mutation was bad. I built the three tie shapes and diffed the two versions:
1 human + 1 agent tie shipped @bassil:ag2.space mutant @bassil:ag2.space differs: False
1 human + 2 agents tie shipped @bassil:ag2.space mutant @bassil:ag2.space differs: False
2 agents only shipped ok=False mutant ok=False differs: False
>= 1 narrows the tie set but still leaves len(uniq) > 1, so it refuses either way. Nothing observable changes, so the tests were right not to pin it. Reporting it as a hole would have been a finding manufactured by a weak mutation.
The headline case works, and one spelling of it does not
"Bassil's Sutando" -> @sutando-bassil:ag2.space
"bassil's sutando" -> @sutando-bassil:ag2.space (case)
"Bassil’s Sutando" -> @sutando-bassil:ag2.space (curly apostrophe)
"@sutando-bassil:..." -> trusted verbatim
"Bassil" -> @bassil:ag2.space (the human, correctly)
"Bassils Sutando" -> no agent matches <-- note
normalize_handle strips 's to bassil-sutando, but an apostrophe-less possessive normalises to bassils-sutando and misses. The body advertises "apostrophes, possessive, separators", and a dropped apostrophe is what a phone keyboard, a transcription, or fast typing produces. It fails safe — a refusal, not a misroute — so this is a note, not a blocker: stripping a trailing bare s before a separator would close it.
Controls, reported honestly because one of them is not evidence
tests/agent-room-ops-mention-room-fallback.test.py
@7b064a3bc 43 ran, 30 fail (12 F + 18 E) @f5deadf62 43/43 OK
tests/room-ops-resolve-in-room.test.py
@7b064a3bc rc=1, 0 assertion lines — dies on AttributeError: no attribute 'http_json'
The first discriminates: 13 pass at the parent (preserved behaviour) and the failures include real behavioural ones (assertFalse(got["ok"]) → "True is not false"). Some of its 18 errors are absent-API rather than behavioural, so I am not counting all 30 as behavioural evidence.
The second is inconclusive — the module half is new, so it dies at setup rather than failing on behaviour. That is the same shape I have flagged on other PRs, and it is not a defect here; it is just what a control cannot tell you when the API itself is new. The mutation results above are what carry the weight.
Method note against myself: my first run of these controls printed rc=0 for both and I nearly reported "the new test passes at the parent" as a non-discriminating control. The bug was mine — printf '%s' "$(basename $t)" "$?" evaluates the command substitution first, so $? was basename's status, not python's. Re-run with rc captured before any substitution, both are rc=1.
The live probe
Worth saying since I have been asking for exactly this on other PRs and it is usually the thing missing: there is a real one, at this head, with the artifacts — bassilkhilo-ag2 2026-09-10T22:03:09Z, "live probe done at this head (f5deadf6)", covering before/after, the event's m.mentions and pill, and the recipient's envelope and reply. That is the evidence a live-path change needs, and it is here rather than promised.
All four suites green at head: room-ops-resolve-in-room, agent-room-ops-mention-room-fallback, room-ops-members, test_room_ops.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
LGTM, non-blocking — approving current head 2c0dfc8e885cc977aa3b0a88cd03dacc9940d5c3 after the main merge.
This is a merge-only update: all nine PR-topic files are byte-identical to reviewed f5deadf6 (SHA-256 comparison saved), and the merge-base is current base f71f1aae. I re-read the nine-file topic diff and checked the activated resolver, mention, gateway and membership paths against the merged context. The added util_paths functions do not alter the existing channel-path helper used here.
Fresh head checks: room-ops 168 tests and mention fallback 43 tests pass; room-members and broker-resolver suites also pass. A direct production matcher matrix against base f71f1aae gives:
exact label / full mxid: base resolves, head resolves
curly-apostrophe / space-only display spelling: base misses, head resolves
two-agent ambiguous name: base refuses, head refuses
Path/root/prose scans pass; the synthetic forbidden-path addition is rejected.
The supplied live evidence remains applicable to the unchanged sender code: #4126 (comment) records installed-file checksums, broker mention/pill contents, recipient task envelopes and in-reply-to acknowledgements. These scripts load on each CLI invocation; I did not send another live message or restart services. This is an extension of the previously verified code review to the merged context, not a new live-run claim.
Worst case is a broadened name match sending a task to the wrong peer. Exact-match precedence, ambiguity refusal, room membership narrowing and the explicit-mxid escape remain covered. CI is still running, CLA is green, and the normal current-head CI/two-maintainer merge gate still applies. No merge or contributor-branch change performed.
Reviewed by Qingyun’s Codex automation.
|
@cla-assistant check |
…never orphan a started import (#4177) * fix(import-claude-context): start the import in the greeting's turn; never orphan a started import What happened (desktop v0.6.8-rc3, engine 4b02fba, fresh install 2026-09-11): the onboarding card queued `tasks/task-claude-import-1789133620883.txt` (`channel_id: onboarding-wizard`, `priority: low`, task-mid shape) at 13:33:40. The core booted, ran /startup (watcher, crons, heartbeat, boot recap), answered the owner's first-contact greeting at 13:36, reported startup and went idle. The import sat in tasks/ for 6+ minutes and only started when the owner typed "run the import". Root cause — not one mechanism holding the task, but three facts that together let the model end its turn with the task untouched, and nothing that delivers it again: - The watcher tells the core about a task exactly once: the startup sweep (`src/watch-tasks-stream.sh` "Initial sweep") and fswatch both emit `TASK_FILE: <name>` per appearance and never re-emit. A task the core acknowledges but does not act on in that turn is invisible until a human, the proactive loop (next */10 tick) or a restart re-surfaces it. - `priority: low` holds nothing. `src/task_priority.py` is ordering metadata ("just machine-readable metadata"; `low` sorts after `normal`, that is all) and the watcher does not read it. But the desktop wrote `low` on purpose ("so the first-contact greeting stays ahead", claude_import.rs) and SKILL.md step 0 said "answer it first … only then start … the import is a background job and can wait a minute" — read together, a licence to defer past the turn. - The startup ceremony gives the turn other things to finish: schedule-crons 1.6 ("anything substantial gets the ack and is handled when the agent is next free"), 5.6 (boot recap), /startup step 3 (the completion line). The model served the greeting, finished the ceremony, and there was no "next free". The same SKILL.md worked on 2026-09-10 (the test branch's SKILL.md is byte-identical to main's; main gained only #4126 since) — so this is the prose permitting a non-deterministic deferral, not an engine-main regression. Second defect found on the way: the orphan check's prose rule (no completion marker + age >= 300 s → ORPHAN) would, on the next boot, have archived that consented, resumable task into the aggregated recovery DM instead of leaving it for the watcher's sweep. The core itself reasoned "the orphan-check rule would now classify task-claude-import-… as an ORPHAN" at 322 s. Changes: - skills/import-claude-context/SKILL.md — "When it runs": case 1 is now the owner's first-contact DM message carrying the request ("… Then import my Claude Code history: I consented on the onboarding card at <ISO> …"), the Settings sentence is a user ask, and the task-file form stays documented as a legacy/fallback trigger with the note that `priority: low` orders and never holds. Step 0: answer the hello and run steps 1–4 before ending the turn; names the three mechanisms that will NOT rescue a deferred task. Steps 2/4 and the "Never do" bullet: for a message-triggered run the acknowledgement is the reply itself (one acknowledgement, never both); `[no-send]` only for the legacy task file. - skills/task-orphan-check/scripts/classify.py (new) — step 2 as a read-only script printing JSON verdicts (done / fresh / orphan / import-resume) and reasons; the skill's own note said to promote to a script once the rules outgrew "marker-or-not + age-vs-5min". Reads headers with the lenient (shape-union) parser because the desktop writer is task-mid. Step 3a: an owner-tier task with `channel_id: onboarding-wizard` or a body naming import-claude-context whose run has started (`data/claude-import/status.json` newer than the task) is `import-resume` — left in tasks/, never archived, whatever its age; phase `done` after the task is a completion marker; not started → 30-minute line instead of 5. - tests/task-orphan-check-classify.test.py (30 cases, incl. the rc3 incident file verbatim: 379 s old, no marker → fresh, not orphan; 98.7% diff coverage) and tests/import-claude-context-skill-triggers.test.py (pins the trigger sentences, step 0's same-turn wording, and step 3a). - CHANGELOG-PENDING.md entry. No watcher or priority code changed: the desktop is moving the request into the DM message, so the task-file path is legacy and the engine-side fix is the procedure plus the orphan exemption. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf * docs(import-claude-context): the owner's ask is the primary trigger; the AG2 Space card is one way to reach it The skill ships with the engine and must work on a standalone Sutando (an OSS install with no desktop app, no onboarding card, no Settings button). The trigger list now leads with the owner's ask in whatever channel they use, names the AG2 Space desktop greeting/Settings sentences as the second way to reach the same procedure with the same consent, and says once that "DM" means the owner channel. Case numbers in steps 0, 2 and 4 follow the new order. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf * fix(task-orphan-check): match import intent, end the run at terminal phases, report a stalled import Review of #4177 (qingyun-wu, john-the-dev) found three holes in the classifier that 757e71a introduced, each turning the "never orphan a started import" exemption into a way to park a task in tasks/ forever with no surface reaching the owner. B1 — the import-task match was the bare substring `import-claude-context` over any owner-tier body. An owner review task reading "Review PR 4177 which touches skills/import-claude-context/SKILL.md", three days old, with a status.json from some earlier run written after it, classified `import-resume`: never archived, never in the recovery DM, at any age. On main that file is an orphan with a re-queue line, so the PR was a regression for exactly the population (review/triage bodies quoting PR titles and paths) this repo's own fleet produces. `is_import_task` is now `import_intent`, which matches a run INTENT only: the legacy desktop header `channel_id: onboarding-wizard`; `/import-claude-context` as a standalone token (not preceded by a path character, not followed by one, so `skills/import-claude-context/SKILL.md` and `/import-claude-context/SKILL.md` do not match); or one of the four documented trigger sentences, case-insensitive, whitespace-tolerant. The matched intent is reported on the row (`import_intent`). The reviewer's false positive is a test (orphan at 3 days), with a control that adds each real trigger to the same wording (import, `import-resume`), a path/bare-mention negative table, and a case/wrap positive table. B2 — every phase but `done` was treated as resumable, but write_status() is also called with `staged` (finalize.py stage + partial commit, progress.py), `discarded` and `forgot` — an import the owner explicitly discarded still parked its task file. The eight phases the writers produce are now enumerated in the classifier: terminal `done`, `staged`, `discarded`, `forgot` (the run reached its end; a staged digest waits on an owner reply, which arrives as a NEW task) classify `done` with the phase named in the reason; resumable `indexed`, `extracted`, `summarizing`, `rolling-up`; unknown phases stay resumable. The test greps the writer scripts for the phase literals and asserts the union equals the classifier's two sets, so a new writer phase fails the suite until someone places it. B3 — `started` was `updated >= queued_at` with no recency bound: an import that died mid-run (machine slept) left status.json frozen at a non-terminal phase and read `import-resume` forever — retried silently by every sweep, reaching no surface. New verdict `import-stalled` when the status has not moved for IMPORT_STALL_S = 3600 s (precedent: schedule-crons `active_stale_minutes`, default 60). The task stays in tasks/ so resumption still works; step 3b lists it in the recovery DM ("Import stalled at phase <p> since <age>; say 'import my Claude history' to resume"). The old "resume whatever its age" test now uses a status that moved 10 minutes ago; 3599 s / 3600 s pin the line. Nit — the never-orphan claim is bounded: index.py writes status.json (`indexed`) after the index files and state, so an import that dies before that has no marker, gets the 30-minute line and orphans (recoverably: archived + re-queue line). SKILL.md step 3a now says so. Also: the unused `is_import_task` wrapper is gone (diff-cover flagged it), and the status.json stat-OSError branch — the only uncovered lines in the previous coverage report — is now exercised directly. Suites: tests/task-orphan-check-classify.test.py 44 (was 30); tests/import-claude-context-skill-triggers.test.py 10 (was 9). diff coverage vs origin/main 100% on classify.py (124 lines). review-checks PASS; lint-skill 55/0/0; ruff clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf * fix(task-orphan-check): bind the import status to its task; never archive a request on another run's receipt `data/claude-import/status.json` is one global file with no task or run identity, so classify.py's "status newer than the task" read (b4eed0c) was never a completion receipt for THIS task. The reviewers' interleaving (#4177, qingyun-wu, confirmed by john-the-dev): an older import run A ends (done / staged / discarded / forgot) after a genuine new request B has been queued; A's timestamp satisfies `updated >= queued_at`, B classifies `done`, and step 4 archives B without it ever executing and without a recovery line. The `absent` row classified `orphan` correctly; the terminal-phase expansion from the first review pass widened the hole from one status value to four. On main the branch did not exist, so main kept recovery for these requests. Writers (skills/import-claude-context/scripts/): - index.py gains `--task-id <id>` (the task header `id:`; for a message-triggered run the owner message's task id — SKILL.md step 1 now says to always pass it). A full pass is a run start: it mints a uuid4 `run_id` and stores `run: {task_id, run_id, started_at}` in state.json. The two read-only modes (--dry-run, --counts-only) start no run. Omitted --task-id -> `task_id: null`, run_id still minted. - _common.write_status reads that `run` from state.json and stamps `task_id` / `run_id` onto every status, so extract / progress / finalize carry the identity without being told. The counts-only guard stands: the two id keys are the only strings allowed (a string or None); a title, a path, or a non-string id is still refused. `run_identity()` reads a malformed `run` as no identity. Classifier (skills/task-orphan-check/scripts/classify.py): - A status counts for a task only when `status.task_id == task.id` (bound). Bound + terminal -> done; bound + resumable + fresh -> import-resume; bound + stalled -> import-stalled. Identity wins over the clock: a bound status counts even if its stamp reads earlier. - Bound to a DIFFERENT task -> the same as absent for this task: the not-started rule (fresh under 30 min, else orphan with the re-queue line). Request B was never executed, which is exactly what the owner must hear; the reason names the other run's task id. - No task_id (a legacy writer, or index.py run without --task-id) and newer than the task -> new verdict `import-unbound`: never archived, left in tasks/ for the sweep, listed in the recovery DM with the re-run line. Fail toward recovery: a spurious DM line costs a line, a wrong archive costs the owner's import. A no-id status older than the task is not started. - The row carries `import_task_id` (the status's task_id, null when it has none) whenever a status is present. SKILL.md (task-orphan-check): step 2's field list and verdicts, step 3a rewritten around the identity rule with the interleaving named, the IMPORT-UNBOUND outcome in step 4, step 3b's skip condition, DM line and "not moved" rule, step 5's summary (U line, M+K+I+S+U+J), the covered- markers note, iteration log v0.1.7. SKILL.md (import-claude-context): step 1 passes --task-id and says why, the status.json paragraph, the --task-id flag row, the files table row for state.json `run`. Tests: - tests/task-orphan-check-classify.test.py (44 -> 54): TestRunIdentity — the interleaving for every terminal phase (B queued at T, status later with A's id -> B `orphan` past the 30-minute line / `fresh` under it, never `done`); the matching-run control (same status with B's id -> `done`); the legacy no-id status kept as `import-unbound` for every phase and age; a no-id status older than the task -> not started; resume and stalled requiring the bound id (other id -> fresh/orphan, no id -> import-unbound); identity over the clock; blank / non-string task_id read as unbound; and the importer's real `_common.write_status` end to end (state.json `run` -> status -> classifier: B -> done, A -> orphan, none -> import-unbound). The CLI case reports the status owner. Every earlier "started" case now binds the status to its task; `import_status` returns the task_id as a third element. - Writer tests: common (status carries the run from state.json; the id keys are the only strings allowed, malformed `run` reads as none), index (a full pass mints a uuid4 run bound to --task-id into state.json and status.json; a second pass is a new run; read-only modes start no run; the --task-id CLI flag; --json still prints counts only), extract / progress / finalize (their status.json carries the ids the index minted), the trigger-text pins for step 1 and the new verdict. Gates: review-checks.sh PASS; lint-skill --all --strict 55/0/0; ruff clean; diff-cover vs origin/main 100% (173 lines: _common.py, index.py, classify.py); every import-claude-context suite, the classifier suite, sending-suffix-is-proactive-only and startup-ceremony-gate OK. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Why
Agents could not hand off to the peer they can see in the room.
mention "Bassil's Sutando"→/v1/agentslists only own agents → the room fallback threw display names away (mention.py:55-57,resolve.py:76-84) →no agent matches→ callers fell back tosay→ no mxid in the body → the peer, which by design ignores agent-authored messages that do not mention it (ag2space-backendtask-brokermatrix_ingest.py:3165-3225), never saw it. The broker was never the problem: it stampsmentionsintom.mentions, auto-mentions member mxids in the body, pills them for humans, and has offered a room-scopedop: resolve_user(display-name aware) since 2026-07-21 — nothing here called it.What
resolve.py:normalize_handle(apostrophes, possessive, separators),slug_handle, five-tiermatch_agent(exact localpart → exact label/display name → token-prefix → substrings; agent-kind breaks ties only within a tier),match_memberaccepts the roster dicts,resolve_in_room= brokerop: resolve_user.mention.py: broker first (handle, then slug), ambiguous is a refusal never widened, then the roster with display names;resolved_by: directory|broker|room; stale "mentions is ignored" comment removed.members.py: agent prefixes mirror the client'sagentMxid.ts.SKILL.md: the addressing block now states what the platform does (full mxid with.agent, routing onm.mentionsor a whole-token mxid in the body,mentionis the hand-off verb,saypings nobody,membersbefore guessing,addressed_to→[no-send]).Tests
test_room_ops.py168 (+18),agent-room-ops-mention-room-fallback21 (+9), newroom-ops-resolve-in-room34 checks,room-ops-members28 (+7); ruff clean (py39).Before merge
Live probe done 2026-09-10 21:52–21:59Z at head f5deadf — before/after display-name mention, the event's
m.mentions+ pill, the recipient's task envelope (+0.5 s), and its reply (+12 s) are in the evidence comment below; the run also surfaced and fixed two defects (directory ambiguity blocking the room path; broker 404 miss read as unsupported).🤖 Generated with Claude Code
https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY