Skip to content

fix: resolve push_files commit action per file - #643

Closed
nikes wants to merge 3 commits into
zereight:mainfrom
nikes:fix/push-files-update-existing
Closed

fix: resolve push_files commit action per file#643
nikes wants to merge 3 commits into
zereight:mainfrom
nikes:fix/push-files-update-existing

Conversation

@nikes

@nikes nikes commented Aug 6, 2026

Copy link
Copy Markdown

What

createCommit built its payload with action: "create" hardcoded for every entry, so
push_files could only ever add files. GitLab's Commits API rejects create for a
path already tracked in the branch:

400 A file with this name already exists

The action is now resolved per file: update when the path exists in the target branch,
create when it does not.

const resolvedActions = await Promise.all(
  actions.map(async action => ({
    action: (await repositoryFileExists(projectId, action.path, branch)) ? "update" : "create",
    ...
  }))
);

repositoryFileExists issues a HEAD against the Repository Files API — headers rather
than the whole blob, since a commit may carry several files and their current content is
irrelevant to the decision. A branch that does not exist yet answers 404 exactly like a
missing file, which is the right answer: a commit that also creates the branch has to use
create.

create_or_update_file already does the same existence probe for a single file, so this
brings push_files in line with it rather than introducing a new pattern.

Why

Any commit touching existing code failed, which makes push_files unusable for the most
common editing workflow — an agent reading a file and writing it back. The error also
reads like a GitLab problem rather than a missing capability, so it is easy to
misdiagnose as a permissions or path issue.

The only working way to change a tracked file today is create_or_update_file, which
handles one file per commit — so a multi-file change cannot be made atomic at all.

No existing issue; happy to open one if you'd prefer to track it separately.

How tested

  • npm run test:mock — 565 passing, including the new suite.
  • npm run test:consumer-smoke — passes.
  • npx tsc --noEmit, npm run build, npm run check:runtime-deps,
    npm run check:skill-sync — all clean.
  • npx tsx scripts/generate-tool-docs.ts — regenerated; docs/tools/ committed with no
    drift.

New file test/test-push-files.ts (mock GitLab server, following the
test-create-repository.ts pattern) covers three cases:

  • a path that already exists in the branch → action: "update"
  • a path that does not → action: "create"
  • a commit mixing both → resolved per file

The first case fails against main, where every action is create.

MockGitLabServer.addMockHandler gained "head" in its method union so the probe can be
stubbed. The dispatcher already keyed on the real request method, so nothing changed at
runtime.

Not run locally: npm run test:live (needs a real GitLab token) and
mkdocs build --strict (no local MkDocs). The docs change is a single generated
description line with no new links.

Breaking changes

None. No schema change, no new parameter. The only behavioural difference is that a call
which previously failed with a 400 now succeeds; callers that only ever pushed new files
are unaffected.

The tool description changes from "Push multiple files in a single commit" to "Create or
update multiple files in a single commit", and docs/tools/ is regenerated to match.

createCommit hardcoded `action: "create"` for every entry, and GitLab
rejects that for a path already tracked in the branch:

    400 A file with this name already exists

So push_files could only ever ADD files. Any commit touching existing
code failed, and the error read like a GitLab problem rather than a
missing capability — create_or_update_file was the only working way to
change a file, one file per commit.

The action is now resolved per file with a HEAD request against the
Repository Files API: `update` when the path exists in the target
branch, `create` when it does not. HEAD keeps the probe to headers
rather than the whole blob, and a missing branch answers 404 like a
missing file — which is correct, since a commit that also creates the
branch has to use `create`.

Adds test/test-push-files.ts covering an existing path, a new path, and
a commit mixing both.
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e2a4fde2-27d6-449c-a396-7ce0d32678ed

📥 Commits

Reviewing files that changed from the base of the PR and between f5da568 and 57719ec.

📒 Files selected for processing (1)
  • test/test-push-files.ts
📜 Recent review details
🔇 Additional comments (2)
test/test-push-files.ts (2)

59-60: LGTM!


159-202: LGTM!


📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • push_files now creates new files and updates existing files in a single commit.
    • Mixed batches automatically apply the correct action to each file.
  • Bug Fixes

    • Existing files are no longer rejected or incorrectly treated as new files.
    • File updates and creations now work reliably together in one operation.
  • Documentation

    • Updated tool descriptions and reference documentation to reflect create-or-update behavior.

Walkthrough

push_files checks each repository path with a HEAD request and selects update or create per file. Integration tests cover existing, missing, mixed, and lookup-error cases. Tool documentation and the changelog describe the new behavior.

Changes

push_files update support

Layer / File(s) Summary
Resolve file actions
index.ts, CHANGELOG.md, README.md, docs/tools/*, tools/registry.ts
createCommit checks each path and submits update for existing files or create for missing files. Documentation describes create-or-update behavior.
Validate mixed file actions
test/test-push-files.ts, test/utils/mock-gitlab-server.ts
Integration tests verify existing, missing, mixed, and lookup-error cases. The mock server accepts HEAD handlers.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant createCommit
  participant repositoryFileExists
  participant GitLabRepositoryFilesAPI
  createCommit->>repositoryFileExists: Check each repository path
  repositoryFileExists->>GitLabRepositoryFilesAPI: Send HEAD request at branch ref
  GitLabRepositoryFilesAPI-->>repositoryFileExists: Return response status
  repositoryFileExists-->>createCommit: Return file existence
  createCommit->>GitLabRepositoryFilesAPI: Submit update or create action
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: resolving the commit action for each file in push_files.
Description check ✅ Passed The description directly explains the bug, implementation, tests, documentation updates, and validation results for the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 4812-4817: Update repositoryFileExists() so the HEAD response
returns false only when response.status is 404, returns true for successful
responses, and throws for every other non-success status. Preserve the original
response/API failure details when propagating the error so callers do not
incorrectly choose the create action.

In `@test/test-push-files.ts`:
- Around line 90-98: Update the HEAD mock handlers in the existingPaths loop to
inspect the request’s ref query parameter and assert it matches the intended
target branch before returning success. Keep the existing encoded file-path
routing and successful response behavior unchanged.
🪄 Autofix

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: 747494bc-fdea-43d1-a4a4-f5b55a560ebf

📥 Commits

Reviewing files that changed from the base of the PR and between 926d42c and f277221.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • README.md
  • docs/tools/index.md
  • docs/tools/repositories.md
  • index.ts
  • test/test-push-files.ts
  • test/utils/mock-gitlab-server.ts
  • tools/registry.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
test/test-push-files.ts

[warning] 2-2: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

🔇 Additional comments (6)
CHANGELOG.md (1)

9-11: LGTM!

README.md (1)

562-562: LGTM!

docs/tools/index.md (1)

77-77: LGTM!

docs/tools/repositories.md (1)

67-67: LGTM!

tools/registry.ts (1)

299-299: LGTM!

test/utils/mock-gitlab-server.ts (1)

78-82: LGTM!

Comment thread index.ts Outdated
Comment thread test/test-push-files.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@test/test-push-files.ts`:
- Around line 94-95: Add a regression case in the push_files tests around the
existing request handler, configuring the HEAD handler to return a non-404
failure such as 403 or 500. Assert that push_files propagates the failure and
does not send the commit request, while preserving the existing behavior where
only a 404 selects create.
🪄 Autofix

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: 49d1fa7c-543a-43ca-a739-009351f25849

📥 Commits

Reviewing files that changed from the base of the PR and between f277221 and f5da568.

📒 Files selected for processing (2)
  • index.ts
  • test/test-push-files.ts
📜 Review details
🔇 Additional comments (1)
index.ts (1)

4817-4822: LGTM!

Comment thread test/test-push-files.ts
@nikes nikes closed this Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant