Compare commits

...
Author SHA1 Message Date
Tabish Bidiwale 8382ee8660 refactor: optimize tool state detection and improve code clarity
Address code review feedback:

1. **Parallelize tool state checks**: Changed from sequential `for` loop to
   `Promise.all()` for checking multiple tools simultaneously. This reduces
   I/O latency during extend mode initialization.

2. **Extract marker validation helper**: Created `fileHasMarkers()` helper
   function to eliminate code duplication between config file and slash
   command checks. Makes the logic clearer and more maintainable.

3. **Clarify slash command policy**: Added explicit comment that "at least
   one file with markers is sufficient" (not all required) for slash commands.
   This is correct because OpenSpec creates all files together - if any
   exists with markers, the tool was configured by OpenSpec.

4. **Simplify fresh init path**: Use `Object.fromEntries()` for cleaner
   initialization of all-false states.

Performance improvement: Extend mode now checks tools in parallel instead
of serially, reducing init time especially for projects with many tools.
2025-10-25 20:27:28 +11:00
Tabish Bidiwale 93326be3e9 fix: prevent false "already configured" detection for tools
Fixes #195

## Problem
Users with existing tool config files (like CLAUDE.md) would see those
tools marked as "already configured" even when running `openspec init`
for the first time. This caused confusion as users thought OpenSpec was
already set up when it wasn't.

Root causes:
1. Tool detection checked only for file existence, not OpenSpec ownership
2. Detection ran even in fresh projects without openspec/ folder

## Solution
Two-part fix:

1. **Conditional detection**: Only check tool configuration when in extend
   mode (when openspec/ directory already exists). Fresh initializations
   skip the check entirely, treating all tools as unconfigured.

2. **Marker-based validation**: For tools to be considered "configured by
   OpenSpec", their files must contain OpenSpec markers (<!-- OPENSPEC:START -->
   and <!-- OPENSPEC:END -->). For tools with both config files and slash
   commands (like Claude Code), BOTH must have markers.

## Changes
- Modified getExistingToolStates() to accept extendMode parameter
- Rewrote isToolConfigured() to verify OpenSpec markers in files
- Added OPENSPEC_MARKERS to imports
- Added 4 comprehensive test cases covering the new behavior

## Test Coverage
- Fresh init with existing CLAUDE.md → NOT shown as configured ✅
- Fresh init with existing slash commands → NOT shown as configured ✅
- Extend mode with OpenSpec files → shown as configured ✅
- Fresh init with global Codex prompts → NOT shown as configured ✅

All 240 tests pass.
2025-10-25 19:37:48 +11:00
2 changed files with 145 additions and 20 deletions
+64 -20
View File
@@ -21,6 +21,7 @@ import {
AI_TOOLS,
OPENSPEC_DIR_NAME,
AIToolOption,
OPENSPEC_MARKERS,
} from './config.js';
import { PALETTE } from './styles/palette.js';
@@ -388,7 +389,7 @@ export class InitCommand {
// Validation happens silently in the background
const extendMode = await this.validate(projectPath, openspecPath);
const existingToolStates = await this.getExistingToolStates(projectPath);
const existingToolStates = await this.getExistingToolStates(projectPath, extendMode);
this.renderBanner(extendMode);
@@ -627,35 +628,78 @@ export class InitCommand {
}
private async getExistingToolStates(
projectPath: string
projectPath: string,
extendMode: boolean
): Promise<Record<string, boolean>> {
const states: Record<string, boolean> = {};
for (const tool of AI_TOOLS) {
states[tool.value] = await this.isToolConfigured(projectPath, tool.value);
// Fresh initialization - no tools configured yet
if (!extendMode) {
return Object.fromEntries(AI_TOOLS.map(t => [t.value, false]));
}
return states;
// Extend mode - check all tools in parallel for better performance
const entries = await Promise.all(
AI_TOOLS.map(async (t) => [t.value, await this.isToolConfigured(projectPath, t.value)] as const)
);
return Object.fromEntries(entries);
}
private async isToolConfigured(
projectPath: string,
toolId: string
): Promise<boolean> {
const configFile = ToolRegistry.get(toolId)?.configFileName;
if (
configFile &&
(await FileSystemUtils.fileExists(path.join(projectPath, configFile)))
)
return true;
// A tool is only considered "configured by OpenSpec" if its files contain OpenSpec markers.
// For tools with both config files and slash commands, BOTH must have markers.
// For slash commands, at least one file with markers is sufficient (not all required).
const slashConfigurator = SlashCommandRegistry.get(toolId);
if (!slashConfigurator) return false;
for (const target of slashConfigurator.getTargets()) {
const absolute = slashConfigurator.resolveAbsolutePath(
projectPath,
target.id
);
if (await FileSystemUtils.fileExists(absolute)) return true;
// Helper to check if a file exists and contains OpenSpec markers
const fileHasMarkers = async (absolutePath: string): Promise<boolean> => {
try {
const content = await FileSystemUtils.readFile(absolutePath);
return content.includes(OPENSPEC_MARKERS.start) && content.includes(OPENSPEC_MARKERS.end);
} catch {
return false;
}
};
let hasConfigFile = false;
let hasSlashCommands = false;
// Check if the tool has a config file with OpenSpec markers
const configFile = ToolRegistry.get(toolId)?.configFileName;
if (configFile) {
const configPath = path.join(projectPath, configFile);
hasConfigFile = (await FileSystemUtils.fileExists(configPath)) && (await fileHasMarkers(configPath));
}
// Check if any slash command file exists with OpenSpec markers
const slashConfigurator = SlashCommandRegistry.get(toolId);
if (slashConfigurator) {
for (const target of slashConfigurator.getTargets()) {
const absolute = slashConfigurator.resolveAbsolutePath(projectPath, target.id);
if ((await FileSystemUtils.fileExists(absolute)) && (await fileHasMarkers(absolute))) {
hasSlashCommands = true;
break; // At least one file with markers is sufficient
}
}
}
// Tool is only configured if BOTH exist with markers
// OR if the tool has no config file requirement (slash commands only)
// OR if the tool has no slash commands requirement (config file only)
const hasConfigFileRequirement = configFile !== undefined;
const hasSlashCommandRequirement = slashConfigurator !== undefined;
if (hasConfigFileRequirement && hasSlashCommandRequirement) {
// Both are required - both must be present with markers
return hasConfigFile && hasSlashCommands;
} else if (hasConfigFileRequirement) {
// Only config file required
return hasConfigFile;
} else if (hasSlashCommandRequirement) {
// Only slash commands required
return hasSlashCommands;
}
return false;
}
+81
View File
@@ -1051,6 +1051,87 @@ describe('InitCommand', () => {
});
});
describe('already configured detection', () => {
it('should NOT show tools as already configured in fresh project with existing CLAUDE.md', async () => {
// Simulate user having their own CLAUDE.md before running openspec init
const claudePath = path.join(testDir, 'CLAUDE.md');
await fs.writeFile(claudePath, '# My Custom Claude Instructions\n');
queueSelections('claude', DONE);
await initCommand.execute(testDir);
// In the first run (non-interactive mode via queueSelections),
// the prompt is called with configured: false for claude
const firstCallArgs = mockPrompt.mock.calls[0][0];
const claudeChoice = firstCallArgs.choices.find(
(choice: any) => choice.value === 'claude'
);
expect(claudeChoice.configured).toBe(false);
});
it('should NOT show tools as already configured in fresh project with existing slash commands', async () => {
// Simulate user having their own custom slash commands
const customCommandDir = path.join(testDir, '.claude/commands/custom');
await fs.mkdir(customCommandDir, { recursive: true });
await fs.writeFile(
path.join(customCommandDir, 'mycommand.md'),
'# My Custom Command\n'
);
queueSelections('claude', DONE);
await initCommand.execute(testDir);
const firstCallArgs = mockPrompt.mock.calls[0][0];
const claudeChoice = firstCallArgs.choices.find(
(choice: any) => choice.value === 'claude'
);
expect(claudeChoice.configured).toBe(false);
});
it('should show tools as already configured in extend mode', async () => {
// First initialization
queueSelections('claude', DONE);
await initCommand.execute(testDir);
// Second initialization (extend mode)
queueSelections('cursor', DONE);
await initCommand.execute(testDir);
const secondCallArgs = mockPrompt.mock.calls[1][0];
const claudeChoice = secondCallArgs.choices.find(
(choice: any) => choice.value === 'claude'
);
expect(claudeChoice.configured).toBe(true);
});
it('should NOT show already configured for Codex in fresh init even with global prompts', async () => {
// Create global Codex prompts (simulating previous installation)
const codexPromptsDir = path.join(testDir, '.codex/prompts');
await fs.mkdir(codexPromptsDir, { recursive: true });
await fs.writeFile(
path.join(codexPromptsDir, 'openspec-proposal.md'),
'# Existing prompt\n'
);
queueSelections('claude', DONE);
await initCommand.execute(testDir);
const firstCallArgs = mockPrompt.mock.calls[0][0];
const codexChoice = firstCallArgs.choices.find(
(choice: any) => choice.value === 'codex'
);
// In fresh init, even global tools should not show as configured
expect(codexChoice.configured).toBe(false);
});
});
describe('error handling', () => {
it('should provide helpful error for insufficient permissions', async () => {
// This is tricky to test cross-platform, but we can test the error message