fix(kiro): Add direct API fetch for enterprise/IdC accounts#2164
fix(kiro): Add direct API fetch for enterprise/IdC accounts#2164stormrise wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb3fa52536
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| defer { sqlite3_close(db) } | ||
|
|
||
| let query = "SELECT value FROM auth_kv WHERE key = 'kirocli:odic:token'" |
There was a problem hiding this comment.
For current Kiro CLI logins the token row is stored under kirocli:social:token (and IdC/OIDC installs use kirocli:oidc:token); this query only looks for the misspelled kirocli:odic:token. In those environments resolveCredentials() returns nil, so the new API strategy is marked unavailable and auto mode falls straight back to the old CLI path, leaving the enterprise/IdC failure this change is meant to fix. Please try the real key names and keep the typo only as a fallback if needed.
Useful? React with 👍 / 👎.
| pipeline: ProviderFetchPipeline(resolveStrategies: { _ in | ||
| [KiroAPIFetchStrategy(), KiroCLIFetchStrategy()] |
There was a problem hiding this comment.
Because the resolver ignores context.sourceMode, selecting Kiro's CLI source still returns the API strategy first; if the SQLite token is present the app will make the direct HTTP request even though the user requested CLI. The opposite can also happen after an API failure because KiroAPIFetchStrategy.shouldFallback always returns true, so an explicit API refresh can report a CLI result. Please switch on the source mode here and limit cross-source fallback to .auto.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 16, 2026, 10:49 PM ET / July 17, 2026, 02:49 UTC. Summary Reproducibility: no. high-confidence live reproduction is available in the maintainer environment. The source path, upstream profileArn incompatibility, linked report, and before screenshot support the failure, but the exact enterprise credential class was not demonstrated. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep automatic API use narrowly limited to the affected enterprise credential classes, obtain credentials through a host-owned injectable boundary, preserve explicit CLI behavior and safe fallback, document the source contract, and verify fresh and expired IdC and external-IdP accounts with redacted runtime evidence. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction is available in the maintainer environment. The source path, upstream profileArn incompatibility, linked report, and before screenshot support the failure, but the exact enterprise credential class was not demonstrated. Is this the best way to solve the issue? No, not yet. A narrowly scoped direct API path is reasonable, but the credential store should be host-owned and the latest authentication and refresh behavior needs account-specific runtime proof before this is the safest maintainable solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a264acaddab9. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (2 earlier review cycles)
|
|
before: after fix: to fix this issue |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@chatgpt-codex-connector re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0af8d2adf9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| guard let data = jsonString.data(using: .utf8), | ||
| let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any], | ||
| let accessToken = json["access_token"] as? String, | ||
| !accessToken.isEmpty |
There was a problem hiding this comment.
When the SQLite credential's access_token has expired, this strategy still sends it because it only reads access_token and ignores the row's expiry/refresh credentials. The API will return 401 and, in auto mode, falls back to the CLI path that this change is specifically trying to bypass, so enterprise/IdC usage stops working again after the cached token ages out unless the user manually logs in to refresh it.
Useful? React with 👍 / 👎.
| // MARK: - API Response Models | ||
|
|
||
| struct KiroUsageLimitsResponse: Decodable { | ||
| let nextDateReset: Double? |
There was a problem hiding this comment.
When getUsageLimits includes nextDateReset as an ISO-8601 string, the synthesized decoder for this Double? throws before usage can be parsed; the dateDecodingStrategy set on the decoder does not apply because the model is not a Date. That makes otherwise successful 200 responses fail and fall back to the broken CLI path for the affected Kiro responses.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: 📸 screenshot
|
Thanks for finding the upstream
Local verification on exact head
The screenshots show timeout-before and usage-after, but do not establish account type, selected source, token-refresh behavior, or the external-IdP header. Live enterprise/IdC verification is unavailable in this maintainer environment. Please add redacted proof for those facts after the source/test blockers above are fixed. Leaving the PR open for revision. |
…sage Fixes Kiro usage fetching for enterprise/IdC accounts where kiro-cli fails because it passes profileArn to the legacy REST API, causing a 400 error. The fix adds a new KiroAPIFetchStrategy that: - Reads credentials directly from kiro-cli's SQLite database - Makes direct HTTP calls to AWS Q getUsageLimits API - Uses fixed KiroIDE-0.9.2 version (doesn't require profileArn) - Falls back to CLI strategy if API fails This is the same approach used by kiro.rs (v0.6.11) which correctly handles enterprise accounts by NOT passing profileArn to usage APIs. Reference: https://github.com/ZyphrZero/kiro.rs/issues/XX Root cause: Enterprise credentials have a profileArn that gets resolved via ListAvailableProfiles. When passed to getUsageLimits, it causes 'Improperly formed request' 400 error.
- Add source mode handling: auto (API+CLI), api (API only), cli (CLI only) - Support multiple credential keys: kirocli:odic:token, kirocli:oidc:token, kirocli:social:token - shouldFallback only in auto mode, not when explicit source selected
Refactor the direct fetch path into injectable credential and HTTP seams, add token refresh and EXTERNAL_IDP handling, narrow auto mode to enterprise credentials, and cover the new behavior with focused tests.
0af8d2a to
27dc245
Compare
|
@clawsweeper @chatgpt-codex-connector re-review |
|
To use Codex here, create a Codex account and connect to github. |
2 participants
Problem
Kiro usage fetching fails for enterprise/IdC accounts because passes to the legacy REST API, causing a error.
Root Cause
Enterprise credentials have a that gets resolved via . When this ARN is passed to , the Kiro 0.9.2 version of the API rejects it as malformed. The same issue was fixed in kiro.rs v0.6.11.
Solution
Added a new that:
Changes
Testing
Reference