diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 9389dc014..bd4fb9695 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -184,9 +184,10 @@ the readiness dashboard and the PR body, and never count it as a pass. heading with the value on the next line. A last verdict that is not an approval (`ready`, `LGTM`, `approve`, `ship it`, `pass`, `clean`, `good to go`), such as `Can't verify`, `blocked`, `revise` or an empty one, or an - approval that says something was excluded, blocked, skipped, not run or - could not be verified, is [`unverified`](#outside-review-unverified-verdict), - never `clean`. + approval that says something was excluded, blocked, skipped, not run, not + executed or could not be verified (or "not yet", or a question such as + `Ready? No`), is [`unverified`](#outside-review-unverified-verdict), never + `clean`. Inline code inside a verdict line is read as text. - **Numbered findings.** In a reply with a verdict line, every numbered item at the start of a line (`1. x`, `1. **Nonblocking — x**`) is a finding, unless the reply concludes that it found nothing. When one has no readable diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index cb906c5a8..99e7d1636 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -202,8 +202,8 @@ function plainReview(text: string): string { * an opener closes only when a later marker of its kind is at least as long. */ const CODE_LABEL_BEFORE = /^[\t ]*(?:>[\t ]*)?(?:#{1,6}[\t ]+|[-+*][\t ]+|\(?\d{1,3}[.)][\t ]+)?(?:\*\*|__)?$|\b(?:severity|priority)[\t ]*[:=][\t ]*(?:\*\*|__)?$|\|[\t ]*(?:\*\*)?$/i; -/** A verdict label before an inline span: `Verdict: \`ready\`` keeps its value. */ -const VERDICT_BEFORE = /^[\t ]*(?:>[\t ]*)*(?:#{1,6}[\t ]+|[-+*][\t ]+)?(?:\*\*|__)?(?:(?:overall|final)[\t ]+)?verdict(?:\*\*|__)?[\t ]*(?::|\u2014|\u2013|-)?[\t ]*(?:\*\*|__)?[\t ]*$/i; +/** A verdict line keeps every inline span: `Verdict: \`ready\` (tests \`not run\`)` is read whole. */ +const VERDICT_LINE = /^[\t ]*(?:>[\t ]*)*(?:#{1,6}[\t ]+|[-+*][\t ]+)?(?:\*\*|__)?(?:(?:overall|final)[\t ]+)?verdict\b/i; function withoutCode(text: string, readUnclosed = false): string { const lines = text.split(/\r?\n/); const markers = lines.map(line => /^[\t ]*(`{3,}|~{3,})/.exec(line)?.[1]); @@ -227,7 +227,7 @@ function withoutCode(text: string, readUnclosed = false): string { if (marker) { if (closes[i]) fence = marker; return ''; } return line.replace(/`([^`]+)`/g, (span, inner: string, at: number) => inner.trim() === 'NO_FINDINGS' || /^\[?(?:P[0-3]|critical|high|medium|low)\]?$/i.test(inner.trim()) && CODE_LABEL_BEFORE.test(line.slice(0, at)) - || VERDICT_BEFORE.test(line.slice(0, at)) ? inner : ' '); + || VERDICT_LINE.test(line) ? inner : ' '); }).join('\n'); } @@ -269,9 +269,9 @@ function lastVerdict(plain: string): string | undefined { return undefined; } /** A verdict that approves the change. */ -const READY_VERDICT = /^(?:ready|lgtm|approved?|ship(?:[\t ]+it)?|pass(?:ed)?|clean|good to (?:go|merge|ship))\b/i; +const READY_VERDICT = /^(?:ready|lgtm|approved?|ship(?:[\t ]+it)?|pass(?:ed)?|clean|good to (?:go|merge|ship))\b(?![-?/])/i; /** A verdict that says the reviewer could not check, run or see something is never a ready one (#3925). */ -const UNVERIFIED_CLAIM = /\b(?:can['\u2019]?t|cannot|can not|couldn['\u2019]t|could not|unable to|not able to|did not|didn['\u2019]t)\s+(?:be\s+)?(?:verif|run|test|check|open|read|review)|\bunverifi|\bnot (?:been )?(?:verified|run|tested|checked|reviewed)\b|\b(?:excluded|blocked|skipped)\b/i; +const UNVERIFIED_CLAIM = /\b(?:can['\u2019]?t|cannot|can not|couldn['\u2019]t|could not|unable to|not able to|did not|didn['\u2019]t)\s+(?:be\s+)?(?:verif|run|test|check|open|read|review|execut)|\bunverifi|\bnot (?:yet|until)\b|\bnot (?:been )?(?:verified|run|tested|checked|reviewed|executed)\b|\b(?:excluded|blocked|skipped)\b/i; /** A numbered item at the start of a line; in a reply with a `Verdict:` line it is a finding (#3925). */ const NUMBERED_ITEM = /^(?:\*\*|__)?\(?\d{1,3}[.)][\t ]+\S.*$/gm; diff --git a/test/outside-review-unparsed-severity.test.ts b/test/outside-review-unparsed-severity.test.ts index 024c9cab3..589adf6e4 100644 --- a/test/outside-review-unparsed-severity.test.ts +++ b/test/outside-review-unparsed-severity.test.ts @@ -226,6 +226,8 @@ describe('#3925 an unrecognized verdict is not clean; numbered findings never pr 'Verdict: ready, but I could not verify the migration', 'Verdict: ready (unverified)', "> Verdict: Can't verify (suites blocked)", "## Verdict\nCan't verify (one changed file excluded)", "Overall verdict: can't verify", "Verdict \u2014 can't verify", 'Verdict: ready, though one changed file was excluded', 'Verdict: ready (tests not run, sandbox blocked)', + 'Verdict: `ready` (tests `not run`)', 'Verdict: Ready? Not until the migration is fixed.', 'Verdict: clean-up required before merge', + 'Verdict: ready, but I did not execute the suites', 'Verdict: ready (tests not executed)', ]) expect({ gate, verdictLine, ...see(`No issues found.\n${verdictLine}`, gate) }).toEqual({ gate, verdictLine, verdict: 'unverified', reason: 'unverified_verdict', unlabeled: 0 }); });