From 96714db8e4fb54a00eb9c70e5f69050326e7c46f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 15:49:47 +0000 Subject: [PATCH 1/8] issues: queue four document-viewer ledger corrections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 2d270392, 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 (65cd909f) 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 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- .../3f1a672f-9038-4a36-897f-5286e4fa028d.json | 13 +++++++++++++ .../89ac3ee8-550a-44aa-b411-88687e5935e4.json | 10 ++++++++++ .../9db75fae-07e6-454d-b2c4-eb88b0e8ff06.json | 10 ++++++++++ .../c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json | 10 ++++++++++ 4 files changed, 43 insertions(+) create mode 100644 docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json create mode 100644 docs/outstanding-issues-inbox/89ac3ee8-550a-44aa-b411-88687e5935e4.json create mode 100644 docs/outstanding-issues-inbox/9db75fae-07e6-454d-b2c4-eb88b0e8ff06.json create mode 100644 docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json diff --git a/docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json b/docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json new file mode 100644 index 0000000000..65e8264eb6 --- /dev/null +++ b/docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "id": "3f1a672f-9038-4a36-897f-5286e4fa028d", + "createdOn": "2026-08-13", + "action": "add", + "payload": { + "pri": "P3", + "type": "task", + "summary": "Crop to page overlay is the last unbuilt document-viewer capability, and its geometry is fetched then dropped", + "detail": "**Outcome:** selecting an indexed table or diagram can highlight its region on the PDF page, or the capability is deliberately retired — either way it stops living only in a plan document. **Detail:** this is the one Phase 3 capability never built (docs/plans/document-viewer-redesign-plan.md, Phase 3 table, 'Out of scope'). It had no ledger row until now, which is how work disappears between sessions: the plan doc marks it out of scope and nothing in durable memory says it remains owed. **The data already exists and is thrown away.** src/lib/document-detail.ts SELECTs bbox alongside the other image columns, but bbox is absent from DocumentDetailImage in src/lib/document-detail-contract.ts AND from ImageRow in src/components/document-viewer/types.ts, so the geometry reaches the server loader and never reaches the viewer. Verified against main 2d270392 on 2026-08-13. **Shape of the work, in order:** (1) plumb bbox through the contract — document-detail-contract.ts, the loader projection, and the viewer's ImageRow — including whatever normalisation the stored value needs; coordinate space and page-relative-vs-absolute are UNVERIFIED, so check ingestion before trusting it. (2) Only then the viewer half: draw the highlight over the rendered page when a figure is selected, which must now account for the virtualized page column, the per-page raster scale from resolveViewportScale, and rotation. **Why it was scoped out rather than overlooked:** step 1 is a src/lib/**document** contract change with a wider blast radius than the component-only diff Phase 3 was, and crop geometry quality from ingestion is separate debt — the redesign plan's residual-risk section says not to block viewer UX on perfect crops. **Stop:** do not land the contract half inside a viewer-only PR; it changes what document detail returns, so it needs its own review and governance preflight. Do not build the overlay against unvalidated bbox values — a highlight over the wrong region of a clinical source is worse than no highlight.", + "source": "session 2026-08-13 document-viewer remaining-work inventory; docs/plans/document-viewer-redesign-plan.md Phase 3 table; src/lib/document-detail.ts bbox projection" + } +} diff --git a/docs/outstanding-issues-inbox/89ac3ee8-550a-44aa-b411-88687e5935e4.json b/docs/outstanding-issues-inbox/89ac3ee8-550a-44aa-b411-88687e5935e4.json new file mode 100644 index 0000000000..711363f790 --- /dev/null +++ b/docs/outstanding-issues-inbox/89ac3ee8-550a-44aa-b411-88687e5935e4.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "89ac3ee8-550a-44aa-b411-88687e5935e4", + "createdOn": "2026-08-13", + "action": "update", + "payload": { + "id": "#280", + "detail": "**Outcome:** the two phone-only viewer fixes are confirmed on the device class they were written for. **Detail:** the viewer-optimisation branch revives pinch-to-zoom in fit mode (it was gated off in the default state, so a pinch reached neither the viewer nor the browser) and adds a canvas pixel budget so WebKit stops blanking the page above roughly 2.3x zoom on a dpr-3 display. Neither is verifiable in this container (see the Chromium/pdf.js row) and neither is a Chromium behaviour anyway — the canvas ceiling is a WebKit limit and the touch-action contention is a Safari gesture question. **Next:** on a real iPhone, in Safari and in the installed PWA: pinch a freshly opened document and confirm it zooms without first tapping a control; zoom to maximum and confirm the page stays painted rather than going blank; confirm a pinch that drifts vertically is not cancelled mid-gesture by the holder's 'touch-action: pan-y' (the mitigation if it is, is switching touch-action to none while two pointers are down — the gesture hook already tracks pointer count and exposes 'pinching'). Record the result against docs/phone-chrome-physical-acceptance.md. **Stop:** do not re-gate pinch on '!fitWidth' to resolve a gesture-contention finding — that restores the original defect. **THIRD ACCEPTANCE ITEM ADDED 2026-08-13 (document viewer Phase 3, PR #1772).** Multi-page documents now read as one continuous column inside a bounded reading pane: pdf-canvas-viewer.tsx gives the holder max-h-[72vh] sm:max-h-[80vh] when totalPages > 1 and not fullscreen, so the holder becomes the scroller, and its overscroll changes from overscroll-contain to overscroll-x-contain precisely so vertical scroll chains OUT of the pane at its ends rather than trapping the reader. Single-page documents keep their previous geometry exactly and need no re-check. A nested vertical scroller inside a page is a known iOS hazard and no Chromium gate says anything about it, so it belongs on this same device pass. **On a real iPhone, in Safari and the installed PWA:** open a multi-page document (the 2-page synthetic clozapine demo doc, or any real guideline), confirm pages scroll inside the pane, and confirm that reaching its top or bottom continues scrolling the page rather than dead-ending. **Stop:** if it does trap, do not fix it by removing the pane — the pane is what makes a long guideline readable; adjust the overscroll behaviour or the pane height instead." + } +} diff --git a/docs/outstanding-issues-inbox/9db75fae-07e6-454d-b2c4-eb88b0e8ff06.json b/docs/outstanding-issues-inbox/9db75fae-07e6-454d-b2c4-eb88b0e8ff06.json new file mode 100644 index 0000000000..6aa87600a2 --- /dev/null +++ b/docs/outstanding-issues-inbox/9db75fae-07e6-454d-b2c4-eb88b0e8ff06.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "9db75fae-07e6-454d-b2c4-eb88b0e8ff06", + "createdOn": "2026-08-13", + "action": "done", + "payload": { + "id": "#278", + "outcome": "Resolved by PR #1743 (65cd909f, 'mask the pinned chrome in the document-viewer baseline, and add a baseline adopt helper'). Confirmed by inspection against main 2d270392 on 2026-08-13: tests/ui-visual-baseline.spec.ts carries mask: [\"[data-document-sticky-header]\", \".document-viewer-composer\"] on the document-viewer target, which is the second of the two remedies this row proposed. The header selector is the document-specific data attribute rather than .edge-glass-header, because the universal search header also carries that class and would have kept the fail-loud mask guard green after a DocumentViewer rename. The clip was NOT narrowed and fullPage was NOT used — the latter correctly avoided because ledger #093 leaves a hidden duplicate page root under CI load. Residual, deliberately not reopened: a mask is a hole in the gate, so those two elements' geometry is now covered only by the phone-chrome contracts in docs/search-chrome-behaviour.md, which is the trade the fix's own comment records." + } +} diff --git a/docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json b/docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json new file mode 100644 index 0000000000..fbbbb0f4d5 --- /dev/null +++ b/docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "c15dfcea-2b4e-4c25-983e-c699ef7d1a52", + "createdOn": "2026-08-13", + "action": "update", + "payload": { + "id": "#215", + "detail": "**Outcome:** the image-only findings from the 2026-08-02 audit are each shipped or explicitly dropped. **RESTATED 2026-08-13 after inspection against main 2d270392 — two of the four items already shipped and the row no longer describes them as open.** DONE: src/components/clinical-dashboard/image-lightbox.tsx carries decoding=\"async\" (Phase 0, PR #1660), asserted by tests/signed-image.dom.test.tsx. DONE: SignedImage has the priority prop for above-fold evidence — it also skips the IntersectionObserver deferral entirely — and document viewer Phase 3 (PR #1772) added the other half of that pair: an explicit fetchPriority of high when priority is set and low otherwise, so a deferred rail figure does not contend with the page's own above-the-fold work. The document rail additionally passes a 240px observer root margin against the shared 640px default. REMAINING, both confirmed by inspection rather than inferred: (a) src/components/pwa-lifecycle.tsx still has no decoding attribute; (b) public/demo-documents/ still contains no .webp — the PNGs are ~80 KB each and served as-is, so the conversion with a PNG fallback has not been done. **Next:** apply decoding=async in pwa-lifecycle.tsx, and either convert the demo PNGs to WebP with a PNG fallback or record that an ~80 KB synthetic demo asset is not worth the build step. **Stop:** do not treat this row as covering the broader performance findings — those live under #016, #013, #117 and #147." + } +} From 263a3803b18f26e7277e0cf3262beaafbda54042 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 15:49:47 +0000 Subject: [PATCH 2/8] docs(plans): stop the viewer plan claiming Phase 3 is unmerged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/plans/document-viewer-redesign-plan.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/plans/document-viewer-redesign-plan.md b/docs/plans/document-viewer-redesign-plan.md index 55e061bea6..7c71edb626 100644 --- a/docs/plans/document-viewer-redesign-plan.md +++ b/docs/plans/document-viewer-redesign-plan.md @@ -4,7 +4,7 @@ **Branch seed:** `cursor/document-viewer-redesign-plan-1db8` **Flightplan evidence:** `.local/workflow-evidence/2026-08-06T17-27-05-553Z-flightplan.json` **Related ledger:** `#214` / `#219` resolved; `#215` residual (PWA/demo WebP) optional -**Landed:** Phase 0 [#1660](https://github.com/BigSimmo/Database/pull/1660), Phase 1 [#1665](https://github.com/BigSimmo/Database/pull/1665), Phase 2 [#1741](https://github.com/BigSimmo/Database/pull/1741), Phase 3 (this branch) +**Landed:** Phase 0 [#1660](https://github.com/BigSimmo/Database/pull/1660), Phase 1 [#1665](https://github.com/BigSimmo/Database/pull/1665), Phase 2 [#1741](https://github.com/BigSimmo/Database/pull/1741), Phase 3 [#1772](https://github.com/BigSimmo/Database/pull/1772) **Phase details:** [`document-viewer-phase2-unified-chrome.md`](./document-viewer-phase2-unified-chrome.md) · [`document-viewer-phase3-handover.md`](./document-viewer-phase3-handover.md) This is the execution plan for a dramatic improvement of design, style, approach, @@ -164,7 +164,7 @@ Execution brief: [`document-viewer-phase3-handover.md`](./document-viewer-phase3 | Capability | Status | | ---------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Multi-page virtualization | **Done** — windowed page column, document-wide live-canvas budget, idle ±1 render-ahead | -| Crop → page overlay | **Out of scope** — needs `bbox` through `DocumentDetailImage`; a wider contract change than this phase | +| Crop → page overlay | **Out of scope, now tracked as a ledger row** — needs `bbox` through `DocumentDetailImage` and `ImageRow` | | Keyboard reading mode | **Done** — Page Up/Down, Home/End, `f` fit, `r` rotate; `docs/wiring-conventions.md` | | Rail virtualization | **Done** — `DocumentImageList` windows `#source-images` and the audit list | | Smarter signed-URL / decode priority | **Done** — explicit `fetchPriority`, tighter rail root margin; batch route still deferred (`#283`) | From 648b86ad079e563c7be830ef07dedf63a1ff91a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 15:54:23 +0000 Subject: [PATCH 3/8] issues: queue the stale-runtime provisioning gap found while verifying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- .../faa4299e-0a68-4747-9509-e3eb2578b64c.json | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json diff --git a/docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json b/docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json new file mode 100644 index 0000000000..a7701cc309 --- /dev/null +++ b/docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "id": "faa4299e-0a68-4747-9509-e3eb2578b64c", + "createdOn": "2026-08-13", + "action": "add", + "payload": { + "pri": "P3", + "type": "issue", + "summary": "A long-lived session that re-bases onto newer main never runs the Node provisioning hook, and running it by hand aborts before it exports PATH", + "detail": "**Outcome:** a session whose checkout advances past a new SessionStart hook still ends up on a supported runtime, instead of failing every gate at step one with no obvious cause. **Detail, observed 2026-08-13.** #285 closed as resolved because .claude/hooks/session-start.sh provisions Node 24.19.0 and 'npm ci succeeded in-session' (PR #1771). That fix is real, but it only helps sessions that START after it exists. A session that began on an older main and later did 'git checkout -B origin/main' acquires the hook file without ever executing it — SessionStart hooks do not re-fire on checkout — so PATH stays on the container's Node 24.13.0. Since package.json now declares engines.node >=24.15.0 <25, the consequences are total rather than partial: 'npm ci' refuses outright (notsup), and check:runtime is the FIRST step of verify:pr-local, so no verify gate can complete for any diff, including a docs-only one. The failure names the Node version but nothing points at the hook as the remedy. **Second, smaller defect found while working around it:** running 'bash .claude/hooks/session-start.sh' manually aborts with 'CLAUDE_ENV_FILE: unbound variable' — the script runs under set -u and references that variable unguarded at line 51. It aborts AFTER the tarball has downloaded and extracted but BEFORE it exports PATH and before its npm ci, so the operator sees a failure, gets no runtime, and cannot tell that the download actually succeeded. Working around it needed 'export PATH=/root/.node24/node-v24.19.0-linux-x64/bin:$PATH' by hand; after that npm ci and the full verify:pr-local passed (11 checks completed, none failed). **Next:** guard the CLAUDE_ENV_FILE write (${CLAUDE_ENV_FILE:-} plus a skip when empty) so manual invocation is a supported path, and consider having check:runtime's failure message name the hook as the remedy — the error currently says 'Install Node 24.15.0 or newer' when the repo already ships a script that does exactly that. **Stop:** do not lower engines.node to accommodate a stale container; the floor is real (jsdom@30 requires it) and #285 raised it deliberately.", + "source": "session 2026-08-13 document-viewer ledger pass; .claude/hooks/session-start.sh:51; package.json engines; archived #285 / PR #1771" + } +} From 7d88d9556c2fe8c86f1b3ccc394ff8c65e6d4240 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 16:08:04 +0000 Subject: [PATCH 4/8] docs(ledger): record the truth-pass review for PR #1930 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- ...9449b5ec74d46133bfbd30b4eafff3aef196f42c11095b095f0.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/80267ba60c8b79449b5ec74d46133bfbd30b4eafff3aef196f42c11095b095f0.record.md diff --git a/docs/branch-review-records/80267ba60c8b79449b5ec74d46133bfbd30b4eafff3aef196f42c11095b095f0.record.md b/docs/branch-review-records/80267ba60c8b79449b5ec74d46133bfbd30b4eafff3aef196f42c11095b095f0.record.md new file mode 100644 index 0000000000..58521bd963 --- /dev/null +++ b/docs/branch-review-records/80267ba60c8b79449b5ec74d46133bfbd30b4eafff3aef196f42c11095b095f0.record.md @@ -0,0 +1 @@ +| 2026-08-13 | claude/viewer-ledger-truth-pass | 648b86ad079e563c7be830ef07dedf63a1ff91a5 | document-viewer ledger truth pass: queue five inbox requests (crop-overlay row, #278 done, #215 restated, #280 third acceptance item, stale-runtime provisioning gap) plus one plan-doc correction | PR #1930 opened. Inventory of remaining document-viewer work found four ledger rows stating things no longer true; every claim re-verified against main 2d270392 rather than the four-day-stale base the inventory began on. Crop to page overlay had NO row at all despite being the one unbuilt Phase 3 capability - its geometry is SELECTed at document-detail.ts and dropped before DocumentDetailImage and ImageRow. #294 and #283 left alone as deliberate deferrals. Queued as inbox requests under the new intake contract, canonical ledger untouched; an earlier attempt on the stale base had allocated #295, which is now taken on main by an unrelated row - reconciliation assigns ids instead. Opened from a fresh branch with user agreement: the designated branch holds dead #1777 history, force-push was blocked by check:ledger-write-discipline diffing against that stale tip, and remote branch deletion is refused by this session's transport. | verify:pr-local COMPLETE, failed: (none) - all 11 selected checks passed (check:runtime, installed-lock-parity, format:changed, sitemap:check, four docs checks, branch-review-ledger, outstanding-issues, ledger-write-discipline). Docs-only scope so build/lint/typecheck/unit/RAG skipped by risk routing, confirmed via --dry-run first. Gate only became runnable after provisioning Node 24.19.0 by hand - queued as its own finding. | From 7a666bb1dd1645bbf04e38aa52cc250a0c94e259 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 16:17:24 +0000 Subject: [PATCH 5/8] fix(hooks): make the Node provisioning hook survive being run by hand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 2d270392. 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 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- .claude/hooks/session-start.sh | 25 +++++- scripts/check-runtime.ts | 9 +- tests/session-start-hook.test.ts | 138 +++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 3 deletions(-) create mode 100644 tests/session-start-hook.test.ts diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index afbf9d0ac7..e52d960fb9 100755 --- a/.claude/hooks/session-start.sh +++ b/.claude/hooks/session-start.sh @@ -48,7 +48,25 @@ fi if [ -x "$NODE_BIN/node" ]; then export PATH="$NODE_BIN:$PATH" - echo "export PATH=\"$NODE_BIN:\$PATH\"" >> "$CLAUDE_ENV_FILE" + # CLAUDE_ENV_FILE and CLAUDE_PROJECT_DIR below are set by Claude Code when this + # runs as a hook, and unset when a human or agent runs it by hand. Under + # `set -u` an unguarded expansion aborts the script — and it aborts *here*, + # after the tarball has downloaded and extracted but before PATH is exported + # and before the install below, so the operator sees a failure, gets no + # runtime, and has no way to tell the download actually succeeded. + # + # That matters because manual invocation is not a hypothetical: SessionStart + # hooks do not re-fire when a long-lived session re-bases onto a newer main, so + # a session that predates this file acquires it without ever running it, stays + # on the container's older Node, and then fails `check:runtime` — the first + # step of verify:pr-local — for every diff. Running this script is the remedy, + # so it has to work when run. + if [ -n "${CLAUDE_ENV_FILE:-}" ]; then + echo "export PATH=\"$NODE_BIN:\$PATH\"" >> "$CLAUDE_ENV_FILE" + else + echo "[session-start] CLAUDE_ENV_FILE is unset (run by hand?); PATH is exported for this process only." + echo "[session-start] To keep it: export PATH=\"$NODE_BIN:\$PATH\"" + fi fi if ! supported_runtime "$(node -v 2>/dev/null | sed -E 's/^v//' || true)"; then @@ -57,7 +75,10 @@ fi echo "[session-start] Using node $(node -v) / npm $(npm -v)" -cd "$CLAUDE_PROJECT_DIR" +# Fall back to the repository this script lives in, derived from its own path +# rather than from the caller's cwd, so a manual run installs into the right tree +# from anywhere. +cd "${CLAUDE_PROJECT_DIR:-"$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"}" # npm ci keeps the lockfile untouched (npm install rewrites peer/optional # metadata and dirties the worktree). A bare "node_modules exists" check is not # enough: a cached container keeps stale node_modules after dependency-bumping diff --git a/scripts/check-runtime.ts b/scripts/check-runtime.ts index 8101c7fec4..c8bd4c4ce5 100644 --- a/scripts/check-runtime.ts +++ b/scripts/check-runtime.ts @@ -74,7 +74,14 @@ export function checkNodeRuntime( ok: false, expectedMajor, actualVersion: version, - message: `Node ${version} is below the ${minimumVersion} floor this project requires (package.json engines.node). Install Node ${minimumVersion} or newer.`, + // Name the remedy the repo already ships. This check is the first step of + // verify:pr-local, so it is where a stale-runtime session lands for every + // diff — and "install Node yourself" sends the reader off to do by hand + // what `.claude/hooks/session-start.sh` does correctly, including the + // exclusive major ceiling that a manual install of "latest" would miss. + message: + `Node ${version} is below the ${minimumVersion} floor this project requires (package.json engines.node). ` + + `Run \`bash .claude/hooks/session-start.sh\` to provision a supported Node, or install ${minimumVersion} or newer yourself.`, }; } diff --git a/tests/session-start-hook.test.ts b/tests/session-start-hook.test.ts new file mode 100644 index 0000000000..d10d0f0759 --- /dev/null +++ b/tests/session-start-hook.test.ts @@ -0,0 +1,138 @@ +import { createHash } from "node:crypto"; +import { spawnSync } from "node:child_process"; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; + +/** + * `.claude/hooks/session-start.sh` provisions a Node meeting the repo's + * engine floor. It normally runs as a SessionStart hook, where Claude Code + * supplies `CLAUDE_ENV_FILE` and `CLAUDE_PROJECT_DIR`. + * + * **Running it by hand is a supported path, not a curiosity.** 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, stays on the container's older Node, and then fails + * `check:runtime` — the first step of `verify:pr-local` — for every diff, + * docs-only ones included. Running this script is the documented remedy, and + * `checkNodeRuntime`'s failure message now names it. + * + * It ran under `set -u` with both variables expanded unguarded, so a manual run + * aborted with `CLAUDE_ENV_FILE: unbound variable` — *after* the Node tarball had + * downloaded and extracted, but before PATH was exported and before the install. + * The operator saw a failure, got no runtime, and had no way to tell the download + * had actually succeeded. These tests pin that it survives both variables being + * absent, and that it still writes the env file when one is provided. + * + * The hook is driven with a stub `node` already in place so it never downloads, + * and with a lockfile stamp that matches, so it never installs. + */ + +const hook = join(process.cwd(), ".claude/hooks/session-start.sh"); +const NODE_VERSION = "24.19.0"; +const scratchRoots: string[] = []; + +afterEach(() => { + for (const root of scratchRoots.splice(0)) { + rmSync(root, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } +}); + +/** + * A HOME containing the exact provisioned Node layout the hook looks for, and a + * project whose dependency stamp already matches its lockfile. + * + * Both halves exist to keep the test hermetic: with a supported `node` on PATH + * the hook skips the network download, and with a current stamp it skips + * `npm ci`. What is left is precisely the variable handling under test. + */ +function stubEnvironment(): { home: string; project: string } { + const home = mkdtempSync(join(tmpdir(), "session-start-home-")); + const project = mkdtempSync(join(tmpdir(), "session-start-project-")); + scratchRoots.push(home, project); + + const nodeBin = join(home, ".node24", `node-v${NODE_VERSION}-linux-x64`, "bin"); + mkdirSync(nodeBin, { recursive: true }); + const stub = join(nodeBin, "node"); + writeFileSync(stub, `#!/bin/bash\necho "v${NODE_VERSION}"\n`); + chmodSync(stub, 0o755); + + const lockfile = join(project, "package-lock.json"); + const lockContents = '{"name":"stub","lockfileVersion":3}\n'; + writeFileSync(lockfile, lockContents); + const stampDir = join(project, "node_modules", ".cache"); + mkdirSync(stampDir, { recursive: true }); + writeFileSync( + join(stampDir, "session-start-lock-hash"), + `${createHash("sha256").update(lockContents).digest("hex")}\n`.trimEnd() + "\n", + ); + + return { home, project }; +} + +function runHook(env: Record, cwd: string) { + const base = { ...process.env, CLAUDE_CODE_REMOTE: "true", ...env }; + for (const [key, value] of Object.entries(env)) { + if (value === undefined) delete (base as Record)[key]; + } + return spawnSync("bash", [hook], { cwd, env: base as NodeJS.ProcessEnv, encoding: "utf8" }); +} + +describe("session-start hook", () => { + it("survives a manual run with no CLAUDE_ENV_FILE or CLAUDE_PROJECT_DIR", () => { + const { home, project } = stubEnvironment(); + + const result = runHook({ HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: project }, project); + + expect(result.stderr).not.toContain("unbound variable"); + expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0); + // It must say so rather than silently exporting into the void, and hand back + // the line that makes the runtime stick. + expect(result.stdout).toContain("CLAUDE_ENV_FILE is unset"); + expect(result.stdout).toContain("export PATH="); + }); + + it("falls back to its own repository when CLAUDE_PROJECT_DIR is absent", () => { + const { home } = stubEnvironment(); + + // Deliberately run from a directory that is neither the repo nor a project: + // the fallback derives from the script's own path, so cwd must not matter. + const elsewhere = mkdtempSync(join(tmpdir(), "session-start-elsewhere-")); + scratchRoots.push(elsewhere); + const result = runHook({ HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: undefined }, elsewhere); + + expect(result.stderr).not.toContain("unbound variable"); + expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0); + // It reached the dependency step in the real repo, which is the only place a + // lockfile exists — proof the fallback resolved somewhere real. + expect(result.stdout).toMatch(/node_modules matches the lockfile|Dependencies (re)?installed|reinstalling/); + }); + + it("still writes the env file when Claude Code provides one", () => { + const { home, project } = stubEnvironment(); + const envFile = join(project, "claude-env"); + writeFileSync(envFile, ""); + + const result = runHook({ HOME: home, CLAUDE_ENV_FILE: envFile, CLAUDE_PROJECT_DIR: project }, project); + + expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0); + const written = readFileSync(envFile, "utf8"); + expect(written).toContain(join(home, ".node24", `node-v${NODE_VERSION}-linux-x64`, "bin")); + expect(written).toContain("export PATH="); + // The manual-run advice belongs only to the manual-run branch. + expect(result.stdout).not.toContain("CLAUDE_ENV_FILE is unset"); + }); + + it("does nothing at all outside a Claude Code remote container", () => { + const { home, project } = stubEnvironment(); + + const result = runHook( + { HOME: home, CLAUDE_CODE_REMOTE: undefined, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: undefined }, + project, + ); + + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe(""); + }); +}); From 9525dd7b55826c974663595ab26196250d2e71e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 16:17:28 +0000 Subject: [PATCH 6/8] issues: queue the residual inbox staleness gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- .../9ae893ef-7073-4dfa-8602-899d53b177b7.json | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 docs/outstanding-issues-inbox/9ae893ef-7073-4dfa-8602-899d53b177b7.json diff --git a/docs/outstanding-issues-inbox/9ae893ef-7073-4dfa-8602-899d53b177b7.json b/docs/outstanding-issues-inbox/9ae893ef-7073-4dfa-8602-899d53b177b7.json new file mode 100644 index 0000000000..4740b3697b --- /dev/null +++ b/docs/outstanding-issues-inbox/9ae893ef-7073-4dfa-8602-899d53b177b7.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "id": "9ae893ef-7073-4dfa-8602-899d53b177b7", + "createdOn": "2026-08-13", + "action": "add", + "payload": { + "pri": "P3", + "type": "rec", + "summary": "A queued update request can silently clobber a row that changed after the request was written", + "detail": "**Outcome:** the inbox cannot apply a stale rewrite over someone else's newer content without anyone noticing. **Detail:** the inbox intake fixed ID allocation — ids are assigned at reconciliation, so two branches can no longer collide on a number, which was the sharper of the two hazards. It does not address content staleness. An 'update' request carries a full replacement '--detail' string written against whatever the author read at queue time; reconciliation applies it verbatim. If the target row changed on main between queueing and reconciling, the newer content is overwritten with no signal. The multiple-pending-mutations guard does not catch this: it fires only when two requests target the same id, not when one request is simply old. **Live near-miss, 2026-08-13:** a document-viewer ledger pass was drafted against a base four days stale, and its '#215' restatement was composed from that stale reading. It was caught only because the author re-read every row against current main before queueing — a discipline, not a gate. The same pass had already had to discard a directly-allocated '#295' because main had since claimed it; that half is now structurally impossible, this half is not. **Next:** consider fingerprinting the target row at queue time — the request schema is versioned ('version: 1'), so a 'baseRow' hash could be added to add/update/done payloads and compared at reconcile, refusing (or requiring an explicit override) when the row moved underneath. Weigh against just documenting the re-read discipline: this costs a schema bump plus writer, reconcile and self-test changes, and the failure needs a multi-day-stale base to bite. **Stop:** do not make reconciliation merge or three-way-diff detail text — a replacement that silently becomes a merge is harder to reason about than one that refuses.", + "source": "session 2026-08-13 document-viewer ledger truth pass, PR #1930; scripts/ledger-inbox.mjs request schema" + } +} From dc2540443de30125902d2b11701b22390265c89e Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:14:30 +0800 Subject: [PATCH 7/8] fix: address PR 1930 review findings --- .claude/hooks/session-start.sh | 12 +- ...8b4573c2e7adb30125925ccccca1596c.record.md | 1 + .../3f1a672f-9038-4a36-897f-5286e4fa028d.json | 4 +- .../c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json | 2 +- .../faa4299e-0a68-4747-9509-e3eb2578b64c.json | 13 --- scripts/check-runtime.ts | 3 +- tests/check-runtime.test.ts | 3 + tests/session-start-hook.test.ts | 107 ++++++++++++------ 8 files changed, 88 insertions(+), 57 deletions(-) create mode 100644 docs/branch-review-records/e45950d8553bb606f144e9ece801d1328b4573c2e7adb30125925ccccca1596c.record.md delete mode 100644 docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index e52d960fb9..e7fb30e98a 100755 --- a/.claude/hooks/session-start.sh +++ b/.claude/hooks/session-start.sh @@ -51,9 +51,10 @@ if [ -x "$NODE_BIN/node" ]; then # CLAUDE_ENV_FILE and CLAUDE_PROJECT_DIR below are set by Claude Code when this # runs as a hook, and unset when a human or agent runs it by hand. Under # `set -u` an unguarded expansion aborts the script — and it aborts *here*, - # after the tarball has downloaded and extracted but before PATH is exported - # and before the install below, so the operator sees a failure, gets no - # runtime, and has no way to tell the download actually succeeded. + # after the tarball has downloaded and after PATH is exported only inside this + # soon-to-exit child process, but before that PATH can be persisted for the + # caller and before the install below. The operator sees a failure and cannot + # tell the download actually succeeded. # # That matters because manual invocation is not a hypothetical: SessionStart # hooks do not re-fire when a long-lived session re-bases onto a newer main, so @@ -64,8 +65,9 @@ if [ -x "$NODE_BIN/node" ]; then if [ -n "${CLAUDE_ENV_FILE:-}" ]; then echo "export PATH=\"$NODE_BIN:\$PATH\"" >> "$CLAUDE_ENV_FILE" else - echo "[session-start] CLAUDE_ENV_FILE is unset (run by hand?); PATH is exported for this process only." - echo "[session-start] To keep it: export PATH=\"$NODE_BIN:\$PATH\"" + echo "[session-start] CLAUDE_ENV_FILE is unset (run by hand?); PATH is active only inside this hook process." + echo "[session-start] To keep it in your invoking shell, run this command there:" + echo "export PATH=\"$NODE_BIN:\$PATH\"" fi fi diff --git a/docs/branch-review-records/e45950d8553bb606f144e9ece801d1328b4573c2e7adb30125925ccccca1596c.record.md b/docs/branch-review-records/e45950d8553bb606f144e9ece801d1328b4573c2e7adb30125925ccccca1596c.record.md new file mode 100644 index 0000000000..c4523743e2 --- /dev/null +++ b/docs/branch-review-records/e45950d8553bb606f144e9ece801d1328b4573c2e7adb30125925ccccca1596c.record.md @@ -0,0 +1 @@ +| 2026-08-13 | PR #1930 / claude/viewer-ledger-truth-pass | 2ac0f48a820be62947112efbb5d0845a702dad8e | heavy review-and-fix: ledger truth, SessionStart runtime remediation, and focused tests | Fixed three confirmed P2 review findings and one additional P2 test-isolation defect. Corrected bbox runtime-flow and #215 completion claims; completed caller-persistent Node remediation; removed the resolved queued runtime issue; no P0/P1 findings. Remaining viewer and ledger requests were source-validated. | bash -n; focused Node/shell regression harness PASS; TypeScript syntax PASS under Node strip-types; JSON parse PASS; distinct manual adversarial pass; full Node 24 npm gates deferred to exact-head CI because no authenticated checkout or compatible local runtime | diff --git a/docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json b/docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json index 65e8264eb6..61994bf7db 100644 --- a/docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json +++ b/docs/outstanding-issues-inbox/3f1a672f-9038-4a36-897f-5286e4fa028d.json @@ -6,8 +6,8 @@ "payload": { "pri": "P3", "type": "task", - "summary": "Crop to page overlay is the last unbuilt document-viewer capability, and its geometry is fetched then dropped", - "detail": "**Outcome:** selecting an indexed table or diagram can highlight its region on the PDF page, or the capability is deliberately retired — either way it stops living only in a plan document. **Detail:** this is the one Phase 3 capability never built (docs/plans/document-viewer-redesign-plan.md, Phase 3 table, 'Out of scope'). It had no ledger row until now, which is how work disappears between sessions: the plan doc marks it out of scope and nothing in durable memory says it remains owed. **The data already exists and is thrown away.** src/lib/document-detail.ts SELECTs bbox alongside the other image columns, but bbox is absent from DocumentDetailImage in src/lib/document-detail-contract.ts AND from ImageRow in src/components/document-viewer/types.ts, so the geometry reaches the server loader and never reaches the viewer. Verified against main 2d270392 on 2026-08-13. **Shape of the work, in order:** (1) plumb bbox through the contract — document-detail-contract.ts, the loader projection, and the viewer's ImageRow — including whatever normalisation the stored value needs; coordinate space and page-relative-vs-absolute are UNVERIFIED, so check ingestion before trusting it. (2) Only then the viewer half: draw the highlight over the rendered page when a figure is selected, which must now account for the virtualized page column, the per-page raster scale from resolveViewportScale, and rotation. **Why it was scoped out rather than overlooked:** step 1 is a src/lib/**document** contract change with a wider blast radius than the component-only diff Phase 3 was, and crop geometry quality from ingestion is separate debt — the redesign plan's residual-risk section says not to block viewer UX on perfect crops. **Stop:** do not land the contract half inside a viewer-only PR; it changes what document detail returns, so it needs its own review and governance preflight. Do not build the overlay against unvalidated bbox values — a highlight over the wrong region of a clinical source is worse than no highlight.", + "summary": "Crop-to-page overlay remains unbuilt; bbox already reaches viewer state at runtime but is untyped, unvalidated, and unused", + "detail": "**Outcome:** selecting an indexed table or diagram can highlight its region on the PDF page, or the capability is deliberately retired — either way it stops living only in a plan document. **Detail:** this is the one Phase 3 capability never built (docs/plans/document-viewer-redesign-plan.md, Phase 3 table, 'Out of scope'). It had no ledger row until now, which is how work disappears between sessions: the plan doc marks it out of scope and nothing in durable memory says it remains owed. **The data path is partially live, not dropped.** src/lib/document-detail.ts SELECTs bbox alongside the other image columns, and withImageTableMetadata spreads every selected field except metadata. bbox therefore survives the runtime response and reaches DocumentViewer's image state. The gap is static and behavioural: DocumentDetailImage in src/lib/document-detail-contract.ts does not declare bbox, ImageRow in src/components/document-viewer/types.ts aliases that contract, no normalisation validates the stored value, and no viewer code renders it. Verified against exact PR head 2ac0f48a820be62947112efbb5d0845a702dad8e on 2026-08-13. **Shape of the work, in order:** (1) establish the ingestion coordinate space and stored shape, add a normalised bbox field to DocumentDetailImage, and add a focused loader or route-serialization test proving bbox survives with the promised shape. Do not change the selected-field mapping unless that test demonstrates an actual loss. (2) Only then draw the highlight over the rendered page when a figure is selected, accounting for the virtualized page column, the per-page raster scale from resolveViewportScale, and rotation. **Why it was scoped out rather than overlooked:** the contract and normalisation work has a wider blast radius than the component-only Phase 3 diff, and crop geometry quality from ingestion is separate debt — the redesign plan's residual-risk section says not to block viewer UX on perfect crops. **Stop:** do not land the typed-contract and normalisation half inside a viewer-only PR; it changes what the document-detail API promises and needs its own review and governance preflight. Do not render raw, unvalidated bbox values — a highlight over the wrong region of a clinical source is worse than no highlight.", "source": "session 2026-08-13 document-viewer remaining-work inventory; docs/plans/document-viewer-redesign-plan.md Phase 3 table; src/lib/document-detail.ts bbox projection" } } diff --git a/docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json b/docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json index fbbbb0f4d5..207253dd4f 100644 --- a/docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json +++ b/docs/outstanding-issues-inbox/c15dfcea-2b4e-4c25-983e-c699ef7d1a52.json @@ -5,6 +5,6 @@ "action": "update", "payload": { "id": "#215", - "detail": "**Outcome:** the image-only findings from the 2026-08-02 audit are each shipped or explicitly dropped. **RESTATED 2026-08-13 after inspection against main 2d270392 — two of the four items already shipped and the row no longer describes them as open.** DONE: src/components/clinical-dashboard/image-lightbox.tsx carries decoding=\"async\" (Phase 0, PR #1660), asserted by tests/signed-image.dom.test.tsx. DONE: SignedImage has the priority prop for above-fold evidence — it also skips the IntersectionObserver deferral entirely — and document viewer Phase 3 (PR #1772) added the other half of that pair: an explicit fetchPriority of high when priority is set and low otherwise, so a deferred rail figure does not contend with the page's own above-the-fold work. The document rail additionally passes a 240px observer root margin against the shared 640px default. REMAINING, both confirmed by inspection rather than inferred: (a) src/components/pwa-lifecycle.tsx still has no decoding attribute; (b) public/demo-documents/ still contains no .webp — the PNGs are ~80 KB each and served as-is, so the conversion with a PNG fallback has not been done. **Next:** apply decoding=async in pwa-lifecycle.tsx, and either convert the demo PNGs to WebP with a PNG fallback or record that an ~80 KB synthetic demo asset is not worth the build step. **Stop:** do not treat this row as covering the broader performance findings — those live under #016, #013, #117 and #147." + "detail": "**Outcome:** two of the four image-only findings from the 2026-08-02 audit are shipped; two remain open for an explicit implementation-or-drop decision. **RESTATED 2026-08-13 after inspection against main 2d270392 — two of the four items already shipped and the row no longer describes them as open.** DONE: src/components/clinical-dashboard/image-lightbox.tsx carries decoding=\"async\" (Phase 0, PR #1660), asserted by tests/signed-image.dom.test.tsx. DONE: SignedImage has the priority prop for above-fold evidence — it also skips the IntersectionObserver deferral entirely — and document viewer Phase 3 (PR #1772) added the other half of that pair: an explicit fetchPriority of high when priority is set and low otherwise, so a deferred rail figure does not contend with the page's own above-the-fold work. The document rail additionally passes a 240px observer root margin against the shared 640px default. REMAINING, both confirmed by inspection rather than inferred: (a) src/components/pwa-lifecycle.tsx still has no decoding attribute; (b) public/demo-documents/ still contains no .webp — the PNGs are ~80 KB each and served as-is, so the conversion with a PNG fallback has not been done. **Next:** apply decoding=async in pwa-lifecycle.tsx, and either convert the demo PNGs to WebP with a PNG fallback or record that an ~80 KB synthetic demo asset is not worth the build step. **Stop:** do not treat this row as covering the broader performance findings — those live under #016, #013, #117 and #147." } } diff --git a/docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json b/docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json deleted file mode 100644 index a7701cc309..0000000000 --- a/docs/outstanding-issues-inbox/faa4299e-0a68-4747-9509-e3eb2578b64c.json +++ /dev/null @@ -1,13 +0,0 @@ -{ - "version": 1, - "id": "faa4299e-0a68-4747-9509-e3eb2578b64c", - "createdOn": "2026-08-13", - "action": "add", - "payload": { - "pri": "P3", - "type": "issue", - "summary": "A long-lived session that re-bases onto newer main never runs the Node provisioning hook, and running it by hand aborts before it exports PATH", - "detail": "**Outcome:** a session whose checkout advances past a new SessionStart hook still ends up on a supported runtime, instead of failing every gate at step one with no obvious cause. **Detail, observed 2026-08-13.** #285 closed as resolved because .claude/hooks/session-start.sh provisions Node 24.19.0 and 'npm ci succeeded in-session' (PR #1771). That fix is real, but it only helps sessions that START after it exists. A session that began on an older main and later did 'git checkout -B origin/main' acquires the hook file without ever executing it — SessionStart hooks do not re-fire on checkout — so PATH stays on the container's Node 24.13.0. Since package.json now declares engines.node >=24.15.0 <25, the consequences are total rather than partial: 'npm ci' refuses outright (notsup), and check:runtime is the FIRST step of verify:pr-local, so no verify gate can complete for any diff, including a docs-only one. The failure names the Node version but nothing points at the hook as the remedy. **Second, smaller defect found while working around it:** running 'bash .claude/hooks/session-start.sh' manually aborts with 'CLAUDE_ENV_FILE: unbound variable' — the script runs under set -u and references that variable unguarded at line 51. It aborts AFTER the tarball has downloaded and extracted but BEFORE it exports PATH and before its npm ci, so the operator sees a failure, gets no runtime, and cannot tell that the download actually succeeded. Working around it needed 'export PATH=/root/.node24/node-v24.19.0-linux-x64/bin:$PATH' by hand; after that npm ci and the full verify:pr-local passed (11 checks completed, none failed). **Next:** guard the CLAUDE_ENV_FILE write (${CLAUDE_ENV_FILE:-} plus a skip when empty) so manual invocation is a supported path, and consider having check:runtime's failure message name the hook as the remedy — the error currently says 'Install Node 24.15.0 or newer' when the repo already ships a script that does exactly that. **Stop:** do not lower engines.node to accommodate a stale container; the floor is real (jsdom@30 requires it) and #285 raised it deliberately.", - "source": "session 2026-08-13 document-viewer ledger pass; .claude/hooks/session-start.sh:51; package.json engines; archived #285 / PR #1771" - } -} diff --git a/scripts/check-runtime.ts b/scripts/check-runtime.ts index c8bd4c4ce5..22f40ef3b1 100644 --- a/scripts/check-runtime.ts +++ b/scripts/check-runtime.ts @@ -81,7 +81,8 @@ export function checkNodeRuntime( // exclusive major ceiling that a manual install of "latest" would miss. message: `Node ${version} is below the ${minimumVersion} floor this project requires (package.json engines.node). ` + - `Run \`bash .claude/hooks/session-start.sh\` to provision a supported Node, or install ${minimumVersion} or newer yourself.`, + `In a Claude Code remote session, run \`bash .claude/hooks/session-start.sh\`, then run the printed ` + + `\`export PATH=...\` command in your current shell before retrying. Otherwise install ${minimumVersion} or newer yourself.`, }; } diff --git a/tests/check-runtime.test.ts b/tests/check-runtime.test.ts index 844cf78439..53a05cb4fb 100644 --- a/tests/check-runtime.test.ts +++ b/tests/check-runtime.test.ts @@ -24,6 +24,9 @@ describe("runtime release gate", () => { const result = checkNodeRuntime("24.13.0"); expect(result.ok).toBe(false); expect(result.message).toContain(NODE_MINIMUM_VERSION); + expect(result.message).toContain("Claude Code remote"); + expect(result.message).toContain("export PATH="); + expect(result.message).toContain("current shell"); }); it("accepts runtimes at or above the floor", () => { diff --git a/tests/session-start-hook.test.ts b/tests/session-start-hook.test.ts index d10d0f0759..b4ee2517c4 100644 --- a/tests/session-start-hook.test.ts +++ b/tests/session-start-hook.test.ts @@ -1,6 +1,14 @@ import { createHash } from "node:crypto"; import { spawnSync } from "node:child_process"; -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + chmodSync, + copyFileSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -16,20 +24,23 @@ import { afterEach, describe, expect, it } from "vitest"; * executing it, stays on the container's older Node, and then fails * `check:runtime` — the first step of `verify:pr-local` — for every diff, * docs-only ones included. Running this script is the documented remedy, and - * `checkNodeRuntime`'s failure message now names it. + * `checkNodeRuntime`'s failure message now names both the hook and the caller- + * persistent export required after a manual invocation. * * It ran under `set -u` with both variables expanded unguarded, so a manual run - * aborted with `CLAUDE_ENV_FILE: unbound variable` — *after* the Node tarball had - * downloaded and extracted, but before PATH was exported and before the install. - * The operator saw a failure, got no runtime, and had no way to tell the download - * had actually succeeded. These tests pin that it survives both variables being - * absent, and that it still writes the env file when one is provided. + * aborted with `CLAUDE_ENV_FILE: unbound variable` after the Node tarball had + * downloaded and after PATH had changed only inside the soon-to-exit child + * process, but before PATH persistence and before the install. These tests pin + * that it survives both variables being absent, prints a command that activates + * the provisioned Node in the invoking shell, and still writes the env file when + * one is provided. * - * The hook is driven with a stub `node` already in place so it never downloads, - * and with a lockfile stamp that matches, so it never installs. + * Each test copies the hook into a temporary project, supplies stub `node` and + * `npm` executables, and writes a matching lockfile stamp. That keeps the tests + * hermetic: they never download, install, or touch the real checkout. */ -const hook = join(process.cwd(), ".claude/hooks/session-start.sh"); +const sourceHook = join(process.cwd(), ".claude/hooks/session-start.sh"); const NODE_VERSION = "24.19.0"; const scratchRoots: string[] = []; @@ -42,21 +53,29 @@ afterEach(() => { /** * A HOME containing the exact provisioned Node layout the hook looks for, and a * project whose dependency stamp already matches its lockfile. - * - * Both halves exist to keep the test hermetic: with a supported `node` on PATH - * the hook skips the network download, and with a current stamp it skips - * `npm ci`. What is left is precisely the variable handling under test. */ -function stubEnvironment(): { home: string; project: string } { +function stubEnvironment(): { home: string; project: string; hook: string } { const home = mkdtempSync(join(tmpdir(), "session-start-home-")); const project = mkdtempSync(join(tmpdir(), "session-start-project-")); scratchRoots.push(home, project); + const hookDir = join(project, ".claude", "hooks"); + mkdirSync(hookDir, { recursive: true }); + const hook = join(hookDir, "session-start.sh"); + copyFileSync(sourceHook, hook); + chmodSync(hook, 0o755); + const nodeBin = join(home, ".node24", `node-v${NODE_VERSION}-linux-x64`, "bin"); mkdirSync(nodeBin, { recursive: true }); - const stub = join(nodeBin, "node"); - writeFileSync(stub, `#!/bin/bash\necho "v${NODE_VERSION}"\n`); - chmodSync(stub, 0o755); + const nodeStub = join(nodeBin, "node"); + writeFileSync(nodeStub, `#!/bin/bash\necho "v${NODE_VERSION}"\n`); + chmodSync(nodeStub, 0o755); + const npmStub = join(nodeBin, "npm"); + writeFileSync( + npmStub, + '#!/bin/bash\nif [ "${1:-}" = "ci" ]; then exit 97; fi\necho "11.17.0"\n', + ); + chmodSync(npmStub, 0o755); const lockfile = join(project, "package-lock.json"); const lockContents = '{"name":"stub","lockfileVersion":3}\n'; @@ -68,10 +87,10 @@ function stubEnvironment(): { home: string; project: string } { `${createHash("sha256").update(lockContents).digest("hex")}\n`.trimEnd() + "\n", ); - return { home, project }; + return { home, project, hook }; } -function runHook(env: Record, cwd: string) { +function runHook(hook: string, env: Record, cwd: string) { const base = { ...process.env, CLAUDE_CODE_REMOTE: "true", ...env }; for (const [key, value] of Object.entries(env)) { if (value === undefined) delete (base as Record)[key]; @@ -80,41 +99,58 @@ function runHook(env: Record, cwd: string) { } describe("session-start hook", () => { - it("survives a manual run with no CLAUDE_ENV_FILE or CLAUDE_PROJECT_DIR", () => { - const { home, project } = stubEnvironment(); + it("survives a manual run with no CLAUDE_ENV_FILE", () => { + const { home, project, hook } = stubEnvironment(); - const result = runHook({ HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: project }, project); + const result = runHook(hook, { HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: project }, project); expect(result.stderr).not.toContain("unbound variable"); expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0); - // It must say so rather than silently exporting into the void, and hand back - // the line that makes the runtime stick. expect(result.stdout).toContain("CLAUDE_ENV_FILE is unset"); expect(result.stdout).toContain("export PATH="); }); + it("prints a PATH command that activates the provisioned Node in the invoking shell", () => { + const { home, project, hook } = stubEnvironment(); + const result = runHook(hook, { HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: project }, project); + + expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0); + const exportLine = result.stdout.split(/\r?\n/).find((line) => line.startsWith("export PATH=")); + expect(exportLine).toBeDefined(); + + const caller = spawnSync("bash", ["-c", `${exportLine}; node -v`], { + cwd: project, + env: { ...process.env, HOME: home }, + encoding: "utf8", + }); + expect(caller.status, `caller exited ${caller.status}: ${caller.stderr}`).toBe(0); + expect(caller.stdout.trim()).toBe(`v${NODE_VERSION}`); + }); + it("falls back to its own repository when CLAUDE_PROJECT_DIR is absent", () => { - const { home } = stubEnvironment(); + const { home, hook } = stubEnvironment(); - // Deliberately run from a directory that is neither the repo nor a project: - // the fallback derives from the script's own path, so cwd must not matter. + // Deliberately run from a directory that is neither the repo nor the + // temporary project: the fallback must derive from the copied hook's path. const elsewhere = mkdtempSync(join(tmpdir(), "session-start-elsewhere-")); scratchRoots.push(elsewhere); - const result = runHook({ HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: undefined }, elsewhere); + const result = runHook( + hook, + { HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: undefined }, + elsewhere, + ); expect(result.stderr).not.toContain("unbound variable"); expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0); - // It reached the dependency step in the real repo, which is the only place a - // lockfile exists — proof the fallback resolved somewhere real. - expect(result.stdout).toMatch(/node_modules matches the lockfile|Dependencies (re)?installed|reinstalling/); + expect(result.stdout).toContain("node_modules matches the lockfile, skipping install"); }); it("still writes the env file when Claude Code provides one", () => { - const { home, project } = stubEnvironment(); + const { home, project, hook } = stubEnvironment(); const envFile = join(project, "claude-env"); writeFileSync(envFile, ""); - const result = runHook({ HOME: home, CLAUDE_ENV_FILE: envFile, CLAUDE_PROJECT_DIR: project }, project); + const result = runHook(hook, { HOME: home, CLAUDE_ENV_FILE: envFile, CLAUDE_PROJECT_DIR: project }, project); expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0); const written = readFileSync(envFile, "utf8"); @@ -125,9 +161,10 @@ describe("session-start hook", () => { }); it("does nothing at all outside a Claude Code remote container", () => { - const { home, project } = stubEnvironment(); + const { home, project, hook } = stubEnvironment(); const result = runHook( + hook, { HOME: home, CLAUDE_CODE_REMOTE: undefined, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: undefined }, project, ); From 8354842bff2f2f6453512f599a57304644aa0463 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:09:07 +0800 Subject: [PATCH 8/8] style: format SessionStart hook test --- tests/session-start-hook.test.ts | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/tests/session-start-hook.test.ts b/tests/session-start-hook.test.ts index b4ee2517c4..49a32b1df4 100644 --- a/tests/session-start-hook.test.ts +++ b/tests/session-start-hook.test.ts @@ -1,14 +1,6 @@ import { createHash } from "node:crypto"; import { spawnSync } from "node:child_process"; -import { - chmodSync, - copyFileSync, - mkdirSync, - mkdtempSync, - readFileSync, - rmSync, - writeFileSync, -} from "node:fs"; +import { chmodSync, copyFileSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -71,10 +63,7 @@ function stubEnvironment(): { home: string; project: string; hook: string } { writeFileSync(nodeStub, `#!/bin/bash\necho "v${NODE_VERSION}"\n`); chmodSync(nodeStub, 0o755); const npmStub = join(nodeBin, "npm"); - writeFileSync( - npmStub, - '#!/bin/bash\nif [ "${1:-}" = "ci" ]; then exit 97; fi\necho "11.17.0"\n', - ); + writeFileSync(npmStub, '#!/bin/bash\nif [ "${1:-}" = "ci" ]; then exit 97; fi\necho "11.17.0"\n'); chmodSync(npmStub, 0o755); const lockfile = join(project, "package-lock.json"); @@ -134,11 +123,7 @@ describe("session-start hook", () => { // temporary project: the fallback must derive from the copied hook's path. const elsewhere = mkdtempSync(join(tmpdir(), "session-start-elsewhere-")); scratchRoots.push(elsewhere); - const result = runHook( - hook, - { HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: undefined }, - elsewhere, - ); + const result = runHook(hook, { HOME: home, CLAUDE_ENV_FILE: undefined, CLAUDE_PROJECT_DIR: undefined }, elsewhere); expect(result.stderr).not.toContain("unbound variable"); expect(result.status, `hook exited ${result.status}: ${result.stderr}`).toBe(0);