laoc/stash-bookmark
Watch
1
0
Fork
You've already forked stash-bookmark
 0
Code Issues Pull requests 1 Projects Releases Packages Wiki Activity

File a bookmark in a list from the edit sheet, with one signature not two #13

Closed
laoc wants to merge 0 commits from fix/nip51-lists-012 into main
pull from: fix/nip51-lists-012
merge into: laoc:main
Conversation 1 Commits - Files changed -
laoc commented 2026-08-06 12:21:32 +00:00
Owner
Copy link

Closes the NIP-51 report end to end, and adds the Update path — the branch laoc's real-Amber test actually took, which had no coverage at all.

What was broken

Every existing list test launches a share intent for a URL that is not bookmarked yet, so startSaving finds existing == null and the draft never runs with isUpdate = true. laoc, testing with Amber:

when i edit an existing bookmark and want to add it to a list it first asks me to sign the bookmark again (why? did not change anything), then i get the list event to sign ... and also the first signing request does not make sense.

Two causes, both on that branch:

  1. saveDraft signed a kind:39701 unconditionally. It is addressable, so re-publishing identical content replaces the event with itself: no gain, and it costs the one thing a user cannot ignore. Now skipped when the tags and content that would be signed match what is stored. The list sync still runs — ticking a chip changes the list event, never the bookmark — so it moved into fileInLists, shared by both save paths.

  2. published_at was minted as now on an update when the stored event carried none. This is what made cause 1 insufficient: the "identical" check could never be true. NIP-B0 defines the field as "the first time the bookmark was published", and now is provably not that for an event that already exists — so this was inventing data as well as raising a prompt for it. Measured on laoc's own relays (haven.laoc.xyz + nos.lol): 6 of their 8 bookmarks carry no published_at, so this was the common case, not an edge.

What it also carries

Everything already reported in #NIP-51 support and on the ngit issues, rebased onto main (which now has #6–#11):

  • the Lists tab never learning about a save — 543b520 dropped the post-save refresh() and, unlike createList/editList/deleteList in that same commit, put nothing back
  • the uniffi process abort on NIP-42 (Can't lift flat errors — native SIGABRT on any bridge throw, including a plain user decline)
  • signer requests queueing instead of a concurrent one getting RESULT_CANCELED and being misread as a rejected signature
  • kind 22242 pre-authorised at login, so an auth challenge never takes the Activity-intent path

Verification

At 6a26aac, the head of this PR:
on-device 96/96, 0 failed, complete run (started == expected)
JVM 393/393, 0 failed
mutations 4/4 landed, 4/4 caught

Mutations run against the two production changes, each asserted to have landed (git diff --numstat) before being measured:

mutation caught by
remove the skip entirely adding_an_unchanged_bookmark_to_a_list_asks_for_one_signature_not_two
put back the invented published_at that test and editing_a_bookmark_that_never_carried_a_published_at_does_not_invent_one
skip drops the list sync too a_bookmark_added_to_a_list_from_the_edit_sheet_shows_without_a_manual_refresh + the signature test
always skip, even for a real edit editing_a_bookmark_that_never_carried_a_published_at_does_not_invent_one

Three new tests drive the real gesture — pencil, tick, Update. They open the sheet only after the list fetch has landed, because startSaving reads the pre-ticked set once and an empty one would make Update un-file the bookmark from every list it was already in. The signature count is read from the stand-in signer's own record of the payloads it was handed, so a prompt counts whichever code path raised it.

Two things this does NOT settle

  • "it does not show up there till i hit the refresh button", on the Update path. The test for it passed before any fix, so my harness cannot produce that symptom. The likeliest reason is measured and it is my fault: stash-0.12.0-bunker-relogin-debug.apk (no list fix) and stash-0.12.0-nip42-abort-debug.apk (has it) both shipped as versionCode 19 with different bytes and the same version string, so nothing on screen could tell them apart. This branch is versionCode 22. Awaiting laoc's confirmation of what they were running.
  • "i tested with bunker login ... i got no signing request." Untouched here, and deliberately not guessed at — a NIP-46 signer often auto-approves after pairing, so that can mean signed silently and worked or nothing was ever sent. Will be its own issue once laoc says whether the bookmark reached the list.

Real-Amber confirmation on a real phone is still outstanding, as with #6. The emulator signs with a stand-in.

Closes the NIP-51 report end to end, and adds the Update path — the branch laoc's real-Amber test actually took, which had no coverage at all. ## What was broken Every existing list test launches a share intent for a URL that is *not* bookmarked yet, so `startSaving` finds `existing == null` and the draft never runs with `isUpdate = true`. laoc, testing with Amber: > when i edit an existing bookmark and want to add it to a list it first asks me to sign the bookmark again (why? did not change anything), then i get the list event to sign ... and also the first signing request does not make sense. Two causes, both on that branch: 1. **`saveDraft` signed a kind:39701 unconditionally.** It is addressable, so re-publishing identical content replaces the event with itself: no gain, and it costs the one thing a user cannot ignore. Now skipped when the tags and content that would be signed match what is stored. The list sync still runs — ticking a chip changes the *list* event, never the bookmark — so it moved into `fileInLists`, shared by both save paths. 2. **`published_at` was minted as `now` on an update when the stored event carried none.** This is what made cause 1 insufficient: the "identical" check could never be true. NIP-B0 defines the field as "the first time the bookmark was published", and `now` is provably not that for an event that already exists — so this was inventing data as well as raising a prompt for it. Measured on laoc's own relays (haven.laoc.xyz + nos.lol): **6 of their 8 bookmarks carry no `published_at`**, so this was the common case, not an edge. ## What it also carries Everything already reported in #NIP-51 support and on the ngit issues, rebased onto `main` (which now has #6–#11): - the Lists tab never learning about a save — `543b520` dropped the post-save `refresh()` and, unlike `createList`/`editList`/`deleteList` in that same commit, put nothing back - the uniffi **process abort** on NIP-42 (`Can't lift flat errors` — native SIGABRT on any bridge throw, including a plain user decline) - signer requests **queueing** instead of a concurrent one getting `RESULT_CANCELED` and being misread as a rejected signature - kind **22242** pre-authorised at login, so an auth challenge never takes the Activity-intent path ## Verification At `6a26aac`, the head of this PR: | | | |---|---| | on-device | **96/96**, 0 failed, complete run (`started == expected`) | | JVM | **393/393**, 0 failed | | mutations | **4/4 landed, 4/4 caught** | Mutations run against the two production changes, each asserted to have landed (`git diff --numstat`) before being measured: | mutation | caught by | |---|---| | remove the skip entirely | `adding_an_unchanged_bookmark_to_a_list_asks_for_one_signature_not_two` | | put back the invented `published_at` | that test **and** `editing_a_bookmark_that_never_carried_a_published_at_does_not_invent_one` | | skip drops the list sync too | `a_bookmark_added_to_a_list_from_the_edit_sheet_shows_without_a_manual_refresh` + the signature test | | always skip, even for a real edit | `editing_a_bookmark_that_never_carried_a_published_at_does_not_invent_one` | Three new tests drive the real gesture — pencil, tick, Update. They open the sheet only *after* the list fetch has landed, because `startSaving` reads the pre-ticked set once and an empty one would make Update un-file the bookmark from every list it was already in. The signature count is read from the stand-in signer's own record of the payloads it was handed, so a prompt counts whichever code path raised it. ## Two things this does NOT settle - **"it does not show up there till i hit the refresh button", on the Update path.** The test for it passed *before* any fix, so my harness cannot produce that symptom. The likeliest reason is measured and it is my fault: `stash-0.12.0-bunker-relogin-debug.apk` (no list fix) and `stash-0.12.0-nip42-abort-debug.apk` (has it) **both shipped as versionCode 19** with different bytes and the same version string, so nothing on screen could tell them apart. This branch is versionCode 22. Awaiting laoc's confirmation of what they were running. - **"i tested with bunker login ... i got no signing request."** Untouched here, and deliberately not guessed at — a NIP-46 signer often auto-approves after pairing, so that can mean *signed silently and worked* or *nothing was ever sent*. Will be its own issue once laoc says whether the bookmark reached the list. Real-Amber confirmation on a real phone is still outstanding, as with #6. The emulator signs with a stand-in. 
RelayRoundTripTest.a_bookmark_joins_a_nip_51_list_and_the_membership_
survives_the_relay is named for this and cannot fail on it: it builds the
list with withItem() and publishes it through the repository directly, so
the save sheet, the draft's selection and syncListMembership are all
absent from the run. Everything between the chip and the relay was
untested, which is where laoc's report lives.

Three tests, driving a real ACTION_SEND share the way ShareIntentTest
does: tick an existing set, tick the global list that does not exist yet,
and check the Lists tab without a manual refresh in between. The first
two ask the relay; the third asks the screen, because a membership that
reached the relay and never reached the Lists tab is still, to the person
using it, a bookmark that did not go in the list.

No production change here on purpose. These have to be red against the
shipped code before any fix is worth believing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every other list test points at one plain relay and they all pass, on
main and here, while laoc's list events never reach any relay. This one
adds nak serve --auth --eager-auth to prefs.appRelays and does an
ordinary save. It does not fail — the process aborts: uniffi 'Can't lift
flat errors', SIGABRT on a rust-nostr worker, because SdkSignerBridge
throws across the CustomNostrSigner callback boundary.

Seeding and assertions stay on the plain relay so an auth failure on the
second one cannot confound the read.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SdkSignerBridge is a uniffi callback interface: the SDK calls it from
Rust, on its own worker. An exception thrown out of one of these does not
reach Rust as an error — uniffi cannot lift it and calls abort(). The
process dies with a native SIGABRT and "Can't lift flat errors", no Java
stack, nothing above able to catch it.

Measured, not theorised: with an auth-demanding relay in the working set,
an ordinary save killed the app twice, once in isolation. A plain user
decline was enough. haven.laoc.xyz demands NIP-42, so this was live on
the relay laoc uses most.

getPublicKey and signEvent now return null on failure, which is the
channel the SDK expects (its own error text is "Received None instead of
event"), and guard() catches Throwable — not Exception — because the cost
of one escaping is the whole process.

The four encryption methods still throw, and that is a constraint rather
than a choice: the SDK declares those NON-null, so no decline channel
exists. The compiler is the evidence — widening them to String? fails
where the same widening on the other two compiles. Returning a sentinel
string would be worse than the abort, handing back garbage where
ciphertext is expected.

Verified: same test, same relay, no abort, run completes. It now surfaces
the two failures the crash was hiding, both logged rather than fatal —
the launcher refusing a concurrent request (resultCode=0), and
ActivityResultSignerLauncher.launch itself throwing. Neither is fixed
here; this commit only stops them being fatal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects the uniffi abort was hiding, both of which dropped the user's
list membership once the crash stopped masking them.

The launcher answered a second concurrent request with RESULT_CANCELED,
which every caller reads as "the user declined". The SDK answers NIP-42
challenges on its own worker thread, so an auth signature and the user's
own save genuinely collide; the loser was dropped silently. Requests now
take turns on a mutex.

No timeout on that queue, deliberately. A late result arriving after one
would be paired with the NEXT request, which is the mis-pairing the
single-slot design existed to prevent, and the Activity Result API gives
no request id to tell them apart. Instead attach() releases any
outstanding request: the one way a reply never arrives is the Activity
dying, and that is observable exactly when a new one registers.

launch() itself is now wrapped, because it throws — measured, from an
Activity no longer in a valid state — and on the auth path that call sits
inside SdkSignerBridge's uniffi callback, where a throw aborts the
process rather than failing the call.

And kind 22242 joins the pre-authorised set at login, so a challenge is
answered through the signer's remembered-permission path instead of a
full intent that then has to queue behind a save at all.

Verified on the auth-demanding relay: OK (1 test), zero aborts, four
declines down to one, 35s to 6.2s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
543b520 moved list membership behind the save and dropped the refresh()
that used to follow it. Every other list mutation in that commit got a
hand-rolled state update in exchange — createList gained upsertList,
editList and deleteList gained theirs. syncListMembership got nothing, so
the version it published never reached UiState.lists, and
show(Screen.Lists) re-reads nothing. The bookmark was on the relay and
absent from the Lists tab, and from the list chips on its own row, until
the user found the refresh button.

Signing and publishing are split rather than going through
signAndPublish, so the stamp is the event's own created_at. upsertList
refuses a version older than the one it holds and supersedes can push a
list past wall-clock now, so a guessed timestamp would be dropped exactly
in the case that made the guess necessary.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
publishListVersion splits sign from publish so the state update carries
the event's own created_at rather than wall-clock now. Nothing tested
that, and it is exactly the kind of thing that reads as an over-
complication and gets simplified back into a bug.

It cannot be pinned by the ordinary case: for a list stamped a few
seconds ago, `now` is newer than the version on screen, upsertList
accepts it and every existing test stays green with the defence removed.
A list dated ahead of now is republished at that date + 1, so a wall-
clock stamp is older than what state already holds and upsertList drops
it — the membership reaches the relay and never reaches the screen.

Same seeding trick as an_edit_outranks_the_version_it_read: seeding ahead
turns a race that would reproduce about half the time into a certainty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
publishListVersion came across from the main-based branch typed to
AmberSigner; syncListMembership on this stack takes the NostrSigner
interface, so bunker and local-key logins reach it too.
laoc, testing with Amber: "when i edit an existing bookmark and want to
add it to a list it first asks me to sign the bookmark again (why? did
not change anything) ... the first signing request does not make sense."

Two causes, both on the update path, which had no coverage at all — every
existing list test launches a share intent for a URL that is not
bookmarked yet, so `startSaving` finds `existing == null` and the draft
never runs with `isUpdate = true`.

1. `saveDraft` signed a kind:39701 unconditionally. It is addressable, so
   re-publishing identical content replaces the event with itself: no
   gain, and it costs the one thing a user cannot ignore. Skipped when
   the tags and content that would be signed match what is already
   stored. The list sync still runs — ticking a chip changes the LIST
   event, never the bookmark — so it moved into `fileInLists`, shared by
   both paths.

2. On an update, `published_at` was minted as `now` when the stored event
   carried none. NIP-B0 defines it as "the first time the bookmark was
   published", and `now` is provably not that for an event that already
   exists; inventing it also made every such update a genuinely different
   event, so the skip above could never fire. Six of laoc's eight
   bookmarks carry no published_at (measured on haven.laoc.xyz and
   nos.lol), so this was the common case, not an edge.

Three tests drive the real gesture — pencil, tick, Update — and open the
sheet only after the list fetch has landed, because `startSaving` reads
the pre-ticked set once and an empty one would make Update un-file the
bookmark from every list it was in.

The signature count is read from the stand-in signer's own record of the
payloads it was handed, so a prompt counts whichever code path raised it.
21 is already on disk as the client-tag debug APK. Two different builds
have shared a versionCode once already this week — bunker-relogin and
nip42-abort both went out as 19, with different bytes and the same
version string, so nothing on screen could tell them apart and it is
still unsettled which of the two laoc tested.
laoc commented 2026-08-06 13:04:37 +00:00
Author
Owner
Copy link

Landed on main via ngit (nostr is now the primary repo) — commit 924aa2b already on this branch. Closing rather than fighting the merge API for a cosmetic ribbon on what laoc has since called a mirror-only copy.

Landed on main via ngit (nostr is now the primary repo) — commit 924aa2b already on this branch. Closing rather than fighting the merge API for a cosmetic ribbon on what laoc has since called a mirror-only copy.
laoc closed this pull request 2026-08-06 13:04:37 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
laoc/stash-bookmark!13
No description provided.