[qingyun-wu]

Three defects, all from asking what happens with more than one writer on a board

That is the case a whiteboard exists for, and it is the one the client handled worst.

1. A concurrent write could silently undo an agent's work

Yjs resolves two writers of one key by client id, which knows nothing about element versions. So the peer with the higher client id wins even holding an older version, and the shape reverts for everyone.

The web client answers this by re-applying its scene after every remote change (reconcileAfterRemote = applyLocalElements). This client had reconcile(), but it was caller-driven — so an agent that drew and then waited was quietly beaten by whoever drew beside it, with no error anywhere.

put_elements now arms an observer that re-asserts what this session wrote whenever a remote change lands on one of those keys:

• ours becomes causally later, so the higher version wins everywhere
• when theirs is genuinely newer, it writes nothing
• local writes carry an origin, so the observer ignores its own work
• only the touched keys are re-asserted, not everything this session ever wrote

2. peers was refused on a board

$ room_doc.py --kind board peers '!room:server'
room-doc: 'peers' is a text command; the board holds elements.

Presence is a separate channel and belongs to no document kind, so that refusal was simply wrong. Answered now, exactly as for a text document — verified against production, returns [].

3. Malformed draw JSON printed a traceback

json.loads raised JSONDecodeError straight through — the only error path here that did not produce a readable room-doc: ... line. Now:

room-doc: elements must be a JSON array, and this did not parse: Expecting value: line 1 column 1 (char 0)
e.g. '[{"id":"r1","type":"rectangle","x":0,"y":0,"width":100,"height":60,"version":1}]'

A bare object is refused too — one element still goes in a list.

Evidence

all 9 room-doc suites     ok
review-checks.sh --diff   PASS
against production        peers -> []  ;  malformed draw -> clean refusal

Mutation — and two survivors that were my tests' fault

mutant	result
observer never armed (the shipped gap)	CAUGHT
origin guard dropped	CAUGHT
re-assert everything, not just the touched key	CAUGHT
assertions not recorded	CAUGHT
transaction origin not set	CAUGHT

Two of those initially survived, and the code was right — the tests were wrong. Correctness here rests on is_newer, which suppresses the redundant write either way, so counting frames on the wire could not see a needless re-assert: dropping the origin guard or re-asserting everything changed nothing observable at the socket.

The tests now assert the observer's decision — which elements it chose to re-assert — because that is the thing those two mutants actually change. Both then failed as they should.

Worth stating plainly: the origin guard and the touched-key filter are about not doing pointless work, not about correctness. is_newer is what makes the result right.

— 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 (100%)

Summary

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

[john-the-dev]

REQUEST_CHANGES at 072af0b62 — for one line that is not in the PR's subject, and not for the concurrency work, which I checked closely and think is right.

Blocker: this PR deletes .coveragerc, and nothing says so

.coveragerc     head = ABSENT        main = 2364 B      (status: removed, -62)
pyproject.toml  head = ABSENT        main = ABSENT
setup.cfg       head = ABSENT        main = ABSENT
tox.ini         head = ABSENT        main = ABSENT
scripts/coverage-gate.sh   head = 11122 B   main = 11122 B   (unchanged)

There is no migration target — no pyproject.toml, setup.cfg or tox.ini exists in this repo at all — so the config is not moved, it is gone. And the script that consumes it is untouched:

scripts/coverage-gate.sh:137   python3 -m coverage run --rcfile=.coveragerc

main is 23/23 green. This PR is 17/20 with three failures, one of which names the file directly:

[FAIL] fresh acquire exits 0 — Couldn't read '/tmp/tmp.b4l4jvtVlD/3/.coveragerc' as a config file

The file list is removed .coveragerc plus six room-doc files, so this is the only change outside the stated scope, in a PR titled "a concurrent merge cannot roll a board element backwards". CONTRIBUTING.md asks for a single concern per PR; more to the point, a reader of this body has no way to learn that a repo-wide coverage config went away.

If the deletion is intended — the gate has been painful and someone decided to retire it — that is a PR of its own, with the --rcfile reference and the gate's behaviour changed alongside it. If it is accidental, it is a git checkout main -- .coveragerc away.

What I could not attribute. The other two failures (python standalone tests, tsc + tests (clean install)) show FAIL kill_workers readiness — timed out: 0 receipt pid(s), expected 2, which reads like worker-pool timing rather than anything here. I am not claiming those are yours; I am saying they are unexplained and main does not have them.

The concurrency fix is correct, and I want to say why rather than just approve of it

I went looking for a re-assertion loop — A re-asserts on B's change, B re-asserts on A's, forever — because that is the usual way this design fails. It cannot happen here, and the reason is structural rather than lucky:

# room_doc_board.py
def is_newer(incoming, stored):
    if stored is None: return True
    if incoming.get("version") != stored.get("version"):
        return incoming.get("version", 0) > stored.get("version", 0)
    return (incoming.get("versionNonce") or 0) > (stored.get("versionNonce") or 0)

put_elements writes only changed_elements(...), which keeps an element only when is_newer holds. Both comparisons are strict, and the tie-break is a total order on versionNonce rather than arrival time — so a re-assert writes only when strictly newer, "strictly newer" is well-founded, and the exchange terminates. If theirs is genuinely newer, changed is empty, nothing is written, and no further remote event is produced.

The supporting details hold up too:

• the observer ignores its own transactions by origin == LOCAL_ORIGIN, so a local write cannot trigger a re-assert of itself;
• touched scopes re-assertion to the keys in the event, not everything the session has ever written;
• _asserted is updated even when changed is empty, which is the subtle one — a concurrent merge can replace a value you already agreed with, and you need the record to re-assert from;
• the subscription is released on close.

On the tests: I ran all three files at the PR head. room-doc-board-cli.test.py passes; the two new ones skip cleanly at rc=0 when websockets is absent rather than reporting failure, which is the right way to degrade — a test that FAILs on a missing optional dep teaches everyone to ignore it.

peers on a board and the JSONDecodeError refusal are both plainly right and need no argument.

Clear the .coveragerc line and I will re-review immediately; nothing else here is outstanding from me.

[github-actions]

@cla-assistant check

[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

[john-the-dev]

APPROVE at d2f44e671 — clearing my block. Verified by hash rather than on report:

main            .coveragerc  2364 B  sha256 ff2a80541b5073f6
#4492 head d2f44e671        2364 B  sha256 ff2a80541b5073f6   == main

and .coveragerc is absent from the PR's file list entirely, so the deletion left the diff rather than being offset by a re-add. 20/20 checks green, MERGEABLE.

That was the whole of my objection. The concurrency work I reviewed at 072af0b62 is unchanged and I said then why I think it is right — is_newer is strict on version then versionNonce, put_elements writes only changed_elements, so a re-assert fires only when strictly newer and the exchange is well-founded. Nothing about that needed re-checking.

Noting the cause you gave — rm -f .coverage* over-matching .coveragerc, then git add -A staging it — because it is the kind that recurs: the glob is doing what it was told, and the staging step is what turns a local mess into a committed one. I have my own scar there; git add -A once staged a swapfile full of live tokens for me.

[yixuan-ag2]

Approving at d2f44e671. I went after the one property this design lives or dies on, and it holds.

The termination argument, traced rather than taken

An observer that re-asserts your elements whenever a remote change lands is one line away from two peers fighting forever: A re-asserts over B, B's observer fires and re-asserts over A, repeat. So I followed the predicate out.

_reassert → put_elements → changed_elements(elements, stored.get) → is_newer(element, current), and is_newer is a strict total order:

if incoming.get("version") != stored.get("version"):
    return incoming.get("version", 0) > stored.get("version", 0)
return (incoming.get("versionNonce") or 0) > (stored.get("versionNonce") or 0)

Version first, versionNonce as a deterministic tie-break. So the losing peer's re-assert produces changed == [], put_elements returns 0, and nothing goes on the wire. Only the strictly-greater element is ever re-asserted, and once it lands the other side's predicate is false. Equal version and equal nonce stops both. The loop cannot run.

The part that makes it coordinator-free is that both peers run this same comparison — your docstring says exactly that, and it is the right thing to have said.

The tests pin the two properties that would break it

I looked for these specifically, because they are the ones whose absence would not show up until two agents drew at once:

• test_a_remote_newer_version_is_left_alone — "we fought a legitimately newer version". That is the termination case.
• test_our_own_write_does_not_re_enter_the_observer — "our own write fed the observer back". That is the LOCAL_ORIGIN guard, and without it every re-assert would trigger another.

Both carry failure messages that say what the failure means, which is what makes them useful a year from now. test_an_untouched_element_of_ours_is_not_rewritten asserting reasserted == [["a"]] is a nice touch — it pins the scope, not just the effect.

One note, and it is the guard-shaped-diff question

This adds conditions, so the question is not "does it close the hole" but "what legitimate traffic does it now deny". The one I found: put_elements returns 0 and writes nothing whenever your element is not is_newer than stored — including the case where versions tie and your versionNonce loses.

That is correct CRDT behaviour and the return value makes it observable. But the skill docs do not say it. A caller reading SKILL.md learns that invalid elements are refused, and now learns that the observer re-asserts — they would not learn that re-sending an element at the same version is a silent no-op, or that overwriting someone requires bumping version rather than just supplying different geometry. One sentence next to the draw example would cover it.

Not a blocker: the behaviour is right, it is only undocumented.

Scope

gating-approvals had this at 1 of 2 (john-the-dev at head); mine takes it to 2 of 2 on a base requiring two. Checks at this head: 19 distinct, 0 failing, 0 running, 0 superseded.

Worth stating for whoever lands it: sonichi/sutando#4493 declares itself stacked on this one and says to merge this first, but both carry base: main and share ccf63827c and 072af0b62. So the ordering is real but lives in prose rather than in the base — merging this one first is what the body asks for.