Updated installer version discovery and format. #2161
Conversation
WalkthroughIntroduces a new immutable Artifact value object, moves repo/ref parsing and validation to artifact-centric flows, updates downloader interface and implementation to accept Artifact, adds git-ref validation, switches URI delimiter examples from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstallCmd as InstallCommand
participant Artifact
participant RepoDL as RepositoryDownloader
participant Validator
participant Remote as Remote Git host
participant LocalFS as Local filesystem
User->>InstallCmd: run installer (--uri or repo#ref)
InstallCmd->>Artifact: Artifact::fromUri(uri)
InstallCmd->>InstallCmd: store artifact, update config
InstallCmd->>RepoDL: validate(Artifact) (conditional)
RepoDL->>Validator: gitRef(artifact.getRef())
alt artifact.isRemote()
RepoDL->>Remote: HEAD getRepoUrl()
Remote-->>RepoDL: response (exists?/404)
alt non-default/ref
RepoDL->>Remote: check ref (commit/archive)
Remote-->>RepoDL: exists / not found
end
else artifact.isLocal()
RepoDL->>LocalFS: stat path & check .git
LocalFS-->>RepoDL: exists / missing / not-git
alt non-HEAD ref
RepoDL->>LocalFS: git rev-parse --verify ref
LocalFS-->>RepoDL: found / not found
end
end
RepoDL-->>InstallCmd: validation result or exception
InstallCmd->>User: prompt/continue
InstallCmd->>RepoDL: download(Artifact)
RepoDL->>Remote/LocalFS: create/archive & return path
RepoDL-->>InstallCmd: downloaded path/version
InstallCmd->>User: complete message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2161 +/- ##
===========================================
- Coverage 76.34% 76.00% -0.34%
===========================================
Files 110 104 -6
Lines 5702 5677 -25
Branches 44 0 -44
===========================================
- Hits 4353 4315 -38
- Misses 1349 1362 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.vortex/installer/src/Command/InstallCommand.php(2 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(1 hunks).vortex/installer/src/Utils/Validator.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(2 hunks).vortex/installer/tests/Unit/ValidatorTest.php(1 hunks)plan.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.vortex/installer/src/Command/InstallCommand.php (1)
.vortex/installer/src/Utils/Config.php (2)
get(80-82)Config(14-128)
🪛 GitHub Actions: Vortex - Test installer
.vortex/installer/src/Downloader/RepositoryDownloader.php
[error] 150-150: Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses (Drupal.Commenting.InlineComment.InvalidEndChar)
.vortex/installer/src/Utils/Validator.php
[error] 59-59: Documentation comment contains forbidden comment "- Cannot". (SlevomatCodingStandard.Commenting.ForbiddenComments.CommentForbidden)
[error] 63-63: Parameter tags must be defined first in a doc comment (Drupal.Commenting.DocComment.ParamNotFirst)
[error] 101-101: Expected 1 newline at end of file; 2 found (Drupal.Files.EndFileNewline.TooMany)
.vortex/installer/tests/Unit/ValidatorTest.php
[error] 244-244: Expected 1 newline at end of file; 2 found (Drupal.Files.EndFileNewline.TooMany)
🪛 markdownlint-cli2 (0.18.1)
plan.md
46-46: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
175-175: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
230-230: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
292-292: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
358-358: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
398-398: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
475-475: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
481-481: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
490-490: Bare URL used
(MD034, no-bare-urls)
491-491: Bare URL used
(MD034, no-bare-urls)
492-492: Bare URL used
(MD034, no-bare-urls)
493-493: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-common
🔇 Additional comments (10)
.vortex/installer/src/Command/InstallCommand.php (2)
101-118: Well-documented URI format examples.The help text comprehensively covers the supported URI formats including direct GitHub URLs,
#syntax, and@syntax. This aligns well with the Phase 2 implementation goals.
170-170: Improved hint message using configuration values.The hint now correctly references
Config::REPOandConfig::REFto display the download source, which is cleaner than the previous approach and aligns with the configuration-driven ref retrieval..vortex/installer/src/Utils/Validator.php (1)
69-99: Solid git reference validation implementation.The validation logic correctly implements git's reference naming rules:
- Accepts reserved keywords (
stable,HEAD)- Delegates to existing SHA validators for commit hashes
- Uses regex for general git ref pattern matching
- Explicitly rejects disallowed characters
.vortex/installer/tests/Unit/ValidatorTest.php (1)
170-242: Comprehensive test coverage for git reference validation.The data provider covers an excellent range of cases:
- Special keywords (
stable,HEAD)- Commit hashes (40-char and 7-char)
- Multiple versioning schemes (SemVer, CalVer, Drupal-style, hybrid)
- Branch names with various formats
- Invalid patterns with special characters and disallowed patterns
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (3)
504-590: Excellent test coverage for version tag formats.The test cases comprehensively cover:
- Semantic versioning (with/without
vprefix, pre-release variants)- Calendar versioning (YY.MM.PATCH and YYYY.MM.PATCH)
- Drupal-style versioning (
8.x-1.10,9.x-2.3)- Hybrid SemVer+CalVer format
- Branch names with slashes
591-680: Good coverage of GitHub URL patterns and alternative syntax.The test cases cover:
- Direct release URLs (
/releases/tag/{ref})- Tree URLs (
/tree/{ref}) including feature branches with slashes- Commit URLs (
/commit/{sha})- Alternative
#syntax for both HTTPS and SSHThis aligns well with the Phase 2 implementation for user-friendly URI formats.
766-789: Updated error messages reflect new validation.The error message format has been updated to:
"Invalid git reference: \"{ref}\". Reference must be a valid git tag, branch, or commit hash."This is more descriptive and helpful for users than the previous format.
plan.md (1)
1-6: Well-structured implementation plan.The plan document provides excellent context for the PR, clearly documenting:
- Phase 1 completion status with implemented features
- Phase 2 requirements and implementation approach
- Supported URI formats and versioning schemes
- Backward compatibility considerations
.vortex/installer/src/Downloader/RepositoryDownloader.php (2)
63-74: LGTM! Clean GitHub pattern detection with proper validation.The early detection of GitHub-specific patterns before falling back to standard parsing is a good design choice. The extracted reference is properly validated before returning.
127-141: Regex patterns are permissive but downstream validation and escaping mitigate the concern.While the patterns use
(.+)to capture refs, the extracted ref is validated byValidator::gitRef()(line 38 of thedownload()method), which restricts allowed characters to[a-zA-Z0-9._\-\/]. Additionally, inGitDownloader.phpthe ref is passed to Symfony'sProcessclass as an array element (['git', 'clone', '--branch', $ref, ...]), which automatically escapes arguments usingescapeshellarg()internally. No raw shell command construction exists in the codebase, so shell injection through refs is not a security risk.
| /** | ||
| * Validate a git reference (tag, branch, or commit). | ||
| * | ||
| * Accepts any valid git reference format including: | ||
| * - Special keywords: "stable", "HEAD" | ||
| * - Commit hashes: 40-character or 7-character SHA-1 hashes | ||
| * - Version tags: "1.2.3", "v1.2.3", "25.11.0", "1.0.0-2025.11.0" | ||
| * - Drupal-style tags: "8.x-1.10", "9.x-2.3" | ||
| * - Pre-release tags: "1.x-rc1", "2.0.0-beta" | ||
| * - Branch names: "main", "develop", "feature/my-feature" | ||
| * | ||
| * Follows git reference naming rules: | ||
| * - Can contain alphanumeric, dot, hyphen, underscore, slash | ||
| * - Cannot start with dot or hyphen | ||
| * - Cannot contain: @, ^, ~, :, ?, *, [, space, \, @{ | ||
| * - Cannot end with .lock or contain .. | ||
| * | ||
| * @see https://git-scm.com/docs/git-check-ref-format | ||
| * | ||
| * @param string $value | ||
| * The reference string to validate. | ||
| * | ||
| * @return bool | ||
| * TRUE if valid, FALSE otherwise. | ||
| */ |
There was a problem hiding this comment.
Fix PHPDoc to resolve pipeline failures.
The pipeline reports two issues:
- Line 59: Documentation contains forbidden comment pattern "- Cannot"
- Line 63: Parameter tags must be defined first in doc comment
Restructure the PHPDoc to place @param and @return before the detailed description, and rephrase the forbidden pattern.
/**
* Validate a git reference (tag, branch, or commit).
*
+ * @param string $value
+ * The reference string to validate.
+ *
+ * @return bool
+ * TRUE if valid, FALSE otherwise.
+ *
* Accepts any valid git reference format including:
* - Special keywords: "stable", "HEAD"
* - Commit hashes: 40-character or 7-character SHA-1 hashes
* - Version tags: "1.2.3", "v1.2.3", "25.11.0", "1.0.0-2025.11.0"
* - Drupal-style tags: "8.x-1.10", "9.x-2.3"
* - Pre-release tags: "1.x-rc1", "2.0.0-beta"
* - Branch names: "main", "develop", "feature/my-feature"
*
* Follows git reference naming rules:
* - Can contain alphanumeric, dot, hyphen, underscore, slash
- * - Cannot start with dot or hyphen
- * - Cannot contain: @, ^, ~, :, ?, *, [, space, \, @{
- * - Cannot end with .lock or contain ..
+ * - Must not start with dot or hyphen
+ * - Must not contain: @, ^, ~, :, ?, *, [, space, \, @{
+ * - Must not end with .lock or contain ..
*
* @see https://git-scm.com/docs/git-check-ref-format
- *
- * @param string $value
- * The reference string to validate.
- *
- * @return bool
- * TRUE if valid, FALSE otherwise.
*/🧰 Tools
🪛 GitHub Actions: Vortex - Test installer
[error] 59-59: Documentation comment contains forbidden comment "- Cannot". (SlevomatCodingStandard.Commenting.ForbiddenComments.CommentForbidden)
[error] 63-63: Parameter tags must be defined first in a doc comment (Drupal.Commenting.DocComment.ParamNotFirst)
🤖 Prompt for AI Agents
.vortex/installer/src/Utils/Validator.php lines 44-68: The PHPDoc block must
place @param and @return tags before the long descriptive detail and avoid the
forbidden comment pattern "- Cannot"; move the @param string $value and @return
bool tags directly after the short description summary, then follow with the
detailed list of accepted formats and rules, and rephrase any lines starting
with "- Cannot" to use allowed wording such as "Must not" or "Must not contain"
to eliminate the forbidden pattern.
| return TRUE; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Remove extra newline at end of file.
Pipeline failure indicates there are 2 newlines at EOF but only 1 is expected.
return TRUE;
}
}
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| return TRUE; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Vortex - Test installer
[error] 101-101: Expected 1 newline at end of file; 2 found (Drupal.Files.EndFileNewline.TooMany)
🤖 Prompt for AI Agents
.vortex/installer/src/Utils/Validator.php around line 101: the file currently
ends with two newline characters which fails the pipeline; remove the extra
blank line so the file ends with exactly one newline character (ensure there's a
single trailing newline and no additional empty lines at EOF), save the file,
and re-run the linter/CI.
| ]; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Remove extra newline at end of file.
Pipeline failure indicates there are 2 newlines at EOF but only 1 is expected.
}
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Vortex - Test installer
[error] 244-244: Expected 1 newline at end of file; 2 found (Drupal.Files.EndFileNewline.TooMany)
🤖 Prompt for AI Agents
.vortex/installer/tests/Unit/ValidatorTest.php around line 244: the file
currently has two trailing newlines at EOF; edit the file to remove the extra
blank line so the file ends with exactly one newline character, then save the
file to ensure only a single newline terminator remains.
plan.md
Outdated
| ## Success Criteria | ||
|
|
||
| ### Phase 1 (COMPLETE) ✅ | ||
| - [x] Users can specify version tags using `@ref` syntax | ||
| - [x] Common tag formats work: `1.2.3`, `v1.2.3`, `25.11.0`, etc. | ||
| - [x] All existing tests pass (1093 tests, 1561 assertions) | ||
| - [x] Documentation updated with examples | ||
|
|
||
| ### Phase 2 (PENDING) ⏳ | ||
| - [ ] Users can paste GitHub URLs directly from browser | ||
| - [ ] Alternative `#ref` syntax works (shell-safe) | ||
| - [ ] All new URL patterns pass tests | ||
| - [ ] Help text updated with new examples | ||
| - [ ] Full backward compatibility maintained |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add blank lines around headings for markdown consistency.
Static analysis indicates missing blank lines below headings at lines 475 and 481.
### Phase 1 (COMPLETE) ✅
+
- [x] Users can specify version tags using `@ref` syntax
- [x] Common tag formats work: `1.2.3`, `v1.2.3`, `25.11.0`, etc.
- [x] All existing tests pass (1093 tests, 1561 assertions)
- [x] Documentation updated with examples
### Phase 2 (PENDING) ⏳
+
- [ ] Users can paste GitHub URLs directly from browser📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Success Criteria | |
| ### Phase 1 (COMPLETE) ✅ | |
| - [x] Users can specify version tags using `@ref` syntax | |
| - [x] Common tag formats work: `1.2.3`, `v1.2.3`, `25.11.0`, etc. | |
| - [x] All existing tests pass (1093 tests, 1561 assertions) | |
| - [x] Documentation updated with examples | |
| ### Phase 2 (PENDING) ⏳ | |
| - [ ] Users can paste GitHub URLs directly from browser | |
| - [ ] Alternative `#ref` syntax works (shell-safe) | |
| - [ ] All new URL patterns pass tests | |
| - [ ] Help text updated with new examples | |
| - [ ] Full backward compatibility maintained | |
| ## Success Criteria | |
| ### Phase 1 (COMPLETE) ✅ | |
| - [x] Users can specify version tags using `@ref` syntax | |
| - [x] Common tag formats work: `1.2.3`, `v1.2.3`, `25.11.0`, etc. | |
| - [x] All existing tests pass (1093 tests, 1561 assertions) | |
| - [x] Documentation updated with examples | |
| ### Phase 2 (PENDING) ⏳ | |
| - [ ] Users can paste GitHub URLs directly from browser | |
| - [ ] Alternative `#ref` syntax works (shell-safe) | |
| - [ ] All new URL patterns pass tests | |
| - [ ] Help text updated with new examples | |
| - [ ] Full backward compatibility maintained |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
475-475: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
481-481: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In plan.md around lines 473-486, the Markdown headings "### Phase 1 (COMPLETE)
✅" (line ~475) and "### Phase 2 (PENDING) ⏳" (line ~481) lack the required
blank-line spacing; add a single blank line after each heading (and ensure there
is a blank line before each heading as well) so that there is exactly one empty
line separating headings from surrounding content for consistent Markdown
formatting.
plan.md
Outdated
| ## References | ||
|
|
||
| - **GitHub Issue:** https://github.com/drevops/vortex/issues/2162 | ||
| - **Git reference naming:** https://git-scm.com/docs/git-check-ref-format | ||
| - **GitHub Archive API:** https://docs.github.com/en/rest/repos/contents#download-a-repository-archive-tar | ||
| - **GitHub Releases API:** https://docs.github.com/en/rest/releases/releases |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider formatting bare URLs as markdown links.
The References section contains bare URLs which trigger markdownlint warnings. Consider formatting them as proper markdown links for consistency.
## References
-- **GitHub Issue:** https://github.com/drevops/vortex/issues/2162
-- **Git reference naming:** https://git-scm.com/docs/git-check-ref-format
-- **GitHub Archive API:** https://docs.github.com/en/rest/repos/contents#download-a-repository-archive-tar
-- **GitHub Releases API:** https://docs.github.com/en/rest/releases/releases
+- **GitHub Issue:** <https://github.com/drevops/vortex/issues/2162>
+- **Git reference naming:** <https://git-scm.com/docs/git-check-ref-format>
+- **GitHub Archive API:** <https://docs.github.com/en/rest/repos/contents#download-a-repository-archive-tar>
+- **GitHub Releases API:** <https://docs.github.com/en/rest/releases/releases>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## References | |
| - **GitHub Issue:** https://github.com/drevops/vortex/issues/2162 | |
| - **Git reference naming:** https://git-scm.com/docs/git-check-ref-format | |
| - **GitHub Archive API:** https://docs.github.com/en/rest/repos/contents#download-a-repository-archive-tar | |
| - **GitHub Releases API:** https://docs.github.com/en/rest/releases/releases | |
| ## References | |
| - **GitHub Issue:** <https://github.com/drevops/vortex/issues/2162> | |
| - **Git reference naming:** <https://git-scm.com/docs/git-check-ref-format> | |
| - **GitHub Archive API:** <https://docs.github.com/en/rest/repos/contents#download-a-repository-archive-tar> | |
| - **GitHub Releases API:** <https://docs.github.com/en/rest/releases/releases> |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
490-490: Bare URL used
(MD034, no-bare-urls)
491-491: Bare URL used
(MD034, no-bare-urls)
492-492: Bare URL used
(MD034, no-bare-urls)
493-493: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In plan.md around lines 488 to 493, the References section uses bare URLs which
trigger markdownlint warnings; replace each bare URL with a proper markdown link
using the [text](url) format (e.g., use descriptive link text like "GitHub Issue
#2162", "Git reference naming", "GitHub Archive API", "GitHub Releases API") so
the references are consistent and lint-clean.
AlexSkrypnyk
force-pushed
the
feature/update-installer-downloader-version
branch
from
253abf1 to
96f8102
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/src/Command/InstallCommand.php (1)
121-121: Inconsistent delimiter in option description.The
--urioption description still references the old@delimiter, but all examples and documentation now use#.Apply this diff to fix the inconsistency:
- $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after @.'); + $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after #.');
♻️ Duplicate comments (4)
.vortex/installer/src/Utils/Validator.php (2)
44-68: PHPDoc structure needs correction per pipeline failures.The pipeline reports two issues that need to be fixed:
- Line 59: Forbidden comment pattern "- Cannot" should be rephrased to "- Must not"
- Line 63:
@paramand@returntags must be defined first in the doc comment, before the detailed description
101-102: Remove extra newline at EOF.Pipeline failure indicates 2 newlines at EOF but only 1 is expected.
.vortex/installer/tests/Unit/ValidatorTest.php (1)
248-249: Remove extra newline at EOF.Pipeline failure indicates 2 newlines at EOF but only 1 is expected.
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
150-150: Fix missing punctuation to satisfy linter.Apply this diff:
- // Pattern 5: git@...#{ref} (SSH) - alternative to @ syntax + // Pattern 5: git@...#{ref} (SSH) - alternative to @ syntax.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.vortex/installer/src/Command/InstallCommand.php(2 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(5 hunks).vortex/installer/src/Utils/Validator.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(13 hunks).vortex/installer/tests/Unit/ValidatorTest.php(1 hunks).vortex/tests/bats/unit/update-vortex.bats(8 hunks)scripts/vortex/update-vortex.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/installer/src/Utils/Validator.php
🧬 Code graph analysis (3)
.vortex/installer/src/Command/InstallCommand.php (2)
.vortex/installer/src/Utils/Config.php (2)
get(80-82)Config(14-128).vortex/installer/src/Utils/Env.php (1)
get(18-22)
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (1)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(17-458)
.vortex/installer/src/Downloader/RepositoryDownloader.php (3)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-101)gitRef(69-99).vortex/installer/src/Utils/Env.php (1)
Env(7-247).vortex/installer/src/Utils/Git.php (1)
Git(11-117)
🪛 GitHub Actions: Vortex - Test installer
.vortex/installer/tests/Unit/ValidatorTest.php
[error] 248-248: Expected 1 newline at end of file; 2 found (Drupal.Files.EndFileNewline.TooMany)
.vortex/installer/src/Utils/Validator.php
[error] 59-59: Documentation comment contains forbidden comment "- Cannot end with .lock or contain .." (SlevomatCodingStandard.Commenting.ForbiddenComments.CommentForbidden)
[error] 63-63: Parameter tags must be defined first in a doc comment (Drupal.Commenting.DocComment.ParamNotFirst)
[error] 101-101: Expected 1 newline at end of file; 2 found (Drupal.Files.EndFileNewline.TooMany)
.vortex/installer/src/Downloader/RepositoryDownloader.php
[error] 150-150: Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses (Drupal.Commenting.InlineComment.InvalidEndChar)
[warning] 398-398: Line exceeds 80 characters; contains 81 characters (Drupal.Files.LineLength.TooLong)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
🔇 Additional comments (22)
.vortex/installer/src/Utils/Validator.php (1)
69-99: LGTM on the gitRef validation logic.The implementation correctly handles:
- Reserved keywords (
stable,HEAD)- Commit hash delegation to existing validators
- Git ref naming rules via regex (no leading dot/hyphen, no
.., no.locksuffix)- Disallowed character checking
scripts/vortex/update-vortex.sh (1)
14-29: LGTM on delimiter change from@to#.The documentation examples and default value have been consistently updated to use the
#delimiter for git refs. The examples clearly demonstrate all supported URI formats (remote HTTPS, local file://, local path) with optional refs..vortex/installer/src/Command/InstallCommand.php (2)
95-115: LGTM on the updated help text.The help text now clearly demonstrates:
- Interactive and non-interactive installation examples
- URI syntax with
#delimiter for git refs- Direct GitHub URL support (releases, tree, commit pages)
166-167: LGTM on the hint update.The hint now correctly displays "ref" instead of "commit", which is more accurate since it can be a tag, branch, or commit hash.
.vortex/tests/bats/unit/update-vortex.bats (6)
11-34: LGTM on default behavior test.Test correctly validates that the default URI uses
#stabledelimiter.
36-58: LGTM on custom repository test.Test correctly passes and validates custom repository URI with
#mainref.
60-87: LGTM on local installer path test.Test correctly validates local installer execution with default
#stableref.
135-157: LGTM on local path test.Test correctly validates local path with
#stableref appended.
159-181: LGTM on git SSH URL test.Test correctly validates git SSH URL format with
#v1.2.3ref.
207-253: LGTM on interactive mode tests.Tests correctly validate interactive mode execution with and without custom repository URIs using the
#delimiter..vortex/installer/tests/Unit/ValidatorTest.php (1)
170-246: LGTM on the comprehensive gitRef test coverage.The data provider covers an excellent range of test cases:
- Special keywords (
stable,HEAD)- Full and short commit hashes
- Semantic versioning variants (with/without
vprefix, pre-release, build metadata)- Calendar versioning (CalVer)
- Drupal-style versioning (
8.x-1.10)- Hybrid SemVer+CalVer formats
- Branch names with various conventions
- Comprehensive invalid input testing
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (3)
233-234: LGTM! Error message matches the implementation.The updated error message correctly reflects the validation logic added in
validateLocalRefExists()in the implementation file.
248-254: Verify the two-call pattern is consistently applied.The test correctly expects two HTTP calls for 'stable' ref downloads: a HEAD request to validate repository existence and a GET request to fetch releases. Ensure this pattern is documented and that the validation call is performant for users without tokens.
822-830: LGTM! Error message aligns with new validation.The test correctly expects "Unable to access repository" when a RequestException occurs, which matches the error message thrown by the new
validateRemoteRepositoryExists()method at line 373 of the implementation..vortex/installer/src/Downloader/RepositoryDownloader.php (8)
63-74: LGTM! GitHub pattern detection with validation.The two-phase approach correctly prioritizes GitHub-specific URL patterns before falling back to standard
#refparsing. The validation withValidator::gitRef()ensures extracted references are valid.
76-110: LGTM! Consistent # delimiter parsing across all URI schemes.The fallback parsing correctly handles
#refsyntax for HTTPS, SSH, file://, and local paths using tilde-delimited regex patterns. The validation at line 106-108 ensures all references are valid before returning.
164-183: LGTM! Appropriate pre-flight validation for remote downloads.The validation logic correctly:
- Validates repository existence upfront for all refs
- Validates ref existence only for non-special refs (excluding 'stable' and 'HEAD')
- Avoids redundant validation for 'stable' refs that are resolved via the releases API
200-216: LGTM! Consistent validation pattern for local downloads.The local validation mirrors the remote validation pattern:
- Repository existence checked upfront
- Ref existence validated only for non-HEAD refs
- Clean separation between special refs and user-provided refs
377-412: LGTM! Smart validation using archive URL.The method cleverly validates ref existence by checking the archive URL with a HEAD request, distinguishing between 404 (ref not found) and other errors. This approach is efficient and provides clear error messages.
414-431: LGTM! Straightforward local repository validation.The method performs two essential checks:
- Path exists and is a directory
- Path contains a .git folder (indicating it's a git repository)
Error messages are clear and actionable.
433-456: LGTM! Proper local ref validation using git.The method correctly uses
git rev-parse --verifyto check if a reference exists in the local repository. The error handling provides clear context when a ref is not found.
346-375: LGTM! Efficient repository existence check.The method uses a HEAD request to verify repository accessibility before attempting downloads. The implementation correctly handles authentication via GitHub tokens and provides clear error messages. HEAD requests to GitHub web URLs reliably return appropriate status codes (200 for existing repositories, 404 for non-existent ones), confirming this approach is sound.
| 'Empty reference defaults to HEAD - # is captured in repo part' => [ | ||
| '/path/to/repo#', | ||
| '/path/to/repo#', | ||
| RepositoryDownloader::REF_HEAD, | ||
| ], |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider rejecting empty reference delimiter.
The edge case /path/to/repo# treats the trailing # as part of the repository path rather than an empty reference. While this behavior is technically consistent with the regex, it could confuse users who might expect an error for malformed input.
Consider adding validation to reject URIs ending with # and provide a clearer error message.
+ // Reject trailing # without ref.
+ if (str_ends_with($src, '#')) {
+ throw new \RuntimeException(sprintf('Invalid reference format: "%s". Reference cannot be empty after #.', $src));
+ }
+
// Try GitHub-specific patterns first (Phase 2).Committable suggestion skipped: line range outside the PR's diff.
AlexSkrypnyk
force-pushed
the
feature/update-installer-downloader-version
branch
from
96f8102 to
2b09776
Compare
AlexSkrypnyk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/src/Command/InstallCommand.php (1)
121-121: Inconsistent delimiter in option description.The help text (lines 95-114) correctly shows
#as the delimiter, but this option description still references@. This will confuse users.Apply this diff:
- $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after @.'); + $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after #.');
♻️ Duplicate comments (3)
.vortex/installer/src/Utils/Validator.php (1)
44-68: Fix PHPDoc structure and incomplete documentation.The PHPDoc has two issues:
- Line 59 has incomplete text: "- Cannot end with .lock or contain" is truncated
- Per Drupal coding standards,
@paramand@returntags should be defined first, and "- Cannot" patterns may be flagged as forbidden commentsApply this diff to fix:
/** * Validate a git reference (tag, branch, or commit). * + * @param string $value + * The reference string to validate. + * + * @return bool + * TRUE if valid, FALSE otherwise. + * * Accepts any valid git reference format including: * - Special keywords: "stable", "HEAD" * - Commit hashes: 40-character or 7-character SHA-1 hashes * - Version tags: "1.2.3", "v1.2.3", "25.11.0", "1.0.0-2025.11.0" * - Drupal-style tags: "8.x-1.10", "9.x-2.3" * - Pre-release tags: "1.x-rc1", "2.0.0-beta" * - Branch names: "main", "develop", "feature/my-feature" * * Follows git reference naming rules: * - Can contain alphanumeric, dot, hyphen, underscore, slash - * - Cannot start with dot or hyphen - * - Cannot contain: @, ^, ~, :, ?, *, [, space, \, @{ - * - Cannot end with .lock or contain + * - Must not start with dot or hyphen + * - Must not contain: @, ^, ~, :, ?, *, [, space, \, @{ + * - Must not end with .lock or contain .. * - * @param string $value - * The reference string to validate. - * - * @return bool - * TRUE if valid, FALSE otherwise. - * * @see https://git-scm.com/docs/git-check-ref-format */.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (1)
498-502: Consider addressing the edge case for trailing#.The test shows that
/path/to/repo#treats the trailing#as part of the repository path rather than an empty reference. While this behavior is consistent with the regex, it could confuse users who might expect an error for malformed input likerepo.git#(empty ref after delimiter).This was flagged in a previous review. The suggestion to reject URIs ending with
#and provide a clearer error message still stands if you want to improve UX..vortex/installer/src/Downloader/RepositoryDownloader.php (1)
126-158: Method name doesn't fully reflect broader scope (patterns 4-5).Patterns 1-3 are truly GitHub-specific, but patterns 4-5 (
*.git#refandgit@...#ref) work with any git hosting service. The method namedetectGitHubUrlPatternis slightly misleading for these cases.This was flagged in a previous review as a nitpick. The functionality is correct; this is purely a naming suggestion for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.vortex/installer/src/Command/InstallCommand.php(5 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(6 hunks).vortex/installer/src/Runner/ProcessRunner.php(0 hunks).vortex/installer/src/Utils/Validator.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(14 hunks).vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php(2 hunks).vortex/installer/tests/Unit/ValidatorTest.php(1 hunks).vortex/tests/bats/unit/update-vortex.bats(8 hunks)scripts/vortex/update-vortex.sh(1 hunks)
💤 Files with no reviewable changes (1)
- .vortex/installer/src/Runner/ProcessRunner.php
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/installer/src/Utils/Validator.php
🧬 Code graph analysis (5)
.vortex/tests/bats/unit/update-vortex.bats (1)
.vortex/installer/src/Runner/ProcessRunner.php (1)
run(21-76)
.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(17-461)
.vortex/installer/src/Command/InstallCommand.php (1)
.vortex/installer/src/Downloader/RepositoryDownloader.php (6)
RepositoryDownloader(17-461)parseUri(64-113)validateRemoteRepositoryExists(357-377)validateRemoteRefExists(390-415)validateLocalRepositoryExists(426-434)validateLocalRefExists(447-459)
.vortex/installer/tests/Unit/ValidatorTest.php (1)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-101)gitRef(69-99)
.vortex/installer/src/Downloader/RepositoryDownloader.php (2)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-101)gitRef(69-99).vortex/installer/src/Utils/Env.php (1)
Env(7-247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
🔇 Additional comments (22)
.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
8-8: LGTM! Good use of centralized constant.Using
RepositoryDownloader::DEFAULT_REPOto derive the badge URL ensures test fixtures remain consistent with the actual default repository configuration. Thestr_replace('.git', '', ...)approach is appropriate here since the constant is a controlled GitHub URL.Also applies to: 270-271
scripts/vortex/update-vortex.sh (1)
14-29: LGTM! Delimiter change is consistent and well-documented.The switch from
@to#for the repository/ref delimiter is properly reflected in both the documentation examples and the default value. The#character is a better choice as it avoids conflicts with git SSH URL syntax (git@github.com:...)..vortex/installer/src/Utils/Validator.php (1)
69-99: Implementation is correct and well-structured.The validation logic properly handles:
- Reserved keywords (
stable,HEAD)- Commit hash delegation to existing validators
- Comprehensive regex for git ref naming rules
- Explicit disallowed character checks
Minor note: The PHPDoc mentions the pattern can contain "alphanumeric, dot, hyphen, underscore, slash" but the regex also allows
+(line 84). Consider updating line 56 to include "plus" for completeness..vortex/tests/bats/unit/update-vortex.bats (4)
20-25: LGTM! Default value test correctly updated.The test properly verifies that the default template repository URI now uses
#stableinstead of@stable.
44-57: LGTM! Custom repository test updated consistently.Both the expected mock invocation and the actual script argument correctly use the
#maindelimiter.
167-180: LGTM! Git SSH URL test validates the delimiter doesn't conflict.Good test case - the
#delimiter (git@github.com:drevops/vortex.git#v1.2.3) avoids ambiguity with the@in git SSH URLs, which was a key motivation for this change.
72-73: LGTM! Remaining tests consistently updated.All other test cases (local installer path, local path URI, failure handling, and interactive modes) correctly reflect the
#delimiter change.Also applies to: 143-151, 191-193, 215-247
.vortex/installer/tests/Unit/ValidatorTest.php (1)
170-173: LGTM! Test method correctly delegates to the Validator.The test method follows the established pattern in this file, using a data provider for comprehensive coverage.
.vortex/installer/src/Command/InstallCommand.php (3)
147-160: LGTM!The validation flow is well-structured, using
Task::actionconsistently with the rest of the codebase and providing clear feedback to the user about what's being validated.
735-755: LGTM!The logic correctly determines when validation should be skipped—only when using the default Vortex repository with default refs (
stableorHEAD). The handling of the.gitsuffix variation is appropriate.
766-800: LGTM!The validation method correctly distinguishes between remote and local repositories, handles the
.gitsuffix appropriately, and properly skips validation for special refs (stable,HEAD) where early validation isn't meaningful. The logic for mappingstabletoHEADfor local repos aligns with howdownloadFromLocalhandles this case..vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (3)
248-268: LGTM!The test correctly verifies that GitHub token authentication is applied to both HTTP calls (HEAD for repository validation and GET for releases API), ensuring consistent authorization across the validation and discovery flow.
997-1078: LGTM!Comprehensive test coverage for the new validation methods. The tests properly cover:
- HTTP 404 responses for missing repositories and refs
- Non-existent local paths and non-git directories
- Network errors via
RequestExceptionThe mock setup correctly simulates the different HTTP call patterns.
849-897: LGTM!Excellent test coverage for the SemVer+CalVer hybrid versioning format, including draft skipping and mixed format scenarios. This ensures the release discovery correctly handles the project's versioning scheme.
.vortex/installer/src/Downloader/RepositoryDownloader.php (8)
23-24: LGTM!The
DEFAULT_REPOconstant provides a single source of truth for the default repository URL, improving maintainability and consistency across the codebase.
64-113: LGTM!The
parseUrirefactoring is well-structured:
- Tries specialized GitHub URL patterns first for better UX with copy-pasted URLs
- Falls back to standard
#refparsing with scheme-specific regexes- Validates all extracted refs through
Validator::gitRef()- Provides clear error messages guiding users to the correct syntax
160-195: LGTM!The pre-flight validation flow is well-designed:
- Repository existence is validated first
- For
stable, the discovered release tag is implicitly valid (from API)- For
HEAD, no ref validation is needed- For custom refs, validation prevents confusing archive download errors
This provides better user experience by failing early with clear error messages.
197-226: LGTM!The local download flow mirrors the remote flow with appropriate local-specific validations:
- Checks path existence and
.gitdirectory- Maps
stabletoHEADfor local repos- Validates non-HEAD refs via
git rev-parse
357-377: LGTM!Efficient implementation using HEAD request to verify repository accessibility without downloading content. Proper error handling with informative messages including HTTP status codes, and GitHub token support for private repositories.
390-415: LGTM!Smart approach using the archive URL to validate ref existence—this validates the exact resource that will be downloaded later. Clear differentiation between 404 (ref not found) and other HTTP errors provides actionable error messages.
426-434: LGTM!Simple and effective validation for local repositories. The two-step check (directory exists,
.gitsubdirectory exists) covers the common case well.
447-459: LGTM!Correct use of
git rev-parse --verifyto validate local refs. The method properly reuses existing Git instances and provides clear error messages when refs don't exist.
AlexSkrypnyk
force-pushed
the
feature/update-installer-downloader-version
branch
from
2b09776 to
858b927
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/src/Command/InstallCommand.php (1)
121-121: Update the option description to reflect the new delimiter.The help text examples (lines 95-115) demonstrate the
#delimiter, but this option description still references@. This inconsistency may confuse users.Apply this diff:
- $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after @.'); + $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after #.');
♻️ Duplicate comments (4)
.vortex/installer/src/Utils/Validator.php (1)
44-68: PHPDoc structure violates Drupal coding standards.The
@paramand@returntags must be defined first in the doc comment (after the summary), not after the detailed description. Additionally, lines 57-59 still use the forbidden "Cannot" pattern, and line 59 appears truncated ("Cannot end with .lock or contain" - missing the rest)./** * Validate a git reference (tag, branch, or commit). * + * @param string $value + * The reference string to validate. + * + * @return bool + * TRUE if valid, FALSE otherwise. + * * Accepts any valid git reference format including: * - Special keywords: "stable", "HEAD" * - Commit hashes: 40-character or 7-character SHA-1 hashes * - Version tags: "1.2.3", "v1.2.3", "25.11.0", "1.0.0-2025.11.0" * - Drupal-style tags: "8.x-1.10", "9.x-2.3" * - Pre-release tags: "1.x-rc1", "2.0.0-beta" * - Branch names: "main", "develop", "feature/my-feature" * * Follows git reference naming rules: * - Can contain alphanumeric, dot, hyphen, underscore, slash - * - Cannot start with dot or hyphen - * - Cannot contain: @, ^, ~, :, ?, *, [, space, \, @{ - * - Cannot end with .lock or contain + * - Must not start with dot or hyphen + * - Must not contain: @, ^, ~, :, ?, *, [, space, \, @{ + * - Must not end with .lock or contain .. * * @see https://git-scm.com/docs/git-check-ref-format - * - * @param string $value - * The reference string to validate. - * - * @return bool - * TRUE if valid, FALSE otherwise. */.vortex/installer/tests/Unit/ValidatorTest.php (1)
236-245: Add missing edge case tests for invalid refs with slashes.Per
git check-ref-format, refs with trailing slashes or consecutive slashes are invalid. Adding these test cases would expose the current validation gap:'invalid containing ..' => ['invalid..ref', FALSE], + 'invalid trailing slash' => ['feature/', FALSE], + 'invalid consecutive slashes' => ['feature//name', FALSE], // Empty and edge cases. 'invalid empty string' => ['', FALSE],.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (1)
498-502: Consider validating empty ref after#delimiter.The test case
'Empty reference defaults to HEAD - # is captured in repo part'shows that/path/to/repo#treats the trailing#as part of the repository path rather than as an empty reference. While consistent with the regex behavior, this could confuse users expecting an error for malformed input..vortex/installer/src/Downloader/RepositoryDownloader.php (1)
115-158: Method name issue previously flagged.Past review comments noted that
detectGitHubUrlPatternis misleading because patterns 4 and 5 (\.git#refandgit@...#ref) work with any git hosting service, not just GitHub. Consider the previously suggested refactoring to split GitHub-specific patterns (1-3) from generic#refsyntax patterns (4-5).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.vortex/installer/src/Command/InstallCommand.php(5 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(6 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Utils/Validator.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(14 hunks).vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php(2 hunks).vortex/installer/tests/Unit/ValidatorTest.php(1 hunks).vortex/tests/bats/unit/update-vortex.bats(8 hunks)scripts/vortex/update-vortex.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/installer/tests/Unit/ValidatorTest.php
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/installer/src/Utils/Validator.php
🧬 Code graph analysis (4)
.vortex/tests/bats/unit/update-vortex.bats (1)
.vortex/installer/src/Runner/ProcessRunner.php (1)
run(21-76)
.vortex/installer/tests/Unit/ValidatorTest.php (1)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-101)gitRef(69-99)
.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(17-461)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-101)gitRef(69-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-common
🔇 Additional comments (16)
.vortex/installer/src/Runner/ProcessRunner.php (1)
49-56: Tighten iterable handling and newline consistency in Process callbackThe new coercion covers strings, but the generic
(array) $bufferfallback is risky given the declaredstring|iterabletype:
- If
$bufferis a non-arrayTraversable(e.g., a Generator), casting to array will use object properties rather than iterated values, so the logged/output content may be wrong.- You’re also using
"\n"here while earlier you build input withPHP_EOL; not a bug, but slightly inconsistent cross‑platform.You could make this more robust and explicit:
$buffer = is_string($buffer) ? $buffer : implode( PHP_EOL, is_array($buffer) ? $buffer : iterator_to_array($buffer) );Optionally, if in practice the callback only ever receives
stringfromSymfony\Component\Process\Process, you might narrow the union or document whyiterableis needed, to keep types aligned with the underlying library’s contract.scripts/vortex/update-vortex.sh (1)
14-29: LGTM!The documentation and default value consistently use the new
#delimiter for URI/ref separation, aligning with the broader PR changes..vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
8-8: LGTM!Deriving the badge URL from
RepositoryDownloader::DEFAULT_REPOensures consistency and avoids hardcoded duplication.Also applies to: 270-271
.vortex/tests/bats/unit/update-vortex.bats (3)
20-28: LGTM!Test expectations correctly updated to use the
#delimiter, maintaining coverage for the default installer flow.
44-57: LGTM!Custom template repository test properly updated with
#mainsyntax in both the assertion and the test invocation.
167-180: LGTM!Git SSH URL test correctly uses the new
#v1.2.3syntax for version reference..vortex/installer/tests/Unit/ValidatorTest.php (1)
170-173: LGTM!Test method correctly delegates to
Validator::gitRefand asserts expected boolean outcomes..vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (4)
248-254: LGTM!Mock expectations correctly updated to expect two HTTP calls (HEAD for repository validation, GET for releases API), with proper authorization header verification on both.
504-589: LGTM!Comprehensive test coverage for version tag formats including SemVer, CalVer, Drupal-style, hybrid versioning, pre-release tags, and branch names with the new
#delimiter syntax.
591-680: LGTM!GitHub URL pattern tests properly cover release URLs, tree URLs, commit URLs, and the alternative
#syntax for both HTTPS and SSH formats.
997-1078: LGTM!New validation tests provide good coverage for error scenarios:
- Remote repository not found (404)
- Remote ref not found (404)
- Local path doesn't exist
- Local path is not a git repository
- Request exceptions for both repo and ref validation
These tests ensure robust error handling and user-friendly error messages.
.vortex/installer/src/Command/InstallCommand.php (2)
147-156: LGTM! Well-structured validation flow.The pre-install validation appropriately skips default configurations and provides early feedback for custom repositories or references. The use of
Task::actionkeeps the validation consistent with the rest of the installation flow.
735-800: LGTM! Validation logic is correct and handles edge cases well.The implementation correctly:
- Skips validation for default repo + default ref combinations
- Handles both remote and local repositories
- Converts 'stable' to 'HEAD' for local repos (line 793)
- Validates non-special refs appropriately
.vortex/installer/src/Downloader/RepositoryDownloader.php (3)
64-113: LGTM! parseUri enhancement is well-implemented.The two-phase approach (GitHub-specific patterns first, then standard parsing) provides excellent UX by supporting URLs copied directly from browsers while maintaining backward compatibility with standard git syntax. The validation via
Validator::gitRefensures only valid references are accepted.
166-185: LGTM! Pre-download validation provides excellent early feedback.The validation calls in both
downloadFromRemoteanddownloadFromLocalappropriately:
- Validate repository accessibility before attempting download
- Skip validation for special refs ('stable', 'HEAD') that are resolved dynamically
- Provide clear error messages for missing repositories or references
Note: The TOCTOU (time-of-check to time-of-use) gap between validation and download is acceptable here, as any race condition will still result in a clear error during download, and the validation primarily catches configuration mistakes early.
Also applies to: 202-218
348-459: LGTM! Validation methods are well-structured and handle edge cases correctly.All four validation methods:
- Use appropriate mechanisms (HEAD requests for remote, git commands for local)
- Handle authentication tokens when available
- Provide clear, actionable error messages
- Use proper exception handling
The public visibility enables testability while serving the pre-install validation flow.
AlexSkrypnyk
force-pushed
the
feature/update-installer-downloader-version
branch
from
858b927 to
dc408b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/src/Command/InstallCommand.php (1)
121-121: Documentation inconsistency: option description still references@delimiter.The option description mentions "optional git ref set after @" but the implementation now uses
#as the delimiter.- $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after @.'); + $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after #.');
♻️ Duplicate comments (4)
.vortex/installer/src/Utils/Validator.php (2)
44-68: Fix PHPDoc to resolve pipeline failures.The PHPDoc structure needs correction:
- Move
@paramand@returntags before the detailed description- Replace "Cannot" patterns with "Must not" to avoid forbidden comment pattern
Apply this diff:
/** * Validate a git reference (tag, branch, or commit). * + * @param string $value + * The reference string to validate. + * + * @return bool + * TRUE if valid, FALSE otherwise. + * * Accepts any valid git reference format including: * - Special keywords: "stable", "HEAD" * - Commit hashes: 40-character or 7-character SHA-1 hashes * - Version tags: "1.2.3", "v1.2.3", "25.11.0", "1.0.0-2025.11.0" * - Drupal-style tags: "8.x-1.10", "9.x-2.3" * - Pre-release tags: "1.x-rc1", "2.0.0-beta" * - Branch names: "main", "develop", "feature/my-feature" * * Follows git reference naming rules: * - Can contain alphanumeric, dot, hyphen, underscore, slash - * - Cannot start with dot or hyphen - * - Cannot contain: @, ^, ~, :, ?, *, [, space, \, @{ - * - Cannot end with .lock or contain .. + * - Must not start with dot or hyphen + * - Must not contain: @, ^, ~, :, ?, *, [, space, \, @{ + * - Must not end with .lock or contain .. * * @see https://git-scm.com/docs/git-check-ref-format - * - * @param string $value - * The reference string to validate. - * - * @return bool - * TRUE if valid, FALSE otherwise. */
80-88: Regex allows invalid git refs with trailing or consecutive slashes.The pattern permits refs ending with
/(e.g.,feature/) and containing//(e.g.,feature//name), butgit check-ref-formatrejects both.Add explicit checks after the regex:
if (!preg_match($pattern, $value)) { return FALSE; } + // Reject trailing slash and consecutive slashes. + if (str_ends_with($value, '/') || str_contains($value, '//')) { + return FALSE; + } + // Additional disallowed patterns..vortex/installer/tests/Unit/ValidatorTest.php (1)
175-246: Add missing edge case test data for trailing and consecutive slashes.The data provider has excellent coverage but is missing two edge cases that the current regex incorrectly allows:
- Refs ending with
/(e.g.,'invalid trailing slash' => ['feature/', FALSE])- Refs with consecutive slashes (e.g.,
'invalid consecutive slashes' => ['feature//name', FALSE])Add these test cases to reveal the validation gaps:
// Invalid formats - starting/ending patterns. 'invalid starting with dot' => ['.invalid', FALSE], 'invalid starting with hyphen' => ['-invalid', FALSE], 'invalid ending with .lock' => ['invalid.lock', FALSE], 'invalid containing ..' => ['invalid..ref', FALSE], + 'invalid trailing slash' => ['feature/', FALSE], + 'invalid consecutive slashes' => ['feature//name', FALSE], // Empty and edge cases..vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (1)
498-502: Edge case: trailing#is captured in repo path.As noted in past review comments, this edge case where
/path/to/repo#treats the trailing#as part of the repository path rather than an empty reference could confuse users. Consider whether this behavior should be rejected with a clearer error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.vortex/installer/src/Command/InstallCommand.php(5 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(6 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Utils/Validator.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(14 hunks).vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php(2 hunks).vortex/installer/tests/Unit/ValidatorTest.php(1 hunks).vortex/tests/bats/unit/update-vortex.bats(8 hunks).vortex/tests/phpunit/Functional/AhoyWorkflowTest.php(1 hunks).vortex/tests/phpunit/Functional/InstallerTest.php(2 hunks)scripts/vortex/update-vortex.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/installer/tests/Unit/ValidatorTest.php.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/installer/src/Utils/Validator.php
🧬 Code graph analysis (5)
.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(17-461)
.vortex/tests/bats/unit/update-vortex.bats (1)
.vortex/installer/src/Runner/ProcessRunner.php (1)
run(21-75)
.vortex/installer/src/Runner/ProcessRunner.php (3)
.vortex/installer/src/Runner/RunnerInterface.php (1)
run(41-41).vortex/installer/src/Runner/CommandRunner.php (1)
run(34-72).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
run(656-660)
.vortex/tests/phpunit/Functional/InstallerTest.php (1)
.vortex/tests/phpunit/Traits/SutTrait.php (1)
runInstaller(71-129)
.vortex/installer/src/Command/InstallCommand.php (3)
.vortex/installer/src/Task/Task.php (2)
Task(11-107)action(17-68).vortex/installer/src/Utils/Config.php (2)
get(80-82)Config(14-128).vortex/installer/src/Downloader/RepositoryDownloader.php (2)
RepositoryDownloader(17-461)parseUri(64-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
🔇 Additional comments (21)
.vortex/installer/src/Runner/ProcessRunner.php (1)
49-49: LGTM! Type hint correctly reflects Symfony Process API.The callback signature now properly type-hints the
$bufferparameter asstring, aligning with Symfony Process's documented callback interface. This removes unnecessary runtime conversion logic.scripts/vortex/update-vortex.sh (1)
18-29: LGTM! Consistent delimiter change from @ to #.All repository reference examples and the default value have been updated to use the
#delimiter instead of@, aligning with GitHub URL fragment syntax. The changes are consistent across all URI formats (HTTPS, file://, and local paths)..vortex/tests/phpunit/Functional/InstallerTest.php (2)
74-74: LGTM! Test updated for # delimiter.The functional test correctly uses the new
#delimiter syntax for specifying repository references, testing installation from the stable tag.
134-134: LGTM! Test updated for # delimiter.The functional test correctly uses the new
#delimiter syntax for specifying repository references, testing installation from a specific commit hash..vortex/tests/phpunit/Functional/AhoyWorkflowTest.php (1)
383-383: LGTM! Test command updated for # delimiter.The Ahoy workflow test correctly uses the new
#delimiter when invokingahoy update-vortexwith a specific commit reference, validating the updated URI format..vortex/tests/bats/unit/update-vortex.bats (1)
22-22: LGTM! Comprehensive test coverage updated for # delimiter.All Bats test cases have been consistently updated to use the
#delimiter across various scenarios:
- Default repository with stable tag
- Custom repositories
- Local paths and file:// URIs
- Git SSH URLs
- Interactive mode invocations
The changes maintain comprehensive coverage while aligning with the new delimiter syntax.
Also applies to: 46-46, 52-52, 73-73, 145-145, 151-151, 169-169, 175-175, 193-193, 217-217, 241-241, 247-247
.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
8-8: LGTM! Good refactor to centralize default repository reference.The badge URL generation now derives from
RepositoryDownloader::DEFAULT_REPO, eliminating a hardcoded URL and establishing a single source of truth. This improves maintainability.Also applies to: 270-271
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (5)
248-254: LGTM! Mock setup correctly validates two HTTP calls.The test now properly validates that both the HEAD request (for repository validation) and GET request (for releases API) include the authorization header with the correct token format.
997-1028: Comprehensive validation test coverage for remote repositories.These tests properly verify:
- 404 response handling for nonexistent repositories
- Reference validation with separate repo/ref responses
- Correct error message expectations
1030-1049: Good coverage for local repository validation scenarios.Tests correctly verify error handling for:
- Non-existent paths
- Directories that aren't git repositories
1051-1078: Request exception handling tests are well-structured.Both remote repository and remote ref validation tests properly simulate network errors and verify the appropriate error messages are thrown.
1016-1021: The mock callback correctly differentiates between archive and repository API requests.The
str_contains($url, '/archive/')check is appropriate and accurate. Archive URLs follow the pattern{repo_url}/archive/{ref}.tar.gz, while repository API requests useapi.github.com/repos/{owner}/{repo}. These patterns are sufficiently distinct and the current approach is both clear and correct for the test's purpose.Likely an incorrect or invalid review comment.
.vortex/installer/src/Command/InstallCommand.php (3)
147-160: LGTM! Pre-validation flow is well-structured.The validation task provides good UX with:
- Clear label indicating what's being validated
- Informative hint showing repo and ref being checked
- Success message on completion
- Proper gating via
shouldValidateRepositoryAndRef()
744-755: LGTM! Validation skip logic is correct.The method correctly identifies when to skip validation:
- Handles both with and without
.gitsuffix for default repo comparison- Checks for both
stableandHEADas default refs- Only skips when both repo AND ref are defaults
773-773: Remove the type safety concern about$repo— it cannot be null in this code path.
Config::get()can return null in general, but$repois guaranteed to be non-null here becauseresolveOptions()(called beforevalidateRepositoryAndRef()) always invokesRepositoryDownloader::parseUri(), which returns a non-null string for the repository. The repository value is then stored in config and retrieved as a valid string. The(string)casts are unnecessary in this context.Likely an incorrect or invalid review comment.
.vortex/installer/src/Downloader/RepositoryDownloader.php (6)
23-23: LGTM! DEFAULT_REPO constant centralizes repository URL.This constant allows consistent reference to the default Vortex repository across the codebase.
65-76: LGTM! GitHub pattern detection with immediate validation.The flow correctly:
- Attempts GitHub-specific pattern detection first
- Validates the extracted ref immediately
- Returns early if successful, avoiding redundant parsing
126-158: LGTM! Comprehensive URL pattern detection.The method handles multiple GitHub URL formats that users might copy from browsers:
- Release tag URLs
- Tree URLs (branches/tags)
- Commit URLs
.git#refsyntax- SSH
git@...#refsyntax
390-414: LGTM! HEAD request for ref validation is efficient.Using a HEAD request to check the archive URL existence avoids downloading the entire archive while still verifying the reference is valid. The error messages are clear and actionable.
426-434: LGTM! Local repository validation is straightforward.The two-step check (directory exists, then .git subdirectory exists) correctly identifies valid git repositories and provides clear error messages for each failure case.
166-185: Validation order ensures fail-fast behavior.The validation sequence is correct:
- Validate repository exists first
- Then validate ref (for non-special refs)
- Only proceed to download if both pass
This prevents unnecessary work and provides clear error messages early.
AlexSkrypnyk
force-pushed
the
feature/update-installer-downloader-version
branch
2 times, most recently
from
358c05d to
1ede5e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/src/Command/InstallCommand.php (1)
127-127: Help text uses incorrect delimiter character.The option description says "after @" but the actual URI parsing uses
#as the delimiter (as shown in the examples on lines 109-120 and the Artifact implementation).- $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after @.'); + $this->addOption(static::OPTION_URI, 'l', InputOption::VALUE_REQUIRED, 'Remote or local repository URI with an optional git ref set after #.');
♻️ Duplicate comments (2)
.vortex/installer/src/Utils/Validator.php (1)
44-68: PHPDoc structure still has issues from prior review.The docblock still needs restructuring:
- Lines 57-59 use "Cannot" which triggers the
SlevomatCodingStandard.Commenting.ForbiddenCommentserror@paramand@returntags (lines 61-65) should appear before the detailed description- Line 59 appears truncated: "Cannot end with .lock or contain" - missing the rest of the sentence
/** * Validate a git reference (tag, branch, or commit). * + * @param string $value + * The reference string to validate. + * + * @return bool + * TRUE if valid, FALSE otherwise. + * * Accepts any valid git reference format including: * - Special keywords: "stable", "HEAD" * - Commit hashes: 40-character or 7-character SHA-1 hashes * - Version tags: "1.2.3", "v1.2.3", "25.11.0", "1.0.0-2025.11.0" * - Drupal-style tags: "8.x-1.10", "9.x-2.3" * - Pre-release tags: "1.x-rc1", "2.0.0-beta" * - Branch names: "main", "develop", "feature/my-feature" * * Follows git reference naming rules: * - Can contain alphanumeric, dot, hyphen, underscore, slash - * - Cannot start with dot or hyphen - * - Cannot contain: @, ^, ~, :, ?, *, [, space, \, @{ - * - Cannot end with .lock or contain + * - Must not start with dot or hyphen + * - Must not contain: @, ^, ~, :, ?, *, [, space, \, @{ + * - Must not end with .lock or contain .. * * @see https://git-scm.com/docs/git-check-ref-format - * - * @param string $value - * The reference string to validate. - * - * @return bool - * TRUE if valid, FALSE otherwise. */.vortex/installer/src/Downloader/Artifact.php (1)
111-122:isDefault()/ GitHub pattern helper behavior and naming
isDefault()’s logic aroundDEFAULT_REPO(with and without.git) and the stable/HEAD refs is clear and matches the intended semantics for “default artifact”.For
detectGitHubUrlPattern(), note that patterns 4 and 5 (.git#refandgit@...#ref) are actually host-agnostic even though the method name and docblock say “GitHub-specific”. This is fine functionally (broader support is nice), but the naming/doc can be slightly misleading for future readers.If you want to reduce that confusion, you could either narrow the regexes to
github.comor adjust the docblock/method name to reflect that patterns 4–5 are generic#refsyntaxes for any git host.Also applies to: 193-229
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
.vortex/tests/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.vortex/installer/src/Command/InstallCommand.php(6 hunks).vortex/installer/src/Downloader/Artifact.php(1 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(6 hunks).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php(2 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Utils/Validator.php(1 hunks).vortex/installer/tests/Unit/Downloader/ArchiverTest.php(9 hunks).vortex/installer/tests/Unit/Downloader/ArtifactTest.php(1 hunks).vortex/installer/tests/Unit/Downloader/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(19 hunks).vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php(2 hunks).vortex/installer/tests/Unit/ValidatorTest.php(1 hunks).vortex/tests/bats/unit/update-vortex.bats(8 hunks).vortex/tests/phpunit/Functional/AhoyWorkflowTest.php(1 hunks).vortex/tests/phpunit/Functional/InstallerTest.php(2 hunks)scripts/vortex/update-vortex.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/phpunit/Functional/InstallerTest.php.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/installer/tests/Unit/ValidatorTest.php.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php.vortex/installer/src/Utils/Validator.php
🧬 Code graph analysis (7)
.vortex/installer/src/Runner/ProcessRunner.php (1)
.vortex/installer/src/Runner/RunnerInterface.php (1)
run(41-41)
.vortex/installer/tests/Unit/Downloader/ArtifactTest.php (1)
.vortex/installer/src/Downloader/Artifact.php (11)
Artifact(15-238)fromUri(43-51)getRepo(78-80)getRef(85-87)create(67-73)isRemote(99-101)isLocal(106-108)isDefault(113-122)getRepoUrl(92-94)isStable(127-129)isDevelopment(134-136)
.vortex/installer/tests/Unit/Downloader/ArchiverTest.php (2)
.vortex/installer/src/Downloader/ArchiverInterface.php (1)
validate(35-35).vortex/installer/src/Downloader/Archiver.php (1)
validate(52-66)
.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php (1)
.vortex/tests/phpunit/Traits/ProcessTrait.php (1)
cmd(59-80)
.vortex/tests/bats/unit/update-vortex.bats (1)
.vortex/installer/src/Runner/ProcessRunner.php (1)
run(21-75)
.vortex/installer/src/Downloader/Artifact.php (2)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-107)gitRef(69-105).vortex/installer/src/Downloader/RepositoryDownloader.php (2)
__construct(40-46)RepositoryDownloader(16-406)
.vortex/installer/src/Downloader/RepositoryDownloader.php (2)
.vortex/installer/src/Downloader/Artifact.php (5)
Artifact(15-238)isRemote(99-101)getRepoUrl(92-94)getRef(85-87)getRepo(78-80).vortex/installer/src/Utils/Env.php (1)
Env(7-247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.3)
🔇 Additional comments (34)
.vortex/installer/tests/Unit/Downloader/ArchiverTest.php (1)
23-23: LGTM! Data provider naming standardization improves clarity.The renaming from
provider*todataProvider*follows common PHPUnit conventions and makes the purpose of these methods more explicit. All attribute references correctly match their corresponding method names.Also applies to: 43-43, 50-50, 64-64, 80-80, 104-104, 127-127, 144-144, 170-170, 201-201
scripts/vortex/update-vortex.sh (1)
14-29: LGTM! Delimiter change from@to#is well-documented and consistent.The switch to
#is a good choice as it avoids ambiguity with SSH URLs that contain@(e.g.,git@github.com:...). The documentation examples comprehensively cover all URI formats: HTTPS,file://, and local paths..vortex/tests/phpunit/Functional/AhoyWorkflowTest.php (1)
383-388: LGTM! Test updated to use#delimiter consistently.The test correctly constructs the URI with the new
#delimiter and verifies that the specific commit reference is properly handled during the update process..vortex/tests/phpunit/Functional/InstallerTest.php (2)
74-74: LGTM! URI delimiter updated consistently.The test correctly uses the
#delimiter for the stable ref.
134-134: LGTM! URI delimiter updated consistently.The test correctly uses the
#delimiter for the commit ref..vortex/tests/bats/unit/update-vortex.bats (8)
20-25: LGTM! Test expectations updated for default URI with#stable.The escaped
\#is correct in bash context to prevent interpretation as a comment character.
44-52: LGTM! Custom repository test updated with#delimiter.Both the STEPS expectation and the actual command invocation correctly use the
#delimiter.
72-73: LGTM! Local installer path test updated.
143-151: LGTM! Local path test updated with#stableref.
167-175: LGTM! SSH URL test updated with#v1.2.3ref.
191-193: LGTM! Failure scenario test updated.
215-217: LGTM! Interactive mode test updated.
239-247: LGTM! Interactive mode with custom repository test updated..vortex/installer/tests/Unit/Downloader/DownloaderTest.php (1)
83-111: LGTM! Comprehensive test for custom headers functionality.The test properly mocks the HTTP client and verifies that custom headers (Authorization, X-Custom-Header) are correctly passed through to the Guzzle request options.
.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
267-274: LGTM! Good refactor to use centralized constant.Deriving the repository URL from
RepositoryDownloader::DEFAULT_REPOensures consistency with production code and reduces the risk of hardcoded values drifting out of sync.Note: The
str_replace('.git', '', ...)approach works correctly for the known constant value (https://github.com/drevops/vortex.git), but if more robust normalization is needed elsewhere, consider using a dedicated helper method (theArtifact::getRepoUrl()already provides this vianormalizeRepoUrl())..vortex/installer/tests/Unit/Downloader/ArtifactTest.php (7)
1-17: LGTM! Well-structured test class setup.The class is properly attributed with
#[CoversClass(Artifact::class)]for code coverage tracking.
38-171: Excellent test coverage forfromUri()with comprehensive data provider.The data provider covers:
- Default/null URI handling
- GitHub HTTPS patterns (with
#ref, without ref, release tags, tree branches, commits)- SSH patterns
- Local paths (absolute, trailing slash,
file://URLs)- Invalid ref formats (spaces, trailing slashes, consecutive slashes)
- Invalid URI formats
This provides strong confidence in the URI parsing logic.
173-215: LGTM! Good coverage forcreate()factory method.Tests both valid scenarios and invalid ref validation, ensuring the
Validator::gitRef()integration works correctly.
217-253: LGTM!isRemote()andisLocal()tests are symmetric and correct.The test at line 232/251 correctly verifies that
file://URLs are treated as local, which is the expected behavior since they reference local filesystem paths.
255-297: LGTM!isDefault()tests cover key scenarios.Good coverage of:
- Default repo with various refs (stable, HEAD, custom)
- Default repo with and without
.gitsuffix- Custom repos (should always return false)
299-327: LGTM!getRepoUrl()normalization tests are correct.Verifies that
.gitsuffix is properly stripped for URLs while local paths remain unchanged.
329-363: LGTM!isStable()andisDevelopment()predicate tests are thorough.Tests correctly verify that only the exact ref values (
stableandHEADrespectively) returntrue..vortex/installer/src/Runner/ProcessRunner.php (1)
49-55: Callback signature correctly simplified tostring.The Symfony Process callback buffer is always a string per the official documentation. Removing the unnecessary
|iterableunion type is correct and aligns with the actual callback signaturecallable(string $type, string $buffer): void..vortex/installer/src/Utils/Validator.php (1)
91-94: LGTM - Trailing and consecutive slash validation added.The explicit checks for
str_ends_with($value, '/')andstr_contains($value, '//')properly address the git ref validation gaps that were previously identified. This ensures refs likefeature/andfeature//nameare correctly rejected..vortex/installer/tests/Unit/ValidatorTest.php (1)
170-248: LGTM - Comprehensive test coverage for gitRef validation.The test method and data provider cover an extensive range of valid and invalid git references including:
- Reserved keywords (
stable,HEAD)- Commit hashes (40-char and 7-char)
- Version formats (SemVer, CalVer, Drupal-style, hybrid)
- Branch naming patterns
- Invalid special characters and patterns
- Edge cases for trailing slash (line 241) and consecutive slashes (line 242) that were previously missing
.vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
15-29: LGTM - Clean interface refactor to artifact-centric design.The method signature change from
download(string $repo, string $ref, ...)todownload(Artifact $artifact, ...)properly encapsulates repository and reference handling into a single value object. The docblock accurately describes the parameter..vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (2)
8-8: LGTM - Tests properly adapted to Artifact-based API.The test file correctly imports and uses
Artifact::create()throughout all test methods. The migration from string-based repo/ref parameters to Artifact objects is consistent.Also applies to: 36-36
489-610: LGTM - Comprehensive validation test coverage added.The new validation tests thoroughly cover:
- Remote repository existence checks (404, request exceptions)
- Remote ref existence checks (404, request exceptions)
- Local repository validation (non-existent path, non-git directory)
- Successful validation for stable, custom refs (remote), and HEAD/custom refs (local)
The use of
expectNotToPerformAssertions()is appropriate for tests that verify no exception is thrown..vortex/installer/src/Command/InstallCommand.php (3)
89-92: LGTM - Artifact property declaration.The protected property cleanly encapsulates the repository and reference for use throughout the command lifecycle.
333-351: LGTM - Artifact creation with proper error handling.The URI building logic correctly prioritizes option URI over env/config values, and the try-catch wraps the
Artifact::fromUri()call with a contextual error message while preserving the original exception's code and trace.
556-564: LGTM - Header messaging correctly reflects artifact state.The conditional messaging using
isStable(),isDevelopment(), andgetRef()provides clear feedback to users about which version type they're installing..vortex/installer/src/Downloader/RepositoryDownloader.php (2)
48-61: Artifact-based download flow looks coherentUsing
Artifactto drivedownload()and delegating todownloadFromRemote()/downloadFromLocal()based onisRemote()/isLocal()keeps the public API clean and centralizes the decision logic. The post-downloadcomposer.jsonpresence check is also a solid sanity guard.Also applies to: 99-167
289-318: Local/remote validation helpers are well-structuredThe new
validateRemoteRepositoryExists(),validateRemoteRefExists(),validateLocalRepositoryExists(), andvalidateLocalRefExists()helpers give you a clear separation of concerns and readable failure modes. In particular, therealpath()+getRepositoryPath()check invalidateLocalRefExists()closes the earlier bug where refs were validated against the wrong repo.Also applies to: 320-356, 367-404
.vortex/installer/src/Downloader/Artifact.php (1)
15-88: Artifact value object design looks solidThe immutable
ArtifactwithfromUri()/create(), central parsing, and Validator-backed ref checks is a nice encapsulation of repo+ref concerns and keeps the downloader API much cleaner.
AlexSkrypnyk
force-pushed
the
feature/update-installer-downloader-version
branch
from
1ede5e9 to
660fb55
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/vortex/update-vortex.sh (1)
14-29: Consider documenting quoting for#-delimited URIs in examplesSwitching examples and the default to
...vortex.git#stablealigns the script with the new artifact format, but#is a shell comment delimiter. To prevent confusion when users copy/paste these examples, it’s worth either:
- showing them as quoted (e.g.,
./update-vortex.sh 'https://github.com/drevops/vortex.git#stable'), or- adding a short note that URIs containing
#should be quoted or have#escaped when passed on the shell command line..vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (1)
332-340: Test case 'invalid url' expects an incorrect error message.The test data provider specifies that
Artifact::create('https://', 'stable')should throw an error with message "Local repository path does not exist", but the actual error thrown is "Invalid remote repository format: 'https://'. Use # to specify a reference (e.g., repo.git#tag)." This happens during URI parsing inArtifact::parseUri()before the downloader's validation methods are invoked.Update the test expectation to:
'expectedMessage' => 'Invalid remote repository format'.
♻️ Duplicate comments (4)
.vortex/installer/src/Utils/Validator.php (1)
44-68: PHPDoc structure needs adjustment per coding standards.The past review flagged two issues that appear to still be present:
- Lines 57-59: Documentation contains forbidden comment pattern "- Cannot" (should use "Must not")
- Lines 61-65: Parameter tags should be defined first in doc comment, before the detailed description
Additionally, line 59 appears incomplete: "- Cannot end with .lock or contain" is missing the continuation about
...Apply this diff to fix the PHPDoc:
/** * Validate a git reference (tag, branch, or commit). * + * @param string $value + * The reference string to validate. + * + * @return bool + * TRUE if valid, FALSE otherwise. + * * Accepts any valid git reference format including: * - Special keywords: "stable", "HEAD" * - Commit hashes: 40-character or 7-character SHA-1 hashes * - Version tags: "1.2.3", "v1.2.3", "25.11.0", "1.0.0-2025.11.0" * - Drupal-style tags: "8.x-1.10", "9.x-2.3" * - Pre-release tags: "1.x-rc1", "2.0.0-beta" * - Branch names: "main", "develop", "feature/my-feature" * * Follows git reference naming rules: * - Can contain alphanumeric, dot, hyphen, underscore, slash - * - Cannot start with dot or hyphen - * - Cannot contain: @, ^, ~, :, ?, *, [, space, \, @{ - * - Cannot end with .lock or contain + * - Must not start with dot or hyphen + * - Must not contain: @, ^, ~, :, ?, *, [, space, \, @{ + * - Must not end with .lock or contain .. * - * @param string $value - * The reference string to validate. - * - * @return bool - * TRUE if valid, FALSE otherwise. - * * @see https://git-scm.com/docs/git-check-ref-format */.vortex/installer/src/Downloader/Artifact.php (1)
43-51: Consider usingcreate()for consistency.The default case (lines 45-46) bypasses
create()and constructs directly vianew self(). While this is safe becauseDEFAULT_REPOandREF_STABLEare trusted constants, usingcreate()would centralize all construction paths and ensure validation behavior is consistent if requirements change.public static function fromUri(?string $uri): self { // Use default repository and stable reference if URI is empty or null. if ($uri === NULL || $uri === '') { - return new self(RepositoryDownloader::DEFAULT_REPO, RepositoryDownloader::REF_STABLE); + return self::create(RepositoryDownloader::DEFAULT_REPO, RepositoryDownloader::REF_STABLE); } [$repo, $ref] = self::parseUri($uri); - return new self($repo, $ref); + return self::create($repo, $ref); }Note: This would add redundant validation for the already-validated parseUri case, but keeps the code DRY.
.vortex/installer/src/Downloader/RepositoryDownloader.php (2)
265-268: Same Git instance reuse bug inarchiveFromLocal().This method has the same issue: the Git instance may reference a different repository than the one being archived.
protected function archiveFromLocal(string $repo, string $ref): string { - if (!$this->git instanceof Git) { - $this->git = new Git($repo); - } + $repo_path = (string) realpath($repo); + if ( + !$this->git instanceof Git || + $this->git->getRepositoryPath() !== $repo_path + ) { + $this->git = new Git($repo); + }
149-155: Git instance reuse bug persists for HEAD downloads across different repositories.This issue was flagged in a past review but appears unresolved. When
downloadFromLocal()is called multiple times with different repositories usingHEAD:
- First call with
repoAandHEADinitializes$this->gitwithrepoA- Second call with
repoBandHEAD- the check at line 150 passes because$this->git instanceof Gitis alreadyTRUEgetLastShortCommitId()at line 153 runs againstrepoA, returning the wrong commit IDApply the same fix pattern used in
validateLocalRefExists():if ($ref === RepositoryDownloader::REF_HEAD) { - if (!$this->git instanceof Git) { + $repo_path = (string) realpath($artifact->getRepo()); + if ( + !$this->git instanceof Git || + $this->git->getRepositoryPath() !== $repo_path + ) { $this->git = new Git($artifact->getRepo()); } $ref = $this->git->getLastShortCommitId(); $version = 'develop'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
.vortex/tests/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.vortex/installer/src/Command/InstallCommand.php(6 hunks).vortex/installer/src/Downloader/Artifact.php(1 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(6 hunks).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php(2 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Utils/Validator.php(1 hunks).vortex/installer/tests/Unit/Downloader/ArchiverTest.php(9 hunks).vortex/installer/tests/Unit/Downloader/ArtifactTest.php(1 hunks).vortex/installer/tests/Unit/Downloader/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(19 hunks).vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php(2 hunks).vortex/installer/tests/Unit/ValidatorTest.php(1 hunks).vortex/tests/bats/unit/update-vortex.bats(8 hunks).vortex/tests/phpunit/Functional/AhoyWorkflowTest.php(1 hunks).vortex/tests/phpunit/Functional/InstallerTest.php(2 hunks)scripts/vortex/update-vortex.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/phpunit/Functional/InstallerTest.php.vortex/tests/bats/unit/update-vortex.bats
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/installer/src/Utils/Validator.php.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/installer/tests/Unit/ValidatorTest.php.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php.vortex/tests/bats/unit/update-vortex.bats
🧬 Code graph analysis (11)
.vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (3)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
download(48-61).vortex/installer/src/Downloader/Downloader.php (1)
download(41-57).vortex/installer/src/Downloader/Artifact.php (1)
Artifact(15-276)
.vortex/installer/tests/Unit/Downloader/DownloaderTest.php (1)
.vortex/installer/src/Downloader/Downloader.php (2)
Downloader(14-59)download(41-57)
.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php (1)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(16-406)
.vortex/tests/phpunit/Functional/InstallerTest.php (1)
.vortex/tests/phpunit/Traits/SutTrait.php (1)
runInstaller(71-129)
.vortex/installer/src/Downloader/RepositoryDownloader.php (3)
.vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
download(29-29).vortex/installer/src/Downloader/Artifact.php (5)
Artifact(15-276)isRemote(99-113)getRepoUrl(92-94)getRef(85-87)getRepo(78-80).vortex/installer/src/Utils/Env.php (1)
Env(7-247)
.vortex/installer/src/Runner/ProcessRunner.php (2)
.vortex/installer/src/Prompts/Handlers/Starter.php (1)
process(99-133).vortex/installer/src/Runner/CommandRunner.php (1)
run(34-72)
.vortex/installer/src/Downloader/Artifact.php (2)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-107)gitRef(69-105).vortex/installer/src/Downloader/RepositoryDownloader.php (2)
__construct(40-46)RepositoryDownloader(16-406)
.vortex/installer/tests/Unit/ValidatorTest.php (1)
.vortex/installer/src/Utils/Validator.php (2)
Validator(14-107)gitRef(69-105)
.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php (1)
.vortex/tests/phpunit/Traits/ProcessTrait.php (1)
cmd(59-80)
.vortex/tests/bats/unit/update-vortex.bats (1)
.vortex/installer/src/Runner/ProcessRunner.php (1)
run(21-75)
.vortex/installer/tests/Unit/Downloader/ArtifactTest.php (2)
.vortex/installer/src/Downloader/Artifact.php (11)
Artifact(15-276)fromUri(43-51)getRepo(78-80)getRef(85-87)create(67-73)isRemote(99-113)isLocal(118-120)isDefault(125-134)getRepoUrl(92-94)isStable(139-141)isDevelopment(146-148).vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(16-406)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
🔇 Additional comments (39)
.vortex/installer/tests/Unit/Downloader/ArchiverTest.php (2)
23-23: LGTM! Consistent data provider renaming improves PHPUnit convention adherence.All data provider method names have been consistently updated from
provider*todataProvider*, and the corresponding#[DataProvider]attributes correctly reference the renamed methods. This improves alignment with PHPUnit naming conventions.Also applies to: 43-43, 50-50, 64-64, 80-80, 104-104, 127-127, 144-144, 170-170, 201-201
104-104: No remaining references to the old method names (providerDetectFormat,providerValidateValidArchive,providerValidateInvalid,providerExtract,providerExtractErrors) exist in the codebase. The refactoring is complete..vortex/tests/phpunit/Functional/AhoyWorkflowTest.php (1)
383-388: Verify#-delimited ref is not lost to shell commentsUsing
#directly in the command string ('ahoy update-vortex ' . static::$repo . '#' . $latest_installer_commit1) can cause everything after#to be treated as a shell comment if the underlying runner executes this via a shell command line. Please confirm thatcmd()/processRun()either:
- runs the command with proper argument escaping/quoting, or
- uses an array-based execution path that preserves the
#suffix as part of the argument..vortex/installer/src/Runner/ProcessRunner.php (1)
49-55: Callback$buffertype narrowed tostringis appropriateSymfony’s
Process::run()callback passes a string$buffer, so type-hinting it asstringhere aligns with the library’s API and simplifies handling. No functional regression is apparent..vortex/tests/phpunit/Functional/InstallerTest.php (1)
74-80: Installer--uritests updated to#delimiter are consistentBoth latest and ref-based installer tests now use
--uri=%s#%s, which is consistent with the new artifact/URI format and with the updated update-vortex script/tests. ProvidedrunInstaller()passes these as proper arguments (not a raw shell string), this should behave as intended.Also applies to: 134-139
.vortex/installer/tests/Unit/ValidatorTest.php (1)
170-248:gitRefdata provider offers strong, targeted coverageThe new
testGitRefanddataProviderGitRefcomprehensively exerciseValidator::gitRef(), including reserved keywords, hashes, multiple versioning schemes, valid branch patterns, and the key invalid cases (@, spaces,.lock,.., trailing/, and//). This also addresses the earlier review feedback about trailing and consecutive slashes..vortex/installer/tests/Unit/Downloader/DownloaderTest.php (1)
83-111: Custom-headers test correctly validates Guzzle options
testDownloadWithCustomHeaderscleanly verifies thatDownloader::download()forwards the headers into the Guzzle options alongsidesinkandallow_redirects. The expectation onClientInterface::request()matches the production signature and behavior..vortex/installer/src/Utils/Validator.php (1)
69-105: Well-structured git reference validation.The implementation correctly handles:
- Reserved keywords (
stable,HEAD)- Full and short commit hashes via existing validators
- Git ref naming rules with proper regex and explicit checks for edge cases
- Disallowed characters per git-check-ref-format specification
The explicit checks for trailing slashes and consecutive slashes (lines 91-94) properly address git's ref naming constraints.
.vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
15-29: Clean interface simplification with Artifact.The interface change from separate repo/ref parameters to an
Artifactvalue object is a good design improvement:
- Encapsulates related data (repo + ref) in a single immutable object
- Reduces parameter coupling
- Makes the API clearer and less error-prone
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (3)
8-8: Appropriate import for Artifact class.The import is correctly added to support the new Artifact-based test assertions.
36-37: Tests correctly use Artifact::create for remote downloads.The migration to
Artifact::create()is consistent and properly exercises the new artifact-centric download API.
489-540: Comprehensive validation error handling tests.The new validation tests cover important error scenarios:
- Remote repository not found (HTTP 404)
- Reference not found in repository
- Local path doesn't exist
- Path exists but isn't a git repository
- Network/request exceptions
This provides good coverage for the validation flow.
.vortex/installer/src/Command/InstallCommand.php (6)
7-7: Appropriate import for Artifact class.The import is correctly added to support the new Artifact-based installation flow.
89-92: Well-documented Artifact property.The protected property with clear documentation encapsulates the repository and reference information for the installation.
101-121: Clear and comprehensive help text updates.The help text now documents:
- Default stable release behavior
- The
#delimiter for specifying refs- Various GitHub URL patterns that are supported (releases/tag, tree, commit)
This helps users understand the new URI format.
153-168: Validation conditionally runs for non-default artifacts.The validation flow correctly skips validation for default artifacts (official repo + stable/HEAD ref), avoiding unnecessary network calls.
The "Press any key to continue..." pause on lines 167-168 runs unconditionally. If this pause is intended only to let users review validation results, consider moving it inside the conditional block. However, if it's for UX consistency before prompts begin, the current placement is fine.
333-351: Clean URI resolution and Artifact creation.The implementation correctly:
- Prioritizes
--urioption over env/config values- Constructs URI from repo+ref when no explicit URI is provided
- Creates the Artifact via
fromUri()which handles defaults- Wraps exceptions with contextual error messages
- Stores parsed values back to config for downstream use
556-564: Header messaging correctly uses Artifact state.The header now uses
$this->artifact->isStable(),isDevelopment(), andgetRef()to provide appropriate messaging about what version will be installed..vortex/installer/src/Downloader/Artifact.php (6)
1-28: Well-designed immutable value object.The
final readonlyclass with private constructor enforces immutability and ensures all instances are created through factory methods, which is a clean design pattern.
67-73: Clean factory with ref validation.The
create()method validates the git reference format before construction, providing early failure with clear error messages. Repository validation is appropriately deferred to download time when the context (local/remote) is known.
96-113: Robust remote detection covering common git URL schemes.The
isRemote()method correctly identifies:
- SCP-style git URLs (
git@host:path)- URL schemes:
http,https,ssh,gitThe
file://scheme is correctly excluded, treating it as local. This addresses the prior review concern about scheme coverage.
154-171: Good deprecation handling for legacy@refsyntax.The code converts
@refto#refbefore further parsing, ensuring backward compatibility while preparing for removal in version 1.1.0. The regex patterns correctly distinguish between@inuser@hostand@refat the end of URLs.
235-267: Excellent UX for GitHub URL patterns.The detection of browser-copied GitHub URLs (
/releases/tag/,/tree/,/commit/) is a great usability feature. Users can paste URLs directly from their browser without reformatting.
269-274: Simple and effective URL normalization.The helper correctly strips the
.gitsuffix for consistent URL comparison inisDefault()andgetRepoUrl()..vortex/installer/tests/Unit/Downloader/ArtifactTest.php (8)
1-17: LGTM! Well-structured test class with proper PHPUnit attributes.The test class follows PHPUnit 11 best practices with
#[CoversClass]and#[DataProvider]attributes.
19-33: LGTM! Test method correctly handles both success and exception scenarios.The test structure properly separates exception expectation setup from assertion logic.
38-230: Excellent test coverage with comprehensive data provider.The data provider covers all URI formats, edge cases, and the deprecated
@refsyntax. The descriptive array keys improve test output readability.
232-274: LGTM! Properly tests thecreate()factory method's ref validation.The test correctly verifies that
create()validates ref syntax while accepting pre-parsed repo values.
276-318: LGTM! Remote/local detection tests provide symmetric coverage.The test cases correctly verify that
isRemote()andisLocal()are logical inverses, withfile://URLs properly treated as local paths.
320-362: LGTM! Comprehensive coverage of default artifact detection.Tests correctly verify the combination logic requiring both default repository (with or without
.git) and default reference (stableorHEAD).
364-392: LGTM! URL normalization tests cover key scenarios.Tests verify
.gitextension stripping for remote URLs while ensuring local paths remain unchanged.
394-428: LGTM! Stability and development reference checks are properly tested.Tests verify exact ref matching against
stableandHEADconstants..vortex/installer/src/Downloader/RepositoryDownloader.php (7)
22-22: LGTM! Centralizing the default repository URL as a constant.This improves maintainability by having a single source of truth for the default repository.
48-61: LGTM! Clean artifact-based routing with post-download validation.The download method correctly delegates to remote/local handlers and validates the repository structure.
99-136: LGTM! Remote download logic correctly handles special refs and validation.The method properly handles
stable(discovers latest release),HEAD, and custom refs with appropriate validation.
289-318: LGTM! Remote repository validation with proper error handling.The method correctly uses HTTP HEAD requests with GitHub token support and distinguishes between HTTP errors and connection failures.
331-356: LGTM! Remote ref validation via archive URL check.Using HEAD requests on the archive URL is an efficient way to verify ref existence without downloading.
367-375: LGTM! Local repository validation with proper git detection.The
.gitdirectory check correctly identifies valid git repositories.
388-404: LGTM! Correct Git instance management pattern.This method properly reinitializes the Git instance when the repository path differs. This is the pattern that should be applied to
downloadFromLocal()(lines 149-155) andarchiveFromLocal()(lines 265-268) to fix the Git instance reuse bug.
| // Fall back to #ref parsing (standard git reference syntax). | ||
| if (str_starts_with($src, 'https://') || str_starts_with($src, 'http://')) { | ||
| if (!preg_match('~^(https?://[^/]+/[^/]+/[^#]+)(?:#(.+))?$~', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid remote repository format: "%s". Use # to specify a reference (e.g., repo.git#tag).', $src)); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? RepositoryDownloader::REF_HEAD; | ||
| } | ||
| elseif (str_starts_with($src, 'ssh://') || str_starts_with($src, 'git://')) { | ||
| if (!preg_match('~^((?:ssh|git)://(?:[^@/]+@)?[^#/]+/.+?)(?:#(.+))?$~', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid remote repository format: "%s". Use # to specify a reference (e.g., git://host/repo#tag).', $src)); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? RepositoryDownloader::REF_HEAD; | ||
| } | ||
| elseif (str_starts_with($src, 'git@')) { | ||
| if (!preg_match('~^(git@[^:]+:[^#]+)(?:#(.+))?$~', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid remote repository format: "%s". Use # to specify a reference (e.g., git@host:repo#tag).', $src)); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? RepositoryDownloader::REF_HEAD; | ||
| } | ||
| elseif (str_starts_with($src, 'file://')) { | ||
| if (!preg_match('~^file://(.+?)(?:#(.+))?$~', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid local repository format: "%s". Use # to specify a reference.', $src)); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? RepositoryDownloader::REF_HEAD; | ||
| } | ||
| else { | ||
| if (!preg_match('~^(.+?)(?:#(.+))?$~', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid local repository format: "%s". Use # to specify a reference.', $src)); | ||
| } | ||
| $repo = rtrim($matches[1], '/'); | ||
| $ref = $matches[2] ?? RepositoryDownloader::REF_HEAD; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider improving error message for malformed remote URLs.
The HTTPS pattern (line 188) requires at least 3 path segments, so incomplete URLs like https:// or https://github.com fall through to the local path branch and produce a "Local repository path does not exist" error.
While functionally correct (they're indeed not valid remote repos), the error could confuse users who intended to specify a remote URL. Consider adding an early check for malformed remote URL prefixes:
+ // Detect incomplete remote URLs early for better error messages.
+ if (preg_match('~^(https?|ssh|git)://[^/]*/?$~', $src)) {
+ throw new \RuntimeException(sprintf('Incomplete remote repository URL: "%s". URL must include organization and repository name.', $src));
+ }
+
// Fall back to #ref parsing (standard git reference syntax).
if (str_starts_with($src, 'https://') || str_starts_with($src, 'http://')) {🤖 Prompt for AI Agents
.vortex/installer/src/Downloader/Artifact.php around lines 186-221: the HTTPS
remote branch currently only matches URLs with at least three path segments so
host-only or incomplete remote URLs (e.g., "https://", "https://github.com")
fall through to the local-path branch and produce confusing "local repository"
errors; add an explicit validation branch when the source starts with "http://"
or "https://" (and similarly consider ssh/git/git@/file schemes) that detects
malformed remote URLs and throws a clear RuntimeException like "Invalid remote
repository format: \"%s\"; expected full remote URL (e.g. repo.git#ref)". In
practice, perform an early regex or parse check immediately after the
str_starts_with(...) for remote schemes and, if it fails, throw the new
descriptive error instead of letting the value fall through to the local-path
handling so users see a correct message for malformed remote URLs.
| /** | ||
| * Validate repository and reference exist. | ||
| * | ||
| * @param \DrevOps\VortexInstaller\Downloader\Artifact $artifact | ||
| * The artifact to validate. | ||
| * | ||
| * @throws \RuntimeException | ||
| * If validation fails. | ||
| */ | ||
| public function validate(Artifact $artifact): void { | ||
| // Determine if this is a remote or local repository. | ||
| if ($artifact->isRemote()) { | ||
| // Remote repository. | ||
| $repo_url = $artifact->getRepoUrl(); | ||
|
|
||
| // Validate repository exists. | ||
| $this->validateRemoteRepositoryExists($repo_url); | ||
|
|
||
| // Validate ref exists (skip for special refs). | ||
| if ($artifact->getRef() !== self::REF_STABLE && $artifact->getRef() !== self::REF_HEAD) { | ||
| $this->validateRemoteRefExists($repo_url, $artifact->getRef()); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? static::REF_HEAD; | ||
| } | ||
| else { | ||
| if (!preg_match('#^(.+?)(?:@(.+))?$#', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid local repository format: "%s".', $src)); | ||
| // Local repository. | ||
| // Validate repository exists. | ||
| $this->validateLocalRepositoryExists($artifact->getRepo()); | ||
|
|
||
| // Validate ref exists (skip for HEAD). | ||
| $actual_ref = $artifact->getRef() === self::REF_STABLE ? self::REF_HEAD : $artifact->getRef(); | ||
| if ($actual_ref !== self::REF_HEAD) { | ||
| $this->validateLocalRefExists($artifact->getRepo(), $actual_ref); | ||
| } | ||
| $repo = rtrim($matches[1], '/'); | ||
| $ref = $matches[2] ?? static::REF_HEAD; | ||
| } | ||
|
|
||
| if ($ref != static::REF_STABLE && $ref != static::REF_HEAD && !Validator::gitCommitSha($ref) && !Validator::gitCommitShaShort($ref)) { | ||
| throw new \RuntimeException(sprintf('Invalid reference format: "%s". Supported formats are: %s, %s, %s, %s.', $ref, static::REF_STABLE, static::REF_HEAD, '40-character commit hash', '7-character commit hash')); | ||
| } | ||
|
|
||
| return [$repo, $ref]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider avoiding duplicate validation when validate() is called before download().
Both validate() and the internal downloadFromRemote()/downloadFromLocal() methods perform the same repository and ref existence checks. When callers follow the pattern of calling validate() before download(), this results in duplicate HTTP requests and Git operations.
Consider one of these approaches:
- Add an internal flag to track if validation was already performed for the current artifact
- Have
download()callvalidate()internally and remove duplicate checks fromdownloadFromRemote/downloadFromLocal - Accept this as a design trade-off for flexibility (validate can be called standalone)
🤖 Prompt for AI Agents
.vortex/installer/src/Downloader/RepositoryDownloader.php around lines 63-97:
the validate() method duplicates repository/ref existence checks later in
downloadFromRemote()/downloadFromLocal(), causing redundant network/git work
when callers call validate() then download(); add a lightweight internal
tracking mechanism to skip repeated validation: add a private property (e.g.
array or associative map) keyed by a stable artifact identifier (repo URL or
path + ref) and mark the entry as validated when validate() completes
successfully, then update downloadFromRemote() and downloadFromLocal() to check
this map and skip their internal validation steps if the artifact is already
marked validated (and remove or keep entries as desired); ensure
thread-safety/clearing if downloader is reused and include unit tests covering
validate-then-download and download-only paths.
| public function testValidateLocalArtifactWithCustomRef(): void { | ||
| $temp_repo_dir = $this->createGitRepo(); | ||
| $downloader = new RepositoryDownloader(); | ||
| $artifact = Artifact::create($temp_repo_dir, 'main'); | ||
| $downloader->validate($artifact); | ||
| $this->expectNotToPerformAssertions(); | ||
| $this->removeGitRepo($temp_repo_dir); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php | sed -n '450,466p'Repository: drevops/vortex
Length of output: 868
🏁 Script executed:
# Check if there's a git config setting in the repository or test setup
rg -n "init.defaultBranch|checkout -b main|branch -m main" .vortex/installer/tests/Repository: drevops/vortex
Length of output: 40
The test will fail on systems where Git's default branch is 'master'.
The createGitRepo() method doesn't explicitly create a 'main' branch. It only calls git init and makes commits on the default branch, which varies by Git configuration. If the system Git defaults to 'master' (Git < 2.28), the test at line 603 will fail when Artifact::create($temp_repo_dir, 'main') attempts to validate a non-existent 'main' branch.
Add git checkout -b main in createGitRepo() after the initial commit to ensure 'main' always exists.
🤖 Prompt for AI Agents
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php around
lines 603 to 610: the test assumes a 'main' branch exists but createGitRepo()
only runs git init and commits on the system default branch, which may be
'master'; update createGitRepo() to explicitly create and switch to a 'main'
branch (e.g., run git checkout -b main immediately after the initial commit) so
Artifact::create($temp_repo_dir, 'main') always validates successfully.
| use AlexSkrypnyk\PhpunitHelpers\Traits\TuiTrait as UpstreamTuiTrait; | ||
| use DrevOps\VortexInstaller\Downloader\RepositoryDownloader; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Centralising README badge URL on RepositoryDownloader::DEFAULT_REPO looks good
Using RepositoryDownloader::DEFAULT_REPO and stripping .git keeps the badge link in sync with the installer’s default repo, which is more robust than a hard-coded URL. If DEFAULT_REPO ever changes to include .git in more than one place, consider using a suffix-only removal (e.g., preg_replace('~\.git$~', '', ...)) instead of a blind str_replace, but it's fine with the current constant.
Also applies to: 267-274
🤖 Prompt for AI Agents
In .vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php
around lines 7-8 (and also apply the same change at lines 267-274), replace the
hard-coded README badge URL usage with RepositoryDownloader::DEFAULT_REPO and
ensure any removal of a trailing ".git" uses a suffix-only removal instead of a
blind replace; specifically, call the constant and strip only a trailing ".git"
(e.g., using a regex that removes the suffix) so changes to DEFAULT_REPO remain
in sync and only a trailing ".git" is removed.
| declare -a STEPS=( | ||
| "@curl -fsSL https://www.vortextemplate.com/install?1234567890 -o installer.php # 0" | ||
| "@php installer.php --uri=https://github.com/custom/repo.git@main # 0" | ||
| "@php installer.php --uri=https://github.com/custom/repo.git\#main # 0" | ||
| "Using installer script from URL: https://www.vortextemplate.com/install" | ||
| "Downloading installer to installer.php" | ||
| ) | ||
|
|
||
| mocks="$(run_steps "setup")" | ||
| run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" --interactive https://github.com/custom/repo.git@main | ||
| run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" --interactive https://github.com/custom/repo.git#main | ||
| run_steps "assert" "${mocks[@]}" |
There was a problem hiding this comment.
Quote custom template repo with # in interactive test
In the interactive test, this line:
run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" --interactive https://github.com/custom/repo.git#mainleaves #main unquoted, so the shell treats #main as a comment and only passes https://github.com/custom/repo.git to the script. This will prevent the test from actually exercising the #main ref.
Wrap the URI in quotes (or escape #) to preserve the ref:
- run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" --interactive https://github.com/custom/repo.git#main
+ run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" --interactive "https://github.com/custom/repo.git#main"🤖 Prompt for AI Agents
.vortex/tests/bats/unit/update-vortex.bats around lines 239 to 248: the
interactive test invokes update-vortex.sh with an unquoted URI containing a
hash, so the shell treats `#main` as a comment and the script never receives the
ref; fix by quoting the URI or escaping the `#` in the run invocation (for
example change the run line to pass "https://github.com/custom/repo.git#main" or
escape the hash as https://github.com/custom/repo.git\#main) so the full ref is
preserved and the test exercises the `#main` branch.
Closes #2162
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.