Skip to content

Commit ec62c16

Browse files
committed
fix: address PR review findings
Reject a non-https webhook URL before curl sees it, and add --fail so an HTTP error status reaches the retries and the warning instead of exiting 0. Compare release identifiers with BigInt so two versions past 2^53 cannot round to the same float and read as a match. Drop the conditional fallbacks from the status colour test.
1 parent a13fce0 commit ec62c16

4 files changed

Lines changed: 34 additions & 7 deletions

File tree

.github/workflows/release.yml

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,15 +239,28 @@ jobs:
239239
echo "::warning::DOKPLOY_CDN_DEPLOY_WEBHOOK not set — skipping CDN redeploy."
240240
exit 0
241241
fi
242+
# The URL is itself the deploy credential, so never send it over a
243+
# scheme that puts it on the wire in cleartext. Warn rather than fail:
244+
# verify-cdn-release runs only if this job succeeds, and failing here
245+
# would drop the consistency gate instead of tripping it.
246+
case "$WEBHOOK" in
247+
https://*) ;;
248+
*)
249+
echo "::warning::DOKPLOY_CDN_DEPLOY_WEBHOOK is not an https:// URL — refusing to send the deploy credential in cleartext."
250+
exit 0
251+
;;
252+
esac
242253
# The webhook matches the branch from the request body: a bare POST
243254
# answers 301 {"message":"Branch Not Match"} and deploys nothing.
244255
#
245256
# A transient failure must never fail the workflow. npm has already
246257
# published by now and that is irreversible, so dying here buys
247258
# nothing — an earlier version of this job was deleted because a
248259
# curl exit-28 timeout failed the 0.5.0 release. verify-cdn-release
249-
# polls the manifest and is the gate that fails loudly.
250-
curl -sS -X POST "$WEBHOOK" \
260+
# polls the manifest and is the gate that fails loudly. `--fail` is
261+
# what makes an HTTP error status reach the retries and the warning
262+
# instead of exiting 0 and reading as a successful deploy.
263+
curl -sS --fail -X POST "$WEBHOOK" \
251264
-H 'Content-Type: application/json' \
252265
-d '{"ref":"refs/heads/main"}' \
253266
--retry 3 --retry-all-errors --retry-delay 10 --max-time 60 \

apps/pythinker-code/test/scripts/release/cdn-consistency.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,13 @@ describe('compareRelease', () => {
4949
expect(compareRelease(parse('0.12.1'), parse('0.12.2'))).toBeLessThan(0);
5050
expect(compareRelease(parse('0.12.0'), parse('0.12.0'))).toBe(0);
5151
});
52+
53+
it('separates identifiers that float arithmetic would round together', () => {
54+
const parse = (value: string) => /^(\d+)\.(\d+)\.(\d+)$/u.exec(value) as RegExpExecArray;
55+
// 9007199254740992 and 9007199254740993 are the same IEEE-754 double.
56+
expect(compareRelease(parse('9007199254740993.0.0'), parse('9007199254740992.0.0'))).toBe(1);
57+
expect(compareRelease(parse('0.9007199254740992.0'), parse('0.9007199254740993.0'))).toBe(-1);
58+
});
5259
});
5360

5461
describe('classifyCdnVersion', () => {

apps/pythinker-code/test/tui/components/messages/background-agent-status.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ describe('BackgroundAgentStatusComponent', () => {
6464
const previousLevel = chalk.level;
6565
chalk.level = 3;
6666
try {
67-
const startedLine = started.render(120)[1] ?? '';
68-
const completedLine = completed.render(120)[1] ?? '';
67+
const startedLine = started.render(120).join('\n');
68+
const completedLine = completed.render(120).join('\n');
6969

7070
// A running task is ambient: dim dot, dim wording, no accent colour.
7171
expect(startedLine).toContain(currentTheme.fg('textDim', STATUS_BULLET));

scripts/release/cdn-consistency.mjs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,18 @@
1515
/** Stable release semver. The CDN manifest never advertises a prerelease. */
1616
const RELEASE_SEMVER = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)$/u;
1717

18-
/** Numeric major/minor/patch compare over two RELEASE_SEMVER matches. */
18+
/**
19+
* Numeric major/minor/patch compare over two RELEASE_SEMVER matches.
20+
*
21+
* BigInt rather than Number: semver puts no ceiling on an identifier, and two
22+
* distinct versions past 2^53 would round to the same float and compare equal —
23+
* reporting a stale or impossible CDN as a match.
24+
*/
1925
export function compareRelease(left, right) {
2026
for (let index = 1; index <= 3; index += 1) {
21-
const diff = Number(left[index]) - Number(right[index]);
22-
if (diff !== 0) return diff;
27+
const a = BigInt(left[index]);
28+
const b = BigInt(right[index]);
29+
if (a !== b) return a < b ? -1 : 1;
2330
}
2431
return 0;
2532
}

0 commit comments

Comments
 (0)