696f5424e3ce2c3ec88589d0836a3acc9b894d8b lrnassar Mon Aug 31 12:14:05 2026 -0700 Stop the daily code review emailing approved digests whose OVERALL STATUS line the model omitted. refs #38020 Since the 38020 fix landed, the model dropped the OVERALL STATUS block in about one digest in six, nearly always alongside the optional sections the prompt invites it to omit, and the fail-open email decision mailed every one of those as a possible FEEDBACK even when every per-commit verdict said APPROVED. Two changes. The prompt now says OVERALL STATUS is not one of the omittable sections and a review without it gets emailed. And when the status line is still unreadable, the per-commit Verdict lines now decide instead of sending unconditionally - but only on a full accounting: exactly one readable verdict per commit reviewed and every one APPROVED. A missing or extra verdict, or any FEEDBACK, still sends, so a real review can still never be dropped. Replayed against all 129 digests since 6 Aug: 17 of the 21 phantom emails stop, the 4 that carried real per-commit FEEDBACK still send, and nothing that skipped before sends now. diff --git src/utils/codeReviewAi.py src/utils/codeReviewAi.py index c6fd9879cdf..ae1af213db6 100755 --- src/utils/codeReviewAi.py +++ src/utils/codeReviewAi.py @@ -1520,30 +1520,31 @@ [Summary. If FEEDBACK, list all items that need attention.] --- Automated daily code review | {datetime.now().strftime('%Y-%m-%d')} | {len(commits)} commits ``` IMPORTANT: - Be thorough - check every commit, read every diff - Give FEEDBACK only for real issues that need attention - Be constructive - this email goes directly to the author OUTPUT REQUIREMENTS: - Output the COMPLETE review in the plain text email format shown above - Start with "DAILY CODE REVIEW" - no preamble - Include ALL sections, but omit CROSS-COMMIT OBSERVATIONS and RISK ASSESSMENT if they would only contain boilerplate (e.g., "only one commit", "all low risk with no concerns") +- OVERALL STATUS is NOT omittable: end every review with the OVERALL STATUS line, even for a single commit whose Verdict says the same thing. A review without it is treated as possible FEEDBACK and emailed. BEGIN YOUR REVIEW NOW. Use your tools to investigate thoroughly. """ return prompt def build_daily_commit_prompt(author_name, commit, index, total): """Build a prompt for reviewing ONE commit during a split review. Asks for a single commit block and nothing else. The digest header, summary, overall status and footer are assembled locally in assemble_split_digest(), so the model must not emit them here - otherwise stitching the blocks together would produce an email with repeated headers and several conflicting status lines.""" refs = ', '.join('#' + r for r in commit['referenced_issues']) or 'None' @@ -1665,49 +1666,55 @@ def is_envelope_line(line): """True if this line is one of the digest's own envelope lines rather than review text that happens to mention the same words.""" normalized = normalize_marker_line(line) if any(pattern.match(normalized) for pattern in VALUE_ENVELOPE_RES): return True # The bare patterns additionally require column 0. The assembler writes its headings # flush left, while an indented bare 'SUMMARY' is far more likely a line inside a prose # list. An indented heading from the model therefore survives - cosmetic, and the safe # direction, since the alternative is deleting somebody's finding. if line[:1].isspace(): return False undecorated = undecorate_line(line) return any(pattern.match(undecorated) for pattern in BARE_ENVELOPE_RES) -def verdict_words(text): - """Every decision a 'Verdict:' line in text states, as a set of APPROVED/FEEDBACK. - - Collects rather than returning the first, because a block can contain more than one - such line - a quoted diff hunk, or the template quoted back - and reading whichever - came first let a quoted "+Verdict: APPROVED" override the real "Verdict: FEEDBACK" - below it. Placeholder lines that name both words state nothing and are skipped.""" - words = set() +def verdict_lines(text): + """The decision of every readable 'Verdict:' line in text, in order, one entry per + line. A list rather than a set because digest_wants_email() checks the count + against the number of commits reviewed, not just which decisions appear.""" + decisions = [] for raw in text.splitlines(): match = re.match(VERDICT_LINE_RE, normalize_marker_line(raw), re.IGNORECASE) if not match: continue # The first word of the value decides this line. Testing "FEEDBACK in line" # instead read "Verdict: APPROVED (no feedback required)" as FEEDBACK, which mailed # the author a "needs attention" digest over a review that approved their commit. word = verdict_value(match.group(1)) if word: - words.add(word) - return words + decisions.append(word) + return decisions + +def verdict_words(text): + """Every decision a 'Verdict:' line in text states, as a set of APPROVED/FEEDBACK. + + Collects rather than returning the first, because a block can contain more than one + such line - a quoted diff hunk, or the template quoted back - and reading whichever + came first let a quoted "+Verdict: APPROVED" override the real "Verdict: FEEDBACK" + below it. Placeholder lines that name both words state nothing and are skipped.""" + return set(verdict_lines(text)) def read_verdict(text): """The verdict a block states, or None when it states nothing or contradicts itself. None is not a silent failure: callers route it to the "did not state a clear verdict" path, which forces the digest to FEEDBACK so a human still looks.""" words = verdict_words(text) return words.pop() if len(words) == 1 else None def digest_is_incomplete(review): """True if a digest carries a REVIEW INCOMPLETE section, i.e. some of the author's commits were never reviewed. Requires the split footer as well as the section header. Only assemble_split_digest() ever emits this section, so on a batch digest the phrase can only be review prose - @@ -1755,40 +1762,55 @@ Those templates label the line "h3. Verdict:" or "h3. Status:" and never write "OVERALL STATUS:", so digest_overall_status() cannot read them. Reusing the daily readers here reported every approved Textile review as FEEDBACK.""" verdict = read_verdict(review) if verdict: return verdict for raw in review.splitlines(): match = re.match(r'^Status\s*[*_`]*\s*:\s*(.+)$', normalize_marker_line(raw), re.IGNORECASE) if match: word = verdict_value(match.group(1)) if word: return word return None -def digest_wants_email(review): +def digest_wants_email(review, num_commits=None): """True if this digest should be emailed to its author. Sends unless the digest definitively says APPROVED, and keeps the old bare substring test as a backstop so this can only ever send more than production did, never less. An unreadable status sends: between mailing a review nobody needed and silently - binning a real one, the first is the recoverable mistake.""" + binning a real one, the first is the recoverable mistake. + + One narrowing of that rule: the model drops the OVERALL STATUS block roughly one + digest in six, nearly always alongside the sections the prompt invites it to omit, + and in the first three weeks every one of those was an approved review mailed as + "possible FEEDBACK". So when the status is missing, the per-commit Verdict lines + decide instead - but only on a full accounting: exactly one readable verdict per + commit reviewed and every one APPROVED. A missing or unreadable verdict, an extra + one (e.g. quoted from the template), or any FEEDBACK still sends.""" if 'OVERALL STATUS: FEEDBACK' in review: return True - return digest_overall_status(review) != 'APPROVED' + status = digest_overall_status(review) + if status is not None: + return status != 'APPROVED' + if num_commits: + verdicts = verdict_lines(review) + if len(verdicts) == num_commits and set(verdicts) == {'APPROVED'}: + return False + return True def sanitize_commit_block(block, label=''): """Strip envelope lines and separator rules out of one commit block. build_daily_commit_prompt() tells the model not to emit them, but the model can ignore that, and a stray header or OVERALL STATUS line inside a block would show up as a duplicate in the assembled email. Also drops any preamble before the COMMIT line, and the code fence the model tends to wrap the block in. Reports what it removed. Every round of this function's history has had a bug where it silently ate real review text, and the reason each one took a code review to find is that deletion left no trace. A count and a preview in the log turns the next one into something anybody can spot in the nightly output.""" if not block: return '' @@ -2385,50 +2407,56 @@ # without counting as one, so the exit status still means "a review broke". incomplete = [(d['name'], "Reviewed, but some commits were left unreviewed " "(see REVIEW INCOMPLETE in the digest)") for d in reviews.values() if not d['error'] and digest_is_incomplete(d['review'])] def verdict_of(data): if data.get('skipped'): return 'NOT STARTED' if data['error']: return 'FAILED' # Report the send decision, not the parsed status. They can differ - the substring # backstop in digest_wants_email fires on a digest whose own status line reads # APPROVED - and a dry run that printed the status was telling the operator "no # email" on a night the live run mails. - return 'FEEDBACK' if digest_wants_email(data['review']) else 'APPROVED' + return ('FEEDBACK' if digest_wants_email(data['review'], data.get('num_commits')) + else 'APPROVED') # Phase 3: Send emails (only for reviews with FEEDBACK; never for failures) print(f"\nPhase 3: Sending emails (FEEDBACK only)...") if dry_run: print("[DRY RUN] Emails not sent. Reviews saved locally:") for author_email, data in reviews.items(): print(f" {data['name']} <{author_email}>: {verdict_of(data)} - {data['file']}") else: gmail_service = get_gmail_service() for author_email, data in reviews.items(): if data['error']: print(f" {data['name']}: FAILED - skipping author email (will alert maintainer)") continue - if not digest_wants_email(data['review']): + if not digest_wants_email(data['review'], data.get('num_commits')): + if digest_overall_status(data['review']) is None: + print(f" {data['name']}: APPROVED (no readable OVERALL STATUS, but " + f"every per-commit verdict is APPROVED) - skipping email") + else: print(f" {data['name']}: APPROVED - skipping email") continue if digest_overall_status(data['review']) is None: - print(f" {data['name']}: WARNING - could not read OVERALL STATUS, " - f"emailing anyway rather than dropping a possible FEEDBACK") + print(f" {data['name']}: WARNING - could not read OVERALL STATUS or " + f"account for every per-commit verdict, emailing anyway rather " + f"than dropping a possible FEEDBACK") print(f" Emailing {data['name']} <{author_email}> (FEEDBACK)...") try: send_review_email(gmail_service, author_email, data['name'], data['review'], cc=cc_address, review_file=data.get('file')) print(f" SENT") except Exception as e: # This email is the only channel that tells an author a commit of # theirs went unreviewed, so a send failure has to reach the # maintainer rather than being swallowed into the log. print(f" FAILED: {e}") failures.append((data['name'], f"Review completed but the email could " f"not be sent: {e}")) # Phase 3b: Alert the maintainer if anything failed, so it does not fail silently