Ported recent shell script changes to PHP tooling package.#2472
Ported recent shell script changes to PHP tooling package.#2472AlexSkrypnyk merged 2 commits intoproject/2.xfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughAdds per-channel branch allowlists to notification scripts, updates Acquia DB download to capture 302 redirect targets and validate gzip download/decompression (retaining failed .gz), extends HTTP helper to optionally not follow redirects, and adds an optional repeated config:import in provisioning. Tests updated/added accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Download script
participant Helper as request()
participant Acquia as Acquia /download-action
participant S3 as S3 (redirect URL)
participant FS as Filesystem
Script->>Helper: GET /download-action (follow_redirects=false)
Helper->>Acquia: HTTP GET (no follow)
Acquia-->>Helper: 302 + Location -> info.redirect_url
Helper-->>Script: return info.redirect_url
Script->>S3: GET redirect_url (no Acquia Authorization)
S3-->>Script: 200 -> write file dump.gz
Script->>FS: validate gzip header & size
alt gzip valid
Script->>FS: decompress -> write output.sql
FS-->>Script: output exists & non-empty
Script->>FS: unlink dump.gz
else gzip invalid or decompression failed
Script-->>FS: leave dump.gz for inspection (no unlink)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/tooling/tests/Unit/DownloadDbAcquiaTest.php (1)
159-169:⚠️ Potential issue | 🟠 MajorAdd an explicit assertion that Acquia auth headers are not sent to S3.
This PR’s core security fix is header-leak prevention, but this test path currently validates only redirect/failure flow and not header stripping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tooling/tests/Unit/DownloadDbAcquiaTest.php around lines 159 - 169, Add an assertion in the DownloadDbAcquiaTest path that verifies Acquia auth headers are not forwarded to the S3 GET request: after simulating the 302 redirect and the subsequent GET to 'https://acquia-backup.s3.amazonaws.com/backup.sql.gz', inspect the recorded/mock request for that URL and assert it does not contain the Acquia auth header(s) (e.g., 'Authorization' and any 'X-Acquia-*' headers). Use the test's existing HTTP mock/recorder (the same structure used to provide the fake responses) to fetch the GET request headers and add assertions like assertArrayNotHasKey for those header names to ensure header-leak prevention in this flow.
🤖 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/tooling/src/download-db-acquia:
- Around line 233-237: The code reads
$backup_url_response['info']['redirect_url'] without first checking the request
result; add an explicit status/ok/error check on $backup_url_response (e.g.
verify an 'ok' or 'status' field and any error message) before accessing
['info']['redirect_url'], and if the request failed call fail() with a clear
message that includes the original error/status and $backup_id so you preserve
the root cause; keep the existing empty-string fallback for $backup_url but only
after the success check and include the detailed error in the fail() message.
In @.vortex/tooling/src/notify-github:
- Around line 32-39: The gate currently reads the current branch from
VORTEX_NOTIFY_BRANCH only, causing it to ignore channel-specific env
VORTEX_NOTIFY_GITHUB_BRANCH; update the current_branch determination in the
block that uses getenv_default('VORTEX_NOTIFY_GITHUB_BRANCHES') so it first
checks getenv('VORTEX_NOTIFY_GITHUB_BRANCH') and falls back to
getenv('VORTEX_NOTIFY_BRANCH') (both cast to string), then use that
current_branch when comparing against branch_list (functions/variables to
locate: getenv_default, VORTEX_NOTIFY_GITHUB_BRANCHES,
VORTEX_NOTIFY_GITHUB_BRANCH, VORTEX_NOTIFY_BRANCH, current_branch, branch_list,
pass, quit) so branch-specific notifications aren’t skipped incorrectly.
In @.vortex/tooling/src/notify-jira:
- Around line 32-39: The branch gate currently only reads VORTEX_NOTIFY_BRANCH
into $current_branch and ignores a channel-specific fallback; update the
assignment of $current_branch to prefer VORTEX_NOTIFY_JIRA_BRANCH then fall back
to VORTEX_NOTIFY_BRANCH (e.g. use getenv('VORTEX_NOTIFY_JIRA_BRANCH') ?:
getenv('VORTEX_NOTIFY_BRANCH') ?: '') so the check against $notify_jira_branches
via in_array(...) correctly honors the channel-specific environment variable
before defaulting to the generic one.
In @.vortex/tooling/tests/Unit/NotifyNewrelicTest.php:
- Around line 39-63: Add a new unit test in the NotifyNewrelicTest class (e.g.
testDisabledTakesPrecedenceOverBranchFilter) that sets
VORTEX_NOTIFY_NEWRELIC_ENABLED to "false", sets VORTEX_NOTIFY_NEWRELIC_BRANCHES
to "main,master" and VORTEX_NOTIFY_BRANCH to "develop", then call the notify
script and assert the output is "New Relic is not enabled" (use the same helper
used elsewhere, e.g. runScriptEarlyPass('src/notify-newrelic', "New Relic is not
enabled")). This ensures the "enabled" gate in the notify-newrelic logic runs
before the branch allowlist check.
In @.vortex/tooling/tests/Unit/ProvisionTest.php:
- Line 519: The assertion checking that the output does not contain 'Repeating
configuration import.' is non-deterministic because it depends on the external
VORTEX_PROVISION_CONFIG_IMPORT_REPEAT environment variable; update the
ProvisionTest (the test that contains the
$this->assertStringNotContainsString('Repeating configuration import.', $output)
assertion) to force the repeat flag off before invoking the code under test by
setting VORTEX_PROVISION_CONFIG_IMPORT_REPEAT to "0" (or ensuring it is unset)
in the test setup or immediately before the command invocation so the assertion
becomes deterministic.
---
Outside diff comments:
In @.vortex/tooling/tests/Unit/DownloadDbAcquiaTest.php:
- Around line 159-169: Add an assertion in the DownloadDbAcquiaTest path that
verifies Acquia auth headers are not forwarded to the S3 GET request: after
simulating the 302 redirect and the subsequent GET to
'https://acquia-backup.s3.amazonaws.com/backup.sql.gz', inspect the
recorded/mock request for that URL and assert it does not contain the Acquia
auth header(s) (e.g., 'Authorization' and any 'X-Acquia-*' headers). Use the
test's existing HTTP mock/recorder (the same structure used to provide the fake
responses) to fetch the GET request headers and add assertions like
assertArrayNotHasKey for those header names to ensure header-leak prevention in
this flow.
🪄 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: e427a9fe-60fe-4603-9e3f-2bbf367b74ee
📒 Files selected for processing (17)
.vortex/tooling/src/download-db-acquia.vortex/tooling/src/helpers.php.vortex/tooling/src/notify-email.vortex/tooling/src/notify-github.vortex/tooling/src/notify-jira.vortex/tooling/src/notify-newrelic.vortex/tooling/src/notify-slack.vortex/tooling/src/notify-webhook.vortex/tooling/src/provision.vortex/tooling/tests/Unit/DownloadDbAcquiaTest.php.vortex/tooling/tests/Unit/NotifyEmailTest.php.vortex/tooling/tests/Unit/NotifyGithubTest.php.vortex/tooling/tests/Unit/NotifyJiraTest.php.vortex/tooling/tests/Unit/NotifyNewrelicTest.php.vortex/tooling/tests/Unit/NotifySlackTest.php.vortex/tooling/tests/Unit/NotifyWebhookTest.php.vortex/tooling/tests/Unit/ProvisionTest.php
| $notify_github_branches = getenv_default('VORTEX_NOTIFY_GITHUB_BRANCHES', ''); | ||
| if ($notify_github_branches !== '') { | ||
| $current_branch = (string) (getenv('VORTEX_NOTIFY_BRANCH') ?: ''); | ||
| $branch_list = array_map('trim', explode(',', $notify_github_branches)); | ||
| if (!in_array($current_branch, $branch_list, TRUE)) { | ||
| pass(sprintf('Skipping GitHub notification for branch \'%s\'.', $current_branch)); | ||
| quit(); | ||
| } |
There was a problem hiding this comment.
Use the same branch source as the GitHub notifier config.
The gate ignores VORTEX_NOTIFY_GITHUB_BRANCH, so deployments can be skipped incorrectly when channel-specific branch is set without VORTEX_NOTIFY_BRANCH.
Proposed fix
-$notify_github_branches = getenv_default('VORTEX_NOTIFY_GITHUB_BRANCHES', '');
+$notify_github_branches = getenv_default('VORTEX_NOTIFY_GITHUB_BRANCHES', '');
if ($notify_github_branches !== '') {
- $current_branch = (string) (getenv('VORTEX_NOTIFY_BRANCH') ?: '');
- $branch_list = array_map('trim', explode(',', $notify_github_branches));
+ $current_branch = (string) getenv_default('VORTEX_NOTIFY_GITHUB_BRANCH', 'VORTEX_NOTIFY_BRANCH', '');
+ $branch_list = array_values(array_filter(
+ array_map('trim', explode(',', $notify_github_branches)),
+ static fn(string $branch): bool => $branch !== ''
+ ));
if (!in_array($current_branch, $branch_list, TRUE)) {
pass(sprintf('Skipping GitHub notification for branch \'%s\'.', $current_branch));
quit();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tooling/src/notify-github around lines 32 - 39, The gate currently
reads the current branch from VORTEX_NOTIFY_BRANCH only, causing it to ignore
channel-specific env VORTEX_NOTIFY_GITHUB_BRANCH; update the current_branch
determination in the block that uses
getenv_default('VORTEX_NOTIFY_GITHUB_BRANCHES') so it first checks
getenv('VORTEX_NOTIFY_GITHUB_BRANCH') and falls back to
getenv('VORTEX_NOTIFY_BRANCH') (both cast to string), then use that
current_branch when comparing against branch_list (functions/variables to
locate: getenv_default, VORTEX_NOTIFY_GITHUB_BRANCHES,
VORTEX_NOTIFY_GITHUB_BRANCH, VORTEX_NOTIFY_BRANCH, current_branch, branch_list,
pass, quit) so branch-specific notifications aren’t skipped incorrectly.
There was a problem hiding this comment.
Not adopting. This PR is a direct port of the upstream shell scripts/vortex/notify-github.sh, which uses only VORTEX_NOTIFY_BRANCH for the branch filter (introduced in 9a000921). VORTEX_NOTIFY_GITHUB_BRANCH is semantically different in that script — it is the deployment ref (branch name, SHA, or PR ref) passed to the GitHub Deployments API, and in PR workflows it is commonly set to a PR number rather than a branch name. Reusing it as the filter source would cause PR deployments to be skipped incorrectly and diverge from the shell behavior. If this behavior is worth changing, it should be proposed upstream against the shell script first so both implementations stay in sync.
There was a problem hiding this comment.
@AlexSkrypnyk, understood — that's a clear and well-reasoned distinction. VORTEX_NOTIFY_GITHUB_BRANCH serving as a deployment ref (which can be a PR number or SHA rather than a real branch name) makes it unsuitable as a filter source, and keeping the gate aligned with the upstream shell script's use of VORTEX_NOTIFY_BRANCH is the right call. I'll retract the suggestion.
(ᓀ ᓀ)
✏️ Learnings added
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2472
File: .vortex/tooling/src/notify-github:32-39
Timestamp: 2026-04-23T00:43:24.132Z
Learning: In `.vortex/tooling/src/notify-github` (and its upstream shell counterpart `scripts/vortex/notify-github.sh`), `VORTEX_NOTIFY_GITHUB_BRANCH` is semantically different from `VORTEX_NOTIFY_BRANCH`. `VORTEX_NOTIFY_GITHUB_BRANCH` is the deployment ref passed to the GitHub Deployments API and can be a PR number or SHA rather than a real branch name (common in PR workflows). The branch filter gate (`VORTEX_NOTIFY_GITHUB_BRANCHES`) must use `VORTEX_NOTIFY_BRANCH` as the current-branch source — not `VORTEX_NOTIFY_GITHUB_BRANCH` — to avoid skipping PR deployments incorrectly and to stay in sync with the upstream shell script (introduced in commit 9a000921).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2464
File: .vortex/installer/src/Prompts/Handlers/CodeCoverageProvider.php:0-0
Timestamp: 2026-04-20T01:43:30.611Z
Learning: In `.vortex/installer/src/Prompts/Handlers/CodeCoverageProvider.php`, the CircleCI discovery check intentionally uses the narrow substring `codecov -Z -s` rather than a broader match like `codecov `. This is by design: `codecov -Z -s` is the exact line Vortex writes, making it a reliable discriminator. Broadening the match would risk false positives from comments, environment variable references, or documentation lines that embed the word "codecov", silently flipping new projects into the `CODECOV` provider selection.
| $notify_jira_branches = getenv_default('VORTEX_NOTIFY_JIRA_BRANCHES', ''); | ||
| if ($notify_jira_branches !== '') { | ||
| $current_branch = (string) (getenv('VORTEX_NOTIFY_BRANCH') ?: ''); | ||
| $branch_list = array_map('trim', explode(',', $notify_jira_branches)); | ||
| if (!in_array($current_branch, $branch_list, TRUE)) { | ||
| pass(sprintf('Skipping JIRA notification for branch \'%s\'.', $current_branch)); | ||
| quit(); | ||
| } |
There was a problem hiding this comment.
Branch gate should honor VORTEX_NOTIFY_JIRA_BRANCH fallback chain.
Current gating can skip valid JIRA notifications if only the channel-specific branch variable is configured.
Proposed fix
$notify_jira_branches = getenv_default('VORTEX_NOTIFY_JIRA_BRANCHES', '');
if ($notify_jira_branches !== '') {
- $current_branch = (string) (getenv('VORTEX_NOTIFY_BRANCH') ?: '');
- $branch_list = array_map('trim', explode(',', $notify_jira_branches));
+ $current_branch = (string) getenv_default('VORTEX_NOTIFY_JIRA_BRANCH', 'VORTEX_NOTIFY_BRANCH', '');
+ $branch_list = array_values(array_filter(
+ array_map('trim', explode(',', $notify_jira_branches)),
+ static fn(string $branch): bool => $branch !== ''
+ ));
if (!in_array($current_branch, $branch_list, TRUE)) {
pass(sprintf('Skipping JIRA notification for branch \'%s\'.', $current_branch));
quit();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tooling/src/notify-jira around lines 32 - 39, The branch gate
currently only reads VORTEX_NOTIFY_BRANCH into $current_branch and ignores a
channel-specific fallback; update the assignment of $current_branch to prefer
VORTEX_NOTIFY_JIRA_BRANCH then fall back to VORTEX_NOTIFY_BRANCH (e.g. use
getenv('VORTEX_NOTIFY_JIRA_BRANCH') ?: getenv('VORTEX_NOTIFY_BRANCH') ?: '') so
the check against $notify_jira_branches via in_array(...) correctly honors the
channel-specific environment variable before defaulting to the generic one.
There was a problem hiding this comment.
Not adopting, same reasoning as the notify-github thread above: upstream shell notify-jira.sh uses only VORTEX_NOTIFY_BRANCH for the filter, and VORTEX_NOTIFY_JIRA_BRANCH is the branch string used for issue-key extraction (e.g. feature/TEST-123-description), which is not the same as a filterable branch list. Diverging from the shell here would break existing users whose VORTEX_NOTIFY_JIRA_BRANCH carries the issue-key prefix. A semantic change like this needs an upstream shell change first.
There was a problem hiding this comment.
@AlexSkrypnyk, thanks for the clear explanation — that's an important semantic distinction I missed. VORTEX_NOTIFY_JIRA_BRANCH carrying the full issue-key prefix (e.g. feature/TEST-123-description) makes it unsuitable as a branch filter, and deviating from the upstream shell behaviour without a corresponding shell-side change first would indeed risk breaking existing users. I'll withdraw the suggestion.
(´(ｴ)`) ﾉ
✏️ Learnings added
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2472
File: .vortex/tooling/src/notify-jira:32-39
Timestamp: 2026-04-23T00:43:24.389Z
Learning: In `.vortex/tooling/src/notify-jira` (PHP), `VORTEX_NOTIFY_JIRA_BRANCH` is semantically different from `VORTEX_NOTIFY_BRANCH`. `VORTEX_NOTIFY_JIRA_BRANCH` carries the full branch string used for Jira issue-key extraction (e.g. `feature/TEST-123-description`), while `VORTEX_NOTIFY_BRANCH` is the generic deployment branch name used for the branch filter gate. Do not suggest using `VORTEX_NOTIFY_JIRA_BRANCH` as a fallback for the branch filter check — the upstream shell `notify-jira.sh` intentionally uses only `VORTEX_NOTIFY_BRANCH` for filtering.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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 @@
## project/2.x #2472 +/- ##
============================================
Coverage 79.47% 79.47%
============================================
Files 122 122
Lines 6724 6724
Branches 3 3
============================================
Hits 5344 5344
Misses 1380 1380 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…t, deterministic repeat flag.
|
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.
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 |
1 participant
Summary
This PR syncs the PHP tooling package under
.vortex/tooling/src/with recent fixesand features added to the shell counterparts in
scripts/vortex/. It covers threeupstream areas: Acquia DB download authentication hardening, an opt-in repeated
config:importfor the provision script, and per-channel branch filtering for allsix notification scripts.
Changes
Acquia DB download (
src/download-db-acquia)Authorizationheader to S3 (which rejects it). Added a newfollow_redirectsoption to
request()insrc/helpers.phpso the call can opt out ofCURLOPT_FOLLOWLOCATIONand read the target URL frominfo.redirect_url.inspected; only
unlink()after a successful decompression.7a3d7d3a,7f3fb567.Provision (
src/provision)VORTEX_PROVISION_CONFIG_IMPORT_REPEATthat runsdrush config:importa second time after the initial import. Useful when update hooks introduce new
configuration that affects subsequent imports (e.g., new
config_splitsettings).1283c798.Notification branch filter (
src/notify-*)email,github,jira,newrelic,slack,webhook). SetVORTEX_NOTIFY_<CHANNEL>_BRANCHEStoa comma-separated list to restrict notifications to specific branches; empty
(default) disables filtering.
notify-newrelicdefaults the filter tomain,master,developand places the checkafter the "enabled" gate, matching shell.
9a000921,39ef0606.Tests
DownloadDbAcquiaTest- switched backup-URL mocks frombody: {"url": ...}toinfo: {redirect_url: ...}; added assertion that the invalid gzip file is left inplace on failure.
ProvisionTest- addedtestPostOperationsWithRepeatedConfigImportand an extraassertion that the repeat does NOT happen by default.
Notify*Test- addedtestNotificationSkippedWhenBranchNotInFilterandtestNotificationProceedsWhenBranchInFilterto each channel.NotifyNewrelicTest::setUp()now sets
VORTEX_NOTIFY_BRANCH=mainto stay inside the defaultmain,master,developfilter.
Testing
Local
php -lpasses on all 17 changed files. Vendor is not installed locally; rely on CI for PHPUnit.Before / After
Acquia backup URL discovery:
Provision config import sequence when
CONFIG_IMPORT_REPEAT=1:Notify branch filter gate:
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests