[openlibrary-bot]

State: ready-for-review
Updated: 2026-10-10T02:48Z
Owner: pr-13865-lenny-borrow-ux (the provider_tokens/deploy half below was pr-13865-provider-tokens-table)
Adversarial-review: subagent-ab034b0f 2e24cbcd5 PASS — 11 findings, 3 fixed in 2e24cbcd5, the rest listed under Next steps and What this does NOT do

Verified by RUNNING — this revision (borrow-UX, head 2e24cbcd5)

• Every new assertion was watched failing against unfixed source first, and each implementation then survived a mutation battery with every mutant killed by the test written for it: 4 on prime_provider_loans, 5 on the stored-grant reuse, 5 on the popup script, 1 on the unreadable-store guard, 2 on the opener/scheme guards. The reviewer independently ran 10 more mutants against the shipped popup script — 10/10 dead.
• tests/unit/js/provider_popup_result.test.js runs the popup's inline script extracted from the shipped Jinja template, not a copy. This script had no coverage at all before. The reviewer verified the extraction claim rather than reading it: exactly one <script> tag, and src.slice(start,end) === match[1] is true — byte-identical, cannot match nothing (the helper throws) or the wrong block.
• The priming tests exist in two sets with measurably uncorrelated blind spots. Eight use a FakeMemo; four use the shipping memoizer so memcache_set/memcache_get do real JSON encoding. With a set() in the loan, the four go red and the eight stay green.
• Full Python suite, with a control: run with the change and again with it stashed. 20 failures both times, sorted FAILED lists diff empty — all pre-existing. Full JS suite: 64 files, 1263 tests, green.
• Header-injection probe on the new BorrowRedirect, since the mediated path moved from a Jinja-escaped attribute into a Location header. /books/OL123\nM does reach mediated_borrow; Starlette percent-encodes it and web.py's header() raises ValueError("invalid characters in header") by design. The value is always prefixed /borrow/lenny/, so it cannot be an open redirect.
• New tests run 3× each, stable; nothing in them reads a clock except a constant.
• pre-commit green on every changed file, on the host, so generate-pot really ran.
• Re-run at this exact head after the commentary trim: 238 Lenny-specific tests pass; the full pytest openlibrary/ failure set is diff-identical to the stashed baseline (20, all pre-existing, none mine); npx vitest run 64 files / 1263 tests; CI 10/10.

Verified by RUNNING — carried forward (provider_tokens / deploy)

• A borrow completed end to end against a real throwaway node, 4 runs, all PASS; all three --break modes red at the step they name. The Postgres-gated single-flight tests ran against postgres:18.3 (65 passed) and went red three times with FOR UPDATE mutated out. 9-of-9 mutation battery on the token store and schema.sql.

Unverified / READ only

• No browser, and no borrow against lennyforlibraries.org. Everything about the popup is jsdom plus reading: nobody has watched it resize, navigate, or leave the book page showing Read. window.resizeTo has never run outside jsdom. This is the biggest gap and one real borrow closes it.
• The popup is a popup=yes window, so in Chrome it has no address bar — the patron reads a third-party book in a window whose origin they cannot see, and the resize to 1200×1000 is best-effort. Reviewer's finding; worth a product call from Mek rather than a silent default.
• A background loans refresh started just before the borrow can clobber the primed entry and re-stamp it fresh, re-exposing the original "still says Borrow" for up to 60s. Likely a sub-second window in practice. The test suite is structurally blind to it — the real-cache fixture stubs _refresh_in_background, which is the mechanism.
• The reused-grant path lands the patron on the node's sign-in, not the book, whenever their node session has lapsed. No longer "may" and no longer reasoned — measured on ArchiveLabs/lenny 0690a65: redirect_reader (api.py:314) and proxy_readium (api.py:327) take session: Optional[str] = Cookie(None) and make zero get_authenticated_identity calls, and docs/OAUTH2.md:249 says it outright — "Returning a book and reading it still need the patron's login cookie." The OAuth grant does not confer reading. First borrow is fine (they just signed in at the node); a later one on a lapsed cookie is not. This bounds what grant reuse buys and is worth a decision: either the node accepts a bearer token for reading, or reuse saves the consent screen but not the sign-in.
• The deploy steps below have still not been run on testing.openlibrary.org.

Next steps

• Lenny repo: tests/e2e/ol_half.py:216 will now raise. mediated_borrow returns str | None instead of (path, library). Two-line fix: path = offered, and assert the name against lenny.node_display_name(...), where it is resolved now. I enumerated all 10 lenny.* symbols that harness uses; this is the only signature I changed.
• Operator runs the deploy steps below and completes one real borrow — which is also the browser verification this revision lacks.
• Decide on the chromeless popup (above) and on whether state-less GET borrow wants a confirmation step now that the interstitial is gone. Note the latter is not new: the IA path has borrowed on a GET with the same cookie semantics all along.
• REJECTED_TOKEN_STATUSES' docstring describes the loans endpoint; borrow_with_stored_grant is now a second consumer where 403 may mean something else.
• invalidate_provider_loans still has no production caller.
• After step 3 has been run on a host, wire OpenLibraryAccount.anonymize to ProviderToken.delete_all_by_username.
• Lenny borrow: register Lenny as a book provider so the existing button offers it #13686 (per-node provider names) is still open.

Blocked on

• Nothing. Mek for merge.

Composes the six open Lenny drafts onto current master and gives the patron's
grant a durable home: the provider_tokens table from #13689, with the
CREATE TABLE in openlibrary/core/schema.sql so a fresh install gets it.

Closes nothing on its own — this is the demo vehicle for #12844.

What changed since the last revision

The previous revision stored the grant in an encrypted cookie, because a
database was explicitly off the table for the first demo. That constraint is
lifted, so:

1. Lenny borrow: encrypted per-provider OAuth token storage (#13685) #13689's table-backed store is back, recovered rather than rewritten —
   encryption, rotation under a row lock, and its ~450-line test suite.
2. CREATE TABLE provider_tokens is in openlibrary/core/schema.sql, taken
   from the POSTGRES_DDL constant that has actually been executed.
3. The borrow was exercised for real, against a live (throwaway) node, four
   times, plus all three deliberate-failure modes.

The seam is kept and both backends ship. DbTokenStore is the default;
CookieTokenStore stays as a demo/test backend for a database that has not had
the DDL applied. TokenStore now carries the whole operation set rather than a
coarse load/save pair, because get_fresh is the one thing the two backends
do differently — the table single-flights a rotating refresh token with
SELECT … FOR UPDATE on the patron's row, and expressing that through
load/save would have imposed the cookie's limitation on the table.
ProviderToken is now a facade holding no storage logic of its own.

TestWhatTheCookieCannotDo is re-pointed, not deleted. It is still a
characterisation test of the cookie's single-flight race, but it now installs
CookieTokenStore by hand and says in its name and docstring that it is a
statement about a non-default backend. What the shipped default does is
TestSingleFlightUnderConcurrency, which needs a real Postgres.

The borrow flow after Mek's live walk (2026-10-09)

He borrowed "Catboy" (OL26947526M) on testing.openlibrary.org against the
real lennyforlibraries.org. The loan worked end to end — created at the
node, grant stored in provider_tokens, loan visible on the book page after a
refresh. Everything around it did not. His four reports, and what each turned
out to be:

1. "A long interstitial page when I clicked borrow." The five-second
countdown on /books/<olid>/-/borrow?action=borrow. Mek's follow-up was to
remove it for Lenny rather than skip it conditionally, and that is what this
does: handle_borrow_async now returns BorrowRedirect straight to
lenny_borrow. The screen exists to warn a patron that the next page belongs
to somebody else, and its mediated wording said in as many words that this was
not happening — so it was five seconds spent denying the hazard it was built to
announce. The handoff case keeps it: a node Open Library holds no
credentials for still gets the interstitial, because that one really does leave
the site. The now-unreachable borrowing branch and its three translatable
strings are deleted rather than left in place, and mediated_borrow stops
returning a library name nothing reads.

2 and 3. "Shows 'Borrowed from Lenny' but did not bring me to the book…
clicking borrow again retriggers the flow. When I refresh, it then shows
read." Not popup blocking and not a postMessage failure — this was the
first hypothesis and it is wrong. The flash is written by render_borrowed and
rendered only by the site layout, which the popup deliberately skips, so seeing
"Borrowed from Lenny" on the book page is the opener having reloaded. The
channel worked.

What reloaded was a page re-rendering from the provider_loans memcache entry
written before the borrow, because nothing in the borrow flow had ever told
that cache anything. The timeline fits exactly, including why the second load
worked: _cached_loans found a stale pre-borrow entry, returned [] (so:
Borrow) and started a background refresh; that refresh wrote the real set;
the manual refresh a moment later read it (so: Read).

invalidate_provider_loans was written for precisely this and documented as
"the borrow flow should call this once a borrow succeeds". It has exactly one
call site in the tree and it is its own unit test. Calling it would not have
fixed this either — a cold entry answers "no loans" and only starts the
fetch, by design, so the CTA would still have said Borrow on the reload. The
new prime_provider_loans writes the loan instead, stamped stale so the node
is still asked behind it. test_invalidating_alone_still_leaves_the_page_saying_borrow
pins the trap and is green before and after.

Screenshots: none, and that is a real gap. No browser was available to me,
so there is nothing to show and nothing has been seen rendered. Every claim
about the popup comes from the jsdom harness over the shipped script plus
reading. The consent screens in Mek's screenshots are drawn by the node and are
not produced by this diff at all.

4. "I'd like the book to open up in Lenny in a new window." Three places
render a Lenny reader link: the book page CTA, the loans page, and the popup's
result page. (The list embed's read_url is not a fourth — get_ebook_info is
gated on ocaid and a provider loan has none.) The book page CTA already had
target="_blank"; it is pinned by a test rather than changed. The other two
did not and now do.

And the popup ends in the book rather than on a receipt. Its last page
posts to the opener, then navigates itself to the node's reader. A navigation
rather than window.open, because by the time the node has mailed a one-time
code and the patron has typed it the transient activation from the Borrow click
is long gone and a popup opened there would be blocked. Since it is the same
window either way, the target="_blank" fallback for a blocked popup now
reaches the reader too — which is the "degrade better" half of item 2.
provider_borrow_popup.js stops closing that window; the two sides have to
agree or the reader is killed as it loads.

What "ends in the book" does and does not mean. The node's reader is
cookie-authenticated, not token-authenticated — redirect_reader
(api.py:314) and proxy_readium (api.py:327) on ArchiveLabs/lenny
0690a65 take session: Optional[str] = Cookie(None) and make zero
get_authenticated_identity calls, and docs/OAUTH2.md:249 says it outright:
"Returning a book and reading it still need the patron's login cookie." So on
a first borrow the patron reads the book, because they signed in at the node
moments earlier. On a reused-grant borrow with a lapsed node session they
land on the node's sign-in instead. The grant does not confer reading, and that
bounds what grant reuse buys — see the status block.

The grant in provider_tokens is finally used for borrowing. lenny_borrow
read no token at all: every click ran a full OAuth round trip for a credential
already in the table, which was then overwritten with an equivalent one. It now
borrows with the stored grant, and falls back to the handshake when there is
none, when the node rejects it (401/403 — a revoked grant stays locally
unexpired forever, so a refused borrow is the only place that surfaces), or
when the node does not answer. A refusal the handshake cannot fix — "every copy
is out", "you have too many books out" — is shown instead, because
re-authorizing would mail the patron a code and reach the same answer.

Deploying to testing.openlibrary.org

schema.sql is applied at database init only, and Open Library has no
DDL-migration mechanism — every script under scripts/migrations/ migrates
data. Testing has an existing database and will never re-run schema.sql, so
step 3 is not optional and not a footnote: without it every borrow raises on
a missing relation.

1. Register Open Library as an OAuth client, on the node

On the Lenny node (lennyforlibraries.org), as its operator:

make ol-connect URI=https://testing.openlibrary.org/borrow/lenny/callback

(which is docker compose -p lenny exec api python3 scripts/oauth2_client.py ol-connect --redirect-uri …).
It prints client_id and client_secret; the secret is printed once. The
URI= is required — the script's default is https://openlibrary.org/..., and
the redirect URI is matched byte-for-byte, including scheme, host and path.

2. Put the credentials in config — never in the repo

On the testing host, in the olsystem checkout that sits beside the
openlibrary one (compose mounts ../olsystem → /olsystem and sets
OL_CONFIG=/olsystem/etc/openlibrary.yml), add to etc/openlibrary.yml:

lenny_redirect_uri: https://testing.openlibrary.org/borrow/lenny/callback
lenny_nodes:
  lenny:
    name: Lenny for Libraries
    issuer: https://lennyforlibraries.org
    client_id: <from step 1>
    client_secret: <from step 1>

Three things about this block:

• The key under lenny_nodes is the provider_name the harvest wrote into
  acquisitions, which today is the literal lenny for every node (Lenny borrow: register Lenny as a book provider so the existing button offers it #13686).
• lenny_redirect_uri must be byte-identical to the URI= from step 1.
• issuer is used to build the loans URL, while the token endpoint comes
  from discovery. A mismatch gives working loans and broken refresh — and a
  failed refresh deletes the grant. The e2e refuses to proceed when the two
  disagree; that is what --break advertised-issuer exercises.

openlibrary's own conf/openlibrary.yml is development-only and carries this
same block commented out.

3. Create the table by hand, on that host's database

CREATE TABLE provider_tokens (
    id serial primary key,
    username text not null,
    provider_name text not null,
    access_token text not null,
    refresh_token text default null,
    expires timestamp without time zone default null,
    scope text not null default '',
    created timestamp without time zone default (current_timestamp at time zone 'utc'),
    updated timestamp without time zone default (current_timestamp at time zone 'utc'),
    UNIQUE (username, provider_name)
);

UNIQUE (username, provider_name) is load-bearing, not decoration:
ProviderToken.upsert is an ON CONFLICT (username, provider_name) DO UPDATE,
which Postgres rejects outright without that index — so the first repeat
borrow by any patron would be a 500. Confirm with:

\d provider_tokens

This is byte-identical to schema.sql and to the POSTGRES_DDL constant in
openlibrary/tests/core/test_provider_tokens.py; a test asserts all of that on
every run.

4. Make sure there is something to borrow

The Borrow button comes from an acquisitions row, not from
identifiers.lenny. For /books/OL59176589M ("The Quintet", which carries
identifiers: {'lenny': ['59176589']} on production):

INSERT INTO acquisitions (work_id, edition_id, provider_name, local_id, data)
VALUES (<work OLID numeric>, 59176589, 'lenny', '59176589',
        '{"access": "borrowable"}'::jsonb);

• edition_id and local_id are OLID numerics (59176589), not thing.id.
• provider_name is the literal lenny, and local_id must equal the value in
  identifiers.lenny — that is the key find_many looks the row up by.
• The data blob must contain an access key or the button renders nothing.

5. Deploy the branch and restart

Normal testing deploy of 13552/lenny-oauth-demo, then restart web so the
config is re-read.

6. Walk it

Sign in, open the book page, click Borrow. The patron goes to the node, signs
in there, consents, and lands back on
https://testing.openlibrary.org/borrow/lenny/callback; the loan should then
appear on /account/loans.

Testing is https, so the ptok cookie's Secure flag is honoured there.
That flag only ever caused trouble on plain-http localhost, where an
unconditional secure=True made the browser drop the cookie silently — a borrow
that appears to succeed and a loans page that stays empty. It is now keyed on the
request scheme. (With the table as the default store the cookie is not used for
the grant at all; this note is for anyone selecting the cookie backend.)

A caveat for the local walkthrough, not for testing

On production /books/OL59176589M also carries an ocaid and a providers
GitHub link, and both outrank Lenny in PROVIDER_ORDER — LennyProvider sits
deliberately below InternetArchiveProvider, so an edition that already
renders an IA button keeps rendering it. A local copydocs walkthrough
therefore still needs those two fields removed by hand to make the Borrow button
appear. Do not "fix" the provider ordering here — that is deliberately
deferred.

What this does NOT do

• It does not borrow from lennyforlibraries.org. Every borrow in this PR's
  evidence happened against a throwaway node the harness created and destroyed.
  A completed borrow on the live node creates a real loan against real lending
  limits on a real library, and is the operator's call to make once.
• It does not run the web app under test, and nobody has opened a browser.
  The e2e exercises the functions behind the pages, not the pages — no request
  context, no templates, no infogami app. The popup's script now has a jsdom
  harness that runs the real thing (tests/unit/js/provider_popup_result.test.js),
  which is a long way from nothing, but no human has watched the popup resize,
  navigate to the reader, or leave the book page showing Read. window.resizeTo
  has never run outside jsdom. The COOP dependency is still unverified, and no
  a11y tooling exists on origin/master for page/template changes.
• It does not update Lenny's e2e harness. mediated_borrow now returns
  str | None rather than (path, library), so tests/e2e/ol_half.py:216's
  path, library = offered will raise. That is a two-line fix in the Lenny
  repo and it is listed as a next step rather than done here.
• It does not make the reused-grant path guarantee a reader session. The
  node's reader wants the node's own browser session, which Open Library's
  stored grant outlives; a patron borrowing with a reused grant may be asked to
  sign in at the node before the book opens. Same as opening last week's loan.
  Noted in read_url rather than solved.
• It does not migrate an existing database. It cannot: there is no mechanism.
  Step 3 is a human running one statement, and that is stated here rather than
  discovered on deploy.
• It does not wire anonymize. ProviderToken.delete_all_by_username exists
  and is tested, but OpenLibraryAccount.anonymize is not changed to call it,
  deliberately: adding DDL to schema.sql does not put the table in a database
  that already exists, so wiring it today would make account deletion raise on a
  missing relation in production. One line, once a host has run step 3.
• It does not fix Lenny borrow: register Lenny as a book provider so the existing button offers it #13686 (per-node provider names). The button still resolves
  acquisitions under the literal lenny.
• It does not run the concurrency tests in CI. They need a real Postgres, so
  they skip — a green check on this PR does not include them. They were run here
  against postgres:18.3, and the command is in the class docstring.

Why the two DDL copies are compared rather than deduplicated

POSTGRES_DDL in the test file has to stay a literal constant: Lenny's
tests/e2e/ol_half.py parses it out by AST and executes it. schema.sql is
where a fresh install gets the table. Two copies of a DDL drift silently and in
the worst direction — the tests keep passing against the shape they created
themselves, while a fresh install gets a different table. That is the
fixture-only-table shape that let 109 green tests sit around a table no
deployment had ever had, and it died on the last hop. TestTheShippedSchemaHasTheTable
compares them, whitespace-insensitively, on every run, with a separate named
assertion for the UNIQUE constraint so a failure says which property went.

Composed from

#13600 (borrow route and callback) · #13689 (token storage — now restored in
full, table and all) · #13690 (Lenny as a book provider) · #13691 (loans merge,
incl. the timezone fix) · #13693 (popup) · #13768 (unservable-node logging) ·
#13706 (Read-not-Borrow CTA, by @iamtanmaybaranwal)

https://claude.ai/code/session_01UViYm1nKqJkiKJWJUr1eas