[Obs Ai Assistant] Add system message

[arturoliduena]

Fix: System Message Missing in Inference Plugin
Closes #209548

Summary

A regression was introduced in 8.18 (#199286), where the system message is no longer passed to the inference plugin and, consequently, the LLM.

Currently, only user messages are being sent, which impacts conversation guidance and guardrails. The system message is crucial for steering responses and maintaining contextual integrity.

The filtering of the system message happens here:

kibana/x-pack/platform/plugins/shared/observability_ai_assistant/server/service/client/index.ts

Lines 510 to 512 in 771a080

	messages: convertMessagesForInference(
	messages.filter((message) => message.message.role !== MessageRole.System)
	),

Fix Approach

• Ensure the system message is included as a parameter in inferenceClient.chatComplete.

const options = {
      connectorId,
      system,
      messages: convertMessagesForInference(messages),
      toolChoice,
      tools,
      functionCalling: (simulateFunctionCalling ? 'simulated' : 'native') as FunctionCallingMode,
    };
    if (stream) {
      return defer(() =>
        this.dependencies.inferenceClient.chatComplete({
          ...options,
          stream: true,
        })
      ).pipe(
        convertInferenceEventsToStreamingEvents(),
        instrumentAndCountTokens(name),
        failOnNonExistingFunctionCall({ functions }),
        tap((event) => {
          if (
            event.type === StreamingChatResponseEventType.ChatCompletionChunk &&
            this.dependencies.logger.isLevelEnabled('trace')
          ) {
            this.dependencies.logger.trace(`Received chunk: ${JSON.stringify(event.message)}`);
          }
        }),
        shareReplay()
      ) as TStream extends true
        ? Observable<ChatCompletionChunkEvent | TokenCountEvent | ChatCompletionMessageEvent>
        : never;
    } else {
      return this.dependencies.inferenceClient.chatComplete({
        ...options,
        stream: false,
      }) as TStream extends true ? never : Promise<ChatCompleteResponse>;
    }
  }

• Add an API test to verify that the system message is correctly passed to the LLM.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
• This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
• Flaky Test Runner was used on any tests changed
• The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

• See some risk examples
• ...

[elasticmachine]

Pinging @elastic/obs-ai-assistant (Team:Obs AI Assistant)

[arturoliduena]

@sorenlouv On chatComplete, we always replace the system message using replaceSystemMessage with the one generated by getSystemMessageFromInstructions. I've added an argument systemMessage and save it in the conversation but still use the generated system message from getSystemMessageFromInstructions to passing a system to the LLM (inference.chat). I’m not sure if this is the intended behavior, should we always use the provided systemMessage, or is overriding it the correct approach?

[sorenlouv]

I think we should not pass the system message to the complete function because adHocInstructions are updated inside complete.

Instead the system message should be constructed from applicationInstructions, userInstructions and adHocInstructions at the time when they are all available.

[arturoliduena]

That makes sense. So instead of passing a predefined systemMessage to complete, we are always constructing it dynamically using applicationInstructions, userInstructions, and adHocInstructions.

I've removed the systemMessage argument from complete, we are still saving systemMessage in the conversation.

[arturoliduena]

@sorenlouv As you can see here, this was redundant since it gets replaced by the same message.

[arturoliduena]

@sorenlouv - the systemMessage is generated in the UI and then replaced in the chatComplete method. If the system message is needed in the UI, we should clarify whether it should persist or be overridden later. otherwise just remove it:

kibana/x-pack/platform/plugins/shared/observability_ai_assistant/public/service/create_chat_service.ts

Lines 187 to 210 in 8a26cf6

	async initialize() {
	this.functionRegistry = new Map();
	const systemMessages: string[] = [];
	const scopePromise = this.apiClient('GET /internal/observability_ai_assistant/functions', {
	signal: this.abortSignal,
	params: {
	query: {
	scopes: this.getScopes(),
	},
	},
	}).then(({ functionDefinitions, systemMessage }) => {
	functionDefinitions.forEach((fn) => this.functionRegistry.set(fn.name, fn));
	systemMessages.push(systemMessage);
	});
	await Promise.all([
	scopePromise,
	...this.registrations.map((registration) => {
	return registration({
	registerRenderFunction: (name, renderFn) => {
	this.renderFunctionRegistry.set(name, renderFn);
	},
	});
	}),

[dgieselaar]

@arturoliduena the system message is stored in the conversation for debug purposes, we don't strictly require to store it but it's useful when folks copy the conversation.

[sorenlouv]

@arturoliduena the system message is stored in the conversation for debug purposes, we don't strictly require to store it but it's useful when folks copy the conversation.

Could we add a new system (or perhaps systemMessage) field to the conversation index in order to properly separate it from the list of messages? This would align well with the inference plugin.

[arturoliduena]

Could we add a new system (or perhaps systemMessage) field to the conversation index in order to properly separate it from the list of messages? This would align well with the inference plugin.

@sorenlouv The message with system role is removed from the list of messages and a new property systemMessage is stored within the conversation. The same when updating a conversation, ensuring better separation and alignment with the inference plugin.

[dgieselaar]

I like this change - but let's make sure we test this properly. There are likely places where we do things like messages.length > 1 (not great anyway) to see if there are user messages

[arturoliduena]

I am working on updating the current tests and I’ll also add tests to verify the expected behavior.

[sorenlouv]

Thanks @arturoliduena !

[elasticmachine]

⏳ Build in-progress, with failures

• Buildkite Build
• Commit: 66d8a4a1e9307fd3e11857361fb78359109b083c

Failed CI Steps

• Quick Checks
• Check Types

History

• 💔 Build #273567 failed 92a6ce739626f0f86b975a5db99454a1181fe6e5

[sorenlouv]

Would it make sense to move this to getSystemMessageFromInstructions ?

          adHocInstructions.push({
            instruction_type: 'application_instruction',
            text: `This conversation will be persisted in Kibana and available at this url: ${
              kibanaPublicUrl + `/app/observabilityAIAssistant/conversations/${conversationId}`
            }.`,
          });

[arturoliduena]

There are cases where this instruction should not be added to the system message:

kibana/x-pack/platform/plugins/shared/observability_ai_assistant/server/routes/functions/route.ts

Lines 65 to 80 in 652385c

	const functionDefinitions = functionClient.getFunctions().map((fn) => fn.definition);
	const availableFunctionNames = functionDefinitions.map((def) => def.name);
	return {
	functionDefinitions,
	systemMessage: getSystemMessageFromInstructions({
	applicationInstructions: functionClient.getInstructions(),
	userInstructions,
	adHocInstructions: functionClient.getAdhocInstructions(),
	availableFunctionNames,
	}),
	};
	},
	});

[sorenlouv]

I think we can get rid of getAdhocInstructions and registerAdhocInstruction and just use getInstructions and registerInstruction instead.

adhocInstructions should be limited to those instructions coming via the API. And perhaps we should just treat those as user instructions, and get rid of the concept of adhoc instructions entirely.

[github-actions]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

• /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
• run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

[arturoliduena]

Is the LLMProxy used for any of these tests? We shouldn't just mock the calls to the inference plugin. We need to validate that we actually send the system message to the LLM.

@sorenlouv We are already validating that inferenceClient.chatComplete is called twice (once for the title and once for the conversation). Additionally, we verify that each call includes the expected system message. Wouldn’t this be sufficient for the AI Assistant side? Why do we also need to check if it is passed to the LLM?

test: x-pack/platform/plugins/shared/observability_ai_assistant/server/service/client/index.test.ts

cases:

it('calls the llm to generate a new title', () => {
        expect(inferenceClientMock.chatComplete.mock.calls[0]).toEqual([
          expect.objectContaining({
            connectorId: 'foo',
            stream: false,
            system:
              'You are a helpful assistant for Elastic Observability. Assume the following message is the start of a conversation between you and a user; give this conversation a title based on the content below. DO NOT UNDER ANY CIRCUMSTANCES wrap this title in single or double quotes. This title is shown in a list of conversations to the user, so title it for the user, not for you.',
...

      it('calls the llm again with the messages', () => {
        expect(inferenceClientMock.chatComplete.mock.calls[1]).toEqual([
          {
            connectorId: 'foo',
            stream: true,
            system: EXPECTED_STORED_SYSTEM_MESSAGE,
...

[elasticmachine]

⏳ Build in-progress

• Buildkite Build
• Commit: 66b4a12
• Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-209773-66b4a12578e8

History

• 💛 Build #276698 was flaky 9d5b2fd
• 💚 Build #276291 succeeded 8506ad355febf890baae0ecd1ca5736dc8edd518
• 💛 Build #275490 was flaky ae018133ce8dcd2f2f9fdc69fc128e7f05bdc034
• 💔 Build #275437 failed b88af80682acaefcab03a4902e47d68b37ab13ee
• 💔 Build #275278 failed 2cdf1776c58602b09d95f4bec9f999919aad6b92

[kibanamachine]

Starting backport for target branches: 8.x, 9.0

https://github.com/elastic/kibana/actions/runs/13367833759

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.x
✅	9.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

[sorenlouv]

Is the LLMProxy used for any of these tests? We shouldn't just mock the calls to the inference plugin. We need to validate that we actually send the system message to the LLM.

@sorenlouv We are already validating that inferenceClient.chatComplete is called twice (once for the title and once for the conversation). Additionally, we verify that each call includes the expected system message. Wouldn’t this be sufficient for the AI Assistant side? Why do we also need to check if it is passed to the LLM?

@arturoliduena We should also have a test that checks what we send to the LLM. We should not blindly trust the inference plugin. It could change in some way that we do not understand causing it to no longer pass the system message to the LLM - a little bit like what just happened.

It doesn't need to be a big convoluted test - just simply checking that the system message is sent along. A think this is good practice to do for most of what we do since it's so easy with the LLM Proxy