docs(adr): serialize agent runs and coalesce subsequent events - #6909
docs(adr): serialize agent runs and coalesce subsequent events#6909ifireball wants to merge 2 commits into
Conversation
PR Summary by QodoDocument serialized agent runs and bounded event coalescing
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
|
🤖 Review · Commit: |
Site previewPreview: https://65a0e30b-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1. Coalescing discards event contents
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
e0db092 to
7f56095
Compare
ifireball
changed the title
Signed-off-by: Barak Korren <bkorren@redhat.com> Assisted-by: Codex (gpt-5.6-sol)
|
🤖 Review · Commit: |
7f56095 to
0493886
Compare
|
🤖 Finished Review · ✅ Success · Started 11:59 AM UTC · Completed 12:17 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.93 |
|
Risk Assessment: moderate (2/5) DetailsDocs/config-only PR adding a new ADR with minor cross-references; no protected paths, security-sensitive files, CI, or dependency changes. Slight elevation from medium blast radius and high churn on docs/architecture.md, but the actual change to that file is small (10 added, 1 deleted). Signals unchanged from prior assessment; score preserved at 2. Previous runRisk Assessment: moderate (2/5) DetailsDocs/config-only PR adding a new ADR with minor cross-references; no protected paths, security-sensitive files, CI, or dependency changes. Slight elevation from medium blast radius and high churn on docs/architecture.md, but the actual change to that file is trivial (9 added lines). Overall low-to-moderate risk. |
|
Looks good to me Previous runReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Barak Korren <bkorren@redhat.com> Assisted-by: Codex (gpt-5.6-sol)
|
🤖 Finished Review · ✅ Success · Started 11:09 AM UTC · Completed 11:29 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.55 |
Superseded by updated review
fullsend-ai-review
Bot
added
the
ready-for-merge
| The execution platform MUST allow the active run to finish and coalesce later | ||
| matching events into one pending run representing the newest retained event. | ||
| GitHub Actions provides these semantics with a subject-scoped concurrency group, | ||
| `cancel-in-progress: false`, and its default single-pending queue |
There was a problem hiding this comment.
To me this is yet another perfect example of GH-AW overlap.
A thing I find very compelling about GH-AW in this respect is that what GitHub Actions providers, GH-AW automatically provides too!
This is the "input side" mirroring the discussion in #6614
We could expose e.g.
github:
concurrency: cancel-in-progress: false
BTW there's also some experimental support in GH-AW for feeding dynamic events into agent runs automatically, check e.g. https://github.github.com/gh-aw/reference/safe-outputs/#steering-issues-steer
Of course some of this is implementable via prompts; just tell the review agent (for example) to check for changes since it started (though this complicates "what it reviewed").
There was a problem hiding this comment.
ref https://github.github.com/gh-aw/reference/concurrency/#per-workflow-concurrency - the compiler seems to tune some defaults around this
There was a problem hiding this comment.
I do not think we should expose this to users like gh-aw does. I think ideally fullsend should work at a higher level, users sohuld be concerned with that they want the agent to do, not the details of when exactly it gets triggered - whice may be slightly different across platforms but should result in a similar user-facing behaviour.
There was a problem hiding this comment.
expose this to users
"users" = humans? I think you have a mindset that humans will be writing the fullsend BYO agent YAML by hand. That seems unlikely to be true broadly.
Again I think GH-AW has done a good job in this because there's lots of examples that both humans and agents can reference/fork - but an especially good idea here is the compiler - agents (and humans!) are much more effective with tooling to check their output quickly and reliably.
I am not seeing a choice on concurrency that would make sense for all workflows.
There was a problem hiding this comment.
I want to make an opinionated educated choice here - agent instances derived from the same harness file (line in the config) never run in parallel against the same target entity - that simplified the mental model for a lot of things. I would be willing to revise this in a future ADR given concrete examples were different behaviour is crucial.
There was a problem hiding this comment.
against the same target entity
I think the tricky thing is that while Fullsend's current builtin agents have a pretty hardcoded concept of "target entity", there are valid agentic flows that do not; https://github.github.com/gh-aw/patterns/workqueue-ops/ is one example.
Basically this arises with actions that are scheduled.
Though of course triggering based on time can have real problems too, and it can often make sense to ensure that any pending work is automatically enqueued by event-based triggers.
For example, fullsend's retro agent runs once per PR today, but it could actually make sense for a PR to add a checklist item to a queued retro, and have the retro agent actually run against multiple PRs as a group so it gets a more holistic view - that could be schedule based but would ideally deterministically no-op if there was no work to do.
There was a problem hiding this comment.
Wanting to implement this kind of pattern to facilitate automated recovery from missed events as well as enable flows like staleness detection, in a way that pust less burden on the agent implementers is exactly why I'm proposing this change and other related ones.
I think this discussion in not really productive at this point. You seem o be making an argument for theoretical agents that do not exist while I'm trying to make changes to significantly improve the current set of agent and remove major automation road blocks from the existing workflow.
We can always expand the API and add lowe level primitives if use cases show up, removing stuff once its out there is much harder. So I'll keep the API narrow for now and focused on wnat we ave concrete use cases for.
There was a problem hiding this comment.
Sure, I think that's a reasonable position.
You seem o be making an argument for theoretical agents that do not exist
Kind of. What I really like just to emphasize is that core model with GH-AW that agent invocations are just action runs and so existing verbs like concurrency which are already well-understood and known apply.
(To be clear my preferred outcome is still that we target Tekton as a baseline, which would drop the abstractions and mean in the end we control the flow much more e2e)
| compatible groups and cancellation settings. Integrations for platforms without | ||
| equivalent semantics MUST emulate them outside the agent execution process. |
There was a problem hiding this comment.
Integrations for platforms without equivalent semantics MUST emulate them outside the agent execution process.
My instinct says that's going to get hard fast without going with my suggestion of actually always executing in e.g. Tekton (Konflux) which then becomes the baseline target...
There was a problem hiding this comment.
We can still aspire to this, and write another ADR explaining why this cannot be done on a particular platform once we actually try to implement it.
| Each agent run MUST reconcile the subject's current state rather than assume the | ||
| triggering event describes all outstanding work. The retained event may still |
There was a problem hiding this comment.
Again, the thing I like about GH-AW is it's a very clear framework.
Each agent run MUST reconcile the subject's current state
Do we really need that for all agents? I don't think so.
Also it could mean a review agent on an active PR being discussed spends a long time looping and perhaps gets timed out and killed.
There was a problem hiding this comment.
I think we really want this - I think focusing too much on events is a consistent design issue we keep introducing into CI systems, and keep avoiding to fix because it becomes too expensive once we realize it was the wrong desing choice.
WRT review timing out - its really up to us to avoid that kind of bug, and balance retries with execution time - nothing in the ADR sais otherwise. The only thing this ADR says is that if there are 10 comment/CI results on a PR - it best to launch review once to deal with them all rather then 10 times.
There was a problem hiding this comment.
Yeah - the event-oriented (event obsessed?) nature of things like Konflux and Tekton are a problem. When things flake, there's nowhere in that model that makes it easy or obvious how to recover later.
The kube controller model, on the other hand, handles this well. The controller tracks a kind of resource and an event-like mechanism functions as a performance optimization to prompt it to respond, but in the model it is oriented on the entity, on the resource and its state. If things flake, reconciling in the next pass is obvious.
There was a problem hiding this comment.
OMG comparing this to the controller model is brilliant! That probably gives us the right mental framework to document this in a way that would be most productive!
There was a problem hiding this comment.
Yeah - the event-oriented (event obsessed?) nature of things like Konflux and Tekton are a problem.
While you know this, I think it's really worth spelling out that that behavior is more specific to PaC. It's absolutely possible to skip PaC as e.g. rhel-on-gitlab does, and implement something more like a "controller" there that triggers a pipeline possibly multiple times, especially on push events - the reconciliation loop there is clear: "is there a successful pipeline corresponding to the current git HEAD".
(Also the way PaC spams comments on gitlab is just absolutely awful)
There was a problem hiding this comment.
The controller tracks a kind of resource and an event-like mechanism functions as a performance optimization to prompt it to respond, but in the model it is oriented on the entity, on the resource and its state.
I would just link https://kubernetes.io/docs/concepts/architecture/controller/ which explains this pretty clearly, and I think concrete important terms to use here are "spec" vs "status" among others.
ADR 0098 (#6909) is open and under review, and it and this change were written independently against the same feedback. Saying the dispatch half here is 0098 implemented claims an alignment nobody has established. State the shared first move, that the two were not designed against each other, and that they should be reconciled where they overlap. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0098 (#6909) is open and under review, and it and this change were written independently against the same feedback. Saying the dispatch half here is 0098 implemented claims an alignment nobody has established. State the shared first move, that the two were not designed against each other, and that they should be reconciled where they overlap. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0098 (#6909) is open and under review, and it and this change were written independently against the same feedback. Saying the dispatch half here is 0098 implemented claims an alignment nobody has established. State the shared first move, that the two were not designed against each other, and that they should be reconciled where they overlap. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0098 (#6909) is open and under review, and it and this change were written independently against the same feedback. Saying the dispatch half here is 0098 implemented claims an alignment nobody has established. State the shared first move, that the two were not designed against each other, and that they should be reconciled where they overlap. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0098 (#6909) is open and under review, and it and this change were written independently against the same feedback. Saying the dispatch half here is 0098 implemented claims an alignment nobody has established. State the shared first move, that the two were not designed against each other, and that they should be reconciled where they overlap. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0098 (#6909) is open and under review, and it and this change were written independently against the same feedback. Saying the dispatch half here is 0098 implemented claims an alignment nobody has established. State the shared first move, that the two were not designed against each other, and that they should be reconciled where they overlap. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0098 (#6909) is open and under review, and it and this change were written independently against the same feedback. Saying the dispatch half here is 0098 implemented claims an alignment nobody has established. State the shared first move, that the two were not designed against each other, and that they should be reconciled where they overlap. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
Thinking about the layering here, I wonder if ADR 0098 (this coalescing one) would be clearer after ADR 0106 (the entity-first one), both numerically and conceptually. Entity-first evaluation establishes the current-state reconciliation model; coalescing then becomes an optimization on top of that framework. Consider renumbering this to follow ADR 0106 (the entity-first one) and simplifying some of the text here by referring back to it directly. Like Greg brought up in slack, I’d also like to see GitLab as a first-class consideration. The policy is forge-agnostic, but the implementation discussion is currently anchored in GitHub Actions concurrency (cancel-in-progress: false and one pending run). Please include how GitLab provides or would emulate the same invariant, along with any platform-specific gaps. |
3 participants
Summary
fullsend dispatch;fullsend runexecutes one run and does not poll or schedule follow-ups.cancel-in-progress: false, plus the need for equivalent semantics on other execution platforms.Accepted ADR annotations
cancel-in-progressdiscussions as replaced by ADR 0098 while retaining source locks and idempotency as defense in depth.Living documentation
docs/architecture.mdwith the platform-managed coalescing and state-reconciliation model.Verification
make lint