Skip to content

Derive the forms PDF password badge from committed bytes and gate it - #2531

Merged
BigSimmo merged 11 commits into
mainfrom
claude/form-12a-warning
Sep 2, 2026
Merged

Derive the forms PDF password badge from committed bytes and gate it#2531
BigSimmo merged 11 commits into
mainfrom
claude/form-12a-warning

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • data/forms-pdf-manifest.json carries a passwordProtected flag per WA Mental Health Act statutory form, and the form detail page renders a clinician-facing badge from it. The manifest was entirely hand-maintained: no generator, no validator, no gate. The existing test compared the flag only against itself, plus one hardcoded literal for Form 12A, so a wrong flag on any of the other 50 forms would have passed silently.
  • Adds scripts/build-forms-pdf-manifest.mjs with a --check mode, following the build-therapies-index.mjs / check:mha-act-sections idiom, and registers check:forms-pdf-manifest in verify:cheap:internal and CI.
  • The flag is derived by attempting to open each PDF with an empty user password, not by looking for an /Encrypt marker. A PDF encrypted with an owner password but no user password carries /Encrypt and still opens freely, so the marker answers a different question than the badge asks. Every committed form happens to agree under both rules — which is exactly why the weaker rule would have survived review — so a synthetic owner-password-only fixture is now the discriminating case, and it is the only test that fails if the deriver is ever simplified to a grep.
  • Any error path (unreadable file, truncated body, malformed xref, unparseable encryption dictionary, unsupported revision) yields passwordProtected: true and a hard non-zero exit, never a silent false. Under-warning is the unsafe direction: false is the assertion that a clinician can open the form.
  • --check fails on a manifest/bytes disagreement rather than auto-correcting, because the manifest also carries the sha256 provenance record. The generator never synthesises an officialPdfUrl, never reorders entries, hard-errors on a PDF with no entry, and pins the publisher host rather than accepting any https URL.
  • The badge reads "Password required" (previously "Password protected").

The manifest data itself is unchanged and regenerates byte-identically; this PR adds the derivation and the gate that keep it honest.

Two review rounds changed the badge copy before merge, in opposite directions. An interim wording read "Opens with a password from the publisher"; a clinical governance review blocked it, because the bytes establish only that opening requires a non-empty user password — not who holds it, whether the publisher issues it, or whether a clinician can obtain it. On a statutory form that is the difference between a warning and a false errand at the bedside. The replacement, "Password required to open", was then measured in a browser and found to squeeze the document title (below); "Password required" says everything the bytes support and fits.

The #9P4XAE closure request rides as its own separately revertible commit.

Verification

  • npm run test (full unit suite) — Test Files 947 passed (947) / Tests 12058 passed | 1 skipped (12059)
  • node scripts/build-forms-pdf-manifest.mjs --checkForms PDF manifest is current (51 PDFs, 50 require a user password).
  • tests/forms.test.ts + tests/source-catalogue-providers.test.tsTest Files 2 passed (2) / Tests 27 passed (27) (re-run after the badge change; still 27 passed)
  • npm run typecheckrecorded a pass for "typecheck:internal" (6001 input files)
  • npm run check:gate-manifest — "all 38 verify:cheap gates are enforced in CI (static-pr + mapped jobs), and the 35 static gates are documented consistently"
  • npm run check:ledger-write-discipline — "Ledger write discipline passed for 45a3dca..HEAD"
  • npx eslint on the changed source files — exit 0
  • npm run format (whole tree) — no files changed

Regeneration without --check produced a byte-identical manifest, which is the evidence that the committed data was already correct under the stricter rule.

Two negative controls were run. Flipping Form 12A's flag to true made the gate exit 1 and the new derived-from-bytes assertion fail with 12A: expected false to be true. Replacing the deriver with a naive /Encrypt grep made exactly the two relevant tests fail — expected { passwordProtected: true, …(1) } to deeply equal { passwordProtected: false, …(1) } on the owner-password fixture, plus the truncated-input case in the fail-closed test. Both were reverted and the suite re-verified green.

npm run test:focused refuses by design here ("Focused test selection is unsafe: test or configuration paths changed") and instructs the full suite, so the full suite is what ran.

Two failures seen on an earlier run of this branch (tests/clinical-hazard-controls.test.ts, tests/rag-plan-package-parity.test.ts) were a shallow-clone artefact — the container held 98 commits and both tests name commits absent from it. After git fetch --deepen=2000 the suite is green as quoted above, with no code change.

Verification not run: npm run check:production-readiness cannot pass in this repository. check:privacy-readiness:release blocks on six release-blocking legal/provider items (OpenAI ZDR, OpenAI and Railway DPAs, APP 8 cross-border basis, APP 1/APP 5 notice, PHI minimisation). The same command on unmodified main at 45a3dca, with no changes, produces identical output — this is repository state, not a consequence of this diff, and none of those items is touched here.

Verification not run: npm run verify:pr-local was not run; its risk-routed selection adds no failure class this diff can reach beyond the full unit suite already run above.

UI verified in a browser. npm run ensure (server identity confirmed via /api/local-project-id), then Chromium at 320px and 360px on /forms/form-10a and /forms/form-12a. No horizontal page overflow at either width.

The first attempt at this badge was measurably worse than main and was fixed before merge. In the PDF row the badge shares a two-column grid with the document title and is not allowed to shrink, so an A/B at 320px changing only that string gave:

"Password protected"        (main)  -> title/subtitle track 70px
"Password required to open"         -> title/subtitle track 35px   <- rejected
"Password required"                 -> title/subtitle track 78px   <- shipped

At 35px the statutory form's title rendered as "Rec…" and its publisher line as "Offic…". The shipped wording leaves the row slightly better off than main.

UI verification not run: the full Chromium journey gate npm run verify:ui was not run because the change is one string with no layout, tone, routing or tap-target change, and the affected row was inspected directly.

Risk and rollout

  • Risk: low — the manifest data is unchanged (the generator reproduces the committed file byte for byte); the diff adds a derivation and a fail-closed check, and rewords one badge. Any detection failure defaults to the warning state, never to the permissive one.
  • Rollback: revert this branch's commits. The manifest content is byte-identical to what is on main, so a revert removes the generator and the check without any data change to repair.
  • Provider or production effects: None — the generator reads committed bytes only and never contacts chiefpsychiatrist.wa.gov.au or any other host.
  • RAG impact: none

Clinical Governance Preflight

  • Source-backed claims still require linked source verification before clinical use
  • No patient-identifiable document workflow was introduced or expanded without explicit governance approval
  • Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy)
  • Service-role keys and private document access remain server-only
  • Demo/synthetic content remains clearly separated from real clinical sources
  • Source metadata, review status, and outdated/unknown-source behavior remain conservative
  • Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed

Notes

scripts/pr-policy.mjs warns that operational-risk changes are bundled with UI changes here. That is unavoidable rather than incidental: check:gate-manifest enforces a one-way invariant that CI must never run less of the verify:cheap static set than the local chain does, so a new local gate cannot be added without registering it in .github/workflows/ci.yml in the same commit. The CLAUDE.md (34 → 35) and .claude/skills/gates/SKILL.md (37 → 38) edits are the documented gate counts the same check compares against, and docs/scripts-index.md is generated inventory. No policy text was altered — only numerals and one curated script entry.

This branch is two commits behind main (569d8565, a1aa449a). git merge-tree --write-tree origin/main reports a clean merge. 569d8565 restructured the instruction docs, but the two gate-count sentences check:gate-manifest reads still live in CLAUDE.md and .claude/skills/gates/SKILL.md on current main, and the new docs/agents/verification-gates.md carries no count — so this branch's numerals still land in the right place after merge. Behind, not conflicted, and deliberately not re-synced to avoid restarting CI for nothing.

🤖 Generated with Claude Code

https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL


Note

Low Risk
Manifest JSON is unchanged when regenerated; the change adds validation and conservative derivation plus a small UI label tweak, with no auth, data migration, or network surface.

Overview
Adds an offline build-forms-pdf-manifest.mjs pipeline so data/forms-pdf-manifest.json is no longer trusted by hand for sha256, bytes, and passwordProtected. passwordProtected is decided by trying to open each committed PDF with an empty user password (pdf.js), not by grepping /Encrypt; corrupt or unclassifiable files fail closed to protected plus a hard exit. check:forms-pdf-manifest (--check) is wired into verify:cheap:internal and CI static-heavy checks so drift against public/forms-pdf/ blocks merge.

tests/forms.test.ts now pins every flag to the bytes, fail-closed error paths, manifest field preservation on regen, and an owner-password-only synthetic PDF so a naive /Encrypt shortcut would fail. The form detail badge copy changes from "Password protected" to "Password required". Docs/gate counts bump to 36 static gates; outstanding issue #9P4XAE is closed in the inbox ledger.

Reviewed by Cursor Bugbot for commit 3fcd6cf. Configure here.

`data/forms-pdf-manifest.json` carries a `passwordProtected` flag per WA
Mental Health Act statutory form, and the form detail page renders a
clinician-facing badge from it. The manifest was entirely hand-maintained —
no generator, no validator, no gate — and the existing test compared the flag
only against itself plus one hardcoded literal for Form 12A, so a wrong flag
on any of the other 50 forms would have passed silently.

`scripts/build-forms-pdf-manifest.mjs` now derives sha256, bytes and
passwordProtected from the committed bytes, offline, with a `--check` mode
registered as `check:forms-pdf-manifest` in `verify:cheap:internal` and CI.

The flag is derived by ATTEMPTING to open each PDF with an empty user
password, not by looking for an `/Encrypt` marker. A PDF encrypted with an
owner password but no user password carries `/Encrypt` and still opens
freely, so the marker answers a different question than the badge asks. Every
committed form happens to agree under both rules, which is exactly why the
weaker rule would have survived review: a synthetic owner-password-only
fixture is now the discriminating case, and it is the only test that fails if
the deriver is ever simplified to a grep.

Every error path — unreadable file, truncated body, malformed xref,
unparseable encryption dictionary, unsupported revision — yields
`passwordProtected: true` and a hard non-zero exit, never a silent `false`.
`false` is the assertion that a clinician can open the form, so under-warning
is the unsafe direction. `--check` fails on a manifest/bytes disagreement
rather than auto-correcting, because the manifest also carries the sha256
provenance record. The generator never synthesises an `officialPdfUrl`, never
reorders entries, hard-errors on a PDF with no entry, and now pins the
publisher host rather than accepting any https URL.

The manifest data itself is unchanged and regenerates byte-identically.

The badge reads "Password required to open". It previously read "Password
protected"; an interim wording claimed the password comes from the publisher,
which nothing here establishes — the bytes prove only that opening requires a
non-empty user password, not who holds it or whether a clinician can obtain
it. On a statutory form that difference is the difference between a warning
and a false errand at the bedside.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
Append-only inbox request recording that the forms PDF manifest now derives
its password flag from committed bytes and gates it. Separate commit so it
stays independently revertible from the product change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: b20662ef-afde-4d78-89e1-6737ed12524b


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@supabase

supabase Bot commented Sep 2, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

…ent title

Verified in Chromium at a 320px viewport, A/B against the same page with only
this string changed. In the PDF row the badge and the title share a two-column
grid, and the badge is not allowed to shrink:

  "Password protected"        (main)  -> title/subtitle track 70px
  "Password required to open"         -> title/subtitle track 35px
  "Password required"                 -> title/subtitle track 78px

At 35px the document title rendered as "Rec…" and its publisher line as
"Offic…". That is a real regression against main, so the longer wording is
not worth its cost: it squeezed the name of the statutory form in order to
describe the lock on it.

"Password required" keeps everything the bytes establish — opening the file
needs a non-empty user password — and still makes no claim about who holds it,
which was the reason the earlier "…from the publisher" wording was rejected.
It is also one character shorter than the string on main, so the row is
slightly better off than before this branch.

No layout or tone class changed; the fix is entirely in the wording.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
@BigSimmo
BigSimmo marked this pull request as ready for review September 2, 2026 05:57
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-02T06:02:25.175299Z 111a5ee Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_54a1303f-ba21-444f-9acd-1a2cfdfdebdc)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 111a5ee089

ℹ️ 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".

Comment thread scripts/build-forms-pdf-manifest.mjs
Codex review, verified. `buildManifest` rebuilt each asset by enumerating six
keys, so any other field an asset carried was dropped on regeneration. The
generator computes exactly three fields — sha256, bytes, passwordProtected —
and everything else is provenance it cannot recompute. This script is offline
by contract, so a dropped `officialPdfUrl` could never be restored from the
PDFs themselves.

The failure mode is quiet and then destructive: `--check` reports drift, and
the regeneration it instructs the operator to run erases the field for good.

The code already disagreed with itself, which is the clearest evidence the
finding is real. The unreadable-file path a few lines above spreads `...asset`
and only overrides the derived fields; the success path did not. They now
match.

Not hypothetical: an `editingRestricted` fact derived from the PDFs' /P
permission bits is already a queued follow-up against this same manifest, so
the first field added would have hit this.

Pinned by a test that adds two extra properties to an asset and asserts they
survive while the three derived fields stay authoritative. Negative control:
restoring the enumerated-keys version fails it with
`expected undefined to be '2026-09-02'`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_dd107e8d-b785-4ec7-bce1-964182954a63)

Resolves the generated docs/scripts-index.md conflict: main's build-and-assets
section plus this branch's build-forms-pdf-manifest.mjs entry, with the counts
sentence regenerated by scripts/update-docs-inventory.mjs rather than hand-edited.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
Same generated-counts conflict as the previous sync: docs/scripts-index.md's
"(N files) / (N entries)" sentence is rewritten by scripts/update-docs-inventory.mjs,
so both sides are regenerated output. Resolved by regenerating (285 files, 288 npm
scripts) rather than picking a side, with this branch's build-forms-pdf-manifest.mjs
entry preserved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_084c88bb-0e98-44e2-af93-040ad20f9c7e)

@BigSimmo
BigSimmo enabled auto-merge (squash) September 2, 2026 09:15
BigSimmo pushed a commit that referenced this pull request Sep 2, 2026
…Y090R5 sibling)

The generated counts sentence at the top of docs/scripts-index.md conflicts on
every concurrent PR — measured five times on PR #2531 in about three hours, at
five different main heads, each time as the sole conflicting line.

Same root cause as #Y090R5 for data/outstanding-issues-snapshot.json: a
single-line generated artefact every PR must regenerate. PR #2530 fixed the
snapshot half by moving regeneration into the serialised reconcile step; this
half is untouched.

Records the two traps found while resolving it: taking main's whole file
silently drops the branch's own new script entry, and a bundled
verify:cheap:internal union merge desynchronises the gate counts that
check:gate-manifest reads.

Append-only inbox request; no canonical ledger edit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
Two conflicts, both from concurrent additions rather than disagreement:

package.json — main added check:diff-integrity to verify:cheap:internal while
this branch added check:forms-pdf-manifest. Neither removed anything, so the
resolution is the union: both gates in the chain, each in its own position.
That in turn desynchronised the gate counts check:gate-manifest compares
against, so CLAUDE.md (35 -> 36 static) and .claude/skills/gates/SKILL.md
(38 -> 39 total) were re-derived from the merged chain, not hand-guessed.

docs/scripts-index.md — the generated counts sentence, for the fifth time on
this branch. Regenerated with scripts/update-docs-inventory.mjs (286 files,
290 npm scripts) rather than picking a side, with this branch's
build-forms-pdf-manifest.mjs entry preserved. Queued as a ledger request on
claude/issues-followups: it is the same single-line-generated-artefact
collision that #Y090R5 records for the issues snapshot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_95e9ac53-38a1-4cd7-bfa5-75ca13c018ff)

Sixth sync of the generated docs/scripts-index.md counts sentence on this
branch. Regenerated with scripts/update-docs-inventory.mjs (287 files, 292 npm
scripts) rather than picking a side; this branch's build-forms-pdf-manifest.mjs
entry preserved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
@BigSimmo
BigSimmo merged commit d1e4ae7 into main Sep 2, 2026
29 checks passed
@BigSimmo
BigSimmo deleted the claude/form-12a-warning branch September 2, 2026 12:46
BigSimmo pushed a commit that referenced this pull request Sep 2, 2026
…form PDFs

A concurrent review comment on this PR asserted that PyMuPDF does not enforce
/P permission bits and that these files therefore "open normally and yield
their text layer". Measured against the committed bytes with PyMuPDF 1.28.0,
the library the worker actually uses, that is false for 50 of the 51 files.

  needs_pass=1, is_encrypted=True, page_count=0
  doc.authenticate("") -> 0        (the empty user password is rejected)
  load_page(0) -> ValueError('document closed or encrypted')

form-12a.pdf is the sole exception: no /Encrypt, opens, 3274 characters of
first-page text — which is exactly the file PR #2531 corrected.

The distinction the comment missed is that /P alone would not block opening;
these files also carry a non-empty /U, and that does. The practical
consequence is that extraction fails at the open call, before
should_ocr_page() is ever reached, so the OCR fallback cannot rescue them.
The JavaScript fallback was not measured and is not claimed either way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL
BigSimmo added a commit that referenced this pull request Sep 4, 2026
…r review (#2544)

* issues: queue seven verified follow-ups from PRs #2538, #2536 and #2531

Seven immutable inbox requests, no canonical ledger edit. Every claim was
checked against the code before it was written: three against origin/main,
four against the PR heads the follow-up belongs to (#2538 ef7c55a,
#2536 e24e0ee, #2531 60f5e8e), since none of the three has merged yet.

- P2 issue: differential-records.ts asserts validation_status
  "locally_reviewed" from a literal over a snapshot that says
  "Pending review" — the sibling of the fix already in medication-records.ts.
- P2 issue: registry-records.ts and differential-records.ts both return the
  frozen source_status column verbatim, and derive it with a substring test
  that also matches "not checked"/"unchecked".
- P3 issue: a stored source_status of "outdated" can never be cleared, since
  nothing writes that column back on any of the three record tables.
- P3 rec: patient-alert rows with action "info" reach neither unassessed
  tier, leaving a green all-clear for nystatin, levetiracetam and lorazepam.
- P3 issue: isProfileEmpty treats a recorded hepatic "none" as no
  information, disagreeing with the engine, which treats it as an answer.
- P3 task: the considerations panel's two not-assessed sentences format
  their input lists differently.
- P3 rec: the forms PDF manifest records only passwordProtected, though the
  committed bytes also say modification, text extraction and assembly are
  blocked while printing and form-filling are permitted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL

* issues: queue the recurring docs/scripts-index.md counts collision (#Y090R5 sibling)

The generated counts sentence at the top of docs/scripts-index.md conflicts on
every concurrent PR — measured five times on PR #2531 in about three hours, at
five different main heads, each time as the sole conflicting line.

Same root cause as #Y090R5 for data/outstanding-issues-snapshot.json: a
single-line generated artefact every PR must regenerate. PR #2530 fixed the
snapshot half by moving regeneration into the serialised reconcile step; this
half is untouched.

Records the two traps found while resolving it: taking main's whole file
silently drops the branch's own new script entry, and a bundled
verify:cheap:internal union merge desynchronises the gate counts that
check:gate-manifest reads.

Append-only inbox request; no canonical ledger edit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL

* issues: cancel the OCR-routing claim and replace it with a corrected record

Codex review finding on this PR, verified against the code it cited rather than
taken on trust. The finding is correct.

Request e3133c1b asserted that disabled text extraction is "exactly the
condition that sends the worker down the OCR fallback path". It is not.
should_ocr_page() in worker/python/extract_pdf_assets.py (line 186) decides on
extracted text length and image coverage ratio only, and never reads the /P
permission bits. The extractor has no needs_pass or authenticate handling at
all, so a user-password PDF fails before any OCR decision is reached.

A wrong causal mechanism in a durable record is worse than no record: the next
person plans ingestion work from it. Cancelled rather than edited, because
inbox requests are immutable by design and the cancel action exists for exactly
this — the audit trail keeps the wrong claim, its refutation, and the
correction.

The permission-bits finding itself stands unchanged and is restated in the
replacement request, which makes no ingestion claim and says plainly that what
happens to these files on ingestion is untested.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL

* issues: record the reproducible mobile /documents/search LCP breach

Two confirmed measurements on different heads of PR #2536 — +466ms (+20.4%)
and +471ms (+20.6%) against a +20%/+100ms tolerance, five milliseconds apart.
That is reproducible, not noise, and the passing re-run between them was the
outlier.

Ruled out as PR #2536's doing: its diff is six medication files, and
documents/search/page.tsx imports one symbol (Metadata from next). Nothing
outside medication-named files calls /api/medications.

Recorded as a hypothesis, not a finding: lighthouse-budget.json was last
refreshed 2026-08-27 and main has taken heavy change since, so the PR carrying
the newest main absorbs the blame. Same disease as #QSHHGK for the bundle
budget. Main itself was never measured, and the record says so.

Needs an owner decision — find the regression, or refresh the baseline on a
schedule rather than reactively to clear a red PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL

* issues: cancel the Lighthouse regression claim and replace it with the measured variance

A third graded run refutes the "reproducible regression" reading in request
d1b3491f. CI run 33684986161 (head 957a038) measured mobile /documents/search
at 2323ms against the 2282ms baseline — +41ms — and reported "Every graded
route is within tolerance of the committed baseline."

The cell has now produced two breaches near 2750ms and at least two passes near
2320ms on the same pinned Chromium and the same route. That is a bimodal
measurement whose two modes straddle the +20%/+100ms tolerance, not a page that
became half a second slower. The gate's 2-of-3 sampling already tolerates noise
within a run; this split is between runs, which that design does not cover.

Cancelled rather than edited, because inbox requests are immutable and the
cancel action exists for this. The replacement states all four outcomes and asks
for the variance to be characterised — repeat the dispatch-only baseline-refresh
job against main and compare the spread — rather than for a regression hunt or a
reactive baseline refresh.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL

* issues: record the measured encryption state of the committed WA MHA form PDFs

A concurrent review comment on this PR asserted that PyMuPDF does not enforce
/P permission bits and that these files therefore "open normally and yield
their text layer". Measured against the committed bytes with PyMuPDF 1.28.0,
the library the worker actually uses, that is false for 50 of the 51 files.

  needs_pass=1, is_encrypted=True, page_count=0
  doc.authenticate("") -> 0        (the empty user password is rejected)
  load_page(0) -> ValueError('document closed or encrypted')

form-12a.pdf is the sole exception: no /Encrypt, opens, 3274 characters of
first-page text — which is exactly the file PR #2531 corrected.

The distinction the comment missed is that /P alone would not block opening;
these files also carry a non-empty /U, and that does. The practical
consequence is that extraction fails at the open call, before
should_ocr_page() is ever reached, so the OCR fallback cannot rescue them.
The JavaScript fallback was not measured and is not claimed either way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL

* issues: record the intermittent caring-contacts-guidance strict-mode violation

Observed once on PR #2536 (CI run 33690849576 attempt 1, Production UI shard 2)
and cleared by a single re-run of the same commit:

  strict mode violation: getByTestId("caring-contacts-guidance")
  resolved to 2 elements

The source renders that test id in exactly one place, and both the page and the
shell interpolate it once, so the duplicate is not a second render site. The
likely window is React relocating out-of-order streamed content from the page's
next/dynamic shell, which the test's waitUntil:"load" does not wait past — but
that is a hypothesis and the record says so; the retained trace should confirm
it before anyone edits the test.

Not PR #2536's: its diff is medication and documentation files only, and the
same shard passed on its previous head.

The record asks for the three unscoped locators to be scoped to the main
landmark, and explicitly rules out quarantining — one reproduction is below the
repository's three-on-the-same-SHA bar, and a locator fix is not a suppression.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0183EiexuZe6uKqoACXGuANL

* docs(ledger): record Run PR sweep review for PR #2544

Merged origin/main into claude/issues-followups (clean, no conflicts)
and verified the narrow gates for this append-only inbox PR; both
review threads were already resolved from a prior pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0137etKe6X2p9HkXsXoYY8TA

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants