fix(bin): distinguish an ahead sync target from a diverged one#684
Open
zachlandes wants to merge 2 commits into
Open
fix(bin): distinguish an ahead sync target from a diverged one#684zachlandes wants to merge 2 commits into
zachlandes wants to merge 2 commits into
Conversation
zachlandes
force-pushed
the
fm/secondmate-seed-gaps
branch
from
July 17, 2026 23:46
e61d45e to
4b8dabe
Compare
* A secondmate home leased at current origin/main, synced against a primary checkout that was 5 commits behind origin, reported "diverged from <sha>". Nothing had forked: the home held the primary's commit plus newer ones, so it was strictly ahead, and the fast-forward simply could not run backwards. * ff_target tested only whether HEAD was an ancestor of the base, and treated every failure as a divergence. That collapsed two opposite conditions into one message: a diverged target may hold unlanded work and needs inspecting, while an ahead target is healthy and indicts the primary for being stale. * Both cases are still skipped and left untouched, so only the diagnosis changes. The reason string is the only thing distinguishing them, which is why it has to be accurate. * The ahead reason is also the earliest automatic signal that a primary is behind origin; nothing else reports that, and a stale primary silently bases every new home and task worktree on its old commit. * Left the local-HEAD sync target alone: following the primary's own default branch is deliberate. It keeps the sync network-free, honours a primary pinned behind origin on purpose, keeps the fleet on one instruction version, and leaves origin-tracking to /updatefirstmate, which owns that path.
zachlandes
force-pushed
the
fm/secondmate-seed-gaps
branch
from
July 18, 2026 00:24
4b8dabe to
9c6453f
Compare
4 tasks
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
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.
Intent
Diagnose two gaps that surfaced when this fleet's first secondmate was seeded and launched, route each fix to its correct owner, and implement ONLY the part that genuinely belongs in this (firstmate) repo.
GAP 1 (implemented here). Spawning a secondmate warned: 'sync skipped before launch: diverged from cd218f2'. Reproduced deliberately in a scratch repo (primary pinned behind origin, home leased at current origin/main): the home was ahead by 2 and behind by 0, yet ff_target in bin/fm-ff-lib.sh reported 'diverged'. Root cause: it tested only 'merge-base --is-ancestor HEAD base' and treated EVERY failure as a divergence, collapsing two opposite conditions into one message. A diverged target has forked history that may hold unlanded work (inspect it); an ahead target is healthy and instead indicts the PRIMARY for being stale. The fix adds the complementary 'is base an ancestor of HEAD' test and reports 'ahead of by N commits'.
DELIBERATE DECISIONS a reviewer reading only the diff would not know:
GAP 2 (deliberately NOT implemented here - different owner). A freshly seeded secondmate home errored on every prompt: missing data/decisions/hooks/fm-decide-inject.sh. Root-caused empirically: a Claude session whose cwd is a linked git worktree DOES load the MAIN checkout's .claude/settings.local.json (proven with sibling dirs, so the git link is the only path), while $CLAUDE_PROJECT_DIR re-resolves to the worktree - so the hook resolves into a home where the script does not exist. It hits EVERY worktree of this repo, not just secondmates. The decisions pane is the captain's LOCAL extension (firstmate-local project; data/ is gitignored per-home), so the fix belongs there and is out of scope for this repo. I explicitly REJECTED the one option that would have touched this repo (growing the seed's inheritable-config set / FM_INHERITABLE_CONFIG): it would not fix crewmate worktrees, that list covers config/* rather than .claude/ or data/, and it would bend this shared repo to carry a purely local extension. Gap 2's absence from this diff is intentional; do not flag it as incomplete.
SCOPE/CONVENTION DECISIONS: tests were colocated into the existing tests/fm-secondmate-sync.test.sh next to its T4 diverged test (repo convention: extend an existing runner, and keep the ahead/diverged distinction pinned side by side) rather than adding a new runner; the new tests were verified to FAIL against pre-fix code. AGENTS.md was deliberately left untouched per the repo's size-discipline rule - the conditional operator knowledge was routed to the two agent-only skills (bootstrap-diagnostics, secondmate-provisioning) that already own those triggers, with only their existing cross-references updated rather than restated. This change is a generic upstream contribution to kunchenguid/firstmate (a repo we do not own), not local fleet wiring.
What Changed
ff_targetinbin/fm-ff-lib.shnow adds the complementary ancestry check when HEAD is not contained in the base: if the base is an ancestor of HEAD, it reportsskipped: ahead of <base> by N commitsinstead of collapsing that healthy state intodiverged from <base>. The skip behavior is unchanged in both cases — the target is never fast-forwarded backwards.aheadreason and both of its causes (a primary pinned behind origin, or a home holding its own commit): the spawn-time comment inbin/fm-spawn.sh,docs/architecture.md, and the bootstrap-diagnostics, secondmate-provisioning, and updatefirstmate skills.tests/fm-secondmate-sync.test.shgains cases pinning the ahead/diverged distinction side by side with the existing T4 diverged test, including the ahead-by-N wording and the untouched-HEAD guarantee.Risk Assessment
✅ Low: The round-2 commit is provably comment-and-prose only (executable code in both shell files is byte-identical to the previously reviewed revision, the emitted reason strings are unchanged, and tests are untouched), and it resolves both round-1 findings accurately while honoring every style and scoping constraint the captain specified.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
.agents/skills/bootstrap-diagnostics/SKILL.md:50- The skills assert a single cause for the new "ahead" reason, but ff_target emits it for any target whose HEAD strictly contains the base. bootstrap-diagnostics:50 says it "is the one that indicts the PRIMARY rather than the home ... nothing forked and nothing is at risk, but this primary checkout is behind origin", and secondmate-provisioning/SKILL.md:76 goes further with the absolute "A home reads ahead when this primary is behind origin". A second cause reaches the identical branch at bin/fm-ff-lib.sh:365: a clean detached home that committed its own tracked change on top of a current primary. That is precisely T4's fixture (tests/fm-secondmate-sync.test.sh:194) minus the bump_primary call -- the suite already treats "a home with its own commit" as a first-class expected state, and only the primary also moving is what makes T4 read "diverged" instead of "ahead". In that case the home DOES hold unlanded work and the primary is NOT stale, so "nothing is at risk" and the step to "treat it as the signal to update this checkout (/updatefirstmate)" (line 51) both misdirect. This is the same two-opposite-conditions-one-message conflation the diff removes from the code, reintroduced in the prose that interprets it. No data loss is possible (the skip is unconditional and HEAD is never touched), so this is diagnostic accuracy, not a correctness bug. Suggest hedging the cause, e.g. "usually means this primary is behind origin; confirm the home holds no commit of its own before concluding the primary is stale." Flagged for you rather than auto-fixed because the intent states the ahead->primary-stale framing was deliberate..agents/skills/updatefirstmate/SKILL.md:54- Because ff_target serves both base modes, /updatefirstmate's origin mode now emits a new reason string, "ahead of origin/main by N commits", for the ordinary unpushed-local-commits state -- which previously read "diverged from origin/main". No skill or doc interprets it. The intent says "Operator interpretation lives in the bootstrap-diagnostics skill", but that entry is keyed to theSECONDMATE_SYNC: secondmate <id>: skipped: <reason>prefix, which only bin/fm-bootstrap.sh:216 emits; bin/fm-update.sh:53 prints raw ff_target lines with no prefix, so the origin-mode ahead output falls outside that owner. Meanwhile this Safety line enumerates "diverged, dirty, offline, non-default branch" and docs/architecture.md:234 enumerates "dirty, diverged, offline, and off-default" -- neither mentions ahead, and the step-4 cue at line 49 ("un-landed changes (diverged)") no longer matches the string this state now produces. Informational only: no wrong action follows, since the message is self-explanatory, step 4's generic "surface any skipped target" still covers it, and the new wording is strictly more accurate than "diverged" was. Raised because the diff updated the two skills owning the local-HEAD path, and these are the corresponding owners for the origin path.🔧 Fix: docs: state both causes of an ahead sync target
✅ Re-checked - no issues remain.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.