Feature/dev pull command - #18
Conversation
- Introduced 'repository' configuration in notur.php for GitHub source code. - Registered DevPullCommand in NoturServiceProvider for enhanced development workflow.
…ntegration - Bumped framework version to 1.2.4 in configuration. - Introduced `notur:dev:pull` command to facilitate pulling updates from GitHub, including options for specific branches and dry-run functionality. - Added `repository` configuration key for GitHub source management. - Updated documentation to reflect new command and configuration changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf01adb5a7
ℹ️ 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".
| private function resolvePackageManager(string $cwd): string | ||
| { | ||
| if (file_exists($cwd . '/bun.lockb') || file_exists($cwd . '/bun.lock')) { | ||
| return 'bun'; | ||
| } | ||
| if (file_exists($cwd . '/pnpm-lock.yaml')) { | ||
| return 'pnpm'; | ||
| } | ||
| if (file_exists($cwd . '/yarn.lock')) { | ||
| return 'yarn'; | ||
| } | ||
|
|
||
| return 'bun'; | ||
| } |
There was a problem hiding this comment.
If the pulled framework (or a fork) uses npm and only has a package-lock.json, resolvePackageManager still falls back to bun, which will fail on systems without Bun and prevents the rebuild steps from running even though npm is available. This makes notur:dev:pull unreliable for npm-based setups. Consider checking for package-lock.json and returning npm (or defaulting to npm when no lock file is found).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Updated package manager detection to recognize
package-lock.jsonand default to npm when no lockfile is present, improving npm-based rebuilds. src/Console/Commands/DevPullCommand.phpL305-L321
Testing
⚠️ not run (not requested)
There was a problem hiding this comment.
Pull request overview
Adds a new Notur development workflow for pulling unreleased framework code from GitHub into the local Composer-installed framework directory, with corresponding config and documentation updates.
Changes:
- Introduces
notur:dev:pullArtisan command to download/extract a GitHub commit and optionally rebuild bridge assets. - Registers the new command and adds
notur.repositoryconfiguration (defaultsak0a/notur). - Updates docs/changelog and bumps framework version to
1.2.4.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/Console/Commands/DevPullCommand.php |
New command that fetches commit info, downloads/extracts zipball, replaces vendor/notur/notur, and rebuilds frontend assets. |
src/NoturServiceProvider.php |
Registers the new DevPullCommand with Artisan. |
config/notur.php |
Bumps version to 1.2.4 and adds repository config key. |
website/docs/admin/guide.md |
Documents notur:dev:pull usage and the new repository config option. |
website/docs/reference/changelog.md |
Adds 1.2.4 changelog entry describing the new command/config. |
CLAUDE.md |
Updates documented Artisan command count/list to include notur:dev:pull. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private function runProcess(string $command, string $cwd): int | ||
| { | ||
| $process = proc_open( | ||
| $command, | ||
| [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], | ||
| $pipes, | ||
| $cwd, | ||
| ); | ||
|
|
||
| if (!is_resource($process)) { | ||
| return 1; | ||
| } | ||
|
|
||
| $output = stream_get_contents($pipes[1]); | ||
| $errors = stream_get_contents($pipes[2]); | ||
|
|
||
| fclose($pipes[1]); | ||
| fclose($pipes[2]); | ||
|
|
||
| $exitCode = proc_close($process); | ||
|
|
||
| if ($output) { | ||
| $this->line($output); | ||
| } | ||
| if ($errors && $exitCode !== 0) { | ||
| $this->error($errors); | ||
| } | ||
|
|
||
| return $exitCode; | ||
| } |
There was a problem hiding this comment.
runProcess() is duplicated from BuildCommand with identical implementation. Consider extracting this into a shared helper/trait (or a small internal service) so process execution behavior stays consistent across commands and future fixes only need to be made once.
| public function handle(): int | ||
| { | ||
| $branch = $this->argument('branch'); | ||
| $commit = $this->argument('commit'); | ||
| $ref = $commit ?? $branch; | ||
| $isDryRun = (bool) $this->option('dry-run'); | ||
| $noRebuild = (bool) $this->option('no-rebuild'); | ||
|
|
||
| $repo = config('notur.repository', self::DEFAULT_REPO); | ||
| $noturRoot = base_path('vendor/notur/notur'); | ||
|
|
||
| $client = new Client([ | ||
| 'timeout' => 30, | ||
| 'connect_timeout' => 10, | ||
| 'headers' => [ | ||
| 'Accept' => 'application/vnd.github.v3+json', | ||
| 'User-Agent' => 'Notur-DevPull/1.0', | ||
| ], | ||
| ]); | ||
|
|
||
| // Step 1: Fetch commit info | ||
| $this->info("Fetching commit info for '{$ref}' from {$repo}..."); | ||
|
|
||
| try { | ||
| $commitInfo = $this->fetchCommitInfo($client, $repo, $ref); | ||
| } catch (\Throwable $e) { | ||
| $this->error("Failed to fetch commit info: {$e->getMessage()}"); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
There are integration tests for other Artisan commands under tests/Integration/Console/, but this new command has no coverage. Consider adding tests that fake the GitHub API responses and exercise at least the --dry-run path and error handling paths (e.g. invalid ref / download failure).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| private function fetchCommitInfo(Client $client, string $repo, string $ref): array | ||
| { | ||
| $url = self::GITHUB_API_BASE . "/repos/{$repo}/commits/{$ref}"; | ||
|
|
||
| try { | ||
| $response = $client->get($url); | ||
| } catch (GuzzleException $e) { |
There was a problem hiding this comment.
$ref is interpolated directly into the GitHub API URL. Branch names commonly contain slashes (e.g. feature/foo), which will break the /commits/{ref} path and cause 404s. URL-encode the ref (and ideally validate/sanitize it) before building the request URL.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| private function extractArchive(string $zipPath, string $targetDir): string | ||
| { | ||
| if (!is_dir($targetDir)) { | ||
| mkdir($targetDir, 0755, true); | ||
| } | ||
|
|
||
| $zip = new \ZipArchive(); | ||
| $result = $zip->open($zipPath); | ||
|
|
||
| if ($result !== true) { | ||
| throw new \RuntimeException("Failed to open zip archive (error code: {$result})"); | ||
| } | ||
|
|
||
| $zip->extractTo($targetDir); | ||
| $zip->close(); |
There was a problem hiding this comment.
ZipArchive::extractTo() is called on an untrusted zip without validating entry paths. A crafted archive could use path traversal (e.g. ../) to write outside the temp directory before files are copied into vendor/. Validate all zip entry names (no absolute paths, no .. segments) and/or extract entries manually to a controlled location.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Step 4: Extract to temp directory | ||
| $tmpDir = sys_get_temp_dir() . '/notur-dev-pull-extract-' . uniqid(); | ||
|
|
||
| try { | ||
| $innerDir = $this->extractArchive($tmpZip, $tmpDir); | ||
| } catch (\Throwable $e) { | ||
| $this->error("Extraction failed: {$e->getMessage()}"); | ||
| @unlink($tmpZip); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
If extraction fails, the temporary extraction directory created at $tmpDir is not cleaned up, which can leak directories in the system temp folder. Ensure the catch block deletes $tmpDir (when it exists) before returning.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| foreach ($preserveDirs as $dir) { | ||
| $dirPath = $noturRoot . '/' . $dir; | ||
| if (is_dir($dirPath)) { | ||
| if (!is_dir($tmpPreserve)) { | ||
| mkdir($tmpPreserve, 0755, true); | ||
| } | ||
| rename($dirPath, $tmpPreserve . '/' . $dir); | ||
| } | ||
| } | ||
|
|
||
| // Delete old vendor/notur/notur/ contents | ||
| if (is_dir($noturRoot)) { | ||
| $this->deleteDirectory($noturRoot); | ||
| } | ||
|
|
||
| // Copy new files | ||
| $this->copyDirectory($sourcePath, $noturRoot); | ||
|
|
||
| // Restore preserved directories | ||
| foreach ($preserveDirs as $dir) { | ||
| $tmpSource = $tmpPreserve . '/' . $dir; | ||
| if (is_dir($tmpSource)) { | ||
| $destPath = $noturRoot . '/' . $dir; | ||
| if (is_dir($destPath)) { | ||
| $this->deleteDirectory($destPath); | ||
| } | ||
| rename($tmpSource, $destPath); | ||
| } |
There was a problem hiding this comment.
rename() results are not checked when preserving/restoring vendor/ and node_modules/. If a rename fails (permissions, locks, etc.), the command can silently proceed and later delete/overwrite directories, potentially losing install artifacts. Check the return values and abort with a clear error if a preserve/restore move fails.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Add integration tests for DevPullCommand
URL-encode branch refs in DevPullCommand GitHub API requests
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Add error checking for rename() calls in DevPullCommand
Fix temp directory leak on extraction failure in DevPullCommand
- Removed the constructor dependency for the HTTP client and added a private method `resolveClient()` to handle client instantiation. - Moved validation for the Notur installation directory to occur before making changes, ensuring better error handling for directory existence and writability.
This pull request introduces a new development command for the Notur framework, allowing developers to pull unreleased commits directly from GitHub and update their local framework installation for testing. It also adds a new configuration option for specifying the GitHub repository and updates documentation to reflect these features and the new framework version.
New development features:
notur:dev:pullArtisan command (src/Console/Commands/DevPullCommand.php), which downloads a branch or commit from GitHub, replaces files invendor/notur/notur/, rebuilds the frontend bridge, and supports--no-rebuildand--dry-runoptions.DevPullCommandin the service provider (src/NoturServiceProvider.php) so it is available in Artisan. [1] [2]Configuration enhancements:
repositoryconfig key toconfig/notur.phpfor specifying the GitHub repository used bynotur:dev:pull(defaults tosak0a/notur).Documentation updates:
website/docs/admin/guide.md) to document the newnotur:dev:pullcommand and explain its usage and configuration. [1] [2]Version bump:
1.2.4inconfig/notur.phpand updated the command count in documentation. [1] [2]