From 63e63f03928daae5b53b33fa7f8559e80519cbed Mon Sep 17 00:00:00 2001 From: eldonm Date: Sat, 8 Aug 2026 11:31:12 -0400 Subject: [PATCH 1/2] fix(scripts): make postgres smoke run env-correct and concurrency-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three script-only defects made smoke_postgres.sh report failures that had nothing to do with the code under test. 1. Interpreter split. PYBIN came from $PYTHON (scripts/smoke_postgres.sh:43) but the checks shelled out to a bare `jvagent` off PATH (:96, :125). On a host with more than one install those are different environments; here PATH resolved to one whose jvspatial was an editable sibling checkout without asyncpg, so bootstrap died with "ImportError: asyncpg is required for the Postgres backend" and 8 checks failed spuriously. The entrypoint is now derived from PYBIN — $(dirname $PYBIN)/jvagent, falling back to "$PYBIN -m jvagent" — and used everywhere. 2. Overlapping runs killed each other. Container name and port were fixed and teardown was `pkill -f "jvagent $APP_ROOT"` (:54, :104), which matches every concurrent run's server: an earlier run's EXIT trap terminated a later run's server mid-restart, yielding "server failed to restart", "login failed post-restart" and a bogus event-loop error count. Container name and both ports are now per-run unique (PID-seeded, then probed for a free port), and the server is tracked and killed by PID instead of by name pattern. 3. Logs deleted on failure. The EXIT trap ran `rm -rf "$WORKDIR"` unconditionally (:56), so the bootstrap.log / server_*.log a failure message pointed at were already gone. The workdir is now preserved and its path printed whenever any check failed. Verified: 15/15 with no PATH manipulation (PYTHON=.venv/bin/python), and two deliberately overlapping runs no longer interfere. --- scripts/smoke_postgres.sh | 90 ++++++++++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 26 deletions(-) diff --git a/scripts/smoke_postgres.sh b/scripts/smoke_postgres.sh index 3bb7e814..3a9fd52c 100755 --- a/scripts/smoke_postgres.sh +++ b/scripts/smoke_postgres.sh @@ -21,7 +21,13 @@ # (`pip install asyncpg`). Agent-turn checks need a model key in the app's # .env; without one they are skipped and the persistence checks still run. # -# Exits non-zero with the number of failed checks. See docs/postgres.md. +# The jvagent entrypoint is derived from $PYTHON (default python3) so the run +# exercises the same environment as PYBIN, not whatever `jvagent` PATH happens +# to resolve to. Container name and ports are unique per run, and the server is +# tracked by PID, so concurrent runs do not kill each other. +# +# Exits non-zero with the number of failed checks. The temp workdir (bootstrap +# and server logs) is kept when any check fails. See docs/postgres.md. set -u @@ -30,18 +36,36 @@ USE_DOCKER=1 for arg in "$@"; do case "$arg" in --no-docker) USE_DOCKER=0 ;; - -h|--help) sed -n '2,25p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; + -h|--help) sed -n '2,30p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; *) APP_ROOT="$arg" ;; esac done -CONTAINER="${JVAGENT_SMOKE_CONTAINER:-jvagent-smoke-pg}" -PGPORT="${JVAGENT_SMOKE_PGPORT:-55432}" -PORT="${JVAGENT_SMOKE_PORT:-8123}" -BASE="http://127.0.0.1:$PORT" WORKDIR="$(mktemp -d)" PYBIN="${PYTHON:-python3}" +# Run the *same* install PYBIN belongs to. A bare `jvagent` off PATH can be a +# different environment entirely (different jvspatial, missing asyncpg), which +# turns an environment problem into a wall of bogus check failures. +PYBIN_PATH="$(command -v "$PYBIN" 2>/dev/null || printf '%s' "$PYBIN")" +if [ -x "$(dirname "$PYBIN_PATH")/jvagent" ]; then + JVAGENT_CMD=("$(dirname "$PYBIN_PATH")/jvagent") +else + JVAGENT_CMD=("$PYBIN" -m jvagent) +fi + +# Everything externally visible is per-run unique so two overlapping runs do not +# fight over a container name, a port, or each other's server process. +RUN_ID=$$ +port_in_use() { (exec 3<>"/dev/tcp/127.0.0.1/$1") >/dev/null 2>&1; } +pick_port() { local p=$1; while port_in_use "$p"; do p=$((p + 1)); done; printf '%s' "$p"; } + +CONTAINER="${JVAGENT_SMOKE_CONTAINER:-jvagent-smoke-pg-$RUN_ID}" +PGPORT="${JVAGENT_SMOKE_PGPORT:-$(pick_port $((55432 + RUN_ID % 1000)))}" +PORT="${JVAGENT_SMOKE_PORT:-$(pick_port $((8123 + RUN_ID % 1000)))}" +BASE="http://127.0.0.1:$PORT" +SERVER_PID="" + pass=0 fail=0 skip=0 @@ -50,10 +74,40 @@ ok() { printf 'PASS %s\n' "$1"; pass=$((pass + 1)); } bad() { printf 'FAIL %s\n' "$1"; fail=$((fail + 1)); } note() { printf 'SKIP %s\n' "$1"; skip=$((skip + 1)); } +start_server() { + nohup "${JVAGENT_CMD[@]}" "$APP_ROOT" > "$WORKDIR/server_$1.log" 2>&1 & + SERVER_PID=$! + for _ in $(seq 1 30); do + curl -s -m 2 "$BASE/health" >/dev/null 2>&1 && return 0 + kill -0 "$SERVER_PID" 2>/dev/null || return 1 # died before serving + sleep 3 + done + return 1 +} +# Kill by PID, never `pkill -f jvagent ` — that pattern matches every +# concurrent run's server too. The trailing redirect swallows bash's +# "Terminated: 15" job report, which is expected here, not a failure. +stop_server() { + [ -n "$SERVER_PID" ] || return 0 + pkill -P "$SERVER_PID" + kill "$SERVER_PID" + for _ in $(seq 1 15); do + kill -0 "$SERVER_PID" 2>/dev/null || break + sleep 1 + done + kill -9 "$SERVER_PID" + wait "$SERVER_PID" + SERVER_PID="" +} 2>/dev/null + cleanup() { - pkill -f "jvagent $APP_ROOT" 2>/dev/null + stop_server [ "$USE_DOCKER" = "1" ] && docker rm -f "$CONTAINER" >/dev/null 2>&1 - rm -rf "$WORKDIR" + if [ "$fail" -gt 0 ]; then + printf '\nlogs kept: %s\n' "$WORKDIR" + else + rm -rf "$WORKDIR" + fi } trap cleanup EXIT @@ -92,22 +146,6 @@ export JVSPATIAL_ENVIRONMENT=development psql_q() { docker exec "$CONTAINER" psql -U jvagent -d jvagent_smoke -tA -c "$1" 2>/dev/null; } -start_server() { - nohup jvagent "$APP_ROOT" > "$WORKDIR/server_$1.log" 2>&1 & - for _ in $(seq 1 30); do - curl -s -m 2 "$BASE/health" >/dev/null 2>&1 && return 0 - sleep 3 - done - return 1 -} -stop_server() { - pkill -f "jvagent $APP_ROOT" 2>/dev/null - for _ in $(seq 1 15); do - pgrep -f "jvagent $APP_ROOT" >/dev/null || return 0 - sleep 1 - done -} - login() { curl -s -m 15 -X POST "$BASE/api/auth/login" -H 'Content-Type: application/json' \ -d "{\"email\":\"$JVAGENT_ADMIN_EMAIL\",\"password\":\"$JVAGENT_ADMIN_PASSWORD\"}" | @@ -122,11 +160,11 @@ say() { # token agent utterance } step "1. bootstrap graph onto Postgres" -if jvagent "$APP_ROOT" bootstrap > "$WORKDIR/bootstrap.log" 2>&1; then +if "${JVAGENT_CMD[@]}" "$APP_ROOT" bootstrap > "$WORKDIR/bootstrap.log" 2>&1; then ok "jvagent bootstrap" else bad "jvagent bootstrap — see $WORKDIR/bootstrap.log" - grep -E "ValueError|Unsupported database type" "$WORKDIR/bootstrap.log" | head -2 + grep -E "ValueError|Unsupported database type|ImportError" "$WORKDIR/bootstrap.log" | head -2 fi if [ "$USE_DOCKER" = "1" ]; then From 77fb5d72300831d1ed3c070dc0741225c087cb59 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Sat, 8 Aug 2026 12:08:39 -0400 Subject: [PATCH 2/2] fix(scripts): anchor the smoke app root to the repo, and reject scaffolding replies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from running the concurrency/interpreter fix in anger. APP_ROOT defaulted to the cwd-relative "examples/jvagent_app". The app's .env is gitignored, so running the script from a worktree — or anywhere that is not the checkout root — silently picks up a different app, or a keyless one. Resolve it from BASH_SOURCE instead so the default always means "this repo's example app". An explicit positional argument still overrides it. The turn check tested only that the reply was non-empty. Without a usable model key the agent still answers — by echoing the prompt scaffolding it was handed — so the check passed on output that proved nothing, TURNS stayed 1, and the run came apart two steps later at the recall check with a failure that pointed at Postgres rather than at the missing key. That is how a 15/15 suite reported "14 passed, 1 failed" for a reason that had nothing to do with the database. reply_is_substantive() rejects the scaffolding markers and a too-short reply. A scaffolding reply now demotes the turn to SKIP, names the likely cause, and clears TURNS so the dependent persistence and recall checks skip with it. Verified three ways against merged main, each isolating one behavior: - keyed app, explicit root, cwd=/tmp -> 15 passed, 0 failed, 0 skipped - keyless app (worktree), default root -> 12 passed, 0 failed, 2 skipped - keyless app, default root, cwd=/tmp -> 12 passed, 0 failed, 2 skipped The last two are the point: identical results from inside and outside the repo, and zero failures where the same condition previously produced a misleading one. --- scripts/smoke_postgres.sh | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/scripts/smoke_postgres.sh b/scripts/smoke_postgres.sh index 3a9fd52c..2934d11b 100755 --- a/scripts/smoke_postgres.sh +++ b/scripts/smoke_postgres.sh @@ -31,7 +31,13 @@ set -u -APP_ROOT="examples/jvagent_app" +# Resolve the default app relative to the REPO, not the caller's cwd. The app's +# .env is gitignored, so a cwd-relative default silently yields a keyless app +# when the script is run from a worktree or anywhere else — and a keyless run +# still answers, with prompt scaffolding, so it fails three steps later at the +# recall check instead of here. +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +APP_ROOT="$REPO_ROOT/examples/jvagent_app" USE_DOCKER=1 for arg in "$@"; do case "$arg" in @@ -189,15 +195,32 @@ AGENT=$(curl -s -m 15 "$BASE/api/agents" -H "Authorization: Bearer $TOK" | "$PYBIN" -c 'import sys,json; a=json.load(sys.stdin).get("agents",[]); print(a[0]["id"] if a else "")' 2>/dev/null) [ -n "$AGENT" ] && ok "agent listed ($AGENT)" || bad "no agents returned" +# A reply is not automatically an answer. Without a model key the agent still +# responds — by echoing the prompt scaffolding it was handed — so a bare +# non-empty test passes on output that proves nothing, and the run only comes +# apart at the recall check two steps later. Reject the scaffolding markers and +# require the agent to have actually taken the utterance. +reply_is_substantive() { + case "$1" in + *"MANDATORY directive"*|*"[Deliver every"*|*"User message:"*) return 1 ;; + esac + [ "${#1}" -ge 12 ] +} + step "4. agent turn" TURNS=1 r1=$(say "$TOK" "$AGENT" "Remember the number 8675309.") -if [ -n "$r1" ]; then +if [ -z "$r1" ]; then + TURNS=0 + note "agent turn — no reply (model key missing?); persistence checks continue" +elif reply_is_substantive "$r1"; then printf 'agent: %s\n' "$r1" ok "turn produced a reply" else TURNS=0 - note "agent turn — no reply (model key missing?); persistence checks continue" + printf 'agent: %s\n' "$r1" + note "agent turn — reply was prompt scaffolding, not an answer (model key \ +missing or misconfigured?); persistence checks continue" fi if [ "$USE_DOCKER" = "1" ] && [ "$TURNS" = "1" ]; then