Skip to main content

Severity

Enum Severity 

Source
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§

§

Blocking

§

NonBlocking

§

Nit

Implementations§

Source§

impl Severity

Source

pub fn as_str(self) -> &'static str

Source

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}
Source

pub fn valid_values() -> String

Source§

impl Severity

Source

pub fn rank(self) -> u8

How badly it matters, independent of the order the variants happen to be declared in. Relying on derived Ord here would silently invert the moment somebody reorders the enum.

Source

pub fn graver(self, other: Self) -> Self

The graver of two judgements.

Two reviewers disagreeing about severity is resolved upward on purpose: advice that under-reports a real defect is worse than advice that over-reports a small one.

Trait Implementations§

Source§

impl Clone for Severity

Source§

fn clone(&self) -> Self

Returns a duplicate of the value. Read more
1.0.0 (const: unstable) · Source§

fn clone_from(&mut self, source: &Self)

Performs copy-assignment from source. Read more
Source§

impl Copy for Severity

Source§

impl Debug for Severity

Source§

fn fmt(&self, f: &mut Formatter<'_>) -> Result

Formats the value using the given formatter. Read more
Source§

impl<'de> Deserialize<'de> for Severity

Source§

fn deserialize<D: Deserializer<'de>>(d: D) -> Result<Self, D::Error>

Deserialize this value from the given Serde deserializer. Read more
Source§

impl Display for Severity

Source§

fn fmt(&self, f: &mut Formatter<'_>) -> Result

Formats the value using the given formatter. Read more
Source§

impl Eq for Severity

Source§

impl Hash for Severity

Source§

fn hash<__H: Hasher>(&self, state: &mut __H)

Feeds this value into the given Hasher. Read more
1.3.0 · Source§

fn hash_slice<H>(data: &[Self], state: &mut H)
where H: Hasher, Self: Sized,

Feeds a slice of this type into the given Hasher. Read more
Source§

impl Ord for Severity

Source§

fn cmp(&self, other: &Self) -> Ordering

This method returns an Ordering between self and other. Read more
1.21.0 (const: unstable) · Source§

fn max(self, other: Self) -> Self
where Self: Sized,

Compares and returns the maximum of two values. Read more
1.21.0 (const: unstable) · Source§

fn min(self, other: Self) -> Self
where Self: Sized,

Compares and returns the minimum of two values. Read more
1.50.0 (const: unstable) · Source§

fn clamp(self, min: Self, max: Self) -> Self
where Self: Sized,

Restrict a value to a certain interval. Read more
Source§

fn clamp_to<R>(self, range: R) -> Self
where Self: Sized, R: ClampBounds<Self>,

🔬This is a nightly-only experimental API. (clamp_to)
Restrict a value to a certain range. Read more
Source§

impl PartialEq for Severity

Source§

fn eq(&self, other: &Self) -> bool

Equality operator ==. Read more
1.0.0 (const: unstable) · Source§

fn ne(&self, other: &Rhs) -> bool

Inequality operator !=. Read more
Source§

impl PartialOrd for Severity

Source§

fn partial_cmp(&self, other: &Self) -> Option<Ordering>

This method returns an ordering between self and other values if one exists. Read more
1.0.0 (const: unstable) · Source§

fn lt(&self, other: &Rhs) -> bool

Tests less than (for self and other) and is used by the < operator. Read more
1.0.0 (const: unstable) · Source§

fn le(&self, other: &Rhs) -> bool

Tests less than or equal to (for self and other) and is used by the <= operator. Read more
1.0.0 (const: unstable) · Source§

fn gt(&self, other: &Rhs) -> bool

Tests greater than (for self and other) and is used by the > operator. Read more
1.0.0 (const: unstable) · Source§

fn ge(&self, other: &Rhs) -> bool

Tests greater than or equal to (for self and other) and is used by the >= operator. Read more
Source§

impl Serialize for Severity

Source§

fn serialize<S: Serializer>(&self, s: S) -> Result<S::Ok, S::Error>

Serialize this value into the given Serde serializer. Read more
Source§

impl StructuralPartialEq for Severity

Auto Trait Implementations§

Blanket Implementations§

Source§

impl<T> Any for T
where T: 'static + ?Sized,

Source§

fn type_id(&self) -> TypeId

Gets the TypeId of self. Read more
Source§

impl<T> Borrow<T> for T
where T: ?Sized,

Source§

fn borrow(&self) -> &T

Immutably borrows from an owned value. Read more
Source§

impl<T> BorrowMut<T> for T
where T: ?Sized,

Source§

fn borrow_mut(&mut self) -> &mut T

Mutably borrows from an owned value. Read more
Source§

impl<T> CloneToUninit for T
where T: Clone,

Source§

unsafe fn clone_to_uninit(&self, dest: *mut u8)

🔬This is a nightly-only experimental API. (clone_to_uninit)
Performs copy-assignment from self to dest. Read more
Source§

impl<Q, K> Comparable<K> for Q
where Q: Ord + ?Sized, K: Borrow<Q> + ?Sized,

Source§

fn compare(&self, key: &K) -> Ordering

Compare self to key and return their ordering.
Source§

impl<T> DeserializeOwned for T
where T: for<'de> Deserialize<'de>,

Source§

impl<Q, K> Equivalent<K> for Q
where Q: Eq + ?Sized, K: Borrow<Q> + ?Sized,

Source§

fn equivalent(&self, key: &K) -> bool

Checks if this value is equivalent to the given key. Read more
Source§

impl<Q, K> Equivalent<K> for Q
where Q: Eq + ?Sized, K: Borrow<Q> + ?Sized,

Source§

fn equivalent(&self, key: &K) -> bool

Compare self to key and return true if they are equal.
Source§

impl<T> From<T> for T

Source§

fn from(t: T) -> T

Returns the argument unchanged.

Source§

impl<T, U> Into<U> for T
where U: From<T>,

Source§

fn into(self) -> U

Calls U::from(self).

That is, this conversion is whatever the implementation of From<T> for U chooses to do.

Source§

impl<T> Same for T

Source§

type Output = T

Should always be Self
Source§

impl<T> ToOwned for T
where T: Clone,

Source§

type Owned = T

The resulting type after obtaining ownership.
Source§

fn to_owned(&self) -> T

Creates owned data from borrowed data, usually by cloning. Read more
Source§

fn clone_into(&self, target: &mut T)

Uses borrowed data to replace owned data, usually by cloning. Read more
Source§

impl<T> ToString for T
where T: Display + ?Sized,

Source§

fn to_string(&self) -> String

Converts the given value to a String. Read more
Source§

impl<T, U> TryFrom<U> for T
where U: Into<T>,

Source§

type Error = !

The type returned in the event of a conversion error.
Source§

fn try_from(value: U) -> Result<T, !>

Performs the conversion.
Source§

impl<T, U> TryInto<U> for T
where U: TryFrom<T>,

Source§

type Error = <U as TryFrom<T>>::Error

The type returned in the event of a conversion error.
Source§

fn try_into(self) -> Result<U, <U as TryFrom<T>>::Error>

Performs the conversion.