docs: Add ADR 074 with a single-auth - #5649
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoDocs: Add ADR 0074 lite auth mode and clarify ADR 0033
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Site previewPreview: https://cffac541-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. ADR 0074 has two decisions
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ## Decision | ||
|
|
||
| Make **lite auth mode** the primary, default architecture for `fullsend`. The existing per-role GitHub App + mint mode is relegated to an "Enterprise / Org-scoped opt-in" for those who explicitly need the complex scaling/zero-touch automation. In lite mode, | ||
| no GitHub Apps or mint are provisioned; the default `GITHUB_TOKEN` is used | ||
| for triage, code, and review. Stage handoffs use `workflow_dispatch` | ||
| (`gh workflow run`) rather than `repository_dispatch`, because | ||
| `workflow_dispatch` requires only `actions: write` while | ||
| `repository_dispatch` requires `contents: write`. This distinction lets the | ||
| triage and review jobs keep `contents: read`, matching the canonical | ||
| least-privilege role definitions in `internal/mintcore/github.go` (triage: | ||
| `contents: read`, review: `contents: read`). Only the code and fix jobs, | ||
| which genuinely need to push commits, carry `contents: write`. | ||
|
|
||
| This requires follow-up engineering beyond a config flag: the per-repo shim | ||
| workflow (`fullsend.yaml`) must declare `on: workflow_dispatch: inputs:` | ||
| with a `stage`, `issue_number`, and `pr_number` input schema, and forward | ||
| those inputs as `dispatch_stage`, `dispatch_issue_number`, and | ||
| `dispatch_pr_number` `workflow_call` inputs to `reusable-dispatch.yml`, | ||
| which routes on them when `github.event_name == 'workflow_dispatch'`. | ||
| Threading routing hints through explicit `workflow_call` inputs — rather | ||
| than reading `github.event.inputs` directly inside `reusable-dispatch.yml` | ||
| — keeps the reusable workflow's input contract self-documenting and | ||
| matches how it already receives `event_action`. Concurrency-group keys and | ||
| stale-head checks read the same `dispatch_*` inputs for this path, falling | ||
| back to the native event shape for standard-mode triggers. `GITHUB_TOKEN` | ||
| is preferred over the single-self-owned-App alternative above specifically | ||
| to keep this mode's credentials fully ephemeral, consistent with | ||
| fullsend's mint-based model elsewhere, even though it costs more new | ||
| engineering. | ||
|
|
||
| The `gh workflow run` command used by post-scripts targets the shim | ||
| workflow by filename (`fullsend.yaml`) and passes only routing hints — stage | ||
| name, issue number, or PR number — as string inputs. Downstream jobs | ||
| re-fetch PR state (HEAD SHA, labels) from the GitHub API rather than | ||
| trusting the dispatch payload as authoritative, keeping untrusted external | ||
| content (titles, bodies, branch names) out of the trigger path. | ||
|
|
||
| The review stage stops short of an `APPROVE` review — self-approval may be | ||
| restricted for other review states too, not just `APPROVE`, so it posts its | ||
| verdict as a `COMMENT`-type review plus an outcome label. To proceed with the merge, | ||
| a human must approve and merge through the normal GitHub UI, an | ||
| ordinary human-authored event unaffected by any of the above. | ||
|
|
||
| To enable auto-merge, fullsend explicitly requires a "Bring Your Own App" (BYOA) path. While `GITHUB_TOKEN` is the primary auth mechanism for the pipeline, auto-merging PRs strictly requires providing a dedicated Reviewer App to bypass GitHub's self-approval restrictions. Operators can create a dedicated "Reviewer" GitHub App and supply its PEM as a repository secret (e.g., `FULLSEND_REVIEWER_APP_PEM`). When this secret is present, the `review` job uses it to mint an installation token under a distinct identity, bypassing the 422 wall to officially `APPROVE` the PR and allow auto-merge. |
There was a problem hiding this comment.
1. Adr 0074 has two decisions 📜 Skill insight ⚙ Maintainability
docs/ADRs/0074-lite-auth-mode.md records both (1) making lite auth mode the default and (2) requiring a BYOA Reviewer App for auto-merge, which violates the requirement that each ADR record exactly one decision. This makes the ADR harder to supersede/reason about because two independent decisions are coupled in one record.
Agent Prompt
## Issue description
`docs/ADRs/0074-lite-auth-mode.md` contains more than one distinct decision (default auth mode selection and the BYOA auto-merge requirement), violating the “one ADR = one decision” rule.
## Issue Context
The Decision section first decides to make lite auth mode the default, and later separately decides that auto-merge “strictly requires” a BYOA Reviewer App path.
## Fix Focus Areas
- docs/ADRs/0074-lite-auth-mode.md[104-147]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ## Consequences | ||
|
|
||
| - Repos can adopt fullsend with zero secrets beyond enabling default workflow permissions, through the review stage — no GitHub Apps, no mint, no PATs. | ||
| - Auto-merging PRs strictly requires providing a dedicated Reviewer App. The BYOA path provides a supported way to bypass GitHub's self-approval restrictions for operators willing to manage one App PEM. | ||
| - Triage and review jobs stay at `contents: read`, matching the Go role definitions; only code and fix carry `contents: write`. Using `repository_dispatch` would have forced `contents: write` on all jobs that perform handoffs, an unnecessary over-privilege. | ||
| - `reusable-dispatch.yml`'s routing and concurrency-group logic need `workflow_dispatch`-aware follow-up work before lite mode can ship; they are not drop-in reusable as-is. The shim workflow must also declare `on: workflow_dispatch: inputs:` to receive the handoff. | ||
| - The single-self-owned-App alternative avoids that follow-up work entirely by keeping native event handoffs, at the cost of a standing PEM secret instead of a token minted fresh per run — an option operators can reach for if the engineering cost above outweighs that risk in their case. | ||
| - Per-role least-privilege scoping is lost at the identity level — one identity covers triage, code, and review — but GHA `permissions:` blocks still enforce per-job least-privilege at the token scope level, and `GITHUB_TOKEN` never leaves the single enrolled repo. | ||
| - Lite mode becomes the default for all new installs. Org-scoped or high-automation installs must explicitly opt-in to the full App + mint stack, and inference credential provisioning (WIF/GCP) is unaffected either way. | ||
| - The `workflow_dispatch` handoff logic lives in `post-code.sh`/`post-triage.sh`/`post-fix.sh`, which per-repo installs fetch at runtime from `fullsend-ai/agents` rather than from this repo, decoupling lite mode's rollout from a `fullsend-ai/fullsend` release: `reusable-dispatch.yml`'s routing can ship independently, but lite mode has no effect on a real install until the corresponding agent-content changes are also released to `fullsend-ai/agents`. |
There was a problem hiding this comment.
3. Consequences bullets too many/long 📜 Skill insight ⚙ Maintainability
ADR 0074 ## Consequences has more than 5 bullets and several bullets are multi-sentence, violating the required 3–5 one-sentence bullet format. This makes the consequences harder to parse consistently across ADRs.
Agent Prompt
## Issue description
ADR 0074 Consequences must be 3–5 bullet points and each bullet must be a single sentence.
## Issue Context
The current Consequences section has 8 bullets and multiple bullets contain multiple sentences.
## Fix Focus Areas
- docs/ADRs/0074-lite-auth-mode.md[149-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| instead of relying on the label/review event. `workflow_dispatch` requires | ||
| `actions: write`, which triage and review already carry for other reasons, | ||
| so no new permissions are needed. This allows both jobs to keep | ||
| `contents: read`, perfectly aligning GHA `permissions:` blocks with the | ||
| canonical Go least-privilege definitions. Labels and reviews are still |
There was a problem hiding this comment.
5. Overstated permissions alignment 🐞 Bug ⚙ Maintainability
ADR 0074 claims workflow_dispatch lets triage/review keep contents: read, “perfectly aligning” GitHub Actions permissions: blocks with the canonical Go role definitions, but the real workflow permission blocks include additional scopes (e.g., actions: write, id-token: write, pull-requests: write) that are not present in the canonical role-permissions map. This wording can mislead readers into thinking the full permission surface matches the Go-defined least-privilege set, when the alignment is only partial (notably on contents).
Agent Prompt
### Issue description
ADR 0074 states that using `workflow_dispatch` “perfectly align[s] GHA `permissions:` blocks with the canonical Go least-privilege definitions,” but the GHA workflows still require additional permissions beyond the canonical Go role-permissions (e.g., `actions: write`, `id-token: write`, and for review `pull-requests: write`). This is a documentation-accuracy issue: the alignment is primarily about keeping `contents: read`, not about the entire permissions block matching the Go map.
### Issue Context
- Canonical role permissions (Go) define triage/review `contents: read`, but do not include the extra scopes present in the workflows.
- The reusable triage/review workflows declare additional scopes in `permissions:`.
### Fix Focus Areas
- docs/ADRs/0074-lite-auth-mode.md[72-76]
### Suggested change
Reword the sentence to something like:
- “This allows both jobs to keep `contents: read`, aligning the *contents* scope with the canonical Go role definitions (while still requiring other workflow-specific permissions like `actions: write` / OIDC `id-token: write`).”
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
07f5e86 to
b4b6637
Compare
|
One concern from internal chat:
A valid concern. The way GH-AW seems to solve this by default is that things like issue comments automatically get trailing metadata about the workflow identity. To be clear, I think in the general case any agent workflow should have a configurable identity, so if someone wants to e.g. have the coder agent post under some app identity, of course that should work. I just don't think it should be the default. |
|
|
||
| ## Decision | ||
|
|
||
| Introduce **lite auth mode** as an opt-in per-repo authentication mode for `fullsend`. The existing per-role GitHub App + mint mode remains the default for org-scoped installations; lite mode targets per-repo adopters who want to avoid provisioning Apps and a mint. In lite mode, |
There was a problem hiding this comment.
Org-mode is deprecated as of docs/ADRs/0044-deprecate-per-org-installation-mode.md - that should be removed from here and this re-focused on per-repo adopters.
| ([ADR 0007](0007-per-role-github-apps.md)) and either run or point at a token | ||
| mint ([ADR 0029](0029-central-token-mint-secretless-fullsend.md), | ||
| [ADR 0059](0059-public-mint-mode-with-wildcard-allowlists.md), | ||
| [ADR 0068](0068-public-community-mint-architecture.md)). |
There was a problem hiding this comment.
We should expand more here on ADRs 0059 and 0068 and why together they don't satisfy the ergonomic problem.
There was a problem hiding this comment.
That's my primary reaction.
Adding support to use the stock github-actions[bot] sounds like a fine idea.
Making it the default is surprising when ADR 0059 and 0068 taken together should dramatically reduce the friction. Are we missing something there?
There was a problem hiding this comment.
From slack, it just comes down to how privileged code agent is contents:write for a repo with no other restrictions in place is a lot to ask at install time.
There was a problem hiding this comment.
Making it the default is surprising when ADR 0059 and 0068 taken together should dramatically reduce the friction. Are we missing something there?
OK, I've reframed this to make it not the default.
However (per internal chat) I personally went from being a fan of the mint to considering it a risk.
Bigger picture I think if we do implement this mode, it will quickly become a maintenance hazard to have two different ways to do things.
The thing that feels by far the cleanest to me is to make the builtin agents not special (which is how GH-AW handles things) - anyone who wants to attach a non-default identity to a specific workflow can do so.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 7 findings (2 HIGH, 5 MEDIUM) posted as inline comments on docs/architecture.md and docs/ADRs/0074-lite-auth-mode.md.
| `workflow_dispatch` follow-up engineering below is more costly than | ||
| accepting a long-lived PEM in one low-stakes repo's secrets. | ||
|
|
||
| **Bring Your Own App (BYOA) for Review (Author identity: `GITHUB_TOKEN`, Reviewer identity: Dedicated App).** Sidesteps the `422 Can not approve your own pull request` wall by introducing a secondary identity solely for the review step. The operator creates a dedicated "Reviewer" GitHub App, installs it on their repo, and provides its App ID and PEM as repository secrets (`FULLSEND_REVIEWER_APP_ID`, `FULLSEND_REVIEWER_APP_PEM`). The `review` job uses `actions/create-github-app-token` to exchange the PEM for an installation token natively within the workflow — no mint interaction required. This token is used solely for the approval step, bypassing the 422 wall with a distinct identity to allow fully autonomous auto-merge. Chosen as the official path for auto-merge; while `GITHUB_TOKEN` is the primary auth mechanism for the pipeline, auto-merging PRs strictly requires providing a dedicated Reviewer App to bypass GitHub's self-approval restrictions. This approach is effortlessly scalable across hundreds of repositories via GitHub Organization Secrets without needing a central service. While this exposes the App PEM directly to the runner—creating a theoretical lateral movement risk if the runner is compromised—GitHub's native security model inherently mitigates this by making secrets unavailable to untrusted or fork PR workflows by default. Running `mintcore` strictly enforces least-privilege token scoping (protecting even trusted runners), but introduces the operational and supply-chain overhead of maintaining a separate service. For many organizations, native Org Secrets hit the optimal balance of scale and security. |
There was a problem hiding this comment.
[HIGH] BYOA "official path" contradicts the ADR's own standing-credential rejection rationale and ephemeral-credentials principle
The single self-owned App option (lines 85-101) is rejected because "the PEM must live as a static repo secret... reintroducing the standing-credential exposure the central mint (ADR 0029) exists to eliminate," and the Decision (line 131) says GITHUB_TOKEN was preferred "specifically to keep this mode's credentials fully ephemeral." Yet the BYOA option (line 103) — which has the identical standing-PEM-in-secrets exposure, attached to an APPROVE-capable identity — is "Chosen as the official path" that the Decision (line 148) "explicitly requires" for auto-merge. The framing also conflicts with the PR body and architecture.md, which both call BYOA an "escape hatch," and the paragraph's promotional register ("effortlessly scalable", "hit the optimal balance") is out of step with the rest of the doc. The text is additionally triplicated near-verbatim across Options (103), Decision (148), and Consequences (153).
Suggestion: Rewrite BYOA as a documented opt-in escape hatch for operators who accept a standing PEM in exchange for autonomous auto-merge, carrying the same standing-credential caveat the single-App option received. State the mechanism once normatively (in Decision) and reduce the Options and Consequences copies to brief trade-off/reference text. Pick one framing (escape hatch vs. official required path) consistently across the ADR, architecture.md, and the PR body.
| `workflow_dispatch` follow-up engineering below is more costly than | ||
| accepting a long-lived PEM in one low-stakes repo's secrets. | ||
|
|
||
| **Bring Your Own App (BYOA) for Review (Author identity: `GITHUB_TOKEN`, Reviewer identity: Dedicated App).** Sidesteps the `422 Can not approve your own pull request` wall by introducing a secondary identity solely for the review step. The operator creates a dedicated "Reviewer" GitHub App, installs it on their repo, and provides its App ID and PEM as repository secrets (`FULLSEND_REVIEWER_APP_ID`, `FULLSEND_REVIEWER_APP_PEM`). The `review` job uses `actions/create-github-app-token` to exchange the PEM for an installation token natively within the workflow — no mint interaction required. This token is used solely for the approval step, bypassing the 422 wall with a distinct identity to allow fully autonomous auto-merge. Chosen as the official path for auto-merge; while `GITHUB_TOKEN` is the primary auth mechanism for the pipeline, auto-merging PRs strictly requires providing a dedicated Reviewer App to bypass GitHub's self-approval restrictions. This approach is effortlessly scalable across hundreds of repositories via GitHub Organization Secrets without needing a central service. While this exposes the App PEM directly to the runner—creating a theoretical lateral movement risk if the runner is compromised—GitHub's native security model inherently mitigates this by making secrets unavailable to untrusted or fork PR workflows by default. Running `mintcore` strictly enforces least-privilege token scoping (protecting even trusted runners), but introduces the operational and supply-chain overhead of maintaining a separate service. For many organizations, native Org Secrets hit the optimal balance of scale and security. |
There was a problem hiding this comment.
[MEDIUM] "Inherently mitigates" claim addresses the wrong threat; Reviewer-token isolation from the LLM-facing review step is unspecified
The BYOA paragraph (repeated at line 153) dismisses the PEM-on-runner lateral-movement risk because "GitHub's native security model inherently mitigates this by making secrets unavailable to untrusted or fork PR workflows by default." That covers fork-based exfiltration only. The risk the sentence itself names — a compromised runner — is a trusted-workflow runner executing LLM-agent-driven code processing untrusted PR content (diff, title, body, comments): exactly the prompt-injection/credential-exfiltration threat model ADR 0017/0025's credential-isolation architecture exists for, where secrets ARE available. A leaked Reviewer App PEM is a durable, reusable APPROVE-capable identity valid on every repo the App is installed on — materially worse than a per-run mint token. The ADR says the token is "used solely for the approval step" but states no design constraint keeping the PEM/minted token out of the agent's execution context (shared job env, GITHUB_ENV, artifacts/logs).
Suggestion: Replace "inherently mitigates" with an honest residual-risk statement: fork isolation covers untrusted PRs, but a compromised or prompt-injected trusted run can exfiltrate the PEM. Add an explicit design constraint to Decision/Consequences: the Reviewer App token must be minted in a step/job that runs after the agent's verdict is finalized and never executes agent-controlled code or exposes the token to the LLM sandbox; recommend scoping the App to pull_requests: write only, minimal installation footprint, and PEM rotation.
| trusting the dispatch payload as authoritative, keeping untrusted external | ||
| content (titles, bodies, branch names) out of the trigger path. | ||
|
|
||
| The review stage stops short of an `APPROVE` review — self-approval may be |
There was a problem hiding this comment.
[MEDIUM] Premature decision: "self-approval may be restricted for other review states too" is an unverified guess in an Accepted ADR
Lines 142-144 justify stopping at a COMMENT-type verdict with the hedge "self-approval may be restricted for other review states too, not just APPROVE." This is speculative load-bearing rationale about trivially verifiable GitHub API behavior, in a document that elsewhere states API behavior as precise verified fact (the 422 message, dispatch permission scopes). It also contradicts this same PR's other artifacts: the PR body and the ADR 0033 clarification both name only APPROVE as blocked. If REQUEST_CHANGES from the authoring identity is in fact allowed, lite mode is locking its default review flow into a weaker signal (COMMENT + label) than necessary, forfeiting branch-protection "changes requested" blocking without ever noting that trade-off in Consequences.
Suggestion: Verify empirically (e.g. a gh api call submitting REQUEST_CHANGES and COMMENT reviews from the authoring identity on a test PR) and replace the hedge with the confirmed behavior. If REQUEST_CHANGES is allowed, consider using it for failing verdicts to match today's per-role behavior; if it is blocked, state that as verified fact and add the lost merge-blocking capability to Consequences.
| @@ -0,0 +1,159 @@ | |||
| --- | |||
There was a problem hiding this comment.
[MEDIUM] ADR number collision: open PR #5611 also claims ADR 0074
Open draft PR #5611 ("docs(#2821): add ADR 0074 for human-gated permission adjustments") also adds docs/ADRs/0074-human-gated-permission-adjustments.md (verified via the PR's file list). Whichever PR merges second produces two ADRs numbered 0074, breaking the numbering scheme and the "ADR 0074" cross-references this PR adds in ADR 0033 and architecture.md.
Suggestion: Coordinate with #5611: renumber one PR (e.g. take 0075 here, updating filename, frontmatter title, H1, and the cross-references in ADR 0033 and architecture.md), or agree on a landing order with the later PR renumbering.
There was a problem hiding this comment.
Probably worth engineering something less conflict-prone here...like creating draft ADRs without a numeric allocation, and only committing to that allocation on acceptance.
There was a problem hiding this comment.
Sorry about this comment, I have updated my agent memory to not take the fullsend ADR number conflict into review finding, the final gate is the when ADR pr in merge check will be notified.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Two follow-up findings from a deeper trace of the delivered pipeline loops against this design (both verified against the actual scripts and with live tests of GITHUB_TOKEN dispatch/suppression behavior on a scratch repo pair).
Records the single-identity lite auth mode design which simplifies the default UX to avoid having any Github apps at all. We use workflow_dispatch (not repository_dispatch) for stage handoffs so triage and review can keep contents:read per the canonical Go role definitions. Review stops at a COMMENT-type verdict because GitHub rejects a self-authored PR approval regardless of token type; a BYOA (Bring Your Own App) escape hatch is documented for operators who want a fully autonomous auto-merge path anyway. Also annotates ADR 0033 with a clarification: its stated reason for needing two Apps (App tokens are suppressed like GITHUB_TOKEN) was imprecise. App tokens aren't suppressed at all — self-approval was the real constraint, and only for auto-merge specifically. Assisted-by: https://github.com/cgwalters/cgwalters#llms Signed-off-by: Colin Walters <walters@verbum.org>
b4b6637 to
3f38bfc
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 9 findings (4 HIGH, 5 MEDIUM) posted as inline comments on docs/ADRs/0075-lite-auth-mode.md and docs/ADRs/0033-per-repo-installation-mode.md, covering the renumbered/reworked ADR text now on this branch. Checked against all prior inline comments and threads on this PR (including the earlier rounds on the old 0074-lite-auth-mode.md path) — these are new observations on content introduced or left unresolved by the rewrite, not repeats.
| Introduce **Lite Auth Mode** as an opt-in alternative to the default Per-role GitHub App + Mint architecture for per-repo installations. In Lite Auth Mode: | ||
|
|
||
| 1. **Authentication:** The default `GITHUB_TOKEN` is used for triage, code, and review steps instead of a mint-provided token. | ||
| 2. **Handoffs:** Stage handoffs use `workflow_dispatch` (`gh workflow run`) rather than `repository_dispatch` or native events. This requires only `actions: write`, allowing triage and review to maintain least-privilege `contents: read`. `post-review.sh` and `post-retro.sh` will also be updated to use `workflow_dispatch` to close pipeline loops. |
There was a problem hiding this comment.
[HIGH] ADR assigns post-review.sh/post-retro.sh work to a repo where those scripts no longer exist
Decision item 2 states: "post-review.sh and post-retro.sh will also be updated to use workflow_dispatch to close pipeline loops." Verified directly against origin/main: internal/scaffold/fullsend-repo/scripts/ contains no post-review.sh or post-retro.sh (only pre-code.sh, pre-fix.sh, pre-fetch-prior-review.sh, reconcile-repos.sh, validate-source-repo.sh, install-precommit-tools.sh, setup-prioritize.sh). Both scripts were migrated to fullsend-ai/agents per ADR 0058 Phase 4: docs/plans/agent-extraction-to-agents-repo.md's migration table lists scripts/post-review.sh and scripts/post-retro.sh as "Copy as-is", docs/problems/review-autonomy-evidence.md:156 explicitly says "the existing protected-path downgrade in post-review.sh (now in fullsend-ai/agents)", and internal/cli/run.go has a comment stating "...agent scripts now live in that repo, not internal/scaffold/fullsend-repo/... post-review.sh/post-triage.sh/post-retro.sh/post-prioritize.sh still scan for the last iteration-*/output blindly" (referencing fullsend-ai/agents#411). The ADR is silent about this repo split, so an implementer following Decision item 2 literally would look for (or try to edit) files that no longer live in fullsend-ai/fullsend. By contrast, the other follow-up item in Consequences (line 59, reusable-dispatch workflow and shim configuration) correctly remains fullsend-ai/fullsend's responsibility.
Suggestion: Add a note to Decision item 2 (or Consequences) that the post-review.sh/post-retro.sh workflow_dispatch updates are fullsend-ai/agents-side follow-up work, tracked separately per the ADR-0058 Phase 4 repo split.
|
|
||
| A design spike investigated using `secrets.GITHUB_TOKEN` (`github-actions[bot]`). Because GitHub suppresses events triggered by `GITHUB_TOKEN`, standard stage handoffs (`labeled`, `pull_request_review.submitted`) cannot initiate subsequent workflow steps natively. However, `workflow_dispatch` is exempt from event suppression and can be invoked securely to chain stages without over-privileged scopes (maintaining `contents: read` for triage and review, per `internal/mintcore/github.go`). | ||
|
|
||
| Additionally, native auto-merge requires `APPROVE` reviews, which fail with a 422 error if the same identity that authored the PR (e.g., `GITHUB_TOKEN`) tries to approve it. Exchanging a Bring-Your-Own-App (BYOA) PEM for a separate token bypasses this restriction, but exposes the PEM to prompt-injection or runner-compromise threats if not properly isolated. |
There was a problem hiding this comment.
[HIGH] "BYOA" acronym collides with the established "Bring Your Own Agent" term
This ADR defines "BYOA" to mean "Bring-Your-Own-App"/"Bring Your Own App" (a dedicated Reviewer GitHub App) at lines 34, 42, 50, and 51, echoed in docs/architecture.md:185 ("a BYOA escape hatch"). But "BYOA" is already a heavily established acronym in this repo meaning "Bring Your Own Agent." Verified: docs/roadmap.md uses "BYOA" as a bolded, linked heading and literally the team's #1 "Now" priority category ("| 1 | BYOA | Agent catalog, harness triggers, config knobs, shareable config profiles | Now |", plus a full "### BYOA" section describing it as "the team's highest priority for July"). The underlying concept also has a dedicated guide (docs/guides/user/bring-your-own-agent.md) referenced from docs/glossary.md and from ADRs 0058, 0065, and 0036. Reusing the same acronym for an unrelated GitHub-App-credential mechanism will collide with anyone searching docs/glossary or cross-referencing the roadmap's top-priority term.
Suggestion: Rename the new concept to avoid the collision — e.g. "Bring Your Own Reviewer (App)" or "BYO Reviewer App" — in both docs/ADRs/0075-lite-auth-mode.md and the docs/architecture.md bullet.
| 1. **Authentication:** The default `GITHUB_TOKEN` is used for triage, code, and review steps instead of a mint-provided token. | ||
| 2. **Handoffs:** Stage handoffs use `workflow_dispatch` (`gh workflow run`) rather than `repository_dispatch` or native events. This requires only `actions: write`, allowing triage and review to maintain least-privilege `contents: read`. `post-review.sh` and `post-retro.sh` will also be updated to use `workflow_dispatch` to close pipeline loops. | ||
| 3. **Auto-Merge (BYOA):** To bypass GitHub's self-approval 422 restriction, operators can provide a dedicated "Reviewer" App ID and PEM as repository secrets (`FULLSEND_REVIEWER_APP_ID`, `FULLSEND_REVIEWER_APP_PEM`). | ||
| 4. **BYOA Security Architecture:** To mitigate prompt-injection threats, the Reviewer App token **MUST** be minted in an isolated workflow step or job *after* the LLM agent has safely exited, ensuring the PEM is never exposed to the LLM execution context. |
There was a problem hiding this comment.
[HIGH] Reviewer-App-token isolation requirement contradicts itself on step vs. job, and "step" alone doesn't defend the named threat
Decision item 4 requires the Reviewer App token be minted "in an isolated workflow step or job" after the LLM agent exits — but Consequences (line 57) narrows the same claim to only "a post-execution job," eliminating "step" as an option. The ADR contradicts itself on its own central security control. This isn't pedantic: Context (line 34) names "runner-compromise" as one of the two threats being defended against, and GitHub Actions steps within a single job share the same runner filesystem/process environment as every prior step in that job — a later step does not isolate the PEM from a compromised or prompt-injected agent that ran earlier in the same job; only a separate job (fresh runner) does. As written, an implementation can satisfy the letter of the MUST clause ("a step... after the agent has safely exited") while remaining exposed to the exact threat the ADR names, and the Consequences section's stronger claim ("eliminates the threat") is not actually guaranteed by what Decision item 4 requires.
Suggestion: Resolve the contradiction by requiring a separate job specifically (drop "step" as a satisfying option) in Decision item 4, matching what Consequences already claims and reserving any step-level option for steps that themselves run in a freshly provisioned container/runner.
| @@ -0,0 +1,61 @@ | |||
| --- | |||
There was a problem hiding this comment.
[HIGH] ADR number 0075 collides with open PR #5562, which independently also claims 0075
Verified via gh pr view 5562: open PR #5562 ("fix(telemetry)!: respect OTEL variables default behaviour", updated 2026-07-28) adds docs/ADRs/0075-diataxis-documentation-quadrants.md. Both this PR and #5562 claim ADR number 75 with different filename slugs, so git will not flag a conflict at merge time — if both merge, docs/ADRs/ ends up with two files numbered 75. This is exactly the failure mode CONTRIBUTING.md's "ADR numbering" section exists to catch ("Before merging, use the /renumber-adr skill to check whether your ADR number is still available on the target branch and renumber if needed"), backed by skills/renumber-adr/scripts/inflight-adr-numbers.sh. Also verified: open PR #5611 separately claims ADR 0074, and open PR #5685 separately claims ADR 0076 — so 74/75/76 are simultaneously contested by three different open PRs right now, meaning even a naive renumber isn't safe without re-checking at actual merge time.
Suggestion: Run the /renumber-adr skill (or skills/renumber-adr/scripts/inflight-adr-numbers.sh) before merge and take the next free number, re-verifying at merge time since other ADR PRs may land in the meantime.
|
|
||
| 1. **Authentication:** The default `GITHUB_TOKEN` is used for triage, code, and review steps instead of a mint-provided token. | ||
| 2. **Handoffs:** Stage handoffs use `workflow_dispatch` (`gh workflow run`) rather than `repository_dispatch` or native events. This requires only `actions: write`, allowing triage and review to maintain least-privilege `contents: read`. `post-review.sh` and `post-retro.sh` will also be updated to use `workflow_dispatch` to close pipeline loops. | ||
| 3. **Auto-Merge (BYOA):** To bypass GitHub's self-approval 422 restriction, operators can provide a dedicated "Reviewer" App ID and PEM as repository secrets (`FULLSEND_REVIEWER_APP_ID`, `FULLSEND_REVIEWER_APP_PEM`). |
There was a problem hiding this comment.
[MEDIUM] BYOA Reviewer App's required permission scope for satisfying branch-protection approvals is unspecified
Decision item 3 has operators provision a dedicated "Reviewer" App ID/PEM but never states what GitHub permission level that App needs (e.g. contents:write). This matters: open PR #2791 ("docs: ADR 0071 — Auto-merge") states as a verified key finding, "GitHub only counts approvals toward branch protection when the reviewer has contents: write — our review bot has contents: read, so its approvals are currently informational only." Verified internal/mintcore/github.go's canonicalRolePermissions["review"] is indeed {"contents": "read", "pull_requests": "write", ...}. If an operator provisions the Reviewer App with that same low-privilege convention (the natural default, since it stands in for the review role), its APPROVE review may not count toward required-approvals at all, silently defeating the auto-merge escape hatch this ADR introduces. Separately, ADR 0075 and the parallel, still-open ADR 0071 proposal (PR #2791) are two independent, non-cross-referenced designs solving the identical self-approval/auto-merge restriction via a separate App identity.
Suggestion: Specify the required permission scope for the Reviewer App (explicitly call out contents:write if that's what's needed for the approval to count toward branch protection), and cross-reference PR #2791/ADR 0071 so the two auto-merge designs are reconciled before either is implemented.
|
|
||
| **Rejected**: GitHub suppresses events triggered by pushes made with any `GITHUB_TOKEN` or GitHub App installation token, to prevent infinite loops. Two separate Apps work because a push made with App-A's token _does_ generate events that trigger workflows authenticated as App-B. The fix→review loop requires the coder/fix agent to push commits that trigger review — if both roles share one App, the push token matches the workflow's App and the event is silently suppressed, breaking the feedback cycle. At minimum, coder and review must be separate Apps. | ||
|
|
||
| _Clarification (2026-07):_ GitHub's event-suppression is specific to the |
There was a problem hiding this comment.
[MEDIUM] ADR 0033 annotation substantially re-argues the superseded rationale instead of a short clarifying note
The block added at lines 69-81 doesn't just cross-reference ADR 0075 — it re-derives the technical reasoning and explicitly states the original "Rejected" verdict for Alternative 2 rested on an incorrect premise ("the fix→review loop this alternative worried about is not actually broken by sharing one App", "not because sharing one App is technically broken"). This appears technically accurate (GitHub's documented event-suppression is specific to GITHUB_TOKEN and does exempt App tokens/PATs), but it goes well beyond a "short note" or "clarifying remark." It's inconsistent with this repo's own ADR-immutability convention: ADR 0029 states "Once this ADR is Accepted, its content is frozen. Do not edit the Context, Decision, or Consequences sections... Only status changes and links to superseding ADRs should be added," and this same PR's new ADR 0075 codifies an equivalent norm in its own template comment ("Minor annotations are welcome... However, do not substantially rewrite the Context, Decision, or Consequences sections"). ADR 0033 itself carries no such comment, so nothing is technically violated, but the edit sets a precedent inconsistent with the standard this very PR introduces elsewhere.
Suggestion: Trim the ADR 0033 annotation to a short pointer (e.g. "Clarification (2026-07): the GITHUB_TOKEN-suppression reasoning above was imprecise — see ADR 0075 for the corrected analysis") and keep the full technical re-argument only in ADR 0075's own Context, which already restates it.
|
|
||
| 1. **Authentication:** The default `GITHUB_TOKEN` is used for triage, code, and review steps instead of a mint-provided token. | ||
| 2. **Handoffs:** Stage handoffs use `workflow_dispatch` (`gh workflow run`) rather than `repository_dispatch` or native events. This requires only `actions: write`, allowing triage and review to maintain least-privilege `contents: read`. `post-review.sh` and `post-retro.sh` will also be updated to use `workflow_dispatch` to close pipeline loops. | ||
| 3. **Auto-Merge (BYOA):** To bypass GitHub's self-approval 422 restriction, operators can provide a dedicated "Reviewer" App ID and PEM as repository secrets (`FULLSEND_REVIEWER_APP_ID`, `FULLSEND_REVIEWER_APP_PEM`). |
There was a problem hiding this comment.
[MEDIUM] FULLSEND_REVIEWER_APP_ID described as a secret, contradicting the established App-ID-as-variable convention
Decision item 3 has operators provide "a dedicated 'Reviewer' App ID and PEM as repository secrets (FULLSEND_REVIEWER_APP_ID, FULLSEND_REVIEWER_APP_PEM)." Verified this contradicts the existing, implemented convention: ADR 0007 states "Private keys (PEMs) are stored as repo-level secrets... Client IDs are stored as repo-level variables," and ADR 0014 codifies exactly this as repository secrets FULLSEND_<ROLE>_APP_PRIVATE_KEY plus variables FULLSEND_<ROLE>_CLIENT_ID — a pattern actually implemented in internal/layers/secrets.go (with matching unit tests) and exercised today by .github/workflows/reusable-review.yml, which reads vars.FULLSEND_REVIEW_CLIENT_ID. A GitHub App ID isn't sensitive and doesn't need secrets-manager handling — only the private key does. The naming also diverges from the established vocabulary (_CLIENT_ID/_APP_PRIVATE_KEY, not _APP_ID/_APP_PEM) and from the canonical role spelling used elsewhere ("review"/"REVIEW", e.g. FULLSEND_REVIEW_CLIENT_ID) — this ADR instead uses "REVIEWER".
Suggestion: Store the App ID as a repository variable (e.g. vars.FULLSEND_REVIEWER_CLIENT_ID) rather than a secret, and align naming with the FULLSEND_<ROLE>_APP_PRIVATE_KEY / FULLSEND_<ROLE>_CLIENT_ID pattern (and the existing "REVIEW" role spelling) unless there's a stated reason this new credential surface should diverge.
|
|
||
| Introduce **Lite Auth Mode** as an opt-in alternative to the default Per-role GitHub App + Mint architecture for per-repo installations. In Lite Auth Mode: | ||
|
|
||
| 1. **Authentication:** The default `GITHUB_TOKEN` is used for triage, code, and review steps instead of a mint-provided token. |
There was a problem hiding this comment.
[MEDIUM] Stage-coverage list omits fix/retro, inconsistent with the rest of the ADR
Decision item 1 says the default GITHUB_TOKEN "is used for triage, code, and review steps" — omitting fix and retro. But Decision item 2 (line 49) calls for updating post-retro.sh, implying retro is in-pipeline under Lite Auth Mode, and Consequences (line 58) states "only code and fix carry contents: write," implying fix is in-pipeline too. It's never stated whether fix and retro also run under the default GITHUB_TOKEN — which is needed for the "zero GitHub-credential secrets" claim in Consequences (line 55) to actually hold across the full pipeline — or authenticate some other way.
Suggestion: Enumerate all pipeline stages Lite Auth Mode covers (triage, code, review, fix, retro) consistently everywhere a stage list appears in this ADR, or explicitly scope fix/retro out and explain their authentication path.
|
|
||
| Per-repo installation ([ADR 0033](0033-per-repo-installation-mode.md)) historically required provisioning per-role GitHub Apps ([ADR 0007](0007-per-role-github-apps.md)) and a token mint ([ADR 0029](0029-central-token-mint-secretless-fullsend.md)), creating nontrivial setup friction. While the Per-role App + Mint architecture remains the default for scaled, highly isolated environments, some per-repo adopters need a zero-ceremony opt-in path using the default `secrets.GITHUB_TOKEN`. | ||
|
|
||
| A design spike investigated using `secrets.GITHUB_TOKEN` (`github-actions[bot]`). Because GitHub suppresses events triggered by `GITHUB_TOKEN`, standard stage handoffs (`labeled`, `pull_request_review.submitted`) cannot initiate subsequent workflow steps natively. However, `workflow_dispatch` is exempt from event suppression and can be invoked securely to chain stages without over-privileged scopes (maintaining `contents: read` for triage and review, per `internal/mintcore/github.go`). |
There was a problem hiding this comment.
[MEDIUM] internal/mintcore/github.go citation implies mint enforcement that Lite Auth Mode explicitly bypasses
Context justifies keeping contents:read for triage/review "per internal/mintcore/github.go" — but Decision item 1 states Lite Auth Mode uses the default GITHUB_TOKEN "instead of a mint-provided token," i.e. the mint (and its Go role-permission map) is bypassed entirely in this mode. Verified internal/mintcore/github.go's canonicalRolePermissions: triage and review do show contents:read there (so that part happens to coincidentally hold), but that map governs mint-issued GitHub App installation tokens (the default, non-Lite mode) and has no "actions" key at all for either role. The actual enforcement point for Lite Auth Mode's GITHUB_TOKEN is verified to be the workflow-level permissions: blocks already present in internal/scaffold/fullsend-repo/.github/workflows/triage.yml and review.yml (contents: read, actions: write, id-token: write, issues: write, plus pull-requests: write for review) — a mechanism unrelated to, and not covered by, mintcore's canonicalRolePermissions map cited here.
Suggestion: Cite internal/scaffold/fullsend-repo/.github/workflows/{triage,review}.yml's permissions: blocks as the actual source for the GITHUB_TOKEN scopes, and reword the internal/mintcore/github.go reference to signal design parity ("matching the same read/write split as...") rather than a shared enforcement path, since Lite Auth Mode never calls the mint.
- Correct the GITHUB_TOKEN-retriggering argument: workflow_dispatch and repository_dispatch are exempt from the suppression regardless of token identity, so an explicit `gh workflow run` handoff avoids the problem without a separate bot identity (per cgwalters' review and the pending fullsend PR #5649 / Lite Auth Mode investigation). Real identity separation still matters for least-privilege and for GitHub's self-approval 422 restriction, not for retriggering itself. - Sharpen "pwn request" attribution: GitHub Security Lab's term covers pull_request_target privilege escalation specifically; the trigger- authorization gap for issue_comment/issues.opened is a related but distinct concern. - Verified two other review findings against gh-aw's live docs and release history: `request_review` is genuinely gh-aw's documented default supply-chain policy, and v0.83.x is the current release line (v0.83.4, 2026-07-27) — both flagged claims were already correct, no change needed. - Update docs/landscape.md's two stale gh-aw references (preview status, supply-chain default policy) to match. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the |
3 participants
Records the single-identity lite auth mode design which simplifies the default UX to avoid having any Github apps at all.
We use workflow_dispatch (not repository_dispatch) for stage handoffs so triage and review can keep contents:read per the canonical Go role definitions. Review stops at a COMMENT-type verdict because GitHub rejects a self-authored PR approval regardless of token type; a BYOA (Bring Your Own App) escape hatch is documented for operators who want a fully autonomous auto-merge path anyway.
Also annotates ADR 0033 with a clarification: its stated reason for needing two Apps (App tokens are suppressed like GITHUB_TOKEN) was imprecise. App tokens aren't suppressed at all — self-approval was the real constraint, and only for auto-merge specifically.
Assisted-by: https://github.com/cgwalters/cgwalters#llms