Repository navigation
feat(api)!: send problem bodies for commit-write failures upstream documents as ProblemDetails - #669
Conversation
…cuments as ProblemDetails commit-pack, diff-commit and restore-commit keep the result envelope only on the statuses for which upstream documents CommitWriteResponse or ReposCommitsRestoreResponse (400, 409, 412 and 413 on commit-pack; 400, 409 and 412 on diff-commit; 412 on restore-commit). Every other failure is the plain problem body, and the 404s carry repository_not_found, branch_not_found or ref_not_found. A test pins the table to the vendored spec. _Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high_ Change-Id: Id4ab091377681015946d0be57b8142591c0a5cb3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Race lanes (
After the rebase, on |
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
commit-pack, diff-commit and restore-commit now send the result envelope only on the failure statuses for which upstream documents it, and the plain problem body on every other failure. This is part 2 of item 3 of #352 (scope: issuecomment-6628583093).
Why
Upstream's spec (
412b90c3, nowee8092bf) documents these failure bodies:CommitWriteResponse/ReposCommitsRestoreResponse)ProblemDetailsopen.storage sent the envelope on all of them. SDK 0.1.0 turns any body with
resultinto aRefUpdateErrorwith the message "N: request failed" and nocode.What changes
withResultEnvelopewraps each commit write route with its envelope statuses (commitWriteEnvelopeStatuses).wcFailfinds them through the writer'sUnwrapchain, asproblemInstancedoes. It writes the envelope for a listed status and the plain problem body for any other status. These handlers never send 502, so the table leaves it out.repository_not_found,branch_not_found("base branch …", "target branch …"), andref_not_found("base_ref … does not resolve to a commit", "restore target … not found").wcFailasserts that it never gets a 404.wcInternalwrites the 500 problem body itself, so merge can still share it throughwcResolveRepo. Merge's repository-not-found 404 and its 500 come from that shared lookup, so they become plain problem bodies too, as upstream documents for merge.TestCommitWriteEnvelopeStatusesMatchUpstreamparses the vendored spec and pins the table, for both route styles.wctRequireFailasks the same spec parser which shape each test failure must have.requireAuthwas the only caller ofrequireAuthWithand itsauthFailWriterparameter, so I folded them intorequireAuth. The README citations follow.streaming-commit-protocols.md§3 (a table of the envelope statuses, which also replaces the status list that part 1 left contradicting the next sentence),http-api.md, and the RFC 9457 row ofwire-compatibility.md.Validation
go build ./...,golangci-lint run(2.15.0): 0 issues. Docs gate: 0 broken links.go test ./internal/apipasses, with the new unit testsTestWcFailEnvelopeStatusesandTestCommitWriteEnvelopeStatusesMatchUpstream.Dogfood
I used the verify-openstorage harness on a node built from part 1 (
db643380, the #663 head) and on this change before its rebase onto #666 (fb60003b). Each run passed 29 checks.base_branch(404)not_foundbranch_not_foundno-push(403)forbiddenforbiddenbase_ref(400)invalidbad_requestbase_ref(404)not_foundref_not_foundmain:GET .../branch?name=mainequals the last accepted commit.commits.restorewith an unknownbase_refchanged fromRefUpdateError"404: request failed" toNotFoundErrorref_not_found, and with an unknown target branch toNotFoundErrorbranch_not_found. The no-op restore (412) stays aRefUpdateError.RestoreCommiton a missing target or branch changed fromRefUpdateErrorstatusnot_foundwith the server's message to status404with the message "restore commit failed with HTTP 404".Refs #352
BREAKING CHANGE: commit-pack, diff-commit and restore-commit answer the failures for which upstream documents ProblemDetails with the plain problem body instead of the result envelope: 403 (ref policy, quota), 404, 429 and 500 on all three, and 400 and 409 on restore-commit. Merge's repository-not-found 404 and its 500 also answer the plain problem body. The 404s carry
repository_not_found,branch_not_foundorref_not_found. SDK 1.17.1 Go reports these restore-commit failures asRefUpdateErrorwith status404(or the HTTP status) and a generic message, and builds the commit-pack and diff-commitRefUpdateErrorwith statusfailed. I did not check the 1.17.1 TypeScript and Python clients.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high