feat: add dependency proxy toolset#474
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: 0b8bcaf9ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d211c8b42d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2a442089c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| dependencyProxyImageTtlPolicy { enabled ttl } | ||
| } | ||
| }`, | ||
| { fullPath: groupPath } |
There was a problem hiding this comment.
When group_id is a numeric ID or an encoded subgroup path such as my-org%2Fteam, the input schema accepts it, but this sends it unchanged as GraphQL fullPath. GitLab's group(fullPath:) and UpdateDependencyProxySettingsInput.groupPath require the real full path, so the get/list/update dependency proxy tools fail for valid REST-style group identifiers that other group tools support; decode paths or resolve /groups/:id to full_path before issuing GraphQL.
Useful? React with 👍 / 👎.
zereight
left a comment
zereight
left a comment
There was a problem hiding this comment.
Thanks for the update, and 0a2cdc4—the overall direction looks good. The dependency proxy being an opt-in non-default toolset makes sense.
I’m requesting changes for two actionable issues here; there are existing inline comments about the group ID / GraphQL path mismatch that should also be addressed in the same pass.
1. Merge conflict with main
The PR is currently CONFLICTING / DIRTY and cannot merge until it is rebased or conflicts are resolved.
2. New test file is not wired into CI
test/test-dependency-proxy.ts is added but does not appear in the test:mock script in package.json. This repository enumerates test files explicitly, so the new tests will not run under npm test / npm run test:all. Please add it to the test:mock script so the regression coverage is exercised in CI.
|
Thanks for the detailed review — all three issues are addressed in this update. Merge conflict resolved — rebased onto current Test wired into CI — Group ID / GraphQL path mismatch — added a
All three GraphQL-based tools ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c195828ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "list_webhooks", | ||
| "list_webhook_events", | ||
| "get_webhook_event", | ||
| "get_dependency_proxy_settings", |
There was a problem hiding this comment.
When GITLAB_READ_ONLY_MODE=true, this still exposes get_dependency_proxy_settings, but that tool queries dependencyProxySetting; GitLab's current GroupType resolver backs that field with group.dependency_proxy_setting || group.create_dependency_proxy_setting, so a “read” against a group that has no settings row yet writes one to the GitLab database. This breaks the server's read-only contract for those groups; either avoid this field in read-only mode or don't classify the tool as read-only.
Useful? React with 👍 / 👎.
zereight
left a comment
zereight
left a comment
There was a problem hiding this comment.
Thanks for the follow-up commit 8c19582 — the GraphQL rewrite, resolveGroupFullPath, and wiring test/test-dependency-proxy.ts into test:mock address my earlier review items.
I'm still requesting changes for one blocking issue:
Build fails on current head
npm run build fails with TS18046: 'data' is of type 'unknown' at resolveGroupFullPath (index.ts:7825). Please parse the /groups/:id response with an existing Zod schema (same pattern as other REST helpers) instead of reading data.full_path from an unknown value.
Residual (non-blocking): consider documenting or avoiding the dependencyProxySetting GraphQL field in read-only deployments — GitLab's Group#dependency_proxy_setting uses a lazy build_dependency_proxy_setting fallback.
| const response = await fetch(`${getEffectiveApiUrl()}/groups/${decoded}`, getFetchConfig()); | ||
| await handleGitLabError(response); | ||
| const data = await response.json(); | ||
| return data.full_path as string; |
There was a problem hiding this comment.
This fails npm run build on current main because response.json() is typed as unknown under the repo TypeScript settings. Could we parse the /groups/:id response with an existing Zod group schema (same pattern as other REST helpers) instead of data.full_path as string?
|
Sorry about that — fixed in the latest commit. Replaced the unsafe |
|
Thanks for addressing the earlier feedback — the The PR is still merge-conflicting with |
|
Rebased onto current One small note for the future: this PR has now been rebased three times in response to review requests, each time because new commits landed on |
|
Verified locally against the same steps CI runs:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4836aa4c32
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (/^\d+$/.test(decoded)) { | ||
| const response = await fetch(`${getEffectiveApiUrl()}/groups/${decoded}`, getFetchConfig()); | ||
| await handleGitLabError(response); |
There was a problem hiding this comment.
resolveGroupFullPath treats any all-digit group_id as a numeric ID and immediately calls GET /groups/:id. That breaks valid numeric-only group paths (for example "5000"): instead of querying that path in GraphQL, these tools resolve or fail against group ID 5000. Because the input schema explicitly accepts "ID or URL-encoded path", this can make get_dependency_proxy_settings, update_dependency_proxy_settings, and list_dependency_proxy_blobs target the wrong group or return 404 for legitimate path inputs.
Useful? React with 👍 / 👎.
|
Rebased again. Fourth time now. The conflict is the same every single time: a new test file lands on |
babyhuey
force-pushed
the
main
branch
2 times, most recently
from
752c819 to
ab4fc2e
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Adds five tools for interacting with the GitLab group-level dependency proxy API: get/update settings, list blobs, delete a blob, and purge the cache. Exposed as an opt-in toolset (dependency_proxy, isDefault: false).
- GET/PATCH /groups/:id/dependency_proxy and GET/DELETE blobs do not exist as REST endpoints; only DELETE /cache is documented. Rewrote get_dependency_proxy_settings, update_dependency_proxy_settings, and list_dependency_proxy_blobs to use the GraphQL API instead. - Removed delete_dependency_proxy_blob (no REST or GraphQL equivalent). - Fixed GitLabDependencyProxyBlobSchema.size to z.string() (GitLab returns a human-readable string, not a number). - Fixed purge success message to "scheduled" (GitLab returns 202 and queues a background job, not an immediate delete). - Added dependency_proxy to test-toolset-filtering.ts counts/samples.
- Add test/test-dependency-proxy.ts covering get settings, list blobs, purge cache, toolset activation, and read-only mode enforcement - Add GitLabDependencyProxySchema and GitLabDependencyProxyBlobSchema tests to test/schema-tests.ts (6 new cases, 90 total) - Add 4 new tools to README.md tool list (155-158) - Guard updateDependencyProxySettings against no-op calls with no update fields provided
- Raise an error when updateDependencyProxySettings mutation returns payload-level errors rather than silently succeeding - Decode group_id before re-encoding in purgeDependencyProxyCache to avoid double-encoding pre-encoded paths
- Use z.coerce.string() for all 4 group_id fields (matches every peer schema in the codebase; allows numeric IDs to be auto-coerced) - Standardize group_id description to "Group ID or URL-encoded path" across all 4 input schemas - Simplify purgeDependencyProxyCache to plain encodeURIComponent(groupId) instead of the unusual decode-then-re-encode pattern - Fix GraphQL mock router collision: check for updateDependencyProxySettings mutation before the dependencyProxySetting query substring match - Add success-path test for update_dependency_proxy_settings - Update design spec to reflect actual implementation (4 tools, GraphQL)
…ructor
- remove "TTL" from update_dependency_proxy_settings description (no ttl
field in the input schema)
- pass { port, validTokens } config object to MockGitLabServer constructor
(positional number was wrong type per MockGitLabConfig interface)
- reject(response.error) rejected with a plain object, causing assert.rejects(/regex/) to never match; wrap in new Error() instead - GITLAB_READ_ONLY_MODE only filters tools/list, not tools/call; rewrite the read-only test to verify write tools are absent from tools/list and read-only tools are still present
…to CI - Add resolveGroupFullPath() helper: decodes URL-encoded paths, resolves numeric IDs to full_path via GET /groups/:id before GraphQL usage - Use resolved fullPath in get/update/list dependency proxy GraphQL calls - Use decode-then-encode pattern in purgeDependencyProxyCache (matches peers) - Add test/test-dependency-proxy.ts to test:mock script in package.json
Replaces unsafe `data.full_path as string` cast with a typed Zod parse, fixing TS18046 compile error reported in review.
2 participants
Closes #473
Summary
dependency_proxy(isDefault: false) — activate viaGITLAB_TOOLSETS=dependency_proxyordiscover_toolsTools added
get_dependency_proxy_settingsgroup { dependencyProxySetting ... }update_dependency_proxy_settingsmutation updateDependencyProxySettingslist_dependency_proxy_blobsgroup { dependencyProxyBlobs ... }purge_dependency_proxy_cacheDELETE /groups/:id/dependency_proxy/cacheFiles changed
schemas.ts— 2 response types + 4 input schemasindex.ts— 4 API functions + 4 switch case handlerstools/registry.ts— tool entries,readOnlyTools,destructiveTools,ToolsetIdunion, toolset definitiontest/test-dependency-proxy.ts— integration teststest/schema-tests.ts— Zod parse tests for new response typestest/test-toolset-filtering.ts— updated toolset countsTest plan
get_dependency_proxy_settingsreturns settings for a groupupdate_dependency_proxy_settingsenables proxy and returns updated settingsupdate_dependency_proxy_settingsrejects calls with no update fieldslist_dependency_proxy_blobsreturns paginated blob list with string sizespurge_dependency_proxy_cachereturns scheduled status messagedependency_proxytoolset is not activated