Skip to content

chore: extract getRepositoryAccessType method#75

Merged
Meldiron merged 3 commits intomainfrom
ser-1170
Mar 23, 2026
Merged

chore: extract getRepositoryAccessType method#75
Meldiron merged 3 commits intomainfrom
ser-1170

Conversation

@hmacr
Copy link
Copy Markdown
Contributor

@hmacr hmacr commented Mar 23, 2026

Related SER-1170

@hmacr hmacr changed the title feat: implement getRepositoryAccessType method chore: extract getRepositoryAccessType method Mar 23, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 23, 2026

Greptile Summary

This PR extracts the "find installation access type" logic from searchRepositories into a dedicated getRepositoryAccessType() method on the Adapter base class, and removes the now-redundant $installationId parameter from searchRepositories across both the GitHub and Gitea adapters.

  • The refactoring is structurally clean and the new abstract method is well-documented.
  • **The fallback value in GitHub::getRepositoryAccessType() was changed from '' (which resolved to false/'selected') to 'all'**, reversing the conservative error-path default from the original code. If the GitHub API call fails or returns an unexpected structure, the method now returns 'all'` instead of implicitly indicating restricted access — this is a silent behavioural regression worth addressing.
  • The searchRepositories docblock in GitHub.php still lists the removed $installationId @param entry.
  • The new integration test hardcodes 'all' as the expected access type, making it fragile against installation-configuration changes.

Confidence Score: 3/5

  • Safe to merge after addressing the fallback-value regression in getRepositoryAccessType().
  • The structural refactoring is correct, but changing the fallback from '' to 'all' silently flips the error-path behaviour of searchRepositories from "assume restricted access" to "assume full access", which is a meaningful behavioural change introduced unintentionally during the extraction.
  • src/VCS/Adapter/Git/GitHub.php — specifically the fallback value on line 189 of getRepositoryAccessType().

Important Files Changed

Filename Overview
src/VCS/Adapter.php Adds abstract getRepositoryAccessType(): string method and removes $installationId from the searchRepositories signature cleanly, with updated docblock.
src/VCS/Adapter/Git/GitHub.php Implements getRepositoryAccessType() by calling the GitHub App installations API, but the fallback value was changed from '' (conservative/"selected") to 'all' (permissive), altering error-path behavior. The docblock for searchRepositories also retains the now-removed $installationId param entry.
src/VCS/Adapter/Git/Gitea.php Adds a stub getRepositoryAccessType() that throws Not implemented yet, and removes the unused $installationId parameter from searchRepositories. Both changes are clean.
tests/VCS/Adapter/GitHubTest.php Adds an integration test for getRepositoryAccessType(), but the assertion hardcodes 'all', making the test brittle and dependent on the live installation's configuration.

Reviews (1): Last reviewed commit: "feat: implement `getRepositoryAccessType..." | Re-trigger Greptile

@utopia-php utopia-php deleted a comment from hmacr Mar 23, 2026
@Meldiron Meldiron merged commit fa3fa76 into main Mar 23, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants