feat(behaviour): add isolated dev behaviour tests - #1244
Conversation
Signed-off-by: Barak Korren <bkorren@redhat.com> Assisted-by: Codex (gpt-5.6-sol)
PR Summary by QodoAdd isolated dev behaviour tests for agents
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
|
🤖 Finished Review · ✅ Success · Started 11:05 AM UTC · Completed 11:28 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.87 |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Fixed since the prior review (verified against the current PR head):
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Scope behaviour-test credentials to the execution step, cover schema changes, and document the new suite. Make live tests uncached with an explicit timeout and keep the execution wrapper and Makefile organized. Signed-off-by: Barak Korren <bkorren@redhat.com> Assisted-by: Codex (gpt-5.6-sol)
|
🤖 Finished Review · ✅ Success · Started 12:03 PM UTC · Completed 12:20 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.46 |
|
Risk Assessment: elevated (3/5) DetailsLarge blast-radius PR (12 files, 610 lines) touching two protected paths (CI workflow, CONTRIBUTING.md) with a new dependency-affecting go.mod/go.sum and low test-file ratio, but the shared docs/config files it modifies show high recent churn and multi-author activity; issue linkage shows scope aligned with acceptance criteria and an additive, dev-gated, easily revertible design, yielding an elevated but not high composite score. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on the behaviour-test infrastructure, verified against the pinned fullsend commit c85577b87895 and reproduced locally with go1.26.5 where a command is quoted. 8 inline findings (2 high, 6 medium); nothing here duplicates an open or already-addressed thread. No approval or change request implied — flagging for your judgement.
The two high findings are the ones I would weigh before merge: the suite resolves agent content from agents@main rather than the PR head (so harness/**, scripts/**, schemas/** and policies/** are not actually under test at PR time, despite the path filter and the new CONTRIBUTING wording), and the committed module cannot be built or tidied outside the CI-generated modfile, which Renovate's gomod manager will hit with patch automerge enabled.
| push: | ||
| branches: [main] | ||
| paths: | ||
| - 'agents/**' |
There was a problem hiding this comment.
[HIGH] Behaviour runs exercise agents@main, not the PR head — harness/scripts/schemas/policies are never under test at PR time
Verified end-to-end against the pinned fullsend commit c85577b87895:
pkg/behaviourtest/run.go:61builds the CLI viae2etest.BuildModuleBinary, which isbuildCLIBinaryinpkg/e2etest/build.go:42— a plaingo build -o <bin> ./cmd/fullsend/with NO ldflags.internal/cli/root.go:10-11therefore leavesversion = "dev"andcommitSHA = "dev", soresolveBuildVersion()returns("", "").internal/cli/run.go:5303 resolveAgentsRef()returns("main", "heads/main")for any non-release build, andfetchPinnedAgentsRepoFile(run.go:5360) resolvesheads/mainonfullsend-ai/agentswith no env, flag, or config override anywhere in the tree (git grep AGENTS_REFfinds nothing).pkg/behaviourtest/drivers/install/common/setup.go:26— the dev/preview driver uses vendored mode by default (Vendor: true,FullsendRef: ""), so this dev-stamped binary is what runsfullsend runinside the pool repo.
Consequence: the vendored binary fetches harness/triage.yaml, the pre/post scripts, and the result schema from fullsend-ai/agents@main, NOT from the PR head. Only behaviour/features (absolute path via runtime.Caller in behaviour/suite_test.go:19-22) and behaviour/fixtures (via the exec wrapper's symlink + findModuleSubdir) come from the PR.
This contradicts what the PR advertises: the push paths: filter lists harness/**, policies/**, scripts/**, schemas/** (lines 13-24), and CONTRIBUTING.md now tells external contributors that Behaviour Tests is a gate on their change. A PR that breaks scripts/post-triage.sh or harness/triage.yaml still passes this suite green (the scenario's ready-to-code assertion is satisfied by main's content) and only breaks after merge. The sibling .github/workflows/functional-tests.yml explicitly solves this by threading needs.detect.outputs.agents_sha into its checkout (lines 110, 225-230); this workflow has no equivalent.
Suggestion: Either (a) add an agents-ref override upstream in fullsend that resolveAgentsRef honours for dev builds and thread the PR head SHA through the driver (e.g. FULLSEND_AGENTS_REF), or (b) if that is out of scope for this PR, state plainly in docs/behaviour-testing.md and the workflow header that PR/merge-group runs validate the feature specs and fixtures only, that agent content is resolved from agents@main and is exercised solely by the post-merge push run, and drop the Behaviour Tests claim from the CONTRIBUTING external-contributor gate wording so the coverage promise matches reality.
|
|
||
| go 1.26.5 | ||
|
|
||
| require github.com/fullsend-ai/fullsend v0.43.1-0.20260909221251-c85577b87895 |
There was a problem hiding this comment.
[HIGH] Committed go.mod/go.sum is not a buildable or tidy-able module — Renovate (with automerge on patch) will break on it
Distinct from the go mod tidy -tags thread at behaviour-tests.yml:140, and that dismissal was correct: my run confirms tidy IS tag-agnostic (it did see ./behaviour). The failure here is the nested mintcore module, not build tags.
Reproduced with go1.26.5 against the exact committed files at head 1593b59:
go test -tags behaviour -run '^$' ./behaviour->go: updates to go.mod needed; to update it: go mod tidygo mod tidy->reading github.com/fullsend-ai/fullsend/internal/mintcore/go.mod at revision internal/mintcore/v0.0.0: unknown revision(two chains:pkg/behaviourtest->pkg/e2etest->internal/mintcore, and ->internal/cli->internal/mintcore/mintconsts)- The committed
go.sumis 229 lines with ZERO mintcore entries, andgo.mod(5 lines) declares no indirect requires.
Root cause is upstream: internal/mintcore is an unpublished nested module whose replace in fullsend's own go.mod does not propagate to consumers. Only the CI step at behaviour-tests.yml:133-140 makes it build, by copying go.mod to a temp modfile, truncating the sum file, and adding two -replace directives pointing at the cloned checkout.
This matters concretely because the repo has renovate.json extending config:recommended, which enables the gomod manager. Its only disable rule is scoped to matchManagers: ["github-actions"], so nothing exempts gomod. Renovate will now open github.com/fullsend-ai/fullsend bumps whose go.sum artifact regeneration (go mod tidy) hits the failure above — and the config sets automerge: true for patch/pin updates with platformAutomerge, so a bump with a stale/unregenerated go.sum can land unreviewed.
Suggestion: File and reference the upstream root cause (publish or fold internal/mintcore, or break the pkg/e2etest -> internal/cli import). In the meantime: (1) add a gomod disable/ignore rule to renovate.json for github.com/fullsend-ai/fullsend so the bot cannot automerge a module it cannot tidy; (2) state in docs/behaviour-testing.md that this module builds only via the CI-generated modfile and that plain go build/go mod tidy will fail; (3) consider a CI guard that regenerates the modfile and diffs it against a committed artefact so drift is caught.
This also blocks adding a cheap credential-free go test -tags behaviour -run '^$' compile lane to default CI, which is the only way a PR that breaks behaviour/suite_test.go compilation would be caught outside the 45-minute secretful job.
| BEHAVIOUR_CI: githubactions | ||
| BEHAVIOUR_INSTALL_MODE: per-repo | ||
| BEHAVIOUR_APP_SET: fullsend-test | ||
| BEHAVIOUR_SUITE_NAME: agents-bt-${{ github.run_id }} |
There was a problem hiding this comment.
[MEDIUM] Per-run BEHAVIOUR_SUITE_NAME leaks one durable Cloudflare Worker script per workflow run, forever
Verified against pinned fullsend c85577b87895:
repopool_cfmint_previews.go:311 envSuiteName()readsBEHAVIOUR_SUITE_NAMEand defaults to"bt". Upstream.github/workflows/e2e.ymldoes NOT set this variable at all — every upstream run shares the single Workerbt-mint.CFMintWorkerName(suiteName) = suiteName + "-mint"(repopool_cfmint_previews.go, ~line 176).internal/dispatch/cf/provisioner.go(preview branch, ~lines 249-268): when the Worker script does not exist, it performs a one-time bootstrap DURABLE deploy sowrangler versions uploadcan target it.Provisioner.Teardown(provisioner.go, ~lines 336-345): forDeployPreviewit abandons the alias and explicitly does NOT delete the durable Worker script —return nil.pkg/e2etest/cleanup.go CleanupStaleResourcesis forge-org scoped (repos, org secrets, locks); it contains no Cloudflare Worker enumeration or prefix cleanup.
So BEHAVIOUR_SUITE_NAME: agents-bt-${{ github.run_id }} (line 92) yields a brand-new agents-bt-<run_id>-mint Worker script on every single run, each of which triggers a fresh bootstrap durable deploy and is never deleted by anything. Per-run isolation is already provided by the random preview alias bt-<8hex> from GenerateCFMintPreviewAlias() (repopool_cfmint_previews.go:206), so the unique suite name buys no isolation while accumulating Cloudflare Worker scripts without bound against the account.
Suggestion: Use a stable per-consumer suite name — BEHAVIOUR_SUITE_NAME: agents-bt — so all runs share one bootstrapped agents-bt-mint script and rely on the random preview alias for run isolation, exactly as upstream does with the default bt.
| ${{ github.event_name == 'pull_request_target' | ||
| && format('behaviour-{0}', github.event.pull_request.number) | ||
| || format('{0}-{1}', github.workflow, github.ref) }} | ||
| cancel-in-progress: >- |
There was a problem hiding this comment.
[MEDIUM] Any label event cancels the in-flight 45-minute run, and the replacement run does nothing — skipping all driver teardown
pull_request_target fires on types: [opened, synchronize, reopened, labeled] (line 28); the concurrency group for that event is keyed only on format('behaviour-{0}', github.event.pull_request.number) (line 35) with cancel-in-progress: true for every pull_request_target action (lines 37-39). Adding ANY label to a PR (e.g. documentation, size/L) therefore lands in the same group and cancels a running suite.
The replacement run then does nothing: gate is skipped by its own if (line 45, the label is not ok-to-test), so needs.gate.outputs.authorized is empty and the behaviour job's condition (line 78) is false — the PR is left with a cancelled 45-minute run and no completed result.
Worse, cancellation skips t.Cleanup, so the preview alias, org pool lock, and repo-level FULLSEND_MINT_URL are abandoned mid-run (recovered only opportunistically by the next run's CleanupStaleResources), and with the per-run Worker naming above the bootstrapped Worker script is orphaned too.
Upstream fullsend .github/workflows/e2e.yml:57-63 — the direct template for this workflow — guards exactly this, with the inline comment "Label events are auth, not a code change — never cancel an in-progress run":
cancel-in-progress: ${{ (github.event_name == 'pull_request_target' && github.event.action != 'labeled') || (github.event_name != 'pull_request_target' && github.ref != 'refs/heads/main') }}This PR copied e2e.yml's gate/checkout pattern but not this guard. (The sibling functional-tests.yml has the same unguarded form, so this is also a pre-existing repo convention — but the cost is far higher for a 45-minute live suite holding shared dev-pool resources.)
Suggestion: Adopt upstream e2e.yml's form:
cancel-in-progress: >-
${{ (github.event_name == 'pull_request_target' && github.event.action != 'labeled')
|| (github.event_name != 'pull_request_target' && github.ref != 'refs/heads/main') }}and consider the same fix in functional-tests.yml.
| before running it. With that same setup, a credential-free compile is: | ||
|
|
||
| ```bash | ||
| go test -tags behaviour -run '^$' -exec ./scripts/run-behaviour-test-exec.sh ./behaviour |
There was a problem hiding this comment.
[MEDIUM] The documented local compile command fails as written — wrong -exec path resolution and no modfile
Two independent failures, both reproduced with go1.26.5.
-
-exec ./scripts/run-behaviour-test-exec.sh—go testresolves the-execprogram relative to the PACKAGE directory, not the module root. Reproduced with a minimal module:go test -run '^$' -exec ./scripts/wrap.sh ./sub->fork/exec ./scripts/wrap.sh: no such file or directory, while-exec "$PWD/scripts/wrap.sh" ./sub->ok. The repo's ownMakefile:59already avoids this withBEHAVIOUR_TEST_EXEC ?= $(CURDIR)/scripts/run-behaviour-test-exec.sh; only this doc uses the broken relative form. -
The command omits
-modfile, so even with the path fixed it hits the go.mod failure: running it verbatim on the committed module givesgo: updates to go.mod needed; to update it: go mod tidy. The preceding sentence — "The CI workflow prepares a temporary module file... With that same setup, a credential-free compile is:" — points at a setup it never shows, andgo mod tidy(the obvious next step a reader takes) fails too, so a reader cannot recover.
This is the only local-check instruction in the new doc, so as written no contributor can run the credential-free compile.
Suggestion: Use an absolute path — -exec "$PWD/scripts/run-behaviour-test-exec.sh" — or better, document make behaviour-test BEHAVIOUR_GOFLAGS=-modfile=<modfile> so the Makefile's $(CURDIR) handles it. Then show the modfile preparation explicitly (the go mod edit -replace pair for github.com/fullsend-ai/fullsend and its nested internal/mintcore) as a numbered prerequisite instead of "with that same setup", and note that plain go mod tidy will fail.
| TEST_FULLSEND_PEM TEST_TRIAGE_PEM TEST_CODER_PEM TEST_REVIEW_PEM | ||
| TEST_RETRO_PEM TEST_PRIORITIZE_PEM CLOUDFLARE_ACCOUNT_ID | ||
| CLOUDFLARE_API_TOKEN TEST_ACTOR_WRITE_PAT TEST_ACTOR_TRIAGE_PAT | ||
| TEST_ACTOR_OUTSIDER_PAT |
There was a problem hiding this comment.
[MEDIUM] Three actor PATs are hard-required (job exits 1) but nothing in the pinned framework consumes them
TEST_ACTOR_WRITE_PAT, TEST_ACTOR_TRIAGE_PAT, and TEST_ACTOR_OUTSIDER_PAT are in the required=(...) list at lines 170-171 and cause exit 1 when unset (lines 173-177).
At the pinned fullsend commit c85577b87895, git grep -l TEST_ACTOR returns only .github/workflows/e2e.yml, docs/guides/dev/{behaviour,e2e}-testing.md, hack/setup-new-e2e-org.sh, and scripts/redact-behaviour-artifacts.sh — ZERO hits under pkg/behaviourtest, pkg/e2etest, internal/, or cmd/. The redaction script is the only real consumer, and this workflow never runs it. Upstream e2e.yml says so in its own inline comment at lines 290-291: "Human-like actor PATs (write / triage / outsider) for behaviour SCM ops. Env names match repo secret names; consumption is driver/step work (#6027)." — i.e. upstream knows they are not yet consumed, and upstream merely passes them, it does not hard-require them.
By contrast the PEMs ARE consumed (repopool_cfmint_previews.go:353-354 maps app names to TEST_FULLSEND_PEM / TEST_TRIAGE_PEM / ...), so the requirement list looks copied wholesale from e2e.yml rather than derived from what the pinned driver reads. Net effect: the job blocks on minting three unused PATs, and injects three unused credentials into the one step that executes PR-head-controlled make under pull_request_target — needlessly widening the blast radius that the (accepted) fork-code-with-secrets tradeoff is already carrying. E2E_GCP_SERVICE_ACCOUNT is likewise only read by the google-github-actions/auth step, not the test process.
Suggestion: Require and inject only what the pinned framework actually reads (the six PEMs, the two Cloudflare values, E2E_GCP_PROJECT_ID, E2E_GCP_WIF_PROVIDER). If you want to pre-provision the actor PATs for future scenarios, pass them but drop them from the required list and add the same inline comment upstream uses (referencing fullsend#6027) so the hard gate is not blocking on secrets nothing consumes.
| - '.github/workflows/behaviour-tests.yml' | ||
| - '.github/actions/check-e2e-authorization/**' | ||
| - '.github/scripts/check-e2e-authorization.sh' | ||
| pull_request_target: |
There was a problem hiding this comment.
[MEDIUM] No relevance filter on pull_request_target/merge_group — a docs-only PR burns a dev-pool org, a preview mint, and 45 minutes
push is path-filtered (lines 12-26) but pull_request_target (line 27), merge_group (line 29), and workflow_dispatch (line 30) have no path filter and no change-detection step, so every authorized PR — including one touching only docs/** or README.md — acquires a dev pool org lock, deploys a Cloudflare preview mint, and runs up to the 45-minute timeout-minutes budget.
Both reference workflows do gate this:
- Upstream fullsend
.github/workflows/e2e.yml:208-243has a "Check for behaviour-relevant changes" step forpull_request_target/merge_group, marked# SYNC-WITH: push.paths behaviour entries above, that compares the API file list against the same regex and setsrelevant=falseto skip (failing open with a warning if the API call fails or the compare truncates at 300 files). - The sibling
.github/workflows/functional-tests.yml:98-135uses a lightweightdetectjob over changed files for the same purpose.
The path set is already written down at lines 13-26 of this file, so the filter has no new information to derive.
Suggestion: Port e2e.yml's relevance step: for pull_request_target/merge_group, fetch the changed-file list and grep it against the same path set already in push.paths (add a # SYNC-WITH: push.paths above comment), gate the checkout and subsequent steps on steps.changes.outputs.relevant != 'false', and fail open with a ::warning:: on API error or a truncated compare, exactly as upstream does.
|
|
||
| behaviour: | ||
| needs: gate | ||
| if: >- |
There was a problem hiding this comment.
[MEDIUM] Behaviour job omits the github.repository_owner == 'fullsend-ai' guard that the sibling secretful workflow has
Distinct from the (dismissed) fork-PR-secrets thread: that one is about fork PRs into this repo, which the gate covers. This is about forks of the repository running the privileged path on their own events.
The behaviour job's if: (lines 71-79) checks event name and gate authorization but never the repository owner. The sibling .github/workflows/functional-tests.yml applies github.repository_owner == 'fullsend-ai' && to BOTH of its secretful jobs (line 102 on detect, line 209 on functional-tests) — verified at this head SHA.
Without it, a fork of this repo runs the full privileged path on its own pushes and PRs: environment: dev, id-token: write, GCP WIF auth, and the make behaviour-test step with all fourteen mapped secrets. In practice a fork without those secrets fails at the credential-validation loop (lines 173-177), so this is defence-in-depth and wasted-run prevention rather than a live bypass — but it is an unexplained divergence from the security posture the repo's other pull_request_target workflow establishes, in a job whose blast radius (six App private keys, Cloudflare API token, three PATs, GCP WIF) is larger than functional-tests'.
Suggestion: Add github.repository_owner == 'fullsend-ai' && to the behaviour job's if: condition, matching functional-tests.yml lines 102 and 209.
2 participants
Closes #1242
Summary
Provisioning
Validation