[#2843] Suppressed Composer output of the initial tooling install and adopted the standard progress helpers. - #2858
Conversation
WalkthroughThe bootstrap script centralizes Composer execution to suppress successful output and replay failures. Functional tests cover installation, short-circuit, and dependency-resolution failure output, including temporary-directory cleanup. ChangesTooling bootstrap output
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BootstrapTest
participant VortexTooling
participant composer_run
participant Composer
BootstrapTest->>VortexTooling: Run bootstrap with controlled environment
VortexTooling->>composer_run: Configure and install tooling
composer_run->>Composer: Execute Composer
Composer-->>composer_run: Return output and status
composer_run-->>VortexTooling: Suppress success or replay failure output
VortexTooling-->>BootstrapTest: Return bootstrap result
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.vortex/tests/phpunit/Functional/ToolingBootstrapTest.php:
- Line 165: Rename the prepareProject parameter from $tooling_constraint to
$toolingConstraint, and update the corresponding named argument at the caller
and the parameter usage within prepareProject while preserving existing
behavior.
- Around line 69-83: The assertions around the ToolingBootstrapTest invocation
must verify exact silent output rather than only absence of selected strings: at
.vortex/tests/phpunit/Functional/ToolingBootstrapTest.php lines 69-83, assert
that the non-short-circuit path has only the installation announcement on stdout
and empty stderr, while the short-circuit path has empty stdout and stderr; at
line 109, capture the repeated invocation’s returned process and assert both
output streams are empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ae26c43-c046-4720-b665-b29c62fce9f2
⛔ Files ignored due to path filters (1)
.vortex/installer/tests/Fixtures/handler_process/_baseline/scripts/vortex-tooling.shis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (2)
.vortex/tests/phpunit/Functional/ToolingBootstrapTest.phpscripts/vortex-tooling.sh
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2858 +/- ##
==========================================
- Coverage 86.91% 86.39% -0.52%
==========================================
Files 100 93 -7
Lines 4830 4683 -147
Branches 47 3 -44
==========================================
- Hits 4198 4046 -152
- Misses 632 637 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
📖 Documentation preview for this pull request has been deployed to Netlify: https://6a699108754f21a0c63fdea6--vortex-docs.netlify.app This preview is rebuilt on every commit and is not the production documentation site. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlexSkrypnyk
changed the title
AlexSkrypnyk
added
the
Needs review
The ahoy entrypoint bootstraps 'drevops/vortex-tooling' before every command, so on a fresh clone ten lines of Composer progress preceded the requested command. The 'No composer.lock file present' warning refers to the throwaway 'vendor-temp/' project and read as a problem with the user's site. Every mutating Composer call now runs through a helper that captures its output and replays it on stderr only when the command fails, preserving Composer's exit code. 'VORTEX_DEBUG=1' still streams the output as it happens. A single '[INFO]' line announces the install so a fresh clone does not sit silent. 'composer --quiet' was not used: it also suppresses failure messages, and a failing bootstrap aborts the ahoy entrypoint and blocks every command.
The short-circuit path exits before any Composer call, so asserting the absence of Composer output there duplicated the assertion that the announcement line is not printed.
…ring probes. Negative substring checks still passed when the bootstrap emitted unanticipated output, so they did not actually prove the output suppression or the silent short-circuit.
The bootstrap printed a hand-rolled announcement line and reported nothing on completion, unlike every other shipped script. It now defines the same 'info'/'note'/'task'/'pass'/'fail' block and brackets the install with a started message and a closing '[ OK ]' line, with '[FAIL]' labelling the replayed Composer output. The output assertions pin 'TERM' so the colour branch of the helpers is deterministic across environments.
3bb7e24 to
21e105a
Compare
AlexSkrypnyk
added
the
AUTOMERGE
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.vortex/tests/phpunit/Functional/ToolingBootstrapTest.php:
- Line 30: Update the OUTPUT_ENV constant in ToolingBootstrapTest to include
VORTEX_DEBUG set to 0, preserving the existing SHELL_VERBOSITY and TERM values
so the merged process environment consistently disables debug output.
In `@scripts/vortex-tooling.sh`:
- Around line 38-42: Update composer_run so the VORTEX_DEBUG=1 streaming
Composer invocation is handled through an if/else rather than exiting directly
under set -e; on failure, call fail "Composer command failed." and return
Composer’s original status, while preserving successful execution behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 068cf2a0-4854-427f-8a1f-e39d82e83366
⛔ Files ignored due to path filters (1)
.vortex/installer/tests/Fixtures/handler_process/_baseline/scripts/vortex-tooling.shis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (2)
.vortex/tests/phpunit/Functional/ToolingBootstrapTest.phpscripts/vortex-tooling.sh
| * regardless of the script's behaviour, and the progress helpers emit ANSI | ||
| * colour whenever the terminal reports support for it. | ||
| */ | ||
| protected const OUTPUT_ENV = ['SHELL_VERBOSITY' => 0, 'TERM' => 'dumb']; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files '.vortex/tests/phpunit/Functional/ToolingBootstrapTest.php' 'scripts/vortex-tooling.sh' | cat
printf '\n== outline: ToolingBootstrapTest ==\n'
ast-grep outline .vortex/tests/phpunit/Functional/ToolingBootstrapTest.php --view expanded || true
printf '\n== outline: vortex-tooling.sh ==\n'
ast-grep outline scripts/vortex-tooling.sh --view expanded || true
printf '\n== relevant snippets ==\n'
nl -ba .vortex/tests/phpunit/Functional/ToolingBootstrapTest.php | sed -n '1,180p'
printf '\n--- scripts/vortex-tooling.sh ---\n'
nl -ba scripts/vortex-tooling.sh | sed -n '1,220p'
printf '\n== search for VORTEX_DEBUG and env merging ==\n'
rg -n "VORTEX_DEBUG|OUTPUT_ENV|env override|merge.*env|environment" .vortex tests scripts -g '!**/vendor/**' || trueRepository: drevops/vortex
Length of output: 1040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files '.vortex/tests/phpunit/Functional/ToolingBootstrapTest.php' 'scripts/vortex-tooling.sh'Repository: drevops/vortex
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwdRepository: drevops/vortex
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ToolingBootstrapTest excerpt ==\n'
sed -n '1,260p' .vortex/tests/phpunit/Functional/ToolingBootstrapTest.php
printf '\n== vortex-tooling.sh ==\n'
sed -n '1,220p' scripts/vortex-tooling.sh
printf '\n== search for command helper and env merging ==\n'
rg -n "proc_open|passthru|shell_exec|exec\(|env\s*=|OUTPUT_ENV|VORTEX_DEBUG|SHELL_VERBOSITY|TERM|putenv|inherit.*env|merge.*env" .vortex tests scripts -g '!**/vendor/**' || trueRepository: drevops/vortex
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate FunctionalTestCase ==\n'
fd -a 'FunctionalTestCase.php' . || true
printf '\n== outline candidate test base classes ==\n'
for f in $(fd -a 'FunctionalTestCase.php' .); do
echo "--- $f ---"
ast-grep outline "$f" --view expanded || true
done
printf '\n== search for cmd / cmdFail implementation ==\n'
rg -n "function cmdFail|function cmd\(|protected function cmd|public function cmd|cmdFail\(" .vortex tests -g '!**/vendor/**' || true
printf '\n== likely helper file excerpts ==\n'
for f in $(rg -l "function cmdFail|function cmd\(" .vortex tests -g '!**/vendor/**' | head -n 20); do
echo "--- $f ---"
nl -ba "$f" | sed -n '1,260p'
doneRepository: drevops/vortex
Length of output: 3545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ProcessTrait ==\n'
sed -n '1,220p' .vortex/tests/phpunit/Traits/ProcessTrait.php
printf '\n== related docs ==\n'
sed -n '1,120p' .vortex/tests/CLAUDE.md
printf '\n== search for env handling in process helpers ==\n'
rg -n "env:|array_merge|+ \\\$env|Process|setEnv|putenv|inherit" .vortex/tests/phpunit/Traits .vortex/tests/phpunit/Functional -g '!**/vendor/**' || trueRepository: drevops/vortex
Length of output: 5686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' .vortex/tests/phpunit/Traits/ProcessTrait.phpRepository: drevops/vortex
Length of output: 3044
🌐 Web query:
Symfony Process env array merges inherited environment variables or replaces them; unset variable behavior and defaults
💡 Result:
In the Symfony Process component, environment variables are inherited by default [1][2]. When you provide an array of environment variables to a Process, these variables are merged with the system's existing environment variables [1]. Environment Variable Merging and Replacement When you pass an array of environment variables to a Process (e.g., via the constructor or the run/start methods), these explicitly provided values take precedence over the inherited system environment variables [1][3]. If a key in your provided array matches an existing environment variable, the inherited value is effectively replaced by the one you provided [1][3]. Unsetting Variables To prevent a specific inherited environment variable from being passed to the child process, you can explicitly set that variable's value to false in the environment array [1][4]. For example: $process = new Process(['command'], null, [ 'UNWANTED_VAR' => false, ]); This signals the component to exclude the variable from the environment block passed to the underlying system process [1]. Important Considerations - Inheritance is mandatory: The option to disable inheritance entirely was deprecated and removed in earlier versions (around Symfony 4.0), meaning processes now always inherit system environment variables [2][5]. - Value Types: Ensure that the values in your environment array are strings or types that can be cast to strings. Passing arrays as values within the environment array can lead to errors such as "Array to string conversion" when the component processes them [6][7]. - Implementation Details: Internally, the component performs a merge between the system's inherited environment (collected via getenv and $_SERVER/$_ENV) and your provided array [3][8]. When unsetting via false, the internal logic filters or diffs these arrays to ensure the excluded variables are not passed to the final execution command [3].
Citations:
- 1: https://symfony.com/doc/current/components/process.html
- 2: https://github.com/symfony/process/blob/master/CHANGELOG.md
- 3: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Process/Process.php
- 4: https://symfony.com/doc/7.0/components/process.html
- 5: [Process] Deprecate Process::inheritEnvironmentVariables() symfony/symfony#32475
- 6: [Process] arrays in
$envresult inArray to string conversionsymfony/symfony#44940 - 7: [Process] always manually inherit $_SERVER symfony/symfony#8067
- 8: https://github.com/symfony/process/blob/8.1/Process.php
🌐 Web query:
Symfony\Component\Process\Process constructor env parameter inherit current environment documentation
💡 Result:
In Symfony's Process component, processes launched via the Process class automatically inherit all environment variables defined in the current system environment [1][2][3]. When providing an environment variable array to the Process constructor via the env parameter, these variables are merged with the existing environment [1][2]. If you need to prevent a specific environment variable from being inherited by the child process, you can explicitly set that variable to false within the env array provided to the constructor [1][2][4]. Example of passing custom variables while disabling an inherited one: $process = new Process(['command'], null, [ 'CUSTOM_VAR' => 'value', 'APP_ENV' => false, // Prevents inheritance of APP_ENV ]); If the env parameter is passed as null to the constructor, the child process will simply inherit the full environment of the current PHP process [5][6][7].
Citations:
- 1: https://symfony.com/doc/7.4/components/process.html
- 2: https://symfony.com/doc/7.0/components/process.html
- 3: https://symfony.com/doc/5.x/components/process.html
- 4: https://symfony.com/doc/4.x/components/process.html
- 5: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Process/Process.php
- 6: https://github.com/symfony/symfony/blob/8.0/src/Symfony/Component/Process/Process.php
- 7: https://github.com/symfony/process/blob/8.1/Process.php
🌐 Web query:
Symfony Process env parameter inherit or replace current environment docs
💡 Result:
In the Symfony Process component, environment variables defined in the system are always inherited by default [1][2]. The legacy method inheritEnvironmentVariables() has been removed and is no longer available [1][2]. To manage environment variables when running a process: 1. Inherit and Extend: Any environment variables passed to the Process constructor or execution methods (like run(), mustRun(), or start()) are merged with the system's environment variables [3][4][2]. 2. Remove/Disable Specific Variables: If you need to prevent the process from inheriting specific system environment variables, you can explicitly remove them by setting those keys to false in the environment array provided to the process [3][5][6]. Example of removing environment variables: $process = new Process(['your_command'], null, [ 'APP_ENV' => false, 'UNWANTED_VAR' => false, ]); $process->run; By setting a variable to false, the Process component treats it as removed from the environment block passed to the underlying process execution [3][4].
Citations:
- 1: [Process] Deprecate Process::inheritEnvironmentVariables() symfony/symfony#32475
- 2: https://github.com/symfony/process/blob/master/CHANGELOG.md
- 3: https://symfony.com/doc/current/components/process.html
- 4: https://symfony.com/doc/7.0/components/process.html
- 5: https://symfony.com/doc/6.4/components/process.html
- 6: https://symfony.com/doc/7.4/components/process.html
🌐 Web query:
Symfony\Component\Process\Process environment variables false unset inherit current env
💡 Result:
In the Symfony Process component, environment variables are inherited from the current system environment by default [1][2][3]. There is no configuration option to completely disable this inheritance [2][3]. To prevent specific environment variables from being inherited by the process you are executing, you must explicitly set them to false in the environment variable array provided to the Process constructor or its execution methods [1][4]. Example: $process = new Process(['your_command'], null, [ 'VAR_TO_UNSET' => false, 'ANOTHER_VAR' => false, ]); $process->run; This mechanism allows you to maintain the necessary system environment (such as PATH or other critical variables) while ensuring that specific variables from the parent process do not leak into the child process [1][5]. The older method, Process::inheritEnvironmentVariables, has been removed [3].
Citations:
- 1: https://symfony.com/doc/current/components/process.html
- 2: [Process] Deprecate Process::inheritEnvironmentVariables() symfony/symfony#32475
- 3: https://github.com/symfony/process/blob/master/CHANGELOG.md
- 4: https://symfony.com/doc/7.0/components/process.html
- 5: [Process] Be able to not inherit ENV var symfony/symfony#24397
Set VORTEX_DEBUG=0 in OUTPUT_ENV
ProcessTrait merges this array with the inherited process env, so a parent VORTEX_DEBUG=1 still reaches scripts/vortex-tooling.sh and can change the Composer output these assertions depend on. .vortex/tests/phpunit/Functional/ToolingBootstrapTest.php:30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.vortex/tests/phpunit/Functional/ToolingBootstrapTest.php at line 30, Update
the OUTPUT_ENV constant in ToolingBootstrapTest to include VORTEX_DEBUG set to
0, preserving the existing SHELL_VERBOSITY and TERM values so the merged process
environment consistently disables debug output.
| composer_run() { | ||
| if [ "${VORTEX_DEBUG-}" = "1" ]; then | ||
| composer "$@" | ||
| return | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Emit the standard failure marker in debug mode.
When VORTEX_DEBUG=1, a failed Composer command exits through set -e before reaching fail "Composer command failed." at Line [48]. Debug failures therefore have a different failure contract and omit the standard [FAIL] message. Handle the streaming call in an if/else, emit fail, and return Composer’s original status.
Proposed fix
composer_run() {
+ local output status=0
if [ "${VORTEX_DEBUG-}" = "1" ]; then
- composer "$@"
- return
+ if composer "$@"; then
+ return 0
+ else
+ status=$?
+ fail "Composer command failed."
+ return "${status}"
+ fi
fi
- local output status=0
output=$(composer "$@" 2>&1) || status=$?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| composer_run() { | |
| if [ "${VORTEX_DEBUG-}" = "1" ]; then | |
| composer "$@" | |
| return | |
| fi | |
| composer_run() { | |
| local output status=0 | |
| if [ "${VORTEX_DEBUG-}" = "1" ]; then | |
| if composer "$@"; then | |
| return 0 | |
| else | |
| status=$? | |
| fail "Composer command failed." | |
| return "${status}" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/vortex-tooling.sh` around lines 38 - 42, Update composer_run so the
VORTEX_DEBUG=1 streaming Composer invocation is handled through an if/else
rather than exiting directly under set -e; on failure, call fail "Composer
command failed." and return Composer’s original status, while preserving
successful execution behavior.
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
Code coverage (threshold: 90%) Per-class coverage |
1 participant
Closes #2843
Summary
The
ahoyentrypoint bootstrapsdrevops/vortex-toolingintovendor/before every command, so on a fresh clone the firstahoy <anything>was preceded by ten lines of raw Composer progress output before the requested command produced a single line of its own. The highlightedNo composer.lock file presentwarning was the most misleading part: it refers to the throwawayvendor-temp/project the bootstrap creates, not to the user's site, so it read as a problem with their clone.Every mutating Composer call in
scripts/vortex-tooling.shnow runs through a helper that captures its output and replays it on stderr only when the command fails, preserving Composer's own exit code rather than flattening it.VORTEX_DEBUG=1still streams the output as it happens, matching theset -xbehaviour already in the script.composer --quietwas deliberately not used. It also suppresses failure messages, and because this bootstrap runs from theahoyentrypoint, a silent failure would abort every command including the ones needed to recover.Changes
scripts/vortex-tooling.shinfo,note,task,pass,fail) used by the shipped tooling scripts, replacing the absence of any progress reporting in this script.composer_run()helper that streams Composer directly underVORTEX_DEBUG=1and otherwise captures stdout and stderr, writing them to stderr and returning Composer's exit code only when the command fails.configwrites,require --no-update, andinstall. The read-onlycomposer showandcomposer configlookups keep their existing capture, since their output is consumed rather than displayed.info "Started Vortex tooling installation."and a closingpassline, both emitted only on runs that actually install. The early-exit guard means every subsequent command prints nothing at all.failso a failure report is not an unexplained blob..vortex/tests/phpunit/Functional/ToolingBootstrapTest.phptestBootstrapnow matches both output streams in full rather than probing for individual Composer strings, since unanticipated output would slip past a list of negative substring checks. The installing path asserts stdout is exactly the two progress messages and stderr is empty; the short-circuit path asserts both streams are empty, as does the repeated short-circuited run.testBootstrapReplaysComposerOutputOnFailure, which forces an unresolvable tooling constraint and asserts the failure is reported, the suppressed Composer output still reaches the terminal, andvendor-temp/is cleaned up. This is the assertion that pins the design choice against a future simplification tocomposer --quiet.OUTPUT_ENVconstant used by every output assertion. It restores Composer's default verbosity, which the harness otherwise suppresses throughSHELL_VERBOSITY(without it the assertions would hold regardless of the script's behaviour), and pinsTERMso the progress helpers' colour branch is deterministic across environments.prepareProject()with an optional tooling-constraint override used by the new failure test.Generated
.vortex/installer/tests/Fixtures/handler_process/_baseline/scripts/vortex-tooling.shis the regenerated installer snapshot of the same script, with theVORTEX_DEVfenced block stripped as the installer does for consumer sites. Produced byahoy update-snapshots, not hand-edited.Before / After
Summary by CodeRabbit
New Features
Bug Fixes
Tests