feat: add snippet CRUD tools#470
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e5f431e08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (args.include_content) { | ||
| const files = snippet.files ?? []; | ||
| if (files.length > 1) { | ||
| const ref = snippet.default_branch ?? "main"; |
There was a problem hiding this comment.
When get_snippet is called with include_content=true for multi-file snippets, the code falls back to "main" if default_branch is absent, but the GitLab raw-file endpoint requires the correct :ref and the snippet payload does not guarantee a default_branch field. On instances/projects that still use another default branch (for example master) this produces 404s and breaks content retrieval for otherwise valid snippets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
addressed in the last commit 🙇
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee5e4724ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const encodedRef = ref.split("/").map(encodeURIComponent).join("/"); | ||
| const encodedPath = filePath.split("/").map(encodeURIComponent).join("/"); | ||
| const url = `${getSnippetsEndpoint(projectId)}/${snippetId}/files/${encodedRef}/${encodedPath}/raw`; |
There was a problem hiding this comment.
When building the /files/:ref/:file_path/raw URL, ref is encoded segment-by-segment and rejoined with /, which leaves slashes unescaped (for example, feature/foo stays feature/foo). In that case GitLab parses only feature as :ref and shifts the rest into :file_path, so get_snippet(include_content=true) can return 404 or wrong content for multi-file snippets whose ref contains / (common for branch names). Encode ref as one value (encodeURIComponent(ref)) before inserting it into the path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Argh yeah it's obvious, my bad (also why is my code reviewer not getting it? =/). Gonna handle it.
renanliberato
force-pushed
the
feat/snippets
branch
from
ee5e472 to
4ff40ac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ff40ace3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| filePath: string | ||
| ): Promise<string> { | ||
| const encodedRef = encodeURIComponent(ref); | ||
| const encodedPath = filePath.split("/").map(encodeURIComponent).join("/"); |
There was a problem hiding this comment.
When fetching multi-file snippet content, filePath is encoded per segment and then rejoined with /, so directory separators remain unescaped. GitLab’s snippets file-content endpoint expects :file_path to be URL-encoded as one value; for files under subdirectories (for example docs/policy.md), this can cause 404s or path misrouting because the slash is treated as a path delimiter instead of part of file_path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for adding the snippet toolset. I found a few API-contract issues that should be fixed before merging.
- Nested multi-file snippet content fetch can 404
getSnippetFileRawContent() encodes the file path per path segment:
filePath.split("/").map(encodeURIComponent).join("/")
For a nested snippet file like dir/policy.md, the request stays as /files/main/dir/policy.md/raw. GitLab's snippets API expects file_path to be a URL-encoded path for the /files/:ref/:file_path/raw endpoint, so nested paths can be routed incorrectly or return 404.
Suggested fix: encode the entire path with encodeURIComponent(filePath) and add a test with a nested file path such as dir/policy.md.
- URL-encoded project paths are double-encoded
getSnippetsEndpoint() accepts project_id values described as either project IDs or URL-encoded paths, but it passes the value directly through encodeURIComponent. With project_id=group%2Fproject, the endpoint becomes /projects/group%252Fproject/snippets instead of the intended project path.
Suggested fix: follow the existing project endpoint pattern and decode before re-encoding, e.g. decodeURIComponent(projectId) before getEffectiveProjectId/encodeURIComponent.
- Project snippet responses may fail schema parsing
GitLabSnippetSchema requires visibility, but the Project Snippets API response shape does not consistently document/return that field. A valid project snippet response without visibility would fail with a ZodError across list/get/create/update paths.
Suggested fix: make visibility optional or split personal/project snippet response schemas.
update_snippetdoes not expose the modern multi-file update contract
create_snippet supports files[], but update_snippet only exposes the legacy-ish file_name + content shape and converts it to a single action: "update" file. GitLab supports multi-file update actions such as create/update/delete/move with previous_path. As written, users can create multi-file snippets but cannot fully update/move/delete files through this tool.
Suggested fix: add a files[] update shape with action and previous_path support, while keeping file_name/content as a backward-compatible shortcut if desired.
Adds list_snippets, get_snippet, create_snippet, update_snippet, and delete_snippet — supporting both project-scoped and personal snippets via optional project_id. Registered under a new non-default "snippets" toolset. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The multi-file content endpoint requires a ref to construct the API URL.
Using default_branch caused 404s on instances where the default branch
is not "main"; fetching raw_url directly also 404s because it is a web
URL, not an API endpoint.
GitLab embeds the correct ref inside each file's raw_url
(.../raw/{ref}/{path}). Parse it out and pass it to the REST API
endpoint (/snippets/{id}/files/{ref}/{path}/raw) instead.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
getSnippetFileRawContent now takes an explicit ref string instead of a raw_url to parse. The ref-extraction logic is factored into a standalone extractSnippetRef helper, resolved once at the call site rather than once per file inside the fetch function. get_snippet also gains an optional ref parameter (matching get_file_contents) so callers can override the ref directly without relying on raw_url parsing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Nested multi-file paths now encode the full file_path as a single segment (encodeURIComponent) per GitLab's /files/:ref/:file_path/raw contract. Previously, paths like dir/policy.md kept the slash and 404'd. - getSnippetsEndpoint decodes URL-encoded project paths before re-encoding, matching the existing pattern in merge_requests/issues. Eliminates double-encoded URLs like /projects/group%252Fproject/snippets. - GitLabSnippetSchema.visibility is now optional — the project snippets API does not consistently return it and ZodError was breaking list/get/create/ update for valid responses. - update_snippet exposes the modern multi-file contract: files[] with action (create/update/delete/move) and previous_path. The file_name+content shortcut remains for backward compatibility and is mutually exclusive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a34d885f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!firstFile.raw_url) throw new Error(`Snippet file "${firstFile.path}" has no raw_url`); | ||
| const ref = args.ref ?? extractSnippetRef(firstFile.raw_url, args.snippet_id, firstFile.path); |
There was a problem hiding this comment.
get_snippet(include_content=true) for multi-file snippets currently throws if files[0].raw_url is missing before checking args.ref, so requests that explicitly pass ref still fail even though no raw_url parsing is needed. This breaks valid calls on snippet payloads where raw_url is omitted/empty but path is present, and prevents content retrieval despite having all required inputs (snippet_id, ref, file_path).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This still looks applicable at the latest head. args.ref is only used after the raw_url guard, so a caller who explicitly provides ref can still be rejected when the snippet file object has no raw_url.
Could we only require/parse raw_url when args.ref is absent? The effective ref could be const ref = args.ref ?? extractSnippetRef(firstFile.raw_url) with the raw_url check inside the fallback branch.
renanliberato
force-pushed
the
feat/snippets
branch
from
8a34d88 to
a5314e8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5314e83f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| previous_path: z | ||
| .string() | ||
| .optional() | ||
| .describe("Previous path of the snippet file (required for 'move')."), | ||
| content: z |
There was a problem hiding this comment.
SnippetFileUpdateActionSchema declares previous_path as optional even though this same schema describes it as required for action: "move", so update_snippet currently accepts payloads like {action:"move", file_path:"new.md"} and only fails later with a GitLab 400. This weakens tool-level validation and makes LLM-generated update requests brittle, because malformed move operations are not rejected at parse time even though the required field is known locally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
oh, I didn't know zod had a way to handle it, but indeed has. Addressed.
SnippetFileUpdateActionSchema had previous_path optional even though the schema description said it was required for action: "move". That left malformed move actions to fail late with a GitLab 400 instead of being caught at parse time. Adds a superRefine that surfaces the constraint as a Zod error so LLM-generated update requests get a clear, local rejection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68b4aa690a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const effectiveProjectId = getEffectiveProjectId(decoded); | ||
| return `${getEffectiveApiUrl()}/projects/${encodeURIComponent(effectiveProjectId)}/snippets`; | ||
| } | ||
| return `${getEffectiveApiUrl()}/snippets`; |
There was a problem hiding this comment.
When project_id is omitted, getSnippetsEndpoint always returns /snippets, which bypasses getEffectiveProjectId and therefore skips GITLAB_ALLOWED_PROJECT_IDS enforcement. In restricted deployments, a caller can still invoke get_snippet/update_snippet/delete_snippet against global snippet IDs and reach snippets outside the configured project allowlist if the token can access them, which defeats the project-scoping guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@zereight , could you enlighten me with any project standards? Not sure if other API besides Snippets have this "User or Project" behaviour. I'm not sure if an env var about projects should have influence over user snippets too.
There was a problem hiding this comment.
I agree with this concern. In this repo, GITLAB_PROJECT_ID and GITLAB_ALLOWED_PROJECT_IDS are used as project-scope access boundaries, not just defaults. When either restriction is configured, calls that omit project_id should not fall back to the global /snippets endpoint, because that can read, update, or delete personal/global snippets outside the configured project scope.
I would either reject omitted project_id while project scoping is active, or explicitly default to the single configured project if that is the intended behavior. Personal snippets should only be available when no project-scope restriction is configured.
| .superRefine((data, ctx) => { | ||
| if (data.action === "move" && data.previous_path === undefined) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: "previous_path is required for action: 'move'", | ||
| path: ["previous_path"], | ||
| }); | ||
| } |
There was a problem hiding this comment.
SnippetFileUpdateActionSchema only enforces previous_path for move, but it still accepts invalid actions like {"action":"create","file_path":"a.md"} (missing content) or {"action":"delete"} (missing file_path). Those payloads pass local validation and then fail with GitLab 400s, making tool behavior brittle and inconsistent with the schema descriptions that mark these fields as required for specific actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
hm, not sure how much exhaustive I should be on this. Any opinion @zereight ?
There was a problem hiding this comment.
I would keep this validation exhaustive. Since files[] is already modeling the action-specific GitLab contract, malformed actions should fail locally before becoming GitLab 400s.
A small matrix should be enough:
- every action requires
file_path createandupdaterequirecontentmoverequires bothprevious_pathand the newfile_pathdeleterequiresfile_pathand does not needcontentfiles[]should probably be non-empty when provided
That keeps LLM-generated tool calls predictable and matches the project pattern of surfacing schema errors before upstream API errors when the requirement is known locally.
zereight
left a comment
zereight
left a comment
There was a problem hiding this comment.
Thanks for the follow-up fixes. The original API-contract issues I raised are mostly addressed now: nested snippet file paths are encoded correctly, URL-encoded project paths are decoded before re-encoding, visibility is no longer required for project snippet responses, and update_snippet now exposes the modern files[] update shape. The new test/test-snippets.ts file is also wired into test:mock.
I’m refreshing the request-changes baseline because a few issues remain:
-
Project-scoped deployments still need a guard for omitted
project_id. WhenGITLAB_PROJECT_IDorGITLAB_ALLOWED_PROJECT_IDSis configured, falling back to the global/snippetsendpoint bypasses the project boundary. I replied on the existing thread with the expected project-standard behavior. -
get_snippet(include_content=true, ref=...)still requiresraw_urlbefore it checks the explicitref, so valid calls can fail when the snippet file payload haspathbut noraw_url. I replied on the existing thread with the minimal shape. -
SnippetFileUpdateActionSchemashould validate the action-specific required fields locally:file_pathfor every action,contentfor create/update,previous_pathfor move, and a non-emptyfiles[]array when provided. I replied on the existing schema thread with the matrix.
Once these are addressed, this should be close to merge-ready.
get_snippet(include_content=true) previously threw if files[0].raw_url was missing, even when the caller passed ref explicitly. Flip the guard so raw_url is only required as a fallback for inferring ref.
getSnippetsEndpoint used to fall back to the global /snippets endpoint whenever project_id was omitted, bypassing GITLAB_PROJECT_ID and GITLAB_ALLOWED_PROJECT_IDS access boundaries. Route omitted project_id through getEffectiveProjectId when either scope env is set, reusing its existing default-when-unambiguous, reject-when-ambiguous semantics. Personal snippets remain available only when no scope is configured.
SnippetFileUpdateActionSchema only enforced previous_path for 'move', so malformed actions (create/update without content, delete without file_path) and empty files[] arrays passed local validation and only failed later as GitLab 400s. Expand the superRefine with the full action matrix and require files[] to be non-empty when provided.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79eb6d6f57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result.content = await getSnippetRawContent( | ||
| args.project_id, | ||
| args.snippet_id | ||
| ); |
There was a problem hiding this comment.
get_snippet ignores args.ref when files.length <= 1 and always calls the /raw endpoint, so callers requesting historical content by branch/tag/commit still receive default-branch HEAD content. GitLab exposes .../files/:ref/:file_path/raw for ref-specific reads, and this path is already used for multi-file snippets, so this is a functional mismatch that returns incorrect content whenever include_content=true, ref is set, and the snippet has one file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Oh my, historical snippets content ? Gonna test and address it
2 participants
Adds list_snippets, get_snippet, create_snippet, update_snippet, and delete_snippet — supporting both project-scoped and personal snippets via optional project_id. Registered under a new non-default "snippets" toolset.
It seems well covered and tested:
My first pull request here, let me know anything I missed.