Repository navigation
Conversation
…and grep responses A response path that is not valid UTF-8 keeps its raw bytes in <field>_base64.
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (2)
ℹ️ 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. Bravo. 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. |
…sed schemas Review follow-ups for the merge siblings.
|
@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. |
Both fields of a commit-pack file operation count as absent when empty, the fuzz target checks the resolved path, and the docs describe rolling upgrades correctly.
A renamed file above the inline diff limit lands in filtered_files. No test filled its old_path_base64 field.
|
@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". |
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: d9b89c728d
ℹ️ 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.
…tra copy A push can make the conflict paths as long and as many as it likes. The sibling would have added a second and third copy of them to the request's memory (about 3 times the allocation on main). The 409 body now writes both lists item by item.
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 073fc00acb
ℹ️ 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 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 conflict path of any length costs no second string and no marshal buffer for conflict_paths_base64.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: dc2aad173f
ℹ️ 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.
conflict_paths_base64 and the other lists of a conflicted preview are written item by item, so the sibling holds no copy of the conflict paths.
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2b386a6a0
ℹ️ 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".
| } | ||
| conflicts = append(conflicts, wmPreviewConflict{ | ||
| Path: path, Result: blobs[0], Base: blobs[1], Ours: blobs[2], Theirs: blobs[3], | ||
| Path: path, PathBase64: rawPathBase64(path), Result: blobs[0], Base: blobs[1], Ours: blobs[2], Theirs: blobs[3], |
There was a problem hiding this comment.
Stream nested preview path siblings lazily
When include_content=true returns many invalid-UTF-8 conflict names, wmRenderConflicts eagerly allocates and retains a base64 string for every conflicts[].path_base64 (and likewise for filtered_conflicts) before wmWritePreview starts streaming. Thus a push-controlled merge-tree output containing tens of MiB of such paths adds roughly 4/3 of their total size to the request heap, despite the new streaming path being intended to avoid a sibling-sized copy. Generate these nested siblings while each item is written rather than storing them in the response slices.
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. |
Why
ThomasK33 asked to fix and build the open issues of this repo and land them on main. For #587 (a path that is not valid UTF-8 comes back changed in JSON) this is the second of three PRs. #721 is on main. #721 gave the file, diff, blame and grep responses an exact
<field>_base64sibling. This PR does the same for the merge conflict paths. It also ships four follow-ups that the clean-context review of #721 raised.What changes
Merge conflict responses (a path that is not valid UTF-8 gets a sibling, a valid path gets none, so a conflict on valid names answers the same bytes as before):
POST /repos/merge, 409 bodyconflict_paths_base64, same length and order asconflict_pathsGET /repos/merge/previewconflict_paths_base64, andpath_base64inconflicts[]andfiltered_conflicts[]The 409 body streams its two path lists (
writeJSONListsnow also takes a 409). A push can make the conflict paths as long and as many as it likes, andPOST /repos/mergehas no read budget. Buffered, the sibling tripled the allocation (191 MiB against 564 MiB for 32 MiB of conflict paths). Streamed, no copy of either list is held. Each sibling item also writes through a base64 encoder (base64JSONString), so a path of any length costs no second string and no marshal buffer: an invalid 8 MiB path allocates the same as a valid one.TestMergeConflictBodyStreamsfails when the body is written whole, andTestMergeConflictSiblingHoldsNoCopyOfAPathfails when the sibling holds a copy of a path. A conflicted merge preview body streams all of its lists (wmWritePreview) the same way, soconflict_paths_base64adds no copy there either.TestMergePreviewStreamschecks the bytes againstjson.Marshal, the 64 KiB write chunks, and that an invalid 8 MiB path allocates the same as a valid one.Follow-ups from the review of #721:
TestRawPathSiblings/files_paged_and_flatnow checkspaths_base64on a flat listing, where a tree entry sits inentriesbut not inpaths. A reviewer's mutant, which encoded every entry instead of the blobs, survived the whole package before. It now fails this test.The path-field guard test is harder to get around. It reads every struct type in the package, also types declared inside a function or a
var. It accepts any field type, compares the sibling's type with the field's, and flags an untagged field with a path-like Go name in a struct that has json tags. A synthetic source in the test names each shape. The guard's exemptions shrink to the two request fields (the last PR).docs/reference/http-api.mdsays that a list sibling repeats the whole list, so an unpagedfileslisting with one invalid path grows by about 1.33 times the size ofpaths. It also says that a replica without the siblings sends none during a rolling upgrade, so a missing sibling proves nothing until every replica has them.docs/explanation/wire-compatibility.mdnames the siblings in the list of extensions that upstream's spec does not list. It also says that upstream's schemas for these responses are closed (additionalProperties: false), so a client that validates a response against the vendored spec as written rejects one with a sibling.TestRawPathFilteredRenamefillsfiltered_files[].old_path_base64, which no test did (the verifier's mutant R13 survived). A renamed file above the inline diff limit lands infiltered_files, and the test checks bothpath_base64andold_path_base64.Not changed: the guard reads struct types and not map literals. No response in this package puts a path key in a map literal, so a rule for map keys would have nothing to test against.
Verification
go build ./...,go vet ./internal/api/,golangci-lint2.15.0 on./internal/api/...: 0 issues. Doc link gate: 0 broken.go test ./internal/api/... ./cmd/openstorage/... -tags=openstorage_viewcheck -race -skip '/^pg$': ok (internal/api173 s,cmd/openstorage20 s). CI runs both backends.409 conflict_paths_base64 not setfailsTestRawPathMergeConflicts/merge_409, and a removedwmPreviewFiltered.PathBase64field fails the guard withwmPreviewFiltered.path has no path_base64 sibling.serveand real git: two branches that conflict ona<0xff>b,a<0xfe>b,plain.txtandbig<0xfa>.txt(one side above the 1 MiB inline limit). 409 and merge preview (with and without content) carry the siblings: 6 checks pass, and theourscontent of each conflict is read through the raw path.Refs #587
Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh