Added confirmation to ahoy provision.
#1846
Conversation
|
""" WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2).vortex/tests/phpunit/Functional/FunctionalTestCase.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information .vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/web/themes/custom/star_wars/-yarn.lockis excluded by!**/*.lock,!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (1)
.ahoy.yml(3 hunks)
⏰ 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). (12)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-docs
🔇 Additional comments (2)
.ahoy.yml (2)
145-147: Confirmation step added correctly – looks good.The extra
ahoy confirmguard is correctly chained with&&, so the provisioning script will only run when the user explicitly approves (or automation injectsAHOY_CONFIRM_RESPONSE=y).
No issues spotted here.
214-214: Single-line simplification is fine.Inlining the Gherkin-lint command improves readability without changing behaviour.
|
|
||
| # Internal utility command to confirm actions before running them. | ||
| # Accepts a message as an argument and waits for user confirmation. | ||
| # Will proceed only if the user types 'y' or 'true'. | ||
| # If AHOY_CONFIRM_RESPONSE is set, it will use that value without prompting | ||
| # the user, and will print the prompt with an answer. It will also give a | ||
| # 3 seconds delay before proceeding to allow the user to cancel the operation. | ||
| # If AHOY_CONFIRM_WAIT_SKIP is also set to '1', it will skip the wait and | ||
| # proceed immediately. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Docs list accepted answers, but code later ignores incorrect presets.
The comment says the command “will proceed only if the user types y or true”.
That is true for interactive input (validated below), but not when AHOY_CONFIRM_RESPONSE
is pre-set: any value (including n/false) will currently bypass validation.
This mismatch can lead to destructive operations running in CI or scripts that were
meant to abort.
See next comment for a concrete fix.
🤖 Prompt for AI Agents
In the .ahoy.yml file around lines 283 to 291, the documentation states that the
command proceeds only if the user inputs 'y' or 'true', but when the
AHOY_CONFIRM_RESPONSE environment variable is set, the code bypasses this
validation and proceeds regardless of the value. To fix this, modify the code to
validate the AHOY_CONFIRM_RESPONSE value against the accepted answers ('y' or
'true') before proceeding, ensuring it aborts if the value is not one of these
accepted inputs, thus aligning the behavior with the documented expectations.
| if [ -z "${AHOY_CONFIRM_RESPONSE}" ]; then | ||
| read -r -p ">> $1 [y/N] " AHOY_CONFIRM_RESPONSE | ||
| [ "${AHOY_CONFIRM_RESPONSE}" = "y" ] || [ "${AHOY_CONFIRM_RESPONSE}" = true ] || ( echo "The operation was canceled." && exit 1 ) | ||
| if [ "${AHOY_CONFIRM_RESPONSE}" != "y" ] && [ "${AHOY_CONFIRM_RESPONSE}" != "true" ]; then | ||
| echo "The operation was canceled." | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}" \ | ||
| && if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then echo "Waiting for 3 seconds... Press Ctrl+C to cancel."; sleep 3; fi | ||
| if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then | ||
| echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}" | ||
| echo "Waiting for 3 seconds... Press Ctrl+C to cancel." | ||
| sleep 3 | ||
| fi |
There was a problem hiding this comment.
AHOY_CONFIRM_RESPONSE is not validated – destructive commands may run unintentionally.
When the environment variable is preset, the branch starting on line 301 skips the
approval check entirely. If someone sets AHOY_CONFIRM_RESPONSE=n (or any other
string), the command still exits 0 and the caller proceeds:
AHOY_CONFIRM_RESPONSE=n ahoy provision # ← will happily wipe DBAt the same time, interactive input only accepts the lower-case literal y or true,
ignoring Y, yes, etc.
Proposed hardened logic:
-else
- if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then
- echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}"
- echo "Waiting for 3 seconds... Press Ctrl+C to cancel."
- sleep 3
- fi
-fi
+else
+ # Normalise to lowercase for comparison.
+ _resp=$(printf '%s' "${AHOY_CONFIRM_RESPONSE}" | tr '[:upper:]' '[:lower:]')
+ case "${_resp}" in
+ y|yes|true)
+ if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then
+ echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}"
+ echo "Waiting for 3 seconds... Press Ctrl+C to cancel."
+ sleep 3
+ fi
+ ;;
+ *)
+ echo "The operation was canceled."
+ exit 1
+ ;;
+ esac
+fiBenefits:
- Rejects any preset value other than
y|yes|true. - Accepts common affirmative variants (
yes, uppercase). - Keeps the optional wait.
A similar case-folding check can be applied to the interactive branch to accept
Y/YES/True.
🤖 Prompt for AI Agents
In .ahoy.yml around lines 294 to 305, the current logic does not properly
validate the preset AHOY_CONFIRM_RESPONSE environment variable, allowing any
value other than "y" or "true" to proceed and potentially run destructive
commands unintentionally. Update the validation to accept only affirmative
values like "y", "yes", or "true" in any case (case-insensitive), and reject all
others by exiting with an error. Also, modify the interactive input check to
accept common affirmative variants such as "Y", "YES", and "True" in a
case-insensitive manner, while preserving the optional wait behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1846 +/- ##
===========================================
+ Coverage 75.05% 75.11% +0.06%
===========================================
Files 84 84
Lines 4835 4835
Branches 35 35
===========================================
+ Hits 3629 3632 +3
+ Misses 1206 1203 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/add-confirm-to-ahoy-provision
branch
from
bf9d3f3 to
e77a3ee
Compare
AlexSkrypnyk
force-pushed
the
feature/add-confirm-to-ahoy-provision
branch
from
e77a3ee to
a02e012
Compare
AlexSkrypnyk
force-pushed
the
feature/add-confirm-to-ahoy-provision
branch
from
a02e012 to
ac5cbe6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.ahoy.yml (2)
285-292: Documentation claims validation but implementation bypasses it for preset values.The documentation states that the command "will proceed only if the user types
yortrue", but this is only true for interactive input. WhenAHOY_CONFIRM_RESPONSEis preset, any value (includingn/false) will bypass validation and proceed, potentially allowing destructive operations to run unintentionally.
295-307: Critical security issue:AHOY_CONFIRM_RESPONSEvalidation is completely bypassed.When the environment variable is preset, the
elsebranch (lines 301-307) skips validation entirely. This means destructive commands will proceed regardless of the preset value:AHOY_CONFIRM_RESPONSE=n ahoy provision # ← will still proceed and wipe DB AHOY_CONFIRM_RESPONSE=false ahoy reset # ← will still proceed and remove containersThe interactive branch correctly validates only
yandtrue, but preset values need the same validation.Apply this fix to validate preset responses:
else - if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then - echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}" - echo "Waiting for 3 seconds... Press Ctrl+C to cancel." - sleep 3 - fi + # Validate preset response + if [ "${AHOY_CONFIRM_RESPONSE}" != "y" ] && [ "${AHOY_CONFIRM_RESPONSE}" != "true" ]; then + echo "The operation was canceled." + exit 1 + fi + if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then + echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}" + echo "Waiting for 3 seconds... Press Ctrl+C to cancel." + sleep 3 + fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (7)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_solr/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_valkey/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/web/themes/custom/star_wars/-yarn.lockis excluded by!**/*.lock,!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (8)
.ahoy.yml(10 hunks).github/workflows/vortex-test-common.yml(3 hunks).vortex/tests/bats/_helper.bash(1 hunks).vortex/tests/composer.json(1 hunks).vortex/tests/phpunit/Functional/FunctionalTestCase.php(1 hunks).vortex/tests/phpunit/Traits/LoggerTrait.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/test.workflow.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
.github/workflows/vortex-test-common.yml (2)
Learnt from: AlexSkrypnyk
PR: #1714
File: scripts/vortex/doctor.sh:69-70
Timestamp: 2025-06-03T03:15:29.849Z
Learning: In the Vortex project, logging functions (task(), info(), note(), pass(), fail(), warn()) are intentionally duplicated across all script files to make each script self-contained and independent, rather than sharing them through a common library. This design choice prioritizes script independence over reducing code duplication.
Learnt from: AlexSkrypnyk
PR: #1714
File: scripts/vortex/doctor.sh:69-70
Timestamp: 2025-06-03T03:15:29.849Z
Learning: In the Vortex project, logging functions (task(), info(), note(), pass(), fail(), warn()) are intentionally duplicated across all script files to make each script self-contained and independent, rather than sharing them through a common library. This design choice prioritizes script independence over reducing code duplication.
.vortex/tests/test.workflow.sh (4)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
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.
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/tests/Fixtures/install/hosting_acquia/docs/deployment.md:6-8
Timestamp: 2025-06-01T08:09:18.993Z
Learning: Do not review files in test fixture directories (e.g., files under paths containing /tests/Fixtures/ or similar test fixture patterns). These files are used for testing purposes and should remain as-is.
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:00:28.327Z
Learning: Node.js dependencies are updated in theme directories via RenovateBot
.vortex/tests/composer.json (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:00:28.327Z
Learning: Composer dependencies are updated automatically with compatibility checks via RenovateBot
Learnt from: AlexSkrypnyk
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.
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
Learnt from: AlexSkrypnyk
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.
.vortex/tests/bats/_helper.bash (1)
Learnt from: AlexSkrypnyk
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.
.vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (1)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
.ahoy.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:00:28.327Z
Learning: Applies to tests/behat/**/*.feature : All Behat feature files must use the standard user story format: 'As a [user type]\nI want to [action]\nSo that [benefit]'
⏰ 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). (13)
- 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-installer (8.2)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-docs
🔇 Additional comments (21)
.ahoy.yml (9)
22-22: Good use of confirmation for destructive build operation.The confirmation prompt appropriately warns users about the destructive nature of the build command (removing containers, build files, and database) and uses proper exit handling.
64-64: Confirmation added to prevent accidental database removal.Good safety measure for the
downcommand which removes databases and volumes.
135-135: Appropriate confirmation for database replacement.The warning message clearly communicates that the current database will be replaced.
145-147: Provision command now properly protected with confirmation.The confirmation prevents accidental database removal, which aligns with the PR objective of adding confirmation to
ahoy provision.
164-167: Reset command improved with confirmation and cleaner structure.The confirmation protects against accidental removal of containers and build files. The command structure is also improved by breaking the chained commands into separate lines for better readability.
264-264: Deployment confirmation helps prevent accidental manual deployments.The detailed warning message appropriately discourages manual deployment while still allowing it when intentional.
215-215: Simplified lint-tests command structure.The command was consolidated from multi-line to single-line format without changing functionality.
122-122: Minor correction in login command usage text.Fixed the usage description to properly describe generating a login link.
241-241: Corrected PHPUnit test command descriptions.The usage descriptions now properly specify "PHPUnit" instead of just "unit", "kernel", and "functional" tests, providing clearer context.
Also applies to: 245-245, 249-249
.github/workflows/vortex-test-common.yml (3)
93-94: Added debug environment variable for consistent test verbosity control.The
TEST_VORTEX_DEBUGvariable enables debug output control in the common test job, aligning with the environment variable changes in the test suite.
162-163: Debug variable added to workflow test job.Consistent with the debug environment variable pattern across all test jobs.
239-240: Debug variable added to deployment test job.Completes the consistent debug variable pattern across all three test job types.
.vortex/tests/composer.json (1)
31-32: PSR-4 autoload order change is cosmetic.Swapping the order of namespace mappings doesn't affect functionality.
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
50-50: Environment variable updated for consistent debug naming.The change from
TEST_VORTEX_VERBOSEtoTEST_VORTEX_DEBUGaligns with the debug variable naming used in the GitHub workflow and provides consistent verbosity control across the test suite..vortex/tests/test.workflow.sh (1)
28-29: Suppressed dependency installation output for cleaner test logs.The output redirection reduces console verbosity during test setup while preserving the conditional installation logic. This aligns with the broader test output control improvements in this PR.
.vortex/tests/phpunit/Traits/LoggerTrait.php (3)
21-21: LGTM: Consistent newline handling for step boundaries.The explicit newline write to stderr improves the visual separation of test step boundaries in the output.
29-29: LGTM: Consistent newline handling for step boundaries.The explicit newline write to stderr improves the visual separation of test step boundaries in the output.
33-33: LGTM: Direct stderr output for substeps with consistent formatting.The change to use
fwrite(STDERR, ...)directly provides better control over substep output formatting and ensures consistent indentation with the-->prefix..vortex/tests/bats/_helper.bash (1)
1427-1427: LGTM: Pattern adjustment aligns with .ahoy.yml format changes.The removal of the
cmd:prefix in the sed pattern correctly reflects the updated command structure in.ahoy.ymlafter the confirmation mechanism was added..vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (2)
105-109: LGTM: Assertion pattern updated to match new .ahoy.yml format.The removal of the
cmd:prefix in the assertion correctly reflects the updated command structure in.ahoy.ymlafter the confirmation mechanism changes.
111-114: LGTM: String replacement pattern updated consistently.The File::replaceContent call correctly removes the
cmd:prefix from both the search pattern and replacement string, maintaining consistency with the .ahoy.yml format changes.
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/bats/_helper.bash(1 hunks)
⏰ 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). (13)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-docs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.vortex/tests/bats/_helper.bash (1)
1427-1427: Sed pattern remains overly rigid despite previous feedbackThe sed substitution pattern still hardcodes exactly six leading spaces, making it fragile to indentation changes in
.ahoy.yml. This is the same issue raised in the previous review that suggested using^[[:space:]]*for flexible whitespace matching.The pattern should be updated to handle any amount of leading whitespace:
- sed "${sed_opts[@]}" 's| ahoy cli ./scripts/vortex/provision.sh| if [ -f .data/db.sql ]; then docker compose exec cli mkdir -p .data; docker compose cp -L .data/db.sql cli:/app/.data/db.sql; fi; ahoy cli \.\/scripts\/vortex\/provision\.sh|g' .ahoy.yml + sed "${sed_opts[@]}" 's|^[[:space:]]*ahoy cli[[:space:]]./scripts/vortex/provision\.sh| if [ -f .data/db.sql ]; then docker compose exec cli mkdir -p .data; docker compose cp -L .data/db.sql cli:/app/.data/db.sql; fi; ahoy cli ./scripts/vortex/provision.sh|g' .ahoy.yml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.vortex/tests/bats/_helper.bash(2 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
.vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (1)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
.vortex/tests/bats/_helper.bash (2)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
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.
⏰ 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). (13)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-docs
🔇 Additional comments (3)
.vortex/tests/bats/_helper.bash (1)
32-34: LGTM - Conditional platform setting commented out appropriatelyThe conditional ARM architecture platform setting is appropriately commented out and marked for code coverage exclusion. This change appears intentional and maintains the existing logic structure while disabling the automatic platform detection.
.vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (2)
105-109: Good adjustment for new .ahoy.yml command structureThe removal of the
cmd:prefix from the assertion aligns well with the new multi-line command structure in.ahoy.yml. This change ensures the test continues to work with the updated provision command format.
111-116: Excellent documentation of replacement logicThe added comments clearly explain the purpose of the replacement and the important caveat about the provision script potentially appearing in multiple sections. This documentation will help future maintainers understand the specific targeting requirement.
| File::replaceContent('.ahoy.yml', | ||
| 'cmd: ahoy cli ./scripts/vortex/provision.sh', | ||
| 'cmd: if [ -f .data/db.sql ]; then docker compose exec cli mkdir -p .data; docker compose cp -L .data/db.sql cli:/app/.data/db.sql; fi; ahoy cli ./scripts/vortex/provision.sh', | ||
| ' ahoy cli ./scripts/vortex/provision.sh', | ||
| ' if [ -f .data/db.sql ]; then docker compose exec cli mkdir -p .data; docker compose cp -L .data/db.sql cli:/app/.data/db.sql; fi; ahoy cli ./scripts/vortex/provision.sh', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replacement logic is sound but uses hardcoded indentation
The replacement command structure correctly handles the new confirmation-based provision workflow. However, the hardcoded 6-space indentation (' ahoy cli ./scripts/vortex/provision.sh') makes this fragile to indentation changes in .ahoy.yml.
Consider using a more flexible pattern that matches any leading whitespace to make the code more maintainable:
- ' ahoy cli ./scripts/vortex/provision.sh',
+ '/^[[:space:]]*ahoy cli[[:space:]]\.\/scripts\/vortex\/provision\.sh/',This would make the replacement more resilient to formatting changes while maintaining the precise targeting mentioned in the comments.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php around lines 117
to 121, the replacement uses a hardcoded 6-space indentation which makes it
fragile to changes in the .ahoy.yml file's formatting. Modify the replacement
logic to match any leading whitespace before the command instead of fixed
spaces, using a regex pattern that captures all leading whitespace and replaces
the command accordingly. This will make the replacement more flexible and
maintainable against indentation changes.
83a1a84 to
0eca68e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.ahoy.yml (1)
295-307: Environment-variable bypass still unvalidated – destructive commands can run unintentionallyThe
elsebranch trusts any presetAHOY_CONFIRM_RESPONSEvalue and proceeds, contradicting the earlier doc-comment (“will proceed only if the user typesyortrue”).
export AHOY_CONFIRM_RESPONSE=n ahoy provisionwill still wipe the database.The same issue was raised in a previous review and is still unresolved.
Harden the check by normalising/validating the preset value and aborting on anything but an affirmative answer.-else - if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then - echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}" - echo "Waiting for 3 seconds... Press Ctrl+C to cancel." - sleep 3 - fi -fi +else + _resp=$(printf '%s' "${AHOY_CONFIRM_RESPONSE}" | tr '[:upper:]' '[:lower:]') + case "${_resp}" in + y|yes|true) + if [ "${AHOY_CONFIRM_WAIT_SKIP}" != "1" ]; then + echo ">> $1 [y/N] ${AHOY_CONFIRM_RESPONSE}" + echo "Waiting for 3 seconds... Press Ctrl+C to cancel." + sleep 3 + fi + ;; + *) + echo "The operation was canceled." + exit 1 + ;; + esac +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (7)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_solr/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_valkey/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/web/themes/custom/star_wars/-yarn.lockis excluded by!**/*.lock,!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (7)
.ahoy.yml(10 hunks).github/workflows/vortex-test-common.yml(3 hunks).vortex/tests/bats/_helper.bash(1 hunks).vortex/tests/composer.json(2 hunks).vortex/tests/phpunit/Functional/FunctionalTestCase.php(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/test.workflow.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
.vortex/tests/test.workflow.sh (5)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: #1714
File: scripts/vortex/doctor.sh:69-70
Timestamp: 2025-06-03T03:15:29.849Z
Learning: In the Vortex project, logging functions (task(), info(), note(), pass(), fail(), warn()) are intentionally duplicated across all script files to make each script self-contained and independent, rather than sharing them through a common library. This design choice prioritizes script independence over reducing code duplication.
Learnt from: AlexSkrypnyk
PR: #1714
File: scripts/vortex/doctor.sh:69-70
Timestamp: 2025-06-03T03:15:29.849Z
Learning: In the Vortex project, logging functions (task(), info(), note(), pass(), fail(), warn()) are intentionally duplicated across all script files to make each script self-contained and independent, rather than sharing them through a common library. This design choice prioritizes script independence over reducing code duplication.
Learnt from: AlexSkrypnyk
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.
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:00:28.327Z
Learning: Node.js dependencies are updated in theme directories via RenovateBot
.github/workflows/vortex-test-common.yml (2)
Learnt from: AlexSkrypnyk
PR: #1714
File: scripts/vortex/doctor.sh:69-70
Timestamp: 2025-06-03T03:15:29.849Z
Learning: In the Vortex project, logging functions (task(), info(), note(), pass(), fail(), warn()) are intentionally duplicated across all script files to make each script self-contained and independent, rather than sharing them through a common library. This design choice prioritizes script independence over reducing code duplication.
Learnt from: AlexSkrypnyk
PR: #1714
File: scripts/vortex/doctor.sh:69-70
Timestamp: 2025-06-03T03:15:29.849Z
Learning: In the Vortex project, logging functions (task(), info(), note(), pass(), fail(), warn()) are intentionally duplicated across all script files to make each script self-contained and independent, rather than sharing them through a common library. This design choice prioritizes script independence over reducing code duplication.
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
Learnt from: AlexSkrypnyk
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.
.vortex/tests/composer.json (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:00:28.327Z
Learning: Composer dependencies are updated automatically with compatibility checks via RenovateBot
Learnt from: AlexSkrypnyk
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.
.vortex/tests/bats/_helper.bash (3)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
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.
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/tests/Fixtures/install/hosting_acquia/docs/deployment.md:6-8
Timestamp: 2025-06-01T08:09:18.993Z
Learning: Do not review files in test fixture directories (e.g., files under paths containing /tests/Fixtures/ or similar test fixture patterns). These files are used for testing purposes and should remain as-is.
.ahoy.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:00:28.327Z
Learning: Applies to tests/behat/**/*.feature : All Behat feature files must use the standard user story format: 'As a [user type]\nI want to [action]\nSo that [benefit]'
⏰ 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). (13)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-docs
🔇 Additional comments (7)
.github/workflows/vortex-test-common.yml (1)
62-66: Verify pinned Node.js version exists
actions/setup-nodewill fail if the requested version (22.17.1) is not yet published.
Double-check the version string or switch to an LTS version to keep CI green.Also applies to: 144-147, 209-211
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
50-52: LGTM – env-variable switch correctly updatedThe verbosity flag now honours
TEST_VORTEX_DEBUG; implementation is straightforward and consistent with workflow changes..vortex/tests/composer.json (1)
31-33: Structure corrected and timeout added – looks goodNested
configissue is gone andprocess-timeoutis now at the right level.Also applies to: 41-41
.vortex/tests/bats/_helper.bash (1)
1427-1427: Code change correctly adapts to new .ahoy.yml format.The sed substitution pattern has been appropriately updated to match the new provision command structure without the
cmd:prefix. The replacement logic correctly handles the unmounted volumes scenario by conditionally copying the database file before executing the provision script..vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (3)
106-106: Assertion correctly updated for new .ahoy.yml format.The file content assertion has been appropriately updated to match the provision command without the
cmd:prefix, aligning with the new command structure.
111-116: Comments clearly explain the targeted replacement logic.The updated comments provide good context about why the replacement targets specifically the provision section and the challenges of handling multiple occurrences of the provision script call.
118-119: Replacement logic correctly handles unmounted volumes scenario.The file replacement correctly targets the indented provision command and adds appropriate conditional logic to copy database files when volumes are not mounted. This maintains consistency with the BATS helper implementation.
| if [ -z "${AHOY_CONFIRM_RESPONSE}" ]; then | ||
| read -r -p ">> $1 [y/N] " AHOY_CONFIRM_RESPONSE | ||
| [ "${AHOY_CONFIRM_RESPONSE}" = "y" ] || [ "${AHOY_CONFIRM_RESPONSE}" = true ] || ( echo "The operation was canceled." && exit 1 ) | ||
| if [ "${AHOY_CONFIRM_RESPONSE}" != "y" ] && [ "${AHOY_CONFIRM_RESPONSE}" != "true" ]; then | ||
| echo "The operation was canceled." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make interactive check case-insensitive & accept common variants
Requiring exactly y or true is unnecessarily strict—users tend to type Y, yes, Yes, etc.
Convert the input to lowercase and match y|yes|true to improve UX.
🤖 Prompt for AI Agents
In the .ahoy.yml file around lines 295 to 300, the interactive confirmation
check only accepts lowercase 'y' or 'true', which is too strict. Modify the
script to convert the user input to lowercase before the comparison, then check
if it matches 'y', 'yes', or 'true' to allow common variants and improve user
experience.
| [ ! -d "${TEST_DIR}/node_modules" ] && echo " > Install test Node dependencies." && yarn --cwd="${TEST_DIR}" install --frozen-lockfile >/dev/null 2>&1 | ||
| [ ! -d "${TEST_DIR}/vendor" ] && echo " > Install test PHP dependencies." && composer --working-dir="${TEST_DIR}" install --no-interaction --no-progress --optimize-autoloader >/dev/null 2>&1 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider keeping dependency install output under debug flag
Swallowing all yarn/composer output can hide useful failure clues. Redirect only when TEST_VORTEX_DEBUG is not set, e.g.:
-... install --frozen-lockfile >/dev/null 2>&1
+... install --frozen-lockfile ${TEST_VORTEX_DEBUG:+} >/dev/null 2>&1Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .vortex/tests/test.workflow.sh around lines 28 to 30, the current code
redirects all output from yarn and composer installs to /dev/null, hiding useful
error messages. Modify the script to conditionally redirect output only if the
environment variable TEST_VORTEX_DEBUG is not set, so that when debugging is
enabled, the full output is visible. Implement this by checking
TEST_VORTEX_DEBUG before deciding whether to redirect output.
Summary by CodeRabbit
New Features
Refactor
Chores
TEST_VORTEX_VERBOSEtoTEST_VORTEX_DEBUG.