a73x

d53c2e87

Cover issue-side close/reopen convergence and release --json

a73x   2026-08-12 17:11

Commit message
Cover issue-side close/reopen convergence and release --json

Two gaps left open by 64194908, both coverage rather than behaviour: no
production code changes.

`reconcile_refs` globs the active *and* the archive sync prefix because
`close` moves a ref between namespaces while the id stays the same. Only
the patch side of that had a convergence test. `tests/issue_reopen_test.rs`
is its issue-side twin, and pins which side wins rather than only asserting
the two clones agree: the fold is ordered by `(clock, oid)` and a clock is
`max_clock(tip) + 1`, so an extra local event before the winning clone's
status event puts it a clock ahead — no tie, no dependence on how two oids
happen to sort. Both directions are covered, close-wins and reopen-wins,
plus that no clone's offline events are dropped by the merge. Reverting the
fix to glob only the active prefix fails all three, each on a different
assertion.

`release publish --json` and `release delete --json` were wired but never
run against a server. Four tests in `tests/release_cli_test.rs` assert the
whole contract under the SSH harness: exactly one JSON object on stdout,
sha256 carried in full and agreeing with what the server indexed, `file`
null for a whole-version delete, and — the half a script depends on — a
failure printing `{"error": ...}` on stdout with exit 1, on both the
server-rejected path and the client-side-rejected one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tests/issue_reopen_test.rs
Old New
@@ -0,0 +1,281 @@
1 //! Issue-side close/reopen convergence across two clones.
2 //!
3 //! `close` moves an object's ref between the active and the archive namespace
4 //! while its id stays the same, so one clone that closed and another that
5 //! reopened publish the *same* object under two different sync prefixes. Sync
6 //! has to recognise those as one object and reconcile them into one DAG;
7 //! reconciling only the active prefix adopts them as two divergent histories
8 //! for one id, which no later sync can reunite. See `64194908` and
9 //! `state::existing_events_ref`.
10 //!
11 //! `tests/patch_reopen_test.rs` covers the patch side of the same code path.
12 //! This is its issue-side twin — issues are the more common object and
13 //! `issue reopen` is much older than `patch reopen`, so this is the side more
14 //! likely to have been hit in practice.
15 //!
16 //! Unlike the patch-side test, both cases below pin *which* side wins rather
17 //! than only asserting the two clones agree. The fold is ordered by
18 //! `(clock, oid)` and a clock is `max_clock(tip) + 1`, so giving the winning
19 //! clone one extra local event before its status event puts its status event a
20 //! clock ahead — no tie, no dependence on how two oids happen to sort.
21
22 mod common;
23
24 use std::process::Command;
25
26 use serde_json::Value;
27 use tempfile::TempDir;
28
29 use common::TestRepo;
30
31 // ---------------------------------------------------------------------------
32 // Harness
33 // ---------------------------------------------------------------------------
34
35 fn repo_with_origin() -> (TestRepo, TempDir) {
36 let bare = TempDir::new().unwrap();
37 let status = Command::new("git")
38 .args(["init", "--bare", "-b", "main"])
39 .arg(bare.path())
40 .status()
41 .unwrap();
42 assert!(status.success());
43
44 let repo = TestRepo::new("Alice", "alice@example.com");
45 repo.git(&["remote", "add", "origin", bare.path().to_str().unwrap()]);
46 repo.git(&["push", "-u", "origin", "main"]);
47 repo.run_ok(&["init"]);
48 (repo, bare)
49 }
50
51 fn git_in(env_from: &TestRepo, dir: &std::path::Path, args: &[&str]) -> String {
52 let mut command = Command::new("git");
53 env_from.apply_env(&mut command);
54 let output = command.args(args).current_dir(dir).output().unwrap();
55 assert!(
56 output.status.success(),
57 "git {:?} failed: {}",
58 args,
59 String::from_utf8_lossy(&output.stderr)
60 );
61 String::from_utf8(output.stdout).unwrap()
62 }
63
64 fn collab_in(env_from: &TestRepo, dir: &std::path::Path, args: &[&str]) -> String {
65 let mut command = Command::new(env!("CARGO_BIN_EXE_git-collab"));
66 env_from.apply_env(&mut command);
67 let output = command.args(args).current_dir(dir).output().unwrap();
68 assert!(
69 output.status.success(),
70 "git-collab {:?} failed:\nstdout: {}\nstderr: {}",
71 args,
72 String::from_utf8_lossy(&output.stdout),
73 String::from_utf8_lossy(&output.stderr)
74 );
75 String::from_utf8(output.stdout).unwrap()
76 }
77
78 /// A second clone of `bare`, configured as Bob and initialised for collab.
79 /// Returns the tempdir keeping it alive and the path to the clone itself.
80 fn second_clone(alice: &TestRepo, bare: &TempDir) -> (TempDir, std::path::PathBuf) {
81 let root = TempDir::new().unwrap();
82 let dir = root.path().join("clone");
83 git_in(
84 alice,
85 root.path(),
86 &["clone", bare.path().to_str().unwrap(), "clone"],
87 );
88 git_in(alice, &dir, &["config", "user.name", "Bob"]);
89 git_in(alice, &dir, &["config", "user.email", "bob@example.com"]);
90 git_in(alice, &dir, &["config", "collab.autoSync", "false"]);
91 collab_in(alice, &dir, &["init"]);
92 collab_in(alice, &dir, &["sync"]);
93 (root, dir)
94 }
95
96 fn show_json(repo: &TestRepo, id: &str) -> Value {
97 serde_json::from_str(&repo.run_ok(&["issue", "show", id, "--json"])).unwrap()
98 }
99
100 fn show_json_in(env_from: &TestRepo, dir: &std::path::Path, id: &str) -> Value {
101 serde_json::from_str(&collab_in(env_from, dir, &["issue", "show", id, "--json"])).unwrap()
102 }
103
104 /// The events ref for `short` in a `for-each-ref` listing.
105 ///
106 /// Each clone is asked for its own. Which namespace a clone files the object
107 /// in is a local decision — only the clone that ran `close` moves the ref, so
108 /// a close that merely *arrives* by sync leaves it where it was — and neither
109 /// filing is wrong. The DAG tip and the folded status are the facts that have
110 /// to match.
111 fn issue_events_ref_in(listing: &str, short: &str) -> String {
112 listing
113 .lines()
114 .find(|r| r.contains(short))
115 .unwrap_or_else(|| panic!("no events ref for {} in {}", short, listing))
116 .to_string()
117 }
118
119 fn issue_events_ref(repo: &TestRepo, short: &str) -> String {
120 let out = repo.git(&["for-each-ref", "--format=%(refname)", "refs/collab/"]);
121 issue_events_ref_in(&out, short)
122 }
123
124 fn issue_events_ref_at(env_from: &TestRepo, dir: &std::path::Path, short: &str) -> String {
125 let out = git_in(
126 env_from,
127 dir,
128 &["for-each-ref", "--format=%(refname)", "refs/collab/"],
129 );
130 issue_events_ref_in(&out, short)
131 }
132
133 /// Assert both clones ended on one DAG — the property the whole fix exists for.
134 /// Divergence is exactly "two tips for one id", so comparing tips is what
135 /// catches it; the folded status can coincide even when the histories have not
136 /// been reunited.
137 fn assert_same_tip(alice: &TestRepo, bob: &std::path::Path, short: &str) {
138 let alice_ref = issue_events_ref(alice, short);
139 let bob_ref = issue_events_ref_at(alice, bob, short);
140 assert_eq!(
141 alice.git(&["rev-parse", &alice_ref]).trim(),
142 git_in(alice, bob, &["rev-parse", &bob_ref]).trim(),
143 "both clones must end at the same DAG tip ({} vs {})",
144 alice_ref,
145 bob_ref
146 );
147 }
148
149 // ---------------------------------------------------------------------------
150 // Convergence: one clone closes, another reopens, neither having seen the other
151 // ---------------------------------------------------------------------------
152
153 #[test]
154 fn a_concurrent_close_and_reopen_converge_on_the_later_close() {
155 // Alice's close is the later event, so `closed` is the answer both clones
156 // owe. Alice is also the clone whose ref moves into the archive namespace,
157 // which is the half that a sync globbing only the active prefix never sees.
158 let (alice, bare) = repo_with_origin();
159 let short = alice.issue_open("A bug");
160 alice.run_ok(&["sync"]);
161
162 let (_bob_root, bob) = second_clone(&alice, &bare);
163
164 // Alice takes an extra clock tick before closing, so her close sits at a
165 // strictly higher clock than Bob's reopen and the winner is not decided by
166 // whichever oid happens to sort higher this run.
167 alice.run_ok(&["issue", "comment", &short, "-b", "cannot reproduce"]);
168 alice.run_ok(&["issue", "close", &short]);
169 collab_in(&alice, &bob, &["issue", "reopen", &short]);
170
171 // Alice publishes first, so Bob's sync is the one that has to reconcile a
172 // genuinely divergent DAG arriving under the *other* namespace.
173 alice.run_ok(&["sync"]);
174 collab_in(&alice, &bob, &["sync"]);
175 alice.run_ok(&["sync"]);
176
177 assert_eq!(
178 show_json(&alice, &short)["status"],
179 "closed",
180 "the later close wins on the clone that wrote it"
181 );
182 assert_eq!(
183 show_json_in(&alice, &bob, &short)["status"],
184 "closed",
185 "and the clone that reopened has to accept it once it has seen it"
186 );
187 assert_same_tip(&alice, &bob, &short);
188 }
189
190 #[test]
191 fn a_concurrent_close_and_reopen_converge_on_the_later_reopen() {
192 // The mirror image, and the direction that also exercises
193 // `unarchive_if_reopened`: Alice filed the issue away under `close`, and
194 // the reconciled DAG says it is open again, so it has to come back out of
195 // her archive or it is open and unfindable on her clone.
196 let (alice, bare) = repo_with_origin();
197 let short = alice.issue_open("A bug");
198 alice.run_ok(&["sync"]);
199
200 let (_bob_root, bob) = second_clone(&alice, &bare);
201
202 alice.run_ok(&["issue", "close", &short]);
203 // Bob's extra tick, so his reopen is the strictly later event.
204 collab_in(&alice, &bob, &["issue", "comment", &short, "-b", "still here"]);
205 collab_in(&alice, &bob, &["issue", "reopen", &short]);
206
207 alice.run_ok(&["sync"]);
208 collab_in(&alice, &bob, &["sync"]);
209 alice.run_ok(&["sync"]);
210
211 assert_eq!(
212 show_json_in(&alice, &bob, &short)["status"],
213 "open",
214 "the later reopen wins on the clone that wrote it"
215 );
216 let alice_view = show_json(&alice, &short);
217 assert_eq!(
218 alice_view["status"], "open",
219 "and the clone that closed has to accept it once it has seen it"
220 );
221 assert!(
222 alice_view["close_reason"].is_null(),
223 "an open issue must not still carry the close it denies: {}",
224 alice_view
225 );
226 assert_same_tip(&alice, &bob, &short);
227
228 // The user-visible half: nothing enumerates the archive namespace, so an
229 // issue reopened elsewhere that stayed filed away would be open and
230 // invisible here.
231 assert!(
232 issue_events_ref(&alice, &short).starts_with("refs/collab/issues/"),
233 "a reopen arriving by sync has to bring the issue back out of the archive, got {}",
234 issue_events_ref(&alice, &short)
235 );
236 let listed = alice.run_ok(&["issue", "list"]);
237 assert!(
238 listed.contains(&short),
239 "and back into the default list: {}",
240 listed
241 );
242 }
243
244 #[test]
245 fn both_clones_keep_every_concurrent_event() {
246 // Convergence on one tip is not enough if the merge dropped events on the
247 // way: the comment each clone wrote while offline has to survive on both,
248 // which is what proves the two histories were reunited rather than one of
249 // them being adopted wholesale.
250 let (alice, bare) = repo_with_origin();
251 let short = alice.issue_open("A bug");
252 alice.run_ok(&["sync"]);
253
254 let (_bob_root, bob) = second_clone(&alice, &bare);
255
256 alice.run_ok(&["issue", "comment", &short, "-b", "from alice"]);
257 alice.run_ok(&["issue", "close", &short]);
258 collab_in(&alice, &bob, &["issue", "comment", &short, "-b", "from bob"]);
259 collab_in(&alice, &bob, &["issue", "reopen", &short]);
260
261 alice.run_ok(&["sync"]);
262 collab_in(&alice, &bob, &["sync"]);
263 alice.run_ok(&["sync"]);
264
265 for view in [
266 show_json(&alice, &short),
267 show_json_in(&alice, &bob, &short),
268 ] {
269 let bodies: Vec<String> = view["comments"]
270 .as_array()
271 .unwrap_or_else(|| panic!("no comments array: {}", view))
272 .iter()
273 .map(|c| c["body"].as_str().unwrap_or("").to_string())
274 .collect();
275 assert!(
276 bodies.iter().any(|b| b == "from alice") && bodies.iter().any(|b| b == "from bob"),
277 "both clones' offline comments must survive the merge, got {:?}",
278 bodies
279 );
280 }
281 }
tests/release_cli_test.rs
Old New
@@ -282,3 +282,291 @@ fn multi_file_publish_reports_partial_failure() {
282 stderr 282 stderr
283 ); 283 );
284 } 284 }
285
286 // ---------------------------------------------------------------------------
287 // `--json` on the two release commands that write
288 // ---------------------------------------------------------------------------
289 //
290 // `release list --json` has been covered since it existed, because it is a
291 // pass-through of the server's own index. `publish` and `delete` build their
292 // object client-side and were never exercised against a live server at all.
293 // Three rules make up the contract, and all three are asserted below:
294 //
295 // - stdout is exactly one JSON value and nothing else (parsing the *whole* of
296 // stdout is the assertion — serde_json rejects trailing content), so the
297 // per-file prose `publish` otherwise prints has to be held back;
298 // - every identifier is the full one. For a release that is the sha256, which
299 // must be the whole 64-character digest of the bytes that landed, never an
300 // abbreviation;
301 // - a failure prints `{"error": ...}` on stdout and exits 1, per e049a2bb — a
302 // caller that asked for JSON never has to read stderr, and that is the half
303 // of the contract a script actually depends on.
304
305 /// Parse the whole of a successful command's stdout as one JSON value.
306 fn json_ok(output: &Output, what: &str) -> serde_json::Value {
307 let stdout = String::from_utf8_lossy(&output.stdout);
308 assert!(
309 output.status.success(),
310 "{} failed: {}{}",
311 what,
312 stdout,
313 String::from_utf8_lossy(&output.stderr)
314 );
315 serde_json::from_str(&stdout).unwrap_or_else(|e| {
316 panic!(
317 "{} did not print exactly one JSON value: {}\nstdout was:\n{}",
318 what, e, stdout
319 )
320 })
321 }
322
323 /// Parse a failed command's stdout as the one error object it owes, and return
324 /// the message it carried.
325 fn json_err(output: &Output, what: &str) -> String {
326 let stdout = String::from_utf8_lossy(&output.stdout);
327 assert!(
328 !output.status.success(),
329 "{} was expected to fail but succeeded: {}",
330 what,
331 stdout
332 );
333 assert_eq!(
334 output.status.code(),
335 Some(1),
336 "{} must exit 1, not {:?}",
337 what,
338 output.status.code()
339 );
340 let json: serde_json::Value = serde_json::from_str(&stdout).unwrap_or_else(|e| {
341 panic!(
342 "{} did not print exactly one JSON value on stdout: {}\nstdout was:\n{}",
343 what, e, stdout
344 )
345 });
346 assert!(
347 json.get("error").is_some(),
348 "{} must report the failure under \"error\": {}",
349 what,
350 json
351 );
352 // A failure reports nothing else: no half-written result a caller could
353 // mistake for success.
354 assert!(
355 json.get("action").is_none() && json.get("files").is_none(),
356 "{} must not report a result alongside the error: {}",
357 what,
358 json
359 );
360 // Still on stderr as well, for the human running the same command by hand.
361 let stderr = String::from_utf8_lossy(&output.stderr);
362 assert!(
363 stderr.contains("error:"),
364 "{} must still say so on stderr: {}",
365 what,
366 stderr
367 );
368 json["error"].as_str().unwrap().to_string()
369 }
370
371 fn sha256_hex(bytes: &[u8]) -> String {
372 use sha2::Digest;
373 sha2::Sha256::digest(bytes)
374 .iter()
375 .map(|b| format!("{:02x}", b))
376 .collect()
377 }
378
379 fn assert_full_sha256(value: &serde_json::Value, expected: &str) {
380 let s = value
381 .as_str()
382 .unwrap_or_else(|| panic!("sha256 is not a string: {}", value));
383 assert_eq!(
384 s.len(),
385 64,
386 "sha256 must be the full digest, got {:?} ({} chars)",
387 s,
388 s.len()
389 );
390 assert!(
391 s.chars().all(|c| c.is_ascii_hexdigit() && !c.is_uppercase()),
392 "sha256 must be lowercase hex, got {:?}",
393 s
394 );
395 assert_eq!(s, expected, "sha256 must be the digest of what was sent");
396 }
397
398 #[test]
399 fn publish_json_is_one_object_naming_every_file_by_full_checksum() {
400 let harness = setup("cli-publish-json");
401 let dir = harness.work_repo().dir.path();
402 let first = dir.join("first.tar.gz");
403 let second = dir.join("second.tar.gz");
404 std::fs::write(&first, b"first release bytes").unwrap();
405 std::fs::write(&second, b"second release bytes").unwrap();
406
407 let output = release_cmd(
408 &harness,
409 &[
410 "publish",
411 "v2.0.0",
412 first.to_str().unwrap(),
413 second.to_str().unwrap(),
414 "--remote",
415 "srv",
416 "--json",
417 ],
418 );
419 let json = json_ok(&output, "release publish --json");
420
421 assert_eq!(json["action"], "release.publish");
422 assert_eq!(json["version"], "v2.0.0");
423 let files = json["files"].as_array().expect("files is not an array");
424 assert_eq!(files.len(), 2, "every file published belongs in the object");
425 assert_eq!(files[0]["name"], "first.tar.gz");
426 assert_full_sha256(&files[0]["sha256"], &sha256_hex(b"first release bytes"));
427 assert_eq!(files[1]["name"], "second.tar.gz");
428 assert_full_sha256(&files[1]["sha256"], &sha256_hex(b"second release bytes"));
429
430 // The prose the non-JSON path prints per file must not be mixed in. The
431 // parse above already proves it, but this says which rule was broken.
432 let stdout = String::from_utf8_lossy(&output.stdout);
433 assert!(
434 !stdout.contains("Published"),
435 "the per-file prose has to be held back under --json: {}",
436 stdout
437 );
438
439 // And the checksums are the ones the server acknowledged, not a hopeful
440 // client-side guess: what `list` reports for the same files has to agree.
441 let index = json_ok(
442 &release_cmd(&harness, &["list", "--json", "--remote", "srv"]),
443 "release list --json",
444 );
445 // Matched by name, not position: `list` sorts and `publish` reports in the
446 // order it was given, and this is a claim about the checksums.
447 let listed = index["versions"][0]["files"].as_array().unwrap().clone();
448 for published in files {
449 let same = listed
450 .iter()
451 .find(|f| f["name"] == published["name"])
452 .unwrap_or_else(|| panic!("{} is not in the index", published["name"]));
453 assert_eq!(same["sha256"], published["sha256"]);
454 }
455 }
456
457 #[test]
458 fn delete_json_distinguishes_one_file_from_the_whole_version() {
459 let harness = setup("cli-delete-json");
460 let dir = harness.work_repo().dir.path();
461 let doomed = dir.join("doomed.tar.gz");
462 let keeper = dir.join("keeper.tar.gz");
463 std::fs::write(&doomed, b"doomed").unwrap();
464 std::fs::write(&keeper, b"keeper").unwrap();
465 assert!(release_cmd(
466 &harness,
467 &[
468 "publish",
469 "v3",
470 doomed.to_str().unwrap(),
471 keeper.to_str().unwrap(),
472 "--remote",
473 "srv",
474 ]
475 )
476 .status
477 .success());
478
479 let one = json_ok(
480 &release_cmd(
481 &harness,
482 &["delete", "v3", "doomed.tar.gz", "--remote", "srv", "--json"],
483 ),
484 "release delete <file> --json",
485 );
486 assert_eq!(one["action"], "release.delete");
487 assert_eq!(one["version"], "v3");
488 assert_eq!(one["file"], "doomed.tar.gz");
489
490 let after = json_ok(
491 &release_cmd(&harness, &["list", "--json", "--remote", "srv"]),
492 "release list --json",
493 );
494 let remaining = after["versions"][0]["files"].as_array().unwrap();
495 assert_eq!(remaining.len(), 1, "only the named file went: {}", after);
496 assert_eq!(remaining[0]["name"], "keeper.tar.gz");
497
498 let whole = json_ok(
499 &release_cmd(&harness, &["delete", "v3", "--remote", "srv", "--json"]),
500 "release delete --json",
501 );
502 assert_eq!(whole["action"], "release.delete");
503 assert_eq!(whole["version"], "v3");
504 assert!(
505 whole["file"].is_null(),
506 "null is how the object says the whole version went, not a file with no name: {}",
507 whole
508 );
509
510 let after = json_ok(
511 &release_cmd(&harness, &["list", "--json", "--remote", "srv"]),
512 "release list --json",
513 );
514 assert_eq!(after["versions"].as_array().unwrap().len(), 0);
515 }
516
517 #[test]
518 fn a_failing_publish_prints_the_error_object_on_stdout() {
519 // The server rejects a duplicate without --force. That error is born on
520 // the far end of an ssh pipe, which is the path most likely to leak onto
521 // stdout as prose or arrive glued to ssh's own chatter.
522 let harness = setup("cli-publish-json-fail");
523 let tarball = harness.work_repo().dir.path().join("dup.tar.gz");
524 std::fs::write(&tarball, b"one").unwrap();
525 let path = tarball.to_str().unwrap();
526
527 assert!(
528 release_cmd(&harness, &["publish", "v1", path, "--remote", "srv"])
529 .status
530 .success()
531 );
532
533 let output = release_cmd(
534 &harness,
535 &["publish", "v1", path, "--remote", "srv", "--json"],
536 );
537 let message = json_err(&output, "a duplicate release publish --json");
538 assert!(
539 message.contains("already exists"),
540 "the server's reason has to survive into the object: {:?}",
541 message
542 );
543 assert!(
544 !message.contains("Permanently added"),
545 "ssh's host-key banner must not be glued into the message: {:?}",
546 message
547 );
548 }
549
550 #[test]
551 fn a_failing_delete_prints_the_error_object_on_stdout() {
552 let harness = setup("cli-delete-json-fail");
553
554 // Rejected by the server: there is no such version to delete.
555 let missing = release_cmd(&harness, &["delete", "v9", "--remote", "srv", "--json"]);
556 let message = json_err(&missing, "deleting a missing release with --json");
557 assert!(
558 message.contains("not found"),
559 "the server's reason has to survive into the object: {:?}",
560 message
561 );
562
563 // Rejected client-side, before any network round trip: the same contract
564 // has to hold on the path that never reaches the server.
565 let bad = release_cmd(&harness, &["delete", "../evil", "--remote", "srv", "--json"]);
566 let message = json_err(&bad, "deleting an invalid version with --json");
567 assert!(
568 message.contains("invalid version"),
569 "expected an invalid-version message, got {:?}",
570 message
571 );
572 }