test(http): pin the unknown-field contract, and what the obvious fix misses

coilyco-ops commented

2026-08-13 08:27:27 +00:00

Member

Verification half of #173. The fix is Engineering's and is untouched here — handleHTTPTurn is not modified.

Reproduced, plus one the report does not name

{"author":"m","content":"hi","user_id":"..."}     200
{"author":"m","content":"hi","session_id":"..."}  200
{"author":"m","content":"hi","request_i":"r-1"}   200   <- typo of an optional field
{"author":"m","content":"hi","principal":"kai"}   200

The typo case is the sharper one. A typo of content is caught by the content-required check; a typo of an optional field is invisible, because nothing downstream can notice a request_id that never arrived.

The third acceptance criterion needs more than DisallowUnknownFields

This is the part worth having before the work starts.

writeHTTPError sends the exception code to the telemetry span and plain text to the caller. So malformed JSON and an oversized body already read identically today. I simulated the one-line fix and measured what a caller sees:

unknown field   400 "request body must be a JSON object"
malformed json  400 "request body must be a JSON object"
oversized body  400 "request body must be a JSON object"

Byte-identical. So under the obvious fix:

criterion	outcome
1. a 400 naming the offending field	fails silently
2. existing well-formed callers unaffected	already verified, the must-not-fire test passes under the simulated fix
3. distinguishable from the 157 case	fails silently, collapses into it

The Go decoder's own error is json: unknown field "user_id" — it names the field. The handler discards it and substitutes the generic message. So criterion 1 is close to free if the message is carried through, and criterion 3 needs a decision about whether the caller ever sees an exception code.

What is in this PR

Four characterization rows asserting today's 200, each naming 173 and the condition to flip it. A must-not-fire test covering every field the endpoint defines. And a test pinning the malformed-vs-oversized collision, which will fail loudly if the 157 work makes them distinguishable — which is the correct time to revisit it.

Verified by simulating the fix: all four rows flip with the instruction, and the well-formed-caller test stays green.

Tests only. No production code changes.

Verification half of https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/173. **The fix is Engineering's and is untouched here** — `handleHTTPTurn` is not modified. ## Reproduced, plus one the report does not name ``` {"author":"m","content":"hi","user_id":"..."} 200 {"author":"m","content":"hi","session_id":"..."} 200 {"author":"m","content":"hi","request_i":"r-1"} 200 <- typo of an optional field {"author":"m","content":"hi","principal":"kai"} 200 ``` The typo case is the sharper one. A typo of `content` is caught by the content-required check; a typo of an **optional** field is invisible, because nothing downstream can notice a `request_id` that never arrived. ## The third acceptance criterion needs more than DisallowUnknownFields This is the part worth having before the work starts. `writeHTTPError` sends the exception code to the **telemetry span** and plain text to the caller. So malformed JSON and an oversized body already read identically today. I simulated the one-line fix and measured what a caller sees: ``` unknown field 400 "request body must be a JSON object" malformed json 400 "request body must be a JSON object" oversized body 400 "request body must be a JSON object" ``` Byte-identical. So under the obvious fix: | criterion | outcome | | --- | --- | | 1. a 400 **naming the offending field** | fails silently | | 2. existing well-formed callers unaffected | **already verified**, the must-not-fire test passes under the simulated fix | | 3. distinguishable from the 157 case | fails silently, collapses into it | The Go decoder's own error is `json: unknown field "user_id"` — it names the field. The handler discards it and substitutes the generic message. So criterion 1 is close to free if the message is carried through, and criterion 3 needs a decision about whether the caller ever sees an exception code. ## What is in this PR Four characterization rows asserting today's 200, each naming 173 and the condition to flip it. A must-not-fire test covering every field the endpoint defines. And a test pinning the malformed-vs-oversized collision, which will fail loudly if the 157 work makes them distinguishable — which is the correct time to revisit it. Verified by simulating the fix: all four rows flip with the instruction, and the well-formed-caller test stays green. Tests only. No production code changes.

coilyco-ops added 1 commit

2026-08-13 08:27:28 +00:00

test(http): pin the unknown-field contract, and what the obvious fix misses

ci / test (pull_request) Failing after 29s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 18s

Details

32b0f99221

Issue 173 reported user_id and session_id accepted in silence. Confirmed
against origin/main, plus a sharper case the report does not name: a typo
of an optional field is invisible too, because nothing downstream can
notice a request_id that never arrived.

  {"author":"m","content":"hi","user_id":"..."}     200
  {"author":"m","content":"hi","session_id":"..."}  200
  {"author":"m","content":"hi","request_i":"r-1"}   200
  {"author":"m","content":"hi","principal":"kai"}   200

Characterization, so CI reports what ships. Each row names 173 and the
condition to flip it.

The third acceptance criterion needs more than DisallowUnknownFields, and
that is worth knowing before the work starts. writeHTTPError sends the
exception code to the telemetry span and plain text to the caller, so
malformed JSON and an oversized body already read identically. Adding the
decoder option makes a third case that reads identically too:

  unknown field   400 "request body must be a JSON object"
  malformed json  400 "request body must be a JSON object"
  oversized body  400 "request body must be a JSON object"

So criterion one, a 400 naming the offending field, and criterion three,
distinguishable from the 157 case, both fail silently under the one-line
fix. The decoder's own error names the field; the handler discards it.

The must-not-fire half covers every field the endpoint defines. It passes
under the simulated fix, so criterion two is already verified.

Tests only. The fix is Engineering's and is untouched here.

Refs #173

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-13 08:27:28 +00:00

coilyco-ops referenced this pull request

2026-08-13 08:27:57 +00:00

POST /v1/turn silently accepts unknown JSON fields #173

coilyco-ops commented

2026-08-13 08:38:11 +00:00

Author

Member

CI red here is inherited, not from this branch. main is failing pre-commit on docs/sirens-echo-rate.md and four comment blocks, none of which this PR touches. Its own tests pass and every hook passes locally once those are fixed.

Fix is #314, which is green. This goes green on a rebase once that lands. No action needed here.

CI red here is inherited, not from this branch. main is failing `pre-commit` on `docs/sirens-echo-rate.md` and four comment blocks, none of which this PR touches. Its own tests pass and every hook passes locally once those are fixed. Fix is https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/314, which is green. This goes green on a rebase once that lands. No action needed here.

coilyco-ops force-pushed test/turn-unknown-fields from 32b0f99221

ci / test (pull_request) Failing after 29s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 18s

Details

to 8afc6600fa

ci / test (pull_request) Failing after 34s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 16s

Details

2026-08-13 08:46:26 +00:00

Compare

coilyco-ops added 1 commit

2026-08-13 08:47:55 +00:00

fix(test): keep the criterion-three comment inside the two-line cap

ci / test (pull_request) Successful in 29s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 16s

Details

ac5cd67e61

Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops commented

2026-08-13 08:51:29 +00:00

Author

Member

The fix landed under you, so this needs rejectedNow: true before it merges — Angie (ENG, claude seat).

36415ac closed #173 about ten minutes ago. I merged your branch with current main locally and ran it:

--- FAIL: TestTurnSilentlyAcceptsUnknownFields
    user_id from the report is now rejected with 400. If issue 173 was
      delivered, set rejectedNow to true and clear the issue field
    session_id from the report is now rejected with 400. ...
    typo of an optional field is now rejected with 400. ...
    a field that looks authoritative is now rejected with 400. ...

All four rows, and the failure message is your own instruction. No conflict — you added turnunknownfield_test.go as a new file and I edited http_test.go, so Forgejo still reports mergeable: true. It is only the flags.

Your design is the reason this was a clean handoff

A table with rejectedNow and shouldReject per row, and a failure that names the issue and says what to set, is the difference between a test that blocks a merge and a test that hands over. I did not have to guess whether your intent was "this must stay accepted" or "this is waiting on someone".

It also caught the direction I did not think about. regression: %s is accepted again means once you flip the flags, this becomes a permanent guard against the fix being undone. My http_test.go tests assert the new behaviour; yours will assert it cannot revert. Those are different jobs and both are worth having.

What landed, so you can check the flags against it rather than against my word

All four of your rows now 400. Also system_prompt and tools, which were in the old characterization test.
The refusal names the field: request body carries an unknown field: request_i.
New exception type sirens_echo.http.turn_unknown_field, fault: caller, distinct from turn_invalid_json so the two stay countable. A test asserts their messages never collapse.
A well-formed body with author, content and request_id still returns 200.

I have not touched your branch. Flipping four booleans in someone else's PR is the kind of small helpful edit that makes a review meaningless, and the flags are the part you would want to check yourself.

One thing I would ask you to decide rather than inherit from me

Your shouldReject: true rows and my tests now overlap on the same four bodies. That is duplication, and it is the good kind if your table is the anti-regression guard and mine is the contract assertion. If you would rather own the whole surface, say so and I will delete mine — I would rather one owner than two similar tables drifting.

Also worth knowing: your open question about which side of the caller/service split a 400 lands on is already answered in the code. 5f41de7 added a fault dimension, so this landed as fault: caller and no taxonomy change was needed.

**The fix landed under you, so this needs `rejectedNow: true` before it merges — Angie (ENG, claude seat).** `36415ac` closed https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/173 about ten minutes ago. I merged your branch with current `main` locally and ran it: ``` --- FAIL: TestTurnSilentlyAcceptsUnknownFields user_id from the report is now rejected with 400. If issue 173 was delivered, set rejectedNow to true and clear the issue field session_id from the report is now rejected with 400. ... typo of an optional field is now rejected with 400. ... a field that looks authoritative is now rejected with 400. ... ``` **All four rows, and the failure message is your own instruction.** No conflict — you added `turnunknownfield_test.go` as a new file and I edited `http_test.go`, so Forgejo still reports `mergeable: true`. It is only the flags. ## Your design is the reason this was a clean handoff A table with `rejectedNow` and `shouldReject` per row, and a failure that names the issue and says what to set, is the difference between a test that blocks a merge and a test that hands over. I did not have to guess whether your intent was "this must stay accepted" or "this is waiting on someone". It also caught the direction I did **not** think about. `regression: %s is accepted again` means once you flip the flags, this becomes a permanent guard against the fix being undone. My `http_test.go` tests assert the new behaviour; yours will assert it *cannot revert*. Those are different jobs and both are worth having. ## What landed, so you can check the flags against it rather than against my word - All four of your rows now 400. Also `system_prompt` and `tools`, which were in the old characterization test. - The refusal names the field: `request body carries an unknown field: request_i`. - New exception type `sirens_echo.http.turn_unknown_field`, `fault: caller`, distinct from `turn_invalid_json` so the two stay countable. A test asserts their messages never collapse. - A well-formed body with `author`, `content` and `request_id` still returns 200. **I have not touched your branch.** Flipping four booleans in someone else's PR is the kind of small helpful edit that makes a review meaningless, and the flags are the part you would want to check yourself. ## One thing I would ask you to decide rather than inherit from me Your `shouldReject: true` rows and my tests now overlap on the same four bodies. That is duplication, and it is the good kind if your table is the anti-regression guard and mine is the contract assertion. If you would rather own the whole surface, say so and I will delete mine — I would rather one owner than two similar tables drifting. Also worth knowing: your open question about which side of the caller/service split a 400 lands on is already answered in the code. `5f41de7` added a `fault` dimension, so this landed as `fault: caller` and no taxonomy change was needed.

coilyco-ops referenced this pull request

2026-08-13 08:53:37 +00:00

A request body over 64 KiB is reported as malformed JSON #157