chore: apply the findings of the 2026-08-03 repository review - #4
Merged
Conversation
'src' and 'tests' were literal paths, not globs, so they matched nothing. Only '**/*.php' actually triggered the workflow, which meant a change to tests/Fixture/config/enum_tag.yaml could alter what the integration tests assert without running them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A moved tag or a compromised upstream account silently changes what runs here. technote-space/assign-author is the one that matters most: it holds issues: write and pull-requests: write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
php-extensions only reached the php-actions/composer image. The runner PHP that setup-php provisions is the one that runs phpstan, phpcs, rector and phpunit, and composer.json requires ext-sodium. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cron is '0 3 1 * *' — the first day of the month, not every night. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The composer block used reviewers, which Dependabot deprecated in favour of assignees, and the github-actions block had no commit message prefix or PR limit. Actions are pinned to SHAs now, and Dependabot bumps those the same way it bumps a tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A prototyped node always reports a default value, so an omitted "keys" never reached the requiresAtLeastOneElement check and fell through to the extension instead, where it failed with a different exception than the same group with an explicitly empty list. isRequired() puts it where key_prefix and active_key already were. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The example called encryptWithAdditionalData on properties the section above declares as Encryptor and Decryptor, where the method does not exist. Copied into a project running PHPStan it failed with method.notFound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tural checks Empty additional data was only asserted on the encrypt side, and the write-only anonymous encryptor had no direct test for isEncrypted() or needsReEncryption() — the two methods a rotation sweep needs from a group that can never read what it wrote. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Applies all six findings of the deep repository review of
811d5db, plus three side findings noticed while verifying them. Every finding was re-verified against the working tree before being touched.Review findings
'src'and'tests'were literal paths, not globs, so a change totests/Fixture/config/enum_tag.yamlcould alter what the integration tests assert without triggering CI. Filters are nowsrc/**andtests/**.main— not in this PR, it is a repository setting. See below.keysnode fell through the config component and failed in the extension with a different exception than the same group with an explicitly empty list.isRequired()puts it wherekey_prefixandactive_keyalready were, with a regression test for the omitted case.encryptWithAdditionalDataon properties declared asEncryptor/Decryptor, where the method does not exist. Verified: the old snippet fails PHPStan withmethod.notFound, the new one does not.technote-space/assign-authorfirst — it is the only write-scoped one.WriteOnlyAnonymousAsymmetricEncryptor::isEncrypted()/needsReEncryption()had no direct test. Both covered.Side findings
shivammathur/setup-phphad noextensions:, so the runner PHP that actually runs phpstan/phpcs/rector/phpunit was provisioned withoutext-sodium, whichcomposer.jsonrequires.0 3 1 * *runs monthly. Renamed to match the schedule.reviewers(deprecated in favour ofassignees) and the github-actions block had no commit prefix or PR limit.Quality gates
composer validate --strict,composer audit,composer phpstan(level 10),composer phpcs,composer rector:checkall pass. PHPUnit: 88 tests, 186 assertions, up from 84/177 — four new regression tests.Still open: F-02
Branch protection was deliberately left out of this PR. Note the interaction with F-01: because
ci.yamlkeepspaths:filters, a PR touching onlyREADME.mdorCHANGELOG.mddoes not run CI, and required status checks would make such a PR unmergeable. Decide that before enabling protection — either droppaths:from thepull_requesttrigger, or add the documentation paths.🤖 Generated with Claude Code