docs(adr): propose ADR 0044 gate workflows:write on repo secret allowlist

[ifireball]

Summary

Proposes ADR 0044 (Proposed): a minimal-privilege path to grant the coder/fix GitHub App workflows: write only when target-repo workflow exposure is acceptable — without implementing the full repo-readiness checklist from #788.

Background / why this ADR exists

Link	Role in this design
#470	Coder agent cannot push .github/workflows/ changes today; GitHub requires workflows permission (see failed push in that issue).
#788	Granting workflows: write without repo hardening enables prompt-injection via new on: push workflows on agent branches; issue argues for a gate before elevation. PR #787 was closed pending this work.
ADR 0041	Implementation blocked until ADR 41 lands — assess + conditional mint in reusable-code.yml / reusable-fix.yml needs stable dispatch.

Design discussion refined the #788 “full assessment” into a narrower, deterministic gate:

• contents: write alone does not allow authoring workflow files; workflows: write is the distinct escalation.
• Assessment (code workflow only) inventories secret names visible to repo-context workflows on source_repo: repository secrets, org secrets visible to the repo, environment secrets (with fail-closed deployment-branch rules), plus default GITHUB_TOKEN workflow permissions.
• Minimal privilege: only assess when triage (or human) sets fullsend-needs-workflow-write on the issue; builtins (FULLSEND_GCP_*, FULLSEND_DISPATCH_TOKEN) are hard-allowlisted; admins may extend via explicit workflow_secret_allowlist in config.yaml (no FULLSEND_* prefix wildcard).
• Fix agent does not assess (PR job context ≠ push-time workflow secret visibility); it elevates only when PR has fullsend-workflow-write from coder (post-assess) or an authorized human (OWNER | MEMBER | COLLABORATOR).
• Policy stays in workflows, not in mint.

Test plan

• pre-commit ADR linters pass on the new file
• Reviewers agree the Proposed ADR captures intent before implementation (post–ADR 41)

Made with Cursor

[github-actions]

Site preview

Preview: https://65ad1ce2-site.fullsend-ai.workers.dev

Commit: 834cb247c90b0cf1ea64a5d559e2929a9b3d6344

[fullsend-ai-review]

Review

Findings

Medium

• [correctness] docs/ADRs/0044-gate-workflows-write-on-repo-secret-allowlist.md:2,22 — ADR status is set to Accepted in both the YAML frontmatter (line 2: status: Accepted) and the body (line 22: Accepted), but the PR description explicitly calls this a "Proposed ADR" and the test plan says "Reviewers agree the Proposed ADR captures intent before implementation." Per the ADR template, new ADRs enter as Proposed and move to Accepted when the decision is approved. Merging with Accepted status would freeze the content immediately (per the template comment: "Once this ADR is Accepted, its content is frozen"), which conflicts with the stated goal of gathering reviewer agreement first.
  Remediation: Change status: Accepted to status: Proposed in both the frontmatter and the ## Status section. Update to Accepted in a follow-up PR once reviewers have agreed on the design.

• [documentation-currency] docs/architecture.md:118 — The architecture.md living document states: "When an ADR is accepted (or superseded), this document is updated to match." If the ADR should be Proposed (not yet accepted), adding it to the "Decided" section is premature. This entry should be deferred until the ADR reaches Accepted status, or the ADR status should be corrected to Accepted intentionally (in which case the first finding is moot).
  Remediation: Either (a) remove the architecture.md bullet from this PR and add it when the ADR is accepted, or (b) if the intent is to merge as Accepted, keep the bullet and update the ADR status consistently.

Low

• [style] docs/architecture.md:118 — The new bullet is placed under Agent Identity Provider > Decided, but the ADR primarily concerns workflow permission gating, secret allowlist assessment, and label-based policy enforcement in the code/fix workflows. The Agent Infrastructure > Decided section (where ADR 0041 and other workflow-related decisions are listed) appears to be a more natural fit. The mint connection provides some justification for the current placement, but readers looking for workflow permission decisions would check Infrastructure first.
  Remediation: Consider moving the bullet to the Agent Infrastructure > Decided section.

[ralphbean]

I think this needs one change before we can merge. See inline.

[ralphbean]

[critical] I think we should be explicit here that check (4) is defense-in-depth, not a reliable gate. GitHub Actions lets individual workflow YAML files set their own permissions: block, escalating beyond the repo default. A rogue workflow pushed via workflows: write can include permissions: contents: write regardless of the repo-level default — the default only controls what workflows get when they omit the permissions: key entirely.

So this check catches repos where the defaults are already wide open (a signal that the org isn't thinking about least privilege), but it can't actually prevent a rogue workflow from obtaining a write-scoped GITHUB_TOKEN. The assessment as written treats it as a hard gate alongside (1)–(3), which I think overstates what it provides.

[ifireball]

@ralphbean

hmm, you are right, I was focused on secret exfiltration, but writing into main is also possible via a rogue workflow.

So perhaps I should refocus on trying to ensure the pushed bot commit does not create or modify any workflows that would trigger as a result of the merge itself, or perhaps add that in addition to the mechanism I have already described.

WDYT?

[ralphbean]

[moderate, non-blocking] The authorization floor for the PR label override is OWNER | MEMBER | COLLABORATOR — same as /fs-fix. In large orgs, COLLABORATOR can be pretty broad (external contributors with push access). Since this override bypasses assessment entirely, do we want a tighter floor here (OWNER | MEMBER only)? Or does matching the /fs-fix authorization rule feel right for consistency?

[rh-hemartin]

This only prevent our agents from submitting a PR that wants to extract secrets, but what people is doing without agents? They shoyld have some prevention mechanism so GH does not run workflows without permission, right? Can't we just use that and do not implement this?

[ifireball]

This only prevent our agents from submitting a PR that wants to extract secrets, but what people is doing without agents? They shoyld have some prevention mechanism so GH does not run workflows without permission, right? Can't we just use that and do not implement this?

This is not about running workflows, its about writing workflow files.

Unauthorized humans typically create branches in their forks and PRs out of those - so as long as the PR isn't merged, a contributor cannot get his workflow to run in the context of the target repo.

Bots, however, create PRs by pushing branches directly into the target repo, such a branch can contain a new workflow that would trigger on push. To mitigate that, GitHub blocks commits pushed with workflow file modifications unless the pusher has a token with a "workflow: write" permission.

This whole PR and set of issues is about finding a way to grant "workflow: write" to the code agent so that it can edit workflows without causing it to become an attack platform into the repo.

[rh-hemartin]

Ok, so the current protection is forcing contributors to open a fork.

[ifireball]

Ok, so the current protection is forcing contributors to open a fork.

Yes, if you don't trust them. don't let them push (write) into the repo...

[waynesun09]

Review squad (5 agents: 2x claude-coder, claude-researcher, gemini-code-review, cursor-code-review) — 14 findings total after dedup, 4 false positives removed.

Filtered out findings already raised by ralphbean (check 4 GITHUB_TOKEN, COLLABORATOR floor) and fullsend-ai-review[bot] (status field, architecture.md section placement). 5 unique MEDIUM+ findings posted inline.

Overall: Core design is sound. The main gap is that several mechanism details (label provenance verification, environment fail-closed criteria, allowlist matching semantics) need to be specified precisely enough for implementers to build consistent behavior. The workflow_secret_allowlist config field should be framed as a new introduction, not a reference to existing schema.

[waynesun09]

[high] workflow_secret_allowlist is referenced as if it already exists in ADR 0011 and ADR 0033, but neither ADR mentions this field and internal/config/config.go has no corresponding struct field (RepoConfig contains only Roles and Enabled; PerRepoConfig has only Version, KillSwitch, and Roles). This ADR is introducing a new config field, not referencing an existing one — the text should say so explicitly, and note that implementation requires schema changes to the Go structs and both referenced ADRs.

Additionally, "No prefix wildcards" is ambiguous — it could be read as "no wildcards at the prefix position" (allowing suffix wildcards like *_API_KEY). If the intent is exact-match-only, say "Exact secret name match only; no wildcards. Matching is case-insensitive (consistent with GitHub)."

[waynesun09]

[medium] The threat model scopes to "runs on push to an agent branch" but on: push is not the only trigger that fires without review. A rogue workflow could use on: workflow_run (fires when another workflow completes), on: create (fires on branch creation), or other non-PR-gated triggers to execute on agent branches without merge or review.

Consider broadening to: "if the coder agent adds a workflow on source_repo that runs on an agent branch via any non-PR-gated trigger (on: push, on: create, on: workflow_run, etc.), what credentials could that workflow obtain?"

[waynesun09]

[medium] "fail closed when policies are permissive" is underspecified — what counts as "permissive" for agent branches?

GitHub environment deployment-branch policies can be: (a) no restriction / "All branches", (b) "Protected branches only", (c) specific branch name patterns. The ADR should define which configurations are permissive in the context of agent branches. For example: fail closed if policy is "All branches" or includes patterns matching agent branch naming (fullsend/*, *, etc.). Environments restricted to specific protected branches where agent branches cannot match could be excluded.

Also: should environment protection rules (required reviewers, wait timers) factor in, or are they out of scope since a rogue workflow could bypass them with the right token?

[waynesun09]

[medium] Label provenance verification via "issue labeled timeline" is underspecified. The ADR should clarify:

1. Which event is authoritative? If a label is added by an unauthorized user, removed, then re-added by an authorized user (or vice versa), does the check use the most recent labeled event or any matching event?
2. Timing: Is the actor's association (OWNER/MEMBER/COLLABORATOR) checked at the time the label was applied, or at workflow run time? Associations can change between the two.
3. Label removal semantics: If fullsend-needs-workflow-write is removed from the issue after assessment passes but before fix runs, does that invalidate the PR label grant? Similarly, does removing fullsend-workflow-write from the PR revoke elevation for subsequent fix runs?

Recommend specifying: "The most recent labeled timeline event for the label name must have a trusted actor. Label removal from the PR revokes elevation for subsequent runs."

[waynesun09]

[medium] The consequences section covers the happy path well but omits key failure modes that implementers will need to handle:

1. API errors during assessment — should fail closed (no elevation), but this isn't stated.
2. Assessment staleness — secrets or environments can change between the code assessment and subsequent fix runs. The PR label represents a point-in-time grant, not a continuous assessment. Worth noting explicitly.
3. UX on assessment failure — what feedback does the coder give when assessment fails? Does it produce a PR without workflow changes? Output which secrets blocked elevation? Both?
4. Triage guidance — triage agent guidance and label_actions config need updating to recognize when issues require workflow edits; failure to label means the coder can't push workflow changes, leading to a failed run that must be retried after manual labeling.

[ifireball]

Following @ralphbean comment, it seems the direction I was going for here has some major security flaws in it and it does not sufficiently address the code injection risk.

I am moving the PR to draft for now to reconsider the approach.

[ifireball]

As mentioned before, the approach here was troublesome. PR #2548 was yet another attempt to fix the underlying issue (Now closed as well, but with a suggested plan at the closing comment)

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 9:59 AM UTC · Completed 10:05 AM UTC
Commit: 834cb24 · View workflow run →

[fullsend-ai-retro]

Retro: PR #1739 — ADR 0044 (gate workflows:write on repo secret allowlist)

Outcome: Closed without merge after human reviewers identified fundamental security flaws the review agent missed.

Timeline

1. 2026-06-01 — ifireball opens PR #1739 proposing ADR 0044, a security gate for granting workflows:write to the coder/fix agent.
2. 2026-06-01 (~9 min later) — Review agent posts 3 findings: status mismatch (Accepted vs Proposed), premature architecture.md entry, and a style nit. All process-level, no substantive design critique.
3. 2026-06-02 — ralphbean identifies a critical security flaw: GitHub Actions workflow YAML can set its own permissions: block, escalating beyond repo defaults. This makes the ADR's check (4) unreliable as a hard gate — the core assumption of the design was wrong.
4. 2026-06-05 — waynesun09 (using a 5-agent review squad) finds 5 additional MEDIUM+ gaps: missing threat vectors (on: workflow_run, on: create), underspecified label provenance, non-existent config field references, and missing failure mode analysis.
5. 2026-06-08 — Author acknowledges flaws, moves PR to draft.
6. 2026-06-30 — PR closed. Successor PR feat(auth): label-gated workflow-change elevation #2548 also closed, with a revised plan in its closing comment.

Key finding

The review agent's findings were valid but exclusively process-level (formatting, status, placement). The critical gap — that the ADR's security model was fundamentally flawed — was caught only by human reviewers with deep platform security knowledge. This is the most important class of review failure: catching style while missing substance on a security-critical design document.

Existing coverage

Several open issues already address the improvements this retro would propose:

• #1086 — Expand security sub-agent to cover threat modeling, OWASP Top 10, and adversarial thinking. Directly addresses the gap: the security sub-agent should have challenged the ADR's threat model completeness.
• #1906 — Detect omissions in ADRs, specs, and design documents by exploring project context. Covers the failure to notice the ADR's incomplete analysis of GitHub Actions permission escalation.
• #1659 — Review agent should comment-only (not approve) on ADR PRs. Already addresses the autonomy boundary for design documents.

Tension worth noting: #2678 proposes deprioritizing findings on planning/design documents. This retro demonstrates the opposite need — design documents can contain critical security flaws that warrant deep review. The resolution likely depends on #2678's scope (deprioritizing style findings is fine; deprioritizing substantive design critique is not).

No new proposals

The improvement opportunities identified here are already covered by open issues #1086, #1906, and #1659. Filing new issues would create duplicates. Prioritizing #1086 (adversarial security reasoning) would have the highest impact on preventing this class of review miss.