docker-compose.yml (1)

74-74: Guard docker.sock mount behind a tests-only override or profile

Mounting the host Docker socket into the appwrite service is powerful and risky. Even though this is a dev compose and the comment says “Only needed for tests,” it grants root-equivalent control of the host Docker daemon to the container.

• Prefer moving this mount into a dedicated tests-only override (e.g., docker-compose.tests.yml) and using it only in CI: docker compose -f docker-compose.yml -f docker-compose.tests.yml up.
• Alternatively, gate under a compose profile used only in tests.

Also, note that tests call docker compose logs ... from inside the appwrite container. That requires the Docker CLI and compose plugin to be present in the image. If that’s not guaranteed, consider switching the test code to docker logs (see inline suggestion in SitesBase.php).

Would you like me to draft a docker-compose.tests.yml override that adds only this mount for CI runs?

tests/e2e/Services/Sites/SitesCustomServerTest.php (1)

408-411: Avoid passing $site by reference and set explicit polling budget

The reference capture isn’t used later and can confuse readers. Also, consider setting an explicit timeout/interval for consistency with other tests.

Apply this diff:

-        $this->assertEventually(function () use ($siteId, &$site) {
-            $site = $this->getSite($siteId);
-            $this->assertEquals('ssr', $site['body']['adapter']);
-        });
+        $this->assertEventually(function () use ($siteId) {
+            $site = $this->getSite($siteId);
+            $this->assertEquals('ssr', $site['body']['adapter']);
+        }, 100000, 500);

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

tests/e2e/Services/Sites/SitesBase.php (1)

57-66: Harden exit code check and avoid docker compose plugin dependency for logs

• Compare numeric exit code, not string, to avoid surprises.
• Prefer docker logs with the explicit container name to avoid requiring the Compose plugin inside the test container, and to limit output size for readability.

Apply this diff:

-            Console::execute("docker inspect openruntimes-executor --format='{{.State.ExitCode}}'", '', $this->stdout, $this->stderr);
-            if (\trim($this->stdout) !== '0') {
-                $msg = 'Executor has a problem: ' . $this->stderr . ' (' . $this->stdout . '), current status: ';
-
-                Console::execute("docker compose logs openruntimes-executor", '', $this->stdout, $this->stderr);
-                $msg .= $this->stdout . ' (' . $this->stderr . ')';
-
-                throw new Critical($msg . json_encode($deployment['body'], JSON_PRETTY_PRINT));
-            }
+            $inspectOut = '';
+            $inspectErr = '';
+            Console::execute("docker inspect openruntimes-executor --format='{{.State.ExitCode}}'", '', $inspectOut, $inspectErr);
+            $exitCode = (int) \trim($inspectOut);
+            if ($exitCode !== 0) {
+                $msg = 'Executor has a problem: ' . \trim($inspectErr) . ' (exitCode=' . $exitCode . '), current status: ';
+
+                $logsOut = '';
+                $logsErr = '';
+                // Avoid compose plugin dependency; limit log size for readability
+                Console::execute("docker logs --tail=500 openruntimes-executor", '', $logsOut, $logsErr);
+                $msg .= $logsOut . ' (' . $logsErr . ')';
+
+                throw new Critical($msg . json_encode($deployment['body'], JSON_PRETTY_PRINT));
+            }

.github/workflows/tests.yml (1)

137-144: Bound and harden failure log collection to keep CI output readable and resilient

Large unbounded logs can overwhelm CI output, and docker compose may exit non‑zero if a service is absent. Tail and no-color reduce noise; || true ensures logs collection never masks the original failure.

Apply this diff to all Failure Logs steps:

-          echo "=== Appwrite Worker Builds Logs ==="
-          docker compose logs appwrite-worker-builds
-          echo "=== OpenRuntimes Executor Logs ==="
-          docker compose logs openruntimes-executor
+          echo "=== Appwrite Worker Builds Logs ==="
+          docker compose logs --tail=200 --no-color appwrite-worker-builds || true
+          echo "=== OpenRuntimes Executor Logs ==="
+          docker compose logs --tail=200 --no-color openruntimes-executor || true

Also applies to: 212-219, 299-306, 340-347, 395-402, 437-444, 493-499

tests/extensions/Async/Eventually.php (1)

36-41: Guard against null throw on timeout with no captured exception

If the probe never throws an Exception (e.g., throws an Error/Throwable or returns falsey without asserting), $lastException could be null leading to a fatal when thrown. Prefer a safe fallback.

Example adjustment (outside the selected lines):

// before
throw $lastException;

// after
throw $lastException ?? new \RuntimeException('Eventually timed out without a captured exception.');

src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)

822-825: Use info level for normal “finished” messages

listLogs finished reads as a normal completion, not a warning. Consider lowering the severity.

-                        Console::warning('listLogs finished');
+                        Console::info('listLogs finished');

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

tests/e2e/Services/Sites/SitesCustomServerTest.php (2)

408-411: Good switch to eventual consistency; remove by-ref capture and bound the wait

Polling for adapter detection is the right call. Capturing $site by reference isn’t needed (not used after the block) and can be dropped. Also consider an explicit timeout to cap worst-case retries.

Apply this diff:

-        $this->assertEventually(function () use ($siteId, &$site) {
-            $site = $this->getSite($siteId);
-            $this->assertEquals('ssr', $site['body']['adapter']);
-        });
+        $this->assertEventually(function () use ($siteId) {
+            $site = $this->getSite($siteId);
+            $this->assertEquals('ssr', $site['body']['adapter']);
+        }, 120000, 500);

---

421-421: Retries added—confirm idempotent cleanup across attempts

Adding #[Retry(count: 3)] is reasonable. Please verify this test leaves no residual sites/domains when a prior attempt fails mid-run, so that retries won’t encounter conflicts or leak resources.

Optional: You may further reduce flakiness in this test by polling for adapter detection (like the SSR test) rather than relying on a single read after deployment. If you want, I can draft that change.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.