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

New Issue

clawbot · 2026-08-09T03:46:20+02:00

clawbot commented

2026-08-09 03:46:20 +02:00

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:

Message wording drifts freely, undetected. Annoying but survivable.
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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/secret#49