[Haxxyyyy]

Closes #71.

Add a table-driven test for the POST /v1/jobs/{job}/runs Idempotency-Key rule. Valid boundary values reach the unknown-job lookup (404); invalid values are rejected first (400). Cases cover lengths 1, 128, and 129, the ! and ~ visible-ASCII endpoints, empty input, a space, a control byte, DEL, and a non-ASCII letter. The test uses the journal store and does not invoke the Runtime.

Validation: reviewed the request path and table expectations against the handler. I could not run go test, go vet, formatting checks, or the four requested mutation runs because Go is not installed here. The repository CI runs the Go test suite with pinned Runtime/Gateway binaries after its first-time contributor workflow approval. Both commits include DCO sign-off.

AI assistance: OpenAI Codex assisted with the test implementation and review.

[kikashy]

Thank you for this. Checked on the branch at 4434150:

• gofmt -l internal/ prints nothing; go vet ./internal/runner/ is clean.
• TestSubmitRunIdempotencyKeyVisibleASCIIBoundaries passes with no Runtime binary configured, as the issue intended.
• Each of the four mutations #71 names, applied one at a time to internal/runner/http.go, fails exactly the row that should catch it: > 129 fails "129 characters"; c < 32 fails "space"; c > 127 fails "DEL"; < 0 fails "empty". Every other row still passes under each mutation.
• The ten rows match the issue's edges; nothing outside #71's scope is touched; both commits carry a sign-off whose email matches the author.

One change before merge. The new function was inserted between an existing comment and the function that comment describes: lines 283–284 of events_admission_test.go ("The store-wide route for a job serves what the job's route does…") now sit above your test, and TestTheStoreWideRouteForAJobHasItsReleasesEntries has lost its comment. Move your function so that comment and its function stay together (above the comment, or after that function), push, and I will merge once CI is green.

[kikashy]

One more thing to fold into the same push: on both commits the Signed-off-by name differs from the commit author's name (the emails match). This repository has no DCO check yet, but the others in the organization require the two to match, and this one will from now on. While you move the test, run git config user.name to the signed name and rewrite the two commits with git rebase origin/main --exec 'git commit --amend --no-edit --reset-author', then push with --force-with-lease.

[kikashy]

Checking in: CI is green on all three jobs, and two small things stand between this and the merge, the test moved so the displaced comment stays with its function, and the author name matched to the sign-off name on both commits (the rebase above). Both fit in one push. If anything is unclear, ask here.

[kikashy]

Thank you for this contribution. Here is the maintainers' review record, as every change here gets one.

Review

Reviewer: OpenAI Codex, gpt-5.6-sol, reasoning effort medium, in a clean room: a fresh clone at the head 4434150, a Runtime built from CI's pin placed for it, no network, every run under a memory cap, run by the coordinator. One round by the house rule.

Prompt (verbatim)

You are reviewing one pull request in the Judgment-Pack Runner repository (Go, module root at the repository root; `internal/runner` holds the service; the event route `POST /v1/jobs/{job}/runs` and its `Idempotency-Key` rule in the HTTP handlers). Review adversarially and read-only: change no tracked file. Judge the diff; you have no knowledge of how it was written. This is a contributor's pull request for a good-first-issue; judge it kindly in tone but exactly in substance.

Under review: PR #76, head 4434150, over `origin/main` (`git diff origin/main...HEAD`), closing issue #71 ("Hold the Idempotency-Key's length and character range with a test"). What it claims: a table-driven test for the `Idempotency-Key` rule: valid boundary values reach the unknown-job lookup (404), invalid ones are rejected first (400); cases for lengths 1, 128 and 129, the `!` and `~` visible-ASCII endpoints, empty input, a space, a control byte, DEL and a non-ASCII letter; the test uses the journal store and does not invoke the Runtime; the author could not run the suite locally.

The questions that matter most: (1) Does the test exercise the real handler through the router (not a helper), and does each case assert the status the handler gives (run it; if a case fails, say which and why: is the test wrong or the handler?); (2) does the table cover the rule as the handler states it (read the handler's check: length 1 to 128, visible ASCII 0x21 to 0x7E; is a missing header a 400 or admitted?); (3) does the test start any program or need a Runtime (it must not); (4) the branch is behind `main` by many commits: does it still compile and pass on top of `main` (merge `origin/main` into a scratch copy and run it); (5) form: gofmt, vet, DCO sign-off present, no personal names or machine-local paths. Mutation: on a scratch copy, widen the handler's length limit to 129 and confirm the test fails.

Severities: HIGH (a case asserting the wrong status; a test that passes with the rule widened), MEDIUM (a boundary missing from the issue's list; a test that does not reach the handler), LOW (words).

How to work. Build and test offline only: `export GOFLAGS=-mod=mod GOPROXY=off GOCACHE=$PWD/.review-scratch/gocache JPACK_TEST_BIN=$PWD/.review-scratch/bin/jpack`; cap every go command `( ulimit -v 12000000; timeout 900 go test ... )`; focused: `go test -count=1 ./internal/runner -run 'Idempotency|Admission|Event'`; `go vet ./internal/runner`; `gofmt -l .`. No network; no installed program by name; the sandbox may refuse loopback listeners: then say so and read. Probes and mutants under `.review-scratch/`, never on the tracked tree. Write a one-line verdict last: `mergeable — head <sha>` or `not mergeable — head <sha>`.

Report findings first, the most severe first (severity; file and line; what happens; what would fix it; confirmed by running or by reading); then at most six lines on what you ran and did not. Save the full report to `.review-scratch/findings.md` and print it to stdout.

Complete output (verbatim, .review-scratch/findings.md as the reviewer saved it)

Findings

• LOW — internal/runner/events_admission_test.go:283: the pre-existing comment “The store-wide route for a job…” now immediately precedes TestSubmitRunIdempotencyKeyVisibleASCIIBoundaries, so it describes the wrong test. Move the new test above that comment (or move the comment back beside TestTheStoreWideRouteForAJobHasItsReleasesEntries). Confirmed by reading the diff; this is editorial and does not affect coverage.

• The added test passed all ten subtests on head and on a conflict-free scratch merge of origin/main; valid boundaries reached the unknown-job lookup (404), while empty/missing-equivalent, 129 bytes, space, control, DEL, and non-ASCII were rejected (400).

• serve constructs a request and calls s.Handler(...).ServeHTTP, exercising the real POST /v1/jobs/{job}/runs mux route; Header.Get makes a truly missing header equivalent to the tested empty value, hence 400.

• The journal fixture disables automation and dispatch and supplies a non-executable placeholder; this test starts no listener or program and invokes no Runtime.

• The scratch 128→129 handler mutant failed exactly 129_characters (got 404, wanted 400), proving the test detects the requested widening.

• gofmt -l ., go vet ./internal/runner, diff checks, and merged-tree vet were clean; both PR commits have DCO sign-offs, and the diff contains no personal names or machine-local paths.

• The requested broad focused test command was attempted but an unrelated httptest.NewServer panicked because sandbox loopback binding is forbidden; the new test was run separately, and no full suite was run.

mergeable — head 4434150

Maintainer disposition

#	Severity	Finding	Disposition
1	LOW	The pre-existing comment "The store-wide route for a job…" now sits above the new test instead of the test it describes.	Accept, optional before merge: if you have a moment, move the new test above that comment (or the comment back beside its test); otherwise the maintainers tidy it at merge.

Mergeable at 4434150: the test reaches the real route through the mux, all ten subtests pass on the head and on a scratch merge of main, the mutant widening the limit to 129 is caught, nothing starts a program or a Runtime, DCO is in order. The branch is behind main by a few weeks and merges clean; the maintainers rebase it at merge. Parked for the maintainer's word.

[kikashy]

Thank you for this. One thing blocks the merge: the DCO check reports both commits as incorrectly signed off. Each commit's Signed-off-by carries a name that differs from the commit author's name (the emails match), and the check requires the two to be the same, name and email. The check did not run on the branch until I brought it up to date with main today, which is why it shows only now.

Because I updated the branch, fetch it first so your local copy matches, then rewrite the two commits:

git fetch <your fork's remote>
git checkout codex/issue-71-idempotency-key-boundaries
git reset --hard <your fork's remote>/codex/issue-71-idempotency-key-boundaries
git config user.name "<the name on your Signed-off-by lines>"
git rebase HEAD~2 --exec 'git commit --amend --no-edit --reset-author'
git push --force-with-lease

That sets the author to the signed name, keeps every change as it is, and re-runs the check. If you would rather keep the author name, change the sign-off instead: after git config user.name matches the author, run git rebase HEAD~2 --exec 'git commit --amend --no-edit --signoff' and remove the old Signed-off-by line from each message. The review is done and the tests pass; once DCO passes I merge.

[kikashy]

A reminder, with a date: the sign-off fix in the note above is the only thing between this and a merge. If the branch is updated by Wednesday 2026-10-21, it merges the same day. If not, I will carry the change in a maintainer pull request that credits you as its co-author, so the test lands either way. Your judgment-pack-spec#143 has the same sign-off fix pending.

[kikashy]

Carried in #168 (merged as dbfbf93), which credits you as co-author: your test is in main byte for byte, and the maintainer commit adds the runs you could not do without Go (the test alone three times and under the race detector, and the four mutation runs #71 asks for, 4 of 4 failing the test as they should). Thank you for the cases; #71 is closed by it. Closing this pull request in favour of the carry, as the reminder said. For a next contribution, git commit -s with the same name and email as the commit's author makes the DCO check pass on its own.