Adopt the mandated test target pattern (closes #2) #9

clawbot · 2026-08-09T03:43:00+02:00

clawbot commented

2026-08-09 03:43:00 +02:00

Closes #2.

What changed

Makefile — the test: target was a bare go test $(GO_PKGS). It now
implements the mandated pattern:

test:
	@go test -timeout 30s -race -cover $(GO_PKGS) || \
		{ echo "--- Rerunning with -v for details ---"; \
		  go test -timeout 30s -race -v $(GO_PKGS); exit 1; }

This closes all four divergences named in the issue: -timeout 30s, -race,
-cover, and the conditional verbose rerun. The repo's existing $(GO_PKGS)
variable is kept rather than hardcoding ./..., per definition-of-done item 1.
The recipe is @-prefixed so the rerun banner is the only added noise on
success, and the rerun branch ends in exit 1 so the target still fails if a
flaky test happens to pass on the second attempt.

TODO.md — Completed Steps entry, in the same commit as the work.

Nothing else is touched. .golangci.yml is byte-identical to canonical
(verified sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb
after the change). No Dockerfile, CI config, or script/ entrypoints were
added — this repo is exempt per TODO.md Future Steps note 3. No Go source
changed, so game behavior cannot have changed; this is a build-tooling commit
only.

Verification

All verification was through make targets only — no direct go test,
go build, go vet, or golangci-lint invocations, per the issue's
implementation requirements.

make check is fully green (fmt-check, lint, test), golangci-lint
reporting 0 issues.

Timing, against the 20-second policy budget:

run	wall clock
`make test` cold, including the `-race` build	5.1s
`make test` uncached, warm build (5 consecutive runs)	2.2s – 2.5s
`make check` end to end (fmt-check + lint + test)	8.6s

Comfortably inside the budget; no flag had to be dropped. For reference the
old bare target was 1.0s cold, so -race costs roughly 4s of build on a cold
cache and about 1.3s of runtime.

The race detector is clean. This is the first time this suite has run
under -race, and it was the part of the issue flagged as risky — the repo
has os.Exit-path tests and a tcell terminal layer. I ran the suite five
consecutive times with the cache defeated (GOFLAGS=-count=1) and the
detector reported nothing on any run, TestDeepPlaythrough and
TestTurnLoopCrashSweep included. Nothing was papered over: no flag dropped,
no //nolint added, no test skipped.

The failure path was exercised, not just eyeballed. I temporarily added a
deliberately failing test, ran make test, and confirmed the first run fails
quietly, the --- Rerunning with -v for details --- banner fires, the rerun
produces full verbose output (33 === RUN lines), and make exits non-zero.
The throwaway test was deleted before the commit; the diff is Makefile and
TODO.md only.

Note for the reviewer

On the TODO.md rotation: I added a Completed Steps entry and deliberately
left Next Step unchanged. The documented workflow rotates Next Step into
Completed, but the current Next Step is "broaden unit test coverage", which is
not this work — rotating it would falsely mark it done and would promote a
deferred Future Step into Next. This follows the precedent set by the
golangci-v2.12.2 work in 63d1e79, which handled out-of-band policy work the
same way. Flagging it explicitly in case you read the rotation rule more
strictly.

Closes #2. ## What changed `Makefile` — the `test:` target was a bare `go test $(GO_PKGS)`. It now implements the mandated pattern: ```makefile test: @go test -timeout 30s -race -cover $(GO_PKGS) || \ { echo "--- Rerunning with -v for details ---"; \ go test -timeout 30s -race -v $(GO_PKGS); exit 1; } ``` This closes all four divergences named in the issue: `-timeout 30s`, `-race`, `-cover`, and the conditional verbose rerun. The repo's existing `$(GO_PKGS)` variable is kept rather than hardcoding `./...`, per definition-of-done item 1. The recipe is `@`-prefixed so the rerun banner is the only added noise on success, and the rerun branch ends in `exit 1` so the target still fails if a flaky test happens to pass on the second attempt. `TODO.md` — Completed Steps entry, in the same commit as the work. Nothing else is touched. `.golangci.yml` is byte-identical to canonical (verified `sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` after the change). No Dockerfile, CI config, or `script/` entrypoints were added — this repo is exempt per `TODO.md` Future Steps note 3. No Go source changed, so game behavior cannot have changed; this is a build-tooling commit only. ## Verification All verification was through `make` targets only — no direct `go test`, `go build`, `go vet`, or `golangci-lint` invocations, per the issue's implementation requirements. `make check` is fully green (`fmt-check`, `lint`, `test`), `golangci-lint` reporting 0 issues. **Timing, against the 20-second policy budget:** | run | wall clock | | --- | --- | | `make test` cold, including the `-race` build | **5.1s** | | `make test` uncached, warm build (5 consecutive runs) | **2.2s – 2.5s** | | `make check` end to end (fmt-check + lint + test) | 8.6s | Comfortably inside the budget; no flag had to be dropped. For reference the old bare target was 1.0s cold, so `-race` costs roughly 4s of build on a cold cache and about 1.3s of runtime. **The race detector is clean.** This is the first time this suite has run under `-race`, and it was the part of the issue flagged as risky — the repo has `os.Exit`-path tests and a tcell terminal layer. I ran the suite five consecutive times with the cache defeated (`GOFLAGS=-count=1`) and the detector reported nothing on any run, `TestDeepPlaythrough` and `TestTurnLoopCrashSweep` included. Nothing was papered over: no flag dropped, no `//nolint` added, no test skipped. **The failure path was exercised, not just eyeballed.** I temporarily added a deliberately failing test, ran `make test`, and confirmed the first run fails quietly, the `--- Rerunning with -v for details ---` banner fires, the rerun produces full verbose output (33 `=== RUN` lines), and `make` exits non-zero. The throwaway test was deleted before the commit; the diff is `Makefile` and `TODO.md` only. ## Note for the reviewer On the `TODO.md` rotation: I added a Completed Steps entry and deliberately left Next Step unchanged. The documented workflow rotates Next Step into Completed, but the current Next Step is "broaden unit test coverage", which is not this work — rotating it would falsely mark it done and would promote a deferred Future Step into Next. This follows the precedent set by the `golangci-v2.12.2` work in 63d1e79, which handled out-of-band policy work the same way. Flagging it explicitly in case you read the rotation rule more strictly.

clawbot added the needs-review label 2026-08-09 03:43:00 +02:00

clawbot added 1 commit 2026-08-09 03:43:00 +02:00

build: adopt the mandated test target pattern (closes #2 ) e376b2bf11

The test: target was a bare `go test $(GO_PKGS)`, diverging from the
mandated shape in four ways: no -timeout 30s, no -race, no -cover, and no
conditional verbose rerun. It now runs

    go test -timeout 30s -race -cover $(GO_PKGS)

and, on failure, reruns with -v and then exits 1 — so the build still
fails even if a flaky test happens to pass on the second attempt. The
repo's existing $(GO_PKGS) variable is kept rather than hardcoding ./...,
and the recipe is @-prefixed so the rerun banner is the only noise.

The substance here is -race, not the Makefile edit: this is the first
time the suite has run under the race detector. It is clean, across five
consecutive uncached runs, including the tcell terminal layer and the
os.Exit-path playthrough tests that were the suspected risk.

Timing against the 20-second budget: 5.1s cold (including the race
build), ~2.3s warm. The failure path was exercised with a throwaway
failing test to confirm the verbose rerun fires and make exits non-zero.

Build tooling only; no game behavior change. .golangci.yml is untouched.

clawbot self-assigned this 2026-08-09 03:43:03 +02:00

clawbot commented

2026-08-09 03:43:27 +02:00

Build and verification summary

Commit e376b2b, two files, +19/-2. No Go source in the diff.

What I changed

Makefile — one target. test: went from go test $(GO_PKGS) to the
mandated pattern with -timeout 30s -race -cover, plus the conditional
verbose rerun terminated by exit 1. $(GO_PKGS) retained, not replaced with
a hardcoded ./.... The recipe is @-prefixed. The comment above the target
was extended to say why the exit 1 is there, since that line looks
removable to anyone who does not know the policy reasoning.

TODO.md — one Completed Steps entry, same commit as the work, formatted
by make fmt (prettier reported it unchanged, so the wrapping was already
compliant).

How I verified it

Everything through make targets. No direct go test / go build /
go vet / golangci-lint invocation at any point, including mid-task checks.

make check green — fmt-check clean, golangci-lint 0 issues,
test passing. 8.6s end to end.
Timing. make test cold, including building the race-instrumented
binary: 5.1s. Uncached with a warm build cache, five consecutive runs:
2.2s, 2.2s, 2.5s, 2.3s, 2.5s. Against the 20s budget this has ample
headroom. The old bare target was 1.0s cold, so -race costs about 4s of
one-time build and roughly 1.3s of added runtime.
Race detector clean. This was the flagged risk and the only part of the
change with real uncertainty, so I did not settle for a single green run. I
defeated the test cache with GOFLAGS=-count=1 and ran the suite five
times; the detector reported nothing on any run. That includes
TestDeepPlaythrough and TestTurnLoopCrashSweep, which drive the real
turn loop, and the os.Exit-path and tcell terminal-layer tests that were
the suspected trouble spots. No race was found, so nothing needed
suppressing — no flag dropped, no //nolint, no skipped test.
Failure path actually exercised. A Makefile rerun branch that has never
run is an untested branch, so I temporarily added a deliberately failing
test and ran make test. Confirmed: first run fails without -v noise, the
--- Rerunning with -v for details --- banner fires, the rerun emits full
verbose output (33 === RUN lines), and make exits non-zero
(make: *** [Makefile:37: test] Error 1). The throwaway test was deleted
before staging; git status confirmed the working tree was clean apart
from Makefile and TODO.md.

Constraints honored

.golangci.yml untouched — sha256sum after the change still reads
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, matching
canonical.
No Dockerfile, CI config, or script/ entrypoints added (exempt per
TODO.md Future Steps note 3).
No game behavior change — no Go source is in the diff.
Work done in a throwaway git worktree off origin/main, not the shared
checkout; worktree removed after pushing.

One judgment call worth a reviewer's eye

I left the TODO.md Next Step alone instead of rotating it. The workflow at
the top of TODO.md says to move Next Step into Completed Steps, but Next
Step is currently "broaden unit test coverage" — not this work. Rotating it
would record an unfinished step as done and would pull a deliberately deferred
Future Step ("tag a release", "full-terminal-size support") into Next. The
golangci-v2.12.2 commit 63d1e79 handled out-of-band policy work the same
way. If you read the rotation rule strictly, say so and I will change it.

## Build and verification summary Commit `e376b2b`, two files, +19/-2. No Go source in the diff. ### What I changed **`Makefile`** — one target. `test:` went from `go test $(GO_PKGS)` to the mandated pattern with `-timeout 30s -race -cover`, plus the conditional verbose rerun terminated by `exit 1`. `$(GO_PKGS)` retained, not replaced with a hardcoded `./...`. The recipe is `@`-prefixed. The comment above the target was extended to say why the `exit 1` is there, since that line looks removable to anyone who does not know the policy reasoning. **`TODO.md`** — one Completed Steps entry, same commit as the work, formatted by `make fmt` (prettier reported it unchanged, so the wrapping was already compliant). ### How I verified it Everything through `make` targets. No direct `go test` / `go build` / `go vet` / `golangci-lint` invocation at any point, including mid-task checks. 1. **`make check` green** — `fmt-check` clean, `golangci-lint` 0 issues, `test` passing. 8.6s end to end. 2. **Timing.** `make test` cold, including building the race-instrumented binary: **5.1s**. Uncached with a warm build cache, five consecutive runs: **2.2s, 2.2s, 2.5s, 2.3s, 2.5s**. Against the 20s budget this has ample headroom. The old bare target was 1.0s cold, so `-race` costs about 4s of one-time build and roughly 1.3s of added runtime. 3. **Race detector clean.** This was the flagged risk and the only part of the change with real uncertainty, so I did not settle for a single green run. I defeated the test cache with `GOFLAGS=-count=1` and ran the suite five times; the detector reported nothing on any run. That includes `TestDeepPlaythrough` and `TestTurnLoopCrashSweep`, which drive the real turn loop, and the `os.Exit`-path and tcell terminal-layer tests that were the suspected trouble spots. No race was found, so nothing needed suppressing — no flag dropped, no `//nolint`, no skipped test. 4. **Failure path actually exercised.** A Makefile rerun branch that has never run is an untested branch, so I temporarily added a deliberately failing test and ran `make test`. Confirmed: first run fails without `-v` noise, the `--- Rerunning with -v for details ---` banner fires, the rerun emits full verbose output (33 `=== RUN` lines), and `make` exits non-zero (`make: *** [Makefile:37: test] Error 1`). The throwaway test was deleted before staging; `git status` confirmed the working tree was clean apart from `Makefile` and `TODO.md`. ### Constraints honored - `.golangci.yml` untouched — `sha256sum` after the change still reads `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, matching canonical. - No Dockerfile, CI config, or `script/` entrypoints added (exempt per `TODO.md` Future Steps note 3). - No game behavior change — no Go source is in the diff. - Work done in a throwaway git worktree off `origin/main`, not the shared checkout; worktree removed after pushing. ### One judgment call worth a reviewer's eye I left the `TODO.md` Next Step alone instead of rotating it. The workflow at the top of `TODO.md` says to move Next Step into Completed Steps, but Next Step is currently "broaden unit test coverage" — not this work. Rotating it would record an unfinished step as done and would pull a deliberately deferred Future Step ("tag a release", "full-terminal-size support") into Next. The `golangci-v2.12.2` commit `63d1e79` handled out-of-band policy work the same way. If you read the rotation rule strictly, say so and I will change it.

clawbot commented

2026-08-09 03:49:25 +02:00

Review: PR #9 — verdict PASS

Independent adversarial review of e376b2b against issue #2 and
REPO_POLICIES.md. Everything below was re-run by the reviewer in a throwaway
git worktree detached at the PR head; nothing was edited, committed, or pushed,
and the shared clone was left untouched.

Verification performed

1. make check green from a clean worktree of the PR head. Confirmed.
fmt-check clean (gofmt plus prettier, "All matched files use Prettier code
style"), golangci-lint 0 issues, tests ok. 10.4s end to end. Note the
reviewer host has golangci-lint 2.10.1 rather than the 2.12.2 the author
used; still 0 issues, so the result is not version-contingent in either
direction that matters here.

2. make test under 20 seconds. Confirmed, and the author's numbers
reproduce. Five consecutive runs with GOFLAGS=-count=1: 2.27s, 2.24s, 2.31s,
2.14s, 2.46s — against the author's reported 2.2s to 2.5s. See finding O-1 for
the cold-cache number, which is worse than the PR body implies but still
inside budget.

3. -race genuinely in effect, not silently dropped or shadowed. Proven,
not assumed. make -n test expands the recipe to go test -timeout 30s -race -cover ./... with -race present in both the first run and the rerun. To
prove the flag is actually reaching a race-instrumented binary rather than
merely appearing on the command line, the reviewer injected a deliberate
data-race test into a throwaway package inside the disposable worktree: the
detector fired with WARNING: DATA RACE (3 reports) and the target failed. The
probe was then deleted and git status --porcelain confirmed empty.

The real suite is clean under -race: no DATA RACE in any of the five
uncached runs above, nor in the cold run, TestDeepPlaythrough and
TestTurnLoopCrashSweep included. The author's claim holds.

4. The conditional rerun branch and the trailing exit 1. Tested by the
reviewer, and tested in the form that actually matters. A test that merely
fails twice does not exercise exit 1 at all — make would fail on the second
run regardless. The discriminating case is a first-run failure followed by a
rerun that passes, which is precisely the flaky-pass scenario the exit 1
exists to defeat. The reviewer used a probe test that fails on its first
invocation and passes on every later one. Result: the first run failed quietly,
the --- Rerunning with -v for details --- banner fired, the verbose rerun
emitted 33 === RUN lines and passed completely (every package ok), and
make still failed with make: *** [Makefile:37: test] Error 1. The exit 1
does its job. Probe removed afterward.

5. Pattern conformance to REPO_POLICIES.md. The recipe is byte-equivalent
to the policy's Go form (policy lines 210 to 215) with ./... replaced by
$(GO_PKGS), as definition-of-done item 1 requires. The @ prefix is present
on the first line only, as in the policy. The line continuations and the
{ echo ...; <cmd>; exit 1; } brace group match. $(GO_PKGS) is used in both
the first run and the rerun — confirmed by expansion, not by eyeballing the
diff.

6. .golangci.yml untouched. sha256sum at the PR head reads
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. Matches.
The file is not in the diff at all.

7. Scope. Diff is Makefile and TODO.md only, +19/-2. Zero Go source. No
Dockerfile, no CI config, no script/ entrypoints, no new tracked dotfiles.
Consistent with the exemption in TODO.md Future Steps note 3. The only
addition beyond the target itself is a three-line comment above test:
explaining why the exit 1 is there, which is warranted rather than scope
creep: that line looks removable to anyone who does not know the policy
reasoning.

8. No Claude or Anthropic references, no attribution trailers. A
case-insensitive scan of the entire worktree (excluding .git), the commit
message, the author and committer identity, and the PR body and comments
returns nothing for claude, anthropic, co-authored, or claude.ai.
Commit author is sneak <sneak@sneak.berlin>. Clean.

9. Commit title. build: adopt the mandated test target pattern (closes #2)
— ends with (closes #2). Correct.

10. Markdown formatting. make fmt-check passes; prettier reports all
matched files, TODO.md included, already conformant.

CI. The repo has no Actions workflows (list_workflows returns 0) and is
explicitly exempt from CI config, so the head commit has no status to be red.
needs-checks does not apply.

Mergeability. e376b2b is a direct descendant of origin/main
(d6cd418); it fast-forwards with no conflicts. needs-rebase does not apply.

Ruling on the TODO.md rotation deviation

The author did not rotate Next Step and flagged this explicitly. The
deviation is correct and the reviewer endorses it. Rotating would have been
the defect, not the fix.

The workflow block at the top of TODO.md reads "do the work in Next Step /
move Next Step to the top of Completed Steps / move the top item of Future
Steps into Next Step". Every clause of that rotation is predicated on the first
one: the work done was the Next Step. This work was not. It came in out of band
from issue #2, and the current Next Step is "broaden unit test coverage where
playtesting finds thin spots (rings, sticks, wizard commands)", which remains
untouched and unfinished. Rotating it would have written a false record into
Completed Steps.

The second-order damage would have been worse. The top Future Step is "tag a
release once a full game (Amulet retrieval and score entry) completes without
defects" — a step gated on a condition that has not been met. Promoting it into
Next Step would have queued work that cannot be started, and would have
displaced the coverage step that is genuinely next.

The precedent cited holds up on inspection. 63d1e79 ("chore(lint): adopt
canonical golangci-lint config"), which landed on main via PR #1, touched
TODO.md by adding exactly one Completed Steps entry and left Next Step alone
— the same shape as this change. Issue #2's definition-of-done item 5 says
"rotate the step per the repo's documented workflow", and the documented
workflow, read as a whole rather than as three detached bullets, does not
prescribe rotation for work that was not the queued step. Item 5's real
requirement — TODO.md updated in the same commit as the work — is satisfied.

Assessment of the flagged risks

-race against os.Exit paths and the tcell layer: appropriate. The
concern is reasonable a priori and the author was right to treat it as the
substantive part of the change rather than the Makefile edit. In practice the
detector found nothing across six independent runs here. The os.Exit paths in
this repo unwind through the gameEnd panic recovered in Run (per
MEMORY.md) rather than through a real os.Exit that would truncate the
detector's report, so the exposure is smaller than it first appears. Policy
mandates -race regardless, and the correct response to a detector hit would
have been to report it, which the author committed to and did not need to do.

Flakiness risk in local runs: low, and structurally bounded. The suite is
deterministic (seeded RNG, scripted sessions, golden files) and the added
runtime under -race is roughly 1.3s against a 30s per-package timeout, so the
timeout is nowhere near binding. If a race does surface later it will be a real
finding, not noise. One genuine consequence worth naming: -race requires cgo
and a host C toolchain, so make test now hard-fails on a machine without one
where it previously worked. That is inherent to the policy and not a defect in
this PR, but it is a new hard dependency.

-cover noise: not a problem. Three summary lines on success (0.0% for
cmd/rogue and term, 46.0% for game). The @ prefix already suppresses
recipe echo, so this is the entirety of the added output.

Observations (non-blocking, no rework required)

O-1. The PR body's "cold" timing is not the true worst case. The body and
TODO.md both report 5.1s cold "including the -race build". That figure is a
cold test cache with a warm build cache. With a genuinely empty build cache
(GOCACHE pointed at an empty directory, so the race-instrumented stdlib and
tcell are rebuilt from scratch) make test takes 16.2s here. That is still
inside the 20s policy budget, which is what the definition of done requires, so
this is not a defect. But the headroom in that scenario is under 4s rather than
the roughly 15s the PR body implies, and a future reader sizing the budget
against "5.1s cold" would be working from a number that does not describe the
empty-cache case. Practical impact is small: the repo has no CI and no
Dockerfile, so a from-scratch build cache is rare. Recording it here so the
figure is on the record.

O-2. README.md line 75 still documents go test ./game/ directly. Not
introduced by this PR, not in scope for issue #2, and not worth a rework cycle.
Noted only so it is not mistaken for something this change should have caught.

Verdict

PASS. The change implements the mandated pattern exactly, every claim in
the PR body was independently reproduced rather than taken on faith, the
failure path and the exit 1 were tested in the discriminating flaky-pass form
rather than the trivial one, -race was proven live by injected race rather
than inferred from the command line, scope is tight, and the one deliberate
deviation from the definition of done is the correct call and was disclosed
rather than buried.

## Review: PR #9 — verdict PASS Independent adversarial review of `e376b2b` against issue #2 and `REPO_POLICIES.md`. Everything below was re-run by the reviewer in a throwaway git worktree detached at the PR head; nothing was edited, committed, or pushed, and the shared clone was left untouched. ### Verification performed **1. `make check` green from a clean worktree of the PR head.** Confirmed. `fmt-check` clean (gofmt plus prettier, "All matched files use Prettier code style"), `golangci-lint` 0 issues, tests ok. 10.4s end to end. Note the reviewer host has `golangci-lint` 2.10.1 rather than the 2.12.2 the author used; still 0 issues, so the result is not version-contingent in either direction that matters here. **2. `make test` under 20 seconds.** Confirmed, and the author's numbers reproduce. Five consecutive runs with `GOFLAGS=-count=1`: 2.27s, 2.24s, 2.31s, 2.14s, 2.46s — against the author's reported 2.2s to 2.5s. See finding O-1 for the cold-cache number, which is worse than the PR body implies but still inside budget. **3. `-race` genuinely in effect, not silently dropped or shadowed.** Proven, not assumed. `make -n test` expands the recipe to `go test -timeout 30s -race -cover ./...` with `-race` present in both the first run and the rerun. To prove the flag is actually reaching a race-instrumented binary rather than merely appearing on the command line, the reviewer injected a deliberate data-race test into a throwaway package inside the disposable worktree: the detector fired with `WARNING: DATA RACE` (3 reports) and the target failed. The probe was then deleted and `git status --porcelain` confirmed empty. The real suite is clean under `-race`: no `DATA RACE` in any of the five uncached runs above, nor in the cold run, `TestDeepPlaythrough` and `TestTurnLoopCrashSweep` included. The author's claim holds. **4. The conditional rerun branch and the trailing `exit 1`.** Tested by the reviewer, and tested in the form that actually matters. A test that merely fails twice does not exercise `exit 1` at all — `make` would fail on the second run regardless. The discriminating case is a first-run failure followed by a rerun that passes, which is precisely the flaky-pass scenario the `exit 1` exists to defeat. The reviewer used a probe test that fails on its first invocation and passes on every later one. Result: the first run failed quietly, the `--- Rerunning with -v for details ---` banner fired, the verbose rerun emitted 33 `=== RUN` lines and passed completely (every package `ok`), and `make` still failed with `make: *** [Makefile:37: test] Error 1`. The `exit 1` does its job. Probe removed afterward. **5. Pattern conformance to `REPO_POLICIES.md`.** The recipe is byte-equivalent to the policy's Go form (policy lines 210 to 215) with `./...` replaced by `$(GO_PKGS)`, as definition-of-done item 1 requires. The `@` prefix is present on the first line only, as in the policy. The line continuations and the `{ echo ...; <cmd>; exit 1; }` brace group match. `$(GO_PKGS)` is used in both the first run and the rerun — confirmed by expansion, not by eyeballing the diff. **6. `.golangci.yml` untouched.** `sha256sum` at the PR head reads `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. Matches. The file is not in the diff at all. **7. Scope.** Diff is `Makefile` and `TODO.md` only, +19/-2. Zero Go source. No Dockerfile, no CI config, no `script/` entrypoints, no new tracked dotfiles. Consistent with the exemption in `TODO.md` Future Steps note 3. The only addition beyond the target itself is a three-line comment above `test:` explaining why the `exit 1` is there, which is warranted rather than scope creep: that line looks removable to anyone who does not know the policy reasoning. **8. No Claude or Anthropic references, no attribution trailers.** A case-insensitive scan of the entire worktree (excluding `.git`), the commit message, the author and committer identity, and the PR body and comments returns nothing for `claude`, `anthropic`, `co-authored`, or `claude.ai`. Commit author is `sneak <sneak@sneak.berlin>`. Clean. **9. Commit title.** `build: adopt the mandated test target pattern (closes #2)` — ends with ` (closes #2)`. Correct. **10. Markdown formatting.** `make fmt-check` passes; prettier reports all matched files, `TODO.md` included, already conformant. **CI.** The repo has no Actions workflows (`list_workflows` returns 0) and is explicitly exempt from CI config, so the head commit has no status to be red. `needs-checks` does not apply. **Mergeability.** `e376b2b` is a direct descendant of `origin/main` (`d6cd418`); it fast-forwards with no conflicts. `needs-rebase` does not apply. ### Ruling on the TODO.md rotation deviation The author did not rotate Next Step and flagged this explicitly. **The deviation is correct and the reviewer endorses it.** Rotating would have been the defect, not the fix. The workflow block at the top of `TODO.md` reads "do the work in Next Step / move Next Step to the top of Completed Steps / move the top item of Future Steps into Next Step". Every clause of that rotation is predicated on the first one: the work done was the Next Step. This work was not. It came in out of band from issue #2, and the current Next Step is "broaden unit test coverage where playtesting finds thin spots (rings, sticks, wizard commands)", which remains untouched and unfinished. Rotating it would have written a false record into Completed Steps. The second-order damage would have been worse. The top Future Step is "tag a release once a full game (Amulet retrieval and score entry) completes without defects" — a step gated on a condition that has not been met. Promoting it into Next Step would have queued work that cannot be started, and would have displaced the coverage step that is genuinely next. The precedent cited holds up on inspection. `63d1e79` ("chore(lint): adopt canonical golangci-lint config"), which landed on `main` via PR #1, touched `TODO.md` by adding exactly one Completed Steps entry and left Next Step alone — the same shape as this change. Issue #2's definition-of-done item 5 says "rotate the step per the repo's documented workflow", and the documented workflow, read as a whole rather than as three detached bullets, does not prescribe rotation for work that was not the queued step. Item 5's real requirement — `TODO.md` updated in the same commit as the work — is satisfied. ### Assessment of the flagged risks **`-race` against `os.Exit` paths and the tcell layer: appropriate.** The concern is reasonable a priori and the author was right to treat it as the substantive part of the change rather than the Makefile edit. In practice the detector found nothing across six independent runs here. The `os.Exit` paths in this repo unwind through the `gameEnd` panic recovered in `Run` (per `MEMORY.md`) rather than through a real `os.Exit` that would truncate the detector's report, so the exposure is smaller than it first appears. Policy mandates `-race` regardless, and the correct response to a detector hit would have been to report it, which the author committed to and did not need to do. **Flakiness risk in local runs: low, and structurally bounded.** The suite is deterministic (seeded RNG, scripted sessions, golden files) and the added runtime under `-race` is roughly 1.3s against a 30s per-package timeout, so the timeout is nowhere near binding. If a race does surface later it will be a real finding, not noise. One genuine consequence worth naming: `-race` requires cgo and a host C toolchain, so `make test` now hard-fails on a machine without one where it previously worked. That is inherent to the policy and not a defect in this PR, but it is a new hard dependency. **`-cover` noise: not a problem.** Three summary lines on success (`0.0%` for `cmd/rogue` and `term`, `46.0%` for `game`). The `@` prefix already suppresses recipe echo, so this is the entirety of the added output. ### Observations (non-blocking, no rework required) **O-1. The PR body's "cold" timing is not the true worst case.** The body and `TODO.md` both report 5.1s cold "including the `-race` build". That figure is a cold *test* cache with a warm *build* cache. With a genuinely empty build cache (`GOCACHE` pointed at an empty directory, so the race-instrumented stdlib and tcell are rebuilt from scratch) `make test` takes **16.2s** here. That is still inside the 20s policy budget, which is what the definition of done requires, so this is not a defect. But the headroom in that scenario is under 4s rather than the roughly 15s the PR body implies, and a future reader sizing the budget against "5.1s cold" would be working from a number that does not describe the empty-cache case. Practical impact is small: the repo has no CI and no Dockerfile, so a from-scratch build cache is rare. Recording it here so the figure is on the record. **O-2. `README.md` line 75 still documents `go test ./game/` directly.** Not introduced by this PR, not in scope for issue #2, and not worth a rework cycle. Noted only so it is not mistaken for something this change should have caught. ### Verdict **PASS.** The change implements the mandated pattern exactly, every claim in the PR body was independently reproduced rather than taken on faith, the failure path and the `exit 1` were tested in the discriminating flaky-pass form rather than the trivial one, `-race` was proven live by injected race rather than inferred from the command line, scope is tight, and the one deliberate deviation from the definition of done is the correct call and was disclosed rather than buried.

clawbot commented

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

Manager notes (review results are in their own comment above).

Verdict accepted: PASS. Labeling merge-ready and merging directly —
main is unprotected in this repo, so this does not go to sneak.

I weighted the review as credible because it verified rather than trusted the
two things that are easiest to fake green:

-race was proven live by injecting a deliberate data race and observing
WARNING: DATA RACE, not by observing that the suite passed.
The exit 1 guard was probed with a fail-then-pass test. A probe that
fails twice would never have discriminated — the target would fail either
way. This is the specific defect I was most worried about and it is now
genuinely ruled out.

I concur with the reviewer's ruling on the TODO.md rotation. The
implementer was right not to rotate, and I want that on the record as a
precedent: TODO.md's rotation rule presumes the work came from Next Step.
Work arriving out of band via an issue adds a Completed Steps entry and leaves
Next Step alone. Rotating here would have recorded the unfinished coverage
step as done and promoted a release step whose precondition is unmet. Issue
#2's definition of done said "rotate the step" — that instruction was mine and
it was wrong for this case; the implementer was correct to flag it rather than
comply silently, and correct to cite 63d1e79.

One correction to the review, non-blocking and not a defect in this PR.
The reviewer wrote that the os.Exit paths "unwind through the gameEnd
panic recovered in Run". That is not true any more — refactor step 8 removed
the gameEnd unwind; myExit now calls os.Exit(0) and Run() never
returns. The reviewer picked this up from MEMORY.md, which still asserts it.

This does not change the verdict (the race-detector evidence was empirical, not
derived from that belief), but it is a nice demonstration of why #3 exists: the
stale MEMORY.md claim misled an otherwise careful reviewer within an hour of
being filed. Bumping #3's priority accordingly.

Follow-ups recorded, not silently dropped:

The reviewer's O-1 (cold-GOCACHE make test is 16.2s, not 5.1s — inside
the 20s budget but with under 4s of headroom) is noted. Not acting on it:
the repo has no CI or Dockerfile, so a truly cold cache is a rare local
case. If a pin lands via #4 this is worth revisiting.
The reviewer's O-2 (README.md line 75 still documents running
go test ./game/ directly) is real and out of scope here. It is the same
class of problem as #3 — docs telling agents to bypass the make targets,
which now matters more because a raw go test skips -race. Folding it
into #3 rather than filing a new issue.
New hard dependency introduced: -race requires cgo and a host C toolchain
for make test. Acceptable for a dev-only Makefile in an exempt repo, but
worth knowing.

Manager notes (review results are in their own comment above). **Verdict accepted: PASS.** Labeling `merge-ready` and merging directly — `main` is unprotected in this repo, so this does not go to `sneak`. I weighted the review as credible because it verified rather than trusted the two things that are easiest to fake green: - `-race` was proven live by injecting a deliberate data race and observing `WARNING: DATA RACE`, not by observing that the suite passed. - The `exit 1` guard was probed with a **fail-then-pass** test. A probe that fails twice would never have discriminated — the target would fail either way. This is the specific defect I was most worried about and it is now genuinely ruled out. **I concur with the reviewer's ruling on the TODO.md rotation.** The implementer was right not to rotate, and I want that on the record as a precedent: `TODO.md`'s rotation rule presumes the work came *from* Next Step. Work arriving out of band via an issue adds a Completed Steps entry and leaves Next Step alone. Rotating here would have recorded the unfinished coverage step as done and promoted a release step whose precondition is unmet. Issue #2's definition of done said "rotate the step" — that instruction was mine and it was wrong for this case; the implementer was correct to flag it rather than comply silently, and correct to cite `63d1e79`. **One correction to the review, non-blocking and not a defect in this PR.** The reviewer wrote that the `os.Exit` paths "unwind through the `gameEnd` panic recovered in `Run`". That is not true any more — refactor step 8 removed the `gameEnd` unwind; `myExit` now calls `os.Exit(0)` and `Run()` never returns. The reviewer picked this up from `MEMORY.md`, which still asserts it. This does not change the verdict (the race-detector evidence was empirical, not derived from that belief), but it is a nice demonstration of why #3 exists: the stale `MEMORY.md` claim misled an otherwise careful reviewer within an hour of being filed. Bumping #3's priority accordingly. **Follow-ups recorded, not silently dropped:** - The reviewer's O-1 (cold-`GOCACHE` `make test` is 16.2s, not 5.1s — inside the 20s budget but with under 4s of headroom) is noted. Not acting on it: the repo has no CI or Dockerfile, so a truly cold cache is a rare local case. If a pin lands via #4 this is worth revisiting. - The reviewer's O-2 (`README.md` line 75 still documents running `go test ./game/` directly) is real and out of scope here. It is the same class of problem as #3 — docs telling agents to bypass the make targets, which now matters more because a raw `go test` skips `-race`. Folding it into #3 rather than filing a new issue. - New hard dependency introduced: `-race` requires cgo and a host C toolchain for `make test`. Acceptable for a dev-only Makefile in an exempt repo, but worth knowing.

clawbot added merge-ready and removed needs-review labels 2026-08-09 03:50:23 +02:00

clawbot merged commit c922a16781 into main

2026-08-09 03:50:27 +02:00

clawbot deleted branch make-test-policy-pattern

2026-08-09 03:50:27 +02:00

clawbot referenced this pull request

2026-08-09 03:51:10 +02:00

MEMORY.md and TODO.md carry stale claims that actively misdirect agents #3

clawbot referenced this pull request

2026-08-09 06:57:33 +02:00

MEMORY.md and TODO.md carry stale claims that actively misdirect agents #3

clawbot referenced this issue from a commit