Skip to content

test: pin each alert's kind to its message [RAI-3141] - #1718

Closed
JuaniRios wants to merge 1 commit into
rai-3148-board-dialog-polishfrom
rai-3141-alert-kind-tests
Closed

JuaniRios wants to merge 1 commit into
rai-3148-board-dialog-polishfrom
rai-3141-alert-kind-tests

Conversation

@JuaniRios

@JuaniRios JuaniRios commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

Part of RAI-3141.

Tests now pin every operational alert's kind to the phrase in its message, so a wrong kind fails a test instead of paging under the wrong rule once the pager reads jsonPayload.kind. A job's dead-letter kind now comes only from its message, so a new job cannot override the message and forget the kind.

Live effect: none (same alerts, same kinds; tests and one removed constant) · Risk: low (each job's message already carried its kind's phrase, so the derived kind is the one the constant gave) · Ships: next bot release · Blocks: needs #1717 below it

Decisions

  • Remove TERMINAL_FAILURE_KIND. Keeping it would mean adding it to all 28 jobs plus a hand-kept list of jobs for the test. Both alert paths now use terminal_failure_kind(alert): the most specific phrase in the message, else "Job failed after retries". A source scan fails if a job's TERMINAL_FAILURE_MSG names no known phrase.
  • Direct sites check their own message. The direct-site test reads each operational_alert call's kind = AlertKind::X and checks that the call's message literal holds X's phrase. Limits are in the test's doc: the kind must be written that way, and the message must be a plain literal.
  • Test notifiers check the kind. The hedge, gas and portfolio-snapshot test notifiers now assert the kind against the message, which covers the PlaceHedge dead-letter kind and LowGas. Notifiers whose tests use made-up failure text stay as they are.

Proof

  • New tests: every_direct_operational_alert_line_carries_its_messages_kind, every_terminal_failure_message_names_an_extracted_kind and terminal_failure_kind_prefers_the_most_specific_phrase. A kind the extractor does not know must now appear in its message. The full suite passes locally.
  • Not verified: the t0.devops extractor against these kinds.

@linear-code

linear-code Bot commented Oct 10, 2026

Copy link
Copy Markdown

RAI-3141

This was referenced Oct 10, 2026
@JuaniRios
JuaniRios force-pushed the rai-3141-alert-kind-tests branch from 84b0c0a to e1d8f6e Compare October 11, 2026 09:51
@JuaniRios
JuaniRios force-pushed the rai-3148-board-dialog-polish branch from 03ec8da to 47c7585 Compare October 11, 2026 09:51
@JuaniRios

Copy link
Copy Markdown
Collaborator Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Comments resolved and changes approved.

@rain-marvin

rain-marvin Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

🔎 Reviewing e1d8f6e, started by @JuaniRios. The review will appear here when it's done.

@JuaniRios

Copy link
Copy Markdown
Collaborator Author

@rain-marvin approve

@rain-marvin

rain-marvin Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

⏳ Queued behind 52 other jobs, @JuaniRios. I'll post here when I start.

@rain-marvin rain-marvin Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes the tests pin each operational alert's kind to the phrase in its message, so a wrong kind fails a test instead of paging under the wrong rule. It removes TERMINAL_FAILURE_KIND: a job's terminal page now takes its kind only from its text (terminal_failure_kind), and both worker macros fail the build when a TERMINAL_FAILURE_MSG holds no extracted phrase.

Overall read: good to merge. Live behavior does not change. Every overriding job's message (dashboard delivery, mint authorization, tokenization resume) and the trait default already held the phrase of the kind the removed constant named, so the old fallback never decided a page. The removed kind fields were in memory only, so no persisted state or upgrade path is affected. CI is green on this head, and every earlier thread is fixed and resolved.

Two small notes on the PR description, not the code. The reason given for removing the constant ("adding it to all 28 jobs plus a hand-kept list") no longer holds now that the const assert in the worker macros checks every job. The real trade-off is less plumbing in exchange for no not_extracted kind on a terminal page. And "every operational alert" is wider than the tests reach: an alert that no test drives, such as page_underlying_mismatch, is still not pinned, although its message does carry its kind's phrase today.

claude-opus-5-5 · high · 1 h 2 min

Comment thread src/conductor/job.rs
@JuaniRios
JuaniRios force-pushed the rai-3141-alert-kind-tests branch from e1d8f6e to c0c18d0 Compare October 11, 2026 12:02
@JuaniRios
JuaniRios force-pushed the rai-3148-board-dialog-polish branch from 47c7585 to 66f8d8d Compare October 11, 2026 12:02
@JuaniRios

Copy link
Copy Markdown
Collaborator Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Comments resolved and changes approved.

@rain-marvin

rain-marvin Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

🔎 Verifying that my findings from c0c18d0 are addressed at c0c18d0, started by @JuaniRios. The review will appear here when it's done.

@JuaniRios
JuaniRios force-pushed the rai-3148-board-dialog-polish branch from 66f8d8d to c3fce3b Compare October 11, 2026 15:23
@JuaniRios
JuaniRios force-pushed the rai-3141-alert-kind-tests branch from c0c18d0 to 1685202 Compare October 11, 2026 15:23
@JuaniRios

Copy link
Copy Markdown
Collaborator Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Comments resolved and changes approved.

@rain-marvin rain-marvin Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR pins each operational alert's kind to its message text. Direct alert sites are scanned and checked against their message literal. Every test notifier calls assert_kind_matches_extractor. A job's terminal page takes its kind from the most specific extracted phrase, falling back to JobFailedAfterRetries, and the worker macros fail the build when a TERMINAL_FAILURE_MSG holds no extracted phrase.

The head (1685202) is c0c18d0 rebased onto the new tip of rai-3148-board-dialog-polish. This PR's own patch is byte for byte the same as the one I reviewed. The base moved only in dashboard/ and docs/cli-ops.md, and none of those files overlap with this PR. I found no new defects.

Earlier findings:

  • terminal_failure_kind / standing_delta_page one-liners (nit): resolved. Keeping terminal_failure_kind gives both terminal paths one shared fallback rule. Inlining standing_delta_page is tracked in RAI-3195.

Threads resolved by hand:

  • Test notifiers skipping assert_kind_matches_extractor (src/alerts/mod.rs): addressed. Every non-capturing notifier now calls it.
  • %/? sigil field values in the direct-site parser: addressed, with a parser test for every form.
  • TERMINAL_FAILURE_MSG scan missing other type spellings: addressed. A const { assert!(...) } in both worker macros now fails the build, and the scan accepts any type spelling.
  • docs/observability.md alert-kind rules out of date: addressed. The section was rewritten.
  • Terminal page cannot use a not_extracted kind: addressed. The docs now say so.
  • Redundant TERMINAL_FAILURE_MSG source scan: dismissed for this PR. Removing it is tracked in RAI-3195. That holds, because the scan pages nothing differently and only duplicates the build check.
  • terminal_failure_kind one-liner nit: dismissed with a sound reason (one shared fallback for both paths).

CodeRabbit's extractor-drift thread was resolved by CodeRabbit itself. The repo pins the checked-in extractor alternation and covers the precedence case in the_rightmost_phrase_wins. Checking for drift against the deployed extractor belongs in the ops repo.

CI on the new head is still running (backend test shards pending). The failing backend entry comes from a separate run that ended in 5s, not from the current one.

claude-opus-5-5 · high · 16 min

@JuaniRios
JuaniRios force-pushed the rai-3148-board-dialog-polish branch from c3fce3b to c7cdf8f Compare October 11, 2026 17:41
@JuaniRios
JuaniRios force-pushed the rai-3141-alert-kind-tests branch from 1685202 to b02453f Compare October 11, 2026 17:41
@JuaniRios

Copy link
Copy Markdown
Collaborator Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Comments resolved and changes approved.

@agryaznov agryaznov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the deferred minors are tracked in the threads.

@graphite-app
graphite-app Bot force-pushed the rai-3148-board-dialog-polish branch from c7cdf8f to 490858b Compare October 12, 2026 16:32
@graphite-app
graphite-app Bot force-pushed the rai-3141-alert-kind-tests branch from b02453f to 1db48ad Compare October 12, 2026 16:33
@graphite-app

graphite-app Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Merge activity

  • Oct 12, 5:07 PM UTC: JuaniRios added this pull request to the Graphite merge queue.
  • Oct 12, 5:08 PM UTC: CI is running for this pull request on a draft pull request (#1728) due to your merge queue CI optimization settings.
  • Oct 12, 5:25 PM UTC: Merged by the Graphite merge queue via draft PR: #1728.

@graphite-app graphite-app Bot closed this Oct 12, 2026
@graphite-app
graphite-app Bot deleted the rai-3141-alert-kind-tests branch October 12, 2026 17:26
@github-actions github-actions Bot added the externally-merged Added to a PR that appears closed but is actually merged by an external actor like Graphite label Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

externally-merged Added to a PR that appears closed but is actually merged by an external actor like Graphite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants