test(#3732): add kill switch and CEL is_fork exclusion to fork dispatch contract

[fullsend-ai-coder]

Summary

Add the two remaining fork-PR behaviour contract rows (kill switch, CEL is_fork exclusion) to fork-dispatch.feature and document the four-row contract in behaviour-testing.md as the canonical template for per-agent CEL port PRs.

Related Issue

Closes #3732

Changes

• Add a disabled harness fork-pr-killed to the existing "Fork PR label dispatches harness" scenario, asserting the kill switch prevents dispatch on fork events
• Add a CEL-guarded harness fork-pr-nofork with !event.state.change_proposal.is_fork in its trigger, asserting harness-level fork exclusion produces an empty matrix
• Both harnesses are consolidated into the existing scenario (not separate scenarios) per maintainer guidance to conserve parallel execution slots
• Document the four-row fork PR behaviour contract in docs/guides/dev/behaviour-testing.md including the consolidation pattern and unauthorized-actor split reference

Testing

• go vet -tags behaviour ./e2e/behaviour/... passes
• make behaviour-test (requires live GitHub infrastructure — must pass in CI)
• make lint (pre-commit could not run due to sandbox network restrictions — must pass in CI)

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• No secrets in diff
• Changes scoped to issue test(behaviour): harness-dispatch fork PR behaviour contract #3732 only

---

Closes #3732

Post-script verification

• Branch is not main/master (agent/3732-fork-dispatch-contract)
• Secret scan passed (gitleaks — 5e2b16052a01aab8ece6d4911d1f65b977dd9313..HEAD)
• PR body secret scan passed (gitleaks — no-git)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:06 AM UTC · Completed 10:27 AM UTC
Commit: c5d5b07 · View workflow run →

[github-actions]

Site preview

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

Commit: 186e96c91b0a65e0f5d05c03946a627f134f8c0e

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [test coverage gap] pkg/behaviourtest/steps/dispatch_test.go — The givenKillSwitchActive function has four error paths (GetFileContent, ParsePerRepoConfigWriter, Marshal, CommitFile) but tests cover only GetFileContent and CommitFile errors, omitting the ParsePerRepoConfigWriter error path. Providing malformed YAML as config content is a realistic failure mode (corrupted or incompatible config) that should be exercised. The Marshal error path is difficult to trigger with the real implementation and can be reasonably skipped.
  Remediation: Add a test that provides malformed YAML to fakeDispatchSCM.fileContent (e.g., []byte(":::invalid")) and asserts the returned error contains "parsing config".

Previous run

Review

Findings

Low

• [test coverage gap] pkg/behaviourtest/steps/dispatch_test.go — The givenKillSwitchActive function has four error paths (GetFileContent, ParsePerRepoConfigWriter, Marshal, CommitFile) but tests cover only GetFileContent and CommitFile errors, omitting the ParsePerRepoConfigWriter error path. Providing malformed YAML as config content is a realistic failure mode (corrupted or incompatible config) that should be exercised. The Marshal error path is difficult to trigger with the real implementation and can be reasonably skipped.
  Remediation: Add a test that provides malformed YAML to fakeDispatchSCM.fileContent (e.g., []byte(":::invalid")) and asserts the returned error contains "parsing config".

Previous run (2)

Review

Findings

Low

• [test coverage gap] pkg/behaviourtest/steps/dispatch_test.go — The givenKillSwitchActive function has four error paths (GetFileContent, ParsePerRepoConfigWriter, Marshal, CommitFile) but tests cover only GetFileContent and CommitFile errors, omitting the ParsePerRepoConfigWriter error path. Providing malformed YAML as config content is a realistic failure mode (corrupted or incompatible config) that should be exercised. The Marshal error path is difficult to trigger with the real implementation and can be reasonably skipped.
  Remediation: Add a test that provides malformed YAML to fakeDispatchSCM.fileContent (e.g., []byte(":::invalid")) and asserts the returned error contains "parsing config".

Previous run (3)

Review

Findings

Low

• [documentation accuracy] docs/guides/dev/behaviour-testing.md:240 — The behaviour contract table row for "Fork PR synchronize re-triggers" states Expected: "Agent runs again per concurrency rules", but the actual test scenario ("Fork PR sync + label dispatches harness") pushes a sync commit then labels the PR, and asserts dispatched exactly 1 time. The test verifies the sync+label sequence dispatches successfully, not that synchronize events independently re-trigger a previously-dispatched harness. The "How tested" column ("Separate scenario with sync + label") is accurate, but the "Expected" column overspecifies what the test actually verifies.

Previous run (4)

Review

Findings

High

• [compilation error] pkg/behaviourtest/steps/dispatch_test.go:95 — The fakeDispatchSCM type is missing 4 methods required by the scm.Driver interface: CreateRepo, EnsureRepoPublic, GetDefaultBranch, and GetBranchRef. Since world.World.SCM is typed as scm.Driver, assigning a *fakeDispatchSCM to w.SCM will fail to compile. The existing fakeURLSCM in url_dispatch_test.go (same package) implements all 20 interface methods including these 4.
  Remediation: Add the 4 missing stub methods to fakeDispatchSCM: CreateRepo(context.Context, string, string, string) error, EnsureRepoPublic(context.Context, string, string) error, GetDefaultBranch(context.Context, string, string) (string, error), and GetBranchRef(context.Context, string, string, string) (string, error). Alternatively, consider reusing fakeURLSCM from the same package instead of duplicating a partial fake.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (5)

Review

Findings

Low

• [undocumented-step] docs/guides/dev/behaviour-testing.md:173 — The documentation references 'disabled harness' as a test step type (seen in the feature file: a disabled custom harness), but this step type is not documented in behaviour-testing.md. The implementation exists in dispatch.go but developers writing new scenarios may not know this capability exists or how to use it.
  Remediation: Add a brief explanation of the disabled custom harness step to the behaviour-testing.md guide, explaining when and why to use disabled harnesses in test scenarios (e.g., for testing kill-switch behavior or CEL exclusion guards).

Previous run (6)

Review

Findings

Low

• [documentation-reference-format] docs/guides/dev/behaviour-testing.md — ADR reference uses unlinked format (ADR 54) instead of the established pattern of linking ADRs with zero-padded 4-digit format. All other ADR references in docs/guides/dev/*.md files use the linked format (e.g., [ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md)).
  Remediation: Change (ADR 54) to ([ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md)) to match the consistent linking pattern.

---

Labels: PR adds behaviour contract tests for fork dispatch (component/e2e) and updates developer documentation (component/docs).

[ifireball]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:24 AM UTC · Completed 5:29 AM UTC
Commit: c5d5b07 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 1 review finding: linked the ADR 54 reference using the established zero-padded format with relative path, matching all other ADR references in the docs/guides/dev/ directory.

Fixed (1):

1. ADR reference uses unlinked format (ADR 54) instead of established linked zero-padded pattern (docs/guides/dev/behaviour-testing.md): Changed (ADR 54) to ([ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md)) to match the consistent ADR linking pattern used throughout docs/guides/dev/ files

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]

🤖 Finished Review · ✅ Success · Started 5:30 AM UTC · Completed 5:42 AM UTC
Commit: 9432941 · View workflow run →

[ifireball]

/fs-fix

Please address the following — keep the fork-pr-killed (disabled harness / enabled: false) and fork-pr-nofork (CEL is_fork exclusion) coverage already in this PR. Those rows are correct and should stay; do not remove, rewrite, or fold them away.

1. Missing real kill-switch contract row

Issue #3732 asks for kill switch active in config on a fork event → empty matrix, exit 0. That means repo/org kill_switch: true (see Dispatch early-return in internal/harnessdispatch/core.go), not per-agent enabled: false.

a disabled custom harness / fork-pr-killed covers agent disablement only. Please add a fork-path scenario that actually engages kill_switch: true and asserts no harness runs / empty matrix. Because a live kill switch blocks all harnesses, this must be its own scenario (do not put kill_switch: true into the existing positive fork-pr-ping scenario). Add a behaviour step for setting the kill switch if one does not exist yet, and update the fork PR behaviour contract table in behaviour-testing.md so “kill switch” and “disabled harness (enabled: false)” are clearly distinct rows/mechanisms.

2. Review finding: document a disabled custom harness

Address the [undocumented-step] finding from the review agent on docs/guides/dev/behaviour-testing.md: briefly document the a disabled custom harness step (what it does — registers the agent with enabled: false — and when to use it for negative dispatch assertions). When documenting it, do not describe it as the kill switch; point readers at the separate kill-switch step/scenario for the global emergency stop.

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:48 AM UTC · Completed 5:58 AM UTC
Commit: 9432941 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (human-triggered)

Addressed both human-instruction findings: (1) added a real kill-switch scenario using kill_switch: true with a new 'the kill switch is active' step and unit tests, (2) documented both 'a disabled custom harness' and 'the kill switch is active' steps in behaviour-testing.md with clear distinction between the two mechanisms. Preserved existing fork-pr-killed and fork-pr-nofork coverage.

Fixed (2):

1. Missing real kill-switch contract row (e2e/behaviour/features/dispatch/fork-dispatch.feature): Added a new 'Fork PR kill switch blocks all harnesses' scenario that engages kill_switch: true via a new 'the kill switch is active' step, separate from the existing disabled-harness coverage. Added the step implementation in dispatch.go (reads config, sets kill_switch: true, commits) and unit tests covering the happy path, GetFileContent error, and CommitFile error.
2. Document the 'a disabled custom harness' step (docs/guides/dev/behaviour-testing.md): Added a 'Dispatch step reference' section documenting both 'a disabled custom harness' (registers harness with enabled: false for single-agent exclusion) and 'the kill switch is active' (sets kill_switch: true for global emergency stop). Updated the contract table to show kill switch and disabled harness as distinct rows with their separate mechanisms and testing patterns. Clarified that the kill switch scenario cannot be consolidated with positive-path scenarios.

Tests: passed

Decision points

• Chose to make the kill switch scenario standalone rather than piggybacking on the existing positive-path scenario (alternatives: Piggyback on existing scenario with kill switch step after positive assertion; rationale: kill_switch: true blocks ALL harnesses globally, so the positive-path fork-pr-ping harness would also be blocked. A separate scenario is the only correct approach, as stated in the human instruction.)
• Used a unique label 'ready-for-fork-killswitch' for the new scenario (alternatives: Reuse 'ready-for-fork-ping' label; rationale: Each scenario should use a unique label to avoid cross-scenario interference in parallel execution. Follows the existing pattern where fork-pr-sync uses 'ready-for-fork-sync'.)

---

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]

🤖 Finished Review · ✅ Success · Started 5:59 AM UTC · Completed 6:14 AM UTC
Commit: 6912790 · View workflow run →

[ifireball]

/fs-fix pull updates from main and fix conflicts, fix review comments

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 6:58 AM UTC · Completed 7:06 AM UTC
Commit: 6912790 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 4 (human-triggered)

Addressed 1 review finding: corrected the synchronize row in the fork PR behaviour contract table to accurately describe what the test verifies.

Fixed (1):

1. documentation accuracy: synchronize re-triggers row overspecifies expected behaviour (docs/guides/dev/behaviour-testing.md): Updated the fork PR behaviour contract table row from 'Fork PR synchronize re-triggers / Agent runs again per concurrency rules' to 'Fork PR synchronize + label dispatches harness / Harness dispatched exactly 1 time; workflow completes' to accurately reflect what the test scenario actually verifies (sync+label sequence dispatches once, not re-triggering or concurrency rules).

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]

🤖 Finished Review · ✅ Success · Started 10:15 AM UTC · Completed 10:30 AM UTC
Commit: 021c38f · View workflow run →

[ifireball]

/fs-fix pull updates from main

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 6:09 AM UTC · Completed 6:15 AM UTC
Commit: 021c38f · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 5 (human-triggered)

Merged origin/main into PR branch as requested. Clean merge with no conflicts. go vet, tests, and build all pass.

Fixed (1):

1. pull updates from main: Merged origin/main (123 commits) into the PR branch. Merge completed cleanly with no conflicts. All affected tests pass and the build compiles successfully.

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]

🤖 Finished Review · ✅ Success · Started 6:15 AM UTC · Completed 6:28 AM UTC
Commit: dbdbb8b · View workflow run →

[ifireball]

/fs-fix

The new kill-switch scenario is polluting other parallel behaviour scenarios.

Bug

the kill switch is active (givenKillSwitchActive) commits kill_switch: true to the enrolled repo's .fullsend/config.yaml and never clears it. Behaviour slots are reused within a suite run, so a later scenario on the same test-repo-N hits bash Route's kill-switch check and fails the whole fullsend.yaml run.

Latest evidence on halfsend-02/test-repo-03 (BT run https://github.com/fullsend-ai/fullsend/actions/runs/30883363287/job/91909245696):

1. Kill-switch scenario commits behaviour: activate kill switch
2. Triage scenario reuses the same slot
3. Triage after-scenario hook fails: Route step Check kill switch → Kill switch is active — all agent dispatch halted

All fork-contract assertions themselves pass; the red is collateral damage from sticky kill-switch state.

Fix

Keep the existing kill-switch / disabled-harness / CEL exclusion coverage. Do not remove those scenarios.

Make kill-switch activation scenario-scoped cleanup:

• After the kill-switch scenario (AfterScenario hook and/or an explicit deactivate step used at the end of that scenario), set kill_switch: false (or omit it) back in config and commit it before the slot can be reused
• Prefer automatic cleanup tied to activating the switch (so every future use is safe), not only a one-off feature-file step that can be forgotten
• Optionally assert in the kill-switch scenario that bash Route / dispatch fails closed as expected, but the required fix is restoring config so sibling scenarios are unaffected

Re-run behaviour and confirm triage (and other non-kill-switch scenarios) no longer fail with an active kill switch left behind.

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 6:38 AM UTC · Completed 6:49 AM UTC
Commit: dbdbb8b · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 6 (human-triggered)

Added automatic kill-switch cleanup to prevent cross-scenario pollution. When givenKillSwitchActive sets kill_switch: true, it marks KillSwitchActivated on the World. CleanupScenario (AfterScenario) deactivates the kill switch before the slot is reused. All 4 changes follow the established DummyOps cleanup pattern.

Fixed (4):

1. kill switch scenario pollutes parallel behaviour scenarios via sticky config state (pkg/behaviourtest/world/world.go): Added KillSwitchActivated bool field to World struct to track when a scenario activates the kill switch
2. givenKillSwitchActive never clears kill_switch: true from config.yaml (pkg/behaviourtest/steps/dispatch.go): Set w.KillSwitchActivated = true after successful activation; added exported DeactivateKillSwitch function that reads config, sets kill_switch: false, and commits
3. no scenario cleanup for kill switch state (pkg/behaviourtest/steps/cleanup.go): Added kill switch cleanup section to CleanupScenario that calls DeactivateKillSwitch when KillSwitchActivated is true, following the established DummyOps cleanup pattern
4. KillSwitchActivated field not reset between scenarios (pkg/behaviourtest/suite/init.go): Reset w.KillSwitchActivated = false in resetScenarioWorld to ensure clean state for each scenario

Tests: passed

Strategy change: Iteration 6 (exceeds threshold 3). Previous iterations addressed other review findings. This is the first time the kill-switch cleanup bug was reported — implemented the automatic cleanup approach the human requested rather than a manual Gherkin step.

Decision points

• Chose World field + CleanupScenario pattern over AfterScenario hook or explicit Gherkin step (alternatives: AfterScenario hook in init.go, Explicit 'the kill switch is deactivated' Gherkin step in feature file; rationale: Follows the established DummyOps cleanup pattern exactly — automatic, cannot be forgotten, and keeps all cleanup in one place. The human instruction preferred automatic cleanup tied to activating the switch.)

---

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]

🤖 Finished Review · ✅ Success · Started 6:50 AM UTC · Completed 7:02 AM UTC
Commit: 186e96c · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:39 AM UTC · Completed 7:58 AM UTC
Commit: 186e96c · View workflow run →

[fullsend-ai-retro]

Retro: PR #5665 — fork dispatch behaviour contract

PR #5665 added kill-switch and CEL is_fork exclusion rows to the fork-dispatch behaviour contract. The code agent created the PR from issue #3732 on Jul 28; it merged Aug 4 after 6 fix iterations and 16 total agent runs (1 triage, 1 code, 7 review, 6 fix, 1 retro).

Timeline highlights

• Jul 28: Code agent opens PR. Review 1 approves with minor ADR formatting nit.
• Aug 3: Human (ifireball) triggers /fs-fix x4 over the day. Most significant intervention: iteration 2 explained that enabled: false (disabled harness) ≠ kill_switch: true (global emergency stop) — the code agent had conflated the two, following the triage agent's inaccurate gherkin examples rather than the issue body's clear contract table.
• Aug 3: Review 3 caught a real compilation error (missing scm.Driver interface stubs) — the only high-severity finding across 7 reviews.
• Aug 4: Human caught a test-isolation bug — the kill-switch scenario committed kill_switch: true to shared config without cleanup, poisoning sibling parallel scenarios. The review agent missed this across reviews 5–7, instead repeating a low-severity ParsePerRepoConfigWriter test coverage gap finding verbatim three times.
• Aug 4: Human approved after fix iteration 6 added automatic cleanup via CleanupScenario.

What went well

• The triage agent ran 5 iterations over 20 days, progressively incorporating blocker resolution and maintainer guidance. Its final output correctly scoped the work to 2 of 4 contract rows.
• The review agent caught a real compilation error (review 3) and several documentation accuracy issues.
• The fix agent reliably executed each /fs-fix instruction, including complex multi-step guidance.

Existing-issue evidence (not filed as new proposals)

• Repeated findings (fullsend#1013, Review agent should deduplicate findings across re-review iterations on the same PR #2959, Review agent: persistent finding cache for cross-iteration deduplication and dismissal enforcement #2816): The ParsePerRepoConfigWriter test gap finding was raised identically in reviews 5, 6, and 7 without change. This adds to the evidence that cross-iteration finding deduplication remains unresolved.
• Code agent not following codebase patterns (agents#288, Label PRs as workarounds when the fix cannot address root cause #464): The code agent added kill_switch: true to shared config without cleanup, despite CleanupScenario with DummyOps being an established pattern in the same package. This is another instance of the agent not aligning to dominant codebase patterns.
• Code agent not cross-referencing issue vs triage (agents#210): The code agent followed the triage agent's gherkin examples literally rather than cross-referencing the issue body's contract table, which clearly distinguished kill switch from disabled harness.

Proposals filed

• Review correctness sub-agent should detect missing cleanup for test-shared-state mutations (in fullsend-ai/agents)
• Triage agent should validate domain-mechanism accuracy when producing code examples (in fullsend-ai/agents)