Skip to content

test(#3928): add behaviour scenarios for URL-sourced harness dispatch - #5407

Merged
ifireball merged 4 commits into
mainfrom
agent/3928-url-dispatch-behaviour-tests
Jul 28, 2026
Merged

test(#3928): add behaviour scenarios for URL-sourced harness dispatch#5407
ifireball merged 4 commits into
mainfrom
agent/3928-url-dispatch-behaviour-tests

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Add behaviour test scenarios for URL-sourced custom agent harness dispatch, verifying that FetchAgentHarness URL resolution works end-to-end in the harness-dispatch pipeline.

Related Issue

Fixes #3928

Changes

  • New feature file (e2e/behaviour/features/dispatch/url-dispatch.feature): Three scenarios covering URL-sourced harness CEL trigger dispatch, mixed URL+local harness enumeration, and graceful skip on integrity hash failure.
  • New step definitions (pkg/behaviourtest/steps/url_dispatch.go): givenURLSourcedCustomHarness commits harness YAML to the config repo, computes SHA256 integrity hash, constructs raw.githubusercontent.com URL, and registers the agent in config with the URL source and allowed_remote_resources update. Supports badHash and skipAllowlist option variants for failure testing.
  • New fixture (e2e/behaviour/fixtures/dispatch/url-harness.yaml): Reference harness YAML for URL-sourced tests.
  • Unit tests (pkg/behaviourtest/steps/url_dispatch_test.go): Covers input validation, URL format construction, bad hash injection, allowlist skip, and containsPrefix helper.
  • Registry update (pkg/behaviourtest/steps/registry.go): Registers new URL dispatch step definitions.

Testing

  • Unit tests pass: go test ./pkg/behaviourtest/steps/...
  • All existing behaviour test step tests continue to pass
  • go vet clean
  • Secret scan clean
  • E2E behaviour tests require pool org infrastructure (run in CI)

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • Commits are signed off (DCO) — human and human-directed agent sessions only
  • I wrote this contribution myself and can explain all changes in it

Closes #3928

Post-script verification

  • Branch is not main/master (agent/3928-url-dispatch-behaviour-tests)
  • Secret scan passed (gitleaks — d9185cb4c47c583de659ff76e15a7ba167899d3a..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner July 21, 2026 10:14
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Jul 21, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:16 AM UTC · Completed 10:31 AM UTC
Commit: 2ff4c11 · View workflow run →

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown

Site preview

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

Commit: af7677adddc03b942480d4955eedcaaa923d60c0

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] .github/workflows/e2e.yml — This file is under .github/ (a protected path). The timeout-minutes value is bumped from 30 to 45. While likely needed to accommodate the new URL-dispatch behaviour test scenarios (CDN propagation delays, cross-repo setup), the PR description does not explicitly list this change. Human approval is required for all changes to protected paths. See also: [timeout-justification] finding.

  • [abstraction-consistency] pkg/behaviourtest/steps/url_dispatch.go:134 — The raw URL construction hardcodes https://raw.githubusercontent.com, making the URL-dispatch behaviour tests GitHub-specific. The rest of the test infrastructure is forge-agnostic via the scm.Driver interface, but this step definition is tightly coupled to GitHub's raw content URL pattern. If multi-forge behaviour tests are a future goal, consider adding a GetRawFileURL method to scm.Driver.

Low

  • [test-fidelity] pkg/behaviourtest/steps/url_dispatch_test.go:590fakeURLSCM.CommitFile stores files keyed by path alone (f.files[path]), ignoring owner and repo parameters. GetFileContent also looks up by path alone. Files committed to different repositories at the same path would silently overwrite each other. Current tests avoid collisions by using distinct paths (harness/*.yaml for the hosting repo, .fullsend/config.yaml for the config repo), but this is fragile for future test expansion.

  • [comment-style] internal/forge/forge.go:287 — The doc comment for UpdateRepoVisibility includes usage context ("Used by behaviour tests to enforce public visibility...") which breaks from the pattern of other Client interface methods that only describe what the method does. See also: [scope-alignment] finding.

  • [missing-documentation] docs/guides/dev/behaviour-testing.md — New behaviour test feature file introduces new Gherkin steps (a harness-hosting repository, a URL-sourced custom harness, variants for bad hash and skip allowlist) not mentioned in docs/guides/dev/behaviour-testing.md.

  • [scope-alignment] internal/forge/forge.goUpdateRepoVisibility is added to the forge.Client interface to support behaviour test infrastructure (ensuring public repos for raw.githubusercontent.com access when org policies override CreateRepo(private=false)). The PR description does not acknowledge this interface expansion. All existing implementations (GitHub, GitLab, FakeClient) are provided in the diff.

  • [timeout-justification] .github/workflows/e2e.yml — The e2e workflow timeout increases from 30m to 45m, and the Makefile behaviour-test target matches. The increase is not explained in the PR description but is likely needed for CDN propagation delays and cross-repo setup in the new URL-dispatch scenarios. See also: [protected-path] finding.

  • [architectural-coherence] pkg/behaviourtest/steps/url_dispatch.go:62EnsureRepoPublic conflates verification and remediation (checks repo visibility then attempts UpdateRepoVisibility to fix it). This fix-on-fail pattern is deliberate and well-documented in the code comments, but is atypical for test setup helpers in this codebase.

  • [response-body-handling] internal/forge/github/github.go:290UpdateRepoVisibility calls resp.Body.Close() without draining the body first. Consistent with the established pattern in this file (e.g., DeleteRepo), but may prevent HTTP/2 connection reuse.

Previous run

Review

Findings

Low

  • [test-fidelity] pkg/behaviourtest/steps/url_dispatch_test.go:590fakeURLSCM.CommitFile stores files keyed by path alone (f.files[path]), ignoring owner and repo parameters. GetFileContent also looks up by path alone. Files committed to different repositories at the same path would silently overwrite each other. Current tests avoid collisions by using distinct paths (harness/*.yaml for the hosting repo, .fullsend/config.yaml for the config repo), but this is fragile for future test expansion.

  • [comment-style] internal/forge/forge.go:287 — The doc comment for UpdateRepoVisibility includes usage context ("Used by behaviour tests to enforce public visibility...") which breaks from the pattern of other Client interface methods that only describe what the method does.

  • [missing-documentation] e2e/behaviour/features/dispatch/url-dispatch.feature — New behaviour test feature file for URL-sourced harness dispatch introduces new Gherkin steps (a harness-hosting repository, a URL-sourced custom harness) not mentioned in docs/guides/dev/behaviour-testing.md.

Previous run (2)

Review

Findings

Low

  • [portability] pkg/behaviourtest/steps/url_dispatch.go:92filepath.Join is used to construct harnessPath (line 92) and cfgPath (line 155). These paths are embedded in a raw.githubusercontent.com URL and used as repository file paths in API calls. filepath.Join is OS-specific and produces backslashes on Windows, which would break URL construction. While behaviour tests only run on Linux CI, path.Join (import "path") is the correct choice for URL/repository path construction. Note: the existing codebase uses this same pattern in world.go, so this is a pre-existing convention rather than a regression.

  • [test-fidelity] pkg/behaviourtest/steps/url_dispatch_test.go:590fakeURLSCM.CommitFile stores files keyed by path alone (f.files[path]), ignoring owner and repo parameters. GetFileContent also looks up by path alone. Files committed to different repositories at the same path would silently overwrite each other. Current tests avoid collisions by using distinct paths (harness/*.yaml for the hosting repo, .fullsend/config.yaml for the config repo), but this is fragile for future test expansion.

  • [comment-style] internal/forge/forge.go:289 — The doc comment for UpdateRepoVisibility includes usage context ("Used by behaviour tests to enforce public visibility...") which breaks from the pattern of other Client interface methods that only describe what the method does. Similarly, the GitHub and GitLab implementations include HTTP verb/endpoint details ("via PATCH /repos/...", "via PUT /projects/...") while sibling methods omit such implementation specifics.

Previous run (3)

Review

Findings

Low

  • [portability] pkg/behaviourtest/steps/url_dispatch.go:92filepath.Join is used to construct harnessPath which is then embedded in a raw.githubusercontent.com URL. filepath.Join is OS-specific and produces backslashes on Windows, which would break the URL. While behaviour tests only run on Linux CI, path.Join (import "path") is the correct choice for URL path construction. The cfgPath usage at line 146 is unaffected since it is only passed to SCM driver API calls that abstract the path.

  • [test-fidelity] pkg/behaviourtest/steps/url_dispatch_test.go:483fakeURLSCM.CommitFile stores files keyed by path alone (f.files[path]), ignoring owner and repo parameters. GetFileContent also looks up by path alone. Files committed to different repositories at the same path would silently overwrite each other. Current tests avoid collisions by using distinct paths (harness/*.yaml for the hosting repo, .fullsend/config.yaml for the config repo), but this is fragile for future test expansion.

  • [comment-style] internal/forge/forge.go:287 — The doc comment for UpdateRepoVisibility includes usage context ("Used by behaviour tests to enforce public visibility...") which breaks from the pattern of other Client interface methods that only describe what the method does. Similarly, the GitHub and GitLab implementations include HTTP verb/endpoint details ("via PATCH /repos/...", "via PUT /projects/...") while sibling methods omit such implementation specifics.

Previous run (4)

Review

Findings

Low

  • [authorization] internal/forge/forge.go:290UpdateRepoVisibility is added to the production forge.Client interface but is only consumed by behaviour-test code (EnsureRepoPublic in pkg/behaviourtest/drivers/scm/github/github.go). The method follows the established pattern — CreateRepo and DeleteRepo are equally powerful operations already on the same interface — and the comment documents the test-only intent. Consider a forge.TestClient interface in future refactoring if more test-only methods accrue.
Previous run (5)

Review

Findings

High

  • [logic-error] e2e/behaviour/features/dispatch/url-dispatch.feature:96 — Scenario "URL source not in allowlist is skipped and dispatch continues" describes behavior that conflicts with the production code. The test expects that when a URL-sourced agent is not in the allowlist, it is skipped and dispatch continues for other agents. However, the production dispatch path (DispatchListTriggeredHarnessesRegisteredAgentsValidateAgentEntries) validates ALL agent entries upfront. ValidateAgentEntries (internal/config/config.go) returns a hard error for the entire config when any URL agent is not covered by allowed_remote_resources, preventing all agents from dispatching — including the good-allowed local harness. Contrast with Scenario 3 (bad integrity hash): the hash mismatch is caught later in FetchAgentHarness where per-agent skip logic applies, so that scenario correctly models production behavior.
    Remediation: Either (a) modify ValidateAgentEntries to skip/warn instead of hard-failing when a URL agent is not in the allowlist, moving the check into ResolveRegisteredPath where failures are gracefully skipped per-agent, or (b) rewrite this scenario to match actual production behavior where the entire dispatch fails when a non-allowlisted URL agent is present.

Low

  • [error-wrapping-consistency] pkg/behaviourtest/steps/url_dispatch.go:105yaml.Marshal error is wrapped with fmt.Errorf("marshalling config: %w", err), but the established pattern in dispatch.go for the identical yaml.Marshal(cfg) call is to return the error unwrapped (bare return err).

  • [spelling-consistency] pkg/behaviourtest/steps/url_dispatch.go:105 — Uses British spelling "marshalling" (double-l) but codebase convention is US spelling "marshaling" (single-l), as seen in dummy_agent.go and config.go.

Previous run (6)

Review

Findings

Low

  • [dead-code] e2e/behaviour/fixtures/dispatch/url-harness.yaml:1 — Fixture file is not referenced anywhere in the codebase. It duplicates the inline harness YAML from Scenario 1's docstring but serves no purpose.
    Remediation: Either remove the file or wire it into a test step.

  • [scope-question] e2e/behaviour/features/dispatch/url-dispatch.feature:58 — The third scenario tests hash failure but not allowlist failure, though issue test(behaviour): harness-dispatch URL-sourced custom agents #3928 says "fails allowlist or integrity hash." The step definition for allowlist failure (not in allowlist with:) is already registered but no scenario exercises it.
    Remediation: Confirm whether a fourth scenario for allowlist failure is intended for this PR or a follow-up.

  • [naming-consistency] pkg/behaviourtest/steps/url_dispatch.go:112 — Function name containsPrefix suggests prefix matching but implementation uses exact match via slices.Contains.
    Remediation: Rename to containsExact or inline the slices.Contains call.

  • [error-wrapping-consistency] pkg/behaviourtest/steps/url_dispatch.go:103 — Bare return err from yaml.Marshal differs from the wrapping pattern used elsewhere in the file. Note: dispatch.go uses the same bare pattern.

  • [missing-documentation] docs/guides/dev/behaviour-testing.md — The behaviour testing guide does not document the new Given a URL-sourced custom harness step, though existing steps like Given a custom harness are also not individually documented.
    Remediation: Consider adding a brief mention of URL-sourced harness testing.


Labels: PR adds e2e behaviour test scenarios for URL-sourced harness dispatch

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/e2e End-to-end tests component/dispatch Workflow dispatch and triggers testing labels Jul 21, 2026
@ifireball ifireball self-assigned this Jul 21, 2026
@ifireball

Copy link
Copy Markdown
Member

/fs-fix The new step functions assume the existence of the .fullsend repo in the test org, that repo, belongs to the deprecated per-org install mode while the behaviour tests are based on per-repo mode. A step for creating an arbitrarily named new repo should be introduced instead (if no such step exists already, similar to the fork repo creation step) and called from the "background" section. A supplementry step for pushing a harness file to that repo should then be introduced as well (again, if we don't already have a step for pushing a harness to a given arbitrary repo)

Fix the comments by the review agent as well.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 11:15 PM UTC · Completed 11:28 PM UTC
Commit: 7ada4e0 · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 5 actionable review findings and the human instruction. Introduced a harness-hosting repository step (CreateRepo/DeleteRepo on SCM driver) so URL dispatch tests use a separate repo instead of assuming per-org mode. Removed dead fixture, added allowlist failure scenario, fixed naming and error wrapping. Disagreed with documentation finding (out of scope). All tests pass.

Fixed (5):

  1. URL dispatch steps assume per-org .fullsend repo (pkg/behaviourtest/steps/url_dispatch.go): Added CreateRepo/DeleteRepo to SCM driver interface and introduced a 'Given a harness-hosting repository' step that creates a separate repo for hosting URL-sourced harness files. Modified givenURLSourcedCustomHarness to commit harness YAML to the hosting repo (not the config repo) and register the URL in the enrolled test repo's config. Added cleanup in CleanupScenario to delete the hosting repo.
  2. dead-code: fixture url-harness.yaml not referenced (e2e/behaviour/fixtures/dispatch/url-harness.yaml): Removed the unreferenced fixture file that duplicated inline harness YAML from Scenario 1.
  3. scope-question: missing allowlist failure scenario (e2e/behaviour/features/dispatch/url-dispatch.feature): Added fourth scenario 'URL source not in allowlist is skipped and dispatch continues' that exercises the already-wired 'not in allowlist with:' step definition.
  4. naming-consistency: containsPrefix uses exact match (pkg/behaviourtest/steps/url_dispatch.go): Removed the misleadingly named containsPrefix helper and inlined slices.Contains directly at the call site.
  5. error-wrapping-consistency: bare return err from yaml.Marshal (pkg/behaviourtest/steps/url_dispatch.go): Wrapped yaml.Marshal error with fmt.Errorf for consistency with the error wrapping pattern used throughout the file.

Disagreed (1):

  1. missing-documentation: behaviour testing guide lacks URL-sourced harness step docs: The behaviour testing guide does not individually document any step definitions (including existing ones like 'Given a custom harness'). Adding documentation for just this step would be inconsistent. This is better addressed in a separate documentation PR that covers all step definitions.

Tests: passed

Decision points
  • Chose to model the harness-hosting repo step after the existing fork creation pattern (alternatives: Reuse fork creation step, Commit harness to existing test repo; rationale: The human explicitly asked for a step similar to the fork repo creation step. Creating a separate repo properly tests URL-sourced harness fetch from a different repository.)
  • Chose to make harness path relative (harness/name.yaml) in the hosting repo instead of using .fullsend prefix (alternatives: Use .fullsend/harness/name.yaml path in hosting repo; rationale: The hosting repo is a standalone test repo, not a fullsend config repo. The .fullsend prefix is specific to the per-repo config structure and would be misleading in a general-purpose hosting repo.)

Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Jul 21, 2026
…r URL dispatch tests

- Add CreateRepo/DeleteRepo to SCM driver interface for creating test repos
- Introduce "Given a harness-hosting repository" step that creates a
  separate repo for hosting URL-sourced harness files (instead of
  committing to the config repo, which assumed per-org mode)
- Update givenURLSourcedCustomHarness to commit harness YAML to the
  hosting repo and register the URL source in the enrolled test repo's
  config
- Add cleanup for the harness-hosting repo in CleanupScenario
- Add fourth scenario exercising allowlist failure path (scope-question)
- Remove dead fixture file url-harness.yaml (dead-code)
- Inline slices.Contains and remove misleadingly named containsPrefix
  helper (naming-consistency)
- Wrap yaml.Marshal error for consistency (error-wrapping-consistency)

Addresses review feedback on #5407
@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown

E2E tests are running

Authorization passed for this commit. See the E2E Tests workflow for results.

@github-actions

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@ifireball ifireball added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 22, 2026
@ifireball

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:08 AM UTC · Completed 12:26 AM UTC
Commit: 7ada4e0 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot removed the ready-for-merge All reviewers approved — ready to merge label Jul 22, 2026
@ifireball

Copy link
Copy Markdown
Member

/fs-fix

Diagnosis (babysit)

1. Behaviour tests timed out (blocking)

Run: https://github.com/fullsend-ai/fullsend/actions/runs/29879270671
Job behaviour hit the 30m timeout (go test -timeout 30m) and was cancelled.

Progress before cancel:

  • Existing dispatch + fork-dispatch scenarios passed
  • Hung in the first new scenario: URL-sourced harness with CEL trigger dispatches agent (url-dispatch.feature:7)
  • Steps completed through When the issue is labeled "ready-for-url-ping" (~00:26:16 UTC), then waited ~11m with no Then the harness ... workflow completes successfully before cancel

Please investigate why the URL-sourced harness never dispatches / never completes after the label event (harness-hosting repo setup, allowlist wiring for the host URL, dispatch config push, artifact fetch, etc.). Do not just bump the timeout — the hang indicates the scenario is not working.

2. High review finding — allowlist scenario mismatches production

url-dispatch.feature scenario "URL source not in allowlist is skipped and dispatch continues" expects per-agent skip + other agents continue. Production ValidateAgentEntries (internal/config/config.go) hard-fails the whole config when any URL agent is outside allowed_remote_resources, so no agents dispatch (including local good-allowed).

Fix by rewriting the scenario to assert actual production behavior (entire dispatch fails / no agents run), or change production to per-agent skip if that is the intended product behavior for #3928. Prefer matching production unless the issue explicitly requires soft-skip.

3. Codecov patch coverage failing (50%, need ~80%)

Missing coverage concentrated in:

  • pkg/behaviourtest/steps/url_dispatch.go (~58% patch)
  • pkg/behaviourtest/drivers/scm/github/github.go (0% on new CreateRepo/DeleteRepo paths)
  • pkg/behaviourtest/steps/cleanup.go, registry.go

Add/extend unit tests (similar to existing url_dispatch_test.go / driver mocks) for the new step and SCM methods.

4. Remaining low review nits

  • url_dispatch.go:105: align yaml.Marshal error handling with dispatch.go (bare return err) or keep wrapping but use US spelling marshaling (not marshalling) to match codebase convention.

After pushing fixes, leave a short summary of what changed. Behaviour will need ok-to-test re-applied after the new HEAD if the gate requires it.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 12:40 AM UTC · Completed 12:59 AM UTC
Commit: 7ada4e0 · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Addressed all 4 review findings. Fixed allowlist scenario to match production ValidateAgentEntries hard-fail behavior. Fixed resetScenarioWorld to clear URL harness fields preventing cross-scenario leaks. Fixed marshalling→marshaling spelling. Added 8 new unit tests improving patch coverage for url_dispatch.go, cleanup.go, and their error paths. The first-scenario timeout requires infrastructure investigation: FetchAgentHarness uses unauthenticated HTTP GET so private hosting repos will silently fail.

Fixed (4):

  1. behaviour tests timed out — resetScenarioWorld does not clear URL harness fields (pkg/behaviourtest/suite/init.go): Added URLHarnessRepoOwner and URLHarnessRepoName to resetScenarioWorld to prevent cross-scenario state leaks. Updated test to verify the new fields are cleared. Root cause of the timeout (first scenario hanging at 'Then the harness url-ping workflow completes successfully') is that FetchAgentHarness uses unauthenticated HTTP GET via internal/fetch/fetch.go — if the hosting repo is private (org policy), raw.githubusercontent.com returns 403/404 and the agent is gracefully skipped by ListTriggeredHarnesses, causing no workflow to dispatch. This likely requires infrastructure investigation (verify the test org allows public repos, or add authenticated URL fetching).
  2. allowlist scenario mismatches production — expects per-agent skip but production hard-fails (e2e/behaviour/features/dispatch/url-dispatch.feature): Rewrote the 'URL source not in allowlist' scenario to match production ValidateAgentEntries behavior. Production hard-fails the entire config when any URL agent is outside allowed_remote_resources (via RegisteredAgents → ValidateAgentEntries → ListTriggeredHarnesses). No agents dispatch, including the valid local harness. Scenario now asserts neither agent ran, removed dummy agent setup that implied success.
  3. codecov patch coverage failing — missing tests for new code paths (pkg/behaviourtest/steps/url_dispatch_test.go): Added 5 new test cases for url_dispatch.go: agent entry update dedup, allowlist dedup, CreateRepo error propagation, CommitFile error propagation, GetConfig error propagation. Added 3 new test cases for cleanup.go: URL harness repo deletion, skip when not set, error logging. Updated fakeURLSCM and fakeCleanupSCM to support error injection.
  4. marshalling spelling inconsistent with codebase convention (pkg/behaviourtest/steps/url_dispatch.go): Changed 'marshalling' to 'marshaling' (US spelling) to match the codebase convention used in dispatch.go, config.go, dummy_agent.go, and all other files.

Tests: passed

Decision points
  • Chose to rewrite allowlist scenario to match production (hard-fail) rather than changing production to per-agent skip (alternatives: Change ValidateAgentEntries to per-agent skip, Keep scenario as-is and skip the test; rationale: Human instruction said 'Prefer matching production unless the issue explicitly requires soft-skip'. Issue test(behaviour): harness-dispatch URL-sourced custom agents #3928 does not require soft-skip behavior.)
  • Timeout root cause diagnosis rather than bump-timeout fix (alternatives: Increase test timeout, Add retry/wait for raw.githubusercontent.com propagation; rationale: Human instruction explicitly said 'Do not just bump the timeout'. Root cause is likely that the hosting repo is private (org policy blocks public repos) or there is a raw.githubusercontent.com CDN propagation delay. Fixed the detectable code issue (resetScenarioWorld leak) and documented the infrastructure investigation needed.)

Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Jul 22, 2026
- Rewrite allowlist scenario to match production ValidateAgentEntries
  behavior: config validation hard-fails when any URL agent is outside
  allowed_remote_resources, preventing all agents from dispatching
  (not just the offending one).

- Fix resetScenarioWorld to clear URLHarnessRepoOwner and
  URLHarnessRepoName, preventing cross-scenario state leaks.

- Fix US spelling: marshalling → marshaling to match codebase convention.

- Add test coverage for URL harness repo cleanup (delete, skip, error
  logging), agent entry update dedup, allowlist dedup, and error paths
  for CreateRepo and CommitFile failures.

Addresses review feedback on #5407
@github-actions github-actions Bot removed the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 22, 2026
@ifireball

Copy link
Copy Markdown
Member

/fs-fix

Scope (this iteration only)

Make harness-hosting repo steps use the same concurrency/lifetime model as fork steps. Do not fix the production FetchPolicy / URL-dispatch bug in this iteration — we want a CI signal that the tip race is gone while URL happy-path may still fail.

Confirmed diagnosis

Behaviour run https://github.com/fullsend-ai/fullsend/actions/runs/30303864364 job behaviour failed 3 scenarios. The second failure:

  • Scenario: Config mixes URL-sourced and local harnesses
  • Error: committing harness to hosting repo: update ref: github api: 422 Reference cannot be updated
  • Cause: parallel scenarios share one long-lived url-harness-host tip. Forks already avoid this.

Fork model to mirror (do the same for host)

From pkg/behaviourtest/steps/fork.go + cleanup.go + behaviour-testing.md:

  1. Logical name → leased base: Gherkin keeps a stable name; runtime remaps using the leased repo number, e.g. "test-repo-fork" + leased test-repo-07test-repo-07-fork via resolveForkName.
  2. Ephemeral / short-lived: fork repos are created per-scenario and deleted by CleanupScenario (not long-lived pool assets).
  3. Feature files keep the logical name; do not hard-code test-repo-NN-… in Gherkin.

Required changes

  1. Resolve host repo name from leased test repo (same pattern as resolveForkName). Example: Background And a harness-hosting repository "url-harness-host" with leased test-repo-04 → actual repo test-repo-04-url-harness-host (or an equally clear {RepoName}-… scheme — document it). Prefer starting from a logical name that remaps cleanly like forks do if that fits better (e.g. test-repo-url-harness-host{RepoName}-url-harness-host).
  2. Make host repos ephemeral: create in the host step; delete in CleanupScenario (same safety pattern as forks: only delete when owner/name set and not the enrolled test repo). Remove/update comments and docs that call the host repo long-lived.
  3. Update docs/guides/dev/behaviour-testing.md URL-sourced section to match fork lifecycle table language (per-scenario create + cleanup delete).
  4. Unit tests for name resolution and cleanup registration (mirror fork tests).
  5. Keep EnsureRepoPublic and existing URL-sourced harness registration behavior.

Explicitly out of scope

  • Do not change internal/harnessdispatch FetchPolicy / DefaultPolicy plumbing in this iteration.
  • Do not broaden 422 retry classification unless needed as a tiny within-scenario safety net after ephemeral hosts; prefer isolation via per-lease host repos.

Done when

  • Host naming + ephemeral cleanup land on this PR.
  • Behaviour CI re-runs. Expect: no 422 Reference cannot be updated on the shared host tip. URL happy-path may still fail until a later FetchPolicy fix.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:37 AM UTC · Completed 9:46 AM UTC
Commit: 5438572 · View workflow run →

Mirror the fork lifecycle model for URL-sourced harness hosting repos:

- Add resolveHostRepoName to remap logical hosting repo names using the
  leased test repo name (e.g. "url-harness-host" + "test-repo-07" ->
  "test-repo-07-url-harness-host"), eliminating the shared-tip race
  that caused 422 "Reference cannot be updated" errors in parallel CI.
- Add hosting repo deletion to CleanupScenario with the same safety
  guards as fork repos (skip when fields missing or name matches
  enrolled repo, silently ignore NotFound).
- Update docs to reflect ephemeral lifecycle matching fork repos.
- Add unit tests for name resolution and cleanup registration.

Addresses review feedback on #5407
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Made harness-hosting repos ephemeral with per-lease naming to eliminate the shared-tip race causing 422 errors. Added resolveHostRepoName (mirrors resolveForkName), CleanupScenario deletion with safety guards, updated docs, and added 9 unit tests. All tests pass.

Fixed (5):

  1. Resolve host repo name from leased test repo (pkg/behaviourtest/steps/url_dispatch.go): Added resolveHostRepoName function mirroring resolveForkName pattern. Maps logical hosting repo name to {RepoName}-{logicalName} when a lease is active, giving each parallel scenario its own isolated hosting repo.
  2. Make host repos ephemeral with CleanupScenario deletion (pkg/behaviourtest/steps/cleanup.go): Added hosting repo deletion to CleanupScenario with same safety guards as fork repos: only delete when URLHarnessRepoOwner and URLHarnessRepoName are set and URLHarnessRepoName differs from RepoName; silently ignore NotFound errors.
  3. Update docs to match fork lifecycle language (docs/guides/dev/behaviour-testing.md): Changed harness-hosting repo description from 'Long-lived' to 'Ephemeral / per-scenario' with explanation of resolveHostRepoName remapping, matching the fork lifecycle table language.
  4. Unit tests for name resolution and cleanup registration (pkg/behaviourtest/steps/url_dispatch_test.go): Added TestResolveHostRepoName_NoLease, TestResolveHostRepoName_LeasedRepoMaps, TestResolveHostRepoName_DifferentLease, and TestGivenHarnessHostingRepo_LeasedRepoResolvesHostName mirroring fork test patterns.
  5. Unit tests for hosting repo cleanup (pkg/behaviourtest/steps/cleanup_test.go): Added TestCleanupScenario_DeletesHostingRepo, TestCleanupScenario_SkipsHostingRepoDelete_WhenEqualsRepoName, TestCleanupScenario_SkipsHostingRepoDelete_WhenFieldsMissing, TestCleanupScenario_DeleteHostingRepoNotFound_SilentlyIgnored, and TestCleanupScenario_DeleteHostingRepoError_Logged mirroring fork cleanup test patterns.

Tests: passed

Updated by fullsend fix agent

@ifireball

Copy link
Copy Markdown
Member

/fs-fix

Status after host-concurrency iteration

Run: https://github.com/fullsend-ai/fullsend/actions/runs/30347973275

  • Host tip race fixed: per-lease names like test-repo-07-url-harness-host are in use. Scenario Config mixes URL-sourced and local harnesses no longer fails with 422 Reference cannot be updated. Suite is 9 passed / 2 failed (was 8/3).
  • Remaining product failure: URL-sourced harness with CEL trigger dispatches agentharness agent "url-ping" did not complete successfully (no fullsend-url-ping artifact within wait). That means the agent was never dispatched.
  • Unrelated infra flake: PR review dispatches review-only harness hit WIF IAM 409 on test-repo-11 during ensure. Ignore unless easy; do not block on it.

Root cause of url-ping failure

Two layers:

  1. Production bug (on main / stale installs): ListTriggeredHarnesses historically passed zero-value ComposeOpts.FetchPolicy (empty AllowedDomains), so FetchURL rejects every URL and URL-sourced agents are silently skipped at dispatch. fullsend run already sets a real policy; dispatch did not.

  2. Why this PR still fails in CI even though FetchPolicy plumbing is already on the branch: url-ping leased halfsend-06/test-repo-07, and ensure logged already installed, skipping. Behaviour only re-runs github setup --vendor when post-install validation fails. So the pool repo kept a pre-fix vendored CLI without DefaultPolicy, and never exercised the fixed binary from this PR.

Required work this iteration

A. Production fix (verify / complete)

Ensure dispatch uses fetch.DefaultPolicy when Options.FetchPolicy is nil:

  • internal/harnessdispatch/enumerate.go / core.go — already partially present; verify end-to-end that DispatchListTriggeredHarnessesResolveRegisteredPath / FetchAgentHarness always gets DefaultPolicy (or an explicit test policy) rather than a zero-value policy.
  • Keep/extend unit coverage (TestDispatch_FetchPolicyPlumbing or equivalent) proving github/raw.githubusercontent.com URLs are fetchable under DefaultPolicy and non-allowlisted domains are skipped.

B. Make behaviour CI actually run the fixed binary (required for green)

URL scenarios must not run against stale vendored installs. Pick the smallest durable approach consistent with existing patterns:

  • Preferred: for behaviour ensure/install, re-run github setup --vendor (using the suite’s current fullsend binary from the PR checkout) even when post-install files already exist — or add a cheap freshness check that forces re-vendor when the vendored CLI is outdated relative to the runner binary.
  • Alternative acceptable: a Background/hook step for URL-dispatch features that forces re-vendor on the leased enrolled repo before registering URL harnesses.

Do not rely on “hope the leased repo needs install.” test-repo-07 already installed, skipping is exactly why url-ping still fails.

C. Docs

Briefly note in behaviour-testing.md that URL-dispatch scenarios require a vendored CLI that includes FetchPolicy-aware harness dispatch (and how ensure refreshes it).

Out of scope

  • Do not revert ephemeral per-lease host repos.
  • Do not chase the WIF 409 flake unless incidental.

Done when

  • Production DefaultPolicy path is correct and unit-tested.
  • Behaviour ensure (or URL scenario setup) re-vendors so pool repos run this PR’s CLI.
  • Behaviour CI: URL happy-path (url-ping) passes. Other URL scenarios stay green. No shared-host 422 regressions.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:14 AM UTC · Completed 10:29 AM UTC
Commit: f45f2f5 · View workflow run →

Pool repos that pass post-install validation kept stale vendored binaries
from prior CI runs, causing URL-dispatch scenarios to fail silently because
the old binary lacked FetchPolicy-aware harness dispatch.

doEnsure now always runs github setup --vendor to push the current binary,
while skipping the settle wait for already-installed repos (GitHub Actions
already indexed the workflow). This ensures leased pool repos always run
the binary built from the current checkout.

Addresses review feedback on #5407
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (human-triggered)

Addressed all 3 required items from the human instruction: (A) verified the FetchPolicy production fix is complete and unit-tested, (B) changed doEnsure to always re-vendor the CLI binary so pool repos run the current checkout's binary instead of stale prior-run binaries, and (C) documented the FetchPolicy and binary freshness requirements in behaviour-testing.md. The WIF 409 flake was excluded per the human's instruction.

Fixed (3):

  1. Production DefaultPolicy path is correct and unit-tested: Verified end-to-end that Dispatch → ListTriggeredHarnesses → ResolveRegisteredPath always gets DefaultPolicy when Options.FetchPolicy is nil. The existing TestDispatch_FetchPolicyPlumbing test confirms non-github domains are skipped. No code changes needed — the plumbing was already correct on the branch.
  2. Behaviour ensure re-vendors so pool repos run current binary (pkg/behaviourtest/drivers/install/ensure.go): Changed doEnsure to always run github setup --vendor even when post-install validation passes. This ensures leased pool repos run the binary built from the current checkout rather than a stale binary from a prior CI run. Settle step is skipped on re-vendors since the workflow file already existed. Updated 7 tests in ensure_test.go to provide noopCLI stubs for the new always-install path and renamed TestDoEnsure_AlreadyInstalledSkipsCLI to TestDoEnsure_AlreadyInstalledReVendors with inverted assertion.
  3. Document FetchPolicy and binary freshness in behaviour-testing.md (docs/guides/dev/behaviour-testing.md): Added 'FetchPolicy and binary freshness' subsection under URL-sourced harness scenarios explaining that URL-dispatch requires FetchPolicy-aware dispatch, that RepoEnsurer always re-vendors, and that settle is skipped on re-vendors.

Disagreed (1):

  1. WIF 409 flake on test-repo-11: Human instruction explicitly said to ignore unless easy and do not block on it. This is an unrelated infra flake.

Tests: passed

Updated by fullsend fix agent

@ifireball

Copy link
Copy Markdown
Member

/fs-fix get coverage to 80% or above

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 11:03 AM UTC · Completed 11:22 AM UTC
Commit: a51c1ff · View workflow run →

Add tests for newly added PR functions to meet 80% patch coverage:

- scm/github: GetDefaultBranch, EnsureRepoPublic (all paths including
  re-verify error and still-private-after-update), ParseRepo,
  AddIssueLabels, CloseIssue, CommitFileToBranch, CreateChangeProposal,
  SubmitPullRequestReview (100% coverage for all PR-added methods)
- forge/fake: UpdateRepoVisibility (Repos, CreatedRepos, not-found,
  error injection paths) plus error injection and thread safety entries
- steps/cleanup: issue close, artifact dir removal, dummy ops clear
  (CleanupScenario 73.1% → 96.2%)
- steps/url_dispatch: relative resource not accessible after commit
  (givenURLSourcedCustomHarness 93.8% → 95.4%)

Addresses review feedback on #5407
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 4 (human-triggered)

Added 511 lines of unit tests across 4 test files to close patch coverage gaps. Key improvements: scm/github 43.5% to 100%, fake.UpdateRepoVisibility 0% to 100%, CleanupScenario 73.1% to 96.2%, givenURLSourcedCustomHarness 93.8% to 95.4%. All tests pass with -race.

Fixed (4):

  1. patch coverage below 80% for scm/github driver methods (pkg/behaviourtest/drivers/scm/github/github_test.go): Added unit tests for GetDefaultBranch, EnsureRepoPublic (all code paths including re-verify error and still-private-after-update), ParseRepo, AddIssueLabels, CloseIssue, CommitFileToBranch, CreateChangeProposal, SubmitPullRequestReview — achieving 100% coverage for all PR-added methods
  2. patch coverage below 80% for UpdateRepoVisibility fake (internal/forge/fake_test.go): Added tests for UpdateRepoVisibility covering Repos update, CreatedRepos update, not-found error, and error injection paths. Also added to error injection table and thread safety test.
  3. patch coverage below 80% for CleanupScenario paths (pkg/behaviourtest/steps/cleanup_test.go): Added tests for issue close, issue close error, artifact directory removal, dummy ops clearing, and dummy ops clear error paths. CleanupScenario coverage improved from 73.1% to 96.2%.
  4. patch coverage below 80% for relative resource accessibility check (pkg/behaviourtest/steps/url_dispatch_test.go): Added test for relative resource file not accessible after commit path in givenURLSourcedCustomHarness, with selectiveFailSCM that fails GetFileContent only for specific paths. Coverage improved from 93.8% to 95.4%.

Tests: passed

Strategy change: Iteration 4 (above escalation threshold): shifted from incremental test additions to systematically covering all thin-wrapper methods in the scm/github driver, which had 44 new lines at 0% coverage — the largest single gap

Decision points
  • Strategy escalation at iteration 4: focused on previously untested thin-wrapper methods in scm/github driver instead of hard-to-test integration code (alternatives: Test registerURLDispatchSteps godog registration, Test internal/forge/github LiveClient methods; rationale: scm/github driver methods are thin wrappers testable with FakeClient, giving the largest patch coverage improvement per line of test code)

Updated by fullsend fix agent

@ifireball
ifireball enabled auto-merge July 28, 2026 11:33
@ifireball
ifireball added this pull request to the merge queue Jul 28, 2026
Merged via the queue into main with commit a3dd597 Jul 28, 2026
16 of 17 checks passed
@ifireball
ifireball deleted the agent/3928-url-dispatch-behaviour-tests branch July 28, 2026 11:44
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 11:47 AM UTC · Completed 12:05 PM UTC
Commit: af7677a · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #5407 — behaviour tests for URL-sourced harness dispatch

PR: #5407 (test(#3928): add behaviour scenarios for URL-sourced harness dispatch)
Author: fullsend-ai-coder[bot] | Merged by: ifireball | Stats: +2267/−48, 24 files, 7 days open

Timeline

  1. Jul 9 — Issue #3928 filed by ifireball. Triage agent correctly identified prerequisite PR feat(dispatch): harness CEL dispatch for custom agents (#2889) #3820.
  2. Jul 21 10:15 UTC/fs-code triggered. Code agent produced initial PR with 3 Gherkin scenarios, step definitions, and fixtures.
  3. Jul 21–22 (7 fix iterations) — ifireball drove 7 /fs-fix cycles, each with detailed root-cause diagnosis:
    • Iter 1: Code agent assumed deprecated per-org .fullsend repo model; human corrected to per-repo mode.
    • Iter 2: Test asserted per-agent soft-skip on allowlist failure; human identified that production ValidateAgentEntries hard-fails the entire config.
    • Iter 3: Human demanded a concrete fix ("not another diagnosis-only note") for the behaviour hang.
    • Iter 4: Human identified that raw URL verification was unauthenticated + branch was hardcoded to /main/.
    • Iter 5: Code agent disagreed that hosting repo needed agents/triage.md for ADR-0045 — human traced LoadWithBaseresolveBaseResources and proved the agent wrong.
    • Iter 6: Human read pool-org dispatch logs and discovered a production bugListTriggeredHarnesses passed zero-value FetchPolicy, making URL-sourced dispatch silently fail.
    • Iter 7: Suite exceeded 30m timeout; push rejected due to workflows permission.
  4. Jul 22 — waynesun09 posted independent multi-agent review: 3 HIGH + 4 MEDIUM findings (HTTP timeout, resource lifecycle, path-boundary security, resource leak).
  5. Jul 26 — PR parked as draft (serial suite runtime too long, blocked on parallelism epic ci(e2e): enable parallel godog behaviour scenarios #3454).
  6. Jul 27–28 (4 more iterations) — Squash/rebase onto main, ephemeral per-lease host repos (fixing 422 tip race), always re-vendor CLI binary, coverage push to 80%+.
  7. Jul 28 11:44 UTC — Merged after ifireball's final approval.

Key Findings

1. Very high rework rate (11 iterations, all human-guided). Every fix iteration was driven by detailed human diagnosis with code-path traces, CI log evidence, and explicit scope boundaries. The code agent never independently diagnosed a CI failure or traced production behavior — it executed human-prescribed fixes. The root cause pattern: the agent wrote test assertions based on assumed production behavior instead of verified behavior, requiring 6 corrections before the tests matched reality.

2. Review agent quality gap. The review bot ran 7 passes and approved 5 times. Its findings were mostly low-severity (spelling, naming, comment style). It caught 1 legitimate HIGH (allowlist scenario mismatch). However, it missed all 3 HIGH findings that waynesun09 found: no-timeout HTTP client, resource leak on EnsureRepoPublic failure, and path-boundary prefix match security gap. It also repeated the same findings (test-fidelity, comment-style) across 3 consecutive iterations without deduplication.

3. Production bug discovered. The most impactful outcome was discovering that ListTriggeredHarnesses passed a zero-value FetchPolicy (nil AllowedDomains), making URL-sourced harness dispatch silently fail in production. This was found through the test-writing process but diagnosed by the human, not the agent.

Evidence for Existing Issues

Autonomy Assessment

The requires-manual-review label was correctly applied. Human reviewers added critical value: ifireball's architectural corrections drove all 11 iterations, and waynesun09's independent review found 3 HIGH issues the review agent missed entirely. The review agent's value was limited to consistent low-severity nit-catching and 1 legitimate HIGH finding. This PR provides evidence that the review agent is not yet ready for autonomous approval on complex integration/infrastructure test PRs involving multi-system interactions and security-relevant code paths.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/dispatch Workflow dispatch and triggers component/e2e End-to-end tests ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(behaviour): harness-dispatch URL-sourced custom agents

2 participants