Allow Codecov to fail in CI without failing the pipeline.#2476
Merged
AlexSkrypnyk merged 9 commits intomainfrom Apr 27, 2026
Merged
Allow Codecov to fail in CI without failing the pipeline.#2476AlexSkrypnyk merged 9 commits intomainfrom
AlexSkrypnyk merged 9 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds resilient Codecov upload handling across multiple GitHub workflows; introduces a new executable CLI Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as ".vortex/tests/update-test-assets"
participant FS as "Filesystem"
participant Drush
participant Docker as "Docker Engine"
participant Registry as "Docker Registry"
User->>CLI: invoke with mode [--tag]
CLI->>FS: validate tools & prepare workspace
CLI->>Drush: run provisioning / seed / export commands
Drush-->>CLI: produce SQL dumps
CLI->>FS: normalize SQL collations and write dumps
CLI->>Docker: build and tag images
Docker->>Registry: push image(s) to destination tags
Registry-->>CLI: return push status
CLI-->>User: report results and next steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2476 +/- ##
==========================================
- Coverage 79.97% 79.51% -0.46%
==========================================
Files 129 122 -7
Lines 6895 6736 -159
Branches 47 3 -44
==========================================
- Hits 5514 5356 -158
+ Misses 1381 1380 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/tests/update-test-assets:
- Line 87: mode_destination_images currently assumes the source image tag is
":latest", ignoring the CLI --tag option; update mode_destination_images (and
the other affected mappings at 139-147) to accept and use the resolved tag
parameter (or read the shared tag variable) when constructing image references
so images are built/pushed and referenced as "<image>:<tag>" instead of always
":latest"; locate mode_destination_images and any call sites that register
'destination-images' and change the signature/usage to propagate the tag value
through and interpolate it into the source image name.
- Around line 186-193: The code does not check the results of mkdir() or
file_put_contents(), so failures will surface later; after computing $seed_dir
and before writing, validate mkdir($seed_dir, 0755, TRUE) and handle a false
return (e.g., log and exit/throw), then call build_seed_script($title, $body)
and write with file_put_contents($seed_dir . '/seed-test-asset.php', $script)
but check its return value as well and fail fast with a descriptive error if it
returns false; update the block that creates $seed_dir and writes the seed file
to perform these checks and error handling.
- Around line 218-224: The test currently downloads and executes seed.sh from
SEED_URL (which points at /releases/latest/) creating an unreproducible
supply-chain risk; change SEED_URL to a pinned release URL (non-latest) and add
an integrity check using a stored expected SHA256 before executing: after
run(sprintf('curl -fsSL -o seed.sh %s', escapeshellarg(SEED_URL)), $cwd) compute
and verify the file's SHA256 against a new constant like SEED_SHA256 (or fetch a
.sha256 from the same pinned release) and fail the test (do not run
chmod/chroot/./seed.sh) if the checksum does not match; update references to
seed.sh, SEED_URL and add SEED_SHA256 so the test fails fast on mismatch and
uses a fixed, audited script version.
🪄 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: fc264a7b-722b-453c-b31c-5d0335da7fd8
📒 Files selected for processing (2)
.vortex/docs/content/contributing/maintenance/template.mdx.vortex/tests/update-test-assets
Comment on lines
+186
to
+193
| $seed_dir = $cwd . '/.data'; | ||
| if (!is_dir($seed_dir)) { | ||
| mkdir($seed_dir, 0755, TRUE); | ||
| } | ||
|
|
||
| $script = build_seed_script($title, $body); | ||
| file_put_contents($seed_dir . '/seed-test-asset.php', $script); | ||
|
|
There was a problem hiding this comment.
Fail fast when seed script file creation fails.
mkdir() and file_put_contents() results are not validated. A write failure will surface later as a less-actionable command error.
Suggested fix
$seed_dir = $cwd . '/.data';
- if (!is_dir($seed_dir)) {
- mkdir($seed_dir, 0755, TRUE);
+ if (!is_dir($seed_dir) && !mkdir($seed_dir, 0755, TRUE) && !is_dir($seed_dir)) {
+ fwrite(STDERR, sprintf("Error: failed to create directory %s.\n", $seed_dir));
+ exit(1);
}
$script = build_seed_script($title, $body);
- file_put_contents($seed_dir . '/seed-test-asset.php', $script);
+ $seed_script = $seed_dir . '/seed-test-asset.php';
+ if (file_put_contents($seed_script, $script) === FALSE) {
+ fwrite(STDERR, sprintf("Error: failed to write %s.\n", $seed_script));
+ exit(1);
+ }📝 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.
Suggested change
| $seed_dir = $cwd . '/.data'; | |
| if (!is_dir($seed_dir)) { | |
| mkdir($seed_dir, 0755, TRUE); | |
| } | |
| $script = build_seed_script($title, $body); | |
| file_put_contents($seed_dir . '/seed-test-asset.php', $script); | |
| $seed_dir = $cwd . '/.data'; | |
| if (!is_dir($seed_dir) && !mkdir($seed_dir, 0755, TRUE) && !is_dir($seed_dir)) { | |
| fwrite(STDERR, sprintf("Error: failed to create directory %s.\n", $seed_dir)); | |
| exit(1); | |
| } | |
| $script = build_seed_script($title, $body); | |
| $seed_script = $seed_dir . '/seed-test-asset.php'; | |
| if (file_put_contents($seed_script, $script) === FALSE) { | |
| fwrite(STDERR, sprintf("Error: failed to write %s.\n", $seed_script)); | |
| exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/update-test-assets around lines 186 - 193, The code does not
check the results of mkdir() or file_put_contents(), so failures will surface
later; after computing $seed_dir and before writing, validate mkdir($seed_dir,
0755, TRUE) and handle a false return (e.g., log and exit/throw), then call
build_seed_script($title, $body) and write with file_put_contents($seed_dir .
'/seed-test-asset.php', $script) but check its return value as well and fail
fast with a descriptive error if it returns false; update the block that creates
$seed_dir and writes the seed file to perform these checks and error handling.
Comment on lines
+218
to
+224
| step('Download seed.sh'); | ||
| run(sprintf('curl -fsSL -o seed.sh %s', escapeshellarg(SEED_URL)), $cwd); | ||
| run('chmod +x seed.sh', $cwd); | ||
|
|
||
| step(sprintf('Seed and push image to %s', $image)); | ||
| run(sprintf('./seed.sh .data/%s %s', escapeshellarg($filename), escapeshellarg($image)), $cwd); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "update-test-assets" -type fRepository: drevops/vortex
Length of output: 93
🏁 Script executed:
cat -n ./.vortex/tests/update-test-assets | head -250 | tail -100Repository: drevops/vortex
Length of output: 4181
🏁 Script executed:
grep -n "SEED_URL" ./.vortex/tests/update-test-assetsRepository: drevops/vortex
Length of output: 241
🏁 Script executed:
grep -n "hash\|SEED_SHA\|integrity\|checksum" ./.vortex/tests/update-test-assetsRepository: drevops/vortex
Length of output: 40
Pin the remote script version and add SHA256 integrity verification.
The code downloads seed.sh from https://github.com/drevops/mariadb-drupal-data/releases/latest/download/seed.sh and executes it directly without pinning the version or verifying its integrity. Using /releases/latest/ is a supply-chain risk that breaks reproducibility and makes the script vulnerable to unexpected changes in the release.
Suggested hardening
-const SEED_URL = 'https://github.com/drevops/mariadb-drupal-data/releases/latest/download/seed.sh';
+const SEED_URL = 'https://github.com/drevops/mariadb-drupal-data/releases/download/vX.Y.Z/seed.sh';
+const SEED_SHA256 = 'REPLACE_WITH_RELEASE_SHA256';
@@
function seed_image(string $cwd, string $filename, string $image): void {
step('Download seed.sh');
run(sprintf('curl -fsSL -o seed.sh %s', escapeshellarg(SEED_URL)), $cwd);
+ $actual = hash_file('sha256', $cwd . '/seed.sh');
+ if ($actual !== SEED_SHA256) {
+ fwrite(STDERR, sprintf("Error: seed.sh checksum mismatch (expected %s, got %s).\n", SEED_SHA256, (string) $actual));
+ exit(1);
+ }
run('chmod +x seed.sh', $cwd);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/update-test-assets around lines 218 - 224, The test currently
downloads and executes seed.sh from SEED_URL (which points at /releases/latest/)
creating an unreproducible supply-chain risk; change SEED_URL to a pinned
release URL (non-latest) and add an integrity check using a stored expected
SHA256 before executing: after run(sprintf('curl -fsSL -o seed.sh %s',
escapeshellarg(SEED_URL)), $cwd) compute and verify the file's SHA256 against a
new constant like SEED_SHA256 (or fetch a .sha256 from the same pinned release)
and fail the test (do not run chmod/chroot/./seed.sh) if the checksum does not
match; update references to seed.sh, SEED_URL and add SEED_SHA256 so the test
fails fast on mismatch and uses a fixed, audited script version.
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
force-pushed
the
feature/optional-codecov
branch
from
e2f5e76 to
edc0c83
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.vortex/tests/update-test-assets (1)
43-43:⚠️ Potential issue | 🔴 CriticalPin and verify
seed.shbefore executing it.The script still downloads
seed.shfrom/releases/latest/and executes it unchecked. That makes the asset build non-reproducible and turns an upstream release change into code execution on the maintainer machine.Suggested fix
-const SEED_URL = 'https://github.com/drevops/mariadb-drupal-data/releases/latest/download/seed.sh'; +const SEED_URL = 'https://github.com/drevops/mariadb-drupal-data/releases/download/vX.Y.Z/seed.sh'; +const SEED_SHA256 = 'REPLACE_WITH_RELEASE_SHA256'; @@ function seed_image(string $cwd, string $filename, string $image): void { step('Download seed.sh'); run(sprintf('curl -fsSL -o seed.sh %s', escapeshellarg(SEED_URL)), $cwd); + $actual = hash_file('sha256', $cwd . '/seed.sh'); + if ($actual !== SEED_SHA256) { + fwrite(STDERR, sprintf("Error: seed.sh checksum mismatch (expected %s, got %s).\n", SEED_SHA256, (string) $actual)); + exit(1); + } run('chmod +x seed.sh', $cwd);Also applies to: 306-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tests/update-test-assets at line 43, The code defines SEED_URL and downloads/executes seed.sh directly which is unsafe and unreproducible; change SEED_URL to point to a pinned release artifact (specific tag or commit) and add verification before execution: fetch a published checksum or signature for that exact artifact and verify it (e.g., compare SHA256 or verify GPG) prior to running, and fail the build if verification fails; update the download-and-execute logic (the code that uses SEED_URL) to perform the checksum/signature retrieval and verification step and only then execute the local file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/docs/content/contributing/maintenance/template.mdx:
- Around line 271-275: Update the stale bullet about "destination-images":
replace the statement that it "expects ... to already exist locally" with a note
that `.vortex/tests/update-test-assets` now pulls the source demo image before
tagging, so `destination-images` only requires the requested tag to exist in the
registry (it no longer requires a local image or that `demo-image`/`all` were
run in the same session); edit the bullet referencing `destination-images`
accordingly.
In @.vortex/tests/update-test-assets:
- Around line 331-343: The parsing of the --tag option must reject empty or
whitespace-only values in both forms; update the handling around the $tag
assignment (both the $arg === '--tag' branch and the str_starts_with($arg,
'--tag=') branch) to trim the candidate value (use trim($args[$i + 1]) and
trim(substr(...))) and if the trimmed string === '' call usage_error($script,
$valid_modes, "Option '--tag' requires a value."); ensure $i is still advanced
appropriately in the first branch only after validating the trimmed tag.
- Around line 256-257: The test currently swallows any real front-end build
errors by using run('ahoy fe || true', $cwd) in the step('Build front-end assets
(skipped if theme not configured)') block; change this so failures are not
ignored: detect whether the theme is actually configured (e.g., check the theme
directory or config flag) and only skip the command when absent, otherwise run
'ahoy fe' without the '|| true' fallback so a non-zero exit will fail the
test/run; update the conditional logic around the run(...) call accordingly.
---
Duplicate comments:
In @.vortex/tests/update-test-assets:
- Line 43: The code defines SEED_URL and downloads/executes seed.sh directly
which is unsafe and unreproducible; change SEED_URL to point to a pinned release
artifact (specific tag or commit) and add verification before execution: fetch a
published checksum or signature for that exact artifact and verify it (e.g.,
compare SHA256 or verify GPG) prior to running, and fail the build if
verification fails; update the download-and-execute logic (the code that uses
SEED_URL) to perform the checksum/signature retrieval and verification step and
only then execute the local file.
🪄 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: d74d7029-a122-4e78-b1a1-362551cfb4ff
⛔ Files ignored due to path filters (19)
.vortex/installer/tests/Fixtures/handler_process/_baseline/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_acquia/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_container_registry/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_ftp/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_s3/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_disabled_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_profile/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/starter_drupal_cms_profile/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/starter_drupal_profile/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom_non_vortex/.envis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (6)
.env.vortex/docs/content/contributing/maintenance/template.mdx.vortex/tests/bats/unit/provision.bats.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php.vortex/tests/phpunit/Traits/SutTrait.php.vortex/tests/update-test-assets
Comment on lines
+256
to
+257
| step('Build front-end assets (skipped if theme not configured)'); | ||
| run('ahoy fe || true', $cwd); |
There was a problem hiding this comment.
Do not swallow real front-end build failures.
Line 257 turns every ahoy fe failure into success. That can publish refreshed dumps/images with broken or stale built assets. Skip this step only when the theme is genuinely absent; otherwise let the failure stop the run.
Suggested fix
- step('Build front-end assets (skipped if theme not configured)');
- run('ahoy fe || true', $cwd);
+ $theme_packages = glob($cwd . '/web/themes/custom/*/package.json') ?: [];
+ if ($theme_packages !== []) {
+ step('Build front-end assets');
+ run('ahoy fe', $cwd);
+ }
+ else {
+ step('Skip front-end asset build (theme not configured)');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/update-test-assets around lines 256 - 257, The test currently
swallows any real front-end build errors by using run('ahoy fe || true', $cwd)
in the step('Build front-end assets (skipped if theme not configured)') block;
change this so failures are not ignored: detect whether the theme is actually
configured (e.g., check the theme directory or config flag) and only skip the
command when absent, otherwise run 'ahoy fe' without the '|| true' fallback so a
non-zero exit will fail the test/run; update the conditional logic around the
run(...) call accordingly.
Comment on lines
+331
to
+343
| if ($arg === '--tag') { | ||
| if (!isset($args[$i + 1])) { | ||
| usage_error($script, $valid_modes, "Option '--tag' requires a value."); | ||
| } | ||
| $tag = $args[$i + 1]; | ||
| $i += 2; | ||
| continue; | ||
| } | ||
|
|
||
| if (str_starts_with($arg, '--tag=')) { | ||
| $tag = substr($arg, strlen('--tag=')); | ||
| if ($tag === '') { | ||
| usage_error($script, $valid_modes, "Option '--tag' requires a value."); |
There was a problem hiding this comment.
Reject blank --tag values in both syntaxes.
--tag "" currently passes the --tag <tag> branch, and whitespace-only values pass both branches. The first failure then happens later in Docker with an invalid image reference instead of a clear usage error.
Suggested fix
if ($arg === '--tag') {
- if (!isset($args[$i + 1])) {
+ if (!isset($args[$i + 1]) || trim($args[$i + 1]) === '') {
usage_error($script, $valid_modes, "Option '--tag' requires a value.");
}
$tag = $args[$i + 1];
@@
if (str_starts_with($arg, '--tag=')) {
$tag = substr($arg, strlen('--tag='));
- if ($tag === '') {
+ if (trim($tag) === '') {
usage_error($script, $valid_modes, "Option '--tag' requires a value.");
}📝 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.
Suggested change
| if ($arg === '--tag') { | |
| if (!isset($args[$i + 1])) { | |
| usage_error($script, $valid_modes, "Option '--tag' requires a value."); | |
| } | |
| $tag = $args[$i + 1]; | |
| $i += 2; | |
| continue; | |
| } | |
| if (str_starts_with($arg, '--tag=')) { | |
| $tag = substr($arg, strlen('--tag=')); | |
| if ($tag === '') { | |
| usage_error($script, $valid_modes, "Option '--tag' requires a value."); | |
| if ($arg === '--tag') { | |
| if (!isset($args[$i + 1]) || trim($args[$i + 1]) === '') { | |
| usage_error($script, $valid_modes, "Option '--tag' requires a value."); | |
| } | |
| $tag = $args[$i + 1]; | |
| $i += 2; | |
| continue; | |
| } | |
| if (str_starts_with($arg, '--tag=')) { | |
| $tag = substr($arg, strlen('--tag=')); | |
| if (trim($tag) === '') { | |
| usage_error($script, $valid_modes, "Option '--tag' requires a value."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/update-test-assets around lines 331 - 343, The parsing of the
--tag option must reject empty or whitespace-only values in both forms; update
the handling around the $tag assignment (both the $arg === '--tag' branch and
the str_starts_with($arg, '--tag=') branch) to trim the candidate value (use
trim($args[$i + 1]) and trim(substr(...))) and if the trimmed string === '' call
usage_error($script, $valid_modes, "Option '--tag' requires a value."); ensure
$i is still advanced appropriately in the first branch only after validating the
trimmed tag.
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
force-pushed
the
feature/optional-codecov
branch
from
edc0c83 to
64a7da4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.vortex/docs/content/contributing/maintenance/template.mdx (1)
274-275:⚠️ Potential issue | 🟡 MinorThe
destination-imagescaveat is stale.The script now pulls the source demo image before tagging (see
mode_destination_imagesat lines 139-140 in.vortex/tests/update-test-assets), sodestination-imagesno longer requires the image to already exist locally.,
📝 Suggested documentation update
-- `destination-images` expects `drevops/vortex-dev-mariadb-drupal-data-demo-11.x:1.38.0-rc1` - to already exist locally - run `demo-image` (or `all`) first. +- `destination-images` pulls the source demo image from the registry before + tagging, so the image only needs to exist in the registry (not locally).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/docs/content/contributing/maintenance/template.mdx around lines 274 - 275, Update the stale caveat about `destination-images`: change the sentence that claims `destination-images` expects `drevops/vortex-dev-mariadb-drupal-data-demo-11.x:1.38.0-rc1` to already exist locally to reflect that the script now pulls the source image before tagging (see the `mode_destination_images` logic), so remove the requirement or reword the note to state the script will pull the source demo image automatically prior to tagging; ensure the reference to `destination-images` and `mode_destination_images` is used so reviewers can locate and verify the change..vortex/tests/update-test-assets (3)
407-422:⚠️ Potential issue | 🟡 MinorReject whitespace-only
--tagvalues.The validation checks for empty string but not whitespace-only values like
--tag " ". This would result in a confusing Docker error later rather than a clear usage message.,
🔧 Suggested fix
if ($arg === '--tag') { - if (!isset($args[$i + 1])) { + if (!isset($args[$i + 1]) || trim($args[$i + 1]) === '') { usage_error($script, $valid_modes, "Option '--tag' requires a value."); } - $tag = $args[$i + 1]; + $tag = trim($args[$i + 1]); $i += 2; continue; } if (str_starts_with($arg, '--tag=')) { $tag = substr($arg, strlen('--tag=')); - if ($tag === '') { + if (trim($tag) === '') { usage_error($script, $valid_modes, "Option '--tag' requires a value."); } + $tag = trim($tag); $i += 1; continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tests/update-test-assets around lines 407 - 422, The code accepts a tag value that may be whitespace-only (e.g. --tag " "), so update both parsing branches to reject values that are empty after trimming: when setting $tag from $args[$i + 1] (in the '--tag' branch) and when setting $tag from substr(...) (in the '--tag=' branch), validate with trim($tag) and call usage_error($script, $valid_modes, "Option '--tag' requires a value.") if trim($tag) === '' to ensure whitespace-only tags are treated as missing.
43-43:⚠️ Potential issue | 🟠 MajorPin the remote seed script version and add integrity verification.
Downloading
seed.shfrom/releases/latest/and executing it directly is a supply-chain risk that breaks reproducibility. A malicious or buggy update to the upstream release could affect all future runs without warning.,
🔒 Suggested hardening
-const SEED_URL = 'https://github.com/drevops/mariadb-drupal-data/releases/latest/download/seed.sh'; +const SEED_URL = 'https://github.com/drevops/mariadb-drupal-data/releases/download/v1.0.0/seed.sh'; +const SEED_SHA256 = 'REPLACE_WITH_ACTUAL_SHA256_HASH';Then in
seed_image():function seed_image(string $cwd, string $filename, string $image): void { step('Download seed.sh'); run(sprintf('curl -fsSL -o seed.sh %s', escapeshellarg(SEED_URL)), $cwd); // Verify integrity before execution. $actual = hash_file('sha256', $cwd . '/seed.sh'); if ($actual !== SEED_SHA256) { fwrite(STDERR, sprintf("Error: seed.sh checksum mismatch (expected %s, got %s).\n", SEED_SHA256, (string) $actual)); exit(1); } run('chmod +x seed.sh', $cwd); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tests/update-test-assets at line 43, Replace the floating release URL in SEED_URL with a pinned release URL and add a corresponding SEED_SHA256 constant, then update the seed_image() flow (function seed_image) to verify the downloaded seed.sh before execution: download using the pinned SEED_URL, compute hash_file('sha256') of the downloaded file and compare against SEED_SHA256, and abort with a clear error and non-zero exit if the checksum mismatches, only then chmod +x and run the script. Ensure the constants SEED_URL and SEED_SHA256 are adjacent and documented so future updates require changing both.
275-276:⚠️ Potential issue | 🟠 MajorDo not swallow real front-end build failures.
Using
|| truemasks genuine build errors, potentially publishing assets with broken or stale front-end artifacts. The step comment acknowledges the theme may not be configured, but the failure mode should be more precise.,
🔧 Suggested fix
- step('Build front-end assets (skipped if theme not configured)'); - run('ahoy fe || true', $cwd); + $theme_packages = glob($cwd . '/web/themes/custom/*/package.json') ?: []; + if ($theme_packages !== []) { + step('Build front-end assets'); + run('ahoy fe', $cwd); + } + else { + step('Skip front-end asset build (no theme package.json found)'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tests/update-test-assets around lines 275 - 276, The test is swallowing real front-end build failures by using the tolerant command run('ahoy fe || true', $cwd) inside the step('Build front-end assets (skipped if theme not configured)'); remove the "|| true" and instead make the step explicitly conditional: check the theme/config presence (or a specific env/test flag) before calling run('ahoy fe', $cwd), and if the theme is absent skip the step gracefully; if the theme is present run the command and allow failures to surface (i.e., fail the test on non‑zero exit) so real build errors are not masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/tests/update-test-assets:
- Around line 228-240: The reset_sut function currently shells out to 'mkdir -p'
without checking for failure; replace that shell call with PHP's mkdir(SUT_DIR,
0777, true) (or at least check the return of mkdir) and handle errors: if mkdir
returns false (or is_dir(SUT_DIR) is still false), log or throw a clear error
and exit so later docker run doesn't fail with a less actionable message; update
references in reset_sut and keep using run(...) for the docker cleanup step
unchanged.
---
Duplicate comments:
In @.vortex/docs/content/contributing/maintenance/template.mdx:
- Around line 274-275: Update the stale caveat about `destination-images`:
change the sentence that claims `destination-images` expects
`drevops/vortex-dev-mariadb-drupal-data-demo-11.x:1.38.0-rc1` to already exist
locally to reflect that the script now pulls the source image before tagging
(see the `mode_destination_images` logic), so remove the requirement or reword
the note to state the script will pull the source demo image automatically prior
to tagging; ensure the reference to `destination-images` and
`mode_destination_images` is used so reviewers can locate and verify the change.
In @.vortex/tests/update-test-assets:
- Around line 407-422: The code accepts a tag value that may be whitespace-only
(e.g. --tag " "), so update both parsing branches to reject values that are
empty after trimming: when setting $tag from $args[$i + 1] (in the '--tag'
branch) and when setting $tag from substr(...) (in the '--tag=' branch),
validate with trim($tag) and call usage_error($script, $valid_modes, "Option
'--tag' requires a value.") if trim($tag) === '' to ensure whitespace-only tags
are treated as missing.
- Line 43: Replace the floating release URL in SEED_URL with a pinned release
URL and add a corresponding SEED_SHA256 constant, then update the seed_image()
flow (function seed_image) to verify the downloaded seed.sh before execution:
download using the pinned SEED_URL, compute hash_file('sha256') of the
downloaded file and compare against SEED_SHA256, and abort with a clear error
and non-zero exit if the checksum mismatches, only then chmod +x and run the
script. Ensure the constants SEED_URL and SEED_SHA256 are adjacent and
documented so future updates require changing both.
- Around line 275-276: The test is swallowing real front-end build failures by
using the tolerant command run('ahoy fe || true', $cwd) inside the step('Build
front-end assets (skipped if theme not configured)'); remove the "|| true" and
instead make the step explicitly conditional: check the theme/config presence
(or a specific env/test flag) before calling run('ahoy fe', $cwd), and if the
theme is absent skip the step gracefully; if the theme is present run the
command and allow failures to surface (i.e., fail the test on non‑zero exit) so
real build errors are not masked.
🪄 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: ed79ada3-1a68-4c55-89a7-0635e2705b96
⛔ Files ignored due to path filters (19)
.vortex/installer/tests/Fixtures/handler_process/_baseline/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_acquia/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_container_registry/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_ftp/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/db_download_source_s3/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_disabled_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_profile/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/starter_drupal_cms_profile/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/starter_drupal_profile/.envis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom_non_vortex/.envis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (6)
.env.vortex/docs/content/contributing/maintenance/template.mdx.vortex/tests/bats/unit/provision.bats.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php.vortex/tests/phpunit/Traits/SutTrait.php.vortex/tests/update-test-assets
Comment on lines
+228
to
+240
| function reset_sut(string $root_dir, string $installer): void { | ||
| step(sprintf('Reset SUT directory %s', SUT_DIR)); | ||
|
|
||
| // Drupal stages public files inside containers running as 'www-data' | ||
| // (uid 33). When those bind-mounts surface on the host, plain 'rm -rf' | ||
| // by the current user fails with "Permission denied". Clear the contents | ||
| // via an ephemeral root-privileged container so file ownership doesn't | ||
| // matter, then leave the dir itself in place for the installer. | ||
| run(sprintf('mkdir -p %s', escapeshellarg(SUT_DIR))); | ||
| run(sprintf( | ||
| 'docker run --rm -v %s:/sut alpine find /sut -mindepth 1 -delete', | ||
| escapeshellarg(SUT_DIR) | ||
| )); |
There was a problem hiding this comment.
Add error handling for directory creation failure.
The mkdir call at line 236 does not verify success. If the directory cannot be created (permissions, disk full), the failure surfaces later with a less actionable error.
🔧 Suggested fix
step(sprintf('Reset SUT directory %s', SUT_DIR));
- run(sprintf('mkdir -p %s', escapeshellarg(SUT_DIR)));
+ if (!is_dir(SUT_DIR) && !mkdir(SUT_DIR, 0755, TRUE) && !is_dir(SUT_DIR)) {
+ fwrite(STDERR, sprintf("Error: failed to create directory %s.\n", SUT_DIR));
+ exit(1);
+ }
run(sprintf(Note: Using PHP's mkdir() with error handling is more robust than shelling out to mkdir -p.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/update-test-assets around lines 228 - 240, The reset_sut
function currently shells out to 'mkdir -p' without checking for failure;
replace that shell call with PHP's mkdir(SUT_DIR, 0777, true) (or at least check
the return of mkdir) and handle errors: if mkdir returns false (or
is_dir(SUT_DIR) is still false), log or throw a clear error and exit so later
docker run doesn't fail with a less actionable message; update references in
reset_sut and keep using run(...) for the docker cleanup step unchanged.
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.
1 similar comment
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php:
- Around line 565-567: The test currently prints partial secret material by
calling logSubstep then executing cmd with an ahoy drush php:eval that outputs
"hash_salt=" and part of Settings::get("hash_salt"); change this to avoid
exposing secret bytes: modify the cmd(...) PHP snippet so it only emits
non-secret metadata (e.g. presence, boolean or masked length) for
Settings::get("hash_salt") and continue to print deployment_identifier as before
(or print its presence similarly); keep references to the same call site (the
logSubstep and the cmd('ahoy drush php:eval ...') invocation) so the test still
verifies existence without logging secret content.
- Around line 533-579: The debug probe calls currently call $this->cmd(...)
directly which makes them fatal on transient failures; wrap each debug probe
invocation in a try/catch that catches \Throwable (or \Exception) and logs the
failure with $this->logSubstep so the test continues (e.g. try {
$this->cmd(...); } catch (\Throwable $e) { $this->logSubstep('DEBUG: probe
failed: ' . $e->getMessage()); }). Apply this to every probe invocation that
currently calls $this->cmd(...) in the diagnostic block (the sequence of
$this->logSubstep(...) followed by $this->cmd(...)), ensuring any shell commands
remain unchanged but are protected by the try/catch to make them
best-effort/non-fatal.
🪄 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: a55dc723-2596-4628-95ed-b67f94deb6d2
⛔ Files ignored due to path filters (1)
.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.envis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (1)
.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php
Comment on lines
+565
to
+567
| $this->logSubstep('DEBUG: hash_salt and deployment_identifier'); | ||
| $this->cmd('ahoy drush php:eval "print \"hash_salt=\" . substr(\Drupal\Core\Site\Settings::get(\"hash_salt\"), 0, 12) . PHP_EOL . \"deployment_identifier=\" . (\Drupal\Core\Site\Settings::get(\"deployment_identifier\") ?? \"(null)\") . PHP_EOL;"'); | ||
|
|
There was a problem hiding this comment.
Avoid logging hash_salt material in CI output.
Line 566 prints part of hash_salt. Even partial secret disclosure should be avoided in logs; log only presence/metadata.
Proposed fix (log presence only, not secret bytes)
- $this->cmd('ahoy drush php:eval "print \"hash_salt=\" . substr(\Drupal\Core\Site\Settings::get(\"hash_salt\"), 0, 12) . PHP_EOL . \"deployment_identifier=\" . (\Drupal\Core\Site\Settings::get(\"deployment_identifier\") ?? \"(null)\") . PHP_EOL;"');
+ $this->cmd('ahoy drush php:eval "print \"hash_salt_set=\" . (\Drupal\Core\Site\Settings::get(\"hash_salt\") ? \"1\" : \"0\") . PHP_EOL . \"deployment_identifier_set=\" . (\Drupal\Core\Site\Settings::get(\"deployment_identifier\") ? \"1\" : \"0\") . PHP_EOL;"');📝 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.
Suggested change
| $this->logSubstep('DEBUG: hash_salt and deployment_identifier'); | |
| $this->cmd('ahoy drush php:eval "print \"hash_salt=\" . substr(\Drupal\Core\Site\Settings::get(\"hash_salt\"), 0, 12) . PHP_EOL . \"deployment_identifier=\" . (\Drupal\Core\Site\Settings::get(\"deployment_identifier\") ?? \"(null)\") . PHP_EOL;"'); | |
| $this->logSubstep('DEBUG: hash_salt and deployment_identifier'); | |
| $this->cmd('ahoy drush php:eval "print \"hash_salt_set=\" . (\Drupal\Core\Site\Settings::get(\"hash_salt\") ? \"1\" : \"0\") . PHP_EOL . \"deployment_identifier_set=\" . (\Drupal\Core\Site\Settings::get(\"deployment_identifier\") ? \"1\" : \"0\") . PHP_EOL;"'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php around lines 565 -
567, The test currently prints partial secret material by calling logSubstep
then executing cmd with an ahoy drush php:eval that outputs "hash_salt=" and
part of Settings::get("hash_salt"); change this to avoid exposing secret bytes:
modify the cmd(...) PHP snippet so it only emits non-secret metadata (e.g.
presence, boolean or masked length) for Settings::get("hash_salt") and continue
to print deployment_identifier as before (or print its presence similarly); keep
references to the same call site (the logSubstep and the cmd('ahoy drush
php:eval ...') invocation) so the test still verifies existence without logging
secret content.
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.
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.
1 similar comment
|
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.
Member
Author
|
Code coverage (threshold: 90%) Per-class coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
New Features
Tests
Documentation