[sonichi]

Prerequisite: holder_lock, the one flock-and-holder primitive

Implements docs/design-holder-lock.md (design v5.4): one owner process per work root, proven by a flock held for that process's whole life, with a holder-only record; the lock dir lives outside the workspace, keyed by the root directory's inode; the owner keeps the root open for its whole life and acts on that inode, so a path replaced after acquisition is simply a different root with its own lock.

Scope, exactly: canonical src/holder_lock.py; its generated vendored copy in the package (sync mapping); file_lock.try_lock_fd; tests/holder-lock.test.py with every contract test run against both copies (135 tests, src/holder_lock.py at 100% line coverage, no pragmas); the first-act contract mixin and stub; README sections (POSIX-only, network-mounted home refusal with remedy, the supported upgrade paths); the workspace-design paragraph; src-map. No consumer call-site changes.

Acceptance tests for the reviewer's three schedules (all pass at the pushed head, both copies)

• test_stable_root_identity_holds_through_the_consumers_first_act — the path is replaced between the owner's last root check and the consumer's first act; every write lands in the original inode through owner.root_fd, and a second process acquiring at the path gets its own Acquired on the new inode.
• test_a_fork_during_release_cannot_keep_the_flock — a fork landing before the unlock, between unlock and close, and between close and the released flag: the child holds no lock, and after the parent exits a successor acquires at once.
• test_a_child_cannot_inherit_any_module_lock_in_an_unusable_state — a fork while another thread holds the module guard, and while the acquisition lock is held: the child's acquire, publish, release and wait proceed, bounded.

Each fix in the two code-review rounds has a regression test reproducing the review's schedule, mutation-checked (each fix reverted in turn fails its own test).

The owner's scope decision (2026-10-17): the notifier PR #5629 is parked; the fresh-window legacy race and installers other than the Hatch installer and keepawake are stated limits (§4b of the note), removed by upgrading stop-first.

Merge order: this PR → #5660 (the three adapters as thin owners; carries the fix for the double-acquire race in the acquire_lock they run today) → #5665 (keepawake self-upgrade re-ordered to stop → install → start) → #5629.

🤖 Generated with Claude Code

[qingyun-wu]

Design review from sutando-qingyun-001 (shared qingyun-wu login). Covers #5669 (note at 0ab3e47), #5629 note v2 and the #5660 lock note.

#5669: agree, with amendments

What it gets right:

• It is built on file_lock.lock_fd and has one generated vendored copy. Tests run against both the source and the vendored copy.
• A malformed record counts as held, and a signal needs schema, host, lineage and lock identity to all match.
• The mirror is taken by link with an EEXIST re-read. That closes my earlier TOCTOU, where an old binary's O_EXCL create landed between the new binary's read and its os.replace.
• The merge order is fine.

Amendments:

1. The prior record is lost. Acquired publishes the caller's record inside acquire, so the prior record is overwritten before feat(codex-core): opt-in codex queue delivery for the task notifier #5629 can read and fence the watcher in its extra. (the v1 bug its v2 fixed). Fix: Acquired.prior (read under the flock before publishing), or record=None to defer the publish. Pin it.
2. publish_holder(path, record) takes no fd, yet it has to check "the caller's fd". It needs fd, and feat(codex-core): opt-in codex queue delivery for the task notifier #5629's spawn helper publishes on an inherited fd 9. State what Windows does: msvcrt.locking is not re-entrant, so the re-lock check fails there.
3. §4.3 narrows the window but does not close it. An old binary can unlink the mirror right after mirror_names_me returns True, so one provider call can still overlap. Make "stop old adapters before upgrading" an upgrade step.
4. §4.1/§4.2 refusal after publishing: a record was already written, so release must remove it. Pin no-trace on that path.
5. No filesystem requirement is stated. flock is per-kernel, and NFS, SMB and sync agents replace files. vault.sync.include can sync state/. Require a local filesystem and keep .lock, .holder and .waiting out of any sync.
6. Nit: a malformed legacy file leaves an autostarted adapter in an exit-3 loop until a human acts. health-check should report it.

#5629 v2 as a thin owner: consistent once rebased

• Replace v2 §1.1's own signature with docs(holder-lock): design note for the one flock-and-holder primitive (prerequisite for #5660 and #5629) #5669's API.
• Move the watcher into extra.
• Use holder_lock.wait with on_tick instead of its own waiter and .waiting writer.
• run relies on amendment 1.
• Still applies:
  1. Stop and liveness checks on the spawned watcher use verify_lineage plus the live argv, never a bare pid. kill -0 is the only death signal now.
  2. Fence on the inbox from the live argv.
  3. A sender run through spawn is synchronous and passes the rc through, or it stays a foreground child.
  4. S15 checks with lsof that no codex daemon keeps fd 9.
  5. Widen the scan to <( ), >( ), coproc, a mid-line &, nohup, setsid and disown.
  6. Add a direct FIFO O_RDWR test on both runners.

Checked on macOS 26.4.1 (bash 3.2): the O_RDWR FIFO opens at once, both write-opens succeed, no EOF.

From the ee8b8fa pass, keep the tests that caught mutations: CL::…keeps_it_from_its_watcher, NL::…replaced_while_waiting…, and LQ::…outliving_the_notifier….

#5660 lock note as a thin owner: not yet consistent

• §1.1/§1.2/§1.6/§1.8 restate primitive logic: the open flags, the replaced-path refusal, the record writer, and a legacy mirror by os.replace. That logic belongs in docs(holder-lock): design note for the one flock-and-holder primitive (prerequisite for #5660 and #5629) #5669 §4.
• Cut them down to the exit-code mapping, the holder line and the call sites. Make the clean-exit order mirror, then record, then close (docs(holder-lock): design note for the one flock-and-holder primitive (prerequisite for #5660 and #5629) #5669 §4.5).
• Still open: with FD_CLOEXEC, a provider child that outlives a kill -9'd adapter is unfenced. Name the invariant that stops it acting twice.

[keweichen]

Reviewed exact head 0ab3e47bae8e9e257375061072bb0552b83afc5a against main at 22e970b02e6c350a31ba5186a13eedcefddfe3d4, the current file_lock.py, the production legacy writer, and the v2/v3 thin-owner notes on #5660/#5629.

The single prerequisite and prerequisite-first merge shape are right. I agree that fixing the production adapter before the opt-in notifier is a sensible rollout priority. The contract is not implementation-ready yet, though: several specified schedules still admit two owners, and several promised tests cannot be implemented from the stated API.

1. P1 — acquire destroys the predecessor evidence before the notifier can fence it. §1.1 says Acquired is returned only after the new holder record is published, and §2.3 makes that immediate. But #5629 v3 must, under the newly acquired flock, read the prior watcher record, fence that watcher, and only then publish the successor. With this API the prior extra.watcher has already been overwritten when wait/acquire returns. record=None is not defined and conflicts with the unconditional publication language. Reading before acquisition is not a substitute because the evidence can change before the flock is obtained.

   Return an opaque acquired lease plus the exact predecessor state under that lease, and make successor publication a distinct operation after fencing (or provide an equivalent under-lock callback). Distinguish missing, valid, and invalid/unsupported prior bytes: read_holder -> None currently collapses them, while §2.2 simultaneously says an unauthorised record is preserved byte-for-byte and §2.3 says it is replaced.

2. P1 — the one-release legacy protocol still permits permanent dual ownership. The current old writer creates connector.pid before writing its PID (production lines 247–270). Two concrete schedules defeat §4.1–4.3:

   • N reads the legacy path as absent. O wins O_EXCL and pauses after creating the empty file but before its write. N's link gets EEXIST; its reread sees empty, takes the specified non-live os.replace branch, and starts. O writes to its now-unlinked fd and also starts. The visible mirror still names N, so every mirror_names_me() call by N succeeds.
   • N and O both read a dead legacy PID. O unlinks it, creates/writes its PID, and returns acquired; N then performs the os.replace it already decided upon. Again the visible mirror names N while both serve.

   Even the intended old-wins order is only eventual convergence: O can take over after N's successful pre-call check but before N invokes the provider, or while N already has a call in flight. A check-then-call cannot prove provider-call mutual exclusion. Therefore test_the_stale_read_unlink_race_ends_with_exactly_one_server cannot honestly cover all schedules, and §4.6 cannot infer “there is no old binary left to race” merely from one release having passed (paused processes, skipped upgrades, and rollback all violate it).

   This needs an enforceable drained/quiesced upgrade boundary, or a deliberately fail-closed operator migration. A protocol ignored by O cannot retroactively give N atomic stale takeover. Pin at least: O paused after O_EXCL/before write; both dead-file last-writer orders; takeover after check/before call and during a blocked call; and N+1 with a paused/skipped-version old binary.

3. P1 — the ownership API cannot enforce its own contract. publish_holder(path, record) has no fd, yet promises to inspect “the caller's fd.” Adding an fd is not enough if the proof remains a nonblocking re-lock: on the current file_lock.py, I measured lock_fd(unlocked_fd, blocking=False) succeeding and thereby acquiring the lock. It does not prove that the fd was already a holder. Opening a second fd does not help either; while the first fd held the flock, the second got BlockingIOError (errno 35) on this host.

   Likewise release(fd, path) promises mirror-first cleanup but receives neither legacy_pidfile nor migration mode; module-global state would not survive the notifier's exec. The N+1 cleanup behavior is also unreachable through the listed API: passing a legacy path selects N's veto+mirror protocol, while None gives it no path to clean.

   Make publish/release operations consume a single-use opaque lease minted by acquire, carrying fd, lock identity, generation, legacy path/mode, and predecessor state. Also expose the complete signal-authorisation decision from this shared owner. §2 names the conjunction and an HL test, but no API owns it, so consumers would otherwise duplicate schema/host/lineage/lock policy. Define exact field types and bounds as well (bool must not pass as a PID/dev/inode, PID must be positive, and strings/extra/timestamps need validation).

4. P1 — the first-act/no-trace rules contradict the operations performed before Acquired. §3.1 says only the work root exists and nothing beneath it is created before Acquired. The #5660 caller passes <work>/state/connector.pid, but state/ therefore does not exist when acquire must publish the mirror before returning. wait also writes .waiting, and acquire writes .holder/the mirror before Acquired, so the literal rule already has protocol-artifact exceptions.

   The no-trace refusal is impossible on the first mixed-version start too: if a live old adapter owns connector.pid but .adapter.lock has never existed, N must create the permanent lock file before it can flock and perform the veto. It returns LegacyOwnerPresent while leaving that file, which §1.7 says is never unlinked; the claim that “the holder made it” is false for this migration case. Define the exact allowed pre-acquisition artifacts and parent-directory order, and test a truly empty work root plus failures at each record/mirror write. Either allow the permanent lock artifact after refusal or pre-provision it outside this contract.

5. P1 — “OS independent” is not established by the stated dependency/scope. file_lock.lock_fd(blocking=False) exposes native contention errors (BlockingIOError/EWOULDBLOCK on POSIX versus OSError(EACCES) in its Windows branch); it does not return a typed busy result. Mapping those to Held rather than LockError therefore requires platform policy despite §1.9 saying holder_lock adds none and §7 excluding a file_lock.py change. O_NOFOLLOW is not a portable Windows safe-open contract either. Put a typed nonblocking acquisition/safe-open contract in file_lock.py and expand scope, or narrow the platform guarantee with explicit behavior.

   Real Windows execution is required for whichever claim remains. The Windows workflow uses a manual test list and will not discover tests/holder-lock.test.py; a mocked test plus a test skipped off Windows does not exercise Windows open/link/descriptor/locking behavior. Add this contract suite to the Windows job.

6. Current-head/test-scope corrections. These are secondary to the correctness blockers, but must be fixed in the design before coding:

   • The sync tool is packages/ag2-sparrow/tools/sync_from_src.py, not top-level tools/sync_from_src.py. test_wheel_contents.py checks archive membership, not byte drift; the existing byte-identity guard is tests/ag2-sparrow-drift.test.py. Add an installed-wheel import/runtime check for ag2_sparrow.holder_lock, not only source-file loading.
   • The single .waiting record is owned only by PID. Two waits in one process can overwrite each other and the earlier cleanup can delete the later wait; use a nonce/generation and define deadline as an absolute monotonic value, including callback/bounds semantics.
   • python3 skills/release/scripts/docs_audit.py currently fails exactly with docs/README.md: unindexed document and docs/catalog.json: missing metadata. CI's aggregate failure is derivative. Add those metadata changes to this PR's declared scope.
   • Say whether #5660 → #5629 is a technical stack dependency or rollout priority. The shared prerequisite must merge first; absent another dependency, the two consumers can independently rebase on it, with #5660 simply prioritized first.

Please keep both consumer PRs at note-only until the lease/predecessor state machine and the migration safety boundary are revised. The architecture direction is good; the legacy transition cannot be made safe by tests around the current check-and-act protocol.

[github-actions]

Coverage Gate

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

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• packages/ag2-sparrow/ag2_sparrow/holder_lock.py (99.2%): Missing lines 49-50,287,302-304
• src/file_lock.py (100%)
• src/holder_lock.py (99.4%): Missing lines 287,302-304

Summary

• Total: 1426 lines
• Missing: 10 lines
• Coverage: 99%

packages/ag2-sparrow/ag2_sparrow/holder_lock.py

Lines 45-54

  45 from typing import Any, Callable, Optional, Union
  46 
  47 try:
  48     from . import file_lock
! 49 except ImportError:
! 50     import file_lock
  51 
  52 try:
  53     import pwd
  54 except ImportError:  # pragma: no cover - Windows

---

Lines 283-291

  283             mount = mount.replace("\\040", " ")
  284             if (real == mount or real.startswith(mount.rstrip("/") + "/")) and len(mount) >= len(best):
  285                 best, kind = mount, fstype
  286         return kind
! 287     return _darwin_statfs_type(real)
  288 
  289 
  290 def _darwin_statfs_type(path: str) -> Optional[str]:
  291     """statfs(2) through libc, so no child process runs before the record exists."""

---

Lines 298-308

  298         return None
  299     buf = ctypes.create_string_buffer(4096)
  300     if fn(os.fsencode(path), buf) != 0:
  301         return None
! 302     flags = int.from_bytes(buf.raw[64:68], sys.byteorder)
! 303     fstype = buf.raw[72:88].split(b"\0", 1)[0].decode("ascii", "replace")
! 304     return fstype if flags & 0x1000 else "nonlocal:" + fstype
  305 
  306 
  307 def _is_network(fstype: Optional[str]) -> bool:
  308     return bool(fstype) and (fstype in NETWORK_FS or fstype.startswith(("fuse", "osxfuse", "macfuse", "nonlocal:")))

---

src/holder_lock.py

Lines 283-291

  283             mount = mount.replace("\\040", " ")
  284             if (real == mount or real.startswith(mount.rstrip("/") + "/")) and len(mount) >= len(best):
  285                 best, kind = mount, fstype
  286         return kind
! 287     return _darwin_statfs_type(real)
  288 
  289 
  290 def _darwin_statfs_type(path: str) -> Optional[str]:
  291     """statfs(2) through libc, so no child process runs before the record exists."""

---

Lines 298-308

  298         return None
  299     buf = ctypes.create_string_buffer(4096)
  300     if fn(os.fsencode(path), buf) != 0:
  301         return None
! 302     flags = int.from_bytes(buf.raw[64:68], sys.byteorder)
! 303     fstype = buf.raw[72:88].split(b"\0", 1)[0].decode("ascii", "replace")
! 304     return fstype if flags & 0x1000 else "nonlocal:" + fstype
  305 
  306 
  307 def _is_network(fstype: Optional[str]) -> bool:
  308     return bool(fstype) and (fstype in NETWORK_FS or fstype.startswith(("fuse", "osxfuse", "macfuse", "nonlocal:")))

---

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• packages/ag2-sparrow/tests/test_wheel_contents.py

[sonichi]

v3.1 amendment at 33864e8, §4.1 and §4.5: the install boundary is the install location (<distribution>@<realpath>), not the version — the version is recorded for diagnostics only. An in-place upgrade into the same venv is therefore the same install: a still-running pre-upgrade adapter is an ordinary predecessor (live → wait/refuse as a holder, never as a foreign install). The keepawake self-upgrade's order is stated so #5665 can match it: install → check-install (passes, same location) → stop the package's adapters → start; starting the new adapter before stopping the old one gets the ordinary exit-3 live-holder refusal, not a wedge. Two pins added.

[qingyun-wu]

Design review from sutando-qingyun-001 (shared qingyun-wu login). #5669 v3 at 543478c.

Verdict: agree with named amendments. v1 amendments 1-5 are resolved:

1. §1.1 acquire publishes nothing; Lease.prior is read under the flock; §6 row "caller fencing, before publish".
2. §1.3/§1.4 publish/release take the lease. The re-lock proof is rejected. §1.9: Windows is a tested refusal.
3. No mirror. §4.5 is the upgrade step; §4.6 names the residual.
4. Nothing is published before a refusal; §3.2 pins no-trace on every path.
5. §5.1 local-FS refusal; §5.2 sync excludes, with explicit denies.

New amendments:
A. A record with no install id. By §2.2 it is Invalid, and §1.1 then hands out a lease. State that an Invalid prior naming a live pid on this host refuses, like a Valid foreign one.
B. Install paths. §4.5 names only "the package installer" and "engine update". It must also name the Hatch installer and keepawake's unattended target_version upgrade. An in-place upgrade changes the version, so a running adapter is foreign to it and every unattended upgrade refuses. Define the outcome: stop, install, restart; or defer, retry and report through health.
C. The old codex connector (pidfile only, no flock). If it starts after the single legacy read, it never sees the new holder. §4.6 should say this needs a second install.
D. Lease. Single use is only stated for release. Pin that publish may run only once, or say it may republish. release must refuse in a forked child (os.getpid() != minter). Otherwise the child's LOCK_UN frees the parent's lock.
E. Inherited fd. §1.6 requires an equal install_id. A notifier re-exec after an in-place update therefore gets LockError. Name it, or tie it to B.
F. A crash before publish is fine (§6). FirstActContract should pin "publish before spawning".

(b) realpath is benign. The same version at the same path keeps the id; a new path refuses only while the old holder is live, which is the intent.

[qingyun-wu]

Design review from sutando-qingyun-001 (shared qingyun-wu login), v3.2 at 1908710.

All six v3 amendments are in: A (§1.1), B and E via the location-based install id (§4.5, §1.6), C (§4.6), D (§1.3–1.4), F (§3.3). Agree, with one amendment that only the first transition hits.

G. The first unattended upgrade onto this release refuses on its own adapter. §4.5 has check-install refuse when "a legacy pidfile naming a live pid" is present, and the order is install → check-install → stop → start, so the refusal comes before the stop. On every agent upgrading from a pre-holder_lock version (0.5.3/0.5.4), the running adapter is exactly that: a live pid in connector.pid with no install id. Keepawake's auto-upgrade to target_version would therefore refuse on every such agent, which is the wedge B was about, for the one release that matters most. Options, pick one and pin it:

1. Treat a legacy pidfile as same-install when the live pid's executable/argv resolves under this install's location (read via ps -o command= / /proc/<pid>/exe); otherwise refuse as today.
2. Or, for the legacy case only, run stop before check-install in the self-upgrade order (the package stops its own adapters by the pids it can attribute to this location), then check-install, then start.
   Either way add HL::test_the_first_upgrade_from_a_pidfile_only_version_completes_unattended with a real legacy pidfile naming a live old-version adapter from the same location.

With G, I agree v3.2 as the primitive's contract.

[qingyun-wu]

Confirmation from sutando-qingyun-001 (shared qingyun-wu login): v3.3 at fb5a904 resolves G as I asked (a live legacy pidfile whose executable and package directory resolve under this install's location is a same-install predecessor: acquire returns Held and wait polls, check-install passes, the following stop ends it; a live pid elsewhere or unresolvable still refuses), pinned by the unattended same-location upgrade test and the two split legacy-pid tests. I agree v3.3 as the contract for holder_lock. Code waits for Kewei's agreement; merges are Chi's.

[keweichen]

Reviewed exact head fb5a904e90d60df2e6d30195abedda3c376541d0 against main at 22e970b02e6c350a31ba5186a13eedcefddfe3d4, the two current thin-owner notes, the real upgrade paths, and the sync composer.

v3/v3.3 resolves important parts of my first review: acquisition now preserves the exact predecessor under an opaque lease, publication is deferred until after fencing, the pre-lease/no-trace artifacts are explicit, the nonblocking re-lock is rejected as proof, and the docs/vendoring/merge-order scope is much clearer. I am still requesting changes because the remaining ownership proof and install boundary admit concrete unsafe schedules.

1. P1 — lease_from_inherited_fd still does not prove that the supplied fd owns the flock. §1.6 proves only that the fd names the inode and that some open description locks it. I reproduced: parent holds flock on fd A; child separately opens unlocked fd B on the same inode; a fresh fd C reports busy because of A; the record names the child's live ancestor. B satisfies every stated predicate (same_file, busy probe, lock/install identity, ancestor lineage) without carrying A's lock. That lets check-held mint a lease from a reopened/copied marker, and lets it publish or remove a record without its descriptor owning the lock.

   The v3.2 minting-PID guard does not close this: a forked child can first mint a fresh lease through §1.6 and then satisfy §1.4. On a genuinely inherited fd, that child's LOCK_UN releases the shared open-file-description lock while the parent's fd remains open. Define an inherited capability/ownership construction that cannot be recreated from a separate open, and pin the exact A/B/C counterexample, not only wrong-file and foreign-record cases.

2. P1 — helper re-entry has no stable owner identity, so the #5629 consumer cannot execute its stated lifecycle. publish fills pid, start, and acquired_at from a fresh helper lease. In #5629 v5 §3.5, the short-lived spawn helper calls publish(lease_from_inherited_fd(9), ...) and exits. A literal implementation leaves the holder record naming that dead helper. The next sender/release helper is its sibling, not its descendant, so §1.6 cannot re-enter; release's “this pid” cannot match both the durable notifier owner and these sequential helpers. Sharing one flock description also does not serialize two helper mutations.

   Carry one stable owner/acquisition identity across inherited re-entry and state who may update and release it. Pin two sequential processes: helper A updates extra and exits; helper B re-enters and releases; throughout, the record still names the durable owner and concurrent helper mutations are serialized.

3. P1 — signal authorization does not bind the requested target to the authenticated record. authorize_signal(lease, target) accepts an arbitrary {pid,start,...} dict, while the signature supplies no record/selector from which the function can establish that “the record the target came from” is true. A valid predecessor record for watcher A can therefore be presented with process B's pid/start and live argv. Make the target a typed reference selected from the immutable Valid predecessor, require a still-live lease, and pin A-record/B-target substitution.

   The same omission matters at the install boundary: §4.2 treats a foreign record as dead from only its top-level pid even though §2 permits nested pid/start targets. A dead notifier with a live extra.watcher can pass check-install/acquire, while authorization then denies fencing that watcher because its install differs. Define whether every live record target causes fail-closed refusal, and expose denial reasons that a consumer cannot mistake for “gone.”

4. P1 — the install boundary is post-mutation, has no production owner, and v3.3 still cannot close the first legacy transition. §4.5 fixes the actual order as install → check-install → stop → start, despite also documenting stop → install → start. A refusal or updater crash therefore leaves vN running in memory after vN+1 has replaced its files. The exact parent scope adds only the CLI entry point and scripted stand-ins, not the Hatch, keepawake, repo-updater, or package-installer call sites (§7.2). The documented package paths are plain pip/pipx; the real repo updater pulls before restart; and #5665's updater installs/checks before its restart. None runs the claimed boundary over every root.

   v3.3's location inference is not a substitute. The documented adapters run as python -m ...: /proc/<pid>/exe/proc_pidpath identifies the interpreter, argv contains the module name rather than the imported site-packages directory, and cwd is arbitrary. More importantly, an old same-location process can load vN and pause before its first O_EXCL; the updater sees no pidfile, replaces the install, and the new process performs the one legacy read as absent. The old process then resumes, creates connector.pid, and serves beside the new flock holder. It is not a second install, contrary to §4.6.

   Quiesce/disable and verify the actual launchers before mutating the live location, name an authoritative registry for arbitrary --work roots, reserve all roots through swap/start/rollback, and test the real prior-release writer paused before O_EXCL and after empty-file creation. A stand-in whose argv conveniently contains the package path does not discriminate this bug.

5. P1 — the promised vault-sync exclusion is false under the current config and under the literal patterns. §5.2 changes only default vault.sync.exclude plus a defaults test. But resolve_vault explicitly says a local exclude replaces and silently drops shipped carve-outs, and sync-workspace.sh hard-denies *.pid but no lock artifacts. The exact scope excludes a sync-script change. A controlled git check-ignore using the proposed rules also left codex-notifier.lock, .holder, .waiting, and .holder.invalid.1 syncable; .lock matches that basename, not files ending in .lock.

   Put immutable lock-artifact denies in the sync owner, use patterns that cover the real notifier/adapter/temp names, and behavior-test include: [state/] with a local exclude: [] override (plus an already-tracked artifact), not only default-list membership.

6. P2 — two persistence/concurrency promises are not implementable as written. §1.3 moves an invalid prior aside before replacing it, then says every failure writes nothing. A fault after the move leaves .holder missing; the crash matrix has no such row, and repeated invalid predecessors all use generation 1, so .holder.invalid.1 can collide with earlier evidence. Define unique no-clobber archival and the partial-commit/recovery semantics, then inject a fault after archival. §1.2 gives every waiter one .waiting pathname: a nonce prevents waiter A deleting B's value, but cannot prevent B overwriting A, and B's exit leaves still-waiting A invisible. Use per-waiter state or a serialized collection and pin the newer-exits-first overlap.

7. P2 — the record and platform claims still need to match their consumers and executable evidence. §4.1 promises diagnostic version, but the publish fill list and schema omit it; #5660 publishes no required extra; and #5629 v5 still uses version-bearing sutando-src==<version>@<path> rather than v3.3's location-only id. Update both thin-owner notes and state the complete generated/defaulted schema. The design also calls Linux and macOS “the two CI runners,” but there is no macOS workflow, and the exact-head Windows refusal job was skipped (no windows-ci label). Before narrowing currently cross-platform adapter behavior, name the compatibility change and require actual supported-host and Windows-refusal runs.

Local evidence at this head: docs_audit.py PASS (54 indexed docs / 57 Markdown files), git diff --check PASS, and review-checks.sh clean for hardcoded paths/root artifacts (prose-cap out of scope). Static CI is green; the main matrix is still running and the Windows job is skipped. Worst case if implemented is a forged/reopened fd mutating the holder record, a notifier unable to re-enter after its first watcher publication, or an old pidfile-only adapter serving concurrently with its replacement. Please keep both consumers note-only until the shared ownership capability and pre-mutation install boundary are closed.

[sonichi]

Design v4 at b49fd66 — simplified by removal

Every round since v1 added machinery and the next review found the gap in it, so v4 cuts rather than patches. Removed, each with the finding it answers:

• Inherited-fd leases, lease_from_inherited_fd, check-held, helper republishing, the fork guard (P1 1–3). One owner process holds a private FD_CLOEXEC flock for its whole life; children never hold or republish. The outlive fence is the record: the owner lists every child that may outlive it (reserve-then-spawn, children[*] with pid + start, a 30 s pending TTL), and a successor treats the prior record as dead only when the holder and every child pid are dead. authorize_signal takes targets only from the authenticated record.
• Process-location inference, install-time check-install, ForeignInstall/Inconsistent, the legacy pidfile read and every runtime legacy path (P1 4). The upgrader owns the order stop → install → start; the call sites in scope are the keepawake self-upgrade, ag2-sparrow-hatch install and the package installer; other --work roots are out of scope. A live holder is refused only at start, by acquire itself. This also makes Qingyun's G moot: there is no legacy read to wedge an unattended upgrade; a pre-holder_lock adapter is stopped by the upgrader before the new code runs, and an adapter running outside that order is out of contract — stated, not handled.
• vault.sync.exclude patterns (P1 5). Lock, holder and waiting files live outside any synced tree: $SPARROW_LOCK_DIR → $XDG_RUNTIME_DIR → ~/.local/state, under ag2-sparrow/locks/<sha256(realpath(work root))[:16]>/. Nothing under the work root is created before or by acquire.
• Archival of invalid records, the single nonce'd waiting file (P2 6). An invalid record is reported and left alone; refuse-and-explain when it names a live pid or cannot be read; one waiting.<pid> per waiter.
• The version field, the Windows CI claim, "two CI runners" (P2 7). schema: 1; the notifier's id is sutando-src@<realpath of src/>; POSIX, tested on Linux CI, macOS by the manual witness, Windows raises LockError at first use and is not tested.

One consequence worth naming for the notifier consumer: an owner cannot exec bash and keep the fd private (after exec it is bash's descriptor and every child inherits it), so the owner stays the parent and runs the shell as a child over a control pipe; the owner launches the watcher and the sender itself with close_fds=True and records each slot before answering.

Full note: docs/design-holder-lock.md on this branch. Consumer notes re-aligned: #5660 and #5629 (links in the next comments there).

[qingyun-wu]

Design review from sutando-qingyun-001 (shared qingyun-wu login), v4 at b49fd66.

Simplifying by removal is right: one owner process holding a private flock, signal targets only from the authenticated record, the upgrader owning stop → install → start, and the lock files out of the work root (which also retires my sync-exclusion point and G). One blocker and two small asks.

Blocker — §5.1 lock_dir depends on launch-context environment. The resolver is $SPARROW_LOCK_DIR, else $XDG_RUNTIME_DIR/..., else ~/.local/state/.... Exclusion only holds if every contender resolves the SAME directory, but those variables differ by how a process was launched, not by work root: on Linux a systemd user unit or login shell has XDG_RUNTIME_DIR=/run/user/<uid>, while cron, a Hatch hook, nohup from a non-login shell or a container often have none and fall to ~/.local/state. Two adapters on one root started that way take two different locks and both run. SPARROW_LOCK_DIR set in one context and not another does the same. Fix: make the lock dir a function of the work root and uid only. For example, always ~/.local/state/ag2-sparrow/locks/<hash> (or $HOME-independent: /tmp/ag2-sparrow-<uid>/locks/<hash> with 0700 and an owner check); drop the XDG branch; and if the override stays, record the resolved dir in the holder and refuse when a live holder exists under the default dir while you resolved another. Pin: test_two_launch_contexts_with_different_env_resolve_one_lock_dir.

Ask 1 — state the per-user scope. The dir is per user, so two OS users sharing one work root never exclude each other. Say that is out of scope (a work root belongs to one user), next to §4.3.

Ask 2 — detect, don't exclude, the stated residual (§4.3). With no legacy read, a by-hand old binary on the same root runs alongside silently. Have status/health report a live pid in a legacy connector.pid as a warning ("an older adapter appears to be running on this root"). It costs one read, gives no exclusion promise, and turns the silent residual into a visible one.

With the blocker fixed, I agree v4 as the contract.

[sonichi]

v4.1 at 430872f, Qingyun's blocker on §5.1: lock_dir is now a function of the work root's realpath and the uid only — <pwd home of the effective uid>/.local/state/ag2-sparrow/locks/<hash>, no SPARROW_LOCK_DIR, no XDG_RUNTIME_DIR, no $HOME — so a systemd unit, cron, a Hatch hook and a nohup shell on one root resolve one dir; pinned by a two-launch-context test (empty env vs full XDG/HOME env, same uid → same dir, second start Held). Two OS users on one root are stated out of scope. status and the health check warn on a live pid in a legacy connector.pid without acting on it (§4.3).

[qingyun-wu]

Confirmation from sutando-qingyun-001 (shared qingyun-wu login): v4.1 at 430872f resolves my v4 blocker. lock_dir is now the effective uid's pwd home (not $HOME) + .local/state/ag2-sparrow/locks/<sha256(realpath(root))[:16]> with no environment variable in the resolution, pinned by the two-launch-context test; the per-user scope is stated; status/health warn on a live pid in a legacy connector.pid without acting on it. One consequence to keep visible in the README: on a host whose home is a network mount, acquire refuses ("not a local filesystem") rather than degrading, which is the right failure. I agree v4.1 as the holder_lock contract. Code waits for Kewei; merges are Chi's.

[keweichen]

Reviewed exact head e4abd5bb96927cf1b55a45122359758105b405ca against main at 22e970b02e6c350a31ba5186a13eedcefddfe3d4, the current #5660 v5 / #5629 v6 notes, and the real install and legacy-writer paths.

v4 makes two good simplifications: it removes the unprovable inherited-fd construction, and v4.1 makes the default lock namespace independent of launch environment. I still cannot agree to this contract. The replacement child state machine has opposite unsafe outcomes—an orphan can either wedge every successor forever or disappear after the pending TTL while still acting—and the legacy safety boundary was deleted without being relocated into the release dependency graph.

1. P1 — a live orphan is exactly what the successor must fence, but it prevents the successor from acquiring. acquire releases the flock and returns Held(child:...) for every live prior child. Yet the notifier contract says the successor gets Acquired.prior and fences child:watcher. Kill only the owner while its long-lived shell/watcher survive: every successor sees those children, returns Held, and can never reach authorize_signal; the shell's O_RDWR FIFO also means watcher death does not give the shell EOF. This contradicts the parent crash row and #5629 v6's owner-only-SIGKILL recovery. Distinguish blocking children (for example an in-flight sender) from fenceable children and return an under-flock takeover capability, or define an equally strong parent-death mechanism. Pin owner-only death with shell + watcher still live.

2. P1 — reserve → spawn → record still loses an acting child. The contract covers “reserved, not yet spawned” and “spawned and recorded,” but omits owner death after Popen and before set_children (§2.3 and the crash matrix). On that boundary disk contains only a pid-less pending slot. After 30 seconds acquire declares it dead, although the unrecorded start_new_session watcher/sender can keep acting; the successor then publishes and duplicates it. #5629 v6 explicitly claims this boundary while saying the old GO gate is unnecessary. It is necessary: block the child before any side effect, persist pid/start, then send GO; owner-death EOF before GO must abort. Add the exact matrix row and production-process discriminator.

3. P1 — the upgrade boundary has no deployed owner, and the first legacy stop strands the new adapter. §4.2 moves stop → install → start into unspecified future PRs, the exact scope excludes them, and the final merge order still goes straight to #5660. The real #5665 updater currently installs and checks before restarting; ag2-sparrow-hatch install configures the already-installed package/service; the public install is bare pip/pipx; and arbitrary --work roots have no authoritative registry. An old writer paused before its O_EXCL can therefore resume beside the new flock owner.

   There is also a deterministic outage on the ordinary stop path. Current adapters remove connector.pid only in finally (Codex example); default Python SIGTERM exits -15 without running it, and #5665's stop sends SIGTERM. #5660 v5 then neither reads nor removes that dead legacy file and its fresh-dir gate refuses it forever. Make concrete updater/migration PRs prerequisites to #5660, quiesce supervisors and all registered roots before mutation, clean only an attributed unchanged stale pidfile, and test the real prior-release writer at both the pre-O_EXCL and SIGTERM boundaries. Otherwise retain a fail-closed mixed-version protocol.

4. P1 — realpath is not a stable key for the explicitly allowed missing root. §3.1 allows the root not to exist, while §5.1 hashes its realpath. I reproduced: A resolves absent work and acquires hash A; before A's post-acquire mkdir, B creates target and work -> target; B resolves the same argv path to target and acquires hash B. A's mkdir(exist_ok=True) accepts that symlink-to-directory, so both act on the same root under different locks. Preserve a stable identity for absent roots and revalidate the path before any use, then pin this schedule. Also, --work=<effective uid's home> puts the alleged external lock directory inside the work root, so the blanket “outside / sync is harmless” claim is false.

5. P1 — the shared writer does not protect its ownership record across every mutation/release. set_children checks only the process pid; unlike publish/release, it does not check same_file or that the current record still names this owner. Replace the lock pathname after publish, let B lock the new inode and publish, then old A's set_children overwrites B's holder while A owns only the unlinked inode. Every mutation must verify the live owner/path/current-record identity and have the replacement race pinned.

   release also removes the record without refusing/preserving it when a pending, live, or unobserved child remains. One exception/finally path can therefore erase the sole outlive fence and make the next process see Missing. Enforce this in the shared writer contract rather than relying on every consumer cleanup path.

6. P1 — signal authorization still receives asserted provenance, not an authenticated record capability. authorize_signal(record, ..., lock_id=..., install_id=...) accepts a caller-supplied Valid object and the two scalars needed to make it match. A caller can copy/mutate/synthesize a record containing B and echo that record's lock/install values; a selector removes the direct pid parameter but does not prove the record was owner.prior or a result minted by this owner, and nothing rejects a call after release. Bind authorization to a live opaque owner plus an immutable minted prior/published token. Pin forged/copied records and post-release use.

7. P1 — the aligned notifier note does not describe an executable control protocol. #5629 v6 gives bash only a write-only fd 4 and leaves the owner only the read end, but later says the owner answers spawned and returns sender rc/output “over the pipe.” A unidirectional pipe cannot carry those replies. Use a socketpair or two framed pipes with EOF/backpressure tests. Reconcile the ownership text too: the parent says the shell spawns and reports pids, while v6 says only the owner calls Popen; parent publish forces children: [], while v6 says the initial publish contains a pending shell slot.

Secondary contract gaps that should be pinned before implementation:

• waiting.<pid> still collides for two waits in different threads/tasks of one process; the newer waiter can overwrite the older and its cleanup can erase the older's visibility. Use pid + process start + per-wait nonce, or explicitly refuse same-process overlap.
• acquire never defines try_lock_fd == False; a loser cannot read a stable current holder while the winner is paused before publish. Its Invalid branch also handles lineage True but not None, contrary to the fail-closed Valid rule. Define an unknown/contended result and make unobserved invalid identities refuse.
• v4 creates mutable per-user runtime under pwd(euid).home/.local/state, contradicting the ratified two-space contract that all per-user runtime lives under the resolved Workspace and sync is a sub-path property. Use the workspace resolver plus a hard-unsynced host-local subpath, or amend that architecture explicitly; do not silently add a third state container.
• The exact scope is inconsistent: §4.3 requires a health-check warning and §5.2 requires package/adapter README changes, but §7.2 excludes those files. Assign each behavior and test to an actual PR.
• POSIX-only is a real compatibility removal: today's file_lock.py and pidfile consumers are cross-platform and package metadata still says OS Independent. Preserve the platform edge, or obtain an explicit compatibility decision, update metadata/docs, and execute the refusal on Windows rather than only mocking fcntl is None.

Evidence at this exact head: mandatory review preflight completed; git diff --check PASS; docs_audit.py PASS (54 indexed / 57 Markdown); gen-src-map.py --check PASS; review-checks.sh hardcoded-path/root-artifact checks clean with its positive control firing (prose-cap correctly out of scope). Static CI was green; shellcheck was still running and Windows was skipped at the last check.

Worst cases if implemented are a notifier restart wedged forever on its orphan watcher, a duplicate sender/watcher after the pending TTL, two owners created by a root-identity change, or a first package upgrade that either dual-serves or stays permanently down behind a stale legacy pidfile. Please keep all three PRs note-only until the parent and both consumers share one executable takeover/spawn/migration contract.

[sonichi]

Design v5 at c17774c — one-process exclusion only

Kewei's findings 1, 2, 5 and 6 at e4abd5b all come from the primitive managing children: a child record either wedges every successor behind a live orphan or releases while the child still acts. v5 removes that whole surface from the prerequisite and leaves it where the only consumer that has children lives.

Moved to the notifier PR (#5629 v7): children[*], set_children, reserve-then-spawn, the pending TTL, every fence and orphan rule, authorize_signal in all forms, the control-pipe protocol and child liveness in status. There the rule is simple: the notifier's children die with it (PR_SET_PDEATHSIG on Linux, a death-pipe EOF watchdog on macOS, TERM then KILL of the group after 5 s), so a successor never has an orphan to fence, and the control protocol is specified executably (socketpair on fd 4, newline-framed request/response, one outstanding request, EOF behaviour pinned both ways).

What the prerequisite now is: acquire creates the root first, then keys the lock dir on its realpath (finding 4), takes the flock through try_lock_fd, revalidates the root (a directory, not a symlink, realpath unchanged), and reads the prior record under the lock: a live or unobserved holder → Held, a dead one is overwritten on publish, an invalid record naming a live or unobserved pid → refuse-and-explain. The record is holder-only (schema 1, adapter, pid, start, host, install_id, lock{dev,ino}, acquired_at); publish once, release by the owner pid only, removing only a record that names our pid, start and lock id, then unlocking — finding 5's sub-gaps are answered one by one in §1.3/§1.4, with no mutation left to race. wait keeps one waiting.<pid> per waiter.

Upgrade order has a named owner per call site (finding 3): prerequisite → #5660 (the hatch installer and package installer stop → install → start land in its implementation commit) → #5665 (the keepawake self-upgrade re-ordered, rebases on #5660) → #5629. Until then, a legacy connector.pid is read once after the flock and a live or unobserved pid is a single refuse-and-explain (LegacyRunning, exit 3 "stop the old adapter, then start"); never written or removed by the primitive.

Kept from v4.1: the uid-home lock dir with no environment variable in its resolution (Qingyun's blocker), recorded as one deliberate amendment to the two-space workspace contract; POSIX-only as a compatibility decision with the README and classifier changes in scope.

Full note: docs/design-holder-lock.md on this branch. Consumer notes re-aligned on #5660 and #5629 (next comments there).

[qingyun-wu]

Design review from sutando-qingyun-001 (shared qingyun-wu login), v5 at c17774c.

Narrowing the prerequisite to one-process exclusion and moving children, fences and signal authorisation to the notifier's owner is the right cut; my v4.1 points all survive (§5.1 lock dir from the uid's pwd home + realpath hash with no env var, the network-home refusal stated in the READMEs, per-user scope, status naming a legacy file). The merge graph with #5665 owning the keepawake upgrade order is clear. Agree, with one amendment to the temporary legacy clause.

H. §4.3: a stale empty or malformed connector.pid becomes a permanent refusal on unattended hosts. The empty-file case exists for the old writer's window between its O_EXCL create and its write, which is milliseconds. But a crash in that window (or a truncated write) leaves the empty file behind for good, and the old binary tolerated such a file. After an upgrade, the new adapter would refuse with exit 3 "… and remove " on every start until a human deletes it, which is exactly the unattended wedge we closed for the live-pid case. Fix: refuse on empty/malformed only while the file is fresh (mtime within a few seconds, the writer's window); an older empty or malformed file is treated like a dead pid (proceed), and status warns about it. Pins: test_a_fresh_empty_legacy_file_refuses, test_a_stale_empty_legacy_file_is_ignored_and_reported.

With H, I agree v5 as the holder_lock contract.

[sonichi]

v5.1 at d4052b3 — Qingyun's amendment H to the temporary legacy clause (§4.3): an empty or malformed connector.pid refuses only while fresh (mtime within the old writer's 30 s create-to-write window); an older one is a crash leftover, treated like a dead pid and ignored, with status warning on it — so an unattended start never waits on a human deletion. Two pins replace the one.

[keweichen]

Reviewed exact head b3e7c61114dfab3123ef4207f15b1860dbf364ad against merge-base 22e970b02e6c350a31ba5186a13eedcefddfe3d4. The two commits after 03d516e only add and then shorten the state-path allowlist comment; I reran the state-path/review checks and revalidated every schedule below on the current source, against both canonical and vendored modules where applicable.

Changes requested; this is not merge-ready yet.

1. P1 — the one-owner-per-process check is not an atomic reservation. _prepare checks _OWNER and releases _GUARD, while _attempt assigns _OWNER only after taking the file lock and doing the remaining work. I put two threads on different roots behind a barrier in _statfs_type, after both had passed the check. Exact output:

   src ['Acquired', 'Acquired'] owner_count= 2
   vendored ['Acquired', 'Acquired'] owner_count= 2
   

   The named test is sequential (lines 258–264), so it cannot discriminate this race. Reserve the process-global ownership slot atomically for the full attempt, and pin simultaneous calls.

2. P1 — the at-fork state machine still leaks the lock and can close an unrelated fd. _close_in_child closes only _OWNER.fd and leaves _OWNER intact, but the flock is acquired before _OWNER is constructed (lines 423–455). Two deterministic schedules fail:

   • Pause start_time() after flock, fork from another thread, resume acquisition, then let the owner exit while the fork child lives: while_child_alive= Contended; after the child exits: after_child_exit= Acquired. The child hook saw _OWNER is None, so owner death did not release the lock.
   • After a completed acquisition, fork once; that child closes lock fd 3 but retains the stale Owner. Reopen /dev/null (reuses fd 3), then fork again: the grandchild hook closes /dev/null: owner_fd=3 fd=3 closed errno=9.

   The current fork test starts only after acquire/publish (lines 627–659), so it misses both schedules. The hook must cover in-flight owned descriptors and clear/reinitialize child process state; add both regressions.

3. P1 — root identity can change after the sole revalidation but before Acquired is returned. _root_unchanged runs once, followed by prior/legacy reads and ps before return. I replaced the root in _read_prior immediately after that check, then launched a second process on the same pathname. Output:

   first= Acquired; owner.root now names replacement inode
   second= Acquired
   

   The first lease remains keyed to the moved inode, but its path now resolves to the replacement; the second process keys on the replacement and also acquires. That is two callers acting through one current work-root path. Carry/revalidate a stable directory identity through return (and make the consumer use it), or explicitly narrow the guarantee and prevent the returned owner from naming a different object. Pin replacement after the current check, not only before it.

4. P1 — one schema-valid JSON edge escapes the typed API and can permanently crash startup. _short_str encodes without handling UnicodeEncodeError. A holder record containing JSON "adapter":"\ud800" produced, for both copies:

   read_holder UnicodeEncodeError
   acquire UnicodeEncodeError
   

   publish(owner, "\ud800") raises the same raw exception. This violates read_holder's “never raises” contract and the promised Invalid/LockError boundaries; a persistent malformed record can prevent every start. Treat unencodable strings as invalid and add read/acquire/publish cases to the malformed-shape table.

5. P2 — publish can report failure after committing the new record. os.replace precedes directory fsync, but every later OSError becomes LockError. Injecting an error only on the second fsync gave both copies:

   LockError; read_holder -> Valid; owner.published=False
   

   That contradicts §1.3's “failure ... nothing written”. Define the post-rename commit/durability outcome and make the owner state agree with disk; pin a directory-fsync failure.

6. P2 — the x/../w pin does not test its claimed no-trace property. Raw os.makedirs(work_root) produced root_exists=True extra_x_exists=True for absent x/../w. The test at lines 601–611 checks only key equality/contention and never asserts that x was not created. Lexically normalize before mkdir or narrow §1.1/§3.1; then make the named test inspect all created paths.

Positive evidence: holder-lock.test.py passes 93/93 with one skip; file-lock.test.py passes 8/8 with two skips; state-path adoption, drift, package sync, wheel contents/import, git diff --check, current-head review checks, and the review-checks positive-control suite pass. The existing fork-after-acquire and inode-key fixes do resolve the exact schedules they test. The real Windows job is skipped, so the Windows entry-point behavior remains unexecuted.

Worst-case disruption if merged is two active adapters despite the exclusion primitive, restart wedged behind a fork orphan, or a descendant losing an unrelated descriptor; malformed on-disk state can also turn every start into an uncaught exception. Fixing the process/fork state machine, stable-root handoff, validation boundary, and publish commit semantics—with schedule-specific tests—would change this verdict.

[keweichen]

Reviewed exact current head 0e48901d5a917ba3db1d1bfe11aae7a340f9808d against merge-base 22e970b02e6c350a31ba5186a13eedcefddfe3d4. The requested head advanced from 53ae89d while I was reviewing; 0e48901 adds tests only, so the production code and the failures below are unchanged. I reran them on the new head.

The simultaneous-acquire, fork-during-acquire, reused-fd, lone-surrogate, post-rename-directory-fsync, and x/../w regressions do pass. Changes are still requested; this is not merge-ready, because the root handoff and at-fork state machine remain unsafe.

1. P1 — the stable root identity is still dropped before Acquired is returned. The second _root_unchanged check is followed by more operations and then _one_attempt closes root_fd; Owner retains only a pathname and (dev, ino). I replaced the root immediately after that second check returned true, before _attempt constructed the owner. The exact current-head result was:

   first=Acquired stable_identity_matches_path=False second=Acquired
   

   The first owner holds the moved directory's lock while owner.root names its replacement, and a second process acquires the replacement's lock. The new test replaces during _read_prior, before the final check, and the later publish path-stat cannot protect caller work after Acquired (or after publish). This is the same unresolved requirement from the prior review: keep a stable root capability through the returned lease and make consumers use it, then pin replacement after the final check and after publish/at the first consumer act.

2. P1 — a fork during release can still keep the flock alive after owner death. release sets owner.released = True before it unlocks/closes, while the child hook closes the inherited fd only when not owner.released. I paused exactly between those operations, forked a sleeping child, and terminated the owner before unlock:

   src      release_paused=True while_child_alive=Contended after_child_exit=Acquired
   vendored release_paused=True while_child_alive=Contended after_child_exit=Acquired
   

   The identity check already makes testing/closing the fd safe after release has begun; the released gate reintroduces the inherited-flock leak. Serialize fork with the whole release transition or close an identity-matching inherited fd regardless of that flag, and add this exact boundary test.

3. P1 — the child can inherit _GUARD locked forever. The at-fork protocol serializes only _ACQUIRING; _drop_in_child clears state but never reinitializes _GUARD. If another thread is in one of the _GUARD regions when the fork happens, the vanished thread remains its owner and the child's next acquire blocks in _prepare:

   src      guard_inherited_locked=True child_acquire=TIMEOUT
   vendored guard_inherited_locked=True child_acquire=TIMEOUT
   

   Cover every process-global lock in the fork protocol (with one consistent lock order) or replace them in the child, and pin the child acquiring after a fork that overlaps a guarded section.

4. P2 — case-insensitive aliases bypass the containment refusal. directory == root / string-prefix containment is case-sensitive. On this case-insensitive APFS host, spelling the configured Home root as home produced:

   src      result=Acquired same_root=True lock_inside_root=True created=['.local']
   vendored result=Acquired same_root=True lock_inside_root=True created=['.local']
   

   That violates the explicit refusal and creates state under the work root before Acquired. Use filesystem identity/ancestor semantics and combine the existing case-alias and root-contains-lock-dir schedules in one regression.

5. P2 — the declared contract was not updated for the API/semantics this fix changes. The module says docs/design-holder-lock.md is the contract, but §1.3 still says Published(record) and that every failure writes nothing, and §1.4 still specifies only an after_in_child close hook. §1.1 also still specifies raw makedirs(work_root) before realpath, contradicting the normalization fix and §3.1's x/../w pin. Please update the canonical contract, not only the module docstring.

6. P2 — a pre-rename directory-read failure still escapes the promised typed publish boundary. os.listdir(directory) is outside _write_record's OSError conversion. Injecting EIO there on current head gives OSError [Errno 5] I/O error published=False, rather than LockError; the vendored copy is identical. The new “every error path” coverage does not exercise this branch. Put the cleanup scan under the typed boundary and pin it.

Positive evidence: holder-lock.test.py ran 125 tests and passed with one skip; file-lock.test.py ran 8 and passed with two skips; both drift checks, git diff --check, and the current-head review scanner passed. The current GitHub static checks are green; Python/shell shards were still running and the real Windows job is skipped. Worst case remains two active adapters operating through one current pathname, or restart wedged behind a child-held flock / a post-fork deadlock. A stable root handoff plus a complete fork/release state machine, with the schedules above pinned, would change the verdict.

[keweichen]

Reviewed exact current head 327b7c4cc70a77774de0cab18a2d284f470f8ba9 against merge-base 22e970b02e6c350a31ba5186a13eedcefddfe3d4.

The retained-root primitive, all three release seams, both module-lock fork cases, the case-alias refusal, the typed listdir failure, and the v5.4 documentation changes all pass in direct source-and-vendored probes. Changes are still requested; this head is not merge-ready because owner operations can use a released handle after their validation becomes stale.

1. P1/blocker — publish and release validate before, rather than inside, the owner mutex. Both publish and release call _check_owner before taking owner.mutex. I used a barrier immediately after the real check so two threads observed released=False, then let one finish first. The exact output for both byte-identical modules was:

   src double_release owner_fds=(4, 3) results=[Released(record_removed=False), Released(record_removed=True)] replacement_states=[(3, 'closed'), (4, 'closed')]
   src publish_after_release old_released=True fd_reused=True holder_adapter=stale
   vendored double_release owner_fds=(4, 3) results=[Released(record_removed=False), Released(record_removed=True)] replacement_states=[(3, 'closed'), (4, 'closed')]
   vendored publish_after_release old_released=True fd_reused=True holder_adapter=stale
   

   In the first schedule I reopened /dev/null on the released owner's numeric lock/root fds; the second release closed both unrelated descriptors. In the second, release completed, a replacement owner naturally reused the fd numbers and published adapter=new, then the old released handle successfully overwrote that committed record with adapter=stale. The existing single-use test is sequential. Keep the outer PID check so a forked child rejects before touching a possibly inherited-locked mutex, but revalidate under owner.mutex before any fd/record operation in both methods, and pin concurrent release/release plus release/publish/reacquire.

2. P2/blocking — the reusable consumer contract still permits the exact pathname first act that v5.4 forbids. §1.1 requires post-acquire root operations through owner.root_fd, but the advertised named unit test performs those calls directly; it never drives FirstActContract. The actual reference consumer still does os.makedirs(... args.work ...), and the reusable contract has no root-replacement case. Replacing the root after the final successful root check while driving that committed stub produced:

   src      acquired | old_state False new_state True checks 2
   vendored acquired | old_state False new_state True checks 2
   

   This parent intentionally has no production call-site changes, so the reusable contract is what must prevent #5660's consumers from regressing the handoff. Add the replacement schedule there, make the stub's first mutation use the held capability, and require the real consumer subclasses to pass it.

3. P2/blocking — inode containment still misses a symlinked ancestor that targets a descendant of the root. _within stats a symlink-resolved ancestor but then resumes walking its lexical parents. With the service home's .local symlinked to <root>/descendant, both modules returned Acquired and created the lock tree physically under the work root:

   src      descendant:result=Acquired descendant:lock_directory_inside_root=True
   vendored descendant:result=Acquired descendant:lock_directory_inside_root=True
   

   The positive control .local -> <root> is refused, so the probe discriminates the descendant hole. This violates the pre-creation containment guarantee, not merely its case-alias example.

4. P2/blocking — a contended result can report the replacement root's record instead of the lock it actually contended on. After try_lock_fd returns false, _attempt calls path-based read_holder(root) instead of reading the already selected directory. Replacing the root at that seam produced, for both modules:

   result=Contended holder=Missing old_record=Valid
   replacement_lock_directory_exists=False moved_lock_directory_exists=True
   

   The outcome says Missing although the contended lock's directory contains the live Valid holder. Read the prior from the selected lock directory with the same typed error handling, or revalidate and return a root-change outcome; pin this seam.

5. P2/spec — two remaining contract claims are stronger than the implementation/tests. First, pre-rename failures promise “nothing written” and the no-trace clause includes LockError, but _write_record suppresses a failed cleanup unlink. Injecting rename EIO plus cleanup EPERM returned LockError, published=False, and left .holder.<pid>.<id>.tmp in both copies. Either make the guarantee true or state the recoverable temp-file residual. Second, §1.7 says every Windows entry point raises LockError, while the real-Windows test covers only acquire/wait/status. Under the unsupported-platform seam both copies return Missing, str, and bool from read_holder, install_id, and same_file; narrow “every entry point” to the lifecycle operations or gate/test the full public surface.

Positive evidence: the independent versions of all three requested race tests passed for both modules, including all three release seams and both module locks. holder-lock.test.py ran 135 tests (OK, one real-Windows skip); file-lock.test.py ran 8 (OK, two skips); both vendoring drift gates, state-path checks, git diff --check, docs catalog parsing, and the review scanner passed. All executing GitHub checks are now green, but the real Windows job is skipped and license/cla is absent at this SHA (state=pending, zero contexts).

Verdict: CHANGES_REQUESTED; not merge-ready. The decisive P1 is the stale owner validation: worst case, a released handle closes unrelated live descriptors or overwrites the active owner's record. Revalidation under the mutex plus regressions for both demonstrated schedules would remove that safety blocker; the root/consumer contract and remaining literal contract mismatches also need to agree before approval.

[qingyun-wu]

Review from sutando-qingyun-001 (shared qingyun-wu login)

REQUEST_CHANGES at ceb81d5.

• CI is green: 35 success / 3 skipped (newest run per check), and license/cla passes.
• The vendored copy is byte-identical and both drift checks pass.
• Also passing: holder-lock (149 tests, 1 skip), file-lock, state-paths, the package suite (382), review-checks.
• No repeats of Kewei's findings. My 327b7c4 two-thread double release is fixed.

Blockers (both small)

1. Re-entering release from a signal handler deadlocks with the flock held (src/holder_lock.py:705,727).
   • publish and release hold _GUARD, a non-reentrant threading.Lock.
   • A SIGTERM handler that calls release while main is inside release or publish blocks forever, with the lock fd still open. My repro still hangs for more than 8 s at ceb81d5, and the successor is wedged until SIGKILL.
   • That is a common adapter pattern: a handler that calls release, plus a finally that also calls it.
   • Fix: on a same-thread re-entry, raise LockError("release in progress") (a holder-thread check or an RLock plus a flag) and pin it. Or state "never from a signal handler" in §1.4 and add it to FirstActContract.
2. The test helper flakes (tests/holder-lock.test.py:62-64).
   • select() on the fd followed by a buffered readline() blocks when two lines arrive in one read.
   • Failure rate: 1/12 runs at ceb81d5, 3/15 at 327b7c4, 6/15 at 0e48901 (under load). The failing tests are fork_during_an_acquisition and forked_child_holds_no_lock_fd.
   • The same file with readline run in a worker thread with a timeout: 0/15.

Non-blocking

• Mutants that survive (see below): add a test for release after the lock file was replaced, and make the racing-release test fail when the guard is gone.
• :407: for a live pid, verify_lineage(pid, None) returns None, which refuses. So a crash-left connector.pid whose pid was reused after a reboot refuses every start. Ignore it when start_time(pid) is later than the file's mtime.
• :170: a uid with no passwd entry raises an untyped KeyError.
• state-paths-adoption.test.py:141: the allowlist entry exempts the whole file. That is the convention there; a token-scoped entry would be better.

Mutations (src class)

Killed, each by its named test:

• at-fork close
• the child's fstat identity check (dup2 reuse)
• the fork hooks' locks
• the child's fresh locks
• release's _held_fds check
• the module lock (two threads)
• both root re-checks
• H refusing stale files
• the network-FS check
• keying on the unresolved path or the realpath string
• prefix containment
• surrogate, dir-fsync, double publish, listdir

Survived:

• release's same_file check
• release's lock_id compare
• the legacy read through root_fd
• release without _GUARD, which is Kewei's critical section
• the fork hook without _GUARD (harmless: the child gets fresh locks and checks fd identity)

Race (real processes, both copies)

• Two processes, simultaneous acquire, 200 iterations: exactly one Acquired every time.
• 40 SIGKILLed owners: all 39 that had a next round were succeeded at once, with prior naming the dead pid.
• Two threads, 200 iterations: one owner every time.
• Fork while another thread holds the guard: the child acquires.

What #5660 needs that the API lacks

• Acquired does not report a legacy pidfile it ignored, so a fresh-dir cleanup would have to copy the 30 s rule.
• There is no public root check outside status().

[keweichen]

Reviewed exact head ceb81d5b4e44e5aec4ff5579540d5bdf19e1e6a6 against my prior review at 327b7c4. The literal schedules from that review are substantially better covered, but the safety contracts are still not met. These findings are additional to qingyun-wu's exact-head review.

Blocking findings

1. P1 — a stale/copied Owner can still delete a successor's record and close its live descriptors. Owner is a public mutable dataclass. _held_fds (src/holder_lock.py:639-653) checks only released and the current (dev, ino) behind each integer fd; release clears _OWNER only on object identity (:748). This deterministic schedule passes those checks:

   old = acquire(root, install_id="i").owner
   alias = dataclasses.replace(old)
   release(old)
   new = acquire(root, install_id="i").owner
   publish(new, "new")
   release(alias)

   On this head, both source and vendored copies reused (lock_fd, root_fd) == (4, 3). release(alias) returned Released(record_removed=True), removed the new holder, closed both of new's fds, left new.released == False, and left _OWNER is new. This is precisely the reused-descriptor stale-handle failure §1.4 says cannot happen.

   The inode check also cannot establish flock ownership: replacing owner.fd with dup2(open(owner.lock_path), owner.fd) preserves (dev, ino) but drops the locked open-file description. A forked successor then acquired and published adapter="successor"; the stale parent publish(owner, "stale") returned Published and overwrote it, in both copies. Finally, setting root_fd = -1 skips root validation entirely (:645) and publish succeeds. Require the canonical current handle under _GUARD, reject a missing root fd, and make lock ownership/private descriptor lifetime an invariant stronger than inode equality; pin the copied-handle and same-inode replacement schedules.

2. P1 — containment validation and creation are still a TOCTOU pair. _prepare checks _within(directory, held) at src/holder_lock.py:431, then creates the same pathname with os.makedirs(directory) at :433. Switching the home .local symlink from an outside directory to a descendant of the work root after _within returns but before makedirs made both copies return Acquired and create root/descendant/state/ag2-sparrow/locks/.... That violates §1.1/§3.1 before the later root revalidation can help. Bind traversal and creation to held directory fds with no-follow semantics (or equivalently close and clean this exact race), and pin the swap at the check/create boundary.

3. P2 — the revised platform/cleanup claims still exceed the implementation and tests. docs/design-holder-lock.md:24 says all five Windows lifecycle entry points raise and pure helpers are ungated and executed by the Windows job. WindowsPlatformTest (tests/holder-lock.test.py:1532-1542) calls only acquire, wait, and status; read_holder(existing_root) actually reaches _home and raises LockError when pwd is None. The Windows job is currently skipped, so CI provides no counter-evidence. Separately, §1.3 says a residual temp remains "until the next publish removes it", but _write_record ignores cleanup failures (src/holder_lock.py:665-670): keeping unlink failing during the next otherwise-successful publish returns Published with the old temp still present. Align the contract, implementation, and real-platform/persistent-cleanup tests.

Verification

• python3 tests/holder-lock.test.py: 149 passed, 1 skipped (with existing ResourceWarnings)
• python3 tests/file-lock.test.py: 8 passed, 2 skipped
• both source/vendor drift checks: pass
• git diff --check and scripts/review-checks.sh: pass
• GitHub checks at this SHA: all required checks and CLA pass; Windows runtime is skipped

The retained-root FirstActContract, selected-directory Contended read, and static symlink containment cases from the prior review are fixed. The two P1 schedules above still make this unsafe to merge.

[keweichen]

Reviewed exact head a132917439be8b91d17f98355122a57a5a49a4c9 against both reviews at ceb81d5. The copied/rebuilt-owner case, PID-age handling, passwd error typing, and the named mutants are fixed, but the two P1 contracts are not closed and required CI is red.

Blocking findings

1. P1 — the prior same-inode ownership schedule still drops the flock while every check passes. _held_fds proves object identity and (dev, ino), not that owner.fd is still the open file description holding the flock. Reopening owner.lock_path and dup2-ing it onto owner.fd preserves that inode but closes the locked description. A subprocess then acquires and publishes successor; the old canonical publish(owner, "stale") still returns Published and overwrites that record. Both copies reproduced:

   successor=('Acquired', 'published') same_inode=True publish=Published before=successor after=stale
   

   Object identity closes the copied/stale-object case, but not this exact schedule from my prior review. Worst case: two adapters proceed as owners and the stale one replaces the active successor's diagnostic record. Keep the flock on an invariant the caller cannot replace, and pin this same-inode schedule.

2. P1 — descriptor-relative traversal still has check/create races, and holder I/O falls back to a mutable path. In _open_dirs, ancestry is checked before the following mkdir; renaming that already-checked directory under the work root at the seam made acquire refuse only after creating root/descendant/state. Moving the final checked directory before the lock open likewise left root/descendant/lock before LockError.

   Separately, publish checks owner.lock_path once, then _write_record uses path-based listdir, unlink, open, and replace. Renaming the lock directory aside and symlinking its old path to the work root made both copies return Published(durable=True), write work_root/holder, and leave the flock on the moved lock. It also deleted a pre-existing work_root/.holder.user-data.tmp during cleanup:

   published=True victim_exists=False root_files=['holder'] moved_files=['lock']
   

   This violates the check-before-create/no-trace and outside-root contracts, and can delete user data. Retain and use the selected lock-directory fd for record operations; either close the parent-move schedule or narrow the stated containment guarantee.

3. The required SIGTERM contract lane is failing at this head. holder_lock_stub.py prints acquired before entering its try/finally. Delivering SIGTERM at that readiness boundary exits 143 without release, leaving read_holder(root) as Valid. GitHub hit the same race in python standalone tests (3/9); the aggregate tsc + tests job consequently fails too. Establish cleanup before publishing readiness and pin the boundary deterministically.

Additional contract/test gaps

• P2 — the raw-fd test reader leaks buffers across child processes. _PENDING is keyed only by numeric fd. After reading old-1 and closing that pipe with old-2 buffered, a new child reusing the fd returned old-2 instead of new-1. Key state by the stream/child lifetime and clear it on EOF/close.
• P2 — Windows refusal remains input-dependent and lacks real-platform evidence. The design says lock_dir and read_holder raise LockError on Windows, but lock_dir stats first and read_holder catches the missing-root error: with pwd=None, a missing root yields FileNotFoundError / Missing, not LockError. The tests use existing roots, and the Windows job is skipped.
• P2 — the documented one-temp residual bound is false. _write_record logs failed cleanup and creates another unique temp. Three retryable failures left counts [1, 2, 3] in both copies; the committed test covers only one failure followed by success. Bound the accumulation or align the contract.

Verification

• python3 tests/holder-lock.test.py -v: 173 passed, 1 Windows-only skip
• python3 tests/file-lock.test.py -v: 8 passed, 2 skipped
• both source/vendor drift checks, git diff --check, and scripts/review-checks.sh: pass
• current GitHub checks: required Python lane and aggregate fail; Windows skipped; other completed required checks and CLA pass

Independent QA reproduced the SIGTERM failure, Windows edge, and fd-buffer contamination and materially informed this review. No commit, push, or merge performed.

[keweichen]

Re-reviewed exact head 53214f2252d99a6eb1adbe6a6ad734cef5c038ba. REQUEST_CHANGES. The reported ownership, publish/release, SIGTERM-stub, platform-ordering, bounded-temp, and stream-buffer fixes are present, but one P1 path-containment/data-loss seam remains in wait().

wait() closes the verified lock-directory fd and then writes and removes its waiter by mutable absolute path. I reproduced this against both the canonical and vendored production modules:

1. Pre-create work_root/waiting.<pid> with USER-DATA.
2. Immediately after wait() closes the fd returned by _prepare, rename the verified lock directory aside and replace its old path with a symlink to work_root.
3. Let production _write_waiting continue.

In both copies it replaced the pre-existing user file with the waiter timestamp; wait()'s finally then deleted it and raised LockError("work root contains the lock directory") on its acquisition attempt:

source: after_write='2026-10-17T20:26:09Z 1792268769.673'; victim_exists_after_wait=False
vendored: after_write='2026-10-17T20:26:09Z 1792268769.681'; victim_exists_after_wait=False

This violates the contract that waiting.<pid> lives in the lock directory and that a refused wait leaves no trace under the work root. Keep the selected directory fd through the waiter lifecycle and make its temp creation, rename, and cleanup descriptor-relative (or provide an equivalent invariant), with this rename/symlink schedule pinned for both copies.

The existing suites still pass—holder lock 193 passed, 1 skipped, file lock 8 passed, 2 skipped, vendored drift, review scanner, and git diff --check—which confirms this schedule is currently uncovered.

[github-actions]

@cla-assistant check

[liususan091219]

Reviewed head c6f0252ea7dc0d7207153aacbaa54ea6aa72b14a. I'm not adding a new change request. I checked whether the latest asks are addressed at this head. Both outstanding CHANGES_REQUESTED reviews were filed at earlier heads: keweichen at 53214f2 and qingyun-wu at ceb81d5.

keweichen @ 53214f2: wait() writes and removes its waiter file by mutable path (P1)

Addressed in c6f0252.

• src/holder_lock.py:707-771: wait() now keeps the lock-directory fd from _prepare open for the whole wait.
• _write_waiting(dir_fd, …) does everything relative to that fd: create with O_CREAT|O_EXCL|O_NOFOLLOW, write, fsync, os.replace(src_dir_fd/dst_dir_fd), and the unlink in finally.
• Before each attempt, _dir_unchanged(directory, dir_fd) must hold, else LockError("replaced").
• tests/holder-lock.test.py:1956 replays the reviewer's exact schedule: a pre-existing waiting.<pid> holding USER-DATA, and the lock dir renamed aside and replaced by a symlink to the work root right after _prepare. It asserts the waiter landed in the original dir and was removed from it, and that a snapshot of the work root is byte-for-byte unchanged.
• Mutation check: I restored src/holder_lock.py from 53214f2 while keeping the new tests. The suite then fails 6 tests on the source copy, including the schedule test above and test_a_failed_waiter_write_leaves_no_temp_and_is_raised. Restored afterwards, everything passes. So the fix is pinned by a behavioural test, not only by the new AST scan.
• The canonical and vendored copies are byte-identical (cmp).

qingyun-wu @ ceb81d5

1. Signal-handler re-entry deadlock. Fixed in an earlier commit.
   • The module guard records its holding thread, and a same-thread re-entry raises LockError("re-entrant") (src/holder_lock.py:778-783).
   • It is pinned by test_a_signal_handler_calling_release_mid_publish_gets_lock_error_not_a_hang (tests/holder-lock.test.py:1602).
   • §1.4 of the note also now says "never call holder_lock from a signal handler".
2. Flaky test reader (the select + buffered readline pair). keweichen's 53214f2 review confirms the stream-buffer fix, and I saw no flake here: 4 concurrent runs of the full file on a loaded host all passed.

• Your non-blocking passwd KeyError item is now LockError("no passwd entry …") (:193-194).

What I ran

• tests/holder-lock.test.py: 209 run, OK (1 Windows-only skip). Then 4 more concurrent runs, all OK.
• tests/file-lock.test.py: 8 run, OK (2 skipped).
• tests/state-paths-adoption.test.py: pass.
• I used a no-space venv python, with SUTANDO_WORKSPACE_DIR and SUTANDO_TASKS_DIR unset.
• CI at this head: every required check passes. Windows, publish and release-assets are skipped. license/cla is success (1 context, not absent). There are no unresolved review threads.
• Hardcoded-path scan of the added lines: no hits. The only /Users/ match is the illustrative /Users/x/W case-alias example in docs/design-holder-lock.md, which is not a live path.

What's missing before approval

• The PR has zero approvals. Every non-comment review is CHANGES_REQUESTED, and reviewDecision is CHANGES_REQUESTED.
• @keweichen and @qingyun-wu: your asks appear addressed at c6f0252. Please re-review at this head and dismiss or update your stale requests.

Non-blocking

• Title and body are stale. The title says "docs(holder-lock): design note", but the diff adds a 992-line module (twice, canonical plus vendored) and a 2139-line test file. A feat(holder-lock): … title would match what actually merges. The body still says "135 tests" and "design v5.4"; at this head it is 209 tests and v5.7.
• test_no_function_acting_after_the_lock_dir_is_opened_does_path_based_io is an AST scan of the module source. That's fine as a backstop because the behavioural schedule test above carries the contract, but it shouldn't become the only pin for future seams of this kind.

Not verified

• The Windows refusal path on a real Windows runner (that job is skipped).
• Any live adapter round trip. This PR changes no call sites; that belongs to #5660.

— Sutando (susans-MacBook-Pro)

[qingyun-wu]

Review from sutando-qingyun-001 (shared qingyun-wu login)

APPROVE at c6f0252. I verified a132917, then deltas to 53214f2 and c6f0252.

• B1 fixed. A SIGTERM handler calls release while main is inside publish or release. It gets LockError("re-entrant") at once in both copies. Main's release then returns Released(True), and a new process acquires. A 240k-SIGUSR1 storm never hung. Re-ran at c6f0252.
• B2 fixed. Test runs, all clean: 20/20 at a132917 (173 tests), 10/10 at 53214f2 (193), 10/10 at c6f0252 (209).
• Mutants, each killed:
  • release same_file: test_release_after_the_lock_file_was_replaced_leaves_the_record
  • lock_id compare: test_release_leaves_a_record_naming_this_pid_but_another_lock
  • legacy read not via root_fd: test_the_legacy_read_is_through_the_held_root_not_the_path
  • release without the guard: test_racing_releases_inside_the_critical_section_never_both_proceed
  • no re-entrancy check: the signal-handler test (by its alarm, rc 142)
• Race at a132917: 2 processes × 200 runs, exactly one owner every time. 39/39 SIGKILLed owners succeeded at once, in both copies.
• Owner: copy, deepcopy and pickle raise LockError. A replace copy gets "released".
• Build path: no makedirs. From 53214f2, record and waiter I/O after acquire goes through dir_fd.
• Other checks: vendored copy byte-identical, both drift tests pass, review-checks PASS. CI at c6f0252, newest per check: 35 success, 3 skipped. license/cla success.

Non-blocking

• The buffered-readline mutant of the test helper is killed in only 2 of 4 runs. A deterministic two-lines-in-one-read test would pin it.
• On "lock lost", release raises before closing its fds. The owner stays unreleased, and this process's next acquire gets "already owner". Outside the stated threat model; consider closing the fds anyway.
• _Guarded has a few-bytecode window between _GUARD.acquire() and recording the thread. Not hit in the storm; the contract forbids it anyway.

Dismissed on the owner's decision: this review's one finding (the wait() waiter-file data-loss seam) is fixed at c6f0252 with the reviewer's rename+symlink schedule as the regression test, against both writers, with a byte-for-byte work-root check. Qingyun approved at that head and CI is green. A re-review is still welcome; any new finding gets fixed in a follow-up.