Wizard: no dead ends after cloning a store without its key #16

claude-bot · 2026-10-13T18:52:54Z

claude-bot commented

2026-10-13 18:52:54 +00:00

What

Closes #9. The wizard could already import a key for a cloned store, but two paths still trapped the user:

The "import its key" step had no way out. It now has Start over, which deletes the local clone only if origin had everything in it at the last sync. Anything else keeps the store and says why: unpushed or uncommitted (even ignored) files, local-only refs, or no remote.
A clone without .gpg-id (e.g. an empty repo) led to the import step with no recipients. The wizard now asks you to choose a key and sets the store up in place. The same happens if the app is restarted in that state.

Blast radius

Tier: trunk
Trunk paths touched: passcore/src/lib.rs (Store::has_files), passcore/src/sync.rs (sync::discard), src/bridge.rs (has_files, discard_store), src/session.rs (try_repo_lock)
Gate / rollback: No existing behaviour changes. discard and has_files are new, and discard runs only after the user confirms Start over. To undo, revert this merge commit or reinstall the previous .click. manifest.json stays at the unreleased 0.1.0; the version bump belongs to the release steps (#12).

Proof

CI: QMAKE=qmake-qt5 cargo make (fmt, clippy -D warnings, coverage, release check, audit) gives test result: ok. 75 passed; 0 failed, warning: 2 allowed warnings found (the existing allowlist) and Build Done.
QML: cargo make qml-test gives Totals: 81 passed, 0 failed. The new tst_SetupWizard.qml covers all wizard paths and the Start over dialog against a fake store.
Real temp store: discards_only_what_the_remote_has keeps the store in each of these cases:
- an unpushed commit
- an uncommitted file or an ignored file
- another branch, a note, or another remote's ref
- a stash
- a tag on an unpushed commit
- no repo, or no remote
It discards a synced store, tags on pushed commits and refs/touchpass/checked included. It also discards an empty-repo clone, retries a leftover only when it goes ahead, and is idempotent.
Real temp store: sets_up_a_clone_of_an_empty_repository clones an empty bare repo, then runs init, writes and syncs (pushed 1). A second clone decrypts the entry.
Fails without the change: I broke each check on purpose and re-ran the tests. All 18 mutations make a test fail:
- passcore (14): the ahead check, the uncommitted check, the no-remote check, has_files, the branch/stash checks, the local-ref check, include_ignored, the tag reachability check, the checked ref allowance, the stash ref, the origin-only remotes, the leftover cleanup, and cleanup before the checks.
- QML (4): the no-gpg-id branch, resume on start, the dialog ignoring the error, and Start over keeping needKeyOnly.
UI: I grabbed the needKeyOnly step (Start over plus explanation) and needGpgId step 1 under Xvfb in the clickable image with QtTest grabImage and checked both by eye. They are not attached here. Please look at both screens on a device before merging.

Review

Reviewer: fresh-context code-review at high on the full branch, then medium on each of the 4 follow-up commits. That gave 30 findings: 13 fixed and 17 declined or refuted with evidence. Examples of declines:

the clone atomically moves .partial into place, so a store folder without a repo is unreachable
the QML tests that were called flaky pass 81 of 81 in the clickable image
forcing delete failures needs permission tricks that root bypasses

Review-Confidence: 70/100. Trunk was reviewed at high and every finding was fixed or refuted, with strong mutation evidence. The two untested paths are the "busy syncing" refusal and the delete-failure handling; check those by hand.

Confidence by part

passcore/src/sync.rs discard / has_local_only_refs: high. Real-repo tests plus a mutation for every check.
qml/SetupWizard.qml flow: high. 6 wizard tests and 4 mutations.
src/bridge.rs discard_store "busy syncing" (try_repo_lock): medium. Not exercised; read it.
discard's delete-failure arms ("cannot remove …", the ignored final delete error): medium. Not exercised; read them.
qml/StartOverDialog.qml look: medium. Xvfb only, not seen on a device.

🤖 Generated with Claude Code

## What Closes #9. The wizard could already import a key for a cloned store, but two paths still trapped the user: - **The "import its key" step had no way out.** It now has **Start over**, which deletes the local clone only if `origin` had everything in it at the last sync. Anything else keeps the store and says why: unpushed or uncommitted (even ignored) files, local-only refs, or no remote. - **A clone without `.gpg-id` (e.g. an empty repo) led to the import step with no recipients.** The wizard now asks you to choose a key and sets the store up in place. The same happens if the app is restarted in that state. ## Blast radius **Tier:** trunk **Trunk paths touched:** `passcore/src/lib.rs` (`Store::has_files`), `passcore/src/sync.rs` (`sync::discard`), `src/bridge.rs` (`has_files`, `discard_store`), `src/session.rs` (`try_repo_lock`) **Gate / rollback:** No existing behaviour changes. `discard` and `has_files` are new, and `discard` runs only after the user confirms Start over. To undo, revert this merge commit or reinstall the previous `.click`. `manifest.json` stays at the unreleased 0.1.0; the version bump belongs to the release steps (#12). ## Proof - **CI:** `QMAKE=qmake-qt5 cargo make` (fmt, clippy `-D warnings`, coverage, release check, audit) gives `test result: ok. 75 passed; 0 failed`, `warning: 2 allowed warnings found` (the existing allowlist) and `Build Done`. - **QML:** `cargo make qml-test` gives `Totals: 81 passed, 0 failed`. The new `tst_SetupWizard.qml` covers all wizard paths and the Start over dialog against a fake store. - **Real temp store:** `discards_only_what_the_remote_has` keeps the store in each of these cases: - an unpushed commit - an uncommitted file or an ignored file - another branch, a note, or another remote's ref - a stash - a tag on an unpushed commit - no repo, or no remote It discards a synced store, tags on pushed commits and `refs/touchpass/checked` included. It also discards an empty-repo clone, retries a leftover only when it goes ahead, and is idempotent. - **Real temp store:** `sets_up_a_clone_of_an_empty_repository` clones an empty bare repo, then runs `init`, writes and syncs (pushed 1). A second clone decrypts the entry. - **Fails without the change:** I broke each check on purpose and re-ran the tests. All 18 mutations make a test fail: - **passcore (14):** the ahead check, the uncommitted check, the no-remote check, `has_files`, the branch/stash checks, the local-ref check, `include_ignored`, the tag reachability check, the `checked` ref allowance, the stash ref, the `origin`-only remotes, the leftover cleanup, and cleanup before the checks. - **QML (4):** the no-gpg-id branch, resume on start, the dialog ignoring the error, and Start over keeping `needKeyOnly`. - **UI:** I grabbed the needKeyOnly step (Start over plus explanation) and needGpgId step 1 under Xvfb in the clickable image with QtTest `grabImage` and checked both by eye. They are not attached here. Please look at both screens on a device before merging. ## Review **Reviewer:** fresh-context `code-review` at high on the full branch, then medium on each of the 4 follow-up commits. That gave 30 findings: 13 fixed and 17 declined or refuted with evidence. Examples of declines: - the clone atomically moves `.partial` into place, so a store folder without a repo is unreachable - the QML tests that were called flaky pass 81 of 81 in the clickable image - forcing delete failures needs permission tricks that root bypasses **Review-Confidence:** 70/100. Trunk was reviewed at high and every finding was fixed or refuted, with strong mutation evidence. The two untested paths are the "busy syncing" refusal and the delete-failure handling; check those by hand. ## Confidence by part - `passcore/src/sync.rs` `discard` / `has_local_only_refs`: **high**. Real-repo tests plus a mutation for every check. - `qml/SetupWizard.qml` flow: **high**. 6 wizard tests and 4 mutations. - `src/bridge.rs` `discard_store` "busy syncing" (`try_repo_lock`): **medium**. Not exercised; read it. - `discard`'s delete-failure arms ("cannot remove …", the ignored final delete error): **medium**. Not exercised; read them. - `qml/StartOverDialog.qml` look: **medium**. Xvfb only, not seen on a device. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 5 commits

2026-10-13 18:52:54 +00:00

Wizard: no dead ends after cloning a store without its key 8294dc7745

Cloning a repository that has no .gpg-id (e.g. an empty one) used to land
on "import the key" with nothing to match; it now goes to choosing a key
and sets the store up in the clone, also when the app was closed in
between. The import-only step, and that one, can now start over: a
confirmed sync::discard deletes the store, refused unless origin has every
commit and nothing is uncommitted.

Refs #9

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Start over: keep stores with other branches or stashes; never block the UI on a sync 8f3b259c10

Review follow-up for #9: discard compared only the synced branch with the
remote, and waited for the repo lock on the UI thread.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Start over: keep any ref, ignored file or half-deleted store the remote lacks 1bbb664725

Review follow-up for #9. discard now refuses when a ref other than the
synced branch, the remote's refs and TouchPass's own checked marker exists
(branches, stashes, notes, tags on commits the remote lacks), and when
ignored files exist. It moves the store aside before deleting it, so a
failed delete never leaves half a store for the wizard to resume. The
dialog no longer promises the remote has everything before it is checked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Start over: trust only origin's refs; a failed delete still counts 1d16f17e74

Review follow-up for #9. Refs of other remotes now keep the store. Once
the store is moved aside it is gone from its place, so a failed delete of
the moved copy no longer reports an error; the next discard retries it.
The leftover's name keeps the store folder's full name. The dialog says
the remote is checked as of the last sync.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Start over: touch a leftover only once the discard goes ahead

CI / ci (pull_request) Successful in 3m34s

Details

CI / audit (pull_request) Successful in 10s

Details

CI / qml (pull_request) Successful in 24s

Details

bf66f5bc8a

Review follow-up for #9. The leftover of an earlier failed delete was
removed before the checks, so a refused discard still deleted it; and one
that couldn't be removed made the rename fail with a bare io error. It is
now retried just before the rename, and one that can't be removed keeps
the store with a message naming it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review-Confidence: 70
Review-Tier: trunk

schaefera reviewed

2026-10-13 18:58:42 +00:00

passcore/src/sync.rs

					
				@ -94,0 +98,4 @@

				/// no remote, unpushed commits, refs only here, or uncommitted (even ignored)

				/// files keep it.

				pub fn discard(store: &Store) -> Result<(), PasscoreError> {

				    let keep = |why: &str| {

This doesn't look particularly reuseable from here. If there are other places where such a utility conversion function could be used, extract it and give it test coverage. Otherwise, leave it for now.

claude-bot referenced this pull request

2026-10-13 19:19:06 +00:00

Scripted app flows: drive the full app on the desktop, like Playwright #17

claude-bot referenced this pull request

2026-10-13 19:21:28 +00:00

Scripted app flows: drive the full app on the desktop, like Playwright #17

claude-bot commented

2026-10-13 20:07:31 +00:00

Re the line comment on passcore/src/sync.rs:101 (the keep closure): I checked for reuse, and there is none outside discard. The "the password store was kept: …" error is built only in this function: by keep, and once inline at line 144 for the "cannot remove the leftover" case. The other PasscoreError::InvalidInput sites (in git.rs, entry.rs, keyring.rs, generator.rs and lib.rs) each write their own message inline, with nothing shared to factor out. So I'm leaving it in place as you suggested, with no extraction or new tests.

Re the line comment on `passcore/src/sync.rs:101` (the `keep` closure): I checked for reuse, and there is none outside `discard`. The "the password store was kept: …" error is built only in this function: by `keep`, and once inline at line 144 for the "cannot remove the leftover" case. The other `PasscoreError::InvalidInput` sites (in `git.rs`, `entry.rs`, `keyring.rs`, `generator.rs` and `lib.rs`) each write their own message inline, with nothing shared to factor out. So I'm leaving it in place as you suggested, with no extraction or new tests.

👍 1

schaefera approved these changes

2026-10-13 20:07:53 +00:00

claude-bot changed title from ~~WIP: Wizard: no dead ends after cloning a store without its key~~ to Wizard: no dead ends after cloning a store without its key

2026-10-13 20:08:10 +00:00

schaefera merged commit 34d400a346 into main

2026-10-13 20:08:22 +00:00

schaefera referenced this pull request from a commit

2026-10-13 20:08:22 +00:00

Merge pull request 'Wizard: no dead ends after cloning a store without its key' (#16) from wizard-missing-key into main

Sign in to join this conversation.

No reviewers