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