fix: implement all 12 database review findings - #134
Merged
Conversation
- Drop redundant indexes: documents_owner_hash_idx, ingestion_jobs_claim_idx - Add covering index documents_owner_id_covering_idx (owner_id + id + source_document_id) - Remove duplicate reset_document_index v1 (was missing DELETE FROM document_index_units) - Promote index_generation_id to typed uuid column on all 6 artifact tables - Fix match_document_chunks_text N+1: replace per-row scalar calls with CTEs - Fix invoke_indexing_v3_agent hardcoded URL -> GUC current_setting fallback - Add pg_cron retention job for rag_retrieval_logs (90-day window) - Add missing FK storage_cleanup_jobs.document_id -> documents.id - Create dedicated indexing_v3_agent_jobs table with SKIP LOCKED claim - Seed existing JSONB claim state into new jobs table - Add update_indexing_v3_agent_job_status RPC for edge function callback - Add COMMENT ON indexing claim pattern and deprecation markers - Sync supabase/schema.sql to reflect all DDL changes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8a5c62647
ℹ️ 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".
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
BigSimmo
enabled auto-merge (squash)
July 2, 2026 07:52
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93ae467fae
ℹ️ 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".
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore replacement-exists guards for NULL generation artifacts so last good chunks/artifacts remain if a generation commits without replacement rows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Map legacy JSONB statuses (deferred/retry_pending and unknown values) to pending during jobs-table seeding so inserts always satisfy the new status check constraint. Co-authored-by: Copilot <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: f9781f066e
ℹ️ 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".
Backfill missing indexing_v3_agent_jobs rows inside claim_indexing_v3_agent_jobs for indexed documents that are still queued via metadata status, so newly enqueued v3 work is claimable after migration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
13 tasks
This was referenced Jul 2, 2026
13 tasks
13 tasks
auto-merge was automatically disabled
July 2, 2026 10:04
Head branch was pushed to by a user without write access
…s in commit The artifact writer still writes index_generation_id only into JSONB metadata, leaving the new typed column NULL. The original EXISTS guard only checked for typed-column replacements, so it always returned false for artifact tables, meaning stale null-typed rows from a previous run could never be purged on re-index (they accumulate silently). For the null-typed-column branch of the six artifact DELETE statements in commit_document_index_generation: - Add (metadata->>''index_generation_id'')::uuid IS DISTINCT FROM p_index_generation_id so that rows belonging to the *current* generation (metadata gen = p_index_generation_id) are excluded from deletion. - Expand the EXISTS replacement check to accept both typed-column matches and metadata-based matches, so the safety guard fires correctly even when the writer only populates metadata. document_chunks is unchanged: the worker already writes its typed column. Co-authored-by: Copilot <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: 9ae167406c
ℹ️ 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".
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
BigSimmo
enabled auto-merge (squash)
July 2, 2026 11:38
auto-merge was automatically disabled
July 2, 2026 11:40
Head branch was pushed to by a user without write access
Contributor
BigSimmo
enabled auto-merge (squash)
July 2, 2026 11:42
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
Sixth conflict today, and the first that auto-merged *wrongly*: git's text merge concatenated both tables, duplicating all 63 open rows. PR #1421 had landed on main using #128/#129/#130 — the exact id collision #112 describes — so both sides had those ids with different content and the merge kept both. `npm run check:outstanding-issues` caught it and stated the correct resolution verbatim: renumber the incoming rows above the marker and bump it, rather than taking one side wholesale and dropping the other's rows. Done exactly that — main's table is authoritative, this branch's four rows renumber to #131/#132/#133/#134, marker to 135. Verified both sides' rows survive: main's #128-#130 and mine are all present and distinct. Worth noting main's new #129 (`update-branch` API does not honour the `merge=ledger` driver) is the server-side twin of my #134 (the driver is absent wherever `npm install` was skipped). Same root cause from two directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
BigSimmo
added a commit
that referenced
this pull request
Jul 30, 2026
…indings (#1424) * docs(ledger): record PR #1400 closeout and capture three unrecorded findings Documentation only — two ledger files, no code. **Review closeout for PR #1400** appended with `ledger:append` (never hand-written), recording the 17 findings fixed, the verification behind each, and the post-merge check that all 8 commits are ancestors of main with the 4 changed files byte-identical. **Three findings from that session that nothing else records:** - `#125` — `@codex fix` produced 11 commits across a branch named `work`, none fetchable, the same finding rewritten four times. It reads as success while the branch is unchanged, which is the actual hazard. - `#126` — both client-side push guards are inert for agent pushes: `gh` absent makes the auto-merge sentinel fail open, and `core.hooksPath` is set only by a local install. They protect the environment least likely to need them. - `#127` — this ledger's fixed-width padding makes one row's edit re-pad all 59, so it conflicts on nearly every main advance; each conflict silently stopped all CI on #1400 via `#116`. Records that `merge=union` is the wrong fix, with the evidence. CircleCI was deliberately not filed — already captured as `#122`. Checked before writing rather than after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * docs(issues): correct unsafe pull_request_target advice in #129 Review caught a real problem in the guidance I filed, not in code: #129's next-action suggested moving *both* push guards server-side into a `pull_request_target` job. That context carries secrets and a write token, and a format check must execute PR-head code — including the dynamic `prettier.config.*` this very PR taught the guard to load. That is the classic privileged-context vector, and `.github/workflows/pr-policy.yml` already avoids it deliberately by checking out only `github.workflow_sha`. Corrected, and the row now records why the whole idea was unnecessary: formatting is already enforced server-side by `Static PR checks` running `format:check` on ordinary `pull_request` CI, so the guard's only unique value is failing fast before the push. Only the metadata-only auto-merge sentinel could safely live in a target job. Bad advice in a durable ledger is worse than no advice — someone would have acted on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * fix(issues): repair the duplicated table from the sixth main merge Sixth conflict today, and the first that auto-merged *wrongly*: git's text merge concatenated both tables, duplicating all 63 open rows. PR #1421 had landed on main using #128/#129/#130 — the exact id collision #112 describes — so both sides had those ids with different content and the merge kept both. `npm run check:outstanding-issues` caught it and stated the correct resolution verbatim: renumber the incoming rows above the marker and bump it, rather than taking one side wholesale and dropping the other's rows. Done exactly that — main's table is authoritative, this branch's four rows renumber to #131/#132/#133/#134, marker to 135. Verified both sides' rows survive: main's #128-#130 and mine are all present and distinct. Worth noting main's new #129 (`update-branch` API does not honour the `merge=ledger` driver) is the server-side twin of my #134 (the driver is absent wherever `npm install` was skipped). Same root cause from two directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * docs: record PR 1424 review --------- Co-authored-by: Claude <noreply@anthropic.com>
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
`PR mergeability` flagged this branch, but `git merge-tree` returned a clean tree — behind-but-clean staleness, not a content conflict. The merge itself then reported success while producing every open-items row twice (`#59 appears 2 times (lines 101, 166)` and so on for the whole table): `.gitattributes` sets `merge=union` on this file, which is git's built-in concatenate-both-sides driver with no dedupe, and the table is not append-only. Rebuilt from `origin/main` (now through #134) with only the one row this branch actually changed re-applied. Recorded as #135, since the driver turns a resolvable conflict into a guaranteed guard failure and makes `merge-tree` look clean. Also corrects #127's own framing. `Production UI` PASSED on run 30530393684, so the failure is intermittent at 2 of 3 completed runs, not reproducible as the previous row claimed — that was premature on two datapoints. The `data-scroll-signal` diagnostic therefore has not yet had a failure to report; it is still the thing that will name the cause when one comes. Verified: check:outstanding-issues 133 rows / 67 open / unique ids / next-id=136; check:branch-review-ledger 112 live + 1206 archived; whole-tree prettier clean; no conflict markers under docs/, tests/ or src/. Not re-run for this merge: verify:cheap and the phone-scroll spec — the code changes are unchanged from 5495f28, where both passed (434 test files / 4562 tests, and 56 passed), and this commit touches only the ledger plus main's own already-verified tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
Third staleness resolution on this branch in about forty minutes. `PR mergeability` again reported `mergeable_state: dirty` and "a real merge conflict", but `git merge-tree --write-tree` returned a clean tree, so this is behind-but-clean staleness, not a content conflict. The single overlapping path between this branch and main is `docs/outstanding-issues.md` — nothing else on this branch is contested, which is why the code files are byte-identical to 5495f28. The file is rebuilt from `origin/main` with this branch's two rows re-applied (#127 corrected, #135 added after #134, marker 136) instead of keeping the union driver's output, which concatenates both sides of every overlapping hunk without dedupe and doubled the whole table last time — that behaviour is what #135 records. Main's #127 still carried the withdrawn "sharedChromePinned is stuck" text and #135 was unclaimed, so neither graft overwrote anyone else's edit. Verified: check:outstanding-issues 133 rows / unique ids / next-id=136; check:branch-review-ledger 113 live + 1206 archived; whole-tree prettier clean. Not re-run: verify:cheap and the phone-scroll spec — `git diff 5495f28 -- src/ tests/` is empty, so the code carries that commit's evidence (434 test files / 4562 tests, and 56 passed) unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
…ocation CI caught what I did not: `static-pr` failed on `check:outstanding-issues` with #131-#134 duplicated and two `issues:next-id` markers. Cause: main's PR #1424 allocated #131-#134 for its own findings at the same time this branch held #131-#135, and `merge=union` did what union does — kept both sides under the same ids. That is #112's documented limit: union preserves concurrent appends but cannot allocate unique ids, so the structural gate is the only thing that catches it. My error was pushing without re-running that gate. The previous push resolved a `docs/branch-review-ledger.md` conflict, and I validated only that file before pushing to win the race against main — but the same merge also touched `docs/outstanding-issues.md`. `verify:cheap` would have caught it locally. Main's rows keep #131-#134 (already merged and referenced elsewhere); this branch's five renumber to #136-#140, one marker at 141, and the cold-cache cross-reference in process-hardening follows its row. Two of main's new rows also make a planned addition here redundant: #134 is the absent ledger merge driver and #133 is the outstanding-issues merge churn — both hit during this branch's work, both already captured upstream, so nothing new is filed for them.
BigSimmo
added a commit
that referenced
this pull request
Jul 30, 2026
…t did (#1427) * test(phone-scroll): prove the drag delivered before asserting the chrome hid CI run 30518866604 failed `ui-phone-scroll.spec.ts:423` on expect(getByTestId('universal-header-collapse')) .toHaveAttribute('data-scroll-hidden', 'true') // received "" after the full 10s auto-retry, and the classifier recorded it as "needs investigation". The assertion was right; the scroll never happened. `dragScrollBy` moved the scroller with `scrollTop +=`, which clamps silently at the end of the range, and returned nothing. When a page lays out shorter than the test assumed — content still settling under full-suite CI load — a 720px request delivers a fraction of that, the chrome correctly stays visible because document-detail chrome only hides past `scrollTop > 120`, and the failure surfaces ten seconds later looking like a product regression. The helper also resolved the scroll owner once up front, so a mid-drag layout change left it pushing an element that had stopped scrolling. - `dragScrollBy` now re-resolves the owner each step and returns the distance actually travelled. - `dragScrollUntilHidden` waits for the remaining downward runway (a condition wait, not a settle sleep), drags, and fails naming the shortfall if the drag could not cross the threshold. Used at the four sites that assert a hide immediately after a fixed-distance drag. - `addPhoneScrollRunway` waits for its 1600px filler to reach layout instead of sleeping 50ms. All 14 call sites already depend on that runway existing. Every assertion is byte-identical: a genuinely stuck header still fails exactly as before, once the drag is proven to have happened. No `.first()` was added (#93's stop rule) and no tolerance was relaxed. * ci: shard Production UI across three runners Measured on 2026-07-30 from the Actions API, two full UI-scope PR runs (30520443076, 30519912667): `Production UI` took 15m26-16m31 of a 16.8-18.6 minute run — 83-89% of wall clock — while every other job finished by minute 4 and then waited. Playwright itself reported `339 passed (13.5m)`; the balance is the isolated production build. That single job is also where the churn cost lands: 42% of PR runs in the sampled window were cancelled (25 of 60 completed), almost all superseded mid-Production-UI. Sharding is across runners, not workers. `workers: 1`, `fullyParallel: false` and `retries: 0` are unchanged inside each shard, so determinism is identical and per-runner load falls — which matters because #93's duplicate page root is load-dependent. `run-playwright.mjs` already forwards argv to `playwright test`, so `--shard` needed no runner change. The shard count is measured, not chosen. `fullyParallel: false` makes a spec file indivisible, so shard sizes are lumpy and more shards is not monotonically faster. Over the 340 required chromium tests: N=3 -> 121/106/113 largest 121 N=4 -> 121/106/96/17 largest 121 (same critical path, one more runner) N=6 -> 65/56/106/5/91/17 largest 106 N=5 -> 121/106/0/96/17 and N=8 -> two empty shards N=4 buys nothing over N=3, and any N with an empty shard would go red because `test:e2e:pr` deliberately omits `--pass-with-no-tests`. Expected critical path ~15.5 -> ~7 min, assuming per-test cost is roughly uniform. `fail-fast: false` so a failing shard cannot cancel its siblings and re-create the cancelled-vs-failed ambiguity #95 removed. Artifact names are shard-scoped because upload-artifact runs with `overwrite: false`. Branch protection requires only the `pr-required` aggregate, and `needs` on a matrix job yields the roll-up of all shards, so the aggregate is unchanged. Also adds `restore-keys` to both Playwright browser caches: without a prefix fallback a lockfile bump forced a cold browser download in every UI job at once, now three times over. * ci: bound the codex auto-resolve jobs and serialise the visual config Two inconsistencies found while mapping the pipeline, neither load-bearing but both silent: - `codex-autofix-review-comments.yml` was the only workflow in the repo with no `timeout-minutes` on either job, so both inherited GitHub's 360-minute default for work that reads PR metadata and posts one comment. - `playwright.visual.config.ts` set neither `workers` nor `fullyParallel`, so it inherited Playwright's default `workers = 50% of CPUs`. The production config pins both to serial deliberately; the visual lane was quietly opting out of the anti-flake posture the rest of the suite is configured for. * chore(gates): pin the documented gate count to the real chain Both numbers were wrong. `CLAUDE.md` said 24 static/consistency gates against an actual 25 — `check:assets` landed before that line was written, so it was wrong at authoring — and the `gates` skill said "check 2 of 26" against an actual 28. A stale count is not cosmetic here. The skill's whole point at that line is that `verify:cheap` stops at the first failure and everything after it never ran; an agent that believes the chain is 26 long cannot say how much a mid-chain failure skipped. `check:gate-manifest` already derives the real count from `verify:cheap:internal`, so it now asserts the documented numbers against it. The assertions fail closed: if the anchor phrasing disappears, the guard reports a lost anchor rather than passing on a document it no longer checks. Mutation-proven: reverting the skill to "26" fails with ".claude/skills/gates/SKILL.md says 26 where the chain has 28". * docs(issues): capture the CI review's deferred findings Five items from the CI/testing review that should not be changed blind: - #125 `ui_changed` matches all of `src/app`, so an API-only diff pays the 15-minute UI gate. Narrowing it can hide a real regression, so it needs a decision plus a compensating check rather than a quieter filter. - #126 the Playwright build writes to a per-run distDir, so Next's build cache is cold every run (~2 min, now ~29% of the sharded critical path). Fixing it means suppressing the runner's documented always-cleanup, which must not ship without executing the runner. - #127 the advisory UI lane spends ~3 min per UI PR on 5 mockup tests; there are currently zero `@quarantine` tests for it to cover. - #128 CI Triage is complete and self-tested but inert pending a repo variable. - #129 four `changes` outputs are computed and consumed by nothing, and `coverage_changed` fires on any non-doc file. * docs(ledger): record the ci-testing-review pass at this HEAD * ci: re-measure the shard split on the merged tree and refresh stale gate counts The merge changed both numbers this branch had recorded. Shard balance, re-measured against 342 required chromium tests (was 340): N=3 -> 121/111/110 largest 121 N=4 -> 121/106/98/17 largest 121 N=3 remains correct — one 121-test spec group bounds both, so N=4 spends an extra runner for the same critical path. The re-measure command is now in the workflow comment so the next person does not have to rediscover it. Gate counts: merging main added `check:gitleaks-pinned` and `check:pr-mergeability` to `verify:cheap:internal`, so the documented counts went stale the moment the merge landed — 25 -> 27 static, 28 -> 30 total. The guard added earlier in this branch caught it immediately rather than letting the docs drift again, which is the whole reason it exists. Also records the `ui-critical-fast` interaction: the UI critical path is now that 15-test fail-fast job plus the slowest shard, not the full 13.5-minute suite, so neither of this branch's pre-merge timings can be read on its own. * docs(issues): rebuild the ledger after a union-merge duplication The `merge=union` driver on `docs/outstanding-issues.md` preserves concurrent appends, but when both sides restructure the same region it concatenates them wholesale. Merging the latest main did exactly that: every open row appeared twice and both `issues:next-id` markers survived — 66 duplicate-id errors from `check:outstanding-issues`, which is precisely the failure that gate exists to catch (#112). Resolved by rebuilding on main's canonical file rather than by hand-editing the duplicated table: reset to `origin/main`, then re-apply this branch's five captured rows at #131-#135 (main had advanced its allocation to #130 while this branch was open, so the earlier #128-#132 numbering collided again) and re-apply the #127 narrowing note. Marker bumped to 136. Union merge cannot allocate unique ids; only the structural gate can catch when it has produced an invalid file. It did. * ci: record the measured shard result, correcting the predicted one First real run of the sharded shape (CI 30530618838, all green, whole run 13m39 against a 16.8-18.6 min unsharded baseline): ui-critical-fast 15 tests 3m14 Production UI (1) 121 tests 9m36 Production UI (2) 111 tests 6m54 Production UI (3) 110 tests 6m20 The prediction was wrong by ~40%. ~6.8 min was expected for the largest shard from 121/342 tests x 13.5 min; 9m36 happened. Per-test cost is not uniform — 111 tests took 6m54 while 121 took 9m36 — so a count-balanced split understates the slowest shard whenever the slow specs land in one group. `--shard` can only balance by count; balancing by duration would mean splitting the slow spec files themselves. The win is real but smaller than claimed, and the workflow comment and process-hardening now carry the measured numbers plus the reason the arithmetic misleads, so the next person re-measures instead of re-deriving. Also merges origin/main. The ledger conflict was GitHub-visible only: that file carries merge=union locally, which GitHub does not honour (#129). Resolved by keeping the one genuinely new record and dropping three that main already had elsewhere in the file — append-only forbids dropping a record that exists once, not keeping a second copy. Superseding record appended for this HEAD, since the prior one asserted a root cause that #127's trace evidence refutes. * docs(issues): renumber this branch's rows above main's concurrent allocation CI caught what I did not: `static-pr` failed on `check:outstanding-issues` with #131-#134 duplicated and two `issues:next-id` markers. Cause: main's PR #1424 allocated #131-#134 for its own findings at the same time this branch held #131-#135, and `merge=union` did what union does — kept both sides under the same ids. That is #112's documented limit: union preserves concurrent appends but cannot allocate unique ids, so the structural gate is the only thing that catches it. My error was pushing without re-running that gate. The previous push resolved a `docs/branch-review-ledger.md` conflict, and I validated only that file before pushing to win the race against main — but the same merge also touched `docs/outstanding-issues.md`. `verify:cheap` would have caught it locally. Main's rows keep #131-#134 (already merged and referenced elsewhere); this branch's five renumber to #136-#140, one marker at 141, and the cold-cache cross-reference in process-hardening follows its row. Two of main's new rows also make a planned addition here redundant: #134 is the absent ledger merge driver and #133 is the outstanding-issues merge churn — both hit during this branch's work, both already captured upstream, so nothing new is filed for them. * docs(issues): rebuild against main's current id allocation The union merge duplicated the whole open and archive tables again (two header rows, every id twice) because main restructured the file while this branch held rows in it. Same resolution as before and for the same reason: rebuild on main's canonical file rather than hand-editing a doubled table, then re-apply this branch's five rows. Main is now at next-id=135, so they land as #135-#139 with the marker at 140. None of the five is duplicated upstream — checked by summary before re-applying. This is the third renumber of the same five rows in one PR. That is not a mistake being repeated, it is #133 ("outstanding-issues conflicts on nearly every main advance") happening: any branch that holds rows in this file re-collides every time main lands one. Worth weighing whether captures should land in their own PR ahead of the work rather than riding along with it. * docs: record PR 1427 review --------- Co-authored-by: Claude <noreply@anthropic.com>
BigSimmo
added a commit
that referenced
this pull request
Jul 30, 2026
…1430) * test(phone-chrome): name which value holds the header open, not just that it did `data-scroll-hidden` on the collapse wrapper is `scrollHidden && !sharedChromePinned`, so a missing attribute has two very different causes the assertion cannot separate: the scroll state machine never fired, or it fired and a pin held the chrome open. The bare assertion reads as the first even when it is the second — which is how a stuck pin was misread as a flaky scroll gesture across two CI runs on 2026-07-30. `expectChromeHidden` keeps the same pass condition and adds a failure message. The discriminator is already in the DOM: DocumentViewer's page-owned composer hides on `composerScrollHidden`, which consults `scrollHidden` and not the pin, so composer-hidden plus header-visible proves the pin. Every term of `sharedChromePinned` also has a DOM tell — an `aria-expanded` trigger, a popover, or focus inside the portaled addon host — so when all read false the pin is a stale latch rather than a live surface, and the message says so. Also updates ledger #127 with what the traces establish: `scrollHidden` is TRUE and `sharedChromePinned` is stuck, reproducible on both completed full-suite runs and both variants, always at the reduced-motion hide that follows the section-sheet round-trip and never at the first hide. `main` only looks green because `Production UI` is skipped on its docs-only pushes; it has not run this test since 90b3e34, with zero `src/` changes since. Deliberately not the fix. Which term latched is proven; the mechanism is inferred, and it does not reproduce locally — every local run used the container's Chromium 1194 rather than the bundled 1234 CI installs (#121), so no local green is evidence here. This makes the next CI failure name its own cause instead of costing another trace download. Verified: typecheck clean, lint clean, prettier clean, check:outstanding-issues 125 rows / unique ids / next-id=128, and both affected tests still pass locally (2 passed, 12.6s). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ * fix(test): publish the header's own scroll signal; withdraw the pin claim Codex review is correct and this retracts the previous commit's central claim. Reading `form.document-viewer-composer[data-scroll-hidden]` as a proxy for the header's `scrollHidden` was wrong: they are separate state machines. The header is driven by the shell's `chromeScrollHide` (global-search-shell.tsx:332), fed only by `useDocumentScrollHideReporter` (line 345) and passed in at line 876, while DocumentViewer runs its own two `useHideOnScroll` instances (use-document-viewer-chrome-scroll.ts:20-30). Composer-hidden therefore proves DocumentViewer's reporter fired and says nothing about the header's, so it never separated a pin from a reporter-never-fired — the exact distinction the helper claimed to make. `data-scroll-signal` on the collapse wrapper now publishes the header's raw `scrollHidden` before the pin is applied, and `expectChromeHidden` reports it alongside DocumentViewer's so a divergence between the two feeds is visible instead of collapsed into one verdict. Nothing styles the attribute; no CSS or code reads it (verified by grep), so behaviour is unchanged. Ledger #127 is corrected rather than patched over: the "traces prove sharedChromePinned is stuck" claim is explicitly withdrawn, what the traces do establish is separated from what they do not, and the new leading hypothesis is recorded as untested — the shell's feed is document-only, so where `#main-content` owns scrolling `window.scrollY` never moves and the shell reporter cannot see the gesture, which would explain the standalone-PWA variant directly. It also now says not to infer the header's scroll state from any page-owned composer. Verified: verify:cheap exit 0 — Test Files 434 passed (434), Tests 4562 passed | 4 skipped (4566); typecheck and lint clean; prettier clean; the full phone-scroll spec 56 passed (4.4m) against an isolated production build. That build used the container's Chromium 1194, not the bundled 1234 CI installs (#121), so it proves the attribute broke nothing and nothing more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ * Merge origin/main; rebuild the ledger the union driver doubled `PR mergeability` flagged this branch, but `git merge-tree` returned a clean tree — behind-but-clean staleness, not a content conflict. The merge itself then reported success while producing every open-items row twice (`#59 appears 2 times (lines 101, 166)` and so on for the whole table): `.gitattributes` sets `merge=union` on this file, which is git's built-in concatenate-both-sides driver with no dedupe, and the table is not append-only. Rebuilt from `origin/main` (now through #134) with only the one row this branch actually changed re-applied. Recorded as #135, since the driver turns a resolvable conflict into a guaranteed guard failure and makes `merge-tree` look clean. Also corrects #127's own framing. `Production UI` PASSED on run 30530393684, so the failure is intermittent at 2 of 3 completed runs, not reproducible as the previous row claimed — that was premature on two datapoints. The `data-scroll-signal` diagnostic therefore has not yet had a failure to report; it is still the thing that will name the cause when one comes. Verified: check:outstanding-issues 133 rows / 67 open / unique ids / next-id=136; check:branch-review-ledger 112 live + 1206 archived; whole-tree prettier clean; no conflict markers under docs/, tests/ or src/. Not re-run for this merge: verify:cheap and the phone-scroll spec — the code changes are unchanged from 5495f28, where both passed (434 test files / 4562 tests, and 56 passed), and this commit touches only the ledger plus main's own already-verified tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ * Merge origin/main (fcd9041); rebuild the ledger rather than trust union Third staleness resolution on this branch in about forty minutes. `PR mergeability` again reported `mergeable_state: dirty` and "a real merge conflict", but `git merge-tree --write-tree` returned a clean tree, so this is behind-but-clean staleness, not a content conflict. The single overlapping path between this branch and main is `docs/outstanding-issues.md` — nothing else on this branch is contested, which is why the code files are byte-identical to 5495f28. The file is rebuilt from `origin/main` with this branch's two rows re-applied (#127 corrected, #135 added after #134, marker 136) instead of keeping the union driver's output, which concatenates both sides of every overlapping hunk without dedupe and doubled the whole table last time — that behaviour is what #135 records. Main's #127 still carried the withdrawn "sharedChromePinned is stuck" text and #135 was unclaimed, so neither graft overwrote anyone else's edit. Verified: check:outstanding-issues 133 rows / unique ids / next-id=136; check:branch-review-ledger 113 live + 1206 archived; whole-tree prettier clean. Not re-run: verify:cheap and the phone-scroll spec — `git diff 5495f28 -- src/ tests/` is empty, so the code carries that commit's evidence (434 test files / 4562 tests, and 56 passed) unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ * docs: record PR 1430 review --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jul 30, 2026
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
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.
Summary
Full implementation of all 12 findings from the database review.
Changes
Integrity
storage_cleanup_jobs.document_id → documents.idwithON DELETE CASCADEreset_document_indexv1 (was missingDELETE FROM document_index_units)Performance
documents_owner_hash_idx,ingestion_jobs_claim_idxdocuments_owner_id_covering_idx(owner_id, id, source_document_id)match_document_chunks_text: per-row scalar calls → CTEs + LEFT JOINsCorrectness
index_generation_idfrom JSONB lookup to typeduuidon all 6 artifact tablesinvoke_indexing_v3_agentwithcurrent_setting('app.indexing_v3_agent_base_url', true)+ fallbackArchitecture
indexing_v3_agent_jobstable replacing JSONB claim state; seed from existing state; addupdate_indexing_v3_agent_job_statusRPCMaintenance
pg_cronretention job forrag_retrieval_logs(90-day window)Follow-up (out of scope)
Edge function
indexing-v3-agentshould callupdate_indexing_v3_agent_job_statusto close the loop. Until then completed jobs re-claim after ~45 min stale timeout (wasteful, not corrupting).