Repository navigation
push: stop before object transfer when required negotiation fails - #141
Open
friel-openai wants to merge 5 commits into
Open
friel-openai wants to merge 5 commits into
friel-openai wants to merge 5 commits into
Conversation
friel-openai
force-pushed
the
af/codex/required-push-negotiation
branch
from
October 14, 2026 17:50
6097c6d to
a1e201c
Compare
friel-openai
marked this pull request as ready for review
October 14, 2026 18:07
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. |
friel-openai
changed the title
ttaylorr-oai
approved these changes
Oct 14, 2026
When no commits are shared, negotiate_using_fetch() receives a valid NAK for all available haves, then sends another request with no haves. A server can answer that empty request with a packfile section, which the client rejects while expecting acknowledgments. Stop before sending another request once the negotiator is exhausted. Keep the first request even when add_haves() reports zero, because it may contain explicit negotiation-include commits. Only server ACKs are returned as common commits; a completed NAK-only exchange returns an empty set successfully. Cover both negotiation algorithms, a first request containing only included commits, and HTTP response cleanup. Keep total_rounds equal to the number of requests sent and balance the round trace regions. Signed-off-by: Friel <[email protected]>
A failed push negotiation currently emits a warning and proceeds to construct a pack. Users who rely on negotiation to avoid an expensive pack have no way to require it to succeed. Accept push.negotiate=required and abort when the negotiate-only fetch fails, before sending ref update commands or starting pack-objects. Keep false as the default and true as best effort, including existing boolean spellings and command-line configuration overrides. Successful negotiation can return no common commits. It does not impose a pack-size limit. Unsupported negotiation, transport errors and an interrupted fetch remain failures. Document these distinctions and test that failures start no pack-objects process and leave the target ref unchanged, while common and no-common pushes succeed. Signed-off-by: Friel <[email protected]>
Exercise HTTP 400 and 500 responses during the negotiation POST, with successful receive-pack discovery. Best-effort negotiation continues, while required negotiation aborts before pack-objects and leaves the pushed ref absent. Also cover required negotiation success, an unsupported wait-for-done capability, and a malformed acknowledgments section. Reuse the existing one-time response modifier so the fixtures need no new server handler. Signed-off-by: Friel <[email protected]>
WebDAV and remote-helper push/export can bypass send_pack(), so checking push.negotiate there alone can still allow an unsupported transport to upload objects. Share the configuration parser and reject required mode before entering http-push or starting fast-export. Check direct http-push before HTTP initialization as well, including mixed deletion/update batches that could otherwise partially update refs. Require push helpers to advertise push-negotiate-required before sending them updates. remote-curl advertises the capability because it now either uses send-pack's negotiation or rejects WebDAV. Successful helper connect takeover continues through native negotiation. Boolean settings retain their existing behavior. Advertise feature: push-negotiate-required in version --build-options so callers can distinguish complete enforcement from a version that ignores the configuration on some transports. Document the helper contract and cover unsupported helpers, connect fallback/takeover, export, and WebDAV with small generic regressions. Signed-off-by: Friel <[email protected]>
friel-openai
force-pushed
the
af/codex/required-push-negotiation
branch
from
October 16, 2026 05:10
27c328e to
530474a
Compare
The explicit-haves source prerequisite predates the CI updates already published in codex. Its unpinned actions are rejected before checkout by the repository's full-commit-SHA policy. Use the exact check-style, check-whitespace, and main workflow versions already accepted by ci_workflow_pins_are_reviewed(). This also retains the reviewed Debian image and Windows SDK setup. Keep workflow permissions and the controller's admission policy unchanged. Signed-off-by: Friel <[email protected]>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A failed push negotiation currently produces a warning and continues to
pack-objects. Addpush.negotiate=requiredso negotiation failures stop the push before ref update commands, pack construction, or upload.falseremains the default andtrueremains best effort; command-line configuration can explicitly overriderequired.The first commit fixes
fetch --negotiate-onlywhen no commits are shared: after a complete NAK-only exchange, stop once there are no more haves instead of sending an empty extra request. Preserve the first request containing only explicit negotiation includes. Only server ACKs identify common commits; a valid empty result succeeds and can still produce a large push pack.Required mode also rejects transports that bypass
send_pack(). Both WebDAV entry points stop before object transfer, including mixed deletion/update batches. Remote-helperexportstops beforefast-export. A helper usingpushmust advertisepush-negotiate-required, promising to enforce the inherited configuration.remote-curladvertises that capability and either uses native negotiation or rejects WebDAV. Successful helperconnecttakeover retains native negotiation. Supported transports may skip negotiation when there are no negotiation tips.git version --build-optionsadvertisesfeature: push-negotiate-requiredso callers can detect enforcement across transports without inferring support from a version number. The marker does not promise server compatibility.The topic starts at the admitted
tb/codex/explicit-havessource prerequisite. The four runtime/test commits are unchanged by the ancestry correction. A separate CI preparation commit retains the exact workflow versions already published incodexand permitted byci_workflow_pins_are_reviewed(), so this source branch satisfies the full-commit-SHA action policy.Validation:
t5549,t5556,t5702,t5801, andt5802, including smart HTTP and unauthenticated WebDAV.t5540andt0610. It changes no files beyond the original runtime patches and the independent fixes. This Linux validation does not replace native Windows checks or exact staging CI.codexpassed 71 SHA-256 tests and 61 reftable tests, with unsupported DAV cases skipped.wait-for-done, malformed acknowledgments, cancellation, common/empty results, explicit overrides, and unsupported helper/DAV transfers. Failure assertions check that packing/export/upload never starts and destination refs remain unchanged.propose-plan --no-pushdiagnostic accepted the corrected ancestry and effective writer approval. No plan or release was published by that diagnostic.Current-source CI completed with 39 successful jobs, six failures and two skips. All Windows and macOS variants passed. Five failed jobs report the existing
t5540.18andt5550.15/.16/.20URL-credential failures; Debian12 reportst5559.15HTTP2 half-auth clone failure and its dependent test16. ASan and leaks report those HTTP tests without a sanitizer finding. The ordinary HTTP implementation and failing fetch fixtures are unchanged from this topic’s prerequisite. These failures remain separate from this change; the complete CI matrix is not green.The independent fixes address WebDAV authentication, t5540.18, and concurrent reftable writers, t0610.48. The source branches and reviews remain separate. This topic PR is review-only; normal plan admission and successful full staging CI are still required before publication.
Codex Task:
codex://threads/01a13b71-995e-71f3-86ec-81ad7df43f1b-frielbot