Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions activate-fork.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/usr/bin/env bash
# Point Claude Code's `gitlab` MCP server at the hodyhq fork (binary + update support).
# RUN THIS WITH CLAUDE CODE FULLY QUIT, then relaunch — the running app owns
# ~/.claude.json and will clobber the edit otherwise.
#
# bash ~/.dev/Projects/gitlab-mcp/activate-fork.sh # activate fork
# bash ~/.dev/Projects/gitlab-mcp/activate-fork.sh --revert # back to upstream npx
Comment on lines +6 to +7

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Documentation inconsistency: --revert vs revert.

The usage example shows --revert, but line 12 accepts the first positional argument directly without validating or stripping a -- prefix. A user following the docs would pass --revert, which the script would treat as the mode string --revert (not revert), causing the Python code on line 29 to fail the equality check and activate instead of reverting.

Either update the docs to show revert (no dashes) or add argument parsing to strip the -- prefix.

📝 Proposed fix (update docs to match implementation)
-#   bash ~/.dev/Projects/gitlab-mcp/activate-fork.sh --revert # back to upstream npx
+#   bash ~/.dev/Projects/gitlab-mcp/activate-fork.sh revert # back to upstream npx
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@activate-fork.sh` around lines 6 - 7, The documentation comments on lines 6-7
show the usage example with a `--revert` flag, but the script reads the first
positional argument on line 12 without stripping the `--` prefix, and the
equality check on line 29 expects just `revert`. Update the usage comments to
show `revert` without dashes instead of `--revert` so the documentation matches
the actual implementation that the script expects.

set -euo pipefail

CFG="$HOME/.claude.json"
BUILD="/home/hody/.dev/Projects/gitlab-mcp/build/index.js"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Hardcoded user-specific path breaks portability.

BUILD is hardcoded to /home/hody/.dev/Projects/gitlab-mcp/build/index.js, which will fail for any user other than hody. Line 15's error message also hardcodes the same path prefix.

Derive BUILD from the script's own location or make it relative to $HOME (like CFG), or accept it as an environment variable.

🔧 Proposed fix using script-relative path
-BUILD="/home/hody/.dev/Projects/gitlab-mcp/build/index.js"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+BUILD="$SCRIPT_DIR/build/index.js"

Then update line 15:

-[ "$MODE" = "activate" ] && [ ! -f "$BUILD" ] && { echo "build missing: $BUILD (run: cd ~/.dev/Projects/gitlab-mcp && npm install && npm run build)"; exit 1; }
+[ "$MODE" = "activate" ] && [ ! -f "$BUILD" ] && { echo "build missing: $BUILD (run: cd \"$SCRIPT_DIR\" && npm install && npm run build)"; exit 1; }
📝 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.

Suggested change
BUILD="/home/hody/.dev/Projects/gitlab-mcp/build/index.js"
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
BUILD="$SCRIPT_DIR/build/index.js"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@activate-fork.sh` at line 11, The BUILD variable on line 11 uses a hardcoded
absolute path with a username that only works for the user hody, breaking
portability. Replace this hardcoded path with a script-relative approach (using
$( cd "$(dirname "${BASH_SOURCE[0]}")" && pwd ) to get the script's directory
and build the path from there), or make it relative to $HOME like the CFG
variable, or accept it as an environment variable with a fallback default.
Additionally, update the error message on line 15 that also references this
hardcoded path to use the same BUILD variable so the path stays consistent and
portable across users.

MODE="${1:-activate}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

No validation of MODE argument.

The script accepts any value for MODE but only checks for "revert" in the Python code (line 29). An invalid mode like unknown silently falls through to the activate branch, leading to unexpected behavior.

Validate MODE immediately after assignment.

✅ Proposed fix to validate MODE
 MODE="${1:-activate}"
+[[ "$MODE" =~ ^(activate|revert)$ ]] || { echo "Invalid mode: $MODE (expected 'activate' or 'revert')"; exit 1; }
📝 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.

Suggested change
MODE="${1:-activate}"
MODE="${1:-activate}"
[[ "$MODE" =~ ^(activate|revert)$ ]] || { echo "Invalid mode: $MODE (expected 'activate' or 'revert')"; exit 1; }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@activate-fork.sh` at line 12, The MODE variable is assigned from the first
argument with a default of "activate" but is never validated to ensure it
contains only acceptable values. Add validation logic immediately after the MODE
assignment that checks whether MODE is either "activate" or "revert", and
terminates the script with a descriptive error message if an invalid mode is
provided. This prevents invalid modes from silently falling through to the
activate branch.


[ -f "$CFG" ] || { echo "no $CFG"; exit 1; }
[ "$MODE" = "activate" ] && [ ! -f "$BUILD" ] && { echo "build missing: $BUILD (run: cd ~/.dev/Projects/gitlab-mcp && npm install && npm run build)"; exit 1; }

cp "$CFG" "$CFG.bak.$(date +%s)"

python3 - "$CFG" "$BUILD" "$MODE" <<'PY'
import json, sys
cfg, build, mode = sys.argv[1], sys.argv[2], sys.argv[3]
d = json.load(open(cfg)); n = 0
def walk(o):
global n
if isinstance(o, dict):
ms = o.get("mcpServers")
if isinstance(ms, dict) and "gitlab" in ms:
g = ms["gitlab"]
if mode == "revert":
g["command"] = "npx"; g["args"] = ["-y", "@zereight/mcp-gitlab"]
else:
g["command"] = "node"; g["args"] = [build]
n += 1
for v in o.values(): walk(v)
walk(d)
Comment on lines +23 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Recursive walk is over-engineered for known config structure.

The walk function recursively searches the entire config tree for any dict containing mcpServers.gitlab. Given the well-known config structure documented in docs/claude-code-setup.md, direct access to d["mcpServers"]["gitlab"] with proper error handling would be clearer and fail explicitly if the expected path is missing.

Current approach works but adds cognitive overhead and could theoretically match unintended nested structures.

♻️ Proposed refactor to use direct access
-def walk(o):
-    global n
-    if isinstance(o, dict):
-        ms = o.get("mcpServers")
-        if isinstance(ms, dict) and "gitlab" in ms:
-            g = ms["gitlab"]
-            if mode == "revert":
-                g["command"] = "npx"; g["args"] = ["-y", "`@zereight/mcp-gitlab`"]
-            else:
-                g["command"] = "node"; g["args"] = [build]
-            n += 1
-        for v in o.values(): walk(v)
-walk(d)
+try:
+    g = d["mcpServers"]["gitlab"]
+    if mode == "revert":
+        g["command"] = "npx"; g["args"] = ["-y", "`@zereight/mcp-gitlab`"]
+    else:
+        g["command"] = "node"; g["args"] = [build]
+    n = 1
+except (KeyError, TypeError) as e:
+    print(f"Error: mcpServers.gitlab not found in config: {e}", file=sys.stderr); sys.exit(1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@activate-fork.sh` around lines 23 - 35, Replace the recursive walk function
with direct access to the known config structure. Instead of iterating through
the entire config tree with the walk function, directly access
d["mcpServers"]["gitlab"] and apply the mode-based transformations (setting
"command" and "args" based on whether mode is "revert" or not) to this specific
path. Add explicit error handling with try-except or dictionary lookups using
.get() methods to fail clearly if the expected mcpServers or gitlab keys are
missing, and increment the counter variable n only after successful modification
of the gitlab configuration.

json.dump(d, open(cfg, "w"), indent=2); open(cfg, "a").write("\n")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

File opened twice for writing.

json.dump(d, open(cfg, "w"), indent=2) writes the JSON, then open(cfg, "a").write("\n") appends a newline. Opening the file twice is unnecessary; a single write operation with explicit newline would be clearer.

♻️ Proposed refactor to consolidate file writes
-json.dump(d, open(cfg, "w"), indent=2); open(cfg, "a").write("\n")
+with open(cfg, "w") as f:
+    json.dump(d, f, indent=2)
+    f.write("\n")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@activate-fork.sh` at line 36, The code on line 36 opens the configuration
file twice unnecessarily: first with open(cfg, "w") for the json.dump operation,
then again with open(cfg, "a") to append a newline. Consolidate these into a
single file operation by using a single open call with write mode that handles
both the JSON dump and the newline. Consider using a context manager with the
file opened once, writing the JSON output with json.dumps instead of json.dump
(which returns a string rather than writing directly), and then appending the
newline in the same write operation.

print(f"{'reverted' if mode=='revert' else 'activated fork on'} {n} gitlab server(s). Backup saved. Now relaunch Claude Code.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Silent success when no servers are modified.

If the config does not contain mcpServers.gitlab, the script prints "activated fork on 0 gitlab server(s)" and exits successfully. This is misleading—the user expects the script to modify the config, and zero modifications likely indicates a problem (missing or misplaced config entry).

Check n > 0 and exit with an error if no servers were updated.

🛡️ Proposed fix to validate modification count
-print(f"{'reverted' if mode=='revert' else 'activated fork on'} {n} gitlab server(s). Backup saved. Now relaunch Claude Code.")
+if n == 0:
+    print("Error: no gitlab server found in mcpServers. Check ~/.claude.json structure.", file=sys.stderr)
+    sys.exit(1)
+print(f"{'reverted' if mode=='revert' else 'activated fork on'} {n} gitlab server(s). Backup saved. Now relaunch Claude Code.")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@activate-fork.sh` at line 37, The print statement that outputs the success
message (containing "activated fork on" and the mode check for "reverted")
currently executes regardless of whether any servers were actually modified,
allowing the script to report success when zero modifications occurred. Add a
validation check before this print statement to verify that the count variable n
is greater than 0; if n equals 0, the script should exit with an error code to
indicate that no gitlab servers were found or modified in the config, rather
than printing a misleading success message.

PY
53 changes: 42 additions & 11 deletions index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4464,7 +4464,8 @@ async function createOrUpdateFile(
branch: string,
previousPath?: string,
last_commit_id?: string,
commit_id?: string
commit_id?: string,
encoding?: "text" | "base64"
): Promise<GitLabCreateUpdateFileResponse> {
projectId = decodeURIComponent(projectId); // Decode project ID
const encodedPath = encodeURIComponent(filePath);
Expand All @@ -4474,9 +4475,11 @@ async function createOrUpdateFile(

const body: Record<string, any> = {
branch,
content: encodeRepoFilePayloadContent(content),
// An explicit per-call encoding wins (text or base64, content sent as-is);
// when omitted, fall back to the global text/base64 convenience toggle.
content: encoding !== undefined ? content : encodeRepoFilePayloadContent(content),
commit_message: commitMessage,
encoding: GITLAB_REPO_FILE_ENCODING,
encoding: encoding ?? GITLAB_REPO_FILE_ENCODING,
...(previousPath ? { previous_path: previousPath } : {}),
};

Expand Down Expand Up @@ -4558,12 +4561,33 @@ async function createCommit(
body: JSON.stringify({
branch,
commit_message: message,
actions: actions.map(action => ({
action: "create",
file_path: action.path,
content: encodeRepoFilePayloadContent(action.content),
encoding: GITLAB_REPO_FILE_ENCODING,
})),
actions: actions.map(action => {
// Honour per-file action (create/update/delete/move) instead of
// hardcoding "create", and pass base64 content through untouched so
// binary files commit correctly.
const act = action.action ?? "create";
const entry: Record<string, any> = { action: act, file_path: action.path };
if (act === "move" && action.previous_path) {
entry.previous_path = action.previous_path;
}
if (act !== "delete") {
const hasContent = action.content !== undefined;
// A `move` with no content must keep the original file's content — omit the
// content field entirely rather than blanking it with "".
if (!(act === "move" && !hasContent)) {
if (action.encoding !== undefined) {
// explicit text or base64 — send content as-is
entry.content = action.content ?? "";
entry.encoding = action.encoding;
} else {
// fall back to the global text/base64 convenience toggle
entry.content = encodeRepoFilePayloadContent(action.content ?? "");
entry.encoding = GITLAB_REPO_FILE_ENCODING;
}
Comment on lines +4574 to +4586

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent silent empty-file writes when content is omitted for create/update.

At Line 4580 and Line 4584, missing action.content is coerced to "". For action: "create"/"update", this can unintentionally create or overwrite files with empty content instead of failing fast.

Suggested fix
         if (act !== "delete") {
           const hasContent = action.content !== undefined;
           // A `move` with no content must keep the original file's content — omit the
           // content field entirely rather than blanking it with "".
           if (!(act === "move" && !hasContent)) {
+            if (!hasContent && (act === "create" || act === "update")) {
+              throw new Error(`content is required for action='${act}' on file '${action.path}'`);
+            }
             if (action.encoding !== undefined) {
               // explicit text or base64 — send content as-is
-              entry.content = action.content ?? "";
+              entry.content = action.content;
               entry.encoding = action.encoding;
             } else {
               // fall back to the global text/base64 convenience toggle
-              entry.content = encodeRepoFilePayloadContent(action.content ?? "");
+              entry.content = encodeRepoFilePayloadContent(action.content as string);
               entry.encoding = GITLAB_REPO_FILE_ENCODING;
             }
           }
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@index.ts` around lines 4574 - 4586, The code at lines 4580 and 4584 uses the
nullish coalescing operator to default action.content to an empty string when it
is undefined. For create and update actions, this silently allows empty files to
be written instead of failing when content is missing. Add validation earlier in
the function to ensure that action.content is provided (not undefined) for
create and update actions, and only permit the empty-string default for move
actions where it is intentionally handled to preserve the original file's
content.

}
}
return entry;
}),
}),
});

Expand Down Expand Up @@ -9120,7 +9144,8 @@ async function handleToolCall(params: any) {
args.branch,
args.previous_path,
args.last_commit_id,
args.commit_id
args.commit_id,
args.encoding
);
return {
content: [{ type: "text", text: JSON.stringify(result, null, 2) }],
Expand All @@ -9133,7 +9158,13 @@ async function handleToolCall(params: any) {
args.project_id,
args.commit_message,
args.branch,
args.files.map(f => ({ path: f.file_path, content: f.content }))
args.files.map(f => ({
path: f.file_path,
content: f.content,
action: f.action,
encoding: f.encoding,
previous_path: f.previous_path,
}))
);
return {
content: [{ type: "text", text: JSON.stringify(result, null, 2) }],
Expand Down
26 changes: 22 additions & 4 deletions schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,12 @@ export const GitLabContentSchema = z.union([
// Operation schemas
export const FileOperationSchema = z.object({
path: z.string(),
content: z.string(),
content: z.string().optional(),
// Per-file action + encoding so push_files can update/delete/move and carry
// binary files (base64). Defaults preserve existing behaviour.
action: z.enum(["create", "update", "delete", "move"]).optional(),
encoding: z.enum(["text", "base64"]).optional(),
previous_path: z.string().optional(),
});
Comment on lines 823 to 831

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Schema extension for per-file actions and encoding looks good.

The additions to FileOperationSchema correctly support the new functionality:

  • Making content optional enables delete operations
  • The action enum covers all commit operations (create/update/delete/move)
  • The encoding enum enables binary file handling via base64
  • previous_path supports move/rename operations

All fields are optional with sensible defaults, maintaining backward compatibility as stated in the PR objectives.

💡 Optional: Consider adding schema-level validation for field combinations

While the current implementation likely validates at the handler level (tests confirm this), adding Zod refinements would catch invalid combinations earlier and make the schema self-documenting:

-});
+}).refine(
+  (data) => {
+    // Require previous_path when action is 'move'
+    if (data.action === 'move' && !data.previous_path) {
+      return false;
+    }
+    return true;
+  },
+  { message: "action='move' requires previous_path to be specified" }
+);

This would provide clearer error messages at the schema validation stage rather than deferring to API-level errors.

📝 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.

Suggested change
export const FileOperationSchema = z.object({
path: z.string(),
content: z.string(),
content: z.string().optional(),
// Per-file action + encoding so push_files can update/delete/move and carry
// binary files (base64). Defaults preserve existing behaviour.
action: z.enum(["create", "update", "delete", "move"]).optional(),
encoding: z.enum(["text", "base64"]).optional(),
previous_path: z.string().optional(),
});
export const FileOperationSchema = z.object({
path: z.string(),
content: z.string().optional(),
// Per-file action + encoding so push_files can update/delete/move and carry
// binary files (base64). Defaults preserve existing behaviour.
action: z.enum(["create", "update", "delete", "move"]).optional(),
encoding: z.enum(["text", "base64"]).optional(),
previous_path: z.string().optional(),
}).refine(
(data) => {
// Require previous_path when action is 'move'
if (data.action === 'move' && !data.previous_path) {
return false;
}
return true;
},
{ message: "action='move' requires previous_path to be specified" }
);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@schemas.ts` around lines 823 - 831, Add Zod refinements to the
FileOperationSchema to validate field combinations based on the action type.
This will catch invalid combinations at schema validation time rather than
deferring to handler-level validation. Use .refine() or .superRefine() on the
FileOperationSchema object to add constraints such as: when action is "delete",
content should not be present; when action is "move", previous_path must be
provided; when action is "create", content should likely be required; and when
action is "update", previous_path should not be present. This makes the schema
self-documenting and provides clearer error messages during validation.


// Tree and commit schemas
Expand Down Expand Up @@ -1473,6 +1478,10 @@ export const CreateOrUpdateFileSchema = ProjectParamsSchema.extend({
previous_path: z.string().optional().describe("Path of the file to move/rename"),
last_commit_id: z.string().optional().describe("Last known file commit ID"),
commit_id: z.string().optional().describe("Current file commit ID (for update operations)"),
encoding: z
.enum(["text", "base64"])
.optional()
.describe("Content encoding. Use 'base64' for binary files (content must already be base64-encoded); defaults to text."),
});

export const SearchRepositoriesSchema = z
Expand Down Expand Up @@ -1544,11 +1553,20 @@ export const PushFilesSchema = ProjectParamsSchema.extend({
files: z
.array(
z.object({
file_path: z.string().describe("Path where to create the file"),
content: z.string().describe("Content of the file"),
file_path: z.string().describe("Path of the file in the repo"),
content: z.string().optional().describe("File content (base64-encoded when encoding='base64'; omit for action='delete')"),
action: z
.enum(["create", "update", "delete", "move"])
.optional()
.describe("Commit action for this file. Defaults to 'create'."),
encoding: z
.enum(["text", "base64"])
.optional()
.describe("Use 'base64' for binary files (content must already be base64-encoded); defaults to text."),
previous_path: z.string().optional().describe("Source path when action='move'"),
})
)
.describe("Array of files to push"),
.describe("Array of files to commit in a single commit (create/update/delete/move, text or binary)"),
commit_message: z.string().describe("Commit message"),
});

Expand Down
110 changes: 108 additions & 2 deletions test/schema-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import {
GetFileContentsSchema,
PushFilesSchema,
CreateOrUpdateFileSchema,
GitLabFileContentSchema,
GitLabRepositorySchema,
CreatePipelineSchema,
Expand Down Expand Up @@ -156,6 +158,109 @@ function runGetFileContentsSchemaTests(): { passed: number; failed: number } {
return { passed, failed };
}

function runPushFilesSchemaTests(): { passed: number; failed: number } {
console.log('🧪 Testing PushFilesSchema / CreateOrUpdateFileSchema (per-file action + encoding)...');

interface Case {
name: string;
input: Record<string, any>;
shouldFail?: boolean;
check?: (data: any) => boolean;
}

const base = { project_id: '1', branch: 'main', commit_message: 'm' };
const cases: Case[] = [
{
name: 'schema:push_files:plain-text-default',
input: { ...base, files: [{ file_path: 'a.txt', content: 'hi' }] },
check: d => d.files[0].action === undefined && d.files[0].encoding === undefined,
},
{
name: 'schema:push_files:binary-base64',
input: { ...base, files: [{ file_path: 'logo.png', content: 'aGk=', encoding: 'base64' }] },
check: d => d.files[0].encoding === 'base64',
},
{
name: 'schema:push_files:action-update',
input: { ...base, files: [{ file_path: 'a.txt', content: 'new', action: 'update' }] },
check: d => d.files[0].action === 'update',
},
{
name: 'schema:push_files:action-delete-content-optional',
input: { ...base, files: [{ file_path: 'a.txt', action: 'delete' }] },
check: d => d.files[0].action === 'delete' && d.files[0].content === undefined,
},
{
name: 'schema:push_files:action-move-previous-path',
input: { ...base, files: [{ file_path: 'b.txt', action: 'move', previous_path: 'a.txt' }] },
check: d => d.files[0].action === 'move' && d.files[0].previous_path === 'a.txt',
},
{
// schema permits move without previous_path (GitLab's API validates it);
// this documents that contract.
name: 'schema:push_files:move-without-previous-path-parses',
input: { ...base, files: [{ file_path: 'b.txt', action: 'move' }] },
check: d => d.files[0].action === 'move' && d.files[0].previous_path === undefined,
},
{
name: 'schema:push_files:explicit-text-encoding',
input: { ...base, files: [{ file_path: 'a.txt', content: 'hi', encoding: 'text' }] },
check: d => d.files[0].encoding === 'text',
},
{
name: 'schema:push_files:reject-bad-action',
input: { ...base, files: [{ file_path: 'a.txt', content: 'x', action: 'frobnicate' }] },
shouldFail: true,
},
{
name: 'schema:push_files:reject-bad-encoding',
input: { ...base, files: [{ file_path: 'a.txt', content: 'x', encoding: 'rot13' }] },
shouldFail: true,
},
{
name: 'schema:create_or_update_file:encoding-base64',
input: { project_id: '1', file_path: 'logo.png', content: 'aGk=', commit_message: 'm', branch: 'main', encoding: 'base64' },
check: d => d.encoding === 'base64',
},
];

let passed = 0;
let failed = 0;

cases.forEach(testCase => {
const result: TestResult = { name: testCase.name, status: 'failed' };
const schema = testCase.name.startsWith('schema:create_or_update_file')
? CreateOrUpdateFileSchema
: PushFilesSchema;
const parsed = schema.safeParse(testCase.input);

if (testCase.shouldFail) {
result.status = parsed.success ? 'failed' : 'passed';
if (parsed.success) result.error = 'Expected schema validation to fail';
} else if (parsed.success) {
if (!testCase.check || testCase.check(parsed.data)) {
result.status = 'passed';
} else {
result.error = `Unexpected parsed result: ${JSON.stringify(parsed.data)}`;
}
} else {
result.error = parsed.error?.message || 'Schema validation failed';
}

if (result.status === 'passed') {
passed++;
console.log(`✅ ${result.name}`);
} else {
failed++;
console.log(`❌ ${result.name}: ${result.error}`);
}
});

console.log(`\nResults: ${passed} passed, ${failed} failed`);

return { passed, failed };
}
Comment on lines +161 to +262

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Consider adding tests for move action validation and content requirements.

The current test suite covers core scenarios well, but would benefit from a few additional edge cases:

  1. Move without previous_path: Add a test to verify whether move action fails or succeeds when previous_path is omitted. This clarifies the schema contract.

  2. Content requirement for create/update: Add tests verifying that content is required (or at least accepted) for create and update actions, not just delete.

  3. Explicit action 'create': Test explicitly setting action: 'create' rather than relying only on the default behavior.

These tests would strengthen validation coverage and help catch schema definition bugs.

✨ Example additional test cases
     {
       name: 'schema:push_files:action-move-previous-path',
       input: { ...base, files: [{ file_path: 'b.txt', action: 'move', previous_path: 'a.txt' }] },
       check: d => d.files[0].action === 'move' && d.files[0].previous_path === 'a.txt',
     },
+    {
+      name: 'schema:push_files:action-move-requires-previous-path',
+      input: { ...base, files: [{ file_path: 'b.txt', action: 'move' }] },
+      shouldFail: true,  // or false, depending on schema design
+    },
+    {
+      name: 'schema:push_files:action-create-explicit',
+      input: { ...base, files: [{ file_path: 'a.txt', content: 'hi', action: 'create' }] },
+      check: d => d.files[0].action === 'create',
+    },
+    {
+      name: 'schema:push_files:create-requires-content',
+      input: { ...base, files: [{ file_path: 'a.txt', action: 'create' }] },
+      shouldFail: true,  // verify content is required for create
+    },
     {
       name: 'schema:push_files:reject-bad-action',
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/schema-tests.ts` around lines 161 - 250, The test suite in the
runPushFilesSchemaTests() function lacks coverage for important edge cases
around move actions and content requirements. Add three new test cases to the
cases array: first, a test case for move action without previous_path to verify
the schema properly rejects or accepts this scenario; second, test cases that
verify content is required or properly handled for create and update actions
(not just delete); and third, a test case that explicitly sets action to
'create' rather than relying on default behavior. Each new test case should
follow the existing Case interface pattern with appropriate name, input,
shouldFail flag, and check function as needed.


function runGitLabFileContentSchemaTests(): { passed: number; failed: number } {
console.log('\n🧪 Testing GitLabFileContentSchema...');

Expand Down Expand Up @@ -1658,6 +1763,7 @@ function runGitLabDependencyProxyBlobSchemaTests(): { passed: number; failed: nu

if (import.meta.url === `file://${process.argv[1]}`) {
const getFileContentsResult = runGetFileContentsSchemaTests();
const pushFilesResult = runPushFilesSchemaTests();
const fileContentResult = runGitLabFileContentSchemaTests();
const createPipelineResult = runCreatePipelineSchemaTests();
const commitStatusResult = runCommitStatusSchemaTests();
Expand All @@ -1677,8 +1783,8 @@ if (import.meta.url === `file://${process.argv[1]}`) {
const dependencyProxyResult = runGitLabDependencyProxySchemaTests();
const dependencyProxyBlobResult = runGitLabDependencyProxyBlobSchemaTests();

const totalPassed = getFileContentsResult.passed + fileContentResult.passed + createPipelineResult.passed + commitStatusResult.passed + createIssueNoteResult.passed + getMergeRequestResult.passed + listMergeRequestPipelinesResult.passed + gitLabMergeRequestResult.passed + emojiReactionResult.passed + repositorySchemaResult.passed + labelsCoercionResult.passed + approvedByUsernamesResult.passed + listLabelsResult.passed + treeItemResult.passed + repositoryTreeResult.passed + gitLabUserFullResult.passed + gitLabMarkdownUploadResult.passed + dependencyProxyResult.passed + dependencyProxyBlobResult.passed;
const totalFailed = getFileContentsResult.failed + fileContentResult.failed + createPipelineResult.failed + commitStatusResult.failed + createIssueNoteResult.failed + getMergeRequestResult.failed + listMergeRequestPipelinesResult.failed + gitLabMergeRequestResult.failed + emojiReactionResult.failed + repositorySchemaResult.failed + labelsCoercionResult.failed + approvedByUsernamesResult.failed + listLabelsResult.failed + treeItemResult.failed + repositoryTreeResult.failed + gitLabUserFullResult.failed + gitLabMarkdownUploadResult.failed + dependencyProxyResult.failed + dependencyProxyBlobResult.failed;
const totalPassed = getFileContentsResult.passed + pushFilesResult.passed + fileContentResult.passed + createPipelineResult.passed + commitStatusResult.passed + createIssueNoteResult.passed + getMergeRequestResult.passed + listMergeRequestPipelinesResult.passed + gitLabMergeRequestResult.passed + emojiReactionResult.passed + repositorySchemaResult.passed + labelsCoercionResult.passed + approvedByUsernamesResult.passed + listLabelsResult.passed + treeItemResult.passed + repositoryTreeResult.passed + gitLabUserFullResult.passed + gitLabMarkdownUploadResult.passed + dependencyProxyResult.passed + dependencyProxyBlobResult.passed;
const totalFailed = getFileContentsResult.failed + pushFilesResult.failed + fileContentResult.failed + createPipelineResult.failed + commitStatusResult.failed + createIssueNoteResult.failed + getMergeRequestResult.failed + listMergeRequestPipelinesResult.failed + gitLabMergeRequestResult.failed + emojiReactionResult.failed + repositorySchemaResult.failed + labelsCoercionResult.failed + approvedByUsernamesResult.failed + listLabelsResult.failed + treeItemResult.failed + repositoryTreeResult.failed + gitLabUserFullResult.failed + gitLabMarkdownUploadResult.failed + dependencyProxyResult.failed + dependencyProxyBlobResult.failed;

console.log(`\nTotal Results: ${totalPassed} passed, ${totalFailed} failed`);

Expand Down