fix(#6127): dispatch URL-sourced child harnesses that inherit a URL base - #6367
Conversation
Add e2e behaviour test coverage for harness dispatch with base: composition, testing three variants in a single consolidated scenario: 1. Local base + local child — both harnesses in the enrolled repo 2. Remote base + local child — base in hosting repo, child local 3. Both remote — base and child both in the hosting repo New Gherkin step definitions in base_dispatch.go: - "a custom harness X with base Y and:" — local child with local base reference - "a URL-sourced base harness X with:" — commits base to hosting repo without registering as agent - "a custom harness X with URL base Y and:" — local child with URL base reference - "a URL-sourced custom harness X with URL base Y and:" — remote child with URL base reference The URLBaseHarnesses field on World stores base harness URLs so child steps can reference them by name. The code fix (dispatch using LoadWithBase) was completed previously. This adds the missing behaviour test coverage identified in the triage. Note: pre-commit could not run (sandbox network restriction). golangci-lint not available in sandbox. go vet and go test -race passed. Related to #6127
|
🤖 Finished Review · ✅ Success · Started 8:47 AM UTC · Completed 9:02 AM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsLow
|
![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.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
/fs-fix Behaviour tests failed on this PR because of the new scenario, not a flake. Do not drop the remote-child variant. What failedCI: https://github.com/fullsend-ai/fullsend/actions/runs/32234152595 18 scenarios passed, 1 failed: Scenario: Base-composed harnesses dispatch with local and remote base variants Variants 1 and 2 succeeded:
Variant 3 never even scheduled a job:
The label-triggered dispatch run that did run local-child + remote-base-child: Harness dispatch log (exact skip reason): harness dispatch: skipping agent remote-child: load failed: resolving URL-sourced resources after base composition: base agent: fetching https://raw.githubusercontent.com/halfsend-06/test-repo-05-base-harness-host/main/.fullsend-cache/resources/sha256/629f2b41c984714e70a75aefd77c5e747377ad4feb5d02698600aa1e308283f6/content: fetch: non-200 status code: got 404 Root causeThis is a product bug in LoadWithBase, not a Gherkin/step-definition bug. Flow for a URL-sourced child whose base: is also a URL (enumerate.go sets SourceURL, then LoadWithBase):
func isFullsendCachePath(p, workspaceRoot string) bool {
if !filepath.IsAbs(p) || workspaceRoot == "" {
return false
}
...
}Dispatch sets WorkspaceRoot to filepath.Dir(configDir). In CI that is filepath.Dir(".fullsend") == ".", so CachePath returns a relative path. isFullsendCachePath then treats Existing unit test TestLoadWithBase_SourceURL_WithBase_AlreadyResolvedSkipped uses t.TempDir() (absolute), so it does not catch production dispatch. What to changeFix the product code so post-merge SourceURL resolution skips cache paths even when WorkspaceRoot / cache paths are relative (as dispatch uses). Do not weaken the security property that untrusted absolute host paths are still rejected. Concrete options (pick the smallest correct one):
Add a unit test that reproduces the CI failure: URL-sourced child + URL base, ComposeOpts.WorkspaceRoot relative (e.g. "."), agent inherited from the base (child YAML has no agent: field). LoadWithBase must succeed and must not try to fetch Keep the remote-child behaviour scenario. It is the regression test we want. Do not treat this as review-comment cleanup. The review findings are low; this CI failure is the work. Follow docs/contributing/go-code.md: stage changes, make go-test / go-vet, and verify ~80% patch coverage on new/changed production lines before committing. |
|
🤖 Finished Fix · ✅ Success · Started 9:39 AM UTC · Completed 9:51 AM UTC Commit: |
When dispatch sets WorkspaceRoot to filepath.Dir(configDir) and configDir is ".fullsend", WorkspaceRoot becomes "." and CachePath returns relative paths like ".fullsend-cache/resources/sha256/<hash>/content". The old isFullsendCachePath checked filepath.IsAbs(p) first, returning false for these relative cache paths. Post-merge SourceURL resolution then treated them as unresolved relative resources and tried to fetch them from the SourceURL host, producing a 404. Resolve both p and workspaceRoot to absolute paths before comparison so cache paths are recognised regardless of whether WorkspaceRoot was absolute or relative. The security property (untrusted absolute host paths rejected by validateBaseRelPath) is preserved: the callers check isFullsendCachePath to skip already-resolved values, and non-cache paths still flow through validateBaseRelPath which rejects absolute paths, path traversal, and other unsafe patterns. Add unit test coverage for relative cache paths in TestIsFullsendCachePath and a regression test (TestLoadWithBase_SourceURL_WithBase_RelativeWorkspaceRoot) that reproduces the CI failure: URL-sourced child + URL base with WorkspaceRoot=".", agent inherited from base. Addresses review feedback on #6367
🔧 Fix agent — iteration 1 (human-triggered)Fixed isFullsendCachePath to handle relative cache paths by resolving both p and workspaceRoot to absolute before comparison. Added unit tests for relative WorkspaceRoot in isFullsendCachePath and a regression test reproducing the CI failure (URL-sourced child + URL base, WorkspaceRoot="."). All tests pass with -race. Fixed (3):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 9:53 AM UTC · Completed 10:13 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
ready-for-merge
|
🤖 Finished Fix · ✅ Success · Started 10:15 AM UTC · Completed 10:25 AM UTC Commit: |
- Reset URLBaseHarnesses in resetScenarioWorld to prevent state leaking between scenarios via Clone's shallow copy (state-leak finding) - Use path.Join instead of filepath.Join for SCM repo-relative paths, aligning with the convention in url_dispatch.go (path-handling finding) - Extract registerLocalAgentConfig helper to deduplicate config-update boilerplate shared by givenCustomHarnessWithLocalBase and givenCustomHarnessWithURLBase (duplicate-code finding) Addresses review feedback on #6367
🔧 Fix agent — iteration 2 (bot-triggered)Addressed 3 of 6 review findings with code fixes (state-leak reset, path.Join alignment, config helper extraction). Disagreed with 3 findings: 2 are PR metadata concerns outside code scope, 1 is a positive security confirmation requiring no action. Fixed (3):
Disagreed (3):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:27 AM UTC · Completed 10:42 AM UTC Commit: |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
/fs-fix codecov/patch is still failing after the review/fix loop. Do not change product behaviour, drop the remote-child scenario, or retitle the PR. Only add unit tests so patch coverage reaches ~80%. Current gapCodecov on HEAD
Happy-path tests already exist. Error-return branches in Tests to add in
|
|
🤖 Finished Fix · ✅ Success · Started 10:45 AM UTC · Completed 11:01 AM UTC Commit: |
Cover uncovered error-return branches in base_dispatch.go and compose.go to push codecov/patch coverage from ~64% toward 80%. 22 new tests for base_dispatch.go error paths: - registerLocalAgentConfig: GetFileContent error, invalid config YAML, CommitFile error, existing-agent upsert (found == true branch) - givenCustomHarnessWithLocalBase: CommitFile error, invalid resource YAML, GetConfig error, whitespace validation - givenURLSourcedBaseHarness: CommitFile error, commitRelativeResources error, file not accessible, resource not accessible, GetDefaultBranch error, raw URL not accessible, GetConfig error, invalid config YAML, commit allowlist error - givenCustomHarnessWithURLBase: CommitFile error, invalid resource YAML, GetConfig error, whitespace validation - givenURLSourcedCustomHarnessWithURLBase: whitespace validation 3 new test cases for isFullsendCachePath in compose_test.go: - cache directory itself (rel == ".") - relative cache directory - workspace root is not a cache path (rel == "..") Addresses review feedback on #6367
🔧 Fix agent — iteration 3 (human-triggered)Added 25 new test cases across 2 files to close the codecov/patch coverage gap. 22 tests cover error-return branches in base_dispatch.go (CommitFile errors, config read/parse/commit errors, resource accessibility failures, GetDefaultBranch errors, raw URL failures, whitespace validation, existing-agent upsert). 3 tests cover edge cases in compose.go's isFullsendCachePath. No production code changed. Fixed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 11:02 AM UTC · Completed 11:19 AM UTC Commit: |
ifireball
changed the title
|
🤖 Finished Retro · ❌ Failure · Started 12:12 PM UTC · Completed 12:12 PM UTC Commit: |
Merge origin/main to incorporate recent changes including: - PER_REPO_WIF_REPOS=* for GCP public mint mode (PR #6377) - Base dispatch e2e tests (PR #6367) - CEL trigger jira-poll (PR #6340) - Other main-branch updates Resolved conflict in docs/cli/mint.md: adopted main's updated --public flag description (PER_REPO_WIF_REPOS=* replaces ALLOWED_ORGS=* for GCP) while preserving the new --status-auth, --status-github-group, and --status-github-client-id flags from this PR. Addresses review feedback on #6365
1 participant
Summary
Add behaviour (e2e) test coverage for harness dispatch with
base:composition, covering three variants: local base + local child, remote base + local child, and both remote. The code fix for dispatch usingLoadWithBasewas already completed; this PR addresses the missing test coverage gap identified in the triage.Related Issue
Related to #6127
Changes
base-dispatch.featurewith a consolidated scenario testing all three base-composition variants in a single CI roundbase_dispatch.gowith four new Gherkin step definitions:a custom harness "X" with base "Y" and:— local child referencing local basea URL-sourced base harness "X" with:— commits base to hosting repo (not registered as agent)a custom harness "X" with URL base "Y" and:— local child referencing remote URL basea URL-sourced custom harness "X" with URL base "Y" and:— remote child referencing remote URL baseURLBaseHarnessesfield toworld.Worldfor cross-step base URL stateregistry.goTesting
go test -race)pkg/behaviourtest/stepstests pass (no regressions)go build -tags behaviour ./e2e/behaviour/...compiles cleanlygo vetpassesCloses #6127
Post-script verification
agent/6127-base-dispatch-e2e)a7ef381e50b0eb77d3fbedecc9ce88b510642dd0..HEAD)