ESRP: support multiple users, clarify docs, retry transient submit failures - #1412
Elizabeth Craig (ecraig12345) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
PARSE_ERROR can still be retried with a transient status code, and regression coverage is incomplete.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR enhances the ESRP npm release helper with multi-user configuration, clearer API documentation, base64 file hashes, and transient submission retries.
Changes:
- Supports comma-separated
ESRP_USERvalues and clarifies contact restrictions. - Refines ESRP API types and JWS/hash handling.
- Adds three-attempt submit retries and updates parse-error classification.
- Updates documentation, tests, and change records.
File summaries
| File | Description |
|---|---|
packages/esrp-npm-release/src/utils/getEnvOptions.ts |
Parses multiple fallback users. |
packages/esrp-npm-release/src/utils/errorHelpers.ts |
Updates retry classification. |
packages/esrp-npm-release/src/types/EnvOptions.ts |
Clarifies contact restrictions. |
packages/esrp-npm-release/src/types/api.ts |
Refines ESRP API types. |
packages/esrp-npm-release/src/esrpApi/releaseHttp.ts |
Adds transient submission retries. |
packages/esrp-npm-release/src/esrpApi/npmRelease.ts |
Uses base64 file hashes. |
packages/esrp-npm-release/src/auth/generateJwsToken.ts |
Adds typed JWS headers. |
packages/esrp-npm-release/src/__tests__/releaseHttp.test.ts |
Tests submission retries. |
packages/esrp-npm-release/src/__tests__/npmRelease.test.ts |
Updates hash and logging tests. |
packages/esrp-npm-release/src/__tests__/getEnvOptions.test.ts |
Tests multiple fallback users. |
packages/esrp-npm-release/src/__tests__/generateJwsToken.test.ts |
Updates JWS type tests. |
packages/esrp-npm-release/src/__tests__/errorHelpers.test.ts |
Updates retry classification tests. |
packages/esrp-npm-release/README.md |
Documents configuration and contact changes. |
change/@microsoft-esrp-npm-release-b7d0e7c6-9f2a-49e5-b785-7fbd783c8eca.json |
Records retry behavior changes. |
change/@microsoft-esrp-npm-release-b66dea30-7381-4b55-a98b-a0b79dc9c08a.json |
Records user and documentation changes. |
Review details
Suppressed comments (1)
packages/esrp-npm-release/src/tests/errorHelpers.test.ts:23
- The changed classification removes
PARSE_ERRORfrom the retryable cases, but this replacement test only exercisesREQUEST_SEND_ERROR, so a regression could silently make parse failures retryable again. Add an assertion thatRestError.PARSE_ERRORis non-retryable.
it('retries pipeline error REQUEST_SEND_ERROR', () => {
expect(isRetryableAzureError(new RestError('pipeline failure', { code: RestError.REQUEST_SEND_ERROR }))).toBe(true);
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Retries may duplicate release operations without an idempotency guarantee, and regression coverage remains incomplete.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/esrp-npm-release/src/tests/errorHelpers.test.ts:23
- The old parameterized case explicitly covered
RestError.PARSE_ERROR; removing it leaves the new non-retryable classification untested. Add a regression assertion for a parse error without a status so this retry-safety rule cannot regress.
it('retries pipeline error REQUEST_SEND_ERROR', () => {
expect(isRetryableAzureError(new RestError('pipeline failure', { code: RestError.REQUEST_SEND_ERROR }))).toBe(true);
packages/esrp-npm-release/src/tests/releaseHttp.test.ts:65
- Because this scenario succeeds on the third mocked request, it would also pass if
maxAttemptswere 4 or unbounded. Add an all-transient-failures case that asserts the submit stops after exactly three fetches and returns the wrappedReleaseError, so the new hard attempt limit is covered.
it('retries transient failures up to three attempts', async () => {
jest.useFakeTimers();
fetchMock
.mockResolvedValueOnce(makeFetchResponse({ status: 503, body: 'unavailable' }))
.mockRejectedValueOnce(new Error('fetch failed'))
.mockResolvedValueOnce(makeFetchResponse({ body: '{"operationId":"op-1"}' }));
const promise = submitRelease({ ...defaultParams, releaseRequest: mockRequest });
await jest.runAllTimersAsync();
await expect(promise).resolves.toEqual({ operationId: 'op-1' });
expect(fetchMock).toHaveBeenCalledTimes(3);
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
| bearerToken, | ||
| method: 'POST', | ||
| body: releaseRequest, | ||
| maxAttempts: 3, |
ESRP_USERto accept multiple user emails, and clarify where DLs are and are not allowed