fix(fm): restore fork behavior clobbered by upstream reconcile merge - #113
Conversation
The upstream reconcile merge (f7b7cff) preferred fork on textual conflict but dropped fork logic where the dependency was semantic, leaving main's CI red. Restore the intended fork behavior: - fm-spawn: restore relaunch endpoint adoption (replacement reuses the recorded endpoint/worktree instead of creating a second window), skip the fresh-secondmate registry-validation guard on relaunch (the meta-exists precondition already proves the home), and restore the relaunch worktree-verification branch so a relaunch fails closed when its endpoint drifted out of the recorded worktree. - fm-lint: restore fm_lint_changed_base_ref, fm_lint_is_canonical_root, and CHANGED_MODE init (called-but-undefined after the merge). - fm-send, fm-bootstrap, fm-teardown: restore dropped fork behavior. - tests: FM_LINT_CACHE=0 on the two fork selection-tests so the upstream content cache does not contaminate them; fm-backend.test.sh fixups.
…-send with RESOLVE_KEYS
|
Warning Review limit reached
Next review available in: 44 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (3)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR gates deferred network mutations, adds changed-file lint selection and resolution-key parsing, supports resource-preserving task relaunches, adds teardown locking, reorders session-start output, and updates backend and lifecycle tests. ChangesNetwork and session workflows
Lint and decision resolution tooling
Task relaunch and teardown lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Spawn as fm-spawn.sh
participant Metadata as task metadata
participant Endpoint as adopted endpoint
participant Agent
Spawn->>Metadata: read recorded bindings
Spawn->>Endpoint: verify recorded worktree
Endpoint-->>Spawn: return location
Spawn->>Metadata: publish traceparent under lock
Spawn->>Agent: launch replacement agent
Suggested reviewers: 🚥 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 `@tests/fm-backend.test.sh`:
- Around line 135-147: Update resolve_correct_fm_send_ref to exclude the current
implementation under test from its git history search: begin the first-parent
log before HEAD or otherwise restrict matches to revisions predating the
RESOLVE_KEYS restoration. Ensure send_ref cannot select the current
bin/fm-send.sh while preserving the existing historical-content check.
🪄 Autofix
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: f30ce55f-314b-46a7-b6fe-caaf926998a3
📒 Files selected for processing (7)
bin/fm-bootstrap.shbin/fm-lint.shbin/fm-send.shbin/fm-spawn.shbin/fm-teardown.shtests/fm-backend.test.shtests/fm-lint.test.sh
💤 Files with no reviewable changes (1)
- bin/fm-bootstrap.sh
The upstream reconcile merge (f7b7cff) preferred the fork on textual conflict but dropped fork logic wherever the dependency was semantic rather than textual, leaving callers/usages without their definitions. Fix forward by restoring the dropped pieces (no revert): - fm-session-start.sh: restore the fork's digest section ordering the merge scrambled - READ-ONCE CONTRACT ahead of the digests it governs, the NETWORK CHECKS section, and the truncation-safe PERSONA/CONTEXT order - and restore the dropped BACKLOG_LIMIT assignment the merged-in beads compact-backlog emitter depends on (under `set -u` its absence aborted the emitter before it printed the labeled listing). Fixes: portable serial 3, and the captain-shared read-once assertion in portable serial 1. - fm-session-start.test.sh: restore FM_FAKE_HARNESS_PID in the herdr secondmate fixture so fake-ps ancestry proves lock ownership and the deferred network stage actually publishes. - fm-bootstrap.sh / fm-fleet-sync.sh: restore the network-sweep lock re-ownership guards and the timing-lib source the deferred network stage relies on. Fixes: portable serial 2. - fm-backend-herdr-presentation-e2e.test.sh: restore the dropped finish_concurrent_spawn / finish_concurrent_expected_abort helpers whose callers survived the merge (command-not-found aborted the run). Fixes: Behavior tests (Herdr). - fm-cmux-claude-composer-live-e2e.test.sh: set FM_SPAWN_SKIP_PARLAY=1, which the merged-in structural parlay guard now (correctly) requires of every test that drives fm-spawn.sh. Fixes: portable serial 4.
The upstream reconcile merge inlined the tangle check unconditionally in the
bootstrap main body, dropping the fork's local_phase gate (upstream kept it
inside local_phase && detect_local_config). As a result the deferred
network-only worker (FM_BOOTSTRAP_NETWORK=only) re-emitted the tangle line in
the digest's NETWORK CHECKS section.
For a --reemit session that already ran the mutating sweeps at startup, that
deferred worker runs with --locked 0 / DETECT_ONLY=1, so the re-emitted line
used the unlocked read-only wording ('read-only session must leave restore work
to the session holding the fleet lock') even though the re-emitting session
holds the fleet lock. That regressed tests/fm-session-start.test.sh
'--reemit keeps repair ownership with the lock holder'.
Gate the tangle detect line with local_phase so only the local BOOTSTRAP
section emits it (locked -> restore instructions; read-only -> leave-to-holder),
and the network-only worker never reprints it.
…racing exec The orphan-reap fixture starts a worker through fm_remote_job_start_linux_worker, which backgrounds a nohup/env chain and returns before that chain has exec'd into '/bin/bash <worker>'. start_worker then took a single immediate pgrep read for the supervisor pid. On a loaded Linux CI runner (portable serial shard 1), /proc/PID/cmdline still showed the pre-exec 'env ...' form when that read ran, so pgrep matched nothing, start_worker returned empty, and the whole test failed at setup with 'could not start the fixture remote job worker' (exit=1, 95ms). macOS never lost the race, so it passed there; the worker code path is identical on both (the test forces FM_REMOTE_JOB_PLATFORM_OVERRIDE=Linux), leaving only kernel-level exec/pgrep timing as the difference. Poll for the supervisor to surface with the same bounded idiom the file's wait_child/wait_gone helpers use. No behavioral assertion is weakened: the worker must still genuinely appear, and every downstream process-group, orphaning, and reaper assertion is unchanged.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/fm-bootstrap.sh (2)
172-184: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when the expected lock owner is missing.
At Line 180, an empty
FM_BOOTSTRAP_NETWORK_LOCK_PIDreturns success beforeSTATE/.lockis verified. A missing environment value can therefore authorize the deferred mutations in Lines 1246-1274. Require an explicit verified owner or an equivalent lock token.As per coding guidelines,
**/*: If the session lock cannot be acquired and verified, remain read-only and do not spawn, steer, merge, drain queues, repair supervision, or mutate fleet state.Suggested authorization change
- [ -n "$expected" ] || return 0 + [ -n "$expected" ] || return 1If a non-deferred caller legitimately has no expected PID, verify the lock explicitly in that caller instead.
🤖 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 `@bin/fm-bootstrap.sh` around lines 172 - 184, Update network_mutation_authorized so an unset FM_BOOTSTRAP_NETWORK_LOCK_PID fails closed instead of returning success; require a nonempty, numeric expected owner and a verified matching STATE/.lock before authorizing mutations. Move any legitimate no-expectation authorization to the non-deferred caller, with explicit lock verification there, while preserving read-only behavior when verification fails.Source: Coding guidelines
1170-1205: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the remaining local checks with
local_phase.
crew_dispatch_validateand the backlog validation block run unconditionally. Therefore,FM_BOOTSTRAP_NETWORK=onlystill emits crew/backlog diagnostics and can runtask list. Move these checks, including the queue reconciliation call, inside the local phase.🤖 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 `@bin/fm-bootstrap.sh` around lines 1170 - 1205, Guard the remaining local diagnostics with the existing local_phase condition so FM_BOOTSTRAP_NETWORK=only does not emit crew or backlog output or execute task list. Move crew_dispatch_validate, the backlog validation block, and its queue reconciliation call inside the local-phase flow, preserving their current order and behavior during local runs.
🤖 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.
Outside diff comments:
In `@bin/fm-bootstrap.sh`:
- Around line 172-184: Update network_mutation_authorized so an unset
FM_BOOTSTRAP_NETWORK_LOCK_PID fails closed instead of returning success; require
a nonempty, numeric expected owner and a verified matching STATE/.lock before
authorizing mutations. Move any legitimate no-expectation authorization to the
non-deferred caller, with explicit lock verification there, while preserving
read-only behavior when verification fails.
- Around line 1170-1205: Guard the remaining local diagnostics with the existing
local_phase condition so FM_BOOTSTRAP_NETWORK=only does not emit crew or backlog
output or execute task list. Move crew_dispatch_validate, the backlog validation
block, and its queue reconciliation call inside the local-phase flow, preserving
their current order and behavior during local runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fbcc12c-dd3e-46f7-bdd0-951a97cc5886
📒 Files selected for processing (2)
bin/fm-bootstrap.shtests/fm-remote-job-orphan-reap.test.sh
Resolve the fm-send conformance baseline from genuinely historical first-parent history instead of the newest RESOLVE_KEYS commit: this branch restored RESOLVE_KEYS, so the newest match was HEAD's own fm-send and the old-vs-new conformance diff collapsed into a tautology. Walk first-parent from HEAD, skip the leading restoration run, and take the first RESOLVE_KEYS commit after the reconcile drop, which is the fork's pre-bug fm-send. Fail closed in network_mutation_authorized when a deferred network-only worker has no verifiable lock owner; single-phase runs stay authorized via local_phase rather than an empty PID, so the guard no longer authorizes on a missing owner. Gate crew_dispatch_validate and the backlog-backend validation on local_phase so the deferred network-only worker no longer re-emits local diagnostics, and model the deferred worker's lock PID in the partition test so its sweeps stay authorized.
trillium
changed the title
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 `@AGENTS.md`:
- Line 177: Update the NETWORK CHECKS description in AGENTS.md to use the
consistent five-sweep model: change “four network sweeps” to “five network
sweeps” and list secondmate liveness and secondmate convergence as separate
checks, matching the terminology used by the surrounding steps and
bin/fm-startup-network.sh.
🪄 Autofix
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: bdfe59a4-a94b-4bfd-898b-bff9ca52b7e9
📒 Files selected for processing (3)
AGENTS.mdbin/fm-session-start.shtests/fm-session-start.test.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/fm-session-start.test.sh
- bin/fm-session-start.sh
Point 7's NETWORK CHECKS description said four network sweeps while enumerating five checks, and folded secondmate liveness and convergence into one. AGENTS.md line 159 and bin/fm-startup-network.sh both treat them as separate sweeps. Sync the count and enumeration to the authoritative five-check model: GitHub auth, project clone refresh, secondmate liveness, secondmate convergence, and pending handoff delivery.
… five network sweeps
trillium
changed the title
|
@coderabbitai review |
|
1 participant
Intent
Restore firstmate main-branch CI to green after the upstream reconcile merge (f7b7cff) plus hotfix clobbered committed fork behavior, delivered fix-forward on PR #113 without reverting the merge or rewriting main history; scope is merge-fallout restoration. This follow-up commit (6566e28) fixes one CodeRabbit finding on the current head: AGENTS.md section 3 point 7 (NETWORK CHECKS) said 'four network sweeps' while listing five checks and folded secondmate liveness and convergence into one. AGENTS.md line 159 and bin/fm-startup-network.sh both enumerate five separate checks (GitHub auth, project clone refresh, secondmate liveness, secondmate convergence, pending handoff delivery), so this is a doc-consistency sync of the describing artifact to the authoritative runtime model. Keep changes minimal, follow firstmate-coding-guidelines (one sentence per line, plain dash, no agent co-author), do not weaken tests.
What Changed
fm-spawn.shregains its relaunch path (adopts the recorded endpoint/worktree as a replacement, carries the recorded projects list forward, validates the endpoint sits in its recorded worktree, and recordstraceparent),fm-send.shregains--resolve-keyflag parsing with validation,fm-lint.shregains its changed-base-ref and canonical-root helpers,fm-teardown.shregains control/meta lock acquisition with an EXIT-release trap, andfm-fleet-sync.shre-sources the timing lib.fm-session-start.shto its restored 10-stage runtime order (adding the read-once contract, fleet-state, and network-checks stages, and restoringFM_SESSION_START_BACKLOG_LIMIT) and correctedAGENTS.mdto enumerate five network sweeps, the 10-stage order, and persona/digest ordering.RESOLVE_KEYShandling.Risk Assessment
✅ Low: The only new change is a minimal, comment-only doc sync that correctly aligns bin/fm-session-start.sh step 7 to the authoritative five network checks in bin/fm-startup-network.sh and AGENTS.md, with no executable code touched and the distinct "four network sweeps" reference at line 38 correctly preserved.
Testing
Ran the smallest relevant targeted tests (fm-startup-network and fm-session-start), both green, which exercise the actual runtime NETWORK CHECKS emission rather than the doc strings. Because the change is a doc-to-runtime consistency sync, I anchored it to the authoritative executable model: I extracted the runtime's emitted phase enumeration from bin/fm-startup-network.sh and confirmed it names five distinct sweeps (GitHub auth, dead-secondmate relaunch/liveness, secondmate convergence, pending handoff delivery, project clone refresh) with liveness and convergence separate, then verified AGENTS.md point 7, AGENTS.md line 159, and the fm-session-start.sh stage-7 header all now match that five-check model. Evidence transcript saved to the evidence dir. No screenshot applies — this is a shell/doc CI change with a CLI/text surface, and the transcript captures that end-user-visible enumeration directly. Worktree left clean. One non-blocking informational note: the read-only branch's user-facing NETWORK CHECKS output (fm-session-start.sh:888) and the stage-2 header (line 38) still carry the old folded/four phrasing, pre-existing and outside this commit's minimal declared scope.
Evidence: Runtime-vs-doc five-check consistency transcript
phase_label probe,sweeps -> GitHub authentication | dead-secondmate relaunch | secondmate convergence | pending handoff delivery | project clone refresh (liveness and convergence SEPARATE) AGENTS.md line 159: GitHub auth, dead-secondmate relaunch, secondmate convergence, pending handoff delivery, project clone refresh AGENTS.md point 7 (fixed): five network checks (GitHub auth, project clone refresh, secondmate liveness, secondmate convergence, pending handoff delivery) fm-session-start.sh stage-7 (fixed): five network checks (GitHub auth, project clone refresh, secondmate liveness, secondmate convergence, pending handoff delivery)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-session-start.sh:53- The follow-up commit synced AGENTS.md section 3 point 7 to say 'five network checks' and split secondmate liveness/convergence, but the sibling describing artifact bin/fm-session-start.sh still carries the exact same inconsistency the fix corrected: the header stage list says 'the four network sweeps' and folds 'secondmate liveness and convergence' into one item at lines 38 and 53-55. The same file's own comment at lines 80-81 correctly enumerates the five external calls (gh auth, secondmate liveness, secondmate convergence, pending handoff delivery, fleet-sync fetch), matching corrected AGENTS.md:177 and the authoritative runtime bin/fm-startup-network.sh, so the file now self-contradicts (four vs five). This branch rewrote this header (10-stage restructure), so it is changed code in scope; syncing it to five completes the same doc-consistency correction the intent applied to AGENTS.md.🔧 Fix: sync fm-session-start header to five network checks
✅ Re-checked - no issues remain.
bin/fm-session-start.sh:888- The user-facing read-only NETWORK CHECKS runtime output at bin/fm-session-start.sh:888 still folds "secondmate liveness and convergence" into one item ("GitHub authentication, project clone refresh, secondmate liveness and convergence, and pending handoff delivery"), the same folding pattern the CodeRabbit finding flagged, and line 38's stage-2 header still says "the four network sweeps". These are pre-existing and outside this commit's explicitly minimal declared scope (AGENTS.md point 7 + the stage-7 header, both correctly synced to five). Not a regression or test failure; flagging so the author can decide whether the five-check sync should extend to these two spots or stay minimal.bash tests/fm-startup-network.test.sh— exercises the deferred network worker end-to-end; asserts the emitted NETWORK CHECKS report enumerates the distinct sweeps (dead-secondmate relaunch, secondmate-liveness, convergence, handoff, clone refresh) — all assertions passedbash tests/fm-session-start.test.sh— full session-start digest behavior incl. NETWORK CHECKS section emission and truncation banner — all assertions passedManual: extracted the runtime'sphase_label probe,sweepsenumeration and diffed it against AGENTS.md point 7, AGENTS.md line 159, and the fm-session-start.sh stage-7 header to confirm all four now agree on five separate checks with liveness and convergence distinctgrep -niE 'four network|liveness and convergence'across AGENTS.md and both bin scripts to confirm the fixed point-7/stage-7 artifacts carry no stale 'four'/folded phrasing✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.