feat(mcp): add MCP server for ralph-specum #75
Conversation
- 15 user stories covering installation, tools, logging - 12 functional requirements (P0-P2) - 7 non-functional requirements - MVP scope: 11 tools + MCP standard logging - npm/npx distribution option added Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add md.d.ts type declaration for markdown file imports
- TypeScript now recognizes *.md imports with Bun's { type: "text" } attribute
- Typecheck passes successfully
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Verified typecheck passes for all lib modules (logger.ts, state.ts, files.ts). No type errors found. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Typecheck verification passed - all direct tools compile correctly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create lib/errors.ts with standardized error handling utilities - Define RalphErrorCode type with 7 error categories - Implement createErrorResponse for consistent error formatting - Implement handleUnexpectedError to safely catch exceptions (no stack traces) - Add ErrorMessages object with reusable error message templates - Update all 11 tool handlers with try/catch wrapping - Add MCPLogger parameter to all handlers for error logging - Update registerTools to pass logger to all handlers - All error scenarios return structured, helpful messages Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create centralized types module (lib/types.ts) with all shared TypeScript types - Export types for external use: TextContent, ToolResult, Phase, RalphState, etc. - Remove duplicate type definitions from errors.ts and instruction-builder.ts - Add comprehensive JSDoc comments to all public functions and classes - Add @module, @param, @returns, and @example documentation tags - Create lib/index.ts barrel file for cleaner imports - Use type imports in all tool files for better separation - Extract MAX_NAME_LENGTH constant in start.ts - No TODOs remaining in TypeScript source files Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Verified typecheck + tests: - bun run typecheck: PASS (no type errors) - bun test: PASS (62 tests, 0 failures) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Verification results: - typecheck: PASS (no type errors) - tests: PASS (190 tests, 0 failed, 432 expect() calls) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add scripts/build.sh for cross-platform binary builds (darwin-arm64, darwin-x64, linux-x64, windows-x64) - Add scripts/install.sh for GitHub releases installation - Add build:all npm script Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Bun + TypeScript MCP server (Ralph Specum MCP) with 11 tools, asset-embedded agent prompts/templates, core libs for logging/state/files, CLI/stdio server entry, multi-platform build/release scripts, install helper, comprehensive tests, and extensive specs/templates for spec-driven workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant McpServer
participant ToolHandler
participant FileManager
participant StateManager
Client->>McpServer: Send tool call (stdio)
McpServer->>ToolHandler: validate input (Zod)
ToolHandler->>FileManager: read spec files (.progress.md, research.md, tasks.md...)
ToolHandler->>StateManager: read/update .ralph-state.json
Note over ToolHandler: assemble context, build instruction response
ToolHandler-->>McpServer: return ToolResult (content / isError)
McpServer-->>Client: write response (stdio)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
PR: #75 CI: All required checks passing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
All CI checks verified green - no fixes needed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR #75 has no code reviews or inline comments to address. Only automated coderabbitai bot comment present (not blocking). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
All completion criteria verified: - 190 tests passing (zero regressions) - CI green on PR #75 - Code follows MCP SDK patterns from design - POC validation completed in task 1.22 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@mcp-server/.npmrc`:
- Line 1: The committed .npmrc sets the default registry to
"registry=https://registry.npmmirror.com/", which can cause cross-region CI and
contributor issues; either delete the .npmrc from the repo or update its
contents to the official npm registry by replacing that line with
"registry=https://registry.npmjs.org/" so repo-wide installs/publishes use the
standard registry while allowing developers to configure mirrors locally.
In `@mcp-server/scripts/install.sh`:
- Around line 32-34: The install script downloads and installs "$ASSET" without
verifying integrity; add SHA256 checksum verification by downloading the
release's checksums file (e.g., checksums.txt for $LATEST), extract the expected
hash for "$ASSET"/"$BINARY_NAME", compute the downloaded file's SHA256 (for
"/tmp/$BINARY_NAME"), compare them and abort on mismatch before running
chmod/mv; ensure the script logs a clear error and exits non‑zero if
verification fails and only proceeds to chmod +x and sudo mv when the checksum
matches.
In `@mcp-server/src/assets/agents/spec-executor.md`:
- Around line 310-316: The current snippet uses GNU-only `sed -i` and the
`flock` CLI (`flock -x 200`) which breaks on macOS; replace the CLI locking and
in-place sed with a portable pattern: acquire an atomic lock by creating a lock
directory (e.g., mkdir "./specs/<spec>/.tasks.lockdir" in a retry loop, remove
it on exit), write the modified content to a temporary file (e.g., create temp
next to "./specs/<spec>/tasks.md"), run sed without -i to output to that temp,
then atomically mv the temp over "./specs/<spec>/tasks.md" and finally release
the lock directory; reference the symbols "./specs/<spec>/tasks.md",
".tasks.lock" (replace with ".tasks.lockdir"), `sed -i` and `flock -x 200` when
locating the code to change.
In `@mcp-server/src/lib/files.ts`:
- Around line 72-244: Inputs specName and fileName can perform path traversal
and escape the specs directory; update path handling so all paths are resolved
and validated against the specs root. In getSpecDir and getSpecFilePath (and
getCurrentSpecPath) call path.resolve(join(...)) to produce absolute paths and
ensure the resolved path starts with the resolved getSpecsDir() + path.sep;
similarly, before any filesystem operation in specExists, createSpecDir,
deleteSpec, readSpecFile, and writeSpecFile validate that the resolved spec
directory or file path is contained inside the specs root and reject/log and
return false/null on invalid inputs; additionally normalize/strip any leading
path separators or .. segments from specName/fileName (or explicitly disallow
path separators in specName) so callers cannot pass absolute or traversal paths.
In `@mcp-server/src/tools/start.ts`:
- Around line 18-22: The StartInputSchema currently allows any non-empty name
which can enable path traversal (e.g., "../foo"); update StartInputSchema to
reject path separators and enforce a safe pattern (e.g., kebab-case) by adding a
Zod regex or refinement on the name field to disallow "/" "\" and ".." sequences
and require /^[a-z0-9]+(?:-[a-z0-9]+)*$/; also apply the same validation to any
incoming spec_name parameters used by tools (references: StartInputSchema and
any handlers accepting spec_name) and add defensive checks in FileManager
functions (createSpecDir, getSpecDir, specExists, setCurrentSpec) to normalize
and reject unsafe names at the boundary. Ensure error messages are clear for
invalid names.
In `@mcp-server/tests/utils.ts`:
- Around line 78-389: The helpers break on Windows by using literal "/"
separators; update createMockStateFile to extract specName with
path.basename(specDir) instead of specDir.split("/").pop(), change
MockFileManager.listSpecs to compute relative paths with
path.relative(specsPath, d) (or use d.slice(specsPath.length + 1) only after
ensuring path.join semantics) and replace the startsWith(specsPath + "/") check
with path.isAbsolute comparisons or path.relative checks, and in
MockStateManager.read/write/delete/exists replace specDir.split("/").pop()! with
path.basename(specDir) so all path operations use Node's path utilities
(basename, relative, join, path.sep) rather than hardcoded slashes.
🟡 Minor comments (16)
mcp-server/scripts/install.sh-23-26 (1)
23-26: Fragile JSON parsing and missing error handling for release fetching.The
grep | cutapproach for parsing JSON is brittle and can break with formatting changes. Additionally, there's no validation thatLATESTwas successfully retrieved.Suggested improvements
# Get latest release -LATEST=$(curl -fsSL "https://api.github.com/repos/$REPO/releases/latest" | grep tag_name | cut -d'"' -f4) +LATEST=$(curl -fsSL "https://api.github.com/repos/$REPO/releases/latest" | grep -o '"tag_name": *"[^"]*"' | head -1 | cut -d'"' -f4) +if [[ -z "$LATEST" ]]; then + echo "Error: Could not determine latest release version" + exit 1 +fi ASSET="${BINARY_NAME}-${OS}-${ARCH}" [[ "$OS" == "windows" ]] && ASSET="${ASSET}.exe"If
jqcan be assumed available, prefer:LATEST=$(curl -fsSL "https://api.github.com/repos/$REPO/releases/latest" | jq -r '.tag_name')mcp-server/package.json-17-20 (1)
17-20: Confirm zod version strategy.Version 3.25.0 exists in npm. However, consider whether pinning to
^3.25.0is intentional: the current stable release is Zod 4.3.x, and using the 3.x constraint prevents upgrades to version 4. If the library documentation references Zod 4, align the version requirement to match the intended compatibility target.specs/mcp-server/.progress.md-192-194 (1)
192-194: Stale "Next" section references completed task.Line 109 indicates task 4.3 is complete ("Completed 4.3 - awaiting next task"), but the "Next" section still shows "Task 4.3: Local quality check" as pending. Update this section to reflect actual next steps or mark it as complete.
specs/mcp-server/research.md-550-552 (1)
550-552: Remove or relativize local absolute paths.These lines expose the author's local filesystem structure (
/Users/zachbonfil/projects/...). For a cleaner, privacy-respecting document, use relative paths from the repository root.Suggested fix
### Codebase Files -- `/Users/zachbonfil/projects/smart-ralph-mcp-server/plugins/ralph-specum/.claude-plugin/plugin.json` -- `/Users/zachbonfil/projects/smart-ralph-mcp-server/plugins/ralph-specum/commands/*.md` -- `/Users/zachbonfil/projects/smart-ralph-mcp-server/plugins/ralph-specum/agents/*.md` +- `plugins/ralph-specum/.claude-plugin/plugin.json` +- `plugins/ralph-specum/commands/*.md` +- `plugins/ralph-specum/agents/*.md`specs/mcp-server/.progress.md-286-286 (1)
286-286: Duplicate heading "Learnings" conflicts with line 111.This duplicate
## Learningsheading violates markdownlint MD024 (no-duplicate-heading) and may cause navigation issues in markdown viewers with heading-based TOC.Consider renaming to differentiate, e.g.,
## Implementation Learningsor merging with the earlier section.Suggested fix
-## Learnings +## Implementation Learningsspecs/mcp-server/requirements.md-224-226 (1)
224-226: Clarify tool count to avoid conflicting acceptance signals.FR‑5 lists “10 MCP tools” while Success Criteria mentions “10 core tools,” but the overall spec (and US‑12) includes 11 tools with
ralph_complete_phase. Please make the counts consistent so readers don’t interpretcomplete_phaseas out‑of‑scope.📝 Suggested wording update
- 3. **Feature parity**: All 10 core tools functional (excluding refactor) + 3. **Feature parity**: All 11 tools functional (including ralph_complete_phase; excluding refactor)Also applies to: 287-288
specs/mcp-server/requirements.md-108-114 (1)
108-114: Align workflow phases with the 4‑phase POC-first requirement.The glossary defines phases as research/requirements/design/tasks/implement, but the repo standard requires the 4‑phase POC-first workflow. Please explicitly map or incorporate the POC-first phases here (and in the tasks guidance) so the spec doesn’t conflict with the repo’s mandated flow.
📝 Suggested wording update
- - **Phase**: One stage of spec development (research, requirements, design, tasks, implement) + - **Phase**: One stage of spec development (research, requirements, design, tasks, implement). + Task execution must follow the 4‑phase POC-first workflow: + 1) Make It Work (POC, skip tests) + 2) Refactor + 3) Testing + 4) Quality GatesBased on learnings, ensure the 4‑phase POC-first workflow is explicitly reflected.
Also applies to: 247-253
mcp-server/src/tools/status.ts-21-32 (1)
21-32: Remove non-existent phase check; inconsistent progress display between implement and status tools.The phase enum only includes
["research", "requirements", "design", "tasks", "execution"]—there's no"implement"phase. The existing check for"execution"is correct.However, progress display is inconsistent:
implement.tsshows users"Task 1 of 10"(1-based, see line 132), butstatus.tsshows"0/10"(0-based). Consider aligning both to use 1-based display for consistency, though note this would require updating the existing tests instatus.test.tsthat explicitly expect the current 0-based format (lines 81-82, 103).mcp-server/src/assets/agents/task-planner.md-93-101 (1)
93-101: Add a language tag to this fenced block (MD040).Fix markdownlint MD040
-``` +```text Task tool with subagent_type: Explore thoroughness: mediummcp-server/src/tools/cancel.ts-83-89 (1)
83-89: Misleading warning message when state file doesn't exist.Based on the
StateManager.deleteimplementation (returnstrueif file doesn't exist), the warning at line 88 may never trigger. The logic correctly handles this case, but the warning text "may not exist" is confusing sincestateDeletedwould betruein that scenario.Looking at
StateManager.delete:
- Returns
trueif file deleted OR didn't exist- Returns
falseonly on actual deletion errorThe current code path for
stateDeleted === falseindicates a real error, so the warning message should reflect that.🔧 Suggested fix for clearer messaging
// Delete .ralph-state.json const stateDeleted = stateManager.delete(specDir); if (stateDeleted) { results.push("- Deleted .ralph-state.json"); } else { - results.push("- Warning: Failed to delete .ralph-state.json (may not exist)"); + results.push("- Warning: Failed to delete .ralph-state.json (permission or I/O error)"); }mcp-server/src/lib/state.ts-174-211 (1)
174-211: Potential temp file collision in concurrent writes.The temp file path
${statePath}.tmpis deterministic. If multiple processes or async operations attempt to write to the same spec simultaneously, they could overwrite each other's temp files before the rename, leading to data loss.🔧 Suggested fix using unique temp file names
write(specDir: string, state: RalphState): boolean { const statePath = this.getStatePath(specDir); - const tempPath = `${statePath}.tmp`; + const tempPath = `${statePath}.tmp.${Date.now()}.${Math.random().toString(36).slice(2)}`; try { // Ensure directory exists const dir = dirname(statePath); if (!existsSync(dir)) { mkdirSync(dir, { recursive: true }); } // Write to temp file first const content = JSON.stringify(state, null, 2); writeFileSync(tempPath, content, "utf-8"); // Atomic rename renameSync(tempPath, statePath); this.logger.debug("State written successfully", { path: statePath }); return true; } catch (error) {mcp-server/src/tools/cancel.ts-97-109 (1)
97-109: Current spec not cleared when last spec is deleted.When deleting the current spec and no other specs remain (line 106-108), the
.current-specfile still contains the deleted spec's name. This could cause issues if a user later creates a new spec with a different name—the stale.current-specwould point to a non-existent spec.🔧 Suggested fix to clear stale current spec
if (remainingSpecs.length > 0) { fileManager.setCurrentSpec(remainingSpecs[0]); results.push(`- Switched current spec to: ${remainingSpecs[0]}`); } else { - // No need to clear .current-spec as specs dir may be empty + // Clear stale .current-spec reference + fileManager.clearCurrentSpec?.() ?? fileManager.setCurrentSpec(""); results.push("- No remaining specs"); }Note: This assumes
FileManagerhas or could have aclearCurrentSpecmethod, or that setting an empty string is acceptable.specs/mcp-server/tasks.md-54-56 (1)
54-56: Hardcoded absolute paths reduce portability.The task specification contains absolute paths like
/Users/zachbonfil/projects/smart-ralph-mcp-server/...throughout the document. These paths are specific to the original author's machine and would not work for other contributors.Consider using relative paths (e.g.,
./mcp-server/package.json) or placeholder paths (e.g.,<repo-root>/mcp-server/package.json) for better portability.mcp-server/src/assets/agents/research-analyst.md-58-62 (1)
58-62: Add language identifiers to fenced code blocks.
Unlabeled fences at Line 58-62, Line 290-303, and Line 307-318 trigger MD040. Specifytextormarkdownto keep lint clean.Suggested update (apply similarly to the other two fences)
-``` +```text WebSearch: "[topic] best practices 2024" WebSearch: "[library] documentation [specific feature]" WebFetch: [official documentation URL] ```Also applies to: 290-303, 307-318
specs/mcp-server/design.md-148-174 (1)
148-174: Documented interfaces are async but implementation is sync.
Lines 148-174 show Promise-returning StateManager/FileManager, while the current usage inmcp-server/src/tools/start.tsand tests is synchronous. Align the doc with the actual API to avoid confusion.Doc alignment example
-interface StateManager { - read(specName: string): Promise<RalphState | null>; - write(specName: string, state: RalphState): Promise<void>; - delete(specName: string): Promise<void>; - exists(specName: string): Promise<boolean>; -} +interface StateManager { + read(specDir: string): RalphState | null; + write(specDir: string, state: RalphState): void; + delete(specDir: string): void; + exists(specDir: string): boolean; +} -interface FileManager { - readSpecFile(specName: string, filename: string): Promise<string | null>; - writeSpecFile(specName: string, filename: string, content: string): Promise<void>; - listSpecs(): Promise<string[]>; - specExists(specName: string): Promise<boolean>; - createSpecDir(specName: string): Promise<void>; - deleteSpec(specName: string): Promise<void>; - getCurrentSpec(): Promise<string | null>; - setCurrentSpec(name: string): Promise<void>; -} +interface FileManager { + readSpecFile(specName: string, filename: string): string | null; + writeSpecFile(specName: string, filename: string, content: string): boolean; + listSpecs(): string[]; + specExists(specName: string): boolean; + createSpecDir(specName: string): boolean; + deleteSpec(specName: string): boolean; + getCurrentSpec(): string | null; + setCurrentSpec(name: string): boolean; +}mcp-server/src/lib/files.ts-102-105 (1)
102-105: HardenspecExistsagainst TOCTOU/permission errors.
statSynccan throw even ifexistsSyncjust returned true (race or permissions), which would crash callers.🛠️ Suggested guard
specExists(specName: string): boolean { const specDir = this.getSpecDir(specName); - return existsSync(specDir) && statSync(specDir).isDirectory(); + try { + return existsSync(specDir) && statSync(specDir).isDirectory(); + } catch (error) { + this.logger.warning("Failed to stat spec directory", { + path: specDir, + error: error instanceof Error ? error.message : String(error), + }); + return false; + } }
🧹 Nitpick comments (21)
mcp-server/scripts/install.sh (2)
34-34: Unconditionalsudomay fail in containerized or restricted environments.Using
sudounconditionally will fail in environments where it's unavailable or the user already has write access toINSTALL_DIR.Suggested fix
-sudo mv "/tmp/$BINARY_NAME" "$INSTALL_DIR/$BINARY_NAME" +if [[ -w "$INSTALL_DIR" ]]; then + mv "/tmp/$BINARY_NAME" "$INSTALL_DIR/$BINARY_NAME" +else + sudo mv "/tmp/$BINARY_NAME" "$INSTALL_DIR/$BINARY_NAME" +fi
17-21: Windows detection provides limited value in a bash script.The script detects Windows (mingw/msys/cygwin) but native Windows users cannot run bash scripts without WSL or similar environments. Consider documenting this limitation or providing a PowerShell alternative for Windows users.
mcp-server/src/assets/templates/tasks.md (1)
161-228: Phase 4.2 and Phase 5.1 overlap in PR creation logic.Both task 4.2 (lines 161-194) and task 5.1 (lines 211-229) contain PR creation steps with
gh pr create. This duplication could confuse executors about when to actually create the PR. Consider consolidating PR creation into Phase 5 only and having Phase 4.2 focus solely on local validation and pushing the branch.mcp-server/package.json (1)
21-23: Pin@types/bunto a specific version for reproducible builds.Using
"latest"for@types/buncan cause CI instability and type-check failures when new versions introduce breaking changes. Consider pinning to a specific version.Suggested fix
"devDependencies": { - "@types/bun": "latest", + "@types/bun": "^1.2.0", "typescript": "^5.7.0" },mcp-server/scripts/build.sh (1)
4-5: Remove or use theVERSIONvariable.The
VERSIONvariable is extracted from package.json but never used. Either remove it or incorporate it into the output filenames for versioned artifacts.Option 1: Remove unused variable
#!/bin/bash set -e -VERSION=$(jq -r '.version' package.json) OUTDIR="dist"Option 2: Use version in artifact names
for platform in "${platforms[@]}"; do echo "Building for $platform..." - outfile="$OUTDIR/ralph-specum-mcp-${platform#bun-}" + outfile="$OUTDIR/ralph-specum-mcp-${VERSION}-${platform#bun-}" [[ "$platform" == *windows* ]] && outfile="${outfile}.exe".github/workflows/mcp-release.yml (2)
84-111: Add version validation before npm publish.The workflow publishes to npm without verifying that the git tag version matches
package.jsonversion. This could result in publishing an incorrect version or a failed publish if npm rejects a version that already exists.Add version check step
- name: Install dependencies working-directory: mcp-server run: bun install + - name: Verify version matches tag + working-directory: mcp-server + run: | + PACKAGE_VERSION=$(jq -r '.version' package.json) + TAG_VERSION=${GITHUB_REF#refs/tags/v} + if [ "$PACKAGE_VERSION" != "$TAG_VERSION" ]; then + echo "Error: package.json version ($PACKAGE_VERSION) does not match tag ($TAG_VERSION)" + exit 1 + fi + - name: Publish to npm working-directory: mcp-server run: npm publish --access public
55-83: Consider adding checksums to release artifacts.For security-conscious users, consider generating and publishing SHA256 checksums alongside the binaries. This allows users to verify artifact integrity after download.
Add checksum generation
- name: List artifacts run: ls -la dist/ + - name: Generate checksums + run: | + cd dist + sha256sum * > checksums.sha256 + - name: Create Release uses: softprops/action-gh-release@v2 with: files: | dist/ralph-specum-mcp-darwin-arm64 dist/ralph-specum-mcp-darwin-x64 dist/ralph-specum-mcp-linux-x64 dist/ralph-specum-mcp-windows-x64.exe + dist/checksums.sha256 generate_release_notes: truemcp-server/src/assets/agents/product-manager.md (1)
131-141: Temp file approach in jq command may cause issues.The command
jq ... > /tmp/state.json && mv /tmp/state.json ...could fail or cause conflicts:
/tmpmay have restrictive permissions in some environments- Concurrent spec executions could overwrite the same temp file
Consider using
spongefrom moreutils or inline editing with a spec-specific temp file.Safer approach using spec-specific temp file
-jq '.awaitingApproval = true' ./specs/<spec>/.ralph-state.json > /tmp/state.json && mv /tmp/state.json ./specs/<spec>/.ralph-state.json +jq '.awaitingApproval = true' ./specs/<spec>/.ralph-state.json > ./specs/<spec>/.ralph-state.json.tmp && mv ./specs/<spec>/.ralph-state.json.tmp ./specs/<spec>/.ralph-state.jsonmcp-server/tests/setup.test.ts (1)
18-20: Consider removing trivial assertion.This test only asserts
true === truewhich validates nothing meaningful. The subsequent tests already provebun:testis working. Consider removing this test or replacing it with something more useful like validating the test environment.Suggested removal
describe("Test Infrastructure", () => { - test("bun test runs successfully", () => { - expect(true).toBe(true); - }); - test("createTempDir creates a temporary directory", async () => {mcp-server/src/tools/help.ts (1)
14-70: Static tool list may drift from actual registrations.The
TOOLSarray duplicates tool metadata that's also defined intools/index.tsregistrations. If tools are added, removed, or renamed, this list must be manually synchronized. Consider deriving this from the actual tool registrations or a shared constant to ensure consistency.Alternative: export shared tool metadata
Create a shared
TOOL_METADATAconstant intools/index.tsthat bothregisterToolsandhandleHelpcan reference:// In tools/index.ts or a shared constants file export const TOOL_METADATA: ToolInfo[] = [ { name: "ralph_start", description: "Create a new spec and begin the workflow", args: "name?, goal?, quick?" }, // ... rest of tools ];Then in
help.ts:import { TOOL_METADATA } from "./index"; // Use TOOL_METADATA instead of local TOOLS constantmcp-server/src/index.ts (2)
84-96: Unreachable code afterprocess.exit().The
return falsestatements on lines 87 and 91 are unreachable sinceprintHelp()andprintVersion()callprocess.exit(0). While this doesn't cause bugs, it's misleading. Consider inlining the exit logic or usingneverreturn type.Cleaner approach using never return type
-function printVersion(): void { +function printVersion(): never { console.log(`${SERVER_NAME} v${SERVER_VERSION}`); process.exit(0); } -function printHelp(): void { +function printHelp(): never { // ... help text ... process.exit(0); } function handleCliFlags(): boolean { const args = process.argv.slice(2); for (const arg of args) { if (arg === "--help" || arg === "-h") { printHelp(); + // TypeScript knows this is unreachable with never return type } if (arg === "--version" || arg === "-v") { printVersion(); } } return true; }
126-126: Hardcoded tool count may become stale.The tool count
11is hardcoded. If tools are added or removed, this log statement will report incorrect information. Consider deriving this from the actual registration count or removing it.specs/mcp-server/research.md (1)
264-273: Add language specifier to fenced code block.Static analysis (markdownlint MD040) flags this code block as missing a language specifier. Since this is a directory tree structure, use
textorplaintext.Suggested fix
-``` +```text plugins/ralph-specum/ ├── .claude-plugin/plugin.json # Plugin manifest (name, version, description)mcp-server/src/tools/tasks.ts (1)
154-162: Add explicit per-task isolation and progress tracking guidance.This makes it unambiguous that tasks must be independently executable and tracked end‑to‑end during execution.
Suggested tweak
expectedActions: [ "Review the design, requirements, and research", "Break down work into executable tasks with POC-first approach", "Define clear Do, Files, Done when, Verify, and Commit for each task", + "Ensure each task is self-contained for isolated execution (Task tool) with progress/checkmarks tracked in .progress.md and tasks.md", "Insert quality checkpoints every 2-3 tasks", "Organize into phases: POC, Refactoring, Testing, Quality Gates, PR Lifecycle", "Write tasks to ./specs/" + specName + "/tasks.md", "Update .progress.md with task planning summary", ],Based on learnings, please keep task isolation and progress/checkmark updates explicit.
mcp-server/src/assets/agents/task-planner.md (1)
421-427: Call out task isolation + progress/checkmark updates explicitly.This makes the execution contract unambiguous for downstream agents.
Suggested addition
- **Traceable**: References requirements and design sections - **Explicit**: No ambiguity, spell out exact steps - **Verifiable**: Has a command/action to verify completion - **Committable**: Includes conventional commit message - **Autonomous**: Agent can execute without asking questions +- **Isolated**: Each task runs via Task tool; update .progress.md and check off tasks.md on completionBased on learnings, task isolation and progress/checkmark tracking should be explicit.
mcp-server/src/lib/logger.ts (1)
49-60: Consider array handling in data merging.When
datais an array,typeof data === "object"is true, but spreading an array into an object produces numeric string keys ({ message, "0": val, "1": val, ... }), which may not be the intended behavior.🔧 Suggested improvement for explicit array handling
private log(level: LogLevel, message: string, data?: unknown): void { const logMessage: LogMessage = { level, logger: this.name, data: data !== undefined - ? { message, ...((typeof data === "object" && data !== null) ? data : { value: data }) } + ? { message, ...((typeof data === "object" && data !== null && !Array.isArray(data)) ? data : { value: data }) } : { message }, timestamp: new Date().toISOString(), }; // Always use console.error to write to stderr - NEVER console.log console.error(JSON.stringify(logMessage)); }mcp-server/src/lib/state.ts (1)
258-272: Backup may silently overwrite previous backups.If a corrupt file is encountered multiple times, each
backupCorruptFilecall overwrites the previous.bakfile. Consider timestamped backups to preserve history for debugging.mcp-server/src/tools/cancel.ts (1)
48-57: Redundant input validation.The
inputparameter is already typed asCancelInput, which implies it has been validated upstream. ThesafeParsehere will always succeed unless the caller bypasses TypeScript typing. This is defensive but adds unnecessary overhead.Consider removing the redundant validation or documenting why it's needed (e.g., for runtime safety when called from untyped JavaScript).
specs/mcp-server/tasks.md (1)
644-646: Add language specifier to fenced code block.The code block showing phase dependencies lacks a language specifier, which triggers a markdown lint warning (MD040).
🔧 Suggested fix
-``` +```text Phase 1 (POC) -> Phase 2 (Refactor) -> Phase 3 (Testing) -> Phase 4 (Quality) -> Phase 5 (PR Lifecycle)</details> </blockquote></details> <details> <summary>mcp-server/tests/files.test.ts (1)</summary><blockquote> `24-30`: **Logger outputs during tests may add noise.** The comment on line 27 says "Create logger that won't output during tests" but `MCPLogger` writes to stderr unconditionally. Consider using a mock logger or suppressing stderr during tests if log output becomes noisy. </blockquote></details> <details> <summary>mcp-server/src/tools/index.ts (1)</summary><blockquote> `144-305`: **Use the full Zod schemas directly instead of reconstructing raw shape objects.** The MCP SDK v1.0.0 accepts raw shape objects (the current approach), but it also supports full Zod schemas. Since your full schemas are already defined and imported (`SwitchInputSchema`, `CancelInputSchema`, etc.), pass them directly to `inputSchema` for cleaner, more maintainable code: <details> <summary>Example adjustment</summary> ```diff - inputSchema: { - name: SwitchInputSchema.shape.name.describe("Name of the spec to switch to"), - }, + inputSchema: SwitchInputSchema,This eliminates the redundant
.shapeextraction and manual.describe()calls, keeping the schema definition in one place.
| @@ -0,0 +1 @@ | |||
| registry=https://registry.npmmirror.com/ | |||
There was a problem hiding this comment.
Reconsider using a regional npm mirror as the default registry.
Using registry.npmmirror.com (a China-based npm mirror) as the default registry in a committed .npmrc can cause issues:
- CI/CD reliability: Builds running outside China may experience slower downloads or sync delays.
- Global contributors: Developers in other regions may face connectivity or availability issues.
- Package publication: Publishing should target the official npm registry (
https://registry.npmjs.org/).
Consider removing this file or using the official npm registry. Individual developers can configure regional mirrors locally without committing them to the repository.
Suggested fix
Either remove mcp-server/.npmrc entirely, or use the official registry:
-registry=https://registry.npmmirror.com/
+registry=https://registry.npmjs.org/📝 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.
| registry=https://registry.npmmirror.com/ | |
| registry=https://registry.npmjs.org/ |
🤖 Prompt for AI Agents
In `@mcp-server/.npmrc` at line 1, The committed .npmrc sets the default registry
to "registry=https://registry.npmmirror.com/", which can cause cross-region CI
and contributor issues; either delete the .npmrc from the repo or update its
contents to the official npm registry by replacing that line with
"registry=https://registry.npmjs.org/" so repo-wide installs/publishes use the
standard registry while allowing developers to configure mirrors locally.
| curl -fsSL "https://github.com/$REPO/releases/download/$LATEST/$ASSET" -o "/tmp/$BINARY_NAME" | ||
| chmod +x "/tmp/$BINARY_NAME" | ||
| sudo mv "/tmp/$BINARY_NAME" "$INSTALL_DIR/$BINARY_NAME" |
There was a problem hiding this comment.
Security: No checksum verification for downloaded binary.
Downloading and executing a binary without verifying its integrity poses a supply-chain attack risk. Consider publishing checksums with releases and verifying them before installation.
Suggested approach
- Publish a
checksums.txtfile with each release containing SHA256 hashes. - Verify the downloaded binary:
curl -fsSL "https://github.com/$REPO/releases/download/$LATEST/$ASSET" -o "/tmp/$BINARY_NAME"
+curl -fsSL "https://github.com/$REPO/releases/download/$LATEST/checksums.txt" -o "/tmp/checksums.txt"
+
+EXPECTED_HASH=$(grep "$ASSET" /tmp/checksums.txt | awk '{print $1}')
+ACTUAL_HASH=$(sha256sum "/tmp/$BINARY_NAME" | awk '{print $1}')
+if [[ "$EXPECTED_HASH" != "$ACTUAL_HASH" ]]; then
+ echo "Error: Checksum verification failed!"
+ rm -f "/tmp/$BINARY_NAME" "/tmp/checksums.txt"
+ exit 1
+fi
+
chmod +x "/tmp/$BINARY_NAME"📝 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.
| curl -fsSL "https://github.com/$REPO/releases/download/$LATEST/$ASSET" -o "/tmp/$BINARY_NAME" | |
| chmod +x "/tmp/$BINARY_NAME" | |
| sudo mv "/tmp/$BINARY_NAME" "$INSTALL_DIR/$BINARY_NAME" | |
| curl -fsSL "https://github.com/$REPO/releases/download/$LATEST/$ASSET" -o "/tmp/$BINARY_NAME" | |
| curl -fsSL "https://github.com/$REPO/releases/download/$LATEST/checksums.txt" -o "/tmp/checksums.txt" | |
| EXPECTED_HASH=$(grep "$ASSET" /tmp/checksums.txt | awk '{print $1}') | |
| ACTUAL_HASH=$(sha256sum "/tmp/$BINARY_NAME" | awk '{print $1}') | |
| if [[ "$EXPECTED_HASH" != "$ACTUAL_HASH" ]]; then | |
| echo "Error: Checksum verification failed!" | |
| rm -f "/tmp/$BINARY_NAME" "/tmp/checksums.txt" | |
| exit 1 | |
| fi | |
| chmod +x "/tmp/$BINARY_NAME" | |
| sudo mv "/tmp/$BINARY_NAME" "$INSTALL_DIR/$BINARY_NAME" |
🤖 Prompt for AI Agents
In `@mcp-server/scripts/install.sh` around lines 32 - 34, The install script
downloads and installs "$ASSET" without verifying integrity; add SHA256 checksum
verification by downloading the release's checksums file (e.g., checksums.txt
for $LATEST), extract the expected hash for "$ASSET"/"$BINARY_NAME", compute the
downloaded file's SHA256 (for "/tmp/$BINARY_NAME"), compare them and abort on
mismatch before running chmod/mv; ensure the script logs a clear error and exits
non‑zero if verification fails and only proceeds to chmod +x and sudo mv when
the checksum matches.
| ```bash | ||
| ( | ||
| flock -x 200 | ||
| # Read tasks.md, update checkmark, write back | ||
| sed -i 's/- \[ \] X.Y/- [x] X.Y/' "./specs/<spec>/tasks.md" | ||
| ) 200>"./specs/<spec>/.tasks.lock" | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
sed -i option differences GNU sed BSD sed macOS portability
💡 Result:
sed -i on GNU vs BSD/macOS (portability pitfalls)
1) -i exists, but it’s not POSIX
- The in-place flag
-iis not specified by POSIX, so implementations differ. [4]
2) GNU sed (typical on Linux)
-itakes an optional backup suffix. You can do either:- No backup:
sed -i 's/a/b/' file - With backup:
sed -i.bak 's/a/b/' file
- No backup:
- GNU documents this as
-i[SUFFIX]/--in-place[=SUFFIX]. [1]
3) BSD sed (macOS /usr/bin/sed)
-ieffectively requires a suffix argument; to mean “no backup”, you must pass an empty string as a separate argument:- No backup:
sed -i '' 's/a/b/' file[2] - With backup:
sed -i .bak 's/a/b/' file(or equivalently-i.bakwith a non-empty suffix) [2]
- No backup:
4) The classic breakage
- This works on GNU sed but breaks on macOS because the script is mistaken for the suffix:
sed -i 's/a/b/' file→ on macOS,'s/a/b/'is taken as the extension. [2]
Portable patterns
Best “works on GNU + macOS” one-liner (creates backup):
sed -i.bak 's/a/b/g' fileThis works on both because the suffix is non-empty and attached to -i. [3]
If you don’t want backups (portable-ish approach):
sed -i.bak 's/a/b/g' file && rm -f file.bak(Still not POSIX, but works across GNU + macOS sed.) [3]
Fully POSIX-portable (no -i):
tmp=$(mktemp) && sed 's/a/b/g' file >"$tmp" && cat "$tmp" >file && rm -f "$tmp"(Uses redirection + temp file, avoiding -i entirely.) [4]
Sources: GNU sed manual for -i[SUFFIX] behavior [1]; macOS/BSD behavior requiring -i '' for “no backup” [2]; cross-platform recommendation to always use a non-empty suffix like .bak [3]; -i not being POSIX and --posix not governing CLI options like -i [4].
🌐 Web query:
flock command macOS availability support
💡 Result:
- macOS supports
flockas a system call (flock(2)) for advisory file locking (usable from C/C++ and languages that expose it). Check withman 2 flock. [1] - macOS generally does not ship a
flockcommand (flock(1)) in the default userland, so scripts that assume/usr/bin/flockwon’t work out of the box. [2] - To get a
flock(1)CLI on macOS, install it via Homebrew—commonly by installing util-linux, which providesflock(often as a keg-only package, meaning it may not be on PATH automatically). [3]
Sources: [1] Apple flock(2) man page, [2] Stack Overflow discussion on macOS flock(1) availability, [3] Homebrew util-linux formula page.
macOS cross-platform support blocked by sed -i and missing flock command.
The code has two portability issues for macOS (darwin-arm64, darwin-x64):
-
sed -iwithout backup extension fails on macOS. BSD sed (the default on macOS) requiressed -i ''(with empty string) for in-place editing without a backup. The current syntax works only on GNU sed (Linux). -
flock(1)command is not available on macOS by default. macOS hasflock(2)as a system call but does not ship with theflockcommand-line tool. The code'sflock -x 200will fail with "command not found" unless users install util-linux via Homebrew—an undocumented requirement.
Both must be resolved for cross-platform builds. Use a temp file approach (portable across all platforms) or document macOS prerequisites and provide equivalent locking mechanisms.
Portable alternative using temp file and POSIX locking
(
- flock -x 200
# Read tasks.md, update checkmark, write back
- sed -i 's/- \[ \] X.Y/- [x] X.Y/' "./specs/<spec>/tasks.md"
-) 200>"./specs/<spec>/.tasks.lock"
+ # Use temp file and POSIX-compatible approach
+ tmp=$(mktemp)
+ sed 's/- \[ \] X.Y/- [x] X.Y/' "./specs/<spec>/tasks.md" >"$tmp" && \
+ cat "$tmp" >"./specs/<spec>/tasks.md"
+ rm -f "$tmp"
+)🤖 Prompt for AI Agents
In `@mcp-server/src/assets/agents/spec-executor.md` around lines 310 - 316, The
current snippet uses GNU-only `sed -i` and the `flock` CLI (`flock -x 200`)
which breaks on macOS; replace the CLI locking and in-place sed with a portable
pattern: acquire an atomic lock by creating a lock directory (e.g., mkdir
"./specs/<spec>/.tasks.lockdir" in a retry loop, remove it on exit), write the
modified content to a temporary file (e.g., create temp next to
"./specs/<spec>/tasks.md"), run sed without -i to output to that temp, then
atomically mv the temp over "./specs/<spec>/tasks.md" and finally release the
lock directory; reference the symbols "./specs/<spec>/tasks.md", ".tasks.lock"
(replace with ".tasks.lockdir"), `sed -i` and `flock -x 200` when locating the
code to change.
| getSpecDir(specName: string): string { | ||
| return join(this.getSpecsDir(), specName); | ||
| } | ||
|
|
||
| /** | ||
| * Get the absolute path to a file within a spec directory. | ||
| * | ||
| * @param specName - Name of the spec | ||
| * @param fileName - Name of the file within the spec directory | ||
| * @returns Absolute path to ./specs/{specName}/{fileName} | ||
| */ | ||
| getSpecFilePath(specName: string, fileName: string): string { | ||
| return join(this.getSpecDir(specName), fileName); | ||
| } | ||
|
|
||
| /** | ||
| * Get the absolute path to the .current-spec file. | ||
| * | ||
| * @returns Absolute path to ./specs/.current-spec | ||
| */ | ||
| getCurrentSpecPath(): string { | ||
| return join(this.getSpecsDir(), CURRENT_SPEC_FILE); | ||
| } | ||
|
|
||
| /** | ||
| * Check if a spec directory exists. | ||
| * | ||
| * @param specName - Name of the spec to check | ||
| * @returns true if the spec directory exists and is a directory | ||
| */ | ||
| specExists(specName: string): boolean { | ||
| const specDir = this.getSpecDir(specName); | ||
| return existsSync(specDir) && statSync(specDir).isDirectory(); | ||
| } | ||
|
|
||
| /** | ||
| * List all spec directories. | ||
| * | ||
| * Returns only directory names (not files) from the specs directory, | ||
| * sorted alphabetically. | ||
| * | ||
| * @returns Array of spec names, or empty array if none exist | ||
| */ | ||
| listSpecs(): string[] { | ||
| const specsDir = this.getSpecsDir(); | ||
|
|
||
| if (!existsSync(specsDir)) { | ||
| return []; | ||
| } | ||
|
|
||
| try { | ||
| const entries = readdirSync(specsDir, { withFileTypes: true }); | ||
| return entries | ||
| .filter((entry) => entry.isDirectory()) | ||
| .map((entry) => entry.name) | ||
| .sort(); | ||
| } catch (error) { | ||
| this.logger.error("Failed to list specs", { | ||
| path: specsDir, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create a spec directory. | ||
| * | ||
| * Creates the directory recursively if parent directories don't exist. | ||
| * | ||
| * @param specName - Name of the spec directory to create | ||
| * @returns true on success, false on failure | ||
| */ | ||
| createSpecDir(specName: string): boolean { | ||
| const specDir = this.getSpecDir(specName); | ||
|
|
||
| try { | ||
| if (!existsSync(specDir)) { | ||
| mkdirSync(specDir, { recursive: true }); | ||
| this.logger.debug("Created spec directory", { path: specDir }); | ||
| } | ||
| return true; | ||
| } catch (error) { | ||
| this.logger.error("Failed to create spec directory", { | ||
| path: specDir, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Delete a spec directory and all its contents. | ||
| * | ||
| * @param specName - Name of the spec directory to delete | ||
| * @returns true on success or if spec didn't exist, false on error | ||
| */ | ||
| deleteSpec(specName: string): boolean { | ||
| const specDir = this.getSpecDir(specName); | ||
|
|
||
| if (!existsSync(specDir)) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| rmSync(specDir, { recursive: true, force: true }); | ||
| this.logger.debug("Deleted spec directory", { path: specDir }); | ||
| return true; | ||
| } catch (error) { | ||
| this.logger.error("Failed to delete spec directory", { | ||
| path: specDir, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Read a file from a spec directory. | ||
| * | ||
| * @param specName - Name of the spec | ||
| * @param fileName - Name of the file to read | ||
| * @returns File contents as string, or null if file doesn't exist or on error | ||
| */ | ||
| readSpecFile(specName: string, fileName: string): string | null { | ||
| const filePath = this.getSpecFilePath(specName, fileName); | ||
|
|
||
| if (!existsSync(filePath)) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| return readFileSync(filePath, "utf-8"); | ||
| } catch (error) { | ||
| this.logger.error("Failed to read spec file", { | ||
| path: filePath, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Write a file to a spec directory. | ||
| * | ||
| * Creates the spec directory if it doesn't exist. | ||
| * | ||
| * @param specName - Name of the spec | ||
| * @param fileName - Name of the file to write | ||
| * @param content - Content to write to the file | ||
| * @returns true on success, false on failure | ||
| */ | ||
| writeSpecFile(specName: string, fileName: string, content: string): boolean { | ||
| const specDir = this.getSpecDir(specName); | ||
| const filePath = this.getSpecFilePath(specName, fileName); | ||
|
|
||
| try { | ||
| // Ensure spec directory exists | ||
| if (!existsSync(specDir)) { | ||
| mkdirSync(specDir, { recursive: true }); | ||
| } | ||
|
|
||
| writeFileSync(filePath, content, "utf-8"); | ||
| this.logger.debug("Wrote spec file", { path: filePath }); | ||
| return true; | ||
| } catch (error) { | ||
| this.logger.error("Failed to write spec file", { | ||
| path: filePath, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent path traversal for spec/file names.
specName and fileName are used directly in filesystem paths. Inputs like ../ or absolute paths can escape the specs directory, enabling read/write/delete outside the workspace. This is a security issue for MCP clients.
🔒 Proposed fix: resolve paths and enforce specs-dir containment
-import { join } from "node:path";
+import { join, resolve, sep } from "node:path";
@@
export class FileManager {
private readonly logger: MCPLogger;
private readonly basePath: string;
+ private resolveSpecPath(specName: string, fileName?: string): string | null {
+ const specsDir = this.getSpecsDir();
+ const resolved = fileName
+ ? resolve(specsDir, specName, fileName)
+ : resolve(specsDir, specName);
+ if (!resolved.startsWith(specsDir + sep)) {
+ this.logger.error("Invalid spec/file path", { specName, fileName, resolved });
+ return null;
+ }
+ return resolved;
+ }
@@
createSpecDir(specName: string): boolean {
- const specDir = this.getSpecDir(specName);
+ const specDir = this.resolveSpecPath(specName);
+ if (!specDir) return false;
@@
deleteSpec(specName: string): boolean {
- const specDir = this.getSpecDir(specName);
+ const specDir = this.resolveSpecPath(specName);
+ if (!specDir) return false;
@@
readSpecFile(specName: string, fileName: string): string | null {
- const filePath = this.getSpecFilePath(specName, fileName);
+ const filePath = this.resolveSpecPath(specName, fileName);
+ if (!filePath) return null;
@@
writeSpecFile(specName: string, fileName: string, content: string): boolean {
- const specDir = this.getSpecDir(specName);
- const filePath = this.getSpecFilePath(specName, fileName);
+ const specDir = this.resolveSpecPath(specName);
+ const filePath = this.resolveSpecPath(specName, fileName);
+ if (!specDir || !filePath) return false;🤖 Prompt for AI Agents
In `@mcp-server/src/lib/files.ts` around lines 72 - 244, Inputs specName and
fileName can perform path traversal and escape the specs directory; update path
handling so all paths are resolved and validated against the specs root. In
getSpecDir and getSpecFilePath (and getCurrentSpecPath) call
path.resolve(join(...)) to produce absolute paths and ensure the resolved path
starts with the resolved getSpecsDir() + path.sep; similarly, before any
filesystem operation in specExists, createSpecDir, deleteSpec, readSpecFile, and
writeSpecFile validate that the resolved spec directory or file path is
contained inside the specs root and reject/log and return false/null on invalid
inputs; additionally normalize/strip any leading path separators or .. segments
from specName/fileName (or explicitly disallow path separators in specName) so
callers cannot pass absolute or traversal paths.
| export const StartInputSchema = z.object({ | ||
| /** Name of the spec (optional - generated from goal if not provided) */ | ||
| name: z.string().min(1).optional(), | ||
| /** Goal/description for the spec */ | ||
| goal: z.string().min(1).optional(), |
There was a problem hiding this comment.
Validate spec names to prevent path traversal.
Line 18-22 accepts any non-empty name; inputs like ../foo or names with path separators could escape ./specs unless FileManager normalizes. Enforce a safe pattern (kebab-case) or reject path separators at the input boundary, and apply the same guard for spec_name in other tools.
Suggested input hardening
-export const StartInputSchema = z.object({
- /** Name of the spec (optional - generated from goal if not provided) */
- name: z.string().min(1).optional(),
+export const StartInputSchema = z.object({
+ /** Name of the spec (optional - generated from goal if not provided) */
+ name: z
+ .string()
+ .min(1)
+ .regex(/^[a-z0-9-]+$/, "Name must be kebab-case (a-z, 0-9, '-')")
+ .optional(),#!/bin/bash
# Verify FileManager path normalization / validation
rg -n "createSpecDir|getSpecDir|specExists|setCurrentSpec" mcp-server/src/lib/files.ts🤖 Prompt for AI Agents
In `@mcp-server/src/tools/start.ts` around lines 18 - 22, The StartInputSchema
currently allows any non-empty name which can enable path traversal (e.g.,
"../foo"); update StartInputSchema to reject path separators and enforce a safe
pattern (e.g., kebab-case) by adding a Zod regex or refinement on the name field
to disallow "/" "\" and ".." sequences and require /^[a-z0-9]+(?:-[a-z0-9]+)*$/;
also apply the same validation to any incoming spec_name parameters used by
tools (references: StartInputSchema and any handlers accepting spec_name) and
add defensive checks in FileManager functions (createSpecDir, getSpecDir,
specExists, setCurrentSpec) to normalize and reject unsafe names at the
boundary. Ensure error messages are clear for invalid names.
| // Extract spec name from path for default values | ||
| const specName = specDir.split("/").pop() ?? "test-spec"; | ||
| const defaultState: RalphState = { | ||
| source: "spec", | ||
| name: specName, | ||
| basePath: `./specs/${specName}`, | ||
| phase: "research", | ||
| ...state, | ||
| }; | ||
| await writeFile( | ||
| join(specDir, ".ralph-state.json"), | ||
| JSON.stringify(defaultState, null, 2) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a mock .progress.md file in a spec directory. | ||
| * | ||
| * @param specDir - Path to the spec directory | ||
| * @param content - Optional content (defaults to basic progress template) | ||
| * | ||
| * @example | ||
| * await createMockProgressFile(specDir, "# Progress\n\n## Goal\nTest goal"); | ||
| */ | ||
| export async function createMockProgressFile( | ||
| specDir: string, | ||
| content?: string | ||
| ): Promise<void> { | ||
| const defaultContent = `# Progress | ||
|
|
||
| ## Original Goal | ||
| Test goal | ||
|
|
||
| ## Status | ||
| - Phase: research | ||
| - Started: 2026-01-26 | ||
|
|
||
| ## Completed Tasks | ||
| (none) | ||
|
|
||
| ## Current Task | ||
| Awaiting next task | ||
|
|
||
| ## Learnings | ||
| (none) | ||
|
|
||
| ## Blockers | ||
| (none) | ||
|
|
||
| ## Next | ||
| Begin research phase | ||
| `; | ||
| await writeFile(join(specDir, ".progress.md"), content ?? defaultContent); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a mock .current-spec file in the specs directory. | ||
| * | ||
| * @param specsDir - Path to the specs directory | ||
| * @param specName - Name of the current spec | ||
| * | ||
| * @example | ||
| * await createMockCurrentSpec(specsDir, "my-spec"); | ||
| */ | ||
| export async function createMockCurrentSpec( | ||
| specsDir: string, | ||
| specName: string | ||
| ): Promise<void> { | ||
| await writeFile(join(specsDir, ".current-spec"), specName); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a mock tasks.md file in a spec directory. | ||
| * | ||
| * @param specDir - Path to the spec directory | ||
| * @param tasks - Array of task descriptions (unchecked by default) | ||
| * @param completedIndices - Array of indices that should be marked as completed | ||
| * | ||
| * @example | ||
| * await createMockTasksFile(specDir, ["Task 1", "Task 2"], [0]); | ||
| * // Creates tasks with Task 1 checked, Task 2 unchecked | ||
| */ | ||
| export async function createMockTasksFile( | ||
| specDir: string, | ||
| tasks: string[] = ["1.1 First task", "1.2 Second task"], | ||
| completedIndices: number[] = [] | ||
| ): Promise<void> { | ||
| const taskLines = tasks.map((task, index) => { | ||
| const checked = completedIndices.includes(index) ? "x" : " "; | ||
| return `- [${checked}] ${task}`; | ||
| }); | ||
|
|
||
| const content = `--- | ||
| spec: test-spec | ||
| phase: tasks | ||
| total_tasks: ${tasks.length} | ||
| --- | ||
|
|
||
| # Tasks | ||
|
|
||
| ## Phase 1: POC | ||
|
|
||
| ${taskLines.join("\n")} | ||
| `; | ||
| await writeFile(join(specDir, "tasks.md"), content); | ||
| } | ||
|
|
||
| /** | ||
| * Reads a file and returns its content as a string. | ||
| * Useful for asserting file contents in tests. | ||
| * | ||
| * @param filePath - Absolute path to the file | ||
| * @returns Promise<string> - File contents | ||
| * | ||
| * @example | ||
| * const content = await readTestFile(join(specDir, ".progress.md")); | ||
| * expect(content).toContain("research"); | ||
| */ | ||
| export async function readTestFile(filePath: string): Promise<string> { | ||
| return await readFile(filePath, "utf-8"); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a file or directory exists at the given path. | ||
| * | ||
| * @param filePath - Absolute path to check | ||
| * @returns Promise<boolean> - True if file or directory exists | ||
| * | ||
| * @example | ||
| * const exists = await fileExists(join(specDir, ".ralph-state.json")); | ||
| */ | ||
| export async function fileExists(filePath: string): Promise<boolean> { | ||
| try { | ||
| await stat(filePath); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Creates a complete mock spec setup for integration testing. | ||
| * Sets up tempDir, specs directory, spec folder, state file, and progress file. | ||
| * | ||
| * @param specName - Name of the spec to create | ||
| * @param options - Configuration options | ||
| * @returns Object with paths and cleanup function | ||
| * | ||
| * @example | ||
| * const { tempDir, specDir, specsDir, cleanup } = await createFullMockSpec("test-spec", { | ||
| * phase: "design", | ||
| * withTasks: true | ||
| * }); | ||
| * try { | ||
| * // ... run tests ... | ||
| * } finally { | ||
| * await cleanup(); | ||
| * } | ||
| */ | ||
| export async function createFullMockSpec( | ||
| specName: string, | ||
| options: { | ||
| phase?: Phase; | ||
| withTasks?: boolean; | ||
| tasks?: string[]; | ||
| completedTasks?: number[]; | ||
| progressContent?: string; | ||
| } = {} | ||
| ): Promise<{ | ||
| tempDir: string; | ||
| specsDir: string; | ||
| specDir: string; | ||
| cleanup: () => Promise<void>; | ||
| }> { | ||
| const tempDir = await createTempDir(); | ||
| const specsDir = await createMockSpecsDir(tempDir, [specName]); | ||
| const specDir = join(specsDir, specName); | ||
|
|
||
| await createMockStateFile(specDir, { phase: options.phase ?? "research" }); | ||
| await createMockProgressFile(specDir, options.progressContent); | ||
| await createMockCurrentSpec(specsDir, specName); | ||
|
|
||
| if (options.withTasks || options.tasks) { | ||
| await createMockTasksFile( | ||
| specDir, | ||
| options.tasks, | ||
| options.completedTasks ?? [] | ||
| ); | ||
| } | ||
|
|
||
| return { | ||
| tempDir, | ||
| specsDir, | ||
| specDir, | ||
| cleanup: async () => cleanupTempDir(tempDir), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Mock FileManager for unit testing tools without file system access. | ||
| * Provides in-memory implementation of FileManager interface. | ||
| */ | ||
| export class MockFileManager { | ||
| private files: Map<string, string> = new Map(); | ||
| private directories: Set<string> = new Set(); | ||
| private currentSpec: string | null = null; | ||
|
|
||
| constructor(private basePath: string = "/mock") {} | ||
|
|
||
| /** | ||
| * Set up mock files for testing | ||
| */ | ||
| setFile(relativePath: string, content: string): void { | ||
| this.files.set(join(this.basePath, relativePath), content); | ||
| } | ||
|
|
||
| /** | ||
| * Set up mock directories for testing | ||
| */ | ||
| setDirectory(relativePath: string): void { | ||
| this.directories.add(join(this.basePath, relativePath)); | ||
| } | ||
|
|
||
| /** | ||
| * Mock implementations of FileManager methods | ||
| */ | ||
| async readSpecFile(specName: string, fileName: string): Promise<string | null> { | ||
| const path = join(this.basePath, "specs", specName, fileName); | ||
| return this.files.get(path) ?? null; | ||
| } | ||
|
|
||
| async writeSpecFile(specName: string, fileName: string, content: string): Promise<void> { | ||
| const path = join(this.basePath, "specs", specName, fileName); | ||
| this.files.set(path, content); | ||
| } | ||
|
|
||
| async listSpecs(): Promise<string[]> { | ||
| const specsPath = join(this.basePath, "specs"); | ||
| return Array.from(this.directories) | ||
| .filter((d) => d.startsWith(specsPath) && d !== specsPath) | ||
| .map((d) => d.replace(specsPath + "/", "").split("/")[0]) | ||
| .filter((v, i, a) => a.indexOf(v) === i); // unique | ||
| } | ||
|
|
||
| async specExists(specName: string): Promise<boolean> { | ||
| return this.directories.has(join(this.basePath, "specs", specName)); | ||
| } | ||
|
|
||
| async createSpecDir(specName: string): Promise<void> { | ||
| this.directories.add(join(this.basePath, "specs", specName)); | ||
| } | ||
|
|
||
| async deleteSpec(specName: string): Promise<void> { | ||
| const prefix = join(this.basePath, "specs", specName); | ||
| for (const path of this.files.keys()) { | ||
| if (path.startsWith(prefix)) { | ||
| this.files.delete(path); | ||
| } | ||
| } | ||
| this.directories.delete(prefix); | ||
| } | ||
|
|
||
| async getCurrentSpec(): Promise<string | null> { | ||
| return this.currentSpec; | ||
| } | ||
|
|
||
| async setCurrentSpec(specName: string): Promise<void> { | ||
| this.currentSpec = specName; | ||
| } | ||
|
|
||
| getBasePath(): string { | ||
| return this.basePath; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Mock StateManager for unit testing tools without file system access. | ||
| */ | ||
| export class MockStateManager { | ||
| private states: Map<string, RalphState> = new Map(); | ||
|
|
||
| constructor(private basePath: string = "/mock") {} | ||
|
|
||
| /** | ||
| * Set up mock state for testing | ||
| */ | ||
| setState(specName: string, state: RalphState): void { | ||
| this.states.set(specName, state); | ||
| } | ||
|
|
||
| /** | ||
| * Mock implementations of StateManager methods | ||
| */ | ||
| async read(specDir: string): Promise<RalphState | null> { | ||
| const specName = specDir.split("/").pop()!; | ||
| return this.states.get(specName) ?? null; | ||
| } | ||
|
|
||
| async write(specDir: string, state: RalphState): Promise<void> { | ||
| const specName = specDir.split("/").pop()!; | ||
| this.states.set(specName, state); | ||
| } | ||
|
|
||
| async delete(specDir: string): Promise<void> { | ||
| const specName = specDir.split("/").pop()!; | ||
| this.states.delete(specName); | ||
| } | ||
|
|
||
| async exists(specDir: string): Promise<boolean> { | ||
| const specName = specDir.split("/").pop()!; | ||
| return this.states.has(specName); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file structure first
find . -name "utils.ts" -path "*/tests/*" | head -20Repository: tzachbon/smart-ralph
Length of output: 91
🏁 Script executed:
# Search for all split("/") patterns in TypeScript test files
rg -n 'split\("\/"\)' --type tsRepository: tzachbon/smart-ralph
Length of output: 151
🏁 Script executed:
# Also search for the specific file mentioned
cat mcp-server/tests/utils.ts | wc -lRepository: tzachbon/smart-ralph
Length of output: 67
🏁 Script executed:
# Get the specific lines to verify the instances mentioned
sed -n '78,110p' mcp-server/tests/utils.tsRepository: tzachbon/smart-ralph
Length of output: 892
🏁 Script executed:
# Search for other path manipulation patterns that might have the same issue
rg -n 'replace.*\+.*"/"' --type ts mcp-server/tests/Repository: tzachbon/smart-ralph
Length of output: 157
🏁 Script executed:
# Search for split("/") with proper escaping
rg -n 'split\("' --type ts mcp-server/tests/utils.tsRepository: tzachbon/smart-ralph
Length of output: 402
🏁 Script executed:
# Get the MockStateManager methods to check for split("/") usage there
sed -n '320,390p' mcp-server/tests/utils.tsRepository: tzachbon/smart-ralph
Length of output: 1881
🏁 Script executed:
# Search for any .replace patterns with "/" in the file
rg -n 'replace.*"/"' --type ts mcp-server/tests/utils.tsRepository: tzachbon/smart-ralph
Length of output: 131
🏁 Script executed:
# Check if there are any cross-platform or Windows-related claims in project files
rg -i "windows|cross.platform|node_modules" README.md package.json 2>/dev/null | head -20Repository: tzachbon/smart-ralph
Length of output: 46
🏁 Script executed:
# Check the overall path handling approach in the codebase
rg -n 'from.*"node:path"' --type ts | head -10Repository: tzachbon/smart-ralph
Length of output: 793
🏁 Script executed:
# See if basename/relative are already imported elsewhere in the tests
rg -n 'basename|relative' --type ts mcp-server/tests/ | head -20Repository: tzachbon/smart-ralph
Length of output: 410
🏁 Script executed:
# Check the context around line 318 to understand the full listSpecs method
sed -n '310,325p' mcp-server/tests/utils.tsRepository: tzachbon/smart-ralph
Length of output: 630
🏁 Script executed:
# Verify the imports at the top of utils.ts
sed -n '1,15p' mcp-server/tests/utils.tsRepository: tzachbon/smart-ralph
Length of output: 568
Fix Windows path handling in test helpers.
The test utilities use hardcoded "/" separators for path operations, which will fail on Windows (which uses backslash). This breaks cross-platform compatibility despite already using node:path.join() elsewhere. All six instances are in createMockStateFile(), MockFileManager.listSpecs(), and the four methods of MockStateManager.
Replace hardcoded path separators with Node.js path utilities:
Required changes
-import { join } from "node:path";
+import { join, basename, relative, sep } from "node:path";Line 79 in createMockStateFile():
- const specName = specDir.split("/").pop() ?? "test-spec";
+ const specName = basename(specDir) || "test-spec";Line 318 in MockFileManager.listSpecs():
- .map((d) => d.replace(specsPath + "/", "").split("/")[0])
+ .map((d) => relative(specsPath, d).split(sep)[0])Lines 372, 377, 382, 387 in MockStateManager methods:
- const specName = specDir.split("/").pop()!;
+ const specName = basename(specDir);🤖 Prompt for AI Agents
In `@mcp-server/tests/utils.ts` around lines 78 - 389, The helpers break on
Windows by using literal "/" separators; update createMockStateFile to extract
specName with path.basename(specDir) instead of specDir.split("/").pop(), change
MockFileManager.listSpecs to compute relative paths with
path.relative(specsPath, d) (or use d.slice(specsPath.length + 1) only after
ensuring path.join semantics) and replace the startsWith(specsPath + "/") check
with path.isAbsolute comparisons or path.relative checks, and in
MockStateManager.read/write/delete/exists replace specDir.split("/").pop()! with
path.basename(specDir) so all path operations use Node's path utilities
(basename, relative, join, path.sep) rather than hardcoded slashes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@specs/mcp-server/.progress.md`:
- Line 161: There are two identical top-level headings "## Learnings" causing
structural duplication; locate both occurrences of the heading "## Learnings" in
specs/mcp-server/.progress.md and either merge their content under a single "##
Learnings" heading (remove the duplicate) or rename the second occurrence to a
distinct, hierarchical heading such as "### Additional Learnings" or "###
Task-Specific Learnings" so the sections are unambiguous and navigable.
🧹 Nitpick comments (1)
specs/mcp-server/.progress.md (1)
66-66: Consider formatting bare URLs for better markdown rendering.Several PR URLs are included as bare text without markdown link formatting. While functional, wrapping them improves rendering and follows markdown best practices.
✨ Formatting improvement
Wrap bare URLs in angle brackets or use markdown link syntax:
-- PR: https://github.com/tzachbon/smart-ralph/pull/75 +- PR: <https://github.com/tzachbon/smart-ralph/pull/75>Or use descriptive links:
- PR: [`#75`](https://github.com/tzachbon/smart-ralph/pull/75)Also applies to: 145-145, 156-156, 273-273
| - CodeRabbit: AI code review in progress (not blocking) | ||
| - PR ready for review | ||
|
|
||
| ## Learnings |
There was a problem hiding this comment.
Duplicate "Learnings" section heading creates structural confusion.
The document contains two ## Learnings headings at the same hierarchical level (lines 161 and 374). This creates ambiguity about the document structure and makes navigation confusing.
Consider one of these approaches:
- Merge both sections under a single
## Learningsheading - Restructure the second occurrence as a subsection (e.g.,
### Additional Learningsor### Task-Specific Learnings)
📝 Suggested restructuring
If keeping separate sections, differentiate them hierarchically:
-## Learnings
+### Additional Task LearningsOr merge the content under the first ## Learnings section and remove the duplicate heading.
Also applies to: 374-374
🤖 Prompt for AI Agents
In `@specs/mcp-server/.progress.md` at line 161, There are two identical top-level
headings "## Learnings" causing structural duplication; locate both occurrences
of the heading "## Learnings" in specs/mcp-server/.progress.md and either merge
their content under a single "## Learnings" heading (remove the duplicate) or
rename the second occurrence to a distinct, hierarchical heading such as "###
Additional Learnings" or "### Task-Specific Learnings" so the sections are
unambiguous and navigable.
1 participant
Summary
This PR adds a standalone MCP (Model Context Protocol) server for the ralph-specum spec-driven development workflow, enabling use in external AI tools (Cursor, Continue, etc.) beyond Claude Code.
Key Features
Architecture
Distribution Methods
curl -fsSL .../install.sh | bashnpm install -g @smart-ralph/ralph-specum-mcpTest Coverage
Test Plan
bun run typecheckbun testbun run build--version,--help🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.