fix: Fix get_file_contents failing in Trae #311
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request aims to fix compatibility issues with the Trae IDE client and GitLab 10.7.3 by making the get_file_contents API more tolerant of different input formats and response schemas.
Changes:
- Modified
GetFileContentsSchemato accept bothfile_pathandpathparameters, with optionalproject_id - Made all fields in
GitLabFileContentSchemaoptional exceptfile_pathto accommodate older GitLab API versions - Added input normalization via transform to handle parameter variations from different MCP clients
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .transform(data => ({ | ||
| project_id: (data.project_id ?? "").trim(), | ||
| file_path: (data.file_path ?? data.path ?? "").trim(), | ||
| ref: data.ref, |
There was a problem hiding this comment.
The ref field is passed through without trimming, but if it contains only whitespace, this could cause issues. The getFileContents function checks if ref is falsy (line 1446 in index.ts) to determine whether to fetch the default branch. An empty string or whitespace-only string is truthy, so it would be passed to the API as-is, potentially causing errors.
Consider trimming ref and converting empty strings to undefined in the transform, similar to how file_path is handled.
| export const GetFileContentsSchema = z | ||
| .object({ | ||
| project_id: z.coerce.string().optional().describe("Project ID or URL-encoded path"), | ||
| file_path: z.string().optional().describe("Path to the file or directory"), | ||
| path: z.string().optional().describe("Alias of file_path"), | ||
| ref: z.string().optional().describe("Branch/tag/commit to get contents from"), | ||
| }) | ||
| .superRefine((data, ctx) => { | ||
| if (!data.file_path && !data.path) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: "Either 'file_path' or 'path' must be provided", | ||
| path: ["file_path"], | ||
| }); | ||
| } | ||
| }) | ||
| .transform(data => ({ | ||
| project_id: (data.project_id ?? "").trim(), | ||
| file_path: (data.file_path ?? data.path ?? "").trim(), | ||
| ref: data.ref, | ||
| })); |
There was a problem hiding this comment.
The schema changes to GetFileContentsSchema introduce significant behavioral changes, but there are no corresponding test cases to validate these changes. The new behavior around optional project_id, path parameter aliasing, and the transform logic should be covered by unit tests to prevent regressions.
Consider adding test cases that validate: 1) behavior when project_id is omitted and GITLAB_PROJECT_ID is set, 2) behavior when path is provided instead of file_path, 3) behavior when both file_path and path are provided, 4) edge cases like whitespace-only values.
| } | ||
| }) | ||
| .transform(data => ({ | ||
| project_id: (data.project_id ?? "").trim(), |
There was a problem hiding this comment.
The transform always returns an empty string for project_id when it's not provided, even though the field is optional. This causes an issue when GITLAB_PROJECT_ID environment variable is set. When project_id is not provided by the client, the transform returns an empty string "", which is then passed to getFileContents. This empty string bypasses the fallback logic in getEffectiveProjectId (line 1338 in index.ts) which only falls back to GITLAB_PROJECT_ID when projectId is falsy. An empty string is truthy, so the fallback never happens, causing API errors.
Consider returning undefined instead of an empty string when project_id is not provided, or handling empty strings in getEffectiveProjectId.
| file_name: z.string().optional(), | ||
| file_path: z.string(), | ||
| size: z.coerce.number().optional(), | ||
| encoding: z.string().optional(), | ||
| content: z.string().optional(), | ||
| content_sha256: z.string().optional(), | ||
| ref: z.string().optional(), | ||
| blob_id: z.string().optional(), | ||
| commit_id: z.string().optional(), | ||
| last_commit_id: z.string().optional(), | ||
| execute_filemode: z.boolean().optional(), |
There was a problem hiding this comment.
Making all response fields optional in GitLabFileContentSchema could mask actual issues with the GitLab API responses. While this increases compatibility with older GitLab versions, it removes important validations. For example, file_path is the only guaranteed required field but making content, encoding, and size optional means the schema won't catch malformed responses where these critical fields are missing.
Consider keeping essential fields like content and encoding as required, or at least documenting which fields are expected to be present in a valid file response. If certain fields are truly optional only for directory responses, consider using a discriminated union instead.
| }) | ||
| .transform(data => ({ | ||
| project_id: (data.project_id ?? "").trim(), | ||
| file_path: (data.file_path ?? data.path ?? "").trim(), |
There was a problem hiding this comment.
The transform could return an empty file_path string when neither file_path nor path is provided, bypassing the superRefine validation. The superRefine validator checks if both fields are missing and adds an error, but the transform runs after validation passes. If both fields contain only whitespace, trim will produce an empty string, which could cause issues downstream in the getFileContents function.
Consider adding validation to ensure the resulting file_path after trimming is not empty, or handling empty strings in the getFileContents function.
| file_path: z.string().optional().describe("Path to the file or directory"), | ||
| path: z.string().optional().describe("Alias of file_path"), |
There was a problem hiding this comment.
The schema now accepts both file_path and path parameters, but there's no clear precedence documented. While the transform uses file_path first and falls back to path, this behavior is not documented in the schema descriptions. MCP clients using this API need to understand which parameter takes precedence when both are provided.
Consider updating the description for at least one of these fields to document the precedence order, for example: "Path to the file or directory. Takes precedence over 'path' parameter if both are provided."
zereight
left a comment
zereight
left a comment
There was a problem hiding this comment.
Can you check some ai reviews?
2 participants
list_projects/get_project/get_repository_treeworked, butget_file_contentsfailed with Invalid arguments:Invalid input, blocking reads of files like package.jsonpathinstead offile_path, and/or omitproject_idwhenGITLAB_PROJECT_IDis configured, causing strict input schema validation to reject the callschemas.ts):GetFileContentsSchematolerant: accept path alias, allow optional project_id , normalize inputsGitLabFileContentSchemafields to improve compatibility with older/self-hosted GitLab responses