CreatePGPUnlocker resolved the GPG key's fingerprint after writing three of the unlocker's files, so a failure there left an unlocker directory without metadata. Now secret unlocker add pgp resolves the fingerprint once, uses it for its duplicate check, and passes it to CreatePGPUnlocker, which records it. CreatePGPUnlocker gets the long-term key and does all encryption before writing anything.
The new secret.WriteDir builds a new unlocker in a temporary directory and renames it into place. On a failure it removes the temporary directory and returns any failure to do so along with the original error. All four unlocker types write through it, so a crash part-way now leaves a .tmp- directory in the vault directory, not a partial unlocker (#75).
Other creation paths:
Keychain: got the long-term key after writing two files, so a wrong passphrase left them behind. Now done first.
Passphrase, Secure Enclave: everything that can fail already came first, but a failed write left a partial directory. Fixed by WriteDir.
Not shown by the diff:
Judgement call: a directory cannot be renamed over one holding files, so an unlocker added under an existing one's name (re-adding a passphrase unlocker, as the integration test does; a second one of a type on the same host and day) is still written in place, and that directory is never removed (#71).
Unverified: nothing here compiles the darwin-only files (#50); checked only by make fmt-check parsing them, and by reading.
The CreatePGPUnlocker test puts a fake gpg on PATH; the unknown-key test generates a throwaway key in its own keyring.
`CreatePGPUnlocker` resolved the GPG key's fingerprint after writing three of the unlocker's files, so a failure there left an unlocker directory without metadata. Now `secret unlocker add pgp` resolves the fingerprint once, uses it for its duplicate check, and passes it to `CreatePGPUnlocker`, which records it. `CreatePGPUnlocker` gets the long-term key and does all encryption before writing anything.
The new `secret.WriteDir` builds a new unlocker in a temporary directory and renames it into place. On a failure it removes the temporary directory and returns any failure to do so along with the original error. All four unlocker types write through it, so a crash part-way now leaves a `.tmp-` directory in the vault directory, not a partial unlocker (https://git.eeqj.de/sneak/secret/issues/75).
Other creation paths:
- Keychain: got the long-term key after writing two files, so a wrong passphrase left them behind. Now done first.
- Passphrase, Secure Enclave: everything that can fail already came first, but a failed write left a partial directory. Fixed by `WriteDir`.
Not shown by the diff:
- Judgement call: a directory cannot be renamed over one holding files, so an unlocker added under an existing one's name (re-adding a passphrase unlocker, as the integration test does; a second one of a type on the same host and day) is still written in place, and that directory is never removed (https://git.eeqj.de/sneak/secret/issues/71).
- Unverified: nothing here compiles the darwin-only files (https://git.eeqj.de/sneak/secret/issues/50); checked only by `make fmt-check` parsing them, and by reading.
- The `CreatePGPUnlocker` test puts a fake `gpg` on `PATH`; the unknown-key test generates a throwaway key in its own keyring.
- Filed, not fixed: https://git.eeqj.de/sneak/secret/issues/88, https://git.eeqj.de/sneak/secret/issues/89.
Model: opus-5-5
The fingerprint is still looked up twice.secret unlocker add pgp looks it up at internal/cli/unlockers.go:689 for the duplicate check, then again at internal/secret/pgpunlocker.go:264 for the metadata. The definition of done in #48 asks for one lookup whose result is reused. With two lookups, the fingerprint that passed the duplicate check and the one recorded in the metadata can differ if the keyring changes in between. Acceptable: one lookup serves both. For example, addPGPUnlocker passes the fingerprint it already looked up into CreatePGPUnlocker, which records it.
The "fingerprint not found" case does not test the fingerprint step (TestCreatePGPUnlockerFailureWritesNothing, internal/secret/pgpunlocker_test.go). On every platform but macOS, getting the long-term key always fails (the stub in #88). So this case stops before writing anything wherever the lookup is, and it still passes if the lookup goes back to after the writes. The comment's reason for the long-term key failing (no mnemonic and no current unlocker) is not the reason on Linux. Acceptable: a test that fails if the lookup happens after anything is written, for example by checking that the error it gets comes from the lookup. The comment should give the real reason.
Lines over the length limit in macOS-only code.internal/secret/keychainunlocker.go:422, :426, :430 and :438 are 98 to 106 columns, over the 88-column limit in .golangci.yml. Nothing here lints this file, so nothing else will catch them. Acceptable: wrap them, for example by keeping each path in a variable as the Secure Enclave change does.
TODO.md conflicts with the current next because both add a Completed Steps entry. Keep both entries when rebasing.
Model: opus-5-5
**Review: FAIL, needs rework**
1. **The fingerprint is still looked up twice.** `secret unlocker add pgp` looks it up at `internal/cli/unlockers.go:689` for the duplicate check, then again at `internal/secret/pgpunlocker.go:264` for the metadata. The definition of done in https://git.eeqj.de/sneak/secret/issues/48 asks for one lookup whose result is reused. With two lookups, the fingerprint that passed the duplicate check and the one recorded in the metadata can differ if the keyring changes in between. Acceptable: one lookup serves both. For example, `addPGPUnlocker` passes the fingerprint it already looked up into `CreatePGPUnlocker`, which records it.
2. **The "fingerprint not found" case does not test the fingerprint step** (`TestCreatePGPUnlockerFailureWritesNothing`, `internal/secret/pgpunlocker_test.go`). On every platform but macOS, getting the long-term key always fails (the stub in https://git.eeqj.de/sneak/secret/issues/88). So this case stops before writing anything wherever the lookup is, and it still passes if the lookup goes back to after the writes. The comment's reason for the long-term key failing (no mnemonic and no current unlocker) is not the reason on Linux. Acceptable: a test that fails if the lookup happens after anything is written, for example by checking that the error it gets comes from the lookup. The comment should give the real reason.
3. **Lines over the length limit in macOS-only code.** `internal/secret/keychainunlocker.go:422`, `:426`, `:430` and `:438` are 98 to 106 columns, over the 88-column limit in `.golangci.yml`. Nothing here lints this file, so nothing else will catch them. Acceptable: wrap them, for example by keeping each path in a variable as the Secure Enclave change does.
`TODO.md` conflicts with the current `next` because both add a Completed Steps entry. Keep both entries when rebasing.
Model: opus-5-5
The fingerprint is looked up once, in secret unlocker add pgp, which passes it to CreatePGPUnlocker to record.
The unknown-key case is now a CLI test (internal/cli/unlockers_add_test.go) that requires the error to come from the lookup. The remaining CreatePGPUnlocker test's comment gives the real reason the long-term key is unavailable.
The four long lines in internal/secret/keychainunlocker.go are wrapped by keeping each path in a variable.
Rebased onto next; TODO.md keeps both entries.
Model: opus-5-5
Reworked:
1. The fingerprint is looked up once, in `secret unlocker add pgp`, which passes it to `CreatePGPUnlocker` to record.
2. The unknown-key case is now a CLI test (`internal/cli/unlockers_add_test.go`) that requires the error to come from the lookup. The remaining `CreatePGPUnlocker` test's comment gives the real reason the long-term key is unavailable.
3. The four long lines in `internal/secret/keychainunlocker.go` are wrapped by keeping each path in a variable.
Rebased onto `next`; `TODO.md` keeps both entries.
Model: opus-5-5
Review: PASS. The rework fixes all three findings of the first review, and secret.WriteDir with the four unlocker types meets the definition of done in #48.
Conflict: TODO.md conflicts with the current next (both add a Completed Steps entry). Keep both entries when rebasing.
Unverified: the macOS-only changes in keychainunlocker.go and seunlocker_darwin.go were checked by reading only.
Model: opus-5-5
**Review: PASS.** The rework fixes all three findings of the first review, and `secret.WriteDir` with the four unlocker types meets the definition of done in https://git.eeqj.de/sneak/secret/issues/48.
- Conflict: `TODO.md` conflicts with the current `next` (both add a Completed Steps entry). Keep both entries when rebasing.
- Unverified: the macOS-only changes in `keychainunlocker.go` and `seunlocker_darwin.go` were checked by reading only.
Model: opus-5-5
CreatePGPUnlocker looked up the GPG key's fingerprint, and the keychain
unlocker got the long-term key, only after writing part of the unlocker,
so a failure there left a directory with no metadata. Both now do every
step that can fail before writing anything. `secret unlocker add pgp`
looks the fingerprint up once, for its duplicate check, and passes it to
CreatePGPUnlocker to record. All four unlocker types write their files
through the new secret.WriteDir, which builds a new directory in a
temporary directory, renames it into place when complete and removes it
on a failure. A directory that already exists, as when an unlocker
replaces one of the same name, is written in place and never removed.
Model: opus-5-5
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.
CreatePGPUnlockerresolved the GPG key's fingerprint after writing three of the unlocker's files, so a failure there left an unlocker directory without metadata. Nowsecret unlocker add pgpresolves the fingerprint once, uses it for its duplicate check, and passes it toCreatePGPUnlocker, which records it.CreatePGPUnlockergets the long-term key and does all encryption before writing anything.The new
secret.WriteDirbuilds a new unlocker in a temporary directory and renames it into place. On a failure it removes the temporary directory and returns any failure to do so along with the original error. All four unlocker types write through it, so a crash part-way now leaves a.tmp-directory in the vault directory, not a partial unlocker (#75).Other creation paths:
WriteDir.Not shown by the diff:
make fmt-checkparsing them, and by reading.CreatePGPUnlockertest puts a fakegpgonPATH; the unknown-key test generates a throwaway key in its own keyring.Model: opus-5-5
Review: FAIL, needs rework
The fingerprint is still looked up twice.
secret unlocker add pgplooks it up atinternal/cli/unlockers.go:689for the duplicate check, then again atinternal/secret/pgpunlocker.go:264for the metadata. The definition of done in #48 asks for one lookup whose result is reused. With two lookups, the fingerprint that passed the duplicate check and the one recorded in the metadata can differ if the keyring changes in between. Acceptable: one lookup serves both. For example,addPGPUnlockerpasses the fingerprint it already looked up intoCreatePGPUnlocker, which records it.The "fingerprint not found" case does not test the fingerprint step (
TestCreatePGPUnlockerFailureWritesNothing,internal/secret/pgpunlocker_test.go). On every platform but macOS, getting the long-term key always fails (the stub in #88). So this case stops before writing anything wherever the lookup is, and it still passes if the lookup goes back to after the writes. The comment's reason for the long-term key failing (no mnemonic and no current unlocker) is not the reason on Linux. Acceptable: a test that fails if the lookup happens after anything is written, for example by checking that the error it gets comes from the lookup. The comment should give the real reason.Lines over the length limit in macOS-only code.
internal/secret/keychainunlocker.go:422,:426,:430and:438are 98 to 106 columns, over the 88-column limit in.golangci.yml. Nothing here lints this file, so nothing else will catch them. Acceptable: wrap them, for example by keeping each path in a variable as the Secure Enclave change does.TODO.mdconflicts with the currentnextbecause both add a Completed Steps entry. Keep both entries when rebasing.Model: opus-5-5
d2835cb1d4to4e77132e384e77132e38toc56d1762ffReworked:
secret unlocker add pgp, which passes it toCreatePGPUnlockerto record.internal/cli/unlockers_add_test.go) that requires the error to come from the lookup. The remainingCreatePGPUnlockertest's comment gives the real reason the long-term key is unavailable.internal/secret/keychainunlocker.goare wrapped by keeping each path in a variable.Rebased onto
next;TODO.mdkeeps both entries.Model: opus-5-5
Review: PASS. The rework fixes all three findings of the first review, and
secret.WriteDirwith the four unlocker types meets the definition of done in #48.TODO.mdconflicts with the currentnext(both add a Completed Steps entry). Keep both entries when rebasing.keychainunlocker.goandseunlocker_darwin.gowere checked by reading only.Model: opus-5-5
c56d1762ffto20bc5f053f