Prepare MCP 0.2.5 public release candidate#61
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request expands the Martin Loop operator surface by introducing new CLI and MCP commands for diagnostics, run validation, and run-store management, including doctor, preflight, triage, and dossier. The MCP server has been refactored to support the full Model Context Protocol discovery surface with versioned resources and prompts. Key technical changes include hardened path matching in the safety leash, platform-aware MCP configuration generation, and enhanced artifact persistence. Review feedback identifies several hardening opportunities: ensuring loopTimestamp handles invalid dates gracefully, aligning isLoopRecord validation between packages for consistency, correctly escaping the ? wildcard in glob-to-regex conversion, and adding error handling to JSONL parsing to prevent crashes on malformed lines.
| function loopTimestamp(loop: LoopRecord): number { | ||
| return Date.parse(loop.updatedAt || loop.createdAt || ""); | ||
| } |
There was a problem hiding this comment.
The loopTimestamp function uses Date.parse which can return NaN if the input string is not a valid date. This can lead to unstable sorting or incorrect deduplication in upsertLoop. It should be hardened to return a default value (e.g., 0) when parsing fails.
| function loopTimestamp(loop: LoopRecord): number { | |
| return Date.parse(loop.updatedAt || loop.createdAt || ""); | |
| } | |
| function loopTimestamp(loop: LoopRecord): number { | |
| const parsed = Date.parse(loop.updatedAt || loop.createdAt || ""); | |
| return Number.isNaN(parsed) ? 0 : parsed; | |
| } |
| function isLoopRecord(value: unknown): value is LoopRecord { | ||
| if (typeof value !== "object" || value === null) { | ||
| return false; | ||
| } | ||
|
|
||
| const candidate = value as Partial<LoopRecord>; | ||
| return ( | ||
| typeof candidate.loopId === "string" && | ||
| typeof candidate.workspaceId === "string" && | ||
| typeof candidate.projectId === "string" && | ||
| typeof candidate.status === "string" && | ||
| typeof candidate.lifecycleState === "string" && | ||
| typeof candidate.createdAt === "string" && | ||
| typeof candidate.updatedAt === "string" && | ||
| typeof candidate.task?.title === "string" && | ||
| typeof candidate.task?.objective === "string" && | ||
| Array.isArray(candidate.attempts) && | ||
| Array.isArray(candidate.events) && | ||
| Array.isArray(candidate.artifacts) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The isLoopRecord validation in the CLI package is significantly less thorough than the corresponding validation in the MCP package. It lacks checks for the structure and types of nested objects like budget and cost. Aligning these validation routines would improve consistency and prevent runtime errors when processing malformed records.
| const regexStr = normalizedPattern | ||
| .replace(/[.+^${}()|[\]\\]/gu, "\\$&") | ||
| .replace(/\*\*/gu, "__DOUBLESTAR__") | ||
| .replace(/\*/gu, "[^/]*") | ||
| .replace(/__DOUBLESTAR__/gu, ".*"); | ||
|
|
||
| return ( | ||
| normalizedFile === normalizedPattern || | ||
| normalizedFile.startsWith(`${normalizedPattern.replace(/\/$/u, "")}/`) | ||
| ); | ||
| return new RegExp(`^${regexStr}$`, "u").test(normalizedFile); | ||
| } |
There was a problem hiding this comment.
The glob-to-regex conversion in matchesPathPattern does not handle the ? character correctly. It is not escaped in the initial replace call, meaning it will be interpreted as a regex quantifier (making the preceding character optional) instead of a single-character wildcard as expected in globs.
| const regexStr = normalizedPattern | |
| .replace(/[.+^${}()|[\]\\]/gu, "\\$&") | |
| .replace(/\*\*/gu, "__DOUBLESTAR__") | |
| .replace(/\*/gu, "[^/]*") | |
| .replace(/__DOUBLESTAR__/gu, ".*"); | |
| return ( | |
| normalizedFile === normalizedPattern || | |
| normalizedFile.startsWith(`${normalizedPattern.replace(/\/$/u, "")}/`) | |
| ); | |
| return new RegExp(`^${regexStr}$`, "u").test(normalizedFile); | |
| } | |
| const regexStr = normalizedPattern | |
| .replace(/[.+^${}()|[\]\\?]/gu, "\\$&") | |
| .replace(/\\\?/gu, "[^/]") | |
| .replace(/\*\*/gu, "__DOUBLESTAR__") | |
| .replace(/\*/gu, "[^/]*") | |
| .replace(/__DOUBLESTAR__/gu, ".*"); |
| } | ||
|
|
||
| async function readCanonicalLoopRecord(file: string): Promise<InspectableLoopRecord> { | ||
| try { | ||
| const text = await readFile(file, "utf8"); | ||
| return validateInspectableLoopRecord(JSON.parse(text)); | ||
| } catch { | ||
| throw storeUnreadableError(); | ||
| } | ||
| } | ||
|
|
||
| async function readRecordsFromFile(file: string): Promise<InspectableLoopRecord[]> { |
There was a problem hiding this comment.
In parseUnknownLoopValues, the JSON.parse call for .jsonl files is not wrapped in a try-catch block. If a single line in the file contains malformed JSON, the entire command will crash. Using a more resilient parsing approach that skips malformed lines would improve robustness.
function parseUnknownLoopValues(contents: string, file: string): unknown[] {
if (file.endsWith(".jsonl")) {
return contents
.split(/\r?\n/u)
.map((line) => line.trim())
.filter(Boolean)
.map((line) => {
try {
return JSON.parse(line);
} catch {
return null;
}
})
.filter((val) => val !== null);
}
const parsed = JSON.parse(contents) as unknown;
return Array.isArray(parsed) ? parsed : [parsed];
}
Keesan12
force-pushed
the
codex/mcp-v025-release-candidate
branch
from
e95cefc to
fe5ad1f
Compare
1 participant
Summary
Verification
pnpm release:matrix:localpassed on Windows lane.pnpm oss:validate,pnpm public:smoke,pnpm --filter @martinloop/mcp smoke:pack, andpnpm mcp:published:smoke:pack.README.md AGENTS.md CONTEXT.md docs packages .github package.jsonfound no local path/internal repo/private workspace contamination.npm view martin-looplive latest: 0.1.7;npm view @martinloop/mcplive latest: 0.2.0. This PR prepares the next MCP line, 0.2.5.Release Notes