Repository navigation
fix(api)!: answer stale commit and notes guards with 409 ref_guard_failed - #661
Conversation
…iled A stale expected_target_sha on commit-pack, diff-commit and restore-commit, and a stale expected_notes_ref_sha on the legacy notes body forms, now answer 409 ref_guard_failed with guard and current_sha next to the result members (spec 2026-10-27), instead of 412 precondition_failed. Gate G1 of #352 fired with SDK 0.1.0. Fixes #647 Change-Id: I93474abed354d3e72fe06c6bfa85b8d6cf6ab737 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@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 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: ac24914ccc
ℹ️ 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".
Codex round 1 (P2): the commit-write guard body reused wcResult, which has no target_branch and omits an empty new_sha, both required by upstream's RefUpdateResult. Change-Id: Ia67c186558025c8ac1eaa5c1ef10860154cf9452 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@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. |
The spec's CommitWriteRefGuardProblemDetails and RestoreCommitRefGuardProblemDetails descriptions say commit is null. Change-Id: I33fca2be10a1bd7a3af54316d30873e9d46e14e9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@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. |
Change-Id: Ic1aa09ca7c3db9fa050f31cb62ea7d0afb921536 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@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. |
Change-Id: I4f85fb7af15375ecb69be9e2f73c6e9a49abf9a0 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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. |
A stale ref guard on the four write forms that still answered our
412deviation now answers upstream's409ref_guard_failed:commit-pack,diff-commitandrestore-commitwith a staleexpected_target_sha(or the deprecatedexpected_head_sha).expected_notes_ref_sha(or the deprecatedexpected_ref_sha):POST .../notesand the deprecatedDELETE .../notesbody.This is part 2 of the desk decision on #647. Gate G1 of #352 fired on 2026-11-04 with the stable SDK 0.1.0 releases.
What the 409 body holds
The body keeps the members it had and adds the problem members, as upstream's spec of 2026-10-27 says (
CommitWriteRefGuardProblemDetails,RestoreCommitRefGuardProblemDetails,NotesWriteRefGuardProblemDetails):code: ref_guard_failed,type,title,status,detail,errorandinstance.guard:expected_target_shaorexpected_notes_ref_sha, also when the request sent the deprecated alias.current_sha: the commit the guard was checked against. For a commit write that is the target tip, or thebase_branchtip when the target branch is new. For a notes write it is the notes ref tip, or 40 zeros when the notes ref does not exist.resultwithstatus: conflict(and the note members for notes). On the commit routes,resulthas every member that upstream'sRefUpdateResultrequires:target_branch,branch,old_sha,new_sha(empty),successandstatus, plusmessage.result.old_sharepeatscurrent_sha, except on a new branch, where it stays 40 zeros asRefUpdateResult.old_shadefines.The body is sent as
application/problem+json, like the merge and notes guard bodies since #660, which this branch merged from main. On the commit routes,commitisnull, as the schema descriptions say.Code
wcFailRefGuardinhandlers_write_commit.gobuilds the commit-write body fromrefGuardProblem, the same builder that the merge and notes guards use, and writes it withwriteProblemJSON. All three commit routes reach it throughwcPlanCommit, so there is one call site.wrefHandleNotedrops the per-formstaleGuardStatusand always callswrefNoteGuardFail.expected_target_sha, as the merge guard's does, so theexpectedProblemCodesoracle maps it toref_guard_failed.Tests
TestCommitRefGuardContentTypepinsapplication/problem+jsonon the commit guard body, and the commit-pack stale-guard test checks the header end to end. The notes callers use feat(api)!: send application/problem+json on problem bodies #660'sassertNotesRefGuard, which checks the header too.assertCommitRefGuard(problem_test.go) checks the result members (their set must equal the vendoredRefUpdateResultproperties), the guard members and, throughassertProblemFields, the problem members.TestProblemMembersMatchUpstreamnow pins the allOf members ofCommitWriteRefGuardProblemDetailsandRestoreCommitRefGuardProblemDetailstoresult.write_commit_test.go,upstream_names_test.go,upstream_ops_test.goandwrite_refs_test.gonow expect the 409 body.TestGuardSHAUppercasechecks the full body on all three commit routes. The base-branch case checkscurrent_sha= base tip andold_sha= 40 zeros.origin/main, 7 of the changed tests fail on both backends.Docs
docs/explanation/wire-compatibility.md: the four api: a stale expected_*_sha on commit-pack and diff-commit answers 412; the upstream spec lists only 409 #533 deviation rows are gone. The problem-details row names the commit and notes guard bodies.docs/reference/http-api.md: new "Commit write guards" section, updated notes bullets, conventions text and status table. This also closes follow-up 1 of the docs(api): name both SDK lines in the notes stale-guard deviation row #649 merge record.streaming-commit-protocols.mdstep 2,domain-model.mdinvariant 1, and theverify-openstoragecommit-pack recipe.Dogfood
A local
osv.shnode built from this head, driven by the Go SDKgithub.com/pierrecomputer/sdk/gov0.1.0 (taggo/v0.1.0=5f1a4bc). Each stale guard came back as a*pierre.RefGuardError(status 409, unwraps to*pierre.ConflictError) with the expectedguardandcurrent_sha:63f81faaRefUpdateError412precondition_failedRefGuardError,current_sha= tipbase_branchRefUpdateError412RefGuardError,current_sha= base tipRefUpdateError412RefGuardError,current_sha= tipRefUpdateError412RefGuardError,current_sha= tipRefUpdateError412RefGuardError,current_sha= 40 zerosRefUpdateError412RefGuardError,current_sha= notes tipRefGuardError409RefGuardError409 (unchanged)The branch tip did not move in either run. The table rows are from head
ac24914c. The probe passed again on heads9773c1e6,3fc8d5ec,3b3b85c0and809fbe54(main merged in twice), and a jsonschema check of each 409 body against the vendoredCommitWriteRefGuardProblemDetails,RestoreCommitRefGuardProblemDetailsandNotesWriteRefGuardProblemDetailspassed for all 7 bodies on each. On3b3b85c0and809fbe54every 409 came withContent-Type: application/problem+jsonand every 200 withapplication/json. The same check fails onac24914c(notarget_branchandnew_sha, Codex round 1) and on the baseline. The updated commit-pack recipe passed with 46 of 46 checks.The old Go SDK 1.17.1 (
code-storage-go), same probe shape, againstorigin/main3c6bfde3and heads3b3b85c0and809fbe54(same result on both):3c6bfde3RefUpdateErrorprecondition_failedRefUpdateErrorconflictRefUpdateErrorprecondition_failedAPIError409, message = raw body (it parses note bodies only forapplication/json)SDK 0.1.0 signs its tokens with
sub: pierre-sdk-go, which open.storage rejects with 401 today. That is tracked in #658. The probe therefore gave the SDK tokens fromopenstorage mintthroughoption.WithGetToken, with the scopes and repo claim that the SDK asked for. The requests and the error parsing are the SDK's own.Validation
go build ./...,golangci-lint runv2.15.0 (0 issues),python3 scripts/check-doc-links.py(0 broken),python3 scripts/test_check_doc_links.py(OK).go test -tags=openstorage_viewcheck -count=1 -raceon./internal/api/...,./cmd/openstorage/...and./internal/githttp/...atac24914c:okfor all three (230.7 s, 8.8 s and 825.1 s, under the shared heavy-validation lock). At9773c1e6(allRefUpdateResultmembers in the commit-guardresult), the./internal/api/...race lane passed (273.5 s). At3fc8d5ec(commit: null), the./internal/api/...race lane passed (692.6 s under load). At3b3b85c0and809fbe54(main merged in, with feat(api)!: send application/problem+json on problem bodies #660 and then feat(api): serve GET/POST /api/repos and the tag-in-path delete that SDK 0.1.0 calls #662): the focused guard and content-type tests pass locally, and CI'stestjob runs./...with the same flags.809fbe54only merges feat(api): serve GET/POST /api/repos and the tag-in-path delete that SDK 0.1.0 calls #662; the PR's own diff against main is the same as at3b3b85c0apart from hunk context.Fixes #647
BREAKING CHANGE: a stale
expected_target_shaorexpected_head_shaoncommit-pack,diff-commitandrestore-commit, and a staleexpected_notes_ref_shaorexpected_ref_shaonPOST .../notesand theDELETE .../notesbody, now answer409withcode: ref_guard_failed,guard,current_shaandresult.status: conflict, instead of412precondition_failed. Go SDK 1.17.1 (code-storage-go) callers now get aRefUpdateErrorwith statusconflicton the three commit routes. On a note create they get anAPIErrorwith status 409 whose message is the raw body, because that SDK reads note bodies only forapplication/json. Go SDK 0.1.0 (github.com/pierrecomputer/sdk/go) callers get aRefGuardErroron every form.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high