Conversation
Separate specifications
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the compute specification system by splitting the single Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (1)
35-68: Guard against missing second specification entry
testListSpecsnow uses$specifications['body']['specifications'][1]for the runtime spec without asserting that index1exists or thattotal >= 2. If the environment is ever configured with only a single spec, this test will fail with an undefined index before reaching the meaningful assertion.Consider asserting the presence of the second spec (or
total >= 2) before using index1:$this->assertArrayHasKey(0, $specifications['body']['specifications']); + $this->assertArrayHasKey(1, $specifications['body']['specifications']); @@ - $function = $this->createFunction([ + $function = $this->createFunction([ 'functionId' => ID::unique(), 'name' => 'Specs function', 'runtime' => 'node-22', 'buildSpecification' => $specifications['body']['specifications'][0]['slug'], 'runtimeSpecification' => $specifications['body']['specifications'][1]['slug'], ]);tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
36-72: Add safety check before using$specifications[1]Like the functions test,
testListSpecsassumes a second spec exists:'runtimeSpecification' => $specifications['body']['specifications'][1]['slug'],but only asserts key
0exists. If specs are ever configured with a single entry, this will raise an undefined index.Consider asserting index
1as well:- $this->assertArrayHasKey(0, $specifications['body']['specifications']); + $this->assertArrayHasKey(0, $specifications['body']['specifications']); + $this->assertArrayHasKey(1, $specifications['body']['specifications']);
🧹 Nitpick comments (3)
src/Appwrite/Platform/Workers/Functions.php (1)
359-360: Execution now correctly usesruntimeSpecificationfor function resourcesUsing
$function->getAttribute('runtimeSpecification', APP_COMPUTE_SPECIFICATION_DEFAULT)to derive CPUs/memory for both the executor call and executions MB‑seconds metrics aligns execution resources with the new runtime‑specific field, distinct from build specs.If you ever want extra hardening against misconfigured specs, you could wrap the lookup with a fallback, e.g.:
$specs = Config::getParam('specifications', []); $key = $function->getAttribute('runtimeSpecification', APP_COMPUTE_SPECIFICATION_DEFAULT); $spec = $specs[$key] ?? $specs[APP_COMPUTE_SPECIFICATION_DEFAULT] ?? [ 'cpus' => APP_COMPUTE_CPUS_DEFAULT, 'memory' => APP_COMPUTE_MEMORY_DEFAULT, ];Also applies to: 608-610
src/Appwrite/Utopia/Response/Filters/V21.php (1)
11-30: Minor:parsecan avoid the unused$parsedResponsevariable
$parsedResponseis just$contentand is only used in thedefaultarm; it could be dropped and thedefaultarm can simply return$contentdirectly to reduce noise.- $parsedResponse = $content; - return match ($model) { @@ - Response::MODEL_FUNCTION_LIST => $this->handleList( + Response::MODEL_FUNCTION_LIST => $this->handleList( $content, "functions", fn ($item) => $this->parseFunction($item), ), - default => $parsedResponse, + default => $content, };tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
1292-1359: Spec update and negative‑validation coverage for sites looks good; message checks might be slightly brittleThis block correctly:
- Updates
runtimeSpecificationtwice and verifies the resulting env (indirectly via later flows).- Asserts 400s and message prefixes for invalid
buildSpecificationandruntimeSpecification.If error wording is expected to remain stable, the
assertStringStartsWithchecks are fine; if wording may evolve, you could relax them to just assert the parameter name is mentioned (e.g. contains"Invalid \buildSpecification` param"`).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/config/collections/projects.php(2 hunks)app/controllers/general.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php(4 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(2 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php(3 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(4 hunks)src/Appwrite/Platform/Workers/Functions.php(1 hunks)src/Appwrite/Utopia/Request/Filters/V21.php(2 hunks)src/Appwrite/Utopia/Response/Filters/V21.php(1 hunks)src/Appwrite/Utopia/Response/Model/Func.php(1 hunks)src/Appwrite/Utopia/Response/Model/Site.php(1 hunks)tests/e2e/General/UsageTest.php(1 hunks)tests/e2e/Services/Functions/FunctionsCustomServerTest.php(8 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(6 hunks)tests/resources/sites/astro/src/pages/specs.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T12:02:08.071Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Applied to files:
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
📚 Learning: 2025-09-18T07:42:08.734Z
Learnt from: atharvadeosthale
Repo: appwrite/appwrite PR: 10486
File: src/Appwrite/Utopia/Request/Filters/V21.php:25-33
Timestamp: 2025-09-18T07:42:08.734Z
Learning: In Appwrite's V21 request filter for backward compatibility, clients will never send both old (`version`) and new (`type`/`reference`) parameters simultaneously - they use either the old 1.8.0 format or the new 1.8.1+ format, not a mix of both.
Applied to files:
src/Appwrite/Utopia/Request/Filters/V21.phpsrc/Appwrite/Utopia/Response/Filters/V21.phpapp/controllers/general.php
🧬 Code graph analysis (8)
tests/resources/sites/astro/src/pages/specs.js (1)
src/Appwrite/Utopia/Response.php (1)
Response(159-983)
src/Appwrite/Utopia/Response/Model/Func.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (3)
getSite(177-185)createSite(107-115)updateSite(117-125)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
src/Appwrite/Utopia/Response/Model/Site.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
app/controllers/general.php (5)
src/Appwrite/Utopia/Response.php (2)
Response(159-983)addFilter(933-936)src/Appwrite/Utopia/Request/Filters/V20.php (1)
V20(12-203)src/Appwrite/Utopia/Response/Filters/V20.php (1)
V20(8-26)src/Appwrite/Utopia/Response/Filters/V21.php (1)
V21(9-51)src/Appwrite/Utopia/Request/Filters/V21.php (1)
V21(7-51)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (21)
app/config/collections/projects.php (1)
769-789: Dual spec attributes for functions and sites are wired consistently
buildSpecificationandruntimeSpecificationare defined with appropriate type/size/defaults and mirror each other acrossfunctionsandsites. This matches the new split-build/runtime model without introducing schema inconsistencies.Also applies to: 1203-1223
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
255-255: Build worker correctly switches tobuildSpecificationUsing
$resource->getAttribute('buildSpecification', APP_COMPUTE_SPECIFICATION_DEFAULT)for both the actual build (cpus/memory) and build-related MB‑seconds metrics keeps build resource usage aligned with the new field, independent from runtime specs.Also applies to: 1357-1357
tests/resources/sites/astro/src/pages/specs.js (1)
1-13: LGTM! Test endpoint correctly exposes runtime specifications.This serverless route provides a clean way to verify that runtime specification environment variables (APPWRITE_SITE_MEMORY and APPWRITE_SITE_CPUS) are correctly injected during site execution, supporting the test scenarios that validate the new dual-specification architecture.
tests/e2e/General/UsageTest.php (1)
942-943: LGTM! Test correctly validates the dual-specification approach.The test appropriately uses different specifications for build (S_8VCPU_8GB) and runtime (S_4VCPU_4GB), which provides good coverage for the new architecture where build and execution resources can be independently configured.
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
167-167: LGTM! Correctly uses runtimeSpecification for execution context.The change from
specificationtoruntimeSpecificationis semantically correct—function executions should consume runtime resource specifications rather than build specifications. This aligns with the architectural separation where builds and executions may require different resource profiles.app/controllers/general.php (3)
32-33: LGTM! Response filter imports added for backward compatibility.The new V20 and V21 response filters provide the necessary backward compatibility layer for the specification field changes, ensuring older clients continue to receive responses in the expected format.
310-310: LGTM! Correctly uses runtimeSpecification for execution.The router execution path correctly retrieves
runtimeSpecificationfrom the resource, consistent with the architectural principle that runtime executions should use runtime resource specs rather than build specs.
1041-1058: LGTM! Response filter version thresholds correctly ordered.The filter application logic correctly maps version ranges:
- V21 filter for clients < 1.9.0 (converts new dual-spec format to old single-spec)
- V20 filter for clients < 1.8.0 (handles related document selection changes)
- V19, V18, V17, V16 for progressively older versions
This maintains backward compatibility while allowing the codebase to use the new specification structure internally.
src/Appwrite/Utopia/Response/Model/Func.php (1)
179-190: LGTM! Model correctly reflects the dual-specification architecture.The model changes appropriately separate build-time and runtime resource specifications:
buildSpecification: Resources allocated for deployment buildsruntimeSpecification: Resources allocated for function executionsBoth fields use
APP_COMPUTE_SPECIFICATION_DEFAULTas the default value and include clear descriptions. The V21 response filter ensures backward compatibility by mapping these back to the singlespecificationfield for older clients.src/Appwrite/Utopia/Request/Filters/V21.php (2)
17-22: LGTM! Request filter correctly handles specification conversion for backward compatibility.The filter appropriately extends to handle functions/sites create/update operations, ensuring that 1.8.0 clients sending the old
specificationparameter have it properly converted to the new dual-specification format.
41-50: LGTM! Backward compatibility logic correctly duplicates specification.The
convertSpecsmethod correctly implements backward compatibility by setting bothbuildSpecificationandruntimeSpecificationto the same value when an old client provides thespecificationparameter. This preserves the 1.8.0 behavior where a single specification controlled both build and runtime resources.Based on learnings, clients will never send both old and new formats simultaneously, so this conversion is safe.
src/Appwrite/Utopia/Response/Model/Site.php (1)
164-175: LGTM! Site model correctly adopts dual-specification architecture.The model changes appropriately separate build and runtime specifications for sites:
buildSpecification: Resources for deployment buildsruntimeSpecification: Resources for SSR (server-side rendering) executionsThis mirrors the function model changes and provides the same flexibility to configure different resource profiles for builds versus runtime. The V21 response filter ensures backward compatibility with older clients.
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (3)
96-107: LGTM! Function creation API correctly adopts dual-specification parameters.The API now accepts separate
buildSpecificationandruntimeSpecificationparameters, each with appropriate descriptions, validators, and defaults. Both parameters correctly use thegetDefaultSpecification($plan)helper to determine sensible defaults based on the project plan.The descriptions clearly distinguish their purposes:
- buildSpecification: "Build specification for the function deployments."
- runtimeSpecification: "Runtime specification for the function executions."
145-146: LGTM! Action signature correctly updated for dual specifications.The method signature properly reflects the new parameters, replacing the single
$specificationwith separate$buildSpecificationand$runtimeSpecificationparameters, maintaining consistency with the updated API surface.
241-242: LGTM! Document correctly persists both specifications.The function document now stores both
buildSpecificationandruntimeSpecificationseparately, enabling independent control of build-time and runtime resource allocation. This is the foundation for the dual-specification architecture that propagates through the execution and build workflows.tests/e2e/Services/Functions/FunctionsCustomServerTest.php (3)
1365-1465: Runtime spec update + validation coverage looks solidThe
testUpdateSpecschanges correctly exercise updatingruntimeSpecification, verify the resulting CPU/memory env vars, and add negative tests for both invalidbuildSpecificationandruntimeSpecificationmessage formats. This should give good confidence that spec updates and validation errors are wired through end‑to‑end.
2044-2084: Response‑format matrix for spec fields matches new filter behaviorThe updated
testResponseFilterschecks:
- 1.5.0 format: neither
specificationnor the new spec fields are exposed.- 1.8.0 format: legacy
specificationis present,buildSpecification/runtimeSpecificationare hidden.- Latest format: new
buildSpecification/runtimeSpecificationare present andspecificationis hidden.This aligns with the new V21 response filter that maps internal build/runtime specs back to a single
specificationfor older formats, while exposing the split fields for modern clients.
2223-2264: Function spec propagation test is consistent with new split semantics
testFunctionSpecificationsnow clearly distinguishes build vs runtime spec:
- Build logs assert
2048:2frombuildSpecification=S_2VCPU_2GB.- Execution JSON asserts
APPWRITE_FUNCTION_MEMORY=1024,APPWRITE_FUNCTION_CPUS=1fromruntimeSpecification=S_1VCPU_1GB.This is a good, targeted regression test for the new dual‑spec behavior.
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (2)
92-103: Param wiring and persistence for build/runtime specs look consistent with existing patterns
- Both
buildSpecificationandruntimeSpecificationusegetDefaultSpecification($plan)for defaults andCompute\Validator\Specificationfor validation, with['plan']as a dependency, which matches how the old singlespecificationwas likely wired.- The
actionsignature has the new spec strings appended just before the injected dependencies, matching the order of the new->param(...)definitions.- The updated document now persists both fields:
'buildSpecification' => $buildSpecification, 'runtimeSpecification' => $runtimeSpecification,so they become part of the function’s canonical state.
Assuming
planis indeed provided for this route as it is for other compute endpoints, this setup looks correct.Also applies to: 134-135, 286-287
245-263: Cold‑start behavior on spec change is reasonable; 404 handling on runtime deletion is correctThe new block:
- Only runs when a
deploymentIdexists.- Compares the new
runtimeSpecificationandbuildSpecificationagainst the stored values and, if either differs, callsExecutor::deleteRuntime.- Swallows only 404s from
deleteRuntime(runtime not found) and rethrows all other errors, which aligns with the executor’s contract from the provided snippet.This will also trigger a cold start the first time an existing function is updated after migration (since its stored spec fields may be empty), which is a sensible trade‑off for ensuring the runtime matches the new spec.
src/Appwrite/Utopia/Response/Filters/V21.php (1)
44-50: Clarify which specification semantics apply for 1.8.x legacy clientsThe V21 filters show an asymmetry worth documenting:
Request filter (1.8→1.9): converts a single
specificationfield into bothbuildSpecificationandruntimeSpecificationResponse filter (1.9→1.8): exposes only
buildSpecificationas the legacyspecificationfieldModel definitions clarify the purposes:
buildSpecification: "Machine specification for deployment builds" (used in Builds.php)runtimeSpecification: "Machine specification for SSR executions" (used in general.php)These serve different concerns—build infrastructure vs. runtime behavior. A legacy 1.8.x client querying a function likely expects
specificationto reflect what affects runtime execution (SSR), not build deployment. If the current mapping tobuildSpecificationis intentional, a comment explaining why would help. Otherwise, consider mapping toruntimeSpecificationfor semantic correctness.
Feat: Custom start commands
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
3029-3078: Remove leftover var_dump from testSiteSpecifications
testSiteSpecificationscorrectly checks:
- Build logs contain the expected
APPWRITE_SITE_MEMORY:APPWRITE_SITE_CPUStuple for the chosen specs.- The
/specsroute exposesAPPWRITE_SITE_MEMORY=1024andAPPWRITE_SITE_CPUS=1for the runtime spec.However, the
\var_dump($deployment['body']['buildLogs']);inside theassertEventuallyclosure will spam test output and should be removed.- $this->assertEventually(function () use ($siteId, $deploymentId) { - $deployment = $this->getDeployment($siteId, $deploymentId); - \var_dump($deployment['body']['buildLogs']); - $this->assertStringContainsString('2048:2', $deployment['body']['buildLogs']); - }, 10000, 500); + $this->assertEventually(function () use ($siteId, $deploymentId) { + $deployment = $this->getDeployment($siteId, $deploymentId); + $this->assertStringContainsString('2048:2', $deployment['body']['buildLogs']); + }, 10000, 500);src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
246-263: Consider simplifying spec change detection.The
specsChangedlogic (lines 247-252) could be condensed to a single boolean expression for improved readability:- if (!empty($site->getAttribute('deploymentId'))) { - $specsChanged = false; - if ($site->getAttribute('runtimeSpecification') !== $runtimeSpecification) { - $specsChanged = true; - } elseif ($site->getAttribute('buildSpecification') !== $buildSpecification) { - $specsChanged = true; - } - - if ($specsChanged) { + if (!empty($site->getAttribute('deploymentId'))) { + $specsChanged = $site->getAttribute('runtimeSpecification') !== $runtimeSpecification + || $site->getAttribute('buildSpecification') !== $buildSpecification; + + if ($specsChanged) {The 404-tolerant error handling when deleting runtimes (lines 257-261) is good defensive coding.
92-97: Clarify description for sites context.The
runtimeSpecificationdescription at line 97 reads "Runtime specification for the function SSR executions." This should reference "site SSR executions" for consistency with the sites controller context.src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
88-93: Clarify description for sites context.The
runtimeSpecificationdescription at line 93 reads "Runtime specification for the function SSR executions." In the sites controller context, this should reference "site SSR executions" for clarity.
🧹 Nitpick comments (3)
tests/resources/sites/astro-custom-start-command/package.json (2)
7-7: Build script uses platform-specific commands.The build script uses
cpandrm -rf, which are Unix/Linux specific. On Windows systems without WSL or Git Bash, this will fail. Consider using cross-platform alternatives like Node utilities or a build tool for portability.For cross-platform compatibility, consider using a tool like
shxorcross-env, or convert the build steps to a small Node.js script. Example using Node:- "build": "astro build && cp custom-server.js dist/custom-server.js && rm -rf node_modules && npm install --production", + "build": "node scripts/build.js"Then create
scripts/build.jsto handle the build steps portably.
5-10: Unnecessary node_modules reinstallation during build.The build script removes
node_modulesand reinstalls production dependencies (line 7). While this pattern makes sense for deployment artifacts, it's unusual and potentially inefficient during a build process in a test environment. Verify this is intentional and necessary for the test scenario.If this pattern is needed only for deployment builds (not local test runs), consider splitting into separate scripts:
"scripts": { "dev": "astro dev", "build": "astro build && cp custom-server.js dist/custom-server.js", + "build:prod": "npm run build && rm -rf node_modules && npm install --production", "preview": "astro preview", "astro": "astro" }This allows flexibility for different build contexts.
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
36-52: Specs list & basic validation tests are aligned with new build/runtime fieldsThe new
testListSpecscorrectly:
- Creates a site with explicit
buildSpecificationandruntimeSpecificationpulled from the/specslist.- Verifies these fields on both create and subsequent get.
- Exercises validation for invalid spec slugs returning 400.
You might optionally add an assertion that index
1exists in the specs array before using it, to make the intent clearer and guard against single-spec configurations.Also applies to: 55-72
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/resources/sites/astro-custom-start-command/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
app/config/collections/projects.php(4 hunks)app/controllers/api/vcs.php(1 hunks)app/controllers/general.php(4 hunks)src/Appwrite/Platform/Modules/Compute/Base.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Deployments/Create.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Deployments/Duplicate/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Deployments/Template/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php(4 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php(2 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Duplicate/Create.php(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Template/Create.php(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php(6 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(7 hunks)src/Appwrite/Platform/Workers/Functions.php(2 hunks)src/Appwrite/Utopia/Response/Model/Site.php(2 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(6 hunks)tests/resources/sites/astro-custom-start-command/.gitignore(1 hunks)tests/resources/sites/astro-custom-start-command/astro.config.mjs(1 hunks)tests/resources/sites/astro-custom-start-command/custom-server.js(1 hunks)tests/resources/sites/astro-custom-start-command/package.json(1 hunks)tests/resources/sites/astro-custom-start-command/src/pages/index.astro(1 hunks)tests/resources/sites/astro-custom-start-command/src/pages/ssr.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/resources/sites/astro-custom-start-command/.gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:42:08.734Z
Learnt from: atharvadeosthale
Repo: appwrite/appwrite PR: 10486
File: src/Appwrite/Utopia/Request/Filters/V21.php:25-33
Timestamp: 2025-09-18T07:42:08.734Z
Learning: In Appwrite's V21 request filter for backward compatibility, clients will never send both old (`version`) and new (`type`/`reference`) parameters simultaneously - they use either the old 1.8.0 format or the new 1.8.1+ format, not a mix of both.
Applied to files:
app/controllers/general.php
🧬 Code graph analysis (5)
src/Appwrite/Utopia/Response/Model/Site.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (2)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
app/controllers/general.php (4)
src/Appwrite/Utopia/Response.php (2)
Response(159-983)addFilter(933-936)src/Appwrite/Utopia/Response/Filters/V20.php (1)
V20(8-26)src/Appwrite/Utopia/Response/Filters/V21.php (1)
V21(9-51)src/Appwrite/Utopia/Request.php (1)
addFilter(88-91)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (2)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (30)
tests/resources/sites/astro-custom-start-command/src/pages/ssr.js (1)
1-5: SSR test route is concise and appropriate
prerender = falseand theGEThandler returning a simple"SSR OK (...)"response withDate.now()are well-suited for validating SSR behavior and custom start-command wiring. No changes needed.tests/resources/sites/astro-custom-start-command/package.json (1)
12-13: Package versions verified as valid and stable.Web search confirms all three packages are available and actively maintained:
- @astrojs/node 9.5.1 published Nov 17, 2025
- express 5.1.0 is the v5 active release on npm
- astro 5.15.9 is published and fixes CVE-2025-65019
No version updates needed.
app/controllers/api/vcs.php (1)
307-307: LGTM!The
startCommandfield is correctly added to the deployment document, sourced from the resource's attribute with an appropriate empty string default. This aligns with the broader PR changes introducingstartCommandacross deployments.tests/resources/sites/astro-custom-start-command/src/pages/index.astro (1)
1-16: LGTM!This test resource file follows standard Astro conventions and provides a minimal test case for validating the custom start command functionality.
tests/resources/sites/astro-custom-start-command/astro.config.mjs (1)
1-10: LGTM!The Astro configuration correctly sets up the Node adapter in middleware mode, which is appropriate for testing custom server workflows with the new
startCommandfunctionality.src/Appwrite/Platform/Modules/Functions/Http/Deployments/Duplicate/Create.php (1)
107-107: LGTM!The
startCommandfield is correctly propagated to the duplicated deployment from the parent function. This ensures consistency when rebuilding deployments with updated configurations.src/Appwrite/Platform/Modules/Functions/Http/Deployments/Template/Create.php (1)
157-157: LGTM!The
startCommandfield is correctly included in template-based deployments, maintaining consistency with other deployment creation paths in the codebase.src/Appwrite/Platform/Modules/Sites/Http/Deployments/Duplicate/Create.php (1)
120-120: LGTM!The
startCommandfield is correctly propagated to the duplicated site deployment, ensuring that custom server configurations are preserved when rebuilding deployments.src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php (2)
254-254: LGTM!The
startCommandfield is correctly included when creating the initial deployment document after all chunks have been uploaded.
321-321: LGTM!The
startCommandfield is correctly included in the partial upload path, ensuring consistency regardless of whether the deployment is created in a single request or across multiple chunked uploads.src/Appwrite/Platform/Modules/Sites/Http/Deployments/Template/Create.php (1)
166-166: LGTM!The
startCommandfield is correctly included in site template-based deployments, completing the consistent propagation of this field across all deployment creation paths.src/Appwrite/Platform/Modules/Functions/Http/Deployments/Create.php (1)
246-247: startCommand propagation into deployments looks consistent
startCommandis correctly sourced from the function and stored on deployments in both initial and chunked-upload create paths, matching other deployment flows and leaving updates untouched.Also applies to: 284-285
src/Appwrite/Utopia/Response/Model/Site.php (2)
128-133: Site.startCommand field definition is coherentThe new
startCommandrule matches existing convention (string, empty default, example), and aligns with how deployments and controllers handle custom start commands.
170-181: buildSpecification/runtimeSpecification schema matches new compute modelRenaming
specificationtobuildSpecificationand addingruntimeSpecificationwith shared default/example based onAPP_COMPUTE_SPECIFICATION_DEFAULTfits the new separation of build vs runtime specs and matches how env vars are derived elsewhere.src/Appwrite/Platform/Workers/Functions.php (1)
359-360: Runtime spec + custom startCommand handling are aligned with new schemaUsing
runtimeSpecificationfor$specand preferring a deployment-levelstartCommand(with acd /usr/local/server/src/function/ && ...prefix) keeps execution behavior consistent with the HTTP execution path and the new spec fields without altering existing v2 behavior.Also applies to: 526-528
src/Appwrite/Platform/Modules/Compute/Base.php (1)
109-110: Persisting startCommand on VCS deployments is correctStoring
startCommandfrom the function/site on VCS deployments keeps the deployment schema uniform across manual/template/VCS flows and lets router/workers pick up custom runtime commands reliably.Also applies to: 205-207
tests/resources/sites/astro-custom-start-command/custom-server.js (1)
1-16: Custom server script matches test expectationsThe Express server cleanly wires static assets,
/ssr-customand SSR handler, plus a 500 fallback, which is exactly what the custom start-command test asserts against.src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
167-168: Execution path correctly uses runtimeSpecification and deployment startCommandUsing
runtimeSpecificationfor$specand honoring a non-empty deploymentstartCommand(with a working-directorycdprefix) keeps sync with worker and router behavior and ensures compute metrics and env vars reflect the runtime spec.Also applies to: 395-407
app/controllers/general.php (2)
309-311: Router now correctly uses runtimeSpecification and deployment startCommandBasing
$speconruntimeSpecification(not build spec) in the router’s execution path, and allowing deployments to overridestartCommand(while still deriving a framework/adapter-based default) matches the new separation of build vs runtime resources and the custom start-command contract for both functions and sites.Also applies to: 528-548
32-33: Updated response-format filters and thresholds look coherentImporting
ResponseV20/ResponseV21and applying them in descending version order (V21 for<1.9.0, then V20, V19, V18, V17, V16) is consistent with the existing request filters and lets V21 rewritebuildSpecification/runtimeSpecificationinto legacyspecificationbefore older filters run.Also applies to: 1045-1062
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
1293-1325: Site spec update and validation coverage is good
testUpdateSpecsnow:
- Updates
runtimeSpecificationto concrete constants and asserts persistence.- Confirms invalid
buildSpecificationandruntimeSpecificationvalues yield 400 with clear error messages.This nicely mirrors the functions-side spec tests and ensures the new validators behave as expected.
Also applies to: 1330-1344, 1345-1358
2970-3027: Custom startCommand SSR flow is well covered
testSiteCustomStartCommandthoroughly validates that:
- A site with
startCommand => 'node custom-server.js'deploys successfully.- The custom server serves static
/, dynamic/ssrand/ssr-customendpoints (with differing bodies on repeated calls due to timestamps).- The catch‑all error handler returns the expected 500/custom body.
This end-to-end test gives solid confidence in the new site-level
startCommandbehavior.src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (3)
96-107: LGTM! Dual-specification parameters are well-defined.The
buildSpecificationandruntimeSpecificationparameters are correctly configured with appropriate defaults, validation, and clear descriptions.
145-146: LGTM! Signature and persistence are correctly aligned.The action method signature and document creation both properly handle the new dual-specification fields.
Also applies to: 241-242
351-351: startCommand handling is correct and consistent across the codebase.The deployment retrieves
startCommandfrom the function usinggetAttribute('startCommand', ''), which correctly returns an empty string if not set. This pattern is used consistently across all deployment contexts (Sites, Functions, Compute modules), and execution code properly handles empty values with conditional checks before use. The function document doesn't need to explicitly set this optional field—the empty string default is correct and expected.src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
111-111: LGTM! Sites creation properly implements dual-spec fields.The action signature and document persistence correctly handle
startCommand,buildSpecification, andruntimeSpecification.Also applies to: 121-122, 162-162, 174-175
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (1)
275-275: LGTM! Site update correctly persists all new fields.The document update properly includes
startCommand,buildSpecification, andruntimeSpecification.Also applies to: 285-286
app/config/collections/projects.php (3)
769-801: LGTM! Functions collection schema properly defines new attributes.The schema correctly introduces:
startCommand(16384 chars, nullable) for custom start commandsbuildSpecification(128 chars, defaultAPP_COMPUTE_SPECIFICATION_DEFAULT) for build resourcesruntimeSpecification(128 chars, defaultAPP_COMPUTE_SPECIFICATION_DEFAULT) for execution resourcesThe inline comment about
startCommandbeing currently empty is helpful context.
1061-1071: LGTM! Sites collection schema mirrors functions structure.The
startCommand,buildSpecification, andruntimeSpecificationattributes are consistently defined with the same characteristics as the functions collection.Also applies to: 1228-1247
1405-1415: LGTM! Deployments collection correctly adds startCommand.The deployments collection appropriately includes only
startCommand(not the specification fields), which makes sense as deployments would reference their parent function/site for specification details.
✨ Benchmark results
⚡ Benchmark Comparison
|
Feat: Auto-delete deployments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
92-97: Description still references "function" instead of "site".The description "Runtime specification for the function SSR executions." should be updated to "Runtime specification for site SSR executions." to match the context.
248-254: Consider simplifying the specsChanged logic.The boolean expression can be condensed as previously suggested:
- $specsChanged = false; - if ($site->getAttribute('runtimeSpecification') !== $runtimeSpecification) { - $specsChanged = true; - } elseif ($site->getAttribute('buildSpecification') !== $buildSpecification) { - $specsChanged = true; - } + $specsChanged = $site->getAttribute('runtimeSpecification') !== $runtimeSpecification + || $site->getAttribute('buildSpecification') !== $buildSpecification;tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
3029-3078: Remove strayvar_dumpfromtestSiteSpecificationsThe test correctly checks that build specs propagate into build logs (
2048:2) and that runtime specs appear in the/specsresponse. However, the\var_dump($deployment['body']['buildLogs']);insideassertEventuallywill spam test output and should be removed before merging. The assertion onbuildLogsis sufficient on its own.- $this->assertEventually(function () use ($siteId, $deploymentId) { - $deployment = $this->getDeployment($siteId, $deploymentId); - \var_dump($deployment['body']['buildLogs']); - $this->assertStringContainsString('2048:2', $deployment['body']['buildLogs']); - }, 10000, 500); + $this->assertEventually(function () use ($siteId, $deploymentId) { + $deployment = $this->getDeployment($siteId, $deploymentId); + $this->assertStringContainsString('2048:2', $deployment['body']['buildLogs']); + }, 10000, 500);src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
88-93: Misleading description for runtimeSpecification in sites context.The description still reads "Runtime specification for the function SSR executions." which is misleading for sites. As noted in the past review, this should reference "site SSR executions" instead.
🧹 Nitpick comments (5)
tests/e2e/Services/Sites/SitesConsoleClientTest.php (1)
144-196: Retention E2E for sites looks solid; consider tightening assertionsThe flow (set
deploymentRetention, create 3 deployments, time‑travel one, trigger maintenance, assert total=2) correctly exercises the new retention path. To make the test more robust, you could also assert that the active deployment ID is still present and the old one is gone (e.g., by checking the returned deployment IDs), but this is optional.tests/e2e/Services/Functions/FunctionsConsoleClientTest.php (1)
542-594: Function deployment retention test matches maintenance behavior wellThe test cleanly validates retention for functions (3 deployments, time‑travel one, trigger maintenance, assert 2 remain). As with the site test, you could optionally assert which deployment IDs remain to guarantee the active deployment is preserved, but the current check is functionally adequate.
src/Appwrite/Platform/Workers/Deletes.php (1)
67-69: Old deployment cleanup wiring looks correct; verifyresourceTypesemantics and consider minor tuning
- Wiring: Injecting
queueForDeletesand passing it intoaction(...), then callingdeleteOldDeploymentsfrom theDELETE_TYPE_MAINTENANCEbranch is consistent with the existing delete flow. UsingdeleteByGroupplus queuingDELETE_TYPE_DOCUMENTevents for each stale deployment correctly reuses the existing per‑deployment cleanup logic.- Semantics:
deleteOldDeploymentsmatches deployments byresourceInternalIdandresourceType = $resource->getCollection(). This assumes thatdeployments.resourceTypestores collection names like'functions'/'sites'. Please double‑check the deployments schema and any writers to ensure that’s true; if it instead uses singular values (e.g.'function'/'site'), retention would silently skip deletions.- Behavior: Treating
deploymentRetention === 0as “unlimited” and computing the cutoff ascreatedBefore(now - retentionDays)is aligned with the tests for values 0 and 180.- Optional: To reduce work on large projects, you could add a simple query filter when listing resources (e.g., only resources with
deploymentRetention > 0) so you don’t scan functions/sites that can’t ever trigger cleanup.Also applies to: 90-92, 188-196, 315-368
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
36-47: Specs tests correctly exercise build/runtimeSpecification; message asserts may be a bit brittle
testListSpecsandtestUpdateSpecsnow validate thatbuildSpecificationandruntimeSpecificationare accepted, persisted, and rejected when invalid, which is exactly what’s needed for the new schema.- Minor robustness nit: you rely on
$specifications['body']['specifications'][1]without asserting that index 1 exists; adding anassertArrayHasKey(1, ...)would make failures clearer if the spec list ever shrinks.- The
assertStringStartsWithchecks on error messages are fine but somewhat fragile; if the validator text changes or gets localized, these tests will break even though behavior is still correct. As a future tweak, you could assert on a shorter, stable substring (e.g., just'InvalidbuildSpecificationparam').Also applies to: 55-72, 1274-1359
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (1)
247-265: Cold start enforcement logic is correct; consider minor simplification.The logic correctly enforces a cold start when specifications change. The if/elseif pattern works, but could be simplified for readability:
Consider this simpler approach:
- if (!empty($function->getAttribute('deploymentId'))) { - $specsChanged = false; - if ($function->getAttribute('runtimeSpecification') !== $runtimeSpecification) { - $specsChanged = true; - } elseif ($function->getAttribute('buildSpecification') !== $buildSpecification) { - $specsChanged = true; - } - - if ($specsChanged) { + if (!empty($function->getAttribute('deploymentId'))) { + $specsChanged = $function->getAttribute('runtimeSpecification') !== $runtimeSpecification + || $function->getAttribute('buildSpecification') !== $buildSpecification; + + if ($specsChanged) { try {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/config/collections/projects.php(6 hunks)app/controllers/mock.php(2 hunks)app/init/constants.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php(5 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php(6 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php(6 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(7 hunks)src/Appwrite/Platform/Tasks/Maintenance.php(4 hunks)src/Appwrite/Platform/Workers/Deletes.php(5 hunks)src/Appwrite/Utopia/Response/Model/Func.php(2 hunks)src/Appwrite/Utopia/Response/Model/Site.php(3 hunks)tests/e2e/Services/Functions/FunctionsConsoleClientTest.php(2 hunks)tests/e2e/Services/Functions/FunctionsCustomServerTest.php(9 hunks)tests/e2e/Services/Sites/SitesConsoleClientTest.php(2 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/Services/Functions/FunctionsCustomServerTest.php
- src/Appwrite/Utopia/Response/Model/Func.php
🧰 Additional context used
🧬 Code graph analysis (10)
tests/e2e/Services/Sites/SitesConsoleClientTest.php (2)
tests/e2e/Services/Sites/SitesBase.php (3)
setupSite(21-34)packageSite(239-251)cleanupSite(85-94)tests/e2e/Client.php (1)
Client(8-342)
tests/e2e/Services/Functions/FunctionsConsoleClientTest.php (2)
tests/e2e/Services/Functions/FunctionsBase.php (2)
setupFunction(20-33)cleanupFunction(69-78)tests/e2e/Client.php (1)
Client(8-342)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (5)
getSite(177-185)createSite(107-115)updateSite(117-125)setupSite(21-34)cleanupSite(85-94)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (2)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
src/Appwrite/Platform/Workers/Deletes.php (2)
src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Platform/Modules/Functions/Http/Functions/Delete.php (1)
Delete(21-100)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (2)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
src/Appwrite/Utopia/Response/Model/Site.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
app/config/collections/projects.php (1)
src/Appwrite/Utopia/Response/Model/Database.php (1)
Database(8-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (23)
app/init/constants.php (1)
88-88: LGTM!The constant provides a sensible upper bound for deployment retention. The value correctly computes to 36,500 days (~100 years).
app/controllers/mock.php (1)
202-231: Endpoint structure and guards are correctly implemented.The development-only guard, project validation, and resourceType whitelist mapping are all properly implemented. The endpoint will be useful for testing retention-related functionality.
src/Appwrite/Utopia/Response/Model/Site.php (3)
61-66: LGTM!The
deploymentRetentionrule is correctly defined with appropriate type, default, and example values.
134-139: LGTM!The
startCommandrule is correctly defined for custom site runtime commands.
176-187: LGTM!The split of
specificationintobuildSpecificationandruntimeSpecificationis correctly implemented with appropriate descriptions distinguishing between deployment builds and SSR executions.src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
256-266: Runtime deletion on spec change is correctly implemented.The 404-tolerant error handling ensures graceful behavior when the runtime doesn't exist, and the runtime is properly deleted when specifications change to prevent stale resource limits.
268-294: LGTM!The document update correctly persists all new fields (
deploymentRetention,startCommand,buildSpecification,runtimeSpecification) to the site.app/config/collections/projects.php (5)
570-580: LGTM!The
deploymentRetentionattribute for functions is correctly defined with appropriate type and default value of 0.
779-812: LGTM!The new attributes (
startCommand,buildSpecification,runtimeSpecification) for functions are correctly defined. The comment on line 780 provides useful context aboutstartCommandnot being supported by runtimes yet.
1072-1104: LGTM!The
startCommandanddeploymentRetentionattributes for sites are correctly defined and consistent with the functions collection schema.
1248-1269: LGTM!The
buildSpecificationandruntimeSpecificationattributes for sites are correctly defined with consistent sizing and defaults matching the functions collection.
1427-1437: LGTM!The
startCommandattribute for deployments is correctly defined, allowing the command to be captured at deployment time.src/Appwrite/Platform/Tasks/Maintenance.php (1)
16-17: Newtypeparameter cleanly enables one‑off vs looped maintenanceAdding the
typeWhiteList param with default'loop'and branching betweenConsole::loopand a direct$action()preserves existing behavior while enabling manual--type=triggerruns (used by the new tests). The refactor into an$actionclosure keeps the logic centralized and side‑effect free.Also applies to: 25-35, 37-39, 62-109
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
2970-3027: Custom start command E2E coverage looks good
testSiteCustomStartCommandnicely validates that a customstartCommandworks for SSR routes (including dynamic responses viaDate.now()), serves custom SSR paths, and surfaces custom errors for invalid routes. No issues spotted.
3080-3207: Deployment‑retention validation for sites is comprehensive
testSiteDeploymentRetentionthoroughly covers default behavior, valid values (0, 180), invalid values (too large, negative), and update flows (changing, clearing, and rejecting bad updates). This gives strong confidence that the newdeploymentRetentioncontract on sites is enforced end‑to‑end.src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (3)
72-72: LGTM: startCommand parameter added correctly.The parameter is properly validated and optional, with a clear description.
94-94: LGTM: deploymentRetention parameter added correctly.The parameter uses appropriate validation with a clear description of the retention behavior.
112-112: LGTM: New parameters correctly threaded through action signature and document creation.All new parameters (startCommand, buildSpecification, runtimeSpecification, deploymentRetention) are properly added to the action signature and persisted in the site document.
Also applies to: 122-124, 159-159, 165-165, 177-178
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (3)
96-112: LGTM: Specification parameters added correctly for functions.The buildSpecification, runtimeSpecification, and deploymentRetention parameters are properly defined with appropriate validators and descriptions for the functions context.
146-147: LGTM: New parameters correctly integrated into function creation.All new parameters are properly added to the action signature and persisted in the function document.
Also applies to: 152-152, 223-223, 244-245
354-354: LGTM: startCommand correctly added to backwards-compatible deployment creation.The startCommand field is appropriately sourced from the function attribute with a sensible default.
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (2)
92-104: LGTM: Specification parameters added correctly for function updates.The parameters are consistently defined with the same validators and defaults as the create endpoint.
135-137: LGTM: New parameters correctly integrated into function update.All new parameters are properly added to the action signature and persisted in the function document update.
Also applies to: 280-280, 289-290
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/init/constants.php (1)
93-93: The constant's unit should be clarified through better naming or documentation.The constant
APP_COMPUTE_DEPLOYMENT_MAX_RETENTIONuses days as the unit (confirmed by consuming code that documents the parameter as "Days to keep non-active deployments"), but the constant name itself doesn't indicate this. While the calculation100 * 365and comment "100 years" imply days, the constant would benefit from explicit unit designation for self-documenting code.Consider either:
- Renaming to
APP_COMPUTE_DEPLOYMENT_MAX_RETENTION_DAYS, or- Updating the comment to
// 100 years (in days)Suggested improvements
-const APP_COMPUTE_DEPLOYMENT_MAX_RETENTION = 100 * 365; // 100 years +const APP_COMPUTE_DEPLOYMENT_MAX_RETENTION_DAYS = 100 * 365; // 100 yearsOr:
-const APP_COMPUTE_DEPLOYMENT_MAX_RETENTION = 100 * 365; // 100 years +const APP_COMPUTE_DEPLOYMENT_MAX_RETENTION = 100 * 365; // 100 years (in days)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/api/vcs.phpapp/controllers/general.phpapp/controllers/mock.phpapp/init/constants.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/mock.php
- app/controllers/api/vcs.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:42:08.734Z
Learnt from: atharvadeosthale
Repo: appwrite/appwrite PR: 10486
File: src/Appwrite/Utopia/Request/Filters/V21.php:25-33
Timestamp: 2025-09-18T07:42:08.734Z
Learning: In Appwrite's V21 request filter for backward compatibility, clients will never send both old (`version`) and new (`type`/`reference`) parameters simultaneously - they use either the old 1.8.0 format or the new 1.8.1+ format, not a mix of both.
Applied to files:
app/controllers/general.php
🧬 Code graph analysis (1)
app/controllers/general.php (3)
src/Appwrite/Utopia/Request/Filters/V20.php (1)
V20(12-203)src/Appwrite/Utopia/Response/Filters/V20.php (1)
V20(8-26)src/Appwrite/Utopia/Response/Filters/V21.php (1)
V21(9-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (4)
app/controllers/general.php (4)
32-33: LGTM!The new imports for ResponseV20 and ResponseV21 are correctly added to support the new response format filtering logic.
312-312: LGTM!The change from
specificationtoruntimeSpecificationcorrectly aligns with the schema split. UsingruntimeSpecificationfor compute specs during execution is semantically appropriate.
957-974: LGTM!The response filters are correctly ordered from newest to oldest version thresholds. V21 handles the
buildSpecification/runtimeSpecification→specificationtransformation for clients requesting < 1.9.0 format, and V20 handles document transformations for < 1.8.0. This ordering ensures proper filter chaining for backward compatibility.
542-544: Path consistency confirmed across the codebase.The hardcoded
/usr/local/server/src/function/path is used consistently in three locations: the code under review (app/controllers/general.php:543), src/Appwrite/Platform/Workers/Functions.php:538, and src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php:408. All instances use the identical pattern with the&&operator to ensure the startCommand only executes if the directory change succeeds.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/init/constants.php (1)
90-95: Deployment retention cap wiring looks correct
APP_COMPUTE_DEPLOYMENT_MAX_RETENTION = 100 * 365aligns with the “days” semantics used in the Sites deploymentRetention params and is safely within integer bounds. If you expect to reuse this in other contexts, consider clarifying the unit in the comment (e.g., “100 years in days”) to avoid ambiguity.app/config/collections/common.php (1)
1291-1301: Team labels schema matches existing user labels shapeThe new
teams.labelsattribute mirrorsusers.labels(string, size 128, array, no filters), which keeps the schema consistent. If you later introduce queries filtered by team labels, you may want to consider adding an index at that point, but current definition is fine for a first iteration of the feature.tests/e2e/Services/Sites/SitesCustomServerTest.php (4)
36-72: Guard against specs array shape before indexingThe updated
testListSpecscorrectly exercises bothbuildSpecificationandruntimeSpecification, including invalid slugs. You’re directly accessingspecifications[1]['slug']though, while only asserting that index0exists. It would be slightly safer to also assert thattotal >= 2(or that index1exists) before using it to avoid brittle failures if the specs list ever shrinks to a single entry.
1292-1359: Spec update tests look good but are tightly coupled to error textThe success-path assertions for changing
runtimeSpecificationand the failure cases for invalidbuildSpecification/runtimeSpecificationare well aligned with the new API. The negative tests currently assert the full prefix of the error message string; if the validator wording changes, these will start failing even though behavior is correct. Consider either:
- Asserting only on the status code and a shorter, stable substring, or
- Adding explicit tests for the validator’s message in a more focused unit test, and keeping e2e assertions looser.
2968-3025: Custom start command e2e is strong; dynamic-body checks could be fragileThe
testSiteCustomStartCommanddoes a nice job asserting that a customstartCommand-driven SSR server:
- Serves
/and/ssrcorrectly,- Produces non-static responses over time (via
Date.now()),- Handles
/non-existingwith a custom 500 error body.The
assertNotEquals($originalBody, $response['body'])checks rely on the implementation continuing to embed a changing timestamp. If this ever stops varying (or gains caching), the test will fail even if the command wiring is still correct. If you want to future‑proof it, you could instead assert on a stable pattern (e.g., presence of atimestampfield) rather than full-body inequality.
3077-3205: Deployment retention test coverage is solid; consider boundary checks
testSiteDeploymentRetentionthoroughly covers:
- Default behavior (omitted →
0).- Valid explicit values (
0,180) on create and update.- Invalid values (
-1,999999) mapping to HTTP 400.- Update semantics (changing from 180 → 90, then resetting to 0 when omitted).
Given the max is
APP_COMPUTE_DEPLOYMENT_MAX_RETENTION(100 * 365), you could add quick edge tests at exactly1andAPP_COMPUTE_DEPLOYMENT_MAX_RETENTIONto pin the inclusive range behavior, but the current set already exercises the validator reasonably well.src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (1)
248-268: Spec-change runtime teardown logic is correct but can be simplifiedThe
specsChangedblock:
- Properly guards on existing
deploymentId.- Uses a dual-read of
runtimeSpecification/buildSpecificationfalling back to legacyspecification, which preserves backward compatibility.- Deletes the runtime via
Executor::deleteRuntime, swallowing only 404s.Functionally this is solid. If you want to simplify, you could collapse the two branches into a single boolean expression, e.g.:
$specsChanged = $site->getAttribute('runtimeSpecification', $site->getAttribute('specification')) !== $runtimeSpecification || $site->getAttribute('buildSpecification', $site->getAttribute('specification')) !== $buildSpecification;and then check
$specsChangedonce.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/resources/sites/astro-custom-start-command/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
app/config/collections/common.phpapp/controllers/mock.phpapp/init/constants.phpsrc/Appwrite/Platform/Modules/Sites/Http/Sites/Create.phpsrc/Appwrite/Platform/Modules/Sites/Http/Sites/Update.phpsrc/Appwrite/Platform/Workers/Deletes.phptests/e2e/Services/Sites/SitesCustomServerTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/mock.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-13T12:02:08.071Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Applied to files:
src/Appwrite/Platform/Workers/Deletes.php
🧬 Code graph analysis (4)
src/Appwrite/Platform/Workers/Deletes.php (2)
src/Appwrite/Event/Event.php (1)
Event(10-678)src/Appwrite/Platform/Modules/Functions/Http/Functions/Delete.php (1)
Delete(21-100)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (7)
getSite(177-185)createSite(107-115)updateSite(117-125)setupSite(21-34)setupSiteDomain(369-387)setupDeployment(36-83)getDeployment(187-195)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (3)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (9)
src/Appwrite/Platform/Workers/Deletes.php (4)
8-8: LGTM: DeleteEvent injection follows standard patterns.The dependency injection for
queueForDeletesis implemented correctly and follows the same pattern used elsewhere in the codebase for delete event handling.Also applies to: 67-67, 92-92
910-910: Past review concern addressed:resourceTypevalues now use plural forms.These lines now use plural
resourceTypevalues ('sites' and 'functions'), which:
- Matches what
$resource->getCollection()returns at line 335- Aligns with the existing code at lines 1174-1178 that expects plural forms
- Creates consistency across the codebase
This appears to address the critical issue raised in the previous review about singular/plural mismatch. The verification script in the previous comment will confirm the deployment documents actually store plural values in the database.
Also applies to: 998-998
193-197: LGTM: Maintenance workflow properly integrates deployment retention cleanup.The new
deleteOldDeploymentscall is correctly integrated into the maintenance workflow alongside other cleanup tasks. The parameter ordering for bothdeleteAuditLogsanddeleteOldDeploymentsis consistent with their respective method signatures.
317-370: Code is correct as written.The retention cleanup logic properly handles unlimited retention (0 = never delete), correctly calculates the cutoff date in seconds, safely excludes active deployments, and uses the appropriate
resourceTypevalues for querying. Since$resource->getCollection()returns plural forms ('functions', 'sites') which match howresourceTypeis stored in deployment documents, the queries will execute correctly.tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
3027-3075: Specification propagation test is well‑designed
testSiteSpecificationsnicely validates both sides of the spec split:
- Build side:
buildSpecification = S_2VCPU_2GBproducing2048:2inbuildLogsvia$APPWRITE_SITE_MEMORY:$APPWRITE_SITE_CPUS.- Runtime side:
runtimeSpecification = S_1VCPU_1GBreflected by/specsreturningAPPWRITE_SITE_MEMORY=1024andAPPWRITE_SITE_CPUS=1.This gives good confidence that build and runtime specs are independently honored across deployments and executions.
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
70-101: Create-site wiring for startCommand/specs/retention is consistentThe new parameters are wired cleanly:
startCommandis optional, stored as‑is, and defaults to empty string (allowing downstream “use default” behavior).buildSpecification/runtimeSpecificationboth usegetDefaultSpecification($plan)and theSpecificationvalidator with['plan']injection, matching the compute module’s design.deploymentRetentionis constrained viaRange(0, APP_COMPUTE_DEPLOYMENT_MAX_RETENTION)and persisted on the site document.This matches the new tests and centralizes the retention cap through the constant, which is good for future reuse.
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (3)
70-109: Update endpoint param additions align with create semanticsAdding
startCommand,buildSpecification,runtimeSpecification, anddeploymentRetentionto the update endpoint mirrors the create path and keeps validation consistent (sameSpecification/Rangeusage and docstrings). This ensures clients can fully manage these fields post‑creation without special cases.
236-247: Including startCommand in “live” detection is appropriateThe updated “live vs needs redeploy” check now factors in
startCommandalongside name/build/install/output/framework changes. That’s important for SSR/custom-server sites where a start command change should force a new deployment, so the behavior is correct.
270-296: Update persistence of new fields is consistentThe update payload correctly persists:
deploymentRetentionalongside other core settings.startCommandso future builds/SSR use the new command.buildSpecificationandruntimeSpecificationto keep the site document in sync with the compute specs.This matches the behavior exercised in the new e2e tests (
testSiteSpecifications,testSiteDeploymentRetention), so the wiring looks complete.
There was a problem hiding this comment.
lgtm on a quick glance 👍🏻
| const APP_COMPUTE_CPUS_DEFAULT = 0.5; | ||
| const APP_COMPUTE_MEMORY_DEFAULT = 512; | ||
| const APP_COMPUTE_SPECIFICATION_DEFAULT = Specification::S_1VCPU_512MB; | ||
| const APP_COMPUTE_DEPLOYMENT_MAX_RETENTION = 100 * 365; // 100 years |
There was a problem hiding this comment.
Shouldn't this be unlimited by default? We should take care for future generations. With this every app will break by 2126 :)
| // Don't throw if the deployment doesn't exist | ||
| if ($th->getCode() !== 404) { | ||
| throw $th; | ||
| if (!empty($function->getAttribute('deploymentId'))) { |
There was a problem hiding this comment.
When do we plan to remove all these dual read/writes?
app/controllers/mock.php
Outdated
| ->dynamic($key, Response::MODEL_KEY); | ||
| }); | ||
|
|
||
| App::post('/v1/mock/time-travels') |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ting doc: Fix typos in CONTRIBUTING.md
|
@copilot code review[agent] review entire PR please |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/config/collections/projects.php (1)
1248-1291:⚠️ Potential issue | 🔴 CriticalCritical: Duplicate attribute definitions for
buildSpecificationandruntimeSpecification.The
sitescollection has each attribute defined twice:
buildSpecification: defined at lines 1250 and 1272runtimeSpecification: defined at lines 1261 and 1283Duplicate
$idvalues in the same collection will cause schema migration errors or undefined behavior. Thefunctionscollection correctly defines each attribute only once.🐛 Proposed fix: Remove duplicate attribute definitions
[ 'array' => false, '$id' => ID::custom('runtimeSpecification'), 'type' => Database::VAR_STRING, 'format' => '', 'size' => 128, 'signed' => false, 'required' => false, 'default' => APP_COMPUTE_SPECIFICATION_DEFAULT, 'filters' => [], ], - [ - 'array' => false, - '$id' => ID::custom('buildSpecification'), - 'type' => Database::VAR_STRING, - 'format' => '', - 'size' => 128, - 'signed' => false, - 'required' => false, - 'default' => APP_COMPUTE_SPECIFICATION_DEFAULT, - 'filters' => [], - ], - [ - 'array' => false, - '$id' => ID::custom('runtimeSpecification'), - 'type' => Database::VAR_STRING, - 'format' => '', - 'size' => 128, - 'signed' => false, - 'required' => false, - 'default' => APP_COMPUTE_SPECIFICATION_DEFAULT, - 'filters' => [], - ], [ '$id' => ID::custom('buildRuntime'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/config/collections/projects.php` around lines 1248 - 1291, The projects collection contains duplicate attribute blocks for ID::custom('buildSpecification') and ID::custom('runtimeSpecification'); remove the duplicated entries so each $id appears only once (keep one canonical definition for buildSpecification and one for runtimeSpecification), ensure the remaining attribute definitions preserve the intended settings (type Database::VAR_STRING, size 128, default APP_COMPUTE_SPECIFICATION_DEFAULT, filters array) and adjust surrounding array structure if needed to maintain valid schema ordering and indexes.
♻️ Duplicate comments (2)
app/controllers/mock.php (1)
256-259:⚠️ Potential issue | 🟡 MinorUse
GENERAL_NOT_FOUNDfor missing resource.The exception type
GENERAL_ARGUMENT_INVALIDis semantically incorrect for a "Resource not found" scenario. It implies the argument format is invalid rather than the resource doesn't exist.Proposed fix
$resource = $dbForProject->getDocument($collection, $resourceId); if ($resource->isEmpty()) { - throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Resource not found'); + throw new Exception(Exception::GENERAL_NOT_FOUND, 'Resource not found'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/mock.php` around lines 256 - 259, The thrown exception for a missing resource uses the wrong error code; change the Exception raised after $dbForProject->getDocument($collection, $resourceId) when $resource->isEmpty() to use Exception::GENERAL_NOT_FOUND and keep the existing message ("Resource not found") so callers receive the correct "not found" semantics instead of GENERAL_ARGUMENT_INVALID.app/controllers/general.php (1)
557-559:⚠️ Potential issue | 🔴 Critical
startCommandis still shell-injection prone when embedded later.On Line 558,
startCommandis concatenated from deployment input and later injected into a quoted shell command (Line 563). A payload containing quotes or shell metacharacters can break command boundaries.Suggested hardening
- $runtimeEntrypoint = match ($version) { - 'v2' => '', - default => "cp /tmp/code.$extension /mnt/code/code.$extension && nohup helpers/start.sh \"$startCommand\"", - }; + $runtimeEntrypoint = match ($version) { + 'v2' => '', + default => "cp /tmp/code.$extension /mnt/code/code.$extension && nohup helpers/start.sh " . \escapeshellarg($startCommand), + };#!/bin/bash # Verify whether startCommand is validated/escaped across routing + worker execution paths. rg -n -C3 '\bstartCommand\b' app/controllers/general.php src/Appwrite/Platform/Modules src/Appwrite/Platform/Workers rg -n -C3 'helpers/start\.sh|runtimeEntrypoint|escapeshellarg|escapeshellcmd' app/controllers/general.php src/Appwrite/Platform/Workers src/Executor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/general.php` around lines 557 - 559, The $startCommand built from $deployment->getAttribute('startCommand', '') is vulnerable to shell injection when later embedded into a quoted shell invocation; sanitize or reject unsafe input before concatenation: validate against a whitelist of allowed commands/arguments or, if free-form is required, escape the value with a proper shell-escaping function (e.g., use escapeshellarg for arguments or escapeshellcmd for whole commands) before assigning to $startCommand so the later quoted shell usage cannot break out; locate the assignment of $startCommand and the later injection site where it is executed and apply escaping/validation at the source (the $deployment->getAttribute(...) usage) and again immediately before execution to be safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 207: The sentence "If there are multiple resources, use them all in
folder structure" is missing the article "the"; update that text (the snippet
"use them all in folder structure") to read "use them all in the folder
structure" to improve readability.
- Line 114: The sentence "To set up a working **development environment**, just
fork the project git repository and install the backend and frontend
dependencies using the proper package manager and run the docker-compose stack."
is hard to parse; rewrite it into a clearer, comma-separated sequence such as
"fork the repository, install backend and frontend dependencies using the
appropriate package managers, and run the Docker Compose stack" so the actions
(fork, install dependencies, run docker-compose) are distinct and parallel;
update the CONTRIBUTING.md sentence accordingly.
- Around line 398-399: Rewrite the ambiguous sentence into two clear sentences:
first describe the three metric scopes as "daily, monthly, and infinity"
(lowercase) and then explain that adding a new usage metric is simple but
depends on whether statistics are collected via the API or a background worker;
finally instruct readers to add a const variable named using the
METRIC_<RESOURCE_NAME> convention in app/init.php under the usage metrics list.
- Line 192: Fix the grammar and tighten the phrasing in the modules paragraph
that currently starts "As Appwrite grows, we noticed approach of having all
service endpoints...": change it to something like "As Appwrite grows, we
noticed that the approach of having all service endpoints in
`app/controllers/api/[service].php` is not maintainable." Also restructure the
following sentence to be more concise and grammatical (e.g., "Not only does this
create massive files, it also omits product features such as workers and tasks;
while some controller files remain, we avoid this pattern in new development and
are gradually migrating existing controllers to HTTP modules.") to replace the
original paragraph.
- Around line 176-181: Update the container naming paragraph to use plural
pronoun agreement by replacing "depending on its intended use" with "depending
on their intended use" in the container naming section; ensure the lines that
list `appwrite-worker-X`, `appwrite-task-X` and the example `redis` remain
unchanged except for this pronoun fix.
- Line 664: Update the sentence explaining Appwrite Functions preview domains to
correct the grammar: change "This domain has format
`[SOMETHING].functions.localhost` unless you changed `_APP_DOMAIN_FUNCTIONS`
environment variable." to include the article and contraction, e.g. "This domain
has the format `[SOMETHING].functions.localhost` unless you’ve changed the
`_APP_DOMAIN_FUNCTIONS` environment variable." Ensure the
`_APP_DOMAIN_FUNCTIONS` token and the example domain
`[SOMETHING].functions.localhost` remain unchanged.
---
Outside diff comments:
In `@app/config/collections/projects.php`:
- Around line 1248-1291: The projects collection contains duplicate attribute
blocks for ID::custom('buildSpecification') and
ID::custom('runtimeSpecification'); remove the duplicated entries so each $id
appears only once (keep one canonical definition for buildSpecification and one
for runtimeSpecification), ensure the remaining attribute definitions preserve
the intended settings (type Database::VAR_STRING, size 128, default
APP_COMPUTE_SPECIFICATION_DEFAULT, filters array) and adjust surrounding array
structure if needed to maintain valid schema ordering and indexes.
---
Duplicate comments:
In `@app/controllers/general.php`:
- Around line 557-559: The $startCommand built from
$deployment->getAttribute('startCommand', '') is vulnerable to shell injection
when later embedded into a quoted shell invocation; sanitize or reject unsafe
input before concatenation: validate against a whitelist of allowed
commands/arguments or, if free-form is required, escape the value with a proper
shell-escaping function (e.g., use escapeshellarg for arguments or
escapeshellcmd for whole commands) before assigning to $startCommand so the
later quoted shell usage cannot break out; locate the assignment of
$startCommand and the later injection site where it is executed and apply
escaping/validation at the source (the $deployment->getAttribute(...) usage) and
again immediately before execution to be safe.
In `@app/controllers/mock.php`:
- Around line 256-259: The thrown exception for a missing resource uses the
wrong error code; change the Exception raised after
$dbForProject->getDocument($collection, $resourceId) when $resource->isEmpty()
to use Exception::GENERAL_NOT_FOUND and keep the existing message ("Resource not
found") so callers receive the correct "not found" semantics instead of
GENERAL_ARGUMENT_INVALID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9abf3ea-9e10-42bc-b627-bf632162d4a0
📒 Files selected for processing (5)
CONTRIBUTING.mdapp/config/collections/projects.phpapp/controllers/general.phpapp/controllers/mock.phpapp/init/constants.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/init/constants.php
| ## Setup From Source | ||
|
|
||
| To set up a working **development environment**, just fork the project git repository and install the backend and frontend dependencies using the proper package manager and create run the docker-compose stack. | ||
| To set up a working **development environment**, just fork the project git repository and install the backend and frontend dependencies using the proper package manager and run the docker-compose stack. |
There was a problem hiding this comment.
Clarify setup sentence structure.
Line 114 is still hard to parse (fork the project git repository ... and run ...). Consider rewriting to: “fork the repository, install backend/frontend dependencies, and run the Docker Compose stack.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` at line 114, The sentence "To set up a working **development
environment**, just fork the project git repository and install the backend and
frontend dependencies using the proper package manager and run the
docker-compose stack." is hard to parse; rewrite it into a clearer,
comma-separated sequence such as "fork the repository, install backend and
frontend dependencies using the appropriate package managers, and run the Docker
Compose stack" so the actions (fork, install dependencies, run docker-compose)
are distinct and parallel; update the CONTRIBUTING.md sentence accordingly.
| To keep our services easy to understand within Docker we follow a naming convention for all our containers depending on its intended use. | ||
|
|
||
| `appwrite-worker-X` - Workers (`src/Appwrite/Platform/Workers/*`) | ||
| `appwrite-task-X` - Tasks (`src/Appwrite/Platform/Tasks/*`) | ||
|
|
||
| Other containes should be named the same as their service, for example `redis` should just be called `redis`. | ||
| Other containers should be named the same as their service, for example `redis` should just be called `redis`. |
There was a problem hiding this comment.
Fix pronoun agreement in container naming section.
Use plural agreement for containers: “depending on their intended use.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` around lines 176 - 181, Update the container naming
paragraph to use plural pronoun agreement by replacing "depending on its
intended use" with "depending on their intended use" in the container naming
section; ensure the lines that list `appwrite-worker-X`, `appwrite-task-X` and
the example `redis` remain unchanged except for this pronoun fix.
| ## Modules | ||
|
|
||
| As Appwrite grows, we noticed approach of having all service endpoints in `app/controllers/api/[service].php` is not maintainable. Not only it creates massive files, it also doesnt contain all product's features such as workers or tasks. While there might still be some occurances of those controller files, we avoid it in all new development, and gradually migrate existing controllers to **HTTP modules**. | ||
| As Appwrite grows, we noticed approach of having all service endpoints in `app/controllers/api/[service].php` is not maintainable. Not only it creates massive files, it also doesn't contain all product's features such as workers or tasks. While there might still be some occurrences of those controller files, we avoid it in all new development, and gradually migrate existing controllers to **HTTP modules**. |
There was a problem hiding this comment.
Polish grammar in the modules paragraph.
Line 192 reads awkwardly (“we noticed approach of having ...”). Add the missing article and tighten phrasing for readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` at line 192, Fix the grammar and tighten the phrasing in the
modules paragraph that currently starts "As Appwrite grows, we noticed approach
of having all service endpoints...": change it to something like "As Appwrite
grows, we noticed that the approach of having all service endpoints in
`app/controllers/api/[service].php` is not maintainable." Also restructure the
following sentence to be more concise and grammatical (e.g., "Not only does this
create massive files, it also omits product features such as workers and tasks;
while some controller files remain, we avoid this pattern in new development and
are gradually migrating existing controllers to HTTP modules.") to replace the
original paragraph.
| > Example: `Modules/Sites/Http/Sites/Get.php` | ||
|
|
||
| 2. If there are multiple resources, use then all in folder structure | ||
| 2. If there are multiple resources, use them all in folder structure |
There was a problem hiding this comment.
Add missing article for readability.
Line 207 should read more naturally as “use them all in the folder structure.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` at line 207, The sentence "If there are multiple resources,
use them all in folder structure" is missing the article "the"; update that text
(the snippet "use them all in folder structure") to read "use them all in the
folder structure" to improve readability.
| Metrics are collected within 3 scopes Daily, monthly, and infinity. Adding new usage metric in order to aggregate usage stats is very simple, but very much dependent on where do you want to collect | ||
| statistics ,via API or via background worker. For both cases you will need to add a `const` variable in `app/init.php` under the usage metrics list using the naming convention `METRIC_<RESOURCE_NAME>` as shown below. |
There was a problem hiding this comment.
Rewrite this sentence to remove ambiguity and improve flow.
The sentence is grammatically broken and overly long (“within 3 scopes Daily, monthly, and infinity... where do you want to collect statistics ...”). Split into 2 sentences and standardize scope capitalization.
🧰 Tools
🪛 LanguageTool
[style] ~398-~398: Consider a more concise word here.
Context: ..., and infinity. Adding new usage metric in order to aggregate usage stats is very simple, b...
(IN_ORDER_TO_PREMIUM)
[style] ~398-~398: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ic in order to aggregate usage stats is very simple, but very much dependent on where do yo...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` around lines 398 - 399, Rewrite the ambiguous sentence into
two clear sentences: first describe the three metric scopes as "daily, monthly,
and infinity" (lowercase) and then explain that adding a new usage metric is
simple but depends on whether statistics are collected via the API or a
background worker; finally instruct readers to add a const variable named using
the METRIC_<RESOURCE_NAME> convention in app/init.php under the usage metrics
list.
| ## Using preview domains locally | ||
|
|
||
| Appwrite Functions are automatically given a domain you can visit to execute the function. This domain has format `[SOMETHING].functions.localhost` unless you changed `_APP_DOMAIN_FUNCTIONS` environment variable. This default value works great when running Appwrite locally, but it can be impossible to use preview domains with Cloud woekspaces such as Gitpod or GitHub Codespaces. | ||
| Appwrite Functions are automatically given a domain you can visit to execute the function. This domain has format `[SOMETHING].functions.localhost` unless you changed `_APP_DOMAIN_FUNCTIONS` environment variable. This default value works great when running Appwrite locally, but it can be impossible to use preview domains with Cloud workspaces such as Gitpod or GitHub Codespaces. |
There was a problem hiding this comment.
Improve grammar in preview-domain guidance.
Line 664 needs minor grammar fixes: “This domain has the format ... unless you’ve changed _APP_DOMAIN_FUNCTIONS ...”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` at line 664, Update the sentence explaining Appwrite
Functions preview domains to correct the grammar: change "This domain has format
`[SOMETHING].functions.localhost` unless you changed `_APP_DOMAIN_FUNCTIONS`
environment variable." to include the article and contraction, e.g. "This domain
has the format `[SOMETHING].functions.localhost` unless you’ve changed the
`_APP_DOMAIN_FUNCTIONS` environment variable." Ensure the
`_APP_DOMAIN_FUNCTIONS` token and the example domain
`[SOMETHING].functions.localhost` remain unchanged.
What does this PR do?
Group of multiple PRs all requiring migration:
It also adds schema for team labels, feature to be implemented a bit later
Test Plan
All PRs already tested separately
Related PRs and Issues
x
Checklist