feat: enable direct context selection and improve session reliability (#1402)

This commit is contained in:
Ralph Khreish
2025-11-14 20:52:52 +01:00
committed by GitHub
parent 10ec025581
commit 6b44a96d9f
5 changed files with 541 additions and 131 deletions

View File

@@ -76,8 +76,9 @@ export class ContextCommand extends Command {
private addOrgCommand(): void {
this.command('org')
.description('Select an organization')
.action(async () => {
await this.executeSelectOrg();
.argument('[orgId]', 'Organization ID or slug to select directly')
.action(async (orgId?: string) => {
await this.executeSelectOrg(orgId);
});
}
@@ -87,8 +88,9 @@ export class ContextCommand extends Command {
private addBriefCommand(): void {
this.command('brief')
.description('Select a brief within the current organization')
.action(async () => {
await this.executeSelectBrief();
.argument('[briefIdOrUrl]', 'Brief ID or Hamster URL to select directly')
.action(async (briefIdOrUrl?: string) => {
await this.executeSelectBrief(briefIdOrUrl);
});
}
@@ -230,14 +232,14 @@ export class ContextCommand extends Command {
/**
* Execute org selection
*/
private async executeSelectOrg(): Promise<void> {
private async executeSelectOrg(orgId?: string): Promise<void> {
try {
// Check authentication
if (!(await checkAuthentication(this.authManager))) {
process.exit(1);
}
const result = await this.selectOrganization();
const result = await this.selectOrganization(orgId);
this.setLastResult(result);
if (!result.success) {
@@ -252,9 +254,9 @@ export class ContextCommand extends Command {
}
/**
* Select an organization interactively
* Select an organization interactively or by ID/slug/name
*/
private async selectOrganization(): Promise<ContextResult> {
private async selectOrganization(orgId?: string): Promise<ContextResult> {
const spinner = ora('Fetching organizations...').start();
try {
@@ -271,18 +273,57 @@ export class ContextCommand extends Command {
};
}
// Prompt for selection
const { selectedOrg } = await inquirer.prompt([
{
type: 'list',
name: 'selectedOrg',
message: 'Select an organization:',
choices: organizations.map((org) => ({
name: org.name,
value: org
}))
let selectedOrg;
// If orgId provided, find matching org by ID, slug or name
const trimmedOrgId = orgId?.trim();
if (trimmedOrgId) {
const normalizedInput = trimmedOrgId.toLowerCase();
selectedOrg = organizations.find(
(org) =>
org.id === trimmedOrgId ||
org.slug?.toLowerCase() === normalizedInput ||
org.name.toLowerCase() === normalizedInput
);
if (!selectedOrg) {
const totalCount = organizations.length;
const displayLimit = 5;
const orgList = organizations
.slice(0, displayLimit)
.map((o) => o.name)
.join(', ');
let errorMessage = `Organization not found: ${trimmedOrgId}\n`;
if (totalCount <= displayLimit) {
errorMessage += `Available organizations: ${orgList}`;
} else {
errorMessage += `Available organizations (showing ${displayLimit} of ${totalCount}): ${orgList}`;
errorMessage += `\nRun "tm context org" to see all organizations and select interactively`;
}
ui.displayError(errorMessage);
return {
success: false,
action: 'select-org',
message: `Organization not found: ${trimmedOrgId}`
};
}
]);
} else {
// Interactive selection
const response = await inquirer.prompt([
{
type: 'list',
name: 'selectedOrg',
message: 'Select an organization:',
choices: organizations.map((org) => ({
name: org.name,
value: org
}))
}
]);
selectedOrg = response.selectedOrg;
}
// Update context
await this.authManager.updateContext({
@@ -311,14 +352,20 @@ export class ContextCommand extends Command {
/**
* Execute brief selection
*/
private async executeSelectBrief(): Promise<void> {
private async executeSelectBrief(briefIdOrUrl?: string): Promise<void> {
try {
// Check authentication
if (!(await checkAuthentication(this.authManager))) {
process.exit(1);
}
// Check if org is selected
// If briefIdOrUrl provided, use direct selection
if (briefIdOrUrl && briefIdOrUrl.trim().length > 0) {
await this.selectBriefDirectly(briefIdOrUrl.trim(), 'select-brief');
return;
}
// Interactive selection
const context = this.authManager.getContext();
if (!context?.orgId) {
ui.displayError(
@@ -428,6 +475,34 @@ export class ContextCommand extends Command {
}
}
/**
* Helper method to select brief directly from input (URL or ID)
* Used by both executeSelectBrief and executeSetFromBriefInput
*/
private async selectBriefDirectly(
input: string,
action: 'select-brief' | 'set'
): Promise<void> {
await this.initTmCore();
const result = await selectBriefFromInput(
this.authManager,
input,
this.tmCore
);
this.setLastResult({
success: result.success,
action,
context: this.authManager.getContext() || undefined,
message: result.message
});
if (!result.success) {
process.exit(1);
}
}
/**
* Execute setting context from a brief ID or Hamster URL
* All parsing logic is in tm-core
@@ -439,26 +514,7 @@ export class ContextCommand extends Command {
process.exit(1);
}
// Initialize tmCore to access business logic
await this.initTmCore();
// Use shared utility - tm-core handles ALL parsing
const result = await selectBriefFromInput(
this.authManager,
input,
this.tmCore
);
this.setLastResult({
success: result.success,
action: 'set',
context: this.authManager.getContext() || undefined,
message: result.message
});
if (!result.success) {
process.exit(1);
}
await this.selectBriefDirectly(input, 'set');
} catch (error: any) {
ui.displayError(
`Failed to set context from brief: ${(error as Error).message}`

View File

@@ -0,0 +1,214 @@
/**
* Tests for SupabaseSessionStorage
* Verifies session persistence with steno atomic writes
*/
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
import fs from 'fs/promises';
import fsSync from 'fs';
import os from 'os';
import path from 'path';
import { SupabaseSessionStorage } from './supabase-session-storage.js';
describe('SupabaseSessionStorage', () => {
let tempDir: string;
let sessionPath: string;
beforeEach(() => {
// Create unique temp directory for each test
tempDir = fsSync.mkdtempSync(path.join(os.tmpdir(), 'tm-session-test-'));
sessionPath = path.join(tempDir, 'session.json');
});
afterEach(() => {
// Clean up temp directory
if (fsSync.existsSync(tempDir)) {
fsSync.rmSync(tempDir, { recursive: true, force: true });
}
});
describe('persistence with steno', () => {
it('should immediately persist data to disk with setItem', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
// Set a session token
const testSession = JSON.stringify({
access_token: 'test-token',
refresh_token: 'test-refresh'
});
await storage.setItem('sb-localhost-auth-token', testSession);
// Immediately verify file exists and is readable (without any delay)
expect(fsSync.existsSync(sessionPath)).toBe(true);
// Read directly from disk (simulating a new process)
const diskData = JSON.parse(fsSync.readFileSync(sessionPath, 'utf8'));
expect(diskData['sb-localhost-auth-token']).toBe(testSession);
});
it('should guarantee data is on disk before returning', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
// Write large session data
const largeSession = JSON.stringify({
access_token: 'x'.repeat(10000),
refresh_token: 'y'.repeat(10000),
user: { id: 'test', email: 'test@example.com' }
});
await storage.setItem('sb-localhost-auth-token', largeSession);
// Create a NEW storage instance (simulates separate CLI command)
const newStorage = new SupabaseSessionStorage(sessionPath);
// Should immediately read the persisted data
const retrieved = await newStorage.getItem('sb-localhost-auth-token');
expect(retrieved).toBe(largeSession);
});
it('should handle rapid sequential writes without data loss', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
// Simulate rapid token updates (like during refresh)
for (let i = 0; i < 5; i++) {
const session = JSON.stringify({
access_token: `token-${i}`,
expires_at: Date.now() + i * 1000
});
await storage.setItem('sb-localhost-auth-token', session);
// Each write should be immediately readable
const newStorage = new SupabaseSessionStorage(sessionPath);
const retrieved = await newStorage.getItem('sb-localhost-auth-token');
expect(JSON.parse(retrieved!).access_token).toBe(`token-${i}`);
}
});
});
describe('atomic writes', () => {
it('should complete writes atomically without leaving temp files', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
await storage.setItem('test-key', 'test-value');
// Final file should exist with correct content
expect(fsSync.existsSync(sessionPath)).toBe(true);
const diskData = JSON.parse(fsSync.readFileSync(sessionPath, 'utf8'));
expect(diskData['test-key']).toBe('test-value');
// No unexpected extra files should remain in directory
const files = fsSync.readdirSync(path.dirname(sessionPath));
expect(files).toEqual([path.basename(sessionPath)]);
});
it('should maintain correct file permissions (0700 for directory)', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
await storage.setItem('test-key', 'test-value');
// Check that file exists
expect(fsSync.existsSync(sessionPath)).toBe(true);
// Check directory has correct permissions
const dir = path.dirname(sessionPath);
const stats = fsSync.statSync(dir);
const mode = stats.mode & 0o777;
// Directory should be readable/writable/executable by owner only (0700)
expect(mode).toBe(0o700);
});
});
describe('basic operations', () => {
it('should get and set items', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
await storage.setItem('key1', 'value1');
expect(await storage.getItem('key1')).toBe('value1');
});
it('should return null for non-existent items', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
expect(await storage.getItem('non-existent')).toBe(null);
});
it('should remove items', async () => {
const storage = new SupabaseSessionStorage(sessionPath);
await storage.setItem('key1', 'value1');
expect(await storage.getItem('key1')).toBe('value1');
await storage.removeItem('key1');
expect(await storage.getItem('key1')).toBe(null);
// Verify removed from disk
const newStorage = new SupabaseSessionStorage(sessionPath);
expect(await newStorage.getItem('key1')).toBe(null);
});
});
describe('initialization', () => {
it('should load existing session on initialization', async () => {
// Create initial storage and save data
const storage1 = new SupabaseSessionStorage(sessionPath);
await storage1.setItem('key1', 'value1');
// Create new instance (simulates new process)
const storage2 = new SupabaseSessionStorage(sessionPath);
expect(await storage2.getItem('key1')).toBe('value1');
});
it('should handle non-existent session file gracefully', async () => {
// Don't create any file, just initialize
const storage = new SupabaseSessionStorage(sessionPath);
// Should not throw and should work normally
expect(await storage.getItem('any-key')).toBe(null);
await storage.setItem('key1', 'value1');
expect(await storage.getItem('key1')).toBe('value1');
});
it('should create directory if it does not exist', async () => {
const deepPath = path.join(
tempDir,
'deep',
'nested',
'path',
'session.json'
);
const storage = new SupabaseSessionStorage(deepPath);
await storage.setItem('key1', 'value1');
expect(fsSync.existsSync(deepPath)).toBe(true);
expect(fsSync.existsSync(path.dirname(deepPath))).toBe(true);
});
});
describe('error handling', () => {
it('should not throw on persist errors', async () => {
const invalidPath = '/invalid/path/that/cannot/be/written/session.json';
const storage = new SupabaseSessionStorage(invalidPath);
// Should not throw, session should remain in memory
await storage.setItem('key1', 'value1');
// Should still work in memory
expect(await storage.getItem('key1')).toBe('value1');
});
it('should handle corrupted session file gracefully', async () => {
// Write corrupted JSON
fsSync.writeFileSync(sessionPath, 'invalid json {{{');
// Should not throw on initialization
expect(() => new SupabaseSessionStorage(sessionPath)).not.toThrow();
// Should work normally after initialization
const storage = new SupabaseSessionStorage(sessionPath);
await storage.setItem('key1', 'value1');
expect(await storage.getItem('key1')).toBe('value1');
});
});
});

View File

@@ -11,11 +11,13 @@
*
* We handle:
* - Simple get/set/remove/clear operations
* - Persistence to ~/.taskmaster/session.json
* - Persistence to ~/.taskmaster/session.json with atomic writes via steno
*/
import fs from 'fs';
import fs from 'fs/promises';
import fsSync from 'fs';
import path from 'path';
import { Writer } from 'steno';
import type { SupportedStorage } from '@supabase/supabase-js';
import { getLogger } from '../../../common/logger/index.js';
@@ -29,19 +31,27 @@ export class SupabaseSessionStorage implements SupportedStorage {
private storage: Map<string, string> = new Map();
private persistPath: string;
private logger = getLogger('SupabaseSessionStorage');
private writer: Writer;
private initPromise: Promise<void>;
constructor(persistPath: string = DEFAULT_SESSION_FILE) {
this.persistPath = persistPath;
this.load();
this.writer = new Writer(persistPath);
this.initPromise = this.load();
}
/**
* Load session data from disk on initialization
*/
private load(): void {
private async load(): Promise<void> {
try {
if (fs.existsSync(this.persistPath)) {
const data = JSON.parse(fs.readFileSync(this.persistPath, 'utf8'));
// Ensure directory exists
const dir = path.dirname(this.persistPath);
await fs.mkdir(dir, { recursive: true, mode: 0o700 });
// Try to read existing session
if (fsSync.existsSync(this.persistPath)) {
const data = JSON.parse(await fs.readFile(this.persistPath, 'utf8'));
Object.entries(data).forEach(([k, v]) =>
this.storage.set(k, v as string)
);
@@ -57,24 +67,19 @@ export class SupabaseSessionStorage implements SupportedStorage {
/**
* Persist session data to disk immediately
* Uses steno for atomic writes with fsync guarantees
* This prevents race conditions in rapid CLI command sequences
*/
private persist(): void {
private async persist(): Promise<void> {
try {
// Ensure directory exists
const dir = path.dirname(this.persistPath);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true, mode: 0o700 });
}
// Write atomically with temp file
const data = Object.fromEntries(this.storage);
const tempFile = `${this.persistPath}.tmp`;
fs.writeFileSync(tempFile, JSON.stringify(data, null, 2), {
mode: 0o600
});
fs.renameSync(tempFile, this.persistPath);
const jsonContent = JSON.stringify(data, null, 2);
this.logger.debug('Persisted session to disk');
// steno handles atomic writes with temp file + rename
// and ensures data is flushed to disk
await this.writer.write(jsonContent + '\n');
this.logger.debug('Persisted session to disk (steno)');
} catch (error) {
this.logger.error('Failed to persist session:', error);
// Don't throw - session is still in memory
@@ -84,8 +89,12 @@ export class SupabaseSessionStorage implements SupportedStorage {
/**
* Get item from storage
* Supabase will call this to retrieve session data
* Returns a promise to ensure initialization completes first
*/
getItem(key: string): string | null {
async getItem(key: string): Promise<string | null> {
// Wait for initialization to complete
await this.initPromise;
const value = this.storage.get(key) ?? null;
this.logger.debug('getItem called', { key, hasValue: !!value });
return value;
@@ -96,28 +105,41 @@ export class SupabaseSessionStorage implements SupportedStorage {
* Supabase will call this to store/update session data
* CRITICAL: This is called during token refresh - must persist immediately
*/
setItem(key: string, value: string): void {
async setItem(key: string, value: string): Promise<void> {
// Wait for initialization to complete
await this.initPromise;
this.logger.debug('setItem called', { key });
this.storage.set(key, value);
this.persist(); // Immediately persist on every write
// Immediately persist on every write
// steno ensures atomic writes with fsync
await this.persist();
}
/**
* Remove item from storage
* Supabase will call this during sign out
*/
removeItem(key: string): void {
async removeItem(key: string): Promise<void> {
// Wait for initialization to complete
await this.initPromise;
this.logger.debug('removeItem called', { key });
this.storage.delete(key);
this.persist();
await this.persist();
}
/**
* Clear all session data
* Useful for complete logout scenarios
*/
clear(): void {
async clear(): Promise<void> {
// Wait for initialization to complete
await this.initPromise;
this.logger.debug('clear called');
this.storage.clear();
this.persist();
await this.persist();
}
}

View File

@@ -10,6 +10,7 @@ import {
import { AuthManager } from '../auth/managers/auth-manager.js';
import type { TaskRepository } from '../tasks/repositories/task-repository.interface.js';
import { BriefService, type TagWithStats } from './services/brief-service.js';
import { BriefUrlParser } from './utils/url-parser.js';
/**
* Briefs Domain - Unified API for brief operations
@@ -35,15 +36,38 @@ export class BriefsDomain {
* - Brief name (exact or partial match)
*
* @param input - Raw input: URL, UUID, last 8 chars, or brief name
* @param orgId - Optional organization ID. If not provided, uses current context.
* @param orgId - Optional organization ID. If not provided, tries to extract from URL or uses current context.
* @returns The resolved brief object
*/
async resolveBrief(input: string, orgId?: string): Promise<any> {
// Extract brief ID/name from input (could be URL, ID, or name)
const briefIdOrName = this.extractBriefIdentifier(input);
// Parse input using dedicated URL parser
const parsed = BriefUrlParser.parse(input);
const briefIdOrName = parsed.briefId || input.trim();
// Get org ID from parameter or context
const resolvedOrgId = orgId || this.authManager.getContext()?.orgId;
// Resolve organization ID (priority: parameter > URL > context)
let resolvedOrgId = orgId;
// Try to extract org slug from URL if not provided
if (!resolvedOrgId && parsed.orgSlug) {
try {
const orgs = await this.authManager.getOrganizations();
const matchingOrg = orgs.find(
(org) =>
org.slug?.toLowerCase() === parsed.orgSlug?.toLowerCase() ||
org.name.toLowerCase() === parsed.orgSlug?.toLowerCase()
);
if (matchingOrg) {
resolvedOrgId = matchingOrg.id;
}
} catch {
// If we can't fetch orgs, fall through to context
}
}
// Fall back to context if still not resolved
if (!resolvedOrgId) {
resolvedOrgId = this.authManager.getContext()?.orgId;
}
if (!resolvedOrgId) {
throw new TaskMasterError(
@@ -66,64 +90,6 @@ export class BriefsDomain {
return matchingBrief;
}
/**
* Extract brief identifier from raw input
* Handles URLs, paths, and direct IDs
*
* @param input - Raw input string
* @returns Extracted brief identifier
*/
private extractBriefIdentifier(input: string): string {
const raw = input?.trim() ?? '';
if (!raw) {
throw new TaskMasterError(
'Brief identifier cannot be empty',
ERROR_CODES.VALIDATION_ERROR
);
}
const parseUrl = (s: string): URL | null => {
try {
return new URL(s);
} catch {}
try {
return new URL(`https://${s}`);
} catch {}
return null;
};
const fromParts = (path: string): string | null => {
const parts = path.split('/').filter(Boolean);
const briefsIdx = parts.lastIndexOf('briefs');
const candidate =
briefsIdx >= 0 && parts.length > briefsIdx + 1
? parts[briefsIdx + 1]
: parts[parts.length - 1];
return candidate?.trim() || null;
};
// 1) URL (absolute or schemeless)
const url = parseUrl(raw);
if (url) {
const qId = url.searchParams.get('id') || url.searchParams.get('briefId');
const candidate = (qId || fromParts(url.pathname)) ?? null;
if (candidate) {
return candidate;
}
}
// 2) Looks like a path without scheme
if (raw.includes('/')) {
const candidate = fromParts(raw);
if (candidate) {
return candidate;
}
}
// 3) Fallback: raw token (UUID, last 8 chars, or name)
return raw;
}
/**
* Switch to a different brief by name or ID
* Validates context, finds matching brief, and updates auth context

View File

@@ -0,0 +1,152 @@
/**
* @fileoverview URL Parser
* Utility for parsing URLs to extract organization and brief identifiers
*/
import {
ERROR_CODES,
TaskMasterError
} from '../../../common/errors/task-master-error.js';
export interface ParsedBriefUrl {
orgSlug: string | null;
briefId: string | null;
}
/**
* Utility class for parsing brief URLs
* Handles URL formats like: http://localhost:3000/home/{orgSlug}/briefs/{briefId}
*/
export class BriefUrlParser {
/**
* Parse a URL and extract org slug and brief ID
*
* @param input - Raw input string (URL, path, or ID)
* @returns Parsed components
*/
static parse(input: string): ParsedBriefUrl {
const raw = input?.trim() ?? '';
if (!raw) {
return { orgSlug: null, briefId: null };
}
// Try parsing as URL
const url = this.parseAsUrl(raw);
const pathToCheck = url ? url.pathname : raw.includes('/') ? raw : null;
if (!pathToCheck) {
// Not a URL/path, treat as direct ID
return { orgSlug: null, briefId: raw };
}
// Extract components from path
return this.parsePathComponents(pathToCheck, url);
}
/**
* Extract organization slug from URL or path
*
* @param input - Raw input string
* @returns Organization slug or null
*/
static extractOrgSlug(input: string): string | null {
return this.parse(input).orgSlug;
}
/**
* Extract brief identifier from URL or path
*
* @param input - Raw input string
* @returns Brief identifier or null
*/
static extractBriefId(input: string): string | null {
const parsed = this.parse(input);
return parsed.briefId || input.trim();
}
/**
* Try to parse input as URL
* Handles both absolute and scheme-less URLs
*/
private static parseAsUrl(input: string): URL | null {
try {
return new URL(input);
} catch {}
try {
return new URL(`https://${input}`);
} catch {}
return null;
}
/**
* Parse path components to extract org slug and brief ID
* Handles patterns like: /home/{orgSlug}/briefs/{briefId}
*/
private static parsePathComponents(
path: string,
url: URL | null
): ParsedBriefUrl {
const parts = path.split('/').filter(Boolean);
const briefsIdx = parts.lastIndexOf('briefs');
let orgSlug: string | null = null;
let briefId: string | null = null;
// Extract org slug (segment before 'briefs')
if (briefsIdx > 0) {
orgSlug = parts[briefsIdx - 1] || null;
}
// Extract brief ID
// Priority: query param > path segment after 'briefs' > last segment (if not 'briefs')
if (url) {
const qId = url.searchParams.get('id') || url.searchParams.get('briefId');
if (qId) {
briefId = qId;
}
}
if (!briefId && briefsIdx >= 0 && parts.length > briefsIdx + 1) {
briefId = parts[briefsIdx + 1];
}
// Only use last segment as fallback if path doesn't end with 'briefs'
// This prevents treating '/home/org/briefs' as briefId='briefs'
if (
!briefId &&
parts.length > 0 &&
!(briefsIdx >= 0 && briefsIdx === parts.length - 1)
) {
briefId = parts[parts.length - 1];
}
return { orgSlug, briefId };
}
/**
* Validate that required components are present
*
* @param parsed - Parsed URL components
* @param requireOrg - Whether org slug is required
* @param requireBrief - Whether brief ID is required
* @throws TaskMasterError if required components are missing
*/
static validate(
parsed: ParsedBriefUrl,
options: { requireOrg?: boolean; requireBrief?: boolean } = {}
): void {
if (options.requireOrg && !parsed.orgSlug) {
throw new TaskMasterError(
'Organization slug could not be extracted from input',
ERROR_CODES.VALIDATION_ERROR
);
}
if (options.requireBrief && !parsed.briefId) {
throw new TaskMasterError(
'Brief identifier could not be extracted from input',
ERROR_CODES.VALIDATION_ERROR
);
}
}
}