diff --git a/docs/guides/dev/behaviour-drivers.md b/docs/guides/dev/behaviour-drivers.md index a3d19cd43e..bb3c8b565d 100644 --- a/docs/guides/dev/behaviour-drivers.md +++ b/docs/guides/dev/behaviour-drivers.md @@ -47,7 +47,7 @@ Use `forge.Client` for operations it already exposes; add REST helpers inside th ## Adding a CI driver -1. Implement `ci.Driver` — `WaitForWorkflow`, `FindCompletedWorkflowRun`, `AssertNoWorkflow`, `GetRunLogs`, `DownloadArtifacts`, `DownloadNamedArtifactFromRun`, `DownloadNamedArtifactAfter`, `WaitForHarnessAgent`, `AssertNoHarnessAgentArtifact`, `CountHarnessDispatches`. +1. Implement `ci.Driver` — `WaitForWorkflow`, `FindCompletedWorkflowRun`, `AssertNoWorkflow`, `GetRunLogs`, `DownloadArtifacts`, `DownloadNamedArtifactFromRun`, `DownloadNamedArtifactAfter`, `WaitForHarnessAgent`, `WaitForFailedHarnessAgent`, `AssertNoHarnessAgentArtifact`, `CountHarnessDispatches`. 2. Map forge `WorkflowRun` types to portable polling logic; reuse patterns from `e2e/admin/admin_test.go`. 3. Register in suite init for the matching `BEHAVIOUR_CI` value. diff --git a/docs/guides/dev/behaviour-testing.md b/docs/guides/dev/behaviour-testing.md index f8b82205bb..78df8a9e3c 100644 --- a/docs/guides/dev/behaviour-testing.md +++ b/docs/guides/dev/behaviour-testing.md @@ -50,11 +50,15 @@ Given a dummy agent that would: | Column | Meaning | |--------|---------| | `description` | Human label matched by assertion steps | -| `op` | `read_file`, `url_get`, `write_fixture` | +| `op` | `read_file`, `url_get`, `write_fixture`, `assert_env`, `assert_file`, `assert_json`, `checkout_branch` | | `args` | Op-specific; see below | **`write_fixture`:** `dest_path, fixtures/...` — content lives in `e2e/behaviour/fixtures/`, embedded in the committed scenario script at `.fullsend/behaviour/current-scenario.yaml`. +**`checkout_branch`:** a single regex-validated branch name. Probes the remote with `git ls-remote --exit-code`; when the ref exists it is fetched and the branch is based on `FETCH_HEAD` (so the branch carries that ref's commits), when the ref is absent the branch is based on the current `HEAD`, and any other probe failure (network, auth) fails the op instead of silently falling back. The op then records one marker commit on the branch so the applier post-script has content to push — and so a wrongful push moves the target branch tip, giving `branch ... is unchanged` assertions something to detect. Deliberately a narrow capability — not a general shell op. + +The `` placeholder expands to the scenario's issue number in `checkout_branch` args (only that op) and in the branch step definitions' branch names and head-branch patterns; order the `an issue` step before any step using it. + ### Assertion steps Each assertion verifies immediately against workflow artifacts. If the triage workflow has not been waited on yet, the step waits for completion and downloads artifacts first (same as `Then the triage workflow completes successfully`). @@ -68,10 +72,52 @@ And the agent will output issues.out with: """ ``` +### Branch assertion steps + +For scenarios that drive a run through the post-scripts to a real push, +`pkg/behaviourtest/steps/branch.go` adds SCM-level assertions. Record a +branch tip before the run to assert it did not move afterwards: + +```gherkin +Given an open pull request on branch "agent/990000099-decoy" +And the tip of branch "agent/990000099-decoy" is recorded +... +Then the pull request head branch matches "agent/-.*" +And branch "agent/990000099-decoy" is unchanged +``` + +`the pull request head branch matches` asserts exactly **one** open PR +head matches the pattern. The pattern is a Go regular expression, +anchored on both ends by the step — a literal `.` in a branch name must +be escaped, and a pattern that could also match a fixture branch (e.g. a +decoy inside the `agent/` namespace) makes the step fail as ambiguous. + +For fail-closed paths there is a failure counterpart, asserted against +the run conclusion plus the post-script's failure comment on the +scenario PR: + +```gherkin +Then the harness "fix" workflow fails reporting "Refusing to push" +``` + +Pick a stable fragment of the failure-comment contract (the category +label headline or a fixed detail phrase). No shipped scenario uses this +step yet — the fix stage's only dispatch route is a `changes_requested` +review from the org review bot, which the suite cannot produce — but the +step is unit-tested and ready for a suite-reachable fail-closed path. + ### Compatibility tags Use tags only for **exceptions** when a backend cannot run a scenario yet: `@skip:gitlab`, `@skip:per-org`, `@requires:per-repo`. Untagged scenarios run everywhere applicable. +`@requires:capability:` gates scenarios that assert behavior only present past a dependency version (e.g. an agents-repo release). Such scenarios are skipped unless the runner declares the capability in the comma-separated `BEHAVIOUR_CAPABILITIES` env var: + +```bash +BEHAVIOUR_CAPABILITIES=applier-branch-namespace make behaviour-test +``` + +This keeps CI green until the dependency ships; flip the capability on (locally or in the CI env) once the pinned dependency includes the behavior. + ## Fixture authoring Every scenario that dispatches an agent stage must include a `write_fixture` row emitting `output/agent-result.json` with content that conforms to the stage's result schema. The harness post-script validates this file before performing any post-processing (labelling, commenting, PR creation). If the fixture is missing or invalid, the harness fails with `Validation failed: FAIL: output/agent-result.json not found`. @@ -101,6 +147,7 @@ Existing fixtures under `e2e/behaviour/fixtures/`: | `triage/sufficient.json` | `triage-result.schema.json` | Triage stage result with `action: "sufficient"` | | `dispatch/ok.json` | _(none — dispatch proof)_ | Lightweight proof-of-execution marker for dispatch scenarios | | `review/comment.json` | `review-result.schema.json` | Review stage result with `action: "comment"` | +| `code/implemented.json` | `code-result.schema.json` | Code stage result targeting the default branch | The `dispatch/ok.json` fixture is not emitted as `output/agent-result.json` — it is used for auxiliary proof-of-execution files (e.g., `output/bash-routing-ok.json`). Scenarios that dispatch a **real agent stage** (triage, review, code, fix) must emit a schema-valid fixture to `output/agent-result.json`. @@ -367,4 +414,8 @@ suiteRunner := godog.TestSuite{ **`scm.Driver.DeleteRepo` addition:** The `scm.Driver` interface now includes a `DeleteRepo(ctx context.Context, owner, repo string) error` method. `CleanupScenario` calls it to delete ephemeral fork repos after each scenario. External `scm.Driver` implementations must add this method — return `forge.ErrNotFound` when the repository does not exist. +**`scm.Driver.ListOpenChangeProposals` / `scm.Driver.ListComments` additions:** `ListOpenChangeProposals(ctx, owner, repo) ([]forge.ChangeProposal, error)` returns the repository's **open** pull requests including each head branch; `ListComments(ctx, owner, repo, number) ([]forge.IssueComment, error)` returns the comments on an issue or pull request. The branch assertion steps and the scenario-cleanup namespace sweep call them. External `scm.Driver` implementations must add both methods. + +**`ci.Driver.WaitForFailedHarnessAgent` addition:** `WaitForFailedHarnessAgent(ctx, owner, repo, agent string, after time.Time) (*forge.WorkflowRun, error)` waits for the named agent's harness run to complete with a terminal failure conclusion (artifact-first detection, job-name fallback) and errors out early when the run succeeds instead. External `ci.Driver` implementations must add this method. + Bump the pinned version when behaviour step vocabulary or `pkg/e2etest` / `pkg/behaviourtest` APIs change. diff --git a/e2e/behaviour/features/code/branch-namespace.feature b/e2e/behaviour/features/code/branch-namespace.feature new file mode 100644 index 0000000000..966157d3a0 --- /dev/null +++ b/e2e/behaviour/features/code/branch-namespace.feature @@ -0,0 +1,56 @@ +Feature: Code applier branch handling + + The code applier must land pushes inside the dispatched issue's + agent/-* branch namespace no matter which branch the sandbox + leaves checked out, and must leave other issues' branches untouched. + These scenarios drive real code runs through the post-scripts against + live GitHub, so the branch guarantees are asserted on the scripts as + shipped rather than on unit-level copies of their logic. + + The scenarios are gated behind the applier-branch-namespace capability + (declare it via BEHAVIOUR_CAPABILITIES) because they assert applier + behavior that ships with the agents-side branch-namespace enforcement; + runs against older agents releases skip them. + + The fix applier's refuse-to-push-on-branch-mismatch counterpart cannot + be expressed here yet: the fix stage's only dispatch route is a + changes_requested review submitted by the org review bot, which the + behaviour suite cannot produce (suite-posted comments are bot-authored + and bot comments are dropped by the dispatch gates). That path stays + covered by script-level tests in the agents repo until a + suite-reachable fix trigger exists. + + The decoy branch uses issue number 990000099 — far above any issue + number a pool repo will ever reach — so the anchored + agent/-.* head assertion can never match the decoy itself. + + Background: + Given the enrolled test repository + + @requires:capability:applier-branch-namespace + Scenario: Code run is renamed into the issue namespace and other branches are untouched + Given an open pull request on branch "agent/990000099-decoy" + And the tip of branch "agent/990000099-decoy" is recorded + And an issue + And a dummy agent that would: + | description | op | args | + | Check out decoy branch | checkout_branch | agent/990000099-decoy | + | Emit code JSON | write_fixture | output/agent-result.json, fixtures/code/implemented.json | + When the issue is labeled "ready-to-code" + Then the harness "code" workflow completes successfully + And the agent will succeed to Check out decoy branch + And the pull request head branch matches "agent/-.*" + And branch "agent/990000099-decoy" is unchanged + + @requires:capability:applier-branch-namespace + Scenario: Conforming branch is pushed without rename + Given an issue + And a remote branch "agent/-impl" seeded with a commit + And a dummy agent that would: + | description | op | args | + | Check out namespaced branch | checkout_branch | agent/-impl | + | Emit code JSON | write_fixture | output/agent-result.json, fixtures/code/implemented.json | + When the issue is labeled "ready-to-code" + Then the harness "code" workflow completes successfully + And the agent will succeed to Check out namespaced branch + And the pull request head branch matches "agent/-impl" diff --git a/e2e/behaviour/fixtures/code/implemented.json b/e2e/behaviour/fixtures/code/implemented.json new file mode 100644 index 0000000000..7c6cb2f584 --- /dev/null +++ b/e2e/behaviour/fixtures/code/implemented.json @@ -0,0 +1,4 @@ +{ + "target_branch": "main", + "pr_body": "Automated behaviour-test implementation emitted by the scripted code run." +} diff --git a/internal/runtime/dummy.go b/internal/runtime/dummy.go index 0242494057..87dcfca227 100644 --- a/internal/runtime/dummy.go +++ b/internal/runtime/dummy.go @@ -25,6 +25,13 @@ var envVarNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) var jsonPathPattern = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*(\.[a-zA-Z_][a-zA-Z0-9_]*)*$`) +// branchNamePattern restricts checkout_branch to plain branch names: each +// slash-separated segment starts with an alphanumeric and continues with +// alphanumerics, dots, underscores, or dashes. Combined with the explicit +// ".." rejection in executeBehaviourOp this forbids option injection +// (leading dash), refspec tricks, and path traversal. +var branchNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*(/[A-Za-z0-9][A-Za-z0-9._-]*)*$`) + type sandboxExecFunc func(sandboxName, cmd string, timeout time.Duration) (stdout, stderr string, exitCode int, err error) type sandboxUploadFunc func(sandboxName, localPath, remotePath string) error @@ -254,6 +261,23 @@ func executeBehaviourOp(rt DummyRuntime, sandboxName, repoDir string, op Behavio return fmt.Errorf("write_fixture upload: %w", err) } return nil + case "checkout_branch": + name := strings.TrimSpace(op.Args) + if name == "" { + return fmt.Errorf("checkout_branch requires a branch name") + } + if !branchNamePattern.MatchString(name) || strings.Contains(name, "..") { + return fmt.Errorf("checkout_branch invalid branch name %q", name) + } + cmd := checkoutBranchCommand(repoDir, name) + _, stderr, exitCode, err := rt.execFn()(sandboxName, cmd, 120*time.Second) + if err != nil { + return fmt.Errorf("checkout_branch exec: %w", err) + } + if exitCode != 0 { + return fmt.Errorf("checkout_branch %s failed: %s", name, strings.TrimSpace(stderr)) + } + return nil case "assert_env": varName := strings.TrimSpace(op.Args) if varName == "" { @@ -317,6 +341,45 @@ func executeBehaviourOp(rt DummyRuntime, sandboxName, repoDir string, op Behavio } } +// checkoutBranchCommand builds the shell command for the checkout_branch +// op. The branch name must already be validated against branchNamePattern. +// +// Semantics (deliberately a single narrow capability, not a general +// shell op): +// - Probe the remote for the ref with `git ls-remote --exit-code`. +// Exit 2 means the ref does not exist — base the branch off the +// current HEAD. Any other non-zero exit (network, auth) fails the +// op instead of being silently collapsed into the HEAD fallback, +// which would make scenarios pass or fail for the wrong reason. +// - When the ref exists, fetch it and base the branch on FETCH_HEAD +// so the branch carries the remote ref's commits. +// - Record one marker commit on the branch. This gives the applier +// post-script real content to push and — because the local tip now +// differs from every remote tip — makes a wrongful push move the +// target branch, so "branch ... is unchanged" assertions can +// actually detect it. The commit subject uses a conventional-commit +// prefix because post-code derives the applier's PR title from it. +func checkoutBranchCommand(repoDir, name string) string { + quoted := shellQuote(name) + // Scoped to refs/heads/ throughout — the ls-remote probe already + // restricts to --heads, so the fetch must resolve the same ref + // rather than git's default disambiguation order (which would + // prefer a same-named tag over the branch). + refspec := shellQuote("refs/heads/" + name) + return fmt.Sprintf( + "cd %s"+ + " && if git ls-remote --exit-code --heads origin %s >/dev/null 2>&1; then"+ + " git fetch origin %s && git checkout -B %s FETCH_HEAD;"+ + " else rc=$?; if [ \"$rc\" -ne 2 ]; then echo \"checkout_branch: ls-remote failed with $rc\" >&2; exit 1; fi;"+ + " git checkout -B %s; fi"+ + " && mkdir -p behaviour && echo %s > behaviour/marker.txt"+ + " && git add behaviour/marker.txt"+ + " && git -c user.name=fullsend-behaviour -c user.email=behaviour@fullsend.invalid commit -m %s", + shellQuote(repoDir), quoted, refspec, quoted, quoted, + shellQuote("scripted marker for "+name), + shellQuote("test: add scripted marker commit")) +} + func validateHTTPURL(raw string) error { u, err := url.Parse(raw) if err != nil { diff --git a/internal/runtime/dummy_test.go b/internal/runtime/dummy_test.go index 0d7086a255..f31b600e31 100644 --- a/internal/runtime/dummy_test.go +++ b/internal/runtime/dummy_test.go @@ -3,8 +3,10 @@ package runtime import ( "bytes" "context" + "errors" "io" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -522,3 +524,227 @@ func TestExecuteBehaviourScript_CancelledContext(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "cancelled") } + +func TestExecuteBehaviourOp_CheckoutBranchSuccess(t *testing.T) { + t.Parallel() + + repoDir := t.TempDir() + var gotCmd string + rt := DummyRuntime{ExecFn: func(_ string, cmd string, _ time.Duration) (string, string, int, error) { + gotCmd = cmd + return "", "", 0, nil + }} + err := executeBehaviourOp(rt, "sandbox", repoDir, BehaviourOperation{ + Op: "checkout_branch", + Args: "agent/42-fix-widget", + }) + require.NoError(t, err) + assert.Contains(t, gotCmd, "cd '"+repoDir+"'") + assert.Contains(t, gotCmd, "git ls-remote --exit-code --heads origin 'agent/42-fix-widget'") + assert.Contains(t, gotCmd, "git checkout -B 'agent/42-fix-widget' FETCH_HEAD") + assert.Contains(t, gotCmd, "git checkout -B 'agent/42-fix-widget'; fi") + assert.Contains(t, gotCmd, "behaviour/marker.txt") + assert.Contains(t, gotCmd, "commit -m") +} + +// runCheckoutBranchForReal executes the checkout_branch shell command with +// a real shell and git, returning the op error. The ExecFn override runs +// the command via sh -c instead of a sandbox. +func runCheckoutBranchForReal(t *testing.T, repoDir, branch string) error { + t.Helper() + rt := DummyRuntime{ExecFn: func(_ string, cmd string, _ time.Duration) (string, string, int, error) { + out, err := exec.Command("sh", "-c", cmd).CombinedOutput() + if err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return "", string(out), exitErr.ExitCode(), nil + } + return "", string(out), -1, err + } + return string(out), "", 0, nil + }} + return executeBehaviourOp(rt, "sandbox", repoDir, BehaviourOperation{ + Op: "checkout_branch", + Args: branch, + }) +} + +// initCheckoutBranchRepos creates a bare "origin" with an initial commit +// on main plus a clone, and returns the clone path and origin path. +func initCheckoutBranchRepos(t *testing.T) (clone, origin string) { + t.Helper() + base := t.TempDir() + origin = filepath.Join(base, "origin.git") + clone = filepath.Join(base, "clone") + seed := filepath.Join(base, "seed") + + run := func(dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t.invalid", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t.invalid") + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) + return strings.TrimSpace(string(out)) + } + + run(base, "init", "--bare", "-b", "main", origin) + run(base, "init", "-b", "main", seed) + require.NoError(t, os.WriteFile(filepath.Join(seed, "README.md"), []byte("seed\n"), 0o644)) + run(seed, "add", "README.md") + run(seed, "commit", "-m", "initial") + run(seed, "push", origin, "main") + // Seed a remote-only branch with one extra commit. + run(seed, "checkout", "-b", "agent/7-existing") + require.NoError(t, os.WriteFile(filepath.Join(seed, "extra.txt"), []byte("extra\n"), 0o644)) + run(seed, "add", "extra.txt") + run(seed, "commit", "-m", "extra") + run(seed, "push", origin, "agent/7-existing") + run(base, "clone", "-b", "main", origin, clone) + return clone, origin +} + +func gitOut(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) + return strings.TrimSpace(string(out)) +} + +func TestExecuteBehaviourOp_CheckoutBranchRealShellExistingRemote(t *testing.T) { + t.Parallel() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + clone, origin := initCheckoutBranchRepos(t) + require.NoError(t, runCheckoutBranchForReal(t, clone, "agent/7-existing")) + + assert.Equal(t, "agent/7-existing", gitOut(t, clone, "branch", "--show-current")) + // The branch is based on the remote tip plus exactly one marker commit. + remoteTip := gitOut(t, origin, "rev-parse", "refs/heads/agent/7-existing") + assert.Equal(t, remoteTip, gitOut(t, clone, "rev-parse", "HEAD~1")) + assert.Equal(t, "test: add scripted marker commit", gitOut(t, clone, "log", "-1", "--format=%s")) + assert.FileExists(t, filepath.Join(clone, "extra.txt"), "remote branch content is present") + assert.FileExists(t, filepath.Join(clone, "behaviour", "marker.txt")) +} + +func TestExecuteBehaviourOp_CheckoutBranchRealShellMissingRemote(t *testing.T) { + t.Parallel() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + clone, _ := initCheckoutBranchRepos(t) + mainTip := gitOut(t, clone, "rev-parse", "HEAD") + require.NoError(t, runCheckoutBranchForReal(t, clone, "agent/7-brand-new")) + + assert.Equal(t, "agent/7-brand-new", gitOut(t, clone, "branch", "--show-current")) + // Missing remote ref falls back to the current HEAD plus the marker. + assert.Equal(t, mainTip, gitOut(t, clone, "rev-parse", "HEAD~1")) + assert.FileExists(t, filepath.Join(clone, "behaviour", "marker.txt")) +} + +func TestExecuteBehaviourOp_CheckoutBranchRealShellPrefersBranchOverSameNamedTag(t *testing.T) { + t.Parallel() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + clone, origin := initCheckoutBranchRepos(t) + branchTip := gitOut(t, origin, "rev-parse", "refs/heads/agent/7-existing") + + // Tag the *initial* commit with the same name as the branch. Without + // scoping the fetch to refs/heads/, git's default ref disambiguation + // would resolve the tag ahead of the branch. + seedTagCmd := exec.Command("git", "tag", "agent/7-existing", "main") + seedTagCmd.Dir = filepath.Join(filepath.Dir(clone), "seed") + require.NoError(t, seedTagCmd.Run()) + pushTagCmd := exec.Command("git", "push", origin, "refs/tags/agent/7-existing") + pushTagCmd.Dir = filepath.Join(filepath.Dir(clone), "seed") + out, err := pushTagCmd.CombinedOutput() + require.NoError(t, err, string(out)) + + require.NoError(t, runCheckoutBranchForReal(t, clone, "agent/7-existing")) + assert.Equal(t, branchTip, gitOut(t, clone, "rev-parse", "HEAD~1"), "checkout must resolve the branch, not the same-named tag") +} + +func TestExecuteBehaviourOp_CheckoutBranchRealShellRemoteError(t *testing.T) { + t.Parallel() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + clone, origin := initCheckoutBranchRepos(t) + // Break the remote so ls-remote fails with a non-2 exit code: the op + // must fail rather than silently branching off HEAD. + require.NoError(t, os.RemoveAll(origin)) + err := runCheckoutBranchForReal(t, clone, "agent/7-existing") + require.Error(t, err) + assert.Contains(t, err.Error(), "checkout_branch") +} + +func TestExecuteBehaviourOp_CheckoutBranchEmpty(t *testing.T) { + t.Parallel() + + err := executeBehaviourOp(DummyRuntime{}, "sandbox", t.TempDir(), BehaviourOperation{ + Op: "checkout_branch", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "requires a branch name") +} + +func TestExecuteBehaviourOp_CheckoutBranchInvalidName(t *testing.T) { + t.Parallel() + + for _, name := range []string{ + "agent/42; rm -rf /", + "-oProxyCommand=evil", + "--force", + "agent//double-slash", + "agent/../escape", + "has space", + "/leading-slash", + "trailing-slash/", + ".hidden-lead-dot", + } { + err := executeBehaviourOp(DummyRuntime{}, "sandbox", t.TempDir(), BehaviourOperation{ + Op: "checkout_branch", + Args: name, + }) + require.Error(t, err, "branch name %q should be rejected", name) + assert.Contains(t, err.Error(), "invalid branch name", "branch name %q", name) + } +} + +func TestExecuteBehaviourOp_CheckoutBranchNonZeroExit(t *testing.T) { + t.Parallel() + + rt := DummyRuntime{ExecFn: func(_ string, _ string, _ time.Duration) (string, string, int, error) { + return "", "fatal: not a git repository", 1, nil + }} + err := executeBehaviourOp(rt, "sandbox", t.TempDir(), BehaviourOperation{ + Op: "checkout_branch", + Args: "agent/42-fix-widget", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a git repository") +} + +func TestExecuteBehaviourOp_CheckoutBranchExecError(t *testing.T) { + t.Parallel() + + rt := DummyRuntime{ExecFn: func(_ string, _ string, _ time.Duration) (string, string, int, error) { + return "", "", 0, io.ErrUnexpectedEOF + }} + err := executeBehaviourOp(rt, "sandbox", t.TempDir(), BehaviourOperation{ + Op: "checkout_branch", + Args: "agent/42-fix-widget", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "checkout_branch exec") +} diff --git a/pkg/behaviourtest/drivers/ci/driver.go b/pkg/behaviourtest/drivers/ci/driver.go index f0ee044f44..7b31ef4e55 100644 --- a/pkg/behaviourtest/drivers/ci/driver.go +++ b/pkg/behaviourtest/drivers/ci/driver.go @@ -29,6 +29,12 @@ type Driver interface { DownloadNamedArtifactFromRun(ctx context.Context, owner, repo string, runID int, artifactName string, destDir string) error DownloadNamedArtifactAfter(ctx context.Context, owner, repo, artifactName string, after time.Time, destDir string) error WaitForHarnessAgent(ctx context.Context, owner, repo, agent string, after time.Time) (*forge.WorkflowRun, error) + // WaitForFailedHarnessAgent waits for the named agent's harness run to + // complete with a terminal failure conclusion (resolved artifact-first + // via the agent's uploaded artifact, falling back to a job-name scan). + // It errors out early when the run — or, in the fallback path, the + // agent's own job — completes successfully instead. + WaitForFailedHarnessAgent(ctx context.Context, owner, repo, agent string, after time.Time) (*forge.WorkflowRun, error) AssertNoHarnessAgentArtifact(ctx context.Context, owner, repo, agent string, after time.Time) error CountHarnessDispatches(ctx context.Context, owner, repo, agent string, after time.Time) (int, error) } diff --git a/pkg/behaviourtest/drivers/ci/githubactions/githubactions.go b/pkg/behaviourtest/drivers/ci/githubactions/githubactions.go index cc8308bb2c..8a87d6ecb4 100644 --- a/pkg/behaviourtest/drivers/ci/githubactions/githubactions.go +++ b/pkg/behaviourtest/drivers/ci/githubactions/githubactions.go @@ -562,6 +562,86 @@ func (d *Driver) WaitForHarnessAgent(ctx context.Context, owner, repo, agent str agent, formatRunDiagnostics(recentRuns)) } +// WaitForFailedHarnessAgent waits for the named agent's harness run to +// complete with a terminal failure conclusion. It errors out early when +// the run completes successfully instead — callers use this to assert a +// refuse-to-push (or similar fail-closed) path. +// +// Detection is artifact-first: the fullsend action uploads the +// fullsend- artifact with `if: always()`, so it exists for failed +// runs too, and it is the only signal that works for both standard stage +// jobs (named e.g. "Code"/"Fix") and custom-harness matrix jobs (named +// "Harness run ()"). The job-name scan is a fallback for +// custom-harness runs that failed before uploading the artifact. +func (d *Driver) WaitForFailedHarnessAgent(ctx context.Context, owner, repo, agent string, after time.Time) (*forge.WorkflowRun, error) { + artifactName := "fullsend-" + agent + deadline := time.Now().Add(dispatchWait) + var lastJobErr error + for time.Now().Before(deadline) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(dispatchPoll): + } + + // Artifact-first: resolve the agent's run from its artifact and + // inspect the run conclusion. + arts, err := d.Client.ListRepositoryArtifacts(ctx, owner, repo, 100) + if err == nil { + if art := selectRepositoryArtifactAfter(arts, artifactName, after); art != nil { + run, err := d.Client.GetWorkflowRun(ctx, owner, repo, art.WorkflowRunID) + if err == nil && run.Status == "completed" { + if isTerminalFailure(run.Conclusion) { + return run, nil + } + if run.Conclusion == "success" { + return nil, fmt.Errorf("harness agent %q run %d concluded successfully; expected failure (url=%s)", + agent, run.ID, run.HTMLURL) + } + } + } + } + + // Fallback: a custom-harness run can fail before uploading the + // artifact; attribute the failure through its matrix job name. + // Scans every completed run regardless of overall conclusion — + // not just runs already known to have failed — so a run whose + // agent job succeeded despite the artifact lookup missing it + // still fails fast instead of running out the full timeout. + lastJobErr = nil + recentRuns := d.listHarnessRunsAfter(ctx, owner, repo, after) + for i := range recentRuns { + run := recentRuns[i] + if run.Status != "completed" { + continue + } + hasJob, conclusion, err := d.runHasAgentJob(ctx, owner, repo, run.ID, agent) + if err != nil { + lastJobErr = err + continue + } + if !hasJob { + continue + } + if isTerminalFailure(conclusion) { + return &run, nil + } + if conclusion == "success" { + return nil, fmt.Errorf("harness agent %q job in run %d concluded successfully; expected failure (url=%s)", + agent, run.ID, run.HTMLURL) + } + // Skipped/cancelled jobs are concurrency noise — keep polling. + } + } + + recentRuns := d.listHarnessRunsAfter(ctx, owner, repo, after) + diag := formatRunDiagnostics(recentRuns) + if lastJobErr != nil { + diag += fmt.Sprintf("; last job-listing error: %v", lastJobErr) + } + return nil, fmt.Errorf("harness agent %q did not complete with a failure; %s", agent, diag) +} + // CountHarnessDispatches returns the number of harness workflow runs that // scheduled the "Harness run ()" job after the trigger time. func (d *Driver) CountHarnessDispatches(ctx context.Context, owner, repo, agent string, after time.Time) (int, error) { diff --git a/pkg/behaviourtest/drivers/ci/githubactions/githubactions_test.go b/pkg/behaviourtest/drivers/ci/githubactions/githubactions_test.go index 4b17df4292..39322e7eb6 100644 --- a/pkg/behaviourtest/drivers/ci/githubactions/githubactions_test.go +++ b/pkg/behaviourtest/drivers/ci/githubactions/githubactions_test.go @@ -368,6 +368,121 @@ func TestWaitForHarnessAgent_FailFastOnStartupFailure(t *testing.T) { assert.Contains(t, err.Error(), `"startup_failure"`) } +func TestWaitForFailedHarnessAgent_FromRepositoryArtifact(t *testing.T) { + t.Parallel() + + after := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + client := forge.NewFakeClient() + // The fullsend action uploads the artifact with if: always(), so a + // failed standard-stage run (job named "Fix", not "Harness run + // (fix)") is still resolvable through its artifact. + client.RepositoryArtifacts = map[string][]forge.RepositoryArtifact{ + "org/repo": { + {ID: 11, Name: "fullsend-fix", CreatedAt: "2026-01-02T00:00:00Z", WorkflowRunID: 77}, + }, + } + client.WorkflowRuns = map[string]*forge.WorkflowRun{ + "org/repo/fullsend.yaml": { + ID: 77, Status: "completed", Conclusion: "failure", CreatedAt: "2026-01-02T00:00:00Z", + HTMLURL: "https://github.com/org/repo/actions/runs/77", + }, + } + client.WorkflowRunJobs = map[int][]forge.WorkflowJob{ + 77: {{ID: 1, Name: "dispatch / Fix", Status: "completed", Conclusion: "failure"}}, + } + + d := &Driver{Client: client} + run, err := d.WaitForFailedHarnessAgent(context.Background(), "org", "repo", "fix", after) + require.NoError(t, err) + require.NotNil(t, run) + assert.Equal(t, 77, run.ID) +} + +func TestWaitForFailedHarnessAgent_ErrorsOnSuccess(t *testing.T) { + t.Parallel() + + after := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + client := forge.NewFakeClient() + client.RepositoryArtifacts = map[string][]forge.RepositoryArtifact{ + "org/repo": { + {ID: 12, Name: "fullsend-fix", CreatedAt: "2026-01-02T00:00:00Z", WorkflowRunID: 78}, + }, + } + client.WorkflowRuns = map[string]*forge.WorkflowRun{ + "org/repo/fullsend.yaml": { + ID: 78, Status: "completed", Conclusion: "success", CreatedAt: "2026-01-02T00:00:00Z", + HTMLURL: "https://github.com/org/repo/actions/runs/78", + }, + } + + d := &Driver{Client: client} + run, err := d.WaitForFailedHarnessAgent(context.Background(), "org", "repo", "fix", after) + require.Error(t, err) + assert.Nil(t, run) + assert.Contains(t, err.Error(), "concluded successfully; expected failure") +} + +func TestWaitForFailedHarnessAgent_FallbackJobNameMatch(t *testing.T) { + t.Parallel() + + after := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + client := forge.NewFakeClient() + // No artifact (custom harness failed before uploading one); the run + // is attributed through its "Harness run ()" matrix job. + client.WorkflowRuns = map[string]*forge.WorkflowRun{ + "org/repo/fullsend.yaml": { + ID: 79, Status: "completed", Conclusion: "failure", CreatedAt: "2026-01-02T00:00:00Z", + HTMLURL: "https://github.com/org/repo/actions/runs/79", + }, + } + client.WorkflowRunJobs = map[int][]forge.WorkflowJob{ + 79: {{ID: 1, Name: "dispatch / Harness run (fix-ping)", Status: "completed", Conclusion: "failure"}}, + } + + d := &Driver{Client: client} + run, err := d.WaitForFailedHarnessAgent(context.Background(), "org", "repo", "fix-ping", after) + require.NoError(t, err) + require.NotNil(t, run) + assert.Equal(t, 79, run.ID) +} + +func TestWaitForFailedHarnessAgent_FallbackErrorsOnJobSuccess(t *testing.T) { + t.Parallel() + + after := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + client := forge.NewFakeClient() + // No artifact for this run at all — the run's overall conclusion is + // "success", but the fallback must still inspect the agent's own + // job (not pre-filter on the run-level conclusion) so a run that + // completes successfully still fails fast via the fallback path, + // not just the artifact-based one. + client.WorkflowRuns = map[string]*forge.WorkflowRun{ + "org/repo/fullsend.yaml": { + ID: 80, Status: "completed", Conclusion: "success", CreatedAt: "2026-01-02T00:00:00Z", + HTMLURL: "https://github.com/org/repo/actions/runs/80", + }, + } + client.WorkflowRunJobs = map[int][]forge.WorkflowJob{ + 80: {{ID: 1, Name: "dispatch / Harness run (fix-ping)", Status: "completed", Conclusion: "success"}}, + } + + d := &Driver{Client: client} + run, err := d.WaitForFailedHarnessAgent(context.Background(), "org", "repo", "fix-ping", after) + require.Error(t, err) + assert.Nil(t, run) + assert.Contains(t, err.Error(), "concluded successfully; expected failure") +} + +func TestWaitForFailedHarnessAgent_ContextCancelled(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + d := &Driver{Client: forge.NewFakeClient()} + _, err := d.WaitForFailedHarnessAgent(ctx, "org", "repo", "fix", time.Now()) + require.ErrorIs(t, err, context.Canceled) +} + // TestWaitForHarnessAgent_SiblingRunFailureIgnored verifies the fix for // #5852: a sibling fullsend.yaml run (e.g. triggered by PR "opened") // that fails in Route/Review without scheduling the waited agent's diff --git a/pkg/behaviourtest/drivers/env/env.go b/pkg/behaviourtest/drivers/env/env.go index 567a4a7e02..a6f0b74802 100644 --- a/pkg/behaviourtest/drivers/env/env.go +++ b/pkg/behaviourtest/drivers/env/env.go @@ -11,14 +11,42 @@ type RunnerConfig struct { SCM string CI string InstallMode string + + // Capabilities lists environment capabilities the runner declares, + // from the comma-separated BEHAVIOUR_CAPABILITIES env var. Scenarios + // tagged @requires:capability: are skipped unless is + // declared — the gate for coverage of behavior that only exists past + // a certain dependency version (e.g. an agents-repo release). + Capabilities []string } func LoadRunnerConfig() RunnerConfig { return RunnerConfig{ - SCM: stringsTrimOrDefault(os.Getenv("BEHAVIOUR_SCM"), "github"), - CI: stringsTrimOrDefault(os.Getenv("BEHAVIOUR_CI"), "githubactions"), - InstallMode: stringsTrimOrDefault(os.Getenv("BEHAVIOUR_INSTALL_MODE"), "per-repo"), + SCM: stringsTrimOrDefault(os.Getenv("BEHAVIOUR_SCM"), "github"), + CI: stringsTrimOrDefault(os.Getenv("BEHAVIOUR_CI"), "githubactions"), + InstallMode: stringsTrimOrDefault(os.Getenv("BEHAVIOUR_INSTALL_MODE"), "per-repo"), + Capabilities: splitCapabilities(os.Getenv("BEHAVIOUR_CAPABILITIES")), + } +} + +// HasCapability reports whether the runner declared the named capability. +func (c RunnerConfig) HasCapability(name string) bool { + for _, declared := range c.Capabilities { + if declared == name { + return true + } + } + return false +} + +func splitCapabilities(raw string) []string { + var caps []string + for _, part := range strings.Split(raw, ",") { + if p := strings.TrimSpace(part); p != "" { + caps = append(caps, p) + } } + return caps } func (c RunnerConfig) Validate() error { diff --git a/pkg/behaviourtest/drivers/scm/driver.go b/pkg/behaviourtest/drivers/scm/driver.go index b1996074f0..5eb3c15d2b 100644 --- a/pkg/behaviourtest/drivers/scm/driver.go +++ b/pkg/behaviourtest/drivers/scm/driver.go @@ -33,6 +33,11 @@ type Driver interface { CreateChangeProposal(ctx context.Context, owner, repo, title, body, head, base string) (*forge.ChangeProposal, error) SubmitPullRequestReview(ctx context.Context, owner, repo string, number int, event string) error CloseIssue(ctx context.Context, owner, repo string, number int) error + // ListOpenChangeProposals returns the repository's open pull + // requests, including each proposal's head branch. + ListOpenChangeProposals(ctx context.Context, owner, repo string) ([]forge.ChangeProposal, error) + // ListComments returns the comments on an issue or pull request. + ListComments(ctx context.Context, owner, repo string, number int) ([]forge.IssueComment, error) // CreateRepo creates a new repository in the given org. It is // idempotent — if a repo with the given name already exists, diff --git a/pkg/behaviourtest/drivers/scm/github/github.go b/pkg/behaviourtest/drivers/scm/github/github.go index a32b22b16f..c622ca01a5 100644 --- a/pkg/behaviourtest/drivers/scm/github/github.go +++ b/pkg/behaviourtest/drivers/scm/github/github.go @@ -66,6 +66,14 @@ func (d *Driver) CreateChangeProposal(ctx context.Context, owner, repo, title, b return d.Client.CreateChangeProposal(ctx, owner, repo, title, body, head, base) } +func (d *Driver) ListOpenChangeProposals(ctx context.Context, owner, repo string) ([]forge.ChangeProposal, error) { + return d.Client.ListRepoPullRequests(ctx, owner, repo) +} + +func (d *Driver) ListComments(ctx context.Context, owner, repo string, number int) ([]forge.IssueComment, error) { + return d.Client.ListIssueComments(ctx, owner, repo, number) +} + func (d *Driver) SubmitPullRequestReview(ctx context.Context, owner, repo string, number int, event string) error { sha, err := d.Client.GetPullRequestHeadSHA(ctx, owner, repo, number) if err != nil { diff --git a/pkg/behaviourtest/steps/branch.go b/pkg/behaviourtest/steps/branch.go new file mode 100644 index 0000000000..14889cfa3b --- /dev/null +++ b/pkg/behaviourtest/steps/branch.go @@ -0,0 +1,269 @@ +package steps + +import ( + "context" + "fmt" + "regexp" + "strconv" + "strings" + "time" + + "github.com/cucumber/godog" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" +) + +// issuePlaceholder is replaced with the scenario's issue number in branch +// names and head-branch patterns. Branch names for code runs embed the +// dispatched issue's number (agent/-*), which is only known once +// the "an issue" step has run. +const issuePlaceholder = "" + +func registerBranchSteps(sc *godog.ScenarioContext) { + sc.Step(`^an open pull request on branch "([^"]+)"$`, func(ctx context.Context, branch string) (context.Context, error) { + return ctx, givenOpenPullRequestOnBranch(world.FromContext(ctx), branch) + }) + sc.Step(`^a remote branch "([^"]+)" seeded with a commit$`, func(ctx context.Context, branch string) (context.Context, error) { + return ctx, givenSeededRemoteBranch(world.FromContext(ctx), branch) + }) + sc.Step(`^the tip of branch "([^"]+)" is recorded$`, func(ctx context.Context, branch string) (context.Context, error) { + return ctx, givenBranchTipRecorded(world.FromContext(ctx), branch) + }) + sc.Step(`^branch "([^"]+)" is unchanged$`, func(ctx context.Context, branch string) (context.Context, error) { + return ctx, thenBranchUnchanged(world.FromContext(ctx), branch) + }) + sc.Step(`^the pull request head branch matches "([^"]+)"$`, func(ctx context.Context, pattern string) (context.Context, error) { + return ctx, thenPullRequestHeadBranchMatches(world.FromContext(ctx), pattern) + }) + sc.Step(`^a comment "([^"]+)" is posted on the pull request$`, func(ctx context.Context, body string) (context.Context, error) { + return ctx, whenCommentPostedOnPullRequest(world.FromContext(ctx), body) + }) + sc.Step(`^the harness "([^"]+)" workflow fails reporting "([^"]+)"$`, func(ctx context.Context, agent, report string) (context.Context, error) { + return ctx, thenHarnessWorkflowFailsReporting(ctx, world.FromContext(ctx), agent, report) + }) +} + +// expandIssuePlaceholder substitutes the scenario's issue number for +// "" tokens. It is an error to use the placeholder before the +// "an issue" step has created one. +func expandIssuePlaceholder(w *world.World, s string) (string, error) { + if !strings.Contains(s, issuePlaceholder) { + return s, nil + } + if w.IssueNumber == 0 { + return "", fmt.Errorf("%q uses %s but no issue exists yet — order the \"an issue\" step first", s, issuePlaceholder) + } + return strings.ReplaceAll(s, issuePlaceholder, strconv.Itoa(w.IssueNumber)), nil +} + +// resetBranchIfExists deletes a leftover branch from a previous scenario +// on the same pool repo. Pool repos are reused, so fixed branch names in +// feature files must tolerate debris. +func resetBranchIfExists(w *world.World, branch string) error { + if err := w.SCM.DeleteBranch(context.Background(), w.RepoOwner, w.RepoName, branch); err != nil && !forge.IsNotFound(err) { + return fmt.Errorf("deleting leftover branch %s: %w", branch, err) + } + return nil +} + +func ensureScenarioRepo(w *world.World) { + if w.RepoOwner == "" || w.RepoName == "" { + w.RepoOwner = w.Org + w.RepoName = w.Install.TestRepo() + w.RepoFull = w.Org + "/" + w.RepoName + } +} + +func givenSeededRemoteBranch(w *world.World, branch string) error { + ensureScenarioRepo(w) + branch, err := expandIssuePlaceholder(w, branch) + if err != nil { + return err + } + if err := resetBranchIfExists(w, branch); err != nil { + return err + } + ctx := context.Background() + if err := w.SCM.CreateBranch(ctx, w.RepoOwner, w.RepoName, branch); err != nil { + return fmt.Errorf("creating branch %s: %w", branch, err) + } + w.CreatedBranches = append(w.CreatedBranches, branch) + content := fmt.Sprintf("scripted behaviour seed for %s\n", branch) + if err := w.SCM.CommitFileToBranch(ctx, w.RepoOwner, w.RepoName, branch, "behaviour/seed.txt", "behaviour: seed scripted branch", []byte(content)); err != nil { + return fmt.Errorf("seeding branch %s: %w", branch, err) + } + return nil +} + +func givenOpenPullRequestOnBranch(w *world.World, branch string) error { + ensureScenarioRepo(w) + branch, err := expandIssuePlaceholder(w, branch) + if err != nil { + return err + } + if err := givenSeededRemoteBranch(w, branch); err != nil { + return err + } + // Note: a bot-authored PR "opened" event dispatches a review-stage + // run in enrolled repos. It cannot satisfy this scenario's own + // assertions (those filter by agent job/artifact and ScenarioStart), + // but it does consume a runner and may fail after the scenario ends. + base, err := w.SCM.GetDefaultBranch(context.Background(), w.RepoOwner, w.RepoName) + if err != nil { + return fmt.Errorf("resolving default branch: %w", err) + } + w.ScenarioStart = time.Now() + pr, err := w.SCM.CreateChangeProposal(context.Background(), w.RepoOwner, w.RepoName, + "Behaviour decoy PR", "Scripted behaviour scenario fixture.", branch, base) + if err != nil { + return fmt.Errorf("opening PR on %s: %w", branch, err) + } + w.PRNumber = pr.Number + w.CreatedPRNumbers = append(w.CreatedPRNumbers, pr.Number) + return nil +} + +func givenBranchTipRecorded(w *world.World, branch string) error { + ensureScenarioRepo(w) + branch, err := expandIssuePlaceholder(w, branch) + if err != nil { + return err + } + sha, err := w.SCM.GetBranchRef(context.Background(), w.RepoOwner, w.RepoName, branch) + if err != nil { + return fmt.Errorf("recording tip of %s: %w", branch, err) + } + if w.RecordedBranchSHAs == nil { + w.RecordedBranchSHAs = map[string]string{} + } + w.RecordedBranchSHAs[branch] = sha + return nil +} + +func thenBranchUnchanged(w *world.World, branch string) error { + ensureScenarioRepo(w) + branch, err := expandIssuePlaceholder(w, branch) + if err != nil { + return err + } + recorded, ok := w.RecordedBranchSHAs[branch] + if !ok { + return fmt.Errorf("branch %q was never recorded — add a \"the tip of branch ... is recorded\" step before the run", branch) + } + current, err := w.SCM.GetBranchRef(context.Background(), w.RepoOwner, w.RepoName, branch) + if err != nil { + return fmt.Errorf("re-checking tip of %s: %w", branch, err) + } + if current != recorded { + return fmt.Errorf("branch %s moved: recorded %s, now %s", branch, recorded, current) + } + return nil +} + +// thenPullRequestHeadBranchMatches asserts that exactly one open pull +// request has a head branch matching the anchored pattern, and tracks +// the match for scenario cleanup. Patterns support the +// placeholder so features can assert the agent/-* namespace. +func thenPullRequestHeadBranchMatches(w *world.World, pattern string) error { + pattern, err := expandIssuePlaceholder(w, pattern) + if err != nil { + return err + } + re, err := regexp.Compile("^" + pattern + "$") + if err != nil { + return fmt.Errorf("invalid head branch pattern %q: %w", pattern, err) + } + prs, err := w.SCM.ListOpenChangeProposals(context.Background(), w.RepoOwner, w.RepoName) + if err != nil { + return fmt.Errorf("listing open PRs: %w", err) + } + var matched []forge.ChangeProposal + var heads []string + for _, pr := range prs { + heads = append(heads, pr.Head) + if re.MatchString(pr.Head) { + matched = append(matched, pr) + } + } + if len(matched) == 0 { + return fmt.Errorf("no open PR head branch matches %q (open heads: %s)", pattern, strings.Join(heads, ", ")) + } + if len(matched) > 1 { + return fmt.Errorf("%d open PR head branches match %q, want exactly 1 (open heads: %s)", len(matched), pattern, strings.Join(heads, ", ")) + } + w.CreatedPRNumbers = append(w.CreatedPRNumbers, matched[0].Number) + w.CreatedBranches = append(w.CreatedBranches, matched[0].Head) + return nil +} + +func whenCommentPostedOnPullRequest(w *world.World, body string) error { + if w.PRNumber == 0 { + return fmt.Errorf("no pull request opened") + } + w.ScenarioStart = time.Now() + if _, err := w.SCM.AddComment(context.Background(), w.RepoOwner, w.RepoName, w.PRNumber, body); err != nil { + return fmt.Errorf("commenting on PR #%d: %w", w.PRNumber, err) + } + return nil +} + +// failureCommentPollWindow bounds the wait for the post-script's failure +// comment after the harness run has already concluded. The post-script +// convention (post_fail_to_*) posts the comment before exiting non-zero, +// but comment visibility relative to the run's recorded conclusion is +// not guaranteed, so the window is sized generously rather than assuming +// strict ordering. Variables (not constants) so unit tests can shrink +// the window. +var failureCommentPollWindow = 90 * time.Second + +var failureCommentPollInterval = 5 * time.Second + +// thenHarnessWorkflowFailsReporting waits for the agent's harness run to +// conclude with a terminal failure, then asserts the post-script's +// failure comment on the scenario PR contains the given text. Pick a +// stable fragment of the failure-comment contract (the category label +// headline or a fixed detail phrase), not incidental prose. +// +// No shipped scenario uses this step yet: the fix stage's only dispatch +// route is a changes_requested review from the org review bot, which the +// behaviour suite cannot produce. The step is exercised by unit tests +// and ready for when a suite-reachable fail-closed path exists. +func thenHarnessWorkflowFailsReporting(ctx context.Context, w *world.World, agent, report string) error { + agent = strings.TrimSpace(agent) + if w.ScenarioStart.IsZero() { + return fmt.Errorf("no workflow trigger time recorded") + } + if w.PRNumber == 0 { + return fmt.Errorf("no pull request opened") + } + run, err := w.CI.WaitForFailedHarnessAgent(ctx, w.Org, w.Install.TriageWorkflowRepo(), agent, w.ScenarioStart) + if err != nil { + return err + } + w.WorkflowRun = run + + deadline := time.Now().Add(failureCommentPollWindow) + var lastErr error + for { + comments, err := w.SCM.ListComments(ctx, w.RepoOwner, w.RepoName, w.PRNumber) + if err != nil { + lastErr = fmt.Errorf("listing PR #%d comments: %w", w.PRNumber, err) + } else { + for _, c := range comments { + if strings.Contains(c.Body, report) { + return nil + } + } + lastErr = fmt.Errorf("no comment on PR #%d contains %q (%d comments checked)", w.PRNumber, report, len(comments)) + } + if time.Now().After(deadline) { + return lastErr + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(failureCommentPollInterval): + } + } +} diff --git a/pkg/behaviourtest/steps/branch_test.go b/pkg/behaviourtest/steps/branch_test.go new file mode 100644 index 0000000000..c956be1fbf --- /dev/null +++ b/pkg/behaviourtest/steps/branch_test.go @@ -0,0 +1,272 @@ +package steps + +import ( + "context" + "testing" + "time" + + "github.com/cucumber/godog" + messages "github.com/cucumber/messages/go/v21" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/ci" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/scm" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" +) + +// fakeBranchSCM implements the scm.Driver methods the branch steps use. +// Unused methods come from the embedded interface and panic when called. +type fakeBranchSCM struct { + scm.Driver + + branchSHAs map[string]string // branch → tip SHA returned by GetBranchRef + + deletedBranches []string + createdBranches []string + seededBranches []string + openPRs []forge.ChangeProposal + comments []forge.IssueComment + addedComments []string + + nextPRNumber int +} + +func (f *fakeBranchSCM) DeleteBranch(_ context.Context, _, _, branch string) error { + f.deletedBranches = append(f.deletedBranches, branch) + if _, ok := f.branchSHAs[branch]; !ok { + return forge.ErrNotFound + } + delete(f.branchSHAs, branch) + return nil +} + +func (f *fakeBranchSCM) CreateBranch(_ context.Context, _, _, branch string) error { + if f.branchSHAs == nil { + f.branchSHAs = map[string]string{} + } + f.branchSHAs[branch] = "base-sha" + f.createdBranches = append(f.createdBranches, branch) + return nil +} + +func (f *fakeBranchSCM) CommitFileToBranch(_ context.Context, _, _, branch, _, _ string, _ []byte) error { + f.branchSHAs[branch] = "seeded-sha-" + branch + f.seededBranches = append(f.seededBranches, branch) + return nil +} + +func (f *fakeBranchSCM) CreateChangeProposal(_ context.Context, _, _, title, body, head, base string) (*forge.ChangeProposal, error) { + f.nextPRNumber++ + pr := forge.ChangeProposal{Title: title, Number: f.nextPRNumber, Head: head, Base: base} + f.openPRs = append(f.openPRs, pr) + return &pr, nil +} + +func (f *fakeBranchSCM) GetDefaultBranch(context.Context, string, string) (string, error) { + return "main", nil +} + +func (f *fakeBranchSCM) GetBranchRef(_ context.Context, _, _, branch string) (string, error) { + sha, ok := f.branchSHAs[branch] + if !ok { + return "", forge.ErrNotFound + } + return sha, nil +} + +func (f *fakeBranchSCM) ListOpenChangeProposals(context.Context, string, string) ([]forge.ChangeProposal, error) { + return f.openPRs, nil +} + +func (f *fakeBranchSCM) ListComments(context.Context, string, string, int) ([]forge.IssueComment, error) { + return f.comments, nil +} + +func (f *fakeBranchSCM) AddComment(_ context.Context, _, _ string, _ int, body string) (*forge.IssueComment, error) { + f.addedComments = append(f.addedComments, body) + return &forge.IssueComment{Body: body}, nil +} + +// fakeBranchCI implements WaitForFailedHarnessAgent; other ci.Driver +// methods come from the embedded interface and panic when called. +type fakeBranchCI struct { + ci.Driver + + run *forge.WorkflowRun + err error +} + +func (f *fakeBranchCI) WaitForFailedHarnessAgent(context.Context, string, string, string, time.Time) (*forge.WorkflowRun, error) { + return f.run, f.err +} + +func branchTestWorld(scmDriver scm.Driver) *world.World { + return &world.World{ + SCM: scmDriver, + Org: "test-org", + RepoOwner: "test-org", + RepoName: "test-repo", + } +} + +func TestExpandIssuePlaceholder(t *testing.T) { + w := &world.World{IssueNumber: 42} + + got, err := expandIssuePlaceholder(w, "agent/-impl") + require.NoError(t, err) + assert.Equal(t, "agent/42-impl", got) + + got, err = expandIssuePlaceholder(w, "no-placeholder") + require.NoError(t, err) + assert.Equal(t, "no-placeholder", got) +} + +func TestExpandIssuePlaceholder_NoIssue(t *testing.T) { + _, err := expandIssuePlaceholder(&world.World{}, "agent/-impl") + require.Error(t, err) + assert.Contains(t, err.Error(), "no issue exists yet") +} + +func TestGivenSeededRemoteBranch(t *testing.T) { + scmDriver := &fakeBranchSCM{} + w := branchTestWorld(scmDriver) + w.IssueNumber = 7 + + require.NoError(t, givenSeededRemoteBranch(w, "agent/-impl")) + + assert.Equal(t, []string{"agent/7-impl"}, scmDriver.deletedBranches, "leftover branch is reset first") + assert.Equal(t, []string{"agent/7-impl"}, scmDriver.createdBranches) + assert.Equal(t, []string{"agent/7-impl"}, scmDriver.seededBranches) + assert.Equal(t, []string{"agent/7-impl"}, w.CreatedBranches) +} + +func TestGivenOpenPullRequestOnBranch(t *testing.T) { + scmDriver := &fakeBranchSCM{} + w := branchTestWorld(scmDriver) + + require.NoError(t, givenOpenPullRequestOnBranch(w, "agent/99999-decoy")) + + require.Len(t, scmDriver.openPRs, 1) + assert.Equal(t, "agent/99999-decoy", scmDriver.openPRs[0].Head) + assert.Equal(t, scmDriver.openPRs[0].Number, w.PRNumber) + assert.Equal(t, []int{scmDriver.openPRs[0].Number}, w.CreatedPRNumbers) + assert.Equal(t, []string{"agent/99999-decoy"}, w.CreatedBranches) + assert.False(t, w.ScenarioStart.IsZero()) +} + +func TestBranchTipRecordedAndUnchanged(t *testing.T) { + scmDriver := &fakeBranchSCM{branchSHAs: map[string]string{"agent/99999-decoy": "abc123"}} + w := branchTestWorld(scmDriver) + + require.NoError(t, givenBranchTipRecorded(w, "agent/99999-decoy")) + require.NoError(t, thenBranchUnchanged(w, "agent/99999-decoy")) + + scmDriver.branchSHAs["agent/99999-decoy"] = "def456" + err := thenBranchUnchanged(w, "agent/99999-decoy") + require.Error(t, err) + assert.Contains(t, err.Error(), "moved") +} + +func TestThenBranchUnchanged_NotRecorded(t *testing.T) { + w := branchTestWorld(&fakeBranchSCM{}) + err := thenBranchUnchanged(w, "never-recorded") + require.Error(t, err) + assert.Contains(t, err.Error(), "never recorded") +} + +func TestThenPullRequestHeadBranchMatches(t *testing.T) { + scmDriver := &fakeBranchSCM{openPRs: []forge.ChangeProposal{ + {Number: 1, Head: "agent/99999-decoy"}, + {Number: 2, Head: "agent/42-99999-decoy"}, + }} + w := branchTestWorld(scmDriver) + w.IssueNumber = 42 + + require.NoError(t, thenPullRequestHeadBranchMatches(w, `agent/-.*`)) + assert.Equal(t, []int{2}, w.CreatedPRNumbers, "matched PR is tracked for cleanup") + assert.Equal(t, []string{"agent/42-99999-decoy"}, w.CreatedBranches) +} + +func TestThenPullRequestHeadBranchMatches_Anchored(t *testing.T) { + scmDriver := &fakeBranchSCM{openPRs: []forge.ChangeProposal{ + {Number: 1, Head: "agent/123-impl"}, + }} + w := branchTestWorld(scmDriver) + w.IssueNumber = 12 + + err := thenPullRequestHeadBranchMatches(w, `agent/-.*`) + require.Error(t, err, "agent/12-* must not match agent/123-impl") + assert.Contains(t, err.Error(), "no open PR head branch matches") +} + +func TestThenPullRequestHeadBranchMatches_Ambiguous(t *testing.T) { + scmDriver := &fakeBranchSCM{openPRs: []forge.ChangeProposal{ + {Number: 1, Head: "agent/42-a"}, + {Number: 2, Head: "agent/42-b"}, + }} + w := branchTestWorld(scmDriver) + w.IssueNumber = 42 + + err := thenPullRequestHeadBranchMatches(w, `agent/-.*`) + require.Error(t, err) + assert.Contains(t, err.Error(), "want exactly 1") +} + +func TestWhenCommentPostedOnPullRequest(t *testing.T) { + scmDriver := &fakeBranchSCM{} + w := branchTestWorld(scmDriver) + + err := whenCommentPostedOnPullRequest(w, "/fs-fix") + require.Error(t, err, "requires an open PR") + + w.PRNumber = 5 + require.NoError(t, whenCommentPostedOnPullRequest(w, "/fs-fix")) + assert.Equal(t, []string{"/fs-fix"}, scmDriver.addedComments) + assert.False(t, w.ScenarioStart.IsZero()) +} + +func TestThenHarnessWorkflowFailsReporting(t *testing.T) { + origWindow, origInterval := failureCommentPollWindow, failureCommentPollInterval + failureCommentPollWindow, failureCommentPollInterval = 50*time.Millisecond, 10*time.Millisecond + t.Cleanup(func() { + failureCommentPollWindow, failureCommentPollInterval = origWindow, origInterval + }) + + scmDriver := &fakeBranchSCM{comments: []forge.IssueComment{ + {Body: "⚠️ Post-fix script failed — branch does not match. Refusing to push."}, + }} + w := branchTestWorld(scmDriver) + w.PRNumber = 5 + w.ScenarioStart = time.Now() + w.Install = &fakeInstallState{testRepo: "test-repo"} + w.CI = &fakeBranchCI{run: &forge.WorkflowRun{ID: 9}} + + require.NoError(t, thenHarnessWorkflowFailsReporting(context.Background(), w, "fix", "Refusing to push")) + assert.Equal(t, 9, w.WorkflowRun.ID) + + err := thenHarnessWorkflowFailsReporting(context.Background(), w, "fix", "not-present-text") + require.Error(t, err) + assert.Contains(t, err.Error(), "no comment on PR #5") +} + +func TestParseDummyAgentTable_ExpandsIssueInCheckoutBranchOnly(t *testing.T) { + w := &world.World{ + SCM: &fakeCleanupSCM{}, + Install: &fakeInstallState{testRepo: "test-repo"}, + FixturesRoot: "e2e/behaviour", + IssueNumber: 42, + } + table := &godog.Table{ + Rows: []*messages.PickleTableRow{ + {Cells: []*messages.PickleTableCell{{Value: "description"}, {Value: "op"}, {Value: "args"}}}, + {Cells: []*messages.PickleTableCell{{Value: "checkout"}, {Value: "checkout_branch"}, {Value: "agent/-impl"}}}, + {Cells: []*messages.PickleTableCell{{Value: "read"}, {Value: "read_file"}, {Value: "docs/.md"}}}, + }, + } + require.NoError(t, parseDummyAgentTable(w, table)) + require.Len(t, w.DummyOps, 2) + assert.Equal(t, "agent/42-impl", w.DummyOps[0].Args, "checkout_branch args expand ") + assert.Equal(t, "docs/.md", w.DummyOps[1].Args, "other ops keep literal") +} diff --git a/pkg/behaviourtest/steps/cleanup.go b/pkg/behaviourtest/steps/cleanup.go index 4fa7c8eea1..6cdb2788c2 100644 --- a/pkg/behaviourtest/steps/cleanup.go +++ b/pkg/behaviourtest/steps/cleanup.go @@ -2,6 +2,7 @@ package steps import ( "context" + "fmt" "os" "path/filepath" "strings" @@ -26,6 +27,59 @@ func CleanupScenario(w *world.World) { } } + // --- Branch-scenario cleanup --- + // Sweep applier-created PRs by namespace: a code run for this + // scenario's issue pushes to agent/-*, but the PR is only + // registered in CreatedPRNumbers when the head-match assertion ran + // and succeeded. Issue numbers are unique, so anything left open in + // the namespace would otherwise be permanent pool-repo debris. Gated + // on IssueNumber alone (not on CreatedBranches) so it still runs for + // a code-stage scenario that never seeded a decoy/seed branch. + if w.IssueNumber > 0 { + namespacePrefix := fmt.Sprintf("agent/%d-", w.IssueNumber) + seenPR := make(map[int]bool, len(w.CreatedPRNumbers)) + for _, n := range w.CreatedPRNumbers { + seenPR[n] = true + } + if prs, err := w.SCM.ListOpenChangeProposals(ctx, w.RepoOwner, w.RepoName); err != nil { + worldLogf(w, "behaviour cleanup: list open PRs for namespace sweep: %v", err) + } else { + for _, pr := range prs { + if !strings.HasPrefix(pr.Head, namespacePrefix) || seenPR[pr.Number] { + continue + } + seenPR[pr.Number] = true + w.CreatedPRNumbers = append(w.CreatedPRNumbers, pr.Number) + w.CreatedBranches = append(w.CreatedBranches, pr.Head) + } + } + } + + // Close PRs before deleting their head branches so GitHub does not + // auto-close them with a confusing "branch deleted" event first. + closedPR := make(map[int]bool, len(w.CreatedPRNumbers)) + for _, number := range w.CreatedPRNumbers { + if closedPR[number] { + continue + } + closedPR[number] = true + if err := w.SCM.CloseIssue(ctx, w.RepoOwner, w.RepoName, number); err != nil { + worldLogf(w, "behaviour cleanup: close PR #%d: %v", number, err) + } + } + deletedBranch := make(map[string]bool, len(w.CreatedBranches)) + for _, branch := range w.CreatedBranches { + if deletedBranch[branch] { + continue + } + deletedBranch[branch] = true + if err := w.SCM.DeleteBranch(ctx, w.RepoOwner, w.RepoName, branch); err != nil { + if !forge.IsNotFound(err) { + worldLogf(w, "behaviour cleanup: delete branch %s: %v", branch, err) + } + } + } + // --- Fork repo cleanup --- // Fork repos are ephemeral: created per-scenario and deleted here. // Branch and PR cleanup above already ran against the base repo; diff --git a/pkg/behaviourtest/steps/cleanup_test.go b/pkg/behaviourtest/steps/cleanup_test.go index 78cf0ef9cd..35cbb6dcd6 100644 --- a/pkg/behaviourtest/steps/cleanup_test.go +++ b/pkg/behaviourtest/steps/cleanup_test.go @@ -431,6 +431,7 @@ type fakeCleanupSCM struct { commitFileErr error fileContent []byte getFileErr error + openPRs []forge.ChangeProposal } type closedIssueRecord struct { @@ -521,6 +522,14 @@ func (f *fakeCleanupSCM) CreateRepo(context.Context, string, string, string) err return nil } +func (f *fakeCleanupSCM) ListOpenChangeProposals(context.Context, string, string) ([]forge.ChangeProposal, error) { + return f.openPRs, nil +} + +func (f *fakeCleanupSCM) ListComments(context.Context, string, string, int) ([]forge.IssueComment, error) { + return nil, nil +} + func (f *fakeCleanupSCM) EnsureRepoPublic(context.Context, string, string) error { return nil } @@ -707,3 +716,96 @@ func (f *fakeCleanupInstall) TriageWorkflowRepo() string { return f.repo } func (f *fakeCleanupInstall) TriageWorkflowFile() string { return "fullsend.yaml" } func (f *fakeCleanupInstall) AgentWorkflowFile() string { return "reusable-triage.yml" } func (f *fakeCleanupInstall) AgentArtifactName() string { return "fullsend-triage" } + +func TestCleanupScenario_BranchScenarioSweep(t *testing.T) { + t.Parallel() + + scmDriver := &fakeCleanupSCM{openPRs: []forge.ChangeProposal{ + {Number: 71, Head: "agent/7-impl"}, // applier PR for this scenario's issue — swept + {Number: 72, Head: "agent/8-other-issue"}, // different issue's namespace — untouched + }} + w := &world.World{ + RepoOwner: "org", + RepoName: "repo", + IssueNumber: 7, + SCM: scmDriver, + CreatedBranches: []string{"agent/990000099-decoy"}, + CreatedPRNumbers: []int{ + 70, // decoy PR tracked at Given time + }, + } + CleanupScenario(w) + + var closed []int + for _, rec := range scmDriver.closedIssues { + closed = append(closed, rec.number) + } + // Issue #7 itself is closed too (IssueNumber > 0 path). + assert.ElementsMatch(t, []int{7, 70, 71}, closed) + + var deleted []string + for _, rec := range scmDriver.deletedBranches { + deleted = append(deleted, rec.branch) + } + assert.ElementsMatch(t, []string{"agent/990000099-decoy", "agent/7-impl"}, deleted) +} + +func TestCleanupScenario_BranchScenarioSweep_DedupesAlreadyTrackedPR(t *testing.T) { + t.Parallel() + + // Mirrors the shipped "renamed into the issue namespace" scenario: + // the head-match assertion already tracked the applier PR before + // cleanup runs, so the sweep must not close/delete it a second time. + scmDriver := &fakeCleanupSCM{openPRs: []forge.ChangeProposal{ + {Number: 71, Head: "agent/7-impl"}, + }} + w := &world.World{ + RepoOwner: "org", + RepoName: "repo", + IssueNumber: 7, + SCM: scmDriver, + CreatedBranches: []string{"agent/7-impl"}, + CreatedPRNumbers: []int{71}, + } + CleanupScenario(w) + + closedCount := 0 + for _, rec := range scmDriver.closedIssues { + if rec.number == 71 { + closedCount++ + } + } + assert.Equal(t, 1, closedCount, "PR #71 must be closed exactly once") + + deletedCount := 0 + for _, rec := range scmDriver.deletedBranches { + if rec.branch == "agent/7-impl" { + deletedCount++ + } + } + assert.Equal(t, 1, deletedCount, "branch must be deleted exactly once") +} + +func TestCleanupScenario_BranchScenarioSweep_RunsWithoutBranchSteps(t *testing.T) { + t.Parallel() + + // A code-stage scenario that dispatches without any Given branch/PR + // step (CreatedBranches stays nil) must still sweep the applier's + // namespace — the sweep is gated on IssueNumber alone. + scmDriver := &fakeCleanupSCM{openPRs: []forge.ChangeProposal{ + {Number: 71, Head: "agent/7-impl"}, + }} + w := &world.World{ + RepoOwner: "org", + RepoName: "repo", + IssueNumber: 7, + SCM: scmDriver, + } + CleanupScenario(w) + + var closed []int + for _, rec := range scmDriver.closedIssues { + closed = append(closed, rec.number) + } + assert.Contains(t, closed, 71) +} diff --git a/pkg/behaviourtest/steps/dispatch_test.go b/pkg/behaviourtest/steps/dispatch_test.go index 001c39ff92..765bd4bba2 100644 --- a/pkg/behaviourtest/steps/dispatch_test.go +++ b/pkg/behaviourtest/steps/dispatch_test.go @@ -192,6 +192,12 @@ func (f *fakeDispatchSCM) CreateForkChangeProposal(context.Context, string, stri } func (f *fakeDispatchSCM) CreateRepo(context.Context, string, string, string) error { return nil } func (f *fakeDispatchSCM) EnsureRepoPublic(context.Context, string, string) error { return nil } +func (f *fakeDispatchSCM) ListOpenChangeProposals(context.Context, string, string) ([]forge.ChangeProposal, error) { + return nil, nil +} +func (f *fakeDispatchSCM) ListComments(context.Context, string, string, int) ([]forge.IssueComment, error) { + return nil, nil +} func (f *fakeDispatchSCM) GetDefaultBranch(context.Context, string, string) (string, error) { return "main", nil } diff --git a/pkg/behaviourtest/steps/dummy_agent.go b/pkg/behaviourtest/steps/dummy_agent.go index b6c3bfe134..40307fa9f5 100644 --- a/pkg/behaviourtest/steps/dummy_agent.go +++ b/pkg/behaviourtest/steps/dummy_agent.go @@ -63,6 +63,17 @@ func parseDummyAgentTable(w *world.World, table *godog.Table) error { Op: strings.TrimSpace(row.Cells[col["op"]].Value), Args: strings.TrimSpace(row.Cells[col["args"]].Value), } + // expansion is scoped to checkout_branch: it is the only + // op whose args embed the scenario issue number, and keeping the + // substitution off the generic ops avoids surprising rewrites of + // paths or URLs that happen to contain the token. + if op.Op == "checkout_branch" { + args, err := expandIssuePlaceholder(w, op.Args) + if err != nil { + return err + } + op.Args = args + } if op.Op == "write_fixture" { parts := strings.SplitN(op.Args, ",", 2) if len(parts) != 2 { diff --git a/pkg/behaviourtest/steps/fork_test.go b/pkg/behaviourtest/steps/fork_test.go index 477801f5dd..5cb1a1427d 100644 --- a/pkg/behaviourtest/steps/fork_test.go +++ b/pkg/behaviourtest/steps/fork_test.go @@ -545,6 +545,14 @@ func (f *fakeForkSCM) CreateRepo(context.Context, string, string, string) error return nil } +func (f *fakeForkSCM) ListOpenChangeProposals(context.Context, string, string) ([]forge.ChangeProposal, error) { + return nil, nil +} + +func (f *fakeForkSCM) ListComments(context.Context, string, string, int) ([]forge.IssueComment, error) { + return nil, nil +} + func (f *fakeForkSCM) EnsureRepoPublic(context.Context, string, string) error { return nil } diff --git a/pkg/behaviourtest/steps/registry.go b/pkg/behaviourtest/steps/registry.go index 77dc6b7f88..46121f510a 100644 --- a/pkg/behaviourtest/steps/registry.go +++ b/pkg/behaviourtest/steps/registry.go @@ -14,4 +14,5 @@ func Register(sc *godog.ScenarioContext) { registerURLDispatchSteps(sc) registerForkSteps(sc) registerJiraPollSteps(sc) + registerBranchSteps(sc) } diff --git a/pkg/behaviourtest/steps/url_dispatch_test.go b/pkg/behaviourtest/steps/url_dispatch_test.go index 44c21aea08..7ae9787f9f 100644 --- a/pkg/behaviourtest/steps/url_dispatch_test.go +++ b/pkg/behaviourtest/steps/url_dispatch_test.go @@ -814,6 +814,14 @@ func (f *fakeURLSCM) CreateRepo(_ context.Context, _, name, _ string) error { return nil } +func (f *fakeURLSCM) ListOpenChangeProposals(context.Context, string, string) ([]forge.ChangeProposal, error) { + return nil, nil +} + +func (f *fakeURLSCM) ListComments(context.Context, string, string, int) ([]forge.IssueComment, error) { + return nil, nil +} + func (f *fakeURLSCM) EnsureRepoPublic(_ context.Context, _, _ string) error { f.ensurePublicCalled = true return f.ensurePublicErr diff --git a/pkg/behaviourtest/suite/init.go b/pkg/behaviourtest/suite/init.go index 2e0e7d8e8c..65e178816b 100644 --- a/pkg/behaviourtest/suite/init.go +++ b/pkg/behaviourtest/suite/init.go @@ -95,6 +95,9 @@ func resetScenarioWorld(w *world.World) { w.ForkPRBranch = "" w.URLHarnessRepoOwner = "" w.URLHarnessRepoName = "" + w.RecordedBranchSHAs = nil + w.CreatedBranches = nil + w.CreatedPRNumbers = nil w.LeasedRepoName = "" w.KillSwitchActivated = false w.JiraMockServer = nil @@ -123,6 +126,19 @@ func SkipErrorForTagNames(tags []string, w *world.World) error { return godog.ErrSkip case name == "skip:gitlab" && w.Config.SCM == "gitlab": return godog.ErrSkip + case strings.HasPrefix(name, "requires:capability:"): + // Skip unless the runner declares the capability via + // BEHAVIOUR_CAPABILITIES. Gates scenarios that assert + // behavior only present past a dependency version, so CI + // stays green until the dependency ships and the runner + // opts in. + capability := strings.TrimPrefix(name, "requires:capability:") + if capability == "" { + return fmt.Errorf("malformed tag %q: requires:capability: needs a name", tag) + } + if !w.Config.HasCapability(capability) { + return godog.ErrSkip + } } } return nil diff --git a/pkg/behaviourtest/suite/init_test.go b/pkg/behaviourtest/suite/init_test.go index 52cdd0f300..781e5aaf48 100644 --- a/pkg/behaviourtest/suite/init_test.go +++ b/pkg/behaviourtest/suite/init_test.go @@ -55,6 +55,12 @@ func (p *panickingSCM) CloseIssue(context.Context, string, string, int) error { } func (p *panickingSCM) CreateRepo(context.Context, string, string, string) error { return nil } func (p *panickingSCM) EnsureRepoPublic(context.Context, string, string) error { return nil } +func (p *panickingSCM) ListOpenChangeProposals(context.Context, string, string) ([]forge.ChangeProposal, error) { + return nil, nil +} +func (p *panickingSCM) ListComments(context.Context, string, string, int) ([]forge.IssueComment, error) { + return nil, nil +} func (p *panickingSCM) GetDefaultBranch(context.Context, string, string) (string, error) { return "main", nil } @@ -119,6 +125,11 @@ func TestSkipErrorForTagNames(t *testing.T) { {name: "requires per-repo on per-org", tags: []string{"@requires:per-repo"}, wantErr: godog.ErrSkip, cfg: env.RunnerConfig{InstallMode: "per-org"}}, {name: "skip gitlab on github", tags: []string{"@skip:gitlab"}, wantErr: nil}, {name: "skip gitlab on gitlab", tags: []string{"@skip:gitlab"}, wantErr: godog.ErrSkip, cfg: env.RunnerConfig{SCM: "gitlab"}}, + {name: "requires capability undeclared", tags: []string{"@requires:capability:applier-branch-namespace"}, wantErr: godog.ErrSkip}, + {name: "requires capability declared", tags: []string{"@requires:capability:applier-branch-namespace"}, wantErr: nil, + cfg: env.RunnerConfig{InstallMode: "per-repo", SCM: "github", Capabilities: []string{"applier-branch-namespace"}}}, + {name: "requires capability other declared", tags: []string{"@requires:capability:applier-branch-namespace"}, wantErr: godog.ErrSkip, + cfg: env.RunnerConfig{InstallMode: "per-repo", SCM: "github", Capabilities: []string{"something-else"}}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -136,6 +147,14 @@ func TestSkipErrorForTagNames(t *testing.T) { } } +func TestSkipErrorForTagNames_MalformedCapabilityTag(t *testing.T) { + w := &world.World{Config: env.RunnerConfig{InstallMode: "per-repo", SCM: "github"}} + err := SkipErrorForTagNames([]string{"@requires:capability:"}, w) + require.Error(t, err) + assert.NotErrorIs(t, err, godog.ErrSkip, "an empty capability name is a tag-authoring mistake, not a normal skip") + assert.Contains(t, err.Error(), "needs a name") +} + // --- Before/After hook tests --- func TestBeforeScenario_ClonesAndResetsWorld(t *testing.T) { diff --git a/pkg/behaviourtest/world/world.go b/pkg/behaviourtest/world/world.go index 22654d3f73..5ae8700084 100644 --- a/pkg/behaviourtest/world/world.go +++ b/pkg/behaviourtest/world/world.go @@ -55,6 +55,19 @@ type World struct { URLHarnessRepoOwner string URLHarnessRepoName string + // Branch-handling scenario state — set by branch step definitions. + // RecordedBranchSHAs maps branch name → tip SHA captured before a + // run so "branch X is unchanged" can re-check it afterwards. + // CreatedBranches and CreatedPRNumbers track resources the branch + // steps created (or discovered) so CleanupScenario can remove them. + // Isolation across Clone()d Worlds relies on the suite invariant + // that resetScenarioWorld nils these after every clone and the + // template World never populates them — do not set them on a + // template. + RecordedBranchSHAs map[string]string + CreatedBranches []string + CreatedPRNumbers []int + // LeasedRepoName is the logical test-repo name acquired from a RepoPool // for this scenario's duration. Empty when no pool is configured. LeasedRepoName string