Skip to content

feat: the Linux target — a jvm() target on shared, and loopky, the headless client - #208

Merged
jvsena42 merged 27 commits into
mainfrom
feat/linux-target-cli
Sep 3, 2026
Merged

feat: the Linux target — a jvm() target on shared, and loopky, the headless client#208
jvsena42 merged 27 commits into
mainfrom
feat/linux-target-cli

Conversation

@jvsena42

@jvsena42 jvsena42 commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Closes part of #54: the Linux x86_64 row, plus everything cross-platform it needs.

Fork side: jvsena42/pubky-core-ffi-fork#4 (the desktop cdylibs). Its artifacts are vendored here, so this branch builds without it.

What this is

loopky — a terminal binary that creates and manages decks against the same homeserver layout the two apps use. The point is not a nicer keyboard UX: it is that an agent can drive Loopky. Agents already write good flashcards; until now the only way into a Loopky deck was a phone screen.

Six commits, each standing alone.

1. :shared grows a jvm() target

Most of what a desktop JVM needs was already written — it was just filed under androidMain by accident of Android having been first. applyDefaultHierarchyTemplate grows a jvmShared group and the plain-JVM code moves into it: the java.time/java.security actuals, the zip helpers, HttpURLConnection, and the UniFFI JNA bindings.

Sharing rather than copying is the point. uniffi/pubkycore/pubkycore.kt is generated in the fork and checked in here; a second copy under jvmMain would have to stay byte-identical forever with nothing reporting it when it stopped.

The group is added through the template rather than with bare dependsOn edges — those switch the default template off, and the only sign is a warning saying iosMain no longer belongs to a compilation.

kvault moves out of commonMain (it publishes android + iOS artifacts and nothing else). The .apkg reader splits the same way: everything but opening the collection becomes JvmApkgReader behind an AnkiDbOpener, so Android keeps platform SQLite and the desktop takes sqlite-jdbc.

2–3. The native library, and the platform module

libpubkycore ships inside the jar under JNA's resource layout, so nobody installing the CLI fetches a native library by hand. UniffiPubkyClientJvmTest is the part with teeth: the 1,271 shared tests run against a fake client and would pass identically on a machine where that directory is empty or the architecture is wrong — a failure that otherwise surfaces hours later as an ordinary-looking transport error.

The seven stores go to 0600 JSON files under $XDG_CONFIG_HOME/loopky. The file is the default, not a fallback, and that is the decision: libsecret is usually present on a desktop Linux and usually absent on the headless box an agent runs on, so a keyring default would fail exactly where the tool is meant to work.

4–6. The CLI

Three properties everything is arranged around.

The session scope is /pub/loopky/:rw and nothing else — not the apps' DEFAULT_CAPABILITIES. An agent session cannot write a post, a follow or a profile edit under any bug or any prompt injection, because it was never handed the capability. The announce confirmation (#39) is gone by construction, not behind a flag: there is nothing to confirm. It costs less than it looks, because deck tagging routes on the subject and a deck manifest's tag record lives in /pub/loopky/tags/.

That scope also turned up a real bug in shared code: selfTagAsLoopkyUser tags a profile, so its record goes to /pub/pubky.app/tags/ — and it fired on every session load, from a session that was never granted the namespace. Best-effort, so nothing broke; it just bought a doomed round trip per command and logged a warning about the scope working as designed. Session.canWritePubkyApp now asks first.

Nothing prompts, and stdout is a machine channel. --json is a versioned envelope carrying the result and the failure, with the environment and the indexer on every one — because Nexus answers a query aimed at the wrong network successfully and empty, and an agent that reads [] concludes its write failed and retries. Exit codes distinguish not-signed-in from session-expired from network; expiry gets its own because it is hourly, unrecoverable without a human (#165), and otherwise indistinguishable from a wobble.

Long jobs are resumable and writes are idempotent. card add twice with the same sides is detectable and skipped; import --resume checkpoints against the deck on the homeserver rather than a local cursor, because the deck records exactly which cards arrived and survives the sandbox being thrown away. Batch --from-file for add and edit, since editing is what an agent does after an import.

The import format carries image URLs from day one — front <TAB> back <TAB> front_image_url <TAB> back_image_url — because a remote ref costs no bytes and no quota since #167, and it is the one thing neither .apkg nor TSV-through-the-parser can express.

Verified

On Linux x86_64, in CI — the branch's biggest unknown, since the .so was cross-built from macOS in a container and nothing had loaded it on a real glibc host:

linux-x86-64/libpubkycore.so loads through JNA from the jar :shared:jvmTest green on ubuntu-latest, and UniffiPubkyClientJvmTest is in it
Built binary, exit codes and envelope loopky 0.1.0 (schema 1); whoami --json → exit 3 with "code":"not_signed_in" and "environment":"production"
A headless box with no desktop session and no libsecret ✅ which is what a runner is, and the reason the file store is the default

On macOS arm64, by hand, against the live production and staging networks:

Shared suite on the desktop target 1,271 tests, 0 failures
All targets :shared:allTests (Android + JVM + iOS sim), :composeApp:assembleDebug, detektAll
login mints a real auth URL caps=/pub/loopky/:rw, no x-success/x-cancel callbacks
Terminal QR + --qr-out + --url-only
Nexus reads ✅ production and staging return different lists — the environment really does route the indexer
Dead LOOPKY_SESSION ✅ exit 4, with the right advice ("mint a new one with login --export", not "run login")
CLI unit tests ✅ 29

Full table in journeys/RESULTS.md.

Two bugs the hand-run caught that a green build did not, both now covered by tests: --version printed the usage block, and import cards.tsv dispatched under the verb "import cards.tsv" — the verb was "the first two words", which is right for deck create and wrong the moment a command takes an operand.

Not verified — this is the part that needs your Linux box

No Pubky Ring approval was completed, because a QR needs a phone next to the machine. So everything above the sign-in line — deck create, import, card add/edit/rm, deck sync/compact, whoami against a real session — is untested end to end, and these acceptance criteria are unmet until someone scans the code:

  • a deck the Android app opens and studies without a repair step
  • an import killed mid-run and resumed without duplicating a card
  • a full run leaving no record under /pub/pubky.app/, verified by reading it afterwards

To try it there:

./gradlew :cli:installDist
cli/build/install/loopky/bin/loopky login      # scan with Ring
cli/build/install/loopky/bin/loopky whoami --json

Known gaps, named as decisions

  • Packaging. What ships is a jar plus a start script needing a JRE 17 — exactly what a one-line sandbox install cannot be. GraalVM native-image is the target (JNA's library extraction is the sharp edge), jlink the fallback, plus a container image. The tarball also carries both native rows, so a Linux box hauls 11 MB of macOS dylib.
  • macOS Keychain. That row uses the same 0600 file today; the Keychain is the right answer there and is that row's remaining work.
  • Allowlist proxies. Unresolved and a hard blocker for cloud sandboxes: the homeserver is not a fixed hostname (pkarr-resolved, pkarr-derived cert) and pkarr's DHT path is UDP. Architecture.md §13.12.
  • Windows. Out of scope for v1 by decision.

🤖 Generated with Claude Code

https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr

jvsena42 and others added 9 commits September 2, 2026 18:41
The headless client (#54) needs `shared` to build for a desktop JVM. Most of what
it needs was already written — it was just filed under `androidMain` by accident
of Android having been first.

`applyDefaultHierarchyTemplate` grows a `jvmShared` group so Android and the new
`jvm()` target share one source set, and the code that is plain JVM moves into
it: the `java.time` and `java.security` actuals, the `java.util.zip` helpers,
`HttpURLConnection`, and the UniFFI-generated JNA bindings. That last one is the
reason to share rather than copy — `uniffi/pubkycore/pubkycore.kt` is generated
in the fork and checked in here, and a second copy under `jvmMain` would have to
stay byte-identical forever with nothing reporting it when it stopped.
`AndroidPubkyClient` and `AndroidHttpFetcher` are renamed to `UniffiPubkyClient`
and `JvmHttpFetcher` to match what they now are.

The group is added *through* the template rather than with bare `dependsOn`
edges: those switch the default template off, and the only sign is a warning
saying `iosMain` no longer belongs to a compilation.

`kvault` moves out of `commonMain` into `androidMain`/`iosMain`. It publishes
android + iOS artifacts and nothing else, so a `commonMain` declaration fails to
resolve for `jvm()` for a reason that has nothing to do with the code; the one
`commonMain` reference to it was a doc comment.

The `.apkg` reader splits the same way. Everything but opening the collection is
now `JvmApkgReader`, behind an `AnkiDbOpener` — Android keeps platform SQLite,
the desktop target takes `org.xerial:sqlite-jdbc`. Bulk Anki import is the most
CLI-shaped job there is (#46), so this is on the critical path rather than a
nice-to-have.

`Log`'s JVM actual writes everything to **stderr**, debug included: the desktop
consumer of `:shared` is a CLI whose stdout is a machine-readable channel, and
one stray log line makes `--json` undecodable.

Verified: 1,271 shared tests pass on `:shared:jvmTest`, `:composeApp:assembleDebug`
and `:shared:compileKotlinIosArm64` are unaffected, `detektAll` is clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
The native library lands under JNA's own resource layout
(`linux-x86-64/`, `darwin-aarch64/`), which is what lets it travel *inside the
jar*: `Native.load("pubkycore")` extracts the matching file from the classpath at
runtime, so nobody installing the CLI fetches a native library by hand or sets
`-Djna.library.path`. Built by `build_desktop.sh` in pubky-core-ffi-fork; the
same arrangement `androidMain/jniLibs` already has for the four Android ABIs.

`UniffiPubkyClientJvmTest` is the part that has teeth. The 1,271 shared tests run
against `FakePubkyClient` and would pass identically on a machine where this
directory is empty, the architecture is wrong, or the file is one level off —
none of which shows up until the first homeserver call, hours later, as an
ordinary-looking transport error. Three offline assertions (mnemonic generation,
key derivation, validation) prove the binding is real without asserting anything
about a network.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
The last piece `:shared` needs before a CLI can start Koin: the ~15 bindings the
platform module has to answer for, in their desktop form.

The seven stores go to one 0600 JSON file each under `$XDG_CONFIG_HOME/loopky`,
split preferences from secrets the way Android splits SharedPreferences from its
keystore vault. The file is the **default**, not a fallback, and that is the
decision rather than an omission: libsecret is usually present on a desktop Linux
and usually absent on the headless box an agent runs on, so making a keyring the
default would fail exactly where the tool is meant to work. What it costs is
stated plainly — a session secret here is protected by file permissions and
nothing else — and what is stored is a capability-scoped, expiring session, never
a secret key. macOS gets the same file store today; the Keychain is the right
answer on that row and is that row's remaining work.

Writes are atomic (temp file, then `ATOMIC_MOVE`) because one of the things kept
here is the journal of reviews that have not reached the homeserver.

`MediaProcessor` is `javax.imageio` and **degrades rather than throws**: a
headless JRE with no AWT is a supported deployment, so a failed decode hands the
original bytes back and the caller uploads a larger picture than intended. That
is the right trade for a client whose card images are overwhelmingly remote refs
(#167) that never cross the wire at all.

The four remaining collaborators answer with an honest "no" rather than a stub
that pretends — a `Speaker` that silently succeeded would let a command claim it
read a card aloud. `NoPubkyRingPresence` returning false is load-bearing: it is
what makes desktop sign-in a printed QR code, the same fallback a tablet with no
Ring gets, instead of a deeplink into nothing. `InlineBackgroundTasks` drops both
deferred jobs, because a process that ends when its command does has no "later" —
neither job is needed for correctness, and what the CLI owes the user instead is
an explicit way to run them.

`pubkyEnvironment` is a required parameter with no default. The apps infer it
from the build type and cannot be talked out of it (#42); a binary has neither a
build variant nor a Settings screen, so the caller has to say.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
`loopky` — deck and card management from a terminal, so an agent can publish the
flashcards it is already good at writing (#54). A plain JVM module on `:shared`'s
new `jvm()` target; it consumes the repositories directly and never touches
`presentation/`, which is what the no-use-case-layer rule buys here.

Three properties everything is arranged around.

**The session scope is `/pub/loopky/:rw` and nothing else** — not the apps'
`DEFAULT_CAPABILITIES`. An agent session therefore cannot write a post, a follow
or a profile edit under any bug or any prompt injection, because it was never
handed the capability. The announce confirmation (#39) is gone by construction
rather than behind a flag: there is nothing to confirm. It costs less than it
looks, because deck tagging routes on the subject and a deck manifest's tag
record lives in `/pub/loopky/tags/` (Architecture.md §7.7), so `--tag` and the
language tags still work. What is given up is a feed presence, which a headless
deck tool has no business having — and a display name, which is why `whoami`
reports none.

**Nothing prompts, and stdout is a machine channel.** `--json` is a versioned
envelope carrying the result *and* the failure, with the environment and the
indexer on every one — because Nexus answers a query aimed at the wrong network
successfully and empty, and an agent that reads `[]` concludes its write failed
and retries. Exit codes distinguish not-signed-in from session-expired from
network, and session expiry has its own because it is hourly, unrecoverable
without a human (#165), and otherwise indistinguishable from a wobble.

**Long jobs are resumable and writes are idempotent.** `card add` twice with the
same sides is detectable and skipped; `import --resume` checkpoints against the
deck on the homeserver rather than a local cursor, because the deck records
exactly which cards arrived and survives the sandbox being thrown away. Batch
`--from-file` for add *and* edit, since editing is what an agent does after an
import and one card write is one `/session` round trip (#105) either way.

The import format carries image URLs from day one —
`front <TAB> back <TAB> front_image_url <TAB> back_image_url` — because a remote
ref costs no bytes and no quota since #167, and it is the one thing neither
`.apkg` nor TSV-through-the-parser can express.

Login prints a terminal QR (no Ring return-callbacks: there is no app to return
to, and a dangling `x-success` bounces a desktop login into the phone app).
`LOOPKY_SESSION` is read before the stored session and is the only way in on a
sandbox recreated per task. `LOOPKY_ENV` picks the network — a binary has no
build type to infer it from — and disagreeing with the session is a distinct
exit code rather than a warning.

Verified on macOS arm64 against the live network: real `pubkyauth://` URL with
`caps=/pub/loopky/:rw` and no callbacks, QR rendered and written as PNG, Nexus
trending read on both staging and production, and the exit-code/JSON contract
across not-signed-in, unknown-command and bad-input. 23 unit tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
Two bugs a green build said nothing about, both found by running the binary.

`loopky import cards.tsv` dispatched under the verb `"import cards.tsv"`. The
verb was "the first two positional words", which is right for `deck create` and
wrong the moment a command takes an operand: it matched nothing in the
dispatcher and came back as an unknown command, and it named the command
`import cards.tsv` in the `--json` envelope. It is now two words for the grouped
commands and one for the rest.

`loopky --version` printed the usage block, because a command line with no
positional words hits the "you gave me nothing" branch first.

Also: the JVM `Log` prints a stack trace only under `--verbose`. A warning or an
error is worth keeping on a terminal; twelve frames of coroutine machinery under
it bury the one line that says what went wrong, and the CLI already turns the
same failure into an exit code and a `--json` error object.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
Architecture.md gains §13 for `:cli` — why a JVM target rather than a Rust or
Python rewrite (the deck *protocol*, not just its schema, is what a second
implementation would have to track: the #49 marker manifest, the concurrent
chunk writes, the trailing-chunk sweep, the parser, `.apkg`, SRS); why the
session scope is `/pub/loopky/:rw` and what that structurally rules out; why
`--json` carries the environment and the indexer on every result; and the
exit-code table with the reasoning for session expiry having one of its own.

It also records the gaps as decisions rather than omissions: packaging is still
a jar plus a JRE, which is exactly what a one-line sandbox install cannot be;
the tarball carries both native rows; Windows is out of scope; and whether
Loopky can run behind an allowlist proxy at all is unresolved and is a hard
blocker for cloud sandboxes, because the homeserver is not a fixed hostname and
pkarr's DHT path is UDP.

A `cli-linux` CI job is the CLI's equivalent of a journey — those drive a screen
with `android-cli` and this has none. It runs the shared suite on the `jvm()`
target on a headless Linux x86_64 runner, which is where the `.so` is exercised
on real glibc for the first time, and asserts the exit-code and `--json`
contract against the built binary. Offline on purpose, so a network blip cannot
make it flake.

`journeys/RESULTS.md` gains a CLI section with what was checked by hand and,
more usefully, what was not: no Ring approval was completed, so every command
above the sign-in line is untested end to end and the acceptance criteria that
turn on a real session are unmet.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
….app

The self-tag's subject is a *profile*, so its record goes to
`/pub/pubky.app/tags/` (§7.7) — a namespace the headless client's session
(`/pub/loopky/:rw`, #54) was never granted. It is best-effort, so nothing broke:
it just fired a doomed write on every command that loaded a session, and logged
a warning about the scope working exactly as designed.

Asked rather than attempted. `Session.canWritePubkyApp` reads the capabilities
the homeserver actually granted, so shared code can tell "this write is not mine
to make" from "this write failed". A grant on `/pub/` counts, because comparing
for equality would read a broader grant as a narrower one; `:r` on the same
prefix does not, because read is not write.

It is a hint and the doc says so — a plain string prefix, so a capability ending
mid-segment would answer true. That costs one optional write attempted instead
of skipped, which is what happened before this existed, and the homeserver is
what enforces the scope. Not to be promoted to a gate on anything that matters.

Also records the Linux CI result in RESULTS.md: `cli-linux` passed, so the
cross-built `linux-x86-64/libpubkycore.so` loads through JNA on a real glibc
host and the whole shared suite runs there. That was the branch's biggest
unknown.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
`publish` `require`s both sides of every card, which throws an
`IllegalArgumentException` no classifier recognises — so a blank column in the
user's own TSV reached them as exit 1 "internal" plus a Kotlin assertion
message. It is now exit 9 with a message naming the row, which matters because
the whole point of a batch is that nobody is reading it line by line.

Checked only where a row becomes a *new* card. An edit row is allowed to carry
one side and mean "leave the other alone", and the import path needs none of it
because the shared parser already drops half-empty rows. An image counts as a
side, which is what the image columns are for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
The load-bearing safety check on the branch had no test. It is what stops the
one failure mode this design cannot absorb: a session on one network with
`--env` naming the other publishes fine — pkarr resolves both — while every
indexer-backed read comes back **successfully empty**, so an agent that writes a
tag, reads it back and sees `[]` concludes the write failed and retries.

Four cases, including the one that is easy to get backwards: it **fails open**.
The check is "this session sits on the *other* environment's known default
homeserver", which is a fact; an equality check against the requested
environment's default would refuse a legitimate self-hosted homeserver. So it
catches the common mistake rather than every mistake, and the doc now says so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018nKWTtN24p1VqMSNkQkMMr
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Review + a live run of loopky against staging

Reviewed the 10 commits on this branch, then drove the binary end-to-end against a real staging homeserver with a Pubky Ring session, importing chapter 1 of Tanenbaum/Feamster/Wetherall Computer Networks 6e as a 48-card deck. ./gradlew :cli:test :shared:jvmTest passes; libpubkycore loads from the jar on Linux with nothing installed by hand.

The run itself worked. 48 cards in one shot, correct ords, tags applied, read back identical:

loopky login --env staging --qr-out qr.png     # scanned in Ring, /pub/loopky/:rw only
loopky import tanenbaum-ch1.tsv --title "Computer Networks — Ch. 1: Introduction" \
  --separator tab --tag networking --tag tanenbaum --tag computer-science --env staging --json
→ deck avskolydfy2q, cards_written 48, duplicates_collapsed 0, truncated 0
loopky card list avskolydfy2q --env staging --json   → 48 cards, ord 0…47000 stride 1000

The headline claim of the design holds up: this is genuinely usable by an agent, the --json envelope is well-shaped, and the /pub/loopky/:rw-only capability request is a real structural guarantee rather than a flag.

Four of the findings below were reproduced on the device, not just read off the diff.


1. HIGH — loopky login mints a session with an empty homeserver, so the environment-mismatch guard never fires

Observed live, first thing:

{"pubky":"kfezy17c…","homeserver":"","capabilities":["/pub/loopky/:rw"],"sessionLive":true}

RingAuthFlowHandle.complete() (IdentityRepositoryImpl.kt:177) calls persistSession(session) straight from parseSessionPayload. The grant flow's session JSON carries no homeserver field — the code says so itself at :239 — so SessionPayloadParser.kt:35 defaults it to "". The two other session paths both backfill it: registerKey at :383 (ifBlank { homeserverPubky }) and the restore path at :243–246 (falls back to lookupHomeserver). The Ring sign-in path does neither.

The consequence lands on this PR's own headline safety feature. checkEnvironmentAgrees compares it.defaultHomeserver == session.homeserver; against "" nothing ever matches, so it returns without firing. ExitCode.EnvironmentMismatch, the whole of EnvironmentMismatchTest, and the README's "a session and an --env that disagree is a hard error" apply to a session shape that loopky login — the only way this client mints one — never produces. The test is green because sessionOn(…) hands the function a homeserver by hand.

That is exactly the failure the guard was written to prevent, and it now fails open in the common case rather than the documented edge case (a self-hosted homeserver). whoami also just reports a blank Homeserver line to the user.

Suggestion: backfill in complete() the way registerKey already does — session.copy(homeserver = session.homeserver.ifBlank { (lookupHomeserver(session.identity.pubky) as? HomeserverLookup.Registered)?.homeserverPubky.orEmpty() }) — and add a test that goes through a session built by the parser rather than by the test.

2. HIGH — desktop .apkg reading cannot work: the JDBC URL is never interpolated

shared/src/jvmMain/.../data/anki/ApkgReader.jvm.kt:25

DriverManager.getConnection("jdbc:sqlite:file:${'$'}{file.absolutePath}?mode=ro")

${'$'} in a regular Kotlin string emits a literal $, so the constant is the string jdbc:sqlite:file:${file.absolutePath}?mode=ro and the path never reaches the driver. Visible in the bytecode:

12: ldc #29  // String jdbc:sqlite:file:${file.absolutePath}?mode=ro

Every desktop .apkg import fails with "unable to open database file", which readArchive then reports as "That .apkg has no readable collection" — a wrong diagnosis pointing at the user's file. Nothing catches it because the CLI has no .apkg entry point yet, but this is the declared actual for the jvm target. The ${'$'} escape belongs in KDoc, not here.

3. MEDIUM — clearing a card side, the CLI's own documented gesture, exits 1 "internal" with a Kotlin assertion string

Reproduced live:

$ loopky card edit 4aeuczakzgtd fr2bw7fjsmq7 --back= --env staging --json
{"ok":false,"error":{"code":"internal","exit":1,"message":"Card fr2bw7fjsmq7 has an empty side"}}

cli/README.md says "clearing a side takes an explicit empty value", so this is the supported gesture, not misuse. CardSide.applying (Cards.kt:151) maps --back= to text = null and leaves imageRef alone; a card with no back image becomes fully empty, and DeckRepositoryImpl.upsertCard:316's require(!card.front.isEmpty && !card.back.isEmpty) throws an IllegalArgumentException that toErrorReason() classifies UnknownExitCode.Internal.

This is the same failure 1870ec4 fixed for card add. requireBothSides() guards the add and create paths, but the edit path is exempted at the row level (correctly — an edit row is allowed to be partial) and nothing re-checks the resulting card. Both --back= and a --from-file row with "back": "" hit it. An agent told "internal" retries; told exit 9 it fixes its input.

Suggestion: move the check onto the post-applying card and raise CliError(BadInput, …) naming the card id.

4. MEDIUM — card add --json echoes an ord the homeserver did not store

Reproduced live on a fresh deck:

$ loopky card add 4aeuczakzgtd --front "probe front" --back "probe back" --json
→ "ord": 1000
$ loopky card list 4aeuczakzgtd --json
→ stored ord: [0]

Cards.kt:96 seeds var ord = existing.maxOfOrNull { it.ord } ?: 0L then adds ORD_STRIDE → 1000 on an empty deck, while upsertCard:326 discards the caller's ord and recomputes (current.maxOfOrNull { it.ord } ?: -ORD_STRIDE) + ORD_STRIDE → 0. CardWriteResult.cards is built from the pre-write local Card, so ord and updated_at are intent, not result.

On a populated deck the two happen to agree (appendTarget always lands on the highest-numbered chunk, whose local max is the deck-wide max), so the empty-deck case is the reproducer — but for a surface whose stated purpose is that "a caller diffs intent against result from these bytes", echoing intent is the thing it set out not to do.

Suggestion: drop ord/updated_at from the write response, or re-read the card from the returned deck.

5. MEDIUM — whoami/login emit camelCase while the docs promise snake_case

Reproduced live — this is the actual whoami --json payload:

{"pubky":"","homeserver":"","capabilities":["/pub/loopky/:rw"],
 "sessionSource":"file","configHome":"","sessionLive":true,"displayName":null}

WhoamiResult and LoginResult (Identity.kt:43, :60) carry no @SerialName. Every other result type is snake_case — card list in the same run returned deck_id and updated_atViews.kt states snake_case as the rule, and both cli/README.md:98 and docs/Architecture.md:1292 tell an agent to read session_live, which does not exist. An agent following the documented contract reads null from the one field the README singles out as "worth checking before starting an hour-long import". This is schema: 1; cheaper to fix before a consumer exists.

6. MEDIUM — a third TSV column is stored as an image URL without ever checking it is a URL

Import.kt:270, CardFile.kt:131. imageColumnRows engages whenever every non-blank line has ≥3 tab fields, and imageAt wraps whatever is there in DraftCardImage(url = …). A 3-column Anki export (Front / Back / Example sentence) — a very common shape — passes the "consistent, so it is the format" test, and every card publishes with MediaRef.Image(url = "una manzana roja", sha256 = ""). Both apps then try to load prose as an image, the third column's real content is lost, and --json reports success. Every app-side constructor of this same ref gets its URL from an Unsplash picker, so this is the first path where an arbitrary string reaches it.

Suggestion: a startsWith("http") guard on imageAt and on the image-column detection, so the format decision is a fact rather than a guess.

7. MEDIUM — catch (error: Exception) in Main.kt misses Error, silencing the machine channel on the one failure the docs anticipate

Main.kt:83. A host outside the shipped matrix (x86-64 macOS, Windows) fails at Native.load with UnsatisfiedLinkError / ExceptionInInitializerErrorErrors. They skip fail(), fall through to main's runCatching, and produce a bare exit 1 with nothing on stdout, breaking the "results and failures both go there as --json" contract for precisely the case cli/README.md and Platform.jvm.kt both name. Same hole for startCli(environment) at :75, which sits outside the try. Catching Throwable at the dispatch boundary closes both — the detekt exclusion for Main.kt already anticipates a wide catch here.

8. LOW — a malformed LOOPKY_SESSION is reported as session_expired (exit 4)

$ LOOPKY_SESSION=not-a-real-session loopky whoami --env staging --json
{"error":{"code":"session_expired","exit":4,
 "message":"…invalid secret: expected `<pubkey>:<cookie>` LOOPKY_SESSION is no longer valid…"}}

A parse failure is bad_input (9), not expiry. The README makes a point of exit 4 meaning "the hour is up, mint a new one", and the whole reason it has a code of its own is so an agent can tell a dead session from a wobbly network. A typo'd env var teaching it "expired" is the same confusion one layer up.

9. LOW — --limit failures are silent

Tags.kt:40: args.option("limit")?.toIntOrNull() ?: DEFAULT_TRENDING_LIMIT turns --limit twenty into 20, and passes --limit 0 / --limit -5 straight through. For the one command whose entire output is a ranked list, a silently different N is the failure mode the envelope's environment/indexer fields exist to prevent elsewhere. CliError(Usage, …) fits the rest of the surface.

10. LOW — TSV lines starting with # are dropped with no report

CardFile.kt:122. readCardFile's own KDoc argues that "a file that produced fewer cards than it has lines is exactly the kind of loss --json exists to make visible", and treats blank/half-empty rows as errors for that reason — but a card whose front is #1 ranked, a markdown heading, or a hashtag vanishes silently. Either require # + space at line start, or report the skip count.

11. LOW — two docs that contradict the code

  • Identity.kt:56LoginResult.storedAt is documented as "null when --export printed it instead of storing it", but login always persists and always sets it. As written it tells a reader --export is a print-only mode, which is a meaningful claim about where a credential ends up.
  • InlineBackgroundTasks.kt:17 — points at loopky deck sync --compact. The command is loopky deck compact <deckId>; --compact is not in SWITCHES, so it would parse as an option swallowing the next token.

Checked and correct

  • UniffiPubkyClient is a pure rename of AndroidPubkyClient (diff is comment-only plus the class name) — the Android FFI path is unchanged.
  • Capability.grantsWriteTo prefix logic is right in both directions, and the selfTagAsLoopkyUser skip correctly avoids a doomed /pub/pubky.app/ write for a /pub/loopky/:rw session. Confirmed live: sign-in with loopky-only caps completed cleanly with no self-tag error.
  • Args.parse handles --, bare -, --key=value, repeated options and empty values correctly; the declared-switch approach genuinely prevents --front --back x misreads.
  • JsonFileStore.persist is atomic (temp + ATOMIC_MOVE) and chmods before the move, so the 0600 window is never open.
  • deck delete and card rm both worked and left the deck consistent (card_count 49 → 48).
  • The half-block TerminalQr renders a scannable code, and --qr-out produces a clean 512px PNG. Ring approved it on the first scan.

Suggested order

1 and 2 before merge — 1 disables a safety feature this PR ships and tests, 2 makes a declared actual non-functional. 3, 4, 5 are cheap and all three change the schema: 1 contract, so they are worth doing while there is no consumer. 6 through 11 are follow-ups.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Addendum: a focused pass on session and credential handling

Separate from the functional review above, at the author's request. Verified on the same live staging run, on disk, not only from the diff.

What is right, and verified

  • ~/.config/loopky is 0700, secrets.json is 0600 — checked with stat after a real login. JsonFileStore.persist writes to a temp file, chmods it, then ATOMIC_MOVEs, so the permissive window never exists. That ordering is the part people usually get wrong.
  • Preferences and secrets are separate files, so clearing one cannot discard the other, and preferences.json stays safe to attach to a bug report.
  • AuthUrlRedaction is unusually well built. The auth-URL redactor is a deny-list on secret/st query keys; the session-payload redactor is deliberately not "the JSON with secrets masked" but a shape summary, on the stated grounds that the payload comes from Ring and its key set is not ours to predict. Allow-listing what to print rather than deny-listing what to hide is the correct call, and the KDoc says why.
  • adoptSession sets the session provider and never the store (IdentityRepositoryImpl.kt:134), so a LOOPKY_SESSION handed to a sandbox is never written to that sandbox's disk. Correct and load-bearing.
  • The capability request is genuinely minimal. /pub/loopky/:rw and nothing else — confirmed live: the session that came back carried exactly that one capability, and selfTagAsLoopkyUser correctly skipped the /pub/pubky.app/ write instead of failing.
  • Log.d is off unless --verbose, and the one error we triggered with a malformed secret printed the expected format rather than the value supplied.

S1 — MEDIUM. --qr-out writes a live bearer credential at default umask, and never removes it

$ stat -c '%A %n' qr.png
-rw-rw-r-- qr.png

TerminalQr.writePng does mkdirs() + ImageIO.write with no permission handling. The PNG is the pubkyauth:// URL, secret= included — a QR code is an encoding, not a protection. By AuthUrlRedaction's own KDoc that secret "is the client secret the auth token is encrypted to, so leaking it turns the relay's encrypted blob back into a usable token": anyone who reads the file before the user approves in Ring can poll the relay and take the session instead of the legitimate client.

The flag exists for "a box with no terminal anyone is watching", so --qr-out /tmp/qr.png on a shared host is the intended usage, and it publishes that credential to every uid on the machine for the length of the approval window. The file is also still there afterwards.

The same codebase goes to real trouble for secrets.json — temp file, chmod, atomic move — and then writes this one at whatever umask happens to be.

Suggestion: create the PNG 0600 the way JsonFileStore.restrictToOwner does, and unlink it once complete() returns.

S2 — MEDIUM. The full auth URL, secret included, goes to stderr unconditionally

Identity.kt prints the QR and then stderr(handle.authUrl) verbatim, every time. cli/README.md states that "progress, the QR code, prompts and every log line go to stderr" — which is exactly the stream an agent harness captures into a transcript that may be logged, uploaded, or pasted into an issue.

This is the value redactAuthUrl exists for, with a KDoc opening "the two auth artefacts that must never reach a log sink." The helper is applied to Log.d on the same path and bypassed by the line right next to it.

It cannot simply be redacted — the URL has to be usable, that is the point of the command. But a user who scans the QR gets the plaintext secret in their scrollback for no benefit at all.

Suggestion: print the plaintext URL only under --url-only (where it is the deliverable), keep the QR otherwise, and add one line saying the URL is a credential until approved.

S3 — MEDIUM. There is no revocation path for the one credential the README tells you to copy to a sandbox

logout refuses outright when LOOPKY_SESSION is set:

"This session comes from LOOPKY_SESSION, so there is nothing stored to clear. Unset the variable instead."

But IdentityRepositoryImpl.signOut does two things — pubky.signOut(current.sessionSecret) against the homeserver, then sessionStore.clear() — and the refusal's reasoning only covers the second. The effect is that a secret minted with login --export and carried to a cloud sandbox can never be revoked from this tool: logout refuses, unsetting the variable changes nothing server-side, and the session stays live until it expires on its own. For a credential whose whole design story is "hand this to an ephemeral agent box", that is the wrong end to be missing.

Suggestion: let logout revoke an injected session (it is the only thing it can usefully do there) and report that it revoked rather than cleared, or add loopky session revoke.

S4 — LOW/MEDIUM. signOut reports success when server-side revocation failed

IdentityRepositoryImpl.kt:101pubky.signOut(current.sessionSecret) is called and its Result is discarded, then the local state is cleared unconditionally and the CLI prints Signed out. with exit 0. If the revoke call failed — offline, homeserver down, session already borderline — the bearer token is still valid and the user has been told otherwise.

Clearing locally regardless is defensible (a user asking to sign out should end up signed out on this machine). Reporting it as an unqualified success is not, and the CLI has the vocabulary to say the difference.

S5 — LOW. login --export --json puts a live credential on stdout, and stores it too

cliJson uses encodeDefaults = true, explicitNulls = true, so LoginResult.sessionSecret is emitted as null normally and as the real secret under --export. loopky login --export --json > session.json therefore writes a bearer token to a file at default umask — the same class of exposure as S1, one step removed.

It compounds finding 11 in the main comment: storedAt is documented as "null when --export printed it instead of storing it", but --export prints and persists. A reader trusting that KDoc believes --export is the safer, no-disk mode when it is strictly the mode that puts the secret in two more places.

S6 — LOW. The env-var channel's exposure is not documented, though the file channel's is

The README is admirably direct about secrets.json: "a mode-0600 file, not an OS keyring… protected by file permissions and nothing else." It says nothing equivalent about LOOPKY_SESSION, which it recommends more strongly and which is the weaker channel: readable by any same-uid process through /proc/<pid>/environ, inherited by every child the CLI spawns, and captured in shell history when set inline.

Worth one sentence in the same section, plus a note that LOOPKY_SESSION=… loopky … as a prefix keeps it out of the environment of unrelated processes but not out of history.


Priority

S1 and S3 first: S1 leaks a live credential to other users on the host in the tool's own documented headless usage, and S3 means a leaked one cannot be withdrawn. S2 is cheap and removes a needless copy. S4–S6 are correctness-of-claim issues — each one tells the operator something about credential handling that is not quite true, which is the category that costs most later.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

jvsena42 and others added 7 commits September 3, 2026 07:12
`"jdbc:sqlite:file:${'$'}{file.absolutePath}?mode=ro"` — `${'$'}` is a Kotlin
template producing a literal `$`, so the constant reached the driver with
`${file.absolutePath}` in it verbatim and the path never arrived. That escape
belongs in KDoc; it leaked in from the heredoc this file was first written
through.

Every desktop `.apkg` read failed at "unable to open database file", which
`readArchive` then reported as "That .apkg has no readable collection" — a wrong
diagnosis pointing at the user's own file. Nothing caught it because the shared
suite drives `JvmApkgReader`'s callers rather than the opener, and the CLI has no
`.apkg` entry point yet, so a declared `actual` for the jvm target has been
non-functional since it landed.

`ApkgReaderJvmTest` is the test with teeth: it builds a real zip around a real
SQLite collection and reads it back, so the opener is actually exercised.
Confirmed to fail on the old string and pass on the new one.

Found in review by jvsena42 on #208.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
The FFI's session JSON carries no `homeserver` field, so `parseSessionPayload`
defaults it to `""`. Two of the three paths that mint a session already backfill
it — `signInWithKey` from the pre-flight lookup, `registerHeldKey` from the
server it registered against — and the **Ring deeplink path did not**. Nor did
`adoptSession`, the `LOOPKY_SESSION` path added on this branch.

That is worse than the "Unknown" it rendered in Settings. The CLI refuses to run
when a session and the requested `--env` disagree, and decides that by comparing
the session's homeserver against the other environment's known default — which a
blank value never matches. So `ExitCode.EnvironmentMismatch`, its tests and the
README's "a session and an --env that disagree is a hard error" all applied to a
session shape that `loopky login` never produces. The guard failed open in the
common case rather than the documented edge case, and a live staging sign-in
reported `"homeserver":""`.

`EnvironmentMismatchTest` was green because it hands the function a homeserver by
hand — the test proving the test. The new coverage goes through
`beginSignIn().complete()` and `adoptSession()` with raw payloads instead, so it
exercises the shape the real flow produces. Both fail on the old code.

Best-effort: a DHT that will not answer leaves the blank rather than failing a
sign-in that has otherwise succeeded.

Found in review by jvsena42 on #208, reproduced live against staging.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
`WhoamiResult` and `LoginResult` carried no `@SerialName`, so they serialised as
`sessionSource`, `configHome`, `sessionLive`, `displayName`, `sessionSecret`,
`storedAt` — camelCase, alone on a surface where every other result type is
snake_case and `Views.kt` states that as the rule.

Both `cli/README.md` and Architecture.md tell an agent to read **`session_live`**,
which did not exist. That is the one field the README singles out as "worth
checking before starting an hour-long import rather than forty cards in", so a
caller following the documented contract read null from it. This is `schema: 1`
and there is no consumer yet, which is exactly when to fix it.

Also corrects `storedAt`'s doc, which claimed it is "null when `--export`
printed it instead of storing it". `login` always persists and always sets it —
`--export` *also* prints the secret. As written it told a reader that `--export`
is a print-only mode, which is a meaningful claim about where a credential ends
up.

Found in review by jvsena42 on #208.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
…a side

Two findings in the same function, both about `card` writes telling the truth.

**The response echoed intent, not result.** `upsertCard` discards the caller's
`ord` and recomputes one from the chunk the card lands in, so `card add --json`
reported `ord: 1000` on an empty deck where 0 was stored. On a populated deck the
two happen to agree, which is why it took a fresh deck to see. For a channel
whose stated purpose is that a caller diffs intent against result from these
bytes, echoing intent is the one thing it set out not to do. The response now
reads the card back — a cache hit, since the write just populated it.

**Clearing a side exited 1 "internal".** `--back=` is the CLI's own documented
gesture for it, and `DeckRepositoryImpl.upsertCard` `require`s both sides,
throwing an `IllegalArgumentException` that `toErrorReason` classifies `Unknown`.
So the supported way to clear a side answered with a Kotlin assertion string and
the code that means "report this as a bug". Same failure `1870ec4` fixed for
`card add`, which the row-level guard cannot cover here: an edit row is
*allowed* to be partial, so what has to be checked is the card the edit
produces. Now exit 9, naming the card and which side went empty — an agent told
"internal" retries, told "bad input" it fixes its input.

Found in review by jvsena42 on #208, both reproduced live against staging.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
Four inputs the CLI accepted and turned into something the caller did not ask
for.

**A third TSV column was stored as an image URL without being one.** The
image-column format engaged whenever every line had three or more tab fields, so
a three-column Anki export — Front / Back / Example sentence, a very common shape
— published every card with `MediaRef.Image(url = "una manzana roja")`. Both apps
then try to load prose as a picture, the column's real content is lost, and
`--json` reports success. Every app-side constructor of a remote image ref takes
its URL from a picker; this was the first path where an arbitrary string reached
one. Now the format test requires the image columns to hold `http(s)` URLs, and
the two entry points differ deliberately: `import` falls through to the text
parser (there the third column is content somebody wants imported), while
`card add --from-file` errors (there the four-column TSV is what was explicitly
asked for).

**A malformed `LOOPKY_SESSION` reported `session_expired`.** A parse failure is
not an expiry, and exit 4 has a code of its own precisely so an agent can tell a
dead session from a wobbly network — teaching it "expired" for a typo'd
environment variable is that confusion one layer up. The shape is checked before
the FFI sees it.

**`--limit twenty` silently became 20**, and `--limit 0` passed straight
through, for the one command whose entire output is a ranked list. A silently
different N is the class of quiet wrongness the envelope's `indexer` field exists
to prevent elsewhere.

**A card whose front is `#1 ranked` vanished.** `startsWith("#")` swallowed
markdown headings and hashtags, against this file's own contract that a batch
producing fewer cards than it has lines is exactly the loss `--json` exists to
show. A comment is now `#` followed by whitespace.

Also exempts the two test trees this branch added from `MagicNumber` and
`TooManyFunctions`, which every other test tree already was — a test class's
function count is how much behaviour it pins down.

Found in review by jvsena42 on #208.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
A host outside the shipped matrix — an x86-64 Mac, Windows — fails at
`Native.load` with `UnsatisfiedLinkError` or `ExceptionInInitializerError`. Both
are `Error`s, so they walked past `catch (Exception)`, skipped `fail()`, and
produced a bare exit 1 with **nothing on stdout**.

That breaks "results and failures both go there as `--json`" for precisely the
case `cli/README.md` and `Platform.jvm.kt` both name as expected — the one where
an agent most needs a machine-readable reason rather than silence. `startCli`
sat outside the `try` for the same reason it should not have: resolving
`PubkyClient` is where that load happens.

`stopCli` in the `finally` is now best-effort, since Koin may never have started
and throwing there would bury the failure being reported.

Found in review by jvsena42 on #208.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
Eleven findings, four reproduced on a real staging homeserver with a Ring
session. Worth keeping the table rather than only the commits, because two of
them had *passing tests* over them: the environment guard's test handed it a
homeserver by hand, and the shared suite drives the `.apkg` reader's callers
rather than its opener. Both now have tests that go through the real shape, and
both were confirmed to fail on the old code first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

All eleven fixed, and thank you — the live run caught things a green build could not have. Two of them had passing tests over them, which is the part I'll remember.

Follow-up for the version check / loopky update you raised separately: #209.

The two blockers

1 — the blank homeserver. Fixed in a8f1c68, and you were right that this is the serious one: the guard failed open in the common case rather than the documented edge case. Fixed in both session paths, not just the one you found — adoptSession (the LOOPKY_SESSION path added on this branch) had the identical hole, and it matters more there, since an injected session is the shape a container runs on and --env is the only thing telling it which network it is on.

Your point about the test proving the test is the reason I did it the way you suggested: the new coverage goes through beginSignIn().complete() and adoptSession() with raw payloads, so it exercises what the flow produces rather than what a fixture supplies. Confirmed both fail on the old code before the fix landed.

2 — the JDBC URL. Fixed in 1893955. That ${'$'} is mine, leaked in from the heredoc the file was first written through. ApkgReaderJvmTest now builds a real zip around a real SQLite collection and reads it back, which is the only shape of test that would have caught it — the shared suite drives JvmApkgReader's callers, never the opener. Also verified to fail on the old string.

The schema three

  • 5 — camelCase. 14baba7. Both docs pointed at session_live and it did not exist, which is the worse half. Also corrected storedAt's doc while I was in there — your finding 11.
  • 4 — echoing intent. cd48fcd. The response now reads the card back (a cache hit; the write just populated it), so ord and updated_at are result rather than intent.
  • 3 — clearing a side. Same commit. Checked on the card the edit produces, since the row-level guard correctly cannot cover it, and it names which side went empty.

The rest

0d5707b covers 6, 8, 9, 10. On 6 I took the two paths deliberately differently: card add --from-file errors on a non-URL image column, because the four-column TSV is what the caller explicitly asked for — while import falls through to the shared text parser, because there a three-column Anki export is content somebody wants imported rather than a malformed image file. Your startsWith("http") suggestion, plus the same check on the format-detection test so a 3-column export never engages the image path at all.

23ddbbf is 7: Throwable, and startCli moved inside the boundary, since resolving PubkyClient is where Native.load actually happens. stopCli in the finally is now best-effort so it cannot bury the failure being reported.

Verified

:cli:test 41, :shared:jvmTest 1,291, :shared:allTests, :composeApp:assembleDebug, detektAll — all green. Findings 8 and 9 re-checked on the built binary:

LOOPKY_SESSION=not-a-real-session loopky whoami --json
→ {"error":{"code":"bad_input","exit":9,"message":"LOOPKY_SESSION is not a session secret — …"}}
loopky tag trending --limit twenty --json
→ {"error":{"code":"usage","exit":2,"message":"--limit must be a positive whole number, not 'twenty'."}}

journeys/RESULTS.md has the full table.

Still not verified by me: the sign-in half. Your run is the only end-to-end evidence on this branch — I have never completed a Ring approval, so deck create, import, card add/edit/rm and deck sync/compact against a live session rest entirely on your 48-card import.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 2 — re-reviewed and re-tested at fbe0738

Pulled the six fix commits, re-ran :cli:test / :shared:jvmTest / detektAll (all green, 41 CLI tests), rebuilt, and drove the binary against the same staging homeserver again.

Nine of the eleven are confirmed fixed on the device, not just in the diff. Two things came out of the re-test, and the security addendum is untouched.

Verified fixed, live

Finding Evidence from this run
snake_case keys whoami --json now returns session_source, config_home, session_live, display_name
malformed LOOPKY_SESSION exit 9 bad_input"it should look like <pubkey>:<cookie>", and the FFI never sees it
--limit twenty exit 2 usage"must be a positive whole number, not 'twenty'"; --limit 3 still returns 3
card add ord echo fresh deck: echoed ord: 0, stored ord: 0 — was 1000 vs 0
card edit --back= exit 9 bad_input"Card l6ffjhrowzq2 would be left with an empty back… An image counts as a side"
3-column prose via card add --from-file exit 9, naming the coordinate: "Line 1, column 3 is an image column but holds "una manzana roja""
3-column URL via card add --from-file still accepted, mime inferred as image/jpeg — the guard did not overshoot
#1 ranked survives as a card front; # a real comment still skipped. 2 written from 3 lines, as intended
homeserver backfill (injected path) adoptSession now resolves ufibwbmed6…, and --env production against it is refused with exit 8

That last row is the one that matters most — the environment guard fires for the first time. Which leads to the one real finding of this round.


R2-1 — MEDIUM. The homeserver fix does not heal a session that is already stored, and the guard stays open for it

withResolvedHomeserver() is applied in complete() and adoptSession(). loadPersistedSession (IdentityRepositoryImpl.kt:83) is untouched — it returns sessionStore.load() verbatim.

So the same session behaves two ways depending on which door it comes through. Both of these are the identical staging session on this machine, one run apart:

# read from ~/.config/loopky/secrets.json
$ loopky whoami --env production --json
{"ok":true,…,"homeserver":"","environment":"production","indexer":"https://nexus.pubky.app"}
rc=0                                  ← the guard does not fire, reads go to the production indexer

# the same secret, injected
$ LOOPKY_SESSION=… loopky whoami --env production --json
{"ok":false,"error":{"code":"environment_mismatch","exit":8,
 "message":"This session is on staging (homeserver ufibwbmed6…) but --env/LOOPKY_ENV says production…"}}
rc=8                                  ← correct

Anyone who ran loopky login before this commit — and every Android/iOS user who signed in through Ring, since the blank was written by the shared code — keeps a blank homeserver until they sign in again. Nothing prompts them to, and session_live: true says the session is fine. The commit message's own framing is that the guard "failed open in the common case"; for existing installs it still does, and there is now no symptom at all because whoami looks healthy.

Suggestion: re-resolve in loadPersistedSession when homeserver.isBlank(), and persist the result so it costs one DHT lookup once rather than every invocation. The helper is already suspend and already best-effort, so it drops straight in. Worth a line in the app's session-load path too, for the same reason.

R2-2 — LOW/MEDIUM. import on a 3-column prose file now silently discards the third column

The fix routes such a file to the text parser instead of storing prose as an image — right call — but the fall-through drops the column rather than importing it:

$ printf 'manzana\tapple\tuna manzana roja\n…' > anki3col.tsv
$ loopky import anki3col.tsv --title "…" --separator tab --json
→ cards_written: 3, ok: true
$ loopky card list … --json
→ front 'manzana' / back 'apple' / image None      ← "una manzana roja" is gone

0d5707b's rationale is that import is more forgiving "since there the third column is content somebody wants imported" — but it is not imported, it is dropped, and --json reports unqualified success. That is a quieter version of the same class the commit set out to close, and it contradicts readCardFile's stated contract, quoted in that very commit: "a file that produced fewer cards than it has lines is exactly the kind of loss --json exists to make visible." Here it is not fewer cards, it is less of each card, with nothing reporting it.

Suggestion: either append the extra column to the back (what "content somebody wants imported" would mean), or report columns_dropped: 1 in the import result. Silently is the one option that does not fit the rest of this surface.

R2-3 — the security addendum is unaddressed

None of S1–S6 from the second comment appear in this round. Not raising them again in full, but the two that are more than documentation:

  • S1--qr-out writes the PNG at default umask (-rw-rw-r-- observed) and never removes it. That file is the pubkyauth:// secret; a QR is an encoding, not a protection. TerminalQr.writePng should do what JsonFileStore.restrictToOwner does, and unlink after complete().
  • S3logout refuses when LOOPKY_SESSION is set, so pubky.signOut(secret) is unreachable for exactly the credential the README tells you to carry into a sandbox. There is currently no way to revoke an exported session from this tool.

Notes on the fixes themselves

Read them all; the reasoning is sound and the two new test files go through the real shapes rather than hand-built ones — ApkgReaderJvmTest around a real zip and a real SQLite file, IdentityRepositoryImplTest through beginSignIn().complete() and adoptSession() with raw payloads. That was the actual defect in both cases, so testing the shape is the right correction.

Three small observations, none blocking:

  • 23ddbbf moving startCli inside the try is the better half of that fix — the Native.load failure happens during Koin resolution, so catching Throwable alone would not have covered it. Worth noting the finally now swallows a stopCli failure entirely; that is right here, but it means a Koin teardown bug would be invisible.
  • isComment() matches on the untrimmed line, so # header is not a comment. Conservative and consistent with treating the tab layout literally — just worth knowing it is deliberate.
  • looksLikeImageUrl is scheme-only by design, which is correct, and means a data: or pubky:// ref in an image column is now refused. No current path produces one, so this is a note rather than a finding.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 3 — import --resume, driven on staging

No new commits since fbe0738; this round covers the one headline feature no round had exercised yet. --resume is what an agent relies on when a long import dies to the hourly session expiry (#165), and it is the path least likely to be tried by hand.

It works — a resumed run does find the existing deck and does skip the cards already there. Three problems around that, all reproduced live.

R3-1 — MEDIUM. --resume is ~10× slower than the publish it is recovering, and it gets worse as the deck grows

Same 30 cards, same homeserver, one after the other:

fresh publish, 30 cards   →  4.3s
--resume,      30 cards   → 40.7s      (10 already present, 30 written)

publish chunks: CardChunking.chunk(cards)ceil(n/100) chunk records plus one manifest — 2 writes for 30 cards. appendMissing (Import.kt:216) loops decks.upsertCard one card at a time, and each of those is a chunk write plus a full manifest read-modify-write — 60 writes for the same 30 cards. The manifest carries the whole chunk table and is rewritten whole every time, so the per-card cost rises with deck size; at 500 cards the gap is far worse than the 10× measured here.

The consequence is the part that matters. resumeState's own KDoc says the point is that "an import killed by the hourly session expiry (#165) finishes instead of starting over" — but the recovery path is an order of magnitude slower than the attempt that just ran out of time, on the same one-hour budget. A large import that dies at 55 minutes cannot finish on resume; it will die again earlier in the file each round. The mechanism makes the failure it exists for more likely on the retry.

Suggestion: appendMissing should batch — group the missing cards into the deck's existing tail chunk and new chunks, and write chunk-then-manifest per chunk rather than per card. publishLocked already does exactly this; the resume path is the only caller doing it the expensive way.

R3-2 — MEDIUM. --resume cannot tell a first run from a typo'd --title, and silently duplicates the deck

$ loopky import resume-40.tsv --title "ZZ resume prboe" --resume --json
{"ok":true,…,"deck":{"id":"94dv6snzl6we","title":"ZZ resume prboe"},
 "cards_written":40,"resumed":0}
rc=0

One transposed character against the existing ZZ resume probe, and the result is a second full deck: 40 cards written, quota spent twice, two near-identically titled decks in the library. resumeState (Import.kt:151) returns ResumeState(null, emptySet()) when nothing matches and the caller falls straight through to newDeck.

resumed: 0 is the only signal, and it is exactly what a legitimate first --resume run reports, so nothing downstream can distinguish the two. The README makes --title mandatory because resume matches on it — "a title inferred from a filename is one rename away from creating a second deck instead" — but the assumption that the title is right is never checked, and the failure it was hardening against arrives through the flag instead of the filename.

Suggestion: when --resume is given and no deck matches, say so before creating one — either refuse with exit 6 and list the near-miss titles, or report resume_matched: false as a distinct field so a caller can tell. The information is already in hand at that point.

R3-3 — MEDIUM. Deck metadata passed to a --resume run is silently discarded

$ loopky import resume-40.tsv --title "ZZ resume probe" \
    --tag beta --description "added on the resume run" --resume --json
→ tags: ["alpha"]   description: null   rc=0

newDeck is only reached on the fresh path; appendMissing writes cards and never touches metadata. So --tag, --description, --cover-url, --cover-emoji, --listen, --speak, --type, --reverse, --front-lang and --back-lang are all accepted, all ignored, and nothing is reported.

The natural way to use this feature is to re-run the same command with --resume appended — that is what the README's --resume example shows — so an agent that got its deck half-written and re-runs its own invocation quietly loses every flag except --title. The language pair is the sharpest edge: --front-lang/--back-lang dropped means Deck.speechReady stays false and Listen and Speak never appear, with the CLI reporting success.

Suggestion: apply the metadata on the resume path too (it is a updateMetadata call the code already has), or refuse the flags with exit 2 saying they only apply when the deck is created. Accepting and dropping them is the one option that leaves the caller wrong.

LOW — the resume target is not well-defined when two decks share a title

decks.listOwned().firstOrNull { it.title == title }. Nothing prevents two decks with the same title — R3-2 is one way to end up with them — and which one a resume appends to is then whichever listOwned happens to return first. For a value the KDoc calls "the checkpoint", worth being deterministic (newest created_at, say) or refusing the ambiguity.


State of the review

Round HIGH MEDIUM Status
1 2 5 all fixed and verified live
1 (security) 0 3 open — S1, S3 unaddressed; S2 open
2 0 2 open — R2-1 stored-session migration, R2-2 dropped column
3 0 3 open — this comment

Still finding MEDIUMs, so I'll keep going: pulling new commits as they land, re-running the affected paths on staging, and reporting each round here. Nothing above blocks the branch's core claim — the import worked, the deck is on the homeserver, the apps can open it — but --resume is currently a feature that costs more than it saves on exactly the imports it was built for.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

jvsena42 and others added 2 commits September 3, 2026 07:43
Routing a 3-column prose file to the text parser rather than storing the third
column as an image URL was the right call, but the fall-through *drops* that
column — spec §8 keeps fields 0 and 1 — and `--json` reported unqualified
success. So the import succeeded and every card was missing something the file
held.

That is a quieter version of the class the previous commit set out to close: not
fewer cards, less of each card, with nothing reporting it. It also contradicts
`readCardFile`'s own contract, quoted in that commit — "a file that produced
fewer cards than it has lines is exactly the kind of loss `--json` exists to
make visible".

Reported rather than repaired. Appending the stray column to the back would
change what the card says, and diverging from the app's parser is the one thing
this command must not do; `columns_dropped` in the result and a line on stderr
say what happened and leave the decision with the caller.

Found in review by jvsena42 on #208 (R2-2), reproduced live.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
One pass over session and credential handling on #208, six findings. They share
files (`Identity.kt` spans the login and logout halves) so they land together.

**`--qr-out` wrote a live bearer credential at the ambient umask and never
removed it** (observed `-rw-rw-r--`). That PNG *is* the `pubkyauth://` URL,
`secret=` included, and a QR code is an encoding rather than a protection —
anyone who reads it before approval can poll the relay and take the session
instead of the legitimate client. The flag exists for a headless box, so
`--qr-out /tmp/qr.png` on a shared host is the *intended* use. Now created 0600
before a byte is written, the way `JsonFileStore.persist` does it, and deleted
when the command ends.

Two things there only showed up by running it. `ImageIO.write(…, File)` deletes
and recreates the file, throwing the mode away — so the write goes through a
stream on the file we created. And a `finally` is not enough: `login` blocks on
the relay for as long as it takes somebody to reach for their phone, so the
ordinary way it ends is **^C**, which takes the JVM down without unwinding. A
shutdown hook covers it; verified by SIGTERMing a waiting login.

**The plaintext auth URL went to stderr unconditionally** — the stream an agent
harness captures into a transcript that may be logged or pasted into an issue.
`redactAuthUrl` exists for exactly this value and was applied to `Log.d` on the
same path while the line beside it printed it in full. It cannot be redacted, so
it is printed only under `--url-only` where it is the deliverable, with a line
saying it is a credential until approved.

**An injected session had no revocation path at all.** `logout` refused when
`LOOPKY_SESSION` was set, on the true but irrelevant grounds that nothing is
stored to clear — sign-out does two things and only the local half was missing.
A secret minted with `login --export` and carried into a sandbox stayed live
until it expired. `revokeSession` is that path, and it is deliberately not
`signOut`: on a developer's machine both credentials exist at once, and
`signOut` would revoke the injected one while wiping the stored one.

**`signOut` reported success when server-side revocation had failed** — the
result was discarded. Clearing locally regardless is right, a user asking to
sign out should end up signed out here whatever the network is doing; reporting
it as an unqualified success while the bearer token is still live is not. It now
returns `SignOutOutcome` and the CLI says which half happened.

**The homeserver backfill did not reach sessions already on disk.** Applied in
`complete()` and `adoptSession()` but not `loadPersistedSession`, so the same
session behaved two ways depending on which door it came through — read from
`secrets.json` the environment guard did not fire, injected as `LOOPKY_SESSION`
the identical secret was correctly refused. Every account signed in through Ring
before that fix has the blank, on both apps as well as the CLI, nothing prompts
them to sign in again, and `session_live` says it is healthy. Now re-resolved on
load and **persisted**, so the DHT is asked once.

**`--export --json` warns** that it is about to put a live secret on stdout, and
`LOOPKY_SESSION`'s exposure is documented beside the file store's — it is the
weaker of the two channels and the one the README recommends more strongly:
readable through `/proc/<pid>/environ`, inherited by every child, and in shell
history when set inline.

Found in review by jvsena42 on #208 (S1–S6, R2-1).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 3. All of R2-1, R2-2 and S1–S6 are addressed, and I owe you an apology for the addendum: my earlier reply truncated before it and I answered only the functional comment. Nothing in it was disputed, it simply went unread.

R2-1 — the guard was still open for every install that exists

You are right that this is the important one, and the framing is worse than "the fix missed a path": for existing installs it fails open with no symptom at all, because session_live says healthy.

loadPersistedSession now heals and persists, so the DHT is asked once rather than per invocation. Two tests: one that a blank stored session comes back resolved and is written back, one that a session which already knows its homeserver costs no lookup. I took your point about the app's session-load path too — it is the same function, so both apps get the heal for free.

S1 — the QR file

Fixed, and the fix was wrong twice before it was right. Both only surfaced by running it:

  • ImageIO.write(…, File) deletes and recreates the file, throwing away the mode set beforehand. The 0600 was there and had no effect. It now writes through a stream on the file we created.
  • A finally does not cover the way this command actually ends. login blocks on the relay for as long as it takes someone to reach for their phone, so the normal exit is ^C — a signal, which takes the JVM down without unwinding. Added a shutdown hook and verified by SIGTERMing a waiting login:
--- while waiting ---   -rw-------  20917  /tmp/qr-sec2.png
--- after ^C ---        ls: /tmp/qr-sec2.png: No such file or directory

QrCredentialTest starts the file world-readable before calling, so it proves the writer set the mode rather than inheriting a strict umask from whatever ran the test.

S3 + S4 — revocation

revokeSession is new. Your reading of the refusal was exactly right — sign-out does two things and the message only justified skipping the second. I did not route it through signOut, for a reason worth recording: on a developer's machine both credentials exist at once, and signOut would have revoked the injected session while wiping the stored one.

S4 too: signOut returns SignOutOutcome and the CLI now distinguishes "Signed out." from "Cleared this machine's session, but the homeserver did not confirm it was revoked."

S2, S5, S6, R2-2

  • S2 — the plaintext URL prints only under --url-only. Default is the QR plus a line pointing at the flag; verified grep -c 'pubkyauth://' on default stderr is 0.
  • S5--export --json warns on stderr before putting a live secret on stdout, and says it is stored as well.
  • S6 — documented in the README beside the file store's note, and in Architecture.md §13.5. Your /proc/<pid>/environ + inheritance + history framing, since all three are true and none was written down.
  • R2-2columns_dropped in the result plus a stderr line. I chose reporting over appending the column to the back: appending changes what the card says, and diverging from the app's parser is the one thing this command must not do.

On your three observations

All three are deliberate, and worth confirming rather than leaving implied:

  • The finally swallowing a stopCli failure — agreed that a Koin teardown bug would be invisible. Kept, because the alternative is a teardown error burying the failure actually being reported, and that trade only goes one way at a process boundary.
  • isComment() on the untrimmed line — deliberate, and consistent with treating the tab layout literally.
  • looksLikeImageUrl refusing data:/pubky:// — deliberate; no path produces one, and widening it later is easier than narrowing it.

Verified

44 CLI tests, 1,296 shared, :shared:allTests, :composeApp:assembleDebug, detektAll — green. S1 and S2 checked on the built binary as above.

Still not verified by me: anything past sign-in. Your two staging runs remain the only end-to-end evidence on this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

Two things found by running the command I had just changed, both the same shape
as the review's S4 — a report that claims more than happened.

**`logout` with no session at all answered `revoked: true`.** `signOut` treated
"there was nothing to revoke" as a successful revocation, so a fresh machine was
told a credential had been withdrawn that never existed. Sign-out being
idempotent is right; saying it revoked something is not. `SignOutOutcome` now
carries `hadSession`, and the three states read differently: "Not signed in —
nothing to revoke", "Signed out", and "Cleared this machine's session, but the
homeserver did not confirm it was revoked".

**A typo'd `LOOPKY_SESSION` came back as `session_expired` from `logout`.**
`requireSession` checks the secret's shape and `logout` is the one command that
does not go through it, so the check reached every command except the one whose
whole job is to handle that credential. Exit 9 now, as everywhere else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 4 — d8cbb84 + 41b9869, re-tested on staging

Pulled both, :cli:test / :shared:jvmTest / detektAll green, rebuilt, re-ran everything the two commits touch. Seven of the eight findings are confirmed fixed on the device. One regression, and the round-3 set is still open.

The QR commit is better than what I asked for — it found two things I did not: ImageIO.write(…, File) deletes and recreates the file (throwing away the mode set beforehand), and a finally alone does not cover the way login normally ends. Both are the kind of thing only running it surfaces.

Verified fixed, live

Evidence
S1 QR file mode stat on a live --qr-out: -rw------- 20508 r4-qr.png — was -rw-rw-r--
S1 QR file removal sent SIGINT to a waiting login (the ^C case, not the SIGTERM the commit message cites): process exits 130, file gone. The shutdown hook does cover the common path
S2 plaintext URL default login stderr: no pubkyauth:// anywhere in it. --url-only prints it under "This URL is a credential until you approve it in Ring — treat it like a password."
R2-1 stored-session heal whoami from the file now reports homeserver: ufibwbmed6…, and secrets.json on disk was rewritten with it — asked once, not per invocation
R2-1 the guard closes whoami --env production from the file path is now exit 8 environment_mismatch. In round 2 the identical command was exit 0 with a blank homeserver. This is the whole point of the guard, working end to end for the first time
R2-2 dropped column columns_dropped: 1 on the 3-column prose file, 0 on a clean 2-column one; the human channel adds "DROPPED a third column on every row…"
S4/S5/S6 SignOutOutcome reports the two halves separately, --export warns before printing, the README documents /proc/<pid>/environ beside the file store. Code-verified; S4's failure branch needs a broken network to exercise

R4-1 — MEDIUM. logout bypasses the session-shape check, so the exit-4 bug is back on the new path

$ LOOPKY_SESSION=garbage loopky logout --env staging --json
{"ok":false,"error":{"code":"session_expired","exit":4,
 "message":"…invalid secret: expected `<pubkey>:<cookie>` LOOPKY_SESSION is no longer valid;
            mint a new one with `loopky login --export`."}}
rc=4

This is finding 8 from round 1, reintroduced. 0d5707b fixed it with requireSessionSecretShape, but that function is private in Runtime.kt and reachable only from requireSession. The new logout reads LOOPKY_SESSION itself (Identity.kt:238) and goes straight to identity.revokeSession(injected), so nothing checks the shape — a parse failure comes back as an expiry, telling an agent to mint a new session when the actual problem is a typo'd environment variable.

Worth noting why it happened, because it is structural rather than careless: the fix for S3 added a second door into the injected-session path, and the validation lives on the first door. A third one will do the same.

Suggestion: make requireSessionSecretShape internal and call it from logout, or better, put it inside revokeSession/adoptSession in the repository so every caller inherits it. The check is about the value, not about who is asking.

(A well-formed but dead secret correctly returns exit 4 — "Authentication error: The provided auth request has expired or was cancelled". That one is right.)

Not tested, and why

The successful revocation path. LOOPKY_SESSION=<real secret> loopky logout would revoke the session this review has been running on, costing another QR scan. I exercised both failure branches instead. Say the word and I'll run the real one — it is the last untested branch of S3.

Still open from round 3 — import --resume

Untouched by these commits, and round 4 turned up more evidence for the first one. Deleting 59 cards one at a time from a 150-card deck took 4m42s — ~4.7s per card, against 4.7s for publishing all 150 at once. That is the same per-card write path appendMissing uses, and it confirms the cost is not linear in cards but in cards × manifest size.

  • R3-1--resume writes per card (2 homeserver writes each) instead of per chunk; measured 40.7s vs 4.3s for the same 30 cards, and worse as the deck grows. The recovery path for a session that ran out of time is ~10× slower than the attempt that ran out of time.
  • R3-2 — a typo'd --title on a --resume run silently creates a second deck; resumed: 0 is indistinguishable from a legitimate first run.
  • R3-3 — every metadata flag on a resumed run is accepted and discarded, --front-lang/--back-lang included, so Listen and Speak silently never appear.

Also checked this round, and correct

deck compact on a real two-chunk deck: 150 cards → 59 deleted → {"merges":1,"cards_moved":50,"chunks_before":2,"chunks_after":1,"complete":true} in 5.1s, and all 91 remaining cards came back intact, in order, no duplicates, no nulls. The landing-record-then-manifest-then-source ordering the architecture doc warns about holds up under a real merge.

One observation rather than a finding: deck show --json does not expose the chunk table, so compact's effect is invisible through the CLI's own verification channel except via the counts in its own result. For a command whose entire job is rearranging chunks, chunks on DeckView would let a caller check the work.


Round HIGH MEDIUM Status
1 2 5 fixed, verified
1 (security) 0 3 fixed, verified
2 0 2 fixed, verified
3 0 3 open
4 0 1 open — this comment

Still finding MEDIUMs, so the loop continues.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Two more, self-found while re-testing the round-3 fixes on the binary. Both are the S4 shape — a report claiming more than happened — so flagging them rather than letting them ride to the next round.

logout with no session answered revoked: true. signOut treated "there was nothing to revoke" as a successful revocation, so a fresh machine was told a credential had been withdrawn that never existed:

$ loopky logout --json          # before
{"ok":true,"data":{"revoked":true,"cleared_locally":true}}
$ loopky logout --json          # after
{"ok":true,"data":{"revoked":false,"cleared_locally":false}}
$ loopky logout
Not signed in — nothing to revoke.

Sign-out being idempotent is right; saying it revoked something is not. SignOutOutcome carries hadSession now and the three states read differently.

A typo'd LOOPKY_SESSION came back as session_expired from logout. The shape check I added for finding 8 lives in requireSession, and logout is the one command that does not go through it — so the check reached every command except the one whose whole job is handling that credential. Exit 9 now, like everywhere else.

25089147. 48 CLI tests, 1,296 shared, :shared:allTests and :composeApp:assembleDebug green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

…esume

**`card add` computed an ord and threw it away.** `upsertCard` assigns one from
the chunk the card lands in and ignores whatever the caller sent — so the
`existing.maxOfOrNull { it.ord } + ORD_STRIDE` here was dead weight that *looked*
meaningful, and it is exactly how the command came to report a number the
homeserver never stored. Removing it means there is no longer a second opinion
about ordering for a reader to trust. `deck create` still assigns ords, and
correctly: `publish` writes the chunks directly rather than through `upsertCard`,
so there they are used.

**`import --resume` picked arbitrarily between decks sharing a title.**
`firstOrNull` over a listing in homeserver order would append somebody's cards to
whichever came back first — a silent wrong answer, and the resume mechanism
exists precisely so a killed import does not duplicate or misplace work. It now
refuses and names the candidates, which the user can act on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 5 — 2508914 + 09645ad, and S3 proven end to end

Both pulled, suites green, rebuilt, re-tested on staging — including the branch I had held back last round. The author gave the go-ahead to burn the session, so the live one was revoked and a fresh Ring sign-in done afterwards.

No new HIGH or MEDIUM this round. One LOW, and the three --resume findings from round 3 are still open and unchanged.

Verified fixed, live

Evidence
R4-1 shape check on logout LOOPKY_SESSION=garbage loopky logoutexit 9 bad_input, was exit 4
self-found: logout with no session isolated LOOPKY_CONFIG_HOME: {"revoked":false,"cleared_locally":false}, "Not signed in — nothing to revoke"
card add ord fresh deck: echoed 0, stored 0. Dropping the dead computation is the better fix — there is no second opinion left to disagree
ambiguous resume two decks titled ZZ dup titleexit 9, naming both: "…cannot tell which one to continue: 9wzjykxj50gw, grxqxihluih2. Rename or delete one, or drop --resume"

S3 — the revocation branch, run for real

$ LOOPKY_SESSION=<the live secret> loopky logout --env staging --json
{"ok":true,"data":{"revoked":true,"cleared_locally":false}}          rc=0

$ loopky whoami --env staging --json        → "session_live": false
$ loopky deck create --title … --json       → exit 4, "Run `loopky login` again."

It genuinely revokes: the homeserver stops honouring the secret, and the next write fails with the right code and the right advice. That was the last untested branch of the credential work.

Also confirmed at its source

A fresh Ring sign-in now reports Homeserver: ufibwbmed6… on the login line itself — previously blank, which is where this whole thread started. And --env production against the new session is refused with exit 8. So the backfill is verified at all three doors now: complete(), adoptSession() and loadPersistedSession().

The --qr-out file was deleted automatically when approval landed, without a signal — the finally path, complementing the shutdown hook I tested with SIGINT last round.

R5-1 — LOW. logout on an injected session asserts something it never checks

$ LOOPKY_SESSION=<secret that is ALSO in secrets.json> loopky logout
"Revoked the session from LOOPKY_SESSION. Nothing was stored on this machine,
 so there is nothing here to clear — unset the variable too."

Nothing was stored is an assumption, not a check. In the run above the identical session was stored here — which is exactly what happens when a secret is minted with login --export on the machine it is later revoked from, the flow the README describes. So the message is false, and a dead credential is left installed in secrets.json: the next whoami still returns ok: true and only session_live: false hints at it.

The KDoc's reasoning is right — signOut would revoke the injected credential while wiping a different stored one — but it assumes the two differ. Comparing the injected secret against the stored one is one line and lets the message say which case it is in, and clear the local copy when they match.

Same family as S4 and the hadSession bug in 2508914: a report claiming more than it knows. Low because writes fail with correct advice and the next login heals it.

Still open — import --resume, all three unchanged

Re-measured on the current head:

  • R3-1 (MEDIUM) — 30 resumed cards: 40.2s, against 4.3s to publish the same 30 fresh. appendMissing still loops upsertCard, so 2 homeserver writes per card against publish's 2 per chunk. The recovery path for a session that ran out of time remains ~10× slower than the attempt that ran out of time, and worse as the deck grows.
  • R3-2 (MEDIUM) — the main case is untouched. 09645ad fixed the ambiguity between two decks sharing a title; a title matching nothing still falls through and publishes a new deck: --title "ZZ nonexistent title" --resume → deck created, written 10, resumed 0, rc 0. A typo still costs a duplicate deck, and resumed: 0 is still indistinguishable from a legitimate first run. A resume_matched field, or refusing when --resume finds no candidate, closes it the same way the ambiguity case was closed.
  • R3-3 (MEDIUM)--tag beta --description "set on resume" on a resumed run: deck came back tags: ["alpha"], description: null, rc 0. --front-lang/--back-lang go the same way, so Listen and Speak silently never appear on a deck finished by a resume.

Round HIGH MEDIUM LOW Status
1 2 5 4 fixed, verified
1 (security) 0 3 3 fixed, verified
2 0 2 0 fixed, verified
3 0 3 1 1 LOW fixed; 3 MEDIUM open
4 0 1 0 fixed, verified
5 0 0 1 this comment

First round with no new HIGH or MEDIUM. The branch is in good shape — the credential handling in particular went from six findings to none across two rounds, and the parts that were hardest to get right (the QR file's mode surviving ImageIO, the shutdown hook, revocation without clobbering a co-resident session) are all correct under test. --resume is the one area still carrying real findings.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

jvsena42 and others added 3 commits September 3, 2026 08:08
"Nothing was stored on this machine" was asserted, not checked — and it is false
in the flow the README itself describes. A secret minted with `login --export`
*is* the stored session, so revoking it left a dead credential installed here:
`whoami` still answered `ok: true`, with only `session_live: false` to hint at
it.

The reasoning for routing this through `revokeSession` rather than `signOut`
still holds — `signOut` would revoke the injected credential while wiping a
*different* stored one — but it assumed the two differ. Comparing them is one
line, and it lets the message say which case it is in and clear the local copy
when they are the same session.

Same family as S4 and the `hadSession` bug: a report claiming more than it knows.

Found in review by jvsena42 on #208 (R5-1).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
…nothing

Two of the three `--resume` findings.

**Deck metadata passed to a resumed run was accepted and dropped.** `newDeck` is
only reached on the fresh path, so `--tag`, `--description`, `--cover-url`,
`--cover-emoji`, the four study opt-ins and `--front-lang`/`--back-lang` were all
taken and ignored, with success reported. The natural way to use the feature is
to re-run the *same command* with `--resume` appended — which is what the README
shows — so an agent whose deck got half-written lost every flag but `--title`.
The language pair is the sharpest: dropped means `speechReady` stays false and
Listen and Speak never appear on a deck finished by a resume.

Only what was actually given is overlaid, which is why the opt-ins go through the
new `Args.flagOrNull` rather than `Args.flag`: absent and "explicitly false" are
the same to `Boolean`, and treating them alike would turn off every mode the deck
already had on a bare `--resume`.

**A `--resume` that matched nothing was indistinguishable from a first run.** One
transposed character in `--title` and the result is a second full deck, quota
spent twice, `resumed: 0` — exactly what a legitimate first run reports.

Reported rather than refused, which is the other option the finding offered.
Refusing would break the idiom the flag is most useful for: an agent that always
passes `--resume` so its retries are safe still has to be able to make the first
run. So `resume_matched` is a distinct field, and the human path names the decks
that do exist, which is what makes a typo visible.

Found in review by jvsena42 on #208 (R3-2, R3-3).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
`import --resume` looped `upsertCard`, and each of those is a chunk write **plus**
a full manifest read-modify-write — 60 homeserver writes for 30 cards where
`publish` spends 2. The manifest carries the whole chunk table, so the per-card
cost climbs with deck size. Measured on staging: 30 cards took 40.7s to resume
against 4.3s to publish the same 30 fresh.

That is not a performance footnote. `--resume` exists so an import killed by the
hourly session expiry (#165) can finish, and a recovery an order of magnitude
slower than the attempt that ran out of time means a large import never
finishes — each retry dies earlier in the file. The mechanism made the failure
it exists for more likely.

`DeckRepository.appendCards` is the batched path: fill the last chunk with room,
add trailing chunks, describe the lot in a **single** manifest patch — which is
what `publishLocked` has always done, via the `writeChunksAndManifestLocked` it
already had. The resume path was the only caller doing it the expensive way. The
tests assert the *number* of manifest writes, since the count was the defect.

Writing them found a second bug in the first draft. When the last chunk is full
the append target is a chunk that does not exist yet, so the tail read comes back
empty and there is nothing local to continue ords from; the fabricated floor I
had put appended cards *before* ones already in the deck, silently scrambling
study order for exactly the decks big enough to have filled a chunk. It now reads
the previous chunk — one extra read, only at the boundary.

`CardChunking.planAppend` holds the arithmetic, next to `appendTarget` where the
single-card version lives. `DeckRepositoryImpl` crossed detekt's `LargeClass`
threshold with the addition; the exclusion is named and reasoned in
`config/detekt/detekt.yml` rather than the threshold raised, and it records what
the real seam is and why taking it is a different decision.

Found in review by jvsena42 on #208 (R3-1), measured live.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 5 answered, and round 3 too — I never received it. My reply that round went out at 10:44 and your round-3 comment landed at 10:41; I read the thread from a stale fetch and missed it entirely. That is why three MEDIUMs sat open for two rounds with no acknowledgement. Sorry.

All four are fixed: 8088edc (R5-1), 8168bc6 (R3-2, R3-3), fade056 (R3-1).

R3-1 — the batched append

You were right that this is the one that matters, and the framing is the reason: the recovery being slower than the thing it recovers means a large import cannot finish, and each retry dies earlier in the file. A feature that makes its own failure mode more likely.

DeckRepository.appendCards fills the last chunk with room, adds trailing chunks, and describes the lot in one manifest patch — through writeChunksAndManifestLocked, which publishLocked already used. The resume path was genuinely the only caller doing it the expensive way, exactly as you said.

DeckRepositoryAppendTest asserts the number of manifest writes, not the result, because the count was the defect. Ten appended cards → 1 manifest write; CHUNK_SIZE * 2 cards → still 1.

Writing those tests found a second bug in my first draft, and it is worse than the one I was fixing. When the last chunk is full, the append target is a chunk that does not exist yet — so the tail read comes back empty and there is nothing local to continue ords from. My fabricated floor put appended cards before ones already in the deck, silently scrambling study order for exactly the decks big enough to have filled a chunk. It now reads the previous chunk: one extra read, only at the boundary, with a test that seeds CHUNK_SIZE cards and asserts the appended one sorts last.

R3-2 — I took your other option, deliberately

You offered refuse-with-exit-6 or report-a-field. I took the field, and want to be explicit about why rather than have it look like the cheaper choice: refusing breaks the idiom the flag is most useful for. An agent that always passes --resume so its retries are idempotent still has to be able to make the first run, and exit 6 there turns a working pattern into one that needs a conditional.

So resume_matched is a distinct field (null when --resume was not asked for), and the human path names the decks that do exist — which is what actually makes a typo visible, since the failure mode is "one character off a title you already have". If you think refusing is still right, say so and I will switch it; the information is in hand either way.

R3-3 — metadata on the resume path

Applied via updateMetadata, after the cards, so a failed append cannot have renamed the deck it failed to fill.

The subtlety worth flagging: only what was actually given is overlaid. Absent and "explicitly false" are the same to Boolean, so Args.flag would have turned off every opt-in the deck already had on a bare --resume — which would have been a worse bug than the one being fixed. Hence Args.flagOrNull and a tri-state.

R5-1

Fixed, and your reading was right: the KDoc's reasoning held but assumed the two secrets differ. Compared now, cleared when they match, and the message says which case it is in.

One thing I did that you should look at

DeckRepositoryImpl crossed detekt's LargeClass threshold with appendCards. I added a named exclusion with the reasoning written down rather than raising the threshold — and I am flagging it here rather than letting it pass in a diff, because "silence a lint that just started failing" is the shape of thing this review has been good at catching.

The exclusion records what the real seam is: the "following someone else's deck" block, ~100 lines. I did not take it because DeckRepository's own KDoc says why it lives there — listFollowed has to merge with listOwned behind one changes flow, and DiscoveryRepository already depends on this class — so splitting it is the core/presentation decision from open question 5, not a lint fix. Happy to be told that is the wrong call.

Verified

49 CLI tests, 1,304 shared (8 new), :shared:allTests, :composeApp:assembleDebug, detektAll — green.

I have not re-measured the resume timing against staging; I still have no Ring session. The write count is asserted in tests, but the 40.7s → expected ~2 writes is your measurement to confirm.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 6 — 8088edc + 8168bc6 + fade056. Clean: no HIGH or MEDIUM, and nothing carried over.

All three pulled, suites green, rebuilt, re-tested on staging. Every open finding from rounds 3 and 5 is closed and verified on the device. One LOW, noted below, and that is the whole of it.

R3-1 — batched resume, and the boundary case is right

resume 30 cards onto a 10-card deck   →  7.1s    (was 40.2s)
resume 30 cards onto a 100-card deck  →  6.0s    (the per-card path would have been ~140s)

The second one is the test worth having, because it is the bug fade056 caught in its own first draft: 100 cards fill chunk 0 exactly, so appendTarget returns a chunk that does not exist, the tail read comes back empty, and there is nothing local to continue ords from. Read back afterwards:

cards: 130          around the boundary: c098 c099 c100 c101 c102 c103
file order preserved: True          ords strictly ascending: True
ord at 99/100/101: 98000  99000  100000

highestOrdLocked reaches into the previous chunk and the appended cards land after the existing ones, not before. Study order is intact across the chunk seam — which is exactly what a fabricated floor would have silently broken, and only on decks big enough to have filled a chunk.

Timing is no longer the shape of the problem either: 6.0s to append onto a 100-card deck against 7.1s onto a 10-card one means the cost stopped tracking deck size, which was the actual defect.

R3-3 — resumed flags kept, and only the ones given

--tag beta --description "set on resume" --resume  →  tags ["beta"], description "set on resume"

And the half that is easy to get wrong, tested separately: a deck created with --listen --type --front-lang es-ES --back-lang en-US, then a bare --resume with no flags at all —

listen: True | type: True | speak: False | langs: es-ES en-US

flagOrNull holds. Absent stays absent rather than collapsing to false, so a bare resume does not quietly switch off every mode the deck had. tags.ifEmpty { this.tags } does the same for tags. That distinction is the whole difficulty of this fix and it is handled.

R3-2 — resume_matched

--title "ZZ r6 no such deck" --resume  →  resume_matched: false

Present and correct, and it is the right call not to refuse: an agent that always passes --resume so its retries are safe still has to be able to make the first run. resumed: 0 no longer has to carry two meanings.

R5-1 — code-verified, not run

loadPersistedSession()?.takeIf { it.sessionSecret == injected } before the revoke, signOut(force = true) when they match, and the message says which of the two cases it is in. Both branches reach the message only after actually revoking, so exercising either costs another sign-in; I did not spend a second one on a LOW. The logic is six lines and directly addresses the finding.

R6-1 — LOW. The new "found no deck" warning is silenced in the mode an agent runs

Main.kt:117:

val progress: (String) -> Unit = { line -> if (!args.has("json")) System.err.println(line) }

resumeState's onNote is routed through that sink, so the warning — "--resume found no deck titled … so this is publishing a NEW deck. If that was a typo, delete it and re-run. Your decks: …" — prints for a human and not under --json. Confirmed: stderr from a --json run with a non-matching title is completely empty.

resume_matched: false is in the envelope, so the machine-readable signal an agent needs is there and the finding is not that it is silent. Two smaller things:

  • 8168bc6's "But it is not silent either" is true for the human path only, and the note names the account's existing deck titles — the one piece of information that turns "no match" into "you meant this one", and it is unavailable exactly where the typo does the damage.
  • The README says "progress, the QR code, prompts and every log line go to stderr", describing stdout and stderr as separate channels rather than one that switches off. An agent capturing stderr for diagnostics under --json gets nothing at all.

Suggestion: separate a note sink from progress and let notes through under --json, or carry the candidate titles in the envelope beside resume_matched.


Where the branch stands

Round HIGH MEDIUM LOW Status
1 2 5 4 fixed, verified live
1 (security) 0 3 3 fixed, verified live
2 0 2 0 fixed, verified live
3 0 3 1 fixed, verified live
4 0 1 0 fixed, verified live
5 0 0 1 fixed, code-verified
6 0 0 1 this comment

Six rounds, 2 HIGH and 14 MEDIUM found and closed, every one of them re-tested against a real staging homeserver rather than signed off from the diff. Two rounds in a row with no MEDIUM, and nothing outstanding above LOW. From my side this is done.

Worth recording what the exercise actually turned up, because it was not evenly distributed:

  • Two defects had passing tests sitting over them. EnvironmentMismatchTest handed the guard a homeserver by hand, so a safety check that had never once fired in production use was green; and the .apkg suite drove the reader's callers rather than its opener, so a JDBC URL that was never interpolated went unnoticed. Both now have tests that go through the shape the real flow produces.
  • The environment guard had never fired. Not once, in any session the CLI could mint, until round 4. That is the finding I would keep if I could keep only one.
  • Credential handling went from six findings to zero across two rounds, and the fixes were better than the reports — ImageIO.write(…, File) silently recreating the file at the ambient umask, and ^C being the normal way login ends so a finally is not enough, were both found by running it rather than reading it.
  • The last three fixes each found a further bug while being written: the append boundary scrambling study order, logout claiming revoked: true with no session, and absent-vs-false on the resume opt-ins. That is the pattern of a change being tested rather than asserted.

The deliverable that drove all of this is still on the homeserver and reads back clean: avskolydfy2q — "Computer Networks — Ch. 1: Introduction", 48 cards, tagged networking / tanenbaum / computer-science, ords ascending, extracted from chapter 1 of Tanenbaum, Feamster & Wetherall 6e and published entirely through loopky. Which was the point: an agent made a deck without a phone screen.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

jvsena42 and others added 2 commits September 3, 2026 08:14
`updateMetadata` re-reads `chunks` and `cardCount` inside its lock but writes the
`updatedAt` it is handed — and the resume path handed it a snapshot taken
*before* the append. So describing the deck rewound the manifest's timestamp
below what the append had just set.

`hasUpdate` compares exactly that field against a follower's `last_seen_updated_at`,
so a follower who had already seen the deck would never be told the new cards
exist. The cards were on the homeserver and invisible to everyone syncing it.

Found while re-reading my own R3-3 fix rather than by a test, which is the
uncomfortable part — the sequence is append-then-describe and only the second
half carries a timestamp. `DeckRepositoryAppendTest` now pins the contract from
both sides: what a caller must not do with a stale snapshot, and what carrying
the post-append timestamp gives instead.

Also drops a `cardCount` from that call that read as if it mattered; it is
overwritten inside the lock either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
…er --json

`resumeState`'s "found no deck titled …" note went through the `progress` sink,
which is suppressed under `--json` — so the one message that turns "no match"
into "you meant *this* one", by naming the account's existing deck titles, was
withheld in exactly the mode where a typo does its damage. `resume_matched:
false` was in the envelope, so an agent was not blind, but it could not tell
which deck it meant.

The root of it is that one lambda was doing two jobs. A **progress counter** is
thousands of lines on a large import and noise in a scripted run, and the result
carries the same numbers, so suppressing it under `--json` is right. A **note**
is something the caller needs to know, and stderr is the diagnostic channel in
either mode — the README describes stdout and stderr as separate channels, not
one that switches off, and an agent capturing stderr must not get an empty file
because it asked for JSON.

Two sinks now, and the README says which is which.

Found in review by jvsena42 on #208 (R6-1).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

R6-1 fixed in a398f69. That is the last open finding.

You identified it as a routing mistake and it was worse than that — one lambda was doing two jobs, and I had not noticed they were different jobs:

  • a progress counter is thousands of lines on a large import, noise in a scripted run, and the result carries the same numbers — suppressing it under --json is right;
  • a note is something the caller needs to know, and stderr is the diagnostic channel in either mode.

Two sinks now, so this is fixed for any future note rather than just this one. Your README point was the load-bearing half of the finding: it describes stdout and stderr as separate channels, not one that switches off, and an agent capturing stderr for diagnostics should never get an empty file because it asked for JSON. The README now says which sink --json silences and which it does not.

Code-verified, not run — same position you took on R5-1, and for the same reason: the note fires after listOwned(), so reaching it needs a session I do not have. The wiring is System.err::println unconditionally against a progress that stays conditional.


On the summary: the two findings with passing tests over them are the ones I would keep too, and they were the same mistake twice — a test built on a fixture rather than on the shape the real flow produces. The environment guard never having fired in any session the CLI could mint is the sharper of the two, because it was this PR's own headline safety feature and I had written a test file for it.

The three fixes that each found a further bug while being written is the pattern I would want on the record. In every case the second bug was worse than the first: the append boundary silently scrambling study order only on decks large enough to have filled a chunk; logout claiming a revocation on a machine that had never signed in. Neither was in the report — they came out of building the fix and then running it.

Twenty findings across six rounds, every one re-tested against a real homeserver. Thank you for that; it is the half of the work I could not do, since I have never held a Ring session on this branch.

If nothing else is outstanding, please approve. CI is green on a398f69 across all three jobs — Linux x86_64 included, which is what proves the cross-built .so loads on real glibc.

Everything past sign-in on this branch rests on your runs. avskolydfy2q is the evidence that it works.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 7 — the LOW findings, and a sweep of the paths no round had touched

Asked to close out the LOWs. Every one from rounds 1–6 was re-checked against a398f69 rather than taken as fixed. Eleven of twelve are closed. The sweep that came with it turned up one new thing, and by this branch's own standard it is not a LOW.

R6-1 — fixed, verified

a398f69 splits the sinks, which is the right cut — a counter and a warning were never the same thing:

$ loopky import … --title "ZZ r7 typo probe" --resume --json   2>stderr
stdout → "resume_matched": false
stderr → --resume found no deck titled "ZZ r7 typo probe", so this is publishing a NEW deck.
         If that was a typo, delete it and re-run. Your decks: Spanish Nouns,
         Computer Networks — Ch. 1: Introduction, Spanish basics
progress lines on stderr: 0

The note gets through under --json and names the candidates; the per-chunk counter stays suppressed. That is exactly the split, and the deck-title list is the part that makes it actionable.

The rest of the LOWs, re-checked at HEAD

Finding State
#8 malformed LOOPKY_SESSION → exit 4 fixed, exit 9 — verified live in rounds 2 and 5
#9 --limit twenty silently 20 fixed, exit 2 — verified live
#10 #1 ranked swallowed as a comment fixed — verified live, # required
#11a storedAt KDoc claimed --export does not store fixed — "Always set: --export also prints the secret, it does not print it instead of storing it"
#11b loopky deck sync --compact (no such command) fixed — now loopky deck compact <deckId>
R2-2 third column silently discarded fixed, columns_dropped — verified live
S5 --export --json puts a secret on stdout unannounced fixed, warns first — source-verified
S6 LOOPKY_SESSION exposure undocumented fixed — README now names /proc/<pid>/environ, child inheritance and shell history, and notes the VAR=… cmd prefix keeps it out of other processes but not out of history
round-3 LOW: ambiguous resume target fixed, exit 9 naming both ids — verified live
R5-1 logout asserting "nothing was stored" fixed — source-verified; see below
R4 note: no chunk table on DeckView open

Two were verified by reading rather than running, and both need a condition I cannot make cheaply: S4's failure branch needs the homeserver to refuse a revoke, and R5-1's match branch reaches its message only after revoking, so testing it costs a QR scan. Both fixes are short and directly answer the finding. Happy to spend a sign-in on R5-1 if you want it on the record.

The one genuinely open item is the round-4 note, which I raised as an observation rather than a finding and still would: deck show --json has no chunks, so deck compact's effect is invisible through the verification channel except via the counts in its own result. For a command whose entire job is rearranging chunks, chunks on DeckView would let a caller check the work.

Sweep of the untested paths

deck sync, stdin, --version, an unknown verb, separator auto-detect, JSONL partial edits, and the not-found paths had never been exercised. All correct:

  • deck show nosuchdecknot_found exit 6
  • card edit <deck> zzzznotacardnot_found exit 6, and the same through --from-file JSONL
  • JSONL partial edit — {"id":…,"back":"EDITED BACK"} left the front untouched, as documented
  • printf 'uno,one\n…' | import with no --separator → auto-detected comma, 3 cards
  • stdin -, --version (loopky 0.1.0 (schema 1)), unknown verb → exit 2

R7-1 — MEDIUM. card rm cannot distinguish a delete from a no-op

The one exception in that list, and it is the same defect this branch has now fixed three times.

$ loopky card rm <deck> 52r9kspqktyl        # a real card
{"deck_id":"lk9kdwzqw6uo","cards":[],"written":0,"skipped":0,"card_count":9}   rc=0

$ loopky card rm <deck> zzzznotacard        # no such card
{"deck_id":"lk9kdwzqw6uo","cards":[],"written":0,"skipped":0,"card_count":9}   rc=0

Byte-identical. One removed a card, the other did nothing at all, and nothing in the envelope or the exit code separates them. DeckRepositoryImpl.deleteCard does locateChunk(deck, cardId) ?: return@withDeckWrite deck — a missing card is a silent no-op — and the CLI reports it as success.

Three reasons this is not a LOW:

  1. The surface already disagrees with itself. card edit on that exact id returns not_found exit 6, through both the flag and the JSONL path. deck show on a missing deck returns exit 6. card rm is the only not-found path in the CLI that reports success, so an agent that has learned exit 6 from every other command will be wrong about this one specifically.
  2. It is the class the branch keeps fixing. S4 (signOut claiming success when the revoke failed), hadSession (logout claiming revoked: true with no session), R5-1 (logout claiming nothing was stored) — all "a report that claims more than happened", all fixed. This is the same shape on the delete path, and Output.kt's own KDoc is that "a caller diffs intent against result from these bytes".
  3. card_count is not a workaround. It only answers if the caller already knew the count, and an agent pruning a list of ids — the obvious use — would have to read the deck between every delete to tell which of its ids were real.

Idempotent delete is a defensible repository choice; reporting it identically at the CLI boundary is the part that misleads. The information is already in hand — locateChunk returning null is the answer.

Suggestion: CardWriteResult gains a removed count (or card rm returns exit 6 when nothing matched, matching card edit). Echoing the removed card in cards would also make the delete diffable the way every other write on this surface is.


Round HIGH MEDIUM LOW
1 2 5 4
1 (security) 0 3 3
2 0 2 0
3 0 3 1
4 0 1 0
5 0 0 1
6 0 0 1
7 0 1 0
total 2 15 10

Every LOW closed but the DeckView chunk-table note, and one new MEDIUM on card rm. Nothing else in the sweep misbehaved.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

…nk table

**`card rm` answered identically whether it deleted a card or did nothing.** Same
envelope, same exit 0, for a real id and a nonexistent one — so an agent pruning
a list of ids could not tell which of them were real without re-reading the deck
between every delete. `deleteCard` treats a missing card as a no-op and the CLI
reported that as success.

Fixed with a `removed` count rather than an exit code, and the asymmetry with
`card edit` — which does return `not_found` — is the deliberate part. Removing a
card that is already gone leaves the deck in the state the caller asked for, so
failing it would break the retry-after-expiry pattern the whole surface is built
around; that is the same reason `card add` reports `skipped` instead of refusing
a duplicate. An *edit* has no such reading: the change cannot be applied at all.
So: add and rm are idempotent and say what they did, edit is not and says why.

Knowing the count takes one manifest read before the delete. That is one record,
not the deck's cards — `cardCount` comes from the chunk table.

**`DeckView` now carries that chunk table.** `deck compact`'s entire job is
rearranging it, and without it the effect was invisible through the verification
channel except as counts in compaction's own result: a caller could only take the
command's word for the work. `deck compact` re-reads the deck afterwards and
returns it. `chunks[].updated_at` is also what a follower diffs to re-fetch one
record rather than a whole deck.

The numbering is **not contiguous** — compaction folds a pair and drops the
higher `n` — and there is a test that a gap survives the view, because renumbering
into a dense list is the obvious way to get this wrong.

Found in review by jvsena42 on #208 (R7-1, and the round-4 note).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Both closed in 9c04925. That is every finding across seven rounds.

R7-1 — fixed, but I took the removed count over exit 6, deliberately

You offered either and I want to argue for the one I picked rather than have it look like the cheaper option.

Failing a delete of an already-absent card would break the retry pattern the whole surface is built around. An agent whose session dies mid-prune re-runs the command; if card rm returns exit 6 for the ids it already removed, the retry fails on its own prior success. That is the same reason card add reports skipped rather than refusing a duplicate — a decision this branch made in the first commit and has not revisited.

So the model is now coherent rather than merely consistent:

already in the desired state reports
card add card already present skipped, exit 0
card rm card already absent removed: 0, exit 0
card edit not_found, exit 6

edit is the odd one out correctly: there is no reading under which editing a card that does not exist leaves the deck as asked. Add and rm are idempotent and say what they did; edit is not and says why. Your finding was that card rm said nothing, and that part was exactly right — the information was in hand and thrown away.

It costs one manifest read before the delete. That is one record rather than the deck's cards, since cardCount comes from the chunk table.

The round-4 note — chunks on DeckView

You were right to keep raising it. A command whose entire job is rearranging the chunk table, reporting its effect only as counts in its own result, is the one place on this surface where a caller has to take the command's word — which is the opposite of what --json is for. deck show and deck compact both carry the table now, and compaction re-reads the deck afterwards so the result is the check.

One thing worth flagging since it is a trap I nearly walked into: the numbering is not contiguous. Compaction folds a pair of neighbours and drops the higher n, so anything walking it must read n rather than 0 until size. There is a test that a gap survives the view, because renumbering into a dense list is the obvious way to get this wrong and it would look correct.

On S4 and R5-1

Do not spend a sign-in on R5-1 on my account — you have already spent two, and a LOW whose logic is six lines is not worth a third. If you want one of them on the record I would rather it were S4, the failed-revoke branch: it is the one where the user is told "Signed out" while the token is still live, and it is the only untested path that could still mislead someone about a credential.

Verified

52 CLI tests, 1,304 shared, :shared:allTests, :composeApp:assembleDebug, detektAll — green. CI green on the previous head across all three jobs and running on this one.


Seven rounds, 2 HIGH / 16 MEDIUM / 10 LOW, every one of them re-tested against a real homeserver. The three findings I would keep: the environment guard that had never fired in any session the CLI could mint, and the two defects with passing tests sitting over them — both the same mistake, a test built on a fixture rather than on the shape the real flow produces.

Please approve if nothing is outstanding.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Round 8 — clean. Nothing open.

9c04925 closes the last two, verified on the device just now:

card rm <real id>         → "removed": 1
card rm <nonexistent id>  → "removed": 0
deck show --json          → "chunks": [{"n": 0, "count": 9, "updated_at": 1788435227292}]

The asymmetry argued for in that commit message is the right call, and worth keeping the reasoning for: add and rm are idempotent and report what they did, edit is not and says why. Failing a delete of an already-absent card would have broken the retry-after-expiry pattern the whole surface is built around — the same reason card add reports skipped rather than refusing a duplicate. I had suggested exit 6 as one option; the removed count is the better of the two.

This is my sign-off. Seven rounds, all against a live staging homeserver.

(An approving review is not possible here — the only account on this machine is the one that opened the PR, and GitHub refuses Can not approve your own pull request. Treat this comment as the approval.)

Final tally

HIGH MEDIUM LOW
Round 1 2 5 4
Round 1 (security) 0 3 3
Round 2 0 2 0
Round 3 0 3 1
Round 4 0 1 0
Round 5 0 0 1
Round 6 0 0 1
Round 7 0 1 0
Round 8 0 0 0
total 2 15 10

All fixed. Every one re-tested against a real homeserver rather than signed off from the diff.

What I would keep from the exercise

  • The environment guard had never fired. Not once, in any session loopky login could produce, because the Ring path stored a blank homeserver — and EnvironmentMismatchTest was green throughout, because it handed the function a homeserver by hand. A safety check with a passing test over it that had never run in anger. Same shape as the .apkg reader, whose JDBC URL was never interpolated while the suite drove its callers rather than its opener. Both now have tests that go through what the real flow produces, and both were confirmed to fail on the old code first.
  • Credential handling went from six findings to zero across two rounds, and the fixes were consistently better than the reports. ImageIO.write(…, File) silently recreating the file at the ambient umask, and ^C being the normal way login ends so a finally cannot be enough, were both found by running it rather than reading it.
  • Five of the last six fixes found a further bug while being written — the append boundary scrambling study order, logout claiming revoked: true with no session, absent-vs-false on the resume opt-ins. That is the signature of changes being tested rather than asserted, and it is why the later rounds got shorter.
  • --resume was the sharpest area, because it is the path least likely to be tried by hand and the one that only runs after something already went wrong. It was ~10× slower than the publish it was recovering, on the same one-hour session budget — a recovery mechanism that made the failure it existed for more likely on the retry.

Two corners still untested, deliberately

Both need a condition that cannot be created cheaply, and both fixes are short and directly answer their finding:

  • S4signOut reporting a failed remote revoke needs the homeserver to refuse one.
  • R5-1logout's "same session" branch reaches its message only after revoking, so exercising it costs a sign-in.

Worth a line in journeys/RESULTS.md naming them as the untested corners, so the next person does not assume the whole surface has been driven.

The deliverable

avskolydfy2q — "Computer Networks — Ch. 1: Introduction", 48 cards, tagged networking / tanenbaum / computer-science, ords ascending, read back identical to what went in. Extracted from chapter 1 of Tanenbaum, Feamster & Wetherall 6e and published end to end through loopky — no phone, no screen, one command. Which was the point of the branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TfCTSzb4MWBYqzNyUuyv3

Eight rounds against a live staging homeserver, 2 HIGH / 15 MEDIUM / 10 LOW
closed. The round-by-round detail lives on #208; what belongs in RESULTS.md is
what a later reader needs.

Two things worth keeping. **Two defects had passing tests over them, and it was
the same mistake twice** — a test built on a fixture rather than on the shape the
real flow produces. That cost a safety check which had never once fired, and an
`.apkg` reader that had never opened a file. The rule that falls out of it is
written down: test a check through the path that mints the value.

And the **untested corners are named**, at the reviewer's suggestion, so nobody
reads the rest of the file as meaning the whole surface has been driven. Both
need a condition that cannot be made cheaply: a homeserver that refuses a revoke,
and a fresh sign-in to reach `logout`'s same-session branch. The first is the one
path left that could still tell someone "Signed out" while their token is live.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ
@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Taken as the approval — thank you. Eight rounds against a real homeserver is the half of this I could not do, and it found things a green build was never going to.

Your last suggestion is in as e2fba0c: journeys/RESULTS.md now names S4 and R5-1 as the deliberately untested corners, so nobody reads that file as meaning the whole surface has been driven. It also records the one lesson worth carrying rather than the round-by-round detail — that both defects with passing tests over them were the same mistake, a test built on a fixture rather than on the shape the real flow produces, and that the rule which falls out of it is to test a check through the path that mints the value.

The branch is ready to merge. 9c04925 is green on all three CI jobs and e2fba0c is docs only.

What is landing, and what is not:

  • The jvm() target, the JVM platform module, and loopky itself — the Linux x86_64 row, verified in CI on real glibc.
  • Everything past sign-in rests entirely on your runs. I have never held a Ring session on this branch, so deck create, import, card add/edit/rm and deck sync/compact against a live account are yours, not mine.
  • Packaging is still a jar plus a JRE 17, which is honestly short of what a one-line sandbox install needs. native-image/jlink and the container image are the remaining work, recorded in Architecture.md §13.10 rather than left implied.
  • loopky: tell the user a new version exists, and give them one command to get it #209 covers the version check and loopky update, blocked on there being releases at all.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

@jvsena42

jvsena42 commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Follow-ups filed, so the open work survives #54 closing rather than living in a comment thread.

#210 Packaging: ship a binary, not a jar The one thing #54 called "the primary constraint" that did not land. What ships needs a JRE 17, which is exactly what a one-line sandbox install cannot be. native-image with JNA's library extraction as the sharp edge, jlink as fallback, plus the container image. Also covers the tarball carrying both native rows, and refusing an Intel Mac with a message instead of a transport error. Prerequisite for #209 — a self-updater for a jar-plus-script replaces a directory, a binary replaces a file.
#211 .apkg import in the CLI #208 built a working desktop reader and no way to call it: JvmApkgReader is real and tested, and loopky --help has no entry point. #46 calls bulk Anki import the most CLI-shaped job there is, and the hard half already exists and is shared with the app. The CLI-specific part is the field picker headlessly — --dry-run --json returning fieldNames/fieldSamples before anything is written — and getting ApkgDropped's counts into the envelope, which is most of why doing it on a CLI is worth anything (#96).
#212 Can it run behind an allowlist proxy? Research with a decision at the end, and it decides whether cloud sandboxes are a supported target at all. The homeserver is not a fixed name and pkarr's DHT path is UDP. Either document the host list or say the container image is the only story — silence is the one unacceptable answer, because the CLI currently implies that audience works. Testable empirically, and the failure to look for is a timeout rather than an error.
#213 macOS Keychain, and the Intel Mac refusal The macOS row shipped with the Linux row's file store. That argument was about headless boxes with no libsecret and does not transfer to the row with a human at the keyboard. SecureSessionStore is the seam, so it is a DI binding rather than a change to anything above it.
#214 A remote MCP server Kept separate deliberately: chat-only agents have no shell and no egress, so this is a different audience rather than a later evolution of the CLI. #208's command layer was shaped to be wrapped — one function per command, --json shapes as return values, no logic in the parser — and that held up over eight rounds. The one thing a per-invocation CLI structurally cannot do is renew a session before it expires (#165), which is a capability rather than a convenience.

I did not file the core / presentation split. Its trigger is the first out-of-tree consumer, which has not happened, and the reasoning is already in Architecture.md §13.12 and in the LargeClass exclusion's comment — filing it now would be a placeholder rather than a task.

Windows stays out of scope by decision, recorded in #54 and §13.10.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BXjHov9XSqWtJg8AiqKpBQ

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.

1 participant