chore(critique,reconcile): share hardened validators and the manifest lock — sc-1414 clusters 3+5 - #314
Conversation
… lock — sc-1414 clusters 3+5 Cluster 3 (critique): validText (bounded, non-blank, control-char-rejecting text guard) was byte-identical in evidence-bindings and lifecycle/ work-quarantine; the hardened plain-record guard (proxy/array/prototype rejection) was duplicated between work-quarantine's exactObject interior and provider-adapters/claude-subagent-stop's plainRecord. Both now live in evidence-record.mts — the one module all three files already import — exported as validText (+ PLAN_CRITIQUE_MAX_TEXT_BYTES) and plainRecord; work-quarantine's exactObject keeps its stricter exact-fields/descriptor checks on top. NOT touched: evidence-bindings' softer exactObject (different security posture by design, and never clone-flagged). Cluster 5 (reconcile): the atomic-mkdir withLock mutex (identical bodies AND constants) moves from reconcile.mts + ship/reconcile-manifest-write.mts into atomic-write.mts — whose own docstring already names both callers as the mutators it serves. The writer's fuller rationale comment travels with it. Cluster 4 (decisions check-alignment <-> detect) is DEFERRED with the eval- harness rationale: the shared fragment is the standard run-as-main gate dispatch idiom, and both files feed published decisions-suite hashes. Repo-wide clone scan 52 -> 49. Targeted suites green (critique 235; ship/ release 322 incl. reconcile paths); tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a shared CLI directory-lock helper and replaces duplicate lock implementations. It also centralizes bounded text and plain-record validation for critique evidence and updates consumers to use these helpers. ChangesCLI locking
Critique validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/lib/atomic-write.mts`:
- Around line 44-45: Replace the age-only cleanup in the atomic-write lock flow
with ownership-safe locking and stale-holder detection, or require explicit
cleanup; never remove lockDir solely because mtime exceeds LOCK_STALE_MS. Ensure
the mechanism prevents both paused live holders and release/reacquire races from
deleting another process’s lock, while preserving safe manifest updates in the
callers using atomic-write.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c367505-65ff-44c0-a57d-b55f4c573eb0
📒 Files selected for processing (7)
cli/lib/atomic-write.mtscli/lib/reconcile.mtscli/lib/ship/reconcile-manifest-write.mtsgate-engine/critique/evidence-bindings.mtsgate-engine/critique/evidence-record.mtsgate-engine/critique/lifecycle/work-quarantine.mtsgate-engine/critique/provider-adapters/claude-subagent-stop.mts
Review follow-up on the withLock this PR moved into atomic-write.mts. The reaping rule travelled over verbatim from both former copies, and it treated lock AGE as proof the holder had died. It is not: a live writer paused past the window (SIGSTOP, a suspended laptop, a long GC) still owns its lock, so the reap let a second read-modify-write run concurrently with the first -- exactly the lost update the mutex exists to prevent. A reap now needs three agreeing facts instead of one: stale age, a pid that no longer exists, and a holder stamp unchanged across the check. The holder writes `<pid>:<uuid>` inside the lock dir on acquisition, so ownership is read rather than inferred. The stamp re-read is what defeats the release-and-reacquire race -- a holder that released while a second process was mid-check no longer has its successor's fresh lock deleted by age. Every ambiguous liveness answer (EPERM, an unreadable stamp) errs toward not reaping, and refusing to reap is always safe: the caller just times out. Release is now ownership-checked too. The old unconditional rmSync in the finally would strip a live holder's lock had this caller itself been wrongly reaped, admitting a third writer -- the same defect on the way out. Also replaced the sleepless retry spin with a 25ms pause: burning a core for 5s while the holder tries to finish its sub-ms section was making the contention it was waiting on worse. Residual, documented at the call: a reacquire landing between the stamp re-read and the rmSync still races, but it needs a third party to reap the dead holder's lock inside that window. Every wider path is closed. New suite atomic-write-lock.test.mts (7 cases) drives the real filesystem: stale+dead reaps, stale+alive does NOT, fresh+dead does NOT, stale+unstamped reaps, and release leaves a lock that is no longer ours alone. Targeted suites green (reconcile, ship-manifest, detect-merged, the new lock suite -- 61 tests); tsc clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Shortcut sc-1414 part 2, clusters 3+5 (epic 1399).
Cluster 3 — critique
validText(bounded, control-char-rejecting text guard; byte-identical ×2) and the hardened plain-record guard (proxy/array/prototype rejection, duplicated betweenwork-quarantine's exactObject interior andclaude-subagent-stop's plainRecord) now live inevidence-record.mts— the module all three files already import.work-quarantine's exactObject keeps its stricter exact-fields/descriptor checks layered on top. NOT touched:evidence-bindings' softer exactObject (different security posture, never clone-flagged).Cluster 5 — reconcile
The atomic-mkdir
withLockmutex (identical bodies AND constants) moves fromreconcile.mts+ship/reconcile-manifest-write.mtsintoatomic-write.mts— whose docstring already names both callers. The writer's fuller rationale comment travels with it.Cluster 4 — deferred with rationale
decisions/check-alignment ↔ detectshare the standard run-as-main gate dispatch idiom, and both feed published decisions-suite hashes — same deferral class as the eval harness (dedupe when they next change substantively).Scan 52 → 49. Targeted suites green (critique 235 tests; ship/release paths); full suite green at pre-push; tsc clean. Net −35 lines.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor