Repository navigation
docs: ADR 0031 — reusable workflows for action-installed distribution - #688
Conversation
Review: #688Head SHA: 97a9972 SummaryThis PR adds ADR 0031 proposing reusable workflows for distributing agent infrastructure. The ADR is well-structured, thoroughly considers trade-offs (trust boundary shift, upstream availability, cross-repo debugging), and correctly describes GitHub Actions FindingsMedium
Low
Info
FooterOutcome: comment-only Previous runReview: #688Head SHA: 5584a53 SummaryThis ADR is well-structured and thorough. It correctly documents the dispatch chain, FindingsMedium
Info
FooterOutcome: comment-only Previous run (2)Review: #688Head SHA: 92164bc SummaryWell-structured ADR that proposes a sound distribution model change from scaffold copies to reusable FindingsMedium / Low / Info
FooterOutcome: approve Previous run (3)Review: #688Head SHA: 9bd52b4 SummaryThis is a well-written ADR that clearly articulates the problem (workflow drift from scaffold copies), presents three options with trade-offs, and makes a reasoned decision. The consequences section is thorough — particularly the trust boundary analysis around FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (4)Review: #688Head SHA: 18d3feb SummaryThis ADR is well-structured, follows the established template, and provides a thorough analysis of the trade-offs between scaffold copies, composite actions, and reusable workflows. The security analysis around FindingsCriticalNone. HighNone. MediumNone. Low
Info
FooterOutcome: comment-only Previous run (5)Review: #688Head SHA: 85e4054 SummaryThis ADR is well-written and covers the reusable-workflow distribution model with appropriate depth — the options analysis is clear, the trust boundary discussion is thorough, and cross-references to ADR 0026 and 0028 are valid. However, ADR number 0030 conflicts with PR #715 ("ADR 30 — OpenShell sandbox interaction model"), which is also open and claims the same number. One of the two PRs must be renumbered before merge to avoid a collision. Additionally, the branch name FindingsHigh
Info
FooterOutcome: request-changes Previous run (6)Review: #688Head SHA: 306e2e9 SummaryThis ADR is well-structured, technically accurate, and follows the established template. The FindingsMediumNone. Low
Info
FooterOutcome: comment-only |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Review: ADR 0030 — Reusable workflows for action-installed distribution
The direction is sound — scaffold copy-and-drift is a real problem and reusable workflows are a well-understood pattern to address it. The ADR is well-structured and the workflow_call semantics are correct. Two items need addressing before acceptance, plus several observations for consideration.
Requesting changes on:
- Multi-forge implications — this design is GitHub Actions-specific (
workflow_call,secrets: inherit). PR #601 is adding ADR-0028 for GitLab CI support architecture. Consider adding a consequence noting that this distribution model is GitHub-specific, and that multi-forge support (see #601) will need its own equivalent mechanism. This doesn't need to solve the GitLab problem, just acknowledge the boundary. secrets: inherittrust boundary —secrets: inheritpasses all caller secrets to reusable workflow code in a public repo. Worth adding a consequence comparing trust boundaries:- Current model (scaffold copies): org secrets are consumed by workflow code in the org's own repo. Full visibility and control.
- Proposed model (reusable workflows): org secrets flow to workflow code in an upstream public repo. A compromised upstream tag/SHA could exfiltrate secrets from all downstream orgs simultaneously.
- Mitigation: SHA pinning (not just tag pinning) gives orgs control. Tag pinning (
@v1) is convenient but mutable.
Given the project's threat model prioritizes external injection, this shift in trust boundary should be explicitly documented.
Additional observations (note, defer):
- File count (line 28): "~30 files" — the scaffold directory actually contains ~60 files. Consider correcting or qualifying.
- Line count range (line 30): "100–160 line" —
fix.ymlis 355 lines,dispatch.ymlis 188 lines. Worth correcting for accuracy. - Upstream availability (Consequences): Missing consequence — upstream availability becomes a new failure mode. If
fullsend-ai/fullsendhas an outage or a ref is corrupted, all downstream orgs break simultaneously. - Debugging/observability (Consequences): Missing consequence — when workflows fail, the call stack spans two repos. Developers need to look at both the thin caller and the upstream reusable workflow.
There was a problem hiding this comment.
Review of PR #688 - ADR 0030: Reusable Workflows
This ADR addresses a real maintenance problem (workflow drift), but has several critical gaps that need resolution before acceptance.
1. Breaking Changes and Version Pinning Strategy
Problem: The ADR states "Organizations control upgrade timing by pinning to specific tags or SHAs" but doesn't specify what fullsend admin install generates by default. This creates three possible outcomes:
Scenario A: Moving references (e.g., @v1, @main)
uses: fullsend-ai/fullsend/.github/workflows/reusable-triage.yml@v1- ✅ Infrastructure patches propagate automatically
- ❌ Breaking changes also propagate immediately - no testing window
- ❌ One bad merge to
mainbreaks all installations instantly - ❌ Defeats the purpose of controlled upgrades
Scenario B: Pinned references (e.g., @abc123)
uses: fullsend-ai/fullsend/.github/workflows/reusable-triage.yml@abc123def- ✅ Safe - only change when explicitly updated
- ❌ Defeats the entire purpose - orgs still need to re-run
fullsend admin installto get new pins - ❌ Brings workflow drift problem back - just moved from infrastructure to version pins
Scenario C: Hybrid approach (not discussed)
- Some orgs use
@v1for auto-updates, others use pinned SHAs? - What's the recommended approach?
- How do orgs test upstream changes before taking them?
Missing from ADR:
- What does
fullsend admin installgenerate by default? - What's the recommended upgrade workflow?
- How do orgs test upstream changes before production?
- What's the rollback procedure if upstream breaks?
This is a fundamental design decision that must be specified before the ADR can be properly evaluated.
2. Multi-Forge Architectural Divergence
This ADR creates architectural divergence with GitLab support (ADR-0028).
Current state (consistent across forges)
- GitHub:
fullsend admin installcopies.github/workflows/*.yml - GitLab:
fullsend admin installcopies.gitlab/ci/*.yml - Both: Full pipeline files with all infrastructure logic
After ADR 0030 (divergent)
- GitHub: Thin ~20-line callers using
workflow_call - GitLab: Still full pipeline files (?)
Why this matters
GitHub Actions workflow_call has no direct GitLab equivalent:
- GitLab has
include:for remote YAML, but nosecrets: inheritequivalent - GitLab has CI/CD Components (16+), but different syntax/capabilities
- Child pipelines exist but serve a different architectural role
ADR-0028 assumes full file copying:
Phase 2: CI/CD templates
- Create
.gitlab/ci/dispatch.ymldispatcher- Create stage pipelines (triage.yml, code.yml, review.yml, fix.yml)
- Create
templates/shim-pipeline.ymltemplate
Consequences of divergence:
- Instead of maintaining two parallel template sets, now maintaining:
- GitHub: upstream reusable workflows + thin local callers
- GitLab: full pipeline files (100-350 lines each)
- This is worse than the current state for maintenance burden
- Violates CLAUDE.md principle: "Parallel architecture: GitLab implementation closely mirrors GitHub"
Recommendation
The ADR should include a section addressing multi-forge implications with one of these paths:
Option A: Keep both forges on full-file copying (reject this ADR)
Option B: Design a forge-neutral reusable component strategy that works for both
Option C: Accept divergence and document the maintenance trade-offs explicitly
Option D: Defer this ADR until GitLab support lands, then design unified approach
Whichever path is chosen, the Consequences section must document the multi-forge implications.
3. Trust Boundary Shift (already flagged by reviewers)
Echoing Ralph Bean's review: secrets: inherit fundamentally changes the trust model from "secrets stay in org repo" to "secrets flow to public upstream code."
Current state:
- Secrets live in org's
.fullsendrepo - All code executing with secrets is in org-controlled repository
After ADR 0030:
- All infrastructure code runs from
fullsend-ai/fullsend(public repo) - Org secrets are accessible to this code via
secrets: inherit
Mitigation:
- SHA pinning ensures orgs know exactly what code runs with their secrets
- But this only works if orgs actually pin (see issue #1 above)
Recommendation:
Add explicit documentation in the Consequences section about this trust model shift and its dependency on SHA pinning strategy.
4. Technical Accuracy Issues (minor)
The ADR states:
"approximately 30 files via
fullsend admin install, including full agent workflows (100–160 lines each)"
Actual counts:
- Workflow files: 7 (not 30)
- Line counts:
fix.ymlis 355 lines,dispatch.ymlis 188 lines (not 100-160)
These should be corrected for accuracy.
Summary
This ADR addresses a real problem but has critical design gaps:
- Version pinning strategy must be specified - moving refs break installations, pinned refs defeat the purpose
- Multi-forge divergence must be addressed - creates architectural inconsistency with GitLab support
- Trust boundary shift needs explicit documentation -
secrets: inheritchanges security model
I recommend requesting changes until these are resolved. The underlying idea may be sound, but the design is incomplete without answering these questions.
Proposes converting org .fullsend repos from full scaffold-copied workflows to thin callers that delegate to upstream reusable workflows via workflow_call, eliminating cross-org workflow drift. Signed-off-by: Wayne Sun <gsun@redhat.com>
Fix file count (~30 → ~50) and line range (100–160 → 125–354) to match actual scaffold contents. Fix leading zeros in title per lint rules. Add four missing consequences flagged by reviewers: trust boundary shift with secrets: inherit, upstream availability as a new failure mode, cross-repo debugging complexity, and GitHub-specific mechanism acknowledgment for multi-forge considerations. Signed-off-by: Wayne Sun <gsun@redhat.com>
306e2e9 to
85e4054
Compare
Site previewPreview: https://0991cc06-site.fullsend-ai.workers.dev Commit: |
Correct the claim that vars do not cross the workflow_call boundary — they do resolve from the caller's context automatically. Keep explicit inputs mapping for auditability, missing-variable safety, and review checkpoints. Fix nesting limit (4 → 10, changed Nov 2025). Add consequences for scaffold output changes and transitional PEM-based token generation. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
|
@ralphbean Thanks for the thorough review. Pushed updates addressing your feedback: Applied:
Additional changes from research verification:
|
|
@ggallen Thanks for the detailed review. Here's what was applied and what remains out of scope: Applied:
Not applied (out of ADR scope):
Your inline comment thread (custom agents) is already self-resolved. |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Review: ADR 0030 — Reusable workflows for action-installed distribution
The ADR is well-written and technically accurate. The workflow_call semantics (callee runs in caller's repo context, secrets: inherit, vars not crossing the boundary) are correct. The nesting analysis (1 of 4 levels) is sound. The second revision successfully addressed prior reviewer feedback on trust boundary, upstream availability, cross-repo debugging, and multi-forge implications.
Two minor notes inline — neither blocks approval.
| - composite-action | ||
| --- | ||
|
|
||
| # 30. Reusable workflows for action-installed distribution |
There was a problem hiding this comment.
[important] ADR number collision: open PR #715 ("ADR 30 — OpenShell sandbox interaction model") also claims number 0030. Whichever merges second will collide. The /renumber-adr process should be used before merge on one of the two PRs. Already flagged by the review bot — noting for visibility.
Correct scaffold file count (~50 → ~60, actual: 64) and agent workflow line range (125–354 → 78–305) per review bot findings against latest main branch scaffold. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
Should we consider using |
Reflect PR #503 changes: 4 composite actions (fullsend, mint-token, validate-enrollment, setup-gcp) must live upstream since uses: ./ resolves to the reusable workflow's repo. Update vars mapping (mint_url replaces client IDs), add SHA pinning recommendation, document uses: ./ resolution rule. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
Avoids collision with ADR 0030 on main (openshell sandbox interaction model). Renumber to 0031 to fit the updated ADR numbering scheme across open PRs. Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0031 slot now used by reusable workflows (PR #688). Renumber to 0033 and update internal cross-references from ADR 0030 to 0031. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
waynesun09
changed the title
- ~20 line thin callers → ~40 lines (actual) - secrets: inherit → explicit secret passthrough (least-privilege) - workflow_call nesting limit 10 → 4 (GitHub actual limit) - ~60 scaffold files → ~80 (actual count) - Token generation: remove stale "transitional PEM" paragraph, reflect current OIDC mint-token implementation - ADR 0027 → ADR 0029 (token mint renumbered on PR #655) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
|
@ggallen Good question. We considered a separate repo but staying with
If the monorepo grows unwieldy we can split later, but right now co-location keeps things simple. |
Well, the easy answer here is to just include the actions in the new repo as well. |
Summary
workflow_call) and a root composite action fromfullsend-ai/fullsend.fullsendrepos shrink from 100-160 line scaffold copies to ~20 line thin callers withsecrets: inheritvars.*mapped to explicitinputs.*since vars don't crossworkflow_callboundaryContext
Today
fullsend admin installcopies full agent workflows into each org's.fullsendrepo. When a bug fix or security patch lands in the scaffold, every org must re-run install to pick up the change. This ADR proposes the distribution model change; implementation follows in subsequent PRs.Test plan
workflow_callcontext semantics match assumptions (callee runs in caller's repo context)secrets: inheritpasses all required secrets without explicit mapping