Repository navigation
fix(watcher): a foreign SUTANDO_INSTANCE_ID on the core's own inbox is cleared, not trusted - #4976
Conversation
…s cleared, not trusted A --force-restart (or any start) issued from inside another instance's own session inherits that instance's SUTANDO_INSTANCE_ID, even when --inbox explicitly names the core's own canonical <ws>/tasks. The watcher then watches the right directory while believing itself to be that OTHER worker's own inbox-watcher (#4502's rule: a worker's own inbox IS the routing decision) -- it never loads the core's task-event-handler config and silently stops dispatching anything it sees, with no error anywhere. Observed in production 2026-09-30, twice: pid 1184 (18:46) and its replacement pid 66624 (19:16), both with argv correctly reading `--role session --inbox <core's workspace>/tasks` but env carrying a different worker's SUTANDO_INSTANCE_ID, inherited from the shell that force-restarted them. Fix is deliberately narrow: only clears an explicitly-set SUTANDO_INSTANCE_ID when the inbox being served is the core's own canonical tasks/ dir (the same basename=="tasks" signal the existing identity-match gate already uses). It never touches a <ws>/deliveries/<id> inbox -- there, the pre-existing identity-match check already decides whether a mismatched starter must yield, and correcting the env there would make a wrong starter's identity agree with the inbox by fiat, defeating that check (this was my first, too-broad attempt; tests/watch-tasks-stream-unready-holder-restamp.test.sh scenario (d) and tests/watch-tasks-stream-inbox-and-workspace-env.test.py caught it). It also never invents an identity when SUTANDO_INSTANCE_ID was never set, since several tests legitimately run a core against a deliveries/-shaped inbox with no ambient identity at all. tests/watch-tasks-stream-config-hot-reload.test.py's worker case previously set SUTANDO_INSTANCE_ID="worker-1" while watching the bare <ws>/tasks dir -- a combination that cannot occur in production. Updated it to a realistic <ws>/deliveries/worker-1 inbox, which is what makes it a test of "a worker", not of "SUTANDO_INSTANCE_ID happens to be set". New tests/watch-tasks-stream-foreign-instance-id-corrected.test.py pins the fix directly: a foreign SUTANDO_INSTANCE_ID on the core's own inbox is corrected (an already-present handler still runs, and the correction is logged); control run against unmodified main fails cleanly, reproducing the incident exactly (TASK_FILE emitted, handler never runs). Full watch-tasks-stream*/watcher-* suite (29 files) re-run green except the pre-existing watch-tasks-stream-fallback-receipt-scope flake (confirmed failing identically on unmodified main; tracked at #4645) -- unrelated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
review-checks' prose-cap caught three blocks over the cap (two in the hot-reload test, one in the new regression test) and the main fix's own .sh comment, which the Python-only scanner doesn't cover but the same policy applies to. The narrative moved to the PR body; code comments keep only the constraint and a pointer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cope worker cases Same gap as the hot-reload test fix: three of this file's six properties set SUTANDO_INSTANCE_ID="worker-1" while watching the bare <ws>/tasks dir — a combination that cannot occur in production and that the watcher's new foreign-identity correction (this branch's main fix) now treats as a mismatch on the core's own inbox, clearing it and breaking all three "a worker bypasses unconditionally" assertions. Caught by CI on this PR, not locally: my earlier local re-run of this file only surfaced the one already-known flake (#4645) in a truncated tail of the output and I didn't look past it — the full output showed 2/6 passed, not 5/6, meaning three genuine regressions were sitting unnoticed. CI's full log is what caught it. Fix: `run()` now takes the inbox shape from `watcher_instance` itself — <ws>/deliveries/<id> when set, <ws>/tasks when not — matching production and the hot-reload test's same fix. Back to 5/6 passed, identical to unmodified main (the one remaining failure is #4645, confirmed unrelated). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Verified at exact head
Not verified by me: the live |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 68e63b66dcf75b6cd7b26b01b11d69336f2f86f7.
The fix is correct and narrow. src/watch-tasks-stream.sh:113-118 unsets an inherited SUTANDO_INSTANCE_ID only when the canonical inbox's basename is tasks. That is the same rule the existing identity check at :215-216 already applies: an instance id must equal basename(inbox), and an empty id goes with the inbox named tasks.
TASKS_DIR_ABSis assigned at:103, before the new block, so it uses the canonicalised path.- I checked that no pool worker serves an inbox named
tasks.spawn_worker.py:185andpool_remedy.py:216both setSUTANDO_TASKS_DIRtodeliveries_dir(ws, id), which is<ws>/deliveries/<id>, and the id is never the literaltasks. So a real worker is never stripped of its identity. - The cleared id doesn't leak elsewhere. The only other readers in
src/arecheck-pending-tasks.shandcodex/cli/task-notifier.sh, and both run in their own process environment. - The two test-file edits only change the worker cases to use the real
deliveries/<id>inbox shape, which the new rule requires. That is a fixture correction, not a weakened assertion.
What I ran (macOS, bash 3.2 /bin/bash):
- New test
watch-tasks-stream-foreign-instance-id-corrected: 1/2 at head, 0/2 with onlysrc/watch-tasks-stream.shreverted to the merge-base. Case (2), "the correction is logged", flips from fail to pass, so the test does detect the change. - Case (1), "the already-present handler still runs": fails on my machine both at head and on main. The same handler-routing step also fails on main in
config-hot-reload(2/3) andfallback-receipt-scope(4/6), with identical counts at head and main. My environment can't exercise handler routing, so it isn't a PR regression. For that path I'm relying on your run and on CI. - CI on this head: 23 pass, 1 pending, 1 skipped.
Not verified by me: the handler actually running on the corrected core inbox, and the production repro.
|
CI at 68e63b6: the one red lane is |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 68e63b66dcf75b6cd7b26b01b11d69336f2f86f7. CI and CLA are green, but this is not merge-ready because the production failure still reproduces and the new inbox predicate misclassifies a valid worker inbox.
Blocker 1 — clearing only the ID leaves the inherited worker resolver active. The real worker environment exports SUTANDO_INBOX_RESOLVER (spawn_worker.py:181-208, forwarded at launch-worker-session.sh:73-86). This patch unsets only SUTANDO_INSTANCE_ID at watch-tasks-stream.sh:113-118, then still invokes the inherited delivery resolver before any handler or TASK_FILE emission at watch-tasks-stream.sh:830-865.
I reran the added exact-head test with the actual inherited routing variables (SUTANDO_WORKSPACE_DIR, SUTANDO_INBOX_KIND=deliveries, SUTANDO_INBOX_RESOLVER, and SUTANDO_POOL_DELIVERY_SCRIPT). The watcher logged that it cleared the foreign ID, then rejected <ws>/tasks/task-one.txt through the worker resolver three times and printed not dispatching; the handler log and stdout both remained empty. Result: 1/2 passed. The checked-in test passes because it models only the foreign ID, not the worker environment whose inheritance caused the incident.
Blocker 2 — basename == tasks is not a canonical-core-inbox check. tasks is a valid worker/recipient ID under the production grammar. At this head, watching <ws>/deliveries/tasks with SUTANDO_INSTANCE_ID=tasks clears the worker identity and runs the core handler (probe, handle) instead of preserving worker routing. That contradicts the stated “never a deliveries/ inbox” invariant. Please classify the canonical core inbox structurally and add this adjacent negative case.
Please normalize the inherited worker-only inbox-routing context at the canonical core boundary and cover the full production worker environment, not just SUTANDO_INSTANCE_ID. Per REVIEW.md's live-path criterion, the updated head also needs a real post-restart round trip from a contaminated worker shell through task receipt and delivered reply; the current PR body documents only the unpatched mitigation, and the exact-head comment explicitly says that round trip was not verified.
Worst case as written: the restart appears corrected in logs while every core task is still silently discarded; the basename collision can also route a worker delivery through the core handler.
…h not basename
kewei's PR review on this branch's prior head found two blockers:
1. Clearing only SUTANDO_INSTANCE_ID left SUTANDO_INBOX_KIND/
SUTANDO_INBOX_RESOLVER/SUTANDO_INBOX_RESOLVER_TIMEOUT/
SUTANDO_POOL_DELIVERY_SCRIPT behind -- a worker-shaped SUTANDO_INBOX_RESOLVER
still runs on every entry at the core's inbox, rejects every plain task body
(it expects a worker sentinel), and dispatch_task's 3-attempt retry loop
gives up silently. Now the whole inherited worker-routing block is cleared
together.
2. The canonical-inbox check compared basenames ("tasks" == "tasks"), which
can misclassify a worker's own inbox if its path happens to end in
.../tasks. Now it compares the inbox's realpath against
<resolved workspace>/tasks, matching tasks-dir-resolve.sh's own notion of
the core's canonical inbox.
tests/watch-tasks-stream-foreign-instance-id-corrected.test.py grows two
properties for these: (3) a full inherited worker env at the core's canonical
inbox (incl. a real SUTANDO_INBOX_RESOLVER) still dispatches; (4)/(4b) a
worker's own inbox whose basename happens to be "tasks" keeps its identity.
5/5 pass.
Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed Blocker 1 (incomplete env clearing) — the fix now clears the whole inherited worker-routing block together ( Blocker 2 (basename-only match) — the canonical-inbox check now compares the inbox's realpath against Live post-restart round trip (not just the test harness — a real watcher process, real fswatch, a fully contaminated worker env built with Before the fix (built from At this branch's head ( Both runs used the same env: Tests: (Separately: while verifying this locally I found |
|
Verified at exact head
One thing to know, not a blocker: @keweichen both blockers are addressed at this head; could you re-review? @john-the-dev your approval is at |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
COMMENT at f31f10ef65a4a627385a558cac9e927dd596cb7e. The code is right. I'm holding my approval on the live-path evidence, which is the one part of keweichen's request this head doesn't close.
First, a correction to my earlier review: my approval at 68e63b66d missed both of the defects keweichen found. I checked that worker inboxes are deliveries/<id> but never considered tasks as an id, and I never followed the inherited resolver. Thanks for catching both.
Code: both blockers are fixed.
- Realpath, not basename.
watch-tasks-stream.sh:115-116now comparesrealpath(inbox)againstcanonical_tasks_dir("$WORKSPACE_DIR/tasks"). - Full routing env. The
unsetnow coversINSTANCE_ID,INBOX_KIND,INBOX_RESOLVER,INBOX_RESOLVER_TIMEOUTandPOOL_DELIVERY_SCRIPT. I checked that set against whatspawn_worker.pyandlaunch-worker-session.shexport. - Variables left set, and why that's fine:
RESULTS_DIRandWORKSPACE_DIRname the shared workspace (pool_delivery.results_diris<ws>/results).SUTANDO_TMUX_SESSIONalso survives, but its only consumer,_tmux_wake(:984), has no caller at this head.
What I ran (macOS, bash 3.2):
- New test at head: 5/5.
- With only
src/watch-tasks-stream.shat the merge-base: 2/5; (1), (2) and (3) fail. - With only the predicate put back to
basename == tasks(unset list kept): 3/5; (4) and (4b) fail. - So each property detects its own fix.
- CI: 25 pass, 1 pending.
Not closed: the live path. keweichen asked for a real post-restart round trip from a contaminated worker shell, through task receipt to a delivered reply. As far as I can find, the evidence at this head is still:
- The PR body's "Live path" section. It describes force-restarting from an uncontaminated shell, which is the mitigation, not this patch.
- The
env -iharness runs. These are tests, not the production watcher.
The PR body also still describes the removed basename == "tasks" fix and the old 2/2 test, so anyone reading it gets the wrong mechanism. I'll approve at this head once the body is updated and it shows one live run: start the patched watcher from a shell with a worker's full env exported, drop a real task into <ws>/tasks, and show it dispatched and answered (the watcher's stderr correction line, plus the result file / delivered reply).
|
Body updated at the same head |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at f31f10ef65a4a627385a558cac9e927dd596cb7e. The head is unchanged since my COMMENT (review 5374739108), so the code and test evidence there still stand:
- New test: 5/5 at head; 2/5 with only the source at the merge-base; 3/5 with only the predicate put back to
basename == tasks. - CI is green.
Both parts of my hold are done in the body:
- Mechanism. The Fix section now describes the realpath comparison and the five properties. The remaining
basenamementions only explain why basename alone is wrong, plus the honest residual that withSUTANDO_WORKSPACE_DIRunset the check reduces to the old set. I agree that residual is non-blocking, since every spawned worker exports it. - Live run. Live round trip #2 starts the patched watcher from a worker's real, unmodified
SUTANDO_*env, with only the workspace and results dirs redirected to scratch. It drops a task, shows the stderr correction line, and shows the handler's result file. That is the run I asked for.- It runs against a scratch workspace, not the host's live
tasks/. I think that's the right trade-off, and I'm not asking for a run against production. - I read the run from the body. I did not reproduce it.
- It runs against a scratch workspace, not the host's live
keweichen's CHANGES_REQUESTED at 68e63b66d is theirs to lift. This approval doesn't speak for them.
There was a problem hiding this comment.
Re-reviewed exact head f31f10ef65a4a627385a558cac9e927dd596cb7e. The two original dispatch blockers are fixed in the main path: an isolated-log full-worker-env probe dispatches at this head and fails at the partial fix, and <ws>/deliveries/tasks retains worker routing. CI and CLA are green. Changes are still requested for the following current-head blockers.
Blocker 1 — the same core-inbox decision still falls back to basename == tasks in the unready-holder recovery path. The new canonical predicate is established at watch-tasks-stream.sh:115-118, but the live-holder re-stamp at lines 213-219 independently decides that an empty instance ID is core only when the physical inbox basename is tasks.
Exact-head repro: I made <ws>/tasks a symlink to a physical directory named physical-inbox, started a session holder, removed its readiness sentinel, then started a second session watcher. The second watcher correctly found the live holder and emitted WATCHER_HELD, but it left the sentinel absent and printed no re-stamped line. The same harness with an ordinary physical <ws>/tasks restored the sentinel to the holder PID and logged the re-stamp. Reuse the canonical-core predicate here and add this adjacent case; otherwise a supported physical-path spelling leaves the live core watcher without the readiness record this recovery path promises.
Blocker 2 — regression property (3) can pass without the full-env task being dispatched. At the wait/assert on lines 178-188, log already contains handle from property (1), so the predicate is true before watcher p3 processes task-full-env.txt.
I ran the current test body against exact prior head 68e63b66, whose resolver leak is known and visible in stderr. Property (3) still printed ok while the resolver rejected the task three times and logged not dispatching. Resetting the log immediately before property (3) made that same source fail property (3), while f31f10ef remained 5/5. Please give this property fresh state or assert a new/task-specific handler record so it actually guards the bug it names.
The rewritten body improves the live evidence, but the live-path gate remains open. I have now read live run #2: it starts the exact-head role=session watcher from this worker shell's real inherited SUTANDO_* environment, redirects workspace/results to a fresh scratch workspace, drops a task, and shows the scratch handler's result file. This resolves the stale-body point in the initially submitted version of this review; the body now accurately describes the current mechanism and tests.
Run #2 is still a standalone scratch integration, not a restart of the shipped/running service: it does not exercise the supervisor/--force-restart lifecycle or real resolved workspace, and no result consumer/outbox/bridge claims the file or delivers a reply to the origin. Per REVIEW.md, approval still needs an exact-head post-restart round trip through the shipped service from contaminated worker environment to task receipt, result production and consumption, and delivered reply, with restoration/downtime noted.
Verdict: not merge-ready. The worst case is that the exact silent-dispatch regression can return while its named test stays green, and a symlinked core inbox cannot repair its readiness record after a live-holder handoff.
… fix a weak test assertion Two more findings from kewei's formal review at f31f10e: 1. The unready-holder re-stamp path (around line 215) still decided "empty instance id means core" by `basename(inbox) == "tasks"`, even though the dispatch-time check a few lines above it was already switched to a realpath comparison. A `<ws>/tasks` symlinked to a physically-named directory (e.g. `physical-inbox`) has `basename(TASKS_DIR_ABS) == "physical-inbox"`, so the re-stamp silently never fired for a holder on a symlinked core inbox. Now reuses the same `CANONICAL_CORE_TASKS_DIR` realpath comparison already computed earlier in the file. New case (e) in tests/watch-tasks-stream-unready-holder-restamp.test.sh pins this: a symlinked core inbox re-stamps exactly like a physical one — confirmed to fail on the basename-only version and pass on the fix. 2. tests/watch-tasks-stream-foreign-instance-id-corrected.test.py property (3) asserted `"handle" in log`, but the log is shared across properties and property (1) already wrote "handle" to it — so (3) passed trivially even when its own task (task-full-env.txt) was never dispatched. The test handler now records which task file it actually ran for, and (1)/(3) each assert their own task by name. Confirmed: at the pre-Blocker-1-fix head (68e63b6) this version of (3) now correctly FAILS (it previously passed spuriously). 5/5 on tests/watch-tasks-stream-foreign-instance-id-corrected.test.py, all green on tests/watch-tasks-stream-unready-holder-restamp.test.sh. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Verified at exact head
Live-path gate: Chi has authorised me to run the shipped-service round trip on this host's real workspace (restart of the core's watcher from a contaminated worker shell, at this exact head, in a quiet period); it will be posted here with the task → result → delivered-reply transcript and the downtime, once the core has a quiet window. @keweichen both current-head blockers are addressed with your repros as pinned cases; @john-the-dev your approval was at |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
COMMENT at 9ef25f6e7e2987f79007a6c1d90644af70eca756. The code is right. I'll approve once the live-path run lands.
keweichen's two new blockers are fixed, and I checked both on macOS with bash 3.2:
- Re-stamp path.
watch-tasks-stream.sh:217now uses"$TASKS_DIR_ABS" = "$CANONICAL_CORE_TASKS_DIR", assigned at:115. New case (e) inunready-holder-restamp.test.shcovers a symlinked<ws>/tasks→physical-inbox.- The suite passes at head.
- With only the source at
f31f10ef6, (e) fails: the sentinel is empty and nothing is re-stamped. The stderr shows the second watcher yielding to the live holder onphysical-inboxwithout restoring the record, which is exactly keweichen's repro.
- Property (3) is task-specific now. The stub handler logs
handle <task basename>, and (1) and (3) each wait for their own task.- 5/5 at head.
- 2/5 with only the source at
68e63b66d, with (3) failing as it should. On my f31f10e run it would have passed on (1)'s stale line.
- CI right now: 20 pass, 5 pending.
Why COMMENT and not APPROVE: REVIEW.md #15. "A live-path PR is not approvable on harness proof alone … never grounds to approve without it". This PR changes the watcher's startup path, so it needs a task to flow end-to-end through the restarted, shipped service.
- My APPROVE at
f31f10ef6accepted the scratch-workspace run #2. That was below this bar, and keweichen was right to hold it. - You say the real-workspace run is authorised for a quiet window. When it's posted, I'll approve at whatever head it ran on, provided it shows these four things:
- The watcher restarted from a contaminated worker shell.
- The stderr correction line.
- A real task in the core's
tasks/. - The reply delivered to its origin room, with the downtime noted.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 9ef25f6e7e2987f79007a6c1d90644af70eca756. The two runtime blockers from my f31f10ef6 review are fixed:
watch-tasks-stream.sh:215-218now uses the same canonical realpath comparison for the empty-instance/core re-stamp path. The new symlinked-inbox case (e) re-stamped the holder at this head; the same case fails with thef31f10ef6source. On this host, the file's unrelated (a)/(b) process-table probes returnedunknownat both heads, while every new (e) assertion passed; both CI shell lanes are green.watch-tasks-stream-foreign-instance-id-corrected.test.py:134-144now records the handled task basename, and property (3) waits fortask-full-env.txt. It is 5/5 here and the strengthened property fails against68e63b66d, so the stale-log false positive is closed. Adjacent config-hot-reload and fallback-receipt suites passed 3/3 and 6/6.
CI, CLA, git diff --check, syntax checks, and review-checks.sh are green. Changes are still requested for three current-head gates:
Blocker 1 — the new re-stamp predicate regresses a supported custom core inbox. watch-tasks-stream.sh:217 now equates an empty-instance/core seat with the default physical <workspace>/tasks. The watcher explicitly supports the inbox and workspace being named independently (contract and rationale); that contract suite is 4/4 at this head.
Exact probe with SUTANDO_WORKSPACE_DIR=<ws>, no instance ID, and an explicit <custom>/tasks inbox: start a session holder, remove its <ws>/state readiness sentinel, then start a second session watcher on the same inbox. At f31f10ef6, the challenger emits WATCHER_HELD, restores the holder PID, and logs re-stamped. At 9ef25f6e7, it still yields to the live holder but leaves the sentinel absent and emits no re-stamp line. I reproduced both outcomes independently. Core ownership cannot be inferred only from the default inbox location; preserve recovery for a configured/explicit core-owned inbox and add this separate-workspace case to the regression test.
Blocker 2 — the required shipped-service witness is still pending. The PR's latest comment explicitly says the exact-head live run will follow. The body currently contains standalone scratch-workspace watcher runs from the prior head; no restarted shipped service consumes the result and delivers a reply. Because this changes the startup/delivery loop, REVIEW.md lesson 15 and CONTRIBUTING.md require an exact-head post-restart task → produced result → consumed result → delivered reply witness, with downtime/restoration noted. The default-path code evidence is now good; this live witness remains the runtime gate.
Blocker 3 — added comments carry review/incident provenance that the repository forbids. AGENTS.md:45 caps code comments at two lines and says PR, issue, person, and incident history belongs in the commit/PR body. This exact commit adds a three-line comment naming “kewei's review (sutando#4976)” at watch-tasks-stream-unready-holder-restamp.test.sh:119-121; the cumulative diff also adds #4502, incident, and “Blocker 1 (kewei)” provenance in the watcher tests (for example foreign-instance-id-corrected.test.py:150-173 and config-hot-reload.test.py:43-45). The new module docstring also still says “Two properties” while the executable test reports five. Keep the non-obvious behavioral constraints, but move provenance/history to the PR body, make the summary match the test, and keep each inline comment within the two-line cap.
Verdict: not merge-ready yet. The worst-case disruption remains a silently dead or permanently unready core task inbox: the default-path implementation and negative controls now look correct, but a supported custom-inbox path regressed and the shipped path has no exact-head witness. Once those are resolved and the in-code provenance is trimmed, I can re-check the current head for approval. The existing john-the-dev approval predates this commit, so the two-current-maintainer-approval merge gate is also not yet met.
…ox location; trim comment provenance
Blocker 1: the re-stamp arm's "empty SUTANDO_INSTANCE_ID means core" check was
changed in the prior commit to also require the inbox to equal the canonical
<workspace>/tasks by realpath. That regressed a SUPPORTED configuration: a
core watching a custom inbox with its own SUTANDO_WORKSPACE_DIR and no
instance id (pinned by tests/watch-tasks-stream-inbox-and-workspace-env.test.py).
The re-stamp arm's question is "is this seat the core?", which an empty
instance id already answers on its own -- the inbox-location check belongs
only to the earlier env-clearing block (deciding whether an INHERITED id is
foreign), not here, where `sentinel-names-pid` already scopes the write to
this seat's own sentinel. Dropped the inbox condition from the empty-id arm
entirely. New case (f) in watch-tasks-stream-unready-holder-restamp.test.sh
pins a custom inbox + separate workspace re-stamping correctly; confirmed to
fail against the prior commit's realpath-gated version and pass on the fix.
Blocker 3: trimmed comment references to the reviewer, review round, and
issue numbers in tests/watch-tasks-stream-{config-hot-reload,
foreign-instance-id-corrected,unready-holder-restamp}, keeping only the
behavioral constraint each comment exists to state. Updated the
foreign-instance-id-corrected module docstring, which still described two
properties, to describe all five.
Blocker 2 (the shipped-service live witness) is being run separately against
this head once pushed.
5/5 on the corrected-id test, all green (including new case (f)) on the
re-stamp test.
Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…orrected docstring The module docstring still carried an issue number, a date and a pid in its module docstring, matching exactly what AGENTS.md's comment rule forbids. Kept the behavioral statement (a worker's own inbox is the routing decision), dropped the issue number, date and pid. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t-scope's docstring My own grep for leftover provenance used a #49[0-9][0-9] pattern, which never matched #4502 -- missed by Pro-dev's independent check, not mine. The pre-existing #4502/#4503 reference at the top of the file (predating this PR) is untouched; only the one this PR itself added to run()'s docstring is trimmed. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
CI at |
|
Live witness at exact head Receipt → result → delivered reply: 50 s end to end. Downtime: the core's own session-role watcher was absent 06:31:06 → 06:36:16 (5 m 10 s); during it the PR-head watcher served the inbox until 06:32:43, then standbys from 06:33:31. Two other task files present at open were announced by the PR-head watcher to the worker's stream instead of the core's (one was Pro-dev's own pool-ask to the core, "no reply needed"); both were archived with results by the time the core's watcher returned — no task was lost. The same restart on An earlier attempt at the previous head @keweichen this is the shipped-service witness from your review; @john-the-dev your approval is at |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at ab95b0f01eb7ce4e3ecd5615461c6ebaedab47de. This supersedes my COMMENT at 9ef25f6e7, where the open item was the live run.
keweichen's three 9ef25f6e7 blockers, checked at this head
- Custom core inbox: fixed.
- In
watch-tasks-stream.sh:216the re-stamp arm now treats an emptySUTANDO_INSTANCE_IDalone as "this seat is the core". It no longer also requiresTASKS_DIR_ABS == CANONICAL_CORE_TASKS_DIR. - New case (f) in
watch-tasks-stream-unready-holder-restamp.test.shcovers a custom inbox with a separateSUTANDO_WORKSPACE_DIRand no id. - I ran it in a fresh clone:
- At head: ALL TESTS PASS.
- With only
src/watch-tasks-stream.shreverted to9ef25f6e7: (f) fails on both the sentinel re-stamp and there-stampedstderr line. The new case reproduces kewei's probe.
- The relaxation is safe: with no instance id the seat is the core by definition, and the write still goes only to
$WORKSPACE_DIR/state.
- In
- Shipped-service witness: provided at the exact head, in the comment of 2026-10-01T13:37Z. It meets REVIEW.md #15:
- The real workspace's live watcher was force-restarted from a contaminated worker shell (
SUTANDO_INSTANCE_ID,INBOX_KIND=deliveries, resolver and pool paths all set). - The correction line was emitted, and the core sentinel was stamped to the new pid.
- A real owner message arrived with its bridge envelope, was routed, answered (
results/task-e25cf5d2…) and delivered to the DM ($V3mX3…), 50 s end to end. - Downtime was stated (5 m 10 s without a session-role watcher), with standby coverage and restoration.
- The real workspace's live watcher was force-restarted from a contaminated worker shell (
- Provenance: trimmed.
- The added lines carry no
kewei,#NNNN,sutando#,incidentorBlocker 1text; I grepped every+line ofgh pr diff. - The module docstring now says "Five properties".
- The added lines carry no
CI: 27 pass, 1 skipped.
Non-blocking
- During the witness window, two core task files were announced to the worker shell's stream, because that shell had started the PR-head watcher. Both still got results. This comes from running the restart from a worker shell, not from the code. It is still worth a line in the runbook: whoever runs
--force-restarton the core inbox owns its announcements until the core re-arms its own watcher.
Merge gate: keweichen's CHANGES_REQUESTED at 9ef25f6e7 stands until kewei re-reviews. CODEOWNERS approval for src/ (qingyun-wu) is still needed, as the comment says.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ab95b0f01eb7ce4e3ecd5615461c6ebaedab47de. The runtime blockers from my 9ef25f6e7 review are resolved:
watch-tasks-stream.sh:215-217now treats an empty instance ID as the core seat without tying ownership to the default inbox location. In the focused separate-workspace/custom-inbox repro,9ef25f6e7yielded but left the sentinel absent (restamped_log=no); this head restored the holder PID and logged the re-stamp. New case (f) passed, as did the symlinked-core case (e).- The task-specific full-worker-env regression remains 5/5, config hot reload 3/3, fallback receipt scope 6/6, and the independent inbox/workspace contract 4/4. On this host the unready-holder file's pre-existing (a)/(b) process-table probes still returned
unknownat both old and new heads; every changed (e)/(f) assertion passed, and both CI shell lanes are green. - The previously cited review/incident provenance is removed and the new regression module now accurately documents all five properties.
The shipped-service witness satisfies the live-path gate. It is scoped to this head and the real workspace: the live core watcher was force-restarted from the worker's actual contaminated environment; the correction line and core sentinel were observed; a real owner input arrived with its bridge envelope, routed through the configured worker, produced a result, and the bridge delivered the reply. The measured receipt-to-delivery time was about 50 seconds, the 5m10s core-session-watcher absence and restoration were reported, and the root checkout was restored. This exercises the affected shipped component rather than a scratch harness. The production watcher blob is also unchanged from 09df259f through this head (854983700b4718188a69a10a1ba1c62a935ecfab).
CI, CLA, git diff --check, syntax checks, and review-checks.sh are green. One repository-rule blocker remains:
Blocker — the newly added case (f) comment still exceeds the two-line cap. AGENTS.md:45 requires code comments to be at most two lines. watch-tasks-stream-unready-holder-restamp.test.sh:133-136 is a four-line comment. Please retain the non-obvious empty-ID/custom-inbox constraint but condense it to at most two lines. The automated prose-cap reports green because it scans Python comments, not this shell block.
Non-blocking body accuracy: the witness timeline shows the PR watcher stopped at 06:32:43 and standbys appeared at 06:33:30.975, so “covered the inbox throughout” overstates roughly 48 seconds with no named watcher. That does not invalidate the successful round trip—the reply was delivered before teardown and queued tasks were later processed—but the body should describe the durable/no-loss gap rather than continuous watcher coverage.
Also non-blocking for this run: checking a PR out over the live checkout is the evidence method CONTRIBUTING.md warns weakens commit provenance. Future public witnesses should use the documented detached-worktree flow and include the contemporaneous rev-parse, resolved workspace, a sanitized task envelope, and the delivery receipt; private event and worker identifiers are unnecessary.
Verdict: not merge-ready solely because the added four-line comment violates the repository's explicit comment contract; I found no remaining runtime blocker. If the next push only condenses that test comment and leaves the production watcher blob unchanged, the live restart does not need to be repeated; I will re-check the new head and CI. john-the-dev now has a current-head approval; the requested CODEOWNER review is still outstanding, and this review remains blocking until the comment is fixed.
The prose-cap scanner only covers Python, so this .sh comment (4 lines) slipped through CI green while still violating AGENTS.md's comment rule. Kewei caught it by eye. Condensed to 2 lines, same constraint kept (empty-id-alone for a custom inbox + separate workspace). src/watch-tasks-stream.sh is untouched (blob 8549837, unchanged since 09df259) -- test comment + PR body only, no source change, so the shipped-service live witness need not be repeated. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@cla-assistant check |
|
Verified at |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at exact head fc1856dc7f94cbe205a532ed1ff1b914d06036b8 (my ab95b0f01 approval went stale on the push).
Delta ab95b0f01...fc1856dc7: 1 commit, 1 file. tests/watch-tasks-stream-unready-holder-restamp.test.sh changes +2/−4: the case (f) comment is condensed from four lines to two and keeps the empty-id / custom-inbox constraint. That resolves keweichen's remaining comment-cap blocker. bash -n passes on the new file.
The production watcher is unchanged: src/watch-tasks-stream.sh is blob 854983700b47… at both heads, which is the blob the shipped-service witness ran. So the live round trip still holds, and per keweichen's note it doesn't need to be repeated.
CI at the time of review: 20 pass, 5 pending (python standalone 1/3/5, shell serial tail), 1 skipping (Windows). This approval does not certify the pending lanes. Merge still needs them green, plus keweichen's re-check and the CODEOWNER review.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
APPROVE at fc1856dc7f94cbe205a532ed1ff1b914d06036b8. This supersedes my CHANGES_REQUESTED at ab95b0f01; its sole remaining blocker is fixed.
The exact ab95b0f01..fc1856dc7 delta is one commit in one test file, +2/−4 comment lines. Case (f)'s constraint is now exactly two comment lines at tests/watch-tasks-stream-unready-holder-restamp.test.sh:133-134, satisfying AGENTS.md. There is no executable change, and src/watch-tasks-stream.sh remains blob 854983700b4718188a69a10a1ba1c62a935ecfab, identical to the shipped-service witness head. The live restart therefore remains applicable and does not need repeating.
Exact-head evidence:
- Foreign worker environment 5/5, config hot reload 3/3, fallback receipt scope 6/6, and inbox/workspace contract 4/4 all pass.
- Every changed unready-holder case (e)/(f) assertion passes. This host still reports the same unrelated process-table failures in pre-existing (a)/(b)/(c) that I disclosed on the prior head; both CI shell lanes are green.
git diff --check,bash -n, and cumulativereview-checks.shpass. Added-line scans are clean for review/incident provenance and hardcoded host paths.- The PR body now accurately names the approximately 48-second no-watcher interval rather than claiming continuous coverage.
- Current CI and CLA are fully green: 27 pass, 1 intentional Windows skip.
Worst-case disruption remains the original silent core-dispatch failure if a worker's routing environment leaks into a core watcher start. The full routing-env reset, realpath-scoped core-inbox decision, worker-inbox negative control, custom-inbox recovery coverage, and shipped end-to-end witness mitigate that path. This comment-only delta introduces no new runtime risk.
Verdict: ready to merge from my review standpoint. john-the-dev also has a current-head approval; the requested qingyun-wu CODEOWNER approval remains a separate merge gate.
|
@qingyun-wu reminder at Chi's request: CODEOWNERS ( |
…ically (#4996) * fix(test): make the naive-writer mutation control collide deterministically The control in tests/agy-start-cli.test.py ran 8 naive writers on one shared `.tmp` name and asserted that at least one raised. With a one-field payload the write window is microseconds, so a fast runner serialises the eight calls, nobody collides, and the control fails although the production atomic seed() is correct (2/20 locally under CPU load; twice in CI on #4976). Hold every writer's staged file behind a threading.Barrier(8) between its write and its os.replace: the first rename wins, the other seven target a name that no longer exists, so the control now asserts exactly 7 FileNotFoundError on every run instead of hoping for a race. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(test): force the same interleaving on the real seed writer and the naive control The previous commit put the barrier only inside the synthetic naive writer, so the production-path concurrency test still ran seed() under free executor scheduling: a regression from unique mkstemp staging back to one fixed `.tmp` could serialise and pass it (4/20 on review) while the control passed regardless. Both tests now share one harness that patches the module's os.replace with a wrapper holding every writer at an 8-party barrier before it renames, so all writers stage before any rename on every run. The two tests differ only in the writer: seed() must raise nothing and leave intact JSON with no stray staging file; the naive fixed-name writer must fail 7 of 8 with FileNotFoundError. With seed() mutated to a fixed staging name the production test fails 20/20 under load and the control still passes; with the real seed() both pass 20/20. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Reviewed by Rui's Stand and kewei-red (both REQUEST_CHANGES on the same blocker): this PR's unconditional `unset SUTANDO_INSTANCE_ID` made the agy watcher's sentinel the bare canonical name on EVERY inbox, not just the core's own <ws>/tasks. On agy's default tasks-agy inbox (runs alongside the Claude/Codex core, task-notifier.sh:12-16) that collided with a live core watcher's own sentinel and then deleted it on exit -- exactly the regression `test_spawned_watcher_gets_a_distinct_sentinel_not_the_canonical_one` existed to pin. Reproduced before this fix: core watcher pid=90838 sentinel=90838 agy-shaped watcher pid=91722 sentinel=91734 <- overwrote the core's sentinel after agy watcher exits: sentinel=No such file or directory <- removed it Fix: a shared helper, inbox_is_canonical_core_tasks_dir (src/tasks-dir- resolve.sh), mirrors the equality check watch-tasks-stream.sh already used inline (now routed through the same helper). task-notifier.sh clears its instance id only when its own TASKS_DIR passes that check; otherwise it keeps "agy-task-notifier" as before, so a non-canonical inbox's sentinel can never collide with the canonical one. One helper, both callers, so the two can't drift apart again (the drift that caused this bug and #4976 before it). Re-reproduced after the fix: the agy watcher stamps its own distinct sentinel immediately, and the core's canonical sentinel is byte- and mtime-identical before, during, and after the agy watcher's full lifecycle (restored test_tasks_agy_shaped_inbox_never_touches_the_canonical_sentinel). The canonical-inbox clearing behavior (foreign id cleared, existing fixtures) is unchanged -- covered by the existing suite, which was already written against a canonically-named fixture dir. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#4303)" (#5166) * Revert "feat(agy-core): Slice 2 — task-file-injection notifier for agy (Antigravity CLI) (#4303)" This reverts commit 4a72ddf. #4303 was green on its own head but was not re-run against #4976 and #4945, which had landed on main; together they turned four notifier and tmux tests red on main. The forward fix (#5161) is still in review, so main is restored first; agy Slice 2 re-lands with that fix once it is approved. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(agy): retire the task notifier's leftover watcher session on rerun The reverted launcher no longer knows about the "<session>-watcher" tmux session #4303 started, and it returns early once the core session exists, so an update left the old notifier running. Every run of start-cli.sh now kills an exact-name leftover watcher on its own socket before that guard. Regressions run a real tmux server on an isolated socket. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
… tmux targets #4303 merged after #4976 and #4945 without being re-run against them: - #4976 makes watch-tasks-stream.sh clear SUTANDO_INSTANCE_ID on the core's canonical inbox, so the agy notifier's watcher stamps watch-tasks-stream.pid while the notifier polled the per-instance name it forced. The launch receipt never appeared ("did not report ready within 12s"). The notifier and start-cli.sh now run the watcher with no instance id, as the Codex core does. - #4945's scan forbids bare `-t "$SESSION"` targets; the notifier's six capture-pane/send-keys calls now use the exact "=$SESSION:" form. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewed by Rui's Stand and kewei-red (both REQUEST_CHANGES on the same blocker): this PR's unconditional `unset SUTANDO_INSTANCE_ID` made the agy watcher's sentinel the bare canonical name on EVERY inbox, not just the core's own <ws>/tasks. On agy's default tasks-agy inbox (runs alongside the Claude/Codex core, task-notifier.sh:12-16) that collided with a live core watcher's own sentinel and then deleted it on exit -- exactly the regression `test_spawned_watcher_gets_a_distinct_sentinel_not_the_canonical_one` existed to pin. Reproduced before this fix: core watcher pid=90838 sentinel=90838 agy-shaped watcher pid=91722 sentinel=91734 <- overwrote the core's sentinel after agy watcher exits: sentinel=No such file or directory <- removed it Fix: a shared helper, inbox_is_canonical_core_tasks_dir (src/tasks-dir- resolve.sh), mirrors the equality check watch-tasks-stream.sh already used inline (now routed through the same helper). task-notifier.sh clears its instance id only when its own TASKS_DIR passes that check; otherwise it keeps "agy-task-notifier" as before, so a non-canonical inbox's sentinel can never collide with the canonical one. One helper, both callers, so the two can't drift apart again (the drift that caused this bug and #4976 before it). Re-reproduced after the fix: the agy watcher stamps its own distinct sentinel immediately, and the core's canonical sentinel is byte- and mtime-identical before, during, and after the agy watcher's full lifecycle (restored test_tasks_agy_shaped_inbox_never_touches_the_canonical_sentinel). The canonical-inbox clearing behavior (foreign id cleared, existing fixtures) is unchanged -- covered by the existing suite, which was already written against a canonically-named fixture dir. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… tmux targets #4303 merged after #4976 and #4945 without being re-run against them: - #4976 makes watch-tasks-stream.sh clear SUTANDO_INSTANCE_ID on the core's canonical inbox, so the agy notifier's watcher stamps watch-tasks-stream.pid while the notifier polled the per-instance name it forced. The launch receipt never appeared ("did not report ready within 12s"). The notifier and start-cli.sh now run the watcher with no instance id, as the Codex core does. - #4945's scan forbids bare `-t "$SESSION"` targets; the notifier's six capture-pane/send-keys calls now use the exact "=$SESSION:" form. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewed by Rui's Stand and kewei-red (both REQUEST_CHANGES on the same blocker): this PR's unconditional `unset SUTANDO_INSTANCE_ID` made the agy watcher's sentinel the bare canonical name on EVERY inbox, not just the core's own <ws>/tasks. On agy's default tasks-agy inbox (runs alongside the Claude/Codex core, task-notifier.sh:12-16) that collided with a live core watcher's own sentinel and then deleted it on exit -- exactly the regression `test_spawned_watcher_gets_a_distinct_sentinel_not_the_canonical_one` existed to pin. Reproduced before this fix: core watcher pid=90838 sentinel=90838 agy-shaped watcher pid=91722 sentinel=91734 <- overwrote the core's sentinel after agy watcher exits: sentinel=No such file or directory <- removed it Fix: a shared helper, inbox_is_canonical_core_tasks_dir (src/tasks-dir- resolve.sh), mirrors the equality check watch-tasks-stream.sh already used inline (now routed through the same helper). task-notifier.sh clears its instance id only when its own TASKS_DIR passes that check; otherwise it keeps "agy-task-notifier" as before, so a non-canonical inbox's sentinel can never collide with the canonical one. One helper, both callers, so the two can't drift apart again (the drift that caused this bug and #4976 before it). Re-reproduced after the fix: the agy watcher stamps its own distinct sentinel immediately, and the core's canonical sentinel is byte- and mtime-identical before, during, and after the agy watcher's full lifecycle (restored test_tasks_agy_shaped_inbox_never_touches_the_canonical_sentinel). The canonical-inbox clearing behavior (foreign id cleared, existing fixtures) is unchanged -- covered by the existing suite, which was already written against a canonically-named fixture dir. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
A
--force-restart(or any start) of the task watcher issued from inside another instance's own session inherits that instance's worker routing env —SUTANDO_INSTANCE_IDand the vars a worker shell exports alongside it (SUTANDO_INBOX_KIND,SUTANDO_INBOX_RESOLVER,SUTANDO_INBOX_RESOLVER_TIMEOUT,SUTANDO_POOL_DELIVERY_SCRIPT) — even when--inboxexplicitly names the core's own canonical<ws>/tasks. The watcher then watches the right directory while believing itself to be that other worker's own inbox-watcher (#4502's rule: a worker's own inbox IS the routing decision): it never loads the core'stask-event-handlerconfig, and a leftoverSUTANDO_INBOX_RESOLVERindependently rejects every plain task body it sees (it expects a worker sentinel, not a task), so dispatch is silently dead with no error beyond the resolver's own stderr.Observed live in production, twice, during this investigation:
1184, started 18:46 — argv correctly--role session --inbox <core's workspace>/tasks, envSUTANDO_INSTANCE_ID=<a worker's id>.66624, started 19:16 (its replacement, after the first was independently restarted) — same pattern, same foreign env, confirming this is a recurring defect (something keeps re-issuing this restart), not a one-off.Root cause
src/watch-tasks-stream.shgates loading the core's routing config purely on[ -z "$SUTANDO_INSTANCE_ID" ]— it never checks that identity against the inbox actually being served. A restart command run from a worker's own shell inherits that shell's full exported worker-routing env, and nothing resets any of it even though--inboxis explicit and authoritative.Fix
Right after the inbox/workspace are resolved: when an explicitly-set
SUTANDO_INSTANCE_IDis found while serving the core's own canonical inbox, the whole inherited worker-routing block is cleared together —SUTANDO_INSTANCE_ID,SUTANDO_INBOX_KIND,SUTANDO_INBOX_RESOLVER,SUTANDO_INBOX_RESOLVER_TIMEOUT,SUTANDO_POOL_DELIVERY_SCRIPT— not just the instance id (review feedback from kewei: clearing the id alone leavesSUTANDO_INBOX_RESOLVERlive, which fails every entry on its own)."The core's own canonical inbox" is decided by realpath, not basename: the inbox's resolved path is compared against
<resolved workspace>/tasks's resolved path (canonical_tasks_dir(), the same primitivetasks-dir-resolve.shalready uses elsewhere), notbasename(inbox) == "tasks"(also kewei: a worker's own inbox can itself end in.../tasksand would have false-matched a basename-only check).What it deliberately does not do:
<ws>/deliveries/<id>inbox. The pre-existing identity-match check there already decides whether a starter whoseSUTANDO_INSTANCE_IDdisagrees with the inbox must yield. Correcting the env to match the inbox in that branch would make a wrong starter's identity agree with the inbox by fiat, defeating that very check.SUTANDO_INSTANCE_IDwas unset. Several tests legitimately run a core against adeliveries/-shaped inbox with no ambient identity at all; this only corrects a value that was actually asserted and is actually wrong for the inbox being served.Known, non-blocking residual (flagged by johnm-desktop.agent in review discussion, not actioned here):
workspace_dir_for_inbox()resolves<resolved workspace>as${SUTANDO_WORKSPACE_DIR:-dirname(inbox)}. WithSUTANDO_WORKSPACE_DIRset — which every real worker shell always does (spawn_worker.py's env export, and exactly the incident shape this PR targets) — the comparison is a genuine cross-path realpath check. With it unset, the comparison mathematically reduces to the old basename-equivalent set, sincedirname(inbox)/tasks == inboxiffbasename(inbox) == "tasks". Correct either way, just not doing extra work over the old check in the unset case; not fixing speculatively without a concrete production path that reaches it unset.Test changes
tests/watch-tasks-stream-config-hot-reload.test.py's worker case (property 3) previously setSUTANDO_INSTANCE_ID="worker-1"while watching the bare<ws>/tasksdir — a combination that cannot occur in production (a real worker's inbox is always<ws>/deliveries/<id>, confirmed againstskills/worker-pool/scripts/launch-worker-session.sh). Updated it to a realistic<ws>/deliveries/worker-1inbox.tests/watch-tasks-stream-foreign-instance-id-corrected.test.pynow pins 5 properties against the real incident shape and both review blockers:SUTANDO_INSTANCE_IDon the core's own inbox is corrected: an already-present handler still runs.SUTANDO_INBOX_RESOLVER/SUTANDO_INBOX_KIND/SUTANDO_POOL_DELIVERY_SCRIPT— on the core's own inbox is cleared, not just the instance id: the handler still runs.tasks(not the<ws>/tasksconvention, but a path that would false-match a basename-only check) keeps its identity — the handler config is correctly NOT read.4b. No foreign-identity correction is logged for that real worker inbox.
Before/after evidence
The new test's 5 properties, at this head:
Same test, against unmodified
main(confirms both review blockers reproduce without this fix — property 3 is the review's Blocker 1, property 4/4b exercise Blocker 2's basename trap):Full
watch-tasks-stream*/watcher-*suite (29 files), run before and after: all green both before and after except one pre-existing flake,watch-tasks-stream-fallback-receipt-scope, confirmed failing identically on unmodifiedmain(not caused by this change) and already tracked at #4645.(Separately, while verifying locally:
tests/watch-tasks-stream-config-hot-reload.test.pyand a couple of siblings fail when run from inside a live worker's own shell — unrelated ambientSUTANDO_WORKSPACE_DIR/SUTANDO_INBOX_RESOLVERenv leakage into their subprocess, confirmed reproducing identically onmaintoo. Pre-existing, out of scope here; filed as #4979.)Live path
This is exactly the live/delivery-loop path CONTRIBUTING.md asks for a real post-restart round trip on, not just unit tests. Two live runs, both against a real watcher process and real fswatch — never the host's live
<ws>/tasksitself, always a scratch workspace:1. A fully contaminated worker-shaped env built with
env -i(rules out any ambient leakage entirely — isolates exactly what the fix claims to clear):SUTANDO_INSTANCE_ID+ a realSUTANDO_INBOX_RESOLVER/SUTANDO_INBOX_KIND/SUTANDO_POOL_DELIVERY_SCRIPT,SUTANDO_WORKSPACE_DIRpointed at a fresh temp workspace with its owntask-event-handler.json.Before (built from
main):At this head:
2. This worker's own real, unmodified production shell env (per John's review request — not a hand-built set): every
SUTANDO_*this worker actually runs with (SUTANDO_INSTANCE_ID,SUTANDO_INBOX_KIND=deliveries, the realSUTANDO_INBOX_RESOLVER/SUTANDO_POOL_DELIVERY_SCRIPTpaths), with onlySUTANDO_WORKSPACE_DIR/SUTANDO_RESULTS_DIRredirected to a scratch workspace so the demo never touches this host's liveworkspace/tasks:Result file written by the handler on the actual task → result round trip:
3. The shipped service itself, on this host's real workspace, Chi-authorised (run by Pro-dev from its own contaminated worker shell —
SUTANDO_INSTANCE_ID,SUTANDO_INBOX_KIND=deliveries, the realSUTANDO_INBOX_RESOLVER/SUTANDO_POOL_DELIVERY_SCRIPT, nothing unset — against the root checkout moved to this PR's exact head for the window, restored tomainafterwards):Receipt → result → delivered reply: 50s end to end. Downtime: the core's own session-role watcher was absent 5m10s (06:31:06-06:36:16). The PR-head watcher held the inbox until 06:32:43; standby watchers took over at 06:33:30.975 — a ~48s span with no watcher named as holding it. Durable, not lossy: two other task files present at open were announced and answered correctly once a watcher picked them up, and nothing was lost across the gap. Full transcript: PR comment.
Test plan
tests/watch-tasks-stream-foreign-instance-id-corrected.test.py(5 properties) — 5/5, control reproduces both review blockers on unmodifiedmaintests/watch-tasks-stream-config-hot-reload.test.py— 3/3 (outside a live-worker-shell environment; see tests: several watch-tasks-stream-*.test.py don't use the existing clean_watcher_env.py fixture, leaking the live shell's SUTANDO_* env #4979 for that fragility)tests/watch-tasks-stream-inbox-and-workspace-env.test.pywatch-tasks-stream*/watcher-*suite (29 files) — green except pre-existing Linux-only flake: watch-tasks-stream-fallback-receipt-scope (and handler-terminal-rc) fail on PRs that do not touch the watcher, pass on rerun #4645 flakebash -n src/watch-tasks-stream.sh,review-checks.sh(hardcoded-paths + prose-cap) — cleanenv -ifully contaminated worker env against a scratch workspace — fails onmain, dispatches at head🤖 Generated with Claude Code