From 9a5f79ab133c6ab9ea2a47e93b0101df8db44607 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 17:58:45 +0000 Subject: [PATCH 1/2] issues: record the lowercase authorizationHeader trap from PR #1741 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit authorizationHeadersForAccessToken returns lowercase `authorization` per the Fetch/Headers convention, but the value is typed Record, so reading `.Authorization` type-checks, returns undefined, and degrades to whatever fallback the caller wrote. PR #1741 made that mistake twice in one session and both sites were identity-scoping code — the in-flight signed-URL dedupe key collapsed every user onto one key, and the detail-window signature omitted the token it documented as present. Review caught both before merge. Recording the trap so the next person keying identity off that object meets it as a note rather than a defect. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo --- docs/outstanding-issues.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 4b8f7dab37..5ca485dc7e 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -165,7 +165,7 @@ removed after current-main verification; it is not missing recommended work. | 112 | `#257` | Optional | High — formulation/specifiers flake | Standing until second reproduction | 15–30 min | Single unreproduced ui-formulation flake when run with ui-specifiers — record a second sighting only; do not quarantine until three on the same SHA. **Stop:** do not weaken assertions. | - + ## Open items > **Merged-main canary update (2026-07-23, run `30018289898`):** the new structured report correctly recorded evaluated tree `c24f2e8f2d30d0c59fc1eba025d3dcd63478137e`, run/attempt identity and `cross-region-runner` latency context. Golden retrieval remained 36/36 with document/content recall 1.0 and no failed cases. The 44-case answer gate had grounded-supported and unsupported-correct rates of 1.0, but failed because `neuroleptic-side-effect-escalation` again returned one citation where two are required (citation-failure rate 0.0227). `admission-discharge-comparison` again omitted the specific AKG admission document after `comparison_source_extractive_fallback`; `admission-discharge-coverage-paraphrase` was advisory-only at 24,870 ms. Answer cost was reported as `$0.234736`. Do not retry immediately: retain this as the first structured datapoint, compare it with the scheduled 2026-07-26 report, and keep retrieval/ranking unchanged. @@ -321,6 +321,7 @@ removed after current-main verification; it is not missing recommended work. | #282 | P3 | task | Probe the corpus for JBIG2/JPX before deciding whether pdf.js needs its decoder assets shipped | **Outcome:** a measured decision about pdf.js's cMap/standard-font/WASM assets rather than an assumption either way. **Detail:** getDocument is configured with url plus the on-demand fetch flags and nothing else, so 'wasmUrl', 'standardFontDataUrl', 'cMapUrl' and 'iccUrl' are all unset. pdfjs-dist ships those assets (wasm 1.5 MB, standard_fonts 804 KB, cmaps 1.7 MB) and nothing copies them into public/. With wasmUrl null, 'useWorkerFetch' resolves false and the WASM image decoders cannot load, so JBIG2 and JPEG2000 images fall back to the JS decoders or fail; those are exactly the encodings a scanned guideline uses, and this repo runs an OCR pipeline, which implies scanned sources exist. Non-embedded standard-14 fonts fall back to system fonts, which is a fidelity risk on a clinical document rather than a failure. **Next:** sample the real corpus for JBIG2/JPX-encoded images and for PDFs relying on the standard 14 before shipping ~2 MB of static assets; if the corpus does use them, copy into public/pdfjs, set the URLs, and add immutable cache headers in next.config.ts (public/ is not counted by check:bundle-budget, so there is no budget risk — the cost is bytes over the wire on first use). **Stop:** do not ship the assets on the assumption alone. | session 2026-08-08 document-viewer optimisation; node_modules/pdfjs-dist/types/src/display/api.d.ts | 2026-08-08 | | #283 | P3 | rec | The 100-id batch signed-URL route still has no caller | **Outcome:** either the batch minter is used or it is retired, rather than sitting as an untested, unreachable privileged surface. **Detail:** src/app/api/images/signed-urls/route.ts POSTs up to 100 image ids and returns their signed URLs, with its own rate limit, owner scoping and committed-generation filter. Nothing in src/ calls it — only tests/private-access-routes.test.ts imports it. The viewer resolves images one at a time through use-signed-image-url.ts. The 2026-08-08 pass added in-flight deduplication there, which removes the duplicate-consumer case (a figure and its lightbox racing for the same asset) but not the many-distinct-images case: a page of N figures is still N round trips where one batch call would do. **Next:** decide deliberately — wire the rail/filmstrip to the batch route when a page mounts several distinct images at once, or delete the route and its tests. The cost of leaving it is a privileged endpoint no product code exercises. **Stop:** if wiring it, keep the per-image endpoint for the lightbox's retry path; do not make the batch the only way to mint a URL. | session 2026-08-08 document-viewer optimisation; src/app/api/images/signed-urls/route.ts | 2026-08-08 | | #284 | P3 | issue | tests/pr-handoff-stop.test.ts fails whenever the suite runs as root | **Outcome:** 'npm run test' is green in a root container, so a real failure is not hidden behind a known one. **Detail:** 'pr-handoff-stop hook > emits handoff context only when the marker file exists' expects markerExists('sess-readonly') to be false — it makes the marker directory read-only and asserts the hook could not write there. Root ignores the permission bits, so the write succeeds and the assertion fails. Reproduced on an unmodified bc33d41 checkout as well as on the viewer-optimisation branch, so it is environment-dependent, not a regression. Cost is that every full-suite run in a root container reports '1 failed', which trains readers to skim past the failure count. **Next:** skip the case when 'process.getuid?.() === 0' with an explicit reason, or drop privileges for that assertion. **Stop:** do not delete the coverage — the read-only case is the point of the test on a normal user account. | session 2026-08-08 full-suite runs; reproduced on bc33d41 | 2026-08-08 | +| #285 | P2 | issue | authorizationHeader is lowercase, and reading .Authorization off it fails silently | **Outcome:** nobody keys identity off `authorizationHeader` by property again without hitting a gate or a comment first. **Detail:** `authorizationHeadersForAccessToken` returns `{ authorization: 'Bearer …' }` — lowercase, per the Fetch/Headers convention (`src/lib/supabase/client.tsx:82`). The value is typed `Record`, so reading `.Authorization` type-checks, returns undefined, and degrades to whatever fallback the caller wrote. On PR #1741 the same mistake was made twice in one session and both were identity-scoping code: the in-flight signed-URL dedupe key in `use-signed-image-url.ts` collapsed every user onto `endpoint+''`, so an account switch with a request in flight could hand user B user A's signed URL; and `detailRequestSignature` in `DocumentViewer.tsx` omitted the token it documented as being present (weaker in practice — `authStatus`/`initialDetailIdentityStale` and the render-time identity reset still moved on a real switch — but the stated defence was not the shipped one). Review caught both before merge and each site now reads `headers.authorization ?? headers.Authorization ?? ''`. Merged main has no other property reads: every remaining caller passes the header object wholesale to `fetch`, where casing is irrelevant. **Next:** cheapest first — export a named helper (e.g. `authorizationIdentity(headers)`) from the auth module and use it at both sites so there is one definition, then consider a lint rule or a narrower type (a branded `AuthorizationHeader` with a lowercase-only key) so `.Authorization` stops type-checking at all. **Stop:** do not 'fix' this by emitting uppercase from `authorizationHeadersForAccessToken` — lowercase is the correct convention and callers pass the object to `fetch`. | PR #1741 review (Codex signed-URL/cache finding); src/lib/supabase/client.tsx:82; session 2026-08-08 | 2026-08-08 | ## Resolved / archive From 9b092076a619f8c9622b2d98b94405b9c41a20fc Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 18:00:04 +0000 Subject: [PATCH 2/2] docs(ledger): record the authorizationHeader trap capture for PR #1754 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo --- docs/branch-review-ledger.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index cbf48bc2d3..c07129e8b7 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -758,3 +758,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-08 | claude/document-image-mobile-view-30xzw8 | 2394d903a6ca1ba7a84e380c9ed5cada038fa5c0 | document-viewer phone image layout + lightbox geometry (PR #1737) | implemented: capped rail/body grid tracks, removed aspect-ratio min-height transfer, rebuilt phone image viewer (legible open scale, rotation re-fit, clamped pan, double-tap, footer controls) | lint, typecheck, test (5647 pass / 1 pre-existing fail), build, eval:rag:offline, check:bundle-budget, all verify:pr-local static steps by hand; browser gates blocked by #255 | | 2026-08-08 | claude/document-image-mobile-view-30xzw8 | d257df7e11913db1d367535171fac726f47e7f1c | PR #1737 document-viewer phone image review-and-fix | fixed P1 expand fixture/threshold + P2 double-tap stage coords/pointer-up + resize re-clamp; Production UI timeout root cause cleared; merge-tree clean | verify:pr-local PASS (525 files/5653 tests); lint; typecheck; focused vitest 64/64; Production UI delegated to CI | | 2026-08-08 | PR #1740 / claude/inpage-nav-info-pages-v8rhnd | b67f33f65e00529eb0dd1682d6925e708243ee93 | Extract InPageNavHeader (default in-page nav template) + convert differentials detail; PR 1 of 3 | HANDOFF. Template extracted from the duplicated DocumentViewer/differential-detail markup into src/components/in-page-nav/ (InPageNavHeader, PageSection/toDocumentSections, usePageSectionWeights); differential-detail-page converted (-207 lines), behaviour-neutral. section-index.ts untouched so document tests unaffected. DocumentViewer deliberately NOT converged (owns h1, edge-glass-header, visual baselines) - follow-up. Anchor-offset hook generalisation deferred to PR 2 where it is consumed. 3 source-scanning contracts + addon-slot guard updated to follow the markup and additionally assert adoption; addon-slot scan widened to InPageNavHeader or it would go silent for every future adopter. Single failing test (pr-handoff-stop) is a root-uid artifact: chmod 0555 does not block root, reproduced with work stashed on clean tree. | verify:cheap 5618 passed/1 failed (root artifact); verify:pr-local same, short-circuits at test so build not reached; build run separately - Compiled successfully in 53s + client bundle secret check passed; verify:phone-chrome EXIT=0 (stage1 119 passed, stage2 7 passed 23.5s, full UI policy auto not selected); lint/typecheck/prettier --check . clean. No provider-backed gates. Deps installed with engine check relaxed (user-approved; Node 24.13.0 vs jsdom floor 24.15) - lockfile untouched. | +| 2026-08-08 | claude/document-viewer-optimization-tu8tnj | 9a5f79ab133c6ab9ea2a47e93b0101df8db44607 | docs-only: one outstanding-issues row (#285) recording the lowercase authorizationHeader trap surfaced by PR #1741 review | ship: PR #1754 | check:outstanding-issues (283 rows, unique ids, next-id above highest), prettier --check on the changed file; no source touched so lint/typecheck/test/build have no changed failure path |