docs: accept ADR 0041 and update living docs - #1578
Conversation
Site previewPreview: https://bccc31fb-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsLow
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsNo findings. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report (6 agents)
Overall: The PR's docs changes are clean and well-placed — status change is consistent, cross-references are correct, living doc updates land in the right sections. The ADR's core decision is sound (workflow_call is the right fix for UI correlation, and per-repo mode already validates the pattern in production).
However, the ADR body has gaps that should be addressed before or alongside acceptance:
- 3 HIGH findings: outdated nesting limit (4 → 10 since Nov 2025), missing context about
workflow_dispatchrun ID API (Feb 2026), and undocumented concurrency/cancel-in-progress regression - 5 MEDIUM findings: nesting math underspecified, fan-out semantics change, ADR 0026 missing supersession annotation (4-agent consensus), secrets flow dependency on ADR 0029, vague ADR 0038 escape hatch
Two HIGH findings (H1, H2) reference GitHub changelog entries that need source verification — flagged inline.
See inline comments for details and suggestions.
| **Decided:** | ||
|
|
||
| - JSONL reasoning trace exposure: raw JSONL conversation transcripts are extracted from sandboxes and stored with owner-scoped access. Credential scanning acts as an invariant check on [ADR 0017](ADRs/0017-credential-isolation-for-sandboxed-agents.md)'s isolation model. Agents handling data from protected sources beyond the target repo can opt in to JSONL suppression via configuration ([ADR 0021](ADRs/0021-jsonl-reasoning-trace-exposure.md)). | ||
| - Event-driven stage dispatch should remain traceable end-to-end in the GitHub Actions UI by using synchronous `workflow_call` dispatch (see [ADR 0041](ADRs/0041-synchronous-workflow-call-event-dispatch.md)). |
There was a problem hiding this comment.
[MEDIUM] Inconsistent phrasing — "should remain" vs declarative tone
This bullet uses "should remain traceable" which reads as a recommendation, not a decided fact. The other two ADR 0041 bullets in this file use declarative framing: "eliminate workflow_dispatch..." (Agent Infrastructure) and "runs synchronously via workflow_call..." (Dispatch). Other Decided blocks in this doc consistently state what is decided, not what should happen.
Suggestion: Change to "remains traceable end-to-end" or "is traceable end-to-end" to match the declarative tone.
Flagged by 3 of 6 review agents.
There was a problem hiding this comment.
Good catch — updated this to declarative tone in the latest push ("remains traceable end-to-end").
There was a problem hiding this comment.
Applied in PR: updated docs/architecture.md bullet to declarative phrasing ('remains traceable').
There was a problem hiding this comment.
(Thread reply) Applied in PR: updated docs/architecture.md bullet to declarative phrasing (remains traceable).
| --- | ||
| title: "41. Synchronous workflow_call for event-driven agent dispatch" | ||
| status: Proposed | ||
| status: Accepted |
There was a problem hiding this comment.
[HIGH] Nesting depth limit is outdated — "four" should be "ten"
The Decision section says "stay within four workflow_call nesting levels." GitHub raised the limit from 4 to 10 levels (with 50 total workflow calls per run) in November 2025 — 6 months before this ADR was written. ADR 0031 also references "limit is 4."
This doesn't change the decision (workflow_call is still correct), but the nesting guidance is based on a stale constraint. With 10 levels available, the 3-level chain (shim → dispatch → thin caller → reusable) has ample headroom, and the "prefer dispatch.yml → reusable-*.yml@*" guidance is less critical than implied.
Additionally [MEDIUM]: The Consequences section should spell out the nesting arithmetic. Under the old async model, workflow_dispatch resets the nesting counter (ADR 0031 documents this explicitly). After this ADR, the full chain uses 3 of the available levels. The "prefer direct" guidance implies skipping thin callers (dropping to 2 levels), but thin callers exist for secrets: passthrough and vars.* → inputs.* mapping (ADR 0031). The Consequences should document what happens to the thin-caller pattern.
Suggestion: Update the limit reference and add a consequence bullet documenting the nesting math and its impact on ADR 0031's thin-caller architecture.
Nesting limit flagged by 2 of 6 review agents. Needs source verification on the Nov 2025 changelog.
There was a problem hiding this comment.
Thanks — ADR 0041 is already accepted/merged, so we should avoid substantive retroactive edits to its content. I filed #1636 to verify current GitHub limits and document the nesting arithmetic + thin-caller implications as a supplementary ADR / living-docs update.
There was a problem hiding this comment.
Applied in PR: updated ADR 0041 nesting guidance to reflect the current 10-level reusable workflow limit. Issue #1636 is closed.
There was a problem hiding this comment.
(Thread reply) Applied in PR: updated ADR 0041 nesting guidance to reflect the current 10-level reusable workflow limit. Issue #1636 is closed.
| ## Status | ||
|
|
||
| Proposed | ||
| Accepted |
There was a problem hiding this comment.
[HIGH] Context omits that workflow_dispatch API now returns run IDs (Feb 2026)
As of February 2026, the workflow_dispatch API supports return_run_details which returns the run ID, API URL, and workflow URL. gh workflow run in CLI 2.87.0+ enables this by default. This shipped before this ADR was written (May 20, 2026) and is not mentioned.
This doesn't invalidate the decision — the UI still shows separate top-level runs with no native parent/child graph, and monitoring completion from the dispatch job would require polling (adding runner minutes). But it significantly strengthens Option C ("Mitigations only"): dispatch.yml could capture the run ID, post a direct link, and optionally monitor completion.
Suggestion: Acknowledge this in the Context section and explain why it's insufficient. This strengthens the ADR by showing the decision was made with awareness of the alternative, not despite it.
Flagged by 1 of 6 review agents. Needs source verification on the Feb 2026 changelog.
There was a problem hiding this comment.
Agree this is important context, but since ADR 0041 is accepted/merged we should capture it as follow-up rather than rewriting the historical ADR. I filed #1638 to verify behavior and document why it still doesn’t meet the correlation goals (and what mitigations it enables).
There was a problem hiding this comment.
(Follow-up) Captured as follow-up issue #1638 to verify workflow_dispatch return_run_details and document why it still does not meet our correlation goals (plus what mitigations it enables).
There was a problem hiding this comment.
Applied in PR: added note that workflow_dispatch can return run details, but still doesn’t provide native caller→callee UI correlation. Issue #1638 is closed.
There was a problem hiding this comment.
(Thread reply) Applied in PR: added note that workflow_dispatch can return run details, but still doesn’t provide native caller→callee UI correlation. Issue #1638 is closed.
There was a problem hiding this comment.
[HIGH] Consequences section missing: concurrency model change + fan-out semantics
Two significant behavioral changes are not documented in the Consequences:
1. Concurrency / cancel-in-progress regression
Currently, thin callers define per-stage concurrency with cancel-in-progress: true — a new push cancels the running review and starts fresh. The shim defines cancel-in-progress: false, which is fine because the shim job finishes in ~30 seconds under the async model.
In the sync model, the shim job stays in-progress for the full agent duration (20+ minutes). Combined with cancel-in-progress: false, new events for the same PR/issue will queue behind the running agent instead of cancelling it. The per-stage cancel-in-progress behavior from thin callers is lost.
Suggestion: Add a consequence noting that per-stage concurrency groups must be moved to the workflow_call jobs in dispatch.yml, and that the shim's concurrency behavior changes from "quick release" to "held for full agent duration." Also consider whether cancel-in-progress: true should be the default on stage jobs, and whether a triage comment should be able to cancel a running review (cross-stage cancellation).
2. Fan-out semantics change
With workflow_call, all stage jobs run within a single workflow run. This changes: (a) cancellation scope — one concurrency group covers all stages in a dispatch, (b) failure isolation — one stage's failure affects the overall run status, (c) the ability to have multiple independent workflow files per stage (ADR 0026's fan-out model). ADR 0020 describes stages composed of multiple single-responsibility agents — if a future architecture needs two review agents for the same stage, they must now be composed inside a single reusable workflow rather than as independent workflow files.
Suggestion: Add a consequence about fan-out: "Stage workflows called via workflow_call execute as jobs within the dispatch.yml run, not as independent runs. Per-stage fan-out to multiple independent workflow files is no longer possible — composition within a stage should happen inside the reusable workflow."
Concurrency flagged by 1 of 6 agents; fan-out flagged by 2 of 6 agents.
There was a problem hiding this comment.
These are substantive behavioral consequences; given ADR 0041 is accepted/merged, I’m treating them as follow-up work rather than retroactively expanding the ADR. I filed #1637 to spell out the desired concurrency/cancel semantics and the fan-out/composition implications under .
There was a problem hiding this comment.
(Follow-up) Captured as follow-up issue #1637 to spell out desired concurrency/cancel semantics and fan-out/composition implications under workflow_call dispatch.
There was a problem hiding this comment.
Tracked as follow-up: concurrency/cancel-in-progress + fan-out semantics remain in issue #1637 (still open).
There was a problem hiding this comment.
(Thread reply) Tracked as follow-up: concurrency/cancel-in-progress + fan-out semantics remain in issue #1637 (still open).
There was a problem hiding this comment.
[MEDIUM] Three cross-reference and dependency gaps
1. ADR 0026 missing supersession back-reference
ADR 0041's Consequences explicitly state: "Supersedes ADR 0026 Option C for the event path." But ADR 0026's Status section still reads plain Accepted with no annotation. Other ADRs in this repo carry forward references (e.g., ADR 0017 → ADR 0025). Readers of ADR 0026 won't know Option C's marker-scanning mechanism has been deprecated.
Suggestion: Add to ADR 0026's Status: Accepted (Option C superseded by [ADR 0041](0041-synchronous-workflow-call-event-dispatch.md) for the event-driven dispatch path).
Flagged by 4 of 6 review agents.
2. Secrets/credentials flow dependency on ADR 0029 not stated
With workflow_call, secrets and vars are evaluated against the caller's repository context. The enrolled repo doesn't have .fullsend's GCP secrets. ADR 0031 confirms reusable workflows use OIDC-based token minting (ADR 0029), which avoids this. But ADR 0041 doesn't state this dependency.
Suggestion: Add a dependency note: "This decision assumes ADR 0029's token mint model is in place so agent workflows obtain credentials via OIDC without relying on .fullsend repo-level secrets."
3. ADR 0038 escape hatch is vague
"After agent architecture is revised to support ADR 0038, re-evaluate whether a discovery mechanism is needed" — ADR 0038 is still Proposed with broad scope and no timeline. Currently ~6 workflows per org, changed rarely by the team that owns .fullsend, so the static-wiring cost is genuinely low. But the escape hatch would benefit from a concrete trigger.
Suggestion: Add: "Re-evaluate if orgs regularly maintain more than N agent workflows or if ADR 0038 is not accepted within 6 months."
There was a problem hiding this comment.
Good points on cross-references/dependencies. Because ADR 0041 is accepted/merged, I captured this as follow-up work (and a chance to clarify our conventions for annotating supersession/dependencies without rewriting history): #1639.
b3adaf5 to
7faa185
Compare
|
|
||
| Operational pain is documented across multiple issues: | ||
|
|
||
| - [#504](https://github.com/fullsend-ai/fullsend/issues/504) — enrolled-repo shim |
There was a problem hiding this comment.
[low] correctness
The updated text states "GitHub currently allows up to ten levels" for workflow_call nesting depth. Existing accepted ADRs in this repo (ADR 0031, ADR 0033) reference a 4-level limit. This claim should be verified against current GitHub Actions documentation.
Suggested fix: Verify the current GitHub Actions workflow_call nesting limit. If it is still 4, update the parenthetical to match. If GitHub has increased it, the text is correct as-is.
There was a problem hiding this comment.
Confirmed: GitHub increased reusable-workflow limits to 10 nested levels and 50 total calls (GitHub changelog 2025-11-06 and current GitHub Docs ‘Reuse workflows’ section). The ADR statement is intended to reflect the constraint as of May 2026.
There was a problem hiding this comment.
Verified 10-level nesting limit: GitHub changelog 2025-11-06 + GitHub Docs ('Reuse workflows') both state max 10 levels (and 50 total calls).
There was a problem hiding this comment.
(Thread reply) Verified 10-level nesting limit: GitHub changelog 2025-11-06 + GitHub Docs (“Reuse workflows”) both state max 10 levels (and 50 total calls). Older ADRs reflect the prior limit when written.
fullsend-ai-review
Bot
added
ready-for-merge
Mark ADR 0041 as Accepted and update the living architecture and problem documents to reflect the decision. Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com>
58742ea to
9be94f5
Compare
| @@ -108,8 +114,8 @@ convention. Do not add compile-time sync tooling as a substitute. | |||
| `workflow_dispatch` remains allowed for **non-event** entry points only (e.g. | |||
There was a problem hiding this comment.
[low] correctness
The updated text states "GitHub currently allows up to ten levels" for workflow_call nesting depth. Existing accepted ADRs in this repository (ADR 0031, ADR 0033) reference a 4-level limit. This claim should be verified against current GitHub Actions documentation to avoid introducing a factual inconsistency across the ADR corpus.
There was a problem hiding this comment.
Same as above: verified 10-level nesting limit in GitHub Docs + 2025-11-06 changelog. Older ADRs referencing 4 reflect the prior limit at the time they were written.
There was a problem hiding this comment.
Same verification as above: 10 nested levels per GitHub Docs + 2025-11-06 changelog; older ADRs reflect the prior 4-level limit when written.
There was a problem hiding this comment.
(Thread reply) Same verification as above: 10 nested levels per GitHub Docs + 2025-11-06 changelog; older ADRs reflect the prior 4-level limit when written.
fullsend-ai-review
Bot
added
ready-for-merge
|
Applied review follow-ups directly on this PR (instead of separate threads/PRs) and closed the now-unneeded trackers. Applied directly in PR
Closed as resolved/unneeded
Remaining follow-up
(For nesting limit source: GitHub changelog 2025-11-06 and GitHub Docs both state 10 levels / 50 total calls.) |
2 participants
Summary
docs/architecture.mdwith the ADR 0041 decision (living doc).Test plan
pre-commit run --files docs/ADRs/0041-synchronous-workflow-call-event-dispatch.md docs/architecture.md docs/problems/agent-infrastructure.md docs/problems/operational-observability.mdMade with Cursor