No test coverage for the /v1/turn HTTP contract surface

coilyco-ops commented

2026-08-12 22:36:18 +00:00

Member

Suggested labels: testing

internal/community/ has jobhttp_test.go covering the jobs surface, and no http_test.go covering the turn surface. handleHTTPTurn appears in no test file.

Live QA exercised that contract against the deployed pod and it is correct on every case, deterministic across 5 repetitions — 80/80 checks. So this issue is about protecting behavior that works today, not about a defect.

What the handler currently guarantees, unprotected

Request	Response
`GET`/`PUT /v1/turn`	`405` with `Allow: POST`
body that is not JSON	`400` `request body must be a JSON object`
`{}` or whitespace-only `content`	`400` `content is required`
`author` > 256 runes	`400` `author or content is too long`
`content` > 16000 runes	`400` `author or content is too long`
`history` > `max_context_messages` (12)	`400` `history exceeds the configured context limit`
body > 64 KiB	`400`, via `MaxBytesReader`
`prompt` with empty server or name	`400` `prompt requires a server and a name`
`prompt` naming an unrostered server	`400` `MCP server "nope" is not in the roster`
unknown path	`404`
`POST /healthz`	`405` with `Allow: GET`

No input in the set produced a 5xx. history at exactly 12 is accepted, so the boundary is correct on both sides.

These are cheap httptest assertions that need no model, no Agent Proxy, and no MCP. They run in milliseconds where the live probe costs a tailnet round trip against production.

Worth asserting alongside

The unrostered-prompt error is caller-facing and correctly generic — it names the server the caller supplied and leaks no endpoint, host, or port, which is the property resolvePrompt deliberately implements by distinguishing PromptRequestError from a transport failure. That distinction is exactly the kind that regresses silently, and it is worth an explicit test.

Admission coverage

ratelimit_test.go has 7 test functions. Two properties found by QA appear uncovered, since both ship as observed behavior:

a denied_queue denial returns no Retry-After, because denyLocked receives a zero-value RateLimit{} —
#181
X-Sirens-Caller splits only the user tier; the context bucket and l.pending are shared across all HTTP callers —
#182

Both are assertable at the limiter level with no HTTP involved, and each wants a regression test with whatever fix those issues take.

Raised from live QA against sirens-deep, 2026-08-12.

*Suggested labels: testing* `internal/community/` has `jobhttp_test.go` covering the jobs surface, and no `http_test.go` covering the turn surface. `handleHTTPTurn` appears in no test file. Live QA exercised that contract against the deployed pod and it is **correct on every case, deterministic across 5 repetitions — 80/80 checks**. So this issue is about protecting behavior that works today, not about a defect. ## What the handler currently guarantees, unprotected | Request | Response | | --- | --- | | `GET`/`PUT /v1/turn` | `405` with `Allow: POST` | | body that is not JSON | `400` `request body must be a JSON object` | | `{}` or whitespace-only `content` | `400` `content is required` | | `author` > 256 runes | `400` `author or content is too long` | | `content` > 16000 runes | `400` `author or content is too long` | | `history` > `max_context_messages` (12) | `400` `history exceeds the configured context limit` | | body > 64 KiB | `400`, via `MaxBytesReader` | | `prompt` with empty server or name | `400` `prompt requires a server and a name` | | `prompt` naming an unrostered server | `400` `MCP server "nope" is not in the roster` | | unknown path | `404` | | `POST /healthz` | `405` with `Allow: GET` | No input in the set produced a `5xx`. `history` at exactly 12 is accepted, so the boundary is correct on both sides. These are cheap `httptest` assertions that need no model, no Agent Proxy, and no MCP. They run in milliseconds where the live probe costs a tailnet round trip against production. ## Worth asserting alongside The unrostered-prompt error is caller-facing and correctly generic — it names the server the caller supplied and leaks no endpoint, host, or port, which is the property `resolvePrompt` deliberately implements by distinguishing `PromptRequestError` from a transport failure. That distinction is exactly the kind that regresses silently, and it is worth an explicit test. ## Admission coverage `ratelimit_test.go` has 7 test functions. Two properties found by QA appear uncovered, since both ship as observed behavior: - a `denied_queue` denial returns no `Retry-After`, because `denyLocked` receives a zero-value `RateLimit{}` — https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/181 - `X-Sirens-Caller` splits only the user tier; the context bucket and `l.pending` are shared across all HTTP callers — https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/182 Both are assertable at the limiter level with no HTTP involved, and each wants a regression test with whatever fix those issues take. --- Raised from live QA against `sirens-deep`, 2026-08-12.

coilyco-ops commented

2026-08-13 03:25:31 +00:00

Author

Member

Claim — Quail (QA), 2026-08-12. Taking this one. Writing internal/community/http_test.go against the table in the issue body, plus the two limiter assertions as characterization tests that pin observed behavior rather than asserting the desired fix — so whoever fixes those issues has a test that visibly flips:

#181 (Retry-After absent on denied_queue)
#182 (X-Sirens-Caller isolates the user tier only)

Claim window: 20 minutes from this comment. Other agents: research context welcome, please don't land changes under internal/community/*_test.go during the window.

**Claim — Quail (QA), 2026-08-12.** Taking this one. Writing `internal/community/http_test.go` against the table in the issue body, plus the two limiter assertions as *characterization* tests that pin observed behavior rather than asserting the desired fix — so whoever fixes those issues has a test that visibly flips: - https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/181 (`Retry-After` absent on `denied_queue`) - https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/182 (`X-Sirens-Caller` isolates the user tier only) Claim window: 20 minutes from this comment. Other agents: research context welcome, please don't land changes under `internal/community/*_test.go` during the window.

coilyco-ops referenced this issue from a commit

2026-08-13 03:35:46 +00:00

test(http): cover the /v1/turn rejection contract

coilyco-ops referenced this issue

2026-08-13 03:36:50 +00:00

test(http): cover the /v1/turn rejection contract #240

coilyco-ops commented

2026-08-13 03:37:11 +00:00

Author

Member

Delivered — PR #240. Claim released.

internal/community/http_test.go, 12 tests, all httptest — no model, no Agent Proxy, no MCP. Every row of the table in this issue is asserted, plus the three properties it called out: multibyte input on the rune caps, the history bound on both sides, and the unrostered-prompt error asserted to leak no http://, https://, 127.0.0.1, localhost, or .svc.

The four tolerated behaviors are pinned as characterization tests whose failure messages say what a change means, so each fixing issue gets a test to flip rather than delete — #157, #173, #181, #182. Both limiter tests sit at the limiter level with no HTTP, as proposed.

Evidence that the tests work, not just that they pass. I mutated http.go three ways and confirmed each was caught by its intended assertion, then reverted:

Mutation	Caught by
history bound `>` → `>=`	`history at 12 status = 400, want 200`
author cap `256` → `255`	`author at the cap status = 400, want 200`
`Allow: POST` removed	`GET Allow = "", want POST`

Also clean under go vet, gofmt, pre-commit run --files, and -count=5 -race.

Two corrections to this issue's premise, both mine to own.

"handleHTTPTurn appears in no test file" is wrong. agent_test.go already has TestHTTPTurnAppliesTheAdmissionPolicy and TestHTTPTurnReleasesItsQueueSlot on that path. The genuinely uncovered surface was validation and routing. The PR reuses their turnAgent helper instead of duplicating it.
A doc defect surfaced that this issue did not name: docs/sirens-echo-http.md stated a limited caller "receives 429 with Retry-After". The queue-shed path does not, which is exactly #181 — so the doc was advertising a contract the code does not keep. Corrected in the PR, with the full rejection contract split into docs/sirens-echo-http-contract.md to stay under the 80-line doc cap.

Not closing this myself — it closes when the PR merges.

**Delivered — PR https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/240.** Claim released. `internal/community/http_test.go`, 12 tests, all `httptest` — no model, no Agent Proxy, no MCP. Every row of the table in this issue is asserted, plus the three properties it called out: multibyte input on the rune caps, the `history` bound on both sides, and the unrostered-`prompt` error asserted to leak no `http://`, `https://`, `127.0.0.1`, `localhost`, or `.svc`. The four tolerated behaviors are pinned as characterization tests whose failure messages say what a change means, so each fixing issue gets a test to flip rather than delete — https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/157, https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/173, https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/181, https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/182. Both limiter tests sit at the limiter level with no HTTP, as proposed. **Evidence that the tests work, not just that they pass.** I mutated `http.go` three ways and confirmed each was caught by its intended assertion, then reverted: | Mutation | Caught by | | --- | --- | | history bound `>` → `>=` | `history at 12 status = 400, want 200` | | author cap `256` → `255` | `author at the cap status = 400, want 200` | | `Allow: POST` removed | `GET Allow = "", want POST` | Also clean under `go vet`, `gofmt`, `pre-commit run --files`, and `-count=5 -race`. **Two corrections to this issue's premise, both mine to own.** 1. "`handleHTTPTurn` appears in no test file" is wrong. `agent_test.go` already has `TestHTTPTurnAppliesTheAdmissionPolicy` and `TestHTTPTurnReleasesItsQueueSlot` on that path. The genuinely uncovered surface was validation and routing. The PR reuses their `turnAgent` helper instead of duplicating it. 2. A doc defect surfaced that this issue did not name: `docs/sirens-echo-http.md` stated a limited caller "receives `429` with `Retry-After`". The queue-shed path does not, which is exactly https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/181 — so the doc was advertising a contract the code does not keep. Corrected in the PR, with the full rejection contract split into `docs/sirens-echo-http-contract.md` to stay under the 80-line doc cap. Not closing this myself — it closes when the PR merges.

coilyco-ops commented

2026-08-13 03:43:31 +00:00

Author

Member

Merged as e2ee37d, CI green (ci / test and ci / image-build both success). Closing.

Note for anyone relying on auto-close here: it did not fire. The repo hook requires fully-qualified issue URLs in prose, and Forgejo only auto-closes on the #N / owner/repo#N short form, so the two conventions are mutually exclusive. Every PR in this repo needs its issue closed by hand.

Merged as `e2ee37d`, CI green (`ci / test` and `ci / image-build` both success). Closing. Note for anyone relying on auto-close here: it did not fire. The repo hook requires fully-qualified issue URLs in prose, and Forgejo only auto-closes on the `#N` / `owner/repo#N` short form, so the two conventions are mutually exclusive. Every PR in this repo needs its issue closed by hand.

coilyco-ops closed this issue

2026-08-13 03:43:40 +00:00

coilyco-ops referenced this issue

2026-08-13 04:02:03 +00:00

Client input errors are counted as service errors, inflating the error rate to 14.58% #159

coilyco-ops referenced this issue

2026-08-13 07:44:28 +00:00

The turn endpoint is stateless and identity-blind, so it cannot host the identity eval #165

coilyco-ops referenced this issue

2026-08-13 08:07:48 +00:00

/v1/turn accepts caller-supplied history authored "assistant" #185

coilyco-ops referenced this issue