Oh wait, I almost snuck something past you, but I think it's correct to change this? Previously "paused" was considered a status that would indicate a build was "completed". This doesn't make sense semantically.

I see where it will screw this up:

public function canPauseBuild() {
  if ($this->isAutobuild()) {
    return false;
  }

  return !$this->isComplete() &&
         !$this->isPaused() &&
         !$this->isPausing();
}

I did miss that, but agree that "Paused" is not correct semantically. It looks like most of the code already assumes this, and this probably fixes some bugs.

I don't immediately see any behavioral changes which don't seem like bug fixes -- that snippet you cited should be fine/unchanged? The other cases look like they're more correct after the change (e.g., allow you to abort a paused build, don't start following build steps if a build is paused).

I believe this will allow someone to issue a pause command to an already paused build.

Oh no, I confused myself terribly.

It is fine as is.

This one did seem possibly suspicious in BuildEngine:

if ($build->isPausing() && !$build->isComplete()) {
  $build->setBuildStatus(HarbormasterBuild::STATUS_PAUSED);
  $build->save();
}

But I think that one's moot (pausing while paused is fine/safe) and it's impossible to get into a state where it matters anyway (pausing while already paused isn't normally permitted) and it's deep in the internals (so users won't see it / get confused by it doing technically-correct-but-unintuitive things).