refactor(behaviour): extract pkg/behaviourtest and pkg/e2etest public APIs

[ifireball]

Summary

• Extract shared live-test infrastructure (org pool, CLI runner, cleanup, auth) from e2e/admin/ into importable pkg/e2etest/
• Extract Gherkin behaviour framework (world, steps, drivers, artifacts, suite bootstrap) from e2e/behaviour/ into pkg/behaviourtest/
• Leave thin runners in e2e/admin/admin_test.go and e2e/behaviour/suite_test.go; external repos import only pkg/*
• Add BuildModuleBinary for external consumers pinning this module; parameterize fixture lookup via world.FixturesRoot
• Update CI path filters, Codecov ignore list, and developer docs (version-pin guidance)

Related Issue

Closes #3784

Changes

• pkg/e2etest/ — pool acquire/release, env config, CLI build/run, cleanup, mint auth (moved from e2e/admin)
• pkg/behaviourtest/ — drivers, world, steps, suite.InitScenario (moved from e2e/behaviour)
• e2e/admin/helpers.go — admin-e2e-only helpers (labels, repo cleanup registration)
• .github/workflows/e2e.yml — trigger e2e/behaviour jobs on pkg/e2etest/ and pkg/behaviourtest/ changes

Test plan

• make go-test — unit tests pass (including moved tests in pkg/)
• make go-vet
• make lint (staged)
• CI e2e job — admin install e2e
• CI behaviour job — Gherkin triage scenario
• Codecov patch ≥80%

Made with Cursor

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:34 PM UTC · Ended 10:38 PM UTC
Commit: 5cd495a · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Extract behaviour/e2e frameworks into importable pkg/behaviourtest and pkg/e2etest

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Extract shared org-pool, CLI build/run, auth, and cleanup into pkg/e2etest.
• Extract Godog behaviour framework (world, steps, drivers, suite init) into pkg/behaviourtest.
• Update thin in-repo runners, CI path filters, Codecov ignores, and dev docs for external use.

Diagram

graph TD
  Admin["e2e/admin runner"] --> E2E["pkg/e2etest"] --> CLI(["fullsend CLI"]) --> GH{{"GitHub API"}}
  Behaviour["e2e/behaviour runner"] --> BT["pkg/behaviourtest"] --> E2E
  E2E --> Mint{{"Mint service"}}
  subgraph Legend
    direction LR
    _run["Runner"] ~~~ _pkg["Pkg API"] ~~~ _bin(["Binary"]) ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Split into a separate Go module for test frameworks

• ➕ Cleaner dependency boundary for external consumers
• ➕ Independent semantic versioning for test APIs
• ➖ Extra release/process overhead
• ➖ More cross-repo coordination when internal types change

2. Keep implementations under e2e/ and only add wrapper pkg re-exports

• ➕ Less churn from file moves
• ➕ Preserves existing in-repo import structure
• ➖ Still forces primary implementation to live under e2e/ (awkward for external use)
• ➖ Wrappers can drift and complicate ownership

3. Provide a single supported external runner (no exported building blocks)

• ➕ Smaller public API surface
• ➕ Less support burden for custom runners
• ➖ Doesn't meet goal of external repos composing drivers/world/fixtures and orchestration
• ➖ Harder to integrate into other repos’ test harnesses

Recommendation: The chosen approach—moving the real implementations into pkg/e2etest and pkg/behaviourtest and leaving thin e2e/* runners—is the best match for external reuse with minimal duplication. Consider a separate module only if the public API surface grows enough to warrant independent versioning.

Files changed (39) +489 / -320

Enhancement (9) +139 / -124

find.goExtract behaviour artifact discovery into pkg API +0/-0

Extract behaviour artifact discovery into pkg API

• Provides artifact lookup helpers under pkg/behaviourtest so external runners/steps can locate behaviour results and output files.

pkg/behaviourtest/artifacts/find.go

driver.goDefine CI driver interface under pkg/behaviourtest +0/-0

Define CI driver interface under pkg/behaviourtest

• Exposes the CI driver interface used by behaviour steps and suite wiring.

pkg/behaviourtest/drivers/ci/driver.go

env.goExpose runner configuration types/helpers +0/-0

Expose runner configuration types/helpers

• Adds runner configuration under pkg/behaviourtest to support both in-repo and external suite runners.

pkg/behaviourtest/drivers/env/env.go

driver.goDefine install driver interface and state contract +0/-0

Define install driver interface and state contract

• Exposes install abstractions used by world/steps to support different install modes behind a stable interface.

pkg/behaviourtest/drivers/install/driver.go

driver.goDefine SCM driver interface under pkg/behaviourtest +0/-0

Define SCM driver interface under pkg/behaviourtest

• Exposes SCM operations needed by behaviour steps (issues, comments, repo content).

pkg/behaviourtest/drivers/scm/driver.go

dummy_agent.goRequire world.FixturesRoot for fixture lookup +7/-5

Require world.FixturesRoot for fixture lookup

• Removes hard-coded fixture root and validates FixturesRoot is set, enabling external repos to place fixtures under their own module-relative directory.

pkg/behaviourtest/steps/dummy_agent.go

init.goAdd suite.InitScenario for hooks + tag-skip logic +62/-0

Add suite.InitScenario for hooks + tag-skip logic

• Centralizes Before/After hooks, state resets, compatibility tag skipping, and step registration so runners can share scenario bootstrap logic.

pkg/behaviourtest/suite/init.go

world.goMove World to pkg and add FixturesRoot field +7/-4

Move World to pkg and add FixturesRoot field

• Extracts the scenario state container and introduces FixturesRoot to parameterize fixture resolution for external consumers.

pkg/behaviourtest/world/world.go

testutil.goPublic e2e utilities: EnvConfig, AcquireOrg, CLI runners, module-aware builds +63/-115

Public e2e utilities: EnvConfig, AcquireOrg, CLI runners, module-aware builds

• Exports EnvConfig, ErrAllOrgsRateLimited, TestRepo, AcquireOrg/OrgPool/NewLiveClient, and refactors CLI helpers. Adds BuildModuleBinary and ModuleRoot/moduleDir helpers to support external repos pinning this module while building cmd/fullsend.

pkg/e2etest/testutil.go

Refactor (15) +196 / -167

admin_test.goUse pkg/e2etest for env, pool/locks, CLI, and cleanup +41/-40

Use pkg/e2etest for env, pool/locks, CLI, and cleanup

• Replaces local admin e2e helpers with pkg/e2etest APIs (LoadEnvConfig, BuildCLIBinary, AcquireOrg/ReleaseLock, CleanupStaleResources, RunCLI/TryRunCLIWithT) and standardizes test repo references via e2etest.TestRepo.

e2e/admin/admin_test.go

helpers.goAdd admin-only helpers (labels and repo cleanup) +91/-0

Add admin-only helpers (labels and repo cleanup)

• Introduces admin-scoped helpers (ensureRepoLabel/addIssueLabel and registerRepoCleanup) that were removed from shared utilities during extraction to keep pkg/e2etest focused on broadly reusable infra.

e2e/admin/helpers.go

suite_test.goThin behaviour runner wired to pkg/behaviourtest + pkg/e2etest +27/-62

Thin behaviour runner wired to pkg/behaviourtest + pkg/e2etest

• Switches imports from e2e/behaviour and e2e/admin to pkg/behaviourtest and pkg/e2etest, delegates scenario initialization to suite.InitScenario, and sets world.FixturesRoot for fixture resolution.

e2e/behaviour/suite_test.go

githubactions.goMove GitHub Actions CI driver to pkg/behaviourtest +1/-1

Move GitHub Actions CI driver to pkg/behaviourtest

• Updates imports and package paths so the GitHub Actions CI driver is consumable from pkg/behaviourtest.

pkg/behaviourtest/drivers/ci/githubactions/githubactions.go

factory.goInstall driver factory now takes e2etest.EnvConfig +3/-5

Install driver factory now takes e2etest.EnvConfig

• Removes dependency on e2e/admin by switching the factory signature to accept pkg/e2etest.EnvConfig.

pkg/behaviourtest/drivers/install/factory.go

perrepo_github.goPer-repo install driver uses e2etest CLI helpers and teardown +7/-9

Per-repo install driver uses e2etest CLI helpers and teardown

• Replaces admin TryRunCLI/Teardown usage with pkg/e2etest equivalents and updates EnvConfig typing accordingly.

pkg/behaviourtest/drivers/install/perrepo_github.go

github.goMove GitHub SCM driver implementation to pkg path +1/-1

Move GitHub SCM driver implementation to pkg path

• Updates the GitHub SCM implementation to import the SCM interface from pkg/behaviourtest.

pkg/behaviourtest/drivers/scm/github/github.go

parse.goRelocate SCM parsing helpers into pkg/behaviourtest +0/-0

Relocate SCM parsing helpers into pkg/behaviourtest

• Moves parsing utilities used by SCM wiring/tests into the extracted package.

pkg/behaviourtest/drivers/scm/parse.go

artifacts.goUpdate steps to use pkg/behaviourtest artifacts/world +2/-2

Update steps to use pkg/behaviourtest artifacts/world

• Repoints step implementations to the extracted artifacts and world packages for external reuse.

pkg/behaviourtest/steps/artifacts.go

cleanup.goExtract scenario cleanup helper into shared steps package +1/-1

Extract scenario cleanup helper into shared steps package

• Keeps CleanupScenario in pkg/behaviourtest/steps so any runner can invoke consistent teardown behavior.

pkg/behaviourtest/steps/cleanup.go

registry.goExpose step registration entrypoint under pkg/behaviourtest +1/-1

Expose step registration entrypoint under pkg/behaviourtest

• Moves the step registration function to the extracted package and updates imports to the new world type.

pkg/behaviourtest/steps/registry.go

triage.goUpdate triage steps to pkg driver/world packages +2/-2

Update triage steps to pkg driver/world packages

• Repoints triage step dependencies to the extracted SCM driver and world types.

pkg/behaviourtest/steps/triage.go

auth.goExtract auth/token resolution into pkg/e2etest +1/-3

Extract auth/token resolution into pkg/e2etest

• Provides shared token resolution (local vs GHA) and mint token minting helpers for operating on pool orgs.

pkg/e2etest/auth.go

cleanup.goExtract stale-resource cleanup and per-repo teardown +17/-35

Extract stale-resource cleanup and per-repo teardown

• Moves shared cleanup logic from admin e2e into pkg/e2etest, standardizing on TestRepo and providing TeardownPerRepoInstall for reuse by behaviour install drivers.

pkg/e2etest/cleanup.go

lock.goMove org lock implementation into shared pkg/e2etest +1/-5

Move org lock implementation into shared pkg/e2etest

• Hosts distributed lock acquisition/release and stale-lock reclaim logic used by both admin and behaviour runners.

pkg/e2etest/lock.go

Tests (9) +101 / -10

githubactions_test.goAdd/relocate tests for GitHub Actions CI driver +0/-0

Add/relocate tests for GitHub Actions CI driver

• Ensures the CI driver continues to be validated under the extracted package path.

pkg/behaviourtest/drivers/ci/githubactions/githubactions_test.go

perrepo_github_test.goUpdate tests for per-repo install driver extraction +0/-2

Update tests for per-repo install driver extraction

• Keeps the per-repo install logic covered after moving into pkg/behaviourtest.

pkg/behaviourtest/drivers/install/perrepo_github_test.go

parse_test.goRelocate SCM parsing tests +0/-0

Relocate SCM parsing tests

• Maintains unit test coverage for SCM parsing after extraction.

pkg/behaviourtest/drivers/scm/parse_test.go

cleanup_test.goTests for cleanup behavior under pkg path +0/-0

Tests for cleanup behavior under pkg path

• Preserves unit coverage for cleanup helpers after moving packages.

pkg/behaviourtest/steps/cleanup_test.go

dummy_agent_test.goAdd tests for FixturesRoot requirement and module-subdir resolution +32/-0

Add tests for FixturesRoot requirement and module-subdir resolution

• Adds coverage to ensure dummy agent script parsing fails without FixturesRoot and that module-relative directory discovery works.

pkg/behaviourtest/steps/dummy_agent_test.go

init_test.goUnit test tag-based skip semantics +45/-0

Unit test tag-based skip semantics

• Adds table-driven tests validating skip/require tags against runner config (per-org/per-repo and SCM vendor).

pkg/behaviourtest/suite/init_test.go

auth_test.goAuth helper tests moved under pkg/e2etest +1/-3

Auth helper tests moved under pkg/e2etest

• Ensures auth behavior remains tested after the package rename/extraction.

pkg/e2etest/auth_test.go

cli_test.goAdd tests for CLI build helpers +20/-0

Add tests for CLI build helpers

• Adds tests for BuildCLIBinary and the new BuildModuleBinary helper for pinned-module consumers.

pkg/e2etest/cli_test.go

lock_test.goUpdate lock tests for exported sentinel error naming +3/-5

Update lock tests for exported sentinel error naming

• Repoints tests to pkg/e2etest and updates assertions to use ErrAllOrgsRateLimited.

pkg/e2etest/lock_test.go

Documentation (4) +45 / -15

0066-behaviour-tests-with-gherkin-and-drivers.mdDocument new locations for shared behaviour/e2e code +1/-0

Document new locations for shared behaviour/e2e code

• Notes that shared live-test infrastructure lives in pkg/e2etest and the Gherkin framework lives in pkg/behaviourtest, with runners staying under e2e/.

docs/ADRs/0066-behaviour-tests-with-gherkin-and-drivers.md

behaviour-drivers.mdUpdate driver docs to pkg/behaviourtest and e2etest teardown +11/-11

Update driver docs to pkg/behaviourtest and e2etest teardown

• Repoints documented driver package paths to pkg/behaviourtest, updates runner description to mention pkg/e2etest acquisition/cleanup and suite.InitScenario usage, and updates teardown guidance to use e2etest.TeardownPerRepoInstall.

docs/guides/dev/behaviour-drivers.md

behaviour-testing.mdDescribe shared framework layout and external version pinning +31/-4

Describe shared framework layout and external version pinning

• Adds a section describing pkg/behaviourtest + pkg/e2etest as importable shared libraries, clarifies e2e/behaviour as a thin runner, and adds external pinning guidance including FixturesRoot and BuildModuleBinary usage.

docs/guides/dev/behaviour-testing.md

e2e-testing.mdCall out pkg/e2etest as shared admin/behaviour infrastructure +2/-0

Call out pkg/e2etest as shared admin/behaviour infrastructure

• Documents that org pool, CLI, and cleanup helpers are now centralized in pkg/e2etest, while admin-specific logic remains under e2e/admin.

docs/guides/dev/e2e-testing.md

Other (2) +8 / -4

.codecov.ymlRefine Codecov ignores for behaviour suite assets +3/-1

Refine Codecov ignores for behaviour suite assets

• Replaces the broad ignore of e2e/behaviour/** with targeted ignores for suite_test.go and feature/fixture directories, so extracted pkg code can be covered while still excluding non-Go assets.

.codecov.yml

e2e.ymlTrigger e2e/behaviour jobs on new pkg API changes +5/-3

Trigger e2e/behaviour jobs on new pkg API changes

• Updates workflow path filters and 'relevant files' detection to include pkg/e2etest and pkg/behaviourtest, ensuring CI runs when extracted libraries change.

.github/workflows/e2e.yml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (1)

Context used

✅ Tickets: 🎫 feat(behaviour): extract pkg/behaviourtest public API

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Direct GitHub API in helpers.go ✗ Dismissed 📘 Rule violation ⌂ Architecture

Description

e2e/admin/helpers.go and e2e/admin/admin_test.go construct https://api.github.com/... URLs and
use http.DefaultClient to make direct GitHub REST calls, bypassing the centralized forge
abstractions. This violates the compliance requirement that GitHub API callsites live under
internal/forge/github and that forge operations route through forge.Client for consistency and
auditability.

Code

e2e/admin/helpers.go[R23-40]

+func ensureRepoLabel(ctx context.Context, token, owner, repo, label string) error {
+	url := fmt.Sprintf("https://api.github.com/repos/%s/%s/labels", owner, repo)
+	payload, err := json.Marshal(map[string]string{
+		"name":  label,
+		"color": "5319e7",
+	})
+	if err != nil {
+		return fmt.Errorf("encoding label payload: %w", err)
+	}
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload))
+	if err != nil {
+		return fmt.Errorf("creating label request: %w", err)
+	}
+	req.Header.Set("Authorization", "Bearer "+token)
+	req.Header.Set("Accept", "application/vnd.github+json")
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := http.DefaultClient.Do(req)

Relevance

⭐⭐⭐ High

Team codified/enforces forge.Client boundary; GitHub API should live under internal/forge/github
(#1304).

PR-#1304

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance rules state that GitHub REST call construction must be confined to
internal/forge/github and that forge operations should go through forge.Client. In the cited
areas, the code builds GitHub REST API URLs (https://api.github.com/...) and executes requests
directly via http.DefaultClient.Do(...) (including fetching issue labels and performing
label-related operations), demonstrating GitHub API usage outside the allowed boundary and without
using the forge.Client abstraction.

Rule 1062052: Route all git forge operations through forge.Client
Rule 1062054: Restrict direct GitHub API calls to internal/forge/github
e2e/admin/helpers.go[23-76]
e2e/admin/admin_test.go[603-609]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Direct GitHub REST API calls are being made from `e2e/admin/helpers.go` and `e2e/admin/admin_test.go` by constructing `https://api.github.com/...` URLs and issuing requests with `net/http` (`http.DefaultClient.Do(...)`). This is disallowed because GitHub API callsites must be confined to `internal/forge/github`, and forge operations must be routed through `forge.Client` for consistency and auditability.

## Issue Context
The repository has compliance requirements to (1) restrict GitHub-specific REST call construction/callsites to `internal/forge/github` and (2) route all forge operations through `forge.Client`. The current e2e admin helper/test code bypasses these abstractions by calling the GitHub REST API directly.

## Fix Focus Areas
- e2e/admin/helpers.go[23-76]
- e2e/admin/admin_test.go[603-608]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. CLI build tests untagged ✓ Resolved 🐞 Bug ➹ Performance

Description

pkg/e2etest/cli_test.go has no build tags and will run in the default go test ./... suite, but it
shells out to go build twice to compile cmd/fullsend, increasing unit-test runtime and potentially
failing in constrained CI/dev environments.

Code

pkg/e2etest/cli_test.go[R1-20]

+package e2etest
+
+import (
+	"os"
+	"testing"
+)
+
+func TestBuildCLI(t *testing.T) {
+	binary := BuildCLIBinary(t)
+	if _, err := os.Stat(binary); err != nil {
+		t.Fatalf("binary not found at %s: %v", binary, err)
+	}
+}
+
+func TestBuildModuleBinary(t *testing.T) {
+	binary := BuildModuleBinary(t, "github.com/fullsend-ai/fullsend")
+	if _, err := os.Stat(binary); err != nil {
+		t.Fatalf("binary not found at %s: %v", binary, err)
+	}
+}

Relevance

⭐⭐⭐ High

Pattern: build-heavy CLI tests were placed behind e2e build tags in e2e/admin (see #2277).

PR-#2277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test file has no //go:build constraint and calls build helpers that execute go build via
exec.Command, making this work part of the default unit test run.

pkg/e2etest/cli_test.go[1-20]
pkg/e2etest/testutil.go[426-479]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
These tests compile the CLI via subprocess `go build` and currently run in the default unit test suite (no build tags / no short-mode gating). This is a regression from the prior e2e-tagged location and can slow or destabilize normal unit-test runs.

## Issue Context
The tests call `BuildCLIBinary` and `BuildModuleBinary`, both of which execute `go build`.

## Fix Focus Areas
- Add an appropriate build tag (e.g. `//go:build e2e`) to `pkg/e2etest/cli_test.go`, OR
- Skip in short mode (`if testing.Short() { t.Skip(...) }`), OR
- Require an explicit env var to run (fail closed).

- pkg/e2etest/cli_test.go[1-20]
- pkg/e2etest/testutil.go[426-479]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Accepted ADR adds new consequence ✓ Resolved 📜 Skill insight § Compliance

Description

docs/ADRs/0066-... is Accepted, but this PR adds a new Consequences bullet that updates the
ADR’s substantive content. Accepted ADRs should only receive minimal annotations (e.g.,
notes/cross-references) rather than new decision/consequence content.

Code

docs/ADRs/0066-behaviour-tests-with-gherkin-and-drivers.md[41]

+- Shared live-test infrastructure (org pool, CLI runner, cleanup) lives in `pkg/e2etest/`; the Gherkin framework lives in `pkg/behaviourtest/`. In-repo runners remain under `e2e/behaviour/` and `e2e/admin/`.

Relevance

⭐⭐ Medium

Mixed: accepted “don’t change accepted ADRs” in #2465/#1982, but similar warning rejected in #2277.

PR-#2465
PR-#1982
PR-#2277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The ADR’s frontmatter and body mark it as Accepted, and the diff adds a new bullet under `##
Consequences`, which is a substantive modification not limited to typos or cross-reference notes.

Rule 1062057: Restrict modifications to accepted ADRs on main
docs/ADRs/0066-behaviour-tests-with-gherkin-and-drivers.md[1-43]
Skill: writing-adrs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An `Accepted` ADR was updated by adding a new Consequences bullet. Compliance requires that accepted ADRs not have substantive content changed; only small annotations/cross-references/notes are permitted.

## Issue Context
The added line is describing an implementation refactor (moving code into `pkg/e2etest` and `pkg/behaviourtest`). If this information is important, it should be recorded as an annotation-style note (or a new ADR that supersedes/extends the original), rather than expanding the original Consequences list.

## Fix Focus Areas
- docs/ADRs/0066-behaviour-tests-with-gherkin-and-drivers.md[35-43]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Retry CLI output unlogged ✓ Resolved 🐞 Bug ◔ Observability

Description

e2etest.TryRunCLIWithT claims to “log via t” but does not log the command/output; callers that retry
and ignore intermediate errors (e.g. admin analyze retries) won’t emit per-attempt CLI output to
test logs, reducing debuggability of transient failures.

Code

pkg/e2etest/testutil.go[R496-500]

+// TryRunCLIWithT is like TryRunCLI but logs via t and uses ModuleRoot as cwd.
+func TryRunCLIWithT(t *testing.T, binary, token string, args ...string) (string, error) {
	t.Helper()
-	modRoot, err := exec.Command("go", "list", "-m", "-f", "{{.Dir}}").Output()
-	if err != nil {
-		t.Fatalf("finding module root: %v", err)
-	}
-	binary := filepath.Join(t.TempDir(), "fullsend")
-	cmd := exec.Command("go", "build", "-o", binary, "./cmd/fullsend/")
-	cmd.Dir = strings.TrimSpace(string(modRoot))
-	out, err := cmd.CombinedOutput()
-	if err != nil {
-		t.Fatalf("building fullsend binary: %s\n%s", err, out)
-	}
-	return binary
-}
-
-// runCLI executes the fullsend CLI with the given args, passing GITHUB_TOKEN.
-// By default the working directory is the module root. Use runCLIFromDir to
-// run from a subdirectory (GOMOD discovery makes this work for vendoring).
-func runCLI(t *testing.T, binary, token string, args ...string) string {
-	return runCLIFromDir(t, binary, token, moduleRoot(t), args...)
+	return tryRunCLIFromDir(ModuleRoot(t), binary, token, args...)
}

Relevance

⭐⭐ Medium

No clear precedent requiring per-attempt CLI output logging; logging-related review history is mixed
(#1215).

PR-#1215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

TryRunCLIWithT just delegates to tryRunCLIFromDir (which has no t.Logf). The admin analyze retry
loop uses TryRunCLIWithT but doesn’t log intermediate outputs/errors before retrying.

pkg/e2etest/testutil.go[496-512]
e2e/admin/admin_test.go[142-160]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TryRunCLIWithT` does not actually log through `testing.T`, and retry loops that discard intermediate errors won’t surface the CLI output for failed attempts.

## Issue Context
The helper returns output+error, but without logging inside the helper or at the call site, transient failures can be invisible until the final attempt.

## Fix Focus Areas
- Either implement logging in `TryRunCLIWithT` (at least on error), or
- Update the comment/name to reflect behavior and update retry call sites to log `out`/`err` on each failed attempt.

- pkg/e2etest/testutil.go[496-512]
- e2e/admin/admin_test.go[142-160]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Guides added under docs/guides/dev 📜 Skill insight ⌂ Architecture

Description

Modified guide files live under docs/guides/dev/, but guides are required to be placed under
either docs/guides/admin/ or docs/guides/user/. Keeping them in dev/ violates the guide
directory/audience placement requirement.

Code

docs/guides/dev/behaviour-testing.md[R17-27]

+Shared framework (importable by external repos):
+
+```
+pkg/behaviourtest/
+  world/             # Scenario state
+  steps/             # Step definitions + CleanupScenario
+  artifacts/         # Artifact lookup helpers
+  drivers/           # SCM, CI, env, install interfaces + v1 impls
+  suite/             # InitScenario (tags, hooks, step registration)
+pkg/e2etest/         # Org pool, CLI runner, cleanup (shared with admin e2e)
+```

Relevance

⭐ Low

Repo routinely maintains dev guides under docs/guides/dev; changes accepted previously (e.g.,
#1252).

PR-#1252

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The rule requires every guide under docs/guides/ to be located in either admin/ or user/. The
PR modifies multiple guides under docs/guides/dev/, demonstrating non-compliant placement.

docs/guides/dev/behaviour-testing.md[1-35]
docs/guides/dev/behaviour-drivers.md[1-20]
docs/guides/dev/e2e-testing.md[1-50]
Skill: writing-user-docs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Documentation guides under `docs/guides/` must be placed in either the `admin/` or `user/` subdirectory. This PR modifies guides in `docs/guides/dev/`, which violates the required structure.

## Issue Context
These documents appear to be developer/user-facing guides (not org-admin installation guides), so they likely belong under `docs/guides/user/` (unless you intend them for administrators).

## Fix Focus Areas
- docs/guides/dev/behaviour-testing.md[1-120]
- docs/guides/dev/behaviour-drivers.md[1-80]
- docs/guides/dev/e2e-testing.md[1-60]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 10:39 PM UTC · Completed 10:53 PM UTC
Commit: 6a06f21 · View workflow run →

[fullsend-ai-review]

Review

Clean mechanical extraction. The refactoring correctly preserves all runtime behavior — resetScenarioWorld resets the exact same 10 fields as the original initializeScenario, all renamed identifiers are consistently updated with no stale references in Go source, build tag placement is intentional (confirmed by commit 755fc1d), and the dependency graph is acyclic (behaviourtest → e2etest, no reverse). No security, correctness, or cross-repo contract issues found.

Prior review findings resolved:

• [build-tags] build.go build tag is intentional — BuildCLIBinary/BuildModuleBinary invoke go build and should not run during default go test. Addressed by commit 755fc1d.
• [world-fixtures-root-requirement] FixturesRoot is now documented in docs/guides/dev/behaviour-testing.md version-pinning section.
• [documentation-completeness] BuildModuleBinary usage is explained in the version-pinning guidance.

Findings

Medium

• [protected-path] .github/workflows/e2e.yml — PR modifies a CI workflow (protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:887 — References old path e2e/admin/cleanup.go which has been moved to pkg/e2etest/cleanup.go. This is a historical plan document; inline updates are optional but a top-of-file annotation may help future readers.

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:895 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Same context as above.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:219 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Historical plan document.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:269 — References old path e2e/admin/testutil.go in file summary table. Same context as above.

Previous run

Review

Findings

Medium

• [build-tags] pkg/e2etest/build.go:1 — build.go retains //go:build e2e || behaviour while other files in pkg/e2etest/ (lock.go, auth.go, cleanup.go, testutil.go) had their build tags removed. This means BuildCLIBinary and BuildModuleBinary are only available under e2e/behaviour build tags, while LoadEnvConfig and AcquireOrg are unconditionally available. An external consumer importing pkg/e2etest without build tags would get compilation errors when calling build functions.
  Remediation: Remove the build tag from build.go to match other pkg/e2etest files, or document why it intentionally differs.

• [world-fixtures-root-requirement] pkg/behaviourtest/world/world.go — FixturesRoot is a new required field on the World struct. External repos must set it or dummy agent steps fail with "world.FixturesRoot is not set". The behaviour-testing.md version-pinning section documents the field but doesn't explain what fixtures are or how to structure them in an external repo.

• [protected-path] .github/workflows/e2e.yml — PR modifies .github/workflows/e2e.yml (a protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to the e2e workflow's path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:887 — References old path e2e/admin/cleanup.go which has been moved to pkg/e2etest/cleanup.go. This is a historical plan document; inline updates are optional but a top-of-file annotation may help future readers.

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:895 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Same context as above.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:219 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Historical plan document.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:269 — References old path e2e/admin/testutil.go in file summary table. Same context as above.

• [documentation-completeness] docs/guides/dev/behaviour-testing.md:180 — Version-pinning guidance instructs external consumers to use BuildModuleBinary instead of BuildCLIBinary but the explanation of why is terse. Adding that BuildCLIBinary resolves the calling module's root (not the pinned fullsend module) would help new users.

---

Labels: PR extracts shared e2e test infrastructure into public packages, modifying test code and CI workflow

Previous run

Review

Findings

Medium

• [build-tags] pkg/e2etest/build.go:1 — build.go retains //go:build e2e || behaviour while other files in pkg/e2etest/ (lock.go, auth.go, cleanup.go, testutil.go) had their build tags removed. This means BuildCLIBinary and BuildModuleBinary are only available under e2e/behaviour build tags, while LoadEnvConfig and AcquireOrg are unconditionally available. An external consumer importing pkg/e2etest without build tags would get compilation errors when calling build functions.
  Remediation: Remove the build tag from build.go to match other pkg/e2etest files, or document why it intentionally differs.

• [world-fixtures-root-requirement] pkg/behaviourtest/world/world.go — FixturesRoot is a new required field on the World struct. External repos must set it or dummy agent steps fail with "world.FixturesRoot is not set". The behaviour-testing.md version-pinning section documents the field but doesn't explain what fixtures are or how to structure them in an external repo.

• [protected-path] .github/workflows/e2e.yml — PR modifies .github/workflows/e2e.yml (a protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to the e2e workflow's path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:887 — References old path e2e/admin/cleanup.go which has been moved to pkg/e2etest/cleanup.go. This is a historical plan document; inline updates are optional but a top-of-file annotation may help future readers.

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:895 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Same context as above.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:219 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Historical plan document.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:269 — References old path e2e/admin/testutil.go in file summary table. Same context as above.

• [documentation-completeness] docs/guides/dev/behaviour-testing.md:180 — Version-pinning guidance instructs external consumers to use BuildModuleBinary instead of BuildCLIBinary but the explanation of why is terse. Adding that BuildCLIBinary resolves the calling module's root (not the pinned fullsend module) would help new users.

Previous run (2)

Review

Findings

High

• [logic-error] pkg/behaviourtest/suite/init.go:17 — InitScenario's Before hook does not reset w.PRNumber and w.DispatchAgent between scenarios. The old initializeScenario (removed from e2e/behaviour/suite_test.go) reset both fields (w.PRNumber = 0 and w.DispatchAgent = ""). Since World is shared across scenarios with Concurrency: 1, stale values leak from one scenario to the next. Concretely: if a PR-creating scenario (e.g. "PR does not trigger issue-only harness") runs before an issue-only scenario, the leaked non-zero PRNumber causes whenPullRequestLabeled to label the stale PR instead of failing, silently changing the test's semantics. Similarly, a leaked DispatchAgent can cause thenHarnessWorkflowCompletes to wait for the wrong agent.
  Remediation: Add w.PRNumber = 0 and w.DispatchAgent = "" to the Before hook reset block in InitScenario, alongside the existing resets for IssueNumber, WorkflowRun, etc.

Medium

• [protected-path] .github/workflows/e2e.yml — PR modifies .github/workflows/e2e.yml (a protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to the e2e workflow's path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:887 — References old path e2e/admin/cleanup.go which has been moved to pkg/e2etest/cleanup.go. This is a historical plan document; inline updates are optional but a top-of-file annotation may help future readers.

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:895 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Same context as above.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:219 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Historical plan document.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:269 — References old path e2e/admin/testutil.go in file summary table. Same context as above.

Previous run (3)

Review

Findings

High

• [logic-error] pkg/behaviourtest/suite/init.go:17 — The new InitScenario function does not reset w.PRNumber and w.DispatchAgent between scenarios. The old initializeScenario in e2e/behaviour/suite_test.go reset both (w.PRNumber = 0 and w.DispatchAgent = ""). Since World is shared across scenarios with Concurrency: 1, these fields leak between scenarios. PRNumber is set by whenPullRequestOpened in dispatch.go and checked by whenPullRequestLabeled and whenPullRequestReviewComment. DispatchAgent is set by givenCustomHarness. Without reset, stale values from a prior scenario could cause incorrect behavior in subsequent scenarios.
  Remediation: Add w.PRNumber = 0 and w.DispatchAgent = "" to the sc.Before hook in InitScenario, matching the original initializeScenario.

Medium

• [protected-path] .github/workflows/e2e.yml — PR modifies .github/workflows/e2e.yml (a protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to the e2e workflow's path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:887 — References old path e2e/admin/cleanup.go which has been moved to pkg/e2etest/cleanup.go. This is a historical plan document; inline updates are optional but a top-of-file annotation may help future readers.

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:895 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Same context as above.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:219 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Historical plan document.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:269 — References old path e2e/admin/testutil.go in file summary table. Same context as above.

Previous run

Review

Findings

Critical

• [compilation error] pkg/behaviourtest/steps/registry.go:12 — registry.go is renamed from e2e/behaviour/steps/ to pkg/behaviourtest/steps/ and calls registerDispatchSteps(ctx, w), but e2e/behaviour/steps/dispatch.go is NOT included in the rename. After this PR, registerDispatchSteps will be undefined in the pkg/behaviourtest/steps package, causing a compilation failure. Every other .go file in e2e/behaviour/steps/ is moved (artifacts.go, cleanup.go, dummy_agent.go, triage.go) except dispatch.go.
  Remediation: Move e2e/behaviour/steps/dispatch.go to pkg/behaviourtest/steps/dispatch.go and update its import paths from e2e/behaviour/world to pkg/behaviourtest/world and from e2e/behaviour/drivers/scm to pkg/behaviourtest/drivers/scm (if applicable). Also check whether internal/config usage in dispatch.go should be reconsidered for the public API.

Medium

• [protected-path] .github/workflows/e2e.yml — PR modifies .github/workflows/e2e.yml (a protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to the e2e workflow's path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-doc] docs/architecture.md:139 — References e2e/behaviour/ as the home of behaviour tests. After this PR, the shared behaviour test framework (world, steps, drivers) moves to pkg/behaviourtest/ while the in-repo runner and feature files remain at e2e/behaviour/. The PR updates the authoritative guides (behaviour-testing.md, behaviour-drivers.md, e2e-testing.md, ADR 0066) but not this high-level overview.

• [stale-doc] docs/problems/testing-agents.md:11 — References e2e/behaviour/ for behaviour tests. Same partial staleness as architecture.md — the statement remains broadly correct since tests run from e2e/behaviour/, but the framework extraction to pkg/behaviourtest/ makes the reference imprecise.

---

Labels: PR extracts shared e2e test infrastructure into public packages, touching CI workflow and test code.

Previous run (4)

Review

Findings

High

• [logic-error] pkg/behaviourtest/suite/init.go:17 — The new InitScenario function does not reset w.PRNumber and w.DispatchAgent between scenarios. The old initializeScenario in e2e/behaviour/suite_test.go reset both (w.PRNumber = 0 and w.DispatchAgent = ""). Since World is shared across scenarios with Concurrency: 1, these fields leak between scenarios. PRNumber is set by whenPullRequestOpened in dispatch.go and checked by whenPullRequestLabeled and whenPullRequestReviewComment. DispatchAgent is set by givenCustomHarness. Without reset, stale values from a prior scenario could cause incorrect behavior in subsequent scenarios.
  Remediation: Add w.PRNumber = 0 and w.DispatchAgent = "" to the sc.Before hook in InitScenario, matching the original initializeScenario.

Medium

• [protected-path] .github/workflows/e2e.yml — PR modifies .github/workflows/e2e.yml (a protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to the e2e workflow's path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:887 — References old path e2e/admin/cleanup.go which has been moved to pkg/e2etest/cleanup.go. This is a historical plan document; inline updates are optional but a top-of-file annotation may help future readers.

• [stale-path-reference] docs/plans/deprecate-per-org-install.md:895 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Same context as above.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:219 — References old path e2e/admin/testutil.go which has been moved to pkg/e2etest/testutil.go. Historical plan document.

• [stale-path-reference] docs/plans/vertex-inference-provisioning.md:269 — References old path e2e/admin/testutil.go in file summary table. Same context as above.

Previous run (5)

Review

Findings

Critical

• [compilation error] pkg/behaviourtest/steps/registry.go:12 — registry.go is renamed from e2e/behaviour/steps/ to pkg/behaviourtest/steps/ and calls registerDispatchSteps(ctx, w), but e2e/behaviour/steps/dispatch.go is NOT included in the rename. After this PR, registerDispatchSteps will be undefined in the pkg/behaviourtest/steps package, causing a compilation failure. Every other .go file in e2e/behaviour/steps/ is moved (artifacts.go, cleanup.go, dummy_agent.go, triage.go) except dispatch.go.
  Remediation: Move e2e/behaviour/steps/dispatch.go to pkg/behaviourtest/steps/dispatch.go and update its import paths from e2e/behaviour/world to pkg/behaviourtest/world and from e2e/behaviour/drivers/scm to pkg/behaviourtest/drivers/scm (if applicable). Also check whether internal/config usage in dispatch.go should be reconsidered for the public API.

Medium

• [protected-path] .github/workflows/e2e.yml — PR modifies .github/workflows/e2e.yml (a protected path under .github/). The changes add pkg/e2etest/ and pkg/behaviourtest/ to the e2e workflow's path trigger filters and grep-based relevance checks, which is a necessary consequence of the package extraction authorized by issue feat(behaviour): extract pkg/behaviourtest public API #3784. Human approval is required for protected-path changes regardless of context.

Low

• [stale-doc] docs/architecture.md:139 — References e2e/behaviour/ as the home of behaviour tests. After this PR, the shared behaviour test framework (world, steps, drivers) moves to pkg/behaviourtest/ while the in-repo runner and feature files remain at e2e/behaviour/. The PR updates the authoritative guides (behaviour-testing.md, behaviour-drivers.md, e2e-testing.md, ADR 0066) but not this high-level overview.

• [stale-doc] docs/problems/testing-agents.md:11 — References e2e/behaviour/ for behaviour tests. Same partial staleness as architecture.md — the statement remains broadly correct since tests run from e2e/behaviour/, but the framework extraction to pkg/behaviourtest/ makes the reference imprecise.

---

Labels: PR extracts shared e2e test infrastructure into public packages, touching CI workflow and test code.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 4:57 AM UTC · Ended 4:58 AM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 4:59 AM UTC · Ended 5:01 AM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:01 AM UTC · Ended 5:01 AM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 5:02 AM UTC · Ended 5:16 AM UTC
Commit: 5cd495a · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/e2etest/testutil.go	85.00%	3 Missing ⚠️
pkg/behaviourtest/steps/dummy_agent.go	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:02 AM UTC · Completed 5:16 AM UTC
Commit: e32e272 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:23 AM UTC · Ended 5:29 AM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:30 AM UTC · Completed 5:40 AM UTC
Commit: 3d37a09 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 12:46 PM UTC · Completed 1:03 PM UTC
Commit: 41973ea · View workflow run →

Superseded by updated review

[github-actions]

Site preview

Preview: https://a61b5116-site.fullsend-ai.workers.dev

Commit: 755fc1dc546699a540c2ffad3190613cdf934b6b

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:17 AM UTC · Completed 9:29 AM UTC
Commit: 755fc1d · View workflow run →