-
Notifications
You must be signed in to change notification settings - Fork 317
feat: binary (base64) commits and per-file actions in push_files #513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
All fields are optional with sensible defaults, maintaining backward compatibility as stated in the PR objectives. 💡 Optional: Consider adding schema-level validation for field combinationsWhile 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Tree and commit schemas | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |
|
|
||
| import { | ||
| GetFileContentsSchema, | ||
| PushFilesSchema, | ||
| CreateOrUpdateFileSchema, | ||
| GitLabFileContentSchema, | ||
| GitLabRepositorySchema, | ||
| CreatePipelineSchema, | ||
|
|
@@ -156,6 +158,97 @@ 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', | ||
| }, | ||
| { | ||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 |
||
|
|
||
| function runGitLabFileContentSchemaTests(): { passed: number; failed: number } { | ||
| console.log('\n🧪 Testing GitLabFileContentSchema...'); | ||
|
|
||
|
|
@@ -1658,6 +1751,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(); | ||
|
|
@@ -1677,8 +1771,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`); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.