fix: close bug-hunter stale-state paths (re-open) - #146
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:470df91d00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:2e0a47a11a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
💡 Codex ReviewDatabase/src/app/api/documents/[id]/reindex/route.ts Lines 222 to 226 in 214c6fe  Make single reindex rollback atomicThe new Database/src/app/api/documents/bulk/reindex/route.ts Lines 218 to 222 in 214c6fe Make bulk reindex rollback atomicThe new pre-rollback active-job check has the same TOCTOU window in the bulk path: after the Database/src/app/api/ingestion/jobs/[id]/retry/route.ts Lines 101 to 105 in 214c6fe Tie retry rollback to this reset attemptWhen the document status update fails, this rollback predicate only proves the job is in a generic pending/queued state. A second retry of the same job can pass the endpoint's non-processing guard and write those same fields with a newer ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:6f58313e62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address the open Codex review comments on #146: - retry route: next_run_at doubles as a per-request rollback fence, so a losing retry's rollback matches zero rows instead of reverting a concurrent retry's reset; the document re-queue also stamps updated_at - single/bulk reindex routes: queue-state writes stamp documents.updated_at with a per-request value and the enqueue-failure rollback matches on that stamp, turning it into one atomic conditional UPDATE; the competing-job SELECT remains only as a fast path - shared ingestionRollbackFenceStamp helper generates microsecond-unique timestamptz stamps - tests assert the fence filters and stamped payloads The preceding merge of origin/main adopts the current jsonError 5xx contract, aligning the routes with the rollback tests' 500 expectations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ab6d70b20f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Jul 2, 2026
Addressed all open review feedback in ab6d70b (plus a merge of origin/main in 60e5d23):
Verification: focused suites (private-access-routes, api-validation-contract, worker-visual-capture, forms-clipboard-fallback — 103 tests) and @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e627b71f5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1d115153e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // precision while timestamptz stores microseconds, so random microsecond | ||
| // digits keep two same-millisecond requests distinct. | ||
| export function ingestionRollbackFenceStamp(now = new Date()) { | ||
| const microseconds = String(Math.floor(Math.random() * 1000)).padStart(3, "0"); |
There was a problem hiding this comment.
When two retry/reindex requests for the same job or document are created in the same millisecond, this helper has only 1,000 possible stamp values; a Math.random() collision makes both writes use the same next_run_at/updated_at fence. In that case, a failed request's conditional rollback can still match and undo the successful concurrent reset that the fence is meant to protect, so use a guaranteed per-request token or DB-generated/monotonic value instead of a random microsecond suffix.
Useful? React with 👍 / 👎.
Fourth id collision on this one row. `main` (#1461) claimed `#145` for "A branch sync during in-flight CI cancels a near-complete run, and nothing warns" — which is, as it happens, the same CI-cancellation behaviour this branch hit an hour ago. Resolved by taking main's `docs/outstanding-issues.md` wholesale and re-applying this capture at `#146`, rather than hand-editing the conflict region: hand-editing a 144-row table around a conflict marker is how the duplicate and wrong-width rows got in before. Marker bumped to 147.
The merge of origin into this branch hit exactly the damage #133/#140 describe: merge=union concatenated both sides of the ledger rather than merging it. Two collisions, both repaired without dropping either side's rows: - Another agent had already allocated #141-#144 on main for different items while this branch used #141-#143. The incoming rows renumber, per the ledger rule, so the capture becomes #145 (adopt a consolidated answer-home notice block), #146 (answer mode ships no verify-before-use caveat) and #147 (verify:pr-local exits 0 when its build step refuses to run). Their cross-references were updated to match, and the two duplicated next-id markers collapse to one at 148. - #140 appeared in both tables: it was closed on main as a duplicate of #133 (PR #1444) while this branch still carried it open. The resolution is honoured — the stale open row goes, the archive row stays. check:outstanding-issues: 145 rows (72 open, 73 archived), unique ids, next-id=148 above the highest, no merge driver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPyFcMfn1jMmphr6AqiWBg
Resolves the one conflicted file, docs/outstanding-issues.md. With merge=union removed by #1444 the ledger now conflicts honestly instead of silently doubling, so this is a real resolution rather than a repair. main had allocated #145 for "a branch sync during in-flight CI cancels a near-complete run" while this branch used #145-#147 for the answer-home notice capture. Per the ledger's own rule the incoming rows renumber: main's #141-#145 are kept verbatim and the capture becomes #146 (adopt a consolidated answer-home notice block), #147 (answer mode ships no verify-before-use caveat) and #148 (verify:pr-local exits 0 when its build step refuses to run), with their cross-references updated to match and the marker lifted to 149. No row was dropped from either side — main's #141-#145 each appear exactly once, verified against origin/main. check:outstanding-issues: 146 rows (73 open, 73 archived), unique ids, next-id=149 above the highest, no merge driver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPyFcMfn1jMmphr6AqiWBg
Third ledger collision in a row, same shape as the last two: main allocated #146 for the `ui-phone-scroll` Services anchor issue while this branch used #146-#148 for the answer-home notice capture. Main's row is kept and the incoming rows renumber to #147 (adopt a consolidated answer-home notice block), #148 (answer mode ships no verify-before-use caveat) and #149 (verify:pr-local exits 0 when its build step refuses to run), with their cross-references and the marker (150) following. Verified against origin/main that #141-#146 each still appear exactly once; nothing was taken wholesale from either side. check:outstanding-issues: 147 rows (74 open, 73 archived), unique ids, next-id=150 above the highest, no merge driver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPyFcMfn1jMmphr6AqiWBg
The three captured rows are dropped from docs/outstanding-issues.md, leaving it byte-identical to origin/main. They will be re-landed as their own single-file change once this PR merges. AGENTS.md recommends bundling append-only ledger rows because they are normally zero-risk, but that assumes a quiet file. Right now it is the hottest file in the repo: bundling them here cost three ID collisions and three full CI restarts in about thirty minutes (main took #141-#144, then #145, then #146, while this branch needed #145-#149 in turn). #133 already records that this file conflicts on nearly every main advance. The mockups diff itself touches four uncontended files and has not conflicted once today, so removing the ledger rows takes this PR out of a race it has no reason to be in. No content is lost: the row text is preserved verbatim and re-applied against a fresh main with clean ids. check:outstanding-issues: 144 rows (70 open, 74 archived), unique ids, next-id=147 above the highest, no merge driver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPyFcMfn1jMmphr6AqiWBg
…and #146 Conflict was docs/outstanding-issues.md only. Took main's copy wholesale (it carries #122's closure and the #98/#130 records from #1455) and re-applied the two one-line relocation notes, rather than hand-editing the conflict region. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHLPEV4o1rzipPDqshCSHY
…ecord The two Services viewport-anchor data points from PR #1457 were dropped when the withdrawn #149 row and its id allocation were restored. They are unrelated to that decision, so this puts them back and changes nothing else. #149 stays exactly as set: archived as a withdrawn record, with issues:next-id preserved at 150 so the id is retired rather than reused. Restored to #146: the test failed once more on head c739340 (anchorTop expected -138, received -7) then passed on 9da02d9 and a6f2281 across all three shards with the spec byte-identical — six data points, two failures, shard 1 only, both failures on a mockups-only PR. Notes that the 131px delta is roughly 2x the 64px viewport shrink rather than sub-pixel drift, which constrains the element attribution that row already asks for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JdPa3mHCX5ZQZZvU5GHU3r
…#1481) * issues: capture the production live-region defect; record the landed #1457 review Adds #147 — `search-results-header-band.tsx` sets aria-live="off" when faulted, so a clinical search that fails while focus is elsewhere is announced to nobody and the user is never told Retry appeared. Affects all twelve call sites that render the band. Raised by Codex against the copied line on PR #1457, fixed there, and confirmed to exist unchanged in production. The merged mockup carries the proven pattern and a non-vacuous assertion to port. Appends two further data points to #146 from PR #1457: the Services viewport anchor failed once more on head c739340 (expected -138, received -7) and then passed on two later heads with the diff byte-identical — six data points, two failures, shard 1 only. Both failures landed on a PR touching only mockups, which strengthens the unchanged-code reading. Notes that the 131px delta is roughly 2x the 64px shrink rather than sub-pixel drift. Records the prlanded verification for PR #1457 in the branch review ledger: squash e79e499, content diff against the branch tip empty, and the late aria-live/role="alert" commit confirmed present on main rather than orphaned by the squash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JdPa3mHCX5ZQZZvU5GHU3r * issues: withdraw #149 — the live-region defect was false, and the fix was worse Codex caught this on PR #1481 and it is correct. I filed #149 claiming a failed clinical search is announced to nobody because the count span sets aria-live="off" when faulted. I never checked whether another node makes the announcement. It does. search-results-header-band.tsx mounts a fault panel with role="alert" carrying the failure title, body and Retry (lines 407-414), and the mute is deliberate, documented in place: "While faulted the live region is silenced (aria-live='off') and the freshly-mounted fault role='alert' below makes the single announcement, rather than both speaking." tests/search-results-header-band.dom.test.tsx already pins exactly that with singular role queries that throw on duplicates. Escalating the count span in production, as #149 recommended, would have added a second alert beside the fault panel — a duplicate announcement and a broken test. The row is withdrawn to the archive rather than deleted, with the reasoning recorded so nobody re-files it. The mockup is unaffected: search-refine-adaptive-mockups.tsx has no fault panel, so there the count span is the only announcement channel and its escalation is correct. It simply does not port to production. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JdPa3mHCX5ZQZZvU5GHU3r * docs: record PR 1481 review * docs: preserve issue 149 allocation * docs: format withdrawn issue record * issues: restore the #146 data points lost while preserving the #149 record The two Services viewport-anchor data points from PR #1457 were dropped when the withdrawn #149 row and its id allocation were restored. They are unrelated to that decision, so this puts them back and changes nothing else. #149 stays exactly as set: archived as a withdrawn record, with issues:next-id preserved at 150 so the id is retired rather than reused. Restored to #146: the test failed once more on head c739340 (anchorTop expected -138, received -7) then passed on 9da02d9 and a6f2281 across all three shards with the spec byte-identical — six data points, two failures, shard 1 only, both failures on a mockups-only PR. Notes that the 131px delta is roughly 2x the 64px viewport shrink rather than sub-pixel drift, which constrains the element attribution that row already asks for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JdPa3mHCX5ZQZZvU5GHU3r --------- Co-authored-by: Claude <noreply@anthropic.com>
Second time a real conflict has blocked this PR's CI entirely — GitHub could not build refs/pull/1466/merge, so no pull_request workflow ran and the thin check list read as pending rather than blocked (#116). Only docs/outstanding-issues.md conflicted; scripts/ci-change-scope.mjs and docs/process-hardening.md auto-merged (main's regions are 150+ lines from this branch's). Took main's ledger wholesale rather than hand-editing a 140-row table around conflict markers. Only ONE of this branch's two ledger edits was re-applied: - #146 keeps its relocation note. The row is still open, and main's #1481 added two further data points to it (head c739340, anchorTop expected -138 received -7; six data points, two failures, shard 1 only) which are left untouched. - #127's edit is DROPPED as obsolete. Main's #1487 archived that row, and the archived form no longer cites tests/ui-phone-scroll.spec.ts at all, so there is nothing left to relocate. Re-applying it would have matched nothing or corrupted a differently-shaped row. Marker is main's 149 (not this branch's 147): #149 was allocated, withdrawn and retired rather than reused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHLPEV4o1rzipPDqshCSHY
…onflict loop Third consecutive real conflict blocking this PR's CI, always the same file, and the fix is to stop touching it rather than to resolve it a fourth time. main's newest commit is `fix(ledger): stop Prettier padding the issues table, closing #133 (#1479)` — a whole-file reformat of docs/outstanding-issues.md. Every row changed shape, so any edit to any row conflicts. That file is also touched by nearly every main PR (row appends, archival moves), and this branch is competing with a queue that merges several times an hour. This PR's entire stake in it was ONE cosmetic line: a note on #146 saying the cited spec moved to ui-phone-scroll-page-owned.spec.ts in the split. Three CI-blocking conflicts — each one stopping GitHub building refs/pull/1466/merge so no pull_request workflow ran at all — is a bad trade for that. So the file is taken from main verbatim and the note is not re-applied. This PR's diff no longer contains docs/outstanding-issues.md, which removes its only remaining contact point with the hottest file in the repo. Cost, stated rather than hidden: #146 keeps citing the old path. That row already records that the exact test title is the durable identity because declaration lines drift, so the stale path misleads nobody who reads it. The note belongs in a docs-only PR when the ledger is not mid-reformat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHLPEV4o1rzipPDqshCSHY
…f sleeping Root-caused from the failure trace (run 30582678887 artifact 8775836025), not inferred. Two snapshots either side of the resize: before anchorTop -138 documentScrollTop 504 headerBottom 0 after anchorTop -7 documentScrollTop 504 `documentScrollTop` is IDENTICAL at 504. The scroll position never moved; the content shifted down 131 px. And 131 is not arbitrary — the trace records collapseHeight 72, and gotoPhoneSurface injects --safe-area-top:59px. 72 + 59 = 131. The resize transiently re-shows the shared header, which reclaims its collapse row AND its safe-area band, and the assertion measured mid-transient. The cause is the fixed `waitForTimeout(100)` immediately after `setViewportSize`. That is the same sleep-and-hope pattern PR #1427 removed from `addPhoneScrollRunway`, and it explains every observation #146 has collected: - bimodal, never in between: the two states are "settled hidden" (-138) and "header fully re-shown" (-7), nothing between them - pixel-identical across runs: -138/-7/131 reproduced on three separate heads and two different PRs, which timing jitter cannot do - "under CI load": load pushes the settle past 100 ms, nothing more exotic - documentScrollTop unchanged: only chrome layout moved This also retroactively settles the narrowing that #146 WITHDREW. That row once argued the scroll position held because the sibling documentScrollTop assertion did not fail — invalid, since Playwright aborts at the first failing expect so it never ran. The conclusion was right anyway: the trace shows 504 on both sides. It is now measured rather than inferred. The fix polls the same geometry the hidden-state assertions already use (`header#search` bottom <= 1) with a 10 s budget and a message naming the condition. The tolerance is untouched — #146's stop rule forbids loosening it, and loosening it would have hidden a real 131 px content jump. Note what this does NOT claim: if the header ever fails to re-hide rather than merely settling slowly, the poll times out and the test fails naming that. That is strictly better than a 100 ms coin flip either way. Verified: the previously-failing test 3x consecutively (1 passed each), and the full spec 16 passed (43.0s). Local passes are weak evidence for a load-dependent race; the mechanism change is the argument. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHLPEV4o1rzipPDqshCSHY
Resolves the fifth conflict on docs/outstanding-issues.md by taking main's side on both hunks, after verifying that was correct rather than convenient: main renumbered the queue to fix a duplicate rank 18 this branch introduced, and it archived #138 and #146 rather than dropping them (confirmed present in the archive table). The resulting id set is byte-identical to main's. #150 escalated. That row records CodeRabbit reviewing none of a full day's PRs and rests explicitly on the Codex connector being the surviving reviewer that "found three real defects that had survived local gates". On PR #1505 Codex posted its own usage-limit notice alongside CodeRabbit's spending-cap notice, so that PR received zero automated review and so will anything opened while both caps hold. This is the same issue with its fallback removed, not a new one — which matters because the single Codex finding on #1459 was correct and caught a verification that had matched the wrong component, closing #105 on bad evidence. Local gates did not catch it. The attribution harness lands as scripts/measure-cls-attribution.mjs. #147's next step needs a before/after CLS pair, and #118 will want element attribution again; without this, both mean re-deriving it. Build and serve mirror run-lighthouse-budget.mjs so the numbers sit beside the Lighthouse reports. Two traps that cost real time are encoded rather than left to be rediscovered: CHROME_PATH must be set where the browser is outside a standard location, and an init script attaching a MutationObserver to document.documentElement before <html> exists throws and silently takes the CLS observer with it — so the script now fails loudly on an all-zero result instead of reporting a false clean bill. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Summary
Checks run locally
Context
This safely reintroduces the intended fix set while preserving current mainline contracts.