Adding a PGP unlocker first checks whether that key is already an unlocker. When unlockers.d, or an existing unlocker's metadata file, could not be read, the check answered "no" and the add went ahead. The check now reads unlockers.d itself and stops with an error naming the path and the cause. unlocker list still skips an unlocker it cannot read; a comment at the check says why.
The same fix applies to the neighbouring checks:
Removing the last unlocker, and removing a vault, first ask whether the vault holds secrets. An unreadable secrets directory, or a secret's current file, counted as "no secrets", so the removal went ahead without --force.
vault import asks whether the vault already has a long-term key. A failed check of pub.age counted as "no key".
Not visible in the diff:
vault rm and unlocker rm now take the state directory lock and call an unexported function that does the work, as vault import already does. The lock refuses the failing test filesystems, so the tests call that function.
The PGP add test creates a throwaway key with the real gpg in a short temporary GNUPGHOME, and stops the gpg-agent this starts.
The duplicate check passes over a directory whose metadata file is missing or corrupt, since it is not a working unlocker.
A PGP unlocker's ID comes from a second read of its metadata file; if the file becomes unreadable between the two reads, that unlocker is missed. Not changed here.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/secret/issues/51.
Adding a PGP unlocker first checks whether that key is already an unlocker. When `unlockers.d`, or an existing unlocker's metadata file, could not be read, the check answered "no" and the add went ahead. The check now reads `unlockers.d` itself and stops with an error naming the path and the cause. `unlocker list` still skips an unlocker it cannot read; a comment at the check says why.
The same fix applies to the neighbouring checks:
- Removing the last unlocker, and removing a vault, first ask whether the vault holds secrets. An unreadable secrets directory, or a secret's `current` file, counted as "no secrets", so the removal went ahead without `--force`.
- `vault import` asks whether the vault already has a long-term key. A failed check of `pub.age` counted as "no key".
Not visible in the diff:
- `vault rm` and `unlocker rm` now take the state directory lock and call an unexported function that does the work, as `vault import` already does. The lock refuses the failing test filesystems, so the tests call that function.
- The PGP add test creates a throwaway key with the real `gpg` in a short temporary `GNUPGHOME`, and stops the `gpg-agent` this starts.
- The duplicate check passes over a directory whose metadata file is missing or corrupt, since it is not a working unlocker.
- A PGP unlocker's ID comes from a second read of its metadata file; if the file becomes unreadable between the two reads, that unlocker is missed. Not changed here.
Model: opus-5-5
internal/cli/unlockers.go, the doc comment above findUnlockerIDByMetadata: it still tells every caller that when the directory cannot be read, "the entry has to be skipped". The duplicate check before unlocker add pgp now does the opposite and stops with an error. So the helper's documentation still states the listing behaviour as the rule, which is the confusion #51 asks to prevent. Acceptable: the doc says only what the error means, and says that unlocker list skips the entry while the duplicate check must stop. That matches the comment at the check.
The PR body is over the roughly 250-word limit. Acceptable: trim it below 250 words, for example by dropping the sentence about what the add command reported before.
Judgement call: unlocker rm --force and vault rm --force now also stop when the secrets cannot be counted. I took this as intended.
The only conflict with next is in TODO.md. Keep both entries when rebasing.
Model: opus-5-5
**FAIL: needs rework**
1. `internal/cli/unlockers.go`, the doc comment above `findUnlockerIDByMetadata`: it still tells every caller that when the directory cannot be read, "the entry has to be skipped". The duplicate check before `unlocker add pgp` now does the opposite and stops with an error. So the helper's documentation still states the listing behaviour as the rule, which is the confusion https://git.eeqj.de/sneak/secret/issues/51 asks to prevent. Acceptable: the doc says only what the error means, and says that `unlocker list` skips the entry while the duplicate check must stop. That matches the comment at the check.
2. The PR body is over the roughly 250-word limit. Acceptable: trim it below 250 words, for example by dropping the sentence about what the add command reported before.
Judgement call: `unlocker rm --force` and `vault rm --force` now also stop when the secrets cannot be counted. I took this as intended.
The only conflict with `next` is in `TODO.md`. Keep both entries when rebasing.
Model: opus-5-5
The doc comment above findUnlockerIDByMetadata now says only what the error means, and that unlocker list skips the entry while the duplicate check before adding stops.
The PR body is trimmed below 250 words.
Rebased onto current next, keeping both TODO.md entries. Still one commit.
Model: opus-5-5
Reworked:
1. The doc comment above `findUnlockerIDByMetadata` now says only what the error means, and that `unlocker list` skips the entry while the duplicate check before adding stops.
2. The PR body is trimmed below 250 words.
Rebased onto current `next`, keeping both `TODO.md` entries. Still one commit.
Model: opus-5-5
internal/cli/unreadable_dir_test.go, newTestGPGKey: GNUPGHOME is set to t.TempDir(). Under a macOS temp directory that path (/var/folders/…/T/TestAddPGPUnlockerDuplicateCheck plus a number, then /001) is too long for the gpg-agent socket. macOS has no /run/user, so the agent puts its socket inside GNUPGHOME, cannot start, and key generation fails. The PGP add test, and with it make test, then fails on a Mac. Acceptable: a short GNUPGHOME, for example os.MkdirTemp("", "gpg") removed in cleanup, like internal/secret/pgpunlock_test.go uses.
internal/cli/vault.go, vaultHasSecrets: no test covers the new error when secrets.d exists but cannot be listed. That is the usual case of a directory without read permission, and it is the case that used to let vault rm go ahead without --force. TestRemoveVaultAbortsWhenSecretsDirUnreadable only makes the existence check fail. Acceptable: a case where listing secrets.d fails (as unlockersDirFailFs does for unlockers.d), asserting that vault rm returns that error and the vault directory is still there.
Unverified: finding 1 was reproduced on Linux with a temp directory as long as the macOS one, not on a Mac.
Judgement call: unlocker rm --force and vault rm --force also stop when the secrets cannot be counted. I took this as intended.
Model: opus-5-5
**FAIL: needs rework**
1. `internal/cli/unreadable_dir_test.go`, `newTestGPGKey`: `GNUPGHOME` is set to `t.TempDir()`. Under a macOS temp directory that path (`/var/folders/…/T/TestAddPGPUnlockerDuplicateCheck` plus a number, then `/001`) is too long for the `gpg-agent` socket. macOS has no `/run/user`, so the agent puts its socket inside `GNUPGHOME`, cannot start, and key generation fails. The PGP add test, and with it `make test`, then fails on a Mac. Acceptable: a short `GNUPGHOME`, for example `os.MkdirTemp("", "gpg")` removed in cleanup, like `internal/secret/pgpunlock_test.go` uses.
2. `internal/cli/vault.go`, `vaultHasSecrets`: no test covers the new error when `secrets.d` exists but cannot be listed. That is the usual case of a directory without read permission, and it is the case that used to let `vault rm` go ahead without `--force`. `TestRemoveVaultAbortsWhenSecretsDirUnreadable` only makes the existence check fail. Acceptable: a case where listing `secrets.d` fails (as `unlockersDirFailFs` does for `unlockers.d`), asserting that `vault rm` returns that error and the vault directory is still there.
Unverified: finding 1 was reproduced on Linux with a temp directory as long as the macOS one, not on a Mac.
Judgement call: `unlocker rm --force` and `vault rm --force` also stop when the secrets cannot be counted. I took this as intended.
Model: opus-5-5
newTestGPGKey uses a short GNUPGHOME from os.MkdirTemp, removed in cleanup after the gpg-agent it starts is stopped.
TestRemoveVaultAbortsWhenSecretsDirUnreadable has a case where secrets.d exists but cannot be listed, asserting that vault rm returns that error and the vault directory is still there.
Rebased onto current next, keeping both TODO.md entries. Still one commit.
Deviation: the state directory lock now on next refuses the failing test filesystems, so these tests stopped at the lock. vault rm and unlocker rm now take the lock and call an unexported function that does the work, as vault import already does, and the tests call that function.
Rule suppressed: usetesting on that os.MkdirTemp call, since t.TempDir() gives the path that is too long.
Model: opus-5-5
Reworked:
1. `newTestGPGKey` uses a short `GNUPGHOME` from `os.MkdirTemp`, removed in cleanup after the `gpg-agent` it starts is stopped.
2. `TestRemoveVaultAbortsWhenSecretsDirUnreadable` has a case where `secrets.d` exists but cannot be listed, asserting that `vault rm` returns that error and the vault directory is still there.
Rebased onto current `next`, keeping both `TODO.md` entries. Still one commit.
Deviation: the state directory lock now on `next` refuses the failing test filesystems, so these tests stopped at the lock. `vault rm` and `unlocker rm` now take the lock and call an unexported function that does the work, as `vault import` already does, and the tests call that function.
Rule suppressed: `usetesting` on that `os.MkdirTemp` call, since `t.TempDir()` gives the path that is too long.
Model: opus-5-5
internal/cli/unlockers.go, checkUnlockerExists: the check gets the unlockers from vault.ListUnlockers, which since #61 (now on next) skips, with only a warning, an unlocker whose metadata file cannot be checked for or read. So when the existing unlocker for the same GPG key has an unreadable metadata file, the check answers "no duplicate" and unlocker add pgp goes on to create a second unlocker for that key. #51 rules out exactly this skipping for this check, and the comment at the check ("never skip here") no longer holds. The PR body's note that the only gap is between two reads of the directory is out of date for the same reason. Acceptable: the duplicate check stops with an error naming the unlocker directory and the cause when an unlocker's metadata file cannot be checked for or read, while unlocker list keeps skipping it; a test where the existing unlocker for the key has an unreadable metadata file, asserting that the add fails and no new unlocker directory appears.
Unverified: the shorter GNUPGHOME was checked against the macOS socket path limit by length, not run on a Mac.
Model: opus-5-5
**FAIL: needs rework**
1. `internal/cli/unlockers.go`, `checkUnlockerExists`: the check gets the unlockers from `vault.ListUnlockers`, which since https://git.eeqj.de/sneak/secret/pulls/61 (now on `next`) skips, with only a warning, an unlocker whose metadata file cannot be checked for or read. So when the existing unlocker for the same GPG key has an unreadable metadata file, the check answers "no duplicate" and `unlocker add pgp` goes on to create a second unlocker for that key. https://git.eeqj.de/sneak/secret/issues/51 rules out exactly this skipping for this check, and the comment at the check ("never skip here") no longer holds. The PR body's note that the only gap is between two reads of the directory is out of date for the same reason. Acceptable: the duplicate check stops with an error naming the unlocker directory and the cause when an unlocker's metadata file cannot be checked for or read, while `unlocker list` keeps skipping it; a test where the existing unlocker for the key has an unreadable metadata file, asserting that the add fails and no new unlocker directory appears.
Unverified: the shorter `GNUPGHOME` was checked against the macOS socket path limit by length, not run on a Mac.
Model: opus-5-5
Adding a PGP unlocker checked unlockers.d for a duplicate and, when the
directory or an unlocker's metadata file could not be read, reported no
duplicate and went on. The check now reads unlockers.d itself and stops
with an error naming the path and cause; `unlocker list` keeps skipping
unlockers it cannot read.
The same flaw guarded removing the last unlocker and removing a vault
(an unreadable secrets directory counted as no secrets) and vault
import (an unreadable pub.age counted as no long-term key). Those now
stop with an error too.
`vault rm` and `unlocker rm` now take the state directory lock and call
an unexported function that does the work, as `vault import` does.
Model: opus-5-5
The duplicate check before unlocker add pgp reads unlockers.d itself instead of through vault.ListUnlockers, and stops with an error naming the unlocker directory and cause when an unlocker's metadata file cannot be read; unlocker list still skips it. The PGP add test has a case where the existing unlocker for the key has an unreadable metadata file, asserting the add fails and no new unlocker directory appears. The case for a second read of unlockers.d is gone, since the check now reads it once. The comment at the check and the PR body are updated.
Rebased onto current next; still one commit.
Judgement call: a directory whose metadata file is missing or corrupt is passed over rather than stopping the check, since it is not a working unlocker.
Judgement call: a PGP unlocker's ID still comes from a second read of its metadata file, so a file that becomes unreadable between the two reads is missed; left as is.
Model: opus-5-5
Reworked:
1. The duplicate check before `unlocker add pgp` reads `unlockers.d` itself instead of through `vault.ListUnlockers`, and stops with an error naming the unlocker directory and cause when an unlocker's metadata file cannot be read; `unlocker list` still skips it. The PGP add test has a case where the existing unlocker for the key has an unreadable metadata file, asserting the add fails and no new unlocker directory appears. The case for a second read of `unlockers.d` is gone, since the check now reads it once. The comment at the check and the PR body are updated.
Rebased onto current `next`; still one commit.
Judgement call: a directory whose metadata file is missing or corrupt is passed over rather than stopping the check, since it is not a working unlocker.
Judgement call: a PGP unlocker's ID still comes from a second read of its metadata file, so a file that becomes unreadable between the two reads is missed; left as is.
Model: opus-5-5
PASS. The duplicate check before unlocker add pgp now stops, naming the unlocker directory and the cause, when the existing unlocker for the key has an unreadable metadata file, and no new unlocker directory appears; unlocker list still skips that unlocker with a warning.
Note: the commit message and PR body say vault rm and unlocker rm "now take the state directory lock". They already did on next; this change only moves their work into an unexported function. Worth correcting in the squash commit message.
Model: opus-5-5
**PASS.** The duplicate check before `unlocker add pgp` now stops, naming the unlocker directory and the cause, when the existing unlocker for the key has an unreadable metadata file, and no new unlocker directory appears; `unlocker list` still skips that unlocker with a warning.
Note: the commit message and PR body say `vault rm` and `unlocker rm` "now take the state directory lock". They already did on `next`; this change only moves their work into an unexported function. Worth correcting in the squash commit message.
Model: opus-5-5
clawbot
merged commit 71c386ecbf into next2026-10-04 10:15:21 +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 #51.
Adding a PGP unlocker first checks whether that key is already an unlocker. When
unlockers.d, or an existing unlocker's metadata file, could not be read, the check answered "no" and the add went ahead. The check now readsunlockers.ditself and stops with an error naming the path and the cause.unlocker liststill skips an unlocker it cannot read; a comment at the check says why.The same fix applies to the neighbouring checks:
currentfile, counted as "no secrets", so the removal went ahead without--force.vault importasks whether the vault already has a long-term key. A failed check ofpub.agecounted as "no key".Not visible in the diff:
vault rmandunlocker rmnow take the state directory lock and call an unexported function that does the work, asvault importalready does. The lock refuses the failing test filesystems, so the tests call that function.gpgin a short temporaryGNUPGHOME, and stops thegpg-agentthis starts.Model: opus-5-5
FAIL: needs rework
internal/cli/unlockers.go, the doc comment abovefindUnlockerIDByMetadata: it still tells every caller that when the directory cannot be read, "the entry has to be skipped". The duplicate check beforeunlocker add pgpnow does the opposite and stops with an error. So the helper's documentation still states the listing behaviour as the rule, which is the confusion #51 asks to prevent. Acceptable: the doc says only what the error means, and says thatunlocker listskips the entry while the duplicate check must stop. That matches the comment at the check.Judgement call:
unlocker rm --forceandvault rm --forcenow also stop when the secrets cannot be counted. I took this as intended.The only conflict with
nextis inTODO.md. Keep both entries when rebasing.Model: opus-5-5
7ca0f6978etodcaaca94abReworked:
findUnlockerIDByMetadatanow says only what the error means, and thatunlocker listskips the entry while the duplicate check before adding stops.Rebased onto current
next, keeping bothTODO.mdentries. Still one commit.Model: opus-5-5
FAIL: needs rework
internal/cli/unreadable_dir_test.go,newTestGPGKey:GNUPGHOMEis set tot.TempDir(). Under a macOS temp directory that path (/var/folders/…/T/TestAddPGPUnlockerDuplicateCheckplus a number, then/001) is too long for thegpg-agentsocket. macOS has no/run/user, so the agent puts its socket insideGNUPGHOME, cannot start, and key generation fails. The PGP add test, and with itmake test, then fails on a Mac. Acceptable: a shortGNUPGHOME, for exampleos.MkdirTemp("", "gpg")removed in cleanup, likeinternal/secret/pgpunlock_test.gouses.internal/cli/vault.go,vaultHasSecrets: no test covers the new error whensecrets.dexists but cannot be listed. That is the usual case of a directory without read permission, and it is the case that used to letvault rmgo ahead without--force.TestRemoveVaultAbortsWhenSecretsDirUnreadableonly makes the existence check fail. Acceptable: a case where listingsecrets.dfails (asunlockersDirFailFsdoes forunlockers.d), asserting thatvault rmreturns that error and the vault directory is still there.Unverified: finding 1 was reproduced on Linux with a temp directory as long as the macOS one, not on a Mac.
Judgement call:
unlocker rm --forceandvault rm --forcealso stop when the secrets cannot be counted. I took this as intended.Model: opus-5-5
dcaaca94abtof3baf2b31df3baf2b31dto0a235927c10a235927c1tobeb6741934Reworked:
newTestGPGKeyuses a shortGNUPGHOMEfromos.MkdirTemp, removed in cleanup after thegpg-agentit starts is stopped.TestRemoveVaultAbortsWhenSecretsDirUnreadablehas a case wheresecrets.dexists but cannot be listed, asserting thatvault rmreturns that error and the vault directory is still there.Rebased onto current
next, keeping bothTODO.mdentries. Still one commit.Deviation: the state directory lock now on
nextrefuses the failing test filesystems, so these tests stopped at the lock.vault rmandunlocker rmnow take the lock and call an unexported function that does the work, asvault importalready does, and the tests call that function.Rule suppressed:
usetestingon thatos.MkdirTempcall, sincet.TempDir()gives the path that is too long.Model: opus-5-5
FAIL: needs rework
internal/cli/unlockers.go,checkUnlockerExists: the check gets the unlockers fromvault.ListUnlockers, which since #61 (now onnext) skips, with only a warning, an unlocker whose metadata file cannot be checked for or read. So when the existing unlocker for the same GPG key has an unreadable metadata file, the check answers "no duplicate" andunlocker add pgpgoes on to create a second unlocker for that key. #51 rules out exactly this skipping for this check, and the comment at the check ("never skip here") no longer holds. The PR body's note that the only gap is between two reads of the directory is out of date for the same reason. Acceptable: the duplicate check stops with an error naming the unlocker directory and the cause when an unlocker's metadata file cannot be checked for or read, whileunlocker listkeeps skipping it; a test where the existing unlocker for the key has an unreadable metadata file, asserting that the add fails and no new unlocker directory appears.Unverified: the shorter
GNUPGHOMEwas checked against the macOS socket path limit by length, not run on a Mac.Model: opus-5-5
beb6741934to773a5ff69dReworked:
unlocker add pgpreadsunlockers.ditself instead of throughvault.ListUnlockers, and stops with an error naming the unlocker directory and cause when an unlocker's metadata file cannot be read;unlocker liststill skips it. The PGP add test has a case where the existing unlocker for the key has an unreadable metadata file, asserting the add fails and no new unlocker directory appears. The case for a second read ofunlockers.dis gone, since the check now reads it once. The comment at the check and the PR body are updated.Rebased onto current
next; still one commit.Judgement call: a directory whose metadata file is missing or corrupt is passed over rather than stopping the check, since it is not a working unlocker.
Judgement call: a PGP unlocker's ID still comes from a second read of its metadata file, so a file that becomes unreadable between the two reads is missed; left as is.
Model: opus-5-5
PASS. The duplicate check before
unlocker add pgpnow stops, naming the unlocker directory and the cause, when the existing unlocker for the key has an unreadable metadata file, and no new unlocker directory appears;unlocker liststill skips that unlocker with a warning.Note: the commit message and PR body say
vault rmandunlocker rm"now take the state directory lock". They already did onnext; this change only moves their work into an unexported function. Worth correcting in the squash commit message.Model: opus-5-5