fix(fm-send): refuse from-firstmate marked slash commands - #91
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesSecondmate command delivery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/architecture.md`:
- Line 196: Update the documentation sentence around the marked slash-command
refusal to also state that bin/fm-send.sh refuses marked $<skill> commands when
the target harness is Codex. Retain the existing recovery guidance, including
bin/fm-teardown.sh and the never-marked explicit backend target for direct
harness driving.
🪄 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: 5c898e28-afee-4a19-bf69-9f209be33b9f
📒 Files selected for processing (6)
.agents/skills/harness-adapters/SKILL.mdbin/fm-pending-reply-lib.shbin/fm-send.shdocs/architecture.mdtests/fm-send-popup-settle.test.shtests/fm-send-secondmate-marker.test.sh
trillium
pushed a commit
that referenced
this pull request
Aug 6, 2026
…sh refusal CodeRabbit on #91: the carrier paragraph documented only the marked slash-command refusal, but fm-send.sh also refuses a marked $<skill> command when the target's meta records harness=codex. State that condition so the doc matches the code, and keep the existing recovery guidance (fm-teardown for a close, the never-marked explicit backend target to drive the harness directly). Refs: robots-u7gu
trillium
changed the title
Owner
Author
|
@coderabbitai review Re-requesting: the previous run reported |
|
|
added 3 commits
August 7, 2026 09:27
…ing them as prose The from-firstmate carrier occupies column 0 of the composer line, and every verified harness parses a command only when its slash is the line's first character. A slash command to a kind=secondmate target therefore arrived as ordinary prose: the agent read it, sometimes narrated compliance, and the command never ran - while fm-send reported a verified submit and opened a pending-reply expectation for it. The tell was already in the code: the popup-settle matched on the PRE-marker arguments, so fm-send paid a 1.2s slash-popup settle for a line that could not open a popup. Refuse that send loudly, naming the carrier as the cause and the two working paths (fm-teardown to close an agent; the never-marked explicit backend target to drive its harness directly). The check runs on the body the secondmate would actually read, so an already marked and correlated recovery resend cannot slip the same command through, and it runs before the pending-reply record is created, so a refusal leaves no orphan expectation. Crewmate targets, explicit endpoints, the --key path, and prose that merely contains or is indented before a slash are all unaffected. Also select the popup-settle from the final text rather than the arguments, so a marked line takes the fast path instead of paying a settle for a popup that cannot open. Refs: robots-u7gu
…sh refusal CodeRabbit on #91: the carrier paragraph documented only the marked slash-command refusal, but fm-send.sh also refuses a marked $<skill> command when the target's meta records harness=codex. State that condition so the doc matches the code, and keep the existing recovery guidance (fm-teardown for a close, the never-marked explicit backend target to drive the harness directly). Refs: robots-u7gu
trillium
force-pushed
the
fix/robots-u7gu
branch
from
August 7, 2026 16:28
c2b8672 to
c893201
Compare
trillium
added a commit
to trillium/parlay
that referenced
this pull request
Aug 8, 2026
…-bn5d) Every rule the gate asserts is evaluated against origin's headRefOid, which is correct — that is the commit a merge lands. But the caller is a mechanic who has just authored a fix and pushed it, and for whom READY reads as "my fix is cleared to merge". `git push no-mistakes` goes to the MIRROR, and the pipeline pushes on to origin asynchronously. On trillium/firstmate#91 that gap was live: origin's head was still the pre-fix commit while the gate returned exit 0 READY. Merging there would have landed the old head and silently DROPPED the just-authored fix for the reviewer's finding — the exact premature-FIXED failure the mechanic guardrails exist to prevent, with the gate pointing at it. Once the pipeline did push, the same PR went 0 -> 4, so the READY was not merely early; it was the opposite of the eventual answer. The gate cannot see a mirror or a pipeline run. It can see, from any checkout or linked worktree of the repo, that the local branch holds commits origin's PR head does not — the same fact from the side it has access to, and true for a plain forgotten push too. detectHeadFreshness measures that, pinned to the already-resolved repo (robots-g4qz) so a same-named branch in an unrelated checkout can never invent a blocker. - ahead/diverged -> head-not-pushed, pending-class, exit 5. Nothing is wrong with the diff; the commits have not arrived and the answer changes on its own. - MERGED + ahead -> code-class, exit 3. The stale merge already happened, waiting cannot undo it, and `git branch -r --contains` passes for the wrong commit. - behind -> a note only; a stale checkout risks nothing. - unverifiable -> say so with the reason, and hand over the check the gate could not run. "Could not tell" is not "they agree". Origin's head sha is now printed on every verdict, including the READY line — the ticket's minimum ask, and the one thing that made the two commits distinguishable at a glance. Exit 5 now has two shapes with opposite instructions, so its notes name whichever is present rather than always printing the review-in-flight script. Class precedence is unchanged: one code blocker still keeps the whole verdict at 3.
trillium
added a commit
to trillium/parlay
that referenced
this pull request
Aug 20, 2026
…-bn5d) Every rule the gate asserts is evaluated against origin's headRefOid, which is correct — that is the commit a merge lands. But the caller is a mechanic who has just authored a fix and pushed it, and for whom READY reads as "my fix is cleared to merge". `git push no-mistakes` goes to the MIRROR, and the pipeline pushes on to origin asynchronously. On trillium/firstmate#91 that gap was live: origin's head was still the pre-fix commit while the gate returned exit 0 READY. Merging there would have landed the old head and silently DROPPED the just-authored fix for the reviewer's finding — the exact premature-FIXED failure the mechanic guardrails exist to prevent, with the gate pointing at it. Once the pipeline did push, the same PR went 0 -> 4, so the READY was not merely early; it was the opposite of the eventual answer. The gate cannot see a mirror or a pipeline run. It can see, from any checkout or linked worktree of the repo, that the local branch holds commits origin's PR head does not — the same fact from the side it has access to, and true for a plain forgotten push too. detectHeadFreshness measures that, pinned to the already-resolved repo (robots-g4qz) so a same-named branch in an unrelated checkout can never invent a blocker. - ahead/diverged -> head-not-pushed, pending-class, exit 5. Nothing is wrong with the diff; the commits have not arrived and the answer changes on its own. - MERGED + ahead -> code-class, exit 3. The stale merge already happened, waiting cannot undo it, and `git branch -r --contains` passes for the wrong commit. - behind -> a note only; a stale checkout risks nothing. - unverifiable -> say so with the reason, and hand over the check the gate could not run. "Could not tell" is not "they agree". Origin's head sha is now printed on every verdict, including the READY line — the ticket's minimum ask, and the one thing that made the two commits distinguishable at a glance. Exit 5 now has two shapes with opposite instructions, so its notes name whichever is present rather than always printing the review-in-flight script. Class precedence is unchanged: one code blocker still keeps the whole verdict at 3.
trillium
added a commit
to trillium/parlay
that referenced
this pull request
Aug 20, 2026
…-bn5d) Every rule the gate asserts is evaluated against origin's headRefOid, which is correct — that is the commit a merge lands. But the caller is a mechanic who has just authored a fix and pushed it, and for whom READY reads as "my fix is cleared to merge". `git push no-mistakes` goes to the MIRROR, and the pipeline pushes on to origin asynchronously. On trillium/firstmate#91 that gap was live: origin's head was still the pre-fix commit while the gate returned exit 0 READY. Merging there would have landed the old head and silently DROPPED the just-authored fix for the reviewer's finding — the exact premature-FIXED failure the mechanic guardrails exist to prevent, with the gate pointing at it. Once the pipeline did push, the same PR went 0 -> 4, so the READY was not merely early; it was the opposite of the eventual answer. The gate cannot see a mirror or a pipeline run. It can see, from any checkout or linked worktree of the repo, that the local branch holds commits origin's PR head does not — the same fact from the side it has access to, and true for a plain forgotten push too. detectHeadFreshness measures that, pinned to the already-resolved repo (robots-g4qz) so a same-named branch in an unrelated checkout can never invent a blocker. - ahead/diverged -> head-not-pushed, pending-class, exit 5. Nothing is wrong with the diff; the commits have not arrived and the answer changes on its own. - MERGED + ahead -> code-class, exit 3. The stale merge already happened, waiting cannot undo it, and `git branch -r --contains` passes for the wrong commit. - behind -> a note only; a stale checkout risks nothing. - unverifiable -> say so with the reason, and hand over the check the gate could not run. "Could not tell" is not "they agree". Origin's head sha is now printed on every verdict, including the READY line — the ticket's minimum ask, and the one thing that made the two commits distinguishable at a glance. Exit 5 now has two shapes with opposite instructions, so its notes name whichever is present rather than always printing the review-in-flight script. Class precedence is unchanged: one code blocker still keeps the whole verdict at 3.
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
1 participant
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
{"summary": "The developer set out to fix a defect in the fm-send message-delivery path so that slash commands tagged with a secondmate marker are refused rather than being sent through as ordinary prose text. Their intent was to add explicit detection of the secondmate marker in fm-send (and its pending-reply library) and block those marked slash commands from delivery. They also wanted regression tests covering this behavior, adding fm-send-popup-settle and fm-send-secondmate-marker test cases. Finally, they intended to update the architecture documentation and the harness-adapters skill guide to reflect the new secondmate command-handling behavior."}
What Changed
bin/fm-send.shnow detects when a from-firstmate marked message body is a slash command (or a codex$<skill>command targeting the codex harness) and refuses delivery with a loud error, since the column-0 carrier makes the harness read the line as prose so the command never runs while a pending-reply expectation would otherwise be opened; the recovery guidance points tofm-teardownor an explicit backend target. The popup-settlecasenow matches on the final$MESSAGErather than raw args so marked lines take the fast settle.bin/fm-pending-reply-lib.shaddsfm_pending_reply_carrier_body, a byte-exact helper that strips the from-firstmate carrier and any leadingcorr=<16hex>token so the refusal check reasons about the body the secondmate actually reads (letting already-marked recovery resends be judged on their command).tests/fm-send-secondmate-marker.test.shand extendedtests/fm-send-popup-settle.test.sh; updateddocs/architecture.mdand the harness-adapters skill guide to document the new secondmate command-refusal behavior.Risk Assessment
✅ Low: A well-bounded guard added ahead of pending-reply creation with behavioral regression tests and docs; unmarked crewmate/explicit-endpoint paths are unchanged and the durable fix covers every marked-send path I could reach.
Testing
Ran the two targeted test files tied to the intent (fm-send-secondmate-marker and fm-send-popup-settle) — all 22 assertions pass. Confirmed the new secondmate-marker test is a true regression by reverting the source to the base commit, where it fails (a marked/exitwas delivered, exit 0, instead of refused), then restoring the fix. Captured reviewer-visible end-user evidence as a live fm-send.sh CLI transcript showing marked slash and codex$<skill>commands refused with the explanatory diagnostic and zero bytes typed, while slash-bearing prose and$pricetext still deliver. Per instructions I did not run the full suite; broad regression remains for CI. This is a CLI-facing change (no rendered UI surface), so the transcript is the appropriate product-level artifact. Temp scripts were removed and the worktree is clean.Evidence: Live fm-send.sh refusal CLI transcript (robots-u7gu)
$ fm-send fm-domain "/exit" # claude secondmate error: refusing to send the slash command '/exit' to secondmate domain: from-firstmate marked text carries '[fm-from-firstmate]' at column 0 ... [exit code: 1] [composer log bytes typed: 0] $ fm-send fm-cx "$no-mistakes" # codex secondmate skill command error: refusing to send the codex skill command '$no-mistakes' to secondmate cx ... [exit code: 1] [composer log bytes typed: 0] --- control: slash-bearing PROSE still delivers --- [exit code: 0] [composer received: [fm-from-firstmate]corr=... see /Users/x/log for the failure] --- control: codex "$5/month" price still delivers --- [exit code: 0] [composer received: [fm-from-firstmate]corr=... $5/month is cheap]Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-send.sh:327- The codex '$<skill>' refusal matches only\$[a-z]*, so a marked codex command whose skill name begins with an uppercase letter, underscore, or digit (e.g.$_DOTFILES,$ApertureOscillation— such skill names exist in this environment) is NOT refused and is still silently delivered as prose to the secondmate: the exact defect class the change fixes. This is an accepted heuristic tradeoff rather than a clear bug — broadening to uppercase would also refuse$HOME/$PATH-style env-var prose, which the author deliberately keeps deliverable (see comment at fm-send.sh:363-366 and SKILL.md:218). The/(universal) form has no such gap. Flagging so the codex-$narrowing is a conscious choice.bin/fm-pending-reply-lib.sh:228- The leadingcorr=<16hex>+ blanks stripping block is duplicated verbatim betweenfm_pending_reply_embed_corr(lines 205-213) and the newfm_pending_reply_carrier_body(lines 228-236). Extracting the shared strip into one helper would remove the copy and keep the two framing/inverse-framing functions in sync if the corr token format ever changes. Non-functional simplification only.✅ **Test** - passed
✅ No issues found.
bash tests/fm-send-secondmate-marker.test.sh— 13 assertions pass (marked slash/codex-skill refused; lookalike prose,$prices, non-codex$bodies, crewmate and explicit-endpoint commands still send)bash tests/fm-send-popup-settle.test.sh— 9 assertions pass, incl. new codex-secondmate$pricefast-path settle caseRegression proof: reverted only bin/fm-send.sh + bin/fm-pending-reply-lib.sh to base 80058d2 and reran the new test —a slash command to a secondmate should be refused: expected exit 1, got 0(bug reproduced), then restored to target commitLive end-to-end CLI transcript driving the realbin/fm-send.shagainst hermetic claude/codex secondmate targets:/exitand$no-mistakesrefused (exit 1, 0 composer bytes); slash-bearing prose and$5/monthdelivered (exit 0, composer received marked text)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
Bug Fixes
$<skill>commands when delivery cannot preserve command parsing.Documentation
Tests