PRD-004: apiary fleet-root migration (ADR-0003) - #11
Merged
Conversation
Move doctor's state to the neutral ~/.apiary fleet root: shared resolveApiaryRoot chain (absolute-only env roots), registry relocation to ~/.apiary/registry.json with the two-location merge window and one-time migration, device.json relocation, per-product telemetry trust roots, legacy install-lock staleness check, and hermetic tests. Security audit clean (no Critical/High); QA passed with all warnings remediated (667 tests green). See the PRD qa/ report and the superproject execution ledger EXECUTION_LEDGER-apiary-state-root.md.
📝 WalkthroughWalkthroughThis PR introduces a neutral, absolute-path "apiary" fleet root ( ChangesFleet root migration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant runApiaryMigrations
participant migrateDoctorWorkspace
participant migrateRegistry
participant FleetRoot
CLI->>runApiaryMigrations: boot-time migration call
runApiaryMigrations->>migrateDoctorWorkspace: migrate legacy doctor files
migrateDoctorWorkspace->>FleetRoot: move/copy workspace files
runApiaryMigrations->>migrateRegistry: migrate legacy registry
migrateRegistry->>FleetRoot: copy registry.json, rename legacy to .migrated
migrateRegistry-->>runApiaryMigrations: non-throwing result
runApiaryMigrations-->>CLI: combined migration result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service/index.ts (1)
206-225: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCreate
p.stateDirbefore staging the launchd plist
fs.mkdirp(dirname(unitTarget))only covers~/Library/LaunchAgentshere, not<apiaryRoot>/doctor. Since the plist’sStandardOutPath/StandardErrorPathpoint intop.stateDir, addfs.mkdirp(p.stateDir)beforerenderUnit(p)so a fresh install can start cleanly on macOS.🤖 Prompt for 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. In `@src/service/index.ts` around lines 206 - 225, The unit-file staging in the write path for `renderUnit(p)` only creates the parent directory of `unitTarget`, so a fresh macOS install can still fail when `StandardOutPath` and `StandardErrorPath` point into `p.stateDir`. Update the `needsFile` block in `src/service/index.ts` to create `p.stateDir` itself before calling `fs.writeFile`, while keeping the existing `dirname(unitTarget)` creation for the plist target.
🧹 Nitpick comments (4)
tests/apiary-root.test.ts (1)
42-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for the win32 drive-relative absolute-path gotcha.
This suite proves Windows-style absolutes are honored on any platform, but doesn't cover the inverse: a POSIX-style value like
/srv/apiary(no drive letter) asAPIARY_HOMEwithplatform: "win32". Givenwin32.isAbsolutetreats such paths as absolute despite them being drive-relative on real Windows (see the companion comment onsrc/apiary-root.ts), a regression test pinning today's (accepted-as-absolute) behavior would make any future hardening ofisAbsoluteRootintentional and visible.🤖 Prompt for 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. In `@tests/apiary-root.test.ts` around lines 42 - 47, Add a regression test in the resolveApiaryRoot suite to cover the win32 drive-relative absolute-path case: when APIARY_HOME is a POSIX-style path like /srv/apiary and platform is "win32", assert the current accepted-as-absolute behavior is pinned. Use the existing resolveApiaryRoot helper alongside the other security cases so any future change to isAbsoluteRoot or its win32 handling becomes intentional and visible.src/cli/index.ts (1)
132-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated legacy-workspace-dir computation across entry points.
join(legacyHoneycombRoot(home), "doctor")(with the identical explanatory comment) is repeated verbatim insrc/compose/index.ts(Line 503) for the samecreateInstallLockcall. Extracting a single helper (e.g.legacyDoctorWorkspaceDir(home)inapiary-root.ts) would keep both entry points from drifting if the legacy layout changes.♻️ Proposed extraction
// src/apiary-root.ts export function legacyDoctorWorkspaceDir(home: string = homedir()): string { return join(legacyHoneycombRoot(home), "doctor"); }- legacyWorkspaceDir: join(legacyHoneycombRoot(home), "doctor"), + legacyWorkspaceDir: legacyDoctorWorkspaceDir(home),🤖 Prompt for 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. In `@src/cli/index.ts` around lines 132 - 142, The legacy doctor workspace path computation is duplicated in multiple entry points, so extract it into a shared helper and reuse it wherever createInstallLock is called. Add a dedicated helper like legacyDoctorWorkspaceDir in apiary-root.ts to encapsulate join(legacyHoneycombRoot(home), "doctor"), then update src/cli/index.ts and src/compose/index.ts to call that helper instead of repeating the path/comment logic.src/registry.ts (1)
505-562: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHarden
migrateRegistryagainst a silent overwrite race.
exists(newPath)is checked, thencopyFile(legacyPath, newPath)runs with no exclusivity guard. Two gaps:
- If
newPathis created between theexists()check and thecopyFile()call (e.g. a concurrent boot of another process running the same migration), the defaultcopyFileSyncsilently overwrites it with legacy content instead of failing.- If the
exists()probe itself throws (caught and treated as "fall through"), the code proceeds to copy even thoughnewPathmight genuinely already exist, again risking silent clobber.Since
copyFileSyncoverwrites the destination by default, usingCOPYFILE_EXCLturns this race into a safe, already-handled"copy-failed"/"new-present"outcome instead of silent data loss.🛡️ Proposed fix using COPYFILE_EXCL
- const copyFile = seams.copyFile ?? copyFileSync; + const copyFile = + seams.copyFile ?? + ((src: string, dst: string): void => { + copyFileSync(src, dst, constants.COPYFILE_EXCL); + }); const newPath = defaultRegistryPath(home, env, platform); const legacyPath = legacyRegistryPath(home); ... try { makeDir(dirname(newPath)); copyFile(legacyPath, newPath); } catch { + // EEXIST here means a concurrent writer won the race: treat exactly like the + // pre-check "new-present" idempotent no-op rather than losing the race silently. return { migrated: false, reason: "copy-failed" }; }(add
constantsto thenode:fsimport)🤖 Prompt for 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. In `@src/registry.ts` around lines 505 - 562, migrateRegistry has a race where copyFileSync can silently overwrite an already-created new registry file after the exists(newPath) check. Update the migrateRegistry flow to use an exclusive copy via COPYFILE_EXCL (and add constants to the node:fs import) so concurrent boots or a failed exists probe resolve to the existing copy-failed/new-present handling instead of clobbering data; the key symbols to adjust are migrateRegistry, copyFile, and newPath.src/apiary-migration.ts (1)
187-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigration outcomes (
reason,legacyRenamed,failed) are never logged.Both
WorkspaceMigrationResult.reasonandRegistryMigrationResult.reasonare documented as an "audit trail" (Lines 70-71 in this file; similarly commented inregistry.ts), but only theworkspace-overriddenbranch insidemigrateDoctorWorkspaceactually callslogger.info(...). Every other outcome —migrated,no-legacy,legacy-unreadable,error, and the entire registry migration result — is silently discarded here, and the referenced caller (runWatchdoginsrc/cli/index.ts) doesn't consume the return value either. If a production migration silently fails (e.g.copy-failed,legacy-malformed), there will be no trace in the logs to explain why the fleet is still reading from the legacy location.♻️ Proposed fix: log a summary before returning
export function runApiaryMigrations(options: ApiaryMigrationOptions = {}): ApiaryMigrationsResult { + const logger = options.logger ?? silentLogger; const workspace = migrateDoctorWorkspace(options); let registry: RegistryMigrationResult; try { registry = migrateRegistry(options); } catch { // migrateRegistry is itself total; this is defense in depth so boot never throws. registry = { migrated: false, reason: "copy-failed" }; } + logger.info("apiary_migration.summary", { + workspaceReason: workspace.reason, + workspaceFailed: workspace.failed, + registryReason: registry.reason, + }); return { workspace, registry }; }🤖 Prompt for 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. In `@src/apiary-migration.ts` around lines 187 - 203, The migration results from runApiaryMigrations are returned silently, so the audit-trail fields like reason, legacyRenamed, and failed never reach logs. Update runApiaryMigrations to log a concise summary of both migrateDoctorWorkspace and migrateRegistry outcomes before returning, using the existing logger path and the result types WorkspaceMigrationResult and RegistryMigrationResult. Ensure every outcome branch (including no-legacy, legacy-unreadable, error, copy-failed, and migrated) is recorded, not just the workspace-overridden case.
🤖 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
`@library/knowledge/private/architecture/ADR-0003-fleet-directory-ownership-and-neutral-state-root.md`:
- Around line 21-28: The pseudocode signature in the ADR is out of sync with the
mirrored implementation, so update the canonical description to match the actual
parameter order used by resolveApiaryRoot(env, home, platform). Keep the
branching logic the same, but rewrite the resolveFleetRoot header and
surrounding wording so the spec reflects the byte-for-byte mirror and does not
imply a different argument order. Use resolveFleetRoot and resolveApiaryRoot as
the identifying symbols when locating the text.
In `@src/apiary-root.ts`:
- Around line 57-67: The root validation in isAbsoluteRoot currently uses
win32.isAbsolute, which allows POSIX-shaped paths like /srv/apiary on Windows;
tighten this check so only Windows-style absolute roots are accepted. Update
isAbsoluteRoot to require a drive-letter or UNC root explicitly, and keep the
helper as the single place used by the env-root resolution logic in
src/apiary-root.ts.
In `@src/compose/index.ts`:
- Around line 293-330: The explicit registryPath branch in resolveDaemons is
bypassing the injected environment and platform and instead letting
readRegistryFile use process defaults, which breaks hermetic behavior. Update
the options.registryPath path to pass the same env and platform values used by
resolveRegistryEntries so both code paths resolve registry defaults
consistently; use resolveDaemons and readRegistryFile as the key symbols to
locate the change.
---
Outside diff comments:
In `@src/service/index.ts`:
- Around line 206-225: The unit-file staging in the write path for
`renderUnit(p)` only creates the parent directory of `unitTarget`, so a fresh
macOS install can still fail when `StandardOutPath` and `StandardErrorPath`
point into `p.stateDir`. Update the `needsFile` block in `src/service/index.ts`
to create `p.stateDir` itself before calling `fs.writeFile`, while keeping the
existing `dirname(unitTarget)` creation for the plist target.
---
Nitpick comments:
In `@src/apiary-migration.ts`:
- Around line 187-203: The migration results from runApiaryMigrations are
returned silently, so the audit-trail fields like reason, legacyRenamed, and
failed never reach logs. Update runApiaryMigrations to log a concise summary of
both migrateDoctorWorkspace and migrateRegistry outcomes before returning, using
the existing logger path and the result types WorkspaceMigrationResult and
RegistryMigrationResult. Ensure every outcome branch (including no-legacy,
legacy-unreadable, error, copy-failed, and migrated) is recorded, not just the
workspace-overridden case.
In `@src/cli/index.ts`:
- Around line 132-142: The legacy doctor workspace path computation is
duplicated in multiple entry points, so extract it into a shared helper and
reuse it wherever createInstallLock is called. Add a dedicated helper like
legacyDoctorWorkspaceDir in apiary-root.ts to encapsulate
join(legacyHoneycombRoot(home), "doctor"), then update src/cli/index.ts and
src/compose/index.ts to call that helper instead of repeating the path/comment
logic.
In `@src/registry.ts`:
- Around line 505-562: migrateRegistry has a race where copyFileSync can
silently overwrite an already-created new registry file after the
exists(newPath) check. Update the migrateRegistry flow to use an exclusive copy
via COPYFILE_EXCL (and add constants to the node:fs import) so concurrent boots
or a failed exists probe resolve to the existing copy-failed/new-present
handling instead of clobbering data; the key symbols to adjust are
migrateRegistry, copyFile, and newPath.
In `@tests/apiary-root.test.ts`:
- Around line 42-47: Add a regression test in the resolveApiaryRoot suite to
cover the win32 drive-relative absolute-path case: when APIARY_HOME is a
POSIX-style path like /srv/apiary and platform is "win32", assert the current
accepted-as-absolute behavior is pinned. Use the existing resolveApiaryRoot
helper alongside the other security cases so any future change to isAbsoluteRoot
or its win32 handling becomes intentional and visible.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e50aa91-1b74-452e-9bd8-00ec1db3ecf6
📒 Files selected for processing (33)
library/knowledge/private/architecture/ADR-0003-fleet-directory-ownership-and-neutral-state-root.mdlibrary/requirements/backlog/prd-004-apiary-fleet-root-migration/qa/2026-07-04-qa-report.mdsrc/apiary-migration.tssrc/apiary-root.tssrc/cli/index.tssrc/compose/index.tssrc/config.tssrc/device-id.tssrc/install-lock.tssrc/registry.tssrc/service/index.tssrc/service/platform.tssrc/service/templates.tssrc/telemetry/capture.tstests/apiary-registry.test.tstests/apiary-root.test.tstests/apiary-supervision.test.tstests/apiary-workspace-migration.test.tstests/cli/run-watchdog.test.tstests/cli/status-version-wiring.test.tstests/compose/create-doctor.test.tstests/compose/multi-daemon.test.tstests/compose/telemetry-wiring.test.tstests/config.test.tstests/device-id.test.tstests/install-lock.test.tstests/registry.test.tstests/service/helpers.tstests/service/service-module.test.tstests/service/templates.test.tstests/setup.tstests/telemetry/capture.test.tsvitest.config.ts
This was referenced Jul 8, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
~/.apiary/root with per-product subdirs; doctor manages the shared coordination surface.resolveApiaryRootchain (APIARY_HOME> Linux$XDG_STATE_HOME/apiary><home>/.apiary), env roots honored only when ABSOLUTE (security amendment), neverprocess.cwd().~/.apiary/registry.json: one-time idempotent migration (legacy renamed.migrated), two-location merge window (new wins per daemon name, legacy-only merges additively, no write-back), fail-loud preserved at both locations.device.jsonrelocation with copy-on-read fallback; install-id new-first read.~/.apiary/<name>/telemetryplus the legacy root; entry names filename-safe-validated; traversal/UNC/drive-relative shapes rejected (test-proven on win32).DOCTOR_WORKSPACE_DIR; malformed-registry diagnosis names the actually-broken file.Close-out (superproject ledger: library/ledger/EXECUTION_LEDGER-apiary-state-root.md)
library/requirements/backlog/prd-004-apiary-fleet-root-migration/qa/2026-07-04-qa-report.md.npm test60 files / 667 tests green (natural run, hermetic home).Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests