Skip to content

fix(kiro): scope failed-login rollback - #5842

Merged
lidge-jun merged 2 commits into
devfrom
fix/kiro-rollback-ownership
Sep 25, 2026
Merged

lidge-jun merged 2 commits into
devfrom
fix/kiro-rollback-ownership

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • replace forced-login account-ID-difference rollback with an exact auth-store write receipt
  • bind compensation to the written account ID, credential generation, and selection revision
  • restore only the prior slot written by this login or remove only the slot it created
  • preserve concurrent account additions, manual selections, and later credential refreshes
  • keep compensation inside one serialized auth-store mutation
  • document the Kiro rollback ownership decision in ADR-0109

Verification

  • focused Kiro rollback regressions: 3 passed, 0 failed
  • multi-account auth-store suite: 52 passed, 0 failed, 190 assertions
  • structure SSOT gate: passed
  • privacy scan: passed
  • checks ran one at a time with CPUQuota at 75% or lower, MemoryMax at 1536 MiB or lower, no swap, low I/O weight, and bounded task counts
  • full local suite and build were intentionally not run under the conservative host ceiling; exact-head CI remains required

Security and compatibility

  • this changes credential rollback and account-selection authority and requires explicit security review
  • ordinary saveCredential callers keep the same void-returning contract; only forced Kiro login uses the receipt-bearing writer
  • receipt data remains process-local and contains no logging or management projection of credential material
  • later writes to the same account deliberately win when generation or selection ownership no longer matches
  • no Go counterpart was identified for this TypeScript OAuth path; reviewer confirmation is requested before merge

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes
    • Improved recovery when a forced Kiro login fails during account setup. Changes made concurrently—such as adding an account, selecting an account, or refreshing credentials—are preserved.
    • Failed first-time logins no longer leave behind the account created by that attempt when its credentials are still unchanged.
  • Documentation
    • Clarified how account changes are handled when forced Kiro login setup fails.

@Ingwannu
Ingwannu requested a review from lidge-jun as a code owner September 25, 2026 10:35
@Ingwannu

Copy link
Copy Markdown
Owner Author

@lidge-jun @Wibias Please review the auth-store receipt/CAS rollback boundary. This prevents a failed forced Kiro login from deleting or reselecting over a concurrent successful login. @codex security review

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 031eb458-6fc8-4276-991e-cb38d1aa7abc

📥 Commits

Reviewing files that changed from the base of the PR and between 76db92a and 6272f76.

📒 Files selected for processing (5)
  • src/oauth/index.ts
  • src/oauth/store.ts
  • structure/decisions/ADR-0109-kiro-login-rollback-ownership.md
  • structure/providers/kiro.md
  • tests/providers/kiro/kiro-review-regressions.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Forced Kiro login now records a credential-write receipt. If later persistence fails, rollback checks whether the written credential and selection state still match before restoring or removing the account. Regression tests cover concurrent account changes and failure during the first login.

Changes

Forced Kiro login rollback

Layer / File(s) Summary
Credential receipts and conditional rollback
src/oauth/store.ts
mutateStore can finalize a result after selection-revision bookkeeping. Credential saves can return a receipt with the written account, credential generation, selection revision, and prior account state. Rollback changes the store only when its matching conditions hold.
Forced-login integration and validation
src/oauth/index.ts, tests/providers/kiro/kiro-review-regressions.test.ts, structure/providers/kiro.md, structure/decisions/ADR-0109-kiro-login-rollback-ownership.md
Forced Kiro login uses receipt-based saving and conditional rollback when provider publication fails. Tests cover concurrent account addition, a refreshed credential, and a failed first login. Documentation records the rollback contract.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 6272f

The reported first-login rollback issue is addressed and covered by a regression test. No concrete merge-blocking risk remains beyond normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: limiting failed Kiro login rollback to the credential write owned by that login.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 25, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-25T10:38:07.182723Z 062ee79 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 062ee79f4a

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/oauth/store.ts
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 68 / 80

이 PR은 Kiro 강제 로그인이 계정은 저장했는데, 그다음 설정 기록이 실패했을 때 방금 쓴 칸만 되돌리게 바꿉니다.

예전에는 로그인 전에 있던 계정 번호만 기억했습니다. 실패하면 그 뒤에 생긴 번호를 전부 지우고, 예전 활성 계정으로 다시 골랐습니다. 같은 시각에 다른 로그인이 성공해도 그 계정까지 지울 수 있었습니다.

이제는 저장할 때 영수증을 만듭니다. 영수증에는 이번에 쓴 계정 번호, 토큰 내용으로 만든 해시(credentialGeneration), 고른 계정의 버전 번호(selectionRevision), 그 칸의 이전 내용이 들어갑니다. 실패하면 저장소가 그 영수증과 아직 같을 때만 그 칸을 지우거나 예전 내용으로 되돌립니다. 그 사이에 다른 로그인이 같은 칸을 바꿨으면 손대지 않습니다. 일반 저장 함수는 예전처럼 반환값이 없고, 강제 Kiro 로그인만 이 영수증을 씁니다. 이미 계정이 있는 경우는 테스트로 확인됩니다.

src/oauth/store.ts:850 - Kiro 계정이 하나도 없을 때 새 묶음을 만드는데, 여기에는 selectionRevision이 없습니다.
src/oauth/store.ts:899 - 영수증은 그 직후의 버전 번호를 적습니다. 새 묶음이라 값은 undefined입니다.
src/oauth/store.ts:795 - 그 다음 mutateStore가 디스크에 쓰기 직전에 새 UUID를 붙입니다. 영수증에는 이 UUID가 없습니다.
src/oauth/store.ts:928 - 롤백은 활성 계정이 같은데 버전 번호가 다르면 다른 사람이 고른 것으로 보고 stale을 반환한 뒤 아무것도 지우지 않습니다. 계정이 0개인 상태에서 강제 로그인이 실패하면, 방금 받은 토큰이 auth.json에 남습니다.
tests/providers/kiro/kiro-review-regressions.test.ts:256 - 새 테스트는 이미 계정 A가 있는 경우만 봅니다. 빈 저장소에서 실패하는 경우는 없습니다.

메인테이너의 판단이 필요한 지점

첫 계정 실패 때 토큰이 디스크에 남는 문제를 고치기 전에 머지할지 정해 주세요. 작성자가 요청한 대로, 이 TypeScript OAuth 경로에 대응하는 Go 코드가 있는지도 확인해 주세요. ADR-0109는 버전 번호가 어긋나면 실패한 로그인의 토큰을 일부러 남기기로 했습니다. 이번 빈 저장소 경우는 다른 로그인이 고친 것이 아니라, 같은 저장이 영수증을 만든 뒤에 번호를 붙인 것입니다. 그 정책에 넣을 일이 아닙니다.

너의 추천

영수증을 만들기 전에 새 묶음에도 실제로 남을 selectionRevision을 넣으세요. 넣기 어렵으면 mutateStore가 번호를 고친 뒤 그 값을 영수증에 다시 적으세요. 계정이 없는 상태에서 강제 로그인이 실패하면 그 계정이 사라지는 테스트를 추가하세요. 그 테스트가 통과하기 전에는 머지하지 마세요.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun
lidge-jun merged commit 5468a9d into dev Sep 25, 2026
33 of 35 checks passed
@lidge-jun
lidge-jun deleted the fix/kiro-rollback-ownership branch September 25, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants