fix(kiro): support browser-based multi-account login - #447
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
0c73bd1 to
b25f03c
Compare
977ca7a to
48adb2b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Two read-only audit lenses returned 30 contradictions; the verified ones are applied here and recorded in 006_corrections.md. Factual corrections: - claude-code#1124 closed 2025-05-16, not 2025-08-10 - 13 open enhancement issues, not 11 - #529 is merged, so issue #42 phase 2 is done - #418 has no PR behind it; it was miscounted as in-flight - #528 does not contain #424's current head, so it needs a rebase - #527's retarget is manual and independent of #526 Framing corrections: - stale-needs-info.yml is absent from the default branch, so bundle C had no real deadline - the debug switch #543's reporter asked for already exists - whether #545 is our defect is answerable from code, not owner judgment - #491 is an OAuth credential change, so it is a security boundary rather than a warm-up; #533 and #447 keep their security classification too Structure: bundle G added for roadmap honesty, bundle F rescoped to the security boundary, #498 split out of bundle B, cycle order rebuilt, and the loop archetype corrected to decision-elicitation.
|
@Wibias 머지 차단 항목 2건을 1. 스냅샷 실패 시 logout 거부 ( 세션 저장소가 존재하지만 캡처할 수 없으면
2. 다른 계정 리전 차용 차단 ( 저장 크리덴셜이 있는데 회귀 4건
소스 수정을 stash로 되돌리면 이 중 3건이 실패하는 것을 확인했습니다(47 pass / 3 fail → 50 pass / 0 fail). 검증
참고로 문서도 갱신했습니다: 스냅샷 불가 시 로그아웃을 거부한다는 내용을 en/ko/ja/ru/zh-cn |
Wibias
left a comment
There was a problem hiding this comment.
Follow-up re-review @ b1aedd1d
Verdict: request changes for process, not for the two prior code blockers. Tip closes both merge-blocking Codex items from my last review. Before merge: rebase onto current dev, leave draft / mark ready, and keep Cross-platform CI green on the rebased tip. Maintainer security approval still follows that.
Required items from prior review
| # | Item | Status on b1aedd1d |
|---|---|---|
| 1 | Refuse logout when CLI session cannot be snapshotted | Fixed — inspectKiroCliSessionSnapshot() + blocked for unreadable / schema_mismatch / invalid_json / token_ambiguous / token_key_missing / token_found; logout only after a capturable or truly empty session. Regression: schema-mismatch DB → zero runner calls, DB preserved; empty session still proceeds. Docs mention refuse-to-sign-out when store is present but uncapturable. |
| 2 | Do not borrow another account’s refresh region | Fixed — stored credential with missing kiro metadata uses account-scoped empty {} so resolveKiroRegion pins the default instead of KIRO_REGION / local CLI import. Truly accountless refresh keeps env/local fallback. Regressions cover local CLI ap-southeast-1 and KIRO_REGION=eu-central-1. |
The two Codex items I previously marked not required (delay settlement until saveConfig; same-PID restart vs recovery owner) remain acceptable follow-ups.
Still required before merge
- Rebase onto current
dev— tip is 2 ahead / 178 behind. That is a merge blocker for this OAuth boundary PR even with the code fixes in place. - Draft → ready when the rebase is done (currently
isDraft: true; CodeRabbit is skipping for that reason). - Cross-platform CI green on the rebased tip — Ubuntu/macOS already green on this tip; finish/confirm Windows, then re-run after rebase.
- Maintainer security approval for the Kiro OAuth / CLI session mutation path.
Once 1–3 are done, re-request review; I expect to approve the code side unless rebase introduces new conflicts or regressions.
Follow-up re-review — current head
|
5e16e3b to
d972876
Compare
|
Addressed the remaining review findings in
Added focused regressions for all four review paths. The review-fix delta is one commit touching only:
Validation:
@coderabbitai review |
This comment was marked as off-topic.
This comment was marked as off-topic.
Rebased onto current upstream dev and includes all review fixes through d972876.
d972876 to
7c181cd
Compare
|
Re-review on current head
Not merge-ready until these are fixed. |
Summary
kiro-clibrowser login instead of re-importing the cached identityRoot cause
The shared provider UI already sent
addAccount: true, but the Kiro provider ignoredforceLoginand always imported the current local cache. Kiro request and refresh metadata was also resolved globally from the current local session, which could pair one stored account's token with another account's profile or region.User impact
Using Add account for Kiro now runs
kiro-cli logoutfollowed by the supportedkiro-cli loginbrowser flow, imports only the newly authenticated CLI SQLite session, and appends the stable identity to the OCX account pool. This switches the active account used bykiro-cli, while previously stored OCX accounts remain available for switching.Verification
d972876as one commitbun x tsc --noEmitbun test --isolate testsSecurity review
This changes authentication and credential handling and therefore requires explicit maintainer security review. Client registration secrets remain confined to the protected auth-store boundary and are not returned by management APIs or attached to parsed requests.
Checklist
Summary by CodeRabbit