Repository navigation
fix(relay): publish a git push only when refs changed; check the protocol by refinement - #9425
Conversation
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
Without --atomic, git receive-pack applies each ref update independently. A stale old value or a ref-name conflict rejects one ref while its siblings land in the hydrated workspace and are reported ok to the client. finalize_push treated any ng line as a rejected push, skipped the CAS, and returned git's report verbatim, so the client recorded ref updates the relay discarded. Skip publication only when the report rejects every ref, as a pre-receive hook decline does. cas_publish snapshots the workspace refs, so only the accepted updates are published. Signed-off-by: Jordan Mecom <[email protected]>
…tcomes kind:30618 ref-state events are stored side by side, and readers take the one with the greatest created_at. Each event was stamped with the emitting instance's clock, so two pushes in one second tied (lowest id wins, so the older state won about half the time), and a push on a slower clock sorted before its predecessor. Each published manifest now carries published_at, strictly greater than its parent's, and the derived event uses it as created_at. The field is omitted when absent, so the shared announce seed and existing manifests keep their digests. finalize_push and the default-branch setter ran the pointer CAS under ServingWriteGuard::protect, which discards a completed operation when the lease check after it fails. A missed heartbeat renewal after a landed CAS therefore answered 503 and skipped the 30618, although the pointer had moved. Use protect_landed, keep the CAS outcome, and let the lease-guarded 30618 insert decide whether publication may continue. A failed lease release after publication no longer turns success into 503 either. Add docs/spec/GitPushPath.qnt, the Quint model of the push path that found these defects and the partial-push one, with a module per defect. Signed-off-by: Jordan Mecom <[email protected]>
…atabase The previous TLA+ model of the Git-on-object-storage protocol checked eight invariants over a single ref and pointer-id bookkeeping. It had no specification to compare the protocol with, no authorization, no reader, no objects, and it modeled a branch of the push path that the code does not have. It was not run in CI. This replaces it with: - `GitRefDB.tla`, the specification: a ref database in which a push takes effect in one step, is applied only if authorized and only against the old values it names, and is reported truthfully. - `GitOnObjectStore.tla`, the protocol: several refs, an object graph with real reachability, packs, compaction, the pre-receive hook, the publish decision, the default-branch setter, repository creation and a reader. `git receive-pack` is modeled by a stated contract. - The correctness statement `Refines`: every behavior of the protocol is a behavior of the specification. TLC checks it together with the store invariants (fence, reader completeness, closure). - `check.sh`, which pins TLC by SHA-256 and runs 28 checks in three groups. Eight instances must pass. Nineteen seeded design errors must each be detected with the expected violation. One coverage check confirms that a behavior the properties depend on (git reporting `ok` for a command that changes nothing) is reachable. - `lean/GitPointer.lean`, a Lean 4 proof (no dependencies) of the pointer discipline for any number of clients, refs and objects: fence, closure, every pointer change is a legal step from the value it replaces, and a successful conditional write implies no intervening write. - A CI workflow that runs TLC and Lean when a file under `docs/spec` changes. Two publish rules the relay has used are among the seeded errors. The original rule publishes when git exits 0 and reports no `ng`. The status-report rule publishes when git exits 0 and reports an `ok` or no `ng`. The model shows that each publishes a request the push policy did not approve (`PublishIsAuthorizedChange`), and that each can drop or misreport an update when the process stops between the conditional write and the response. `GitPushPath.qnt`, the Quint model of the serving lease and the kind:30618 timestamps, is extended to match. The publish rule becomes a parameter with the three values above, a request may carry no commands or come from a client without a status report, and the new invariant `publicationChangesRefs` states that a published manifest differs in its refs from the one it replaces. The implemented rule satisfies all four invariants in simulation and in a bounded check with Apalache; the status-report rule violates the new one. Quint is not run in CI. `docs/git-on-object-storage.md` is revised to match. The push listing follows the implemented order (one pointer read, receive-pack, decision from the observed refs, uploads, conditional write, response). The verification section separates definitions, assumptions, results checked by TLC, results proved in Lean, results checked with Quint, evidence about the implementation, and limitations. It states one assumption about clocks (C1) on which the order of kind:30618 events depends when a manifest's parent carries no `published_at`. The document describes the publish decision as the next commit implements it. The invariant names cited by code comments and by migration 0002 are kept as aliases in the TLA+ module. Signed-off-by: Jordan Mecom <[email protected]>
8700f17 to
8bc4758
Compare
The relay decided whether to publish a push from the exit status of `git receive-pack` and from its status report: publish when git exited 0 and the report had an `ok` line or no `ng` line. That rule handles a push in which one ref is updated and another refused, but the report is not evidence of what git did: - git exits 0 for a request that carries no commands, and for a push the pre-receive hook declined. A decline is reported only to a client that negotiated `report-status`. For a request with no commands, and for a decline when the client did not negotiate a report, there is no `ng` line, so the relay went on to the publish path, wrote the pointer, and emitted a relay-signed kind:30618 event naming the caller, although no push had been accepted. - The report is read from a stream whose framing the client's capabilities determine (side-band or plain pkt-lines), so the decision depended on parsing a client-shaped stream correctly. The decision is now taken from the workspace. `push_disposition` compares the refs after receive-pack with the refs the workspace was hydrated from: unchanged refs are answered with git's output and nothing is published; changed refs are published if git exited normally; changed refs after an abnormal exit fail the request. The snapshot that is compared is the one `cas_publish` publishes, so git's per-ref report is true of the published state. The status-report scanner and its tests are removed. The snapshot parser now fails closed. Output that is not UTF-8 used to read as an empty ref set and a malformed line was skipped, either of which would publish a manifest that lacks refs. `manifest_changed` in `finalize_push` could not be false and is removed; a push that changes nothing no longer reaches that code. Tests: - `push_is_published_exactly_when_receive_pack_changed_refs` runs real git through the production `run_git_at` and `observe_push` for seven ways a push can end, including the ref-name conflict that the removed scanner test covered. It is Unix-only because its hook fixture sets file modes. - `finalize_push_publishes_nothing_when_receive_pack_changed_no_ref` calls `finalize_push` with an unchanged workspace and checks that the pointer is not written and no kind:30618 event is stored. It needs Postgres and an object store. Both fail if the decision is taken from the exit status alone. Signed-off-by: Jordan Mecom <[email protected]>
8bc4758 to
2e3283b
Compare
…s in CI Moving the ref snapshot into observe_push sent its CasError::ResourceLimit through a catch-all 500 arm. Map it back to 413, as the publish path did, and pin it with an infra-free finalize_push test. Add api::git::cas_publish::tests and transport::track_c_tests to the infra-free unit lane (and its cargo-test fallback) so the publish-decision tests run in CI; their Postgres/MinIO cases stay #[ignore]d. Qualify the doc's Lean claim: a successful write rules out intervening writes that changed the pointer. The model omits the default-branch setter, whose same-value write rewrites the same body. Signed-off-by: Tom Brow <[email protected]> Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@localhost>
brow
left a comment
There was a problem hiding this comment.
🤖 Tom's pre-review recommends landing this with one small fix, #9429 push fixture CI fix: without it the new relay tests run in no CI lane, one of them fails on the CI runner's git 2.56 once they do run, and an oversize ref snapshot answers 500 instead of 413. Please merge the fix into your branch (it needs no approval there: block/buzz's review rules cover only main and the mobile release branches, so anyone with write access can merge a PR that targets their own branch); this PR is then approved and merged on Tom's go-ahead.
buzz://message?channel=e71f9a2a-ac46-473d-a24e-5e16fd8cb7b1&id=888f64b6a44b921b538bb3b822b490c6e7792fd46bc4c0c0879210835626ac22
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed this PR and the Nexus review-tracker notes. Approved with the non-blocking comment below.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
| for n in "${selected[@]}"; do [ "$n" = "$1" ] && return 0; done | ||
| return 1 | ||
| } | ||
|
|
There was a problem hiding this comment.
[P2] Reject unknown model-check selectors
Running docs/spec/check.sh definitely-not-a-check exits 0 and reports that all checks passed, although none ran. wanted() silently skips every entry and failures stays zero. Could this reject unknown names up front so a typo cannot produce false-green targeted verification?
Posted by Codex (gpt-6.1-sol) on behalf of kylez
There was a problem hiding this comment.
🤖 Fixed in 56fe89b. check.sh now compares its arguments with the names on its own check lines (plus refdb) before doing any work. An unknown name exits with status 2 and prints the known names, so docs/spec/check.sh definitely-not-a-check no longer reports success. The full suite (28 checks) and a three-name selection still give the expected results.
`docs/spec/check.sh NAME...` runs only the named checks. A name that matched no check selected nothing, and the script then exited 0 and reported that all checks gave the expected result, so a misspelled name looked like a successful verification. The script now compares its arguments with the names on its own `check` lines (plus `refdb`) before doing any work, and exits with status 2 and the list of known names if one does not match. Signed-off-by: Jordan Mecom <[email protected]>
Signed-off-by: Jordan Mecom <[email protected]>
|
🤖 #9429 is merged into this branch as bc07280 (fast-forward, so the commit is unchanged). I checked its three findings against the code and agree with each: the two test modules were in no unit-test lane, the snapshot resource limit had become a 500 on the path this PR introduced, and the Lean statement about intervening writes was stated too broadly. On the merged head the 45 newly selected tests pass locally, including Two further changes since the reviewed head: 56fe89b makes |
Co-authored-by: mboston.Engineer <932d3b6ebea6cc83e091551ae46d1c7e7f8f7d0fa8e9cef5e6facc8544af413b@buzz.block.builderlab.xyz> * origin/main: fix(mobile): require Builderlab consent for existing identities (#8823) feat(mobile): use HTTP/2 on iOS through app-wide URLSessions (#8897) fix(relay): publish a git push only when refs changed; check the protocol by refinement (#9425) fix(mobile): show shared chats under Solo (#9348) chore(release): release Buzz Desktop version 0.5.29 (#9456) feat(mobile): render messages with gpt_markdown 1.3.4 plusparse (#8945) fix(acp): bound observer retention by bytes (#7987) (#9436) Tune release codegen for faster/smaller binaries (#9434) feat(desktop): add dismissible banner pointing to the new buzz app (#9379) fix(mobile): keep publishing presence while the app is open (#9441) feat(mobile): share into Buzz from the iOS share sheet (#9433) fix(mobile): qualify every namesake mention and keep authored mention casing (#9325) fix(mobile): match browser GIF timing in the iOS reaction sheet (#9437) fix(mobile): play short-delay animation frames at browser speed (#9432) buzz-search: read profile names without the profile_name_fields function (#9397) buzz-db: build the community deletion lock key in Rust (#9396) fix(cli): pair for the active community, skip stopped desktops, accept a bare relay host (#9417) test(mobile): run push tests without a gateway dart-define (#9423) fix(mobile): never pop Home when archiving from channel settings (#9387) perf(acp): reduce observer CPU and lock overhead (#9381) Signed-off-by: Matthew Boston <[email protected]>
…count-backfill Co-authored-by: mboston.Engineer <932d3b6ebea6cc83e091551ae46d1c7e7f8f7d0fa8e9cef5e6facc8544af413b@buzz.block.builderlab.xyz> * bostonaholic--agent-reply-count: fix(mobile): require Builderlab consent for existing identities (#8823) feat(mobile): use HTTP/2 on iOS through app-wide URLSessions (#8897) fix(relay): publish a git push only when refs changed; check the protocol by refinement (#9425) fix(mobile): show shared chats under Solo (#9348) chore(release): release Buzz Desktop version 0.5.29 (#9456) feat(mobile): render messages with gpt_markdown 1.3.4 plusparse (#8945) fix(acp): bound observer retention by bytes (#7987) (#9436) Tune release codegen for faster/smaller binaries (#9434) feat(desktop): add dismissible banner pointing to the new buzz app (#9379) fix(mobile): keep publishing presence while the app is open (#9441) feat(mobile): share into Buzz from the iOS share sheet (#9433) fix(mobile): qualify every namesake mention and keep authored mention casing (#9325) fix(mobile): match browser GIF timing in the iOS reaction sheet (#9437) fix(mobile): play short-delay animation frames at browser speed (#9432) buzz-search: read profile names without the profile_name_fields function (#9397) buzz-db: build the community deletion lock key in Rust (#9396) fix(cli): pair for the active community, skip stopped desktops, accept a bare relay host (#9417) test(mobile): run push tests without a gateway dart-define (#9423) fix(mobile): never pop Home when archiving from channel settings (#9387) perf(acp): reduce observer CPU and lock overhead (#9381) Signed-off-by: Matthew Boston <[email protected]>
Summary
This PR is stacked on #9064. It does two things to the Git-on-object-storage code in
buzz-relay. It replaces the TLA+ model of the push protocol with one that states what the protocol is supposed to implement and checks that it does. It also changes the rule by which the relay decides whether to publish a push, because the new model shows that the rule #9064 introduces still publishes requests in which git accepted nothing.Relationship to #9064
The branch is
main, then the two commits of #9064, then four commits of this PR: the specification work, the publish-rule change, a review fix merged from #9429, and a fix tocheck.sh. A merge ofmainfollows them; it was needed becausemainchangedJustfile, which #9429 also edits, after the branch point. The first two commits are #9064's, replayed onto currentmainwithout conflicts and otherwise unchanged. The base of this PR staysmainbecause #9064's branch is about 200 commits behind it. Once #9064 merges, this branch can be rebased and the diff shrinks to the last four commits. Until then, review the commits after the first two.This PR keeps two of #9064's three changes as they are: the
published_atstamp that orders kind:30618 events by the manifest chain, andprotect_landed, which lets a landed conditional write stand when the serving lease lapses after it. It replaces the third, the publish rule.The publish rule
#9064 publishes when git exits 0 and the status report has an
okline or nongline. That handles a push in which one ref is updated and another refused. The report, however, is not evidence of what git did.report-status. For a request with no commands, and for a decline when the client did not negotiate a report, there is nongline. The relay then writes the pointer and emits a relay-signed kind:30618 event that names the caller as pusher, although the push policy approved nothing.The decision is now taken from the workspace.
push_dispositioncompares the refs after receive-pack with the refs the workspace was hydrated from. Unchanged refs are answered with git's own output and nothing is published. Changed refs are published if git exited normally. Changed refs after an abnormal exit fail the request. The snapshot that is compared is the onecas_publishpublishes, so git's per-ref report is true of the published state. The status-report scanner and its tests are removed, and the ref-name conflict case they covered moves into the table-driven test described below.Three smaller changes follow. The ref-snapshot parser now fails closed: output that was not UTF-8 used to read as an empty ref set and a malformed line was skipped, and either would publish a manifest that lacks refs.
manifest_changedinfinalize_pushcould not be false and is removed. Taking the snapshot before the publish decision moved its resource-limit error (snapshot larger than 4 MiB, or more refs than a manifest may hold) onto a path that answered 500; #9429 restores the 413 the publish path gave before and addssnapshot_resource_limit_answers_413.What the specification work adds
docs/spec/GitRefDB.tlais the specification: a ref database in which a push takes effect in one step, is applied only if authorized and only against the old values it names, and is reported truthfully.docs/spec/GitOnObjectStore.tlais the protocol: several refs, an object graph with reachability, packs, compaction, the pre-receive hook, the publish decision, the default-branch setter, repository creation, and a reader.git receive-packis represented by a stated contract rather than modeled internally.Refines: every behavior of the protocol is a behavior of the specification. TLC checks it together with the store invariants (fence, reader completeness, closure of published manifests).docs/spec/check.shpins TLC 1.7.4 by SHA-256 and runs 28 checks. Eight instances must pass. Nineteen seeded design errors must each be detected with the violation named for them. One coverage check confirms that a behavior the properties depend on (git reportingokfor a command that changes nothing) is reachable. A name passed tocheck.shthat is not a check is an error, so a misspelled selection cannot report success.ng) and fix(relay): Git pushes report and publish what actually landed #9064's status-report rule. For each, TLC reports a publication the policy did not approve (PublishIsAuthorizedChange) and a violation ofRefines.docs/spec/GitPushPath.qnt, the Quint model from fix(relay): Git pushes report and publish what actually landed #9064 that covers the serving lease and the event timestamps, is extended. The publish rule becomes a parameter with three values, a request may carry no commands or come from a client without a status report, and a new invariant,publicationChangesRefs, states that a published manifest differs in its refs from the one it replaces. The implemented rule satisfies all four invariants; the status-report rule violates the new one with a zero-command request.docs/spec/lean/GitPointer.leanis a Lean 4 proof, with no dependencies, of the pointer discipline for any number of clients, refs and objects: the fence, closure, that every pointer change is a legal step from the value it replaces, that the length of the pointer's predecessor chain equals the number of pointer writes, and that a successful conditional write implies no intervening write..github/workflows/spec.ymlruns TLC and Lean when a file underdocs/spec/changes. On this PR's first run the TLC job took 10 minutes 19 seconds and the Lean job 22 seconds. Quint is not run in CI.docs/git-on-object-storage.mdis revised to match. The push listing follows the implemented order. The verification section separates definitions, assumptions, results checked by TLC, results proved in Lean, results checked with Quint, evidence about the implementation, and limitations. The invariant names cited by code comments and by migration 0002 are kept as aliases in the TLA+ module.Assumptions and limitations
The TLC results hold for the bounded instances in
check.sh, not for arbitrary sizes. The Lean proof is unbounded but covers the pointer discipline only, not the refinement. The Quint results are from random simulation (2,000,000 runs of at most 14 steps) and a bounded symbolic check (8 steps, Apalache); neither is a proof for unbounded runs. All of them rest on the assumptions named in the document: the object store's conditional-write semantics, ETag equality implying body equality, no SHA-256 collision among the values that occur, the stated contract forgit receive-pack, and the policy endpoint deciding authorization. The correspondence between the models and the Rust code is established by reading the code and by the tests below; it is not mechanized.The document states one assumption that #9064's ordering fix depends on and that no model covers (Assumption C1). A manifest whose parent carries no
published_at(the empty manifest written when a repository is announced, or a manifest written before the field existed) is stamped with the writing instance's current time plus one second. Its event is selected over the parent's only if that value exceeds the parent event'screated_at, which was taken from the clock of the instance that emitted it. This holds when one instance handles both, and otherwise when the writing instance's clock is not behind the other's by more than the time elapsed between the two events. The assumption concerns one write per repository. The document lists its removal as a proposed extension, for example by having the first push after an unstamped manifest read the stored event'screated_atand exceed it.Related issue
Related to #2621 (CI for the formal models). This adds the TLC job for the git model and a Lean job; it does not cover the Tamarin models or the NIP-PL scripts, so it should not close that issue.
Testing
Run locally on the restacked branch:
docs/spec/check.sh: 28 checks, each with the expected result (8 pass, 19 seeded errors violate the named property, 1 coverage witness reached). About three minutes on 16 cores; the largest instance has 6,773,286 distinct states.lake buildindocs/spec/lean(Lean 4.33.1): succeeds. The safety theorems depend only onpropext,Classical.choiceandQuot.sound.GitPushPath.qnt:quint run --main fixed --invariant safety --max-steps 14 --max-samples 2000000 --seed 0x1reports no violation;quint verify --main fixed --invariant safety --max-steps 8(Apalache 0.62.1) reports no violation; each single-defect module produces the counterexample the document lists.cargo fmt --all -- --checkandcargo clippy -p buzz-relay --all-targets -- -D warnings: clean.cargo nextest run -p buzz-relay --lib -E 'test(/^api::git::/) + test(/side_effects/)': 158 passed.finalize_pushtests (api::git::transport::track_c_tests::external_infra_minio_tests, with--run-ignored ignored-only) against local Postgres and MinIO: 5 passed.e2e_gitagainst a local relay built from this branch, with Postgres, Redis and MinIO: 3 passed (clone/push/fetch/force-push round trip, concurrent push, partially refused push). The storage probe in that test needsBUZZ_E2E_GIT_COMMUNITY_IDset to the local community's id.just file-size-checkandjust security-review-check: pass.Two regression tests bind the changed decision to production code, and both were confirmed to fail when the decision is forced back to the exit status:
push_is_published_exactly_when_receive_pack_changed_refsruns real git through the productionrun_git_atandobserve_pushfor seven ways a push can end: a request with no commands, a push the hook declines with and without a status report, an accepted push, an accepted command that asks for the value the ref already has, a push in which one ref is updated and another refused for its old value, and a push that creates two refs whose names conflict, of which git creates one and refuses the other. It needs no services. Which of the two conflicting refs git creates depends on the git version (2.50 creates the first command's ref, 2.56 on the flake-check runner creates the other), so the test accepts either outcome and requires that each ref is in the published snapshot exactly when git reportedokfor it. The first version of this case assumed one outcome and failed the Rust flake check for that reason. Before fix(relay): keep 413 for ref-snapshot limits and run git publish tests in CI #9429,just test-unitselected neitherapi::git::cas_publish::testsnorapi::git::transport::track_c_tests, so this test ran only in the flake check; both modules are now in the unit-test lane (45 tests, none needing services). It is Unix-only because its hook fixture sets file modes; the first push of this PR failed Windows clippy for that reason, and the test and its fixture are now gated with#[cfg(unix)]. That gating has not been compiled for Windows locally, so the Windows CI job is the check.finalize_push_publishes_nothing_when_receive_pack_changed_no_refcallsfinalize_pushwith an unchanged workspace and checks that the pointer is not written and no kind:30618 event is stored. It needs Postgres and an object store.Not done, or not covered:
finalize_pushtests ore2e_git, so the second regression test and the end-to-end tests are evidence only when someone runs them. Adding them to CI needs a change to the test fixture, which applies migrations itself, and is left as a follow-up.just ciwas not run in full.AGENTS.mdis not met, andbuzz-review-completedis not claimed.The local secret-scanning hook (
sadscan) was skipped for the commits and pushes that touchcrates/buzz-relay/src/api/git/settings_tests.rs. It scans whole files and flags the P-256 test key fixtures that file already contains onmain; this branch adds no key material.