refactor(locks): route with-scoped lock opens through open_lock_file (#9267)

[chenmingwei23]

What is the problem?

The non-truncating lock-open preamble (parent mkdir, then open the lock file
WITHOUT truncating, then acquire) exists as hand-rolled copies in several places.
platform_compat.open_lock_file was added in #9316 to hold that idiom once, but
the pre-existing copies were never migrated onto it, so the duplication #9267
describes is still in the tree.

Why this issue matters to the user

Duplicated preambles are how the hooks.json pair drifted in the first place:
two modules locking the SAME file, each with its own open, each carrying its own
rationale comment that a later edit can update in one place and miss in the other.
Every copy is a place a future edit can reintroduce the Windows lock-loss defect
(#9248) that the helper's O_RDWR | O_CREAT was written to prevent.

How our fix solves it

Symptom: four-plus hand-rolled copies of the preamble. Root cause: no shared entry
point, so each site open-codes it. Fix: route the sites the helper can actually
serve through open_lock_file, and point the rest at it.

Inspection found the "four copies" in #9267 are really seven open-sites across
three calling shapes, and the helper serves only one:

• with-scoped (fd lives and dies inside a with) -- migrated onto the helper:
  session_pid._session_pid_file_lock, session_pid._pid_file_lock,
  webhooks.locked, mcp_tools.control.register_hook.
• acquire-now / release-later (fd stored on self._fd, released in
  __aexit__/__exit__, so it must outlive the method) -- left inline with a
  pointer comment: dashboard/handlers/mcp.py::_McpFileLock and its sync sibling
  _McpFileLockSync. A yielding context manager would close the fd at method
  return.
• shared read (try_acquire_lock(exclusive=False)) -- left inline with a
  pointer comment: session_pid._periodic_pid_sweep. The helper serves exclusive
  acquisition; forcing a shared lock through it would be a behaviour change dressed
  as cleanup.

Each migrated site keeps its own parent.mkdir(...) (the helper does not create
parent dirs), drops its touch + "r+" preamble, and replaces its rationale
comment with a one-line pointer to the helper docstring, which now holds the
reasoning once.

work_ledger._open_lock is deliberately excluded. PR #9374 is open,
MERGEABLE, and edits work_ledger.py. Consolidating there would chain two open
PRs on the same file for no benefit a follow-up would not get, and a review change
on #9374 would cost a second rebase. Its canonical docstring stays as the pointer
target until it lands; migrating it is a clean follow-up afterward.

The gap that open_lock_file covers only the with-scoped exclusive shape is
open_lock_file's own design (jeeshofone, #9316); this PR does not extend the
helper and notes the gap on the issue instead.

What tests we did

• test/test_platform_compat.py -- the repo-wide contract scan
  test_no_lock_site_opens_truncating still passes (275 passed).
• test/test_pid_lifecycle.py (172 passed), test/test_hooks_json_shared_file.py
  (15 passed), test/test_webhooks_store.py (47 passed),
  test/test_mcp_core_coverage.py including TestRegisterHook (164 passed).
• Mutation proof: reverting one migrated site (webhooks.locked) to a truncating
  open(lock_path, "w") turned the contract scan RED with webhooks.py:192; the
  scan is load-bearing, not decorative. Restored, green again.

Any other suggestions on the work

The taxonomy is the finding: a duplication issue's stated copy count is a lower
bound, and the shapes hiding inside it decide whether consolidation is even
possible. Five of the seven sites here were never going to collapse onto the
shared helper regardless of who tried. Whoever owns open_lock_file can decide
whether to grow it toward the acquire-later and shared-read shapes; that is a
separate item.

Refs #9267

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 25e0bde8fd292dfb47083a778f7c1b993cd1e4e8 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The helper yields a raw int fd, matching the migrated call sites that dropped .fileno(). The diff is exactly what the description claims: four with-scoped sites routed through the helper, three sites the helper's shape can't serve left inline with pointer comments, work_ledger deliberately deferred behind an open PR. The fix targets the root cause (no shared entry point) at the right layer (platform_compat owns the POSIX/Windows divergence), and notably resists the tempting over-reach of extending the helper to the acquire-later and shared-read shapes. The mutation proof shows the contract scan enforces the invariant going forward.

Design-Verdict: PASS

Root-cause consolidation at the right layer, with disciplined restraint about which shapes the helper actually serves — no design-level concerns.

[DESIGN-REVIEWED] 25e0bde

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 25e0bde8fd292dfb47083a778f7c1b993cd1e4e8 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 25e0bde

Verdict parsed from the review's SHA-scoped output markers for commit 25e0bde8fd292dfb47083a778f7c1b993cd1e4e8.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 25e0bde8fd292dfb47083a778f7c1b993cd1e4e8: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 25e0bde8fd292dfb47083a778f7c1b993cd1e4e8 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Composing the review.

First-Principles-Verdict: CONCERNS

"Seven open-sites" is the tree's lower bound, not its census: ~28 sibling lock-file "r+" opens remain, five of them the identical with-scoped exclusive shape.

Not justified as shipped

All inventory items are justified; the concern is the census the description rests on, not any shipped item — see Watch.

What this change ships

Inventory (7 items) — 7 justified

Intent: remove the duplicated non-truncating lock-open preamble by routing sites through the existing open_lock_file helper — a FIX (of duplication), per #9267.

1. Both session-PID lock opens now go through open_lock_file — justified
2. webhooks.locked lock open now goes through open_lock_file — justified
3. register_hook's hooks.json lock open now goes through open_lock_file — justified
4. Four per-site Windows rationale blocks replaced by one-line pointers to the helper docstring — justified
5. _McpFileLock/_McpFileLockSync gain kept-inline pointer comments — justified
6. _periodic_pid_sweep gains a kept-inline pointer comment — justified
7. Comment-history baseline totals lowered (6979→6977) — justified

Watch

• Point patch with counted unfixed siblings. The description claims "the 'four copies' in Consolidate the non-truncating lock-open preamble into one shared helper #9267 are really seven open-sites" and "the taxonomy is the finding" — but grep open\(.*"r\+" under src/kiro_crew returns 31 hits, ~28 of them lock-sidecar opens outside this PR: deploy/pending.py:114,138,159, deploy/profiles.py:131,228, skill_trust.py:444, apps/dependency_ledger.py:191, apps/bridges.py:1876, apps/builtins/mochi/activity_log.py:128, plus ~19 in issue_radar/backend/{store,crew_store}.py. The five deploy/* sites are the identical with-scoped exclusive shape and carry the same duplicated rationale comment ("Full rationale: work_ledger._open_lock") the PR names as the drift harm. Deferral is fine; an undercounted census presented as complete is not.
  Clears when: the five deploy/* sites are migrated here, or the deferred sibling list is stated with the real count.
• The stated reason the shared-read sweep cannot migrate — "the helper serves exclusive acquisition" — is wrong: open_lock_file only opens, non-truncating; lock mode is chosen by file_lock. The real barrier at that site is the fd outliving a with block. The wrong premise inflates "five of the seven sites were never going to collapse."
  Clears when: the sweep comment/description states the fd-lifetime reason instead of the exclusivity claim.

Subtractions

• Shrink the new _McpFileLockSync.__enter__ comment (dashboard/handlers/mcp.py:194-196) to the pointer alone — the "kept inline" rationale is already stated 50 lines above in _McpFileLock, restoring the exact two-copies-of-one-rationale pattern this PR exists to remove.

[FIRST-PRINCIPLES-REVIEWED] 25e0bde

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 25e0bde8fd292dfb47083a778f7c1b993cd1e4e8 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 25e0bde

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 25e0bde8fd292dfb47083a778f7c1b993cd1e4e8: <one-sentence reason>

[NicholasRBowers]

Tier 1 auto-approve: refactor (5 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: code-move-only refactor routing with-scoped lock opens through the existing platform_compat.open_lock_file helper, no behaviour change.