Skip to content

Commit 02b9ae7

Browse files
legardRuslan Tabolinclaudepolaz
authored
fix(mrs): use bracket notation for position in form-encoded requests (#95)
* fix(mrs): use bracket notation for position in form-encoded requests GitLab API expects position fields as separate form parameters with bracket notation (position[base_sha], position[head_sha], etc.) when using form-encoded content type. Previously, position was serialized as a JSON string which resulted in 400 Bad Request errors when creating diff-based discussions or draft notes. Fixes: - manage_mr_discussion action 'thread' - manage_mr_discussion action 'suggest' - manage_draft_notes action 'create' - manage_draft_notes action 'update' Co-Authored-By: Claude Opus 4.5 <[email protected]> * test(mrs): update suggest tests for bracket notation position encoding Update test expectations to match the new flattenPositionToFormFields implementation that uses bracket notation (position[base_sha]) instead of JSON.stringify for form-encoded requests. Co-Authored-By: Claude Opus 4.5 <[email protected]> * test(mrs): add unit tests for flattenPositionToFormFields helper Export the helper function and add comprehensive unit tests covering: - Flat position object with bracket notation - Nested objects (2 levels) - Deeply nested objects (3 levels - line_range.start.line_code) - Null and undefined value handling - Array handling at top and nested levels - Preservation of existing body fields - Empty position object - Boolean and number values Co-Authored-By: Claude Opus 4.5 <[email protected]> * test(mrs): add integration tests for position encoding and schema validation - Add integration tests for diff notes with position (E2E validation) - Add integration test for draft notes with position - Add unit tests for schema superRefine validation (list/get/compare field checks) - Add unit tests for index.ts exports --------- Co-authored-by: Ruslan Tabolin <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: Dmitry Prudnikov <[email protected]>
1 parent 9eda6eb commit 02b9ae7

File tree

4 files changed

+555
-7
lines changed

4 files changed

+555
-7
lines changed

‎src/entities/mrs/registry.ts‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,44 @@ import { normalizeProjectId } from "../../utils/projectIdentifier";
1010
import { ToolRegistry, EnhancedToolDefinition } from "../../types";
1111
import { isActionDenied } from "../../config";
1212

13+
/**
14+
* Flattens a position object into form-encoded fields with bracket notation.
15+
* GitLab API expects: position[base_sha]=xxx, position[head_sha]=xxx, etc.
16+
* NOT: position={"base_sha":"xxx","head_sha":"xxx"}
17+
*
18+
* @see https://docs.gitlab.com/ee/api/discussions.html#create-a-new-thread-in-the-merge-request-diff
19+
*/
20+
export function flattenPositionToFormFields(
21+
body: Record<string, unknown>,
22+
position: Record<string, unknown>
23+
): void {
24+
for (const [key, value] of Object.entries(position)) {
25+
if (value === undefined || value === null) continue;
26+
27+
if (typeof value === "object" && !Array.isArray(value)) {
28+
// Handle nested objects like line_range.start, line_range.end
29+
for (const [nestedKey, nestedValue] of Object.entries(value as Record<string, unknown>)) {
30+
if (nestedValue === undefined || nestedValue === null) continue;
31+
32+
if (typeof nestedValue === "object" && !Array.isArray(nestedValue)) {
33+
// Handle deeply nested (line_range.start.line_code, etc.)
34+
for (const [deepKey, deepValue] of Object.entries(
35+
nestedValue as Record<string, unknown>
36+
)) {
37+
if (deepValue !== undefined && deepValue !== null) {
38+
body[`position[${key}][${nestedKey}][${deepKey}]`] = deepValue;
39+
}
40+
}
41+
} else {
42+
body[`position[${key}][${nestedKey}]`] = nestedValue;
43+
}
44+
}
45+
} else {
46+
body[`position[${key}]`] = value;
47+
}
48+
}
49+
}
50+
1351
/**
1452
* MRS (Merge Requests) tools registry - 5 CQRS tools replacing 20 individual tools
1553
*
@@ -340,7 +378,7 @@ export const mrsToolRegistry: ToolRegistry = new Map<string, EnhancedToolDefinit
340378
const { project_id, merge_request_iid, body: noteBody, position, commit_id } = input;
341379

342380
const body: Record<string, unknown> = { body: noteBody };
343-
if (position) body.position = JSON.stringify(position);
381+
if (position) flattenPositionToFormFields(body, position);
344382
if (commit_id) body.commit_id = commit_id;
345383

346384
return gitlab.post(
@@ -444,8 +482,8 @@ export const mrsToolRegistry: ToolRegistry = new Map<string, EnhancedToolDefinit
444482
// Create discussion thread with position (same as thread action)
445483
const body: Record<string, unknown> = {
446484
body: noteBody,
447-
position: JSON.stringify(position),
448485
};
486+
flattenPositionToFormFields(body, position);
449487

450488
return gitlab.post(
451489
`projects/${normalizeProjectId(project_id)}/merge_requests/${merge_request_iid}/discussions`,
@@ -494,7 +532,7 @@ export const mrsToolRegistry: ToolRegistry = new Map<string, EnhancedToolDefinit
494532
} = input;
495533

496534
const body: Record<string, unknown> = { note };
497-
if (position) body.position = JSON.stringify(position);
535+
if (position) flattenPositionToFormFields(body, position);
498536
if (in_reply_to_discussion_id)
499537
body.in_reply_to_discussion_id = in_reply_to_discussion_id;
500538
if (commit_id) body.commit_id = commit_id;
@@ -510,7 +548,7 @@ export const mrsToolRegistry: ToolRegistry = new Map<string, EnhancedToolDefinit
510548
const { project_id, merge_request_iid, draft_note_id, note, position } = input;
511549

512550
const body: Record<string, unknown> = { note };
513-
if (position) body.position = JSON.stringify(position);
551+
if (position) flattenPositionToFormFields(body, position);
514552

515553
return gitlab.put(
516554
`projects/${normalizeProjectId(project_id)}/merge_requests/${merge_request_iid}/draft_notes/${draft_note_id}`,

‎tests/integration/data-lifecycle.test.ts‎

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1205,6 +1205,216 @@ describe("🔄 Data Lifecycle - Complete Infrastructure Setup", () => {
12051205

12061206
console.log("✅ MR discussion thread operations test completed");
12071207
});
1208+
1209+
it("should test diff note creation with position (PR #95 validation)", async () => {
1210+
const testData = getTestData();
1211+
expect(testData.mergeRequests?.length).toBeGreaterThan(0);
1212+
expect(testData.project?.id).toBeDefined();
1213+
console.log("🔧 Testing diff note with position (PR #95 - flattenPositionToFormFields)...");
1214+
1215+
const mr = testData.mergeRequests![0];
1216+
const projectId = testData.project!.id.toString();
1217+
1218+
// Step 1: Add a file to the feature branch to create a diff
1219+
console.log(" 📝 Adding file to feature branch to create diff...");
1220+
const featureBranch = mr.source_branch;
1221+
const testFilePath = `test-diff-note-${Date.now()}.js`;
1222+
const testFileContent = Buffer.from(
1223+
`// Test file for diff note\nconst message = "Hello from diff note test";\nconsole.log(message);\n`
1224+
).toString("base64");
1225+
1226+
const createFileResponse = await fetch(
1227+
`${GITLAB_API_URL}/api/v4/projects/${testData.project!.id}/repository/files/${encodeURIComponent(testFilePath)}`,
1228+
{
1229+
method: "POST",
1230+
headers: {
1231+
Authorization: `Bearer ${GITLAB_TOKEN}`,
1232+
"Content-Type": "application/json",
1233+
},
1234+
body: JSON.stringify({
1235+
branch: featureBranch,
1236+
content: testFileContent,
1237+
encoding: "base64",
1238+
commit_message: "Add test file for diff note testing",
1239+
}),
1240+
}
1241+
);
1242+
1243+
if (!createFileResponse.ok) {
1244+
const errorText = await createFileResponse.text();
1245+
console.log(
1246+
` ⚠️ Could not create test file: ${createFileResponse.status} - ${errorText}`
1247+
);
1248+
console.log(" ⚠️ Skipping diff note test - no diff available");
1249+
return;
1250+
}
1251+
console.log(` ✅ Created test file: ${testFilePath}`);
1252+
1253+
// Step 2: Get updated MR with diff_refs
1254+
console.log(" 🔍 Getting MR diff_refs...");
1255+
const mrDetails = (await helper.browseMergeRequests({
1256+
action: "get",
1257+
project_id: projectId,
1258+
merge_request_iid: mr.iid.toString(),
1259+
})) as any;
1260+
1261+
expect(mrDetails.diff_refs).toBeDefined();
1262+
const { base_sha, head_sha, start_sha } = mrDetails.diff_refs;
1263+
console.log(
1264+
` 📋 diff_refs: base=${base_sha?.substring(0, 8)}, head=${head_sha?.substring(0, 8)}, start=${start_sha?.substring(0, 8)}`
1265+
);
1266+
1267+
// Step 3: Get MR diffs to find the new file
1268+
console.log(" 🔍 Getting MR diffs...");
1269+
const mrDiffs = (await helper.browseMergeRequests({
1270+
action: "diffs",
1271+
project_id: projectId,
1272+
merge_request_iid: mr.iid.toString(),
1273+
})) as any;
1274+
1275+
const testFileDiff = mrDiffs.changes?.find((change: any) => change.new_path === testFilePath);
1276+
if (!testFileDiff) {
1277+
console.log(` ⚠️ Test file not found in diffs - skipping position test`);
1278+
return;
1279+
}
1280+
console.log(` ✅ Found test file in diffs: ${testFileDiff.new_path}`);
1281+
1282+
// Step 4: Create diff note with position using the handler (tests flattenPositionToFormFields)
1283+
console.log(" 📝 Creating diff note with position...");
1284+
const position = {
1285+
base_sha,
1286+
head_sha,
1287+
start_sha,
1288+
position_type: "text" as const,
1289+
new_path: testFilePath,
1290+
old_path: testFilePath,
1291+
new_line: 2, // Comment on line 2: const message = ...
1292+
};
1293+
1294+
try {
1295+
const diffNote = (await helper.manageMrDiscussion({
1296+
action: "thread",
1297+
project_id: projectId,
1298+
merge_request_iid: mr.iid.toString(),
1299+
body: `Test diff note created at ${new Date().toISOString()} - validates PR #95 position encoding fix`,
1300+
position,
1301+
})) as any;
1302+
1303+
expect(diffNote).toBeDefined();
1304+
expect(diffNote.id).toBeDefined();
1305+
console.log(` ✅ Created diff note thread: ${diffNote.id}`);
1306+
1307+
// Verify the note has position data
1308+
const firstNote = diffNote.notes?.[0];
1309+
if (firstNote?.position) {
1310+
expect(firstNote.position.new_path).toBe(testFilePath);
1311+
expect(firstNote.position.new_line).toBe(2);
1312+
console.log(
1313+
` ✅ Diff note has correct position: ${firstNote.position.new_path}:${firstNote.position.new_line}`
1314+
);
1315+
}
1316+
1317+
// Store for later verification
1318+
updateTestData({ diffNoteThread: diffNote });
1319+
1320+
console.log("✅ Diff note with position test PASSED - PR #95 fix verified!");
1321+
} catch (error: any) {
1322+
// If we get a specific error about position, the fix might not be working
1323+
if (error.message?.includes("position") || error.message?.includes("400")) {
1324+
console.error(` ❌ FAILED: Position encoding error - PR #95 fix may not be working!`);
1325+
console.error(` Error: ${error.message}`);
1326+
throw error;
1327+
}
1328+
// Other errors might be GitLab configuration issues
1329+
console.log(` ⚠️ Could not create diff note: ${error.message}`);
1330+
console.log(" ⚠️ This may be a GitLab configuration issue, not a code issue");
1331+
}
1332+
});
1333+
1334+
it("should test draft note with position (validates flattenPositionToFormFields for drafts)", async () => {
1335+
const testData = getTestData();
1336+
expect(testData.mergeRequests?.length).toBeGreaterThan(0);
1337+
console.log("🔧 Testing draft note with position...");
1338+
1339+
const mr = testData.mergeRequests![0];
1340+
const projectId = testData.project!.id.toString();
1341+
1342+
// Get MR diff_refs
1343+
const mrDetails = (await helper.browseMergeRequests({
1344+
action: "get",
1345+
project_id: projectId,
1346+
merge_request_iid: mr.iid.toString(),
1347+
})) as any;
1348+
1349+
if (!mrDetails.diff_refs?.head_sha) {
1350+
console.log(" ⚠️ No diff_refs available - skipping draft note position test");
1351+
return;
1352+
}
1353+
1354+
const { base_sha, head_sha, start_sha } = mrDetails.diff_refs;
1355+
1356+
// Get diffs to find a file
1357+
const mrDiffs = (await helper.browseMergeRequests({
1358+
action: "diffs",
1359+
project_id: projectId,
1360+
merge_request_iid: mr.iid.toString(),
1361+
})) as any;
1362+
1363+
if (!mrDiffs.changes?.length) {
1364+
console.log(" ⚠️ No diffs available - skipping draft note position test");
1365+
return;
1366+
}
1367+
1368+
const firstDiff = mrDiffs.changes[0];
1369+
console.log(` 📝 Creating draft note with position on ${firstDiff.new_path}...`);
1370+
1371+
const position = {
1372+
base_sha,
1373+
head_sha,
1374+
start_sha,
1375+
position_type: "text" as const,
1376+
new_path: firstDiff.new_path,
1377+
old_path: firstDiff.old_path || firstDiff.new_path,
1378+
new_line: 1,
1379+
};
1380+
1381+
try {
1382+
const draftNote = (await helper.executeTool("manage_draft_notes", {
1383+
action: "create",
1384+
project_id: projectId,
1385+
merge_request_iid: mr.iid.toString(),
1386+
note: `Draft diff note test at ${new Date().toISOString()} - validates position encoding`,
1387+
position,
1388+
})) as any;
1389+
1390+
expect(draftNote).toBeDefined();
1391+
expect(draftNote.id).toBeDefined();
1392+
console.log(` ✅ Created draft note: ${draftNote.id}`);
1393+
1394+
// Verify position was correctly encoded
1395+
if (draftNote.position) {
1396+
expect(draftNote.position.new_path).toBe(firstDiff.new_path);
1397+
console.log(` ✅ Draft note has correct position: ${draftNote.position.new_path}`);
1398+
}
1399+
1400+
// Clean up: delete the draft note
1401+
await helper.executeTool("manage_draft_notes", {
1402+
action: "delete",
1403+
project_id: projectId,
1404+
merge_request_iid: mr.iid.toString(),
1405+
draft_note_id: draftNote.id.toString(),
1406+
});
1407+
console.log(` 🧹 Cleaned up draft note`);
1408+
1409+
console.log("✅ Draft note with position test PASSED!");
1410+
} catch (error: any) {
1411+
if (error.message?.includes("position") || error.message?.includes("400")) {
1412+
console.error(` ❌ FAILED: Position encoding error for draft notes!`);
1413+
throw error;
1414+
}
1415+
console.log(` ⚠️ Could not create draft note: ${error.message}`);
1416+
}
1417+
});
12081418
});
12091419

12101420
describe("📬 Step 6.5: Todos Infrastructure", () => {

‎tests/setup/testConfig.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ export interface TestDataState {
3333
labels?: any[];
3434
todos?: any[];
3535
discussionThread?: any;
36+
diffNoteThread?: any;
3637
}
3738

3839
import * as fs from "fs";

0 commit comments

Comments
 (0)