Authenticate index generation as a GitHub App - #41
Merged
Merged
Conversation
Index generation has been failing since `require-pr-review` was added to `agent-protocols`. The generator runs fine; the push is rejected with GH013, because `github-actions[bot]` is not a bypass actor. It cannot be made one. Bypass actors of type Integration must be GitHub Apps installed on the organisation, and GitHub Actions is not one — the API rejects app id 15368 outright. A write-scoped deploy key is a valid bypass actor type and is scoped to the single repository. `actions/checkout` takes an `ssh-key` input, loads the key, and sets the remote to SSH, so the composite action's existing `git push` needs no change. Only the caller's checkout does. Documented in the template workflow, in the action's push step, and as an adoption step in the README, since an adopter with a protected branch hits this before their first index generates. Two notes worth having next to the code. A deploy-key push *does* trigger workflows where a GITHUB_TOKEN push does not, so the loop protection matters now: the index commit is outside the caller's `paths:` filter, and the default commit message carries [skip ci] besides. Also corrects a sentence I broke in the citation rename. The README claimed the starter's `method_origin_citation` is the placeholder "in the required `protocol_citation`", which is incoherent — the placeholder is in `protocol_citation`, and `method_origin_citation` ships commented out. 77 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #39 committed ten `.pyc` files. They are build artifacts keyed to the interpreter version, so every contributor on a different Python adds their own set: running the test suite locally on 3.14 produced ten more alongside the committed 3.12 ones, and `git add -A` swept them into a commit before I noticed. Untracked and gitignored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Scope the loop-protection guarantee to the template workflow and address equivalent safeguards for other callers.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates index generation to use a deploy key for protected branches, documents setup, corrects citation guidance, and ignores Python bytecode caches.
Changes:
- Passes
INDEX_DEPLOY_KEYto checkout. - Documents deploy-key configuration and loop safeguards.
- Corrects citation wording and adds Python cache exclusions.
File summaries
| File | Description |
|---|---|
template/.github/workflows/generate-index.yml |
Configures SSH deploy-key checkout. |
README.md |
Adds adoption instructions and citation correction. |
actions/generate-index/action.yml |
Documents push authentication and loop protection. |
.gitignore |
Ignores Python bytecode artifacts. |
Review details
- Files reviewed: 3/14 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A repository secret is readable by any workflow in the repository, including one added in a pull request — secrets are withheld from fork pull requests, but same-repo branch pull requests do receive them. With five contributors about to get write access and a key that bypasses branch protection, that is the wrong place for it. An environment secret whose deployment branch policy allows only `main` closes it: a job can read the secret only if it declares the environment and is running on `main`. The `on: push: branches: [main]` trigger already prevents pull-request execution today, so this makes the property structural rather than dependent on nobody adding a workflow_dispatch later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A deploy key is a static credential valid until someone revokes it, and ruleset bypass applies to the whole ruleset — so the key could force-push or delete `main`, not merely skip the pull-request requirement. A purpose-built App holds `contents: write` and nothing else, mints a token that expires after an hour, and installs once on the organisation rather than once per repository, which matters as soon as there is a second content node. GitHub's own deploy-key page recommends this; I offered four options earlier and an App was not among them, which was the gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lwaldron
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved workflow authentication and recursion-documentation issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
actions/generate-index/action.yml:50
- The loop-prevention explanation has the same stale assumption: the template's App-token push, like a deploy-key push, does trigger workflows, whereas a
GITHUB_TOKENpush does not. Please make this statement cover both non-GITHUB_TOKENcredentials so it accurately explains why the new template is safe.
# This cannot loop. A deploy-key push does trigger workflows (a GITHUB_TOKEN push does not), but
# the commit touches only the index, which is outside the caller's `paths:` filter, and the
# default commit message carries [skip ci] as well.
actions/generate-index/action.yml:50
generate_protocols_yaml.pywrites a freshgenerated_attimestamp on every run, while this action lets callers change bothinputs.outputandinputs.commit-message. A caller that includes that output in its push-path filter and overrides the message without[skip ci]will retrigger the action and create another changed index, so “This cannot loop” is only true for the template's current filters and default. Qualify the claim or enforce a recursion guard in the action.
# This cannot loop. A deploy-key push does trigger workflows (a GITHUB_TOKEN push does not), but
# the commit touches only the index, which is outside the caller's `paths:` filter, and the
# default commit message carries [skip ci] as well.
- Files reviewed: 3/14 changed files
- Comments generated: 2
- Review effort level: Lite
The template ran `create-github-app-token` unconditionally while the README told adopters with an unprotected `main` to skip creating the App. Both inputs would be empty, the step would fail before checkout, and the first index would never generate. It is now skipped when no App is configured and falls back to GITHUB_TOKEN, so the template works either way. `secrets` is not available to a step `if:`, hence the job-level `env`. `permission-contents: write` is now requested explicitly rather than inheriting whatever the installation happens to hold, so granting the App another permission later cannot silently widen the token. GITHUB_TOKEN drops to `contents: read` in the content repository, where the App always pushes. The template keeps `contents: write`, because its fallback path pushes with GITHUB_TOKEN — reducing it there would have broken the case the fallback exists for. The comment says which is which. The action's comment still described a deploy key and `ssh-key` after the switch to an App token, and claimed the push "cannot loop". Neither was true of the action: loop protection lives in the template's `paths:` filter and `[skip ci]` default, and a caller with neither will loop. Both corrected, with the guarantee scoped to the caller. Commit attribution is now an input rather than a hardcoded github-actions[bot], defaulting to it so existing callers are unaffected. The workflows pass the App's own identity, since that is what pushes. Declined, deliberately: pinning generate-index@v0 to a commit SHA, and protecting tags on this repository. The escalation path needs write access to this repo, which is the same set of people who already have write access to the content repo, and pinning would give up the moving-tag design ADR 0006 §3 argues for. 77 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Address the critical shell-injection risk and the moderate least-privilege documentation issue.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/14 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+70
to
+71
| git config --local user.name "${{ inputs.committer-name }}" | ||
| git config --local user.email "${{ inputs.committer-email }}" |
Comment on lines
+70
to
+71
| configured. The template's workflow already declares the environment and passes both to | ||
| `actions/create-github-app-token`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Index generation on a content repository fails once
mainis protected: the generator runs, the push is rejected withGH013, becausegithub-actions[bot]is not a bypass actor.It cannot be made one — bypass actors of type
Integrationmust be GitHub Apps installed on the organisation, and GitHub Actions is not one. The API rejects app id 15368 outright. A purpose-built App is, and that's what the template now uses.The change
template/.github/workflows/generate-index.yml— mints a token withactions/create-github-app-tokenand passes it toactions/checkout, inside anindex-generationenvironment.actions/generate-index/action.yml— comments only, explaining what the push authenticates with and why it cannot loop.README.md— a new adoption step, since an adopter with a protected branch hits this before their first index ever generates.The composite action's
git pushis unchanged:actions/checkoutpersists whatever credential it was given.Why an App rather than a deploy key
Both work. The App is better on two counts that matter here.
Ruleset bypass is not per-rule. A
DeployKeybypass covers the whole ruleset, so the key could force-push or deletemain, not merely skip the pull-request requirement. An App scoped tocontents: writecannot.Static versus expiring. A deploy key is valid until revoked; an installation token lasts an hour.
It also installs once on an organisation rather than once per repository, which matters as soon as a second content node exists.
Why environment secrets rather than repository secrets
A repository secret is readable by any workflow in the repository, including one added in a pull request — secrets are withheld from fork PRs, but same-repo branch PRs do receive them, and these mint a token that bypasses branch protection. The environment's deployment branch policy restricts it to
main.The
on: push: branches: [main]trigger already prevents PR execution, so this is belt-and-braces — but it makes the property structural rather than dependent on nobody adding aworkflow_dispatchlater.Two smaller things in here
A README sentence I broke in the citation rename. It claimed the starter's
method_origin_citationis the placeholder "in the requiredprotocol_citation", which is incoherent — the placeholder is inprotocol_citation, andmethod_origin_citationships commented out.Untracking the bytecode caches, in its own commit and separable if you'd rather it weren't here. #39 committed ten
.pycfiles; they're interpreter-keyed, so running the suite locally on 3.14 produced ten more beside the committed 3.12 ones.77 tests pass.
🤖 Generated with Claude Code