Pin CLI error messages through their real call sites, not reconstructed format strings #87

New Issue

clawbot · 2026-08-09T04:32:43+02:00

clawbot commented

2026-08-09 04:32:43 +02:00

Context

Follow-up from the PR #59 re-review (non-blocking findings NB1 and NB2).
Two tests added during that PR's rework do not test what they are credited
with testing. Both were found by mutation testing — deliberately
reintroducing the bug and confirming the suite still passed.

NB1 — internal/cli/errmsg_test.go copy-pastes the production format
strings into the test rather than driving the real call sites. It asserts
that a string literal in the test renders the way the same string literal in
the test says it should. A future reword of, for example, check.go:141
would leave this test passing while the user-visible message changed — which
is precisely the regression the test was added to prevent.

mfer/errmsg_test.go already does this correctly by driving ValidatePath
and asserting on what actually comes back. The CLI test should follow it.

NB2 — TestFreshenRecordEntryMtimePresence cannot distinguish the bug it
guards. The stub's mtime is 1_700_000_000. An absent mtime misread as
the epoch also compares unequal to that value, so it also lands in the
"changed" branch. Restoring recordEntry to its exact pre-rework expression
— the original B5 bug — passes the entire suite.

The underlying protection does exist: removing the guard from entryMtime
itself fails two other tests. So the nil-mtime behaviour is covered. It is
just not covered by the test named for it, which is worse than an absent
test because it advertises a guarantee it does not provide.

Definition of done

internal/cli/errmsg_test.go asserts on error text produced by invoking
the real functions — verifyRequiredSigner, generateManifestOperation,
downloadFile, and the other affected call sites — not on format strings
restated in the test file.
No production format string is duplicated in a test file. If a message is
worth pinning, it is pinned by observing the function that emits it.
TestFreshenRecordEntryMtimePresence's stub mtime is time.Unix(0, 0) so
that an absent mtime and an epoch mtime are distinguishable, and the test
fails if recordEntry's nil guard is removed.
Both mutations are verified to fail the suite:
1. Restore recordEntry to its pre-rework expression while leaving
  entryMtime intact — the suite must fail.
2. Reword any one of the eleven pinned messages in production code — the
  suite must fail.
make check passes and docker build . succeeds. TODO.md updated in
the same commit.

Implementation requirements

Run both mutations and confirm each fails before declaring done. State
in the PR body that you ran them and what failed. A test that has not been
seen to fail is not evidence of anything — that is the entire lesson of
this issue.
Do not weaken the assertions to make the tests easier to write. Full-string
equality, not strings.Contains, unless the message legitimately embeds a
variable path or count — in which case assert on the full string with a
known fixed value substituted.
Do not change any production error message text while doing this. If a
message looks wrong, file it; wording changes are #84's scope.
Commit title must end with (closes #87).

## Context Follow-up from the PR #59 re-review (non-blocking findings NB1 and NB2). Two tests added during that PR's rework do not test what they are credited with testing. Both were found by mutation testing — deliberately reintroducing the bug and confirming the suite still passed. **NB1 — `internal/cli/errmsg_test.go` copy-pastes the production format strings into the test** rather than driving the real call sites. It asserts that a string literal in the test renders the way the same string literal in the test says it should. A future reword of, for example, `check.go:141` would leave this test passing while the user-visible message changed — which is precisely the regression the test was added to prevent. `mfer/errmsg_test.go` already does this correctly by driving `ValidatePath` and asserting on what actually comes back. The CLI test should follow it. **NB2 — `TestFreshenRecordEntryMtimePresence` cannot distinguish the bug it guards.** The stub's mtime is `1_700_000_000`. An absent mtime misread as the epoch also compares unequal to that value, so it also lands in the "changed" branch. Restoring `recordEntry` to its exact pre-rework expression — the original B5 bug — passes the entire suite. The underlying protection does exist: removing the guard from `entryMtime` itself fails two other tests. So the nil-mtime behaviour is covered. It is just not covered by the test named for it, which is worse than an absent test because it advertises a guarantee it does not provide. ## Definition of done - `internal/cli/errmsg_test.go` asserts on error text produced by invoking the real functions — `verifyRequiredSigner`, `generateManifestOperation`, `downloadFile`, and the other affected call sites — not on format strings restated in the test file. - No production format string is duplicated in a test file. If a message is worth pinning, it is pinned by observing the function that emits it. - `TestFreshenRecordEntryMtimePresence`'s stub mtime is `time.Unix(0, 0)` so that an absent mtime and an epoch mtime are distinguishable, and the test fails if `recordEntry`'s nil guard is removed. - Both mutations are verified to fail the suite: 1. Restore `recordEntry` to its pre-rework expression while leaving `entryMtime` intact — the suite must fail. 2. Reword any one of the eleven pinned messages in production code — the suite must fail. - `make check` passes and `docker build .` succeeds. `TODO.md` updated in the same commit. ## Implementation requirements - Run both mutations and confirm each fails **before** declaring done. State in the PR body that you ran them and what failed. A test that has not been seen to fail is not evidence of anything — that is the entire lesson of this issue. - Do not weaken the assertions to make the tests easier to write. Full-string equality, not `strings.Contains`, unless the message legitimately embeds a variable path or count — in which case assert on the full string with a known fixed value substituted. - Do not change any production error message text while doing this. If a message looks wrong, file it; wording changes are #84's scope. - Commit title must end with ` (closes #87)`.

clawbot added this to the 1.0.0 milestone 2026-08-09 04:32:43 +02:00

clawbot referenced this issue

2026-08-09 04:33:04 +02:00

Update golangci-lint to v2.12.2 with canonical config (closes #60) #59

clawbot referenced this issue

2026-08-09 07:18:54 +02:00

Configure prettier and make fmt-check cover markdown (closes #69) #88

Sign in to join this conversation.