Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/adapters/kiro-errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,17 @@ function classifyKiroFailure(
retryable: false,
};
}
// #993: a gated model demanding a profileArn gets a stable, actionable code
// instead of the generic validation bucket. Non-retryable by definition.
if (evidence.includes("profilearn") && evidence.includes("required")) {
return {
message: "kiro_profile_required: Kiro requires a CodeWhisperer profileArn for this account and model. Re-login or re-import the matching Kiro account (ocx account login kiro --reauth) so the profile is captured, then retry.",
status: 400,
errorType: "invalid_request_error",
code: "kiro_profile_required",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Propagate the profile-required code on HTTP errors

The new kiro_profile_required code is only carried by stream/parser error events; the ordinary non-stream HTTP path calls safeKiroHttpErrorMessage(), then wraps this response as formatErrorResponse(status, "upstream_error", ...), so the /v1/chat/completions reproduction for this 400 still returns top-level code: "upstream_error" instead of the stable code added here. Return a structured classification to the HTTP wrapper or map this message before formatErrorResponse so non-stream clients can handle the same condition.

Useful? React with 👍 / 👎.

retryable: false,
};
}
Comment on lines +114 to +122

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Restrict kiro_profile_required to a required-profile 400 response.

Line 114 matches independent occurrences of profilearn and required. A 429 payload such as profileArn is required; quota exceeded becomes a 400 non-retryable error before the rate-limit branch. An unrelated 400 payload such as profileArn is malformed; clientId is required also gives incorrect re-login guidance.

Require status === undefined || status === 400 so event-stream exceptions remain supported. Match a bound profileArn ... required phrase. Add negative tests for a 429 response and an unrelated required field.

Proposed fix
   const headerType = headerValue(headers, ":exception-type") || headerValue(headers, ":error-type") || "";
   const evidence = [headerType, ...payloadDetails(payloadText), message].join(" ").toLowerCase();
+  const profileArnRequired =
+    /\bprofile\s*arn\b\s+(?:is\s+)?required\b|\brequired\s+(?:for\s+)?profile\s*arn\b/.test(evidence);
   if (isContentLengthError(evidence)) {
     return {
       // ...
     };
   }
-  if (evidence.includes("profilearn") && evidence.includes("required")) {
+  if ((status === undefined || status === 400) && profileArnRequired) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (evidence.includes("profilearn") && evidence.includes("required")) {
return {
message: "kiro_profile_required: Kiro requires a CodeWhisperer profileArn for this account and model. Re-login or re-import the matching Kiro account (ocx account login kiro --reauth) so the profile is captured, then retry.",
status: 400,
errorType: "invalid_request_error",
code: "kiro_profile_required",
retryable: false,
};
}
const profileArnRequired =
/\bprofile\s*arn\b\s+(?:is\s+)?required\b|\brequired\s+(?:for\s+)?profile\s*arn\b/.test(evidence);
if ((status === undefined || status === 400) && profileArnRequired) {
return {
message: "kiro_profile_required: Kiro requires a CodeWhisperer profileArn for this account and model. Re-login or re-import the matching Kiro account (ocx account login kiro --reauth) so the profile is captured, then retry.",
status: 400,
errorType: "invalid_request_error",
code: "kiro_profile_required",
retryable: false,
};
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/adapters/kiro-errors.ts` around lines 114 - 122, Update the
profile-required detection in the Kiro error mapping to only apply when status
is undefined or 400, preserving event-stream exceptions while excluding
rate-limit responses. Replace the independent evidence checks with a bounded
phrase matching profileArn followed by required, and add negative tests covering
a 429 response and an unrelated required field.

if (
evidence.includes("insufficient_quota")
|| evidence.includes("quota exhausted")
Expand Down
56 changes: 50 additions & 6 deletions src/oauth/kiro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,37 @@ async function defaultKiroCliRunner(args: string[], signal?: AbortSignal): Promi
}
}

async function readKiroCliIdentity(runner: KiroCliRunner, signal?: AbortSignal): Promise<{ email?: string }> {
/** Kiro profile ARN structure: arn:<partition>:codewhisperer:<region>:<account>:profile/<id> */
const KIRO_PROFILE_ARN_PATTERN = /^arn:[a-z0-9-]+:codewhisperer:[a-z0-9-]+:\d{12}:profile\/[A-Za-z0-9-]+$/;
const KIRO_PROFILE_ARN_MAX_LENGTH = 256;

function parseKiroProfileArn(value: unknown): string | undefined {
if (typeof value !== "string") return undefined;
const trimmed = value.trim();
if (trimmed.length === 0 || trimmed.length > KIRO_PROFILE_ARN_MAX_LENGTH) return undefined;
return KIRO_PROFILE_ARN_PATTERN.test(trimmed) ? trimmed : undefined;
}

function profileArnFromWhoami(parsed: Record<string, unknown>): string | undefined {
// Only narrowly-named documented-ish shapes; never invent an ARN (#993).
return parseKiroProfileArn(parsed.profileArn)
?? parseKiroProfileArn(parsed.profile_arn)
?? (parsed.profile && typeof parsed.profile === "object" && !Array.isArray(parsed.profile)
? parseKiroProfileArn((parsed.profile as Record<string, unknown>).arn)
: undefined);
}

async function readKiroCliIdentity(runner: KiroCliRunner, signal?: AbortSignal): Promise<{ email?: string; profileArn?: string }> {
try {
const result = await runner(["whoami", "--format", "json"], signal);
if (result.exitCode !== 0) return {};
const parsed = JSON.parse(result.stdout) as { email?: unknown };
const parsed = JSON.parse(result.stdout) as Record<string, unknown>;
const email = typeof parsed.email === "string" ? parsed.email.trim().toLowerCase() : "";
return email && email.length <= 320 ? { email } : {};
const profileArn = profileArnFromWhoami(parsed);
return {
...(email && email.length <= 320 ? { email } : {}),
...(profileArn ? { profileArn } : {}),
};
} catch {
return {};
}
Expand Down Expand Up @@ -251,14 +275,34 @@ async function oauthCredentialFromImported(
runner: KiroCliRunner,
signal?: AbortSignal,
): Promise<OAuthCredentials> {
const identity = imported.source === "sqlite" ? await readKiroCliIdentity(runner, signal) : {};
const metadata = metadataFromImported(imported);
let identity: { email?: string; profileArn?: string } = {};
if (imported.source === "sqlite") {
identity = await readKiroCliIdentity(runner, signal);
// Session-switch race (#993 review): another process may have switched the
// active Kiro CLI session between the SQLite read and whoami. Accept
// whoami's identity only when the session token STILL matches the import —
// refresh token, or access token when refresh is absent.
if (identity.profileArn !== undefined) {
const current = readKiroCliSqliteCredential();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not borrow whoami ARNs for override DB imports

When a user imports Kiro from KIROCLI_DB_PATH/KIRO_CLI_DB_FILE, the token came from that override database, but kiro-cli whoami still reports the native CLI session. This revalidation calls readKiroCliSqliteCredential() again, which rereads the override and therefore matches the imported token, so OCX can persist account A's access/refresh token with account B's profileArn and send the wrong profile in later Kiro requests. Skip whoami profile capture for import-only DB selectors, or revalidate against the actual native CLI DB that whoami uses.

Useful? React with 👍 / 👎.

const importedKey = imported.refresh || imported.access;
const currentKey = current ? current.refresh || current.access : "";
if (!current || currentKey !== importedKey) identity = {};
}
Comment on lines +285 to +290

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Revalidate every whoami identity field.

Line 285 revalidates the SQLite session only when identity.profileArn exists. If session A is imported, the CLI switches to session B, and whoami returns only B's email, Line 289 does not run. Line 306 then returns B's email with A's credentials and any authoritative SQLite profile metadata.

Run the token comparison when either identity.email or identity.profileArn exists. Add a regression test where the session changes and whoami returns an email without a valid profile ARN.

Proposed fix
-    if (identity.profileArn !== undefined) {
+    if (identity.email !== undefined || identity.profileArn !== undefined) {
       const current = readKiroCliSqliteCredential();
       const importedKey = imported.refresh || imported.access;
       const currentKey = current ? current.refresh || current.access : "";
       if (!current || currentKey !== importedKey) identity = {};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (identity.profileArn !== undefined) {
const current = readKiroCliSqliteCredential();
const importedKey = imported.refresh || imported.access;
const currentKey = current ? current.refresh || current.access : "";
if (!current || currentKey !== importedKey) identity = {};
}
if (identity.email !== undefined || identity.profileArn !== undefined) {
const current = readKiroCliSqliteCredential();
const importedKey = imported.refresh || imported.access;
const currentKey = current ? current.refresh || current.access : "";
if (!current || currentKey !== importedKey) identity = {};
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/oauth/kiro.ts` around lines 285 - 290, Update the identity revalidation
guard in the flow around readKiroCliSqliteCredential so token comparison runs
when either identity.email or identity.profileArn is present, not only when
profileArn exists. Preserve clearing identity when the imported and current
credentials differ, and add a regression test covering a session switch where
whoami returns an email without a valid profile ARN.

}
// Builder ID imports often lack a profileArn in SQLite; whoami against the
// SAME active CLI session can supply it (#993). Imported stays authoritative.
const resolvedProfileArn = imported.profileArn ?? identity.profileArn;
const metadata: KiroOAuthMetadata | undefined = (() => {
const base = metadataFromImported(imported) ?? {};
if (resolvedProfileArn && !base.profileArn) base.profileArn = resolvedProfileArn;
return Object.keys(base).length > 0 ? base : undefined;
})();
return {
access: imported.access,
refresh: imported.refresh,
expires: imported.expires,
source: imported.source === "json" ? "credential-file" : "local-cli",
...(imported.profileArn ? { accountId: imported.profileArn } : {}),
...(resolvedProfileArn ? { accountId: resolvedProfileArn } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the existing slot when adding an ARN

When an existing Kiro account was saved before it had a profile ARN, its stored identity is usually the email. Returning the new credential with accountId set to the newly discovered ARN makes saveCredential() match on accountId ?? email, so a later ordinary login or ocx account login kiro --reauth for the same email appends a second active account instead of updating the old one; the stale profile-less row remains selectable and still fails gated Kiro models. Reconcile by email when adding the ARN to a previously email-only Kiro account, or avoid promoting the ARN to accountId until the store can merge both identities.

Useful? React with 👍 / 👎.

...(identity.email ? { email: identity.email } : {}),
...(metadata ? { kiro: metadata } : {}),
};
Expand Down
125 changes: 125 additions & 0 deletions tests/kiro-oauth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,131 @@ describe("kiro oauth — import-first", () => {
expect(existsSync(kiroCliRecoveryPath())).toBe(false);
});

test("whoami supplies a valid profileArn when the SQLite import lacks one (#993)", async () => {
const arn = "arn:aws:codewhisperer:us-east-1:123456789012:profile/ABCD1234";
seedKiroCliDb({ access_token: "aoa-builder", refresh_token: "rt-builder" });
const runner = async (args: string[]) => {
if (args[0] === "whoami") {
return { exitCode: 0, stdout: JSON.stringify({ email: "builder@example.com", profileArn: arn }) };
}
throw new Error(`unexpected Kiro CLI command: ${args[0]}`);
};

const cred = await loginKiro({}, { cliRunner: runner });

expect(cred).toMatchObject({
access: "aoa-builder",
accountId: arn,
kiro: { profileArn: arn },
});
});

test("a nested profile.arn shape is accepted (#993)", async () => {
const arn = "arn:aws:codewhisperer:eu-west-1:123456789012:profile/XY-99";
seedKiroCliDb({ access_token: "aoa-nested", refresh_token: "rt-nested" });
const nestedRunner = async (args: string[]) => {
if (args[0] === "whoami") return { exitCode: 0, stdout: JSON.stringify({ profile: { arn } }) };
throw new Error("unexpected");
};
const nested = await loginKiro({}, { cliRunner: nestedRunner });
expect(nested.accountId).toBe(arn);
});

test("a malformed whoami ARN is ignored without breaking the login (#993)", async () => {
seedKiroCliDb({ access_token: "aoa-bad", refresh_token: "rt-bad" });
const badRunner = async (args: string[]) => {
if (args[0] === "whoami") {
return { exitCode: 0, stdout: JSON.stringify({ profileArn: "not-an-arn", email: "bad@example.com" }) };
}
throw new Error("unexpected");
};
const bad = await loginKiro({}, { cliRunner: badRunner });
// Malformed ARN: login still succeeds for ungated models, but no ARN is borrowed.
expect(bad.accountId).toBeUndefined();
expect(bad.kiro?.profileArn).toBeUndefined();
});

test("an imported SQLite profileArn stays authoritative over whoami (#993)", async () => {
const sqliteArn = "arn:aws:codewhisperer:us-east-1:123456789012:profile/SQLITE";
const whoamiArn = "arn:aws:codewhisperer:us-east-1:123456789012:profile/WHOAMI";
seedKiroCliDb({ access_token: "aoa-both", refresh_token: "rt-both", profile_arn: sqliteArn });
const runner = async (args: string[]) => {
if (args[0] === "whoami") return { exitCode: 0, stdout: JSON.stringify({ profileArn: whoamiArn }) };
throw new Error("unexpected");
};
const cred = await loginKiro({}, { cliRunner: runner });
expect(cred.accountId).toBe(sqliteArn);
});

test("a session switch between the SQLite read and whoami discards whoami's ARN (#993)", async () => {
const arnB = "arn:aws:codewhisperer:us-east-1:123456789012:profile/OTHER-ACCOUNT";
seedKiroCliDb({ access_token: "aoa-accountA", refresh_token: "rt-accountA" });
const runner = async (args: string[]) => {
if (args[0] === "whoami") {
// Another process switches the active CLI session mid-flight.
removeKiroCliDb();
seedKiroCliDb({ access_token: "aoa-accountB", refresh_token: "rt-accountB" });
return { exitCode: 0, stdout: JSON.stringify({ email: "b@example.com", profileArn: arnB }) };
}
throw new Error("unexpected");
};
const cred = await loginKiro({}, { cliRunner: runner });
// Account A's token must never carry account B's ARN.
expect(cred.accountId).toBeUndefined();
expect(cred.kiro?.profileArn).toBeUndefined();
});

test("the discarded whoami identity takes the email with it, not just the ARN (#993)", async () => {
// Review finding: asserting only that the ARN is absent also passes against
// pre-fix code, which ignored whoami's ARN entirely. The email is the part
// that pre-fix code WOULD have kept, so it is the assertion that actually
// proves the mismatch path clears the whole identity.
const arnB = "arn:aws:codewhisperer:us-east-1:123456789012:profile/OTHER-ACCOUNT";
seedKiroCliDb({ access_token: "aoa-accountA", refresh_token: "rt-accountA" });
const runner = async (args: string[]) => {
if (args[0] === "whoami") {
removeKiroCliDb();
seedKiroCliDb({ access_token: "aoa-accountB", refresh_token: "rt-accountB" });
return { exitCode: 0, stdout: JSON.stringify({ email: "b@example.com", profileArn: arnB }) };
}
throw new Error("unexpected");
};
const cred = await loginKiro({}, { cliRunner: runner });
expect(cred.email).toBeUndefined();
expect(cred.kiro?.profileArn).toBeUndefined();
});

test("with no refresh token the access token is the revalidation key (#993)", async () => {
// The implementation falls back to the access token when refresh is absent.
// Without this case that branch is unexercised in either direction.
const arn = "arn:aws:codewhisperer:us-east-1:123456789012:profile/BUILDER";
seedKiroCliDb({ access_token: "aoa-only" });
const runner = async (args: string[]) => {
if (args[0] === "whoami") {
return { exitCode: 0, stdout: JSON.stringify({ email: "a@example.com", profileArn: arn }) };
}
throw new Error("unexpected");
};
const cred = await loginKiro({}, { cliRunner: runner });
expect(cred.kiro?.profileArn).toBe(arn);
});

test("an access-token-only session that changes under whoami is rejected (#993)", async () => {
const arnB = "arn:aws:codewhisperer:us-east-1:123456789012:profile/OTHER-ACCOUNT";
seedKiroCliDb({ access_token: "aoa-accountA" });
const runner = async (args: string[]) => {
if (args[0] === "whoami") {
removeKiroCliDb();
seedKiroCliDb({ access_token: "aoa-accountB" });
return { exitCode: 0, stdout: JSON.stringify({ email: "b@example.com", profileArn: arnB }) };
}
throw new Error("unexpected");
};
const cred = await loginKiro({}, { cliRunner: runner });
expect(cred.kiro?.profileArn).toBeUndefined();
expect(cred.email).toBeUndefined();
});

test("invalid recovery data names the file the operator must remove", async () => {
seedKiroCliDb({ access_token: "aoa-prior", refresh_token: "rt-prior" });
writeFileSync(kiroCliRecoveryPath(), "not a recovery database", { mode: 0o600 });
Expand Down
16 changes: 16 additions & 0 deletions tests/kiro-retry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,22 @@ describe("kiro retry fetch", () => {
expect(mock.calls).toHaveLength(1);
});

test("a profileArn-required 400 classifies as kiro_profile_required with actionable copy (#993)", async () => {
const mock = mockFetch([
new Response(JSON.stringify({
__type: "ValidationException",
message: "profileArn is required for this account",
}), { status: 400 }),
]);
const res = await fetchKiroWithRetry(request, { timeoutMs: 5_000 });
const text = await res.text();
expect(res.status).toBe(400);
expect(text).toContain("kiro_profile_required");
expect(text).toContain("ocx account login kiro --reauth");
// Non-retryable: exactly one upstream call.
expect(mock.calls).toHaveLength(1);
});

test("normalizes final transient 429 after bounded adapter retries", async () => {
const mock = mockFetch([
new Response("rate limited", { status: 429, headers: { "Retry-After": "0" } }),
Expand Down
15 changes: 15 additions & 0 deletions tests/kiro-stream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1096,6 +1096,21 @@ describe("kiro adapter — parseStream", () => {
expect(errors[0]).not.toContain("{");
});

test("an event-stream profileArn-required exception classifies as kiro_profile_required (#993)", async () => {
const payload = JSON.stringify({
__type: "ValidationException",
message: "profileArn is required for this account",
});
const frame = encodeMessage({ ":message-type": "exception", ":exception-type": "ValidationException" }, enc.encode(payload));
const errors: string[] = [];
for await (const e of createKiroAdapter(provider).parseStream(new Response(streamOf(frame)))) {
if (e.type === "error") errors.push(e.message);
}
expect(errors).toHaveLength(1);
expect(errors[0]).toContain("kiro_profile_required");
expect(errors[0]).toContain("ocx account login kiro --reauth");
});

test("auth and model exceptions become actionable Kiro errors", async () => {
const authFrame = encodeMessage(
{ ":message-type": "exception", ":exception-type": "AccessDeniedException" },
Expand Down
Loading