test: rewrite GitHub adapter tests to be fully dynamic

[jaysomani]

Rewrites GitHubTest.php from hardcoded fixtures to fully dynamic tests following the same pattern as GitLab and Gitea adapters.
Changes:

All tests now create repositories dynamically with uniqid() and clean up in finally blocks
Owner resolved dynamically via getOwnerName() — works with any GitHub App installation, no hardcoding
setUp now skips gracefully when credentials are missing instead of erroring
Added str_replace('\n', "\n", ...) in setUp for local env compatibility (no-op on CI)
35 dynamic tests passing, 7 skipped (require createBranch/createPullRequest which are not implemented in GitHub adapter)

Also fixes in GitHub.php:

getRepository now throws RepositoryNotFound on 404 instead of returning raw error response
getCommit now throws RepositoryNotFound on 404/422 instead of returning empty commit struct

New tests added: testCreatePrivateRepository, testGetDeletedRepositoryFails, testDeleteRepository, testDeleteNonExistingRepositoryFails, testGetRepositoryNameWithInvalidId, testGetRepositoryTreeWithInvalidBranch, testGetRepositoryContentWithRef, testGetRepositoryContentFileNotFound, testGetRepositoryContentCaseSensitive, testListRepositoryContentsNonExistingPath, testListRepositoryLanguages, testListBranches, testGetCommitWithInvalidHash, testGetLatestCommitWithInvalidBranch, testUpdateCommitStatus, testGenerateCloneCommandWithInvalidRepository, testGetOwnerName, testSearchRepositories, testValidateWebhookEvent, testGetEventInstallation
Note: testGenerateCloneCommandWithTag (wildcard tags) not included as createTag is not implemented in GitHub adapter.

[greptile-apps]

Greptile Summary

This PR rewrites GitHubTest.php from hardcoded fixtures to fully dynamic integration tests (creating/deleting real repos per test), and fixes getRepository/getCommit in GitHub.php to throw RepositoryNotFound on 404/422 instead of silently returning empty structures.

• testSearchRepositories: both createRepository calls happen before the try/finally block — if the second creation throws, the first repository is permanently leaked (new instance of the resource-leak pattern flagged across multiple other tests in this PR).
• Several other tests (testGetRepositoryTreeWithInvalidBranch, testGetRepositoryContentFileNotFound, testGetRepositoryContentCaseSensitive, testGetLatestCommitWithInvalidBranch, testGetCommitWithInvalidHash) still have setup calls outside try, and testGenerateCloneCommandWithCommitHash still does not clean up its local temp directory — both patterns were raised in the prior review round and remain unaddressed.

Confidence Score: 3/5

Multiple P1 resource-leak patterns across the test file remain unresolved from the prior review round, plus a new instance in testSearchRepositories.

P1 ceiling is 4/5, but several distinct P1 resource-leak instances (some pre-existing from the last review, one new in testSearchRepositories) pull the score below that ceiling. The adapter changes in GitHub.php are minimal and low-risk.

tests/VCS/Adapter/GitHubTest.php — resource-leak pattern in testSearchRepositories and other tests where repository/file creation calls precede the try/finally block.

Important Files Changed

Filename	Overview
src/VCS/Adapter/Git/GitHub.php	Adds 404/422 error guards to getRepository and getCommit, throwing RepositoryNotFound instead of returning empty arrays. The 422 guard on getRepository is undocumented by the GitHub API (already raised in prior review thread) and consistent with the same pattern in getCommit.
tests/VCS/Adapter/GitHubTest.php	Complete rewrite to dynamic tests using uniqid() repositories. Several resource-leak patterns remain (setup before try blocks, temp directory not cleaned in testGenerateCloneCommandWithCommitHash). testHasAccessToAllRepositories assertion weakened from assertTrue to assertIsBool. testSearchRepositories creates two repos before the try block, leaking the first if the second creation fails.

Comments Outside Diff (1)

1. tests/VCS/Adapter/GitHubTest.php, line 1023-1024 (link)

   Both createRepository calls outside try — first repo leaks on second failure

   Both creation calls precede the try/finally block. If the second createRepository throws (e.g. transient API error or rate-limit), $repo1Name has already been created and will never be deleted because the finally block is never reached. Move both calls inside the try block to guarantee cleanup.

_{Reviews (2): Last reviewed commit: "Merge branch 'utopia-php:main' into feat..." | Re-trigger Greptile}

[greptile-apps]

Repository setup outside try leaks resource on createFile failure

In several tests, createRepository and createFile are called before the try/finally block that triggers deleteRepository. If createFile throws (e.g. a transient API error), the repository created immediately before it is never deleted, leaking a GitHub repository.

Tests affected: testGetRepositoryTreeWithInvalidBranch (lines 488–498), testGetRepositoryContentFileNotFound (lines 559–574), testGetRepositoryContentCaseSensitive (lines 578–596), testGetLatestCommitWithInvalidBranch (lines 853–867), testGetCommitWithInvalidHash (lines 510–520), and others. All setup calls (createRepository + createFile) should be inside the try block so the finally runs regardless of which step fails.

[greptile-apps]

getLatestCommit error path not covered by new adapter changes

testGetLatestCommitWithInvalidBranch expects \Exception::class when an invalid branch is supplied, but getLatestCommit was not updated in this PR to check the HTTP status code before parsing the response body. On a 404, $responseBody will be an error object whose keys differ from a commit object, triggering the pre-existing "Latest commit response is missing required information." exception — so the test passes today. However, for consistency with the getRepository / getCommit fixes in this PR, it would be cleaner to add explicit 404 handling in getLatestCommit so the failure reason is clear, rather than relying on a structural parse error.

Also, the createRepository/createFile setup calls should be moved inside the try block to prevent resource leaks.

Suggested change

	public function testGetLatestCommitWithInvalidBranch(): void
	{
	\exec('rm -rf /tmp/clone-commit');
	$gitCloneCommand = $this->vcsAdapter->generateCloneCommand('test-kh', 'test2', '4fb10447faea8a55c5cad7b5ebdfdbedca349fe4', GitHub::CLONE_TYPE_COMMIT, '/tmp/clone-commit', '*');
	$this->assertNotEmpty($gitCloneCommand);
	$this->assertStringContainsString('sparse-checkout', $gitCloneCommand);
	$repositoryName = 'test-get-latest-commit-invalid-' . \uniqid();
	$this->vcsAdapter->createRepository(static::$owner, $repositoryName, false);
	$this->vcsAdapter->createFile(static::$owner, $repositoryName, 'README.md', '# Test');
	$output = '';
	$resultCode = null;
	\exec($gitCloneCommand, $output, $resultCode);
	$this->assertSame(0, $resultCode);
	try {
	$this->expectException(\Exception::class);
	$this->vcsAdapter->getLatestCommit(static::$owner, $repositoryName, 'non-existing-branch');
	} finally {
	$this->vcsAdapter->deleteRepository(static::$owner, $repositoryName);
	}
	}
	public function testGetLatestCommitWithInvalidBranch(): void
	{
	$repositoryName = 'test-get-latest-commit-invalid-' . \uniqid();
	try {
	$this->vcsAdapter->createRepository(static::$owner, $repositoryName, false);
	$this->vcsAdapter->createFile(static::$owner, $repositoryName, 'README.md', '# Test');
	$this->expectException(\Exception::class);
	$this->vcsAdapter->getLatestCommit(static::$owner, $repositoryName, 'non-existing-branch');
	} finally {
	$this->vcsAdapter->deleteRepository(static::$owner, $repositoryName);
	}
	}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 706a5362e0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid classifying invalid commit refs as repository deletion

GET /repos/{owner}/{repo}/commits/{ref} returns 422 for invalid commit refs even when the repository exists, but this branch throws RepositoryNotFound, which semantically signals a missing repository. Any caller that handles RepositoryNotFound as a repo-level failure (e.g., cleanup/reinstall flows) will now mis-handle bad SHAs as deleted repos; keep RepositoryNotFound for true repo-missing responses (404) and use a non-repo-specific exception for 422.

Useful? React with 👍 / 👎.

[Meldiron]

Possibly valid, we might want another exception, or new one. depending on what other adapters do