fix(brief): default fork-contribution PRs to the captain's fork instead of upstream - #34
Conversation
…in's fork The rango PR kunchenguid#712 incident happened because the existing fork-first rule in fm-brief.sh's scaffold was prose-only and didn't survive contact with no-mistakes's default PR-target behavior or the (backwards, for this use case) 'no-mistakes init --fork-url' flag. - bin/fm-brief.sh: detect both the legacy (origin still upstream) and swapped (origin=trillium fork, upstream remote present) clone shapes, and scaffold mode- and shape-specific instructions: explicit gh pr create --repo trillium/<repo> form for direct-PR, a confirm-and-forbid-fork-url rule for an already-swapped no-mistakes clone, and a stop-and-escalate instruction when a no-mistakes clone hasn't been swapped yet (no safe worker-side override exists in that shape). - .agents/skills/project-management/SKILL.md: document the required origin=fork/upstream=original clone shape for fork-contribution projects, and call out that --fork-url implements the opposite workflow. - AGENTS.md: update section 11's one-line summary to match. - bin/fm-fork-origin-check.sh: new read-only advisory scan for registered clones stuck in the legacy or partially-swapped shape. - tests/fm-brief.test.sh, tests/fm-fork-origin-check.test.sh: cover every clone/mode combination.
…in scripts inventory and skill
📝 WalkthroughWalkthroughFork-contribution handling now distinguishes legacy and correctly swapped Git remotes. Brief generation applies clone-state and delivery-mode rules. A read-only diagnostic scanner reports incomplete configurations and includes regression coverage. ChangesFork origin workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 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
🧹 Nitpick comments (2)
bin/fm-fork-origin-check.sh (2)
50-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the owner-parsing logic instead of duplicating it.
owner_ofreimplements the same owner-extraction steps already present inbin/fm-brief.sh'sfork_repo_for_origin(strip.git, strip trailing/, split on the last/, then on the last:). Two independent copies of this URL-parsing logic risk silently diverging if an edge case (new host format, unusual URL shape) is fixed in only one file. This codebase already uses a*-lib.shconvention for shared helpers between scripts (see the manyfm-*-lib.shentries indocs/scripts.md); extract this logic into such a shared file and source it from bothbin/fm-brief.shandbin/fm-fork-origin-check.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 `@bin/fm-fork-origin-check.sh` around lines 50 - 58, Extract the shared owner-parsing steps from owner_of and fork_repo_for_origin into an appropriate bin/*-lib.sh helper, preserving the existing normalization behavior. Source that shared library from both bin/fm-fork-origin-check.sh and bin/fm-brief.sh, and update both callers to reuse the helper instead of maintaining duplicate parsing logic.
78-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the
gh repo viewcall with a timeout.The best-effort GitHub lookup has no timeout. A network stall on any single registered project would hang the entire triage scan rather than degrading gracefully to "skip, never misreport" as the header describes for other failure modes.
🛡️ Proposed fix to bound the external call
- is_fork=$(gh repo view "trillium/$name" --json isFork -q .isFork 2>/dev/null) || continue + is_fork=$(timeout 10 gh repo view "trillium/$name" --json isFork -q .isFork 2>/dev/null) || continue🤖 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-fork-origin-check.sh` around lines 78 - 83, Bound the `gh repo view` invocation in the fork-detection loop with a short timeout, preserving the existing failure path that skips the project when the command times out or otherwise fails. Keep the `is_fork` check and candidate reporting unchanged so stalled lookups degrade to skipping rather than blocking the scan.
🤖 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-fork-origin-check.sh`:
- Around line 84-86: Update the project-name extraction in the here-document
within the fork-origin check flow to reuse the registry parser contract provided
by bin/fm-project-mode.sh instead of the inline awk expression. Preserve the
existing iteration behavior while ensuring data/projects.md entries are
interpreted through that parser’s supported registry format.
---
Nitpick comments:
In `@bin/fm-fork-origin-check.sh`:
- Around line 50-58: Extract the shared owner-parsing steps from owner_of and
fork_repo_for_origin into an appropriate bin/*-lib.sh helper, preserving the
existing normalization behavior. Source that shared library from both
bin/fm-fork-origin-check.sh and bin/fm-brief.sh, and update both callers to
reuse the helper instead of maintaining duplicate parsing logic.
- Around line 78-83: Bound the `gh repo view` invocation in the fork-detection
loop with a short timeout, preserving the existing failure path that skips the
project when the command times out or otherwise fails. Keep the `is_fork` check
and candidate reporting unchanged so stalled lookups degrade to skipping rather
than blocking the scan.
🪄 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: badc82d2-9d47-43ca-8dd6-219c4a543833
📒 Files selected for processing (7)
.agents/skills/project-management/SKILL.mdAGENTS.mdbin/fm-brief.shbin/fm-fork-origin-check.shdocs/scripts.mdtests/fm-brief.test.shtests/fm-fork-origin-check.test.sh
| done <<EOF | ||
| $(awk '$1=="-"{print $2}' "$REG") | ||
| EOF |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -t f '^fm-project-mode\.sh$' bin)
echo "== bin/fm-project-mode.sh header (parser contract) =="
sed -n '1,60p' "$file"
echo "== fm-fork-origin-check.sh registry read =="
rg -n 'awk|REG' bin/fm-fork-origin-check.shRepository: trillium/firstmate
Length of output: 2827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fm-fork-origin-check.sh relevant sections =="
sed -n '1,130p' bin/fm-fork-origin-check.sh
echo "== data/projects.md sample/line count =="
for f in data/projects.md **/projects.md; do
[ -n "${f:-}" ] && [ -f "$f" ] || continue
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,120p' "$f"
done
echo "== project-management skill registry guidance =="
skill=$(fd -t f '^SKILL\.md$' .agents/skills/project-management)
for s in "${skill[@]:-}"; do
[ -n "$s" ] || continue
echo "--- $s ---"
sed -n '1,220p' "$s"
done
echo "== all fm-fork-origin-check.sh awk invocations =="
awk 'BEGIN{p=0} /awk \'{print NR": "$0} /awk '\''\$1=="-"\{print \$2'\'' "$REG"\n/ /EOF/' bin/fm-fork-origin-check.shRepository: trillium/firstmate
Length of output: 11766
Use bin/fm-project-mode.sh’s registry parser contract here.
bin/fm-fork-origin-check.sh still extracts project names with awk '$1=="-"{print $2}', but the project-management skill requires data/projects.md readers to use the registry format and parser contract owned by bin/fm-project-mode.sh. This simplified extraction does not follow that contract and may miss or misread registry line shapes intended by bin/fm-project-mode.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 `@bin/fm-fork-origin-check.sh` around lines 84 - 86, Update the project-name
extraction in the here-document within the fork-origin check flow to reuse the
registry parser contract provided by bin/fm-project-mode.sh instead of the
inline awk expression. Preserve the existing iteration behavior while ensuring
data/projects.md entries are interpreted through that parser’s supported
registry format.
1 participant
Intent
Fix fork-contribution PRs defaulting to upstream instead of the captain's fork: fm-brief.sh now detects legacy vs swapped clone shapes and scaffolds explicit gh pr create --repo trillium/ / stop-and-escalate instructions per mode; project-management skill documents the required origin=fork/upstream=original clone shape; new bin/fm-fork-origin-check.sh advisory scan; tests added for both.
What Changed
bin/fm-brief.shnow detects legacy vs. correctly-swapped fork clone shapes and scaffolds mode-specific instructions (explicitgh pr create --repo trillium/<repo>, stop-and-escalate on an unset-up clone) so push-mode ship briefs on fork-contribution projects stop defaulting PRs to upstream.bin/fm-fork-origin-check.sh, a read-only advisory scan that classifies a clone's origin/upstream remote shape, plus its test suite (tests/fm-fork-origin-check.test.sh) and adocs/scripts.mdinventory entry.origin=fork/upstream=originalclone convention in.agents/skills/project-management/SKILL.mdand updatedAGENTS.md; extendedtests/fm-brief.test.shto cover legacy, swapped, direct-PR, and exempt clone shapes.Risk Assessment
✅ Low: The change is additive prose/tooling: it extends fork-detection logic to two clone shapes (legacy/swapped) with mode-specific brief text, adds a new standalone read-only advisory script not wired into any automatic execution path, and preserves byte-identical brief output for ordinary (non-fork) projects. Logic was traced end-to-end (heredoc parsing, remote-owner detection, mode branching) with no bugs found, and both new/changed behaviors have direct test coverage.
Testing
Ran the two relevant automated suites (fm-brief.test.sh, fm-fork-origin-check.test.sh) — all cases pass — then independently reproduced the scenario end-to-end outside the test harness by creating three real git clones (legacy unswapped, correctly swapped, and SSH-origin) and generating actual briefs via bin/fm-brief.sh: the legacy no-mistakes clone correctly gets a stop-and-escalate instruction, the swapped no-mistakes clone gets a confirm-and-proceed instruction, and the direct-PR clone gets an explicitgh pr create --repo trillium/<repo>override — matching the intended fix. The advisory scanner also ran cleanly against the same clones. No issues found; no leftover artifacts in the working tree.Evidence: End-to-end demo: generated brief fork-rule sections for legacy/swapped/direct-PR clone shapes plus advisory scan output
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-brief.test.sh(all cases pass, including the new fork-contribution PR-target rule test covering legacy/swapped/direct-PR/exempt clone shapes)bash tests/fm-fork-origin-check.test.sh(all 3 new test cases pass: full clone-shape classification, read-only/always-exit-0 behavior, missing-registry handling)Manual end-to-end: ranbin/fm-brief.shagainst three freshly created git clones (legacy unswapped upstream-origin clone, correctly swapped origin=fork/upstream=original clone, and an SSH-origin direct-PR clone) and inspected the generated brief.md fork-contribution sectionsManual end-to-end: ranbin/fm-fork-origin-check.shagainst the same demo clones to confirm the advisory scan runs cleanly and reports as expected✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit