fix(bugfix): improve PR skill reliability and reduce redundancy#79
fix(bugfix): improve PR skill reliability and reduce redundancy#79jwm4 merged 3 commits intoambient-code:mainfrom
Conversation
WalkthroughReworked the bugfix PR skill control flow to determine authentication context up-front, added a pre-flight gate, moved and renumbered fork/branch/PR steps, added Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LocalGit as Local Git/Workspace
participant GHCLI as gh CLI
participant GitHub as GitHub (remote)
participant Fork as Fork repo
User->>GHCLI: run /pr skill
GHCLI->>GHCLI: Step 0: auth checks (gh auth status, gh api user, installation checks)\ncompute AUTH_TYPE
GHCLI->>User: print Pre-flight Gate table (Step 2e)
alt AUTH_TYPE = none
GHCLI->>User: instruct auth setup steps
end
GHCLI->>GitHub: ensure fork exists / maybe create fork
GHCLI->>LocalGit: create branch, stage selective changes, commit (include PR description in body)
GHCLI->>GHCLI: run gh auth setup-git
LocalGit->>Fork: git push branch
GHCLI->>GitHub: attempt gh pr create --head FORK_OWNER:BRANCH_NAME
alt gh pr create succeeds
GitHub->>User: PR URL
else gh pr create fails
GHCLI->>User: provide compare-URL with URL-encoded title/body (Rung 2)
GHCLI->>User: provide clone-and-checkout commands for testing locally
end
GHCLI->>User: final confirm/report (include required fields when reporting PR for ACP)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 the current code and only fix it if needed.
Inline comments:
In `@workflows/bugfix/.claude/skills/pr/SKILL.md`:
- Around line 59-60: The BRANCH_NAME example and its usages are inconsistent and
can produce refs like bugfix/bugfix/..., so normalize branch handling: decide
whether BRANCH_NAME should contain the full ref or only the short name and make
all consumers consistent (either remove the example's "bugfix/" prefix or change
the consumers that prepend "bugfix/" to first strip any leading "bugfix/").
Update all places that build push/head/compare URLs (the code that concatenates
the branch prefix and BRANCH_NAME) to check startsWith("bugfix/") and only
prepend the prefix when missing, or canonicalize BRANCH_NAME by removing any
leading "bugfix/" once at input, so comparisons and pushes never double-prefix.
- Around line 177-187: The fenced pre-flight block in SKILL.md (the "Pre-flight
summary" table) is missing a language identifier causing markdownlint MD040;
update the opening fence for that block (the triple backticks before "Pre-flight
summary") to include a language label such as text (e.g., change ``` to ```text)
so the block is recognized as a fenced code block with a language and the MD040
warning is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31d297ea-05fc-4c2b-ae3b-1d2981688327
📒 Files selected for processing (23)
AGENTS.mdWORKFLOW_DEVELOPMENT_GUIDE.mdworkflows/bugfix/.ambient/ambient.jsonworkflows/bugfix/.claude/commands/assess.mdworkflows/bugfix/.claude/commands/diagnose.mdworkflows/bugfix/.claude/commands/document.mdworkflows/bugfix/.claude/commands/fix.mdworkflows/bugfix/.claude/commands/pr.mdworkflows/bugfix/.claude/commands/reproduce.mdworkflows/bugfix/.claude/commands/review.mdworkflows/bugfix/.claude/commands/test.mdworkflows/bugfix/.claude/skills/assess/SKILL.mdworkflows/bugfix/.claude/skills/controller/SKILL.mdworkflows/bugfix/.claude/skills/diagnose/SKILL.mdworkflows/bugfix/.claude/skills/document/SKILL.mdworkflows/bugfix/.claude/skills/fix/SKILL.mdworkflows/bugfix/.claude/skills/pr/SKILL.mdworkflows/bugfix/.claude/skills/reproduce/SKILL.mdworkflows/bugfix/.claude/skills/review/SKILL.mdworkflows/bugfix/.claude/skills/speedrun/SKILL.mdworkflows/bugfix/.claude/skills/test/SKILL.mdworkflows/bugfix/CLAUDE.mdworkflows/bugfix/README.md
💤 Files with no reviewable changes (8)
- workflows/bugfix/.claude/commands/pr.md
- workflows/bugfix/.claude/commands/assess.md
- workflows/bugfix/.claude/commands/review.md
- workflows/bugfix/.claude/commands/diagnose.md
- workflows/bugfix/.claude/commands/reproduce.md
- workflows/bugfix/.claude/commands/document.md
- workflows/bugfix/.claude/commands/test.md
- workflows/bugfix/.claude/commands/fix.md
Address four open issues with the PR skill: - ambient-code#47: Add gh auth setup-git before git push to configure credential helper in fresh environments; add error recovery table entry - ambient-code#54: Trim redundant git basics from Step 5 and flag explanations / inline PR body template from Step 7 (platform system instructions already cover basic git workflow) - ambient-code#72: Auto-populate PR description via two complementary approaches: include PR body in commit message (GitHub auto-fills from single commits) and encode title+body as URL query params in compare URL - ambient-code#73: Add clone-and-checkout commands to Rung 2 fallback so users can test the branch locally before creating the PR manually Net effect: 54 insertions, 77 deletions — skill is shorter despite new functionality. Fixes ambient-code#47, fixes ambient-code#54, fixes ambient-code#72, fixes ambient-code#73 Made-with: Cursor
The agent was skipping pre-flight checks entirely. Add structural guardrails to prevent this: - Add Step 0 (Determine Auth Context) as a standalone first step so auth type is resolved before anything else - Add Step 2e (Pre-flight Gate) requiring the agent to print a filled-in summary table before proceeding — makes it structurally hard to skip checks - Add "Expected Flow by Auth Type" section so the agent knows what failures to expect downstream instead of improvising - Fix step numbering: Option 3 (no auth) references Steps 5–6, not 3–6, since fork steps require gh - Soften bot-failure language from "will almost certainly fail" to "may fail" — some repos grant bots sufficient permissions - Document Custom Workflow fields in AGENTS.md so agents always report them after creating a PR Made-with: Cursor
jwm4
force-pushed
the
feature/improve-pr-skill
branch
from
d5428d7 to
be60dd2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
workflows/bugfix/.claude/skills/pr/SKILL.md (1)
59-59:⚠️ Potential issue | 🔴 CriticalCritical: Branch placeholder semantics create invalid refs.
The
BRANCH_NAMEexample includes thebugfix/prefix, but Lines 395, 438, 467, 489, and 516 all prependbugfix/again. This produces malformed refs likebugfix/bugfix/issue-42-null-check, causing push/PR creation failures.🔧 Proposed fix
Either remove the prefix from the example:
-| `BRANCH_NAME` | Step 5: the branch you create | `bugfix/issue-42-null-check` | +| `BRANCH_NAME` | Step 5: the branch you create | `issue-42-null-check` |Or remove it from all consumer sites (Lines 395, 438, 467, 489, 516):
-git checkout -b bugfix/BRANCH_NAME +git checkout -b BRANCH_NAMEAlso applies to: 395-395, 438-438, 467-467, 489-489, 516-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflows/bugfix/.claude/skills/pr/SKILL.md` at line 59, The BRANCH_NAME example currently includes the "bugfix/" prefix, which combined with consumer code that prepends "bugfix/" produces malformed refs like "bugfix/bugfix/issue-42-null-check"; fix by either removing the "bugfix/" prefix from the BRANCH_NAME example in the table row that shows `BRANCH_NAME` or by removing the extra "bugfix/" prefix in all consumers that prepend it (those places that currently construct refs resulting in "bugfix/bugfix/..."); ensure the final branch placeholder is a single canonical value so refs are formed as "bugfix/issue-42-null-check" (or consumers create the full name, not both).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@workflows/bugfix/.claude/skills/pr/SKILL.md`:
- Line 59: The BRANCH_NAME example currently includes the "bugfix/" prefix,
which combined with consumer code that prepends "bugfix/" produces malformed
refs like "bugfix/bugfix/issue-42-null-check"; fix by either removing the
"bugfix/" prefix from the BRANCH_NAME example in the table row that shows
`BRANCH_NAME` or by removing the extra "bugfix/" prefix in all consumers that
prepend it (those places that currently construct refs resulting in
"bugfix/bugfix/..."); ensure the final branch placeholder is a single canonical
value so refs are formed as "bugfix/issue-42-null-check" (or consumers create
the full name, not both).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01140ac5-843e-4150-8539-799354a2ac08
📒 Files selected for processing (2)
AGENTS.mdworkflows/bugfix/.claude/skills/pr/SKILL.md
Address CodeRabbit review feedback on PR ambient-code#79: - Make BRANCH_NAME the full branch name (e.g. bugfix/issue-42-null-check) and remove redundant bugfix/ prefix from all consumers to prevent double-prefixing like bugfix/bugfix/... - Add language label to pre-flight gate fenced code block (MD040) Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workflows/bugfix/.claude/skills/pr/SKILL.md`:
- Around line 431-439: Replace the hardcoded push remote in the Step 7
instruction ("git push -u fork BRANCH_NAME") with a dynamic remote that uses
whatever remote was configured earlier in Step 4 (the fork remote may be named
"origin" or "fork"); update the text to reference a variable remote (e.g., "git
push -u <FORK_REMOTE> BRANCH_NAME" or "git push -u <remote> BRANCH_NAME") and/or
instruct the user to use the remote name they set in Step 4, ensuring the push
command uses the same remote name as configured previously.
- Around line 81-83: Update the documentation and any code examples that derive
GH_USER from the first entry of the /installation/repositories response
(referenced in the text around lines 81–83) to instead fetch the installation
details via GET /app/installations/{installation_id} and use the account.login
field as the installation owner; change all mentions of “repository owner” or
deriving GH_USER from /installation/repositories to “installation account
(account.login)” and show using the installation endpoint for accurate user vs
organization ownership and downstream fork/auth checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9009be8-bcf0-4855-b336-d1263394a9b1
📒 Files selected for processing (1)
workflows/bugfix/.claude/skills/pr/SKILL.md
| The `/installation/repositories` endpoint works because GitHub Apps are | ||
| installed on user accounts — the repo owner is the actual user. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub Apps, can installations be owned by organizations as well as users, and what is the recommended way to determine the correct account/login to use for repository operations?
💡 Result:
Yes, GitHub App installations can be owned by both individual users and organizations. Official documentation confirms that installations occur on user or organization accounts, with the installation object including an account field containing the login of the owning user or organization, and a target_type field indicating "User" or "Organization". The recommended way to determine the correct account/login for repository operations is to fetch the installation details using the GitHub REST API (e.g., GET /app/installations/{installation_id}) and use the account.login from the response. This provides the owner (user or org login) associated with the installation, which is used in repository owner fields for API calls (e.g., /repos/{owner}/{repo}). For authentication, generate an installation access token for that installation and use it with "Authorization: Bearer TOKEN" for repository API operations or Git operations.
Citations:
- 1: https://docs.github.com/en/rest/apps/installations
- 2: https://docs.github.com/en/apps/using-github-apps/installing-a-github-app-from-github-marketplace-for-your-organizations
- 3: https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/authenticating-as-a-github-app-installation
- 4: https://github3.readthedocs.io/en/latest/api-reference/apps.html
- 5: https://docs.github.com/en/enterprise-server@3.0/rest/reference/apps
🏁 Script executed:
find . -name "SKILL.md" -path "*/pr/*" | head -20Repository: ambient-code/workflows
Length of output: 111
🏁 Script executed:
cat -n ./workflows/bugfix/.claude/skills/pr/SKILL.md | sed -n '70,95p'Repository: ambient-code/workflows
Length of output: 1157
Update documentation: GitHub App installations can be org-owned; use installation account instead of repository owner.
Lines 81–83 incorrectly assume GitHub App installations are always user-owned. GitHub allows apps to be installed on both user accounts and organizations. Deriving GH_USER from /installation/repositories endpoint's first result returns the owner of that repository—which could be an organization—not necessarily the correct account for authentication or downstream fork checks.
Use the installation's account details instead: fetch /app/installations/{installation_id} and use the account.login field, which correctly indicates the owner (user or organization) of the installation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflows/bugfix/.claude/skills/pr/SKILL.md` around lines 81 - 83, Update the
documentation and any code examples that derive GH_USER from the first entry of
the /installation/repositories response (referenced in the text around lines
81–83) to instead fetch the installation details via GET
/app/installations/{installation_id} and use the account.login field as the
installation owner; change all mentions of “repository owner” or deriving
GH_USER from /installation/repositories to “installation account
(account.login)” and show using the installation endpoint for accurate user vs
organization ownership and downstream fork/auth checks.
| ### Step 7: Push to Fork | ||
|
|
||
| ```bash | ||
| git push -u fork bugfix/BRANCH_NAME | ||
| # Ensure git uses gh for authentication | ||
| gh auth setup-git | ||
|
|
||
| # Push the branch | ||
| git push -u fork BRANCH_NAME | ||
| ``` |
There was a problem hiding this comment.
Use a dynamic push remote instead of hardcoding fork.
Line 438 can fail when Step 4 kept origin as the fork remote (Line 316–318 guidance). This creates a self-contradictory path in the happy flow.
Suggested fix
### Step 4: Configure the Fork Remote
@@
-Use `fork` as the remote name. If `origin` already points to the fork, that's
-fine — just use `origin` in subsequent commands instead of `fork`.
+Use `fork` as the remote name. If `origin` already points to the fork, that's
+fine — set `PUSH_REMOTE=origin`; otherwise set `PUSH_REMOTE=fork`.
@@
### Step 7: Push to Fork
@@
-# Push the branch
-git push -u fork BRANCH_NAME
+# Push the branch
+git push -u PUSH_REMOTE BRANCH_NAME🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflows/bugfix/.claude/skills/pr/SKILL.md` around lines 431 - 439, Replace
the hardcoded push remote in the Step 7 instruction ("git push -u fork
BRANCH_NAME") with a dynamic remote that uses whatever remote was configured
earlier in Step 4 (the fork remote may be named "origin" or "fork"); update the
text to reference a variable remote (e.g., "git push -u <FORK_REMOTE>
BRANCH_NAME" or "git push -u <remote> BRANCH_NAME") and/or instruct the user to
use the remote name they set in Step 4, ensuring the push command uses the same
remote name as configured previously.
1 participant
Summary
Addresses four open issues with the bugfix workflow's PR skill:
gh auth setup-gitbeforegit pushto configure the credential helper in fresh environments where git doesn't know to useghfor auth. Also adds an Error Recovery table entry for the "could not read Username" symptom.git add/git statuswalkthrough from Step 5, "Key flags explained" section and 20-line inline PR body template from Step 7.gh pr createfails (the common case for bot environments) via two complementary approaches: (1) include PR description in the commit message body so GitHub auto-fills from single commits, (2) encodetitleandbodyas URL query parameters in the compare URL.maininstead of the fix branch.Net result: 54 insertions, 77 deletions — the skill is shorter despite the new functionality.
Note: This PR builds on #78 (commands-to-skills migration). The diff against
mainincludes both sets of changes; once #78 merges, this PR's diff will show only the PR skill improvements.Fixes #47, fixes #54, fixes #72, fixes #73
Test plan
/prin an environment where git credentials aren't pre-configured — verifygh auth setup-gitprevents "could not read Username" errors/pras a GitHub App bot — verify the compare URL includestitleandbodyquery params and opens with the PR form pre-populatedMade with Cursor