From 6c02c905c8cdd4ab0b5f4f5a10fa508a963bb89c Mon Sep 17 00:00:00 2001 From: root Date: Thu, 30 Apr 2026 06:27:24 -0500 Subject: [PATCH] scrum lift_001: 4 fixes from cross-lineage review Cross-lineage scrum on b2e45f7 produced 1 convergent + 3 single-reviewer findings worth fixing. All apply. 1. (Opus WARN + Qwen INFO convergent) scripts/playbook_lift.sh: replace sleep 2.5 in SQL probe with active polling up to 5s. refresh_every=1s is a lower bound; under load the manifest may not be visible in a fixed sleep, which would 4xx the probe and abort the reality run. 2. (Opus INFO) scripts/playbook_lift.sh: report template glued "env JUDGE_MODEL" + value as "env JUDGE_MODELqwen2.5:latest" with no separator. Replaced two :+/:- substitution chains with a single JUDGE_SOURCE variable computed once at the top of the harness. 3. (Opus INFO) scripts/staffing_workers/main.go: -id-prefix "" silently allowed, defeating the flag's purpose (cross-corpus collision prevent). Now log.Fatal at startup with explicit hint. 4. (Opus WARN) cmd/{pathwayd,observerd}/main_test.go: newTestRouter returned http.Handler then re-cast to chi.Router for chi.Walk. Returning chi.Router directly satisfies http.Handler AND avoids an assertion that would panic if future middleware wraps the router. Dismissed (with rationale): - Kimi INFO hardcoded MinIO endpoint: harness is local-by-design. - Kimi WARN matrixd accepts 502/500: documented; real retriever needs real upstreams the test doesn't spin up. - Qwen INFO queryd string.Contains: brittle but very low risk; restating through typed-error path would couple without adding signal. go test ./cmd/{matrixd,queryd,pathwayd,observerd} all green. Verdicts at reports/scrum/_evidence/2026-04-30/verdicts/lift_001_*.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/observerd/main_test.go | 9 +++++--- cmd/pathwayd/main_test.go | 9 +++++--- scripts/playbook_lift.sh | 37 +++++++++++++++++++++++--------- scripts/staffing_workers/main.go | 6 ++++++ 4 files changed, 45 insertions(+), 16 deletions(-) diff --git a/cmd/observerd/main_test.go b/cmd/observerd/main_test.go index a1c7ef7..2911dd2 100644 --- a/cmd/observerd/main_test.go +++ b/cmd/observerd/main_test.go @@ -15,7 +15,11 @@ import ( // newTestRouter builds the observerd router with an in-memory store // and a workflow runner with no modes registered. Closes R-005 for // observerd. -func newTestRouter(t *testing.T) http.Handler { +// +// Returns chi.Router (not http.Handler) so chi.Walk works without a +// type assertion that would panic if a future refactor wraps the +// router in plain net/http middleware. +func newTestRouter(t *testing.T) chi.Router { t.Helper() h := &handlers{ store: observer.NewStore(nil), @@ -34,8 +38,7 @@ func TestRoutesMounted(t *testing.T) { "POST /observer/workflow/run": false, "GET /observer/workflow/modes": false, } - router := r.(chi.Router) - _ = chi.Walk(router, func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { + _ = chi.Walk(r, func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { key := method + " " + route if _, ok := want[key]; ok { want[key] = true diff --git a/cmd/pathwayd/main_test.go b/cmd/pathwayd/main_test.go index d0d00c8..8b44dfe 100644 --- a/cmd/pathwayd/main_test.go +++ b/cmd/pathwayd/main_test.go @@ -14,7 +14,11 @@ import ( // newTestRouter builds the pathwayd router with an in-memory store // (nil persistor). Closes R-005 for pathwayd: 9 routes mounted with // no router-level test prior to this file. -func newTestRouter(t *testing.T) http.Handler { +// +// Returns chi.Router (not http.Handler) so chi.Walk works without a +// type assertion that would panic if a future refactor wraps the +// router in plain net/http middleware. +func newTestRouter(t *testing.T) chi.Router { t.Helper() h := &handlers{store: pathway.NewStore(nil)} r := chi.NewRouter() @@ -36,8 +40,7 @@ func TestRoutesMounted(t *testing.T) { "GET /pathway/stats": "", } got := map[string]bool{} - router := r.(chi.Router) - _ = chi.Walk(router, func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { + _ = chi.Walk(r, func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { got[method+" "+route] = true return nil }) diff --git a/scripts/playbook_lift.sh b/scripts/playbook_lift.sh index bc33376..04fc251 100755 --- a/scripts/playbook_lift.sh +++ b/scripts/playbook_lift.sh @@ -68,7 +68,16 @@ if ! curl -sS http://localhost:11434/api/tags | jq -e --arg m "$EFFECTIVE_JUDGE" echo "[lift] judge model '$EFFECTIVE_JUDGE' not loaded in Ollama — pull it first" exit 1 fi -echo "[lift] judge resolved to: $EFFECTIVE_JUDGE (from ${JUDGE_MODEL:+env}${JUDGE_MODEL:-config})" +# Compute a single string for "where did the judge come from" so the +# log line + the markdown report don't have to chain :+/:- substitutions +# (those silently fuse "env JUDGE_MODEL" + the value into "env JUDGE_MODELx" +# without a separator — the bug Opus caught on lift_001's report). +if [ -n "$JUDGE_MODEL" ]; then + JUDGE_SOURCE="env JUDGE_MODEL=${JUDGE_MODEL}" +else + JUDGE_SOURCE="config [models].local_judge" +fi +echo "[lift] judge resolved to: $EFFECTIVE_JUDGE (from $JUDGE_SOURCE)" echo "[lift] building binaries..." go build -o bin/ ./cmd/storaged ./cmd/catalogd ./cmd/ingestd ./cmd/queryd \ @@ -207,16 +216,24 @@ id,name,role CSVEOF INGEST_RESP="$(curl -sS -F "file=@$PROBE_CSV" "http://127.0.0.1:3110/v1/ingest?name=lift_sql_probe")" echo "[lift] ingest response: $INGEST_RESP" -# queryd refresh_every=1s — give it a couple ticks to discover the new manifest. -sleep 2.5 -SQL_RESP="$(curl -sS -X POST http://127.0.0.1:3110/v1/sql \ - -H 'content-type: application/json' \ - -d '{"sql":"SELECT COUNT(*) FROM lift_sql_probe"}')" -PROBE_COUNT="$(echo "$SQL_RESP" | jq -r '.rows[0][0] // "ERR"' 2>/dev/null || echo "ERR")" +# Poll up to 5s for queryd to discover the manifest. refresh_every=1s +# is a lower bound; under load or slow disks the manifest may not be +# visible in a fixed sleep, which would 4xx the SQL probe spuriously. +PROBE_COUNT=ERR +SQL_RESP="" +deadline=$(($(date +%s) + 5)) +while [ "$(date +%s)" -lt "$deadline" ]; do + SQL_RESP="$(curl -sS -X POST http://127.0.0.1:3110/v1/sql \ + -H 'content-type: application/json' \ + -d '{"sql":"SELECT COUNT(*) FROM lift_sql_probe"}')" + PROBE_COUNT="$(echo "$SQL_RESP" | jq -r '.rows[0][0] // "ERR"' 2>/dev/null || echo "ERR")" + [ "$PROBE_COUNT" = "3" ] && break + sleep 0.25 +done if [ "$PROBE_COUNT" = "3" ]; then echo "[lift] ✓ SQL surface probe passed (rowcount=3)" else - echo "[lift] ✗ SQL surface probe FAILED (got: $SQL_RESP)" + echo "[lift] ✗ SQL surface probe FAILED after 5s (got: $SQL_RESP)" exit 1 fi @@ -275,7 +292,7 @@ generate_md() { # Playbook-Lift Reality Test — Run ${RUN_ID} **Generated:** ${gen_at} -**Judge:** \`${EFFECTIVE_JUDGE}\` (Ollama, resolved from ${JUDGE_MODEL:+env JUDGE_MODEL}${JUDGE_MODEL:-config [models].local_judge}) +**Judge:** \`${EFFECTIVE_JUDGE}\` (Ollama, resolved from ${JUDGE_SOURCE}) **Corpora:** \`${CORPORA}\` **Workers limit:** ${WORKERS_LIMIT} **Queries:** \`${QUERIES_FILE}\` (${total} executed) @@ -339,7 +356,7 @@ MDEOF results land in one corpus, the matrix layer's purpose isn't being tested. Check per-corpus distribution in the JSON. 5. **Judge resolution.** This run used \`${EFFECTIVE_JUDGE}\` from - ${JUDGE_MODEL:+env JUDGE_MODEL override}${JUDGE_MODEL:-the lakehouse.toml [models].local_judge tier}. + ${JUDGE_SOURCE}. Bumping the judge for run #N+1 means editing one line in lakehouse.toml. ## Next moves diff --git a/scripts/staffing_workers/main.go b/scripts/staffing_workers/main.go index 615199c..e981747 100644 --- a/scripts/staffing_workers/main.go +++ b/scripts/staffing_workers/main.go @@ -274,6 +274,12 @@ func main() { ) flag.Parse() + // An empty prefix collides cross-corpus — exactly the bug the + // flag exists to prevent. Force callers to be explicit. + if *idPrefix == "" { + log.Fatalf("--id-prefix cannot be empty (use 'w-', 'e-', etc. — IDs collide cross-corpus without one)") + } + hc := &http.Client{Timeout: 5 * time.Minute} ctx := context.Background()