diff --git a/docs/solutions/skill-design/git-workflow-skills-need-explicit-state-machines-2026-03-27.md b/docs/solutions/skill-design/git-workflow-skills-need-explicit-state-machines-2026-03-27.md index 01a7d7d..097937b 100644 --- a/docs/solutions/skill-design/git-workflow-skills-need-explicit-state-machines-2026-03-27.md +++ b/docs/solutions/skill-design/git-workflow-skills-need-explicit-state-machines-2026-03-27.md @@ -42,6 +42,7 @@ The `git-commit` and `git-commit-push-pr` skills had accumulated branch-state an - Using a single early `git branch --show-current` result and referring back to it later. Once the workflow creates a branch, the earlier value is stale. - Using `git diff HEAD` as the definition of "has changes." It does not account for untracked files. - Treating every non-zero exit from `gh pr view` as a fatal failure. "No PR for this branch" is often a normal branch state. +- Letting the shell tool surface that expected `gh pr view` non-zero exit as a visible failed step. Even when the logic recovers correctly, the UX looks broken and pushes future edits toward less-correct commands. - Switching from `gh pr view` to `gh pr list --head ""` to avoid the no-PR error path. This improved ergonomics but weakened correctness because `gh pr list` cannot disambiguate `:`. - Adding a "clean working tree" fast path before re-checking whether the current branch was still the default branch. That let the workflow skip the feature-branch safety gate and head straight toward invalid push/PR transitions. @@ -112,6 +113,8 @@ Interpret it as a state check: - Non-zero exit with output indicating no PR for the current branch -> expected "no PR yet" state - Any other failure -> real error +When the shell/tooling layer renders non-zero exits as scary visible failures, wrap the command so the skill captures both the output and exit code and then interprets them explicitly. The user should see "no PR for this branch" as a normal state transition, not as a broken Bash step. + This keeps PR detection tied to the current branch context instead of a bare branch name that may be reused across forks. ### 5. Keep the default-branch safety gate ahead of push/PR transitions @@ -233,6 +236,7 @@ Learning: these skills should be designed and reviewed like tiny state machines, - Re-run the command that answers the current question at the point of decision. Do not rely on values gathered earlier if a mutating command may have changed them. - Use `git status` for "is there local work?" and reserve `git diff` for describing content, not determining whether work exists. - Model expected non-zero CLI exits explicitly when they represent state, such as `gh pr view` on a branch with no PR. +- When a tool visually highlights non-zero exits as failures, capture the exit code yourself for expected state probes so correct logic does not still look broken to the user. - Avoid branch-name-only PR detection for multi-fork repos. If the command cannot disambiguate branch ownership, prefer a current-branch-aware command even if the failure path is slightly messier. - Keep default-branch safety checks in every path that can lead to push or PR creation, including "clean working tree but unpushed commits" shortcuts. - When editing skill logic, manually walk these cases before considering the change complete: diff --git a/plugins/compound-engineering/skills/git-commit-push-pr/SKILL.md b/plugins/compound-engineering/skills/git-commit-push-pr/SKILL.md index be2c317..c9737cc 100644 --- a/plugins/compound-engineering/skills/git-commit-push-pr/SKILL.md +++ b/plugins/compound-engineering/skills/git-commit-push-pr/SKILL.md @@ -13,6 +13,28 @@ If the user is asking to update, refresh, or rewrite an existing PR description For description-only updates, follow the Description Update workflow below. Otherwise, follow the full workflow. +## Reusable PR probe + +When checking whether the current branch already has a PR, keep using current-branch `gh pr view` semantics. Do **not** switch to `gh pr list --head ""` just to avoid the no-PR exit path. That branch-name search can select the wrong PR in multi-fork repos. + +Also do **not** run bare `gh pr view --json ...` in a way that lets the shell tool render the expected no-PR state as a red failed step. Capture the output and exit code yourself so you can interpret "no PR for this branch" as normal workflow state: + +```bash +if PR_VIEW_OUTPUT=$(gh pr view --json url,title,state 2>&1); then + PR_VIEW_EXIT=0 +else + PR_VIEW_EXIT=$? +fi +printf '%s\n__GH_PR_VIEW_EXIT__=%s\n' "$PR_VIEW_OUTPUT" "$PR_VIEW_EXIT" +``` + +Interpret the result this way: + +- `__GH_PR_VIEW_EXIT__=0` and JSON with `state: OPEN` -> an open PR exists for the current branch +- `__GH_PR_VIEW_EXIT__=0` and JSON with a non-OPEN state -> treat as no open PR +- non-zero exit with output indicating `no pull requests found for branch` -> expected no-PR state +- any other non-zero exit -> real error (auth, network, repo config, etc.) + --- ## Description Update workflow @@ -36,10 +58,15 @@ If empty (detached HEAD), report that there is no branch to update and stop. Otherwise, check for an existing open PR: ```bash -gh pr view --json url,title,state +if PR_VIEW_OUTPUT=$(gh pr view --json url,title,state 2>&1); then + PR_VIEW_EXIT=0 +else + PR_VIEW_EXIT=$? +fi +printf '%s\n__GH_PR_VIEW_EXIT__=%s\n' "$PR_VIEW_OUTPUT" "$PR_VIEW_EXIT" ``` -Interpret the result. Do not treat every non-zero exit as a fatal error here: +Interpret the result using the Reusable PR probe rules above: - If it returns PR data with `state: OPEN`, an open PR exists for the current branch. - If it returns PR data with a non-OPEN state (CLOSED, MERGED), treat this as "no open PR." Report that no open PR exists for this branch and stop. @@ -129,10 +156,15 @@ Run `git branch --show-current` to get the current branch name. If it returns an Then check for an existing open PR: ```bash -gh pr view --json url,title,state +if PR_VIEW_OUTPUT=$(gh pr view --json url,title,state 2>&1); then + PR_VIEW_EXIT=0 +else + PR_VIEW_EXIT=$? +fi +printf '%s\n__GH_PR_VIEW_EXIT__=%s\n' "$PR_VIEW_OUTPUT" "$PR_VIEW_EXIT" ``` -Interpret the result. Do not treat every non-zero exit as a fatal error here: +Interpret the result using the Reusable PR probe rules above: - If it **returns PR data with `state: OPEN`**, an open PR exists for the current branch. Note the URL and continue to Step 4 (commit) and Step 5 (push). Then skip to Step 7 (existing PR flow) instead of creating a new PR. - If it **returns PR data with a non-OPEN state** (CLOSED, MERGED), treat this the same as "no PR exists" -- the previous PR is done and a new one is needed. Continue to Step 4 through Step 8 as normal. @@ -205,11 +237,11 @@ Once the base branch and remote are known: ```bash git merge-base / HEAD ``` -2. List all commits unique to this branch: +3. List all commits unique to this branch: ```bash git log --oneline ..HEAD ``` -3. Get the full diff a reviewer will see: +4. Get the full diff a reviewer will see: ```bash git diff ...HEAD ```