Skip to content

ci: unrot the Windows PHP download and make PRs run the same matrix as master - #362

Merged
armando-rodriguez-cko merged 2 commits into
masterfrom
fix/ci-windows-php-download-and-pr-matrix
Aug 19, 2026
Merged

ci: unrot the Windows PHP download and make PRs run the same matrix as master#362
armando-rodriguez-cko merged 2 commits into
masterfrom
fix/ci-windows-php-download-and-pr-matrix

Conversation

@armando-rodriguez-cko

Copy link
Copy Markdown
Contributor

Why

build-master has failed on every push since 2026-07-30. The two merges earlier today were not the cause; they just made it visible.

The Windows job downloads PHP from a URL pinning the exact patch:

https://downloads.php.net/~windows/releases/php-8.4.23-Win32-vs17-x64.zip

That directory only keeps the current patch of each branch. Once 8.4.24 shipped, this URL began returning 404 and the job died during setup, before running a single test:

Invoke-WebRequest : The remote server returned an error: (404) Not Found.

8.1.34 still resolves today, which is the only reason that job stayed green. It would have rotted the same way on the next 8.1 patch.

Fix 1 — a URL that does not rot

Both downloads now point at windows.php.net/downloads/releases/archives/, which keeps every patch ever published. Verified both files return successfully.

Bumping the PHP patch becomes a deliberate manual edit rather than a time bomb that fires whenever upstream releases.

Fix 2 — PRs run the same matrix as master

The reason nobody caught this: build-pull-request ran ubuntu-latest only. No pull request could exercise Windows, so a Windows-only break was undiscoverable until master was already red.

While aligning the two I found a second problem: the PR workflow installed the distro default PHP for both matrix entries, ignoring php-version. Its 8.1 and 8.4 jobs were testing the same interpreter, so the matrix was reporting coverage it did not have.

build-pull-request.yml is now generated from build-master.yml and differs only in the trigger: same OS and PHP matrix, same setup steps, same checks. Master's Release-commit guard is dropped, having no meaning on a pull request.

Note

No new secrets or workflow variables.

This PR is itself the test of fix 2: its own checks should now include windows-latest on 8.1 and 8.4, which no previous PR in this repo has run.

build-master has failed on every push since 2026-07-30, and the two merges today were
not the cause.

The Windows job downloads PHP from a URL pinning the exact patch, under
downloads.php.net/~windows/releases/. That directory only keeps the current patch of
each branch. Once 8.4.24 shipped, php-8.4.23-Win32-vs17-x64.zip started returning 404
and the job died in setup, before running a single test. 8.1.34 still resolves today,
which is the only reason that job stayed green; it would have rotted the same way on the
next 8.1 patch.

Both URLs now point at windows.php.net/downloads/releases/archives/, which keeps every
patch. Verified both return the file. Bumping the patch becomes a deliberate manual step
rather than a time bomb.

Second half of the problem: build-pull-request ran ubuntu-latest only, so no PR could
ever exercise Windows and this break was undiscoverable until master was already red. It
also installed the distro default PHP for both matrix entries, ignoring php-version, so
its 8.1 and 8.4 jobs were testing the same interpreter.

build-pull-request is now generated from build-master and differs only in the trigger:
same matrix, same setup steps, same checks. Master's Release-commit guard is dropped,
having no meaning on a pull request.

No new secrets or workflow variables.
@agent-wall-e

agent-wall-e Bot commented Aug 18, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:.github/workflows/build-master.yml
  • security_sensitive_path:.github/workflows/build-pull-request.yml

Operational gates

  • ✅ jira_ticket
  • ✅ independent_review

Files analysed: 2


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Aug 18, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_path.github/workflows/build-master.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path.github/workflows/build-pull-request.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

Comment thread .github/workflows/build-pull-request.yml Fixed
@armando-rodriguez-cko
armando-rodriguez-cko requested a review from a team August 18, 2026 16:06
Follows the review bot on #362. SonarCloud flagged "executing downloaded artifacts
without verification" on the PHP and Composer downloads. The finding only appeared
because copying the setup block into build-pull-request.yml made it new code, but it is a
fair point about code that runs with repository secrets, so it is fixed rather than
sidestepped.

Verification, all three downloads:

  PHP zip       pinned SHA-256 per version, checked before Expand-Archive. php.net
                publishes no checksum next to these archives, so the hashes were computed
                from the official download: that buys integrity, not independent
                provenance, and the comment says so rather than overclaiming.
  Composer      (Windows) pinned to 2.10.2 and checked against the SHA-256 that
                getcomposer.org publishes for that exact version. composer-stable.phar
                was a moving target and could not be checksummed at all.
  Composer      (Ubuntu) the installer was piped straight from curl into php. It is now
                verified against composer.github.io/installer.sig first, which is the
                check Composer's own docs prescribe.

Any mismatch throws with both hashes in the message, so a future patch bump fails loudly
instead of silently running something else.

Second change, and the reason the duplication existed: build-pull-request.yml is now a
20-line caller of build-master.yml via workflow_call, instead of a copy. One definition,
so the two cannot drift again, and the download block is not duplicated. The
Release-commit guard is now conditioned on github.event_name == 'push', since a pull
request has no github.event.commits to test.
@armando-rodriguez-cko

Copy link
Copy Markdown
Contributor Author

Updated after the SonarCloud finding, and the finding is fixed rather than sidestepped.

Why it appeared: copying the setup block into build-pull-request.yml made it new code, so the rule fired on a line that had existed in build-master.yml all along. It is a fair point about a job that runs with repository secrets, so all three downloads are now verified:

Download Verification
PHP zip (Windows) Pinned SHA-256 per version, checked before Expand-Archive. php.net publishes no checksum next to these archives, so the hashes were computed from the official download: that buys integrity, not independent provenance, and the comment in the file says so.
Composer (Windows) Pinned to 2.10.2 and checked against the SHA-256 that getcomposer.org publishes for that exact version. composer-stable.phar was a moving target and could not be checksummed at all.
Composer (Ubuntu) The installer was piped straight from curl into php. Now verified against composer.github.io/installer.sig first, which is the check Composer's own docs prescribe.

A mismatch throws with both hashes in the message, so a future patch bump fails loudly instead of quietly running something else.

And the duplication is gone. build-pull-request.yml is now a 20-line caller of build-master.yml through workflow_call, not a copy. That is a better answer to the original problem than what I first pushed: one definition, so the two cannot drift again, and no duplicated download block. The Release-commit guard is now conditioned on github.event_name == 'push', since a pull request has no github.event.commits to test.

Previous run of this PR did prove the point: windows-latest on 8.1 and 8.4 both passed, which no earlier PR in this repo had ever run.

@agent-wall-e

agent-wall-e Bot commented Aug 18, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:.github/workflows/build-master.yml
  • security_sensitive_path:.github/workflows/build-pull-request.yml

Operational gates

  • ✅ jira_ticket
  • ✅ independent_review

Files analysed: 2


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Aug 18, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_path.github/workflows/build-master.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_path.github/workflows/build-pull-request.yml classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@agent-wall-e

agent-wall-e Bot commented Aug 18, 2026

Copy link
Copy Markdown

🔵 Advisory review: Sound, but needs your judgement

This PR needs a human approval. The code itself reads as correct; whether it should land depends on context I don't have.

The PR fixes a rotting Windows PHP download URL, adds integrity checks for both PHP and Composer, and unifies the PR/master CI matrix via workflow_call — all of which look correct. The main judgement call is accepting SHA-256 hashes that were self-computed by the author against the official downloads (no independent provenance), and confirming that secrets: inherit on the pull_request trigger is acceptable for your threat model.

For you to decide

  • The PHP SHA-256 hashes in build-master.yml (8.1.34 and 8.4.23) are described as 'computed from the official download and pinned here' — a human reviewer should independently verify at least one hash against php.net or a trusted mirror before merging, since this is the integrity anchor for code executed in CI.
  • build-pull-request.yml uses secrets: inherit with a pull_request trigger; for public forks this means secrets are exposed to the called workflow during PR runs — this is the standard GitHub Actions footgun and the team needs to confirm the repo is private or that fork PRs cannot trigger this.
  • The Composer signature check on Linux fetches the expected signature from composer.github.io over the network at job time, meaning a MITM or a compromised composer.github.io could supply a matching sig+installer pair; this is the officially recommended check but reviewers should be aware it is not a pinned hash.
  • The truncated diff means the Windows Composer phar section (mentioned in the comment as 'pinned to a version rather than composer-stable.phar') is not fully visible — the reviewer should confirm the Windows path also verifies the phar before executing it and that the pinned version/hash are present.
  • The if condition change from !contains(...) to github.event_name != 'push' || !contains(...) correctly handles the workflow_call case where github.event.commits is empty; logic looks sound.
  • The old PR workflow installed distro-default PHP ignoring the matrix php-version — this bug is confirmed removed by the diff, which is a correctness improvement worth noting.

⚠️ The diff was too large to read in full, so this review covers only part of the change.


This is not an approval. wall-e cannot auto-approve this PR — it is an opinion to help whoever does. Advisory review · us.anthropic.claude-sonnet-4-6 · wall-e 2026.06.19-02

@sonarqubecloud

Copy link
Copy Markdown

@armando-rodriguez-cko
armando-rodriguez-cko merged commit b026b5b into master Aug 19, 2026
6 checks passed
@armando-rodriguez-cko
armando-rodriguez-cko deleted the fix/ci-windows-php-download-and-pr-matrix branch August 19, 2026 09:26
armando-rodriguez-cko added a commit that referenced this pull request Aug 19, 2026
- Add InstrumentDocumentType with bank_statement for bank account payment instrument documents (#360)
- Model the onboarding bank_verification, articles_of_association and shareholder_structure documents as their own classes with their own type constants, instead of the generic Document whose docblock pointed at the identity DocumentType (#361)
- Revert build-master.yml to push-only with its original release guard, and build-pull-request.yml to its pre-#362 content; from #362 only the archive download URLs and the checksum verification remain, which is what fixed the 404
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants