Repository navigation
Conversation
|
Sentinel 已审核此变更,未发现已核验问题,但发现需要人工确认的风险。 另有 2 个需要人工确认的风险;黄色 advisory 不阻塞合并,建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情: 调用 MCP 工具 由 Sentinel 审核 · 目标提交 05d14ec00aa9 |
|
Sentinel 保留了 2 个尚未确认的审核候选。这些黄色条目不阻塞合并;红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时,可用行末 由 Sentinel 审核 · 目标提交 05d14ec00aa9 |
|
Sentinel 已审核此变更,未发现已核验问题,但发现需要人工确认的风险。 另有 1 个需要人工确认的风险;黄色 advisory 不阻塞合并,建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情: 调用 MCP 工具 由 Sentinel 审核 · 目标提交 71f98f860e4f |
|
Sentinel 保留了 1 个尚未确认的审核候选。这些黄色条目不阻塞合并;红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时,可用行末 由 Sentinel 审核 · 目标提交 71f98f860e4f |
forrestlinfeng
left a comment
There was a problem hiding this comment.
Code review — changes requested
The admission protocol itself is sound (guards are published already holding an owner file, and only the inspected owner is unlinked). Two recovery holes remain.
Major
- A stranded admission guard never heals —
lib/runtime/store/file-lock.jsreclaimAdmission(~L195-204) only clears a guard whose owner PID is gone; there is no age fallback.- PID reuse (common on Windows): a holder is killed, its PID is reused → every later
withLockon that lock returnsfalseforever (outbox drain, engine setup, hub login stop silently). Reproduced with a day-old guard owned by PID 1. - Own PID: if
releaseAdmissionthrows after its retries (e.g. AV holds the owner file), the guard carriesprocess.pid, whichownerIsGonenever reports as gone. The new test attest/runtime/file-lock.test.js:254-270asserts this left-behind state. - Fix: admission is held for a few synchronous ms, so treat a guard older than ~10 s as abandoned regardless of PID. Unlinking only the inspected owner name keeps that race-safe.
- PID reuse (common on Windows): a holder is killed, its PID is reused → every later
- Failed admission release orphans the work lock —
tryLockcreateslockDir+ owner insideadmitted; ifreleaseAdmissionthrows infinally, the handle is lost but the directory stays, owned by a live PID, and the new "live PID is never evicted" rule means it is never reclaimed while this process lives. Remove the work lock before rethrowing (or don't throw and let the guard age out).
Minor
refreshtimer is now mostly dead: new tokens carry a PID, live owners are never evicted, dead ones can't refresh, so the mtime lease matters only for legacy tokens — while each refresh still takes admission and makes contenders see "busy". Delete it or scope it to legacy tokens.await lock.release()infinallymasks the work's own error if release also throws.admissionOfreads like a getter but creates and renames directories — e.g.enterAdmission.- Crash-leftover
*.claimstaging dirs are never swept. ['EPERM','EBUSY','EACCES']appears twice — make it one constant.file-lock.jsgrows to ~340 LOC and now holds state files, state lines, work locks and admission; extracting admission into its own module would bring it back under 300 and give one place for the liveness rule.
Tests: deterministic (4/4 clean runs), but the spawn/marker/wait harness is copy-pasted twice here and a third time in #29 — please extract a helper.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Sentinel 已审核此变更,未发现已核验问题,但发现需要人工确认的风险。 另有 4 个需要人工确认的风险;黄色 advisory 不阻塞合并,建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情: 调用 MCP 工具 由 Sentinel 审核 · 目标提交 a638041595b4 |
|
Sentinel 保留了 4 个尚未确认的审核候选。这些黄色条目不阻塞合并;红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时,可用行末 由 Sentinel 审核 · 目标提交 a638041595b4 |
forrestlinfeng
left a comment
There was a problem hiding this comment.
Round 2 — not yet
The process-start-time ("birth") check is a good approach and I agree it is race-safe: a slow live holder keeps its guard because its start time matches, and the 10 s age only gates when the check runs, so clock skew / GC pauses can't cause a steal. The orphaned work-lock cleanup is also fixed and race-safe. 17/17 tests pass over 3 runs.
Major — a subprocess spawn on every process's first lock
lib/runtime/store/file-lock.js:244:ownBirth = processBirth(process.pid)runsps(Unix) orpowershell.exe(Windows, 2.5 s timeout) the first time any process takes admission. Every hook invocation is a fresh process that callsupdateState, so every hook pays this. Measured firstupdateState≈0.9 ms on main vs ≈4.1 ms here (macOS); PowerShell cold start on Windows is typically hundreds of ms per hook event.reclaimAdmission(~L227) also spawns on each 20 ms retry, soacquirecan overrun its 2 s deadline.- Fix: stamp our own start time without spawning (
Date.now() - process.uptime() * 1000), spawn only on the reclaim path for a foreign PID, and compare with a tolerance (e.g. ±2 s) instead of hashing the exactps/PowerShell string. The start-time lookup belongs inlib/runtime/process/owner.js, notfile-lock.js.
Still open from round 1
file-lock.jsgrew from 340 to 383 LOC (soft limit 300). Please extract admission into its own module — it now has its own liveness rule, identity check and abandoned-guard map.['EPERM','EBUSY','EACCES']still duplicated;await lock.release()infinallystill masks the work's error;refreshtimer still mostly dead;admissionOfnaming.- Test harness (
until, spawn/marker helpers) still copied betweenfile-lock.test.jsandstate-lock.test.js.
Minor
- No protection when
ps/PowerShell is missing or blocked (slim containers, busybox, AppLocker) — falls back to round-1 behaviour. Fine if documented. - If the cleanup's own read fails (
ownsLockthrows, ~L369), that error replacesELOCKEDand the work lock is orphaned again. - The new test only exercises the own-PID branch and fails if
psis unavailable.
|
Sentinel 已审核此变更,未发现已核验问题,但发现需要人工确认的风险。 另有 1 个需要人工确认的风险;黄色 advisory 不阻塞合并,建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情: 调用 MCP 工具 由 Sentinel 审核 · 目标提交 48a7f8312b0c |
|
Sentinel 保留了 1 个尚未确认的审核候选。这些黄色条目不阻塞合并;红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时,可用行末 由 Sentinel 审核 · 目标提交 48a7f8312b0c |
|
Sentinel 已审核此变更,未发现已核验问题,但发现需要人工确认的风险。 另有 7 个需要人工确认的风险;黄色 advisory 不阻塞合并,建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情: 调用 MCP 工具 由 Sentinel 审核 · 目标提交 d1fc44ffe2c5 |
|
Sentinel 保留了 7 个尚未确认的审核候选。这些黄色条目不阻塞合并;红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时,可用行末 由 Sentinel 审核 · 目标提交 d1fc44ffe2c5 |
|
Sentinel 已审核此变更,未发现已核验问题,但发现需要人工确认的风险。 另有 5 个需要人工确认的风险;黄色 advisory 不阻塞合并,建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情: 调用 MCP 工具 由 Sentinel 审核 · 目标提交 a0e143e6362f |
|
Sentinel 保留了 5 个尚未确认的审核候选。这些黄色条目不阻塞合并;红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时,可用行末 由 Sentinel 审核 · 目标提交 a0e143e6362f |
|
Sentinel 已审核此变更,未发现已核验问题,但发现需要人工确认的风险。 另有 11 个需要人工确认的风险;黄色 advisory 不阻塞合并,建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情: 调用 MCP 工具 由 Sentinel 审核 · 目标提交 0490c99a6062 |
|
Sentinel 保留了 11 个尚未确认的审核候选。这些黄色条目不阻塞合并;红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时,可用行末 由 Sentinel 审核 · 目标提交 0490c99a6062 |
Summary
Two reclaimers can inspect an expired worker lock, then one deletes the other's fresh lock and both enter the protected work. An expired mtime can also evict a paused but living owner.
Serialize directory mutations with a short admission protocol. Prepopulated publication and owner-specific unlinking also prevent a dead-admission reclaimer from removing a fresh guard. Known live-PID owners keep their locks; dead and legacy locks retain lease recovery. Cleanup retries transient sharing denial; release contention is bounded and fails closed.
Evidence
Before: deterministic real A/B/C-process probes enter two callbacks concurrently. A first proposed file-guard solution also failed a separate dead-reclaimer ABA probe and was discarded.
After: both interleavings admit one owner. Final runtime tests 14/14 passed on Windows Node 22 and Node 24; all eight hosts pack. Tests cover live/dead/legacy owners, transient cleanup refusal, bounded contention and preservation of unknown guards.
BEFORE the final cleanup-retry change: Linux Node 26 full 652/652 and pack passed. Linux Node 22 full twice had 651 pass / 1 failure, the first OpenCode session-end distillation exceeding its existing 10s wait; isolated OpenCode 8/8 and exact main baseline 644/644 passed. Causal attribution remains unresolved; no test, timeout or workload was relaxed. CI must validate the final commit separately.
Final follow-up head 71f98f8 distinguishes missing owner metadata (ENOENT, recovery retained) from unreadable metadata (refuse takeover). Release retries transient owner reads and reports persistent ELOCKED; whole refresh is best effort for timer callers while final release remains strict. New RED→GREEN regressions bring final runtime checks to 21/21 on Windows Node 22; packing passed. Earlier full-matrix results above predate this follow-up; final CI reruns separately.
Final head 71f98f8 passed GitHub CI run 75. External combined PR19–29 validation passed full 757/757 and packing all nine hosts on Linux Node 22 and 26. That copy includes feat: add native MiniMax Code host support #19's existing OpenCode fixture Connection: close calibration; this does not retroactively replace the earlier standalone failed runs.
Final combined validation (immutable source SHA256
1333f0491d643b2fed2188e6c553d0007cf9bdfd88db0d4235c9f50a2e13b610): Linux Node 22/26 full 757/757 each, pack nine hosts; Windows Node 26 scoped 394 passed / 9 existing skips, pack nine hosts. Windows Node 22 scoped 393 passed / 1 after-hook cleanup failure / 1 cancellation / 9 skips; packing was not run after that failure. The MiniMax body saved its draft and settled its candidate, and report delivery later settled, but cleanup exceeded its unchanged 20s wait. Cause remains unclassified; no rerun, assertion or timeout relaxation. All 388 source files kept their hashes after all four cells. This is a local acceptance gap, separate from each PR's successful CI.Merge Danger
Door: two-way
New owner tokens carry the PID. Legacy fresh locks remain respected; stale legacy locks retain their existing recovery. Old processes do not participate in the new admission protocol, so restart hosts after an update.
Blast Radius: concurrency
Protects outbox and engine work shared by hosts. PID reuse fails closed. Permanent cleanup denial can require repair/restart; crash-only private staging directories can remain. No process is terminated and no dependency is added.