feat(bin): add configurable base/merge-target branch for ship tasks#213
Open
abdullah-kasim wants to merge 3 commits into
Open
feat(bin): add configurable base/merge-target branch for ship tasks#213abdullah-kasim wants to merge 3 commits into
abdullah-kasim wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Captain, this PR adds a configurable base/merge-target branch for ship tasks so work can be branched from—and (for local-only/direct-PR flows) landed into—a long-lived feature branch instead of always using the repo default branch. The change is implemented by recording base=<branch> in state/<id>.meta and having the brief/review/merge/teardown helpers consistently honor it.
Changes:
- Add
fm-spawn.sh --base <branch>(ship only) to recordbase=in task meta, andfm-brief.sh --baseto scaffold branch-from-base and base-targeted landing instructions (with explicit refusals for scout/secondmate). - Update
fm-review-diff.sh,fm-merge-local.sh, andfm-teardown.shto use the recordedbase=as the review/merge/landing target, including guarded error paths for missing base branches. - Add a comprehensive behavior test suite for base-branch flows and document the feature + the no-mistakes limitation.
Reviewed changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
bin/fm-spawn.sh |
Adds --base <branch> for ship tasks, records base= in meta, and refuses --base for scout/secondmate. |
bin/fm-brief.sh |
Adds --base to scaffold branching from the base branch and retarget local-only/direct-PR wording; documents no-mistakes limitation. |
bin/fm-review-diff.sh |
Uses recorded base= as diff base (origin-backed fetch when applicable) and errors explicitly when missing on origin. |
bin/fm-merge-local.sh |
Fast-forwards the recorded base branch (or default) with the same safety refusals (clean checkout, ff-only, divergence refusal). |
bin/fm-teardown.sh |
Treats recorded base= as landing target for local-only and content-landed checks; adds a fail-closed missing-base refusal for local-only. |
tests/fm-base-branch.test.sh |
Adds behavior tests covering meta recording, refusals, brief rewriting, review/merge/teardown base targeting, and missing-base failure modes. |
AGENTS.md |
Documents base= semantics, how it affects local-only landing, and updates usage examples and lifecycle notes. |
docs/architecture.md |
Documents base= as the landing target and clarifies the no-mistakes PR-target limitation. |
docs/scripts.md |
Updates script catalog entries to mention base-branch support in spawn/brief/merge/review/teardown. |
CONTRIBUTING.md |
Adds the new base-branch test file to the enumerated test list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4 tasks
7d064a0 to
3ed4dff
Compare
4 tasks
fm-spawn --base <branch> records base= in the task's meta (ship only; refused for --scout/--secondmate; threaded through batch dispatch). fm-brief --base scaffolds the ship Setup to branch from the base branch and points local-only/direct-PR landing text at it. fm-review-diff, fm-merge-local, and fm-teardown's landed-work checks read base= as the single source of truth for the review base and landing target. A meta without base= behaves byte-for-byte as before.
…ument no-mistakes --base limitation
3ed4dff to
426a7b5
Compare
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
Add a configurable base/merge-target branch for ship tasks so a captain working on a long-lived feature branch can have crewmate work based on - and landed into - that branch instead of the repo's default branch. fm-spawn.sh gains a --base flag (single and batch dispatch) that records base= in state/.meta; the flag is deliberately REFUSED (not ignored) for --scout and --secondmate, since a scout worktree is scratch and a secondmate home has no task branch - this refusal choice was left to the implementer and is documented in the script header and AGENTS.md. base= in meta is the single source of truth all consumers read. fm-brief.sh gains --base: the scaffolded ship Setup tells the crewmate to verify the base branch exists in the worktree (fetching from origin if needed) and branch from it with git checkout -b fm/ ; local-only landing/rebase wording and the direct-PR open-a-PR line also target the base branch. fm-review-diff.sh diffs against origin/ (after fetch) for remote-backed projects or the local base branch otherwise. fm-merge-local.sh fast-forwards the recorded base branch with the same safety refusals adapted (checkout on the base branch and clean, ff-only, divergence refused with rebase advice, plus a clear error for a missing base branch). fm-teardown.sh treats a recorded base= as the landing target via a landing_branch() helper. Everything is backward-compatible: a meta without base= behaves byte-for-byte as today, and fm-brief output without --base was verified byte-identical to the pre-change scaffold for all three delivery modes. A prior run's review gate found three actionable findings which the user decided on; fix them in this run when they resurface, resolved as follows: (1) teardown must verify the recorded base branch exists (mirroring fm-merge-local's refs/heads check) before the local-only landing check and refuse clearly when it does not, so a missing base can never silently pass unlanded work; (2) for the no-mistakes delivery mode the user explicitly chose to DOCUMENT the limitation that --base does not redirect the pipeline-opened PR's landing target (the no-mistakes pipeline owns PR creation against its configured push target) rather than refuse --base or thread the base into the pipeline - the user's primary flow is local-only mode where --base works end to end; (3) fm-review-diff must guard the origin fetch of the base branch and emit an explicit actionable error when the base branch does not exist on origin. Docs: header comments of the touched scripts plus minimal surgical AGENTS.md wording in sections 1, 6, and 7 (CLAUDE.md is a symlink, only AGENTS.md edited). Tests: new tests/fm-base-branch.test.sh with 17 cases following existing conventions. Out of scope per the task: no per-project standing base= field in data/projects.md, no scout/secondmate changes beyond the refusal, no changes to PR-based remote-reachability logic beyond the base generalization. The full test suite (34 files) and CI's shellcheck invocation pass locally.
What Changed
bin/fm-spawn.shgains a--base <branch>flag (single and batch dispatch) that recordsbase=in the task's meta as the single source of truth; the flag is refused for--scoutand--secondmate.bin/fm-brief.sh --basescaffolds the ship Setup to verify and branch from that base, and points local-only/direct-PR landing wording at it.bin/fm-review-diff.sh,bin/fm-merge-local.sh, andbin/fm-teardown.shnow honor a recordedbase=as the review/merge/landing target, with explicit refusals when the base branch is missing locally or on origin (teardown mirrors fm-merge-local's refs/heads existence check; review-diff guards its origin fetch with an actionable error). Tasks withoutbase=behave exactly as before, andfm-brief.shoutput without--basewas verified byte-identical to the pre-change scaffold for all three delivery modes.tests/fm-base-branch.test.sh(19 cases covering meta recording, refusals, brief rewriting, review-diff bases, merge fast-forward and refusal paths, and teardown landing checks), and documents the flag — including the no-mistakes-mode limitation that--basedoes not redirect the pipeline-opened PR's target — in the script headers,AGENTS.md, anddocs/.Risk Assessment
✅ Low: All three previously-decided review findings are correctly fixed and fail closed (missing-base teardown refusal, guarded review-diff fetch, documented no-mistakes limitation), the feature is backward compatible with base= absent, and 17 new tests cover every refusal and retarget path; only two cosmetic/informational observations remain.
Testing
Baseline full suite plus a rerun (34 files, 502 assertions) pass; the new 19-case fm-base-branch test file covers the whole feature matrix including the three review-gate fixes; a manual CLI end-to-end demo showed the captain's primary local-only --base flow working (brief/spawn/review/merge/teardown all targeting the feature branch, main untouched, premature teardown refused); and no-flag brief output was confirmed byte-identical to the pre-change scaffold in all three delivery modes. No issues found; this is a CLI/scripting change with no rendered UI surface, so transcripts are the appropriate visual evidence.
Evidence: End-to-end local-only --base lifecycle transcript (brief → spawn → review → refused teardown → merge into feature branch → teardown)
Evidence: New fm-base-branch test file output (19/19 ok)
Evidence: Brief backward-compatibility check: no --base output byte-identical to pre-change scaffold for all three modes
mode=no-mistakes: brief without --base is BYTE-IDENTICAL to pre-change (951f5cf) scaffold mode=direct-PR: brief without --base is BYTE-IDENTICAL to pre-change (951f5cf) scaffold mode=local-only: brief without --base is BYTE-IDENTICAL to pre-change (951f5cf) scaffoldEvidence: direct-PR base targeting and documented no-mistakes limitation
Evidence: Reproducible e2e demo script
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-teardown.sh:108- landing_branch() returns the recorded base= branch without verifying it exists (unlike fm-merge-local.sh's show-ref check), and the local-only landing check at line 633 swallows git's 'bad revision' error (git log HEAD --not "$DEFAULT" -- 2>/dev/null | head -5 || true), leavingunmergedempty. Reproduced: a clean local-only worktree with unmerged commits whose recorded base branch is missing passes the safety check, and teardown deletes the branch and force-returns the worktree — unlanded work destroyed, violating hard rule docs: align tmux and harness guidance #3. This is prior review finding (1), which the user resolved as: verify the recorded base branch exists (mirroring fm-merge-local's refs/heads check) before the local-only landing check and refuse clearly. The fix is absent at this commit; add it plus a teardown test for the missing-base refusal.bin/fm-review-diff.sh:86- The origin fetch of the review base is unguarded: when the task's recorded base branch does not exist on origin,git fetch origin "+refs/heads/$TARGET:refs/remotes/origin/$TARGET"fails and set -eu kills the script with git's raw fatal message instead of an explicit actionable error naming the missing base branch. This is prior review finding (3), which the user resolved as: guard the fetch and emit an explicit actionable error when the base branch does not exist on origin. It fails closed (no wrong diff is produced), but the decided fix is absent at this commit.bin/fm-spawn.sh:12- Prior review finding (2) was resolved as DOCUMENT the no-mistakes limitation — that --base does not redirect the pipeline-opened PR's landing target, because the no-mistakes pipeline owns PR creation against its configured push target — but no such documentation exists anywhere at this commit: not in the fm-spawn.sh --base header block, not in fm-brief.sh's no-mistakes definition-of-done (which happily emits a branch-from-base Setup step for no-mistakes projects), and not in AGENTS.md. Add the decided limitation note to the fm-spawn.sh header (and ideally the no-mistakes brief wording) so a --base dispatch on a no-mistakes project doesn't silently produce a PR targeting the wrong branch with no warning on record.🔧 Fix: refuse missing base branch in teardown/review-diff, document no-mistakes limitation
2 infos still open:
bin/fm-teardown.sh:273- The local-only backlog reminder hardcodestasks-axi done <id> --note "local main"even when the task's meta records base=, so the Done record misstates the landing target (says "local main" though the work merged into the recorded base branch). Suggest notinglocal <landing branch>using landing_branch() / BASE_BRANCH.bin/fm-review-diff.sh:94- For a remote-backed project the review base is origin/<base> after fetch. If the captain's local base branch is ahead of origin (unpushed commits) and the crewmate branched from the local ref,origin/<base>...fm/<id>attributes those unpushed base commits to the crewmate's diff. This matches the deliberate origin-authoritative design mirrored from the default-branch flow; recording it as a known tradeoff only.✅ **Test** - passed
✅ No issues found.
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"bash tests/fm-base-branch.test.sh (19/19 pass: spawn --base meta recording single+batch, scout/secondmate refusals, brief rewriting, review-diff local/origin base + missing-origin-base error, merge-local ff + wrong-checkout/divergence/missing-base refusals, teardown base-landing + missing-base refusal + content-fallback against origin/<base>)full suite baseline command re-run:for t in tests/*.test.sh; do bash "$t"; done— 34 files, 502 ok, 0 failures, exit 0manual e2e demo driving real bin/ scripts on a local-only project with long-lived feature/train branch:fm-brief.sh --base,fm-spawn.sh --base(meta shows base=feature/train),fm-spawn.sh --scout --baserefusal,fm-review-diff.sh(diff base: feature/train, base-only commits excluded), prematurefm-teardown.shREFUSED naming feature/train,fm-merge-local.shfast-forwarded feature/train with main unchanged, finalfm-teardown.shsucceeded and removed the worktreebackward-compat check: brief generated without --base by HEAD vs 951f5cf fm-brief.sh is byte-identical for no-mistakes, direct-PR, and local-only modesdocs check: direct-PR brief with --base targets the base branch in its open-a-PR line; no-mistakes --base limitation documented in bin/fm-spawn.sh header and AGENTS.md sections 1/7CONTRIBUTING.md:64- Pre-existing gap unrelated to this change: tests/fm-gotmp.test.sh (present at the base commit) is missing from CONTRIBUTING.md's enumerated test list; left unfixed because it is not part of the base-branch change being documented.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.