← Back to all posts

Reviewing an Agent PR Sweep: 27 Pull Requests, Four Bug Classes, and the Line a Merge Dropped

Published:
candyrushagent-runevidenceunity

On November 18, 2025, 26 pull requests were opened against CandyRush in four minutes and nineteen seconds (#117 to #142, 08:23:42 to 08:28:01 UTC), and a 27th (#143) followed on November 20. Each came from an AI agent on its own claude/* branch, and each did one thing: fix a pool bug, replace a lookup, unsubscribe event handlers, tighten a DI registration, fix typos, add tests.

I directed the sweep and gated what landed. The outcome, all of it from the GitHub API and git log:

Count
Pull requests opened27
Merged as written19 (3 on Nov 20, 16 on Nov 21 within two minutes)
Closed for merge conflicts, then recombined into #1448
Follow-up commits on Nov 22 to make the combined result compile and run21
Runtime regressions found after merge1 (enemy waves stopped spawning)

The sweep fixed four real classes of bug. The more useful lesson is about the gate: every diff was plausible on its own, and the problems lived in the combination.

Four classes of bug

A pool that built one extra object (#126)

The object pool's constructors looped while i was less than or equal to startingSize, so a pool configured for 50 built 51. The fix is one character in two places:

-            for (int i = 0; i <= startingSize; i++)
+            for (int i = 0; i < startingSize; i++)

The same PR cached GetComponent results per pooled object, and added 293 lines of EditMode tests that assert exact entity counts. The PR description claims "50-100x faster" component lookups after caching. Nobody measured that; it is the agent's estimate. Both the fix and the cache are still in the pool, which now lives in my shared UTools package.

A lookup labelled O(n³) (#131)

HasEvolution answers "does this ability evolve, and what does it pair with?" for the level-up cards. The old version walked every ability, then each evolution's requirements, then the requirements again. The PR replaced it with a dictionary built once at init.

The title says O(n³) to O(1). Reading it again, that overstates it: every evolution has two requirements, so the inner loops are constant and the old code was linear in the number of abilities, with an early return. The dictionary is still the better shape. The "expected frame time improvement" in the PR body was never measured.

The diff also changed a behaviour quietly. The old loop returned the first match; the dictionary keeps the last write. If one ability ever feeds two evolutions, the answer changes. The PR added no test for either case.

Handlers that outlived their windows (#123)

Several UI windows subscribed to events and never unsubscribed, so destroyed windows stayed reachable from the publisher and could receive callbacks after destruction. The fix added OnDestroy cleanup to seven UI behaviours (GameScreen, Chest, Lobby, Settings, Upgrades, Characters, and the boss health bar), plus an editor tool that scanned scripts for += without a matching -=.

The PR body says "1000+ dangling references after 10 stages, causing memory growth until crash." That is arithmetic in a description, not a measurement, and I would not repeat it as fact. The cleanup is correct regardless. The scanner itself was deleted in a September 2026 tooling cut (#949).

A DI factory that returned null (#137)

The composition root registered IUpgradesManager with a factory that logged a warning and returned null when its configuration asset was missing. Every consumer then failed later with a NullReferenceException far from the cause. The fix inverts the check and throws at startup:

if (upgradesConfig?.upgradesDatabase == null)
{
    throw new System.InvalidOperationException(
        "[RootLifetimeScope] UpgradesConfiguration or database not found ...");
}
return new UpgradesService(saveManager, upgradesConfig.upgradesDatabase);

Six lines added, five removed. A DI container should never hand out null for a registered service; failing at composition time names the real problem.

The rest of the sweep was hygiene: layer constants instead of magic numbers (#117), a platform Strategy pattern (#118), editor-script guards (#127), interface coverage tests (#129, 336 lines), PlayMode DI integration tests (#134, 851 lines), null-operator modernization (#133), class-name typo fixes (#138), validation attributes across 43 ScriptableObject files (#141), XML documentation (#142), and an .editorconfig (#143).

What the gate caught

CI could not tell good from bad

The Unity test job failed on every pull request I checked in this batch, including #137's six-line fix. A check that is red for everything does not gate anything. The real gate was the editor: merge, compile, play.

Eight PRs that did not survive contact

After the first 16 merged, 8 of the remaining PRs no longer applied. Each was closed with a note that the work was captured for later. The same afternoon they were cherry-picked into one consolidating PR, #144 (+1,280/-948, 76 files), with the conflicts resolved by hand. Its description lists the breaking changes it carried: a public API rename (Aquired to Acquired, 103 occurrences), and lifetime scopes that now needed Inspector-assigned assets instead of Resources.Load.

Plausible parts, a set that did not compile

On November 22 an agent session worked through what the combination broke. The commits read like a catalogue of interactions no single PR could have shown:

  • #126 added using System; to the pool, which made Object.Instantiate and Object.Destroy ambiguous between System.Object and UnityEngine.Object.
  • #121 introduced an AnimationTriggers constants class, which collided with UnityEngine.UI.AnimationTriggers in the chest window.
  • #120 turned WaveData fields into properties; the spawner and two test files still used the fields.
  • A boss behaviour ended up with duplicate fields and a duplicate Construct method.

Twenty-one commits in all, including removing the [Obsolete] code that #132 had marked, finishing the move off Resources.Load, and clearing every compile warning.

The line a merge dropped

The one that mattered most compiled cleanly.

#122 replaced a service locator in the Timeline playable behaviours with VContainer injection, and registered the new ITimelineBehaviorInjector in GameLifetimeScope. When #144's conflicts were resolved by hand, that one registration line was lost. I traced it through the history:

2273228d3  #122 merged           GameLifetimeScope registers ITimelineBehaviorInjector: yes
bb3128679  #144 merged           registration: no
c142d6d7e  Resources migration   registration: no
707c08c18  fix commit (Nov 22)   registration: yes

Nothing failed at compile time. At runtime the wave behaviours had no dependencies. The commit that restored the line records the symptoms: Timeline behaviours failing injection and no enemies spawning. A compiler cannot see a missing registration; running the game can. The fix was the one line, restored.

What I took from it

Small single-concern PRs are easy to review and hard to combine. Each of these was readable in a minute. The cost moved to the merge, where nobody reads the diff with the same care. A hand-resolved consolidation is a new change and deserves the same review as the PRs it replaces.

Claims in PR bodies are hypotheses. "50-100x", "O(n³)", "1000+ dangling references": each was written by the author of the change, and none was measured. I now treat an unmeasured number in a description as something to verify or delete.

A red gate is no gate. With CI failing on every PR, the only signal was compiling and playing. Getting the test suite green and fast later became its own project; see the pipeline rework.

Composition roots need a runtime check. A missing registration compiles. In this sweep the gap surfaced as "no enemies", and that is the kind of failure a container validation test or a smoke test should own instead of a person.

Fail fast at the root. #137 is the pattern worth keeping: a factory that throws at startup with the path of the missing asset is worth more than one that logs and returns null.