Skip to content

feat: support guarded local merge targets#718

Open
waleedarafa wants to merge 20 commits into
kunchenguid:mainfrom
waleedarafa:fm/fm-merge-target-support-guarded-local-merge-into-an-appr-f5
Open

feat: support guarded local merge targets#718
waleedarafa wants to merge 20 commits into
kunchenguid:mainfrom
waleedarafa:fm/fm-merge-target-support-guarded-local-merge-into-an-appr-f5

Conversation

@waleedarafa

Copy link
Copy Markdown

Intent

The developer wanted Firstmate’s guarded local-only merge helper to accept an explicit existing local target branch, while leaving its default behavior of fast-forwarding the project’s default branch unchanged and preserving all existing safety checks. The implementation had to validate the target and task branches rigorously, require the main local checkout to be clean and on the exact target, use only fully guarded ff-only local Git operations, reject malformed or injection-shaped arguments, and add isolated regression coverage proving refusals never mutate repositories. Follow-up review requirements expanded the change to persist the selected target only after a successful fast-forward, make teardown validate containment against that recorded target, emit accurate completion history for non-default targets while retaining legacy “local main” behavior, and safely update validated metadata atomically. The developer also required focused and repository-wide validation, ShellCheck cleanliness, no changes to the real Secondbrain repository, no agent co-author, and delivery through the no-mistakes PR workflow without merging; after push and pipeline recovery issues, they explicitly required verifying and fast-forwarding the local branch to the known pipeline commit before rerunning validation without --yes.

What Changed

  • Add --target support to guarded local-only merges while preserving default-branch behavior and enforcing clean, checked-out, fast-forward-only landing.
  • Atomically record validated merge targets so teardown checks the correct branch and reports accurate completion notes.
  • Update workflow documentation and expand regression coverage for target selection, unsafe inputs, metadata handling, and refusal paths.

Risk Assessment

✅ Low: Captain, the prior command-injection path is fully addressed by validating derived targets and using the project’s established shell-quoting helper; the remaining change is well-bounded and guarded.

Testing

The supplied repository-wide baseline and focused merge/teardown suites passed; isolated end-to-end CLI fixtures demonstrated explicit-target landing, unchanged default-main behavior, atomic metadata persistence, teardown history against the recorded target, and non-mutating safety refusals. The retained transcript provides reviewer-visible evidence; no screenshot was applicable because this is a shell CLI change.

Evidence: End-to-end local merge CLI transcript
E2E 1 — explicit existing local target, persisted metadata, and teardown history
$ fm-merge-local.sh task-x1 --target feature/for-you-feed
merged fm/task-x1 into local target feature/for-you-feed (d410c69 -> 18fdf24) in /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/.e2e-scratch/explicit/project
main before:    d410c69f6837e1e1e22dbfefc913405cc4b8fec2
main after:     d410c69f6837e1e1e22dbfefc913405cc4b8fec2
feature before: d410c69f6837e1e1e22dbfefc913405cc4b8fec2
feature after:  18fdf246e3c03009f7ccb01848761f45938170aa
task HEAD:      18fdf246e3c03009f7ccb01848761f45938170aa
main unchanged: yes
feature target equals task HEAD: yes
recorded metadata:
sentinel=preserved
local_merge_target=feature/for-you-feed
$ fm-teardown.sh task-x1
teardown task-x1 complete (window fm-task-x1, worktree /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/.e2e-scratch/explicit/wt)
Backlog: task-x1 just finished. Run tasks-axi done task-x1 --note 'local feature/for-you-feed', then run tasks-axi ready for dependency-cleared candidates, check date gates, and dispatch only work whose blockers are gone and date is due.
task metadata removed after teardown: yes

E2E 2 — no-option legacy default remains local main
$ fm-merge-local.sh task-x1
merged fm/task-x1 into local target main (481b45f -> a44e1a4) in /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/.e2e-scratch/default/project
main before: 481b45f0c6712d3f86ccf0ae8cd0b7229fdbdfac
main after:  a44e1a4b80a823817073229a410c6e180b05d3cd
task HEAD:   a44e1a4b80a823817073229a410c6e180b05d3cd
main equals task HEAD: yes
recorded metadata: local_merge_target=main

E2E 3 — malformed/injection-shaped target refuses without mutation
$ fm-merge-local.sh task-x1 --target <injection-shaped-input>
error: unsafe or option-like target branch 'feature/$(touch /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/.e2e-scratch/injection/injected)'
Usage: fm-merge-local.sh <task-id> [--target <local-branch>]

Fast-forward a clean, checked-out local branch to fm/<task-id> for an approved
local-only task. Without --target, the project's default branch is the target.
--target never checks out or creates the requested local branch.
Successful landing records local_merge_target in the task metadata.
exit status: 2
repository snapshot unchanged: yes
injection marker absent: yes

E2E 4 — wrong checkout refuses without mutation
$ fm-merge-local.sh task-x1 --target feature/for-you-feed
error: /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/.e2e-scratch/wrong-checkout/project is on 'main', expected target branch 'feature/for-you-feed'; cannot merge safely
exit status: 1
repository snapshot unchanged: yes

E2E 5 — dirty target refuses and preserves dirty state exactly
$ fm-merge-local.sh task-x1 --target feature/for-you-feed
error: /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/.e2e-scratch/dirty/project has a dirty working tree; refusing to merge into target 'feature/for-you-feed'
exit status: 1
repository snapshot including dirty file unchanged: yes

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-teardown.sh:418 - LOCAL_MERGE_TARGET is interpolated into a shell command without shell quoting. Explicit targets reject metacharacters, but default targets bypass that guard; a valid default branch such as release$(touch${IFS}/tmp/p) can therefore be recorded and execute command substitution when this suggested command is run. Apply identical validation to derived targets and shell-quote the note argument.

🔧 Fix: Harden local merge target validation and command quoting
✅ Re-checked - no issues remain.

✅ **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"
  • Configured repository-wide baseline: command -v tmux &gt;/dev/null || { echo &#34;tmux is required for e2e tests&#34; &gt;&amp;2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo &#34;== $t ==&#34;; bash &#34;$t&#34; || rc=1; done; exit &#34;$rc&#34;
  • Focused regression suites: bash tests/fm-merge-local.test.sh &amp;&amp; bash tests/fm-teardown.test.sh
  • Evidence harness: chmod +x /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/run-e2e-evidence.sh &amp;&amp; /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/run-e2e-evidence.sh &gt; /var/folders/kl/9kq9q45504qd48wr0y8h1tnw0000gp/T/no-mistakes-evidence/01KXVSAN2B5CE3E18N2S5W6BYC/local-merge-e2e-transcript.txt 2&gt;&amp;1
  • Manual fixture invocations exercised bin/fm-merge-local.sh task-x1 --target feature/for-you-feed, its no-target default, injection-shaped input, wrong-checkout and dirty-target refusals, followed by bin/fm-teardown.sh task-x1 with the repository’s documented FM_GATE_REFUSE_BYPASS=1 test exemption.
🔧 **Document** - 1 issue found → auto-fixed (2) ✅
  • ⚠️ tests/fm-brief.test.sh:88 - Captain, this assertion still requires the obsolete local-main wording; updating it is necessary but prohibited by this documentation-only task.

🔧 Fix: Verify local merge target documentation
1 warning still open:

  • ⚠️ tests/fm-brief.test.sh:88 - Captain, this assertion still expects obsolete local main wording; fixing it requires changing a test, which this documentation-only task explicitly prohibits.

🔧 Fix: Update target-aware local merge assertion
✅ Re-checked - no issues remain.

⚠️ **Lint** - 1 warning
  • ⚠️ linter found issues (exit code 127)

🔧 Fix: Confirm pinned ShellCheck lint passes cleanly
1 warning still open:

  • ⚠️ linter found issues (exit code 127)
✅ **Push** - passed

✅ No issues found.

@waleedarafa
waleedarafa force-pushed the fm/fm-merge-target-support-guarded-local-merge-into-an-appr-f5 branch 2 times, most recently from 75308dd to c9fc942 Compare July 19, 2026 08:41
@waleedarafa
waleedarafa force-pushed the fm/fm-merge-target-support-guarded-local-merge-into-an-appr-f5 branch from e5dff3c to 5baf84f Compare July 19, 2026 08:50
@waleedarafa
waleedarafa force-pushed the fm/fm-merge-target-support-guarded-local-merge-into-an-appr-f5 branch from 5baf84f to 6b44bc3 Compare July 19, 2026 10:52
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