feat: binary (base64) commits and per-file actions in push_files#513
feat: binary (base64) commits and per-file actions in push_files#513hodyhq wants to merge 2 commits into
Conversation
push_files hardcoded `action: "create"`, so it errored on existing files and could not delete or move them, and all content was treated as text — making binary commits (images, etc.) impossible. This adds optional per-file `action` (create/update/delete/move) and `encoding` (text/base64) to push_files, and an `encoding` option to create_or_update_file. base64 content is passed through untouched (binary-safe); `content` may be omitted for deletes. Defaults preserve existing behaviour. Adds schema tests.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds optional per-file ChangesPer-file encoding and action support
Fork activation utility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with 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.
Inline comments:
In `@index.ts`:
- Around line 4478-4482: The encoding field uses a ternary operator that only
explicitly handles the "base64" case, causing any other encoding value
(including explicit "text") to fall back to GITLAB_REPO_FILE_ENCODING. To fix
this, update the encoding assignment to explicitly check for three cases: if
encoding is "base64" use "base64", if encoding is "text" use "text", otherwise
fall back to GITLAB_REPO_FILE_ENCODING. This ensures per-file "text" encoding
requests are respected and not overridden by the global setting.
- Around line 4573-4579: The code applies a default empty string to
action.content for all non-delete operations, but for move operations this
unintentionally overwrites file content when content is not provided. Modify the
condition to exclude move operations from this empty string default behavior.
Check if the action is not "move" (in addition to checking if act is not
"delete") before applying the empty string default in both the base64 encoding
branch and the regular encoding branch where encodeRepoFilePayloadContent is
called. This ensures that for move/rename operations without explicit content,
the existing file content is preserved instead of being replaced with an empty
string.
In `@schemas.ts`:
- Around line 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.
In `@test/schema-tests.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 452a1f62-354a-40a0-b0d0-5dc732d0ddf4
📒 Files selected for processing (3)
index.tsschemas.tstest/schema-tests.ts
📜 Review details
🔇 Additional comments (5)
schemas.ts (2)
1481-1484: LGTM!
1556-1569: Per-file schema updates are well-structured and documented.The
PushFilesSchema.files[]element schema correctly implements the per-file action and encoding support described in the PR objectives. The updated descriptions clearly communicate:
- When to omit
content(action='delete')- Base64-encoding requirement (encoding='base64')
- Move operation requirements (previous_path)
The schema structure is consistent with
FileOperationSchema, ensuring uniform behavior across single-file and multi-file operations.index.ts (1)
9130-9142: LGTM!Also applies to: 9154-9160
test/schema-tests.ts (2)
5-6: LGTM!
1754-1754: LGTM!Also applies to: 1774-1775
| 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(), | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Schema extension for per-file actions and encoding looks good.
The additions to FileOperationSchema correctly support the new functionality:
- Making
contentoptional enables delete operations - The
actionenum covers all commit operations (create/update/delete/move) - The
encodingenum enables binary file handling via base64 previous_pathsupports 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.
| 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.
| 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', | ||
| }, | ||
| { | ||
| 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 }; | ||
| } |
There was a problem hiding this comment.
🧹 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:
-
Move without
previous_path: Add a test to verify whether move action fails or succeeds whenprevious_pathis omitted. This clarifies the schema contract. -
Content requirement for create/update: Add tests verifying that
contentis required (or at least accepted) forcreateandupdateactions, not justdelete. -
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.
Addresses review feedback: - explicit encoding:"text" now takes effect even when the global GITLAB_REPO_FILE_ENCODING is base64 (was falling through to the global). - move without content no longer sends content:"" (which would blank the moved file); the content field is omitted so GitLab preserves the original. - adds schema tests for explicit text encoding and move-without-previous_path.
|
Thanks for the review! Addressed in the latest commit:
Schema suite: 111 passed / 0 failed, build clean. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with 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.
Inline comments:
In `@activate-fork.sh`:
- 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.
- 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.
- Around line 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.
- 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.
- 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.
- Around line 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.
In `@index.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6173106-f906-4014-9a0f-e22416f75fcc
📒 Files selected for processing (3)
activate-fork.shindex.tstest/schema-tests.ts
📜 Review details
🔇 Additional comments (4)
index.ts (1)
4478-4482: LGTM!test/schema-tests.ts (1)
198-209: LGTM!activate-fork.sh (2)
14-15: LGTM!
17-17: LGTM!
| # bash ~/.dev/Projects/gitlab-mcp/activate-fork.sh # activate fork | ||
| # bash ~/.dev/Projects/gitlab-mcp/activate-fork.sh --revert # back to upstream npx |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
| 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.
|
|
||
| CFG="$HOME/.claude.json" | ||
| BUILD="/home/hody/.dev/Projects/gitlab-mcp/build/index.js" | ||
| MODE="${1:-activate}" |
There was a problem hiding this comment.
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.
| 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.
| 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) |
There was a problem hiding this comment.
🧹 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.
| n += 1 | ||
| for v in o.values(): walk(v) | ||
| walk(d) | ||
| json.dump(d, open(cfg, "w"), indent=2); open(cfg, "a").write("\n") |
There was a problem hiding this comment.
🧹 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.
| for v in o.values(): walk(v) | ||
| walk(d) | ||
| json.dump(d, open(cfg, "w"), indent=2); open(cfg, "a").write("\n") | ||
| print(f"{'reverted' if mode=='revert' else 'activated fork on'} {n} gitlab server(s). Backup saved. Now relaunch Claude Code.") |
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
zereight
left a comment
zereight
left a comment
There was a problem hiding this comment.
Thanks for the push_files work — per-file actions and base64 support are a real gap today, and the follow-up commit addressed the encoding/move issues from the first review round.
Before merge, a few things still need attention:
activate-fork.sh — This looks like a local fork-testing helper (hardcoded /home/hody/... path, Claude Code config editing). It shouldn't ship in the upstream repo. Please drop it from the PR.
Rebase onto current main — index.ts and schemas.ts both conflict with recent changes on main. Please rebase and resolve.
Empty content on create/update — The move-without-content case is handled correctly now, but create/update still coerces missing content to "", which can silently write an empty file. Either reject that at schema/runtime or document it clearly.
Docs — docs/tools/repositories.md still shows the old push_files shape. Run make tools-docs after the schema changes and commit the regenerated pages.
Tests — The new schema cases in test/schema-tests.ts are good, but there's no coverage for the actual createCommit payload mapping (base64 passthrough, explicit encoding: "text" vs global base64, delete/move omitting content). A small unit test around that mapping would help.
Happy to take another look once these are sorted.
2 participants
What & why
push_filescurrently hardcodesaction: "create"for every file, so it:I hit all three building a small project with this MCP (committing PNG logos and updating files in one commit).
Changes
push_files— each file entry gains optional fields:action:create|update|delete|move(defaultcreate)encoding:text|base64— base64 content is passed through untouched (binary-safe)previous_path(formove);contentis now optional (omit fordelete)create_or_update_file— gains optionalencoding: "base64"for binary files.Defaults preserve existing behaviour — the global
GITLAB_REPO_FILE_ENCODINGtoggle still works, and text commits are unchanged. The base64 path sends content straight to GitLab's commits/files API withencoding: "base64"instead of double-encoding it.Tests
Added
PushFilesSchema/CreateOrUpdateFileSchemacases totest/schema-tests.ts(action / encoding / optional-content / move, plus rejection of invalidaction/encoding).npm run buildis clean and the schema suite reports 109 passed, 0 failed.Also verified end-to-end against a live GitLab: a PNG committed via
push_files(encoding: "base64") round-trips byte-for-byte (sha256 match), andupdate/deleteactions work as expected.