sneak/secret
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Tests assert on error message fragments, never on sentinel identity — error-handling regressions pass silently #49

New Issue
Closed
opened 2026-08-09 03:46:20 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:46:20 +02:00
Collaborator
Copy Link
Copy Source

Surfaced by the adversarial review of PR #29 (see #29 (comment)). Filed separately because it is a standing weakness in the suite, not a defect of that PR.

Problem

The err113 refactor introduced eleven exported sentinel errors, primarily in the new internal/vault/errors.go. There is not a single errors.Is or errors.As call in any test file in the repository.

Error assertions are instead loose substring checks — assert.Contains on a fragment of the message text. The review demonstrated the concrete consequence: sixteen user-visible error messages had their interpolated values moved to the tail (secret X not found becoming secret not found: X), and every affected test still passed, because the asserted fragment survives the reorder. The suite could not have caught the regression it exists to catch.

Two distinct failure modes follow from this, and the second is the one that matters:

  1. Message wording drifts freely, undetected. Annoying but survivable.
  2. Sentinel identity is untested. Nothing verifies that ErrSecretNotFound is what actually comes back rather than some other error whose text happens to contain the same words. A refactor that returns the wrong sentinel, wraps with %v instead of %w and severs the chain, or collapses two distinct conditions into one error passes green. In this tool the branches that depend on distinguishing errors include "secret does not exist" versus "secret exists but could not be decrypted" — conflating those is the difference between a clear message and a user concluding their data is corrupt.

Substring assertions also make the messages themselves load-bearing: a wording improvement (#47 proposes several) breaks tests for no good reason, which creates pressure to leave bad messages alone.

Definition of done

  • Tests that assert on a specific failure condition use errors.Is (or errors.As where the type carries data), not assert.Contains on the message.
  • Every exported sentinel in internal/vault/errors.go and elsewhere has at least one test proving the function under test actually returns it.
  • Wrapping is verified where it matters: for errors that wrap a cause, a test proves errors.Is still reaches the underlying error, so a future %w-to-%v regression is caught.
  • Where a test genuinely needs to pin user-facing message text — the recovery-guidance messages in #47 are the real case — it does so deliberately and separately from the identity assertion, so the two concerns fail independently and it is obvious which broke.
  • No test asserts on a message fragment as a proxy for identity.
  • make check green. TODO.md updated in the same commit.

Implementation requirements

  • Sequence after PR #29 lands. It rewrites large parts of the test suite; doing this first guarantees conflicts.
  • Coordinate with #47, which changes error message text. If #47 lands first, this work must not reintroduce text-fragment coupling; if this lands first, #47 becomes markedly safer to do — which is an argument for this order.
  • This is test-only work. Do not change non-test code to make an assertion convenient. If a function returns an error that cannot be identified by errors.Is because it was never given a sentinel, that is a real finding: report it on this issue rather than quietly adding a sentinel as a side effect.
  • Do not chase a coverage number. The target is that each distinguishable failure condition is asserted by identity, not that every line is executed.
  • Check whether testifylint in the canonical config has a rule covering error-assertion style. If it does and it is currently satisfied by the loose assertions, note that on this issue — it would mean the linter is not helping here and the gap is purely a review-discipline matter.
Surfaced by the adversarial review of PR #29 (see https://git.eeqj.de/sneak/secret/pulls/29#issuecomment-45591). Filed separately because it is a standing weakness in the suite, not a defect of that PR. ## Problem The `err113` refactor introduced eleven exported sentinel errors, primarily in the new `internal/vault/errors.go`. **There is not a single `errors.Is` or `errors.As` call in any test file in the repository.** Error assertions are instead loose substring checks — `assert.Contains` on a fragment of the message text. The review demonstrated the concrete consequence: sixteen user-visible error messages had their interpolated values moved to the tail (`secret X not found` becoming `secret not found: X`), and **every affected test still passed**, because the asserted fragment survives the reorder. The suite could not have caught the regression it exists to catch. Two distinct failure modes follow from this, and the second is the one that matters: 1. Message wording drifts freely, undetected. Annoying but survivable. 2. **Sentinel identity is untested.** Nothing verifies that `ErrSecretNotFound` is what actually comes back rather than some other error whose text happens to contain the same words. A refactor that returns the wrong sentinel, wraps with `%v` instead of `%w` and severs the chain, or collapses two distinct conditions into one error passes green. In this tool the branches that depend on distinguishing errors include "secret does not exist" versus "secret exists but could not be decrypted" — conflating those is the difference between a clear message and a user concluding their data is corrupt. Substring assertions also make the messages themselves load-bearing: a wording improvement (#47 proposes several) breaks tests for no good reason, which creates pressure to leave bad messages alone. ## Definition of done - Tests that assert on a specific failure condition use `errors.Is` (or `errors.As` where the type carries data), not `assert.Contains` on the message. - Every exported sentinel in `internal/vault/errors.go` and elsewhere has at least one test proving the function under test actually returns it. - Wrapping is verified where it matters: for errors that wrap a cause, a test proves `errors.Is` still reaches the underlying error, so a future `%w`-to-`%v` regression is caught. - Where a test genuinely needs to pin user-facing message text — the recovery-guidance messages in #47 are the real case — it does so **deliberately and separately** from the identity assertion, so the two concerns fail independently and it is obvious which broke. - No test asserts on a message fragment as a proxy for identity. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Sequence **after** PR #29 lands. It rewrites large parts of the test suite; doing this first guarantees conflicts. - Coordinate with #47, which changes error message text. If #47 lands first, this work must not reintroduce text-fragment coupling; if this lands first, #47 becomes markedly safer to do — which is an argument for this order. - This is test-only work. **Do not change non-test code to make an assertion convenient.** If a function returns an error that cannot be identified by `errors.Is` because it was never given a sentinel, that is a real finding: report it on this issue rather than quietly adding a sentinel as a side effect. - Do not chase a coverage number. The target is that each distinguishable failure condition is asserted by identity, not that every line is executed. - Check whether `testifylint` in the canonical config has a rule covering error-assertion style. If it does and it is currently satisfied by the loose assertions, note that on this issue — it would mean the linter is not helping here and the gap is purely a review-discipline matter.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:46:20 +02:00
clawbot commented 2026-10-04 22:06:41 +02:00
Author
Collaborator
Copy Link
Copy Source

Findings: failures a test cannot identify with errors.Is unless code outside the tests changes. Per the issue, no error values were added. These checks still compare message text, so none of them got weaker.

  1. One failure, two different errors. internal/cli declares its own copies of four vault errors: errSecretNotFound, errVaultDoesNotExist, errVersionNotFound and errSecretExistsNoForce, for the same failures as vault.ErrSecretNotFound, ErrVaultNotFound, ErrVersionNotFound and ErrSecretExists. Which one comes back depends on the command. secret get --version X returns vault.ErrVersionNotFound, but secret version promote and secret version rm return the copy, so errors.Is(err, vault.ErrVersionNotFound) is false for them. TestInvalidVersionLeavesVaultsUnchanged passed only because both texts are equal. It still compares text, with a comment saying why. The same split hits secret mv (a missing source; an existing destination in the same vault), secret vault import and secret vault remove (a missing vault), and secret version list (a missing secret).
  2. internal/cli's errors are unexported, and the tests that reach them through commands are in package cli_test (move_test.go, integration_test.go), so for these errors they can only compare text. That includes errMoveOntoItself, which has no vault counterpart. An exported error a command passes on is still checked with errors.Is: secret mv with an invalid vault name returns one wrapping vault.ErrInvalidVaultName.
  3. No terminal and no passphrase, outside an unlocker. A passphrase unlocker that cannot ask for its passphrase now wraps secret.ErrPassphraseNotRead (from #47). secret init and secret vault create call secret.ReadPassphrase themselves, and it returns only unexported errors (errStdinNotTerminal, errStderrNotTerminal), so TestStopAtPassphrasePromptLeavesNothing still matches "failed to read passphrase". Separately, test23ErrorHandling runs the built binary, so it can only read the output, which it still checks for "failed to unlock".
  4. Unknown PGP key. secret.ResolveGPGKeyFingerprint has no error for a key the keyring does not hold; it wraps gpg's exit status. TestAddPGPUnlockerUnknownKey still matches "failed to resolve GPG key fingerprint".
  5. Keychain nil data. storeInKeychain (macOS with cgo) returns a plain fmt.Errorf("data buffer is nil") rather than errNilDataBuffer, which has the same text. TestKeychainNilData still matches text.
  6. Unreachable errors. bip85.ErrPasswordTooShort and bip85.ErrEncodedTooShort can never be returned. The check before them caps the requested length at 86 and 80, and 64 bytes of entropy always encode to 86 Base64 or 80 Base85 characters. No test can produce them.

testifylint: the canonical config enables it with its default checks (v1.6.4, in golangci-lint v2.12.2). None of them flags matching an error's text. error-is-as only catches assert.Error(t, err, errX) and assert.True(t, errors.Is(...)), and require-error accepts EqualError and ErrorContains. The loose assertions satisfy the linter, so keeping tests on error identity is up to review.

Model: opus-5-5

Findings: failures a test cannot identify with `errors.Is` unless code outside the tests changes. Per the issue, no error values were added. These checks still compare message text, so none of them got weaker. 1. **One failure, two different errors.** `internal/cli` declares its own copies of four `vault` errors: `errSecretNotFound`, `errVaultDoesNotExist`, `errVersionNotFound` and `errSecretExistsNoForce`, for the same failures as `vault.ErrSecretNotFound`, `ErrVaultNotFound`, `ErrVersionNotFound` and `ErrSecretExists`. Which one comes back depends on the command. `secret get --version X` returns `vault.ErrVersionNotFound`, but `secret version promote` and `secret version rm` return the copy, so `errors.Is(err, vault.ErrVersionNotFound)` is false for them. `TestInvalidVersionLeavesVaultsUnchanged` passed only because both texts are equal. It still compares text, with a comment saying why. The same split hits `secret mv` (a missing source; an existing destination in the same vault), `secret vault import` and `secret vault remove` (a missing vault), and `secret version list` (a missing secret). 2. **`internal/cli`'s errors are unexported**, and the tests that reach them through commands are in `package cli_test` (`move_test.go`, `integration_test.go`), so for these errors they can only compare text. That includes `errMoveOntoItself`, which has no `vault` counterpart. An exported error a command passes on is still checked with `errors.Is`: `secret mv` with an invalid vault name returns one wrapping `vault.ErrInvalidVaultName`. 3. **No terminal and no passphrase, outside an unlocker.** A passphrase unlocker that cannot ask for its passphrase now wraps `secret.ErrPassphraseNotRead` (from https://git.eeqj.de/sneak/secret/issues/47). `secret init` and `secret vault create` call `secret.ReadPassphrase` themselves, and it returns only unexported errors (`errStdinNotTerminal`, `errStderrNotTerminal`), so `TestStopAtPassphrasePromptLeavesNothing` still matches "failed to read passphrase". Separately, `test23ErrorHandling` runs the built binary, so it can only read the output, which it still checks for "failed to unlock". 4. **Unknown PGP key.** `secret.ResolveGPGKeyFingerprint` has no error for a key the keyring does not hold; it wraps gpg's exit status. `TestAddPGPUnlockerUnknownKey` still matches "failed to resolve GPG key fingerprint". 5. **Keychain nil data.** `storeInKeychain` (macOS with cgo) returns a plain `fmt.Errorf("data buffer is nil")` rather than `errNilDataBuffer`, which has the same text. `TestKeychainNilData` still matches text. 6. **Unreachable errors.** `bip85.ErrPasswordTooShort` and `bip85.ErrEncodedTooShort` can never be returned. The check before them caps the requested length at 86 and 80, and 64 bytes of entropy always encode to 86 Base64 or 80 Base85 characters. No test can produce them. **testifylint:** the canonical config enables it with its default checks (v1.6.4, in golangci-lint v2.12.2). None of them flags matching an error's text. `error-is-as` only catches `assert.Error(t, err, errX)` and `assert.True(t, errors.Is(...))`, and `require-error` accepts `EqualError` and `ErrorContains`. The loose assertions satisfy the linter, so keeping tests on error identity is up to review. Model: opus-5-5
clawbot commented 2026-10-04 22:06:53 +02:00
Author
Collaborator
Copy Link
Copy Source

Built in #112: tests recognise failures with errors.Is instead of message text, and every exported error that can be returned has a test that returns it. The checks that still compare text are the ones in #49 (comment).

Model: opus-5-5

Built in https://git.eeqj.de/sneak/secret/pulls/112: tests recognise failures with `errors.Is` instead of message text, and every exported error that can be returned has a test that returns it. The checks that still compare text are the ones in https://git.eeqj.de/sneak/secret/issues/49#issuecomment-125483. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/secret#49