fix: keep tailnet phone sessions across updates - #6660
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound identity-binding design, but QR minting is now fused to the identity-trust mutation with no degraded path when its prerequisites can't be met. Watch
Suggestions
[DESIGN-REVIEWED] 3423ee4 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/components/TailnetMobileCard.tsx:325 -- after resolving an overlay conflict with timed sessions enabled, [GPT-REVIEWED] 3423ee4 False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of I have everything I need. The i18n keys resolve (pre-existing in UX-Verdict: CONCERNS During the automated restart the card shows the imperative "Restart Kiro Crew to finish" — an instruction to do what the system is already doing. Watch
Suggestions
[UX-REVIEWED] 3423ee4 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Every item traces to the reported defect — phone sessions dying at restart — and the fix lands at cause level: the device binding moves from restart-mortal memory into the signed credential. What this change shipsIntent: FIX — configure phone access once and keep the same identity-bound session across updates, without letting another tailnet node claim the surviving cookie.
Duplication was the real question and it fails to materialize: the only existing mechanism, [FIRST-PRINCIPLES-REVIEWED] 3423ee4 |
7e7e23c to
cba4161
Compare
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBased on my independent falsification of the discovery pass's conclusion and my own review of the changed lines, I could not ground any defect meeting the (a)/(b)/(c) bar at 80+. Key paths I re-derived rather than inherited:
The en No findings. [OPUS-REVIEWED] 3423ee4 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
98340b0 to
a54364f
Compare
|
Review follow-up for
Local evidence: focused backend 489 passed / 1 skipped; expanded auth suite 566 passed / 3 skipped; Tailnet card 44 passed; TypeScript, scoped ESLint/flake8, Black/isort, mypy, and docs lint passed. Two independent final-diff security reviews returned PASS. |
Configure guided phone access with the daemon-reported Tailscale identity so scanned sessions can survive gateway restarts safely. Verify forwarded peers through the Windows Tailscale CLI and wait for the replacement gateway before minting the QR session.
a54364f to
3423ee4
Compare
|
CI follow-up on \3423ee43fb1590360839788b2d6e40ba111e919c: removed the obsolete \components.tailnetMobile.show_qr\ entry from the English, pseudolocale, and all shipped locale catalogs after the renamed setup action made it dead. Local verification now passes: \deadKeys.test.ts\ 3/3, \TailnetMobileCard.test.tsx\ 44/44, and all 19 i18n checks with \I18N_BASE_REF=origin/main. The PR remains one commit. |
|
Final CI status for \3423ee43fb1590360839788b2d6e40ba111e919c: all 66 check runs completed with 62 successes, 4 expected skips, 0 pending, and 0 failures. The transient \setup-uv\ timeout was rerun successfully; its backend shard and the dependent coverage combine/gate both passed. Automated GPT 5.6 and Opus 4.8 reviews report no blocking findings. The remaining branch-protection requirement is one human approval. |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Problem / Motivation Phone access configured through Tailscale could stop working after a Kiro Crew update and ask the user to run
kirocrew tokenagain. The earlier identity-pinning work in #6485 made restart-persistent sessions possible, but the guided setup did not durably enable all prerequisites, Windows skipped the daemonwhoispath, and update/restart handling still left boot-bound or unverified sessions behind. ## Why it matters An ordinary user should configure phone access once and keep using the same authenticated session across Nightly updates and gateway restarts. Requiring a fresh terminal token after every update breaks the one-click mobile setup. Persistence also has to remain identity-bound: a Tailscale daemon outage or a second allowlisted node must never be able to claim a surviving cookie. ## What changed (motivation → approach → change) - The owner-only Phone access setup now performs one locked, atomic config mutation that enables Tailscale, enrolls the daemon-reported local login without replacing existing allowlist entries, enables identity trust, and selects restart-persistent QR sessions. - The handler reloads the merged effective config after writing. Aconfig.local.jsonoverride that prevents safe persistence now returns409 config_overlay_conflictwith the conflicting fields instead of reporting a false success. A retry also compares the running gateway trust snapshot so a previously written base config still triggers the required restart. - Update/restart completion is fenced by a changed gateway boot ID before the UI publishes or mints a QR. The card exposes setup/restart progress instead of hiding a restart behind a read-only-looking action. - Windows now participates in forwarded-peertailscale whoisresolution. - A persistent QR initially acts as a five-minute enrollment bearer. After the first verified redemption, both access and refresh cookies carry the original peer key inside the existing HMAC-signed payload. Every request, refresh, grace replay, and child credential mint preserves and verifies that exact key and its issuance scope, so a restart can only rehydrate the in-memory pin for the original device—not whichever allowed node arrives first. -require_peer=1fails closed when identity is unavailable or the signed device key mismatches. Unsafe legacy claimless persistent cookies/refresh chains require one fresh QR scan rather than silently taking ownership after restart. - Documentation and focused backend/frontend regression coverage were updated. ## Tests -py -3.12 -m pytest -q -n 4 test/test_tailnet_mobile.py test/test_token_auth.py test/test_tailnet_peer.py test/test_auth_refresh_handlers_cov80.py— 489 passed, 1 skipped. - Expanded run including refresh-token and delegated-mobile-link suites — 566 passed, 3 skipped. -npx vitest run src/components/TailnetMobileCard.test.tsx— 44 passed. -npx tsc -bandnpm run build— production build completed (6,788 modules). - CI-equivalent Black ratchet, isort, scoped flake8, and focused mypy checks — passed. -py -3.12 scripts/docs_lint.py— passed (242 Markdown files). - A broader local backend run completed 67,709 tests successfully with 3,000 skipped and 6 xfailed; its 21 failures were host/environment cases outside this diff (primarily Windows symlink privilege, plus existing broker/PPTX environment cases). CI remains the authoritative full-suite run. ## Manual verification Validated on the affected Windows Nightly host: - Tailscale status exposed both the local login and node identity. - Effective config had identity trust enabled, one allowed login, restart persistence enabled, and the normal until-restart mode enabled. - Restarted the gateway and confirmed the phone reconnected without runningkirocrew tokenagain. ## Screenshots / video Why no screenshot: there is no layout or styling change. The existing Phone access card only changes its ready-action copy to “Set up & show QR” and reuses its current inline status treatment while setup/restart is pending; the substantive change is persistence and authentication behavior. ## Related Issues Follow-up to #6485, which introduced the earlier Tailscale identity-pinning fix. No linked issue. ## Checklist - [x] At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...) - [x] Existing tests pass and new tests added for new functionality - [x] Self-review completed; code follows project style guidelines - [x] Documentation updated (if applicable) - [x] No secrets, credentials, or internal references in the diff