Conversation
kriszyp
approved these changes
Apr 28, 2026
Ethan-Arrowood
approved these changes
Apr 29, 2026
Member
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
These scripts are quite long and by embedding inside the yaml file they don't have any sort of syntax coloring (and likely no way to reliably enforce syntax). I'm sure the AI is capable of handling this, but me the human has a hard time reading (and reviewing) it. Due to their length and complexity, I'd prefer we store these in .sh files so they are easier to review. But won't block on that. Maybe something to fix later.
3 tasks
heskew
added a commit
that referenced
this pull request
May 1, 2026
The inline `run: |` blocks in claude-review.yml and claude-mention.yml have grown past the point of being reviewable inside YAML — the log step alone was 142 lines. Pulls them out to standalone bash scripts: - .github/scripts/compose-review-scope.sh (was 36-line inline) - .github/scripts/find-prior-review-comment.sh (was 10-line inline) - .github/scripts/log-review-to-ai-review-log.sh (was 142-line inline) - .github/scripts/parse-claude-mention.sh (was 17-line inline) Each script documents its inputs, outputs, and the rationale for non-obvious mechanics inline. Workflows invoke via `bash .github/scripts/<name>.sh` rather than direct path execution — sidesteps the +x bit being dropped on Windows checkouts and the "forgot to chmod the new script" footgun. The `#!/usr/bin/env bash` shebangs are now informational only. Stacked on top of #444 (marker-based review-comment edit), which introduces the "Find prior review comment" step and the marker'd log-step lookup. Will rebase to main once #444 merges. Tests for these scripts are deliberately out of scope — Nathan's preference per discussion. A separate PR will add coverage via `npm run test:workflows` (or similar) once we settle on the test runner shape. #417 (auth-gate work) carries its own inline scripts and is not touched here. When that branch updates, its scripts adopt the same .github/scripts/ structure as part of that PR's review fixup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…heck Symptom: harper PR #411 from a real org member was silently skipped — `Claude PR Review` evaluated its job-level `if:` to false and ran zero steps. Cause: GitHub's webhook `author_association` is unreliable. It reports `CONTRIBUTOR` (or `NONE`) for org members with private membership AND for users whose repo access comes via team membership rather than direct collaborator status. Real HarperFast team members fall into both buckets. Forcing visibility changes is hostile UX, and the collaborators API would admit a broader population (read-only collaborators, default-org-permission users) than we want. Fix: two-job pattern with team-membership check. Each workflow has an `authorize` job that runs first, mints an installation token from a HarperFast-org-owned GitHub App (Members:Read scope), and checks team membership. The work/review job has ONE `if: needs.authorize.outputs.authorized == 'true'`. No step-level guards, no individual user list to maintain. The App token lives in the authorize job ONLY — the work job uses the default GITHUB_TOKEN, so the org-read capability never reaches the agent step. CODEOWNERS-driven trust set: - The auth check reads `.github/CODEOWNERS` via the default token and extracts every `@HarperFast/<team>` handle as the trust set. - Same set as people we trust to review code; alignment by construction. New owner team in CODEOWNERS automatically extends trigger trust. New consumer repo inherits its own CODEOWNERS. - External-org handles are deliberately ignored — only HarperFast teams. - If CODEOWNERS is missing, empty, or has no @HarperFast handles, falls back to @HarperFast/developers. Per-workflow specifics: - claude-review.yml: checks BOTH the PR author (`pull_request.user.login`) AND the event actor (`github.actor`). A non-trusted user pushing to a trusted user's PR branch changes the actor without changing the PR author; refusing those events closes that loophole. claude[bot] is admitted explicitly so AI-authored PRs from the issue-to-PR pipeline get reviewed (ADMIT_CLAUDE_BOT=true). - claude-mention.yml: checks the commenter. claude[bot] not admitted here (only humans trigger mentions). - claude-issue-to-pr.yml: checks the LABELER (github.actor), not the issue author. The labeler must already have at least triage permission; a maintainer labeling an external-author issue is a legitimate way to invoke the agent on community reports. Per the post-#447 convention, the auth-check bash lives in `.github/scripts/authorize-claude-workflow.sh` (shared across all three workflows; parameterized by env vars). Workflows invoke via `bash .github/scripts/...`. Defense-in-depth lint: - New `.github/workflows/auth-gate-invariants.yml` runs on any PR touching a `claude-*.yml` workflow file. Validates structurally via `bash .github/scripts/validate-auth-gate-invariants.sh`: * `authorize` job exists. * `authorize.outputs.authorized` wired to a step output. * `actions/create-github-app-token` present and pinned to a SHA. * `authorize.permissions` has no `write` scopes. * `HARPERFAST_AI_CLIENT_ID` and `HARPERFAST_AI_APP_PRIVATE_KEY` secrets referenced. * Every non-authorize job has `needs: authorize` and an exact `if: needs.authorize.outputs.authorized == 'true'` (no compound expressions, no tautologies). Make this a REQUIRED status check on `main` via branch protection. Subtle attacks on the bash logic are caught by CODEOWNERS review on `.github/`. Required (organization-level) secrets — must be set on the HarperFast org for any consumer repo to authorize a Claude run: - HARPERFAST_AI_CLIENT_ID (the App's Client ID, like Iv23li…) - HARPERFAST_AI_APP_PRIVATE_KEY (.pem file contents) Replaces #417's two earlier commits (which were on a stale base that pre-dated #432, #437, #438, #439, #442, #444, #447). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
heskew
force-pushed
the
workflow/collaborator-check
branch
from
5ea5a75 to
277616e
Compare
cb1kenobi
approved these changes
May 1, 2026
…r enforces Address review on #417: 1. **Auth check fail-open (high).** `authorize-claude-workflow.sh` would fall through to `authorized=true` when `USERS_TO_CHECK` was empty or whitespace-only. The `read` loop's heredoc on `""` reads zero non-empty lines, every iteration is skipped by `[ -z "$user" ] && continue`, and the trailing `echo "authorized=true"` wins. A workflow that forgot to set USERS_TO_CHECK — or a malicious PR that removed it — would admit every event. Adds a guard right after the trust-set resolution: `${USERS_TO_CHECK//[[:space:]]/}` empty → `::error::`, `authorized=false`, exit 0. 2. **Validator gap (medium).** `validate-auth-gate-invariants.sh` structurally checked the authorize job, secrets, and `needs:`/`if:` shape, but never asserted that USERS_TO_CHECK was wired to a step in the authorize job. With (1) fixed, an omission fails closed at runtime — but the validator should still trip structurally so the omission is caught before merge. Adds new check #6: `yq` over `.jobs.authorize.steps[].env.USERS_TO_CHECK`, fails the workflow if no step sets it. Verified locally: - Empty / whitespace-only USERS_TO_CHECK → script denies. - Non-empty USERS_TO_CHECK → loop runs as before. - All three claude-*.yml workflows already set USERS_TO_CHECK on the right step (validator passes against current PR). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
6 tasks
cb1kenobi
pushed a commit
that referenced
this pull request
May 4, 2026
Symptom: every PR was failing `Auth gate invariants / validate` with `authorize job has no step setting USERS_TO_CHECK env var`, even on workflows that clearly set it. Reported on PR #452 and others. Cause: the check `USERS_TO_CHECK presence` (added in #417 review- fixup) was using a jq-flavored expression — `// empty` in particular — but yq on ubuntu-latest is `mikefarah/yq` (Go), not jq. yq's lexer rejects `empty`. The script had `2>/dev/null` on the yq invocation, so the lexer error was eaten and the variable came back as the empty string, tripping the existence check on every workflow. Fix: rewrite the expression in idiomatic yq: - `.jobs.authorize.steps[].env.USERS_TO_CHECK | select(. != null)` — emits a stream of one value per step that sets the env var, skipping steps that don't. - `head -1` collapses the stream to a single value (or empty) for the `[ -n ]` check. Verified locally with mikefarah/yq v4.53.2 against all three harper claude-*.yml workflows — all pass. Out of scope but worth following up: - The `2>/dev/null` swallowed a real yq error. Worth either removing the suppression or capturing stderr for diagnostics when the expression returns empty. - oauth's mirror (#68) carries the same bug; a copy of this fix needs to land there too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 4, 2026
Symptom: every PR was failing `Auth gate invariants / validate` with `authorize job has no step setting USERS_TO_CHECK env var`, even on workflows that clearly set it. Reported on PR #452 and others. Cause: the check `USERS_TO_CHECK presence` (added in #417 review- fixup) was using a jq-flavored expression — `// empty` in particular — but yq on ubuntu-latest is `mikefarah/yq` (Go), not jq. yq's lexer rejects `empty`. The script had `2>/dev/null` on the yq invocation, so the lexer error was eaten and the variable came back as the empty string, tripping the existence check on every workflow. Fix: rewrite the expression in idiomatic yq: - `.jobs.authorize.steps[].env.USERS_TO_CHECK | select(. != null)` — emits a stream of one value per step that sets the env var, skipping steps that don't. - `head -1` collapses the stream to a single value (or empty) for the `[ -n ]` check. Verified locally with mikefarah/yq v4.53.2 against all three harper claude-*.yml workflows — all pass. Out of scope but worth following up: - The `2>/dev/null` swallowed a real yq error. Worth either removing the suppression or capturing stderr for diagnostics when the expression returns empty. - oauth's mirror (#68) carries the same bug; a copy of this fix needs to land there too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
The inline `run: |` blocks in claude-review.yml and claude-mention.yml have grown past the point of being reviewable inside YAML — the log step alone was 142 lines. Pulls them out to standalone bash scripts: - .github/scripts/compose-review-scope.sh (was 36-line inline) - .github/scripts/find-prior-review-comment.sh (was 10-line inline) - .github/scripts/log-review-to-ai-review-log.sh (was 142-line inline) - .github/scripts/parse-claude-mention.sh (was 17-line inline) Each script documents its inputs, outputs, and the rationale for non-obvious mechanics inline. Workflows invoke via `bash .github/scripts/<name>.sh` rather than direct path execution — sidesteps the +x bit being dropped on Windows checkouts and the "forgot to chmod the new script" footgun. The `#!/usr/bin/env bash` shebangs are now informational only. Stacked on top of #444 (marker-based review-comment edit), which introduces the "Find prior review comment" step and the marker'd log-step lookup. Will rebase to main once #444 merges. Tests for these scripts are deliberately out of scope — Nathan's preference per discussion. A separate PR will add coverage via `npm run test:workflows` (or similar) once we settle on the test runner shape. #417 (auth-gate work) carries its own inline scripts and is not touched here. When that branch updates, its scripts adopt the same .github/scripts/ structure as part of that PR's review fixup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
…heck Symptom: harper PR #411 from a real org member was silently skipped — `Claude PR Review` evaluated its job-level `if:` to false and ran zero steps. Cause: GitHub's webhook `author_association` is unreliable. It reports `CONTRIBUTOR` (or `NONE`) for org members with private membership AND for users whose repo access comes via team membership rather than direct collaborator status. Real HarperFast team members fall into both buckets. Forcing visibility changes is hostile UX, and the collaborators API would admit a broader population (read-only collaborators, default-org-permission users) than we want. Fix: two-job pattern with team-membership check. Each workflow has an `authorize` job that runs first, mints an installation token from a HarperFast-org-owned GitHub App (Members:Read scope), and checks team membership. The work/review job has ONE `if: needs.authorize.outputs.authorized == 'true'`. No step-level guards, no individual user list to maintain. The App token lives in the authorize job ONLY — the work job uses the default GITHUB_TOKEN, so the org-read capability never reaches the agent step. CODEOWNERS-driven trust set: - The auth check reads `.github/CODEOWNERS` via the default token and extracts every `@HarperFast/<team>` handle as the trust set. - Same set as people we trust to review code; alignment by construction. New owner team in CODEOWNERS automatically extends trigger trust. New consumer repo inherits its own CODEOWNERS. - External-org handles are deliberately ignored — only HarperFast teams. - If CODEOWNERS is missing, empty, or has no @HarperFast handles, falls back to @HarperFast/developers. Per-workflow specifics: - claude-review.yml: checks BOTH the PR author (`pull_request.user.login`) AND the event actor (`github.actor`). A non-trusted user pushing to a trusted user's PR branch changes the actor without changing the PR author; refusing those events closes that loophole. claude[bot] is admitted explicitly so AI-authored PRs from the issue-to-PR pipeline get reviewed (ADMIT_CLAUDE_BOT=true). - claude-mention.yml: checks the commenter. claude[bot] not admitted here (only humans trigger mentions). - claude-issue-to-pr.yml: checks the LABELER (github.actor), not the issue author. The labeler must already have at least triage permission; a maintainer labeling an external-author issue is a legitimate way to invoke the agent on community reports. Per the post-#447 convention, the auth-check bash lives in `.github/scripts/authorize-claude-workflow.sh` (shared across all three workflows; parameterized by env vars). Workflows invoke via `bash .github/scripts/...`. Defense-in-depth lint: - New `.github/workflows/auth-gate-invariants.yml` runs on any PR touching a `claude-*.yml` workflow file. Validates structurally via `bash .github/scripts/validate-auth-gate-invariants.sh`: * `authorize` job exists. * `authorize.outputs.authorized` wired to a step output. * `actions/create-github-app-token` present and pinned to a SHA. * `authorize.permissions` has no `write` scopes. * `HARPERFAST_AI_CLIENT_ID` and `HARPERFAST_AI_APP_PRIVATE_KEY` secrets referenced. * Every non-authorize job has `needs: authorize` and an exact `if: needs.authorize.outputs.authorized == 'true'` (no compound expressions, no tautologies). Make this a REQUIRED status check on `main` via branch protection. Subtle attacks on the bash logic are caught by CODEOWNERS review on `.github/`. Required (organization-level) secrets — must be set on the HarperFast org for any consumer repo to authorize a Claude run: - HARPERFAST_AI_CLIENT_ID (the App's Client ID, like Iv23li…) - HARPERFAST_AI_APP_PRIVATE_KEY (.pem file contents) Replaces #417's two earlier commits (which were on a stale base that pre-dated #432, #437, #438, #439, #442, #444, #447). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
…r enforces Address review on #417: 1. **Auth check fail-open (high).** `authorize-claude-workflow.sh` would fall through to `authorized=true` when `USERS_TO_CHECK` was empty or whitespace-only. The `read` loop's heredoc on `""` reads zero non-empty lines, every iteration is skipped by `[ -z "$user" ] && continue`, and the trailing `echo "authorized=true"` wins. A workflow that forgot to set USERS_TO_CHECK — or a malicious PR that removed it — would admit every event. Adds a guard right after the trust-set resolution: `${USERS_TO_CHECK//[[:space:]]/}` empty → `::error::`, `authorized=false`, exit 0. 2. **Validator gap (medium).** `validate-auth-gate-invariants.sh` structurally checked the authorize job, secrets, and `needs:`/`if:` shape, but never asserted that USERS_TO_CHECK was wired to a step in the authorize job. With (1) fixed, an omission fails closed at runtime — but the validator should still trip structurally so the omission is caught before merge. Adds new check #6: `yq` over `.jobs.authorize.steps[].env.USERS_TO_CHECK`, fails the workflow if no step sets it. Verified locally: - Empty / whitespace-only USERS_TO_CHECK → script denies. - Non-empty USERS_TO_CHECK → loop runs as before. - All three claude-*.yml workflows already set USERS_TO_CHECK on the right step (validator passes against current PR). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
Symptom: every PR was failing `Auth gate invariants / validate` with `authorize job has no step setting USERS_TO_CHECK env var`, even on workflows that clearly set it. Reported on PR #452 and others. Cause: the check `USERS_TO_CHECK presence` (added in #417 review- fixup) was using a jq-flavored expression — `// empty` in particular — but yq on ubuntu-latest is `mikefarah/yq` (Go), not jq. yq's lexer rejects `empty`. The script had `2>/dev/null` on the yq invocation, so the lexer error was eaten and the variable came back as the empty string, tripping the existence check on every workflow. Fix: rewrite the expression in idiomatic yq: - `.jobs.authorize.steps[].env.USERS_TO_CHECK | select(. != null)` — emits a stream of one value per step that sets the env var, skipping steps that don't. - `head -1` collapses the stream to a single value (or empty) for the `[ -n ]` check. Verified locally with mikefarah/yq v4.53.2 against all three harper claude-*.yml workflows — all pass. Out of scope but worth following up: - The `2>/dev/null` swallowed a real yq error. Worth either removing the suppression or capturing stderr for diagnostics when the expression returns empty. - oauth's mirror (#68) carries the same bug; a copy of this fix needs to land there too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
4 participants
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.
Summary
Two-job auth gate for the AI workflows. Each
claude-*.ymlworkflow now has anauthorizejob that runs first, mints an org-read token from a HarperFast-org-owned GitHub App, and checks team membership via.github/CODEOWNERS-derived trust set. The work/review job has ONEif: needs.authorize.outputs.authorized == 'true'. App token lives in the authorize job only — the work job uses the defaultGITHUB_TOKEN, so the org-read capability never reaches the agent step.Replaces the original draft commits (which were on a stale base predating #432, #437, #438, #439, #442, #444, #447). Cleanly rebased onto current main, with the auth-check bash extracted to
.github/scripts/per the convention #447 established.What lands
.github/scripts/authorize-claude-workflow.shUSERS_TO_CHECKnewline-separated,ADMIT_CLAUDE_BOTflag, CODEOWNERS-derived trust set, optionalclaude[bot]admission..github/scripts/validate-auth-gate-invariants.shauthorizejob exists, app-token pinned to SHA, no write perms on authorize, secrets referenced,needs:+ strictif:on every other job..github/workflows/auth-gate-invariants.ymlclaude-*.ymlor the validator itself. Belongs as a REQUIRED status check onmain..github/workflows/claude-review.ymlauthorizejob;reviewjob gated. Checks PR author + event actor (both must pass).claude[bot]admitted explicitly..github/workflows/claude-mention.ymlauthorizejob;workjob gated. Checks the commenter only..github/workflows/claude-issue-to-pr.ymlauthorizejob;workjob gated. Checks the LABELER (github.actor), not the issue author — labeler must already have triage permission, so a maintainer labeling an external-author issue is a legitimate trigger.Required setup
Org-level secrets must be set (already done per the App configuration this morning):
HARPERFAST_AI_CLIENT_ID— the GitHub App's Client ID (Iv23li…)HARPERFAST_AI_APP_PRIVATE_KEY— the App's.pemfile contentsThe App is installed with
Organization: Members: Readonly.Branch protection followup
After merge, add
Auth gate invariants / validateas a required status check on themainbranch protection rule. Without that, a PR could weaken the auth gate (delete the authorize job, change the if to a tautology, etc.) and still merge.Test plan
Claude PR Review / authorizesucceeds (and the trust set is logged showing the @HarperFast teams found in CODEOWNERS).Claude PR Review / reviewjob runs afterauthorizeadmits (it will not run ifauthorizeoutputsauthorized=false).@claudemention a PR → confirm the mentionauthorizejob admits the commenter and theworkjob runs.claude-review.ymlto weaken the gate (e.g., removeneeds: authorize, change the if to'true') → confirmauth-gate-invariants / validatefails the check.Followup
oauth needs the same auth gate. Will mirror in a separate PR (and oauth gets its own CODEOWNERS read against its own repo).
🤖 Generated with Claude Code