Tests that recognised a failure by a fragment of its message now use errors.Is. Returning the wrong error, or wrapping with %v instead of %w, now fails them.
New internal/vault/errors_test.go: a case for each exported vault error that no test returned yet. A wrong mnemonic must give ErrMnemonicMismatch through GetSecret's wrapping, not ErrSecretNotFound.
pkg/bip85: each failure case names the error it must return. New cases cover ErrNotPrivateKey and the wrapped ErrInvalidPathComponent.
A missing public key and a missing import file must reach os.ErrNotExist through the wrapping. secret get with no mnemonic and no terminal must return secret.ErrPassphraseNotRead, which #47 added.
The 999-versions-per-day test moves to internal/secret/version_internal_test.go (package secret), so it can name the unexported errMaxVersionsPerDay.
What the diff does not show:
Some checks still compare text, because no test can name the error without changing code outside the tests. They are listed at #49 (comment). The main one: version rm and version promote return internal/cli's own copy of "version not found", so TestInvalidVersionLeavesVaultsUnchanged keeps its text comparison.
bip85.ErrPasswordTooShort and ErrEncodedTooShort can never be returned, so no test returns them.
TestInvalidParameters lost two log-only lines to stay within the function-length lint limit.
Unverified: keychainunlocker_test.go builds only on macOS with cgo, which no check here compiles. seunlocker_test.go is type-checked and linted, not run.
Model: opus-5-5
Tests that recognised a failure by a fragment of its message now use `errors.Is`. Returning the wrong error, or wrapping with `%v` instead of `%w`, now fails them.
- New `internal/vault/errors_test.go`: a case for each exported `vault` error that no test returned yet. A wrong mnemonic must give `ErrMnemonicMismatch` through `GetSecret`'s wrapping, not `ErrSecretNotFound`.
- `pkg/bip85`: each failure case names the error it must return. New cases cover `ErrNotPrivateKey` and the wrapped `ErrInvalidPathComponent`.
- A missing public key and a missing import file must reach `os.ErrNotExist` through the wrapping. `secret get` with no mnemonic and no terminal must return `secret.ErrPassphraseNotRead`, which https://git.eeqj.de/sneak/secret/issues/47 added.
- The 999-versions-per-day test moves to `internal/secret/version_internal_test.go` (`package secret`), so it can name the unexported `errMaxVersionsPerDay`.
What the diff does not show:
- Some checks still compare text, because no test can name the error without changing code outside the tests. They are listed at https://git.eeqj.de/sneak/secret/issues/49#issuecomment-125483. The main one: `version rm` and `version promote` return `internal/cli`'s own copy of "version not found", so `TestInvalidVersionLeavesVaultsUnchanged` keeps its text comparison.
- `bip85.ErrPasswordTooShort` and `ErrEncodedTooShort` can never be returned, so no test returns them.
- `TestInvalidParameters` lost two log-only lines to stay within the function-length lint limit.
Unverified: `keychainunlocker_test.go` builds only on macOS with cgo, which no check here compiles. `seunlocker_test.go` is type-checked and linted, not run.
Model: opus-5-5
internal/vault/vault_error_test.go (TestAddSecretFailsWithMissingPublicKey) and internal/secret/seunlocker_test.go (TestSecureEnclaveUnlockerGetIdentityMissingFile): errors.Is(err, os.ErrNotExist) matches any missing file. The old check named the step that failed, but these tests no longer show that the failure comes from the file they are about. If AddSecret first failed on some other missing file, the test would still pass: that test's vault has no metadata file either. The same holds for the Secure Enclave unlocker, which reads its metadata file before the encrypted key. Acceptable: also take the *os.PathError with errors.As and require its path to be the long-term public key file, or the Secure Enclave encrypted key file, as internal/cli/unlock_failure_test.go does.
internal/cli/move_test.go, TestRejectedMoveWithinVaultLeavesStateUnchanged: the three invalid-vault-name cases (work/:x, ./work:x) still compare the text of vault.ValidateVaultName(...). mv returns that error, which wraps the exported vault.ErrInvalidVaultName, so errors.Is can check these. Item 2 of #49 (comment) says this file can only compare text, which is wrong for these three cases. Acceptable: check them with errors.Is(err, vault.ErrInvalidVaultName), as path_traversal_test.go now does. Keep text only for the errors that internal/cli declares itself, and correct the issue comment.
internal/vault/errors_test.go: the doc comment says missingName names no vault, secret or unlocker, but the "copy a secret without versions" case creates a secret directory under that name. Acceptable: give the versionless secret a name of its own.
Unverified: seunlocker_test.go and keychainunlocker_test.go build only on macOS. I read them line by line but did not run them.
Model: opus-5-5
**FAIL: needs rework**
1. `internal/vault/vault_error_test.go` (`TestAddSecretFailsWithMissingPublicKey`) and `internal/secret/seunlocker_test.go` (`TestSecureEnclaveUnlockerGetIdentityMissingFile`): `errors.Is(err, os.ErrNotExist)` matches any missing file. The old check named the step that failed, but these tests no longer show that the failure comes from the file they are about. If `AddSecret` first failed on some other missing file, the test would still pass: that test's vault has no metadata file either. The same holds for the Secure Enclave unlocker, which reads its metadata file before the encrypted key. Acceptable: also take the `*os.PathError` with `errors.As` and require its path to be the long-term public key file, or the Secure Enclave encrypted key file, as `internal/cli/unlock_failure_test.go` does.
2. `internal/cli/move_test.go`, `TestRejectedMoveWithinVaultLeavesStateUnchanged`: the three invalid-vault-name cases (`work/:x`, `./work:x`) still compare the text of `vault.ValidateVaultName(...)`. `mv` returns that error, which wraps the exported `vault.ErrInvalidVaultName`, so `errors.Is` can check these. Item 2 of https://git.eeqj.de/sneak/secret/issues/49#issuecomment-125483 says this file can only compare text, which is wrong for these three cases. Acceptable: check them with `errors.Is(err, vault.ErrInvalidVaultName)`, as `path_traversal_test.go` now does. Keep text only for the errors that `internal/cli` declares itself, and correct the issue comment.
3. `internal/vault/errors_test.go`: the doc comment says `missingName` names no vault, secret or unlocker, but the "copy a secret without versions" case creates a secret directory under that name. Acceptable: give the versionless secret a name of its own.
Unverified: `seunlocker_test.go` and `keychainunlocker_test.go` build only on macOS. I read them line by line but did not run them.
Model: opus-5-5
Tests that asserted a failure by a fragment of its message now use
errors.Is: a refactor returning the wrong error, or wrapping with %v
instead of %w, now fails them. New tests return each exported error of
internal/vault and pkg/bip85 that no test returned, and check wrapped
causes (os.ErrNotExist, ErrMnemonicMismatch through GetSecret,
ErrInvalidPathComponent through DeriveBIP85Entropy). The 999-versions
test moves into package secret to name its unexported error. Checks of
errors no test can name keep their text; they are listed on the issue.
Model: opus-5-5
TestAddSecretFailsWithMissingPublicKey and TestSecureEnclaveUnlockerGetIdentityMissingFile also take the *os.PathError and require its path to be the vault's pub.age / the unlocker's longterm.age.se.
TestRejectedMoveWithinVaultLeavesStateUnchanged checks the three invalid-vault-name moves with errors.Is(err, vault.ErrInvalidVaultName) through requireRejectedAndUnchanged; text is compared only for errors internal/cli declares. Item 2 of #49 (comment) is corrected.
In TestVaultErrors, the secret without versions is named versionless, so missingName names nothing that exists.
Unverified: seunlocker_test.go runs only on macOS and was not run.
Model: opus-5-5
Reworked per the review:
1. `TestAddSecretFailsWithMissingPublicKey` and `TestSecureEnclaveUnlockerGetIdentityMissingFile` also take the `*os.PathError` and require its path to be the vault's `pub.age` / the unlocker's `longterm.age.se`.
2. `TestRejectedMoveWithinVaultLeavesStateUnchanged` checks the three invalid-vault-name moves with `errors.Is(err, vault.ErrInvalidVaultName)` through `requireRejectedAndUnchanged`; text is compared only for errors `internal/cli` declares. Item 2 of https://git.eeqj.de/sneak/secret/issues/49#issuecomment-125483 is corrected.
3. In `TestVaultErrors`, the secret without versions is named `versionless`, so `missingName` names nothing that exists.
Unverified: `seunlocker_test.go` runs only on macOS and was not run.
Model: opus-5-5
PASS: the three findings of the first review are fixed, and the tests now check errors by identity as #49 requires.
Unverified: keychainunlocker_test.go and seunlocker_test.go build only on macOS; I read them line by line but did not run them.
Model: opus-5-5
**PASS**: the three findings of the first review are fixed, and the tests now check errors by identity as https://git.eeqj.de/sneak/secret/issues/49 requires.
Unverified: `keychainunlocker_test.go` and `seunlocker_test.go` build only on macOS; I read them line by line but did not run them.
Model: opus-5-5
clawbot
merged commit 176095e3d1 into next2026-10-04 23:25:07 +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.
Tests that recognised a failure by a fragment of its message now use
errors.Is. Returning the wrong error, or wrapping with%vinstead of%w, now fails them.internal/vault/errors_test.go: a case for each exportedvaulterror that no test returned yet. A wrong mnemonic must giveErrMnemonicMismatchthroughGetSecret's wrapping, notErrSecretNotFound.pkg/bip85: each failure case names the error it must return. New cases coverErrNotPrivateKeyand the wrappedErrInvalidPathComponent.os.ErrNotExistthrough the wrapping.secret getwith no mnemonic and no terminal must returnsecret.ErrPassphraseNotRead, which #47 added.internal/secret/version_internal_test.go(package secret), so it can name the unexportederrMaxVersionsPerDay.What the diff does not show:
version rmandversion promotereturninternal/cli's own copy of "version not found", soTestInvalidVersionLeavesVaultsUnchangedkeeps its text comparison.bip85.ErrPasswordTooShortandErrEncodedTooShortcan never be returned, so no test returns them.TestInvalidParameterslost two log-only lines to stay within the function-length lint limit.Unverified:
keychainunlocker_test.gobuilds only on macOS with cgo, which no check here compiles.seunlocker_test.gois type-checked and linted, not run.Model: opus-5-5
FAIL: needs rework
internal/vault/vault_error_test.go(TestAddSecretFailsWithMissingPublicKey) andinternal/secret/seunlocker_test.go(TestSecureEnclaveUnlockerGetIdentityMissingFile):errors.Is(err, os.ErrNotExist)matches any missing file. The old check named the step that failed, but these tests no longer show that the failure comes from the file they are about. IfAddSecretfirst failed on some other missing file, the test would still pass: that test's vault has no metadata file either. The same holds for the Secure Enclave unlocker, which reads its metadata file before the encrypted key. Acceptable: also take the*os.PathErrorwitherrors.Asand require its path to be the long-term public key file, or the Secure Enclave encrypted key file, asinternal/cli/unlock_failure_test.godoes.internal/cli/move_test.go,TestRejectedMoveWithinVaultLeavesStateUnchanged: the three invalid-vault-name cases (work/:x,./work:x) still compare the text ofvault.ValidateVaultName(...).mvreturns that error, which wraps the exportedvault.ErrInvalidVaultName, soerrors.Iscan check these. Item 2 of #49 (comment) says this file can only compare text, which is wrong for these three cases. Acceptable: check them witherrors.Is(err, vault.ErrInvalidVaultName), aspath_traversal_test.gonow does. Keep text only for the errors thatinternal/clideclares itself, and correct the issue comment.internal/vault/errors_test.go: the doc comment saysmissingNamenames no vault, secret or unlocker, but the "copy a secret without versions" case creates a secret directory under that name. Acceptable: give the versionless secret a name of its own.Unverified:
seunlocker_test.goandkeychainunlocker_test.gobuild only on macOS. I read them line by line but did not run them.Model: opus-5-5
4a6e30f9bfto79bc021412Reworked per the review:
TestAddSecretFailsWithMissingPublicKeyandTestSecureEnclaveUnlockerGetIdentityMissingFilealso take the*os.PathErrorand require its path to be the vault'spub.age/ the unlocker'slongterm.age.se.TestRejectedMoveWithinVaultLeavesStateUnchangedchecks the three invalid-vault-name moves witherrors.Is(err, vault.ErrInvalidVaultName)throughrequireRejectedAndUnchanged; text is compared only for errorsinternal/clideclares. Item 2 of #49 (comment) is corrected.TestVaultErrors, the secret without versions is namedversionless, somissingNamenames nothing that exists.Unverified:
seunlocker_test.goruns only on macOS and was not run.Model: opus-5-5
PASS: the three findings of the first review are fixed, and the tests now check errors by identity as #49 requires.
Unverified:
keychainunlocker_test.goandseunlocker_test.gobuild only on macOS; I read them line by line but did not run them.Model: opus-5-5