From 1f1b2d731b3eb5835df3ee44ad0e2d399655d43d Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Mon, 13 Apr 2026 16:30:34 -0700 Subject: [PATCH] =?UTF-8?q?chore:=20address=20follow-up=20review=20?= =?UTF-8?q?=E2=80=94=20dead=20helpers,=20lib=20polish,=20CI=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last sweep of code-review items before merging PR #5. ## _lib.sh cleanup - Removed unused e2e_register and e2e_heartbeat helpers (dead code — no caller ever invoked them) - Standardized on $BASE variable set via : "${BASE:=...}" so every script uses one name (was mixed $BASE / $e2e_base) - e2e_extract_token now writes stderr warnings on JSON parse failure or missing auth_token, instead of silently returning empty. Previous behavior made downstream "missing workspace auth token" 401s much harder to diagnose ## Script cleanup - test_api.sh, test_comprehensive_e2e.sh, test_activity_e2e.sh all drop the redundant `e2e_base + BASE="$e2e_base"` aliasing; sourcing _lib.sh sets BASE via : "${BASE:=...}" default ## CI hardening (.github/workflows/ci.yml) - Postgres credentials now match .env.example (dev:dev — was molecule:molecule, caused confusion for local repros) - Added Go module cache via actions/setup-go cache:true + cache-dependency-path: platform/go.sum. ~30s cold-run improvement - New pre-E2E step asserts migrations actually ran by checking for the 'workspaces' table. Catches future migration-author mistakes before they surface as obscure E2E failures ## Follow-up issue Filed Molecule-AI/molecule-monorepo#6 for the deterministic token- mint admin endpoint. PR #5 uses an empirical "beat the container" race (5/5 wins in benchmarks); issue #6 tracks the real fix for any future CI load that invalidates the assumption. ## Verification - bash tests/e2e/test_api.sh -> 62/62 - bash tests/e2e/test_comprehensive_e2e.sh -> 67/67 - python3 -c "import yaml; yaml.safe_load(open('.github/workflows/ci.yml'))" -> ok ## Operational note Hourly PR-triage + issue-pickup cron scheduled this session (job id 0328bc8f, fires at :17 past each hour). Runtime reports it as session-only despite durable:true — re-invoke via /loop or CronCreate in a fresh session if needed. Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/ci.yml | 25 +++++++++++--- tests/e2e/_lib.sh | 53 ++++++++++++++--------------- tests/e2e/test_activity_e2e.sh | 4 +-- tests/e2e/test_api.sh | 4 +-- tests/e2e/test_comprehensive_e2e.sh | 4 +-- 5 files changed, 50 insertions(+), 40 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90b0dce7..34a7a7f0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,15 +79,17 @@ jobs: timeout-minutes: 10 services: postgres: + # Credentials match .env.example (dev:dev) so local reproduction is + # identical to CI. POSTGRES_DB matches the default there too. image: postgres:16 env: - POSTGRES_USER: molecule - POSTGRES_PASSWORD: molecule + POSTGRES_USER: dev + POSTGRES_PASSWORD: dev POSTGRES_DB: molecule ports: - 5432:5432 options: >- - --health-cmd "pg_isready -U molecule" + --health-cmd "pg_isready -U dev" --health-interval 10s --health-timeout 5s --health-retries 5 @@ -101,7 +103,7 @@ jobs: --health-timeout 5s --health-retries 5 env: - DATABASE_URL: postgres://molecule:molecule@localhost:5432/molecule?sslmode=disable + DATABASE_URL: postgres://dev:dev@localhost:5432/molecule?sslmode=disable REDIS_URL: redis://localhost:6379 PORT: "8080" steps: @@ -109,6 +111,8 @@ jobs: - uses: actions/setup-go@v5 with: go-version: 'stable' + cache: true + cache-dependency-path: platform/go.sum - name: Build platform working-directory: platform run: go build -o platform-server ./cmd/server @@ -129,6 +133,19 @@ jobs: echo "::error::Platform did not become healthy in 30s" cat platform/platform.log || true exit 1 + - name: Assert migrations applied + # Migrations auto-run at platform boot. Fail fast if they silently + # didn't — catches future migration-author mistakes (e.g. a new + # privileged op Postgres "dev" can't execute) before the E2E run. + run: | + sudo apt-get install -y -qq postgresql-client > /dev/null + tables=$(PGPASSWORD=dev psql -h localhost -U dev -d molecule -tAc "SELECT count(*) FROM information_schema.tables WHERE table_schema='public' AND table_name='workspaces'") + if [ "$tables" != "1" ]; then + echo "::error::Migrations did not apply — 'workspaces' table missing" + cat platform/platform.log || true + exit 1 + fi + echo "Migrations OK (workspaces table present)" - name: Run E2E API tests run: bash tests/e2e/test_api.sh - name: Dump platform log on failure diff --git a/tests/e2e/_lib.sh b/tests/e2e/_lib.sh index f7059d74..4efd2da9 100755 --- a/tests/e2e/_lib.sh +++ b/tests/e2e/_lib.sh @@ -3,44 +3,43 @@ # # Usage: # source "$(dirname "$0")/_lib.sh" -# e2e_base="http://localhost:8080" # e2e_cleanup_all_workspaces # call at top of script -# token=$(e2e_register "$ID" "$URL" "$CARD_JSON") -# # then use -H "Authorization: Bearer $token" on heartbeat/update-card +# TOKEN=$(echo "$register_response" | e2e_extract_token) +# +# BASE defaults to http://localhost:8080. Set it before sourcing to override. -# Emit the auth_token from a /registry/register response. Prints empty -# string (not an error) when no token was issued so callers can still -# exercise the grandfather path. +: "${BASE:=http://localhost:8080}" +export BASE + +# Emit the auth_token from a /registry/register response on stdout. +# Logs a warning to stderr when the JSON parse fails or the token is +# missing — silent empty strings masked real failures as the +# downstream "missing workspace auth token" 401. Return value is +# still empty-string-on-failure so grandfather-path callers work. e2e_extract_token() { - python3 -c "import sys,json; print(json.load(sys.stdin).get('auth_token',''))" 2>/dev/null || true -} - -# Register a workspace and echo the bearer token on stdout. -# Args: $1 workspace_id $2 url $3 agent_card JSON -e2e_register() { - curl -s -X POST "$e2e_base/registry/register" \ - -H "Content-Type: application/json" \ - -d "{\"id\":\"$1\",\"url\":\"$2\",\"agent_card\":$3}" \ - | e2e_extract_token -} - -# Heartbeat with bearer auth. -# Args: $1 workspace_id $2 token $3 payload_json (without the id) -e2e_heartbeat() { - curl -s -X POST "$e2e_base/registry/heartbeat" \ - -H "Content-Type: application/json" \ - -H "Authorization: Bearer $2" \ - -d "$3" + python3 -c " +import sys, json +try: + data = json.load(sys.stdin) +except Exception as e: + sys.stderr.write(f'e2e_extract_token: invalid JSON response ({e})\n') + print('') + sys.exit(0) +tok = data.get('auth_token', '') +if not tok: + sys.stderr.write('e2e_extract_token: response contained no auth_token field\n') +print(tok) +" } # Delete every workspace currently on the platform. Use at the top of a # script so count-based assertions are reproducible across runs. e2e_cleanup_all_workspaces() { - for _wid in $(curl -s "$e2e_base/workspaces" | python3 -c "import json,sys + for _wid in $(curl -s "$BASE/workspaces" | python3 -c "import json,sys try: [print(w['id']) for w in json.load(sys.stdin)] except Exception: pass" 2>/dev/null); do - curl -s -X DELETE "$e2e_base/workspaces/$_wid?confirm=true" > /dev/null || true + curl -s -X DELETE "$BASE/workspaces/$_wid?confirm=true" > /dev/null || true done } diff --git a/tests/e2e/test_activity_e2e.sh b/tests/e2e/test_activity_e2e.sh index 0de1de25..30f4fc01 100755 --- a/tests/e2e/test_activity_e2e.sh +++ b/tests/e2e/test_activity_e2e.sh @@ -3,9 +3,7 @@ # Requires: platform running on localhost:8080 with at least one online agent. set -euo pipefail -source "$(dirname "$0")/_lib.sh" -e2e_base="http://localhost:8080" -BASE="$e2e_base" +source "$(dirname "$0")/_lib.sh" # sets BASE default PASS=0 FAIL=0 TIMEOUT="${A2A_TIMEOUT:-120}" diff --git a/tests/e2e/test_api.sh b/tests/e2e/test_api.sh index bf8141fc..00ba6bd1 100644 --- a/tests/e2e/test_api.sh +++ b/tests/e2e/test_api.sh @@ -1,9 +1,7 @@ #!/usr/bin/env bash set -euo pipefail -source "$(dirname "$0")/_lib.sh" -e2e_base="http://localhost:8080" -BASE="$e2e_base" +source "$(dirname "$0")/_lib.sh" # sets BASE default PASS=0 FAIL=0 diff --git a/tests/e2e/test_comprehensive_e2e.sh b/tests/e2e/test_comprehensive_e2e.sh index f6ecd44f..bef9f704 100755 --- a/tests/e2e/test_comprehensive_e2e.sh +++ b/tests/e2e/test_comprehensive_e2e.sh @@ -6,9 +6,7 @@ # Does NOT require running agent containers (tests platform-only behavior). set -euo pipefail -source "$(dirname "$0")/_lib.sh" -e2e_base="http://localhost:8080" -BASE="$e2e_base" +source "$(dirname "$0")/_lib.sh" # sets BASE default PASS=0 FAIL=0 SKIP=0