Skip to content

fix(bin): distinguish an ahead sync target from a diverged one#684

Open
zachlandes wants to merge 2 commits into
kunchenguid:mainfrom
zachlandes:fm/secondmate-seed-gaps
Open

fix(bin): distinguish an ahead sync target from a diverged one#684
zachlandes wants to merge 2 commits into
kunchenguid:mainfrom
zachlandes:fm/secondmate-seed-gaps

Conversation

@zachlandes

Copy link
Copy Markdown

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:

  • The SKIP BEHAVIOUR IS INTENTIONALLY UNCHANGED. Both cases still skip and leave the target untouched. A fast-forward cannot run backwards and rewinding would discard commits. Only the diagnosis string changed. This is deliberate, not an incomplete fix.
  • The SYNC TARGET WAS DELIBERATELY LEFT ALONE after arguing it on evidence. The real design question raised was whether the local-HEAD sync should target origin/main instead of the primary's local default branch. I concluded the current target is correct and did NOT change it: it keeps the sync network-free at spawn (tests pin 'no fetch in the local path'), it honours a primary deliberately pinned behind origin (this fleet did that over PR fix(snapshot): assign newline to var before case pattern to avoid tokenize-time split #574; targeting origin/main would defeat the pin), it keeps the primary and its secondmates on ONE instruction version, and origin-tracking is already owned by /updatefirstmate (fm-update.sh, base_mode 'origin'). Do not flag 'why not target origin/main' as a miss - it was considered and rejected.
  • The 'ahead of by N commits' wording is deliberately MODE-NEUTRAL because ff_target serves both base modes: the local-HEAD secondmate sync AND /updatefirstmate's origin mode (where ahead means local unpushed commits). Operator interpretation lives in the bootstrap-diagnostics skill, per the repo's one-owner rule, not duplicated in the message.
  • The ahead reason is also the ONLY automatic signal that a primary is behind origin (verified: nothing else reports primary staleness), which is why the skills now point at it rather than staying silent.

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_target in bin/fm-ff-lib.sh now adds the complementary ancestry check when HEAD is not contained in the base: if the base is an ancestor of HEAD, it reports skipped: ahead of <base> by N commits instead of collapsing that healthy state into diverged from <base>. The skip behavior is unchanged in both cases — the target is never fast-forwarded backwards.
  • The operator-facing docs that interpret the sync-skip reasons were updated to cover the new ahead reason and both of its causes (a primary pinned behind origin, or a home holding its own commit): the spawn-time comment in bin/fm-spawn.sh, docs/architecture.md, and the bootstrap-diagnostics, secondmate-provisioning, and updatefirstmate skills.
  • tests/fm-secondmate-sync.test.sh gains 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.

  • Outcome: ⚠️ 1 error across 1 run (19m35s)

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 the SECONDMATE_SYNC: secondmate &lt;id&gt;: skipped: &lt;reason&gt; 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.

⚠️ **Test** - 1 error
  • 🚨 tests failed with exit code 1
  • 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.

@zachlandes
zachlandes force-pushed the fm/secondmate-seed-gaps branch from e61d45e to 4b8dabe Compare July 17, 2026 23:46
* 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.
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