Conversation
A crewmate has no terminal a human can type into, but it inherits the machine's configured git editor. On this machine that is `code --wait`, which blocks until a human closes the tab. Any git command that opens an editor - `rebase --continue`/`-i`, `commit` without `-m`, non-ff `merge`, `revert`, `tag -a`, `cherry-pick --continue` - therefore hangs forever with no output and no error. The pane looks identical to a thinking pane, so nothing surfaces it until the staleness window expires, and the agent's natural response (retry) stacks more orphaned waiters on the same file. Observed live on task parlay-45-rebase: three wedged attempts, ~15 min lost, resolved only by killing the editor waiters by hand. Two layers, both here: - fm-spawn now exports GIT_EDITOR=true and GIT_SEQUENCE_EDITOR=true into the crewmate's pane shell alongside GOTMPDIR, before the harness launch, so the whole class is impossible rather than each worker remembering. `true` exits 0 without touching the file, so git proceeds with the commit message or rebase todo as written. - fm-brief adds rule 8 to both the scout and ship Rules sections: never let git open an editor, and treat a git command that is silent for over a minute as a blocked editor rather than a slow operation. Also lands the fix violation-0gh asks for, since it is the same one-line brief addition: ship rule 9 tells a worker whose branch is already checked out elsewhere to use a detached HEAD, never to remove or modify another worktree. New rules are appended rather than inserted so the existing "rule 6" cross-reference in the ship brief stays correct. Covered by a new pane-export assertion in tests/fm-kimi-harness.test.sh, which owns fm-spawn's pane environment contract.
📝 WalkthroughWalkthroughThe change adds Git editor-safety rules to scout and ship briefs. Spawned panes now export noninteractive Git editor variables. The Kimi harness verifies both variables. ChangesGit editor safety
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
Closing: this was raised directly with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@bin/fm-brief.sh`:
- Around line 466-467: Scope the editor-waiter recovery guidance at
bin/fm-brief.sh lines 466-467 to the current task by locating only verified
child processes of the current Git process, including pane identity where
available, before terminating a PID; do not use an unrestricted host-wide
search. Apply the identical guarded recovery procedure at bin/fm-brief.sh lines
649-650 for ship briefs.
In `@tests/fm-kimi-harness.test.sh`:
- Around line 212-213: Strengthen the test around the kimi spawn flow: use the
existing tmux call log to assert the export command occurs before the Kimi
launch, and update the fake kimi executable to record the inherited GIT_EDITOR
and GIT_SEQUENCE_EDITOR values, asserting both are true. Keep the regression
coverage in the relevant test case and retain the existing failure diagnostics.
🪄 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: b43285a1-c689-410d-9e70-4948ed93482a
📒 Files selected for processing (3)
bin/fm-brief.shbin/fm-spawn.shtests/fm-kimi-harness.test.sh
| editor waiting on a human, not a slow operation: check with \`pgrep -fl 'COMMIT_EDITMSG|--wait'\` | ||
| and kill the waiter rather than retrying, since each retry stacks another orphaned waiter. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scope blocked-editor recovery to the current task.
The recovery guidance searches the full host process list and does not require process ownership, ancestry, or pane identity before termination. A crewmate can stop another agent's editor or an unrelated --wait process.
bin/fm-brief.sh#L466-L467: require a verified child of the current Git process before terminating a PID.bin/fm-brief.sh#L649-L650: apply the same scoped recovery procedure to ship briefs.
As per coding guidelines, operational shell scripts must use their owning procedures and guarded paths for mutation, merge, teardown, synchronization, and recovery; do not bypass those guards with lower-level commands.
📍 Affects 1 file
bin/fm-brief.sh#L466-L467(this comment)bin/fm-brief.sh#L649-L650
🤖 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-brief.sh` around lines 466 - 467, Scope the editor-waiter recovery
guidance at bin/fm-brief.sh lines 466-467 to the current task by locating only
verified child processes of the current Git process, including pane identity
where available, before terminating a PID; do not use an unrestricted host-wide
search. Apply the identical guarded recovery procedure at bin/fm-brief.sh lines
649-650 for ship briefs.
Source: Coding guidelines
| assert_grep "export GIT_EDITOR=true GIT_SEQUENCE_EDITOR=true" "$CASE_DIR/tmux-calls.log" \ | ||
| "kimi spawn did not export non-interactive git editors into the pane (robots-1xw8)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert export order and inherited environment.
assert_grep proves only that the fake tmux log contains the export text. It passes if the export is sent after the Kimi launch, and it does not prove that the launched process sees either variable. Check call order and record the environment observed by the fake kimi.
As per coding guidelines, **/*.test.* files must add behavioral regression tests when an executable contract exists, especially when correcting a reproduced bug.
🤖 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 `@tests/fm-kimi-harness.test.sh` around lines 212 - 213, Strengthen the test
around the kimi spawn flow: use the existing tmux call log to assert the export
command occurs before the Kimi launch, and update the fake kimi executable to
record the inherited GIT_EDITOR and GIT_SEQUENCE_EDITOR values, asserting both
are true. Keep the regression coverage in the relevant test case and retain the
existing failure diagnostics.
Source: Coding guidelines
1 participant
Problem
A crewmate has no terminal a human can type into, but it inherits the machine's configured git editor — here
bash /usr/local/bin/code --wait, which blocks until a human closes the tab. Any git command that opens an editor hangs forever with no output and no error:rebase --continue/rebase -i,commitwithout-m, non-fast-forwardmerge,revert,tag -a,cherry-pick --continue.A wedged pane looks identical to a thinking pane, so nothing surfaces it until the staleness window expires — and the agent's natural response (retry) stacks more orphaned waiters on the same
COMMIT_EDITMSG.Observed live on task
parlay-45-rebase: three wedged attempts on the same file (waiters aged 7m09s and 5m07s still parked), ~15 min lost. Killing the waiters let git readCOMMIT_EDITMSGas-is; the rebase completed and pushed cleanly, so it was purely an editor deadlock — no work was ever at risk.Fix — two layers
bin/fm-spawn.shnow exportsGIT_EDITOR=true GIT_SEQUENCE_EDITOR=trueinto the crewmate's pane shell alongsideGOTMPDIR, before the harness launch.trueexits 0 without touching the file, so git proceeds with the commit message / rebase todo as written instead of waiting. This makes the whole class impossible rather than asking each worker to remember.bin/fm-brief.shgains rule 8 in both the scout and ship Rules sections: never let git open an editor, and treat a git command that is silent for over a minute as a blocked editor rather than a slow operation.Also lands the fix
violation-0ghasks for, since it is the same one-line brief addition — ship rule 9 tells a worker whose branch is already checked out in another worktree to use a detached HEAD, never to remove or modify that worktree.New rules are appended, not inserted, so the existing "escalate to firstmate (rule 6)" cross-reference in the ship brief stays correct.
Tests
New pane-export assertion in
tests/fm-kimi-harness.test.sh, which already owns fm-spawn's pane environment contract (it is where theGOTMPDIRexport is asserted).bin/fm-test-run.sh --changed --base origin/main→ 36 scripts, 3 failures, all reproduced on cleanorigin/main:fm-arm-pretool-check— "A13 via codex must allow" — fails on baseline.fm-calm-pi-extension— "loaded_off case rendered a duplicate captain answer" — fails on baseline (live-pi E2E).fm-test-run— "scheduler waited for oldest worker" — timing-sensitive (0.5s vs 0.05s fixtures); passes when run alone, flakes under a 36-script load.Closes robots-1xw8. Addresses violation-0gh.
Summary by CodeRabbit
Bug Fixes
Tests