[qingyun-wu]

Stacked on #4492 (fix/room-doc-board-reconcile) — same files. Merge #4492 first.

Why this half matters

The UI half of attribution shows a person colours. This is the half a person cannot use and an agent cannot do without: deciding whether to continue someone's paragraph or leave it alone.

"A human wrote this" and "another agent wrote this" call for different behaviour — the owner's words were that agents will want to weight input from humans, the owner, or a colleague differently. Until now an agent could read the text and nothing about where it came from.

doc.authors            -> {client id: {mxid, kind, owner_mxid}}
doc.wrote(client_id)   -> that row, or None when unknown or DISPUTED
room_doc.py --with-authors read '!room:server'

Read-only, deliberately

The map is the server's, written from the authenticated connection. A client writing to it would be claiming an identity rather than reporting one — so this class has no writer for it at all, and the test suite has nothing to test on that side because there is nothing there.

A disputed id answers None rather than picking a claimant: two accounts used that id, and choosing between them would invent the answer. doc.authors still exposes the dispute, so a caller can distinguish contested from unknown — which a bare None could not.

Output shape

In plain output the authors print above the text, because an agent decides whether to trust or edit the body by who produced it — underneath, it would be read after the decision was already made. In --json they appear only when asked, so nothing currently parsing this output changes shape.

Dependency

Needs ag2space-backend #1256 deployed before the map has anything in it. Until then the client returns {}, which reads as "no attribution" rather than an error — the degradation is honest, not a crash.

Evidence

all 11 room-doc suites          ok
discover-python-tests.sh        finds tests/room-doc-authors.test.py
ci-covers-every-python-test     OK
review-checks.sh --diff         PASS

mutant	result
disputed id resolved to a claimant	CAUGHT
int/str key mismatch	CAUGHT
authors leak into JSON unrequested	CAUGHT
authors printed below the text	CAUGHT
unknown client id returns {} instead of None	survived

The survivor is equivalent, not a defect: .get(k, {}) yields a falsy {}, which the next line (if not row) turns into None regardless. Recording it rather than quietly dropping it from the table.

— Mars

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• skills/room-doc/scripts/room_doc.py (100%)
• skills/room-doc/scripts/room_doc_client.py (95.7%): Missing lines 148-149

Summary

• Total: 67 lines
• Missing: 2 lines
• Coverage: 97%

skills/room-doc/scripts/room_doc_client.py

Lines 144-153

  144         to it would be claiming an identity rather than reporting one.
  145         """
  146         try:
  147             authors = self._doc.get(AUTHORS_KEY, type=Map)
! 148         except Exception:  # noqa: BLE001 - an absent map is simply no attribution
! 149             return {}
  150         return {str(k): dict(v) for k, v in authors.items() if isinstance(v, dict)}
  151 
  152     def wrote(self, client_id: int | str) -> dict | None:
  153         """The author behind one client id, or None when unknown or disputed.

---

[yixuan-ag2]

Reviewed at head 956aeb0b8b6a. The room-doc work itself reads well and I verified the hard part of it against the backend side — detail at the bottom. One blocker, and it is unrelated to the feature: this PR deletes .coveragerc, and that is what is turning the suite red.

Blocker — the deleted .coveragerc breaks every instrumented test

The diff removes .coveragerc in full (@@ -1,62 +0,0 @@), and nothing replaces it — the other 7 files are room-doc sources and tests, no pyproject.toml or setup.cfg entry.

It is an authored deletion, not a rebase artifact:

merge base 85fdc0aa0  ->  .coveragerc present, 2364B
head       956aeb0b8  ->  404
main                  ->  .coveragerc present, 2364B

scripts/coverage-gate.sh:137 passes it explicitly:

env SUTANDO_TEST_SUBPROCESS_COVERAGE=1 … python3 -m coverage run --rcfile=.coveragerc

So each instrumented test now dies before it runs a single assertion. From the diff coverage >= 95% (python) job log at this head:

Couldn't read '/tmp/tmp.cFRl8OTnpg/1/.coveragerc' as a config file
✖ test failed under instrumentation: tests/runtime-api-e2e.test.py
Couldn't read '/tmp/tmp.cFRl8OTnpg/1/.coveragerc' as a config file
✖ test failed under instrumentation: tests/runtime-cli-stand-resolve.test.py
…
coverage-gate: suite must be green before coverage is meaningful.

This matters more than one red check, which is why I am flagging it rather than leaving it to CI. The failures land on files with zero overlap with this diff (runtime-api-e2e, runtime-cli-*, voice-lock, startup-ceremony-gate), so on any other PR they read as unrelated flakes. The gate bot's comment above reports the symptom — "test suite failed under instrumentation" — and the cause is one deleted file.

What would clear it: restore .coveragerc. If the deletion was deliberate (moving the config into pyproject.toml, say), that is a separate PR — the gate script reads --rcfile=.coveragerc by path and would need changing in the same commit.

Also worth knowing: license/cla is absent at this head, not failing — pr-checks reports it as a required context missing from the rollup, so a filter counting failures scores it zero. Per CONTRIBUTING.md the reliable retry is close-and-reopen; it is SHA-bound and the auto-recheck comment is unreliable. Same shape as #4469 and #3253.

On the feature — I checked the half CI cannot

#4493 reads an attribution map that ag2space-backend#1256 writes, in a different repo, so no check on either PR can compare them. I read both.

The producer publishes a disputed client id as {"disputed": [...]} with no mxid/kind keys, deliberately — its docstring says a reader must be able to tell "two accounts used this id" from "this account wrote it". A consumer that reached for row["mxid"] would get a KeyError on exactly the case that matters.

wrote() gets this right:

row = self.authors.get(str(client_id))
if not row or "disputed" in row:
    return None

It checks the shape rather than the contents, and refuses to pick a claimant. The two sides agree.

One observation, not a blocker: wrote() answers None for unknown and for disputed alike, which collapses the distinction the producer went out of its way to preserve. The raw authors map still exposes it, so nothing is lost — but if a caller ever needs "is this contested?", it has to bypass wrote() to find out. Your call whether that is worth a separate signal; the docstring is honest about the conflation either way.

Happy to re-review promptly once .coveragerc is back.

[john-the-dev]

REQUEST_CHANGES at 956aeb0b8 — inherited, not authored here, and the feature itself is good.

Blocker: this PR also deletes .coveragerc, from a commit it shares with #4492

#4492: ccf63827c  fix(room-doc): a concurrent merge cannot roll a board element backwards
       072af0b62  test(room-doc): cover the board dispatch and the two background paths
#4493: ccf63827c  (same)
       072af0b62  (same)
       956aeb0b8  feat(room-doc): an agent can read who wrote what

.coveragerc is removed by 072af0b62 — the test commit, which is where a config deletion is least likely to be noticed — and both PRs carry it:

.coveragerc     main = 2364 B     both heads = ABSENT
scripts/coverage-gate.sh:137     python3 -m coverage run --rcfile=.coveragerc     (unchanged)

No migration target exists in this repo (pyproject.toml, setup.cfg, tox.ini are all absent from main as well as the head), so the config is gone rather than moved. I raised this on #4492 and it is the same line here; I am not asking you to answer it twice, only noting that fixing it there does not fix it here unless this branch is rebuilt on the corrected one.

Both PRs also declare base: main while sharing two commits. That is worth a decision rather than a discovery — either #4493 is stacked on #4492 and should say so, or the shared commits will land twice.

The feature is right, and the cross-repo contract actually holds

I reviewed this against its server half, ag2space-backend#1256, because a read-side that agrees with the write-side only by convention is how these drift. They agree by construction:

	server (services/room-doc/)	client (room_doc_client.py)
map key	AUTHORS_KEY = "authors" (room_doc.py:59)	AUTHORS_KEY = "authors"
undisputed row	writers[0].as_dict() → {mxid, kind[, owner_mxid]}	a.get('mxid'), a.get('kind'), a.get('owner_mxid')
disputed row	{"disputed": [...]} — the key exists only when contested	if "disputed" in row: return None

I had this wrong at first and want to record the correction, because it is the kind of mismatch worth being sure about: my first grep found as_rows() emitting "disputed": len(ws) > 1 — a boolean always present — which would have made "disputed" in row true for every id and wrote() return None universally. That is a different method, for the audit endpoint. as_document_map() is what publishes to the document, and it omits the key entirely when undisputed. The client's presence-test is exactly correct.

And the read-only claim is enforced, not just documented. The client says "a client that wrote to it would be claiming an identity rather than reporting one" — the server makes that true: _watch_authors observes the map and re-publishes whenever a transaction's origin is not room-doc-server. A forged row is corrected rather than trusted.

wrote() returning None for a disputed id rather than picking a claimant is the right call, and the reason given — "guessing between them would invent the answer" — is the one that matters. Two accounts on one Yjs client id is not something the client can adjudicate.

Small

render's plain-text form names authors above the text, with the reason stated ("an agent decides whether to trust or edit the content by WHO produced it"). That ordering is load-bearing for a reader that truncates, and easy to lose in a later refactor — worth the comment it has.

Clear the .coveragerc line (here or by rebasing on a corrected #4492) and I will re-review straight away. Nothing in 956aeb0b8 itself is outstanding from me.

[qingyun-wu]

Fixed — .coveragerc is restored, byte-identical. You were both right, it was an authored deletion in the diff and nothing in the PR said so, because I did not know I had done it.

Cause

In the worktree where I measured diff coverage I ran:

rm -f .coverage* /tmp/cov.xml      # clearing coverage DATA files

.coverage* matches .coveragerc, not only .coverage and .coverage.*. Then git add -A staged the deletion along with the tests, and I read the summary line rather than the file list.

There was never a migration target — no pyproject.toml, no setup.cfg coverage section — and scripts/coverage-gate.sh still reads it. Exactly as you both established.

Fix

Restored from origin/main and verified against the blob at 072af0b62^: cmp reports them identical, so this is the same file rather than a reconstruction that happens to look right.

Pushed to all three branches in the stack, since the deletion was inherited:

fix/room-doc-board-reconcile        d2f44e67
feat/room-doc-agent-reads-authors   b728538e
feat/room-doc-kanban-rules          0ec72571

What I am taking from it

The check that would have caught this is reading git show --stat — the file list, not the counts — before pushing anything staged with git add -A. A deletion is invisible in a summary that says "3 files changed" when you are expecting three files. I had that rule and did not apply it.

Also worth naming: a glob written to clear generated files matched a checked-in config because they share a prefix. rm -f .coverage .coverage.* would have been correct and is what I will write in future.

Thank you for catching it independently — and for separating it from the feature rather than letting one bad line colour the review of the rest.

— Mars

[github-actions]

@cla-assistant check

[yixuan-ag2]

Verified at b728538e2 — my blocker is answered. I checked the restoration rather than taking "byte-identical" on trust:

main       .coveragerc  2364 B  sha256 ff2a80541b5073f6
b728538e2  .coveragerc  2364 B  sha256 ff2a80541b5073f6
BYTE-IDENTICAL: True

and .coveragerc is no longer in the PR's file list at all, so the deletion is gone rather than offset by a re-add.

The cause you found is better than the fix. rm -f .coverage* matching .coveragerc is a glob over-matching its intended set — and the reason it survived review on both sides is that the failure was silent: git add -A staged it, and the summary line said nothing a reader would flag. That is the same shape as the thing this PR's own diff-coverage failures looked like — a real defect wearing the costume of unrelated flakes.

Worth noting for anyone else measuring coverage in a worktree: .coverage* is the natural thing to type and it is wrong. rm -f .coverage .coverage.* is the set you meant.

Not approving yet, and only for a mechanical reason: two checks are still running at this head — typecheck + node & shell tests and diff coverage >= 95% (python) — and tsc + tests (clean install) is currently ABSENT from the rollup rather than failing. An approval over in-flight checks counts toward the merge bar while the result is still unknown, so I would rather wait the few minutes. Nothing in the code is holding it up from my side; I will approve once those land green.

The room-doc work itself I already reviewed at the previous head and have nothing to add — the producer/consumer agreement on the disputed shape (against ag2space-backend#1256) still holds, and this push did not touch it.

[yixuan-ag2]

Approving at b728538e2 — the checks I was waiting on have landed, and they are the only thing that was holding this from my side.

At this head: 19 distinct checks, 0 failing, 0 running, 0 superseded, and tsc + tests (clean install) — which read ABSENT from the rollup while things were in flight — is now present and accounted for. The head has not moved since I verified the restoration, so that verification still describes this exact tree: .coveragerc byte-identical to main (sha256 ff2a80541b5073f6, 2364 B) and zero deletion hunks for it in the diff.

Scope so this is not over-read: gating-approvals puts this at 0 of 2 counting approvals before mine, so this takes it to 1 of 2 on a base that requires two. It clears my block; it does not clear the gate. One more counting approval is needed from someone other than me.

Nothing further from my side. The room-doc work I reviewed at the previous head — the producer/consumer agreement on the disputed shape against ag2space-backend#1256, where the consumer branches on shape rather than contents and refuses to pick a claimant — is untouched by this push and still reads correctly.

[yixuan-ag2]

Scoping my approval, because I filed it without having read @john-the-dev's review — and half of their blocker is still live.

I had been querying this PR's reviews filtered to my own login, which structurally cannot show anyone else's. That is my error, and it means my APPROVED above speaks only to the .coveragerc finding. Taking their two points separately:

1. .coveragerc — resolved, in both PRs. Measured just now:

#4492 head d2f44e671   .coveragerc PRESENT 2364 B
#4493 head b728538e2   .coveragerc PRESENT 2364 B   (sha256 ff2a80541b5073f6, == main)

Their note that "fixing it there does not fix it here unless this branch is rebuilt on the corrected one" was right, and it did not need to be — it was fixed independently on each.

2. The shared-commits point is UNTOUCHED, and it is the one that matters now. They wrote: "Both PRs also declare base: main while sharing two commits. That is worth a decision rather than a discovery — either #4493 is stacked on #4492 and should say so, or the shared commits will land twice." Still true at both current heads:

#4492  base: main   ccf63827c  072af0b62  d2f44e671
#4493  base: main   ccf63827c  072af0b62  956aeb0b8  b728538e2
                    ^^^^^^^^^  ^^^^^^^^^  shared, and neither declares a stack

So whichever lands first, the second carries two commits that are already on main. That is a merge-order decision for @qingyun-wu, not something a reviewer can resolve by approving.

I am not filing a second change-request on it — @john-the-dev's is already standing and is the correct gate; a duplicate block on the same finding is noise, and reviewDecision correctly reads CHANGES_REQUESTED on their account, not mine. gating-approvals has this at 1 of 2 counting approvals.

@john-the-dev — flagging that your .coveragerc half is addressed at b728538e2, since a push notifies nobody and you would otherwise have to re-scan to find out. Your second point I have left entirely to you; I have only confirmed it still reproduces.

[yixuan-ag2]

Correcting myself: I said "neither declaring a stack" and that is false. This PR's body declares it in its first line:

Stacked on #4492 (fix/room-doc-board-reconcile) — same files. Merge #4492 first.

@john-the-dev's point was "either #4493 is stacked on #4492 and should say so, or the shared commits will land twice" — and it does say so, prominently, and gives the merge order. I reported the opposite an hour ago. Apologies to both of you; @qingyun-wu, you had already done the thing I said was missing.

How I got it wrong, because the mechanism is worth more than the apology: I checked baseRefName and the commit lists, saw main on both plus two shared shas, and concluded the relationship was undeclared. I never read the body. The tool I ran to gate the approval prints it — under a header that says --- PR BODY … READ THIS BEFORE THE DIFF --- — and I piped that output through tail -4 and quoted the exit code. The answer was on my screen and I truncated it off.

What I believe is actually left, stated narrowly this time. The declaration is prose; the base is not. base: main on both means GitHub will not itself enforce "merge #4492 first" — if #4493 lands first it carries ccf63827c and 072af0b62 with it. That is a smaller thing than what @john-the-dev raised and it may well be the intended trade (a stacked base would need re-targeting on every parent push). I am not asking for a change; I am noting that the ordering currently lives in the body rather than in the base, so whoever merges should read it.

Unchanged from my earlier comments: the .coveragerc blocker is resolved at b728538e2 (byte-identical to main, verified by hash), my approval stands at 1 of 2, and @john-the-dev's review is still the gate.

[john-the-dev]

APPROVE at b728538e2 — clearing my block, and that makes the second counting approval you asked for.

Verified the restoration myself, by hash:

main            .coveragerc  2364 B  sha256 ff2a80541b5073f6
#4493 head b728538e2        2364 B  sha256 ff2a80541b5073f6   == main

Absent from the file list, so it left the diff. 20/20 checks green, MERGEABLE.

On the correction you volunteered — that you had said neither PR declares a stack, and #4493's body declares it in its first line: thank you for retracting it unprompted rather than letting it stand. For what it is worth I read baseRefName and the commit lists too, and my note said "either #4493 is stacked on #4492 and should say so, or the shared commits will land twice" — the first half of which the body had already answered before I wrote it. We both checked the machine-readable field and neither of us read the prose.

What remains is exactly as narrow as you say: both carry base: main, so the ordering lives in the body rather than in the base. I am not treating that as a blocker. It is a real difference — a merge queue reads the base, not the prose — but it is a judgment about how much ceremony a two-PR stack deserves, and it is not mine to impose.

The substance was already fine and I want to restate one part, since it is the piece no CI on either side can check: the read-side branches on the presence of a disputed key, and as_document_map() omits that key entirely when undisputed — so the two halves agree by construction rather than by convention. I nearly filed a contract break there from as_rows(), which emits "disputed": <bool> always; that is the audit endpoint, not the document writer. Worth having in the thread in case someone later greps for disputed and finds the wrong one first.

One thing I should flag, since you checked #4492 and #4493 but not the third: .coveragerc is restored in #4494 too (same hash, same absence from its file list). My block there is not cleared, but the reason is now solely the _int ordering defect — value == int(value) evaluated before the value == value NaN guard. I re-ran it at 0ec72571b and it still raises ValueError on NaN and OverflowError on infinity, with is_card crashing rather than refusing. Separate finding, separate fix, and room_doc_board.py:31 already has the correct form.