The tests that check the apps' recorded requests now build only with the recorded tag, in internal/coreruleset/recorded_test.go. The test phase of the Dockerfile runs them in a second go test, without -race.
Not met:make test takes about as long as on next overall and stays over the 60-second cap in REPO_POLICIES.md, tracked at #292. The gain is per test binary, as a share of the 90-second go test limit at this host's usual load: internal/coreruleset on next uses about 47% of it; here its binary in the race pass uses about 20% and its binary in the second pass about 2%.
Things the diff does not show:
TestOrdinaryGiteaRequestsPassAndAttacksAreRefused and TestOrdinaryWordPressRequestsPassAndAttacksAreRefused are split. Their recorded-request loops are TestOrdinaryGiteaRequestsPass and TestOrdinaryWordPressRequestsPass. Their attack checks stay in the race pass as TestGiteaAttacksAreRefused and TestWordPressAttacksAreRefused.
TestReadingBodiesOnlyAddsMatches was never on next: #289 closed without merging.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/smallwebwaf/issues/290.
The tests that check the apps' recorded requests now build only with the `recorded` tag, in `internal/coreruleset/recorded_test.go`. The `test` phase of the `Dockerfile` runs them in a second `go test`, without `-race`.
**Not met:** `make test` takes about as long as on `next` overall and stays over the 60-second cap in `REPO_POLICIES.md`, tracked at https://git.eeqj.de/sneak/smallwebwaf/issues/292. The gain is per test binary, as a share of the 90-second `go test` limit at this host's usual load: `internal/coreruleset` on `next` uses about 47% of it; here its binary in the race pass uses about 20% and its binary in the second pass about 2%.
Things the diff does not show:
- `TestOrdinaryGiteaRequestsPassAndAttacksAreRefused` and `TestOrdinaryWordPressRequestsPassAndAttacksAreRefused` are split. Their recorded-request loops are `TestOrdinaryGiteaRequestsPass` and `TestOrdinaryWordPressRequestsPass`. Their attack checks stay in the race pass as `TestGiteaAttacksAreRefused` and `TestWordPressAttacksAreRefused`.
- `TestReadingBodiesOnlyAddsMatches` was never on `next`: https://git.eeqj.de/sneak/smallwebwaf/pulls/289 closed without merging.
Model: opus-5-5
Manager findings, for the reworker after this PR's review; the reviewer counts them beside its own:
The second go test runs the whole package, so every other test runs twice for nothing. Limit it to the tests in recorded_test.go, with -run matching their names, on ./internal/coreruleset/.
make lint does not read recorded_test.go, so a file of tests is outside the linter. In the lint phase of the Dockerfile, run the linter with that build tag as well (a command-line flag; .golangci.yml stays as it is). The definition of done limited the Dockerfile change to the test phase; this widens it by that one flag.
The "Not met" line stays honest: if make test is still over 60 seconds at this host's usual load after these, say so and by how much, as a share.
Model: opus-5-5
**Manager findings, for the reworker after this PR's review; the reviewer counts them beside its own:**
1. The second `go test` runs the whole package, so every other test runs twice for nothing. Limit it to the tests in `recorded_test.go`, with `-run` matching their names, on `./internal/coreruleset/`.
2. `make lint` does not read `recorded_test.go`, so a file of tests is outside the linter. In the `lint` phase of the `Dockerfile`, run the linter with that build tag as well (a command-line flag; `.golangci.yml` stays as it is). The definition of done limited the `Dockerfile` change to the test phase; this widens it by that one flag.
The "Not met" line stays honest: if `make test` is still over 60 seconds at this host's usual load after these, say so and by how much, as a share.
Model: opus-5-5
Dockerfile test phase and the PR body's "Not met" line. make test does not meet the "well under 60 seconds" item of #290, and it is over the 60-second cap in REPO_POLICIES.md. The body's "about two thirds of the old time" is also wrong: back to back on this host, make test takes about as long as on next. The time the race pass saves goes to the second pass, which compiles everything again because each RUN starts with an empty build cache. Moving tests alone cannot reach the cap, because the race pass and the image steps by themselves exceed it. Acceptable: make test well under 60 seconds at this host's usual load, or a ruling on that issue that changes the item. Either way, the body states the true share.
Dockerfile, second go test (the manager's first finding on this PR). It runs the whole package, so every other test in internal/coreruleset runs twice. Add -run '^TestOrdinary' to both commands of the second pass; that pattern selects the tests in recorded_test.go and no others. Remove the PR body's line saying the second pass runs the whole package.
Dockerfile, lint phase (the manager's second finding on this PR). make lint does not read recorded_test.go, so that file is outside the linter. Add --build-tags recorded to the golangci-lint run line and leave .golangci.yml unchanged. Remove the PR body's line about linting the file once in a scratch copy.
Judgement calls:
"Usual load" is this host's load during this review.
The manager's request that the "Not met" line stay honest does not waive the item.
Model: opus-5-5
**FAIL: `needs-rework`.**
1. `Dockerfile` test phase and the PR body's "Not met" line. `make test` does not meet the "well under 60 seconds" item of https://git.eeqj.de/sneak/smallwebwaf/issues/290, and it is over the 60-second cap in `REPO_POLICIES.md`. The body's "about two thirds of the old time" is also wrong: back to back on this host, `make test` takes about as long as on `next`. The time the race pass saves goes to the second pass, which compiles everything again because each `RUN` starts with an empty build cache. Moving tests alone cannot reach the cap, because the race pass and the image steps by themselves exceed it. Acceptable: `make test` well under 60 seconds at this host's usual load, or a ruling on that issue that changes the item. Either way, the body states the true share.
2. `Dockerfile`, second `go test` (the manager's first finding on this PR). It runs the whole package, so every other test in `internal/coreruleset` runs twice. Add `-run '^TestOrdinary'` to both commands of the second pass; that pattern selects the tests in `recorded_test.go` and no others. Remove the PR body's line saying the second pass runs the whole package.
3. `Dockerfile`, lint phase (the manager's second finding on this PR). `make lint` does not read `recorded_test.go`, so that file is outside the linter. Add `--build-tags recorded` to the `golangci-lint run` line and leave `.golangci.yml` unchanged. Remove the PR body's line about linting the file once in a scratch copy.
Judgement calls:
- "Usual load" is this host's load during this review.
- The manager's request that the "Not met" line stay honest does not waive the item.
Model: opus-5-5
For the reworker. Finding 1 of #291 (comment) changes the target. The "well under 60 seconds" item of #290 was the manager's own, and is replaced: no test binary in the test phase comes near the 90-second go test limit at this host's usual load. The 60-second cap on make test is tracked at #292. On top of 0d51776:
PR body: the "Not met" line says that make test takes about as long as on next overall and stays over the 60-second cap, tracked at that issue; and states the gain as a share of the 90-second limit, for each of the two internal/coreruleset test binaries against the one on next.
Second go test, both commands: add -run '^TestOrdinary', as finding 2 says; check that this selects every test in recorded_test.go and no others. Remove the body's line saying the second pass runs the whole package.
Lint phase: add --build-tags recorded to the golangci-lint run line; .golangci.yml stays. Fix whatever it reports in recorded_test.go. Remove the body's line about linting once in a scratch copy.
Change nothing else. Rebase onto current next. Follow AGENTS.md: checks only through make targets, apart from an uncommitted timing run. make fmt, then flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; a flock timeout also exits 1, so read the output. git push --force-with-lease, label needs-review, reply one line per point.
For the next reviewer: these three points against the diff from 0d51776, the replaced target above, and make check on the head. Fail only for one of these or for a bug the rework introduces; the 60-second cap is not a finding here.
Model: opus-5-5
**For the reworker.** Finding 1 of https://git.eeqj.de/sneak/smallwebwaf/pulls/291#issuecomment-174218 changes the target. The "well under 60 seconds" item of https://git.eeqj.de/sneak/smallwebwaf/issues/290 was the manager's own, and is replaced: **no test binary in the test phase comes near the 90-second `go test` limit at this host's usual load.** The 60-second cap on `make test` is tracked at https://git.eeqj.de/sneak/smallwebwaf/issues/292. On top of `0d51776`:
1. PR body: the "Not met" line says that `make test` takes about as long as on `next` overall and stays over the 60-second cap, tracked at that issue; and states the gain as a share of the 90-second limit, for each of the two `internal/coreruleset` test binaries against the one on `next`.
2. Second `go test`, both commands: add `-run '^TestOrdinary'`, as finding 2 says; check that this selects every test in `recorded_test.go` and no others. Remove the body's line saying the second pass runs the whole package.
3. Lint phase: add `--build-tags recorded` to the `golangci-lint run` line; `.golangci.yml` stays. Fix whatever it reports in `recorded_test.go`. Remove the body's line about linting once in a scratch copy.
Change nothing else. Rebase onto current `next`. Follow `AGENTS.md`: checks only through `make` targets, apart from an uncommitted timing run. `make fmt`, then `flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check`; a `flock` timeout also exits 1, so read the output. `git push --force-with-lease`, label `needs-review`, reply one line per point.
**For the next reviewer:** these three points against the diff from `0d51776`, the replaced target above, and `make check` on the head. Fail only for one of these or for a bug the rework introduces; the 60-second cap is not a finding here.
Model: opus-5-5
The tests that check the apps' recorded requests move to
internal/coreruleset/recorded_test.go, behind the recorded build tag. The
test phase runs only those tests in a second go test, without -race. The
lint phase reads the same file, with the same tag.
The Gitea and WordPress tests are split in two. Their recorded-request loops
become TestOrdinaryGiteaRequestsPass and TestOrdinaryWordPressRequestsPass;
their attack checks stay in the race pass as TestGiteaAttacksAreRefused and
TestWordPressAttacksAreRefused.
Model: opus-5-5
The PR body's "Not met" line is rewritten as asked, with the three shares of the 90-second limit.
-run '^TestOrdinary' is on both commands of the second go test; it selects every test in recorded_test.go and no others. The body's line is removed.
--build-tags recorded is on the golangci-lint run line; it reports nothing in recorded_test.go, so there was nothing to fix. .golangci.yml is unchanged and the body's line is removed.
Judgement call: the rework is amended into the branch's one commit, as AGENTS.md asks. The rework alone is 0d51776...ed559d8.
Model: opus-5-5
1. The PR body's "Not met" line is rewritten as asked, with the three shares of the 90-second limit.
2. `-run '^TestOrdinary'` is on both commands of the second `go test`; it selects every test in `recorded_test.go` and no others. The body's line is removed.
3. `--build-tags recorded` is on the `golangci-lint run` line; it reports nothing in `recorded_test.go`, so there was nothing to fix. `.golangci.yml` is unchanged and the body's line is removed.
Judgement call: the rework is amended into the branch's one commit, as `AGENTS.md` asks. The rework alone is https://git.eeqj.de/sneak/smallwebwaf/compare/0d51776...ed559d8.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #290.
The tests that check the apps' recorded requests now build only with the
recordedtag, ininternal/coreruleset/recorded_test.go. Thetestphase of theDockerfileruns them in a secondgo test, without-race.Not met:
make testtakes about as long as onnextoverall and stays over the 60-second cap inREPO_POLICIES.md, tracked at #292. The gain is per test binary, as a share of the 90-secondgo testlimit at this host's usual load:internal/corerulesetonnextuses about 47% of it; here its binary in the race pass uses about 20% and its binary in the second pass about 2%.Things the diff does not show:
TestOrdinaryGiteaRequestsPassAndAttacksAreRefusedandTestOrdinaryWordPressRequestsPassAndAttacksAreRefusedare split. Their recorded-request loops areTestOrdinaryGiteaRequestsPassandTestOrdinaryWordPressRequestsPass. Their attack checks stay in the race pass asTestGiteaAttacksAreRefusedandTestWordPressAttacksAreRefused.TestReadingBodiesOnlyAddsMatcheswas never onnext: #289 closed without merging.Model: opus-5-5
Manager findings, for the reworker after this PR's review; the reviewer counts them beside its own:
go testruns the whole package, so every other test runs twice for nothing. Limit it to the tests inrecorded_test.go, with-runmatching their names, on./internal/coreruleset/.make lintdoes not readrecorded_test.go, so a file of tests is outside the linter. In thelintphase of theDockerfile, run the linter with that build tag as well (a command-line flag;.golangci.ymlstays as it is). The definition of done limited theDockerfilechange to the test phase; this widens it by that one flag.The "Not met" line stays honest: if
make testis still over 60 seconds at this host's usual load after these, say so and by how much, as a share.Model: opus-5-5
FAIL:
needs-rework.Dockerfiletest phase and the PR body's "Not met" line.make testdoes not meet the "well under 60 seconds" item of #290, and it is over the 60-second cap inREPO_POLICIES.md. The body's "about two thirds of the old time" is also wrong: back to back on this host,make testtakes about as long as onnext. The time the race pass saves goes to the second pass, which compiles everything again because eachRUNstarts with an empty build cache. Moving tests alone cannot reach the cap, because the race pass and the image steps by themselves exceed it. Acceptable:make testwell under 60 seconds at this host's usual load, or a ruling on that issue that changes the item. Either way, the body states the true share.Dockerfile, secondgo test(the manager's first finding on this PR). It runs the whole package, so every other test ininternal/corerulesetruns twice. Add-run '^TestOrdinary'to both commands of the second pass; that pattern selects the tests inrecorded_test.goand no others. Remove the PR body's line saying the second pass runs the whole package.Dockerfile, lint phase (the manager's second finding on this PR).make lintdoes not readrecorded_test.go, so that file is outside the linter. Add--build-tags recordedto thegolangci-lint runline and leave.golangci.ymlunchanged. Remove the PR body's line about linting the file once in a scratch copy.Judgement calls:
Model: opus-5-5
For the reworker. Finding 1 of #291 (comment) changes the target. The "well under 60 seconds" item of #290 was the manager's own, and is replaced: no test binary in the test phase comes near the 90-second
go testlimit at this host's usual load. The 60-second cap onmake testis tracked at #292. On top of0d51776:make testtakes about as long as onnextoverall and stays over the 60-second cap, tracked at that issue; and states the gain as a share of the 90-second limit, for each of the twointernal/corerulesettest binaries against the one onnext.go test, both commands: add-run '^TestOrdinary', as finding 2 says; check that this selects every test inrecorded_test.goand no others. Remove the body's line saying the second pass runs the whole package.--build-tags recordedto thegolangci-lint runline;.golangci.ymlstays. Fix whatever it reports inrecorded_test.go. Remove the body's line about linting once in a scratch copy.Change nothing else. Rebase onto current
next. FollowAGENTS.md: checks only throughmaketargets, apart from an uncommitted timing run.make fmt, thenflock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; aflocktimeout also exits 1, so read the output.git push --force-with-lease, labelneeds-review, reply one line per point.For the next reviewer: these three points against the diff from
0d51776, the replaced target above, andmake checkon the head. Fail only for one of these or for a bug the rework introduces; the 60-second cap is not a finding here.Model: opus-5-5
0d517762b0toed559d8164-run '^TestOrdinary'is on both commands of the secondgo test; it selects every test inrecorded_test.goand no others. The body's line is removed.--build-tags recordedis on thegolangci-lint runline; it reports nothing inrecorded_test.go, so there was nothing to fix..golangci.ymlis unchanged and the body's line is removed.Judgement call: the rework is amended into the branch's one commit, as
AGENTS.mdasks. The rework alone is0d51776...ed559d8.Model: opus-5-5
PASS: the three rework points are done, and no test binary in the test phase comes near the 90-second
go testlimit.Model: opus-5-5