docs: ADR 0033 — per-repo installation mode - #707
Conversation
Proposes a per-repo installation mode where fullsend runs entirely within a single repository, building on ADR 0030 (reusable workflows) and ADR 0027 (central token mint). Enables users without org admin access to adopt fullsend by adding a single ~30-line workflow file. Signed-off-by: Wayne Sun <gsun@redhat.com>
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Review Summary
This is a well-structured ADR that addresses a real adoption gap — users without org admin access currently cannot use fullsend. The per-repo approach is sound, building on reusable workflows (ADR 0030) and the token mint (ADR 0027) rather than creating a parallel system.
Strategic assessment: Good idea, fits project scope, no objections to the direction.
Requesting changes on the pull_request_target security analysis — given the project's threat priority order (external injection is highest), the per-repo model's unique risk surface (workflow file lives alongside code) deserves deeper treatment in the Risks section.
Two additional notes are deferred for future consideration (coder/fix role conflation, event payload trimming).
| - `issues` + `labeled` → stage based on label name (`ready-to-code` → code, `ready-for-review` → review) | ||
| - `issue_comment` + slash commands → `/triage`, `/code`, `/review`, `/fix` | ||
| - `pull_request_target` → review (or retro on close) | ||
| - `pull_request_review` + `changes_requested` from bot → fix |
There was a problem hiding this comment.
[important] Note (deferred): The ADR says coder and fix share one app ({user}-coder, role "Code + fix"), but AgentAppConfig() in internal/forge/github/types.go treats "fix" as a separate role — it falls through to the default case and gets only issues:read permissions, not the full coder permission set.
Is this an intentional proposal to consolidate fix into coder for per-repo mode, or an inaccuracy? If intentional, it's worth calling out explicitly as a deviation from the per-org model. If not, fix should be listed as a separate row in the table.
| - **Clear error messages**: Credential auto-detection reports why coder and review Apps must be separate, with a link to setup documentation. | ||
| - **Migration path**: Per-repo users who outgrow the model can migrate to per-org without changing agent behavior — the same reusable workflows power both modes. | ||
|
|
||
| ## Open Questions |
There was a problem hiding this comment.
[moderate] Note (deferred): The routing logic only needs a few fields (action type, label name, comment body, PR number). Passing the full toJSON(github.event) risks hitting the 65KB workflow_call input limit on large PRs with long descriptions or many changed files. Consider proposing selective field extraction from the start rather than deferring trimming until it becomes a problem — the fields needed for routing are well-defined and unlikely to change.
|
/review |
Review: #707Head SHA: 89cd3a9 SummaryThis PR adds ADR 0033 proposing a per-repo installation mode for fullsend. The document is thorough, well-structured, and technically sound — covering architecture, credential models, config layering, CLI support, coexistence, and security risks aligned with the project's threat priority order. One inconsistency was found: the frontmatter status and body status disagree. Cross-references to ADRs 0029 and 0035 depend on other open PRs landing first. No security, injection, or correctness concerns — this is a documentation-only change. FindingsMedium
Info
FooterOutcome: comment-only Previous runReview: #707Head SHA: 3e4a50c SummaryThis PR adds ADR 0033, proposing a per-repo installation mode for fullsend. The ADR is well-structured, thoroughly covers architecture, credential models, coexistence, risks, and mitigations. It correctly identifies the key security surfaces (pull_request_target misconfiguration, insider workflow modification) and proposes appropriate mitigations (CODEOWNERS, base-branch config reads). Two internal inconsistencies should be resolved before merging: a status field contradiction and a nesting depth count discrepancy between sections. FindingsMedium
Info
FooterOutcome: comment-only Previous run (2)Review: #707Head SHA: 3df07bb SummaryThis PR adds ADR 0033, a well-structured architectural decision record proposing a per-repo installation mode. The design is thorough, covers credential models, coexistence, and migration paths, and reuses existing ADR 0031/0034/0035 infrastructure appropriately. Three medium-severity findings relate to internal consistency: a status mismatch between frontmatter and body, a broken cross-reference link to ADR 0034 (wrong filename), and missing ADR files (0029, 0035) that are referenced throughout but don't exist in the repo. None are blocking. FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (3)Review: #707Head SHA: 6455f27 SummaryThis PR adds ADR 0033, proposing a per-repo installation mode for fullsend. The document is well-structured, follows the ADR template conventions, and thoroughly covers architecture, credential models, coexistence, and risks. The security considerations (pull_request_target enforcement, credential scoping, CODEOWNERS governance) are thoughtful and align with the project's threat model. Two broken cross-reference links need attention before merge to avoid dead links in the ADR corpus. FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (4)Review: #707Head SHA: 515d279 SummaryWell-structured ADR that clearly motivates per-repo installation, documents rejected alternatives with reasoning, and reuses existing infrastructure (ADR 0031 reusable workflows, ADR 0029 token mint). The architecture section is thorough and the coexistence/migration story is well thought out. One broken cross-reference in the References section should be fixed, and two informational items are noted below. FindingsMedium
Info
FooterOutcome: comment-only Previous run (5)Review: #707Head SHA: 6c45855 SummaryThis ADR proposes a per-repo installation mode — a sound architectural concept that logically extends reusable workflows and a central token mint as foundations. However, both foundational ADRs are incorrectly referenced: ADR 0027 in this repo is "Allowed and Disallowed Tools for Agents" (not "Central token mint"), and ADR 0031 does not exist at all. The entire decision rests on two predecessor ADRs that are either misidentified or missing, which makes this ADR unresolvable as written. The document itself is well-structured, thoroughly considers alternatives, and correctly identifies security risks (pull_request_target, App identity confusion, payload size limits). Once the cross-reference issues are resolved, this should be straightforward to approve. FindingsHigh
Medium
Info
FooterOutcome: request-changes Previous run (6)Review: #707Head SHA: f3dc56a SummaryThis ADR proposes a per-repo installation mode — a well-structured and useful addition. However, the ADR contains broken cross-references to non-existent ADRs and an incorrect ADR number that must be resolved before merging. The document references "ADR 0027" as a "Central token mint" and "ADR 0030" as "Reusable workflows", but the actual ADR 0027 in the repo is "Allowed and Disallowed Tools for Agents" (an entirely different topic), and no ADR 0028+ exists beyond 0028-gitlab-support.md. The ADR number 0031 itself skips 0029 and 0030, which don't exist. FindingsHigh
Medium
Info
FooterOutcome: request-changes |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
I don't like that this mixes the reusable workflows and the mint pieces because then it makes it dependent on them and I think this is not dependent on those. Basically it is forcing us to merge those.
When these ADRs land they won't be "alternatives" here, they will be the way it works so this ADR is basically rediscussing the two that it mentions.
I think that this ADR does not have any options so far, we just need to support to install in a single repo, which today it means copying all the scaffold to a single repository and having a shim that points to itself somehow, user applications instead of org applications, repo secrets, tec.
That being said I'm all for speed, so we can go with it.
There was a problem hiding this comment.
LGTM (excpet @rh-hemartin's comment about the cli command)
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Review Summary
Directionally sound ADR that addresses a real adoption gap. The approach of layering per-repo on top of reusable workflows (rather than building a parallel system) is the right call.
I've added several notes below as deferred observations — mostly around security surface analysis that should be deepened before this ADR moves from Proposed to Accepted. The existing reviews from @ralphbean (security depth, role conflation, payload size) and @rh-hemartin (CLI naming, dependency coupling) are well-founded; I've thumbs-upped those inline comments rather than duplicating them.
Key themes across my notes:
- The per-repo model introduces a fundamentally different security surface than per-org (credential separation collapse, pwn-request pattern, AGENTS.md sourcing) that deserves explicit treatment in the Risks section
- The event suppression mechanism in Alternative 2 is technically imprecise (correct conclusion, wrong explanation)
- Minor: title numbering convention mismatch with existing ADRs
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Approving — the direction is sound and the deferred notes can be addressed as the ADR moves toward Accepted status.
Align with PR #503 changes: - Token mint is now the default credential model (Model A), own Apps is opt-in (Model B) - Remove dispatch PAT references (PATs eliminated entirely) - Remove *_CLIENT_ID vars (mint-token uses role-based lookup) - Update architecture diagrams for workflow_call dispatch - Clarify org admin still needed for GitHub App installation - Update CLI section: mint is default, --own-apps is opt-in - Reference four composite actions (fullsend, mint-token, validate-enrollment, setup-gcp) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
Site previewPreview: https://bf542025-site.fullsend-ai.workers.dev Commit: |
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
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
|
fullsend fix is working on this — view logs |
Token mint ADR renumbered from 0027 to 0029 on PR #655. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
Consolidate per-repo setup into the existing install command per reviewer feedback (PR #707). Mode is detected from argument format: org name for per-org, owner/repo for per-repo. Removes the standalone init subcommand and adds --mint-url, --gcp-auth-mode, --scaffold-customized flags to install. Signed-off-by: Wayne Sun <gsun@redhat.com>
- Status: Proposed → Accepted - CLI: fullsend init → fullsend admin install <owner/repo> (per Hector's feedback) - Mode detection: document install_mode input parameter on reusable workflows - Open Questions → Resolved Questions (dispatch mechanism, concurrency, stop-fix, CLI design) - Update all fullsend init references throughout the document Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
- Fix event suppression wording: token-based, not ownership-based - Add credential separation collapse as explicit negative consequence - Specify base-branch-only config reads for .fullsend/ and AGENTS.md - Expand Risks section with threat-priority ordering, pwn request surface, insider modification risk, and CODEOWNERS mitigation - Fix broken ADR 0034 cross-reference link filename Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
… ADR 0033 The token mint (ADR 0029) migrates per-org dispatch from workflow_dispatch + gh workflow run to native workflow_call. Update all diagrams and descriptions to reflect the post-token-mint mechanism. 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 |
The markdown link linter checks that linked files exist. ADR 0029 (token mint) and ADR 0035 (layered content) haven't landed yet, so convert their markdown links to plain text references. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
Consolidate per-repo setup into the existing install command per reviewer feedback (PR #707). Mode is detected from argument format: org name for per-org, owner/repo for per-repo. Removes the standalone init subcommand and adds --mint-url, --gcp-auth-mode, --scaffold-customized flags to install. Signed-off-by: Wayne Sun <gsun@redhat.com>
Consolidate per-repo setup into the existing install command per reviewer feedback (PR #707). Mode is detected from argument format: org name for per-org, owner/repo for per-repo. Removes the standalone init subcommand and adds --mint-url, --gcp-auth-mode, --scaffold-customized flags to install. Signed-off-by: Wayne Sun <gsun@redhat.com>
5 participants
Summary
.fullsendconfig repo, no cross-repo dispatch, no org-level secretsreusable-dispatch.yml— an all-in-one routing + dispatch reusable workflow combining event routing with per-stage conditionalworkflow_calljobsfullsend admin install <owner/repo>for per-repo,fullsend admin install <org>for per-org (unified command, implemented in PR feat: add per-repo installation mode (ADR 0033) #799)Changes since initial proposal
fullsend admin install <owner/repo>(no separatefullsend init)install_modeinput parameter documentation for reusable workflowsgh workflow runtoworkflow_call(post-token-mint)Test plan
AgentAppConfig()ininternal/forge/github/types.goworkflow_callnesting depth claim (3 levels: shim → reusable-dispatch → reusable-stage, within GitHub's 4-level limit)