Skip to content

fix(desktop): align source_test metadata contract #7193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
VectorPeak wants to merge 2 commits into from
Closed

fix(desktop): align source_test metadata contract #7193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
455e522
fix(desktop): align source_test metadata contract
VectorPeak Jul 19, 2026
f991554
Merge branch 'main' into codex/source-test-metadata-contract
VectorPeak Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions packages/desktop/packages/session-tools-core/src/handlers/source-test.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,29 @@ interface CtxOverrides {
validateStdioMcpConnection?: SessionToolContext['validateStdioMcpConnection'];
}

const SHARED_CONNECTION_STATUSES = new Set<NonNullable<SourceConfig['connectionStatus']>>([
'connected',
'needs_auth',
'failed',
'untested',
]);
Comment on lines +27 to +32

@qwen-code-ci-bot qwen-code-ci-bot Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The assertSharedSourceMetadataContract helper hardcodes the valid status values as a set literal, but TypeScript's Set<T> type does not enforce exhaustiveness — if a new value is added to ConnectionStatus, this set silently becomes a subset and the helper will reject the new valid value at test runtime with a misleading "unsupported connectionStatus" error from the test infrastructure. — Concrete cost: A developer adds a new status value and writes handler code that produces it; the test fails with an infrastructure error that looks like a bug in the handler, not the test helper.

Suggested change
const SHARED_CONNECTION_STATUSES = new Set<NonNullable<SourceConfig['connectionStatus']>>([
'connected',
'needs_auth',
'failed',
'untested',
]);
const ALL_CONNECTION_STATUSES: ConnectionStatus[] = [
'connected',
'needs_auth',
'failed',
'untested',
] as const;
const SHARED_CONNECTION_STATUSES = new Set<ConnectionStatus>(ALL_CONNECTION_STATUSES);

— qwen3.7-max via Qwen Code /review


function assertSharedSourceMetadataContract(source: SourceConfig): void {
if (
source.lastTestedAt !== undefined &&
(!Number.isInteger(source.lastTestedAt) || source.lastTestedAt < 0)
) {
throw new Error('lastTestedAt must be a non-negative integer timestamp');
}

if (
source.connectionStatus !== undefined &&
!SHARED_CONNECTION_STATUSES.has(source.connectionStatus)
) {
throw new Error(`unsupported connectionStatus: ${source.connectionStatus}`);
}
}

function createCtx(workspacePath: string, overrides: CtxOverrides = {}): SessionToolContext {
const saved: { last?: SourceConfig } = {};
const ctx = {
Expand Down Expand Up @@ -58,6 +81,7 @@ function createCtx(workspacePath: string, overrides: CtxOverrides = {}): Session
return JSON.parse(readFileSync(configPath, 'utf-8')) as SourceConfig;
},
saveSourceConfig: (source: SourceConfig) => {
assertSharedSourceMetadataContract(source);
saved.last = source;
const configPath = join(workspacePath, 'sources', source.slug, 'config.json');
writeFileSync(configPath, JSON.stringify(source, null, 2));
Expand Down Expand Up @@ -149,6 +173,8 @@ describe('source_test auto-enable', () => {
readFileSync(join(tempDir, 'sources', 'craft-kb', 'config.json'), 'utf-8')
) as SourceConfig;
expect(persisted.enabled).toBe(true);
expect(Number.isInteger(persisted.lastTestedAt)).toBe(true);
expect(persisted.connectionStatus).toBe('connected');
});

it('already-enabled source still calls activation callback (session may be stale)', async () => {
Expand Down Expand Up @@ -192,6 +218,8 @@ describe('source_test auto-enable', () => {
) as SourceConfig;
// saveSourceConfig still runs (metadata update), but enabled flag must remain false.
expect(persisted.enabled).toBe(false);
expect(Number.isInteger(persisted.lastTestedAt)).toBe(true);
expect(persisted.connectionStatus).toBe('connected');
});

it('validation errors skip auto-enable entirely (even when autoEnable is default)', async () => {
Expand All @@ -217,6 +245,34 @@ describe('source_test auto-enable', () => {
readFileSync(join(tempDir, 'sources', 'broken', 'config.json'), 'utf-8')
) as SourceConfig;
expect(persisted.enabled).toBe(false);
expect(Number.isInteger(persisted.lastTestedAt)).toBe(true);
expect(persisted.connectionStatus).toBe('failed');
expect(persisted.connectionError).toBe('boom');
});

it('persists needs_auth when connection succeeds but auth is missing', async () => {
writeSource(tempDir, 'oauth-source', {
isAuthenticated: false,
mcp: {
transport: 'stdio',
command: 'echo',
args: ['ok'],
authType: 'oauth',
},
});

const ctx = createCtx(tempDir, {
validateStdioMcpConnection: stubMcpOk(),
});

const result = await handleSourceTest(ctx, { sourceSlug: 'oauth-source' });

expect(result.isError).toBe(false);
const persisted = JSON.parse(
readFileSync(join(tempDir, 'sources', 'oauth-source', 'config.json'), 'utf-8')
) as SourceConfig;
expect(Number.isInteger(persisted.lastTestedAt)).toBe(true);
expect(persisted.connectionStatus).toBe('needs_auth');
});

it('without activateSourceInSession, flag flip still happens with restart hint', async () => {
Expand Down
15 changes: 10 additions & 5 deletions packages/desktop/packages/session-tools-core/src/handlers/source-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export async function handleSourceTest(
const lines: string[] = [];
let hasErrors = false;
let hasWarnings = false;
let connectionStatus: ConnectionStatus = 'unknown';
let connectionStatus: ConnectionStatus = 'untested';
let connectionError: string | undefined;

// 1. Check source exists
Expand Down Expand Up @@ -122,19 +122,24 @@ export async function handleSourceTest(
lines.push(...connectionResult.lines);
if (connectionResult.hasError) {
hasErrors = true;
connectionStatus = 'error';
connectionStatus = 'failed';
connectionError = connectionResult.error;
} else if (connectionResult.success) {
connectionStatus = 'connected';
} else {
connectionStatus = 'disconnected';
connectionStatus = 'untested';
Comment on lines 129 to +130

@qwen-code-ci-bot qwen-code-ci-bot Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No test covers the 'untested' final status — the fallback branch when testConnection returns { success: false, hasError: false }. All existing tests use MCP sources whose stubs produce either success or error. — Concrete cost: If this fallback logic were changed incorrectly (e.g., a refactor that swapped the else branch), no test would catch it, and sources without a connection test handler would silently get the wrong status.

Add a test with a source configuration that causes testConnection to return { success: false, hasError: false }, asserting persisted.connectionStatus === 'untested'.

— qwen3.7-max via Qwen Code /review

}

// 7. Auth status
lines.push('\n## Authentication');
const authResult = await checkAuthStatus(ctx, source, sourceSlug);
lines.push(...authResult.lines);
if (authResult.hasWarning) hasWarnings = true;
if (authResult.hasWarning) {
hasWarnings = true;
if (!connectionResult.hasError) {
connectionStatus = 'needs_auth';
}
Comment on lines +139 to +141

@qwen-code-ci-bot qwen-code-ci-bot Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No test verifies that connectionStatus: 'failed' is preserved when both connection fails AND auth has warnings. The guard !connectionResult.hasError is the only thing preventing 'failed' from being overwritten with 'needs_auth'. — Failure scenario: A source with authType: 'oauth', isAuthenticated: false, and a failing MCP stub exercises both hasError = true (→ 'failed') and authResult.hasWarning = true. If this guard were accidentally removed, the status would incorrectly report 'needs_auth' — misleading the user into re-authenticating when the real issue is connectivity.

Suggested change
if (!connectionResult.hasError) {
connectionStatus = 'needs_auth';
}
if (!connectionResult.hasError) {
connectionStatus = 'needs_auth';
}

Add a test combining stubMcpFail() with isAuthenticated: false + authType: 'oauth', asserting persisted.connectionStatus === 'failed'.

— qwen3.7-max via Qwen Code /review

}

// 8. Auto-enable + metadata update
// Defaults to true; pass autoEnable: false to keep pure validation behavior.
Expand All @@ -145,7 +150,7 @@ export async function handleSourceTest(
if (ctx.saveSourceConfig) {
const updatedSource: SourceConfig = {
...source,
lastTestedAt: new Date().toISOString(),
lastTestedAt: Date.now(),
connectionStatus,
connectionError,
// Fold enabled flip into the same save — one write, not two.
Expand Down
4 changes: 2 additions & 2 deletions packages/desktop/packages/session-tools-core/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ export interface LocalSourceConfig {
/**
* Connection status for sources
*/
export type ConnectionStatus = 'connected' | 'disconnected' | 'error' | 'unknown';
export type ConnectionStatus = 'connected' | 'needs_auth' | 'failed' | 'untested';

/**
* Full source configuration (simplified version for core package)
Expand All @@ -315,7 +315,7 @@ export interface SourceConfig {
api?: ApiSourceConfig;
local?: LocalSourceConfig;
isAuthenticated?: boolean;
lastTestedAt?: string; // ISO date string
lastTestedAt?: number; // Unix timestamp in milliseconds
createdAt?: number;
updatedAt?: number;
// Display fields
Expand Down
Loading