fix: Ensure installation ID is set correctly in GitHub payload handling#64
fix: Ensure installation ID is set correctly in GitHub payload handling#64
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughRefactors GitHub and Gitea adapters to defensively extract and validate HTTP response bodies and headers. Direct nested array access was replaced with local extracts (e.g., $responseBody, $responseHeaders) and guarded checks (array_key_exists / null coalescing). Many methods in src/VCS/Adapter/Git/GitHub.php (searchRepositories, getRepositoryName, getRepositoryTree, content/listing, comments, tokens, owner/PR/branch/commit/event parsing, etc.) now tolerate missing keys and return safe defaults. Gitea changes similarly guard header/status-code and body access and adjust error handling. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
634-634: Consider using??for consistency with the rest of the function.The
isset()ternary is functionally correct, but the function already uses the null-coalescing operator everywhere else (e.g., lines 641–652, 693–704). Since PHP's??has identical semantics toisset($x) ? $x : nullfor chained array accesses, this could be simplified:♻️ Proposed simplification
- $installationId = strval(isset($payload['installation']['id']) ? $payload['installation']['id'] : ''); + $installationId = (string) ($payload['installation']['id'] ?? '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VCS/Adapter/Git/GitHub.php` at line 634, Replace the ternary with the null-coalescing operator for consistency: when building $installationId from $payload use the ?? operator to fall back to an empty string and cast to string (keep the variable name $installationId and the $payload array reference so the change is localized to the same assignment site in the GitHub adapter function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/VCS/Adapter/Git/GitHub.php`:
- Line 634: Replace the ternary with the null-coalescing operator for
consistency: when building $installationId from $payload use the ?? operator to
fall back to an empty string and cast to string (keep the variable name
$installationId and the $payload array reference so the change is localized to
the same assignment site in the GitHub adapter function).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
181-189: Consider explicit non-200 gating ingetRepositoryTreeas well.This method currently relies on missing
treefallback behavior; adding an explicitstatus !== 200early return would make behavior deterministic and align with adapter conventions.Based on learnings, in
src/VCS/Adapter/Git/Gitea.phpandsrc/VCS/Adapter/Git/GitHub.php,getRepositoryTreeandlistRepositoryContentsshould return an empty array for any HTTP status other than 200 or 404 to keep adapter behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VCS/Adapter/Git/GitHub.php` around lines 181 - 189, The method getRepositoryTree in the GitHub adapter currently only special-cases 404 and then falls back to reading 'tree'; change it to explicitly gate on response status codes: read $response['headers']['status-code'] (responseHeadersStatusCode) and if it is not 200 (and not 404) return an empty array immediately, keeping the existing 404->[] behavior and only parsing the body when status === 200; apply the same explicit non-200/404 early-return pattern to listRepositoryContents in both the GitHub and Gitea adapters so both adapters consistently return [] for any HTTP status other than 200 or 404.
🤖 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/VCS/Adapter/Git/GitHub.php`:
- Around line 277-291: The code in GitHub adapter (methods getRepositoryTree /
listRepositoryContents) currently treats any non-404 response body as content,
which creates synthetic entries for error responses; update the response status
handling to immediately return [] when the response status code is neither 200
nor 404. Concretely, in GitHub.php after reading $responseHeaders['status-code']
(and similarly update Gitea.php counterparts), add a guard: if
($responseHeadersStatusCode !== 200 && $responseHeadersStatusCode !== 404)
return []; then keep the existing 404 handling and body-wrapping logic only for
200/404 cases.
---
Nitpick comments:
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 181-189: The method getRepositoryTree in the GitHub adapter
currently only special-cases 404 and then falls back to reading 'tree'; change
it to explicitly gate on response status codes: read
$response['headers']['status-code'] (responseHeadersStatusCode) and if it is not
200 (and not 404) return an empty array immediately, keeping the existing
404->[] behavior and only parsing the body when status === 200; apply the same
explicit non-200/404 early-return pattern to listRepositoryContents in both the
GitHub and Gitea adapters so both adapters consistently return [] for any HTTP
status other than 200 or 404.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4b76cf1-1632-47ee-9f5d-40ae08627df8
📒 Files selected for processing (2)
src/VCS/Adapter/Git/GitHub.phpsrc/VCS/Adapter/Git/Gitea.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/VCS/Adapter/Git/Gitea.php
2 participants
Summary of what was wrong and what was changed:
Cause of the warnings
Line 634 was using $payload['installation']['id'] with no checks.
So you got:
"Trying to access array offset on null" when $payload['installation'] was null.
"Undefined array key 'installation'" when the payload had no installation key.
That can happen for non–GitHub App webhooks (e.g. repo webhooks) or for some event types that don’t send an installation object.
Summary by CodeRabbit