simplify SkillReference, remove `index` field, calculate based on array index by dimaMachina · Pull Request #1947 · inkeep/agents

dimaMachina · 2026-02-12T01:41:37Z

No description provided.

vercel · 2026-02-12T01:41:42Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agents-api	Ready	Preview, Comment	Feb 12, 2026 1:44am
agents-docs	Ready	Preview, Comment	Feb 12, 2026 1:44am
agents-manage-ui	Ready	Preview, Comment	Feb 12, 2026 1:44am

changeset-bot · 2026-02-12T01:41:43Z

⚠️ No Changeset found

Latest commit: 7843a2e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

dimaMachina · 2026-02-12T01:42:04Z

agents-cli/src/commands/pull-v3/components/__tests__/sub-agent-generator.test.ts

      expect(definition).not.toContain('legacyAgent2,');
    });

+    it('should generate skill references without index and keep index-based ordering', () => {


Suggested change

it('should generate skill references without index and keep index-based ordering', () => {

it('should generate skill references and keep index-based ordering', () => {

claude

PR Review Summary

(4) Total Issues | Risk: Medium

🟠⚠️ Major (2) 🟠⚠️

🟠 1) packages/agents-sdk/src/types.ts:90-94 Breaking type contract change

Issue: The SkillReference type was changed from a union type supporting 4 input forms (string, {id}, {skillId}, and inline SkillDefinition) to a simple interface with only {id, alwaysLoaded?}. This removes support for:

String shorthand: skills: () => ['my-skill']
Alternative property name: skills: () => [{ skillId: 'x' }]
Explicit index: skills: () => [{ id: 'x', index: 0 }]
Inline skill definition with content

Why: TypeScript consumers of @inkeep/agents-sdk who use any removed form will get compilation errors after upgrading. The runtime in subAgent.ts still handles all legacy formats (lines 247-272), creating a split where TypeScript users see breakage but JavaScript users don't. This asymmetry can cause confusion and makes the breaking change less predictable.

Fix: Either:

If intentionally breaking: Create a changeset documenting this breaking change and add migration guidance
If preserving compat: Keep the union type and mark deprecated variants with @deprecated JSDoc comments

Refs:

🟠 2) scope Missing changeset for published package

Issue: No changeset was created for this breaking type change to @inkeep/agents-sdk. Per AGENTS.md, "Schema changes requiring migration, significant behavior changes" require version bumps.

Why: Without a changeset, consumers upgrading the package won't be warned about breaking changes. The package is public on npm (publishConfig.access: 'public').

Fix: Create a changeset:

pnpm bump minor --pkg agents-sdk "Simplify SkillReference type: remove index field and legacy input forms (string, skillId, inline definition). Skills now ordered by array position."

Refs:

AGENTS.md — changeset guidance

// Findings posted as inline comments:

🟠 Major: types.ts:90-94 Breaking type contract change

🟡 Minor (1) 🟡

// Findings posted as inline comments:

🟡 Minor: types.ts:94 Type asymmetry between input and output

💭 Consider (1) 💭

💭 1) types.ts:349 setConfig signature cleanup

Issue: The optional skills?: SkillDefinition[] parameter was removed from AgentInterface.setConfig().

Why: This is technically a breaking type change, but no code in the codebase actually passes this parameter. This appears to be dead code cleanup.

Fix: If intentional cleanup, no action needed. If the parameter should remain for future use, keep it.

Refs:

types.ts:349

// Findings posted as inline comments:

💭 Consider: skills.mdx:122-123 Clarify priority direction

🕐 Pending Recommendations (1)

🟡 sub-agent-generator.test.ts:287 Author self-review: test description wording improvement

💡 APPROVE WITH SUGGESTIONS

Summary: This PR successfully simplifies the SkillReference type by removing the explicit index field in favor of array-position ordering — a cleaner mental model for developers. The implementation is sound: the runtime correctly calculates index from array position, and the documentation accurately reflects the new API.

However, a changeset should be added documenting this as a breaking type change for TypeScript consumers. The simplified type removes several previously-accepted input forms, and SDK users should be aware of this when upgrading. Once a changeset is added, this is good to merge! 🎉

Discarded (5)

Location	Issue	Reason Discarded
`types.ts:85-88`	SkillDefinition using Pick is good pattern	Positive observation, not an issue
`types.ts:90`	SkillReference lacks helper function like CredentialReference	Nice-to-have enhancement, out of scope for this PR
`activities-planner-advanced.ts:125-128`	Template adds alwaysLoaded: true to first skill	Intentional pedagogical change demonstrating both loading modes
`agent.test.ts:593-596`	Test doesn't explicitly verify ordering preservation	Low risk - JavaScript arrays preserve order; covered by other tests
`subAgent.ts:247-272`	Legacy format handling lacks dedicated tests	Pre-existing code not introduced by this PR

Reviewers (7)

Reviewer	Returned	Main Findings	Consider	Inline Comments	Pending Recs	Discarded
`pr-review-breaking-changes`	4	2	0	1	0	1
`pr-review-consistency`	4	0	0	1	1	2
`pr-review-types`	3	0	0	0	0	3
`pr-review-product`	4	0	1	1	0	2
`pr-review-tests`	2	0	0	0	0	2
`pr-review-docs`	1	0	0	0	0	1
`pr-review-standards`	0	0	0	0	0	0
Total	18	2	1	3	1	11

claude · 2026-02-12T01:51:16Z

packages/agents-sdk/src/types.ts

+export interface SkillReference {
  id: string;
-  name: string;
-  description: string;
-  content: string;
-  metadata: Record<string, string> | null;
-  createdAt?: string;
-  updatedAt?: string;
+  /** @default false */
+  alwaysLoaded?: boolean;
 }


🟠 MAJOR Breaking type contract change

Issue: The SkillReference type was changed from a union type supporting 4 input forms to a simple interface:

Before:

export type SkillReference = | string | { id: string; index?: number; alwaysLoaded?: boolean } | { skillId: string; index?: number; alwaysLoaded?: boolean } | (SkillDefinition & { index?: number; alwaysLoaded?: boolean });

After:

export interface SkillReference { id: string; alwaysLoaded?: boolean; }

Why: This is a breaking change for TypeScript consumers of @inkeep/agents-sdk who use any of the removed forms:

skills: () => ['my-skill'] (string shorthand)

skills: () => [{ skillId: 'x' }] (skillId variant)

skills: () => [{ id: 'x', index: 0 }] (explicit index)

The runtime implementation in subAgent.ts (lines 247-272) still handles all legacy formats, so JavaScript users won't break, but TypeScript users will get compilation errors after upgrading.

Fix: Either:

If intentionally breaking: Create a changeset with pnpm bump minor --pkg agents-sdk "Simplify SkillReference type: remove index field and legacy input forms. Skills now ordered by array position." and add migration guidance

If preserving compat: Keep the union type and mark deprecated variants with JSDoc @deprecated comments

Refs:

subAgent.ts:247-272 — runtime still handles legacy forms

claude · 2026-02-12T01:51:23Z

packages/agents-sdk/src/types.ts

-  updatedAt?: string;
+  /** @default false */
+  alwaysLoaded?: boolean;
 }


🟡 MINOR Type asymmetry between input and output

Issue: The input type SkillReference no longer has index, but SubAgentInterface.getSkills() return type (lines 311-316) still includes index?: number. This creates a conceptual mismatch.

Why: Users cannot specify index when defining skills, but the returned type suggests index might be absent (optional). In practice, the implementation always calculates and populates index from array position.

Fix: Consider making the return type's index required (index: number) since it's always computed, or document that index is derived from array position:

getSkills(): Array<{ id: string; index: number; // Always calculated from array position alwaysLoaded?: boolean; skill?: SkillDefinition; }>;

Refs:

types.ts:311-316 — getSkills return type

subAgent.ts:258 — index always calculated

claude · 2026-02-12T01:51:32Z

agents-docs/content/typescript-sdk/skills.mdx


+Skill order and priority follow the order of items in the `skills` array.


💭 Consider Clarify priority direction

Issue: The statement "Skill order and priority follow the order of items in the skills array" doesn't specify the priority direction.

Why: The system prompt instruction in PromptConfig.ts (line 267) says "Apply skills by index; later entries weigh more." This implies later array positions = higher priority, which should be explicitly documented for developers.

Fix: Consider clarifying the priority semantics:

Suggested change

Skill order and priority follow the order of items in the `skills` array.

Skill order and priority follow the order of items in the `skills` array. Later entries in the array take higher priority when skills conflict.

Refs:

PromptConfig.ts:267 — "later entries weigh more"

dimaMachina added 6 commits February 12, 2026 08:29

upd

a075799

upd

c25b6e6

upd

fa45f72

upd

0a8cd1d

simplify SkillReference

bc01bea

simplify SkillReference

7843a2e

dimaMachina commented Feb 12, 2026

View reviewed changes

dimaMachina marked this pull request as draft February 12, 2026 01:43

vercel bot deployed to Preview – agents-api February 12, 2026 01:43 View deployment

vercel bot deployed to Preview – agents-manage-ui February 12, 2026 01:43 View deployment

vercel bot deployed to Preview – agents-docs February 12, 2026 01:44 View deployment

claude bot reviewed Feb 12, 2026

View reviewed changes

github-actions bot deleted a comment from claude bot Feb 12, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify SkillReference, remove `index` field, calculate based on array index#1947

simplify SkillReference, remove `index` field, calculate based on array index#1947
dimaMachina wants to merge 6 commits intomainfrom
skill-reference

dimaMachina commented Feb 12, 2026

Uh oh!

vercel bot commented Feb 12, 2026 •

edited

Loading

Uh oh!

changeset-bot bot commented Feb 12, 2026

Uh oh!

dimaMachina Feb 12, 2026

Uh oh!

claude bot left a comment

Uh oh!

claude bot Feb 12, 2026

Uh oh!

claude bot Feb 12, 2026

Uh oh!

claude bot Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	it('should generate skill references without index and keep index-based ordering', () => {
	it('should generate skill references and keep index-based ordering', () => {


		Skill order and priority follow the order of items in the `skills` array.

Conversation

dimaMachina commented Feb 12, 2026

Uh oh!

vercel bot commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Feb 12, 2026

⚠️ No Changeset found

Uh oh!

dimaMachina Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

PR Review Summary

🟠⚠️ Major (2) 🟠⚠️

🟡 Minor (1) 🟡

💭 Consider (1) 💭

🕐 Pending Recommendations (1)

💡 APPROVE WITH SUGGESTIONS

Uh oh!

claude bot Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel bot commented Feb 12, 2026 •

edited

Loading