diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index 9fb229c708..ae560cc716 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -14,9 +14,33 @@ # 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. +# +# 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. # # Flow: shim (per-repo) → reusable-dispatch.yml (route + inlined stage logic) # Or: custom poller → reusable-dispatch.yml (pre-computed matrix → harness-run) @@ -589,7 +613,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 +722,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 +861,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 +989,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 +1269,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 +1379,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 +1634,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/docs/ADRs/0063-polling-based-work-discovery.md b/docs/ADRs/0063-polling-based-work-discovery.md index a1e115297a..7eed7aedbd 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: diff --git a/docs/contributing/ci-workflows.md b/docs/contributing/ci-workflows.md index 7060b62ad9..60bd9a8f51 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. diff --git a/docs/guides/getting-started/operations.md b/docs/guides/getting-started/operations.md index 4e2e7d8b55..5fd4fdc585 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/...` | diff --git a/internal/cli/run.go b/internal/cli/run.go index 918c92ca48..3f55611a68 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) @@ -1884,7 +1898,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 +2792,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,8 +2949,49 @@ 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" + + 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 @@ -2999,29 +3057,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). @@ -4887,6 +4945,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 c4300cf503..5714774640 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 0000000000..a60b29d4b6 --- /dev/null +++ b/internal/cli/runfacts_test.go @@ -0,0 +1,121 @@ +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) { + 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"]) +} + +// 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") +} diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 43bf064e68..d7f2d973f4 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"` @@ -142,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: ". @@ -497,8 +514,13 @@ 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. 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) }) } } @@ -521,8 +543,11 @@ 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) + 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, @@ -549,7 +574,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,8 +1073,11 @@ 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) + 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) }) }