[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.

Backlog: JF3K

What changed and why

POST /rotators/{id}/rules and PUT /rotators/{id}/rules/{ruleId} checked only the entries they were sent. A rule with no criterion therefore answered 200:

• p202 rotator rule-create without --criteria-json sent no criteria key;
• {"criteria": []} on a PUT cleared a rule's criteria.

rtr.php and offrtr.php read a rule with no criterion as every visitor ($count == count($rotate) is 0 == 0), so one call sent all of a redirector's traffic to that rule. Setup › Redirectors and #265's PUT /rotators/{id}/rules refused such a rule.

Measured before the fix (this branch at the #265 merge, live instance):

• POST of {"rule_name", "redirects"} answered 200 and stored 0 criteria;
• three clicks through rtr.php (Firefox, Chrome, an unknown agent) and two through offrtr.php all went to the rule's URL, not the default;
• PUT …/rules/{id} with {"criteria": []} answered 200, and a Chrome click then went to a rule that had been Firefox-only.

The fix: one check of a rule. createRule() and updateRule() now go through planRule(), the check the set already used, inside their transactions after the redirector lock. The three API writes refuse the same rules, with the same field errors in the same words (a test compares them field by field):

• a rule_name;
• at least one criterion (ALL matches every visitor, said in so many words);
• at least one destination;
• one destination unless it is a split test;
• a weight on each destination of a split test, at least one above 0.

A PUT of one rule keeps what it leaves out, as stored and unchecked, as destination() already keeps a held destination: a rename does not re-validate an old criterion. The whole-rule checks, though, are asked of the rule the write would leave, kept parts included, so {"splittest": 0} on a rule with two destinations is a 422.

Rows already stored with no criterion (#25). Decided:

• The click scripts go on reading them as every visitor. That is what they always did, so traffic does not move on upgrade. Nothing in the click path changed.
• GET does not gain a field. "criteria": [] is the mark, and the docs, openapi and the refusal now say what it means. A new read-only key would have to be accepted back by the set, the single PUT, the MCP schema and every client that builds a body from a GET, for a legacy state only.
• Setup › Redirectors' list says "If any visitor (no criteria)" where it said "If no criteria", so the rule can be found on the page.
• A write that would leave such a rule with none is a 422 that says so: "This rule has no criterion, which the redirects read as every visitor; … send its criteria … or remove the rule". This covers a single PUT (a rename included) and a set sending its GET body back. The page refused saving such a rule already.

CLI paths.

• Go rotator rule-create refuses, before any request, a rule with neither --criteria-json nor --country, or neither --redirects-json nor --redirect-campaign. rule-create and rule-update also refuse an empty list. All are validation errors (exit 1) with a hint naming the ALL criterion. This also stops an old server, which would store such a rule, from being sent one.
• PHP CLI rotator:rule:create refuses the same before sending. Its help example "value":"US" (a bare code the server refuses) now reads United States(US).

MCP. It has no single-rule tools; rotator_set_rules already goes through the set.

Staged writes. A staged POST is applied through the same route, so the apply answers the 422 (live, below).

Sync: all of a redirector's rules or none (6d31aa3). A self-review after the adversarial review marked this PR BLOCKING found that this PR turned a source rule the target refuses into a destructive sync:

• SyncEngine::resyncRotatorRules() deleted every rule on the target, then POSTed the source's rules one at a time.
• A legacy rule with no criterion, which this PR's POST now refuses, stopped the sync half way.
• Measured on this branch's previous head: a target holding "kept" and "kept too" was left with only "france". A redirector the sync created got some of its rules.
• With 1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265's controller, the same sync copied the rule and succeeded.

Now SyncEngine sends a redirector's rules as one set (PUT /rotators/{id}/rules). The target takes them all or refuses them with nothing written, and each field error names the source rule:

• A target older than 1.9.79, which has no set route (404), is still written one rule at a time.
• So is a source redirector with no rules, since a set cannot be empty.
• tests/Api/V3/SyncRotatorRulesIntegrationTest (5 cases) runs the real SyncEngine against two accounts, each served by the real RotatorsController, with only the HTTP exchange replaced.
• Live: a real POST /sync/jobs and run against two accounts left the target's rules as they were. The error named the rule (rules.1.criteria: the source rule "everyone": A rule needs at least one criterion…). Once that rule had an ALL criterion, the next sync copied both rules.

The page. Its save (tracking202/ajax/rotator.php) refused a rule posted with no criteria list only after a foreach over the missing key. The answer was two PHP warnings and then ERROR, so the editor showed its generic message by accident. It now checks both lists first and answers ERROR alone. The page keeps its own checks, which the API's do not replace (see "For the reviewer").

A fourth write path. tests/Api/V3/RotatorRuleWritesAreCheckedTest (structural, unit suite) checks two things:

• every served file whose SQL writes 202_rotator_rules, _criteria or _redirects is listed with what makes it safe. It reads strings as PHP assembles them, joined across ..
• every public RotatorsController method that reaches such a write, through any number of the class's calls, is listed. A "checked" one must have the write inside its transaction() closure, below a statement directly in that closure (not in an if, a loop or a try) whose call reaches planRule().

It plants each shape it claims (8 controller plants, 4 string spellings). It does not see a table name built at runtime (none of the tree's unread-table writes reaches these tables), a write in another class, or whether the listed page's checks are right.

Docs.

• documentation/api/12-rotators.md: a new "One rule" section, and the rule fields table.
• docs/openapi.yaml:
  • createRotatorRule now requires criteria and redirects, with minItems: 1;
  • both single-rule writes say how they are checked;
  • the documented POST answer was 201 with a rule; it is 200 with the redirector, which is what the route has always answered;
  • RotatorRule.criteria says what empty means.
• docs/cli.md, docs/cli-agent.md and go-cli/cmd/COMMAND_CONTRACTS.md.
• Both changelogs.

Tests

• RotatorRuleSetIntegrationTest, 16 new cases, on a scratch MariaDB:
  • testEachRuleWriteRefusesWhatTheSetRefusesInItsWords: every case of the set's refusal provider, sent as a new rule (POST) and as the whole of an existing rule (PUT). Each is compared field by field, message by message, with the set's answer for the same rule; nothing is written.
  • testAPutOfOneRuleIsCheckedAsTheRuleItWouldLeave: a missing criteria key, null, [], {"splittest": 0} with two kept destinations, and a split destination with no weight are refused. A switch sent alone keeps the stored lists.
  • testARuleStoredWithNoCriterionStaysAndAWriteOfItSaysWhatItDoes: a legacy row is still tried by RuleOrder::activeRules; a rename, and the set sent back, are refused with the message; a criterion is taken.
  • testAPutOfOneRuleDoesNotCheckWhatItLeavesOutAgain.
• SyncRotatorRulesIntegrationTest (6d31aa3), 5 cases:
  • a refused rule leaves the target's rules as they were;
  • a redirector the sync creates gets every rule or none;
  • the target takes the source's rules in order;
  • a target without the set route is written one rule at a time;
  • an empty source list leaves the target with none.
• Updated fixtures that made rules with no criterion, or two destinations without a split test:
  • RotatorDestinationsIntegrationTest;
  • RotatorRuleSetIntegrationTest::testARuleAddedOnItsOwnGoesLast, which had worked around the gap;
  • RtrClickInstanceTest's spare rule, which bumps rule ids;
  • tests/live/rotator-breakdown.sh rule A, now splittest: 1; clicks split by weight either way;
  • tests/live/offer-rotator-routing.sh's all-zero-weights rule, now created with weights and set to 0 in SQL, as an older write stored it;
  • NestedBodyValuesAreCheckedTest's registry: refuseNestedKeys() folded into planRule().
• New: tests/live/setup-pages.sh cases (the page refuses a rule with no criterion and one whose criterion is empty, with ERROR alone; it lists a stored one as "any visitor"); tests/Cli/Commands/RotatorRuleCreateCommandTest; Go TestRotatorRuleWithNoCriterionIsRefusedWithTheWayToSayEveryVisitor and six new cases in TestRotatorInputErrorsAreValidationErrorsBeforeAnyRequest (exit code and hint asserted).

Plants: 17, 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. The controller as 1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265 shipped it.
2. createRule() without planRule().
3. updateRule() without planRule().
4. Kept lists left out of the whole-rule check.
5. A PUT re-checking what it leaves out.
6. The legacy message dropped.
7. setRules() planning before its transaction. Caught by the structural test, and by the set's race tests.
8. An unlisted file writing criteria.
9. The page's new check removed (setup-pages.sh: "a rule with no criterion is refused").
10. The page's text reverted.
11. Go rule-create without the presence check.
12. Go ruleListError() disabled.
13. The PHP CLI check removed.
14. The sync back on one rule at a time.
15. The sync's 404 fallback removed.
16. The sync's empty-list fallback removed.
17. The source rule's name dropped from the sync's error.

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 (schema, account, API key) without the licence form; the licence stub is down.

The licence check was made unreachable. For the signed-in pages, the server ran with its HTTP proxy pointed at a closed port. AUTH::is_valid_api_key() treats that as a network failure and lets the session through, which is what the app does on a real outage.

What could not run here. The Go CLI answers 402 on this instance, so p202 rotator rule-create and rule-update were not run live; Go unit tests cover them.

Results.

• JF3K pass: 40 passed, 0 failed. Everything was created through the REST API with curl; clicks went through rtr.php (a tracker link) and offrtr.php (record_adv.php, then ?rpi=).
  • Refusals. POST with no criteria key → 422 naming criteria, and POST with criteria: [] → 422. No rule stored; both scripts send to the default.
  • all is taken. POST with value: "all" → 200, stored ALL; both scripts send every agent to the rule.
  • PUT criteria: [] → 422, and the criterion stays. PUT with a Firefox criterion → Firefox goes to the rule on both scripts, Chrome to the default.
  • PUT splittest: 0 with two destinations kept → 422; the rule stays a split test.
  • Staged. A staged POST with no criterion is staged (202); its apply → 422, and no rule is written.
  • Legacy rule. One inserted with no criterion, as the old POST stored it, still takes Chrome and Firefox on both scripts. GET shows criteria: []. A rename → 422 with the "This rule has no criterion, which the redirects read as every visitor" message. Giving it a Chrome criterion → Chrome goes to it, and Firefox goes past it to the Firefox rule, on both scripts.
  • PHP CLI (bin/p202, live): rotator:rule:create without --criteria-json → exit 1, nothing sent; with one → stored.
• Sync (6d31aa3), a separate instance on port 8189: described under "Sync" above.
• tests/live/setup-pages.sh: 332 passed, 0 failed (1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265 reported 328; the 4 new cases are the page refusals and the "any visitor" listing).
• tests/live/offer-rotator-routing.sh: 30/0.
• tests/live/rotator-breakdown.sh: 10/0.
• Instance suites:
  • RtrClickInstanceTest: 3 tests, 36 assertions. The spare-rule branch ran: a rule with two redirects bumped the ids.
  • RotatorRuleSetInstanceTest: 2/29.
  • SetupPermissionsInstanceTest: 5/297.
• Integration suites on a scratch database:
  • RotatorRuleSetIntegrationTest: 53 tests;
  • RotatorDestinationsIntegrationTest: 34 tests;
  • after 6d31aa3: SyncRotatorRulesIntegrationTest 5, ClickRotatorFields 2, RotatorBreakdown 3.

Verification

.claude/skills/p202-verify/scripts/verify.sh --since origin/1.9.79 --changed, run on the final tree before the first commit (everything staged), PHP 8.3.6 with the full vendor/, schema tier on a scratch MariaDB 10.11 database. The unit tier ran 6124 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
------------
  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".

For 6d31aa3, verify.sh --changed passed patterns, phpcs, phpstan, schema, syntax and unit (6250 tests, 4 skipped).

CI status: not yet observed on 6d31aa3.

What could not run here

• The Go CLI against the instance. It gets 402 without the licence stub. rule-create and rule-update were exercised by Go unit tests against a fake server only.
• Agent evals. The cases drive the Go CLI. No case was added: the agent-facing change is a refusal the CLI makes before any request, held by the Go tests.
• CI's MySQL 8.0. Not run here: this sandbox has MariaDB.

For the reviewer to check

1. A legacy rule with no criterion cannot be renamed or paused through the API until it is given a criterion or deleted. That is what the set and the page already ask, and the message says how. If pausing alone should pass, it is one condition in planRule().
2. The single-rule writes now refuse everything the set refuses, not only the missing criterion. That covers two destinations without a split test, a split destination with no weight, and every weight 0. A client that relied on POSTing such a rule now gets a 422; two live scripts in this repo did, and are updated.
3. The page keeps its own checks. ajax/rotator.php was not routed through planRule(). Its posted shape differs, and its checks differ in both directions: it refuses any 0 weight, and the API refuses only all-zero. The structural test lists it with that reason. Unifying them is a larger change (see below).
4. Single-error messages changed. A single-rule refusal's top-level message is now the set's Invalid <fields> form, where it was e.g. Unknown criterion type. The field errors and their words are unchanged.
5. POST's documented status. POST is documented as 200 here (it never answered 201).
6. A sync of a legacy no-criterion rule is refused, names the rule and writes nothing (6d31aa3). The alternative is to send it as country is ALL, which the redirects read the same way. Both comparators (SyncEngine and the Go diff) would then need to treat an empty list as equal to ALL.
7. The set route caps a redirector at 200 rules, so a source with more than 200 now fails to sync, where the old path posted them all.

Found outside this item (one line each)

• PUT /rotators/{id}/rules/{ruleId} writes a rule's criteria and redirects afresh with new ids (the set keeps them), so a click report row that names the redirect it was sent to loses it after a single-rule PUT of redirects.
• Setup › Redirectors' save refuses a split destination weighted 0 (array_filter drops '0'), while the API and the set accept 0 and refuse only all-zero.
• tests/live/setup-pages.sh writes its artifacts to /tmp via a bare mktemp -d, not a directory the caller chooses.
• SyncEngine sends 'status' => $rule['status'] ?? 1, so a rule stored with a NULL (inactive) status becomes active on the target.
• The set's refusal "splittest is 0, so the rule sends every visitor it matches to one destination" (from 1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265) does not match the redirects: rtr.php and offrtr.php never read splittest and split by weight whenever a rule has more than one usable destination.
• The sync's delete-then-post rule copy is fixed in this PR (6d31aa3).

---

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	⚠️ Failed 2026-10-28T15:05:59.031784Z We couldn’t complete this review. Comment @codex review to try again.	6d31aa3	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: CLEAN

For 8623213: 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: 8623213d6e

ℹ️ 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".

[chatgpt-codex-connector]

Run the position migration for existing 1.9.79 installs

Installations that already ran the parent revision record version 1.9.79, so the surrounding if ($prosper202_version == '1.9.78') skips this newly added reconciliation and leaves 202_rotator_rules.position absent. After deploying this commit, API reads, reports, and the Setup page query/order or write that column and therefore fail, while only the click-path helper has a missing-column fallback. Move this schema addition to a new versioned upgrade step or add an idempotent reconciliation that also runs for databases already marked 1.9.79.

Useful? React with 👍 / 👎.

[tracking202]

This line isn't part of this PR's own change. It comes from #265, which this PR is stacked on: git blame puts line 4437 in 11260af, #265's commit. The same finding is open on #265's thread, where I set out why I'm not adding a second migration and left the decision to the owner.

In short:

• 1.9.79 isn't tagged. git ls-remote --tags origin 'v1.9.79*' prints nothing, so no released install has 1.9.79 recorded.
• Every real upgrade runs this rung. An install coming from 1.9.78 takes the 1.9.78 → 1.9.79 step, position included.
• Only a development or staging install can miss it. That means one that ran the 1.9.79 branch before 1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265 merged. CLAUDE.md Fix PHP 8+ compatibility issues throughout Prosper202 #33 freezes only a tagged version's rung, and 1.9.79's is still open.

If the owner wants that case covered too, the change belongs in #265 and this PR would pick it up by merge. Leaving this thread open for the owner's call on #265.

---

Generated by Claude Code

[github-actions]

Adversarial review: NON-BLOCKING

For 6d31aa3: 6 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.

[tracking202]

The adversarial review marks bf0f0a8 BLOCKING (9 findings), so its check is red.

The report is encrypted for maintainers, so I can't read which finding blocks. For context, two neighbouring PRs that cover the same code came back NON-BLOCKING:

• 1.9.79: redirector create, update, delete and whole-rule-set writes in the MCP connector #265, which this PR is stacked on.
• 1.9.79: a PUT of one rule keeps the ids of the redirects it keeps #307, which contains this PR in full.

So this may be reviewer variance, but I'm not assuming that. A self-review of this PR's own change (8623213) is under way. Each real defect it finds will be fixed here, with a test that fails without the fix, and merged into #307. I'll report the result on this thread.

If a maintainer can share the blocking finding's title, I can target it directly.

---

Generated by Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[tracking202]

The self-review found one real defect in this PR's own change, and 6d31aa3 fixes it. The review report is still encrypted, so I can't confirm it's the finding the adversarial review blocked on; the re-run on this push will show.

• Defect: this PR made p202 sync strip a target redirector's rules. The server-side resync deleted every rule on the target first, then posted the source's rules one at a time. A rule stored before 1.9.79 with no criterion, which this PR's POST now refuses, stopped the sync half way. On the previous head, a target holding "kept" and "kept too" ended up with only "france".
• Fix: the sync now sends a redirector's rules as one set (PUT /rotators/{id}/rules), so the target takes all of them or none, and the error names the source rule. A target older than 1.9.79, which has no set route, is still written one rule at a time.
• Tests: SyncRotatorRulesIntegrationTest (5 cases) runs the real SyncEngine against the real controller. 4 planted defects were each caught.
• Live run: a real sync job left the target's rules intact and named the rule. Once the source rule had an ALL criterion, the next sync copied everything.
• Merged up: into 1.9.79: a PUT of one rule keeps the ids of the redirects it keeps #307 (8f5a314).

The description is updated, including two decisions for the reviewer and two new items under "Found outside".

---

Generated by Claude Code