Repository navigation
fix(api): send target_branch and every RefUpdateResult member in commit-write results - #668
Conversation
…it-write results Change-Id: Icfba172afa1a80e308621a91e58659603b380321 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I3ba86a922e6aa42a4a0d616a401468612055a3db 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 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. |
Change-Id: I68a4913144dccf47fd3c39543337c2f647e220d3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
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. |
Change-Id: Idd92749b23ecedffe54d3f0adffb07ff465d3e3c Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: If157de9c372f7e590caeedb682a769abbfc896a5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Delightful! 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. |
Why
Go SDK 0.1.0 decodes the
resultof every commit write intoRefUpdateResultand readsResult.TargetBranch. open.storage never senttarget_branch, so every SDK caller got""oncommit-pack,diff-commitandrestore-commit. Failure bodies also left out empty members andcommit, while upstream'sRefUpdateResultandCommitWriteResponserequire them. This PR follows the desk decision on #665: send the spec's members exactly.What changes for clients
resultofcommit-pack,diff-commitandrestore-commit(v1 and path-style routes) sendstarget_branch, the deprecatedbranch(same value, kept),old_sha,new_sha,successandstatus. A member with no value is"".commit: null, except the lost-CAS 409, which keeps the commit. Since feat(api)!: send problem bodies for write-route auth, rate-limit and alias failures #663 and feat(api)!: send problem bodies for commit-write failures upstream documents as ProblemDetails #669 the envelope is the body only on the statuses for which upstream documents it (commit-pack 400/409/412/413, diff-commit 400/409/412, restore-commit 412). Every other failure is a problem body with noresult, and this PR does not change those.Before, from a local node:
"result":{"branch":"main","old_sha":"…","new_sha":"…","success":true,"status":"ok","message":"commit created"}. After: the same plus"target_branch":"main".How
wcResultis now the fullRefUpdateResult.wcRefUpdatebuilds every one, sotarget_branchandbranchcannot differ andsuccessis true only for statusok. The separate guard typewcGuardResultfrom fix(api)!: answer stale commit and notes guards with 409 ref_guard_failed #661 is gone.wcEnvelope.commithas noomitempty.mergefailureresultkeeps its short form (wmFailResult, unchanged on the wire). Upstream documents merge failures as problem bodies, notRefUpdateResult. The other write routes (branches, tags, notes, credentials) are unchanged too.reset-commitsalso usesRefUpdateResultin the spec, but open.storage does not serve it (recorded deviation).Tests
TestRefUpdateResultMatchesUpstreampins the members to the vendored spec:RefUpdateResultrequired list and properties, and theCommitWriteResponseandReposCommitsRestoreResponserequired lists.wctRequireOKandwctRequireFailnow check the raw body of every commit-write answer in the suite: every required member, no extra member,target_branch == branch, andcommitpresent (null on failures other than a lost CAS).target_branchfrom the builder, putomitemptyback oncommitor onold_sha) each fail a test.go build ./...,golangci-lint run(0 issues), docs link check and its unit tests, and the race lane for./internal/api/...with CI's flags.Dogfood
Real Go SDK
github.com/pierrecomputer/sdk/gov0.1.0 with its own signed tokens (option.WithPrivateKey) against a local node. 15 checks over the three routes. Nine readResult.TargetBranchfrom the success answers and from the 400, 409 and 412RefUpdateErrors (including the stale guard), and check the raw failure body for every member andcommit: null. Six check that the 401s and 404s are typed problem errors (AuthenticationError,NotFoundError).TargetBranchis""on every commit write.verify-openstorage recipes on this branch:
commit-pack50 PASS, 0 FAIL (main fails at the newcp-target-branchcheck), andtoken-scopes37 PASS, 0 FAIL. On main,token-scopesfails atauth-ro-commit-statussince #663, because the commit-pack 403 has noresultany more. This PR updates that check tocodeinsufficient_scope.Docs
docs/explanation/streaming-commit-protocols.md§3 (envelope, failure example, 413 row),docs/reference/http-api.mdconventions, the mirror sentence indocs/explanation/wire-compatibility.md, and the verify-openstoragecommit-packandtoken-scopesrecipes.Fixes #665
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high