REVIEW ONLY — DO NOT MERGE: everything since the last ultrareview (1a32f08..main) - #149
Closed
dknauss wants to merge 27 commits into
Closed
REVIEW ONLY — DO NOT MERGE: everything since the last ultrareview (1a32f08..main)#149dknauss wants to merge 27 commits into
dknauss wants to merge 27 commits into
Conversation
* fix(26-03): stop a delegated editor silently destroying per-user rules Gate 10 of the v1.5.0 release. Probed the authorization boundary with tests instead of reasoning about it, and the probe found a real defect. A delegated Maestro editor (maestro_capability role without list_users) could NOT inject a per-user rule, and could not read display names out of the model — both correct. But they could DESTROY an admin's rules, and not via a crafted POST: get_menu_model() withholds the user axes from them (correctly), so diffItem() never flags those axes client-side, so an item whose only override is a per-user rule is omitted from their full-replace autosave — and a rule omitted from a full replace is a rule deleted. Any edit such an editor made silently wiped every per-user rule they could not see. Ordinary data loss. Round 2's per-item preserve only fired for items PRESENT in the payload, which was never sufficient. The restore now runs over the STORED items instead, re-attaching or re-creating any entry carrying an axis the saver could not touch. My own comment claimed the saver "can neither add nor destroy"; half was true. The probe is kept as PerUserAxisAuthorizationTest rather than deleted — the symptom is a rule quietly disappearing rather than anything failing loudly, which is how this returns unnoticed. Two guardrail tests then failed, and that is the fix working: they removed their rule while acting as the EDITOR, which now correctly no-ops. They author as admin and must remove the same way. Also adds tests/e2e/specs/person-picker-a11y.spec.ts for Gate 9 — 7 checks over surfaces that had never had an independent pass: programmatic label on the search field, four DISTINCT group names (v1.4.0's S1 in this same popover was two groups sharing a name), keyboard-reachable results, per-person accessible names on the chip remove controls, a populated polite live region, focus returning to the field after add and remove, and the focus trap still holding with the new controls. Contrast computed rather than eyeballed: all text 6.83-10.03:1 and the focus ring 5.17:1. Two sub-3:1 borders (#c3c4c7, #dba617) are inherited core tokens — #c3c4c7 already appears 21x in this stylesheet — and neither carries information alone. Recorded as notes rather than "fixed" into an inconsistency with wp-admin. Gate 11 measured cold: name lookup is ONE query for the whole model, the round-2 bounded validation is ONE query rather than a user-table scan, the zero-override path costs 0 queries, and a pathological all-items-targeted config adds +0.108 ms/request — the same order as the entire pre-existing replay cost. Gate: unit 167/167 (223), integration 119/119 (275), JS 83/83, e2e 46 passed / 28 capture-skipped / 0 failed, WPCS clean, PHPStan 0 errors, doc-links clean. Gate 8 (independent code review) is NOT done — requested on the PR. Plan: .planning/phases/26-release-v1.5.0/26-03-PLAN.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(26-03): merge restored per-user axes on the NORMALIZED key Third iteration on the same function, and the second hole in my own fix. Re-attaching a preserved rule under its STORED key is wrong when the payload names the same item in a different but equivalent form — `upload.php?ver=9` for a rule stored under `upload.php`. That is not a contrived input: slug drift is the exact problem Slug::normalize() exists to solve, so it is the expected state after a plugin bumps a ver= string and the client emits the new form. The config then held BOTH keys. They normalize to the same item, so the Axis-1 collision guard resolved to "apply nothing" — silently neutralising the admin's rule AND the delegate's own edit, while the stored option still looked healthy. A rule that is present but inert is worse than one that is missing, because nothing looks wrong. The restore now indexes what is about to be written by normalized key and merges into the equivalent entry instead of adding a second one. Lineage worth recording: round 2 fixed round 1's client-only gate; A3 fixed round 2's payload-scoped preserve; A5 fixes A3's raw-key matching. Each fix was right about the case in front of it and blind to the next. The per-user path now carries four interacting behaviours — reject-on-add, preserve-on-submit, restore-on-omit, merge-on-equivalent-key — which is precisely the shape that wants an independent reviewer rather than another pass by the person who wrote it. Gate: unit 167/167 (223), integration 120/120 (277), WPCS clean, PHPStan 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(26-03): correct the gate summary to two Gate 10 defects, five probes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
main is prepared at 1.5.0 (26-01/02/03 merged), but the independent code review did not happen: Codex hit its usage limit and posted a quota notice with zero reviews and zero inline comments. Recording the hold in both STATE.md and 26-04-PLAN.md so the next person to open either cannot mistake "prepared" for "ready", and cannot mistake an absent review for a clean one. The hold is not procedural tidiness. Gate 10 found TWO real defects in code that had already passed my own review, and the second was a hole in my fix for the first: the payload-scoped preserve was never sufficient, and its replacement re-attached under the raw stored key, so a normalizing-equivalent key left the rule stored but INERT behind the Axis-1 guard while the config looked healthy. That makes three consecutive confident-but-wrong self-assessments of one function, which now carries four interacting behaviours. A fourth pass by the same author is the least informative option available, which is precisely why the tag waits rather than proceeding on gates 9-11 alone. Also notes the alternative explicitly: if the call is to ship without an independent review, that must be recorded as a deliberate choice rather than left to read as a satisfied gate. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Two gaps carried through the v1.5.0 gates. One is now closed; the other is only narrowed, and the difference is stated rather than blurred. MULTISITE — CLOSED. ROLE-02's exemption is `is_multisite() && is_super_admin()`, so a single-site suite never evaluates the second half and the branch was dead code as far as coverage went. A WP_MULTISITE=1 lane now runs the whole integration suite under multisite, wired into CI next to the single-site run — same containers, one extra suite pass, no second environment. Three cases assert every half of the rule: a network super admin IS exempt from the person axis, an ordinary user on the same network is NOT (or the exemption would silently disable the feature on every multisite install), and the role axis still applies to super admins — which is what makes the readme's documented asymmetry true rather than merely claimed. Writing them immediately caught a bad test of my own: it authored the rule while acting as the editor, whom the Gate 10 server-side gate correctly refuses, so it would have asserted against an empty config and "proved" the hide by proving nothing had been saved. Now authored as an admin with an explicit precondition that the rule actually landed. That is the second time this release that a real fix surfaced a test which had been passing for the wrong reason. ACCESSIBILITY — NARROWED, NOT CLOSED. Added axe-core scanning of the popover in BOTH empty and populated states; the chips, results list and live-region messages only exist after interaction, so an empty-state scan would miss most of what this feature renders. Zero violations across wcag2a/2aa/21a/21aa. Scoped to the popover deliberately: wp-admin carries its own pre-existing findings, and a suite that fails on those teaches people to ignore it. This is still not a screen-reader pass. axe catches machine-detectable violations; it cannot judge whether announcements make sense in sequence, whether live-region timing is usable, or whether four similarly-named groups are distinguishable in practice. Recorded as outstanding so no automated green is mistaken for having replaced it. @axe-core/playwright is a devDependency; verified absent from the built ZIP, and npm audit reports no vulnerabilities. Gate: unit 167/167, integration 123/123 single-site + 123/123 multisite, JS 83/83, e2e 47 passed / 0 failed, WPCS clean, PHPStan 0 errors, doc-links clean. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…on path (#128) * fix: address ultrareview findings — collapse the per-user authorization path The review found the invariant my own PR description asked reviewers to hunt for ("the saver can neither add nor destroy") was false on two more paths. Both confirmed by test before fixing. PATH A — MAX_ITEMS starvation, and silent. The restore admitted only on `count($out['items']) < MAX_ITEMS`, so a crafted payload of 200 title-only junk entries filled every slot and left the protected rules nowhere to land. One POST from a saver with no authority to write per-user rules destroyed all of them, with no UI involved. PATH B — the DELETE endpoint bypassed the gate entirely. Rest::reset_config() is gated on capability(), not list_users, and Config::reset() wiped unconditionally. Every sanitize-side round lived on the POST path. A boundary enforced on save but not on reset is not a boundary. RATHER THAN PATCH, COLLAPSED. Preserve-on-submit, restore-on-omit and merge-on-equivalent-key were three mechanisms doing one job, each added to cover a path the previous missed — which is precisely why a fourth path still got through. They are now ONE map, keyed by normalized slug, built before the payload loop by protected_user_axes(), with item-cap capacity RESERVED for it. Starvation is impossible by construction rather than by another guard, and the stored total stays bounded by MAX_ITEMS exactly as before: abusive filler is what gets dropped, never an administrator's rules. Config::reset() uses the same map, so "Reset All" means "reset everything you can affect". An admin is unaffected — they hold list_users, the map is empty, and it stays the plain wipe it always was. Asserted both ways. Also two nits from the review, both real: resetItem() had drifted behind resetSelected() — four fields against six. No production consumer, but its docblock claims to mirror it. The round-trip test passed VACUOUSLY because diffItem() short-circuits on absent keys, so the test could never have caught the drift; it now seeds all four axes and asserts the returned SHAPE, which is what actually pins the claim. Group 1's setSet cleared .maestro-has-hidden on hiddenRoles alone, so unchecking the last role while a person rule remained made the row look untouched until reload. Now matches initModel() and the user group. Gate: unit 167/167 (223), integration 126/126 single-site (285) + 126/126 multisite (290), JS 83/83, e2e 47 passed/0 failed, WPCS clean, PHPStan 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(testing): correct multisite lane counts after the ultrareview fixes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The ultrareview ran against #127 and returned three real findings, all fixed in #128 (707d9b6). It caught the two paths I had missed on the exact invariant the review request asked it to attack: MAX_ITEMS starvation (silent, one crafted POST destroyed every protected rule) and the DELETE endpoint bypassing the gate entirely. Records the outcome rather than just flipping a flag: four consecutive holes in one function, every one found by review rather than by me, and after each fix I believed the path was settled. Also carries forward the caveats that survive — the #128 fixes are themselves unreviewed, there has been no human screen-reader pass, and 21-05's browser verification never happened. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Tag v1.5.0 on 694b1bf; GitHub Release published; SVN deploy dispatched manually and verified FROM SVN — trunk Stable tag 1.5.0, tags/1.5.0/ present and carrying the per-user code (not merely the right version string), assets intact. Marks REL-11 and Phase 26 complete, flips the milestone to shipped, and adds the v1.5 entry to MILESTONES.md. The milestone entry records what this release should actually be remembered for: NINE defects found by verification and review, almost none by the test suite as first written. Most instructive is the run of FOUR consecutive holes in Config::sanitize()'s per-user authorization path — client-only gate, then payload-scoped preserve, then raw-key matching, then item-cap starvation, plus a DELETE endpoint that bypassed all of them. Each fix was correct about the case in front of it and blind to the next, and after each one the path looked settled. It only resolved by COLLAPSING three mechanisms into one, not by adding a fifth guard. The transferable lesson: when a fix keeps needing another fix, the shape is wrong, not the coverage. Also corrects ROLE-02's known-limitation text, which still claimed the multisite exempt branch was untested — the dedicated multisite CI lane closed that. Caveats carried forward rather than dropped at the finish line: the #128 fixes shipped unreviewed, there was no human screen-reader pass, and 21-05's browser verification never happened. A milestone record that quietly drops its caveats is how the next cycle inherits a false premise — which this project has already had to correct once. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Two plans, both autonomous:false. Also removes a stray empty `26-release-v1-5-0/` directory (hyphens) that tooling created alongside the real `26-release-v1.5.0/`. THE SCOPE HAD DECAYED, so 25-01 is an audit rather than an implementation plan. Measured against main @ 2f21818: - Criterion 1 is ALREADY SATISFIED. The toolbar glyphs are #c3c4c7 at 9.11:1, not the reported #3858e9 at 2.83:1 — which is no longer in the CSS at all. v1.4.0's a11y gate found this independently and recorded it as "does not reproduce, closed as stale"; the roadmap was never updated, so it has been reading as open work for a week. - Criterion 2 rests on a false premise. The #2271b1 focus ring measures 3.07:1 on #1d2327 — it PASSES. Widening to #72aee6 (6.74:1) is defensible for robustness, but it is a choice, not a fix, and the plan says so. - Criteria 3 and 4 are unverified: .maestro-status sets flex-shrink:0 but min-width:0, so whether it actually reflows is empirical, not readable. Implementing the list as written would have meant "fixing" a glyph colour that is already correct and replacing a focus ring that meets its bar. This project has already had to correct one stale planning premise mid-phase; catching this one before it costs work is the cheaper version of that lesson. 25-01 is instructed to strike stale criteria WITH the evidence rather than delete them, and — if fewer than two survive — to recommend closing the phase as overtaken by events rather than executing a token version of it. 25-02 implements only what survives, asserts each change with a test that would fail without it, and keeps the commit-on-Enter/blur save model intact (the HARD-03 save-race behaviour depends on it). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…raded, one measured (#132) The phase still justifies itself, but on a materially different scope than it was written with. Evidence in 25-01-SUMMARY.md; roadmap criteria rewritten. STRUCK — criterion 1 was already satisfied. The toolbar glyphs measure 9.11:1 as #c3c4c7; the reported #3858e9 (2.83:1) appears nowhere in maestro.css. v1.4.0's a11y gate found this independently and closed it as stale — the roadmap was never updated, so it read as open work for a week. DOWNGRADED — criterion 2 rests on a false premise. The #2271b1 focus ring measures 3.07:1 on #1d2327: it PASSES. Widening to #72aee6 (6.74:1) is still worth doing, because 0.07 above the floor survives no rendering variance, but it is recorded as a ROBUSTNESS CHOICE rather than a defect fix so it is not re-litigated later as one. CONFIRMED AND MEASURED — criterion 3 is real. Sampling the toolbar 25x across a save cycle: .maestro-status grows 4px -> 24px and the rename field shifts 20px horizontally (x: 230 -> 210) as a direct result. Its own width is stable, so this is pure displacement. flex-shrink:0 prevents compression but min-width:0 reserves nothing. ALREADY IMPLEMENTED — criterion 4's stated remedy exists: Enter-commit produces "Saving… → Saved". The residue is softer than scoped (the confirmation is transient), and is folded into criterion 3, since reserving the slot is most of what "feels unsaved" described. A persistent post-save marker stays an open design question rather than being smuggled in under a layout fix. TRIAGED IN — a11y M2 and M3 both survive; the v1.5.0 axe scanning does not overlap them, because neither is an axe violation. M2 is semantic quality (a natively-disabled checkbox is skipped in SR focus mode, so its lock reason is never heard). M3 is focus management: placePopover()'s outside-click handler drops focus to <body> while Escape restores it. The v1.5.0 a11y spec asserted the Escape path but never outside-click, which is why it passed. Net: two of five original criteria survive, plus two adjacent a11y items — past the threshold 25-01 was given for recommending closure. Had 25-02 run against the original list, its first task would have been recolouring a glyph that is already the right colour. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…nd a11y M2/M3 (#133) Implements exactly the scope 25-01's audit left standing — not the list the phase was written with, two of whose criteria had already decayed. RESERVED STATUS SLOT (audited criterion 1). .maestro-status gets min-width:24px. Measured before: it grew 4px -> 24px as a save cycled and displaced the rename field 20px horizontally (x: 230 -> 210) every single time. flex-shrink:0 stopped it being compressed but nothing reserved the space it needed. This also absorbs the original criterion 4: its stated remedy ("fire the save-status on rename commit") was already implemented — Enter produces Saving… -> Saved — and the real residue was that the confirmation arrived by shoving the toolbar sideways. FOCUS RING #2271b1 -> #72aee6 (audited criterion 2). NOT a compliance fix: the old ring measured 3.07:1 on #1d2327 and PASSED the 3:1 bar. It passed by 0.07, which survives no antialiasing, gamma or future palette nudge, so this is a deliberate robustness margin — recorded as a choice so it is not re-reported as a defect later. New ratio 6.74:1. A11Y M2. A derived-locked checkbox now carries aria-disabled instead of native `disabled`, with the lock reason moved from the accessible NAME to aria-describedby. Native disabled removes the control from the tab order, so a screen-reader user in FOCUS mode never landed on it and never heard why it was locked — the explanation existed and was unreachable. The change handler refuses the toggle and restores the derived value, because making the row focusable also makes it clickable; without that guard this "fix" would have let a display-only row write a derived value into the model, which is worse than the bug. A11Y M3. Outside-click dismissal restores focus to the anchor, mirroring Escape. It previously dropped focus to <body> — WCAG 2.4.3, and an inconsistency INSIDE one component, which users experience as broken rather than merely spare. The tab-trap's disabled filter is unchanged but its comment was: locked rows are now legitimate tab stops, which is the fix working rather than a leak. Six e2e assertions, each written to fail against the pre-fix code. Note the M2 test asserts the NATIVE .disabled property, not locator.isDisabled() — Playwright treats aria-disabled as disabled too, so isDisabled() cannot tell the two states apart, and that distinction is the entire point. Gate: unit 167/167 (223), integration 126/126 single-site (285) + 126/126 multisite (290), JS 83/83, e2e 52 passed/0 failed, WPCS clean, PHPStan 0, doc-links clean. cascade-hide passes unmodified with the M2 change in place. Two full-suite runs were needed: the first showed two unrelated failures that moved and vanished on the second — the known load-flakiness pattern, not a regression. The a11y M2/M3 todo moves to completed. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
There are TWO backlogs and they disagree. SPEC.md's Roadmap and PROJECT.md's V2-xx prose paragraph carry post-1.0 items; todos/pending carries the ones anyone works from. Only 2 of 10 V2 items ever crossed over. The sharp end is SPEC.md item 11 / V2-12, "UI/UX design polish" — control hierarchy, spacing, responsive behaviour, modified-state affordances, status clarity, icon-picker scanability, first-run cues. Phase 23 delivered essentially all of it as UX-13 and shipped it in v1.3.1, but the entry is NOT struck through while items 3-5 beside it are. It reads as open work. That is the identical failure mode as Phase 25's criterion 1, which sat stale for a week and would have had 25-02 recolouring a glyph that was already the right colour. It was caught only because 25-01 was deliberately written as an audit. Nothing is auditing SPEC.md. Two further UX items are real, unbuilt, and have never been schedulable because they exist as clauses in one long prose paragraph: V2-09 (configurable admin-menu width) and V2-10 (admin-toolbar editing research). REQUIREMENTS.md separately defers "UX-11 follow-ups" without naming them, which no one can act on or close. The todo proposes striking what shipped, converting survivors into individual todo files, resolving the UX-11 reference, and — the actual root cause — deciding which list is the system of record and saying so in both. Striking items without fixing that just resets the clock. Surfaced while answering "are you sure we wrangled up all the loose notes about UX changes?" during Phase 25. The answer was no. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The save indicator is event feedback that erases itself: maestro.js ~L1748 sets a 2s timer flipping 'saved' back to 'idle', blanking both the glyph and the SR text. Seconds after a rename commits there is no on-screen answer to "did that save?", and the only recourse is to change something else and watch again. Phase 25 reserved the status slot so it stopped shoving the toolbar sideways — the jarring half. This is the other half, and it is a design change rather than a bug fix, which is why it was deliberately kept out of 25-02 rather than smuggled in under the layout work. Core does not use the self-erasing toast. Block editor persists "Saved" until the document is dirty again; classic autosave persists a timestamp; the Customizer encodes it in the action control; Quick Edit lets the updated row BE the feedback. Gutenberg is the closest precedent because both autosave, so the user never presses Save and needs standing reassurance rather than a moment of it. The proposal keeps it glyph-only (the toolbar is icon-only by design and the slot is 24px) and stays empty until the first save, since claiming "Saved" on entry would be a small lie. Two caveats recorded, both raised in review rather than discovered later: 1. It collides with the existing per-row modified dot, which means "differs from the WordPress default" — NOT "unsaved". Two persistent indicators meaning different things is a decision to take deliberately, and the note lists the options including the one NOT recommended (repurposing the dot would break the documented WCAG 1.4.1 non-colour-signal reasoning). 2. The live region pairs role=status + aria-live + aria-atomic with an explicit speak() on the same transition, risking double-announcement. Pre-existing rather than introduced, but persistence makes it more noticeable and this is the natural moment to listen. Also corrects a worry I had raised out loud: the ERROR state does not self-erase. The timer is gated on `if ( ok )`, so an error correctly persists until the next change. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ystem (#136) Two backlogs that disagreed, now reconciled — and the root cause addressed rather than just the symptoms. STRUCK, with the phase that shipped them. Four entries read as open work while being done: - item 11 / V2-12 "UI/UX design polish" -> Phase 23 UX-13, shipped v1.3.1, with the dark-toolbar follow-ups in v1.5.0 Phase 25 - item 5's "heavier/solid bundled set (V2-11 — outline glyphs read thin)" -> Phase 7 shipped the fill-resolution policy in v1.1; the generated header of includes/icons-bootstrap.php states it outright - V2-13 doc-link hygiene and V2-14 banner pipeline -> Phase 8 (DOC-01, REL-06) CONVERTED into schedulable todos, since they had only ever existed as clauses in one prose sentence: configurable admin-menu width (V2-09), admin-toolbar editing research (V2-10), and the genuinely-remaining icon scope that was buried inside a struck-through item where nobody would find it. Each todo carries the thing that makes it harder than it reads. Menu width would be Maestro's FIRST asset loaded outside edit mode, changing its footprint on every admin page. Toolbar editing has an open question that could return a no-go: whether the cosmetic-only guarantee even holds there, given the sidebar's version rests on core's $_wp_menu_nopriv gate and URL-reachability. SVG upload is a security feature wearing a UI feature's clothes. FLAGGED, not resolved: the "optional enforcement bridge" (item 7) is IN TENSION with REQUIREMENTS' Out of Scope list and the core value — it has Maestro SETTING a capability. It needs a decision, not implementation, and now says so. DELETED: REQUIREMENTS' "UX-11 follow-ups beyond the screenshot recapture". An unresolvable reference — it never said what the follow-ups were, so nobody could act on it or close it. Replaced with a comment explaining the removal. ROOT CAUSE: SPEC.md now declares .planning/todos/pending/ the system of record, with the rule that a shipped item gets struck in the same change. Striking entries without fixing that would just reset the clock — the note cites Phase 25 as the concrete cost, where a stale entry nearly had 25-02 recolouring a glyph that was already correct, caught only because 25-01 was written as an audit. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Records the checkpoint outcome: the changes and resulting UX behaviour were confirmed manually in a running editor. Stated precisely in the verification note, because this project has one checkpoint recorded the other way — 21-05 Task 5 was accepted on automated evidence and the v1.5.0 milestone says so. This one WAS a human pass. The distinction is the whole reason both are written down. Shipped: reserved status slot (the rename field now holds one x position across idle -> saving -> saved; it previously shifted 20px on every save), focus ring 3.07:1 -> 6.74:1, and a11y M2/M3. The verification note also records what the 25-01 audit struck and why it mattered: criterion 1 was already satisfied (#c3c4c7 at 9.11:1; the reported blue was not in the CSS at all), criterion 2 was downgraded from a fix to a choice because the old ring PASSED its bar by 0.07, and criterion 4 was absorbed because its stated remedy already existed. Implemented as written, the phase would have opened by "fixing" a colour that was already correct — the concrete cost of a stale planning entry, and the reason the backlog reconciliation cites this phase. Deliberately not done and logged instead: the persistent saved-state marker. Reserving the slot fixed the jarring half; persistence is a design change and was kept out rather than smuggled in under a layout fix. Also corrects the v1.5 milestone line, which said Phases 22 and 25 "did not land" — true of the release cut, but Phase 25 has now completed post-release and the line would otherwise read as though it were still open. Still open on these surfaces: no human screen-reader pass (axe is clean, and structural assertions prove what AT can reach rather than what it says). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
All three were logged as LOW severity during Phase 20's checkpoint and carried through two releases. None is user-facing; all three are the kind that get worse by sitting. AXIS-2 DRIFT (editor-model-replay-axis2-drift). get_menu_model() applied only the Axis-1 guard (two distinct STORED keys colliding), not the Axis-2 guard replay() enforces (two distinct RENDERED rows normalizing to one key). On a rendered collision replay applied nothing while the popover showed the stored roles checked — and since the editor autosaves a FULL REPLACE built from what it displays, a lying popover is one save away from becoming the stored truth. That is why a "display-only" bug was worth closing. Fixed by folding the Axis-2 skips into the $norm_skip map the resolvers already honour, rather than widening their signatures: an ambiguous key now resolves to nothing in the editor for exactly the reason it does in replay(). The new test was verified FALSIFIABLE — disabling the fold makes it fail, so it is not asserting a vacuous truth. It seeds two live rows (`upload.php?ver=1` and `?ver=2`) that normalize to one key, which is the real-world shape: the same screen registered twice under volatile query params. ENTITY COLLISION (security-qualified-key-entity-collision). Slug::is_qualified() tested the RAW key for '>', but Slug::normalize() html_entity_decodes first — so `a>b` stored as a BARE key and then resolved as a QUALIFIED one, silently becoming a different kind of key than the one that was validated and able to collide with a genuine `a>b`. The qualification decision and the split now use the decoded form. Deliberately narrow: only that decision changed, because "storage stays raw" is the contract normalize() is written against and decoding every key would rewrite `&` slugs FIX-03 already resolves in either form. PREDICATE PARITY (child-role-lock-predicate-parity). The popover re-inlined the lock expression that maestro-logic.js already exports and child-role-lock.test.mjs already covers — so the tested implementation and the running one were separate copies free to drift. Now calls the exported predicate, matching how the rest of maestro.js consumes maestroLogic. Gate: unit 167/167 (223), integration 127/127 single-site (287) + 127/127 multisite (292), JS 83/83, e2e 52 passed/0 failed, WPCS clean, PHPStan 0, doc-links clean. Note: this gate ran on the FALLBACK ports (dev 8890 / tests 8899) because another project's wp-env had taken 8888/8889. e2e needs WP_ENV_TESTS_PORT=8899 to match, which is the pattern STATE.md already records. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…s ROLE-02) (#139) Five plans for ROLE-02's deferred half, the last piece of a requirement that has been marked PARTIAL since v1.5.0. 27-01 IS A DECISIONS CHECKPOINT, not an implementation plan, for two reasons. First, the feasibility note locked storage and resolution in detail (§5c, §7) but explicitly left authoring UX as "the main open design question for a future discuss-phase". Where a profile gets CREATED has no home today — Maestro has deliberately never had a settings screen, and the visibility popover is already at four groups. That is a product decision, and inventing it inside an implementation plan is how a feature acquires a UI nobody asked for. Second, the note predates Phase 21. It cites class-replay.php:299 and :391, both stale after v1.4.1 and Phase 21 moved things. More substantively, it assumes term 3 is a list-intersect like the other two — but membership is a property of the CURRENT USER, not of the item, so the third term probably is NOT the same shape. 27-01 is told that is the most likely place the plan does not survive contact and to work it out concretely before committing. The architecture the note DID lock is carried through unchanged: a `profiles` authoring map that COMPILES onto items[slug].hidden_profiles, so resolution stays in the one seam and the map is consulted only for live membership. Phase 21 built for this deliberately — the seam is independent OR'd terms with the slot reserved and commented at class-replay.php:510, and resolved_override_list() is already field-parameterized. If this phase turns into a restructuring rather than an addition, that is a signal worth stopping on, and 27-03 says so. Each plan carries the lessons its surface already taught, by name rather than as generic advice: idempotence under full-replace autosave and the MAX_ITEMS starvation shape (27-02); the fast path and the edit-mode suspension question (27-03); server-model-client agreement, client-only gates, re-inlined predicates, and the detach-before-outside-click hazard (27-04); the bystander assertion and the fixture/session mechanics hidden-users.spec.ts had to learn (27-05). 27-05 closes ROLE-02 in all three planning files at once — a requirement marked complete in one and partial in another is exactly the drift the V2-backlog reconciliation existed to fix. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…es the width feature (#140) Two things on the same surface, and the second is the reason to log it. The edit-mode toolbar is ~56px of permanently fixed space around icon-only controls — trimmable with straightforward CSS. More significant: forceUnfold() (assets/maestro.js ~L197) does not merely override folded mode, it NEUTERS #collapse-menu with a capture-phase preventDefault() + stopImmediatePropagation(). The control still renders, still looks interactive, still takes focus, and does nothing whatsoever. maestro.css adds width:160px !important as a backstop for the same reason. The intent is sound — a 36px rail has no room for rename fields or drag handles — but the execution is what a user experiences as 'collapse is broken in this plugin'. That matters beyond the papercut because it GATES configurable-admin-menu-width (V2-09), whose own note says it must 'respect folded mode'. Edit mode currently refuses folded mode outright. Those two positions cannot be designed independently without producing a config whose behaviour depends on which screen you are on — so the fold story wants deciding first. Designing width against an unstated conflict is the pattern this project has already paid for twice. Three options recorded, including the cheapest honest one: keep forcing unfold but make the control VISIBLY disabled with a reason, matching the treatment the derived-locked checkbox got in Phase 25. A dead control that looks dead is not a bug. Raised by the user while reviewing the Phase 25 toolbar work. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…link the blocker (#141) Follow-up to #140, replacing assumptions with checked facts and making the conflict impossible to miss from the OTHER side. VERIFIED, so nobody re-derives it: 1. The control is inert, not merely overridden — forceUnfold() registers a CAPTURE-phase listener on #collapse-menu calling preventDefault() + stopImmediatePropagation(), so nothing downstream ever sees the click. 2. There is a CSS backstop as well as the JS: maestro.css forces width:160px !important on #adminmenu/#adminmenuwrap/#adminmenuback plus a matching margin-left on #wpcontent/#wpfooter. Any fold decision must change BOTH layers, not just the JS. 3. The user's folded preference is SAFE. This was an open question in #140; it is now closed. forceUnfold() runs only from init(), which is reached solely in edit mode, and strips the class client-side — so leaving edit mode is a navigation, core re-renders body.folded from user meta, and the MutationObserver dies with the page. Nothing to undo, nothing leaks. 4. It is undocumented outside the code. The behaviour is explained in maestro.js's header and at the function, but appears nowhere in README.md, readme.txt or docs/ — the only other mentions are in docs/archive/FIXES.md, which records a HISTORICAL folded-mode bug and is actively misleading if found while searching for this one. Left deliberately undone: a readme FAQ entry. Documenting "collapse does nothing while editing" as intended, then changing it, costs a second doc edit and a changelog line. Flagged as an open question rather than decided unilaterally. Also cross-links the blocker INTO configurable-admin-menu-width, where it will actually be seen. Stated concretely there: ship both as-is and a site with menu_width:240 renders 240px while browsing and snaps to 160px the moment edit mode opens — the editor showing a different width than the thing it is editing. A pointer in only one direction would have left the width todo reading as ready.
…142) DECISION: keep forcing unfold, stop lying about it, de-hardcode the width. Forced unfold is KEPT and the reasoning is recorded rather than assumed. A folded menu is a 36px icon rail with hover flyouts; Maestro's model is click-a-row and edit its label, so you cannot rename what you cannot read, and submenu editing would happen inside a flyout that vanishes when the pointer leaves. docs/archive/FIXES.md #4 records that the editing UI BROKE in folded mode historically — forcing unfold was the fix, not an oversight. The rejected alternative (adapt the editor to a 36px rail) is recorded too: much larger work whose payoff is editing in a mode where the labels being edited are invisible. What changes is the honesty. #collapse-menu currently renders, takes focus and does nothing, via a capture-phase preventDefault + stopImmediatePropagation. It becomes visibly and programmatically disabled with a reason, matching the treatment Phase 25 gave the derived-locked checkbox: a dead control that LOOKS dead is not a bug, a live-looking one is. I ALSO MIS-FRAMED THE CONFLICT ON 2026-08-09 AND HAVE WITHDRAWN IT. There is no fold-versus-width design conflict. `160px` is hardcoded in three places (maestro.css :22 width, :28 margin-left, :523 the toolbar's left edge), one of which is the exact constant the width feature makes configurable. That is a constant needing to become a variable. Outside edit mode width applies to the expanded menu and folding works normally; inside edit mode folding is off, so width simply applies. The decision UNBLOCKS the width work rather than constraining it, and both todos now say so in both directions. Phase 28 folds the two together, in dependency order: 28-01 is independently shippable and fixes today's defect: one source of truth for the width, and an honest collapse control. It deliberately changes no rendering at the default, because a refactor that also shifts layout by a pixel is two changes wearing one commit. 28-02 crosses the line this feature actually turns on — applying width while merely BROWSING wp-admin. Every asset today is edit-mode-gated except the admin-bar CSS, so "costs nothing unless you are editing" is true now and stops being, for every admin user on every page load. Task 1 is a checkpoint on delivery mechanism and, more importantly, on the OPTION READ — the cost is the non-autoloaded get_option, not the CSS. Measured numbers required, not adjectives. 28-03 adds the control. Flagged there: this is the first GLOBAL setting in a plugin built entirely from per-item overrides, so it must not read as a property of the selected row, and Reset Item versus Reset All semantics need deciding with it rather than discovering later. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Phases 27 and 28 both stalled on the same question from different directions: Maestro has deliberately never had a settings screen, and both features need somewhere to live. Answering it per-phase risked two inconsistent surfaces, then four — precisely the drift the 2026-08-09 backlog reconciliation cleaned up. DECIDED: two icon buttons in the edit-mode toolbar's right zone, each opening a modal — Profiles (create/rename/delete/membership) and Settings (menu_width now; declutter switch and config presets later). Per-item features stay in the per-item panel. No wp-admin settings page, ever. This is consistent rather than novel, on two counts the code already establishes: the toolbar's right zone ALREADY hosts the only menu-wide action (Reset All at maestro.js ~L646-660 — the one control that does not act on the selected item), and the modal idiom ALREADY exists as role=dialog + aria-modal + focus trap in the icon picker, visibility popover and coachmark (:758, :1009, :1942). Both modals reuse that machinery instead of introducing a second dialog pattern. What it protects: a settings page would add an entry to the admin menu, which is self-parodying for a plugin whose purpose is decluttering the admin menu, and it would break the core value's "operates on the menu itself" — the point of which is not having to go elsewhere to configure the menu. GUARDRAIL: two icons is the budget. A fifth menu-wide feature goes INSIDE the Settings modal, not as a third icon. Without that rule the toolbar accretes one icon per feature and becomes the settings screen we just declined to build, arrived at by increments rather than by decision. One deliberate exception recorded for width: it should ALSO be directly draggable at the menu edge, not only a field. 28-01 makes the width a CSS custom property so live preview is nearly free, and dragging the thing you are resizing is more in-place than any field. The field is the precise/accessible path, the drag is the discoverable one — drag-only would be inaccessible, field-only is a settings screen in a costume. Recorded in PROJECT.md's Key Decisions table, written up in full in .planning/DECISION-settings-surface.md, and propagated into 27-01 and 28-03 so neither checkpoint re-opens it. The sub-questions each phase still owns are narrowed rather than deleted.
Session Continuity was badly stale — it still read "Last session 2026-08-02, stopped at Phase 21 context gathered, resume from 21-CONTEXT.md", which predates Phase 21 executing, v1.5.0 shipping, Phase 25, the correctness fixes, and the planning of Phases 27 and 28. Clearing context against that would have lost real continuity, which is the whole thing this section exists to prevent. Rewritten as a START HERE paragraph a fresh session can act on without reconstructing the history: - v1.5.0 is live, but main carries UNRELEASED work (Phase 25 + the three Phase 20 correctness fixes). A v1.5.1 patch is the obvious next release and clears the decks before new phases land on top — this is the single most important fact and was not recorded anywhere a resuming session would look. - Phases 27 and 28 are planned and unblocked, with a recommended order and the reasoning for it (28 is lighter, 28-01 ships alone, and it proves the shared modal shell with a scalar before 27 puts CRUD in one). - Both 2026-08-10 decisions are named with their files, flagged as read-before- reopening: the settings surface, and the fold story — including that the fold-versus-width "conflict" was mis-framed and is withdrawn, so nobody re-derives a constraint that does not exist. - The three carried caveats are restated rather than left only in the milestone entry: #128 shipped unreviewed, no human screen-reader pass, and 21-05's checkpoint accepted on automated evidence. - The wp-env port gotcha is written down, including that Playwright reads WP_ENV_TESTS_PORT independently, and that the other project's containers are not ours to stop. Also updates stopped_at and last_updated, both of which still described the state immediately after the v1.5.0 release.
The 2026-08-01 prior-art spike read Admin Menu Editor architecturally, scoped to Phase 20's questions — identity, apply model, submenu targeting, hook order, storage. Those findings shipped as COMPAT-04/07/14. It never examined AME as a product. Its whole feature-facing output is one "market signals" section, and all four gaps it named are now spent: import/export is queued, reparenting is out of scope, per-role deny shipped, and inline editing UX is the premise rather than a gap. The one artifact meant to say what to build next has nothing unclaimed left in it — and after Phases 27/28 there is no milestone. The todo scopes a second pass over AME free/Pro/add-ons as a product, sorting each feature into have-it / deliberately-not / already-queued / unclaimed / anti-feature. Deliverable is a NOTE on the 19-FEASIBILITY-NOTE model, and "nothing worth adopting" is written in as a legitimate verdict so the pass is not padded to justify itself. Constraints are pre-loaded so a finding gets rejected in the note rather than by a later phase: cosmetic-only, the two-icon budget from DECISION-settings-surface, borrow-patterns-not-code, and editing-happens-on-the-menu. New candidates rank against the existing queue — losing to something already queued is a finding too. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Version strings bumped via bin/prep-release.sh (plugin header, MAESTRO_VERSION, Stable tag, package.json + lock). The changelog is derived from the `v1.5.0..main` DIFF, not the phase list — v1.4.0's gate 8 caught an overclaim by doing exactly that, so it is done first here rather than last. The shippable diff is four files: assets/maestro.css, assets/maestro.js, includes/class-config.php, includes/class-replay.php. Six entries, covering both unreleased code commits: f04dd13 (Phase 25) — the reserved toolbar status slot (measured: the status grew 4px -> 24px and displaced the rename field 20px on every save), the a11y M2 locked-checkbox change (aria-disabled instead of native disabled, so the row stays reachable and its reason is finally announced rather than merely written), the M3 focus return on click-away dismissal, and the lightened focus ring. The focus ring is described as a clarity improvement, NOT a compliance fix. The old #2271b1 ring PASSED WCAG 1.4.11 at 3.07:1; it passed by 0.07, and the change is a deliberate robustness margin. Claiming a fix would be the same species of overclaim gate 8 caught. 4359994 (Phase 20 follow-ups) — the Axis-2 drift fix, written user-facing because its effect was: the popover could show roles checked while replay applied nothing, and autosave's full-replace could then store the lie. Plus the entity collision, kept to one internal line. NOT claimed: the child-role-lock predicate parity change. It routes the popover through the already-tested exported predicate instead of a re-inlined copy — real value, zero behavior change, so it does not belong in a user changelog. Upgrade Notice is 287 chars (Plugin Check limit 300). Gate (local, Docker-free lanes): unit 167/167 (223 assertions), JS 83/83, doc-links clean, composer lint clean (11 files), PHPStan 0 errors. Integration and e2e run in CI. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#147) v1.5.1 verified FROM SVN rather than the wp.org API: trunk Stable tag 1.5.1, tags/1.5.1/ present and carrying the actual 1.5.1 CODE (reserved-slot min-width, the #72aee6 ring, aria-describedby, isChildRoleLockedByParent, the norm_skip fold) rather than merely the right version string, assets/ intact. The more useful finding is the second one. STATE.md has carried "the SVN deploy is not automatic — plan the step in" as a standing lesson for FOUR consecutive releases, framed as something to remember. It is not a discipline problem. wp-deploy.yml declares `release: types: [published]`, and that path has fired zero times in six releases — every deploy in the repo's history is workflow_dispatch. Cause confirmed in source, not inferred: release.yml:36 publishes the Release with softprops/action-gh-release@v3 and no `token:` input, so it authenticates as GITHUB_TOKEN — and GitHub does not create workflow runs from GITHUB_TOKEN- generated events. The trigger cannot fire. The lesson was scar tissue over a workflow that declares automation it is structurally unable to perform. Worth fixing rather than remembering because the failure is quiet in the worst way: tag lands, Release publishes with its ZIP, CI is green, and every visible signal says shipped while users still have the previous version. Todo recommends having release.yml invoke the deploy directly over adding a PAT — the PAT defeats a loop-guard that exists for good reasons, which is a real security tradeoff for a convenience win. It also says not to delete the manual step in the same change that adds the automation, since only a real release can prove the fix. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#148) Reviewed the three items carried out of v1.5.0 rather than restating them, and two of the three moved. 21-05 TASK 5 — STRUCK AS SUPERSEDED. Phase 25 performed a genuine human pass in a live editor (25-VERIFICATION.md, 2026-08-09), and recorded itself as such precisely because this project had one checkpoint recorded the other way. It covered the toolbar and the locked-checkbox row. It did not cover the person picker or the four-group popover — so the residue is real, but it is the SAME residue as the screen-reader item, not a separate one. THE SCREEN-READER ITEM slightly WIDENED and now owns that residue. Phase 25's M2 change altered the same popover AFTER the v1.5.0 axe scan: the derived-locked checkbox went from natively disabled to aria-disabled, so a control that focus mode used to skip is now reachable and refuses its own toggle. That was the right fix — the lock reason was written for assistive technology and could never be heard while the row was skipped — but it changes tab order and announcement sequence in exactly the component that has never had a human pass, and axe passes over it either way. Both now live as one todo, with a concrete six-point script. Neither was ever a todo before; they were prose in STATE.md, which is how they survived two releases without moving. THE #128 GAP IS UNCHANGED AND VERIFIED STILL LIVE. 707d9b6 changed 192 lines of class-config.php plus the logic modules, and that mechanism is intact in main, touched since only by #138's 20-line entity-collision fix. It is the same sanitize path in which the ultrareview found four consecutive holes, and the one round no adversarial pass has seen. Closing it needs /code-review ultra over 1a32f08..707d9b6, which is user-triggered. Also struck one stale entry found while editing: the v1.5.0 milestone's "open, carried deliberately" list still said Phase 25 remains open. It shipped in v1.5.1. Struck in the same change, per the rule the 2026-08-09 reconciliation established. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…h a matrix (#150) The earlier framing was a scoping note that "might legitimately return nothing worth adopting". That under-specified the work. Whether any single feature is worth adopting is a judgement per row — not a reason to skip the survey. The gap actually being closed is that AME's feature surface has NEVER been enumerated; only its architecture has. The deliverable is now a row-per-feature AME<->Maestro MATRIX, built on the R1 conventions in compat/SCHEMA.md rather than invented: fixed cell vocabulary (have / partial / deliberately-not / queued / unclaimed / anti-feature), evidence inline, and a completion check so "comprehensive" is verifiable instead of asserted. Recommendations are a column that falls out of it, not the point. Evidence is its own COLUMN because the rows differ epistemically: free AME is GPL and source-readable, while Pro and both add-ons are only marketing claims, upsell strings and forum reports. Flattening those into one table without marking which is which would launder claims into findings. Coverage is now explicit and checkable — the free build's modules/ directory (skipped entirely by the 2026-08-01 read, and where the discrete features live), Pro, and BOTH add-ons. Branding and Toolbar have never been looked at at all. The four constraints (cosmetic-only, the two-icon settings budget, borrow- patterns-not-code, editing-happens-on-the-menu) moved from gate to filter: they decide whether an `unclaimed` row is a real candidate. A row failing one is marked deliberately-not WITH THE REASON, not dropped — "AME does X and we don't" gets asked repeatedly and should be answered once, in writing. Two sibling todos cross-linked so the matrix feeds them instead of being duplicated: - config-presets-export-import rests on one load-bearing market claim (AME has no named presets even in Pro) checked on 2026-07-03, BEFORE the source read. Flagged for re-verification from the matrix. - admin-toolbar-editing-research gets a sequencing note: AME ships a Toolbar add-on nobody has examined, and its treatment of safely-hideable nodes is direct prior art for that note's hardest question. Soft dependency, not a blocker. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ad trigger (#151) * fix(ci): call the deploy workflow directly instead of relying on a dead trigger wp-deploy.yml declared `release: types: [published]`. That trigger never fired, in six consecutive releases — every deploy in this repo's history was a manual workflow_dispatch, and STATE.md carried "remember the manual step" as a standing lesson across four of them. It is not a discipline problem. release.yml publishes the Release with softprops/action-gh-release@v3 and no `token:` input, so it authenticates as the default GITHUB_TOKEN — and GitHub deliberately does not start workflow runs from GITHUB_TOKEN-generated events. The trigger could not work. Option 1 from the todo, chosen over adding a PAT: a PAT would defeat a loop-guard that exists for good reasons, and buys a convenience win with a long-lived credential holding contents: write on the release path. - wp-deploy.yml gains `workflow_call` with a declared `tag` input and the two SVN secrets; the dead `release:` trigger is REMOVED rather than left in place, since a trigger that reads as automation and isn't is what produced four repetitions - release.yml gains a `deploy` job: uses the local workflow, needs: release, secrets: inherit - workflow_dispatch is deliberately KEPT — it is how you re-deploy a tag whose commit predates these workflows, and how you recover from a half-failed deploy Two details that would bite later: `inputs.tag`, not `github.event.inputs.tag`. The latter is null on a workflow_call and would fall through to github.ref_name — correct today only because the caller is a tag push, and wrong the moment anything else calls this. The concurrency group had the same bug and would have collapsed every release into one group. `needs: release` is the safety property, not just ordering: a failed build, failed tag/version match, or failed Release publish all stop the deploy, so wp.org is never reached by a tag that did not pass the gate. BEHAVIOUR CHANGE: pushing a v* tag now deploys to WordPress.org with no human step in between. That is the fix, but it removes a checkpoint that existed by accident for six releases. The todo stays PENDING and annotated rather than closed — only a real release can prove this, and its verification bar says not to delete the manual step in the same change that adds the automation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): gate the wp.org deploy behind an environment approval Adds `environment: wordpress-org` to the deploy job. The environment was created 2026-08-10 with `dknauss` as a required reviewer. This corrects the trade the previous commit made. Wiring the deploy directly fixes a step that was silently forgotten six times — but on its own it meant a pushed tag publishes to users with nobody in the loop. The ask was automation WITH a final confirm, not without one. Now: tag push -> build -> version check -> Release publish -> the deploy job PAUSES for approval -> SVN. Nothing is forgettable, because the run is sitting there and GitHub notifies rather than never starting. Nothing ships unattended, because it will not proceed unapproved. workflow_dispatch hits the same gate. Required reviewers on environments are free here because the repo is PUBLIC; on a private repo this would need Pro/Team/Enterprise. Worth knowing before anyone reuses the pattern. The sharp edge, commented in place at the `environment:` line and in the todo: the gate is only real while the environment EXISTS with a required reviewer. GitHub auto-creates a missing environment with NO protection rules and the job sails straight through — no error, it simply stops asking. Deleting or recreating wordpress-org without reviewers silently turns this line into a no-op. Not done, logged as optional hardening: WP_ORG_SVN_USERNAME / WP_ORG_SVN_PASSWORD are still repo-level secrets readable by any workflow in the repo. Moving them into the environment would scope them to approved deploy runs only, and needs the values re-entered by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…152) * docs(compat): record the non-autoloaded option as differentiator D4 Dan observed on live sites that Admin Menu Editor's option row can dominate wp_options and slow admin loads. It is a known, acknowledged issue upstream, and Maestro's storage model is the direct counter-position -- but that was recorded nowhere except one cell of the 2026-08-01 architecture table. - PRIOR-ART: add D4 to Differentiate. AME's `ws_menu_editor` is autoloaded, so every request -- front end included, where the data is unused -- pays to fetch and unserialize it, and it grows with the whole admin menu rather than with what the user edited. Evidence is upstream, not inferred: Elsts confirms the autoload behavior in a wordpress.org thread and declines to flip it (an extra admin query), plus three size mitigations across 2.5 / 2.11 / 2.27 -- which is what makes it structural. Maestro's side is stated with the measured numbers and with the trade named honestly (1 extra admin-page query, exactly Elsts' objection; zero front-end cost, which is the bulk of real traffic). Framed as the storage-row consequence of the existing V2 finding, not a new claim. Notes the evidence caveat: the autoload flag comes from the reply and the changelog, not from the 1.15.1 source read. - Feature-sweep todo: the sweep had storage entirely out of scope and would have missed this. Carve out the footprint consequences -- format stays out, but AME's two Settings-tab toggles ("Compress menu configuration data", "Optimize menu configuration size") are user-facing features and earn rows, landing `deliberately-not`. Folds in the open evidence question so the zip read confirms the `update_option` call and upgrades D4 to `source-read`. - class-config.php: the MAX_CONFIG_BYTES docblock said the cap protects "every autoloaded read of it", contradicting the design two methods below it. Say admin-request read, and state that the option is never in `alloptions`. Comment-only code change: php -l and phpcs both clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): sharpen the footprint copy, and pin down where the autoload cost lands Lead the Performance section with the alloptions point instead of burying it fourth, since it is the claim that distinguishes Maestro and the one a user can verify themselves -- `wp option list --autoload=on` does not list maestro_config. Adds the sparse-delta framing (storage tracks edits, not installed-plugin count) and the measured 0.1 ms / 1 ms figures with a link to the method. No competitor is named, and no comparative claim is made about any other plugin -- the copy describes the WordPress mechanism and what Maestro does with it. Also tightens D4 in the prior-art note. "Every request pays to fetch and unserialize it" was loose: wp_load_alloptions() pulls every autoloaded row in one query and holds it in memory, but the option's own maybe_unserialize() runs only when get_option() asks for it. The front-end tax is the bundle -- transfer, memory, and with a persistent object cache a per-request fetch and unserialize of the whole alloptions object, which is where a fat row hurts most. Notes that full-page caching spares requests that never boot WP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): lead the footprint section with the competitive claim, and retag Per Dan: state the performance advantage head-on rather than leaving the reader to infer it from the mechanism. Wording is his. The "nothing to compress, optimize, or tune" clause moves up into it, so it is dropped from the sparse-delta bullet to avoid saying it twice. Tags: drop "admin menu editor" -- guideline 12 treats a competitor's plugin name as spam, and the phrase is already in our plugin TITLE, which the directory weights far above tags, so the SEO cost is close to nil. (Evidence that tags are weak: Admin Menu Editor ranks #1 for "hide admin menu", "menu editor", "admin menu icons" and "hide menu items" while tagging none of them -- it tags only admin/dashboard/menu/security/wpmu.) Filled the slot with "rename menu items" rather than the requested "hide admin menu": Hide Admin Menu is itself a plugin (20k installs), so that swap would have reproduced the exact problem being fixed. "rename menu items" collides with no plugin name, pairs with the existing "hide menu items", and covers the one core capability the tag set had no term for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): work the searched phrases into the description prose The directory indexes the whole readme and weights the title and body far above tags -- AME ranks #1 for "hide admin menu", "menu editor", "admin menu icons" and "hide menu items" while tagging none of them. So the description, not the tag line, is where discoverability is won, and ours led with "orchestrate the appearance of the WordPress admin menu": true to the product, but containing almost none of the literal phrases people type. - Short description now leads with "Hide admin menu items per user role", the highest-intent phrase we can claim. 146 chars, same as before, still under the 150 limit. - Opening bold line names the four capabilities in searchable form (rename / reorder / change icons / hide admin menu items) instead of only "orchestrate". - New second paragraph frames the jobs -- declutter a client site, rename cryptic plugin labels, reorder, hide per role -- so the phrases appear in prose a human wants to read rather than as a keyword list. Guideline 12 bars keyword stuffing; this stays on the right side of that line by describing real use cases. - Adds a one-line performance tease pointing at the footprint section, so the differentiator is visible above the fold rather than only near the bottom. The inline-editing paragraph is unchanged apart from a closing line, since that premise is still the lead story. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Review complete. One finding, confirmed real — a fifth hole in Fixed in #153 (dedupe at the point of write, unconditional). Closing this review-only PR; its base branch is deleted. |
dknauss
added a commit
that referenced
this pull request
Aug 12, 2026
…ize hole (#153) * docs(compat): record the non-autoloaded option as differentiator D4 Dan observed on live sites that Admin Menu Editor's option row can dominate wp_options and slow admin loads. It is a known, acknowledged issue upstream, and Maestro's storage model is the direct counter-position -- but that was recorded nowhere except one cell of the 2026-08-01 architecture table. - PRIOR-ART: add D4 to Differentiate. AME's `ws_menu_editor` is autoloaded, so every request -- front end included, where the data is unused -- pays to fetch and unserialize it, and it grows with the whole admin menu rather than with what the user edited. Evidence is upstream, not inferred: Elsts confirms the autoload behavior in a wordpress.org thread and declines to flip it (an extra admin query), plus three size mitigations across 2.5 / 2.11 / 2.27 -- which is what makes it structural. Maestro's side is stated with the measured numbers and with the trade named honestly (1 extra admin-page query, exactly Elsts' objection; zero front-end cost, which is the bulk of real traffic). Framed as the storage-row consequence of the existing V2 finding, not a new claim. Notes the evidence caveat: the autoload flag comes from the reply and the changelog, not from the 1.15.1 source read. - Feature-sweep todo: the sweep had storage entirely out of scope and would have missed this. Carve out the footprint consequences -- format stays out, but AME's two Settings-tab toggles ("Compress menu configuration data", "Optimize menu configuration size") are user-facing features and earn rows, landing `deliberately-not`. Folds in the open evidence question so the zip read confirms the `update_option` call and upgrades D4 to `source-read`. - class-config.php: the MAX_CONFIG_BYTES docblock said the cap protects "every autoloaded read of it", contradicting the design two methods below it. Say admin-request read, and state that the option is never in `alloptions`. Comment-only code change: php -l and phpcs both clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): sharpen the footprint copy, and pin down where the autoload cost lands Lead the Performance section with the alloptions point instead of burying it fourth, since it is the claim that distinguishes Maestro and the one a user can verify themselves -- `wp option list --autoload=on` does not list maestro_config. Adds the sparse-delta framing (storage tracks edits, not installed-plugin count) and the measured 0.1 ms / 1 ms figures with a link to the method. No competitor is named, and no comparative claim is made about any other plugin -- the copy describes the WordPress mechanism and what Maestro does with it. Also tightens D4 in the prior-art note. "Every request pays to fetch and unserialize it" was loose: wp_load_alloptions() pulls every autoloaded row in one query and holds it in memory, but the option's own maybe_unserialize() runs only when get_option() asks for it. The front-end tax is the bundle -- transfer, memory, and with a persistent object cache a per-request fetch and unserialize of the whole alloptions object, which is where a fat row hurts most. Notes that full-page caching spares requests that never boot WP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): lead the footprint section with the competitive claim, and retag Per Dan: state the performance advantage head-on rather than leaving the reader to infer it from the mechanism. Wording is his. The "nothing to compress, optimize, or tune" clause moves up into it, so it is dropped from the sparse-delta bullet to avoid saying it twice. Tags: drop "admin menu editor" -- guideline 12 treats a competitor's plugin name as spam, and the phrase is already in our plugin TITLE, which the directory weights far above tags, so the SEO cost is close to nil. (Evidence that tags are weak: Admin Menu Editor ranks #1 for "hide admin menu", "menu editor", "admin menu icons" and "hide menu items" while tagging none of them -- it tags only admin/dashboard/menu/security/wpmu.) Filled the slot with "rename menu items" rather than the requested "hide admin menu": Hide Admin Menu is itself a plugin (20k installs), so that swap would have reproduced the exact problem being fixed. "rename menu items" collides with no plugin name, pairs with the existing "hide menu items", and covers the one core capability the tag set had no term for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): work the searched phrases into the description prose The directory indexes the whole readme and weights the title and body far above tags -- AME ranks #1 for "hide admin menu", "menu editor", "admin menu icons" and "hide menu items" while tagging none of them. So the description, not the tag line, is where discoverability is won, and ours led with "orchestrate the appearance of the WordPress admin menu": true to the product, but containing almost none of the literal phrases people type. - Short description now leads with "Hide admin menu items per user role", the highest-intent phrase we can claim. 146 chars, same as before, still under the 150 limit. - Opening bold line names the four capabilities in searchable form (rename / reorder / change icons / hide admin menu items) instead of only "orchestrate". - New second paragraph frames the jobs -- declutter a client site, rename cryptic plugin labels, reorder, hide per role -- so the phrases appear in prose a human wants to read rather than as a keyword list. Guideline 12 bars keyword stuffing; this stays on the right side of that line by describing real use cases. - Adds a one-line performance tease pointing at the footprint section, so the differentiator is visible above the fold rather than only near the bottom. The inline-editing paragraph is unchanged apart from a closing line, since that premise is still the lead story. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(config): store one key per normalized slug, closing a fifth sanitize hole Found by ultrareview against #149 — the review-only PR opened precisely because the #128 round had never been adversarially reviewed. It was the only finding, and it is real. THE BUG Two payload keys that normalize alike (`upload.php` and `upload.php?ver=1`) were both stored. The protected-map merge matches on the NORMALIZED key, claims the entry, and unsets it — so the SECOND equivalent spelling in the same payload found no match, took the no-match branch, and wrote itself as a fresh entry. Two stored keys normalizing to one is exactly the ambiguity Replay's Axis-1 guard resolves to "apply nothing". So a saver WITHOUT list_users could neutralise an administrator's per-user rule with a single POST: the rule stays visibly present in storage and applies nowhere. Same outcome as deletion, invisible to anyone auditing the config. WHY IT SURVIVED FOUR PRIOR ROUNDS The comment directly above the merge already argued this could not happen — matching on the normalized key so an equivalent spelling "lands here rather than producing a second entry later". That reasoning only ever covered the FIRST equivalent spelling. The seam was the unset() after the match. The A5 test covers exactly one equivalent key, so it could never have caught the two-key case. THE FIX Dedupe at the point of write, keyed by normalized slug, UNCONDITIONALLY — not just on the no-list_users path. The attack existed because the collision was constructible at all; blocking it on one path would leave the same seam for a sixth round. First spelling in incoming object order wins, matching the MAX_ITEMS break. Recorded only when an entry is actually stored, so an empty first entry does not shadow a real later one. THREE REPLAY TESTS RE-SEEDED, NOT WEAKENED test_collision_noop_ambiguous_stored_keys_apply_nothing, test_axis1_guard_extends_to_qualified_keys and test_child_hidden_roles_does_not_fire_on_ambiguous_parent all built their fixture by SAVING an ambiguous pair, which sanitize now refuses to write. They now seed via update_option(). The replay-side guard is untouched and still load-bearing: the ambiguity remains reachable from configs written by any version before this fix (i.e. every install upgrading into it), from slug drift where two keys did not normalize alike when saved but do now, and from direct DB edits. Seeding through save() would have asserted sanitize's behaviour while claiming to assert replay's, and gone quietly vacuous. All three re-verified FALSIFIABLE: replacing the guard with first-wins fails all three. A first probe using last-wins only failed two — the third fixture happens to order its entries such that last-wins masks it, which is worth knowing before anyone trusts that shape of probe again. TESTS - A8 (PerUserAxisAuthorizationTest) — the adversarial two-key case. Written first, watched fail, stored keys printed in the failure message. - test_save_stores_one_key_per_normalized_slug (RestConfigTest) — the ordinary admin half, since the dedupe is unconditional. Written after the code, so falsifiability was proven explicitly by disabling the guard. Gate: integration 129/129 single-site (291 assertions) + 129/129 multisite (296), unit 167/167 (223), JS 83/83, WPCS clean, PHPStan 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <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.
⛔ DO NOT MERGE
This PR exists only to give
/code-review ultraa diff to attack. Base is a throwaway branch pinned at1a32f08. Merging it does nothing useful and should never happen. Close it when the review is done.Same pattern as #127, the review-only PR based on the
v1.4.1tag that produced the three real findings fixed in #128.Why this range
1a32f08is the exact commit the last ultrareview ran against. So1a32f08..mainis, precisely, everything no adversarial pass has ever seen:00a2491707d9b6f04dd1343599949caa65dShippable diff: 6 files, +265/−94.
includes/class-config.phpis +212/−94 of that.What to attack, and why this path specifically
Config::sanitize()and the per-user authorization path have had four consecutive holes found in them by review rather than by testing — MAX_ITEMS starvation, the DELETE endpoint bypass, and two before those. After each earlier fix the path was believed settled. It wasn't, four times.707d9b6is the round that collapsed three mechanisms — preserve-on-submit, restore-on-omit, merge-on-equivalent-key — into one normalized-key map with reserved item-cap capacity, so starvation would be impossible by construction andreset()would share the map so the two endpoints could not drift apart again.That collapse is the single most consequential change to this path, and it is the only round no adversarial pass has looked at.
4359994has since modified 20 lines of the same file (the entity-collision fix), so the interaction between the two is also unreviewed.The invariant that must hold
Hiding is cosmetic, always. A rule may never add or remove a capability, and a hidden page must still load by URL for anyone holding the capability it requires. The prior ultrareview was asked to attack exactly this and found two paths through it.
Worth probing:
4359994interact with the map to make a key change identity between validation and resolution?Config::MAX_*bounds: any path that writes before checking?Not in scope
.planning/**churn (planning docs only) and the readme changelog.🤖 Generated with Claude Code