pub enum Severity {
Blocking,
NonBlocking,
Nit,
}Expand description
Only Blocking gates a merge. This is the whole defence against the
nitpick spiral: a competent reviewer can always find something, so
“no objections remaining” is not a stopping condition but “no blocking
objections” is.
Variants§
Implementations§
Source§impl Severity
impl Severity
pub fn as_str(self) -> &'static str
Sourcepub fn parse_lenient(text: &str) -> Option<Self>
pub fn parse_lenient(text: &str) -> Option<Self>
Accept the canonical spelling, any listed alias, and any difference in case, spacing, hyphens, or underscores.
Examples found in repository?
examples/preview.rs (line 26)
24fn finding(severity: &str, title: &str, detail: &str, file: &str, in_scope: bool) -> Finding {
25 Finding {
26 severity: Severity::parse_lenient(severity).expect("severity"),
27 title: title.into(),
28 detail: detail.into(),
29 file: file.into(),
30 in_scope,
31 ..Default::default()
32 }
33}
34
35fn rule(label: &str) {
36 println!("\n\x1b[1m{label}\x1b[0m\n{}", "-".repeat(72));
37}
38
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}pub fn valid_values() -> String
Trait Implementations§
impl Copy for Severity
Source§impl<'de> Deserialize<'de> for Severity
impl<'de> Deserialize<'de> for Severity
Source§fn deserialize<D: Deserializer<'de>>(d: D) -> Result<Self, D::Error>
fn deserialize<D: Deserializer<'de>>(d: D) -> Result<Self, D::Error>
Deserialize this value from the given Serde deserializer. Read more
impl Eq for Severity
Source§impl Ord for Severity
impl Ord for Severity
1.21.0 (const: unstable) · Source§fn max(self, other: Self) -> Selfwhere
Self: Sized,
fn max(self, other: Self) -> Selfwhere
Self: Sized,
Compares and returns the maximum of two values. Read more
1.21.0 (const: unstable) · Source§fn min(self, other: Self) -> Selfwhere
Self: Sized,
fn min(self, other: Self) -> Selfwhere
Self: Sized,
Compares and returns the minimum of two values. Read more
Source§impl PartialOrd for Severity
impl PartialOrd for Severity
impl StructuralPartialEq for Severity
Auto Trait Implementations§
impl Freeze for Severity
impl RefUnwindSafe for Severity
impl Send for Severity
impl Sync for Severity
impl Unpin for Severity
impl UnsafeUnpin for Severity
impl UnwindSafe for Severity
Blanket Implementations§
Source§impl<T> BorrowMut<T> for Twhere
T: ?Sized,
impl<T> BorrowMut<T> for Twhere
T: ?Sized,
Source§fn borrow_mut(&mut self) -> &mut T
fn borrow_mut(&mut self) -> &mut T
Mutably borrows from an owned value. Read more
Source§impl<T> CloneToUninit for Twhere
T: Clone,
impl<T> CloneToUninit for Twhere
T: Clone,
Source§impl<Q, K> Comparable<K> for Q
impl<Q, K> Comparable<K> for Q
impl<T> DeserializeOwned for Twhere
T: for<'de> Deserialize<'de>,
Source§impl<Q, K> Equivalent<K> for Q
impl<Q, K> Equivalent<K> for Q
Source§impl<Q, K> Equivalent<K> for Q
impl<Q, K> Equivalent<K> for Q
Source§fn equivalent(&self, key: &K) -> bool
fn equivalent(&self, key: &K) -> bool
Compare self to
key and return true if they are equal.