x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/rule_generation_evaluators.test.ts (1)

30-33: Consider defensive handling for evaluator lookup.

The non-null assertion (!) assumes the evaluator will always be found. If the evaluator name changes, this would throw at runtime rather than providing a clear error.

🔧 Suggested defensive approach

 const getRuleGenerationCodeEvaluator = (esClient: ElasticsearchClient) =>
-  createRuleGenerationEvaluators(esClient).find(
+  createRuleGenerationEvaluators(esClient).find(
     (evaluator) => evaluator.name === 'rule_generation_code_evaluator'
-  )!;
+  ) ?? (() => { throw new Error('rule_generation_code_evaluator not found'); })();

Alternatively, since this is a test helper, the current approach is acceptable as the test would fail with a clear enough error on first use.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/rule_generation_evaluators.test.ts`
around lines 30 - 33, The helper getRuleGenerationCodeEvaluator uses a non-null
assertion on the result of createRuleGenerationEvaluators(...).find(...), which
will throw a less-descriptive runtime error if the evaluator name is changed or
missing; replace the `!` with an explicit defensive check: locate the creation
call to createRuleGenerationEvaluators(esClient) and the find that compares
evaluator.name === 'rule_generation_code_evaluator' in
getRuleGenerationCodeEvaluator, capture the result in a variable, and if it's
undefined throw a clear Error (e.g., "rule_generation_code_evaluator not found
in createRuleGenerationEvaluators") so tests fail with a descriptive message.

x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/ki_extraction_evaluators.test.ts (1)

12-17: Consider adding explicit existence checks for evaluators.

The non-null assertions (!) on lines 12 and 15 could mask issues if evaluator names change. Consider adding explicit checks:

Safer evaluator lookup

-const evidenceGroundingEvaluator = createKIExtractionEvaluators().find(
-  (evaluator) => evaluator.name === 'evidence_grounding'
-);
-const kiCountEvaluator = createKIExtractionEvaluators().find(
-  (evaluator) => evaluator.name === 'ki_count'
-);
+const evaluators = createKIExtractionEvaluators();
+
+const evidenceGroundingEvaluator = evaluators.find(
+  (evaluator) => evaluator.name === 'evidence_grounding'
+);
+const kiCountEvaluator = evaluators.find(
+  (evaluator) => evaluator.name === 'ki_count'
+);
+
+if (!evidenceGroundingEvaluator || !kiCountEvaluator) {
+  throw new Error('Required evaluators not found - check evaluator names');
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/ki_extraction_evaluators.test.ts`
around lines 12 - 17, The test currently uses non-null assertions for
evidenceGroundingEvaluator and kiCountEvaluator returned from
createKIExtractionEvaluators(), which can hide failures if evaluator names
change; update the test to explicitly check that
createKIExtractionEvaluators().find(...) returned a value for each evaluator
(evidenceGroundingEvaluator and kiCountEvaluator) and fail early with a clear
message (e.g., throw or expect) if not found, before using them in later
assertions so the test errors point to the missing evaluator by name.

x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/rule_generation_evaluators.ts (1)

29-76: Code duplication with ki_extraction_evaluators.ts.

The SHORT_EVIDENCE_MAX_LENGTH, escapeRegExp, and the short-evidence word-boundary matching pattern are duplicated between this file and ki_extraction_evaluators.ts (lines 150-169). Consider extracting to a shared utility.

Suggested shared utility

// src/evaluators/evidence_matching.ts
export const SHORT_EVIDENCE_MAX_LENGTH = 3;

export function escapeRegExp(value: string): string {
  return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
}

export function matchesShortEvidence(text: string, evidence: string): boolean {
  const normalized = evidence.trim();
  if (normalized.length === 0) return false;
  
  if (normalized.length <= SHORT_EVIDENCE_MAX_LENGTH) {
    return new RegExp(`(^|[^a-zA-Z0-9_])${escapeRegExp(normalized)}($|[^a-zA-Z0-9_])`).test(text);
  }
  return text.includes(normalized);
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/rule_generation_evaluators.ts`
around lines 29 - 76, Extract duplicated logic (SHORT_EVIDENCE_MAX_LENGTH,
escapeRegExp, and the short-evidence word-boundary matching) into a shared
utility module (e.g., export SHORT_EVIDENCE_MAX_LENGTH, escapeRegExp, and a
matchesShortEvidence function) and replace the local implementations in
rule_generation_evaluators.ts (functions/constants: SHORT_EVIDENCE_MAX_LENGTH,
escapeRegExp, matchesEvidenceInLog) with imports from that utility; update
matchesEvidenceInLog to delegate to the new matchesShortEvidence (or replace
usages directly) and add appropriate imports so behavior remains identical.

x-pack/platform/packages/shared/kbn-evals-suite-streams/src/data_generators/load_kis_from_snapshot.test.ts (1)

23-40: Consider using typed mocks instead of as any casts.

The test file uses multiple as any casts for mocks (lines 28, 40, and throughout). While this is common in test code, it could mask type mismatches. Consider creating partial mock types:

Example typed mock approach

+type MockToolingLog = Pick<ToolingLog, 'info' | 'debug' | 'warning' | 'error'>;
+
 const log = {
   info: jest.fn(),
   debug: jest.fn(),
   warning: jest.fn(),
   error: jest.fn(),
-} as any;
+} as MockToolingLog;

As per coding guidelines: "Use TypeScript for all new code; avoid any and unknown."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/src/data_generators/load_kis_from_snapshot.test.ts`
around lines 23 - 40, Replace the untyped `as any` mocks with typed
partial/mocked types to preserve compile-time checks: define `log` as a
`jest.Mocked<YourLoggerType>` (or `Partial<YourLoggerType>` with jest.fn() for
the methods) instead of `as any`, type `gcs` with its proper interface/type, and
change `makeEsClient` to return `Partial<Client>` or `jest.Mocked<Client>` (use
the real `Client` type from the ES client) so the `indices.delete` and `search`
mocks are correctly typed; update all occurrences (references: `log`, `gcs`,
`makeEsClient`, and the `Client` type) to use these typed mocks rather than `as
any`.

x-pack/platform/packages/shared/kbn-evals-suite-streams/evals/significant_events/datasets/types.ts (1)

10-10: Keep DatasetConfig independent from evaluator code.

datasets/types.ts is the shared contract for datasets and specs, but ValidKIType now comes from src/evaluators/ki_extraction_evaluators. That couples a public schema module to a concrete evaluator implementation. Moving the KI-type union into a neutral shared types module would keep the dependency direction cleaner and reduce the chance of cycles later.

Also applies to: 41-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/evals/significant_events/datasets/types.ts`
at line 10, DatasetConfig in datasets/types.ts currently depends on ValidKIType
from src/evaluators/ki_extraction_evaluators which couples the shared dataset
contract to a concrete evaluator; extract the KI-type union into a neutral
shared types module (e.g., create/export a KiType or ValidKIType in a shared
package used by both datasets and evaluators), update datasets/types.ts to
import the KI union from that new shared module instead of
src/evaluators/ki_extraction_evaluators, and update the evaluator implementation
to import the same shared type so DatasetConfig (and any other shared
interfaces) remain independent of evaluator code; ensure symbols involved are
DatasetConfig and ValidKIType (or the new shared KiType) so references are
updated consistently.

x-pack/platform/packages/shared/kbn-evals-suite-streams/evals/significant_events/datasets/otel_demo.ts (1)

381-624: Only two rule scenarios opt into the new esql_substrings check.

The new evaluator tightening only applies this grounding check when a scenario defines esql_substrings, but in this dataset that is currently limited to payment-unreachable and cart-redis-cutoff. Adding representative substrings for the remaining rule-generation scenarios would make the new behavior exercise the full matrix instead of just those two cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/evals/significant_events/datasets/otel_demo.ts`
around lines 381 - 624, The ruleGeneration scenarios only include
esql_substrings for payment-unreachable and cart-redis-cutoff, so add
representative esql_substrings arrays to the other scenario objects to exercise
the new evaluator check; update the ruleGeneration entries for scenario_id
values healthy-baseline, currency-unreachable, checkout-memory-starvation,
flagd-unreachable, and load-generator-ramp to include relevant substrings (e.g.,
healthy-baseline: ['throughput','latency','service'], currency-unreachable:
['currency','conversion','unreachable'], checkout-memory-starvation:
['OOMKilled','memory','GC pressure'], flagd-unreachable:
['flagd','feature-flag','evaluation'], load-generator-ramp: ['latency','error
rate','throughput']) so the evaluator sees grounding text across all scenarios.

x-pack/platform/packages/shared/kbn-evals-suite-streams/evals/significant_events/rule_generation/rule_generation.spec.ts (1)

91-96: Consider adding debug logging for cleanup failures.

The empty catch blocks silently swallow errors during cleanup. While this is acceptable for cleanup operations that may legitimately fail (e.g., non-existent streams), adding debug logging would help diagnose issues when cleanup unexpectedly fails.

♻️ Suggested improvement

           for (const name of ['logs.otel', 'logs.ecs']) {
-            await esClient.indices.deleteDataStream({ name }).catch(() => {});
+            await esClient.indices.deleteDataStream({ name }).catch((e) => {
+              log.debug(`Failed to delete data stream ${name}: ${e.message}`);
+            });
             await esClient.indices
               .delete({ index: name, ignore_unavailable: true })
-              .catch(() => {});
+              .catch((e) => {
+                log.debug(`Failed to delete index ${name}: ${e.message}`);
+              });
           }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/evals/significant_events/rule_generation/rule_generation.spec.ts`
around lines 91 - 96, The cleanup loop currently swallows errors silently in the
empty catch blocks around esClient.indices.deleteDataStream and
esClient.indices.delete; update those catches to log the caught error at debug
(or trace) level instead of doing nothing: capture the error in each catch and
call the test-suite logger (or console.debug if no logger is available) with a
short context message referencing deleteDataStream/delete and the error object
so unexpected failures during cleanup are visible without breaking tests.

x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/rule_generation_evaluators.ts (1)

96-112: Consider adding a timeout for ES|QL query validation to prevent test hangs.

The esClient.esql.query call on line 102 lacks a timeout, which could cause test runs to hang on malformed or expensive queries. Per the Elasticsearch Node.js client API, you can add a requestTimeout in the options parameter (2nd argument).

♻️ Suggested improvement

     try {
-      const result = await esClient.esql.query({ query: esql });
+      const result = await esClient.esql.query({ query: esql }, { requestTimeout: 120000 });
       isSyntaxValid = true;

Tune the timeout value (120000ms shown above) based on expected query complexity and your CI environment—shorter timeouts may be suitable for simple queries, but complex ES|QL queries may need the longer window to execute.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@x-pack/platform/packages/shared/kbn-evals-suite-streams/src/evaluators/rule_generation_evaluators.ts`
around lines 96 - 112, The ES|QL validation call in the loop uses
esClient.esql.query without a timeout which can hang tests; update the
esClient.esql.query invocation (in the same block that sets
isSyntaxValid/isExecutionHit and increments validSyntaxCount/executionHitCount)
to pass a second options argument with a requestTimeout (e.g., 120000) so slow
or stuck queries fail fast, and keep the existing try/catch to log the error as
before.