Skip to content

fix(review): preserve exact candidate owner filesystem identity - #1739

Merged
decode2 merged 1 commit into
mainfrom
fix/candidate-owner-file-identity
Oct 4, 2026
Merged

decode2 merged 1 commit into
mainfrom
fix/candidate-owner-file-identity

Conversation

@decode2

@decode2 decode2 commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

Linked issue

Refs #343 (approved cleanup tracker; remains closed). Follow-up to merged #672. Prerequisite for coordination stack #1714 through #1738.

PR type

  • Bug fix (type:bug)

Summary

  • Use exact bigint dev/ino and metadata from a single lstat observation in file/directory ownership checks.
  • Prevent rollback deleting replacement markers and cleanup reaching Git removal for a replaced root when64-bit IDs collide as Number.
  • Preserve all privacy/ACL/path/registration/PID gates, markerJSON and identity-string API.

Changes

Path Change
lib/review-candidate-view-owner.ts 8A/6D exact identity and equivalent bigint guards
tests/review-candidate-view.test.ts 88-line marker/directory regression
odd/tasks/candidate-owner-file-identity.md 30-line root/evidence tracker

Test plan

  • Current main a87192e modeled RED:1pass/2intendedfail; GREEN3/3.
  • Candidate+repository suites173total:165pass,8platformskips,0fail.
  • Independent read-only verifier reran same suites and inspected132-line stagedcandidate; no blocking findings.
  • Types186baseline diagnostics/no regressions;8runtime modulesmatch;whitespace clean.
  • Existing negative privacy/UID/symlink/path/registration/death-proof controls unchanged and pass.
  • Native Windows CI pending; actual historical NTFS ids unlogged. Inferred historical cause, deterministic exact collision reproduced.

Root

Two distinct values2^53 and2^53+1 round to the same Number. Use bigint from the SAME filesystem observation, not a second lookup. No new state/retry/permission weakening. Original Windowsfailure37159261739/job111309064512 markerabsent matches the modeled symptom; not claimed actualNTFSmeasurement.

Contributor checklist

  • Approved tracker linked nonclosing, exactlyone type label.
  • Tests/task/conventionalcommit,no attribution trailers.
  • Shell/skill checks not applicable.
  • Remote CI pending.

132A+D. No main merge/auto-merge/reload. Integrate this prerequisite first, then coordination chain in order with freshCI. Feature-only ancestor merges carry the fix/currentmain forward without inflating existing slices.

Summary by CodeRabbit

  • Bug Fixes
    • Improved file and directory identity checks for metadata values larger than JavaScript can represent precisely.
    • When an item is replaced during a failed publication or cleanup, checks now better distinguish the replacement from the original, helping preserve both and avoid unintended cleanup actions.

@decode2 decode2 added the type:bug Bug fix label Oct 4, 2026
@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 3197d592-54e5-456a-b74f-8c206230a958
📥 Commits

Reviewing files that changed from the base of the PR and between a87192e and 89736a4.

📒 Files selected for processing (3)
  • lib/review-candidate-view-owner.ts
  • odd/tasks/candidate-owner-file-identity.md
  • tests/review-candidate-view.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Directory and file owner checks now compare filesystem metadata as bigints. Regression tests cover replacement identities that round to the same Number during publication and cleanup.

Changes

Candidate owner identity checks

Layer / File(s) Summary
Use bigint metadata for owner checks
lib/review-candidate-view-owner.ts
directory and regular use bigint metadata for ownership, permissions, and file safety checks. Existing rejection conditions remain unchanged.
Test replacement identity handling
tests/review-candidate-view.test.ts, odd/tasks/candidate-owner-file-identity.md
Regression tests check that publication and cleanup preserve replacement files and directories when identities differ but round to the same Number. The tracker records verification results and pending checks.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: alan-thegentleman

Merge Risk: ⚪ Minimal · up to 89736

No identified issue blocks merging after normal checks. Native Windows verification is still pending.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 89736

The change strengthens ownership checks without expanding cleanup privileges or changing the saved owner format. No introduced security concern was established. Native Windows behavior and interruption recovery remain incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant destructive authority is local candidate worktree removal and adjacent marker/lock cleanup within the validated repository candidate parent. Interference by replacing those resources now encounters exact identity comparisons; the inspected change does not add callers or broader deletion authority.

Trust Boundaries and Controls

  • observed — A supplied path or serialized marker is not sufficient authority for deletion. Parent confinement, marker schema and owner equality, process-liveness or death proof, exclusive locking, Git registration, and repeated resource-identity checks remain required before cleanup reaches Git removal.

Resilience and Maintainability Implications

  • inferred — Failure paths conservatively preserve resources whose ownership cannot be proven, and stale-owner sweeping provides bounded recovery for eligible dead owners. Repeated checks do not make filesystem checks and subsequent actions atomic; that limitation predates this PR and was not shown to gain exposure through it. Direct interruption/restart execution was not verified.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preserving exact filesystem identity for review candidates.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@decode2
decode2 merged commit cc87347 into main Oct 4, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant