feat(dispatch): let a repository preserve the agent run already in flight - #7007
feat(dispatch): let a repository preserve the agent run already in flight#7007waynesun09 wants to merge 9 commits into
Conversation
PR Summary by QodoOptionally preserve in-flight agent stage runs
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
|
🤖 Review · ❌ Terminated · Started 2:21 PM UTC · Ended 3:03 PM UTC Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Site previewPreview: https://19b2668e-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 2:21 PM UTC · Completed 3:03 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.04 |
|
🤖 Review · Commit: |
e56312e to
9085c4d
Compare
|
🤖 Finished Review · ✅ Success · Started 3:30 PM UTC · Completed 3:52 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.53 |
|
Risk Assessment: elevated (3/5) DetailsRe-review anchoring preserves the prior score of 3 (elevated): Tier 1 signals are unchanged (CI workflow modified, one protected path, moderate-large change size, decent test ratio), Tier 2 confirms run.go and reusable-dispatch.yml remain extreme churn hotspots under heavy multi-author contention, and Tier 3 retains a rollback-safety concern from the behavioral concurrency toggle — no articulable reason exists to deviate from the prior elevated assessment. Previous runRisk Assessment: elevated (3/5) DetailsTier 1 signals are unchanged from the prior assessment (CI workflow modified, one protected path, moderate-large change size, decent test ratio); Tier 2 confirms run.go and reusable-dispatch.yml are extreme churn hotspots under heavy multi-author contention, and Tier 3 flags a rollback-safety concern from the behavioral concurrency toggle change -- all consistent with the prior score of 3 (elevated), which is preserved under re-review anchoring. Previous run (2)Risk Assessment: elevated (3/5) DetailsPR modifies critical workflow infrastructure (reusable-dispatch.yml) and core CLI logic (run.go) with CI workflow changes and one protected path file. Elevated by active development on run.go and the behavioral nature of the concurrency toggle change. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
|
![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.
fullsend-ai-review
Bot
added
the
requires-manual-review
9085c4d to
c6c05f8
Compare
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 7:17 PM UTC · Completed 7:59 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
Every stage job cancels the run working on a work item as soon as a second event arrives for it. The replacement pays sandbox provisioning and bootstrap before the model reads anything, then re-reads the whole item from cold, so a burst of pushes discards finished work and buys nothing: on #6513 six force-pushes produced five completed reviews of commits that were superseded within minutes. FULLSEND_PRESERVE_RUNS lets a repository choose otherwise. Unset — the default everywhere — is exactly today's behaviour. Set to "true", the run in flight finishes and the newer event waits as the single pending run, which then works from the item's current state. The agent's side of that bargain, reconciling current state rather than the state that dispatched it, is fullsend-ai/agents#1163, and is inert until the run facts in the next commit reach the sandbox. Refs #6957 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com> The alignment test decoded cancel-in-progress as a Go bool, which an expression string cannot unmarshal into, so it moves to a yaml.Node and asserts the exact expression every stage job must carry. Signed-off-by: Wayne Sun <gsun@redhat.com>
Once a repository preserves the run in flight, that run can outlive the
state it was dispatched on, so an agent has to be able to tell what
changed underneath it before it writes its result. Export the two facts
it needs: the work item's head at run start, and the instant the run
started.
They go through bootstrapEnv rather than env.sandbox or an env/*.env
file, because .env.d files are sourced afterwards and would expand ${VAR}
host-side to an empty string, and a ${VAR} in harness env.sandbox
hard-fails ValidateRunnerEnvWith for every consumer that does not define
it. Both are reserved so a harness cannot shadow them.
An issue run exports an empty head rather than omitting the variable: the
agent side skips its re-check on an empty value, and absence and emptiness
would otherwise be indistinguishable. The consumer is
fullsend-ai/agents#1163.
Refs #6957
Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
The concurrency rules describe workflow-level choices; the stage jobs in reusable-dispatch.yml now defer that one to the repository. Say so where a contributor reads the rules, and say that the expression must stay identical across stage jobs, since a mixed setting would let one role cancel while another queues on the same work item. Refs #6957 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
reservedSandboxKeys stops an env.sandbox entry shadowing FULLSEND_RUN_HEAD_SHA and FULLSEND_RUN_STARTED_AT, but it says nothing about .env.d, which bootstrapEnv sources after writing them — so a harness host_files env file could overwrite either value and hand the agent a fabricated baseline. Position in the generated script is what actually protects them, which is why the ADR 0055 env.sandbox block already sits after that line for the same reason. The run facts now sit beside it. Since the ordering is behaviour rather than formatting, the line assembly moves into buildEnvScriptLines so a test can pin it; bootstrapEnv keeps the file write and upload, and host_files copying moves to uploadHostFiles. No behaviour change beyond the ordering. This does not close every route: a host_files entry whose dest is the runner's own .env replaces the file wholesale, and the same exposure applies to FULLSEND_ROLE and FULLSEND_SLUG, which are written four lines above. That gap is repo-wide and predates this change, so it is tracked separately rather than widened into this one. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0063 names per-stage cancel-in-progress as the second of two layers protecting poll dispatch from duplicate side effects. Setting FULLSEND_PRESERVE_RUNS removes that layer, leaving the poller's own lock and whatever idempotency the agent has — and the duplicate-poll case is where the trade is least favourable, because two duplicate dispatches are the same work rather than a newer state superseding an older one, so both run to completion instead of one replacing the other. Recorded where someone deciding will read it: the workflow's concurrency comment, and a cross-reference note on ADR 0063 itself, which its rules allow without rewriting an accepted decision. The default is unchanged, so that ADR's assumption still holds wherever the variable is unset. Deliberately documentation and not a code guard, and deliberately not an exemption for harness-run: all seven stage jobs stay consistent, because a job that quietly ignored the variable would be harder to reason about than the trade itself. The repository owner makes this call; our part is to make sure they make it knowing what they give up. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The concurrency test iterated six built-in stages while the workflow has seven jobs carrying the expression, so harness-run — the matrix fan-out — was unpinned and a change to it would not have been caught. It is in the map now, keyed by matrix identity rather than the event payload because a poller supplies its work item, and the failure message says that a new stage job has to be added there or its concurrency is unguarded. Converting CancelInProgress from bool to yaml.Node also lost a property nobody asked to give up: Node.Value is "true" for both a YAML boolean and the quoted string "true", and only Tag separates them, so the three literal sites would have accepted a string and the expression site would have accepted a bare boolean. Each now asserts Tag alongside Value. Both were checked against tampered fixtures rather than assumed: setting harness-run back to a literal true fails on harness-run, and quoting a reusable workflow's cancel-in-progress fails on the type. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The variable was described only in the contributor CI guide. A repository administrator deciding whether to set it reads the operations guide, which already carries the repo-variable table it belongs in. The value semantics are worth stating rather than leaving to be discovered: GitHub compares strings case-insensitively, so TRUE and True also preserve, while any other value — including 1 and yes, which someone setting a boolean flag might reasonably try — cancels. The row points at the CI guide for what preserving gives up. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
runStartedAt was taken after harness resolution, base composition, token minting and env expansion — a mint call retries over the network — so activity on the work item during setup was exported to the agent as predating the run, and a preserved run could miss an update it was supposed to reconcile against. Moving the capture to the top of runAgent shrinks that window to nothing within this process, which is the whole fix available here. It is not the honest baseline and the comment says so: the run was dispatched before the process started, so the true start is the workflow run's server-side created_at, which no host clock can reach and which costs an API call to read. That is deliberately not added here, but it is recorded so the two halves do not end up disagreeing about what "run start" means — the follow-up run watcher reached the same conclusion and already uses the run record's created_at rather than its own clock. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
c6c05f8 to
6aea370
Compare
|
🤖 Finished Review · ✅ Success · Started 10:29 PM UTC · Completed 10:54 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.65 |
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.
fullsend-ai-review
Bot
removed
the
requires-manual-review
Four review findings on the base half. **The GitLab head was empty on most merge request pipelines.** runHeadSHA returned CI_MERGE_REQUEST_SOURCE_BRANCH_SHA with no fallback, and GitLab's predefined-variables reference says of it: "The variable is empty in merge request pipelines. The SHA is present only in merged results pipelines." So an ordinary MR pipeline exported an empty head — the signal reserved for issue runs — and the agent could not tell the branch had moved underneath it, which is the one capability the run facts exist to provide. CI_COMMIT_SHA fills the gap but only conditionally: in a merged results pipeline it is the merge-result commit rather than the source head, so an unconditional fallback would export the wrong SHA there. Guarding on CI_PIPELINE_SOURCE is self-correcting, because in a merged results pipeline the first variable is populated and the fallback is never reached. A pipeline that is not a merge request still reports no head, which is correct. Tests cover all three shapes; only the first had one. **The ordering test did not pin the ordering.** It asserted the run facts follow .env.d, but not that the iteration source line is last — so a change moving iteration sourcing above .env.d would have satisfied every existing assertion while letting a harness env file overwrite the budget and deadline that #7042 put there. That is the conflict we resolved by hand a few hours ago, unguarded. The whole order is now asserted, including that the final element is iterationEnvSourceLine(). **The operator link promised an explanation that was not there.** The operations guide sends an administrator to ci-workflows.md "for what preserving gives up", and that page described only cancel-versus-queue. It now names the trade: ADR 0063 counts per-stage cancellation among its duplicate-dispatch mitigations, and preserving leaves the poller's lock and agent idempotency. **And one asymmetry:** the deprecated thin-caller assertion checked Value without Tag, so quoting that literal would have escaped it while the seven stage jobs caught it. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 4:39 PM UTC · Completed 4:59 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.62 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
.github/workflows/reusable-dispatch.yml(file-level): Line 47 · [low] scope-authorization-premature
The FULLSEND_PRESERVE_RUNS opt-in ships while ADR 0098 remains open. Since the prior review, ci-workflows.md has been substantially extended with explicit guidance on what preserving gives up (ADR 0063 layer removal, duplicate-poll hazard), and the PR body explicitly disavows being ADR 0098's implementation.
internal/cli/run.go(file-level): Line 169 · [low] architecture-coherence
FULLSEND_RUN_STARTED_AT captures a host-side clock value, not the workflow run's server-side created_at. The two halves of the preserve-and-reconcile system will reference different absolute timestamps. The risk is conservative — the window of undetected updates is bounded and small — but the architectural split is not spelled out in the issue or any ADR.
| @@ -0,0 +1,165 @@ | |||
| package cli | |||
There was a problem hiding this comment.
[low] naming-convention
The filename runfacts_test.go does not follow the established topic-split naming convention for run.go tests. Existing peers use run__test.go (run_models_aliases_test.go, run_openai_test.go, run_overrides_test.go) or _run_test.go (prescript_run_test.go, telemetry_run_test.go).
Suggested fix: Rename to run_facts_test.go to align with the run__test.go pattern.
| // After .env.d, for the same reason env.sandbox is: a file sourced from | ||
| // .env.d would otherwise overwrite these. reservedSandboxKeys stops an | ||
| // env.sandbox entry shadowing them, but it says nothing about .env.d, so | ||
| // position is what actually protects them. It does not close every route — |
There was a problem hiding this comment.
[low] defense-in-depth
Pre-existing gap acknowledged in the PR: a host_files entry whose destination is the runner's own .env file can replace it wholesale, bypassing both reservedSandboxKeys and ordering-based protection for the new run facts. This gap is repo-wide (not introduced by this PR) and is tracked in #7010.
1 participant
Summary
Every agent stage job cancels the run working on a work item as soon as a second event arrives for it. This lets a repository choose otherwise: the run in flight finishes, and the newer event waits as the single pending run GitHub Actions allows, which then works from the item's current state.
Default behaviour is unchanged.
FULLSEND_PRESERVE_RUNSunset, which is everywhere today, still cancels.Scope
In: the concurrency expression on the seven
reusable-dispatch.ymlstage jobs, and the two run facts an agent needs to tell what changed underneath it (FULLSEND_RUN_HEAD_SHA,FULLSEND_RUN_STARTED_AT), exported throughbootstrapEnvafter.env.dis sourced so a harness env file cannot replace them. Note they are not fully protected:reservedSandboxKeysguardsenv.sandboxonly, and an unvalidatedhost_filesdestination can still overwrite the runner's env file. That gap is pre-existing and repo-wide —FULLSEND_ROLEandFULLSEND_SLUGsit above the same line — and is tracked separately in #7010 rather than fixed here.Not in, deliberately: anything that talks to a running agent. No watcher, no receipt, no
/fs-steer, no runtime changes. When nothing steers, the run queued behind simply does the work, so there is nothing to skip and no receipt to get wrong. That half is #6959, which is stacked on this branch and is not a prerequisite for this one.The other half of the bargain is fullsend-ai/agents#1163: once the run in flight is preserved, it can outlive the state it was dispatched on, so the agent re-checks the work item once before writing its result. It skips that check when these variables are empty, so the two merge in either order.
Why
A restart is not only tokens. It pays sandbox provisioning and bootstrap before the model reads anything, then re-reads the whole item from cold. On #6513, six force-pushes produced five completed reviews of commits that were superseded within minutes, none of which led to a change.
Relation to ADR 0098 (#6909)
That ADR proposes preserve-and-coalesce as policy. It is open, and it and this change were written in parallel against the same feedback rather than against each other, so this is not offered as its implementation. If it is accepted, the natural follow-up is to drop the variable and make the behaviour unconditional.
Testing
make lintpasses (pre-commit over the full range)runfacts_test.gocovers the exports, UTC normalisation, quote escaping, the empty head on an issue run, and thatGITHUB_SHAis never used as a fallbackEvery commit builds and vets on its own.
Checklist
Refs #6957
Review round
A Codex gpt-5.6-sol pass (one reviewer, not the full squad) produced five findings, all confirmed and addressed:
.env.d; they now export below that line, beside theenv.sandboxblock that already sits there for the same reason. The general reserved-key gap is reservedSandboxKeys only guards env.sandbox, not .env.d or host_files dest #7010.cancel-in-progressduplicate-dispatch mitigation that accepted ADR 0063 relies on, leaving the Jira lock and agent idempotency. The duplicate-poll case is where preserving is least defensible, because two duplicate dispatches are the same work rather than a newer state superseding an older one. All seven jobs stay consistent; an exemption forharness-runwas considered and rejected, since a job quietly ignoring the variable is harder to reason about than the trade.FULLSEND_PRESERVE_RUNSis documented for operators in the repo-variable table, including that the comparison is case-insensitive:TRUEandTruepreserve, while1andyescancel.created_atrather than any host clock inside the process, which is noted in the code.The export ordering is now load-bearing, so
bootstrapEnv's script assembly was split into a testable function and pinned by a test, rather than being asserted only by a comment.Rebased onto current main.
functional-testswas red here for a reason outside this change: main allowlists**/claude.exeon the Vertex egress profile and this branch predated that, so OPA denied the agent binary and every triage eval case failed at zero cost. The rebase picks that fix up. Thee2ered is separate and unrelated, a pool-repo race inTestAdminInstallUninstall(422 expected head sha didn't match current head ref).Rebased onto current main after this PR went
DIRTY. The conflict was #7042, which exports its own runner-owned values into the sandbox and reasons about ordering against harness-controlled entries the same way this PR does. Three conflicts, all ininternal/cli/run.go: the reserved-key map took the union of both pairs of keys;bootstrapEnv's neighbourhood kept #7042's block plus this PR'srunFactstype under one merged signature; and the third was that #7042's iteration-env line and this PR's run-facts export both claimed to be last, for the same stated reason — coming after every harness-controlled entry so nothing can shadow them.They export disjoint variables, so neither shadows the other, but only one can be last. #7042's sources a file rewritten before every iteration, so its position is load-bearing; the run facts only ever needed to be after
.env.d. Final order is.env.d→env.sandbox→ run facts → iteration env source, and both comments now state which invariant they hold rather than both asserting primacy. The ordering test asserts "after.env.d" rather than "last", so it survives the reordering while still constraining the property that matters; it was mutation-checked after the rebase, since a replay can quietly make an assertion vacuous.