Skip to content

fix(bugfix): comprehensive workflow improvements addressing 9 open issues#90

Open
jwm4 wants to merge 9 commits intoambient-code:mainfrom
jwm4:bugfix/comprehensive-workflow-improvements
Open

fix(bugfix): comprehensive workflow improvements addressing 9 open issues#90
jwm4 wants to merge 9 commits intoambient-code:mainfrom
jwm4:bugfix/comprehensive-workflow-improvements

Conversation

@jwm4
Copy link
Copy Markdown
Contributor

@jwm4 jwm4 commented Mar 28, 2026

Summary

Comprehensive update to the bugfix workflow addressing 9 open issues across 6 skill files.

Changes by file

controller/SKILL.md (#81)

  • Require AskUserQuestion tool for phase gates instead of prose "stop and wait" — creates a hard structural gate with platform notifications
  • Add rule: always read skill files before executing (prevents ad-hoc execution that skips documented instructions)
  • Add rule: urgency does not bypass process (security advisories shouldn't cause the agent to skip phase gates)

assess/SKILL.md (#82)

  • New "Check for Existing Work" step — scans open PRs and related issues before deep analysis
  • "Not actually a bug" assessment path — agent presents options instead of proceeding to fix something it concluded isn't broken

reproduce/SKILL.md (#57)

  • Environment setup recipes: detect project type from config files, provide setup commands per package manager (uv, poetry, pip, npm, pnpm, yarn, Go)
  • Version manager detection (uv, pyenv, nvm, fnm)
  • Error recovery patterns for common setup failures

fix/SKILL.md (#58)

  • New "Review Error Handling UX" step between implementing and addressing related code
  • Guidance on matching error types to context (CLI vs config file vs runtime vs API)
  • Require manual testing of error paths

test/SKILL.md (#55, #56, #59)

pr/SKILL.md (#80, #85)

Test plan

  • Run the bugfix workflow end-to-end against a repo with main as default branch — verify phase gates work with AskUserQuestion
  • Run the bugfix workflow against a repo with dev as default branch — verify PR targets the correct branch
  • Verify the assess phase checks for existing PRs before deep analysis
  • Verify the test phase runs the full suite (not just new tests) and runs formatters afterward
  • Verify the controller stops and waits after each phase (does not auto-advance)

Custom Workflow Testing

Field Value
URL https://github.com/jwm4/workflows.git
Branch bugfix/comprehensive-workflow-improvements
Path workflows/bugfix

Addresses #55, #56, #57, #58, #59, #80, #81, #82, #85

Made with Cursor

…sues

Controller (ambient-code#81): Require AskUserQuestion tool for phase gates instead of
prose "stop and wait" instructions. Add rules enforcing skill file reads and
preventing urgency from bypassing process.

Assess (ambient-code#82): Add "Check for Existing Work" step that scans open PRs and
related issues before deep analysis. Add "not actually a bug" assessment
path so the agent doesn't proceed to fix something it concluded isn't broken.

Reproduce (ambient-code#57): Add environment setup recipes — detect project type from
config files, provide setup commands per package manager, check for version
managers, and include error recovery patterns for common failures.

Fix (ambient-code#58): Add error handling UX review step. Guide agent to match error
types to their context (CLI vs config vs runtime), test error paths manually,
and ensure messages are clear and actionable.

Test (ambient-code#55, ambient-code#56, ambient-code#59): Add "Survey Existing Test Patterns" step requiring
the agent to examine existing tests before writing new ones. Make full test
suite run mandatory with explicit "do not skip" language. Add dedicated
linting/formatting step after writing test code.

PR (ambient-code#80, ambient-code#85): Replace hardcoded "main" with DEFAULT_BRANCH placeholder
detected from the upstream repo. Replace hardcoded "fork" remote name with
FORK_REMOTE placeholder set during remote configuration. Update all
downstream references (push, PR create, compare URLs, sync commands).

Addresses ambient-code#55, ambient-code#56, ambient-code#57, ambient-code#58, ambient-code#59, ambient-code#80, ambient-code#81, ambient-code#82, ambient-code#85

Made-with: Cursor
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Added an assess gate that queries GitHub for existing PRs and pauses via AskUserQuestion on likely duplicates; mandated reading phase SKILL.md before executing phases and replaced ad-hoc waits with AskUserQuestion gates; added a dispatchable final summary phase that synthesizes artifacts; updated PR, reproduce, test, and fix skill docs and step numbering.

Changes

Cohort / File(s) Summary
Controller & Speedrun Orchestration
workflows/bugfix/.claude/skills/controller/SKILL.md, workflows/bugfix/.claude/skills/speedrun/SKILL.md, workflows/bugfix/CLAUDE.md
Added summary phase as required final step for speedrun; require reading each phase's SKILL.md before execution; replaced informal "stop and wait" with hard-gated AskUserQuestion flows; disallowed auto-advancement.
Assess: Existing Work Detection
workflows/bugfix/.claude/skills/assess/SKILL.md
Inserted Step 3 "Check for Existing Work": query GitHub for issue-linked/open PRs, detect overlaps/duplicates, emit "Existing Work" section, and pause via AskUserQuestion when a directly relevant open PR is found; renumbered subsequent steps and added explicit non-applicability handling.
New Summary Skill
workflows/bugfix/.claude/skills/summary/SKILL.md
New public summary skill: discover artifacts/bugfix/*.md (exclude summary.md), read and categorize artifact contents, synthesize a "Bugfix Workflow Summary", write artifacts/bugfix/summary.md, and return it when dispatched.
PR Workflow: Dynamic Branch & Remotes
workflows/bugfix/.claude/skills/pr/SKILL.md
Replaced hardcoded main/fork assumptions with DEFAULT_BRANCH, UPSTREAM_REMOTE, FORK_REMOTE; improved auth recovery for expired tokens; detect upstream default branch when possible; update all git/gh operations and compare-URL fallbacks to use detected values.
Fix Skill — Error Handling UX
workflows/bugfix/.claude/skills/fix/SKILL.md
Added "Review Error Handling UX" step to check contextual error messages, exercise error paths, and avoid leaking internals; renumbered subsequent checklist and moved implementation-notes to final artifact step.
Reproduce: Robust Environment Setup
workflows/bugfix/.claude/skills/reproduce/SKILL.md
Added pre-install inspection for lockfiles/manifests across Python/Node/Go, mapping to package managers and setup commands; check version managers (pyenv/nvm/fnm, etc.); added failure-recovery flows and stop/retry guidance.
Test Skill: Conventions & Mandatory Suite
workflows/bugfix/.claude/skills/test/SKILL.md
Replaced "Create Regression Test" with "Survey Existing Test Patterns"; added mandatory "Run the Full Test Suite" and "Lint and Format All Modified Files"; renumbered test and verification steps.
Docs & Renumbering Across Skills
workflows/bugfix/.claude/skills/*
Multiple SKILL.md documentation updates to reflect the new summary phase, step renumbering, gating changes, and updated guidance across assess, reproduce, test, fix, and pr skills.

Sequence Diagram(s)

sequenceDiagram
    participant Controller
    participant Assess as AssessSkill
    participant GitHub
    participant User
    Note over Controller,Assess: Assess phase runs (must Read SKILL.md first)
    Controller->>Assess: Invoke Assess SKILL
    Assess->>GitHub: Query issue-linked PRs & list open PRs
    GitHub-->>Assess: Return PRs & titles
    alt Directly addressing open PR found
        Assess->>Controller: Pause & AskUserQuestion (present duplicates/related PRs)
        Controller->>User: Notify & present choices (stop / continue / close / mark duplicate)
        User-->>Controller: Selection
        Controller->>Assess: Resume or stop based on response
    else No related work found
        Assess->>Controller: Continue to next step
    end
Loading
sequenceDiagram
    participant Controller
    participant Summary as SummarySkill
    participant Artifacts
    participant User
    Note over Controller,Summary: /summary can be invoked at any point
    Controller->>Summary: Invoke /summary phase
    Summary->>Artifacts: List artifacts/bugfix/*.md (exclude summary.md)
    alt No artifacts
        Summary->>Controller: Report "no artifacts" and stop
    else Artifacts found
        Summary->>Artifacts: Read each artifact
        Summary->>Summary: Extract & categorize findings (existing work, understanding, root cause, tests, PR status, outstanding items)
        Summary->>Artifacts: Write artifacts/bugfix/summary.md
        Summary->>Controller: Return summary file path
        Controller->>User: Present consolidated Bugfix Workflow Summary
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically summarizes the main change: comprehensive workflow improvements across multiple skill files addressing nine open issues.
Description check ✅ Passed The description is directly related to the changeset, providing clear organization of changes by file, addressing multiple workflow improvements with detailed rationale and testing guidance.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

jwm4 added 2 commits March 28, 2026 09:50
When the assess skill finds an open PR that appears to address the bug,
stop and use AskUserQuestion before continuing — even in speedrun mode.
This prevents duplicate work when a fix is already in flight.

The gate is in the skill rather than the controller/speedrun because
"should we even continue this workflow?" is a discovery-time decision,
and a tool call is structurally reliable in both interactive and
speedrun modes (unlike prose escalation framing).

Follow-up to the comprehensive update in the previous commit.
Addresses ambient-code#82

Made-with: Cursor
New skill that scans artifacts/bugfix/ dynamically and presents a
synthesized summary of findings, decisions, and status. Designed to
surface information that gets buried in earlier artifacts — related PRs,
review concerns, unconfirmed assumptions, outstanding items.

Key design choices:
- Discovers artifacts by scanning the directory tree rather than
  maintaining a hardcoded list (won't break when other skills change)
- Invocable at any point mid-workflow, not just at the end
- Added as phase 9 in both controller and speedrun flows
- In speedrun, replaces the static completion report with a
  content-aware summary — the last thing the user sees

Updated controller, speedrun, and CLAUDE.md to include the new phase.

Made-with: Cursor
@jwm4 jwm4 marked this pull request as ready for review March 28, 2026 14:31
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
workflows/bugfix/.claude/skills/assess/SKILL.md (1)

197-203: ⚠️ Potential issue | 🟡 Minor

Step reference is stale after renumbering.

Line 202 says the artifact should mirror content from Step 6, but presentation is now in Step 7. This can cause process drift when operators copy the wrong section.

Proposed fix
-in Step 6.
+in Step 7.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflows/bugfix/.claude/skills/assess/SKILL.md` around lines 197 - 203,
Update the Step 8 description so it references the correct preceding step (Step
7) instead of the stale "Step 6"; specifically, edit the text that instructs
saving the assessment to artifacts/bugfix/reports/assessment.md to state the
file should contain the same content presented in Step 7 (the "presentation"
step) and verify the artifact path and filename remain unchanged. Ensure the
sentence mentioning "Step 6" is replaced with "Step 7" and that the wording
clearly ties the artifact content to the presentation in Step 7 to avoid
operator confusion.
workflows/bugfix/.claude/skills/speedrun/SKILL.md (1)

26-27: ⚠️ Potential issue | 🟠 Major

Conflicting end-of-flow behavior after adding /summary.

Line 26 still mandates printing the Completion Report when all phases are done, but Line 134 says /summary “replaces” that report. This creates contradictory terminal behavior and can produce duplicate end outputs.

Proposed doc fix
-2. If all phases are done, print the completion report and stop
+2. If all phases are done, stop (the `/summary` phase is the terminal user-facing report)
-## Completion Report
-
-When all phases are done (or if you stop early due to escalation), present:
+## Completion Report (Escalation/Early Stop Only)
+
+If you stop early due to escalation (before `/summary`), present:

Also applies to: 132-157

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflows/bugfix/.claude/skills/speedrun/SKILL.md` around lines 26 - 27, The
document has conflicting terminal behavior: line referencing "print the
completion report and stop" conflicts with the note that the `/summary` command
"replaces" that report; update SKILL.md so the end-of-flow behavior is
consistent by specifying that when the `/summary` flag is present the normal
"Completion Report" is suppressed and replaced by the `/summary` output, and
when `/summary` is absent the Completion Report is printed as before; apply this
clarification to the earlier flow description (the "If all phases are done"
step) and the later "/summary" section (around lines 132-157) so both places
state the single consistent rule.
🤖 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 362-365: The git diff invocation used to set WORKFLOW_DIFF places
the --name-only option after the -- pathspec separator, so Git treats it as a
path instead of an option; update the WORKFLOW_DIFF assignment so the
--name-only flag appears before the -- delimiter in the git diff command (i.e.,
call git diff FORK_REMOTE/DEFAULT_BRANCH..DEFAULT_BRANCH --name-only --
.github/workflows/), ensuring WORKFLOW_DIFF contains only filenames; locate the
assignment to WORKFLOW_DIFF in the file and reorder the arguments accordingly.

In `@workflows/bugfix/.claude/skills/summary/SKILL.md`:
- Around line 35-36: The discovery command currently lists all .md files ("find
artifacts/bugfix/ -type f -name '*.md' ... | sort") which will re-include the
generated summary (summary.md); update that command to explicitly exclude
summary.md (e.g., add -not -name 'summary.md' or an equivalent --exclude filter)
so the generated file is never read back in, and ensure any code that writes the
summary (the block that outputs summary.md) remains unchanged but is referenced
as the produced artifact to avoid self-ingestion.

In `@workflows/bugfix/.claude/skills/test/SKILL.md`:
- Around line 129-137: Replace the placeholder "FILE1 FILE2 ..." comments after
"git diff --name-only HEAD" with concrete examples showing how to pipe or
substitute that command into each formatter; update the Python, Node.js and Go
comment lines (the comments mentioning black/isort/ruff, npx prettier/eslint,
and gofmt) to show either command substitution using "$(git diff --name-only
HEAD | grep '\.py$')" / similar for js/ts/go or an xargs form "git diff
--name-only HEAD | grep '\.py$' | xargs black ..." so readers can see exactly
how to feed the git diff output into black/isort/ruff, prettier/eslint and
gofmt.
- Around line 36-41: The shell glob "cat *_test.go 2>/dev/null | head -5" can
fail when the glob doesn't expand; replace that pipeline with a find-based
search that explicitly looks for files matching "*_test.go" (limit search
depth), runs head on each matched file, redirects find/head stderr to /dev/null,
and then limits the overall output (e.g., to 20 lines); update the line
referencing "cat *_test.go" in SKILL.md to use this find+head approach so
missing matches are handled robustly.
- Around line 75-78: The guidance currently includes a Python-specific example
using call_args.args[0] vs call_args[0][0] without qualification; update
SKILL.md to either (a) prepend a clear language qualifier (e.g., "Python
(unittest.mock):") when mentioning call_args.args[0 and call_args[0][0]) or (b)
move that example into the "Python projects" section so cross-language guidance
remains generic; edit the paragraph containing call_args.args[0 and
call_args[0][0] to add the qualifier or relocate the sentence to the Python
projects section and ensure the surrounding generic recommendations stay
language-agnostic.

---

Outside diff comments:
In `@workflows/bugfix/.claude/skills/assess/SKILL.md`:
- Around line 197-203: Update the Step 8 description so it references the
correct preceding step (Step 7) instead of the stale "Step 6"; specifically,
edit the text that instructs saving the assessment to
artifacts/bugfix/reports/assessment.md to state the file should contain the same
content presented in Step 7 (the "presentation" step) and verify the artifact
path and filename remain unchanged. Ensure the sentence mentioning "Step 6" is
replaced with "Step 7" and that the wording clearly ties the artifact content to
the presentation in Step 7 to avoid operator confusion.

In `@workflows/bugfix/.claude/skills/speedrun/SKILL.md`:
- Around line 26-27: The document has conflicting terminal behavior: line
referencing "print the completion report and stop" conflicts with the note that
the `/summary` command "replaces" that report; update SKILL.md so the
end-of-flow behavior is consistent by specifying that when the `/summary` flag
is present the normal "Completion Report" is suppressed and replaced by the
`/summary` output, and when `/summary` is absent the Completion Report is
printed as before; apply this clarification to the earlier flow description (the
"If all phases are done" step) and the later "/summary" section (around lines
132-157) so both places state the single consistent rule.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6ed28089-79fe-42b9-8e8c-d04aaff674ad

📥 Commits

Reviewing files that changed from the base of the PR and between 1bd9d6e and d4e3e21.

📒 Files selected for processing (9)
  • workflows/bugfix/.claude/skills/assess/SKILL.md
  • workflows/bugfix/.claude/skills/controller/SKILL.md
  • workflows/bugfix/.claude/skills/fix/SKILL.md
  • workflows/bugfix/.claude/skills/pr/SKILL.md
  • workflows/bugfix/.claude/skills/reproduce/SKILL.md
  • workflows/bugfix/.claude/skills/speedrun/SKILL.md
  • workflows/bugfix/.claude/skills/summary/SKILL.md
  • workflows/bugfix/.claude/skills/test/SKILL.md
  • workflows/bugfix/CLAUDE.md

Comment on lines +36 to +41
```bash
# Check for test configuration
cat pytest.ini 2>/dev/null || cat setup.cfg 2>/dev/null | head -20
cat jest.config.* 2>/dev/null || cat vitest.config.* 2>/dev/null
cat *_test.go 2>/dev/null | head -5
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Improve shell glob robustness for Go test file detection.

The command cat *_test.go 2>/dev/null | head -5 may behave unexpectedly if no matching files exist. In shells without nullglob enabled, the glob pattern *_test.go won't expand and cat will attempt to read a file literally named *_test.go, which will fail (silently due to 2>/dev/null, but still incorrect).

🔧 Proposed fix using a more robust file detection pattern
 ```bash
 # Check for test configuration
 cat pytest.ini 2>/dev/null || cat setup.cfg 2>/dev/null | head -20
 cat jest.config.* 2>/dev/null || cat vitest.config.* 2>/dev/null
-cat *_test.go 2>/dev/null | head -5
+find . -maxdepth 2 -name '*_test.go' -exec head -5 {} \; 2>/dev/null | head -20

This uses `find` to explicitly search for matching files, avoiding glob expansion issues.
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @workflows/bugfix/.claude/skills/test/SKILL.md around lines 36 - 41, The
shell glob "cat _test.go 2>/dev/null | head -5" can fail when the glob doesn't
expand; replace that pipeline with a find-based search that explicitly looks for
files matching "
_test.go" (limit search depth), runs head on each matched file,
redirects find/head stderr to /dev/null, and then limits the overall output
(e.g., to 20 lines); update the line referencing "cat *_test.go" in SKILL.md to
use this find+head approach so missing matches are handled robustly.


</details>

<!-- fingerprinting:phantom:triton:puma -->

<!-- This is an auto-generated comment by CodeRabbit -->

Comment on lines +129 to +137
```bash
# Identify all modified files
git diff --name-only HEAD

# Then run the appropriate formatters on ALL of them:
# Python: black FILE1 FILE2 ... && isort FILE1 FILE2 ... && ruff check --fix FILE1 FILE2 ...
# Node.js: npx prettier --write FILE1 FILE2 ... && npx eslint --fix FILE1 FILE2 ...
# Go: gofmt -w FILE1 FILE2 ...
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding a concrete example for applying formatters to modified files.

The placeholder syntax FILE1 FILE2 ... in lines 134-136 may not be immediately clear on how to integrate with the git diff --name-only HEAD output. Consider adding a concrete example using command substitution or xargs to show the connection between the two steps.

📋 Example using command substitution
 ```bash
 # Identify all modified files
 git diff --name-only HEAD
 
-# Then run the appropriate formatters on ALL of them:
-# Python:  black FILE1 FILE2 ... && isort FILE1 FILE2 ... && ruff check --fix FILE1 FILE2 ...
-# Node.js: npx prettier --write FILE1 FILE2 ... && npx eslint --fix FILE1 FILE2 ...
-# Go:      gofmt -w FILE1 FILE2 ...
+# Then run the appropriate formatters on ALL of them (example with command substitution):
+# Python:  black $(git diff --name-only HEAD | grep '\.py$') && isort $(git diff --name-only HEAD | grep '\.py$')
+# Node.js: npx prettier --write $(git diff --name-only HEAD | grep '\.(js|ts|tsx)$')
+# Go:      gofmt -w $(git diff --name-only HEAD | grep '\.go$')

Alternatively, mention `xargs` as another approach for handling file lists.
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @workflows/bugfix/.claude/skills/test/SKILL.md around lines 129 - 137,
Replace the placeholder "FILE1 FILE2 ..." comments after "git diff --name-only
HEAD" with concrete examples showing how to pipe or substitute that command into
each formatter; update the Python, Node.js and Go comment lines (the comments
mentioning black/isort/ruff, npx prettier/eslint, and gofmt) to show either
command substitution using "$(git diff --name-only HEAD | grep '.py$')" /
similar for js/ts/go or an xargs form "git diff --name-only HEAD | grep '.py$'
| xargs black ..." so readers can see exactly how to feed the git diff output
into black/isort/ruff, prettier/eslint and gofmt.


</details>

<!-- fingerprinting:phantom:triton:puma -->

<!-- This is an auto-generated comment by CodeRabbit -->

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
workflows/bugfix/.claude/skills/assess/SKILL.md (1)

202-202: ⚠️ Potential issue | 🟡 Minor

Fix the step reference after renumbering.

After adding the new Step 3, "Present to the User" is now Step 7, not Step 6. The assessment artifact should contain the content from Step 7.

📝 Proposed fix
-in Step 6.
+in Step 7.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflows/bugfix/.claude/skills/assess/SKILL.md` at line 202, The document
still references "Step 6" for the assessment artifact but after adding a new
Step 3 the "Present to the User" step is now Step 7; update the SKILL.md content
that mentions "Step 6" (the assessment artifact reference) to point to Step 7
and ensure the assessment artifact includes the content from "Present to the
User" (the updated step title) so the artifact pulls the correct step content.
🤖 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/assess/SKILL.md`:
- Line 84: The hardcoded "--limit 30" in the gh pr list and corresponding issue
listing command should be justified or made configurable; update the SKILL.md
around the gh pr list --limit 30 invocation (and the duplicate at the other
occurrence) to either add a short comment explaining the trade-off (fast listing
vs missing PRs in high-activity repos) or document how to change the limit
(e.g., run the command with a different --limit or remove it), and ensure both
occurrences mention the rationale and how to adjust the limit so readers know
why 30 was chosen and how to increase it when needed.
- Line 78: The existing grep pattern "pull\|PR\|#" is too broad; update the grep
invocation in the pipeline command to use a stricter regex (and -E for extended
regex) that only matches explicit PR/issue references, e.g. replace the pattern
with something like
'(^|[^A-Za-z0-9])(#\d+|PR[[:space:]]*#\d+|pull[[:space:]]+request[[:space:]]*#\d+)([^A-Za-z0-9]|$)'
so the command that contains the current "pull\|PR\|#" pattern only returns real
references like "#123", "PR `#123`" or "pull request `#123`".
- Around line 96-106: The pr phase uses plain text "Stop again and wait for the
user to confirm" instead of invoking the AskUserQuestion tool; update the pr
phase SKILL.md to call AskUserQuestion with the same three options used in the
assess skill ("PR `#N` appears to address this bug — review it instead of starting
fresh", "PR `#N` is related but doesn't fully cover it — continue with
assessment", "Not sure if PR `#N` is relevant — continue with assessment") and
enforce the hard gate so the flow does not proceed until AskUserQuestion returns
the user's choice.

In `@workflows/bugfix/.claude/skills/pr/SKILL.md`:
- Around line 362-365: The git diff invocation assigned to WORKFLOW_DIFF places
the --name-only option after the pathspec which violates git syntax; update the
git diff call inside the WORKFLOW_DIFF assignment to pass --name-only as an
option before the revision range (FORK_REMOTE/DEFAULT_BRANCH..DEFAULT_BRANCH)
and keep the pathspec separator (--) before .github/workflows/ so the command
returns only file names reliably.

In `@workflows/bugfix/.claude/skills/speedrun/SKILL.md`:
- Around line 132-137: Update the "summary" skill description to explicitly
preserve mandatory interactive gates by name: state that the summary phase (the
"summary" section and the Speedrun behavior) must NOT bypass any hard
AskUserQuestion gates (e.g., "assess existing-PR decision") and must pause and
surface those gates for user input before continuing; reference the "summary"
skill and the AskUserQuestion gate type so readers know this final-phase
behavior still enforces earlier interactive checkpoints.

---

Outside diff comments:
In `@workflows/bugfix/.claude/skills/assess/SKILL.md`:
- Line 202: The document still references "Step 6" for the assessment artifact
but after adding a new Step 3 the "Present to the User" step is now Step 7;
update the SKILL.md content that mentions "Step 6" (the assessment artifact
reference) to point to Step 7 and ensure the assessment artifact includes the
content from "Present to the User" (the updated step title) so the artifact
pulls the correct step content.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 17de1b89-1db5-40c1-91d4-31abd08abc3a

📥 Commits

Reviewing files that changed from the base of the PR and between 1bd9d6e and d4e3e21.

📒 Files selected for processing (9)
  • workflows/bugfix/.claude/skills/assess/SKILL.md
  • workflows/bugfix/.claude/skills/controller/SKILL.md
  • workflows/bugfix/.claude/skills/fix/SKILL.md
  • workflows/bugfix/.claude/skills/pr/SKILL.md
  • workflows/bugfix/.claude/skills/reproduce/SKILL.md
  • workflows/bugfix/.claude/skills/speedrun/SKILL.md
  • workflows/bugfix/.claude/skills/summary/SKILL.md
  • workflows/bugfix/.claude/skills/test/SKILL.md
  • workflows/bugfix/CLAUDE.md

- **Check for linked PRs on the issue:**

```bash
gh issue view NUMBER --repo OWNER/REPO --json body,comments --jq '[.body, .comments[].body] | join("\n")' | grep -i "pull\|PR\|#"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Refine the grep pattern to reduce false positives.

The pattern "pull\|PR\|#" is very broad and will match:

  • Any markdown heading (#)
  • Partial word matches ("PR" in "PRIORITIZE")
  • Generic uses of "pull" unrelated to PRs
🔍 Suggested pattern refinement
-gh issue view NUMBER --repo OWNER/REPO --json body,comments --jq '[.body, .comments[].body] | join("\n")' | grep -i "pull\|PR\|#"
+gh issue view NUMBER --repo OWNER/REPO --json body,comments --jq '[.body, .comments[].body] | join("\n")' | grep -iE '#[0-9]+|PR #[0-9]+|pull request #[0-9]+'

This matches actual PR/issue references like #123, PR #123, or `pull request `#123 rather than arbitrary occurrences.

📝 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.

Suggested change
gh issue view NUMBER --repo OWNER/REPO --json body,comments --jq '[.body, .comments[].body] | join("\n")' | grep -i "pull\|PR\|#"
gh issue view NUMBER --repo OWNER/REPO --json body,comments --jq '[.body, .comments[].body] | join("\n")' | grep -iE '#[0-9]+|PR #[0-9]+|pull request #[0-9]+'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflows/bugfix/.claude/skills/assess/SKILL.md` at line 78, The existing
grep pattern "pull\|PR\|#" is too broad; update the grep invocation in the
pipeline command to use a stricter regex (and -E for extended regex) that only
matches explicit PR/issue references, e.g. replace the pattern with something
like
'(^|[^A-Za-z0-9])(#\d+|PR[[:space:]]*#\d+|pull[[:space:]]+request[[:space:]]*#\d+)([^A-Za-z0-9]|$)'
so the command that contains the current "pull\|PR\|#" pattern only returns real
references like "#123", "PR `#123`" or "pull request `#123`".

- **Scan recent PR titles for overlap:**

```bash
gh pr list --repo OWNER/REPO --state open --limit 30 --json number,title,headRefName --jq '.[] | "\(.number)\t\(.title)"'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider documenting the rationale for --limit 30.

The hardcoded limit of 30 for both PR and issue listings is reasonable for most repositories, but relevant work could be missed in high-activity projects. Consider adding a comment explaining this trade-off, or note that users can manually check if they suspect additional relevant work exists.

Also applies to: 93-93

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflows/bugfix/.claude/skills/assess/SKILL.md` at line 84, The hardcoded
"--limit 30" in the gh pr list and corresponding issue listing command should be
justified or made configurable; update the SKILL.md around the gh pr list
--limit 30 invocation (and the duplicate at the other occurrence) to either add
a short comment explaining the trade-off (fast listing vs missing PRs in
high-activity repos) or document how to change the limit (e.g., run the command
with a different --limit or remove it), and ensure both occurrences mention the
rationale and how to adjust the limit so readers know why 30 was chosen and how
to increase it when needed.

Comment on lines +96 to +106
If you find an open PR that appears to directly address this bug, **stop here
and use `AskUserQuestion`** before continuing the assessment. Present the
options:

- "PR #N appears to address this bug — review it instead of starting fresh"
- "PR #N is related but doesn't fully cover it — continue with assessment"
- "Not sure if PR #N is relevant — continue with assessment"

This gate applies in both normal and speedrun mode. Do not continue to Step 4
until the user responds. The `AskUserQuestion` tool triggers platform
notifications so the user knows you need their input.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check for plain-text wait instructions instead of AskUserQuestion in skill files.

# Search for wait/stop patterns that should be replaced with AskUserQuestion
rg -n -C3 --type=md 'stop.*wait|wait.*confirm|wait.*user.*respond|pause.*user' workflows/bugfix/.claude/skills/ | grep -v AskUserQuestion

Repository: ambient-code/workflows

Length of output: 1932


The assess skill correctly uses AskUserQuestion; update pr/SKILL.md to align with the same gate requirement.

The assess phase at lines 96-106 properly uses the AskUserQuestion tool for its hard gate, meeting the controller requirement. However, pr/SKILL.md lines 305-306 still use plain text "Stop again and wait for the user to confirm" instead of the AskUserQuestion tool. Both phase gates must use AskUserQuestion to trigger platform notifications consistently.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflows/bugfix/.claude/skills/assess/SKILL.md` around lines 96 - 106, The
pr phase uses plain text "Stop again and wait for the user to confirm" instead
of invoking the AskUserQuestion tool; update the pr phase SKILL.md to call
AskUserQuestion with the same three options used in the assess skill ("PR `#N`
appears to address this bug — review it instead of starting fresh", "PR `#N` is
related but doesn't fully cover it — continue with assessment", "Not sure if PR
`#N` is relevant — continue with assessment") and enforce the hard gate so the
flow does not proceed until AskUserQuestion returns the user's choice.

jwm4 added 2 commits March 28, 2026 10:46
Remove "bug fix" framing from the description — the skill is general-
purpose PR creation machinery. Rewrite to emphasize what it handles
(auth detection, fork workflows, default branch, fallbacks) and why
you'd use it over ad-hoc gh pr create, so the runner matches it more
effectively when the agent needs to create a PR outside the normal
workflow flow.

Made-with: Cursor
- pr/SKILL.md: move --name-only before -- separator in git diff command
  (was being parsed as a pathspec, silently returning full diff output)
- summary/SKILL.md: exclude summary.md from artifact discovery to prevent
  self-ingestion on reruns
- assess/SKILL.md: fix stale "Step 6" reference → "Step 7" after renumbering
- speedrun/SKILL.md: clarify completion report is only for early stops;
  /summary is the terminal output for normal runs
- test/SKILL.md: qualify call_args example as Python-specific

Made-with: Cursor
@jwm4
Copy link
Copy Markdown
Contributor Author

jwm4 commented Mar 28, 2026

CodeRabbit review response

Addressed 5 findings in commit 9d9a0fc. Here's why the remaining comments were not acted on:

--limit 30 rationale (assess/SKILL.md) — This is a guidance document for an AI agent, not a production script. The agent can adjust the limit based on context. Adding a comment explaining the trade-off would be over-documentation for a skill file.

Grep pattern too broad (assess/SKILL.md) — The grep -i "pull\|PR\|#" is intentionally broad as a quick scan to surface potentially related references. A stricter regex would be harder to read and maintain in markdown, and false positives here are low-cost — the agent reads the matches and decides what's relevant.

PR skill should use AskUserQuestion for fork/auth pauses (assess comment → pr/SKILL.md) — The "stop and wait" instructions in the PR skill are operational pauses during a single phase (e.g., "wait for user to create a fork"), not phase transitions. AskUserQuestion gates are for phase-level decisions per the controller. Using them for every operational pause would be excessive.

Go test file glob robustness (test/SKILL.md) — The cat *_test.go 2>/dev/null | head -5 is a detection heuristic, not a critical operation. If it fails silently (no Go test files), the agent detects the test framework through other signals. The find-based alternative adds complexity without meaningful benefit.

Speedrun summary note about AskUserQuestion gates — The AskUserQuestion gate in the assess skill fires during the assess phase itself, before speedrun advances. The summary phase doesn't need to re-enforce gates from earlier phases — they've already fired by the time summary runs. Speedrun's existing escalation rules (line 75-79) already cover the general principle.

Formatter command examples (test/SKILL.md) — The FILE1 FILE2 ... placeholder is intentional guidance showing the pattern, not a runnable script. Making it more concrete (e.g., with command substitution) would make it harder to adapt to different project setups and would be wrong for projects that use make lint or pre-commit hooks.

When GITHUB_TOKEN expires mid-session, gh auth status fails but the
platform's git credential helper can still provide fresh tokens. The
refresh_credentials MCP tool refreshes the backend but doesn't update
the shell env var, so the skill needs to bridge that gap.

Added recovery path to Step 0: check for a credential helper, query it
for a fresh token, and persist via gh auth login --with-token. Also
added the 401 expired token symptom to the error recovery table.

Found during ACP Custom Workflow testing.

Made-with: Cursor
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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 443-446: The current step runs "git rebase
FORK_REMOTE/DEFAULT_BRANCH" as if rebasing the feature branch but Step 5 creates
that feature branch afterwards; change the flow so you first check out the base
branch (DEFAULT_BRANCH), fetch/update FORK_REMOTE, and run "git rebase
FORK_REMOTE/DEFAULT_BRANCH" on DEFAULT_BRANCH before creating the feature branch
in Step 5, ensuring the rebase operates on DEFAULT_BRANCH (not the
yet-to-be-created feature branch).
- Around line 398-401: The workflow diff currently compares
FORK_REMOTE/DEFAULT_BRANCH to the local DEFAULT_BRANCH which may be stale;
update the logic that sets WORKFLOW_DIFF to first fetch the remotes and then
diff against the upstream-tracking ref (use UPSTREAM_REMOTE/DEFAULT_BRANCH)
instead of the local branch, e.g. run git fetch for the relevant remotes and
replace the diff target DEFAULT_BRANCH with UPSTREAM_REMOTE/DEFAULT_BRANCH so
WORKFLOW_DIFF uses FORK_REMOTE/DEFAULT_BRANCH..UPSTREAM_REMOTE/DEFAULT_BRANCH
against .github/workflows/.

In `@workflows/bugfix/.claude/skills/summary/SKILL.md`:
- Line 128: Update the user-facing text in SKILL.md where the sentence reads "If
your summary is more than ~40 lines of markdown, cut it down." to use proper
noun capitalization ("Markdown"); search for that exact phrase and replace
"markdown" with "Markdown" (and fix any other occurrences of the lowercase
"markdown" in the same file to maintain consistent, user-facing capitalization).

In `@workflows/bugfix/.claude/skills/test/SKILL.md`:
- Around line 101-108: Replace the blanket instruction "Run the entire test
suite — unit tests, integration tests, and E2E tests" with a clarifying sentence
that only the commands relevant to this repository's language/tooling should be
run, and update the three example lines (pytest tests/, npm test, go test ./...)
to be labeled as examples for "Python projects", "Node.js projects", and "Go
projects" respectively so CI reviewers don't try to run all toolchains when they
aren't present.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5a92a2bd-4342-49cd-bfd2-951400d536a8

📥 Commits

Reviewing files that changed from the base of the PR and between d4e3e21 and 99db059.

📒 Files selected for processing (5)
  • workflows/bugfix/.claude/skills/assess/SKILL.md
  • workflows/bugfix/.claude/skills/pr/SKILL.md
  • workflows/bugfix/.claude/skills/speedrun/SKILL.md
  • workflows/bugfix/.claude/skills/summary/SKILL.md
  • workflows/bugfix/.claude/skills/test/SKILL.md

jwm4 added 2 commits March 28, 2026 17:32
The full suite step listed pytest, npm test, and go test without
qualification, which could be read as "run all three." Clarify that
only the command matching the project's stack should run.

Made-with: Cursor
…te clarity

- pr/SKILL.md: diff fork against upstream remote tracking ref instead
  of potentially stale local branch; add UPSTREAM_REMOTE placeholder
- pr/SKILL.md: fix rebase ordering — checkout DEFAULT_BRANCH before
  rebase so the update happens before the feature branch is created
- speedrun/SKILL.md: explicitly note AskUserQuestion gates still apply
  in speedrun mode
- summary/SKILL.md: capitalize "Markdown" as proper noun

Made-with: Cursor
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 195-203: The unauthenticated git remote detection currently
hardcodes "origin" in the commands extracting repo name and default branch;
change those commands to use the UPSTREAM_REMOTE variable determined in Step 2b
(e.g., replace occurrences of "origin" with "$UPSTREAM_REMOTE" or
"${UPSTREAM_REMOTE}") so the repo and HEAD branch are read from the true
upstream remote instead of the fork; ensure the script references the same
UPSTREAM_REMOTE variable name used earlier and quotes it appropriately in the
git remote get-url and git remote show calls.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 141387d9-f018-44ae-b80a-70afd4dafe42

📥 Commits

Reviewing files that changed from the base of the PR and between 2e24cd5 and 51342d4.

📒 Files selected for processing (3)
  • workflows/bugfix/.claude/skills/pr/SKILL.md
  • workflows/bugfix/.claude/skills/speedrun/SKILL.md
  • workflows/bugfix/.claude/skills/summary/SKILL.md

The git-based repo name and default branch detection hardcoded
"origin", which breaks when origin points to the fork. Use the
UPSTREAM_REMOTE identified in Step 2b.

Made-with: Cursor
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
workflows/bugfix/.claude/skills/pr/SKILL.md (1)

168-183: 🛠️ Refactor suggestion | 🟠 Major

Add explicit logic for determining UPSTREAM_REMOTE.

Step 2b lists common patterns but doesn't provide explicit instructions for setting the UPSTREAM_REMOTE variable. This leaves the detection logic implementation-dependent.

Consider adding explicit detection commands, for example:

# Check which remote points to the upstream org (not the fork)
# Assuming UPSTREAM_OWNER was determined in Step 2c
UPSTREAM_REMOTE=$(git remote -v | grep -E "github\.com[:/]${UPSTREAM_OWNER}/" | head -1 | awk '{print $1}')

# Common fallback: if origin points to upstream org, use it
if [ -z "$UPSTREAM_REMOTE" ]; then
  UPSTREAM_REMOTE="origin"
fi

This ensures consistent detection across different AI agent implementations.

🤖 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 168 - 183, Step 2b
describes common remote name patterns but doesn't set UPSTREAM_REMOTE; add
explicit detection logic that finds the remote whose URL contains the upstream
owner (use the already-determined UPSTREAM_OWNER) and assigns its name to
UPSTREAM_REMOTE, falling back to a sensible default such as "origin" if none
found; implement this where remotes are inventoried (referencing UPSTREAM_REMOTE
and UPSTREAM_OWNER) so downstream steps reliably use the correct upstream
remote.
🤖 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 399-404: The script mixes bare template placeholders (FORK_REMOTE,
UPSTREAM_REMOTE, DEFAULT_BRANCH) with shell variables (e.g., "$FRESH_TOKEN",
WORKFLOW_DIFF) which is inconsistent and will break execution; pick the
shell-variable approach: after detection/or assignment set shell variables
UPSTREAM_REMOTE, FORK_REMOTE and DEFAULT_BRANCH, then update all occurrences to
use the shell-variable form (e.g., git fetch "$FORK_REMOTE", git fetch
"$UPSTREAM_REMOTE" and use "$FORK_REMOTE/$DEFAULT_BRANCH" in the WORKFLOW_DIFF
assignment) so every reference (FORK_REMOTE, UPSTREAM_REMOTE, DEFAULT_BRANCH,
WORKFLOW_DIFF, FRESH_TOKEN) is consistently used as a shell variable.
- Around line 100-128: The recovery flow reads a potential token into
FRESH_TOKEN via git credential fill but then uses it unconditionally; add guards
that check FRESH_TOKEN is non-empty before exporting it, calling gh auth status,
or running gh auth login --with-token so these commands are only invoked when a
token was actually returned. Locate the token extraction (FRESH_TOKEN=$(...git
credential fill...)) and wrap the subsequent export GITHUB_TOKEN, gh auth
status, and gh auth login --with-token invocations in a conditional test that
ensures [ -n "$FRESH_TOKEN" ] (or equivalent) before performing those actions.
- Around line 195-203: The two shell commands that reference the upstream
remote—git remote get-url and git remote show—are using the literal token
UPSTREAM_REMOTE instead of the shell variable; update both invocations to use
the shell variable (e.g., wrap the name in quotes as "$UPSTREAM_REMOTE") so the
variable's value is used safely (preserve the sed/grep/awk parts unchanged),
ensuring commands target the resolved upstream remote.

---

Outside diff comments:
In `@workflows/bugfix/.claude/skills/pr/SKILL.md`:
- Around line 168-183: Step 2b describes common remote name patterns but doesn't
set UPSTREAM_REMOTE; add explicit detection logic that finds the remote whose
URL contains the upstream owner (use the already-determined UPSTREAM_OWNER) and
assigns its name to UPSTREAM_REMOTE, falling back to a sensible default such as
"origin" if none found; implement this where remotes are inventoried
(referencing UPSTREAM_REMOTE and UPSTREAM_OWNER) so downstream steps reliably
use the correct upstream remote.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b22788ca-3bc3-4bc1-9d85-a9a8c5cbcf12

📥 Commits

Reviewing files that changed from the base of the PR and between 51342d4 and 44f926e.

📒 Files selected for processing (1)
  • workflows/bugfix/.claude/skills/pr/SKILL.md

Comment on lines +100 to +128
```bash
git config --global credential.helper 2>/dev/null
```

If a credential helper exists (e.g., `/tmp/git-credential-ambient`), query it
for a fresh token:

```bash
FRESH_TOKEN=$(printf 'protocol=https\nhost=github.com\n\n' | git credential fill 2>/dev/null | grep '^password=' | cut -d= -f2)
```

If that returns a token, export it and re-check auth:

```bash
export GITHUB_TOKEN="$FRESH_TOKEN"
gh auth status
```

**Important:** Each shell invocation gets a fresh environment, so you must
prepend the export to every subsequent `gh` command, or write the token to
`~/.config/gh/hosts.yml` so `gh` picks it up natively:

```bash
gh auth login --with-token <<< "$FRESH_TOKEN"
```

The `gh auth login` approach is preferred — it persists for all subsequent
`gh` commands without per-command exports. After recovery, re-run the identity
checks above to set `AUTH_TYPE` and `GH_USER`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add conditional check before using FRESH_TOKEN.

The token recovery flow queries the credential helper but doesn't verify the token is non-empty before using it. If the credential helper returns nothing, the subsequent commands will fail with an empty token.

♻️ Proposed fix
 ```bash
 FRESH_TOKEN=$(printf 'protocol=https\nhost=github.com\n\n' | git credential fill 2>/dev/null | grep '^password=' | cut -d= -f2)

-If that returns a token, export it and re-check auth:
+If that returns a non-empty token, export it and re-check auth:

+if [ -n "$FRESH_TOKEN" ]; then
+  export GITHUB_TOKEN="$FRESH_TOKEN"
+  gh auth status
+fi
-export GITHUB_TOKEN="$FRESH_TOKEN"
-gh auth status

Important: Each shell invocation gets a fresh environment, so you must
prepend the export to every subsequent gh command, or write the token to
~/.config/gh/hosts.yml so gh picks it up natively:

+if [ -n "$FRESH_TOKEN" ]; then
+  gh auth login --with-token <<< "$FRESH_TOKEN"
+fi
-gh auth login --with-token <<< "$FRESH_TOKEN"
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @workflows/bugfix/.claude/skills/pr/SKILL.md around lines 100 - 128, The
recovery flow reads a potential token into FRESH_TOKEN via git credential fill
but then uses it unconditionally; add guards that check FRESH_TOKEN is non-empty
before exporting it, calling gh auth status, or running gh auth login
--with-token so these commands are only invoked when a token was actually
returned. Locate the token extraction (FRESH_TOKEN=$(...git credential fill...))
and wrap the subsequent export GITHUB_TOKEN, gh auth status, and gh auth login
--with-token invocations in a conditional test that ensures [ -n "$FRESH_TOKEN"
] (or equivalent) before performing those actions.


</details>

<!-- fingerprinting:phantom:triton:puma -->

<!-- This is an auto-generated comment by CodeRabbit -->

Comment on lines +195 to +203
If `gh` is NOT authenticated, extract from the upstream remote (identified in
Step 2b as `UPSTREAM_REMOTE`):

```bash
git remote get-url origin | sed -E 's#.*/([^/]+/[^/]+?)(\.git)?$#\1#'
# Repo name (use UPSTREAM_REMOTE, not necessarily origin — origin may be the fork)
git remote get-url UPSTREAM_REMOTE | sed -E 's#.*/([^/]+/[^/]+?)(\.git)?$#\1#'

# Default branch
git remote show UPSTREAM_REMOTE 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Use proper shell variable syntax for UPSTREAM_REMOTE.

The commands use bare UPSTREAM_REMOTE without shell variable syntax, which will fail when executed. Bash will look for a remote literally named "UPSTREAM_REMOTE" rather than using the variable's value.

This is inconsistent with the file's own usage of proper shell syntax elsewhere (e.g., line 108 uses "$FRESH_TOKEN", line 404 assigns to WORKFLOW_DIFF).

🐛 Proposed fix
 ```bash
-# Repo name (use UPSTREAM_REMOTE, not necessarily origin — origin may be the fork)
-git remote get-url UPSTREAM_REMOTE | sed -E 's#.*/([^/]+/[^/]+?)(\.git)?$#\1#'
+# Repo name (from upstream remote found in Step 2b)
+git remote get-url "$UPSTREAM_REMOTE" | sed -E 's#.*/([^/]+/[^/]+?)(\.git)?$#\1#'
 
 # Default branch
-git remote show UPSTREAM_REMOTE 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}'
+git remote show "$UPSTREAM_REMOTE" 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}'
</details>

**Note:** This was marked as addressed in commit 44f926e, but the shell variable syntax was not applied correctly.

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @workflows/bugfix/.claude/skills/pr/SKILL.md around lines 195 - 203, The two
shell commands that reference the upstream remote—git remote get-url and git
remote show—are using the literal token UPSTREAM_REMOTE instead of the shell
variable; update both invocations to use the shell variable (e.g., wrap the name
in quotes as "$UPSTREAM_REMOTE") so the variable's value is used safely
(preserve the sed/grep/awk parts unchanged), ensuring commands target the
resolved upstream remote.


</details>

<!-- fingerprinting:phantom:triton:puma -->

<!-- This is an auto-generated comment by CodeRabbit -->

Comment on lines +399 to +404
git fetch FORK_REMOTE
git fetch UPSTREAM_REMOTE

# Check for workflow file differences between fork/main and local main
# (local main should be synced with upstream)
WORKFLOW_DIFF=$(git diff fork/main..main -- .github/workflows/ --name-only 2>/dev/null)
# Compare fork's DEFAULT_BRANCH against upstream's DEFAULT_BRANCH
# (don't rely on the local branch — it may be stale)
WORKFLOW_DIFF=$(git diff --name-only FORK_REMOTE/DEFAULT_BRANCH..UPSTREAM_REMOTE/DEFAULT_BRANCH -- .github/workflows/ 2>/dev/null)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Systematic inconsistency: Mix of template placeholders and shell variables.

The file mixes two incompatible patterns:

  1. Template placeholders (bare UPPERCASE): git fetch FORK_REMOTE, git checkout DEFAULT_BRANCH
  2. Shell variables (with $ syntax): "$FRESH_TOKEN" (line 108), WORKFLOW_DIFF=$(...) (line 404)

This creates confusion about which commands are executable vs. require manual substitution. For example:

  • Line 108 correctly uses "$FRESH_TOKEN"
  • Line 404 correctly assigns WORKFLOW_DIFF=$(...)
  • But lines 399-400 use bare FORK_REMOTE and UPSTREAM_REMOTE which won't work in bash

The inconsistency makes it unclear whether this file expects:

  • AI agents to substitute placeholders before execution, OR
  • Users to run commands directly with pre-set shell variables
💡 Recommended approach

Option 1 (Recommended): Use shell variables consistently throughout. Set them in Step 2e after detection:

# After determining values in Step 2b-2c, set shell variables:
UPSTREAM_REMOTE="origin"  # or whatever was detected
FORK_REMOTE="fork"        # or whatever was detected  
DEFAULT_BRANCH="main"     # or whatever was detected

Then use "$VAR" syntax in all subsequent commands:

git fetch "$FORK_REMOTE"
git fetch "$UPSTREAM_REMOTE"
WORKFLOW_DIFF=$(git diff --name-only "$FORK_REMOTE/$DEFAULT_BRANCH".."$UPSTREAM_REMOTE/$DEFAULT_BRANCH" -- .github/workflows/ 2>/dev/null)

Option 2: Add explicit guidance that ALL_CAPS placeholders must be manually substituted by the AI agent before running commands, and keep the current template style—but then remove the conflicting shell variable usage of FRESH_TOKEN and WORKFLOW_DIFF to maintain consistency.

🤖 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 399 - 404, The
script mixes bare template placeholders (FORK_REMOTE, UPSTREAM_REMOTE,
DEFAULT_BRANCH) with shell variables (e.g., "$FRESH_TOKEN", WORKFLOW_DIFF) which
is inconsistent and will break execution; pick the shell-variable approach:
after detection/or assignment set shell variables UPSTREAM_REMOTE, FORK_REMOTE
and DEFAULT_BRANCH, then update all occurrences to use the shell-variable form
(e.g., git fetch "$FORK_REMOTE", git fetch "$UPSTREAM_REMOTE" and use
"$FORK_REMOTE/$DEFAULT_BRANCH" in the WORKFLOW_DIFF assignment) so every
reference (FORK_REMOTE, UPSTREAM_REMOTE, DEFAULT_BRANCH, WORKFLOW_DIFF,
FRESH_TOKEN) is consistently used as a shell variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant