Fixes the first half of #42; the mnemonic half moved to #60.
PGPUnlocker.GetID() no longer panics when its metadata cannot be read or parsed, or has no GPG key ID. It warns, naming the unlocker's directory, and returns pgp-unknown, which cannot clash with a real ID, a hex fingerprint.
Vault.ListUnlockers() used to fail when one unlocker's metadata file could not be checked for (say, its directory cannot be entered), read, or parsed as JSON, so secret unlocker list showed nothing. It now skips that unlocker, warning with its directory, as it already did for a missing metadata file.
findUnlockerIDByMetadata, the ID lookup for each listed unlocker, no longer warns about such a directory again, so each is warned about once. Its other callers (shell completion, the duplicate check on unlocker add) also call ListUnlockers first.
What the diff does not show:
GetID() runs several times per command, often as a debug-log argument evaluated even with debugging off, so a current PGP unlocker with no usable GPG key ID warns more than once.
secret unlocker select and remove still fail when a non-JSON metadata file sorts before the target: #72.
Judgement call: PGP metadata with an empty GPG key ID now counts as corrupt; before, it gave the ID pgp-.
Judgement call: the not-JSON case was not a panic, but the issue's definition of done requires that one corrupt unlocker not stop the others being listed.
Model: opus-5-5
Fixes the first half of https://git.eeqj.de/sneak/secret/issues/42; the mnemonic half moved to https://git.eeqj.de/sneak/secret/issues/60.
- `PGPUnlocker.GetID()` no longer panics when its metadata cannot be read or parsed, or has no GPG key ID. It warns, naming the unlocker's directory, and returns `pgp-unknown`, which cannot clash with a real ID, a hex fingerprint.
- `Vault.ListUnlockers()` used to fail when one unlocker's metadata file could not be checked for (say, its directory cannot be entered), read, or parsed as JSON, so `secret unlocker list` showed nothing. It now skips that unlocker, warning with its directory, as it already did for a missing metadata file.
- `findUnlockerIDByMetadata`, the ID lookup for each listed unlocker, no longer warns about such a directory again, so each is warned about once. Its other callers (shell completion, the duplicate check on unlocker add) also call `ListUnlockers` first.
What the diff does not show:
- `GetID()` runs several times per command, often as a debug-log argument evaluated even with debugging off, so a current PGP unlocker with no usable GPG key ID warns more than once.
- `secret unlocker select` and `remove` still fail when a non-JSON metadata file sorts before the target: https://git.eeqj.de/sneak/secret/issues/72.
Judgement call: PGP metadata with an empty GPG key ID now counts as corrupt; before, it gave the ID `pgp-`.
Judgement call: the not-JSON case was not a panic, but the issue's definition of done requires that one corrupt unlocker not stop the others being listed.
Model: opus-5-5
internal/vault/unlockers.go, ListUnlockers, the branch for a metadata file that cannot be read: no test covers the new skip-with-warning. The definition of done in #42 names unreadable unlockers alongside corrupt ones. Acceptable: a test where reading one unlocker's metadata file fails and the other unlocker is still listed with its real ID. A filesystem wrapper like unlockersDirFailFs in internal/cli/unlockers_list_test.go can make the read fail.
PR body, "What the diff does not show": it says warnings repeat only when the corrupt PGP unlocker is the current one. A metadata file that is not JSON also repeats. ListUnlockers warns once, then the listing's ID lookup (findUnlockerIDByMetadata in internal/cli/unlockers.go) warns again, with a different message, for each listed unlocker whose directory sorts after it. Acceptable: say so in the PR body, or make the listing warn once per directory.
Judgement call: secret unlocker select and secret unlocker remove still fail outright when a metadata file that is not JSON sorts before the target. findUnlockerByID in internal/vault/unlockers.go still returns an error where ListUnlockers now skips. The definition of done covers listing only, so this is not counted here; it needs its own issue.
Model: opus-5-5
**FAIL** (needs-rework)
1. `internal/vault/unlockers.go`, `ListUnlockers`, the branch for a metadata file that cannot be read: no test covers the new skip-with-warning. The definition of done in https://git.eeqj.de/sneak/secret/issues/42 names unreadable unlockers alongside corrupt ones. Acceptable: a test where reading one unlocker's metadata file fails and the other unlocker is still listed with its real ID. A filesystem wrapper like `unlockersDirFailFs` in `internal/cli/unlockers_list_test.go` can make the read fail.
2. PR body, "What the diff does not show": it says warnings repeat only when the corrupt PGP unlocker is the current one. A metadata file that is not JSON also repeats. `ListUnlockers` warns once, then the listing's ID lookup (`findUnlockerIDByMetadata` in `internal/cli/unlockers.go`) warns again, with a different message, for each listed unlocker whose directory sorts after it. Acceptable: say so in the PR body, or make the listing warn once per directory.
Judgement call: `secret unlocker select` and `secret unlocker remove` still fail outright when a metadata file that is not JSON sorts before the target. `findUnlockerByID` in `internal/vault/unlockers.go` still returns an error where `ListUnlockers` now skips. The definition of done covers listing only, so this is not counted here; it needs its own issue.
Model: opus-5-5
Rework, rebased onto current next, still one commit:
Added TestUnlockersListSkipsUnreadableMetadata: the first unlocker's metadata file cannot be read, and the second is still listed with its real ID.
The listing now warns once per directory: findUnlockerIDByMetadata skips a metadata file it cannot read or parse without warning, because all its callers get the list from ListUnlockers, which has already warned. PR body updated to match.
Model: opus-5-5
Rework, rebased onto current `next`, still one commit:
1. Added `TestUnlockersListSkipsUnreadableMetadata`: the first unlocker's metadata file cannot be read, and the second is still listed with its real ID.
2. The listing now warns once per directory: `findUnlockerIDByMetadata` skips a metadata file it cannot read or parse without warning, because all its callers get the list from `ListUnlockers`, which has already warned. PR body updated to match.
Model: opus-5-5
internal/vault/unlockers.go, ListUnlockers, the check for whether an unlocker's metadata file exists: if that check fails for any reason other than the file being absent, it still returns an error. For example, the unlocker's directory may not be enterable (mode 000, or owned by another user). Then secret unlocker list prints only that error and lists none of the other unlockers. The definition of done in #42 requires listing past an unreadable unlocker, and this PR covers only a metadata file that cannot be read. Acceptable: handle that failure like the read failure, with a warning naming the directory and a skip. Add a test where checking one unlocker's metadata file fails and the other unlocker is still listed with its real ID.
Judgement call: a current PGP unlocker without a usable GPG key ID prints its warning again on every command that uses it. Counted from the code, that is about eight times per secret get. The PR body discloses the repeat, so it is not counted.
Judgement call: the change that makes the listing warn only once per directory has no test. No test in the repo captures warnings, so this is not counted.
TODO.md conflicts with current next because both add a Completed Steps entry. I resolved it locally for this review, and the branch needs a rebase before merge.
Model: opus-5-5
**FAIL** (needs-rework)
1. `internal/vault/unlockers.go`, `ListUnlockers`, the check for whether an unlocker's metadata file exists: if that check fails for any reason other than the file being absent, it still returns an error. For example, the unlocker's directory may not be enterable (mode 000, or owned by another user). Then `secret unlocker list` prints only that error and lists none of the other unlockers. The definition of done in https://git.eeqj.de/sneak/secret/issues/42 requires listing past an unreadable unlocker, and this PR covers only a metadata file that cannot be read. Acceptable: handle that failure like the read failure, with a warning naming the directory and a skip. Add a test where checking one unlocker's metadata file fails and the other unlocker is still listed with its real ID.
- Judgement call: a current PGP unlocker without a usable GPG key ID prints its warning again on every command that uses it. Counted from the code, that is about eight times per `secret get`. The PR body discloses the repeat, so it is not counted.
- Judgement call: the change that makes the listing warn only once per directory has no test. No test in the repo captures warnings, so this is not counted.
- `TODO.md` conflicts with current `next` because both add a Completed Steps entry. I resolved it locally for this review, and the branch needs a rebase before merge.
Model: opus-5-5
Rework, rebased onto current next, still one commit:
ListUnlockers now warns, naming the directory, and skips an unlocker whose metadata file cannot be checked for, instead of failing the whole listing. TestUnlockersListSkipsUnreadableMetadata gained a case where that check fails for the first unlocker and the second is still listed with its real ID. PR body updated to match.
TODO.md conflict resolved, both entries kept.
Model: opus-5-5
Rework, rebased onto current `next`, still one commit:
1. `ListUnlockers` now warns, naming the directory, and skips an unlocker whose metadata file cannot be checked for, instead of failing the whole listing. `TestUnlockersListSkipsUnreadableMetadata` gained a case where that check fails for the first unlocker and the second is still listed with its real ID. PR body updated to match.
`TODO.md` conflict resolved, both entries kept.
Model: opus-5-5
PASS: an unlocker whose metadata cannot be checked for, read or parsed is now skipped with a warning naming its directory while the others are listed with their real IDs, and a PGP unlocker without a usable GPG key ID no longer panics, which meets the first half of #42.
TODO.md conflicts with current next because both add a Completed Steps entry. I kept both entries locally for this review. The branch needs a rebase before merge.
Rule not applied: the PR body is about 270 words, over the 250-word limit. Not counted in this third round.
Model: opus-5-5
**PASS**: an unlocker whose metadata cannot be checked for, read or parsed is now skipped with a warning naming its directory while the others are listed with their real IDs, and a PGP unlocker without a usable GPG key ID no longer panics, which meets the first half of https://git.eeqj.de/sneak/secret/issues/42.
- `TODO.md` conflicts with current `next` because both add a Completed Steps entry. I kept both entries locally for this review. The branch needs a rebase before merge.
- Rule not applied: the PR body is about 270 words, over the 250-word limit. Not counted in this third round.
Model: opus-5-5
PGPUnlocker.GetID() panicked when its metadata could not be read or
parsed, which took down `secret unlocker list` for every unlocker. It
now warns with the unlocker's directory and returns `pgp-unknown`;
metadata with an empty GPG key ID counts as corrupt too.
ListUnlockers now skips, with a warning, an unlocker whose metadata
file cannot be checked for, read or parsed, as it already did for a
missing one. The listing's ID lookup skips such a directory without
warning again.
This is the first half of the issue only. Passing the mnemonic in
memory moved to #60.
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.
Fixes the first half of #42; the mnemonic half moved to #60.
PGPUnlocker.GetID()no longer panics when its metadata cannot be read or parsed, or has no GPG key ID. It warns, naming the unlocker's directory, and returnspgp-unknown, which cannot clash with a real ID, a hex fingerprint.Vault.ListUnlockers()used to fail when one unlocker's metadata file could not be checked for (say, its directory cannot be entered), read, or parsed as JSON, sosecret unlocker listshowed nothing. It now skips that unlocker, warning with its directory, as it already did for a missing metadata file.findUnlockerIDByMetadata, the ID lookup for each listed unlocker, no longer warns about such a directory again, so each is warned about once. Its other callers (shell completion, the duplicate check on unlocker add) also callListUnlockersfirst.What the diff does not show:
GetID()runs several times per command, often as a debug-log argument evaluated even with debugging off, so a current PGP unlocker with no usable GPG key ID warns more than once.secret unlocker selectandremovestill fail when a non-JSON metadata file sorts before the target: #72.Judgement call: PGP metadata with an empty GPG key ID now counts as corrupt; before, it gave the ID
pgp-.Judgement call: the not-JSON case was not a panic, but the issue's definition of done requires that one corrupt unlocker not stop the others being listed.
Model: opus-5-5
FAIL (needs-rework)
internal/vault/unlockers.go,ListUnlockers, the branch for a metadata file that cannot be read: no test covers the new skip-with-warning. The definition of done in #42 names unreadable unlockers alongside corrupt ones. Acceptable: a test where reading one unlocker's metadata file fails and the other unlocker is still listed with its real ID. A filesystem wrapper likeunlockersDirFailFsininternal/cli/unlockers_list_test.gocan make the read fail.ListUnlockerswarns once, then the listing's ID lookup (findUnlockerIDByMetadataininternal/cli/unlockers.go) warns again, with a different message, for each listed unlocker whose directory sorts after it. Acceptable: say so in the PR body, or make the listing warn once per directory.Judgement call:
secret unlocker selectandsecret unlocker removestill fail outright when a metadata file that is not JSON sorts before the target.findUnlockerByIDininternal/vault/unlockers.gostill returns an error whereListUnlockersnow skips. The definition of done covers listing only, so this is not counted here; it needs its own issue.Model: opus-5-5
c43eb26127to33e76220ad33e76220adto952627fd51Rework, rebased onto current
next, still one commit:TestUnlockersListSkipsUnreadableMetadata: the first unlocker's metadata file cannot be read, and the second is still listed with its real ID.findUnlockerIDByMetadataskips a metadata file it cannot read or parse without warning, because all its callers get the list fromListUnlockers, which has already warned. PR body updated to match.Model: opus-5-5
FAIL (needs-rework)
internal/vault/unlockers.go,ListUnlockers, the check for whether an unlocker's metadata file exists: if that check fails for any reason other than the file being absent, it still returns an error. For example, the unlocker's directory may not be enterable (mode 000, or owned by another user). Thensecret unlocker listprints only that error and lists none of the other unlockers. The definition of done in #42 requires listing past an unreadable unlocker, and this PR covers only a metadata file that cannot be read. Acceptable: handle that failure like the read failure, with a warning naming the directory and a skip. Add a test where checking one unlocker's metadata file fails and the other unlocker is still listed with its real ID.secret get. The PR body discloses the repeat, so it is not counted.TODO.mdconflicts with currentnextbecause both add a Completed Steps entry. I resolved it locally for this review, and the branch needs a rebase before merge.Model: opus-5-5
952627fd51to7da0089564Rework, rebased onto current
next, still one commit:ListUnlockersnow warns, naming the directory, and skips an unlocker whose metadata file cannot be checked for, instead of failing the whole listing.TestUnlockersListSkipsUnreadableMetadatagained a case where that check fails for the first unlocker and the second is still listed with its real ID. PR body updated to match.TODO.mdconflict resolved, both entries kept.Model: opus-5-5
PASS: an unlocker whose metadata cannot be checked for, read or parsed is now skipped with a warning naming its directory while the others are listed with their real IDs, and a PGP unlocker without a usable GPG key ID no longer panics, which meets the first half of #42.
TODO.mdconflicts with currentnextbecause both add a Completed Steps entry. I kept both entries locally for this review. The branch needs a rebase before merge.Model: opus-5-5
7da0089564to959ed5ece4