From c47d63d088a64cb120d6f1fac2324c497595c569 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 5 May 2026 23:33:45 +0200 Subject: [PATCH 1/3] feat(github-project): document PR merge, branch protection, and CodeQL gotchas Capture cross-project gotchas siloed in downstream project memory: - security-config.md: explicit `permissions: read-all` anti-pattern in Token-Permissions section (scores 0 vs 10 for explicit per-permission scopes); CodeQL Supported Languages subsection noting PHP is not supported (use `javascript-typescript` + `actions` matrix on PHP/TYPO3 repos). - tag-validation.md: expand the one-line bullets in "Batch PR Merging Gotchas" into proper subsections covering `gh pr merge --delete-branch` failing under merge queues (with detection snippet) and Contents API commits not satisfying `required_signatures` (with three workarounds: `--admin`, SSH push, GitHub App). Cross-reference auto-merge-guide.md for the stale-reviews / Copilot-race and signed-rebase cases that are already canonically documented there rather than restate them. Signed-off-by: Sebastian Mendel --- .../references/security-config.md | 34 +++++++++++++++ .../references/tag-validation.md | 43 +++++++++++++++++-- 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/skills/github-project/references/security-config.md b/skills/github-project/references/security-config.md index 5311ad7..88e93b5 100644 --- a/skills/github-project/references/security-config.md +++ b/skills/github-project/references/security-config.md @@ -40,6 +40,22 @@ permissions: > **Scorecard note:** The OpenSSF Scorecard Token-Permissions check flags workflow-level `write` permissions. +### Anti-pattern: `permissions: read-all` + +`read-all` is the lazy "make Scorecard stop complaining about missing permissions" knob — but it scores **0** on the Token-Permissions check, not full marks. The check wants **explicit, per-permission scopes** so a reviewer can audit what each workflow actually needs. + +```yaml +# WRONG — scores 0 on Token-Permissions +permissions: read-all + +# RIGHT — explicit scopes, scores 10 +permissions: + contents: read + # add only what's needed (e.g. pull-requests: read for label/check workflows) +``` + +If a workflow only reads code, just `contents: read` is enough. Add other read scopes one-by-one as steps need them. Never use `read-all` as a placeholder you mean to tighten "later" — Scorecard treats it as wide-open. + ### SLSA Provenance: Use actions/attest-build-provenance (not slsa-github-generator) `slsa-framework/slsa-github-generator` **cannot be SHA-pinned** — known unfixable limitation ([#4440](https://github.com/slsa-framework/slsa-github-generator/issues/4440), [slsa-verifier#12](https://github.com/slsa-framework/slsa-verifier/issues/12)). Its internal actions use tag refs that conflict with SHA-pinning rulesets. @@ -208,6 +224,24 @@ gh api repos/OWNER/REPO/code-scanning/default-setup -X PATCH -f state=not-config gh api repos/OWNER/REPO/code-scanning/default-setup --jq 'if .state == "not-configured" then "OK: Default Setup disabled" else "FAIL: Default Setup still enabled - DISABLE IT" end' ``` +### Supported Languages — PHP Is NOT Supported + +CodeQL does **not** support PHP (as of 2026; tracked in [community discussion #158392](https://github.com/orgs/community/discussions/158392)). On a PHP/TYPO3 repo, the only languages worth scanning are: + +- `javascript-typescript` — covers JS, TS, and JSX/TSX in `Resources/Public/JavaScript/` and similar +- `actions` — scans `.github/workflows/*.yml` for misconfigurations + +A PHP-only repo with neither JS nor non-trivial workflows has **nothing CodeQL can scan** — disabling Default Setup and skipping the custom workflow is correct. + +```yaml +# .github/workflows/codeql.yml — PHP/TYPO3 repo +strategy: + matrix: + language: [javascript-typescript, actions] # NOT 'php' +``` + +If you list `php` in the matrix, the workflow fails at the `init` step with "Unrecognised language: php". If you list `javascript` (the old name), CodeQL Action v3+ rejects it — use `javascript-typescript`. + ## Required Reviews from All Requested Reviewers (MANDATORY) PRs must **not be merged until all requested reviewers have submitted their review**. This includes human reviewers and automated reviewers (e.g., GitHub Copilot). Do not merge while any reviewer's status is still "PENDING". diff --git a/skills/github-project/references/tag-validation.md b/skills/github-project/references/tag-validation.md index 8313d86..efdb3fe 100644 --- a/skills/github-project/references/tag-validation.md +++ b/skills/github-project/references/tag-validation.md @@ -204,11 +204,46 @@ Remove the exemption once the upstream fix is released. When merging PRs across many repos: -- **Check allowed merge methods** — repos may only allow rebase, squash, or merge commits. Use `gh api repos/OWNER/REPO --jq '{allow_merge: .allow_merge_commit, allow_rebase: .allow_rebase_merge, allow_squash: .allow_squash_merge}'` +- **Check allowed merge methods** — repos may only allow rebase, squash, or merge commits. Use `gh api repos/OWNER/REPO --jq '{allow_merge: .allow_merge_commit, allow_rebase: .allow_rebase_merge, allow_squash: .allow_squash_merge}'`. Detection snippet + the rebase-merge-cannot-be-signed caveat are in [`auto-merge-guide.md`](./auto-merge-guide.md) → "Signed Commits and Merge Strategy Compatibility". Real-world heterogeneity to expect: across one Netresearch fleet alone, some repos allow only rebase, some only merge commits, and some all three. - **`--admin` bypasses branch protection** — useful when `enforce_admins` is false and you're a repo admin -- **`--delete-branch` fails with merge queues** — omit the flag for repos with merge queues enabled -- **`dismiss_stale_reviews` clears approvals on force-push** — after rebasing, auto-approve workflows must re-run -- **GitHub API content pushes aren't GPG-signed** — commits via the Contents API use GitHub's web-flow committer +- **`dismiss_stale_reviews` clears approvals on force-push** — after rebasing, prior approvals are dismissed and any auto-approve workflow must re-run. This is expected behavior, not a bug. See [`auto-merge-guide.md`](./auto-merge-guide.md) → "Auto-Approve Race Condition with Copilot Reviewer" for the variant where auto-approve fires before Copilot finishes reviewing and the PR ends up `REVIEW_REQUIRED` blocked. + +### `gh pr merge --delete-branch` fails with merge queues + +Repos with merge queues enabled reject the `-d` / `--delete-branch` flag — the queue manages the head-branch lifecycle itself. Symptom: `gh pr merge` exits non-zero with an error mentioning the merge queue, even though the PR was added to the queue successfully. + +**Fix — detect the queue first, then conditionally drop the flag:** + +```bash +has_queue=$(gh api "repos/$REPO" --jq '.merge_queue // null') +if [[ "$has_queue" == "null" ]]; then + gh pr merge "$PR" --repo "$REPO" --merge --delete-branch +else + gh pr merge "$PR" --repo "$REPO" --merge # queue handles branch deletion +fi +``` + +In a batch loop across mixed repos, always run the detection per repo — assuming "no queue" silently leaves stale branches behind on the queue-enabled ones, and assuming "queue" causes `--delete-branch` failures on the unqueued ones. + +### Contents API commits don't satisfy `required_signatures` + +`gh api -X PUT repos/.../contents/...` is the fastest way to land a one-line edit across many repos, but the resulting commits use GitHub's `web-flow` committer identity — they are **not signed with your GPG/SSH key**. On any repo with branch protection `required_signatures: true`, those commits are rejected by the merge gate. + +**Symptom:** the API push itself succeeds (HTTP 201), but a follow-up PR or the same-branch merge fails with: + +``` +Required signatures: At least one of the commits is not signed. +``` + +Or, on default-branch direct pushes, HTTP 409 from the contents API itself when branch protection blocks unsigned commits. + +**Workarounds (pick the one that matches your situation):** + +1. **`gh pr merge --admin`** — bypass branch protection on a per-merge basis. Requires `enforce_admins: false` AND admin role on the repo. Cleanest for solo-maintainer batch ops. +2. **Push real local commits via SSH** instead of using the contents API. Slightly slower per repo, but the commits carry your signature and need no bypass. This is the safer default when the batch mixes signing-required and signing-optional repos — it works uniformly. +3. **GitHub App with verified signing** — if the batch tool is something other people will run too, register a GitHub App with a verified signing identity and have it commit on your behalf. Heavier setup, but no per-repo admin bypass and no per-user keys. + +See also [`multi-repo-operations.md`](./multi-repo-operations.md) → "Contents API vs branch protection" for the related HTTP 409 angle (PR-required / merge-queue-required repos rejecting contents API pushes regardless of signing). ## Why Not Just Use `tailor set-version`? From 123ba0eadb4dab462944326bfba71a18975ac126 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 5 May 2026 23:38:41 +0200 Subject: [PATCH 2/3] fix(github-project): use GraphQL Repository.mergeQueue for queue detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original snippet queried `gh api "repos/$REPO"` for a `merge_queue` field that the REST endpoint does not actually return — it would always evaluate to null and never detect the queue, defeating the gotcha's whole purpose. Caught by Gemini Code Assist on PR #72. Use the GraphQL `Repository.mergeQueue` field instead (returns null when no queue is configured) and add explicit error handling on the API call. Signed-off-by: Sebastian Mendel --- skills/github-project/references/tag-validation.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/skills/github-project/references/tag-validation.md b/skills/github-project/references/tag-validation.md index efdb3fe..75247e7 100644 --- a/skills/github-project/references/tag-validation.md +++ b/skills/github-project/references/tag-validation.md @@ -212,10 +212,16 @@ When merging PRs across many repos: Repos with merge queues enabled reject the `-d` / `--delete-branch` flag — the queue manages the head-branch lifecycle itself. Symptom: `gh pr merge` exits non-zero with an error mentioning the merge queue, even though the PR was added to the queue successfully. -**Fix — detect the queue first, then conditionally drop the flag:** +**Fix — detect the queue first, then conditionally drop the flag.** Note: `gh api "repos/$REPO"` does NOT return a `merge_queue` field; query GraphQL `Repository.mergeQueue` instead, which returns `null` when no queue is configured: ```bash -has_queue=$(gh api "repos/$REPO" --jq '.merge_queue // null') +OWNER="${REPO%/*}"; NAME="${REPO#*/}" +has_queue=$(gh api graphql -f query="{ repository(owner: \"$OWNER\", name: \"$NAME\") { mergeQueue { id } } }" \ + --jq '.data.repository.mergeQueue // "null"') +if [[ "$?" -ne 0 ]]; then + echo "Error: failed to query merge queue status for $REPO" >&2 + exit 1 +fi if [[ "$has_queue" == "null" ]]; then gh pr merge "$PR" --repo "$REPO" --merge --delete-branch else From f5344342a5b8ece5efc31aa656a7b070ad30c997 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 5 May 2026 23:39:26 +0200 Subject: [PATCH 3/3] fix(github-project): auto-detect allowed merge strategy in --delete-branch snippet Hardcoding `--merge` contradicts the bullet right above warning that repos may only allow squash or rebase. In a mixed fleet, copying the "fix" still produces "merge method not allowed" failures on squash-only or rebase-only repos. Wire in the same dynamic strategy detection used in auto-merge-guide.md so both the queue check and the strategy choice are per-repo. Caught by Copilot review on PR #72. Signed-off-by: Sebastian Mendel --- skills/github-project/references/tag-validation.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/skills/github-project/references/tag-validation.md b/skills/github-project/references/tag-validation.md index 75247e7..8b15e47 100644 --- a/skills/github-project/references/tag-validation.md +++ b/skills/github-project/references/tag-validation.md @@ -222,14 +222,22 @@ if [[ "$?" -ne 0 ]]; then echo "Error: failed to query merge queue status for $REPO" >&2 exit 1 fi +# Auto-detect allowed merge strategy too — repos may not allow `--merge`. +# See auto-merge-guide.md → "Signed Commits and Merge Strategy Compatibility". +STRATEGY=$(gh api "repos/$REPO" --jq ' + if .allow_squash_merge then "--squash" + elif .allow_merge_commit then "--merge" + elif .allow_rebase_merge then "--rebase" + else "--squash" end') + if [[ "$has_queue" == "null" ]]; then - gh pr merge "$PR" --repo "$REPO" --merge --delete-branch + gh pr merge "$PR" --repo "$REPO" "$STRATEGY" --delete-branch else - gh pr merge "$PR" --repo "$REPO" --merge # queue handles branch deletion + gh pr merge "$PR" --repo "$REPO" "$STRATEGY" # queue handles branch deletion fi ``` -In a batch loop across mixed repos, always run the detection per repo — assuming "no queue" silently leaves stale branches behind on the queue-enabled ones, and assuming "queue" causes `--delete-branch` failures on the unqueued ones. +In a batch loop across mixed repos, always run **both** detections per repo — assuming "no queue" silently leaves stale branches behind on the queue-enabled ones, assuming "queue" causes `--delete-branch` failures on the unqueued ones, and hardcoding `--merge` causes "merge method not allowed" failures on repos that only permit squash or rebase. ### Contents API commits don't satisfy `required_signatures`