Fix #17: detect locked keychain and preserve credentials on access failure #3

+62 -158

13 changed files

Interdiff #0 → #1

expand all

.claude

settings.json

CLAUDE.md

src

commands

issue.ts

lib

api-client.ts

issues-api.ts

session.ts

utils

auth-helpers.ts

formatting.ts

tests

commands

issue.test.ts

lib

api-client.test.ts

issues-api.test.ts

session.test.ts

utils

auth-helpers.test.ts

src/commands/issue.ts

This patch was likely rebased, as context lines do not match.

+3 -2

src/lib/api-client.ts

··· 111 111 // Don't clear credentials — keychain may just be temporarily locked 112 112 throw error; 113 113 } 114 - // Session data invalid or agent resume failed — clear stale state 115 - await clearCurrentSessionMetadata(); 114 + // Session resume failed (network error, expired refresh token, etc.) 115 + // Don't clear credentials — the error may be transient. The user can 116 + // run "auth login" explicitly if they need to re-authenticate. 116 117 return false; 117 118 } 118 119 }

+21 -15

src/lib/session.ts

··· 1 + import { mkdir, readFile, unlink, writeFile } from 'node:fs/promises'; 2 + import { homedir } from 'node:os'; 3 + import { join } from 'node:path'; 1 4 import type { AtpSessionData } from '@atproto/api'; 2 5 import { AsyncEntry } from '@napi-rs/keyring'; 3 6 4 7 const SERVICE_NAME = 'tangled-cli'; 8 + const SESSION_METADATA_PATH = join(homedir(), '.config', 'tangled', 'session.json'); 5 9 6 10 export class KeychainAccessError extends Error { 7 11 constructor(message: string) { ··· 73 77 } 74 78 75 79 /** 76 - * Store metadata about current session for CLI to track active user 77 - * Uses a special "current" account in keychain 80 + * Store metadata about current session for CLI to track active user. 81 + * Written to a plain file — metadata is not secret and must be readable 82 + * even when the keychain is locked (e.g. after sleep/wake). 78 83 */ 79 84 export async function saveCurrentSessionMetadata(metadata: SessionMetadata): Promise<void> { 80 - const serialized = JSON.stringify(metadata); 81 - const entry = new AsyncEntry(SERVICE_NAME, 'current-session-metadata'); 82 - await entry.setPassword(serialized); 85 + await mkdir(join(homedir(), '.config', 'tangled'), { recursive: true }); 86 + await writeFile(SESSION_METADATA_PATH, JSON.stringify(metadata, null, 2), 'utf-8'); 83 87 } 84 88 85 89 /** ··· 87 91 */ 88 92 export async function getCurrentSessionMetadata(): Promise<SessionMetadata | null> { 89 93 try { 90 - const entry = new AsyncEntry(SERVICE_NAME, 'current-session-metadata'); 91 - const serialized = await entry.getPassword(); 92 - if (!serialized) { 94 + const content = await readFile(SESSION_METADATA_PATH, 'utf-8'); 95 + return JSON.parse(content) as SessionMetadata; 96 + } catch (error) { 97 + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { 93 98 return null; 94 99 } 95 - return JSON.parse(serialized) as SessionMetadata; 96 - } catch (error) { 97 - throw new KeychainAccessError( 98 - `Cannot access keychain: ${error instanceof Error ? error.message : 'Unknown error'}` 99 - ); 100 + throw error; 100 101 } 101 102 } 102 103 ··· 104 105 * Clear current session metadata 105 106 */ 106 107 export async function clearCurrentSessionMetadata(): Promise<void> { 107 - const entry = new AsyncEntry(SERVICE_NAME, 'current-session-metadata'); 108 - await entry.deleteCredential(); 108 + try { 109 + await unlink(SESSION_METADATA_PATH); 110 + } catch (error) { 111 + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { 112 + throw error; 113 + } 114 + } 109 115 }

src/utils/auth-helpers.ts

This file has not been changed.

tests/commands/issue.test.ts

This patch was likely rebased, as context lines do not match.

+2 -2

tests/lib/api-client.test.ts

··· 143 143 expect(vi.mocked(sessionModule.clearCurrentSessionMetadata)).toHaveBeenCalled(); 144 144 }); 145 145 146 - it('should return false and cleanup on resume error', async () => { 146 + it('should return false without clearing metadata on transient resume error', async () => { 147 147 vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(mockSessionMetadata); 148 148 vi.mocked(sessionModule.loadSession).mockResolvedValue(mockSessionData); 149 149 ··· 153 153 const resumed = await client.resumeSession(); 154 154 155 155 expect(resumed).toBe(false); 156 - expect(vi.mocked(sessionModule.clearCurrentSessionMetadata)).toHaveBeenCalled(); 156 + expect(vi.mocked(sessionModule.clearCurrentSessionMetadata)).not.toHaveBeenCalled(); 157 157 }); 158 158 159 159 it('should rethrow KeychainAccessError without clearing metadata', async () => {

tests/lib/issues-api.test.ts

This patch was likely rebased, as context lines do not match.

tests/utils/auth-helpers.test.ts

This file has not been changed.

+1 -10

.claude/settings.json

··· 1 1 { 2 2 "permissions": { 3 - "allow": [ 4 - "Bash(npm run test:*)", 5 - "Bash(npm run build:*)", 6 - "Bash(npm test:*)", 7 - "Bash(npm run typecheck:*)", 8 - "Bash(npm run lint:*)", 9 - "Bash(npm run lint:fix:*)", 10 - "Bash(git checkout:*)", 11 - "Bash(npm run format:*)" 12 - ] 3 + "allow": ["Bash(npm run test:*)", "Bash(npm run build:*)", "Bash(npm test:*)"] 13 4 } 14 5 }

-59

CLAUDE.md

··· 1 - # CLAUDE.md 2 - 3 - This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. 4 - 5 - ## Commands 6 - 7 - ```bash 8 - npm run dev -- <args> # Run CLI in development (use this, not ./tangled) 9 - npm run build # Compile TypeScript to dist/ 10 - npm test # Run all tests once 11 - npm run test:watch # Run tests in watch mode 12 - npm run typecheck # Type-check without building (prefer over npx tsc --noEmit) 13 - npm run lint # Check with Biome 14 - npm run lint:fix # Auto-fix lint/format issues 15 - ``` 16 - 17 - Run a single test file: 18 - ```bash 19 - npx vitest run tests/commands/issue.test.ts 20 - ``` 21 - 22 - ## Architecture 23 - 24 - `src/index.ts` is the entry point — it registers all commands and parses `process.argv`. 25 - 26 - ### Layer structure 27 - 28 - - **`src/commands/`** — Commander.js command factories (e.g. `createIssueCommand()`). Each command: resumes session, gets repo context, calls lib functions, outputs results. 29 - - **`src/lib/`** — Business logic with no Commander dependency: 30 - - `api-client.ts` — `TangledApiClient` wraps `AtpAgent`; `isAuthenticated()` is **synchronous** 31 - - `session.ts` — OS keychain storage via `@napi-rs/keyring`; throws `KeychainAccessError` if keychain is inaccessible (not just missing) 32 - - `context.ts` — Infers repo from `git remote` URLs; resolves `RepositoryContext` with owner DID/handle and repo name 33 - - `issues-api.ts` — All issue CRUD; exports `IssueData` (canonical JSON shape), `getCompleteIssueData`, `resolveSequentialNumber` 34 - - **`src/utils/`** — Stateless helpers: 35 - - `auth-helpers.ts` — `requireAuth(client)` throws if unauthenticated (use in lib functions); `ensureAuthenticated(client)` for commands (calls `resumeSession`, exits on failure) 36 - - `validation.ts` — **All** validation logic lives here (Zod schemas + boolean helpers) 37 - - `formatting.ts` — `outputJson<T extends object>(data, fields?)`, `formatDate`, `formatIssueState` 38 - - `at-uri.ts` — Parse/build AT-URIs and repo AT-URIs 39 - - `body-input.ts` — Reads `--body` / `--body-file` / stdin (`-F -`) 40 - - **`src/lexicon/`** — Auto-generated AT Protocol type definitions; regenerate with `npm run codegen` 41 - 42 - ### Key patterns 43 - 44 - **Issue numbering** — Sequential numbers are not stored; they are computed by sorting all issues for a repo by `createdAt` ascending. The 1-based index is the display number. 45 - 46 - **Issue state** — Stored as separate `sh.tangled.repo.issue.state` records. The latest record wins; default is `'open'` if no record exists. 47 - 48 - **JSON output** — All issue sub-commands use `IssueCommand extends Command` (in `issue.ts`) to share a `--json [fields]` option. The canonical field set is: `number, title, body, state, author, createdAt, uri, cid`. Use `getCompleteIssueData()` to populate all fields. 49 - 50 - **Auth flow** — Commands call `client.resumeSession()` directly, then proceed. Lib functions call `requireAuth(client)`. `KeychainAccessError` from `session.ts` propagates through `resumeSession()` without clearing metadata. 51 - 52 - ### Tests 53 - 54 - Tests mirror `src/` under `tests/`. Command tests mock the entire `issuesApi` module: 55 - ```typescript 56 - vi.mock('../../src/lib/issues-api.js'); 57 - // Use importOriginal to preserve exported classes/errors if needed 58 - ``` 59 - `isAuthenticated()` is synchronous — mock as `vi.fn(() => true)`, not `vi.fn(async () => true)`.

-66

src/lib/issues-api.ts

··· 424 424 } 425 425 426 426 /** 427 - * Resolve a sequential issue number from a displayId or by scanning the issue list. 428 - * Fast path: if displayId is "#N", return N directly. 429 - * Fallback: fetch all issues, sort oldest-first, return 1-based position. 430 - */ 431 - export async function resolveSequentialNumber( 432 - displayId: string, 433 - issueUri: string, 434 - client: TangledApiClient, 435 - repoAtUri: string 436 - ): Promise<number | undefined> { 437 - const match = displayId.match(/^#(\d+)$/); 438 - if (match) return Number.parseInt(match[1], 10); 439 - 440 - const { issues } = await listIssues({ client, repoAtUri, limit: 100 }); 441 - const sorted = issues.sort( 442 - (a, b) => new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime() 443 - ); 444 - const idx = sorted.findIndex((i) => i.uri === issueUri); 445 - return idx >= 0 ? idx + 1 : undefined; 446 - } 447 - 448 - /** 449 - * Canonical JSON shape for a single issue, used by all issue commands. 450 - */ 451 - export interface IssueData { 452 - number: number | undefined; 453 - title: string; 454 - body?: string; 455 - state: 'open' | 'closed'; 456 - author: string; 457 - createdAt: string; 458 - uri: string; 459 - cid: string; 460 - } 461 - 462 - /** 463 - * Fetch a complete IssueData object ready for JSON output. 464 - * Fetches the issue record and sequential number in parallel. 465 - * If stateOverride is supplied (e.g. 'closed' after a close operation), 466 - * getIssueState is skipped; otherwise the current state is fetched. 467 - */ 468 - export async function getCompleteIssueData( 469 - client: TangledApiClient, 470 - issueUri: string, 471 - displayId: string, 472 - repoAtUri: string, 473 - stateOverride?: 'open' | 'closed' 474 - ): Promise<IssueData> { 475 - const [issue, number] = await Promise.all([ 476 - getIssue({ client, issueUri }), 477 - resolveSequentialNumber(displayId, issueUri, client, repoAtUri), 478 - ]); 479 - const state = stateOverride ?? (await getIssueState({ client, issueUri })); 480 - return { 481 - number, 482 - title: issue.title, 483 - body: issue.body, 484 - state, 485 - author: issue.author, 486 - createdAt: issue.createdAt, 487 - uri: issue.uri, 488 - cid: issue.cid, 489 - }; 490 - } 491 - 492 - /** 493 427 * Reopen a closed issue by creating an open state record 494 428 */ 495 429 export async function reopenIssue(params: ReopenIssueParams): Promise<void> {

+4 -4

src/utils/formatting.ts

··· 45 45 * @param data - The data to output (object or array of objects) 46 46 * @param fields - Comma-separated field names to include; omit for all fields 47 47 */ 48 - export function outputJson<T extends object>( 49 - data: T | T[], 48 + export function outputJson( 49 + data: Record<string, unknown> | Record<string, unknown>[], 50 50 fields?: string 51 51 ): void { 52 52 if (fields) { ··· 57 57 if (Array.isArray(data)) { 58 58 console.log( 59 59 JSON.stringify( 60 - data.map((item) => pickFields(item as Record<string, unknown>, fieldList)), 60 + data.map((item) => pickFields(item, fieldList)), 61 61 null, 62 62 2 63 63 ) 64 64 ); 65 65 } else { 66 - console.log(JSON.stringify(pickFields(data as Record<string, unknown>, fieldList), null, 2)); 66 + console.log(JSON.stringify(pickFields(data, fieldList), null, 2)); 67 67 } 68 68 } else { 69 69 console.log(JSON.stringify(data, null, 2));

+31

tests/lib/session.test.ts

··· 33 33 }; 34 34 }); 35 35 36 + // Mock node:fs/promises for metadata file storage 37 + const mockFileStorage = new Map<string, string>(); 38 + 39 + vi.mock('node:fs/promises', () => ({ 40 + mkdir: vi.fn().mockResolvedValue(undefined), 41 + writeFile: vi.fn().mockImplementation(async (path: string, content: string) => { 42 + mockFileStorage.set(path as string, content); 43 + }), 44 + readFile: vi.fn().mockImplementation(async (path: string) => { 45 + const content = mockFileStorage.get(path as string); 46 + if (content === undefined) { 47 + const err = Object.assign(new Error(`ENOENT: no such file or directory, open '${path}'`), { 48 + code: 'ENOENT', 49 + }); 50 + throw err; 51 + } 52 + return content; 53 + }), 54 + unlink: vi.fn().mockImplementation(async (path: string) => { 55 + if (!mockFileStorage.has(path as string)) { 56 + const err = Object.assign(new Error(`ENOENT: no such file or directory, unlink '${path}'`), { 57 + code: 'ENOENT', 58 + }); 59 + throw err; 60 + } 61 + mockFileStorage.delete(path as string); 62 + }), 63 + })); 64 + 36 65 describe('Session Management', () => { 37 66 beforeEach(() => { 38 67 // Clear mock storage before each test 39 68 mockKeyringStorage.clear(); 69 + mockFileStorage.clear(); 40 70 vi.clearAllMocks(); 41 71 }); 42 72 43 73 afterEach(() => { 44 74 // Clean up after each test 45 75 mockKeyringStorage.clear(); 76 + mockFileStorage.clear(); 46 77 }); 47 78 48 79 describe('saveSession', () => {

History

4 rounds 1 comment

markbennett.ca submitted #3 5w

diff interdiff

5 commits

expand

3f52b8eb

Fix #17: detect locked keychain and preserve credentials on access failure

e88c6edf

Move session metadata from keychain to plain file to prevent credential loss

29bfafa9

chore: allow git fetch in Claude permissions

ba9505a3

Formatting

9741e20a

Update pipeline cloning

expand 1 comment

markbennett.ca 5w

Alright, I haven't seen the error for a while now. Will try this and create a new issue if it comes up again!

pull request successfully merged

markbennett.ca submitted #2 5w

diff interdiff

4 commits

expand

fda30bbc

Fix #17: detect locked keychain and preserve credentials on access failure

96add696

Move session metadata from keychain to plain file to prevent credential loss

269745a4

chore: allow git fetch in Claude permissions

afadda07

Formatting

expand 0 comments

markbennett.ca submitted #1 5w

diff

2 commits

expand

fda30bbc

Fix #17: detect locked keychain and preserve credentials on access failure

96add696

Move session metadata from keychain to plain file to prevent credential loss

expand 0 comments

markbennett.ca submitted #0 5w

diff

1 commit

expand

fda30bbc

Fix #17: detect locked keychain and preserve credentials on access failure

expand 0 comments