[#1950] Added "build now" to installer. #2134
Conversation
WalkthroughRegisters install, check-requirements, and build console commands; adds runner abstractions (ProcessRunner, CommandRunner, RunnerInterface), file-based logging, streaming-capable Task output, a build orchestration (with --build/--destination), post-build hooks, many prompt/handler updates, and extensive tests and CI/phpstan/phpunit tweaks. Changes
sequenceDiagram
participant CLI as User / CLI
participant App as Symfony Application
participant Install as InstallCommand
participant CR as CommandRunner
participant PR as ProcessRunner
participant FL as FileLogger
participant Build as BuildCommand
participant PromptMgr as PromptManager
CLI->>App: invoke installer (php installer.php [--build] [--destination=...])
App->>Install: execute()
alt requirements not skipped
Install->>CR: run("check-requirements")
CR->>PR: run(per-tool processes)
PR->>FL: open(log)
PR->>PR: execute & stream output
PR->>FL: write(output)
PR-->>CR: exit code + buffered output
CR-->>Install: present/missing results
end
Install->>Install: prepare destination & download
alt --build requested or confirmed
Install->>CR: run("build") (ahoy build with env, timeout=null)
CR->>PR: run(build process)
PR->>FL: open(build log)
PR->>PR: stream & buffer output
PR->>FL: write(output)
PR-->>CR: exit code + buffered output
CR-->>Build: result
Build->>CLI: show success/failure summary and log path
Install->>PromptMgr: runPostBuild(result)
else
Install->>CLI: finish installation (no build)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (8)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-09-13T04:14:41.765ZApplied to files:
📚 Learning: 2025-07-23T01:16:30.963ZApplied to files:
🧬 Code graph analysis (2).vortex/installer/src/Runner/CommandRunnerAwareTrait.php (2)
.vortex/installer/src/Command/InstallCommand.php (9)
⏰ 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). (10)
🔇 Additional comments (12)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (33)
.vortex/installer/installer.php(1 hunks).vortex/installer/phpstan.neon(1 hunks).vortex/installer/playground/task-streaming.php(1 hunks).vortex/installer/playground/task.php(1 hunks).vortex/installer/playground/tui.php(1 hunks).vortex/installer/src/Command/BuildCommand.php(1 hunks).vortex/installer/src/Command/CheckRequirementsCommand.php(1 hunks).vortex/installer/src/Command/InstallCommand.php(12 hunks).vortex/installer/src/Logger/FileLogger.php(1 hunks).vortex/installer/src/Logger/FileLoggerInterface.php(1 hunks).vortex/installer/src/Logger/LoggableInterface.php(1 hunks).vortex/installer/src/Logger/LoggerInterface.php(1 hunks).vortex/installer/src/Runner/AbstractRunner.php(1 hunks).vortex/installer/src/Runner/CommandRunner.php(1 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Runner/RunnerInterface.php(1 hunks).vortex/installer/src/Task/Task.php(5 hunks).vortex/installer/src/Task/TaskOutput.php(1 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Tui.php(4 hunks).vortex/installer/tests/Functional/Command/BuildCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/InstallCommandTest.php(1 hunks).vortex/installer/tests/Helpers/TuiOutput.php(1 hunks).vortex/installer/tests/Traits/TuiTrait.php(1 hunks).vortex/installer/tests/Unit/ConfigTest.php(2 hunks).vortex/installer/tests/Unit/Logger/FileLoggerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/CommandRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Task/TaskOutputTest.php(1 hunks).vortex/installer/tests/Unit/TaskTest.php(2 hunks).vortex/installer/tests/Unit/TuiTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Logger/FileLoggerInterface.php.vortex/installer/src/Logger/FileLogger.php.vortex/installer/tests/Unit/Logger/FileLoggerTest.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/Functional/Command/CheckRequirementsCommandTest.php.vortex/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/tests/Functional/Command/InstallCommandTest.php.vortex/installer/tests/Unit/Logger/FileLoggerTest.php.vortex/installer/tests/Unit/TuiTest.php.vortex/installer/tests/Unit/TaskTest.php.vortex/installer/tests/Helpers/TuiOutput.php.vortex/installer/tests/Unit/Runner/CommandRunnerTest.php
🧬 Code graph analysis (17)
.vortex/installer/src/Logger/FileLoggerInterface.php (1)
.vortex/installer/src/Logger/FileLogger.php (3)
getPath(85-87)setDir(117-121)getDir(126-132)
.vortex/installer/src/Runner/ProcessRunner.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (9)
AbstractRunner(15-405)reset(201-205)buildCommandString(324-339)initLogger(73-79)resolveOutput(90-92)getCwd(107-114)getExitCode(173-179)parseCommand(226-306)formatArgs(376-403)
.vortex/installer/tests/Functional/Command/BuildCommandTest.php (5)
.vortex/installer/src/Command/BuildCommand.php (2)
BuildCommand(20-213)setRunner(209-211).vortex/installer/src/Command/CheckRequirementsCommand.php (2)
CheckRequirementsCommand(19-355)setRunner(351-353).vortex/installer/src/Runner/ProcessRunner.php (1)
ProcessRunner(15-164).vortex/installer/tests/Helpers/TuiOutput.php (3)
TuiOutput(7-263)present(122-124)absent(135-137).vortex/installer/src/Command/InstallCommand.php (1)
setRunner(647-649)
.vortex/installer/src/Task/TaskOutput.php (2)
.vortex/installer/src/Task/Task.php (1)
Task(11-107).vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)dim(112-116)
.vortex/installer/tests/Unit/ConfigTest.php (1)
.vortex/installer/src/Utils/Config.php (1)
Config(14-128)
.vortex/installer/src/Logger/LoggerInterface.php (3)
.vortex/installer/src/Logger/FileLogger.php (6)
open(44-61)write(66-70)close(75-80)isEnabled(92-94)enable(99-103)disable(108-112).vortex/installer/src/Runner/CommandRunner.php (1)
write(109-118).vortex/installer/src/Task/TaskOutput.php (1)
write(31-36)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (3)
.vortex/installer/src/Runner/ProcessRunner.php (1)
ProcessRunner(15-164).vortex/installer/tests/Helpers/TuiOutput.php (8)
TuiOutput(7-263)present(122-124)absent(135-137)buildRunnerSuccess(159-172)checkRequirementsSuccess(225-240)buildRunnerSuccessProfile(182-195)buildRunnerFailure(205-215)checkRequirementsFailure(250-261).vortex/installer/tests/Traits/TuiTrait.php (1)
tuiOptions(46-52)
.vortex/installer/src/Runner/RunnerInterface.php (3)
.vortex/installer/src/Runner/CommandRunner.php (1)
run(34-72).vortex/installer/src/Runner/ProcessRunner.php (1)
run(20-75).vortex/installer/src/Runner/AbstractRunner.php (5)
getCommand(163-165)getExitCode(173-179)getOutput(184-196)setCwd(97-102)getCwd(107-114)
.vortex/installer/src/Logger/LoggableInterface.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (3)
enableLog(119-123)disableLog(128-132)getLogger(52-58)
.vortex/installer/playground/task-streaming.php (4)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Utils/Tui.php (9)
Tui(17-284)output(40-45)init(25-38)success(60-62)green(84-86)blue(88-90)yellow(96-98)underscore(108-110)bold(104-106).vortex/installer/src/Runner/CommandRunner.php (1)
writeln(128-135).vortex/installer/src/Task/TaskOutput.php (1)
writeln(46-51)
.vortex/installer/src/Runner/AbstractRunner.php (4)
.vortex/installer/src/Logger/FileLogger.php (4)
FileLogger(12-164)setDir(117-121)enable(99-103)disable(108-112).vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)output(40-45).vortex/installer/src/Logger/FileLoggerInterface.php (1)
setDir(29-29).vortex/installer/src/Runner/RunnerInterface.php (5)
getCwd(86-86)setCwd(78-78)getCommand(46-46)getExitCode(54-54)getOutput(67-67)
.vortex/installer/src/Logger/FileLogger.php (2)
.vortex/installer/src/Logger/LoggerInterface.php (6)
open(23-23)write(31-31)close(36-36)isEnabled(44-44)enable(52-52)disable(60-60).vortex/installer/src/Logger/FileLoggerInterface.php (3)
getDir(37-37)getPath(18-18)setDir(29-29)
.vortex/installer/tests/Unit/Logger/FileLoggerTest.php (2)
.vortex/installer/src/Logger/FileLogger.php (10)
FileLogger(12-164)disable(108-112)isEnabled(92-94)enable(99-103)getDir(126-132)setDir(117-121)open(44-61)getPath(85-87)close(75-80)write(66-70).vortex/installer/src/Logger/FileLoggerInterface.php (3)
getDir(37-37)setDir(29-29)getPath(18-18)
.vortex/installer/installer.php (3)
.vortex/installer/src/Command/BuildCommand.php (1)
BuildCommand(20-213).vortex/installer/src/Command/CheckRequirementsCommand.php (1)
CheckRequirementsCommand(19-355).vortex/installer/src/Command/InstallCommand.php (1)
InstallCommand(32-697)
.vortex/installer/tests/Unit/TuiTest.php (4)
.vortex/installer/src/Utils/Tui.php (8)
Tui(17-284)init(25-38)output(40-45)setOutput(47-50)success(60-62)line(80-82)confirm(68-78)getChar(122-141).vortex/installer/src/Utils/Git.php (1)
init(22-24).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
setOutput(599-601).vortex/installer/src/Runner/CommandRunner.php (1)
fetch(137-139)
.vortex/installer/src/Task/Task.php (3)
.vortex/installer/src/Utils/Strings.php (2)
Strings(7-192)isAsciiStart(9-11).vortex/installer/src/Utils/Tui.php (6)
Tui(17-284)output(40-45)setOutput(47-50)yellow(96-98)line(80-82)blue(88-90).vortex/installer/src/Task/TaskOutput.php (2)
TaskOutput(14-123)write(31-36)
.vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (2)
.vortex/installer/src/Logger/FileLogger.php (6)
FileLogger(12-164)getDir(126-132)disable(108-112)isEnabled(92-94)getPath(85-87)close(75-80).vortex/installer/src/Runner/AbstractRunner.php (18)
AbstractRunner(15-405)getLogger(52-58)getCwd(107-114)setCwd(97-102)enableLog(119-123)disableLog(128-132)disableStreaming(154-158)enableStreaming(140-144)getCommand(163-165)getExitCode(173-179)getOutput(184-196)parseCommand(226-306)buildCommandString(324-339)quoteArgument(350-365)formatArgs(376-403)reset(201-205)initLogger(73-79)resolveOutput(90-92)
⏰ 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). (5)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
🔇 Additional comments (77)
.vortex/installer/phpstan.neon (1)
19-20: PHPStan: safer handling of PHPDoc typesSetting
treatPhpDocTypesAsCertain: falseis a sensible choice in a mixed codebase: it avoids over‑trusting annotations while still letting PHPStan infer types from real code paths..vortex/installer/tests/Traits/TuiTrait.php (1)
37-52: tuiOptions helper correctly normalizes CLI optionsThe helper cleanly converts an associative array of options into
--key => valuepairs and matches the documented generic types. This should simplify building expected option arrays in tests..vortex/installer/src/Utils/Tui.php (1)
10-13: New Tui helpers and dim behavior look consistent
setOutput()correctly swaps the underlying output while keeping Tui’s static state coherent and updatingPrompt::setOutput().success()using the Promptsinfo()style gives a distinct “success” look without changing existinginfo()semantics.confirm()’s non‑interactive short‑circuit to$defaultis exactly what you want for automated/CI runs, with a safe empty‑string fallback for$hint.- The
dim()reset substitution should keep text dimmed even when external commands emit raw\033[0mresets.Looks good; just ensure you’ve smoke‑tested an interactive run so
success()/confirm()render as intended in your target terminal.Also applies to: 47-50, 60-62, 68-78, 113-115
.vortex/installer/playground/task.php (1)
1-268: Task playground is comprehensive and safely scopedThe script exercises a wide range of
Task::actionpatterns (return types, success/failure callbacks, hints, streaming) and is clearly marked as a manual playground. As long as it stays out of automated CI hooks, this is a useful debugging/demo tool..vortex/installer/src/Logger/LoggerInterface.php (1)
1-62: LoggerInterface contract is clear and matches usageThe interface cleanly captures the lifecycle you need for command logging (
open→write*→close) plus enable/disable toggling with fluentstaticreturn types. It aligns withFileLoggerand runner usage shown in the snippets..vortex/installer/src/Task/TaskOutput.php (1)
1-123: TaskOutput correctly decorates console output for streaming tasks
TaskOutputis a straightforward decorator:write()/writeln()applyTui::dim()to all messages, while verbosity, decoration, and formatter methods are cleanly forwarded to the wrapped output. This is exactly what you want for dimmed streaming logs, provided it stays in sync with theOutputInterfacemethods of your symfony/console version..vortex/installer/playground/tui.php (1)
1-187: Tui playground thoroughly exercises styling and layout helpersThe script is a well-contained visual demo for Tui’s color, dimming, box, list, width, and centering behavior. It’s a helpful manual tool and doesn’t introduce any production risk.
.vortex/installer/tests/Unit/TuiTest.php (1)
912-990: Additional Tui tests nicely cover new behaviorsThe new tests validate the key additions:
setOutput()swapping,success()output,line()padding, and non‑interactive fallbacks forconfirm()andgetChar(). Assertions are robust (string containment / simple booleans) and don’t over-specify styling, which should keep them stable..vortex/installer/tests/Unit/ConfigTest.php (1)
36-36: BUILD_NOW coverage in tests is completeIncluding
Config::BUILD_NOWin the env cleanup list andtestConstants()keeps behaviour consistent with other config flags and ensures the new constant is exercised.Also applies to: 349-349
.vortex/installer/installer.php (1)
11-12: CLI wiring for new commands looks correct
InstallCommand,CheckRequirementsCommand, andBuildCommandare added explicitly and defaulting to'install'keeps existing UX while exposing the new commands.Also applies to: 20-24
.vortex/installer/src/Utils/Config.php (1)
42-43: New BUILD_NOW config key is well-scoped
Config::BUILD_NOWcleanly extends the existing config key set and matches the installer’s build flag usage without altering any runtime behaviour..vortex/installer/src/Logger/FileLoggerInterface.php (1)
1-39: FileLoggerInterface contract matches current implementation needsThe interface cleanly captures file-specific responsibilities (
getPath(),setDir(),getDir()) on top ofLoggerInterface, and method signatures line up withFileLogger..vortex/installer/src/Logger/LoggableInterface.php (1)
1-36: LoggableInterface cleanly abstracts logging control
enableLog(),disableLog(), andgetLogger()provide a minimal, coherent contract that aligns withAbstractRunnerand allows implementations to return more specific logger types via covariance..vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (1)
1-393: ProcessRunner tests provide good coverage of core behavioursThe new tests thoroughly exercise command execution, streaming toggles, argument/env validation, and cwd handling via
TestableProcessRunner, which should make future changes toProcessRunnermuch safer..vortex/installer/tests/Unit/Task/TaskOutputTest.php (1)
1-128: TaskOutput tests validate the key wrapper semanticsThe suite correctly checks that TaskOutput wraps an OutputInterface, forwards verbosity/decoration/formatter, and passes through (dimmed) content for both write and writeln with scalars and iterables.
.vortex/installer/playground/task-streaming.php (1)
1-196: LGTM! Comprehensive streaming mode demonstration.This playground script effectively demonstrates and tests the various streaming scenarios including echo output,
Tui::output(), mixed modes, failure handling, nested spinners, and styled output. It serves as both documentation and a manual test harness for the streaming feature..vortex/installer/src/Task/Task.php (2)
33-51: Well-structured streaming mode implementation.The streaming path correctly:
- Preserves the original output for restoration
- Uses
TaskOutputwrapper to dim streaming output- Captures both
echooutput (viaob_start) andTui::output()calls (via output swap)- Restores original output after action completes
102-105: LGTM!The
start()helper provides a consistent streaming task header format, complementing the existinglabel()method used in non-streaming mode..vortex/installer/src/Runner/RunnerInterface.php (1)
10-88: Well-designed runner interface.The interface follows good practices:
- Standard exit code constants aligned with shell conventions
- Fluent interface pattern via
selfreturn onrun()- Clear separation of concerns by extending
LoggableInterface- Comprehensive PHPDoc with type annotations including
int<0, 255>range constraint.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (2)
26-64: Well-structured test setup with proper mock injection.The test correctly:
- Uses reference capture (
&$current_command) to track commands across mock calls- Injects the mock via
setRunner()for clean dependency injection- Delegates exit code determination to the data provider's callback for flexible scenario testing
76-393: Comprehensive test coverage for requirements checking.The data provider covers essential scenarios including:
- All requirements present/missing
- Selective requirement checks via
--only- Docker Compose modern vs legacy syntax fallback
- Pygmy status with amazeeio container fallback
- Invalid requirement names handling
--no-summaryoptionBased on learnings, the
*and!prefixes in assertions correctly indicate presence and absence checks respectively..vortex/installer/src/Runner/ProcessRunner.php (2)
136-145: LGTM!The argument preparation correctly merges parsed and additional arguments, then ensures all values are stringified for process execution.
156-162: LGTM!Environment variable validation correctly enforces scalar-only values, preventing potential issues with array or object values being passed to the process.
.vortex/installer/tests/Unit/Runner/CommandRunnerTest.php (4)
1-23: LGTM! Test class setup is well-structured.The
CoversClassattribute is correctly used to specify that this test intends to cover theCommandRunnerclass. The imports and namespace are correctly organized.
57-90: Good coverage of streaming modes.The data-driven approach for testing streaming enabled/disabled is well-designed. The conditional assertion at lines 80-85 properly validates streaming behavior. Same variable shadowing note applies at line 88.
92-155: Good use of reflection for protected method testing.The tests properly exercise the
createCompositeOutputmethod through reflection and verify both single-message and iterable-message code paths. Logger lifecycle is correctly managed with open/close calls.
214-242: Test helper commands are appropriately minimal.The
TestCommandandTestCommandWithExitCodeclasses serve their purpose well for testing the runner. Note that these classes rely on the command name being passed via the constructor (new TestCommand('test:command')), which is the correct Symfony Console pattern..vortex/installer/tests/Functional/Command/InstallCommandTest.php (5)
40-71: Complex mock setup is well-organized.The three-runner mock setup (install, build, check-requirements) with closure-based exit code callbacks provides flexible test configuration. The command tracking via references (
&$install_process_runner_command) enables per-command exit code control.
137-160: Good baseline test case for install without build.This test case correctly verifies that when
--buildflag is not set, the build phase is skipped while installation phases complete successfully.
297-322: Consider asserting multiple missing tool errors.The test case
'Requirements of install command check fails, multiple missing tools'simulates both git and curl failing, but only asserts that the git error is present. If the install command fails fast on the first missing tool, this is correct. Otherwise, consider adding curl error to assertions.
352-377: Comprehensive build success test case.The test properly verifies the complete build flow including success messages, login info, log file path, and next steps guidance. Output assertions cover all expected UI elements.
379-442: Excellent profile-based build test coverage.This test case thoroughly validates the profile-based installation flow with comprehensive assertions covering:
- Install command phases
- Requirements check phases
- Build output (profile-specific)
- Success summary
- Negative assertions for DB-based provisioning and failure states
The assertion structure clearly documents the expected behavior.
.vortex/installer/tests/Functional/Command/BuildCommandTest.php (4)
1-22: Consistent test structure with InstallCommandTest.The file follows the same patterns as
InstallCommandTest.php, including the namespace structure and mock setup approach. Same namespace inconsistency noted as in the previous file.
28-102: Well-structured mock setup for build command testing.The test method properly:
- Sets up build runner mock with configurable exit codes
- Sets up requirements runner with optional callback
- Registers mocked CheckRequirementsCommand
- Runs assertions on output
The default success callback for requirements (line 85) ensures tests that don't need custom requirements behavior work correctly.
235-250: Good use of partial string matching for dynamic assertions.Line 247 shows a pattern for asserting partial content with dynamic values:
'* ' . TuiOutput::BUILD_EXPORT_DATABASE . ' ahoy export-db',This allows verifying both the presence of the constant's content and the associated command.
252-325: Comprehensive failure scenario coverage.The build failure scenarios test multiple exit codes (1, 2, 127) and verify:
- Build failure message is shown
- Exit code is displayed correctly
- Success messages are absent
- Log file path is shown on failure
This covers the key failure paths described in the PR objectives.
.vortex/installer/src/Logger/FileLogger.php (2)
63-80: Write and close methods follow expected patterns.The
write()method silently ignores writes when no handle is open, which is appropriate for a logging utility that shouldn't interrupt the main process. Theclose()method properly nullifies the handle after closing.
134-162: Well-designed filename sanitization.The
buildFilename()method properly:
- Filters out option arguments (prefixed with
-)- Sanitizes for filesystem safety with regex
- Collapses consecutive dashes
- Provides a sensible fallback (
'runner')This ensures safe, readable log filenames regardless of input.
.vortex/installer/src/Runner/CommandRunner.php (2)
85-144: Well-implemented composite output pattern.The anonymous class elegantly combines three output destinations (buffer, stream, log) into a single
OutputInterface. The implementation correctly handles:
- Both string and iterable messages
- Newline handling for
write()vswriteln()- Conditional streaming based on
$shouldStreamflag- Logging all output regardless of streaming mode
34-57: Clean command execution flow.The
run()method has a well-organized execution flow:
- Reset state and build command string
- Find and validate Symfony command
- Initialize logger with positional arguments
- Create composite output for capture/stream/log
- Execute command with proper exit code validation
The exit code range validation (0-255) correctly enforces valid shell exit codes.
.vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (8)
1-20: LGTM - Well-structured test setup.The test class is properly organized with appropriate PHPUnit attributes and namespace. The
#[CoversClass]attribute correctly targets theAbstractRunnerclass.
25-66: LGTM - Logger and directory management tests.Tests appropriately verify lazy initialization, singleton behavior, and the coordination between
setCwdand the logger's directory. Usingself::$tmpfor filesystem tests is a good practice.
68-125: LGTM - Enable/disable and streaming tests.Tests correctly verify method chaining returns and state transitions for both logging and streaming controls.
127-165: LGTM - Initial state and output variation tests.Tests verify sensible defaults (null command, zero exit code, empty output) and the data-driven approach for output variations covers the key scenarios.
167-220: LGTM - Command parsing and formatting tests.Data-driven tests for
parseCommand,buildCommandString,quoteArgument, andformatArgscover edge cases including quoting, escaping, and error conditions.
222-291: LGTM - Reset, initLogger, and resolveOutput tests.Tests verify state reset behavior and output resolution with both null and provided outputs. The Tui mock initialization in
testResolveOutputWithNullis appropriate.
295-536: LGTM - Comprehensive data providers.Data providers cover a wide range of scenarios including edge cases like empty strings, special characters, unclosed quotes, and trailing escapes. The test cases for
formatArgscorrectly verify boolean handling and mixed positional/named arguments.
543-635: LGTM - Well-designed test double.
ConcreteRunnerprovides clean public wrappers for protected methods, enabling thorough unit testing ofAbstractRunner. The exit code validation insetExitCodemirrors the production code's constraints..vortex/installer/tests/Unit/Logger/FileLoggerTest.php (5)
1-39: LGTM - Enable/disable tests with chaining verification.Tests correctly verify state transitions and method chaining for enable/disable operations.
41-106: LGTM - Directory management and open tests.Tests verify default cwd behavior, custom directory setting, and the
open()method's behavior in both enabled and disabled states. Proper cleanup withFile::remove()is good practice.
146-179: LGTM - Close method tests.Tests verify idempotent close behavior and that writes after close are no-ops. Good coverage of the close lifecycle.
181-231: LGTM - GetPath and buildFilename tests.Tests verify path behavior before/after open and with disabled logging. The filename pattern verification correctly strips the timestamp suffix.
233-389: LGTM - Comprehensive data providers.Data providers cover various scenarios including special characters, options filtering, and the fallback to 'runner' for empty results. Good edge case coverage.
.vortex/installer/src/Command/BuildCommand.php (4)
1-54: LGTM - Command configuration.The command is well-configured with clear name, description, help text, and options. Constants are appropriately defined for option names.
59-112: LGTM - Execute method implementation.The execution flow correctly handles requirements checking and build orchestration. Environment variables for profile builds are properly set, and the Task::action pattern provides good UX with streaming output.
114-129: LGTM - Project name extraction.The fallback to directory name when
.envdoesn't containVORTEX_PROJECTis a sensible default.
194-212: LGTM - Runner getter/setter for dependency injection.The factory method pattern with setter injection enables testing while providing sensible defaults.
.vortex/installer/src/Command/CheckRequirementsCommand.php (5)
1-75: LGTM - Command configuration and constants.Well-organized constants for requirements and options. The
REQUIREMENTSarray provides a single source of truth for available checks.
77-131: LGTM - Execute method with validation and check orchestration.The execution flow is clean: validate requirements, run checks, display summary. The
--no-summaryhandling with a fallback success message is good UX.
133-222: LGTM - Validation and result accessors.
validateRequirementsprovides clear error messages for invalid inputs. The result accessors (getPresent,getMissing,getResults,getResultsSummary) are well-designed for both display and testing purposes.
224-264: LGTM - Individual tool checks.The Docker, Docker Compose, and Ahoy checks follow a consistent pattern: check existence, record version if present, record installation URL if missing.
294-354: LGTM - Helper methods and dependency injection.
commandExists,dockerComposeExists, andgetCommandVersionare well-implemented utilities. The runner getter/setter pattern enables testing..vortex/installer/src/Runner/AbstractRunner.php (6)
1-58: LGTM - Class structure and lazy logger initialization.The abstract class properly implements
RunnerInterfaceand provides sensible defaults. Lazy initialization of the logger is efficient.
60-114: LGTM - Logger initialization and directory management.
initLoggercorrectly sets the directory and opens a log file.setCwdandgetCwdproperly synchronize the logger directory.
116-179: LGTM - Logging/streaming controls and accessors.Enable/disable methods for logging and streaming support method chaining. The exit code validation (0-255 range) is appropriately defensive.
181-206: LGTM - Output retrieval and reset.
getOutputcorrectly handles line limiting and array/string return types.resetproperly clears state for reuse.
207-306: LGTM - Robust command parser.The parser correctly handles:
- Single and double quotes
- Backslash escaping (with documented deviation from POSIX for single quotes)
- End-of-options marker (--)
- Error conditions (empty command, unclosed quotes, trailing escape)
The state machine approach is clean and well-documented.
308-403: LGTM - Command string building and argument formatting.
buildCommandStringproduces safe, copy-pasteable commands.quoteArgumentproperly handles shell special characters using single-quote escaping.formatArgscorrectly converts boolean options to flags and excludes false values..vortex/installer/src/Command/InstallCommand.php (4)
628-695: LGTM!The dependency injection pattern using nullable properties with lazy instantiation in getters is well-implemented. This provides clean testability while maintaining sensible defaults for production use.
67-80: LGTM!The nullable property declarations with proper PHPDoc annotations are well-structured and support the dependency injection pattern used throughout the class.
310-312: LGTM!The
BUILD_NOWconfiguration flag is correctly wired from the CLI option, following the established pattern for other options in this method.
225-239: I'll help you verify the PHPStan suppression comments in the InstallCommand.php file. Let me start by examining the ProcessRunner implementation and the RunnerInterface to understand the type annotation issue.
<tool_calls>
First, find the ProcessRunner class and RunnerInterface
find . -type f -name "*.php" | xargs grep -l "class ProcessRunner|interface RunnerInterface" | head -20
</function_calls>Now let me examine the ProcessRunner and RunnerInterface implementations:
<function_calls>
Find and examine ProcessRunner class definition with getExitCode method
ast-grep --pattern $'class ProcessRunner {
$$$
getExitCode() {
$$$
}
$$$
}'
</function_calls>Let me try a different approach to locate these files:
<function_calls>
Search for ProcessRunner and RunnerInterface
rg -n "class ProcessRunner" -A 20
</function_calls><function_calls>
Search for RunnerInterface
rg -n "interface RunnerInterface" -A 10
</function_calls><function_calls>
Search for getExitCode method definition
rg -n "getExitCode()" -B 2 -A 5
</function_calls><function_calls>
Check the InstallCommand file itself to understand context
cat -n .vortex/installer/src/Command/InstallCommand.php | head -50
</function_calls><function_calls>
Look at the imports and surrounding context of the reviewed lines
sed -n '1,30p' .vortex/installer/src/Command/InstallCommand.php
</function_calls><function_calls>
Find the full method containing the reviewed code
sed -n '210,245p' .vortex/installer/src/Command/InstallCommand.php
</function_calls><function_calls>
Search for where $runner is defined in the method
sed -n '200,245p' .vortex/installer/src/Command/InstallCommand.php
</function_calls><function_calls>
Get the full method context
sed -n '190,250p' .vortex/installer/src/Command/InstallCommand.php
</function_calls><function_calls>
Check file length and read a larger section
wc -l .vortex/installer/src/Command/InstallCommand.php
</function_calls><function_calls>
Read lines 1-100 to see imports and class structure
sed -n '1,100p' .vortex/installer/src/Command/InstallCommand.php
</function_calls><function_calls>
Read the method containing the reviewed lines
sed -n '175,245p' .vortex/installer/src/Command/InstallCommand.php
</function_calls>Now let me find the ProcessRunner and RunnerInterface definitions:
<function_calls>
Find files that might contain ProcessRunner
find .vortex/installer -type f -name "*.php" | xargs grep -l "ProcessRunner|RunnerInterface"
</function_calls><function_calls>
Look for these in src directory
fd ".php$" .vortex/installer/src | xargs grep -l "ProcessRunner|RunnerInterface" 2>/dev/null
</function_calls><function_calls>
Search more broadly for these classes
fd ".php$" | xargs grep -l "class ProcessRunner|interface RunnerInterface" 2>/dev/null
</function_calls><function_calls>
Check the directory structure
ls -la .vortex/installer/src/
</function_calls><function_calls>
List all PHP files in the installer
find .vortex/installer -type f -name "*.php"
</function_calls><function_calls>
Look at full InstallCommand file to find RunnerInterface usage
cat .vortex/installer/src/Command/InstallCommand.php
</function_calls><function_calls>
Search for RunnerInterface anywhere in repo
rg "RunnerInterface" -n
</function_calls><function_calls>
Search for ProcessRunner in the entire repo
rg "ProcessRunner" -n
</function_calls><function_calls>
Check for phpstan config to understand the analysis
find . -type f -name "phpstan.neon*" -o -name "phpstan.json"
</function_calls><function_calls>
Search for EXIT_SUCCESS constant
rg "EXIT_SUCCESS" -n
</function_calls><function_calls>
Search more flexibly for exit code related constants
rg "getExitCode|EXIT_" -n
</function_calls><function_calls>
Look at git log to understand recent changes to this file
git log --oneline -- .vortex/installer/src/Command/InstallCommand.php | head -10
</function_calls><function_calls>
Check if the file actually exists and look at the exact lines mentioned
test -f ".vortex/installer/src/Command/InstallCommand.php" && sed -n '215,245p' ".vortex/installer/src/Command/InstallCommand.php" && echo "---FILE EXISTS---" || echo "FILE NOT FOUND"
</function_calls><function_calls>
List the repository root files to understand structure
ls -la | head -30
</function_calls><function_calls>
Find any .vortex directory
find . -type d -name ".vortex" 2>/dev/null
</function_calls><function_calls>
Search for InstallCommand in entire repo
find . -type f -name "InstallCommand"
</function_calls><function_calls>
Get the git branch and status
git branch -a && git status
</function_calls><function_calls>
Look for all PHP files containing ProcessRunner or similar
rg "class.*Runner" -n
</function_calls><function_calls>
Find PHP files with "getExitCode" method
rg "function getExitCode|public.*getExitCode" -n -A 3
</function_calls>Based on my investigation, I'm unable to locate the specific file or classes mentioned in the review comment within the repository. The file
.vortex/installer/src/Command/InstallCommand.phpand related classes (ProcessRunner, RunnerInterface) don't appear to be present in the current repository state.Let me verify this with a broader search:
<function_calls>
Get a list of all PHP files in repo
find . -type f -name "*.php" | head -30
</function_calls><function_calls>
Check the git log for files modified recently
git log --name-only --oneline | head -50
</function_calls>The specified file
.vortex/installer/src/Command/InstallCommand.phpand related classes (ProcessRunner, RunnerInterface) cannot be found in the current repository. The file path, class definitions, and method signatures referenced in the review comment do not exist in the accessible codebase. This suggests either:
- The file has been removed or moved
- The repository state differs from when the review comment was created
- The file exists in a branch or commit not currently checked out
Without access to the actual implementation files, I cannot verify:
- The ProcessRunner::getExitCode() return type annotation
- The RunnerInterface::EXIT_SUCCESS constant definition
- Whether the PHPStan warnings are legitimate type issues or false positives
- The actual behavior and design of the runner implementation
.vortex/installer/tests/Helpers/TuiOutput.php (5)
1-7: LGTM!The namespace and class declaration follow PSR-4 conventions and are appropriately placed under the test helpers namespace.
9-111: LGTM!The constants are well-organized by category with clear, descriptive names. This centralized approach will make tests more maintainable when output messages change.
122-137: LGTM!The
present()andabsent()helper methods correctly implement the marker convention (*for expected presence,!for expected absence) as documented in the project's test patterns. Based on learnings, this aligns with the cmd/cmdFail helper patterns used in PHPUnit tests.
145-149: LGTM!The
echo()method is a straightforward utility that outputs each constant value, enabling the factory methods to simulate realistic command output.
225-261: LGTM!The
checkRequirementsSuccess()andcheckRequirementsFailure()factory methods correctly simulate the requirements checking flow with appropriate output sequences and exit codes.
.vortex/installer/tests/Functional/Command/InstallCommandTest.php
Outdated
Show resolved
Hide resolved
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
7cc1b5a to
3fc9658
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (18)
.vortex/installer/tests/Unit/TaskTest.php (1)
285-311: Type mismatch persists: success callback expects string but action returns int.The success callback on line 296 is typed to accept
string $result, but the action returnsint(42). This will cause aTypeErrorat runtime when the result is passed to the success callback.Apply the fix suggested in the previous review:
- success: fn(string $result): string => sprintf('Processed %s items', $result), + success: fn($result): string => sprintf('Processed %s items', $result),.vortex/installer/src/Logger/FileLogger.php (2)
57-60: Handle resource error handling issue persists.If
fopen()fails and returnsFALSE,$this->handlewill beFALSE(notNULL). Thewrite()method on line 67 checksif ($this->handle !== NULL), which would pass when$this->handleisFALSE, potentially causing issues.Apply the fix from the previous review to set handle to NULL on failure:
// @phpstan-ignore-next-line $this->handle = fopen($this->path, 'w'); - return $this->handle !== FALSE; + if ($this->handle === FALSE) { + $this->handle = NULL; + return FALSE; + } + + return TRUE;
126-132: getcwd() false return value not handled.
getcwd()can returnfalseif the current working directory has been deleted. The cast to string on line 128 would convertfalseto an empty string"", resulting in invalid log file paths.Apply the fix from the previous review:
public function getDir(): string { if ($this->dir === '') { - $this->dir = (string) getcwd(); + $cwd = getcwd(); + if ($cwd === FALSE) { + throw new \RuntimeException('Unable to determine current working directory'); + } + $this->dir = $cwd; } return $this->dir; }.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (1)
76-187: Broad coverage is good; simplify command-matching in callbacks.The matrix of scenarios in
dataProviderCheckRequirementsCommand()gives solid coverage of success/missing/filtered checks and Pygmy/Docker Compose branches, and the use ofTuiOutput::present/absent()lines up with the test harness’*/!semantics. Based on learnings, this matches how output assertions are interpreted.The “Docker missing” callback:
if ((str_contains($current_command, "command -v 'docker'") || str_contains($current_command, 'command -v docker')) && !str_contains($current_command, 'compose') && !str_contains($current_command, 'docker --version')) { return RunnerInterface::EXIT_COMMAND_NOT_FOUND; }is quite brittle to small formatting changes in the built command string.
Consider extracting a small helper or using a concise regex that explicitly matches the “command -v docker” probe and excludes compose/version probes, e.g. a helper like
isDockerProbe($current_command)shared with other tests..vortex/installer/tests/Functional/Command/BuildCommandTest.php (1)
115-177: Strong scenario coverage; factor out complex Docker-missing pattern.The
dataProviderBuildCommand()cases exercise requirements integration, profile flows, exit‑code handling, and success/failure summaries very well.The “Requirements check fails – one missing (Docker)” scenario reuses the same multi‑
str_containslogic as in other tests:if ((str_contains($current_command, "command -v 'docker'") || str_contains($current_command, 'command -v docker')) && !str_contains($current_command, 'compose') && !str_contains($current_command, 'docker --version')) { return RunnerInterface::EXIT_COMMAND_NOT_FOUND; }This is fragile; a slight change in how the command string is built could break the test unintentionally.
Consider extracting a small helper (shared across functional tests) or using a regex that explicitly matches the docker existence check while excluding compose/version invocations, to centralise and harden this logic.
.vortex/installer/src/Task/Task.php (1)
33-51: Streaming branch must restore output/buffers on exceptions, and callback type should bestring.Two issues in the streaming path:
- Missing try/finally around output redirection
If
$action()throws,ob_end_clean()andTui::setOutput($original_output)are never called, leaving an open output buffer andTuistill pointing atTaskOutput. That can corrupt later CLI output and diagnostics.Wrap the streaming block in a try/finally:
- if ($streaming) { - $original_output = Tui::output(); - $task_output = new TaskOutput($original_output); - static::start($label); - Tui::setOutput($task_output); - - ob_start(function (string|iterable $buffer) use ($task_output): string { - $task_output->write($buffer); - return ''; - }, 1); - - $return = $action(); - - ob_end_clean(); - Tui::setOutput($original_output); - } + if ($streaming) { + $original_output = Tui::output(); + $task_output = new TaskOutput($original_output); + + static::start($label); + Tui::setOutput($task_output); + + ob_start(function (string $buffer) use ($task_output): string { + $task_output->write($buffer); + return ''; + }, 1); + + try { + $return = $action(); + } + finally { + ob_end_clean(); + Tui::setOutput($original_output); + } + }
- Incorrect parameter type for
ob_startcallback (previously flagged)PHP’s output buffering callback receives a
stringbuffer, notstring|iterable. Narrowing the type tostring(as in the diff) matches the actual signature and avoids confusion.The new
start()helper itself looks fine and correctly keeps the header undimmed by writing before swappingTui’s output.Also applies to: 102-105
.vortex/installer/src/Runner/ProcessRunner.php (1)
20-75: Fix output accumulation, exit-code mapping, and path-command validation in ProcessRunner.There are three related issues here:
- Output is overwritten instead of accumulated (previously flagged)
In the process callback:
$process->run(function ($type, string|iterable $buffer) use ($logger, $output): void { $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; $this->output = $buffer; if ($this->shouldStream) { $output->write($buffer); } $logger->write($buffer); });
$this->outputis replaced on every chunk, so only the last chunk is preserved. This breaksgetOutput()and logging expectations for long‑running commands.Accumulate instead:
- $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; - $this->output = $buffer; + $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; + $this->output .= $buffer;
- Exit-code mapping is immediately overwritten (previously flagged)
You correctly map the raw code to
RunnerInterfaceconstants:match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, };but then immediately do:
$this->exitCode = $exit_code;which discards the mapping and reverts to the raw code. Even though the constants currently match the raw values, this defeats the intent and could bite later if the mapping changes.
Drop the overwrite:
- match ($exit_code) { + match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - - $this->exitCode = $exit_code;
- Path-based commands are not validated for executability in all cases (previously flagged)
In
resolveCommand():if (str_contains((string) $base_command, '/')) { $resolved = $base_command; if (!str_starts_with((string) $base_command, '/')) { $full_path = $this->getCwd() . '/' . $base_command; if (is_executable($full_path)) { $resolved = $full_path; } } }If
$base_commandis an absolute path or a relative path that isn’t executable viagetCwd(),$resolvedis left as‑is without checking whether it actually exists or is executable. That defers failure to theProcesslayer with a less clear error.Add a final executability check for path commands:
- if (str_contains((string) $base_command, '/')) { - $resolved = $base_command; - if (!str_starts_with((string) $base_command, '/')) { - $full_path = $this->getCwd() . '/' . $base_command; - if (is_executable($full_path)) { - $resolved = $full_path; - } - } - } + if (str_contains((string) $base_command, '/')) { + $resolved = $base_command; + if (!str_starts_with((string) $base_command, '/')) { + $full_path = $this->getCwd() . '/' . $base_command; + if (is_executable($full_path)) { + $resolved = $full_path; + } + } + if (!is_executable($resolved)) { + throw new \InvalidArgumentException(sprintf('Command not found or not executable: %s', $base_command)); + } + }Together, these changes will make
ProcessRunner’s behavior much more predictable for long‑running commands, exit‑code handling, and path‑based executables.Also applies to: 89-120
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (1)
5-5: Namespace inconsistency: useDrevOps\VortexInstaller\Tests\Functional\Command.The namespace
DrevOps\Installer\Tests\Functional\Commanddoes not match the project convention used in imports (lines 7-19) and other test files. This will cause PSR-4 autoloading issues..vortex/installer/tests/Unit/Runner/CommandRunnerTest.php (1)
46-54: Variable shadowing reduces readability.The variable
$outputis assigned aBufferedOutputon line 46, then reassigned to the runner output on line 53. While functionally correct, reusing the variable name for different types makes the test harder to follow..vortex/installer/tests/Unit/Logger/FileLoggerTest.php (1)
138-143: Consider verifying the no-op behavior more explicitly.The placeholder assertion
$this->assertTrue(TRUE, ...)doesn't actually verify thatwrite()is a no-op when the logger is not open. Consider asserting thatgetPath()remainsNULLand no file was created..vortex/installer/src/Runner/CommandRunner.php (1)
59-66: Critical: Exit code mapping is immediately overwritten.The
matchexpression (lines 59-64) maps Symfony Command exit codes to internalRunnerInterfaceconstants, but line 66 immediately overwrites$this->exitCodewith the raw$exit_code, completely negating the mapping. This breaks the exit code abstraction layer..vortex/installer/src/Command/BuildCommand.php (2)
142-142: Potential null pointer access on$this->runner.If
showSuccessSummary()is called when$this->runnerisNULL, accessing$this->runner->getLogger()->getPath()will cause a fatal error. Consider adding a null-safe check.
179-183: Unreachable exception:getOutput(as_array: TRUE)always returns an array.The method signature guarantees that
getOutput(as_array: TRUE)returns an array, making the!is_array()check and subsequent exception unreachable code..vortex/installer/src/Command/CheckRequirementsCommand.php (1)
270-272: Misleading Pygmy installation guidance.When the
pygmycommand is not found, the message "Run: pygmy up" is misleading because users must first install Pygmy. Consider providing an installation URL instead..vortex/installer/src/Command/InstallCommand.php (3)
202-208: Build failure doesn't halt installation or return failure exit code.When
runBuildCommand()returnsfalse, the installation continues and returnsCommand::SUCCESS. Per PR objectives, failures should "show where it stopped" and provide retry guidance.
193-200: Missing CI environment detection per PR objectives.The PR objectives specify "always skip auto-build in CI with a printed note," but there's no check for CI environment variables (e.g.,
CI,GITHUB_ACTIONS,GITLAB_CI).
445-445: Inconsistent exit code constant usage.This method uses
Command::SUCCESSwhilecheckRequirements()usesRunnerInterface::EXIT_SUCCESS. For consistency, use the same constant throughout the file..vortex/installer/tests/Helpers/TuiOutput.php (1)
159-261: Closures ignore the$commandparameter; document or mark it intentionally unused.All the runner/requirements simulation factories (
buildRunnerSuccess(),buildRunnerSuccessProfile(),buildRunnerFailure(),checkRequirementsSuccess(),checkRequirementsFailure()) return closures with astring $commandparameter that is never used:return function (string $command): int { // ... };This is clearly for interface compatibility, but leaving it unused without a hint can be confusing. You could:
- Prefix the parameter name with an underscore to signal intent:
- return function (string $command): int { + return function (string $_command): int {
- Or add a short inline comment inside the closure explaining it’s unused but required by the callback contract.
This is the same concern already noted in a past review on this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (33)
.vortex/installer/installer.php(1 hunks).vortex/installer/phpstan.neon(1 hunks).vortex/installer/playground/task-streaming.php(1 hunks).vortex/installer/playground/task.php(1 hunks).vortex/installer/playground/tui.php(1 hunks).vortex/installer/src/Command/BuildCommand.php(1 hunks).vortex/installer/src/Command/CheckRequirementsCommand.php(1 hunks).vortex/installer/src/Command/InstallCommand.php(12 hunks).vortex/installer/src/Logger/FileLogger.php(1 hunks).vortex/installer/src/Logger/FileLoggerInterface.php(1 hunks).vortex/installer/src/Logger/LoggableInterface.php(1 hunks).vortex/installer/src/Logger/LoggerInterface.php(1 hunks).vortex/installer/src/Runner/AbstractRunner.php(1 hunks).vortex/installer/src/Runner/CommandRunner.php(1 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Runner/RunnerInterface.php(1 hunks).vortex/installer/src/Task/Task.php(5 hunks).vortex/installer/src/Task/TaskOutput.php(1 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Tui.php(4 hunks).vortex/installer/tests/Functional/Command/BuildCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/InstallCommandTest.php(1 hunks).vortex/installer/tests/Helpers/TuiOutput.php(1 hunks).vortex/installer/tests/Traits/TuiTrait.php(1 hunks).vortex/installer/tests/Unit/ConfigTest.php(2 hunks).vortex/installer/tests/Unit/Logger/FileLoggerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/CommandRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Task/TaskOutputTest.php(1 hunks).vortex/installer/tests/Unit/TaskTest.php(2 hunks).vortex/installer/tests/Unit/TuiTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/Functional/Command/InstallCommandTest.php.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php.vortex/installer/tests/Unit/TaskTest.php.vortex/installer/tests/Unit/TuiTest.php.vortex/installer/tests/Unit/Runner/CommandRunnerTest.php.vortex/installer/tests/Unit/Logger/FileLoggerTest.php.vortex/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/tests/Helpers/TuiOutput.php
📚 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/installer/tests/Functional/Command/InstallCommandTest.php
📚 Learning: 2025-06-02T00:41:36.321Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
Applied to files:
.vortex/installer/tests/Functional/Command/InstallCommandTest.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/tests/Unit/Logger/FileLoggerTest.php.vortex/installer/src/Logger/FileLogger.php.vortex/installer/src/Logger/FileLoggerInterface.php
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
.vortex/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/src/Command/InstallCommand.php
🧬 Code graph analysis (19)
.vortex/installer/tests/Unit/ConfigTest.php (1)
.vortex/installer/src/Utils/Config.php (1)
Config(14-128)
.vortex/installer/tests/Unit/Task/TaskOutputTest.php (1)
.vortex/installer/src/Task/TaskOutput.php (13)
TaskOutput(14-123)write(31-36)writeln(46-51)setVerbosity(56-58)getVerbosity(63-65)isQuiet(70-72)isVerbose(77-79)isVeryVerbose(84-86)isDebug(91-93)setDecorated(98-100)isDecorated(105-107)setFormatter(112-114)getFormatter(119-121)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (3)
.vortex/installer/src/Downloader/Downloader.php (1)
Downloader(17-271).vortex/installer/src/Runner/ProcessRunner.php (1)
ProcessRunner(15-164).vortex/installer/tests/Traits/TuiTrait.php (1)
tuiOptions(46-52)
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (2)
.vortex/installer/src/Command/CheckRequirementsCommand.php (2)
CheckRequirementsCommand(19-355)setRunner(351-353).vortex/installer/tests/Helpers/TuiOutput.php (3)
TuiOutput(7-263)present(122-124)absent(135-137)
.vortex/installer/tests/Unit/TaskTest.php (3)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Utils/Tui.php (5)
output(40-45)Tui(17-284)init(25-38)line(80-82)success(60-62).vortex/installer/src/Task/TaskOutput.php (2)
writeln(46-51)write(31-36)
.vortex/installer/installer.php (3)
.vortex/installer/src/Command/BuildCommand.php (1)
BuildCommand(20-213).vortex/installer/src/Command/CheckRequirementsCommand.php (1)
CheckRequirementsCommand(19-355).vortex/installer/src/Command/InstallCommand.php (1)
InstallCommand(32-697)
.vortex/installer/tests/Unit/TuiTest.php (1)
.vortex/installer/src/Utils/Tui.php (7)
Tui(17-284)init(25-38)output(40-45)setOutput(47-50)line(80-82)confirm(68-78)getChar(122-141)
.vortex/installer/tests/Traits/TuiTrait.php (11)
.vortex/installer/src/Prompts/Handlers/CodeProvider.php (1)
options(39-44).vortex/installer/src/Prompts/Handlers/CiProvider.php (1)
options(34-46).vortex/installer/src/Prompts/Handlers/DatabaseDownloadSource.php (1)
options(41-62).vortex/installer/src/Prompts/Handlers/HostingProvider.php (1)
options(45-52).vortex/installer/src/Prompts/Handlers/ProvisionType.php (1)
options(56-61).vortex/installer/src/Prompts/Handlers/Profile.php (1)
options(44-51).vortex/installer/src/Prompts/Handlers/Theme.php (1)
options(45-52).vortex/installer/src/Prompts/Handlers/Services.php (1)
options(35-41).vortex/installer/src/Prompts/Handlers/AbstractHandler.php (1)
options(95-97).vortex/installer/src/Prompts/Handlers/HandlerInterface.php (1)
options(76-76).vortex/installer/src/Prompts/Handlers/DependencyUpdatesProvider.php (1)
options(34-40)
.vortex/installer/playground/task.php (2)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Utils/Tui.php (4)
Tui(17-284)output(40-45)init(25-38)success(60-62)
.vortex/installer/src/Runner/CommandRunner.php (3)
.vortex/installer/src/Runner/AbstractRunner.php (4)
AbstractRunner(15-405)reset(201-205)initLogger(73-79)resolveOutput(90-92).vortex/installer/src/Runner/RunnerInterface.php (1)
run(41-41).vortex/installer/src/Logger/LoggerInterface.php (2)
close(36-36)write(31-31)
.vortex/installer/src/Runner/RunnerInterface.php (4)
.vortex/installer/src/Runner/CommandRunner.php (1)
run(34-72).vortex/installer/src/Runner/ProcessRunner.php (1)
run(20-75).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
run(548-552).vortex/installer/src/Runner/AbstractRunner.php (5)
getCommand(163-165)getExitCode(173-179)getOutput(184-196)setCwd(97-102)getCwd(107-114)
.vortex/installer/src/Command/CheckRequirementsCommand.php (4)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Runner/RunnerInterface.php (3)
run(41-41)getExitCode(54-54)getOutput(67-67).vortex/installer/src/Runner/AbstractRunner.php (3)
getExitCode(173-179)getOutput(184-196)disableLog(128-132).vortex/installer/src/Logger/LoggableInterface.php (1)
disableLog(26-26)
.vortex/installer/tests/Functional/Command/BuildCommandTest.php (3)
.vortex/installer/src/Command/CheckRequirementsCommand.php (1)
CheckRequirementsCommand(19-355).vortex/installer/src/Runner/ProcessRunner.php (1)
ProcessRunner(15-164).vortex/installer/tests/Helpers/TuiOutput.php (2)
TuiOutput(7-263)present(122-124)
.vortex/installer/src/Task/TaskOutput.php (2)
.vortex/installer/src/Task/Task.php (1)
Task(11-107).vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)dim(112-116)
.vortex/installer/src/Logger/LoggerInterface.php (1)
.vortex/installer/src/Logger/FileLogger.php (6)
open(44-61)write(66-70)close(75-80)isEnabled(92-94)enable(99-103)disable(108-112)
.vortex/installer/src/Logger/FileLogger.php (2)
.vortex/installer/src/Logger/LoggerInterface.php (6)
open(23-23)write(31-31)close(36-36)isEnabled(44-44)enable(52-52)disable(60-60).vortex/installer/src/Runner/CommandRunner.php (1)
write(109-118)
.vortex/installer/playground/task-streaming.php (4)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Utils/Tui.php (9)
Tui(17-284)output(40-45)init(25-38)success(60-62)green(84-86)blue(88-90)yellow(96-98)underscore(108-110)bold(104-106).vortex/installer/src/Runner/CommandRunner.php (1)
writeln(128-135).vortex/installer/src/Task/TaskOutput.php (1)
writeln(46-51)
.vortex/installer/src/Runner/AbstractRunner.php (4)
.vortex/installer/src/Logger/FileLogger.php (5)
FileLogger(12-164)setDir(117-121)open(44-61)enable(99-103)disable(108-112).vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)output(40-45).vortex/installer/src/Logger/FileLoggerInterface.php (1)
setDir(29-29).vortex/installer/src/Runner/RunnerInterface.php (2)
getCwd(86-86)setCwd(78-78)
.vortex/installer/src/Logger/FileLoggerInterface.php (1)
.vortex/installer/src/Logger/FileLogger.php (3)
getPath(85-87)setDir(117-121)getDir(126-132)
⏰ 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). (6)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-docs
🔇 Additional comments (42)
.vortex/installer/phpstan.neon (1)
19-20: Based on my verification, the original review comment is accurate and appropriate. Here are my findings:Verification Results:
- ✓ The parameter
treatPhpDocTypesAsCertain: falseexists and is correctly documented in PHPStan 2.1.32- ✓ The default value is
true, so this IS a deliberate change from default behavior- ✓ It was added in commit 3fc9658 as part of the auto-build feature ([#1950])
- ✓ The parameter DOES globally affect how PHPStan treats PHPDoc types across all configured paths
- ✓ The configuration already includes specific
ignoreErrorsrules for known issues (test data providers, bound callbacks)- ✓ The commit message provides no explanation for why this parameter was needed
Why the review comment is valid:
The concern raised is legitimate: without clear documentation of intent, it's unclear whether this is an intentional architectural decision for the auto-build code or a workaround for PHPDoc type issues. The suggestion to use targeted
ignoreErrorsrules instead (if it's a workaround) follows best practices.
Confirm global change to PHPStan's treatment of PHPDoc types
Setting
treatPhpDocTypesAsCertain: false(added in [#1950] for auto-build) changes analysis behaviour across the whole installer: PHPStan will rely less on PHPDoc when inferring types, which can surface more conservative/"mixed" typing in both the new auto-build code and existing installer code.Please confirm this is the desired behaviour for
phpstan/phpstan2.1.32 in this repo. If this was added to work around specific PHPDoc issues in the auto-build code, consider using targetedignoreErrorsrules or a baseline instead of relaxing PHPDoc trust globally..vortex/installer/tests/Unit/TaskTest.php (7)
134-170: LGTM!The streaming test properly validates OutputInterface usage, captures dimmed output, and verifies start/success messages.
178-209: LGTM!The test correctly validates that echo/print statements are captured via output buffering during streaming mode.
217-249: LGTM!The mixed output test comprehensively validates that both OutputInterface and echo/print outputs are captured correctly.
254-280: LGTM!The failure test properly validates that streaming output is captured even when the action returns FALSE and the failure message is displayed.
316-335: LGTM!The test correctly validates that the original output is restored after streaming completes.
340-358: LGTM!The test validates the edge case where streaming is enabled but no output is produced by the action.
367-412: LGTM!The test thoroughly validates that when the action directly uses the output parameter (via
Tui::output()), all output is properly captured and not leaked..vortex/installer/src/Utils/Config.php (1)
42-43: LGTM!The new BUILD_NOW constant follows the established naming pattern and integrates cleanly with the existing configuration constants.
.vortex/installer/tests/Unit/ConfigTest.php (2)
36-36: LGTM!Correctly added BUILD_NOW to the test setup to ensure clean test isolation.
349-349: LGTM!The assertion correctly validates the BUILD_NOW constant value.
.vortex/installer/installer.php (1)
11-24: LGTM!The application correctly registers all three commands (InstallCommand, CheckRequirementsCommand, BuildCommand) and sets the appropriate default command. The refactoring from a single-command to multi-command structure is clean.
.vortex/installer/src/Logger/FileLoggerInterface.php (1)
1-39: LGTM!The FileLoggerInterface is well-designed with clear method signatures and comprehensive PHPDoc annotations. The interface appropriately extends LoggerInterface to provide file-specific logging capabilities.
.vortex/installer/src/Logger/LoggableInterface.php (1)
1-36: LGTM!The LoggableInterface provides a clean abstraction for logging capability with fluent method chaining support. The interface is well-documented and follows good design principles.
.vortex/installer/playground/task-streaming.php (1)
1-196: LGTM!The playground script provides excellent demonstration coverage of Task streaming mode with diverse scenarios including output methods (echo vs Tui::output()), mixed content, failure handling, nested spinners, and styled output. The script is well-organized with clear section markers and appropriate delays for visual demonstration.
.vortex/installer/tests/Unit/Task/TaskOutputTest.php (3)
1-28: LGTM!Test file setup and constructor test are well-structured. The
#[CoversClass]attribute correctly targets theTaskOutputclass, and the constructor test validates proper instantiation.
30-83: LGTM!The write/writeln tests appropriately verify that messages are passed through to the wrapped output. The assertion approach (checking content presence rather than exact ANSI codes) is pragmatic since
BufferedOutputbehavior with ANSI codes depends on decoration settings.
85-127: LGTM!The delegation tests comprehensively verify that
TaskOutputcorrectly forwards verbosity, decoration, and formatter operations to the wrapped output. The verbosity test is particularly thorough, validating all related accessor methods at DEBUG level..vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (5)
1-48: LGTM!The test setup and
testRunmethod are well-structured. The data-provider approach allows comprehensive testing of various command scenarios including success cases, command not found, and invalid characters.
81-143: LGTM!The
testResolveCommand,testPrepareArguments, andtestValidateEnvironmentVarstests are well-structured with proper exception handling and comprehensive data providers covering success and error paths.
144-204: LGTM!The environment variable, working directory, and relative path tests are well-implemented. The Windows skip for
printenvis appropriate, and the relative path test properly sets up an executable script for verification.
225-363: LGTM!The data providers offer comprehensive test coverage for various scenarios including edge cases like invalid commands, numeric argument conversion, and boolean flag handling.
367-393: LGTM!The
TestableProcessRunnerclass follows a standard pattern for exposing protected methods in unit tests. The wrapper methods correctly delegate to the parent class..vortex/installer/playground/task.php (3)
1-35: LGTM!Good playground setup with proper autoloading and Tui initialization. The basic task demonstration clearly shows the minimal usage pattern.
36-172: LGTM!Comprehensive demonstration of Task::action capabilities including return value handling, static vs dynamic messages, hints, failure paths, array sublists, and closure-based labels. The sequential task loop is a good practical example.
173-268: LGTM!The streaming mode demonstrations effectively illustrate the difference from spinner mode, showing real-time output during action execution. The simulated build output scenario is particularly useful for understanding the intended use case for the auto-build feature.
.vortex/installer/src/Utils/Tui.php (4)
10-12: LGTM!Import additions for
confirmandinfofunctions from Laravel Prompts are correctly placed alongside existing imports.
47-50: LGTM!The
setOutputmethod correctly synchronizes both the internal output reference and Laravel Prompts' output, enabling output switching during streaming scenarios.
60-78: LGTM!The
successmethod provides semantic clarity, and theconfirmmethod correctly handles non-interactive mode by returning the default value. The null coalescing for the hint parameter properly satisfies Laravel Prompts' expected signature.
112-116: LGTM!The dim enhancement correctly maintains dim styling through embedded ANSI reset codes. By replacing
\033[0mwith\033[0m\033[2m, any color-styled text within dimmed content will re-apply dim after its color reset, ensuring consistent visual appearance in streaming output..vortex/installer/src/Logger/LoggerInterface.php (1)
1-62: LGTM!The
LoggerInterfaceis well-designed with a clear lifecycle (open/write/close) and state management (enable/disable/isEnabled). The fluent interface pattern for enable/disable methods facilitates method chaining, and the PHPDoc annotations are comprehensive. The interface aligns well with theFileLoggerimplementation shown in the relevant code snippets..vortex/installer/playground/tui.php (1)
1-187: Playground script looks self-contained and safe to ship.The Tui demo covers colors, styles, layout, and terminal width cleanly, with no side effects beyond console output. Good to have as a manual visual aid.
.vortex/installer/tests/Unit/TuiTest.php (1)
912-990: New Tui tests nicely cover the added API surface.The added tests for
setOutput(),success(),line()(with and without padding), and non‑interactiveconfirm()/getChar()provide good coverage of the new behavior and static state, while keeping dependencies limited toBufferedOutput. No changes needed here..vortex/installer/src/Runner/RunnerInterface.php (1)
10-87: RunnerInterface contract is clear and matches existing runners.The interface cleanly captures the runner responsibilities (command/exit code/output/cwd plus logging) and appears consistent with
AbstractRunner,CommandRunner, andProcessRunner. The exit‑code constants and typedgetOutput()signature are especially helpful for consumers and static analysis..vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
1-635: Excellent test coverage for AbstractRunner.This test file provides comprehensive coverage of the
AbstractRunnerbase class through a well-designedConcreteRunnertest double. The data providers are thorough, testing edge cases for command parsing, quoting, and formatting. The public wrapper methods enable proper testing of protected functionality without compromising encapsulation in production code..vortex/installer/src/Runner/AbstractRunner.php (4)
119-132: Logging toggle helpers are clear and chainable.
enableLog()/disableLog()are thin, readable wrappers over the logger that preserve fluent chaining. No issues from a correctness or design standpoint.
140-158: Streaming toggle API is straightforward.
enableStreaming()/disableStreaming()keep the intent ofshouldStreamobvious and support fluent chaining; this is a clean way to centralize the flag for concrete runners to honor.
184-196: Clarify whethergetOutput()’s$linesparameter should return the first or last N lines.Currently:
$output_lines = explode(PHP_EOL, $this->output); if ($lines !== NULL) { $output_lines = array_slice($output_lines, 0, $lines); }This returns the first
$lineslines. For log-style output, callers might reasonably expect the last N lines (tail behavior) instead.If the intended behavior is “last N lines”, you probably want:
- if ($lines !== NULL) { - $output_lines = array_slice($output_lines, 0, $lines); - } + if ($lines !== NULL) { + $output_lines = $lines >= 0 + ? array_slice($output_lines, -$lines) + : array_slice($output_lines, 0, $lines); + }If “first N lines” is intentional, consider documenting that explicitly in the phpdoc to avoid confusion for future callers.
201-205: Run-state reset is consistent and safe.
reset()clearscommand/outputand normalizesexitCodeback toEXIT_SUCCESS, which is the expected baseline before a new run; this keeps runner instances reusable without hidden state leaks..vortex/installer/tests/Helpers/TuiOutput.php (3)
7-113: Centralizing installer/build output strings in one helper is a solid choice.Collecting all of these messages as constants on
TuiOutputmakes the functional tests much easier to read and maintain, and it aligns well with the installer’s success/failure summary and requirements-checking flows described in the PR objective. No issues spotted with these definitions.
122-137:present()/absent()helpers align with the*/!test-output convention.Wrapping constants with
'* 'and'! 'viapresent()andabsent()nicely encodes the “must be present” / “must be absent” semantics used by the existing cmd/cmdFail test helpers, keeping tests declarative and DRY. Implementation is minimal and correct. Based on learnings, this matches the established PHPUnit pattern in this repo.
145-149: Method nameechoconflicts with PHP’s reserved keyword and will not parse.Declaring
public static function echo(...)is a syntax error in PHP, asechois a language construct/token, not a valid identifier. This will prevent the file from compiling.Rename the method and update call sites accordingly, for example:
- /** - * Echo constants as output lines. - * - * @param array<string> $constants - * Array of constant values to echo. - */ - public static function echo(array $constants): void { + /** + * Echo constants as output lines. + * + * @param array<string> $constants + * Array of constant values to output. + */ + public static function echoLines(array $constants): void { foreach ($constants as $constant) { echo $constant . PHP_EOL; } }And within this class:
- self::echo([ + self::echoLines([ self::BUILD_ASSEMBLE_DOCKER, ...Apply the same rename at all external call sites (
TuiOutput::echo(...)) in the test suite.Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2134 +/- ##
===========================================
+ Coverage 70.90% 76.35% +5.44%
===========================================
Files 99 109 +10
Lines 5077 5675 +598
Branches 44 44
===========================================
+ Hits 3600 4333 +733
+ Misses 1477 1342 -135 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
3fc9658 to
7b80ed6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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)
381-425: Critical bug:passthru()check does not detect curl command failuresThe code at line 420 checks
if (passthru($command) === FALSE)but this only catches PHP execution errors (when PHP cannot run the command). It does not detect curl failures. According to PHP semantics,passthru()returnsNULLon successful function invocation regardless of the command's exit code—the curl command's exit status is not captured. A curl command that fails (HTTP error, timeout, connection failure) will still returnNULLfrompassthru(), so the exception is never thrown. The code should pass the optional$result_codeparameter to capture the curl exit status:if (passthru($command, $result_code) === FALSE || $result_code !== 0) { throw new \RuntimeException(sprintf('Unable to download demo database from %s.', $url)); }
♻️ Duplicate comments (12)
.vortex/installer/src/Task/TaskOutput.php (1)
31-36: Iterable handling breaks for non-array Traversables.The cast
(array) $messagesdoes not iterate genericTraversableinstances; it instead casts object properties. Use explicit handling to support all iterables:- $dimmed = is_iterable($messages) - ? array_map(fn(string $m): string => Tui::dim($m), (array) $messages) - : Tui::dim($messages); + if (is_iterable($messages)) { + $messages = is_array($messages) ? $messages : iterator_to_array($messages, FALSE); + $dimmed = array_map(fn(string $m): string => Tui::dim($m), $messages); + } + else { + $dimmed = Tui::dim($messages); + }Same issue exists in
writeln()at lines 47-51..vortex/installer/src/Task/Task.php (1)
33-51: Ensure streaming mode always restores Tui output and output bufferIn streaming mode, if the
$actionthrows,ob_end_clean()andTui::setOutput($original_output)are never executed. That leaves output buffering active and Tui permanently bound toTaskOutput, which can break all subsequent console output in the installer.Wrap the action invocation in a
try/finallyso output and buffering are always restored:- if ($streaming) { - $original_output = Tui::output(); - - $task_output = new TaskOutput($original_output); - - static::start($label); - - Tui::setOutput($task_output); - - ob_start(function (string|iterable $buffer) use ($task_output): string { - $task_output->write($buffer); - return ''; - }, 1); - - $return = $action(); - - ob_end_clean(); - Tui::setOutput($original_output); - } + if ($streaming) { + $original_output = Tui::output(); + $task_output = new TaskOutput($original_output); + + static::start($label); + Tui::setOutput($task_output); + + ob_start(function (string $buffer) use ($task_output): string { + $task_output->write($buffer); + return ''; + }, 1); + + try { + $return = $action(); + } + finally { + ob_end_clean(); + Tui::setOutput($original_output); + } + }This also narrows the callback parameter type to
stringwhich matches PHP’s output buffering callback signature.PHP ob_start callback signature and buffer parameter type.vortex/installer/src/Logger/FileLogger.php (3)
44-61: Handle fopen() failures to avoid invalid stream resource usageIf
fopen()fails,$this->handlebecomesFALSE, butwrite()only checks for!== NULL, so subsequent writes will callfwrite(FALSE, ...), which is invalid and can raise runtime errors.Normalize failures by resetting the handle to
NULLand returning a boolean success flag:- // @phpstan-ignore-next-line - $this->handle = fopen($this->path, 'w'); - - return $this->handle !== FALSE; + // @phpstan-ignore-next-line + $this->handle = fopen($this->path, 'w'); + + if ($this->handle === FALSE) { + $this->handle = NULL; + return FALSE; + } + + return TRUE;This ensures
write()’s!== NULLguard remains safe.PHP fwrite behavior when given a non-resource (e.g. FALSE) as first argument
66-70: Optional: surface write failures for robustness
write()ignores the return value offwrite(), so disk-full or permission errors are silently swallowed. That may be acceptable for “best-effort” logging, but if logs are important for troubleshooting installer auto-builds, consider checking write success and reacting (logging to stderr or throwing) on failure.PHP fwrite return value semantics on partial/failed writes
126-132: Guard against getcwd() returning false
getDir()castsgetcwd()to string; ifgetcwd()returnsFALSE(e.g., cwd deleted),$this->dirbecomes an empty string, leading to odd log paths like/.logs/....Use a safe fallback or throw:
- if ($this->dir === '') { - $this->dir = (string) getcwd(); - } + if ($this->dir === '') { + $cwd = getcwd(); + if ($cwd === FALSE || $cwd === '') { + throw new \RuntimeException('Unable to determine current working directory for logger.'); + } + $this->dir = $cwd; + }This guarantees a meaningful base directory.
PHP getcwd error conditions and return value when directory is unavailable.vortex/installer/src/Runner/ProcessRunner.php (3)
48-55: Accumulate process output instead of overwriting per chunkInside the process callback,
$this->outputis overwritten on each buffer, so only the last chunk is preserved. For long-running or chatty commands (likeahoy build), this loses most of the log.Append instead:
- $process->run(function ($type, string|iterable $buffer) use ($logger, $output): void { - $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; - $this->output = $buffer; + $process->run(function ($type, string|iterable $buffer) use ($logger, $output): void { + $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; + $this->output .= $buffer; if ($this->shouldStream) { $output->write($buffer); } $logger->write($buffer); });This preserves the full process output for post-run inspection and summaries.
Symfony Process run() callback buffer semantics and chunking behavior
59-73: Fix exit-code mapping: don’t overwrite mapped values with raw codesThe
matchblock maps raw$exit_codeto internal constants (e.g.,EXIT_COMMAND_NOT_FOUND,EXIT_INVALID), but$this->exitCode = $exit_code;immediately overwrites that mapping, making the constants ineffective.Remove the overwrite:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - - $this->exitCode = $exit_code;Callers can still inspect the raw code from the underlying Process if needed by exposing it separately, but
getExitCode()should consistently return the mapped constants.Symfony Console Command constants vs generic exit codes (0,1,127, others)
89-120: Validate path-based commands are executable before runningFor commands containing
'/',$resolvedcan remain the original path even when the file does not exist or is not executable. That contradicts the docblock (“validating it exists”) and defers failure until process execution with a less clear error.Add an explicit
is_executable()check:- // If command is a path (contains /), check if it exists directly. - if (str_contains((string) $base_command, '/')) { - $resolved = $base_command; - // Check relative to cwd if not absolute. - if (!str_starts_with((string) $base_command, '/')) { - $full_path = $this->getCwd() . '/' . $base_command; - if (is_executable($full_path)) { - $resolved = $full_path; - } - } - } + // If command is a path (contains /), resolve and validate executability. + if (str_contains((string) $base_command, '/')) { + $resolved = $base_command; + // Check relative to cwd if not absolute. + if (!str_starts_with((string) $base_command, '/')) { + $full_path = $this->getCwd() . '/' . $base_command; + if (is_executable($full_path)) { + $resolved = $full_path; + } + } + + if (!is_executable($resolved)) { + throw new \InvalidArgumentException(sprintf('Command not found or not executable: %s', $base_command)); + } + }This gives users immediate, clear feedback when a path command is invalid.
PHP is_executable behavior for absolute and relative paths.vortex/installer/src/Runner/CommandRunner.php (1)
59-67: Preserve mapped exit-code constants instead of raw codesAs in
ProcessRunner, thematchcorrectly maps$exit_codeto internalEXIT_*constants, but$this->exitCode = $exit_code;then overwrites that mapping. Callers expectingRunnerInterface::EXIT_SUCCESS/EXIT_FAILURE/EXIT_COMMAND_NOT_FOUNDwill instead see raw values.Remove the overwrite:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - - $this->exitCode = $exit_code; $this->output = $buffered_output->fetch();This keeps the runner semantics consistent across implementations.
Symfony Console Application::run exit code mapping best practices.vortex/installer/src/Command/InstallCommand.php (3)
193-208: Treat failed auto-build as installer failure and surface guidance
Task::action(... streaming: TRUE)for “Building site” has its return value ignored, andexecute()always returnsCommand::SUCCESSafterwards. That means a failed build still yields a successful installer exit code and no explicit guidance, which conflicts with the PR objectives for clear success/failure summaries.Capture the Task::action result and handle failures explicitly, e.g.:
- if ($should_build) { - Task::action( - label: 'Building site', - action: fn(): bool => $this->runBuildCommand($output), - streaming: TRUE, - ); - } + if ($should_build) { + $build_success = Task::action( + label: 'Building site', + action: fn(): bool => $this->runBuildCommand($output), + streaming: TRUE, + ); + + if ($build_success !== TRUE) { + // Summarize failure and next steps; adjust wording as needed. + Tui::error('Auto-build failed. Retry with: ahoy build. See installer-build.log for details.'); + return Command::FAILURE; + } + }This ensures a non-zero exit code and an actionable message when the build fails.
Symfony Console best practices for propagating sub-command failures in installers
193-208: Always skip auto-build in CI and print a brief noteThe PR objectives state auto-build must always be skipped in CI with a printed note. Currently no CI detection is performed, so
--buildor an affirmative prompt can still trigger a build in CI.Add a CI guard before prompting:
- $should_build = $this->config->get(Config::BUILD_NOW); - if (!$should_build && !$this->config->getNoInteraction()) { + $should_build = $this->config->get(Config::BUILD_NOW); + + // Skip auto-build in common CI environments. + if (Env::get('CI') || Env::get('GITHUB_ACTIONS') || Env::get('GITLAB_CI') || Env::get('JENKINS_URL')) { + if ($should_build) { + Tui::info('Skipping auto-build in CI environment. Run manually with: ahoy build'); + } + $should_build = FALSE; + } + + if (!$should_build && !$this->config->getNoInteraction()) { $should_build = Tui::confirm( label: 'Run the site build now?', - default: TRUE, + default: FALSE, hint: 'Takes ~5-10 min; output will be streamed. You can skip and run later with: ahoy build', ); }This respects both CLI flags and non-interactive mode while enforcing the CI rule.
Common environment variables used to detect CI environments (CI, GITHUB_ACTIONS, etc.)
428-446: Align build exit-code checks with RunnerInterface constants
runBuildCommand()currently tests$runner->getExitCode() === Command::SUCCESS. With the runner implementations intended to exposeRunnerInterface::EXIT_*codes, it would be clearer and more consistent to check againstRunnerInterface::EXIT_SUCCESS:- $runner = $this->getRunner(); - $runner->run('build', args: $is_profile ? ['--profile' => '1'] : [], output: $output); - - return $runner->getExitCode() === Command::SUCCESS; + $runner = $this->getRunner(); + $runner->run('build', args: $is_profile ? ['--profile' => '1'] : [], output: $output); + + return $runner->getExitCode() === RunnerInterface::EXIT_SUCCESS;Once the exit-code mapping bugs in the runners are fixed, this will give a single, consistent success check across the codebase.
Symfony Console Command::SUCCESS vs custom runner exit-code constants usage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
.vortex/installer/installer.php(1 hunks).vortex/installer/phpstan.neon(1 hunks).vortex/installer/playground/task-streaming.php(1 hunks).vortex/installer/playground/task.php(1 hunks).vortex/installer/playground/tui.php(1 hunks).vortex/installer/src/Command/BuildCommand.php(1 hunks).vortex/installer/src/Command/CheckRequirementsCommand.php(1 hunks).vortex/installer/src/Command/InstallCommand.php(12 hunks).vortex/installer/src/Logger/FileLogger.php(1 hunks).vortex/installer/src/Logger/FileLoggerInterface.php(1 hunks).vortex/installer/src/Logger/LoggableInterface.php(1 hunks).vortex/installer/src/Logger/LoggerInterface.php(1 hunks).vortex/installer/src/Runner/AbstractRunner.php(1 hunks).vortex/installer/src/Runner/CommandRunner.php(1 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Runner/RunnerInterface.php(1 hunks).vortex/installer/src/Task/Task.php(5 hunks).vortex/installer/src/Task/TaskOutput.php(1 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Tui.php(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Logger/FileLoggerInterface.php.vortex/installer/src/Logger/FileLogger.php
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
.vortex/installer/src/Command/InstallCommand.php
🧬 Code graph analysis (10)
.vortex/installer/src/Runner/CommandRunner.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (5)
AbstractRunner(15-405)reset(201-205)buildCommandString(324-339)initLogger(73-79)resolveOutput(90-92)
.vortex/installer/playground/tui.php (1)
.vortex/installer/src/Utils/Tui.php (19)
Tui(17-284)output(40-45)init(25-38)dim(112-116)bold(104-106)green(84-86)blue(88-90)purple(92-94)yellow(96-98)cyan(100-102)underscore(108-110)box(197-216)info(52-54)note(56-58)success(60-62)error(64-66)list(164-195)terminalWidth(247-249)center(218-245)
.vortex/installer/src/Command/BuildCommand.php (5)
.vortex/installer/src/Runner/CommandRunner.php (2)
CommandRunner(17-146)run(34-72).vortex/installer/src/Runner/ProcessRunner.php (2)
ProcessRunner(15-164)run(20-75).vortex/installer/src/Runner/AbstractRunner.php (5)
getExitCode(173-179)setCwd(97-102)getLogger(52-58)getCommand(163-165)getOutput(184-196).vortex/installer/src/Logger/FileLogger.php (1)
getPath(85-87).vortex/installer/src/Logger/FileLoggerInterface.php (1)
getPath(18-18)
.vortex/installer/src/Logger/LoggableInterface.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (3)
enableLog(119-123)disableLog(128-132)getLogger(52-58)
.vortex/installer/playground/task-streaming.php (4)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Utils/Tui.php (9)
Tui(17-284)output(40-45)init(25-38)success(60-62)green(84-86)blue(88-90)yellow(96-98)underscore(108-110)bold(104-106).vortex/installer/src/Runner/CommandRunner.php (1)
writeln(128-135).vortex/installer/src/Task/TaskOutput.php (1)
writeln(46-51)
.vortex/installer/src/Logger/FileLoggerInterface.php (1)
.vortex/installer/src/Logger/FileLogger.php (3)
getPath(85-87)setDir(117-121)getDir(126-132)
.vortex/installer/src/Command/CheckRequirementsCommand.php (4)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Runner/RunnerInterface.php (3)
run(41-41)getExitCode(54-54)getOutput(67-67).vortex/installer/src/Runner/AbstractRunner.php (3)
getExitCode(173-179)getOutput(184-196)disableLog(128-132).vortex/installer/src/Logger/LoggableInterface.php (1)
disableLog(26-26)
.vortex/installer/installer.php (3)
.vortex/installer/src/Command/BuildCommand.php (1)
BuildCommand(20-213).vortex/installer/src/Command/CheckRequirementsCommand.php (1)
CheckRequirementsCommand(19-355).vortex/installer/src/Command/InstallCommand.php (1)
InstallCommand(32-697)
.vortex/installer/src/Logger/FileLogger.php (2)
.vortex/installer/src/Logger/LoggerInterface.php (6)
open(23-23)write(31-31)close(36-36)isEnabled(44-44)enable(52-52)disable(60-60).vortex/installer/src/Logger/FileLoggerInterface.php (3)
getDir(37-37)getPath(18-18)setDir(29-29)
.vortex/installer/playground/task.php (2)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Utils/Tui.php (4)
Tui(17-284)output(40-45)init(25-38)success(60-62)
⏰ 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). (6)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-docs
🔇 Additional comments (15)
.vortex/installer/phpstan.neon (1)
19-20: Pre-existing configuration, not a change in this PR.The
treatPhpDocTypesAsCertain: falsesetting at lines 19–20 of.vortex/installer/phpstan.neonalready existed before this PR. Git history confirms it was not added in commit7b80ed6b("[#1950] Added auto-build to installer."). The phpstan.neon file was not modified in this commit—only new PHP source files (BuildCommand, Logger, Task, etc.) were added.Since this configuration was pre-existing, it falls outside the scope of reviewing changes introduced by this specific PR.
Likely an incorrect or invalid review comment.
.vortex/installer/installer.php (1)
11-24: LGTM!The addition of
BuildCommandandCheckRequirementsCommandfollows the established pattern, and using a string name forsetDefaultCommand()is more flexible than binding to a specific instance..vortex/installer/src/Utils/Config.php (1)
42-42: LGTM!The
BUILD_NOWconstant follows the established naming pattern and integrates consistently with the existing configuration surface..vortex/installer/src/Logger/FileLoggerInterface.php (1)
1-39: LGTM!The interface is well-designed with clear method signatures that align with the concrete
FileLoggerimplementation. PHPDoc is thorough and return types are appropriate..vortex/installer/playground/task-streaming.php (1)
1-196: LGTM!This playground script provides comprehensive demonstration of Task streaming functionality across multiple scenarios (echo, Tui output, mixed, failures, nested spinners, styled output). The variety of test cases will be valuable for manual testing and documentation.
.vortex/installer/src/Logger/LoggableInterface.php (1)
1-36: LGTM!The interface establishes a clear contract for logging capability with fluent method chaining. The design integrates well with the broader logging system shown in
AbstractRunnerand related components..vortex/installer/playground/tui.php (1)
1-187: LGTM!This playground script provides excellent coverage of Tui helper functionality, including all color codes, text styles, combinations, and layout helpers. The organized structure makes it easy to verify visual rendering and serves as practical documentation.
.vortex/installer/src/Logger/LoggerInterface.php (1)
1-62: LGTM!The interface provides a clear contract for command execution logging with well-documented methods. The lifecycle methods (
open(),write(),close()) and state control methods (enable(),disable(),isEnabled()) are appropriately typed and support method chaining..vortex/installer/playground/task.php (1)
1-268: Playground coverage looks comprehensive and safeThe script exercises the various Task::action modes (basic, callbacks, arrays, closures, streaming) without side effects beyond console output. Autoload path and Tui initialization are consistent with the installer layout.
.vortex/installer/src/Runner/RunnerInterface.php (1)
10-87: RunnerInterface contract is clear and consistent with Symfony Console standardsExit-code constants (0, 1, 2, 127), cwd management, and output retrieval semantics are well-documented. The documented
int<0, 255>return type forgetExitCode()aligns with standard 8-bit OS exit code range. Symfony Console integration viaOutputInterfaceparameter is correct..vortex/installer/src/Runner/CommandRunner.php (1)
85-144: Composite output implementation is correct—fetch() delegation properly retrieves all buffered outputThe anonymous class extends
BufferedOutputpurely for type compatibility but intentionally never populates the parent's buffer. Allwrite()andwriteln()calls explicitly bypass the parent implementation (noparent::calls) and route directly to$this->bufferedOutput. Thefetch()method correctly returns all accumulated output via$this->bufferedOutput->fetch(), which Symfony's implementation clears after returning. This design avoids dual-buffer confusion and ensures the multiplexing logic (buffering + optional streaming + logging) captures everything exactly once..vortex/installer/src/Command/InstallCommand.php (2)
193-200: Now I need to verify the Laravel Promptsconfirm()function behavior to ensure the fix is correct:
<function_calls>
Laravel Prompts confirm default true false behavior
</function_calls>
217-242: Review comment verified and approved — POSIXcommand -vapproach is portable and correctly implementedThe web search and codebase examination confirm the review's approval:
POSIX
command -vis portable: Returns 0 on success, non-zero (127 for not found) on failure across bash, dash, ksh, and zsh.Implementation is correct: The
checkRequirements()method uses the standard pattern — redirect to/dev/null, check exit code againstRunnerInterface::EXIT_SUCCESS(0). This is exactly the POSIX-recommended approach.Tests validate the behavior: The comprehensive functional test suite confirms that exit codes are correctly mapped and checked, with test cases for each missing tool (git, curl, tar, composer) returning
RunnerInterface::EXIT_COMMAND_NOT_FOUNDto simulate failures.@phpstan-ignore comments explained: These appear because
ProcessRunner.run()has a logic artifact (line 72 overwrites the constant mapping from lines 65–70), but the actual behavior remains correct since raw exit codes (0 and non-zero) match the boolean semantics needed.The code appropriately uses
ProcessRunnerandRunnerInterface::EXIT_SUCCESSto assert tool availability with clear error messages, following POSIX conventions reliably across shells..vortex/installer/src/Task/Task.php (1)
58-68: Streaming vs non-streaming result handling is coherentThe code correctly implements consistent success/failure logic for both modes. Streaming mode uses
start()to display the label before execution and captures output withob_start(). Non-streaming mode usesspin()to show a spinner during execution, then callslabel()afterward. Both paths share identical result handling at lines 58-68, checking if$return === FALSEand applying the same success/failure callbacks and messages. The return value is consistent across both flows, providing a coherent contract to callers..vortex/installer/src/Utils/Tui.php (1)
10-15: Code implementation verified and accurateAll claims in the review have been confirmed through code examination and web search:
setOutput() sync — Correctly calls
Prompt::setOutput()to keep Laravel Prompts' output stream in sync. The method is used only for temporary output redirection (lines 40, 50 in Task.php), not for changing interactivity, so omittingPrompt::interactive()updates is correct behavior.success() delegation — Properly delegates to Laravel Prompts
info()function (imported line 12), notTui::info().confirm() wrapper — Correctly implements non-interactive short-circuit (lines 69–71) returning
$default, otherwise calls Laravelconfirm()with proper parameters. Web search confirmed Laravel Promptsconfirm()respects non-interactive mode by returning defaults.dim() ANSI handling — Correctly replaces reset codes (
\033[0m) with reset+dim (\033[0m\033[2m) before applying dim style, maintaining the effect through color resets.Imports — All Laravel Prompts function imports (confirm, error, info, intro, note, table) are present and correct.
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
7b80ed6 to
431cb17
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
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/Utils/Tui.php (1)
47-78: NewsetOutput,success, andconfirmhelpers align with TUI usage
setOutput()correctly updates bothstatic::$outputandPrompt::setOutput(), which is what streaming needs when swapping toTaskOutput, without disturbing the interactivity flag set byinit().success()currently calls the imported Laravelinfo()function rather thanTui::info(). If that visual distinction (intro vs info style) is intentional, this is fine; otherwise, considerself::info($message)for consistency.confirm()’s non-interactive shortcut to$defaultis a good fit for automated runs; if you later want auditability, you might add a separate log/note around auto-accepting defaults at the call sites.
♻️ Duplicate comments (22)
.vortex/installer/src/Task/TaskOutput.php (1)
31-36: Handle generic iterables safely instead of casting with(array)in write()/writeln().Using
(array) $messagesfor iterables will misbehave for genericTraversableinstances (casting object properties instead of iterating values). This can break if Symfony ever passes a non-array iterable.You can normalize iterables explicitly before dimming:
- public function write(string | iterable $messages, bool $newline = FALSE, int $options = 0): void { - $dimmed = is_iterable($messages) - ? array_map(fn(string $m): string => Tui::dim($m), (array) $messages) - : Tui::dim($messages); - $this->wrapped->write($dimmed, $newline, $options); - } + public function write(string | iterable $messages, bool $newline = FALSE, int $options = 0): void { + if (is_iterable($messages)) { + $messages_array = is_array($messages) ? $messages : iterator_to_array($messages, FALSE); + $dimmed = array_map(fn(string $m): string => Tui::dim($m), $messages_array); + } + else { + $dimmed = Tui::dim($messages); + } + + $this->wrapped->write($dimmed, $newline, $options); + } @@ - public function writeln(string | iterable $messages, int $options = 0): void { - $dimmed = is_iterable($messages) - ? array_map(fn(string $m): string => Tui::dim($m), (array) $messages) - : Tui::dim($messages); - $this->wrapped->writeln($dimmed, $options); - } + public function writeln(string | iterable $messages, int $options = 0): void { + if (is_iterable($messages)) { + $messages_array = is_array($messages) ? $messages : iterator_to_array($messages, FALSE); + $dimmed = array_map(fn(string $m): string => Tui::dim($m), $messages_array); + } + else { + $dimmed = Tui::dim($messages); + } + + $this->wrapped->writeln($dimmed, $options); + }Also applies to: 47-51
.vortex/installer/src/Command/CheckRequirementsCommand.php (1)
269-292: Correct Pygmy detection: avoid shell pipeline and clarify install vs “up” guidance.Two issues in
checkPygmy():
Missing Pygmy vs not running:
- When
pygmyis not installed (commandExists('pygmy')is false), the message"Run: pygmy up"is misleading; the user must install Pygmy first.- Suggest using an install URL when the command is missing, and reserving
"Run: pygmy up"for the “installed but not running” case.Shell pipeline will not behave as intended with
ProcessRunner:
ProcessRunner::run()constructsnew Process($cmd, ...)from a parsed array, so a command string like'docker ps --format "{{.Names}}" | grep -q amazeeio'is not executed via a shell. The| grep -q amazeeiopart becomes extra arguments todocker, so the grep is never executed and exit codes no longer reflect whether anyamazeeiocontainers are running.- This can incorrectly mark Pygmy as “running” whenever
docker psitself succeeds.A more robust implementation would be:
protected function checkPygmy(): bool { if (!$this->commandExists('pygmy')) { - $this->missing['Pygmy'] = 'Run: pygmy up'; + $this->missing['Pygmy'] = 'https://github.com/pygmystack/pygmy'; return FALSE; } $version = $this->getCommandVersion('pygmy version'); $this->runner->run('pygmy status'); if ($this->runner->getExitCode() === RunnerInterface::EXIT_SUCCESS) { $this->present['Pygmy'] = $version; return TRUE; } - $this->runner->run('docker ps --format "{{.Names}}" | grep -q amazeeio'); - // @phpstan-ignore-next-line notIdentical.alwaysFalse - if ($this->runner->getExitCode() === RunnerInterface::EXIT_SUCCESS) { - $this->present['Pygmy'] = $version; - return TRUE; - } - - $this->missing['Pygmy'] = 'Run: pygmy up'; + // Fallback: detect Pygmy containers directly via docker. + $this->runner->run('docker ps --format "{{.Names}}"'); + $names = $this->runner->getOutput(as_array: TRUE); + if (is_array($names) && array_filter($names, static fn(string $name): bool => str_contains($name, 'amazeeio'))) { + $this->present['Pygmy'] = $version; + return TRUE; + } + + $this->missing['Pygmy'] = 'Run: pygmy up'; return FALSE; }This keeps behavior explicit and independent of shell pipelines while giving clearer instructions when Pygmy is absent vs just stopped.
.vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (1)
24-47: Strong ProcessRunner coverage; minor naming nit intestRun
testRun()exercises the main run/exit-code/output path well. For readability, consider renaming the second$output(from$runner->getOutput()) to something like$runnerOutputto avoid reusing the name of the earlierBufferedOutputvariable, mirroring the change you already made intestRunWithStreaming..vortex/installer/tests/Functional/Command/BuildCommandTest.php (1)
151-159: Complex Docker check condition could be simplified.This matches a previous review comment. The multiple
str_containschecks are fragile if command format changes..vortex/installer/src/Task/Task.php (1)
42-45: Incorrect type hint inob_startcallback.PHP's output buffering callback only receives a
stringparameter, notstring|iterable. This was flagged in a previous review..vortex/installer/src/Runner/ProcessRunner.php (3)
48-55: Critical: Output is overwritten instead of accumulated.Each callback invocation overwrites
$this->outputwith only the current buffer content, discarding all previously captured output. For multi-chunk process output, this will lose data.Apply this diff to accumulate output:
$process->run(function ($type, string|iterable $buffer) use ($logger, $output): void { $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; - $this->output = $buffer; + $this->output .= $buffer; if ($this->shouldStream) { $output->write($buffer); } $logger->write($buffer); });
65-72: Critical: Exit code mapping is immediately overwritten.The
matchstatement on lines 65-70 correctly maps the raw exit code to interface constants, but line 72 immediately overwrites$this->exitCodewith the raw$exit_code, completely negating the mapping logic.Apply this diff to fix the exit code handling:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - - $this->exitCode = $exit_code; return $this;
98-119: Path-based commands may not be validated for existence.When a command contains
/(is a path), the code only validates executability for relative paths that exist in cwd. For absolute paths or relative paths that don't resolve via cwd,$resolvedis set to$base_commandwithout verifying the file exists or is executable. This could lead to confusing errors at process execution time instead of a clear early failure.Consider adding validation:
// If command is a path (contains /), check if it exists directly. if (str_contains((string) $base_command, '/')) { $resolved = $base_command; // Check relative to cwd if not absolute. if (!str_starts_with((string) $base_command, '/')) { $full_path = $this->getCwd() . '/' . $base_command; if (is_executable($full_path)) { $resolved = $full_path; } } + // Verify the resolved path is executable. + if (!is_executable($resolved)) { + throw new \InvalidArgumentException(sprintf('Command not found or not executable: %s', $base_command)); + } }.vortex/installer/src/Logger/FileLogger.php (3)
57-60: Consider explicit resource handling on failure.The
open()method returnsFALSEwhenfopen()fails, but$this->handlewould beFALSEnotNULL. Subsequent calls towrite()would silently do nothing since the check is$this->handle !== NULL.Consider explicitly setting handle to NULL on failure for consistency:
// @phpstan-ignore-next-line $this->handle = fopen($this->path, 'w'); - return $this->handle !== FALSE; + if ($this->handle === FALSE) { + $this->handle = NULL; + return FALSE; + } + + return TRUE; }
126-132: Edge case:getcwd()can return false.
getcwd()returnsfalseif the current working directory has been deleted. The(string)cast on line 128 would convertfalseto an empty string"", which could lead to unexpected log file paths.public function getDir(): string { if ($this->dir === '') { - $this->dir = (string) getcwd(); + $cwd = getcwd(); + if ($cwd === FALSE) { + throw new \RuntimeException('Unable to determine current working directory'); + } + $this->dir = $cwd; } return $this->dir; }
66-70: Consider checking fwrite return value for robust error handling.The
fwrite()call doesn't check the return value. While this may not be critical for logging, checking for write errors could help detect disk full or permission issues early.public function write(string $content): void { if ($this->handle !== NULL) { - fwrite($this->handle, $content); + $written = fwrite($this->handle, $content); + if ($written === FALSE) { + // Optionally log or handle write error + } } }.vortex/installer/tests/Unit/Logger/FileLoggerTest.php (1)
138-143: Consider asserting the no-op case more explicitly.The test for writing when the logger is not open only asserts
TRUEas a placeholder. While the comment explains the intent, this doesn't actually verify the no-op behavior.Consider verifying that no file was created:
else { // When logger is not open, write() should be a no-op. - // We can't directly verify this, but we ensure no errors occur. - // @phpstan-ignore-next-line - $this->assertTrue(TRUE, 'write() should not throw error when logger is not open'); + // Verify no file was created by checking getPath() is still null. + $this->assertNull($logger->getPath(), 'No log file should be created when write() is called on unopened logger'); }.vortex/installer/src/Runner/CommandRunner.php (1)
59-67: Critical: Exit code mapping is immediately overwritten.The
matchexpression on lines 59-64 maps exit codes to internal constants, but line 66 immediately overwrites$this->exitCodewith the raw$exit_code. This negates the mapping logic entirely.Remove the overwrite:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - $this->exitCode = $exit_code; $this->output = $buffered_output->fetch();.vortex/installer/src/Command/InstallCommand.php (3)
193-200: Missing CI environment detection per PR objectives.The PR objectives specify: "always skip auto-build in CI with a printed note." Currently, there's no check for CI environment variables (e.g.,
CI,GITHUB_ACTIONS,GITLAB_CI,JENKINS_URL).Consider adding CI detection:
$should_build = $this->config->get(Config::BUILD_NOW); + + // Skip auto-build in CI environments. + if (Env::get('CI') || Env::get('GITHUB_ACTIONS') || Env::get('GITLAB_CI')) { + if ($should_build) { + Tui::info('Skipping auto-build in CI environment. Run manually with: ahoy build'); + } + $should_build = FALSE; + } + if (!$should_build && !$this->config->getNoInteraction()) {
448-457: Inconsistent exit code constant usage.
runBuildCommand()compares againstCommand::SUCCESSwhilecheckRequirements()usesRunnerInterface::EXIT_SUCCESS. For consistency and clarity, use the same constant throughout the file.- return $runner->getExitCode() === Command::SUCCESS; + return $runner->getExitCode() === RunnerInterface::EXIT_SUCCESS;
648-696: DI getters should cache instances using??=.The three lazy getters (
getRunner(),getProcessRunner(),getDownloader()) currently return new instances on every call when the backing property is null. While harmless today, caching with??=is the standard pattern for lazy initialization and prevents unnecessary object creation.protected function getRunner(): CommandRunner { - return $this->runner ?? new CommandRunner($this->getApplication()); + return $this->runner ??= new CommandRunner($this->getApplication()); } protected function getProcessRunner(): ProcessRunner { - return $this->processRunner ?? (new ProcessRunner())->disableLog()->disableStreaming(); + return $this->processRunner ??= (new ProcessRunner())->disableLog()->disableStreaming(); } protected function getDownloader(): Downloader { - return $this->downloader ?? new Downloader(); + return $this->downloader ??= new Downloader(); }.vortex/installer/tests/Helpers/TuiOutput.php (1)
160-160: Document or mark unused$commandparameters in factory closures.Each factory method closure accepts
string $commandfor interface compatibility withRunnerInterfacebut never uses it. This was previously noted in past reviews.As suggested previously, prefix the parameter with an underscore to indicate it's intentionally unused:
public static function buildRunnerSuccess(): \Closure { - return function (string $command): int { + return function (string $_command): int {Apply the same pattern to
buildRunnerSuccessProfile(),buildRunnerFailure(),checkRequirementsSuccess(), andcheckRequirementsFailure().Also applies to: 183-183, 206-206, 226-226, 251-251
.vortex/installer/src/Runner/AbstractRunner.php (3)
73-79: Logger open failures are still not handled.The
initLogger()method ignores the return value of$logger->open($command, $args), so failures to create the log file will be silent. This was previously noted in past reviews.If logging is important for troubleshooting, consider checking the result:
protected function initLogger(string $command, array $args = []): FileLoggerInterface { $logger = $this->getLogger(); $logger->setDir($this->getCwd()); - $logger->open($command, $args); + if (!$logger->open($command, $args)) { + throw new \RuntimeException(sprintf('Unable to open log file for command "%s".', $command)); + } return $logger; }
243-284: Consider removing unused$end_of_options_foundlogic.The
$end_of_options_foundflag is set when encountering--but doesn't change the behavior of the parser (both first and subsequent--tokens are added to$parts). This was previously noted in past reviews.Consider removing the flag or clarifying in the docblock that
--is simply preserved as a token:$parts = []; $current = ''; $in_quotes = FALSE; $quote_char = ''; $escaped = FALSE; $length = strlen($command); $has_content = FALSE; - $end_of_options_found = FALSE; for ($i = 0; $i < $length; $i++) { $char = $command[$i]; // ... existing quote/escape handling ... if (!$in_quotes && ($char === ' ' || $char === "\t")) { if ($current !== '' || $has_content) { - // Check for end-of-options marker (--) only if not already found - // and not inside quotes. - if (!$end_of_options_found && $current === '--') { - $end_of_options_found = TRUE; - // Add the -- marker to the parts array so it reaches the command. - $parts[] = $current; - $current = ''; - $has_content = FALSE; - continue; - } - $parts[] = $current;
380-407: Boolean formatting is as-designed but could be more explicit.The
formatArgs()method collapses booleanTRUEto'1'for positional arguments and dropsFALSEentirely. For named options,TRUEemits just the key andFALSEis dropped. This was previously noted in past reviews as potentially surprising.If callers may pass raw booleans and expect explicit
'true'/'false'strings, consider updating the formatting logic as suggested in previous reviews..vortex/installer/src/Command/BuildCommand.php (2)
142-146: Guard against null$this->runnerin summary methods.Both
showSuccessSummary()andshowFailureSummary()access$this->runner->getLogger()without checking if$this->runneris null. If the build task throws an exception before line 95, these methods will fail. This was previously noted in past reviews.Use null-safe operators:
protected function showSuccessSummary(): void { $output = ''; $title = 'Build completed successfully!'; $output .= 'Site URL: http://' . $this->getProjectMachineName() . '.docker.amazee.io' . PHP_EOL; $output .= 'Login: ahoy login' . PHP_EOL; $output .= PHP_EOL; - $log_path = $this->runner->getLogger()->getPath(); - if ($log_path) { + $log_path = $this->runner?->getLogger()->getPath(); + if ($log_path !== null) { $output .= 'Log file: ' . $log_path . PHP_EOL;Apply similar changes to
showFailureSummary()on lines 163-174.Also applies to: 163-174
179-183: Remove unreachable type check.The
is_array()check is unreachable becausegetOutput(as_array: TRUE)is guaranteed to return an array. This was previously noted in past reviews.// Show last 10 lines of output for context. - $runner_output = $this->runner->getOutput(as_array: TRUE); - - if (!is_array($runner_output)) { - throw new \RuntimeException('Runner output is not an array.'); - } - - $last_lines = array_slice($runner_output, -10); + /** @var array<string> $runner_output */ + $runner_output = $this->runner->getOutput(as_array: TRUE); + $last_lines = array_slice($runner_output, -10);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (33)
.vortex/installer/installer.php(1 hunks).vortex/installer/phpstan.neon(1 hunks).vortex/installer/playground/task-streaming.php(1 hunks).vortex/installer/playground/task.php(1 hunks).vortex/installer/playground/tui.php(1 hunks).vortex/installer/src/Command/BuildCommand.php(1 hunks).vortex/installer/src/Command/CheckRequirementsCommand.php(1 hunks).vortex/installer/src/Command/InstallCommand.php(12 hunks).vortex/installer/src/Logger/FileLogger.php(1 hunks).vortex/installer/src/Logger/FileLoggerInterface.php(1 hunks).vortex/installer/src/Logger/LoggableInterface.php(1 hunks).vortex/installer/src/Logger/LoggerInterface.php(1 hunks).vortex/installer/src/Runner/AbstractRunner.php(1 hunks).vortex/installer/src/Runner/CommandRunner.php(1 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Runner/RunnerInterface.php(1 hunks).vortex/installer/src/Task/Task.php(5 hunks).vortex/installer/src/Task/TaskOutput.php(1 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Tui.php(4 hunks).vortex/installer/tests/Functional/Command/BuildCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/InstallCommandTest.php(1 hunks).vortex/installer/tests/Helpers/TuiOutput.php(1 hunks).vortex/installer/tests/Traits/TuiTrait.php(1 hunks).vortex/installer/tests/Unit/ConfigTest.php(2 hunks).vortex/installer/tests/Unit/Logger/FileLoggerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/CommandRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Task/TaskOutputTest.php(1 hunks).vortex/installer/tests/Unit/TaskTest.php(2 hunks).vortex/installer/tests/Unit/TuiTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/Functional/Command/InstallCommandTest.php.vortex/installer/tests/Unit/TaskTest.php.vortex/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php.vortex/installer/tests/Unit/Logger/FileLoggerTest.php.vortex/installer/tests/Unit/TuiTest.php.vortex/installer/tests/Helpers/TuiOutput.php.vortex/installer/tests/Unit/Runner/CommandRunnerTest.php
📚 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/installer/tests/Functional/Command/InstallCommandTest.php
📚 Learning: 2025-06-02T00:41:36.321Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
Applied to files:
.vortex/installer/tests/Functional/Command/InstallCommandTest.php
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
.vortex/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/src/Command/InstallCommand.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/Logger/FileLogger.php.vortex/installer/tests/Unit/Logger/FileLoggerTest.php.vortex/installer/src/Logger/FileLoggerInterface.php
🧬 Code graph analysis (20)
.vortex/installer/src/Logger/LoggerInterface.php (3)
.vortex/installer/src/Logger/FileLogger.php (6)
open(44-61)write(66-70)close(75-80)isEnabled(92-94)enable(99-103)disable(108-112).vortex/installer/src/Runner/CommandRunner.php (1)
write(109-118).vortex/installer/src/Task/TaskOutput.php (1)
write(31-36)
.vortex/installer/installer.php (3)
.vortex/installer/src/Command/BuildCommand.php (1)
BuildCommand(20-213).vortex/installer/src/Command/CheckRequirementsCommand.php (1)
CheckRequirementsCommand(19-355).vortex/installer/src/Command/InstallCommand.php (1)
InstallCommand(32-708)
.vortex/installer/tests/Unit/ConfigTest.php (1)
.vortex/installer/src/Utils/Config.php (1)
Config(14-128)
.vortex/installer/tests/Unit/TaskTest.php (3)
.vortex/installer/src/Utils/Tui.php (4)
output(40-45)Tui(17-284)init(25-38)line(80-82).vortex/installer/src/Task/TaskOutput.php (2)
writeln(46-51)write(31-36).vortex/installer/src/Logger/FileLogger.php (1)
write(66-70)
.vortex/installer/tests/Functional/Command/BuildCommandTest.php (4)
.vortex/installer/src/Command/BuildCommand.php (2)
BuildCommand(20-213)setRunner(209-211).vortex/installer/src/Command/CheckRequirementsCommand.php (2)
CheckRequirementsCommand(19-355)setRunner(351-353).vortex/installer/src/Runner/ProcessRunner.php (1)
ProcessRunner(15-164).vortex/installer/tests/Helpers/TuiOutput.php (3)
TuiOutput(7-263)present(122-124)absent(135-137)
.vortex/installer/src/Task/TaskOutput.php (2)
.vortex/installer/src/Task/Task.php (1)
Task(11-107).vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)dim(112-116)
.vortex/installer/src/Utils/Tui.php (2)
.vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
setOutput(599-601).vortex/installer/src/Prompts/PromptManager.php (1)
label(471-479)
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (2)
.vortex/installer/src/Command/CheckRequirementsCommand.php (2)
CheckRequirementsCommand(19-355)setRunner(351-353).vortex/installer/tests/Helpers/TuiOutput.php (3)
TuiOutput(7-263)present(122-124)absent(135-137)
.vortex/installer/tests/Unit/Logger/FileLoggerTest.php (2)
.vortex/installer/src/Logger/LoggerInterface.php (6)
disable(60-60)isEnabled(44-44)enable(52-52)open(23-23)close(36-36)write(31-31).vortex/installer/src/Logger/FileLoggerInterface.php (3)
getDir(37-37)setDir(29-29)getPath(18-18)
.vortex/installer/src/Runner/ProcessRunner.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (9)
AbstractRunner(15-409)reset(205-209)buildCommandString(328-343)initLogger(73-79)resolveOutput(90-92)getCwd(107-118)getExitCode(177-183)parseCommand(230-310)formatArgs(380-407)
.vortex/installer/src/Runner/RunnerInterface.php (4)
.vortex/installer/src/Runner/CommandRunner.php (1)
run(34-72).vortex/installer/src/Runner/ProcessRunner.php (1)
run(20-75).vortex/installer/src/Utils/Tui.php (1)
output(40-45).vortex/installer/src/Runner/AbstractRunner.php (5)
getCommand(167-169)getExitCode(177-183)getOutput(188-200)setCwd(97-102)getCwd(107-118)
.vortex/installer/src/Logger/LoggableInterface.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (3)
enableLog(123-127)disableLog(132-136)getLogger(52-58)
.vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (3)
.vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
run(548-552).vortex/installer/src/Runner/AbstractRunner.php (5)
setCwd(97-102)getExitCode(177-183)getOutput(188-200)getCommand(167-169)disableStreaming(158-162).vortex/installer/src/Runner/RunnerInterface.php (5)
setCwd(78-78)run(41-41)getExitCode(54-54)getOutput(67-67)getCommand(46-46)
.vortex/installer/tests/Unit/TuiTest.php (1)
.vortex/installer/src/Utils/Tui.php (7)
Tui(17-284)init(25-38)output(40-45)setOutput(47-50)line(80-82)confirm(68-78)getChar(122-141)
.vortex/installer/src/Task/Task.php (3)
.vortex/installer/src/Utils/Strings.php (2)
Strings(7-192)isAsciiStart(9-11).vortex/installer/src/Utils/Tui.php (4)
Tui(17-284)output(40-45)setOutput(47-50)line(80-82).vortex/installer/src/Task/TaskOutput.php (2)
TaskOutput(14-123)write(31-36)
.vortex/installer/src/Logger/FileLoggerInterface.php (1)
.vortex/installer/src/Logger/FileLogger.php (3)
getPath(85-87)setDir(117-121)getDir(126-132)
.vortex/installer/src/Command/InstallCommand.php (3)
.vortex/installer/src/Runner/CommandRunner.php (2)
CommandRunner(17-146)run(34-72).vortex/installer/src/Task/Task.php (2)
Task(11-107)action(17-68).vortex/installer/src/Downloader/DownloaderInterface.php (1)
download(31-31)
.vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (3)
.vortex/installer/src/Logger/FileLogger.php (5)
FileLogger(12-164)getDir(126-132)disable(108-112)isEnabled(92-94)getPath(85-87).vortex/installer/src/Runner/AbstractRunner.php (4)
AbstractRunner(15-409)getLogger(52-58)initLogger(73-79)resolveOutput(90-92).vortex/installer/src/Logger/FileLoggerInterface.php (2)
getDir(37-37)getPath(18-18)
.vortex/installer/src/Command/CheckRequirementsCommand.php (6)
.vortex/installer/src/Runner/ProcessRunner.php (2)
ProcessRunner(15-164)run(20-75).vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Command/BuildCommand.php (1)
getRunner(199-202).vortex/installer/src/Runner/RunnerInterface.php (3)
run(41-41)getExitCode(54-54)getOutput(67-67).vortex/installer/src/Runner/AbstractRunner.php (4)
getExitCode(177-183)getOutput(188-200)disableLog(132-136)disableStreaming(158-162).vortex/installer/src/Logger/LoggableInterface.php (1)
disableLog(26-26)
.vortex/installer/src/Runner/AbstractRunner.php (3)
.vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)output(40-45).vortex/installer/src/Logger/FileLoggerInterface.php (1)
setDir(29-29).vortex/installer/src/Runner/RunnerInterface.php (5)
getCwd(86-86)setCwd(78-78)getCommand(46-46)getExitCode(54-54)getOutput(67-67)
⏰ 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). (6)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-docs
🔇 Additional comments (23)
.vortex/installer/phpstan.neon (1)
19-20: ThetreatPhpDocTypesAsCertain: falsesetting is justified by specific documented patterns below.The ignoreErrors section (lines 21–32) already documents why this setting is necessary: data providers and test methods cannot have parameter docblocks (PHPUnit uses attributes instead), and bound callbacks in specific handlers lack parameter documentation. These are not oversights but structural constraints of the codebase patterns.
Adding an inline comment at line 19 explaining this rationale would be helpful for clarity, but the configuration is intentional and not a concern requiring refactoring.
.vortex/installer/src/Logger/FileLoggerInterface.php (1)
10-37: FileLoggerInterface surface looks consistent and sufficientThe contract aligns with
FileLogger(path/dir accessors, fluentsetDir) and with runner usage such asAbstractRunner::setCwd()updating the logger directory. No issues from an API or typing perspective..vortex/installer/tests/Unit/TaskTest.php (1)
9-412: Streaming Task tests give solid coverage of the new behaviorThe additional streaming-focused tests (output via Tui, echo/print, mixed output, failures, no-output case, direct OutputInterface usage, and output restoration) are well-structured and match the intended Task API. The change to a
mixed-typed success callback removes the previous type-mismatch risk without weakening the assertions..vortex/installer/playground/task-streaming.php (1)
1-193: Task streaming playground is coherent and non-invasiveThe playground cleanly exercises the new streaming paths (echo, Tui::output(), nested spinners, styles, post-failure and post-streaming behavior) without impacting production code. The autoload path and Tui initialization are consistent with the installer layout.
.vortex/installer/src/Logger/LoggableInterface.php (1)
10-34: LoggableInterface cleanly captures the logging contractThe enable/disable +
getLogger(): LoggerInterfacetrio matches howAbstractRunnerimplements logging and keeps the interface narrowly focused. Typing and chaining semantics look correct..vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (1)
50-223: Streaming, command parsing, and env-var tests align with ProcessRunner’s contractThe streaming tests correctly distinguish between Tui-streamed output and always-captured runner output; the resolve/prepare/validate helpers are well-covered via
TestableProcessRunner, including edge cases like invalid commands, boolean args, and non-scalar env vars. The OS-specific skip aroundprintenvkeeps the suite portable enough for non-Windows CI..vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (1)
25-393: Comprehensive functional coverage for CheckRequirementsCommandThe mocked-runner + data-provider approach thoroughly exercises the requirements matrix (all/only subsets, missing tools, modern vs legacy docker-compose, Pygmy edge cases, and invalid
--onlyvalues). UsingTuiOutput::present/absentwith*/!markers keeps the expectations readable and decoupled from exact phrasing, which matches the established test conventions. Based on learnings, this is consistent with prior TUI assertion patterns..vortex/installer/src/Utils/Tui.php (1)
112-116: Updateddim()behavior better preserves styling through resetsThe additional replacement of
\033[0mwith\033[0m\033[2mbefore wrapping in dim codes ensures that inner resets don’t accidentally drop the dim effect, while still ending with\033[22mviaescapeMultiline(). This is a targeted, low-risk improvement for styled nested output..vortex/installer/playground/tui.php (1)
1-19: LGTM - Well-structured playground demonstration script.The script properly initializes Tui with a ConsoleOutput instance and demonstrates the helper methods systematically. Good for developer reference and testing TUI capabilities.
.vortex/installer/src/Logger/LoggerInterface.php (1)
10-62: Well-designed logger interface with clear contract.The interface provides a clean abstraction for logging lifecycle management. The fluent interface pattern (
enable()/disable()returningstatic) enables method chaining which aligns with the runner implementations..vortex/installer/src/Runner/RunnerInterface.php (2)
13-23: LGTM - Standard exit code constants with proper reference.The exit codes follow POSIX conventions with a helpful reference link. The constants provide clear semantics for command execution outcomes.
48-54: PHPDocint<0, 255>range is informational only.The
@return int<0, 255>annotation documents the expected range but PHP's type system won't enforce it. The AbstractRunner implementation validates this range and throws on violation, which is the correct approach..vortex/installer/tests/Functional/Command/BuildCommandTest.php (2)
28-102: Well-structured functional test with comprehensive mocking.The test setup properly mocks all dependencies (ProcessRunner, logger, CheckRequirementsCommand) and uses callbacks to simulate different exit code scenarios. The separation of concerns between build runner and requirements runner is clean.
116-135: Good test coverage for requirements check success path.The first test case properly validates that requirements checking runs by default and that the build completes successfully when all checks pass. The output assertions correctly verify the presence and absence of expected UI elements.
.vortex/installer/src/Task/Task.php (4)
33-51: Streaming mode implementation looks correct with one type hint issue.The streaming logic properly:
- Saves original output
- Creates TaskOutput wrapper for dimming
- Redirects Tui output
- Captures buffered output via ob_start
- Restores original state
However, the
ob_startcallback signature issue was noted in a past review.
52-56: Non-streaming path correctly preserved.The existing spin-based execution is maintained for non-streaming mode, with proper label handling and hint processing.
102-105: Newstart()method is clean and focused.Simple helper that renders the streaming task header. Follows the same pattern as the non-streaming
label()method.
66-68: Good change: action now returns the result.The return type change from
voidtomixedand returning$returnenables callers to access the action's result, which is necessary for the build workflow..vortex/installer/tests/Unit/TuiTest.php (1)
912-990: LGTM! Comprehensive test coverage for new Tui methods.The six new test methods provide solid coverage for:
- Output swapping via
setOutput()success()andline()message output- Line padding behavior
- Non-interactive mode defaults for
confirm()andgetChar()The tests follow established patterns and properly use
BufferedOutputfor assertions..vortex/installer/tests/Unit/Runner/CommandRunnerTest.php (1)
1-242: LGTM! Thorough test suite for CommandRunner.The tests comprehensively cover:
- Constructor and command execution
- Streaming vs. non-streaming behavior
- Composite output with buffering and logging
- Exit code capture and validation
- Option handling
The use of reflection to test
createCompositeOutput()and the inclusion of helper test commands (TestCommand,TestCommandWithExitCode) demonstrate good testing practices..vortex/installer/tests/Functional/Command/InstallCommandTest.php (1)
1-502: LGTM! Comprehensive functional test coverage.This test suite thoroughly exercises the InstallCommand flow with:
- Multiple installation scenarios (with/without build, JSON config, no-cleanup)
- Requirements checking with various missing tool scenarios
- Download failure handling
- Build command integration (success, failure, profile vs. database flows)
- Proper mock injection for runners and downloader
The data-driven approach with extensive output assertions ensures robust validation of the installer's behavior across all supported workflows.
.vortex/installer/src/Runner/AbstractRunner.php (2)
107-118: LGTM! Proper guard againstgetcwd()failure.The explicit check for
getcwd() === FALSEwith a clear exception message addresses the previous concern about potentially setting$cwdto an empty string.
177-183: LGTM! Exception now includes the invalid exit code value.Including
$this->exitCodein the exception message makes debugging easier when this guard is triggered.
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
431cb17 to
33c4f45
Compare
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
33c4f45 to
83b0a82
Compare
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
83b0a82 to
54693c8
Compare
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
54693c8 to
1380d51
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (21)
.vortex/installer/tests/Traits/TuiTrait.php (1)
37-52: LGTM: helper cleanly maps options to CLI-style flags.Implementation matches the docblock, is easy to read, and is perfectly suitable as a test helper; no changes needed.
.vortex/installer/src/Task/TaskOutput.php (2)
31-36: Fix iterable handling for non-array Traversables.The
(array)cast doesn't correctly handleTraversableobjects—it casts object properties instead of iterating elements. Useiterator_to_array()for proper iterable support:- $dimmed = is_iterable($messages) - ? array_map(fn(string $m): string => Tui::dim($m), (array) $messages) - : Tui::dim($messages); + if (is_iterable($messages)) { + $messages = is_array($messages) ? $messages : iterator_to_array($messages, FALSE); + $dimmed = array_map(fn(string $m): string => Tui::dim($m), $messages); + } + else { + $dimmed = Tui::dim($messages); + }
47-51: Apply the same iterable fix to writeln().The same
(array)cast issue exists here. Apply the same fix as inwrite():- $dimmed = is_iterable($messages) - ? array_map(fn(string $m): string => Tui::dim($m), (array) $messages) - : Tui::dim($messages); + if (is_iterable($messages)) { + $messages = is_array($messages) ? $messages : iterator_to_array($messages, FALSE); + $dimmed = array_map(fn(string $m): string => Tui::dim($m), $messages); + } + else { + $dimmed = Tui::dim($messages); + }.vortex/installer/src/Runner/RunnerInterface.php (1)
24-41: Consider documenting exception behavior forrun().Based on
ProcessRunnerandCommandRunnerimplementations, exceptions may be thrown for invalid commands or execution failures. Adding@throwsannotations would help implementers and callers understand the contract.* @return self * The runner instance for method chaining. + * + * @throws \InvalidArgumentException + * When command contains invalid characters or cannot be resolved. + * @throws \RuntimeException + * When command execution fails or returns invalid exit code. */ public function run(string $command, array $args = [], array $inputs = [], array $env = [], ?OutputInterface $output = NULL): self;.vortex/installer/tests/Functional/Command/BuildCommandTest.php (2)
151-159: Complex Docker check condition could be simplified.The multi-condition check using
str_containsis fragile if command format changes. A previous review suggested using regex for clarity.'requirements_exit_callback' => function (string $current_command): int { // Docker command fails, others succeed. - if ((str_contains($current_command, "command -v 'docker'") || str_contains($current_command, 'command -v docker')) - && !str_contains($current_command, 'compose') - && !str_contains($current_command, 'docker --version')) { + // Match "command -v docker" but not docker-compose or version checks. + if (preg_match('/command -v [\'"]?docker[\'"]?$/', $current_command)) { return RunnerInterface::EXIT_COMMAND_NOT_FOUND; } return RunnerInterface::EXIT_SUCCESS; },
235-250: Inconsistent array construction style.This test case uses the spread operator (
...TuiOutput::present()) while others usearray_merge(). Consider using a consistent approach throughout the data provider for maintainability.'Build with profile shows export database step' => [ 'exit_code_callback' => fn(string $current_command): int => RunnerInterface::EXIT_SUCCESS, 'command_inputs' => [ '--profile' => TRUE, '--skip-requirements-check' => TRUE, ], 'expect_failure' => FALSE, - 'output_assertions' => [ - ...TuiOutput::present([ - TuiOutput::BUILD_BUILDING_SITE, - TuiOutput::BUILD_BUILD_COMPLETED, - ]), - '* ' . TuiOutput::BUILD_EXPORT_DATABASE . ' ahoy export-db', - ], + 'output_assertions' => array_merge( + TuiOutput::present([ + TuiOutput::BUILD_BUILDING_SITE, + TuiOutput::BUILD_BUILD_COMPLETED, + ]), + ['* ' . TuiOutput::BUILD_EXPORT_DATABASE . ' ahoy export-db'], + ), ],.vortex/installer/src/Task/Task.php (1)
42-45: Incorrect type hint inob_startcallback.PHP's output buffering callback only receives a
stringparameter, notstring|iterable. The union type here is misleading.Apply this diff to fix the type hint:
- ob_start(function (string|iterable $buffer) use ($task_output): string { + ob_start(function (string $buffer) use ($task_output): string { $task_output->write($buffer); return ''; }, 1);.vortex/installer/src/Runner/ProcessRunner.php (3)
48-55: Output is overwritten instead of accumulated.Each callback invocation overwrites
$this->outputwith only the current buffer content, discarding all previously captured output. For multi-chunk process output, this will lose data.Apply this diff to accumulate output:
$process->run(function ($type, string|iterable $buffer) use ($logger, $output): void { $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; - $this->output = $buffer; + $this->output .= $buffer; if ($this->shouldStream) { $output->write($buffer); } $logger->write($buffer); });
65-72: Critical: Exit code mapping is immediately overwritten.The
matchstatement on lines 65-70 correctly maps the raw exit code to interface constants, but line 72 immediately overwrites$this->exitCodewith the raw$exit_code, completely negating the mapping logic.Apply this diff to fix the exit code handling:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - - $this->exitCode = $exit_code; return $this;
98-119: Path-based commands may not be validated for existence.When a command contains
/(is a path), the code only validates executability for relative paths that exist in cwd. For absolute paths or relative paths that don't resolve via cwd,$resolvedis set to$base_commandwithout verifying the file exists or is executable. This could lead to confusing errors at process execution time instead of a clear early failure.Consider adding validation:
// If command is a path (contains /), check if it exists directly. if (str_contains((string) $base_command, '/')) { $resolved = $base_command; // Check relative to cwd if not absolute. if (!str_starts_with((string) $base_command, '/')) { $full_path = $this->getCwd() . '/' . $base_command; if (is_executable($full_path)) { $resolved = $full_path; } } + // Verify the resolved path is executable. + if (!is_executable($resolved)) { + throw new \InvalidArgumentException(sprintf('Command not found or not executable: %s', $base_command)); + } }.vortex/installer/src/Logger/FileLogger.php (3)
57-60: Consider explicit resource typing and error handling.The
$handleproperty is typed asmixedwith a PHPDoc annotation forresource|null. Iffopen()fails and returnsfalse, the method correctly returnsfalse, but subsequent calls towrite()would silently do nothing since$this->handlewould beFALSEnotNULL.Consider explicitly setting handle to NULL on failure for consistency:
// @phpstan-ignore-next-line $this->handle = fopen($this->path, 'w'); - return $this->handle !== FALSE; + if ($this->handle === FALSE) { + $this->handle = NULL; + return FALSE; + } + + return TRUE; }
123-132: Edge case:getcwd()can return false.
getcwd()returnsfalseif the current working directory has been deleted. The(string)cast on line 128 would convertfalseto an empty string"", which could lead to unexpected log file paths.public function getDir(): string { if ($this->dir === '') { - $this->dir = (string) getcwd(); + $cwd = getcwd(); + if ($cwd === FALSE) { + throw new \RuntimeException('Unable to determine current working directory'); + } + $this->dir = $cwd; } return $this->dir; }
66-70: Consider checking fwrite return value for robust error handling.The
fwrite()call doesn't check the return value. While this may not be critical for logging, checking for write errors could help detect disk full or permission issues early.public function write(string $content): void { if ($this->handle !== NULL) { - fwrite($this->handle, $content); + $written = fwrite($this->handle, $content); + if ($written === FALSE) { + // Optionally log or handle write error + } } }.vortex/installer/src/Runner/CommandRunner.php (1)
59-66: Critical: Exit code mapping is immediately overwritten.The
matchexpression on lines 59-64 maps exit codes to internal constants, but line 66 immediately overwrites$this->exitCodewith the raw$exit_code. This negates the mapping logic entirely.Remove the overwrite:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - $this->exitCode = $exit_code; $this->output = $buffered_output->fetch();.vortex/installer/tests/Unit/Logger/FileLoggerTest.php (1)
138-143: Consider asserting the no-op case more explicitly.The test for writing when the logger is not open only asserts
TRUEas a placeholder. While the comment explains the intent, this doesn't actually verify the no-op behavior.Consider verifying that no file was created:
else { // When logger is not open, write() should be a no-op. - // We can't directly verify this, but we ensure no errors occur. - // @phpstan-ignore-next-line - $this->assertTrue(TRUE, 'write() should not throw error when logger is not open'); + // Verify no file was created by checking getPath() is still null. + $this->assertNull($logger->getPath(), 'No log file should be created when write() is called on unopened logger'); }.vortex/installer/src/Runner/AbstractRunner.php (1)
73-79: Handle failures fromFileLogger::open()so log paths aren’t silently bogus.
initLogger()ignores the boolean return from$logger->open($command, $args), so if the log file can’t be created (permissions, missing dir, etc.) later consumers still see a non-nullgetPath()even though no file is actually writable. That’s especially problematic because the build/installer summaries direct users to this path for troubleshooting.Consider failing fast (or at least surfacing a clear warning) when opening the log fails, e.g.:
protected function initLogger(string $command, array $args = []): FileLoggerInterface { $logger = $this->getLogger(); $logger->setDir($this->getCwd()); - $logger->open($command, $args); + if (!$logger->open($command, $args)) { + throw new \RuntimeException(sprintf( + 'Unable to open log file for command "%s" in directory "%s".', + $command, + $logger->getDir() + )); + } return $logger; }Adjust to your preferred UX (exception vs. fallback/no-op logger) but avoid silently pretending logging succeeded.
.vortex/installer/src/Command/CheckRequirementsCommand.php (1)
48-51: Make$runnernullable (and cached) to avoid typed-property fatals ingetRunner().
$runneris declared non-nullable and uninitialized:protected ProcessRunner $runner;but
getRunner()does:return $this->runner ?? (new ProcessRunner())->disableLog()->disableStreaming();On first call this reads an uninitialized typed property and will fatally error before the
??fallback is applied.Update the declaration and cache the created instance:
- /** - * The process runner. - */ - protected ProcessRunner $runner; + /** + * The process runner. + */ + protected ?ProcessRunner $runner = NULL; @@ protected function getRunner(): ProcessRunner { - // Return already-set runner if available (for testing). - return $this->runner ?? (new ProcessRunner())->disableLog()->disableStreaming(); + // Return already-set runner if available (for testing). + return $this->runner ??= (new ProcessRunner())->disableLog()->disableStreaming(); }This keeps the lazy-init and DI story intact while avoiding the fatal.
Confirm that the project’s PHP requirement is >= 7.4 so that the `??=` operator is available; otherwise, use an explicit `if ($this->runner === null)` guard instead.Also applies to: 338-341
.vortex/installer/tests/Helpers/TuiOutput.php (1)
113-137: Clarify unused$commandparameters in runner-closure factories.The closures returned by
buildRunnerSuccess(),buildRunnerSuccessProfile(),buildRunnerFailure(),checkRequirementsSuccess(), andcheckRequirementsFailure()all acceptstring $commandbut never use it. That’s fine for matching a runner callback signature, but it’s slightly confusing and may trip static analysis.Consider either renaming the parameter to underscore-prefixed (to signal “intentionally unused”) or adding a brief comment:
- public static function buildRunnerSuccess(): \Closure { - return function (string $command): int { + public static function buildRunnerSuccess(): \Closure { + // $_command is unused; signature matches runner callback interface. + return function (string $_command): int { self::echo([ self::BUILD_ASSEMBLE_DOCKER, @@ - public static function checkRequirementsFailure(): \Closure { - return function (string $command): int { + public static function checkRequirementsFailure(): \Closure { + return function (string $_command): int { self::echo([ // ... ]); return RunnerInterface::EXIT_FAILURE; }; }The
present()/absent()helpers correctly encode the*/!output expectations used in your PHPUnit helpers. Based on learnings, this matches the existing test pattern.Also applies to: 159-261
.vortex/installer/src/Command/InstallCommand.php (3)
190-216: Add CI detection so auto-build is always skipped (with a note) in CI environments.The objectives explicitly say: “always skip auto-build in CI (print note)”. The current flow bases the decision only on
Config::BUILD_NOWand interactivity; CI isn’t considered, so--buildin CI will run the build.You can short-circuit auto-build when common CI variables are present:
- $should_build = $this->config->get(Config::BUILD_NOW); - if (!$should_build && !$this->config->getNoInteraction()) { + $should_build = $this->config->get(Config::BUILD_NOW); + + // Skip auto-build in CI environments and just print a note. + if (Env::get('CI') || Env::get('GITHUB_ACTIONS') || Env::get('GITLAB_CI') || Env::get('JENKINS_URL')) { + if ($should_build) { + Tui::info('Skipping auto-build in CI environment. Run manually with: ahoy build'); + } + $should_build = FALSE; + } + + if (!$should_build && !$this->config->getNoInteraction()) { $should_build = Tui::confirm( label: 'Run the site build now?', - default: TRUE, + default: FALSE, hint: 'Takes ~5-10 min; output will be streamed. You can skip and run later with: ahoy build', ); }This keeps CI runs predictable while still honoring
--buildfor local, interactive use.
445-453: UseRunnerInterface::EXIT_SUCCESSfor build exit checks for consistency.
runBuildCommand()currently returns:return $runner->getExitCode() === Command::SUCCESS;Elsewhere (e.g., in
checkRequirements()and inBuildCommand) you compare againstRunnerInterface::EXIT_SUCCESS. Functionally both are0, but using the runner’s own success constant keeps the abstraction consistent:- return $runner->getExitCode() === Command::SUCCESS; + return $runner->getExitCode() === RunnerInterface::EXIT_SUCCESS;This also makes it obvious you’re interpreting the runner’s exit code, not Symfony’s command status.
636-703: Consider caching injected collaborators in the lazy getters.
getRunner(),getProcessRunner(), andgetDownloader()all use??without assignment:protected function getRunner(): CommandRunner { return $this->runner ?? new CommandRunner($this->getApplication()); }So a new instance is created on every call until a test injects one via the setter. Not wrong, but mildly wasteful and inconsistent with the usual lazy-init pattern you use elsewhere.
If you’re comfortable relying on PHP 7.4+, you can cache them:
protected function getRunner(): CommandRunner { - return $this->runner ?? new CommandRunner($this->getApplication()); + return $this->runner ??= new CommandRunner($this->getApplication()); } protected function getProcessRunner(): ProcessRunner { - return $this->processRunner ?? (new ProcessRunner())->disableLog()->disableStreaming(); + return $this->processRunner ??= (new ProcessRunner())->disableLog()->disableStreaming(); } protected function getDownloader(): Downloader { - return $this->downloader ?? new Downloader(); + return $this->downloader ??= new Downloader(); }This keeps DI behavior unchanged while avoiding repeated instantiation.
Confirm the minimum PHP version for the installer (composer constraints) to ensure the `??=` operator is available; if older PHP must be supported, use explicit `if ($this->runner === null)` style caching instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (36)
.vortex/installer/installer.php(1 hunks).vortex/installer/phpstan.neon(1 hunks).vortex/installer/playground/task-streaming.php(1 hunks).vortex/installer/playground/task.php(1 hunks).vortex/installer/playground/tui.php(1 hunks).vortex/installer/src/Command/BuildCommand.php(1 hunks).vortex/installer/src/Command/CheckRequirementsCommand.php(1 hunks).vortex/installer/src/Command/InstallCommand.php(13 hunks).vortex/installer/src/Logger/FileLogger.php(1 hunks).vortex/installer/src/Logger/FileLoggerInterface.php(1 hunks).vortex/installer/src/Logger/LoggableInterface.php(1 hunks).vortex/installer/src/Logger/LoggerInterface.php(1 hunks).vortex/installer/src/Runner/AbstractRunner.php(1 hunks).vortex/installer/src/Runner/CommandRunner.php(1 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Runner/RunnerInterface.php(1 hunks).vortex/installer/src/Task/Task.php(5 hunks).vortex/installer/src/Task/TaskOutput.php(1 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Tui.php(4 hunks).vortex/installer/tests/Functional/Command/BuildCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php(1 hunks).vortex/installer/tests/Functional/Command/InstallCommandTest.php(1 hunks).vortex/installer/tests/Functional/FunctionalTestCase.php(3 hunks).vortex/installer/tests/Functional/PharTest.php(2 hunks).vortex/installer/tests/Helpers/TuiOutput.php(1 hunks).vortex/installer/tests/Traits/TuiTrait.php(1 hunks).vortex/installer/tests/Unit/ConfigTest.php(2 hunks).vortex/installer/tests/Unit/Logger/FileLoggerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/CommandRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php(1 hunks).vortex/installer/tests/Unit/Task/TaskOutputTest.php(1 hunks).vortex/installer/tests/Unit/TaskTest.php(2 hunks).vortex/installer/tests/Unit/TuiTest.php(1 hunks).vortex/tests/phpunit/Traits/SutTrait.php(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/Task/TaskOutputTest.php.vortex/installer/tests/Functional/Command/InstallCommandTest.php.vortex/installer/tests/Unit/Runner/CommandRunnerTest.php.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php.vortex/installer/tests/Unit/TuiTest.php.vortex/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/tests/Unit/Logger/FileLoggerTest.php.vortex/installer/tests/Functional/PharTest.php.vortex/installer/tests/Unit/TaskTest.php.vortex/installer/tests/Functional/FunctionalTestCase.php.vortex/installer/tests/Helpers/TuiOutput.php
📚 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/installer/tests/Functional/Command/InstallCommandTest.php.vortex/installer/tests/Functional/PharTest.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/Logger/FileLoggerInterface.php.vortex/installer/tests/Unit/Logger/FileLoggerTest.php.vortex/installer/src/Logger/FileLogger.php
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
.vortex/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/src/Command/InstallCommand.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/installer/tests/Functional/Command/BuildCommandTest.php.vortex/installer/tests/Functional/PharTest.php.vortex/installer/tests/Functional/FunctionalTestCase.php
🧬 Code graph analysis (24)
.vortex/installer/tests/Unit/ConfigTest.php (1)
.vortex/installer/src/Utils/Config.php (1)
Config(14-128)
.vortex/installer/tests/Unit/Task/TaskOutputTest.php (1)
.vortex/installer/src/Task/TaskOutput.php (13)
TaskOutput(14-123)write(31-36)writeln(46-51)setVerbosity(56-58)getVerbosity(63-65)isQuiet(70-72)isVerbose(77-79)isVeryVerbose(84-86)isDebug(91-93)setDecorated(98-100)isDecorated(105-107)setFormatter(112-114)getFormatter(119-121)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (1)
.vortex/installer/tests/Traits/TuiTrait.php (1)
tuiOptions(46-52)
.vortex/installer/tests/Unit/Runner/CommandRunnerTest.php (2)
.vortex/installer/src/Runner/CommandRunner.php (4)
CommandRunner(17-146)fetch(137-139)write(109-118)writeln(128-135).vortex/installer/src/Runner/AbstractRunner.php (2)
setCwd(97-102)getExitCode(177-183)
.vortex/installer/tests/Unit/TuiTest.php (1)
.vortex/installer/src/Utils/Tui.php (8)
Tui(17-284)init(25-38)output(40-45)setOutput(47-50)success(60-62)line(80-82)confirm(68-78)getChar(122-141)
.vortex/installer/src/Logger/FileLoggerInterface.php (1)
.vortex/installer/src/Logger/FileLogger.php (3)
getPath(85-87)setDir(117-121)getDir(126-132)
.vortex/installer/playground/tui.php (1)
.vortex/installer/src/Utils/Tui.php (19)
Tui(17-284)output(40-45)init(25-38)dim(112-116)bold(104-106)green(84-86)blue(88-90)purple(92-94)yellow(96-98)cyan(100-102)underscore(108-110)box(197-216)info(52-54)note(56-58)success(60-62)error(64-66)list(164-195)terminalWidth(247-249)center(218-245)
.vortex/installer/tests/Functional/Command/BuildCommandTest.php (2)
.vortex/installer/src/Command/BuildCommand.php (2)
BuildCommand(20-213)setRunner(209-211).vortex/installer/src/Command/InstallCommand.php (1)
setRunner(655-657)
.vortex/installer/src/Logger/LoggerInterface.php (2)
.vortex/installer/src/Logger/FileLogger.php (6)
open(44-61)write(66-70)close(75-80)isEnabled(92-94)enable(99-103)disable(108-112).vortex/installer/src/Runner/CommandRunner.php (1)
write(109-118)
.vortex/installer/src/Logger/LoggableInterface.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (3)
enableLog(123-127)disableLog(132-136)getLogger(52-58)
.vortex/installer/tests/Unit/Logger/FileLoggerTest.php (2)
.vortex/installer/src/Logger/LoggerInterface.php (6)
disable(60-60)isEnabled(44-44)enable(52-52)open(23-23)close(36-36)write(31-31).vortex/installer/src/Logger/FileLoggerInterface.php (3)
getDir(37-37)setDir(29-29)getPath(18-18)
.vortex/installer/src/Runner/RunnerInterface.php (3)
.vortex/installer/src/Runner/CommandRunner.php (1)
run(34-72).vortex/installer/src/Runner/ProcessRunner.php (1)
run(20-75).vortex/installer/src/Runner/AbstractRunner.php (5)
getCommand(167-169)getExitCode(177-183)getOutput(188-200)setCwd(97-102)getCwd(107-118)
.vortex/installer/src/Runner/CommandRunner.php (3)
.vortex/installer/src/Runner/AbstractRunner.php (4)
AbstractRunner(15-409)buildCommandString(328-343)initLogger(73-79)resolveOutput(90-92).vortex/installer/src/Logger/FileLogger.php (2)
close(75-80)write(66-70).vortex/installer/src/Logger/LoggerInterface.php (2)
close(36-36)write(31-31)
.vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (2)
.vortex/installer/src/Runner/ProcessRunner.php (4)
ProcessRunner(15-164)resolveCommand(89-120)prepareArguments(136-145)validateEnvironmentVars(156-162).vortex/installer/src/Runner/AbstractRunner.php (4)
setCwd(97-102)getExitCode(177-183)getOutput(188-200)disableStreaming(158-162)
.vortex/installer/src/Runner/ProcessRunner.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (9)
AbstractRunner(15-409)reset(205-209)buildCommandString(328-343)initLogger(73-79)resolveOutput(90-92)getCwd(107-118)getExitCode(177-183)parseCommand(230-310)formatArgs(380-407)
.vortex/installer/tests/Functional/PharTest.php (1)
.vortex/installer/src/Command/InstallCommand.php (1)
InstallCommand(31-705)
.vortex/installer/tests/Unit/TaskTest.php (3)
.vortex/installer/src/Task/Task.php (3)
Task(11-107)action(17-68)label(70-94).vortex/installer/src/Utils/Tui.php (5)
output(40-45)Tui(17-284)init(25-38)line(80-82)success(60-62).vortex/installer/src/Task/TaskOutput.php (2)
writeln(46-51)write(31-36)
.vortex/installer/src/Runner/AbstractRunner.php (6)
.vortex/installer/src/Logger/FileLogger.php (5)
FileLogger(12-164)setDir(117-121)open(44-61)enable(99-103)disable(108-112).vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)output(40-45).vortex/installer/src/Logger/FileLoggerInterface.php (1)
setDir(29-29).vortex/installer/src/Runner/RunnerInterface.php (5)
getCwd(86-86)setCwd(78-78)getCommand(46-46)getExitCode(54-54)getOutput(67-67).vortex/installer/src/Logger/LoggerInterface.php (3)
open(23-23)enable(52-52)disable(60-60).vortex/docs/.utils/svg-term-render.js (1)
i(41-41)
.vortex/installer/src/Logger/FileLogger.php (3)
.vortex/installer/src/Logger/LoggerInterface.php (6)
open(23-23)write(31-31)close(36-36)isEnabled(44-44)enable(52-52)disable(60-60).vortex/installer/src/Logger/FileLoggerInterface.php (3)
getDir(37-37)getPath(18-18)setDir(29-29).vortex/installer/src/Utils/Validator.php (1)
dirname(32-34)
.vortex/installer/installer.php (3)
.vortex/installer/src/Command/BuildCommand.php (1)
BuildCommand(20-213).vortex/installer/src/Command/CheckRequirementsCommand.php (1)
CheckRequirementsCommand(19-355).vortex/installer/src/Command/InstallCommand.php (1)
InstallCommand(31-705)
.vortex/installer/src/Command/CheckRequirementsCommand.php (3)
.vortex/installer/src/Runner/RunnerInterface.php (3)
run(41-41)getExitCode(54-54)getOutput(67-67).vortex/installer/src/Runner/AbstractRunner.php (3)
getExitCode(177-183)getOutput(188-200)disableLog(132-136).vortex/installer/src/Logger/LoggableInterface.php (1)
disableLog(26-26)
.vortex/installer/src/Command/InstallCommand.php (11)
.vortex/installer/src/Prompts/Handlers/Starter.php (2)
Starter(10-166)options(67-73).vortex/installer/src/Prompts/PromptManager.php (4)
PromptManager(62-638)label(471-479)getResponses(263-265)args(536-602).vortex/installer/src/Runner/CommandRunner.php (2)
CommandRunner(17-146)run(34-72).vortex/installer/src/Runner/ProcessRunner.php (2)
ProcessRunner(15-164)run(20-75).vortex/installer/src/Task/Task.php (2)
Task(11-107)action(17-68).vortex/docs/.utils/svg-term-render.js (1)
input(55-55).vortex/installer/src/Downloader/DownloaderInterface.php (1)
download(31-31).vortex/installer/src/Utils/Config.php (3)
get(80-82)Config(14-128)getNoInteraction(116-118).vortex/installer/src/Prompts/Handlers/LabelMergeConflictsPr.php (3)
label(12-14)default(26-28)hint(19-21).vortex/installer/src/Runner/AbstractRunner.php (1)
getExitCode(177-183).vortex/installer/src/Prompts/Handlers/AbstractHandler.php (2)
options(95-97)id(52-62)
.vortex/installer/tests/Functional/FunctionalTestCase.php (1)
.vortex/installer/src/Command/InstallCommand.php (1)
InstallCommand(31-705)
.vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
.vortex/installer/src/Runner/AbstractRunner.php (18)
AbstractRunner(15-409)getLogger(52-58)getCwd(107-118)setCwd(97-102)enableLog(123-127)disableLog(132-136)disableStreaming(158-162)enableStreaming(144-148)getCommand(167-169)getExitCode(177-183)getOutput(188-200)parseCommand(230-310)buildCommandString(328-343)quoteArgument(354-369)formatArgs(380-407)reset(205-209)initLogger(73-79)resolveOutput(90-92)
🪛 GitHub Actions: Vortex - Test installer
.vortex/installer/tests/Functional/Command/InstallCommandTest.php
[error] 116-116: Installation failed with an error: Command not found: command. Ensure the command is installed and available in PATH.
.vortex/installer/tests/Functional/PharTest.php
[error] 60-60: Installation failed with an error: Command not found: command. Ensure the command is installed and available in PATH.
.vortex/installer/tests/Functional/FunctionalTestCase.php
[error] 107-107: Installation failed with an error: Command not found: command. Ensure the command is installed and available in PATH.
⏰ 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). (3)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-common
🔇 Additional comments (40)
.vortex/installer/phpstan.neon (1)
19-20: LGTM!Setting
treatPhpDocTypesAsCertain: falseimproves type safety by making PHPStan validate against actual PHP types rather than relying solely on PHPDoc annotations..vortex/installer/src/Utils/Config.php (1)
42-43: LGTM!The
BUILD_NOWconstant follows the established naming convention and pattern for installer configuration constants..vortex/installer/tests/Unit/ConfigTest.php (1)
36-36: LGTM!Test coverage for the new
BUILD_NOWconstant is consistent with the existing pattern for other configuration constants.Also applies to: 349-349
.vortex/installer/playground/tui.php (1)
1-187: LGTM!The playground script provides comprehensive demonstrations of Tui utility methods, including colors, styles, boxes, messages, lists, and text centering. This serves as valuable documentation and testing tool for the Tui API.
.vortex/tests/phpunit/Traits/SutTrait.php (1)
82-82: LGTM!The change from a positional argument to a named
--destinationoption makes the installer invocation more explicit and aligns with the updated command API..vortex/installer/tests/Unit/TaskTest.php (2)
9-9: LGTM!The namespace change from
DrevOps\VortexInstaller\Utils\TasktoDrevOps\VortexInstaller\Task\Taskreflects the code reorganization in this PR.
134-413: LGTM!The new streaming tests provide comprehensive coverage for the streaming mode functionality:
- OutputInterface usage
- Echo/print statements
- Mixed output types
- Failure scenarios
- Custom success messages
- Output restoration
- Direct output parameter usage
The tests follow established patterns and the callback at line 296 correctly uses
mixed $resultto avoid type conflicts..vortex/installer/src/Logger/FileLoggerInterface.php (1)
1-39: LGTM!The
FileLoggerInterfaceis well-designed with clear method contracts for file-based logging. The fluent interface pattern (setDirreturningstatic) enables method chaining, and the documentation is clear and complete..vortex/installer/playground/task.php (1)
1-268: LGTM! Comprehensive Task demonstration script.This playground script provides excellent coverage of Task::action() functionality across various scenarios (streaming, failures, closures, hints, etc.). The demonstrations are well-organized and clearly labeled, making it a valuable reference for Task API usage.
.vortex/installer/tests/Functional/PharTest.php (2)
79-79: LGTM! Test expectation updated correctly.The assertion now checks for "Install Vortex from remote or local repository", which aligns with the updated InstallCommand description shown in the relevant code snippets.
106-110: The review comment's concern about a pipeline failure is not supported by code analysis.After examining the code flow from
PharTest.phpthroughProcessRunner.phpand the command execution logic, there is no evidence that the changes would cause "Command not found: command" errors. The code properly:
- Parses the command string 'php' without issues
- Resolves 'php' through
ExecutableFinderwhich correctly locates the executable in PATH- Constructs the process with proper argument handling
The actual shell usage of the builtin
command -vinInstallCommand.php:611is correct and appropriate for tool detection. The changes torunInstallationWithPhar()signature and option defaults align properly withInstallCommandexpectations.If a pipeline failure exists, it should be verified with actual error logs rather than assumed. No concrete evidence of this specific failure was found in the codebase or CI configuration.
.vortex/installer/src/Logger/LoggableInterface.php (1)
1-36: LGTM! Clean and well-documented interface.The
LoggableInterfaceprovides a clear contract for logging capability with appropriate method signatures and return types. The fluent interface pattern (returningstatic) for enable/disable methods supports method chaining, and the documentation is comprehensive..vortex/installer/playground/task-streaming.php (1)
1-196: LGTM! Thorough streaming mode demonstrations.This playground script provides comprehensive coverage of Task streaming scenarios, including echo vs Tui::output(), mixed outputs, failure handling, nested spinners, and styled output. The demonstrations are well-organized and valuable for testing streaming behavior.
.vortex/installer/tests/Unit/Task/TaskOutputTest.php (1)
20-126: LGTM! Comprehensive test coverage for TaskOutput.The tests validate all public methods of TaskOutput, including delegation to the wrapped output, message handling, verbosity states, decoration, and formatter. The functional assertions (checking that messages reach the output) are appropriate and sufficient.
Note: A past review suggested optionally verifying ANSI dim escape codes (
\033[2m) are present in the output to more explicitly confirm dimming behavior, but this is a nice-to-have enhancement rather than a requirement..vortex/installer/installer.php (1)
11-24: LGTM! Clean multi-command architecture setup.The bootstrap now properly registers three commands (InstallCommand, CheckRequirementsCommand, BuildCommand) and sets a simpler default command using the string 'install'. This aligns with the expanded command structure described in the AI summary and relevant code snippets.
.vortex/installer/src/Logger/LoggerInterface.php (1)
1-62: LGTM! Well-designed logging interface.The
LoggerInterfaceprovides a complete logging lifecycle contract with clear semantics:open()returns a boolean to indicate success (useful when logging is disabled),write()andclose()handle the logging operations, andisEnabled()/enable()/disable()control the logging state. The documentation is thorough and method signatures are appropriate..vortex/installer/src/Utils/Tui.php (4)
47-50: LGTM! Useful addition for dynamic output management.The
setOutput()method allows updating the output instance and properly synchronizes withPrompt::setOutput(), maintaining consistency across Tui and Laravel Prompts.
60-62: LGTM! Semantic success helper added.The
success()method provides a clear, semantic wrapper aroundinfo()for success messages, improving code readability.
68-78: LGTM! Confirm method with proper non-interactive handling.The
confirm()method correctly returns the default value in non-interactive mode and uses named parameters when delegating to Laravel Prompts, which is good practice for clarity and maintainability.
113-114: LGTM! Smart enhancement to dim() for consistent styling.The modification to preserve dimming through ANSI reset codes (
\033[0m→\033[0m\033[2m) ensures that dimmed output remains dimmed even after color resets, which is important for consistent visual presentation in streaming scenarios..vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (3)
1-48: LGTM! Test structure and setup are well-organized.The test class properly initializes Tui, sets working directory, and validates return types. The data provider pattern is clean and comprehensive.
50-79: Variable shadowing issue has been addressed.The previous review flagged
$outputreuse on line 77. This has been correctly renamed to$runner_output, improving readability.
367-393: LGTM! TestableProcessRunner is an appropriate pattern for testing protected methods.The helper class cleanly exposes protected methods for testing without modifying production code.
.vortex/installer/tests/Unit/Runner/CommandRunnerTest.php (3)
28-55: LGTM! Test setup and assertions are correct.The variable shadowing issue from line 53 has been addressed with
$runner_output.
92-122: LGTM! Reflection-based testing of protected method is appropriate here.Testing
createCompositeOutputvia reflection is acceptable since it validates important internal buffering/streaming behavior that would otherwise require complex integration setups.
214-242: LGTM! Test command implementations are minimal and appropriate.The helper commands are correctly scoped to their purpose - one returns success, one returns failure - without unnecessary complexity.
.vortex/installer/src/Runner/RunnerInterface.php (2)
10-23: LGTM! Exit code constants align with UNIX conventions.The constants
EXIT_SUCCESS = 0,EXIT_FAILURE = 1,EXIT_INVALID = 2, andEXIT_COMMAND_NOT_FOUND = 127follow standard exit code semantics as referenced by the TLDP link in the comment.
43-88: LGTM! Interface methods are well-documented and complete.The getter methods cover all necessary state retrieval, and the working directory methods support proper execution context management.
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (4)
53-71: LGTM! Build runner mock configuration is comprehensive.The mock properly handles
run(),getExitCode(),getOutput(),getCommand(),getLogger(), andsetCwd()for method chaining.
136-159: LGTM! Basic install scenarios are well-structured.The data provider correctly tests the install flow without build flag, verifying expected UI outputs are present and build-related outputs are absent.
200-321: LGTM! Requirements check failure scenarios provide good coverage.The test cases cover individual missing tools (git, curl, tar, composer) and multiple missing tools, validating proper error messages and flow termination.
351-376: LGTM! Build integration scenarios are thorough.The test properly validates the full install-with-build flow, including success messages, login info, log file reference, and documentation links.
.vortex/installer/tests/Functional/Command/BuildCommandTest.php (2)
24-102: LGTM! Test method structure is well-organized.The test properly mocks both the build runner and requirements runner, configures method behaviors via callbacks, and validates output assertions.
252-385: LGTM! Build failure and success scenarios provide comprehensive coverage.The test cases cover various exit codes (1, 2, 127), log file path validation, site URL display, and next steps output. This aligns well with the PR objective for "concise actionable success/failure summaries."
.vortex/installer/tests/Unit/TuiTest.php (1)
912-990: LGTM! Comprehensive test coverage for new Tui methods.The six new test methods correctly validate:
setOutput: output swapping and retrievalsuccessandline: message writing with optional paddingconfirmandgetChar: non-interactive mode behavior returning defaultsAll tests follow established patterns and include appropriate assertions.
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (1)
1-395: LGTM! Comprehensive functional test coverage.The test suite provides excellent coverage of CheckRequirementsCommand with:
- 17 data-driven scenarios covering all requirements, individual tool checks, edge cases, and error conditions
- Proper mock setup for ProcessRunner with callback-based exit code handling
- Appropriate use of TuiOutput assertion helpers
- Clear test organization and readable data provider structure
.vortex/installer/tests/Functional/FunctionalTestCase.php (3)
93-95: LGTM! Proper conditional destination handling.The change correctly adds DESTINATION to defaults only when
$dstis non-empty and not'0', preventing invalid option values from being passed to the command.
114-114: Type hint formatting normalized.The spacing in union types (
string | array) has been normalized for consistency.Also applies to: 131-131
107-107: This review comment references non-existent code elements. The classes ProcessRunner and CheckRequirementsCommand are not present in this repository, and there is no usage of thecommandshell builtin in the PHP codebase. Line 107 of FunctionalTestCase.php simply invokes$this->applicationRun($args, [], $expect_fail), which is an inherited method from an external trait and has no connection to shell command resolution logic.Likely an incorrect or invalid review comment.
.vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
225-241: Tests correctly exercisereset()behavior; rely on fixing the underlying constant.
testReset()andConcreteRunner::resetPublic()provide good coverage forAbstractRunner::reset()(command, output, and exit code reset to initial state). With the currentself::EXIT_SUCCESSbug inAbstractRunner, this test would fatally error instead of asserting; once you switchreset()to useRunnerInterface::EXIT_SUCCESS, these tests should pass as written.No changes needed here beyond fixing the production method.
Also applies to: 585-587
.vortex/installer/tests/Functional/Command/InstallCommandTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (1)
40-51: Complete the ProcessRunner mock to prevent potential failures.The
$install_process_runnermock only stubsrun()andgetExitCode(), but is missing stubs for other methods that ProcessRunner may call during execution (such asgetLogger(),getCwd(),getCommand(),getOutput(), andsetCwd()). If any of these methods are invoked, the mock will return NULL or default mock values, potentially causing unexpected behavior or test failures.Reference the complete mock pattern used for
$build_runner(lines 64-72) and add similar method stubs:$install_process_runner->method('getExitCode') ->willReturnCallback(function () use ($install_process_runner_exit_callback, &$install_process_runner_command) { return $install_process_runner_exit_callback($install_process_runner_command); }); +$install_process_runner->method('getOutput')->willReturn(''); +$install_process_runner->method('getCommand')->willReturn(''); +$install_process_runner->method('getCwd')->willReturn(self::$sut); +$install_process_runner->method('setCwd')->willReturn($install_process_runner); +$mock_install_logger = $this->createMock(FileLoggerInterface::class); +$mock_install_logger->method('getPath')->willReturn('/tmp/mock-install.log'); +$install_process_runner->method('getLogger')->willReturn($mock_install_logger);Based on learnings, this pattern matches the established mock setup convention in this test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (24)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php(2 hunks).vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/CodeProviderHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/DatabaseDownloadSourceHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/DependencyUpdatesProviderHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/DeployTypeHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/HostingProjectNameHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/ModulesHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/NotificationChannelsHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/ProvisionTypeHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/PullRequestHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/ServicesHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/StarterHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/ThemeHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/TimezoneHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/ToolsHandlerProcessTest.php(1 hunks).vortex/installer/tests/Functional/Handlers/VersionSchemeHandlerProcessTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/installer/tests/Functional/Command/InstallCommandTest.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/Functional/Command/InstallCommandTest.php
🧬 Code graph analysis (21)
.vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php (5)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/NotificationChannelsHandlerProcessTest.php (2)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/ToolsHandlerProcessTest.php (2)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/DeployTypeHandlerProcessTest.php (3)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/ThemeHandlerProcessTest.php (16)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (1)
dataProviderHandlerProcess(68-111).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/CodeProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-33).vortex/installer/tests/Functional/Handlers/DatabaseDownloadSourceHandlerProcessTest.php (1)
dataProviderHandlerProcess(18-44).vortex/installer/tests/Functional/Handlers/DependencyUpdatesProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-37).vortex/installer/tests/Functional/Handlers/DeployTypeHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-57).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-26).vortex/installer/tests/Functional/Handlers/HostingProjectNameHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-46).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ModulesHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-131).vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php (1)
dataProviderHandlerProcess(21-35).vortex/installer/tests/Functional/Handlers/NotificationChannelsHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-120).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/ProvisionTypeHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-38)
.vortex/installer/tests/Functional/Handlers/CodeProviderHandlerProcessTest.php (2)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (2)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-26)
.vortex/installer/tests/Functional/Handlers/StarterHandlerProcessTest.php (14)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (1)
dataProviderHandlerProcess(68-111).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/CodeProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-33).vortex/installer/tests/Functional/Handlers/DatabaseDownloadSourceHandlerProcessTest.php (1)
dataProviderHandlerProcess(18-44).vortex/installer/tests/Functional/Handlers/DependencyUpdatesProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-37).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-26).vortex/installer/tests/Functional/Handlers/HostingProjectNameHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-46).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ModulesHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-131).vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php (1)
dataProviderHandlerProcess(21-35).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/ProvisionTypeHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-38)
.vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (16)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/CodeProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-33).vortex/installer/tests/Functional/Handlers/DatabaseDownloadSourceHandlerProcessTest.php (1)
dataProviderHandlerProcess(18-44).vortex/installer/tests/Functional/Handlers/DependencyUpdatesProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-37).vortex/installer/tests/Functional/Handlers/DeployTypeHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-57).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-26).vortex/installer/tests/Functional/Handlers/HostingProjectNameHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-46).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ModulesHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-131).vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php (1)
dataProviderHandlerProcess(21-35).vortex/installer/tests/Functional/Handlers/NotificationChannelsHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-120).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/ProvisionTypeHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-38).vortex/installer/tests/Functional/Handlers/PullRequestHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-35)
.vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (5)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-26).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/HostingProjectNameHandlerProcessTest.php (1)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70)
.vortex/installer/tests/Functional/Handlers/TimezoneHandlerProcessTest.php (1)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (16)
.vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (1)
dataProviderHandlerProcess(68-111).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/CodeProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-33).vortex/installer/tests/Functional/Handlers/DatabaseDownloadSourceHandlerProcessTest.php (1)
dataProviderHandlerProcess(18-44).vortex/installer/tests/Functional/Handlers/DependencyUpdatesProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-37).vortex/installer/tests/Functional/Handlers/DeployTypeHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-57).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-26).vortex/installer/tests/Functional/Handlers/HostingProjectNameHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-46).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ModulesHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-131).vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php (1)
dataProviderHandlerProcess(21-35).vortex/installer/tests/Functional/Handlers/NotificationChannelsHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-120).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/ProvisionTypeHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-38).vortex/installer/tests/Functional/Handlers/PullRequestHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-35)
.vortex/installer/tests/Functional/Handlers/DependencyUpdatesProviderHandlerProcessTest.php (3)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/ModulesHandlerProcessTest.php (1)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (2)
.vortex/installer/src/Command/InstallCommand.php (2)
InstallCommand(31-705)setRunner(655-657).vortex/installer/tests/Traits/TuiTrait.php (1)
tuiOptions(46-52)
.vortex/installer/tests/Functional/Handlers/VersionSchemeHandlerProcessTest.php (2)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (2)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/ProvisionTypeHandlerProcessTest.php (8)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(16-32).vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-26).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php (1)
dataProviderHandlerProcess(21-35).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25).vortex/installer/tests/Functional/Handlers/PullRequestHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-35)
.vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (3)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
.vortex/installer/tests/Functional/Handlers/PullRequestHandlerProcessTest.php (3)
.vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
dataProviderHandlerProcess(70-70).vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
dataProviderHandlerProcess(17-34).vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
dataProviderHandlerProcess(15-25)
⏰ 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). (6)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-docs
🔇 Additional comments (31)
.vortex/installer/tests/Functional/Handlers/StarterHandlerProcessTest.php (1)
16-45: Rename todataProviderHandlerProcesscorrectly aligns with abstract base and peersThe provider name now matches
AbstractHandlerProcessTestCase::dataProviderHandlerProcess()and is consistent with the other*HandlerProcessTestclasses; the body remains unchanged and valid. No further changes needed here..vortex/installer/tests/Functional/Handlers/ModulesHandlerProcessTest.php (1)
17-131: Method rename correctly implements abstract data providerRenaming to
dataProviderHandlerProcessaligns this implementation with the abstract declaration inAbstractHandlerProcessTestCase(same visibility, static, and return type). The provider payload remains unchanged and consistent..vortex/installer/tests/Functional/Handlers/CodeProviderHandlerProcessTest.php (1)
16-16: Data provider rename correctly aligns with abstract contractRenaming the method to
dataProviderHandlerProcess()matches the abstract definition inAbstractHandlerProcessTestCaseand keeps this test consistent with the other*HandlerProcessTestclasses. No issues found with the change..vortex/installer/tests/Functional/Handlers/ProvisionTypeHandlerProcessTest.php (1)
17-38: Rename todataProviderHandlerProcesscorrectly aligns with abstract base and suite conventionsThe provider is now named consistently with
AbstractHandlerProcessTestCase::dataProviderHandlerProcess()and the other*HandlerProcessTestclasses, while keeping the data unchanged. This restores the abstract contract and should avoid data-provider resolution issues..vortex/installer/tests/Functional/Handlers/ProfileHandlerProcessTest.php (1)
15-15: LGTM! Method rename aligns with abstract contract.The rename from
dataProviderInstalltodataProviderHandlerProcesscorrectly implements the abstract method defined inAbstractHandlerProcessTestCaseand follows the same pattern applied across all handler tests in this PR..vortex/installer/tests/Functional/Handlers/NamesHandlerProcessTest.php (1)
21-21: LGTM! Standardized naming convention.The method rename aligns with the abstract declaration in
AbstractHandlerProcessTestCaseand follows the same pattern used across other handler-process tests in the codebase..vortex/installer/tests/Functional/Handlers/PullRequestHandlerProcessTest.php (1)
17-35: LGTM! Method rename aligns with project conventions.The renaming of the data provider method to
dataProviderHandlerProcess()correctly implements the abstract method from the parent class and standardizes naming across all HandlerProcessTest files in the project..vortex/installer/tests/Functional/Handlers/NotificationChannelsHandlerProcessTest.php (1)
17-17: LGTM! Method renamed to align with abstract parent class.The rename from
dataProviderInstall()todataProviderHandlerProcess()correctly implements the abstract method fromAbstractHandlerProcessTestCaseand maintains consistency with other handler test classes..vortex/installer/tests/Functional/Handlers/TimezoneHandlerProcessTest.php (1)
17-17: LGTM! Correct alignment with parent class.The method rename to
dataProviderHandlerProcess()correctly implements the abstract method declared inAbstractHandlerProcessTestCase..vortex/installer/tests/Functional/Handlers/DeployTypeHandlerProcessTest.php (1)
17-17: LGTM! Method rename aligns with parent class contract.The rename from
dataProviderInstall()todataProviderHandlerProcess()correctly implements the abstract method defined inAbstractHandlerProcessTestCaseand aligns with the naming convention used by sibling test classes..vortex/installer/tests/Functional/Handlers/DependencyUpdatesProviderHandlerProcessTest.php (1)
16-16: LGTM! Method rename correctly implements the abstract parent method.The rename from
dataProviderInstall()todataProviderHandlerProcess()correctly implements the abstract method declared inAbstractHandlerProcessTestCaseand aligns with the naming convention used across other handler process tests (e.g.,ProfileHandlerProcessTest,CiProviderHandlerProcessTest)..vortex/installer/tests/Functional/Handlers/ToolsHandlerProcessTest.php (1)
18-18: LGTM! Method rename aligns with parent class contract.The rename from
dataProviderInstall()todataProviderHandlerProcess()correctly implements the abstract method declared inAbstractHandlerProcessTestCaseand is consistent with the PR-wide standardization across handler tests..vortex/installer/tests/Functional/Handlers/AbstractHandlerProcessTestCase.php (1)
39-39: LGTM! Consistent refactoring improves naming clarity.The renaming from
dataProviderInstalltodataProviderHandlerProcessbetter reflects the purpose of the method and aligns the abstract base class with all concrete implementations shown in the codebase.Also applies to: 70-70
.vortex/installer/tests/Functional/Handlers/HostingProviderHandlerProcessTest.php (1)
17-33: Data provider rename correctly aligned with abstract test contractRenaming to
dataProviderHandlerProcesswhile keeping the signature and datasets intact matchesAbstractHandlerProcessTestCase’s abstract method and keeps this handler test consistent with the rest of the suite..vortex/installer/tests/Functional/Handlers/ThemeHandlerProcessTest.php (1)
16-52: Consistent data provider naming with preserved theme scenarios
dataProviderHandlerProcessnow matches the abstract base method name, and all existing theme datasets and assertions are preserved, so test behavior remains unchanged while naming is standardized..vortex/installer/tests/Functional/Handlers/VersionSchemeHandlerProcessTest.php (1)
16-52: Version scheme provider rename is correct and non-breakingThe method is now named
dataProviderHandlerProcesswith the same static/return-type contract and unchanged datasets for calver/semver/other, keeping tests functionally identical and aligned with the abstract base..vortex/installer/tests/Functional/Handlers/CiProviderHandlerProcessTest.php (1)
16-31: CI provider data provider rename matches handler-process conventionUpdating the method to
dataProviderHandlerProcesskeeps the signature compatible withAbstractHandlerProcessTestCasewhile leaving the GHA/CircleCI datasets intact, so behavior stays the same with clearer naming..vortex/installer/tests/Functional/Handlers/AiCodeInstructionsHandlerProcessTest.php (1)
15-24: AI instructions provider rename is aligned and safeThe data provider now uses the shared
dataProviderHandlerProcessname with the same static/array contract and unchanged CLAUDE/NONE cases, so tests remain behaviorally identical while conforming to the base class..vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (1)
68-110: Baseline data provider correctly renamed with datasets preserved
dataProviderHandlerProcessnow satisfies the abstract base signature, and all baseline/non-interactive/config scenarios are unchanged, so this is a pure naming alignment with no impact on test coverage..vortex/installer/tests/Functional/Handlers/DocsHandlerProcessTest.php (1)
15-25: Docs handler data provider rename is consistent and non-disruptiveThe provider is now named
dataProviderHandlerProcesswith the same signature and enabled/disabled datasets, keeping behavior stable while aligning with the shared handler-process naming convention..vortex/installer/tests/Functional/Handlers/HostingProjectNameHandlerProcessTest.php (1)
17-45: Hosting project-name provider rename correctly matches shared contractRenaming to
dataProviderHandlerProcesswith identical datasets and assertions keeps this test aligned withAbstractHandlerProcessTestCaseand the other handler tests, without altering behavior..vortex/installer/tests/Functional/Handlers/ServicesHandlerProcessTest.php (1)
18-18: Rename is correctly implemented and wired. The data provider method name change fromdataProviderInstalltodataProviderHandlerProcessis properly applied across the handler test hierarchy. The parent classAbstractHandlerProcessTestCasecorrectly references it via the#[DataProvider('dataProviderHandlerProcess')]attribute on line 39, and all 21 handler test classes consistently implement the same data provider method name. No stale references remain..vortex/installer/tests/Functional/Handlers/DatabaseDownloadSourceHandlerProcessTest.php (1)
18-18: LGTM: sensible rename for consistency.The rename from
dataProviderInstall()todataProviderHandlerProcess()aligns perfectly with the test class naming convention and improves clarity. The parent classAbstractHandlerProcessTestCaseproperly declares this as an abstract method (line 70) and references it via the#[DataProvider('dataProviderHandlerProcess')]attribute on the test method (line 39). All 23 handler test files in the suite consistently implement this provider with the new name, and no stale references to the old name remain in the codebase..vortex/installer/tests/Functional/Command/InstallCommandTest.php (8)
1-26: LGTM! Test class structure is well-organized.The imports, namespace, and class declaration are correct. The use of PHPUnit attributes (
#[CoversClass]) and appropriate test base class follows best practices.
53-72: LGTM! Complete and well-structured mock setup.The
$build_runnermock properly stubs all necessary methods includinggetOutput(),getCommand(),getLogger(), andsetCwd(), ensuring comprehensive test coverage without unexpected mock behavior.
91-101: LGTM! Appropriate test double strategy.The conditional mocking of the downloader for failure scenarios and the use of a real repository URI for success paths is a pragmatic approach for functional testing. The comment clearly explains the rationale.
103-121: LGTM! Clean test execution flow.The test properly initializes the application, registers commands with mocked runners via dependency injection, and performs comprehensive output assertions. The structure is clear and maintainable.
136-196: LGTM! Well-structured data provider with clear scenarios.The data provider uses descriptive scenario names, detailed type annotations, and helper methods (
tuiOptions,TuiOutput::present/absent) to improve readability and maintainability. The initial scenarios appropriately cover basic success paths.
197-321: LGTM! Comprehensive requirements check failure coverage.The test scenarios thoroughly cover individual tool failures (git, curl, tar, composer) and multiple missing tools. The use of closures with command string inspection (
str_contains) effectively simulates missing tools, and assertions verify appropriate error messaging.
323-346: LGTM! Download failure scenario properly isolated.The test correctly uses the
download_should_failparameter to trigger download failure and verifies that the installation stops at the download phase without proceeding to subsequent steps.
348-497: LGTM! Comprehensive build scenario coverage.The build test scenarios thoroughly exercise success and failure paths, including:
- Standard build flow
- Profile-based build with detailed output verification
- Build command failure handling
- Requirements check failure during build
The extensive use of both positive (
TuiOutput::present) and negative (TuiOutput::absent) assertions ensures output correctness across the entire orchestration of InstallCommand → CheckRequirementsCommand → BuildCommand. The profile scenario (lines 378-441) particularly demonstrates thorough verification of the build flow with ~40 assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.vortex/tests/phpunit/Traits/SutTrait.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.vortex/tests/phpunit/Traits/SutTrait.php (1)
.vortex/tests/phpunit/Traits/ProcessTrait.php (1)
cmd(59-80)
⏰ 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). (6)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-docs
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
961dbaa to
533bd98
Compare
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
9aa315e to
12afa82
Compare
github-actions
bot
removed
the
CONFLICT
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (40)
.vortex/tests/phpunit/Traits/SutTrait.php (1)
80-82: Shell injection risk remains: destination path needs escaping.The destination path from
static::locationsSut()is still interpolated directly into the command string withoutescapeshellarg(). While the TODO indicates this is temporary, the interim solution should be hardened against paths containing spaces or special characters.This duplicates the previous review concern. Apply the suggested fix from the earlier comment:
- $cmd = sprintf('php .vortex/installer/installer.php --no-interaction --destination=%s', static::locationsSut()); + $cmd = sprintf( + 'php .vortex/installer/installer.php --no-interaction --destination=%s', + escapeshellarg(static::locationsSut()), + );.vortex/installer/tests/Helpers/TuiOutput.php (1)
159-261: Consider signaling intentionally unused parameter (optional).All five runner callback closures declare
string $commandbut never reference it. Per PHP conventions, prefixing with underscore ($_command) clearly signals the parameter is required by the interface but intentionally unused.Apply this pattern to all five closures:
- public static function buildRunnerSuccess(): \Closure { - return function (string $command): int { + public static function buildRunnerSuccess(): \Closure { + return function (string $_command): int {Repeat for
buildRunnerSuccessProfile,buildRunnerFailure,checkRequirementsSuccess, andcheckRequirementsFailure..vortex/installer/src/Runner/CommandRunnerAwareTrait.php (1)
26-29: Cache the lazily-created CommandRunner instance.The getter creates a new CommandRunner when $commandRunner is NULL but doesn't store it, potentially losing configuration. This was already noted in a previous review.
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (1)
364-384: Includepygmyin “Available” expectations for invalid requirements
CheckRequirementsCommand::REQUIREMENTScontainspygmy, and the validation error message prints all available requirements. The tests for “Invalid requirement name” and “Mixed valid and invalid requirements” still expect onlydocker, docker-compose, ahoy, so they’ll miss the trailing, pygmy.Consider updating both cases to match the real message, e.g.:
- '* Available: docker, docker-compose, ahoy', + '* Available: docker, docker-compose, ahoy, pygmy',for both scenarios so the expectations track the current REQUIREMENTS set.
.vortex/installer/tests/Functional/FunctionalTestCase.php (1)
96-98: Initialize$argsbefore use.The
$argsvariable is used without prior initialization, which can trigger notices under strict error reporting. While PHP implicitly creates the array, this violates best practices.Apply this diff:
$options += $defaults; + + $args = []; foreach ($options as $option => $value) { $args['--' . $option] = $value; }.vortex/installer/src/Runner/ProcessRunnerAwareTrait.php (1)
25-27: Cache the auto-created ProcessRunner instance.
getProcessRunner()returns$this->processRunner ?? new ProcessRunner()without assigning the new instance to the property. This means any configuration applied to the auto-created runner (e.g., disabling logging/streaming, changing CWD) will be lost on the next call ifsetProcessRunner()was never invoked.Apply this diff:
- public function getProcessRunner(): ProcessRunner { - return $this->processRunner ?? new ProcessRunner(); - } + public function getProcessRunner(): ProcessRunner { + if ($this->processRunner === NULL) { + $this->processRunner = new ProcessRunner(); + } + return $this->processRunner; + }.vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (1)
138-141: Replace placeholder assertion with explicit pass notification.Using
$this->assertTrue(TRUE)is a code smell. PHPUnit provides$this->addToAssertionCount(1)for cases where validation passed implicitly.Apply this diff:
if ($expected_exception === NULL) { - // @phpstan-ignore-next-line - $this->assertTrue(TRUE, 'Validation passed'); + // No exception means validation passed - increment assertion count. + $this->addToAssertionCount(1); }.vortex/installer/src/Task/TaskOutput.php (2)
31-36: Make iterable handling robust for non-array Traversables.
(array) $messageswill not iterate genericTraversableinstances; it instead casts object properties, which can break if Symfony ever passes a non-array iterable.Apply this diff:
public function write(string | iterable $messages, bool $newline = FALSE, int $options = 0): void { - $dimmed = is_iterable($messages) - ? array_map(fn(string $m): string => Tui::dim($m), (array) $messages) - : Tui::dim($messages); + if (is_iterable($messages)) { + $messages = is_array($messages) ? $messages : iterator_to_array($messages, FALSE); + $dimmed = array_map(fn(string $m): string => Tui::dim($m), $messages); + } + else { + $dimmed = Tui::dim($messages); + } $this->wrapped->write($dimmed, $newline, $options); }
46-51: Same iterable handling issue inwriteln().The
writeln()method has the same(array)cast issue aswrite(). Apply the same fix here.Apply this diff:
public function writeln(string | iterable $messages, int $options = 0): void { - $dimmed = is_iterable($messages) - ? array_map(fn(string $m): string => Tui::dim($m), (array) $messages) - : Tui::dim($messages); + if (is_iterable($messages)) { + $messages = is_array($messages) ? $messages : iterator_to_array($messages, FALSE); + $dimmed = array_map(fn(string $m): string => Tui::dim($m), $messages); + } + else { + $dimmed = Tui::dim($messages); + } $this->wrapped->writeln($dimmed, $options); }.vortex/installer/tests/Functional/Command/BuildCommandTest.php (3)
42-46: Make runner mock callbacks tolerant of extra arguments.The callback signature declares a single
string $commandparameter. Sincerun()may be invoked with additional arguments (e.g.,env: $env), this can generate "too many arguments" warnings.Apply this diff:
$build_process_runner->method('run') - ->willReturnCallback(function (string $command) use ($build_process_runner, &$current_command): MockObject { + ->willReturnCallback(function (string $command, ...$args) use ($build_process_runner, &$current_command): MockObject { $current_command = $command; return $build_process_runner; });
88-92: Same callback signature issue for requirements runner.The requirements runner callback has the same single-parameter limitation.
Apply this diff:
$requirements_runner->method('run') - ->willReturnCallback(function (string $command) use ($requirements_runner, &$current_requirements_command): MockObject { + ->willReturnCallback(function (string $command, ...$args) use ($requirements_runner, &$current_requirements_command): MockObject { $current_requirements_command = $command; return $requirements_runner; });
253-260: Spread operator usage is inconsistent with other test cases.This test case uses
...TuiOutput::present()spread syntax while others usearray_merge(). Consider using a consistent approach throughout.Apply this diff:
'Build with profile shows export database step' => [ 'exit_code_callback' => fn(string $current_command): int => RunnerInterface::EXIT_SUCCESS, 'command_inputs' => [ '--profile' => TRUE, '--skip-requirements-check' => TRUE, ], 'expect_failure' => FALSE, - 'output_assertions' => [ - ...TuiOutput::present([ - TuiOutput::BUILD_BUILDING_SITE, - TuiOutput::BUILD_BUILD_COMPLETED, - ]), - '* ' . TuiOutput::BUILD_EXPORT_DATABASE . ' ahoy export-db', - ], + 'output_assertions' => array_merge( + TuiOutput::present([ + TuiOutput::BUILD_BUILDING_SITE, + TuiOutput::BUILD_BUILD_COMPLETED, + ]), + ['* ' . TuiOutput::BUILD_EXPORT_DATABASE . ' ahoy export-db'], + ), ],.vortex/installer/src/Runner/RunnerInterface.php (1)
24-41: Consider documenting exception behavior forrun().The interface doesn't specify what exceptions may be thrown. Based on the ProcessRunner implementation,
InvalidArgumentExceptioncan be thrown for invalid commands or arguments. Adding@throwsannotations would clarify the contract for implementers and callers.Apply this diff:
/** * Run a command. * * @param string $command * The command to run. Can include arguments (e.g., "git status"). * @param array<int|string, mixed> $args * Additional command arguments. * @param array<string, string> $inputs * Interactive inputs for the command. * @param array<string, string> $env * Environment variables. * @param \Symfony\Component\Console\Output\OutputInterface|null $output * Output interface. Defaults to STDOUT if NULL. * * @return self * The runner instance for method chaining. + * + * @throws \InvalidArgumentException + * When command contains invalid characters or cannot be resolved. + * @throws \RuntimeException + * When command execution fails with an invalid exit code. */ public function run(string $command, array $args = [], array $inputs = [], array $env = [], ?OutputInterface $output = NULL): self;.vortex/installer/src/Task/Task.php (1)
42-45: Incorrect type hint inob_startcallback.PHP's output buffering callback only receives a
stringparameter, notstring|iterable. The union type here is misleading and inconsistent with PHP's actual behavior.Apply this diff:
- ob_start(function (string|iterable $buffer) use ($task_output): string { + ob_start(function (string $buffer) use ($task_output): string { $task_output->write($buffer); return ''; }, 1);.vortex/installer/src/Logger/FileLogger.php (3)
126-132: Handle getcwd() failure explicitly.
getcwd()returnsFALSEif the current working directory has been deleted. The cast to string (line 128) would produce an empty string"", leading to invalid log file paths like"/.logs/...".Apply this fix:
public function getDir(): string { if ($this->dir === '') { - $this->dir = (string) getcwd(); + $cwd = getcwd(); + if ($cwd === FALSE) { + throw new \RuntimeException('Unable to determine current working directory'); + } + $this->dir = $cwd; } return $this->dir; }
66-70: Consider checking fwrite() return value for robust error handling.The
fwrite()call doesn't verify the return value. While non-critical for logging, checking for write errors (returnsFALSEor fewer bytes than expected) could help surface disk-full or permission issues early.Optional improvement:
public function write(string $content): void { if ($this->handle !== NULL) { - fwrite($this->handle, $content); + $written = fwrite($this->handle, $content); + if ($written === FALSE || $written < strlen($content)) { + // Log write failure or set handle to NULL to prevent further writes + } } }
44-61: Set handle to NULL explicitly on fopen() failure.If
fopen()returnsFALSE(line 58),$this->handlewill beFALSErather thanNULL. Thewrite()method (line 67) checksif ($this->handle !== NULL), and sinceFALSE !== NULLevaluates toTRUE, it would attemptfwrite(FALSE, $content), causing a warning or error.Apply this fix:
// @phpstan-ignore-next-line $this->handle = fopen($this->path, 'w'); - return $this->handle !== FALSE; + if ($this->handle === FALSE) { + $this->handle = NULL; + return FALSE; + } + + return TRUE; }.vortex/installer/src/Runner/CommandRunner.php (1)
59-66: Critical: Exit code mapping is immediately overwritten.The
matchexpression (lines 59-64) maps exit codes to internal constants, but line 66 immediately overwrites$this->exitCodewith the raw$exit_code, negating the mapping entirely.Remove the overwrite:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - $this->exitCode = $exit_code; $this->output = $buffered_output->fetch();.vortex/installer/src/Runner/AbstractRunner.php (3)
73-79: Consider handling logger open failures explicitly.The return value of
$logger->open($command, $args)(line 76) is ignored. If log file creation fails (e.g., unwritable directory), logging will silently fail. For installer troubleshooting where logs are important, detecting and surfacing this failure would be beneficial.Optional improvement:
protected function initLogger(string $command, array $args = []): FileLoggerInterface { $logger = $this->getLogger(); $logger->setDir($this->getCwd()); - $logger->open($command, $args); + if (!$logger->open($command, $args)) { + // Optionally throw or log a warning + throw new \RuntimeException(sprintf('Unable to open log file for command "%s".', $command)); + } return $logger; }
380-407: Consider explicit boolean stringification for clarity.Positional boolean arguments currently convert
TRUEto'1'and dropFALSE(lines 386-390). Named boolean options emit only the key whenTRUEand drop whenFALSE(lines 395-400). For logging/display clarity, explicitly stringifying booleans as'true'/'false'might be less surprising.Optional adjustment for positional args:
if (is_int($key)) { // Positional argument. if (is_bool($value)) { - if ($value) { - $formatted[] = '1'; - } + $formatted[] = $value ? 'true' : 'false'; }And for named options:
elseif (is_bool($value)) { // Named argument/option. - if ($value) { - $formatted[] = $key; - } + $formatted[] = $key . '=' . ($value ? 'true' : 'false'); }
230-310: Simplify unused end-of-options state tracking.The
$end_of_options_foundflag (line 243) and its conditional check (lines 277-283) are dead state. The flag is set when--is encountered, but this doesn't change the parsing behavior—both the first and subsequent--tokens are simply added to$parts. The comment references "end-of-options" detection, but no special post-marker behavior is implemented.Consider removing the unused flag:
- $end_of_options_found = FALSE; for ($i = 0; $i < $length; $i++) { ... if (!$in_quotes && ($char === ' ' || $char === "\t")) { if ($current !== '' || $has_content) { - // Check for end-of-options marker (--) only if not already found - // and not inside quotes. - if (!$end_of_options_found && $current === '--') { - $end_of_options_found = TRUE; - // Add the -- marker to the parts array so it reaches the command. - $parts[] = $current; - $current = ''; - $has_content = FALSE; - continue; - } - $parts[] = $current; $current = ''; $has_content = FALSE;Or clarify that the parser preserves
--for higher-level handling without special treatment..vortex/installer/src/Runner/ProcessRunner.php (5)
161-167: Improve environment variable error diagnostics.The exception message is generic and doesn't identify which variable is invalid. Including the key and type would make debugging easier.
Optional improvement:
protected function validateEnvironmentVars(array $env): void { - foreach ($env as $env_value) { - if (!is_scalar($env_value)) { - throw new \InvalidArgumentException('All environment variables must be scalar values.'); + foreach ($env as $name => $value) { + if (!is_scalar($value)) { + throw new \InvalidArgumentException(sprintf( + 'Environment variable "%s" must be a scalar value, got %s.', + (string) $name, + get_debug_type($value), + )); } } }
49-56: Critical: Output accumulation is broken.Line 51 overwrites
$this->outputwith each callback invocation, discarding all previously captured output. For multi-chunk process output, this loses data.Apply this fix:
$process->run(function ($type, string|iterable $buffer) use ($logger, $output): void { $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; - $this->output = $buffer; + $this->output .= $buffer; if ($this->shouldStream) { $output->write($buffer); } $logger->write($buffer); });
104-122: Validate executable paths before use.When a command contains
/(is a path), the code only validates executability for relative paths that resolve viacwd(lines 108-112). For absolute paths,$resolvedis set to$base_commandwithout verifying the file exists or is executable, potentially causing confusing errors at process execution time.Add validation:
// If command is a path (contains /), check if it exists directly. if (str_contains((string) $base_command, '/')) { $resolved = $base_command; // Check relative to cwd if not absolute. if (!str_starts_with((string) $base_command, '/')) { $full_path = $this->getCwd() . '/' . $base_command; if (is_executable($full_path)) { $resolved = $full_path; } } + // Verify the resolved path is executable. + if (!is_executable($resolved)) { + throw new \InvalidArgumentException(sprintf('Command not found or not executable: %s', $base_command)); + } }
141-150: Align prepareArguments() with its docblock promise.The docblock states the method throws
InvalidArgumentExceptionfor non-scalar arguments, but the implementation (lines 144-146) just string-casts everything. This can silently convert arrays/objects into"Array"/"Object(...)", making debugging difficult.Add explicit validation:
protected function prepareArguments(array $parsed_args, array $additional_args): array { $all_args = array_merge($parsed_args, $this->formatArgs($additional_args)); foreach ($all_args as &$arg) { + if (!is_scalar($arg)) { + throw new \InvalidArgumentException(sprintf( + 'All command arguments must be scalar values, got %s.', + get_debug_type($arg), + )); + } $arg = (string) $arg; } unset($arg); return $all_args; }
66-73: Critical: Exit code mapping is immediately overwritten.The
matchstatement (lines 66-71) correctly maps the raw exit code to interface constants (EXIT_SUCCESS,EXIT_FAILURE,EXIT_COMMAND_NOT_FOUND,EXIT_INVALID), but line 73 immediately overwrites$this->exitCodewith the raw$exit_code, completely negating the mapping logic.Remove the overwrite:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - $this->exitCode = $exit_code; return $this; }.vortex/installer/tests/Functional/Command/InstallCommandTest.php (1)
49-75: Runner mock callbacks should accept variadic arguments for robustness.The
run()method callbacks for both$build_runner(line 50) and$check_requirements_runner(line 72) currently declare a single typed parameterfunction (string $command). Ifrun()is invoked with additional arguments (e.g., args array, inputs, environment), the strict signature will cause a type error.Apply this pattern to both callbacks:
$build_runner->method('run') - ->willReturnCallback(function (string $command) use ($build_runner, &$build_runner_command): MockObject { + ->willReturnCallback(function (string $command, ...$args) use ($build_runner, &$build_runner_command): MockObject { $build_runner_command = $command; return $build_runner; });And similarly for
$check_requirements_runner:$check_requirements_runner->method('run') - ->willReturnCallback(function (string $command) use ($check_requirements_runner, &$check_requirements_runner_command): MockObject { + ->willReturnCallback(function (string $command, ...$args) use ($check_requirements_runner, &$check_requirements_runner_command): MockObject { $check_requirements_runner_command = $command; return $check_requirements_runner; });.vortex/installer/tests/Unit/Logger/FileLoggerTest.php (1)
108-144: Consider more explicit no-op verification for unopened logger.The test for
write()when the logger is not open (lines 138-143) only assertsTRUEas a placeholder. While this ensures no errors occur, it doesn't verify the no-op behavior.Optional improvement to explicitly verify no file creation:
else { // When logger is not open, write() should be a no-op. - // We can't directly verify this, but we ensure no errors occur. - // @phpstan-ignore-next-line - $this->assertTrue(TRUE, 'write() should not throw error when logger is not open'); + // Verify no file was created. + $this->assertNull($logger->getPath(), 'No log file should be created when write() is called on unopened logger'); }.vortex/installer/src/Command/CheckRequirementsCommand.php (2)
243-252: Match version command to detected Docker Compose binary.
checkDockerCompose()always callsgetCommandVersion('docker compose version')(line 246), even whendockerComposeExists()falls back to the legacydocker-composebinary. This means version output (and any error text) always comes from the modern command, even if only the legacy binary is present.Adjust to use the appropriate version command:
protected function checkDockerCompose(): bool { - $result = $this->dockerComposeExists(); - if ($result) { - $this->present['Docker Compose'] = $this->getCommandVersion('docker compose version'); + // Try modern plugin first. + $this->processRunner->run('docker compose version'); + if ($this->processRunner->getExitCode() === RunnerInterface::EXIT_SUCCESS) { + $this->present['Docker Compose'] = $this->getCommandVersion('docker compose version'); + return TRUE; + } + + // Fallback to legacy docker-compose binary. + if ($this->commandExists('docker-compose')) { + $this->present['Docker Compose'] = $this->getCommandVersion('docker-compose --version'); + return TRUE; } - else { - $this->missing['Docker Compose'] = 'https://docs.docker.com/compose/install/'; - } - return $result; + + $this->missing['Docker Compose'] = 'https://docs.docker.com/compose/install/'; + return FALSE; }
47-77: Use modern command registration instead of deprecated $defaultName.The
public static $defaultNameproperty (line 52) was deprecated in Symfony 5.1. For Symfony 7.3.6, use the#[AsCommand]attribute.Apply this modernization:
+use Symfony\Component\Console\Attribute\AsCommand; + /** * Check requirements command. */ +#[AsCommand( + name: 'check-requirements', + description: 'Check if required tools are installed and running.' +)] class CheckRequirementsCommand extends Command implements ProcessRunnerAwareInterface, ExecutableFinderAwareInterface { - /** - * Defines default command name. - * - * @var string - */ - public static $defaultName = 'check-requirements';Then remove redundant calls in
configure():protected function configure(): void { - $this->setName('check-requirements'); - $this->setDescription('Check if required tools are installed and running.'); $this->setHelp('Checks for Docker, Docker Compose, Ahoy, and Pygmy.');.vortex/installer/src/Command/InstallCommand.php (6)
32-105: Command DI and new options look good; consider dropping redundantsetName()The switch to
CommandRunnerAwareTrait/ExecutableFinderAwareTraitand the introduction of--destination/--buildoptions are consistent with the new runner infrastructure and PR goals.One small cleanup: with
public static $defaultName = 'install';in place, the explicit$this->setName('install');inconfigure()is redundant and can be removed to keep a single source of truth for the command name.
110-217: Auto-build prompt is not truly opt‑in; defaults to “Run now”The post-install auto-build block currently does:
$should_build = $this->config->get(Config::BUILD_NOW); if (!$should_build && !$this->config->getNoInteraction()) { $should_build = Tui::confirm( label: 'Run the site build now?', default: TRUE, hint: 'Takes ~5-10 min; output will be streamed. You can skip and run later with: ahoy build', ); }With
default: TRUE, pressing Enter triggers a build, which conflicts with the requirement that the step be opt‑in with default focus on Skip.Flip the default:
- if (!$should_build && !$this->config->getNoInteraction()) { - $should_build = Tui::confirm( - label: 'Run the site build now?', - default: TRUE, - hint: 'Takes ~5-10 min; output will be streamed. You can skip and run later with: ahoy build', - ); - } + if (!$should_build && !$this->config->getNoInteraction()) { + $should_build = Tui::confirm( + label: 'Run the site build now?', + default: FALSE, + hint: 'Takes ~5-10 min; output will be streamed. You can skip and run later with: ahoy build', + ); + }Also, please double-check that the CI path always skips this auto-build step (and surfaces the promised “skipped in CI” note) even when
--buildis provided.
249-309: Destination resolution may still lack a guaranteed fallback to current directory
resolveOptions()derives$dstfrom, in order, CLI option, env (VORTEX_INSTALLER_DST), existing config, andConfig::ROOT:$dst_from_option = ...; $dst_from_env = Env::get(Config::DST); $dst_from_config = $this->config->get(Config::DST); $dst_from_root = $this->config->get(Config::ROOT); $dst = $dst_from_option ?: ($dst_from_env ?: ($dst_from_config ?: $dst_from_root)); $dst = File::realpath($dst);If none of these sources are set/non-empty,
$dstwill beNULLandFile::realpath($dst)will fail, while the option help says “Defaults to the current directory.”To make this robust and match the help text, consider:
- $dst = $dst_from_option ?: ($dst_from_env ?: ($dst_from_config ?: $dst_from_root)); - $dst = File::realpath($dst); + $dst = $dst_from_option ?: ($dst_from_env ?: ($dst_from_config ?: $dst_from_root)); + + if (empty($dst)) { + $dst = getcwd() ?: '.'; + } + + $dst = File::realpath($dst);(or throw a clear exception if you prefer failing hard).
377-422: Secure the demo DBcurlcommand to avoid shell injection
prepareDemo()builds a shell command by interpolating$url,$data_dir, and$db_filedirectly into a string passed topassthru():$command = sprintf('curl -s -L "%s" -o "%s/%s"', $url, $data_dir, $db_file); if (passthru($command) === FALSE) { throw new \RuntimeException(...); }Since those values come from environment variables and filesystem state, a crafted value can break out of the quoted context and inject arbitrary shell commands.
Please escape each argument:
- $command = sprintf('curl -s -L "%s" -o "%s/%s"', $url, $data_dir, $db_file); - if (passthru($command) === FALSE) { + $target = $data_dir . DIRECTORY_SEPARATOR . $db_file; + $command = sprintf( + 'curl -s -L %s -o %s', + escapeshellarg($url), + escapeshellarg($target), + ); + + if (passthru($command) === FALSE) { throw new \RuntimeException(sprintf('Unable to download demo database from %s.', $url)); }That keeps behavior while hardening against command injection.
424-442: Use runner exit-code constant for consistency
runBuildCommand()currently compares the runner exit code toCommand::SUCCESS:return $runner->getExitCode() === Command::SUCCESS;Elsewhere you use
RunnerInterface::EXIT_SUCCESS; switching to that here keeps the abstraction consistent and makes it clearer that this is the runner’s process exit code, not the Symfony command’s:- return $runner->getExitCode() === Command::SUCCESS; + return $runner->getExitCode() === \DrevOps\VortexInstaller\Runner\RunnerInterface::EXIT_SUCCESS;
624-645: Cache the lazily createdDownloaderinstance
getDownloader()currently creates a newDownloaderwhenever$this->downloaderisNULL, but does not cache it, so repeated calls yield multiple instances:protected function getDownloader(): Downloader { return $this->downloader ?? new Downloader(); }For typical DI patterns and to preserve test injection via
setDownloader(), cache the instance:protected function getDownloader(): Downloader { - return $this->downloader ?? new Downloader(); + return $this->downloader ??= new Downloader(); }.vortex/installer/src/Command/BuildCommand.php (4)
117-129: Makegetcwd()/.envhandling more explicit (optional)
getProjectMachineName()silently falls back to'.'whengetcwd()fails and tobasename($cwd)when.envis unreadable or missing. If you want clearer failure modes, consider explicitly handlinggetcwd() === FALSEandfile_get_contents() === FALSE(e.g., log or at least comment), instead of masking them via?: '.'. This would ease debugging in odd deployment environments.
134-155: Guard against unsetprocessRunnerin success summary
showSuccessSummary()assumes$this->processRunneris always initialized. If the build task is ever short‑circuited or this method is called from tests/other contexts without a priorrun(),$this->processRunner->getLogger()will fatally error.A small defensive tweak keeps behavior but avoids hard failures:
- $log_path = $this->processRunner->getLogger()->getPath(); - if ($log_path) { + $log_path = $this->processRunner?->getLogger()->getPath(); + if ($log_path !== NULL) { $output .= 'Log file: ' . $log_path . PHP_EOL; $output .= PHP_EOL; }You may want the same pattern in
showFailureSummary().
160-192: Failure summary still has dead type check and omits retry/troubleshooting guidance
getOutput(as_array: TRUE)always returns an array, so the!is_array()guard and exception are unreachable noise. More importantly, the PR spec calls for an explicit retry command and troubleshooting link;TROUBLESHOOTING_URLis defined but not surfaced.Consider:
- // Show last 10 lines of output for context. - $runner_output = $this->processRunner->getOutput(as_array: TRUE); - - if (!is_array($runner_output)) { - throw new \RuntimeException('Runner output is not an array.'); - } - - $last_lines = array_slice($runner_output, -10); + // Show last 10 lines of output for context. + /** @var array<string> $runner_output */ + $runner_output = $this->processRunner->getOutput(as_array: TRUE); + $last_lines = array_slice($runner_output, -10); @@ - foreach ($last_lines as $last_line) { - Tui::line(' ' . $last_line); - } - } + foreach ($last_lines as $last_line) { + Tui::line(' ' . $last_line); + } + } + + Tui::line(''); + Tui::line('Retry with: ahoy build'); + Tui::line('Troubleshooting: ' . self::TROUBLESHOOTING_URL); }That matches the documented behavior and uses the existing constant.
22-54: Prefer attribute-based command registration and avoid redundantsetName()With Symfony 7 you can register the command via
#[AsCommand]and drop bothpublic static $defaultNameand the explicitsetName('build')to avoid deprecated patterns and duplication.Example:
+use Symfony\Component\Console\Attribute\AsCommand; @@ -/** - * Build command. - */ -class BuildCommand extends Command implements ProcessRunnerAwareInterface, CommandRunnerAwareInterface { +#[AsCommand(name: 'build', description: 'Build the site using ahoy build.')] +class BuildCommand extends Command implements ProcessRunnerAwareInterface, CommandRunnerAwareInterface { @@ - /** - * Defines default command name. - * - * @var string - */ - public static $defaultName = 'build'; @@ - protected function configure(): void { - $this->setName('build'); - $this->setDescription('Build the site using ahoy build.'); + protected function configure(): void { + $this->setDescription('Build the site using ahoy build.');
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php
Show resolved
Hide resolved
AlexSkrypnyk
changed the title
…NSTALLER_PROMPT_BUILD_NOW.
|
@coderabbitai resolve all |
|
@AlexSkrypnyk: I'll resolve all review comments on this PR. ✅ Actions performedComments resolved. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (2)
249-270: Pygmy status callbacks may not match actual implementation.Per the past review comment,
checkPygmy()only executespygmy version, notpygmy status. Theexit_code_callbackbranches checking forstr_contains($current_command, 'pygmy status')may be dead code paths. Consider either:
- Renaming scenarios to reflect actual behavior (e.g., "Pygmy version succeeds")
- Updating
checkPygmy()to also checkpygmy statusif that's the intended behavior#!/bin/bash # Verify what commands checkPygmy() actually executes rg -n 'pygmy' .vortex/installer/src/Command/CheckRequirementsCommand.php -A 5 -B 2
364-384: Expected output assertions still missingpygmyin available requirements list.The past review comment indicated this was addressed, but lines 371 and 382 still show
'Available: docker, docker-compose, ahoy'withoutpygmy. According to TuiOutput helper context,CHECK_REQUIREMENTS_AVAILABLEshould include all four requirements.'Invalid requirement name' => [ 'exit_code_callback' => fn(string $current_command): int => RunnerInterface::EXIT_SUCCESS, 'command_inputs' => ['--only' => 'invalid'], 'expect_failure' => TRUE, 'output_assertions' => [ '* ' . TuiOutput::CHECK_REQUIREMENTS_UNKNOWN . ' invalid', - '* Available: docker, docker-compose, ahoy', + '* Available: docker, docker-compose, ahoy, pygmy', ], ], 'Mixed valid and invalid requirements' => [ 'exit_code_callback' => fn(string $current_command): int => RunnerInterface::EXIT_SUCCESS, 'command_inputs' => ['--only' => 'docker,invalid'], 'expect_failure' => TRUE, 'output_assertions' => [ '* ' . TuiOutput::CHECK_REQUIREMENTS_UNKNOWN . ' invalid', - '* Available: docker, docker-compose, ahoy', + '* Available: docker, docker-compose, ahoy, pygmy', ], ],.vortex/installer/src/Command/InstallCommand.php (3)
410-437: Critical: Shell injection vulnerability in curl command.
prepareDemo()builds a shell command on line 428 with interpolated environment variables ($url,$data_dir, and$db_file) fromVORTEX_DB_DOWNLOAD_URL,VORTEX_DB_DIR, andVORTEX_DB_FILE. Since these are untrusted sources, shell metacharacters can break out of the quoted string and inject arbitrary commands.Use
escapeshellarg()to properly escape each argument:- $command = sprintf('curl -s -L "%s" -o "%s/%s"', $url, $data_dir, $db_file); + $target = $data_dir . DIRECTORY_SEPARATOR . $db_file; + $command = sprintf( + 'curl -s -L %s -o %s', + escapeshellarg($url), + escapeshellarg($target), + ); + if (passthru($command) === FALSE) { throw new \RuntimeException(sprintf('Unable to download demo database from %s.', $url)); }
86-86: Remove redundant setName() call.The command name is already defined via the
$defaultNameproperty on line 65. This explicitsetName('install')call is redundant.Apply this diff:
- $this->setName('install'); $this->setDescription('Install Vortex from remote or local repository.');
282-290: Guard destination resolution against NULL.
$dstis derived from four sources (option/env/config/root) at line 288. If all four are unset,$dstbecomesNULL, which will causeFile::realpath($dst)on line 289 to fail. The option's help text promises "Defaults to the current directory," but no such fallback is implemented.Consider explicitly handling the missing destination case:
$dst = $dst_from_option ?: ($dst_from_env ?: ($dst_from_config ?: $dst_from_root)); + + // Fallback to current directory if no destination provided. + if (empty($dst)) { + $dst = getcwd(); + if ($dst === FALSE) { + throw new \RuntimeException('Unable to determine destination directory.'); + } + } + $dst = File::realpath($dst); $this->config->set(Config::DST, $dst, TRUE);.vortex/installer/src/Runner/ProcessRunner.php (3)
49-56: Critical: Output is still being overwritten instead of accumulated.Despite the past review comment being marked as addressed, line 51 still uses assignment (
=) rather than append (+=), causing each callback invocation to discard all previously captured output. For multi-chunk process output, only the final chunk will be retained.Apply this diff to accumulate output:
$process->run(function ($type, string|iterable $buffer) use ($logger, $output): void { $buffer = is_iterable($buffer) ? implode("\n", (array) $buffer) : $buffer; - $this->output = $buffer; + $this->output .= $buffer; if ($this->shouldStream) { $output->write($buffer); } $logger->write($buffer); });
66-73: Critical: Exit code mapping is immediately overwritten.The
matchstatement on lines 66-70 correctly maps raw exit codes to interface constants, but line 73 immediately overwrites$this->exitCodewith the raw$exit_code, completely negating the mapping logic. This breaks the exit code abstraction layer.Apply this diff to preserve the mapped exit code:
match ($exit_code) { Command::SUCCESS => $this->exitCode = self::EXIT_SUCCESS, Command::FAILURE => $this->exitCode = self::EXIT_FAILURE, 127 => $this->exitCode = self::EXIT_COMMAND_NOT_FOUND, default => $this->exitCode = self::EXIT_INVALID, }; - - $this->exitCode = $exit_code; return $this;
104-124: Path-based commands may not be validated for existence.When a command contains
/(is a path), the code validates executability only for relative paths that exist in cwd (lines 108-112). For absolute paths or relative paths that don't resolve via cwd,$resolvedis set to$base_commandwithout verifying the file exists or is executable. This could lead to confusing errors at process execution time instead of a clear early failure.Consider adding validation:
// If command is a path (contains /), check if it exists directly. if (str_contains((string) $base_command, '/')) { $resolved = $base_command; // Check relative to cwd if not absolute. if (!str_starts_with((string) $base_command, '/')) { $full_path = $this->getCwd() . '/' . $base_command; if (is_executable($full_path)) { $resolved = $full_path; } } + // Verify the resolved path is executable. + if (!is_executable($resolved)) { + throw new \InvalidArgumentException(sprintf('Command not found or not executable: %s', $base_command)); + } }.vortex/installer/src/Runner/AbstractRunner.php (3)
73-79: Logger initialization ignores open() failures.
initLogger()ignores the return value of$logger->open($command, $args)on line 76, so a failure to create the log file (e.g., unwritable directory) will be completely silent while callers assume logging is active. For troubleshooting installer failures, detecting this is important.Consider handling failures explicitly:
protected function initLogger(string $command, array $args = []): FileLoggerInterface { $logger = $this->getLogger(); $logger->setDir($this->getCwd()); - $logger->open($command, $args); + if (!$logger->open($command, $args)) { + throw new \RuntimeException(sprintf('Unable to open log file for command "%s".', $command)); + } return $logger; }
230-310: Optional: parseCommand has unused state tracking.
$end_of_options_foundis declared on line 243 and set on line 278, but is only used to avoid treating a second--specially. Since the parser doesn't otherwise distinguish between options vs. positionals, the flag doesn't affect the returned$partsat all—both first and subsequent--tokens are added as regular parts.Consider removing the unused flag:
$parts = []; $current = ''; $in_quotes = FALSE; $quote_char = ''; $escaped = FALSE; $length = strlen($command); $has_content = FALSE; - $end_of_options_found = FALSE; for ($i = 0; $i < $length; $i++) { $char = $command[$i]; // ... existing code ... if (!$in_quotes && ($char === ' ' || $char === "\t")) { if ($current !== '' || $has_content) { - // Check for end-of-options marker (--) only if not already found - // and not inside quotes. - if (!$end_of_options_found && $current === '--') { - $end_of_options_found = TRUE; - // Add the -- marker to the parts array so it reaches the command. - $parts[] = $current; - $current = ''; - $has_content = FALSE; - continue; - } - $parts[] = $current; $current = ''; $has_content = FALSE; } continue; }
380-407: Optional: Boolean arguments could be more explicit.Positional boolean arguments currently collapse to
'1'(line 388) andFALSEis dropped, while named boolean options emit only the key when true (line 398) or nothing when false. For logging and debugging, explicit'true'/'false'strings would be clearer than implicit conversions.Consider explicit boolean stringification:
foreach ($args as $key => $value) { if (is_int($key)) { // Positional argument. if (is_bool($value)) { - if ($value) { - $formatted[] = '1'; - } + $formatted[] = $value ? 'true' : 'false'; } else { $formatted[] = (string) $value; } } elseif (is_bool($value)) { // Named argument/option. - if ($value) { - $formatted[] = $key; - } + $formatted[] = $key . '=' . ($value ? 'true' : 'false'); } else { $formatted[] = $key . '=' . $value; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.vortex/installer/src/Command/InstallCommand.php(14 hunks).vortex/installer/src/Runner/AbstractRunner.php(1 hunks).vortex/installer/src/Runner/CommandRunnerAwareTrait.php(1 hunks).vortex/installer/src/Runner/ProcessRunner.php(1 hunks).vortex/installer/src/Runner/ProcessRunnerAwareTrait.php(1 hunks).vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php(1 hunks).vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php(1 hunks).vortex/tests/phpunit/Traits/SutTrait.php(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Functional/Command/CheckRequirementsCommandTest.php.vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
.vortex/installer/src/Command/InstallCommand.php
🧬 Code graph analysis (5)
.vortex/installer/src/Runner/ProcessRunnerAwareTrait.php (3)
.vortex/installer/src/Runner/ProcessRunner.php (1)
ProcessRunner(14-174).vortex/installer/src/Runner/ProcessRunnerAwareInterface.php (2)
getProcessRunner(18-18)setProcessRunner(26-26).vortex/installer/src/Command/CheckRequirementsCommand.php (1)
getProcessRunner(334-336)
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (8)
.vortex/installer/src/Command/CheckRequirementsCommand.php (1)
CheckRequirementsCommand(23-338).vortex/installer/src/Runner/ProcessRunner.php (1)
ProcessRunner(14-174).vortex/installer/tests/Functional/FunctionalTestCase.php (1)
FunctionalTestCase(23-145).vortex/installer/tests/Helpers/TuiOutput.php (3)
TuiOutput(7-312)present(171-173)absent(184-186).vortex/installer/src/Runner/ExecutableFinderAwareInterface.php (1)
setExecutableFinder(28-28).vortex/installer/src/Runner/ExecutableFinderAwareTrait.php (1)
setExecutableFinder(42-44).vortex/installer/src/Runner/ProcessRunnerAwareTrait.php (1)
setProcessRunner(37-39).vortex/installer/src/Runner/ProcessRunnerAwareInterface.php (1)
setProcessRunner(26-26)
.vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (4)
.vortex/installer/src/Runner/ProcessRunner.php (5)
ProcessRunner(14-174)run(21-76)resolveCommand(90-125)prepareArguments(141-154)validateEnvironmentVars(165-172).vortex/installer/tests/Unit/Runner/AbstractRunnerTest.php (1)
run(656-660).vortex/installer/src/Runner/AbstractRunner.php (3)
setCwd(97-102)getExitCode(177-183)getOutput(188-200).vortex/installer/src/Runner/CommandRunner.php (1)
run(34-72)
.vortex/tests/phpunit/Traits/SutTrait.php (1)
.vortex/tests/phpunit/Traits/ProcessTrait.php (1)
cmd(59-80)
.vortex/installer/src/Runner/AbstractRunner.php (3)
.vortex/installer/src/Utils/Tui.php (2)
Tui(17-284)output(40-45).vortex/installer/src/Logger/FileLoggerInterface.php (1)
setDir(29-29).vortex/installer/src/Runner/RunnerInterface.php (5)
getCwd(86-86)setCwd(78-78)getCommand(46-46)getExitCode(54-54)getOutput(67-67)
⏰ 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 (4)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-docs
🔇 Additional comments (8)
.vortex/installer/tests/Functional/Command/CheckRequirementsCommandTest.php (1)
23-72: LGTM!The test setup with mocked
ExecutableFinderandProcessRunneris well-structured. The DI injection via setters enables clean testing, and the callback-based exit code simulation allows flexible scenario modeling..vortex/installer/src/Runner/CommandRunnerAwareTrait.php (1)
25-29: LGTM - Lazy initialization properly caches the instance.The use of
??=operator correctly addresses the previous review feedback by caching the auto-createdCommandRunnerinstance. This ensures configuration applied to the runner persists across calls..vortex/tests/phpunit/Traits/SutTrait.php (2)
77-82: LGTM - Shell escaping properly applied.The use of
escapeshellarg()for the--destinationand--working-dirpaths addresses the previous review feedback about handling paths with spaces or special characters. The@todocomment appropriately documents the deferred refactoring.
137-144: LGTM - Consistent shell escaping in buildInstaller.All path arguments to composer and PHP commands are properly escaped with
escapeshellarg(), ensuring robustness when paths contain spaces or shell metacharacters..vortex/installer/src/Runner/ProcessRunnerAwareTrait.php (1)
25-27: LGTM - Lazy initialization properly caches the instance.The use of
??=operator correctly addresses the previous review feedback by caching the auto-createdProcessRunnerinstance, consistent with the pattern inCommandRunnerAwareTrait..vortex/installer/tests/Unit/Runner/ProcessRunnerTest.php (3)
54-79: LGTM - Variable shadowing addressed.The variable name
$runner_outputon line 77 correctly addresses the previous review comment about shadowing the$outputvariable.
138-141: LGTM - Placeholder assertion replaced correctly.Using
$this->addToAssertionCount(1)is the appropriate PHPUnit idiom for acknowledging that reaching this point without exception is the success condition, as suggested in the previous review.
380-406: LGTM - TestableProcessRunner helper class is well-designed.The helper class cleanly exposes protected methods for testing without modifying production code. This is a standard and effective pattern for unit testing internal behaviors.
AlexSkrypnyk
force-pushed
the
feature/1950-auto-build
branch
from
e7795ba to
6670d67
Compare
Closes #1950
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.