test(#5206): add fork dispatch feature file, cleanup, and docs - #5309
Conversation
Add Gherkin feature file with three fork PR dispatch scenarios (opened, synchronize, negative issue-only), extend CleanupScenario to close fork PRs and delete test branches on fork repos, and document pool-org prerequisites and fork lifecycle in behaviour-testing.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 10:30 AM UTC · Completed 10:43 AM UTC |
Site previewPreview: https://58c573e4-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsHigh
Medium
Low
Labels: PR adds fork dispatch behaviour test scenarios under e2e/behaviour/features/dispatch/ Previous runReviewFindingsMedium
Low
Labels: PR adds e2e behaviour test infrastructure for fork dispatch scenarios |
|
/fs-fix the review agent comments |
|
🤖 Finished Fix · ✅ Success · Started 5:41 PM UTC · Completed 5:50 PM UTC |
Use httptest.NewServer instead of real HTTP calls in fork branch deletion tests. Parameterize deleteForkBranch base URL, drain response body for connection reuse, and add comment separators in CleanupScenario. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔧 Fix agent — iteration 1 (human-triggered)Addressed all four review findings. Fixed test-integrity by parameterizing deleteForkBranch's base URL and replacing the real-HTTP test with three httptest-based tests covering success (204), not-found (404), and error (500) cases. Fixed error-handling by draining response body before closing. Fixed code-organization by adding comment separators in CleanupScenario. Recorded reasoned disagreement on architectural-violation — adding DeleteBranch to forge.Client is out of scope for this PR and the code follows established precedent. Fixed (3):
Disagreed (1):
Tests: passed Decision points
Updated by fullsend fix agent |
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
|
/fs-fix get the coverage up to 80% or more |
|
🤖 Finished Fix · ✅ Success · Started 6:48 AM UTC · Completed 6:57 AM UTC |
Cover the two remaining untested error paths in deleteForkBranch: request creation failure (invalid URL) and HTTP client failure (connection refused). Raises patch coverage on new code from ~77% to ~95%. Addresses review feedback on #5309
🔧 Fix agent — iteration 2 (human-triggered)Added 2 unit tests covering deleteForkBranch error paths (request creation failure and HTTP client failure). Patch coverage on new code improved from ~77% to ~95%, exceeding the 80% threshold. deleteForkBranch function is now at 100% coverage. Fixed (1):
Tests: passed Decision points
Updated by fullsend fix agent |
|
/fs-fix fix the architectural-violation noted by the fullsend-ai-agent, the fact that we have architecture violations elsewhere in the code does not mean we are allowed make the problem bigger, the BT driver architecture is important! |
|
🤖 Finished Fix · ✅ Success · Started 7:53 AM UTC · Completed 8:06 AM UTC |
…lation in cleanup Route fork branch deletion through the forge abstraction instead of making direct GitHub REST API calls from pkg/behaviourtest/steps/cleanup.go. - Add DeleteRef method to forge.Client interface and implement in GitHub LiveClient (using existing delete_ helper) and FakeClient - Add DeleteBranch method to scm.Driver interface and implement in GitHub scm driver (wraps forge.Client.DeleteRef) - Replace deleteForkBranch direct HTTP function in cleanup.go with w.SCM.DeleteBranch, removing net/http dependency and githubAPIBaseURL - Replace httptest-based TestDeleteForkBranch_* tests with scm.Driver fake-based tests that verify the integration through CleanupScenario - Remove w.Token guard on branch deletion (auth handled by forge client) Addresses review feedback on #5309
🔧 Fix agent — iteration 3 (human-triggered)Fixed the architectural-violation finding by adding DeleteRef to forge.Client and DeleteBranch to scm.Driver, then routing cleanup.go through the forge abstraction instead of making direct GitHub REST API calls. This is a strategy change from iterations 1-2 which disagreed with the finding. Fixed (1):
Tests: passed
Decision points
Updated by fullsend fix agent |
|
/fs-fix Address the remaining open review findings (API versioning / interface-breakage comments were dismissed — ignore those):
Do not reopen or "fix" the dismissed breaking-interface / DeleteBranch version-pin findings. |
|
🤖 Finished Fix · ✅ Success · Started 8:56 PM UTC · Completed 9:04 PM UTC |
- Set w.ForkPRBranch immediately after CreateBranch succeeds so CleanupScenario can delete the branch even when CommitFileToFork or CreateForkChangeProposal fails (orphaned branch leak fix) - Add test assertions verifying ForkPRBranch is set on commit and PR-creation error paths - Rename CreateForkChangeProposal params headBranch/baseBranch to head/base for consistency with CreateChangeProposal Addresses review feedback on #5309
🔧 Fix agent — iteration 8 (human-triggered)Addressed both remaining review findings: (1) fixed orphaned fork branch leak by setting ForkPRBranch immediately after CreateBranch succeeds, with test coverage for both error paths; (2) renamed CreateForkChangeProposal parameters headBranch/baseBranch to head/base for consistency with CreateChangeProposal. Fixed (2):
Tests: passed Updated by fullsend fix agent |
|
/fs-fix CI Cause
Fix
Do not remove the method from |
|
🤖 Finished Fix · ✅ Success · Started 5:41 AM UTC · Completed 5:51 AM UTC |
…GitLab client The forge.Client interface gained DeleteRef and CreateCrossRepoChangeProposal in this PR branch, but the GitLab LiveClient (merged from main) was not updated, causing a compile-time interface check failure (go vet). - DeleteRef: implements branch and tag deletion via the GitLab Branches and Tags APIs, mapping "heads/<name>" and "tags/<name>" ref paths to the appropriate endpoints. - CreateCrossRepoChangeProposal: returns forge.ErrNotSupported because GitLab handles fork MRs via its existing source_branch/target_branch API (the fork relationship is implicit in the project). - Adds unit tests for both methods covering success, not-found, error, and unsupported-prefix paths. Addresses /fs-fix feedback on #5309
🔧 Fix agent — iteration 9 (human-triggered)Fixed go vet failure caused by GitLab LiveClient missing two new forge.Client interface methods (CreateCrossRepoChangeProposal, DeleteRef). Merged main to bring GitLab files onto the branch, implemented both methods, and added 5 unit tests. All tests and go vet pass. Fixed (3):
Tests: passed
Decision points
Updated by fullsend fix agent |
Findings addressed or dismissed; re-verifying behaviour on current HEAD.
ifireball
left a comment
There was a problem hiding this comment.
Approving after addressing remaining review threads (orphan-branch fix confirmed; naming dismissed as intentional; API-versioning dismissed earlier; bot CHANGES_REQUESTED dismissed) and verifying CI on current HEAD including behaviour.
|
🤖 Finished Retro · ✅ Success · Started 8:54 AM UTC · Completed 9:11 AM UTC |
Retro: PR #5309 — fork dispatch feature file, cleanup, and docsTimeline
Review qualityZero false positives. Every review finding was technically valid. The architectural-violation finding was the standout — the human strongly agreed and overruled the fix agent's initial disagreement. Five false negatives. The human caught critical runtime bugs the review agent missed: (1) CEL typo Rework rate9 iterations is high. ~5.5 were avoidable:
The forge abstraction disagreement in iteration 1 was especially costly — the fix agent cited precedent to justify expanding the violation, the human overruled, and the subsequent proper fix cascaded into 4+ additional iterations. Evidence for existing issues
Proposals filed
|
Add docs/contributing/cel-triggers.md documenting that harness trigger CEL expressions and .feature CEL filters match the normalized event model, not raw forge webhook action names. The raw name silently evaluates false, so a mismatched trigger never fires (root cause of the PR #5309 fork-dispatch bug: "synchronize" vs normalized "synchronized"). The file is CEL/normalized-event focused rather than coupling CEL editing to behaviour-test authoring: CEL expressions appear in harnesses whether or not BT exercises them, and BT covers plenty unrelated to CEL. It points to docs/normative/normalized-event/v1/README.md as the source of truth for the full transition-kind vocabulary and per-kind sub-object / entity.kind requirements instead of duplicating a table that drifts. Add a "CEL Triggers" row to the AGENTS.md topic-guidance index. The website sidebar auto-discovers docs/contributing/ via getMarkdownFiles(), so no vitepress config change is needed. Closes #5402 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Add docs/contributing/cel-triggers.md documenting that harness trigger CEL expressions and .feature CEL filters match the normalized event model, not raw forge webhook action names. The raw name silently evaluates false, so a mismatched trigger never fires (root cause of the PR #5309 fork-dispatch bug: "synchronize" vs normalized "synchronized"). The file is CEL/normalized-event focused rather than coupling CEL editing to behaviour-test authoring: CEL expressions appear in harnesses whether or not BT exercises them, and BT covers plenty unrelated to CEL. It points to docs/normative/normalized-event/v1/README.md as the source of truth for the full transition-kind vocabulary and per-kind sub-object / entity.kind requirements instead of duplicating a table that drifts. Add a "CEL Triggers" row to the AGENTS.md topic-guidance index. The website sidebar auto-discovers docs/contributing/ via getMarkdownFiles(), so no vitepress config change is needed. Closes #5402 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Summary
e2e/behaviour/features/dispatch/fork-dispatch.featurewith three Gherkin scenarios: fork PR opened dispatchespull_request_targetharness, fork PR synchronize dispatches harness, and negative test verifying fork PR does not trigger issue-only harnessCleanupScenarioto close open fork PRs (viaCloseIssueon the base repo) and delete test branches on the fork repo (via GitHub REST API, followingpkg/e2etest/cleanup.goprecedent)docs/guides/dev/behaviour-testing.mdcovering pool-org prerequisites, fork lifecycle (long-lived fork repo, per-scenario branch/PR cleanup), andBackground:step usage patternRelated Issue
Closes #5206 (sub-issue D of #3731)
Changes
e2e/behaviour/features/dispatch/fork-dispatch.featurepkg/behaviourtest/steps/cleanup.goCleanupScenariopkg/behaviourtest/steps/cleanup_test.godocs/guides/dev/behaviour-testing.mdTesting
go test ./pkg/behaviourtest/...passes (all 10 packages)go vet ./pkg/behaviourtest/...cleanscan-secrets --stagedpassesChecklist
dispatch.featureconventionspkg/e2etest/cleanup.gopattern🤖 Generated with Claude Code
Closes #5206
Post-script verification
agent/5206-fork-dispatch-feature)24d41836a2de6b7abaa6b20be2bdac9a61f7e0d3..HEAD)