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

No make target runs the tests under -race, so no gate ever exercises the detector #23

New Issue
Closed
opened 2026-08-10 14:50:53 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-10 14:50:53 +02:00
Collaborator
Copy Link
Copy Source

Found during the adversarial review of
#21
(#21 (comment)).

The reviewer needed to check that handlers derived with WithAttrs/WithGroup
are safe when shared across goroutines, and found no make target or script/
entrypoint that runs the tests with -race. They had to invoke the toolchain
directly to do it, and disclosed the deviation.

That is a gate gap, not a reviewer problem. This library's handlers are shared
across goroutines by construction — that is what a process-wide slog default
is — so data races are exactly the defect class most worth catching here, and
nothing in the repo catches them. Raw toolchain invocation is disallowed across
this account precisely because the targets are supposed to carry the flags.

Definition of done

  • The tests run under -race as part of the repo's standard check, not as an
    optional extra target a person has to remember.
  • The race build works in whatever container the checks run in — -race needs
    cgo and a C toolchain, so an Alpine or CGO_ENABLED=0 stage will need
    attention rather than a flag.
  • A deliberately racy test is used as a negative control: confirm the detector
    actually fires and the check goes red, then remove it and confirm green. A
    -race flag that is present but not exercising anything is worse than none,
    because it reads as coverage.
  • The full check stays green and does not become materially slower than the
    suite justifies; if it does, say so rather than quietly accepting it.

Implementation requirements

  • Coordinate with #16 (STRTA scaffold
    divergence) and #20 (linting in
    Docker) — all three touch the entrypoints, and whichever lands later must not
    revert the others.
  • Landing commit title must end with (closes #<this issue>).
Found during the adversarial review of https://git.eeqj.de/sneak/simplelog/pulls/21 (https://git.eeqj.de/sneak/simplelog/pulls/21#issuecomment-53518). The reviewer needed to check that handlers derived with `WithAttrs`/`WithGroup` are safe when shared across goroutines, and found no `make` target or `script/` entrypoint that runs the tests with `-race`. They had to invoke the toolchain directly to do it, and disclosed the deviation. That is a gate gap, not a reviewer problem. This library&#39;s handlers are shared across goroutines by construction — that is what a process-wide `slog` default is — so data races are exactly the defect class most worth catching here, and nothing in the repo catches them. Raw toolchain invocation is disallowed across this account precisely because the targets are supposed to carry the flags. ## Definition of done - The tests run under `-race` as part of the repo&#39;s standard check, not as an optional extra target a person has to remember. - The race build works in whatever container the checks run in — `-race` needs cgo and a C toolchain, so an Alpine or `CGO_ENABLED=0` stage will need attention rather than a flag. - A deliberately racy test is used as a negative control: confirm the detector actually fires and the check goes red, then remove it and confirm green. A `-race` flag that is present but not exercising anything is worse than none, because it reads as coverage. - The full check stays green and does not become materially slower than the suite justifies; if it does, say so rather than quietly accepting it. ## Implementation requirements - Coordinate with https://git.eeqj.de/sneak/simplelog/issues/16 (STRTA scaffold divergence) and https://git.eeqj.de/sneak/simplelog/issues/20 (linting in Docker) — all three touch the entrypoints, and whichever lands later must not revert the others. - Landing commit title must end with ` (closes #<this issue>)`.
clawbot commented 2026-10-03 14:38:22 +02:00
Author
Collaborator
Copy Link
Copy Source

State, 2026-10-03 12:40 UTC: queued, not started. Runs after #16 and #20 have landed on next, because it edits the same script/ entrypoints and Dockerfile. The Dockerfile test stage uses a Debian-based golang image, so cgo for -race should be available there. The issue body is the brief.

Model: opus-5-5

State, 2026-10-03 12:40 UTC: queued, not started. Runs after https://git.eeqj.de/sneak/simplelog/issues/16 and https://git.eeqj.de/sneak/simplelog/issues/20 have landed on `next`, because it edits the same `script/` entrypoints and `Dockerfile`. The `Dockerfile` test stage uses a Debian-based `golang` image, so cgo for `-race` should be available there. The issue body is the brief. Model: opus-5-5
clawbot commented 2026-10-06 09:43:36 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan. Branches from next, which now runs the linter only in Docker (#20 has landed). The standard REPO_POLICIES.md in sneak/prompts makes testing a phase of the Dockerfile too, so this unit moves the tests there and turns on -race in the same step.

  1. script/test becomes a byte copy of https://git.eeqj.de/sneak/prompts/raw/branch/main/script/test: docker build --no-cache --target test, tagged simplelog-test. Nothing runs Go tests on the host any more.
  2. The Dockerfile test stage runs the standard command directly in place of make test: go test -timeout 90s -race -cover ./..., with the standard -v rerun on failure. Its base image is the Debian-based golang image, which has the C toolchain -race needs.
  3. The test stage stops depending on the lint stage, so script/test runs only the tests. A new last stage holds only the two ordering copies (COPY --from=lint, COPY --from=test), so script/cibuild's plain build still runs both phases, as the standard's build stage does.
  4. Negative control, run by the worker and repeated by the reviewer, never committed: a deliberately racy test makes script/test fail with a race report; removing it makes it pass.
  5. The PR says how long script/test takes with -race compared with before, so a slowdown is stated rather than absorbed.
  6. The README Entrypoints entry for script/test, and anything else that says tests run on the host, are brought in line.

Model: opus-5-5

Plan. Branches from `next`, which now runs the linter only in Docker (https://git.eeqj.de/sneak/simplelog/issues/20 has landed). The standard `REPO_POLICIES.md` in sneak/prompts makes testing a phase of the `Dockerfile` too, so this unit moves the tests there and turns on `-race` in the same step. 1. `script/test` becomes a byte copy of https://git.eeqj.de/sneak/prompts/raw/branch/main/script/test: `docker build --no-cache --target test`, tagged `simplelog-test`. Nothing runs Go tests on the host any more. 2. The `Dockerfile` test stage runs the standard command directly in place of `make test`: `go test -timeout 90s -race -cover ./...`, with the standard `-v` rerun on failure. Its base image is the Debian-based `golang` image, which has the C toolchain `-race` needs. 3. The test stage stops depending on the lint stage, so `script/test` runs only the tests. A new last stage holds only the two ordering copies (`COPY --from=lint`, `COPY --from=test`), so `script/cibuild`'s plain build still runs both phases, as the standard's build stage does. 4. Negative control, run by the worker and repeated by the reviewer, never committed: a deliberately racy test makes `script/test` fail with a race report; removing it makes it pass. 5. The PR says how long `script/test` takes with `-race` compared with before, so a slowdown is stated rather than absorbed. 6. The README Entrypoints entry for `script/test`, and anything else that says tests run on the host, are brought in line. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bot
Agent's turn to work
 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
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/simplelog#23