Skip to content

Commit 7d94937

Browse files
PetrBulanektomkis
PetrBulanek
authored and
tomkis
committed
fix(skills): address review findings on marketplace branch
- controller: mirror contentHash + publishes on Go InstanceSpec/SkillRef so hibernate/wake round-trips preserve them - api-server: forward session instanceId from MCP list_skill_sources so template-seeded sources surface - api-server-api: make instanceId optional on listSkills; service throws PRECONDITION_FAILED when private fallback needs an instance - api-server: drop dead AgentRuntimeSkillsClient.readLocal + LocalSkillFile and matching test mocks - api-server: wrap instancesRepo.updateSpec in retryOnConflict to handle concurrent MCP + UI writers Signed-off-by: Petr Bulánek <[email protected]>
1 parent 70e3033 commit 7d94937

11 files changed

Lines changed: 129 additions & 48 deletions

File tree

‎packages/api-server-api/src/modules/skills/router.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,13 @@ export const skillsRouter = t.router({
6565
.mutation(({ ctx, input }) => ctx.skills.refreshSource(input.id)),
6666
}),
6767

68+
/** `instanceId` is optional — public-archive scans don't need an instance.
69+
* Private-source scans (that fall through to the authenticated
70+
* agent-runtime path) will throw with a clear hint if it's missing. */
6871
listSkills: t.procedure
6972
.input(z.object({
7073
sourceId: z.string().min(1),
71-
instanceId: z.string().min(1),
74+
instanceId: z.string().min(1).optional(),
7275
}))
7376
.output(z.array(skillViewSchema))
7477
.query(async ({ ctx, input }) => {

‎packages/api-server-api/src/modules/skills/types.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ export interface SkillsService {
120120
createSource: (input: CreateSkillSourceInput) => Promise<SkillSource>;
121121
deleteSource: (id: string) => Promise<void>;
122122
refreshSource: (id: string) => Promise<void>;
123-
listSkills: (sourceId: string, instanceId: string) => Promise<Skill[]>;
123+
listSkills: (sourceId: string, instanceId?: string) => Promise<Skill[]>;
124124
installSkill: (input: InstallSkillInput) => Promise<SkillRef[]>;
125125
uninstallSkill: (input: UninstallSkillInput) => Promise<SkillRef[]>;
126126
listLocal: (instanceId: string) => Promise<LocalSkill[]>;

‎packages/api-server/src/__tests__/unit/publish-service.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ function makeDeps() {
4747
install: vi.fn(),
4848
uninstall: vi.fn(),
4949
listLocal: vi.fn(),
50-
readLocal: vi.fn(),
5150
publish: vi.fn().mockResolvedValue({
5251
prUrl: "https://github.com/foo/bar/pull/1",
5352
branch: "humr/publish-demo-20260101000000",

‎packages/api-server/src/__tests__/unit/skills-mcp-tools.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@ describe("skills MCP tool handlers", () => {
3838
expect(JSON.parse(res.content[0].text)).toEqual([SOURCE]);
3939
});
4040

41+
it("list_skill_sources forwards the session instanceId so template-seeded sources are included", async () => {
42+
const listSources = vi.fn<SkillsService["listSources"]>().mockResolvedValue([SOURCE]);
43+
const t = createSkillsToolHandlers(SESSION_INSTANCE_ID, makeSkills({ listSources }));
44+
await t.listSources();
45+
expect(listSources).toHaveBeenCalledTimes(1);
46+
expect(listSources).toHaveBeenCalledWith(SESSION_INSTANCE_ID);
47+
});
48+
4149
it("list_skills_in_source rejects an unknown sourceId without calling listSkills", async () => {
4250
const listSkills = vi.fn<SkillsService["listSkills"]>().mockResolvedValue([]);
4351
const t = createSkillsToolHandlers(SESSION_INSTANCE_ID, makeSkills({

‎packages/api-server/src/__tests__/unit/skills-service.test.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ function makeEnv(opts: {
103103
install: runtimeInstall,
104104
uninstall: runtimeUninstall,
105105
listLocal: vi.fn<AgentRuntimeSkillsClient["listLocal"]>().mockResolvedValue([]),
106-
readLocal: vi.fn<AgentRuntimeSkillsClient["readLocal"]>().mockResolvedValue([]),
107106
publish: vi.fn<AgentRuntimeSkillsClient["publish"]>().mockResolvedValue({ prUrl: "x", branch: "y" }),
108107
scan: vi.fn<AgentRuntimeSkillsClient["scan"]>().mockResolvedValue([]),
109108
};
@@ -219,7 +218,6 @@ describe("skills-service install", () => {
219218
install: env.runtimeInstall,
220219
uninstall: env.runtimeUninstall,
221220
listLocal: vi.fn(),
222-
readLocal: vi.fn(),
223221
publish: vi.fn(),
224222
scan: vi.fn(),
225223
},
@@ -318,7 +316,6 @@ describe("skills-service listLocal", () => {
318316
install: vi.fn(),
319317
uninstall: vi.fn(),
320318
listLocal: runtimeListLocal,
321-
readLocal: vi.fn(),
322319
publish: vi.fn(),
323320
scan: vi.fn(),
324321
},
@@ -356,7 +353,6 @@ describe("skills-service listLocal", () => {
356353
install: vi.fn(),
357354
uninstall: vi.fn(),
358355
listLocal: runtimeListLocal,
359-
readLocal: vi.fn(),
360356
publish: vi.fn(),
361357
scan: vi.fn(),
362358
},
@@ -383,7 +379,6 @@ describe("skills-service listLocal", () => {
383379
install: vi.fn(),
384380
uninstall: vi.fn(),
385381
listLocal: runtimeListLocal,
386-
readLocal: vi.fn(),
387382
publish: vi.fn(),
388383
scan: vi.fn(),
389384
},
@@ -414,7 +409,6 @@ describe("skills-service listSkills routing", () => {
414409
install: vi.fn(),
415410
uninstall: vi.fn(),
416411
listLocal: vi.fn(),
417-
readLocal: vi.fn(),
418412
publish: vi.fn(),
419413
scan: opts.runtimeScan,
420414
};
@@ -564,7 +558,6 @@ describe("skills-service listSkills routing", () => {
564558
install: vi.fn(),
565559
uninstall: vi.fn(),
566560
listLocal: vi.fn(),
567-
readLocal: vi.fn(),
568561
publish: vi.fn(),
569562
scan: runtimeScan,
570563
},
@@ -596,7 +589,6 @@ describe("skills-service getState (ghost reconciliation)", () => {
596589
install: vi.fn(),
597590
uninstall: vi.fn(),
598591
listLocal: vi.fn().mockResolvedValue(opts.local ?? []),
599-
readLocal: vi.fn(),
600592
publish: vi.fn(),
601593
scan: vi.fn(),
602594
};

‎packages/api-server/src/apps/harness-api-server/skills-tools.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export function createSkillsToolHandlers(instanceId: string, skills: SkillsServi
3636
return {
3737
async listSources(): Promise<ToolContent> {
3838
try {
39-
const sources = await skills.listSources();
39+
const sources = await skills.listSources(instanceId);
4040
return textResult(JSON.stringify(sources));
4141
} catch (err) {
4242
return formatToolError(err, "Failed to list skill sources");

‎packages/api-server/src/modules/agents/infrastructure/instances-repository.ts‎

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,32 @@ import {
1313
} from "./poll-until-ready.js";
1414
import type { InfraInstance } from "../domain/instance-assembly.js";
1515

16+
/** Matches K8s "The object has been modified..." 409 Conflict. */
17+
function is409(err: unknown): boolean {
18+
return (
19+
err instanceof Error &&
20+
"code" in err &&
21+
(err as { code: number }).code === 409
22+
);
23+
}
24+
25+
/** Re-run a read-modify-write routine when the K8s API rejects the write
26+
* with 409 Conflict. Mirrors the Go controller's `retry.RetryOnConflict`
27+
* so concurrent MCP + UI writers don't surface racy errors to the user. */
28+
async function retryOnConflict<T>(fn: () => Promise<T>): Promise<T> {
29+
const MAX_ATTEMPTS = 5;
30+
let lastErr: unknown;
31+
for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
32+
try {
33+
return await fn();
34+
} catch (err) {
35+
if (!is409(err)) throw err;
36+
lastErr = err;
37+
}
38+
}
39+
throw lastErr;
40+
}
41+
1642
export interface InstancesRepository {
1743
list(owner?: string): Promise<InfraInstance[]>;
1844
get(id: string, owner?: string): Promise<InfraInstance | null>;
@@ -88,12 +114,17 @@ export function createInstancesRepository(k8s: K8sClient): InstancesRepository {
88114
},
89115

90116
async updateSpec(id, owner, patch) {
91-
const cm = await k8s.getConfigMap(id);
92-
if (!cm) return null;
93-
if (owner && !isOwnedBy(cm, owner)) return null;
94-
cm.data = patchSpecField(cm, patch);
95-
const updated = await k8s.replaceConfigMap(id, cm);
96-
return parseInfraInstance(updated);
117+
// read-modify-write under a conflict-retry loop: re-fetch the
118+
// ConfigMap (fresh resourceVersion) on 409 so concurrent writers
119+
// (MCP + UI, or two tabs) don't surface racy errors.
120+
return retryOnConflict(async () => {
121+
const cm = await k8s.getConfigMap(id);
122+
if (!cm) return null;
123+
if (owner && !isOwnedBy(cm, owner)) return null;
124+
cm.data = patchSpecField(cm, patch);
125+
const updated = await k8s.replaceConfigMap(id, cm);
126+
return parseInfraInstance(updated);
127+
});
97128
},
98129

99130
async delete(id, owner?) {

‎packages/api-server/src/modules/skills/infrastructure/agent-runtime-client.ts‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,6 @@ export interface UninstallSkillCall {
1313
skillPaths: string[];
1414
}
1515

16-
export interface LocalSkillFile {
17-
relPath: string;
18-
content: string;
19-
base64?: true;
20-
}
21-
2216
export interface PublishSkillCall {
2317
name: string;
2418
skillPaths: string[];
@@ -55,12 +49,6 @@ export interface AgentRuntimeSkillsClient {
5549
install(instanceId: string, token: string, body: InstallSkillCall): Promise<InstallSkillResult>;
5650
uninstall(instanceId: string, token: string, body: UninstallSkillCall): Promise<void>;
5751
listLocal(instanceId: string, token: string, skillPaths: string[]): Promise<LocalSkill[]>;
58-
readLocal(
59-
instanceId: string,
60-
token: string,
61-
name: string,
62-
skillPaths: string[],
63-
): Promise<LocalSkillFile[]>;
6452
publish(instanceId: string, token: string, body: PublishSkillCall): Promise<PublishSkillResult>;
6553
scan(instanceId: string, token: string, source: string): Promise<Skill[]>;
6654
}
@@ -138,12 +126,6 @@ export function createAgentRuntimeSkillsClient(namespace: string): AgentRuntimeS
138126
const { skills } = await getJson<{ skills: LocalSkill[] }>(url, token);
139127
return skills;
140128
},
141-
async readLocal(instanceId, token, name, skillPaths) {
142-
const qs = skillPaths.map((p) => `skillPaths=${encodeURIComponent(p)}`).join("&");
143-
const url = `${base(instanceId)}/api/skills/local/${encodeURIComponent(name)}?${qs}`;
144-
const { files } = await getJson<{ files: LocalSkillFile[] }>(url, token);
145-
return files;
146-
},
147129
publish: (instanceId, token, body) =>
148130
postJson<PublishSkillResult>(`${base(instanceId)}/api/skills/publish`, token, body),
149131
async scan(instanceId, token, source) {

‎packages/api-server/src/modules/skills/services/skills-service.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ export function createSkillsService(deps: SkillsServiceDeps): SkillsService {
278278
}
279279
},
280280

281-
async listSkills(sourceId: string, instanceId: string) {
281+
async listSkills(sourceId: string, instanceId?: string) {
282282
const src = await resolveSource(deps, sourceId);
283283
if (!src) return [];
284284

@@ -298,7 +298,14 @@ export function createSkillsService(deps: SkillsServiceDeps): SkillsService {
298298
}
299299

300300
// Private/authenticated path: delegate to agent-runtime inside a
301-
// running instance pod, which uses OneCLI's token swap.
301+
// running instance pod, which uses OneCLI's token swap. Without an
302+
// instanceId we can't target a pod — refuse with a clear message.
303+
if (!instanceId) {
304+
throw new TRPCError({
305+
code: "PRECONDITION_FAILED",
306+
message: "source is private; select an instance to scan it",
307+
});
308+
}
302309
const infra = await deps.instancesRepo.get(instanceId, deps.owner);
303310
if (!infra) throw new TRPCError({ code: "NOT_FOUND", message: "instance not found" });
304311
if (infra.currentState !== "running") {

‎packages/controller/pkg/types/types.go‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -73,21 +73,37 @@ type MCPServerConfig struct {
7373
// --- Instance ---
7474

7575
type InstanceSpec struct {
76-
Version string `yaml:"version"`
77-
DesiredState string `yaml:"desiredState"`
78-
AgentName string `yaml:"agentId,omitempty"`
79-
Env []EnvVar `yaml:"env,omitempty"`
80-
SecretRef string `yaml:"secretRef,omitempty"`
81-
Description string `yaml:"description,omitempty"`
82-
Skills []SkillRef `yaml:"skills,omitempty"`
76+
Version string `yaml:"version"`
77+
DesiredState string `yaml:"desiredState"`
78+
AgentName string `yaml:"agentId,omitempty"`
79+
Env []EnvVar `yaml:"env,omitempty"`
80+
SecretRef string `yaml:"secretRef,omitempty"`
81+
Description string `yaml:"description,omitempty"`
82+
Skills []SkillRef `yaml:"skills,omitempty"`
83+
Publishes []SkillPublishRecord `yaml:"publishes,omitempty"`
8384
}
8485

8586
// SkillRef identifies an installed skill on an instance.
8687
// Source is the source git URL; Name is the skill directory name; Version is a commit SHA.
88+
// ContentHash is mirrored from the TS api-server for drift detection; the Go
89+
// controller does not use it but must preserve it across spec round-trips.
8790
type SkillRef struct {
88-
Source string `yaml:"source"`
89-
Name string `yaml:"name"`
90-
Version string `yaml:"version"`
91+
Source string `yaml:"source"`
92+
Name string `yaml:"name"`
93+
Version string `yaml:"version"`
94+
ContentHash string `yaml:"contentHash,omitempty"`
95+
}
96+
97+
// SkillPublishRecord mirrors the TS api-server's record of a successful
98+
// `publishSkill` call. The Go controller does not use these fields; they
99+
// exist only so spec round-trips (hibernate / wake / reconcile) preserve them.
100+
type SkillPublishRecord struct {
101+
SkillName string `yaml:"skillName"`
102+
SourceID string `yaml:"sourceId"`
103+
SourceName string `yaml:"sourceName"`
104+
SourceGitURL string `yaml:"sourceGitUrl"`
105+
PRURL string `yaml:"prUrl"`
106+
PublishedAt string `yaml:"publishedAt"`
91107
}
92108

93109
type InstanceStatus struct {

0 commit comments

Comments
 (0)