feat(brief): inject fork-first push rule for non-Trillium origins - #12
Conversation
Push-mode ship briefs (direct-PR, no-mistakes) whose project clone has a git origin outside the trillium/ namespace now tell the worker to push its branch to the trillium/<repo> fork and open the PR from there, instead of stalling on a refused upstream push. Detection reads the clone's real origin remote, not registry prose. Trillium-origin, unreadable, or absent origins and every local-only brief are unchanged. Extends tests/fm-brief.test.sh with the rule-present (HTTPS and SSH origins), Trillium rule-absent, and local-only exemption cases.
…upstream-safety guard
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPush-mode ship briefs now detect non-Trillium Git origins and direct workers to push to ChangesFork-first push guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant fm-brief.sh
participant Git
participant ProjectClone
fm-brief.sh->>ProjectClone: Resolve clone path
fm-brief.sh->>Git: Read origin URL
Git-->>fm-brief.sh: Return origin
fm-brief.sh->>fm-brief.sh: Build trillium/<repo> fork rule
fm-brief.sh-->>ProjectClone: Add rule to ship brief
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 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 `@AGENTS.md`:
- Line 469: Do not edit AGENTS.md directly; apply this generated brief behavior
through the selected delivery path using bin/fm-ensure-agents-md.sh. Ensure the
script handles the fork-first rule for eligible push-mode briefs while
preserving exemptions for local-only briefs, Trillium-owned origins, unreadable
origins, and absent clones.
In `@bin/fm-brief.sh`:
- Around line 153-165: Update the origin parsing logic in the repository-name
function to validate the host, owner, and non-empty repository components before
deriving or printing name; reject malformed origins such as
https://github.com/trillium without adding a rule, while preserving the existing
trillium-owner exclusion. Add a regression test covering an origin with a
missing repository segment.
🪄 Autofix (Beta)
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: 5439ed92-a185-47e9-b47c-cfc6628561d5
📒 Files selected for processing (3)
AGENTS.mdbin/fm-brief.shtests/fm-brief.test.sh
| If a task will drive Herdr lifecycle behavior, scaffold with `--herdr-lab`; if that need appears after an unguarded scaffold, stop and regenerate rather than adding commands by hand. | ||
| The generated Herdr contract must use a named non-`default` isolated lab and its guarded helper for every lifecycle action. | ||
| When a task is linked to an external bead (via `--beads <id>` at spawn), set `FM_HOOK_BEADS_ID=<id>` before scaffolding so the brief receives Bead Receipt and Bead Closure sections that guide the worker's interaction with the tracking system. | ||
| For push-mode ship briefs (direct-PR and no-mistakes) on projects whose git origin is not under the trillium/ namespace (upstream forks the worker cannot push to), the generated brief receives a fork-first push rule that directs the worker to push to the `trillium/<repo>` fork and open the PR from there; local-only briefs are exempt, as are briefs for Trillium-owned origins, unreadable origins, or absent clones. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Do not edit AGENTS.md directly.
Apply this addition through the selected delivery path with bin/fm-ensure-agents-md.sh. Do not commit this direct file edit.
As per coding guidelines, Firstmate must not write AGENTS.md directly; contributors must update it lazily through bin/fm-ensure-agents-md.sh.
🤖 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 `@AGENTS.md` at line 469, Do not edit AGENTS.md directly; apply this generated
brief behavior through the selected delivery path using
bin/fm-ensure-agents-md.sh. Ensure the script handles the fork-first rule for
eligible push-mode briefs while preserving exemptions for local-only briefs,
Trillium-owned origins, unreadable origins, and absent clones.
Source: Coding guidelines
| origin=$(git -C "$dir" remote get-url origin 2>/dev/null) || return 0 | ||
| [ -n "$origin" ] || return 0 | ||
| origin=${origin%.git} | ||
| origin=${origin%/} | ||
| name=${origin##*/} | ||
| rest=${origin%/*} | ||
| owner=${rest##*/} # https://host/owner/repo -> owner | ||
| owner=${owner##*:} # git@host:owner/repo -> owner | ||
| case "$(printf '%s' "$owner" | tr '[:upper:]' '[:lower:]')" in | ||
| trillium) return 0 ;; | ||
| esac | ||
| [ -n "$name" ] || return 0 | ||
| printf '%s\n' "$name" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed origin URLs before deriving the fork name.
https://github.com/trillium has no repository segment. This code returns trillium and generates guidance for trillium/trillium. That conflicts with the stated behavior that invalid origins add no rule.
Validate the host, owner, and repository components before printing $name. Add a regression test for an origin with a missing repository component.
Proposed fix
- origin=${origin%.git}
origin=${origin%/}
+ origin=${origin%.git}
+ if [[ ! "$origin" =~ ^(https?|ssh)://[^/]+/[^/]+/[^/]+$ ]] &&
+ [[ ! "$origin" =~ ^[^`@/`:]+@[^/:]+:[^/]+/[^/]+$ ]]; then
+ return 0
+ fi
name=${origin##*/}
rest=${origin%/*}
owner=${rest##*/} # https://host/owner/repo -> owner
owner=${owner##*:} # git@host:owner/repo -> owner
+ [ -n "$owner" ] || return 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| origin=$(git -C "$dir" remote get-url origin 2>/dev/null) || return 0 | |
| [ -n "$origin" ] || return 0 | |
| origin=${origin%.git} | |
| origin=${origin%/} | |
| name=${origin##*/} | |
| rest=${origin%/*} | |
| owner=${rest##*/} # https://host/owner/repo -> owner | |
| owner=${owner##*:} # git@host:owner/repo -> owner | |
| case "$(printf '%s' "$owner" | tr '[:upper:]' '[:lower:]')" in | |
| trillium) return 0 ;; | |
| esac | |
| [ -n "$name" ] || return 0 | |
| printf '%s\n' "$name" | |
| origin=$(git -C "$dir" remote get-url origin 2>/dev/null) || return 0 | |
| [ -n "$origin" ] || return 0 | |
| origin=${origin%/} | |
| origin=${origin%.git} | |
| if [[ ! "$origin" =~ ^(https?|ssh)://[^/]+/[^/]+/[^/]+$ ]] && | |
| [[ ! "$origin" =~ ^[^`@/`:]+@[^/:]+:[^/]+/[^/]+$ ]]; then | |
| return 0 | |
| fi | |
| name=${origin##*/} | |
| rest=${origin%/*} | |
| owner=${rest##*/} # https://host/owner/repo -> owner | |
| owner=${owner##*:} # git@host:owner/repo -> owner | |
| [ -n "$owner" ] || return 0 | |
| case "$(printf '%s' "$owner" | tr '[:upper:]' '[:lower:]')" in | |
| trillium) return 0 ;; | |
| esac | |
| [ -n "$name" ] || return 0 | |
| printf '%s\n' "$name" |
🤖 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 153 - 165, Update the origin parsing logic in
the repository-name function to validate the host, owner, and non-empty
repository components before deriving or printing name; reject malformed origins
such as https://github.com/trillium without adding a rule, while preserving the
existing trillium-owner exclusion. Add a regression test covering an origin with
a missing repository segment.
1 participant
Intent
Inject a fork-first push rule into the ship briefs that bin/fm-brief.sh generates. Several registered firstmate projects are forks: the clone's git origin points at an upstream repo (e.g. kunchenguid/gnhf, david-tejada/rango) that the worker cannot push to, while Trillium maintains a fork at trillium/. Today a worker on such a project hits a refused upstream push and stalls. The change detects, per ship task, whether the project clone's real 'git remote get-url origin' is under the trillium/ namespace; when it is NOT, the generated push-mode brief (direct-PR and no-mistakes) gains a concise fork-first instruction: push the fm/ branch to the trillium/ fork, create the fork with gh-axi if absent, open the PR from the fork, expect the refused origin push, never ask fork-vs-local, never push upstream. Deliberate decisions: (1) detection uses the real git origin remote of the resolved clone, NOT data/projects.md prose, because the coding guidelines call registry free-text an unreliable machine signal; the fork repo name is derived from the origin URL basename and owner-segment parsing handles both https and git@ SSH forms. (2) The rule text lives exactly once as a single shared FORK_FIRST string (one-owner rule) injected once into the shared ship heredoc, gated by mode and origin. (3) local-only is exempt because it never pushes or opens a PR. (4) Trillium-origin, unreadable-origin, and absent-clone cases add NO rule and stay byte-identical to pre-change output (verified: diffing the original HEAD script vs modified for a no-clone project showed only the task-id substitution differing). Built with the IFS= read -r -d '' heredoc pattern to avoid the forbidden heredoc-in-command-substitution class that breaks Bash 3.2 parsing. Tests extend tests/fm-brief.test.sh (not a new runner) covering rule-present for https and ssh origins, Trillium rule-absent, and local-only exemption. bin/fm-lint.sh passes. Out of scope: fm-spawn.sh, fm-teardown.sh, push/merge scripts, and data/projects.md were not touched.
What Changed
trillium/<repo>and open the PR from the fork, preventing refused upstream pushes.Risk Assessment
✅ Low: The change is well-bounded and correct: fork detection uses git origin reading (not unreliable prose), the rule injects only for non-Trillium push modes, scout and local-only are properly excluded, and the second commit's reword correctly makes guidance mode-agnostic while adding safety guardrails.
Testing
Comprehensive validation of fork-first push rule injection: all 18 automated tests pass including fork-first-specific test, 5 manual test scenarios confirm correct rule injection for push modes on upstream origins and correct rule absence for Trillium/local-only/nonexistent cases, URL parsing validated for both HTTPS and SSH formats, Bash 3.2 parse safety confirmed with safe heredoc pattern, ShellCheck linting passes cleanly.
Evidence: Fork-First Rule Test Evidence
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-brief.sh:413- The fork-first rule is injected into both push modes, but no-mistakes mode ships the PR via the pipeline, not the worker. The no-mistakes DOD (fm-brief.sh:373-388) has the worker run /no-mistakes and let the pipeline push and open the PR, and forbids the worker from acting directly during a run. The injected rule (fm-brief.sh:413) instead tells the worker to manually push itsfm/<id>branch to the trillium fork and open the PR from there. If the no-mistakes pipeline pushes toorigin(the unpushable upstream), the brief text does not resolve the stall for the default mode; and a worker following the rule literally may open a fork PR while the pipeline separately attempts an origin push, yielding a refused push or conflicting/duplicate PRs. Confirm no-mistakes honors a fork remote (or scope the rule to direct-PR / add pipeline-specific guidance).🔧 Fix: Reword fork-first rule to be mode-agnostic with upstream-safety guard
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-brief.test.sh test_fork_first_push_rule (all 18 tests pass)bash tests/fm-brief.test.sh | grep fork-first (fork-first-specific test passes)bin/fm-lint.sh (ShellCheck 0.11.0 - no warnings)Manual verification: 5 test cases - HTTPS upstream (rule present, correct fork), SSH upstream (rule present, correct fork), Trillium-owned (rule absent), local-only exemption (rule absent), nonexistent clone (rule absent)URL parsing verification: HTTPS with/without .git, SSH with/without .git, Trillium detection (case-insensitive), all correctBash 3.2 parse safety: safe heredoc-in-read pattern verified✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
New Features
Bug Fixes
Tests