Skip to content

fix(agent-sync): reconcile native publication outcomes - #2590

Merged
namastex888 merged 1 commit into
devfrom
fix/routing-delivery-review-followups
Jul 14, 2026
Merged

namastex888 merged 1 commit into
devfrom
fix/routing-delivery-review-followups

Conversation

@namastex888

@namastex888 namastex888 commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

Context

Post-merge closure for review findings discovered while #2588 was still being audited. This branch is rebased onto current dev, including #2588 and the adjacent #2589 lock-quarantine repair.

Fixes

  • Centralize Darwin renamex_np and Linux renameat2 regular-file publication behind one exact-outcome reconciler.
    • Native capability setup unavailable: use the verified portable hard-link transaction.
    • Native nonzero result: preserve the no-clobber failure.
    • Exception before the syscall commits: fail closed with no portable retry.
    • Exception after the syscall commits: recognize the commit only when the staged name is absent and the target has the exact staged inode identity and bytes with a stable re-stat.
    • Injected Linux capability probes do not reuse the process-global memoized probe.
  • Let missing-home publication select its dedicated portable mkdir commit when Darwin FFI setup is unavailable.
  • Remove both post-release pathname-only empty-home deletions from full uninstall. Identity-bound lock cleanup remains authoritative, so a foreign GENIE_HOME replacement is never deleted.
  • Add deterministic Darwin/Linux pre- and post-syscall regressions plus the full-uninstall foreign-home swap regression.

Validation

  • bun test src/lib/agent-sync.test.ts -t 'claude agent fan-out|cross-process sync lock' — 64 pass
  • bun test src/genie-commands/uninstall.test.ts — 74 pass
  • bun run check:fast — pass
  • git diff --check origin/dev...HEAD — pass
  • Independent final review — SHIP, no CRITICAL/HIGH gaps

Scope note

The pre-existing shared publishDirectoryViaNameClaim claim-to-rename advisory remains outside this regular-file and dedicated lock-home repair. Neither portable path changed here reaches that primitive.

Summary by CodeRabbit

  • Bug Fixes

    • Improved uninstall behavior when the installation directory changes during removal, preserving lock files and existing state backups.
    • Prevented sync updates from overwriting existing agent manifests or installation directories.
    • Improved recovery after interrupted or partially completed publication operations.
    • Added safer platform-specific handling, including portable fallbacks when native operations are unavailable and clear failures when they cannot be completed safely.
  • Tests

    • Expanded coverage for uninstall and cross-platform synchronization scenarios.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change makes native no-clobber publication injectable and failure-aware across manifests and GENIE_HOME creation. Uninstall cleanup now uses lock-preserving install-state removal instead of empty-directory deletion, with tests covering native failures and swapped-in GENIE_HOME directories.

Changes

Agent sync publication and uninstall

Layer / File(s) Summary
Native capability contracts and resolution
src/lib/agent-sync.ts
Publication options now accept injected no-clobber dependencies, with platform-specific Linux and Darwin capability resolution.
Manifest no-clobber publication
src/lib/agent-sync.ts, src/lib/agent-sync.test.ts
Manifest publishing wires injected dependencies into atomic publication, validates staged content, reconciles native exceptions, and tests fallback and fail-closed outcomes.
GENIE_HOME publication behavior
src/lib/agent-sync.ts, src/lib/agent-sync.test.ts
GENIE_HOME creation selects native or portable publication and tests Darwin fallback and native failure behavior.
Lock-aware GENIE_HOME cleanup
src/genie-commands/uninstall.ts, src/genie-commands/uninstall.test.ts
Uninstall removes eligible state through the preserving cleanup path, eliminates empty-directory removal, and tests a swapped-in GENIE_HOME while the sync lock is held.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: reconciling native publication outcomes in agent-sync.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/routing-delivery-review-followups

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.

@namastex888

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 14, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces native Darwin support for atomic, no-clobber file and directory publishing using renamex_np via dlopen, alongside comprehensive test coverage and dependency injection seams for testing native platform fallbacks. It also removes the automatic cleanup of empty GENIE_HOME directories during uninstallation. The feedback highlights a critical issue where closing the libc handle inside the returned Darwin rename function makes it single-use, which could lead to failures if the function is invoked multiple times.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/lib/agent-sync.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@src/lib/agent-sync.ts`:
- Around line 6246-6270: Extract the shared staged-to-target publishing closure
from the Darwin and Linux branches into a helper such as
makeAgentSyncHomePublisher, accepting the resolved rename function and
preserving the existing lstatSafe detail selection and NoClobberPublishError
message. Have both branches return this helper after resolving their
platform-specific renamer, leaving platform detection and null handling
unchanged.
- Around line 6240-6271: Update resolveNativeAgentSyncHomePublisher to use the
shared platform value from options.homePublishDeps.platform instead of reading
process.platform directly, while preserving the existing Darwin and Linux branch
behavior and fallback handling.
- Around line 2134-2184: Cache the result of the Darwin opener capability probe
used by resolveDarwinRenameExclusive, so defaultDarwinRenameOpener does not
perform dlopen/dlclose for every publish call. Memoize it once per process or
sync run while preserving dependency-injected darwinOpener behavior and the
existing portable fallback when FFI setup is unavailable.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c05dc20b-08c9-447b-993a-a0f837c62f92

📥 Commits

Reviewing files that changed from the base of the PR and between e594af9 and 0ef3727.

📒 Files selected for processing (4)
  • src/genie-commands/uninstall.test.ts
  • src/genie-commands/uninstall.ts
  • src/lib/agent-sync.test.ts
  • src/lib/agent-sync.ts
💤 Files with no reviewable changes (1)
  • src/genie-commands/uninstall.ts

Comment thread src/lib/agent-sync.ts
Comment thread src/lib/agent-sync.ts
Comment thread src/lib/agent-sync.ts
@namastex888
namastex888 merged commit 9a2c91b into dev Jul 14, 2026
17 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant