Skip to content

fix(gui): bound Kiro status reconciliation - #6025

Closed
Ingwannu wants to merge 1 commit into
devfrom
fix/6021-kiro-status-deadline
Closed

Ingwannu wants to merge 1 commit into
devfrom
fix/6021-kiro-status-deadline

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • replace cloned Kiro status Response bodies with one shared status-read operation that owns fetch, bounded stream consumption, parsing, and cancellation
  • transfer that operation from the React hook to the detached finalizer so an already-sent terminal reply can still win after close
  • enforce the 45-second complete-read ceiling, 64 KiB body cap, existing flow-wide deadline, clamped retry sleep, and already-expired handoff cancellation
  • document the ownership decision and neutral timeout behavior

Closes #6021.

Why

The merged GUI bounded only Promise<Response> acquisition. Once headers arrived, both the hook original and finalizer clone could wait forever in response.json(), bypassing the 45-second/16-minute claims and retaining the finalizer singleflight entry. Aborting on unmount alone would lose a terminal status already in flight, so the fix transfers one parsed operation rather than starting competing body consumers.

Validation

All local commands ran one at a time in transient user units with CPUQuota=75%, MemoryHigh=1G, MemoryMax=1536M, swap disabled, and disposable HOME/CODEX_HOME/OPENCODEX_HOME/TMPDIR; structure/privacy gates used 50% CPU and 512 MiB max.

  • cd gui && bun test ./tests/kiro-device-login.test.tsx — 27 pass, 0 fail
  • cd gui && node ./node_modules/typescript/bin/tsc -b --pretty false — pass; 515.8 MiB peak
  • bun run structure:check — pass
  • bun run privacy:scan — pass
  • bun scripts/file-size-ratchet.ts — pass
  • git diff --check — pass

A preliminary Bun-as-Node TypeScript invocation stopped making progress in a futex wait and was terminated inside its capped transient unit; the canonical Node compiler command above completed cleanly. No full repository suite, full build, live service restart, or production-home command was run.

Scope

This deliberately fixes status polling/handoff only. Initial-login and cancellation response bodies keep their existing behavior, as recorded in ADR-6021.

Summary by CodeRabbit

  • Bug Fixes

    • Closing the Kiro device login dialog now allows an already-in-progress status response to finish, so a confirmed login can still be recognized after cancellation.
    • Stalled status checks are bounded and cancelled when their time limit expires, preventing them from delaying the final outcome indefinitely.
    • If the login flow expires before a status check completes, the outcome is reported as ended rather than successful.
  • Documentation

    • Clarified how status checks and cancellation behave when closing the Kiro device login dialog.

@Ingwannu
Ingwannu requested a review from lidge-jun as a code owner September 27, 2026 01:39
@Ingwannu

Copy link
Copy Markdown
Owner Author

@lidge-jun review requested on exact head cdc0e7979f. The original merged-body hang, uncooperative fetch, delayed EOF handoff, overall deadline, already-expired handoff, and active-entry cleanup cases are covered. Local focused tests/typecheck/gates are green under the CPU/RAM caps documented in the PR; waiting for hosted exact-head CI before merge consideration.

@coderabbitai

coderabbitai Bot commented Sep 27, 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: 456ce2c8-cdea-4609-a2aa-3f7a840ea701

📥 Commits

Reviewing files that changed from the base of the PR and between d8b85ad and cdc0e79.

📒 Files selected for processing (7)
  • devlog/_plan/260926_kiro_lb_parity2/100_gui_device_login_and_skip_reason.md
  • docs-site/src/content/docs/guides/providers.md
  • gui/src/components/use-kiro-device-login.ts
  • gui/src/kiro-device-login-finalizer.ts
  • gui/tests/kiro-device-login.test.tsx
  • structure/decisions/ADR-6021-kiro-status-read-ownership.md
  • structure/gui-and-management-api.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The Kiro login flow now uses one cancellable status read for fetching, consuming, and parsing status responses. Closing the dialog transfers an in-flight read to the detached finalizer. Read and flow deadlines bound the operation, and tests cover stalled bodies and cancellation.

Changes

Kiro status reconciliation

Layer / File(s) Summary
Status-read and finalizer deadlines
gui/src/kiro-device-login-finalizer.ts, devlog/_plan/260926_kiro_lb_parity2/100_gui_device_login_and_skip_reason.md, structure/decisions/ADR-6021-kiro-status-read-ownership.md
readKiroDeviceStatus owns the fetch, body read, JSON parsing, and view validation. It retries invalid or stalled reads, returns missing for a 404, and uses a 45-second default timeout with a 64 KiB body limit. The finalizer awaits this read within the flow deadline and cancels it if the deadline wins.
Hook handoff and regression coverage
gui/src/components/use-kiro-device-login.ts, gui/tests/kiro-device-login.test.tsx, docs-site/src/content/docs/guides/providers.md, structure/gui-and-management-api.md
The hook tracks the shared status read and aborts its closure signal when the session closes. Polling consumes the reader’s result instead of fetching and parsing directly. Tests cover terminal replies after unmount, stalled and late bodies, and expired finalizer deadlines. The documentation describes cancellation and bounded background reconciliation.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Hook as Kiro login hook
  participant Reader as readKiroDeviceStatus
  participant Endpoint as Status endpoint
  participant Finalizer as finalizeKiroDeviceFlow
  Hook->>Reader: Start status read
  Reader->>Endpoint: Fetch status
  Endpoint-->>Reader: Response body
  Hook->>Finalizer: Transfer in-flight read on close
  Reader-->>Finalizer: Parsed status result
  Finalizer->>Reader: Cancel read if deadline expires
Loading

Merge Risk: ⚪ Minimal · up to cdc0e

The status-read handoff and deadline changes have no identified merge-blocking issue. The reported checks should be confirmed in exact-head CI before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to cdc0e

The shared read appears to reduce the risk of stalled login reconciliation without expanding who can read or cancel a flow. No new security issue was established, but production behavior and consumers outside this repository remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed client path reconciles the same flow ID through the existing status and cancellation routes; the inspected server ownership check limits access to that principal’s flow.

Security Findings and Attack Paths

  • observed — A status response must finish within the read ceiling and body cap before it can become a validated view; invalid or incomplete bodies yield a retry rather than a terminal credential outcome.

Trust Boundaries and Controls

  • observed — The finalizer rejects a returned view for a different flow ID, while the server independently enforces flow ownership on status and cancellation.

Resilience and Maintainability Implications

  • observed — Cancellation settles the shared result once even if transport cancellation is cooperative; a late response body is cancelled, and an expired handoff cancels its inherited read.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (4 skipped: 4… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR satisfies the coding requirements in issue #6021. gui/src/kiro-device-login-finalizer.ts adds readKiroDeviceStatus, which owns fetch, streamed body completion, JSON parsing, validation, the…
Out of Scope Changes check ✅ Passed The changes remain within issue #6021. The hook and finalizer changes implement status-read ownership, timeout, cancellation, handoff, retry, and cleanup behavior. The added tests verify those behavio…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: bounding Kiro status reconciliation, including the status-read and finalizer timeout behavior.
Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (4 skipped: 4 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 github-actions Bot added the bug Something isn't working label Sep 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • this PR is ready for review.

UI screenshot waived by the gui-screenshot-waived label.

@github-actions
github-actions Bot marked this pull request as draft September 27, 2026 01:46
@Ingwannu Ingwannu added the gui-screenshot-waived Maintainer waiver for false-positive GUI screenshot requirements label Sep 27, 2026
@Ingwannu

Copy link
Copy Markdown
Owner Author

Applied gui-screenshot-waived: this patch changes only Kiro status-fetch ownership, stream deadlines, and detached reconciliation. It does not alter rendered components, copy, layout, styles, or visual states, so a screenshot cannot demonstrate the corrected lifecycle. Behavioral proof is in the stalled-stream/unmount/deadline tests.

@github-actions
github-actions Bot marked this pull request as ready for review September 27, 2026 01:48
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 30 / 80

이 PR은 Kiro 기기 로그인 상태 확인이 끝나지 않고 남는 문제를 고칩니다.

예전 코드는 응답 헤더가 도착하면 45초 제한이 풀렸습니다. 그 다음 본문을 읽는 response.json()에는 제한이 없었습니다. 서버가 본문을 닫지 않으면, 화면의 확인 루프와 창을 닫은 뒤 결과를 이어 받는 쪽이 그 자리에서 기다렸습니다. 이어 받는 쪽의 진행 항목도 목록에서 빠지지 않았습니다.

지금은 상태 읽기 함수 하나가 요청, 본문 읽기, JSON 해석, 공개 화면 값 검사를 맡습니다. 한 번의 읽기는 45초를 넘기지 않습니다. 본문이 64KiB를 넘으면 그 읽기는 다시 시도로 끝납니다. 서버가 취소를 무시해도 45초가 되면 결과는 다시 시도입니다. 창을 닫으면 진행 중인 읽기 하나를 이어 받는 쪽에 넘깁니다. 이미 보낸 요청의 본문이 마감 전에 done으로 끝나면 계정 추가로 처리합니다. 흐름 마감이 먼저면 그 읽기를 취소합니다. 다시 시도하기 전 잠은 남은 시간과 2초 중 짧은 쪽입니다. 마감이 이미 지난 뒤에 넘어온 읽기는 새 폴링 없이 취소하고 ended로 끝냅니다.

로그인 시작 요청과 취소 요청의 본문은 그대로입니다. ADR-6021이 상태 확인만 이번 범위로 적습니다. 베이스는 dev입니다. #6021을 닫는 다른 열린 PR은 없습니다. 헤드 cdc0e7979f에서 test 1/4부터 4/4, gates, structure gate는 통과했습니다.

라인 - gui/src/components/use-kiro-device-login.ts 133행. 창이 열려 있을 때의 읽기는 항상 45초입니다. 127행에서 만료를 확인한 뒤에 읽기를 시작합니다. 읽는 동안 만료 시각이 지나도 그 읽기는 계속됩니다. 화면의 expired는 다음 바퀴에 나옵니다. 늦게 잡히면 약 45초입니다. gui/src/kiro-device-login-finalizer.ts 127행은 남은 흐름 시간과 45초 중 짧은 쪽을 씁니다.

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

창이 열린 동안의 45초를 만료 시각에 맞춰 줄일지입니다. 줄이면 만료 화면은 빨라집니다. 만료 직전에 이미 보낸 done 본문을 창이 직접 받는 시간은 짧아집니다.

로그인 시작과 취소 응답도 같은 본문 제한을 둘지입니다. 그 둘은 이번 이슈 범위 밖에 있습니다.

너의 추천

머지하세요. 베이스는 dev로 두세요. 닫을 중복 PR은 없습니다. 133행의 45초는 #6021의 무한 대기를 이미 끊습니다. 만료 시각에 맞추는 일은 다음으로 둬도 됩니다. 로그인 시작과 취소 본문은 다음 이슈로 두세요.

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

@Ingwannu

Copy link
Copy Markdown
Owner Author

@lidge-jun exact head cdc0e7979f5f28a7d6e675ae8b5cba9d2fb905cc is fully green in Cross-platform CI 36286199813, including all four test shards, gates, docs/structure, packaging, and desktop shell. The screenshot waiver gate now passes, the independent lifecycle review is addressed, and there are no unresolved review threads. Maintainer review/approval can proceed.

@Ingwannu
Ingwannu requested a review from luvs01 September 27, 2026 02:54
lidge-jun added a commit that referenced this pull request Sep 27, 2026
Merge train round 3 B7: GUI bug fixes (#6025 #6010 #6007)
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in #6070 (merge 29cef45a86) as one squashed commit that keeps your authorship. Thank you. Closing because this repository merges into dev, so GitHub does not close carried PRs automatically.

@lidge-jun lidge-jun closed this Sep 27, 2026
luvs01 pushed a commit to luvs01/opencodex that referenced this pull request Sep 27, 2026
Carried from lidge-jun#6025 into merge train round 3.

Co-authored-by: Ingwannu <ingwannu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working gui-screenshot-waived Maintainer waiver for false-positive GUI screenshot requirements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants