From 4ae9f11ec1616813614f567a399be98569853ea6 Mon Sep 17 00:00:00 2001 From: Barnabas Nsoh Date: Fri, 14 Aug 2026 10:32:36 +0000 Subject: [PATCH 1/2] test(previews): unit-test the dev-mobile helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors smileidentity/web-sdk#71, keeping the two copies of this script in step. The bug fixed in #724 — an ERE alternation that anchored only its first branch, so ordinary app log lines aborted a healthy deploy — was pure string matching. It shipped in #651 and passed review, which is a good argument for covering this code. dev-mobile.sh could not be tested as written: its top-level code runs port preflight, `mktemp -d` and eventually a full build, tunnel and `sst dev` the moment the file is read, so sourcing it to reach a function is not an option. Move the pure helpers into scripts/lib/dev-mobile-lib.sh, which is side-effect free to source, and have the script source it. Extracted: env_file_default (now takes the file as an argument rather than reading $PREVIEWS_DIR), is_valid_port, extract_tunnel_url, sst_log_plain (likewise now takes a path), sst_log_is_ready, sst_log_has_error. The last two were previously inline greps. Behaviour is unchanged. Adds 14 node --test cases covering the regexes, the env-file parsing and port validation, matching the .test.mjs convention already used in packages/. Deliberately not covered: tunnels, the sst readiness loop and process-group cleanup — mocking cloudflared and sst would cost more than it protects. Wires `npm run test` into a previews job in lint.yml. The repo's existing node --test suites are not run by any workflow; this one is, otherwise it would not defend against the regression that prompted it. --- .github/workflows/lint.yml | 16 ++ previews/package.json | 1 + previews/scripts/dev-mobile.sh | 59 ++----- previews/scripts/dev-mobile.test.mjs | 221 +++++++++++++++++++++++++ previews/scripts/lib/dev-mobile-lib.sh | 72 ++++++++ 5 files changed, 326 insertions(+), 43 deletions(-) create mode 100644 previews/scripts/dev-mobile.test.mjs create mode 100755 previews/scripts/lib/dev-mobile-lib.sh diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index ef1032ff..3bceaf6d 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -65,3 +65,19 @@ jobs: run: npm ci - name: lint html run: npm run lint:html + previews: + runs-on: ubuntu-latest + defaults: + run: + working-directory: ./previews + steps: + - name: checkout code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: set node version + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v6 + - name: install dependencies + run: npm ci + - name: lint + run: npm run lint + - name: unit tests + run: npm run test diff --git a/previews/package.json b/previews/package.json index 6fce6dd5..ec3ef038 100644 --- a/previews/package.json +++ b/previews/package.json @@ -8,6 +8,7 @@ "dev": "sst dev react-router dev", "dev:mobile": "./scripts/dev-mobile.sh", "lint": "eslint --ignore-path .gitignore --cache --cache-location ./node_modules/.cache/eslint .", + "test": "node --test scripts/*.test.mjs", "start": "react-router-serve ./build/server/index.js", "typecheck": "react-router typegen && tsc", "sst": "sst" diff --git a/previews/scripts/dev-mobile.sh b/previews/scripts/dev-mobile.sh index 5d32c5f4..3922eb7e 100755 --- a/previews/scripts/dev-mobile.sh +++ b/previews/scripts/dev-mobile.sh @@ -18,42 +18,32 @@ set -euo pipefail -PREVIEWS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PREVIEWS_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" REPO_ROOT="$(cd "$PREVIEWS_DIR/.." && pwd)" EMBED_DIR="$REPO_ROOT/packages/embed" WEB_COMPONENTS_DIR="$REPO_ROOT/packages/web-components" +# Pure string helpers live in lib/ so they can be unit-tested without running +# this script; see dev-mobile.test.mjs. +# shellcheck source=lib/dev-mobile-lib.sh +source "$SCRIPT_DIR/lib/dev-mobile-lib.sh" + # Optionally read port defaults from previews/.env (gitignored). -# Only these two keys are read — the file is deliberately not sourced: sourcing -# it under `set -e` lets any line that returns non-zero abort the script with no -# diagnostic, and would export every variable in it (PATH, NODE_ENV, -# AWS_PROFILE, …) into the environment of `sst dev`. +# Only these two keys are read, and the file is parsed rather than sourced — +# see env_file_default for why. # Explicitly exported environment variables still take precedence. -env_file_default() { - local key=$1 file="$PREVIEWS_DIR/.env" value - [ -f "$file" ] || return 0 - value=$(grep -E "^[[:space:]]*(export[[:space:]]+)?${key}[[:space:]]*=" "$file" | tail -1 || true) - [ -n "$value" ] || return 0 - value=${value#*=} - # Ports carry no internal whitespace, so stripping it wholesale is safe; - # anything left that isn't numeric is rejected by the check below. - value=$(printf '%s' "$value" | tr -d '[:space:]') - value=${value//\"/} - value=${value//\'/} - printf '%s' "$value" -} - if [ -z "${EMBED_PORT:-}" ]; then - EMBED_PORT="$(env_file_default EMBED_PORT)" + EMBED_PORT="$(env_file_default EMBED_PORT "$PREVIEWS_DIR/.env")" fi if [ -z "${APP_PORT:-}" ]; then - APP_PORT="$(env_file_default APP_PORT)" + APP_PORT="$(env_file_default APP_PORT "$PREVIEWS_DIR/.env")" fi EMBED_PORT="${EMBED_PORT:-8000}" APP_PORT="${APP_PORT:-5173}" -if ! [[ "$EMBED_PORT" =~ ^[0-9]+$ ]] || ! [[ "$APP_PORT" =~ ^[0-9]+$ ]]; then +if ! is_valid_port "$EMBED_PORT" || ! is_valid_port "$APP_PORT"; then echo "❌ EMBED_PORT and APP_PORT must be numeric." echo " Example: EMBED_PORT=8001 APP_PORT=5174 npm run dev:mobile" exit 1 @@ -152,7 +142,7 @@ wait_for_tunnel_url() { cat "$log_file" >&2 return 1 fi - url=$(grep -Eo 'https://[a-z0-9-]+\.trycloudflare\.com' "$log_file" | head -1 || true) + url=$(extract_tunnel_url "$log_file") if [ -n "$url" ]; then echo "$url" return 0 @@ -299,15 +289,6 @@ start_bg env EmbedUrl="$EMBED_TUNNEL_URL" \ bash -c 'exec npx sst dev --mode=basic >"$1" 2>&1' _ "$SST_SERVER_LOG" SST_SERVER_PID="$LAST_BG_PID" -# Readiness is detected by scraping the log, so normalize it first: SST colorizes -# its output and redraws progress with carriage returns even when writing to a -# pipe, which leaves ANSI escapes and overwritten lines that defeat an -# end-of-line anchor. Strip the escapes, turn CRs into newlines, drop trailing -# whitespace. (`\033` rather than `\x1b` — BSD sed on macOS doesn't grok \x.) -sst_log_plain() { - sed -E $'s/\r/\\\n/g; s/\033\\[[0-9;]*[a-zA-Z]//g; s/[[:space:]]+$//' "$SST_SERVER_LOG" 2>/dev/null -} - echo " Waiting for the dev stack to finish deploying..." sst_ready="" for _ in $(seq 1 150); do @@ -322,20 +303,12 @@ for _ in $(seq 1 150); do # `set -o pipefail` would report 141 — reading a ready log as not-ready and # waiting out the whole timeout. `sst dev` streams function logs in here, so # the log does get big enough for that race to land. - sst_log=$(sst_log_plain || true) - # Wait for SST's "Complete" line (deploy done). The line must *end* at - # "Complete", so "Completed 3 files" doesn't count as ready — starting the - # client early leaves react-router unbound. - if grep -qE '(^|[[:space:]])Complete$' <<<"$sst_log"; then + sst_log=$(sst_log_plain "$SST_SERVER_LOG" || true) + if sst_log_is_ready "$sst_log"; then sst_ready=1 break fi - # Match case-sensitively and only at the start of a line: `sst dev` streams - # function logs into this file too, and an unanchored match would abort a - # healthy deploy on any app log line that merely mentions an error. - # Every alternative lives inside the group — `|` binds looser than the `^` - # anchor, so hoisting any of them out would leave it matching mid-line. - if grep -qE '^[[:space:]]*(✕|Error:|does not exist|[Ee]xpired [Tt]oken|ExpiredToken)' <<<"$sst_log"; then + if sst_log_has_error "$sst_log"; then echo "❌ sst dev server failed to start. Logs:" >&2 cat "$SST_SERVER_LOG" >&2 exit 1 diff --git a/previews/scripts/dev-mobile.test.mjs b/previews/scripts/dev-mobile.test.mjs new file mode 100644 index 00000000..a2ea9291 --- /dev/null +++ b/previews/scripts/dev-mobile.test.mjs @@ -0,0 +1,221 @@ +// Unit tests for the pure helpers behind dev-mobile.sh. +// +// These cover the string-handling that has no business failing in front of a +// contributor: env-file parsing, port validation, and the regexes that decide +// whether an `sst dev` deploy is ready, broken, or still going. Everything that +// needs a real process, port or tunnel is deliberately out of scope — mocking +// cloudflared and sst would cost more than it protects. + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const SCRIPTS_DIR = dirname(fileURLToPath(import.meta.url)); +const LIB = join(SCRIPTS_DIR, 'lib', 'dev-mobile-lib.sh'); + +// Run a snippet with the library sourced. Extra args land as $2, $3, … so test +// data never has to survive a round trip through shell quoting. +function sh(snippet, args = []) { + const argv = [ + '-c', + `set -euo pipefail; source "$1"; ${snippet}`, + '_', + LIB, + ...args, + ]; + try { + return { + stdout: execFileSync('bash', argv, { encoding: 'utf8' }), + status: 0, + }; + } catch (error) { + return { stdout: error.stdout ?? '', status: error.status }; + } +} + +// Exit status as a boolean, for the predicate helpers. +function ok(snippet, args = []) { + return sh(snippet, args).status === 0; +} + +function withTempFile(contents, run) { + const dir = mkdtempSync(join(tmpdir(), 'dev-mobile-test-')); + try { + const file = join(dir, 'fixture'); + writeFileSync(file, contents); + return run(file); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +test('sst_log_has_error ignores app log lines that merely mention an error', () => { + // Regression: `|` binds looser than `^`, so anchoring only the first group + // left these matching mid-line and aborting healthy deploys. `sst dev` + // streams function logs into the very file this predicate scans. + const healthy = [ + 'INFO lookup failed: user does not exist', + 'log: partner record does not exist, creating', + 'GET /api/token 200 - token expired token refresh ok', + 'handler: ExpiredTokenException handled gracefully downstream', + ]; + for (const line of healthy) { + assert.equal( + ok('sst_log_has_error "$2"', [line]), + false, + `should not match: ${line}`, + ); + } +}); + +test('sst_log_has_error catches genuine sst failures', () => { + const failures = [ + '✕ Failed', + ' ✕ Failed to deploy', + 'Error: Could not find an sst dev session', + 'ExpiredToken: The security token included in the request is expired', + ' Error: stage does not exist', + ]; + for (const line of failures) { + assert.equal( + ok('sst_log_has_error "$2"', [line]), + true, + `should match: ${line}`, + ); + } +}); + +test('sst_log_has_error finds a failure anywhere in a multi-line log', () => { + const log = [ + 'building...', + 'uploading assets', + '✕ Failed', + 'see above', + ].join('\n'); + assert.equal(ok('sst_log_has_error "$2"', [log]), true); +}); + +test('sst_log_is_ready requires the line to end at Complete', () => { + assert.equal(ok('sst_log_is_ready "$2"', ['| Complete']), true); + assert.equal(ok('sst_log_is_ready "$2"', ['Complete']), true); + // Starting the react-router client early leaves it unbound, so a progress + // line about copying files must not read as ready. + assert.equal(ok('sst_log_is_ready "$2"', ['Completed 3 files']), false); + assert.equal(ok('sst_log_is_ready "$2"', ['Incomplete']), false); +}); + +test('sst_log_is_ready finds the line inside a longer log', () => { + const log = [ + 'deploying', + 'PreviewApp sst:aws:React', + '| Complete', + ' url: https://x', + ].join('\n'); + assert.equal(ok('sst_log_is_ready "$2"', [log]), true); +}); + +test('sst_log_plain strips ANSI escapes, carriage returns and trailing space', () => { + // A colorized, CR-redrawn "Complete" line: unnormalized it defeats the + // end-of-line anchor that sst_log_is_ready relies on. + const raw = 'building\rdeploying\r\x1b[32m| Complete\x1b[0m \n'; + withTempFile(raw, (file) => { + const { stdout } = sh('sst_log_plain "$2"', [file]); + assert.ok(!stdout.includes('\x1b'), 'ANSI escapes should be gone'); + assert.ok(!stdout.includes('\r'), 'carriage returns should be gone'); + assert.ok( + stdout.split('\n').some((line) => line === '| Complete'), + `expected a bare "| Complete" line, got ${JSON.stringify(stdout)}`, + ); + }); +}); + +test('sst_log_plain output is what makes a redrawn ready line detectable', () => { + const raw = 'progress\r\x1b[32m| Complete\x1b[0m \n'; + withTempFile(raw, (file) => { + const { stdout } = sh('sst_log_plain "$2"', [file]); + assert.equal(ok('sst_log_is_ready "$2"', [stdout]), true); + }); +}); + +test('env_file_default reads plain and exported assignments', () => { + withTempFile('EMBED_PORT=9000\n', (file) => { + assert.equal(sh('env_file_default EMBED_PORT "$2"', [file]).stdout, '9000'); + }); + withTempFile('export APP_PORT = "5180"\n', (file) => { + assert.equal(sh('env_file_default APP_PORT "$2"', [file]).stdout, '5180'); + }); + withTempFile(" EMBED_PORT = '8123' \n", (file) => { + assert.equal(sh('env_file_default EMBED_PORT "$2"', [file]).stdout, '8123'); + }); +}); + +test('env_file_default takes the last assignment', () => { + withTempFile('EMBED_PORT=8000\nEMBED_PORT=8001\n', (file) => { + assert.equal(sh('env_file_default EMBED_PORT "$2"', [file]).stdout, '8001'); + }); +}); + +test('env_file_default returns empty for an absent key, file or commented line', () => { + withTempFile('APP_PORT=5173\n', (file) => { + assert.equal(sh('env_file_default EMBED_PORT "$2"', [file]).stdout, ''); + }); + withTempFile('# EMBED_PORT=9000\n', (file) => { + assert.equal(sh('env_file_default EMBED_PORT "$2"', [file]).stdout, ''); + }); + const missing = join(tmpdir(), 'dev-mobile-test-does-not-exist'); + assert.equal(sh('env_file_default EMBED_PORT "$2"', [missing]).stdout, ''); +}); + +test('env_file_default does not match a key that merely shares a prefix', () => { + withTempFile('EMBED_PORT_OLD=1234\n', (file) => { + // A prefix hit would return "_OLD=1234" stripped to junk, which + // is_valid_port then rejects — surfacing as a confusing failure. + const value = sh('env_file_default EMBED_PORT "$2"', [file]).stdout; + assert.equal(ok('is_valid_port "$2"', [value]) && value !== '', false); + }); +}); + +test('is_valid_port accepts digits only', () => { + for (const good of ['8000', '5173', '0', '65535']) { + assert.equal( + ok('is_valid_port "$2"', [good]), + true, + `should accept ${good}`, + ); + } + for (const bad of ['abc', '80a0', '', '-1', '80.5', '8000 ']) { + assert.equal( + ok('is_valid_port "$2"', [bad]), + false, + `should reject ${JSON.stringify(bad)}`, + ); + } +}); + +test('extract_tunnel_url returns the first quick-tunnel URL', () => { + const log = [ + 'INF Requesting new quick tunnel', + '| https://mild-tapir-quiet.trycloudflare.com |', + '| https://second-one-here.trycloudflare.com |', + ].join('\n'); + withTempFile(log, (file) => { + assert.equal( + sh('extract_tunnel_url "$2"', [file]).stdout.trim(), + 'https://mild-tapir-quiet.trycloudflare.com', + ); + }); +}); + +test('extract_tunnel_url is quiet and successful when no URL has appeared yet', () => { + withTempFile('INF Starting tunnel\n', (file) => { + const { stdout, status } = sh('extract_tunnel_url "$2"', [file]); + assert.equal(stdout.trim(), ''); + // Must not fail: the caller polls this in a loop under `set -o pipefail`, + // so a non-zero exit before the URL lands would kill the script. + assert.equal(status, 0); + }); +}); diff --git a/previews/scripts/lib/dev-mobile-lib.sh b/previews/scripts/lib/dev-mobile-lib.sh new file mode 100755 index 00000000..e789c3f0 --- /dev/null +++ b/previews/scripts/lib/dev-mobile-lib.sh @@ -0,0 +1,72 @@ +#!/usr/bin/env bash +# Pure helpers for dev-mobile.sh. +# +# These live here rather than inline so they can be unit-tested from +# previews/scripts/dev-mobile.test.mjs. dev-mobile.sh itself cannot be sourced +# for that purpose: its top-level code runs port preflight, `mktemp -d` and +# eventually a full build + tunnel + `sst dev` the moment the file is read. +# +# Everything in here must stay side-effect free — no globals read, no processes +# started, no files written. Inputs arrive as arguments; results come back on +# stdout or as an exit status. That property is what makes the tests cheap, so +# please keep new helpers to the same standard. + +# Read one key's value out of an env file, without sourcing it. +# +# Sourcing is deliberately avoided: under `set -e` any line in the file that +# returns non-zero aborts the caller with no diagnostic, and it would export +# every variable in the file (PATH, NODE_ENV, AWS_PROFILE, …) into the +# environment of whatever runs next. +# +# Accepts `KEY=value` and `export KEY = "value"`, with surrounding whitespace +# and quotes. The last assignment wins. Prints nothing if the file or key is +# absent, so callers can fall back with `${VAR:-default}`. +env_file_default() { + local key=$1 file=$2 value + [ -f "$file" ] || return 0 + value=$(grep -E "^[[:space:]]*(export[[:space:]]+)?${key}[[:space:]]*=" "$file" | tail -1 || true) + [ -n "$value" ] || return 0 + value=${value#*=} + # Ports carry no internal whitespace, so stripping it wholesale is safe; + # anything left that isn't numeric is rejected by is_valid_port. + value=$(printf '%s' "$value" | tr -d '[:space:]') + value=${value//\"/} + value=${value//\'/} + printf '%s' "$value" +} + +is_valid_port() { + [[ "$1" =~ ^[0-9]+$ ]] +} + +# First cloudflare quick-tunnel URL published into a log, if any. +extract_tunnel_url() { + grep -Eo 'https://[a-z0-9-]+\.trycloudflare\.com' "$1" | head -1 || true +} + +# Normalize an sst log for scraping. SST colorizes its output and redraws +# progress with carriage returns even when writing to a pipe, which leaves ANSI +# escapes and overwritten lines that defeat an end-of-line anchor. Strip the +# escapes, turn CRs into newlines, drop trailing whitespace. (`\033` rather +# than `\x1b` — BSD sed on macOS doesn't grok \x.) +sst_log_plain() { + sed -E $'s/\r/\\\n/g; s/\033\\[[0-9;]*[a-zA-Z]//g; s/[[:space:]]+$//' "$1" 2>/dev/null +} + +# Deploy finished. The line must *end* at "Complete", so "Completed 3 files" +# doesn't count as ready — starting the client early leaves react-router +# unbound. +sst_log_is_ready() { + grep -qE '(^|[[:space:]])Complete$' <<<"$1" +} + +# Deploy failed. Match case-sensitively and only at the start of a line: `sst +# dev` streams function logs into the same file, so an unanchored match would +# abort a healthy deploy on any app log line that merely mentions an error. +# +# Every alternative lives inside the group — `|` binds looser than the `^` +# anchor, so hoisting any of them out would leave it matching mid-line. That is +# exactly the bug the tests for this function pin down; don't "simplify" it. +sst_log_has_error() { + grep -qE '^[[:space:]]*(✕|Error:|does not exist|[Ee]xpired [Tt]oken|ExpiredToken)' <<<"$1" +} From c0c83e56dd1da1953e8cf2b197fe09b7b9feef89 Mon Sep 17 00:00:00 2001 From: Barnabas A Nsoh Date: Fri, 14 Aug 2026 15:54:12 +0000 Subject: [PATCH 2/2] Potential fix for pull request finding 'CodeQL / Workflow does not contain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .github/workflows/lint.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 3bceaf6d..750754af 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -5,6 +5,8 @@ on: - main pull_request: workflow_dispatch: +permissions: + contents: read jobs: main: runs-on: ubuntu-latest