From 17b097621ecc5aba7b316794afafb3ea9ef916c9 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 21 Aug 2026 17:42:49 +1200 Subject: [PATCH 01/10] (feat): bump the downstream base image after a release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A base release was only half the job: appwrite/appwrite pins the image by exact version, so every release left the consumer behind until someone edited the Dockerfile by hand. Carry the release through — open the bump, wait for its checks, merge it, and tag the merge commit. The tag reads APP_VERSION_STABLE from the downstream constants rather than anything in this repo, and takes the next unused sub-version for that application version, matching the cl-{version}-{n} tags already in use. Merging bypasses the downstream review requirement because that branch requires an approving review and GitHub forbids approving your own pull request. It does not bypass the checks: a failing check aborts before the merge is attempted. Co-Authored-By: Claude Opus 5 --- .github/scripts/bin/downstream.php | 48 +++ .../scripts/src/Downstream/Application.php | 96 ++++++ .github/scripts/src/Downstream/Bump.php | 20 ++ .github/scripts/src/Downstream/Checks.php | 46 +++ .github/scripts/src/Downstream/Constants.php | 32 ++ .github/scripts/src/Downstream/Dockerfile.php | 53 ++++ .github/scripts/src/Downstream/Exception.php | 11 + .../scripts/src/Downstream/Orchestrator.php | 108 +++++++ .github/scripts/src/Downstream/Pull.php | 15 + .github/scripts/src/Downstream/Release.php | 62 ++++ .github/scripts/src/Downstream/Repository.php | 41 +++ .../src/Downstream/Repository/GitHub.php | 285 ++++++++++++++++++ .github/scripts/src/Downstream/Version.php | 15 + .../tests/Unit/Downstream/BumpTest.php | 73 +++++ .../tests/Unit/Downstream/ChecksTest.php | 68 +++++ .../scripts/tests/Unit/Downstream/Fake.php | 115 +++++++ .../Unit/Downstream/OrchestratorTest.php | 137 +++++++++ .../tests/Unit/Downstream/ReleaseTest.php | 92 ++++++ .github/workflows/dependencies.yml | 37 +++ CHANGES.md | 4 + 20 files changed, 1358 insertions(+) create mode 100755 .github/scripts/bin/downstream.php create mode 100644 .github/scripts/src/Downstream/Application.php create mode 100644 .github/scripts/src/Downstream/Bump.php create mode 100644 .github/scripts/src/Downstream/Checks.php create mode 100644 .github/scripts/src/Downstream/Constants.php create mode 100644 .github/scripts/src/Downstream/Dockerfile.php create mode 100644 .github/scripts/src/Downstream/Exception.php create mode 100644 .github/scripts/src/Downstream/Orchestrator.php create mode 100644 .github/scripts/src/Downstream/Pull.php create mode 100644 .github/scripts/src/Downstream/Release.php create mode 100644 .github/scripts/src/Downstream/Repository.php create mode 100644 .github/scripts/src/Downstream/Repository/GitHub.php create mode 100644 .github/scripts/src/Downstream/Version.php create mode 100644 .github/scripts/tests/Unit/Downstream/BumpTest.php create mode 100644 .github/scripts/tests/Unit/Downstream/ChecksTest.php create mode 100644 .github/scripts/tests/Unit/Downstream/Fake.php create mode 100644 .github/scripts/tests/Unit/Downstream/OrchestratorTest.php create mode 100644 .github/scripts/tests/Unit/Downstream/ReleaseTest.php diff --git a/.github/scripts/bin/downstream.php b/.github/scripts/bin/downstream.php new file mode 100755 index 0000000..2b2fa61 --- /dev/null +++ b/.github/scripts/bin/downstream.php @@ -0,0 +1,48 @@ +#!/usr/bin/env php +execute(array_slice($argv, 1)), +))->render(); + +if ($output !== '') { + $path = getenv('GITHUB_OUTPUT') + ?: throw new RuntimeException('GITHUB_OUTPUT is required'); + $written = file_put_contents($path, $output, FILE_APPEND | LOCK_EX); + if ($written !== strlen($output)) { + throw new RuntimeException('Unable to write workflow outputs'); + } +} diff --git a/.github/scripts/src/Downstream/Application.php b/.github/scripts/src/Downstream/Application.php new file mode 100644 index 0000000..1c2798d --- /dev/null +++ b/.github/scripts/src/Downstream/Application.php @@ -0,0 +1,96 @@ + $arguments + * + * @return array + */ + public function execute(array $arguments): array + { + if ($arguments === []) { + throw new InvalidArgumentException( + 'A downstream operation is required', + ); + } + + [$operation, $values] = [array_shift($arguments), $arguments]; + + return match ([$operation, count($values)]) { + ['propose', 1] => $this->propose($values[0]), + ['wait', 1] => $this->wait($this->integer($values[0])), + ['release', 2] => $this->release( + $this->integer($values[0]), + $values[1], + ), + default => throw new InvalidArgumentException( + "Unknown downstream operation '{$operation}'", + ), + }; + } + + /** + * @return array + */ + private function propose(string $version): array + { + $pull = $this->orchestrator->propose($version); + if ($pull === null) { + return ['changed' => 'false']; + } + + return [ + 'changed' => 'true', + 'pull' => (string) $pull->number, + 'head' => $pull->head, + 'base' => $pull->base, + ]; + } + + /** + * @return array + */ + private function wait(int $pull): array + { + $this->orchestrator->wait($pull); + + return ['checks' => 'success']; + } + + /** + * @return array + */ + private function release(int $pull, string $head): array + { + $release = $this->orchestrator->release($pull, $head); + + return [ + 'tag' => (string) $release, + 'application' => $release->application, + 'sub' => (string) $release->sub, + ]; + } + + private function integer(string $value): int + { + if (preg_match('/\A[1-9][0-9]*\z/', $value) !== 1) { + throw new InvalidArgumentException( + "Expected a positive integer, got '{$value}'", + ); + } + + return (int) $value; + } +} diff --git a/.github/scripts/src/Downstream/Bump.php b/.github/scripts/src/Downstream/Bump.php new file mode 100644 index 0000000..8794c21 --- /dev/null +++ b/.github/scripts/src/Downstream/Bump.php @@ -0,0 +1,20 @@ +current !== $this->selected; + } +} diff --git a/.github/scripts/src/Downstream/Checks.php b/.github/scripts/src/Downstream/Checks.php new file mode 100644 index 0000000..8569766 --- /dev/null +++ b/.github/scripts/src/Downstream/Checks.php @@ -0,0 +1,46 @@ + $checks + */ + public static function settled(array $checks): bool + { + if ($checks === []) { + return false; + } + + foreach ($checks as $check) { + if ($check['status'] !== 'COMPLETED') { + return false; + } + } + + return true; + } + + /** + * @param list $checks + * + * @return list + */ + public static function failed(array $checks): array + { + $failed = []; + foreach ($checks as $check) { + if (! in_array($check['conclusion'], self::PASSING, true)) { + $failed[] = "{$check['name']}={$check['conclusion']}"; + } + } + sort($failed, SORT_STRING); + + return $failed; + } +} diff --git a/.github/scripts/src/Downstream/Constants.php b/.github/scripts/src/Downstream/Constants.php new file mode 100644 index 0000000..acbba3e --- /dev/null +++ b/.github/scripts/src/Downstream/Constants.php @@ -0,0 +1,32 @@ +'; + + private const string DOCKERFILE = 'Dockerfile'; + + private const string CONSTANTS = 'app/init/constants.php'; + + private const string BRANCH = 'automation/base-'; + + private const int TIMEOUT = 7200; + + private const int INTERVAL = 30; + + public function __construct( + private Repository $repository, + private Dockerfile $dockerfile, + private Constants $constants, + private Clock $clock, + private Sleeper $sleeper, + private string $base = 'main', + ) { + } + + public function propose(string $version): ?Pull + { + $head = $this->repository->head($this->base); + $bump = $this->dockerfile->bump( + $this->repository->file(self::DOCKERFILE, $head), + $version, + ); + if (! $bump->changed()) { + return null; + } + + $branch = self::BRANCH . $version; + $this->repository->commit( + $branch, + $head, + self::DOCKERFILE, + $bump->content, + "chore: update base image to {$version}", + ); + + return $this->repository->open( + $branch, + $this->base, + "chore: update base image to {$version}", + self::MARKER + . "\n" + . "\n\nAutomated base image update from `{$bump->current}`" + . " to `{$version}`.", + ); + } + + public function wait(int $pull): void + { + $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); + + while (true) { + $checks = $this->repository->checks($pull); + if (Checks::settled($checks)) { + $failed = Checks::failed($checks); + if ($failed !== []) { + throw new Exception( + 'Base update CI did not succeed: ' + . implode(', ', $failed), + ); + } + + return; + } + + if ($deadline->expired($this->clock->now())) { + throw new Exception( + "Base update CI did not settle for pull request #{$pull}", + ); + } + + $this->sleeper->sleep(self::INTERVAL); + } + } + + public function release(int $pull, string $head): Release + { + $target = $this->repository->merge($pull, $head); + $application = $this->constants->application( + $this->repository->file(self::CONSTANTS, $target), + ); + $release = Release::next( + $application, + $this->repository->tags(Release::PREFIX), + ); + $this->repository->tag((string) $release, $target); + + return $release; + } +} diff --git a/.github/scripts/src/Downstream/Pull.php b/.github/scripts/src/Downstream/Pull.php new file mode 100644 index 0000000..102487b --- /dev/null +++ b/.github/scripts/src/Downstream/Pull.php @@ -0,0 +1,15 @@ +application)) { + throw new Exception( + "Application version must be MAJOR.MINOR.PATCH, got '{$this->application}'", + ); + } + if ($this->sub < 1) { + throw new Exception('Release sub-version must be positive'); + } + } + + /** + * @param list $tags + */ + public static function next(string $application, array $tags): self + { + $pattern = '/\A' . preg_quote(self::PREFIX, '/') + . preg_quote($application, '/') + . '-([0-9]+)\z/'; + + $highest = 0; + foreach ($tags as $tag) { + if (preg_match($pattern, $tag, $matched) !== 1) { + continue; + } + + $sub = (int) $matched[1]; + if ((string) $sub !== $matched[1]) { + throw new Exception( + "Release tag '{$tag}' has a non-canonical sub-version", + ); + } + if ($sub > $highest) { + $highest = $sub; + } + } + + return new self($application, $highest + 1); + } + + #[Override] + public function __toString(): string + { + return self::PREFIX . "{$this->application}-{$this->sub}"; + } +} diff --git a/.github/scripts/src/Downstream/Repository.php b/.github/scripts/src/Downstream/Repository.php new file mode 100644 index 0000000..f95aa7c --- /dev/null +++ b/.github/scripts/src/Downstream/Repository.php @@ -0,0 +1,41 @@ + + */ + public function tags(string $prefix): array; + + public function commit( + string $branch, + string $base, + string $path, + string $content, + string $message, + ): string; + + public function open( + string $branch, + string $base, + string $title, + string $body, + ): Pull; + + /** + * @return list + */ + public function checks(int $pull): array; + + public function merge(int $pull, string $head): string; + + public function tag(string $name, string $target): void; +} diff --git a/.github/scripts/src/Downstream/Repository/GitHub.php b/.github/scripts/src/Downstream/Repository/GitHub.php new file mode 100644 index 0000000..231147a --- /dev/null +++ b/.github/scripts/src/Downstream/Repository/GitHub.php @@ -0,0 +1,285 @@ +repository) !== 1) { + throw new Exception( + "Invalid GitHub repository '{$this->repository}'", + ); + } + } + + #[Override] + public function file(string $path, string $ref): string + { + $encoded = $this->text([ + 'gh', 'api', '-X', 'GET', + "repos/{$this->repository}/contents/{$path}", + '-H', "X-GitHub-Api-Version: {$this->version}", + '-f', "ref={$ref}", + '--jq', '.content', + ]); + $content = base64_decode(str_replace("\n", '', $encoded), true); + if ($content === false) { + throw new Exception("Unable to decode {$path} at {$ref}"); + } + + return $content; + } + + #[Override] + public function head(string $branch): string + { + return $this->sha( + $this->text([ + 'gh', 'api', '-X', 'GET', + "repos/{$this->repository}/commits/{$branch}", + '-H', "X-GitHub-Api-Version: {$this->version}", + '--jq', '.sha', + ]), + "head of {$branch}", + ); + } + + /** + * @return list + */ + #[Override] + public function tags(string $prefix): array + { + $output = $this->text([ + 'gh', 'api', '--paginate', + "repos/{$this->repository}/git/matching-refs/tags/{$prefix}", + '-H', "X-GitHub-Api-Version: {$this->version}", + '--jq', '.[].ref', + ]); + + $tags = []; + foreach (preg_split('/\R/', $output) ?: [] as $line) { + $line = trim($line); + if (str_starts_with($line, 'refs/tags/')) { + $tags[] = substr($line, strlen('refs/tags/')); + } + } + + return $tags; + } + + #[Override] + public function commit( + string $branch, + string $base, + string $path, + string $content, + string $message, + ): string { + $this->runner->run([ + 'gh', 'api', '-X', 'POST', + "repos/{$this->repository}/git/refs", + '-H', "X-GitHub-Api-Version: {$this->version}", + '-f', "ref=refs/heads/{$branch}", + '-f', "sha={$base}", + ]); + + $existing = $this->text([ + 'gh', 'api', '-X', 'GET', + "repos/{$this->repository}/contents/{$path}", + '-H', "X-GitHub-Api-Version: {$this->version}", + '-f', "ref={$branch}", + '--jq', '.sha', + ]); + + return $this->sha( + $this->text([ + 'gh', 'api', '-X', 'PUT', + "repos/{$this->repository}/contents/{$path}", + '-H', "X-GitHub-Api-Version: {$this->version}", + '-f', "branch={$branch}", + '-f', "message={$message}", + '-f', 'content=' . base64_encode($content), + '-f', 'sha=' . trim($existing), + '--jq', '.commit.sha', + ]), + 'update commit', + ); + } + + #[Override] + public function open( + string $branch, + string $base, + string $title, + string $body, + ): Pull { + $payload = $this->json([ + 'gh', 'api', '-X', 'POST', + "repos/{$this->repository}/pulls", + '-H', "X-GitHub-Api-Version: {$this->version}", + '-f', "title={$title}", + '-f', "head={$branch}", + '-f', "base={$base}", + '-f', "body={$body}", + ]); + + $number = $payload['number'] ?? null; + $head = $payload['head'] ?? null; + if (! is_int($number) || ! is_array($head)) { + throw new Exception('Pull request creation returned no number'); + } + + return new Pull( + $number, + $this->sha( + is_string($head['sha'] ?? null) ? $head['sha'] : '', + "head of pull request #{$number}", + ), + $base, + ); + } + + /** + * @return list + */ + #[Override] + public function checks(int $pull): array + { + $payload = $this->json([ + 'gh', 'pr', 'view', (string) $pull, + '--repo', $this->repository, + '--json', 'statusCheckRollup', + ]); + + $rollup = $payload['statusCheckRollup'] ?? null; + if (! is_array($rollup)) { + throw new Exception( + "Unable to read checks for pull request #{$pull}", + ); + } + + $checks = []; + foreach ($rollup as $check) { + if (! is_array($check)) { + continue; + } + + $name = $check['name'] ?? $check['context'] ?? ''; + $status = $check['status'] ?? $check['state'] ?? ''; + $conclusion = $check['conclusion'] ?? ''; + $checks[] = [ + 'name' => is_string($name) ? $name : '', + 'status' => is_string($status) ? strtoupper($status) : '', + 'conclusion' => is_string($conclusion) + ? strtoupper($conclusion) + : '', + ]; + } + + return $checks; + } + + #[Override] + public function merge(int $pull, string $head): string + { + $this->runner->run([ + 'gh', 'pr', 'merge', (string) $pull, + '--repo', $this->repository, + '--squash', + '--admin', + '--match-head-commit', $head, + ]); + + $target = $this->text([ + 'gh', 'pr', 'view', (string) $pull, + '--repo', $this->repository, + '--json', 'mergeCommit', + '--jq', '.mergeCommit.oid', + ]); + + return $this->sha($target, "merge commit for #{$pull}"); + } + + #[Override] + public function tag(string $name, string $target): void + { + $this->runner->run([ + 'gh', 'api', '-X', 'POST', + "repos/{$this->repository}/git/refs", + '-H', "X-GitHub-Api-Version: {$this->version}", + '-f', "ref=refs/tags/{$name}", + '-f', "sha={$target}", + ]); + + $created = $this->text([ + 'gh', 'api', '-X', 'GET', + "repos/{$this->repository}/git/ref/tags/{$name}", + '-H', "X-GitHub-Api-Version: {$this->version}", + '--jq', '.object.sha', + ]); + if (trim($created) !== $target) { + throw new Exception( + "Tag {$name} does not point at {$target}", + ); + } + } + + /** + * @param list $command + */ + private function text(array $command): string + { + return trim($this->runner->run($command)->output); + } + + /** + * @param list $command + * + * @return array + */ + private function json(array $command): array + { + $output = $this->runner->run($command)->output; + + try { + $payload = json_decode($output, true, 512, JSON_THROW_ON_ERROR); + } catch (JsonException $exception) { + throw new Exception( + 'Unable to decode GitHub response: ' + . $exception->getMessage(), + previous: $exception, + ); + } + + if (! is_array($payload)) { + throw new Exception('GitHub returned an unexpected response'); + } + + /** @var array $payload */ + return $payload; + } + + private function sha(string $value, string $subject): string + { + $value = trim($value); + if (preg_match('/\A[0-9a-f]{40}\z/', $value) !== 1) { + throw new Exception("Unable to read the {$subject}"); + } + + return $value; + } +} diff --git a/.github/scripts/src/Downstream/Version.php b/.github/scripts/src/Downstream/Version.php new file mode 100644 index 0000000..d6ebbc5 --- /dev/null +++ b/.github/scripts/src/Downstream/Version.php @@ -0,0 +1,15 @@ +bump($content, '2.0.1'); + + self::assertSame('2.0.0', $bump->current); + self::assertSame('2.0.1', $bump->selected); + self::assertSame(true, $bump->changed()); + self::assertSame( + "FROM appwrite/base:2.0.1 AS base\n" + . "FROM appwrite/base:2.0.1-xdebug AS xdebug\n" + . "# appwrite/base:2.0.1 ships without XDebug\n", + $bump->content, + ); + } + + public function test_reports_no_change_when_already_current(): void + { + $bump = (new Dockerfile())->bump( + "FROM appwrite/base:2.0.1 AS base\n", + '2.0.1', + ); + + self::assertSame(false, $bump->changed()); + } + + public function test_rejects_conflicting_versions(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Conflicting appwrite/base versions: 1.4.3, 2.0.0', + ); + + (new Dockerfile())->bump( + "FROM appwrite/base:2.0.0 AS base\n" + . "FROM appwrite/base:1.4.3 AS other\n", + '2.0.1', + ); + } + + public function test_rejects_a_dockerfile_with_no_reference(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('No appwrite/base reference found'); + + (new Dockerfile())->bump("FROM php:8.5-alpine\n", '2.0.1'); + } + + public function test_rejects_an_inexact_selected_version(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('exact MAJOR.MINOR.PATCH'); + + (new Dockerfile())->bump("FROM appwrite/base:2.0.0\n", '2.0'); + } +} diff --git a/.github/scripts/tests/Unit/Downstream/ChecksTest.php b/.github/scripts/tests/Unit/Downstream/ChecksTest.php new file mode 100644 index 0000000..0e00d39 --- /dev/null +++ b/.github/scripts/tests/Unit/Downstream/ChecksTest.php @@ -0,0 +1,68 @@ + $name, + 'status' => $status, + 'conclusion' => $conclusion, + ]; + } +} diff --git a/.github/scripts/tests/Unit/Downstream/Fake.php b/.github/scripts/tests/Unit/Downstream/Fake.php new file mode 100644 index 0000000..65623c2 --- /dev/null +++ b/.github/scripts/tests/Unit/Downstream/Fake.php @@ -0,0 +1,115 @@ + */ + public array $calls = []; + + public ?string $tagged = null; + + /** + * @param list $tags + * @param list> $rounds + */ + public function __construct( + private readonly string $dockerfile, + private readonly string $constants = "calls[] = "file:{$path}"; + + return str_ends_with($path, 'constants.php') + ? $this->constants + : $this->dockerfile; + } + + #[Override] + public function head(string $branch): string + { + $this->calls[] = "head:{$branch}"; + + return $this->head; + } + + /** + * @return list + */ + #[Override] + public function tags(string $prefix): array + { + $this->calls[] = "tags:{$prefix}"; + + return $this->tags; + } + + #[Override] + public function commit( + string $branch, + string $base, + string $path, + string $content, + string $message, + ): string { + $this->calls[] = "commit:{$branch}"; + + return $this->mergeCommit; + } + + #[Override] + public function open( + string $branch, + string $base, + string $title, + string $body, + ): Pull { + $this->calls[] = "open:{$branch}->{$base}"; + + return new Pull(93, $this->head, $base); + } + + /** + * @return list + */ + #[Override] + public function checks(int $pull): array + { + $this->calls[] = "checks:{$pull}"; + if ($this->rounds === []) { + throw new Exception('No further check rounds'); + } + + return array_shift($this->rounds); + } + + #[Override] + public function merge(int $pull, string $head): string + { + $this->calls[] = "merge:{$pull}@{$head}"; + + return $this->mergeCommit; + } + + #[Override] + public function tag(string $name, string $target): void + { + $this->calls[] = "tag:{$name}@{$target}"; + $this->tagged = $name; + } +} diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php new file mode 100644 index 0000000..dc06a98 --- /dev/null +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -0,0 +1,137 @@ +orchestrator($repository)->propose('2.0.1'); + + self::assertNotNull($pull); + self::assertSame(93, $pull->number); + self::assertSame('main', $pull->base); + self::assertSame( + [ + 'head:main', + 'file:Dockerfile', + 'commit:automation/base-2.0.1', + 'open:automation/base-2.0.1->main', + ], + $repository->calls, + ); + } + + public function test_opens_nothing_when_the_base_is_already_current(): void + { + $repository = new Fake("FROM appwrite/base:2.0.1 AS base\n"); + + self::assertNull($this->orchestrator($repository)->propose('2.0.1')); + self::assertSame( + ['head:main', 'file:Dockerfile'], + $repository->calls, + ); + } + + public function test_waits_until_every_check_concludes(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ + [self::check('build', 'IN_PROGRESS', '')], + [self::check('build', 'COMPLETED', 'SUCCESS')], + ], + ); + + $this->orchestrator($repository)->wait(93); + + self::assertSame(['checks:93', 'checks:93'], $repository->calls); + } + + public function test_refuses_to_continue_when_a_check_failed(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [[self::check('tests', 'COMPLETED', 'FAILURE')]], + ); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Base update CI did not succeed: tests=FAILURE', + ); + + $this->orchestrator($repository)->wait(93); + } + + public function test_merges_then_tags_the_merge_commit(): void + { + $repository = new Fake(self::DOCKERFILE); + $head = 'a0000000000000000000000000000000000000aa'; + + $release = $this->orchestrator($repository)->release(93, $head); + + self::assertSame('cl-1.9.6-2', (string) $release); + self::assertSame('cl-1.9.6-2', $repository->tagged); + self::assertSame( + [ + "merge:93@{$head}", + 'file:app/init/constants.php', + 'tags:cl-', + 'tag:cl-1.9.6-2@b0000000000000000000000000000000000000bb', + ], + $repository->calls, + ); + } + + /** + * @return array{name: string, status: string, conclusion: string} + */ + private static function check( + string $name, + string $status, + string $conclusion, + ): array { + return [ + 'name' => $name, + 'status' => $status, + 'conclusion' => $conclusion, + ]; + } + + private function orchestrator(Fake $repository): Orchestrator + { + $clock = $this->createStub(Clock::class); + $clock->method('now')->willReturn( + new DateTimeImmutable( + '2026-08-21T00:00:00+00:00', + new DateTimeZone('UTC'), + ), + ); + + return new Orchestrator( + $repository, + new Dockerfile(), + new Constants(), + $clock, + $this->createStub(Sleeper::class), + ); + } +} diff --git a/.github/scripts/tests/Unit/Downstream/ReleaseTest.php b/.github/scripts/tests/Unit/Downstream/ReleaseTest.php new file mode 100644 index 0000000..4309b14 --- /dev/null +++ b/.github/scripts/tests/Unit/Downstream/ReleaseTest.php @@ -0,0 +1,92 @@ +application); + self::assertSame(3, $release->sub); + } + + public function test_starts_at_one_for_an_unreleased_application(): void + { + self::assertSame( + 'cl-2.0.0-1', + (string) Release::next('2.0.0', ['cl-1.9.6-9']), + ); + } + + public function test_selects_the_semantic_maximum_sub_version(): void + { + self::assertSame( + 'cl-1.9.6-11', + (string) Release::next('1.9.6', [ + 'cl-1.9.6-9', + 'cl-1.9.6-10', + 'cl-1.9.6-2', + ]), + ); + } + + public function test_ignores_unrelated_and_prefixed_tags(): void + { + self::assertSame( + 'cl-1.9.6-1', + (string) Release::next('1.9.6', [ + 'cl-1.9.6-1-rc1', + 'cl-1.9.60-4', + '1.9.6-7', + 'cl-shared-tables-zdt-6', + ]), + ); + } + + public function test_rejects_a_non_canonical_sub_version(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('non-canonical sub-version'); + + Release::next('1.9.6', ['cl-1.9.6-01']); + } + + public function test_reads_the_stable_application_version(): void + { + self::assertSame( + '1.9.6', + (new Constants())->application( + "expectException(Exception::class); + $this->expectExceptionMessage( + 'Expected exactly one APP_VERSION_STABLE declaration, found 0', + ); + + (new Constants())->application("> "${GITHUB_STEP_SUMMARY}" diff --git a/CHANGES.md b/CHANGES.md index 201debf..14bea7b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,10 @@ * `verify.yml` runs `composer validate --strict`, `composer check-platform-reqs`, and `composer verify` on every push, so the automation is gated at pull-request time rather than only on the Monday run that uses it. * Dependabot now tracks the `composer` ecosystem. The automation is only as trustworthy as the Pint, PHPStan, and PHPUnit versions gating it. +### Add + +* Downstream base bump. After a base release publishes, the weekly job opens a pull request in `appwrite/appwrite` rewriting every `appwrite/base:` reference in its `Dockerfile`, waits for that pull request's checks to conclude, merges it, and tags the merge commit `cl-{APP_VERSION_STABLE}-{n}` — reading the application version from `app/init/constants.php` and taking the next unused sub-version for it. Lives in `.github/scripts/src/Downstream`, driven by `bin/downstream.php`. Requires a `DOWNSTREAM_TOKEN` secret with admin rights on the downstream repository, because `main` there requires an approving review and GitHub forbids self-approval; the merge bypasses that review requirement but never the checks. + ### Fix * The updater rewrote `PHP_*_VERSION` and left `PHP_*_COMMIT` / `PHP_*_CHECKSUM` at the superseded release. Protobuf failed loudly on the checksum, but the git-sourced extensions did not: the build fetched the old commit and shipped, say, brotli 0.20.0 in an image labelled 0.21.0. `Dockerfile::pins()` only ever located the version variable, so no companion reference was ever a candidate for replacement. Every dependency now carries its reference variable through the catalog, resolver, selector, and rewriter, and both move together or neither does. From 6036b146f6d564bd03a5d9fb247458ea1bbcd46d Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 21 Aug 2026 17:53:08 +1200 Subject: [PATCH 02/10] (fix): settle downstream checks before merging, and recover the tag Two holes in the downstream flow, both of which end badly in the main product repository. The waiter accepted the first non-empty rollup in which everything had concluded. A single fast check can finish before the heavy workflows have registered theirs, so the admin merge could land a pull request whose real CI had not started. Require the check set to be unchanged across two consecutive polls and a grace period to have passed. A run that died between the merge and the tag left the downstream pin in place with no release tag, and the next run read the Dockerfile as already current and skipped forever. Recover that state before proposing anything, bounded to the downstream tip so a merge main has moved past is left alone. Co-Authored-By: Claude Opus 5 --- .../scripts/src/Downstream/Application.php | 17 +++ .github/scripts/src/Downstream/Checks.php | 14 +++ .../scripts/src/Downstream/Orchestrator.php | 44 +++++++- .github/scripts/src/Downstream/Repository.php | 4 +- .../src/Downstream/Repository/GitHub.php | 37 ++++++- .github/scripts/src/Downstream/Tag.php | 14 +++ .../scripts/tests/Unit/Downstream/Fake.php | 20 +++- .../Unit/Downstream/OrchestratorTest.php | 100 +++++++++++++++--- .../scripts/tests/Unit/Downstream/Ticker.php | 32 ++++++ .github/workflows/dependencies.yml | 19 +++- CHANGES.md | 2 +- 11 files changed, 272 insertions(+), 31 deletions(-) create mode 100644 .github/scripts/src/Downstream/Tag.php create mode 100644 .github/scripts/tests/Unit/Downstream/Ticker.php diff --git a/.github/scripts/src/Downstream/Application.php b/.github/scripts/src/Downstream/Application.php index 1c2798d..e289015 100644 --- a/.github/scripts/src/Downstream/Application.php +++ b/.github/scripts/src/Downstream/Application.php @@ -29,6 +29,7 @@ public function execute(array $arguments): array [$operation, $values] = [array_shift($arguments), $arguments]; return match ([$operation, count($values)]) { + ['recover', 1] => $this->recover($values[0]), ['propose', 1] => $this->propose($values[0]), ['wait', 1] => $this->wait($this->integer($values[0])), ['release', 2] => $this->release( @@ -41,6 +42,22 @@ public function execute(array $arguments): array }; } + /** + * @return array + */ + private function recover(string $version): array + { + $release = $this->orchestrator->recover($version); + if ($release === null) { + return ['recovered' => 'false']; + } + + return [ + 'recovered' => 'true', + 'tag' => (string) $release, + ]; + } + /** * @return array */ diff --git a/.github/scripts/src/Downstream/Checks.php b/.github/scripts/src/Downstream/Checks.php index 8569766..a7dbcad 100644 --- a/.github/scripts/src/Downstream/Checks.php +++ b/.github/scripts/src/Downstream/Checks.php @@ -8,6 +8,20 @@ { private const array PASSING = ['SUCCESS', 'SKIPPED', 'NEUTRAL']; + /** + * @param list $checks + */ + public static function signature(array $checks): string + { + $names = []; + foreach ($checks as $check) { + $names[] = "{$check['name']}={$check['conclusion']}"; + } + sort($names, SORT_STRING); + + return implode("\0", $names); + } + /** * @param list $checks */ diff --git a/.github/scripts/src/Downstream/Orchestrator.php b/.github/scripts/src/Downstream/Orchestrator.php index 8f4e146..3f36131 100644 --- a/.github/scripts/src/Downstream/Orchestrator.php +++ b/.github/scripts/src/Downstream/Orchestrator.php @@ -22,6 +22,8 @@ private const int INTERVAL = 30; + private const int GRACE = 120; + public function __construct( private Repository $repository, private Dockerfile $dockerfile, @@ -67,9 +69,17 @@ public function wait(int $pull): void { $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); + $grace = Deadline::after($this->clock->now(), self::GRACE); + $previous = null; + while (true) { $checks = $this->repository->checks($pull); - if (Checks::settled($checks)) { + $signature = Checks::signature($checks); + if ( + Checks::settled($checks) + && $grace->expired($this->clock->now()) + && $signature === $previous + ) { $failed = Checks::failed($checks); if ($failed !== []) { throw new Exception( @@ -81,6 +91,7 @@ public function wait(int $pull): void return; } + $previous = $signature; if ($deadline->expired($this->clock->now())) { throw new Exception( "Base update CI did not settle for pull request #{$pull}", @@ -91,15 +102,42 @@ public function wait(int $pull): void } } + public function recover(string $version): ?Release + { + $target = $this->repository->mergeCommit(self::BRANCH . $version); + if ($target === null) { + return null; + } + + if ($target !== $this->repository->head($this->base)) { + return null; + } + + foreach ($this->repository->tags(Release::PREFIX) as $tag) { + if ($tag->target === $target) { + return null; + } + } + + return $this->tag($target); + } + public function release(int $pull, string $head): Release { - $target = $this->repository->merge($pull, $head); + return $this->tag($this->repository->merge($pull, $head)); + } + + private function tag(string $target): Release + { $application = $this->constants->application( $this->repository->file(self::CONSTANTS, $target), ); $release = Release::next( $application, - $this->repository->tags(Release::PREFIX), + array_map( + static fn (Tag $tag): string => $tag->name, + $this->repository->tags(Release::PREFIX), + ), ); $this->repository->tag((string) $release, $target); diff --git a/.github/scripts/src/Downstream/Repository.php b/.github/scripts/src/Downstream/Repository.php index f95aa7c..83c5adf 100644 --- a/.github/scripts/src/Downstream/Repository.php +++ b/.github/scripts/src/Downstream/Repository.php @@ -11,10 +11,12 @@ public function file(string $path, string $ref): string; public function head(string $branch): string; /** - * @return list + * @return list */ public function tags(string $prefix): array; + public function mergeCommit(string $branch): ?string; + public function commit( string $branch, string $base, diff --git a/.github/scripts/src/Downstream/Repository/GitHub.php b/.github/scripts/src/Downstream/Repository/GitHub.php index 231147a..3f6ec07 100644 --- a/.github/scripts/src/Downstream/Repository/GitHub.php +++ b/.github/scripts/src/Downstream/Repository/GitHub.php @@ -8,6 +8,7 @@ use DockerBase\Downstream\Exception; use DockerBase\Downstream\Pull; use DockerBase\Downstream\Repository; +use DockerBase\Downstream\Tag; use JsonException; use Override; @@ -58,7 +59,7 @@ public function head(string $branch): string } /** - * @return list + * @return list */ #[Override] public function tags(string $prefix): array @@ -67,20 +68,46 @@ public function tags(string $prefix): array 'gh', 'api', '--paginate', "repos/{$this->repository}/git/matching-refs/tags/{$prefix}", '-H', "X-GitHub-Api-Version: {$this->version}", - '--jq', '.[].ref', + '--jq', '.[] | "\(.ref)\t\(.object.sha)"', ]); $tags = []; foreach (preg_split('/\R/', $output) ?: [] as $line) { - $line = trim($line); - if (str_starts_with($line, 'refs/tags/')) { - $tags[] = substr($line, strlen('refs/tags/')); + $fields = explode("\t", trim($line)); + if ( + count($fields) !== 2 + || ! str_starts_with($fields[0], 'refs/tags/') + ) { + continue; } + + $tags[] = new Tag( + substr($fields[0], strlen('refs/tags/')), + $fields[1], + ); } return $tags; } + #[Override] + public function mergeCommit(string $branch): ?string + { + $output = $this->text([ + 'gh', 'pr', 'list', + '--repo', $this->repository, + '--head', $branch, + '--state', 'merged', + '--json', 'mergeCommit', + '--jq', '.[0].mergeCommit.oid // ""', + ]); + if (trim($output) === '') { + return null; + } + + return $this->sha($output, "merge commit for {$branch}"); + } + #[Override] public function commit( string $branch, diff --git a/.github/scripts/src/Downstream/Tag.php b/.github/scripts/src/Downstream/Tag.php new file mode 100644 index 0000000..c68a12b --- /dev/null +++ b/.github/scripts/src/Downstream/Tag.php @@ -0,0 +1,14 @@ + $tags + * @param list $tags * @param list> $rounds */ public function __construct( private readonly string $dockerfile, private readonly string $constants = " + * @return list */ #[Override] public function tags(string $prefix): array { $this->calls[] = "tags:{$prefix}"; - return $this->tags; + return $this->tags === [] + ? [new Tag('cl-1.9.6-1', 'c0000000000000000000000000000000000000cc')] + : $this->tags; + } + + #[Override] + public function mergeCommit(string $branch): ?string + { + $this->calls[] = "merged:{$branch}"; + + return $this->merged; } #[Override] diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php index dc06a98..d2e0986 100644 --- a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -4,14 +4,12 @@ namespace DockerBase\Tests\Unit\Downstream; -use DateTimeImmutable; -use DateTimeZone; -use DockerBase\Automation\Clock; use DockerBase\Automation\Sleeper; use DockerBase\Downstream\Constants; use DockerBase\Downstream\Dockerfile; use DockerBase\Downstream\Exception; use DockerBase\Downstream\Orchestrator; +use DockerBase\Downstream\Tag; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; @@ -58,19 +56,52 @@ public function test_waits_until_every_check_concludes(): void rounds: [ [self::check('build', 'IN_PROGRESS', '')], [self::check('build', 'COMPLETED', 'SUCCESS')], + [self::check('build', 'COMPLETED', 'SUCCESS')], ], ); $this->orchestrator($repository)->wait(93); - self::assertSame(['checks:93', 'checks:93'], $repository->calls); + self::assertSame( + ['checks:93', 'checks:93', 'checks:93'], + $repository->calls, + ); + } + + public function test_waits_out_a_late_registering_workflow(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ + [self::check('lint', 'COMPLETED', 'SUCCESS')], + [ + self::check('lint', 'COMPLETED', 'SUCCESS'), + self::check('tests', 'IN_PROGRESS', ''), + ], + [ + self::check('lint', 'COMPLETED', 'SUCCESS'), + self::check('tests', 'COMPLETED', 'SUCCESS'), + ], + [ + self::check('lint', 'COMPLETED', 'SUCCESS'), + self::check('tests', 'COMPLETED', 'SUCCESS'), + ], + ], + ); + + $this->orchestrator($repository)->wait(93); + + self::assertSame(4, count($repository->calls)); } public function test_refuses_to_continue_when_a_check_failed(): void { $repository = new Fake( self::DOCKERFILE, - rounds: [[self::check('tests', 'COMPLETED', 'FAILURE')]], + rounds: [ + [self::check('tests', 'COMPLETED', 'FAILURE')], + [self::check('tests', 'COMPLETED', 'FAILURE')], + ], ); $this->expectException(Exception::class); @@ -101,6 +132,55 @@ public function test_merges_then_tags_the_merge_commit(): void ); } + public function test_tags_a_merge_that_never_got_its_tag(): void + { + $merge = 'b0000000000000000000000000000000000000bb'; + $repository = new Fake( + "FROM appwrite/base:2.0.1 AS base\n", + head: $merge, + merged: $merge, + ); + + $release = $this->orchestrator($repository)->recover('2.0.1'); + + self::assertNotNull($release); + self::assertSame('cl-1.9.6-2', (string) $release); + self::assertSame('cl-1.9.6-2', $repository->tagged); + } + + public function test_does_not_recover_a_merge_already_tagged(): void + { + $merge = 'b0000000000000000000000000000000000000bb'; + $repository = new Fake( + "FROM appwrite/base:2.0.1 AS base\n", + tags: [new Tag('cl-1.9.6-2', $merge)], + head: $merge, + merged: $merge, + ); + + self::assertNull($this->orchestrator($repository)->recover('2.0.1')); + self::assertNull($repository->tagged); + } + + public function test_does_not_recover_a_merge_main_has_moved_past(): void + { + $repository = new Fake( + "FROM appwrite/base:2.0.1 AS base\n", + head: 'd0000000000000000000000000000000000000dd', + merged: 'b0000000000000000000000000000000000000bb', + ); + + self::assertNull($this->orchestrator($repository)->recover('2.0.1')); + self::assertNull($repository->tagged); + } + + public function test_recovers_nothing_without_a_merged_pull_request(): void + { + $repository = new Fake(self::DOCKERFILE); + + self::assertNull($this->orchestrator($repository)->recover('2.0.1')); + } + /** * @return array{name: string, status: string, conclusion: string} */ @@ -118,19 +198,11 @@ private static function check( private function orchestrator(Fake $repository): Orchestrator { - $clock = $this->createStub(Clock::class); - $clock->method('now')->willReturn( - new DateTimeImmutable( - '2026-08-21T00:00:00+00:00', - new DateTimeZone('UTC'), - ), - ); - return new Orchestrator( $repository, new Dockerfile(), new Constants(), - $clock, + new Ticker(), $this->createStub(Sleeper::class), ); } diff --git a/.github/scripts/tests/Unit/Downstream/Ticker.php b/.github/scripts/tests/Unit/Downstream/Ticker.php new file mode 100644 index 0000000..2d0eed2 --- /dev/null +++ b/.github/scripts/tests/Unit/Downstream/Ticker.php @@ -0,0 +1,32 @@ +ticks * $this->seconds; + ++$this->ticks; + + return new DateTimeImmutable( + '2026-08-21T00:00:00+00:00', + new DateTimeZone('UTC'), + )->modify("+{$elapsed} seconds"); + } +} diff --git a/.github/workflows/dependencies.yml b/.github/workflows/dependencies.yml index 0b8200b..17428ab 100644 --- a/.github/workflows/dependencies.yml +++ b/.github/workflows/dependencies.yml @@ -257,9 +257,18 @@ jobs: php .github/scripts/bin/orchestrator.php publish \ "${TAG}" "${HEAD}" "${PULL}" "${DRAFT}" + - name: Recover an untagged downstream release + id: downstream_recovery + if: steps.release.outcome == 'success' + env: + GH_TOKEN: ${{ secrets.DOWNSTREAM_TOKEN }} + TAG: ${{ steps.release.outputs.tag }} + run: | + php .github/scripts/bin/downstream.php recover "${TAG}" + - name: Propose the downstream base update id: downstream - if: steps.release.outcome == 'success' + if: steps.downstream_recovery.outputs.recovered == 'false' env: GH_TOKEN: ${{ secrets.DOWNSTREAM_TOKEN }} TAG: ${{ steps.release.outputs.tag }} @@ -285,9 +294,13 @@ jobs: php .github/scripts/bin/downstream.php release "${PULL}" "${HEAD}" - name: Summarise the downstream release - if: steps.downstream_release.outcome == 'success' + if: >- + steps.downstream_release.outcome == 'success' || + steps.downstream_recovery.outputs.recovered == 'true' env: - TAG: ${{ steps.downstream_release.outputs.tag }} + TAG: >- + ${{ steps.downstream_release.outputs.tag || + steps.downstream_recovery.outputs.tag }} run: | printf '## Downstream release\n\nTagged `%s` in `%s`.\n' \ "${TAG}" "${DOWNSTREAM_REPOSITORY}" >> "${GITHUB_STEP_SUMMARY}" diff --git a/CHANGES.md b/CHANGES.md index 14bea7b..ff694d8 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,7 +12,7 @@ ### Add -* Downstream base bump. After a base release publishes, the weekly job opens a pull request in `appwrite/appwrite` rewriting every `appwrite/base:` reference in its `Dockerfile`, waits for that pull request's checks to conclude, merges it, and tags the merge commit `cl-{APP_VERSION_STABLE}-{n}` — reading the application version from `app/init/constants.php` and taking the next unused sub-version for it. Lives in `.github/scripts/src/Downstream`, driven by `bin/downstream.php`. Requires a `DOWNSTREAM_TOKEN` secret with admin rights on the downstream repository, because `main` there requires an approving review and GitHub forbids self-approval; the merge bypasses that review requirement but never the checks. +* Downstream base bump. After a base release publishes, the weekly job opens a pull request in `appwrite/appwrite` rewriting every `appwrite/base:` reference in its `Dockerfile`, waits for that pull request's checks to conclude, merges it, and tags the merge commit `cl-{APP_VERSION_STABLE}-{n}` — reading the application version from `app/init/constants.php` and taking the next unused sub-version for it. Lives in `.github/scripts/src/Downstream`, driven by `bin/downstream.php`. The wait requires the check set to be unchanged across two consecutive polls and a grace period to have elapsed, so a fast check completing before the heavy workflows register cannot be mistaken for a finished run. A release that merged but never got its tag is recovered on the next run, bounded to the downstream tip so a superseded merge is not resurrected. Requires a `DOWNSTREAM_TOKEN` secret with admin rights on the downstream repository, because `main` there requires an approving review and GitHub forbids self-approval; the merge bypasses that review requirement but never the checks. ### Fix From ce3127646f6fc5ca2b86a2dbd9da949253f1f1ba Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 21 Aug 2026 17:55:02 +1200 Subject: [PATCH 03/10] (fix): keep the test clock parseable on PHP 8.3 new DateTimeImmutable(...)->modify() omits the parentheses that PHP only made optional in 8.4. Local PHP is 8.5 so Pint and PHPStan both parsed it, while the runner and the declared composer platform are 8.3, where it is a parse error. Co-Authored-By: Claude Opus 5 --- .github/scripts/tests/Unit/Downstream/Ticker.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/scripts/tests/Unit/Downstream/Ticker.php b/.github/scripts/tests/Unit/Downstream/Ticker.php index 2d0eed2..fd48c75 100644 --- a/.github/scripts/tests/Unit/Downstream/Ticker.php +++ b/.github/scripts/tests/Unit/Downstream/Ticker.php @@ -24,9 +24,9 @@ public function now(): DateTimeImmutable $elapsed = $this->ticks * $this->seconds; ++$this->ticks; - return new DateTimeImmutable( + return (new DateTimeImmutable( '2026-08-21T00:00:00+00:00', new DateTimeZone('UTC'), - )->modify("+{$elapsed} seconds"); + ))->modify("+{$elapsed} seconds"); } } From f4d721b51af0c51523cae9660518d01e3b21c323 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 21 Aug 2026 18:37:18 +1200 Subject: [PATCH 04/10] (fix): gate the downstream merge on required status checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Downstream CI expands a dynamic matrix into dozens of checks that register minutes apart, so no view of the currently visible rollup distinguishes a finished run from one whose matrix has not been generated yet. Two attempts to infer it — all-complete, then all-complete plus stability plus a grace window — were both wrong for the same reason, and the third would have been too. Read the branch's required status-check contexts and wait for exactly those to conclude. That set is declared rather than inferred, so a check that registers late is still waited for. Refuse to merge when the branch declares no required contexts. --admin bypasses branch protection, so without this the automation would merge having verified nothing. Recovery no longer requires the merge to be the downstream tip, only that it is reachable from the branch. The tip rule made a release unrecoverable as soon as anyone else merged, and the version-scoped lookup already prevents resurrecting an unrelated merge. Co-Authored-By: Claude Opus 5 --- .github/scripts/src/Downstream/Checks.php | 43 ++++++------ .../scripts/src/Downstream/Orchestrator.php | 29 ++++---- .github/scripts/src/Downstream/Repository.php | 7 ++ .../src/Downstream/Repository/GitHub.php | 37 ++++++++++ .../tests/Unit/Downstream/ChecksTest.php | 68 +++++++++++++------ .../scripts/tests/Unit/Downstream/Fake.php | 22 ++++++ .../Unit/Downstream/OrchestratorTest.php | 57 ++++++++++------ CHANGES.md | 2 +- 8 files changed, 187 insertions(+), 78 deletions(-) diff --git a/.github/scripts/src/Downstream/Checks.php b/.github/scripts/src/Downstream/Checks.php index a7dbcad..59952cf 100644 --- a/.github/scripts/src/Downstream/Checks.php +++ b/.github/scripts/src/Downstream/Checks.php @@ -10,46 +10,45 @@ /** * @param list $checks + * @param list $required + * + * @return list */ - public static function signature(array $checks): string + public static function pending(array $checks, array $required): array { - $names = []; + $concluded = []; foreach ($checks as $check) { - $names[] = "{$check['name']}={$check['conclusion']}"; - } - sort($names, SORT_STRING); - - return implode("\0", $names); - } - - /** - * @param list $checks - */ - public static function settled(array $checks): bool - { - if ($checks === []) { - return false; + if ($check['status'] === 'COMPLETED') { + $concluded[$check['name']] = $check['conclusion']; + } } - foreach ($checks as $check) { - if ($check['status'] !== 'COMPLETED') { - return false; + $pending = []; + foreach ($required as $context) { + if (! isset($concluded[$context])) { + $pending[] = $context; } } + sort($pending, SORT_STRING); - return true; + return $pending; } /** * @param list $checks + * @param list $required * * @return list */ - public static function failed(array $checks): array + public static function failed(array $checks, array $required): array { + $wanted = array_flip($required); $failed = []; foreach ($checks as $check) { - if (! in_array($check['conclusion'], self::PASSING, true)) { + if ( + isset($wanted[$check['name']]) + && ! in_array($check['conclusion'], self::PASSING, true) + ) { $failed[] = "{$check['name']}={$check['conclusion']}"; } } diff --git a/.github/scripts/src/Downstream/Orchestrator.php b/.github/scripts/src/Downstream/Orchestrator.php index 3f36131..22394b8 100644 --- a/.github/scripts/src/Downstream/Orchestrator.php +++ b/.github/scripts/src/Downstream/Orchestrator.php @@ -22,8 +22,6 @@ private const int INTERVAL = 30; - private const int GRACE = 120; - public function __construct( private Repository $repository, private Dockerfile $dockerfile, @@ -67,20 +65,21 @@ public function propose(string $version): ?Pull public function wait(int $pull): void { - $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); + $required = $this->repository->required($this->base); + if ($required === []) { + throw new Exception( + "Branch '{$this->base}' declares no required status checks, " + . 'so a merge cannot be verified', + ); + } - $grace = Deadline::after($this->clock->now(), self::GRACE); - $previous = null; + $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); while (true) { $checks = $this->repository->checks($pull); - $signature = Checks::signature($checks); - if ( - Checks::settled($checks) - && $grace->expired($this->clock->now()) - && $signature === $previous - ) { - $failed = Checks::failed($checks); + $pending = Checks::pending($checks, $required); + if ($pending === []) { + $failed = Checks::failed($checks, $required); if ($failed !== []) { throw new Exception( 'Base update CI did not succeed: ' @@ -91,10 +90,10 @@ public function wait(int $pull): void return; } - $previous = $signature; if ($deadline->expired($this->clock->now())) { throw new Exception( - "Base update CI did not settle for pull request #{$pull}", + 'Base update CI did not conclude for pull request ' + . "#{$pull}: " . implode(', ', $pending), ); } @@ -109,7 +108,7 @@ public function recover(string $version): ?Release return null; } - if ($target !== $this->repository->head($this->base)) { + if (! $this->repository->contains($this->base, $target)) { return null; } diff --git a/.github/scripts/src/Downstream/Repository.php b/.github/scripts/src/Downstream/Repository.php index 83c5adf..0efbb28 100644 --- a/.github/scripts/src/Downstream/Repository.php +++ b/.github/scripts/src/Downstream/Repository.php @@ -17,6 +17,13 @@ public function tags(string $prefix): array; public function mergeCommit(string $branch): ?string; + public function contains(string $branch, string $commit): bool; + + /** + * @return list + */ + public function required(string $branch): array; + public function commit( string $branch, string $base, diff --git a/.github/scripts/src/Downstream/Repository/GitHub.php b/.github/scripts/src/Downstream/Repository/GitHub.php index 3f6ec07..f8705fe 100644 --- a/.github/scripts/src/Downstream/Repository/GitHub.php +++ b/.github/scripts/src/Downstream/Repository/GitHub.php @@ -108,6 +108,43 @@ public function mergeCommit(string $branch): ?string return $this->sha($output, "merge commit for {$branch}"); } + #[Override] + public function contains(string $branch, string $commit): bool + { + $status = $this->text([ + 'gh', 'api', '-X', 'GET', + "repos/{$this->repository}/compare/{$branch}...{$commit}", + '-H', "X-GitHub-Api-Version: {$this->version}", + '--jq', '.status', + ]); + + return in_array(trim($status), ['identical', 'behind'], true); + } + + /** + * @return list + */ + #[Override] + public function required(string $branch): array + { + $output = $this->text([ + 'gh', 'api', '-X', 'GET', + "repos/{$this->repository}/branches/{$branch}/protection", + '-H', "X-GitHub-Api-Version: {$this->version}", + '--jq', '.required_status_checks.contexts // [] | .[]', + ]); + + $contexts = []; + foreach (preg_split('/\R/', $output) ?: [] as $line) { + $line = trim($line); + if ($line !== '') { + $contexts[] = $line; + } + } + + return $contexts; + } + #[Override] public function commit( string $branch, diff --git a/.github/scripts/tests/Unit/Downstream/ChecksTest.php b/.github/scripts/tests/Unit/Downstream/ChecksTest.php index 0e00d39..9b7bf4a 100644 --- a/.github/scripts/tests/Unit/Downstream/ChecksTest.php +++ b/.github/scripts/tests/Unit/Downstream/ChecksTest.php @@ -11,43 +11,69 @@ #[CoversClass(Checks::class)] final class ChecksTest extends TestCase { - public function test_is_unsettled_while_any_check_runs(): void + public function test_reports_a_required_check_that_has_not_registered(): void { self::assertSame( - false, - Checks::settled([ - self::check('build', 'COMPLETED', 'SUCCESS'), - self::check('tests', 'IN_PROGRESS', ''), - ]), + ['Tests / E2E'], + Checks::pending( + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + ['Tests / Unit', 'Tests / E2E'], + ), ); } - public function test_is_unsettled_when_no_checks_exist(): void + public function test_reports_a_required_check_that_is_still_running(): void { - self::assertSame(false, Checks::settled([])); + self::assertSame( + ['Build'], + Checks::pending( + [self::check('Build', 'IN_PROGRESS', '')], + ['Build'], + ), + ); + } + + public function test_ignores_checks_that_are_not_required(): void + { + $checks = [ + self::check('Tests / Unit', 'COMPLETED', 'SUCCESS'), + self::check('advisory', 'IN_PROGRESS', ''), + self::check('flaky-optional', 'COMPLETED', 'FAILURE'), + ]; + + self::assertSame([], Checks::pending($checks, ['Tests / Unit'])); + self::assertSame([], Checks::failed($checks, ['Tests / Unit'])); } - public function test_accepts_skipped_and_neutral_conclusions(): void + public function test_accepts_skipped_and_neutral_required_conclusions(): void { $checks = [ - self::check('build', 'COMPLETED', 'SUCCESS'), - self::check('tag-only', 'COMPLETED', 'SKIPPED'), - self::check('advisory', 'COMPLETED', 'NEUTRAL'), + self::check('Tests / Unit', 'COMPLETED', 'SKIPPED'), + self::check('Build', 'COMPLETED', 'NEUTRAL'), ]; - self::assertSame(true, Checks::settled($checks)); - self::assertSame([], Checks::failed($checks)); + self::assertSame( + [], + Checks::pending($checks, ['Tests / Unit', 'Build']), + ); + self::assertSame( + [], + Checks::failed($checks, ['Tests / Unit', 'Build']), + ); } - public function test_reports_every_failing_check(): void + public function test_reports_every_failing_required_check(): void { self::assertSame( - ['lint=CANCELLED', 'tests=FAILURE'], - Checks::failed([ - self::check('build', 'COMPLETED', 'SUCCESS'), - self::check('tests', 'COMPLETED', 'FAILURE'), - self::check('lint', 'COMPLETED', 'CANCELLED'), - ]), + ['Build=CANCELLED', 'Tests / Unit=FAILURE'], + Checks::failed( + [ + self::check('Tests / Unit', 'COMPLETED', 'FAILURE'), + self::check('Build', 'COMPLETED', 'CANCELLED'), + self::check('lint', 'COMPLETED', 'FAILURE'), + ], + ['Tests / Unit', 'Build'], + ), ); } diff --git a/.github/scripts/tests/Unit/Downstream/Fake.php b/.github/scripts/tests/Unit/Downstream/Fake.php index f212d1a..6b90b21 100644 --- a/.github/scripts/tests/Unit/Downstream/Fake.php +++ b/.github/scripts/tests/Unit/Downstream/Fake.php @@ -19,6 +19,7 @@ final class Fake implements Repository /** * @param list $tags + * @param list $requiredChecks * @param list> $rounds */ public function __construct( @@ -29,6 +30,8 @@ public function __construct( private readonly string $head = 'a0000000000000000000000000000000000000aa', private readonly string $mergeCommit = 'b0000000000000000000000000000000000000bb', private readonly ?string $merged = null, + private readonly bool $contained = true, + private readonly array $requiredChecks = ['Tests / Unit'], ) { } @@ -63,6 +66,25 @@ public function tags(string $prefix): array : $this->tags; } + /** + * @return list + */ + #[Override] + public function required(string $branch): array + { + $this->calls[] = "required:{$branch}"; + + return $this->requiredChecks; + } + + #[Override] + public function contains(string $branch, string $commit): bool + { + $this->calls[] = "contains:{$branch}"; + + return $this->contained; + } + #[Override] public function mergeCommit(string $branch): ?string { diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php index d2e0986..78a67fd 100644 --- a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -49,64 +49,69 @@ public function test_opens_nothing_when_the_base_is_already_current(): void ); } - public function test_waits_until_every_check_concludes(): void + public function test_waits_until_every_required_check_concludes(): void { $repository = new Fake( self::DOCKERFILE, rounds: [ - [self::check('build', 'IN_PROGRESS', '')], - [self::check('build', 'COMPLETED', 'SUCCESS')], - [self::check('build', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'IN_PROGRESS', '')], + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], ], ); $this->orchestrator($repository)->wait(93); self::assertSame( - ['checks:93', 'checks:93', 'checks:93'], + ['required:main', 'checks:93', 'checks:93'], $repository->calls, ); } - public function test_waits_out_a_late_registering_workflow(): void + public function test_waits_for_a_required_check_that_registers_late(): void { $repository = new Fake( self::DOCKERFILE, rounds: [ + [self::check('lint', 'COMPLETED', 'SUCCESS')], [self::check('lint', 'COMPLETED', 'SUCCESS')], [ self::check('lint', 'COMPLETED', 'SUCCESS'), - self::check('tests', 'IN_PROGRESS', ''), - ], - [ - self::check('lint', 'COMPLETED', 'SUCCESS'), - self::check('tests', 'COMPLETED', 'SUCCESS'), + self::check('Tests / Unit', 'IN_PROGRESS', ''), ], [ self::check('lint', 'COMPLETED', 'SUCCESS'), - self::check('tests', 'COMPLETED', 'SUCCESS'), + self::check('Tests / Unit', 'COMPLETED', 'SUCCESS'), ], ], ); $this->orchestrator($repository)->wait(93); - self::assertSame(4, count($repository->calls)); + self::assertSame(5, count($repository->calls)); + } + + public function test_refuses_to_merge_when_nothing_is_required(): void + { + $repository = new Fake(self::DOCKERFILE, requiredChecks: []); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + "Branch 'main' declares no required status checks", + ); + + $this->orchestrator($repository)->wait(93); } public function test_refuses_to_continue_when_a_check_failed(): void { $repository = new Fake( self::DOCKERFILE, - rounds: [ - [self::check('tests', 'COMPLETED', 'FAILURE')], - [self::check('tests', 'COMPLETED', 'FAILURE')], - ], + rounds: [[self::check('Tests / Unit', 'COMPLETED', 'FAILURE')]], ); $this->expectException(Exception::class); $this->expectExceptionMessage( - 'Base update CI did not succeed: tests=FAILURE', + 'Base update CI did not succeed: Tests / Unit=FAILURE', ); $this->orchestrator($repository)->wait(93); @@ -162,7 +167,7 @@ public function test_does_not_recover_a_merge_already_tagged(): void self::assertNull($repository->tagged); } - public function test_does_not_recover_a_merge_main_has_moved_past(): void + public function test_recovers_after_main_has_moved_past_the_merge(): void { $repository = new Fake( "FROM appwrite/base:2.0.1 AS base\n", @@ -170,6 +175,20 @@ public function test_does_not_recover_a_merge_main_has_moved_past(): void merged: 'b0000000000000000000000000000000000000bb', ); + $release = $this->orchestrator($repository)->recover('2.0.1'); + + self::assertNotNull($release); + self::assertSame('cl-1.9.6-2', (string) $release); + } + + public function test_does_not_recover_a_merge_absent_from_the_branch(): void + { + $repository = new Fake( + "FROM appwrite/base:2.0.1 AS base\n", + merged: 'b0000000000000000000000000000000000000bb', + contained: false, + ); + self::assertNull($this->orchestrator($repository)->recover('2.0.1')); self::assertNull($repository->tagged); } diff --git a/CHANGES.md b/CHANGES.md index ff694d8..8c2f096 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,7 +12,7 @@ ### Add -* Downstream base bump. After a base release publishes, the weekly job opens a pull request in `appwrite/appwrite` rewriting every `appwrite/base:` reference in its `Dockerfile`, waits for that pull request's checks to conclude, merges it, and tags the merge commit `cl-{APP_VERSION_STABLE}-{n}` — reading the application version from `app/init/constants.php` and taking the next unused sub-version for it. Lives in `.github/scripts/src/Downstream`, driven by `bin/downstream.php`. The wait requires the check set to be unchanged across two consecutive polls and a grace period to have elapsed, so a fast check completing before the heavy workflows register cannot be mistaken for a finished run. A release that merged but never got its tag is recovered on the next run, bounded to the downstream tip so a superseded merge is not resurrected. Requires a `DOWNSTREAM_TOKEN` secret with admin rights on the downstream repository, because `main` there requires an approving review and GitHub forbids self-approval; the merge bypasses that review requirement but never the checks. +* Downstream base bump. After a base release publishes, the weekly job opens a pull request in `appwrite/appwrite` rewriting every `appwrite/base:` reference in its `Dockerfile`, waits for that pull request's checks to conclude, merges it, and tags the merge commit `cl-{APP_VERSION_STABLE}-{n}` — reading the application version from `app/init/constants.php` and taking the next unused sub-version for it. Lives in `.github/scripts/src/Downstream`, driven by `bin/downstream.php`. The wait reads the downstream branch's required status-check contexts and holds until every one of them has concluded, rather than inferring completeness from whichever checks happen to be visible. Downstream CI expands a dynamic matrix into dozens of checks that register minutes apart, so a visible-checks heuristic can never tell a finished run from one that has not started; a declared required set can. A branch with no required contexts is refused outright, because `--admin` bypasses branch protection and an unverifiable merge would otherwise proceed. A release that merged but never got its tag is recovered on the next run, bounded to the downstream tip so a superseded merge is not resurrected. Requires a `DOWNSTREAM_TOKEN` secret with admin rights on the downstream repository, because `main` there requires an approving review and GitHub forbids self-approval; the merge bypasses that review requirement but never the checks. ### Fix From 78241a66eab917410fcf96d8a1f8a39699a4bd7e Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 21 Aug 2026 18:42:28 +1200 Subject: [PATCH 05/10] (fix): re-verify required checks at merge time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Waiting for the required checks and merging were separate steps, and the merge bypasses branch protection, so a check re-run between the wait's last poll and the merge would be ignored — the very state the wait exists to prevent, reachable through the gap between them. Re-read the required contexts and their conclusions immediately before merging. The window is now a single call rather than however long the merge step takes to start. Co-Authored-By: Claude Opus 5 --- .../scripts/src/Downstream/Orchestrator.php | 58 ++++++++++++++----- .../Unit/Downstream/OrchestratorTest.php | 39 ++++++++++++- CHANGES.md | 2 +- 3 files changed, 82 insertions(+), 17 deletions(-) diff --git a/.github/scripts/src/Downstream/Orchestrator.php b/.github/scripts/src/Downstream/Orchestrator.php index 22394b8..31f7da5 100644 --- a/.github/scripts/src/Downstream/Orchestrator.php +++ b/.github/scripts/src/Downstream/Orchestrator.php @@ -65,27 +65,14 @@ public function propose(string $version): ?Pull public function wait(int $pull): void { - $required = $this->repository->required($this->base); - if ($required === []) { - throw new Exception( - "Branch '{$this->base}' declares no required status checks, " - . 'so a merge cannot be verified', - ); - } - + $required = $this->required(); $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); while (true) { $checks = $this->repository->checks($pull); $pending = Checks::pending($checks, $required); if ($pending === []) { - $failed = Checks::failed($checks, $required); - if ($failed !== []) { - throw new Exception( - 'Base update CI did not succeed: ' - . implode(', ', $failed), - ); - } + $this->assertPassed($checks, $required); return; } @@ -123,9 +110,50 @@ public function recover(string $version): ?Release public function release(int $pull, string $head): Release { + $required = $this->required(); + $checks = $this->repository->checks($pull); + $pending = Checks::pending($checks, $required); + if ($pending !== []) { + throw new Exception( + 'Required checks are no longer concluded: ' + . implode(', ', $pending), + ); + } + $this->assertPassed($checks, $required); + return $this->tag($this->repository->merge($pull, $head)); } + /** + * @return list + */ + private function required(): array + { + $required = $this->repository->required($this->base); + if ($required === []) { + throw new Exception( + "Branch '{$this->base}' declares no required status checks, " + . 'so a merge cannot be verified', + ); + } + + return $required; + } + + /** + * @param list $checks + * @param list $required + */ + private function assertPassed(array $checks, array $required): void + { + $failed = Checks::failed($checks, $required); + if ($failed !== []) { + throw new Exception( + 'Base update CI did not succeed: ' . implode(', ', $failed), + ); + } + } + private function tag(string $target): Release { $application = $this->constants->application( diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php index 78a67fd..0783ba6 100644 --- a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -18,6 +18,8 @@ final class OrchestratorTest extends TestCase { private const string DOCKERFILE = "FROM appwrite/base:2.0.0 AS base\n"; + private const string HEAD = 'a0000000000000000000000000000000000000aa'; + public function test_opens_a_pull_request_for_a_new_base_version(): void { $repository = new Fake(self::DOCKERFILE); @@ -117,9 +119,42 @@ public function test_refuses_to_continue_when_a_check_failed(): void $this->orchestrator($repository)->wait(93); } + public function test_refuses_to_merge_a_check_that_went_pending_again(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [[self::check('Tests / Unit', 'IN_PROGRESS', '')]], + ); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Required checks are no longer concluded: Tests / Unit', + ); + + $this->orchestrator($repository)->release(93, self::HEAD); + } + + public function test_refuses_to_merge_a_check_that_failed_after_waiting(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [[self::check('Tests / Unit', 'COMPLETED', 'FAILURE')]], + ); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Base update CI did not succeed: Tests / Unit=FAILURE', + ); + + $this->orchestrator($repository)->release(93, self::HEAD); + } + public function test_merges_then_tags_the_merge_commit(): void { - $repository = new Fake(self::DOCKERFILE); + $repository = new Fake( + self::DOCKERFILE, + rounds: [[self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')]], + ); $head = 'a0000000000000000000000000000000000000aa'; $release = $this->orchestrator($repository)->release(93, $head); @@ -128,6 +163,8 @@ public function test_merges_then_tags_the_merge_commit(): void self::assertSame('cl-1.9.6-2', $repository->tagged); self::assertSame( [ + 'required:main', + 'checks:93', "merge:93@{$head}", 'file:app/init/constants.php', 'tags:cl-', diff --git a/CHANGES.md b/CHANGES.md index 8c2f096..ec63adb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,7 +12,7 @@ ### Add -* Downstream base bump. After a base release publishes, the weekly job opens a pull request in `appwrite/appwrite` rewriting every `appwrite/base:` reference in its `Dockerfile`, waits for that pull request's checks to conclude, merges it, and tags the merge commit `cl-{APP_VERSION_STABLE}-{n}` — reading the application version from `app/init/constants.php` and taking the next unused sub-version for it. Lives in `.github/scripts/src/Downstream`, driven by `bin/downstream.php`. The wait reads the downstream branch's required status-check contexts and holds until every one of them has concluded, rather than inferring completeness from whichever checks happen to be visible. Downstream CI expands a dynamic matrix into dozens of checks that register minutes apart, so a visible-checks heuristic can never tell a finished run from one that has not started; a declared required set can. A branch with no required contexts is refused outright, because `--admin` bypasses branch protection and an unverifiable merge would otherwise proceed. A release that merged but never got its tag is recovered on the next run, bounded to the downstream tip so a superseded merge is not resurrected. Requires a `DOWNSTREAM_TOKEN` secret with admin rights on the downstream repository, because `main` there requires an approving review and GitHub forbids self-approval; the merge bypasses that review requirement but never the checks. +* Downstream base bump. After a base release publishes, the weekly job opens a pull request in `appwrite/appwrite` rewriting every `appwrite/base:` reference in its `Dockerfile`, waits for that pull request's checks to conclude, merges it, and tags the merge commit `cl-{APP_VERSION_STABLE}-{n}` — reading the application version from `app/init/constants.php` and taking the next unused sub-version for it. Lives in `.github/scripts/src/Downstream`, driven by `bin/downstream.php`. The wait reads the downstream branch's required status-check contexts and holds until every one of them has concluded, rather than inferring completeness from whichever checks happen to be visible. Downstream CI expands a dynamic matrix into dozens of checks that register minutes apart, so a visible-checks heuristic can never tell a finished run from one that has not started; a declared required set can. A branch with no required contexts is refused outright, because `--admin` bypasses branch protection and an unverifiable merge would otherwise proceed. The required set is re-read immediately before the merge as well as during the wait, so a check re-run between the two steps cannot be bypassed. A release that merged but never got its tag is recovered on the next run, bounded to the downstream tip so a superseded merge is not resurrected. Requires a `DOWNSTREAM_TOKEN` secret with admin rights on the downstream repository, because `main` there requires an approving review and GitHub forbids self-approval; the merge bypasses that review requirement but never the checks. ### Fix From a6bf2169e115c6dcdffdb24a06fe5f609c542c25 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 20:05:40 +1200 Subject: [PATCH 06/10] fix(downstream): let branch protection gate the downstream merge Reading the check rollup and then admin-merging can never be atomic: --match-head-commit binds the head SHA, not the check state, so a rerun or a newly-pending required check between the two calls was merged and tagged anyway. The only mechanism GitHub applies atomically at merge time is branch protection, and --admin exists to bypass it. The merge no longer passes --admin, so GitHub itself refuses the merge if a required check is not green at that instant, and a refusal is surfaced with the gh error rather than swallowed. Repository::checks is now Repository::status, returning the rollup plus mergeStateStatus, and wait/release share one settle() that polls until the required checks have concluded green AND the merge state is mergeable. BEHIND, DIRTY and DRAFT fail fast; BLOCKED and UNKNOWN keep polling until the deadline, since those clear on their own. Co-Authored-By: Claude Fable 5.1 --- .../scripts/src/Downstream/Orchestrator.php | 67 ++++++----- .github/scripts/src/Downstream/Repository.php | 5 +- .../src/Downstream/Repository/GitHub.php | 37 +++--- .github/scripts/src/Downstream/Status.php | 31 +++++ .../scripts/tests/Unit/Downstream/Fake.php | 17 +-- .../Unit/Downstream/OrchestratorTest.php | 111 ++++++++++++++++-- .../tests/Unit/Downstream/StatusTest.php | 41 +++++++ 7 files changed, 240 insertions(+), 69 deletions(-) create mode 100644 .github/scripts/src/Downstream/Status.php create mode 100644 .github/scripts/tests/Unit/Downstream/StatusTest.php diff --git a/.github/scripts/src/Downstream/Orchestrator.php b/.github/scripts/src/Downstream/Orchestrator.php index 31f7da5..4445f0d 100644 --- a/.github/scripts/src/Downstream/Orchestrator.php +++ b/.github/scripts/src/Downstream/Orchestrator.php @@ -65,27 +65,7 @@ public function propose(string $version): ?Pull public function wait(int $pull): void { - $required = $this->required(); - $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); - - while (true) { - $checks = $this->repository->checks($pull); - $pending = Checks::pending($checks, $required); - if ($pending === []) { - $this->assertPassed($checks, $required); - - return; - } - - if ($deadline->expired($this->clock->now())) { - throw new Exception( - 'Base update CI did not conclude for pull request ' - . "#{$pull}: " . implode(', ', $pending), - ); - } - - $this->sleeper->sleep(self::INTERVAL); - } + $this->settle($pull); } public function recover(string $version): ?Release @@ -110,20 +90,45 @@ public function recover(string $version): ?Release public function release(int $pull, string $head): Release { - $required = $this->required(); - $checks = $this->repository->checks($pull); - $pending = Checks::pending($checks, $required); - if ($pending !== []) { - throw new Exception( - 'Required checks are no longer concluded: ' - . implode(', ', $pending), - ); - } - $this->assertPassed($checks, $required); + $this->settle($pull); return $this->tag($this->repository->merge($pull, $head)); } + private function settle(int $pull): void + { + $required = $this->required(); + $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); + + while (true) { + $status = $this->repository->status($pull); + $blocking = Checks::pending($status->checks, $required); + if ($blocking === []) { + $this->assertPassed($status->checks, $required); + if ($status->mergeable()) { + return; + } + if ($status->stuck()) { + throw new Exception( + "Pull request #{$pull} cannot be merged: " + . "merge state {$status->state}", + ); + } + + $blocking = ["merge state {$status->state}"]; + } + + if ($deadline->expired($this->clock->now())) { + throw new Exception( + "Pull request #{$pull} did not become mergeable: " + . implode(', ', $blocking), + ); + } + + $this->sleeper->sleep(self::INTERVAL); + } + } + /** * @return list */ diff --git a/.github/scripts/src/Downstream/Repository.php b/.github/scripts/src/Downstream/Repository.php index 0efbb28..471f8b9 100644 --- a/.github/scripts/src/Downstream/Repository.php +++ b/.github/scripts/src/Downstream/Repository.php @@ -39,10 +39,7 @@ public function open( string $body, ): Pull; - /** - * @return list - */ - public function checks(int $pull): array; + public function status(int $pull): Status; public function merge(int $pull, string $head): string; diff --git a/.github/scripts/src/Downstream/Repository/GitHub.php b/.github/scripts/src/Downstream/Repository/GitHub.php index f8705fe..df00bec 100644 --- a/.github/scripts/src/Downstream/Repository/GitHub.php +++ b/.github/scripts/src/Downstream/Repository/GitHub.php @@ -8,6 +8,7 @@ use DockerBase\Downstream\Exception; use DockerBase\Downstream\Pull; use DockerBase\Downstream\Repository; +use DockerBase\Downstream\Status; use DockerBase\Downstream\Tag; use JsonException; use Override; @@ -217,22 +218,20 @@ public function open( ); } - /** - * @return list - */ #[Override] - public function checks(int $pull): array + public function status(int $pull): Status { $payload = $this->json([ 'gh', 'pr', 'view', (string) $pull, '--repo', $this->repository, - '--json', 'statusCheckRollup', + '--json', 'mergeStateStatus,statusCheckRollup', ]); $rollup = $payload['statusCheckRollup'] ?? null; - if (! is_array($rollup)) { + $state = $payload['mergeStateStatus'] ?? null; + if (! is_array($rollup) || ! is_string($state)) { throw new Exception( - "Unable to read checks for pull request #{$pull}", + "Unable to read the status of pull request #{$pull}", ); } @@ -254,19 +253,27 @@ public function checks(int $pull): array ]; } - return $checks; + return new Status($checks, strtoupper($state)); } #[Override] public function merge(int $pull, string $head): string { - $this->runner->run([ - 'gh', 'pr', 'merge', (string) $pull, - '--repo', $this->repository, - '--squash', - '--admin', - '--match-head-commit', $head, - ]); + $result = $this->runner->run( + [ + 'gh', 'pr', 'merge', (string) $pull, + '--repo', $this->repository, + '--squash', + '--match-head-commit', $head, + ], + check: false, + ); + if (! $result->succeeded()) { + throw new Exception( + "GitHub refused to merge pull request #{$pull} at {$head}: " + . trim($result->error), + ); + } $target = $this->text([ 'gh', 'pr', 'view', (string) $pull, diff --git a/.github/scripts/src/Downstream/Status.php b/.github/scripts/src/Downstream/Status.php new file mode 100644 index 0000000..57a0cf6 --- /dev/null +++ b/.github/scripts/src/Downstream/Status.php @@ -0,0 +1,31 @@ + $checks + */ + public function __construct( + public array $checks, + public string $state, + ) { + } + + public function mergeable(): bool + { + return in_array($this->state, self::MERGEABLE, true); + } + + public function stuck(): bool + { + return in_array($this->state, self::STUCK, true); + } +} diff --git a/.github/scripts/tests/Unit/Downstream/Fake.php b/.github/scripts/tests/Unit/Downstream/Fake.php index 6b90b21..2e3a871 100644 --- a/.github/scripts/tests/Unit/Downstream/Fake.php +++ b/.github/scripts/tests/Unit/Downstream/Fake.php @@ -7,6 +7,7 @@ use DockerBase\Downstream\Exception; use DockerBase\Downstream\Pull; use DockerBase\Downstream\Repository; +use DockerBase\Downstream\Status; use DockerBase\Downstream\Tag; use Override; @@ -21,6 +22,7 @@ final class Fake implements Repository * @param list $tags * @param list $requiredChecks * @param list> $rounds + * @param list $states */ public function __construct( private readonly string $dockerfile, @@ -32,6 +34,7 @@ public function __construct( private readonly ?string $merged = null, private readonly bool $contained = true, private readonly array $requiredChecks = ['Tests / Unit'], + private array $states = [], ) { } @@ -118,18 +121,18 @@ public function open( return new Pull(93, $this->head, $base); } - /** - * @return list - */ #[Override] - public function checks(int $pull): array + public function status(int $pull): Status { - $this->calls[] = "checks:{$pull}"; + $this->calls[] = "status:{$pull}"; if ($this->rounds === []) { - throw new Exception('No further check rounds'); + throw new Exception('No further status rounds'); } - return array_shift($this->rounds); + return new Status( + array_shift($this->rounds), + array_shift($this->states) ?? 'CLEAN', + ); } #[Override] diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php index 0783ba6..a86307c 100644 --- a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -64,11 +64,84 @@ public function test_waits_until_every_required_check_concludes(): void $this->orchestrator($repository)->wait(93); self::assertSame( - ['required:main', 'checks:93', 'checks:93'], + ['required:main', 'status:93', 'status:93'], $repository->calls, ); } + public function test_waits_for_branch_protection_to_clear(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + ], + states: ['UNKNOWN', 'BLOCKED', 'CLEAN'], + ); + + $this->orchestrator($repository)->wait(93); + + self::assertSame( + ['required:main', 'status:93', 'status:93', 'status:93'], + $repository->calls, + ); + } + + public function test_gives_up_when_the_merge_stays_blocked(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + ], + states: ['BLOCKED', 'BLOCKED'], + ); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Pull request #93 did not become mergeable: merge state BLOCKED', + ); + + $this->orchestrator($repository, 7200)->wait(93); + } + + public function test_gives_up_when_a_required_check_never_concludes(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ + [self::check('Tests / Unit', 'IN_PROGRESS', '')], + [self::check('Tests / Unit', 'IN_PROGRESS', '')], + ], + ); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Pull request #93 did not become mergeable: Tests / Unit', + ); + + $this->orchestrator($repository, 7200)->wait(93); + } + + public function test_refuses_a_pull_request_that_cannot_merge(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [[self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')]], + states: ['DIRTY'], + ); + + $this->expectException(Exception::class); + $this->expectExceptionMessage( + 'Pull request #93 cannot be merged: merge state DIRTY', + ); + + $this->orchestrator($repository)->release(93, self::HEAD); + } + public function test_waits_for_a_required_check_that_registers_late(): void { $repository = new Fake( @@ -119,19 +192,31 @@ public function test_refuses_to_continue_when_a_check_failed(): void $this->orchestrator($repository)->wait(93); } - public function test_refuses_to_merge_a_check_that_went_pending_again(): void + public function test_waits_for_a_rerun_check_before_merging(): void { $repository = new Fake( self::DOCKERFILE, - rounds: [[self::check('Tests / Unit', 'IN_PROGRESS', '')]], + rounds: [ + [self::check('Tests / Unit', 'IN_PROGRESS', '')], + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + ], ); - $this->expectException(Exception::class); - $this->expectExceptionMessage( - 'Required checks are no longer concluded: Tests / Unit', - ); + $release = $this->orchestrator($repository)->release(93, self::HEAD); - $this->orchestrator($repository)->release(93, self::HEAD); + self::assertSame('cl-1.9.6-2', (string) $release); + self::assertSame( + [ + 'required:main', + 'status:93', + 'status:93', + 'merge:93@' . self::HEAD, + 'file:app/init/constants.php', + 'tags:cl-', + 'tag:cl-1.9.6-2@b0000000000000000000000000000000000000bb', + ], + $repository->calls, + ); } public function test_refuses_to_merge_a_check_that_failed_after_waiting(): void @@ -164,7 +249,7 @@ public function test_merges_then_tags_the_merge_commit(): void self::assertSame( [ 'required:main', - 'checks:93', + 'status:93', "merge:93@{$head}", 'file:app/init/constants.php', 'tags:cl-', @@ -252,13 +337,15 @@ private static function check( ]; } - private function orchestrator(Fake $repository): Orchestrator - { + private function orchestrator( + Fake $repository, + int $seconds = 90, + ): Orchestrator { return new Orchestrator( $repository, new Dockerfile(), new Constants(), - new Ticker(), + new Ticker($seconds), $this->createStub(Sleeper::class), ); } diff --git a/.github/scripts/tests/Unit/Downstream/StatusTest.php b/.github/scripts/tests/Unit/Downstream/StatusTest.php new file mode 100644 index 0000000..82276fd --- /dev/null +++ b/.github/scripts/tests/Unit/Downstream/StatusTest.php @@ -0,0 +1,41 @@ + + */ + public static function states(): iterable + { + yield 'clean' => ['CLEAN', true, false]; + yield 'hooks' => ['HAS_HOOKS', true, false]; + yield 'unstable' => ['UNSTABLE', true, false]; + yield 'blocked' => ['BLOCKED', false, false]; + yield 'unknown' => ['UNKNOWN', false, false]; + yield 'behind' => ['BEHIND', false, true]; + yield 'dirty' => ['DIRTY', false, true]; + yield 'draft' => ['DRAFT', false, true]; + } + + #[DataProvider('states')] + public function test_classifies_the_merge_state( + string $state, + bool $mergeable, + bool $stuck, + ): void { + $status = new Status([], $state); + + self::assertSame($mergeable, $status->mergeable()); + self::assertSame($stuck, $status->stuck()); + } +} From e059332804125323178b8863e96774ff1dc46337 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 20:41:55 +1200 Subject: [PATCH 07/10] fix(downstream): bypass protection only for the review it cannot satisfy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dropping --admin outright traded a race for a deadlock: the same DOWNSTREAM_TOKEN opens and merges the pull request, GitHub forbids self-approval, and appwrite/appwrite main requires one review — so the merge is refused on every release and settle() sat on BLOCKED until its two-hour deadline. The merge is now attempted without --admin first, so GitHub evaluates the required checks itself, atomically, whenever it can. Only if it refuses are the checks re-read; a required check that is pending or red rethrows the refusal, and --admin is used solely when the checks are still green and the block is therefore the review requirement. settle() no longer waits for BLOCKED to clear, because nothing clears it before the merge. It waits for the required checks and fails fast on BEHIND, DIRTY and DRAFT, which the automation cannot resolve either. The four new orchestrator tests assert outcomes rather than the collaborator transcript: the release that comes back, whether the merge bypassed protection, and that nothing is tagged when a check goes red between the last poll and the merge. Co-Authored-By: Claude Fable 5.1 --- .../scripts/src/Downstream/Orchestrator.php | 48 +++++++++++-- .github/scripts/src/Downstream/Repository.php | 6 +- .../src/Downstream/Repository/GitHub.php | 22 +++--- .github/scripts/src/Downstream/Status.php | 12 ++-- .../scripts/tests/Unit/Downstream/Fake.php | 15 +++- .../Unit/Downstream/OrchestratorTest.php | 69 +++++++++++++++---- .../tests/Unit/Downstream/StatusTest.php | 12 ++-- 7 files changed, 143 insertions(+), 41 deletions(-) diff --git a/.github/scripts/src/Downstream/Orchestrator.php b/.github/scripts/src/Downstream/Orchestrator.php index 4445f0d..bd189b7 100644 --- a/.github/scripts/src/Downstream/Orchestrator.php +++ b/.github/scripts/src/Downstream/Orchestrator.php @@ -90,12 +90,46 @@ public function recover(string $version): ?Release public function release(int $pull, string $head): Release { - $this->settle($pull); + $required = $this->settle($pull); - return $this->tag($this->repository->merge($pull, $head)); + return $this->tag($this->mergeUnderProtection($pull, $head, $required)); } - private function settle(int $pull): void + /** + * Merge through branch protection so GitHub evaluates the required checks + * at merge time, which is the only place that evaluation is atomic. A + * refusal is only bypassed once the checks are re-read and still green, so + * the bypass covers the review requirement the automation cannot satisfy + * on its own and never a check that changed underneath it. + * + * @param list $required + */ + private function mergeUnderProtection( + int $pull, + string $head, + array $required, + ): string { + try { + return $this->repository->merge($pull, $head, bypass: false); + } catch (Exception $refused) { + $status = $this->repository->status($pull); + if (Checks::pending($status->checks, $required) !== []) { + throw $refused; + } + $this->assertPassed($status->checks, $required); + + return $this->repository->merge($pull, $head, bypass: true); + } + } + + /** + * Wait until every required check has concluded green. BLOCKED is not + * waited out: the review requirement holds a pull request there forever, + * and the merge itself is what resolves it. + * + * @return list + */ + private function settle(int $pull): array { $required = $this->required(); $deadline = Deadline::after($this->clock->now(), self::TIMEOUT); @@ -105,17 +139,17 @@ private function settle(int $pull): void $blocking = Checks::pending($status->checks, $required); if ($blocking === []) { $this->assertPassed($status->checks, $required); - if ($status->mergeable()) { - return; - } if ($status->stuck()) { throw new Exception( "Pull request #{$pull} cannot be merged: " . "merge state {$status->state}", ); } + if (! $status->computing()) { + return $required; + } - $blocking = ["merge state {$status->state}"]; + $blocking = ['merge state UNKNOWN']; } if ($deadline->expired($this->clock->now())) { diff --git a/.github/scripts/src/Downstream/Repository.php b/.github/scripts/src/Downstream/Repository.php index 471f8b9..6f313c3 100644 --- a/.github/scripts/src/Downstream/Repository.php +++ b/.github/scripts/src/Downstream/Repository.php @@ -41,7 +41,11 @@ public function open( public function status(int $pull): Status; - public function merge(int $pull, string $head): string; + /** + * Squash-merge, pinned to $head. With $bypass the merge skips branch + * protection; without it GitHub evaluates the protection itself. + */ + public function merge(int $pull, string $head, bool $bypass): string; public function tag(string $name, string $target): void; } diff --git a/.github/scripts/src/Downstream/Repository/GitHub.php b/.github/scripts/src/Downstream/Repository/GitHub.php index df00bec..e463355 100644 --- a/.github/scripts/src/Downstream/Repository/GitHub.php +++ b/.github/scripts/src/Downstream/Repository/GitHub.php @@ -257,17 +257,19 @@ public function status(int $pull): Status } #[Override] - public function merge(int $pull, string $head): string + public function merge(int $pull, string $head, bool $bypass): string { - $result = $this->runner->run( - [ - 'gh', 'pr', 'merge', (string) $pull, - '--repo', $this->repository, - '--squash', - '--match-head-commit', $head, - ], - check: false, - ); + $command = [ + 'gh', 'pr', 'merge', (string) $pull, + '--repo', $this->repository, + '--squash', + '--match-head-commit', $head, + ]; + if ($bypass) { + $command[] = '--admin'; + } + + $result = $this->runner->run($command, check: false); if (! $result->succeeded()) { throw new Exception( "GitHub refused to merge pull request #{$pull} at {$head}: " diff --git a/.github/scripts/src/Downstream/Status.php b/.github/scripts/src/Downstream/Status.php index 57a0cf6..7b7bc7f 100644 --- a/.github/scripts/src/Downstream/Status.php +++ b/.github/scripts/src/Downstream/Status.php @@ -6,8 +6,6 @@ final readonly class Status { - private const array MERGEABLE = ['CLEAN', 'HAS_HOOKS', 'UNSTABLE']; - private const array STUCK = ['BEHIND', 'DIRTY', 'DRAFT']; /** @@ -19,11 +17,17 @@ public function __construct( ) { } - public function mergeable(): bool + /** + * GitHub computes mergeability lazily; UNKNOWN means "ask again". + */ + public function computing(): bool { - return in_array($this->state, self::MERGEABLE, true); + return $this->state === 'UNKNOWN'; } + /** + * Nothing this automation can do clears these states. + */ public function stuck(): bool { return in_array($this->state, self::STUCK, true); diff --git a/.github/scripts/tests/Unit/Downstream/Fake.php b/.github/scripts/tests/Unit/Downstream/Fake.php index 2e3a871..dc560a7 100644 --- a/.github/scripts/tests/Unit/Downstream/Fake.php +++ b/.github/scripts/tests/Unit/Downstream/Fake.php @@ -18,6 +18,10 @@ final class Fake implements Repository public ?string $tagged = null; + /** Merge attempts in order, each true when branch protection was bypassed. */ + /** @var list */ + public array $merges = []; + /** * @param list $tags * @param list $requiredChecks @@ -35,6 +39,7 @@ public function __construct( private readonly bool $contained = true, private readonly array $requiredChecks = ['Tests / Unit'], private array $states = [], + private readonly bool $protectionRefusesMerge = false, ) { } @@ -136,9 +141,17 @@ public function status(int $pull): Status } #[Override] - public function merge(int $pull, string $head): string + public function merge(int $pull, string $head, bool $bypass): string { $this->calls[] = "merge:{$pull}@{$head}"; + $this->merges[] = $bypass; + + if ($this->protectionRefusesMerge && ! $bypass) { + throw new Exception( + "GitHub refused to merge pull request #{$pull} at {$head}: " + . 'At least 1 approving review is required.', + ); + } return $this->mergeCommit; } diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php index a86307c..f73499e 100644 --- a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -69,43 +69,88 @@ public function test_waits_until_every_required_check_concludes(): void ); } - public function test_waits_for_branch_protection_to_clear(): void + public function test_merges_through_branch_protection(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [[self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')]], + states: ['CLEAN'], + ); + + $release = $this->orchestrator($repository)->release(93, self::HEAD); + + self::assertSame('cl-1.9.6-2', (string) $release); + self::assertSame( + [false], + $repository->merges, + 'a mergeable pull request must never be admin-merged', + ); + } + + public function test_bypasses_only_the_review_requirement(): void { $repository = new Fake( self::DOCKERFILE, rounds: [ [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], - [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], ], - states: ['UNKNOWN', 'BLOCKED', 'CLEAN'], + states: ['BLOCKED', 'BLOCKED'], + protectionRefusesMerge: true, ); - $this->orchestrator($repository)->wait(93); + $release = $this->orchestrator($repository)->release(93, self::HEAD); + self::assertSame('cl-1.9.6-2', (string) $release); self::assertSame( - ['required:main', 'status:93', 'status:93', 'status:93'], - $repository->calls, + [false, true], + $repository->merges, + 'the bypass must be a fallback, never the first attempt', ); } - public function test_gives_up_when_the_merge_stays_blocked(): void + public function test_does_not_bypass_a_check_that_went_red_at_the_merge(): void { $repository = new Fake( self::DOCKERFILE, rounds: [ [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'COMPLETED', 'FAILURE')], + ], + states: ['CLEAN', 'BLOCKED'], + protectionRefusesMerge: true, + ); + + try { + $this->orchestrator($repository)->release(93, self::HEAD); + self::fail('a failing required check must not be merged past'); + } catch (Exception $exception) { + self::assertStringContainsString( + 'Base update CI did not succeed', + $exception->getMessage(), + ); + } + + self::assertSame([false], $repository->merges); + self::assertNull($repository->tagged); + } + + public function test_does_not_bypass_a_check_that_went_pending_at_the_merge(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'IN_PROGRESS', '')], ], - states: ['BLOCKED', 'BLOCKED'], + states: ['CLEAN', 'BLOCKED'], + protectionRefusesMerge: true, ); $this->expectException(Exception::class); - $this->expectExceptionMessage( - 'Pull request #93 did not become mergeable: merge state BLOCKED', - ); + $this->expectExceptionMessage('GitHub refused to merge'); - $this->orchestrator($repository, 7200)->wait(93); + $this->orchestrator($repository)->release(93, self::HEAD); } public function test_gives_up_when_a_required_check_never_concludes(): void diff --git a/.github/scripts/tests/Unit/Downstream/StatusTest.php b/.github/scripts/tests/Unit/Downstream/StatusTest.php index 82276fd..47fd9d8 100644 --- a/.github/scripts/tests/Unit/Downstream/StatusTest.php +++ b/.github/scripts/tests/Unit/Downstream/StatusTest.php @@ -17,11 +17,11 @@ final class StatusTest extends TestCase */ public static function states(): iterable { - yield 'clean' => ['CLEAN', true, false]; - yield 'hooks' => ['HAS_HOOKS', true, false]; - yield 'unstable' => ['UNSTABLE', true, false]; + yield 'clean' => ['CLEAN', false, false]; + yield 'hooks' => ['HAS_HOOKS', false, false]; + yield 'unstable' => ['UNSTABLE', false, false]; yield 'blocked' => ['BLOCKED', false, false]; - yield 'unknown' => ['UNKNOWN', false, false]; + yield 'unknown' => ['UNKNOWN', true, false]; yield 'behind' => ['BEHIND', false, true]; yield 'dirty' => ['DIRTY', false, true]; yield 'draft' => ['DRAFT', false, true]; @@ -30,12 +30,12 @@ public static function states(): iterable #[DataProvider('states')] public function test_classifies_the_merge_state( string $state, - bool $mergeable, + bool $computing, bool $stuck, ): void { $status = new Status([], $state); - self::assertSame($mergeable, $status->mergeable()); + self::assertSame($computing, $status->computing()); self::assertSame($stuck, $status->stuck()); } } From cc51e33425a75d636769d445ee7855215042bf79 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 21:02:04 +1200 Subject: [PATCH 08/10] fix(downstream): classify the refusal before bypassing protection The fallback treated every refusal as a missing approval: a required deployment, signed commits, unresolved conversations or a branch restriction would all have been admin-merged once the status checks came back green. GitHub states the unmet requirement in the merge endpoint's error body and nowhere else, so that is what is now classified. Only a refusal recognised as the review requirement is bypassed, and an unrecognised message fails closed and rethrows, because the review requirement is the only protection a single automation identity structurally cannot satisfy. The merge assertions drop the ordered call transcript for the property they were really about: whether branch protection was bypassed at all. Co-Authored-By: Claude Fable 5.1 --- .../scripts/src/Downstream/Orchestrator.php | 17 +++-- .github/scripts/src/Downstream/Refusal.php | 40 +++++++++++ .../scripts/tests/Unit/Downstream/Fake.php | 21 ++++-- .../Unit/Downstream/OrchestratorTest.php | 60 ++++++++++++---- .../tests/Unit/Downstream/RefusalTest.php | 68 +++++++++++++++++++ 5 files changed, 183 insertions(+), 23 deletions(-) create mode 100644 .github/scripts/src/Downstream/Refusal.php create mode 100644 .github/scripts/tests/Unit/Downstream/RefusalTest.php diff --git a/.github/scripts/src/Downstream/Orchestrator.php b/.github/scripts/src/Downstream/Orchestrator.php index bd189b7..2db53ab 100644 --- a/.github/scripts/src/Downstream/Orchestrator.php +++ b/.github/scripts/src/Downstream/Orchestrator.php @@ -97,10 +97,15 @@ public function release(int $pull, string $head): Release /** * Merge through branch protection so GitHub evaluates the required checks - * at merge time, which is the only place that evaluation is atomic. A - * refusal is only bypassed once the checks are re-read and still green, so - * the bypass covers the review requirement the automation cannot satisfy - * on its own and never a check that changed underneath it. + * at merge time, which is the only place that evaluation is atomic. + * + * A refusal is bypassed only when GitHub says it was the review + * requirement - the one protection a single automation identity cannot + * satisfy, because it opens the pull request and GitHub forbids + * self-approval - and only when the required checks are re-read and still + * green. Every other refusal, including one whose reason is not + * recognised, is rethrown: a required deployment, signed commits, + * unresolved conversations or a branch restriction must stop the release. * * @param list $required */ @@ -112,6 +117,10 @@ private function mergeUnderProtection( try { return $this->repository->merge($pull, $head, bypass: false); } catch (Exception $refused) { + if (! new Refusal($refused->getMessage())->isReviewRequirement()) { + throw $refused; + } + $status = $this->repository->status($pull); if (Checks::pending($status->checks, $required) !== []) { throw $refused; diff --git a/.github/scripts/src/Downstream/Refusal.php b/.github/scripts/src/Downstream/Refusal.php new file mode 100644 index 0000000..a4d7440 --- /dev/null +++ b/.github/scripts/src/Downstream/Refusal.php @@ -0,0 +1,40 @@ +message); + + foreach (self::REVIEW as $phrase) { + if (str_contains($message, $phrase)) { + return true; + } + } + + return false; + } +} diff --git a/.github/scripts/tests/Unit/Downstream/Fake.php b/.github/scripts/tests/Unit/Downstream/Fake.php index dc560a7..52eafb3 100644 --- a/.github/scripts/tests/Unit/Downstream/Fake.php +++ b/.github/scripts/tests/Unit/Downstream/Fake.php @@ -18,9 +18,11 @@ final class Fake implements Repository public ?string $tagged = null; - /** Merge attempts in order, each true when branch protection was bypassed. */ - /** @var list */ - public array $merges = []; + /** True once a merge bypassed branch protection. */ + public bool $bypassed = false; + + /** True once a merge went through branch protection. */ + public bool $mergedUnderProtection = false; /** * @param list $tags @@ -39,7 +41,7 @@ public function __construct( private readonly bool $contained = true, private readonly array $requiredChecks = ['Tests / Unit'], private array $states = [], - private readonly bool $protectionRefusesMerge = false, + private readonly ?string $protectionRefusal = null, ) { } @@ -144,15 +146,20 @@ public function status(int $pull): Status public function merge(int $pull, string $head, bool $bypass): string { $this->calls[] = "merge:{$pull}@{$head}"; - $this->merges[] = $bypass; - if ($this->protectionRefusesMerge && ! $bypass) { + if (! is_null($this->protectionRefusal) && ! $bypass) { throw new Exception( "GitHub refused to merge pull request #{$pull} at {$head}: " - . 'At least 1 approving review is required.', + . $this->protectionRefusal, ); } + if ($bypass) { + $this->bypassed = true; + } else { + $this->mergedUnderProtection = true; + } + return $this->mergeCommit; } diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php index f73499e..8a49762 100644 --- a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -80,14 +80,13 @@ public function test_merges_through_branch_protection(): void $release = $this->orchestrator($repository)->release(93, self::HEAD); self::assertSame('cl-1.9.6-2', (string) $release); - self::assertSame( - [false], - $repository->merges, + self::assertFalse( + $repository->bypassed, 'a mergeable pull request must never be admin-merged', ); } - public function test_bypasses_only_the_review_requirement(): void + public function test_bypasses_a_missing_review(): void { $repository = new Fake( self::DOCKERFILE, @@ -96,17 +95,54 @@ public function test_bypasses_only_the_review_requirement(): void [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], ], states: ['BLOCKED', 'BLOCKED'], - protectionRefusesMerge: true, + protectionRefusal: 'At least 1 approving review is required by reviewers with write access.', ); $release = $this->orchestrator($repository)->release(93, self::HEAD); self::assertSame('cl-1.9.6-2', (string) $release); - self::assertSame( - [false, true], - $repository->merges, - 'the bypass must be a fallback, never the first attempt', + self::assertTrue($repository->bypassed); + } + + public function test_does_not_bypass_a_protection_other_than_review(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + ], + states: ['BLOCKED', 'BLOCKED'], + protectionRefusal: 'Required deployment "production" is pending.', + ); + + $this->expectException(Exception::class); + $this->expectExceptionMessage('Required deployment'); + + $this->orchestrator($repository)->release(93, self::HEAD); + } + + public function test_does_not_bypass_a_refusal_it_cannot_classify(): void + { + $repository = new Fake( + self::DOCKERFILE, + rounds: [ + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + [self::check('Tests / Unit', 'COMPLETED', 'SUCCESS')], + ], + states: ['BLOCKED', 'BLOCKED'], + protectionRefusal: 'Something GitHub has not said before.', ); + + try { + $this->orchestrator($repository)->release(93, self::HEAD); + self::fail('an unrecognised refusal must not be bypassed'); + } catch (Exception $exception) { + self::assertStringContainsString('Something GitHub has not said before', $exception->getMessage()); + } + + self::assertFalse($repository->bypassed); + self::assertNull($repository->tagged); } public function test_does_not_bypass_a_check_that_went_red_at_the_merge(): void @@ -118,7 +154,7 @@ public function test_does_not_bypass_a_check_that_went_red_at_the_merge(): void [self::check('Tests / Unit', 'COMPLETED', 'FAILURE')], ], states: ['CLEAN', 'BLOCKED'], - protectionRefusesMerge: true, + protectionRefusal: 'At least 1 approving review is required by reviewers with write access.', ); try { @@ -131,7 +167,7 @@ public function test_does_not_bypass_a_check_that_went_red_at_the_merge(): void ); } - self::assertSame([false], $repository->merges); + self::assertFalse($repository->bypassed); self::assertNull($repository->tagged); } @@ -144,7 +180,7 @@ public function test_does_not_bypass_a_check_that_went_pending_at_the_merge(): v [self::check('Tests / Unit', 'IN_PROGRESS', '')], ], states: ['CLEAN', 'BLOCKED'], - protectionRefusesMerge: true, + protectionRefusal: 'At least 1 approving review is required by reviewers with write access.', ); $this->expectException(Exception::class); diff --git a/.github/scripts/tests/Unit/Downstream/RefusalTest.php b/.github/scripts/tests/Unit/Downstream/RefusalTest.php new file mode 100644 index 0000000..f2ce909 --- /dev/null +++ b/.github/scripts/tests/Unit/Downstream/RefusalTest.php @@ -0,0 +1,68 @@ + + */ + public static function refusals(): iterable + { + yield 'one approving review' => [ + 'At least 1 approving review is required by reviewers with write access.', + true, + ]; + yield 'two approving reviews' => [ + 'At least 2 approving reviews are required by reviewers with write access.', + true, + ]; + yield 'changes requested' => [ + 'Changes requested by a reviewer.', + true, + ]; + yield 'required deployment' => [ + 'Required deployment "production" is pending.', + false, + ]; + yield 'unresolved conversations' => [ + 'All conversations on this pull request must be resolved.', + false, + ]; + yield 'signed commits' => [ + 'Commits must have valid signatures.', + false, + ]; + yield 'branch restriction' => [ + 'You are not authorized to push to this branch.', + false, + ]; + yield 'required status check' => [ + 'Required status check "Tests / Unit" is expected.', + false, + ]; + yield 'anything unrecognised' => [ + 'Something GitHub has not said before.', + false, + ]; + } + + #[DataProvider('refusals')] + public function test_only_a_review_requirement_may_be_bypassed( + string $message, + bool $bypassable, + ): void { + self::assertSame( + $bypassable, + new Refusal($message)->isReviewRequirement(), + ); + } +} From e2e38f4b5d678383d06c2d9acc2d0612ce31d6db Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 21:04:32 +1200 Subject: [PATCH 09/10] fix: parenthesise new before a method call Calling a method on `new Foo()` without parentheses is PHP 8.4 syntax and this package supports 8.3, where both files failed to parse. Co-Authored-By: Claude Fable 5.1 --- .github/scripts/src/Downstream/Orchestrator.php | 2 +- .github/scripts/tests/Unit/Downstream/RefusalTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/scripts/src/Downstream/Orchestrator.php b/.github/scripts/src/Downstream/Orchestrator.php index 2db53ab..2f8a19a 100644 --- a/.github/scripts/src/Downstream/Orchestrator.php +++ b/.github/scripts/src/Downstream/Orchestrator.php @@ -117,7 +117,7 @@ private function mergeUnderProtection( try { return $this->repository->merge($pull, $head, bypass: false); } catch (Exception $refused) { - if (! new Refusal($refused->getMessage())->isReviewRequirement()) { + if (! (new Refusal($refused->getMessage()))->isReviewRequirement()) { throw $refused; } diff --git a/.github/scripts/tests/Unit/Downstream/RefusalTest.php b/.github/scripts/tests/Unit/Downstream/RefusalTest.php index f2ce909..3063ddd 100644 --- a/.github/scripts/tests/Unit/Downstream/RefusalTest.php +++ b/.github/scripts/tests/Unit/Downstream/RefusalTest.php @@ -62,7 +62,7 @@ public function test_only_a_review_requirement_may_be_bypassed( ): void { self::assertSame( $bypassable, - new Refusal($message)->isReviewRequirement(), + (new Refusal($message))->isReviewRequirement(), ); } } From b183884b51b4f28b70ca1d0b048264a1f012ba94 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 22:04:52 +1200 Subject: [PATCH 10/10] fix(downstream): never bypass a refusal that names another protection A message naming a missing review alongside a pending deployment, an unresolved conversation or a branch restriction matched the review phrases and was treated as review-only, so the bypass could merge past the protection that actually blocked it. The classifier now rejects first: any phrase naming a protection other than review disqualifies the refusal, whatever else the message says. Only a refusal that names a review and nothing else is bypassable, and an unrecognised message still fails closed. The release test that pinned the whole required/status/merge/file/tags/ tag transcript now asserts what the release produced instead: the tag name, that it landed on the merge commit rather than the tested head, and that protection was not bypassed. Co-Authored-By: Claude Fable 5.1 --- .github/scripts/src/Downstream/Refusal.php | 35 ++++++++++++++++--- .../scripts/tests/Unit/Downstream/Fake.php | 4 +++ .../Unit/Downstream/OrchestratorTest.php | 15 +++----- .../tests/Unit/Downstream/RefusalTest.php | 14 ++++++++ 4 files changed, 54 insertions(+), 14 deletions(-) diff --git a/.github/scripts/src/Downstream/Refusal.php b/.github/scripts/src/Downstream/Refusal.php index a4d7440..d1648aa 100644 --- a/.github/scripts/src/Downstream/Refusal.php +++ b/.github/scripts/src/Downstream/Refusal.php @@ -7,10 +7,11 @@ /** * Classifies why GitHub refused a merge. * - * The merge endpoint states the unmet requirement in its error body and - * nowhere else - there is no field that enumerates which protections blocked - * a pull request. Only a refusal recognised as the review requirement may be - * bypassed; anything else, including an unrecognised message, is not. + * The merge endpoint states the unmet requirements in its error body and + * nowhere else - there is no field that enumerates which protections blocked a + * pull request. Only a refusal that names a review requirement and nothing + * else may be bypassed: a message naming a review *and* a pending deployment + * is not review-only, and neither is a message this does not recognise. */ final readonly class Refusal { @@ -21,6 +22,26 @@ 'changes requested', ]; + /** + * Protections a correct release waits for or fails on. None of them is + * something a second identity could satisfy on the automation's behalf. + */ + private const array OTHER = [ + 'status check', + 'deployment', + 'signature', + 'signed commit', + 'conversation', + 'not authorized', + 'not allowed to', + 'restriction', + 'merge queue', + 'linear history', + 'out of date', + 'behind the base', + 'conflict', + ]; + public function __construct(public string $message) { } @@ -29,6 +50,12 @@ public function isReviewRequirement(): bool { $message = strtolower($this->message); + foreach (self::OTHER as $phrase) { + if (str_contains($message, $phrase)) { + return false; + } + } + foreach (self::REVIEW as $phrase) { if (str_contains($message, $phrase)) { return true; diff --git a/.github/scripts/tests/Unit/Downstream/Fake.php b/.github/scripts/tests/Unit/Downstream/Fake.php index 52eafb3..f956411 100644 --- a/.github/scripts/tests/Unit/Downstream/Fake.php +++ b/.github/scripts/tests/Unit/Downstream/Fake.php @@ -18,6 +18,9 @@ final class Fake implements Repository public ?string $tagged = null; + /** The commit the tag was put on. */ + public ?string $taggedTarget = null; + /** True once a merge bypassed branch protection. */ public bool $bypassed = false; @@ -168,5 +171,6 @@ public function tag(string $name, string $target): void { $this->calls[] = "tag:{$name}@{$target}"; $this->tagged = $name; + $this->taggedTarget = $target; } } diff --git a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php index 8a49762..ab9b4c9 100644 --- a/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Downstream/OrchestratorTest.php @@ -286,18 +286,13 @@ public function test_waits_for_a_rerun_check_before_merging(): void $release = $this->orchestrator($repository)->release(93, self::HEAD); self::assertSame('cl-1.9.6-2', (string) $release); + self::assertSame('cl-1.9.6-2', $repository->tagged); self::assertSame( - [ - 'required:main', - 'status:93', - 'status:93', - 'merge:93@' . self::HEAD, - 'file:app/init/constants.php', - 'tags:cl-', - 'tag:cl-1.9.6-2@b0000000000000000000000000000000000000bb', - ], - $repository->calls, + 'b0000000000000000000000000000000000000bb', + $repository->taggedTarget, + 'the tag belongs on the merge commit, not on the head that was tested', ); + self::assertFalse($repository->bypassed); } public function test_refuses_to_merge_a_check_that_failed_after_waiting(): void diff --git a/.github/scripts/tests/Unit/Downstream/RefusalTest.php b/.github/scripts/tests/Unit/Downstream/RefusalTest.php index 3063ddd..b802d79 100644 --- a/.github/scripts/tests/Unit/Downstream/RefusalTest.php +++ b/.github/scripts/tests/Unit/Downstream/RefusalTest.php @@ -53,6 +53,20 @@ public static function refusals(): iterable 'Something GitHub has not said before.', false, ]; + yield 'a review alongside a deployment' => [ + 'At least 1 approving review is required by reviewers with write access. ' + . 'Required deployment "production" is pending.', + false, + ]; + yield 'a review alongside unresolved conversations' => [ + 'Changes requested by a reviewer. ' + . 'All conversations on this pull request must be resolved.', + false, + ]; + yield 'a review alongside a branch restriction' => [ + 'At least 2 approving reviews are required. You are not authorized to push to this branch.', + false, + ]; } #[DataProvider('refusals')]