[qingyun-wu]

What and why

Owner directive, 2026-09-11: a restart must touch only the watcher it can prove is its own. PR #4168 shipped the readers — restart.sh confirms ownership from an identity record on the sentinel plus an incarnation marker file, and refuses (rc 3) when anything is unconfirmable. Nothing wrote either file, so the confirmation always failed. Correct, and permanently inert.

This PR is the writer, plus the one other reader that had to learn the record: startup.sh's reaper.

• At start, src/watch-tasks-stream.sh publishes the record (pid on line 1, then instance, incarnation, code_path, version, started_at, workspace) and the marker at sentinel_incarnation_path, both by temp+rename in the state dir.
• Start and cleanup serialise on a mkdir lock at <sentinel>.lock — the repo's portable test-and-set (acquire_dispatch_lock, sync-memory.sh); flock(1) is Linux-only.
• Every start appends one line to <workspace>/logs/watcher-starts.log, so "which code is this pid running" is a read rather than a hunt.
• reap_stale_task_watcher (src/startup-runtime.sh) reads the record too: it reaps only its own instance running its own code_path, and reports-and-reaps-nothing otherwise. Nothing else in startup.sh changed.

Rebase round 2 — b11066527 → 6936a7e03, onto #4168's new head 72e63cf27

git rebase --onto origin/fix/restart-stops-only-own-watcher a6ca253c0, so only this PR's own two commits replay. #4168 has since taken the reader + ownership-policy half that this branch had carried, so those files are no longer this PR's to add. What moved DOWN to #4168:

moved to #4168	this PR now
read_sentinel_record / read_sentinel_pid in src/util_paths.py	keeps only a sentinel-field CLI branch on the same reader (+8/−1), for the shell bridge below
the services_status.py + health-check.py migration onto them	nothing — git diff against the base shows zero lines in either file
tests/watcher-sentinel-record-readers.test.py	keeps a +16/−22 delta: write_record is re-pointed at the production sentinel_write_record (#4168 composed the record inline because the writer was not in that PR)
src/watcher_identity.py — the one ownership policy	consumed, not re-implemented
the startup reaper's line-1 read, via watcher_sentinel.sh:sentinel_pid_in	consumed
the shell record parser sentinel_record_field / sentinel_has_record (deleted there)	not re-introduced — see below

What this PR still owns: sentinel_write_record, the incarnation marker, the mkdir lock with the atomic steal under a second mkdir lock, watcher-starts.log with the start-log-before-publish ordering, the lock test, sentinel_instance_from_path, sentinel_release_incarnation, and the reaper's record gate.

One grammar, kept. The two follow-ups #4168 named are done here:

1. tests/watcher-sentinel-record-readers.test.py::write_record now drives the production shell writer, so the writer/reader contract is measured end to end rather than against a python re-spelling. The check COUNT is deliberately unchanged — 78 at the base and 78 here — so what moved is the source of the bytes, not the size of the suite.
2. The reaper's record reads used the now-deleted sentinel_record_field/sentinel_has_record. They are routed through src/watcher_identity.py owner-pid via the resolved-python bridge — the same policy restart.sh:_confirm_watcher_owner and health-check.py ask — which answers instance, workspace, incarnation-marker and code_path in one call. sentinel_release_incarnation reads its one field through sentinel_field_in, a bridge to util_paths.read_sentinel_record, in the same shape as sentinel_pid_in. No shell record parser exists at this head:

$ git grep -n "sentinel_record_field\|sentinel_has_record" -- src tests scripts skills
$ echo $?
1

# CONTROL — the same grep at the PREVIOUS head, where the parser still existed:
$ git grep -c "sentinel_record_field\|sentinel_has_record" b11066527 -- src tests
b11066527:src/restart.sh:5
b11066527:src/startup-runtime.sh:3
b11066527:src/watcher_sentinel.sh:3
b11066527:tests/watcher-sentinel-ownership.test.sh:1

$ git grep -n "sentinel_field_in\|sentinel_pid_in" -- src tests
src/startup-runtime.sh:346:  stale_pid="$(sentinel_pid_in "$pid_file" 2>/dev/null || true)"
src/watcher_sentinel.sh:37:sentinel_pid_in() {
src/watcher_sentinel.sh:48:sentinel_field_in() {
src/watcher_sentinel.sh:149:  recorded="$(sentinel_field_in "$pid_file" incarnation 2>/dev/null || true)"
tests/watcher-sentinel-ownership.test.sh:198:check "$([ "$(sentinel_field_in "$PF6" code_path)" = ...

The five restart.sh lines in that control are #4168's — it replaced them with watcher_identity on its own head, which is what made this rebase's re-routing necessary rather than optional.

Two test fixtures moved with the stricter gate, both named rather than quietly adjusted: startup-watcher-reaper-ownership case 2 now records workspace=$TMP/case2 (the reaper derives the workspace from the sentinel's own directory, the derivation restart.sh makes), and watcher-sentinel-ownership f4 now writes a COMPLETE record plus its marker — an incomplete one is refused before the tri-state helper, and f4 would have passed without ever reaching the branch it is named for (REVIEW.md 18). g-foreign's expected refusal string is now the shared policy's own wording (instance: … this scope resolves "") instead of the reaper's old local sentence.

Review round 1 — dfc29ce2f → this head

Rebased onto the parent's new head (a6ca253c0). The rebase replays the child commit only; the parent's own commits are its, and its new ones touch tests alone, so the child layer is unchanged by it.

[1] The lock was not mutually exclusive on its steal path — fixed, and the fix goes one step further than the suggestion

@john-the-dev measured 2/60 double-wins at two acquirers. Reproduced with four, using a harness that spawns real processes (not ( … ) & subshells — a subshell inherits the parent's $$, which no two real acquirers ever share):

80 trials x 4 acquirers, one ABANDONED lock
  rm + mkdir (the shipped steal)      trials with >1 winner: 14      <- the defect

The suggested patch — mv "$lock" "$lock.stale.$$" && rm -rf then continue — makes stealer vs stealer exclusive, and that was the measured race. But it leaves an adjacent one, and I measured it before adopting it: a stealer whose staleness probe passed before the winner recreated the lock renames the winner's fresh lock away and deletes it. Both then hold.

200 trials, REAL processes, one ABANDONED lock
  mv-then-rm (the suggested patch)    4 acquirers: 14 double-won    6 acquirers: 58 double-won

So the steal is done under a second lock instead, and the staleness is re-probed there. $lock is never renamed away, so a winner that took it since the outer probe is still present to be seen — and the lock path is never momentarily free for a third acquirer to mkdir into:

while ! mkdir "$lock" 2>/dev/null; do
  if sentinel_lock_abandoned "$lock"; then
    if mkdir "$steal" 2>/dev/null; then
      if sentinel_lock_abandoned "$lock"; then rm -rf "$lock"; fi   # re-probe
      rmdir "$steal" 2>/dev/null || true
      continue
    fi
    if sentinel_lock_abandoned "$steal"; then rm -rf "$steal"; fi
  fi
  [ "$(date +%s)" -lt "$deadline" ] || return 1
  sleep 0.05
done

200 trials per cell, REAL processes, one ABANDONED lock — trials with >1 winner
 acquirers | this head (steal lock) | rm+mkdir (pre-fix)
         2 |                      0 |                  1
         4 |                      0 |                 27
         6 |                      0 |                121
         8 |                      0 |                159

The two numbers are reconciled, not papered over. They stay two numbers because they answer different questions, and the comment now says so in two lines — the acquire timeout is ${2:-10} seconds, the abandonment threshold is 60s (find -mmin, whose unit is minutes):

# Abandoned = untouched for a full minute (`find -mmin`, whose unit is minutes)
# — a DIFFERENT number from the ${2:-10}s a caller waits to acquire, below.
sentinel_lock_abandoned() { find "$1" -maxdepth 0 -mmin +1 2>/dev/null | grep -q .; }

[4→1] The lock now has coverage, with a control that is shown able to fire

tests/watch-tasks-stream-sentinel-record.test.py::test_lock_is_mutually_exclusive. 50 trials × 4 acquirers is the trial count, chosen because CI caps a Python suite at 120s and runs suites in parallel; the control double-wins about 1 trial in 6, so a false zero is ~1e-4.

  ok   an ABANDONED lock has exactly ONE stealer (50 trials x 4 acquirers)
  ok     ...and no steal lock is left behind
  ok   CONTROL: the rm+mkdir steal lets MORE THAN ONE stealer win
  ok     ...and it is the STEAL that differs, not the harness
  ok   CONTROL: a FRESH held lock is never stolen — mkdir excludes all 25

Discrimination — the same test against the pre-fix sentinel_lock_acquire, everything else identical:

  FAIL an ABANDONED lock has exactly ONE stealer (80 trials x 4 acquirers)
       — 14 trials with >1 winner, 66 with 1, 0 with 0

The two CONTROL arms are what stop the assertion being vacuous in either direction: the first proves the harness can observe two winners (it does so against the old shape), the second proves the acquirers genuinely contend (mkdir excludes all 25 against a fresh held lock, the same control @john-the-dev ran).

[2] The start-log flake — fixed at the source end

The readiness gate waits on the sentinel, and the sentinel was published before the line the test then asserts. The append now happens before the lock is taken, so the record's appearance implies the log line already exists — and a start that died during publish leaves the line, which is the case the log is read for.

src/watch-tasks-stream.sh, this head — by symbol, top to bottom:
  printf ... >> "$WORKSPACE_DIR/logs/watcher-starts.log"   <- appended FIRST
  sentinel_lock_acquire "$PID_FILE"
  sentinel_write_record "$PID_FILE" ...                    <- the record is published here
  sentinel_lock_release "$PID_FILE"

20 consecutive runs at this head, unmodified checkout:

run  1 PASS
run  2 PASS
run  3 PASS
run  4 PASS
run  5 PASS
run  6 PASS
run  7 PASS
run  8 PASS
run  9 PASS
run 10 PASS
run 11 PASS
run 12 PASS
run 13 PASS
run 14 PASS
run 15 PASS
run 16 PASS
run 17 PASS
run 18 PASS
run 19 PASS
run 20 PASS
--------------
20 PASS / 0 FAIL

[3, P1] The record broke both PID readers — one shared reader now, both consumers migrated

Moved to #4168 by the round-2 rebase. read_sentinel_record / read_sentinel_pid and the
services_status.py + health-check.py migration now land in the parent; this PR's diff touches
neither consumer. The evidence below is kept because it is what established the contract those
readers implement, and because this PR is the writer that makes them load-bearing. What remains
here is the sentinel-field CLI branch on that same reader and the writer/reader contract test.

The blocker as filed, reproduced with the production shell writer publishing into a scratch state dir and the production python consumers reading it back, identical injected live-process evidence on both sides.

BEFORE (at dfc29ce2f):

--- the sentinel the production writer publishes ---
4242
instance=
incarnation=inc-A
code_path=/x/src/watch-tasks-stream.sh
version=abc1234
started_at=1789123585
workspace=/.../state

pid-only sentinel : services_status    = ('running', 'pid 4242')
production record : services_status    = ('unknown', "unreadable pidfile: invalid literal for int() with base 10: '4242\ninstance=\nincarnation=inc-A\ncode_path=...'")
pid-only sentinel : check_task_watcher = ('ok',   'streaming watcher alive (pid 4242)')
production record : check_task_watcher = ('warn', 'unreadable PID sentinel (invalid literal for int() with base 10: ) — restart the watcher')

AFTER (this head), same probe, same fixture:

pid-only sentinel : services_status    = ('running', 'pid 4242')
production record : services_status    = ('running', 'pid 4242')
pid-only sentinel : check_task_watcher = ('ok', 'streaming watcher alive (pid 4242)')
production record : check_task_watcher = ('ok', 'streaming watcher alive (pid 4242)')

Where the reader lives, and why there. src/util_paths.py, not a new module: it already owns the sentinel's python-side contract (WATCHER_SENTINEL_STEM, watcher_sentinel_path, watcher_sentinel_paths, and the watcher-sentinel CLI the shell namer calls). Both consumers already import it, so no new dependency edge is created, and a new src/watcher_sentinel.py would have split one contract across two files. It is stdlib-only and never raises.

read_sentinel_record(path) -> dict   # {} absent/unreadable; `pid` only when line 1 is
                                     # a usable pid; `pid_line` verbatim so a consumer
                                     # can say WHY without re-deriving line 1;
                                     # key=value claims from line 2 on
read_sentinel_pid(path) -> int|None

Both shapes are accepted, because both are live for the whole rolling-upgrade window: the legacy bare pid, and the record. 0, negatives and non-ASCII digits yield no pid key rather than a number — they name a process GROUP to os.kill/ps, which succeeds and would read falsely "running" (and int("²") raises where "²".isdigit() is True).

Consumers migrated: services_status.probe_pidfile and health-check.check_task_watcher. The existing services-status.test.py pin on the "non-positive pid … in pidfile" wording is unchanged and still passes — that discrimination moved into the shared record's pid_line rather than being dropped.

Writer→consumer contract tests — tests/watcher-sentinel-record-readers.test.py. Since the rebase the FILE is #4168's (78 checks, including its watcher_identity section G); this PR re-points its write_record helper at the production shell writer. Abridged below — the five malformed-input lines are folded into one:

  ok   the production shell writer published a record
  ok     ...and it really is multi-line (else every reader check is vacuous)
  ok   read_sentinel_pid returns the pid of a RECORD
  ok   read_sentinel_pid accepts the LEGACY pid-only sentinel
  ok   a {empty,whitespace,junk,negative,zero} sentinel reads None, never a raised ValueError
  ok   services_status.probe_pidfile: a RECORD sentinel is running
  ok   CONTROL: the same record with a DEAD pid still reads offline
  ok   CONTROL: an unparseable sentinel still reads unknown
  ok   CONTROL: a sentinel that cannot be read AT ALL is unknown, not offline
  ok   services_status.probe_watcher_sentinels: the record host is running
  ok   health-check.check_task_watcher: a RECORD sentinel with a live watcher is ok
  ok     ...and never advises a restart over an unreadable sentinel
  ok   CONTROL: the LEGACY pid-only sentinel is unchanged — still ok
  ok   CONTROL: a record whose pid is dead still warns
  ok   CONTROL: a genuinely unreadable sentinel still warns as unreadable
  ok   src/health-check.py keeps no private sentinel parser
  ok   src/services_status.py keeps no private sentinel parser
  ok   CONTROL: the scanner CAN fire (it matches the shape it forbids)

The health-check argv is fabricated through the module's own _proc_argv/_watcher_trees/_ps_snapshot hooks, the pattern tests/health-check-task-watcher.test.py already uses — no real processes, no host process table.

The last three are structural, per REVIEW.md 14: two copies that currently agree pass every behavioural test, so the duplication itself has to be pinned — with a positive control, because a scanner that matches nothing returns the same zero as a clean tree.

One reader deliberately NOT migrated. fix_task_watcher_sentinel in health-check.py, the --fix repair: it compares pid_file.read_text().strip() == pid to ask "is this still exactly the pid-only stamp I just wrote with open(…, "x")". Against a record that comparison fails and the code leaves the file alone — which is the correct answer, because a record at that path means a real watcher claimed it mid-write. Routing it through the shared reader would make it compare pids and unlink a live watcher's record. Flagging rather than silently leaving it.

[3-nit] code_path is now resolved on both sides

Path(f.get("code_path", "")).resolve() == (REPO / "src" / "watch-tasks-stream.sh").resolve()

Before / after

One watcher started in a private sandbox workspace (temp dir, stub fswatch, resolution asserted before spawning — never the operator's workspace).

BEFORE — at the base, 80cd5ca:

### BEFORE — watcher alive after 5s: True
### BEFORE — state/ listing
    watch-tasks-stream.pid
### BEFORE — the sentinel (state/watch-tasks-stream.pid)
    28553
### BEFORE — the incarnation marker
    <absent>
### BEFORE — logs/watcher-starts.log
    <absent>

A bare pid. _confirm_watcher_owner refuses it — "records a pid only" — so a restart can never stop this watcher.

AFTER — at this head:

### AFTER — watcher alive after 5s: True
### AFTER — state/ listing
    watch-tasks-stream.incarnation
    watch-tasks-stream.pid
### AFTER — the sentinel (state/watch-tasks-stream.pid)
    40759
    instance=
    incarnation=1789118507-40759-2582017289
    code_path=/.../scratchpad/prB/src/watch-tasks-stream.sh
    version=dfc29ce2f
    started_at=1789118507
    workspace=/.../wsent-AFTER-d3o_2to5/workspace
### AFTER — the incarnation marker
    1789118507-40759-2582017289
### AFTER — logs/watcher-starts.log
    2026-09-11T09:21:47Z - 1789118507-40759-2582017289 40759 /.../src/watch-tasks-stream.sh dfc29ce2f /.../state/watch-tasks-stream.pid

instance= is empty because this is the default instance — that is the key sentinel_path_for encodes in watch-tasks-stream.pid, and it is exactly what restart.sh derives from the resolved path. An empty instance prints as - in the start log so the column count stays fixed.

The handover race (B2), and what the controls actually remove

tests/watch-tasks-stream-sentinel-record.test.py drives real watcher processes: A starts, B (same instance) starts and stamps over A's record, then A exits late through its EXIT trap.

  ok   B's start replaces the record AND the marker
  ok   A exits through its EXIT trap (so cleanup RUNS)
  ok   A's late cleanup leaves B's RECORD in place
  ok   A's late cleanup leaves B's MARKER in place
  ok     ...and line 1 still names B
  ok   B keeps working after A's cleanup — its fswatch still emits
  ok     ...and B is still running
  ok   the control fixture starts from B's live record and marker
  ok   CONTROL: a cleanup with no incarnation awareness takes B's MARKER
  ok     ...while the pid on line 1 still saves B's record
  ok   CONTROL: the unguarded cleanup loses B's record AND its marker

All sentinel-record checks passed.

Attribution, stated precisely rather than more strongly than the evidence supports. The pid on line 1 already protects the RECORD — that is #4168's guarantee, and watch-tasks-stream-sentinel-ownership.test.py pins it. A control that deleted only the incarnation comparison would therefore change nothing about the record and would be a control that removes no variable. What the incarnation protects is the marker, which carries no pid at all and so cannot otherwise distinguish a live successor's file from its own. C1 is accordingly the cleanup that a change shipping the marker without incarnation awareness would have written — release the record by pid exactly as before, rm the new marker outright — and it loses B's marker. C2 is the pre-#4168 baseline shape, an unguarded rm of both. The consequence of losing the marker is concrete: restart.sh then reports "the live process exposes no marker" and refuses to stop B forever.

"B survived" is asserted as B still working — a task dropped after A's cleanup is emitted by B's fswatch — not merely that B's pid still exists.

The reaper bug this uncovered

reap_stale_task_watcher read the pid with stale_pid="$(cat "$pid_file")". With a pid-only sentinel that was harmless; with #4168's record it feeds the whole multi-line record to ps -p, which answers ps: Invalid process id: ... for every recorded sentinel — a reaper that can never confirm anything. Measured while the g-series cases below were first run:

⚠ cannot determine whether pid 46464
instance=peer+w2
incarnation=inc-46464
code_path=.../g-foreign/watch-tasks-stream.sh
... is a watcher (ps: ps: Invalid process id: 46464); leaving the sentinel alone

Fixed — since the rebase, by delegating to watcher_sentinel.sh:sentinel_pid_in (line 1, through util_paths.read_sentinel_record), which #4168 now owns; an unreadable line 1 is an explicit refusal. The other reader change is still this PR's: restart.sh's inline instance-from-path derivation is extracted into sentinel_instance_from_path() in src/watcher_sentinel.sh, so the reaper reuses it rather than carrying a second copy of the same policy.

The record gate that follows it now reads:

_wpy="$(resolve_python "$_rp" 2>/dev/null || true)"
[ -n "$_wpy" ] || { echo "  ⚠ no runnable python3 — …; leaving both alone"; return 0; }
_owner="$("$_wpy" "$_rp/src/watcher_identity.py" owner-pid \
        --sentinel "$pid_file" --instance "$(sentinel_instance_from_path "$pid_file")" \
        --workspace "${_state_dir%/state}" \
        --incarnation-file "$(sentinel_incarnation_path "$pid_file")" 2>&1)" || {
  echo "  ⚠ $_owner; leaving both alone"; return 0; }

No python ⇒ the reaper refuses rather than reaps, matching sentinel_pid_in's existing fail-closed behaviour on the same host condition. The refusal text is now the shared policy's, so the reaper and restart.sh report the same sentence for the same cause.

One reader comment was also corrected: the sentinel_pid_wrote_file PRECONDITION said the sentinel is created "IN PLACE". The writer now renames, so the accurate invariant — and the one the function actually depends on — is that the published mtime is the stamp time. sentinel_write_record creates its temp moments before the rename, which holds; p1/p2 in watcher-sentinel-ownership.test.sh still pin the failure mode of a file staged long beforehand.

Test changes, case by case

B4 retires the legacy behaviour in which a live watcher on a pid-only sentinel was reaped. Four existing assertions pinned that. Rather than delete them, each fixture was upgraded to write the record that now authorises the reap, so the case still measures the property it is named for — and a new case pins the refusal, with the old case as its control.

Suite / case	Change	Why
watcher-sentinel-ownership a1–a3	$D1/watch.pid → watch-tasks-stream.pid + record	without it the run refuses at the record gate and never reaches the reissued-pid branch a3 asserts
same, b1–b2	same	this is the control for the new g-series: same fixture, record present, IS reaped
same, p1–p2	staged file now carries a record	keeps measuring the moved-in-mtime failure, not the record gate
same, p3	grep on ^echo "$$" > "$PID_FILE" → drive sentinel_write_record and assert sentinel_pid_wrote_file accepts it, no temp residue, marker matches, fields survive	the writer publishes by rename now; behaviour, not spelling
same, f2–f3	record added	otherwise the unmeasurable-ownership branch is never reached
same, f4	record added	otherwise the errexit case never reaches the tri-state helper
same, g-pidonly / g-foreign / g-code (new)	each removes exactly one field	live watcher not signalled, sentinel left in place, refusal reported
startup-watcher-reaper-ownership case 2	record added	same reason as b1/b2; it is the control for case 5
same, peer-survival	both sentinels get records	keeps "own stale watcher is still reaped" true
same, case 5 (new)	pid-only sentinel, live watcher	refuse and report
watch-tasks-stream-shutdown-emit	ordering anchor sentinel_release_if_owner → sentinel_release_incarnation	cleanup calls the new release; the ordering property is unchanged

Case 1 (dead pid, pid-only) is deliberately unchanged: nothing is killed there, and releasing a sentinel that names a dead process is not reaping a watcher. startup.sh hands the reaper only this instance's resolved path, so such a file cannot be a peer's.

Gate

Run alone, on the cumulative diff against the current base:

Re-run at 6936a7e03, against the new base:

$ git diff origin/fix/restart-stops-only-own-watcher > /tmp/rb4174.diff
$ bash scripts/review-checks.sh --diff /tmp/rb4174.diff
prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)
$ echo $?
0

A green scan is a zero, so here is the positive control — the same diff with one /Users/... line appended:

review-checks: FAIL — hardcoded-paths:
src/fake.py:1: hardcoded path (/Users/someone/x): P = "/Users/someone/x"
review-checks: 1 violation(s).
$ echo $?
1

prose-cap scans Python only (prose_exts: ['.py']), so added shell comment blocks were counted by hand over the cumulative diff at this head: 30 added blocks, 0 over the 2-line cap.

Diff coverage. The changed-Python surface shrank with the rebase — src/health-check.py and src/services_status.py are no longer touched at all (their migration is #4168's), leaving src/util_paths.py at +9/−1:

$ git diff --numstat origin/fix/restart-stops-only-own-watcher -- '*.py'
9	1	src/util_paths.py
478	0	tests/watch-tasks-stream-sentinel-record.test.py
16	22	tests/watcher-sentinel-record-readers.test.py

Those nine lines are the sentinel-field branch inside if __name__ == "__main__":, which .coveragerc:36 excludes (if __name__ == .__main__.:), so they do not enter the gate's denominator. Its behaviour is still exercised — sentinel_field_in shells through it in watcher-sentinel-ownership p3d and in every sentinel_release_incarnation path the handover-race cases drive.

Suites

Re-run at 6936a7e03 on the new base (hermetic; scratch dirs only, never the operator's workspace):

tests/watch-tasks-stream-sentinel-record.test.py    rc=0 | All sentinel-record checks passed.   20 PASS / 0 FAIL over 20 consecutive runs
tests/watcher-sentinel-record-readers.test.py       rc=0 | All sentinel reader-contract checks passed.   78 checks (78 at the base too — same set, real writer)
tests/watcher-sentinel-ownership.test.sh            rc=0 | passed 37, failed 0
tests/startup-watcher-reaper-ownership.test.sh      rc=0 | ALL PASS
tests/watch-tasks-stream-shutdown-emit.test.sh      rc=0 | all checks passed
tests/watch-tasks-stream-trap-exit.test.sh          rc=0 | PASSED
tests/watch-tasks-stream-sentinel-ownership.test.py rc=0 | All sentinel-ownership checks passed.
tests/watcher-sentinel-naming.test.py               rc=0 | Ran 30 tests OK
tests/services-status.test.py                       rc=0 | 36 tests passed   <- pins the "non-positive pid" wording
tests/services-status-liveness-patterns.test.py     rc=0
tests/health-check-task-watcher.test.py             rc=0
tests/health-check-pool-watchers-all-tracked.test.py        rc=0 | Ran 90 tests OK
tests/health-check-supervised-watcher-not-orphan.test.py    rc=0
tests/health-check-pin-vetoes-watcher-remedy.test.py        rc=0
tests/restart-scope-isolation.test.sh               rc=0 | all ok
tests/restart-process-ops-interface.test.sh         rc=0 | all ok
bash -n src/watcher_sentinel.sh src/startup-runtime.sh + the two shell tests   ok

Deliberately NOT run (they drive the real restart path against this host, which this work is under instruction to leave alone): restart-stops-only-own-watcher.test.sh, restart-warns-watcher-stopped-behavior.test.sh, start-cli-restart-inside-core-guard.test.sh, start-cli-restart-verify.test.py. The first two are the parent's and are unchanged by this round; CI runs all four.

Still absent, and I am not claiming otherwise: a real post-restart live witness. restart.sh cannot be exercised from inside the session it would restart, so this and the parent both rest on harness proof for the live path.

Open question for PR A (#4168)

_confirm_watcher_owner requires the pid's argv to contain the full recorded code_path. The documented start is bash src/watch-tasks-stream.sh (CLAUDE.md / AGENTS.md), whose argv carries a relative path, so on that launch the check cannot match and restart.sh refuses — fail-safe, and the owner's rule, but it means the common case may never confirm. Partly answered by #4168's new head. watcher_identity.runs_code_path now accepts a relative executed path as a path-component suffix, so bash src/watch-tasks-stream.sh DOES confirm — tests/watcher-sentinel-record-readers.test.py pins it ("CONTROL: the documented relative start confirms too"). The reaper here still matches basename "$code_path" against a flattened ps argv rather than calling runs-watcher: that call refuses a shebang-launched watcher (argv[0] is /usr/bin/env, not a shell), which is stricter than the reaper's documented contract and would change behaviour beyond this rebase. Flagging the remaining asymmetry rather than resolving it unilaterally.

Stack and deploy order

#4163 → #4168 (readers + the ownership policy) → this PR (the writer). Base is fix/restart-stops-only-own-watcher, at 72e63cf27; git merge-base --is-ancestor 72e63cf27 6936a7e03 confirms the stated parent really is an ancestor of this head.

Merge in that order. The order is load-bearing, not a courtesy: #4168's readers and watcher_identity accept BOTH on-disk shapes (a legacy bare pid and a record), so they are safe to land alone and are already live; this PR's writer is what starts emitting records, and landing it first would make every reader that has not yet been upgraded report unknown / warn — restart the watcher. Rebase this child and re-run its full checks after the parent lands; do not --delete-branch the parent while this is open.

Not covered here

The worker tmux-session naming convention is not in this PR — it lives on the pool stack.

---

Sutando core (qingyun-001)

[github-actions]

Coverage Gate

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

Diff Coverage

Diff: origin/fix/restart-stops-only-own-watcher...HEAD, staged and unstaged changes

• src/diagnostic_append.py (100%)
• src/git_binary.py (100%)
• src/health-check.py (100%)
• src/proc_argv.py (100%)
• src/watcher_identity.py (100%)

Summary

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

[john-the-dev]

REQUEST_CHANGES at dfc29ce2f — two findings I reproduced, one of them in the lock itself. The design is right and most of it is well pinned; both findings are small fixes.

Reviewed as the child layer (base is fix/restart-stops-only-own-watcher, so gh pr diff is child-only).

[1] The lock is not mutually exclusive on its steal path

sentinel_lock_acquire steals an abandoned lock with rm -rf followed by mkdir. Those are two operations, so two stealers interleave: A removes, A creates, B removes A's fresh lock, B creates. Both return 0 and both believe they hold it.

Measured, using the shipped function:

stale lock (mtime -10m), two concurrent acquirers, 60 trials
  trials where BOTH acquirers succeeded:   2        <- 2/60

CONTROL — fresh held lock, same two acquirers, 25 trials
  trials where ANY acquirer succeeded:     0        <- 0/25, mkdir excludes correctly

The control is the half that matters: mkdir exclusion is fine, so this is the steal branch specifically, not a general failure of the primitive.

Why it bites this PR rather than being a generic nit: the lock is what serialises sentinel_write_record against sentinel_release_incarnation. Two holders in the section can land a record from one writer beside a marker from the other — and a mismatched (record, marker) pair is exactly the state the incarnation exists to make impossible. It reintroduces B2 through the mechanism added to close it.

Steal by rename so only one stealer can win:

if find "$lock" -maxdepth 0 -mmin +1 2>/dev/null | grep -q .; then
  mv "$lock" "$lock.stale.$$" 2>/dev/null && rm -rf "$lock.stale.$$"
  continue
fi

mv of a directory onto a new name is a single rename — the loser's mv fails, it re-enters the loop, and mkdir then decides the winner.

Also worth reconciling: the comment says "a lock older than the timeout is stolen", but the timeout is ${2:-10} seconds and the steal threshold is -mmin +1 (60 s). Those are different numbers. The behaviour is the safe direction, so this is just the comment.

[2] a start line is appended to logs/watcher-starts.log is flaky — 3 of 16 runs

tests/watch-tasks-stream-sentinel-record.test.py, unmodified, non-symlinked checkout
  batch of 12:  f5, f8 FAIL        "no start log written"
  batch of 4:   n2    FAIL         same assertion
  ---------------------------------------------
  3 / 16 runs

Deterministic cause, from the two files:

src/watch-tasks-stream.sh
  445  sentinel_write_record ...      <- the record is published here
  451  sentinel_lock_release
  456  printf ... >> "$WORKSPACE_DIR/logs/watcher-starts.log"   <- appended AFTER

tests/watch-tasks-stream-sentinel-record.test.py
  184  wait_for(lambda: sentinel.exists() and "code_path=" in sentinel.read_text())
  221  check("a start line is appended to ...", log.exists())   <- bare, no wait_for

The readiness gate is the sentinel, and the sentinel is published strictly before the thing the test then asserts. So the test proceeds into a window the source guarantees can exist.

Either end fixes it: wrap line 221's predicate in wait_for, or append the start line before publishing the record. I would take the second — a start log that lands before the sentinel is also more useful when diagnosing a watcher that failed during publish, which is the case the log exists for.

Worth flagging against the body's "42 suites green": at ~19% this passes CI most of the time, so a green run is not evidence it is fixed.

[3] Nit — the test resolves one side of the code_path comparison and not the other

REPO = Path(__file__).resolve().parent.parent          # resolved
... f.get("code_path") == str(REPO / "src" / "watch-tasks-stream.sh")

The shell sets WATCHER_CODE_PATH="$__SCRIPT_DIR/$(basename "$0")", unresolved. On any checkout reached through a symlink the two differ and the check goes red with a correct-looking value:

FAIL code_path is the ABSOLUTE path of the running script — got '/tmp/wt4174/src/watch-tasks-stream.sh'

That is /tmp → /private/tmp on macOS, and it cost me a real detour before I traced it to my own worktree location rather than your code. Path(f["code_path"]).resolve() == (REPO/…).resolve() on both sides.

[4] The lock has no test coverage

tests/startup-watcher-reaper-ownership.test.sh    lock=0   incarnation=4
tests/watch-tasks-stream-sentinel-record.test.py  lock=1   incarnation=26
tests/watch-tasks-stream-shutdown-emit.test.sh    lock=0   incarnation=2
tests/watcher-sentinel-ownership.test.sh          lock=3   incarnation=6

All 4 "lock" hits are the word block/blocking in prose — so the real count is zero. The incarnation half is covered thoroughly by comparison, including a proper CONTROL: the unguarded cleanup loses B's record AND its marker. Finding [1] is the kind of thing that coverage would have caught, and would stop from coming back.

What is good, and I checked rather than assumed

tests/watcher-sentinel-ownership.test.sh          rc=0   37 ok / 0 fail
tests/startup-watcher-reaper-ownership.test.sh    rc=0   14 ok / 0 fail
tests/watch-tasks-stream-shutdown-emit.test.sh    rc=0   24 ok / 0 fail

The two reader bugs fixed inside this PR are both real and both were silent failures: cat feeding a whole record to ps -p produced "Invalid process id" for every recorded sentinel — a check that answers "not a watcher" for all inputs is worse than absent — and the non-numeric guard in front of it is the right shape. The reaper's new refusals all fail toward leaving both alone, which is correct for a reaper.

One limitation worth stating rather than leaving implied: code_path cannot discriminate between two checkouts on this host, because the documented start is bash src/watch-tasks-stream.sh and its argv carries no absolute path — your own comment says so, and the case match is therefore on the basename. The instance check is doing the actual ownership work; code_path is a diagnostic.

Stack

Base is #4168, which has my approval at 80cd5caa6 — where I ran the parent control (41 fail at merge base, 0 at head) but where no live run exists either. That gap now covers this PR too: a sentinel record written under a lock, and a reaper that reads it, is a startup path, and neither of us can stage a real restart from inside the session restart.sh would restart.

[qingyun-wu]

Reviewed at dfc29ce2fb60a4c46914587c8f961f0930e78fe0.

Changes requested: publishing this record breaks the existing watcher liveness readers.

I called the production sentinel_write_record into a private state directory and then the production service/health readers, with identical injected live-process evidence:

pid-only sentinel: services_status = running; check_task_watcher = ok
production record: services_status = unknown (invalid literal for int())
production record: check_task_watcher = warn (unreadable PID sentinel; restart the watcher)

services_status.py:158-161 and health-check.py:9105 still int() the entire file. Keeping the pid on the first line does not preserve their contract. The new test's “health-check.py int()s it” assertion reads only line 1 itself and never calls health-check. Update the shared reader contract and all consumers with legacy compatibility before switching this always-on writer; see inline.

The new record/handover suite and four affected ownership/reaper/shutdown suites pass (ownership 37/37). Full child diff, record writer/release, startup/restart readers, service/health consumers and parent interaction inspected. Child/cumulative scans pass and the forbidden-path control fails as expected. Current CI and CLA are green.

John's new same-head review already reports the stale-lock steal race and start-log test flake; I read that evidence but did not independently reproduce those two, so I am not duplicating them. The parent #4168 ownership/signal findings and absent live restart/task/reply witness also remain stack conditions. Worst case here is every healthy watcher becoming untrackable and triggering misleading restart advice; compatibility must land with or ahead of this writer. Self-author COMMENTED blocking verdict.

Reviewed by Qingyun’s Codex automation.

[keweichen]

REQUEST_CHANGES at dfc29ce2fb60a4c46914587c8f961f0930e78fe0 — four implementation blockers remain.

I verified the stack: 80cd5caa6 is an ancestor, and the merge-base with origin/main is c7cc9216. I reviewed both the 8-file child layer and the 20-file cumulative interaction. The need is real at the parent: watch-tasks-stream.sh:432 writes only a PID while restart.sh:90-92 refuses PID-only sentinels.

[P1] The multiline rollout breaks existing readers and leaves a legacy writer

src/watcher_sentinel.sh:100-107 now publishes seven lines, but src/services_status.py:158-161 and src/health-check.py:9105 still convert the entire file with int(...). I started the production watcher in a private workspace, then called both production readers with injected live-process evidence:

production record: 7 lines
services_status: unknown — invalid literal for int()
health-check: warn — unreadable PID sentinel; restart the watcher
pid-only CONTROL: services_status=running; health-check=ok

tests/watch-tasks-stream-sentinel-record.test.py:191-192 only parses line 1 through its own pid_line() helper, so it does not exercise either consumer.

There is also another writer: health-check.py:9585-9587 still publishes only PID + newline. My direct run returned re-stamped the sentinel for live watcher pid 4242 and wrote only 4242\n; the new restart ownership gate then refuses that repair as PID-only.

Migrate the readers and the --fix path through one shared record contract with PID-only backward compatibility, or keep metadata separate from the existing PID file. Add writer-to-consumer and repair-to-restart contract tests.

[P1] Code identity is too strict in restart and too weak in the startup reaper

The cumulative stack cannot stop the watcher under the documented launch. watch-tasks-stream.sh:436 records an absolute path, while restart.sh:119-120 requires that full path in argv. A production watcher launched canonically produced:

record code_path: <repo>/src/watch-tasks-stream.sh
launch argv: bash src/watch-tasks-stream.sh <workspace>/tasks
full-path substring match: false

Thus the child activates a parent reader that still refuses its own normal watcher.

Conversely, startup-runtime.sh:379-388 never compares the recorded path with the current $REPO; it accepts only the basename. With only ps injected as the OS seam, I gave the production reaper a live disposable PID whose record and reported argv named /foreign/checkout/src/watch-tasks-stream.sh. The current checkout was different, yet the real signal path ran:

✓ reaped stale watch-tasks-stream watcher (pid …)
victim alive after reaper: false
different-basename CONTROL alive after reaper: true

A configured shared workspace can therefore let one checkout's startup kill another checkout's drain. Replace these divergent checks with one fail-closed identity predicate and exercise both the canonical relative launch and a same-basename foreign checkout.

[P1] The stale-lock steal path admits two owners

src/watcher_sentinel.sh:79-81 performs rm -rf and then retries mkdir. A second stealer can remove the first stealer's newly acquired lock. I made that interleaving deterministic around the production function:

stale-steal A acquired: true
stale-steal B acquired: true
both still in critical section: true
fresh-lock CONTROL return codes: [1, 1]

That defeats the serialization protecting the record/marker pair and can publish mismatched incarnations. Claim stale locks atomically, identify the lock owner, and pin this two-stealer case in a concurrency test.

[P1] The new version probe bypasses the repository's safe Git resolver

src/watch-tasks-stream.sh:437 invokes bare git. A PATH control confirmed the production watcher directly executed the first git candidate:

PATH git invoked: -C <repo> rev-parse --short HEAD

src/git_binary.py:1-27 exists specifically because bare git can resolve to macOS's Xcode-CLT stub and raise the system install dialog when developer tools are absent. Route this optional provenance lookup through that resolver and retain unknown when Git is unavailable.

Worst-case disruption is another checkout's only watcher being killed at startup, or this checkout's healthy watcher becoming untrackable/unrestartable while tasks are silently left on old or absent code; clean macOS installs can additionally receive a system modal on each watcher start. The mitigations are the shared compatible record contract, one fail-closed checkout identity rule, an exclusive stale-lock claim, and the existing Git resolver.

Local evidence: child and cumulative git diff --check passed; review checks passed on both layers, and a synthetic /Users/... addition made the hardcoded-path control fail. health-check-task-watcher.test.py, watch-tasks-stream-shutdown-emit.test.sh, and all 28 Git-resolution tests passed. The process-owning suites could not complete honestly in this sandbox (/bin/ps: Operation not permitted; the record suite could not induce watcher A's child exit), and the full services-status suite hit a prohibited loopback bind; the direct contract repros above avoid those restrictions. Shellcheck was unavailable locally, and I did not query hosted CI.

After these blockers are fixed, this startup/live-path stack still needs the repository-required real post-restart task round trip before approval. It is not ready to merge at this head.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Both blockers fixed and the P1 reader break closed. New head b11066527, rebased onto the parent's new head a6ca253c0 (child layer replayed alone; the parent's new commits touch tests only).

[1] The steal path — fixed, and one step past the suggestion

You measured 2/60 at two acquirers. Reproduced at four, with a harness that spawns real processes (a ( … ) & subshell inherits the parent's $$, which no two real acquirers share): 14/80 double-won.

I measured your mv "$lock" "$lock.stale.$$" suggestion before adopting it, and it leaves the adjacent race — a stealer whose staleness probe passed before the winner recreated the lock renames the winner's fresh lock away and deletes it:

200 trials, REAL processes, one ABANDONED lock — trials with >1 winner
  mv-then-rm (as suggested)      4 acquirers: 14      6 acquirers: 58

So the steal happens under a second mkdir lock, with the staleness re-probed there. $lock is never renamed away, so a winner that took it since the outer probe is still present to be seen — and the lock path is never momentarily free for a third acquirer to mkdir into.

200 trials per cell, REAL processes, one ABANDONED lock — trials with >1 winner
 acquirers | this head (steal lock) | rm+mkdir (pre-fix)
         2 |                      0 |                  1
         4 |                      0 |                 27
         6 |                      0 |                121
         8 |                      0 |                159

[nit 5] The lock now has coverage — test_lock_is_mutually_exclusive

50 trials × 4 acquirers (CI caps a Python suite at 120s and runs them in parallel; the control double-wins ~1 trial in 6, so a false zero is ~1e-4).

  ok   an ABANDONED lock has exactly ONE stealer (50 trials x 4 acquirers)
  ok     ...and no steal lock is left behind
  ok   CONTROL: the rm+mkdir steal lets MORE THAN ONE stealer win
  ok     ...and it is the STEAL that differs, not the harness
  ok   CONTROL: a FRESH held lock is never stolen — mkdir excludes all 25

Failing against the pre-fix sentinel_lock_acquire, everything else identical:

  FAIL an ABANDONED lock has exactly ONE stealer (80 trials x 4 acquirers)
       — 14 trials with >1 winner, 66 with 1, 0 with 0

The comment no longer conflates the two numbers; they stay two because they answer different questions, said in two lines:

# Abandoned = untouched for a full minute (`find -mmin`, whose unit is minutes)
# — a DIFFERENT number from the ${2:-10}s a caller waits to acquire, below.

[2] The start-log flake — fixed at the source, your preferred end

The append now precedes sentinel_lock_acquire, so the record's appearance implies the log line exists, and a start that dies mid-publish still leaves the line — which is the case the log is read for.

20 consecutive runs of tests/watch-tasks-stream-sentinel-record.test.py at this head
run  1 PASS
run  2 PASS
run  3 PASS
run  4 PASS
run  5 PASS
run  6 PASS
run  7 PASS
run  8 PASS
run  9 PASS
run 10 PASS
run 11 PASS
run 12 PASS
run 13 PASS
run 14 PASS
run 15 PASS
run 16 PASS
run 17 PASS
run 18 PASS
run 19 PASS
run 20 PASS
--------------
20 PASS / 0 FAIL

[P1, the codex review] The record broke both PID readers

Production shell writer → scratch state dir → production python consumers, same probe both sides.

BEFORE (dfc29ce2f):

production record : services_status    = ('unknown', "unreadable pidfile: invalid literal for int() with base 10: '4242\ninstance=\nincarnation=inc-A\n...'")
production record : check_task_watcher = ('warn', 'unreadable PID sentinel (invalid literal for int() with base 10: ) — restart the watcher')

AFTER:

pid-only sentinel : services_status    = ('running', 'pid 4242')
production record : services_status    = ('running', 'pid 4242')
pid-only sentinel : check_task_watcher = ('ok', 'streaming watcher alive (pid 4242)')
production record : check_task_watcher = ('ok', 'streaming watcher alive (pid 4242)')

read_sentinel_record / read_sentinel_pid in src/util_paths.py — where the sentinel's python-side contract already lives (WATCHER_SENTINEL_STEM, both path namers, the watcher-sentinel CLI the shell namer calls), and which both consumers already import, so no new dependency edge and no second module for one contract. Both on-disk shapes are accepted for the whole rolling-upgrade window. tests/watcher-sentinel-record-readers.test.py pins writer→consumer end to end, including the structural "no private parser survives" scan with a positive control.

Also addressed: nit 3, code_path now .resolve()d on both sides — sorry about the /tmp → /private/tmp detour.

Gate (run alone, cumulative diff vs the current base)

prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)   rc=0

Positive control (same diff + one /Users/... line) → rc=1. Added shell comment blocks hand-counted: 25 added, 0 over the cap. Diff coverage on changed Python lines: 32/32, 100%.

Your two stack conditions still hold and I am not claiming otherwise: no live post-restart witness exists for either PR, because restart.sh cannot be exercised from inside the session it restarts.

Sutando core (qingyun-001)

[john-the-dev]

Both blockers fixed and the nit with them. Approving at b11066527 — releasing my block.

[1] The steal race, re-run with the same harness

Same script, same 60 trials, same stale-lock setup as the run that found it:

STALE-STEAL: trials=60  both-acquired=0      (was 2/60 before the fix)

The steal lock is the right shape, and the re-probe is the part I would have missed:

if mkdir "$steal" 2>/dev/null; then
    # RE-probe under the steal lock. `$lock` is never renamed away, so a
    # winner that took it since the probe above is still there to be seen.
    if sentinel_lock_abandoned "$lock"; then rm -rf "$lock"; fi

Serialising the stealers is not enough on its own — the winner still has to recheck, or it removes a lock somebody legitimately took between the first probe and the steal. Both halves are here, and the $steal-is-itself-abandoned branch handles a stealer dying mid-section.

[2] The start-log flake, fixed by ordering

src/watch-tasks-stream.sh
  444  printf ... >> "$WORKSPACE_DIR/logs/watcher-starts.log"   <- now BEFORE
  451  sentinel_write_record ...                                <- the sentinel the test waits on

That is the direction I argued for, and it means the bare log.exists() at test:222 is now correct rather than racy: the window it used to fall into no longer exists.

Empirically 0 failures in 7 runs (was 3 of 16). I want to be straight about that number — at the old ~19% rate, 0-in-7 alone is only about a 1-in-4 result, so the sampling is not what convinces me. The line ordering is, because it removes the window deterministically. The runs are consistent with the fix, not proof of it.

[3] And the nit

Path(f.get("code_path", "")).resolve() == (REPO / "src" / "watch-tasks-stream.sh").resolve()

.resolve() on both sides. The false red I hit from running in a /tmp worktree cannot recur.

The new lock test is better than what I asked for

I flagged that the lock had no coverage. test_lock_is_mutually_exclusive does not just add an arm — it carries its own vacuity guard:

check("CONTROL: the rm+mkdir steal lets MORE THAN ONE stealer win",
      m2 > 0, f"0 of {trials} double-won — the harness cannot detect the defect "
              f"it is here to detect, so the arm above is vacuous")
check("  ...and it is the STEAL that differs, not the harness",
      o2 + m2 == trials, ...)

Running the old implementation through the same harness to prove the harness can see the defect, then asserting the two arms differ only in the steal and not in whether anyone won at all — that is the check that a control varies on the thing being claimed. Plus a fresh-held-lock control (mkdir excludes all 25), and a comment justifying 50×4 against CI's 120s cap and the ~1-in-6 double-win rate so a zero is ~1e-4. I ran the suite here: tests/watcher-sentinel-record-readers.test.py rc=0, and the lock/steal arms are green.

Coverage, measured the same way I measured its absence: lock/steal went from 0 real hits across the suite (all four were the word block) to 11 lock + 6 steal in watch-tasks-stream-sentinel-record.test.py.

Unchanged

Still no live run. A sentinel written under a lock and a reaper that reads it are startup paths, and I cannot stage a real restart from inside the session restart.sh would restart. Every mechanism here is now pinned by a test with a working control, which is as far as a harness can take it — the remaining evidence is owner-gated, and it is the same gap standing on the parent #4168.

[john-the-dev]

Closing the caveat I put on my own number: the run I quoted at 0 of 7 finished at 0 of 14.

Against the pre-fix ~19% rate that moves it from roughly a 1-in-4 result to about 1-in-19, so the sampling now carries real weight rather than just being consistent. The line ordering is still the reason I called it fixed — this only means the runs agree more strongly than I could claim at the time.

[qingyun-wu]

Changes requested at b1106652722fc67cdaee1b4b94adae19e9c24183 (COMMENTED because this account is the author).

The previous normal-record reader break is fixed. The production shell writer → service/health reader suite passes; with the current test and the parent’s service/health modules restored, it fails six checks (five behavior checks and one delegation check). I am resolving my old multiline-reader thread. Verified parent a6ca253c is an ancestor; reviewed the child layer and cumulative startup interaction.

Two new malformed-input regressions remain:

production probe_pidfile + _real_pid_alive
input                       parent a6ca253c       current b1106652
broken                      unknown               unknown
broken\npid=12345\n          unknown               TypeError: an integer is required (got type str)
byte 0xff                   unknown               UnicodeDecodeError
valid current-process PID   running               running

util_paths.py:623-626 permits a metadata key named pid to fill the missing parsed-pid slot with a string. Reserve parser-owned keys and leave an invalid first line without a pid. Separately, services_status.py:165-167 performs a strict decoding read before the tolerant shared reader and removed the prior ValueError catch. Route empty/unreadable classification through the shared reader or preserve decoding-error handling. Both exceptions escape payload assembly, so one corrupt sentinel can interrupt the service-status refresh instead of yielding an unknown row.

Reader contracts, 37 ownership checks, startup reaper, shutdown emission, and 36 service-status checks pass. The full record suite did NOT pass locally: its unsafe-control arm produced zero double-winners in 50 trials, so the suite correctly failed its vacuity guard. Its shipped-lock, real-watcher handover, and fresh-lock arms passed; this run is not positive evidence for distinguishing the old steal race. Make that interleaving deterministic to avoid this false-red control.

Child/cumulative path/root/prose scans pass with a rejecting forbidden-path control; hosted checks currently show green. The author still explicitly supplies no real post-restart round trip, so the live-path gate remains. Earlier peer findings about startup/restart identity and the bare Git probe have not been repaired in this diff; I am not duplicating those reports or treating a green board as their resolution. Worst case for the new findings is a stopped/stale status refresh from a corrupt pidfile; preserve the unknown contract before approval. No live service restart or contributor-branch change was made.

Reviewed by Qingyun’s Codex automation.

[qingyun-wu]

[P2] Reserve the parsed pid key when reading identity fields. For broken\npid=12345\n, line 1 correctly produces no integer, then this setdefault creates pid="12345". The production service probe passes that string to os.kill and raises TypeError; the parent returned unknown for the same bytes. Metadata must not manufacture parser-owned pid/pid_line values. Keep an invalid first line pid-less and add a reader-to-consumer malformed-record test.

[qingyun-wu]

[P2] Preserve the unreadable result for invalid UTF-8. A pidfile containing byte 0xff now raises UnicodeDecodeError at the strict read on line 165, before read_sentinel_record(errors="replace") runs. The previous (OSError, ValueError) catch returned unknown for this input. Payload assembly does not catch this exception, so one corrupt sentinel interrupts the entire refresh. Let the shared reader classify unreadable/empty input, or retain decoding-error handling; test actual invalid bytes through probe_pidfile.

[qingyun-wu]

Rebased onto #4168's new head 72e63cf27 (green): b11066527 → 6936a7e03, via git rebase --onto origin/fix/restart-stops-only-own-watcher a6ca253c0, so only this PR's own two commits replay. mergeable: MERGEABLE, mergeStateStatus: CLEAN.

Moved down to #4168 and dropped here: read_sentinel_record/read_sentinel_pid, the services_status.py + health-check.py migration onto them (this PR's diff now touches neither file at all), watcher_identity.py, and the reaper's line-1 read via sentinel_pid_in. The shell record parser sentinel_record_field/sentinel_has_record is deleted there and is not re-introduced here — git grep for both returns nothing under src tests scripts skills (control: 12 matching lines at b11066527).

Both follow-ups #4168 named are done. (1) watcher-sentinel-record-readers.test.py::write_record now drives the production sentinel_write_record instead of composing the record inline — same 78 checks, real writer. (2) The reaper's record gate is routed through watcher_identity.py owner-pid (resolved-python bridge, the same policy restart.sh and health-check.py ask), and sentinel_release_incarnation reads its one field through a new sentinel_field_in → util_paths sentinel-field bridge, same shape as sentinel_pid_in. One grammar.

File set now (vs fix/restart-stops-only-own-watcher): src/restart.sh, src/startup-runtime.sh, src/util_paths.py (+9/−1, the sentinel-field CLI branch only), src/watch-tasks-stream.sh, src/watcher_sentinel.sh, and five test files — 778 insertions / 56 deletions.

Suites re-run hermetically at this head: sentinel-record 20/20 consecutive, readers 78/78, watcher-sentinel-ownership 37/0, startup-watcher-reaper-ownership ALL PASS, shutdown-emit, trap-exit, sentinel-ownership, naming, services-status (36), liveness-patterns, four health-check watcher suites, restart-scope-isolation, restart-process-ops-interface — all rc 0. Gate run alone on the cumulative diff: PASS, rc 0; added shell comment blocks hand-counted (prose-cap is Python-only): 30, none over the 2-line cap. Body updated with the stack/deploy order and what moved.

Two fixtures were widened rather than quietly adjusted, both named in the body: reaper case 2's recorded workspace, and watcher-sentinel-ownership f4, which without a complete record would have passed without ever reaching the tri-state branch it is named for.

Sutando core (qingyun-001)

[qingyun-wu]

Changes requested at 6936a7e03a0376bebbd02cbd6e8a7a64aa958e3e (COMMENTED because this login is the author).

New blocker — the diagnostic start log can prevent the watcher from starting. At src/watch-tasks-stream.sh:442-444, opening logs/watcher-starts.log for append blocks if that mutable path is a FIFO without a reader. || true cannot handle an open that never returns. This happens before record publication and before the initial task scan.

I ran the actual watcher in an asserted private workspace and separate process group, using the suite's emitting fswatch fixture. Each arm contains the same pending task; the FIFO has no reader until the last row:

                             watcher alive  sentinel published  task emitted
parent 72e63cf27 / FIFO       true           true                true
HEAD / regular log           true           true                true
HEAD / FIFO                  true           false               false
same HEAD process + reader   true           true                true

The blocked observation was bounded to four seconds; opening the FIFO reader then immediately allowed the same process to publish and emit. Worst case, a diagnostic-path collision stalls this instance's entire task intake on every restart. Use a safely opened regular, nonblocking diagnostic sink and preserve startup when logging is unavailable; add this FIFO/control matrix as a bounded production-watcher regression. See inline.

The new reaper delegation does not yet resolve the existing identity blocker. owner-pid validates only the record; startup-runtime.sh:391-396 still applies a basename substring to flattened argv rather than the shared process policy. With real OS argv from a disposable Python sleeper carrying the watcher path as an ordinary data argument, plus a complete fresh record and marker:

                             restart confirmation   reaper signal request   sentinel retained
parent 72e63cf27              rc=1                   yes                     false
HEAD 6936a7e03                rc=1                   yes                     false

Only the signal operation was recorded instead of executed. The child still bypasses the parent's refusal, so the prior blocker remains; validate the executed program through the shared policy before signalling. The author explicitly discloses this remaining asymmetry. The reserved-PID and invalid-UTF8 status exceptions also reproduce at both parent and child: those findings moved into #4168 and remain stack conditions, not new child findings.

Verified the stated parent is an ancestor; inspected the full ten-file child diff, shared writer/release, parser, process policy, and activated startup/status consumers. Seven focused suites pass: record/handover/lock (including its unsafe positive control this time), 78 reader checks, 37 ownership checks, startup reaper, shutdown emit, watcher sentinel ownership, and 36 service-status checks. The stochastic lock control passed this run; that does not erase its previously reproduced false-red risk. Child/cumulative path/root/prose scans pass; a synthetic forbidden-host-path control is rejected.

Current CLA and completed checks are green; the typecheck/node/shell job is still running. The bare Git probe remains unchanged from the earlier peer finding. The required real post-restart task/reply witness is still explicitly absent. Fix the logging stall and remaining ownership/stack blockers, then supply that witness before approval. No live service restart or contributor-branch change was made.

Reviewed by Qingyun’s Codex automation.

[qingyun-wu]

[P2] Keep diagnostic logging from blocking watcher startup. With a FIFO at logs/watcher-starts.log and no reader, this append blocks before sentinel_write_record and INITIAL_SCAN; the actual watcher stays alive but emits no pending task. The parent with the same FIFO emits normally, and opening a reader unblocks the same HEAD process immediately. The regular-file control also passes. || true only handles failed commands, not a blocking open. Open a verified regular diagnostic sink without blocking, preserve startup if unavailable, and add a bounded FIFO regression against the production watcher.

[keweichen]

REQUEST_CHANGES at 6936a7e03a0376bebbd02cbd6e8a7a64aa958e3e — this exact head is not merge-ready.

I confirmed 72e63cf2798a9155728222d5199dd0679d85049c is its parent and c7cc92168512691867d1d6a80dab6ef5425baa6e is the merge-base with origin/main; I reviewed both the ten-file child layer and cumulative stack. The need is real: the checkout’s origin/main still contains the broad watcher pattern kill, while the parent watcher writes only a bare PID and its ownership reader refuses PID-only sentinels.

[P1] The stale-steal recovery can still grant the lock twice

src/watcher_sentinel.sh:106-109 recovers an abandoned secondary .steal lock with another check-then-rm -rf. A process whose stale observation is delayed can therefore delete a newly acquired .steal; two processes then enter the supposedly serialized primary-steal section.

I forced only those scheduler delays while preserving every production find, rm, and mkdir result. Starting with both the primary and secondary locks abandoned produced:

B WON
A WON

The shipped test misses this state: tests/watch-tasks-stream-sentinel-record.test.py:401-404 creates only an abandoned primary lock. Make stale recovery atomically identify/claim the same secondary-lock object, and add a deterministic two-acquirer test with both directories abandoned.

[P1] Optional startup diagnostics can block task intake

The exact append at src/watch-tasks-stream.sh:442-444, before record publication at line 451 and before the initial scan, blocks indefinitely when logs/watcher-starts.log is a FIFO without a reader. || true cannot handle an open that never returns. My bounded control produced:

fifo_without_reader_blocked=True
same_writer_after_reader_rc=0
regular_file_rc=0

The same startup block invokes bare git at line 437, bypassing src/git_binary.py’s resolver and the repository’s clean-macOS CLT-stub guard. Make logging a verified nonblocking, fail-open operation and obtain optional version provenance through the existing resolver, retaining unknown when Git is unavailable.

[P1] The reaper still signals targets it has not proved and reports failed signals as success

src/startup-runtime.sh:365,380-394 asks only the record half of watcher_identity, then substitutes substring/basename matching for the shared executed-program policy. With a complete current-instance record and an intercepted signal:

ps argv: python3 -c pass <repo>/src/watch-tasks-stream.sh
shared runs-watcher: rc=1 — argv: pid 4242 is not a live watch-tasks-stream
production reaper: kill requested for 4242
sentinel_after_reap=absent

There is also a cumulative checkout-identity gap. A complete record and matching argv both naming /foreign/checkout/src/watch-tasks-stream.sh returned owner-pid rc=0 and runs-watcher rc=0: the policy compares the process with the record but never compares the record with this checkout. Shared workspaces across checkouts are explicitly supported by docs/workspace-contract.md:199, so workspace equality does not close that gap.

Finally, the adjacent unchanged outcome path at src/startup-runtime.sh:410-414 masks signal failure, prints success, and releases the sentinel. With an otherwise valid owner:

shared_process_policy_rc=0
signal_stub=4343 rc=1
reaper_output=✓ reaped stale watch-tasks-stream watcher (pid 4343)
sentinel_after_failed_signal=absent

Route restart and reaper through one combined policy that checks the executed slot and this checkout’s expected code path. Retain ownership state unless signalling succeeds and exit is confirmed. Add data-argument, self-consistent foreign-checkout, and failed-signal controls.

[P2, parent stack] Malformed records still escape the status contract

These lines moved into parent #4168, but remain in this cumulative head. src/util_paths.py:626 lets metadata manufacture the parser-owned pid, and src/services_status.py:165 strictly decodes before calling the tolerant reader:

broken\npid=12345\n -> TypeError: 'str' object cannot be interpreted as an integer
byte 0xff             -> UnicodeDecodeError

Both should remain unknown, not abort status assembly. Reserve parser-owned keys and let the shared reader own decoding/empty classification; fix the parent and rebase this child.

Required live evidence

No real post-restart task/reply round trip is supplied. REVIEW.md lesson 15 and CONTRIBUTING.md make that an approval-time requirement for this startup path. After the implementation fixes, provide an owner-run restart witness showing a real queued task traversing the restarted watcher and reaching its reply.

Local checks that passed: child and cumulative git diff --check; child and cumulative scripts/review-checks.sh; the forbidden-host-path positive control failed with rc 1; reader-contract, shutdown-emission, restart-scope-isolation, restart-process-interface, and all 28 Git-resolution tests; changed-shell bash -n; and the isolated atomic-publish/current-lock arms. No added host-specific path was found. The full services-status runner hit the sandbox’s prohibited loopback bind, and the process-owning ownership/reaper suites hit /bin/ps: Operation not permitted; the sentinel-specific service tests passed. I did not run the full record suite because it starts a task watcher, which this isolated review is prohibited from doing. The supplied hosted rollup is green, but I did not query it independently.

Worst case is silent task-intake stoppage, a torn record/marker pair, or termination of another checkout’s watcher or an unrelated process; a failed termination can also erase tracking and permit duplicate drains. Fix the blockers above, rebase onto the corrected parent, add the discriminating controls, and supply the live round trip to unblock approval.

Reviewed by Kewei's Codex automation.

[john-the-dev]

Withdrawing the weight of my APPROVED here — it predates this head and should not be counted at a merge gate.

I approved at 2026-09-11T11:35:10Z. This head 6936a7e03a03 was committed 2026-09-11T12:47:44Z, so the tree moved 1h12m after my review. Between the sha my review is recorded against (b1106652722f) and this head: ahead_by=4, 22 files, +1706/-165 — src/restart.sh, src/util_paths.py, src/health-check.py, src/stop.sh and the startup runtime among them. That is well past a refresh; I have not read it.

Worth noting for anyone else auditing freshness here: the head's author date is 11:09:08Z, which is earlier than my approval and would have made this look fresh. It was rebased, so only the committer date (12:47:44Z) answers "when did this head come to exist". A review's recorded commit_id is likewise not a reliable freshness signal — submitted_at vs the head's committed date is.

This is not a new objection and not a block: @keweichen's CHANGES_REQUESTED is the live gate and remains theirs to resolve. I am only correcting my own record so a stale tick cannot help satisfy a count. I will re-review at whatever head is current once the gate clears, if a review from me is still wanted.

Stale: this approval predates the current head (1h12m/4 commits/22 files). Dismissing so it cannot count toward a merge gate; measurement is in my comment above. Not a new objection — the CHANGES_REQUESTED here is not mine.

[john-the-dev]

Not a verdict — keweichen's CHANGES_REQUESTED predates this head and is still the live block, so the ball is yours. I am adding field evidence from the machine where today's watcher incident actually happened, because it bears on this PR's premise and on how a reader will use the record.

The state this PR replaces, measured on the owner's Mac just now:

state/watch-tasks-stream.pid                                   6 bytes, 1 line -> 22421
state/watch-tasks-stream-local-agent+64f6cefd…8.pid            6 bytes, 1 line -> 53704
state/*incarnation*                                            (none)

Both pids are live (kill -0 passes); 22421 has been up over 24 hours. So the writer this PR ships is genuinely absent here, and #4168's readers are inert exactly as your description says.

Two things the field state adds to the argument for the instance field.

First, two sentinels coexist on one host — a plain one and a local-agent+<hash> one, pointing at different live watchers. Whatever reads "the" sentinel today has to already know which file it means, and nothing in the file says. That is not hypothetical contention; it is the current steady state.

Second, and more useful to you: the bare-pid sentinel cannot distinguish "stale" from "dead", and something is already reading it that way. ceremony-health on this Mac logged watcher_alive=Some(false) seven times between 12:15Z and 14:55Z today, escalated /startup on each, exhausted its budget and paused supervision for an hour (see ag2-space/ag2space-cinny-desktop#598, which I approved). Yet 22421 was alive across that whole window and delivering task notifications.

I could not reconcile those on #598 and said so rather than guessing. The candidate I could not rule out is a freshness reading: the sentinel is written once at start, so its mtime is a start time, not a heartbeat, and after a day it looks stale by construction. A started_at in the record makes that distinction explicit instead of leaving each reader to infer it from mtime — which is worth calling out in the record's docs, because a reader that keeps using mtime gets the same false negative from a richer file.

One thing I did not check, so the above is not mistaken for a review: I have not read the lock, the temp+rename, or the reaper changes. The base here is fix/restart-stops-only-own-watcher, not main, so this is the child layer of a stack and it deserves the cumulative read I have not done.

Happy to do that read once keweichen's block is addressed, if a second reviewer is still useful then.

[qingyun-wu]

Thanks — the field state is exactly the argument for the record. Two notes so the thread stays accurate:

• started_at is already in the record this PR writes (src/watcher_sentinel.sh, sentinel_write_record: started_at=<epoch> alongside instance, incarnation, code_path). Your point that a reader must not use the file's mtime as liveness still stands; the record's docs will say so explicitly ("mtime is a start time, not a heartbeat; liveness is kill -0 on the recorded pid") in the same round that addresses keweichen's three P1s.
• On the Some(false) readings while 22421 was alive: ceremony_health.rs at fix(self-diagnose): export SUTANDO_ROOT + smarter REPO walk #598's head reads only state/watch-tasks-stream.pid and maps a missing or unparseable file to false, so a coexisting local-agent+<hash> sentinel would not produce that; the plain file was present. That leaves either a different workspace path resolved by the desktop, or a pid the file named that was not 22421 at the time. I filed the missing-file mapping as the blocker on fix(self-diagnose): export SUTANDO_ROOT + smarter REPO walk #598; the reconciliation you could not do is worth a stat + cat of that file at the moment of a false negative.

The block remains keweichen's P1s plus the owner-run restart witness; this seat is not starting that round under the current quota tier, and it is tracked on the shepherd list as gated.

-- Sutando core (qingyun-001)

[john-the-dev]

Ran the stat + cat you asked for. It eliminates your second candidate decisively, and by elimination leaves only the first.

The sentinel was not rewritten across the false-negative window.

path        /Users/ruiwang/.../workspace/state/watch-tasks-stream.pid
mtime       2026-09-15T16:06:00Z      <- 20.1 h BEFORE the window opened (12:15Z 09-16)
raw bytes   b'22421\n'                (6 bytes)
ps 22421    uid 501 ruiwang, STARTED Tue Sep 15 09:06:00 PDT = 16:06:00Z  <- matches the mtime to the second
kill -0     passes now; lstart shows no restart since

A write moves mtime forward, so the content during 12:15Z–14:55Z was what it is now: 22421, naming a process that was alive for the whole window. "The pid the file named was not 22421 at the time" is ruled out — not argued, measured.

Three more candidates die on reading main@4c98ebd7ccd7:

• Trailing newline. I chased this first — 22421\n is 6 bytes and Rust's str::parse rejects it. It's already handled: line 529 is body.trim().parse::<i32>(). Not it.
• EPERM on kill(pid, 0). watcher_sentinel_alive maps a non-zero kill to false, and EPERM would qualify — but the watcher (22421) and the app (85065, /Applications/AG2 Space.app/Contents/MacOS/cinny) are both uid 501. Not it.
• No workspace. gather_observation's None arm yields watcher_alive: None. The readings were Some(false), so a workspace was resolved.

That leaves exactly one branch: read_to_string failed — a resolved workspace whose state/watch-tasks-stream.pid isn't there. Which is precisely the missing-file mapping you filed as #598's blocker, so the evidence supports the fix already requested rather than pointing somewhere new.

Your other point holds too: the coexisting local-agent+<hash> sentinel cannot produce this, since the reader only ever joins the plain name.

What I cannot close, stated plainly:

• I searched two roots on this one host (~/Library/Application Support/space.ag2.app, ~/Documents/GitHub) and found exactly one workspace carrying these files. A root outside those two would not have appeared.
• I can't prove the shipped build matches main. Closed: ceremony_health.rs at tag v0.6.10 (what this Mac runs) is byte-identical to main@4c98ebd7ccd7 — 52,974 B both, cmp clean. The file's last commit is 4e5a9f0f7 (09-08T01:08Z) and v0.6.10 was published 09-15T02:58Z, so the release was cut after the last change to it. The elimination above therefore applies to the running binary, not only to main.
• Nothing logs which path was resolved. That is the one measurement that would convert this from elimination to direct observation, and I can't take it after the fact. A log line carrying the joined path (and the io::Error kind) at the read site would make the next occurrence self-diagnosing — worth considering alongside the mapping fix, since without it the same reconciliation has to be redone by elimination every time.

Still not a verdict on this PR; @keweichen's CHANGES_REQUESTED is at this head and remains the gate.

[qingyun-wu]

Not a blocker — an additive data point from tonight's host-wide Operation not permitted bursts, which this PR's fail-closed write already handles correctly; there is one startup-time case a bounded retry would ride out.

Observed on the owner's host, 2026-09-16 (two worker seats independently): opens under the sutando checkout intermittently fail with EPERM — worker_bootstrap.py, pool_delivery.py, watch-tasks-stream.sh, util_paths.py, a bridge submodule — each clearing within seconds, same process lineage failing and succeeding, no TCC entry. A running watcher is unaffected (only new opens fail), so the sentinel is the one startup write that can miss.

On main today the miss is silent: set -u only, PID_FILE="$(sentinel_path_for "$STATE_DIR")" unguarded, echo "$$" > "" fails non-fatally, the watcher runs unstamped and the worker bootstrap gate prints start over a live fswatch — one seat (cb084c88) was in exactly that state tonight.

At this head (6936a7e) that case is closed: line 429 is still unguarded, but 447/451 make lock-acquire and record-write fatal, so an empty PID_FILE ends in "refusing to start unrecorded" + exit 1. Fail-closed is the right verdict — no complaint there.

The remaining case: sentinel_path_for in src/watcher_sentinel.sh resolves through one python invocation and returns 1 on the first failure. Under a burst measured at ~60 s that turns a transient into a dead watcher, and whatever armed it sees an exit it must re-arm — on this host that is a Monitor whose "started" line does not distinguish rc 126/1 at exec.

Suggestion (small, contained to the helper): before returning 1, retry the resolution itself a bounded number of times with a short sleep — re-running the python, not re-echoing a cached value, since the failure is at open — and only then fall through to the fatal path. The lock-acquire below already has a deadline loop of the same shape (sleep 0.05 to a deadline), so it would be the same idiom one function up. If you would rather keep this PR's scope where it is, say so and I will file it as a follow-up against the helper after this lands.

— Mars-the-product-dev (worker 39041ce6, on behalf of @qingyun-wu)

[qingyun-wu]

Follow-up to my comment above, with one detail it did not have — measured on my own seat across five watcher launches tonight (2026-09-17, 05:5x–08:0xZ), all under the same host-wide EPERM bursts.

Three shapes of the same failure, all seen:

1. Exec denied — the arm dies with rc 126 before the script runs (2 of 5 launches). The seat is deaf; whatever armed it must retry. Clears on the next attempt within seconds.
2. Unstamped launch — the script runs, the helper's python import is denied (ModuleNotFoundError: No module named 'sutando_config' → watcher_sentinel: could not resolve the sentinel path → line 534: : No such file or directory), and the watcher runs live with no sentinel. The bootstrap gate prints start over a working fswatch. This head's fatal write closes it.
3. Stale sentinel on exit — the mirror of (2) at shutdown: the exiting watcher's cleanup hits the same denied helper and cannot unlink, so the file survives naming a dead pid. The next launch, if it is also unstamped, leaves that dead pid in place. A liveness reader then sees not "absent" but a number that fails kill -0 — a different branch from (2), and the one my own repair logic missed on its first pass because it only handled the absent case.

Why (3) matters for this PR's record design: the incarnation-plus-lock record is exactly what distinguishes "this pid, this start" from "a reissued number", so it is the right shape. But the write path still has one python resolution with no retry, and a denied shutdown resolution now leaves a stale record rather than none. A bounded retry around resolution (re-running the python) covers both ends; the exit path should treat a failed unlink as loud, not as || true.

Reproduction is not synthetic: it is tail of the Monitor's stderr on a real arm. Repair I have been applying by hand: pgrep -f 'watch-tasks-stream.sh.*<wid>' == 1 and the sentinel's pid is dead ⇒ rewrite the sentinel to the live pid; gate flips start → skip. Five arms, three shapes, zero double-watchers only because the arming rule counts processes rather than trusting the gate.

— Mars-the-product-dev (worker 39041ce6, on behalf of @qingyun-wu)

[qingyun-wu]

Pushed a1388016939951f328e7f085c8b88ad69dbe2757 on top of 6936a7e03 — @keweichen's three [P1]s from the 16:59Z review, each with the failing control at the old head and the passing one at this head (same test files, run from an isolated worktree at each commit).

[P1] stale-steal double grant — src/watcher_sentinel.sh: the secondary .steal lock is gone. A lock is a directory that carries its taker's stamp <lock>/held.<pid>.<epoch>.<random> from the instant it exists (built privately, rename(2)d onto the path), so a held lock is never empty and no rename can displace it; stale recovery unlinks only stamps older than a minute, keyed by name, and never removes the directory. Release removes its own stamp then rmdir.

BASE 6936a7e03: FAIL both directories abandoned, two acquirers, forced delays -> exactly ONE winner — winners: A-WON B-WON
HEAD a13880169: ok   both directories abandoned, two acquirers, forced delays -> exactly ONE winner
                ok     CONTROL: the pre-fix two-lock steal lets BOTH win under the same forced delays
stress at HEAD: 300 trials x 4 acquirers -> >1 winner = 0; 100 x 8 -> 0

The deterministic forced-schedule control replaces the stochastic vacuity control that could false-red.

[P1] startup diagnostics can block intake — src/diagnostic_append.py (new): O_WRONLY|O_APPEND|O_CREAT|O_NONBLOCK, fstat must be a regular file, always exits 0; src/git_binary.py gains short_head through git_argv() ("unknown" when git is unavailable or the path is not a repo); src/watch-tasks-stream.sh uses both instead of the bare git and >>.

BASE: FAIL FIFO without a reader: the watcher still publishes its record (bounded 15s) — no record — the start log blocked startup
HEAD: ok   CONTROL: a plain >> onto the reader-less FIFO blocks
      ok   FIFO without a reader: the watcher still publishes its record (bounded 15s)
      ok     ...and still emits a task

[P1] reaper signals unproven targets and reports failed signals as success — src/watcher_identity.sh (new): one sequence for restart and reaper — watcher_confirm_owner <sentinel> <instance> <workspace> <code_path> (record incl. THIS checkout's script → alive → argv → executed slot → age) and watcher_stop_owned (TERM, wait, release only on confirmed exit; sentinel retained on rc 1 signal-failed / rc 2 still-alive). watcher_identity.py's owner-pid takes --code-path and refuses a record naming another checkout's script even when argv agrees. restart.sh drops its private _confirm_watcher_owner; startup-runtime.sh prints "reaped" only on rc 0.

BASE tests/startup-watcher-reaper-ownership.test.sh: FAILED (14), e.g.
  FAIL data-argument argv: the process is NOT signalled — it was killed (✓ reaped stale watch-tasks-stream watcher (pid 81365))
  FAIL foreign checkout: its watcher is NOT signalled — it was killed
  FAIL failed signal: the sentinel is RETAINED — unlinked
HEAD: ALL PASS (30 ok)   also green at HEAD, red at base: watcher-sentinel-ownership (4), restart-stops-only-own-watcher (3), restart-scope-isolation (6)

Verification at HEAD (my own re-run after the agent's): watch-tasks-stream-sentinel-record.test.py 59 ok / 0 fail; watch-tasks-stream-start-log.test.py pass; reaper-ownership 30 ok; restart-stops-only 24 ok; restart-scope-isolation 122 ok; watcher-sentinel-ownership 40 ok; scripts/review-checks.sh --diff PASS; scripts/lint-workspace-resolution.sh --diff clean; 19 files, +1022/−280.

Stated plainly:

• The [P2] (util_paths.py:626 / services_status.py:165) belongs to parent fix(restart): stop only the core's own task watcher, never every watcher on the host #4168 and is untouched here.
• watcher-sentinel-record-readers.test.py (2) and health-check-task-watcher.test.py (3) fail identically at base and head on this host: their fixture pid 4242 is a live crashpad_handler here, so the real ps says "not the watcher". Host artifact; CI will tell.
• Lock protocol change: a lock left by the OLD protocol is an empty directory, which the new acquire takes by rename regardless of age. After a restart only a stale one can exist, but a mixed-version window (old-protocol live holder + new-protocol acquirer on one workspace) would treat the old holder's lock as free.
• Each acquire spawns python for rename(2) (~40 ms), once per watcher start and once per cleanup.
• No live post-restart round trip yet — that witness is owner-gated on this host and stays owed.

@keweichen @john-the-dev — re-requesting on this head.

-- Sutando core (qingyun-001)

[qingyun-wu]

Blocking at a138801: the production changes address the previously reported ownership, signal-failure, stale-lock, FIFO, and bare-git issues, but the newly changed required test suite is not green.

1. tests/watcher-sentinel-record-readers.test.py:430-435 invokes the now-required owner-pid CLI without --code-path. argparse exits 2 during the test, so both hosted Python checks fail before the assertions complete. Pass the expected watcher script to both CLI cases.

2. tests/watch-tasks-stream-start-log.test.py:74-86 has a race that can hang indefinitely. diagnostic_append opens and closes the FIFO while the reader thread is active; the reader can consume EOF and exit before line 84 opens a second writer. That blocking os.open then has no reader. I reproduced the hang locally and interrupted it at line 84 after 90 seconds. Coordinate reader readiness/lifetime explicitly or use O_NONBLOCK for the release write and assert the thread terminates.

Hosted python standalone tests and diff coverage are currently failed. Focused restart interface/scope tests and git diff --check pass; macOS process-table tests are additionally constrained in this sandbox, but these two failures reproduce independently.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Blocking at 9c33f00 only on the previously required live-path evidence.

The two concrete test failures from the prior head are fixed correctly: the owner-pid CLI cases now pass --code-path, and the FIFO-reader fixture holds a nonblocking read descriptor across append_line() with no blocking release open. Both focused suites pass locally, along with the restart process-interface and scope-isolation suites; git diff --check is clean.

The full sentinel-record suite reached all new lock/FIFO assertions successfully here, but its real-process handover arm could not make watcher A exit in this restricted process sandbox. Hosted Python checks are still running. More importantly, the PR body still explicitly says the real post-restart task/reply witness is absent. That live restart path was an existing merge gate and this test-only head does not supply it. Please provide the post-restart watcher ownership/stop/start plus task-and-reply witness at this exact head; approval can follow once that evidence and hosted CI are green.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Additional blocking gate result at 9c33f00: hosted diff coverage failed at 74% (31 changed production lines, 8 uncovered). The uncovered branch is src/diagnostic_append.py:25-26 and 30-35, covering open failure plus non-regular/write-error handling. Python standalone tests are green, so this is specifically the required diff-coverage gate. Please route the focused helper cases through the repository's measured coverage path or otherwise add counted tests for those branches; the gate must be green in addition to the live witness noted above.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Rechecked 09c42b9. The added append-helper tests cover missing-parent open failure, a successful in-process write, and an injected os.write failure; the focused start-diagnostics suite passes locally, diff coverage is now above the gate, and every hosted check is green. That resolves the earlier coverage concern.\n\nApproval remains withheld only for the already-requested final-head receiver-side live witness: a fresh-process send/read-back and the addressed Stand's actual receipt or response. No duplicate code finding is being added.\n\nReviewed by Qingyun's Personal Codex.

[keweichen]

REQUEST_CHANGES at 09c42b988c9e6f19412d701d98e033cf5eb71217 — the cumulative production path still refuses a watcher started by the Codex notifier, the parent stack retains its malformed-record failure, and the required live restart witness remains absent.

[P1] The shell bridge discards argv boundaries, so it cannot confirm the Codex notifier's watcher

src/agent/codex/cli/task-notifier.sh:379-381 starts the watcher with three argv entries: /bin/bash, the watcher script, and $TASKS_DIR. src/watcher_identity.sh:57-60 obtains only the flattened ps string and passes that to watcher_identity.py runs-watcher. At src/watcher_identity.py:85-88, a flattened command containing any operand after the script deliberately returns “unprovable.”

I drove the production record writer and watcher_confirm_owner, injecting only the process observations:

WITH_TASKS_ARG rc=1 reason=argv: cannot prove pid 4242 executes watch-tasks-stream.sh from "/bin/bash /x/src/watch-tasks-stream.sh /x/ws/tasks" — an unprovable identity is a refusal
NO_ARG_CONTROL rc=0 pid=4242

The pure-policy test at tests/watcher-sentinel-record-readers.test.py:367-378 shows that an authoritative vector with operands works, but the shell adapter never supplies that vector. Its CLI integration test at lines 438-440 and the restart fixture at tests/restart-scope-isolation.test.sh:84-86 omit the tasks operand, so they miss the production invocation.

Worst case, restart leaves its own old watcher running. src/restart.sh:216-219 then directs the operator to re-arm a watcher, which can create two drains and duplicate task processing; declining to re-arm instead leaves pre-restart code consuming tasks. Preserve authoritative argv boundaries from the PID through the adapter—do not split the flattened ps text—and add restart and reaper controls for the exact notifier form /bin/bash <script> <tasks-dir>.

[P2, parent stack] Malformed records still escape the status contract

I independently reproduced the existing cumulative condition at src/util_paths.py:623-626 and src/services_status.py:165:

read_sentinel_record("broken\npid=12345\n") -> {'pid_line': 'broken', 'pid': '12345'}
probe_pidfile(..., _real_pid_alive)          -> TypeError
probe_pidfile(file containing byte 0xff, …) -> UnicodeDecodeError

Metadata can still manufacture the parser-owned pid, and the preliminary strict read still bypasses the tolerant shared reader. One corrupt sentinel can abort service-status refresh instead of returning unknown. This belongs in parent #4168: reserve parser-owned keys, route decoding through the shared reader, then rebase this child.

Live-path gate

The PR body still explicitly says there is no real post-restart round trip. The latest commits add tests but no owner-run witness. For this startup/restart path, approval still requires a replacement-head witness showing ownership confirmation, stop/start, and a real queued task reaching its reply through the restarted watcher. The witness should exercise the Codex notifier invocation above, not only a no-operand Monitor launch.

Verification

The stack parent 72e63cf2798a9155728222d5199dd0679d85049c is an ancestor; the merge-base with origin/main is c7cc92168512691867d1d6a80dab6ef5425baa6e. The need is real on the checkout's origin/main: restart.sh still uses broad pkill -f "watch-tasks", while the watcher still writes a bare PID.

Passed locally: both child and cumulative git diff --check; both scripts/review-checks.sh scans; start-diagnostics, sentinel reader-contract, restart process-interface, and restart scope-isolation suites; and changed-shell bash -n. No added host-specific path was found, and a synthetic /Users/reviewer/private control made the review scan fail with rc 1.

I did not run tests/watch-tasks-stream-sentinel-record.test.py because it starts real task watchers, which this isolated review explicitly prohibited. The local interpreter lacks the coverage module, so I am not claiming an independently measured diff-coverage result or hosted-CI state.

Fix the argv-vector bridge and its production-form regression, correct the parent malformed-record contract and rebase, then provide the live task/reply witness to unblock approval.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

I independently confirmed Kewei's two current-head code blockers at 09c42b9.\n\n1. src/watcher_identity.sh:57-59 passes a flattened ps string into runs-watcher, while the production Codex notifier starts /bin/bash . watcher_identity.py deliberately returns unprovable for that flattened three-token shape, so restart cannot confirm its own notifier-started watcher. The existing CLI fixture omits the tasks operand.\n2. src/util_paths.py:623-626 lets a later pid= metadata line populate the parser-owned pid after an invalid first line, and services_status.py:165 performs a strict read before the tolerant parser. A malformed record can therefore produce a string pid/TypeError, while invalid UTF-8 can still escape as UnicodeDecodeError instead of an unknown status.\n\nThe final-head restart/task/reply witness remains required after these code fixes. The focused coverage repair is green, but approval remains blocked on all three items.\n\nReviewed by Qingyun's Personal Codex.

[qingyun-wu]

Pushed 7f790ab1e — @keweichen's [P1] from 11:17Z fixed, and the child rebased onto the parent's new head 79458f5ec (#4168, which carries the [P2]); the rebase replayed all six child commits with no conflicts and the child-only diff is byte-identical to the pre-rebase one.

[P1] argv boundaries from the pid through the adapter — the flattened ps text is never split:

• src/proc_argv.py (new, the one owner): Linux /proc/<pid>/cmdline; macOS KERN_PROCARGS2 via sysctl (argc + exec path + argv). argv_vector(pid) -> list | None; CLI prints a JSON list, rc 1 + nothing when unreadable. src/health-check.py's private copy of that sysctl code (on main since refactor(watcher): one anchored, tri-state watcher-identity policy in core #4230) now delegates to it.
• src/process-ops.sh pops_argv_vector <pid>; src/watcher_identity.sh asks for the vector first and passes --argv-vector <json>; the flat --argv is still passed for the reason text and as the fallback, where an operand-bearing flat string with no vector stays a refusal. watcher_identity.py parse_vector refuses malformed JSON / non-list / non-string elements — no silent fallback from a vector the adapter could not hand over intact.

Your control, real processes, production writer + watcher_confirm_owner, real seam:

at 09c42b988:  WITH_TASKS_ARG rc=1 reason=argv: cannot prove pid 88548 executes watch-tasks-stream.sh from "/bin/bash .../co/src" — an unprovable identity is a refusal
               NO_ARG_CONTROL rc=0 pid=88549
at 7f790ab1e:  WITH_TASKS_ARG rc=0 pid=49508        (argv: /bin/bash .../co/src/watch-tasks-stream.sh .../ws/tasks)
               NO_ARG_CONTROL rc=0 pid=49509

Controls added for the exact production forms: reaper cases 10–12 (a real notifier-form sleeper /bin/bash <script> <tasks-dir> confirmed and reaped with the REAL pops_argv_vector output compared to json.dumps(argv); Monitor form bash <script> <tasks-dir> reaped; dead pid → rc 1, nothing printed); restart-stops-only-own-watcher spawns its own watcher in the notifier form with a vacuity check that the ps text carries the operand; restart-scope-isolation (f): notifier form confirmed+signalled, Monitor form confirmed, operand+no-vector refused "unprovable identity", no-operand+no-vector control confirms, the same flat text with a one-spaced-path vector refused, malformed vector refused; readers test case H runs proc_argv against a live Popen(["/bin/bash", script, tasks]).

Verified at 7f790ab1e (my own re-run after the agent's): reaper-ownership 38 ok · restart-stops-only 28 ok · scope-isolation 148 ok · process-ops-interface 37 ok · readers contract all pass · services-status 40 · start-log pass · sentinel-record 59/59 · src-map 27 · review-checks.sh --diff PASS (positive control rc 1) · lint-workspace-resolution.sh --diff clean.

Stated plainly: the live restart witness exercising the notifier form is still owner-run and not supplied; health-check-pool-watchers-all-tracked.test.py has 2 cases that fail identically at base on this host (fixture pid 4242 is a live process here) and were left alone; the readers-contract driver now stubs the vector reader for its fabricated pid (same host condition) — say so if you would rather that be split out.

-- Sutando core (qingyun-001)

[qingyun-wu]

Rechecked current head 7f790ab1e08e49f0ba8594c2e0dd407b5b6edf09 in an isolated checkout.

The two current-head code blockers are fixed:

• the restart/reaper path now obtains an authoritative kernel argv vector and correctly confirms the production /bin/bash <watcher> <tasks-dir> launch without splitting flattened ps text;
• sentinel parsing reserves parser-owned PID fields, bounds PID values, tolerates invalid bytes, and status probing no longer performs a strict decode first.

Focused verification passed locally: the full sentinel reader contract (including a live notifier-form process), 40 service-status cases, 28 restart ownership checks, and 38 startup reaper checks. I found no new blocking code issue in this update.

Approval remains withheld only for the explicitly outstanding owner-run final-head live witness: restart the real service, confirm ownership/stop/start on this head, enqueue a real task, and show its reply reaching the addressed Stand through the restarted watcher.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

One additional blocker surfaced in the hosted coverage run at current head 7f790ab1e08e49f0ba8594c2e0dd407b5b6edf09.

tests/watcher-sentinel-record-readers.test.py:568-570 assumes the kernel argv after the interpreter is exactly sys.argv. Under the repository's coverage runner, the kernel correctly reports python3 -m coverage run --rcfile=.coveragerc tests/watcher-sentinel-record-readers.test.py, while sys.argv contains only the test path, so the new test fails and the diff-coverage job exits 1. Please assert an invariant that survives wrappers (for example, that the live vector is nonempty and ends with the current sys.argv, with an explicit direct-process control), then restore the required hosted check to green.

The final-head owner-run restart/task/reply witness remains separately required.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

The hosted argv assertion is fixed on 1aaab0f1144ded0c0bbbd69a5ee6dd28b5e79d5b: the focused sentinel suite now passes both normally and under coverage locally. The full hosted coverage gate still fails, however, with src/proc_argv.py at 81% diff coverage (missing lines 76-77 and 85-93; 89% total versus the required 95%). Please add coverage for those error branches and rerun the gate. The requested live owner-run restart/task/reply witness also remains outstanding.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check