diff --git a/review-enrichment/scripts/validate-sentry-release.mjs b/review-enrichment/scripts/validate-sentry-release.mjs index 6391fdfbf0..dad650902f 100644 --- a/review-enrichment/scripts/validate-sentry-release.mjs +++ b/review-enrichment/scripts/validate-sentry-release.mjs @@ -197,7 +197,13 @@ export async function validateSentryRelease(env = process.env, fetchImpl = globa } let commits = []; - if (config.requireCommits || config.expectedCommitSha) { + // Gated on requireCommits ALONE (not `|| config.expectedCommitSha`): upload-sourcemaps.ts always passes + // SENTRY_COMMIT_SHA (the deploy's actual git SHA, not itself a strictness signal), so expectedCommitSha is + // essentially always set -- fetching here whenever it was merely present, independent of requireCommits, meant + // a non-strict deploy still depended on the /commits/ endpoint being reachable (sentryJson throws on any non-OK + // response) even though the checks that consume the result are now all requireCommits-gated below. Skipping the + // fetch entirely in non-strict mode is the only way "non-strict" actually means "commits don't matter." + if (config.requireCommits) { commits = asArray( await sentryJson( config, @@ -211,7 +217,7 @@ export async function validateSentryRelease(env = process.env, fetchImpl = globa ...commitIdsFrom(release), ...commits.flatMap((commit) => commitIdsFrom(commit)), ]; - if (config.requireCommits && commitCount <= 0 && commitIds.length === 0) { + if (commitCount <= 0 && commitIds.length === 0) { failures.push("release has no associated commits"); } if (config.expectedCommitSha && !commitMatches(config.expectedCommitSha, commitIds)) { diff --git a/review-enrichment/test/sentry-release-validation.test.ts b/review-enrichment/test/sentry-release-validation.test.ts index 1f898293b3..74a5c3d2cd 100644 --- a/review-enrichment/test/sentry-release-validation.test.ts +++ b/review-enrichment/test/sentry-release-validation.test.ts @@ -119,6 +119,57 @@ test("validateSentryRelease rejects a release missing the expected commit", asyn ); }); +test("REGRESSION: validateSentryRelease does NOT enforce the expected-commit match when SENTRY_REQUIRE_COMMITS=false", async () => { + // Same mismatched-commit fixture as the strict-mode test above ("def456" vs the expected "abc123"), but with + // requireCommits off (upload-sourcemaps.ts's non-strict deploy path: SENTRY_REQUIRE_COMMITS: fields.strict ? + // "true" : "false"). SENTRY_COMMIT_SHA is still passed unconditionally (it's the deploy's actual git SHA, not + // itself a strictness signal), so expectedCommitSha stays set -- the bug this guards was that the match check + // read only `config.expectedCommitSha`, never `config.requireCommits`, so it fired regardless of strict mode. + const calls: string[] = []; + const fetchImpl = async (input: string | URL | Request): Promise => { + const path = new URL(String(input)).pathname; + calls.push(path); + if (path.endsWith("/commits/")) return response([{ id: "def456" }]); + if (path.endsWith("/deploys/")) return response([{ name: "deploy-1", environment: "production" }]); + return response({ + version: "gittensory-rees@abc123", + dateReleased: "2026-06-29T00:00:00Z", + commitCount: 1, + deployCount: 1, + projects: [{ slug: "gittensory" }], + }); + }; + + const result = await validateSentryRelease(validationEnv({ SENTRY_REQUIRE_COMMITS: "false" }), fetchImpl); + assert.equal(result.release, "gittensory-rees@abc123"); + // Non-strict mode must not even CALL the commits endpoint -- confirmed below with a second regression pinning + // this exact call-skip, since a real Sentry API hiccup on /commits/ would otherwise still fail a non-strict + // deploy (sentryJson throws on any non-OK response) even though no commit check would run on the result. + assert.equal(calls.includes("/api/0/organizations/jsonbored/releases/gittensory-rees%40abc123/commits/"), false); +}); + +test("REGRESSION: validateSentryRelease never calls the commits endpoint at all when SENTRY_REQUIRE_COMMITS=false, even if that endpoint is unhealthy", async () => { + // The specific gap the AI reviewer caught on the first pass of this fix: gating only the FAILURE pushes on + // requireCommits, while leaving the fetch itself conditioned on `requireCommits || expectedCommitSha`, meant a + // non-strict deploy still depended on /commits/ succeeding even though nothing would ever fail from its result. + // Prove it directly: the endpoint returns a hard 500, and non-strict validation must still succeed. + const fetchImpl = async (input: string | URL | Request): Promise => { + const path = new URL(String(input)).pathname; + if (path.endsWith("/commits/")) return response({ detail: "internal error" }, 500); + if (path.endsWith("/deploys/")) return response([{ name: "deploy-1", environment: "production" }]); + return response({ + version: "gittensory-rees@abc123", + dateReleased: "2026-06-29T00:00:00Z", + commitCount: 1, + deployCount: 1, + projects: [{ slug: "gittensory" }], + }); + }; + + const result = await validateSentryRelease(validationEnv({ SENTRY_REQUIRE_COMMITS: "false" }), fetchImpl); + assert.equal(result.release, "gittensory-rees@abc123"); +}); + test("validateSentryRelease rejects a release missing the required deploy", async () => { const fetchImpl = async (input: string | URL | Request): Promise => { const path = new URL(String(input)).pathname;