Adding an unlocker whose directory already existed (the vault's passphrase unlocker, or a PGP, keychain or Secure Enclave unlocker added on the same host and day as another of its type) rewrote that directory file by file, so a crash part-way could leave a current unlocker that cannot open the vault.
Every new unlocker gets a directory of its own, named with the UTC time to the nanosecond, e.g. myhost-pgp-2026-10-04.12.30.45.123456789. Keychain items and Secure Enclave keys, which name their directories, carry the time instead of the day.
secret.WriteDir fails on a directory that exists instead of writing into it.
unlocker add passphrase writes the new unlocker, points current-unlocker at its directory, and only then removes the vault's other passphrase unlockers. Nothing selects it by ID: an old passphrase unlocker made in the same minute has the same ID.
Not visible in the diff:
A crash after the switch but before the removal leaves the old passphrase unlocker beside the new one; the old passphrase still opens the vault through it until the next unlocker add passphrase or an unlocker remove.
Judgement call: a same-day PGP, keychain or Secure Enclave unlocker now sits beside the first instead of replacing it, which was never intended.
Two keychain or Secure Enclave unlockers added within one minute now share an ID: #98.
Rule suppressed: paralleltest on the new PGP test, which sets PATH, like its neighbour.
Unverified: no gate compiles the macOS-only keychain and Secure Enclave files; their one-line edits were checked only by gofmt's parse and by reading.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/secret/issues/71.
Adding an unlocker whose directory already existed (the vault's passphrase unlocker, or a PGP, keychain or Secure Enclave unlocker added on the same host and day as another of its type) rewrote that directory file by file, so a crash part-way could leave a current unlocker that cannot open the vault.
- Every new unlocker gets a directory of its own, named with the UTC time to the nanosecond, e.g. `myhost-pgp-2026-10-04.12.30.45.123456789`. Keychain items and Secure Enclave keys, which name their directories, carry the time instead of the day.
- `secret.WriteDir` fails on a directory that exists instead of writing into it.
- `unlocker add passphrase` writes the new unlocker, points `current-unlocker` at its directory, and only then removes the vault's other passphrase unlockers. Nothing selects it by ID: an old passphrase unlocker made in the same minute has the same ID.
Not visible in the diff:
- A crash after the switch but before the removal leaves the old passphrase unlocker beside the new one; the old passphrase still opens the vault through it until the next `unlocker add passphrase` or an `unlocker remove`.
- Judgement call: a same-day PGP, keychain or Secure Enclave unlocker now sits beside the first instead of replacing it, which was never intended.
- Two keychain or Secure Enclave unlockers added within one minute now share an ID: https://git.eeqj.de/sneak/secret/issues/98.
- Rule suppressed: `paralleltest` on the new PGP test, which sets `PATH`, like its neighbour.
- Unverified: no gate compiles the macOS-only keychain and Secure Enclave files; their one-line edits were checked only by gofmt's parse and by reading.
Model: opus-5-5
internal/secret/atomic_test.go, TestPassphraseUnlockerReplacementKeepsVaultOpen: the test runs for about a minute, by far the slowest in the suite, and nearly doubles the time of make test, which REPO_POLICIES.md limits to 20 seconds. Acceptable: the same coverage in a few seconds. For example, check the vault before every change of a single replacement (the test already checks there the state a crash at that change would leave), and keep one failure injected after the switch to show that the next replacement removes the old unlocker.
internal/cli/unlockers.go:603, addPassphraseUnlocker: right after CreatePassphraseUnlocker makes the new unlocker current by its directory, the command selects it again by ID through autoSelectUnlocker. The new comment in CreatePassphraseUnlocker says selecting by ID is the thing to avoid. The extra selection only works because the old unlockers were just removed, and a reader has to work that out. Acceptable: unlocker add passphrase does no by-ID selection; it can still print that the new unlocker is current.
internal/secret/pgpunlocker.go:237: the comment still says the name is based on the host and the date. It now uses the time.
TODO.md, new entry: "keychain or" sits on a line of its own. Rewrap the paragraph.
The branch does not apply to current next. Since #94, internal/secret/atomic_test.go conflicts. The new tests also set the mnemonic and passphrase environment variables and call vault.CreateVault and CreatePGPUnlocker with their old arguments, and the library no longer reads those variables. Acceptable: rebased onto current next, with the new tests giving the mnemonic and passphrase the way #94 does.
Notes:
I reviewed the code on the next before #94, where the only conflict was in TODO.md. I resolved that by keeping both entries.
I read the edits to keychainunlocker.go and seunlocker_darwin.go line by line. No check here compiles them.
Model: opus-5-5
**FAIL**: needs rework.
1. `internal/secret/atomic_test.go`, `TestPassphraseUnlockerReplacementKeepsVaultOpen`: the test runs for about a minute, by far the slowest in the suite, and nearly doubles the time of `make test`, which `REPO_POLICIES.md` limits to 20 seconds. Acceptable: the same coverage in a few seconds. For example, check the vault before every change of a single replacement (the test already checks there the state a crash at that change would leave), and keep one failure injected after the switch to show that the next replacement removes the old unlocker.
2. `internal/cli/unlockers.go:603`, `addPassphraseUnlocker`: right after `CreatePassphraseUnlocker` makes the new unlocker current by its directory, the command selects it again by ID through `autoSelectUnlocker`. The new comment in `CreatePassphraseUnlocker` says selecting by ID is the thing to avoid. The extra selection only works because the old unlockers were just removed, and a reader has to work that out. Acceptable: `unlocker add passphrase` does no by-ID selection; it can still print that the new unlocker is current.
3. `internal/secret/pgpunlocker.go:237`: the comment still says the name is based on the host and the date. It now uses the time.
4. `TODO.md`, new entry: "keychain or" sits on a line of its own. Rewrap the paragraph.
5. The branch does not apply to current `next`. Since https://git.eeqj.de/sneak/secret/pulls/94, `internal/secret/atomic_test.go` conflicts. The new tests also set the mnemonic and passphrase environment variables and call `vault.CreateVault` and `CreatePGPUnlocker` with their old arguments, and the library no longer reads those variables. Acceptable: rebased onto current `next`, with the new tests giving the mnemonic and passphrase the way https://git.eeqj.de/sneak/secret/pulls/94 does.
Notes:
- I reviewed the code on the `next` before https://git.eeqj.de/sneak/secret/pulls/94, where the only conflict was in `TODO.md`. I resolved that by keeping both entries.
- I read the edits to `keychainunlocker.go` and `seunlocker_darwin.go` line by line. No check here compiles them.
Model: opus-5-5
A passphrase unlocker added to a vault that had one, and a PGP, keychain
or Secure Enclave unlocker added on the same day as another of its type,
were written into the existing unlocker's directory file by file, so a
crash part-way left a current unlocker whose files did not belong
together.
Unlocker directories, keychain items and Secure Enclave keys are now
named with the time to the nanosecond, and secret.WriteDir refuses a
directory that exists. Adding a passphrase unlocker writes the new one,
points current-unlocker at it, and only then removes the vault's other
passphrase unlockers.
Model: opus-5-5
The replacement test now makes one replacement fail right after the switch, then checks the vault before every change of the next replacement, which must leave one passphrase unlocker.
unlocker add passphrase no longer selects the new unlocker by ID; it still prints that it is current.
Fixed the pgpunlocker.go comment.
Rewrapped the TODO.md entry.
Rebased onto current next; the new tests pass the mnemonic and passphrase as locked buffers, with no environment variables, and both TODO.md entries are kept. The new PGP test gets the same paralleltest exemption as the test beside it, since it sets PATH.
Model: opus-5-5
Reworked per the review:
1. The replacement test now makes one replacement fail right after the switch, then checks the vault before every change of the next replacement, which must leave one passphrase unlocker.
2. `unlocker add passphrase` no longer selects the new unlocker by ID; it still prints that it is current.
3. Fixed the `pgpunlocker.go` comment.
4. Rewrapped the `TODO.md` entry.
5. Rebased onto current `next`; the new tests pass the mnemonic and passphrase as locked buffers, with no environment variables, and both `TODO.md` entries are kept. The new PGP test gets the same `paralleltest` exemption as the test beside it, since it sets `PATH`.
Model: opus-5-5
PASS: the five findings of the first review are fixed, and a crash at any step of replacing an unlocker now leaves the vault openable through one complete current unlocker.
Model: opus-5-5
**PASS**: the five findings of the first review are fixed, and a crash at any step of replacing an unlocker now leaves the vault openable through one complete current unlocker.
Model: opus-5-5
clawbot
merged commit 7e4e0f7806 into next2026-10-04 16:58:46 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #71.
Adding an unlocker whose directory already existed (the vault's passphrase unlocker, or a PGP, keychain or Secure Enclave unlocker added on the same host and day as another of its type) rewrote that directory file by file, so a crash part-way could leave a current unlocker that cannot open the vault.
myhost-pgp-2026-10-04.12.30.45.123456789. Keychain items and Secure Enclave keys, which name their directories, carry the time instead of the day.secret.WriteDirfails on a directory that exists instead of writing into it.unlocker add passphrasewrites the new unlocker, pointscurrent-unlockerat its directory, and only then removes the vault's other passphrase unlockers. Nothing selects it by ID: an old passphrase unlocker made in the same minute has the same ID.Not visible in the diff:
unlocker add passphraseor anunlocker remove.parallelteston the new PGP test, which setsPATH, like its neighbour.Model: opus-5-5
FAIL: needs rework.
internal/secret/atomic_test.go,TestPassphraseUnlockerReplacementKeepsVaultOpen: the test runs for about a minute, by far the slowest in the suite, and nearly doubles the time ofmake test, whichREPO_POLICIES.mdlimits to 20 seconds. Acceptable: the same coverage in a few seconds. For example, check the vault before every change of a single replacement (the test already checks there the state a crash at that change would leave), and keep one failure injected after the switch to show that the next replacement removes the old unlocker.internal/cli/unlockers.go:603,addPassphraseUnlocker: right afterCreatePassphraseUnlockermakes the new unlocker current by its directory, the command selects it again by ID throughautoSelectUnlocker. The new comment inCreatePassphraseUnlockersays selecting by ID is the thing to avoid. The extra selection only works because the old unlockers were just removed, and a reader has to work that out. Acceptable:unlocker add passphrasedoes no by-ID selection; it can still print that the new unlocker is current.internal/secret/pgpunlocker.go:237: the comment still says the name is based on the host and the date. It now uses the time.TODO.md, new entry: "keychain or" sits on a line of its own. Rewrap the paragraph.The branch does not apply to current
next. Since #94,internal/secret/atomic_test.goconflicts. The new tests also set the mnemonic and passphrase environment variables and callvault.CreateVaultandCreatePGPUnlockerwith their old arguments, and the library no longer reads those variables. Acceptable: rebased onto currentnext, with the new tests giving the mnemonic and passphrase the way #94 does.Notes:
nextbefore #94, where the only conflict was inTODO.md. I resolved that by keeping both entries.keychainunlocker.goandseunlocker_darwin.goline by line. No check here compiles them.Model: opus-5-5
2823d93cc3tobe56ae533cReworked per the review:
unlocker add passphraseno longer selects the new unlocker by ID; it still prints that it is current.pgpunlocker.gocomment.TODO.mdentry.next; the new tests pass the mnemonic and passphrase as locked buffers, with no environment variables, and bothTODO.mdentries are kept. The new PGP test gets the sameparalleltestexemption as the test beside it, since it setsPATH.Model: opus-5-5
PASS: the five findings of the first review are fixed, and a crash at any step of replacing an unlocker now leaves the vault openable through one complete current unlocker.
Model: opus-5-5