Skip to content

Commit 9fab706

Browse files
dcramercodex
andcommitted
fix(auth): Provision plugin creds on skill activation
Move per-turn plugin credential provisioning out of the bash execution path and into skill activation. This keeps the implicit auth model intact while avoiding unnecessary token minting for unrelated shell commands and ensuring checkpoint-loaded skills are reprovisioned at the start of each turn. Refs GH-209 Co-Authored-By: GPT-5 <noreply@openai.com>
1 parent 1ce220a commit 9fab706

6 files changed

Lines changed: 77 additions & 27 deletions

File tree

packages/junior/src/chat/respond.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
} from "@/chat/capabilities/factory";
2222
import { maybeExecuteJrRpcCustomCommand } from "@/chat/capabilities/jr-rpc-command";
2323
import type { ChannelConfigurationService } from "@/chat/configuration/types";
24+
import { CredentialUnavailableError } from "@/chat/credentials/broker";
2425
import { SkillSandbox } from "@/chat/sandbox/skill-sandbox";
2526
import {
2627
discoverSkills,
@@ -479,6 +480,32 @@ export async function generateAssistantReply(
479480
const syncResumeState = () => {
480481
loadedSkillNamesForResume = activeSkills.map((skill) => skill.name);
481482
};
483+
const enableSkillCredentials = async (
484+
skill: Skill | null,
485+
reason: string,
486+
): Promise<void> => {
487+
if (!skill?.pluginProvider) {
488+
return;
489+
}
490+
491+
try {
492+
await capabilityRuntime.enableCredentialsForTurn({
493+
activeSkill: skill,
494+
reason,
495+
});
496+
} catch (error) {
497+
if (
498+
error instanceof CredentialUnavailableError &&
499+
context.requester?.userId
500+
) {
501+
await pluginAuth.handleCredentialUnavailable({
502+
activeSkill: skill,
503+
error,
504+
});
505+
}
506+
throw error;
507+
}
508+
};
482509

483510
setTags({
484511
conversationId: spanContext.conversationId,
@@ -526,6 +553,10 @@ export async function generateAssistantReply(
526553
// aborted turn park cleanly.
527554
return undefined;
528555
}
556+
await enableSkillCredentials(
557+
effective,
558+
`skill:${effective.name}:turn:load`,
559+
);
529560
if (!effective.pluginProvider) {
530561
return undefined;
531562
}
@@ -563,6 +594,7 @@ export async function generateAssistantReply(
563594
timeoutResumeMessages = existingCheckpoint?.piMessages ?? [];
564595
throw mcpAuth.getPendingPause()!;
565596
}
597+
await enableSkillCredentials(skill, `skill:${skill.name}:turn:resume`);
566598
}
567599
syncResumeState();
568600

packages/junior/src/chat/tools/agent-tools.ts

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import { serializeGenAiAttribute } from "@/chat/logging";
33
import { setSpanAttributes, withSpan, type LogContext } from "@/chat/logging";
44
import { GEN_AI_PROVIDER_NAME } from "@/chat/pi/client";
55
import { shouldEmitDevAgentTrace } from "@/chat/runtime/dev-agent-trace";
6-
import { CredentialUnavailableError } from "@/chat/credentials/broker";
76
import {
87
PluginAuthorizationPauseError,
98
type PluginAuthOrchestration,
@@ -79,25 +78,6 @@ export function createAgentTools(
7978
toolName === "bash" && typeof parsed.command === "string"
8079
? parsed.command.trim()
8180
: "";
82-
if (bashCommand && capabilityRuntime) {
83-
try {
84-
await capabilityRuntime.enableCredentialsForTurn({
85-
activeSkill: sandbox.getActiveSkill(),
86-
reason: `skill:${sandbox.getActiveSkill()?.name ?? "unknown"}:bash:auto-enable`,
87-
});
88-
} catch (error) {
89-
if (
90-
error instanceof CredentialUnavailableError &&
91-
pluginAuthOrchestration
92-
) {
93-
await pluginAuthOrchestration.handleCredentialUnavailable({
94-
activeSkill: sandbox.getActiveSkill(),
95-
error,
96-
});
97-
}
98-
throw error;
99-
}
100-
}
10181
const injection = resolveCredentialInjection(
10282
toolName,
10383
bashCommand,

packages/junior/tests/integration/respond-mcp-progressive-loading.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,9 @@ vi.mock("@/chat/config", async (importOriginal) => {
334334

335335
vi.mock("@/chat/capabilities/factory", () => ({
336336
createSkillCapabilityRuntime: () => ({
337+
enableCredentialsForTurn: async () => undefined,
337338
getTurnHeaderTransforms: () => undefined,
339+
getTurnEnv: () => undefined,
338340
}),
339341
createUserTokenStore: () => ({
340342
get: async () => undefined,

packages/junior/tests/unit/runtime/respond-lazy-sandbox.test.ts

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ const {
55
createSandboxCallCount,
66
activeSandboxVersion,
77
attachFileReadVersions,
8+
enabledCredentialSkillNames,
9+
checkpointLoadedSkillNames,
810
pendingWorkspaceRelease,
911
} = vi.hoisted(() => ({
1012
agentMode: {
@@ -26,6 +28,12 @@ const {
2628
attachFileReadVersions: {
2729
value: [] as number[],
2830
},
31+
enabledCredentialSkillNames: {
32+
value: [] as string[],
33+
},
34+
checkpointLoadedSkillNames: {
35+
value: [] as string[],
36+
},
2937
pendingWorkspaceRelease: {
3038
value: undefined as (() => void) | undefined,
3139
},
@@ -218,7 +226,15 @@ vi.mock("@/chat/runtime/dev-agent-trace", () => ({
218226

219227
vi.mock("@/chat/capabilities/factory", () => ({
220228
createSkillCapabilityRuntime: () => ({
221-
enableCredentialsForTurn: async () => undefined,
229+
enableCredentialsForTurn: async (input: {
230+
activeSkill: { name?: string } | null;
231+
}) => {
232+
const skillName = input.activeSkill?.name;
233+
if (skillName) {
234+
enabledCredentialSkillNames.value.push(skillName);
235+
}
236+
return undefined;
237+
},
222238
getTurnHeaderTransforms: () => undefined,
223239
getTurnEnv: () => undefined,
224240
}),
@@ -246,7 +262,13 @@ vi.mock("@/chat/services/turn-checkpoint", () => ({
246262
loadTurnCheckpoint: async () => ({
247263
resumedFromCheckpoint: false,
248264
currentSliceId: 1,
249-
existingCheckpoint: undefined,
265+
existingCheckpoint:
266+
checkpointLoadedSkillNames.value.length > 0
267+
? {
268+
loadedSkillNames: [...checkpointLoadedSkillNames.value],
269+
piMessages: [],
270+
}
271+
: undefined,
250272
canUseTurnSession: false,
251273
}),
252274
persistCompletedCheckpoint: async () => undefined,
@@ -271,6 +293,7 @@ vi.mock("@/chat/skills", () => {
271293
name: "demo-skill",
272294
description: "Demo skill",
273295
skillPath: "/tmp/skills/demo-skill",
296+
pluginProvider: "demo",
274297
};
275298

276299
return {
@@ -439,6 +462,8 @@ describe("generateAssistantReply lazy sandbox boot", () => {
439462
createSandboxCallCount.value = 0;
440463
activeSandboxVersion.value = 1;
441464
attachFileReadVersions.value = [];
465+
enabledCredentialSkillNames.value = [];
466+
checkpointLoadedSkillNames.value = [];
442467
pendingWorkspaceRelease.value = undefined;
443468
});
444469

@@ -458,10 +483,22 @@ describe("generateAssistantReply lazy sandbox boot", () => {
458483

459484
expect(reply.text).toBe("Loaded demo skill.");
460485
expect(createSandboxCallCount.value).toBe(0);
486+
expect(enabledCredentialSkillNames.value).toEqual(["demo-skill"]);
461487
expect(reply.sandboxId).toBeUndefined();
462488
expect(reply.diagnostics.toolCalls).toEqual(["loadSkill"]);
463489
});
464490

491+
it("reprovisions plugin credentials for checkpoint-loaded skills at turn start", async () => {
492+
checkpointLoadedSkillNames.value = ["demo-skill"];
493+
494+
const reply = await generateAssistantReply("hello");
495+
496+
expect(reply.text).toBe("Plain reply.");
497+
expect(createSandboxCallCount.value).toBe(0);
498+
expect(enabledCredentialSkillNames.value).toEqual(["demo-skill"]);
499+
expect(reply.diagnostics.toolCalls).toEqual([]);
500+
});
501+
465502
it("memoizes the lazy sandbox workspace across multiple workspace calls", async () => {
466503
agentMode.value = "attachFile";
467504

packages/junior/tests/unit/runtime/respond-timeout-resume.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,9 @@ vi.mock("@/chat/config", async (importOriginal) => {
101101

102102
vi.mock("@/chat/capabilities/factory", () => ({
103103
createSkillCapabilityRuntime: () => ({
104+
enableCredentialsForTurn: async () => undefined,
104105
getTurnHeaderTransforms: () => undefined,
106+
getTurnEnv: () => undefined,
105107
}),
106108
createUserTokenStore: () => ({
107109
get: async () => undefined,

packages/junior/tests/unit/tools/agent-tools.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ describe("createAgentTools", () => {
2929
handleToolExecutionError.mockClear();
3030
});
3131

32-
it("auto-enables provider credentials before executing bash", async () => {
32+
it("injects already-enabled provider credentials into bash", async () => {
3333
const sandbox = new SkillSandbox([githubSkill], [githubSkill]);
3434
const enableCredentialsForTurn = vi.fn(async () => {});
3535
const capabilityRuntime = {
@@ -81,10 +81,7 @@ describe("createAgentTools", () => {
8181
command: "gh issue view 123 --repo getsentry/junior",
8282
});
8383

84-
expect(enableCredentialsForTurn).toHaveBeenCalledWith({
85-
activeSkill: githubSkill,
86-
reason: "skill:github:bash:auto-enable",
87-
});
84+
expect(enableCredentialsForTurn).not.toHaveBeenCalled();
8885
expect(sandboxExecutor.execute).toHaveBeenCalledWith({
8986
toolName: "bash",
9087
input: {

0 commit comments

Comments
 (0)