From cb02badf2691c79c9cb1492356dc885333397406 Mon Sep 17 00:00:00 2001 From: Alexandra Doak Date: Wed, 2 Sep 2026 12:06:27 -0700 Subject: [PATCH 1/8] Updating validation for recovery delay --- .../src/rule_data_schema.test.ts | 130 ++++++++++++++++++ .../src/rule_data_schema.ts | 35 +++++ .../tools/manage_rule/operations.test.ts | 32 +++++ .../tools/manage_rule/operations.ts | 7 + .../alerting_v2/server/lib/errors/README.md | 1 + .../server/lib/errors/error_codes.ts | 7 + .../lib/rules_client/rules_client.test.ts | 30 ++++ .../server/lib/rules_client/utils.test.ts | 52 +++++++ .../server/lib/rules_client/utils.ts | 8 ++ .../api/tests/director.spec.ts | 5 +- .../rules/api/tests/create_rule.spec.ts | 17 +++ .../rules/api/tests/update_rule.spec.ts | 24 ++++ .../rules/api/tests/upsert_rule.spec.ts | 16 +++ 13 files changed, 362 insertions(+), 2 deletions(-) diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts index 2a95da9ce3dc7..cf7bf85d2e19d 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts @@ -9,6 +9,7 @@ import { RUNBOOK_ARTIFACT_TYPE, RUNBOOK_CONTENT_LIMIT } from '@kbn/alerting-v2-c import { createRuleDataBaseSchema, createRuleDataSchema, + isRecoveryDelayAllowed, updateRuleDataSchema, IMMUTABLE_RULE_FIELDS, getBreachEsqlQuery, @@ -55,6 +56,7 @@ describe('createRuleDataSchema', () => { metadata: { name: 'test rule', owner: 'team-a', tags: ['label-1', 'label-2'] }, time_field: 'event.created', schedule: { every: '5m', lookback: '10m' }, + recovery_strategy: 'no_breach', grouping: { fields: ['host.name'] }, state_transition: { pending_operator: 'AND', @@ -72,6 +74,7 @@ describe('createRuleDataSchema', () => { metadata: { name: 'test rule', owner: 'team-a', tags: ['label-1', 'label-2'] }, time_field: 'event.created', schedule: { every: '5m', lookback: '10m' }, + recovery_strategy: 'no_breach', grouping: { fields: ['host.name'] }, state_transition: { pending_operator: 'AND', @@ -702,6 +705,7 @@ describe('createRuleDataSchema', () => { it('accepts state_transition with only recovering fields', () => { const result = createRuleDataSchema.parse({ ...validCreateData, + recovery_strategy: 'no_breach', state_transition: { recovering_operator: 'OR', recovering_count: 5, @@ -863,6 +867,83 @@ describe('createRuleDataSchema', () => { }); }); + describe('recovery delay allowed', () => { + it('rejects a recovering_count when recovery_strategy is unset', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + state_transition: { pending_count: 0, recovering_count: 2 }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects a recovering_count when recovery_strategy is "none"', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { recovering_count: 2 }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects a recovering_timeframe when recovery is disabled', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { recovering_timeframe: '5m' }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects an inert recovery delay even when no_data_strategy is "recover"', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + no_data_strategy: 'recover', + query: { + format: 'standalone', + breach: { query: 'FROM logs-* | LIMIT 1' }, + no_data: { query: 'FROM logs-* | STATS c = COUNT(*)' }, + }, + state_transition: { recovering_count: 2 }, + }); + + expect(result.success).toBe(false); + }); + + it('accepts recovering_count of 0 when recovery is disabled', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 0 }, + }); + + expect(result.success).toBe(true); + }); + + it('accepts pending-only state_transition when recovery is disabled', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { pending_count: 3 }, + }); + + expect(result.success).toBe(true); + }); + + it('accepts a recovering delay when recovery_strategy is "no_breach"', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 2, recovering_timeframe: '5m' }, + }); + + expect(result.success).toBe(true); + }); + }); + describe('artifacts envelope', () => { const parseWithArtifact = (artifact: Record) => createRuleDataSchema.safeParse({ ...validCreateData, artifacts: [artifact] }); @@ -1662,3 +1743,52 @@ describe('tagsResponseSchema', () => { expect(() => tagsResponseSchema.parse({})).toThrow(); }); }); + +describe('isRecoveryDelayAllowed', () => { + it('returns true when recovery is enabled, regardless of recovering delay', () => { + expect( + isRecoveryDelayAllowed({ + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 3, recovering_timeframe: '5m' }, + }) + ).toBe(true); + expect( + isRecoveryDelayAllowed({ + recovery_strategy: 'query', + state_transition: { recovering_count: 3 }, + }) + ).toBe(true); + }); + + it('returns true when recovery is disabled but no recovering delay is set', () => { + expect(isRecoveryDelayAllowed({ recovery_strategy: 'none' })).toBe(true); + expect(isRecoveryDelayAllowed({ recovery_strategy: null })).toBe(true); + expect(isRecoveryDelayAllowed({})).toBe(true); + expect(isRecoveryDelayAllowed({ recovery_strategy: 'none', state_transition: {} })).toBe(true); + }); + + it('treats recovering_count 0 as no delay even when recovery is disabled', () => { + expect( + isRecoveryDelayAllowed({ + recovery_strategy: 'none', + state_transition: { recovering_count: 0 }, + }) + ).toBe(true); + }); + + it('returns false for a positive recovering delay when recovery is disabled', () => { + expect( + isRecoveryDelayAllowed({ + recovery_strategy: 'none', + state_transition: { recovering_count: 1 }, + }) + ).toBe(false); + expect( + isRecoveryDelayAllowed({ + recovery_strategy: null, + state_transition: { recovering_timeframe: '5m' }, + }) + ).toBe(false); + expect(isRecoveryDelayAllowed({ state_transition: { recovering_count: 2 } })).toBe(false); + }); +}); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts index 46c3dd2ee464a..87b88d0d7807d 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts @@ -541,6 +541,36 @@ export const isNoDataQueryProvidedForStrategy = (data: { export const isNoDataStrategyNotEmit = (data: { no_data_strategy?: NoDataStrategy | null; }): boolean => data.no_data_strategy !== noDataStrategy.emit; + +/** + * A recovery delay is inert when recovery is disabled (`recovery_strategy` is + * `none` or unset), so we reject it. Only a positive delay counts + * (`recovering_count > 0` or any `recovering_timeframe`); `recovering_count: 0` + * means "recover immediately" and is always allowed. + */ +export const isRecoveryDelayAllowed = (data: { + recovery_strategy?: RecoveryStrategy | null; + state_transition?: { + recovering_count?: number | null; + recovering_timeframe?: string | null; + } | null; +}): boolean => { + const recoveryEnabled = + data.recovery_strategy != null && data.recovery_strategy !== recoveryStrategy.none; + if (recoveryEnabled) { + return true; + } + + const stateTransition = data.state_transition; + if (stateTransition == null) { + return true; + } + + const hasRecoveryDelay = + (stateTransition.recovering_count != null && stateTransition.recovering_count > 0) || + stateTransition.recovering_timeframe != null; + return !hasRecoveryDelay; +}; const rejectEmitNoDataStrategy = { message: 'no_data_strategy "emit" is not currently supported.', path: ['no_data_strategy'], @@ -577,6 +607,11 @@ export const createRuleDataSchema = createRuleDataBaseSchema path: ['query', 'no_data'], }) .refine(isNoDataStrategyNotEmit, rejectEmitNoDataStrategy) + .refine(isRecoveryDelayAllowed, { + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + path: ['state_transition', 'recovering_count'], + }) .meta({ id: 'alerting_new_rule' }); export type CreateRuleData = z.infer; diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts index 1cd6ae106e685..3dd49c6e7b135 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts @@ -704,6 +704,38 @@ describe('executeRuleOperations', () => { ); }); + it('throws when a recovering delay is set while recovery is disabled', async () => { + const ops: RuleOperation[] = [ + { operation: 'set_kind', kind: 'alert' }, + { + operation: 'set_query', + query: { format: 'standalone', breach: { query: 'FROM metrics-* | STATS COUNT(*)' } }, + }, + { operation: 'set_state_transition', pending_count: 0, recovering_count: 2 }, + ]; + + await expect(executeRuleOperations({}, ops)).rejects.toThrow( + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled' + ); + await expect(executeRuleOperations({}, ops)).rejects.toBeInstanceOf( + RuleOperationValidationError + ); + }); + + it('allows recovering_count 0 (immediate) while recovery is disabled', async () => { + const ops: RuleOperation[] = [ + { operation: 'set_kind', kind: 'alert' }, + { + operation: 'set_query', + query: { format: 'standalone', breach: { query: 'FROM metrics-* | STATS COUNT(*)' } }, + }, + { operation: 'set_state_transition', pending_count: 0, recovering_count: 0 }, + ]; + + const result = await executeRuleOperations({}, ops); + expect(result.data.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); + }); + it('throws when signal rule uses composed query format', async () => { const ops: RuleOperation[] = [ { operation: 'set_kind', kind: 'signal' }, diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts index 761ef86af4af2..97ee8b46d03f7 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts @@ -28,6 +28,7 @@ import { isStateTransitionAllowed, isSignalUsingStandaloneFormat, isSignalQueryBreachOnly, + isRecoveryDelayAllowed, isRecoveryQueryConsistentWithStrategy, isRecoveryQueryProvidedForStrategy, isNoDataQueryConsistentWithStrategy, @@ -501,6 +502,12 @@ export const executeRuleOperations = async ( ); } + if (!isRecoveryDelayAllowed(next)) { + throw new RuleOperationValidationError( + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).' + ); + } + return { data: next, ...(lastQueryColumns ? { queryColumns: lastQueryColumns } : {}), diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md index 8de1943b59a82..1c25b2856256f 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md @@ -66,6 +66,7 @@ backwards compatible. Renaming or removing a code is a breaking change. | `INVALID_STATE_TRANSITION` | 400 | `state_transition` is incompatible with the rule's `kind` | `{ rule_id, kind, transition }` | | `INVALID_SIGNAL_RULE` | 400 | An update would leave a signal rule in an invalid shape | `{ rule_id, rule_kind }` | | `INVALID_RULE_QUERY_CONFIG` | 400 | An update would desynchronize a `recovery_strategy`/`no_data_strategy` from its `query.recovery`/`query.no_data` block | `{ rule_id }` | +| `INVALID_STATE_TRANSITION_CONFIG` | 400 | A recovery delay (`state_transition.recovering_count`/`recovering_timeframe`) is set while recovery is disabled, where it is inert | `{ rule_id }` | | `BULK_QUERY_MATCH_LIMIT_EXCEEDED` | 400 | By-query bulk operation with `force: true` matched more resources than the per-request cap; rejected before any resource is mutated | `{ match_count, limit }` | | `IMMUTABLE_FIELDS_CHANGED` | 400 | PUT (upsert) request changes a field flagged as immutable | `{ fields }` | | `INVALID_FILTER_FIELD` | 400 | The `filter` references a field that is not in the allow-list | `{ field, allowed_fields }` | diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts index da3c5edc1d5f8..5f7230b5416a8 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts @@ -35,6 +35,13 @@ export const ALERTING_ERROR_CODES = { INVALID_ARTIFACT_DATA: 'INVALID_ARTIFACT_DATA', /** `state_transition` cannot be applied to the rule's `kind`. */ INVALID_STATE_TRANSITION: 'INVALID_STATE_TRANSITION', + /** + * A rule's merged shape carries a recovery delay + * (`state_transition.recovering_count` / `recovering_timeframe`) while + * recovery is disabled (`recovery_strategy` is `none` or unset), where it + * would never take effect. + */ + INVALID_STATE_TRANSITION_CONFIG: 'INVALID_STATE_TRANSITION_CONFIG', /** A signal rule's merged shape violates signal constraints. */ INVALID_SIGNAL_RULE: 'INVALID_SIGNAL_RULE', /** diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts index 1e896b3a0c0fc..62ee30947d28c 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts @@ -458,6 +458,36 @@ describe('RulesClient', () => { ).resolves.not.toThrow(); }); + it('throws 400 when disabling recovery leaves a stored recovering delay inert', async () => { + const client = createClient(); + + const existingAttributes: RuleSavedObjectAttributes = { + ...baseSoAttrs, + kind: 'alert', + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 3 }, + }; + + rulesSavedObjectService.get.mockResolvedValueOnce({ + id: 'rule-id-inert-recovery-delay', + attributes: existingAttributes, + version: 'WzEsMV0=', + }); + + await expect( + client.updateRule({ + id: 'rule-id-inert-recovery-delay', + data: { recovery_strategy: 'none' }, + }) + ).rejects.toMatchObject({ + output: { statusCode: 400 }, + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + }); + + expect(rulesSavedObjectService.update).not.toHaveBeenCalled(); + }); + it('throws 400 when updating a signal rule query to composed format', async () => { const client = createClient(); diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts index 3abf67271866a..d0ba1c9d037ad 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts @@ -766,6 +766,58 @@ describe('utils', () => { expect(() => validateMergedRuleAttributes('rule-1', attrs)).not.toThrow(); }); + + it('throws INVALID_STATE_TRANSITION_CONFIG (400) when a recovering delay is set while recovery is disabled', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: 'none', + state_transition: { recovering_count: 3 }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).toThrow( + expect.objectContaining({ + isBoom: true, + output: expect.objectContaining({ statusCode: 400 }), + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + data: { code: 'INVALID_STATE_TRANSITION_CONFIG', details: { rule_id: 'rule-1' } }, + }) + ); + }); + + it('throws INVALID_STATE_TRANSITION_CONFIG when a recovering_timeframe is set while recovery is unset', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: undefined, + state_transition: { recovering_timeframe: '5m' }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).toThrow( + expect.objectContaining({ + data: { code: 'INVALID_STATE_TRANSITION_CONFIG', details: { rule_id: 'rule-1' } }, + }) + ); + }); + + it('does not throw for a recovering delay when recovery is enabled', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 3, recovering_timeframe: '5m' }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).not.toThrow(); + }); + + it('does not throw for recovering_count 0 (immediate) when recovery is disabled', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 0 }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).not.toThrow(); + }); }); describe('pickImmutable', () => { diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts index 8c1d1b78a57ab..a3a7fcedd063c 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts @@ -12,6 +12,7 @@ import { IMMUTABLE_RULE_FIELDS, isNoDataQueryConsistentWithStrategy, isNoDataQueryProvidedForStrategy, + isRecoveryDelayAllowed, isRecoveryQueryConsistentWithStrategy, isRecoveryQueryProvidedForStrategy, isSignalQueryBreachOnly, @@ -405,6 +406,13 @@ export function validateMergedRuleAttributes( code: ALERTING_ERROR_CODES.INVALID_RULE_QUERY_CONFIG, details: { rule_id: ruleId }, }, + { + valid: isRecoveryDelayAllowed(attrs), + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + code: ALERTING_ERROR_CODES.INVALID_STATE_TRANSITION_CONFIG, + details: { rule_id: ruleId }, + }, ]; for (const invariant of invariants) { diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts index a66bf21c817cd..6685037e25431 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts @@ -1513,7 +1513,7 @@ apiTest.describe('Director', { tag: tags.stateful.classic }, () => { }, recovery_strategy: 'none', no_data_strategy: 'recover', - state_transition: { pending_count: 0, recovering_count: 1 }, + state_transition: { pending_count: 0 }, }) ); @@ -1535,7 +1535,8 @@ apiTest.describe('Director', { tag: tags.stateful.classic }, () => { }); expect(noDataEvents.length).toBeGreaterThanOrEqual(1); - // The director resolves the episode directly to inactive, ignoring recovering_count. + // The director resolves the episode directly to inactive on a no_data + // event when no_data_strategy is 'recover'. for (const event of noDataEvents) { expect(event.episode?.status).toBe('inactive'); } diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts index 7ff6488ffa7b3..37fe8f86b595e 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts @@ -309,6 +309,23 @@ apiTest.describe('Create rule API', { tag: '@local-stateful-classic' }, () => { } ); + apiTest( + 'validation: rejects a recovering delay when recovery is disabled', + async ({ apiClient }) => { + const body = buildCreateRuleData({ + metadata: { name: 'invalid-inert-recovery-delay' }, + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 2 }, + }); + const response = await apiClient.post(testData.RULE_API_PATH, { + headers: writerHeaders, + body, + }); + expect(response).toHaveStatusCode(400); + expect(response.body.code).toBe('BAD_REQUEST'); + } + ); + apiTest( 'create: returns 201 with the signal kind round-tripped to the response', async ({ apiClient, apiServices }) => { diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts index 2bd8e693e822b..b143689dd68d0 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts @@ -486,6 +486,30 @@ apiTest.describe('Update rule API', { tag: '@local-stateful-classic' }, () => { } ); + apiTest( + 'validation: should reject disabling recovery that would leave a stored recovering delay inert', + async ({ apiClient, apiServices }) => { + const created = await apiServices.alertingV2.rules.create( + buildCreateRuleData({ + metadata: { name: 'rule-inert-recovery-delay-on-update' }, + recovery_strategy: 'no_breach', + state_transition: { pending_count: 0, recovering_count: 3 }, + }) + ); + + const response = await apiClient.patch(getRuleUrl(created.id), { + headers: writerHeaders, + body: { recovery_strategy: 'none' }, + }); + + expect(response).toHaveStatusCode(400); + expect(response.body.code).toBe('INVALID_STATE_TRANSITION_CONFIG'); + + const stored = await apiServices.alertingV2.rules.get(created.id); + expect(stored.recovery_strategy).toBe('no_breach'); + } + ); + const buildSignalRuleData = (name: string) => buildCreateRuleData({ kind: 'signal', diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts index a80896f10e604..2fa8bb8ce4963 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts @@ -287,6 +287,22 @@ apiTest.describe('Upsert rule API', { tag: '@local-stateful-classic' }, () => { expect(response.body.code).toBe('BAD_REQUEST'); }); + apiTest( + 'validation: should reject a recovering delay when recovery is disabled', + async ({ apiClient }) => { + const response = await apiClient.put(getRuleUrl('any-id'), { + headers: writerHeaders, + body: buildCreateRuleData({ + metadata: { name: 'upsert-inert-recovery-delay' }, + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 2 }, + }), + }); + expect(response).toHaveStatusCode(400); + expect(response.body.code).toBe('BAD_REQUEST'); + } + ); + apiTest( 'authorization: should return 201 for a user with full alerting_v2 privileges', async ({ apiClient }) => { From 00f662bdf026ab980355813dc6bdcf1b969069c8 Mon Sep 17 00:00:00 2001 From: Alexandra Doak Date: Tue, 8 Sep 2026 10:28:58 -0700 Subject: [PATCH 2/8] Addressing PR comments --- .../src/rule_data_schema.test.ts | 40 +++++++++++-------- .../src/rule_data_schema.ts | 20 +++++----- .../tools/manage_rule/operations.test.ts | 10 +++-- .../tools/manage_rule/operations.ts | 4 +- .../server/lib/rules_client/utils.test.ts | 8 +++- .../server/lib/rules_client/utils.ts | 4 +- 6 files changed, 51 insertions(+), 35 deletions(-) diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts index cf7bf85d2e19d..d52372e77aa8e 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts @@ -9,7 +9,7 @@ import { RUNBOOK_ARTIFACT_TYPE, RUNBOOK_CONTENT_LIMIT } from '@kbn/alerting-v2-c import { createRuleDataBaseSchema, createRuleDataSchema, - isRecoveryDelayAllowed, + isRecoveryTransitionConsistentWithStrategy, updateRuleDataSchema, IMMUTABLE_RULE_FIELDS, getBreachEsqlQuery, @@ -732,6 +732,7 @@ describe('createRuleDataSchema', () => { it('accepts recovering_count of 0', () => { const result = createRuleDataSchema.parse({ ...validCreateData, + recovery_strategy: 'no_breach', state_transition: { recovering_count: 0 }, }); @@ -913,14 +914,14 @@ describe('createRuleDataSchema', () => { expect(result.success).toBe(false); }); - it('accepts recovering_count of 0 when recovery is disabled', () => { + it('rejects recovering_count of 0 when recovery is disabled', () => { const result = createRuleDataSchema.safeParse({ ...validCreateData, recovery_strategy: 'none', state_transition: { pending_count: 0, recovering_count: 0 }, }); - expect(result.success).toBe(true); + expect(result.success).toBe(false); }); it('accepts pending-only state_transition when recovery is disabled', () => { @@ -1744,16 +1745,16 @@ describe('tagsResponseSchema', () => { }); }); -describe('isRecoveryDelayAllowed', () => { +describe('isRecoveryTransitionConsistentWithStrategy', () => { it('returns true when recovery is enabled, regardless of recovering delay', () => { expect( - isRecoveryDelayAllowed({ + isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: 'no_breach', state_transition: { recovering_count: 3, recovering_timeframe: '5m' }, }) ).toBe(true); expect( - isRecoveryDelayAllowed({ + isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: 'query', state_transition: { recovering_count: 3 }, }) @@ -1761,34 +1762,41 @@ describe('isRecoveryDelayAllowed', () => { }); it('returns true when recovery is disabled but no recovering delay is set', () => { - expect(isRecoveryDelayAllowed({ recovery_strategy: 'none' })).toBe(true); - expect(isRecoveryDelayAllowed({ recovery_strategy: null })).toBe(true); - expect(isRecoveryDelayAllowed({})).toBe(true); - expect(isRecoveryDelayAllowed({ recovery_strategy: 'none', state_transition: {} })).toBe(true); + expect(isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: 'none' })).toBe(true); + expect(isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: null })).toBe(true); + expect(isRecoveryTransitionConsistentWithStrategy({})).toBe(true); + expect( + isRecoveryTransitionConsistentWithStrategy({ + recovery_strategy: 'none', + state_transition: {}, + }) + ).toBe(true); }); - it('treats recovering_count 0 as no delay even when recovery is disabled', () => { + it('rejects recovering_count 0 when recovery is disabled (immediate recovery is not a delay)', () => { expect( - isRecoveryDelayAllowed({ + isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: 'none', state_transition: { recovering_count: 0 }, }) - ).toBe(true); + ).toBe(false); }); it('returns false for a positive recovering delay when recovery is disabled', () => { expect( - isRecoveryDelayAllowed({ + isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: 'none', state_transition: { recovering_count: 1 }, }) ).toBe(false); expect( - isRecoveryDelayAllowed({ + isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: null, state_transition: { recovering_timeframe: '5m' }, }) ).toBe(false); - expect(isRecoveryDelayAllowed({ state_transition: { recovering_count: 2 } })).toBe(false); + expect( + isRecoveryTransitionConsistentWithStrategy({ state_transition: { recovering_count: 2 } }) + ).toBe(false); }); }); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts index 87b88d0d7807d..d3a920aa10c20 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts @@ -543,12 +543,13 @@ export const isNoDataStrategyNotEmit = (data: { }): boolean => data.no_data_strategy !== noDataStrategy.emit; /** - * A recovery delay is inert when recovery is disabled (`recovery_strategy` is - * `none` or unset), so we reject it. Only a positive delay counts - * (`recovering_count > 0` or any `recovering_timeframe`); `recovering_count: 0` - * means "recover immediately" and is always allowed. + * Recovery transition thresholds are inert when recovery is disabled + * (`recovery_strategy` is `none` or unset), so we reject any `recovering_count` + * (including `0`) or `recovering_timeframe`. `recovering_count: 0` is not a + * delay — the episode recovers immediately — so it must not be configured while + * recovery is off. */ -export const isRecoveryDelayAllowed = (data: { +export const isRecoveryTransitionConsistentWithStrategy = (data: { recovery_strategy?: RecoveryStrategy | null; state_transition?: { recovering_count?: number | null; @@ -566,10 +567,9 @@ export const isRecoveryDelayAllowed = (data: { return true; } - const hasRecoveryDelay = - (stateTransition.recovering_count != null && stateTransition.recovering_count > 0) || - stateTransition.recovering_timeframe != null; - return !hasRecoveryDelay; + const hasRecoveringConfig = + stateTransition.recovering_count != null || stateTransition.recovering_timeframe != null; + return !hasRecoveringConfig; }; const rejectEmitNoDataStrategy = { message: 'no_data_strategy "emit" is not currently supported.', @@ -607,7 +607,7 @@ export const createRuleDataSchema = createRuleDataBaseSchema path: ['query', 'no_data'], }) .refine(isNoDataStrategyNotEmit, rejectEmitNoDataStrategy) - .refine(isRecoveryDelayAllowed, { + .refine(isRecoveryTransitionConsistentWithStrategy, { message: 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', path: ['state_transition', 'recovering_count'], diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts index 3dd49c6e7b135..2e2b2d9a8492d 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts @@ -722,7 +722,7 @@ describe('executeRuleOperations', () => { ); }); - it('allows recovering_count 0 (immediate) while recovery is disabled', async () => { + it('throws when recovering_count 0 is set while recovery is disabled', async () => { const ops: RuleOperation[] = [ { operation: 'set_kind', kind: 'alert' }, { @@ -732,8 +732,12 @@ describe('executeRuleOperations', () => { { operation: 'set_state_transition', pending_count: 0, recovering_count: 0 }, ]; - const result = await executeRuleOperations({}, ops); - expect(result.data.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); + await expect(executeRuleOperations({}, ops)).rejects.toThrow( + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled' + ); + await expect(executeRuleOperations({}, ops)).rejects.toBeInstanceOf( + RuleOperationValidationError + ); }); it('throws when signal rule uses composed query format', async () => { diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts index 97ee8b46d03f7..4fad38ddffb15 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts @@ -28,7 +28,7 @@ import { isStateTransitionAllowed, isSignalUsingStandaloneFormat, isSignalQueryBreachOnly, - isRecoveryDelayAllowed, + isRecoveryTransitionConsistentWithStrategy, isRecoveryQueryConsistentWithStrategy, isRecoveryQueryProvidedForStrategy, isNoDataQueryConsistentWithStrategy, @@ -502,7 +502,7 @@ export const executeRuleOperations = async ( ); } - if (!isRecoveryDelayAllowed(next)) { + if (!isRecoveryTransitionConsistentWithStrategy(next)) { throw new RuleOperationValidationError( 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).' ); diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts index d0ba1c9d037ad..89b0c56b82cf5 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts @@ -809,14 +809,18 @@ describe('utils', () => { expect(() => validateMergedRuleAttributes('rule-1', attrs)).not.toThrow(); }); - it('does not throw for recovering_count 0 (immediate) when recovery is disabled', () => { + it('throws INVALID_STATE_TRANSITION_CONFIG for recovering_count 0 when recovery is disabled', () => { const attrs = createRuleSoAttributes({ kind: 'alert', recovery_strategy: 'none', state_transition: { pending_count: 0, recovering_count: 0 }, }); - expect(() => validateMergedRuleAttributes('rule-1', attrs)).not.toThrow(); + expect(() => validateMergedRuleAttributes('rule-1', attrs)).toThrow( + expect.objectContaining({ + data: { code: 'INVALID_STATE_TRANSITION_CONFIG', details: { rule_id: 'rule-1' } }, + }) + ); }); }); diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts index a3a7fcedd063c..0ea03edad52ac 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts @@ -12,7 +12,7 @@ import { IMMUTABLE_RULE_FIELDS, isNoDataQueryConsistentWithStrategy, isNoDataQueryProvidedForStrategy, - isRecoveryDelayAllowed, + isRecoveryTransitionConsistentWithStrategy, isRecoveryQueryConsistentWithStrategy, isRecoveryQueryProvidedForStrategy, isSignalQueryBreachOnly, @@ -407,7 +407,7 @@ export function validateMergedRuleAttributes( details: { rule_id: ruleId }, }, { - valid: isRecoveryDelayAllowed(attrs), + valid: isRecoveryTransitionConsistentWithStrategy(attrs), message: 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', code: ALERTING_ERROR_CODES.INVALID_STATE_TRANSITION_CONFIG, From 9b736cc467454fd280491a7a429e811d480390b2 Mon Sep 17 00:00:00 2001 From: Alexandra Doak Date: Tue, 8 Sep 2026 10:30:13 -0700 Subject: [PATCH 3/8] Adding UI changes --- .../recovery_condition_step.test.tsx | 18 ++++++++ .../recovery_condition_step.tsx | 14 +++++- .../compose_discover/compose_mappers.test.ts | 19 +++++++- .../compose_discover/compose_mappers.ts | 22 ++++++---- .../form/fields/alert_delay_field.test.tsx | 1 + .../form/fields/recovery_delay_field.test.tsx | 1 + .../form/utils/rule_request_mappers.test.ts | 44 ++++++++++++++++--- .../form/utils/rule_request_mappers.ts | 37 ++++++++++++---- 8 files changed, 129 insertions(+), 27 deletions(-) diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx index 985afac0e6a9d..f05a2f5caa4fc 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx @@ -120,6 +120,24 @@ describe('RecoveryConditionStep', () => { expect(screen.queryByTestId('composeDiscoverEditRecovery')).not.toBeInTheDocument(); }); + it('renders the recovery delay field when recovery type is default', () => { + renderRecoveryStep({ recoveryType: 'default' }); + + expect(screen.getByTestId('recoveryDelayFormRow')).toBeInTheDocument(); + }); + + it('renders the recovery delay field when recovery type is custom', () => { + renderRecoveryStep({ recoveryType: 'custom' }, CUSTOM_RECOVERY_QUERY); + + expect(screen.getByTestId('recoveryDelayFormRow')).toBeInTheDocument(); + }); + + it('hides the recovery delay field when recovery type is none (delay is inert)', () => { + renderRecoveryStep({ recoveryType: 'none' }); + + expect(screen.queryByTestId('recoveryDelayFormRow')).not.toBeInTheDocument(); + }); + it('renders query summaries and edit button in custom mode', () => { renderRecoveryStep({ recoveryType: 'custom' }, CUSTOM_RECOVERY_QUERY); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx index 1a9c171e081cf..000f35934cc26 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx @@ -146,8 +146,18 @@ export function RecoveryConditionStep({ )} - - + {/* + * Recovery delay only applies to condition-based recovery. When recovery is + * disabled ('none'), the delay is inert (and rejected by the write API), so we + * hide it. `no_data_strategy: 'recover'` does not re-enable it — the director + * bypasses recovering gating for no-data recovery. + */} + {state.recoveryType !== 'none' && ( + <> + + + + )} ); } diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts index 78499f8c72b7b..3a9ee75a8ef36 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts @@ -170,11 +170,28 @@ describe('composeFormToCreateRequest', () => { expect(result.state_transition).toBeUndefined(); }); - it('maps state_transition for immediate delay mode', () => { + it('maps state_transition for immediate delay mode (recovery disabled omits recovering_count)', () => { const result = composeFormToCreateRequest(baseFormValues); + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + + it('emits recovering_count: 0 for immediate delay mode when recovery is enabled', () => { + const values: FormValues = { ...baseFormValues, recoveryStrategy: 'no_breach' }; + const result = composeFormToCreateRequest(values); expect(result.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); }); + it('omits recovering fields when recovery_strategy is "none" even if recovering values are set', () => { + const values: FormValues = { + ...baseFormValues, + recoveryStrategy: 'none', + stateTransitionRecoveryDelayMode: 'recoveries', + stateTransition: { recoveringCount: 3 }, + }; + const result = composeFormToCreateRequest(values); + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + it('maps state_transition for breaches delay mode', () => { const values: FormValues = { ...baseFormValues, diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts index 32048c7133f58..4d964ae699683 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts @@ -16,7 +16,7 @@ import { deriveAlertDelayModeFromStateTransition, deriveRecoveryDelayModeFromStateTransition, } from '../../form/utils/state_transition_helpers'; -import { resolveRecoveryStrategy } from '../../form/utils/rule_request_mappers'; +import { isRecoveryEnabled, resolveRecoveryStrategy } from '../../form/utils/rule_request_mappers'; import type { FormValues } from '../../form/types'; const DELAY_IMMEDIATE = 'immediate'; @@ -42,15 +42,19 @@ const mapStateTransition = (formValues: FormValues) => { if (stateTransition?.pendingCount != null) out.pending_count = stateTransition.pendingCount; } - if (recoveryMode === DELAY_IMMEDIATE) { - out.recovering_count = 0; - } else if (recoveryMode !== DELAY_DURATION && stateTransition?.recoveringCount != null) { - out.recovering_count = stateTransition.recoveringCount; - } else if (recoveryMode === DELAY_DURATION) { - if (stateTransition?.recoveringTimeframe != null) - out.recovering_timeframe = stateTransition.recoveringTimeframe; - if (stateTransition?.recoveringCount != null) + // Recovering thresholds are only meaningful when recovery is enabled; emitting them + // while recovery is disabled is inert and rejected by the write API. + if (isRecoveryEnabled(formValues)) { + if (recoveryMode === DELAY_IMMEDIATE) { + out.recovering_count = 0; + } else if (recoveryMode !== DELAY_DURATION && stateTransition?.recoveringCount != null) { out.recovering_count = stateTransition.recoveringCount; + } else if (recoveryMode === DELAY_DURATION) { + if (stateTransition?.recoveringTimeframe != null) + out.recovering_timeframe = stateTransition.recoveringTimeframe; + if (stateTransition?.recoveringCount != null) + out.recovering_count = stateTransition.recoveringCount; + } } return Object.keys(out).length ? out : undefined; diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx index 61e7ea24b6597..f3f49a181dca6 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx @@ -160,6 +160,7 @@ describe('AlertDelayField', () => { { wrapper: createFormWrapper({ kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'breaches', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx index b143d3256fc47..1c1367fa7e515 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx @@ -160,6 +160,7 @@ describe('RecoveryDelayField', () => { { wrapper: createFormWrapper({ kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'breaches', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts index fe2bbb3cbe924..e3181fdd83e2a 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts @@ -91,7 +91,6 @@ describe('rule_request_mappers', () => { expect(result.state_transition).toEqual({ pending_count: 3, pending_timeframe: '10m', - recovering_count: 0, }); }); @@ -106,7 +105,7 @@ describe('rule_request_mappers', () => { const result = mapFormValuesToRuleRequest(formValues); - expect(result.state_transition).toEqual({ pending_count: 5, recovering_count: 0 }); + expect(result.state_transition).toEqual({ pending_count: 5 }); expect(result.state_transition).not.toHaveProperty('pending_timeframe'); }); @@ -122,10 +121,11 @@ describe('rule_request_mappers', () => { expect(result.state_transition).toBeUndefined(); }); - it('emits pending_count: 0 and recovering_count: 0 for alert kind when both modes are immediate', () => { + it('emits pending_count: 0 and recovering_count: 0 for an alert with recovery enabled when both modes are immediate', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransition: {}, }; @@ -134,21 +134,49 @@ describe('rule_request_mappers', () => { expect(result.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); }); - it('emits pending_count: 0 and recovering_count: 0 for alert kind when stateTransition is undefined', () => { + it('omits recovering_count for an alert when recovery is disabled and both modes are immediate', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + stateTransition: {}, }; const result = mapFormValuesToRuleRequest(formValues); - expect(result.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + + it('omits recovering_count for an alert when recovery is disabled and stateTransition is undefined', () => { + const formValues: FormValues = { + ...baseFormValues, + kind: 'alert', + }; + + const result = mapFormValuesToRuleRequest(formValues); + + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + + it('omits recovering fields when recovery_strategy is "none" even if recovering values are set', () => { + const formValues: FormValues = { + ...baseFormValues, + kind: 'alert', + recoveryStrategy: 'none', + stateTransitionAlertDelayMode: 'immediate', + stateTransitionRecoveryDelayMode: 'duration', + stateTransition: { recoveringCount: 3, recoveringTimeframe: '5m' }, + }; + + const result = mapFormValuesToRuleRequest(formValues); + + expect(result.state_transition).toEqual({ pending_count: 0 }); }); it('emits pending_count: 0 when alert delay mode is immediate even if pendingCount is stale', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'immediate', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { @@ -169,6 +197,7 @@ describe('rule_request_mappers', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'immediate', stateTransitionRecoveryDelayMode: 'duration', stateTransition: { recoveringCount: 4, recoveringTimeframe: '15m' }, @@ -187,6 +216,7 @@ describe('rule_request_mappers', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'immediate', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { recoveringCount: 3 }, @@ -202,6 +232,7 @@ describe('rule_request_mappers', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'breaches', stateTransitionRecoveryDelayMode: 'duration', stateTransition: { @@ -1006,10 +1037,11 @@ describe('rule_request_mappers', () => { breach: { query: 'FROM logs-* | STATS count() BY host' }, }); expect(createPayload.grouping).toEqual({ fields: ['host.name'] }); + // baseRuleResponse has no recovery_strategy, so recovery is disabled and the + // inert recovering_count is not emitted. expect(createPayload.state_transition).toEqual({ pending_count: 3, pending_timeframe: '10m', - recovering_count: 0, }); }); }); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts index 8e9a276e9e54d..b3572bc087435 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts @@ -50,6 +50,21 @@ export const resolveRecoveryStrategy = ( return formValues.query.recovery != null ? ('query' as const) : undefined; }; +/** + * Recovery is enabled only for condition-based strategies (`no_breach` / `query`). + * `none`/unset means condition-based recovery never fires, so the recovering delay + * thresholds (`recovering_count` / `recovering_timeframe`) are inert and must not be + * emitted — the write API rejects them (see `isRecoveryTransitionConsistentWithStrategy`). + * `no_data_strategy: 'recover'` does NOT re-enable them: the director bypasses recovering + * gating for no-data recovery (see `count_timeframe_strategy`). + */ +export const isRecoveryEnabled = ( + formValues: Pick +): boolean => { + const strategy = resolveRecoveryStrategy(formValues); + return strategy != null && strategy !== 'none'; +}; + // --------------------------------------------------------------------------- // FormValues → API request // --------------------------------------------------------------------------- @@ -95,16 +110,20 @@ const mapStateTransition = (formValues: FormValues) => { } } - if (recoveryMode === DELAY_MODE.immediate) { - out.recovering_count = 0; - } else if (recoveryMode !== DELAY_MODE.duration && stateTransition?.recoveringCount != null) { - out.recovering_count = stateTransition.recoveringCount; - } else if (recoveryMode === DELAY_MODE.duration) { - if (stateTransition?.recoveringTimeframe != null) { - out.recovering_timeframe = stateTransition.recoveringTimeframe; - } - if (stateTransition?.recoveringCount != null) { + // Recovering thresholds are only meaningful when recovery is enabled; emitting them + // while recovery is disabled is inert and rejected by the write API. + if (isRecoveryEnabled(formValues)) { + if (recoveryMode === DELAY_MODE.immediate) { + out.recovering_count = 0; + } else if (recoveryMode !== DELAY_MODE.duration && stateTransition?.recoveringCount != null) { out.recovering_count = stateTransition.recoveringCount; + } else if (recoveryMode === DELAY_MODE.duration) { + if (stateTransition?.recoveringTimeframe != null) { + out.recovering_timeframe = stateTransition.recoveringTimeframe; + } + if (stateTransition?.recoveringCount != null) { + out.recovering_count = stateTransition.recoveringCount; + } } } From d34739c8341eef9de9ed439352f0094f930d77f4 Mon Sep 17 00:00:00 2001 From: Alexandra Doak Date: Tue, 8 Sep 2026 14:17:27 -0700 Subject: [PATCH 4/8] Fixing check after merge with main --- .../compose_discover_form/recovery_condition_step.test.tsx | 6 +++--- .../compose_discover_form/recovery_condition_step.tsx | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx index 36a3e57333752..040a6b1519813 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx @@ -127,19 +127,19 @@ describe('RecoveryConditionStep', () => { }); it('renders the recovery delay field when recovery type is default', () => { - renderRecoveryStep({ recoveryType: 'default' }); + renderRecoveryStep('no_breach'); expect(screen.getByTestId('recoveryDelayFormRow')).toBeInTheDocument(); }); it('renders the recovery delay field when recovery type is custom', () => { - renderRecoveryStep({ recoveryType: 'custom' }, CUSTOM_RECOVERY_QUERY); + renderRecoveryStep('query', {}, CUSTOM_RECOVERY_QUERY); expect(screen.getByTestId('recoveryDelayFormRow')).toBeInTheDocument(); }); it('hides the recovery delay field when recovery type is none (delay is inert)', () => { - renderRecoveryStep({ recoveryType: 'none' }); + renderRecoveryStep('none'); expect(screen.queryByTestId('recoveryDelayFormRow')).not.toBeInTheDocument(); }); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx index 5be771e887542..e3009b8783f65 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx @@ -157,7 +157,7 @@ export function RecoveryConditionStep({ * hide it. `no_data_strategy: 'recover'` does not re-enable it — the director * bypasses recovering gating for no-data recovery. */} - {state.recoveryType !== 'none' && ( + {recoveryStrategy !== 'none' && ( <> From f26d7c58164b66294debebec46d101a319d02c3f Mon Sep 17 00:00:00 2001 From: Alexandra Doak Date: Tue, 8 Sep 2026 15:04:31 -0700 Subject: [PATCH 5/8] Fixing test failures --- .../test/scout_alerting_v2/common/builders.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts index bf4206e920d15..d7b2f78f1d385 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts @@ -33,6 +33,10 @@ import { LOOKBACK_WINDOW, SCHEDULE_INTERVAL } from './constants'; * recovery strategies on `kind: 'signal'`. Tests that override `query` * should include `recovery_strategy: 'no_breach'` (or another valid * strategy) if they want the executor to emit recovery events. + * - When a caller disables recovery (`recovery_strategy: 'none'` or `undefined`) + * without supplying its own `state_transition`, `buildCreateRuleData` strips + * the default `recovering_count`/`recovering_timeframe`, since the write API + * rejects an inert recovering delay when recovery is off. */ const DEFAULTS: CreateRuleData = { kind: 'alert', @@ -56,10 +60,19 @@ const ACTION_POLICY_DEFAULTS: CreateActionPolicyDataInput = { export type BuildCreateRuleDataInput = Partial; -export const buildCreateRuleData = (input: BuildCreateRuleDataInput = {}): CreateRuleData => ({ - ...DEFAULTS, - ...input, -}); +export const buildCreateRuleData = (input: BuildCreateRuleDataInput = {}): CreateRuleData => { + const merged: CreateRuleData = { ...DEFAULTS, ...input }; + + // The write API rejects a recovering delay when recovery is off, + // so drop the default one unless the caller set its own `state_transition`. + const recoveryEnabled = merged.recovery_strategy != null && merged.recovery_strategy !== 'none'; + if (!recoveryEnabled && input.state_transition === undefined && merged.state_transition != null) { + const { recovering_count, recovering_timeframe, ...rest } = merged.state_transition; + merged.state_transition = rest; + } + + return merged; +}; export const buildRuleTemplateData = (rule: BuildCreateRuleDataInput = {}): RuleTemplateData => ({ engine: 'v2', From e95a043e16c5331a1dcd23bd70bc6a17095d5d5f Mon Sep 17 00:00:00 2001 From: Alexandra Doak Date: Fri, 11 Sep 2026 11:40:00 -0700 Subject: [PATCH 6/8] Removing comments --- .../compose_discover_form/recovery_condition_step.tsx | 6 ------ .../flyout/compose_discover/compose_mappers.ts | 2 -- .../form/utils/rule_request_mappers.ts | 8 -------- 3 files changed, 16 deletions(-) diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx index e3009b8783f65..79645716cce9b 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx @@ -151,12 +151,6 @@ export function RecoveryConditionStep({ )} - {/* - * Recovery delay only applies to condition-based recovery. When recovery is - * disabled ('none'), the delay is inert (and rejected by the write API), so we - * hide it. `no_data_strategy: 'recover'` does not re-enable it — the director - * bypasses recovering gating for no-data recovery. - */} {recoveryStrategy !== 'none' && ( <> diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts index b0257a207d8e4..39c603457c0d4 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts @@ -42,8 +42,6 @@ const mapStateTransition = (formValues: FormValues) => { if (stateTransition?.pendingCount != null) out.pending_count = stateTransition.pendingCount; } - // Recovering thresholds are only meaningful when recovery is enabled; emitting them - // while recovery is disabled is inert and rejected by the write API. if (isRecoveryEnabled(formValues)) { if (recoveryMode === DELAY_IMMEDIATE) { out.recovering_count = 0; diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts index b3572bc087435..d1ca838f81e15 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts @@ -50,14 +50,6 @@ export const resolveRecoveryStrategy = ( return formValues.query.recovery != null ? ('query' as const) : undefined; }; -/** - * Recovery is enabled only for condition-based strategies (`no_breach` / `query`). - * `none`/unset means condition-based recovery never fires, so the recovering delay - * thresholds (`recovering_count` / `recovering_timeframe`) are inert and must not be - * emitted — the write API rejects them (see `isRecoveryTransitionConsistentWithStrategy`). - * `no_data_strategy: 'recover'` does NOT re-enable them: the director bypasses recovering - * gating for no-data recovery (see `count_timeframe_strategy`). - */ export const isRecoveryEnabled = ( formValues: Pick ): boolean => { From c6ea536be95e2b168ccf9a19d151e7e665197c66 Mon Sep 17 00:00:00 2001 From: Alexi Doak <109488926+doakalexi@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:41:08 -0700 Subject: [PATCH 7/8] Update x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts Co-authored-by: Bailey Cash --- .../shared/alerting_v2/server/lib/errors/error_codes.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts index 302ee40f73dc4..86aa507776f11 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts @@ -35,12 +35,6 @@ export const ALERTING_ERROR_CODES = { INVALID_ARTIFACT_DATA: 'INVALID_ARTIFACT_DATA', /** `state_transition` cannot be applied to the rule's `kind`. */ INVALID_STATE_TRANSITION: 'INVALID_STATE_TRANSITION', - /** - * A rule's merged shape carries a recovery delay - * (`state_transition.recovering_count` / `recovering_timeframe`) while - * recovery is disabled (`recovery_strategy` is `none` or unset), where it - * would never take effect. - */ INVALID_STATE_TRANSITION_CONFIG: 'INVALID_STATE_TRANSITION_CONFIG', /** A signal rule's merged shape violates signal constraints. */ INVALID_SIGNAL_RULE: 'INVALID_SIGNAL_RULE', From 684bc243015dcddab5cc8802276eee6c373ddc1f Mon Sep 17 00:00:00 2001 From: Alexi Doak <109488926+doakalexi@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:41:18 -0700 Subject: [PATCH 8/8] Update x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts Co-authored-by: Bailey Cash --- .../alerting_v2/test/scout_alerting_v2/common/builders.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts index d7b2f78f1d385..dc8304d2ed9cc 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts @@ -63,8 +63,6 @@ export type BuildCreateRuleDataInput = Partial; export const buildCreateRuleData = (input: BuildCreateRuleDataInput = {}): CreateRuleData => { const merged: CreateRuleData = { ...DEFAULTS, ...input }; - // The write API rejects a recovering delay when recovery is off, - // so drop the default one unless the caller set its own `state_transition`. const recoveryEnabled = merged.recovery_strategy != null && merged.recovery_strategy !== 'none'; if (!recoveryEnabled && input.state_transition === undefined && merged.state_transition != null) { const { recovering_count, recovering_timeframe, ...rest } = merged.state_transition;