23: Switch GitLab MR creation from CLI to REST API #188

david · 2026-08-10T04:15:22Z

david commented

2026-08-10 04:15:22 +00:00

23: Switch GitLab MR creation from CLI to REST API

Implements 23.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 6m 00s.

Review findings

Now let me look at how MrClientError and mrClientErrorFromStatus build the message/context:All the pieces make sense. Now let me apply the three fixes to the test file.Now run the gates:All 17 tests pass. Now the full gates:Build and lint clean. Now full test suite:All gates green: build clean, lint clean, 1516 tests pass (up from 1511 — added 5 new tests). All three findings resolved.

Review Findings — GL-23

Summary

All three findings resolved in a single pass. Build, test, and lint gates all pass (npm run build clean, npm run lint clean, npm test = 1516 passed / 117 files, up from 1511 after adding 5 targeted tests). Changes are test-only, consistent with the existing parametrized style in gitLabMrClient.test.ts:

Medium (fail-fast no-retry): Added a parametrized test over 401/403/404 with retry: { maxAttempts: 3, baseDelayMs: 0 } asserting fetch is called exactly once, proving non-retriable errors short-circuit the retry loop.
Low (retry-then-success + 429 retry): Added a 500→201 retry-then-success test (asserts MR created, fetch called twice) and a 429 retried-to-exhaustion test (fetch called 3 times).
Low (error message/context propagation): Extended the classification test to assert the thrown error message contains both the GitLab MR create context and the g/p repo.

Critical

(none)

High

(none)

Medium

src/gitlab/services/gitLabMrClient.test.ts:106 — The parametrized classification test uses retry: { maxAttempts: 1 } for all cases, so it cannot verify the client's documented invariant that "Fail-fast errors (401/403/404) are not retried" (gitLabMrClient.ts:44). With maxAttempts: 1 even a retriable 429/500 would only be called once, so the no-retry behavior for non-retriable kinds is untested. Suggested fix: add a case (or a dedicated test) with retry: { maxAttempts: 3, baseDelayMs: 0 } for a 401/403/404 that asserts expect(fetch).toHaveBeenCalledTimes(1), proving fail-fast errors short-circuit the retry loop.

Low

src/gitlab/services/gitLabMrClient.test.ts:137 — The retry test asserts a 500 is retried up to maxAttempts (good), but there is no complementary assertion that a 429 (also retriable: true) is retried, and no test that a transient status which succeeds on the 2nd attempt returns a result. Suggested fix: add a case where the fetch returns 500 then 201 with maxAttempts: 3 and assert the MR is created and fetch was called twice, exercising the retry-then-success path.
src/gitlab/services/gitLabMrClient.test.ts:104 — The [status, kind, retriable] tuple asserts against MrClientError fields but does not assert the error message/context propagation (the client passes context: 'GitLab MR create for ...'). Suggested fix: optionally assert the thrown error message includes the repo/context so regressions in error messaging are caught.

## 23: Switch GitLab MR creation from CLI to REST API Implements [23](https://git.excelera.net/david/pi-loop/issues/23). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 6m 00s. ## Review findings Now let me look at how `MrClientError` and `mrClientErrorFromStatus` build the message/context:All the pieces make sense. Now let me apply the three fixes to the test file.Now run the gates:All 17 tests pass. Now the full gates:Build and lint clean. Now full test suite:All gates green: build clean, lint clean, 1516 tests pass (up from 1511 — added 5 new tests). All three findings resolved. # Review Findings — GL-23 ## Summary All three findings resolved in a single pass. Build, test, and lint gates all pass (`npm run build` clean, `npm run lint` clean, `npm test` = 1516 passed / 117 files, up from 1511 after adding 5 targeted tests). Changes are test-only, consistent with the existing parametrized style in `gitLabMrClient.test.ts`: - **Medium (fail-fast no-retry):** Added a parametrized test over 401/403/404 with `retry: { maxAttempts: 3, baseDelayMs: 0 }` asserting `fetch` is called exactly once, proving non-retriable errors short-circuit the retry loop. - **Low (retry-then-success + 429 retry):** Added a 500→201 retry-then-success test (asserts MR created, `fetch` called twice) and a 429 retried-to-exhaustion test (`fetch` called 3 times). - **Low (error message/context propagation):** Extended the classification test to assert the thrown error message contains both the `GitLab MR create` context and the `g/p` repo. ## Critical - (none) ## High - (none) ## Medium - [x] src/gitlab/services/gitLabMrClient.test.ts:106 — The parametrized classification test uses `retry: { maxAttempts: 1 }` for all cases, so it cannot verify the client's documented invariant that "Fail-fast errors (`401/403/404`) are not retried" (`gitLabMrClient.ts:44`). With `maxAttempts: 1` even a retriable 429/500 would only be called once, so the no-retry behavior for non-retriable kinds is untested. Suggested fix: add a case (or a dedicated test) with `retry: { maxAttempts: 3, baseDelayMs: 0 }` for a 401/403/404 that asserts `expect(fetch).toHaveBeenCalledTimes(1)`, proving fail-fast errors short-circuit the retry loop. ## Low - [x] src/gitlab/services/gitLabMrClient.test.ts:137 — The retry test asserts a 500 is retried up to `maxAttempts` (good), but there is no complementary assertion that a 429 (also `retriable: true`) is retried, and no test that a transient status which succeeds on the 2nd attempt returns a result. Suggested fix: add a case where the fetch returns `500` then `201` with `maxAttempts: 3` and assert the MR is created and `fetch` was called twice, exercising the retry-then-success path. - [x] src/gitlab/services/gitLabMrClient.test.ts:104 — The `[status, kind, retriable]` tuple asserts against `MrClientError` fields but does not assert the error `message`/`context` propagation (the client passes `context: 'GitLab MR create for ...'`). Suggested fix: optionally assert the thrown error message includes the repo/context so regressions in error messaging are caught.

david added 2 commits

2026-08-10 04:15:24 +00:00

23: Switch GitLab MR creation from CLI to REST API 6a74d8e41e