Repository navigation
feat(api)!: send problem bodies and MergeConflict for merge and note-write failures - #670
Conversation
…write failures Upstream documents a plain ProblemDetails body for every merge and note write failure, except merge's 409 MergeConflict for file conflicts and a target that moved (application/json: error, conflict_type, conflict_paths, merge_base_sha). Merge now sends exactly that: file conflicts as merge_conflict, a lost CAS as target_moved, and the plain problem body (code conflict) for other conflicts. Note writes send the plain problem body, including a lost notes CAS (409 conflict). The body forms' stale-guard 412 keeps the note envelope. _Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high_ Change-Id: I55afa498cac0f62f128e8217e8b42ec64c3e34f7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Validation evidence for this head (
Each rebase added exactly the diff of the PR it rebased onto (I compared the changed lines). The first |
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0becf9b2c0
ℹ️ 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".
| } | ||
| if err := wmValidateMergeRequest(&req); err != nil { | ||
| wmFail(w, http.StatusBadRequest, "invalid", err.Error(), req.TargetBranch, "") | ||
| writeInvalid(w, err.Error(), err) |
There was a problem hiding this comment.
Preserve field metadata for merge validation errors
When a merge body fails a field-specific check such as a missing strategy or malformed expected_target_sha, wmValidateMergeRequest returns an ordinary error, so writeInvalid cannot derive any invalid_params. These newly introduced problem responses therefore omit the field information required for nameable body-validation failures; return fieldError values or explicitly supply the affected field.
Useful? React with 👍 / 👎.
| } | ||
| if objectSHA == "" { | ||
| wrefNoteFail(w, http.StatusBadRequest, "object_ref is required", objectSHA, targetRef, "", ref.ZeroSHA) | ||
| writeErr(w, http.StatusBadRequest, "object_ref is required") |
There was a problem hiding this comment.
Add invalid_params to note body validation failures
For a POST /repos/notes body with no object_ref, this now emits a problem body through writeErr, but it lacks invalid_params even though the failing required field is known. The same omission affects the adjacent checks for action, note, expected_notes_ref_sha, and author; body-form failures should use the field-aware validation writer, while query-form DELETE validation can remain unannotated.
Useful? React with 👍 / 👎.
| if _, err := wrefGit(ctx, repoPath, nil, nil, | ||
| "rev-parse", "--verify", "--quiet", "--end-of-options", objectSHA+"^{object}"); err != nil { | ||
| wrefNoteFail(w, http.StatusNotFound, "object not found: "+objectSHA, objectSHA, targetRef, old, oldOrZero) | ||
| writeErr(w, http.StatusNotFound, "object not found: "+objectSHA) |
There was a problem hiding this comment.
Emit ref_not_found for an absent full object SHA
When object_ref is a syntactically valid 40-hex SHA that is absent from the repository, execution reaches this branch and writeErr assigns the generic not_found code. Non-SHA object references already receive ref_not_found, so clients see different machine-readable codes for the same unresolved object_ref; use the specific ref_not_found problem here as well.
Useful? React with 👍 / 👎.
🛡️ 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. |
Codex review on #670: merge and note-write failures are problem bodies now, so their single-field body checks name the field in invalid_params, as the other ValidationProblemDetails routes do. DELETE query parameters stay unnamed. A note on an absent full object id answers 404 ref_not_found, as an unresolvable revision does. Change-Id: Iaed09b92ae880f9864661b550e7c4a44353f0014 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Codex round 1 had three P2 findings (medium severity). I fixed all three in 7b17d77, a new commit (no force-push):
Tests: new rows in |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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. |
The #670 advisor found that upstream's merge 412 names both refusals ("a merge with no common ancestor" and a target that is not a fast-forward under ff_only) with precondition_failed. open.storage answered 409 conflict. The body stays the plain problem body. Change-Id: I02a046aa1b8d76c07e3976eedff589feffbde454 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
The independent advisor on 7b17d77 found that two merge refusals used the wrong status. Upstream's merge 412 says storage returns 6687043 (new commit, no force-push) answers both with 412 Two smaller advisor notes are now in the PR body: |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🎉 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. |
…problems Change-Id: I15ea29c14f9a3ad55b629b72ab62853bc467a640 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
#668 landed while this PR waited, so I merged
Validation on 4b98ea4: |
|
@codex review |
|
@codex security review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b98ea48f2
ℹ️ 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".
| if req.Author == nil { | ||
| wmFail(w, http.StatusBadRequest, "invalid", "author is required when the merge creates a commit", req.TargetBranch, targetSHA) | ||
| writeErr(w, http.StatusBadRequest, "author is required when the merge creates a commit") | ||
| return |
There was a problem hiding this comment.
Include author in conditional merge validation errors
When a non-fast-forward merge or squash needs to create a commit and the request omits author, this branch now emits a ValidationProblemDetails body without invalid_params, although the missing request-body field is known and the API conventions require nameable required fields to be identified. Use writeInvalidField for author so clients receive the same field metadata as for the other merge validation failures.
Useful? React with 👍 / 👎.
| if !won { | ||
| wmFail(w, http.StatusConflict, "conflict", "ref "+targetRef+" moved concurrently", req.TargetBranch, targetSHA) | ||
| wmFailConflict(w, "target_moved", "ref "+targetRef+" moved concurrently", nil, "") | ||
| return |
There was a problem hiding this comment.
Reserve target_moved for failed stale-target retries
When another writer wins the first target CAS, this path immediately returns an application/json MergeConflict with conflict_type: target_moved, but the upstream operation contract in docs/upstream/openapi.yaml:7721 reserves that shape for an automatic stale-target retry that could not absorb the movement and says other conflicts use a problem body. This handler performs no retry, so a routine lost CAS is misclassified and SDKs surface a MergeConflictError; either retry before using target_moved or return the ordinary 409 problem body here and in the merge-commit lost-CAS path.
Useful? React with 👍 / 👎.
Codex round 4 on #670: upstream sends MergeConflict with target_moved only after an automatic stale-target retry fails, and a problem body for any other conflict. open.storage does not retry, so a lost target CAS is now a 409 conflict problem body. The conditional author 400 names author in invalid_params. Change-Id: I049c985ca5c85f1ecde64d86f6954a4b185f5afd Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Codex round 4 had two P2 findings (medium severity). I fixed both in 2583a82, a new commit (no force-push):
|
|
@codex review |
|
@codex security review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2583a82683
ℹ️ 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".
| `commit-pack`, `diff-commit` and `restore-commit` return the richer `result` envelope | ||
| on the statuses that the | ||
| [streaming protocols](../explanation/streaming-commit-protocols.md) list, a merge | ||
| `409` for file conflicts or a target that moved returns upstream's `MergeConflict` |
There was a problem hiding this comment.
Describe moved-target merges as problem bodies
When a merge loses either target-ref CAS, the handler now calls writeErr and returns a problem body without conflict_type; only file conflicts return MergeConflict. This convention instead tells API consumers that a moved target returns MergeConflict, contradicting both the implementation and the detailed merge table and potentially causing clients to parse the wrong response shape.
Useful? React with 👍 / 👎.
…tions too Codex round 5 on #670: the http-api conventions paragraph still said a moved target returns MergeConflict. Change-Id: Ibfc8b3d7af8e5d8021edc16cd2bcb896a821f959 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Codex round 5 found one stale sentence: the |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 👍 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. |
…tus comment, and the 429 in the preview caveat Change-Id: If41a01c5adaf3135d96afc0d695d444c2d697adf Signed-off-by: Thomas Kosiewski <tk@coder.com>
Merge and note-write failures now use the bodies upstream documents: upstream's
MergeConflictfor merge file conflicts, and the plain problem body for every other failure. This is part 3, the last part, of item 3 of #352 (scope: issuecomment-6628583093).Why
Upstream's spec documents a plain
ProblemDetailsbody for every merge and note-write failure, with these exceptions:MergeConflict(application/json):error,conflict_type(merge_conflict,target_movedorrepo_changed),conflict_pathsandmerge_base_sha, with noresult. "A problem body reports any other conflict."NotesWriteRefGuardProblemDetails), which open.storage already sends.open.storage sent the result envelope (with
conflict_pathsfor conflicts) on every merge failure and the note envelope on every note-write failure. SDK 0.1.0 turned all of them intoRefUpdateError"409: request failed". With aMergeConflictbody it givesMergeConflictError.What changes
wmFailis gone. File conflicts writeMergeConflictwithconflict_type: merge_conflict. A lost CAS on the target ("moved concurrently") writes a 409conflictproblem body: upstream'starget_movedandrepo_changedreport a failed automatic retry, and open.storage does not retry (Codex round 4). A non-fast-forwardff_onlymerge and unrelated histories answer 412precondition_failed(they were 409), because upstream's merge 412 names both. The 400s usewriteInvalid, the 404sbranch_not_found, and the 403s, 429 and 500 the generic codes.bad_request, 403, 404 (ref_not_foundfor anobject_refthat does not resolve), 409conflict(note exists, lost notes CAS), 429. The lost notes CAS now carries problem members, which the 2026-10-27 comment on api: problem-details later tiers (specific codes, instance, result envelope, git/LFS writers, problem+json) — held on SDK trigger #352 left open until G1.ref_guard_failed). fix(api)!: answer stale commit and notes guards with 409 ref_guard_failed #661 moved the body forms' stale guard from 412 to that 409, sowrefNoteFaillost its last caller, and I deleted it.TestLostCASBodiesforces a lost CAS with aRefStoredecorator for both merge paths (fast-forward and merge commit) and for a note write. The merge and notes tests assert the new shapes. The 429 row-cap test now expects the problem body on merge too.http-api.md(Errors, merge row, notes), and the RFC 9457 row ofwire-compatibility.md.invalid_params(Codex round 1), a note on an absent full object id answers 404ref_not_found(Codex round 1), the 412 above (the advisor), the lost-CAS problem body above andinvalid_paramson the conditionalauthor400 (Codex round 4).streaming-commit-protocols.mdnow names the 502 envelope that upstream documents and these handlers never send, and thewcResolveRepocomment says problem-body errors instead of envelope errors.Validation
go build ./...,golangci-lint run(2.15.0): 0 issues. Docs gate: 0 broken links.go test ./internal/apipasses.Dogfood
I used the verify-openstorage harness on a node built from
mainwith part 2 (74c96847) and on this change (66870438). Each run passed 21 checks.conflict_pathsMergeConflict:conflict_type: merge_conflict,conflict_paths: [README.md],merge_base_shaff_only, not a fast-forwardprecondition_failedbranch_not_foundrebase(400)bad_requestexpected_notes_ref_sha(409)ref_guard_failedmain.RefUpdateError"409: request failed" toMergeConflictErrorwith the message. Theff_onlymerge that is not a fast-forward becameAPIError412precondition_failed. The existing note becameConflictErrorconflict, and the missing noteNotFoundErrornot_found.Refs #352
BREAKING CHANGE: merge failures no longer carry the
resultenvelope. File conflicts answer 409 with upstream'sMergeConflictbody (error,conflict_type,conflict_paths,merge_base_sha), and every other merge failure answers the plain problem body. A non-fast-forwardff_onlymerge and a merge of unrelated histories answer 412precondition_failedinstead of 409. Note-write failures answer the plain problem body instead of the note envelope, except the 409 guard body. SDK 1.17.1 Go reads the note envelope only forapplication/json(util.go:154), so it now reports these note-write failures as a genericAPIErrorinstead of aRefUpdateError. I did not check the 1.17.1 TypeScript and Python clients.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high