Repository navigation
feat(room-collab): a host that keeps a summoned agent in its surfaces - #4646
Conversation
Groundwork for the daemon that keeps a summoned agent in a surface. Both pieces are pure or near-pure so the rules that decide an agent's visible presence are testable without a server. presence_policy.plan() takes the desired set, what the daemon last observed and `now`, and returns what to connect and what to drop. Owner's parameters: drop after 30 minutes with no activity on the surface, cap 16 connections. `idle` and `capped` are separate drop reasons because only one needs a new summon to come back — a surface that idled out must NOT be rejoined by the next reconcile pass, or the timeout means nothing. presence_store owns the two records, one writer each: `desired`, written by agents under an exclusive lock, and `live`, written only by the daemon. The whole read-modify-write is inside the lock; locking only the write still loses a registration, which the concurrency test demonstrates. Both records publish through a temp file plus os.replace. A reader polling during a truncate-in-place would read a zero-length file, which parses as "nothing is connected". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The presence daemon's idle timer must measure activity on the SURFACE, not activity addressed to the agent (owner chose that reading: a document someone else is editing is exactly when an agent's presence is worth showing). `events()` answers the other question — what here is addressed to me — so it cannot supply that signal. `on_activity()` does, and the subscription switch over the document kinds is now shared between them rather than copied: two copies drift the moment a fourth kind lands and only one is taught about it. `events()` keeps its behaviour exactly. Verified on the room-collab venv: all ten client suites pass, and kanban-convergence's output is byte-identical to the same suite run against the unmodified client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A summon asks an agent to BE somewhere, but the connection that establishes it was a foreground child of whichever session ran `watch` — so the agent dropped out the moment that session ended, compacted or restarted. The rule existed only as prose in SKILL.md; nothing executed it (#4642). Three parts, thin where the sockets are and testable where the decisions are: presence_daemon.py one asyncio process holding N surfaces. Owns sockets, the clock and the process; decides nothing. room_collab stay an agent registers a surface when it reads a summon — a record, no token, nothing held open. sparrowd supervises the daemon, because `watch` ends on its own (clean close, refusal, service restart) and a launcher that only starts it leaves the agent quietly absent. Owner's parameters: drop after 30 minutes with no activity ON THE SURFACE (someone else's keystroke counts — a document being edited is exactly when an agent's presence is worth showing), cap 16, LRU. Two defects this found, both in code written earlier in the same branch: * `plan` undid its own idle drop. The entry it had just timed out was still CONNECTED in the record it was reading, so the same pass queued it straight back. The policy suite asserted the drop and passed throughout — asserting a drop is not asserting the surface goes away. Now pinned. * The daemon mixed two clocks: `reconcile(now)` took the time as an argument while `hold` stamped `time.time()`. In production they agree by luck; the idle rule could not be tested at all. One injectable clock now. sparrowd refuses to supervise the daemon rather than guess an interpreter: the skill imports pycrdt and websockets, which the core's python need not have, so an unconfigured install skips the worker with a reason instead of crash-looping under the supervisor. ROOM_COLLAB_PYTHON is declared in the skill's manifest per skills/MANIFEST.md, not as an ad-hoc env var. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Gate❌ Diff coverage BELOW the 95% bar — uncovered changed lines listed below. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
scripts/lint-skill.pyLines 108-138 108 # supervised_worker: sparrowd reads this to decide what to keep running, and
109 # it names no skill, so a malformed block is a silently unsupervised daemon.
110 sw = m.get("supervised_worker")
111 if sw is not None:
! 112 if not isinstance(sw, dict):
! 113 err("supervised_worker must be an object")
114 else:
! 115 for k in sw:
! 116 if k not in {"name", "script", "interpreter"}:
! 117 warn(f"unknown supervised_worker field '{k}'")
! 118 wname = sw.get("name")
! 119 if not isinstance(wname, str) or not re.match(r"^[A-Za-z0-9][A-Za-z0-9_-]{0,63}$", wname):
! 120 err("supervised_worker.name must be a plain name, not a path")
! 121 script = sw.get("script")
! 122 if not isinstance(script, str) or not script:
! 123 err("supervised_worker.script is required")
! 124 elif script.startswith("/") or ".." in Path(script).parts:
! 125 err("supervised_worker.script must stay inside the skill directory")
! 126 elif not (skill_dir / script).is_file():
! 127 err(f"supervised_worker.script does not exist: {script}")
! 128 interp = sw.get("interpreter")
! 129 if not isinstance(interp, dict) or not isinstance(interp.get("config"), str) \
130 or not interp["config"]:
! 131 err("supervised_worker.interpreter.config is required "
132 "(the config key naming the interpreter)")
! 133 elif interp["config"] not in (m.get("config") or {}):
! 134 warn(f"supervised_worker.interpreter.config '{interp['config']}' "
135 "is not declared in the config block")
136
137 # required
138 for req in ("name", "version", "owner", "stability"):skills/room-collab/scripts/presence_daemon.pyLines 26-34 26 from pathlib import Path
27
28 HERE = Path(__file__).resolve().parent
29 if str(HERE) not in sys.path:
! 30 sys.path.insert(0, str(HERE))
31
32 import presence_policy as policy # noqa: E402
33 import presence_store as store # noqa: E402Lines 164-172 164 held.last_activity = self.clock()
165 failures = 0
166
167 def touched() -> None:
! 168 held.last_activity = self.clock()
169
170 stop = doc.on_activity(touched)
171 try:
172 # Awaited, not slept through: the read loop records theLines 194-202 194
195 async def stop(self, key: tuple[str, str], reason: str, now: float) -> None:
196 held = self.held.pop(key, None)
197 if held is None:
! 198 return
199 if held.task is not None:
200 held.task.cancel()
201 with contextlib.suppress(asyncio.CancelledError, Exception):
202 await held.taskLines 219-227 219 return
220 want = {policy.key_of(e) for e in desired}
221 # A surface nobody asks for any more has nothing left to remember.
222 for key in [k for k in self.retired if k not in want]:
! 223 self.retired.pop(key, None)
224 step = policy.plan(desired, self.live_rows(), now,
225 idle_seconds=self.idle_seconds, cap=self.cap)
226 for key, reason in step["drop"]:
227 await self.stop(key, reason, now)Lines 263-277 263 if held is None:
264 print("presence daemon: another copy is already running for this workspace",
265 file=sys.stderr)
266 return 3
! 267 d = Daemon(ws, url, token, idle_seconds=a.idle_seconds, cap=a.cap, insecure=a.insecure)
! 268 try:
! 269 return asyncio.run(d.run())
! 270 except KeyboardInterrupt:
! 271 return 0
272 finally:
! 273 held.close()
274
275
276 if __name__ == "__main__": # pragma: no cover
277 raise SystemExit(main())
|
Owner's review of #4646. Each finding reproduced in the code before fixing; each fix pinned by a test that fails without it. [P0] The transport could die unnoticed. `hold` entered the session and then only slept, and `RoomDoc` records a closed socket without raising into a holder. So a server restart removed the agent from presence while the daemon still listed the surface and never reconnected. `RoomDoc.closed()` is now a public awaitable end-of-session, `hold` awaits it and reconnects with backoff, and the live record carries `transport: up|down` beside `state` — `state` says this daemon holds the surface, only `transport` claims a live connection. [P0] The live record was written and never read. `reconcile` assembled its view from in-process state alone, so a restart emptied what had been retired and rejoined surfaces that had just idled out — the 30-minute rule reset on every restart. `resume()` reads it back, and is called by `reconcile` rather than by a caller who must remember to: a reconciler that only remembers when asked is the same defect one level up. Only `idle` and `capped` survive a restart; a `connected` row describes a socket that died with the process, and taking it back would leave the surface reported connected and never reopened. [P0] `stay` was not the path an agent would take. SKILL.md still said "to be seen you must HOLD it open", so the normal path stayed on session-owned `watch` whatever this daemon did — now it documents `stay` and says what it is for. `stay` also took `--user-id`/`--name` raw, registering identity=None and name=None, and the daemon publishes no presence without a name: a held socket nobody can see. Both now resolve exactly as the rest of the skill does. [P1] "cap 16 + LRU" was a ceiling, not admission. Shedding happened only when the cap was LOWERED, so at the cap a fresh summon queued behind sixteen idle surfaces forever. A candidate newer than the quietest holder's last activity now takes its place; one that is not stops the scan, since the queue is newest-first. The three invariants the owner set as the gate now have regression tests: connection dies -> membership survives -> connection returns without a new summon; daemon restarts -> idle/capped/desired semantics survive, with no incorrect resurrection; summon -> durable desired membership, established by the real CLI in a subprocess and independent of any session. Two of those tests did not discriminate on the first attempt: "it opened again" is also true of a holder that merely loops, and a restart test that calls `resume()` by hand cannot see the wiring missing. Both were rewritten until the mutants died. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…' into feat/room-collab-presence-daemon
CI reported 79% diff coverage against a 95% gate. The uncovered lines were
the ones a reader would most want covered, so this closes them rather than
arguing the threshold.
room_collab.py 21.1% — the whole `stay` handler read as untested while
being exercised end to end: the test runs it in a
subprocess, and the repo measures those only under
SUTANDO_TEST_SUBPROCESS_COVERAGE. Wired the same
way the sibling suites do it.
src/sparrowd.py 70.8% — the three decisions about the presence worker had
no test at all. tests/sparrowd-presence-worker
pins them: unconfigured and missing-interpreter
both REFUSE with a reason, and neither disturbs
the core's own worker.
presence_daemon.py 78.7% — the entry point was unexercised; it now has a
test that it exits 2 naming the missing URL rather
than supervising nothing.
room_collab_client 82.8% — `closed()` had no test, and it is the whole reason
a holder can notice a dead socket. Loaded by path,
since sys.modules holds this suite's stub under
that name; skipped cleanly on an interpreter
without the client's deps, which EXITS at import.
presence_store.py 94.2% — a publish that cannot serialise now asserts no
temp file survives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI went red on lint-old-collab-literals: the new daemon test scrubbed the environment by naming the variables to remove, and two of those names carry the retired room-doc spelling (ROOM_DOC_TOKEN, AG2_ROOM_DOC_URL). The list was the real mistake, not the spelling. A copy of the skill's own TOKEN_VARS/URL_VARS in a test is the thing that drifts the day one is added: the test would still pass while no longer scrubbing what it claims to. Build the child's environment from what an interpreter needs instead, and the duplication and the lint failure go together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI put the diff at 93% against a 95% gate. The remaining uncovered lines were
branches an operator actually hits, so they get tests rather than a waiver.
sparrowd the manifest is the DOCUMENTED place to name the interpreter,
and only the env var was exercised — so the documented path was
the untested one. Also the unreadable-manifest branch, and the
skill absent entirely, which is the case the core must boot
through. 87.5% -> 100%.
room_collab `stay --json`, the form anything parsing this will use.
daemon --idle-seconds and --cap reaching the policy. A daemon whose
knobs are ignored looks configured and is not.
Also consolidated three local `import subprocess` into one at the top; three
spellings of the same import in one file is how one of them drifts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last of the diff-coverage gap was `run()` itself — every test drove `reconcile` directly, so the loop around it had never executed. That is the part that must not stop: a reconciler that dies on one bad pass leaves the agent absent everywhere until someone notices. The test runs the real loop, makes its second pass raise, and asserts it keeps going and still holds the surface. Also `--help`, which is how anyone finds the knobs the owner set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ran it, on the owner's host, against the live serviceThe PR said the daemon had never held a real surface. It has now. The agent is in the surface, under the name the record carries, with no session holding it open. Invariant 2, liveMembership survived the process that was serving it, and came back from the record alone. The property this exists forThat is the whole point of #4642: presence was a foreground child of whichever session typed sparrowdConfigured, the worker appears; unconfigured it is skipped with a reason, which is the behaviour already pinned. Still not verifiedInvariant 1 — transport death and automatic reconnect — was not exercised live. I can end the daemon, but not the server's side of the socket, and I am not going to restart a service on the owner's machine to produce one. It is covered by a regression test that ends the session and asserts the daemon reconnects without a new summon, and the reconnect path will show in the daemon's log the first time the service does drop us. I would rather say that plainly than let the run above read as proof of something it did not touch. The idle rule was run at — Sudoo (c8138e81) |
Found by running the daemon on the owner's host rather than by reading it.
The workspace briefly became unreadable (a macOS Documents/TCC blip, which
this host does), and the daemon's log shows what happened:
presence: joined !oZUw… (markdown)
presence: left !oZUw… (markdown): left
presence: reconcile failed: [Errno 1] Operation not permitted: …
`read_entries` caught OSError and returned [], so `plan` saw nothing desired
and evicted the agent from the surface with reason `left`. A transient
permission error was recorded as "the user no longer wants this", and the
agent silently left a room nobody had asked it to leave.
Absent, empty, truncated or of another schema still read as empty — there is
nothing to act on. A record that EXISTS and cannot be read now raises
`RecordUnreadable`, and `reconcile` holds what it holds rather than
reconciling toward nothing. This is the rule `task_queue.pending` already
states for its own queue: "one that exists but cannot be read raises …,
never reads as empty."
The docstring had asserted the wrong behaviour as if it were the design.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The live run found a defect the tests did notLeaving the daemon running on the owner's host surfaced this, about half an hour in: The workspace briefly became unreadable — this host's macOS Documents/TCC access flaps, which is a real condition, not a contrived one. A transient permission error was recorded as "the user no longer wants this", and the agent quietly left a room nobody had asked it to leave. The record it would have needed to recover from was the same one it could not read. Worse, my own docstring asserted that behaviour as though it were the design: """… A file that is missing, empty, truncated or not of this schema reads as
empty — the daemon then reconciles toward nothing …"""FixAbsent, empty, truncated or of another schema still read as empty: there is nothing to act on. A record that exists and cannot be read now raises This is the rule
Two mutants, each killed by its own assertion: swallowing the error again fails both suites; holding the error but reconciling toward Why the tests missed itEvery fixture wrote a readable record or no record. "Present but unreadable" is a third state, and it is the one a real host produces. That is the argument for running it, and it is why the owner asking to try it was worth more than another round of review. The live daemon has been swapped onto the fixed build and is holding the doc again. — Sudoo (c8138e81) |
|
Independently reviewed at head b84331f (isolated worktree, The reported live defect is real. Reverted The fix is correctly scoped. Worst-case / blast-radius (per REVIEW.md's mandatory question): the sparrowd integration ( No blockers found. CI green, diff coverage 93.5%+ (gate passes), tests genuinely exercise the failure mode. Recommend approval. Stand: Echo Act IV Mini (Reviewed after reading qingyun-wu's comment in full: issuecomment-5790397954, 2026-09-23T06:50:43Z — the fix + mutation-testing writeup verified above.) |
|
@john-the-dev @bassilkhilo-ag2 — review please. You two approved #4641 on this track, so this is the same lane. What it is. A summoned agent never actually appeared in a surface (#4642): Where to look first, in the order the risk sits:
What it does to the host: nothing, until a deployer sets It has been run for real on the owner's machine (two comments above): registered, joined, survived its own death and came back from the record with no new summon, parented to launchd rather than to a session. That run also found a defect no test of mine had — an unreadable record read as empty, so the policy concluded nothing was wanted and evicted the agent with reason Not verified: transport death and automatic reconnect. I can end the daemon, not the server's half of the socket, and I would not restart a service on the owner's machine to manufacture one. It has a regression test; it has not been seen in the wild. Diff coverage is 96% against the 95% gate; every other check is green. — Sudoo (c8138e81) |
Invariant 1 has now been observed in the wildThe PR said transport death and automatic reconnect had a regression test but had never been seen for real. It has now — the service does it on its own, roughly every half hour: Current state of that daemon: So: the server drops the socket on a cycle, the daemon notices (it awaits This also means the old arrangement was worse than "presence dies with the session": even inside a live session, a hand-run Nothing in the PR changes on the strength of this; the test that covers it was already there. I am recording it because I said the evidence did not exist, and now it does. — Sudoo (c8138e81) |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Requesting changes at b84331f9. Two blockers, then nits.
What passes
- The 4 new tests pass at head, and all 4 fail against main's source.
- All 31 existing
room-collab-*/sparrowd-*tests pass. I ran them in a scratch venv with pycrdt + websockets. review-checks.shon the PR diff: PASS. CI: 23 pass, 1 skipped.
Blocker 1: the 30-minute idle drop can't fire on a live connection.
room_collab_client.py:527 subscribes self._awareness.observe(lambda *_: fn()). That counts every awareness event as activity, whatever its origin or kind.
- pycrdt's
Awarenesscallsset_local_stateeveryoutdated_timeout/2to keep this client's own presence alive, and each call emits a local update. - With the default
Awareness(doc)(line 806), that timeout is 30 s, solast_activityrefreshes about every 15 s.now - last_activity >= 1800then never holds. - The same is true of any peer's renewal heartbeat: an open browser tab, or another agent's daemon, keeps the surface "active" forever.
- Probe: a real
RoomDocwith renewal running, no peer, no edits, and a 1 s awareness timeout recorded 6on_activityhits in 3 s. tests/room-collab-presence-daemon.test.pydoesn't catch this because it stubson_activityand setslast_activityby hand (~L170).
Suggested fix: count only remote document edits, plus awareness events that are remote changes (not local, and not renewal-only updates). Add one test that drives the real client, not a stub.
Blocker 2: core src/sparrowd.py now names and configures a concrete skill.
_presence_daemon_spec (L22-51) hardcodes skills/room-collab/scripts/presence_daemon.py and parses that skill's manifest.json for ROOM_COLLAB_PYTHON. It also carries the skill's interpreter policy.
docs/architecture-boundaries.md("Optional adapter capabilities") says core helpers must not name, locate, or import a concrete skill.- It also re-implements manifest-config reading, which
src/skill-manifest-config.shalready does generically.
It's arguable that sparrowd, as the composition root, may own the worker list. But this is the first skill it names, and the next skill daemon would copy the pattern. The shape that scales is a generic "skill declares a supervised worker" entry in manifest.json, which sparrowd discovers without naming any skill.
Nits
- Config lost on update.
ROOM_COLLAB_PYTHONlives in a trackedmanifest.json, and on desktop installs the engine tree is replaced on every update. So the "edit the manifest" instruction in the body doesn't survive an upgrade; only the env override does. - Unused config key.
ROOM_COLLAB_IDENTITYis declared but nothing reads it. Drop it. - Corrupt store evicts everything. In
presence_store.read_entries, aValueErrorreturns[], andreconcilethen drops every held surface. The docstring says "unreadable must not evict." (Read, not run.) - No single-instance guard on the daemon. A second copy would open duplicate sockets and both would write
room-collab-presence-live.json. (Read, not run.) desiredentries never expire. They're removed only bystay --leave, so the file andretiredgrow without bound.- Comment rules.
presence_policy.py:20reads "Owner 2026-09-22: …", but code comments may not reference people or dates.
Not verified: a real server round trip, SIGTERM/shutdown behaviour, and whether sparrowd's environment inherits exports from skill-manifest-config.sh.
|
@john-the-dev — taking both blockers. Not accepting them on your word; I will reproduce Blocker 1's probe against the real client first and report what I measure, whichever way it comes out. Blocker 2 I agree with on sight — On the nit about the corrupt store: you are right and my earlier fix was narrower than I described it. I raised — Sudoo (c8138e81) |
…r names no skill john-the-dev, CHANGES_REQUESTED at b84331f. Both blockers, and five of the six nits. Blocker 1 — the 30-minute idle drop could never fire. Reproduced before fixing: `Awareness.start()` re-sends this client's own state every outdated_timeout/2 so the server does not expire it, and `_observe_changes` counted every awareness event, so each renewal refreshed last_activity. probe, 1s timeout, no peer and no edits: 5 hits in 3s, every one kind='update' at the production default (30s): a refresh every ~15s, forever The discriminator was already in this file — the renewal sender at line 278 tests `kind`/origin to avoid re-sending a peer's state. Awareness activity is now the same test: a remote `change` only. Measured: local renewal kind='update' origin='local' -> not activity a real remote peer kind='change' origin='remote-peer' -> activity remote renewal kind='update' origin='remote-peer' -> not activity tests/room-collab-activity-filter.test.py drives the REAL client with renewal running, per the review. It is mutation-checked: restoring the old subscription byte-for-byte turns "our own presence renewal is not activity" red (4 hits in 2.5s) and restoring the fix returns 3/3. Blocker 2 — `src/sparrowd.py` no longer names a skill. `grep -c room-collab src/sparrowd.py` is 0, asserted in the suite. A skill declares its own loop: "supervised_worker": {"name", "script", "interpreter": {"config", "needs"}} and sparrowd scans skills/*/manifest.json. The refusals the old spec had are kept (no interpreter, missing interpreter, absent skill) and the generic reader replaces the private manifest-config parse. The script must resolve inside its own skill and the name must be a plain name: a manifest is attacker-adjacent. tests/sparrowd-skill-workers.test.py pins that with a synthetic skill nothing in src/ references, which is the only real proof the discovery names no skill. Nits: ROOM_COLLAB_IDENTITY dropped (nothing read it — grep is now empty). A corrupt record raises RecordUnreadable instead of returning [] and evicting every surface; absent, empty, whitespace-only and another schema still read as empty. A flock singleton guard, so two copies cannot both write `live`. The comment naming a person and a date is gone. MANIFEST.md documents the convention, including that the env override is the one that survives a desktop update. Not done: `desired` entries still never expire. That needs a policy decision (when does an un-left summon lapse?) and I am not inventing a TTL. 32 room-collab/sparrowd suites pass; hardcoded-path scan and lint-workspace-resolution clean on this diff.
|
@john-the-dev — both blockers fixed at Blocker 1 — reproduced, then fixedYour probe, independently: The discriminator was already in this file — the renewal sender at line 278 tests So it is exactly your suggested rule: remote
It also pins that a remote peer still wakes it, so the filter cannot be "fixed" by dropping awareness entirely. Blocker 2 — the supervisor names no skillYou were right that the scaling shape is a declaration, and I did not try to argue the composition-root point. A skill declares its own loop: "supervised_worker": {
"name": "room-collab-presence",
"script": "scripts/presence_daemon.py",
"interpreter": { "config": "ROOM_COLLAB_PYTHON", "needs": "pycrdt + websockets" }
}
Nits
Not done, deliberately
Your "not verified" listTwo of the three are now covered, from an earlier run on the owner's host: a real server round trip ran 2 h 51 m with 5 server-initiated 32 room-collab/sparrowd suites pass. Hardcoded-path scan and — Sudoo (c8138e81) |
…e pool wait
Two CI failures from the previous commit, both mine.
lint-skill warned "unknown manifest field 'supervised_worker'" and the job
treats a warning as a failure. The field is now known AND validated there,
which is where every other manifest field is checked: name must be a plain
name, script must exist and stay inside the skill, interpreter.config is
required, and a config key not declared in the `config` block is a warning.
`--all` is 59 manifests, 0 errors, 0 warnings.
The store suite reported TIMED OUT (>240s) on CI while passing in 10s locally,
so the timeout is the symptom and not the cause. `pool.map` is now
`map_async(...).get(timeout=120)`, because a worker exception that cannot be
pickled back leaves the parent waiting forever and the suite then blames a
timeout instead of naming the fault. Measured directly: a worker raising
RecordUnreadable inside the pool gives
MaybeEncodingError: ... PicklingError("Can't pickle
<class 'presence_store.RecordUnreadable'>: No module named 'presence_store'")
because the module is loaded by path, so its classes are not importable in a
child. With the bound, that surfaces as a named failing check.
NOT REPRODUCED: the CI timeout itself. Every check the job printed had already
passed, and 32 suites pass locally in 10s. If it recurs the bound will now say
which worker and why.
tests/watch-tasks-stream-fallback-receipt-scope.test.py also failed on that
run. It is not this PR's: nothing here touches the watcher.
The gate failed at 90% (bar 95%). Four gaps, from its own artifact rather than a guess, all mine: scripts/lint-skill.py 9.1% -> validation added with no test at all src/sparrowd.py 92.3% -> the missing-script / missing-interpreter refusals room_collab_client.py 97.1% -> on_activity, the entry the daemon calls presence_daemon.py 91.4% -> main()'s singleton refusal lint-skill.test.py gains 11 cases over the new block: well-formed accepted, non-object, path-as-name, missing script, script outside the skill, absolute path, script absent on disk, missing interpreter.config, an interpreter key not declared in `config` (warns), and an unknown field (warns). The activity test now goes through `on_activity` rather than the private `_observe_changes`, and covers all three surface kinds — the board and kanban branches were changed lines too, and the filter has to hold for each. The singleton case now also runs `main()`, which is where the refusal must happen: it returns 3 before opening a socket, so credentials it never uses are supplied. If that ordering regressed, the call would hang instead of return. That case found a bug in my first attempt, worth recording: I asserted `acquire(ws) is not None` without keeping the handle. An flock is held by the open file description, so CPython collected the handle, the close released the lock, and main() acquired it and dialled wss://example.invalid for real. The handle is now bound and closed explicitly. Locally lint-skill.py and sparrowd.py are at 100% on changed lines. The rest of the subset I can run here is not comparable to CI's number — CI instruments the whole suite, and several of these lines are reached only through other tests or through subprocesses (which need SUTANDO_TEST_SUBPROCESS_COVERAGE=1). CI is the instrument for the gate; this commit fixes what its artifact named.
…an see them The gate went 90% -> 92% but scripts/lint-skill.py stayed at exactly 9.1%, which is the tell: the cases I added were in scripts/lint-skill.test.py, and that file is run by its own CI step (`python3 scripts/lint-skill.test.py`), which is NOT the instrumented run. coverage-gate.sh measures tests/**/*.test.py only, so validation tested beside the linter reads as untested. The same twelve cases now live in tests/lint-skill-supervised-worker.test.py, and the additions to scripts/lint-skill.test.py are reverted rather than kept in both places — two copies of one assertion set drift. Measured, not assumed: with only this new file plus the one other tests/ file that touches the linter, `coverage report -m` on scripts/lint-skill.py shows no missing line in 108-138 (the changed block); the remaining misses in that window, 106/140/145/147, are outside it. That should leave presence_daemon.py's real-run path as the only uncovered changed lines (30, 168, 198, 223, 267-271, 273 — Daemon construction, asyncio.run and its finally), i.e. ~10 of 411, about 97.6%. Not covering those here: reaching them means actually running the daemon loop, and a test that fakes its way there would assert the fake. 34 suites pass. lint-skill --all: 59 manifests, 0 errors, 0 warnings.
|
@cla-assistant check |
…t deadlocks under coverage (#4693) tests/room-collab-presence-store.test.py times out on CI (>240s) while passing in 10s locally. It is red on main now, merged with #4646. The cause is not the code under test. The runner's faulthandler dump shows a Pool worker parked in the result queue while coverage's SIGTERM handler runs inside it: multiprocessing/pool.py:114 in worker multiprocessing/queues.py:386 in get multiprocessing/synchronize.py:95 in __enter__ <- non-reentrant lock held coverage/control.py:762 in _on_sigterm coverage/control.py:750 in _atexit -> stop coverage/collector.py:336 in stop .coveragerc sets `sigterm = True` and `concurrency = multiprocessing`, so tearing the pool down SIGTERMs workers that are holding the queue lock, the handler blocks acquiring it, the workers never exit, and the parent blocks in the Pool context exit. The 120s bound I added earlier could not help: the hang is in the teardown, after the map already returned. Nothing here needs a result channel — the assertion is what landed in the file, not what a worker returned. So: 16 plain Processes, each joined with a timeout, plus a check that each exited cleanly. No pool, no result queue, no interaction with the SIGTERM handler. Evidence: under `coverage run` (the CI path) rc=0 in 5s, was >240s plain run all presence-store checks ok mutation: `with _exclusive(path)` -> `if True` FAIL "all 16 concurrent registrations survive — got 14 of 16" That last line is the point: the case still proves the flock, so this is a harness change and not a weakened assertion.
Closes #4642.
What was wrong
A summon asks an agent to be somewhere. The mechanism for that —
room_collab watch— is a complete resident websocket loop (announces presence, reconnects with backoff, replays what landed while it was away). Nothing ever ran it:So an agent's presence was a foreground child of whichever session typed the command, and vanished when that session ended, compacted or restarted. Measured on this host before the fix: a
watchI started by hand survived 1 h 15 m, and dropping it removed me frompeerswithin ~12 s.Shape
presence_policy.pynowpresence_store.pypresence_daemon.pyroom_collab staysrc/sparrowd.pyOwner's parameters (2026-09-22): drop after 30 minutes with no activity on the surface, cap 16, LRU.
"On the surface" was a real fork and he chose it: someone else's keystroke counts, because a document being edited is exactly when an agent's presence is worth showing.
events(handles)answers the other question — what here is addressed to me — so it cannot supply that signal.RoomDoc.on_activity()does, and the subscription switch is now shared withevents()rather than copied.The two records, and why one writer each
desiredis written by agents under an exclusive lock;liveonly by the daemon. The lock covers the whole read-modify-write: locking only the write still loses a registration, which the concurrency test demonstrates by running the production writer in 16 real subprocesses (flockis per open file description — threads would prove nothing).Both publish through a temp file plus
os.replace. A reader polling during a truncate-in-place sees a zero-length file, which parses as "nothing is connected".idleandcappedare separate drop reasons because only one needs a new summon to come back. A surface that idled out must NOT be rejoined by the next pass, or the timeout means nothing.Two defects this found in code written earlier in the same branch
1.
planundid its own idle drop. The entry it had just timed out was stillCONNECTEDin the record it was reading, so the same pass queued it straight back —drop: [(key, 'idle')]andconnect: [same key]together. The policy suite asserted the drop and passed throughout: asserting a drop is not asserting the surface goes away. Found by building the daemon on top of it, pinned by...and NOT reconnected in the same pass.2. The daemon mixed two clocks.
reconcile(now)took the time as an argument whileholdstampedtime.time(). In production they agree by luck; the idle rule could not be tested at all. One injectable clock now, and a mutant reverting it is killed.Evidence
The skill's existing suites on its venv, unchanged:
watch,summon,cli,board-client,kanban-client,composer-client,presence-ownership,presence-name-default— all ok. The client refactor is behaviour-preserving:kanban-convergencerun against the unmodified client produces byte-identical output.stayexercised end to end, no token and no network:Mutation
15 mutants across the three suites, each killed by its own assertion and no other. The load-bearing ones:
all 16 concurrent registrations surviveos.replacea rewrite replaces the file rather than truncating it...and NOT reconnected in the same passhold()stamps its own clockleftremembered like idle...and nothing is remembered about itthe live record is published for the next passAn earlier batched mutation harness mis-attributed one kill because its restore between mutants was unreliable; every row above was re-run individually with the baseline verified clean before and after.
The open question, not silently decided
sparrowdrefuses to supervise the daemon rather than guess an interpreter. The skill importspycrdtandwebsockets; the core's own python is not required to have them, and started under the wrong one the daemon would crash-loop under the supervisor. So:Declared in the skill's
manifest.jsonperskills/MANIFEST.md, not as an ad-hoc env var. A deployer must set it; until then this changes nothing on any host, which is deliberate for a first landing.ROOM_COLLAB_IDENTITYis declared and unused — the daemon currently takes the identity from each registration. Left as a config surface for a host that runs one daemon for several seats; say the word and I will remove it rather than ship an unused key.Not verified here
The daemon has not held a real surface under
sparrowd— the supervisor path needs a host with the interpreter configured, and I did not configure one on the owner's machine. Every layer below it is exercised: the policy and store against fixtures, the daemon against a stubbed client,stayagainst a real record, andsparrowd's spec in all three configurations.Once it runs for real, the summon card gains a third state — was in, then left — which today renders as "not in yet". That is a one-line client change and it is meaningless until this lands, so it is not in here.