From ad238254279ea3fd0c2a6d20b358c8a42c78d8ca Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sun, 1 Feb 2026 11:38:07 +0100 Subject: [PATCH 1/3] docs: add GraphQL pagination for many review threads Add guidance for handling PRs with >100 review threads (common with automated reviewers like Copilot, Entelligence). GitHub GraphQL API has 100-item limit per page. Learned from PR #575 which had 127 review threads. Signed-off-by: Sebastian Mendel --- .../references/pull-request-workflow.md | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/skills/git-workflow/references/pull-request-workflow.md b/skills/git-workflow/references/pull-request-workflow.md index 36ea2f7..b0d0102 100644 --- a/skills/git-workflow/references/pull-request-workflow.md +++ b/skills/git-workflow/references/pull-request-workflow.md @@ -772,6 +772,41 @@ gh api graphql -f query=' }' -f owner=OWNER -f repo=REPO -F pr=NUMBER ``` +### Handling Many Review Threads (Pagination) + +**Critical:** GitHub GraphQL API has a limit of 100 items per page. For PRs with many +review comments (e.g., 127+ threads from automated reviewers), you MUST use pagination: + +```bash +# Fetch ALL threads with pagination (handles >100 threads) +gh api graphql -f query=' + query($owner: String!, $repo: String!, $pr: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + isResolved + comments(first: 1) { + nodes { body path } + } + } + } + } + } + }' -f owner=OWNER -f repo=REPO -F pr=NUMBER + +# Then fetch next page using endCursor: +# -f cursor="Y3Vyc29yOnYyOpHOABCD..." +``` + +**Real-world lesson (PR #575):** Automated reviewers can generate 100+ comment threads. +Without pagination, only the first 100 threads are returned, leaving others unaddressed. + ## Diagnosing CI Failures (Annotations First) > Failure first-step, not pre-merge gate. The Merge Gate below uses `annotations_count` as a *warnings present?* signal after success. This section is the inverse: when a workflow has *failed* and you don't yet know why, read the annotation text **first**, before any other diagnostic action. From b622a0079fd0052743e469c8e753c6c27e64e1e7 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 9 Jul 2026 12:37:18 +0200 Subject: [PATCH 2/3] docs(references): worktree path resolution and unpiped gating commands - advanced-git: with git -C .bare, a relative worktree path resolves from inside .bare - ../ or an absolute path; check for a pre-existing plain clone before nesting a .bare (two mis-placed worktrees in one session) - pull-request-workflow: generalize the piped-push trap to every gating command - a verification docker build piped through tail reported the pipe's rc and a red build was pushed as 'OK'; log-file + $? pattern Signed-off-by: Sebastian Mendel --- .../git-workflow/references/advanced-git.md | 19 +++++++++++++++++++ .../references/pull-request-workflow.md | 12 ++++++++++++ 2 files changed, 31 insertions(+) diff --git a/skills/git-workflow/references/advanced-git.md b/skills/git-workflow/references/advanced-git.md index d6cbe60..7ed7905 100644 --- a/skills/git-workflow/references/advanced-git.md +++ b/skills/git-workflow/references/advanced-git.md @@ -299,6 +299,25 @@ git worktree remove ../project-feature git worktree prune ``` +### Bare-Repo Layouts: Relative Paths Resolve from the Repo, Not Your Shell + +With the bare-clone convention (`project/.bare` + one directory per branch), +`git -C .bare worktree add ` resolves a **relative** `` from *inside* +`.bare` — `git -C` changes the process working directory first. From the project +directory, `../` is therefore correct (a sibling of `.bare`), while +`./` lands the worktree *inside* `.bare/` and `..//` +nests a stray copy one level deep: + +```bash +cd /path/to/project # contains .bare/ +git -C .bare worktree add ../feature-x origin/main # -> /path/to/project/feature-x OK +git -C .bare worktree add ./feature-x origin/main # -> /path/to/project/.bare/feature-x WRONG +``` + +When in doubt, pass an absolute path. Also check whether the target directory +already holds a **plain clone** before nesting a `.bare` into it — mixing the two +layouts in one directory leaves a repo checkout *and* a worktree tree side by side. + ### Bare-Worktree Project Layout (Recommended) **One directory per branch; never switch branches in the same folder.** diff --git a/skills/git-workflow/references/pull-request-workflow.md b/skills/git-workflow/references/pull-request-workflow.md index b0d0102..e9ff9d8 100644 --- a/skills/git-workflow/references/pull-request-workflow.md +++ b/skills/git-workflow/references/pull-request-workflow.md @@ -660,6 +660,18 @@ REMOTE_SHA=$(git ls-remote origin refs/heads/"$BR" | cut -f1) # no fetch, no f Likewise verify staging of any path that might be gitignored with `git status --short` before committing — an empty commit pushes "successfully" yet changes nothing. +The same trap applies to **every command whose exit code gates the next step**, not +just `git push`: a verification build or test run piped through `tail`/`grep` reports +the *pipe's* exit code. Real case: `docker build … 2>&1 | tail -2 && echo OK` printed +`OK` for a **failed** build, and the broken branch was pushed before anyone noticed. +Run gating commands unpiped — capture to a log file and check `$?`, then grep the log: + +```bash +docker build . > build.log 2>&1; RC=$? +echo "rc=$RC"; grep -E 'error|Good signature' build.log +[ "$RC" -eq 0 ] || exit 1 +``` + ### `--force-with-lease` Rejected with "stale info" On PRs that bots touch (auto-approve, Renovate/Dependabot, a CI step that pushes), `git push --force-with-lease` can be rejected with `stale info` even when your local work is correct: a bot updated the remote branch since your last fetch, so the lease's expected ref (your `origin/` tracking ref) no longer matches and the push aborts. This is the safety check working — don't escalate to plain `--force`. From c421d8a8b58425e0016aa4f605bfa928b431b6c6 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 9 Jul 2026 13:06:32 +0200 Subject: [PATCH 3/3] docs: address review feedback - pipeline-status wording: last command's rc unless pipefail; trailing grep fails green builds even with pipefail - gate snippet: gate on the command's own rc, log inspection non-fatal - worktree section: replaced with pointer to the existing bare-repo path-resolution section plus the unique plain-clone-mixing warning (also removes the 'worktree tree' typo) - pagination: one page per call, loop on endCursor - no ALL-threads overpromise Signed-off-by: Sebastian Mendel --- .../git-workflow/references/advanced-git.md | 21 +++++------------- .../references/pull-request-workflow.md | 22 +++++++++++-------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/skills/git-workflow/references/advanced-git.md b/skills/git-workflow/references/advanced-git.md index 7ed7905..d3a7e5b 100644 --- a/skills/git-workflow/references/advanced-git.md +++ b/skills/git-workflow/references/advanced-git.md @@ -299,24 +299,15 @@ git worktree remove ../project-feature git worktree prune ``` -### Bare-Repo Layouts: Relative Paths Resolve from the Repo, Not Your Shell +### Bare-Repo Layouts With the bare-clone convention (`project/.bare` + one directory per branch), -`git -C .bare worktree add ` resolves a **relative** `` from *inside* -`.bare` — `git -C` changes the process working directory first. From the project -directory, `../` is therefore correct (a sibling of `.bare`), while -`./` lands the worktree *inside* `.bare/` and `..//` -nests a stray copy one level deep: +relative `worktree add` paths resolve from *inside* `.bare` — see the detailed +path-resolution rules and recovery steps in the bare-repo section below. -```bash -cd /path/to/project # contains .bare/ -git -C .bare worktree add ../feature-x origin/main # -> /path/to/project/feature-x OK -git -C .bare worktree add ./feature-x origin/main # -> /path/to/project/.bare/feature-x WRONG -``` - -When in doubt, pass an absolute path. Also check whether the target directory -already holds a **plain clone** before nesting a `.bare` into it — mixing the two -layouts in one directory leaves a repo checkout *and* a worktree tree side by side. +Before nesting a `.bare` into an existing directory, check whether it already +holds a **plain clone** — mixing the two layouts leaves a repo checkout *and* a +worktree side by side in one directory. ### Bare-Worktree Project Layout (Recommended) diff --git a/skills/git-workflow/references/pull-request-workflow.md b/skills/git-workflow/references/pull-request-workflow.md index e9ff9d8..d13231d 100644 --- a/skills/git-workflow/references/pull-request-workflow.md +++ b/skills/git-workflow/references/pull-request-workflow.md @@ -661,15 +661,18 @@ REMOTE_SHA=$(git ls-remote origin refs/heads/"$BR" | cut -f1) # no fetch, no f Likewise verify staging of any path that might be gitignored with `git status --short` before committing — an empty commit pushes "successfully" yet changes nothing. The same trap applies to **every command whose exit code gates the next step**, not -just `git push`: a verification build or test run piped through `tail`/`grep` reports -the *pipe's* exit code. Real case: `docker build … 2>&1 | tail -2 && echo OK` printed -`OK` for a **failed** build, and the broken branch was pushed before anyone noticed. -Run gating commands unpiped — capture to a log file and check `$?`, then grep the log: +just `git push`: in POSIX shells a pipeline's status is that of its **last** command +(`tail`, `grep`) unless `set -o pipefail` is active — and even with pipefail, a +trailing `grep` that matches nothing fails a *green* build. Real case: +`docker build … 2>&1 | tail -2 && echo OK` printed `OK` for a **failed** build, and +the broken branch was pushed before anyone noticed. Gate on the command's own exit +code; keep log inspection out of the gate: ```bash -docker build . > build.log 2>&1; RC=$? -echo "rc=$RC"; grep -E 'error|Good signature' build.log -[ "$RC" -eq 0 ] || exit 1 +docker build . > build.log 2>&1 +rc=$? +tail -20 build.log # inspection only — never part of the gate +[ "$rc" -eq 0 ] || exit 1 ``` ### `--force-with-lease` Rejected with "stale info" @@ -790,7 +793,8 @@ gh api graphql -f query=' review comments (e.g., 127+ threads from automated reviewers), you MUST use pagination: ```bash -# Fetch ALL threads with pagination (handles >100 threads) +# Fetch ONE page of up to 100 threads; repeat with the returned endCursor +# until hasNextPage is false to cover all threads gh api graphql -f query=' query($owner: String!, $repo: String!, $pr: Int!, $cursor: String) { repository(owner: $owner, name: $repo) { @@ -812,7 +816,7 @@ gh api graphql -f query=' } }' -f owner=OWNER -f repo=REPO -F pr=NUMBER -# Then fetch next page using endCursor: +# Loop until pageInfo.hasNextPage is false, passing each endCursor: # -f cursor="Y3Vyc29yOnYyOpHOABCD..." ```