Feature: MCP Server Integration and Gemini Extension Support #488
Conversation
- Add gemini-extension.json and GEMINI.md for native Gemini CLI integration - Extract slash command prompts to src/core/templates/prompts.ts for unified agent usage - Configure dynamic slash command generation and maintenance in GEMINI.md - Update README.md with Gemini extension installation guides and benefits - Add OpenSpec change proposal and spec deltas for the integration - Refactor templates and update existing tests to match improved prompt wording
Refactor core logic into standalone files (init-logic.ts, update-logic.ts, etc.) to enable reuse between CLI and MCP server. Expand OpenSpec MCP server with tools for init, update, view, create_change, and archive_change. Update documentation to reflect the new zero-install workflow and MCP capabilities.
Extract artifact workflow, configuration, and bulk validation logic into pure core functions. Register new MCP tools: openspec_config_*, openspec_artifact_*, openspec_validate_all, and openspec_list_schemas.
…sts plan - Archived the change as it was completed. - Updated proposal and tasks to include unused imports cleanup and emphasize test migration. - Includes file moves related to previous refactoring.
Refactor list/show/validate logic from ChangeCommand to src/core/change-logic.ts. Add comprehensive tests for core change logic and new MCP server tests covering tools, resources, and prompts. Update mcp-server spec with test requirements.
Refactor InitCommand to use dynamic imports for @InQuirer modules. Add mandatory descriptions to @ts-expect-error in MCP resources.
Update openspec_show_change execution to use refactored core change logic (getChangeMarkdown, getChangeJson) instead of invoking ChangeCommand methods.
Moved spec logic to src/core/spec-logic.ts and updated src/commands/spec.ts and src/mcp/tools.ts to use it.
📝 WalkthroughWalkthroughAdds a native MCP server and Gemini extension, migrates the OpenSpec layout to a hidden Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI (src/cli)
participant Cmd as Command (src/commands/*)
participant Core as Core Logic (src/core/*)
participant FS as FileSystem
User->>CLI: invokes CLI (e.g., create/change/serve)
CLI->>Cmd: call command.execute(...)
Cmd->>Core: call core function (runCreateChange/runInit/runArchive/runUpdate)
Core->>FS: read/write .openspec files (proposal.md, tasks.md, specs, AGENTS.md)
FS-->>Core: file results
Core-->>Cmd: structured result (CreateChangeResult / InitResult / ArchiveResult / UpdateResult)
Cmd-->>User: formatted output, prompts, or exit
sequenceDiagram
actor Gemini
participant MCP as OpenSpecMCPServer (stdio)
participant Core as Core Logic (src/core/*)
participant FS as FileSystem
Gemini->>MCP: tool call (e.g., openspec_create_change)
MCP->>Core: runCreateChange(projectRoot, name, {schema})
Core->>FS: validate/read/write OpenSpec resources
FS-->>Core: success/failure
Core-->>MCP: JSON CreateChangeResult
MCP-->>Gemini: tool response JSON
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-25T01:08:19.004ZApplied to files:
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
openspec/specs/cli-init/spec.md (1)
22-34: Directory structure updated to.openspec/but several references remain inconsistent.Lines 22 and 28 correctly update to
.openspec/, but multiple other lines still reference the oldopenspec/path:
- Line 41:
openspec/AGENTS.md- Line 88, 95, 96: References to
openspec/AGENTS.md- Line 114, 150:
openspec/directory checks- Line 125:
openspec/AGENTS.mdThese inconsistencies may cause confusion about the canonical path.
Example fixes (non-exhaustive)
- **THEN** generate `openspec/AGENTS.md` containing complete OpenSpec instructions + **THEN** generate `.openspec/AGENTS.md` containing complete OpenSpec instructions- - Full guidance lives in '@/openspec/AGENTS.md'. + - Full guidance lives in '@/.openspec/AGENTS.md'.src/core/specs-apply.ts (2)
101-329: Bug: ignored REMOVED operations are still counted asremovedin outputs/totals.
For new specs, REMOVED is ignored (Line 209-217) and tracked viaignoredRemovals, but the returnedcounts.removedremainsplan.removed.length(Line 321-326). ThenapplySpecs()adds that intototals.removed(Line 435-438), so callers will see removals that never happened.Proposed fix
// Load or create base target content let targetContent: string; let isNewSpec = false; let ignoredRemovals = 0; @@ } catch { @@ ignoredRemovals = plan.removed.length; isNewSpec = true; targetContent = buildSpecSkeleton(specName, changeName); } @@ return { rebuilt, counts: { added: plan.added.length, modified: plan.modified.length, - removed: plan.removed.length, + removed: isNewSpec ? 0 : plan.removed.length, renamed: plan.renamed.length, }, ignoredRemovals };
355-451: Path handling likely needs the same “OpenSpec dir resolver” used elsewhere.
applySpecs()hard-codespath.join(projectRoot, 'openspec', ...)(Line 363-365). If this PR is standardizing on.openspec/(or supporting both), this should likely call the shared resolver and derivechangeDir/mainSpecsDirfrom it.src/commands/artifact-workflow.ts (1)
741-775: Success output path can be wrong under.openspec(message hardcodesopenspec/...).
runCreateChange()resolves the OpenSpec directory (legacyopenspec/vs default.openspec/). The spinner success line should not hardcodeopenspec/changes/....Proposed fix
try { const projectRoot = process.cwd(); const result = await runCreateChange(projectRoot, name, { schema: options.schema }); @@ - spinner.succeed(`Created change '${name}' at openspec/changes/${name}/ (schema: ${result.schema})`); + const relDir = FileSystemUtils.toPosixPath(path.relative(projectRoot, result.changeDir)); + spinner.succeed(`Created change '${name}' at ${relDir}/ (schema: ${result.schema})`); } catch (error) {
🤖 Fix all issues with AI agents
In @gemini-extension.json:
- Around line 1-14: Update the "version" field in gemini-extension.json from
"0.18.0" to "0.19.0" so it matches package.json; alternatively run the provided
sync script scripts/check-extension-version.mjs (or the project's version-sync
utility) to automatically sync the extension manifest before committing.
In @GEMINI.md:
- Around line 5-6: Update the GEMINI.md text that claims a "zero-install
workflow" but then suggests `npm install -g "@fission-ai/openspec"`: remove or
reword the global-install recommendation and instead promote the bundled MCP
server as the default zero-install path; if a fallback is needed, recommend
using `npx` or `pnpm dlx` (or explicitly prompt the user for confirmation before
suggesting a global install). Also adjust the related sections (around the
current "zero-install workflow" wording and the block that shows `npm install
-g`/fallback) to reflect this non-global-install guidance.
In
@openspec/changes/archive/2025-12-21-add-gemini-extension-support/proposal.md:
- Around line 1-18: Rewrite the proposal.md to follow the required scaffold by
replacing the current freeform sections with three top-level headings: "Why"
(1–2 concise sentences summarizing motivation), "What Changes" (bullet-list the
concrete changes from the Proposed Solution such as creating
gemini-extension.json, GEMINI.md, unified prompts in
src/core/templates/prompts.ts, and .gemini/commands/openspec/*.toml; explicitly
mark any breaking changes), and "Impact" (list affected specs and code paths —
e.g., AGENTS.md, openspec/project.md, src/core/templates/prompts.ts, .gemini
command files — and note runtime/installation impacts). Keep each heading
concise and preserve the original content under the appropriate heading.
In @package.json:
- Line 81: The package.json lists a nonexistent fastmcp version "3.26.8"; update
the fastmcp dependency entry to a published version (e.g., change the version
string to "^3.15.2") or confirm the intended release and set the correct semver
value for the "fastmcp" entry so installs no longer fail.
In @src/core/artifact-logic.ts:
- Line 251: The changeDir assignment hardcodes 'openspec' like
validateChangeExists did; replace that hardcoded path by calling
resolveOpenSpecDir(projectRoot) and then joining its result with 'changes' and
name so changeDir is derived from resolveOpenSpecDir; update any related logic
in the same scope (where changeDir is used) to rely on the resolved directory
instead of the literal 'openspec' string.
- Around line 28-29: The code hardcodes const changesPath =
path.join(projectRoot, 'openspec') which breaks the legacy vs new directory
resolution; replace this with const changesPath =
path.join(resolveOpenSpecDir(projectRoot), 'changes') (or derive the changes
directory directly from resolveOpenSpecDir) and update the inline
getAvailableChanges function to use that new changesPath variable so all
reads/writes target the resolved .openspec/ or openspec/ directory consistently;
reference resolveOpenSpecDir, changesPath, and getAvailableChanges when making
the change.
In @src/core/config-logic.ts:
- Around line 30-61: In setConfigValue, you validate newConfig but persist
mutations to config, so any defaults/transforms from GlobalConfigSchema.parse()
are lost; update the validation step (validateConfig / GlobalConfigSchema.parse)
to return the parsed/configured object and then apply setNestedValue to that
parsedConfig (or merge the parsed result back into config) and call
saveGlobalConfig(parsedConfig as GlobalConfig) so the saved config includes
schema defaults/transforms; keep allowUnknown behavior only for key-path
validation and do not rely on it to control schema parsing.
In @src/core/validation-logic.ts:
- Around line 29-60: runBulkValidation currently builds paths using
process.cwd() which breaks when caller runs from a different working directory;
change runBulkValidation signature to accept a projectRoot (or call
resolveOpenSpecDir() inside it) and use that root instead of process.cwd() when
constructing changeDir and spec file paths and when discovering IDs (replace
getActiveChangeIds()/getSpecIds() calls to their variants that accept a root or
pass projectRoot into any existing resolution helpers). Update the calls to
Validator.validateChangeDeltaSpecs and Validator.validateSpec to receive the
resolved paths built from projectRoot, and preserve existing concurrency logic
(normalizeConcurrency/process.env.OPENSPEC_CONCURRENCY) and return shape.
In @src/mcp/resources.ts:
- Around line 6-54: The load functions registered in registerResources (the
three async load callbacks) currently interpolate variables.name / variables.id
into path.join, enabling path traversal; fix by validating and sanitizing those
variables before building the filePath: reject or return an error if the
variable contains path separators, '.' or '..' segments (e.g., contains '/' or
'\\' or equals '.' or '..'), then construct the absolute path with
path.resolve(openspecPath, 'changes'|'specs', variables.X, '<file>.md') and
assert the resolved path startsWith the resolved openspecPath to ensure it is
inside the OpenSpec dir; apply this check in the load functions for "Change
Proposal", "Change Tasks", and "Specification" (the load callbacks that
reference variables.name and variables.id) and return a safe error response if
validation fails.
🟡 Minor comments (13)
openspec/changes/archive/2026-01-12-agent-only-mcp-workflow/tasks.md-25-25 (1)
25-25: Fix duplicate section numbering.This section is numbered "3" but should be "5" since there's already a Section 3 at Line 16 and a Section 4 at Line 21.
🔢 Proposed fix
-## 3. Verification +## 5. Verificationopenspec/specs/mcp-server/spec.md-4-4 (1)
4-4: Update the TBD Purpose placeholder.The Purpose section contains a placeholder asking to be updated after archiving, but this change has already been archived. Please update this with the actual purpose of the MCP server specification.
📝 Suggested Purpose content
-TBD - created by archiving change add-mcp-server. Update Purpose after archive. +The MCP server specification defines the requirements and behavior for exposing OpenSpec functionality through the Model Context Protocol, enabling AI agents to interact with OpenSpec tools, resources, and prompts in a standardized way.src/commands/view.ts-5-71 (1)
5-71: Harden progress/percentage rendering against invalid inputs (completed > total, negatives, NaN).
Right nowpercentageandcreateProgressBar()implicitly trustchange.progress.*(Line 29-33, Line 110-121). If upstream ever returnscompleted > total(or negative), the bar can overflow and the percentage can exceed 100%.Proposed fix
- const progressBar = this.createProgressBar(change.progress.completed, change.progress.total); - const percentage = - change.progress.total > 0 - ? Math.round((change.progress.completed / change.progress.total) * 100) - : 0; + const progressBar = this.createProgressBar(change.progress.completed, change.progress.total); + const percentage = (() => { + const total = Number(change.progress.total); + const completed = Number(change.progress.completed); + if (!Number.isFinite(total) || total <= 0) return 0; + if (!Number.isFinite(completed) || completed <= 0) return 0; + return Math.max(0, Math.min(100, Math.round((completed / total) * 100))); + })();- private createProgressBar(completed: number, total: number, width: number = 20): string { - if (total === 0) return chalk.dim('─'.repeat(width)); - - const percentage = completed / total; - const filled = Math.round(percentage * width); + private createProgressBar(completed: number, total: number, width: number = 20): string { + const t = Number(total); + const c = Number(completed); + if (!Number.isFinite(t) || t <= 0) return chalk.dim('─'.repeat(width)); + if (!Number.isFinite(c) || c <= 0) return `[${chalk.dim('░'.repeat(width))}]`; + + const ratio = Math.max(0, Math.min(1, c / t)); + const filled = Math.round(ratio * width); const empty = width - filled;openspec/changes/archive/2026-01-12-add-mcp-server/specs/cli-init/spec.md-10-17 (1)
10-17: Add language specifier to fenced code block.The directory structure code block is missing a language identifier. Use
textorplaintextfor non-code content to satisfy markdownlint (MD040).Proposed fix
-``` +```text .openspec/ ├── project.md ├── AGENTS.md ├── specs/ └── changes/ └── archive/</details> </blockquote></details> <details> <summary>openspec/changes/archive/2026-01-12-agent-only-mcp-workflow/proposal.md-18-31 (1)</summary><blockquote> `18-31`: **Merge duplicate "Impact" sections into one.** There are three separate `## Impact` headings (lines 18, 23, 28), each with distinct bullet points. This violates markdown conventions (flagged by markdownlint) and fragments related information. Consolidate all impact points under a single `## Impact` section. <details> <summary>📝 Suggested fix</summary> ```diff ## Impact - **Architecture Cleanliness**: Enforces separation between presentation (CLI/MCP) and logic (Core). - **Full Parity**: Ensures agents have the exact same "superpowers" as users on the command line. - **Continuous Reliability**: CI ensures that refactoring for MCP parity never breaks the legacy CLI experience. - -## Impact -- **Architecture Cleanliness**: Enforces separation between presentation (CLI/MCP) and logic (Core). - **Flexibility**: Users can choose between CLI, MCP, or both. - **Adoption**: Significantly lowers the barrier for entry by allowing agents to "self-initialize" via MCP. - -## Impact - **Zero-Install Adoption**: Users only need to add the Gemini extension; no separate CLI installation is required for AI-driven workflows. - **Consistent Agent Experience**: Agents interact with a structured API (MCP) rather than parsing CLI output or managing shell command strings. - **Future-Proofing**: Aligns OpenSpec with the emerging "plugin" architecture of modern AI coding assistants.Note: Line 24 duplicates "Architecture Cleanliness" from line 19—remove the duplicate when merging.
openspec/changes/archive/2026-01-12-add-mcp-server/tasks.md-32-35 (1)
32-35: Fix task numbering in “7. Verification” (currently labeled 6.x).These look like copy/paste leftovers and break the “section.task” convention.
Proposed patch
## 7. Verification -- [x] 6.1 Verify `openspec serve` starts and communicates over stdio. -- [x] 6.2 Verify tools, resources, and prompts are discoverable by an MCP client. -- [x] 6.3 Verify `openspec init` creates `.openspec/`. +- [x] 7.1 Verify `openspec serve` starts and communicates over stdio. +- [x] 7.2 Verify tools, resources, and prompts are discoverable by an MCP client. +- [x] 7.3 Verify `openspec init` creates `.openspec/`.Also applies to: 28-31
openspec/changes/archive/2026-01-12-add-mcp-tests/tasks.md-1-29 (1)
1-29: Align tasks.md with the required “numbered sections + checkbox items” format.This reads like a checklist, but the sections aren’t numbered (e.g.,
## 1. ...,- [ ] 1.1 ...), which is the convention foropenspec/changes/*/tasks.md. Based on learnings, please convert headings/items to the numbered format for consistency across changes.Proposed patch
# Implementation Tasks -## Spec Updates +## 1. Spec Updates - [x] Update `openspec/specs/mcp-server/spec.md` to include test coverage and shared logic requirements. -## Refactoring (CLI -> Core) -- [x] Refactor `getActiveChanges` from `src/commands/change.ts` to `src/core/change-logic.ts`. +## 2. Refactoring (CLI -> Core) +- [x] 2.1 Refactor `getActiveChanges` from `src/commands/change.ts` to `src/core/change-logic.ts`. - [x] Refactor `getChangeMarkdown` and `getChangeJson` (logic part) to `src/core/change-logic.ts`. - [x] Refactor `validate` logic to `src/core/change-logic.ts` (or `validation-logic.ts`). - [x] Update `src/commands/change.ts` to use the new core functions. ... -## Testing +## 3. Testing ### Core ... ### MCP ... -## Cleanup +## 4. Cleanup ... -## Verification +## 5. Verification - [x] Verify all tests pass with `npm test`.openspec/specs/ci-sync/spec.md-3-5 (1)
3-5: Replace the “TBD” Purpose with a stable statement.Since this is now in
openspec/specs/(not a change delta), leavingTBDwill propagate ambiguous guidance to agents/users. Suggest a concrete purpose like: “Ensure CI enforces version parity betweenpackage.jsonandgemini-extension.json.”test/core/view.test.ts-5-58 (1)
5-58: Potential test flake:specs/dir not created in “categorize changes” case.If
getViewData()/getSpecsData()assumes${openspecDir}/specsexists, Line 36+ may fail depending on implementation. Create thespecsdir in that test as well.Proposed fix
it('should categorize changes correctly', async () => { const openspecPath = path.join(tempDir, 'openspec'); const changesDir = path.join(openspecPath, 'changes'); + const specsDir = path.join(openspecPath, 'specs'); await fs.mkdir(changesDir, { recursive: true }); + await fs.mkdir(specsDir, { recursive: true });test/core/init.test.ts-47-59 (1)
47-59: Usefs.mkdtemp()for temp dirs to reduce collision risk (especially under parallel Vitest runs).
Date-based names can still collide;mkdtempis the standard safe pattern.Proposed change
beforeEach(async () => { - tempDir = path.join(os.tmpdir(), `openspec-init-test-${Date.now()}`); - await fs.mkdir(tempDir, { recursive: true }); + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-init-test-'));test/core/change-logic.test.ts-51-60 (1)
51-60: Add at least one stable contract assertion forgetChangeJsonbeyond “defined”.
A low-cost invariant is thatdeltaCount === deltas.lengthand is a number; this catches accidental API regressions without depending on the parser’s internal delta heuristics.Proposed assertion
it('getChangeJson returns parsed JSON object', async () => { const json = await getChangeJson(tempRoot, changeName); expect(json.id).toBe(changeName); expect(json.title).toBe('Demo Change'); expect(json.deltas).toBeDefined(); + expect(typeof json.deltaCount).toBe('number'); + expect(json.deltaCount).toBe(json.deltas.length); });test/core/change-logic.test.ts-19-34 (1)
19-34: Preferfs.mkdtemp()and remove the unnecessaryprocess.chdir()call.
All functions tested here acceptprojectRootexplicitly and don't depend on the current working directory. Theprocess.chdir(tempRoot)call is unnecessary sinceresolveOpenSpecDir,validateChange, and other tested functions are pure with respect to cwd.Suggested fix
- tempRoot = path.join(os.tmpdir(), `openspec-core-change-logic-${Date.now()}`); + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-core-change-logic-')); ... - process.chdir(tempRoot);Also update
afterAllto remove theprocess.chdir(originalCwd)call or keep it only for safety if other test files may have side effects.test/core/archive.test.ts-10-27 (1)
10-27: Restore original working directory in afterEach to prevent test pollution.Using
process.chdir(tempDir)without restoring the original directory inafterEachcan cause issues if a test fails mid-execution or when running tests in parallel. Subsequent tests may run from an unexpected directory.Proposed fix
describe('runArchive', () => { let tempDir: string; + let originalCwd: string; beforeEach(async () => { + originalCwd = process.cwd(); tempDir = path.join(os.tmpdir(), `openspec-archive-test-${Date.now()}`); // ... }); afterEach(async () => { + process.chdir(originalCwd); await fs.rm(tempDir, { recursive: true, force: true }); });
🧹 Nitpick comments (46)
src/core/config-logic.ts (1)
57-60: Use safer string rendering fordisplayValue; consider deep-copy for reset.
- Line 58:
"${coercedValue}"won’t escape quotes/newlines;JSON.stringify(coercedValue)is safer for CLI/MCP output.- Line 79:
{ ...DEFAULT_CONFIG }is a shallow copy; today it’s fine, but it’s easy to regress if defaults become nested.Proposed tweak
const displayValue = - typeof coercedValue === 'string' ? `"${coercedValue}"` : String(coercedValue); + typeof coercedValue === 'string' ? JSON.stringify(coercedValue) : String(coercedValue); return { key, value: coercedValue, displayValue }; } ... export function resetConfig(all: boolean): boolean { if (!all) { throw new Error('All flag is required for reset'); } - saveGlobalConfig({ ...DEFAULT_CONFIG }); + saveGlobalConfig(JSON.parse(JSON.stringify(DEFAULT_CONFIG))); return true; }Also applies to: 74-81
scripts/check-extension-version.mjs (1)
1-11: Consider adding error handling for missing or malformed files.The script works correctly for the happy path. However, if either file is missing or contains invalid JSON, the script throws an unhandled exception with a stack trace rather than a user-friendly error message.
🔧 Optional: Add error handling for clearer CI failures
import fs from 'fs'; -const pkg = JSON.parse(fs.readFileSync('package.json', 'utf-8')); -const ext = JSON.parse(fs.readFileSync('gemini-extension.json', 'utf-8')); +let pkg, ext; +try { + pkg = JSON.parse(fs.readFileSync('package.json', 'utf-8')); +} catch (e) { + console.error('Failed to read package.json:', e.message); + process.exit(1); +} +try { + ext = JSON.parse(fs.readFileSync('gemini-extension.json', 'utf-8')); +} catch (e) { + console.error('Failed to read gemini-extension.json:', e.message); + process.exit(1); +} if (pkg.version !== ext.version) { console.error(`Version mismatch! package.json: ${pkg.version}, gemini-extension.json: ${ext.version}`); process.exit(1); } console.log('Version check passed.');openspec/changes/archive/2026-01-12-add-mcp-tests/proposal.md (1)
1-27: Proposal structure slightly differs from convention.Based on learnings,
proposal.mdshould include explicit sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code).This proposal has Goal and Motivation (covering Why), and Success Criteria (covering What Changes), but lacks an explicit Impact section listing affected specs and code files. Since this is archived, it's informational only—consider aligning future proposals with the convention.
openspec/specs/cli-init/spec.md (1)
27-34: Add language identifier to fenced code block.The code block showing the directory structure lacks a language specifier, triggering MD040. Use
textorplaintextfor non-code content.Proposed fix
-- **THEN** create the following directory structure: - ``` + ```text .openspec/ ├── project.mdopenspec/changes/archive/2026-01-12-add-mcp-server/proposal.md (1)
3-28: Consider adding an explicit Impact section per project conventions.Based on learnings, proposal.md files should include sections for Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code). While this proposal covers the content well, it would benefit from an explicit "Impact" section listing affected specifications and code areas, such as:
- New specs: mcp-server/spec.md
- Modified code: CLI commands, core logic extraction
- Migration impact: openspec/ → .openspec/ directory change
📋 Suggested Impact section
Add after the Solution section:
+## Impact +**Affected Specifications:** +- New: `mcp-server/spec.md` +- Modified: CLI command specifications + +**Affected Code:** +- New: `src/mcp/` (server, tools, resources, prompts) +- Modified: `src/core/` (logic extraction), `src/commands/` (refactored to use core logic) +- New: `gemini-extension.json`, `GEMINI.md` + +**Breaking Changes:** +- ⚠️ Directory migration: `openspec/` → `.openspec/` (with backward compatibility and deprecation warnings)Based on learnings, proposal structure should follow project conventions.
src/commands/view.ts (1)
5-71: Make error printing resilient whenerroris not anError.
chalk.red(error.message)(Line 69) can printundefinedor throw iferroris a string/unknown.Proposed fix
- } catch (error: any) { - console.error(chalk.red(error.message)); - process.exit(1); - } + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + console.error(chalk.red(message)); + process.exit(1); + }src/core/update-logic.ts (2)
41-71:updatedFilesvscreatedFilesaccounting is confusing (created files are also marked updated).
Today you alwaysupdatedFiles.push(...)(Line 62) and additionallycreatedFiles.push(...)for new files (Line 64-66), so newly created config files appear in both lists.Proposed fix
- await configurator.configure(resolvedProjectPath, openspecPath); - updatedFiles.push(configurator.configFileName); - - if (!fileExists) { - createdFiles.push(configurator.configFileName); - } + await configurator.configure(resolvedProjectPath, openspecPath); + if (fileExists) updatedFiles.push(configurator.configFileName); + else createdFiles.push(configurator.configFileName);
73-88: Surface which slash files were attempted vs successfully updated.
Right nowupdatedSlashFilesonly contains successful updates (Line 83-84), while failures only recordtoolId(Line 85-87). If a tool updates multiple files, it may help to include “attempted” file paths (or at least report “0 files updated” explicitly) for easier debugging.src/core/validation-logic.ts (2)
68-101: Concurrency worker loop should guarantee stable mapping between planned item and results (especially for errors).
In the.catch()path you reconstruct{ id, type }fromcurrentIndex(Line 83-85). This is generally fine, but it’s fragile if queue construction ever changes (e.g., interleaving change/spec tasks).A low-effort hardening is to carry
{ id, type }alongside each queued task instead of re-deriving it from indices.
103-117: Avoid a fixed'1.0'version string unless it’s a formal API version.
If consumers expect tool output versioning, consider using package version (or a dedicated protocol version constant) rather than a hard-coded string (Line 116).src/core/specs-apply.ts (1)
347-350: Skeleton text says “created by archiving” even when just applying specs.
buildSpecSkeleton()(Line 347-350) is used byapplySpecs()too; the message may be misleading.src/mcp/index.ts (1)
1-11: Preferprocess.exitCode = 1overprocess.exit(1)to allow stream flushing.
Not a blocker, butprocess.exit(1)can cut off pending writes in some environments (Line 10).Proposed tweak
main().catch((error) => { console.error("Failed to start OpenSpec MCP Server:", error); - process.exit(1); + process.exitCode = 1; });test/mcp/server.test.ts (1)
4-10: Consider expanding test coverage for the MCP server.The instantiation test is valid, but the
OpenSpecMCPServerclass has astart()method that registers tools, resources, and prompts before starting the transport. Consider adding tests that verify:
- Tools, resources, and prompts are registered after
start()is called- Error handling if the server fails to start
The
as anycast to access the privateserverproperty is a pragmatic approach for testing internals.openspec/specs/cli-spec/spec.md (1)
88-94: LGTM - requirement follows spec conventions.The requirement correctly uses SHALL for normative language and includes a scenario. Consider whether additional scenarios for error handling or graceful shutdown would be valuable for completeness (e.g., "Scenario: Graceful shutdown on SIGINT").
src/commands/validate.ts (1)
159-166: Minor: Inconsistent spacing in next steps output.The comment alignment appears inconsistent between the two lines. The
archivecommand is longer thanshow, but both have similar spacing before the comment, which will result in misaligned output.♻️ Suggested fix for aligned output
if (type === 'change') { console.log(); console.log(chalk.bold('Next steps:')); - console.log(` ${chalk.white('openspec show')} ${chalk.cyan(id)} ${chalk.gray('# Inspect change details')}`); - console.log(` ${chalk.white('openspec archive')} ${chalk.cyan(id)} ${chalk.gray('# Archive when tasks are complete')}`); + console.log(` ${chalk.white('openspec show')} ${chalk.cyan(id)} ${chalk.gray('# Inspect change details')}`); + console.log(` ${chalk.white('openspec archive')} ${chalk.cyan(id)} ${chalk.gray('# Archive when tasks are complete')}`); console.log(); }Alternatively, consider computing padding based on the longest command to ensure alignment regardless of change ID length.
scripts/sync-extension-version.mjs (1)
7-8: Consider adding error handling for file operations.The script will crash with an unhelpful stack trace if either file is missing or contains invalid JSON. Since this runs in CI, clearer error messages would improve debugging.
♻️ Suggested improvement with error handling
-const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')); -const ext = JSON.parse(readFileSync(extPath, 'utf-8')); +let pkg, ext; +try { + pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')); +} catch (err) { + console.error(`Failed to read package.json: ${err.message}`); + process.exit(1); +} +try { + ext = JSON.parse(readFileSync(extPath, 'utf-8')); +} catch (err) { + console.error(`Failed to read gemini-extension.json: ${err.message}`); + process.exit(1); +}test/mcp/tools.test.ts (2)
5-11: Consider extracting shared mock to reduce duplication.This
MockFastMCPpattern is repeated inprompts.test.tsand likelyresources.test.ts. Consider extracting to a shared test utility (e.g.,test/mcp/helpers.ts).Example shared mock
// test/mcp/helpers.ts export class MockFastMCP { tools: any[] = []; prompts: any[] = []; resources: any[] = []; addTool(tool: any) { this.tools.push(tool); } addPrompt(prompt: any) { this.prompts.push(prompt); } addResourceTemplate(resource: any) { this.resources.push(resource); } }
44-50: Schema test could be more thorough.The test only verifies that
parametersis defined. Consider validating the schema shape to catch regressions if the tool's parameter contract changes.Example enhancement
it('openspec_create_change has correct schema', () => { registerTools(server as unknown as FastMCP); const tool = server.tools.find(t => t.name === 'openspec_create_change'); expect(tool).toBeDefined(); expect(tool.parameters).toBeDefined(); - // Zod schema parsing is internal, but we can check if it exists + // Validate expected shape of the Zod schema + expect(tool.parameters.shape).toHaveProperty('name'); + expect(tool.parameters.shape).toHaveProperty('schema'); });test/mcp/prompts.test.ts (2)
37-45: Clarify or remove the ambiguous comment.The comment on lines 39-41 is confusing—it hedges about what the assertion actually validates. If the assertion is correct, simplify the comment; if there's genuine uncertainty about the transformation behavior, the test may need adjustment.
Suggested simplification
// Check for replacement of CLI commands with MCP tools expect(text).toContain('openspec_list_changes'); - expect(text).not.toContain('openspec list'); // Should be replaced/not present as primary instruction ideally, - // but regex replacement might leave some if strictly looking for full command lines. - // The toMcpInstructions function replaces specific patterns. + // CLI commands should be transformed to MCP tool calls // Check for specific replacements expect(text).toContain('openspec_validate_change(name: "');
5-10: Duplicate mock class.Same
MockFastMCPpattern as intools.test.ts. Consider consolidating into a shared test helper.test/mcp/resources.test.ts (1)
1-10: Useimport type+ tighten the mock surface (avoidany+ runtime import).Right now
FastMCPis imported as a value but only used for a type cast. Preferimport typeand type the mock to the minimal API you need.Proposed patch
import { describe, it, expect, beforeEach } from 'vitest'; import { registerResources } from '../../src/mcp/resources.js'; -import { FastMCP } from 'fastmcp'; +import type { FastMCP } from 'fastmcp'; +type ResourceTemplate = { uriTemplate: string; load: unknown }; + class MockFastMCP { - resources: any[] = []; - addResourceTemplate(resource: any) { + resources: ResourceTemplate[] = []; + addResourceTemplate(resource: ResourceTemplate) { this.resources.push(resource); } }Also applies to: 19-33
GEMINI.md (2)
62-67: Fix.openspec/vsopenspec/path inconsistencies (and document legacy behavior explicitly).
This doc describes checking/reading/writingopenspec/..., but the PR’s init logic/tests indicate the default is.openspec/withopenspec/as legacy. If you intentionally provide a compatibility alias (e.g., stub/symlink) soopenspec/...stays valid, call that out explicitly here; otherwise update paths to.openspec/...and mention the legacy directory name. (Based on learnings, the authoritative conventions live inopenspec/AGENTS.md/ related guidelines, so path correctness matters.)Proposed doc patch
-3. **Check for Project Initialization**: Verify if the `openspec/` directory exists in the project root. - - If missing, automatically run `openspec init` to scaffold the project structure. +3. **Check for Project Initialization**: Verify if the `.openspec/` directory exists in the project root. + - If missing, automatically run `openspec init` to scaffold the project structure. + - If you find a legacy `openspec/` directory, prefer `.openspec/` (migrate when safe) and treat `openspec/` as legacy. ... - - **Action**: Read `openspec/project.md`, analyze project config (`package.json`, etc.), and fill in the details. **Do not** ask the user to do this manually. + - **Action**: Read `.openspec/project.md`, analyze project config (`package.json`, etc.), and fill in the details. **Do not** ask the user to do this manually. ... -- `openspec/project.md`: Project-specific conventions and tech stack. -- `openspec/specs/`: Current requirements and scenarios (the "truth"). -- `openspec/changes/`: Pending proposals and implementation tasks. +- `.openspec/project.md`: Project-specific conventions and tech stack. +- `.openspec/specs/`: Current requirements and scenarios (the "truth"). +- `.openspec/changes/`: Pending proposals and implementation tasks. ... -For more detailed instructions, refer to `openspec/AGENTS.md`. +For more detailed instructions, refer to `.openspec/AGENTS.md` (or `openspec/AGENTS.md` if your project still uses the legacy directory name).Also applies to: 89-94, 105-106
107-118: Point maintenance instructions atsrc/core/templates/slash-command-templates.tsto avoid doc/code drift.
Right now you describe manually re-deriving bodies fromsrc/core/templates/prompts.ts; but the repo now has a dedicated mapping insrc/core/templates/slash-command-templates.tsthat encodes which constants belong to which command.Proposed doc patch
-The slash commands for this extension (`.gemini/commands/openspec/*.toml`) are generated artifacts. The single source of truth for their content is `src/core/templates/prompts.ts`. +The slash commands for this extension (`.gemini/commands/openspec/*.toml`) are generated artifacts. The single source of truth for command-to-body composition is `src/core/templates/slash-command-templates.ts` (which pulls from `src/core/templates/prompts.ts`).src/core/templates/slash-command-templates.ts (1)
12-22: MakeslashCommandBodiesreadonly to prevent accidental mutation across callers.
Since this is shared “template source-of-truth” data, it’s safer if it can’t be mutated at runtime.Proposed change
-export const slashCommandBodies: Record<SlashCommandId, string> = { +export const slashCommandBodies: Readonly<Record<SlashCommandId, string>> = Object.freeze({ proposal: [PROPOSAL_GUARDRAILS, PROPOSAL_STEPS, PROPOSAL_REFERENCES].join('\n\n'), apply: [BASE_GUARDRAILS, APPLY_STEPS, APPLY_REFERENCES].join('\n\n'), archive: [BASE_GUARDRAILS, ARCHIVE_STEPS, ARCHIVE_REFERENCES].join('\n\n') -}; +});test/core/init.test.ts (4)
5-13: Split/rename describes: this suite is not justrunInit.
MixingrunInitand interactiveInitCommandtests underdescribe('runInit')will make future failures misleading.Proposed structure tweak
-describe('runInit', () => { +describe('init', () => { let tempDir: string; let mockPrompt: any; let initCommand: InitCommand; + + describe('runInit', () => { + // keep the runInit-focused tests here + }); + + describe('InitCommand', () => { + // keep prompt-driven tests here + });Also applies to: 12-16
37-45: DeduplicatefileExists: delegate toFileSystemUtils.fileExists().
You already importFileSystemUtils; using it keeps test helpers consistent with production behavior and reduces local boilerplate.Proposed change
- const fileExists = async (p: string) => { - try { - await fs.stat(p); - return true; - } catch { - return false; - } - }; + const fileExists = (p: string) => FileSystemUtils.fileExists(p);
109-119: Brittle prompt call indexing (mockPrompt.mock.calls[1])—prefer “last relevant call” selection.
IfInitCommand.execute()adds an extra prompt later, these tests will break even if behavior is correct. Consider pulling the last call (or filtering calls bybaseMessagecontent) instead of assuming index1is always “second run tool selection”.Also applies to: 121-131, 133-141, 143-153, 155-165, 167-177, 179-201
728-737: Legacy migration test looks good; consider also assertingextendModeto lock behavior.
After renamingopenspec/→.openspec/,extendModeshould likely betrue(directory exists) — asserting it would catch accidental regressions in migration flow.src/core/view-logic.ts (1)
43-43: Consider using async fs methods for consistency.The module mixes synchronous (
readdirSync,existsSync,readFileSync) and asynchronous (getTaskProgressForChange) filesystem operations. While functionally correct, usingfs.promisesthroughout would improve consistency and avoid blocking the event loop on larger directories.♻️ Optional: Use async fs methods
- const entries = fs.readdirSync(changesDir, { withFileTypes: true }); + const entries = await fs.promises.readdir(changesDir, { withFileTypes: true });- const content = fs.readFileSync(specFile, 'utf-8'); + const content = await fs.promises.readFile(specFile, 'utf-8');Also applies to: 80-80, 86-88
src/commands/archive.ts (1)
95-109: Potentially unreachable validation failure handling.Since
runOptions.throwOnValidationErroris set totrueat line 56, validation failures will throw aValidationErrorcaught in thecatchblock (lines 78-93). The check at lines 99-109 for!result.validationReport.validappears unreachable under normal flow.Consider removing this block or clarifying when it would execute.
♻️ Remove unreachable code
if (result.alreadyExists) { throw new Error(`Archive '${result.archiveName}' already exists.`); } - if (result.validationReport && !result.validationReport.valid) { - console.log(chalk.red(`\nValidation failed for '${changeName}':`)); - for (const issue of result.validationReport.issues) { - if (issue.level === 'ERROR') { - console.log(chalk.red(` ✗ ${issue.message}`)); - } else if (issue.level === 'WARNING') { - console.log(chalk.yellow(` ⚠ ${issue.message}`)); - } - } - return; - } - console.log(`Task status: ${formatTaskStatus(result.taskStatus)}`);test/core/list.test.ts (1)
21-21: Consider renaming the nested describe block.The nested
describe('execute', ...)block is a holdover from the class-basedListCommandAPI. Since the tests now exercise thelistChangesfunction directly, consider renaming to something more descriptive likedescribe('listChanges behavior', ...)or simply flattening into the parent describe block.src/commands/list.ts (1)
9-34: Consider extractingformatRelativeTimeto a shared utility.The comment on lines 11-13 correctly identifies this as presentation logic that was copied from
core/list.ts. If this function is used elsewhere or likely to be reused, consider moving it tosrc/utils/format.tsto avoid duplication.The implementation itself is correct and handles edge cases appropriately (negative diffs would show "just now", which is reasonable).
src/commands/change.ts (1)
64-101: Remove commented-out code.Line 89 contains a commented-out variable declaration that's no longer needed after the refactor.
Proposed fix
// Long format: id: title and minimal counts - // const changesPath = path.join(await resolveOpenSpecDir(process.cwd()), 'changes'); // unused now for (const changeName of sorted) {src/core/archive-logic.ts (2)
31-56: HardcodedtargetPath = '.'limits reusability.The function hardcodes
targetPath = '.'on line 35, meaning it always operates from the current working directory. This limits reusability and testability. Consider acceptingtargetPathas a parameter.Proposed fix
export async function runArchive( changeName: string, - options: { skipSpecs?: boolean; noValidate?: boolean; validate?: boolean; throwOnValidationError?: boolean } = {} + options: { skipSpecs?: boolean; noValidate?: boolean; validate?: boolean; throwOnValidationError?: boolean; targetPath?: string } = {} ): Promise<ArchiveResult> { - const targetPath = '.'; + const targetPath = options.targetPath ?? '.'; const openspecPath = await resolveOpenSpecDir(targetPath);
14-29: Replaceanytypes with properValidationReporttype from the validation types module.The
validationReportfield (line 18) andValidationError.reportproperty (line 25) are currently typed asany, reducing type safety. Replace with theValidationReportinterface exported from the validation types module.Suggested improvement
+import type { ValidationReport } from './validation/types.js'; + export interface ArchiveResult { changeName: string; archiveName: string; taskStatus: { total: number; completed: number }; - validationReport?: any; + validationReport?: ValidationReport; specUpdates: Array<{ capability: string; status: 'create' | 'update' }>; totals: { added: number; modified: number; removed: number; renamed: number }; alreadyExists?: boolean; } export class ValidationError extends Error { - constructor(public report: any) { + constructor(public report: ValidationReport) { super('Validation failed'); this.name = 'ValidationError'; } }test/core/update.test.ts (1)
38-56: Describe block name doesn't match test implementation.The describe block is named
'runUpdate'but most tests (lines 58+) useupdateCommand.execute()rather than therunUpdatefunction directly. Only the first two tests actually exerciserunUpdate. This inconsistency could confuse future maintainers.Consider either:
- Renaming the describe block to reflect that it tests update functionality generally
- Updating more tests to use
runUpdatedirectlySuggested rename
-describe('runUpdate', () => { +describe('Update functionality', () => {src/core/init-logic.ts (2)
30-56: Clarify behavior when both legacy and default directories exist.When both
openspec/(legacy) and.openspec/(default) directories exist (lines 52-55), the code silently uses the legacy path. This could be surprising to users who expect the default.openspec/to be used.Consider logging a warning or documenting this behavior in the function's JSDoc.
89-96: Simplify duplicated directory creation calls.Both branches of the if/else call
createDirectoryStructure(openspecPath). Only theskipExistingparameter differs forwriteTemplateFiles.Proposed simplification
- // Step 1: Create directory structure - if (!extendMode) { - await createDirectoryStructure(openspecPath); - await writeTemplateFiles(openspecPath, { aiTools: selectedToolIds }, false); - } else { - await createDirectoryStructure(openspecPath); - await writeTemplateFiles(openspecPath, { aiTools: selectedToolIds }, true); - } + // Step 1: Create directory structure and write templates + await createDirectoryStructure(openspecPath); + await writeTemplateFiles(openspecPath, { aiTools: selectedToolIds }, extendMode);src/commands/init.ts (2)
432-440: Tool spinner creates misleading UX since work is already complete.The comment acknowledges that
runInitalready configured the tools, but the spinner still shows "Configuring AI tools..." after the fact. This creates a false impression of ongoing work.Consider either:
- Removing the tool spinner entirely since the work is already done
- Passing a progress callback to
runInitto update spinners in real-time- At minimum, changing this to a simple success message rather than a spinner
Suggested simplification
- const toolSpinner = this.startSpinner('Configuring AI tools...'); - // runInit already did this, but we want the spinner experience in CLI. - // Actually, runInit is meant to be the shared logic. - // To keep spinners, we might need to split runInit or accept progress callbacks. - // For now, let's just finish the spinner. - toolSpinner.stopAndPersist({ - symbol: PALETTE.white('▌'), - text: PALETTE.white('AI tools configured'), - }); + console.log(`${PALETTE.white('▌')} ${PALETTE.white('AI tools configured')}`);
633-642: Minor: Unnecessary type assertion in array literal.The
as ToolWizardChoice[]cast is redundant since the array already satisfies the type through the object literal typing.Simplification
...(availableTools.length - ? ([ + ? [ { kind: 'info' as const, value: LIST_SPACER_VALUE, label: { primary: '' }, selectable: false, }, - ] as ToolWizardChoice[]) + ] : []),src/core/list.ts (2)
3-8: Inconsistent import style: mixing sync and async fs APIs.The file imports both
promises as fs(async) andreadFileSync(sync). This inconsistency can lead to confusion and mixing blocking with non-blocking code.Prefer consistent async usage
import { promises as fs } from 'fs'; import path from 'path'; import { getTaskProgressForChange } from '../utils/task-progress.js'; -import { readFileSync } from 'fs'; -import { join } from 'path'; +import { join } from 'path'; import { MarkdownParser } from './parsers/markdown-parser.js'; import { resolveOpenSpecDir } from './path-resolver.js'; import { FileSystemUtils } from '../utils/file-system.js';Then update
listSpecsto use the asyncfs.readFile:- const content = readFileSync(specPath, 'utf-8'); + const content = await fs.readFile(specPath, 'utf-8');
113-122: Sync file read blocks the event loop in async function.
readFileSyncis a blocking call used inside an async function. For consistency with the rest of the file and to avoid blocking, use the asyncfs.readFileinstead.Use async file read
for (const id of specDirs) { const specPath = join(specsDir, id, 'spec.md'); try { - const content = readFileSync(specPath, 'utf-8'); + const content = await fs.readFile(specPath, 'utf-8'); const parser = new MarkdownParser(content); const spec = parser.parseSpec(id); specs.push({ id, requirementCount: spec.requirements.length }); } catch { // If spec cannot be read or parsed, include with 0 count specs.push({ id, requirementCount: 0 }); } }src/core/spec-logic.ts (1)
32-52: Return typeanyloses type safety.The function returns
Promise<any>but constructs a well-defined object. Consider defining an interface for the return type to improve type safety and IDE support.Add return type interface
+export interface SpecJsonOutput { + id: string; + title: string; + overview: string; + requirementCount: number; + requirements: Array<{ text: string; scenarios: any[] }>; + metadata: { version: string; format: string }; +} + -export async function getSpecJson(projectRoot: string, specId: string, options: ShowOptions = {}): Promise<any> { +export async function getSpecJson(projectRoot: string, specId: string, options: ShowOptions = {}): Promise<SpecJsonOutput> {src/core/change-logic.ts (1)
184-199: Minor: Prefertest()overmatch()for boolean checks.Using
RegExp.test()is slightly more efficient thanString.match()when you only need to check for a match, not capture groups.Use test() for boolean matching
export function countTasks(content: string): { total: number; completed: number } { const lines = content.split('\n'); let total = 0; let completed = 0; for (const line of lines) { - if (line.match(TASK_PATTERN)) { + if (TASK_PATTERN.test(line)) { total++; - if (line.match(COMPLETED_TASK_PATTERN)) { + if (COMPLETED_TASK_PATTERN.test(line)) { completed++; } } } return { total, completed }; }src/core/artifact-logic.ts (2)
56-58: Mixing sync and async fs operations blocks the event loop.The function is async but uses
fs.existsSyncandfs.statSyncinstead of their async counterparts. This is inconsistent with other files in this PR and can block the event loop.Use async fs operations
- const exists = fs.existsSync(changePath) && fs.statSync(changePath).isDirectory(); + let exists = false; + try { + const stat = await fs.promises.stat(changePath); + exists = stat.isDirectory(); + } catch { + exists = false; + }Or use
FileSystemUtils.directoryExists(changePath)which is already available and used in other core modules.
189-237: Sync recursive file system traversal may cause performance issues.The
artifactOutputExistsfunction uses synchronous operations (fs.existsSync,fs.statSync,fs.readdirSync) for potentially deep recursive directory traversal. In an MCP server context where this will be called frequently, this could block the event loop.Consider converting to async operations or caching results if this is called repeatedly for the same change.
openspec/changes/archive/2025-12-21-add-gemini-extension-support/proposal.md
Outdated
Show resolved
Hide resolved
| "chalk": "^5.5.0", | ||
| "commander": "^14.0.0", | ||
| "fast-glob": "^3.3.3", | ||
| "fastmcp": "^3.26.8", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
fastmcp npm package latest version
💡 Result:
The latest published version of the npm package fastmcp is 3.15.2. [1]
Install: npm i fastmcp. [1]
Sources:
[1] npm package page for fastmcp.
Update fastmcp dependency to an available version.
The specified version 3.26.8 does not exist on npm. The latest published version is 3.15.2. This will cause installation failures. Update the dependency to "^3.15.2" or verify the intended version number.
🤖 Prompt for AI Agents
In @package.json at line 81, The package.json lists a nonexistent fastmcp
version "3.26.8"; update the fastmcp dependency entry to a published version
(e.g., change the version string to "^3.15.2") or confirm the intended release
and set the correct semver value for the "fastmcp" entry so installs no longer
fail.
- Migrate default data directory from openspec/ to .openspec/ (with backward compatibility). - Implement prototype pollution prevention for configuration keys. - Implement path traversal protection for MCP resource resolution. - Update documentation and Gemini extension manifest (v0.19.0). - Incorporate feedback from PR Fission-AI#488.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/core/config-logic.ts:
- Around line 75-91: unsetConfigValue currently mutates the original object
returned by getGlobalConfig via deleteNestedValue and may persist an invalid
state; change it to clone the config (same cloning approach used in
setConfigValue) into a new object, run deleteNestedValue against that clone,
then validate the clone with validateConfig and call saveGlobalConfig only with
the validated/clone data (or fall back to the original unmodified config if
validation fails) so the original config is never saved in a mutated/invalid
form.
In @src/mcp/resources.ts:
- Around line 29-32: Replace the vulnerable prefix-only containment check that
uses startsWith(path.resolve(openspecPath)) with a strict containment check:
compute const resolvedDir = path.resolve(openspecPath) and const resolvedPath =
path.resolve(filePath) and then allow access only if resolvedPath ===
resolvedDir || resolvedPath.startsWith(resolvedDir + path.sep); update the shown
startsWith check and the two other identical checks (the other occurrences of
startsWith(path.resolve(openspecPath))) so they use this exact
equality-or-startsWith(dir + path.sep) logic.
- Around line 23-24: The load implementation in resources.ts is mismatched with
fastmcp's InputResourceTemplate: remove the `@ts-expect-error`, change the
parameter signature of load to the proper typed form (e.g., load: async (args: {
[name: string]: string } | { name: string }) => ...), preserve the existing
validation logic using args.name, read the file with fs.readFile(filePath,
'utf-8'), and return the expected shape Promise<{ text: string } | { blob:
string }> by returning { text } (not wrapped in a content array); ensure the
function signature and return type align with fastmcp so TypeScript errors are
resolved.
In @src/mcp/tools.ts:
- Line 220: The schema defines concurrency as z.string() but it should be
numeric; update the schema entry for concurrency to use a numeric Zod type
(e.g., z.number()) or a numeric-coercing type if inputs are strings, and ensure
any default/optional handling matches how runBulkValidation consumes it; change
the symbol named concurrency in the relevant zod schema and validate/convert it
before passing into runBulkValidation so the function receives a number.
- Around line 196-200: The code constructs changeDir using args.name without
sanitization, allowing path traversal; import and call the same
assertSafePathSegment (or a shared exported equivalent) before using args.name
in path.join (e.g., validate args.name at the start of the handler), then use
the validated segment to build changeDir and proceed with
Validator.validateChangeDeltaSpecs; ensure you add the import for
assertSafePathSegment and replace direct use of args.name with the validated
value (or throw/report an error if validation fails).
🧹 Nitpick comments (8)
src/core/config-schema.ts (1)
216-234: LGTM! The enhanced return type improves API ergonomics.Returning the parsed data avoids redundant re-parsing by callers. One minor note: line 228 has a redundant cast since
error instanceof z.ZodErroralready narrows the type.🔧 Optional cleanup for redundant cast
} catch (error) { if (error instanceof z.ZodError) { - const zodError = error as z.ZodError; - const messages = zodError.issues.map((e) => `${e.path.join('.')}: ${e.message}`); + const messages = error.issues.map((e) => `${e.path.join('.')}: ${e.message}`); return { success: false, error: messages.join('; ') }; }src/core/validation-logic.ts (2)
71-104: Custom concurrency limiter is correct but consider adding timeout protection.The worker pool implementation correctly manages concurrency, handles errors gracefully, and recovers item identity on failures. However, individual validation tasks have no timeout, which could cause the entire batch to hang if a single validation stalls.
💡 Consider wrapping tasks with a timeout
// Helper to add timeout to a promise function withTimeout<T>(promise: Promise<T>, ms: number, id: string): Promise<T> { return Promise.race([ promise, new Promise<T>((_, reject) => setTimeout(() => reject(new Error(`Validation timed out for ${id}`)), ms) ) ]); } // Usage in the queue task: // const report = await withTimeout(validator.validateChangeDeltaSpecs(changeDir), 30000, id);
145-151: Minor: Redundant bounds check.The
specIndex >= 0check on line 149 is always true when this branch executes, sincespecIndex = index - totalChangesand we only reach this line whenindex >= totalChanges.🔧 Simplified bounds check
function getPlannedType(index: number, changeIds: string[], specIds: string[]): ItemType | undefined { const totalChanges = changeIds.length; if (index < totalChanges) return 'change'; const specIndex = index - totalChanges; - if (specIndex >= 0 && specIndex < specIds.length) return 'spec'; + if (specIndex < specIds.length) return 'spec'; return undefined; }src/core/archive-logic.ts (2)
48-56: Consider preserving the original error for debugging.The catch block at line 54 discards the original error, which could contain useful diagnostic information (e.g., permission denied vs. not found).
♻️ Suggested improvement
try { const stat = await fs.stat(changeDir); if (!stat.isDirectory()) { throw new Error(`Change '${changeName}' not found.`); } - } catch { - throw new Error(`Change '${changeName}' not found.`); + } catch (err) { + const detail = err instanceof Error && err.code !== 'ENOENT' ? `: ${err.message}` : ''; + throw new Error(`Change '${changeName}' not found${detail}`); }
101-109: Inconsistent error handling for validation failures.When
throwOnValidationErroris true, aValidationErroris thrown (line 106). However, when it's false, a genericErroris thrown (line 107). This inconsistency may confuse callers who expect uniform error types. Consider throwingValidationErrorin both cases or documenting this behavioral difference.♻️ Proposed fix for consistent error handling
if (!report.valid) { - if (options.throwOnValidationError) throw new ValidationError(report); - throw new Error(`Validation failed for rebuilt spec: ${specName}`); + throw new ValidationError(report); }src/core/artifact-logic.ts (1)
191-239: Consider performance for large directory trees.The
hasMatchingFilesrecursive helper uses synchronousfs.readdirSyncwhich could block the event loop for large directory structures, especially when glob patterns like**are used.For now this is acceptable if the expected directory sizes are small, but consider async iteration if performance becomes an issue.
src/mcp/resources.ts (1)
6-15: Path validation may be overly restrictive for legitimate names.The check
value.includes('..')on line 11 will reject legitimate path segments likefoo..barortest..change. Consider checking specifically for../and..\patterns, or use a more targeted regex.However, this is a defense-in-depth measure and the final
startsWithcheck provides additional protection, so this is a minor concern.Suggested refinement
function assertSafePathSegment(value: unknown, label: string): string { if (typeof value !== 'string' || value.length === 0) { throw new Error(`Invalid ${label}`); } - // Disallow traversal and path separators (both posix + win) - if (value === '.' || value === '..' || value.includes('..') || /[\\/]/u.test(value)) { + // Disallow traversal sequences and path separators (both posix + win) + if (value === '.' || value === '..' || /\.\.[\\/]|[\\/]\.\.|[\\/]/u.test(value)) { throw new Error(`Invalid ${label}`); } return value; }src/core/update-logic.ts (1)
55-78: Add a clarifying comment for the dual-path AGENTS.md behavior.The condition on line 56 ensures
AGENTS.mdis always configured, allowing the code to support both:
- Internal
.openspec/AGENTS.md(lines 35-43) — full template replacement- Root-level
AGENTS.md(lines 55-78) — managed updates via markersThis is intentional: the root-level file is created/updated by
AgentsStandardConfiguratoreven if it doesn't exist. Consider adding an inline comment clarifying this dual-path design to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
GEMINI.mdREADME.mdgemini-extension.jsonopenspec/changes/archive/2025-12-21-add-gemini-extension-support/proposal.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/.openspec.yamlopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/global-config/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.mdopenspec/specs/global-config/spec.mdopenspec/specs/mcp-server/spec.mdsrc/commands/update.tssrc/commands/validate.tssrc/core/archive-logic.tssrc/core/artifact-logic.tssrc/core/config-logic.tssrc/core/config-schema.tssrc/core/update-logic.tssrc/core/validation-logic.tssrc/mcp/resources.tssrc/mcp/tools.tssrc/utils/item-discovery.ts
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/archive/2026-01-13-fix-pr-488-feedback/.openspec.yaml
- openspec/specs/mcp-server/spec.md
🚧 Files skipped from review as they are similar to previous changes (3)
- openspec/changes/archive/2025-12-21-add-gemini-extension-support/proposal.md
- README.md
- gemini-extension.json
🧰 Additional context used
📓 Path-based instructions (3)
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/global-config/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.md
openspec/changes/**/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/spec.md: Use## ADDED|MODIFIED|REMOVED|RENAMED Requirementsheaders in spec delta files
Include at least one#### Scenario:per requirement in spec delta files
Use#### Scenario: Nameformat (4 hashtags) for scenario headers, not bullets or bold text
Use## ADDED Requirementsfor new orthogonal capabilities that can stand alone; use## MODIFIED Requirementsfor behavior changes of existing requirements
When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/global-config/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.md
openspec/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Files:
openspec/specs/global-config/spec.md
🧠 Learnings (13)
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED Requirements` for new orthogonal capabilities that can stand alone; use `## MODIFIED Requirements` for behavior changes of existing requirements
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/global-config/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.mdopenspec/specs/global-config/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/proposal.md : Ensure `proposal.md` includes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/global-config/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.mdopenspec/specs/global-config/spec.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.mdopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.mdGEMINI.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.mdopenspec/specs/global-config/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` headers in spec delta files
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.mdGEMINI.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/specs/**/spec.md : Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Use `openspec archive <change-id> --skip-specs --yes` for tooling-only changes
Applied to files:
src/core/archive-logic.tssrc/utils/item-discovery.tssrc/core/artifact-logic.ts
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
src/utils/item-discovery.tssrc/commands/validate.tsopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.mdsrc/core/update-logic.tssrc/core/artifact-logic.tsopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.mdGEMINI.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Run `openspec validate [change-id] --strict` before requesting approval
Applied to files:
src/commands/validate.tssrc/core/artifact-logic.tssrc/core/validation-logic.ts
📚 Learning: 2025-11-25T01:08:02.839Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:02.839Z
Learning: Use `@/openspec/AGENTS.md` to learn how to create and apply change proposals, spec format and conventions, and project structure and guidelines
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.mdsrc/core/update-logic.tssrc/core/artifact-logic.tsopenspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.mdGEMINI.md
📚 Learning: 2025-11-25T01:08:02.839Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:02.839Z
Learning: Always open `@/openspec/AGENTS.md` when the request mentions planning or proposals (words like proposal, spec, change, plan), introduces new capabilities, breaking changes, architecture shifts, or performance/security work, or sounds ambiguous and needs the authoritative spec before coding
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.mdsrc/core/update-logic.tssrc/core/artifact-logic.tsGEMINI.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Check `openspec/project.md` for project conventions before creating specs
Applied to files:
src/core/artifact-logic.tsGEMINI.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/tasks.md : Ensure `tasks.md` contains implementation checklist with numbered sections and checkbox items
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Create `design.md` only when needed: cross-cutting changes, new external dependencies, significant data model changes, security/performance complexity, or pre-coding ambiguity
Applied to files:
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.mdGEMINI.md
🧬 Code graph analysis (8)
src/commands/update.ts (3)
src/core/update-logic.ts (1)
runUpdate(18-111)scripts/pack-version-check.mjs (1)
file(33-33)src/utils/file-system.ts (1)
FileSystemUtils(44-256)
src/mcp/tools.ts (12)
src/core/init-logic.ts (1)
runInit(30-118)src/core/update-logic.ts (1)
runUpdate(18-111)src/core/view-logic.ts (1)
getViewData(16-30)src/core/change-logic.ts (3)
runCreateChange(37-70)getChangeMarkdown(94-102)getChangeJson(104-128)src/core/list.ts (2)
listChanges(55-98)listSpecs(100-127)src/commands/spec.ts (2)
getSpecMarkdown(16-18)getSpecJson(20-22)src/core/spec-logic.ts (2)
getSpecMarkdown(23-30)getSpecJson(32-52)src/core/path-resolver.ts (1)
resolveOpenSpecDir(11-19)src/core/validation-logic.ts (1)
runBulkValidation(30-121)src/core/archive-logic.ts (1)
runArchive(31-145)src/core/config-logic.ts (3)
getConfigValue(35-39)setConfigValue(41-73)getConfigList(31-33)src/core/artifact-logic.ts (3)
getArtifactStatus(91-104)getArtifactInstructions(110-141)getApplyInstructions(241-329)
src/mcp/resources.ts (1)
src/core/path-resolver.ts (1)
resolveOpenSpecDir(11-19)
src/utils/item-discovery.ts (1)
src/core/path-resolver.ts (1)
resolveOpenSpecDir(11-19)
src/commands/validate.ts (1)
src/core/path-resolver.ts (1)
resolveOpenSpecDir(11-19)
src/core/config-logic.ts (1)
src/core/config-schema.ts (7)
getNestedValue(64-79)validateConfigKeyPath(31-55)coerceValue(142-162)setNestedValue(89-103)validateConfig(218-234)deleteNestedValue(112-130)DEFAULT_CONFIG(21-23)
src/core/update-logic.ts (5)
src/core/path-resolver.ts (1)
resolveOpenSpecDir(11-19)src/utils/file-system.ts (2)
FileSystemUtils(44-256)fileExists(88-98)src/core/templates/agents-template.ts (1)
agentsTemplate(1-465)src/core/configurators/registry.ts (1)
ToolRegistry(11-49)src/core/configurators/slash/registry.ts (1)
SlashCommandRegistry(24-84)
src/core/artifact-logic.ts (1)
src/core/path-resolver.ts (1)
resolveOpenSpecDir(11-19)
🪛 markdownlint-cli2 (0.18.1)
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.md
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.md
4-4: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (40)
src/utils/item-discovery.ts (4)
3-3: LGTM!Correct import for the centralized path resolver that supports both legacy (
openspec/) and new (.openspec/) directory structures.
5-25: LGTM!The function correctly awaits
resolveOpenSpecDirand constructs the changes path from the resolved base. The existing error handling is preserved, returning an empty array when the directory is inaccessible.
27-47: LGTM!Consistent refactoring pattern applied - correctly resolves the OpenSpec directory and appends the
specssubdirectory.
49-69: LGTM!Correctly resolves the base OpenSpec directory and constructs the archive path. The error handling appropriately returns an empty array when the archive directory doesn't exist.
GEMINI.md (1)
1-141: Comprehensive documentation for Gemini CLI integration.The document clearly explains MCP capabilities, zero-install workflow, and agent mandates. The auto-initialization sequence (lines 43-59) and slash command synchronization instructions (lines 100-141) provide actionable guidance for both agents and maintainers.
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/tasks.md (1)
1-26: Task list follows the expected structure.The implementation checklist correctly identifies security hardening tasks (path traversal, prototype pollution) and core logic refactoring items aligned with the PR objectives. Based on learnings, the file contains the required numbered sections and checkbox items format.
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/proposal.md (1)
1-25: Proposal follows the standard format with all required sections.The document correctly includes Why (1 sentence), What Changes (categorized bullet list), and Impact sections. Security hardening items (path traversal, prototype pollution) are appropriately marked as high-severity mitigations. Based on learnings, this meets the
proposal.mdstructure requirements.src/core/validation-logic.ts (1)
30-43: Well-designed bulk validation API with flexible configuration.The function signature cleanly separates scope selection from execution options. The fallback chain for concurrency (opts → env → default) provides good flexibility.
src/commands/validate.ts (4)
7-8: LGTM!The new imports for
chalkandresolveOpenSpecDirare appropriate for the added terminal styling and centralized path resolution.
132-151: LGTM!The
validateByTypemethod now correctly usesresolveOpenSpecDirto dynamically resolve the OpenSpec directory path before constructing change/spec file paths. This aligns with the PR's migration to centralized path handling with legacy directory support.
161-168: LGTM!The "Next steps" guidance for valid changes is a helpful UX improvement. The chalk styling is consistent and provides clear, actionable next steps for users.
195-226: LGTM!The bulk validation path resolution is correctly updated to use
resolveOpenSpecDir. The async/await pattern is properly applied, and the path construction for both changes and specs directories is consistent with the per-item validation approach.src/core/archive-logic.ts (3)
1-12: LGTM!The imports are well-organized, covering all necessary dependencies for archive orchestration including path resolution, validation, spec updates, and file system utilities.
14-29: LGTM!The
ArchiveResultinterface provides a comprehensive return structure, andValidationErrorproperly extendsErrorwith the validation report payload for structured error handling.
118-145: LGTM!The archive naming convention (date-prefixed) and the idempotency check for existing archives are well-implemented. The final result structure properly includes all relevant metadata.
src/core/artifact-logic.ts (5)
1-19: LGTM!The imports are comprehensive and the
DEFAULT_SCHEMAconstant provides a clear default for schema-related operations.
25-74: LGTM with minor note.The validation logic is thorough with helpful error messages listing available changes. The mix of sync (
fs.existsSync,fs.statSync) and async (fs.promises.readdir) operations at lines 59 and 34 works but is slightly inconsistent. This is minor and doesn't affect correctness.
169-189: LGTM!The task file parser correctly handles markdown checkbox syntax with both checked (
[x]/[X]) and unchecked ([ ]) states.
241-329: LGTM!The
getApplyInstructionsfunction provides a well-structured state machine with clear states (blocked,ready,all_done) and appropriate instructions for each. The context file collection and task progress tracking are thorough.
341-372: LGTM!The template path resolution correctly distinguishes between user-overridden and package templates, and the schema enumeration provides useful metadata.
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/global-config/spec.md (1)
1-9: LGTM!The spec delta correctly uses:
## ADDED Requirementsheader for a new standalone capabilitySHALLfor normative requirements#### Scenario:format with at least one scenario per requirementThe prototype pollution prevention requirement is well-specified with clear preconditions and postconditions. As per coding guidelines, this follows the expected format.
openspec/changes/archive/2026-01-13-fix-pr-488-feedback/specs/mcp-server/spec.md (1)
1-9: LGTM!The spec delta properly documents the path traversal prevention security requirement for MCP resource resolution. The format follows coding guidelines with correct use of
## ADDED Requirements,SHALLnormative language, and the#### Scenario:format.openspec/specs/global-config/spec.md (1)
102-109: LGTM!The security requirement for prototype pollution prevention has been properly integrated into the main spec. The format is consistent with existing requirements, uses appropriate normative language (
SHALL), and includes a well-defined scenario. As per coding guidelines, the#### Scenario:format is correctly used.src/commands/update.ts (4)
1-4: LGTM!The imports are appropriate for the command's responsibilities: path handling, file utilities, and the core update logic.
11-19: LGTM!The error logging correctly distinguishes between slash command failures (prefixed with
slash:) and regular file failures, providing clear context for each.
35-59: LGTM!The summary building logic is well-structured:
- Properly filters AI tool files excluding AGENTS.md variants
- Normalizes slash command paths for cross-platform consistency
- Aggregates failures into a clear list
The final summary output provides comprehensive feedback to users.
21-33: No changes needed. The dual AGENTS.md tracking logic is intentional and correct.The repository maintains two AGENTS.md files with distinct purposes:
openspec/AGENTS.md: The authoritative specification fileAGENTS.md(root): A generated file synced from the openspec version, marked with<!-- OPENSPEC:START -->and<!-- OPENSPEC:END -->for the 'openspec update' commandTracking both files in the summary output is appropriate since both are updated by the command and serve different roles in the project.
src/mcp/resources.ts (1)
17-86: LGTM on the overall structure and security approach.The resource registration follows a consistent pattern with good security practices:
- Input validation via
assertSafePathSegment- Defense-in-depth with final path containment check
- Clear error messages
- Proper async handling
The layered security approach (input validation + path containment) is commendable.
src/mcp/tools.ts (5)
1-16: LGTM on imports and module structure.Clean imports with good organization. The module exposes a single
registerToolsfunction that encapsulates all tool registrations, following a clean public API pattern.
17-98: LGTM on initialization and change management tools.The
openspec_init,openspec_update,openspec_view, andopenspec_create_changetools follow consistent patterns with proper error handling and delegation to core logic.
100-186: LGTM on listing and show tools.The
openspec_list_changes,openspec_list_specs,openspec_show_change, andopenspec_show_spectools are well-structured with format options and proper error handling.
264-327: LGTM on config tools.The configuration tools (
openspec_config_get,openspec_config_set,openspec_config_list) properly delegate to core config logic which includes prototype-pollution protection.
329-414: LGTM on artifact workflow tools.The artifact tools (
openspec_artifact_status,openspec_artifact_instructions,openspec_apply_instructions,openspec_list_schemas) are well-structured and delegate appropriately to core logic.src/core/update-logic.ts (3)
8-16: LGTM on UpdateResult interface.The interface comprehensively captures update outcomes with proper categorization of updated, created, and failed files, plus detailed error information.
18-43: LGTM on initialization and AGENTS.md handling.Proper path resolution, existence check with clear error message, and internal AGENTS.md update with error capture.
85-100: LGTM on slash command configurator handling.Good pattern for iterating slash configurators with availability check, error capture per tool, and proper namespacing in errorDetails (
slash:${toolId}).src/core/config-logic.ts (4)
17-25: LGTM on prototype-pollution prevention.The
FORBIDDEN_PATH_KEYSset andassertSafeConfigKeyPathfunction provide effective protection against prototype-pollution attacks via configuration keys.
35-39: LGTM ongetConfigValue.Properly validates the key path before retrieving the value.
41-73: LGTM onsetConfigValue.Good implementation with:
- Key path validation
- Deep clone before modification
- Schema validation before save
- Proper error handling
93-100: LGTM onresetConfig.The explicit
allflag requirement is a good safety measure to prevent accidental resets. SpreadingDEFAULT_CONFIGcreates a new object for saving.
| export function unsetConfigValue(key: string): boolean { | ||
| assertSafeConfigKeyPath(key); | ||
| const config = getGlobalConfig() as Record<string, unknown>; | ||
| const existed = deleteNestedValue(config, key); | ||
|
|
||
| if (existed) { | ||
| // Validate after deletion to ensure schema defaults are applied if necessary | ||
| const validation = validateConfig(config); | ||
| if (validation.success && validation.data) { | ||
| saveGlobalConfig(validation.data as GlobalConfig); | ||
| } else { | ||
| saveGlobalConfig(config as GlobalConfig); | ||
| } | ||
| } | ||
|
|
||
| return existed; | ||
| } |
There was a problem hiding this comment.
Bug: unsetConfigValue mutates the original config and may save invalid state.
Unlike setConfigValue which clones the config before modifying (line 58), unsetConfigValue modifies the original config object directly via deleteNestedValue. If validation fails on line 82, the already-mutated config is saved on line 86, bypassing validation.
This could lead to saving invalid configuration state.
Proposed fix
export function unsetConfigValue(key: string): boolean {
assertSafeConfigKeyPath(key);
- const config = getGlobalConfig() as Record<string, unknown>;
- const existed = deleteNestedValue(config, key);
+ const config = getGlobalConfig();
+ const newConfig = JSON.parse(JSON.stringify(config)) as Record<string, unknown>;
+ const existed = deleteNestedValue(newConfig, key);
if (existed) {
// Validate after deletion to ensure schema defaults are applied if necessary
- const validation = validateConfig(config);
+ const validation = validateConfig(newConfig);
if (validation.success && validation.data) {
saveGlobalConfig(validation.data as GlobalConfig);
} else {
- saveGlobalConfig(config as GlobalConfig);
+ // Validation failed - throw instead of saving invalid config
+ throw new Error(`Invalid configuration after deletion - ${validation.error || 'Unknown error'}`);
}
}
return existed;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function unsetConfigValue(key: string): boolean { | |
| assertSafeConfigKeyPath(key); | |
| const config = getGlobalConfig() as Record<string, unknown>; | |
| const existed = deleteNestedValue(config, key); | |
| if (existed) { | |
| // Validate after deletion to ensure schema defaults are applied if necessary | |
| const validation = validateConfig(config); | |
| if (validation.success && validation.data) { | |
| saveGlobalConfig(validation.data as GlobalConfig); | |
| } else { | |
| saveGlobalConfig(config as GlobalConfig); | |
| } | |
| } | |
| return existed; | |
| } | |
| export function unsetConfigValue(key: string): boolean { | |
| assertSafeConfigKeyPath(key); | |
| const config = getGlobalConfig(); | |
| const newConfig = JSON.parse(JSON.stringify(config)) as Record<string, unknown>; | |
| const existed = deleteNestedValue(newConfig, key); | |
| if (existed) { | |
| // Validate after deletion to ensure schema defaults are applied if necessary | |
| const validation = validateConfig(newConfig); | |
| if (validation.success && validation.data) { | |
| saveGlobalConfig(validation.data as GlobalConfig); | |
| } else { | |
| // Validation failed - throw instead of saving invalid config | |
| throw new Error(`Invalid configuration after deletion - ${validation.error || 'Unknown error'}`); | |
| } | |
| } | |
| return existed; | |
| } |
🤖 Prompt for AI Agents
In @src/core/config-logic.ts around lines 75 - 91, unsetConfigValue currently
mutates the original object returned by getGlobalConfig via deleteNestedValue
and may persist an invalid state; change it to clone the config (same cloning
approach used in setConfigValue) into a new object, run deleteNestedValue
against that clone, then validate the clone with validateConfig and call
saveGlobalConfig only with the validated/clone data (or fall back to the
original unmodified config if validation fails) so the original config is never
saved in a mutated/invalid form.
| // @ts-expect-error - variables type mismatch in fastmcp | ||
| load: async (variables: any) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
fastmcp addResourceTemplate variables type signature
💡 Result:
Signature (TypeScript, concise)
server.addResourceTemplate(input: InputResourceTemplate)
InputResourceTemplate {
uriTemplate: string;
name: string;
description?: string;
mimeType?: string;
arguments?: ReadonlyArray<{
name: string;
description?: string;
complete?: ArgumentValueCompleter;
}>;
load: (args: { [K in Arguments[number]["name"]]: string }) => Promise;
}
ArgumentValueCompleter = (value: string) => Promise;
ResourceResult = { text: string } | { blob: string }.
Notes: URI template parameter names must match function argument names; template arguments are passed as strings. [1][2]
Sources:
- FastMCP source/docs (InputResourceTemplate / ResourceTemplate types & load result). [1]
- FastMCP docs: Resource templates overview and example usage. [2]
🏁 Script executed:
cat -n src/mcp/resources.ts | head -40Repository: Fission-AI/OpenSpec
Length of output: 2027
Fix the return type and parameter type mismatch with the fastmcp API.
The @ts-expect-error is suppressing a genuine API incompatibility. According to fastmcp's InputResourceTemplate signature, the load function should accept (args: { [name: string]: string }) and return Promise<{ text: string } | { blob: string }>. However, this code:
- Uses
variables: anyinstead of properly typed parameters - Returns
{ content: [{ uri: ..., text }] }which doesn't match the expected return type
The correct implementation should be:
load: async (args: { name: string }) => {
// ... existing validation logic ...
const text = await fs.readFile(filePath, 'utf-8');
return { text }; // Return text directly, not wrapped in content array
}🤖 Prompt for AI Agents
In @src/mcp/resources.ts around lines 23 - 24, The load implementation in
resources.ts is mismatched with fastmcp's InputResourceTemplate: remove the
`@ts-expect-error`, change the parameter signature of load to the proper typed
form (e.g., load: async (args: { [name: string]: string } | { name: string }) =>
...), preserve the existing validation logic using args.name, read the file with
fs.readFile(filePath, 'utf-8'), and return the expected shape Promise<{ text:
string } | { blob: string }> by returning { text } (not wrapped in a content
array); ensure the function signature and return type align with fastmcp so
TypeScript errors are resolved.
| // Final safety check: ensure resolved path is within openspecPath | ||
| if (!path.resolve(filePath).startsWith(path.resolve(openspecPath))) { | ||
| throw new Error("Unauthorized path access"); | ||
| } |
There was a problem hiding this comment.
Path containment check vulnerable to prefix-based bypass.
The startsWith() check can be bypassed if an attacker crafts a path that shares the same prefix. For example, if openspecPath is /project/.openspec, a path like /project/.openspec-malicious/file would pass this check.
Use path.resolve(openspecPath) + path.sep as the prefix to ensure the resolved path is strictly within the directory.
Proposed fix
// Final safety check: ensure resolved path is within openspecPath
- if (!path.resolve(filePath).startsWith(path.resolve(openspecPath))) {
+ const resolvedBase = path.resolve(openspecPath) + path.sep;
+ if (!path.resolve(filePath).startsWith(resolvedBase)) {
throw new Error("Unauthorized path access");
}Apply the same fix to lines 52-55 and 75-78.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Final safety check: ensure resolved path is within openspecPath | |
| if (!path.resolve(filePath).startsWith(path.resolve(openspecPath))) { | |
| throw new Error("Unauthorized path access"); | |
| } | |
| // Final safety check: ensure resolved path is within openspecPath | |
| const resolvedBase = path.resolve(openspecPath) + path.sep; | |
| if (!path.resolve(filePath).startsWith(resolvedBase)) { | |
| throw new Error("Unauthorized path access"); | |
| } |
🤖 Prompt for AI Agents
In @src/mcp/resources.ts around lines 29 - 32, Replace the vulnerable
prefix-only containment check that uses startsWith(path.resolve(openspecPath))
with a strict containment check: compute const resolvedDir =
path.resolve(openspecPath) and const resolvedPath = path.resolve(filePath) and
then allow access only if resolvedPath === resolvedDir ||
resolvedPath.startsWith(resolvedDir + path.sep); update the shown startsWith
check and the two other identical checks (the other occurrences of
startsWith(path.resolve(openspecPath))) so they use this exact
equality-or-startsWith(dir + path.sep) logic.
| try { | ||
| const openspecPath = await resolveOpenSpecDir(process.cwd()); | ||
| const changeDir = path.join(openspecPath, 'changes', args.name); | ||
| const validator = new Validator(args.strict); | ||
| const report = await validator.validateChangeDeltaSpecs(changeDir); |
There was a problem hiding this comment.
Potential path traversal vulnerability in change validation.
Unlike resources.ts, the args.name parameter is used directly in path construction without validation. A malicious input like ../../../etc could escape the intended directory.
Apply the same assertSafePathSegment validation used in resources.ts, or delegate to a core function that performs this validation.
Proposed fix
Import and use the validation function:
+import { assertSafePathSegment } from './resources.js';Then in the execute handler:
execute: async (args) => {
try {
const openspecPath = await resolveOpenSpecDir(process.cwd());
- const changeDir = path.join(openspecPath, 'changes', args.name);
+ const safeName = assertSafePathSegment(args.name, 'change name');
+ const changeDir = path.join(openspecPath, 'changes', safeName);
const validator = new Validator(args.strict);Alternatively, export assertSafePathSegment from a shared utility module to avoid cross-importing between MCP modules.
🤖 Prompt for AI Agents
In @src/mcp/tools.ts around lines 196 - 200, The code constructs changeDir using
args.name without sanitization, allowing path traversal; import and call the
same assertSafePathSegment (or a shared exported equivalent) before using
args.name in path.join (e.g., validate args.name at the start of the handler),
then use the validated segment to build changeDir and proceed with
Validator.validateChangeDeltaSpecs; ensure you add the import for
assertSafePathSegment and replace direct use of args.name with the validated
value (or throw/report an error if validation fails).
| changes: z.boolean().optional().default(true).describe("Validate changes"), | ||
| specs: z.boolean().optional().default(true).describe("Validate specs"), | ||
| strict: z.boolean().optional().default(false).describe("Enable strict validation"), | ||
| concurrency: z.string().optional().describe("Concurrency limit") |
There was a problem hiding this comment.
Type mismatch: concurrency should be a number, not a string.
The concurrency parameter is defined as z.string() but represents a numeric limit. Based on the relevant code snippet for runBulkValidation, it expects a concurrency value that would be used numerically.
Proposed fix
- concurrency: z.string().optional().describe("Concurrency limit")
+ concurrency: z.number().int().positive().optional().describe("Concurrency limit")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| concurrency: z.string().optional().describe("Concurrency limit") | |
| concurrency: z.number().int().positive().optional().describe("Concurrency limit") |
🤖 Prompt for AI Agents
In @src/mcp/tools.ts at line 220, The schema defines concurrency as z.string()
but it should be numeric; update the schema entry for concurrency to use a
numeric Zod type (e.g., z.number()) or a numeric-coercing type if inputs are
strings, and ensure any default/optional handling matches how runBulkValidation
consumes it; change the symbol named concurrency in the relevant zod schema and
validate/convert it before passing into runBulkValidation so the function
receives a number.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @openspec/specs/mcp-server/spec.md:
- Around line 3-4: Replace the "TBD" under the "## Purpose" heading with a
concise description that states the purpose and scope of the MCP server
specification: explain that this document defines the goals, responsibilities,
and intended audience of the MCP server (e.g., what problems it solves), list
the main functional areas covered (APIs, protocols, security, deployment
assumptions, and interoperability expectations), and clarify any boundaries or
excluded concerns so readers of the spec understand its remit and usage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gemini-extension.jsonopenspec/changes/archive/2026-01-13-direct-mcp-entry/.openspec.yamlopenspec/changes/archive/2026-01-13-direct-mcp-entry/proposal.mdopenspec/changes/archive/2026-01-13-direct-mcp-entry/specs/mcp-server/spec.mdopenspec/changes/archive/2026-01-13-direct-mcp-entry/tasks.mdopenspec/specs/mcp-server/spec.md
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/archive/2026-01-13-direct-mcp-entry/.openspec.yaml
- openspec/changes/archive/2026-01-13-direct-mcp-entry/tasks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- gemini-extension.json
🧰 Additional context used
📓 Path-based instructions (3)
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/archive/2026-01-13-direct-mcp-entry/specs/mcp-server/spec.mdopenspec/changes/archive/2026-01-13-direct-mcp-entry/proposal.md
openspec/changes/**/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/spec.md: Use## ADDED|MODIFIED|REMOVED|RENAMED Requirementsheaders in spec delta files
Include at least one#### Scenario:per requirement in spec delta files
Use#### Scenario: Nameformat (4 hashtags) for scenario headers, not bullets or bold text
Use## ADDED Requirementsfor new orthogonal capabilities that can stand alone; use## MODIFIED Requirementsfor behavior changes of existing requirements
When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Files:
openspec/changes/archive/2026-01-13-direct-mcp-entry/specs/mcp-server/spec.md
openspec/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Files:
openspec/specs/mcp-server/spec.md
🧠 Learnings (4)
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED Requirements` for new orthogonal capabilities that can stand alone; use `## MODIFIED Requirements` for behavior changes of existing requirements
Applied to files:
openspec/changes/archive/2026-01-13-direct-mcp-entry/specs/mcp-server/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/proposal.md : Ensure `proposal.md` includes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Applied to files:
openspec/changes/archive/2026-01-13-direct-mcp-entry/proposal.mdopenspec/specs/mcp-server/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
openspec/changes/archive/2026-01-13-direct-mcp-entry/proposal.mdopenspec/specs/mcp-server/spec.md
📚 Learning: 2025-11-25T01:08:02.839Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:02.839Z
Learning: Use `@/openspec/AGENTS.md` to learn how to create and apply change proposals, spec format and conventions, and project structure and guidelines
Applied to files:
openspec/changes/archive/2026-01-13-direct-mcp-entry/proposal.mdopenspec/specs/mcp-server/spec.md
🔇 Additional comments (2)
openspec/changes/archive/2026-01-13-direct-mcp-entry/specs/mcp-server/spec.md (1)
1-9: LGTM! Spec follows all coding guidelines.The specification correctly uses
## ADDED Requirementsfor this new orthogonal capability, includes a properly formatted scenario with the#### Scenario: Direct Invocationheader (4 hashtags), and provides clear, testable acceptance criteria using the WHEN/THEN/AND structure. The requirement is well-defined and aligns with the PR's objective of providing a dedicated MCP entry point.openspec/specs/mcp-server/spec.md (1)
5-110: Excellent use of normative language and requirement structure.The specification correctly uses SHALL/SHALL NOT for all normative requirements and appropriately uses MAY on Line 70 for an intentionally non-normative optional feature (CLI reference). The WHEN/THEN/AND scenario structure is consistent throughout and makes requirements clear and testable. The spec comprehensively covers functionality, testing, security, and architectural concerns.
| # Proposal: Direct MCP Entry Point | ||
|
|
||
| ## Context | ||
| Currently, the OpenSpec Gemini extension launches the MCP server via the CLI `serve` command (`bin/openspec.js serve`). This introduces unnecessary overhead and couples the server to the CLI's initialization logic. | ||
|
|
||
| ## Goal | ||
| Update the extension configuration to use a dedicated entry point for the MCP server (`dist/mcp/index.js`), bypassing the CLI wrapper. | ||
|
|
||
| ## Requirements | ||
| - Update `gemini-extension.json` to point to `dist/mcp/index.js` | ||
| - Ensure `src/mcp/index.ts` is compiled to `dist/mcp/index.js` (already covered by existing build) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Explore the proposal file and surrounding structure
ls -la openspec/changes/archive/2026-01-13-direct-mcp-entry/ 2>/dev/null || echo "Directory check"
cat -n openspec/changes/archive/2026-01-13-direct-mcp-entry/proposal.md 2>/dev/null || echo "File check"Repository: Fission-AI/OpenSpec
Length of output: 1082
🏁 Script executed:
# Check repository structure for build config files
fd -t f "(tsconfig|package\.json|webpack|vite|rollup)" --max-results 20Repository: Fission-AI/OpenSpec
Length of output: 122
🏁 Script executed:
# Search for references to dist/mcp or src/mcp in the codebase
rg -l "dist/mcp|src/mcp" --type ts --type js --type json | head -20Repository: Fission-AI/OpenSpec
Length of output: 183
🏁 Script executed:
# Check if src/mcp/index.ts exists
fd "src/mcp/index" --type fRepository: Fission-AI/OpenSpec
Length of output: 45
🏁 Script executed:
# Read tsconfig.json to check build config
cat -n tsconfig.json | head -50Repository: Fission-AI/OpenSpec
Length of output: 740
🏁 Script executed:
# Read package.json to check build script and files field
cat -n package.json | head -80Repository: Fission-AI/OpenSpec
Length of output: 2843
🏁 Script executed:
# Check gemini-extension.json current state
cat -n gemini-extension.jsonRepository: Fission-AI/OpenSpec
Length of output: 376
🏁 Script executed:
# Check build.js to see how it handles the build process
cat -n build.js | head -80Repository: Fission-AI/OpenSpec
Length of output: 1109
🏁 Script executed:
# Check the .openspec.yaml in the proposal directory for status
cat -n openspec/changes/archive/2026-01-13-direct-mcp-entry/.openspec.yamlRepository: Fission-AI/OpenSpec
Length of output: 116
🏁 Script executed:
# Verify if src/mcp/index.ts exists or will be created by the proposal
find . -path "*/src/mcp*" -type f -o -path "*/src/mcp*" -type d 2>/dev/null | head -10Repository: Fission-AI/OpenSpec
Length of output: 174
Restructure proposal to match required template (Why / What Changes / Impact).
The build guarantee is sound: tsconfig.json includes src/**/* and outputs to dist, build.js runs the TypeScript compiler, and package.json includes dist in its files field, so dist/mcp/index.js will be created and packaged. However, the proposal structure doesn't follow the required template. Replace Context/Goal/Requirements with Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code).
Also note: gemini-extension.json is already configured to invoke ${extensionPath}/dist/mcp/index.js, so clarify in the proposal whether this change is already implemented or describes planned modifications.
| ## Purpose | ||
| TBD - created by archiving change add-mcp-server. Update Purpose after archive. |
There was a problem hiding this comment.
Complete the Purpose section.
The Purpose section contains a TBD placeholder. Please update this section to clearly describe the purpose and scope of the MCP server specification.
🤖 Prompt for AI Agents
In @openspec/specs/mcp-server/spec.md around lines 3 - 4, Replace the "TBD"
under the "## Purpose" heading with a concise description that states the
purpose and scope of the MCP server specification: explain that this document
defines the goals, responsibilities, and intended audience of the MCP server
(e.g., what problems it solves), list the main functional areas covered (APIs,
protocols, security, deployment assumptions, and interoperability expectations),
and clarify any boundaries or excluded concerns so readers of the spec
understand its remit and usage.
Vibe Kanban Review FailedUnfortunately, the code review could not be completed. Review ID: |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
1 participant
This PR introduces comprehensive support for the Model Context Protocol (MCP) and Gemini Extensions. My goal was to make OpenSpec natively accessible to AI agents using an MCP interface so that one-off integrations wouldn't be necessary for each successive agent launched.
I preserved the logic by pulling it out into its own core, then both the cli and mcp features import the shared code.
Openspec can now be added without having to install the tool(although thats still an option):
{ "mcpServers": { "openspec": { "command": "npx", "args": ["-y", "@fission-ai/openspec@latest", "serve"] } } }Key Changes
A Note on PR Size:
My bad :-( My original attempt to introduce gemini extension support was too clunky. It looked like the best approach was to go with an mcp integration that would solve all the integration issues with any agent tool that supports mcp(most now). When I chose the mcp route, it meant refactoring and splitting the code up into core/cli/mcp. The transition to a decoupled logic-implementation model required touching most of our command handlers and core modules. I felt this one-time
"lifting" was necessary to establish a clean foundation for the next generation of OpenSpec.
Gemini Extension showing mcp running
Sample agent interaction with openspec
Summary by CodeRabbit
New Features
Documentation
CI / Scripts
Bug Fixes / Security
Tests
✏️ Tip: You can customize this high-level summary in your review settings.