[tracking202]

Stacked on #265 (1.9.79-mcp-rotator-crud, merged in here with a merge commit). Merge #265 first; until then this PR's diff includes #265's.

Sibling of #290 (JF3K), which is also stacked on #265. The two merge cleanly in code. They conflict only where both append: changelogs.txt, 202-config/changelogs.json, and one block each in tests/live/setup-pages.sh after the same anchor. Keep both sides in each. I checked this on a local merge of the two branches:

• tests/Rotator (43 tests);
• RotatorRuleWritesAreCheckedTest;
• both rotator integration suites (53 and 34 tests);
• Go ./cmd;
• setup-pages.sh (335/0), the spellings pass (21/0) and 1.9.79: a redirector rule written on its own needs a criterion, as the page and the set ask #290's live pass (40/0).

Backlog: 3C1D

What changed and why

rtr.php read a criterion value of ALL or all as every visitor; offrtr.php read only ALL. So a rule stored as all sent a visitor to the rule from a redirector link and to the default from a landing page's offer link. Two writers stored all:

• the API before 1.9.79, which stored values as sent;
• Setup › Redirectors' save, which stores what is typed.

#265 makes the API store ALL, but rows already stored keep all.

Measured before the fix on a live instance: one redirector per stored spelling, one rule to /rule, default /default, and a Chrome click through each entry point.

stored	rtr.php	offrtr.php
is ALL	rule	rule
is all	rule	default
is All	default	default
is " all"	default	default
is "United States(US), all"	default	default
is_not all	rule	rule

Where the other spellings came from. The history here goes back to 1.9.29, where rtr.php gained || in_array('all', $values). The first v3 rule write (fc26a2a7) stored (string) ($c['value'] ?? ''): any spelling and any spacing, and '' for a criterion sent without a value. p202 rotator test (go-cli/cmd/verify.go) predicted EqualFold(TrimSpace(value), "ALL"), which is a third reading: it called All every visitor, which neither script did, and missed United States(US), all, which rtr.php would have read had the space not been there.

One reading. Prosper202\Rotator\Criterion reads a stored value as follows:

• split on commas, each value trimmed, the empty ones dropped;
• ALL in any case, alone or among other values, matches every visitor, whatever the statement. is not ALL already matched everyone on both scripts, and still does.

Its readers and writers:

Who	Change
rtr.php, offrtr.php	read the value through Criterion::values() and matchesEveryone(). Nothing else in the click path changed: same reads, same in_array comparisons per type.
API (RotatorsController::criterion())	stores Criterion::normalize(). All and ALL now become ALL too, not only all.
Setup › Redirectors' save (ajax/rotator.php)	stores Criterion::normalize(); it stored all as typed.
Setup › Redirectors' list, Overview › Rotator Breakdown (sort_rotator.php)	list such a criterion as "any visitor", through Criterion::describe(). The Breakdown's value is no longer in bold.
p202 rotator test (verify.go)	predicts what the links do, through the Go twins in go-cli/cmd/rotator_criterion.go.
p202 diff (diff.go)	no longer reports all against ALL, or a list with and without spaces, as a difference between two installs.
GET	still answers values as stored.

Rows already stored (#25). The reader takes every spelling a writer stored, and nothing in the database is rewritten. A rule is rewritten in canonical form only when it is next saved, by the page or the API.

What this changes for stored rows.

• all: offer-link clicks now go to the rule. Redirector-link clicks did already.
• All, all, United States(US), all: both links now send every visitor to the rule; neither did before. This is the decision a reviewer should check, below.
• United States(US), Canada(CA), stored with the space by the old API: now matches Canada. This is 1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265's "found outside" item.

"None". I found no writer that stored a "none" criterion value. The nearest is '', an empty value from the old API. It is now an empty list, so is matches no visitor and is_not every visitor, as before, with one exception: an ISP criterion is "" matched every visitor while MaxMind ISP lookups were off (in_array(null, ['']) is true). It now matches none.

ClickPathReads (#27). The criteria read in both scripts is unchanged. It is a read by rule id, from the redirector's own active rules (RuleOrder::activeRules), and no read or join was added.

Docs. documentation/api/12-rotators.md (the criterion value row), docs/openapi.yaml (the criterion value, and setRotatorRules), and both changelogs.

Tests

• tests/Rotator/CriterionTest (unit). Every spelling in tests/fixtures/rotator-criterion-values.json is checked for its values, its every-visitor reading and its normalized form, and normalizing twice changes nothing. That fixture lists the 13 spellings writers stored, each with its writer.
  • It also checks describe().
  • Structural: every served file that names 202_rotator_rules_criteria is listed, so a new reader fails until it is listed. Each reader must call Criterion:: and must contain no string literal ALL in any case; offrtr.php's own 'ALL' is how the two scripts came to differ.
• Go:
  • TestCriterionValuesReadAsThePHPTreeReadsThem reads the same fixture through repoPath(); the fixture is added to go-cli.yml's paths, both lists.
  • TestRotatorDestinationReadsEverySpellingOfAll.
  • TestDiffComparesCriteriaAsTheRedirectsReadThem.
  • TestOnlyTheCriterionReaderSpellsAll, an AST scan that refuses a literal ALL/All outside rotator_criterion.go. Lower-case "all" names the --all flags and is not looked for.
• tests/live/rotator-criterion-spellings.sh (new). One redirector per stored spelling, written into the row as an older writer left it. A click goes through rtr.php and one through offrtr.php for each, and every spelling sends the visitor to the rule; a value that only resembles ALL (Mall) does not.
• tests/live/setup-pages.sh: the page saves a criterion typed all as ALL and lists the rule as "If any visitor".
• Updated:
  • RotatorRuleSetIntegrationTest::testValuesAreStoredAsThePageStoresThem now also covers All and Chrome, aLL;
  • IpCriterionTest, which pins the page's write statement, now pins the new one.

Plants: 11, each caught by a named test. Backups were mktemp copies checked with cmp, the tree was restored, and the run was repeated on the final tree.

1. rtr.php with its own ALL/all check again. Caught by CriterionTest and the spellings pass (All, all, the list).
2. offrtr.php with its own ALL check again. Same tests, all included.
3. Criterion case-sensitive.
4. Criterion not trimming.
5. The API storing only lower-case all as ALL. Caught by the integration test and CriterionTest.
6. The page storing as typed (setup-pages.sh).
7. The page's list as stored (setup-pages.sh and CriterionTest).
8. The Breakdown as stored.
9. rotator test back to its whole-value EqualFold.
10. diff comparing raw values.
11. The Go reader not trimming.

Live pass

Instance. A local MariaDB 10.11 / PHP 8.3.6 instance on this branch, port 8291. It was installed by running install.php's own steps without the licence form; the licence stub is down.

The licence check was made unreachable. For the signed-in pages, the server's proxy points at a closed port, which AUTH::is_valid_api_key() treats as a network failure and lets through.

What could not run here. The Go CLI answers 402, so p202 rotator test and p202 diff ran only in Go unit tests.

Results.

• tests/live/rotator-criterion-spellings.sh: 21 passed, 0 failed. Every stored spelling sends the visitor to the rule through both rtr.php and offrtr.php; Mall goes to the default on both.
• Overview › Rotator Breakdown, read as the page draws it: the six spellings' rules are listed "If any visitor", Mall as "If Country is Mall", and the browser rules as before.
• tests/live/setup-pages.sh: 331 passed, 0 failed (1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265 reported 328).
• tests/live/offer-rotator-routing.sh: 30/0.
• tests/live/rotator-breakdown.sh: 10/0.
  • One run failed first, with a "Deleted rule" counting 2 old clicks. setup-pages.sh TRUNCATEs the rotator tables and leaves 202_clicks_rotator, so a reused instance hands new redirectors ids that old clicks name.
  • It passed once AUTO_INCREMENT was moved past them (see "Found outside").

Verification

.claude/skills/p202-verify/scripts/verify.sh --since origin/1.9.79 --changed, run on the final tree before the commit (everything staged), PHP 8.3.6 with the full vendor/, schema tier on a scratch MariaDB 10.11 database. The unit tier ran 6119 tests, 4 skipped (the documented "User class requires DB singleton" ones). The live instance's 202-config.php was moved out of the checkout for the run.

Scope report
------------
  actionlint SKIP  actionlint not installed (brew install actionlint, or the download script CI uses)
  go         PASS  ran under CI's go1.25.14 via GOTOOLCHAIN (local go is 1.24)
  golangci   PASS  ran under CI's go1.25.14 via GOTOOLCHAIN (local go is 1.24)
  openapi    PASS
  patterns   PASS
  phpcs      PASS
  phpstan    PASS
  schema     PASS
  syntax     PASS
  unit       PASS

Not covered by this script:
  live      stand up an instance, drive the path a user would take
  agent     add and run a case under tests/fixtures/agent-eval/cases/
  ci        a workflow still in_progress has not passed; read the run

Report the lines above verbatim. Do not write "tests pass".

Then actionlint v1.7.7 was built into a scratch directory (go install github.com/rhysd/actionlint/cmd/actionlint@v1.7.7), and verify.sh --tier actionlint was run with it on PATH: actionlint PASS.

CI status: not yet observed.

What could not run here

• The Go CLI against the instance. It gets 402 without the licence stub. rotator test and diff ran only in Go unit tests.
• CI's MySQL 8.0. Not run here: this sandbox has MariaDB.
• Agent evals. None was added; the change is in the click path and how two CLI commands read a value.

For the reviewer to check

1. ALL in any case, and values trimmed. This moves traffic for stored rows that were not ALL/all. A rule stored as All, all or United States(US), all matched no visitor on either link and now matches every visitor on both. A stored United States(US), Canada(CA) now matches Canada.
   • Why: in each case the stored value says what the rule was meant to do; p202 rotator test already told users All meant every visitor; and the writers now store these spellings as ALL, so reading them as anything else would make a saved-again rule change meaning.
   • If only all should change, make Criterion::isEveryone() compare ALL and all exactly, and drop the trimming in values(). The fixture's cases say which rows move.
2. is not ALL stays "every visitor". That is what both scripts did, and the docs now say it.
3. The page now normalizes on save (all → ALL), so the editor shows ALL after a save. It already trimmed in its JavaScript.
4. The Rotator Breakdown's criterion line is now plain text through Criterion::describe(), where the value was in <strong>.

Review follow-up: ALL on an IP criterion (9I0R, 3de05fa)

This answers Codex's P2 on RotatorsController.php ("Allow ALL in IP criteria"). It is the defect this PR first listed under "Found outside".

Both links read a criterion holding ALL as every visitor, whatever its type. The writers, though, checked an IP value with IpCriterion::refusal() first, which refused ALL as "not an IP address". So the following could not be written, nor could a stored IP=ALL rule be sent back as GET answered it:

• the set PUT;
• the single-rule POST and PUT, which share planRule() → ruleParts() → criterion();
• the Redirectors page.

The changes:

• API. criterion() returns a value holding ALL before any type's own check is made. It reads the value with Criterion::values() and matchesEveryone(), the same reading the redirects use, so all, All, 203.0.113.7, ALL and US, ALL are taken on any type and stored with ALL as ALL. A country or device list beside ALL is no longer refused for its form either.
• Page. Its check and write go through the new Criterion::refusal() and Criterion::stored().
• Test. RotatorRuleSetIntegrationTest::testAValueHoldingAllIsTakenWhateverTheType stores an IP=ALL rule, reads it with GET and sends it back through the set PUT (answered unchanged, rows untouched) and the single-rule PUT. Before the fix it failed with Invalid rules.0.criteria.0.value. CriterionTest holds refusal() and stored() to the same spellings.
• Plants: 3, each caught by a named test.
  • API short-circuit removed: caught by the integration test.
  • Criterion::refusal() ALL handling removed: caught by CriterionTest and the page pass.
  • Criterion::stored() ALL handling removed: caught by CriterionTest.
• Live.
  • setup-pages.sh 333/0; it posts an IP criterion of all and checks that ip|ALL is stored.
  • Spellings pass 24/0, including an IP rule stored as All on both links.
  • offer-rotator-routing.sh 30/0 and rotator-breakdown.sh 10/0.
  • Over HTTP, a stored IP All rule went through GET and then PUT twice, answering 200 both times.
• Ladder on the final tree: every tier PASS except actionlint SKIP (no workflow changed). The unit tier ran 6127 tests, 4 skipped.
• 1.9.79: a redirector rule written on its own needs a criterion, as the page and the set ask #290. It needs no commit of its own: its single-rule writes reach criterion() through planRule(), and a trial merge of the two branches has the fix on both paths. The conflicts are still only the two changelogs and setup-pages.sh.

Found outside this item (one line each)

• rtr.php compares an ISP criterion with the first comma-part of getIspData(), offrtr.php with the whole answer, so an ISP rule can match on one entry point and not the other.
• p202 rotator test reads a bare country code (US) as that country, while the redirects never match one (countryCodeFromValue).
• tests/live/setup-pages.sh TRUNCATEs the rotator tables but not 202_clicks_rotator, so on a reused instance new redirectors get ids old clicks name, and tests/live/rotator-breakdown.sh then counts those clicks.

---

Generated by Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-28T12:31:12.114376Z	4904aee	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

Code review: FINDINGS

For 8dab59b: 0 issue(s). The review is encrypted for the maintainers in the review-findings artifact of this run.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8dab59b923

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Adversarial review: NON-BLOCKING

For 4904aee: 4 finding(s). The report is encrypted for the maintainers in the review-findings artifact of this run.

Rewritten on every push; the check fails on BLOCKING.