Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAccepted SDK Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR adds Key changes:
Confidence Score: 3/5
Important Files Changed
Reviews (1): Last reviewed commit: "chore: update sdks script" | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Tasks/SDKs.php`:
- Around line 978-998: The current $entryPattern requires a trailing comma in
the final capture group which fails for last-array entries; update the regex
used to build $entryPattern so the closing quote and comma are captured but the
comma is optional (so the replacement using '${1}' . $newVersion . '${3}' still
works). Locate the $entryPattern definition in SDKs.php (the variable named
$entryPattern) and change its final capture group from the strict comma form to
one that allows an optional comma (e.g., capture the closing quote and an
optional comma/whitespace) so preg_match()/preg_replace_callback will match
entries whether or not they have a trailing comma. Ensure $updated behavior and
the existing replacement logic remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69508df8-b243-4543-8963-9cc4610a3769
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/SDKs.php
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testVectorsDBStats |
1 | 10.06s | Logs |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Appwrite/Platform/Tasks/SDKs.php (2)
1073-1075: Add PHPDoc forupdateExistingPr().This helper mutates
$prUrlsby reference and has a non-obvious$existingPrUrlfast path. Please document that contract now that the method is part of the PR update flow.As per coding guidelines:
**/*.php: Include comprehensive PHPDoc comments.`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/SDKs.php` around lines 1073 - 1075, Add a comprehensive PHPDoc block above the updateExistingPr() method describing its purpose in the PR update flow, document each parameter including types, explicitly note that the array parameter $prUrls is passed by reference and will be mutated by this method, explain the optional $existingPrUrl fast-path behaviour (used when a pre-existing PR URL is supplied to skip lookup), and declare the void return type; include any exceptions the method may throw if applicable and a short example of the side-effect on $prUrls.
702-705: Drop the dead$languageNameargument fromcleanupTarget().After the log message became generic, this parameter is no longer used. Removing it will clear the PHPMD warning and simplify the call site.
Suggested fix
- $this->cleanupTarget($target, $language['name']); + $this->cleanupTarget($target);- private function cleanupTarget(string $target, string $languageName): void + private function cleanupTarget(string $target): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/SDKs.php` around lines 702 - 705, The cleanupTarget method signature includes an unused parameter $languageName which should be removed to eliminate the dead-parameter warning and simplify callers: update the method declaration cleanupTarget(string $target, string $languageName): void to cleanupTarget(string $target): void, remove any references to $languageName inside the method body (none currently), and update all call sites that pass a language name to instead call cleanupTarget with only the $target argument so signatures match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Tasks/SDKs.php`:
- Around line 984-990: The inline SDK key/version replacement must be limited to
the matched platform block instead of operating on the whole file: use the
existing $blockPattern to preg_match and capture the block content, then run the
$entryPattern replacement against the captured block (e.g., preg_replace on
$matches[2]) so only the $platform block's inline entries are updated; if no
inline entry is found inside that block fall back to updating the
${$platform}Versions fallback as currently implemented. Ensure you reference and
update the same $entryPattern and $blockPattern variables and write the modified
block back into $content after replacement.
- Around line 998-1010: The success message is logged before the file write so a
failed write can produce both success and error; move the Console::success("
Config updated: {$sdkKey} {$oldVersion} → {$newVersion}"); call so it runs only
after file_put_contents($configPath, $newContent) returns !== false (i.e., after
the write succeeds), and ensure the existing Console::error(' Failed to write
config file'); remains for the false branch; adjust surrounding logic in the
method that builds $newContent (the preg_replace callback and $updated flag) so
the success log uses the same variables ($sdkKey, $oldVersion, $newVersion)
after the write completes.
---
Nitpick comments:
In `@src/Appwrite/Platform/Tasks/SDKs.php`:
- Around line 1073-1075: Add a comprehensive PHPDoc block above the
updateExistingPr() method describing its purpose in the PR update flow, document
each parameter including types, explicitly note that the array parameter $prUrls
is passed by reference and will be mutated by this method, explain the optional
$existingPrUrl fast-path behaviour (used when a pre-existing PR URL is supplied
to skip lookup), and declare the void return type; include any exceptions the
method may throw if applicable and a short example of the side-effect on
$prUrls.
- Around line 702-705: The cleanupTarget method signature includes an unused
parameter $languageName which should be removed to eliminate the dead-parameter
warning and simplify callers: update the method declaration cleanupTarget(string
$target, string $languageName): void to cleanupTarget(string $target): void,
remove any references to $languageName inside the method body (none currently),
and update all call sites that pass a language name to instead call
cleanupTarget with only the $target argument so signatures match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bad14163-5b24-4c3a-96d0-30e7b7098f05
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/SDKs.php
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/Appwrite/Platform/Tasks/SDKs.php (3)
1001-1001:⚠️ Potential issue | 🟡 MinorLog success message only after the file write succeeds.
The
Console::success()call at line 1001 executes inside thepreg_replace_callbackbefore the actualfile_put_contents()at line 1012. If the write fails, both a success and an error message will be logged.Suggested fix
$updated = false; - $newContent = preg_replace_callback($blockPattern, function ($blockMatch) use ($entryPattern, $newVersion, $sdkKey, &$updated) { + $oldVersion = ''; + $newContent = preg_replace_callback($blockPattern, function ($blockMatch) use ($entryPattern, $newVersion, $sdkKey, &$updated, &$oldVersion) { $blockContent = $blockMatch[2]; if (preg_match($entryPattern, $blockContent, $entryMatch)) { $oldVersion = $entryMatch[2]; - Console::success(" Config updated: {$sdkKey} {$oldVersion} → {$newVersion}"); $blockContent = preg_replace($entryPattern, '${1}' . $newVersion . '${3}', $blockContent); $updated = true; } return $blockMatch[1] . $blockContent . $blockMatch[3]; }, $content); if (! $updated) { throw new \RuntimeException("Could not find version entry for {$sdkKey} in \${$platform}Versions block"); } if (file_put_contents($configPath, $newContent) === false) { Console::error(' Failed to write config file'); return false; } + Console::success(" Config updated: {$sdkKey} {$oldVersion} → {$newVersion}"); return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/SDKs.php` at line 1001, The success log is emitted inside the preg_replace_callback (Console::success(" Config updated: {$sdkKey} {$oldVersion} → {$newVersion}")) before the actual file write, so move or defer that Console::success call until after file_put_contents() completes successfully; update the code to collect the replacement result in the callback (or record the changed keys like $sdkKey/$oldVersion/$newVersion), perform file_put_contents(), check its return value/false, and only then call Console::success(...) for each successful update, otherwise log an error.
990-990:⚠️ Potential issue | 🟡 MinorMake the trailing comma optional in the entry pattern.
The
([\'"],)capture group requires a trailing comma, but PHP arrays don't mandate trailing commas on the last element. If the SDK entry is last in its block without a trailing comma, this pattern will fail to match.Suggested fix
- $entryPattern = '/([\'"]' . preg_quote($sdkKey, '/') . '[\'"]\s*=>\s*[\'"])([^\'"]+)([\'"],)/m'; + $entryPattern = '/([\'"]' . preg_quote($sdkKey, '/') . '[\'"]\s*=>\s*[\'"])([^\'"]+)([\'"],?)/m';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/SDKs.php` at line 990, The regex stored in $entryPattern requires a trailing comma via the capture group ([\'"],), which fails when the SDK entry is the last array element; update the pattern so the trailing comma is optional (e.g., make the comma part optional in that final capture group or use a non-capturing optional comma) so the regex matches both comma-terminated and final entries when locating SDK entries.
969-982:⚠️ Potential issue | 🟠 MajorInline pattern is not scoped to platform—duplicate SDK keys will cause incorrect updates.
The
$inlinePatternmatches the first occurrence of the SDK key in the entire file, regardless of which platform block it's in. For SDK keys that appear under multiple platforms (e.g.,webexists in bothclientandconsole), this will update the wrong entry.Based on the config file structure (versions stored inline within nested SDK arrays), the replacement must be scoped to the correct platform block first, then match the SDK entry within that block.
Suggested approach
- // First, try to find inline version in SDK array (pattern 1) - // Pattern matches: ['key' => 'nodejs', ... 'version' => '22.1.2'] - $inlinePattern = '/(\[\s*[\'"]key[\'"]\s*=>\s*[\'"]' . preg_quote($sdkKey, '/') . '[\'"]\s*,[\s\S]*?[\'"]version[\'"]\s*=>\s*[\'"])([^\'"]+)([\'"])/m'; - - if (preg_match($inlinePattern, $content, $matches)) { - $oldVersion = $matches[2]; - $newContent = preg_replace($inlinePattern, '${1}' . $newVersion . '${3}', $content); + // Scope to the platform block first, then find the SDK entry within it + // Platform constant pattern: APP_SDK_PLATFORM_CLIENT, APP_SDK_PLATFORM_SERVER, APP_SDK_PLATFORM_CONSOLE + $platformConstant = 'APP_SDK_PLATFORM_' . strtoupper($platform); + $platformBlockPattern = '/(' . preg_quote($platformConstant, '/') . '\s*=>\s*\[)([\s\S]*?)(\n\s*\],)/m'; + + $inlinePattern = '/(\[\s*[\'"]key[\'"]\s*=>\s*[\'"]' . preg_quote($sdkKey, '/') . '[\'"]\s*,[\s\S]*?[\'"]version[\'"]\s*=>\s*[\'"])([^\'"]+)([\'"])/m'; + + $updated = false; + $oldVersion = ''; + $newContent = preg_replace_callback($platformBlockPattern, function ($blockMatch) use ($inlinePattern, $newVersion, &$updated, &$oldVersion) { + if (preg_match($inlinePattern, $blockMatch[2], $entryMatch)) { + $oldVersion = $entryMatch[2]; + $updatedBlock = preg_replace($inlinePattern, '${1}' . $newVersion . '${3}', $blockMatch[2]); + $updated = true; + return $blockMatch[1] . $updatedBlock . $blockMatch[3]; + } + return $blockMatch[0]; + }, $content); + + if ($updated) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/SDKs.php` around lines 969 - 982, The inline regex in SDKs.php ($inlinePattern) is matching the first SDK key globally in $content and can update the wrong platform; change the logic to first locate the correct platform block (e.g. use a $platformBlockPattern anchored to the platform name/block that captures its contents), run preg_match to extract that block, perform the SDK-specific preg_match/preg_replace for $sdkKey against the captured block only to replace its version with $newVersion, then splice the updated block back into $content and write $configPath; update the Console messages to use the $oldVersion found inside the platform block and keep existing file_put_contents handling.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Tasks/SDKs.php (1)
556-556: Remove unused$languageparameter.The
$languageparameter is never used within the method body. Consider removing it to avoid confusion.Suggested fix
- private function pushToGit(array $language, string $target, string $result, string $gitUrl, string $gitBranch, string $repoBranch, string $commitMessage): bool + private function pushToGit(string $target, string $result, string $gitUrl, string $gitBranch, string $repoBranch, string $commitMessage): boolAlso update the call site at line 532:
- $pushSuccess = $this->pushToGit($language, $target, $result, $gitUrl, $gitBranch, $repoBranch, $commitMessage); + $pushSuccess = $this->pushToGit($target, $result, $gitUrl, $gitBranch, $repoBranch, $commitMessage);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/SDKs.php` at line 556, The method pushToGit has an unused parameter $language; remove that parameter from the signature (private function pushToGit(string $target, string $result, string $gitUrl, string $gitBranch, string $repoBranch, string $commitMessage): bool) and update every call site that still passes $language to instead call the new signature (remove the extra argument), and update any related docblocks, type hints, or unit tests referencing pushToGit to match the new parameter list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Appwrite/Platform/Tasks/SDKs.php`:
- Line 1001: The success log is emitted inside the preg_replace_callback
(Console::success(" Config updated: {$sdkKey} {$oldVersion} → {$newVersion}"))
before the actual file write, so move or defer that Console::success call until
after file_put_contents() completes successfully; update the code to collect the
replacement result in the callback (or record the changed keys like
$sdkKey/$oldVersion/$newVersion), perform file_put_contents(), check its return
value/false, and only then call Console::success(...) for each successful
update, otherwise log an error.
- Line 990: The regex stored in $entryPattern requires a trailing comma via the
capture group ([\'"],), which fails when the SDK entry is the last array
element; update the pattern so the trailing comma is optional (e.g., make the
comma part optional in that final capture group or use a non-capturing optional
comma) so the regex matches both comma-terminated and final entries when
locating SDK entries.
- Around line 969-982: The inline regex in SDKs.php ($inlinePattern) is matching
the first SDK key globally in $content and can update the wrong platform; change
the logic to first locate the correct platform block (e.g. use a
$platformBlockPattern anchored to the platform name/block that captures its
contents), run preg_match to extract that block, perform the SDK-specific
preg_match/preg_replace for $sdkKey against the captured block only to replace
its version with $newVersion, then splice the updated block back into $content
and write $configPath; update the Console messages to use the $oldVersion found
inside the platform block and keep existing file_put_contents handling.
---
Nitpick comments:
In `@src/Appwrite/Platform/Tasks/SDKs.php`:
- Line 556: The method pushToGit has an unused parameter $language; remove that
parameter from the signature (private function pushToGit(string $target, string
$result, string $gitUrl, string $gitBranch, string $repoBranch, string
$commitMessage): bool) and update every call site that still passes $language to
instead call the new signature (remove the extra argument), and update any
related docblocks, type hints, or unit tests referencing pushToGit to match the
new parameter list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34af1aaf-591f-4ee5-9232-37968bf6bf49
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/SDKs.php
✨ Benchmark results
⚡ Benchmark Comparison
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Appwrite/Platform/Tasks/SDKs.php (1)
556-556: Remove unused$languageparameter.The
$languageparameter is not used anywhere in the function body. Consider removing it from the signature and updating callers accordingly.Suggested fix
-private function pushToGit(array $language, string $target, string $result, string $gitUrl, string $gitBranch, string $repoBranch, string $commitMessage): bool +private function pushToGit(string $target, string $result, string $gitUrl, string $gitBranch, string $repoBranch, string $commitMessage): boolAnd update the caller at line 532:
-$pushSuccess = $this->pushToGit($language, $target, $result, $gitUrl, $gitBranch, $repoBranch, $commitMessage); +$pushSuccess = $this->pushToGit($target, $result, $gitUrl, $gitBranch, $repoBranch, $commitMessage);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/SDKs.php` at line 556, Remove the unused $language parameter from the pushToGit method signature (function pushToGit) and update all call sites that pass that extra argument to call pushToGit with the remaining parameters (target, result, gitUrl, gitBranch, repoBranch, commitMessage); ensure the method declaration and any type hints are adjusted accordingly and run tests to confirm no remaining references to $language exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Tasks/SDKs.php`:
- Around line 642-648: The catch block around $repo->commit currently swallows
all \Throwable and assumes "no changes to commit"; change it to inspect and log
the exception and only treat known "nothing to commit" cases as success: in the
catch for \Throwable $e (in the method using $repo->commit), call Console::error
or similar to log $e->getMessage() and $e->__toString(), and if the message
indicates "nothing to commit" or the specific git status code, return true,
otherwise rethrow the exception (or return false) so real errors (permissions,
corruption) are not masked; ensure you reference $repo->commit and
Console::log/Console::error in the updated handling.
- Around line 538-539: The shell command built for cleanup uses $target directly
in exec() which is unsafe; update the call in the cleanup code (the exec(...)
that runs "chmod -R u+w ... && rm -rf ...") to wrap $target with
escapeshellarg() (e.g. escapeshellarg($target) used for both chmod and rm -rf)
so the command arguments are safely escaped, leaving the following
Console::log(' Cleaned up temp directory') unchanged.
---
Nitpick comments:
In `@src/Appwrite/Platform/Tasks/SDKs.php`:
- Line 556: Remove the unused $language parameter from the pushToGit method
signature (function pushToGit) and update all call sites that pass that extra
argument to call pushToGit with the remaining parameters (target, result,
gitUrl, gitBranch, repoBranch, commitMessage); ensure the method declaration and
any type hints are adjusted accordingly and run tests to confirm no remaining
references to $language exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfc5b311-63c2-4dc5-8341-03533734e075
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/SDKs.php
Comment on lines
+538
to
+539
| \exec('chmod -R u+w ' . $target . ' && rm -rf ' . $target); | ||
| Console::log(' Cleaned up temp directory'); |
Contributor
There was a problem hiding this comment.
Use escapeshellarg() for shell argument safety.
While $target is constructed from controlled sources, it's safer to escape shell arguments to guard against unexpected values in $language['key'].
Suggested fix
-\exec('chmod -R u+w ' . $target . ' && rm -rf ' . $target);
+\exec('chmod -R u+w ' . \escapeshellarg($target) . ' && rm -rf ' . \escapeshellarg($target));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Tasks/SDKs.php` around lines 538 - 539, The shell
command built for cleanup uses $target directly in exec() which is unsafe;
update the call in the cleanup code (the exec(...) that runs "chmod -R u+w ...
&& rm -rf ...") to wrap $target with escapeshellarg() (e.g.
escapeshellarg($target) used for both chmod and rm -rf) so the command arguments
are safely escaped, leaving the following Console::log(' Cleaned up temp
directory') unchanged.
Comment on lines
+642
to
+648
| try { | ||
| $repo->commit($commitMessage); | ||
| } catch (\Throwable $e) { | ||
| // Exit code 1 (256 in PHP) = nothing to commit | ||
| Console::log(' No changes to commit, SDK is up to date'); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Catching all exceptions may mask real commit errors.
The catch block assumes any exception means "nothing to commit," but other failures (permission issues, git corruption) would be silently treated as success. Consider verifying the exception message or logging it for debugging.
Suggested fix
try {
$repo->commit($commitMessage);
} catch (\Throwable $e) {
- // Exit code 1 (256 in PHP) = nothing to commit
- Console::log(' No changes to commit, SDK is up to date');
- return true;
+ // Exit code 1 (256 in PHP) = nothing to commit
+ if (\stripos($e->getMessage(), 'nothing to commit') !== false) {
+ Console::log(' No changes to commit, SDK is up to date');
+ return true;
+ }
+ throw $e; // Re-throw unexpected errors
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Tasks/SDKs.php` around lines 642 - 648, The catch block
around $repo->commit currently swallows all \Throwable and assumes "no changes
to commit"; change it to inspect and log the exception and only treat known
"nothing to commit" cases as success: in the catch for \Throwable $e (in the
method using $repo->commit), call Console::error or similar to log
$e->getMessage() and $e->__toString(), and if the message indicates "nothing to
commit" or the specific git status code, return true, otherwise rethrow the
exception (or return false) so real errors (permissions, corruption) are not
masked; ensure you reference $repo->commit and Console::log/Console::error in
the updated handling.
abnegate
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.