Skip to content

Commit 00f93d2

Browse files
polazpolaz
authored
fix(milestones): simplify schema to use single milestone_id field (#121)
* fix(milestones): simplify schema to use single milestone_id field - Remove redundant iid field from milestone schemas, keep only milestone_id - Update integration tests to use global ID instead of IID (GitLab API requirement) - Add stale test data cleanup in globalSetup to prevent 404 errors - Add milestones.test.ts to test sequencer for proper ordering - Add per_page tests for browse_mr_discussions handler * docs(milestones): clarify milestone_id uses global ID, not IID GitLab Milestones API uses global ID in :milestone_id parameter, unlike issues/MRs which use IID in URL paths. * test(milestones): add index.test.ts for module exports * fix(tests): enable all features in unit tests and fix index.test.ts - Remove feature flag disabling from setupTests.unit.ts Unit tests should test all code paths, not disabled state - Remove GITLAB_READONLY from unit test setup This was preventing proper testing of write operations - Rewrite index.test.ts to test registry functions directly Tests both read-only and full mode properly
1 parent ce5a352 commit 00f93d2

File tree

11 files changed

+284
-554
lines changed

11 files changed

+284
-554
lines changed

‎src/entities/milestones/registry.ts‎

Lines changed: 20 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -45,46 +45,38 @@ export const milestonesToolRegistry: ToolRegistry = new Map<string, EnhancedTool
4545
}
4646

4747
case "get": {
48-
// TypeScript knows: input has milestone_id or iid (at least one required by schema)
49-
// Prefer iid if both are provided, fallback to milestone_id
50-
const milestoneIdentifier = input.iid ?? input.milestone_id;
51-
return gitlab.get(`${entityType}/${encodedPath}/milestones/${milestoneIdentifier}`);
48+
// TypeScript knows: input has milestone_id (uses global ID, not IID)
49+
return gitlab.get(`${entityType}/${encodedPath}/milestones/${input.milestone_id}`);
5250
}
5351

5452
case "issues": {
55-
// TypeScript knows: input has milestone_id or iid (at least one required), per_page, page (optional)
56-
const { action: _action, namespace: _namespace, milestone_id, iid, ...rest } = input;
57-
const milestoneIdentifier = iid ?? milestone_id;
53+
// TypeScript knows: input has milestone_id (uses global ID), per_page, page (optional)
54+
const { action: _action, namespace: _namespace, milestone_id, ...rest } = input;
5855
const query = toQuery(rest, []);
5956

60-
return gitlab.get(
61-
`${entityType}/${encodedPath}/milestones/${milestoneIdentifier}/issues`,
62-
{
63-
query,
64-
}
65-
);
57+
return gitlab.get(`${entityType}/${encodedPath}/milestones/${milestone_id}/issues`, {
58+
query,
59+
});
6660
}
6761

6862
case "merge_requests": {
69-
// TypeScript knows: input has milestone_id or iid (at least one required), per_page, page (optional)
70-
const { action: _action, namespace: _namespace, milestone_id, iid, ...rest } = input;
71-
const milestoneIdentifier = iid ?? milestone_id;
63+
// TypeScript knows: input has milestone_id (uses global ID), per_page, page (optional)
64+
const { action: _action, namespace: _namespace, milestone_id, ...rest } = input;
7265
const query = toQuery(rest, []);
7366

7467
return gitlab.get(
75-
`${entityType}/${encodedPath}/milestones/${milestoneIdentifier}/merge_requests`,
68+
`${entityType}/${encodedPath}/milestones/${milestone_id}/merge_requests`,
7669
{ query }
7770
);
7871
}
7972

8073
case "burndown": {
81-
// TypeScript knows: input has milestone_id or iid (at least one required), per_page, page (optional)
82-
const { action: _action, namespace: _namespace, milestone_id, iid, ...rest } = input;
83-
const milestoneIdentifier = iid ?? milestone_id;
74+
// TypeScript knows: input has milestone_id (uses global ID), per_page, page (optional)
75+
const { action: _action, namespace: _namespace, milestone_id, ...rest } = input;
8476
const query = toQuery(rest, []);
8577

8678
return gitlab.get(
87-
`${entityType}/${encodedPath}/milestones/${milestoneIdentifier}/burndown_events`,
79+
`${entityType}/${encodedPath}/milestones/${milestone_id}/burndown_events`,
8880
{ query }
8981
);
9082
}
@@ -131,32 +123,28 @@ export const milestonesToolRegistry: ToolRegistry = new Map<string, EnhancedTool
131123
}
132124

133125
case "update": {
134-
// TypeScript knows: input has milestone_id or iid (at least one required), title, description, etc. (optional)
135-
const { action: _action, namespace: _namespace, milestone_id, iid, ...body } = input;
136-
const milestoneIdentifier = iid ?? milestone_id;
126+
// TypeScript knows: input has milestone_id (uses global ID), title, description, etc. (optional)
127+
const { action: _action, namespace: _namespace, milestone_id, ...body } = input;
137128

138-
return gitlab.put(`${entityType}/${encodedPath}/milestones/${milestoneIdentifier}`, {
129+
return gitlab.put(`${entityType}/${encodedPath}/milestones/${milestone_id}`, {
139130
body,
140131
contentType: "json",
141132
});
142133
}
143134

144135
case "delete": {
145-
// TypeScript knows: input has milestone_id or iid (at least one required)
146-
const milestoneIdentifier = input.iid ?? input.milestone_id;
147-
await gitlab.delete(`${entityType}/${encodedPath}/milestones/${milestoneIdentifier}`);
136+
// TypeScript knows: input has milestone_id (uses global ID)
137+
await gitlab.delete(`${entityType}/${encodedPath}/milestones/${input.milestone_id}`);
148138
return { deleted: true };
149139
}
150140

151141
case "promote": {
152-
// TypeScript knows: input has milestone_id or iid (at least one required by schema)
142+
// TypeScript knows: input has milestone_id (uses global ID)
153143
if (entityType !== "projects") {
154144
throw new Error("Milestone promotion is only available for projects, not groups");
155145
}
156146

157-
// Schema validation guarantees at least one identifier is present
158-
const milestoneIdentifier = input.iid ?? input.milestone_id;
159-
return gitlab.post(`projects/${encodedPath}/milestones/${milestoneIdentifier}/promote`);
147+
return gitlab.post(`projects/${encodedPath}/milestones/${input.milestone_id}/promote`);
160148
}
161149

162150
/* istanbul ignore next -- unreachable with Zod discriminatedUnion */

‎src/entities/milestones/schema-readonly.ts‎

Lines changed: 9 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -30,28 +30,15 @@ export const GitLabMilestonesSchema = z.object({
3030

3131
// --- Shared fields ---
3232
const namespaceField = z.string().describe("Namespace path (group or project)");
33-
const milestoneIdField = requiredId
34-
.optional()
35-
.describe("Milestone ID (same as IID in GitLab URLs, e.g., '3' from /milestones/3)");
36-
const milestoneIidField = z
37-
.string()
38-
.min(1)
39-
.optional()
40-
.describe("Milestone IID from URL (e.g., '3' from /milestones/3). Alternative to milestone_id.");
4133

42-
// Refinement to require either milestone_id or iid
43-
const requireMilestoneIdentifier = (
44-
data: { milestone_id?: string; iid?: string },
45-
ctx: z.RefinementCtx
46-
) => {
47-
if (data.milestone_id === undefined && data.iid === undefined) {
48-
ctx.addIssue({
49-
code: z.ZodIssueCode.custom,
50-
message: "Either 'milestone_id' or 'iid' must be provided",
51-
path: ["milestone_id"],
52-
});
53-
}
54-
};
34+
// NOTE on milestone_id:
35+
// GitLab Milestones REST API uses the global ID in URL paths, NOT the IID.
36+
// Example: GET /projects/:id/milestones/:milestone_id where :milestone_id is the global ID.
37+
// The API response contains both 'id' (global unique) and 'iid' (project-scoped).
38+
// Unlike issues/MRs which use IID in URLs, milestones use the global ID.
39+
const milestoneIdField = requiredId.describe(
40+
"The ID of a project or group milestone. Required for 'get', 'issues', 'merge_requests', 'burndown' action(s)."
41+
);
5542

5643
// --- Action: list ---
5744
const ListMilestonesSchema = z.object({
@@ -83,20 +70,17 @@ const ListMilestonesSchema = z.object({
8370
});
8471

8572
// --- Action: get ---
86-
// Supports lookup by either milestone_id or iid (both refer to the same value in GitLab)
8773
const GetMilestoneSchema = z.object({
8874
action: z.literal("get").describe("Get a single milestone by ID"),
8975
namespace: namespaceField,
9076
milestone_id: milestoneIdField,
91-
iid: milestoneIidField,
9277
});
9378

9479
// --- Action: issues ---
9580
const MilestoneIssuesSchema = z.object({
9681
action: z.literal("issues").describe("List issues assigned to a milestone"),
9782
namespace: namespaceField,
9883
milestone_id: milestoneIdField,
99-
iid: milestoneIidField,
10084
...paginationFields(),
10185
});
10286

@@ -105,7 +89,6 @@ const MilestoneMergeRequestsSchema = z.object({
10589
action: z.literal("merge_requests").describe("List merge requests assigned to a milestone"),
10690
namespace: namespaceField,
10791
milestone_id: milestoneIdField,
108-
iid: milestoneIidField,
10992
...paginationFields(),
11093
});
11194

@@ -114,31 +97,18 @@ const MilestoneBurndownSchema = z.object({
11497
action: z.literal("burndown").describe("Get burndown chart data for a milestone"),
11598
namespace: namespaceField,
11699
milestone_id: milestoneIdField,
117-
iid: milestoneIidField,
118100
...paginationFields(),
119101
});
120102

121103
// --- Discriminated union combining all actions ---
122-
// Base union for type narrowing
123-
const BrowseMilestonesBaseSchema = z.discriminatedUnion("action", [
104+
export const BrowseMilestonesSchema = z.discriminatedUnion("action", [
124105
ListMilestonesSchema,
125106
GetMilestoneSchema,
126107
MilestoneIssuesSchema,
127108
MilestoneMergeRequestsSchema,
128109
MilestoneBurndownSchema,
129110
]);
130111

131-
// Add validation for actions that require milestone identifier
132-
export const BrowseMilestonesSchema = BrowseMilestonesBaseSchema.superRefine((data, ctx) => {
133-
// Actions that require milestone_id or iid
134-
const actionsRequiringMilestone = ["get", "issues", "merge_requests", "burndown"];
135-
136-
if (actionsRequiringMilestone.includes(data.action)) {
137-
const dataWithIds = data as { milestone_id?: string; iid?: string };
138-
requireMilestoneIdentifier(dataWithIds, ctx);
139-
}
140-
});
141-
142112
// ============================================================================
143113
// Type exports
144114
// ============================================================================

‎src/entities/milestones/schema.ts‎

Lines changed: 9 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -15,28 +15,15 @@ import { requiredId } from "../utils";
1515

1616
// --- Base fields shared by all actions ---
1717
const namespaceField = z.string().describe("Namespace path (group or project)");
18-
const milestoneIdField = requiredId
19-
.optional()
20-
.describe("Milestone ID (same as IID in GitLab URLs, e.g., '3' from /milestones/3)");
21-
const milestoneIidField = z
22-
.string()
23-
.min(1)
24-
.optional()
25-
.describe("Milestone IID from URL (e.g., '3' from /milestones/3). Alternative to milestone_id.");
2618

27-
// Refinement to require either milestone_id or iid
28-
const requireMilestoneIdentifier = (
29-
data: { milestone_id?: string; iid?: string },
30-
ctx: z.RefinementCtx
31-
) => {
32-
if (data.milestone_id === undefined && data.iid === undefined) {
33-
ctx.addIssue({
34-
code: z.ZodIssueCode.custom,
35-
message: "Either 'milestone_id' or 'iid' must be provided",
36-
path: ["milestone_id"],
37-
});
38-
}
39-
};
19+
// NOTE on milestone_id:
20+
// GitLab Milestones REST API uses the global ID in URL paths, NOT the IID.
21+
// Example: PUT /projects/:id/milestones/:milestone_id where :milestone_id is the global ID.
22+
// The API response contains both 'id' (global unique) and 'iid' (project-scoped).
23+
// Unlike issues/MRs which use IID in URLs, milestones use the global ID.
24+
const milestoneIdField = requiredId.describe(
25+
"The ID of a project or group milestone. Required for 'update', 'delete', 'promote' action(s)."
26+
);
4027

4128
// --- Create action: creates a new milestone ---
4229
const CreateMilestoneSchema = z.object({
@@ -53,7 +40,6 @@ const UpdateMilestoneSchema = z.object({
5340
action: z.literal("update"),
5441
namespace: namespaceField,
5542
milestone_id: milestoneIdField,
56-
iid: milestoneIidField,
5743
title: z.string().optional().describe("The new title of the milestone"),
5844
description: z.string().optional().describe("The new description of the milestone"),
5945
due_date: z.string().optional().describe("The due date of the milestone (YYYY-MM-DD)"),
@@ -71,37 +57,23 @@ const DeleteMilestoneSchema = z.object({
7157
action: z.literal("delete"),
7258
namespace: namespaceField,
7359
milestone_id: milestoneIdField,
74-
iid: milestoneIidField,
7560
});
7661

7762
// --- Promote action: elevates project milestone to group level ---
7863
const PromoteMilestoneSchema = z.object({
7964
action: z.literal("promote"),
8065
namespace: namespaceField,
8166
milestone_id: milestoneIdField,
82-
iid: milestoneIidField,
8367
});
8468

8569
// --- Discriminated union combining all actions ---
86-
// Base union for type narrowing
87-
const ManageMilestoneBaseSchema = z.discriminatedUnion("action", [
70+
export const ManageMilestoneSchema = z.discriminatedUnion("action", [
8871
CreateMilestoneSchema,
8972
UpdateMilestoneSchema,
9073
DeleteMilestoneSchema,
9174
PromoteMilestoneSchema,
9275
]);
9376

94-
// Add validation for actions that require milestone identifier
95-
export const ManageMilestoneSchema = ManageMilestoneBaseSchema.superRefine((data, ctx) => {
96-
// Actions that require milestone_id or iid
97-
const actionsRequiringMilestone = ["update", "delete", "promote"];
98-
99-
if (actionsRequiringMilestone.includes(data.action)) {
100-
const dataWithIds = data as { milestone_id?: string; iid?: string };
101-
requireMilestoneIdentifier(dataWithIds, ctx);
102-
}
103-
});
104-
10577
// ============================================================================
10678
// Type exports
10779
// ============================================================================

0 commit comments

Comments
 (0)