Queue five ledger corrections: the document-viewer rows say things that are no longer true - #1930
Conversation
An inventory of remaining viewer work found the ledger wrong in four places. None is a code defect; all of it is memory that would mislead the next session, which is the failure mode this ledger exists to prevent. Every claim below was re-verified against main 2d27039, not against the base the inventory started on. New row for crop -> page overlay. It is the one Phase 3 capability never built and it had no row at all — it lived only in a plan document marked "out of scope". The request carries what makes it actionable without that document: the geometry is already fetched and thrown away. document-detail.ts SELECTs bbox, but bbox is absent from DocumentDetailImage and from the viewer's ImageRow, so it reaches the server loader and never reaches the viewer. It also records why this was scoped out rather than overlooked, and that the stored values are unvalidated — a highlight over the wrong region of a clinical source is worse than no highlight. #278 closed. It asks for the pinned chrome to be masked in the document-viewer visual baseline; PR #1743 (65cd909) did that and the row was never updated. #215 restated. Two of its four items shipped — the lightbox has decoding="async" from #1660, and SignedImage has priority plus the fetchPriority split from #1772. Only pwa-lifecycle.tsx and the demo PNG->WebP conversion remain. #280 gained a third acceptance item. Phase 3 put multi-page documents in a bounded reading pane that chains overscroll out at its ends; a nested vertical scroller is a known iOS hazard that no Chromium gate covers, so it belongs on the same physical device pass as the pinch and canvas-budget checks. Queued as inbox requests rather than canonical edits, per the intake contract that landed while this work was in flight. docs/outstanding-issues.md is deliberately untouched; reconciliation allocates the new id and applies these. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7
The header said Phase 3 landed in 'this branch'; it merged as #1772. The Phase 3 table also described crop -> page overlay as out of scope with no pointer to anything durable — it now says a ledger row tracks it, and names ImageRow as the second half of the contract gap alongside DocumentDetailImage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7
#285 closed because session-start.sh provisions Node 24.19.0, which is true for sessions that start after it exists. A session that began on an older main and later re-based onto current main gets the hook file without ever executing it, so PATH stays on the container's 24.13.0 — and since engines.node is now >=24.15.0, npm ci refuses and check:runtime fails as the FIRST step of verify:pr-local, blocking every gate for any diff. Running the hook by hand aborts on 'CLAUDE_ENV_FILE: unbound variable' under set -u, after the tarball has extracted but before it exports PATH, so the operator sees a failure and cannot tell the download actually worked. Both worked around here: exporting the provisioned bin by hand let npm ci and the full verify:pr-local pass (11 checks completed, none failed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:46 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:648b86ad07
ℹ️ 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.
Running `bash .claude/hooks/session-start.sh` aborted with `CLAUDE_ENV_FILE: unbound variable`. The script runs under `set -u` and expanded both CLAUDE_ENV_FILE and CLAUDE_PROJECT_DIR unguarded; Claude Code sets them when it invokes the hook, nothing sets them otherwise. It aborted at the first of the two — after the Node tarball had downloaded and extracted, but before PATH was exported and before the install — so the operator saw a failure, got no runtime, and had no way to tell the download had actually worked. Manual invocation is not a curiosity, it is the documented remedy for a real situation. SessionStart hooks do not re-fire when a long-lived session re-bases its checkout onto a newer main, so a session that predates this file acquires it without ever executing it and stays on the container's older Node. Since engines.node is now >=24.15.0 <25 that is total rather than partial: npm ci refuses outright, and check:runtime is the FIRST step of verify:pr-local, so no gate can complete for any diff — a docs-only one included. Reproduced in this session against main 2d27039. CLAUDE_ENV_FILE is now guarded and the no-env-file branch says so, printing the export line so the runtime can be kept deliberately. CLAUDE_PROJECT_DIR falls back to the repository the script itself lives in, derived from BASH_SOURCE rather than the caller's cwd, so a manual run installs into the right tree from anywhere. checkNodeRuntime's failure message now names the hook. It said "Install Node 24.15.0 or newer", which sends the reader off to do by hand what this repo already ships a script for — including the exclusive major ceiling that installing "latest" would miss. tests/session-start-hook.test.ts drives the hook with a stubbed node and a current lockfile stamp, so it exercises the variable handling without downloading or installing. Verified in both directions: reverting the guard turns exactly the two manual-run cases red with the original `unbound variable`, while the hook-invoked and non-remote cases stay green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7
The inbox intake fixed id allocation — ids are assigned at reconciliation, so two branches can no longer collide on a number. It does not fix content staleness: an update request carries a full replacement detail written at queue time, and reconciliation applies it verbatim, so a row that changed on main in between is overwritten with no signal. The multiple-pending-mutations guard only fires when two requests target the same id, not when one request is simply old. This session's near-miss is the evidence, and it was caught by re-reading rather than by a gate. Records the option (fingerprint the target row at queue time, the schema is versioned) and the reason to weigh it rather than assume it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7
BigSimmo
commented
Aug 13, 2026
Two commits added after the description was written — it now understates this PRThe Summary above says "No source changes." That was true when this opened and is no longer. Two commits have been pushed since, and I cannot edit the description from this session (the PR-handoff guard denies any tool whose name carries Please read the Summary as amended by this comment. Reviewers should expect a source change, not a docs-only diff.
|
BigSimmo
left a comment
There was a problem hiding this comment.
Codex is taking over the authorised review-and-fix pass on this exact head. I will independently validate all three unresolved P2 findings against the current implementation and repository contracts, make only proven in-scope corrections, run the relevant offline gates, and resolve each thread with evidence. I will not merge, close, rebase, force-push, alter auto-merge, deploy, or run provider-backed/live checks.
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #10668 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo
commented
Aug 13, 2026
Codex final review-and-fix summaryFinal state
Sync and merge-tree decisionNo base sync was performed. The branch is behind but clean, and no conflict or exact-head required-CI blocker required restarting the green checks. GitHub's synthetic merge commit Confirmed issues fixed
Dispositioned or left out of scope
Independent adversarial reviewCodeRabbit attempted an exact-head review but returned no findings because the organisation had reached its temporary review/spending limit. A separate fresh manual adversarial pass therefore challenged correctness, shell-process persistence, compatibility, test isolation, regression risk, and scope. It found one additional P2 defect: the first version of the new hook test could have run Review threadsAll three actionable P2 threads have exact-head fix dispositions and are resolved. No unresolved actionable review thread remains. Decisive local verification
A full local Node 24/npm 11 repository run was unavailable because this execution environment had no authenticated checkout, blocked repository-network access, and only Node 22/npm 10. Exact-head GitHub CI provided the broad verification instead. Exact-head required CIOn
The successful CI workflow included runtime alignment, installed dependency parity, ledger integrity and write-discipline gates, documentation checks, changed-file formatting, lint, type-check, unit coverage, build and the required aggregate. Advisory and non-required checks
Limitations and residual risk
The PR was not merged. Final merge is left to the user. |
Uh oh!
There was an error while loading. Please reload this page.
… squash (#1937) * issues: record a queued request lost between a branch and its squash PR #1930 (squash a544719) merged claude/viewer-ledger-truth-pass at head 9525dd7. The branch added six inbox requests across three commits; the squash contains five. The missing one, faa4299e, came from commit 648b86a, which `git merge-base --is-ancestor 648b86a9525dd7` confirms was part of the merged head. It is absent from main entirely — not pending, not applied, and its text appears nowhere in the canonical ledger. The mechanism is not established and the row says so. A GitHub squash does not drop files on its own, so something happened at merge time; the row records what was verified rather than a theory. This instance cost nothing in substance: the lost request was a finding whose two remedies — guarding CLAUDE_ENV_FILE in the session-start hook, and having checkNodeRuntime name that hook — shipped in the same pull request and are on main now. The record was lost, not the work. A request carrying something not fixed in its own PR would simply have vanished. What makes it worth a row is that nothing noticed. check:ledger-write-discipline proves request files are immutable from the merge base and audits canonical edits against moved requests, but no check asserts that a request committed on a branch still exists after that branch merges. This was found by counting five against six by hand, days later, while answering an unrelated question. Deliberately not re-queued under its old UUID: an immutable record that reappears with new provenance is worse than an absent one, and its content is already delivered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 * ledger: record the branch review for PR #1937 Immutable review record for claude/ledger-merge-loss-finding at 7a8d57d. Scope is the single inbox request filed by that PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Five immutable outstanding-issue requests plus the viewer plan now accurately describe the remaining document-viewer work. The review also fixes the stale-runtime remediation path instead of leaving that confirmed defect as another queued issue.
Changes
bboxalready survives the runtime loader projection and reaches viewer state. The remaining work is typed contract coverage, coordinate normalisation, validation, and rendering.#215: two image findings are shipped and two remain open. The row no longer claims all four are complete.#278,#280, and stale-update hazard requests.export PATH=...command for the invoking shell.check:runtimetell remote callers to run the hook and then apply that export in their current shell.nodeandnpm, prove the printed export activates Node 24.19.0, and cannot runnpm ciagainst the real checkout.docs/outstanding-issues.mdremains untouched. Reconciliation and numeric ID allocation remain deliberately separate.Verification
Focused offline verification for the repair commit:
bash -n .claude/hooks/session-start.sh— passThe execution environment did not provide an authenticated repository checkout or the required Node 24/npm 11 toolchain, so the full repository Vitest, formatting,
verify:pr-local, type-check, and build gates were not rerun locally after the repair. Required exact-head GitHub CI is the authoritative broad verification.No Lighthouse, production, live evaluation, deployment, release, paid, credential-dependent, or provider-backed test was run.
Risk and rollback
Reconciliation
After this PR lands, run
npm run issues:reconcilefrom a fresh ledger branch. Do not reconcile the canonical ledger inside this PR.