Skip to content

ADR 0034: Centralized shim routing via dispatch.yml#730

Merged
waynesun09 merged 5 commits into
mainfrom
adr-0033-centralized-shim-routing
May 10, 2026
Merged

ADR 0034: Centralized shim routing via dispatch.yml#730
waynesun09 merged 5 commits into
mainfrom
adr-0033-centralized-shim-routing

Conversation

@waynesun09
Copy link
Copy Markdown
Contributor

@waynesun09 waynesun09 commented May 7, 2026

Summary

  • Proposes moving event-to-stage routing from the per-target-repo shim workflow into dispatch.yml, reducing the shim from 9 jobs (~470 lines) to 3 jobs (~75 lines)
  • New stages (command or event trigger) require only a case branch in dispatch.yml — zero changes to enrolled repos
  • Sequenced after the token mint migration (ADR 0029 / PR docs: ADR 0029 — central token mint for secretless .fullsend #655) which provides the workflow_call mechanism this ADR uses

Context

The shim workflow (fullsend.yaml) is duplicated in every enrolled repo. Each new stage requires updating every repo's shim. The routing logic — "which events map to which stages" — is conceptually part of the centralized fullsend system, not target-repo configuration.

This ADR resolves the inconsistency in the token mint plan (PR #655) between the Phase 3 single-job shim template and Resolved Decision #1's per-stage job structure, by choosing the centralized routing approach.

Related

Move event-to-stage routing from the per-target-repo shim into
dispatch.yml. The shim shrinks from 8 jobs (~380 lines) to 2 jobs
(~50 lines). New stages require zero changes to enrolled repos.

Sequenced after the token mint migration (ADR 0027 / PR #655)
which provides the workflow_call mechanism this ADR uses.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

Site preview

Preview: https://10571993-site.fullsend-ai.workers.dev

Commit: 7bff81a24e52d6949f691e38f2a63ea8b92b2521

@fullsend-ai-review
Copy link
Copy Markdown

fullsend-ai-review Bot commented May 7, 2026

Review: #730

Head SHA: 96567a1
Timestamp: 2026-05-10T00:00:00Z
Outcome: comment-only

Summary

This PR adds a well-structured ADR proposing centralized shim routing via dispatch.yml (Option C). The ADR follows the template conventions, presents clear options with trade-offs, and addresses security properties thoughtfully. The only notable issues are incorrect cross-references to other in-flight ADRs — most importantly a broken relative link in the ADR file itself that points to a non-existent filename.

Findings

Medium

Low

Info

  • [Style/conventions] Branch name adr-0033-centralized-shim-routing doesn't match the file number 0034. No functional impact, but worth noting if the renumbering was intentional.
  • [Correctness] docs/ADRs/0034-centralized-shim-routing-via-dispatch.md:149 — Reference to 0031-per-repo-installation-mode.md which doesn't exist on disk. Likely an in-flight PR — verify the filename will match once that ADR merges, or this will also be a broken link.

Footer

Outcome: comment-only
This review applies to SHA 96567a13256ef032e22f77313627482dee940607. Any push to the PR head clears this review and requires a new evaluation.

Previous run

Review: #730

Head SHA: 73922c8
Timestamp: 2026-05-10T00:00:00Z
Outcome: request-changes

Summary

The ADR is well-structured and presents a clear, well-reasoned case for centralizing shim routing. However, the document has a numbering conflict (PR title says "ADR 0033", file is numbered 0034) and several cross-references to ADRs that either don't exist on main or point to the wrong document. These broken references undermine the document's reliability and will produce dead links for readers.

Findings

High

  • [Correctness] docs/ADRs/0034-centralized-shim-routing-via-dispatch.md — ADR number conflict: the PR title says "ADR 0033" but the file is numbered 0034 (both in the filename and in the frontmatter title "34."). The next available number after 0032 on main is 0033. Either the title or the file needs to be corrected, and the number should be validated against what's already on the target branch to avoid collisions.
    Remediation: Reconcile the ADR number. If 0033 is correct, rename the file to 0033-centralized-shim-routing-via-dispatch.md and update the frontmatter/heading. If 0034 is intentional (reserving 0033 for another in-flight ADR), update the PR title.

  • [Correctness] docs/ADRs/0034-centralized-shim-routing-via-dispatch.md — Multiple broken cross-references to ADRs that don't exist or reference the wrong document:

    1. References "ADR 0027" as "Token mint (docs: ADR 0029 — central token mint for secretless .fullsend #655)" but 0027 on main is "Allowed and Disallowed Tools for Agents" — different topic entirely.
    2. References 0030-reusable-workflows-for-action-installed-distribution.md but 0030 on main is 0030-openshell-sandbox-interaction-model.md — different topic.
    3. References 0031-per-repo-installation-mode.md which does not exist on main.
      These may be in-flight ADRs in other branches, but as written they will be dead/misleading links for any reader on main.
      Remediation: Verify the correct ADR numbers for these references. If the referenced ADRs haven't been merged yet, use full PR links (as done for ADR 0027/PR docs: ADR 0029 — central token mint for secretless .fullsend #655) rather than relative file links, or note that the ADR numbers are provisional.

Info

  • [Style] The ADR is well-written: clear problem statement, three options with trade-offs, explicit security properties preserved, and thoughtful consequences section. Follows repo ADR conventions including frontmatter, status, and relates_to fields.

  • [Intent alignment] No linked issue. The PR is a design document (ADR) proposing centralized shim routing. Scope is appropriate — single new file, no code changes.

Footer

Outcome: request-changes
This review applies to SHA 73922c84de57ea6db538ac3e7a0594f01ba7f18c. Any push to the PR head clears this review and requires a new evaluation.

Previous run (2)

Review: #730

Head SHA: c4be98f
Timestamp: 2026-05-07T00:00:00Z
Outcome: comment-only

Summary

This ADR is well-structured, correctly motivated, and technically sound. It proposes moving event-to-stage routing from the per-repo shim into dispatch.yml, which directly resolves the limitation noted in ADR 0026 (line 153–154: "Adding a new stage still requires changes to the shim template"). The security analysis is thorough and correctly identifies that pull_request_target protection, payload injection defense, and fork-PR blocking are preserved. A few minor factual inaccuracies and informational observations are noted below — none blocking.

Findings

Medium

  • [Correctness] docs/ADRs/0033-centralized-shim-routing-via-dispatch.md:28 — The ADR states the shim "currently has 8 jobs and ~380 lines." The actual shim (fullsend.yaml) has 9 jobs (8 dispatch/action jobs + post-run-link) and 468 lines. Even excluding post-run-link, the 8 remaining jobs span ~434 lines, not ~380. Consider correcting to avoid confusion when readers cross-reference.
    Remediation: Update to "8 dispatch jobs and ~430 lines" or "9 jobs and ~470 lines" depending on whether post-run-link is counted.

Low

  • [Correctness] docs/ADRs/0033-centralized-shim-routing-via-dispatch.md:56 — Option A says "8 jobs, ~250 lines" for the post-token-mint status quo, but the derivation of 250 from the current 468 isn't explained. The token mint removes the FULLSEND_DISPATCH_TOKEN and gh workflow run boilerplate but the dispatch steps themselves remain. The claimed reduction (~47%) seems large for switching from workflow_dispatch to workflow_call alone.
    Remediation: Add a brief note explaining what accounts for the ~200-line reduction (e.g., "payload-building steps shrink because workflow_call passes inputs directly").

Info

  • [Correctness] docs/ADRs/0033-centralized-shim-routing-via-dispatch.md:107 — The workflow_call input arithmetic ("current 5 inputs minus stage plus event_action = 5 inputs") is correct against the current dispatch.yml inputs: stage, event_type, source_repo, event_payload, trigger_source.

  • [Intent alignment] The ADR correctly positions itself as sequenced after ADR 0027 (token mint, PR docs: ADR 0029 — central token mint for secretless .fullsend #655) and complementary to ADR 0030 (reusable workflows, PR docs: ADR 0031 — reusable workflows for action-installed distribution #688). Note that PR docs: ADR 0029 — central token mint for secretless .fullsend #655 uses number 0027 which collides with the existing ADR 0027 on main ("Allowed and Disallowed Tools for Agents"). This is a pre-existing numbering conflict in PR docs: ADR 0029 — central token mint for secretless .fullsend #655, not an issue with this ADR, but the cross-reference may need updating once docs: ADR 0029 — central token mint for secretless .fullsend #655 is renumbered.

  • [Correctness] docs/ADRs/0033-centralized-shim-routing-via-dispatch.md:131 — The ADR notes runner cost for no-match events (~20s spin-up). The current shim avoids this cost entirely via if: filters that skip jobs before they run. The cost increase is acknowledged but not quantified (e.g., how many no-match events per day). This is acceptable for a Proposed ADR — quantification can come during implementation.

  • [Style/conventions] The ADR follows the template from 0000-adr-template.md correctly: frontmatter with title, status, relates_to, topics; sections for Status, Context, Options, Decision, Consequences. Title uses the post-PR#719 convention (no leading zeros in title text, leading zeros in filename).

Footer

Outcome: comment-only
This review applies to SHA c4be98f2fef0ea17303c54ed3acd7e4cfc3b1464. Any push to the PR head clears this review and requires a new evaluation.

Copy link
Copy Markdown
Contributor

@ralphbean ralphbean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good ADR — the problem is real and Option C is the right direction. One blocking issue (broken cross-references) and several informational notes below.

The fullsend-ai-review bot's findings on line counts and input arithmetic align with what I found; I've noted those inline as well.

Comment thread docs/ADRs/0034-centralized-shim-routing-via-dispatch.md
Comment thread docs/ADRs/0034-centralized-shim-routing-via-dispatch.md Outdated
Comment thread docs/ADRs/0034-centralized-shim-routing-via-dispatch.md
Comment thread docs/ADRs/0034-centralized-shim-routing-via-dispatch.md
Comment thread docs/ADRs/0034-centralized-shim-routing-via-dispatch.md
Comment thread docs/ADRs/0034-centralized-shim-routing-via-dispatch.md
Comment thread docs/ADRs/0034-centralized-shim-routing-via-dispatch.md Outdated
Copy link
Copy Markdown
Contributor

@ralphbean ralphbean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — this is a well-motivated ADR and Option C is the right call. The inline comments from the previous review still stand as informational notes. The one thing to sort out before merging: the ADR 0027 and 0030 cross-references need correcting (0027 on main is "Allowed and Disallowed Tools for Agents", not the token mint; 0030 doesn't exist on main yet).

Connect centralized shim routing to per-repo installation mode
(ADR 0031). Per-repo repos call reusable-fullsend.yml directly
from fullsend-ai/fullsend — the routing implementation should be
shared between per-org dispatch.yml and per-repo.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

fullsend review is working on this — view logs

Avoids collision with ADR 0033 on main. Renumber to 0034 to fit
the updated ADR numbering scheme across open PRs.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

Copy link
Copy Markdown

@fullsend-ai-review fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

@github-actions
Copy link
Copy Markdown

fullsend fix is working on this — view logs

@waynesun09 waynesun09 changed the title ADR 0033: Centralized shim routing via dispatch.yml ADR 0034: Centralized shim routing via dispatch.yml May 10, 2026
Token mint ADR renumbered from 0027 to 0029 on PR #655.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

- Fix cross-refs: ADR 0030→0031 (reusable workflows), ADR 0031→0033 (per-repo)
- Add post-run-link as third shim job, update "two jobs" → "three jobs"
- Add trigger_source to dispatch inputs, fix input count (5→6)
- Fix Option A job/line counts to match current shim (9 jobs, ~350 lines)
- Remove backslash escapes on ready_for_review
- Note concurrency group is a behavioral change from independent per-stage runs

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

@waynesun09 waynesun09 added this pull request to the merge queue May 10, 2026
Merged via the queue into main with commit 2e3c285 May 10, 2026
83 checks passed
@waynesun09 waynesun09 deleted the adr-0033-centralized-shim-routing branch May 10, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants