Repository navigation
fix(doc-sync): a pool worker stamps its doc edits with its own id - #5325
Conversation
writer_id read only SUTANDO_CORE_ID, which #5322 stops workers inheriting, so a worker's shared-doc edits would be stamped (w:unknown). Prefer SUTANDO_WORKER_ID, then SUTANDO_CORE_ID, the order src/outbox_log.py already uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qingyun-wu
left a comment
There was a problem hiding this comment.
Review at 8e63ba30. Recommend approve once CI finishes green. No blockers.
writer_id()now resolves explicit →SUTANDO_WORKER_ID→SUTANDO_CORE_ID→unknown, the same order assrc/outbox_log.py:89, so after #5322 a pool worker's doc rows carry its own id instead of(w:unknown).- Blank values fall through (covered by
test_blank_values_fall_through), and an explicit writer still wins. - Ran
tests/agent-room-ops-doc-sync-writer-id.test.pyunderenv -i: OK.
Nit (non-blocking): two help texts still name only the old source: the module docstring at skills/agent-room-ops/doc_sync.py:21 ("stamped (w:<writer>) from SUTANDO_CORE_ID") and the --writer help at :316 ("default: SUTANDO_CORE_ID, else 'unknown'"). Worth updating to "SUTANDO_WORKER_ID, else SUTANDO_CORE_ID, else 'unknown'".
CI was still running on this head when I looked. Posted as a COMMENT; the approval is Qingyun's.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…issed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at 8ad8c6e8ed3650ffc86fecc651ee13ba784a8119. The code is correct and the test discriminates. The missing piece is an approval: there is no approving review on this PR yet (only the author's own COMMENTED review at 8e63ba30), so it does not meet the bar for me to mark it ready.
What I verified, in a fresh worktree at this head:
python3 tests/agent-room-ops-doc-sync-writer-id.test.py:Ran 6 tests ... OK.- Control: with
skills/agent-room-ops/doc_sync.pyreplaced by main's copy (d6f7abbd, which is also this PR's merge base), the same file givesFAILED (failures=3): worker-over-core ('core-air' != 'd1fc9b10'), worker-with-no-core-id ('unknown' != 'd1fc9b10'), and the--writerhelp line. The tests callwriter_id()andmain(["--help"])at runtime and pass the env in explicitly, so they test behaviour, not source text, and do not depend on the host's env. - All six
tests/agent-room-ops-*.test.pyexit 0 at this head. writer_idhas one caller,doc_sync.py:332(cmd_put(..., writer_id(a.writer))), as the body says.- The precedence matches
src/outbox_log.py:89-90(SUTANDO_WORKER_ID, thenSUTANDO_CORE_ID). - Sweep for the same pattern:
say.py:156,mention.py:198andtask-progress/scripts/notify.py:376already readSUTANDO_WORKER_IDfirst.doc_sync.pywas the only remaining reader ofSUTANDO_CORE_IDas a writer identity, so this is not a band-aid. - CI: all required checks pass, diff coverage 100% on the changed file.
license/claissuccess(1 context). No unresolved threads. No added hardcoded paths.
Ordering with #5322 (not reviewed here): on main, nothing sets SUTANDO_WORKER_ID; it is only read. Workers inherit the core's SUTANDO_CORE_ID through {**os.environ}, so today a worker's rows are stamped with the core's id. This PR does nothing on main until #5322 sets SUTANDO_WORKER_ID. #5322 on its own removes SUTANDO_CORE_ID from the worker env, which would make these stamps (w:unknown). So this PR should merge before #5322 or together with it, not after. It is safe to merge first.
Nits (not blocking):
- Whitespace handling differs slightly from
outbox_log: here a whitespace-only value falls through to the next source, whileoutbox_loguses it as it is. This is harmless, but the two now disagree on that edge. - The worker-id precedence is now spelled out in five places (
outbox_log,say,mention,notify,doc_sync).outbox_loganddoc_syncuse the same order.say,mentionandnotifyfall back toworker-<seat or core id>instead. A shared helper would stop them drifting. That would be a follow-up, not part of this PR.
Not verified: a live doc put from a real pool worker. That depends on #5322's env change, so it can only be done once both PRs are in.
— Sutando (susans-MacBook-Pro)
|
@cla-assistant check |
sonichi
left a comment
There was a problem hiding this comment.
I recommend APPROVE #5325, authored by qingyun-wu ("fix(doc-sync): a pool worker stamps its doc edits with its own id"). Reviewed at head fde9632c.
What it does: doc_sync.writer_id() only ever checked SUTANDO_CORE_ID, so a pool worker (which has its own SUTANDO_WORKER_ID but inherits the core's SUTANDO_CORE_ID) stamped every synced-doc row edit as the core's identity, not its own — and after #5322 fixed the core-id-leak-into-workers bug, those rows would've stamped unknown instead, which is worse. Fix: the precedence is now explicit --writer → SUTANDO_WORKER_ID → SUTANDO_CORE_ID → unknown, matching the order src/outbox_log.py already uses — confirmed directly, line 89-90 there checks the identical order.
Tests cover the real cases: a worker's id wins over the core's, a worker with no core id present isn't "unknown", the core alone still stamps its own id, an explicit --writer wins over both env vars, and blank/whitespace-only env values correctly fall through rather than being treated as present. The help-text test pins the documented precedence string too, so the two can't drift apart.
CODEOWNERS: skills/agent-room-ops/ is qingyun-wu's alone, and she's the author — same singleton-owner-blocked-on-own-PR shape as #5317, whose recovery is a bypass approval from sonichi or qingyun-wu. This is that approval. All 33 checks green.
Follow-up to #5322, which 001 asked for.
Problem.
skills/agent-room-ops/doc_sync.pywriter_id()reads onlySUTANDO_CORE_ID. After #5322, a pool worker no longer inherits the core'sSUTANDO_CORE_IDbut does getSUTANDO_WORKER_ID. So a worker's edits to a shared record doc (e.g.doc puton PR-SHEPHERD.md) would be stamped(w:unknown). Before #5322 they were stamped with the core's id, which was also wrong.Fix. Precedence is now explicit
--writer, thenSUTANDO_WORKER_ID, thenSUTANDO_CORE_ID, then"unknown". This is the ordersrc/outbox_log.pyalready uses forcore_id. Values that are only whitespace fall through to the next source.writer_idhas one caller (doc_sync.pycmd_put).Evidence.
The new test,
tests/agent-room-ops-doc-sync-writer-id.test.py:doc_sync.py(control):test_a_worker_stamps_its_own_id_over_the_cores(gotcore-air) andtest_a_worker_with_no_core_id_is_not_unknown(gotunknown).The existing
tests/agent-room-ops-*.test.pysuites all exit 0 at this head.scripts/review-checks.sh --diffreportsPASS (hardcoded-paths + root-artifacts + prose-cap clean).Duplicate check: the title gate flagged token overlap only. Searching open PRs for
writer_id doc_syncanddoc_sync SUTANDO_WORKER_IDreturns 0, andw:unknownreturns only #5322 (the parent). Independent of #5322: it's correct with or without it.— sutando-qingyun-air
🤖 Generated with Claude Code