From 539305e26ac2bb87e2199a2174a6403b7380299f Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:08:58 -0400 Subject: [PATCH 1/8] feat(dispatch): let a repository preserve the run already in flight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 --- .github/workflows/reusable-dispatch.yml | 34 +++++++++++++------ .../scaffold/workflow_call_alignment_test.go | 24 +++++++++---- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index 9fb229c70..fd40af81f 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -14,9 +14,23 @@ # caller-provided matrix. This enables reuse without extracting separate workflows # while preserving ADR 62's inlining decision. # -# Concurrency: each stage job declares a per-role cancel-in-progress group -# (fullsend-{stage}-…). Roles operate independently — review dispatches do -# not cancel triage, code, fix, etc. +# Concurrency: each stage job declares a per-role group (fullsend-{stage}-...). +# Roles operate independently: a review dispatch does not cancel triage, code, +# fix, and so on. +# +# Whether a second event on the same work item cancels the run already working +# on it is the repository's choice, made with the FULLSEND_PRESERVE_RUNS +# variable. Unset, the default everywhere, is the historical behaviour: the +# newer event cancels the run in flight, discarding whatever it had done. +# Set to "true", the run in flight is left to finish and the newer event waits +# as the single pending run GitHub Actions allows, which then works from the +# work item's current state rather than the state that dispatched it. That +# reconciliation is the agent's side of the bargain, and is why the runner +# exports FULLSEND_RUN_HEAD_SHA and FULLSEND_RUN_STARTED_AT. +# +# queue: max is deliberately not used: it is incompatible with +# cancel-in-progress: true, so it cannot be expressed here, and N pending full +# runs is the failure mode preserving the active run exists to remove. # # Flow: shim (per-repo) → reusable-dispatch.yml (route + inlined stage logic) # Or: custom poller → reusable-dispatch.yml (pre-computed matrix → harness-run) @@ -589,7 +603,7 @@ jobs: runs-on: ${{ inputs.runner_image }} concurrency: group: fullsend-triage-${{ github.repository }}-${{ github.event.issue.number || github.event.pull_request.number }} - cancel-in-progress: true + cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }} permissions: actions: write contents: read @@ -698,7 +712,7 @@ jobs: runs-on: ${{ inputs.runner_image }} concurrency: group: fullsend-code-${{ github.repository }}-${{ github.event.issue.number || github.event.pull_request.number }} - cancel-in-progress: true + cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }} permissions: actions: write contents: write @@ -837,7 +851,7 @@ jobs: runs-on: ${{ inputs.runner_image }} concurrency: group: fullsend-review-${{ github.repository }}-${{ github.event.pull_request.number || github.event.issue.number }} - cancel-in-progress: true + cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }} permissions: actions: write contents: read @@ -965,7 +979,7 @@ jobs: runs-on: ${{ inputs.runner_image }} concurrency: group: fullsend-fix-${{ github.repository }}-${{ github.event.pull_request.number || github.event.issue.number }} - cancel-in-progress: true + cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }} permissions: actions: write contents: write @@ -1245,7 +1259,7 @@ jobs: runs-on: ${{ inputs.runner_image }} concurrency: group: fullsend-retro-${{ github.repository }}-${{ github.event.pull_request.number || github.event.issue.number }} - cancel-in-progress: true + cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }} permissions: actions: write contents: read @@ -1355,7 +1369,7 @@ jobs: runs-on: ${{ inputs.runner_image }} concurrency: group: fullsend-prioritize-${{ github.repository }}-${{ github.event.issue.number || github.event.pull_request.number }} - cancel-in-progress: true + cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }} permissions: actions: write contents: read @@ -1610,7 +1624,7 @@ jobs: matrix: ${{ fromJSON(inputs.matrix != '' && inputs.matrix || (needs.harness-dispatch.outputs.matrix != '' && needs.harness-dispatch.outputs.matrix || '{"include":[]}')) }} concurrency: group: fullsend-harness-${{ matrix.agent }}-${{ github.repository }}-${{ matrix.status_repo }}-${{ matrix.status_number }} - cancel-in-progress: true + cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }} runs-on: ${{ inputs.runner_image }} permissions: actions: write diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 43bf064e6..9f615a322 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -48,10 +48,19 @@ type callerJob struct { } type jobConcurrency struct { - Group string `yaml:"group"` - CancelInProgress bool `yaml:"cancel-in-progress"` + Group string `yaml:"group"` + // CancelInProgress is a Node, not a bool: reusable-dispatch.yml stage + // jobs gate it on a repository variable, so the value is an expression + // string on those jobs and a literal boolean everywhere else. + CancelInProgress yaml.Node `yaml:"cancel-in-progress"` } +// preserveGatedCancel is the cancel-in-progress value every +// reusable-dispatch stage job must carry: today's cancelling behaviour +// unless the consumer repository sets FULLSEND_PRESERVE_RUNS=true, in which +// case the run in flight is left to finish. +const preserveGatedCancel = "${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }}" + // reusableStageWorkflow includes workflow-level concurrency on reusable agent workflows. type reusableStageWorkflow struct { Concurrency *jobConcurrency `yaml:"concurrency"` @@ -497,8 +506,9 @@ func TestReusableDispatchStageConcurrency(t *testing.T) { assert.Contains(t, job.Concurrency.Group, fragment, "job %q concurrency group should reference %q", stage, fragment) } - assert.True(t, job.Concurrency.CancelInProgress, - "job %q should cancel in-progress runs when a newer dispatch arrives", stage) + assert.Equal(t, preserveGatedCancel, job.Concurrency.CancelInProgress.Value, + "job %q must cancel in-progress runs by default and stop cancelling "+ + "only when the consumer sets FULLSEND_PRESERVE_RUNS=true", stage) }) } } @@ -521,7 +531,7 @@ func TestReusableAgentWorkflowConcurrency(t *testing.T) { assert.Contains(t, wf.Concurrency.Group, fragment, "reusable-%s.yml concurrency group should reference %q", stage, fragment) } - assert.True(t, wf.Concurrency.CancelInProgress, + assert.Equal(t, "true", wf.Concurrency.CancelInProgress.Value, "reusable-%s.yml should cancel in-progress runs", stage) callerExpect := thinCallerConcurrencyExpectations[stage] @@ -549,7 +559,7 @@ func TestThinCallerStageConcurrency(t *testing.T) { assert.Contains(t, wf.Concurrency.Group, fragment, "%s concurrency group should reference %q", path, fragment) } - assert.True(t, wf.Concurrency.CancelInProgress, + assert.Equal(t, "true", wf.Concurrency.CancelInProgress.Value, "%s should cancel in-progress runs when a newer dispatch arrives", path) }) } @@ -1048,7 +1058,7 @@ func TestShimLabeledEventFiltering(t *testing.T) { `fullsend-dispatch-\$\{\{\s*github\.event\.issue\.number\s*\|\|\s*github\.event\.pull_request\.number\s*\}\}-\$\{\{\s*github\.event\.action\s*==\s*'labeled'\s*&&\s*format\('label-\{0\}',\s*github\.event\.label\.name\)\s*\|\|\s*'dispatch'\s*\}\}`, job.Concurrency.Group, "%s concurrency group must match full label-aware structure", tc.name) - assert.False(t, job.Concurrency.CancelInProgress, + assert.Equal(t, "false", job.Concurrency.CancelInProgress.Value, "%s concurrency group must have cancel-in-progress: false", tc.name) }) } From 899825a0f13399b948930c6b1ed0c24d93d9949c Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:08:58 -0400 Subject: [PATCH 2/8] feat(run): export this run's baseline into the sandbox 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 --- internal/cli/run.go | 58 +++++++++++++++++++++++- internal/cli/run_test.go | 8 ++-- internal/cli/runfacts_test.go | 85 +++++++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 internal/cli/runfacts_test.go diff --git a/internal/cli/run.go b/internal/cli/run.go index 918c92ca4..637d385dd 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -1173,6 +1173,10 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // final values for the completion comment footer. var aggMetrics aggregateMetrics + // The instant this run started, exported into the sandbox so the agent + // can tell which activity on the work item postdates it. + runStartedAt := time.Now().UTC() + // 1c. Set up status notifications (comments on the issue/PR). // Lives in the CLI layer (not harness or post-script) so it wraps the // entire run lifecycle including sandbox setup, validation loop, and @@ -1884,7 +1888,8 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep printer.StepFail("Failed to bootstrap sandbox") return err } - if err := bootstrapEnv(sandboxName, remoteRepositoryDir, h, rt.EnvExports(), fetchEnvVal); err != nil { + if err := bootstrapEnv(sandboxName, remoteRepositoryDir, h, rt.EnvExports(), + runFacts{headSHA: runHeadSHA(forgePlatform), startedAt: runStartedAt}, fetchEnvVal); err != nil { printer.StepFail("Failed to bootstrap sandbox") return err } @@ -2777,6 +2782,8 @@ var reservedSandboxKeys = map[string]bool{ "FULLSEND_SLUG": true, "FULLSEND_TIMEOUT_MINUTES": true, "FULLSEND_ITERATION_DEADLINE": true, + "FULLSEND_RUN_HEAD_SHA": true, + "FULLSEND_RUN_STARTED_AT": true, // OPENAI_API_KEY is reserved through oidcDenyKeys (merged by init()). } @@ -2932,7 +2939,19 @@ func runTerminalError(hasLoop, validationPassed, timedOut bool, runCount int, el return nil } -func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, runtimeEnvExports []string, fetchEnv ...fetchServiceEnv) error { +// runFacts is what the work item looked like when this run started. It is +// exported into the sandbox so an agent can tell, before it writes its +// result, whether the item moved under it — which it must do once a +// repository sets FULLSEND_PRESERVE_RUNS, because the run in flight is then +// no longer cancelled when a newer event arrives. +type runFacts struct { + // headSHA is the work item's head at run start; empty for issues. + headSHA string + // startedAt is when the run started, in UTC. + startedAt time.Time +} + +func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, runtimeEnvExports []string, facts runFacts, fetchEnv ...fetchServiceEnv) error { remoteEnvFile := sandbox.SandboxWorkspace + "/.env" outputDir := sandbox.SandboxWorkspace + "/output" @@ -2953,6 +2972,10 @@ func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, r // without hardcoding values that drift from the harness YAML. See #6045. lines = append(lines, buildRoleSlugEnvLines(h)...) + // Expose this run's baseline so the agent can re-check, once, whether the + // work item moved under it before it writes its result. + lines = append(lines, buildRunFactsEnvLines(facts)...) + // Expose output schema and expected filename inside the sandbox so // agents can self-check output with fullsend-check-output. See #1107. // Prefer validation_loop.schema (already resolved by compose); fall @@ -4887,6 +4910,37 @@ func extractMapString(m map[string]any, keys ...string) string { return "" } +// runHeadSHA returns the work item's head at run start, or "" when the run +// is not against a pull or merge request. +// +// GITHUB_SHA is deliberately not a fallback: on pull_request_target it is the +// base branch, and a base SHA presented as the head would make an agent +// report a head move on every run. +func runHeadSHA(forgePlatform string) string { + if forgePlatform == "gitlab" { + return os.Getenv("CI_MERGE_REQUEST_SOURCE_BRANCH_SHA") + } + if sha := os.Getenv("PR_HEAD_SHA"); sha != "" { + return sha + } + return prHeadSHAFromEventPath(os.Getenv("GITHUB_EVENT_PATH")) +} + +// buildRunFactsEnvLines exports this run's baseline into the sandbox. +// +// They are written here rather than through env.sandbox or an env/*.env file: +// .env.d files are sourced after this one and would expand ${VAR} host-side to +// an empty string, and a ${VAR} in harness env.sandbox hard-fails +// ValidateRunnerEnvWith for consumers that do not define it. +func buildRunFactsEnvLines(facts runFacts) []string { + return []string{ + fmt.Sprintf("export FULLSEND_RUN_STARTED_AT='%s'", facts.startedAt.UTC().Format(time.RFC3339)), + // The SHA is forge-supplied; single-quote it the way every other + // exported value here is, rather than trusting its shape. + fmt.Sprintf("export FULLSEND_RUN_HEAD_SHA='%s'", strings.ReplaceAll(facts.headSHA, "'", "'\\''")), + } +} + // prHeadSHAFromEventPath extracts pull_request.head.sha from the event // payload embedded in a workflow_dispatch event file. For workflow_dispatch // events, the file contains {"inputs": {"event_payload": ""}}. diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index c4300cf50..571477464 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -3536,7 +3536,7 @@ func TestBootstrapEnv_IncludesFetchServiceVars(t *testing.T) { h := &harness.Harness{Agent: "agents/test.md"} fEnv := fetchServiceEnv{addr: "127.0.0.1:54321", token: "deadbeef"} - err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil, fEnv) + err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil, runFacts{}, fEnv) // Expected to fail at sandbox.UploadFile — we just verify the fetch // env var code path was reached (coverage) and the error is from upload. @@ -3547,7 +3547,7 @@ func TestBootstrapEnv_IncludesFetchServiceVars(t *testing.T) { func TestBootstrapEnv_SkipsFetchVarsWhenEmpty(t *testing.T) { h := &harness.Harness{Agent: "agents/test.md"} - err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil) + err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil, runFacts{}) require.Error(t, err) assert.Contains(t, err.Error(), "copying .env file to sandbox") @@ -3569,7 +3569,7 @@ func TestBootstrapEnv_ValidationLoopSchemaPrecedence(t *testing.T) { }, } - err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil) + err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil, runFacts{}) // Expected to fail at sandbox operations — the schema code path is // exercised before the failure. @@ -3584,7 +3584,7 @@ func TestBootstrapEnv_ValidationLoopSchemaFallback(t *testing.T) { }, } - err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil) + err := bootstrapEnv("nonexistent-sandbox", "/workspace/repo", h, nil, runFacts{}) require.Error(t, err) assert.Contains(t, err.Error(), "copying .env file to sandbox") diff --git a/internal/cli/runfacts_test.go b/internal/cli/runfacts_test.go new file mode 100644 index 000000000..609191729 --- /dev/null +++ b/internal/cli/runfacts_test.go @@ -0,0 +1,85 @@ +package cli + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBuildRunFactsEnvLines(t *testing.T) { + at := time.Date(2026, 9, 4, 8, 30, 0, 0, time.UTC) + + t.Run("exports both facts", func(t *testing.T) { + lines := buildRunFactsEnvLines(runFacts{headSHA: "abc123", startedAt: at}) + assert.Equal(t, []string{ + "export FULLSEND_RUN_STARTED_AT='2026-09-04T08:30:00Z'", + "export FULLSEND_RUN_HEAD_SHA='abc123'", + }, lines) + }) + + t.Run("an issue run exports an empty head rather than omitting it", func(t *testing.T) { + // The agent skips its re-check on an empty value, so the variable + // must exist and be empty rather than be absent. + lines := buildRunFactsEnvLines(runFacts{startedAt: at}) + assert.Contains(t, lines, "export FULLSEND_RUN_HEAD_SHA=''") + }) + + t.Run("the start is normalised to UTC", func(t *testing.T) { + east := time.FixedZone("UTC+5", 5*60*60) + lines := buildRunFactsEnvLines(runFacts{startedAt: at.In(east)}) + assert.Contains(t, lines, "export FULLSEND_RUN_STARTED_AT='2026-09-04T08:30:00Z'") + }) + + t.Run("a quote in the SHA cannot break out of the export", func(t *testing.T) { + lines := buildRunFactsEnvLines(runFacts{headSHA: "a'; rm -rf /; '", startedAt: at}) + assert.Contains(t, lines, `export FULLSEND_RUN_HEAD_SHA='a'\''; rm -rf /; '\'''`) + }) +} + +func TestRunHeadSHA(t *testing.T) { + t.Run("gitlab reads the merge request source sha", func(t *testing.T) { + t.Setenv("CI_MERGE_REQUEST_SOURCE_BRANCH_SHA", "gl123") + assert.Equal(t, "gl123", runHeadSHA("gitlab")) + }) + + t.Run("github prefers the explicit PR_HEAD_SHA", func(t *testing.T) { + t.Setenv("PR_HEAD_SHA", "gh123") + assert.Equal(t, "gh123", runHeadSHA("github")) + }) + + t.Run("github falls back to the dispatched event payload", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "event.json") + require.NoError(t, os.WriteFile(path, + []byte(`{"inputs":{"event_payload":"{\"pull_request\":{\"head\":{\"sha\":\"ev123\"}}}"}}`), 0o600)) + t.Setenv("PR_HEAD_SHA", "") + t.Setenv("GITHUB_EVENT_PATH", path) + assert.Equal(t, "ev123", runHeadSHA("github")) + }) + + t.Run("an issue run has no head", func(t *testing.T) { + t.Setenv("PR_HEAD_SHA", "") + t.Setenv("GITHUB_EVENT_PATH", "") + assert.Empty(t, runHeadSHA("github")) + }) +} + +func TestRunHeadSHA_NeverFallsBackToTheBaseSHA(t *testing.T) { + // On pull_request_target GITHUB_SHA is the base branch. Presenting it as + // the head would make an agent report a head move on every run. + t.Setenv("PR_HEAD_SHA", "") + t.Setenv("GITHUB_EVENT_PATH", "") + t.Setenv("GITHUB_SHA", "base000") + assert.Empty(t, runHeadSHA("github")) +} + +func TestRunFactsKeysAreReserved(t *testing.T) { + // Reserved so a harness env.sandbox entry cannot shadow the baseline the + // agent's re-check depends on. + assert.True(t, reservedSandboxKeys["FULLSEND_RUN_HEAD_SHA"]) + assert.True(t, reservedSandboxKeys["FULLSEND_RUN_STARTED_AT"]) +} From 1fde57d273471190931a353d7250fe67cd93ba60 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:08:58 -0400 Subject: [PATCH 3/8] docs(contributing): record who decides whether an agent run is cancelled 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 --- docs/contributing/ci-workflows.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/contributing/ci-workflows.md b/docs/contributing/ci-workflows.md index 7060b62ad..60bd9a8f5 100644 --- a/docs/contributing/ci-workflows.md +++ b/docs/contributing/ci-workflows.md @@ -26,6 +26,7 @@ Conventions for GitHub Actions workflows under `.github/workflows/`. Follow thes ``` Hybrid workflows that combine `workflow_call` with direct triggers like `pull_request_target` or `push` (e.g., `e2e.yml`, `functional-tests.yml`) may still use `${{ github.workflow }}` in the branch of their concurrency expression that handles direct triggers — the `workflow_call` invocations in these cases come from a thin caller that shares the same concurrency intent. - Never cancel in-progress runs on the default branch (`refs/heads/main`). Gate `cancel-in-progress` when the workflow triggers on `push` to `main`. +- **Agent stage jobs are the repository's choice, not the workflow's.** Every stage job in `reusable-dispatch.yml` reads `cancel-in-progress: ${{ vars.FULLSEND_PRESERVE_RUNS != 'true' }}`. Unset, the default everywhere, keeps the historical behaviour: a newer event on the same work item cancels the run already working on it. Set to `"true"`, that run is left to finish and the newer event waits as the single pending run, which then works from the item's current state. Keep the expression identical across every stage job — a mixed setting would let one role cancel while another queues on the same item, which is harder to reason about than either choice on its own. **Why:** A hardcoded prefix like `my-workflow-${{ github.workflow }}` is redundant — `github.workflow` already resolves to the workflow `name:` field. The duplication creates a confusing group key and wastes characters. The reusable-workflow exception exists because GitHub resolves `github.workflow` from the caller's context, so a reusable workflow using it would share a concurrency group with its caller. From 570c1e9a1192b433fa9468cb1ece7583ae73eda0 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:48:10 -0400 Subject: [PATCH 4/8] fix(run): export the run facts after .env.d is sourced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/cli/run.go | 71 +++++++++++++++++++++++------------ internal/cli/runfacts_test.go | 36 ++++++++++++++++++ 2 files changed, 84 insertions(+), 23 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 637d385dd..395fa69cd 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -2953,6 +2953,35 @@ type runFacts struct { func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, runtimeEnvExports []string, facts runFacts, fetchEnv ...fetchServiceEnv) error { remoteEnvFile := sandbox.SandboxWorkspace + "/.env" + + content := strings.Join(buildEnvScriptLines(sandboxName, remoteRepositoryDir, h, runtimeEnvExports, facts, fetchEnv...), "\n") + "\n" + + tmpFile, err := os.CreateTemp("", "fullsend-env-*.sh") + if err != nil { + return fmt.Errorf("creating temp env file: %w", err) + } + defer os.Remove(tmpFile.Name()) + + if _, err := tmpFile.WriteString(content); err != nil { + tmpFile.Close() + return fmt.Errorf("writing temp env file: %w", err) + } + tmpFile.Close() + + if err := sandbox.UploadFile(sandboxName, tmpFile.Name(), remoteEnvFile); err != nil { + return fmt.Errorf("copying .env file to sandbox: %w", err) + } + + return uploadHostFiles(sandboxName, h) +} + +// buildEnvScriptLines assembles the sandbox .env script. +// +// The ORDER of these lines is behaviour, not formatting: later exports win, +// and .env.d is sourced in the middle, so anything that must outrank a +// harness-supplied env file has to come after it. There is a test pinning +// that for the run facts. +func buildEnvScriptLines(sandboxName, remoteRepositoryDir string, h *harness.Harness, runtimeEnvExports []string, facts runFacts, fetchEnv ...fetchServiceEnv) []string { outputDir := sandbox.SandboxWorkspace + "/output" var lines []string @@ -2972,10 +3001,6 @@ func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, r // without hardcoding values that drift from the harness YAML. See #6045. lines = append(lines, buildRoleSlugEnvLines(h)...) - // Expose this run's baseline so the agent can re-check, once, whether the - // work item moved under it before it writes its result. - lines = append(lines, buildRunFactsEnvLines(facts)...) - // Expose output schema and expected filename inside the sandbox so // agents can self-check output with fullsend-check-output. See #1107. // Prefer validation_loop.schema (already resolved by compose); fall @@ -3022,29 +3047,29 @@ func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, r // overriding a single var from a shared host_files .env file. lines = append(lines, buildSandboxEnvLines(h)...) + // Expose this run's baseline so the agent can re-check, once, whether the + // work item moved under it before it writes its result. + // + // 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 — + // a host_files entry whose dest is the runner's own .env replaces the file + // wholesale — and that gap is repo-wide rather than specific to these two + // keys; see the tracking issue. + lines = append(lines, buildRunFactsEnvLines(facts)...) + // Runner-owned budget and deadline come after every harness-controlled - // entry so none of them can shadow the values (#7042). + // entry so none of them can shadow the values (#7042). This stays the last + // line: unlike the static exports above it sources a file rewritten before + // every iteration, so it must be re-read after everything else. lines = append(lines, iterationEnvSourceLine()) - content := strings.Join(lines, "\n") + "\n" - - tmpFile, err := os.CreateTemp("", "fullsend-env-*.sh") - if err != nil { - return fmt.Errorf("creating temp env file: %w", err) - } - defer os.Remove(tmpFile.Name()) - - if _, err := tmpFile.WriteString(content); err != nil { - tmpFile.Close() - return fmt.Errorf("writing temp env file: %w", err) - } - tmpFile.Close() - - if err := sandbox.UploadFile(sandboxName, tmpFile.Name(), remoteEnvFile); err != nil { - return fmt.Errorf("copying .env file to sandbox: %w", err) - } + return lines +} - // Copy host files into the sandbox. +// uploadHostFiles copies the harness's host_files into the sandbox. +func uploadHostFiles(sandboxName string, h *harness.Harness) error { for _, hf := range h.HostFiles { // Use safeExpandEnv instead of os.ExpandEnv to refuse OIDC // credential vars in host_files src path expansion (#5832). diff --git a/internal/cli/runfacts_test.go b/internal/cli/runfacts_test.go index 609191729..a60b29d4b 100644 --- a/internal/cli/runfacts_test.go +++ b/internal/cli/runfacts_test.go @@ -3,11 +3,14 @@ package cli import ( "os" "path/filepath" + "strings" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/harness" ) func TestBuildRunFactsEnvLines(t *testing.T) { @@ -83,3 +86,36 @@ func TestRunFactsKeysAreReserved(t *testing.T) { assert.True(t, reservedSandboxKeys["FULLSEND_RUN_HEAD_SHA"]) assert.True(t, reservedSandboxKeys["FULLSEND_RUN_STARTED_AT"]) } + +// The run facts must be exported after .env.d is sourced, or a harness +// host_files env file would overwrite them. reservedSandboxKeys stops an +// env.sandbox entry shadowing them but says nothing about .env.d, so +// position in the generated script is what actually protects them. +func TestRunFactsAreExportedAfterEnvDSourcing(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Env: &harness.EnvConfig{Sandbox: map[string]string{"SOME_VAR": "x"}}, + } + lines := buildEnvScriptLines("", "/workspace/repo", h, nil, + runFacts{headSHA: "abc123", startedAt: time.Now()}) + + idx := func(substr string) int { + t.Helper() + for i, l := range lines { + if strings.Contains(l, substr) { + return i + } + } + t.Fatalf("no line containing %q in:\n%s", substr, strings.Join(lines, "\n")) + return -1 + } + + envD := idx("/.env.d/*.env") + sandboxVar := idx("export SOME_VAR=") + headSHA := idx("export FULLSEND_RUN_HEAD_SHA=") + startedAt := idx("export FULLSEND_RUN_STARTED_AT=") + + assert.Greater(t, headSHA, envD, "FULLSEND_RUN_HEAD_SHA must be exported after .env.d is sourced") + assert.Greater(t, startedAt, envD, "FULLSEND_RUN_STARTED_AT must be exported after .env.d is sourced") + assert.Greater(t, headSHA, sandboxVar, "the run facts must also outrank env.sandbox") +} From 0a31be235a1ff1ee91bd26cb192e21b9b20d5a20 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:48:48 -0400 Subject: [PATCH 5/8] docs: say what preserving a run gives up for poll dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/reusable-dispatch.yml | 10 ++++++++++ docs/ADRs/0063-polling-based-work-discovery.md | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index fd40af81f..ae560cc71 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -28,6 +28,16 @@ # reconciliation is the agent's side of the bargain, and is why the runner # exports FULLSEND_RUN_HEAD_SHA and FULLSEND_RUN_STARTED_AT. # +# What preserving gives up: ADR 0063 names per-stage cancel-in-progress as the +# second of two layers protecting poll dispatch from duplicate side effects, +# so a repository that sets FULLSEND_PRESERVE_RUNS is left with the first +# layer alone — the poller's own lock — plus whatever idempotency the agent +# has. The duplicate-poll case is where preserving is least defensible: 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. This is the repository owner's call to make, which is +# why it is a variable and not a guard, but it should be made knowingly. +# # queue: max is deliberately not used: it is incompatible with # cancel-in-progress: true, so it cannot be expressed here, and N pending full # runs is the failure mode preserving the active run exists to remove. diff --git a/docs/ADRs/0063-polling-based-work-discovery.md b/docs/ADRs/0063-polling-based-work-discovery.md index a1e115297..7eed7aedb 100644 --- a/docs/ADRs/0063-polling-based-work-discovery.md +++ b/docs/ADRs/0063-polling-based-work-discovery.md @@ -351,6 +351,14 @@ still be safe to re-run (idempotent or gracefully no-op on repeat) as defense in depth — but polling does not impose a new idempotency requirement beyond what event-driven dispatch already assumes under `cancel-in-progress`. +> **Cross-reference (added later).** Layer 2 above is no longer unconditional. A +> repository may set `FULLSEND_PRESERVE_RUNS=true` so stage jobs stop cancelling +> the run in flight, which removes this layer and leaves the poller's lock and +> agent idempotency. The duplicate-poll case is where that trade is least +> favourable, because two duplicate dispatches are the same work rather than a +> newer state superseding an older one. The default is unchanged and this ADR's +> assumption still holds wherever the variable is unset. + Property keys are namespaced by target repo to avoid collisions when multiple repos poll the same Jira project: From de559ffddbce215a19681dd010bb7dba50c144e0 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:49:43 -0400 Subject: [PATCH 6/8] test(scaffold): pin all seven stage jobs and the scalar types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../scaffold/workflow_call_alignment_test.go | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 9f615a322..d7f2d973f 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -151,6 +151,14 @@ var dispatchStageConcurrencyExpectations = map[string]stageConcurrencyExpectatio groupPrefix: "fullsend-prioritize-", groupMust: []string{"github.repository", "github.event.issue.number", "github.event.pull_request.number"}, }, + // The matrix fan-out is a stage job too, and carries the same + // cancel-in-progress expression, so it is pinned here alongside the six + // built-in stages. Its group is keyed by matrix identity rather than by + // the event payload, because a poller supplies the work item. + "harness-run": { + groupPrefix: "fullsend-harness-", + groupMust: []string{"matrix.agent", "github.repository", "matrix.status_repo", "matrix.status_number"}, + }, } // stepDeclRe matches a YAML step declaration line: " - name: ". @@ -508,7 +516,11 @@ func TestReusableDispatchStageConcurrency(t *testing.T) { } assert.Equal(t, preserveGatedCancel, job.Concurrency.CancelInProgress.Value, "job %q must cancel in-progress runs by default and stop cancelling "+ - "only when the consumer sets FULLSEND_PRESERVE_RUNS=true", stage) + "only when the consumer sets FULLSEND_PRESERVE_RUNS=true. A new stage "+ + "job must be added to dispatchStageConcurrencyExpectations, or its "+ + "concurrency is unpinned", stage) + assert.Equal(t, "!!str", job.Concurrency.CancelInProgress.Tag, + "job %q must carry the gate as an expression, not a literal", stage) }) } } @@ -533,6 +545,9 @@ func TestReusableAgentWorkflowConcurrency(t *testing.T) { } assert.Equal(t, "true", wf.Concurrency.CancelInProgress.Value, "reusable-%s.yml should cancel in-progress runs", stage) + assert.Equal(t, "!!bool", wf.Concurrency.CancelInProgress.Tag, + "reusable-%s.yml cancel-in-progress must stay a literal boolean, "+ + "not the string \"true\" — Node.Value cannot tell them apart", stage) callerExpect := thinCallerConcurrencyExpectations[stage] assert.NotEqual(t, callerExpect.groupPrefix, expect.groupPrefix, @@ -1060,6 +1075,9 @@ func TestShimLabeledEventFiltering(t *testing.T) { "%s concurrency group must match full label-aware structure", tc.name) assert.Equal(t, "false", job.Concurrency.CancelInProgress.Value, "%s concurrency group must have cancel-in-progress: false", tc.name) + assert.Equal(t, "!!bool", job.Concurrency.CancelInProgress.Tag, + "%s cancel-in-progress must stay a literal boolean, not the string "+ + "\"false\" — Node.Value cannot tell them apart", tc.name) }) } From db28ce5e2894dacee13c2b52e194fe51f4a96877 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:51:32 -0400 Subject: [PATCH 7/8] docs: document FULLSEND_PRESERVE_RUNS for the people who set it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/guides/getting-started/operations.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/guides/getting-started/operations.md b/docs/guides/getting-started/operations.md index 4e2e7d8b5..5fd4fdc58 100644 --- a/docs/guides/getting-started/operations.md +++ b/docs/guides/getting-started/operations.md @@ -22,6 +22,7 @@ fullsend github set "$OWNER/$REPO" FULLSEND_GCP_REGION global | Key | Storage Type | Description | Example value | |-----|-------------|-------------|---------------| | `FULLSEND_GCP_REGION` | Repo variable | GCP region for Agent Platform inference | `global` | +| `FULLSEND_PRESERVE_RUNS` | Repo variable | Let the agent run already working on an issue or PR finish when a newer event arrives, instead of cancelling it. The newer event then waits as the single pending run. Unset — the default — cancels, as before. Comparison is case-insensitive, so `TRUE` and `True` also preserve; any other value, including `1` and `yes`, cancels. See [CI workflows](../../contributing/ci-workflows.md) for what preserving gives up | `true` | | `FULLSEND_REVIEW_CLIENT_ID` | Repo variable | OAuth client ID of the review agent's GitHub App (best-effort, auto-set by installer) | `Iv23li1nIorNLIQy6NWK` | | `FULLSEND_GCP_PROJECT_ID` | Repo secret | GCP project ID where Agent Platform is enabled | `my-gcp-project` | | `FULLSEND_GCP_WIF_PROVIDER` | Repo secret | Full WIF provider resource name for OIDC authentication | `projects/123456789/locations/global/...` | From 6aea3707ba4384a71bee98848e6e2d55cc67ecfd Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Fri, 4 Sep 2026 10:51:35 -0400 Subject: [PATCH 8/8] fix(run): capture the run start before the work that precedes it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/cli/run.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 395fa69cd..3f55611a6 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -489,6 +489,20 @@ func newRunCmd() *cobra.Command { } func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRepo, fullsendBinary string, envFiles []string, noPostScript bool, debug string, forgeFlag string, eventFile string, rFlags resolveFlags, sOpts statusOpts, printer *ui.Printer, keepSandbox bool, oFlags runOverrideFlags) (runErr error) { + // Captured first, before harness resolution, token minting and env + // expansion, each of which can take real time — a mint call retries over + // the network. Anything on the work item after this instant is activity + // the agent must reconcile against, and a later capture would classify + // some of it as predating the run. + // + // This is still not the honest baseline. The run was dispatched before + // this process started, so the true start is the workflow run's + // server-side created_at, which costs an API call to read; no host clock + // inside this process can reach it. Recorded here so the two halves do + // not disagree about what "run start" means — the follow-up run watcher + // reached the same conclusion and uses the run record's created_at. + runStartedAt := time.Now().UTC() + printer.Banner(Version()) printer.Blank() printer.Header("Running agent: " + agentName) @@ -1173,10 +1187,6 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // final values for the completion comment footer. var aggMetrics aggregateMetrics - // The instant this run started, exported into the sandbox so the agent - // can tell which activity on the work item postdates it. - runStartedAt := time.Now().UTC() - // 1c. Set up status notifications (comments on the issue/PR). // Lives in the CLI layer (not harness or post-script) so it wraps the // entire run lifecycle including sandbox setup, validation loop, and