Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/guides/dev/behaviour-drivers.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
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.

Expand Down
53 changes: 52 additions & 1 deletion docs/guides/dev/behaviour-testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<issue>` 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`).
Expand All @@ -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/<issue>-.*"
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:<name>` 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`.
Expand Down Expand Up @@ -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`.

Expand Down Expand Up @@ -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.
56 changes: 56 additions & 0 deletions e2e/behaviour/features/code/branch-namespace.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
Feature: Code applier branch handling

The code applier must land pushes inside the dispatched issue's
agent/<issue>-* 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/<issue>-.* 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/<issue>-.*"
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/<issue>-impl" seeded with a commit
And a dummy agent that would:
| description | op | args |
| Check out namespaced branch | checkout_branch | agent/<issue>-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/<issue>-impl"
4 changes: 4 additions & 0 deletions e2e/behaviour/fixtures/code/implemented.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"target_branch": "main",
"pr_body": "Automated behaviour-test implementation emitted by the scripted code run."
}
63 changes: 63 additions & 0 deletions internal/runtime/dummy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -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 {
Expand Down
Loading
Loading