Skip to content

fix(factory): confirm a freshly-started CLI mount via daemon.pid (follow-up to #12)#13

Merged
khaliqgant merged 1 commit into
mainfrom
fix/factory-isvalidmountstate-daemon-pid
Jun 19, 2026
Merged

fix(factory): confirm a freshly-started CLI mount via daemon.pid (follow-up to #12)#13
khaliqgant merged 1 commit into
mainfrom
fix/factory-isvalidmountstate-daemon-pid

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Jun 19, 2026

Copy link
Copy Markdown
Member

Follow-up to #12.

Problem

#12 fixed checkMountStaleness to honor daemon.pid, but the other pid reader — waitForStateFileisValidMountState in local-mount-preflight.ts, which confirms a mount is ready right after the factory spawns it — still required a top-level pid. A CLI-daemonized mount (relayfile start --background) records its pid under daemon.pid, so the validator rejected the freshly-written state.json and timed out:

[factory] relayfile mount did not become ready within 10000ms (state file is malformed or for another workspace: …/.integrations/.relay/state.json)

The factory then warned writeback may not propagate even though the mount came up fine. Observed live driving the local E2E (relayfile 0.10.5, after relayfile#321 unblocked the cred path).

Fix

Export coercePid from relayfile-binary.ts and use coercePid(state.pid) ?? coercePid(state.daemon?.pid) in isValidMountState, matching checkMountStaleness from #12. The two pid readers now agree.

Tests

local-mount-preflight.test.ts: new case — a CLI mount that writes daemon: { pid } (no top-level pid) is confirmed ready (ensureLocalMount resolves). 526 tests green; build clean.

🤖 Generated with Claude Code


Summary by cubic

Accept daemon.pid during mount state validation so freshly started CLI mounts are confirmed immediately and don’t time out. Aligns the post-spawn readiness check with the staleness check to stop false factory warnings after relayfile start --background.

  • Bug Fixes
    • Exported coercePid and used coercePid(state.pid) ?? coercePid(state.daemon?.pid) in isValidMountState.
    • Added a test verifying a CLI mount that writes only daemon.pid is accepted.

Written for commit 56bffd9. Summary will update on new commits.

Review in cubic

Follow-up to #12. That PR fixed `checkMountStaleness` to honor `daemon.pid`,
but `waitForStateFile`/`isValidMountState` (the post-spawn readiness check) still
required a top-level `pid`. A CLI-daemonized mount (`relayfile start --background`)
records its pid under `daemon.pid`, so after the factory spawns the mount the
validator rejected the freshly-written state.json and timed out:
`relayfile mount did not become ready within 10000ms (state file is malformed…)`,
leaving the factory to warn "writeback may not propagate" even though the mount
came up fine.

Export `coercePid` and use `coercePid(state.pid) ?? coercePid(state.daemon?.pid)`
in `isValidMountState`, matching `checkMountStaleness`.

526 tests green; build clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d444446-8664-42e8-aafe-8c5a91380628

📥 Commits

Reviewing files that changed from the base of the PR and between 73f27fc and 56bffd9.

📒 Files selected for processing (3)
  • src/mount/local-mount-preflight.test.ts
  • src/mount/local-mount-preflight.ts
  • src/mount/relayfile-binary.ts

📝 Walkthrough

Walkthrough

coercePid is exported from relayfile-binary.ts. isValidMountState is updated to accept the PID from either pid or daemon.pid using coercePid. ensureLocalMount gains configurable stale-mount behavior: auto-refresh via spawnMount or warn-return when refreshStaleMount === false. A new test verifies readiness when state.json carries the PID under daemon.pid.

Changes

CLI daemon PID support and stale-mount refresh

Layer / File(s) Summary
Export coercePid and update PID validation
src/mount/relayfile-binary.ts, src/mount/local-mount-preflight.ts
coercePid is exported from relayfile-binary.ts and imported in local-mount-preflight.ts. isValidMountState now extracts the PID from state.pid or state.daemon.pid via coercePid, replacing the previous strict positive-integer check on pid only.
Stale-mount refresh logic and CLI daemon PID test
src/mount/local-mount-preflight.ts, src/mount/local-mount-preflight.test.ts
ensureLocalMount now checks refreshStaleMount: if false, it logs a warning and returns; otherwise it calls spawnMount, waits for the state file, and catches failures without throwing. A test exercises the path where state.json has daemon: { pid } and no top-level pid.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • AgentWorkforce/factory#12: Directly related — both PRs address CLI-daemonized mount readiness by reading daemon.pid and share the coercePid export change.

Poem

🐇 A daemon hides its pid deep inside,
Nested under daemon where it likes to hide.
I coercePid carefully, check every nest,
Stale mounts get refreshed — I handle the rest.
No errors thrown, just a warn and a bow,
The rabbit says mounts are all ready now! 🎉

🚥 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main fix: accepting daemon.pid in mount state validation for CLI mounts, with context that it's a follow-up to PR #12.
Description check ✅ Passed The description thoroughly explains the problem (CLI mounts use daemon.pid but validator only checks top-level pid), the fix (export coercePid and check both pid fields), and test coverage, directly addressing the changeset.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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/factory-isvalidmountstate-daemon-pid

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 and usage tips.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates the mount preflight check to support CLI-daemonized mounts, which store their process ID (PID) under 'daemon.pid' instead of a top-level 'pid' field in the state file. This is achieved by exporting and utilizing the 'coercePid' helper function in 'isValidMountState' to resolve the PID from either location. A corresponding unit test has been added to verify this behavior. There are no review comments, and I have no feedback to provide.

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.

@khaliqgant khaliqgant merged commit 735a958 into main Jun 19, 2026
3 checks passed
@khaliqgant khaliqgant deleted the fix/factory-isvalidmountstate-daemon-pid branch June 19, 2026 09:46
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