From 1e2c9d91b9495e4f2801944d153902fa2768bf9c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 23:47:38 -0700 Subject: [PATCH 1/2] fix(rees): gate the expected-commit check on SENTRY_REQUIRE_COMMITS validateSentryRelease's "release commits do not include expected commit" check read only config.expectedCommitSha, never config.requireCommits, so it fired regardless of strict mode. upload-sourcemaps.ts always passes SENTRY_COMMIT_SHA (the deploy's actual git SHA, not itself a strictness signal), so this check was effectively always enforced even when strict was explicitly off, causing recurring release-validation failures in non-strict deploys (Sentry GITTENSORY-X). --- .../scripts/validate-sentry-release.mjs | 2 +- .../test/sentry-release-validation.test.ts | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/review-enrichment/scripts/validate-sentry-release.mjs b/review-enrichment/scripts/validate-sentry-release.mjs index 6391fdfbf0..eb796ce451 100644 --- a/review-enrichment/scripts/validate-sentry-release.mjs +++ b/review-enrichment/scripts/validate-sentry-release.mjs @@ -214,7 +214,7 @@ export async function validateSentryRelease(env = process.env, fetchImpl = globa if (config.requireCommits && commitCount <= 0 && commitIds.length === 0) { failures.push("release has no associated commits"); } - if (config.expectedCommitSha && !commitMatches(config.expectedCommitSha, commitIds)) { + if (config.requireCommits && config.expectedCommitSha && !commitMatches(config.expectedCommitSha, commitIds)) { failures.push(`release commits do not include expected commit ${config.expectedCommitSha}`); } } diff --git a/review-enrichment/test/sentry-release-validation.test.ts b/review-enrichment/test/sentry-release-validation.test.ts index 1f898293b3..c613f99b16 100644 --- a/review-enrichment/test/sentry-release-validation.test.ts +++ b/review-enrichment/test/sentry-release-validation.test.ts @@ -119,6 +119,29 @@ 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 fetchImpl = async (input: string | URL | Request): Promise => { + const path = new URL(String(input)).pathname; + 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"); +}); + 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; From fed0d6a5aa1dbdee6f4200870d31649347f27870 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 00:27:18 -0700 Subject: [PATCH 2/2] fix(rees): skip the commits fetch entirely in non-strict release validation The prior fix only gated the failure conditions on requireCommits, leaving the outer block's fetch to /commits/ conditioned on `requireCommits || expectedCommitSha`. Since expectedCommitSha is essentially always set (upload-sourcemaps.ts always passes the deploy's actual git SHA), a non-strict deploy still depended on the commits endpoint succeeding even though nothing would ever fail from its result. Gate the fetch on requireCommits alone. --- .../scripts/validate-sentry-release.mjs | 12 ++++++-- .../test/sentry-release-validation.test.ts | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/review-enrichment/scripts/validate-sentry-release.mjs b/review-enrichment/scripts/validate-sentry-release.mjs index eb796ce451..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,10 +217,10 @@ 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.requireCommits && config.expectedCommitSha && !commitMatches(config.expectedCommitSha, commitIds)) { + if (config.expectedCommitSha && !commitMatches(config.expectedCommitSha, commitIds)) { failures.push(`release commits do not include expected commit ${config.expectedCommitSha}`); } } diff --git a/review-enrichment/test/sentry-release-validation.test.ts b/review-enrichment/test/sentry-release-validation.test.ts index c613f99b16..74a5c3d2cd 100644 --- a/review-enrichment/test/sentry-release-validation.test.ts +++ b/review-enrichment/test/sentry-release-validation.test.ts @@ -125,8 +125,10 @@ test("REGRESSION: validateSentryRelease does NOT enforce the expected-commit mat // "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({ @@ -138,6 +140,32 @@ test("REGRESSION: validateSentryRelease does NOT enforce the expected-commit mat }); }; + 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"); });