test(#6168): stop BT steps from using install.State config indirection by fullsend-ai-coder[bot] · Pull Request #6171 · fullsend-ai/fullsend

fullsend-ai-coder · 2026-08-13T09:57:23Z

Summary

Replace all behaviour-test step (and step-test fake) uses of install.State config indirection (ConfigOwner(), ConfigRepo(), ConfigPathPrefix()) with direct World fields (w.Org, w.RepoName) and fixed .fullsend/ conventions.

Changes

dispatch.go: Replace 14 ConfigOwner()/ConfigRepo() calls with w.Org/w.RepoName across givenKillSwitchActive, DeactivateKillSwitch, givenCustomHarness, givenDisabledCustomHarness, and commitLocalHarnessResources
cleanup.go: Replace 1 ConfigOwner()/ConfigRepo() call in dummy script cleanup with w.Org/w.RepoName
dummy_agent.go: Replace 1 ConfigOwner()/ConfigRepo() call in parseDummyAgentTable with w.Org/w.RepoName
url_dispatch.go: Replace 2 ConfigOwner()/ConfigRepo() calls in givenURLSourcedCustomHarness with w.Org/w.RepoName
world.go: Simplify BehaviourScriptPath() to hardcode .fullsend prefix instead of reading Install.ConfigPathPrefix()
Test fakes: Remove fakeDispatchInstall, fakeCleanupInstall, and fakeURLInstall types; update all tests to set w.Org/w.RepoName directly
Workflow/artifact helpers (TriageWorkflowRepo, TriageWorkflowFile, etc.) remain on install.State as permitted by the issue

Testing

All pkg/behaviourtest/steps tests pass (with -race)
All pkg/behaviourtest/world tests pass
go vet and go build clean
No ConfigOwner/ConfigRepo/Mode/ConfigPathPrefix references remain in step files or world

Closes #6168

Post-script verification

Branch is not main/master (agent/6168-bt-steps-drop-state-indirection)
Secret scan passed (gitleaks — fafb2df30a66eb2dfa0ae9e4dff6641bfebe41a4..HEAD)
PR body secret scan passed (gitleaks — no-git)

Replace all step (and step-test fake) uses of ConfigOwner(), ConfigRepo(), and ConfigPathPrefix() with direct World fields: w.Org, w.RepoName, and hardcoded ".fullsend/" convention. Production step changes: - dispatch.go: 14 ConfigOwner/ConfigRepo calls → w.Org/w.RepoName - cleanup.go: 1 ConfigOwner/ConfigRepo call → w.Org/w.RepoName - dummy_agent.go: 1 ConfigOwner/ConfigRepo call → w.Org/w.RepoName - url_dispatch.go: 2 ConfigOwner/ConfigRepo calls → w.Org/w.RepoName - world.go: BehaviourScriptPath() now hardcodes .fullsend prefix instead of reading Install.ConfigPathPrefix() Test fake cleanup: - Remove fakeDispatchInstall (dispatch_test.go) - Remove fakeCleanupInstall (cleanup_test.go) - Remove fakeURLInstall (url_dispatch_test.go) - Update all affected tests to set w.Org/w.RepoName directly Workflow/artifact helpers (TriageWorkflowRepo, TriageWorkflowFile, AgentWorkflowFile, AgentArtifactName) remain on install.State as permitted by the issue — they are fixed per-repo constants. Closes #6168

fullsend-ai-review · 2026-08-13T09:58:41Z

🤖 Finished Review · ✅ Success · Started 9:58 AM UTC · Completed 10:09 AM UTC

Commit: 7eccc94 · View workflow run →

codecov · 2026-08-13T10:01:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-08-13T10:09:28Z

Review

Findings

Low

[error handling] pkg/behaviourtest/steps/cleanup.go:522 — The identity guard for dummy-script cleanup in CleanupScenario logs directly when Org/RepoName are empty, while the kill-switch cleanup path calls DeactivateKillSwitch (which returns an error that is then logged). Both patterns are correct within CleanupScenario's void-function design — the kill-switch guard lives inside a helper, while the dummy-ops guard is inline — but the resulting log messages differ in shape between the two paths. Informational only; no functional impact.

Previous run

Findings

Medium

[test integrity] pkg/behaviourtest/steps/dispatch_test.go:17 — TestGivenCustomHarness_Validation now passes for the wrong reason. The test uses w := &world.World{} (no Org or RepoName), but the new identity guard at the top of givenCustomHarness returns an error for empty Org/RepoName before the name/doc emptiness check is reached. Both calls return "no repo configured" instead of "harness name and contents are required". The test still passes (it only checks require.Error), but it no longer validates name/doc emptiness. Compare with TestGivenURLSourcedCustomHarness_Validation which was correctly updated to set Org: "org", RepoName: "repo".
Remediation: Set Org and RepoName on the World: w := &world.World{Org: "org", RepoName: "repo"}

Previous run

Looks good to me

Labels: PR modifies behaviour test steps and world under pkg/behaviourtest/, which is the e2e test infrastructure.

Previous run

Review

Findings

Medium

[test integrity] pkg/behaviourtest/steps/dispatch_test.go:17 — TestGivenCustomHarness_Validation now passes for the wrong reason. The test uses w := &world.World{} (no Org or RepoName), but the new identity guard at the top of givenCustomHarness returns an error for empty Org/RepoName before the name/doc emptiness check is reached. Both calls return "no repo configured" instead of "harness name and contents are required". The test still passes (it only checks require.Error), but it no longer validates name/doc emptiness. Compare with TestGivenURLSourcedCustomHarness_Validation which was correctly updated to set Org: "org", RepoName: "repo".
Remediation: Set Org and RepoName on the World: w := &world.World{Org: "org", RepoName: "repo"}

Previous run (2)

Looks good to me

Labels: PR modifies behaviour test steps and world under pkg/behaviourtest/, which is the e2e test infrastructure.

ifireball · 2026-08-13T10:17:28Z

/fs-fix Add clear fail-fast errors when steps use w.Org / w.RepoName before the enrolled repo identity is set (empty values), instead of calling SCM with blank owner/repo. Prefer the same style of error already used elsewhere (e.g. "no repo configured; call 'Given the enrolled test repository' …"). Cover the paths this PR switched from Install.ConfigOwner()/ConfigRepo() to World fields (dispatch/cleanup/dummy_agent/url_dispatch as needed). Add unit tests that assert the clear error on empty identity.

fullsend-ai-coder · 2026-08-13T10:18:36Z

🤖 Finished Fix · ✅ Success · Started 10:18 AM UTC · Completed 10:30 AM UTC

Commit: 7eccc94 · View workflow run →

Add checks at the top of functions that use w.Org / w.RepoName for SCM calls. Without these guards, an unset identity silently passes empty strings to the SCM driver, producing confusing API errors instead of a clear "no repo configured" message. Guarded functions: givenKillSwitchActive, DeactivateKillSwitch, givenCustomHarness, givenDisabledCustomHarness, commitLocalHarnessResources, parseDummyAgentTable, givenURLSourcedCustomHarness, and the cleanup dummy-ops path (logs instead of returning, since CleanupScenario is void). Addresses review feedback on #6171

fullsend-ai-coder · 2026-08-13T10:30:24Z

🔧 Fix agent — iteration 1 (human-triggered)

Added fail-fast empty-identity guards to all 8 functions (plus cleanup log-guard) that use w.Org/w.RepoName for SCM calls, matching the existing 'no repo configured' error style. Added 7 new test functions covering the guards. Fixed 2 existing tests that needed Org/RepoName set to pass the new guards.

Fixed (12):

missing fail-fast guard in givenKillSwitchActive (pkg/behaviourtest/steps/dispatch.go): Added early return with clear error when w.Org or w.RepoName is empty, preventing blank owner/repo SCM calls
missing fail-fast guard in DeactivateKillSwitch (pkg/behaviourtest/steps/dispatch.go): Added early return with clear error when w.Org or w.RepoName is empty
missing fail-fast guard in givenCustomHarness (pkg/behaviourtest/steps/dispatch.go): Added early return with clear error when w.Org or w.RepoName is empty
missing fail-fast guard in givenDisabledCustomHarness (pkg/behaviourtest/steps/dispatch.go): Added early return with clear error when w.Org or w.RepoName is empty
missing fail-fast guard in commitLocalHarnessResources (pkg/behaviourtest/steps/dispatch.go): Added early return with clear error when w.Org or w.RepoName is empty
missing fail-fast guard in CleanupScenario dummy-ops path (pkg/behaviourtest/steps/cleanup.go): Added guard that logs and skips CommitFile when w.Org or w.RepoName is empty (CleanupScenario is void, so it logs instead of returning an error)
missing fail-fast guard in parseDummyAgentTable (pkg/behaviourtest/steps/dummy_agent.go): Added early return with clear error when w.Org or w.RepoName is empty
missing fail-fast guard in givenURLSourcedCustomHarness (pkg/behaviourtest/steps/url_dispatch.go): Added early return with clear error when w.Org or w.RepoName is empty
unit tests for empty identity guards (pkg/behaviourtest/steps/dispatch_test.go): Added TestGivenKillSwitchActive_EmptyIdentity (subtable), TestDeactivateKillSwitch_EmptyIdentity, TestGivenCustomHarness_EmptyIdentity, TestGivenDisabledCustomHarness_EmptyIdentity, TestCommitLocalHarnessResources_EmptyIdentity
unit test for cleanup empty identity guard (pkg/behaviourtest/steps/cleanup_test.go): Added TestCleanupScenario_ClearsDummyOps_EmptyIdentity verifying log output and no SCM call
unit test for dummy_agent empty identity guard (pkg/behaviourtest/steps/dummy_agent_test.go): Added TestParseDummyAgentTable_EmptyIdentity and updated TestParseDummyAgentTable_RequiresFixturesRoot to set Org/RepoName so the new guard doesn't mask the FixturesRoot check
unit test for url_dispatch empty identity guard (pkg/behaviourtest/steps/url_dispatch_test.go): Added TestGivenURLSourcedCustomHarness_EmptyIdentity and updated TestGivenURLSourcedCustomHarness_Validation to set Org/RepoName

Tests: passed

Next steps:

/fs-review — request a re-review of the changes
/fs-fix <your instruction> — run another fix pass with specific guidance
Push commits directly — review re-runs automatically on push
_{Updated by fullsend fix agent}

fullsend-ai-review · 2026-08-13T10:31:34Z

🤖 Finished Review · ✅ Success · Started 10:31 AM UTC · Completed 10:43 AM UTC

Commit: fe760ee · View workflow run →

ifireball · 2026-08-13T10:49:48Z

/fs-fix

fullsend-ai-coder · 2026-08-13T10:51:10Z

🤖 Finished Fix · ✅ Success · Started 10:51 AM UTC · Completed 10:58 AM UTC

Commit: fe760ee · View workflow run →

The test was passing for the wrong reason: the empty-identity guard added in the previous commit fires before the name/doc validation check, so both error paths returned "no repo configured" instead of "harness name and contents are required". Set Org and RepoName on the World, matching the pattern in TestGivenURLSourcedCustomHarness_Validation. Addresses review feedback on #6171

fullsend-ai-coder · 2026-08-13T10:58:50Z

🔧 Fix agent — iteration 2 (human-triggered)

Fixed 1 review finding: TestGivenCustomHarness_Validation was testing the identity guard instead of the name/doc validation. Set Org/RepoName on the World to match the correctly-updated URL dispatch test.

Fixed (1):

TestGivenCustomHarness_Validation passes for the wrong reason due to missing Org/RepoName: Set Org and RepoName on the World struct in TestGivenCustomHarness_Validation so the identity guard is satisfied and the test actually validates the name/doc emptiness check, matching the pattern in TestGivenURLSourcedCustomHarness_Validation

Tests: passed

Next steps:

/fs-review — request a re-review of the changes
/fs-fix <your instruction> — run another fix pass with specific guidance
Push commits directly — review re-runs automatically on push
_{Updated by fullsend fix agent}

fullsend-ai-review · 2026-08-13T11:00:16Z

🤖 Finished Review · ✅ Success · Started 11:00 AM UTC · Completed 11:11 AM UTC

Commit: ce94937 · View workflow run →

fullsend-ai-retro · 2026-08-13T11:53:28Z

🤖 Finished Retro · ✅ Success · Started 11:53 AM UTC · Completed 12:14 PM UTC

Commit: ce94937 · View workflow run →

fullsend-ai-retro · 2026-08-13T12:14:40Z

Retro: PR #6171 — BT steps drop install.State config indirection

Workflow outcome: Good. The pipeline worked well overall — 3 commits across 2 hours from creation to merge, with the review-fix-review loop catching a bug introduced by the fix agent.

Timeline

Issue #6168 created by ifireball — refactor BT steps to use w.Org/w.RepoName instead of install.State indirection.
Triage (run 31687089781) completed in ~5 min, correctly identifying affected files and scope.
Code agent (run 31687582369) produced a clean implementation in ~18 min (commit 7eccc94), opening PR test(#6168): stop BT steps from using install.State config indirection #6171.
Review agent (run 31688951109) approved — implementation was correct as scoped by the issue.
Human (ifireball) triggered /fs-fix requesting fail-fast identity guards — a proactive enhancement beyond the issue scope.
Fix agent (run 31690445435) added guards to 8 functions and 7 new tests (commit fe760ee), but missed updating TestGivenCustomHarness_Validation while correctly updating the analogous TestGivenURLSourcedCustomHarness_Validation.
Review agent (run 31691398795) correctly caught the missed test — medium finding that the test now passed for the wrong reason. Inline comment posting failed with 422 (fell back to review body).
Human triggered bare /fs-fix, fix agent (run 31692849816) fixed the test (commit ce94937).
Review agent (run 31693500097) approved. Human approved. Merged.

What went well

Review agent caught a fix-agent regression. The second review correctly identified that TestGivenCustomHarness_Validation was passing for the wrong reason after the fix agent added identity guards but didn't update the test's World literal.
The initial review was correct. The test integrity issue did not exist at commit 7eccc94 — it was introduced by the fix agent's commit. The initial approval was justified.
Human-triggered scope expansion worked smoothly. The human's /fs-fix with detailed instructions to add defensive guards was a proactive quality improvement, and the fix agent implemented it well (minus the one missed test).
Graceful 422 fallback. When inline comment posting failed, the finding was correctly embedded in the review body and still influenced the fix cycle.

Rework cost

The fix agent's missed analogous test caused one extra review+fix cycle (~30 min of wall-clock time, ~2 additional agent runs). This is a well-known pattern.

Evidence for existing issues (no new proposals needed)

Fix agent missed analogous test update: Directly covered by agents#661 (mirrored implementations), agents#129 (parallel instances), agents#587 (fix completeness across similar code sites), and agents#759 (self-check for same-defect-class regressions). This retro provides another concrete example of the pattern.
Inline comment 422 on re-review: Covered by agents#193, agents#699, agents#760, and fullsend#6039. The fallback logic handled it correctly.
Code agent didn't proactively add guards: The code agent implemented the issue as scoped. Existing issues agents#288 and agents#477 track the broader concern of proactive defensive coding.

fullsend-ai-coder Bot requested a review from a team as a code owner August 13, 2026 09:57

fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Aug 13, 2026

fullsend-ai-coder Bot assigned ifireball Aug 13, 2026

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/e2e End-to-end tests labels Aug 13, 2026

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Aug 13, 2026

fullsend-ai-review Bot approved these changes Aug 13, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Aug 13, 2026

ifireball approved these changes Aug 13, 2026

View reviewed changes

ifireball added this pull request to the merge queue Aug 13, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 13, 2026

ifireball added this pull request to the merge queue Aug 13, 2026

Merged via the queue into main with commit 2358e92 Aug 13, 2026
23 checks passed

ifireball deleted the agent/6168-bt-steps-drop-state-indirection branch August 13, 2026 11:51

This was referenced Aug 16, 2026

test(#6169): suite uses unified Driver for repo allocate/deallocate/finalize #6172

Closed

test(behaviour): suite uses one install Driver for repo allocate/deallocate/finalize #6169

Closed

fullsend-ai-retro Bot mentioned this pull request Aug 16, 2026

test(#6169): suite uses unified Driver for repo allocate/deallocate/finalize #6260

Merged

10 tasks

Uh oh!

Conversation

fullsend-ai-coder Bot commented Aug 13, 2026

Summary

Changes

Testing

Post-script verification

Uh oh!

fullsend-ai-review Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 13, 2026

Codecov Report

Uh oh!

fullsend-ai-review Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Low

Findings

Medium

Review

Findings

Medium

Uh oh!

This comment was marked as outdated.

Uh oh!

ifireball commented Aug 13, 2026

Uh oh!

fullsend-ai-coder Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-coder Bot commented Aug 13, 2026

🔧 Fix agent — iteration 1 (human-triggered)

Tests: passed

Uh oh!

fullsend-ai-review Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

ifireball commented Aug 13, 2026

Uh oh!

fullsend-ai-coder Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-coder Bot commented Aug 13, 2026

🔧 Fix agent — iteration 2 (human-triggered)

Tests: passed

Uh oh!

fullsend-ai-review Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fullsend-ai-retro Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-retro Bot commented Aug 13, 2026

Retro: PR #6171 — BT steps drop install.State config indirection

Timeline

What went well

Rework cost

Evidence for existing issues (no new proposals needed)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fullsend-ai-review Bot commented Aug 13, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 13, 2026 •

edited

Loading

fullsend-ai-coder Bot commented Aug 13, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 13, 2026 •

edited

Loading

fullsend-ai-coder Bot commented Aug 13, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 13, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Aug 13, 2026 •

edited

Loading