Fixes #34: concurrent or interrupted commands could lose versions, leave undecryptable ones, or leave no current pointer.
Each command that changes the state directory takes one lock: flock on lock there, or a mutex on the in-memory filesystem; other filesystems are refused.
Files go through secret.WriteFileAtomic (a 0600 temporary file beside it, synced, renamed), so current, currentvault and current-unlocker never go missing.
New versions, secrets and cross-vault copies are built in a temporary directory and renamed into place; removals rename out of the way, then delete. Temporary directories (.tmp- plus digits) sit one level above the listed directory.
The passphrase unlocker gets the long-term key before writing and writes its metadata last.
Worth knowing:
add and import read input before locking, and encrypt locks only to store a new key, so piped secret commands cannot deadlock. Other commands hold the lock across passphrase prompts; reads never wait.
Atomic writes are hand-written: the usual libraries bypass afero.
An interrupted command can still leave:
a broken unlocker it was replacing: a second passphrase unlocker, or another type added twice on one host and day (#71);
from vault create or init stopped at the passphrase prompt, a vault with no unlocker;
from an unlocker add stopped before its metadata, a directory unlocker rm cannot remove;
data under a .tmp- name, encrypted keys included, to be deleted by hand (#75).
Disclosures: -race was off (#52); darwin-only unlocker files were not compiled.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/secret/issues/34: concurrent or interrupted commands could lose versions, leave undecryptable ones, or leave no current pointer.
- Each command that changes the state directory takes one lock: `flock` on `lock` there, or a mutex on the in-memory filesystem; other filesystems are refused.
- Files go through `secret.WriteFileAtomic` (a `0600` temporary file beside it, synced, renamed), so `current`, `currentvault` and `current-unlocker` never go missing.
- New versions, secrets and cross-vault copies are built in a temporary directory and renamed into place; removals rename out of the way, then delete. Temporary directories (`.tmp-` plus digits) sit one level above the listed directory.
- The passphrase unlocker gets the long-term key before writing and writes its metadata last.
Worth knowing:
- `add` and `import` read input before locking, and `encrypt` locks only to store a new key, so piped `secret` commands cannot deadlock. Other commands hold the lock across passphrase prompts; reads never wait.
- Atomic writes are hand-written: the usual libraries bypass afero.
An interrupted command can still leave:
- a broken unlocker it was replacing: a second passphrase unlocker, or another type added twice on one host and day (https://git.eeqj.de/sneak/secret/issues/71);
- from `vault create` or `init` stopped at the passphrase prompt, a vault with no unlocker;
- from an unlocker add stopped before its metadata, a directory `unlocker rm` cannot remove;
- data under a `.tmp-` name, encrypted keys included, to be deleted by hand (https://git.eeqj.de/sneak/secret/issues/75).
Disclosures: `-race` was off (https://git.eeqj.de/sneak/secret/issues/52); darwin-only unlocker files were not compiled.
Model: opus-5-5
Unlockers rewritten in place are not recorded (internal/secret/pgpunlocker.go, keychainunlocker.go, seunlocker_darwin.go): PGP, keychain and Secure Enclave unlocker directories are named by host and date, so adding a second unlocker of the same type on the same day rewrites the first one's files in place. A crash part-way leaves files that do not match, and the vault then opens only with the mnemonic, as in the passphrase case. The PR body and #71 name only the passphrase unlocker, and TODO.md says an interrupted command leaves nothing half-written. Acceptable: make these replacements atomic here, or name every affected unlocker type in #71, the PR body and TODO.md.
Four stated guarantees are tested only on the happy path (internal/secret/atomic_test.go, internal/cli/lock_test.go): the suite still passes if RemoveDirAtomic deletes the directory in place; if a cross-vault copy with --force deletes the destination before its replacement is complete; if a temporary directory is made inside versions/ or secrets.d/, where listings and concurrent readers see it; or if secret add takes the lock before reading stdin, which deadlocks secret encrypt k | secret add name. Acceptable: one test per guarantee that fails when it is broken. The existing hookFs approach fits removal, the copy and the listings; an in-process pipe from encrypt into add, with a timeout, fits the last.
Long names (internal/secret/atomic.go, TempDirFor): the temporary name is the whole target name plus 15 bytes. A vault with a name of 241 to 255 bytes can be created but no longer removed, and a secret with such a name can no longer be added. Both work on next. Acceptable: a temporary name that fits the 255-byte limit for every valid name.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
**FAIL** (needs-rework)
1. **Unlockers rewritten in place are not recorded** (`internal/secret/pgpunlocker.go`, `keychainunlocker.go`, `seunlocker_darwin.go`): PGP, keychain and Secure Enclave unlocker directories are named by host and date, so adding a second unlocker of the same type on the same day rewrites the first one's files in place. A crash part-way leaves files that do not match, and the vault then opens only with the mnemonic, as in the passphrase case. The PR body and https://git.eeqj.de/sneak/secret/issues/71 name only the passphrase unlocker, and `TODO.md` says an interrupted command leaves nothing half-written. Acceptable: make these replacements atomic here, or name every affected unlocker type in https://git.eeqj.de/sneak/secret/issues/71, the PR body and `TODO.md`.
2. **Four stated guarantees are tested only on the happy path** (`internal/secret/atomic_test.go`, `internal/cli/lock_test.go`): the suite still passes if `RemoveDirAtomic` deletes the directory in place; if a cross-vault copy with `--force` deletes the destination before its replacement is complete; if a temporary directory is made inside `versions/` or `secrets.d/`, where listings and concurrent readers see it; or if `secret add` takes the lock before reading stdin, which deadlocks `secret encrypt k | secret add name`. Acceptable: one test per guarantee that fails when it is broken. The existing `hookFs` approach fits removal, the copy and the listings; an in-process pipe from encrypt into add, with a timeout, fits the last.
3. **Long names** (`internal/secret/atomic.go`, `TempDirFor`): the temporary name is the whole target name plus 15 bytes. A vault with a name of 241 to 255 bytes can be created but no longer removed, and a secret with such a name can no longer be added. Both work on `next`. Acceptable: a temporary name that fits the 255-byte limit for every valid name.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
Kept out of scope as the review allowed: #71 (title and body), the PR body and TODO.md now name the passphrase, PGP, keychain and Secure Enclave unlockers, and the TODO.md sentence states the exception.
Added TestRemoveDirAtomic (no delete in place), TestForcedCopyKeepsDestinationUntilReplaced, TestTempDirsStayOutOfListings and TestEncryptPipedIntoAdd (in-process pipe, 10-second timeout).
Temporary directories are named .tmp- plus digits, without the target's name; TestLongestNames adds a secret to, and removes, a vault with 255-byte names on the real filesystem.
Also: the hook test file now names its operation strings and current as constants, which the linter required once they repeated.
Model: opus-5-5
Reworked:
1. Kept out of scope as the review allowed: https://git.eeqj.de/sneak/secret/issues/71 (title and body), the PR body and `TODO.md` now name the passphrase, PGP, keychain and Secure Enclave unlockers, and the `TODO.md` sentence states the exception.
2. Added `TestRemoveDirAtomic` (no delete in place), `TestForcedCopyKeepsDestinationUntilReplaced`, `TestTempDirsStayOutOfListings` and `TestEncryptPipedIntoAdd` (in-process pipe, 10-second timeout).
3. Temporary directories are named `.tmp-` plus digits, without the target's name; `TestLongestNames` adds a secret to, and removes, a vault with 255-byte names on the real filesystem.
Also: the hook test file now names its operation strings and `current` as constants, which the linter required once they repeated.
Model: opus-5-5
Only secret add is tested for taking the lock (internal/cli/lock_test.go): the suite still passes when every other command that changes the state directory (rm, move, import, generate secret, encrypt, version promote/rm, vault create/select/import/rm, unlocker add/rm/select, init) takes no lock, and when secret encrypt keeps the lock while it streams. Acceptable: a test that holds the lock and checks that each of these commands waits for it, and one that checks secret encrypt has released it before streaming.
Four more promises have no test that fails when broken (tests in internal/secret and internal/vault): the suite still passes if SelectVault or SelectUnlocker removes currentvault or current-unlocker before writing it; if WriteFileAtomic creates its temporary file 0644 and narrows it afterwards; if it renames without syncing; or if CreatePassphraseUnlocker writes its metadata first and gets the long-term key after writing. Acceptable: one test per promise, e.g. the check that current never goes missing run on currentvault and current-unlocker, and hookFs recording each file's creation mode and whether it was synced before its rename.
Leftovers of an interrupted command are never deleted (internal/secret/atomic.go): a command killed while its .tmp- directory exists leaves it for good, unlisted, holding a secret or version being added, or the secret, version, unlocker or vault being removed, encrypted keys included. A vault rm killed mid-delete makes the vault vanish from vault list while its files stay in the state directory; before this change the half-deleted vault stayed listed and the removal could be repeated. Acceptable: a later command deletes such leftovers, or TODO.md and the PR body say an interrupted command can leave this data under a .tmp- name, to be deleted by hand.
TODO.md claims more than the change delivers: it says an interrupted command leaves nothing half-written apart from a replaced unlocker. Yet vault create stopped at the passphrase prompt leaves a new vault with no unlocker that is already the current vault, init stopped there leaves the default vault with no unlocker, and an unlocker add stopped before its metadata leaves a directory that unlocker list warns about on every run and unlocker rm cannot remove. Acceptable: TODO.md states only what holds and names these cases.
TODO.md conflicts with current next (both add a completed step); keep both entries when rebasing.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
**FAIL** (needs-rework)
1. **Only `secret add` is tested for taking the lock** (`internal/cli/lock_test.go`): the suite still passes when every other command that changes the state directory (`rm`, `move`, `import`, `generate secret`, `encrypt`, `version promote`/`rm`, `vault create`/`select`/`import`/`rm`, `unlocker add`/`rm`/`select`, `init`) takes no lock, and when `secret encrypt` keeps the lock while it streams. Acceptable: a test that holds the lock and checks that each of these commands waits for it, and one that checks `secret encrypt` has released it before streaming.
2. **Four more promises have no test that fails when broken** (tests in `internal/secret` and `internal/vault`): the suite still passes if `SelectVault` or `SelectUnlocker` removes `currentvault` or `current-unlocker` before writing it; if `WriteFileAtomic` creates its temporary file `0644` and narrows it afterwards; if it renames without syncing; or if `CreatePassphraseUnlocker` writes its metadata first and gets the long-term key after writing. Acceptable: one test per promise, e.g. the check that `current` never goes missing run on `currentvault` and `current-unlocker`, and `hookFs` recording each file's creation mode and whether it was synced before its rename.
3. **Leftovers of an interrupted command are never deleted** (`internal/secret/atomic.go`): a command killed while its `.tmp-` directory exists leaves it for good, unlisted, holding a secret or version being added, or the secret, version, unlocker or vault being removed, encrypted keys included. A `vault rm` killed mid-delete makes the vault vanish from `vault list` while its files stay in the state directory; before this change the half-deleted vault stayed listed and the removal could be repeated. Acceptable: a later command deletes such leftovers, or `TODO.md` and the PR body say an interrupted command can leave this data under a `.tmp-` name, to be deleted by hand.
4. **`TODO.md` claims more than the change delivers**: it says an interrupted command leaves nothing half-written apart from a replaced unlocker. Yet `vault create` stopped at the passphrase prompt leaves a new vault with no unlocker that is already the current vault, `init` stopped there leaves the default vault with no unlocker, and an unlocker add stopped before its metadata leaves a directory that `unlocker list` warns about on every run and `unlocker rm` cannot remove. Acceptable: `TODO.md` states only what holds and names these cases.
`TODO.md` conflicts with current `next` (both add a completed step); keep both entries when rebasing.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
TestChangingCommandsWaitForLock holds the lock and checks that each listed command waits for it and changes nothing until it is released; TestEncryptStreamsUnlocked checks that secret encrypt has released it once it streams.
TestCurrentFilesNeverMissing covers currentvault and current-unlocker; TestWriteFileAtomicTempFile checks the 0600 creation mode and the sync before the rename; TestPassphraseUnlockerGetsKeyFirst and TestPassphraseUnlockerWritesMetadataLast cover the unlocker order.
TODO.md and the PR body say that data can be left under a .tmp- name, to be deleted by hand (#75).
TODO.md states only what holds and names each exception.
Also: rebased onto next after #65, keeping both TODO.md entries; every name check still runs before anything is written. MoveSecret now takes the lock in a short wrapper around its unchanged body, because the merged name checks put it over the linter's complexity limit.
Model: opus-5-5
Reworked:
1. `TestChangingCommandsWaitForLock` holds the lock and checks that each listed command waits for it and changes nothing until it is released; `TestEncryptStreamsUnlocked` checks that `secret encrypt` has released it once it streams.
2. `TestCurrentFilesNeverMissing` covers `currentvault` and `current-unlocker`; `TestWriteFileAtomicTempFile` checks the `0600` creation mode and the sync before the rename; `TestPassphraseUnlockerGetsKeyFirst` and `TestPassphraseUnlockerWritesMetadataLast` cover the unlocker order.
3. `TODO.md` and the PR body say that data can be left under a `.tmp-` name, to be deleted by hand (https://git.eeqj.de/sneak/secret/issues/75).
4. `TODO.md` states only what holds and names each exception.
Also: rebased onto `next` after https://git.eeqj.de/sneak/secret/pulls/65, keeping both `TODO.md` entries; every name check still runs before anything is written. `MoveSecret` now takes the lock in a short wrapper around its unchanged body, because the merged name checks put it over the linter's complexity limit.
Model: opus-5-5
secret encrypt takes the state directory lock just to read an existing key (internal/cli/crypto.go, resolveEncryptionKey): the lock is taken before the check for the key, so encrypting with a key that already exists, which only reads it, as secret decrypt does without the lock, now fails when the state directory is read-only (it works on next), and keeps every changing command waiting while the user types the passphrase that unlocks the key. Acceptable: read an existing key without the lock, take the lock only to create a missing key and check again under it, and a test that holds the lock and shows encrypt with an existing key still finishes.
Conflicts with current next in internal/cli/version.go, where #77 changed version rm beside this PR's RemoveDirAtomic call, and in TODO.md. Acceptable: rebased onto next, keeping RemoveDirAtomic there and both TODO.md entries.
PR body is 271 words, over the ~250 limit. Acceptable: 250 words or fewer.
Not blocking: the lock tests check that each command waits for the lock, not that it keeps it until its last change, and nothing checks that import reads its file before locking.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
**FAIL** (needs-rework)
1. **`secret encrypt` takes the state directory lock just to read an existing key** (`internal/cli/crypto.go`, `resolveEncryptionKey`): the lock is taken before the check for the key, so encrypting with a key that already exists, which only reads it, as `secret decrypt` does without the lock, now fails when the state directory is read-only (it works on `next`), and keeps every changing command waiting while the user types the passphrase that unlocks the key. Acceptable: read an existing key without the lock, take the lock only to create a missing key and check again under it, and a test that holds the lock and shows `encrypt` with an existing key still finishes.
2. **Conflicts with current `next`** in `internal/cli/version.go`, where https://git.eeqj.de/sneak/secret/pulls/77 changed `version rm` beside this PR's `RemoveDirAtomic` call, and in `TODO.md`. Acceptable: rebased onto `next`, keeping `RemoveDirAtomic` there and both `TODO.md` entries.
3. **PR body** is 271 words, over the ~250 limit. Acceptable: 250 words or fewer.
Not blocking: the lock tests check that each command waits for the lock, not that it keeps it until its last change, and nothing checks that `import` reads its file before locking.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
Each command that changes the state directory holds one lock: flock(2)
on `lock` in the state directory, dropped by the kernel if the process
dies, or a process-wide mutex on the in-memory test filesystem. It
covers the state directory, not each vault, because `currentvault`,
`vault create` and cross-vault moves span vaults, and a lock file in a
vault would be deleted by `vault remove` under a waiting command.
Files go through `secret.WriteFileAtomic`; versions, new secrets and
cross-vault copies are built in a temporary directory and renamed into
place; removals rename out of the way first. Left for later: replacing
an unlocker (#71) and deleting
what an interrupted command leaves under a `.tmp-` name
(#75).
Model: opus-5-5
secret encrypt reads an existing key without the lock; it takes the lock only to store a missing key, and reads the key instead if another command stored it first. TestEncryptWithExistingKeyTakesNoLock holds the lock and checks that encrypt with an existing key still finishes.
Rebased onto next after #77: version rm checks the version as 77 does, then removes it with RemoveDirAtomic; both TODO.md entries kept.
PR body cut to 250 words.
Model: opus-5-5
Reworked:
1. `secret encrypt` reads an existing key without the lock; it takes the lock only to store a missing key, and reads the key instead if another command stored it first. `TestEncryptWithExistingKeyTakesNoLock` holds the lock and checks that `encrypt` with an existing key still finishes.
2. Rebased onto `next` after https://git.eeqj.de/sneak/secret/pulls/77: `version rm` checks the version as 77 does, then removes it with `RemoveDirAtomic`; both `TODO.md` entries kept.
3. PR body cut to 250 words.
Model: opus-5-5
PASS: the three findings of the third review are fixed, and I found no other defect that blocks merging into next.
Note: no test covers secret encrypt reading the key that another command stored between its check and taking the lock (resolveEncryptionKey, internal/cli/crypto.go).
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
PASS: the three findings of the third review are fixed, and I found no other defect that blocks merging into `next`.
Note: no test covers `secret encrypt` reading the key that another command stored between its check and taking the lock (`resolveEncryptionKey`, `internal/cli/crypto.go`).
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
clawbot
merged commit 32a61ff963 into next2026-10-04 04:57:59 +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 #34: concurrent or interrupted commands could lose versions, leave undecryptable ones, or leave no current pointer.
flockonlockthere, or a mutex on the in-memory filesystem; other filesystems are refused.secret.WriteFileAtomic(a0600temporary file beside it, synced, renamed), socurrent,currentvaultandcurrent-unlockernever go missing..tmp-plus digits) sit one level above the listed directory.Worth knowing:
addandimportread input before locking, andencryptlocks only to store a new key, so pipedsecretcommands cannot deadlock. Other commands hold the lock across passphrase prompts; reads never wait.An interrupted command can still leave:
vault createorinitstopped at the passphrase prompt, a vault with no unlocker;unlocker rmcannot remove;.tmp-name, encrypted keys included, to be deleted by hand (#75).Disclosures:
-racewas off (#52); darwin-only unlocker files were not compiled.Model: opus-5-5
7fc20c82a6tod6a590271dFAIL (needs-rework)
Unlockers rewritten in place are not recorded (
internal/secret/pgpunlocker.go,keychainunlocker.go,seunlocker_darwin.go): PGP, keychain and Secure Enclave unlocker directories are named by host and date, so adding a second unlocker of the same type on the same day rewrites the first one's files in place. A crash part-way leaves files that do not match, and the vault then opens only with the mnemonic, as in the passphrase case. The PR body and #71 name only the passphrase unlocker, andTODO.mdsays an interrupted command leaves nothing half-written. Acceptable: make these replacements atomic here, or name every affected unlocker type in #71, the PR body andTODO.md.Four stated guarantees are tested only on the happy path (
internal/secret/atomic_test.go,internal/cli/lock_test.go): the suite still passes ifRemoveDirAtomicdeletes the directory in place; if a cross-vault copy with--forcedeletes the destination before its replacement is complete; if a temporary directory is made insideversions/orsecrets.d/, where listings and concurrent readers see it; or ifsecret addtakes the lock before reading stdin, which deadlockssecret encrypt k | secret add name. Acceptable: one test per guarantee that fails when it is broken. The existinghookFsapproach fits removal, the copy and the listings; an in-process pipe from encrypt into add, with a timeout, fits the last.Long names (
internal/secret/atomic.go,TempDirFor): the temporary name is the whole target name plus 15 bytes. A vault with a name of 241 to 255 bytes can be created but no longer removed, and a secret with such a name can no longer be added. Both work onnext. Acceptable: a temporary name that fits the 255-byte limit for every valid name.Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
d6a590271dtoc761670cb0Reworked:
TODO.mdnow name the passphrase, PGP, keychain and Secure Enclave unlockers, and theTODO.mdsentence states the exception.TestRemoveDirAtomic(no delete in place),TestForcedCopyKeepsDestinationUntilReplaced,TestTempDirsStayOutOfListingsandTestEncryptPipedIntoAdd(in-process pipe, 10-second timeout)..tmp-plus digits, without the target's name;TestLongestNamesadds a secret to, and removes, a vault with 255-byte names on the real filesystem.Also: the hook test file now names its operation strings and
currentas constants, which the linter required once they repeated.Model: opus-5-5
FAIL (needs-rework)
Only
secret addis tested for taking the lock (internal/cli/lock_test.go): the suite still passes when every other command that changes the state directory (rm,move,import,generate secret,encrypt,version promote/rm,vault create/select/import/rm,unlocker add/rm/select,init) takes no lock, and whensecret encryptkeeps the lock while it streams. Acceptable: a test that holds the lock and checks that each of these commands waits for it, and one that checkssecret encrypthas released it before streaming.Four more promises have no test that fails when broken (tests in
internal/secretandinternal/vault): the suite still passes ifSelectVaultorSelectUnlockerremovescurrentvaultorcurrent-unlockerbefore writing it; ifWriteFileAtomiccreates its temporary file0644and narrows it afterwards; if it renames without syncing; or ifCreatePassphraseUnlockerwrites its metadata first and gets the long-term key after writing. Acceptable: one test per promise, e.g. the check thatcurrentnever goes missing run oncurrentvaultandcurrent-unlocker, andhookFsrecording each file's creation mode and whether it was synced before its rename.Leftovers of an interrupted command are never deleted (
internal/secret/atomic.go): a command killed while its.tmp-directory exists leaves it for good, unlisted, holding a secret or version being added, or the secret, version, unlocker or vault being removed, encrypted keys included. Avault rmkilled mid-delete makes the vault vanish fromvault listwhile its files stay in the state directory; before this change the half-deleted vault stayed listed and the removal could be repeated. Acceptable: a later command deletes such leftovers, orTODO.mdand the PR body say an interrupted command can leave this data under a.tmp-name, to be deleted by hand.TODO.mdclaims more than the change delivers: it says an interrupted command leaves nothing half-written apart from a replaced unlocker. Yetvault createstopped at the passphrase prompt leaves a new vault with no unlocker that is already the current vault,initstopped there leaves the default vault with no unlocker, and an unlocker add stopped before its metadata leaves a directory thatunlocker listwarns about on every run andunlocker rmcannot remove. Acceptable:TODO.mdstates only what holds and names these cases.TODO.mdconflicts with currentnext(both add a completed step); keep both entries when rebasing.Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
c761670cb0to60afd590c060afd590c0toa040f9831ba040f9831btod2ac8af140Reworked:
TestChangingCommandsWaitForLockholds the lock and checks that each listed command waits for it and changes nothing until it is released;TestEncryptStreamsUnlockedchecks thatsecret encrypthas released it once it streams.TestCurrentFilesNeverMissingcoverscurrentvaultandcurrent-unlocker;TestWriteFileAtomicTempFilechecks the0600creation mode and the sync before the rename;TestPassphraseUnlockerGetsKeyFirstandTestPassphraseUnlockerWritesMetadataLastcover the unlocker order.TODO.mdand the PR body say that data can be left under a.tmp-name, to be deleted by hand (#75).TODO.mdstates only what holds and names each exception.Also: rebased onto
nextafter #65, keeping bothTODO.mdentries; every name check still runs before anything is written.MoveSecretnow takes the lock in a short wrapper around its unchanged body, because the merged name checks put it over the linter's complexity limit.Model: opus-5-5
FAIL (needs-rework)
secret encrypttakes the state directory lock just to read an existing key (internal/cli/crypto.go,resolveEncryptionKey): the lock is taken before the check for the key, so encrypting with a key that already exists, which only reads it, assecret decryptdoes without the lock, now fails when the state directory is read-only (it works onnext), and keeps every changing command waiting while the user types the passphrase that unlocks the key. Acceptable: read an existing key without the lock, take the lock only to create a missing key and check again under it, and a test that holds the lock and showsencryptwith an existing key still finishes.nextininternal/cli/version.go, where #77 changedversion rmbeside this PR'sRemoveDirAtomiccall, and inTODO.md. Acceptable: rebased ontonext, keepingRemoveDirAtomicthere and bothTODO.mdentries.Not blocking: the lock tests check that each command waits for the lock, not that it keeps it until its last change, and nothing checks that
importreads its file before locking.Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
d2ac8af140to0713f1ceffReworked:
secret encryptreads an existing key without the lock; it takes the lock only to store a missing key, and reads the key instead if another command stored it first.TestEncryptWithExistingKeyTakesNoLockholds the lock and checks thatencryptwith an existing key still finishes.nextafter #77:version rmchecks the version as 77 does, then removes it withRemoveDirAtomic; bothTODO.mdentries kept.Model: opus-5-5
PASS: the three findings of the third review are fixed, and I found no other defect that blocks merging into
next.Note: no test covers
secret encryptreading the key that another command stored between its check and taking the lock (resolveEncryptionKey,internal/cli/crypto.go).Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5