ℹ️ About Codex in GitHub

\\n
\\n\\n[Your team has set up Codex to review pull requests in this repo](https://chatgpt.com/codex/cloud/settings/general). Reviews are triggered when you\\n- Open a pull request for review\\n- Mark a draft as ready\\n- Comment \\\"@codex review\\\".\\n\\nIf Codex has suggestions, it will comment; otherwise it will react with 👍.\\n\\n\\n\\n\\nCodex can also answer questions or update the PR. Try commenting \\\"@codex address that feedback\\\".\\n \\n

\",\"path\":null,\"side\":null,\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"Fixed in 0ed46041 — and you were right about the cause being structural, not local.\\n\\nThe settle polled `#root`'s `inert`, which **is** the sweep's own outcome, so `a11y-inert-sweep-removed` died there and the CDP assertions were never reached. Worse, that poll was itself the previous review round's fix for a passive-effect race, so the fix created the hole.\\n\\nIt now polls `document.body.style.overflow`. `pushModal` sets it on the line *before* `syncModalBackground()` and `popModal` restores it on the line before its own call, so it proves the effect ran while asserting nothing about what it did — a settling signal must never be the thing under test.\\n\\nVerification: `a11y-inert-sweep-removed` now dies at `is still in the accessibility tree with a dialog open`, i.e. the CDP assertion, not the poll.\\n\\nTwo things I did beyond the literal fix:\\n\\n- That signal is sound for **one** dialog and false for a nested one — `pushModal` sets overflow unconditionally, so a second open would settle on the first poll proving nothing, and `popModal` only restores when the stack empties, so an inner close never settles. The helper documents that rather than stating the property absolutely.\\n- A review of this fix then found the mirror gap: nothing covered the `never returned to the accessibility tree` assertion, because this mutant only breaks the *marking*. Added `a11y-inert-never-lifted` for the *un-marking*.\",\"path\":\"tools/simulation/ui/specs/a11y-live-regions.spec.ts\",\"side\":\"RIGHT\",\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"Correct, and the correction matters — my comment claimed the observer made the bound non-load-bearing, which it does not. It removes the timing dependence *between* reads; inspecting the array is itself a moment, and a write scheduled after it still escapes. Fixed in 0ed46041.\\n\\nI took the second option you offered. There is no deterministic completion signal for an arbitrary regression's latency — for the real close path (microtasks and one React render, no timers) any bound is enormous, but a regression can schedule whatever it likes. So the drain is now 1200ms and `a11y-announcer-writes-late` validates it by writing 600ms after the last close: measured against a mutant rather than guessed.\\n\\nGetting that mutant right took two attempts, and the harness caught the first one, which is the part worth reporting. Version one wrote 600ms after **every** close, and the new `EXPECT_MSG_FOR` check reported `WRONG ASSERTION`: cycle one's delayed write was still on screen when cycle two's in-loop read happened, so it died at `re-announced a standing warning after dialog cycle 2` and left the final drain exactly as unvalidated as before. Firing once on the second close puts it after the 250ms in-loop settle and inside the 1200ms drain, which is the only window where the cumulative assertion is the one doing the work. It now dies at `was written to during the dialog cycles`.\\n\\nKnown and deliberate: the mutant is coupled to the spec running two cycles, and its margin before the in-loop read is ~350ms, which is the tighter side. Both failure modes surface as `WRONG ASSERTION` rather than a silent mis-score, and `mutation-check.sh` is local-only, so the cost is a re-run, not a false green.\",\"path\":\"tools/simulation/ui/specs/a11y-live-regions.spec.ts\",\"side\":\"RIGHT\",\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"Right, and this one was the worst of the three because the comment read as a correction. Fixed in 0ed46041.\\n\\n`GREP_FOR[a11y-inert-sweep-removed]` is now `leave the accessibility tree|recorded browser facts`, so both tests run under that mutant and both must go red. Verified: the run reports two failures, and the browser-facts precondition is one of them.\\n\\nThe comment itself had been wrong twice in opposite directions, which I have recorded in place rather than just fixing:\\n\\n1. It first claimed *no mutant of this app can break it* — false; the FACT 2 precondition is product behaviour and does break.\\n2. Corrected to *it goes red under that mutant* — also false, because the `-g` filter never executed it. **A true statement about code that never runs is not coverage**, and that is the sentence I most want the next reader to see.\\n\\nSince this was the third same-shaped finding in a row, I changed the method rather than the list. `mutation-check.sh` now has an `EXPECT_MSG_FOR` table: a mutant declares a substring of the failure it must die on, and a kill anywhere else is reported as a survivor rather than counted. Opt-in, because a wrong expectation would demote a genuine kill — populated for the six a11y mutants, left unset for the twelve older ones with that disclosed in the comment rather than backfilled by guessing.\\n\\nIt earned itself immediately: it caught my own new late-write mutant dying at the wrong assertion (see the thread above), and an agent review then found a fourth instance the harness would now also catch — nothing covered the `never returned to the accessibility tree` assertion, so `a11y-inert-never-lifted` was added for the un-marking half of the sweep.\\n\\nFull run after all of it: baseline GREEN, 17 killed, 0 survivors, restore GREEN.\",\"path\":\"tools/simulation/ui/mutation-check.sh\",\"side\":\"RIGHT\",\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"@codex all three findings are addressed in `0ed46041`, replies on each thread.\\n\\nSummary, because the three turned out to be one defect wearing three hats — a mutant killing an assertion **earlier** than the one its name claims, leaving the named assertion uncovered while the run printed a clean kill:\\n\\n- **Settling signal** now polls `document.body.style.overflow` (set by `pushModal` on the line before `syncModalBackground()`) instead of `#root`'s `inert`, which was the sweep's own outcome. The inert mutant now reaches and dies on the CDP assertion. Scoped in the docstring to the single-dialog case, since it is false for a nested stack.\\n- **Final drain** is 1200ms and validated by a new `a11y-announcer-writes-late` rather than guessed.\\n- **`GREP_FOR`** for the inert mutant runs the browser-facts test too, so the precondition a comment claimed goes red actually executes.\\n\\nBeyond the literal fixes, two things I would like you to attack specifically:\\n\\n1. **`EXPECT_MSG_FOR` in `mutation-check.sh`** — the method change. A mutant declares a substring of the failure it must die on; a kill elsewhere is now reported as a survivor. It is opt-in (six a11y mutants populated, twelve older ones deliberately not, disclosed in the comment). Is opt-in the wrong call? Can a loose or duplicated expectation string accept a bad kill? Two mutants share `was written to during the dialog cycles` — I believe that is safe because it is the only assertion either can legitimately die on, but that is exactly the kind of reasoning that has been wrong three times here.\\n\\n2. **`a11y-inert-never-lifted`**, added after a review of the fix found a fourth instance of the same shape: nothing covered the `never returned to the accessibility tree` assertion, because the existing inert mutant only breaks the *marking*, never the *un-marking*. Please check whether any assertion in `specs/a11y-live-regions.spec.ts` still has no mutant reaching it. That question has produced a real finding four rounds running, so I would rather you asked it again than assumed it is now exhausted.\\n\\nFull run: baseline GREEN, 17 killed, 0 survivors, restore GREEN.\",\"path\":null,\"side\":null,\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"**_{_{![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)}} Require an expected assertion for every counted kill**\\n\\nWhen a mutant has no `EXPECT_MSG_FOR` entry, `want` is empty and any assertion failure is still counted as `KILLED`; therefore all twelve older mutants remain vulnerable to the exact earlier-assertion false kill this method was added to prevent, while the final headline presents them as verified coverage. Treat an absent expectation as unverified/surviving and populate the table from observed runs rather than making the safety check opt-in.\\n\\nAGENTS.md reference: [AGENTS.md:L180-L184](https://github.com/mforce/cluckwork/blob/0ed46041447389adcca872c905d8540254b29c18/AGENTS.md#L180-L184)\\n\\nUseful? React with 👍 / 👎.\",\"path\":\"tools/simulation/ui/mutation-check.sh\",\"side\":\"RIGHT\",\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"**_{_{![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)}} Verify both failures produced by the multi-test inert mutant**\\n\\nUnder `a11y-inert-sweep-removed`, the widened grep now runs both the announcer test and the recorded-browser-facts test, but this single expected substring belongs only to the first test; once that assertion fails, the run is reported as `KILLED` even if the browser-facts precondition passes or stops executing. The fresh evidence after the prior review is that broadening `GREP_FOR` alone does not enforce the comment's claim that both tests must go red; this mutant needs both expected messages to be required.\\n\\nAGENTS.md reference: [AGENTS.md:L180-L184](https://github.com/mforce/cluckwork/blob/0ed46041447389adcca872c905d8540254b29c18/AGENTS.md#L180-L184)\\n\\nUseful? React with 👍 / 👎.\",\"path\":\"tools/simulation/ui/mutation-check.sh\",\"side\":\"RIGHT\",\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"**_{_{![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)}} Mutate each announcer independently**\\n\\nBoth inert mutants affect the farm and update announcers together, but this loop evaluates the farm announcer first, so `a11y-inert-sweep-removed` dies there before checking the update announcer; the restoration loop has the same ordering under `a11y-inert-never-lifted`. Consequently the update announcer's open and close AX assertions can become vacuous while all 17 mutants still report killed; add update-specific marking and un-marking mutants, or otherwise make each selector's assertion independently killable.\\n\\nAGENTS.md reference: [AGENTS.md:L180-L184](https://github.com/mforce/cluckwork/blob/0ed46041447389adcca872c905d8540254b29c18/AGENTS.md#L180-L184)\\n\\nUseful? React with 👍 / 👎.\",\"path\":\"tools/simulation/ui/specs/a11y-live-regions.spec.ts\",\"side\":\"RIGHT\",\"line\":245}{\"type\":\"pull_request_comment\",\"body\":\"\\n### 💡 Codex Review\\n\\nHere are some automated review suggestions for this pull request.\\n\\n**Reviewed commit:** `0ed4604144`\\n \\n\\n

ℹ️ About Codex in GitHub

\",\"path\":null,\"side\":null,\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"Agreed, and this was the finding I most needed. Fixed in `3b4434a5`.\\n\\nOpt-in was the wrong call for the reason you give: the headline said `killed: 17` and a reader takes that as seventeen verified guarantees, when twelve of them were verified only to the standard the table exists to replace. That is the same overstatement as the false-kill problem this script already had, which makes it worse — the file had the lesson written in it and I reproduced it anyway.\\n\\nNow required. A mutant with no declaration is reported `UNVERIFIED` and excluded from the killed count, with its own line in the verdict so it cannot hide in the total.\\n\\nAll eighteen entries were **copied from observed runs**, per your \\\"populate from observed runs\\\" — never guessed, since a wrong expectation demotes a genuine kill. The residual weakness, stated in the file: three mutants trip assertions with no custom message (`expect(locator).toBeVisible() failed`), so for those the pinned text is Playwright's `Locator:` line. That is genuinely weaker than a message — it pins which locator was being asserted, not which claim — and I would rather name that limit than paper over it.\\n\\nI also exercised the new branch on purpose rather than shipping it unexercised: removed one entry, confirmed the mutant reports `UNVERIFIED`, restored it, confirmed the same mutant reports `KILLED` again.\\n\\nFull run: baseline GREEN, 17 killed, 1 false kill, **0 unverified**, 0 survivors, restore GREEN.\",\"path\":\"tools/simulation/ui/mutation-check.sh\",\"side\":\"RIGHT\",\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"Correct — widening `GREP_FOR` made both tests *run*, which is not the same as requiring both to *fail*, and the comment claimed the latter. Fixed in `3b4434a5`.\\n\\n`EXPECT_MSG_FOR` entries are now newline-separated lists and **every** line must appear in the log. `a11y-inert-sweep-removed` declares three:\\n\\n```\\nmain.content > p.sr-only[aria-live=\\\"assertive\\\"] is still in the accessibility tree with a dialog open\\n#root > p.sr-only[aria-live=\\\"polite\\\"] is still in the accessibility tree with a dialog open\\nthe injected probe is a body child but the modal sweep did not inert it\\n```\\n\\nThe third is the browser-facts precondition. So the sentence in that comment is now a thing the harness checks on every run rather than a thing a reader has to believe.\\n\\nWorth noting how this interacts with your third finding: requiring several messages only works if the spec actually evaluates several assertions, and it did not — the announcer loop died on the first one. `expect.soft` in both loops is what makes the first two lines above independently reachable. The two findings had to be fixed together; either alone would have produced a requirement that could never be met, which would have shown up as a permanent `WRONG ASSERTION`.\\n\\nVerified: `a11y-inert-sweep-removed` produces all three messages and is counted as killed; the count of `WRONG ASSERTION` reports across the full run is zero.\",\"path\":\"tools/simulation/ui/mutation-check.sh\",\"side\":\"RIGHT\",\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"This is the sharpest of the three and I had not seen it. Fixed in `3b4434a5`.\\n\\nThe loop asserted both announcers, so it *read* as covering both — but every mutant that breaks the sweep breaks both, the farm announcer is evaluated first, and a hard `expect` ends the test there. So the update announcer's open and close assertions could not be killed by anything, in the test whose title names them. Deleting them, or having them assert the opposite, would have changed no mutation result.\\n\\nBoth loops now use `expect.soft`, so each selector is judged independently and both failures reach the log, and the two inert mutants are **required** to produce both messages (the multi-line `EXPECT_MSG_FOR` from your other thread). That is what makes the update announcer's assertions killable rather than merely present.\\n\\nI considered your first suggestion — update-specific marking/un-marking mutants — and it does not work here: both announcers live inside `#root`, `inert` is inherited and cannot be opted out of by a descendant, so no DOM mutant can expose one without the other. Relocating the element out of `#root` would break the selector and kill at `inDom` instead, which is the wrong assertion again. Soft assertions get the same coverage without inventing a DOM state the app can never be in.\\n\\nVerified: under `a11y-inert-sweep-removed` the log now carries the failure for `main.content > p.sr-only[aria-live=\\\"assertive\\\"]` **and** for `#root > p.sr-only[aria-live=\\\"polite\\\"]`; same for `a11y-inert-never-lifted` on the restoration loop.\\n\\nThat is the fifth round in which the same defect shape turned up — an assertion that no mutant reaches. I have stopped treating each one as a fix and started treating the detection as the deliverable, which is what the required `EXPECT_MSG_FOR` is for.\",\"path\":\"tools/simulation/ui/specs/a11y-live-regions.spec.ts\",\"side\":\"RIGHT\",\"line\":245}{\"type\":\"pull_request_comment\",\"body\":\"@codex round 2 addressed in `3b4434a5`, replies on each thread.\\n\\nAll three were about the verification method, not the specs, and you were right on each:\\n\\n- **`EXPECT_MSG_FOR` is now required.** A mutant with no declared assertion is reported `UNVERIFIED` and excluded from the killed count, on its own line in the verdict. All eighteen entries copied from observed runs; the three that pin a `Locator:` line rather than a message are named in the file as the weaker case.\\n- **Entries may list several messages, all required.** `a11y-inert-sweep-removed` must produce both announcer failures *and* the browser-facts precondition — so the comment that claimed both tests go red is now checked rather than believed. It had been wrong three times.\\n- **Both announcer loops use `expect.soft`.** The update announcer's assertions were unkillable, since every sweep mutant breaks both and the farm one always failed first. Update-specific mutants are not possible — `inert` is inherited and a descendant cannot opt out — so soft assertions are the way to make each independently killable.\\n\\nI also exercised the new `UNVERIFIED` branch deliberately (removed an entry → `UNVERIFIED`; restored it → `KILLED`), because an unexercised safety branch is precisely what these rounds keep turning up.\\n\\nFull run: baseline GREEN, 17 killed, 1 false kill, 0 unverified, 0 survivors, restore GREEN.\\n\\n**The question I would most like you to ask again**: is there any assertion in `specs/a11y-live-regions.spec.ts` that no mutant reaches? It has produced a real finding in five consecutive rounds — the settling poll, the anti-nag loop, the browser-facts precondition, the un-inert path, and now the update announcer. I would rather you assumed it is still not exhausted.\\n\\nSecond, narrower: the `expect.soft` change means a failure no longer stops the test. Please check I have not made some *later* assertion in that test unreachable-but-passing as a result — that would be the same defect wearing a sixth hat.\",\"path\":null,\"side\":null,\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"**_{_{![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)}} Wait for the modal effect before reading the probe**\\n\\nThis test reads the probe's AX state immediately after `toBeVisible()`, even though `waitForModalEffect` documents that dialog visibility settles the render but not the passive effect that applies `inert`. On a slower run, the CDP snapshot can therefore precede the sweep and fail this precondition against correct product code; call `waitForModalEffect(page, true)` before the AX read, as the first test does.\\n\\nAGENTS.md reference: [AGENTS.md:L65-L65](https://github.com/mforce/cluckwork/blob/3b4434a50f8667b1e4db11dd24372d149e90b283/AGENTS.md#L65-L65)\\n\\nUseful? React with 👍 / 👎.\",\"path\":\"tools/simulation/ui/specs/a11y-live-regions.spec.ts\",\"side\":\"RIGHT\",\"line\":433}{\"type\":\"pull_request_comment\",\"body\":\"**_{_{![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)}} Add a mutant that can fail the AX control**\\n\\nNone of the six a11y mutants can make this load-bearing control false: the sweep-removal mutant leaves the dialog accessible, the never-lifted mutant acts on the background after close, and the other four only write announcer text. Consequently, if this control stopped detecting an incomplete/broken AX snapshot, all declared mutant messages would still appear and the run would remain clean even though the central absence assertions could false-pass; add a mutant that removes the dialog control from the AX tree and require this assertion's message.\\n\\nAGENTS.md reference: [AGENTS.md:L180-L184](https://github.com/mforce/cluckwork/blob/3b4434a50f8667b1e4db11dd24372d149e90b283/AGENTS.md#L180-L184)\\n\\nUseful? React with 👍 / 👎.\",\"path\":\"tools/simulation/ui/specs/a11y-live-regions.spec.ts\",\"side\":\"RIGHT\",\"line\":257}{\"type\":\"pull_request_comment\",\"body\":\"\\n### 💡 Codex Review\\n\\nhttps://github.com/mforce/cluckwork/blob/3b4434a50f8667b1e4db11dd24372d149e90b283/tools/simulation/ui/mutation-check.sh#L345\\n**_{_{![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)}} Fail the mutation run when a mutant is unverified**\\n\\nWhen an `EXPECT_MSG_FOR` entry is missing, the mutant is added only to `unverified`, but this exit condition ignores that array. The script therefore returns success—and can even print “No survivors”—despite a required assertion declaration being absent, so a future omitted table entry silently weakens `npm run mutation`; include nonzero `unverified` in the failing verdict.\\n\\nAGENTS.md reference: [AGENTS.md:L180-L184](https://github.com/mforce/cluckwork/blob/3b4434a50f8667b1e4db11dd24372d149e90b283/AGENTS.md#L180-L184)\\n \\n\\n

ℹ️ About Codex in GitHub

\",\"path\":null,\"side\":null,\"line\":null}{\"type\":\"pull_request_comment\",\"body\":\"Fixed in `d02dcfef` — `waitForModalEffect(page, true)` now precedes that read, same as the first test.\\n\\nStraightforwardly my inconsistency: I added the helper in round 1 specifically because dialog visibility does not settle the passive effect, documented exactly that in its docstring, and then left a third-test read that does the thing the docstring warns about. Worth noting the failure direction, since it is the less common one — this would have produced a **false failure against correct product code** on a slow runner, not a false pass. Still a defect, and on CI it would have read as \\\"the modal sweep is broken\\\" when nothing was.\",\"path\":\"tools/simulation/ui/specs/a11y-live-regions.spec.ts\",\"side\":\"RIGHT\",\"line\":433}{\"type\":\"pull_request_comment\",\"body\":\"Fixed in `d02dcfef`, and writing this mutant found something worse than the gap you named.\\n\\nYou were right that the control had no mutant — sixth instance of that shape here. So I wrote one that `aria-hidden`s the dialog's buttons. **It survived.**\\n\\n```\\nAX: {\\\"inTree\\\":true,\\\"ignored\\\":true,\\\"ignoredReasons\\\":[\\\"ariaHiddenElement\\\"],\\\"role\\\":\\\"none\\\"}\\n```\\n\\n`inTree` was never the right question. Chromium excludes an element from assistive technology two ways and only one is absence: an `inert` subtree's nodes are gone entirely, while an `aria-hidden` element may be **present with `ignored: true`**. Which you get depends on the element — measured on 151, an `aria-hidden` `

` is absent, an `aria-hidden` `