x-pack/platform/packages/shared/agent-builder/agent-builder-server/skills/internal.ts (1)

62-65: Consider using camelCase for the property name.

The property plugin_id uses snake_case, but the coding guidelines specify camelCase for object keys. If this naming is intentional for consistency with persistence or API contracts across the codebase, please disregard.

♻️ Suggested naming change

   /**
-   * If this skill was installed from a plugin, the plugin name.
+   * If this skill was installed from a plugin, the plugin ID.
    */
-  plugin_id?: string;
+  pluginId?: string;

As per coding guidelines: "Use camelCase for functions, variables, and object keys."

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/packages/shared/agent-builder/agent-builder-server/skills/internal.ts`
around lines 62 - 65, The property plugin_id should be renamed to camelCase
pluginId across the code to follow coding guidelines; update the declaration in
internal.ts (replace plugin_id with pluginId), then update all references/usages
(type annotations, object literals, destructuring, assignments) to use pluginId
and adjust any serialization/deserialization or API mapping code that expects
snake_case (add mapping from plugin_id → pluginId or update serializers) and
update affected tests/types to preserve backward compatibility where needed.

x-pack/platform/packages/shared/agent-builder/agent-builder-common/plugins/plugin_definition.ts (1)

21-57: Make the plugin DTOs readonly.

These interfaces represent persisted, read-only plugin state, but all of the arrays are mutable today. Mark the exported fields/arrays as readonly/ReadonlyArray so consumers cannot accidentally mutate skill_ids or unmanaged_assets in memory.

As per coding guidelines, "Prefer readonly and as const for immutable structures in TypeScript."

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/packages/shared/agent-builder/agent-builder-common/plugins/plugin_definition.ts`
around lines 21 - 57, The DTOs are mutable; make UnmanagedPluginAssets,
PluginManifestMetadata, and PluginDefinition use readonly fields and
ReadonlyArray for all arrays so consumers cannot mutate them: mark each property
in UnmanagedPluginAssets (commands, agents, hooks, mcp_servers, output_styles,
lsp_servers) as readonly ReadonlyArray<string>, mark
PluginManifestMetadata.keywords as readonly ReadonlyArray<string> (and other
optional fields readonly), and update PluginDefinition to use readonly for all
top-level properties (id, name, version, description, manifest, source_url,
skill_ids as ReadonlyArray<string>, unmanaged_assets, created_at, updated_at) so
the DTOs are fully immutable in memory.

x-pack/platform/plugins/shared/agent_builder/server/services/skills/utils.ts (1)

15-30: Omit plugin_id when it is unset.

Right now this helper always creates the plugin_id key, so plain-object consumers still see it as present with undefined. If the public contract is “only include this for plugin-managed skills,” build it conditionally instead.

♻️ Proposed change

 export const internalToPublicDefinition = async (
   skill: InternalSkillDefinition
 ): Promise<PublicSkillDefinition> => ({
   id: skill.id,
   name: skill.name,
   description: skill.description,
   content: skill.content,
   referenced_content: skill.referencedContent?.map((rc) => ({
     name: rc.name,
     relativePath: rc.relativePath,
     content: rc.content,
   })),
   tool_ids: await skill.getRegistryTools(),
   readonly: skill.readonly,
-  plugin_id: skill.plugin_id,
+  ...(skill.plugin_id !== undefined ? { plugin_id: skill.plugin_id } : {}),
 });

🤖 Prompt for AI Agents

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

In `@x-pack/platform/plugins/shared/agent_builder/server/services/skills/utils.ts`
around lines 15 - 30, internalToPublicDefinition always includes the plugin_id
key even when undefined; change it to only add plugin_id when skill.plugin_id is
set (e.g., check for null/undefined) so the returned plain object omits the key
for non-plugin-managed skills—update the internalToPublicDefinition return
construction to conditionally include plugin_id (keeping the rest of the fields
and tool_ids await logic intact).

x-pack/platform/test/agent_builder_api_integration/apis/plugins/index.ts (1)

10-14: Add an explicit return type and prefer a typed const export here.

This entrypoint is new exported TS code, so I’d annotate it with : void and switch to the preferred const-arrow form.

♻️ Proposed change

-export default function ({ loadTestFile }: FtrProviderContext) {
+const registerPluginsApiTests = ({ loadTestFile }: FtrProviderContext): void => {
   describe('Plugins API', function () {
     loadTestFile(require.resolve('./installation'));
   });
-}
+};
+
+export default registerPluginsApiTests;

As per coding guidelines, "Prefer explicit return types for public APIs and exported functions in TypeScript" and "Prefer const arrow functions in TypeScript."

🤖 Prompt for AI Agents

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

In `@x-pack/platform/test/agent_builder_api_integration/apis/plugins/index.ts`
around lines 10 - 14, The exported unnamed function lacks an explicit return
type and should be a typed const-arrow export; change the default export to a
named const arrow like e.g. const pluginsApi = ({ loadTestFile }:
FtrProviderContext): void => { describe('Plugins API', function () {
loadTestFile(require.resolve('./installation')); }); }; and then export default
pluginsApi; ensuring the parameter is typed with FtrProviderContext and the
function return type is explicitly void.

x-pack/platform/plugins/shared/agent_builder/server/services/skills/skill_service.test.ts (1)

30-33: Give the new persisted-client mocks async defaults.

bulkCreate and deleteByPluginId are async on the real client. Leaving them as bare jest.fn() makes the default mock return undefined, which can hide contract mismatches or produce misleading failures once these paths are exercised.

♻️ Proposed fix

-    bulkCreate: jest.fn(),
+    bulkCreate: jest.fn().mockResolvedValue([]),
...
-    deleteByPluginId: jest.fn(),
+    deleteByPluginId: jest.fn().mockResolvedValue(undefined),

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/plugins/shared/agent_builder/server/services/skills/skill_service.test.ts`
around lines 30 - 33, The mocks for the persisted-client need async defaults:
change the plain jest.fn() for bulkCreate and deleteByPluginId to resolved mocks
so they return a Promise (e.g., jest.fn().mockResolvedValue(undefined) or an
appropriate resolved value) instead of undefined synchronously; update the mock
declarations for bulkCreate and deleteByPluginId in the test file so they use
mockResolvedValue(...) while leaving update and delete as-is.

x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/sourcing/download_plugin.test.ts (1)

11-19: Replace the unknown[] passthrough mocks with typed signatures.

These mocks work, but they lose the openZipArchive / parsePluginZipFile contract and introduce new unknown usage in TS test code. Typing the forwarded parameters directly keeps the mock surface aligned with the real functions.

♻️ Proposed fix

 jest.mock('../archive', () => ({
-  openZipArchive: (...args: unknown[]) => mockOpenZipArchive(...args),
+  openZipArchive: (filePath: string) => mockOpenZipArchive(filePath),
 }));
 
 const mockParsePluginZipFile = jest.fn();
 jest.mock('../parsing', () => ({
-  parsePluginZipFile: (...args: unknown[]) => mockParsePluginZipFile(...args),
+  parsePluginZipFile: (archive: ZipArchive) => mockParsePluginZipFile(archive),
   PluginArchiveError: class extends Error {},
 }));

As per coding guidelines, `**/*.{ts,tsx}`: 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/plugins/shared/agent_builder/server/services/plugins/utils/sourcing/download_plugin.test.ts`
around lines 11 - 19, The test mocks currently forward parameters as (...args:
unknown[]) which loses the real function signatures for openZipArchive and
parsePluginZipFile; replace the unknown[] passthroughs with the actual parameter
types by using Parameters<typeof openZipArchive> and Parameters<typeof
parsePluginZipFile> (or import the functions and use their parameter types) when
defining the mock factories so the mockOpenZipArchive and mockParsePluginZipFile
preserve the real contracts; keep the PluginArchiveError class export as-is.

x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/archive/create_scoped_archive.test.ts (1)

11-23: Consider extracting createMockArchive into a shared test helper.

The same ZipArchive test double is duplicated in x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/parsing/parse_plugin_zip_file.test.ts at Line 11-Line 23. Pulling it into one helper would make future interface changes less error-prone.

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/archive/create_scoped_archive.test.ts`
around lines 11 - 23, Extract the duplicated ZipArchive test double into a
single test helper by moving the createMockArchive function (which returns an
object implementing hasEntry, getEntryPaths, getEntryContent and close) into a
shared test utilities module and replace the local definitions in both tests
with imports of that helper; ensure the helper exports createMockArchive and
uses the same function signature (files: Record<string,string>) so existing
tests referencing createMockArchive, ZipArchive, getEntryContent, hasEntry and
close continue to work without further changes.

x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/archive/open_zip_archive.test.ts (2)

14-18: Let the test helper generate raw-byte fixtures.

ZipArchive.getEntryContent() returns Buffer, but this helper only builds UTF-8 text entries. That means the “byte-for-byte” case still passes if the implementation accidentally round-trips through strings.

♻️ Suggested change

-const createTestZip = async (files: Record<string, string>, destPath: string): Promise<void> => {
+const createTestZip = async (
+  files: Record<string, string | Buffer>,
+  destPath: string
+): Promise<void> => {
   return new Promise((resolve, reject) => {
     const zipFile = new yazl.ZipFile();
     for (const [filePath, content] of Object.entries(files)) {
-      zipFile.addBuffer(Buffer.from(content, 'utf-8'), filePath);
+      zipFile.addBuffer(
+        typeof content === 'string' ? Buffer.from(content, 'utf-8') : content,
+        filePath
+      );
     }

Then use one binary fixture in the byte-preservation test, e.g. Buffer.from([0x00, 0xff, 0x0a]).

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/archive/open_zip_archive.test.ts`
around lines 14 - 18, The helper createTestZip currently encodes every entry as
UTF-8 text which prevents true byte-for-byte tests; modify createTestZip so it
accepts file contents as either strings or Buffers and when adding entries to
the yazl.ZipFile call zipFile.addBuffer(...) with the raw Buffer when the value
is a Buffer, otherwise convert strings with Buffer.from(value, 'utf-8');
reference createTestZip and ZipArchive.getEntryContent in the test and then add
a binary fixture like Buffer.from([0x00, 0xff, 0x0a]) to the byte-preservation
test so the test verifies exact bytes are preserved.

---

98-104: Assert the missing-entry path as a rejected promise.

getEntryContent() is a Promise<Buffer> API, but toThrow() only covers synchronous exceptions. This test currently locks in a sync-throw implementation detail instead of the async contract.

✅ Suggested change

-    archive = await openZipArchive(zipPath);
-
-    expect(() => archive!.getEntryContent('does-not-exist.txt')).toThrow(/not found in archive/);
+    const zipArchive = await openZipArchive(zipPath);
+    archive = zipArchive;
+
+    await expect(
+      Promise.resolve().then(() => zipArchive.getEntryContent('does-not-exist.txt'))
+    ).rejects.toThrow(/not found in archive/);

Run this to confirm the contract and current implementation shape:

#!/bin/bash
set -euo pipefail

file="$(fd 'open_zip_archive\.ts$' x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/archive | head -n1)"
rg -n -C4 '\binterface ZipArchive\b|\bgetEntryContent\s*\(' "$file"

Expected: the contract shows Promise<Buffer>; if the current implementation throws before returning a promise, the wrapper above still keeps the test aligned with the public async API.

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/plugins/shared/agent_builder/server/services/plugins/utils/archive/open_zip_archive.test.ts`
around lines 98 - 104, The test is asserting a synchronous throw but
getEntryContent() returns a Promise; change the assertion to expect the promise
to reject: call archive!.getEntryContent('does-not-exist.txt') and use Jest's
.rejects.toThrow(/not found in archive/) (keeping the same regex) so the test
verifies the async contract of getEntryContent from openZipArchive; update the
test invocation accordingly to await or return the expect(...).rejects chain.

oas_docs/output/kibana.yaml (2)

3236-3245: Code sample placeholders should match the parameter name.

The path parameter is named pluginId, but the code samples use {id} as the placeholder. For clarity and consistency, consider using {pluginId} in the examples.

📝 Suggested fix

       x-codeSamples:
         - lang: curl
           source: |
             curl \
-              -X DELETE "${KIBANA_URL}/api/agent_builder/plugins/{id}" \
+              -X DELETE "${KIBANA_URL}/api/agent_builder/plugins/{pluginId}" \
               -H "Authorization: ApiKey ${API_KEY}" \
               -H "kbn-xsrf: true"
         - lang: Console
           source: |
-            DELETE kbn://api/agent_builder/plugins/{id}
+            DELETE kbn://api/agent_builder/plugins/{pluginId}

🤖 Prompt for AI Agents

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

In `@oas_docs/output/kibana.yaml` around lines 3236 - 3245, The code samples under
x-codeSamples use the placeholder {id} but the OpenAPI path parameter is named
pluginId; update both examples (the curl and Console entries) to use {pluginId}
so the placeholder matches the parameter name (look for x-codeSamples, the curl
"-X DELETE" sample and the Console "DELETE kbn://api/agent_builder/plugins/{id}"
sample and replace {id} with {pluginId}).

---

3303-3311: Code sample placeholders should match the parameter name.

Same issue as the DELETE endpoint - the code samples use {id} but the path parameter is pluginId.

📝 Suggested fix

       x-codeSamples:
         - lang: curl
           source: |
             curl \
-              -X GET "${KIBANA_URL}/api/agent_builder/plugins/{id}" \
+              -X GET "${KIBANA_URL}/api/agent_builder/plugins/{pluginId}" \
               -H "Authorization: ApiKey ${API_KEY}"
         - lang: Console
           source: |
-            GET kbn://api/agent_builder/plugins/{id}
+            GET kbn://api/agent_builder/plugins/{pluginId}

🤖 Prompt for AI Agents

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

In `@oas_docs/output/kibana.yaml` around lines 3303 - 3311, The code samples under
x-codeSamples for the GET operation use the placeholder "{id}" but the path
parameter is named "pluginId"; update the sample sources in the GET
x-codeSamples (both the curl sample and the Console sample) to use "{pluginId}"
so they match the operation parameter name (e.g., curl -X GET
"${KIBANA_URL}/api/agent_builder/plugins/{pluginId}" and GET
kbn://api/agent_builder/plugins/{pluginId}).

x-pack/platform/plugins/shared/agent_builder/server/services/plugins/client/converters.test.ts (1)

84-87: Avoid any in the fixture mutation.

This edge case can be modeled with the helper input instead of dropping type safety. If createPluginDocument({ skill_ids: undefined }) does not type-check under the current compiler flags, widen the helper input rather than casting here.

💡 Suggested fix

-      const doc = createPluginDocument();
-      (doc._source as any).skill_ids = undefined;
+      const doc = createPluginDocument({ skill_ids: undefined });

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/plugins/shared/agent_builder/server/services/plugins/client/converters.test.ts`
around lines 84 - 87, The test is mutating the fixture with an any cast; instead
update the fixture helper to accept an optional skill_ids so you can call
createPluginDocument({ skill_ids: undefined }) instead of casting, or widen the
helper's input type to include skill_ids?: string[] | undefined; modify the
createPluginDocument signature (and its input type/interface) used by the test
so the edge-case can be modeled directly and remove the (doc._source as any)
mutation in the test.

x-pack/platform/plugins/shared/agent_builder/server/services/plugins/plugin_service.ts (1)

22-23: Remove the eslint-disable comment and address the empty interface.

Per coding guidelines, linting errors should not be suppressed. Consider using a type alias or adding a placeholder comment property if the interface will be extended later.

♻️ Suggested fix

-// eslint-disable-next-line `@typescript-eslint/no-empty-interface`
-export interface PluginsServiceSetup {}
+export type PluginsServiceSetup = Record<string, never>;

Alternatively, if the interface will gain members soon, add a TODO comment explaining the planned additions.

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/plugins/shared/agent_builder/server/services/plugins/plugin_service.ts`
around lines 22 - 23, Remove the eslint-disable comment and address the empty
interface PluginsServiceSetup: either convert the empty interface to an
equivalent type alias (e.g., type PluginsServiceSetup = {}) or keep the
interface but add a self-documenting placeholder (a TODO comment plus a
non-exported placeholder property or JSDoc indicating future members) so linting
is satisfied without suppressing the rule; update the declaration for
PluginsServiceSetup accordingly and ensure no eslint-disable comment remains.

x-pack/platform/plugins/shared/agent_builder/server/services/plugins/client/client.ts (1)

150-153: Avoid non-null assertion on _source.

Per coding guidelines, non-null assertions (!) should be avoided unless locally justified. The _source field could theoretically be undefined. Add a guard to handle this case.

♻️ Suggested fix

   async update(pluginId: string, update: PluginUpdateRequest): Promise<PersistedPluginDefinition> {
     const document = await this._getById(pluginId);
     if (!document) {
       throw createPluginNotFoundError({ pluginId });
     }
+    if (!document._source) {
+      throw createPluginNotFoundError({ pluginId });
+    }

     const updatedAttributes = updateRequestToEs({
-      current: document._source!,
+      current: document._source,
       update,
     });

🤖 Prompt for AI Agents

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

In
`@x-pack/platform/plugins/shared/agent_builder/server/services/plugins/client/client.ts`
around lines 150 - 153, The code calls updateRequestToEs with document._source
using a non-null assertion; instead, guard against a missing source by checking
document._source before calling updateRequestToEs (e.g., if (!document._source)
{ /* handle: throw a descriptive Error or return a failed Result/log and exit
early */ }), then only call updateRequestToEs({ current: document._source,
update }) when present; update references to updatedAttributes and any
downstream logic to handle the early-failure path accordingly so you don't rely
on the non-null assertion.