test(#5987): add behaviour coverage for code applier branch handling

[waynesun09]

Summary

Adds e2e/behaviour scenarios that assert the code applier's branch handling end to end against live GitHub, plus the primitives the suite was missing to express them (issue #5987). Until now every scenario stopped at dispatch/labeling; nothing drove a run through the post-scripts to a real push and asserted the resulting branch state.

The scenarios are mechanically gated: they carry @requires:capability:applier-branch-namespace and are skipped unless the runner declares that capability via BEHAVIOUR_CAPABILITIES, because they assert applier behavior that ships with the upcoming agents-side branch-namespace enforcement. CI stays green until the pinned agents release includes it; then the capability gets enabled.

Related Issue

Refs #5987 (delivers the primitives and scenarios A/C; see "Deviation" below for scenario B)

Changes

• checkout_branch dummy-runtime op (internal/runtime/dummy.go): takes a single regex-validated branch name. Probes the remote with git ls-remote --exit-code --heads — a missing ref (rc=2) falls back to branching off HEAD, any other probe failure fails the op instead of silently degrading. When the ref exists it's fetched via the same refs/heads/ scope (so a same-named tag can never be resolved instead — regression-tested) and the branch is based on FETCH_HEAD. The op then records one marker commit, so the applier post-script has content to push and a wrongful push visibly moves the target branch tip. Covered by mock-exec unit tests plus real-shell/git integration tests for the existing-ref, missing-ref, broken-remote, and tag-collision paths.
• Branch assertion steps (pkg/behaviourtest/steps/branch.go):
  • an open pull request on branch "<name>" / a remote branch "<name>" seeded with a commit — deterministic Given fixtures that tolerate pool-repo debris; PR base resolved via GetDefaultBranch.
  • the tip of branch "<name>" is recorded / branch "<name>" is unchanged — capture GetBranchRef before the run, re-check after.
  • the pull request head branch matches "<pattern>" — exactly one open PR head must match the anchored Go-regex pattern; supports an <issue> placeholder (also honored in checkout_branch table args — scoped to that op only).
  • the harness "<agent>" workflow fails reporting "<text>" — waits for a terminally-failed harness run, then asserts the post-script failure comment on the scenario PR; context-cancellable end to end.
• Capability gating: new @requires:capability:<name> compatibility tag recognized by SkipErrorForTagNames, wired to the comma-separated BEHAVIOUR_CAPABILITIES env var. A malformed tag (empty capability name) is a hard error, not a silently-permanent skip.
• Driver surface: scm.Driver gains ListOpenChangeProposals/ListComments; ci.Driver gains WaitForFailedHarnessAgent using artifact-first detection (fullsend-<agent> uploads with if: always(), resolving standard stage jobs like Fix/Code too, not just custom-harness matrix jobs), with a job-name fallback that scans every completed run — not just ones already known to have failed — so a successful job still fails fast through that path too. Surfaced job-listing errors on timeout, unit tests for both detection paths. All three methods documented in the behaviour-testing "API changes" section.
• Feature file e2e/behaviour/features/code/branch-namespace.feature: scenario A (foreign branch renamed into the issue namespace, decoy tip unchanged; decoy uses issue number 990000099 so it can never collide with a real issue's anchored pattern) and scenario C (conforming agent/<issue>-<slug> branch pushed with no rename).
• Cleanup: scenario-created branches/PRs are tracked and removed in CleanupScenario, plus a namespace sweep — gated on the scenario's issue number alone, not on whether a branch step happened to run — that reclaims applier-created agent/<issue>-* PRs even when the scenario fails before its head-match assertion. Deduplicated against PRs the assertion step already tracked, so a passing run doesn't double-close/delete.

Deviation from the issue plan

Issue #5987's scenario B (fix run refuses to push from a non-matching branch) is not expressible in this suite today: the fix stage's only dispatch route is a changes_requested review submitted by the org review bot (reusable-dispatch.yml), and suite-posted comments are bot-authored, which both the per-repo shim and the /fs-fix route drop. The failure-assertion step and WaitForFailedHarnessAgent are landed and unit-tested as ready primitives, the limitation is documented in the feature file and suite docs, and the fix-side refusal stays covered by script-level tests in the agents repo until a suite-reachable fix trigger exists.

Testing

• make lint passes (staged first)
• go test ./internal/runtime/ ./pkg/behaviourtest/... — unit tests for the op (valid/invalid names, exec failure, real-shell integration for both basing paths, the fail-closed remote-error path, and the tag-collision regression), every new step, the capability gate (including the malformed-tag error path), the cleanup sweep (including dedup and no-branch-step gating), and both WaitForFailedHarnessAgent detection paths
• Live make behaviour-test with BEHAVIOUR_CAPABILITIES=applier-branch-namespace — pending the agents-side branch-namespace enforcement in the pinned agents release; without the capability the scenarios skip (verified by the gating unit tests)

The two pre-existing TestDummyRuntime_Bootstrap/TestDummyRuntime_ClearIterationArtifacts failures on machines with a live local sandbox are unrelated (also fail on clean main).

This PR went through two rounds of a 3-agent review squad (Claude, Grok). Round 1 found the fix-stage scenario was undispatchable, the failure waiter only matched custom-harness job names, tip-unchanged assertions were vacuous, and the branch-checkout fetch failed open on transient errors — all fixed above. Round 2 caught a cleanup double-close/delete regression, a fragile sweep gate, a fetch/tag-collision edge case, and a fast-fail regression in the job-name fallback — also fixed and covered by new regression tests.

Checklist

• PR title follows Conventional Commits
• Commits are signed off (DCO)
• I wrote this contribution myself and can explain all changes in it

[qodo-code-review]

PR Summary by Qodo

Add e2e behaviour coverage for code/fix applier branch handling

🧪 Tests ✨ Enhancement 📝 Documentation 🕐 40+ Minutes

AI Description

• Add live GitHub e2e scenarios asserting applier branch namespace and no-op branch movement.
• Extend behaviour suite with branch/PR assertions, failed-harness waiting, and cleanup of created
 refs.
• Add a narrowly-scoped dummy runtime git checkout op to drive post-script push paths safely.

Diagram

graph TD
A["Gherkin scenarios"] --> B["Branch steps"] --> C("World state") --> D["Dummy runtime"]
C --> E["SCM driver"] --> G[["GitHub API"]]
C --> F["CI driver"] --> G
subgraph Legend
direction LR
_step["Step defs"] ~~~ _state("Scenario state") ~~~ _ext[["External system"]]
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add a generic shell/git op to dummy runtime

• ➕ More flexible for future scenarios (any git plumbing).
• ➕ Less need for adding new dedicated ops over time.
• ➖ Much higher security risk (option injection, arbitrary commands).
• ➖ Harder to keep deterministic across environments/sandboxes.
• ➖ Expands test framework surface in ways reviewers may resist.

2. Assert branch outcomes via workflow artifacts only

• ➕ Avoids extra SCM API calls (ListOpenPRs/ListComments) and new cleanup paths.
• ➕ May be faster/less flaky than live GitHub branch polling.
• ➖ Weaker signal: artifacts can drift from real GitHub state (especially branch renames/push outcomes).
• ➖ Cannot easily assert "other branch tip unchanged" without remote refs.

Recommendation: Keep the PR’s approach: narrow, validated checkout_branch plus SCM/CI-backed assertions. It provides true end-to-end coverage of push/rename behaviour while deliberately minimizing the dummy runtime’s power surface (regex validation + no '..' + no leading dash) and adding explicit cleanup to keep pooled repos stable.

Files changed (23) +837 / -3

Enhancement (8) +120 / -1

dummy.goAdd checkout_branch dummy runtime operation with validation +32/-0

Add checkout_branch dummy runtime operation with validation

• Introduces a regex-restricted branch name pattern and a new checkout_branch op. The op fetches origin/<branch> and checks out a local branch based on FETCH_HEAD when present, otherwise branches from current HEAD; rejects traversal and option injection inputs.

internal/runtime/dummy.go

driver.goExtend CI driver interface with failed-harness wait +4/-0

Extend CI driver interface with failed-harness wait

• Adds WaitForFailedHarnessAgent to allow scenarios to assert that an agent’s harness job terminates in failure and to fail fast if it succeeds.

pkg/behaviourtest/drivers/ci/driver.go

githubactions.goImplement WaitForFailedHarnessAgent for GitHub Actions +39/-0

Implement WaitForFailedHarnessAgent for GitHub Actions

• Adds polling logic to find a completed harness run containing the agent job and return only on terminal failures. Explicitly errors if the job concludes successfully; treats cancelled/skipped as noise and continues polling.

pkg/behaviourtest/drivers/ci/githubactions/githubactions.go

driver.goExtend SCM driver interface with PR/comment listing +5/-0

Extend SCM driver interface with PR/comment listing

• Adds ListOpenChangeProposals (for PR head branch inspection) and ListComments (for post-script failure comment assertions), with interface documentation.

pkg/behaviourtest/drivers/scm/driver.go

github.goImplement new SCM listing methods via forge client +8/-0

Implement new SCM listing methods via forge client

• Implements ListOpenChangeProposals and ListComments as thin passthroughs to the forge client’s PR and issue-comment list APIs.

pkg/behaviourtest/drivers/scm/github/github.go

cleanup.goClean up branches and PRs created by branch scenarios +16/-0

Clean up branches and PRs created by branch scenarios

• Extends scenario cleanup to close created PRs before deleting their head branches, avoiding confusing GitHub auto-close events. Ignores not-found errors for idempotency on pooled repos.

pkg/behaviourtest/steps/cleanup.go

dummy_agent.goSupport <issue> placeholder expansion in dummy agent table args +5/-1

Support <issue> placeholder expansion in dummy agent table args

• Expands <issue> in dummy-agent operation args at parse time, enabling scripted ops (including checkout_branch) to reference the scenario’s issue number deterministically.

pkg/behaviourtest/steps/dummy_agent.go

world.goAdd per-scenario branch tracking fields to World +11/-0

Add per-scenario branch tracking fields to World

• Adds RecordedBranchSHAs, CreatedBranches, and CreatedPRNumbers with documentation to support branch assertion steps and deterministic cleanup on pooled repositories.

pkg/behaviourtest/world/world.go

Tests (10) +436 / -0

branch-namespace.featureAdd branch-namespace end-to-end scenarios +55/-0

Add branch-namespace end-to-end scenarios

• Introduces three live GitHub scenarios tagged @branch-namespace covering code-run rename into agent/<issue>-* namespace, fix-run refuse-to-push on head mismatch, and no-rename for already-conforming namespaced branches.

e2e/behaviour/features/code/branch-namespace.feature

implemented.jsonAdd schema-valid code stage fixture +4/-0

Add schema-valid code stage fixture

• Adds a minimal code-result fixture targeting main for scripted code runs used by the new scenarios.

e2e/behaviour/fixtures/code/implemented.json

scripted.jsonAdd schema-valid fix stage fixture +14/-0

Add schema-valid fix stage fixture

• Adds a minimal fix-result fixture with a single scripted fix action and summary metadata for end-to-end fix runs.

e2e/behaviour/fixtures/fix/scripted.json

dummy_test.goUnit test checkout_branch behaviour and input hardening +81/-0

Unit test checkout_branch behaviour and input hardening

• Adds tests for successful command construction, empty args, invalid branch names, non-zero git exit codes, and exec errors.

internal/runtime/dummy_test.go

branch_test.goUnit test new branch step primitives +246/-0

Unit test new branch step primitives

• Adds fakes for SCM/CI interfaces and tests placeholder expansion, seeded branch creation/reset, PR opening, branch tip stability assertions, anchored/ambiguous head matching, PR commenting, and failure comment polling.

pkg/behaviourtest/steps/branch_test.go

cleanup_test.goUpdate cleanup SCM fake for new interface methods +8/-0

Update cleanup SCM fake for new interface methods

• Adds stub implementations for ListOpenChangeProposals and ListComments to satisfy the expanded SCM driver interface in tests.

pkg/behaviourtest/steps/cleanup_test.go

dispatch_test.goUpdate dispatch SCM fake for new interface methods +6/-0

Update dispatch SCM fake for new interface methods

• Adds no-op stubs for ListOpenChangeProposals and ListComments on the dispatch test fake SCM driver.

pkg/behaviourtest/steps/dispatch_test.go

fork_test.goUpdate fork SCM fake for new interface methods +8/-0

Update fork SCM fake for new interface methods

• Adds no-op stubs for ListOpenChangeProposals and ListComments on the fork test fake SCM driver.

pkg/behaviourtest/steps/fork_test.go

url_dispatch_test.goUpdate URL-dispatch SCM fake for new interface methods +8/-0

Update URL-dispatch SCM fake for new interface methods

• Adds no-op stubs for ListOpenChangeProposals and ListComments on the URL dispatch test fake SCM driver.

pkg/behaviourtest/steps/url_dispatch_test.go

init_test.goUpdate panicking SCM stub for new interface methods +6/-0

Update panicking SCM stub for new interface methods

• Adds stubs for ListOpenChangeProposals and ListComments to keep suite init tests compiling against the expanded SCM interface.

pkg/behaviourtest/suite/init_test.go

Documentation (2) +27 / -2

behaviour-drivers.mdDocument new CI driver failure-wait hook +1/-1

Document new CI driver failure-wait hook

• Updates CI driver implementation checklist to include WaitForFailedHarnessAgent for scenarios asserting fail-closed harness outcomes.

docs/guides/dev/behaviour-drivers.md

behaviour-testing.mdDocument checkout_branch op and branch assertion steps +26/-1

Document checkout_branch op and branch assertion steps

• Adds checkout_branch to the dummy-agent op table and explains its fetch/checkout semantics and <issue> placeholder expansion. Documents new SCM-level branch/head/failure-comment steps and adds new fixtures to the fixture catalog.

docs/guides/dev/behaviour-testing.md

Other (3) +254 / -0

branch.goAdd branch/PR head assertion and failure-comment steps +250/-0

Add branch/PR head assertion and failure-comment steps

• Adds step definitions to seed remote branches, open PRs on branches, record and re-check branch tips, assert exactly one open PR head matches an anchored pattern, post PR comments, and wait for failed harness jobs plus verify the failure comment text. Introduces <issue> placeholder expansion and branch reset logic for pooled repos.

pkg/behaviourtest/steps/branch.go

registry.goRegister new branch step suite +1/-0

Register new branch step suite

• Adds registerBranchSteps to the global godog step registry so new steps are available to feature files.

pkg/behaviourtest/steps/registry.go

init.goReset branch-scenario state between runs +3/-0

Reset branch-scenario state between runs

• Ensures per-scenario branch tracking (recorded SHAs, created branches, created PR numbers) is reset to nil during world initialization to avoid leakage across scenarios.

pkg/behaviourtest/suite/init.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. behaviour-drivers.md not in admin/user ✗ Dismissed 📜 Skill insight ⌂ Architecture

Description

The modified guide files are located under docs/guides/dev/, but guides must live under either
docs/guides/admin/ or docs/guides/user/. Additionally, docs/guides/dev/behaviour-drivers.md
contains procedural numbered steps without a preceding Prerequisites section, violating the guide
format requirements.

Code

docs/guides/dev/behaviour-drivers.md[50]

+1. Implement `ci.Driver` — `WaitForWorkflow`, `FindCompletedWorkflowRun`, `AssertNoWorkflow`, `GetRunLogs`, `DownloadArtifacts`, `DownloadNamedArtifactFromRun`, `DownloadNamedArtifactAfter`, `WaitForHarnessAgent`, `WaitForFailedHarnessAgent`, `AssertNoHarnessAgentArtifact`, `CountHarnessDispatches`.

Relevance

●● Moderate

Guide relocation from docs/guides/dev previously rejected, but adding Prerequisites sections is
often accepted.

PR-#4901
PR-#2663

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance checklist requires that any guide under docs/guides/ be placed only in the admin/
or user/ subdirectories (not dev/) and that a clearly labeled Prerequisites section appear
before numbered procedural steps. This PR modifies docs/guides/dev/behaviour-drivers.md and
docs/guides/dev/behaviour-testing.md, both of which remain in the non-compliant dev/ directory,
and behaviour-drivers.md specifically includes a numbered step list without any prerequisites
section above it, demonstrating both directory and formatting non-compliance.

docs/guides/dev/behaviour-drivers.md[39-52]
docs/guides/dev/behaviour-testing.md[1-60]
Skill: writing-user-docs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Guide documentation files were modified under `docs/guides/dev/`, but guides must be placed under `docs/guides/admin/` or `docs/guides/user/`; additionally, `docs/guides/dev/behaviour-drivers.md` includes numbered procedural steps without a preceding `Prerequisites` section.

## Issue Context
The compliance checklist enforces a strict directory layout for guides (to keep audience targeting consistent) and requires that any procedural, numbered steps be preceded by a clearly labeled `Prerequisites` section.

## Fix Focus Areas
- docs/guides/dev/behaviour-drivers.md[1-60]
- docs/guides/dev/behaviour-testing.md[1-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Fetch failure silently falls back ✓ Resolved 🐞 Bug ☼ Reliability

Description

checkout_branch treats any git fetch origin <branch> failure as “remote ref missing” and
silently checks out a new branch from current HEAD, which can make branch-handling scenarios run
against the wrong commit and hide real auth/network failures.

Code

internal/runtime/dummy.go[R277-280]

+		quoted := shellQuote(name)
+		cmd := fmt.Sprintf(
+			"cd %s && if git fetch origin %s 2>/dev/null; then git checkout -B %s FETCH_HEAD; else git checkout -B %s; fi",
+			shellQuote(repoDir), quoted, quoted, quoted)

Relevance

●● Moderate

Behavioral change to branch checkout semantics; no close historical precedent for
accepting/rejecting stricter fetch error handling.

PR-#1982

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation redirects fetch stderr away and uses a shell if that falls back on *any*
non-zero fetch exit code; the unit test asserts this exact fetch/else checkout shape, confirming the
fail-open behavior is intentional in the current code.

internal/runtime/dummy.go[264-287]
internal/runtime/dummy_test.go[526-544]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `checkout_branch` dummy runtime op currently runs:

- `if git fetch origin <branch> ...; then checkout from FETCH_HEAD; else checkout from HEAD; fi`

This treats **all** fetch failures (auth/network/repo state) the same as “remote branch does not exist”, which can produce a successful checkout off the wrong commit and hide the true failure reason.

### Issue Context
This op is used by the new end-to-end branch namespace scenarios, so failing open here can cause false positives or confusing downstream failures.

### Fix Focus Areas
- internal/runtime/dummy.go[264-287]
- internal/runtime/dummy_test.go[526-544]

### Implementation notes
- Preserve stderr from `git fetch` (do not redirect to `/dev/null`).
- Only fall back to `git checkout -B <branch>` from `HEAD` when the failure is specifically “remote ref not found” (e.g., stderr contains `couldn't find remote ref`), otherwise return an error.
- Update/extend unit tests to cover:
 - missing remote ref -> fallback path
 - non-ref fetch failure -> op returns error containing fetch stderr

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Harness polling drops API errors ✓ Resolved 🐞 Bug ◔ Observability

Description

WaitForFailedHarnessAgent discards errors from runHasAgentJob (GitHub job listing), so
persistent API failures can degrade into timeouts with misleading diagnostics rather than surfacing
the underlying error.

Code

pkg/behaviourtest/drivers/ci/githubactions/githubactions.go[R584-586]

+			hasJob, conclusion, err := d.runHasAgentJob(ctx, owner, repo, run.ID, agent)
+			if err != nil || !hasJob {
+				continue

Relevance

● Weak

Team previously rejected surfacing/propagating CI polling API errors instead of swallowing them in
githubactions driver loops.

PR-#5732

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

runHasAgentJob can return an error when listing workflow run jobs, and the new failed-harness
polling loop explicitly continues when err != nil, losing that error context and risking
misleading timeouts.

pkg/behaviourtest/drivers/ci/githubactions/githubactions.go[565-602]
pkg/behaviourtest/drivers/ci/githubactions/githubactions.go[494-508]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`WaitForFailedHarnessAgent` calls `runHasAgentJob` and does:

- `if err != nil || !hasJob { continue }`

This drops API errors (including non-transient ones), which can turn real failures (permissions, rate limits, API outages) into a generic polling timeout.

### Issue Context
This method was added specifically to assert fail-closed paths; when it fails, good diagnostics are important to keep the new e2e suite debuggable.

### Fix Focus Areas
- pkg/behaviourtest/drivers/ci/githubactions/githubactions.go[565-602]
- pkg/behaviourtest/drivers/ci/githubactions/githubactions.go[494-508]

### Implementation notes
One of:
- Return immediately on `runHasAgentJob` error (preferred for non-transient errors), OR
- Track `lastErr` and, on timeout, return an error that includes the last job-listing error (and optionally the run ID) so the root cause is not lost.

Add/adjust unit tests if this driver has coverage for polling error paths.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[rh-hemartin]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:52 PM UTC · Completed 1:05 PM UTC

Commit: ee5a2ba · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [missing-documentation] docs/runtimes.md:141 — The new checkout_branch operation added to the dummy runtime is not documented in the "Dummy runtime operations" table at docs/runtimes.md. This table currently lists assert_env, assert_file, and assert_json but omits the new checkout_branch op. The primary documentation in behaviour-testing.md (updated in this PR) already covers checkout_branch comprehensively, but the secondary reference table in docs/runtimes.md has a gap.
  Remediation: Add a row to the table at line 141: | checkout_branch | branch_name | Check out a git branch (fetches from remote if exists, creates based on HEAD otherwise); adds a marker commit |

Low

• [scope-creep] pkg/behaviourtest/suite/init.go:129 — The PR adds a general capability-gating mechanism (@requires:capability:<name>) for the behaviour test suite, which was not explicitly mentioned in the authorizing issue test: add behaviour coverage for code/fix applier branch handling #5987. The mechanism is pragmatically necessary to keep CI green until the agents release ships and follows the existing tag-based skip pattern (skip:gitlab, requires:per-repo). The implementation is minimal and well-tested.
  Remediation: Update issue test: add behaviour coverage for code/fix applier branch handling #5987 to explicitly authorize the capability-gating infrastructure, or file a follow-up issue documenting it as a general suite feature.

---

Labels: PR adds behaviour test coverage for code applier branch handling (e2e/behaviour/, pkg/behaviourtest/)

Previous run

Looks good to me

[rh-hemartin]

Looks good to me.

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 1:34 PM UTC · Ended 1:49 PM UTC

Commit: c8f5177 · View workflow run →

[github-actions]

Site preview

Preview: https://542c0109-site.fullsend-ai.workers.dev

Commit: c8f51773119ae05fb780ad8e0e14833ca8c7b227

[codecov]

Codecov Report

❌ Patch coverage is 57.77778% with 114 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/behaviourtest/steps/branch.go	53.59%	48 Missing and 23 partials ⚠️
pkg/behaviourtest/drivers/env/env.go	0.00%	15 Missing ⚠️
...iourtest/drivers/ci/githubactions/githubactions.go	70.73%	9 Missing and 3 partials ⚠️
pkg/behaviourtest/steps/cleanup.go	58.62%	6 Missing and 6 partials ⚠️
pkg/behaviourtest/drivers/scm/github/github.go	0.00%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:34 PM UTC · Completed 1:49 PM UTC

Commit: c8f5177 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:42 PM UTC · Completed 2:57 PM UTC

Commit: c8f5177 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5992 — behaviour coverage for code applier branch handling

Workflow outcome: Smooth. Human-authored PR, two review runs, approved and merged with no rework required.

Timeline

1. Aug 6 17:59 UTC — waynesun09 opens PR #5992 (test-5987-branch-namespace → main): +1426/−5 across 24 files adding e2e behaviour scenarios for code applier branch handling. The PR had already been through two rounds of external 3-agent review (Claude + Grok).
2. Aug 6 18:06 UTC — qodo-code-review bot posts 3 findings. Finding Add problem areas: Tekton pipeline review, migration path, multi-tenancy #2 (fetch failure silent fallback) was substantive but had already been fixed in the reviewed SHA.
3. Aug 7 ~12:52 UTC — rh-hemartin triggers /fs-review.
4. Aug 7 12:52–13:05 UTC — First review run (SHA ee5a2ba): 5 sub-agents dispatched. Style-conventions (Sonnet) produced 5 findings; challenger exonerated all 5 with evidence. Orchestrator agreed all were false positives. Verdict: APPROVE. Cost: $6.53.
5. Aug 7 13:07 UTC — rh-hemartin approves.
6. Aug 7 13:32 UTC — Author pushes updated commit c8f5177.
7. Aug 7 13:33–13:49 UTC — Second review run (SHA c8f5177): auto-triggered on push. 4 sub-agents dispatched (security skipped — reasonable for test infrastructure). Found 2 findings: missing-documentation (medium, runtimes.md table omits checkout_branch) and scope-creep (low, capability gating not in issue test: add behaviour coverage for code/fix applier branch handling #5987). Style produced 3 findings, 2 removed by challenger. Verdict: COMMENT. Applied requires-manual-review. Cost: $6.17.
8. Aug 7 14:40 UTC — Author merges.

What went well

• Review quality: The first review correctly approved a well-constructed PR. The second review's missing-documentation finding about the runtimes.md table was a genuine gap — the external review rounds and qodo bot missed it.
• Challenger effectiveness: Correctly exonerated 7 of 8 style-conventions findings across both reviews as false positives, with evidence-based reasoning verified by the orchestrator.
• Delta handling: The second review properly detected the prior approved review and applied appropriate re-review scoping.
• Cost: $12.70 total for two reviews of a complex 24-file PR is reasonable.

Evidence for existing issues (no new proposals needed)

• agents#675 (challenger empty-result fallback): The first review hit this exactly — all 5 pre-challenger findings were correctly exonerated, producing empty adjudicated findings. The orchestrator noted the tension with step 6d (which treats empty adjudicated = challenger failure) and overrode it after independent verification. This is another data point confirming the fallback heuristic is counterproductive when the challenger provides valid evidence.
• agents#688 (self-acknowledged-justified findings posted): The second review's scope-creep finding on capability gating was downgraded from medium to low by the challenger, which acknowledged it was "pragmatically necessary" infrastructure. Despite this self-acknowledgment, the finding was still posted. This adds evidence for the pattern described in agents#688.
• Style sub-agent false positive rate: 7/8 (87.5%) of style-conventions findings across both reviews were false positives about Go comment conventions and code organization. Partially covered by agents#300 and agents#493, though neither specifically addresses Go convention awareness. Single-PR data point — insufficient to generalize.

Why no proposals

The workflow functioned well. The two improvement opportunities I identified (challenger fallback, self-justified findings) are already tracked in open issues with clear problem statements. The style sub-agent false positive pattern is worth monitoring but is a single data point insufficient for a concrete proposal.