Skip to content

feat(api): name the invalid field in invalid_params on body decode failures - #436

Merged
ThomasK33 merged 2 commits into
mainfrom
api-352-invalid-decode
Oct 13, 2026
Merged

ThomasK33 merged 2 commits into
mainfrom
api-352-invalid-decode

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

A 400 for a request body that fails to decode now names the invalid field in invalid_params, as in upstream's ValidationProblemDetails. This is PR 4 of the plan in #352 (issuecomment-6143529650).

{"error":"invalid JSON body: json: cannot unmarshal number into Go struct field patchRepoRequest.default_branch of type string", ..., "invalid_params":[{"name":"default_branch","reason":"must be a string"}]}

That item is upstream's own example for PATCH /repos.

Which failures name a field

Failure name reason
Wrong JSON type on any body decode (*json.UnmarshalTypeError with a field path) the dotted wire path, for example default_branch, paths.0 (an array element), or base_repo.auth.token (members decoded on their own, such as base_repo and upstream, are prefixed) must be a string, a boolean, an integer, a number, an object, an array
strictJSON (strict note writes, the strict upstream/pull form): unknown member the member path, for example author.x is not a known field
strictJSON: duplicate member the member path appears more than once
strictJSON: null member the member path must not be null
strictJSON: nested member that is not an object the member path must be a JSON object

These leave invalid_params out, as upstream does when it cannot name a field: an unreadable or empty body, a JSON syntax error, trailing data, a body that is not an object, an empty member name ({"":1}), and encoding/json's DisallowUnknownFields error (it carries no path).

What stays the same

How

  • errorEnvelope gains invalid_params (omitempty). writeProblemParams asserts that only a 400 carries it and that each item has a name and a reason.
  • writeInvalid(w, msg, cause) is the one writer for these 400s. invalidParamsFor(cause) derives the items from a *fieldError or a *json.UnmarshalTypeError.
  • The 12 "invalid JSON body: " sites in the plain-envelope decoders call writeInvalid. checkStrictObject returns *fieldError values whose Error() is the old message.

Tests

  • TestProblemMembersMatchUpstream pins the member set to the spec's ProblemDetails properties plus invalid_params, the InvalidParam properties (name, reason), and upstream's default_branch / must be a string example.
  • assertProblemFields (run on every full-server plain-error assertion, pg and sqlite lanes) now fails on any member outside that set, and on invalid_params that is not a non-empty list on a 400.
  • TestInvalidParamsFor: each decode-failure class, named or not.
  • TestInvalidParamsOnBodyFailures (full server, both lanes): PATCH, alias PATCH, create, create-branch, nested base_repo and upstream members, and the strict notes append, plus negative controls (syntax error, wrong top-level type, unknown upstream member, empty member name).

Dogfood

The same requests ran on a real local-mode server (SQLite + files) built from origin/main (5cadc73) and from this branch (270c699), then through the stable Go and TS SDKs (1.17.1). The only difference is invalid_params on the five nameable failures. SDK messages are identical, including the TS client's message for a create with a non-string default branch. The clips are real-time asciinema replays rendered with agg.

origin/main: no invalid_params

This branch: invalid_params on the nameable failures, none on the syntax error, SDK output unchanged

pr4-ab.mp4

Docs: docs/reference/http-api.md (Errors) and the RFC 9457 row of docs/explanation/wire-compatibility.md.

Refs #352


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

…ilures

Refs #352

Change-Id: I135270d4134af7b76cca69e2626a5582b7ed2754
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T15:35:17.535283Z 54fb285 Manual request
🔒 Security Review ✅ Completed 2026-10-13T15:35:22.638111Z 54fb285 Manual request

Security findings

Advisory findings (1)

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 270c699aeb

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread internal/api/handlers_write_refs.go Resolved

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 270c699aeb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/api/errors.go Resolved
Comment thread internal/api/handlers_sync.go Resolved
Comment thread internal/api/handlers_write_refs.go Resolved
…in 400

Change-Id: I4726eb96d99e542e7732929e86e448e5e52b64f6
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 54fb285026

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 54fb285026

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit 66f5c32 Oct 13, 2026
9 checks passed
@ThomasK33
ThomasK33 deleted the api-352-invalid-decode branch October 13, 2026 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant