pub fn checkin_comment(items: &[Settled], style: &Style) -> Option<String>Expand description
The one comment that answers everything with no thread of its own, and says what changed.
One comment and not one per point: there is nowhere to thread these, so five
replies to five comments is spar talking to itself down the page, while one
comment naming each of them is the thing somebody reads. A comment that
asked for nothing is not named. Returns None when there is nothing to say,
on the same principle as outcome_comment.
Examples found in repository?
examples/preview.rs (line 478)
39fn main() {
40 let loose = std::env::args().any(|a| a == "--loose");
41 let style = if loose {
42 Style {
43 terse: false,
44 ..Style::default()
45 }
46 } else {
47 Style::default()
48 };
49 if loose {
50 println!("(concision gate OFF: this is what a model would post unedited)");
51 }
52
53 rule("A clean review");
54 println!(
55 "{}",
56 review_comment(
57 "codex",
58 1,
59 &Review {
60 verdict: Verdict::Approve,
61 next_action: NextAction::Merge,
62 summary: "I reviewed the changes on this branch carefully and I am happy to \
63 report that the retry path is correct, the backoff calculation is \
64 sound, and the new test covers the 429 case that the issue described. \
65 I have no objections to this change landing as it stands."
66 .into(),
67 findings: vec![],
68 },
69 &style
70 )
71 );
72
73 rule("A review with real work in it");
74 println!(
75 "{}",
76 review_comment(
77 "codex",
78 2,
79 &Review {
80 verdict: Verdict::ChangesRequested,
81 next_action: NextAction::HandBack,
82 summary: "There is one genuine defect here that should block, along with a \
83 couple of improvements that I do not think need to gate this \
84 particular pull request, and one pre-existing problem I noticed \
85 while reading the surrounding code."
86 .into(),
87 findings: vec![
88 finding(
89 "blocking",
90 "Retry loop never terminates when max_attempts is unset",
91 "I confirmed this by running the 429 test with max_attempts left at its \
92 default of None: the loop spins forever because the guard on line 91 \
93 compares against Some(0) rather than checking for None first. This is \
94 not a theoretical concern, the test hangs and I had to kill it.",
95 "src/net.rs:88",
96 true,
97 ),
98 finding(
99 "non-blocking",
100 "The request timeout is hard coded to thirty seconds",
101 "It would be better if this were configurable, since a slow upstream \
102 will now fail rather than wait, but the previous code had the same \
103 limitation so this is not a regression introduced by the change.",
104 "src/net.rs:44",
105 true,
106 ),
107 finding(
108 "nit",
109 "Log line says \"retrying\" without saying how many attempts remain",
110 "Purely a readability point for whoever is reading the logs at 3am.",
111 "src/net.rs:102",
112 true,
113 ),
114 finding(
115 "blocking",
116 "Config loader swallows a parse error",
117 "Unrelated to this PR, but load_config discards the error from serde and \
118 returns Default::default(), so a typo in the config file is silently \
119 ignored.",
120 "src/config.rs:210",
121 false,
122 ),
123 ],
124 },
125 &style
126 )
127 );
128
129 rule("Answering that review");
130 println!(
131 "{}",
132 disposition_comment(
133 "claude",
134 &ResponseDoc {
135 summary: "One of the two blocking points was right and I have fixed it. I do not \
136 agree with the other and have explained why below rather than changing \
137 working code to make the review go away."
138 .into(),
139 dispositions: vec![],
140 },
141 &["Retry loop never terminates when max_attempts is unset".to_string()],
142 &[
143 "Config loader swallows a parse error. The caller already validates the file \
144 against the schema before load_config is reached, so the discarded error is \
145 unreachable in practice."
146 .to_string()
147 ],
148 &["https://github.com/you/thing/issues/512".to_string()],
149 &style
150 )
151 .unwrap_or_default()
152 );
153
154 rule("The pull request body");
155 println!(
156 "{}",
157 pr_body(
158 478,
159 &Implementation {
160 summary: "Retry a 429 with exponential backoff instead of failing the request."
161 .into(),
162 problem: "A rate limited response was treated as fatal, so a single throttled \
163 call ended a run that had hours of work left in it. The retry path \
164 existed but only covered connection errors, and nothing in the logs \
165 said which of the two had happened."
166 .into(),
167 changes: vec![
168 "`send` now retries a 429, honouring `Retry-After` when the server sets it \
169 and backing off exponentially when it does not"
170 .into(),
171 "the retry budget is bounded at five attempts, so a permanent 429 still \
172 ends the call rather than spinning"
173 .into(),
174 "a retry logs the status it is retrying, which is what made the original \
175 failure impossible to tell apart from a dropped connection"
176 .into(),
177 ],
178 testing: vec![
179 "`cargo test retries_a_rate_limited_request`, which fakes a 429 with a \
180 `Retry-After` of 2 and asserts the wait"
181 .into(),
182 "point it at a throttled endpoint and watch a run finish rather than stop \
183 on the first 429"
184 .into(),
185 ],
186 notes: Some(
187 "Streaming calls do not go through `send` and are unchanged, which is worth \
188 a follow-up but not this one."
189 .into()
190 ),
191 ..Implementation::default()
192 },
193 Some("cargo test"),
194 &style
195 )
196 );
197
198 rule("What a whole run leaves on the PR (the default, one comment)");
199 let mut ended = IssueRun::new(482, "t");
200 ended.disputes = vec![Dispute {
201 title: "Config loader swallows a parse error".into(),
202 file: "src/config.rs".into(),
203 reasoning: "the caller validates against the schema before load_config is reached".into(),
204 }];
205 ended.filed = vec![
206 "https://github.com/you/thing/issues/485".into(),
207 "https://github.com/you/thing/issues/486".into(),
208 ];
209 println!(
210 "{}",
211 outcome_comment(
212 &ended,
213 &spar::model::Ledger::new(),
214 &Ending::OutOfRounds,
215 &style
216 )
217 .unwrap_or_default()
218 );
219 println!("\n (and a clean run that filed nothing posts no comment at all)");
220
221 rule("What a run leaves when the closing pass did not sign it off");
222 let mut left = IssueRun::new(482, "t");
223 left.noted = vec![finding(
224 "non-blocking",
225 "Timeout is not configurable",
226 "The retry budget is fixed at three.",
227 "src/net.rs",
228 true,
229 )];
230 let unresolved = vec![finding(
231 "blocking",
232 "The retry fix never reaches the 429 path",
233 "The guard added in round 2 sits after the early return on line 88, so a rate limited \
234 response still takes the old path. Reproduced with the 429 test.",
235 "src/net.rs:88",
236 true,
237 )];
238 println!(
239 "{}",
240 outcome_comment(
241 &left,
242 &spar::model::Ledger::new(),
243 &Ending::Unresolved(&unresolved),
244 &style
245 )
246 .unwrap_or_default()
247 );
248
249 rule("A review of somebody else's pull request (spar review)");
250 let judged = |standing, severity, title: &str, detail: &str, file: &str, by: &str| Judged {
251 finding: finding(severity, title, detail, file, true),
252 raised_by: by.to_string(),
253 standing,
254 counterpoint: None,
255 defence: None,
256 };
257 let mut disputed = judged(
258 FindingStanding::Disputed,
259 "blocking",
260 "Config loader swallows a parse error",
261 "load_config discards the error from serde and returns a default.",
262 "src/config.rs:210",
263 "claude",
264 );
265 disputed.counterpoint =
266 Some("the caller validates against the schema before load_config is reached".into());
267 println!(
268 "{}",
269 verdict_comment(
270 &[
271 judged(
272 FindingStanding::Corroborated,
273 "blocking",
274 "Retry loop never terminates when max_attempts is unset",
275 "Both reviewers reproduced this: the guard on line 91 compares against \
276 Some(0) rather than checking for None, so the 429 test hangs.",
277 "src/net.rs:88",
278 "claude and codex",
279 ),
280 judged(
281 FindingStanding::Confirmed,
282 "non-blocking",
283 "The request timeout is hard coded",
284 "Not a regression, the previous code had the same limitation.",
285 "src/net.rs:44",
286 "codex",
287 ),
288 judged(
289 FindingStanding::Unverified,
290 "nit",
291 "Log line does not say how many attempts remain",
292 "Readability for whoever reads the logs at 3am.",
293 "src/net.rs:102",
294 "claude",
295 ),
296 disputed,
297 judged(
298 FindingStanding::Withdrawn,
299 "blocking",
300 "Off by one in the backoff",
301 "Withdrawn after the other reviewer pointed at the test that covers it.",
302 "src/net.rs:70",
303 "codex",
304 ),
305 ],
306 &style
307 )
308 );
309
310 rule("A second look at that pull request, after the author pushed (spar review --recheck)");
311 let recorded = |severity: &str, title: &str, detail: &str, file: &str, standing| {
312 spar::model::RecordedFinding {
313 title: title.into(),
314 file: file.into(),
315 severity: Severity::parse_lenient(severity).expect("severity"),
316 detail: detail.into(),
317 in_scope: true,
318 raised_by: "claude and codex".into(),
319 standing,
320 counterpoint: None,
321 defence: None,
322 }
323 };
324 println!(
325 "{}",
326 recheck_comment(
327 &Delta {
328 from: "0123abcdef".into(),
329 commits: vec![
330 "a1b2c3d check for None before comparing attempts".into(),
331 "e4f5a6b make the request timeout configurable".into(),
332 ],
333 rewritten: false,
334 },
335 &[
336 Rechecked {
337 recorded: recorded(
338 "blocking",
339 "Retry loop never terminates when max_attempts is unset",
340 "The guard on line 91 compares against Some(0) rather than checking for None.",
341 "src/net.rs:88",
342 FindingStanding::Corroborated,
343 ),
344 resolution: Resolution::Fixed(
345 "src/net.rs:91 now matches on None first, and the 429 test that used to \
346 hang passes in 0.3s when run with cargo test retry"
347 .into(),
348 ),
349 },
350 Rechecked {
351 recorded: recorded(
352 "non-blocking",
353 "The request timeout is hard coded",
354 "Not a regression, the previous code had the same limitation.",
355 "src/net.rs:44",
356 FindingStanding::Confirmed,
357 ),
358 resolution: Resolution::Rebutted(
359 "the author's reply is right that the timeout comes from the caller's \
360 Client builder since e4f5a6b, so there is nothing left to make configurable \
361 here"
362 .into(),
363 ),
364 },
365 Rechecked {
366 recorded: recorded(
367 "blocking",
368 "Config loader swallows a parse error",
369 "load_config discards the error from serde and returns a default.",
370 "src/config.rs:210",
371 FindingStanding::Confirmed,
372 ),
373 resolution: Resolution::Open(
374 "codex still sees it: load_config still returns Default on a parse error, \
375 and nothing upstream validates the file first"
376 .into(),
377 ),
378 },
379 ],
380 &[judged(
381 FindingStanding::Corroborated,
382 "non-blocking",
383 "The new None branch logs at error level on every retry",
384 "Each retry now writes an error line, so a flaky upstream fills the log with \
385 errors for a request that succeeds on the third try.",
386 "src/net.rs:93",
387 "claude and codex",
388 )],
389 &style,
390 2
391 )
392 );
393
394 rule("An issue both reviewers declined");
395 println!(
396 "{}",
397 skip_comment(
398 &SkippedItem {
399 issue: 91,
400 title: "Add a dark mode".into(),
401 tracker: false,
402 reasons: [
403 (
404 "claude".to_string(),
405 "This was already implemented in 1.4 and shipped behind the theme \
406 setting, so there is nothing left to do here."
407 .to_string()
408 ),
409 (
410 "codex".to_string(),
411 "Duplicate of #62, which is still open and has the full discussion."
412 .to_string()
413 ),
414 ]
415 .into_iter()
416 .collect(),
417 },
418 &style
419 )
420 );
421 rule("Answering a comment in its own thread");
422 for item in [
423 settled(
424 Ask::Implement,
425 "alice",
426 "src/retry.rs:91",
427 "Added the guard on the retry path and a test that reproduces the original failure.",
428 "",
429 true,
430 ),
431 settled(
432 Ask::Decline,
433 "bob",
434 "src/pool.rs:14",
435 "",
436 "Every caller of this function already holds the pool lock, so the check on line 14 \
437 cannot be reached with a null connection. I ran the suite with an assertion in its \
438 place and nothing tripped it.",
439 false,
440 ),
441 settled(
442 Ask::Defer,
443 "carol",
444 "src/electrum.rs:203",
445 "",
446 "This is real and it predates the branch: the reconnect monitor puts a stopped wallet \
447 back into the header router. It is not caused by anything here, so fixing it in this \
448 pull request would put an unrelated change in front of whoever reviews it.",
449 false,
450 ),
451 ] {
452 println!("{}\n", thread_reply(&item, &style));
453 }
454
455 rule("What a check-in leaves on the pull request");
456 let mut fixed = settled(
457 Ask::Implement,
458 "alice",
459 "the pull request",
460 "Retried the 429 with the backoff the issue asked for.",
461 "",
462 true,
463 );
464 fixed.pending.file = None;
465 let mut parked = settled(
466 Ask::Decline,
467 "dave",
468 "the pull request",
469 "",
470 "The two reviewers read the same code and did not agree about whether the guard is \
471 reachable.",
472 false,
473 );
474 parked.pending.file = None;
475 parked.parked = true;
476 println!(
477 "{}",
478 checkin_comment(&[fixed, parked], &style)
479 .unwrap_or_else(|| "(nothing to say, so no comment is posted)".into())
480 );
481 println!("\n (and a check-in with nothing outstanding posts no comment at all)");
482
483 rule("An idea filed from a brainstorm");
484 let idea = Candidate {
485 id: 1,
486 idea: Idea {
487 title: "Batch settlement with hash timelocks".into(),
488 combines: vec![
489 "hash time locked contracts".into(),
490 "batch auctions".into(),
491 "the OP_CHECKSEQUENCEVERIFY delay".into(),
492 ],
493 how_it_works: "Each trade posts a hash timelock, and one batch auction clears every \
494 one of them at the deadline together, so the batch is the settlement."
495 .into(),
496 why_new: "The nearest thing is a plain escrow, which settles one trade at a time \
497 and cannot net them against each other."
498 .into(),
499 enables: "Settling many trades in one transaction.".into(),
500 risks: "A fee spike at the deadline makes the batch uneconomic.".into(),
501 first_experiment: "Simulate ten trades on regtest and count the bytes.".into(),
502 },
503 proposers: vec!["claude".into()],
504 parent: None,
505 objection: Some((
506 "codex".into(),
507 "Coinjoin coordinators already batch settlement.".into(),
508 )),
509 defence: Some(
510 "A coordinator batches inputs, not conditional trades; nothing there clears a hash \
511 lock."
512 .into(),
513 ),
514 standing: Standing::Defended,
515 rank_sum: 3,
516 };
517 println!("{}", issue_body(&idea, &style));
518 println!("\n (who proposed and who objected stays in the session file, never in the issue)");
519
520 println!();
521}