Fix isolated Claude subscription login on macOS - #23
Open
Him188 wants to merge 1 commit into
Open
Conversation
Reviewer's GuideImplements a dedicated Claude subscription login helper that runs the official OAuth flow, snapshots macOS Keychain-backed credentials into per-subscription directories with atomic, local-only fallbacks, and threads new localOnly/shippable semantics plus precise per-attempt login status through the server, host probing, WS spawning, and UI layers, with regression tests and documentation updates. Sequence diagram for macOS Claude subscription login via helpersequenceDiagram
actor User
participant WebUI as ManageAgents_UI
participant WS as ws_handler
participant Host as RemoteHost_shell
participant Helper as vibespace_claude_subscription_login.mjs
participant Claude as claude_CLI
participant Keychain as macOS_Keychain
User->>WebUI: Click "Add subscription…" (remote host)
WebUI->>WebUI: remoteClaudeSubscriptionLoginCommand(id)
WebUI->>WS: openShellTerminal(initialCommand=login.command)
WS->>WS: detect needsClaudeLoginHelper
WS->>Host: deviceAgentSetup (ship vibespace-claude-subscription-login.mjs)
Host->>Helper: run initialCommand
Helper->>Helper: parseArgs(--config-dir, --claude, --attempt)
Helper->>Helper: writeLoginStatus(state=running, attempt)
Helper->>Claude: runLogin(configDir, claudeCmd)
Claude-->>Helper: auth login --claudeai succeeds
alt platform is darwin
Helper->>Keychain: readMacOSKeychain(configDir)
Keychain-->>Helper: claudeAiOauth JSON
Helper->>Helper: writeCredentialsFile(configDir, credentials)
else non-darwin
Helper->>Helper: readCredentialsFile(configDir)
end
Helper->>Helper: writeLoginStatus(state=success, attempt)
loop poll host status
WebUI->>WS: GET /api/hosts/{id}/accounts-status
WS->>Host: HostManager.probeHostStatus
Host-->>WS: hostSubLoginStatus[accountId]
WS-->>WebUI: { hostSubLoginStatus }
WebUI->>WebUI: _watchHostLogin(hostId, accountId, loginAttempt)
alt status.state == success and attempt matches
WebUI->>WebUI: complete(false)
else status.state == error
WebUI->>User: showToast("Subscription login could not be saved…")
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Him188
marked this pull request as ready for review
July 28, 2026 16:36
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
needsClaudeLoginHelperdetection inws-handlercurrently relies onString(data.initialCommand || '').includes('/vibespace-claude-subscription-login.mjs'), which is fairly brittle; consider passing an explicit flag in the spawn request instead so future changes to the command string don’t silently bypass helper shipping/enforcement. - In
shellQuote, rejecting any control characters in the paths (including newlines) is good for safety but will throw at runtime if a config dir or binary path ever contains one; if that’s a realistic risk, you might want to validate/sanitize earlier when these paths are configured so the error surfaces closer to the source.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `needsClaudeLoginHelper` detection in `ws-handler` currently relies on `String(data.initialCommand || '').includes('/vibespace-claude-subscription-login.mjs')`, which is fairly brittle; consider passing an explicit flag in the spawn request instead so future changes to the command string don’t silently bypass helper shipping/enforcement.
- In `shellQuote`, rejecting any control characters in the paths (including newlines) is good for safety but will throw at runtime if a config dir or binary path ever contains one; if that’s a realistic risk, you might want to validate/sanitize earlier when these paths are configured so the error surfaces closer to the source.
## Individual Comments
### Comment 1
<location path="src/accounts.js" line_range="179-185" />
<code_context>
} catch { return { loggedIn: false }; }
}
+ _subscriptionLoginStatus(id) {
+ try {
+ const status = JSON.parse(fs.readFileSync(path.join(this.subDir(id), '.vibespace-login-status.json'), 'utf-8'));
+ if (status?.state !== 'error' || !/^[a-z0-9-]{1,40}$/.test(status.code || '')) return null;
+ return { state: 'error', code: status.code };
+ } catch { return null; }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Login-status code validation is stricter than the writer and may silently ignore future error codes.
`_subscriptionLoginStatus` only returns errors whose `code` matches `/^[a-z0-9-]{1,40}$/`, but `writeLoginStatus` can persist any trimmed string and current callers already use values like `claude-login-exit`. If future codes add characters like `_` or uppercase, they’ll be written but then silently ignored, so `finalizeSubscription` will never see `loginFailed` for those cases.
To avoid this mismatch, either broaden the regex to match what `writeLoginStatus` can emit (e.g. `[A-Za-z0-9._-]{1,40}`) or drop the format check and just require `status.state === 'error'` and a non-empty `code`.
```suggestion
_subscriptionLoginStatus(id) {
try {
const status = JSON.parse(
fs.readFileSync(
path.join(this.subDir(id), '.vibespace-login-status.json'),
'utf-8',
),
);
if (
status?.state !== 'error' ||
!/^[A-Za-z0-9._-]{1,40}$/.test(status.code || '')
) {
return null;
}
return { state: 'error', code: status.code };
} catch {
return null;
}
}
```
</issue_to_address>
### Comment 2
<location path="src/lib/manage-agents.js" line_range="141" />
<code_context>
// (§ban-safety) — until the credential files CHANGE vs the pre-login
// snapshot, then brings the Agents surface back on the SAME machine.
- _watchHostLogin(hostId, hostLabel) {
+ _watchHostLogin(hostId, hostLabel, accountId = null, loginAttempt = null) {
if (!hostId) return;
if (this._hostLoginWatch) { clearInterval(this._hostLoginWatch); this._hostLoginWatch = null; }
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the per-attempt and global polling paths plus the shared completion behavior in `_watchHostLogin` into separate helper methods to make the logic clearer and flatter.
You can simplify `_watchHostLogin` by making the dual-mode logic and the `complete` side effects explicit helpers. That keeps the polling loops single-purpose and flattens the nested conditions without changing behavior.
Example refactor:
```js
_watchHostLogin(hostId, hostLabel, accountId = null, loginAttempt = null) {
if (!hostId) return;
if (this._hostLoginWatch) {
clearInterval(this._hostLoginWatch);
this._hostLoginWatch = null;
}
if (accountId && loginAttempt) {
this._watchHostLoginForAttempt({ hostId, hostLabel, accountId, loginAttempt });
} else {
this._watchHostLoginGlobal({ hostId, hostLabel });
}
}
```
Then split out the two polling modes and the shared completion:
```js
_completeHostLoginWatch({ hostId, hostLabel, machineLoginChanged }) {
clearInterval(this._hostLoginWatch);
this._hostLoginWatch = null;
if (machineLoginChanged) (this._hostLoginSeenAt ||= {})[hostId] = Date.now();
if (this._agentsHostPref && this._agentsHostPref !== hostId) {
showToast(t('✓ Login on {host} updated', { host: hostLabel }), { duration: 5000 });
return;
}
showToast(t('✓ Login on {host} updated — reopening Agents there', { host: hostLabel }), { duration: 5000 });
this._agentsHostPref = hostId;
if (!this._agentsRefreshHook?.(hostId)) this._showAgentsDialog();
}
_watchHostLoginForAttempt({ hostId, hostLabel, accountId, loginAttempt }) {
let tries = 0;
this._hostLoginWatch = setInterval(async () => {
if (++tries > 50) { clearInterval(this._hostLoginWatch); this._hostLoginWatch = null; return; }
let cur;
try { cur = await fetchJson(`/api/hosts/${encodeURIComponent(hostId)}/accounts-status`); } catch { return; }
const loginStatus = cur?.hostSubLoginStatus?.[accountId];
if (!loginStatus || loginStatus.attempt !== loginAttempt || loginStatus.state === 'running') return;
if (loginStatus.state === 'error') {
clearInterval(this._hostLoginWatch); this._hostLoginWatch = null;
showToast(t('Subscription login could not be saved. Check the login terminal for details, then try again.'), { type: 'error', duration: 8000 });
return;
}
if (loginStatus.state === 'success') {
this._completeHostLoginWatch({ hostId, hostLabel, machineLoginChanged: false });
}
}, 6000);
}
_watchHostLoginGlobal({ hostId, hostLabel }) {
const sig = (r) => (r && !r.error)
? [r.credsMtime || 0, r.codexAuthMtime || 0, r.subscription?.loggedIn ? 1 : 0, r.subscription?.email || '', r.codex?.email || '', (r.hostSubs || []).join('+')].join('|')
: null;
let baseSig = null;
let tries = 0;
this._hostLoginWatch = setInterval(async () => {
if (++tries > 50) { clearInterval(this._hostLoginWatch); this._hostLoginWatch = null; return; }
let cur;
try { cur = await fetchJson(`/api/hosts/${encodeURIComponent(hostId)}/accounts-status`); } catch { return; }
const s = sig(cur);
if (s === null) return;
if (baseSig === null) { baseSig = s; return; }
if (s === baseSig) return;
const machinePart = (x) => x.split('|').slice(0, 5).join('|');
const machineLoginChanged = machinePart(s) !== machinePart(baseSig);
this._completeHostLoginWatch({ hostId, hostLabel, machineLoginChanged });
}, 6000);
}
```
This keeps all current behaviors (including the per-attempt short-circuiting and machine-login-change stamp) but makes:
- The two modes (`attempt` vs global fingerprint) explicit and mutually exclusive.
- The “what happens when we’re done watching” logic clearly visible and reusable.
- The interval bodies simpler and easier to reason about and test in isolation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+179
to
+185
| _subscriptionLoginStatus(id) { | ||
| try { | ||
| const status = JSON.parse(fs.readFileSync(path.join(this.subDir(id), '.vibespace-login-status.json'), 'utf-8')); | ||
| if (status?.state !== 'error' || !/^[a-z0-9-]{1,40}$/.test(status.code || '')) return null; | ||
| return { state: 'error', code: status.code }; | ||
| } catch { return null; } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Login-status code validation is stricter than the writer and may silently ignore future error codes.
_subscriptionLoginStatus only returns errors whose code matches /^[a-z0-9-]{1,40}$/, but writeLoginStatus can persist any trimmed string and current callers already use values like claude-login-exit. If future codes add characters like _ or uppercase, they’ll be written but then silently ignored, so finalizeSubscription will never see loginFailed for those cases.
To avoid this mismatch, either broaden the regex to match what writeLoginStatus can emit (e.g. [A-Za-z0-9._-]{1,40}) or drop the format check and just require status.state === 'error' and a non-empty code.
Suggested change
| _subscriptionLoginStatus(id) { | |
| try { | |
| const status = JSON.parse(fs.readFileSync(path.join(this.subDir(id), '.vibespace-login-status.json'), 'utf-8')); | |
| if (status?.state !== 'error' || !/^[a-z0-9-]{1,40}$/.test(status.code || '')) return null; | |
| return { state: 'error', code: status.code }; | |
| } catch { return null; } | |
| } | |
| _subscriptionLoginStatus(id) { | |
| try { | |
| const status = JSON.parse( | |
| fs.readFileSync( | |
| path.join(this.subDir(id), '.vibespace-login-status.json'), | |
| 'utf-8', | |
| ), | |
| ); | |
| if ( | |
| status?.state !== 'error' || | |
| !/^[A-Za-z0-9._-]{1,40}$/.test(status.code || '') | |
| ) { | |
| return null; | |
| } | |
| return { state: 'error', code: status.code }; | |
| } catch { | |
| return null; | |
| } | |
| } |
| // (§ban-safety) — until the credential files CHANGE vs the pre-login | ||
| // snapshot, then brings the Agents surface back on the SAME machine. | ||
| _watchHostLogin(hostId, hostLabel) { | ||
| _watchHostLogin(hostId, hostLabel, accountId = null, loginAttempt = null) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the per-attempt and global polling paths plus the shared completion behavior in _watchHostLogin into separate helper methods to make the logic clearer and flatter.
You can simplify _watchHostLogin by making the dual-mode logic and the complete side effects explicit helpers. That keeps the polling loops single-purpose and flattens the nested conditions without changing behavior.
Example refactor:
_watchHostLogin(hostId, hostLabel, accountId = null, loginAttempt = null) {
if (!hostId) return;
if (this._hostLoginWatch) {
clearInterval(this._hostLoginWatch);
this._hostLoginWatch = null;
}
if (accountId && loginAttempt) {
this._watchHostLoginForAttempt({ hostId, hostLabel, accountId, loginAttempt });
} else {
this._watchHostLoginGlobal({ hostId, hostLabel });
}
}Then split out the two polling modes and the shared completion:
_completeHostLoginWatch({ hostId, hostLabel, machineLoginChanged }) {
clearInterval(this._hostLoginWatch);
this._hostLoginWatch = null;
if (machineLoginChanged) (this._hostLoginSeenAt ||= {})[hostId] = Date.now();
if (this._agentsHostPref && this._agentsHostPref !== hostId) {
showToast(t('✓ Login on {host} updated', { host: hostLabel }), { duration: 5000 });
return;
}
showToast(t('✓ Login on {host} updated — reopening Agents there', { host: hostLabel }), { duration: 5000 });
this._agentsHostPref = hostId;
if (!this._agentsRefreshHook?.(hostId)) this._showAgentsDialog();
}
_watchHostLoginForAttempt({ hostId, hostLabel, accountId, loginAttempt }) {
let tries = 0;
this._hostLoginWatch = setInterval(async () => {
if (++tries > 50) { clearInterval(this._hostLoginWatch); this._hostLoginWatch = null; return; }
let cur;
try { cur = await fetchJson(`/api/hosts/${encodeURIComponent(hostId)}/accounts-status`); } catch { return; }
const loginStatus = cur?.hostSubLoginStatus?.[accountId];
if (!loginStatus || loginStatus.attempt !== loginAttempt || loginStatus.state === 'running') return;
if (loginStatus.state === 'error') {
clearInterval(this._hostLoginWatch); this._hostLoginWatch = null;
showToast(t('Subscription login could not be saved. Check the login terminal for details, then try again.'), { type: 'error', duration: 8000 });
return;
}
if (loginStatus.state === 'success') {
this._completeHostLoginWatch({ hostId, hostLabel, machineLoginChanged: false });
}
}, 6000);
}
_watchHostLoginGlobal({ hostId, hostLabel }) {
const sig = (r) => (r && !r.error)
? [r.credsMtime || 0, r.codexAuthMtime || 0, r.subscription?.loggedIn ? 1 : 0, r.subscription?.email || '', r.codex?.email || '', (r.hostSubs || []).join('+')].join('|')
: null;
let baseSig = null;
let tries = 0;
this._hostLoginWatch = setInterval(async () => {
if (++tries > 50) { clearInterval(this._hostLoginWatch); this._hostLoginWatch = null; return; }
let cur;
try { cur = await fetchJson(`/api/hosts/${encodeURIComponent(hostId)}/accounts-status`); } catch { return; }
const s = sig(cur);
if (s === null) return;
if (baseSig === null) { baseSig = s; return; }
if (s === baseSig) return;
const machinePart = (x) => x.split('|').slice(0, 5).join('|');
const machineLoginChanged = machinePart(s) !== machinePart(baseSig);
this._completeHostLoginWatch({ hostId, hostLabel, machineLoginChanged });
}, 6000);
}This keeps all current behaviors (including the per-attempt short-circuiting and machine-login-change stamp) but makes:
- The two modes (
attemptvs global fingerprint) explicit and mutually exclusive. - The “what happens when we’re done watching” logic clearly visible and reusable.
- The interval bodies simpler and easier to reason about and test in isolation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
claudeAiOauthto the isolated fallback file.Why
On macOS,
claude auth login --claudeaican report success for an isolatedCLAUDE_SECURESTORAGE_CONFIG_DIRwhile storing the credential only in Keychain. A VibeSpace server started by launchd often cannot read that item later, so the named subscription stays "not logged in" even though the interactive login completed.Root cause
Claude Code 2.1.220 derives a Keychain service from the NFC-normalized secure-storage directory:
Claude Code-credentials-${sha256(dir).slice(0, 8)}The interactive login terminal can access the newly written item, while the launchd/daemon context may receive a Keychain authorization error. VibeSpace previously waited only for
<dir>/.credentials.json, so it never observed the successful Keychain-only login.Related Claude Code reports:
/doctormisdiagnoses keychain authorization as locked/corrupt in daemon context anthropics/claude-code#69631Impact and safety
claudeAiOauthroot using a same-directory0600temp file,fsync, and atomic rename; account directories are forced to0700.Checks
node scripts/test-claude-subscription-login.mjsnpm run buildnode scripts/secret-scan.mjs <changed files>git diff --checkSummary by Sourcery
Handle Claude subscription logins via a dedicated helper that captures macOS Keychain-backed credentials into per-subscription dirs and treats those logins as local-only, with precise login status reporting and safer remote handling.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: